fix(cli): bound capture runtime stages - #2928
Conversation
vanceingalls
left a comment
There was a problem hiding this comment.
Nice cooperative-budget contract — the schema-versioned phase record (hyperframes.capture.phase.v1), monotonic post-navigation deadline, and threaded remainingMs give EF a real handoff instead of a "I've been sitting here for a while" heuristic. APPROVE as the base of the stack.
Things I checked and liked:
- Font attempt-cap re-order in
assetDownloader.ts— movingfamilyCount++/count++BEFOREfetchBuffer(was AFTERif (buffer)) is a real defect fix, not just plumbing. With the old code, a slow/failing font endpoint could bypassMAX_TOTAL_FONTS=30because failed fetches didn't count. The new test atassetDownloader.test.ts:44-48(35 URLs → asserts exactly 30 fetch attempts) captures this cleanly. Same for the six-per-family cap. runBoundedVisionRequesttimeout race —timerfires →reject(VisionRequestTimeoutError)runs BEFOREcontroller.abort(), soPromise.racealways resolves with the typed timeout error even when the SDK ignores abort. Necessary for@google/genaiper the memory this stack encodes. Clean.- Pre-nav
remainingMs()returnsbudgetMsunbounded beforepostNavigationDeadlineis set (line 613-616). Every pre-nav call sees full budget; every post-nav call sees the deadline. Correct — page-load has its owntimeoutopt and shouldn't share the same budget. phase("browser", "started")fires at line 646, beforeloadEnvFile/ browser launch — so EF sees the first phase event even if browser init throws. Good.skipVisionreturns empty captions early (line 384), and thehasVisionKeycomputation at index.ts line 693 correctly includes!skipVision— so the fallback header prose matches actual behavior instead of pretending the key is present.- CLI
HYPERFRAMES_CAPTURE_PHASEprefix on stderr viadiag.notice— machine-readable framing that stays out of stdout JSON. indexBudget.test.tssource-string assertion — I know these are fragile, but the invariant (a template-literal JSif (Date.now() >= lazyLoadDeadline) break;insidepage.evaluate) genuinely can't be reached with a behavioral test. This is the right trade-off; just accept the maintenance friction.
Nits (none blocking):
- Lottie stages not budget-threaded internally — the gate is
if (discoveredLotties.length > 0 && remainingMs() > 0)at index.ts line 719, butsaveLottieAnimations/renderLottiePreviewsdon't receiveremainingMsin this PR. A hang inside either won't be deadline-enforced; only the outer gate protects against them. I see #2930 addresses this — worth calling that out here as "known follow-up in stack tip" in the PR body so a future reader doesn't wonder. - Vision per-request timeout
Math.min(requestTimeoutMs, remainingMs)— when remainingMs is small (say 500ms), every OpenRouter/Gemini call gets 500ms and effectively all time out. ThetimedOutCount+ summary warning handles this gracefully (no throw, no crash), and it's arguably correct behavior (don't start work you can't finish). Just be aware that during tight-budget captures the "vision degraded" state will be normal, not an anomaly worth investigating. captureVideoManifestreceivesdownloadBudgetMsas a snapshot ofremainingMs()at call time (index.ts line 748) — not a live callback. Video downloads happen serially before downstream stages so this is fine; the interlock holds because downstream stages recompute fromremainingMs(). Just noting that the snapshot boundary is here (in case it needs to become live in the follow-up stack).
CI: build + lint + producer-integration + typecheck + shim (linux/mac/win) all green at ca9bba1d; Windows tests + CLI smoke pending.
— Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at ca9bba1d.
Clean primitive: a monotonic postNavigationDeadline anchored to page1.goto completion, a remainingMs() closure passed as { remainingMs } into every optional stage, and schema-tagged (hyperframes.capture.phase.v1) CapturePhaseProgress events fed through onPhase. The commit-to-callback-not-snapshot design is the load-bearing choice for this fix — a caller reading the callback at each boundary observes the current remaining budget, not one frozen when the stage entered. That's carried through consistently for fonts, assets, contact-sheets, screenshots, and vision.
Concerns
- Video stage is the one exception to the callback-not-snapshot rule at this SHA.
packages/cli/src/capture/index.tsat thecaptureVideoManifestcall site passesdownloadBudgetMs: videoBudgetMs— a scalar snapshot ofremainingMs()taken before the video stage runs. InsidemediaCapture.ts(captureVideoManifest→sampleVideoDom→ the download loop), the loop bounds itself withremainingVideoDownloadTimeoutMs(dlStart, downloadBudgetMs)wheredlStart = Date.now()is taken aftersampleVideoDomcompletes (up tosampleMs≈ 12s). Net: the video stage can overrun the shared deadline by up tosampleMs. This is exactly the "outlives EF's watchdog" regression the PR is meant to close — the fix is real for every other optional stage but skipped for video. #2930 closes this gap by threadingliveRemainingMsthroughcaptureVideoManifestandsampleVideoDom. Worth calling out here because if #2928 landed onmainin isolation (unlikely given the stack intent, but the branches merge base-first), STUDIO-5419 still recurs for sites with video content. - Phase-event contract is asymmetric —
degraded-only branches skipstarted. Theelsebranches for fonts (index.ts:538), assets (index.ts:613), vision (index.ts:669and:674), and thephase("complete", "completed")at:812all emit a terminal state without a pairedphase(..., "started").CapturePhaseProgressdoesn't document thatstartedis optional, so any watchdog reconstructing a per-phase state machine (started → {completed|degraded}) sees skipped phases as terminal-only orphans. Either document the invariant inCapturePhase/CapturePhaseProgressJSDoc ("a phase MAY appear as a terminaldegradedevent without astartedpredecessor when it never ran") or emit a syntheticstartedbefore the terminal event. Impacts EF's ability to build a reliable capture-timeline visualisation on top ofHYPERFRAMES_CAPTURE_PHASErecords. lastPhaseis not emitted in the error-path JSON.packages/cli/src/commands/capture.tscatch block writes{ ok: false, error: errMsg }(nolastPhase) and aBLOCKED.md, then exits. The success path writeslastPhase: result.lastPhase. A caller consuming stdout JSON alone loses "how far did capture get" on exactly the failure the PR is meant to diagnose (outer-watchdog kill → thrown error). TheHYPERFRAMES_CAPTURE_PHASEdiag notices to stderr partially compensate, but the stdout JSON becomes a strictly weaker signal than the streamed events on the error path. Adding a capturedlastPhase(initialised to the module-levelbrowser/startedrecord and updated byonPhase) into the caller's scope, then emitted in the catch, would restore parity.
Nits
postNavigationBudgetMs = 0is coerced to default (120 000 ms).Number.isFinite(postNavigationBudgetMs) && postNavigationBudgetMs > 0atpackages/cli/src/capture/index.ts:72-74treats zero/negative/invalid identically. A programmatic caller wanting "no post-navigation work" can't express it — they'd get the full budget silently. The JSDoc(default: 120000)doesn't disclose the fallback also fires on0. Either accept0as "disable post-navigation stages" or document the coercion. Given the fix motivation, the fallback-is-safety-floor semantics is defensible — just make it discoverable.packages/cli/src/capture/indexBudget.test.tsis a source-text tripwire, not a behavioural test. The threeexpect(source).toContain(...)assertions match on internal template-literal substrings inside apage1.evaluateblock. They'd trip on any whitespace refactor without catching a real regression, and they verify zero runtime behaviour. #2930 deletes it; nothing of substantive coverage is lost. Noting because for the intervening merge state (mainimmediately post-#2928), this fragile pattern is protecting a browser-side eval string that no other test exercises.
What I didn't verify
- Windows CI (
Tests on windows-latest,Render on windows-latest) was stillpendingat review time; Ubuntu + macOS matrix is green. - I reasoned about (didn't repro) the video-stage overrun; the sub-agent I ran on this PR confirmed the snapshot semantics against the diff, and #2930's addition of
liveRemainingMs(opts ?? {}, ...)insampleVideoDomis prima-facie evidence the issue is real.
— Review by Rames D Jusso
ca9bba1 to
765a5ae
Compare
What
Add a cooperative post-navigation capture budget and stable phase breadcrumbs. Bound optional fonts, assets, screenshots, media, vision, and contact-sheet entry points without changing the navigation timeout.
Stack 1/4 for STUDIO-5419. Next:
fix/studio-5419-capture-runtime-tests.Why
Optional capture stages had no shared deadline, so HyperFrames could outlive EF's outer watchdog and leave only a caller-side timeout. This PR establishes the runtime contract used by the rest of the stack.
How
Track one monotonic deadline after navigation, pass live remaining-budget callbacks into optional stages, emit non-sensitive phase records, and expose the final phase in JSON output. Changed LOC: 661.
Test plan