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 2 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 2h 4m 15s ⏱️ + 5m 30s Results for commit a77a24a. ± Comparison against earlier commit 0a762f4. Realm Server Test Results 1 files ±0 190 suites ±0 1h 12m 47s ⏱️ + 1m 24s Results for commit a77a24a. ± Comparison against earlier commit 0a762f4. |
|
[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
| // The 500 is not free — the manager prunes a server that returns one | ||
| // from its registry — so this must fire only for a pool that really | ||
| // cannot reach the current shell, never for a render that was on the | ||
| // current pool and simply found a broken card. That is why both warmed | ||
| // samples describe the render being judged, and why the retry's start | ||
| // is sampled after its recycle rather than carried forward. |
There was a problem hiding this comment.
[Claude Code 🤖] Blocking — needs a decision. The prune is not one server. It is every server this visit can reach, and the condition that triggers the refusal is fleet-correlated.
In manager-app.ts's proxy loop, a 5xx runs pruneServer(target) and then attempts.delete(target) before if (attempts.size < registry.servers.size) continue — the pruned server leaves both sets, so the loop re-enters, picks the next server, and prunes that one too. One refusal therefore walks the whole registry, prunes every entry, and answers 503 No servers. The affinity assignments pruneServer deletes go with it, and requests arriving in the gap park in the discoveryWaitMs loop (10s default) waiting for heartbeats to refill it — that includes on-demand renders and screenshots, not just this index visit.
The trigger is not per-server either: every prerender server learns the same token from the same manager within a heartbeat, so "the pool has not reached the current shell" tends to be true of all of them at once. The cheapest path in needs no failed recycle at all — see the comment on retryWarmedAtStart above.
The cost compounds, because each hop is two renders: one visit is 2 × servers renders inside remote-prerenderer's per-attempt abort (resolvePrerenderManagerRequestTimeoutMs, 60s render + 60s overhead = 120s), and that AbortError is explicitly classified not retryable there. So the fan-out can turn the refusal into a hard, non-retried failure rather than the retry it is written to be.
Suggestion: give the refusal a disposition the manager passes through rather than one it punishes. The draining status (PRERENDER_SERVER_DRAINING_STATUS_CODE + PRERENDER_SERVER_STATUS_HEADER) already gets markDraining instead of pruneServer, self-heals on the next active heartbeat, and remote-prerenderer already treats it as retryable — though it still walks the fleet. A distinct status or header the manager hands straight back to the client, without retrying and without pruning, avoids both halves; no other server is likelier to be current, so retrying across the fleet buys nothing here.
Whichever way it goes, this comment needs to say what the 500 actually costs — "the manager prunes a server that returns one" is what makes the trade look bounded.
Generated by Claude Code
| throw new Error( | ||
| `prerender pool has not reached host shell ${shellAtCompletion}; ` + | ||
| `refusing to persist a module-resolution failure for ${url}`, | ||
| ); |
There was a problem hiding this comment.
[Claude Code 🤖] Needs an answer. Where does this refusal actually land? Traced end to end, it does not stop the row being written — it changes the message on it.
remote-prerenderer retries a 5xx up to PRERENDER_MANAGER_RETRY_ATTEMPTS (12) with 200ms × 2^n backoff capped at 180s — roughly 6.5 minutes of backoff — and then rethrows. In index-runner.ts, #renderVisit catches that into { status: 'error' }, #handleVisitError calls #bufferErrorEntriesFor, and that buffers a file-error row plus, when the URL is a card instance, an instance-error row carrying this exact message. So a pool that genuinely cannot reach the current shell persists an error doc for the card either way; what the refusal buys is time for the pool to catch up, not the absence of a poisoned row.
That may well be the trade you want, but the comment above and the PR description both say such a pool "fails visibly by refusing to index" rather than poisoning rows, and the terminal state is still a row. Either say so here — the guarantee is the retry budget, and it is ~6.5 minutes — or give the refusal something the indexer can recognize so #handleVisitError leaves the row untouched instead of substituting one error doc for another.
Non-blocking by itself; it becomes load-bearing together with the non-retryable abort described in the comment below, which can shorten that budget to a single attempt.
Generated by Claude Code
| await options.awaitHostShellRecycle?.(); | ||
| retryWarmedAtStart = options.getWarmedHostShellHash?.(); | ||
| return { result: await prerenderer.prerenderVisit(visitArgs) }; |
There was a problem hiding this comment.
[Claude Code 🤖] Blocking. Awaiting hostShellRecycle does not guarantee a recycle for the token the re-check compares against, so the refusal can fire with no wait and no failed recycle.
reconcileHostShell records reportedHostShellHash before its guards, then returns early while recyclingForHostChange is true. A token learned during an in-flight recycle therefore starts no recycle of its own until a later heartbeat. In that window hostShellRecycle is the earlier token's recycle: await here resolves (immediately, once it has settled), retryWarmedAtStart reads the earlier token, warmedAtCompletion reads it too, and the re-check below refuses. All it takes is two distinct tokens inside one recycle plus one heartbeat interval — no recycle failure, no second deploy landing "on top" mid-render.
Given what the 500 costs (see the comment on the refusal below), this wants a stronger precondition than "the recycle we happened to be holding has settled": either wait on a recycle whose target token equals the reported one, or let a heartbeat interval pass and re-sample before refusing. As it stands the comment's "after the recycle" reads as a guarantee the plumbing doesn't provide for this case.
Generated by Claude Code
| // Nothing reported by the end of the render: this server has never been told | ||
| // which shell is current, so it has no grounds in either direction. | ||
| if (shellAtCompletion === undefined) { | ||
| if (!hasMissingExportError(response)) { |
There was a problem hiding this comment.
[Claude Code 🤖] Pre-existing scope, newly load-bearing. hasMissingExportError also matches inside error.additionalErrors — the console errors RenderRunner merges onto a failure of an entirely different kind. Under the old rule that only ever cost one extra render. This predicate now decides the refusal too, so a render that failed on a timeout or a wedge, whose console happened to carry one missing-export line, gets a 500 on a stale pool along with everything a 500 costs.
Should the refusal require the missing export to be the failure (error.message), leaving additionalErrors to drive only the re-render? Not this PR's bug, but this PR is what makes it expensive.
Generated by Claude Code
| test('a module error under a changed shell is re-rendered', function (assert) { | ||
| assert.true( | ||
| shouldRerenderForShellChange({ | ||
| shouldRerenderForStaleShell({ | ||
| response: visitResponse(MISSING_EXPORT), | ||
| shellAtStart: 'babf3612', | ||
| shellAtCompletion: 'b778fe76', | ||
| ...poolBehind, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
[Claude Code 🤖] Non-blocking. This case and a pool that never caught up is stale even though nothing moved below are now byte-identical — same ...poolBehind, same visitResponse(MISSING_EXPORT), same assert.true — so one of the two pins nothing and both move together on any future change.
The name is stale as well: poolBehind holds reportedAtCompletion fixed, so nothing changes under this render. Suggest dropping this one and letting the named case below carry it.
Generated by Claude Code
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.