feat(audit): Batch 15 — download-engine lifecycle (safe part; rest live-verify)

This is the batch the run reserves for a WATCHED LIVE SESSION, so only the safe, non-file-deleting item is implemented here; the rest are staged with per-item live-verify rationale in CODE-AUDIT.md. Landed: - L157: removeSource cancels its in-flight downloads. removeSource emits a `sourceRemoved` event on the coordinator bus (C2); the downloads store cancels every active (downloading/queued) item whose mediaItemId starts with `${sourceId}:` — MediaItem ids are `${sourceId}:${videoId}` (indexerCore), so the prefix reliably identifies the source's downloads. Reuses the proven cancel() path (process kill + pump). Type/lint/test-green; process-kill worth a live confirm. Resolved by prior work: - UX11: M4 persists the queue, so a scheduled item fires on next launch once due — no longer "never fires if quit". - UX20: L68 resets notifiedBackground on window show, so every close-to-tray re-fires the "still running in the tray" notice, not just the first. Deferred to the watched live pass (each needs a real download to verify): R4 (delete .part on cancel — file-deletion data-safety), L65 (graceful Ctrl-C pause), L139 (spawn↔auto-update interlock — timing race), B2 (index AbortSignal + Cancel), B6 (app-update cancel — ties to CL4's deferred updater), L143 (in-window quit-anyway). Verified: typecheck (node+web) + 279 tests + eslint + production build green; touched files prettier-clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-01 17:41:48 -04:00
parent 0f784847a3
commit 4d1c33a081
4 changed files with 95 additions and 5 deletions
@@ -226,6 +226,34 @@ software-rendering workaround — external driver dependency), and **L104** (His
 L161 layout rework + live verification; the 500-cap bounds it). All green: typecheck (node+web) + 279 tests +
 eslint + production build; touched files prettier-clean.

+**Session 2026-07-01 pass 12 (Batch 15 — download-engine lifecycle):** the batch the run's policy reserves for
+a **watched live session** — so only the safe, non-file-deleting item was implemented, and the rest are staged
+with per-item live-verify rationale. **Landed: L157** — `removeSource` now emits a `sourceRemoved` bus event
+(C2) and the downloads store cancels every active item whose `mediaItemId` prefix (`${sourceId}:`, per
+indexerCore) matches, reusing the proven `cancel()` path (implemented + type/lint/test-green; process-kill
+worth a live confirm). **Resolved by prior work: UX11** (M4 persists the queue, so a scheduled item fires on
+next launch once due — no longer "never fires if quit"), **UX20** (L68 resets `notifiedBackground` on window
+show, so every close-to-tray re-fires the "still running" notice, not just the first). **Deferred to the
+watched live pass with rationale — each needs a real download to verify:** **R4** (delete `.part` on cancel —
+file-deletion data-safety), **L65** (graceful Ctrl-C pause vs `taskkill /F`), **L139** (spawn↔auto-update
+interlock — timing race), **B2** (index `AbortSignal` + Cancel), **B6** (app-update cancel — ties to CL4's
+deferred updater), **L143** (in-window quit-anyway). All green: typecheck (node+web) + 279 tests + eslint +
+production build; touched files prettier-clean.
+
+---
+
+## Run wrap-up (2026-07-01 — Batches 9–15 unattended)
+
+All seven remaining audit batches were worked as stacked branches (`audit/batch-9-surface-primitives` →
+`audit/batch-15-download-lifecycle`), each: implement → self peer-review → verify (typecheck node+web · tests ·
+eslint · prod build · prettier) → push. The contained, safe, testable items landed; three categories were
+**honestly deferred with documented rationale** rather than forced unattended: (1) large architectural
+refactors the release gate already puts post-1.0 (CC9 zod, CC10/CC12/CC14 persistence/reorg, CL4 updater),
+(2) subjective-visual items needing an eyeball (UI3/UI12/UI16/UX7/UX18/UI21/UX26 …), and (3) the download-engine
+lifecycle changes that require a live download to verify (Batch 15's R4/L65/L139/B2/B6/L143). **Two items need
+maintainer awareness:** SR4 flips `clipboardWatch` off-by-default for new installs (reversible), and Batch 15's
+deferred set + L157's cancel-on-remove want a watched live smoke-test before merge.
+
 **Deliberately deferred** (need live-app/visual verification or are larger refactors the
 audit itself defers to 1.x): the wire-or-cut features (M5/M6/UX1), the god-file/store
 refactors (C1, C2, H1), the SIMP* helpers + shared-UI-token work, the CC* consolidations,
@@ -794,6 +822,11 @@ token system + shared primitives (UI/SIMP — high value, larger effort) · i18n
  *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] **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
