fix(media): bound the ffmpeg tool's child processes, inputs and scale targets - #6989
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview Every operation now shares a 10-minute wall-clock deadline; timeout and explicit user cancel both The handler also caps 20 inputs, checks recorded file size before download, aborts between input fetches, and refuses oversized outputs before buffering. Limits live in Reviewed by Cursor Bugbot for commit 0dee35b. Configure here. |
|
Greptile SummaryThe PR bounds in-process FFmpeg work with operation deadlines, cancellation, input/output limits, and validated scale targets while synchronizing the generated Copilot schemas.
Confidence Score: 4/5The PR needs a cancellation-plumbing fix before merging because an explicit Stop cannot interrupt an in-flight input download. The child-process cancellation path is bounded, but input preparation still performs non-cancellable storage work after its single pre-iteration abort check, so a stopped turn can continue a slow or large current download until completion. Files Needing Attention: apps/sim/lib/copilot/tools/server/media/ffmpeg.ts
|
| Filename | Overview |
|---|---|
| apps/sim/lib/copilot/tools/server/media/ffmpeg.ts | Adds input-count, recorded-size, and cancellation checks, but cancellation cannot interrupt the current input download. |
| apps/sim/lib/media/ffmpeg.ts | Adds shared operation deadlines, process termination, bounded probing and output reads, and corrected scale-area enforcement. |
| apps/sim/lib/media/ffmpeg-limits.ts | Centralizes input-count and frame-dimension limits used by runtime enforcement and schema parity. |
| apps/sim/lib/media/ffmpeg.test.ts | Covers process bounds, output limits, scale validation, and concat frame clamping, including prior rounding counterexamples. |
| apps/sim/lib/media/ffmpeg-schema-parity.test.ts | Ensures generated tool schemas remain synchronized with runtime FFmpeg limits. |
| apps/sim/lib/copilot/generated/tool-catalog-v1.ts | Updates generated FFmpeg parameter descriptions and structural limits. |
| apps/sim/lib/copilot/generated/tool-schemas-v1.ts | Mirrors FFmpeg input-count and dimension bounds into runtime validation schemas. |
Sequence Diagram
sequenceDiagram
participant User
participant Handler as FFmpeg server tool
participant Storage
participant Media as Media FFmpeg runner
participant Child as ffprobe/ffmpeg
User->>Handler: Invoke media operation
loop Each input
Handler->>Handler: Check abort signal
Handler->>Storage: Resolve and download input
end
Handler->>Media: Run with shared deadline and signal
Media->>Child: Start bounded child process
alt User stops during child execution
User->>Handler: Abort
Media->>Child: SIGKILL
else Deadline expires
Media->>Child: SIGKILL
else Operation completes
Child-->>Media: Output
Media->>Media: Enforce output-size limit
end
Reviews (3): Last reviewed commit: "fix(media): floor the scaled concat axes..." | Re-trigger Greptile
… targets The copilot ffmpeg tool shells out to FFmpeg in the Sim app process — not in a sandbox — and nothing bounded the run. runCommand had no timer, no kill and no signal, so a transcode ran until it finished and survived the request that asked for it: a stopped copilot turn left the encode pinning both cores of a shared instance. Every operation now shares one 10-minute wall-clock deadline, and both the deadline and the caller's cancellation SIGKILL the child. The budget is per-operation rather than per-command because concat runs a full re-encode per input, so a per-command timeout would let N inputs multiply into N timeouts. Cancellation is wired to context.abortSignal, whose every abort reason is an explicit user stop — the copilot lifecycle tracks a passive client disconnect separately and does not abort on it — so an encode dies when the user asks and not before. (userStopSignal, which assertServerToolNotAborted reads, has no producer on this path.) ffprobe had the same shape in miniature: fluent-ffmpeg's static ffprobe hands back no process handle, so a timeout could only race the callback and leave a wedged prober alive, and concat probes once per input. It now runs through execFile, which takes timeout, killSignal and signal natively. Scale targets reached the filter graph unvalidated, where libavfilter sizes its per-frame buffers from them — scale=30000:30000 is ~2.7 GB a frame, allocated in a child that shares the instance's memory. Dimensions are now rejected outside 16-4096 with a message the model can act on, plus an area cap. concat's targets come from the source container rather than a caller assertion, so those are clamped instead. Two further holes: readOut buffered the output with no ceiling, and CRF-18 re-encodes routinely exceed their input, so the input budget did not bound it; and the handler accepted unlimited input files. Both are capped, and the byte check now runs against the recorded size before the download rather than after. FFMPEG_LIMITS is the single source for the four numbers the Go tool catalog mirrors into its schema, pinned by ffmpeg-schema-parity.test.ts so the model is never told a ceiling the executor does not enforce. Companion: simstudioai/copilot#PENDING Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rep on cancel Two review findings, both real. concat clamped each probed axis independently, so a 4096x4096 source produced a normalization target of exactly that — nearly double the area budget scale_pad enforces through the same scale=/pad= graph. The pair is now scaled down together, so aspect ratio survives and one ceiling governs both entry points. Dimensions come back even too, which yuv420p requires and per-axis rounding did not guarantee. Preparing the inputs is itself the expensive half of a many-file call — up to the whole byte budget in storage reads — and the abort signal was only consulted after the loop, so an explicit stop was observed only once every download had already finished. The loop now checks it per input. Also corrects the rationale comments on the mirrored limits. maxItems/minimum/ maximum do not reach the model: copilot's NormalizeToolParameters allowlists type/properties/items/description/enum/required, so the bounds travel only the contract path, where Sim's router validates against them. That is still worth having — it turns an out-of-range argument into a structured rejection before any storage read or child process — but the model learns the limits from the parameter descriptions, and the comments now say so instead of claiming the schema teaches it.
2361d23 to
627509f
Compare
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 627509f. Configure here.
…olds The scale factor lands both axes on a product of exactly the budget, so an axis allowed to round up can put the pair back over it — and when both round up and both land even, nothing pulls them back. A brute force over every dimension pair in 16..4096 finds 219,280 that violate the bound under round-to-even, worst 2694x3520 -> 2688x3512, 3072 pixels over. Under floor-to-even: none. Flooring keeps each axis at or below its exact target, so the product cannot exceed the budget. Probed dimensions are already integers, so this changes only the scaled path.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 0dee35b. Configure here.
Problem
The copilot
ffmpegtool shells out to FFmpeg in the Sim app process — not in a sandbox (docker/app.Dockerfileinstalls the binary for exactly this) — and nothing bounded the run.runCommandhad no timer, no.kill()and no signal, so a transcode ran until it finished and survived the request that asked for it: a stopped copilot turn left the encode pinning both cores of a shared instance.scale=30000:30000reached libavfilter unvalidated, which sizes its per-frame buffers from those numbers (~2.7 GB a frame) inside a child that shares the instance's memory.Change
Deadline + kill. Every operation shares one 10-minute wall-clock deadline; both the deadline and the caller's cancellation
SIGKILLthe child. Per-operation rather than per-command, becauseconcatruns a full re-encode per input — a per-command timeout would let N inputs multiply into N timeouts.Cancellation is wired to
context.abortSignal. Every abort reason on that controller is an explicit user stop (UserStop/RedisPoller/MarkerObservedAtBodyClose); the copilot lifecycle tracks a passive client disconnect separately aspublisher.clientDisconnectedand does not abort on it. So an encode dies when the user presses stop and never otherwise.ffprobe had the same shape in miniature.
fluent-ffmpeg's staticffprobehands back no process handle, so a timeout could only race the callback and leave a wedged prober alive — andconcatprobes once per input, so the leak scaled with the request. Now runs throughexecFile, which takestimeout,killSignalandsignalnatively.Scale targets are rejected outside 16–4096 with a message the model can act on, plus an area cap.
concat's targets come from the source container rather than a caller assertion, so those are clamped instead of rejected.Two holes not in the original report:
readOutbuffered the output with no ceiling (CRF-18 re-encodes routinely exceed their input, so the input budget did not bound it), and the handler accepted unlimited input files. Both capped; the byte check now runs against the recordedsizebefore the download rather than after.Single source of truth
FFMPEG_LIMITSholds the four numbers the Go tool catalog mirrors into its schema, pinned byffmpeg-schema-parity.test.ts. Without it, changing a limit here would leave the model reading a stale ceiling off its own schema — silent, and user-visible as "the tool said 4096 but refused at 2048". Verified non-vacuous: flippingmaxInputFilesto 21 andmaxScaleDimensionto 2048 fails it on exactly those assertions.Scope note on the generated bindings
Regenerating the catalog the obvious way pulls in
load_slide_layout, an unrelated tool from copilot7e741a39(house deck system / slide-layout library) that this branch has not synced. The bindings here were generated from the copilot commit these files already matched, plus only this change — proven by first regenerating from the unmodified baseline contract and confirming a byte-identical match with the committed files. The diff touches exactlyFfmpeg,GenerateAudio,GenerateImage,GenerateVideo; zero slide-layout leakage. That feature syncs whenever someone brings it over.Verification
tsc --noEmit,lint:check, 2586 tests acrosslib/copilot+lib/media+lib/uploads+lib/workspace-files, andcheck:api-validation/boundaries/client-boundary/utils— all pass, rebased onto latest staging (which includes #6986's provenance change to this same file).Companion
Companion: https://github.com/simstudioai/mothership/pull/447
That PR carries the schema side — the tool-catalog wording and the
maxItems/minimum/maximumthis branch mirrors. Merge in lockstep.🤖 Generated with Claude Code