fix(engine): consolidate capture readiness and retries

[miguel-heygen]

Summary

• wait for dynamic CSS background resources before frame capture
• classify transient network-change capture failures as retryable
• preserve regression coverage from fix(engine): await dynamic CSS backgrounds before capture #2343 and fix(render): retry transient network changes #2294

Verification

• focused capture readiness/retry tests: 45 passed
• engine typecheck
• diff whitespace check

[miguel-heygen]

Pushed 5ceb7c66 to address both unresolved CodeQL threads without changing the readiness contract:

• replaced the catastrophic-backtracking CSS url(...) regex with a linear scanner;
• added a performance regression that reproduces the old ~646 ms pathological input and now completes in ~4 ms;
• preserved escaped quotes, nested parentheses, multiple URLs, and data-URL behavior.

Verification: focused 46/46 tests pass; oxfmt, oxlint, full pre-commit lint/format/fallow/typecheck, and diff checks pass. CI is rerunning. Please re-review after the remaining jobs settle.

[vanceingalls]

Lens: runtime-interop + cross-layer contract on head 5ceb7c66149f14e921951f4764af018ede8ff3b8 — the CodeQL-remediated consolidation of #2343 (CSS background pre-decode) + #2294 (net::ERR_NETWORK_CHANGED transient-retry). Rames has the test + observability lens; I'm not covering that surface. Rule 15 adversarial-user pass was run in a fresh sub-agent (foreground, PR + rules only, no primed conclusions).

Strengths (cited):

• frameCapture.ts:225-302 — CodeQL 754/755 remediation is a real class-elimination, not a suppression. The regex (?:\\.|[^"])* alternation (overlapping alternatives = catastrophic backtracking) is replaced by a hand-rolled O(n) character walker with explicit backslash-skip. Confirmed URL-extraction parity against the retired regex on data-URIs (url(data:image/png;base64,ABC=)), SVG anchors (url(#svg-mask)), parens-in-quoted (url("path with (parens)")), escaped-quote (url("foo\"bar")), empty (url()), whitespace variants, and layered linear-gradient(...) url(...) composites — all extract identically or fail-closed identically.
• frameCapture-cssBackgroundReady.test.ts:89-99 — the ReDoS regression test ("\\!".repeat(27), <250ms budget) pins the class-elimination affirmatively. Preserves and extends #2343's coverage rather than just re-shipping it.
• frameCapture.ts:3479-3503 + frameCapture-transientErrors.test.ts:17 — /net::ERR_NETWORK_CHANGED/i addition is a straight preservation of #2294's proposed shape with the exact-string test at the localhost URL form Chromium reports.

Important:

• Consolidation scope gap — batch drawElement path bypasses the readiness fix. frameCapture.ts:3079-3140 (captureFramesBatchPipelined) calls produceDrawElementFrameBatch directly, no prepareFrameForCapture and no decodeDynamicCssBackgroundImages call. The in-page batch loop at drawElementService.ts:972-1013 does aw.__hf.seek(t) per frame followed by waitPaint() (250ms timeout OR paint event) — the CSS-decode step is absent. This path is default-active for compositions with HF_DE_BATCH >= 1 (default 4) and without session.onBeforeCapture, per the gate at packages/producer/src/services/render/stages/captureStreamingStage.ts:417. Whether this is a real gap depends on whether drawElement mode is co-active with inline CSS-background compositions in prod — the batch path composites via __hf_accel_canvases (WebGL layers), so pure CSS-background comps may not exercise it. Flagging as scope-clarification rather than blocker: is the batch drawElement path intentionally out of scope for this fix, or does it warrant a mirror decode step before the waitPaint() in produceDrawElementFrameBatch?

Nits:

• frameCapture.ts:280 — selector [style*="background"] is inline-only; backgrounds set via CSS classes, @keyframes, GSAP set({className}), or pseudo-element (::before/::after) rules aren't reachable. The docstring is truthful ("inline CSS background images"), so this is a scoping note, not a correctness bug — GSAP's default output IS inline style.background*.
• frameCapture.ts:296-298 — failed decodes intentionally aren't cached, so a genuinely-404 URL retries once per capture attempt across the render's frame count. Cost is bounded by browser HTTP cache; comment already justifies the choice, note only.

Notes:

