fix(server): scan only the non-acked delta when committing inbox ACKs

[bestony]

Closes #1671 (PERF-008).

Problem

ackThroughEntryIdForBoundAgents selected — and, under FOR UPDATE, locked — every notify=true row in the (inbox_id, chat_id) partition up to the ACK cursor, including rows acked long ago, then materialized them in JavaScript.

Two things made that expensive, and only one of them is in the issue text:

1. Acked rows were pure waste. They can never change the outcome (see the equivalence argument below).
2. No index matched the query at all. The planner fell back to the primary key and walked the whole id <= cursor range with inbox_id / chat_id / notify degraded to filters. On a 150k-row history that is 150,001 locked rows and 304,921 buffer hits per ACK:

LockRows (actual rows=150001)  Buffers: shared hit=304921
  ->  Index Scan using inbox_entries_pkey
        Index Cond: (id <= 300001)
        Filter: (notify AND inbox_id = ... AND chat_id = ...)
        Rows Removed by Filter: 150000

Per-ACK cost was O(history), lifetime cost O(N²), and a duplicate ACK that commits nothing paid full price.

Why excluding acked rows is exact, not approximate

prefixRows is consumed in exactly three places, and an acked row contributes nothing to any of them:

Consumer	Expression	Value for an acked row
gap check	status !== 'acked' && …	first term false → never a gap
deliveredIds	status === 'delivered'	always false
resetPendingIds	status === 'pending' && deliveredAt !== null	always false

So pushing status <> 'acked' into SQL is a row-by-row equivalence. It holds only because acked is terminal: recoverUnackedForScope and resetDeliveredForInboxes reset delivered rows, and pruneStaleSilentEntries only deletes notify=false rows — nothing moves a row back out of acked. That premise is written into the code comments so a future change to recovery can't quietly invalidate it.

The clause is an exclusion, not an allow-list — and that is load-bearing

ck_inbox_entries_status is NOT VALID, so a legacy row can still carry a status outside today's enum. status IN ('pending', 'delivered') would select the same rows today and match the same partial index, but it is an allow-list: it drops such a row out of the prefix entirely, so the gap check never sees it and the commit silently steps over it. Excluding only acked keeps every unexpected status visible to the gap check, which is where it has to be handled. There is a regression test pinning this, and it was verified in both directions — swapping the clause to the allow-list form makes that case return ok: true instead of prefix_gap.

The gap check itself keeps its original status !== "acked" && … shape, but purely as defensive redundancy: that term is unreachable now that the SQL excludes acked rows, and removing it changes no test outcome. The comments say so rather than crediting it with protection it does not provide.

Why the predicate alone is not enough

Adding the clause without an index is a fake optimization — the scan volume does not change, only the locking:

Bitmap Heap Scan   Filter: ((status <> 'acked') AND (id <= 300001))
  Rows Removed by Filter: 150499        <-- still O(history)

So this change ships the clause and a matching partial index:

CREATE INDEX idx_inbox_unacked_cursor
  ON inbox_entries (inbox_id, chat_id, notify, id)
  WHERE status <> 'acked';

Partial on status <> 'acked' because the non-acked set is the live in-flight window: the index stays sized to concurrent traffic rather than to history. Measured 48 kB on a 2.7M-row table whose comparable full index (idx_inbox_chat_silent) is 26 MB.

The literal is load-bearing, not stylistic

A partial index applies only when the planner can prove the query implies its predicate, and a bound parameter in a cached (named) statement proves nothing — the scan then filters the whole partition.

That does not happen on today's path, and the PR does not claim it does. Drizzle issues every query through postgres-js unsafe(), which hardcodes prepare: false and receives no options, so these statements are unnamed: one-shot plans, always planned as custom, parameter substituted before planning. Measured through the real driver (auto_explain, 60k-row history) on PostgreSQL 16.14 and 17, the bound-parameter spelling kept using idx_inbox_unacked_cursor on all 41 logged scans — including under plan_cache_mode = force_generic_plan, which only governs cached statements. There is also no named-statement path in packages/server/src today (no .prepare( outside tests).

