Keep the snapshot a ref resolves against where every replica can read it - #46
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
c6f0d42 to
7ebe503
Compare
The gateway turns the opaque ref in an acting call into the element it points at, from the snapshot this server took, and then decides and records against that element. That mapping lived in a Map in the process. OpenBot runs several server processes behind a load balancer, and the process that answers a snapshot is rarely the one that answers the click that uses its refs, so on every other replica the Map was empty: the ref resolved to nothing, the policy decided with no element in front of it, and the audit row could not name what was touched. Neither outcome is loud. A deny rule written against the element fails closed and refuses every click on the replicas that did not snapshot, which reads as the computer being flaky. A rule that does not name the element lets the click through unresolved and unrecorded, which reads as the boundary being quiet because nothing matched. Either way the boundary is not doing what the operator wrote, and nothing says so. CopilotKit#21 took two features back for this exact shape and noted this cache had it too. So the snapshot goes through Postgres, the way channel activity and the policy already do: one row per computer, upserted on every snapshot, read on the action path. A ref resolves on whichever replica the click lands on. Staleness, the reason a persisted snapshot cache is rightly regarded with suspicion, is answered by the generation the far-side computer stamps on every snapshot. A ref resolves only when its generation matches the stored one, so a ref from a superseded page resolves to nothing rather than to whatever now holds it: it cannot resolve to a name that is no longer on screen, because a ref from an old screen no longer matches. The computer makes the same generation check when the action reaches it; this keeps the policy decision and the audit row honest first, on whichever replica the action landed. The client-supplied snapshotId only ever narrows resolution. It can yield the true element of the current snapshot or nothing, never a different element than the ref already names, so nothing new is trusted from the client that the server does not resolve itself. Without a database the gateway still keeps snapshots in memory, so a single-process test does not need Postgres, exactly as the policy store does not. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
7ebe503 to
ca79016
Compare
Two things the cross-replica move needs that a single process did not. The upsert was last-write-wins. Two replicas snapshotting the same computer is the case this table exists for, and Postgres has no say in which of their writes arrives second, so the older generation could overwrite the newer one and every ref the model was holding would stop resolving. The generation decides instead: it is stamped by the computer and increases by one per snapshot, so a lower number is an older page. Clocks cannot decide it, because there are two of them. Wiping a computer now clears the row. A fresh computer counts generations from one again, so a row left behind lets a ref from the previous session match the new session's first snapshot, and the policy decides against an element from a page that is gone. The generation only tells snapshots apart within a session, so the row has to go when the session does. Tests for both, and for clear on each store. The ordering one fails without the guard.
ca79016 to
f0e1785
Compare
davidmckayv
left a comment
davidmckayv
left a comment
There was a problem hiding this comment.
This is the one that actually changes what the product is, and I proved the failure it fixes rather than reasoning about it.
Two replicas on one Postgres, same deployment, same deny rule contains(element.name, "submit"), same ref. Replica A snapshotted; replica B, which never did, handled the click.
With the store in Postgres: B resolved f1e11 to "Submit order" and refused it by name, and allowed the Cancel button beside it with the element correctly resolved.
With the store back in process memory, which is what main does: B submitted the order. The URL moved to /thanks, and the audit row recorded it as allowed, with element "not in the current snapshot" and no rule. Not a refusal, not a flake. The boundary was simply not there, and the trail said the action was fine.
That is worth stating plainly, because it is a sharper failure than the PR body claims. The body says a deny rule fails closed and refuses everything on the other replicas. It does not: govern binds every context field to a neutral value when nothing resolves, so contains("", "submit") is false, the deny does not match, and the click goes through. It fails open, quietly, on every replica but one.
The generation check is safe. asRef requires ref and snapshotId together on all three acting routes, so a ref never reaches the gateway without the generation it came from, and the far side already made the same check.
Two things I added on top, in their own commit:
- The upsert was last-write-wins. Two replicas snapshotting the same computer is the case this table exists for, and Postgres has no say in which write lands second, so an older generation could overwrite a newer one and every ref the model was holding would stop resolving. It is now forward-only on
snapshot_id, which is stamped by the computer and increases by one. Clocks cannot decide it; there are two of them. The test fails without the guard. - Wiping a computer now clears the row.
maingainedsnapshots.delete(botId)on reset after this branch was written. A fresh computer counts generations from one again, so a row left behind lets a ref from the previous session match the new session's first snapshot, and the policy decides against a page that is gone.
I also rebased onto main, which had moved a long way: the migration renumbered to 0005 (regenerated, not hand-edited), the gateway is now behind the ComputerProvider seam and keyed on botId, and the cross-replica tests needed porting to that harness.
48 tests pass across the three files.
2 participants
What this changes
The gateway resolves the opaque
refin an acting call into the element it points at, from thesnapshot this server took, and then decides and audits against that element — step 1 of
resolve → decide → record → act, the one the file header calls "fatal to skip". That mapping lived
in a
Mapin the process.With more than one server process the snapshot lands on one and the click that uses its refs on
another, so the
Mapis empty on the replica that answers the click: the ref resolves to nothing,the policy decides with no
elementin its context, and the audit row can only record"not in the current snapshot". A deny rule written against the element then fails closed andrefuses every click on those replicas, which reads as a flaky computer; a rule that does not name the
element lets the click through unresolved and unrecorded, which reads as the boundary being quiet
because nothing matched. Either way the boundary stops doing what the operator wrote, and nothing
says so. This is the cache #21 flagged as still having the same problem, after taking two features
back for exactly this shape.
This moves the snapshot into Postgres — one row per computer (
computer_snapshot), upserted on everysnapshot, read on the action path — so a ref resolves on whichever replica the click lands on. A ref
resolves only when the
snapshotIdit carries matches the stored generation, so a ref from asuperseded page resolves to nothing rather than to whatever now holds it: the concern that a
persisted cache would "decide on fiction" is answered by the generation, not by keeping the cache in
memory. The agent-computer still makes the same generation check when the action arrives; this keeps
the policy decision and the audit row honest first, on whichever replica the action landed.
Where it runs
table
computer_snapshot, keyed bycomputer_id— not aMapor a closure.the ref, so the policy sees the real element and the audit row names it. Before this change the
ref resolved to nothing there and the boundary was blind.
computer_id, written withINSERT … ON CONFLICT (computer_id) DO UPDATE. Two processes snapshotting the same computer:last write wins, which is the correct meaning, because the newest generation is the current
page. No check-then-write.
action path, never pushed to a socket.
Boundary and audit
resolvechanged — it reads the shared snapshot and matches the generation.resolves, the row says so, and the computer refuses it on arrival.
snapshotIdonly narrows resolution: it yields thetrue element of the current snapshot or nothing, never a different element than the ref already
names. The ref-to-element mapping stays server-held.
Proof
Local (Windows, Bun 1.3.14, no Postgres):
bun run typecheckacross app/server/worker — clean.biome check(format, lint, organizeImports) on every changed file — clean.bun test server/tests/computer-gateway.test.ts server/tests/computer-snapshot-store.test.ts— 24 pass. The three new gateway tests build two gateways over one shared store (two replicas, one
database) and prove a click is resolved and refused on the replica that never took the snapshot,
that the resolved element label is available there, and that a ref only resolves against its own
generation.
channel-storeandcredential tests that need Postgres and the agent-computer symlink tests that need Windows symlink
permission — both environmental, neither in the changed code.
CI covers what this machine cannot:
drizzle-kit migrateapplies0001_computer_snapshot, andcomputer-snapshot-store.integration.test.ts(modelled onpolicy-durability.integration.test.ts)proves a snapshot saved by one store is resolvable by a second store on the same database, that a
newer snapshot supersedes the last with one row per computer, and that the JSON round-trip keeps each
element's
type.This is independent of my open #45 (fleet listing); it touches
computer/gateway.ts, a newcomputer/snapshot-store.ts, thecomputer_snapshottable, and the composition root only.