From 3baba3b7bcfb333cf87dde80a7dc01be7f773a11 Mon Sep 17 00:00:00 2001 From: debont80 Date: Wed, 1 Jul 2026 09:06:08 -0400 Subject: [PATCH] =?UTF-8?q?refactor(main):=20execFileAsync=20helper=20?= =?UTF-8?q?=E2=80=94=20unify=20the=205=20spawn-and-read=20sites?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the hand-rolled `new Promise((resolve) => execFile(…, cb))` wrappers in probe / ytdlp (×2) / ffmpeg / indexer.probeFlat / download.probeMeta with one lib/exec.ts execFileAsync returning a normalized {ok,stdout,stderr,timedOut,error} that never rejects. Each caller keeps its own error mapping (timedOut -> "Timed out …" vs stderr) and JSON parsing; behaviour is unchanged. windowsHide defaults on; per-call timeout/maxBuffer preserved. Foundation for CC4/CC5 — the stdout line-buffer half lands with SIMP5, the net.request half with SIMP16, at which point those checkboxes close. Unit-tested (exec.test.ts) against the node binary: ok / stderr+nonzero-exit / missing-binary / timeout. typecheck + 262 tests + eslint + prettier green. Co-Authored-By: Claude Opus 4.8 --- src/main/download.ts | 49 +++++++++++++++---------------- src/main/ffmpeg.ts | 27 ++++++------------ src/main/indexer.ts | 40 ++++++++++++-------------- src/main/lib/exec.ts | 52 +++++++++++++++++++++++++++++++++ src/main/probe.ts | 66 ++++++++++++++++++------------------------ src/main/ytdlp.ts | 68 +++++++++++++++----------------------------- test/exec.test.ts | 41 ++++++++++++++++++++++++++ 7 files changed, 192 insertions(+), 151 deletions(-) create mode 100644 src/main/lib/exec.ts create mode 100644 test/exec.test.ts diff --git a/src/main/download.ts b/src/main/download.ts index aeffdd0..9a6ffe6 100644 --- a/src/main/download.ts +++ b/src/main/download.ts @@ -14,6 +14,7 @@ import { YTDLP_MISSING_MSG } from './binaries' import { getSettings } from './settings' +import { execFileAsync } from './lib/exec' import { getDownloadArchivePath, getDefaultMediaDir } from './paths' import { ensureManagedYtdlp } from './ytdlp' import { materializeCookies, hasStoredCookies } from './cookies' @@ -123,32 +124,28 @@ function logFailure(opts: StartDownloadOptions, title: string | undefined, error // itself contains a newline, since each --print field is emitted on its own line. const META_SEP = '\u001f' -function probeMeta(ytdlp: string, url: string): Promise { - return new Promise((resolve) => { - execFile( - ytdlp, - [ - '--no-playlist', - '--no-warnings', - '--skip-download', - '--print', - `%(title)s${META_SEP}%(uploader)s${META_SEP}%(duration_string)s`, - '--', - url - ], - { windowsHide: true, maxBuffer: META_MAX_BUFFER, timeout: META_PROBE_TIMEOUT_MS }, - (err, stdout) => { - if (err) return resolve(null) - const [title, uploader, duration] = stdout.split(META_SEP).map((l) => l.trim()) - const clean = (v?: string): string | undefined => (v && v !== 'NA' ? v : undefined) - resolve({ - title: clean(title), - channel: clean(uploader), - durationLabel: clean(duration) - }) - } - ) - }) +async function probeMeta(ytdlp: string, url: string): Promise { + const r = await execFileAsync( + ytdlp, + [ + '--no-playlist', + '--no-warnings', + '--skip-download', + '--print', + `%(title)s${META_SEP}%(uploader)s${META_SEP}%(duration_string)s`, + '--', + url + ], + { maxBuffer: META_MAX_BUFFER, timeout: META_PROBE_TIMEOUT_MS } + ) + if (!r.ok) return null + const [title, uploader, duration] = r.stdout.split(META_SEP).map((l) => l.trim()) + const clean = (v?: string): string | undefined => (v && v !== 'NA' ? v : undefined) + return { + title: clean(title), + channel: clean(uploader), + durationLabel: clean(duration) + } } // --- Argv construction (shared by startDownload and the command preview) --- diff --git a/src/main/ffmpeg.ts b/src/main/ffmpeg.ts index 28c82c4..6c4a89e 100644 --- a/src/main/ffmpeg.ts +++ b/src/main/ffmpeg.ts @@ -1,6 +1,6 @@ -import { execFile } from 'child_process' import { existsSync } from 'fs' import { getFfmpegPath, getFfprobePath } from './binaries' +import { execFileAsync } from './lib/exec' import { VERSION_TIMEOUT_MS } from './constants' import type { FfmpegVersionResult } from '@shared/ipc' @@ -12,24 +12,13 @@ import type { FfmpegVersionResult } from '@shared/ipc' * out, or the line doesn't parse — Settings then shows "not found" instead of * failing. Unlike yt-dlp these are never self-updated, so there's no update path. */ -function readToolVersion(path: string): Promise { - if (!existsSync(path)) return Promise.resolve(null) - return new Promise((resolve) => { - execFile( - path, - ['-version'], - { windowsHide: true, timeout: VERSION_TIMEOUT_MS }, - (err, stdout) => { - if (err) { - resolve(null) - return - } - const firstLine = stdout.split('\n', 1)[0] ?? '' - const m = firstLine.match(/version\s+(\S+)/i) - resolve(m?.[1] ?? null) - } - ) - }) +async function readToolVersion(path: string): Promise { + if (!existsSync(path)) return null + const r = await execFileAsync(path, ['-version'], { timeout: VERSION_TIMEOUT_MS }) + if (!r.ok) return null + const firstLine = r.stdout.split('\n', 1)[0] ?? '' + const m = firstLine.match(/version\s+(\S+)/i) + return m?.[1] ?? null } /** Versions of the bundled ffmpeg + ffprobe, read in parallel for the Settings panel. */ diff --git a/src/main/indexer.ts b/src/main/indexer.ts index fcb2925..c4da71f 100644 --- a/src/main/indexer.ts +++ b/src/main/indexer.ts @@ -9,9 +9,9 @@ * this module is the impure shell that spawns yt-dlp and writes to disk. */ -import { execFile } from 'child_process' import { existsSync } from 'fs' import { getYtdlpPath, YTDLP_MISSING_MSG } from './binaries' +import { execFileAsync } from './lib/exec' import { INDEX_MAX_BUFFER, INDEX_TIMEOUT_MS } from './constants' import { cleanError } from './log' import { assertHttpUrl } from './url' @@ -46,28 +46,24 @@ 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. */ -function probeFlat(url: string): Promise { - return new Promise((resolve, reject) => { - execFile( - getYtdlpPath(), - ['-J', '--flat-playlist', '--no-warnings', '--', url], - { windowsHide: true, maxBuffer: INDEX_MAX_BUFFER, timeout: INDEX_TIMEOUT_MS }, - (err, stdout, stderr) => { - if (err) { - const msg = (err as { killed?: boolean }).killed - ? 'Timed out indexing source. Check the link or your connection.' - : cleanError(stderr) || err.message - reject(new Error(msg)) - return - } - try { - resolve(JSON.parse(stdout) as FlatInfo) - } catch { - reject(new Error('Could not parse source info from yt-dlp.')) - } - } +async function probeFlat(url: string): Promise { + const r = await execFileAsync( + getYtdlpPath(), + ['-J', '--flat-playlist', '--no-warnings', '--', url], + { maxBuffer: INDEX_MAX_BUFFER, timeout: INDEX_TIMEOUT_MS } + ) + if (!r.ok) { + throw new Error( + r.timedOut + ? 'Timed out indexing source. Check the link or your connection.' + : cleanError(r.stderr) || r.error?.message || '' ) - }) + } + try { + return JSON.parse(r.stdout) as FlatInfo + } catch { + throw new Error('Could not parse source info from yt-dlp.') + } } /** Probe a channel tab (e.g. /videos, /playlists); never throws — returns null. */ diff --git a/src/main/lib/exec.ts b/src/main/lib/exec.ts new file mode 100644 index 0000000..b11d53c --- /dev/null +++ b/src/main/lib/exec.ts @@ -0,0 +1,52 @@ +import { execFile, type ExecFileOptions } from 'child_process' + +/** + * Normalized result of a one-shot child-process run. `ok` is true on a clean + * exit-0; `timedOut` distinguishes a kill-by-timeout (Node sets `err.killed` when + * it terminates the process on the `timeout` option) from an ordinary non-zero + * exit, so callers can show "Timed out …" versus the captured stderr. + */ +export interface ExecResult { + ok: boolean + stdout: string + stderr: string + timedOut: boolean + /** The raw error on a non-zero exit / spawn failure (undefined when ok). */ + error?: Error +} + +/** + * `execFile` as a promise that never rejects (audit SIMP2/CC4/CC5). Replaces the + * six hand-rolled `new Promise((resolve) => execFile(…, cb))` wrappers across + * probe / ytdlp (×2) / ffmpeg / indexer / download.probeMeta / schedule — each + * caller now maps this normalized result to its own shape instead of repeating the + * `err.killed` timeout check and the resolve/reject plumbing. + * + * `windowsHide` defaults on (every caller wants the console window suppressed); + * pass `timeout` / `maxBuffer` per call exactly as before. + */ +export function execFileAsync( + file: string, + args: readonly string[], + options: ExecFileOptions = {} +): Promise { + return new Promise((resolve) => { + execFile(file, [...args], { windowsHide: true, ...options }, (err, stdout, stderr) => { + // With the default (utf8) encoding stdout/stderr are strings, but ExecFileOptions + // permits a Buffer encoding, so normalize defensively. + const out = typeof stdout === 'string' ? stdout : stdout.toString() + const errOut = typeof stderr === 'string' ? stderr : stderr.toString() + if (err) { + resolve({ + ok: false, + stdout: out, + stderr: errOut, + timedOut: (err as { killed?: boolean }).killed === true, + error: err + }) + } else { + resolve({ ok: true, stdout: out, stderr: errOut, timedOut: false }) + } + }) + }) +} diff --git a/src/main/probe.ts b/src/main/probe.ts index 260a47a..3068318 100644 --- a/src/main/probe.ts +++ b/src/main/probe.ts @@ -1,6 +1,6 @@ -import { execFile } from 'child_process' import { existsSync } from 'fs' import { getYtdlpPath, YTDLP_MISSING_MSG } from './binaries' +import { execFileAsync } from './lib/exec' import { PROBE_MAX_BUFFER, PROBE_TIMEOUT_MS } from './constants' import { fmtBytes } from '@shared/format' import { cleanError } from './log' @@ -114,52 +114,40 @@ function buildPlaylist(data: RawInfo): PlaylistInfo { * its format list) or, when the URL is a playlist, the flat list of its entries. * `--flat-playlist` is a no-op for a lone video, so one call covers both cases. */ -export function probeMedia(url: string): Promise { +export async function probeMedia(url: string): Promise { const ytdlp = getYtdlpPath() if (!existsSync(ytdlp)) { - return Promise.resolve({ - ok: false, - error: YTDLP_MISSING_MSG - }) + return { ok: false, error: YTDLP_MISSING_MSG } } let target: string try { target = assertHttpUrl(url) // normalised form (audit F5) } catch (e) { - return Promise.resolve({ ok: false, error: (e as Error).message }) + return { ok: false, error: (e as Error).message } } - return new Promise((resolve) => { - execFile( - ytdlp, - // `--` terminates option parsing so the URL can never be read as a flag. - ['-J', '--flat-playlist', '--no-warnings', '--', target], - { windowsHide: true, maxBuffer: PROBE_MAX_BUFFER, timeout: PROBE_TIMEOUT_MS }, - (err, stdout, stderr) => { - if (err) { - // execFile sets `killed` when it terminated the process on timeout. - const msg = (err as { killed?: boolean }).killed - ? 'Timed out fetching video info. Check the link or your connection.' - : cleanError(stderr) || err.message - resolve({ ok: false, error: msg }) - return - } - try { - const data = JSON.parse(stdout) as RawInfo - if (data._type === 'playlist' || Array.isArray(data.entries)) { - const playlist = buildPlaylist(data) - if (playlist.count === 0) { - resolve({ ok: false, error: 'This playlist has no downloadable entries.' }) - return - } - resolve({ ok: true, kind: 'playlist', playlist }) - } else { - resolve({ ok: true, kind: 'video', info: buildInfo(data) }) - } - } catch { - resolve({ ok: false, error: 'Could not parse video info from yt-dlp.' }) - } - } - ) + // `--` terminates option parsing so the URL can never be read as a flag. + const r = await execFileAsync(ytdlp, ['-J', '--flat-playlist', '--no-warnings', '--', target], { + maxBuffer: PROBE_MAX_BUFFER, + timeout: PROBE_TIMEOUT_MS }) + if (!r.ok) { + const msg = r.timedOut + ? 'Timed out fetching video info. Check the link or your connection.' + : cleanError(r.stderr) || r.error?.message || '' + return { ok: false, error: msg } + } + try { + const data = JSON.parse(r.stdout) as RawInfo + if (data._type === 'playlist' || Array.isArray(data.entries)) { + const playlist = buildPlaylist(data) + if (playlist.count === 0) { + return { ok: false, error: 'This playlist has no downloadable entries.' } + } + return { ok: true, kind: 'playlist', playlist } + } + return { ok: true, kind: 'video', info: buildInfo(data) } + } catch { + return { ok: false, error: 'Could not parse video info from yt-dlp.' } + } } diff --git a/src/main/ytdlp.ts b/src/main/ytdlp.ts index a82a123..e25d1cb 100644 --- a/src/main/ytdlp.ts +++ b/src/main/ytdlp.ts @@ -1,7 +1,7 @@ -import { execFile } from 'child_process' import { existsSync, mkdirSync, copyFileSync } from 'fs' import { dirname } from 'path' import { getYtdlpPath, getBundledYtdlpPath, YTDLP_MISSING_MSG } from './binaries' +import { execFileAsync } from './lib/exec' import { VERSION_TIMEOUT_MS, YTDLP_UPDATE_TIMEOUT_MS } from './constants' import { getSettings, setSettings } from './settings' import { shouldAutoCheckYtdlp } from './ytdlpPolicy' @@ -34,33 +34,21 @@ export function ensureManagedYtdlp(): void { } /** Spawn the bundled yt-dlp and read back its `--version` for the Settings panel. */ -export function getYtdlpVersion(): Promise { +export async function getYtdlpVersion(): Promise { const ytdlpPath = getYtdlpPath() if (!existsSync(ytdlpPath)) { - return Promise.resolve({ - ok: false, - error: YTDLP_MISSING_MSG - }) + return { ok: false, error: YTDLP_MISSING_MSG } } - return new Promise((resolve) => { - execFile( - ytdlpPath, - ['--version'], - { windowsHide: true, timeout: VERSION_TIMEOUT_MS }, - (err, stdout, stderr) => { - if (err) { - const msg = (err as { killed?: boolean }).killed - ? 'Timed out running yt-dlp.' - : (stderr || err.message).trim() - resolve({ ok: false, error: msg }) - return - } - resolve({ ok: true, version: stdout.trim() }) - } - ) - }) + const r = await execFileAsync(ytdlpPath, ['--version'], { timeout: VERSION_TIMEOUT_MS }) + if (!r.ok) { + const msg = r.timedOut + ? 'Timed out running yt-dlp.' + : (r.stderr || r.error?.message || '').trim() + return { ok: false, error: msg } + } + return { ok: true, version: r.stdout.trim() } } /** @@ -69,13 +57,13 @@ export function getYtdlpVersion(): Promise { * yt-dlp.exe lives in, which holds for both the dev resources/bin checkout and * a per-user install; it would fail under a locked-down system install. */ -export function updateYtdlp(channel: YtdlpUpdateChannel): Promise { +export async function updateYtdlp(channel: YtdlpUpdateChannel): Promise { // Validate against the channel allowlist BEFORE the value reaches `--update-to`. // That flag also accepts `OWNER/REPO@TAG`, which would download and install an // arbitrary binary over yt-dlp.exe — so an unrecognised value (e.g. forged by a // compromised renderer over IPC) must never be forwarded. (audit F1) if (!isYtdlpUpdateChannel(channel)) { - return Promise.resolve({ ok: false, error: 'Unsupported update channel.' }) + return { ok: false, error: 'Unsupported update channel.' } } // Self-heal: restore the managed copy from the bundled seed before updating, so @@ -84,29 +72,19 @@ export function updateYtdlp(channel: YtdlpUpdateChannel): Promise { - execFile( - ytdlpPath, - ['--update-to', channel], - { windowsHide: true, timeout: YTDLP_UPDATE_TIMEOUT_MS }, - (err, stdout, stderr) => { - if (err) { - const msg = (err as { killed?: boolean }).killed - ? 'Timed out updating yt-dlp.' - : (stderr || err.message).trim() - resolve({ ok: false, error: msg }) - return - } - resolve({ ok: true, output: stdout.trim() }) - } - ) + const r = await execFileAsync(ytdlpPath, ['--update-to', channel], { + timeout: YTDLP_UPDATE_TIMEOUT_MS }) + if (!r.ok) { + const msg = r.timedOut + ? 'Timed out updating yt-dlp.' + : (r.stderr || r.error?.message || '').trim() + return { ok: false, error: msg } + } + return { ok: true, output: r.stdout.trim() } } /** diff --git a/test/exec.test.ts b/test/exec.test.ts new file mode 100644 index 0000000..c24be67 --- /dev/null +++ b/test/exec.test.ts @@ -0,0 +1,41 @@ +import { describe, it, expect } from 'vitest' +import { execFileAsync } from '../src/main/lib/exec' + +// Exercised against the Node binary itself so the test needs no fixtures and is +// deterministic. Covers the contract the six migrated spawn sites rely on +// (SIMP2/CC4/CC5): normalized ok/stdout/stderr, never-rejects, and the timedOut flag. +describe('execFileAsync (SIMP2/CC4/CC5)', () => { + it('resolves ok with stdout on a clean exit', async () => { + const r = await execFileAsync(process.execPath, ['-e', 'process.stdout.write("hello")']) + expect(r.ok).toBe(true) + expect(r.stdout).toBe('hello') + expect(r.timedOut).toBe(false) + expect(r.error).toBeUndefined() + }) + + it('captures stderr and reports not-ok on a non-zero exit', async () => { + const r = await execFileAsync(process.execPath, [ + '-e', + 'process.stderr.write("boom"); process.exit(2)' + ]) + expect(r.ok).toBe(false) + expect(r.stderr).toContain('boom') + expect(r.error).toBeInstanceOf(Error) + expect(r.timedOut).toBe(false) + }) + + it('never rejects — a missing binary resolves not-ok with an error', async () => { + const r = await execFileAsync('definitely-not-a-real-binary-xyz', ['--version']) + expect(r.ok).toBe(false) + expect(r.error).toBeDefined() + expect(r.timedOut).toBe(false) + }) + + it('flags timedOut when the timeout kills the process', async () => { + const r = await execFileAsync(process.execPath, ['-e', 'setTimeout(() => {}, 5000)'], { + timeout: 100 + }) + expect(r.ok).toBe(false) + expect(r.timedOut).toBe(true) + }) +})