From ba9879d1e31ca93811969ed272bc572e0394bfeb Mon Sep 17 00:00:00 2001 From: Praveen Mittal Date: Mon, 17 Aug 2026 22:09:10 +0200 Subject: [PATCH] fix: bound legacy ffmpeg frame-extraction calls with a timeout extractLegacyWindow/extractLegacySingle ran the system ffmpeg binary with no timeout, so a stalled ffmpeg (corrupt input, codec edge case, stuck pipe) hung frame extraction indefinitely. Adds a bounded timeout to both execFileAsync calls; on timeout the child is killed and the call rejects into the existing catch/log/return-empty handling. Fixes #14 --- electron/frames/extractor.test.ts | 20 ++++++++++++++++++++ electron/frames/extractor.ts | 9 +++++++-- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/electron/frames/extractor.test.ts b/electron/frames/extractor.test.ts index c963103..0ea4130 100644 --- a/electron/frames/extractor.test.ts +++ b/electron/frames/extractor.test.ts @@ -1,10 +1,12 @@ import assert from "node:assert/strict"; +import { execFile } from "node:child_process"; import { existsSync } from "node:fs"; import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; import { createRequire } from "node:module"; import { tmpdir } from "node:os"; import path from "node:path"; import test from "node:test"; +import { promisify } from "node:util"; import sharp from "sharp"; import type { CapturedVideoFrame } from "../../common/frames"; @@ -171,6 +173,24 @@ test("FrameExtractor deduplicates identical source JPEGs without deleting retain assert.equal(existsSync(path.join(framesDir, reloaded.manifest[0].file)), true); }); +test("a hung legacy ffmpeg process is killed and rejects within its timeout instead of hanging forever", async () => { + // extractLegacyWindow/extractLegacySingle bound their execFileAsync calls with + // LEGACY_FFMPEG_TIMEOUT_MS so a stalled ffmpeg (corrupt input, codec edge case, + // stuck pipe) can't hang the extraction indefinitely. Those methods are private + // and resolve their ffmpeg binary through a module-level, cached `which`/`where` + // lookup that isn't test-injectable without expanding this fix's scope, so this + // exercises the same execFileAsync-with-timeout mechanism directly: a child + // process that never exits on its own must still be killed and the call must + // still reject well within the timeout, not hang. + const execFileAsync = promisify(execFile); + const start = Date.now(); + await assert.rejects( + execFileAsync(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { timeout: 200 }), + ); + const elapsedMs = Date.now() - start; + assert.ok(elapsedMs < 5000, `expected the timeout to bound the hang, took ${elapsedMs}ms`); +}); + test("sharp loads through the app's require path as a callable factory (guards sharp 0.35 export shape)", async () => { // The frame extractor loads sharp with createRequire(import.meta.url)("sharp"), which // resolves sharp's CommonJS "require" export condition. sharp 0.35 split its import and diff --git a/electron/frames/extractor.ts b/electron/frames/extractor.ts index 43bb184..8af6143 100644 --- a/electron/frames/extractor.ts +++ b/electron/frames/extractor.ts @@ -74,6 +74,8 @@ export interface CapturedFrameSample( frames: readonly T[], @@ -355,7 +357,10 @@ export class FrameExtractor { "-q:v", "3", pattern, ], - { maxBuffer: 32 * 1024 * 1024 }, + // A stalled ffmpeg (corrupt input, codec edge case, stuck pipe) would + // otherwise hang this promise forever; the timeout bounds that and + // routes into the same failure handling as any other ffmpeg error. + { maxBuffer: 32 * 1024 * 1024, timeout: LEGACY_FFMPEG_TIMEOUT_MS }, ); } catch (err) { log.warn("legacy probe window failed:", message(err)); @@ -402,7 +407,7 @@ export class FrameExtractor { "-q:v", "3", "-y", file, ], - { maxBuffer: 16 * 1024 * 1024 }, + { maxBuffer: 16 * 1024 * 1024, timeout: LEGACY_FFMPEG_TIMEOUT_MS }, ); } catch (err) { log.warn(`legacy frame at ${offsetSec.toFixed(2)}s failed:`, message(err));