Unify the driver observer drain, survive mid-step navigation, clean up on signal - #170
Merged
Merged
Conversation
…p on signal Close the Event Timing delivery race: the end-of-step flush drains every in-page observer's takeRecords() and waits bounded for a dispatched interaction's entry, so a lost entry can no longer read INP low past a --max-inp gate. The default settle re-attaches to the new document when a hard navigation commits mid-settle instead of failing the record. A module-level disposer registry SIGKILLs Chrome and unlinks temp files on SIGINT/SIGTERM/SIGHUP, then re-raises. Also renames the in-page __cp* globals to __wpd*.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three driver-reliability fixes from the concurrency/cleanup review. Patch-level (post-1.0): all three are fixes, no surface change.
1. Unified observer drain + INP delivery-race (top priority)
The end-of-step flush read INP, LoAF and layout-shift point-in-time after one rAF + one macrotask — only LCP and soft-nav had a
takeRecords()drain. An entry queued-but-undispatched at that instant was silently lost, and a lost Event Timing entry reads INP lower, soassert --max-inpcould pass a real regression.takeRecords()drain in one in-page registry (win.__wpdDrains); the flush drains every observer before it reads, from both the immediate-finish path and the boundedsettleRead.win.__wpdSawInteraction, set only by trustedpointerup/click/keydown/keyup— a syntheticpage.evaluateclick is untrusted and arms nothing) waits up toINP_ENTRY_WAIT_MS(250 ms) for its entry, a tighter sibling ofLCP_ENTRY_WAIT_MS.docs/dev/navigation-and-lcp.md. E2e guard: a 45 ms handler's INP must survive the race (non-null, >=40 ms, processing recovered).2. Disposer registry + signal handlers
src/had zero signal handling, so SIGINT/SIGTERM/SIGHUP orphaned the Chrome process, the temp static server, and the gecko dump temp (field-confirmed: an orphaned Chrome after a SIGTERM'd run). Newsrc/browser/disposers.ts: acquisition sites (browser launch, server listen, gecko temp creation) register a synchronous best-effort disposer and deregister on clean release; a handler runs them once, synchronously (browser.process()?.kill("SIGKILL"),unlinkSync), then re-raises the signal so exit codes stay conventional. No async in the handler, no new dependency. Unit test covers register/release/run-once/throwing-disposer.3. Default settle survives a mid-step hard navigation
A hard navigation that commits during the default
page.evaluate(SETTLE_SOURCE)threw "Execution context was destroyed" — not matched byisTransientNavError, so the record hard-failed. The settle now catches the destroyed-context family and re-attaches to the new document (mirroringwaitForStable's re-attach), bounded. A single immediate re-attach still raced the mid-commit context, so the re-attach backs off briefly and retries up toSETTLE_REATTACH_LIMIT(3) before failing honestly — truer towaitForStable's bounded-loop re-attach than a literal single retry. E2e guard: a step whose action reloads mid-settle completes as a hard navigation instead of failing.4.
__cp*->__wpd*(cosmetic)Mechanical rename of the in-page globals to match the internal docs; all tests stay green.
Gates
build, lint, format:check, knip, unit (685), measurement (6), chrome e2e (51) — all green. Self-reviewed via a hostile two-pass (the review subagent is unavailable in this environment): pass 1 caught the single-retry commit race (-> bounded backoff loop) and a lint spread warning (-> direct Set iteration); pass 2 verified the INP wait stays after the end mark, disposer idempotency/deregistration across every launch/close/gecko path, and that the drain registry and interaction listeners do not accumulate across a navigation re-arm.