fix(cli): propagate the live capture budget - #2930
Conversation
vanceingalls
left a comment
There was a problem hiding this comment.
Live-budget propagation into the primitives that #2928 gated externally. APPROVE.
Notable behavior:
resolveVisionPhaseCompletion(outcome, remainingMs)distinguishesrequest-timeoutfrombudget-exhausted— newreason: "request-timeout"in theCapturePhaseProgressunion. Priority order is correct:budgetExhausted || remainingMs<=0wins overtimedOutRequests>0(a capture that ran out of budget WHILE also timing out reports the budget cause, which is the more actionable signal for EF). Good.- Lottie now budget-threaded internally —
saveLottieAnimationsgates per-iteration + capsAbortSignal.timeoutatMath.min(10_000, remainingMs).renderLottiePreviewshas three fresh gates (beforereaddircontinuation, beforenewPage(), and immediately beforescreenshot()).breakinside the inner try correctly runs thefinallythat closespreviewPage, so no page leak on budget-exhaust mid-iteration. Verified with the "does not open another Lottie preview page after the live budget expires" test (line 261-282). captureFullPagePlatenow checks budget betweendocHeightmeasurement andpage.screenshot({fullPage: true})— thescreenshotcall is the expensive one (native Chrome capture of an 8000px plate); gating just before it is the right seam. The fake-timer test at line 630-644 (evaluate advances time by 100ms mid-call, then post-call check returns null) validates the exact narrow window.sampleVideoDomwaitsMath.min(2000, budgetMs-elapsed, liveRemaining)— if any of the three is 0, waitMs=0 and setTimeout is skipped entirely (not zero-ms setTimeout that microtask-loops). While-loop next iteration exits onliveRemainingMs > 0check. Clean.--capture-budgetdescription contains all the safety keywords — "post-navigation", "cooperative", "not a hard wall-clock timeout", "already-started native/core work", and "--timeout". Test at line 51-64 asserts every substring. The description-is-part-of-the-contract shape is unusual but here it's justified: this flag WILL be miscategorized as a hard timeout by anyone reading only the name, and the corrective language belongs in the description proper (not just docs). Fine.parseCaptureBudgetvalidation —Number.isIntegerrejects"Infinity"(returns false),"0.5","not-a-number", and<= 0. Test coverage is comprehensive (line 77-97).
Nits (none blocking):
indexBudget.test.tsis deleted with no replacement — the invariant it guarded (Math.min(15_000, remainingMs())bound + JS-side deadline gate in thepage.evaluatetemplate literal) is no longer test-locked. I understand the deletion (source-string tests are fragile), but the specific invariant is the one place in the code where the budget lives inside a template-literal string that's evaluated in the browser, so behavioral testing genuinely can't reach it. Consider re-adding at least theMath.min(15_000, remainingMs())substring assertion; the other two (lazyLoadDeadlinename +if (Date.now() >= lazyLoadDeadline) break;shape) can go.liveRemainingMs(budget, fallbackMs)uses different fallbacks per site — 10_000 for save/download,1for lottie/screenshot gates, 2000 for sampleVideoDom wait clamp, 120_000 for video download. That's fine but the heterogeneity means "no budget provided" behaves very differently in different call sites (video download runs to 120s, screenshot gate runs to 1ms). Documented by the parameter name; just note that no-budget tests are effectively no-op-timeout scenarios that don't exercise the timeout path. That matters for the tests in this PR that intentionally passbudget: {}— those test the shape but not the timing.- The
assetDownloader.tscomment update (line 267-272) is scope-adjacent to #2928's font-attempt cap change and would arguably read better as part of that PR's diff. Trivial — Graphite handles the interleaving fine either way. - Video preview screenshot gate at line 543-553 is
if (liveRemainingMs(opts ?? {}, 1) > 0)— the fallback of1means "unbounded" in the no-budget test case (any positive value passes> 0). That's semantically "unlimited" here which is correct. Consistent with earlier code.
CI clean at ce7ffcee. remainingVideoDownloadTimeoutMs unit tests + all four new behavioral tests (lottie fetch, lottie preview, video sampling, scroll settling, plate screenshot) are the strongest layer of the stack — thanks for the fake-timer discipline.
Stack-level observation: the combined package (#2928 sets contract + gates externally, #2929 covers regressions, #2930 threads it through the primitives) is a clean 3-PR shape. EF gets a schema-versioned phase stream, HF gets cooperative deadlines all the way down, and rollback is per-PR if any layer misbehaves. Ready to ship.
— Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at ce7ffcee.
Completes the fix. #2928 established the callback contract; this PR propagates it into provider layers where SDKs may ignore AbortSignal — the Gemini generateContent path, media probe walks, screenshot capture, and asset/font fetches. parseCaptureBudget in packages/cli/src/commands/capture.ts rejects "0", "-1", "0.5", "Infinity", and "not-a-number" before capture starts, so bad CLI input surfaces at the argv boundary rather than mid-run. resolveVisionPhaseCompletion codifies the priority: budget-exhausted beats request-timeout when both are true, which reads as the right call — budget exhaustion is the higher-order cause and gets recorded first. The new reason: "request-timeout" in the CapturePhaseProgress union closes the gap where a vision batch produced timedOutRequests > 0 but a positive remainingMs, and downstream watchers can now distinguish "we ran out of budget" from "provider hung on request N".
Concerns
- OpenRouter response-body reads are still unbounded — the PR body claims "bound provider response-body reads" but this is not honored on the OpenRouter path.
packages/cli/src/capture/contentExtractor.ts:60-78—runBoundedVisionRequestclears itssetTimeoutinsidefinallyand only callscontroller.abort()from within the timer callback. Oncefetch()wins the race with headers, the timer is cleared and theAbortControlleris orphaned. The subsequentconst detail = await res.text().catch(() => "")at:311andconst data = (await res.json()) as { ... }at:314are executed with no active signal — no budget race, no size cap, no timeout. A provider that returns headers withintimeoutMsbut streams (or stalls) the body will hang past the vision timeout and past the capture budget, which is the exact "SDKs that ignore abort" failure mode the PR is closing. Contrast withpackages/cli/src/capture/assetDownloader.ts'sfetchBuffer(url, timeoutMs)— it usesAbortSignal.timeout(timeoutMs)directly on the fetch call, so the signal stays armed througharrayBuffer(). Two viable fixes: (a) useAbortSignal.timeout(timeoutMs)inrunBoundedVisionRequestand drop the manual timer-then-clear pattern, so the signal aborts a stalled body read; (b) wrapres.text()/res.json()in anotherPromise.raceagainstremainingMs(). Either closes the OpenRouter body-read gap. Gemini path is unaffected — the SDK returns a parsed response, not a streamingResponse.
Nits
RemainingBudget = {}default on every new signature (saveLottieAnimations,renderLottiePreviews,captureFullPagePlate,sampleVideoDomviaopts ?? {}, etc.) makes each budget guard silently a no-op for any caller that forgets to plumb it —budget.remainingMs?.() ?? fallbackreturns the fallback (10_000,2000,1), which tripsguard > 0as true. Every call site inpackages/cli/src/capture/index.tsdoes thread the callback through, so today this is defence-in-depth rather than a live regression. But the type-level default hides future additions where a new caller is added and the guard silently disables. Making the parameter required (and having any legitimate no-budget caller pass an explicit{ remainingMs: () => Number.POSITIVE_INFINITY }) would make the guard load-bearing at compile time.packages/cli/src/capture/mediaCapture.ts:506-510drops the aggregatedownloadBudgetMsceiling whenopts.remainingMsis defined. The iteration break usesliveRemainingMs(opts ?? {}, remainingVideoDownloadTimeoutMs(dlStart, downloadBudgetMs))— that's fallback semantics: ifopts.remainingMsis present, the aggregate cap is dropped entirely rather thanMath.min'd in. Compare with the per-request timeout at:556-559which correctly takesMath.min(remainingVideoDownloadTimeoutMs(...), liveRemainingMs(opts, ...)). Currently harmless becauseindex.ts:499passesdownloadBudgetMs: videoBudgetMs(a snapshot ofremainingMs()) and both signals shrink in parallel with the live budget. A future caller that wants a stricter video-specific cap would have it silently bypassed whileremainingMs()still has time — same shape as the concern above.indexBudget.test.tsdeletion is coverage-neutral in practice, but the three source-text assertions on the lazy-load loop are gone with no direct behavioural replacement (the loop is insidepage1.evaluateand hard to exercise in vitest). The invariant is now defended only by review; the deleted test was low-signal, so no substantive loss.
What I didn't verify
- Windows CI at
ce7ffceeis green. I did not repro the vision-body-unbounded failure against a real slow-body provider — the finding is from tracing signal ownership throughrunBoundedVisionRequestand its callers. - The stack's 4/4 (
fix/studio-5419-blocked-and-degraded-captures) may already fix the OpenRouter body-read gap. If so, the finding above can defer there instead of landing in this PR — worth confirming.
— Review by Rames D Jusso
ce7ffce to
d360760
Compare
647a5ec to
45aead8
Compare
What
Propagate the live remaining capture budget through provider and media boundaries, bound provider response-body reads, and validate budget milliseconds before work starts.
Stack 3/4 for STUDIO-5419. Base:
fix/studio-5419-capture-runtime-tests. Next:fix/studio-5419-blocked-and-degraded-captures.Why
A timeout snapshot can expire while a phase is already waiting. The caller then observes a hang even though the capture command accepted a budget.
How
Pass deadline callbacks instead of static values and race response/body settlement against the remaining budget, including SDKs that ignore abort. Changed LOC: 466.
Test plan