fix(engine): self-verify parallel disk drawElement samples

[vanceingalls]

Summary

Fixes the enforcement gap behind PRINFRA-352 (parallel-capture "worker-boundary" corruption, darwin/arm64 0.7.68, n=2).

Diagnosis

The corrupt repro ran --experimental-fast-capture --workers 2 on a 16GB machine. The explicit fast-capture opt-in bypasses the parallel clamp (by design), producing parallel disk drawElement with zero runtime verification:

• Parallel disk workers already arm pre-injection ground-truth samples (resolveParallelDeVerifySamples even raises density for multi-worker capture, citing exactly this compositor risk)…
• …but only the streaming drain ever checked them. The disk path armed samples and never compared — damaged frames shipped silently.

Two concurrent hardware-GPU Chromes on a 16GB host is the documented compositor-tile-eviction damage class (the wild 0.7.52 "vertical black slabs" report — the reason the DE router has a 24GB RAM floor). The "corruption starting exactly at the worker-boundary midpoint" is one worker's entire range being damaged: workers share no state; the boundary is simply where the damaged worker's output begins. Not a handoff bug.

Fix

After a disk-path worker completes its range, re-read its captured frame files at the armed sample indices and PSNR-compare against the session's pre-injection ground truth (verifyDiskDrawElementSamples). A breach throws DrawElementVerificationError (message keeps the contiguous "drawElement self-verify" phrase so classifyCaptureFailure labels it verification), and the orchestrator's existing pinned-fallback retry converts it into a screenshot re-render — identical recovery to the streaming drain. No new recovery machinery.

• psnrDb + resolveDeVerifyMinDb moved to @hyperframes/engine (utils/psnr.ts) — one comparison implementation shared by the drain guard (producer) and the disk verify; producer's local copy deleted.
• selectVerifySampleIndicesForTask exported pure: armed samples inside a task's [start, end) on its stride lattice (mirrors captureFrameRange's loop) — each sample checked by exactly the worker that owns the frame.
• Streaming path unchanged (drain guard already verifies; no double-checking).
• Cost: ≤8 file reads + ffmpeg PSNR comparisons per worker, once per range (~100ms).

Test plan

