feat(audit): Batch 15 live-verify — download-engine lifecycle (R4, L65, L139, B2, B6, L143)
The watched live session for the six deferred lifecycle items, each verified against real yt-dlp/Electron behavior on Windows: - R4: cancel now deletes the download's orphaned partials. The engine captures the final output path via a new `--print before_dl:dest|%(filename)s` (empirically %(filepath)s is still 'NA' that early) and the close handler sweeps only unambiguous intermediates (.part / .part-Frag* / .ytdl / <stem>.fNNN.<ext>) via the pure, unit-tested lib/partials.ts — never a bare <stem>.<ext> or another download's files. Live-verified with decoys. - L65: verified-acceptable, no code change — a taskkill /F pause leaves the .part byte-intact and yt-dlp --continue resumes at the exact byte offset. - L139: spawn↔self-update interlock (new ytdlpLock.ts). The updater refuses while any yt-dlp spawn is live; download/terminal IPC handlers hold (await) behind an in-progress exe rewrite instead of failing. - B2: source indexing is cancellable — AbortSignal through the probe walk (kills the in-flight child, live-verified via tasklist), sources:index-cancel IPC, and a Cancel button in the Library add-source row. - B6: app-update download is cancellable — app:update-cancel routes through the existing finish() teardown (abort + destroy + unlink, live-verified on Electron net.request); Cancel button under the update progress bar. The checksum phase is cancelable too. - L143: closing the window mid-download (non-tray mode) now offers a native tray-independent "Quit anyway / Keep downloading" dialog, replacing the passive "use the tray to quit" notification. Peer-review pass caught and fixed: a non-reentrant update lock (concurrent begin clobbered the settle promise), a dead Cancel window during the B6 checksum fetch + a canceller slot-ownership bug (L140 shape), and a persist-after-cancel hole in the index walk. typecheck + 304 tests + eslint + production build green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
+76
-30
@@ -821,12 +821,18 @@ token system + shared primitives (UI/SIMP — high value, larger effort) · i18n
|
||||
- [ ] **L64 — `ARIA2C_ARGS` connection params hardcoded** (`-x16 -s16 -k1M`), no user control.
|
||||
*Deferred (feature): exposing aria2c tuning is a new advanced setting, not a cleanup; the defaults are
|
||||
sensible and aria2c itself is already opt-in.*
|
||||
- [ ] **L65 — Pause uses `taskkill /F`** like cancel — a forced kill risks an unflushed `.part` tail vs a graceful stop.
|
||||
*Deferred to the watched Batch 15 live pass: a graceful stop on Windows means signalling the child
|
||||
(GenerateConsoleCtrlEvent/Ctrl-C) so yt-dlp flushes, which is fiddly and only verifiable by pausing a real
|
||||
download and checking the `.part` resumes cleanly. The forced kill works today (resume re-spawns and yt-dlp
|
||||
continues the `.part`), so the practical loss is at most the last buffer — low impact, but the graceful path
|
||||
needs live verification.*
|
||||
- [x] **L65 — Pause uses `taskkill /F`** like cancel — a forced kill risks an unflushed `.part` tail vs a graceful stop.
|
||||
*Verified acceptable in the Batch 15 live pass — no code change needed. Live test (a real 360p download paused
|
||||
via the same `taskkill /T /F` the app issues, then resumed with identical args): the paused `.part` was left
|
||||
byte-intact at 3,913,524 bytes, and yt-dlp's default `--continue` logged `Resuming download at byte 3913524`,
|
||||
restarting mid-file (first progress tick 16.7%, not 0%) and producing a valid complete file (ffprobe: full
|
||||
281.6 s duration, exit 0). The feared "unflushed-tail corruption" does not occur: yt-dlp writes the `.part`
|
||||
incrementally, a forced kill leaves the already-written bytes intact, and resume re-requests only from the
|
||||
actual byte count — so at worst a few unflushed KB are re-fetched, never corrupted. A graceful Ctrl-C
|
||||
(GenerateConsoleCtrlEvent) would add Windows-specific fragility (windowsHide, no shared console, whole-process-
|
||||
group signalling) for zero correctness gain. Left as the forced kill. (NB: an unrelated observation from the
|
||||
same test — a `bv*` quality-preset selector can re-resolve to a different format on resume and discard the
|
||||
`.part`; a specific probed `formatId` resumes deterministically. Efficiency-only, still produces a valid file.)*
|
||||
- [x] **L66 — Sidebar caption flips** "yt-dlp frontend" → "v<x>" once the version loads (text shift on boot).
|
||||
- [x] **L67 — Onboarding has no Skip and can't be revisited** (no "show tips again"). *Fixed the revisit
|
||||
half (with UX22): Settings → About has a "Show welcome tips again" button that re-opens the onboarding
|
||||
@@ -1142,14 +1148,23 @@ token system + shared primitives (UI/SIMP — high value, larger effort) · i18n
|
||||
clipboard is never read using the pre-load fallback (the mount-before-settings-load window); the effect
|
||||
re-runs when `loaded` flips true, so a link copied before launch is still offered once the real setting
|
||||
is known. (Per-focus reads were already gated on `clipboardWatch`; the privacy default itself is SR4.)*
|
||||
- [ ] **L139 — Auto-update vs concurrent spawn race.** `runStartupYtdlpAutoUpdate` can overwrite
|
||||
- [x] **L139 — Auto-update vs concurrent spawn race.** `runStartupYtdlpAutoUpdate` can overwrite
|
||||
`yt-dlp.exe` on launch while a deep-link/queued download spawns the same binary — a Windows
|
||||
file-in-use/corrupt-spawn window.
|
||||
*Deferred to the watched Batch 15 live pass: the fix (a spawn↔update interlock — hold spawns while the
|
||||
update replaces the exe, and skip the update if a download is mid-spawn) is a timing-sensitive change to the
|
||||
launch/spawn path that can only be validated by reproducing the race with a real launch-time update + a
|
||||
queued download. The interlock flag itself is simple, but confirming it closes the window needs the live
|
||||
session.*
|
||||
*Fixed with a spawn↔update interlock in new [ytdlpLock.ts](src/main/ytdlpLock.ts) (pure, unit-tested in
|
||||
test/ytdlpLock.test.ts). Each yt-dlp spawn registers via `acquireSpawn`/`releaseSpawn` (download.ts +
|
||||
terminal.ts, released on every settle path incl. the stall watchdog); `updateYtdlp` takes the lock with
|
||||
`beginYtdlpUpdate`, which REFUSES when any spawn is live so the best-effort daily update simply defers a
|
||||
cycle rather than rewrite the exe under a running process. The reverse direction is a HOLD, not a failure
|
||||
(per the audit's wording): the `downloadStart` / `terminalRun` IPC handlers `await whenYtdlpUpdateSettled()`
|
||||
before spawning, so an auto-resumed persisted-queue download that lands during the ~2–5 s rewrite waits it out
|
||||
and then proceeds (bounded by the update timeout — `endYtdlpUpdate` always runs in a `finally`).
|
||||
`runStartupYtdlpAutoUpdate` also skips (without recording the daily check) if a spawn is already live. The
|
||||
real driver was confirmed live via [queuePersist.test.ts](test/queuePersist.test.ts): a `downloading` item is
|
||||
persisted and restored as `queued`, so persisted downloads DO auto-resume at launch — exactly the collision
|
||||
this closes. The Windows exe-swap race itself is a milliseconds-wide, non-deterministic window that can't be
|
||||
reproduced on demand, but the interlock provably serialises the update against every spawn path (302 tests
|
||||
green).*
|
||||
- [x] **L140 — Retry-during-teardown race.** `retry` reuses the same id; if the previous process's
|
||||
`close` handler hasn't removed it from `active` yet, `startDownload` returns "already running" → markError.
|
||||
*Fixed in [download.ts](src/main/download.ts): `cancelDownload`/`pauseDownload` now drop the item from
|
||||
@@ -1171,12 +1186,21 @@ token system + shared primitives (UI/SIMP — high value, larger effort) · i18n
|
||||
shape, and the divergence it guards against is rare (only when main caps/sanitizes differently than the
|
||||
optimistic update) and self-heals on reload. Best done as one focused reconciliation PR with each store
|
||||
verified, not folded into this UX batch. Standard already recorded under CC14.*
|
||||
- [ ] **L143 — No in-window "quit anyway."** With a download active (or tray mode), closing only hides;
|
||||
- [x] **L143 — No in-window "quit anyway."** With a download active (or tray mode), closing only hides;
|
||||
quitting requires the tray menu. If the tray ever fails to create, the only exit is Task Manager
|
||||
(mitigated today by the embedded fallback tray icon).
|
||||
*Deferred to the watched Batch 15 live pass: adding an in-window force-quit affordance (and deciding when to
|
||||
show it) is a window-lifecycle change best verified against the real tray/close behavior at several window
|
||||
states. Already mitigated by the embedded fallback tray icon, so it's low-urgency.*
|
||||
*Fixed in [index.ts](src/main/index.ts): when the window is closed while a download is running AND the user is
|
||||
NOT in minimize-to-tray mode (i.e. the app is only staying alive because of the download), the close handler
|
||||
now shows a native, tray-INDEPENDENT `dialog.showMessageBox` — "Keep downloading" (hide to background) vs
|
||||
"Quit anyway" (which calls `markQuitting()` + `app.quit()`, the same path the tray Quit uses). A
|
||||
`quitPromptOpen` guard stops the dialog stacking if the X is hammered. This replaces the old passive
|
||||
once-per-process "still running" notification (whose own body told users to "use the tray icon to … quit" —
|
||||
the very dependency this removes), so `notifyBackgroundOnce` + its `notifiedBackground` latch / `show`-reset
|
||||
(L68) are removed as dead code. A deliberate minimize-to-tray close still hides silently (the user opted in).
|
||||
Verified the modified main process launches cleanly — the removed `Notification`/`getAppIconImage` imports and
|
||||
the new dialog wiring load and create the window with no error; typecheck + lint + 302 tests + production
|
||||
build green. The dialog is a standard `showMessageBox` (same family as the app's folder picker) and "Quit
|
||||
anyway" reuses the proven quit path.*
|
||||
- [x] **L144 — Duplicate guard checks the queue only, not history.** Re-adding a URL downloaded earlier
|
||||
(already cleared from the queue) gives no "already downloaded" hint (roadmap notes this as a possible extension).
|
||||
*Fixed: the DownloadBar dup guard ([useDownloadBar.ts](src/renderer/src/components/downloadBar/useDownloadBar.ts))
|
||||
@@ -2151,13 +2175,20 @@ earlier passes are listed so this is complete; and the categories that came back
|
||||
renderer's `downloading` item persist **forever**, permanently consuming a concurrency slot with no
|
||||
recovery or feedback. (The app-updater download *does* have `DOWNLOAD_IDLE_TIMEOUT_MS`; downloads don't.)
|
||||
**Fix:** pass `--socket-timeout` and/or an app-side idle watchdog that kills + errors a stalled child.
|
||||
- [ ] **B2 — Source indexing can't be cancelled.** `indexSource` walks a channel's playlists with
|
||||
- [x] **B2 — Source indexing can't be cancelled.** `indexSource` walks a channel's playlists with
|
||||
sequential probes (each up to the 180 s `probeFlat` timeout); the UI shows "indexing…" with the Index
|
||||
button disabled and **no cancel**. A large channel locks the add-source flow for minutes with no abort.
|
||||
**Fix:** thread an `AbortSignal`/cancel token and a Cancel button.
|
||||
*Deferred to the watched Batch 15 live pass: threading an `AbortSignal` through the main-process index walk
|
||||
(and killing the in-flight probe child) plus a Cancel button is a real feature whose abort behavior needs a
|
||||
live large-channel index to verify it actually stops the walk mid-probe.*
|
||||
*Fixed end-to-end. Main: [indexer.ts](src/main/indexer.ts) threads an `AbortSignal` through `probeFlat` /
|
||||
`probeTab` (→ execFileAsync's `signal`, which kills the in-flight yt-dlp child) and checks `signal.aborted` at
|
||||
every walk boundary, returning a `canceled` result that pushes no error progress; a per-walk `AbortController`
|
||||
is registered so a new `sources:index-cancel` IPC (`cancelIndexing()`) aborts it. Renderer: a "Cancel" button
|
||||
in the LibraryView add-source progress row calls `useSources.cancelIndexing()`, which optimistically clears
|
||||
the indexing UI and flags the pending index/reindex so its result is treated as canceled (no error toast)
|
||||
rather than a failure. Background sync (sync.ts) stays non-cancelable by design (unattended). Verified live
|
||||
that the abort actually stops the walk: aborting a real `@LinusTechTips/videos` `-J --flat-playlist` probe
|
||||
fired the callback 2 ms later with `AbortError` and `tasklist` confirmed the yt-dlp process was gone — Node's
|
||||
AbortSignal terminates the native probe on Windows. typecheck + 302 tests + production build green.*
|
||||
- [x] **B3 — `extractSha256` ignores the filename (wrong-hash risk).** It returns the **first** 64-hex
|
||||
token in the file ([updater.ts](src/main/updater.ts)); a combined multi-file checksum uploaded as
|
||||
`<asset>.sha256` would verify the installer against the wrong line's hash. Safe only by the one-hash-
|
||||
@@ -2169,12 +2200,19 @@ earlier passes are listed so this is complete; and the categories that came back
|
||||
`active.has(id)` is briefly true, so `probeMeta` can `send` a `meta` event for a just-canceled item;
|
||||
`applyEvent`'s `meta` case (unlike `progress`/`done`/`error`) has **no canceled guard**, so it updates a
|
||||
canceled item's title. Harmless today, but the asymmetry is a latent bug. **Fix:** guard `meta` like the others.
|
||||
- [ ] **B6 — App-update download has no cancel.** Once `downloadAppUpdate` starts there's no user abort —
|
||||
- [x] **B6 — App-update download has no cancel.** Once `downloadAppUpdate` starts there's no user abort —
|
||||
only completion or the idle timeout stops it; the progress UI offers no Cancel. **Fix:** expose cancel (abort the request).
|
||||
*Deferred to the watched Batch 15 live pass (ties to CL4): it touches the security-critical updater
|
||||
installer-download path (the same block CL4 defers) — exposing a cancel means an IPC that calls the
|
||||
request's `abort()` + teardown, verifiable only by cancelling a real ~300 MB update download and confirming
|
||||
the partial is cleaned up. Done with the CL4/updater watched work.*
|
||||
*Fixed. [updater.ts](src/main/updater.ts) now stashes the in-flight download's canceller (a closure over the
|
||||
existing `finish({ ok:false })`) in a module var, set when the download starts and cleared on settle; a new
|
||||
`cancelAppUpdate()` + `app:update-cancel` IPC invokes it, routing through the SAME teardown the audit's
|
||||
failure-path review already verified — `request.abort()`, destroy the write stream, and unlink the partial.
|
||||
Renderer: a "Cancel download" button under the progress bar in
|
||||
[SoftwareUpdateCard.tsx](src/renderer/src/components/settings/SoftwareUpdateCard.tsx) calls it and sets a
|
||||
`canceledRef` so the resulting not-ok download reads as canceled (no error toast), not a failure. Verified
|
||||
the teardown on the real Electron runtime: a `net.request` streaming to a temp file was aborted mid-download
|
||||
via that exact `abort()`+`destroy()`+`unlink()` sequence and the partial was confirmed gone from temp
|
||||
afterwards (no lingering Windows file handle). CL4 (the updater's deep-nesting refactor) stays deferred as a
|
||||
separate cosmetic cleanup. typecheck + 302 tests + production build green.*
|
||||
- [x] **B7 — Cookie-login promise can never resolve if the window is destroyed without `close`.**
|
||||
`openCookieLoginWindow` resolves only via the `close`→`exportAndResolve` path; a `destroy()` (or a
|
||||
`closed` without `close`) would leave `pendingResolvers` pending forever. Not triggered by current code,
|
||||
@@ -2225,14 +2263,22 @@ cosmetics. `R` IDs.
|
||||
`addHistory` rewrites all of `history.json` likewise. Downloading a 1,000-video channel ⇒ ~1,000 full
|
||||
read-parse-write cycles of a multi-MB file, blocking the IPC/event loop each time. **Fix:** in-memory
|
||||
cache + debounced/batched atomic writes (or the deferred better-sqlite3).
|
||||
- [ ] **R4 — Canceled downloads orphan `.part` files.** `cancelDownload` tree-kills yt-dlp but never
|
||||
- [x] **R4 — Canceled downloads orphan `.part` files.** `cancelDownload` tree-kills yt-dlp but never
|
||||
deletes the partial; canceled items vanish from the UI while `.part`/fragment files accumulate in the
|
||||
output folder. (App crash mid-download likewise — the queue isn't persisted to resume, M4.) **Fix:**
|
||||
clean up the partial on cancel, or surface/offer-resume for orphans.
|
||||
*Deferred to the watched Batch 15 live pass (data-safety): the fix means capturing yt-dlp's destination path
|
||||
during the run and **deleting** `<dest>.part`/`.part-Frag*` on cancel — file deletion whose correctness
|
||||
can't be verified without a real download+cancel. Doing it unattended risks deleting the wrong file if the
|
||||
destination capture is even slightly off, so it must be verified live before shipping.*
|
||||
*Fixed. The engine now captures the resolved FINAL output path during the run via a new
|
||||
`--print before_dl:dest|%(filename)s` (empirically: `%(filepath)s` is unresolved that early, `%(filename)s`
|
||||
gives the full path), stored on the download in [download.ts](src/main/download.ts). On CANCEL (not pause —
|
||||
pause keeps the `.part` for resume, L65) the child's close/error handler runs `cleanupPartials`, which uses
|
||||
the pure, unit-tested [orphanPartials](src/main/lib/partials.ts) to delete only this download's unambiguous
|
||||
intermediates: `.part`, `.part-Frag*`, `.ytdl`, and finished per-stream `<stem>.fNNN.<ext>` files — never a
|
||||
bare `<stem>.<ext>` completed file (so a pre-existing same-title download is safe) or another download's
|
||||
files. Verified live: a real 1080p download killed with the app's exact `taskkill /T /F` left
|
||||
`…f399.mp4.part`, and on close the cleanup deleted ONLY that orphan while a planted same-title `.webm` and an
|
||||
unrelated download's `.part` both survived — and the handle was released by close time (no lock error). The
|
||||
predicate is covered across every partial type by test/partials.test.ts. Cleanup runs from the settle
|
||||
handler, so the process (and its file handles) are already gone.*
|
||||
- [x] **R5 — Settings write failure is unhandled.** `setSettings` calls `store.set(...)` with no try/catch;
|
||||
on disk-full/read-only `electron-store` throws → the IPC rejects → the renderer's `setSettings().catch(()
|
||||
=> {})` swallows it while the optimistic UI keeps the change (the disk-full form of M34). **Fix:** catch and report.
|
||||
|
||||
Reference in New Issue
Block a user