perf(policies): load the conversation and spawn tree once per engine build - #4320
Conversation
github-actions
Bot
added
P1-high
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Benchmark results (SQLite, PR #4320)Commit: Benchmark comparisonRegression threshold: 100% on avg P50 or avg P95.
PASS — no regressions detected. |
This comment was marked as off-topic.
This comment was marked as off-topic.
da2b60b to
e6aeb44
Compare
|
/review |
|
e6aeb44 to
469da68
Compare
…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> Signed-off-by: Tomu Hirata <tomu.hirata@gmail.com>
469da68 to
dd04dcb
Compare
|
/review |
1 similar comment
|
/review |
|
|
🏷️ Doc impact: Internal refactor and concurrency-hardening of the policy-engine builder (single tree scan, fresh-read/fail-closed semantics, archived-spend accounting) plus tests and benchmarks — no user-facing surface, integration, or built-in policy was added, removed, or reconfigured. Auto-classified on merge. Set the label manually before merging to override. · run |
Related issue
Closes #3003
Summary
Policy evaluation sits on the PreToolUse critical path — the hook blocks on the verdict before a tool call can proceed. `build_policy_engine` fetched the conversation about four times per call (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 independently called `load_session_usage`, which does its own conversation read plus a full paged tree scan.
ELI5: instead of asking "who's the root, what are the labels, what's the state, what's the model, and what has everyone spent" as five separate trips to the database, the engine now makes one trip, reads everything off that one snapshot, and derives every other answer from it in memory.
SQL statement counts (measured, now pinned by automated oracles):
The builder used to re-fetch the conversation ~4× and walk the tree twice; the route number (11) covers ACL resolution (3), handler conversation load (3), session-policy lookup, agent row, and spawn-tree scan (3). A caller that already holds the conversation row can pass it as a hint — the builder verifies identity against the fresh tree and skips its own read, dropping the builder cost from 6 → 3 statements.
One conversation read and one tree scan now feed everything. Both usage seeds derive from that same list through a pure aggregation, so they stay semantically distinct: cost gating remains tree-wide (a sub-agent gates against the whole session's spend), while the subtree total remains the per-node display figure.
Two real correctness fixes ship in the same change, because they touch the same tree load:
Also included: `apply_state_updates` suppresses `ConversationNotFoundError` consistently on all approval paths (native ask gate, MCP retry, relay `_apply_pending_policy_ask_writes`). The `_apply_pending_policy_ask_writes` pop is deferred until after a successful engine build to avoid silently dropping approved writes when a concurrent agent rebind raises.
A `policy_evaluate` journey is added to the benchmark harness (`dev/benchmarks/omnigent`) so future regressions in evaluate-route latency are detectable without re-measuring manually.
Test Plan
Demo
N/A — backend/policy-engine change, no UI surface.
Type of change
Test coverage
Coverage notes
Every oracle is paired with the mutation that kills it. The SQL statement counts are pinned by `test_build_issues_one_read_and_one_tree_scan` (builder, both preload/no-preload shapes) and `test_authenticated_evaluate_route_sql_budget` (route level). Removing the `conversation=` preload arg changes the builder count from 3 → 6, which the budget oracle detects even though both variants return the same verdict.
Changelog
Fixed a bug where an archived session (or an archived sub-agent) could be gated as if it had spent nothing, letting a tool call proceed over its actual budget.