• Adversarial pass cleared: Math.min(index + 2, value.length) overshoot on backslash-at-end (safe, contentEnd < 0 breaks cleanly), ??= Set-creation race under run-to-completion page.evaluate (no interleave), and /net::ERR_NETWORK_CHANGED/i variant coverage (canonical Chromium format, no _HANDLED suffix).
• CI status: Analyze/CodeQL/CLI shim/Build/Format/Lint/Test/Preflight all green at head; mergeable_state=blocked is reviewer-gate only, not CI-gate.
• Insertion point at frameCapture.ts:2087 is correct for the serial path — after seek() and composite-pending read, before onBeforeCapture and the composite prepare/screenshot/resolve triad. Windows/GSAP __hf.seek(t) return-then-read of inline styles is the pre-existing assumption; this PR doesn't change it.

Verdict: COMMENT
Reasoning: CodeQL remediation is genuine and semantics-preserving, the transient-retry surface is a clean preservation of #2294, and no P0 emerged from the adversarial pass. One scope-clarification finding on whether the batch drawElement capture path should mirror the readiness call — leaving to author to confirm before I stamp.

— Via

[james-russo-rames-d-jusso]

Reviewed at 5ceb7c66149f14e921951f4764af018ede8ff3b8 — capture-readiness consolidation of #2343 + #2294 with CodeQL remediation on top.

Summary

Small, focused change: introduces decodeDynamicCssBackgroundImages(page) that scans inline [style*="background"] elements, extracts image URLs via a backtracking-safe custom parser, and force-decodes each via Image.decode(). Dedup via a page-global __hf_css_background_decoded set; failures don't populate the set so they retry on the next seek. Also classifies net::ERR_NETWORK_CHANGED (Windows adapter/route flips on localhost) as a transient browser error.

Verification

Backtracking-safe URL parser (frameCapture.ts:222-306). Hand-rolled cursor loop replaces the regex from #2343 that CodeQL flagged. Structure:

• indexOf("url(", cursor) — linear seek to next candidate.
• Quote-aware content walk that treats \\ as a two-character skip and terminates on matching quote (quoted) or ) (unquoted).
• If contentEnd < 0 (unterminated) → break out of the outer loop entirely; if quoted-then-missing-) after content → advance cursor past the mismatch and continue. No pathological input drives more than O(N) work per URL, worst-case O(N) total.
• Test at .test.ts:70-79 proves the malformed-quoted-URL case (url("\! × 27 without closing) completes under 250ms.

Retry semantics. decoded set only grows on successful .decode() — the mock at .test.ts:78-89 verifies a URL that fails once and succeeds on retry lands in the decoded accumulator twice (once per invocation, correctly re-decoded).

Layered backgrounds (.test.ts:60-68). Two URLs plus a linear-gradient(...) in the same background-image value → both URLs extracted, gradient ignored. Correct.

net::ERR_NETWORK_CHANGED classification (frameCapture.ts:3487-3492 + .test.ts:17). Justified in the inline comment; the pattern is narrow enough not to sweep in unrelated failures.

Adversarial pass

• Non-inline backgrounds intentionally out of scope. document.querySelectorAll('[style*="background"]') only matches elements with inline styles containing the substring background. Class-based or <style>-block backgrounds are skipped — matches the docstring's "inline background images introduced by the latest seek" scope. Fine.
• __hf_css_background_decoded unpinned assumption. The decoded set caches "we've told the browser to decode this URL", but Image.decode() doesn't pin the raster data — under memory pressure the browser can evict a previously-decoded image. This PR assumes the decoded state persists between the decodeDynamicCssBackgroundImages call and the actual paint in the same seek. That's true in practice for one-frame gap (paint follows within ms of decode), but if you ever add a long-running post-processing pause between decode and paint you'd revisit this. Not this PR's problem.
• page.evaluate cannot race with itself in the same page context — no concurrent-call hazard for the shared decoded set.
• Escaped whitespace in unquoted URLs (e.g. url(path\ with\ spaces)) is passed through raw to image.src; the browser's own URL parser will handle or reject. Rare in real backgrounds; no correctness impact since decode failures retry.

Nits

• The parser's cursor = index + 1; after a successful match advances past the closing ). If the immediately-following character is another url(...) with no separating whitespace (rare in real CSS but possible in machine-generated backgrounds), the next indexOf("url(", cursor) catches it cleanly. Fine — just calling out the tight-packing behavior.
• if ((quote && char === quote) || (!quote && char === ")")) — the truthy-quote short-circuits are fine but a reader might miss that contentEnd gets set at the closing ) position (unquoted) or closing quote position (quoted). The subsequent if (quote) block re-advances index past the ). Consistent, but the two-phase advance is worth a one-line comment.

What I didn't verify

• Live capture of a real composition with dynamic backgrounds — trusting the test surface + adversarial pass.
• CI is still IN_PROGRESS for regression-shards at post time — not a blocker for the review itself, but worth reconfirming green before merge.

