Fix C1/C2 + reliability cluster (L140/L141/L148, R6/R7)

Critical: - C1: single source of truth for IPC mock + Settings defaults. DEFAULT_SETTINGS in @shared/ipc; main DEFAULTS, renderer FALLBACK, and preview MOCK_SETTINGS all derive from it. Whole browser mock collapsed into one typed mockApi.ts; main.tsx shrinks to window.api = mockApi. PREVIEW check single-sourced in isPreview.ts. - C2: break the downloads<->sources circular import via a typed event bus (store/coordinator.ts). App eagerly imports sources for side-effects so startup load + scheduled --sync + downloadCompleted subscription still run. Reliability: - L140/L148: cancel/pause release the active slot synchronously; identity-guarded releaseActive; per-spawn cookie jar so a same-id retry/resume is never rejected by a stale entry nor run over the concurrency cap. - L141: renderer history capped at 500 + url de-dup to match main. - R6: writeJsonAtomic reports/logs write failures and keeps data dirty for retry instead of an empty catch. - R7: encryptSecret warns before the plaintext fallback. typecheck + 243 tests + eslint green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-06-30 20:25:49 -04:00
parent 6c19899f75
commit 7e3e5af52d
20 changed files with 596 additions and 413 deletions
@@ -47,6 +47,21 @@ interface ActiveDownload {

 const active = new Map<string, ActiveDownload>()

+// Per-spawn sequence, so a retry/resume that reuses the item id still gets a
+// unique transient cookie file (below) — otherwise a superseded download's
+// teardown could unlink the new spawn's jar mid-read.
+let spawnSeq = 0
+
+// Remove an item from the active map, but only if THIS rec still owns the slot.
+// cancel/pause release the slot synchronously (so the renderer's just-promoted
+// next item isn't rejected by the maxConcurrent guard while the killed tree is
+// still tearing down); the doomed child's later 'close'/'error' then runs this as
+// a no-op. The identity check matters because a retry/resume reuses the item id,
+// so a stale teardown must not evict the newer same-id spawn. (L140/L148)
+function releaseActive(id: string, rec: ActiveDownload): void {
+  if (active.get(id) === rec) active.delete(id)
+}
+
 // Backstop for B1: if a spawned yt-dlp goes completely silent — no stdout or
 // stderr — for this long, treat it as wedged, kill it, and error the item so the
 // concurrency slot frees instead of leaking forever. Generous on purpose so a
@@ -269,7 +284,9 @@ export function startDownload(wc: WebContents, opts: StartDownloadOptions): Star
  // settles, so the plaintext never lingers at rest.
  let cookiesFile: string | undefined
  if (getSettings().cookieSource === 'login' && hasStoredCookies()) {
-    const tmp = join(tmpdir(), `aerofetch-cookies-${opts.id}.txt`)
+    // Unique per spawn (not just per id): a retry/resume reuses opts.id, and the
+    // superseded download's cleanupCookies() must not unlink the new spawn's jar.
+    const tmp = join(tmpdir(), `aerofetch-cookies-${opts.id}-${++spawnSeq}.txt`)
    if (materializeCookies(tmp)) cookiesFile = tmp
  }
  function cleanupCookies(): void {
@@ -337,11 +354,14 @@ export function startDownload(wc: WebContents, opts: StartDownloadOptions): Star
  function bumpWatchdog(): void {
    clearWatchdog()
    stallTimer = setTimeout(() => {
-      if (settled) return
+      // A cancel/pause races the timer: it already released the slot and the
+      // child's close stays silent, so don't fire a spurious stall error (mirrors
+      // the canceled/paused guards on the close handler below).
+      if (settled || rec.canceled || rec.paused) return
      settled = true
      clearWatchdog()
      cleanupCookies()
-      active.delete(opts.id)
+      releaseActive(opts.id, rec)
      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 })
@@ -385,7 +405,7 @@ export function startDownload(wc: WebContents, opts: StartDownloadOptions): Star
    settled = true
    clearWatchdog()
    cleanupCookies()
-    active.delete(opts.id)
+    releaseActive(opts.id, rec)
    // 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 })
@@ -399,7 +419,7 @@ export function startDownload(wc: WebContents, opts: StartDownloadOptions): Star
    settled = true
    clearWatchdog()
    cleanupCookies()
-    active.delete(opts.id)
+    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
@@ -438,6 +458,11 @@ export function cancelDownload(id: string): void {
  const rec = active.get(id)
  if (!rec) return
  rec.canceled = true
+  // Free the slot now (not on the async 'close') so the renderer's just-promoted
+  // next item isn't rejected by the maxConcurrent guard — or run briefly over the
+  // cap — while taskkill tears down this tree. The child's later 'close' stays
+  // silent (rec.canceled) and releaseActive() no-ops. (L148)
+  active.delete(id)
  killTree(rec)
 }

@@ -452,5 +477,9 @@ export function pauseDownload(id: string): void {
  const rec = active.get(id)
  if (!rec) return
  rec.paused = true
+  // Free the slot now (see cancelDownload / L148). The partial .part stays on disk
+  // for a later resume, which reuses this id — releaseActive()'s identity check
+  // keeps the doomed child's 'close' from evicting that fresh spawn. (L140)
+  active.delete(id)
  killTree(rec)
 }
@@ -1,16 +1,16 @@
-import type { HistoryEntry } from '@shared/ipc'
+import { HISTORY_MAX_ENTRIES, type HistoryEntry } from '@shared/ipc'
 import { isValidHistoryEntry } from './validation'
 import { createJsonStore } from './jsonStore'

 // Plain JSON in userData (portable build redirects userData next to the exe).
 // Kept simple per the build plan; can migrate to better-sqlite3 later. Atomic
 // writes, corruption backup, and caching come from the shared jsonStore (R1–R3).
-const MAX_ENTRIES = 500
-
+// The row cap (HISTORY_MAX_ENTRIES) is shared with the renderer's optimistic list.
+//
 // Per-entry validation (isValidHistoryEntry) so a hand-edited or corrupted
 // history.json can't feed the UI (or openPath) entries with the wrong shape —
 // invalid rows are dropped rather than trusted. (audit S5)
-const store = createJsonStore('history.json', isValidHistoryEntry, MAX_ENTRIES)
+const store = createJsonStore('history.json', isValidHistoryEntry, HISTORY_MAX_ENTRIES)

 export function listHistory(): HistoryEntry[] {
  return store.read()
@@ -1,6 +1,6 @@
 import { app } from 'electron'
 import { join } from 'path'
-import { readFileSync, writeFileSync, renameSync, existsSync, copyFileSync } from 'fs'
+import { readFileSync, writeFileSync, renameSync, existsSync, copyFileSync, unlinkSync } from 'fs'

 /**
 * One shared persistence layer for the hand-rolled JSON array stores (history,
@@ -55,16 +55,29 @@ export function readJsonArraySafe<T>(path: string, isValid: (o: unknown) => o is
 * `rename` it over the target (an atomic operation on the same volume), so a
 * crash mid-write leaves either the old file or the new one -- never a truncated
 * one (R1). `pretty` indents for human-readable files; pass false for large
- * machine-only stores to avoid inflating size/write time (R8). Best-effort: a
- * read-only data dir just means nothing is persisted.
+ * machine-only stores to avoid inflating size/write time (R8). Returns whether the
+ * write landed: a failure (disk full, read-only data dir) is logged rather than
+ * swallowed silently, so a record that looked saved but vanished on restart is at
+ * least diagnosable (R6). The leftover temp file is removed on failure so a partial
+ * `.tmp` can't accumulate.
 */
-export function writeJsonAtomic(path: string, value: unknown, pretty = true): void {
+export function writeJsonAtomic(path: string, value: unknown, pretty = true): boolean {
  const tmp = `${path}.tmp`
  try {
    writeFileSync(tmp, pretty ? JSON.stringify(value, null, 2) : JSON.stringify(value))
    renameSync(tmp, path)
-  } catch {
-    /* best-effort; e.g. a read-only data dir */
+    return true
+  } catch (e) {
+    // R6: previously a silent catch — a failed persist (disk full, read-only
+    // profile) dropped the change with no trace. Log it; the user-facing warning
+    // is deferred to the leveled file logger (CC8).
+    console.error(`[AeroFetch] failed to write ${path}:`, e)
+    try {
+      if (existsSync(tmp)) unlinkSync(tmp)
+    } catch {
+      /* best-effort temp cleanup */
+    }
+    return false
  }
 }

@@ -110,8 +123,10 @@ export function createJsonStore<T>(
      timer = null
    }
    if (!dirty) return
-    dirty = false
-    writeJsonAtomic(filePath(), cache ?? [], pretty)
+    // Only clear the dirty flag once the write actually lands. A failed flush
+    // (disk full, transient lock) stays dirty so the next write() or the
+    // quit-time flushAllStores() retries it instead of silently dropping it (R6).
+    if (writeJsonAtomic(filePath(), cache ?? [], pretty)) dirty = false
  }

  function write(items: T[]): T[] {
@@ -13,6 +13,7 @@ import {
  VIDEO_QUALITY_OPTIONS,
  AUDIO_QUALITY_OPTIONS,
  DEFAULT_DOWNLOAD_OPTIONS,
+  DEFAULT_SETTINGS,
  isYtdlpUpdateChannel,
  type Settings,
  type DownloadOptions,
@@ -22,51 +23,11 @@ import {
  type VideoCodecPref
 } from '@shared/ipc'

-const DEFAULTS: Settings = {
-  // Both blank by default → downloads land in Documents\Video / Documents\Audio
-  // (see getDefaultMediaDir). A non-empty value is an explicit per-kind override.
-  videoDir: '',
-  audioDir: '',
-  defaultKind: 'video',
-  defaultVideoQuality: 'Best available',
-  defaultAudioQuality: 'Best',
-  maxConcurrent: 2,
-  filenameTemplate: '%(title)s.%(ext)s',
-  // Follow the OS theme on first launch (SR1) so a dark-mode Windows user isn't
-  // greeted by a bright white window despite full system-theme support.
-  theme: 'system',
-  accentColor: 'teal',
-  clipboardWatch: true,
-  downloadOptions: DEFAULT_DOWNLOAD_OPTIONS,
-  proxy: '',
-  rateLimit: '',
-  useAria2c: false,
-  cookieSource: 'none',
-  cookiesBrowser: 'chrome',
-  youtubePlayerClient: '',
-  youtubePoToken: '',
-  restrictFilenames: false,
-  downloadArchive: false,
-  // yt-dlp is self-managed: a writable copy under userData, auto-updated on the
-  // configured channel so a stale binary can't silently cause YouTube 403s.
-  autoUpdateYtdlp: true,
-  // Default to stable so a nightly regression doesn't break downloads for
-  // everyone out of the box (SR2). Users who want the latest YouTube fixes
-  // can switch to nightly in Settings → Software.
-  ytdlpChannel: 'stable',
-  ytdlpLastUpdateCheck: 0,
-  customCommandEnabled: false,
-  defaultTemplateId: null,
-  notifyOnComplete: true,
-  // Default off so adding a watched channel doesn't silently fill the user's
-  // disk on first use (SR3). Enable explicitly once they know what it does.
-  autoDownloadNew: false,
-  hasCompletedOnboarding: false,
-  minimizeToTray: false,
-  launchAtStartup: false,
-  sidebarCollapsed: false,
-  updateToken: ''
-}
+// The electron-store defaults are the canonical DEFAULT_SETTINGS from the shared
+// contract — single-sourced so the renderer FALLBACK and the preview mock can't
+// drift from main (C1). The rationale for individual defaults (SR1/SR2/SR3) lives
+// alongside DEFAULT_SETTINGS in shared/ipc.ts.
+const DEFAULTS: Settings = DEFAULT_SETTINGS

 /**
 * Sync the OS "run at sign-in" entry with the launchAtStartup setting. Called at
@@ -197,6 +158,11 @@ function encryptSecret(plain: string): string {
  } catch {
    /* fall through — store plaintext, as it was before encryption existed */
  }
+  // R7: OS encryption (DPAPI/keychain) is unavailable, so this credential is about
+  // to be written in cleartext. Warn rather than fall back silently — a leaked
+  // settings.json would then expose the proxy password / API token. (DPAPI is
+  // effectively always present on Windows, so this should never fire in practice.)
+  console.warn('[AeroFetch] OS secret encryption unavailable — storing credential as plaintext.')
  return plain
 }