Drop the prerender app's persistence block on the command route - #6021
Conversation
`__boxelPrerenderApp` blocks persistence on every store in the prerender app. The render route raised it and nothing ever cleared it, so a pool tab that had served a card render kept the block for the rest of its life — including after `PagePool` retagged it from a realm affinity onto a user affinity for a command. Commands write, so such a command answered `status: "ready"` with a card that was never saved. Clear it in the render route's teardown so its lifetime tracks that route being entered (`beforeModel` raises it again ahead of every visit's model work), and clear it in the command route's `beforeModel` too: an in-app transition runs the departing route's exit hooks after the entering route's, so the command route cannot rely on the render route having torn down yet. Writes from a command stay deadlock-safe through `__boxelHeadlessCommand`, which indexes them deferred. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012JN9nUPZmHWKKL8KnwLmTs
`store.add()` returned the unpersisted instance when `renderContextBlocksPersistence()` was true. An instance with no id is indistinguishable from a saved one to every caller — `SaveCardTool` reports it as a success — so a blocked write surfaced as a wrong answer rather than a failure. Throw instead, unless the caller passed `doNotPersist` and asked for the in-memory instance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012JN9nUPZmHWKKL8KnwLmTs
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟢 Approval recommended
The changes consistently scope the persistence block to the correct routes, add a clear failure mode for mis-scoped persisting writes, and include focused tests covering the regression scenario.
Pull request overview
This PR fixes a prerender-only correctness issue where the global __boxelPrerenderApp persistence block could outlive the /render route that raised it, causing subsequent in-tab transitions (notably /command-runner) to silently drop writes and return “successful” results with no durable card id.
Changes:
- Bound
__boxelPrerenderAppto the render route lifetime by clearing it on render route deactivation and via the render globals destructor. - Cleared
__boxelPrerenderAppwhen entering the command-runner route to ensure commands can persist even when transitioned to from a render tab. - Made
StoreService.add()throw when persistence is blocked unless the caller explicitly opts intodoNotPersist, and added targeted integration + acceptance coverage.
File summaries
| File | Description |
|---|---|
| packages/host/app/routes/render.ts | Clears __boxelPrerenderApp on route exit/destruction; continues to raise it before model work in non-test prerender app. |
| packages/host/app/routes/command-runner.ts | Clears __boxelPrerenderApp on entry so command writes are not inadvertently suppressed during in-app transitions. |
| packages/host/app/services/store.ts | Throws on persisting add() calls under a blocked persistence context unless doNotPersist is set. |
| packages/host/tests/integration/store-test.gts | Verifies persisting add() rejects under the block while doNotPersist still returns the in-memory instance. |
| packages/host/tests/acceptance/prerender-persistence-block-test.gts | Ensures the block doesn’t outlive render route, and that a command after a render produces a durable saved card. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Preview deploymentsHost Test Results 1 files ±0 1 suites ±0 2h 23m 14s ⏱️ - 7m 40s Results for commit 1022402. ± Comparison against earlier commit 69e8570. Realm Server Test Results 1 files ±0 190 suites ±0 1h 8m 46s ⏱️ - 2m 47s Results for commit 1022402. ± Comparison against earlier commit 69e8570. |
…er-route teardown The render route's teardown clear went further than the defect needed. `deactivate()` fires when a transition leaves the `render` tree — Ember's `partitionRoutes` only exits a route when the route object itself changes, so a per-card dynamic-segment change never reaches it — and one of the routes it leaves for is `module`, on the same realm affinity within the same indexing job. `routes/module.ts` raises `__boxelRenderContext` but not `__boxelPrerenderApp`, and it renders through the render store, so clearing the flag on the way out leaves the regular `StoreService` unblocked for the whole module render while the tab still holds the worker. No write is reachable there today, but the invariant fails open instead of closed for the next route that forgets about it. `__boxelPrerenderApp` therefore keeps its app-lifetime scope, and the command route stays the single explicit carve-out. It drops rather than restores the flag: the render route raises it in `beforeModel`, which runs before the exit hooks of the route being left, so restoring a never-raised value would lower the block across the next render's model hook. Scope the `add()` error to the prerender app as well. `Store` reaches card authors through `@context.store`, and the render store is blocked during the interactive app's in-browser index renders too — where a card that writes while rendering has always had that write dropped rather than failing its render. Raise it before the instance is registered so a failed `add` leaves nothing resident. Pin the invariant the code now holds: entering the command route drops the block. Render against the card's `.json` URL, the shape the prerender driver uses, and guard the durability read on a non-empty id so a blocked save fails on its own assertion rather than on URL parsing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012JN9nUPZmHWKKL8KnwLmTs
habdelra
changed the title
…mand The prerenderer is not an avenue for mutations, but a card whose template or computed writes to the store is doing what it was designed to do. Such a write is dropped, as it always has been, and the card renders — erroring would fail the render and index the card as an error instead. The error is reserved for the state that is genuinely impossible: a headless command running while the prerender app's persistence block is still raised. The command route drops the block on entry so a command's writes can land, so the block being up there means the drop did not happen, and the save resolves to an instance with no id that every caller reads as a saved card. The render route clears `__boxelHeadlessCommand` on entry so a render can never be taken for a command. A transition runs the entering route's model hooks before the departing route's exit hooks, so a tab arriving from the command route would otherwise carry that flag through the render's whole model hook — which also kept a render's writes from being marked for deferred indexing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012JN9nUPZmHWKKL8KnwLmTs
backspace
left a comment
backspace
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Lens: whether the three flag sites (raise in render.ts, drop in command-runner.ts, report in store.ts) leave any tab state where a write is silently dropped or wrongly reported, and whether the scope argument for keeping the flag app-lifetime still holds once the command route drops it. Not run locally: the host suites, so the acceptance test's fail-without-fix property comes from reading its two branches rather than from a revert.
No blocking issues. Approving. Two non-blocking items:
-
The
add()comment block instore.tshas a leftover fragment line, and its "leaves nothing resident" justification is broader than what the placement delivers. See the inline thread. -
routes/module.tsnever raises__boxelPrerenderApp, and the cross-affinity steal inPagePool#selectEntryForAffinitypicks donors from any other affinity regardless of type, so a tab that has run a command (flag dropped) can serve the nextmodulerender with the interactiveStoreServiceunblocked. That is the fail-open shape the scope section rejects for the teardown option. Pre-existing (a fresh tab starting onmoduleis in the same state) and no write is reachable in a module render today, so non-blocking. Raising the flag inmodule.ts'sbeforeModelunder!isTesting(), asrender.tsdoes, would make the fail-closed claim hold. Fine as a follow-up.
Drop an orphaned sentence fragment above the check, and the claim that raising it early leaves nothing resident. Both input shapes register before this point: a doc carrying an id is set into the store by card-api's createFromSerialized, and an instance has its deps setCard'd on the way in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012JN9nUPZmHWKKL8KnwLmTs
__boxelPrerenderAppmarks the dedicated prerender app for its whole lifetime and blocks persistence on every store in it: a write from a render would aim at a realm whose sole indexing worker that render is occupying, and the write would then wait on a job that needs the held worker.Commands are the exception — they are expected to write — but the flag was never dropped for them.
PagePool#reassignAffinityTabcan retag a tab from the realm affinity a card render uses onto the user affinity a command uses (via the dormant-tab commandeer, or the cross-affinity steal when standby refill fails), andrender-runner.tsenters the command route with an in-apptransitionTorather than a document load. So a tab that had served a card render ran commands with all persistence disabled:store.add()returned the unpersisted instance,SaveCardTooltreated it as saved, and the command answeredstatus: "ready"with a result carrying no card id.Changes
app/routes/command-runner.ts— drops__boxelPrerenderAppinbeforeModel, making the command route the single explicit carve-out from the block. Writes from here stay deadlock-safe through__boxelHeadlessCommand, which marks them for deferred indexing.It drops rather than restores the flag on teardown. The render route raises it in its own
beforeModel, and a transition runs the entering route's model hooks before the exit hooks of the route being left — so restoring a never-raised value would lower the block for the whole of the next render's model hook.app/routes/render.ts— clears__boxelHeadlessCommandinbeforeModel, by the same ordering: a tab arriving from the command route would otherwise carry that flag through the render's whole model hook, marking a render's writes for deferred indexing and putting them on the command error path below.app/services/store.ts—add()reports one state instead of absorbing it: a headless command running while the block is still raised. The command route drops the block on entry so a command's writes can land, so the block being up there means the drop did not happen, and the save resolves to an instance with no id that every caller —boxel run-commandand the software-factory steps that wrap it included — reads as a saved card.create()already throws on the same state. Raised before the instance is registered, so a failedaddleaves nothing resident.A card render stays silent. The prerenderer is not an avenue for mutations, but a card whose template or computed writes to the store is doing what it was designed to do; that write is dropped as it always has been and the card renders. Erroring would fail the render and index the card as an error.
Scope
The flag keeps its app-lifetime scope. Clearing it in the render route's teardown was considered and rejected:
deactivate()fires when a transition leaves therendertree, and one of the routes it leaves for ismodule, on the same realm affinity within the same indexing job.routes/module.tsraises__boxelRenderContextbut not__boxelPrerenderApp, and renders through the render store, so clearing the flag on the way out would leave the regularStoreServiceunblocked for the whole module render while the tab still holds the worker. No write is reachable there today, but that shape fails open for the next route that doesn't consider the flag, whereas leaving the flag up fails closed.Left alone:
patch()reports a blocked write asundefined, which its callers already branch on;delete()has no block guard at all and issues the realmDELETEregardless. The second is worth its own change and is not touched here.Tests
tests/acceptance/prerender-persistence-block-test.gts— entering the command route drops the block; and a card-saving command run on a tab that has just served a card render produces a card that reads back 200 from the realm's own source.tests/integration/store-test.gts— a card render keeps its instance rather than failing, a headless command under the block reports it, and adoNotPersistcaller still gets the in-memory instance.Not included: a prerender-server test that forces a cross-affinity steal on a single-page pool.
PagePoolkeeps one warm standby above its active ceiling —#desiredStandbyCount()returns 1 onceactiveTabs >= maxPages, and#prepareSlotForStandby()admitsmaxPages + 1contexts — so a command on a fresh affinity takes that standby and gets a fresh tab; the steal is unreachable from a single-page pool without a new test seam inPrerenderer. The acceptance test covers the same app-level hand-off deterministically.Also uncovered: the raise side of the invariant.
render.tsonly raises__boxelPrerenderAppwhen!isTesting(), so no host test can observe it, and nothing fails if that line is removed.Verification
pnpm lint:js,pnpm lint:hbsandpnpm lint:typespass inpackages/host. The host suites need the full stack (Synapse, realm servers, host dist), which this environment cannot provision, so CI is what runs them.🤖 Generated with Claude Code
https://claude.ai/code/session_012JN9nUPZmHWKKL8KnwLmTs