Review feedback on #3805.
captureFrameToBufferPipelined checked isRecoverableDrawElementError and
threw DrawElementCaptureError before reaching captureFrameErrorDiagnostics,
so the NCPR/canvas failures that now abort the whole attempt produced no
frame-error PNG/HTML/JSON bundle — the exact case worth debugging, and the
one the adjacent comment promised mirrored the serial path. The serial path
was unaffected because its own DrawElementCaptureError throw propagates
through captureFrameCore's outer diagnostics catch.
Run diagnostics first, then the recoverable wrapper. Bounded to at most one
bundle per attempt, since a recoverable error fails the whole attempt, and
captureFrameErrorDiagnostics self-catches, so a dead page cannot mask the
structural error the producer's fresh-page retry depends on.
Covered by a new test asserting the bundle lands for a recoverable pipelined
failure; verified by mutation (restoring the old order fails it).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(build): stop generated files from being read half-written during the build
Two unrelated pull requests kept failing CI for reasons that had nothing to do
with their changes.
Typecheck failed with "TS1002: Unterminated string literal" pointing at a
generated file. The root build compiles several packages at the same time, and
more than one of them regenerates files under a package's src/generated while a
sibling's tsc is already reading them. A plain write empties the file and then
refills it, so for a few milliseconds what is on disk is the first half of the
new file. Whichever build read it in that gap saw a truncated file and stopped.
It never happened locally because locally nothing else is reading.
Every generator now writes the new version under a temporary name and then
renames it over the target, which the filesystem does in one step. A reader
either gets the whole previous version or the whole new one; there is no moment
where it can see half of either. Content that has not changed is not rewritten
at all, so repeat builds no longer touch these files. Four generators were
affected and all four now go through one shared helper, which is where the rule
lives from now on.
The build also rebuilt the core package a second time, in parallel with the
packages that read it. That second rebuild was already redundant, and removing
it takes the writers out of the window entirely. Renaming is what makes the file
safe; dropping the duplicate build makes the build shorter as well.
Separately, a linter performance test failed on four of the last ten failed runs
on main. It timed a scan with a stopwatch and demanded the result come in under
two seconds; one run took 3.7s. That is a statement about how busy the shared
runner was, not about the code, because a stopwatch also counts the time the
machine spent running someone else's job. The test now counts only the processor
time this process actually used, and checks that the cost grows in step with the
input rather than against a fixed number of milliseconds. Measured on a machine
under load, the stopwatch ratio for the same input reached 45x while the
processor-time ratio stayed at 20x.
Both fixes come with a check that fails if the fix is removed.
* fix(build): keep the shared write helper inside the core package
The helper the generators call had been placed in the repo-root scripts folder.
Every container image that builds a package copies the packages folders whole
but cherry-picks root scripts one file at a time, so the image builds failed on
a missing module. The repo already shows both halves of that convention: the one
root script a package build imports has a matching copy line in the image, and
cross-package imports into the core package need none.
The helper now lives with the core package's other build scripts, and the one
generator outside that package reaches it the way the engine package already
reaches core.
* test(engine): keep the probe cache bound test off the filesystem
The eviction test wrote, stat'ed and deleted 129 temp files to exercise an
in-memory LRU rule. Its runtime tracked filesystem contention rather than
the code under test, and on a busy Windows runner the file churn alone
pushed a ~300ms test past the 5s timeout, failing unrelated pull requests.
Cache identity comes from stat, so the test now synthesises stat results
and never touches disk. While here, the test also asserts the LRU touch:
re-probing an entry before the bound is hit must keep it resident and
evict the next-oldest one instead. The previous shape never hit the cache
during the fill, so that branch was untested.
* test(lint): keep the scaling check inside the scanner's linear range
The CPU-ratio version sampled 320k characters, where the output string's
own growth dominates and the whole test cost seconds of CPU; on a shared
runner that tripped the default test timeout, the failure this change
exists to remove. Sample 10k and 80k characters instead, where 8x input
measures ~8x and a quadratic scan still measures 60x or more, and give
the test an explicit timeout so a slow runner reports the ratio.
* test(core): snapshot file identity through a descriptor
The before/after inode and mtime checks read the file through a path stat
and then exercised the writer on the same path, which reads as a
check-then-use race to static analysis. Snapshot through an open
descriptor instead; the assertions are unchanged.
The static-frame dedup predictor walked tween intervals from
window.__timelines but was blind to onUpdate callbacks — motion driven
from a timeline's onUpdate in a tween-free window was predicted static.
The verifier compounded the gap by seeking with suppressEvents: true,
so the onUpdate never fired and the frozen frame passed verification.
Two changes:
1. Predictor: when a timeline carries vars.onUpdate, mark its full span
as animated so those frames are never predicted static.
2. Verifier: seek with suppressEvents: false so the verification page
behaves identically to the capture page. The verification page is
already isolated (separate Page instance), so out-of-order event
side effects cannot corrupt sequential capture.
Fixes#3793
* test(producer): pin unpinned drawElement stall retry
* fix(engine): bound drawElement frames so a wedged renderer falls back instead of failing (#3172)
* fix(engine): bound drawElement frames so a wedged renderer falls back instead of failing
A single drawElement frame could kill an entire render. On one comp
(caption-editorial-emphasis) drawElementImage returns normally and the renderer
then stops draining its task queue: the setTimeout(…, 0) that schedules
toDataURL never fires, the capture page.evaluate never settles, and 60s later
the stage watchdog fails the whole render. Deterministic on 152.0.7977.30,
always the same frame. Root-cause detail in PRINFRA-488.
Two gaps, both closed here.
1. Nothing below the 60s stage watchdog bounded a frame. Adds a per-frame
deadline (HF_DE_FRAME_TIMEOUT_MS, default 15000, 0 disables) around the WHOLE
frame operation, not just the drawElementImage call — the stall surfaces at
whichever page round-trip comes next, and was observed at both the capture
and the seek's background-image decode. Bounding one call missed it.
Deliberately NO per-frame screenshot fallback for this error: once the
renderer stops scheduling it is wedged for every subsequent round-trip on
that page. Measured — the screenshot fallback blew the same deadline. The
frame fails fast instead, so the recovery that actually works can run.
2. That recovery was ineligible. shouldRetryViaPinnedFallback only retried a
generic capture failure when the worker count was PINNED by the inversion or
the router; a comp that engaged drawElement on the ordinary single-worker
path had no whole-render fallback at all. A renderer stall is now retryable
on any routing — the failure is a property of drawElement itself, and the
retry re-renders on a fresh page via screenshot. Reported as
de_fallback_reason "de_renderer_stall" so it is distinguishable from
capture_error in telemetry, and counted per-session as deFrameTimeouts.
Verified end to end on the repro: previously exit 1 with "stalled: no frame
progress for 60000ms"; now the deadline fires, the render retries via
screenshot, and RENDER_OK with a valid 1920x1080 / 240-frame / 8.0s MP4.
Tests cover the predicate both ways plus the cross-package error match. Engine
1,481 and producer 587 green (audioPadTrim.integration flakes only under
full-suite parallel load — passes in isolation, and on clean HEAD it is green
too, so it is not from this change).
Refs PRINFRA-488
* fix(engine,producer,cli): count drawElement frame timeouts where they happen
`session.deFrameTimeouts` was incremented inside `captureFrameCore`'s catch,
but the deadline races that function from OUTSIDE it — `withFrameDeadline` is
applied by `captureFrameToBuffer`, and `DeFrameTimeoutError` is constructed
nowhere else. So the branch was unreachable: on a wedged renderer the counter
stayed 0, the engine-side stall log never printed, and
`CapturePerfSummary.deFrameTimeouts` reported 0 on every affected render. The
end-to-end recovery worked throughout — `isDeRendererStallError` matches by
name at the producer — only the observability was dead.
Moved the increment and the log into an `onTimeout` hook that fires from the
deadline itself, deleted the unreachable branch and the now-unused
`isDeFrameTimeoutError`, and threaded the count out to
`render_completed.de_frame_timeouts` so the rate is graphable apart from the
other `capture_error` fallbacks. A stall always costs a whole-render re-run,
which is why it deserves its own series.
Also corrected the `fallbackReason` docstring, which still listed only
`psnr | blank | oom | capture_error` after this branch added
`de_renderer_stall`.
`withFrameDeadline` is exported for a two-case fake-timer test: the deadline
fires and calls `onTimeout` once, and a resolving inner promise passes its
value through with no late timer. The first case fails if the counter moves
back inside the work promise.
(cherry picked from commit ec2b2b5b2a)
* fix(render): retry sequential screenshot stalls
* fix(render): bound parallel worker capture phases
---------
Co-authored-by: Vance Ingalls <vance@heygen.com>
* fix(producer): transport safe extraction failure metadata
* refactor(producer): generalize public error metadata
* test(producer): use vendor-neutral media hosts
The ENAMETOOLONG regression writes 150 real clip files and the mixer
existence-checks each one: ~58ms on Linux, but past vitest's 5s default on
the Windows lane. packages/engine sets no global testTimeout, so heavy
tests here carry an explicit one.
On timeout its abandoned async work kept calling the shared runFfmpegMock
after afterEach cleared it, so the next test saw 5 calls instead of 3 and
lost its queued once-implementations to the leak. mockReset stops an
aborted test from handing leftovers to the next one.
When a composition script throws during execution, the GSAP timeline
registration never arrives and pollSubCompositionTimelines times out.
Previously the render continued with a degenerate 2-frame output and
reported success — now it fails loudly.
Two changes:
1. Detect composition script runtime errors in the browser console
handler and feed them into scriptLoadFailures, triggering the
existing fail-fast path (same as script load 404s).
2. Make sub_timeline_script_failure a fatal warning in
applyRenderWarningPolicy, alongside audio_processing_failed.
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(producer): assert render artifact duration and frame count before commit
Refuse to publish an artifact that is significantly shorter or has fewer frames
than the capture pipeline just reported. Adds a duration/frame-count gate on top
of the existing readable-non-empty check inside ArtifactTransaction.validate(),
keyed off the values the orchestrator already carries. Closes#3395.
* fix(producer): wire ffprobe frame count into the artifact duration probe
The frame-count gate added in #3395 accepts an expectedFrames value from
the orchestrator, but defaultArtifactDurationProbe was still returning
only durationSeconds - so the wire was half-built and the assertion
short-circuited on undefined for every real render. Forward meta.frames
from ffprobe so the field-packet case the issue names (container duration
correct, stream shorter) is actually caught by the frame-count check,
not just the duration one.
extractMediaMetadata now populates a new frames field from the video
stream's nb_frames tag, returning undefined when the demuxer did not
report one (fragmented MP4, malformed streams, muxes that require
-count_packets). Callers that gate on the count must treat undefined as
no answer; the assertion already does.
The previous CI run (#32589981916) cancelled shard-6 at the 1h job
timeout after bun install failed to extract the aws-cdk-lib tarball
mid-Docker-build - a cache flake, not a code regression. Pushing a
follow-up commit retriggers CI against the now-populated cache layer;
the regression should clear without further code changes.
---------
Co-authored-by: Santhi Prakash <b.santhiprakash@gmail.com>
`muxVideoWithAudio` passed `-avoid_negative_ts make_zero` unless the caller
set `preserveAudioPrimingEditList`. In practice the dominant path is an AAC
sidecar copied into mp4, where that flag is actively harmful: ffmpeg's
default is `auto`, which the mp4/mov muxers (AVFMT_TS_NEGATIVE) already
resolve to `disabled`. Forcing `make_zero` overrides the correct default,
discards the priming edit list the sidecar encode created, shifts the video
start_time forward by one AAC frame and writes an empty video edit at t=0 —
which edit-list-honoring players (QuickTime/Safari) render as a black first
frame.
Verified with ffprobe on a copy mux of a 30fps h264 mp4 and an AAC sidecar:
with `make_zero` video start_time 0.066000, elst: [media time -1,
dur 5940] + [media time 6000, dur 180000]
audio start_time 0.042993, elst: [media time -1, ...]
without (this fix) video start_time 0.000000, elst: [media time 6000,
dur 180000]
audio start_time 0.000000, elst: [media time 1024, ...]
The empty leading edit and the offset both disappear, and the audio keeps
its 1024-sample priming edit.
The flag is now never passed for a mux, in any mode. `preserveAudioPrimingEditList`
is part of the exported engine API, so it stays on `MuxVideoWithAudioOptions`
as `@deprecated` and no-op rather than being removed; the two internal callers
that set it (`assembleStage`, distributed `assemble`) drop it.
`buildEncoderArgs` and `streamingEncoder` still pass the flag for video-only
output and are deliberately left alone — those chunks are consumed as
intermediates, not as a delivered mp4/mov.
Fixes#3487
Co-authored-by: Alexandru Mincu <alex@mountsoftware.ro>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Miguel Ángel