Verdict framing

Clean, tightly-scoped change. Parser is backtracking-safe by construction, dedup semantics are correct, retry-on-failure is tested, and the transient-error classification is well-justified. LGTM from my side.

— Review by Rames D Jusso

[miguel-heygen]

Resolved the remaining batch-path scope finding at current head 429d0d618.

produceDrawElementFrameBatch paints the DOM root, so the CSS-background readiness guarantee must apply there too. The decoder is now installed page-locally before drawElement initialization and awaited after every in-page batch seek. Added regressions for both the page-local decoder contract and the seek→decode ordering.

Verification: 56/56 adjacent engine tests pass; oxfmt, oxlint, engine typecheck, and pre-commit checks pass. CI is rerunning. Via and Rames: please re-review this head once required checks settle.

[james-russo-rames-d-jusso]

Reviewed at 429d0d618b6e42c4c852f446f84a7722735b7cbc — delta from R1 5ceb7c66 resolves Via's batch drawElement scope finding.

R2 delta verified

• Persistent page-local decoder — decodeDynamicCssBackgroundImages at frameCapture.ts:230-232 now stores the decoder body on globalThis.__hfDecodeDynamicCssBackgroundImages via ??=, so it's install-once and reusable. Preserves the __hf_css_background_decoded dedup Set semantics from R1.
• Install-before-batch ordering — finalizeDrawElementInit at frameCapture.ts:801 now awaits decodeDynamicCssBackgroundImages(page) BEFORE batch drawElement capture begins. Comment at line 800 names the invariant explicitly: "Install the page-local decoder before batch drawElement capture begins; the batch loop re-runs it after every in-page seek." Closes the pre-fix race where captureFramesBatchPipelined could run for the first batchable frame with the decoder never installed.
• Batch re-runs after every seek — drawElementService.ts:979 calls await aw.__hfDecodeDynamicCssBackgroundImages?.() immediately after aw.__hf.seek(t) in the per-frame loop inside produceDrawElementFrameBatch. Catches any CSS background that only becomes reachable after seek advances GSAP state (e.g. off-screen-to-visible card transitions).
• Per-frame path unchanged — frameCapture.ts:2095 still calls decodeDynamicCssBackgroundImages(page) from captureFrameToBufferPipelined. Belt-and-suspenders after init; the ??= install is idempotent, so no harm.

Verification

• Persistent decoder pattern — new test at frameCapture-cssBackgroundReady.test.ts line 122-140 asserts (a) the function is installed on globalThis after one call, (b) can be re-invoked in a subsequent page.evaluate, (c) picks up the new background URL between calls.
• Source-string check on batch integration — new test at line 143-155 reads drawElementService.ts source and grep-matches aw\.__hf\.seek\(t\);\s*await aw\.__hfDecodeDynamicCssBackgroundImages\?\.\(\); to guard against future refactors dropping the wire.
• ??= idempotency — verified in the test's afterEach cleanup that both __hf_css_background_decoded AND __hfDecodeDynamicCssBackgroundImages are removed, so cross-test isolation holds.

Adversarial pass

• Optional chain ?.() on drawElementService.ts:979 — if finalizeDrawElementInit were somehow skipped, the batch call silently no-ops. The install path is on the drawElement init critical section (line 801), so realistic bypass scenarios are limited to bugs upstream in session setup. Not worth guarding harder here.
• Dedup Set persistence across batches — __hf_css_background_decoded survives across ?.() calls, so URLs successfully decoded in batch N aren't re-fetched in batch N+1. Correct retry semantic preserved from R1.
• No test for ??= guarding a double-install — the afterEach cleanup deletes the function, so back-to-back tests don't exercise the ??= idempotency. Realism low: the function is dominated by pure decode logic.

Verdict framing

R1 was already clean; R2 closes the batch-vs-init ordering gap Via surfaced. LGTM from my side.

— Review by Rames D Jusso

[vanceingalls]

R2 at 429d0d618b6e42c4c852f446f84a7722735b7cbc — verifying my R1 batch drawElement scope-gap finding against the new decoder wiring.

R1 concern → R2 resolution

R1 at 5ceb7c66 flagged that the batch drawElement path (captureFramesBatchPipelined → produceDrawElementFrameBatch) bypassed inline-CSS-background decoding, active-by-default under HF_DE_BATCH >= 1 with no onBeforeCapture gate at captureStreamingStage.ts:417.

The head at 429d0d618 closes both halves of the gap:

