Repository navigation
fix(engine): a PNG-sequence frame no longer captures DOM still pending in virtual rAF - #6599
santhiprakash wants to merge 5 commits into
Conversation
|
Verification numbers on this branch (Linux aarch64, Chromium 156,
One caveat worth stating plainly: the single remaining blank frame is the t=0 frame. |
8a28f42 to
37ee072
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
Reviewed 37ee0721eb4f0c24275de121e1e0be8c1b8917ba across all five changed files, including the actual generated shim, bridge seek order, capture barrier, same-origin child caller, and package-budget delta. Two changes are needed:
- [P2] Fresh finite work is blocked when its callback was used in the previous seek.
packages/producer/src/services/fileServer.ts(isDeferred, lines 361–365) tests callback identity rather than whether this registration belongs to the old loop. With one reusablerevealfunction, each renderSeek resets a five-turn reveal and requests it anew. The first seek settles all five turns; the second flush runs one, then the drain leaves four pending. A fresh function per seek passes. I reproduced this using the exact shim + bridge + settle bodies in Chrome 156: the first screenshot is green, the second stays red. Track loop status/allowances per registration lineage so independent invocations can settle, and add a regression with the same finite callback on consecutive seeks. Keep the perpetual-loop controls. - [P2] Retain bounded lineage, rather than every historical entry.
packages/producer/src/services/fileServer.ts:461: every rescheduling entry points to the entire previousactiveEntrythroughparent, but lookup stops after 64 hops. A page-load self-loop leaves one queued callback retaining 1,001 entry ancestors after 1,000 seeks. Separately, after 1,000 completed two-turn finite invocations the queue is empty butdrainRunCountsstill strongly retains all 1,000 callbacks. Long renders therefore accumulate history that the guard no longer needs. Bound/compact parent history and use collectible or reclaimed accounting; add a many-seek retention control. This is demonstrated growth, not a measured out-of-memory failure.
Verification: the prior 13 passing shim cases are reused with identical generated execution AST (comments removed); the queued rerun was cancelled before execution due runner backlog. Five independent capture-barrier checks pass (free-running/BeginFrame/idle, real deadline, absent shim). The reused-callback failure has raw Chrome screenshots and pixel checks, with a passing fresh-callback VM control. No API-absent mutant is counted as behavioral discrimination. I did not rerender the original issue composition, run full PNG/encode parity, or independently measure the reported +14/+18 ms per-frame cost. The documented remaining t=0 blank frame and performance tradeoff remain limits, not evidence of complete parity. Same-origin children are sought directly, but the added barrier evaluates the top document only.
Current-head checks: required checks pass; the skills-manifest check is skipped. No other peer review was present at the publication guard.
Need: the capture-side placement addresses a real readiness gap. Size: localized changes; the unrelated CLI budget increase should stay justified by measured upstream drift. Reuse/Simpler/Structure: preserve the single seek flush and real timer, while separating fresh work from loop identity and bounding retained state.
Verdict: request changes.
Reasoning: repeated finite work still reaches capture unfinished, and scheduling bookkeeping grows with render length.
— Magi
Deferral and the drain-run allowance were keyed on callback identity, so a
finite chain re-requested with the same function on the next seek was left
pending, and per-callback maps plus unbounded parent chains grew retained
state with render length. Entries now carry their registration lineage
({epoch, drainRuns}, shared by every link of one invocation) plus a bounded,
callback-deduped ancestor record list for cycle detection: re-requested
finite work opens a fresh lineage and settles, established loops keep their
one-turn-per-seek pace, and a completed invocation's bookkeeping is
collectible.
|
Both P2s are addressed on [P2] Fresh finite work blocked when its callback was used in the previous seek. Root cause: Sabotage on [P2] Bounded lineage, collectible accounting. Verification: |
4b5ba84 to
269f9eb
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 269f9eb2b7e310f1a9a9e633fb0855c7263ccd3d.
The earlier callback-history and retention findings are resolved by per-registration lineage and collectible bookkeeping. I traced the virtual clock, capture caller and post-seek settling path in two independent passes.
The exact extracted shim passes 16 authored Bun VM tests, including repeated finite callbacks and retention controls. The exact extracted settling function passes 6 authored tests. The extraction keeps the function bodies unchanged and changes only test imports. This is not a packaged runtime build, Chrome capture or PNG-render acceptance.
Nit: frameCapture.ts:3339 reports imagesDecoded = decodes.length even if the deadline wins before completion. Count completed decodes inside their handlers so the diagnostic reflects completed work; keep the bounded wait.
Need: yes · Size: ok · Reuse: existing frame clock and settling budget · Simpler: bounded registration state · Structure: fine
Risk: medium - virtual frame callback lifecycle and post-seek settling.
Verdict: APPROVE
Reasoning: The scoped controls and independent source pass resolve the prior findings; the remaining counter accuracy issue is non-blocking.
Reasoning: The scoped shim and settling controls pass, and earlier callback-retention findings are resolved.
269f9eb to
157c10e
Compare
157c10e to
f8c8f7b
Compare
- Problem: the virtual-rAF drain and seek-settle fix adds engine and producer code that is bundled into producer's four dist bundles (index.js, public-server.js, distributed.js, runtime.iife.js) and the rolled-up service declarations, pushing the unpacked tarball to 3178884 bytes — over the 3130000 budget by 1.56%, past the 1% gate. - Fix: raise @hyperframes/producer unpacked budget 3130000 -> 3180000, the repo-sanctioned path (verify-packed-manifests error text: raise the budget only with the reason in the PR). Reason: size growth is the fix's own code multiplied across the dist bundles; no source maps, tests, or unused assets entered the tarball. - Verification: fresh bun run build at f8c8f7b reproduces CI's number exactly (3178884, ci.yml Build job 128704040260); npm pack after the bump: 3.2 MB unpacked <= 3211800 allowance (headroom 32916).
What
Keeps
seekToTime's single virtual-rAF flush and moves the bounded drain into the capture path. After__hf.seekreturns,prepareFrameForCaptureruns a new per-seek barrier,settlePostSeekDomUpdates: the shim'sdrainAnimationFramesruns still-queued author rAF work at the frozen virtual time, then the barrier decodes newly active images, waitsdocument.fonts, and commits two native paints before the screenshot when the compositor free-runs.Why
Fixes #5888. On a PNG-sequence render, each worker's first frames could capture the DOM while an update deferred through author
requestAnimationFrameturns was still pending — reproduced upstream at 18/48 frames with no text on 0.8.193. This is the resubmission invited in #6007, in the requested shape: the drain lives where the post-seek settle runs, and it stops instead of multiplying perpetual loops.Related work
Fixes #5888. Resubmits the closed #6007 in the shape its review requested — drain in the capture path rather than inside
seekToTime, a registration-lineage guard for loops, and a timing measurement on a render longer than 48 frames. The settle barrier and the shim change are split into separate commits.How
packages/producer/src/services/fileServer.ts(virtual-time shim):seekToTimestill calls onlyflushAnimationFrame(). Registrations now carry seek lineage:hf.seekarms a boundary viabeginSeek()beforerenderSeek, which is where timelineonUpdatehooks queue the deferred work a seek must settle;seekToTimerolls the boundary itself for direct callers such as same-origin child frames. A self- or ancestor re-registration inherits that ancestor's lineage, so a registration cycle stays distinguishable from spawned deferred work.__HF_VIRTUAL_TIME__.drainAnimationFramesruns queued entries to quiescence except: lineage registered before the first seek (a page-load self-rescheduling loop keeps exactly one turn per seek), callbacks that already ran in the previous seek (established loops), and callbacks past a 16-execution lifetime drain allowance (a perpetual driver spawned mid-seek stays bounded). Turn (64) and execution (2048) caps bound queues that never converge.function tick(){ rotation++; requestAnimationFrame(tick) }ends at 24 runs over 24 frames.packages/engine/src/services/frameCapture.ts:settlePostSeekDomUpdatesruns inprepareFrameForCaptureafter__hf.seekand LUT waits, before compositing.Date/performance.noware virtualized in-page, so the shared 1500 ms wall-clock budget is armed through the shim's storedoriginalSetTimeout(HF_POST_SEEK_SETTLE_MS; 0 disables the settle). The two-paint commit uses the storedoriginalRequestAnimationFrameand only runs for free-running compositors — BeginFrame sessions skip it since the capture's own BeginFrame is the commit — and only when the settle observed pending work.Test plan
packages/producer/src/services/fileServer-rafDrain.test.ts(13 bun:test cases: deferred chains settle at the current seek, first-seek registrations drain, page-load perpetual loops keep one turn per seek, spawned loops stay bounded, non-converging queues cap, stats and cancellation) andpackages/engine/src/services/frameCapture.postSeekSettle.test.ts(6 vitest cases: drains work queued after the seek, decodes images once across seeks, two-paint commit only on observed work and a free-running compositor, no-ops without the shim).PRODUCER_FORCE_SCREENSHOT=true,--format png-sequence --fps 24 --workers 2 --experimental-fast-capture=false):onUpdatedoes not fire and the only pending rAF work is the chain queued at page load — a self-re-registering lineage indistinguishable from a perpetual loop. Running it to completion is the 64-turns-per-frame behavior the earlier version was closed for, so the drain correctly leaves it on its one-turn-per-seek pace.+18 ms/frame). The same composition without the deferred reveal: 3.4 s to 4.7 s (+14 ms/frame) — the two-paint commit dominates whenever a seek's flush ran rAF work, which GSAP's ticker always does.Sabotage check: all 13 shim tests fail against upstream's
fileServer.ts(the drain API does not exist there); the 6 settle tests exercise a function absent before this change.scripts/package-size-budgets.jsonbumps only@hyperframes/cliunpacked for upstream drift (24,067,526 bytes measured vs the 23,722,874 budget — over by more than 1% before this change). The packages this PR touches still fit:@hyperframes/engine451,480 packed vs 456,397,@hyperframes/producer820,762 vs 828,932.AI-assisted. I wrote and verified this change.