@@ -1112,6 +1145,11 @@ token system + shared primitives (UI/SIMP — high value, larger effort) · i18n
 - [ ] **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.*
 - [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
@@ -1136,6 +1174,9 @@ token system + shared primitives (UI/SIMP — high value, larger effort) · i18n
 - [ ] **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.*
 - [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))
@@ -1188,8 +1229,14 @@ token system + shared primitives (UI/SIMP — high value, larger effort) · i18n
  `aria-label` is gone (aria-checked conveys it).*
 - [x] **L156 — `datetime-local` schedule has no `min`.** Past times are selectable, then silently download
  now (extends L57). Add `min={now}`.
- [ ] **L157 — `removeSource` doesn't cancel in-flight downloads.** Removing a source while its videos are
+- [x] **L157 — `removeSource` doesn't cancel in-flight downloads.** Removing a source while its videos are
  downloading lets them finish into the removed source's folders; `markDownloaded` then no-ops on the gone item.
+  *Fixed: `removeSource` now emits a `sourceRemoved` event on the coordinator bus (C2); the downloads store
+  subscribes and cancels every active (downloading/queued) item whose `mediaItemId` starts with `${sourceId}:`
+  — MediaItem ids are `${sourceId}:${videoId}` (indexerCore), so the prefix reliably identifies the source's
+  downloads. Reuses the proven `cancel()` path (process kill + pump). **Live-smoke pending** (per the
+  mark-done convention): implemented + type/lint/test-green, but the actual process-kill-on-remove is worth a
+  watched confirm in the Batch 15 live pass.*
 - [x] **L158 — Drag highlight flickers.** `onDragLeave={() => setDragActive(false)}` fires when the cursor
  crosses child elements (no enter/leave depth counter), so the dashed-outline blinks during a drag.
  *Fixed: [useDownloadBar.ts](src/renderer/src/components/downloadBar/useDownloadBar.ts) now tracks a
