A rejected seed promise poisons a session into silent total row loss

[philcunliffe]

Addresses item A2 of #692 (the actionable, pre-existing defect on master). Item A1 is covered at the bottom.

Root cause

Two lines that only bite together, both pre-existing on master:

• scanCommittedMessageIds puts only the await storage.discoverCachePartitions(...) call inside its try/catch. The for (const part of partitions ?? []) walk over the answer sits outside it, and ?? [] covers only a nullish answer. A storage resolving a truthy non-iterable (a violation of its own declared CachePartitionMeta[] return type) therefore throws out of the function, whose JSDoc claimed it "NEVER throws".
• seedSeenMessagesForSession memoizes that promise in seedPromises and never removes it on rejection.

projectExchange awaits the seed, so the rejection is caught by source.js and the row is dropped. Because the memo is never rewritten, every later exchange for that session short-circuits onto the poisoned memo and is dropped with no warn at all. Net symptom: under a storage that violates the contract on every call, the daemon survives but drops every row for every session while going nearly silent (review round 2 on #690 measured five exchanges producing two warn lines and zero rows).

The fix, and why this shape

Enforce the guarantee where the whole seed path passes through rather than resting on a "never throws" contract each leaf asserts about itself:

pending = seedSessionIfCommitted(...).catch((err) => {
  log?.warn?.('aigw.seed_seen_messages_failed', { session_id: sessionId, error_kind: 'seed_rejected', error: ... })
  if (seedPromises.get(sessionId) === pending) seedPromises.delete(sessionId)
})

Three decisions worth naming:

1. Swallow, but only into "seeded nothing". This is the documented failure envelope of the seed itself (LLP 0204, and the function's own JSDoc): a seeding miss risks the duplicate the seed exists to prevent, which settlement/compaction still collapse, whereas a rejection costs a real row. Silently dropping the row is the one outcome the envelope forbids.
2. Degrade loudly, per exchange. The silence was half the defect, so the absorbed rejection warns on aigw.seed_seen_messages_failed with error_kind: 'seed_rejected' (matching the aigw.* warn conventions, and the error_kind discrimination introduced for the sibling scan in Committed-session index survives a throwing scan instead of wedging it #690). The existing discover-failure warn in the same function is tagged discover_failed so every line in the family names its kind rather than only some of them.
3. Drop the memo instead of caching the failure. Caching "could not seed" is right for a scan that ran and came back empty-handed (LLP 0204 caches to spare the daemon whole-table scans that succeed) and wrong for one that broke: nothing ever rewrites a memo entry, so a cached rejection is a per-session verdict, for the listener's lifetime, that no scan ever produced. Dropping it lets the next exchange retry and re-warn. The seedPromises.get(sessionId) === pending guard is so a concurrent caller's newer memo is not evicted by this one's failure (same posture as the index's built === attempt guard).

I deliberately did not copy #690's shape here. #690 puts a .catch on the index's scan promise because that promise has an orphan .then consumer whose rejection would be unhandled and kill the daemon. This call path has no orphan: the seed promise is awaited, so its failure mode is a lost row, not a dead process, and the fix belongs at the memo that turns one lost row into all of them.

I also chose not to guard the walk inside scanCommittedMessageIds on top of this. It would fix the one reported input while leaving the absolute contract claim still technically false (e.g. part.partition?.session_id at the top of the loop body is likewise outside the inner try), it would leave the general backstop untested, and it does not help the other way this seed path can reject (the committed-session index it consults first). One mechanism covering the whole path is both smaller and strictly more general.

The JSDoc contract

Requirement was not to leave code and doc contradicting each other. The scanCommittedMessageIds JSDoc no longer claims "NEVER throws": it now states plainly that only the discover call is guarded, that a storage breaking its own return type throws out of the walk, and that the guarantee callers actually need (seeding never costs a row) is enforced one level up in seedSeenMessagesForSession. seedSeenMessagesForSession's JSDoc gained the matching sentence saying it is where that guarantee lives.

Regression test

test/plugins/ai-gateway-message-projector.test.js -> seed failure: a storage that breaks its discover contract loses no rows and does not poison the session memo

It drives the real symptom, not just the throw: two exchanges for one session against a storage whose per-session discover answers with a truthy non-iterable every time, asserting per exchange that the row still lands, that discoverCachePartitions was re-called for the second exchange (direct evidence no failed memo survived), and that each failing exchange emits its own seed_rejected warn. The index build (discover call 1) is kept well-formed on purpose so the test isolates this defect from #685/#690's index-scan rejection and does not depend on that fix landing.

FAIL before (node --test test/plugins/ai-gateway-message-projector.test.js, fix reverted, test kept):

# Subtest: seed failure: a storage that breaks its discover contract loses no rows and does not poison the session memo
not ok 36 - seed failure: a storage that breaks its discover contract loses no rows and does not poison the session memo
  ---
  duration_ms: 0.807318
  type: 'test'
  location: '/tmp/tmp.XuuOI5rbl4/test/plugins/ai-gateway-message-projector.test.js:1003:1'
  failureType: 'testCodeFailure'
  error: |-
    exchange 1: a seed that could not run must not fail the projection
    + actual - expected

    + 'TypeError: (partitions ?? []) is not iterable'
    - undefined

  code: 'ERR_ASSERTION'
  name: 'AssertionError'
  actual: 'TypeError: (partitions ?? []) is not iterable'
  operator: 'strictEqual'
  ...
1..36
# tests 36
# pass 35
# fail 1

PASS after (same command, fix applied):

# Subtest: seed failure: a storage that breaks its discover contract loses no rows and does not poison the session memo
ok 36 - seed failure: a storage that breaks its discover contract loses no rows and does not poison the session memo
1..36
# tests 36
# pass 36
# fail 0

Checks

npm test:

1..3862
# tests 3864
# pass 3858
# fail 0
# skipped 6
# duration_ms 15832.543059

npm run typecheck: clean (tsc -p tsconfig.json --noEmit, no output).

On A1 (item 2 of #692): no code change, by design

A1 is a merge-sequencing note, not a defect in either PR. It records that #689 and #690 conflict on message_projector.js and the projector test file, and names the correct composed resolution for whoever merges second (keep #690's .catch normalizing the scan rejection, then layer #689's completion-stamping chain on top of the caught promise; never take #689's rebuild() wholesale). There is nothing to fix in the tree, so this PR makes no change for it.

Note for that merge: this PR also touches message_projector.js, in seedSeenMessagesForSession and in scanCommittedMessageIds's JSDoc and discover-failure warn. Those are disjoint from both #689's and #690's regions (rebuild() and scanCommittedSessionIds), so composing is additive; the test file addition is likewise appended well away from #690's insertion point.

Fixes #692

[philcunliffe]

Neutral review: PR #693 (head 2dcbbf6d) - APPROVE, no actionable findings

Reviewed in a detached worktree off origin/fix/issue-692. npm test 3858 pass / 0 fail / 6 skipped (3864 total), npm run typecheck clean. Diff is 2 files, no LLP edits.

---

Verdict 1: is the memo the right layer? Yes, and it is the only layer that closes the defect.

I enumerated the call sites rather than trusting the summary:

• scanCommittedMessageIds is module-private (absent from the export list at message_projector.js:6,30,142,358,556,627,1035,1041,1056) and has exactly one call site: seedSessionIfCommitted at message_projector.js:345.
• seedSessionIfCommitted in turn has exactly one call site: the memo at message_projector.js:314.

So fixing at the memo leaves zero other callers exposed. Both author claims check out against the code:

• "the walk is outside the try" - confirmed: message_projector.js:509 for (const part of partitions ?? []) sits after the catch that guards only the discoverCachePartitions call.
• "part.partition?.session_id is also outside the inner try" - confirmed: message_projector.js:517, between the loop head and the inner try at :519. This is the weaker of the two arguments (moving the whole loop inside the existing try would have covered it too), but it is factually accurate.
• The decisive argument is the other one, and it holds: seedSessionIfCommitted first awaits sessionIndex.mightHaveCommittedRows (message_projector.js:344), whose scanCommittedSessionIds carries the identical unguarded walk at message_projector.js:445. Guarding only :509 would leave the index path rejecting straight into the memo and reproducing Follow-up: deferred review findings from PR #690 #692 verbatim. Guarding the walk is a half-fix; the memo is the single choke point the whole seed path passes through.

Verdict 2: is the identity-guarded seedPromises.delete correct under concurrency? Yes. It cannot mis-delete, cannot leak, and does not cause a scan storm.

Interleavings I constructed:

• Closure identity. pending is a per-invocation let; the .catch callback closes over it, and the assignment at :314 completes synchronously before any microtask, so the guard at :322 compares against the exact derived promise stored at :324. Correct object, not the pre-catch one.
• Two exchanges racing the same session. seedSeenMessagesForSession runs get -> set with no await between them, so B always joins A's promise. The catch fires once, both callers resolve to undefined, both rows land. One warn for the pair rather than two - correct, since it was one scan.
• Delete landing after a newer promise was installed. Unreachable today: installing a newer promise requires get to return undefined, which requires the delete to have already run, and the catch body (warn then delete) is fully synchronous with no yield point. Grep confirms :170/:290/:322/:324 are the only touch points of seedPromises - nothing else mutates or evicts it (LLP 0204's follow-ups list projector-state eviction as not implemented). The guard is therefore correct dead-safe defence that makes a future eviction path safe rather than a live race fix. Keeping it is right.
• Re-scan storm. Quantified per reachable rejection source, and it does not materialise:
  • non-iterable discover result: throws immediately, before a single row read, so the retry costs one discoverCachePartitions (a partition listing) per exchange - not a whole-table scan;
  • index-path rejection: built at :376 is never cleared on rejection (the .then has only an onFulfilled), so every later exchange awaits the same already-rejected promise - cheap, no rebuild;
  • genuine I/O failures (failed discover, unreadable partition) are caught internally at :494 / :527 and resolve, so they still memoize and never retry at all.
    The only expensive case is a throwing part.partition getter mid-walk, which is exotic. LLP 0204's cost concern (item 3: whole-table scans per new session id) is untouched.

Verdict 3: is swallowing correct? Yes, verified against the doc, not the summary.

llp/0204-gateway-daemon-memory-leak.issue.md is Status: Draft (so no immutability constraint applies; the PR does not edit it anyway). Its "Fix" section states the seed scan's envelope directly: "a stale miss only risks the duplicate seeding guards against, which settlement/compaction still collapse (the seed scan's documented failure envelope)". That is precisely what the memo catch enforces. The @ref LLP 0204#fix anchor resolves to the ## Fix heading.

Verdict 4: does the test prove the symptom? Yes, and I verified it fails without the fix.

Reverting only message_projector.js to HEAD~1 and re-running: not ok 36, actual: 'TypeError: (partitions ?? []) is not iterable' at exchange 1. Restored and green.

• Deterministic. Stub-driven, no timers; exchange 2 short-circuits at if (ids.has(sessionId)) return true before any now() comparison, so SESSION_INDEX_REBUILD_MS never enters the asserted path. discoverCalls === 3 is exact (1 index build + 1 scan per exchange). assert.match(..., /not iterable/) pins only the version-stable half of the V8 message.
• It discriminates the delete, not just the catch. A catch-without-delete fix would still land both rows, but yields discoverCalls === 2 and warns.length === 1, failing both assertions. Good.
• The Committed-session-index self-clearing guard has no rejection handler: a rejecting scan would kill the daemon (deferred from #683) #685/Committed-session index survives a throwing scan instead of wedging it #690 isolation is legitimate, not a dodge. Committed-session index survives a throwing scan instead of wedging it #690 is still OPEN and edits the same file; keeping discover call 1 well-formed makes this test pass/fail solely on A2 and independent of merge order. The poisoned-memo symptom is still fully reproduced, through the per-session scan path that Follow-up: deferred review findings from PR #690 #692 A2 actually names.

Conventions: no semicolons in added code, zero U+2014 (grepped both files), JSDoc only, no @typedef, no inline import() types - the new test helpers resolve through the existing @import block at test/plugins/ai-gateway-message-projector.test.js:17-19. error_kind matches the documented structured-attribute vocabulary.

---

NITs (no change requested, nothing pushed)

1. message_projector.js:308-311 - the rationale says "one broken session logged once and then went quiet while every row for it was dropped." Pre-fix, source.js:261 emitted aigw.exchange_write_failed at error level for every dropped exchange, so it was not literally quiet; what was lost was the projector-level attribution and the rows. The re-warn justification is still sound (post-fix the projector warn becomes the only signal, since source.js no longer errors), but the sentence mis-describes the pre-fix state.
2. message_projector.js:315-323 - if log.warn itself throws (or String(err) throws on an exotic rejection value), the catch callback throws, pending rejects, and the memo retains a rejected promise: Follow-up: deferred review findings from PR #690 #692 reinstated exactly. Unreachable with the kernel logger and the TypeErrors this path actually produces. Hardening would be to delete before warning.
3. Merge-order note for Committed-session index survives a throwing scan instead of wedging it #690 - if Committed-session index survives a throwing scan instead of wedging it #690 makes the committed-session index self-clear on a rejecting scan, this PR's per-exchange memo retry would then trigger a per-exchange index rebuild (a session_id-column scan across all partitions) while storage stays broken. Today built is never cleared on rejection so this cannot happen; worth a look when Committed-session index survives a throwing scan instead of wedging it #690 lands.
4. Test - assert.equal(warn.fields.error_kind, 'seed_rejected') couples the test to the chosen layer: a future refactor moving the guard into the leaf would yield discover_failed and fail the test even though rows still land. Defensible (it asserts the operator-facing distinction the PR introduces), just worth knowing.

Fixed: nothing - no actionable findings, no commits pushed.

[philcunliffe]

Review - head 97d7e1f (merge of master into fix/issue-692)

Verdict: approve. No findings. This head existed only because a human merged five PRs to master, the PR went DIRTY, and a resolver merged origin/master in. The substance was reviewed before at 2dcbbf6d; what was unreviewed was the merge. It is clean, and the composition is strictly better than either parent.

Verified

The merge is purely additive, with no evil-merge edits. git diff-tree --cc 97d7e1fe lists exactly one file (test/plugins/ai-gateway-message-projector.test.js) and its combined diff contains zero removal lines. message_projector.js does not appear in the combined diff at all, which is positive evidence every hunk came verbatim from exactly one parent, i.e. a true auto-merge. No conflict markers anywhere.

The two source-side changes are genuinely disjoint. Master's projector diff touches only the SESSION_INDEX_REBUILD_MS JSDoc and rebuild()'s body; the branch's touches createAiGatewayMessageProjector, seedSeenMessagesForSession, the scanCommittedMessageIds JSDoc and the discover_failed field. No line overlap, and no shared mutable state: master's change touches only built/attempt.atMs, the branch's only seedPromises.

A clean git merge is not evidence the combination composes, so the specific hazards were probed, not assumed:

• No double-clearing of built - only rebuild()'s .then assigns built = undefined, guarded on built === attempt; the branch's code never touches built.
• No rebuild storm from a rejected seed - probed with a storage resolving a truthy non-iterable on every call: over 5 exchanges, discoverCalls: 1, 5 rows emitted, 5 seed_rejected warns. The branch's memo deletion drives a per-exchange seed retry that short-circuits on the cached index promise. Bounded.
• atMs and the session memo cannot disagree - atMs gates only mightHaveCommittedRows' trust-the-miss branch; the memo decides only whether seedSessionIfCommitted is re-entered.

The regression test still discriminates. Swapping master's message_projector.js into the merged tree makes exactly one test fail, with TypeError: (partitions ?? []) is not iterable at line 1107 - the signature recorded in the PR body. Master's own 37 tests still pass, so neither side's suite was weakened.

Ordering and hoisting are safe. Master's test sits immediately after restart replay: a throwing storage degrades to not-seeded, so its "The test above" comment still resolves; the branch's helpers are hoisted top-level function declarations. No duplicate titles or helper names; both projectors are constructed per-test.

Nothing else in the five merged PRs interacts. #658's source.js work leaves the projectExchange call site at source.js:255 untouched, so the branch's "a rejection here drops the row" premise still holds. Master's llp/0204 edit is editorial and inside ## Fix, so the branch's two @ref LLP 0204#fix annotations remain honest and their anchor still exists.

Gates. npm test 3889 pass / 0 fail / 6 skipped; npm run typecheck clean; smoke gateway_claude_capture ok. Test counts reconcile exactly: 3894 (master parent) + 1 branch test = 3895 = 3864 (branch parent) + the 31 tests the five master PRs add. Nothing dropped. No em dashes, no statement-terminating semicolons.

Worth knowing

The merge fixes an unhandled rejection the branch had on its own. The same probe against 2dcbbf6d's projector alone exits 1 with an unhandled rejection at exit, because the pre-#689 rebuild() used a floating attempt.ids.then(...) nobody awaited. Master's attempt.ids = scan.then(...) routes that derived promise into the awaited path. So: master alone drops all 5 rows, the branch alone keeps the rows but leaks an unhandled rejection, and merged keeps the rows with no unhandled rejection.

Pre-existing and correctly out of scope: when the index build's own scanCommittedSessionIds rejects, built is never cleared and the listener's committed-session index stays poisoned for its lifetime. It costs duplicates, never rows, exists identically on both parents, and is the separate defect PR #690 fixes. Nothing to change here.