feat: make SSR action seeding observable and assert determinism in dev - #1311
feat: make SSR action seeding observable and assert determinism in dev#1311vivek7405 wants to merge 18 commits into
Conversation
A seed miss is indistinguishable from a hit from the outside, so a refactor that breaks seeding for a whole app produces no error, no warning, and no log line. The app just quietly re-issues one RPC per async component on every first load, which is exactly what the feature exists to remove. Dev now reports it. `X-Webjs-Seed` carries `off` / `html-cache` / `collected=<m>, emitted=<n>` / `... streamed`, folded into the existing access log line as a `seed` field. The browser logs one warning per page view when a hydration action call missed, naming which of the three causes applies. That gate comes from a server-stamped `data-webjs-dev` marker, never from `process.env.NODE_ENV`, which esbuild folds to a constant in the built core bundle and `publicEnvShim` then inverts. `ingest` also flips to last-write-wins. First-write-wins protected a case that cannot occur (a hit deletes its key), and its real effect was to hand a component a value from a render no longer on screen after a soft nav. Refs #1309
Unit coverage at both ends: the server's three header shapes plus prod silence, and the client's marker-gated report with its defect-only rule. Each carries the counterfactual the issue named: comparing on the full key rather than hash/fn, prod byte-identity for buildSeedScript, the prod-silence case for the client gate, and fault injection proving a throwing console.warn still records the seed.
The node unit file drives a hand-rolled fake DOM, which cannot stand in for the contract the client depends on: that the selector matches the block the server emits, that removal detaches, and that the real requestIdleCallback fires the report once per scan batch.
The headline criterion is a browser one: a developer whose seeding broke must see it without opening the network tab. The fixture's miss page reaches its miss honestly, through a connectedCallback that SSR never runs, so the id the client asks for is one the server render never used.
The skill documented seeding nowhere, which matters most: an agent building an app is the reader who has to recognise a broken seed from a console line. The new reference section is written for that reader, with the three causes and the fix for each, and the determinism rule stated as an authoring constraint rather than an aside.
|
Design rationale: why the client dev gate is a server-stamped marker, not This was the sharpest constraint in the whole change and the answer is not the obvious one, so it is worth writing down before someone "simplifies" it back.
So a Consequence worth noting: dev and prod HTML now differ by that empty block, so Why the counters are not gated at all. WebJs is no-build on the server, so there is no dead-code elimination there, and the one build that does exist cannot express a dev gate for the reason above. Rather than pretend, the cost is stated: four integer increments on a path that already awaits |
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
The header, the access-log field, and the determinism assertion all hold up, and the data-webjs-dev marker is the right answer to the no-build dev-gate problem. The part I would keep an eye on is the client reporter, because its whole value rests on never crying wolf, and two things there let it do exactly that.
The measurement window is the bigger one. The design argues, correctly, that a miss after hydration is CORRECT behaviour and must not be reported. The code says so in a comment and the docs say so in prose, but the counters do not: the snapshot is taken when a report ENDS rather than when its window STARTS, so every legitimate post-hydration refetch is banked and charged to the next page you navigate to. Use an app normally, click a link, get a warning about page B for page A's refetches. That is precisely the channel-poisoning the defect-only rule exists to prevent, and it turns the feature against itself.
The second is narrower but has the same shape: the marker is sticky and the schedule gate reads it rather than asking whether THIS scan found one, so a back/forward restore (whose snapshot carries no block, because the first scan already removed it) inherits the previous page's marker and reports a cause that is not just unhelpful but actively wrong advice.
Also two wiring gaps: the access-log field list is enumerated in two places that were not updated, and the new e2e is not in ci.yml or test:e2e, so the assertion the PR calls its headline criterion never runs again after merge.
… last report The window is snapshotted when the report is SCHEDULED rather than when it runs, so a post-hydration miss (correct behaviour, since the seed is consume-once) no longer banks up and gets charged to the next page on the next soft navigation. Scheduling is also gated on THIS scan having found a marker rather than on one ever having been seen, so a back/forward restore, whose snapshot carries no seed block, cannot report a cause read off the previous page. An epoch token retires a callback whose state was reset under it. Wires the dev-seed e2e into ci.yml and test:e2e, and brings the two surfaces enumerating the access-log fields in line with the new seed field.
…ndow Two ways the report could name a cause its own numbers contradict. The merged count read stats.ingested, which counts only NEW keys, so a scan whose seeds all REPLACE unconsumed ones measured as zero. Revisiting a page whose seeding component elided is exactly that shape, and it printed "1 seed(s) on this page ... the page carried no seeds at all". The marker was still a module global, so a second scan landing inside a pending window overwrote it without scheduling anything, and the report named a cause read off a page the window never measured. It is part of the window snapshot now, and the global is gone.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Delta pass over 08c2b073. The window fix itself holds, but moving the snapshot to schedule time exposed two more places where the report can name a cause its own numbers contradict, and both are worth the round.
The first is a counting error I introduced with last-write-wins and then depended on: the merged count reads stats.ingested, which only counts NEW keys, so a scan whose seeds all REPLACE unconsumed ones measures as zero. That is not a corner case, it is precisely the shape last-write-wins was added for, and the line it produces contradicts itself inside one sentence.
The second is the same wrong-cause class the last commit fixed for the back/forward path, left in place for the marker: the counters became part of the window but the marker stayed a module global, so a scan landing inside a pending window overwrites it without scheduling anything.
Both are fixed and the module global is gone, which kills the class rather than the two instances.
The last two commits fixed this defect class one field at a time: the counters, then the marker. The mechanism was the model itself. A scan landing inside a pending window was dropped, so it got no report of its own, and its seeds and calls still landed in the earlier window's numbers. A healthy page collected the next page's correct misses and printed a defect line; a page with no seeds inherited the next page's and lost the branch that says so. Each scan now closes the previous window and opens its own. The close runs BEFORE the ingest loops, since the moment this page's content arrives is where the previous page's numbers stop. A markerless scan still closes, because the navigation happened either way.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Delta pass over 0647a4ba, and it caught the thing I should have caught myself: I had been fixing this defect class one FIELD at a time. First the counters moved into the window, then the marker, and the finding is that the counts half was still open through the same mechanism the marker half had just been closed through. Three rounds on the same family is the signal that the model was wrong rather than the fields.
It was. A scan landing inside a pending window was dropped entirely, so it got no report of its own, AND its seeds and calls still landed in the earlier window's numbers. Both directions are real: a page carrying no seeds inherits the next page's and loses the branch that exists to say so, and a HEALTHY page collects the next page's correct, expected misses and prints a defect line while the page that actually streamed gets no report at all.
Fixed by changing the model rather than patching another field: each scan closes the previous window and opens its own, and the close runs BEFORE the ingest loops, because the moment this page's content arrives is exactly where the previous page's numbers stop. Closing after the ingest was my first attempt and the new test caught it, which is a reasonable sign the test is measuring the right thing.
…ng one The last-write-wins flip opened a real stale-data hole, reproduced in a browser: a hit returning a value from a page no longer on screen, which is the exact hole this PR exists to close. The initial scan is lazy, so a page whose async components all elided never triggers it, and its seed block sits after the body content outside every boundary range, so no swap removes it. On the next soft navigation applySwap ingests the incoming page's seeds from a detached parse, then the incoming component's first takeSeed finally fires the lazy scan over the LIVE document and ingests the outgoing page's block on top. For a key both renders share, last-write-wins then hands the component the older value. First-write-wins happened to mask this. Draining the live document first, in ingest order, is what makes last-write-wins correct: outgoing values go in before incoming ones, so a shared key ends up holding the render whose paint is on screen. Keeping them rather than discarding them still answers an in-flight render from the outgoing page.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Delta pass over 91d6dc5c, and this one found a real correctness bug that the last-write-wins flip introduced, not a diagnostics problem. I reproduced it in Chromium, Firefox and WebKit before touching anything: a hit returning a value from a page that is no longer on screen, which is the exact hole this PR exists to close.
The chain is four things that are each individually fine. The initial scan is lazy, so a page whose async components all elided never triggers it. That page's seed block sits after the body content, outside every boundary range, so no swap removes it and it stays in the live DOM. applySwap scans a DETACHED parse, so it never marks the live document as scanned. Then the incoming component's first takeSeed finally runs the lazy scan over the LIVE document and ingests the outgoing page's block on top of the incoming one, and for a key both renders share, last-write-wins hands the component the older value. First-write-wins masked this, which is why it surfaced only now.
The fix is ordering rather than a new rule: drain the live document BEFORE ingesting the incoming page, so outgoing values go in first and a shared key ends up holding the render whose paint is on screen. Keeping them rather than discarding them still answers an in-flight async render() from the page being navigated away from, and costs nothing. The repro is now a browser test and goes red when the drain is removed.
Also corrected the comment whose back/forward premise was wrong in exactly the case that produces a leftover block, and a test comment describing the early-return the previous commit deleted.
…ains a live subtree Three gaps the last commit left. A <webjs-frame> self-load routes its subtree through applySwap, and ssr.js returns that subtree before the seed block is appended, so the parse carries no marker; suppressing the lazy scan then left the whole page with no report at all, which is the silent failure this feature exists to remove. The window now falls back to the drained live page's own marker and merged count. The drain is also scoped to a DETACHED root. scanSeeds is a public export, and a live subtree passed to it would have been stripped wholesale before it could be read. Also corrects two comments that contradicted the code, one of which an earlier edit had silently failed to apply, and brings the core module-map row up to date with the ordering the last-write-wins argument rests on.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Delta pass over abd2aab6. Five findings, all real, and the first is the one that matters: suppressing the lazy scan on ANY explicit-root scan meant a <webjs-frame> self-load, whose subtree ssr.js returns BEFORE the seed block is appended, opened no window and left the whole page it sits on with no report at all. Seeds kept working, only the diagnostic died, which is exactly the silent failure this PR exists to remove. The window now falls back to the drained live page's own marker.
Second: scanSeeds is a public export and the drain would have stripped a LIVE subtree passed to it before the caller could read it. Scoped to a detached root now.
Then two comments contradicting the code, and the core module-map row left stale across three commits.
The coverage point was the useful one, and chasing it turned up something worse than the gap itself. The e2e drives a real client-router soft navigation now, off a fixture page whose async component genuinely elides, with a sentinel proving the navigation was soft rather than a document load. Writing that test is how I found that the e2e exercises the BUILT core bundle rather than src, so the first version of it was not discriminating at all: it passed with the drain removed. Rebuilt, it goes red on that revert as it should. CI was never affected, since dist is gitignored and built fresh there.
|
Context: the e2e exercises the BUILT core bundle, not Worth writing down because it cost me a false-green and it is not obvious from the test file.
The consequence is sharp for a counterfactual. The first version of the soft-navigation test passed with the drain deliberately removed, which reads as "the test does not discriminate" and would normally mean rewriting the test. The test was fine. It was measuring a bundle built several commits earlier. Rebuilt, the same revert produces one leftover CI is not affected: |
…ds are dropped A <webjs-frame> swap goes through applySwap like a page navigation, and a frame response cannot be told apart from a page that seeded nothing: ssr.js returns the subtree before the seed block is appended, so it arrives with no block, no marker, and no X-Webjs-Seed header. Both guesses failed in turn. Treating it as a page killed the report for the whole surrounding page; attributing the window to the live page instead made it print one of three causes, none of which describes a frame response, so a confident misdiagnosis replaced silence. The router now says which it is, and a frame swap leaves every bit of page state alone. The outgoing page's leftover block is now stripped WITHOUT being ingested. Ingesting it ordered the values correctly but kept them forever: the block went unconsumed because its components elided, so nothing will ever call takeSeed for those keys and only a hit deletes one. That grew the store by a page payload per navigation, in production, and padded the unconsumed figure in the dev line with keys from pages already left.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Delta pass over 7818d948, and it overturns the fix in that commit rather than refining it, which is the right call.
I had made a markerless swap fall back to the live page's marker so a frame swap would not kill the report. That trades silence for a CONFIDENT MISDIAGNOSIS: the reporter can only name three causes, none of them is "a frame response carries no seed block", and the frame response has no X-Webjs-Seed header to cross-check against either, so the developer is told to go check a 'use server' directive that is already correct. Worse than saying nothing, and squarely against the point of the feature.
The root problem is that a frame response is genuinely indistinguishable from a page that seeded nothing, because ssr.js returns the subtree before the seed block is appended. Both of my guesses failed, in opposite directions, so the router now says which it is and a frame swap leaves every bit of page state alone.
The unbounded-growth finding is also right and is mine: the outgoing block went unconsumed BECAUSE its components elided, so ingesting it kept keys nothing would ever consume, in production, growing by a page payload per navigation. Stripping without ingesting fixes it and simplifies the correctness argument, at the cost of an in-flight render from the outgoing page missing, which is a round-trip rather than wrong data.
This is the fifth delta round, which is the cap. These fixes are on the branch UNREVIEWED, so the PR stays a draft and I am not calling it ready.
…ly names a cause it can prove Five findings from a whole-diff read, and the first reopens the hole this PR exists to close, by a second route. Stripping the outgoing block from the DOM was not enough. A page carrying an elided component alongside a shipping one has its WHOLE block ingested by the lazy scan the shipping one triggers, so the elided component's keys sit in the store with no carrier left to strip and nothing that will ever consume them. Navigate to a page that emits no seed for such a key and a component calling the same action with the same arguments gets the departed page's value over the fresh paint. A navigation now evicts the store too. The browser also warned as a defect for correct code. Every action call routes through the seed lookup, including ones never SSR-invoked and never seedable (a mutation, a Task autorun, a connectedCallback read), so on a page that emitted no seeds a miss is not evidence of anything. It now reports only the causes it can prove and leaves that case to the server header, where collected=0 is unambiguous. A serializer drop, the one failure the counts exist to expose, emitted no dev marker, so the browser said nothing at all; it now names it. Also declares the scanSeeds options argument in the .d.ts overlay, and corrects five doc surfaces that stated the guarantee more strongly than the code provided.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Final read over the whole diff, and it found the stale-seed hole still OPEN by a second route, which is the finding that matters most on this PR.
Stripping the outgoing block from the DOM only covers a page nothing scanned. A page carrying an elided component ALONGSIDE a shipping one has its whole block ingested by the lazy scan the shipping one triggers, so the elided component's keys are in the store with no carrier left to strip and nothing that will ever consume them. Navigate to a page that emits no seed for such a key and a component calling the same action with the same arguments gets the departed page's value over the fresh paint. I reproduced it before fixing it. A navigation now evicts the store as well as the DOM, and four doc surfaces that stated the guarantee more strongly than the code provided are corrected rather than left to be technically-almost-true.
The second one I would call the most valuable, because it was invisible to every test: the browser warned as a DEFECT for correct code. Every action call routes through the seed lookup, including ones never SSR-invoked and never seedable (a mutation, a Task autorun, a connectedCallback read), so on a page that emitted no seeds a miss is not evidence of anything. It told such a developer to check a 'use server' directive that is already right, on every page view, with no way to silence it. That is precisely the false alarm the defect-only rule exists to prevent, so the client now reports only causes it can prove and leaves the ambiguous case to the server header, where collected=0 is unambiguous.
Also: a serializer drop, the one failure the counts exist to expose, emitted no dev marker and so printed nothing at all; the .d.ts had not caught up with the scanSeeds options argument; and the blog line claiming no stale path could ever be served is corrected.
All five fixed here, none filed.
…st everywhere The drop marker added in the previous commit is a truthy string, and the header inferred emitted from exactly that, so in dev a serializer drop reported collected=N, emitted=N. Zero seeds shipped and the header claimed all of them did, on the one failure these counts exist to expose. It also falsified the client's own warning text, which tells the developer the response reports collected above emitted. The drop block is a named export now and the emitter excludes it, with a header-level regression test; the gap that let this through was that the drop was only ever tested through buildSeedScript in isolation, never through the response. The rest is sync. Narrowing the client to provable causes changed the contract in five prose surfaces that still listed the old three causes and still said silence means every call hit, and two comments plus a test comment still argued from paths the store eviction forecloses.
…s tests A frame request only gets a sliced subtree when the id was found and the render did not stream. Otherwise ssr.js falls through and serves the WHOLE page, seed block included, and the router dispatches webjs:frame-missing and throws that response away. The carve-out ingested it, so a discarded response's seeds entered the store, where last-write-wins hands them to a component on the page still on screen. It discards now, which is right for both shapes: an isolable frame carries nothing to begin with. The drop cause, which the previous commit made load-bearing, had no test at any layer; it now has a unit and a browser one. The header test's payload assertion was vacuous (it checked the whole document for a string that fixture never renders) and now asserts the block's own body is empty. Also the sixth prose surface: server-actions still carried the pre-narrowing contract.
Every claim about the frame path was written for the isolable case and then reused for the fallthrough, where it does not hold. ssr.js slices a bare subtree only when the id was found and the render did not stream, so a frame response does not reliably arrive with no block: a streamed render and a #241 cache hit both swap SUCCESSFULLY off a full page, and the cached one carries real seeds. Only a missing id discards the response. Discarding is still right for all of them, but for the reason that the block describes the whole page rather than the region being swapped in, not because there is nothing there. The comment, the module-map row, and an e2e comment still arguing from the replaced ingest-ordering rationale now say that.
The narrowing was half done. Dropping the no-seeds branch removed the false alarm only where the page seeded nothing; the surviving branch warned on ANY miss once the page had seeded something, and the argument for why a miss proves nothing never depended on that. A page with one seeded async component plus any mutation, Task autorun or connectedCallback read hydrating alongside it warned on every view and told a developer whose keys are fine to go audit their arguments. A miss is provable in exactly one shape: the page seeded that same action under DIFFERENT arguments. Then the action is demonstrably reachable and seeded, and the call still asked for a key the page does not carry. Track the hash/fn set the page seeded and report only that subset, plus the two the server asserts outright (streamed, drop). A miss on an action never seeded at all is left to the header, where collected states it plainly.
applySwap scanned as its first statement, but it can still throw the response away much later: a background revalidation with no trustworthy boundary plan returns 'discard' and the caller drops it. By then the scan had cleared the visible page's own unconsumed seeds and ingested a render that is never painted, keyed for the same actions and args that page uses, since it is a revalidation of the same URL. The next async render() on the page still on screen then hit on data disagreeing with the HTML. The scan is a closure called at each of the three commit points (frame swap, boundary swap, in-place full-body swap) and at neither discard branch. It still precedes every graft, which is the ordering the seeds need to be available on upgrade. Also declares keyMisses in the .d.ts, the JSDoc and the skill reference, corrects openWindow's annotation, and stops the warning line from stating the total while reporting the provable subset.
Changing the numerator to the provable subset without changing the predicate made the line understate real traffic: a page mixing one key mismatch with one mutation sent two calls to the network and reported one. A server-asserted cause explains every miss, so it keeps the total and the round-trip claim; a key mismatch now reports its own count against its own claim and never speaks for the misses it cannot account for. Also corrects a comment claiming a frame response carries no X-Webjs-Seed (true only for the isolable case, since the three fallthrough shapes carry the surrounding page's header) and brings the core module-map row up to the narrowed warning contract and the six-field seedStats.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Clean pass over the whole diff. Nothing found.
That is the first round in this cycle to come back with nothing, and it is worth saying what the cycle actually cost, because the shape of it is the useful record. Thirteen reviews, and the single defect class behind most of them was one sentence: a hit cannot show the user something different from the HTML they are already looking at. Four separate mechanisms reached that same failure, and each was only visible once the previous one was closed.
- The outgoing page's seed block still sitting in the DOM, ingested LAST by the lazy scan.
- What that page had ALREADY ingested, which stripping the DOM carrier does not touch.
- A frame request that fell through to a full-page response, whose block the frame carve-out was ingesting.
- A background revalidation discarded AFTER
applySwaphad already scanned it.
Every one of those was opened or exposed by the last-write-wins flip, which is a one-line change. That is the honest summary of this PR: the observability half was straightforward, and the correctness half took the rest.
The other recurring theme was the diagnostic accusing correct code. It warned on a page that seeded nothing, then on any miss once a page seeded something, and both were wrong for the same reason: every action call routes through the seed lookup, including mutations and client-only reads that could never have been seeded. It now reports only what it can prove, which is a miss on an action the page DID seed under other arguments, plus the two the server asserts outright.
All findings across the cycle were fixed on this branch. None was deferred, rejected, or filed elsewhere.
1 participant
Closes #1309
Summary
SSR action seeding (#472) is fail-open by construction, so its real production risk is not wrong data, it is a silent performance regression. A miss is indistinguishable from a hit from the outside, so a refactor that breaks seeding for an entire app produces no error, no warning, and no log line. The app just quietly re-issues one RPC per async component on every first load, which is exactly what the feature exists to remove.
This makes that observable in dev, and closes the one shape where a hit could disagree with the paint.
What changed
X-Webjs-Seed, dev only.offwhen seeding is disabled,html-cacheon a Add a server HTML response cache with TTL and on-demand revalidation #241 cache hit,collected=<m>, emitted=<n>on a buffered render,collected=<m>, emitted=0, streamedon a streamed one.offstays distinct fromcollected=0so a seeding-disabled app never looks like a seeding-broken one, andemitteddiverges fromcollectedexactly when the serializer threw and dropped the whole block, which was previously invisible.seedfield. No new log line.data-webjs-devmarker, neverprocess.env.NODE_ENV. esbuild folds that comparison to a constant in the built core bundle andpublicEnvShimdefineswindow.process.envon every page, so a NODE_ENV guard there is not merely dropped, it silently inverts. Rationale in a comment on this PR.ingestflips to last-write-wins. First-write-wins protected a case that cannot occur (a hit deletes its key); what it actually did was prefer an unconsumed seed from a render that is no longer on screen, reachable on a soft nav, a background revalidation, and a back/forward restore.recordSeed, compared on the fullhash/fn/argsKeyso a legitimate second call with different args cannot false-fire, deduped per action function, and in its own try/catch so a diagnostic failure can never drop a seed.seedStats()exported from@webjsdev/coreand declared in the.d.tsoverlay.Deliberately excluded, per the issue: no
webjs doctorcheck (nothing static to report), no newwebjs.*config key, no per-elementdata-webjs-seedemission or seeding of streamed regions, and no change to the key format, the wire, the facade, or theseeddefault.Test plan
packages/server/test/seed/*green, including the newseed-observability.test.jspackages/core/test/seed/action-seed-client.test.jsgreenpackages/core/test/seed/browser/action-seed-client.test.js, 6 tests green on Chromium, Firefox, and WebKit; full suite 0 failures on all threetest/e2e/dev-seed-observability.test.mjs(3 tests) plus the existinge2e.test.mjsnode scripts/run-bun-tests.js: 293 pass, 27 documented node-only skips, 0 genuine failures.test/bun/seed.mjsgreen under bothnodeandbunwebjs checkandwebjs doctorclean onwebsiteandexamples/blog/,/docs/{configuration,data-fetching,server-actions},/ui,/ui/button; blog boots 200 on/; no broken modulepreloads; noX-Webjs-Seedheader and nodata-webjs-devmarker on any prod responseCounterfactuals run (each toggled at
b8923e11, confirmed red, then restored):ingestback to first-write-wins reds both last-write-wins client tests.hash/fninstead of the full key reds the different-args test.opts.devguard on the header reds the prod-leak test.Docs
.agents/skills/webjs/references/data-and-actions.mdAGENTS.mdpackages/server/AGENTS.md,packages/core/AGENTS.mdwebsite/app/docs/configuration/page.tswebjs.seed/WEBJS_SEED(absent from the page before) and the dev diagnosticswebsite/app/docs/data-fetching/page.ts,website/app/docs/server-actions/page.tsseedStatsis exempted ingallery-coverage.jsonas a dev diagnostic rather than something an app writes code againstblog/ssr-action-seeding-no-refetch.md