So the literal is insurance, not a fix for an observed regression: it costs one inlined constant and stays correct if this query ever becomes a named statement — a Drizzle .prepare(), a driver change, or a pooler that prepares. Under that hypothetical the two spellings diverge cleanly, which is what the regression guard pins:

Clause form	Drizzle spelling	Generic-plan result
status <> 'acked'	sql`… <> 'acked'`	Index Scan using idx_inbox_unacked_cursor, no sort
status <> $n	ne(status, "acked")	falls back to idx_inbox_chat_silent, Rows Removed by Filter: entire history

Writing this the idiomatic Drizzle way would have made the optimization disappear in production while every benchmark that passes constants still looked fast. For the same reason notify sits in the index key rather than in the predicate: the service passes notify as a parameter, so a WHERE notify = true predicate would stop matching too. Keeping it in the key means the index depends on exactly one inlined literal — and that literal is the fix itself, so it is self-documenting rather than an easily-"normalized" pipeline detail.

Verified on both server versions this repo runs, since CI and self-host differ: .github/workflows/ci.yml and the test harness use postgres:17, while docker-compose.yml ships postgres:16-alpine. Replaying the exact service shape (notify and the cursor as bind parameters, status as a literal) under force_generic_plan:

	PostgreSQL 17	PostgreSQL 16.14
literal status <> 'acked'	Index Scan on idx_inbox_unacked_cursor, all four key columns in the index condition	same, 2 buffers, 0.026 ms
status <> $n (bind parameter)	whole-partition scan, Rows Removed by Filter = the entire chat history	same

Same conclusion on both. Which index the degraded plan falls back to is statistics-dependent, not a version property — on one PG 16.14 instance, changing only the dataset composition moved it between the primary key and idx_inbox_chat_silent. The invariant across every variation is that the whole partition is scanned; that is the part worth relying on. A green CI run on 17 alone would not have established any of this, since a self-hosted deployment runs 16.

The chat_id IS NULL partition compiles to a different statement and so plans separately. It uses the same index (chat_id IS NULL enters the index condition) but adds one bounded sort, because PostgreSQL does not derive path ordering through a NullTest. That sort is over the delta, not the history, and LockRows still sits above it, so the ascending FOR UPDATE lock order is unchanged.

Also fixed: ackedEntryIds ordering

committableIds was [...deliveredIds, ...resetPendingIds] — two concatenated filters, so not ascending — and the returned order relied on UPDATE … RETURNING's undefined row order happening to come back sorted. ws-client.ts consumes ackedEntryIds as an ascending cursor list. It is now built in one ascending pass and explicitly sorted. Pinned by a test that interleaves delivered and recovery-reset rows.

Results

PostgreSQL 17, 150k notify-row history, median of 7, timing the whole ACK transaction (including the untouched silent drain):

Shape	Before	After	Rows scanned
long history + one incremental ACK	70.8 ms	0.068 ms	150001 → 1
duplicate ACK (issue names this)	78.1 ms	0.010 ms	150000 → 0
500-row backlog committed at once	80.1 ms	4.34 ms	150500 → 500

Buffers for the incremental shape: shared hit=304921 → shared hit=4.

The backlog case improves least, which is correct — those 500 rows are real work that should not be optimized away.

Write amplification (same table size, drop/create A/B, 50k-row bulk writes): roughly +10%, about 1 µs/row, on a table that already carries six indexes. Run-to-run noise on this microbenchmark is ±15%, so treat it as an order of magnitude rather than a precise figure.

Migration

Generated with drizzle-kit generate; only the comment header was added by hand, following the existing format in 0025_inbox_silent_entries.sql and 0064_superb_betty_brant.sql (both index additions to this same table).

