Committed-session index survives a throwing scan instead of wedging it

[philcunliffe]

What was wrong

hypaware-core/plugins-workspace/ai-gateway/src/message_projector.js, createCommittedSessionIndex().rebuild():

attempt.ids.then((ids) => {
  if (ids === undefined && built === attempt) built = undefined
})

The self-clearing guard attached a fulfillment-only handler to the scan promise. Nothing awaits the derived promise, so if scanCommittedSessionIds ever rejected, that derived promise was an unhandled rejection and Node terminated the daemon.

This is the one escape hatch out of the projector that reaches Node's default handler: source.js onExchangeFinished wraps projectExchange in try/catch, so a rejection travelling the normal awaited path is absorbed and logged. Only the orphan derived promise is fatal. That makes it strictly worse than the pre-#683 behaviour, where the scan promise always had an awaiter, and it sits in the shutdown-safety path LLP 0204 exists to harden.

Secondary consequence of the same defect: because the fulfillment handler never ran on a rejection, built was left pointing at the rejected attempt, so every subsequent mightHaveCommittedRows re-awaited the same rejected promise for the listener's lifetime instead of retrying: a permanently wedged index, not a degraded one.

Root cause

scanCommittedSessionIds signals "the scan could not run" by resolving to undefined, never by rejecting (both awaits sit inside try/catch), and the guard was written to assume that contract rather than enforce it. Nothing structural held it.

The fix

rebuild() normalizes a rejecting scan into the same undefined "could not scan" outcome the scan already reports for a failed partition discovery (logging the same aigw.session_index_scan_failed warn):

const ids = scanCommittedSessionIds(storage, log).catch((err) => {
  log?.warn?.('aigw.session_index_scan_failed', {
    error: err instanceof Error ? err.message : String(err),
  })
  return undefined
})
const attempt = { atMs: now(), ids }

One .catch at the single point where the promise is created makes every consumer total, not just the guard:

• the guard's derived promise can no longer reject, so it can no longer be unhandled;
• the existing ids === undefined branch fires, so built self-clears and the next caller retries;
• mightHaveCommittedRows's await current.ids yields undefined and returns true, so the index degrades to "err toward scanning" and the row is still emitted.

Adding only attempt.ids.then(handler, () => {}) (the two-character fix suggested in the issue) would silence the crash but leave the index wedged on the rejected attempt and still lose the exchange. This is the same size of change and satisfies the issue's own gate.

scanCommittedSessionIds's JSDoc now states the non-rejecting contract explicitly, as the issue suggested, and the normalization carries // @ref LLP 0204#fix [constrained-by]: the seed index is documented best-effort, so a scan it cannot complete must degrade the index, never end the process. No LLP text was edited (nothing it decided has changed).

Regression test

test/plugins/ai-gateway-message-projector.test.js
committed-session index: a scan that throws degrades the index instead of killing the process

Driven by a storage whose first discoverCachePartitions (the index build) answers with a non-iterable, so the for (const part of partitions ?? []) loop throws from outside scanCommittedSessionIds's try/catch and the scan promise genuinely rejects. Later calls are well-formed, so the per-session fallback scan is unaffected and the "err toward scanning" degradation is observable on its own. The test installs a process.on('unhandledRejection') collector, drains the loop, and asserts it is empty, plus asserts the row is still emitted and the fallback scan ran.

FAIL before the fix

git stash on message_projector.js only, test unchanged:

not ok 1 - committed-session index: a scan that throws degrades the index instead of killing the process
  error: '(partitions ?? []) is not iterable'
  code: 'ERR_TEST_FAILURE'
  name: 'TypeError'
  stack: |-
    scanCommittedSessionIds (.../ai-gateway/src/message_projector.js:411:36)
# Error: Test "committed-session index: a scan that throws degrades the index instead of killing the process" ... generated asynchronous activity after the test ended. This activity created the error "TypeError: (partitions ?? []) is not iterable" and would have caused the test to fail, but instead triggered an unhandledRejection event.
# pass 0
# fail 1

