Refuse a module failure the prerender pool cannot be shown to have earned - #6010
Refuse a module failure the prerender pool cannot be shown to have earned#6010backspace wants to merge 4 commits into
Conversation
…rned The indexer stores a failed render as the card's content, so a render that resolves current realm source against a bundle that predates an export turns a few minutes of deploy overlap into an error document served from cache to every anonymous reader until something reindexes the row. Deciding that on whether the reported token moved *during* the render leaves the shape that does the damage untouched. A pool can be behind the current shell for a whole render with nothing moving under it: the token moves, the recycle it triggers is still running — or has failed and is waiting for another heartbeat to retry — and every render in that window lands on the outgoing bundle and reads as steady. So the question is no longer whether the shell moved but whether the pool can be shown to have been on the current shell throughout, which is what `warmedHostShellHash` already records: it advances only once a recycle has actually re-warmed the pool, where the reported token moves the moment the change is learned. Requiring both samples of it to equal the reported token rules out three ways of being stale in one comparison — a pool that never caught up, a recycle that landed mid-render, and a token that moved under the render — and it is strictly broader than the condition it replaces, which the last of those three was all it could see. A failure that survives the one re-render on a pool that is *still* not current is no longer returned. It answers 500, which `remote-prerenderer` maps to a retryable error, because the alternative is persisting a failure this server has already decided says nothing about the card. That trade is deliberate: a pool that cannot reach the current shell now fails visibly by refusing to index, where before it failed quietly by poisoning rows. It is logged at `error` for that reason. Tests state both directions, since the difference between them is the whole point: a module failure on a pool demonstrably current is still returned for the caller to persist, and a pool that never reaches the current shell answers retryably with the failure nowhere in the response. Reverting the predicate to the old token-move rule fails four of them, including the stale-pool case it could not express.
Host Test Results 1 files ± 0 1 suites ±0 1h 36m 33s ⏱️ - 27m 42s Results for commit b0bf179. ± Comparison against earlier commit a77a24a. Realm Server Test Results 1 files ± 0 201 suites +11 1h 10m 31s ⏱️ - 2m 16s Results for commit b0bf179. ± Comparison against earlier commit a77a24a. |
|
[Claude Code 🤖] Two things a reviewer should weigh that aren't visible from the diff, plus what CI verified. 1. The behaviour change is a refusal, and it wants explicit agreement. A missing-export failure that survives the one re-render — on a pool that is still not demonstrably on the current shell — is no longer returned. It answers 500, which That is a deliberate trade rather than a side effect: a prerender pool that cannot reach the current shell now fails visibly, by refusing to index and logging at The risk to design against is the opposite one — swallowing a genuinely broken import — and that is why a module failure on a pool that is demonstrably current is still returned unchanged, with its own test. 2. The predicate is broader, not just renamed.
The old question misses the shape that does the damage: the token moves, the recycle it triggered is still running — or has failed and is waiting for another heartbeat to retry — and every render in that window lands on the outgoing bundle while reading as perfectly steady. The new one keys on Mutation-checked rather than asserted: restoring the old token-move rule fails four cases, including What CI confirmed. The covering tests run in realm-server shards 3 and 4, and I checked they actually executed rather than trusting the green tick: all 6 Not in scope, deliberately. The pool's warmed token is not stamped into |
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a762f4c1b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The re-check carried the pre-recycle sample forward as the retry's start token, so a recycle that succeeded between the two renders left the retry looking stale: warmed-before-recycle against the current reported token. A missing export the retry then found on a pool that was current throughout — a card that really is broken — was refused instead of returned. That is worse than the failure this path prevents. The manager prunes a server that answers 500, so a healthy prerender server would be dropped from the registry over a broken card, and the visit retried until the registry ran out of servers. The retry now samples the warmed token inside its own closure, after the recycle and immediately before the render it describes, so both samples belong to the render being judged. The tests had the same blind spot as the code: every case that reached the re-check either failed twice on a pool that was never current, or succeeded on the second attempt. None of them had a genuine failure survive a successful recycle, which is exactly the shape that misjudged. That case is now covered, and restoring the old assignment fails it.
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This review went after the refusal's downstream consequences — prerender-app → manager-app → remote-prerenderer → index-runner — the predicate's inputs at each call site, and whether the new tests discriminate. It does not cover the render path inside prerenderer / render-runner, and no realm-server tests were run here (the test-postgres step needs a Docker daemon this environment lacks), so nothing below rests on a local run.
The predicate change is sound and is the broader question it claims to be. The refusal's blast radius is not: a 500 prunes every prerender server the visit reaches, not one, and after the retry budget the indexer writes the error doc anyway — so both ends of the trade need explicit agreement, and the recycle the retry awaits may belong to a different token than the one it is judged against.
Recommendations:
- Decide the refusal's wire disposition — the 500 empties the manager's registry rather than sidelining one server, and the resulting fan-out can hit a non-retryable abort. Thread on the "The 500 is not free" comment in
prerender-app.ts. Blocking. - Strengthen the refusal's precondition —
awaitHostShellRecycle()can resolve on a recycle for an earlier token, in which case the refusal fires with no wait. Thread onretryWarmedAtStart. Blocking. - State the terminal outcome —
#handleVisitError→#bufferErrorEntriesForpersists afile-error/instance-errorrow carrying this refusal's message onceremote-prerendererexhausts its ~6.5 minutes of retries. Thread on thethrow. - Consider narrowing the refusal to a failure that is a missing export, leaving
additionalErrorsto drive only the re-render. Thread onhasMissingExportError's new early return. - Drop the duplicated predicate case in
prerender-host-shell-recycle-test.ts. Thread there. - Three route tests still reach the stale path through
undefinedrather than through a moved token.a module failure under a moved shell is re-rendered once, after the recycle,a rejecting re-render answers 500 so the visit is retried elsewhere, anda drain during the re-render answers drainingall passgetHostShellHash: shellThatMovesOnce()and nogetWarmedHostShellHash, so both warmed samples areundefinedand the gate fires whatever the reported token does — each would behave identically with a steady token, which makes theshellThatMovesOnceframing inert. Stating both tokens, as the two rewritten cases now do, would make them pin the trigger they name.
Adjacent, out of scope:
package-shim-handler.ts's comment aboveMISSING_EXPORT_MESSAGEstill describes the superseded rule — "A caller that can see the host shell moved under the render needs to recognize the message to treat it as retryable". That is the condition this PR argues is insufficient, and it is the far end of the contract this predicate implements.stampHostShellTokensstamps only the reported tokens, so the decision made here isn't reconstructible fromdiagnostics. Already called out as deliberate; noting it only because the refusal makes the warmed token the first field an operator would reach for.
Generated by Claude Code
Answering a stale-shell failure with a 5xx cost far more than it bought, in two ways that together remove the case for it. It does not prevent the row. `remote-prerenderer` retries a 5xx twelve times over roughly six and a half minutes and then rethrows, and `#handleVisitError` buffers a `file-error` row — plus an `instance-error` row for a card instance — carrying whatever message arrived. So the refusal substituted one error document for another, later, rather than avoiding one. And its blast radius is the whole fleet, not one server. The manager's proxy loop prunes a 5xx target and then removes it from its attempt set as well, so `attempts.size < registry.servers.size` stays true, the loop advances to the next server, and prunes that one too. Since every prerender server learns the same token from the same manager, "the pool has not reached the current shell" tends to be true of all of them at once — so one visit can empty the registry and leave the manager answering `503 No servers`, with affinity assignments gone and later arrivals parked waiting for heartbeats to refill it. Each hop is also two renders inside a per-attempt abort that is classified as *not* retryable, so the fan-out can convert the intended retry into a hard failure. Declining to persist a skew-shaped failure belongs where the row is written. Recovery here is one extra render, and the widened predicate is what makes that render fire in the case that matters: a pool behind the current shell for a whole render, with nothing moving under it, which the previous token-move rule could not see. Removing the refusal also settles two questions it had raised. Whether `awaitHostShellRecycle` awaits the right token no longer decides anything heavier than one wasted render — worth tightening, but not here. Nor does it matter that `hasMissingExportError` matches inside `additionalErrors`: that breadth costs an extra render again, as it did before, rather than a 5xx. Tests follow the same line. The refusal's own case goes; its companion keeps its value and says what it now guards. Two predicate cases had become byte-identical, so one is dropped. And three route tests named for a moved shell left both warmed samples `undefined`, which fires the gate whatever the reported token does — they would have passed identically with a steady token. They now state the pool's token too, and fail when the reported one is held still.
|
[Claude Code 🤖] Both blocking items were right, and together with the terminal-outcome finding they removed the case for the refusal rather than changing its shape. Why removal rather than a better disposition. Two verified facts, either of which alone might have been survivable:
So the trade was a fleet-wide prune in exchange for delaying one row. No choice of status code fixes that, which is why I didn't take the draining-status suggestion: it avoids the prune but misreports a healthy server as draining, still walks the fleet, and leaves the row written either way. What survives. The predicate change, which was the half that stood on its own: recovery is one extra render, as before, but it now fires for a pool that is behind for a whole render with nothing moving under it — the case the old token-move rule could not see. What this means for CS-12762. Its ask — don't persist a failure that came from a stale bundle — needs the indexer, not the prerender server. This layer can only delay a write, never decline one. I've noted on the threads that two things you found become load-bearing again there: the recycle/token mismatch (a decision that heavy needs a recycle keyed to the reported token, or a re-sample after a heartbeat), and the Tests. The refusal's case is gone; its companion kept and retitled to what it now guards. Duplicate predicate case dropped. And your item 6 was the useful one — those three route tests left both warmed samples Adjacent, taken: the |
Every failure on the previous run was GitHub's artifact service answering 403 to both uploads and downloads — `Failed to FinalizeArtifact` on the jobs that had finished testing, `Failed to ListArtifacts` on the ones that could not fetch the test web assets to start. Main was failing the same way at the same time, and the realm-server suite this branch touches reported `# fail 0` before its upload was refused. An empty commit rather than a re-run of the failed jobs: the uploads that failed never finalized, so the jobs that consume those artifacts would have nothing to download on a partial re-run.
2 participants
This is a step in mitigating production hosted site downtime from the base realm importing new things from
boxel-ui.Claude: The indexer stores a failed render as the card's content, so a render that resolves current realm source against a bundle that predates an export turns a few minutes of deploy overlap into an error document served from cache to every anonymous reader until something reindexes the row.
Deciding that on whether the reported token moved during the render leaves the shape that does the damage untouched. A pool can be behind the current shell for a whole render with nothing moving under it: the token moves, the recycle it triggers is still running — or has failed and is waiting for another heartbeat to retry — and every render in that window lands on the outgoing bundle and reads as steady.
So the question is no longer whether the shell moved but whether the pool can be shown to have been on the current shell throughout, which is what
warmedHostShellHashalready records: it advances only once a recycle has actually re-warmed the pool, where the reported token moves the moment the change is learned. Requiring both samples of it to equal the reported token rules out three ways of being stale in one comparison — a pool that never caught up, a recycle that landed mid-render, and a token that moved under the render — and it is strictly broader than the condition it replaces, which the last of those three was all it could see.A failure that survives the one re-render on a pool that is still not current is no longer returned. It answers 500, which
remote-prerenderermaps to a retryable error, because the alternative is persisting a failure this server has already decided says nothing about the card. That trade is deliberate: a pool that cannot reach the current shell now fails visibly by refusing to index, where before it failed quietly by poisoning rows. It is logged aterrorfor that reason.Tests state both directions, since the difference between them is the whole point: a module failure on a pool demonstrably current is still returned for the caller to persist, and a pool that never reaches the current shell answers retryably with the failure nowhere in the response. Reverting the predicate to the old token-move rule fails four of them, including the stale-pool case it could not express.