Repository navigation
Conversation
…peg/ffprobe On Windows, spawning the managed ffmpeg/ffprobe binaries can fail transiently with EBUSY/ETXTBSY when the executable is briefly locked — overwhelmingly antivirus real-time scanning a just-written or just-executed exe, occasionally a lingering handle from a concurrent sibling invocation (one render shares a single managed ffmpeg binary across many stages). All three spawn sites surfaced the raw child-process error: no retry, no binary path, no stage context — the whole render failed with a bare "spawn EBUSY". Add a shared engine helper withTransientSpawnRetry that classifies transient file-lock errnos (EBUSY, ETXTBSY, EAGAIN, EMFILE, ENFILE), re-runs the caller's spawn-and-wait flow with exponential backoff (250ms base, up to 3 attempts), logs each retry with the binary path, and enriches exhausted failures with the binary path, errno, and a transient-file-lock hint. runFfmpeg adopts it at the outcome level (ManagedChildProcess spawn_error); audioExtractor and audioPadTrim's ffprobe runner wrap their manual flows with it. A healthy spawn path keeps its exact synchronous timing.
miga-heygen
left a comment
There was a problem hiding this comment.
Reviewed at 80226960 (external contributor). Genesis's evidence gap is real and I'm grading it explicitly below, then answering the six technical questions with code citations.
Evidence grade
The mechanism (retry a spawn-time failure on a specific errno family) is plausible and internally consistent, but nothing in the PR is a witnessed Windows failure: no errno/stack from a real occurrence, no AV vendor/version, no frequency or concurrency data, no confirmation the managed binary is actually what's locked versus e.g. a stale file handle from the OS. The unit tests prove the helper's branching logic, not that this classification matches what Windows actually returns for an AV-locked exe. What would settle it: a support ticket or log excerpt from #4058 quoting the actual error.code and error.syscall, or — cheaper — a note from someone who has reproduced spawn EBUSY on this binary on real Windows hardware confirming the code is exactly EBUSY/ETXTBSY and not EPERM (Windows commonly surfaces AV/sharing-violation failures as EPERM via libuv's uv_translate_sys_error, not always EBUSY). Absent that, this ships a plausible fix for an unconfirmed root cause.
The six questions
1. Only three of the sites that spawn the managed binary — not five, and the ones left out matter more. Grepped the whole repo for spawn/execFile/spawnSync calls against getFfmpegBinary()/getFfprobeBinary() (the managed-binary resolver these three sites use), excluding tests:
-
packages/engine/src/utils/ffprobe.ts:68(runFfprobe) — unfixed. This is the general-purpose ffprobe wrapper used across the engine (4 non-test import sites), not justaudioPadTrim's own probe call. Almost certainly called more often than the three fixed sites combined. -
packages/engine/src/services/streamingEncoder.ts:487— unfixed. Live production path, consumed byproducer/services/renderOrchestrator.tsandrender/stages/captureStreamingStage.ts. -
packages/engine/src/utils/gpuEncoder.ts:65,180— unfixed. Two moregetFfmpegBinary()spawns (encoder capability probing).packages/cli/*andpackages/studio-server/helpers/proxyTranscoder.tsalso spawn ffmpeg but via a different resolver (findFfBinary/findFFmpeg(), notgetFfmpegBinary()) — plausibly out of scope for the managed embedded binary this bug is about, but worth the author confirming rather than assuming.If the root cause is real, it fires on the unfixed sites too. This is a blocker: the fix is incomplete on its own stated scope, not just under-verified.
2. Errno classification excludes EPERM/EACCES correctly (they're not in TRANSIENT_SPAWN_ERRNOS, ffSpawnErrno.ts:15-20), so a genuine permission error still fails fast — good. But see the evidence grade above: on Windows, AV/sharing-violation spawn failures often surface as EPERM, not EBUSY. If that's what #4058 actually hit, this PR retries nothing and the bug isn't fixed at all, just re-described. Needs the real errno from the issue to confirm.
3. Idempotency: safe, but by an implicit invariant in one of the three sites. runFfmpeg.ts and audioPadTrim.ts both retry through ManagedChildProcess, whose onError explicitly guards if (this.spawned) return (managedChildProcess.ts:107) — a spawn_error outcome is only produced when the child process image never loaded, so no output file could have been touched. audioExtractor.ts's runFFmpeg (producer) is a manual child_process.spawn + Promise wrapper with no such guard — it relies on the assumption that Node's raw 'error' event for these specific errno codes can only fire pre-spawn, which is true for EBUSY/ETXTBSY/ENOENT-class OS-level exec failures but is asserted nowhere in this file the way the engine's explicit spawned flag asserts it. Not a proven live bug, but a real asymmetry: the same invariant is enforced explicitly in one call site and by assumption in another. Worth a one-line comment or the same explicit guard.
- No cross-attempt cleanup issue found otherwise — a
spawn_errorretry re-spawns fresh each time, no stale process or fd carried over.
4. Observable: yes. Every retry logs via console.warn with the binary path and errno (ffSpawnRetry.ts:80-83) before the delay. One gap: on retry exhaustion, the result-shaped path (runFfmpeg.ts's getTransientError callback) just returns the raw unenriched result (ffSpawnRetry.ts:118, if (attemptIndex + 2 > plan.attempts) return result;) — it never calls describeSpawnFailure, so runFfmpeg's caller sees the same bare errno message it always did on final failure, while the thrown-error path (audioExtractor, and audioPadTrim via its rejected-promise style) gets the enriched "after N attempts... antivirus" message via handleThrownSpawnError. Minor, but it means the retry's own context is invisible on the one call site (runFfmpeg) that's arguably the most-used.
5. Wiring verified by reading the call sites, not the tests, per the ask:
-
runFfmpeg.ts:116—return withTransientSpawnRetry(getFfmpegBinary(), async () => {...}, (result) => ...). Confirmed. -
audioExtractor.ts:90—return withTransientSpawnRetry(getFfmpegBinary(), async () => {...}, () => undefined). Confirmed (the() => undefinedthird arg means this call site only retries on the thrown path, never the result-shaped path — correct, since this function only ever rejects or resolves void). -
audioPadTrim.ts:660—const outcome = await withTransientSpawnRetry(getFfprobeBinary(), () => spawnAndCollectFfprobe(args, signal), (result) => result.reason === "spawn_error" ? ... : undefined). Confirmed.All three genuinely call the helper.
ffSpawnRetry.test.ts(13/13, ran myself) and the mutation test (neutralized theisTransientSpawnErrnoguard inhandleThrownSpawnError— the "never retries permanent failures" test caught it, 1 call became 3) confirm the helper's own branching is non-vacuous. No test exercises the retry through the realaudioExtractor/audioPadTrimcall sites specifically (only the generic helper and the pre-existing regression suites, both green —audioPadTrim.test.ts37/37 viabun test,runFfmpeg.test.ts18/18,ffSpawnRetry.test.ts13/13), which is acceptable given the wiring is trivial and directly readable, but it means the only thing proving the site-level integration is this review, not CI.
6. Commit is unsigned (gh api .../commits/80226960 --jq .commit.verification → verified: false, reason: "unsigned"). Flagging per the org ruleset as asked — not acting on it.
Verdict: REQUEST CHANGES
Reasoning: The mechanism is well-built where it's applied — explicit spawn-vs-post-spawn distinction, correct errno exclusion, observable retries, non-vacuous unit tests — but it's applied to 3 of at least 6 confirmed managed-binary spawn sites, two of which (ffprobe.ts, streamingEncoder.ts) are likely hotter paths than the ones fixed. Combined with an unconfirmed root-cause errno (no real Windows repro, and Windows AV lockouts commonly surface as EPERM rather than the classified EBUSY/ETXTBSY), this needs either a real repro confirming the errno, or the same helper applied to the remaining sites (or an explicit, reviewable reason they're excluded), before it's ready to merge.
— Miga
|
Closing as stale after a few weeks without activity. Thanks for the contribution; feel free to reopen after rebasing on main. |
Summary
Fixes #4058
On Windows,
spawnof the managedffmpeg/ffprobebinaries can fail transiently withEBUSY/ETXTBSY— typically antivirus real-time scanning a just-executed exe, or a lingering handle from a concurrent sibling invocation (one render shares a single managed ffmpeg binary across many stages). Today a single transient failure fails the whole render with a barespawn EBUSYand no retry.This adds a shared engine helper,
withTransientSpawnRetry, and adopts it at the three spawn sites:runFfmpeg(engine): retries when theManagedChildProcessoutcome is aspawn_errorcarrying a transient errno.audioExtractor.runFFmpeg(producer): retries a rejected transient spawn error.audioPadTrim.runFfprobeJson(producer): retries a result-shapedspawn_erroroutcome.Behavior
EBUSY,ETXTBSY,EAGAIN,EMFILE,ENFILE— retried with exponential backoff (250 ms base, up to 3 attempts total); each retry is logged with the binary path and errno.ENOENT,EACCES, …) pass through on the first attempt, unchanged.spawn()call, so a healthy spawn keeps its exact synchronous timing — no changes to existing fake-timer test choreography.Test plan
packages/engine/src/utils/ffSpawnRetry.test.ts, 13 cases): errno classification, backoff math, message enrichment, result-shaped vs rejected failure retry, exhaustion enrichment, permanent-failure pass-through.runFfmpeg(incl. thewindowsHideregression) andchunkEncodertimeout suites pass unchanged.audioPadTrimsuite passes (37/37).tsc --noEmitclean for@hyperframes/engineand@hyperframes/producer.