fix(engine): carry frameStride onto WorkerResult#2679
Conversation
…orker false-positive)
miga-heygen
left a comment
There was a problem hiding this comment.
SSOT Review: PRINFRA-300 rendering fixes — frameStride on WorkerResult + live map warning
Combined review: #2679 (2 files, +83/-10) + #2681 (3 files, +75/-1)
SSOT inventory
#2679 — carry frameStride onto WorkerResult
| Concept | Source of truth | Consumers | Verdict |
|---|---|---|---|
frameStride on WorkerResult |
WorkerResult.frameStride (optional, default 1) |
expectedFramesForTask, flagSilentWorkerExits |
Single owner |
| Silent-exit flagging | flagSilentWorkerExits (extracted from inline loop) |
executeParallelCapture |
Single owner |
| Expected frame count | expectedFramesForTask |
flagSilentWorkerExits |
Single owner (pre-existing, already stride-aware) |
#2681 — live map viewport warning
| Concept | Source of truth | Consumers | Verdict |
|---|---|---|---|
| Live map CSS markers | LIVE_MAP_MARKERS ReadonlyArray |
recordLiveMapWarning |
Single owner |
| Warning construction | buildLiveMapWarning |
recordLiveMapWarning |
Single owner |
| Warning code | "live_map_detected" in CaptureWarningCode union |
buildLiveMapWarning |
Single owner |
What I checked
-
frameStridecarried in both return paths. Success path (line 513) and catch path (line 536) both setframeStride: task.frameStride. Without this, the field would beundefined, andexpectedFramesForTaskwould fall back to stride=1, computing the full contiguous range and false-positive flagging every interleaved worker. Correct. -
flagSilentWorkerExitsextraction. The inline loop inexecuteParallelCaptureis extracted verbatim into an exported function. Same logic: skip results with existing errors (!r.error), compareframesCapturedagainstexpectedFramesForTask(r), synthesize error string if under-captured. The inline loop is replaced with a singleflagSilentWorkerExits(results)call. Correct — behavior preserved, now testable. -
Test: fully-successful interleaved worker. 3-way interleave over 150 frames: worker 0 captures 50 frames (0, 3, 6, …, 147) at stride=3. Expected = ceil(150/3) = 50.
framesCaptured === expected→ no error. This is the PRINFRA-300 regression case. ✓ -
Test: genuinely under-captured worker. Same stride=3, but only 20 of 50 frames captured. Error synthesized with
expected=50. ✓ -
Test: existing error not overwritten. Worker with
erroralready set →!r.errorguard preserves it. ✓ -
frameStrideis optional (?: number) — contiguous workers (stride 1 or undefined) continue working becauseexpectedFramesForTaskpresumably defaults to stride 1 when absent. The PR body confirms "expectedFramesForTask correctly divides by frameStride" was already correct — the missing piece was the data, not the math.
-
CSS selector detection, not script detection.
LIVE_MAP_MARKERSuses selectors like.leaflet-container,.mapboxgl-map, etc. — these are class names that map libraries stamp on their live viewport containers. A composition that baked its map to video first won't have these elements in the DOM, so no false positive. Correct granularity. -
Both init paths.
recordLiveMapWarning(session, page)is called at both session init sites (regular screenshot path, line 2049, and BeginFrame path, line 2212). Correct — both capture modes need the warning. -
Array copy.
buildLiveMapWarningstores[...libraries](spread copy) so later mutation of the input array doesn't alter the recorded warning. Test: push to input array after build, warning'sdetails.sourcesunchanged. ✓ -
Warning code registered.
"live_map_detected"added to theCaptureWarningCodeunion type intypes.ts. Type-safe at all downstream consumers. ✓ -
Warning message is actionable. Points the author at the specific fix: "see the motion-graphics maps skill / bake-basemap.mjs". Not just "this is broken" but "here's how to fix it." Good.
Blocking SSOT issues
None found.
Verdict
Approve both. #2679 fixes the immediate regression (56% revert rate on v0.7.60+ due to missing frameStride on WorkerResult causing every interleaved worker to be misclassified as a silent death). #2681 gives the PRINFRA-300 failure class a voice — live map viewports now produce a live_map_detected warning at session init instead of silently shipping blank/partial map frames. Clean complementary fixes: one stops the false-positive revert cascade, the other prevents future silent map corruption.
--- Miga
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R1 against 4f53dd4f2cd607de17b2d2329746fa5da0cadeaa. Reviewed alongside its sibling #2681 (both off main, coordinated pair for PRINFRA-300 but no merge-order dependency between them).
The fix reads clean and the fleet telemetry (56% revert rate on 0.7.60–0.7.64 vs ~2.4% baseline) matches the shape of "every interleaved worker false-positives once the guard shipped" — that's the smoking gun. Also verified:
Writer end — both return paths. executeWorkerTask sets frameStride: task.frameStride on the success return at parallelCoordinator.ts:516 and the catch return at parallelCoordinator.ts:539. No third exit path.
Reader end unchanged. expectedFramesForTask at parallelCoordinator.ts:171-178 was already stride-aware — its shape parameter ({ startFrame; endFrame; frameStride? }) is polymorphic across WorkerTask and WorkerResult, so once the field lands on the result the same computation gives the right expected count. The math was right; the data was missing.
Bonus fix. formatWorkerFailure at parallelCoordinator.ts:224 also calls expectedFramesForTask(result) for its fallback message. It was silently broken with the same stride=1 fallback pre-PR when synthesizing failure text for an errored interleaved result — this PR fixes that too as a side effect. Not called out in the PR body but worth knowing.
No stray consumers. WorkerResult is only re-exported via packages/producer/src/services/parallelCoordinator.ts:12 as a type — no observability serializer or telemetry emitter reads frameStride off it. The added optional field can't break downstream shape.
Extraction of flagSilentWorkerExits. Verbatim lift of the inline loop from executeParallelCapture — same guard (!r.error), same comparison, same synth. Now unit-testable, which is exactly the shape the three new tests exercise.
Nit
The third test case at parallelCoordinator.test.ts:291-302 (existing-error preservation) omits frameStride on the fixture. That's fine because !r.error short-circuits before expectedFramesForTask runs — but a future reader may wonder. If you want to lock the invariant more sharply, adding frameStride: 3 to that fixture makes it symmetric with the other two and doesn't change behavior.
What I didn't verify
- Behavior in a real interleaved parallel render — the unit test is unit-shape; end-to-end trust is Vance's telemetry claim.
LGTM from my side, sibling review at #2681 covers the same day's guard. Stamp routing continues to sit with James.
jrusso1020
left a comment
There was a problem hiding this comment.
Approve — own pass, verified at source. The fix carries frameStride: task.frameStride on BOTH worker-exit return paths (parallelCoordinator.ts:124 success + :132 catch), and WorkerResult now types the field — so expectedFramesForTask stops falling back to stride=1, computing the full contiguous range, and misclassifying a fully-successful interleaved worker as a silent death (the 56%-revert cliff on v0.7.60+). flagSilentWorkerExits is cleanly extracted and the regression test pins the exact 3-way-interleave / 50%-frames scenario with no false positive. Tight scope — 2 files, no creep. CI green.
— Jerrai
Summary
expectedFramesForTaskcorrectly divides byframeStridefor interleaved parallel workers, butWorkerResultnever carriedframeStridein the first place — both return paths inexecuteWorkerTask(success and catch) omitted it. So the silent-worker-exit guard added in971bcf39a(v0.7.60) always computed the expected frame count as the full contiguous range instead ofrange/stride.Net effect: every fully-successful interleaved worker gets misclassified as a silent death, and the whole parallel capture throws after 100% of frames were captured correctly.
Impact (confirmed via fleet telemetry, not just local repro)
de_parallel_routerrevert rate by CLI version — the cliff lands exactly on the release that shipped the guard:Also 100% reproducible locally with
HF_CAPTURE_PARALLEL_STREAM=true/HF_DE_PARALLEL_STREAM=true(any interleaved multi-worker render) — loggedframesCompleted: 150/150immediately before the synthetic "worker exited without terminal error string" failure.Related: PRINFRA-300 (this fixes part of that ticket's failure cluster — the "silent worker exit, no terminal error string" reports — but not the ticket's primary blank-hook-frame symptom, which is a separate, still-open issue on the non-interleaved disk path).
Fix
WorkerResultgains an optionalframeStridefield mirroring the originatingWorkerTask.executeWorkerTaskreturn paths (success, catch) now populate it.flagSilentWorkerExits(results)so it's unit-testable without a real browser session.Test plan
parallelCoordinator.test.ts: a fully-successful interleaved worker is no longer flagged; a genuinely under-captured interleaved worker is still flagged; an existing error is never overwritten.bun test parallelCoordinator.test.ts— 37/37 passtsc --noEmitcleanoxlint+oxfmt --checkclean🤖 Generated with Claude Code