Skip to content

Fix #1038: cancel in-flight chapter-video downloads on delete/replace#1073

Open
sampjvv wants to merge 4 commits into
mainfrom
1038-deleted-video-doesnt-stop-loading
Open

Fix #1038: cancel in-flight chapter-video downloads on delete/replace#1073
sampjvv wants to merge 4 commits into
mainfrom
1038-deleted-video-doesnt-stop-loading

Conversation

@sampjvv

@sampjvv sampjvv commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Closes #1038.

Problem

While a chapter video is downloading ("Get video → Load/Save to project"), deleting or replacing it doesn't stop the download. The fetch runs to completion in the background and writes the bytes to files/, resurrecting the video the user just deleted.

Root cause

downloadVideoToProject / downloadVideoToSessionCache (videoDownloadUtils.ts) fetch the whole LFS object via authApi.downloadLFSFile with no AbortController, then writeFile unconditionally. The in-flight tracker was a plain Set (no way to cancel), the delete/replace handlers never signalled it, and the download progress was cancellable: false.

Fix

  • In-flight tracker → Map<string, AbortController>. beginVideoOperation returns the controller; new abortVideoOperation() cancels it.
  • Both downloaders accept a signal, pass it to downloadLFSFile (its type already accepts one), and guard the write with signal.aborted — so even if the downloader ignores the signal, the deleted file is never rewritten.
  • deleteVideoFile and the metadata replace path call abortVideoOperation(...) before removing the file.
  • The "Saving/Loading video…" progress is now cancellable and wired to abort (a user cancel no longer pops an error).

Scope decision

The streamed <video> path is intentionally unchanged. It self-limits — the browser buffers a chunk, goes idle, and releases on GC — so there's no equivalent background-load leak to fix there. (Verified by instrumenting the element's networkState/buffered live.)

Verification

  • Unit (6): the cancellation contract — abort propagates to the signal, re-begin supersedes without leaking, ops scoped per workspace+video.
  • Integration (3): drives the real downloadVideoToProject against a temp workspace + LFS pointer with a mocked, abortable downloadLFSFile:
    • control — no abort → bytes are written (harness is genuine);
    • delete + abort mid-download → file stays deleted, even when the downloader ignores the signal (the core guarantee);
    • signal-honoring downloader → also stays deleted.
  • Frontier source confirmed (genesis-ai-dev/frontier-authentication): downloadLFSFile forwards the AbortSignal through downloadLFSObject (refcounted) into the underlying fetch, so the network request is genuinely cancelled — a shared object download stops once the last waiter aborts.
  • Live GUI: in a synced, stream-only project, started "Get video → Save to project" and deleted the video mid-download — the file was not resurrected.
  • tsc --noEmit + eslint clean. The targeted videoDownloadUtils suite is 9/9.

Note: the full local extension-test run currently hangs in an unrelated, pre-existing E2E test (E2E: Timer Management / Timer During Sync, project-swap timers) that runs before — and independently of — these video tests; the same changes passed the full suite (1202 passing / 0 failing) before rebasing onto current main. CI runs the authoritative suite.

🤖 Generated with Claude Code

sampjvv and others added 2 commits June 26, 2026 14:20
…placed

A chapter-video "Get/Save video" download fetched the whole LFS object and
wrote it to files/ unconditionally, with no AbortController (the progress was
even marked cancellable: false). Deleting or replacing the video mid-download
didn't stop the fetch, so when it finished it wrote the bytes back —
resurrecting the file the user just deleted.

- videoDownloadUtils: the in-flight op tracker now holds an AbortController per
  video; beginVideoOperation returns it and a new abortVideoOperation() cancels
  it. Both downloaders take a signal, pass it to downloadLFSFile, and guard the
  write with `signal.aborted` so an aborted op never writes bytes back — even if
  the downloader ignores the signal.
- deleteVideoFile and the metadata replace path abort the op before deleting.
- The download progress notification is now cancellable and wired to abort.

Scope: the streamed <video> path is left as-is — it self-limits (the browser
stops buffering and releases on its own), so it has no equivalent background
load to leak.

Tests: 6 unit (cancellation contract) + 3 integration driving the real
downloadVideoToProject against a temp workspace + LFS pointer with a mocked,
abortable downloadLFSFile — asserting a deleted file is not rewritten even when
the downloader ignores the signal.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@LeviXIII LeviXIII left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple of places and scenarios where the cancel button is missing. If we try to save or load the video to the project from pressing the Show Video button, the cancel button is missing in that flow. There is also the scenario where the video is missing. If we try to download it, it will try to retrieve it, but we should be able to cancel from there as well.

LeviXIII and others added 2 commits July 2, 2026 12:43
Review follow-up: the "Show Video" save/load flow and the missing-video
"Download & play" button both post downloadVideoFile, whose progress toast
was cancellable: false with no abort signal — the one video download left
without a cancel button.

- downloadVideoFile: cancellable progress; the fetch is registered via
  beginVideoOperation so a cancel OR a delete/replace mid-download aborts
  it (previously the delete-abort only covered the sidebar flow); signal
  threaded into downloadLFSFile; cancel resets the player quietly
  ("Download cancelled." + Retry) instead of an error toast.

- Post-write abort check in all three write sites (handler + both nav
  utils): a cancel that lands while the verified bytes are being WRITTEN
  (a large video's write takes real time; the pre-write check can't see
  it) now reverts files/ to the exact pre-download pointer stub (or
  deletes it if none existed) and drops the session-cache entry. The
  check is the last statement inside the progress callback, so any
  cancel that can be clicked is honored — no window where a cancel is
  silently ignored.

- Tests: two deterministic write-race cases (abort fires during the fs
  write) — stub restored byte-identical / files/ removed when no stub
  pre-existed. 11/11 videoDownloadUtils tests green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@sampjvv

sampjvv commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

@LeviXIII Both missing-cancel scenarios are fixed in f69fe30. They turned out to share one handler: the Show Video → "Save to project"/"Load for session" buttons and the missing-video "Download & play" button all post downloadVideoFile, whose progress toast was the one video download still cancellable: false.

  • That progress is now cancellable, and the fetch is registered in the same in-flight registry as the sidebar flow — so a cancel or deleting/replacing the video mid-download aborts it (the delete-abort previously only covered the sidebar downloads). Cancel resets the player quietly ("Download cancelled." + Retry), no error toast.
  • Also sealed the write race: a cancel that lands while the verified bytes are being written now reverts files/ to the exact pre-download pointer stub (or removes the file/cache entry if none existed). The abort check brackets the write in all three write sites (handler + both nav utils), and it's the last statement inside the progress callback — so any cancel that can physically be clicked is honored; no window where a cancel is silently ignored and a video stays saved.
  • Two new deterministic write-race tests (abort fired during the fs write): stub restored byte-identical / file removed when no stub pre-existed. 11/11 videoDownloadUtils tests green.

Verified in the app: stream-only → Show Video → Save to project, and stream-and-save → Download & play — both show Cancel; cancelling mid-download leaves files/ as the pointer stub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Deleted video doesn't stop loading

2 participants