From e19f988c72eede40909f301a0762f8f19b183e1d Mon Sep 17 00:00:00 2001 From: debont80 Date: Wed, 1 Jul 2026 21:36:22 -0400 Subject: [PATCH] =?UTF-8?q?feat(audit):=20Batch=2015=20live-verify=20?= =?UTF-8?q?=E2=80=94=20download-engine=20lifecycle=20(R4,=20L65,=20L139,?= =?UTF-8?q?=20B2,=20B6,=20L143)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 / .fNNN.) via the pure, unit-tested lib/partials.ts — never a bare . 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 --- CODE-AUDIT.md | 106 +++++++++++++----- src/main/buildArgs.ts | 10 ++ src/main/download.ts | 76 ++++++++++++- src/main/index.ts | 66 +++++++---- src/main/indexer.ts | 62 ++++++++-- src/main/ipc.ts | 25 +++-- src/main/lib/partials.ts | 39 +++++++ src/main/terminal.ts | 9 ++ src/main/updater.ts | 47 +++++++- src/main/ytdlp.ts | 44 ++++++-- src/main/ytdlpLock.ts | 80 +++++++++++++ src/preload/index.ts | 6 + src/renderer/src/components/LibraryView.tsx | 15 ++- .../settings/SoftwareUpdateCard.tsx | 38 ++++++- src/renderer/src/mockApi.ts | 2 + src/renderer/src/store/sources.ts | 29 ++++- src/shared/ipc.ts | 4 + test/buildArgs.test.ts | 9 ++ test/partials.test.ts | 74 ++++++++++++ test/ytdlpLock.test.ts | 94 ++++++++++++++++ 20 files changed, 737 insertions(+), 98 deletions(-) create mode 100644 src/main/lib/partials.ts create mode 100644 src/main/ytdlpLock.ts create mode 100644 test/partials.test.ts create mode 100644 test/ytdlpLock.test.ts diff --git a/CODE-AUDIT.md b/CODE-AUDIT.md index 36cb924..15e95a0 100644 --- a/CODE-AUDIT.md +++ b/CODE-AUDIT.md @@ -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" 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 `.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** `.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 `.fNNN.` files — never a + bare `.` 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. diff --git a/src/main/buildArgs.ts b/src/main/buildArgs.ts index 997c8c6..0020280 100644 --- a/src/main/buildArgs.ts +++ b/src/main/buildArgs.ts @@ -68,6 +68,12 @@ function audioQuality(quality: string): string { // break the parser that splits on it (CL1). export const PROGRESS_MARKER = 'prog|' export const FILEPATH_MARKER = 'path|' +// Emitted once at the before_dl stage carrying the resolved FINAL output path, so +// the engine knows the destination stem while the download is still in flight — +// used to delete orphaned .part / .fNNN intermediates when a download is canceled +// (R4). `%(filepath)s` is still unresolved this early (prints 'NA'); `%(filename)s` +// already resolves to the full output path. +export const DEST_MARKER = 'dest|' // The progress line yt-dlp emits (one per --newline tick). Note the leading // `download:` is the progress-template TYPE selector and is consumed by yt-dlp @@ -415,6 +421,10 @@ export function buildArgs(input: BuildArgsInput): string[] { // Print the final path after post-processing/move so we can open it later. '--print', `after_move:${FILEPATH_MARKER}%(filepath)s`, + // Print the resolved destination up front (before_dl) so a cancel can find and + // delete this download's orphaned partials by stem, mid-flight (R4). + '--print', + `before_dl:${DEST_MARKER}%(filename)s`, '--no-simulate' ] diff --git a/src/main/download.ts b/src/main/download.ts index ec40772..8f54875 100644 --- a/src/main/download.ts +++ b/src/main/download.ts @@ -1,7 +1,7 @@ import { spawn, execFile, type ChildProcess } from 'child_process' -import { existsSync, unlinkSync } from 'fs' +import { existsSync, unlinkSync, readdirSync } from 'fs' import { tmpdir } from 'os' -import { join } from 'path' +import { join, parse } from 'path' import { BrowserWindow, Notification, type WebContents } from 'electron' import { getYtdlpPath, @@ -16,6 +16,8 @@ import { import { getSettings } from './settings' import { execFileAsync } from './lib/exec' import { createLineBuffer } from './lib/lineBuffer' +import { orphanPartials } from './lib/partials' +import { isYtdlpUpdating, acquireSpawn, releaseSpawn } from './ytdlpLock' import { getDownloadArchivePath, getDefaultMediaDir } from './paths' import { ensureManagedYtdlp } from './ytdlp' import { materializeCookies, hasStoredCookies } from './cookies' @@ -28,7 +30,8 @@ import { formatCommandLine, collectionOutputTemplate, PROGRESS_MARKER, - FILEPATH_MARKER + FILEPATH_MARKER, + DEST_MARKER } from './buildArgs' import { cleanError } from './log' import { addErrorLog } from './errorlog' @@ -288,6 +291,16 @@ export function startDownload(wc: WebContents, opts: StartDownloadOptions): Star if (active.size >= maxConcurrent) { return { ok: false, error: 'Max concurrent downloads reached. Wait for a slot to free up.' } } + // L139: don't launch the managed yt-dlp while a self-update is rewriting the exe — + // on Windows that races into a sharing violation or a half-written binary. The + // updater backs off whenever a download is live and only holds the lock for the + // brief rewrite, so this is a rare, retryable refusal. + if (isYtdlpUpdating()) { + return { + ok: false, + error: 'yt-dlp is updating in the background — try this download again in a moment.' + } + } // Decrypt the stored cookie jar to a short-lived per-download temp file (H7). // yt-dlp reads --cookies once at startup; we delete it the moment the download @@ -319,6 +332,9 @@ export function startDownload(wc: WebContents, opts: StartDownloadOptions): Star cleanupCookies() return { ok: false, error: (e as Error).message } } + // L139: a live yt-dlp process now exists — the updater must defer to it until the + // matching releaseSpawn() on settle (close/error/stall) below. + acquireSpawn() const rec: ActiveDownload = { child, canceled: false, paused: false } active.set(opts.id, rec) @@ -375,6 +391,10 @@ function wireChildProcess(params: { let stderrTail = '' let filePath: string | undefined + // The resolved FINAL output path (from the before_dl `dest|` print), captured + // while the download is still running so a cancel can delete this download's + // orphaned partials by stem (R4). Undefined until the first stream starts. + let outputPath: string | undefined // Latched once the first download stream reports 'finished'; flags later // progress as the merge/post-processing "finishing" phase (SR7). let finishing = false @@ -401,6 +421,7 @@ function wireChildProcess(params: { clearWatchdog() cleanup() releaseActive(opts.id, rec) + releaseSpawn() killTree(rec) const msg = `Download stalled — no activity for ${Math.round(STALL_TIMEOUT_MS / 60_000)} min. Stopped; retry to resume.` send(wc, { type: 'error', id: opts.id, error: msg }) @@ -432,6 +453,10 @@ function wireChildProcess(params: { } } else if (line.startsWith(FILEPATH_MARKER)) { filePath = line.slice(FILEPATH_MARKER.length).trim() + } else if (line.startsWith(DEST_MARKER)) { + // before_dl fires once per stream; every emission carries the same final + // path, so the last-write-wins overwrite is harmless. + outputPath = line.slice(DEST_MARKER.length).trim() } }) child.stdout?.on('data', (chunk: Buffer) => { @@ -450,6 +475,10 @@ function wireChildProcess(params: { clearWatchdog() cleanup() releaseActive(opts.id, rec) + releaseSpawn() + // A canceled download's partials are now orphans — remove them (R4). A paused + // download was also killed on purpose, but keeps its .part for a later resume. + if (rec.canceled) cleanupPartials(outputPath) // A paused download was killed on purpose — stay silent, like a cancel. if (!rec.canceled && !rec.paused) { send(wc, { type: 'error', id: opts.id, error: err.message }) @@ -469,9 +498,16 @@ function wireChildProcess(params: { clearWatchdog() cleanup() releaseActive(opts.id, rec) - // Canceled: renderer already showed 'canceled'. Paused: renderer showed - // 'paused' and keeps the .part for a later resume. Either way, no event. - if (rec.canceled || rec.paused) return + releaseSpawn() + // Canceled: renderer already showed 'canceled'; the process is now gone (its + // file handles released), so delete this download's orphaned partials (R4). + // Paused: renderer showed 'paused' and KEEPS the .part for a later resume. + // Either way, no event. + if (rec.canceled) { + cleanupPartials(outputPath) + return + } + if (rec.paused) return if (code === 0) { send(wc, { type: 'done', id: opts.id, filePath }) notify( @@ -511,6 +547,34 @@ function killTree(rec: ActiveDownload): void { } } +/** + * Delete a canceled download's orphaned intermediate files (R4). `outputPath` is the + * resolved FINAL destination captured from the before_dl `dest|` print; orphanPartials + * (pure, unit-tested in lib/partials.ts) turns its dir listing into the exact set of + * this download's `.part` / `.part-Frag*` / `.ytdl` / `.fNNN.` intermediates — + * never a completed `.` or another download's files. Best-effort: a + * still-locked or already-gone entry is skipped. Called from the child's close/error + * handler, so the process (and its file handles) are already gone. + */ +function cleanupPartials(outputPath: string | undefined): void { + if (!outputPath) return + const { dir } = parse(outputPath) + if (!dir) return + let entries: string[] + try { + entries = readdirSync(dir) + } catch { + return // folder vanished / unreadable — nothing to clean + } + for (const entry of orphanPartials(outputPath, entries)) { + try { + unlinkSync(join(dir, entry)) + } catch { + /* best-effort: locked or already removed */ + } + } +} + export function cancelDownload(id: string): void { const rec = active.get(id) if (!rec) return diff --git a/src/main/index.ts b/src/main/index.ts index 1eaca5f..f5827e2 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -1,4 +1,4 @@ -import { app, shell, BrowserWindow, nativeTheme, Notification, Menu } from 'electron' +import { app, shell, BrowserWindow, nativeTheme, dialog, Menu } from 'electron' import { join, resolve } from 'path' import { electronApp, optimizer, is } from '@electron-toolkit/utils' import { IpcChannels } from '@shared/ipc' @@ -10,7 +10,6 @@ import { getSystemThemeInfo } from './ipc' import { runStartupYtdlpAutoUpdate } from './ytdlp' -import { getAppIconImage } from './binaries' import { hasActiveDownloads } from './download' import { getSettings, applyLaunchAtStartup, migrateSecretsAtRest } from './settings' import { ensureMediaDirs } from './paths' @@ -64,15 +63,39 @@ let mainWindow: BrowserWindow | null = null // Tell the user (once per run) that closing the window left AeroFetch running so // an in-progress download could finish — shown only when they haven't already // opted into tray mode, so a window that "won't close" doesn't read as a bug. -let notifiedBackground = false -function notifyBackgroundOnce(): void { - if (notifiedBackground || !Notification.isSupported()) return - notifiedBackground = true - new Notification({ - title: 'AeroFetch is still running', - body: 'Your download is finishing in the background. Use the tray icon to reopen or quit.', - icon: getAppIconImage() - }).show() +// L143: an in-window, tray-INDEPENDENT way to quit at the moment the window "won't +// close" because a download is running — without it the only exit is the tray menu +// (and Task Manager if the tray ever failed to create). Guarded so hammering the X +// can't stack dialogs. Only used when the user did NOT opt into tray mode; a +// deliberate minimize-to-tray close stays a silent hide. +let quitPromptOpen = false +function promptQuitWhileDownloading(win: BrowserWindow): void { + if (quitPromptOpen) return + quitPromptOpen = true + dialog + .showMessageBox(win, { + type: 'question', + buttons: ['Keep downloading', 'Quit anyway'], + defaultId: 0, + cancelId: 0, + title: 'Download in progress', + message: 'A download is still in progress.', + detail: + 'AeroFetch will keep downloading in the background — reopen it from the tray icon. Quit anyway to stop the download and exit now.' + }) + .then(({ response }) => { + quitPromptOpen = false + if (response === 1) { + markQuitting() // lets the close handler through + runs the before-quit teardown + app.quit() + } else { + win.hide() // keep downloading in the background + } + }) + .catch(() => { + quitPromptOpen = false + win.hide() + }) } // Web permissions a download manager never needs. They're denied for the app @@ -155,11 +178,15 @@ function createWindow(): void { const downloadsRunning = hasActiveDownloads() if (getSettings().minimizeToTray || downloadsRunning) { e.preventDefault() - win.hide() - // If we're only staying alive because a download is running (the user - // didn't opt into tray mode), tell them once — otherwise a window that - // won't close looks like a bug. - if (downloadsRunning && !getSettings().minimizeToTray) notifyBackgroundOnce() + // A download (not tray mode) is the only thing holding the app open: offer a + // tray-independent "Quit anyway" instead of silently hiding — otherwise a + // window that won't close looks like a bug, with no in-window way out (L143). + // A tray-mode close is the user's explicit choice, so that stays a silent hide. + if (downloadsRunning && !getSettings().minimizeToTray) { + promptQuitWhileDownloading(win) + } else { + win.hide() + } } }) @@ -167,13 +194,6 @@ function createWindow(): void { if (mainWindow === win) mainWindow = null }) - // When the user re-opens the window (via tray, second-instance, or deeplink) - // reset the background-notify latch so they're informed again if they close - // while a download is still running (L68). - win.on('show', () => { - notifiedBackground = false - }) - // The OS may have launched us with an aerofetch:// link or a "Send to" .url // file on the command line — hand it to DownloadBar's link-suggestion banner // once the page (and its IPC listener) is actually ready to receive it. diff --git a/src/main/indexer.ts b/src/main/indexer.ts index c4da71f..ff0f2c3 100644 --- a/src/main/indexer.ts +++ b/src/main/indexer.ts @@ -46,11 +46,13 @@ interface FlatInfo { * flat upload list can be a few MB of JSON; the timeout is generous for the same * reason. `--` terminates option parsing so the URL can't be read as a flag. */ -async function probeFlat(url: string): Promise { +async function probeFlat(url: string, signal?: AbortSignal): Promise { const r = await execFileAsync( getYtdlpPath(), ['-J', '--flat-playlist', '--no-warnings', '--', url], - { maxBuffer: INDEX_MAX_BUFFER, timeout: INDEX_TIMEOUT_MS } + // `signal` lets a Cancel abort the in-flight probe child mid-walk (B2); Node + // kills the process and the call rejects, which the caller treats as canceled. + { maxBuffer: INDEX_MAX_BUFFER, timeout: INDEX_TIMEOUT_MS, signal } ) if (!r.ok) { throw new Error( @@ -67,8 +69,36 @@ async function probeFlat(url: string): Promise { } /** Probe a channel tab (e.g. /videos, /playlists); never throws — returns null. */ -function probeTab(url: string): Promise { - return probeFlat(url).catch(() => null) +function probeTab(url: string, signal?: AbortSignal): Promise { + return probeFlat(url, signal).catch(() => null) +} + +// Active user-initiated index walks. `cancelIndexing()` aborts them so an +// `index:cancel` IPC can stop the walk and kill the in-flight probe child (B2). +// Background sync (sync.ts) calls indexSource without a signal, so it isn't +// user-cancelable — it's throttled and unattended, not a blocking UI action. +const activeIndexAborts = new Set() + +/** Abort every in-flight user index/reindex walk (the add-source "Cancel" button). */ +export function cancelIndexing(): void { + for (const c of activeIndexAborts) c.abort() +} + +/** + * indexSource wrapped with a fresh AbortController registered for cancelIndexing(). + * The IPC layer calls this; sync.ts calls the raw indexSource (not cancelable). + */ +export async function indexSourceCancelable( + url: string, + onProgress: (p: IndexProgress) => void +): Promise { + const controller = new AbortController() + activeIndexAborts.add(controller) + try { + return await indexSource(url, onProgress, controller.signal) + } finally { + activeIndexAborts.delete(controller) + } } /** @@ -78,8 +108,14 @@ function probeTab(url: string): Promise { */ export async function indexSource( url: string, - onProgress: (p: IndexProgress) => void + onProgress: (p: IndexProgress) => void, + signal?: AbortSignal ): Promise { + // Returned whenever a Cancel arrives mid-walk. It deliberately does NOT push a + // progress event: the renderer drives the cancel and resets its own indexing UI, + // so a late 'error' event can't re-surface as a failure (B2). + const canceled: IndexSourceResult = { ok: false, error: 'Indexing canceled.' } + let normalizedUrl: string try { normalizedUrl = assertHttpUrl(url) // normalised form for spawning (audit F5) @@ -106,11 +142,13 @@ export async function indexSource( // 1. Enumerate the channel's playlists, then each playlist's videos. onProgress({ url, phase: 'playlists', message: 'Finding playlists…' }) - const playlistTab = await probeTab(`${cls.base}/playlists`) + const playlistTab = await probeTab(`${cls.base}/playlists`, signal) + if (signal?.aborted) return canceled const playlistEntries = playlistTab?.entries ?? [] const total = playlistEntries.length let done = 0 for (const pe of playlistEntries) { + if (signal?.aborted) return canceled // stop before the next probe done++ const purl = entryUrl(pe) if (!purl) continue @@ -121,7 +159,7 @@ export async function indexSource( current: done, total }) - const pdata = await probeTab(purl) + const pdata = await probeTab(purl, signal) if (pdata?.entries?.length) { playlists.push({ title: pe.title || 'Playlist', entries: pdata.entries }) } @@ -129,7 +167,8 @@ export async function indexSource( // 2. The full uploads feed — the catch-all for videos in no playlist. onProgress({ url, phase: 'uploads', message: 'Indexing channel uploads…' }) - const videoTab = await probeTab(`${cls.base}/videos`) + const videoTab = await probeTab(`${cls.base}/videos`, signal) + if (signal?.aborted) return canceled uploads = videoTab?.entries ?? [] title = @@ -144,7 +183,8 @@ export async function indexSource( // resolve to a playlist when probed. A lone video has no entries → error. kind = cls?.kind ?? 'playlist' onProgress({ url, phase: 'uploads', message: 'Indexing playlist…' }) - const data = await probeFlat(cls?.base ?? normalizedUrl) + const data = await probeFlat(cls?.base ?? normalizedUrl, signal) + if (signal?.aborted) return canceled const entries = data.entries ?? [] if (entries.length === 0) { return { @@ -166,6 +206,9 @@ export async function indexSource( if (fresh.length === 0) { return { ok: false, error: 'No downloadable videos found for this source.' } } + // A cancel that landed after the final probe must not persist the source — + // otherwise a "canceled" add still pops into the library later (B2). + if (signal?.aborted) return canceled // Incremental merge: preserve the downloaded state of anything already on // disk, and report how many videos are new since the last index. @@ -196,6 +239,7 @@ export async function indexSource( }) return { ok: true, source, itemCount: items.length, newCount } } catch (e) { + if (signal?.aborted) return canceled // the throw was our own abort — not an error const msg = e instanceof Error ? e.message : String(e) onProgress({ url, phase: 'error', message: msg }) return { ok: false, error: msg } diff --git a/src/main/ipc.ts b/src/main/ipc.ts index 0584a63..ea102c9 100644 --- a/src/main/ipc.ts +++ b/src/main/ipc.ts @@ -27,9 +27,10 @@ import { import { PAGE_BACKGROUND } from '@shared/theme' import { getYtdlpVersion, updateYtdlp } from './ytdlp' import { getFfmpegVersions } from './ffmpeg' -import { checkForAppUpdate, downloadAppUpdate, runAppUpdate } from './updater' +import { checkForAppUpdate, downloadAppUpdate, runAppUpdate, cancelAppUpdate } from './updater' import { probeMedia } from './probe' import { startDownload, cancelDownload, pauseDownload, previewCommand } from './download' +import { whenYtdlpUpdateSettled } from './ytdlpLock' import { runTerminal, cancelTerminal } from './terminal' import { getSettings, setSettings } from './settings' import { listHistory, addHistory, removeHistory, removeManyHistory, clearHistory } from './history' @@ -47,7 +48,7 @@ import { setMediaItemDownloaded, setSourceWatched } from './sources' -import { indexSource } from './indexer' +import { indexSourceCancelable, cancelIndexing } from './indexer' import { syncWatchedSources } from './sync' import { getScheduledSync, setScheduledSync } from './schedule' import { getActiveBadge, getErrorBadge } from './badge' @@ -102,6 +103,7 @@ export function registerIpcHandlers(getMainWindow: () => BrowserWindow | null): ipcMain.handle(IpcChannels.appUpdateDownload, (e, url: string) => downloadAppUpdate(url, e.sender) ) + ipcMain.handle(IpcChannels.appUpdateCancel, () => cancelAppUpdate()) ipcMain.handle(IpcChannels.appUpdateRun, (_e, filePath: string) => runAppUpdate(filePath)) ipcMain.handle(IpcChannels.ytdlpVersion, () => getYtdlpVersion()) @@ -110,7 +112,11 @@ export function registerIpcHandlers(getMainWindow: () => BrowserWindow | null): ipcMain.handle(IpcChannels.probe, (_e, url: string) => probeMedia(url)) - ipcMain.handle(IpcChannels.downloadStart, (e, opts: StartDownloadOptions) => { + ipcMain.handle(IpcChannels.downloadStart, async (e, opts: StartDownloadOptions) => { + // L139: hold the spawn until any in-progress yt-dlp self-update has finished + // swapping the exe, so a launch-time (auto-resumed) download waits it out instead + // of racing the rewrite. Resolves immediately when no update is running. + await whenYtdlpUpdateSettled() const result = startDownload(e.sender, opts) // Pre-spawn failures (missing yt-dlp.exe, bad URL, duplicate id) never reach // download.ts's own close/error handlers, so log them here instead — unless the @@ -128,9 +134,10 @@ export function registerIpcHandlers(getMainWindow: () => BrowserWindow | null): }) ipcMain.handle(IpcChannels.downloadCancel, (_e, id: string) => cancelDownload(id)) ipcMain.handle(IpcChannels.downloadPause, (_e, id: string) => pauseDownload(id)) - ipcMain.handle(IpcChannels.terminalRun, (e, id: string, args: string) => - runTerminal(e.sender, id, args) - ) + ipcMain.handle(IpcChannels.terminalRun, async (e, id: string, args: string) => { + await whenYtdlpUpdateSettled() // L139: hold behind an in-progress exe rewrite + return runTerminal(e.sender, id, args) + }) ipcMain.handle(IpcChannels.terminalCancel, (_e, id: string) => cancelTerminal(id)) ipcMain.handle(IpcChannels.chooseFolder, async (e, current?: string) => { @@ -241,17 +248,19 @@ export function registerIpcHandlers(getMainWindow: () => BrowserWindow | null): // Indexing pushes live progress to the requesting renderer over `indexProgress` // and resolves with the final result. ipcMain.handle(IpcChannels.sourceIndex, (e, url: string) => - indexSource(url, (p) => { + indexSourceCancelable(url, (p) => { if (!e.sender.isDestroyed()) e.sender.send(IpcChannels.indexProgress, p) }) ) ipcMain.handle(IpcChannels.sourceReindex, (e, id: string) => { const src = getSource(id) if (!src) return { ok: false, error: 'Source not found.' } - return indexSource(src.url, (p) => { + return indexSourceCancelable(src.url, (p) => { if (!e.sender.isDestroyed()) e.sender.send(IpcChannels.indexProgress, p) }) }) + // Abort the in-flight index/reindex walk (kills the current probe child). (B2) + ipcMain.handle(IpcChannels.sourceIndexCancel, () => cancelIndexing()) ipcMain.handle(IpcChannels.sourceSetWatched, (_e, id: string, watched: boolean) => setSourceWatched(id, watched) ) diff --git a/src/main/lib/partials.ts b/src/main/lib/partials.ts new file mode 100644 index 0000000..e390862 --- /dev/null +++ b/src/main/lib/partials.ts @@ -0,0 +1,39 @@ +/** + * Pure identification of a download's orphaned yt-dlp intermediate files (R4). + * + * When a running download is canceled, yt-dlp is tree-killed and leaves partial + * files behind in the output folder: the growing `.part`, fragment pieces + * (`.part-Frag*`) and the `.ytdl` fragment-state file for fragmented formats, plus + * any already-finished per-stream file (`.fNNN.` — e.g. the video half + * of a video+audio download whose audio was still in flight). A plain cancel never + * removes these, so they accumulate. download.ts captures the resolved FINAL output + * path from the before_dl `dest|` print; this module turns that path + a folder + * listing into the exact set to delete. + * + * Kept free of any electron/node-runtime chain (only `path.parse`) so it's + * unit-testable in isolation, the same posture as lib/formatters.ts (L37). + * + * SAFETY: it returns only unambiguous yt-dlp intermediates, matched on the output + * stem. It never returns a bare `.` completed file — so a pre-existing + * same-title download the user kept in the same folder is safe — nor any file + * belonging to a different download (different stem). This is the guard against the + * audit's "delete the wrong file if the destination capture is even slightly off". + */ +import { parse } from 'path' + +export function orphanPartials(outputPath: string, entries: string[]): string[] { + // `name` is the basename without the final extension; a title containing dots is + // preserved (only the trailing `.` is stripped), so the stem stays exact. + const stem = parse(outputPath).name + if (!stem) return [] + const stemDot = `${stem}.` + return entries.filter((entry) => { + if (!entry.startsWith(stemDot)) return false + return ( + entry.endsWith('.part') || // ….part — in-progress stream or merge + entry.includes('.part-Frag') || // fragment pieces (HLS/DASH) + entry.endsWith('.ytdl') || // fragment-download state file + /\.f\d+\.[^.]+$/i.test(entry) // finished per-stream file, e.g. .f399.mp4 + ) + }) +} diff --git a/src/main/terminal.ts b/src/main/terminal.ts index a2e382d..99e27a0 100644 --- a/src/main/terminal.ts +++ b/src/main/terminal.ts @@ -13,6 +13,7 @@ import { type WebContents } from 'electron' import { getYtdlpPath, getBinDir, getSystem32Path } from './binaries' import { getSettings } from './settings' import { ensureManagedYtdlp } from './ytdlp' +import { isYtdlpUpdating, acquireSpawn, releaseSpawn } from './ytdlpLock' import { parseExtraArgs } from './buildArgs' import { createLineBuffer } from './lib/lineBuffer' import { IpcChannels, type TerminalEvent, type TerminalRunResult } from '@shared/ipc' @@ -36,6 +37,11 @@ export function runTerminal(wc: WebContents, id: string, argsRaw: string): Termi return { ok: false, error: 'yt-dlp.exe is missing and could not be restored.' } } if (active.has(id)) return { ok: false, error: 'A command is already running.' } + // L139: same exe-rewrite interlock as downloads — don't launch yt-dlp while a + // self-update is replacing the binary. + if (isYtdlpUpdating()) { + return { ok: false, error: 'yt-dlp is updating — try again in a moment.' } + } // Always pin ffmpeg so post-processing works; the rest is whatever the user typed. const args = ['--ffmpeg-location', getBinDir(), ...parseExtraArgs(argsRaw)] @@ -46,6 +52,7 @@ export function runTerminal(wc: WebContents, id: string, argsRaw: string): Termi return { ok: false, error: (e as Error).message } } active.set(id, child) + acquireSpawn() // L139: live yt-dlp process — the updater must defer to it // Buffer each stream and emit on newline boundaries (flush the remainder on close). const lineBufs = { @@ -60,12 +67,14 @@ export function runTerminal(wc: WebContents, id: string, argsRaw: string): Termi if (settled) return settled = true active.delete(id) + releaseSpawn() send(wc, { type: 'error', id, error: err.message }) }) child.on('close', (code) => { if (settled) return settled = true active.delete(id) + releaseSpawn() // Flush any trailing partial lines that never hit a newline. lineBufs.stdout.flush() lineBufs.stderr.flush() diff --git a/src/main/updater.ts b/src/main/updater.ts index 4e54665..ea0bf3e 100644 --- a/src/main/updater.ts +++ b/src/main/updater.ts @@ -199,6 +199,19 @@ export async function checkForAppUpdate(): Promise { /** How long the download may stall (no bytes received) before we give up. */ const DOWNLOAD_IDLE_TIMEOUT_MS = 60_000 +// The in-flight installer download's canceller, set while a download runs and +// cleared on settle. During the pre-download checksum phase it just flags the +// cancel; during the download phase it routes through `finish()` (abort the +// request + clean up the partial). Each phase clears the slot only if it still +// owns it, so a superseded download's late settle can't knock out a newer +// download's canceller (same ownership rule as L140's releaseActive). (B6) +let cancelActiveDownload: (() => void) | null = null + +/** Abort the in-flight app-update installer download, if any (the update card's Cancel). */ +export function cancelAppUpdate(): void { + cancelActiveDownload?.() +} + // Integrity: every release MUST attach a checksum asset named `.sha256` // (e.g. AeroFetch-Setup-0.5.0.exe.sha256) whose contents include the installer's // lowercase SHA-256 hex — bare, or in `sha256sum`/`Get-FileHash` form. We refuse @@ -280,20 +293,37 @@ export async function downloadAppUpdate(url: string, wc: WebContents): Promise { + canceledEarly = true + } + cancelActiveDownload = earlyCancel + const settleEarly = (result: AppUpdateDownload): AppUpdateDownload => { + if (cancelActiveDownload === earlyCancel) cancelActiveDownload = null + return result + } + const sum = await fetchTrustedText(checksumUrl) + if (canceledEarly) return settleEarly({ ok: false, error: 'Update download canceled.' }) let expectedSha: string | null = null if (sum.ok) { expectedSha = extractSha256(sum.text, safeName) - if (!expectedSha) return { ok: false, error: "The update's checksum file is malformed." } + if (!expectedSha) { + return settleEarly({ ok: false, error: "The update's checksum file is malformed." }) + } } else if (sum.status === 404) { if (REQUIRE_CHECKSUM) { - return { + return settleEarly({ ok: false, error: `This release has no checksum (${safeName}.sha256) attached — refusing to install an unverified update.` - } + }) } } else { - return { ok: false, error: `Couldn't fetch the update's checksum — ${sum.error}.` } + return settleEarly({ ok: false, error: `Couldn't fetch the update's checksum — ${sum.error}.` }) } return new Promise((resolve) => { @@ -307,6 +337,8 @@ export async function downloadAppUpdate(url: string, wc: WebContents): Promise { if (settled) return settled = true + // Done — release the cancel slot, but only if this download still owns it (B6). + if (cancelActiveDownload === requestCancel) cancelActiveDownload = null if (idle) clearTimeout(idle) if (!result.ok) { request.abort() @@ -318,6 +350,13 @@ export async function downloadAppUpdate(url: string, wc: WebContents): Promise finish({ ok: false, error: 'Update download canceled.' }) + cancelActiveDownload = requestCancel + // net.request (not fetch) so we can re-validate the host on EVERY redirect // hop: Gitea 302s a release download to its attachment store, and undici's // fetch hides the Location header under redirect:'manual', so it can't gate diff --git a/src/main/ytdlp.ts b/src/main/ytdlp.ts index 8b02919..f82d39f 100644 --- a/src/main/ytdlp.ts +++ b/src/main/ytdlp.ts @@ -6,6 +6,7 @@ import { cleanError } from './log' import { VERSION_TIMEOUT_MS, YTDLP_UPDATE_TIMEOUT_MS } from './constants' import { getSettings, setSettings } from './settings' import { shouldAutoCheckYtdlp } from './ytdlpPolicy' +import { beginYtdlpUpdate, endYtdlpUpdate, liveSpawnCount } from './ytdlpLock' import { isYtdlpUpdateChannel, type YtdlpVersionResult, @@ -78,16 +79,31 @@ export async function updateYtdlp(channel: YtdlpUpdateChannel): Promise 0) return const channel = settings.ytdlpChannel send({ phase: 'checking', channel }) const before = await getYtdlpVersion() + if (liveSpawnCount() > 0) { + // A spawn started during the version probe — defer to next launch (no record). + send({ phase: 'current', channel, version: before.ok ? before.version : undefined }) + return + } const result = await updateYtdlp(channel) setSettings({ ytdlpLastUpdateCheck: Date.now() }) diff --git a/src/main/ytdlpLock.ts b/src/main/ytdlpLock.ts new file mode 100644 index 0000000..e046d95 --- /dev/null +++ b/src/main/ytdlpLock.ts @@ -0,0 +1,80 @@ +/** + * Interlock between the yt-dlp self-update (which REWRITES the managed yt-dlp.exe) + * and the download / terminal spawns (which EXECUTE it). On Windows, spawning the + * exe while `--update-to` is swapping it in place races into a sharing violation or + * launches a half-written binary (L139). This serialises the two: + * + * - each live yt-dlp process registers via acquireSpawn()/releaseSpawn(); + * - the updater calls beginYtdlpUpdate(), which REFUSES (returns false) when any + * spawn is live — the update is best-effort and daily-throttled, so it simply + * skips this cycle rather than rewrite the exe out from under a running process; + * - a spawn that arrives while an update holds the lock is HELD (not failed): the + * IPC layer awaits whenYtdlpUpdateSettled() before spawning, so the download just + * waits out the brief rewrite instead of erroring. isYtdlpUpdating() is the + * synchronous backstop for any caller that doesn't await. + * + * The main process is single-threaded, so these plain counters/flags need no real + * locking: a check-then-act sequence (e.g. await whenYtdlpUpdateSettled() → spawn → + * acquireSpawn) runs with no await between the settle and the spawn, so no update can + * interleave. Pure (no electron/node chain) so it's unit-testable in isolation. + */ +let updating = false +let liveSpawns = 0 +// Resolved by endYtdlpUpdate(); recreated per update so a held spawn wakes exactly +// when the current rewrite finishes. Only meaningful while `updating` is true. +let settle: (() => void) | null = null +let settledPromise: Promise = Promise.resolve() + +/** True while an update is rewriting yt-dlp.exe — synchronous spawn backstop. */ +export function isYtdlpUpdating(): boolean { + return updating +} + +/** Register a live yt-dlp process — call immediately after a successful spawn. */ +export function acquireSpawn(): void { + liveSpawns++ +} + +/** Deregister a yt-dlp process on settle (close/error). Never drops below zero. */ +export function releaseSpawn(): void { + if (liveSpawns > 0) liveSpawns-- +} + +/** How many yt-dlp processes are currently live (test seam / diagnostics). */ +export function liveSpawnCount(): number { + return liveSpawns +} + +/** + * Resolves once no update is in progress — used to HOLD a spawn until the exe swap + * finishes, rather than refuse it. Returns an already-resolved promise when idle, so + * the common (non-updating) path adds no delay. Bounded by the caller's update + * timeout: endYtdlpUpdate() always runs (finally), so this can't hang forever. + */ +export function whenYtdlpUpdateSettled(): Promise { + return updating ? settledPromise : Promise.resolve() +} + +/** + * Enter the update critical section. Returns false — and does NOT take the lock — if + * any spawn is live OR another update already holds the lock, in which case the + * caller must not update. The updating check matters: a second concurrent begin + * (startup auto-update racing a manual "Update now") would otherwise replace the + * settle promise, letting the FIRST update's end() release spawns held by the + * second one mid-rewrite. On true the caller owns the lock until endYtdlpUpdate(). + */ +export function beginYtdlpUpdate(): boolean { + if (updating || liveSpawns > 0) return false + updating = true + settledPromise = new Promise((resolve) => { + settle = resolve + }) + return true +} + +/** Leave the update critical section, waking any spawn held on whenYtdlpUpdateSettled(). */ +export function endYtdlpUpdate(): void { + updating = false + settle?.() + settle = null +} diff --git a/src/preload/index.ts b/src/preload/index.ts index 87093df..1b05b7a 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -47,6 +47,9 @@ const api = { downloadAppUpdate: (url: string): Promise => ipcRenderer.invoke(IpcChannels.appUpdateDownload, url), + /** Abort the in-flight installer download (the update card's Cancel button). */ + cancelAppUpdate: (): Promise => ipcRenderer.invoke(IpcChannels.appUpdateCancel), + /** Launch a downloaded installer and quit so it can replace the app. */ runAppUpdate: (filePath: string): Promise<{ ok: boolean; error?: string }> => ipcRenderer.invoke(IpcChannels.appUpdateRun, filePath), @@ -194,6 +197,9 @@ const api = { reindexSource: (id: string): Promise => ipcRenderer.invoke(IpcChannels.sourceReindex, id), + /** Abort the in-flight index/reindex walk (the add-source "Cancel" button). */ + cancelIndex: (): Promise => ipcRenderer.invoke(IpcChannels.sourceIndexCancel), + removeSource: (id: string): Promise => ipcRenderer.invoke(IpcChannels.sourceRemove, id), listSourceItems: (sourceId: string): Promise => diff --git a/src/renderer/src/components/LibraryView.tsx b/src/renderer/src/components/LibraryView.tsx index 734d1fb..eb67d51 100644 --- a/src/renderer/src/components/LibraryView.tsx +++ b/src/renderer/src/components/LibraryView.tsx @@ -22,7 +22,8 @@ import { AppsListRegular, VideoClipMultipleRegular, AlertRegular, - LibraryRegular + LibraryRegular, + DismissRegular } from '@fluentui/react-icons' import type { MediaItem, Source, MediaKind } from '@shared/ipc' import { isPreview as PREVIEW } from '../isPreview' @@ -228,6 +229,7 @@ export function LibraryView(): React.JSX.Element { const selectSource = useSources((s) => s.selectSource) const indexSource = useSources((s) => s.indexSource) const reindexSource = useSources((s) => s.reindexSource) + const cancelIndexing = useSources((s) => s.cancelIndexing) const removeSource = useSources((s) => s.removeSource) const enqueueItems = useSources((s) => s.enqueueItems) const setWatched = useSources((s) => s.setWatched) @@ -358,7 +360,7 @@ export function LibraryView(): React.JSX.Element { if (!u || indexing.active) return const res = await indexSource(u) if (res.ok) setUrl('') - else setError(res.error ?? 'Could not add that link.') + else if (!res.canceled) setError(res.error ?? 'Could not add that link.') } function toggle(id: string, on: boolean): void { @@ -586,6 +588,15 @@ export function LibraryView(): React.JSX.Element { {indexing.message ?? 'Adding…'} {indexing.current && indexing.total ? ` (${indexing.current}/${indexing.total})` : ''} + {/* B2: a big channel's index walk can take minutes — let the user abort it. */} + )} {error && {error}} diff --git a/src/renderer/src/components/settings/SoftwareUpdateCard.tsx b/src/renderer/src/components/settings/SoftwareUpdateCard.tsx index 3fae425..2292aa2 100644 --- a/src/renderer/src/components/settings/SoftwareUpdateCard.tsx +++ b/src/renderer/src/components/settings/SoftwareUpdateCard.tsx @@ -1,4 +1,4 @@ -import { useEffect, useState } from 'react' +import { useEffect, useRef, useState } from 'react' import { Field, Input, @@ -11,7 +11,12 @@ import { ProgressBar, tokens } from '@fluentui/react-components' -import { ArrowSyncRegular, ArrowDownloadRegular, OpenRegular } from '@fluentui/react-icons' +import { + ArrowSyncRegular, + ArrowDownloadRegular, + OpenRegular, + DismissRegular +} from '@fluentui/react-icons' import { type AppUpdateInfo } from '@shared/ipc' import { useSettings } from '../../store/settings' import { useErrorTextStyles } from '../ui/errorText' @@ -30,6 +35,9 @@ export function SoftwareUpdateCard(): React.JSX.Element { const [appDownloading, setAppDownloading] = useState(false) const [appFraction, setAppFraction] = useState(undefined) const [appUpdError, setAppUpdError] = useState(null) + // Set when the user cancels the download, so the awaited result (which comes back + // not-ok) is treated as canceled rather than surfaced as an error (B6). + const canceledRef = useRef(false) useEffect(() => { window.api.getAppVersion().then(setAppVersion).catch(logError('getAppVersion')) @@ -55,11 +63,15 @@ export function SoftwareUpdateCard(): React.JSX.Element { async function installAppUpdate(): Promise { if (!appUpd?.downloadUrl) return + canceledRef.current = false setAppDownloading(true) setAppFraction(undefined) setAppUpdError(null) try { const dl = await window.api.downloadAppUpdate(appUpd.downloadUrl) + // The user hit Cancel: main aborted the download and cleaned up the partial — + // don't surface its not-ok result as an error or try to install (B6). + if (canceledRef.current) return if (!dl.ok || !dl.filePath) { setAppUpdError(dl.error ?? 'Download failed.') return @@ -68,12 +80,17 @@ export function SoftwareUpdateCard(): React.JSX.Element { // On success the app quits as the installer launches; only errors return here. if (!run.ok) setAppUpdError(run.error ?? 'Could not start the installer.') } catch (e) { - setAppUpdError(e instanceof Error ? e.message : String(e)) + if (!canceledRef.current) setAppUpdError(e instanceof Error ? e.message : String(e)) } finally { setAppDownloading(false) } } + function cancelDownload(): void { + canceledRef.current = true + window.api.cancelAppUpdate?.().catch(logError('cancelAppUpdate')) + } + return (
@@ -141,7 +158,20 @@ export function SoftwareUpdateCard(): React.JSX.Element { )} {appDownloading && ( - + <> + + {/* B6: a ~300 MB installer download can take a while — let the user abort it. */} +
+ +
+ )} )} diff --git a/src/renderer/src/mockApi.ts b/src/renderer/src/mockApi.ts index b4f21d8..52050d2 100644 --- a/src/renderer/src/mockApi.ts +++ b/src/renderer/src/mockApi.ts @@ -60,6 +60,7 @@ export const mockApi: Api = { ok: false, error: 'Downloading updates is disabled in the browser preview.' }), + cancelAppUpdate: async () => {}, runAppUpdate: async () => ({ ok: true }), onAppUpdateProgress: () => () => {}, getYtdlpVersion: async () => ({ ok: true, version: '2025.06.01 (UI preview mock)' }), @@ -237,6 +238,7 @@ export const mockApi: Api = { } }, reindexSource: async () => ({ ok: true, itemCount: 7, newCount: 0 }), + cancelIndex: async () => {}, removeSource: async () => [], setMediaItemDownloaded: async () => [], setSourceWatched: async () => [], diff --git a/src/renderer/src/store/sources.ts b/src/renderer/src/store/sources.ts index ce8b74f..fa09521 100644 --- a/src/renderer/src/store/sources.ts +++ b/src/renderer/src/store/sources.ts @@ -80,9 +80,12 @@ interface SourcesState { indexing: IndexingState loadSources: () => void selectSource: (id: string | null) => void - /** index a pasted channel/playlist URL; resolves with ok + an error message */ - indexSource: (url: string) => Promise<{ ok: boolean; error?: string }> + /** index a pasted channel/playlist URL; resolves with ok + an error message. + * `canceled` is set when the user aborted, so the caller shows no error (B2). */ + indexSource: (url: string) => Promise<{ ok: boolean; error?: string; canceled?: boolean }> reindexSource: (id: string) => Promise + /** abort an in-flight index/reindex walk (the add-source "Cancel" button, B2) */ + cancelIndexing: () => void removeSource: (id: string) => void /** * Enqueue the given media items into the download queue with folder context. @@ -104,6 +107,10 @@ interface SourcesState { syncWatched: () => Promise } +// Set by cancelIndexing() so the awaiting index/reindex call knows the user +// aborted and can resolve as canceled (no error UI) rather than as a failure (B2). +let indexCanceledRequested = false + export const useSources = create((set, get) => ({ sources: seedSources, itemsBySource: seedItemsBySource, @@ -131,15 +138,19 @@ export const useSources = create((set, get) => ({ }, indexSource: async (url) => { + indexCanceledRequested = false set({ indexing: { active: true, url, message: 'Reading source…' } }) if (PREVIEW) { await new Promise((r) => setTimeout(r, 900)) // simulate the index walk set({ indexing: { active: false } }) - return { ok: true } + return indexCanceledRequested ? { ok: false, canceled: true } : { ok: true } } try { const res = await window.api.indexSource(url) set({ indexing: { active: false } }) + // The user hit Cancel: report canceled (no error UI) regardless of what main + // returned for the aborted walk (B2). + if (indexCanceledRequested) return { ok: false, canceled: true } if (res.ok) { get().loadSources() if (res.source) get().selectSource(res.source.id) @@ -147,11 +158,13 @@ export const useSources = create((set, get) => ({ return { ok: res.ok, error: res.error } } catch (e) { set({ indexing: { active: false } }) + if (indexCanceledRequested) return { ok: false, canceled: true } return { ok: false, error: e instanceof Error ? e.message : String(e) } } }, reindexSource: async (id) => { + indexCanceledRequested = false const src = get().sources.find((s) => s.id === id) set({ indexing: { active: true, url: src?.url, message: 'Refreshing…' } }) if (PREVIEW) { @@ -162,6 +175,7 @@ export const useSources = create((set, get) => ({ try { const res = await window.api.reindexSource(id) set({ indexing: { active: false } }) + if (indexCanceledRequested) return // user aborted — leave the source as-is if (res.ok) { get().loadSources() const items = await window.api.listSourceItems(id) @@ -173,6 +187,15 @@ export const useSources = create((set, get) => ({ } }, + cancelIndexing: () => { + // Optimistically clear the indexing UI; main aborts the in-flight probe child. + // The pending index/reindex promise then resolves and is treated as canceled + // via indexCanceledRequested (B2). + indexCanceledRequested = true + set({ indexing: { active: false } }) + if (!PREVIEW) window.api.cancelIndex().catch(logError('cancelIndex')) + }, + removeSource: (id) => { set((s) => ({ sources: s.sources.filter((x) => x.id !== id), diff --git a/src/shared/ipc.ts b/src/shared/ipc.ts index 331ee6e..5f619f0 100644 --- a/src/shared/ipc.ts +++ b/src/shared/ipc.ts @@ -10,6 +10,8 @@ export const IpcChannels = { appUpdateCheck: 'app:update-check', /** download the latest release's installer to a temp file */ appUpdateDownload: 'app:update-download', + /** abort the in-flight installer download (B6) */ + appUpdateCancel: 'app:update-cancel', /** launch a downloaded installer and quit so it can replace the app */ appUpdateRun: 'app:update-run', /** main → renderer push channel for installer download progress */ @@ -68,6 +70,8 @@ export const IpcChannels = { sourcesList: 'sources:list', sourceIndex: 'sources:index', sourceReindex: 'sources:reindex', + /** renderer → main: abort the in-flight index/reindex walk (B2) */ + sourceIndexCancel: 'sources:index-cancel', sourceRemove: 'sources:remove', sourceItems: 'sources:items', /** mark a media item downloaded once its collection download completes */ diff --git a/test/buildArgs.test.ts b/test/buildArgs.test.ts index 089ee5d..6516691 100644 --- a/test/buildArgs.test.ts +++ b/test/buildArgs.test.ts @@ -9,6 +9,7 @@ import { collectionOutputTemplate, CROP_SQUARE_PPA, ARIA2C_ARGS, + DEST_MARKER, type AccessOptions } from '../src/main/buildArgs' import { @@ -63,6 +64,14 @@ describe('buildArgs scaffolding', () => { expect(argv[argv.length - 2]).toBe('--') expect(argv[argv.length - 1]).toBe(URL) }) + + it('prints the before_dl destination (dest|) so a cancel can locate partials (R4)', () => { + // download.ts's cancel cleanup depends on this print to know the output stem; + // removing it would silently disable orphan-.part deletion. + expect(hasSeq(build(opts(), dlo()), '--print', `before_dl:${DEST_MARKER}%(filename)s`)).toBe( + true + ) + }) }) // --- Network: proxy / rate limit / aria2c ----------------------------------- diff --git a/test/partials.test.ts b/test/partials.test.ts new file mode 100644 index 0000000..dae87bc --- /dev/null +++ b/test/partials.test.ts @@ -0,0 +1,74 @@ +import { describe, it, expect } from 'vitest' +// Pure module — no electron import chain (L37), same as lib/formatters. +import { orphanPartials } from '../src/main/lib/partials' + +// The resolved FINAL output path the before_dl `dest|` print gives download.ts. +const OUT = 'C:\\Users\\me\\Videos\\Despacito.webm' + +describe('orphanPartials (R4 cancel cleanup)', () => { + it('deletes the in-progress .part stream file', () => { + expect(orphanPartials(OUT, ['Despacito.f399.mp4.part'])).toEqual(['Despacito.f399.mp4.part']) + }) + + it('deletes a finished per-stream intermediate (no .part) awaiting merge', () => { + // e.g. the video half of a video+audio download whose audio was still in flight. + expect(orphanPartials(OUT, ['Despacito.f251.webm'])).toEqual(['Despacito.f251.webm']) + }) + + it('deletes fragment pieces and the .ytdl fragment-state file', () => { + const entries = [ + 'Despacito.f140.m4a.part-Frag0', + 'Despacito.f140.m4a.part-Frag12', + 'Despacito.f140.m4a.ytdl' + ] + expect(orphanPartials(OUT, entries)).toEqual(entries) + }) + + it('KEEPS a pre-existing same-title completed file (video and audio)', () => { + // The audit's "delete the wrong file" risk: a bare . is never ours. + expect(orphanPartials(OUT, ['Despacito.webm', 'Despacito.mp3', 'Despacito.mkv'])).toEqual([]) + }) + + it('KEEPS a different download in the same folder (different stem)', () => { + expect(orphanPartials(OUT, ['Other Video.f399.mp4.part', 'Other Video.f251.webm'])).toEqual([]) + }) + + it('does not match a stem that is only a prefix of a longer name', () => { + // "Despacito 2.f399.mp4.part" must NOT be swept by the "Despacito" download — + // startsWith('Despacito.') excludes it (the dot boundary matters). + expect(orphanPartials(OUT, ['Despacito 2.f399.mp4.part', 'Despacito2.webm.part'])).toEqual([]) + }) + + it('handles a title containing dots (only the final extension is the boundary)', () => { + const out = 'C:\\Users\\me\\Music\\Track 3.14 - Pi.mp3' + const entries = [ + 'Track 3.14 - Pi.webm.part', + 'Track 3.14 - Pi.mp3', + 'Track 3.14 - Pi.f251.webm' + ] + // .part + intermediate deleted; the completed .mp3 kept. + expect(orphanPartials(out, entries)).toEqual([ + 'Track 3.14 - Pi.webm.part', + 'Track 3.14 - Pi.f251.webm' + ]) + }) + + it('sorts nothing / preserves order and mixes correctly in a realistic listing', () => { + const entries = [ + 'Despacito.f399.mp4.part', // delete: in-progress video + 'Despacito.f251.webm', // delete: finished audio intermediate + 'Despacito.webm', // keep: pre-existing final + 'Unrelated.f140.m4a.part', // keep: other download + 'Despacito.f140.m4a.ytdl' // delete: fragment state + ] + expect(orphanPartials(OUT, entries)).toEqual([ + 'Despacito.f399.mp4.part', + 'Despacito.f251.webm', + 'Despacito.f140.m4a.ytdl' + ]) + }) + + it('returns [] when the path has no usable stem', () => { + expect(orphanPartials('', ['x.part'])).toEqual([]) + }) +}) diff --git a/test/ytdlpLock.test.ts b/test/ytdlpLock.test.ts new file mode 100644 index 0000000..32726f5 --- /dev/null +++ b/test/ytdlpLock.test.ts @@ -0,0 +1,94 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest' + +// Module-level mutable state — reset to a fresh instance per test (L139 interlock). +type Lock = typeof import('../src/main/ytdlpLock') +let lock: Lock +beforeEach(async () => { + vi.resetModules() + lock = await import('../src/main/ytdlpLock') +}) + +describe('ytdlpLock (L139 spawn↔update interlock)', () => { + it('starts idle: not updating, zero spawns, settle already resolved', async () => { + expect(lock.isYtdlpUpdating()).toBe(false) + expect(lock.liveSpawnCount()).toBe(0) + await expect(lock.whenYtdlpUpdateSettled()).resolves.toBeUndefined() + }) + + it('acquire/release tracks the live count and clamps at zero', () => { + lock.acquireSpawn() + lock.acquireSpawn() + expect(lock.liveSpawnCount()).toBe(2) + lock.releaseSpawn() + expect(lock.liveSpawnCount()).toBe(1) + lock.releaseSpawn() + lock.releaseSpawn() // extra release must not underflow + expect(lock.liveSpawnCount()).toBe(0) + }) + + it('beginYtdlpUpdate refuses (and does NOT take the lock) while a spawn is live', () => { + lock.acquireSpawn() + expect(lock.beginYtdlpUpdate()).toBe(false) + expect(lock.isYtdlpUpdating()).toBe(false) + lock.releaseSpawn() + expect(lock.beginYtdlpUpdate()).toBe(true) + expect(lock.isYtdlpUpdating()).toBe(true) + lock.endYtdlpUpdate() + expect(lock.isYtdlpUpdating()).toBe(false) + }) + + it('holds a spawn until endYtdlpUpdate resolves whenYtdlpUpdateSettled', async () => { + expect(lock.beginYtdlpUpdate()).toBe(true) + let resolved = false + const held = lock.whenYtdlpUpdateSettled().then(() => { + resolved = true + }) + await Promise.resolve() // flush a microtask tick — still pending mid-update + expect(resolved).toBe(false) + lock.endYtdlpUpdate() + await held + expect(resolved).toBe(true) + }) + + it('resolves immediately (no hold) when no update is in progress', async () => { + let resolved = false + await lock.whenYtdlpUpdateSettled().then(() => { + resolved = true + }) + expect(resolved).toBe(true) + }) + + it('refuses a CONCURRENT second update — the first end() must release only its own lock', async () => { + expect(lock.beginYtdlpUpdate()).toBe(true) + // A racing second begin (startup auto-update vs manual "Update now") must be + // refused — accepting it would swap the settle promise, letting the first + // update's end() release spawns held by the still-running second update. + expect(lock.beginYtdlpUpdate()).toBe(false) + expect(lock.isYtdlpUpdating()).toBe(true) // the refusal didn't clobber the lock + let resolved = false + const held = lock.whenYtdlpUpdateSettled().then(() => { + resolved = true + }) + await Promise.resolve() + expect(resolved).toBe(false) // still held by the rightful owner + lock.endYtdlpUpdate() + await held + expect(resolved).toBe(true) + expect(lock.isYtdlpUpdating()).toBe(false) + }) + + it('a second update after one settles holds again on a fresh promise', async () => { + lock.beginYtdlpUpdate() + lock.endYtdlpUpdate() + expect(lock.beginYtdlpUpdate()).toBe(true) + let resolved = false + const held = lock.whenYtdlpUpdateSettled().then(() => { + resolved = true + }) + await Promise.resolve() + expect(resolved).toBe(false) + lock.endYtdlpUpdate() + await held + expect(resolved).toBe(true) + }) +})