Skip to content

fix(memory): reflection must not reject after a store closes - #326

Open
plombeer31 wants to merge 5 commits into
mainfrom
fix/reflection-hydration-fire-safe
Open

fix(memory): reflection must not reject after a store closes#326
plombeer31 wants to merge 5 commits into
mainfrom
fix/reflection-hydration-fire-safe

Conversation

@plombeer31

@plombeer31 plombeer31 commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

What

ReflectionRunner.reflect() is documented fire-safe — see the invariants block in src/memory/reflection/reflection-runner.ts ("The caller can void runner.reflect(input) safely") — and AgentLoop.runTurn relies on exactly that with a bare void. Two decorators wrap that runner, and both hydrate candidate ids out of SQLite-backed stores after awaiting the inner runner, outside every try:

  • src/memory/voting/vote-aware-reflection.tshydrateCandidates() reads memoryStore, lessonStore, profileStore, procedureStore
  • src/memory/links/link-aware-reflection.ts — the notesStore.get(id) loop

A throw from any of those rejects reflect(), and with nothing attached to the promise the process reports an unhandled rejection, which src/error-reporting/error-reporter.ts:91 forwards to the crash reporter.

Why it fires in the wild

Runtime shutdown() (src/runtime/bootstrap.ts:2214) does this, in order:

reflectionRunner?.abortPending();   // settles the INNER reflection
… await telegram.stop(); await mcpManager.shutdown(); await browserBackend.shutdown();
profileStore.close(); lessonStore.close(); procedureStore.close(); notesStore.close();

The abort settles the inner runner, so the decorator's continuation resumes and hydrates from stores the shutdown has since closed. better-sqlite3 answers a statement on a closed handle with a real TypeError:

TypeError: The database connection is not open

The synchronous closes win the race: the reflection chain needs more microtask hops to unwind (abort listener → Promise.raceawait llmComplete resume → runOne catch → finish → settle → decorator resume) than shutdown() has awaits. Reproduced end-to-end in a bootstrap-shaped composition (real stores, real createReflectionRunner, decorators layered the way bootstrap.ts layers them), both with resolved-promise teardown and with ~10 ms of real async teardown.

The shutdown comment already names the hazard — "Cancel any in-flight reflection before tearing down the profile store — otherwise a late-arriving completion could try to write into a closed SQLite connection" — but abortPending() only signals; nothing is awaited, and it only reaches the inner runner. The decorators were added after that comment and read stores past the abort point. (The stale claim at agent-loop.ts:507 that shutdown "drains" in-flight reflection is corrected in this PR.)

Second, separate hazard: profileFactsProvider

profileFactsProvider is wired as a raw () => profileStore.list() (bootstrap.ts:2121) and evaluated synchronously at two points in runTurn:

  • agent-loop.ts:654, inside the step loop's own try. A throw there lands in the step catch, is classified by classify-failure.ts (a TypeError matches neither isAbortError — "The database connection is not open" contains no aborted — nor isNetworkError, so it defaults to tool), emits loop_failed, and fails the turn the user is waiting on.
  • agent-loop.ts:1150, purely to build the reflection allowlist.

Fix

  • Both decorators guard their hydration and warn on the caught failure; bootstrap.ts passes the logger already in scope at both construction sites. Guarded wholesale rather than per-id: the vote-runner scores a set, and a silently truncated allowlist would let it deprecate whichever entries happened to hydrate before the failure.
  • profileFactsProvider guarded at both call sites, each with its own warning. The step-loop one renders that step without profile facts.
  • The void reflect(...) call site carries a trailing .catch that warns, as the outermost guarantee.

Every new guard reports. Swallowing silently would trade a visible crash for an invisible loss of curation, and it is not what the sibling paths do (agent-loop.ts:1327 logs memory context provider failed).

No behaviour change on the healthy path: profileFacts is spread as ...(profileFacts !== undefined ? { profileFacts } : {}) on both sides, so a healthy provider is byte-identical and a throwing one produces exactly the "no provider wired" shape. Control tests assert the full allowlist still reaches the vote runner and the link generator.

Sentry

Four clusters carry this signature. Only shortIds, counts and code paths quoted.

Cluster Events / users Window Signature
CLI-B6 16 / 2 2026-08-28 → 2026-09-03, releases 0.5.4 (11) + 0.4.2 (5) TypeError at store.gethydrateCandidatesObject.reflectprocessTicksAndRejections
CLI-6G 16 / 2 win32, 0.4.1 / 0.3.2 / 0.2.1 / 0.1.72 same, via getById
CLI-6H 14 / 3 linux + darwin, 0.3.5 / 0.4.1 / … same, via .get
CLI-34 9 / 1 category=tool TypeError at profileStore.listObject.profileFactsProviderrunTurnInner — the second hazard above

