fix(render): recover explicit parallel capture timeouts

[miguel-heygen]

What

Keep adaptive missing-frame recovery enabled when a user explicitly selects a parallel worker count. The explicit value remains the initial concurrency, but a recoverable timeout can now halve workers and retry only missing frames down to the sequential path.

Why

A Windows render with explicit 6 workers and then 3 workers twice reached 4084/4085 completed worker frames before one worker hit Runtime.callFunctionOn timed out; both runs exited without an MP4. captureStage disabled the existing bounded retry loop whenever --workers was explicit, forcing the user to manually rerun with --workers 1.

Reproduction

• Confirmed locally that an induced parallel protocol/navigation timeout exits without output and only recommends --workers 1.
• Traced the reporter signature to allowRetry: job.config.workers === undefined: explicit parallel counts bypassed recovery despite thousands of successfully captured frames.

Safety

The existing adaptive loop remains bounded: it retries only missing frames, requires forward progress for protocol timeouts, halves worker count each attempt, and stops at one worker. Zero-progress structural failures still fail immediately.

Tests

• Added coverage that explicit parallel worker counts keep recovery enabled and sequential captures do not retry.
• Producer render orchestrator + capture-stage tests: 143 passed.
• Producer TypeScript typecheck.
• Targeted oxlint, oxfmt, and git diff --check.
• Pre-commit tracked-artifacts, lint, format, fallow, and typecheck hooks passed.

[james-russo-rames-d-jusso]

Reviewed at d43fd74.

Correct policy change. The old gate — "retry only when workers were implicit" — punished users of --workers N even though the adaptive loop is bounded (halves workers, requires forward progress for protocol timeouts, single transient retry with MAX_TRANSIENT_CAPTURE_RETRIES, stops at sequential). The new gate keys off workerCount > 1, which is exactly what the retry loop already enforces at renderOrchestrator.ts:860 / :906 / :934 — belt-and-braces, and semantically what matters. A --workers 6 render that gets to 4084/4085 no longer forces the user to manually rerun with --workers 1.

Nits

