fix(server): make inbox ACK cost independent of chat history

[bestony]

Closes #1671 (PERF-008).

Problem

ackThroughEntryIdForBoundAgents treats entryId as a cursor, so it has to verify the notify=true prefix below it in the same (inbox_id, chat_id) partition. It did that by selecting and FOR UPDATE-locking every such row from the beginning of the chat, SELECT *, then filtering in JavaScript.

Rows already acked are terminal: they cannot open a prefix gap, cannot be committed a second time, and no other code path ever writes them. Every one of them was pure waste. The result was O(history) per ACK, O(N²) over a chat's lifetime, and lock contention spread across rows nothing could act on — with duplicate ACKs paying the full scan again.

Change

1. Delta scan (packages/server/src/services/inbox.ts). The prefix scan now selects only uncommitted rows and only the three columns the commit decision reads:

WHERE inbox_id = $1 AND chat_id = $2
  AND notify = true AND status <> 'acked' AND id <= $3
ORDER BY id FOR UPDATE

Commit semantics are byte-for-byte unchanged. The gap check already skipped acked rows, deliveredIds/resetPendingIds could never contain one, and already_acked still falls out of an empty committable set. Concurrency is preserved too: two ACKs whose cursors overlap still contend on the same uncommitted rows, and under READ COMMITTED a waiter re-evaluating FOR UPDATE against a row the winner just acked drops it from the result — the same already_acked outcome the old code reached by filtering it in JS.

2. Compact ACK ledger (migration 0091). A partial index over (inbox_id, chat_id, id) WHERE notify = true AND status <> 'acked'. It contains only in-flight rows, so it stays small however long a chat runs; id last bounds the scan by the cursor and returns it pre-ordered. A duplicate ACK probes zero tuples.

3. Literal predicates — the non-obvious part. notify and status are compared against SQL literals rather than bound parameters. PostgreSQL only applies a partial index when it can prove the query clauses imply the index predicate, and that proof operates on constants; written as notify = $1 it fails for a generic plan and the scan silently degrades back to walking the whole history. The comment on uncommittedNotifyPrefixWhere, the migration header, and a test all say so.

4. Batched silent-row GC. pruneStaleSilentEntries is the counterweight to ACK-through — ACK's silent drain is bounded by a chat's pending silent rows, so a GC that falls behind is what lets that set grow. It deleted an unbounded number of rows per statement and materialized every deleted id in JS to count them. Now it drains in 2 000-row batches with a loop cap that defers the remainder to the next background tick.

Design note: why not a cursor table

The issue suggested "a per-inbox/chat committed high-water cursor or a compact ACK ledger". I took the ledger. A cursor row would add a write and a serialization point per ACK, and short-circuiting on entryId <= acked_through is unsafe against sequence-visibility gaps (a lower id committing after a higher one was acked would be stranded in delivered forever). The partial index gets the same O(delta) behavior with zero new state and zero semantic change.

Verification

• pnpm check && pnpm typecheck clean; 2 854 server tests pass, including all 22 existing ACK/WS data-plane behavior tests unchanged.
• New inbox-ack-scan-cost.test.ts runs EXPLAIN (ANALYZE) over the service's own exported predicate (no hand-copied lookalike that can drift) and counts rows the plan examined — emitted plus filter-discarded, so the metric is plan-shape independent:
  • at 1 500 rows of acked history, ≤ 8 rows examined, via idx_inbox_ack_prefix;
  • examined rows at depth 1 500 ≤ examined rows at depth 1 (flat, not proportional);
  • commit + idempotence over deep history — only the in-flight row is committed, so the history's ids do not leak into the caller's WS in-flight bookkeeping;
  • a prefix gap buried under deep history is still rejected (dropping acked rows must not drop uncommitted ones).
• inbox-delivery-indexes.test.ts pins the index predicate that the literal-spelling contract depends on.

Operator note

The migration is a plain CREATE INDEX, which takes a SHARE lock blocking writes to inbox_entries for its duration — fine on a small table, an outage on a large one. Drizzle wraps migrations in a transaction so CONCURRENTLY cannot go in the file. The header carries the same pre-create runbook as 0025_inbox_silent_entries.sql, and the statement uses IF NOT EXISTS so a concurrently pre-created index is detected and skipped.

QA

Touches the WS/inbox path, so formal QA is warranted. The matching case is packages/qa/cases/cross-surface/authenticated-ws-inbox-delivery.md. No new case added: the cost and commit contracts here are deterministic and now live in product tests, which is where the QA package says stable behavior belongs.

[baixiaohang]

Recommendation: approve

• Rationale: The ACK path now scales with uncommitted delivery depth while preserving the existing prefix, recovery, idempotence, and silent-context commit boundaries.

Risk level: B-high

