fix(producer): record routing state on the failure path - #3081
Conversation
de_parallel_router is present on 95.4% of render_complete events and 0.83% of render_error. Capture context itself survives failures fine (capture_mode is on 98.6% of them), so this is not renders failing before capture — the routing state specifically is being dropped. Cause is ordering. deParallelRouter is assigned twice: once before the capture-observability update, and again inside syncCapturePlan where routing is actually resolved — including the 'reverted' case, which the earlier assignment cannot know. The update in between recorded whatever was true first, so a render that failed while routed reported no routing state at all. The existing comment at the earlier call site says it is recorded there precisely so hard failures carry it; that intent was correct and the value just arrived too late. This matters for the #2840 ramp specifically. The per-install circuit breaker only arms on a revert, which requires the render to finish and self-detect — it cannot catch a crash or hang. Those are exactly the failure modes a percentage ramp exists to bound, and they were the ones telemetry could not see. Also makes the ffprobe contract sweep resilient per entry. A dangling symlink under packages/studio/data/projects threw ENOENT on stat and aborted the whole traversal, so every package sorting after 'studio' — both studio-server callers included — silently stopped being checked. main is currently red on this. The manifest assertion is what caught it, which is what it was added for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
miga-heygen
left a comment
There was a problem hiding this comment.
Review: routing state on the failure path
The observability gap
The numbers tell the whole story: de_parallel_router is present on 95.4% of render_complete events and 0.83% of render_error. That's not "renders fail before capture" — it's a timing bug in the telemetry recording. The 0.83% is the fix.
What I verified
The timing chain. Traced the flow in executeRenderPipeline:
- ~Line 2948:
deParallelRouter = "routed"set when eligible, BEFORE capture plan resolution syncCapturePlan(): resolves actual routing state (can produce"reverted")updateCaptureObservability(...): this call NOW includesdeWorkerInversionanddeParallelRouter— the resolved values, not the pre-resolution ones
Before this fix, a render that failed after step 2 would report the pre-step-2 observability state (no routing field). Now it reports the resolved state.
The fields are already in scope. deWorkerInversion and deParallelRouter are let variables in the enclosing closure, set by syncCapturePlan() on the line immediately before the updateCaptureObservability call. No new data plumbing needed — they were just not passed to the update.
No semantic change to success path. render_complete events already had 95.4% coverage because the observability fields eventually propagated through later call sites. This fix makes the failure path match.
ffprobe contract test fix
The dangling symlink at packages/studio/data/projects/marys-room-companion threw ENOENT on statSync, aborting the entire traversal. Every package sorting after studio — including both studio-server callers — silently stopped being checked. The per-entry try/catch is the right fix: the manifest assertion is what caught the truncation, and it will continue to catch future cases.
Honest verification
The PR is right to say the orchestrator change has no unit seam. updateCaptureObservability is a closure inside the render function, unreachable without a full render. The real acceptance check is de_parallel_router coverage on render_error rising from 0.83% toward ~95% post-deployment. That's a testable prediction.
Core CI checks passing (Producer unit + integration, Build, Lint, Format, CLI smoke). Regression shards pending. No blocking concerns.
— Miga
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at a3d13e26.
Small, surgical, well-argued. Nothing blocking from my side — leaving as a comment.
Trace verified. deWorkerInversion and deParallelRouter are outer let bindings from :2030 / :2169, mutated by syncCapturePlan at :3253 / :3256, and updateCaptureObservability is Object.assign-merge (:2004-2009), so the added :3274-3275 fields are exactly the values that will ride on captureObservability when the render_error path emits from :3976 (or render_complete from :3813). Comment at :3267-3273 — mechanism + 95%/0.8% impact + tie-in to the rollout the ramp is watching — is load-bearing and belongs.
The other three syncCapturePlan() call sites don't need the same fix, verified pairwise:
:3489(streaming-drain retry viareplanAfterFailure({ kind: "draw_element_verification" | "capture_failure" })) reaches the finalrevertedRouting(plan.routing)branch incapturePlan.ts:161-170and can actually flip state to "reverted" — its followupupdateCaptureObservabilityat:3499-3500already carriesdeWorkerInversion, deParallelRouter. Correct.:3563(streaming_unavailable replan) passesroutingthrough unchanged (capturePlan.ts:143-149—createCapturePlan({...plan, useStreamingEncode: false, ...})with the frozen routing from{...plan}). State can't flip here, so the missing fields on the followup update at:3588are fine.:3676(disk-path DE-verification replan) same shape —capturePlan.ts:129-138passes routing through unchanged. State can't flip, missing fields on:3677-3684fine.
So the added :3260 update is the one hole in the four-syncCapturePlan coverage, and this closes it exactly.
On the correlation you mention ("226 errored vs 175 completed among renders that did revert" — a revert correlates with outright failure more often than not) — worth carrying as a follow-up hypothesis for #2840. The revert is nominally the safe-path exit, but this cohort's error rate says the DE path may be systemically wedging some renders in a way the self-verify catches only some of the time. Once telemetry lands and you get failure-mode attribution on the errored-and-reverted cohort, that number is worth splitting by GPU / capture_mode / worker-count-at-failure — it may be that "reverted" is a lagging signal for a class of hosts where the router should never have fired in the first place. Not in scope here, and the honest causal uncertainty in the body is the right stance.
ffprobe contract fix. Try/catch around the per-entry statSync(abs).isDirectory() walk is the right shape — the outer sweep-root catch already models the same failure-isolation pattern (:192-196 — /* root absent in a partial checkout */), and the new one carries the same style with an entry-specific reason. One minor observation, not a finding: the catch also swallows exceptions from classify(abs) (readFileSync on line :160), so a genuinely-unreadable source file (real permissions issue) would silently drop from the sweep instead of showing as a per-entry problem. In practice the manifest assertion at the end catches the case that matters (any listed caller missing from the found set), so this is defense-in-depth working as designed — worth naming only because a future "why isn't my new caller being flagged?" would want a console.warn in that catch.
On bundling the two fixes. The ffprobe fix is technically its own PR — different subsystem, different failure mode. Given main was red and the ffprobe truncation was silently letting studio-server callers escape the sweep, unblocking the sweep here is defensible. Non-blocker; noted only because a future reader hunting de_parallel_router history will see an unrelated ffprobe hunk in the same commit.
On the acceptance check. The PR body's framing ("de_parallel_router coverage on render_error should rise from 0.83% toward the ~95% seen on completions once this ships. I would treat that as the acceptance check rather than claiming it is proven here") is the right posture — every alternative (mocking the closure, refactoring updateCaptureObservability out for testability) is a bigger change than the fix itself. Ship, watch the coverage, use the resulting error-cohort data to unblock the #2840 ramp.
LGTM from my side.
The gap
de_parallel_routeris present on 95.4% ofrender_completeevents and 0.83% ofrender_error.That is not "renders fail before capture" — capture context survives failures fine:
render_completerender_errorcapture_modede_parallel_routerCause
deParallelRouteris assigned twice in the orchestrator::2948—"routed", before the capture-observability update:3256— insidesyncCapturePlan, where routing is actually resolved, after it — and this is the only assignment that can produce"reverted"The update in between recorded whatever was true first. A render that failed while routed reported no routing state at all. The existing comment at the earlier call site says it is recorded there "so a hard failure while routed/inverted still tells us" — the intent was right, the value just arrived too late. This adds the fields to the update that already runs immediately after
syncCapturePlan().Why this blocks the #2840 ramp
The per-install circuit breaker only arms on a revert, which requires the render to finish and self-detect. It cannot catch a crash, hang, or OOM — those emit no
render_completeat all, so the install is never protected and hits it again next render.Those are precisely the failure modes a percentage ramp exists to bound, on precisely the profiles with no trial coverage (≤4 CPUs at 8.18% of eligible renders, Docker at 3.99%). Ramping while 99.2% of failures carry no routing state means being blind to the thing the ramp is watching for.
Related: among renders that did revert, 226 errored vs 175 completed — a revert correlates with outright failure more often than not. Causality is unclear (the render may have been failing anyway), but it argues against treating a revert as a benign "one slow render".
Also: main is currently red
The ffprobe contract sweep aborted on the first unreadable entry. A dangling symlink at
packages/studio/data/projects/marys-room-companionthrowsENOENTonstat, killing the whole traversal — so every package sorting afterstudio, including bothstudio-servercallers, silently stopped being checked. Now skipped per entry.The manifest assertion is what caught this, which is exactly what it was added for.
Verification
Producer suite 598 passing, unit lane exits 0, lint clean. Mutation-tested: removing the per-entry guard reproduces the failure.
The orchestrator change has no unit seam —
updateCaptureObservabilityis a closure inside the render function, unreachable without a full render. The real verification is the telemetry itself:de_parallel_routercoverage onrender_errorshould rise from 0.83% toward the ~95% seen on completions once this ships. I would treat that as the acceptance check rather than claiming it is proven here.🤖 Generated with Claude Code