CREATE INDEX holds a SHARE lock that blocks writes to inbox_entries — including message fan-out — for one heap scan: 30 ms at 400k rows, 73 ms at 2.7M rows. Those are warm-cache numbers; a cold table on slower storage will be higher, since the cost is the heap scan rather than the index writes. The migration carries an operator note with the CREATE INDEX CONCURRENTLY escape hatch (Drizzle runs migrations in a transaction, so CONCURRENTLY cannot go in the file itself; IF NOT EXISTS lets a pre-created index be skipped).

Tests

packages/server — 254 files / 2856 tests pass. New coverage:

• inbox-ack-scaling.test.ts
  • the non-acked clause still compiles to zero bind parameters — the only guard that catches someone changing the service back to ne(...);
  • a plan guard under forced generic plan asserting the scan touches < 50 rows against a 3000-row history and uses idx_inbox_unacked_cursor — it has to be generic-plan, because under a custom plan every spelling looks fine and the guard would be decorative;
  • an ACK completes while another transaction holds a row lock on already-acked history — the pre-fix scan would have blocked there, which is the lock-contention half of the issue;
  • functional correctness and duplicate-ACK no-op behind a long acked prefix.
• inbox-delivery-indexes.test.ts — pins the index key order and the predicate spelling.
• inbox-ws-push.test.ts — the 11 existing ACK cases are unchanged and serve as the equivalence acceptance set; added the interleaved-ordering case and the legacy out-of-enum status case.

Both new guards were verified to actually fail when the fix is reverted (ne(...) → literal guard fails; parameterized predicate → plan guard reports 3001 rows touched).

Known boundary, recorded in the test rather than left implicit: the plan guard exercises an equivalent statement, not the query object the service builds. Together with the literal guard it pins the clause and the index contract, but a future restructuring of the service's own WHERE could stop matching the index without either guard noticing. Closing that would mean exporting the query builder purely for the test, which was judged not worth the coupling.

Scope

Only the ACK path changed. pollInbox, claimAndBuildForPush, claimBacklogForPushFair, collectPrecedingContext, drainPendingSilentRows, recoverUnackedForScope, resetDeliveredForInboxes and pruneStaleSilentEntries are untouched.

QA: this touches the WS/inbox path, so per AGENTS.md formal QA is warranted. The matching existing case is packages/qa/cases/cross-surface/authenticated-ws-inbox-delivery.md; no new case was added because the behavior contract is unchanged.

Note on unrelated CI: packages/web has 117 failing tests on this branch. They fail identically on a clean origin/main@830de274a checkout — pre-existing and unrelated to this change, which touches packages/server only (and packages/web has no dependency path to @first-tree/server).

One full server run also showed github-entity-follow and me-chat-pin-activity failing together, and me-chat-pin-activity never touches inbox_entries at all, so this change cannot reach it. Chasing it down found a pre-existing defect worth reporting separately: both tests compare a timestamp written by PostgreSQL (defaultNow() on github_entity_chat_mappings.bound_at / chats.activity_at) against one written by Node (new Date() in github-entity-follow.ts, the GREATEST(...) argument in chat.ts). Both therefore encode the same inequality — PostgreSQL's clock must not run ahead of Node's — which is why they fail as a pair rather than independently.

Sampling that skew across a full suite run (892 tight samples) put it within ±1 ms, far too small to cross either assertion, so routine drift does not explain the original failure and something coarser was involved. Not reproduced since: 13 subsequent full-suite/targeted runs green on this branch, 4/4 on origin/main. Recorded rather than silently re-run; the cross-clock comparison itself is follow-up material, not something this PR touches.

Suggested follow-up (not in this PR): the same partial-index-vs-bound-parameter mismatch likely affects idx_inbox_pending_notify, whose predicate is status = 'pending' AND notify = true while pollInbox passes both as parameters. Worth one issue covering the class rather than fixing it opportunistically here.