• Path baseline: packages/server/** with a new partial database index -> B-high
• Semantic lift: none

PR summary

• Author / repo: bestony / agent-team-foundation/first-tree
• Problem: Long-running chats made every Inbox ACK scan and lock the entire acknowledged history, causing quadratic lifetime work and unnecessary contention for agents closing turns or retrying ACKs.
• Approach: Restrict the locked prefix scan to uncommitted notify rows, back it with a predicate-matched partial index, and bound silent-row GC work by batch and loop caps.
• Impacted modules: Inbox service, inbox schema/migration, ACK cost and index regression tests.

Review findings

• ✅ Excluding terminal acked rows still leaves every uncommitted pending gap visible, including recovery-reset rows distinguished by deliveredAt.
• ✅ ACK expansion remains scoped to the same inbox, chat, and cursor, so later silent context is not consumed early.
• ✅ The service predicate and partial-index predicate use matching SQL literals, with regression coverage for both scan cost and the installed index definition.
• ✅ The migration documents the required concurrent pre-create runbook for large tables where a plain CREATE INDEX would block writes.

Action taken

• Approved exact head cabdd2df7b529197b38c5784ccc33c88fc60016b.

[yuezengwu]

Reviewed head cabdd2df7b52.

The goal is sound: replace the ACK-through whole-history lock/scan with an uncommitted-prefix query backed by a matching partial index, narrow the selected columns, and bound silent-row GC work. The literal partial-index predicate and the READ COMMITTED row-lock recheck preserve the intended query/concurrency shape.

Blocker — the scan-cost test does not reproduce the physical index history created in production (packages/server/src/__tests__/inbox-ack-scan-cost.test.ts:73-92, :132-170). The fixture inserts old rows directly with status = 'acked', so those rows never enter idx_inbox_ack_prefix. Production rows enter that index while pending/delivered and are later updated to acked; PostgreSQL retains obsolete index tuples until VACUUM/index cleanup. Actual Rows + Rows Removed by Filter does not count invisible/dead index tuples visited by the index scan, so these assertions can stay flat even if the first duplicate ACK still walks a large unvacuumed tail. That leaves PERF-008's central guarantee—and the “zero index tuples” / O(uncommitted) comments—unverified.

Please seed history through the real state transition (insert as delivered, then update to acked), measure physical work before VACUUM (for example EXPLAIN (ANALYZE, BUFFERS, FORMAT JSON) buffer/page work rather than only returned/filter rows), and compare shallow vs deep history. If that grows, the implementation also needs an explicit maintenance/design bound; if it stays flat, the corrected regression test will prove the actual workload.

Human/operator check: migration 0091 uses plain CREATE INDEX, so the documented concurrent pre-create runbook must be used for any large production inbox_entries table to avoid blocking writes. The PR is still draft, so I am leaving a review comment rather than approving. I did not rerun tests/QA; the current GitHub checks are green.

[baixiaohang]

Recommendation: request changes

• Rationale: New review evidence shows that the cost fixture never creates the dead partial-index tuples produced by the real delivered -> acked transition, so the central history-independent cost guarantee is not yet demonstrated.

Risk level: B-high

• Path baseline: packages/server/** with a new partial database index -> B-high
• Semantic lift: none

PR summary

• Author / repo: bestony / agent-team-foundation/first-tree
• Problem: Long-running chats currently make ACK work grow with acknowledged history.
• Approach: Scan only logically uncommitted notify rows through a matching partial index and bound silent-row GC.
• Impacted modules: Inbox ACK service, inbox index migration/schema, cost regression tests.

Review findings

• ❌ 1. The fixture inserts historical rows directly as acked, so they never enter idx_inbox_ack_prefix. The production path inserts them as pending/delivered and later updates them to acked; under PostgreSQL MVCC, obsolete B-tree index tuples can remain until deferred index cleanup/VACUUM. The current Actual Rows + Rows Removed by Filter metric counts executor output/filter rejection, not those invisible physical tuples, so a flat result can coexist with history-proportional page/tuple traversal. PostgreSQL 16 documents both deferred B-tree garbage cleanup and that EXPLAIN's rows is output rather than total scanned work: https://www.postgresql.org/docs/16/btree-implementation.html#BTREE-IMPLEMENTATION and https://www.postgresql.org/docs/16/using-explain.html. Seed history through the real state transition, measure physical work before VACUUM (for example buffer/page work), and compare shallow versus deep histories. If it grows, the implementation needs an explicit design or maintenance bound rather than the current unconditional O(uncommitted) / zero-index-tuple claims. [packages/server/src/__tests__/inbox-ack-scan-cost.test.ts:73]
• ✅ 2. The logical predicate, prefix-gap behavior, and READ COMMITTED row-lock recheck remain sound; this blocker is specifically about the physical cost guarantee the PR exists to establish.

Action taken

• Submitted request changes on exact head cabdd2df7b529197b38c5784ccc33c88fc60016b, superseding my earlier approval after the new physical-index-history evidence.