Skip to content

Check a ref against the run this action is reaching, not the last one - #271

Open
zopeVaibhav wants to merge 2 commits into
CopilotKit:mainfrom
zopeVaibhav:fix/supervisor-session-per-process
Open

Check a ref against the run this action is reaching, not the last one#271
zopeVaibhav wants to merge 2 commits into
CopilotKit:mainfrom
zopeVaibhav:fix/supervisor-session-per-process

Conversation

@zopeVaibhav

Copy link
Copy Markdown
Contributor

Closes #236

What this changes

A ref names 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 same ref at 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. sessions was a Map at module scope in supervisor.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 inside createDockerSupervisorProvider: 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. govern asked sessionOf before this action's own locate, 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 ref locate 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 that locate returns 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.

locateForAction answers undefined rather 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

  • New state that outlives a request? None added. Existing state was narrowed: a module-scope Map became one per provider.
  • What happens on the second replica? This is the fix. Before, a replica that had not located the Bot skipped the check silently and a stale ref resolved. 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.
  • Anything serialised? Nothing new. The supervisor's /ensure is already the single source of which run is current; this reads it at the right moment rather than caching an older answer.
  • Anything fanned out to a browser? No.
  • New listener, port, or schedule? No.

Boundary and audit

  • Every acting call still goes through the gateway: resolve, decide, audit, then act. The locate is added before resolve, which is what makes resolve correct.
  • New refusals and new failures each write a row. A stale ref now raises StaleSnapshotError on 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.
  • Nothing new is trusted from the client. The run comes from the supervisor; the caller supplies only the ref, which is what is being checked.

Changelog

No CHANGELOG.md entry 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 under Unreleased. 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 createDockerSupervisorProvider in-process; the second is the deployment shape the bug is actually about.

server/tests/computer-session-guard.test.ts — five cases:

the first action after the computer is replaced is refused, not the second
a replica that never located the Bot itself refuses too
a deny rule stops naming an element from a run that has ended
the run is read per provider, not once per process
CONTROL: a ref from the run that is still current still resolves

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 in computer_snapshot carrying 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 main and passes here. Reverting just gateway.ts and supervisor.ts to main and running it:

error: expect(received).toBeInstanceOf(expected)
Expected constructor: [class StaleSnapshotError extends Error]
Received value: {
  action: "click",
  url: "https://example.com/order",
  elapsedMs: 1,
  element: { role: "button", name: "Submit order" },
}
(fail) replica B refuses a ref that replica A took before the container was replaced
 1 pass
 1 fail

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:

$ bun test tests/computer-session-guard-replicas.integration.test.ts tests/computer-session-guard.test.ts
 7 pass
 0 fail

Whole server suite on this branch:

 1300 pass
 0 fail
Ran 1300 tests across 92 files. [9.32s]

bun run typecheck passes across app, server and worker. bun run lint reports no findings on 447 files, and biome format leaves 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A ref from a replaced computer still resolves, because the run it is checked against is held in one process's memory

1 participant