Skip to content

fix(render): retry timed-out browser initialization once - #3801

Merged
vanceingalls merged 1 commit into
mainfrom
fix/prinfra-612-browser-init-retry
Sep 9, 2026
Merged

fix(render): retry timed-out browser initialization once#3801
vanceingalls merged 1 commit into
mainfrom
fix/prinfra-612-browser-init-retry

Conversation

@vanceingalls

Copy link
Copy Markdown
Collaborator

What

A disk-capture Network.enable timed out failure now gets one fresh-session retry at lower concurrency, including when Chrome initialization produced zero frames. Refs PRINFRA-612.

Why

The recorded all-worker initialization failure was classified as authoring, and zero-progress capture failures could not enter the worker-halving retry. Either gap prevented recovery.

How

Classify the exact CDP signature as a protocol timeout and allow one bounded initialization retry. Preserve cancellation, disabled-retry behavior, and the existing fail-fast rule for other zero-progress failures. This fixes recovery; it does not establish or fix the initiating Chrome regression.

Test plan

  • Unit tests added/updated: capture classifier 16 passed; orchestrator 207 passed, including zero-progress recovery, repeated timeout, and cancellation.
  • Manual browser reproduction: unavailable because this environment blocks Chrome startup.
  • Documentation updated (not applicable).

Producer typecheck, changed-file oxlint/oxfmt, and fallow new-issue gate passed (duplication warnings only).

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

Read the full files at head, not just the diff, and verified the Why section empirically rather than by reading the pattern lists.

Strengths

  • The classifier addition lands in the right place. PROTOCOL_TIMEOUT_PATTERNS is evaluated before TRANSIENT_BROWSER_ERROR_PATTERNS (packages/engine/src/services/captureFailure.ts:153-155), so the new signature can't be swallowed by the transient branch and reach the same-worker-count retry instead. And the cancelled guard still precedes the new block (packages/producer/src/services/renderOrchestrator.ts:1151), so an aborted render can't burn the initialization retry — the parameterized cancellation test pins that for both messages.
  • The new branch is tightly scoped: initializationRetriesUsed === 0 plus the kind plus an exact-signature regex, so it can't drift into a general zero-progress retry. The comment says exactly that.
  • The "Why" claim checks out. I ran both versions of the classifier against the three real message shapes. At base 3a7fcd10e036ff0f5837821bb1143ca529934fae, Network.enable timed out classified as authoring with isFatalCaptureFailure true; at head it is protocol_timeout and false. "Classified as authoring" is accurate.

important — the reclassification also changes peer-abort semantics in the engine, and nothing states or tests it

isFatalCaptureFailure (captureFailure.ts:180) excludes protocol_timeout, and the parallel coordinator uses exactly that predicate to decide whether one worker's failure kills the others (packages/engine/src/services/parallelCoordinator.ts:1051-1055):

const onFailure = (failure: CaptureFailure): void => {
  if (firstFatalFailure || !isFatalCaptureFailure(failure)) return;
  firstFatalFailure = failure;
  peerController.abort(failure);
};

Before this PR a worker's Network.enable timeout was authoring → fatal → every peer worker was aborted. After it, the failure is non-fatal, so peers are no longer aborted and run to completion. Concretely, with 4 workers where worker 0's session never initializes and 1-3 are healthy:

  1. The failing attempt now takes as long as the slowest healthy worker instead of failing fast. protocolTimeout defaults to 300_000 ms (packages/engine/src/config.ts:282), so the stuck worker alone can sit 5 minutes.
  2. Because those peers now write frames, madeProgress is true — so the new branch at :1215 does not fire (it requires !madeProgress). Recovery falls through to the pre-existing halving retry at :1249. The new branch's real scope is therefore "no worker made any progress at all."

I think (1)+(2) are probably a net improvement — progress is preserved, so the retry redoes less — but this is a behavior change in a different package than the PR's stated scope, it changes a publicly exported predicate (packages/engine/src/index.ts:133-135), and no test covers it. Worth a line in the body and a coordinator test pinning whichever semantics you intend. Related: because (2) routes the partial case to the halving retry, which requires currentWorkers > 1, a single-worker render with partial progress still throws exactly as before.

important — the bound test asserts a constant that does not govern the new path

In renderOrchestrator.test.ts, it.each(["Session closed", "Network.enable timed out"])("bounds repeated %s failures") asserts 1 + MAX_TRANSIENT_CAPTURE_RETRIES. That is the right constant for Session closed. For Network.enable timed out the governing bound is the new hardcoded initializationRetriesUsed === 0, not MAX_TRANSIENT_CAPTURE_RETRIES. Traced: attempt 1 fails → init retry fires and halves 1 → 1; attempt 2 fails → init retry spent, transient branch skipped (wrong kind), so currentWorkers <= 1 throws. Two calls. It passes only because MAX_TRANSIENT_CAPTURE_RETRIES is 1 (renderOrchestrator.ts:944) and coincidentally equals the initialization budget.

Raise that constant to 2 and the Network.enable case fails at 2-vs-3 while pointing the reader straight at the transient path, which isn't the code that bounded it. Assert the literal 2, or add a MAX_INITIALIZATION_RETRIES constant and assert against that.

nit — once the initialization retry is spent, a second Network.enable failure lands on the !madeProgress warning at :1240: "composition is likely structurally broken — not retrying." That text predates this PR, but the PR makes browser-startup timeouts a routine visitor to it. Including failure.kind there would save the next person a detour. Adjacent code, so entirely your call.

Audited / Trusting

  • Audited: captureFailure.ts and the executeDiskCaptureWithAdaptiveRetry loop end-to-end at head. Enumerated every consumer of the failure taxonomy across all 2219 TS files at this SHA (isFatalCaptureFailure, classifyCaptureFailure, isRecoverableParallelCaptureError).
  • Checked and NOT affected: packages/producer/src/services/distributed/renderChunk.ts:274shouldRetryChunkCaptureWithScreenshot uses the kind only to exclude cancelled/memory_exhaustion, and this signature matches none of its beginFrame regexes, so the distributed screenshot fallback is unchanged. captureStageError.ts:9 only propagates the kind.

Head reviewed: d0ee207b06432edc48d3d0879c4cb4ca387cd0fd

Verdict: APPROVE
Reasoning: The recovery logic is correct and narrowly gated, cancellation is preserved, and the body's central claim about the old classification is verifiable and true. The two important items are an unstated side effect and a test coupled to the wrong constant — neither breaks the path this fixes, but both are worth resolving while the context is fresh.

— Rames Jusso

@vanceingalls
vanceingalls merged commit a153e63 into main Sep 9, 2026
56 checks passed
@vanceingalls
vanceingalls deleted the fix/prinfra-612-browser-init-retry branch September 9, 2026 07:45
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.

2 participants