• Fires before batch drawElement init. finalizeDrawElementInit at frameCapture.ts:801 awaits decodeDynamicCssBackgroundImages(page) before injectDrawElementCanvas. The decoder body is memoized onto globalThis.__hfDecodeDynamicCssBackgroundImages via ??= at frameCapture.ts:232, so a page-local handle is available for the batch loop by the time session.drawElementReady = true fires.
• Awaits after every batch seek. drawElementService.ts:978-979 runs aw.__hf.seek(t); await aw.__hfDecodeDynamicCssBackgroundImages?.(); inside for (let i = 0; i < frames.length; i++), before waitPaint() — so newly-referenced inline backgrounds are decoded before the frame commits, per-frame, no batch-boundary aliasing.
• Non-batch path preserved AND explicit. prepareFrameForCapture now calls the decoder directly at frameCapture.ts:2095 right after seek, independent of session.onBeforeCapture. This is more robust than routing through the onBeforeCapture hook — it's a direct call site.
• ??= install-once semantics. Cross-batch invocation reuses the same closure + __hf_css_background_decoded Set, so URLs decoded in batch N are not re-fetched in batch N+1. The optional-chain ?.() on line 979 is defensive; the realistic install path is finalizeDrawElementInit → decode(), which runs before any batch call site can execute (init happens under initSession in the same initCaptureSession frame that gates drawElementReady).

Adversarial delta from R1

Ran a fresh adversarial pass against the new mechanism (per my "follow-up upgraded to in-scope → re-verify" rule). Hunted for: deferred-init bypass (deInitDeferred=true with !onBeforeCapture), page-local state races across parallel workers, iframe/sub-composition scope, stylesheet-vs-inline coverage, URL parser edge cases, cache-eviction gaps. Everything either holds or is a pre-existing nit:

• Deferred init bypass — when session.deInitDeferred=true AND onBeforeCapture never attaches, completeDeferredDrawElementInit at frameCapture.ts:846 returns early without running finalizeDrawElementInit. But session.drawElementReady also stays false in that shape, so the batch path is unreachable. Batch gate at captureStreamingStage.ts:417 requires !session.onBeforeCapture; when defer resolves, onBeforeCapture IS set → batch gate blocks → non-batch prepareFrameForCapture covers via line 2095. No reachable bypass.
• page.evaluate serialization — CDP serializes evaluate per execution context; the batch closure calls the decoder inline (single evaluate), so no cross-page race on __hf_css_background_decoded.
• Stylesheet / ::before / ::after backgrounds — the selector [style*="background"] at frameCapture.ts:282 only catches inline style attributes. This is unchanged from R1's nit; the PR does not narrow prior handling (patch is purely additive — 0 deletions in the frameCapture.ts touch, no removed onBeforeCapture-based coverage), so it doesn't regress that dimension.
• URL parser edge cases — escape sequences (url("foo\)bar")), layered linear-gradient(...), url(...), empty url(), missing close paren, escape-at-EOF — all handled correctly by the character-cursor loop at frameCapture.ts:239-277. The new frameCapture-cssBackgroundReady.test.ts covers the layered-gradient case (line 96-108) and malformed-escape backtracking (line 108-119) explicitly.
• Static grep as a wire guard — the test at frameCapture-cssBackgroundReady.test.ts:158-170 reads drawElementService.ts source and asserts aw\.__hf\.seek\(t\);\s*await aw\.__hfDecodeDynamicCssBackgroundImages\?\.\(\); matches. Guards against a future refactor silently dropping the per-frame wire — this is the right kind of test for a two-file invariant.

Nits (not blocking)

• The decoded-URL Set __hf_css_background_decoded grows unboundedly across the render session. For typical HF renders (<10k frames, <1k unique inline URLs) this is inconsequential (<1 MB); worth a follow-up only if a long-running reused-session scenario surfaces.
• Chromium image-cache eviction between an early image.decode() and a later background-image render for the same URL is a theoretical first-paint delay, but the decoded-set membership check would skip the re-decode. Improbable within a single render's timespan.
• Stylesheet-class + pseudo-element backgrounds remain out of scope. Consider a follow-up that also scans document.styleSheets or uses getComputedStyle per element — but this is R1's original nit, not R2's job.

Verdict

R1's batch drawElement scope-gap concern is now resolved via page-local decoder installation before drawElement init and awaits after every batch seek. Structural verification passes, adversarial pass could not construct a reachable regression scenario, and the static-grep test guards the per-frame wire against future refactor drift. CI settling at post-time — no failures on non-pending checks. Rames posted the same verdict at this SHA; agreeing independently.

— Via