CLI-B6 is still firing on 0.5.4, which is why this is worth a fix rather than a note. CLI-34 is single-user and on an old release; it is cited as corroboration for a code path that is still live on main, not as the justification.

Test evidence

src/memory/reflection-decorator-fire-safety.test.ts (11 cases) builds real MemoryStore / ProfileStore / LessonStore / ProcedureStore on a temp SQLite file and closes them from inside the inner runner, reproducing the shutdown interleaving rather than mocking a throw. It pins the premise (a post-close read really is a TypeError), the healthy path, the wholesale-vs-per-id decision, that the guards are not narrowed to TypeError, and the warnings themselves.

src/agent/agent-loop-reflection-fire-safety.test.ts (4 cases) installs an unhandledRejection listener around a real runTurn and asserts none fires, pins the turn outcome (reason / status, not just the session id, which is identical on the failing path), and asserts both profile-facts guards report.

Non-vacuity, by reverting each src file onto the branch:

  • revert both decorators → 8 of 11 decorator cases fail (the 3 survivors are the premise pin and the two "stores open" controls)
  • revert agent-loop.ts3 of 4 loop cases fail, with the unhandled rejection actually observed on main (expected [ …(1) ] to deeply equal []) and the turn outcome as expected 'failed' to be 'reply'

Mutation battery — 9 of 9 functional mutations killed:

Mutation Result
vote hydration made per-id (continue past a bad store) killed (4 tests)
link-aware guard falls through with a partial allowlist killed
vote-aware warn dropped killed
link-aware warn dropped killed
both guards narrowed to TypeError, rethrow the rest killed
trailing .catch deleted killed
:654 step guard deleted killed (expected 'failed' to be 'reply')
:1150 reflection-input guard deleted killed
:1150 guard's warn dropped killed

The single survivor is an equivalent mutant: catch { return; }catch { candidates = []; } in the vote-aware guard, whose very next line is if (candidates.length === 0) return;.

The three-ids detail in the link-aware partial test is load-bearing, not incidental: minCandidates defaults to 2, so with only two ids a truncated list of one is dropped by the length gate whether the guard returns or falls through — the fall-through mutant survived that version.

npm run lint                                     clean (tsc --noEmit)
npx vitest run src/memory src/agent src/runtime   75 files / 975 tests passed
full suite, branch vs origin/main                 +15 tests, same single pre-existing
                                                  failure (src/sidecar/send-message-concurrency)

git merge-tree clean against origin/main (92515d3, v0.5.5).

Deliberately out of scope

  • The bootstrap wiring of the two decorator loggers is not pinned by a test. Deleting logger, from bootstrap.ts:1866 / :1953 passes the whole suite. The guards still contain the throw either way — only the reporting would go quiet — and a runtime-level pin would need a full createAgentRuntime harness driving a real reflection. Called out rather than hidden; a maintainer may want it anyway.
  • Shutdown still does not drain. abortPending() signals and returns; after this PR a shutdown mid-reflection silently discards the vote/link work instead of crashing. A real drain() is a maintainer call, so this PR fixes the comment rather than the semantics.
  • The crash reporter still receives an unhandledRejection for anything else that escapes a fire-and-forget path. This closes the reflection one rather than adding a blanket process-level swallow, which would hide real defects. Every other bare void in src/agent, src/memory, src/runtime was checked: memory-store.ts:441 is total-catch by construction (embedding-writer.ts:54, "Never throws"), consolidator-job.ts:239 and bootstrap.ts:2934 already carry .catch.
  • A thrown value whose String() conversion itself throws would still escape the new .catch callback. That shape (err instanceof Error ? err.message : String(err)) is used at ~183 non-test sites in this repo; making it safe is a repo-wide change, not this PR's.

plombeer31 added 5 commits September 3, 2026 23:03
`ReflectionRunner.reflect()` is documented fire-safe and `AgentLoop`
calls it as a bare `void`. Both decorators that wrap it hydrate
candidate ids out of SQLite-backed stores *after* awaiting the inner
runner, and that hydration sat outside every `try`.

Runtime `shutdown()` calls `reflectionRunner.abortPending()` and then
closes every store. The abort settles the inner reflection, so the
decorator continuation resumes and reads stores the shutdown has since
closed — better-sqlite3 answers a statement on a closed handle with a
real `TypeError: The database connection is not open`. That escaped
`reflect()` and surfaced as an unhandled rejection.

Separately, `profileFactsProvider` is a raw `profileStore.list()`
evaluated synchronously at two points in `runTurn`. The one inside the
step loop throws into the step's own catch, where a `TypeError` is
classified `tool` and fails the turn the user is waiting on — for
prompt decoration the renderer would have dropped anyway.