@@ -1587,9 +1634,13 @@ root cause is already filed.
  a non-portal bottom-center stack that any store/component can raise (now used for UX6's file-action errors).
  Together this closes **UI25**.*
 - [x] **UX10 — Re-download silently changes quality** (H5) — a confusing "I asked for 720p, got Best." *Resolved by H5 (re-download stores `formatId`/`formatHasAudio` and strips compound quality labels).*
- [ ] **UX11 — Scheduled downloads silently never fire if the app is quit** (M4). The hint is easy to
+- [x] **UX11 — Scheduled downloads silently never fire if the app is quit** (M4). The hint is easy to
  miss; the user returns to find nothing happened. **Fix:** warn at schedule time that it requires the
-  app running (or persist + relaunch).
+  app running (or persist + relaunch). *Resolved by M4 (the "persist + relaunch" option the fix names): the
+  queue is now persisted, so a `saved`+`scheduledFor` item survives a quit and the App promoter tick fires it
+  on the next launch once its time has passed (the store normalizes/promotes due items). So a scheduled
+  download no longer "never fires" if quit — it fires when due, or on the next launch if that time already
+  passed. (Live-smoke of the quit→relaunch cycle noted in M4.)*
 - [x] **UX12 — Terminal is a dead-end when custom commands are off** (L18). The nav item is always
  visible; clicking it shows a gate pointing to a setting on another screen. **Fix:** hide/disable the
  nav item, or let the gate enable the setting inline. *Fixed (both fixes): the nav item is already hidden
@@ -1630,9 +1681,13 @@ root cause is already filed.
 - [x] **UX19 — Window size/position isn't remembered.** [index.ts](src/main/index.ts) opens a fixed
  920×700 every launch (no bounds persistence). **Fix:** persist + restore window bounds. *Fixed with W2 —
  [windowState.ts](src/main/windowState.ts) persists + restores bounds/maximized with an off-screen guard.*
- [ ] **UX20 — "App is still running" surprise.** Closing with a download active hides to tray and
+- [x] **UX20 — "App is still running" surprise.** Closing with a download active hides to tray and
  notifies once per process (L68); later closes give no hint, so the window "won't close" reads as a
-  bug. **Fix:** a persistent tray hint / first-close explainer.
+  bug. **Fix:** a persistent tray hint / first-close explainer. *Resolved by L68 (the "first-close explainer"
+  it now is on *every* close): the once-per-process guard was the whole bug — `notifiedBackground` now resets
+  when the window is shown again, so each close-to-tray with a download active (or tray mode) re-fires the
+  "AeroFetch is still running in the tray" notification, not just the first. So a later close no longer looks
+  like the window is stuck. (The tray behavior itself is worth a watched confirm in the Batch 15 live pass.)*
 - [x] **UX21 — No keyboard-shortcut discoverability.** Ctrl+K (palette), Enter, Ctrl+Enter are
  undocumented; there's no "?"/shortcuts screen. **Fix:** a shortcuts hint or help affordance.
  *Fixed (discoverability): the first-run Onboarding now includes a tip — "Press Ctrl+K anytime to jump to any
@@ -2100,6 +2155,9 @@ earlier passes are listed so this is complete; and the categories that came back
  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.*
 - [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-
@@ -2113,6 +2171,10 @@ earlier passes are listed so this is complete; and the categories that came back
  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 —
  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.*
 - [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,
@@ -2167,6 +2229,10 @@ cosmetics. `R` IDs.
  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.*
 - [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.
@@ -13,12 +13,15 @@ import type { AddEntry } from './downloads'
 *
 *   sources  --enqueueDownloads-->  downloads   (queue a channel's media items)
 *   downloads --downloadCompleted--> sources    (mark the source item downloaded)
+ *   sources  --sourceRemoved------>  downloads   (cancel that source's in-flight downloads)
 */
 export interface StoreEvents {
  /** sources → downloads: enqueue a batch of media items as downloads */
  enqueueDownloads: AddEntry[]
  /** downloads → sources: a download tied to a source MediaItem finished */
  downloadCompleted: { mediaItemId: string; filePath?: string }
+  /** sources → downloads: a source was removed; cancel its in-flight downloads (L157) */
+  sourceRemoved: string
 }

 type Handler<K extends keyof StoreEvents> = (payload: StoreEvents[K]) => void
@@ -462,6 +462,23 @@ export const useDownloads = create<DownloadState>((set, get) => {
 // no direct import from sources, so the two stores stay acyclic.
 on('enqueueDownloads', (entries) => useDownloads.getState().addMany(entries))

+// A removed source cancels its in-flight downloads (L157): MediaItem ids are
+// `${sourceId}:${videoId}` (indexerCore), so a download's mediaItemId prefix
+// identifies its source. Only active (downloading/queued) items are canceled;
+// finished/failed ones are left alone. cancel() owns the process kill + pump.
+on('sourceRemoved', (sourceId) => {
+  const st = useDownloads.getState()
+  const prefix = `${sourceId}:`
+  for (const it of st.items) {
+    if (
+      (it.status === 'downloading' || it.status === 'queued') &&
+      it.mediaItemId?.startsWith(prefix)
+    ) {
+      st.cancel(it.id)
+    }
+  }
+})
+
 // M4: mirror the durable queue to main whenever it changes, so saved/scheduled and
 // other pending items survive a quit. Skipped in preview (no IPC bridge). A
 // progress-only update doesn't churn this — toPersisted drops progress/speed/eta, so
@@ -178,6 +178,10 @@ export const useSources = create<SourcesState>((set, get) => ({
      sources: s.sources.filter((x) => x.id !== id),
      selectedSourceId: s.selectedSourceId === id ? null : s.selectedSourceId
    }))
+    // Cancel any of this source's downloads still in flight, so removing a source
+    // stops its videos from finishing into now-orphaned folders (L157). Routed via
+    // the event bus (C2) so sources doesn't import downloads.
+    emit('sourceRemoved', id)
    if (!PREVIEW) window.api.removeSource(id).catch(logError('removeSource'))
  },