Repository navigation
fix(cli): --timeout on check and snapshot takes units like 45s - #6521
Conversation
…eout takes units The copy phase of check had no overall limit: a source that kept moving was never stopped, and stdout stays silent the whole time. It now ends after 10 minutes with an error naming the files still unfinished and the elapsed seconds; finished copies are reused on the next run. The stderr progress lines are unchanged. --timeout on check and snapshot is milliseconds, so "--timeout 45" waited 45 ms. It now accepts 45s, 2m and 45000ms, and a bare number under 1000 warns on stderr that it is milliseconds. Output on stdout is unchanged.
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 blocker · 0 important · 1 nit · head f444828f
The 10-minute wall clock can fail a long copy that is still making progress, and running check again does not help. ↓ inline
Scope
Read the whole diff, plus resolveProxies/joinJob/leave in studio-server proxyTranscoder.ts, checkDeadline.ts, and how errors reach the user from runCheckPipeline and runPrepareMedia. Both changed test files pass locally (18/18). CI was still running when I reviewed. The --timeout parser looks right.
Nit: check --timeout 45 warns "45 is 45 ms", but parseCheckOptions then clamps it to 500 ms, so the warning names a value that is not used.
— Claude
| `Making browser-playable copies took longer than ${elapsedText(COPY_PHASE_CAP_MS)} and was stopped after ${Math.round((Date.now() - startedAt) / 1000)}s. Still unfinished: ${unfinished.join(", ")}. Re-run check to keep going (finished copies are reused), or pass --no-proxy to skip them.`, | ||
| ), | ||
| ); | ||
| }, COPY_PHASE_CAP_MS); |
There was a problem hiding this comment.
blocker: This adds a wall-clock limit to copies that are still making progress. The transcoder was designed to avoid that. resolveProxies says "A copy is ended only when it stops making progress (TRANSCODE_STALL_MS), so a long source that keeps moving is never cut by a clock", and CheckDeadline marks copies as before the deadline on purpose.
When the limit fires, stopped.abort() → joinJob → leave() → job.abandoned.abort() kills ffmpeg and throws away the partial copy. Each unfinished file then starts again from zero on the next run. So:
- one source whose copy takes more than 10 minutes (a long 4K ProRes, a slow disk, a 2-vCPU runner) can never pass
check. The message's "Re-run check to keep going" can't come true for that file. - the docstring above names a fresh CI checkout as the main cold-cache case. A CI checkout starts with an empty cache every time, so a project that took 12 minutes before now fails every run, where it used to pass slowly.
- the limit also counts time spent waiting in the shared transcode queue, so a preview running at the same time eats into it.
A silent stdout is a better problem to have than a check that can never pass. Options: (a) keep the stall-only design and fix the silence; the stderr heartbeat already shows progress. (b) Fire only when nothing has made progress for N minutes (reset on heartbeat.progress), which is what TRANSCODE_STALL_MS already does per copy. (c) Make the limit opt-in or configurable, defaulting to off. If you keep a hard limit, the error should not tell the user that re-running will make progress.
A copy that keeps making progress must not be cut by a clock, and a re-run would only start it over. The copy already ends when ffmpeg stops reporting progress for 3 minutes (finished pieces are kept for the next run), so the phase is bounded on no progress. The --timeout warning now names the value actually used when check raises a small value to its 500 ms floor.
miguel-heygen
changed the title
Edit accuracy: accurate 271 (base branch 271), smooth 135 of thoseThe gate passes. Quarantined, measured but not gated (0) |
There was a problem hiding this comment.
Verdict: APPROVE — 1 resolved, 0 open, 0 new · f444828f → c4c8e64d. The wall clock is gone, so this is a units fix only. The floor is now named in the warning. Changed suites pass locally (220/220). CI is green; I re-ran the failed Test (cli) job, an unrelated Chromium launch timeout in motion-signature.browser.
— Claude
This is a units fix only. It does not find the root of the silent 90 s to 4 minute hangs: the copy phase is already progress-bounded, so it does not explain them, and the hang root is still open.
What changes for the user
--timeoutoncheckandsnapshotis milliseconds, so--timeout 45waited 45 ms. It now also takes45s,2mand45000ms. A bare number under 1000 is still milliseconds but warns on stderr, naming the value actually used (checkraises anything under 500 ms to 500 ms). Stdout is unchanged.Copy phase: no new limit
An earlier version of this PR capped
check's video copies at 10 minutes of wall time. That is dropped: it could kill a copy that was still progressing, and a re-run would start it over. The copy phase is already bounded on no progress: each ffmpeg copy ends when it reports no progress for 3 minutes (TRANSCODE_STALL_MS, adjustable withHYPERFRAMES_PROXY_STALL_MS), a long source is copied in 15 s pieces and finished pieces are kept for the next run, and the stderr heartbeat names each copy's progress. A copy that stalls shows up as the existing "Could not make a browser-playable copy of " finding.Measured: a 676 MB 4K ProRes source spends 10.6 s in the copy phase, and stdout stays silent until the command exits. Minutes of silence therefore need bigger or longer media or a slow disk.
How it is checked
timeoutArg.test.tscovers bare,ms,s,m, fractional, zero and junk values, the warning for small bare values, and that the warning names the floored value; the floor test fails when the floor is not applied. 3 green runs.Why under 100 lines
One small helper (
timeoutArg.ts, 28 lines), its test, and a one-line call in each ofcheckandsnapshot. It is a single change (how--timeoutis read), so splitting it would leavecheckandsnapshotdisagreeing on the unit.