Check a ref against the run this action is reaching, not the last one - #271
Open
zopeVaibhav wants to merge 2 commits into
Open
Check a ref against the run this action is reaching, not the last one#271zopeVaibhav wants to merge 2 commits into
zopeVaibhav wants to merge 2 commits into
Conversation
zopeVaibhav
requested review from
MikeRyanDev,
davidmckayv,
guidovizoso and
tylerslaton
as code owners
August 27, 2026 05:36
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
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.
Closes #236
What this changes
A
refnames an element in a snapshot, and it only means anything for the run of the computer that produced it. A replaced container starts counting generations at one again, reaches seven again, and the samerefat generation seven resolves on both sides — to two different pages. The gateway already checked the run before resolving. The check did not work, in two separate ways.The run was remembered in one process.
sessionswas aMapat module scope insupervisor.ts, so every provider built in that process shared it. Two supervisors, or a second stack inside one test, answered each other's question about which run a Bot is on, and the answer was whichever wrote last. It is now built insidecreateDockerSupervisorProvider: one map per provider. It is still process-local, and the comment above it still says so — a replica that has never located this Bot has nothing in it.The check ran one action too late.
governaskedsessionOfbefore this action's ownlocate, so it compared the stored snapshot against the previous action's run. That is the same run on every action but one: the first after a replacement, which is exactly the action the check exists to catch. The click after a replaced container was allowed and the one after it refused — a guarantee arriving one action late. The order is now locate, then ask which run that was.Only actions that cite a
reflocate first. Nothing else resolves against a snapshot, so a scroll or a file read should not have to reach the supervisor before the policy has seen it. The address thatlocatereturns is threaded into the attempt, so the check and the send are one trip rather than two — and two could disagree, which would mean deciding against a run the action was not sent to.locateForActionanswersundefinedrather than throwing when the computer cannot be located. Throwing there would take the action off the audit trail entirely; the action still has to be decided and recorded, and the attempt failing is what writes the failure row beside the decision.The question the issue asked
#236 ends by asking what an unknown run should mean. This keeps the existing answer — unknown skips the generation check rather than refusing — because treating it as a mismatch would refuse for the shared computer, which has no run to report at all. The two holes above are closed without changing that, so a replica now knows the run because it located the Bot itself, not because unknown became fatal. If you would rather unknown were a refusal, that is a smaller change on top of this one and I am happy to make it.
Where it runs
Mapbecame one per provider.refresolved. Now every ref-bearing action locates first, so the replica learns the run from the supervisor rather than from a map it does not have./ensureis already the single source of which run is current; this reads it at the right moment rather than caching an older answer.Boundary and audit
StaleSnapshotErroron the first action rather than the second, and that refusal is audited like any other. A locate that fails does not throw, so the attempt still writes its failure row.Changelog
No
CHANGELOG.mdentry proposed, but say the word and I will add one. A deployment does behave differently — a stale ref is refused where it was previously allowed — so if that is worth an operator's attention it belongs underUnreleased. I left it out because the previous behaviour was a defect rather than a documented guarantee.Proof
Two test files. The first drives the real
createDockerSupervisorProviderin-process; the second is the deployment shape the bug is actually about.server/tests/computer-session-guard.test.ts— five cases:server/tests/computer-session-guard-replicas.integration.test.ts— two replicas against a real PostgreSQL. Each builds its own provider and its own gateway and shares nothing but the database, which is how the snapshot crosses between them. Replica A takes the snapshot during the first run; the test asserts the row really is incomputer_snapshotcarrying that run. The container is then replaced, and replica B — a different process that has never located this Bot — gets the click.Both files carry a control, because "refuses the stale ref" is also satisfied by refusing everything.
The new test fails on
mainand passes here. Reverting justgateway.tsandsupervisor.tstomainand running it:The click is not refused, it is performed, and the audit row names
Submit order— a button on a page that is gone. The control passes in both, so the difference is the guarantee and not the harness.With the fix restored:
Whole server suite on this branch:
bun run typecheckpasses acrossapp,serverandworker.bun run lintreports no findings on 447 files, andbiome formatleaves the branch clean.Not done: a live cluster or compose run with a container genuinely replaced underneath a conversation. The supervisor and the computer are stubbed in both files; PostgreSQL is real. So this proves the ordering, the map scope and the cross-process path, but not the container lifecycle itself.