diff --git a/src/agent/agent-loop-reflection-fire-safety.test.ts b/src/agent/agent-loop-reflection-fire-safety.test.ts new file mode 100644 index 00000000..9a30e9da --- /dev/null +++ b/src/agent/agent-loop-reflection-fire-safety.test.ts @@ -0,0 +1,302 @@ +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { AgentLoop } from "./agent-loop.js"; +import type { MemoryContextProvider } from "./agent-loop.js"; +import { buildDefaultToolRegistry } from "../tools/index.js"; +import { SlotManager } from "../llm/slot-manager.js"; +import { createEmptySessionState } from "../session/session-state.js"; +import type { CompletionResult } from "../llm/llama-server-client.js"; +import type { + CapabilitiesSummary, + SkillCatalogEntry, + ToolDescriptor, +} from "../prompt/stable-prefix.js"; +import type { ProfileFact } from "../memory/profile-store.js"; +import type { StructuredLogger } from "../tracing/structured-logger.js"; +import type { + ReflectionInput, + ReflectionRunner, +} from "../memory/reflection/reflection-runner.js"; + +/** + * `runTurn` fires reflection as a bare `void` — it is background + * bookkeeping the user is not waiting on. Two ways that used to hurt + * the turn: + * + * - a decorator that reads a store closed by shutdown rejects, and + * with nothing attached to the promise the process reports an + * unhandled rejection (`error-reporting/error-reporter.ts` + * forwards those to the crash reporter); + * - `profileFactsProvider` is a raw `profileStore.list()` evaluated + * synchronously to build the reflection allowlist, so a store + * failure there failed the *turn*. + * + * Neither is recoverable by the loop and neither should be visible to + * the user, so both are pinned here. + */ + +function makeCompletion(content: string): CompletionResult { + return { + content, + reasoningContent: "", + stop: true, + truncated: false, + timing: { + promptMs: 1, + predictedMs: 1, + promptTokens: 10, + predictedTokens: 5, + }, + cacheHitTokens: 0, + slotId: 0, + modelId: "mock", + }; +} + +const TOOLS: ToolDescriptor[] = [ + { + name: "finish", + summary: "Finish the session with a summary.", + argsSchema: '{"summary": string}', + }, +]; + +const CAPS: CapabilitiesSummary = { + platform: "darwin", + arch: "arm64", + browserChannel: "chrome", + workingDir: "/work", + hasClipboard: true, + hasWmctrl: false, + hasNotifications: true, +}; + +const SKILLS: SkillCatalogEntry[] = []; + +const NOOP_PROVIDER: MemoryContextProvider = { + buildMemoryContext: () => ({ recalled: [], index: [] }), +}; + +function makeFact(id: number): ProfileFact { + return { + id, + key: "editor", + value: "vim", + validFrom: 1, + updatedAt: 1, + pinned: true, + keywords: [], + supersedes: null, + supersededBy: null, + voteScore: 0, + }; +} + +function makeLoop(deps: { + reflectionRunner: ReflectionRunner; + profileFactsProvider?: () => readonly ProfileFact[]; + logger?: StructuredLogger; +}): AgentLoop { + return new AgentLoop({ + registry: buildDefaultToolRegistry(), + slotManager: new SlotManager(2), + grammar: 'root ::= "ok"', + llmComplete: async () => + makeCompletion(JSON.stringify({ tool: "reply", args: { text: "ok" } })), + toolDescriptors: TOOLS, + capabilities: CAPS, + skillCatalog: SKILLS, + memoryContextProvider: NOOP_PROVIDER, + reflectionRunner: deps.reflectionRunner, + ...(deps.profileFactsProvider + ? { profileFactsProvider: deps.profileFactsProvider } + : {}), + ...(deps.logger ? { logger: deps.logger } : {}), + }); +} + +interface Warning { + message: string; + fields?: Record; +} + +function capturingLogger(into: Warning[]): StructuredLogger { + return { + debug() { + /* unused */ + }, + info() { + /* unused */ + }, + warn(message: string, fields?: Record) { + into.push({ message, ...(fields ? { fields } : {}) }); + }, + error() { + /* unused */ + }, + } as unknown as StructuredLogger; +} + +/** Collect unhandled rejections raised while `body` runs. */ +async function withUnhandledRejectionWatch( + body: () => Promise, +): Promise { + const seen: unknown[] = []; + const onRejection = (reason: unknown) => seen.push(reason); + // Vitest installs its own handler; prepend so ours observes first + // and keep the runner's in place. + process.prependListener("unhandledRejection", onRejection); + try { + await body(); + // An unhandled rejection is reported after the microtask queue + // drains — give the loop's `void` promise two macrotask ticks. + await new Promise((r) => setTimeout(r, 0)); + await new Promise((r) => setTimeout(r, 0)); + } finally { + process.removeListener("unhandledRejection", onRejection); + } + return seen; +} + +describe("AgentLoop reflection is background work, never a turn hazard", () => { + let workingDir: string; + + beforeEach(() => { + workingDir = mkdtempSync(join(tmpdir(), "atomic-reflect-loop-")); + }); + + afterEach(() => { + rmSync(workingDir, { recursive: true, force: true }); + }); + + it("a rejecting reflectionRunner raises no unhandled rejection", async () => { + let called = false; + const loop = makeLoop({ + reflectionRunner: { + async reflect(_input: ReflectionInput) { + called = true; + throw new TypeError("The database connection is not open"); + }, + abortPending() { + /* no-op */ + }, + }, + }); + const session = createEmptySessionState({ id: "s1", workingDir }); + + const seen = await withUnhandledRejectionWatch(async () => { + const result = await loop.runTurn(session, { + userMessage: "hello", + maxSteps: 2, + signal: new AbortController().signal, + }); + expect(result.session.id).toBe("s1"); + expect(result.reason).toBe("reply"); + }); + + expect(called).toBe(true); + expect(seen).toEqual([]); + }); + + it("a throwing profileFactsProvider does not fail the turn", async () => { + const inputs: ReflectionInput[] = []; + const loop = makeLoop({ + reflectionRunner: { + async reflect(input: ReflectionInput) { + inputs.push(input); + }, + abortPending() { + /* no-op */ + }, + }, + profileFactsProvider: () => { + throw new TypeError("The database connection is not open"); + }, + }); + const session = createEmptySessionState({ id: "s2", workingDir }); + + const result = await loop.runTurn(session, { + userMessage: "hello", + maxSteps: 2, + signal: new AbortController().signal, + }); + + // The load-bearing assertion: on main this returns + // `reason: "failed"` / `status: "failed"` — the session id alone + // is the same either way, so it proves nothing on its own. + expect(result.reason).toBe("reply"); + expect(result.session.status).not.toBe("failed"); + expect(result.session.id).toBe("s2"); + // Reflection still fires — just without profile candidates. + expect(inputs).toHaveLength(1); + expect(inputs[0]!.recalledProfileFactIds).toBeUndefined(); + }); + + it("both profile-facts guards report rather than swallow", async () => { + const warnings: Warning[] = []; + const inputs: ReflectionInput[] = []; + const loop = makeLoop({ + reflectionRunner: { + async reflect(input: ReflectionInput) { + inputs.push(input); + }, + abortPending() { + /* no-op */ + }, + }, + profileFactsProvider: () => { + throw new TypeError("The database connection is not open"); + }, + logger: capturingLogger(warnings), + }); + const session = createEmptySessionState({ id: "s4", workingDir }); + + const result = await loop.runTurn(session, { + userMessage: "hello", + maxSteps: 2, + signal: new AbortController().signal, + }); + + expect(result.reason).toBe("reply"); + expect(inputs).toHaveLength(1); + // The step guard fires per step; the reflection guard fires once + // at the end of the turn. Neither may be silent. + const messages = warnings.map((w) => w.message); + expect(messages).toContain("profile facts unavailable for this step"); + expect(messages).toContain("profile facts unavailable for reflection"); + for (const w of warnings) { + expect(w.fields).toMatchObject({ sessionId: "s4" }); + expect(String(w.fields?.error)).toContain( + "database connection is not open", + ); + } + }); + + it("a healthy profileFactsProvider still supplies the allowlist", async () => { + const inputs: ReflectionInput[] = []; + const loop = makeLoop({ + reflectionRunner: { + async reflect(input: ReflectionInput) { + inputs.push(input); + }, + abortPending() { + /* no-op */ + }, + }, + profileFactsProvider: () => [makeFact(7)], + }); + const session = createEmptySessionState({ id: "s3", workingDir }); + + await loop.runTurn(session, { + userMessage: "hello", + maxSteps: 2, + signal: new AbortController().signal, + }); + + expect(inputs).toHaveLength(1); + expect(inputs[0]!.recalledProfileFactIds).toEqual([7]); + }); +}); diff --git a/src/agent/agent-loop.ts b/src/agent/agent-loop.ts index ae8f91ec..00a80e76 100644 --- a/src/agent/agent-loop.ts +++ b/src/agent/agent-loop.ts @@ -505,8 +505,11 @@ export class AgentLoop { // strictly larger memory set. // // Shutdown path still calls `abortPending()` with no sessionId - // to drain every in-flight reflection before the runtime tears - // down SQLite handles. + // before the runtime tears down SQLite handles. Note that it + // *signals* — nothing is awaited, so a reflection can still be + // resuming when the stores close. That is why the decorators and + // this call site guard their store reads rather than relying on + // the abort to have finished. if (options.userMessage !== undefined) { const text = options.userMessage; @@ -651,7 +654,23 @@ export class AgentLoop { "This is the final allowed step. Do not call any non-terminal tool; " + "summarize the completed work with reply, or end the session with finish."; try { - const profileFacts = this.deps.profileFactsProvider?.(); + // `profileFactsProvider` is a raw `profileStore.list()`. + // Dropping the facts is a real loss — `profile-renderer` emits + // pinned facts regardless of the contextual gate, so this step + // renders with no `### profile` section at all — but it is the + // lesser one: a throw here lands in the + // catch below, where a `TypeError` from a closed SQLite handle + // classifies `tool` and fails the turn outright. + let profileFacts: readonly ProfileFact[] | undefined; + try { + profileFacts = this.deps.profileFactsProvider?.(); + } catch (err) { + this.deps.logger?.warn("profile facts unavailable for this step", { + sessionId: state.id, + stepIndex: i, + error: err instanceof Error ? err.message : String(err), + }); + } const activeProfile = this.deps.profileManager?.getProfile() ?? this.deps.profile ?? @@ -1144,11 +1163,31 @@ export class AgentLoop { // recalled across all steps of this turn) ∪ (profile // facts currently active). Profile facts are not gated // by recall — they're always candidates because the - // renderer already surfaces them whenever they pass the - // contextual-keyword gate. Sourcing them here keeps the + // renderer surfaces them whenever they are pinned or pass + // the contextual-keyword gate. Sourcing them here keeps the // decorator's hydration cheap. - const profileFacts = - this.deps.profileFactsProvider?.() ?? []; + // `profileFactsProvider` is a raw `profileStore.list()`. + // It is only ever an input to the fire-and-forget reflection + // below, so a store failure here must not fail the turn the + // user is waiting on — an empty allowlist just means the + // vote-runner sees no profile candidates this turn. + let profileFacts: readonly ProfileFact[] = []; + try { + profileFacts = this.deps.profileFactsProvider?.() ?? []; + } catch (err) { + // Usually the step guard above has already warned for this + // turn — same provider, same store. Not always: the store + // can close between the last step and this block. + this.deps.logger?.warn("profile facts unavailable for reflection", { + sessionId: state.id, + error: err instanceof Error ? err.message : String(err), + }); + } + // `reflect()` is documented fire-safe, but it is composed at + // runtime from decorators that read SQLite stores. A bare + // `void` turns any escape into an unhandled rejection the + // loop can neither see nor recover from, so the trailing + // `.catch` pins the contract at the call site too. void this.deps.reflectionRunner.reflect({ sessionId: state.id, userMessage, @@ -1184,6 +1223,11 @@ export class AgentLoop { ...(segmentationActive && transcript.length > 0 ? { transcript } : {}), + }).catch((err: unknown) => { + this.deps.logger?.warn("reflection failed after dispatch", { + sessionId: state.id, + error: err instanceof Error ? err.message : String(err), + }); }); } } diff --git a/src/memory/links/link-aware-reflection.ts b/src/memory/links/link-aware-reflection.ts index 409730a7..c9ef08d7 100644 --- a/src/memory/links/link-aware-reflection.ts +++ b/src/memory/links/link-aware-reflection.ts @@ -1,3 +1,4 @@ +import type { StructuredLogger } from "../../tracing/structured-logger.js"; import type { MemoryStore } from "../memory-store.js"; import type { ReflectionInput, @@ -40,6 +41,8 @@ export function createLinkAwareReflectionRunner(args: { notesStore: MemoryStore; /** Mirrors `LinkGeneratorRunnerDeps.minCandidates`. Defaults to 2. */ minCandidates?: number; + /** Reports a hydration failure — see the guard in `reflect`. */ + logger?: StructuredLogger; }): ReflectionRunner { const minCandidates = args.minCandidates ?? 2; return { @@ -51,11 +54,24 @@ export function createLinkAwareReflectionRunner(args: { } const ids = input.recalledMemoryIds ?? []; if (ids.length < minCandidates) return; + // Same shutdown race as the vote-aware decorator: `notesStore` + // is a SQLite handle that runtime shutdown may close while this + // fire-and-forget continuation is pending, and a closed + // better-sqlite3 statement throws `TypeError`. `reflect()` must + // stay fire-safe for the agent loop's bare `void` call. const candidates: { id: number; body: string }[] = []; - for (const id of ids) { - const entry = args.notesStore.get(id); - if (!entry) continue; - candidates.push({ id: entry.id, body: entry.content }); + try { + for (const id of ids) { + const entry = args.notesStore.get(id); + if (!entry) continue; + candidates.push({ id: entry.id, body: entry.content }); + } + } catch (err) { + args.logger?.warn("link candidate hydration failed", { + sessionId: input.sessionId, + error: err instanceof Error ? err.message : String(err), + }); + return; } if (candidates.length < minCandidates) return; try { diff --git a/src/memory/reflection-decorator-fire-safety.test.ts b/src/memory/reflection-decorator-fire-safety.test.ts new file mode 100644 index 00000000..c8373c1b --- /dev/null +++ b/src/memory/reflection-decorator-fire-safety.test.ts @@ -0,0 +1,477 @@ +import { afterEach, describe, expect, it } from "vitest"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { MemoryStore } from "./memory-store.js"; +import type { StructuredLogger } from "../tracing/structured-logger.js"; +import { ProfileStore } from "./profile-store.js"; +import { LessonStore } from "./lessons/lesson-store.js"; +import { ProcedureStore } from "./procedures/procedure-store.js"; +import { createVoteAwareReflectionRunner } from "./voting/vote-aware-reflection.js"; +import { createLinkAwareReflectionRunner } from "./links/link-aware-reflection.js"; +import type { ReflectionInput, ReflectionRunner } from "./reflection/reflection-runner.js"; +import type { VoteRunner, VoteRunnerInput } from "./voting/vote-runner.js"; +import type { + LinkGeneratorInput, + LinkGeneratorRunner, +} from "./links/link-generator-runner.js"; + +/** + * Regression pin for the reflection decorators' fire-safety contract. + * + * `ReflectionRunner.reflect()` is documented fire-safe — see the + * invariants block in `reflection/reflection-runner.ts` — and + * `AgentLoop.runTurn` relies on that by calling it as a bare `void`. + * Both decorators hydrate candidate ids out of SQLite-backed stores + * *after* awaiting the inner runner, and that hydration used to sit + * outside every `try`. + * + * The live failure that motivated this file: runtime `shutdown()` + * calls `reflectionRunner.abortPending()` and then closes every store + * (`bootstrap.ts`). The abort settles the *inner* reflection, so the + * decorator's 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`, which + * escaped `reflect()` and landed as an unhandled rejection. + */ + +interface Fixture { + dir: string; + memoryStore: MemoryStore; + profileStore: ProfileStore; + lessonStore: LessonStore; + procedureStore: ProcedureStore; + ids: { + memory: number; + memory2: number; + profile: number; + lesson: number; + procedure: number; + }; + closeAll: () => void; +} + +const fixtures: Fixture[] = []; + +function makeFixture(): Fixture { + const dir = mkdtempSync(join(tmpdir(), "atomic-reflect-firesafe-")); + const dbFile = join(dir, "memory.sqlite"); + const memoryStore = new MemoryStore({ + dbFile, + maxEntries: 100, + eviction: { utilityWeighted: true, maxAgeMs: 1_000_000 }, + }); + const profileStore = new ProfileStore({ dbFile }); + const lessonStore = new LessonStore({ dbFile }); + const procedureStore = new ProcedureStore({ dbFile }); + + const memory = memoryStore.store({ content: "note one" }).id; + const memory2 = memoryStore.store({ content: "note two" }).id; + const profile = profileStore.set("editor", "vim").id; + const lesson = lessonStore.create({ + activation: "when the build fails", + principle: "read the first error", + parentIds: [memory], + }).id; + const procedure = procedureStore.create({ + activation: "when releasing", + steps: [ + { description: "run the tests" }, + { description: "tag the commit" }, + ], + parentLessonIds: [lesson], + parentMemoryIds: [memory], + }).id; + + const fx: Fixture = { + dir, + memoryStore, + profileStore, + lessonStore, + procedureStore, + ids: { memory, memory2, profile, lesson, procedure }, + closeAll() { + // Mirrors bootstrap `shutdown()` ordering. + try { + profileStore.close(); + } catch { + /* already closed */ + } + try { + lessonStore.close(); + } catch { + /* already closed */ + } + try { + procedureStore.close(); + } catch { + /* already closed */ + } + try { + memoryStore.close(); + } catch { + /* already closed */ + } + }, + }; + fixtures.push(fx); + return fx; +} + +afterEach(() => { + for (const fx of fixtures.splice(0)) { + fx.closeAll(); + rmSync(fx.dir, { recursive: true, force: true }); + } +}); + +/** Inner runner that resolves normally, optionally with a side effect. */ +function innerRunner(onReflect?: () => void): ReflectionRunner { + return { + async reflect() { + onReflect?.(); + }, + abortPending() { + /* no-op */ + }, + }; +} + +function recordingVoteRunner(calls: VoteRunnerInput[]): VoteRunner { + return { + async run(input) { + calls.push(input); + return { outcome: "applied", applied: 1, rejected: 0 }; + }, + abortPending() { + /* no-op */ + }, + }; +} + +function recordingLinkGenerator(calls: LinkGeneratorInput[]): LinkGeneratorRunner { + return { + async generate(input) { + calls.push(input); + return 1; + }, + abortPending() { + /* no-op */ + }, + }; +} + +function inputFor(fx: Fixture): ReflectionInput { + return { + sessionId: "s1", + userMessage: "hello", + assistantReply: "hi", + recalledMemoryIds: [fx.ids.memory, fx.ids.memory2], + recalledLessonIds: [fx.ids.lesson], + recalledProfileFactIds: [fx.ids.profile], + recalledProcedureIds: [fx.ids.procedure], + }; +} + +describe("reflection decorators are fire-safe across a store close", () => { + it("premise: a better-sqlite3 read after close throws a TypeError", () => { + const fx = makeFixture(); + expect(fx.memoryStore.get(fx.ids.memory)).not.toBeNull(); + fx.closeAll(); + let thrown: unknown; + try { + fx.memoryStore.get(fx.ids.memory); + } catch (err) { + thrown = err; + } + expect(thrown).toBeInstanceOf(TypeError); + expect((thrown as Error).message).toContain("database connection is not open"); + }); + + it("vote-aware: hydration reaches the vote runner while the stores are open", async () => { + const fx = makeFixture(); + const calls: VoteRunnerInput[] = []; + const runner = createVoteAwareReflectionRunner({ + reflection: innerRunner(), + voteRunner: recordingVoteRunner(calls), + memoryStore: fx.memoryStore, + lessonStore: fx.lessonStore, + profileStore: fx.profileStore, + procedureStore: fx.procedureStore, + }); + + await runner.reflect(inputFor(fx)); + + expect(calls).toHaveLength(1); + // All four kinds hydrated — this is what the close-race test kills. + expect(calls[0]!.candidates.map((c) => c.kind).sort()).toEqual([ + "lesson", + "memory", + "memory", + "procedure", + "profile", + ]); + }); + + it("vote-aware: a store closed mid-flight does not reject reflect()", async () => { + const fx = makeFixture(); + const calls: VoteRunnerInput[] = []; + const runner = createVoteAwareReflectionRunner({ + // The shutdown race: `abortPending()` settles the inner + // reflection, then the closes land before this continuation + // gets to hydrate. + reflection: innerRunner(() => fx.closeAll()), + voteRunner: recordingVoteRunner(calls), + memoryStore: fx.memoryStore, + lessonStore: fx.lessonStore, + profileStore: fx.profileStore, + procedureStore: fx.procedureStore, + }); + + await expect(runner.reflect(inputFor(fx))).resolves.toBeUndefined(); + // Hydration failed wholesale, so the vote runner is never called + // with a partial allowlist. + expect(calls).toHaveLength(0); + }); + + it("vote-aware: a closed profile store alone does not reject reflect()", async () => { + const fx = makeFixture(); + const calls: VoteRunnerInput[] = []; + const runner = createVoteAwareReflectionRunner({ + reflection: innerRunner(() => fx.profileStore.close()), + voteRunner: recordingVoteRunner(calls), + memoryStore: fx.memoryStore, + lessonStore: fx.lessonStore, + profileStore: fx.profileStore, + procedureStore: fx.procedureStore, + }); + + await expect(runner.reflect(inputFor(fx))).resolves.toBeUndefined(); + expect(calls).toHaveLength(0); + }); + + it("link-aware: hydration reaches the link generator while the store is open", async () => { + const fx = makeFixture(); + const calls: LinkGeneratorInput[] = []; + const runner = createLinkAwareReflectionRunner({ + reflection: innerRunner(), + linkGenerator: recordingLinkGenerator(calls), + notesStore: fx.memoryStore, + }); + + await runner.reflect(inputFor(fx)); + + expect(calls).toHaveLength(1); + expect(calls[0]!.candidates.map((c) => c.body)).toEqual([ + "note one", + "note two", + ]); + }); + + it("link-aware: a store closed mid-flight does not reject reflect()", async () => { + const fx = makeFixture(); + const calls: LinkGeneratorInput[] = []; + const runner = createLinkAwareReflectionRunner({ + reflection: innerRunner(() => fx.closeAll()), + linkGenerator: recordingLinkGenerator(calls), + notesStore: fx.memoryStore, + }); + + await expect(runner.reflect(inputFor(fx))).resolves.toBeUndefined(); + expect(calls).toHaveLength(0); + }); + + it("vote-aware: a store that fails part-way yields no partial allowlist", async () => { + const fx = makeFixture(); + const calls: VoteRunnerInput[] = []; + // `memoryStore` answers the first id, then the handle goes away — + // the interleaving where hydration is already under way when + // shutdown lands. Guarding wholesale (not per-id) is a deliberate + // choice: the vote-runner scores a *set*, and a silently truncated + // allowlist would let it deprecate the entries that happened to be + // hydrated before the failure. + let reads = 0; + const flaky = new Proxy(fx.memoryStore, { + get(target, prop, receiver) { + if (prop === "get") { + return (id: number) => { + reads += 1; + if (reads > 1) { + throw new TypeError("The database connection is not open"); + } + return target.get(id); + }; + } + return Reflect.get(target, prop, receiver) as unknown; + }, + }) as MemoryStore; + + const runner = createVoteAwareReflectionRunner({ + reflection: innerRunner(), + voteRunner: recordingVoteRunner(calls), + memoryStore: flaky, + lessonStore: fx.lessonStore, + profileStore: fx.profileStore, + procedureStore: fx.procedureStore, + }); + + await expect(runner.reflect(inputFor(fx))).resolves.toBeUndefined(); + expect(reads).toBe(2); + expect(calls).toEqual([]); + }); + + it("link-aware: a store that fails part-way yields no partial allowlist", async () => { + const fx = makeFixture(); + const calls: LinkGeneratorInput[] = []; + // Three ids, failing on the third. Two is not enough to prove + // anything: `minCandidates` defaults to 2, so a truncated list of + // one is dropped by the length gate whether the guard returns or + // falls through. With three, a "continue with what we have" guard + // would hand the link-generator a 2-entry set that passes the + // gate — which is exactly the behaviour being ruled out. + const third = fx.memoryStore.store({ content: "note three" }).id; + let reads = 0; + const flaky = new Proxy(fx.memoryStore, { + get(target, prop, receiver) { + if (prop === "get") { + return (id: number) => { + reads += 1; + if (reads > 2) { + throw new TypeError("The database connection is not open"); + } + return target.get(id); + }; + } + return Reflect.get(target, prop, receiver) as unknown; + }, + }) as MemoryStore; + + const runner = createLinkAwareReflectionRunner({ + reflection: innerRunner(), + linkGenerator: recordingLinkGenerator(calls), + notesStore: flaky, + }); + + const input = inputFor(fx); + await expect( + runner.reflect({ + ...input, + recalledMemoryIds: [...(input.recalledMemoryIds ?? []), third], + }), + ).resolves.toBeUndefined(); + expect(reads).toBe(3); + expect(calls).toEqual([]); + }); + + it("the guards are not narrowed to TypeError — any store error is contained", async () => { + const fx = makeFixture(); + const voteCalls: VoteRunnerInput[] = []; + const linkCalls: LinkGeneratorInput[] = []; + // `MemoryStore.get` validates its id first and answers a bad one + // with a plain `MemoryValidationError`, not a `TypeError`. The + // closed-handle case is merely the one Sentry saw most. + const boom = new Proxy(fx.memoryStore, { + get(target, prop, receiver) { + if (prop === "get") { + return () => { + throw new RangeError("Too few parameter values were provided"); + }; + } + return Reflect.get(target, prop, receiver) as unknown; + }, + }) as MemoryStore; + + const vote = createVoteAwareReflectionRunner({ + reflection: innerRunner(), + voteRunner: recordingVoteRunner(voteCalls), + memoryStore: boom, + lessonStore: fx.lessonStore, + profileStore: fx.profileStore, + procedureStore: fx.procedureStore, + }); + const link = createLinkAwareReflectionRunner({ + reflection: innerRunner(), + linkGenerator: recordingLinkGenerator(linkCalls), + notesStore: boom, + }); + + await expect(vote.reflect(inputFor(fx))).resolves.toBeUndefined(); + await expect(link.reflect(inputFor(fx))).resolves.toBeUndefined(); + expect(voteCalls).toEqual([]); + expect(linkCalls).toEqual([]); + }); + + it("a hydration failure is logged, not silently swallowed", async () => { + const fx = makeFixture(); + const warnings: { message: string; fields?: Record }[] = []; + const logger = { + debug() { + /* unused */ + }, + info() { + /* unused */ + }, + warn(message: string, fields?: Record) { + warnings.push({ message, ...(fields ? { fields } : {}) }); + }, + error() { + /* unused */ + }, + } as unknown as StructuredLogger; + + const voteCalls: VoteRunnerInput[] = []; + const vote = createVoteAwareReflectionRunner({ + reflection: innerRunner(() => fx.closeAll()), + voteRunner: recordingVoteRunner(voteCalls), + memoryStore: fx.memoryStore, + lessonStore: fx.lessonStore, + profileStore: fx.profileStore, + procedureStore: fx.procedureStore, + logger, + }); + await vote.reflect(inputFor(fx)); + + expect(warnings).toHaveLength(1); + expect(warnings[0]!.message).toBe("vote candidate hydration failed"); + expect(warnings[0]!.fields).toMatchObject({ sessionId: "s1" }); + expect(String(warnings[0]!.fields?.error)).toContain( + "database connection is not open", + ); + + const fx2 = makeFixture(); + const linkCalls: LinkGeneratorInput[] = []; + const link = createLinkAwareReflectionRunner({ + reflection: innerRunner(() => fx2.closeAll()), + linkGenerator: recordingLinkGenerator(linkCalls), + notesStore: fx2.memoryStore, + logger, + }); + await link.reflect(inputFor(fx2)); + + expect(warnings).toHaveLength(2); + expect(warnings[1]!.message).toBe("link candidate hydration failed"); + }); + + it("both decorators composed: the close race still cannot reject", async () => { + const fx = makeFixture(); + const voteCalls: VoteRunnerInput[] = []; + const linkCalls: LinkGeneratorInput[] = []; + const runner = createVoteAwareReflectionRunner({ + reflection: createLinkAwareReflectionRunner({ + reflection: innerRunner(() => fx.closeAll()), + linkGenerator: recordingLinkGenerator(linkCalls), + notesStore: fx.memoryStore, + }), + voteRunner: recordingVoteRunner(voteCalls), + memoryStore: fx.memoryStore, + lessonStore: fx.lessonStore, + profileStore: fx.profileStore, + procedureStore: fx.procedureStore, + }); + + await expect(runner.reflect(inputFor(fx))).resolves.toBeUndefined(); + expect(linkCalls).toHaveLength(0); + expect(voteCalls).toHaveLength(0); + }); +}); diff --git a/src/memory/voting/vote-aware-reflection.ts b/src/memory/voting/vote-aware-reflection.ts index 2c5dac65..fed748cc 100644 --- a/src/memory/voting/vote-aware-reflection.ts +++ b/src/memory/voting/vote-aware-reflection.ts @@ -1,3 +1,5 @@ +import type { StructuredLogger } from "../../tracing/structured-logger.js"; + import type { LessonStore } from "../lessons/lesson-store.js"; import type { MemoryStore } from "../memory-store.js"; import type { ProcedureStore } from "../procedures/procedure-store.js"; @@ -49,6 +51,8 @@ export function createVoteAwareReflectionRunner(args: { procedureStore?: ProcedureStore | null; /** Per-preview character cap. Defaults to 80. */ previewChars?: number; + /** Reports a hydration failure — see the guard in `reflect`. */ + logger?: StructuredLogger; }): ReflectionRunner { const previewChars = args.previewChars ?? 80; return { @@ -58,7 +62,26 @@ export function createVoteAwareReflectionRunner(args: { } catch { // ReflectionRunner is already fire-safe — defence in depth. } - const candidates = hydrateCandidates(input, args, previewChars); + // Hydration reads four SQLite-backed stores. Those reads can + // throw — most often `TypeError: The database connection is not + // open`, because runtime shutdown settles the inner reflection + // via `abortPending()` and then closes every store while this + // fire-and-forget continuation is still pending. `reflect()` is + // contractually fire-safe (see `reflection-runner.ts`) and the + // agent loop calls it as a bare `void`, so a throw escaping here + // becomes an unhandled rejection rather than a swallowed miss. + let candidates: VoteCandidate[]; + try { + candidates = hydrateCandidates(input, args, previewChars); + } catch (err) { + // Swallowing without a word would trade a visible crash for + // silent curation loss, so the failure still gets a line. + args.logger?.warn("vote candidate hydration failed", { + sessionId: input.sessionId, + error: err instanceof Error ? err.message : String(err), + }); + return; + } if (candidates.length === 0) return; try { await args.voteRunner.run({ diff --git a/src/runtime/bootstrap.ts b/src/runtime/bootstrap.ts index 742a4bc4..5c6c5e7f 100644 --- a/src/runtime/bootstrap.ts +++ b/src/runtime/bootstrap.ts @@ -1863,6 +1863,7 @@ export async function createAgentRuntime( linkGenerator, notesStore, minCandidates: config.memory.links.minCandidates, + logger, }); } @@ -1949,6 +1950,7 @@ export async function createAgentRuntime( lessonStore, profileStore, procedureStore: config.memory.procedures.enabled ? procedureStore : null, + logger, }); }