• _explicitlyConfigured parameter is unused (prefix-underscore aside). The docstring makes clear it's a policy-shape indicator, but leaving it declared means (a) readers must trust the docstring over the code and (b) future policy tweaks that DO consult the flag wouldn't be caught by the current 2-case truth table. Either fold it into the check (return workerCount > 1 covers today's intent already) or drop it from the signature. Non-blocking.
• Test truth table is 2 cases; the omitted _explicitlyConfigured=false cases would round it out for free and defend against the drift described above.

— Review by Rames D Jusso

[vanceingalls]

R1 by Via — posted after Rames-D's parallel review; independent verification.

Peer-lens with Rames-D: convergent LGTM. Same-lens findings on the _explicitlyConfigured dead parameter; adds an independent trace of the retry-loop bounds and a pre-existing telemetry-gap FYI.

PR #2331 — fix(render): recover explicit parallel capture timeouts
Verdict: 🟢 LGTM
Head: d43fd74b on main
Status: all CI green (Build/Test/Typecheck/Producer unit+integration/Render on windows-latest/Tests on windows-latest/regression shards 1-8/runtime contract/CLI smoke/SDK/Studio/CodeQL/Preflight/player-perf); mergeable_state=blocked (branch protection — needs formal approving review; no prior reviews on record).

Summary of change
Restores adaptive parallel-capture recovery when the user specifies --workers explicitly. Previously allowRetry: job.config.workers === undefined bypassed the bounded halving-retry loop for any explicit worker count, so a single worker timing out at frame 4084/4085 killed the render with no fallback. The PR extracts a new pure helper shouldAllowAdaptiveCaptureRetry(workerCount, explicitlyConfigured) (currently returns workerCount > 1; the explicit flag is documented but unused) and swaps it into the allowRetry call site. Existing halving/forward-progress/recoverable-error guards downstream in executeDiskCaptureWithAdaptiveRetry are untouched.

Findings

• ⚪ Nit: _explicitlyConfigured parameter is dead code. packages/producer/src/services/render/stages/captureStage.ts:142-147. The second parameter is prefixed _ and never read. Callers pass job.config.workers !== undefined, but the return value is workerCount > 1 regardless. Function is behaviorally equivalent to workerCount > 1. The docstring justifies the design intent ("explicit config must not disable recovery"), so the parameter reads as documentation. Fine to accept as-is; alternatives are (a) drop the parameter or (b) actually use it in future policy. Refute attempt: any downstream site could grow to consume the flag → grepped repo, the helper has one caller (line 216 same file) and the test file; no downstream. Nit stands but low value to churn.

• ⚪ Nit: redundant workerCount > 1 short-circuit. Same helper. The one production caller sits inside if (workerCount > 1) block at line 213 of runCaptureStage, so the helper's workerCount > 1 guard can never observe false in production; only the second unit test exercises it. Independently, executeDiskCaptureWithAdaptiveRetry internally guards on currentWorkers <= 1 at renderOrchestrator.ts:860 and :934, so passing allowRetry: true with initialWorkerCount = 1 would still be safe. Defensible as belt-and-suspenders; also fine as-is.

• ℹ️ FYI (pre-existing, not caused by this PR): captureAttempts telemetry on final-throw path is empty. executeDiskCaptureWithAdaptiveRetry returns CaptureAttemptSummary[]; the caller pushes with captureAttempts.push(...attempts) at captureStage.ts:245 only on the success return. On terminal throw the local attempts array is lost. Later, in the orchestrator catch at renderOrchestrator.ts:2987, recordTransientRetryObservability() filters captureAttempts — which is empty on the failure branch — so the "retry burn on a render that STILL failed" metric the comment describes is always 0 for renders that exhaust the retry loop. This funnel gap is pre-existing (not introduced by this PR) but this PR ENLARGES the affected population (explicit-workers renders now enter the retry loop and can now fail through the same untelemetered exit). Worth a follow-up (thread attempts through the throw via an option-owned array like the docstring already implies for dedupPerfs), but not a blocker for this PR — the fix does what it advertises regardless.

Cross-checks

• Fix reachability verified: job.config.workers=6 → resolveRenderWorkerCount() returns 6 (unclamped; the html-in-canvas clamp is the only forced-1 path, and it fires regardless of explicit config) → if (workerCount > 1) parallel branch enters → allowRetry: true → recoverable timeout + madeProgress=true triggers halving retry to 3 → potentially to 1 (sequential fallback). Full path exercised by pre-existing tests in renderOrchestrator.test.ts around lines 180-345 (executeDiskCaptureWithAdaptiveRetry — transient Target-closed single retry, zero-progress bail, etc.), which cover the retry helper's behavior with allowRetry: true. The new test file only re-tests the gate helper.
• Retry bounds verified: MAX_TRANSIENT_CAPTURE_RETRIES = 1, halving via getNextRetryWorkerCount (Math.floor(currentWorkers / 2)), currentWorkers <= 1 stop, !isRecoverableParallelCaptureError short-circuit (specific message regex — timeouts, protocol errors, target closed only), !captureAttemptMadeProgress structural-failure stop, abortSignal.aborted rethrow. Already-captured frames preserved via findMissingFrameRanges + buildMissingFrameRetryBatches (frame files land in framesDir and survive across attempts). Zero wasted frame work.
• Widened-retry-policy audit: this PR widens the gate (allowRetry from false→true for one input class), NOT the retry cap or attempt count, and NOT the set of raises that flow through the retry loop. Every terminal raise inside the loop retains its isRecoverableParallelCaptureError/madeProgress/currentWorkers<=1/allowRetry guard. No new bare-raise added. Not the vault's classic widened-retry hazard.
• Telemetry-gap-on-parallel-branch audit: the fix does NOT add a new retry path parallel to an instrumented one — it re-enables the existing path (executeDiskCaptureWithAdaptiveRetry) whose telemetry already flows through captureAttempts reason tags (initial/retry/transient-retry) → recordTransientRetryObservability on the success side. The pre-existing failure-side gap is called out above.
• Scope: fix is scoped to disk parallel path only. Grepped streaming and HDR stages — neither has the config.workers retry gate the fix targets (streaming path doesn't route through executeDiskCaptureWithAdaptiveRetry at all per its module docstring). Correct scope.
• Head SHA current: yes (d43fd74b9fd55e84842a28b45bd3863d8e6551c3).
• Prior reviewers on record: none.
• Mergeability: mergeable: true, mergeable_state: blocked (needs formal review — branch protection).
• CI lanes: all green including Windows-latest Render + Tests, producer unit + integration, regression shards 1-8, runtime contract, CodeQL, Preflight.
• Timeout bounds documented: yes (docstrings on MAX_TRANSIENT_CAPTURE_RETRIES, captureAttemptMadeProgress, getNextRetryWorkerCount, isRecoverableParallelCaptureError).
• Telemetry emits new retry path: no new path added; reuses existing captureAttempts reason tags. Pre-existing failure-side telemetry gap noted as FYI, not this PR's regression.

R1 by Via

[jrusso1020]

Stamped — verified before approving: CI green (39 pass) at d43fd74b, author miguel-heygen, no standing CHANGES_REQUESTED. Confirmed the mechanism at source: shouldAllowAdaptiveCaptureRetry gates on workerCount > 1 (was job.config.workers === undefined), so an explicit worker count no longer disables timeout recovery — the adaptive loop stays bounded (halves workers, requires forward progress, stops at sequential). The unused _explicitlyConfigured param is a deliberate no-op (Rames-D's non-blocking nit).

— Rames Jusso