Skip to content

fix(engine): carry frameStride onto WorkerResult#2679

Merged
vanceingalls merged 1 commit into
mainfrom
07-21-fix_engine_carry_framestride_onto_workerresult_fixes_interleaved_worker_false-positive_
Jul 21, 2026
Merged

fix(engine): carry frameStride onto WorkerResult#2679
vanceingalls merged 1 commit into
mainfrom
07-21-fix_engine_carry_framestride_onto_workerresult_fixes_interleaved_worker_false-positive_

Conversation

@vanceingalls

Copy link
Copy Markdown
Collaborator

Summary

expectedFramesForTask correctly divides by frameStride for interleaved parallel workers, but WorkerResult never carried frameStride in the first place — both return paths in executeWorkerTask (success and catch) omitted it. So the silent-worker-exit guard added in 971bcf39a (v0.7.60) always computed the expected frame count as the full contiguous range instead of range/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_router revert rate by CLI version — the cliff lands exactly on the release that shipped the guard:

version routed reverted revert rate
0.7.55–0.7.59 3582 88 ~2.4%
0.7.60–0.7.64 669 845 ~56%

Also 100% reproducible locally with HF_CAPTURE_PARALLEL_STREAM=true / HF_DE_PARALLEL_STREAM=true (any interleaved multi-worker render) — logged framesCompleted: 150/150 immediately 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

  • WorkerResult gains an optional frameStride field mirroring the originating WorkerTask.
  • Both executeWorkerTask return paths (success, catch) now populate it.
  • Extracted the inline guard loop into an exported flagSilentWorkerExits(results) so it's unit-testable without a real browser session.

Test plan

  • New unit tests in 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 pass
  • tsc --noEmit clean
  • oxlint + oxfmt --check clean

🤖 Generated with Claude Code

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#2679

  1. frameStride carried in both return paths. Success path (line 513) and catch path (line 536) both set frameStride: task.frameStride. Without this, the field would be undefined, and expectedFramesForTask would fall back to stride=1, computing the full contiguous range and false-positive flagging every interleaved worker. Correct.

  2. flagSilentWorkerExits extraction. The inline loop in executeParallelCapture is extracted verbatim into an exported function. Same logic: skip results with existing errors (!r.error), compare framesCaptured against expectedFramesForTask(r), synthesize error string if under-captured. The inline loop is replaced with a single flagSilentWorkerExits(results) call. Correct — behavior preserved, now testable.

  3. 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. ✓

  4. Test: genuinely under-captured worker. Same stride=3, but only 20 of 50 frames captured. Error synthesized with expected=50. ✓

  5. Test: existing error not overwritten. Worker with error already set → !r.error guard preserves it. ✓

  6. frameStride is optional (?: number) — contiguous workers (stride 1 or undefined) continue working because expectedFramesForTask presumably 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.

#2681

  1. CSS selector detection, not script detection. LIVE_MAP_MARKERS uses 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.

  2. 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.

  3. Array copy. buildLiveMapWarning stores [...libraries] (spread copy) so later mutation of the input array doesn't alter the recorded warning. Test: push to input array after build, warning's details.sources unchanged. ✓

  4. Warning code registered. "live_map_detected" added to the CaptureWarningCode union type in types.ts. Type-safe at all downstream consumers. ✓

  5. 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 james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Review by Rames D Jusso

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@vanceingalls
vanceingalls merged commit 8f17143 into main Jul 21, 2026
49 checks passed
@vanceingalls
vanceingalls deleted the 07-21-fix_engine_carry_framestride_onto_workerresult_fixes_interleaved_worker_false-positive_ branch July 21, 2026 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants