Pre-deploy fixes for readiness contracts: the quiet monitor was off by one plateau; expectations keyed per page (browser v1.31.0, plugin v0.80.0)

[harper-joseph]

A pre-deployment review of what #187 shipped (browser 1.30.0 / plugin 0.79.0), before either reaches a node. Three reviewers read the browser path, the plugin path, and the deploy machinery independently; everything below was then reproduced against the compiled bits or against captured markup.

Report mode is confirmed non-gating and cheap (~0.5% of settle CPU; no settle phase changes; outcome never consults satisfied). What follows is what would have gone wrong after that — when the contracts are armed — plus what would have made the report window's data untrustworthy.

browser v1.31.0 — the DOM monitor did not measure quiet

onSatisfied: 'quiet' stops a render when the contract holds and the DOM has been quiet for quietMs. The second half comes from domMonitor.ts, and it had two defects:

quietMs() returned the start of the previous plateau. The scan found the last history entry outside tolerance and returned its timestamp — when the count was last far from its current value — rather than the next entry's, which is when the current plateau began. After a still period followed by a burst, it reported the pre-burst quiet as if it had accrued since. Reproduced against the compiled monitor: 2s still, 500 insertions, read 150ms later → 2,153ms quiet. On the canonical page shape (lull, then the rails arrive and make the last clause true) held && quiet stopped the instant the contract held, with no dwell — the stop-on-contract-alone behaviour we measured losing 99% of product links. It also explains why catalog needed quietMs: 750 where 250 lost links: the pre-rails lull was being misread as post-rails quiet.

The element count was inflated 2–3×. Every added node was countTree'd at callback time, so a parent inserted with its children in one task counted the children through the parent and again through their own records. Measured: parent + 10 children → +21; a 13,128-element document → 41,878, varying with where the parser yielded. Against a tolerance of 120, ~100 elements of carousel churn read as ~250 and the page read as never quiet — fail-safe, but every churning page would have paid the rot valve plus the full fallback settle.

Both fixed; test/domMonitor.test.ts pins them (there was no monitor test). Also: the monitor is not installed in report mode (nothing reads it, and it is the one page-visible artefact — a patched attachShadow); the observer stops polling once every clause has held (~half the evaluates per fleet render); the settle is wrapped so a throw releases the observer instead of letting the slow tail drop out of the satisfied_ms distribution.

plugin v0.80.0 — the expectation history mixed device variants

RenderExpectation was keyed by URL and fed by "the first stored variant" — desktop in steady state, but mobile whenever desktop failed to store. Measured on the live home page, same URL, one job: 625 img desktop vs 256 mobile, a 59% gap against a shortfall rule that fires at 50%. That artefact lands on the shortfall series and in the warn log looking exactly like real content loss — the one thing the table exists to detect.

Now keyed by cache key (URL + device), every stored variant recorded under its own key. 0.79.0 has not been deployed, so nothing migrates. Also: the table expires at 30 days and Target.delete cascades to it (it had no cleanup path); the pre-commit metric loop reads wire arrays defensively (a non-iterable from the unauthenticated result endpoint would have 500'd the result and held the lease); the missing-table warn is once-then-every-thousandth instead of once per render; verdict is documented as one per device variant (a two-device job emits two — read shares against render.time_ms, not render.outcome); learned: {} no longer wins over a sibling with observations. RenderQueue-level tests added for all of it.

console — the coverage guard has been red on main since 0.79.0

test/adminAssets.test.js correctly refuses a plugin metric no view reads, and render_readiness is one. Waived per series pointing at #189 (the Readiness panel); the metric is read raw from /prerender_admin/analytics during the report-only window.

Verification

• browser 294/294, plugin 1172/1172, console 276/276; lint and format:check clean.
• All five captured page types (PDP with/without reviews, populated/empty catalog, home) satisfy their contract under the shipped evaluator, and 9 of 9 simulated failure modes are caught (reviews missing, un-hydrated islands, empty rails, missing price, imageless tiles, missing hero…), using the contracts' own selectors.
• assess() driven with the measured desktop/mobile counts against one URL-keyed row: 6 spurious shortfalls in 12 records, never converging. Against per-page rows: 0.

Deploy consequence

Stage 1.31.0 + 0.80.0, not 1.30.0 + 0.79.0. The fleet rollout is Recreate (all pods down before any come up), so shipping 1.30.0 and rolling again to fix the monitor before arming would be a wasted capacity-zero window.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request refactors the readiness contract tracking to be per-page (URL and device variant) rather than per-URL, preventing false shortfall alarms caused by layout differences between desktop and mobile. It also optimizes the DOM monitor's element counting to avoid double-counting nested nodes, fixes the quiet period calculation, and ensures resources are cleaned up properly on failure. A review comment correctly identifies a potential bug in the DOM monitor's insideAdded helper, where standard HTML elements with a native string host property (like anchor or area tags) could cause incorrect traversal, and suggests a robust check to ensure host is only accessed when it is an object.

[gemini-code-assist]

The insideAdded helper traverses up the parent chain using node.parentNode || node.host. However, standard HTML elements like <a> (HTMLAnchorElement) and <area> (HTMLAreaElement) also have a native host property which returns a string (the hostname). If one of these elements is the root of an added subtree (having no parent), node.parentNode will be null, causing the traversal to fall back to node.host. This evaluates to a string, causing the loop to query added.has(string) and potentially leading to unexpected behavior or incorrect quiet measurements. To prevent this, ensure that host is only accessed if it is an object (representing a ShadowRoot host).

Suggested change

	const insideAdded = (node, added) => {
	for (let p = node.parentNode || node.host; p; p = p.parentNode || p.host) if (added.has(p)) return true;
	return false;
	};
	const insideAdded = (node, added) => {
	for (let p = node.parentNode || (node.host && typeof node.host === 'object' ? node.host : null); p; p = p.parentNode || (p.host && typeof p.host === 'object' ? p.host : null)) if (added.has(p)) return true;
	return false;
	};

[harper-joseph]

Right — HTMLAnchorElement.host (and HTMLAreaElement's) is a string. As written it was benign (a string has no parentNode or host, so the walk ended on the next step) but it was walking through the wrong thing to get there. Fixed in e4dae55 the way the intent reads: follow host only from a ShadowRoot (nodeType === 11), via a small up() helper rather than the inline conditional. The parent-then-children test now also inserts an <a>-rooted subtree — a product tile is exactly that shape — and asserts it is counted once and agrees with recount().