- vote-aware / link-aware decorators: hydration is guarded, so a
  failed read skips the sub-call instead of rejecting.
- `profileFactsProvider` is guarded at both call sites; the step-loop
  one logs a warning and renders without profile facts.
- the `void reflect(...)` call site carries a `.catch` as the outermost
  guarantee.

Tests: `src/memory/reflection-decorator-fire-safety.test.ts` drives the
real stores and closes them mid-flight (4 of its 7 cases fail without
the src change); `src/agent/agent-loop-reflection-fire-safety.test.ts`
watches for an unhandled rejection and pins the turn outcome (2 of 3
fail without it). `npm run lint` clean; `src/memory` + `src/agent`
64 files / 849 tests green, `src/runtime` 11 / 121 green.

Sentry: CLI-B6 (16 events / 2 users, live on 0.5.4), CLI-6G (16 / 2),
CLI-6H (14 / 3) all carry the `hydrateCandidates` -> store `.get` /
`.getById` TypeError signature; CLI-34 (9 / 1) is the
`profileFactsProvider` -> `list` variant with `category=tool`.
Review of the first commit: the guards traded a visible crash for total
silence. Both decorator factories took no logger, so a persistent
hydration failure would disable the vote-runner and link-generator for
the life of the process with no signal anywhere — the opposite of what
the PR argued for, and out of step with the sibling paths
(`memory context provider failed` is logged).

- `createVoteAwareReflectionRunner` / `createLinkAwareReflectionRunner`
  take an optional `StructuredLogger` and warn on a caught hydration
  failure; `bootstrap.ts` passes the `logger` already in scope at both
  construction sites.
- the trailing `.catch` on the `void reflect(...)` call warns instead
  of discarding.
- the stale claim at `agent-loop.ts:507` that shutdown "drains" every
  in-flight reflection is corrected: `abortPending()` only signals,
  which is precisely why these guards exist.

Tests, closing the coverage the review measured:
- partial-hydration cases for both decorators — a store that answers
  the first id and then fails yields NO partial allowlist. This pins
  the wholesale-vs-per-id decision the PR body argues for; the
  link-aware "continue with a partial set" mutation survived the whole
  suite before this.
- a non-`TypeError` store failure is contained just the same, so the
  guards are not silently narrowed to the closed-handle case.
- the warn itself is asserted (message, sessionId, error text) for
  both decorators.
- the turn-outcome assertions now check `reason` / `status`, not just
  the session id, which is identical on the failing path.

`npm run lint` clean; `src/memory src/agent src/runtime` 75 files /
974 tests green.

Not changed: `catch { return; }` → `catch { candidates = []; }` in the
vote-aware guard survives the suite, but it is an equivalent mutant —
the very next line is `if (candidates.length === 0) return;`.
The first version used two ids and failed on the second. That proves
nothing: `minCandidates` defaults to 2, so a truncated list of one is
dropped by the length gate whether the guard returns or falls through
— the "continue with a partial set" mutation survived it.

Three ids, failing on the third, so a fall-through guard would hand
the link-generator a 2-entry set that passes the gate. Mutation
confirmed killed.

Battery re-run on this branch, all 8 functional mutations killed:
per-id vote hydration (2 tests), link-aware partial fall-through,
either warn dropped, guards narrowed to TypeError, the trailing
`.catch` deleted, and each `profileFactsProvider` guard deleted — the
step-loop one now dies on `expected 'failed' to be 'reply'`, i.e. on
the turn outcome rather than incidentally.

The one survivor is an equivalent mutant: `catch { return; }` →
`catch { candidates = []; }` in the vote-aware guard, whose very next
line is `if (candidates.length === 0) return;`.
Review caught the comment (and the PR body) claiming the renderer
already drops these facts when the contextual gate does not match.
`profile-renderer.ts:63` returns true for every pinned fact before
the gate is consulted, so the guard omits the whole `### profile`
section for the rest of the turn. Still the right trade against
failing the turn, but say so accurately.
Second review round found the one remaining silent swallow — the
`profileFactsProvider` guard feeding the reflection allowlist — which
made the PR's own "nothing is swallowed silently" claim false. Usually
the step guard has already warned for that turn (same provider, same
store), but the store can close between the last step and this block.

Also corrected two comments the review measured as imprecise:
- the step guard drops the `### profile` section for that *step*, not
  the rest of the turn;
- the surviving comment at the reflection block still said the renderer
  surfaces profile facts "whenever they pass the contextual-keyword
  gate", the same imprecision fixed 500 lines above — pinned facts
  bypass the gate.

New test asserts BOTH guards report, with sessionId and error text;
reverting the new warn kills it.

`npm run lint` clean; `src/memory src/agent src/runtime` 75 files /
975 tests green.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant