Give host shells an ordering, assigned on first observation - #6023
Open
backspace wants to merge 5 commits into
Open
Give host shells an ordering, assigned on first observation#6023backspace wants to merge 5 commits into
backspace wants to merge 5 commits into
Conversation
A realm server identifies the host bundle it serves by hashing the index HTML it fetches over HTTP, which is all it can observe about a separately deployed artifact. A hash answers "is this the same shell?" and never "is this shell older?" — and the second question is the one that repairing deploy-skewed rows depends on, so today there is no query for "the rows this deploy left behind". `host_shell_generation` supplies the ordering: one row, whose generation advances each time a realm server observes a shell hash different from the one recorded. Servers observing the same shell read back the same number. The number is assigned here rather than by the deploy pipeline because the host has nowhere to carry one. It is static files published to S3 by `ember deploy`, with no environment to inject and no task definition to stamp, which is exactly why the existing token is a hash of what the realm server fetches. Advancing on a transition rather than per distinct hash is what makes a rollback behave. Redeploying a bundle that ran before takes a new, higher generation, because a row's generation records when it was rendered rather than which artifact is semantically newer — and a row from the bundle being rolled away from has to remain findable, which it would not be if the rollback reused that bundle's earlier number. The claim is a single UPDATE for a reason a read-then-write cannot satisfy. A rolling deploy overlaps a task booting against the outgoing bundle with its neighbour on the new one, so two *different* shells are claimed at once. Two readers see the same starting generation, compute the same successor, and two distinct shells end up sharing one number — which destroys the ordering, because rows from either then carry the same generation and nothing can tell them apart. One UPDATE cannot collapse that way: the second writer blocks on the row, and READ COMMITTED re-evaluates its predicate against the committed transition, so it counts its own on top. That is asserted against a real lock wait rather than a hopeful `Promise.all` — the commit waits until `pg_stat_activity` reports a backend blocked on the row, so the overlap exists rather than being hoped for. Substituting a read-then-write fails that test and only that test; a first draft that raced eight same-shell claims passed against both implementations, since claimants of one shell compute the same successor either way. Nothing reads the generation yet. Threading it onto index rows, and the repair query it exists to serve, follow separately.
Contributor
Preview deploymentsHost Test Results 1 files ± 0 1 suites ±0 2h 30m 25s ⏱️ + 33m 16s Results for commit 8b2637d. ± Comparison against earlier commit c0cebe9. Realm Server Test Results 1 files ±0 204 suites ±0 1h 12m 26s ⏱️ +23s Results for commit 8b2637d. ± Comparison against earlier commit c0cebe9. |
The host derives its in-browser SQLite schema from a dump of the migrated Postgres database, and refuses to build when the newest migration's timestamp does not match the schema file's. Adding a migration therefore requires regenerating that file, which is what three jobs were failing on — the two Matrix report merges among them, cascading from an asset build that never produced reports. `host_shell_generation` joins the tables the dump excludes. The exclusions are realm-server operational state the browser never reads — jobs, queues, users, session rooms — and which host bundle a server is serving belongs with them, not with the index tables the host queries. Excluding it also avoids a trap. The dump is schema-only, so an included table would arrive in the browser without the row the migration seeds, and a claim against it would find nothing to update and silently never advance. Absent, the table cannot be reached by mistake. The regenerated file is byte-identical to the one it replaces; only the timestamp moved, which is all the check reads.
One conflict, in `tests/index.ts`. Main replaced the hand-maintained `ALL_TEST_FILES` array with a walk over every `*-test.ts` under the directory — the same walk that assigns files to CI shards, so a file the sharder assigns is by construction a file the runner parses. Taking main's side wholesale is the whole resolution: the list entry this branch added for `host-shell-generation-test` was the only thing it contributed to that file, and discovery now supplies it. Confirmed rather than assumed — `discoverTestFiles()` returns 198 files and both of this branch's new suites are among them.
…tonic-deploy-generation-on-index-rows-and-repair
backspace
marked this pull request as ready for review
September 9, 2026 15:57
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: c0cebe9f35
ℹ️ 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 idempotent path answered from a second query, and that reopened the gap the first statement exists to close. A same-shell claim's `UPDATE` matched no rows, committed, released the row lock, and only then read the row back — so a different-shell claim landing in between made this return *that* shell's hash and generation. A caller stamping its own render with the number would have attributed it to a bundle it never ran on, defeating the ordering during exactly the rolling-deploy concurrency this helper is for. One statement now, with `WHERE id = 1` unconditional so it always takes the lock and always returns the row it observed, and a `CASE` deciding from the committed value whether the claim is a transition to count or the shell already recorded. A same-shell claim writes its own values back — a no-op — and reads the generation out of the same locked statement. Two tests. One states the contract directly: the answer describes the shell that was asked about. The other drives the gap rather than hoping to meet it — a querier that lets a competing shell claim the row the moment this claim's statement finishes, which is the interleaving a read-back would have been exposed to. Restoring the two-statement shape fails it with `bbbbbbbb` where `aaaaaaaa` was claimed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is another step in the production hosted site downtime mitigation.
Claude: A realm server identifies the host bundle it serves by hashing the index HTML it fetches over HTTP, which is all it can observe about a separately deployed artifact. A hash answers "is this the same shell?" and never "is this shell older?" — and the second question is the one that repairing deploy-skewed rows depends on, so today there is no query for "the rows this deploy left behind".
host_shell_generationsupplies the ordering: one row, whose generation advances each time a realm server observes a shell hash different from the one recorded. Servers observing the same shell read back the same number.Why the number is assigned here
The host has nowhere to carry one. It is static files published to S3 by
ember deploy, with no environment to inject and no task definition to stamp — which is exactly why the existing token is a hash of what the realm server fetches.Advancing on a transition rather than per distinct hash is what makes a rollback behave. Redeploying a bundle that ran before takes a new, higher generation, because a row's generation records when it was rendered rather than which artifact is semantically newer — and a row from the bundle being rolled away from has to remain findable, which it would not be if the rollback reused that bundle's earlier number.
Why the claim is one statement
A rolling deploy overlaps a task booting against the outgoing bundle with its neighbour on the new one, so two different shells get claimed at once. A read-then-write collapses them: both readers see the same starting generation, both compute the same successor, and two distinct shells end up sharing one number — which destroys the ordering, because rows from either then carry the same generation and nothing can tell them apart.
One
UPDATEcannot collapse that way.WHERE id = 1is unconditional, so the statement always takes the row lock and always returns the row it observed; the second writer blocks, and once the first commits, READ COMMITTED re-evaluates against the committed row, where aCASEdecides whether this claim is a transition to count or the shell already recorded.What changed under review
Worth calling out, because the original version of this description overstated the property above. The first implementation was two statements: the
UPDATE, plus a separate read for the idempotent case. Since eachexecuteis its own autocommit statement, that released the row lock in between — so a different-shell claim landing in the gap madeclaimHostShellGeneration(db, A)return B's hash and B's generation. A caller stamping its own render with that number would have attributed it to a bundle it never ran on, defeating the ordering during exactly the concurrency this exists to survive.Caught by Codex as a P1 and fixed in
8b2637da85. There is no second query left to race.Commits
8b2637da85— the claim answers from the statement that holds the lock (the P1 fix above).c659805772— regenerate the host's SQLite schema. Adding a migration requires it:packages/host/config/environment.jsrefuses to build unless the newest migration's timestamp matches the schema filename.host_shell_generationjoins the tables the dump excludes, alongsidejobs,queuesandsession_rooms— realm-server operational state the browser never reads. That also avoids a trap: the dump is schema-only, so an included table would reach the browser without the row the migration seeds, and a claim against it would find nothing to update and silently never advance.8b2637da85's predecessor — the table, the claim, and the tests.The other two commits are merges from
main.Tests
Eight cases cover the ordering itself; two more were added with the P1 fix.
The concurrency case is asserted against a real lock wait rather than a hopeful
Promise.all— the commit waits untilpg_stat_activityreports a backend blocked on the row, so the overlap exists rather than being hoped for. Substituting a read-then-write fails that test and only that test.A first draft raced eight same-shell claims and passed against both implementations, since claimants of one shell compute the same successor either way; it was replaced because it discriminated nothing. The P1 fix's own test is mutation-checked the same way: restoring the two-statement shape fails it with
actual: bbbbbbbb, expected: aaaaaaaa.All 10 pass in CI (Realm Server Tests shard 4).
Scope
Nothing reads the generation yet. Threading it onto index rows, and the repair query it exists to serve, follow separately on CS-12763.
Note on CI:
percy/-cardstack-hostreports "1 visual change needs review". This PR touches a migration, a shell script, a schema-file rename, oneruntime-commonmodule and tests — no UI code — and #6047 carried the identical Percy status when it merged. It needs approving in Percy rather than fixing here.