perf(events): resolve the runner binding without loading the conversation - #3419
Open
andrewreid wants to merge 3 commits into
Open
perf(events): resolve the runner binding without loading the conversation#3419andrewreid wants to merge 3 commits into
andrewreid wants to merge 3 commits into
Conversation
Three writes on the conversation store decided from a snapshot taken before the write, and lost or invented data when anything landed in between. Label seeding read the current labels, worked out which declared initials were missing, and upserted those. A policy write landing between the read and the upsert was overwritten back to its initial value. seed_labels_if_absent does insert-if-absent in one statement instead, so the database decides which keys are missing and a concurrent write survives. Session state was persisted by writing the whole blob back from the caller's snapshot. Two parallel tool calls on one session each held their own copy, so the second overwrote the first — two policies each incrementing a counter by one persisted one. mutate_session_state applies the caller's change inside a locked read-merge-write, serialised by SELECT ... FOR UPDATE where the dialect supports it and by BEGIN IMMEDIATE on SQLite, which takes the write lock before the first read. All three share one row-missing contract: a write that reads before it writes must not treat an absent row as empty. increment_session_usage did — absent row treated as empty, an UPDATE matching nothing, and the mutated total returned as persisted. Label seeding did too, differently: labels carry no foreign key, so seeding for a conversation that is gone left orphan rows and reported a snapshot nothing can read back. Both now raise, existence being checked in the same transaction as the write for the same reason the insert is insert-if-absent — a check the caller made earlier can already be stale. The contract is stated on the abstraction, so another implementation cannot phantom-write while nominally conforming. Two callers absorb that exception rather than propagate it, and they ship here because the primitive that raises owns its callers' handling: usage accumulation on the relay path and the cost-ask checkpoint mirrored to a tree root. A session deleted mid-turn has nothing to bill and a lost checkpoint re-prompts, so neither should fail a streaming turn. The engine's hot-state cache had the same "absence means something, but which thing" problem one layer up. apply_state_updates merges a caller's change into the persisted row and folds the authoritative result onto the in-memory cache — but a key the caller just DELETEd is, correctly, simply absent from that result, and a blanket union with the old cache put it right back: the delete took effect in the database and nowhere else, and the next evaluation in the same engine instance saw the pre-delete value again. The fix tracks which keys this batch of operations actually deleted, and only those are excluded from what survives from the old cache — a key missing from the merged result for any other reason (a sub-agent's inherited root-approval key, which is never part of its own row; a value seeded straight into the engine without ever being persisted) still survives, exactly as before. BREAKING: ConversationStore gains two abstract methods, so an out-of-tree subclass must implement them before upgrading. There is no compatible default — the whole point of both is that the decision happens inside one statement, and a base-class fallback built from the existing primitives would reintroduce exactly the race and the lost update being removed. SqlAlchemyConversationStore is the only in-tree implementation. Each oracle here is paired with the mutation that kills it. The seeding race is constructed rather than simulated: a store proxy commits a competing write while the helper reads its snapshot. The state race runs two threads whose transactions overlap; removing PostgreSQL's row lock loses an increment, and so does replacing SQLite's BEGIN IMMEDIATE with a deferred session. Skipping the existence check leaves orphan label rows the test then finds. Reverting the cost-ask mirror to snapshot-then-write is caught by observing which primitive the engine reaches for, since the visible outcome is identical either way. A delete of a session's own state key, a delete of a sub-agent's inherited approval key, and a delete of that same key name on a top-level session are each pinned separately, since an exemption scoped by key name instead of by which keys this call deleted gets the last of those three wrong. Signed-off-by: Andrew Reid <andrew@reid.ee>
…build Policy evaluation sits on the PreToolUse critical path — the hook blocks on the verdict — and spent most of its time re-reading the same rows. build_policy_engine fetched the conversation about four times (root resolution, labels, session state, model override) and walked the spawn tree twice, because the session-wide gating seed and the per-node subtree seed each called load_session_usage, which does its own conversation read plus a full paged tree scan. One conversation read and one tree scan now feed everything. Both usage seeds derive from that list through a pure aggregation, so they stay semantically distinct: cost gating remains tree-wide, so a sub-agent gates against the whole session's spend, while the subtree total remains the per-node display figure. A caller that already holds the row can pass it and skip the read. A row the caller supplies is a HINT, not a fact. It names a tree, and loading that tree verifies the claim: if the conversation is not in it, the root is resolved again. Everything downstream — the rows, the root id, the policies attached to that root, the accounting sums — comes from the tree that verification produced. Deriving the root from the caller's row while taking rows from a corrected tree mixes two epochs, and a conversation deleted and recreated under a different root then seeded the old tree's spend. Mutable state is likewise re-derived rather than trusted: labels, session state, model override and agent binding all come from the verified tree, whoever read the row first, because a caller's preload and this function's own read are equally stale by the time a decision is made. A row absent from the tree is confirmed with one re-read and then fails closed. A tree that needed more than one page cannot vouch for its own rows — page one was read before page two — so identity is confirmed once in that case, which single-page trees never pay for. Also here, because it is the same tree: the ancestor cost re-publish used to do a conversation read plus a full tree scan PER ancestor, and derived the chain from a row read earlier in the request. It now walks the verified tree, so the whole fan-out costs one load and cannot publish to a chain that has since changed. A chain that cannot be walked to the root yields nothing rather than a prefix, since the caller publishes to every id returned. The tree also stopped excluding archived conversations. Archiving is a listing concern; the tree is an accounting structure. Excluding them let an archived root — or an archived mid-tree node, which orphaned its descendants from the walk — seed the enforcement total as $0 and allow a tool call over budget. Archived spend consequently appears in displayed totals too, which is the intended reading: the badge should agree with the gate. Measured on both dialects: 30 queries per build to 6, or 3 when the caller supplies the row. The whole authenticated route, by (tree size, whether the caller supplies the row): 11 on a one-page tree when supplied, 14 when not; 17 on a 101-node tree when supplied, 20 when not. The tree load pages, so cost is not independent of tree size, and the extra 3 on a paged tree over the one-page count are the paging confirmation above, a full conversation read — consistent at both tree sizes and both supplied/not-supplied. Counted as SQL statements rather than store calls, because a store-call count cannot see a helper that issues three statements per call. The route-level oracle below covers only the one-page shape; the 101-node figures are measured, not pinned by a test yet. Every oracle here is paired with the mutation that kills it, including the two that pin this round's fixes: deriving the root from the pre-refresh row fails the recreated-child test, and skipping the paged-tree confirmation fails the switch-during-paging test. Signed-off-by: Andrew Reid <andrew@reid.ee>
…tion The per-event endpoint fires once per streamed chunk and accounts for most of the server's query volume during an active turn; one observed thirty-eight-minute turn cost roughly thirteen thousand queries. The redundancy this removes is one read per event. Runner routing loaded the whole conversation — row, metadata and labels — to use a single column. It now reads just the binding. The resolution stays current, which an event path resolving a runner per chunk requires: a rebind or host handoff that has already landed routes to the new runner. It does not fence the window between resolving and forwarding; that needs a binding generation on the wire and is out of scope here. The narrowed lookup keeps the existence semantics the full read carried, in both directions. The binding lives on the Omnigent metadata row while the AP conversations row is the existence authority — deletion is ordered AP row first, and a failed second transaction leaves orphaned metadata as a documented trade-off, acceptable only because reachability checks consult the AP row. Creation writes the same two rows in the opposite order, so either can outlive the other, and the two states mean different things: a deleted conversation must be reported missing, an existing one without a binding must be reported unbound. A metadata-rooted lookup answered the first correctly and turned the second into a false not-found. The lookup is therefore rooted in the AP row, by outer join on one bind and by seeding existence then overlaying bindings on two. Usage events reuse the handler's row where the value is immutable: the tree root passed to the subtree sum. They deliberately do NOT reuse it for the ancestor fan-out. What an ancestor's badge should read is a fact about publication time, not about when the request arrived, so the fan-out loads its own tree — reusing the one summed for this session meant two siblings publishing from their own request-start snapshots delivered a newer total followed by an older one, leaving the parent showing the smaller figure. That costs one tree load per usage event with ancestors, and the honest measurement is below. Deliberately unchanged: the per-event access check stays fresh, so a caller who loses access mid-stream stays gated; the native anti-replay clamp keeps its own read, because it must merge against the newest stored usage; and the message and tool-result paths keep fresh runner resolution, since input policy evaluation can hold those requests for seconds and a wrong-runner tool result returns 200 while dropping the result. `_ancestor_session_ids` likewise refuses a caller-supplied starting row: those ids decide where events are published, and a row read earlier can name a chain the session has left. Measured on both dialects, per table rather than as a ceiling on one of them: an idle status event is 5 statements; a usage event is 4 conversations, 5 metadata and 4 labels. A ceiling on conversations alone hid the other tables and also passed with the anti-replay read removed. Oracles ship with the mutation that kills them. The status event is delivered through the app's real router — for four review rounds this test bound a runner without registering it, so routing reported it offline, delivery fell through to the process-wide client, and the assertion observed nothing about the read it protects; the process-wide client is now a trap that raises if used. The fan-out oracle constructs the interleave rather than hoping for it, committing a sibling's spend immediately after this request's first tree load. A leftover comment at the tree-scan call site claimed it fed both this session's own subtree total and the ancestor fan-out below, from back when the fan-out reused that tree — the fan-out has its own comment explaining why it now reads a fresh one instead, and the two disagreed. And the fan-out's regression test claimed a universal "badge never moves backwards" guarantee; narrowed to the specific request-start-snapshot regression it actually pins, since a sibling landing after the fan-out's own tree read but before publish can still be missed, identically to unmodified main. Signed-off-by: Andrew Reid <andrew@reid.ee>
13 tasks
Contributor
|
@andrewreid This PR is a Bug fix, Feature, or UI / frontend change but the Demo section is missing or only contains a placeholder. These change types require a screenshot or screen recording so reviewers can see the new behaviour without checking out the branch. Please update the Demo section with:
Use |
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
4 participants
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.
Related issue
Closes #3004
Stacked on #3417 (policy-engine-load-once), which is itself stacked on #3407 (store-write-primitives) — this branch includes both those PRs' commits until they merge, so the diff below shows all three; only the last commit (
perf(events): resolve the runner binding without loading the conversation) is new here.Summary
The per-event endpoint fires once per streamed chunk and accounts for most of the server's query volume during an active turn — one observed thirty-eight-minute turn cost roughly thirteen thousand queries. Runner routing loaded the whole conversation (row, metadata, and labels) just to read a single column — the bound runner id. It now reads just that column.
ELI5: every streamed chunk of a running turn used to ask the database "tell me everything about this session" just to find out "which runner is it on right now" — one fact buried in a full row. Now it just asks for that one fact.
The resolution stays current, which an event path resolving a runner per chunk requires: a rebind or host handoff that already landed routes to the new runner. It does not fence the window between resolving and forwarding — that would need a binding generation on the wire, and is out of scope here.
A real correctness fix rides along, because the narrowed lookup has to get the same existence semantics right that the full read carried for free. The runner binding lives on the Omnigent metadata row while the AP conversations row is the existence authority; deletion is ordered AP-row-first, so a failed second transaction can leave orphaned metadata behind (a documented, accepted trade-off). A lookup rooted in the metadata row answered "does it exist" correctly but turned "exists but has no binding yet" into a false not-found. The lookup is now rooted in the AP row instead — outer-joined on a single bind, or existence-seeded-then-overlaid on split-DB — so "deleted" and "exists but unbound" are reported as the two distinct states they actually are.
Usage events reuse the handler's own row where the value is immutable — the tree root passed to the subtree sum — but deliberately do NOT reuse it for the ancestor cost re-publish. What an ancestor's badge should show is a fact about publication time, not about when the request arrived: reusing the tree summed for this session meant two siblings publishing from their own request-start snapshots could deliver a newer total followed by an older one, leaving a parent's badge showing the smaller figure. The fan-out now loads its own tree at publish time, at the cost of one tree load per usage event that has ancestors.
Deliberately left unchanged, and called out explicitly in the commit so it isn't mistaken for an oversight: the per-event access check stays fresh (a caller who loses access mid-stream must stay gated); the native anti-replay clamp keeps its own read (it has to merge against the newest stored usage); the message and tool-result paths keep fresh runner resolution (input-policy evaluation can hold those requests for seconds, and a wrong-runner tool result returns 200 while silently dropping the result); and the ancestor-id walk refuses a caller-supplied starting row, since those ids decide where events get published and a row read earlier can name a chain the session has since left.
Test Plan
Measured on both dialects, per table rather than as a ceiling on one of them: an idle status event is 5 statements; a usage event is 4 conversations, 5 metadata, and 4 labels. No wall-clock benchmark was run for this PR specifically — the internal hook routes it touches aren't exercised by the client-facing benchmark harness in this repo, so the statement-count matrix above is the evidence, same as #3417.
Demo
N/A — backend/event-path change, no UI surface.
Type of change
Test coverage
Coverage notes
Oracles ship with the mutation that kills them. The status-event test is delivered through the app's real router rather than a stub — a prior version of this test bound a runner without registering it, so routing reported it offline, delivery silently fell through to a process-wide fallback client, and the assertion observed nothing about the read it was meant to protect; that fallback client is now a trap that raises if it's ever used, so a regression can't hide behind it again. The ancestor fan-out test constructs the interleave rather than hoping to observe it — it commits a sibling's spend immediately after this request's first tree load, so it reliably exercises the two-siblings-racing scenario the fix addresses. A ceiling on conversation-table statements alone would have hidden the metadata/label tables and also passed with the anti-replay read removed, which is why the oracle counts each table exactly.
Changelog
Fixed a bug where a session that existed but had no runner bound yet could be reported as not found instead of unbound.