The runner names the defect directly: triggered an unhandledRejection event.

Standalone confirmation outside the test runner, with projectExchange caught exactly as source.js catches it (so the only escaping rejection is the orphan), on unfixed code:

projectExchange rejected (caught, as source.js does): (partitions ?? []) is not iterable
.../message_projector.js:411
  for (const part of partitions ?? []) {
TypeError: (partitions ?? []) is not iterable
Node.js v22.23.1
EXIT=1

The same script on fixed code:

projectExchange rejected (caught, as source.js does): ...
survived
EXIT=0

(the process no longer dies; and inside the test, where the fallback scan is left working, the exchange's row survives too).

PASS after the fix

ok 1 - committed-session index: a scan that throws degrades the index instead of killing the process
# pass 1
# fail 0

Full suite

npm test        -> 1..3862  # pass 3858  # fail 0  # skipped 6   (exit 0)
npm run typecheck -> clean, exit 0

🤖 Generated with Claude Code

Fixes #685

---

Update at head 045abc33 (merge of origin/master): what this PR fixes has shifted

Master moved under this PR (5 PRs, b28aae3 -> 3e8f05a), and origin/master was merged into the branch to resolve the conflict with #689. That changes the rationale above in two ways:

• The daemon-kill described under "What was wrong" is no longer reachable on master. Committed-session index stamps atMs at scan start, so a slow scan never serves a cache hit #689 replaced the floating fulfilment-only .then with a chain (attempt.ids = scan.then(...)), which incidentally gives a rejecting scan an awaiter: the rejection propagates through mightHaveCommittedRows into projectExchange, which source.js catches. Verified on the merged-but-unfixed tree: a rejecting index scan produces zero unhandled rejections, exit 0.
• The "secondary consequence" above is now the defect this PR fixes, and on master it is worse than described: a permanent, silent wedge. Without the .catch, the rejected attempt stays published in built, the stamp-and-self-clear handler runs on fulfilment only, and atMs stays Infinity (never stale), so the failed attempt never clears and never ages out. Verified on the merged-but-unfixed tree: 3 sequential exchanges all rejected, discoverCalls stuck at 1, zero rows emitted, zero warns. With the fix: every row emitted, the index self-clears and rebuilds, one error_kind: 'scan_rejected' warn.

The root cause and the fix are unchanged: scanCommittedSessionIds can reject while nothing in rebuild() tolerates a rejection, and one .catch where the promise is created makes every consumer total. Only the surviving symptom changed, from "kills the daemon" to "silently loses every subsequent exchange on the listener".

The regression test is renamed accordingly (committed-session index: a scan that throws degrades the index instead of wedging it) and now pins both shapes: a second exchange asserts the failed attempt clears itself (fails unfixed: the awaited rejection throws before it), and the unhandledRejection collector assertion keeps the crash shape closed, since nothing structural on master holds #689's chaining in place.

The merge resolution is the composed form both review rounds prescribed: the .catch normalization wraps the scan promise that master's completion-stamping chain is built on, so #689's stamp-at-completion and shared in-flight attempt are untouched and the chained handler only ever sees a fulfilled value. At this head: 3895 tests, 3889 pass, 0 fail, 6 skipped; typecheck clean.

[philcunliffe]

Neutral review round - PR #690 @ 8ae05e2

Verdict: the fix is correct and the regression test is genuine. No defect found in the change itself. One actionable cross-PR finding (merge coordination with #689) is recorded below; it is not fixable inside this PR and I made no code changes.

Reviewed in a detached worktree off origin/fix/issue-685 (merge-base with origin/master is d3b3ea3, i.e. the branch is current). I re-ran everything rather than trusting the PR body.

---

Verified: the premise is real, not theoretical

source.js:228-229 does wrap projectExchange in try/catch, so an awaited rejection is absorbed. The floating attempt.ids.then(...) really is the one escape hatch. Reproduced in bare Node against the pre-fix file at its real path, catching projectExchange exactly as source.js does:

pre-fix:  projectExchange rejected (caught as source.js does): (partitions ?? []) is not iterable
          TypeError ... at scanCommittedSessionIds (message_projector.js:411:36)
          Node.js v22.23.1      EXIT=1     <- process dies
post-fix: rows 1 / SURVIVED                 EXIT=0

Verified: error-handling semantics

• Every consumer is total. scanCommittedSessionIds is called in exactly one place (message_projector.js:352), and the .catch is attached at that call. built is only ever assigned an attempt whose .ids is the caught promise, so all three consumers (mightHaveCommittedRows's await current.ids, its second-chance await next.ids, and the self-clearing guard) hold a derived promise that cannot reject. No orphan path remains.
• The warn genuinely fires on the rejection path - not swallowed. Driven with a real log stub through projectExchange:

  warns: [["aigw.session_index_scan_failed", {"error":"(partitions ?? []) is not iterable"}]]
  

  It is emitted exactly once (the in-scan catch at :426 and the new .catch at :353 are mutually exclusive outcomes, so no double-warn).
• built really self-clears; no poisoned cache, no storm. Same probe, second exchange after the failed build: discoverCallsdelta = 1, and that rebuild succeeded and served the answer. The index self-heals on the very next caller. The .then guard is registered synchronously in rebuild(), before any mightHaveCommittedRows awaiter attaches, so built = undefined is always ordered before the awaiter resumes. Retry-per-miss on a persistently failing scan is the pre-existing shape of the undefined path shipped by The gateway daemon leaks until GC thrash: recorder retention, unbounded dedupe and seed scans (LLP 0204) #683, not something this PR introduces, and a rejecting scan fails at the for (const part of ...) line (i.e. cheaply, right after discoverCachePartitions).
• Erring toward true is right per LLP 0204. 0204#fix documents the seed index as tolerant: "a stale miss only risks the duplicate seeding guards against, which settlement/compaction still collapse". Returning true runs the per-session scan - extra work, no lost row. The opposite (false) would silently drop a real row. Matches the seedSessionIfCommitted JSDoc's stated posture.

Verified: the test

• Genuine reproduction. With only message_projector.js reverted to d3b3ea3 and the test unchanged:

  not ok 1 - committed-session index: a scan that throws degrades the index instead of killing the process
    error: '(partitions ?? []) is not iterable'   name: 'TypeError'
  # Error: ... would have caused the test to fail, but instead triggered an unhandledRejection event
  # pass 0  # fail 1
  

• Not vacuous, and not timing-fragile. Node emits unhandledRejection after the microtask queue drains within the tick, so one setImmediate already clears it; the test uses two. Ran the whole file 5x: # pass 36 # fail 0 every time.
• Listener does not leak. process.off is in a finally, so it is removed even when an assertion throws.
• One honest caveat: pre-fix the test fails via the awaited projectExchange rejection, so the unhandled collector's own assert.deepEqual is not the assertion that trips. The collector is still doing real work - it is what stops the pre-fix run from hard-killing the test process - and node:test names the unhandledRejection event explicitly. Recorded as a nit, not a defect.

Conventions

No semicolons, no U+2014 anywhere in the diff, no @typedef, no inline import() types, no .d.ts specifiers. @ref LLP 0204#fix [constrained-by] resolves: llp/0204-gateway-daemon-memory-leak.issue.md has a ## Fix heading, and test/core/llp-ref-hygiene.test.js (the enforcing gate) is green. The annotation sits in the comment block directly above const ids = ... with no blank line, so attachment holds, and the gloss says something the code does not. No LLP text edited, which is correct: this is a defect in 0204's implementation, not a change to what 0204 decided.

---

ACTIONABLE (cross-PR, for the merge rung - deliberately NOT fixed here)

A1 - severity: medium (merge coordination). hypaware-core/plugins-workspace/ai-gateway/src/message_projector.js:341-360 and test/plugins/ai-gateway-message-projector.test.js:897 are the same lines PR #689 (da2cd4f, issue #686) rewrites. Confirmed a hard conflict:

$ git merge-tree --write-tree 8ae05e2 da2cd4f
CONFLICT (content): Merge conflict in .../ai-gateway/src/message_projector.js
CONFLICT (content): Merge conflict in test/plugins/ai-gateway-message-projector.test.js

The two changes are semantically composable, but a resolution that takes #689's rebuild() wholesale would silently undo this PR's fix. #689 replaces the floating .then with a chain (attempt.ids = scan.then(...)), which removes the orphan and therefore the hard crash - but its chain has no rejection handler, so a rejecting scan then propagates out through mightHaveCommittedRows -> projectExchange, gets eaten by source.js, the exchange's row is lost, and built is left pointing at the rejected attempt: the wedged index #685 describes. The correct merged form keeps both:

const scan = scanCommittedSessionIds(storage, log).catch((err) => {
  log?.warn?.('aigw.session_index_scan_failed', {
    error: err instanceof Error ? err.message : String(err),
  })
  return undefined
})
const attempt = { atMs: Infinity, ids: scan }
attempt.ids = scan.then((ids) => {
  attempt.atMs = now()
  if (ids === undefined && built === attempt) built = undefined
  return ids
})
built = attempt

Mitigating: both regression tests guard the resolution in both directions. A merge that drops #690's .catch fails #690's test (projectExchange rejects and the await throws); a merge that drops #689's completion-stamped atMs fails #689's test (discoverCalls becomes 2). The two new tests coexist without conflict once the hunk positions are resolved.

NITS (no change requested)

• N1 message_projector.js:352 / :359 - the inner .then((ids) => ...) parameter shadows the outer const ids. Reads fine, but a distinct name would be clearer.
• N2 message_projector.js:353 reuses aigw.session_index_scan_failed verbatim from :426. An operator cannot distinguish "partition discovery failed" (an I/O condition) from "the scan promise rejected outright" (a contract violation, i.e. a code defect). CLAUDE.md's log-driven-development section suggests an error_kind-style discriminator; the two sites would benefit from one.
• N3 The new test builds its projector via freshSessionProjector(storage, () => 0), which passes no log, so nothing asserts the warn is emitted on the rejection path. I confirmed manually that it is (output above), but that behaviour is currently untested.
• N4 message_projector.js:353 - if log.warn itself threw, ids would reject and the floating guard would be unhandled again, so the promise is not unconditionally total. Purely theoretical (fields is { error: string }, always serializable), and the identical exposure already exists at :426. Committed-session index stamps atMs at scan start, so a slow scan never serves a cache hit #689's chain removes the floating pattern entirely, which closes even this.

Checks run (in worktree at 8ae05e2, unmodified)

npm test         -> 1..3862  # pass 3858  # fail 0  # skipped 6   (exit 0)
npm run typecheck -> clean, exit 0
node --test test/plugins/ai-gateway-message-projector.test.js  x5 -> # pass 36 # fail 0

No commits pushed - nothing in this PR needed fixing.

[philcunliffe]

Neutral review ROUND 2 - PR #690 @ 8ae05e2

Re-reviewed at the same head as round 1, in my own detached worktree off origin/fix/issue-685. I did not inherit round 1's conclusions: I re-derived each one, and probed the failure shapes round 1 did not exercise.

Verdict: round 1 was right about the fix and right about A1. I confirm both, with my own evidence. I found one thing round 1 did not raise that I fixed and pushed (F1), and one thing round 1 did not raise that I am reporting rather than fixing (A2).

New head after this round: 3bb9a5240c5443369e85048e367f26dace3c607d.

---

1. Round 1's claims, independently re-tested

The test is a genuine reproduction. CONFIRMED.

I reverted message_projector.js alone to origin/master and left the test untouched:

not ok 1 - committed-session index: a scan that throws degrades the index instead of killing the process
  error: '(partitions ?? []) is not iterable'   name: 'TypeError'
  stack: scanCommittedSessionIds (.../message_projector.js:411:36)
# Error: ... generated asynchronous activity after the test ended ... would have caused the test
#        to fail, but instead triggered an unhandledRejection event
# pass 0  # fail 1

The unhandledRejection collector is NOT vacuous. CONFIRMED, and I measured the margin.

Round 1 asserted "one setImmediate already clears it" without showing it. I measured it, one process per case, with the exact orphan shape (a derived .then on a rejected promise whose original is otherwise handled):

turns=0 detectedAtAssertion=0    <- with zero turns the assertion WOULD be vacuous
turns=1 detectedAtAssertion=1
turns=2 detectedAtAssertion=1
turns=3 detectedAtAssertion=1

Detection needs exactly one turn and the test spends two: correct, with one turn of margin, and not over-provisioned by accident. The turns=0 row is the load-bearing one: it shows the two setImmediates are doing real work, not decoration. (My probe process at turns=0 also died to Node's default handler, which is the same failure mode the fix prevents.)

Round 1's honest caveat stands and I repeat it: pre-fix, the test trips on the awaited rejection first, so the collector's own deepEqual is not the assertion that fires. The collector is still what keeps the pre-fix run from hard-killing the test process.

Correctness under the conditions round 1 did NOT test

I drove the real createAiGatewayMessageProjector through five scenarios with a storage whose discoverCachePartitions violates its contract on chosen calls (returns a non-iterable, so the throw escapes scanCommittedSessionIds from outside its try/catch):

#	scenario	outcome
S1	10 consecutive index-scan rejections	10/10 rows still emitted, 0 throws, exactly 20 discover calls (1 build + 1 fallback per exchange), exactly 10 warns, 0 unhandled
S2	scan rejects after a previous successful build	stale miss past the window rebuilds, that rebuild rejects, row still emitted, built self-clears, next exchange rebuilds successfully and recovers. Calls: [ok, throw, ok, ok]
S3	rejection while N=6 callers await the shared promise	all 6 fulfilled, 1 row each, exactly one index scan shared, 6 fallback scans, 0 unhandled
S4	N=6 concurrent stale misses past the rebuild window where the rebuild rejects	all 6 fulfilled, exactly one shared rebuild, 0 unhandled
S5	control: persistent discover_failed (the undefined path #683 already shipped)	10 rows, 10 warns, identical steady state

Three things fall out of this that I consider the strongest evidence in the PR's favour, and that round 1 did not produce:

• No unbounded retry and no leak. Failure costs exactly one rebuild per exchange. Not one per caller (S3/S4 share it), not exponential, not a storm. Each rebuild allocates two collectable promises.
• No wedge and no poisoned cache. S2 shows the index recovering on the very next exchange after a failure that followed a success.
• S5 is the decisive one. The new .catch produces a steady state identical to the undefined outcome The gateway daemon leaks until GC thrash: recorder retention, unbounded dedupe and seed scans (LLP 0204) #683 already shipped. That is precisely what "normalize a rejection into the same could-not-scan outcome" should mean, and it is measurable rather than asserted.

I also checked the tempting alternative fix, guarding the loop with Array.isArray(partitions). It would be wrong here: a malformed answer would yield an empty Set, i.e. an authoritative "no session has committed rows", which suppresses seeding and loses real rows. .catch to undefined ("could not scan", err toward scanning) is the right normalization. The PR picked correctly.

Is undefined correct per LLP 0204? CONFIRMED.

llp/0204-gateway-daemon-memory-leak.issue.md "Fix" describes the seed index as tolerant by construction: "a stale miss only risks the duplicate seeding guards against, which settlement/compaction still collapse". Erring toward true costs an extra scan and loses nothing. false would silently drop rows.

Does swallowing the rejection mask a permanent systemic failure?

Partly, and it is worth stating plainly, but it is not a defect this PR introduces. On permanent failure the daemon returns to pre-0204 behaviour: a whole-table per-session scan for every new session, which is root cause #3 of the very incident 0204 documents. So a permanently broken index silently walks the daemon back toward the 4.8 GB shape, with only warn lines to notice. S5 proves this property is identical to the undefined path already shipped in #683, so it belongs to 0204's design as shipped, not to this change. Surviving is still unambiguously better than dying: a crash-looping daemon takes down the proxy and every other dataset with it. I did act on the observability half of this (see F1).

Conventions

Re-checked independently: no semicolons, no U+2014 in the diff, no @typedef, no inline import() types. @ref LLP 0204#fix [constrained-by] resolves (0204 has a ## Fix heading), sits with no blank line above const ids, and the gloss carries information the code does not. No LLP text edited, which is right: this is a defect in 0204's implementation, not a change to what 0204 decided.

---

2. FIXED AND PUSHED

F1 - a survivable failure that does not say which failure it was. (Round 1 logged the shape of this as nit N2 and did not act; it has more teeth than a nit, because this PR is exactly what makes it matter.)

Before this PR, a rejecting scan killed the process, which is at least loud. After it, the daemon keeps serving on a degraded index and the log line is the only thing that says so. Both routes to that line emitted the same message with the same fields:

• message_projector.js:426 - discoverCachePartitions threw. An I/O condition; the next rebuild may well clear it on its own.
• message_projector.js:353 (new in this PR) - the scan rejected, breaking its own documented "resolve, never reject" contract. A code defect; it will keep degrading the index on every exchange until someone fixes it.

Identical output, opposite responses. CLAUDE.md's Log-Driven Development section calls for exactly this discriminator ("prefer structured attributes such as component, operation, status, error_kind").

Pushed as 3bb9a52:

• added error_kind: 'scan_rejected' at the new site and error_kind: 'discover_failed' at the pre-existing one, with a comment saying why the two must be distinguishable
• extended the regression test to actually pass a log and assert the warn. Round 1 noted (N3) that the test builds its projector via freshSessionProjector(storage, () => 0) with no log, so nothing asserted the warn at all; round 1 verified it by hand and left it untested. It is now tested: exactly one warn, error_kind === 'scan_rejected', error message matching /not iterable/.

No behaviour change beyond the added log field.

Verification the fix actually landed (not merely that the suite is green), against the pushed tree rather than my working copy:

$ git show origin/fix/issue-685:.../ai-gateway/src/message_projector.js | grep -n error_kind
352:    // `error_kind` separates the two ways this message is reached, because
360:        error_kind: 'scan_rejected',
434:      error_kind: 'discover_failed',
$ git show origin/fix/issue-685:test/plugins/ai-gateway-message-projector.test.js | grep -n "scan_rejected\|collectingLogger(logged)"
934:    const projector = freshSessionProjector(storage, () => 0, collectingLogger(logged))
952:    assert.equal(scanWarns[0].fields.error_kind, 'scan_rejected')

And the new assertion bites. Deleting only the error_kind: 'scan_rejected' line:

not ok 1 - committed-session index: a scan that throws degrades the index instead of killing the process
  expected: 'scan_rejected'
# pass 0  # fail 1

---

3. STILL ACTIONABLE (not fixed here)

A1 - hard conflict with PR #689. CONFIRMED. Severity: medium (merge coordination). Round 1's finding is real; I reproduced it and re-confirmed it at the new head:

$ git merge-tree --write-tree 3bb9a52 da2cd4f
CONFLICT (content): Merge conflict in .../ai-gateway/src/message_projector.js
CONFLICT (content): Merge conflict in test/plugins/ai-gateway-message-projector.test.js

I also read #689's diff rather than trusting the summary, and can sharpen round 1's warning. #689 replaces the floating .then with attempt.ids = scan.then(...) and that chain has no rejection handler. So a merge that takes #689's rebuild() wholesale does not just lose one row: attempt.ids stays permanently rejected, and because the guard is a fulfillment handler it never runs, so built is never cleared. Every later mightHaveCommittedRows awaits the same rejected promise. That is a permanent, total, silent loss of ai_gateway_messages capture, not a transient miss. Keeping both changes is mandatory, not stylistic. Round 1's suggested merged form is correct; it now also needs the error_kind field from 3bb9a52.

Round 1 checked #689 only. I also checked #688 (6127364), which touches the same test file: git merge-tree --write-tree 3bb9a52 6127364 is clean. No action needed there.

This remains owned by the merge rung, not by this PR.

A2 - NEW, not raised in round 1. Severity: medium. Pre-existing on master; this PR un-masks it.

scanCommittedMessageIds (message_projector.js:480) has the identical unguarded for (const part of partitions ?? []) outside its try/catch that this PR just hardened one function away, and its JSDoc at :454 states it "NEVER throws". That claim is false for the same malformed input.

Round 1 did not reach this because the PR's own fixture is shaped around it: the test makes only the first discoverCachePartitions malformed, so the per-session fallback scan is well-formed. A real storage that violates the contract violates it on every call. Driven that way, against the post-fix code:

results: [ THREW: (partitions ?? []) is not iterable,     <- sess-same, exchange 1
           THREW, THREW, THREW,                            <- sess-same, exchanges 2-4
           other THREW ]                                   <- a different session
warns:   [ aigw.session_index_scan_failed,
           aigw.session_index_scan_failed ]                <- only TWO, for five exchanges
unhandled: []

Read that carefully:

• The daemon survives. Zero unhandled rejections. This PR's stated goal is met, and source.js:228 absorbs the awaited rejection.
• But every projectExchange rejects, so every row is dropped, for every session.
• And it goes silent. seedPromises (:285-291) memoizes the rejected seed promise per session forever, so exchanges 2-4 for sess-same short-circuit on the poisoned memo, never reach the index, and emit no warn at all. Five exchanges, two warn lines, 100% row loss.

So this PR converts "daemon dies loudly in a restart loop" into "daemon lives and silently drops all gateway capture". Surviving is still the right trade, and the poisoned-memo behaviour is genuinely pre-existing (scanCommittedMessageIds and the memo are untouched here). But it is only reachable now, and it deserves its own issue LLP + PR rather than a drive-by widening of this one: the honest fix is to make scanCommittedMessageIds honour its own "never throws" contract (there, unlike in the session scan, degrading to "seeded nothing" is the documented behaviour), plus a decision about not memoizing a rejected seed. That is a separate change set with its own regression test.

Flagged, not fixed here. Note that F1 partially mitigates it: the first warn now says error_kind: 'scan_rejected', which is the operator's one signal that this is a code defect rather than a storage blip.

Nits carried forward, no change requested

• N1 message_projector.js:359/373 - the inner .then((ids) => ...) shadows the outer const ids.
• N4 if log.warn itself threw inside the .catch, the derived promise would be unhandled again. Theoretical (fields is plain strings); the identical exposure predates this PR at :426.

Checks run

At 8ae05e2 (unmodified):

npm test          -> 1..3862  # pass 3858  # fail 0  # skipped 6   (exit 0)
npm run typecheck -> clean, exit 0

At 3bb9a52 (after F1):

npm test          -> 1..3862  # pass 3858  # fail 0  # skipped 6   (exit 0)
npm run typecheck -> clean, exit 0
node --test test/plugins/ai-gateway-message-projector.test.js -> # pass 36 # fail 0

[philcunliffe]

Neutral triage - PR #690 @ 3bb9a5240c5443369e85048e367f26dace3c607d

This PR exhausted its review rounds with two findings still open. Triage re-read both review records, re-verified the code, and classified every residual as non-blocking: the PR can merge safely.

• A1 (conflict with Committed-session index stamps atMs at scan start, so a slow scan never serves a cache hit #689): a property of a future merge, not of this diff. Whichever PR merges second hits a normal content conflict that must be resolved by hand, and each PR's regression test fails a lossy resolution in the opposite direction, so a bad resolution cannot pass the suite. Recorded as a merge-sequencing note with the correct composed resolution (keep Committed-session index survives a throwing scan instead of wedging it #690's .catch, then layer Committed-session index stamps atMs at scan start, so a slow scan never serves a cache hit #689's completion-stamping chain).
• A2 (scanCommittedMessageIds can reject and the rejected seed promise is memoized forever): verified against the code; real and serious (silent capture loss under storage that violates the discover contract on every call), but pre-existing on master. Committed-session index survives a throwing scan instead of wedging it #690 touches none of those lines and reverting it would not fix it; in the trigger scenario master without Committed-session index survives a throwing scan instead of wedging it #690 is strictly worse (the daemon dies outright). Deferred to its own change set with its own regression test.

All deferred findings, with file:line detail and fix guidance, are recorded in follow-up issue #692.

[philcunliffe]

Neutral triage - PR #690 @ 045abc3328811a73ea194004fffd2c5509acc211

The review fix-loop hit its cap with this head, a merge of origin/master (3e8f05a) into the branch, unreviewed in its merged form. Triage re-verified the merge independently in a detached worktree and found zero new residual findings. Every deferred finding is already tracked in issue #692 (its item 1, the scanCommittedMessageIds seed-memo defect, now has PR #693 open), so no new follow-up issue was opened.

What was checked:

• The merge is exactly the composed resolution both review rounds prescribed for A1 (keep this PR's .catch normalization with error_kind: 'scan_rejected', layer Committed-session index stamps atMs at scan start, so a slow scan never serves a cache hit #689's completion-stamping chain on top). Diffed against both parents: no content that belongs to neither side. A1 is thereby resolved, not residual.
• The rationale shift is real, verified in both directions. On the merged-but-unfixed tree (master's rebuild(), no .catch): a rejecting index scan no longer kills the daemon (Committed-session index stamps atMs at scan start, so a slow scan never serves a cache hit #689's chain gives the rejection an awaiter; probe exits 0, zero unhandled rejections), but the rejected attempt stays published in built, the fulfilment-only handler never stamps or clears it, atMs stays Infinity, and 3 sequential exchanges all rejected with discoverCalls stuck at 1, zero rows, zero warns: a permanent, silent wedge. With this PR's fix: 3 rows, the index self-clears and rebuilds (discoverCalls 3), one error_kind: 'scan_rejected' warn. The reshaped regression test (...degrades the index instead of wedging it) pins both shapes and genuinely fails on the unfixed tree.
• Both parents' intent survives. Committed-session index stamps atMs at scan start, so a slow scan never serves a cache hit #689's stamp-at-completion test and Pin the committed-session-index failed-build fallback with a regression test #688's failed-build-fallback test pass unchanged alongside this PR's test. Full suite: 3895 tests, 3889 pass, 0 fail, 6 skipped. Typecheck clean.
• Residual classification: A1 resolved by this merge. A2 pre-existing on master, tracked in Follow-up: deferred review findings from PR #690 #692 / PR A rejected seed promise poisons a session into silent total row loss #693. N1 (shadowed ids) incidentally resolved (the outer binding is now scan). N4 (a throwing log.warn inside the .catch) remains theoretical and is recorded in Follow-up: deferred review findings from PR #690 #692. The PR body's crash-centred rationale was stale; corrected in the accompanying body edit.

Judgement: the wedge is a verified production defect on current master (silent, total, per-listener-permanent loss of ai_gateway_messages capture after one contract-violating storage answer), the fix is minimal and constrained by LLP 0204's best-effort posture, and the shift from "crash" to "wedge" changes the headline symptom, not the root cause or the case for landing. Documentation honesty, fixed in the body; not grounds to park the PR.