Take back the two features that only worked on one machine - #21
Merged
Conversation
The approval registry from #15 kept its pending questions in a Map in the process, and the repetition detector from #17 kept its counts the same way. Both are correct on a laptop and both stop being correct the moment a deployment runs a second server process, which is the deployment every part of this is aimed at: several processes behind a load balancer, serving a company. The failure mode is the reason to take them back rather than leave them and fix them later. Neither one breaks loudly. A question raised on one process and answered on another is reported as no longer open, which reads exactly like an expiry. Counts split across processes mean a rule written as `repeat.count >= 10` never fires, which reads exactly like a Bot behaving itself. A boundary that silently stops enforcing is worse than one that was never advertised, because the deployment is relying on it. The ask lists, the approval surface, the repetition context and the two audit event kinds go with them. What stays is everything that was already right: the stall watchdog, which tracks streams in the process that holds them and belongs there, and the client-side work from #16 and #19. The rule is now on the pull request template, stated before the work rather than at review. Our own gateway snapshot cache has the same problem and is next.
davidmckayv
requested review from
MikeRyanDev,
guidovizoso and
tylerslaton
as code owners
August 20, 2026 02:17
8 tasks
kevin9327
added a commit
to kevin9327/openbot
that referenced
this pull request
Aug 21, 2026
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>
davidmckayv
pushed a commit
to kevin9327/openbot
that referenced
this pull request
Aug 21, 2026
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>
davidmckayv
added a commit
that referenced
this pull request
Aug 21, 2026
… it (#46) * Keep the snapshot a ref resolves against where every replica can read it 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. #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> * Keep the newest snapshot when two replicas take one at once 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. --------- Co-authored-by: kevin9327 <kevin9327@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: David McKay <davidmckayv@users.noreply.github.com>
This was referenced Aug 21, 2026
davidmckayv
pushed a commit
that referenced
this pull request
Aug 22, 2026
#118 asked for the knowledge/ modules and the three tables to be dropped in a change that says so. This is the half that can be done now. server/src/knowledge/acl.ts, repository.ts and types.ts have no importer outside their own two test files. InMemoryKnowledgeRepository holds documents, chunks and ACLs in a Map in the process, which is the shape #21 took back, and canRead(actor, entries[]) filters ACL rows already pulled into memory rather than in SQL. Neither is a starting point for anything #119 describes. The three tables stay for now. PR #97 has not merged, so connectors/sync-persistence.ts still imports documents, chunks, document_acls, connector_cursors and sync_runs and writes to all of them; dropping the tables breaks typecheck there. It is already orphaned at runtime, its only caller being its own integration test, so this is a typecheck dependency rather than a live one, but it is #97's to remove. Left alone deliberately: connector_instances, which server/src/connectors.ts writes to from production code; connector_cursors and sync_runs, which are connector-side and go with #97; and webhook_subscriptions, which is referenced nowhere at all and wants its own change. Docs that describe pgvector holding knowledge records stay accurate while the tables exist and belong in the commit that drops them.
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.
What this changes
Reverts #15 (the approval registry) and #17 (the repetition detector), and puts the deployment rule on the pull request template.
Why
Both features keep state that outlives a request in a
Mapinside the server process. That is correct on one machine and wrong the moment a deployment runs a second one, which is the deployment this product is aimed at: several server processes behind a load balancer, serving a whole company, with people bringing their own agents.Neither fails loudly, which is why they are being taken back rather than left in place and fixed later:
no longer open. That is indistinguishable from an expiry, so nobody investigates.repeat.count >= 10never fires. That is indistinguishable from a Bot behaving itself.A boundary that quietly stops enforcing is worse than one that was never advertised, because the deployment is relying on it.
What stays
Everything that was already right. The stall watchdog from #19 tracks open streams in the process that holds them, which is where they belong. The client-side work in #16 and #19 is unaffected. The
Did not happenaudit filter keepsagent.stream_stalledand loses only the repetition entry.What goes
The
askpolicy list and its surface, the approval routes and registry, the repetition context on the policy engine,COMPUTER_REPEAT_WINDOW_MS, and thecomputer.action_repeatedand approval audit kinds. Migration0001_gigantic_sumogoes with it; it added one column,action_policy.ask. Any database that already applied it keeps a defaulted column nothing reads, anddrizzle-kit generatereports no drift.Still to do
The gateway resolves every action's target from a snapshot cache that is also a
Mapin the process. It predates all of this and it is load-bearing for the action boundary, so it is the next thing to move to Postgres.Proof
format:check,lintandtypecheckclean. 647 pass, 5 skip, 0 fail.db:generatereports no schema changes.