• New selectVerifySampleIndicesForTask tests: contiguous range (incl. the report's exact 3032/2-worker split), stride lattice, empty
• Engine parallelCoordinator suite 46/46; producer render-stage suites 296/296
• tsc --noEmit clean (engine, producer); engine build clean; oxlint/oxfmt clean

Related: PRINFRA-352, PRINFRA-300, PRINFRA-340.

🤖 Generated with Claude Code

[vanceingalls]

Smoke-verified the integration live on darwin/arm64 in the exact reported configuration (--workers 2 + PRODUCER_EXPERIMENTAL_FAST_CAPTURE=true, 180-frame 5-sub-comp fixture): parallel disk DE engaged, both workers armed 6 ground-truth samples, and the new verify checked exactly the samples each worker owns (w0: 30/60; w1: 90/120/150/171), all passing at 46–52dB. Render completed 180/180 in drawelement mode.

The damage itself is not reproducible on a high-memory machine (needs GPU/memory pressure — 16GB class). Detection is mechanism-agnostic though: any damage class diverging >32dB from ground truth at a sampled frame now trips the existing screenshot-fallback retry, and the reports describe whole-worker-range damage, which sampled verification catches with near-certainty. First field self-verify failure log on a ≥this-PR build will double as the repro confirmation.

[vanceingalls]

Fault-injection results (HF_DE_VERIFY_MIN_DB=60 forces healthy ~47dB frames to breach) — found and fixed a recovery gap:

• Detection: confirmed live. Worker 1 breached at frame 90 (46.6dB < 60dB), threw DrawElementVerificationError with intact cause chain, peer worker cancelled cleanly.
• Recovery: was MISSING on the disk path — the render hard-failed instead of falling back. The verify-error retry only wrapped the streaming stage. Second commit (ec791e91d) adds the same recovery to the disk stage: replanAfterFailure now accepts sdr_disk + draw_element_verification (→ forceScreenshot), the orchestrator wipes framesDir before the retry (the failed attempt's damaged frames would otherwise satisfy the completeness check and be silently reused), resets attempt progress + dedup perfs, and re-invokes the disk stage with useDrawElement=false. Telemetry carries the standard de_self_verify_fallback/fallback_reason/failed_db fields.

Two honest open items from the same testing:

1. Nondeterministic stage routing: across repeated identical runs (workers=2, opt-in), some runs route through a sequential-session disk path (probe + one capture session, worker-encode pipelined writes) instead of executeParallelCapture — those runs never hit the new per-worker verify. Cause not yet isolated.
2. Sequential disk DE under the explicit opt-in is also unverified — same exposure class this PR fixes for parallel workers. The armed samples exist on that session too; the check just isn't wired into the sequential loop. Follow-up should cover it (and would moot Initial repo setup #1's impact).

Both noted so reviewers weigh this PR as: parallel-disk verify + recovery proven end-to-end by injection; sequential-disk verify still open.

[vanceingalls]

Both open items closed in 9fc1c2f15:

Sequential disk verify — verifyDiskDrawElementSamples exported from the engine and wired into captureStage's sequential branch with a synthetic whole-range task ([rangeStart, rangeEnd), offset-aware for distributed chunks). A breach throws the same DrawElementVerificationError, and the orchestrator's disk-stage retry (ec791e91d) recovers via screenshot — the recovery wrap sits around runCaptureStage, so it covers both branches.

Routing nondeterminism mooted — the anomalous runs were the sequential/probe-reuse branch, which previously skipped verification entirely. Every disk-path DE configuration (parallel workers, sequential, probe-reused, distributed chunk) now verifies against the same armed samples. Whichever branch a run lands on, unverified DE frames can no longer ship.

Suites green: engine 46/46, producer render stages 296+.

[vanceingalls]

Self-review (max) caught a blocker in my own fix — fixed in c85cfae

🔴 The parallel-disk recovery was a silent no-op. verifyDiskDrawElementSamples threw correctly, but executeDiskCaptureWithAdaptiveRetry's catch only rethrew cancelled — the verification failure fell through to findMissingFrameRanges, which is presence/size-only, so damaged-but-complete frames counted as present → returned SUCCESS → the orchestrator's screenshot-retry never fired → damaged video shipped. Exactly the defect this PR exists to prevent, on its headline (parallel) path. My earlier 'proven end-to-end by injection' claim was wrong — the prior inject run had exited 0 through this swallow, which I'd misattributed to routing nondeterminism.

Fix: rethrow on isDrawElementVerificationError(error) before the completeness check (mirrors the cancelled guard); a worker-halving retry here would only re-run drawElement and re-damage. Structural cause-chain detection walks aggregated CaptureFailure → worker CaptureFailure → DrawElementVerificationError.

Now genuinely proven — fault injection (HF_DE_VERIFY_MIN_DB=60, workers=2, real darwin): both workers breach (49.6/46.6dB) → re-rendering via screenshot → retry in screenshot mode → complete, svFallback: true, reason: psnr, failedDb: 49.6. Added a capturePlan regression test for the sdr_disk verify replan.

Also from the review: deduped the PSNR-threshold clamp onto the shared resolveDeVerifyMinDb() so disk and streaming can't diverge. Skipped two cleanups with rationale: the psnrForDiskSample redundant read (dwarfed by the per-sample ffmpeg spawn it feeds) and the disk/streaming telemetry-mapping duplication (restructuring the delicate pre-existing streaming catch to dedupe a 4-line map adds more risk than it removes).

Suites: engine parallelCoordinator 46/46, producer render (vitest-visible) 296/296 + capturePlan 9/9, streaming stage 10/10 under bun. The 10 vitest 'file failures' are pre-existing bun:test imports vitest can't load, unrelated to this diff.

[miga-heygen]

SSOT Review: disk-path drawElement self-verification (PRINFRA-352)

PR: fix(engine): self-verify parallel disk drawElement samples — 9 files, +403/-80

---

The bug

Parallel disk drawElement capture armed pre-injection ground-truth samples (resolveParallelDeVerifySamples) but only the streaming drain ever CHECKED them. The disk path armed samples and never compared — damaged frames shipped silently. On a 16GB host with two concurrent hardware-GPU Chromes, compositor-tile-eviction damage produces frames with vertical black slabs for one worker's entire range.

Architecture

The fix follows the existing streaming-drain verification shape exactly:

1. Verify: after a worker's range completes, re-read captured frame files at the armed sample indices and PSNR-compare against the session's ground truth.
2. Signal: a breach throws DrawElementVerificationError (same error type, same message phrase for classifyCaptureFailure).
3. Recover: the orchestrator's existing pinned-fallback retry converts it into a screenshot re-render — identical recovery to the streaming drain.

SSOT checks

1. psnrDb — single source. Moved from captureStreamingStage.ts to @hyperframes/engine utils/psnr.ts. The streaming drain and the disk verify both import from the same module. The old local copy is deleted entirely — not left as a re-export or alias. ✓

2. resolveDeVerifyMinDb — single source. Same move. The inline clamp logic ([10, 60] range, 32 default) now lives in one function. The streaming drain's warn (which needs a logger) stays local but uses the shared function for the value. "The disk and streaming verify paths can never apply different PSNR floors to the same composition." ✓

3. selectVerifySampleIndicesForTask — pure, tested. Filters armed sample indices to those inside [startFrame, endFrame) and on the task's stride lattice. Mirrors captureFrameRange's i += stride loop exactly. Tests cover: contiguous 2-worker split (the exact 3032-frame repro), interleaved stride, and empty result. ✓

4. verifyDiskDrawElementSamples guards are exhaustive:

   • streaming → return (drain already verifies, no double-check). ✓
   • captureMode !== "drawelement" → return. ✓
   • No truths armed → return. ✓
   • Infrastructure failure (missing file, ffmpeg error) → skip sample, not a breach. ✓
   • PSNR below threshold → throw DrawElementVerificationError. ✓

5. executeDiskCaptureWithAdaptiveRetry rethrows DE verify errors. Critical invariant: findMissingFrameRanges checks presence/size only — damaged frames are complete files with wrong pixels. Without the rethrow, the missing-frame check would count them present and wrongly return success. ✓

6. replanAfterFailure handles sdr_disk + draw_element_verification. Returns a new plan with forceScreenshot: true, preserving workerCount. The test pins this: sdr_disk, forceScreenshot: true, workerCount: 2, no streaming. ✓

7. Disk-path retry is thorough:

   • Wipes framesDir (damaged frames are untrusted but satisfy completeness). ✓
   • Resets capture state (dedupPerfs.length = 0, probeSession = null, cfg.useDrawElement = false). ✓
   • Replans and syncs (syncCapturePlan). ✓
   • Clears the failure on successful retry (observability.clearFailure). ✓
   • Telemetry via shared deVerifyFallbackTelemetry. ✓

8. deVerifyFallbackTelemetry — shared. Both streaming and disk verify catches extract structured details through the same function. kind is read structurally off the error, not from message text. ✓

9. Sequential disk path also verified. captureStage.ts calls verifyDiskDrawElementSamples with a synthetic task matching the sequential range. Same function, same recovery. This closes the gap for the sequential path too (reachable via probe-session reuse under the fast-capture opt-in). ✓

Stale concept scan

• psnrDb local copy in captureStreamingStage.ts — deleted, replaced by import. ✓
• resolveDeVerifyMinDb inline logic in the drain guard — replaced by shared function call, only the warn stays local. ✓
• No "TODO: verify disk path" comments survive. ✓

Blocking SSOT issues

None found.

Verdict

Approve. The fix plugs the enforcement gap by reusing the exact same machinery the streaming drain already has — same PSNR comparison, same threshold, same error type, same recovery chain. The PSNR function consolidation into utils/psnr.ts ensures both paths can never diverge. The findMissingFrameRanges bypass for correctness failures is the critical safety detail, and the framesDir wipe before retry is the correct consequence of that. Ship it.

— Miga

[james-russo-rames-d-jusso]

Reviewed at 4889b779c (Graphite stack root, latest commit fix(engine): lazy-promisify execFile in psnr…).

Sound diagnosis + narrow fix. The design — arm samples uniformly, check them at range-completion, throw a typed error, ride the existing pinned-fallback retry — is right, and the commit history shows self-correction on the sharp edges (structural rethrow past findMissingFrameRanges, lazy promisify for the CLI mock, sequential-path parity, telemetry helper extraction). One resource-leak concern on the retry catch, everything else verified-safe.

Concerns

• 🟠 Probe session leaked on the disk-verify retry path (renderOrchestrator.ts:3246).
  When invokeDiskCapture throws DrawElementVerificationError, the catch handler does probeSession = null; without first closing the session. If runCaptureStage's parallel branch threw from inside executeDiskCaptureWithAdaptiveRetry (the common case for this fallback), control leaves captureStage.ts BEFORE the success-path closeCaptureSession(probeSession) at captureStage.ts:243-245. The orchestrator still holds the reference — but nulling it here severs it. The execution.defer("close probe session", …) closure at renderOrchestrator.ts:1761-1766 checks if (!probeSession) return; against the SAME variable the catch just nulled, so the deferred cleanup then skips. The Chrome process is orphaned until Node exits.

  Reachable only via --experimental-fast-capture --workers N>1 + a verify breach + non-consumed probe session, and the leak is bounded to render-process lifetime (~minutes). But the fix is a one-liner — close before nulling:

  if (probeSession) {
    await closeCaptureSession(probeSession).catch(() => {});
  }
  probeSession = null;

  Worth checking renderOrchestrator.ts:3093 (streaming-path retry) for the same pattern — I didn't verify it end-to-end but the shape looks identical.

Nits

• 🔵 The sequential disk verify at captureStage.ts:342-352 builds a synthetic single-worker task inline. Not wrong — the field-mapping is unambiguous — but a named helper (createSequentialVerifyTask(rangeStart, rangeEnd, framesDir)) would document intent and let both call sites share the same shape guarantee. Follow-up if you agree; not required to land this.

Verified-safe

• The load-order bug the CLI test tripped is genuinely fixed. psnrDb's lazy promisify(execFile) inside the function body (vs the earlier top-level const execFileP = promisify(execFile)) means packages/cli/src/telemetry/client.test.ts's partial vi.mock("node:child_process", …) no longer crashes any transitive importer of psnr.ts at Vitest module-load time. The prior CI red at 92ffd0476 reproduces the exact failure and 4889b779c is the right narrow fix. Broader rule worth codifying: no top-level builtin promise wrappers in shared engine modules that show up in the CLI test import graph.
• isDrawElementVerificationError rethrow at renderOrchestrator.ts:1032 — closes the findMissingFrameRanges false-positive hole (damaged frames are complete-on-disk and would pass the size/presence check). Structural detection walks the aggregated CaptureFailure → worker CaptureFailure → DrawElementVerificationError cause chain, so a translated / reworded / cross-realm error can't sneak past.
• deVerifyFallbackTelemetry helper at renderOrchestrator.ts:1583-1594 — clean extraction; the "kind is read STRUCTURALLY off the error, never from message text" invariant from the pre-existing streaming-path code is preserved verbatim in the doc-comment. Both call sites (streaming retry + new disk retry) now share one implementation. Adding a new field to the mapping updates one place.
• psnrDb + resolveDeVerifyMinDb centralized in @hyperframes/engine/utils/psnr.ts. Producer's local copies deleted, streaming drain (captureStreamingStage.ts:64-65) and parallel disk verify (parallelCoordinator.ts:28) both import from engine. Threshold clamp [10, 60] → default 32 matches the pre-existing streaming-path clamp behavior; no drift possible.
• selectVerifySampleIndicesForTask — pure. (idx - startFrame) % stride !== 0 filter mirrors captureFrameRange's i += stride loop shape exactly. Sort at end ensures deterministic verify order. 3 test cases (contiguous / interleaved stride 3 / empty range) cover the branches the loop actually takes.
• verifyDiskDrawElementSamples — early-returns on streaming || session.captureMode !== "drawelement", so the streaming-drain path (which already runs the guard) can't get double-verified, and non-drawElement sessions skip. psnrForDiskSample catches ffmpeg spawn/tmpdir infra failures and returns null (skipped sample, NOT damage evidence) — only genuine below-floor PSNR throws DrawElementVerificationError. Message keeps the "drawElement self-verify" phrase so classifyCaptureFailure's VERIFICATION_ERROR_PATTERNS still classifies as verification.
• replanAfterFailure disk-verify transition (capturePlan.ts:127-138) + its test (capturePlan.test.ts:143-163) — locked exactly: sdr_disk + draw_element_verification → sdr_disk with forceScreenshot: true, workerCount preserved (2 in the test), forceParallelStream: false. No cross-plan drift (sdr_streaming + draw_element_verification still throws the pre-existing invariant error).
• Retry-path idempotency — the catch handler: rmSync(framesDir, recursive) (wipes untrusted-but-complete files), mkdirSync(framesDir), resetCaptureAttemptProgress(job), dedupPerfs.length = 0 (DE-session perf must not bleed into screenshot telemetry), cfg.useDrawElement = false, capturePlan = replanAfterFailure(…), syncCapturePlan(), updateCaptureObservability(…), and observability.clearFailure("capture_disk") after the recovery succeeds. Every mutation the retry needs is accounted for; nothing gets carried over that would mislead the recovered attempt.
• Sequential disk-path parity (captureStage.ts:342-352). The PR body only names the parallel disk gap, but commit 9fc1c2f15 fix(producer): verify sequential disk drawElement samples too covers the sequential fast-capture path (also reachable under the explicit opt-in, including via probe-session reuse) with the same synthetic-task shape. Silent scope expansion vs the PR title — worth a body-note in the final Slack rollup so the merge audit trail catches it — but the code is right.

Questions

• Under what timeline are you planning to remove the --experimental-fast-capture gate? The whole reason this verify gap existed was that the disk path was opt-in-only, so drain-time verification never made it here. Once the gate is dropped and the disk path becomes reachable via --workers without the flag, this verify becomes load-bearing (and the leak concern above becomes more consequential per render). Just calibration, not something you need to answer here.

What I didn't verify

• The streaming-retry pattern at renderOrchestrator.ts:3093 for the same probe-session-null-before-close shape. Worth eyeballing when you touch the disk-retry fix.
• Actual runtime behavior of the psnrDb lazy fix under a real drain — the CI job Test was re-running against 4889b779c at review time and hadn't yet reported; the fix is right by inspection and the earlier failure trace at 92ffd0476 reproduces on the exact call site the fix addresses.
• Repro of PRINFRA-352 field conditions (darwin/arm64, 16GB, hardware-GPU Chrome, --workers 2). Verified by design + test coverage, not by field repro.

CI at 4889b779c is running (Test, Windows tests, regression shards, typecheck all in progress); the earlier failures at 92ffd0476 were the load-time mock crash and its Windows echo, both addressed by the lazy-promisify fix. LGTM from my side — leaving as a comment. Fix the probe-session close before nulling and you're ready from where I sit.

— Review by Rames D Jusso

[miguel-heygen]

The disk self-verification and recovery design is sound, but the new parallel-disk retry path leaks its probe Chrome session.

In renderOrchestrator.ts, invokeDiskCapture() enters runCaptureStage() with the existing probeSession. For workerCount > 1, that stage only closes the probe after executeDiskCaptureWithAdaptiveRetry() returns. A verification breach throws before reaching that cleanup, then the catch around line 3246 does probeSession = null and immediately starts the screenshot retry. The original probe browser is therefore orphaned until process exit, increasing memory pressure precisely on the GPU/memory-failure recovery path.

Please close the still-owned probe session before clearing it (with cleanup semantics consistent with the surrounding code), and add a focused regression that makes a parallel disk verify failure assert the probe is closed before the retry. The analogous streaming catch is different: its stage consumes/closes the probe in its own failure path.

Non-blocking: the default verification grid remains capped at 8 samples even though explicit/auto concurrency can exceed 8, so supported high-worker runs can leave entire contiguous worker ranges unsampled. The reported 2-worker case is well-covered, but this is worth a follow-up if high-worker disk fast capture is expected.

Verdict: REQUEST CHANGES
Reasoning: close the leaked probe session before the screenshot retry; the core PSNR, typed-failure, frames-dir wipe, and fallback mechanics otherwise check out.

— Magi

[miguel-heygen]

Re-review at exact head 5fd28e1149b4258ab16b3282c495d7fc54325280.

Verified: packages/producer/src/services/renderOrchestrator.ts:3122-3126 and :3285-3289 now snapshot the owned probe, preserve its browser console, clear the shared reference, and await closeOrphanedProbeForRetry before either screenshot retry begins. The extracted helper at :1481-1494 makes cleanup failure non-fatal while retaining an actionable warning. This closes both the parallel-disk leak I blocked on and the sibling streaming-retry shape Rames identified.

The final 5fd28e1149 delta over b8e1015476 is formatter-only: two files, whitespace wrapping only. Exact-head CI is fully green, including Test, Windows tests/render, all 8 regression shards, CodeQL, CLI smoke, and global install.

Important follow-up: packages/producer/src/services/renderOrchestrator.test.ts:2135-2191 tests the helper in isolation, but does not drive a disk verification failure through the orchestrator and therefore would not fail if the call at :3289 were later removed or reordered after the retry. Please add the originally requested integration-scoped ordering regression in a follow-up; the production wiring is correct by inspection, so I am not holding the fixed leak on that test gap.

Verdict: APPROVE
Reasoning: The resource leak is fixed at every matching retry site, the cleanup ordering is correct, and exact-head CI is green. The remaining gap is regression strength, not a live correctness defect.

— Magi