perf(server): replace inbox context and route-validation N+1 queries - #2030
perf(server): replace inbox context and route-validation N+1 queries#2030bestony wants to merge 1 commit into
Conversation
Inbox delivery and bound-agent route validation both scaled their query count with input size, inside paths that hold locks or run on every heartbeat. collectPrecedingContext ran one previous-notify query per chat plus one silent-context query per trigger, all inside the claiming transaction — a 50-entry drain meant 50+ sequential round trips holding FOR UPDATE locks. It now issues two statements regardless of batch size: a CROSS JOIN LATERAL over a jsonb_to_recordset of per-trigger bounds that selects and locks the silent rows, then one keyed read of the message payloads through the typed builder. Each trigger's lower bound comes from the preceding trigger in the same batch, falling back via COALESCE to the table cursor for a chat's first trigger, so trigger-relative windows, the 24h window, the MAX_ENTRIES cap and SKIP LOCKED semantics are preserved. ensureAgentStillRoutedHere probed one agent per query, and both inbox:ack and every heartbeat validate the socket's whole bound set with no hard agent-count cap. Route resolution is now split into a pure decision step plus a batched ensureAgentsStillRoutedHere that reads all candidates in one inArray query; the single-agent helper delegates to it so the two paths cannot drift. The heartbeat repair pass still re-validates after recordClientHeartbeat — the restore can flip an agent back to active, and only the post-restore row decides whether repair is safe — but does so in one query over the restored set. Tests: a multi-chat multi-trigger batch drain pinning each trigger's window, a statement-count assertion that a 6-trigger drain costs the same as a 2-trigger drain, and a heartbeat test asserting one route query for two bound agents. All three were verified to fail against the pre-fix shapes. Refs #1724
yuezengwu
left a comment
There was a problem hiding this comment.
实现目标与核心改动:把 inbox preceding-context 组装从按 chat/trigger 串行查询改为固定两条语句,并把 ACK/heartbeat 的 bound-agent route 校验改为集合查询;单 agent 路径复用同一决策函数,避免语义漂移。
审查 head 5b856431d,未发现 blocker。LATERAL 查询保留了 per-chat/per-trigger 下界、24h/count cap、closest-first 截取后 oldest-first 输出,以及 FOR UPDATE ... SKIP LOCKED;route 批处理也保留了 active/client/runtime 与 runtime-switch claim 的原判定。
非阻塞建议:packages/server/src/services/inbox.ts 新增的 payload 去重注释仍把同一 message 出现在不同 chat 的原因归为 cross-chat replyTo routing,但 messages.ts 已明确 replyToInbox / replyToChat 是 decision-inert 且该路由已移除。去重本身安全,建议把注释改成 defensive dedupe,避免未来维护者误判当前行为。
另:当前 Check Branch Name 失败是因为 perf/ 不在允许前缀中;这不是本次 diff 的代码 blocker,但合并前仍需处理。
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: approve
- Rationale: The set-based implementation removes query-count growth while preserving trigger-relative Inbox context and the existing authoritative-route decisions.
Risk level: B-high
- Path baseline: only
packages/server/**-> B-low. - Semantic lift:
ws-client.tschanges core agent-route validation flow -> B-high.
PR summary
- Author / repo: bestony / agent-team-foundation/first-tree
- Problem: Large Inbox drains and multi-agent Client heartbeats made database round trips grow with trigger or bound-agent count, increasing lock duration and steady-state heartbeat load.
- Approach: Assemble all preceding-context windows with one LATERAL statement plus one typed payload read, and validate bound-agent routes with one set query shared by single-agent and batch callers. Existing 24-hour/count caps, per-chat cursor boundaries, oldest-first output, ACK/recovery custody,
SKIP LOCKED, and runtime-switch route rules remain unchanged. - Impacted modules:
packages/server/src/services/inbox.ts,packages/server/src/api/agent/ws-client.ts, and their server regression tests.
Review findings
- ✅ The multi-chat cursor fallback, within-batch chaining, recency/count cap, output order, and lock semantics remain aligned with the current Inbox delivery contract.
- ✅ ACK, heartbeat routed-set collection, and heartbeat repair now reuse one authoritative route-decision path without changing active/client/runtime/switch-claim outcomes.
⚠️ Non-blocking: the inline runtime-switch route row inws-client-branch-fake.test.tslacks theuuidselected by the batched query, so that test no longer reaches the claimed-switch branch it names. Adduuidand an observable retained-binding assertion in follow-up.
Action taken
- Approved exact head
5b856431d179b8e429b36708a7a0c94842e9cabawith explicit maintainer authorization.
Closes #1724 (PERF-061).
Problem
Two delivery-path helpers scaled their query count with input size, in places that either hold locks or run on every heartbeat.
collectPrecedingContext(packages/server/src/services/inbox.ts) issued one previous-notify query per chat plus one silent-context query per trigger — all inside the claiming transaction. A 50-entry drain meant 50+ sequential round trips while holdingFOR UPDATElocks, so both DB latency and lock duration grew with batch size.ensureAgentStillRoutedHere(packages/server/src/api/agent/ws-client.ts) probed a single agent per query, butinbox:ackand everyheartbeatre-validate the socket's entire bound-agent set, and a client has no hard agent-count cap.Changes
Inbox preceding-context assembly
Now two statements regardless of batch size:
CROSS JOIN LATERALover ajsonb_to_recordsetof per-trigger bounds that selects and locks the silent rows.Each trigger's lower bound is the preceding trigger in the same batch/chat, computed in JS; a chat's first trigger falls back via
COALESCEto a correlatedmax(id)lookup, so the whole cursor chain resolves in one statement.Preserved exactly: trigger-relative windows, the 24h window, the
PRECEDING_CONTEXT_MAX_ENTRIEScap keeping the rows closest to the trigger, oldest-first output, andFOR UPDATE OF inbox_entries SKIP LOCKED.Two things worth flagging for review:
FOR UPDATE ... SKIP LOCKEDinside a LATERAL is legal only becauseCROSS JOINis an inner join — aLEFT JOIN LATERALwould be rejected as locking the nullable side of an outer join. Verified against PostgreSQL 17 with a concurrent lock-holding session: locked rows are skipped exactly as before.db.executebypasses the postgres-js type parsers and returns raw strings for every column, includingtimestamptzandjsonb(this is why the neighbouringclaimBacklogForPushFairtypes its ids asnumber | string). Rather than hand-parse jsonb and format timestamps in SQL, the raw statement returns only ids and the payload mapping stays in the typed builder. Still O(1), and the second query is a primary-keyIN (...).Bound-agent route validation
Route resolution is split into a pure decision step (
resolveAuthoritativeRoute) plus a batchedensureAgentsStillRoutedHerethat reads all candidates in oneinArrayquery. The single-agentensureAgentStillRoutedHeredelegates to the batched one, so the two paths cannot drift.Applied at all three N+1 sites:
inbox:ack, the heartbeat routed-agent collection, and the heartbeat repair pass.Deliberate non-optimization: the repair pass still re-validates instead of reusing the routed set computed earlier in the same heartbeat.
recordClientHeartbeatcan flip a suspended agent back to active, and only the post-restore row decides whether backlog repair is safe. It is now one query over the restored ∩ bound set rather than one per agent.Tests
Three new regression tests, each verified to fail against the pre-fix shape before being kept:
bundles trigger-relative context for every trigger of a multi-chat batch drainkeeps drain statement count flat as the trigger batch growsdebughookre-validates every bound agent's route in one query per heartbeatThe
ws-client-branch-fakefixture gained auuidfield (same column as its existingid, under the alias the batched query selects) and a projection-routed fake db, because the positional queue can't serve a multi-agent socket — the detached post-bind drain interleaves with the next bind lookup.Verification
packages/server: 245 test files, 2680 tests, all passingpnpm check— clean (exit 0)packages/serverpnpm typecheck— cleanRepo-wide
pnpm typecheckfails in@first-tree/webon a missingqrcode.reactinstall — pre-existing, unrelated, and this branch touches no files underpackages/web.