perf(server): eliminate N+1 queries in inbox context assembly and bound-agent validation - #2034
perf(server): eliminate N+1 queries in inbox context assembly and bound-agent validation#2034bestony wants to merge 2 commits into
Conversation
…ueries collectPrecedingContext issued one previous-notify query per chat plus one silent-context query per trigger, sequentially inside the claim transaction, so a 50-entry drain could run ~100 round trips while holding row locks (PERF-061). Resolve the whole batch with two statements regardless of size: a raw jsonb_to_recordset + lateral statement that computes each chat's previous-notify cursor and every trigger's context membership (same created_at DESC cap selection and FOR UPDATE OF ... SKIP LOCKED semantics), returning ids only, then one typed select that re-reads message payloads so jsonb/timestamptz mapping stays on Drizzle. In-batch cursors are computed in JS and passed in the spec, keeping trigger-relative windows unchanged. Refs #1724
…uery inbox:ack and every heartbeat revalidated each bound agent through ensureAgentStillRoutedHere, one SELECT per agent (twice per heartbeat), and a client has no hard agent-count cap (PERF-061). Add filterAgentsStillRoutedHere, which validates all candidates with a single WHERE uuid IN (...) select and applies the same per-agent decision logic: active route match stays routed, the runtime-switch claimed grace state stays bound but unrouted, anything else drops the local binding. The ack handler, heartbeat route check, and heartbeat repair re-check now issue one query per checkpoint, and the ack path keeps its inbox-to-agent ownership mapping. ensureAgentStillRoutedHere becomes a single-id wrapper, so per-frame handlers keep their exact semantics. Refs #1724
yuezengwu
left a comment
There was a problem hiding this comment.
Implementation goal and core changes are clear: preceding silent context is reduced from per-chat/per-trigger round trips to one lateral membership query plus one typed payload read, while bound-agent validation is consolidated into one set-based lookup per ACK/heartbeat checkpoint.
Reviewed the trigger cursor split, cap/window ordering, FOR UPDATE ... SKIP LOCKED, deferred silent-row ACK behavior, inbox-to-agent ownership mapping, and runtime-switch claimed grace path. These remain equivalent to the prior semantics and consistent with the inbox delivery constraints. No blocking findings.
No schema, index, or core data-structure changes. Diff review only; I did not run additional tests.
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: comment
- Rationale: The implementation appears semantically sound, but this PR overlaps an already-open, approved alternative for the same issue; one canonical implementation and its regression guards should be selected before merge.
Risk level: B-high
- Path baseline: only
packages/server/**→ B-low. - Semantic lift:
ws-client.tschanges core bound-agent route validation → B-high.
PR summary
- Author / repo: bestony / agent-team-foundation/first-tree
- Problem: Large Inbox drains and Clients with many bound agents make database round trips grow with trigger or agent count, increasing delivery lock time and recurring ACK/heartbeat load.
- Approach: Resolve every trigger's silent-context membership through one LATERAL statement plus one typed payload read, and validate all bound-agent routes with one set query per checkpoint while retaining the single-agent wrapper.
- Impacted modules:
packages/server/src/services/inbox.ts,packages/server/src/api/agent/ws-client.ts, and their server tests.
Review findings
❓ 1. PR #2030 is still open, already approved, authored by the same person, and also closes #1724 with a different implementation of the same two changes. Please identify whether #2034 supersedes #2030 and close/cross-reference the non-canonical PR so reviewers and maintainers do not have two competing fixes for one source of truth. [R2 / PR coordination]
packages/server/src/__tests__/inbox-ws-push.test.ts, packages/server/src/__tests__/ws-client-branch-fake.test.ts]
✅ 3. Static review found the current 24-hour/count bounds, per-chat and in-batch cursors, deferred silent-row ACK, SKIP LOCKED, inbox ownership mapping, and runtime-switch claimed grace behavior preserved.
Action taken
- Submitted a comment review; no approval or merge action taken.
Closes #1724 (PERF-061).
Problem
Two hot paths issued sequential per-row queries while holding transaction work / socket-serialized sections, so DB latency and lock hold time grew linearly with batch and client size:
collectPrecedingContext(packages/server/src/services/inbox.ts): a 50-entry drain issued one previous-notify query per chat plus one silent-context query per trigger, all inside the claim transaction.ws-client.ts:inbox:ackand eachheartbeatrevalidated every bound agent throughensureAgentStillRoutedHere, oneSELECTper agent, twice per heartbeat — and a client has no hard agent-count cap.Change
Inbox context assembly — 2 statements total, regardless of batch size
jsonb_to_recordsetspec + two laterals) resolves, per claimed trigger: the previous-notify cursor for each chat's first trigger, then the per-trigger silent-context membership with the sameORDER BY messages.created_at DESC LIMIT capselection andFOR UPDATE OF e SKIP LOCKEDlocking the old per-trigger queries used. In-batch cursors (previous trigger of the same chat) are computed in JS and passed in the spec, so the trigger-relative windows stay exactly as before.::textids becauseexecute()bypasses the postgres-js type parsers (jsonb/timestamptz would come back as raw strings).Preserved semantics: bundling still does not ACK silent rows; cap keeps the rows closest to the trigger; chronological output ordered by
messages.created_at(entry id as tie-break);SKIP LOCKEDstill prevents two parallel polls from double-bundling; per-trigger ranges are disjoint within the batch.Bound-agent route validation — one set-based query per checkpoint
ensureAgentStillRoutedHereis now a thin wrapper over a newfilterAgentsStillRoutedHere(agentIds), which validates all candidates with a singleWHERE uuid IN (...)select and applies the same per-agent decision logic (active/route match → routed; runtime-switch "claimed" grace → not routed but binding retained; anything else →dropLocalAgentBinding). Theinbox:ackhandler, the heartbeat route check, and the heartbeat repair re-check each now issue one query for the whole bound set, and the ack path keeps its inbox-to-agent ownership mapping from the survivingboundAgentsvalues.No schema/index changes: the batched statements use the same predicates as the per-row queries they replace (
idx_inbox_chat_silent,idx_inbox_pending_notify,agentspk).Tests
splits per-trigger silent context inside one multi-chat batch claim— one batch claiming two chats with two same-chat triggers, pinning the in-batch cursor split the lateral now computes.ws-client-branch-fake.test.tsfake agent rows now carryuuid, since the set-based select keys rows by it.pnpm check,pnpm typecheck, and the full@first-tree/servervitest suite pass locally.