From 1dbc457ad97977fdfb593fa8ad0c204276fe4ad3 Mon Sep 17 00:00:00 2001 From: Marc Date: Tue, 11 Aug 2026 17:29:45 +0100 Subject: [PATCH 1/5] fix: harden plugin shutdown and cold passes --- package-lock.json | 3 + package.json | 3 + src/cards.ts | 9 +- src/distill.ts | 245 +++++++++++++++++---- src/opencode-session-recall.ts | 128 +++++++++-- src/summarize.ts | 139 +++++++++--- test/cards.test.ts | 37 ++++ test/distill.test.ts | 386 ++++++++++++++++++++++++++++++++- test/plugin.test.ts | 333 +++++++++++++++++++++++++++- test/summarize.test.ts | 71 +++++- test/tools.test.ts | 9 +- 11 files changed, 1254 insertions(+), 109 deletions(-) diff --git a/package-lock.json b/package-lock.json index 57da9b1..8967f4b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -26,6 +26,9 @@ "typescript-eslint": "^8.58.1", "vitest": "^4.1.5" }, + "engines": { + "opencode": ">=1.15.11" + }, "peerDependencies": { "@opencode-ai/plugin": ">=1.2.0" } diff --git a/package.json b/package.json index 2ab7f70..3e773e7 100644 --- a/package.json +++ b/package.json @@ -62,6 +62,9 @@ "url": "https://github.com/rmk40/opencode-session-recall/issues" }, "license": "MIT", + "engines": { + "opencode": ">=1.15.11" + }, "peerDependencies": { "@opencode-ai/plugin": ">=1.2.0" }, diff --git a/src/cards.ts b/src/cards.ts index 90a90de..f924283 100644 --- a/src/cards.ts +++ b/src/cards.ts @@ -200,6 +200,8 @@ export type CardsRuntime = { semanticStatus(): SemanticStatus | undefined; /** Force the next rank() to rebuild from the source (tests). */ invalidate(): void; + /** Prevent deferred semantic warm-up from touching its source after shutdown. */ + dispose(): void; }; function basename(path: string): string { @@ -366,6 +368,7 @@ export function createCardsRuntime(deps: CardsRuntimeDeps): CardsRuntime { let lastRevision: string | undefined; let lastVectorsRevision: string | undefined; let loaded = false; + let disposed = false; /** Recompute/reuse card vectors and persist newly computed ones. Returns the * store's `{ revision, committed }` result for the caller's own-write accounting, @@ -543,7 +546,7 @@ export function createCardsRuntime(deps: CardsRuntimeDeps): CardsRuntime { // no snapshot is loaded yet (the next query then rebuilds and embeds). if (embedder && semanticWeight > 0 && deps.semanticReady) { void deps.semanticReady.then(() => { - if (!embedder.ready || !loaded) return; + if (disposed || !embedder.ready || !loaded) return; if (cardVectors && cardVectors.size > 0) return; // Deliberately leave lastVectorsRevision untouched: this pass writes vectors // (bumping vectors_rev past the cached value), so the next refresh reloads @@ -554,6 +557,10 @@ export function createCardsRuntime(deps: CardsRuntimeDeps): CardsRuntime { } return { + dispose(): void { + disposed = true; + }, + rank(query, filters): CardHit[] { refreshIfStale(); const excluded = filters.excludeFamilyOf diff --git a/src/distill.ts b/src/distill.ts index f000c34..d54718c 100644 --- a/src/distill.ts +++ b/src/distill.ts @@ -44,11 +44,21 @@ const DEFAULT_LEASE_RETRY_MS = 60_000; const DEFAULT_COLD_PASS_RETRY_MS = 60_000; const LEASE_TTL_MS = 30_000; const HEARTBEAT_MS = 10_000; +/** Bound process-lifetime state retained for malformed legacy sessions. */ +const MAX_QUARANTINED_SESSIONS = 1_000; // ── Shared shapes ──────────────────────────────────────────────────────────── type MsgWithParts = { info: Message; parts: Part[] }; +class MalformedSessionError extends Error { + override name = "MalformedSessionError"; +} + +class SessionMetadataTransportError extends Error { + override name = "SessionMetadataTransportError"; +} + /** Human-layer field extracted from one part; the FTS `norm` column and the * per-row ids are added when this becomes a {@link PartTextRow}. */ export type DistillField = { @@ -100,7 +110,12 @@ export type DistillStatus = { export type Distiller = { start(): void; - stop(): void; + /** Stop accepting or scheduling work immediately, but keep renewing an + * already-held lease until {@link stop} finalizes the handoff. */ + quiesce(): void; + /** Whether this instance still owns a live lease, verified against the store. */ + ownsLease(): boolean; + stop(): Promise; onEvent(event: Event): void; status(): DistillStatus; }; @@ -562,7 +577,10 @@ export async function fetchMessagePage( : { sessionID: opts.sessionID, limit: opts.limit }; const resp = await client.session.messages(params); if (resp.error) throw new Error(errmsg(resp.error)); - const items = Array.isArray(resp.data) ? (resp.data as MsgWithParts[]) : []; + if (!Array.isArray(resp.data)) { + throw new MalformedSessionError("successful message response was not an array"); + } + const items = resp.data as MsgWithParts[]; return { items, nextCursor: readNextCursor(resp.response) }; } @@ -603,7 +621,14 @@ export function createDistiller(options: DistillerOptions): Distiller { // Store unavailable (degraded mode): every method is a clean no-op. if (!options.store) { - return { start() {}, stop() {}, onEvent() {}, status: noopStatus }; + return { + start() {}, + quiesce() {}, + ownsLease: () => false, + async stop() {}, + onEvent() {}, + status: noopStatus, + }; } const store: Store = options.store; @@ -628,6 +653,7 @@ export function createDistiller(options: DistillerOptions): Distiller { type Timer = ReturnType; let stopped = false; + let finalized = false; let leaseHeld = false; let coldPassState: DistillStatus["coldPass"] = "idle"; let lastError: string | undefined; @@ -638,8 +664,11 @@ export function createDistiller(options: DistillerOptions): Distiller { let leaseRetryTimer: Timer | undefined; let coldPassRetryTimer: Timer | undefined; const debounceTimers = new Map(); - const inFlight = new Set(); + let coldPassPromise: Promise | undefined; + const inFlight = new Map>(); const pendingRerun = new Set(); + const quarantined = new Map(); + let stopPromise: Promise | undefined; /** Sessions that saw a removal-shaped event since their last full distill, so * the next re-distill takes the full path rather than appending. */ const removalSince = new Set(); @@ -665,15 +694,30 @@ export function createDistiller(options: DistillerOptions): Distiller { // ── Fetch primitives (all through the gate at background priority) ── async function discoverSessions(): Promise { - const sessions = await gate.runBackground(() => discover()); + const sessions = await gate.runBackground(() => { + if (stopped || !leaseHeld) return Promise.resolve([]); + return discover(); + }); // Never distill the summarizer's worker session — its prompts embed card // digests, which recall must not surface (see isSummarizerTitle). return sessions.map(toMeta).filter((meta) => !isSummarizerTitle(meta.title)); } async function fetchSessionMeta(sessionID: string): Promise { - const resp = await gate.runBackground(() => client.session.get({ sessionID })); - if (resp.error || !resp.data || typeof resp.data !== "object") return null; + const resp = await gate.runBackground(() => { + if (stopped || !leaseHeld) return Promise.resolve(null); + return client.session.get({ sessionID }); + }); + if (!resp) return null; + if (resp.error) { + throw new SessionMetadataTransportError( + `session ${sessionID} metadata fetch failed: ${errmsg(resp.error)}`, + ); + } + if (resp.data == null) return null; + if (typeof resp.data !== "object") { + throw new MalformedSessionError(`session ${sessionID} metadata response was not an object`); + } return toMeta(resp.data as Session); } @@ -686,15 +730,24 @@ export function createDistiller(options: DistillerOptions): Distiller { let rowCount = 0; let first = true; do { - if (!first && limits.distillDelayMs > 0) await sleep(limits.distillDelayMs); + if (stopped || !leaseHeld) return all; + if (!first && limits.distillDelayMs > 0) { + await sleep(limits.distillDelayMs); + if (stopped || !leaseHeld) return all; + } first = false; const before = cursor; - const page = await gate.runBackground(() => - fetchMessagePage(client, { sessionID, limit: pageMessages, before }), - ); - for (const msg of page.items) { - all.push(msg); - for (const part of msg.parts) rowCount += distillFields(part).length; + const page = await gate.runBackground(() => { + if (stopped || !leaseHeld) return Promise.resolve({ items: [], nextCursor: null }); + return fetchMessagePage(client, { sessionID, limit: pageMessages, before }); + }); + try { + for (const msg of page.items) { + all.push(msg); + for (const part of msg.parts) rowCount += distillFields(part).length; + } + } catch (error) { + throw new MalformedSessionError(errmsg(error)); } cursor = page.nextCursor ?? undefined; } while (cursor && rowCount < maxRows); @@ -715,12 +768,17 @@ export function createDistiller(options: DistillerOptions): Distiller { let first = true; let reached = false; do { - if (!first && limits.distillDelayMs > 0) await sleep(limits.distillDelayMs); + if (stopped || !leaseHeld) return { messages: collected, reached: false }; + if (!first && limits.distillDelayMs > 0) { + await sleep(limits.distillDelayMs); + if (stopped || !leaseHeld) return { messages: collected, reached: false }; + } first = false; const before = cursor; - const page = await gate.runBackground(() => - fetchMessagePage(client, { sessionID, limit: pageMessages, before }), - ); + const page = await gate.runBackground(() => { + if (stopped || !leaseHeld) return Promise.resolve({ items: [], nextCursor: null }); + return fetchMessagePage(client, { sessionID, limit: pageMessages, before }); + }); for (const msg of page.items) { if (msg.info.id === checkpoint) { reached = true; @@ -805,6 +863,7 @@ export function createDistiller(options: DistillerOptions): Distiller { parentById: parentChainOf(session), caps, }); + if (stopped || !leaseHeld) return; store.replaceSessionParts(session.id, rows, card); bumpCardsRev(); } @@ -816,6 +875,7 @@ export function createDistiller(options: DistillerOptions): Distiller { return; } const { messages: newMessages, reached } = await fetchNewMessages(session.id, checkpoint); + if (stopped || !leaseHeld) return; // If the checkpoint message was never found, the newest pages are NOT a clean // append tail (they'd re-insert already-stored parts and hit a UNIQUE // violation, wedging the session). Fall back to a full re-distill. @@ -907,6 +967,12 @@ export function createDistiller(options: DistillerOptions): Distiller { try { const discovered = await discoverSessions(); knownCount = discovered.length; + const liveUpdates = new Map(discovered.map((session) => [session.id, session.timeUpdated])); + for (const [sessionId, timeUpdated] of quarantined) { + // Deletions and updates both invalidate quarantine. An updated session is + // retried below; a deleted one no longer consumes retained state. + if (liveUpdates.get(sessionId) !== timeUpdated) quarantined.delete(sessionId); + } const parentById = new Map(discovered.map((s) => [s.id, s.parentId] as const)); const sorted = [...discovered].sort( (a, b) => b.timeUpdated - a.timeUpdated || a.id.localeCompare(b.id), @@ -924,12 +990,33 @@ export function createDistiller(options: DistillerOptions): Distiller { const upToDate = existing?.distillState === "full" && existing.timeUpdated === session.timeUpdated; if (!upToDate) { - const { card, rows } = deriveCard({ - session, - messages: await fetchSessionMessages(session.id, caps.ftsRowsPerSession), - parentById, - caps, - }); + if (quarantined.get(session.id) === session.timeUpdated) { + examined++; + recordProgress(session.timeUpdated); + return; + } + + let messages: MsgWithParts[]; + try { + messages = await fetchSessionMessages(session.id, caps.ftsRowsPerSession); + } catch (error) { + if (!(error instanceof MalformedSessionError)) throw error; + quarantine(session, error); + examined++; + recordProgress(session.timeUpdated); + return; + } + + let card: Card; + let rows: PartTextRow[]; + try { + ({ card, rows } = deriveCard({ session, messages, parentById, caps })); + } catch (error) { + quarantine(session, error); + examined++; + recordProgress(session.timeUpdated); + return; + } // The lease can drop (heartbeat takeover) during the fetch above; a // non-holder must never write. Re-check right before the write and // skip it, counting the session as not distilled. @@ -966,11 +1053,37 @@ export function createDistiller(options: DistillerOptions): Distiller { } } + function quarantine(session: DistillSessionMeta, error: unknown): void { + // Reinsertion keeps FIFO eviction aligned with the latest failure. + quarantined.delete(session.id); + quarantined.set(session.id, session.timeUpdated); + while (quarantined.size > MAX_QUARANTINED_SESSIONS) { + const oldest = quarantined.keys().next().value; + if (oldest === undefined) break; + quarantined.delete(oldest); + } + logMsg( + `session ${session.id} quarantined at timeUpdated ${session.timeUpdated}: ${errmsg(error)}`, + ); + } + + function startColdPass(): Promise { + if (stopped || !leaseHeld) return Promise.resolve(); + if (coldPassPromise) return coldPassPromise; + const running = runColdPass().finally(() => { + if (coldPassPromise === running) coldPassPromise = undefined; + }); + coldPassPromise = running; + return running; + } + function scheduleColdPassRetry(): void { + if (stopped || !leaseHeld) return; clearTimer(coldPassRetryTimer); coldPassRetryTimer = setTimeout(() => { + coldPassRetryTimer = undefined; if (stopped || !leaseHeld) return; - if (coldPassState === "idle" && lastError !== undefined) void runColdPass(); + if (coldPassState === "idle" && lastError !== undefined) void startColdPass(); }, coldPassRetryMs); } @@ -979,6 +1092,7 @@ export function createDistiller(options: DistillerOptions): Distiller { // (Stage 3). Resume correctness comes from the per-card skip above. let progressFloor: number | undefined; function recordProgress(timeUpdated: number): void { + if (stopped || !leaseHeld) return; if (progressFloor == null || timeUpdated < progressFloor) { progressFloor = timeUpdated; store.setMeta("coldpass_cursor", String(timeUpdated)); @@ -994,18 +1108,25 @@ export function createDistiller(options: DistillerOptions): Distiller { sessionID, setTimeout(() => { debounceTimers.delete(sessionID); - void runReDistill(sessionID); + startReDistill(sessionID); }, idleDebounceMs), ); } - async function runReDistill(sessionID: string): Promise { + function startReDistill(sessionID: string): void { if (stopped || !leaseHeld) return; // non-holders and stopped instances write nothing if (inFlight.has(sessionID)) { pendingRerun.add(sessionID); // coalesce: run once more after the in-flight pass return; } - inFlight.add(sessionID); + const running = runReDistill(sessionID).finally(() => { + inFlight.delete(sessionID); + if (!stopped && pendingRerun.delete(sessionID)) scheduleReDistill(sessionID); + }); + inFlight.set(sessionID, running); + } + + async function runReDistill(sessionID: string): Promise { // Snapshot-and-clear the removal flag BEFORE any await: a removal event that // arrives mid-distill re-adds it independently, so the coalesced rerun still // forces a full re-distill instead of appending onto a compacted transcript. @@ -1025,6 +1146,7 @@ export function createDistiller(options: DistillerOptions): Distiller { !hadRemoval; if (canAppend && existing) await distillAppend(session, existing); else await distillFull(session); + if (stopped || !leaseHeld) return; // Keep the root's family highlights current with its live children. const stored = store.getCard(sessionID); if (stored && stored.rootId !== sessionID) recomputeRootRollup(stored.rootId); @@ -1033,14 +1155,10 @@ export function createDistiller(options: DistillerOptions): Distiller { } } catch (error) { lastError = errmsg(error); + logMsg(`session ${sessionID} re-distill failed: ${lastError}`); } finally { // If the distill did not land, preserve the removal signal for the retry. if (!succeeded && hadRemoval) removalSince.add(sessionID); - inFlight.delete(sessionID); - if (pendingRerun.has(sessionID)) { - pendingRerun.delete(sessionID); - scheduleReDistill(sessionID); - } } } @@ -1055,6 +1173,7 @@ export function createDistiller(options: DistillerOptions): Distiller { debounceTimers.delete(sessionID); } removalSince.delete(sessionID); + quarantined.delete(sessionID); if (rootId && rootId !== sessionID) recomputeRootRollup(rootId); } @@ -1067,7 +1186,8 @@ export function createDistiller(options: DistillerOptions): Distiller { function scheduleHeartbeat(): void { clearTimer(heartbeatTimer); heartbeatTimer = setTimeout(() => { - if (stopped || !leaseHeld) return; + heartbeatTimer = undefined; + if (finalized || !leaseHeld) return; if (store.heartbeatLease(instanceId)) scheduleHeartbeat(); else { // Lost the lease (taken over): stop acting as holder and try to regain. @@ -1080,14 +1200,18 @@ export function createDistiller(options: DistillerOptions): Distiller { ? `lease lost to ${taker.holder} (build ${taker.build || "?"}, gen ${taker.gen})` : "lease lost", ); - scheduleLeaseRetry(); + if (!stopped) scheduleLeaseRetry(); } }, HEARTBEAT_MS); } function scheduleLeaseRetry(): void { + if (stopped || finalized) return; clearTimer(leaseRetryTimer); - leaseRetryTimer = setTimeout(acquire, leaseRetryMs); + leaseRetryTimer = setTimeout(() => { + leaseRetryTimer = undefined; + acquire(); + }, leaseRetryMs); } function acquire(): void { @@ -1096,29 +1220,61 @@ export function createDistiller(options: DistillerOptions): Distiller { leaseHeld = true; logMsg(`lease acquired (build ${build}, gen ${gen})`); scheduleHeartbeat(); - if (limits.coldPass && coldPassState === "idle") void runColdPass(); + if (limits.coldPass && coldPassState === "idle") void startColdPass(); } else { leaseHeld = false; scheduleLeaseRetry(); } } + function ownsLease(): boolean { + if (!leaseHeld || finalized) return false; + const current = store.leaseStatus(); + const owned = current?.holder === instanceId && current.heartbeat + current.ttl > now(); + if (!owned) { + leaseHeld = false; + clearTimer(heartbeatTimer); + heartbeatTimer = undefined; + if (!stopped) scheduleLeaseRetry(); + } + return owned; + } + + function quiesce(): void { + if (stopped) return; + stopped = true; + clearTimer(leaseRetryTimer); + clearTimer(coldPassRetryTimer); + leaseRetryTimer = undefined; + coldPassRetryTimer = undefined; + for (const timer of debounceTimers.values()) clearTimeout(timer); + debounceTimers.clear(); + pendingRerun.clear(); + // Deliberately retain heartbeatTimer: summary worker cleanup is lease-owned + // and plugin disposal finalizes this distiller only after that cleanup has + // settled or reached its shutdown bound. + } + return { start(): void { if (stopped) return; acquire(); }, - stop(): void { - stopped = true; + quiesce, + + ownsLease, + + stop(): Promise { + if (stopPromise) return stopPromise; + quiesce(); + finalized = true; clearTimer(heartbeatTimer); clearTimer(leaseRetryTimer); clearTimer(coldPassRetryTimer); heartbeatTimer = undefined; leaseRetryTimer = undefined; coldPassRetryTimer = undefined; - for (const timer of debounceTimers.values()) clearTimeout(timer); - debounceTimers.clear(); if (leaseHeld) { try { store.releaseLease(instanceId); @@ -1127,6 +1283,11 @@ export function createDistiller(options: DistillerOptions): Distiller { } } leaseHeld = false; + const running = [coldPassPromise, ...inFlight.values()].filter( + (promise): promise is Promise => promise !== undefined, + ); + stopPromise = Promise.allSettled(running).then(() => {}); + return stopPromise; }, onEvent(event: Event): void { @@ -1158,7 +1319,7 @@ export function createDistiller(options: DistillerOptions): Distiller { status(): DistillStatus { const lease = store.leaseStatus(); return { - leaseHeld, + leaseHeld: ownsLease(), coldPass: coldPassState, distilledCount, knownCount, diff --git a/src/opencode-session-recall.ts b/src/opencode-session-recall.ts index 46f7b13..8f796cf 100644 --- a/src/opencode-session-recall.ts +++ b/src/opencode-session-recall.ts @@ -1,4 +1,4 @@ -import type { Plugin } from "@opencode-ai/plugin"; +import type { Plugin, ToolDefinition } from "@opencode-ai/plugin"; import { createOpencodeClient, type Session } from "@opencode-ai/sdk/v2"; import { sessions, type SessionEnrichment } from "./sessions.js"; import { search, DISCOVERY_LIMIT, type SearchDeps, type SemanticSearchConfig } from "./search.js"; @@ -9,7 +9,7 @@ import { systemNudge } from "./hooks/system-nudge.js"; import { autoRecall } from "./hooks/auto-recall.js"; import { compactionRecall } from "./hooks/compaction-recall.js"; import { createFetchGate } from "./fetch-gate.js"; -import { openSqlite } from "./sqlite.js"; +import { openSqlite, type SqliteDb } from "./sqlite.js"; import { openStore, defaultStorePath, @@ -26,6 +26,15 @@ import { createDistiller } from "./distill.js"; import { createSummarizer, parseModelId, type Summarizer } from "./summarize.js"; import { TOOLS, DEFAULTS, optionalString, errmsg, type Limits } from "./types.js"; +// `dispose` was added to the host/plugin contract in @opencode-ai/plugin 1.15.11. +// Keep the source compatible with this repository's older tool-result typings +// while declaring the exact minimum-host hook that the package engine requires. +declare module "@opencode-ai/plugin" { + interface Hooks { + dispose?: () => Promise; + } +} + /** Guarded, Node-free logger: `console` is a std global, but `src/` declares no * types, so reach it defensively. */ function pluginLog(message: string): void { @@ -95,6 +104,18 @@ const server: Plugin = async (ctx, options) => { const nudge = opts.nudge !== false; const autoRecallEnabled = opts.autoRecall === true; const compactionRecallEnabled = opts.compactionRecall === true; + let disposed = false; + let disposePromise: Promise | undefined; + const operations = new Set>(); + + const track = (operation: Promise): Promise => { + operations.add(operation); + void operation.then( + () => operations.delete(operation), + () => operations.delete(operation), + ); + return operation; + }; const clamp = (val: number | undefined, fallback: number, min = 1) => Math.max(min, Math.floor(val ?? fallback)); @@ -196,8 +217,9 @@ const server: Plugin = async (ctx, options) => { // cards-lite built from the session list. const storePath = optionalString(opts.storePath) ?? (await defaultStorePath()); let store: Store | null = null; + let db: SqliteDb | null = null; if (storePath) { - const db = await openSqlite(storePath); + db = await openSqlite(storePath); if (db) store = openStore(db); } @@ -214,13 +236,16 @@ const server: Plugin = async (ctx, options) => { } : { getCards: () => liteCards, revision: () => undefined, degraded: true }; if (!store) { - void discover() - .then((list) => { - liteCards = cardsLiteFromSessions(list as Parameters[0]); - }) - .catch(() => { - // Best-effort; a failed list just leaves cards-lite empty until retried. - }); + void track( + discover() + .then((list) => { + if (!disposed) + liteCards = cardsLiteFromSessions(list as Parameters[0]); + }) + .catch(() => { + // Best-effort; a failed list just leaves cards-lite empty until retried. + }), + ); } // One shared fetch gate gates every SDK call in the query/distill paths so the @@ -255,9 +280,18 @@ const server: Plugin = async (ctx, options) => { gen: EMBED_REPRESENTATION, discover, onColdPassDone: () => { - void summarizer?.runColdPass(); + if (!disposed && summarizer) { + // Summarizer.stop() owns the bounded shutdown of this drain. Do not add + // it to the plugin's general operation set, which is intentionally + // unbounded for DB-capable hooks and distiller work. + void summarizer + .runColdPass() + .catch((error) => pluginLog(`summarizer cold pass failed: ${errmsg(error)}`)); + } + }, + onSessionDistilled: (sessionId) => { + if (!disposed) summarizer?.queue(sessionId); }, - onSessionDistilled: (sessionId) => summarizer?.queue(sessionId), }); if (store && opts.summaries?.enabled === true) { const model = optionalString(opts.summaries.model); @@ -279,7 +313,8 @@ const server: Plugin = async (ctx, options) => { ...(agent != null && { agent }), ...(maxPromptsPerPass != null && { maxPromptsPerPass }), }, - leaseHeld: () => distiller.status().leaseHeld, + ownerToken: instanceId, + leaseHeld: () => distiller.ownsLease(), log: pluginLog, }); } else { @@ -298,32 +333,57 @@ const server: Plugin = async (ctx, options) => { ? { cards: () => store.allCards() } : undefined; + const guardTool = (definition: ToolDefinition): ToolDefinition => ({ + ...definition, + execute: (args, context) => { + if (disposed) { + return Promise.reject(new Error("opencode-session-recall: plugin has been disposed")); + } + return track(definition.execute(args, context)); + }, + }); + + const guardHook = + ( + hook: (...args: TArgs) => Promise, + ): ((...args: TArgs) => Promise) => + async (...args) => { + if (disposed) return; + await track(hook(...args)); + }; + + const nudgeHook = nudge ? systemNudge() : undefined; + const autoRecallHook = autoRecallEnabled ? autoRecall(deps) : undefined; + const compactionHook = compactionRecallEnabled ? compactionRecall(deps) : undefined; + return { tool: { - recall_sessions: sessions(client, unscoped, global, limits, enrichment), - recall: search(client, unscoped, global, limits, deps), - recall_get: get(client, gate), - recall_context: context(client, gate, limits), - recall_messages: messages(client, gate, limits), + recall_sessions: guardTool(sessions(client, unscoped, global, limits, enrichment)), + recall: guardTool(search(client, unscoped, global, limits, deps)), + recall_get: guardTool(get(client, gate)), + recall_context: guardTool(context(client, gate, limits)), + recall_messages: guardTool(messages(client, gate, limits)), }, event: async ({ event }) => { + if (disposed) return; // The plugin `event` hook is typed against the default SDK vintage; the // distiller compiles against the v2 event union the live bus actually // delivers. Bridge the vintage gap at this one boundary. distiller.onEvent(event as unknown as Parameters[0]); }, - ...(nudge && { - "experimental.chat.system.transform": systemNudge(), + ...(nudgeHook && { + "experimental.chat.system.transform": guardHook(nudgeHook), }), - ...(autoRecallEnabled && { - "chat.message": autoRecall(deps), + ...(autoRecallHook && { + "chat.message": guardHook(autoRecallHook), }), - ...(compactionRecallEnabled && { - "experimental.session.compacting": compactionRecall(deps), + ...(compactionHook && { + "experimental.session.compacting": guardHook(compactionHook), }), ...(primary && { // eslint-disable-next-line @typescript-eslint/no-explicit-any -- opencode config type not exported config: async (c: any) => { + if (disposed) return; c.experimental ??= {}; const existing: string[] = c.experimental.primary_tools ?? []; const deduped = new Set(existing); @@ -331,6 +391,26 @@ const server: Plugin = async (ctx, options) => { c.experimental.primary_tools = [...deduped]; }, }), + dispose: () => { + if (disposePromise) return disposePromise; + disposed = true; + // Phase 1 is synchronous: no distill/retry/incremental work can start + // after dispose() returns its promise. The heartbeat deliberately remains + // active so lease-owned summarizer cleanup can finish safely. + distiller.quiesce(); + cards.dispose(); + disposePromise = (async () => { + // Summarizer.stop() is bounded. Its late SDK promises are detached and + // ownership/lease guarded, so phase 2 may safely release the lease once + // this settles even when an SDK request itself never does. + if (summarizer) await Promise.allSettled([summarizer.stop()]); + await distiller.stop(); + await Promise.allSettled([...operations]); + db?.close(); + db = null; + })(); + return disposePromise; + }, }; }; diff --git a/src/summarize.ts b/src/summarize.ts index be22a0e..bcf97e0 100644 --- a/src/summarize.ts +++ b/src/summarize.ts @@ -36,6 +36,10 @@ const DEFAULT_BATCH_SIZE = 15; const DEFAULT_MAX_PROMPTS_PER_PASS = 200; const DEFAULT_POLITENESS_MS = 250; const DEFAULT_PROMPT_TIMEOUT_MS = 60_000; +/** Disposal may detach a stuck SDK request after this bound. The request itself + * cannot be cancelled by the SDK; lease/title guards make its late settlement + * incapable of touching SQLite or another instance's worker. */ +const DEFAULT_SHUTDOWN_TIMEOUT_MS = 15_000; const DEFAULT_IDLE_DEBOUNCE_MS = 3_000; /** Abort a drain after this many prompts fail in a row: a misconfigured model * must not burn the whole per-pass budget. Latches until the next cold pass. */ @@ -92,6 +96,9 @@ export type SummarizerDeps = { store: Store; gate: FetchGate; config: SummariesConfig; + /** Unique to this plugin instance; embedded in worker titles and used to keep + * normal cleanup scoped to workers this instance created. */ + ownerToken: string; /** Only the distill-lease holder writes; checked before every persisted write. */ leaseHeld: () => boolean; log?: (message: string) => void; @@ -101,6 +108,7 @@ export type SummarizerDeps = { batchSize?: number; politenessMs?: number; promptTimeoutMs?: number; + shutdownTimeoutMs?: number; idleDebounceMs?: number; }; @@ -111,12 +119,34 @@ export type Summarizer = { /** Debounced incremental re-summarize of one session (the idle-debounce path); * the drain skips it when the content hash is unchanged. */ queue(sessionId: string): void; - stop(): void; + /** Stop accepting work and settle the active serialized drain. */ + stop(): Promise; status(): { summarized: number; lastError?: string }; }; type Timer = ReturnType; +type TimedResult = { timedOut: false; value: T } | { timedOut: true }; + +async function settleWithin(promise: Promise, timeoutMs: number): Promise> { + let timer: Timer | undefined; + const timeout = new Promise>((resolve) => { + timer = setTimeout(() => resolve({ timedOut: true }), timeoutMs); + }); + try { + return await Promise.race([ + promise.then((value): TimedResult => ({ timedOut: false, value })), + timeout, + ]); + } finally { + if (timer) clearTimeout(timer); + } +} + +export function summarizerWorkerTitle(ownerToken: string): string { + return `${SUMMARIZER_SENTINEL} owner=${ownerToken}`; +} + // ── Text helpers ───────────────────────────────────────────────────────────── function cap(text: string, limit: number): string { @@ -235,13 +265,14 @@ function replyText(parts: Part[]): string { // ── Summarizer ─────────────────────────────────────────────────────────────── export function createSummarizer(deps: SummarizerDeps): Summarizer { - const { client, store, gate, config, leaseHeld } = deps; + const { client, store, gate, config, leaseHeld, ownerToken } = deps; const now = deps.now ?? Date.now; const log = deps.log; const rev = deps.rev ?? SUMMARY_REV; const batchSize = Math.max(1, deps.batchSize ?? DEFAULT_BATCH_SIZE); const politenessMs = Math.max(0, deps.politenessMs ?? DEFAULT_POLITENESS_MS); const promptTimeoutMs = Math.max(1, deps.promptTimeoutMs ?? DEFAULT_PROMPT_TIMEOUT_MS); + const shutdownTimeoutMs = Math.max(1, deps.shutdownTimeoutMs ?? DEFAULT_SHUTDOWN_TIMEOUT_MS); const idleDebounceMs = Math.max(0, deps.idleDebounceMs ?? DEFAULT_IDLE_DEBOUNCE_MS); const maxPromptsPerPass = Math.max(1, config.maxPromptsPerPass ?? DEFAULT_MAX_PROMPTS_PER_PASS); @@ -263,24 +294,50 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { const inFlight = new Set(); const attempts = new Map(); let drainPromise: Promise | null = null; + let stopPromise: Promise | undefined; const debounceTimers = new Map(); + const ownedWorkers = new Set(); + const workerTitle = summarizerWorkerTitle(ownerToken); // ── Worker session lifecycle ── // A fresh worker per batch (create, prompt once, delete): create/delete are // unbilled and this keeps every batch's context clean with zero accumulation. + async function leaseSdk( + label: string, + allowWhileStopped: boolean, + operation: () => Promise, + ): Promise { + if ((!allowWhileStopped && stopped) || !leaseHeld()) return undefined; + const result = await settleWithin( + gate.runBackground(() => { + // A gate permit may arrive after shutdown or an involuntary lease loss. + if ((!allowWhileStopped && stopped) || !leaseHeld()) return Promise.resolve(undefined); + return operation(); + }), + shutdownTimeoutMs, + ); + if (result.timedOut) { + logMsg(`${label} timed out after ${shutdownTimeoutMs}ms; late SDK settlement detached`); + return undefined; + } + return result.value; + } + async function createWorker(): Promise { try { // Probe the deny-all permission ruleset once; if the server rejects the // shape, remember that and create plainly thereafter (never let a rejected // ruleset silently disable summaries). if (permissionMode !== "without") { - const resp = await gate.runBackground(() => - client.session.create({ title: SUMMARIZER_SENTINEL, permission: DENY_ALL_PERMISSION }), + const resp = await leaseSdk("worker create", false, () => + client.session.create({ title: workerTitle, permission: DENY_ALL_PERMISSION }), ); + if (!resp) return null; const created = resp.data as Session | undefined; if (!resp.error && created?.id) { permissionMode = "with"; + ownedWorkers.add(created.id); return created.id; } if (permissionMode === undefined) { @@ -288,10 +345,12 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { logMsg("worker permission ruleset rejected; relying on tool-disable + exclusion"); } } - const resp = await gate.runBackground(() => - client.session.create({ title: SUMMARIZER_SENTINEL }), + const resp = await leaseSdk("worker create", false, () => + client.session.create({ title: workerTitle }), ); + if (!resp) return null; const created = resp.data as Session | undefined; + if (created?.id) ownedWorkers.add(created.id); return created?.id ?? null; } catch (error) { lastError = errmsg(error); @@ -299,17 +358,19 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { } } - async function deleteWorker(sessionID: string): Promise { + async function deleteOwnedWorker(sessionID: string): Promise { + if (!ownedWorkers.has(sessionID)) return; try { - await gate.runBackground(() => client.session.delete({ sessionID })); + await leaseSdk("worker delete", true, () => client.session.delete({ sessionID })); } catch { // Best-effort; a lingering sentinel session is excluded everywhere. } } - async function abortWorker(sessionID: string): Promise { + async function abortOwnedWorker(sessionID: string): Promise { + if (!ownedWorkers.has(sessionID)) return; try { - await gate.runBackground(() => client.session.abort({ sessionID })); + await leaseSdk("worker abort", true, () => client.session.abort({ sessionID })); } catch { // Best-effort. } @@ -319,13 +380,20 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { * than adopting one whose accumulated context is unknown. Runs once. */ async function deleteOrphans(): Promise { try { - const resp = await gate.runBackground(() => + const resp = await leaseSdk("worker orphan list", false, () => client.session.list({ search: SUMMARIZER_SENTINEL, limit: 100 }), ); + if (!resp || stopped || !leaseHeld()) return; const rows = Array.isArray(resp.data) ? (resp.data as Session[]) : []; for (const row of rows) { + // Ownership can change while list/delete is in flight. Re-check after + // every await and immediately before each destructive request. + if (stopped || !leaseHeld()) return; if (isSummarizerTitle(row.title) && typeof row.id === "string" && row.id) { - await deleteWorker(row.id); + await leaseSdk("orphan worker delete", false, () => + client.session.delete({ sessionID: row.id }), + ); + if (stopped || !leaseHeld()) return; } } } catch { @@ -342,6 +410,7 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { const workerId = await createWorker(); if (!workerId) return new Map(); try { + if (stopped || !leaseHeld()) return new Map(); const { text, keyToSession } = renderBatch(cards); const reply = await promptWorker(workerId, text); if (reply == null) return new Map(); @@ -353,17 +422,15 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { } return out; } finally { - await deleteWorker(workerId); + // Only ids created by this owner token enter ownedWorkers; normal cleanup + // can therefore never target another instance's worker. + await deleteOwnedWorker(workerId); } } async function promptWorker(workerId: string, text: string): Promise { - let timer: Timer | undefined; - const timeout = new Promise<"timeout">((resolve) => { - timer = setTimeout(() => resolve("timeout"), promptTimeoutMs); - }); try { - const outcome = await Promise.race([ + const outcome = await settleWithin( client.session.prompt({ sessionID: workerId, model: { providerID: config.providerID, modelID: config.modelID }, @@ -374,25 +441,24 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { tools: DISABLE_ALL_TOOLS, parts: [{ type: "text", text }], }), - timeout, - ]); - if (outcome === "timeout") { + promptTimeoutMs, + ); + if (outcome.timedOut) { // Stop the generation so the timeout caps SPEND, not just our waiting. - await abortWorker(workerId); + await abortOwnedWorker(workerId); lastError = "summary prompt timed out"; return null; } - if (outcome.error || !outcome.data) { - lastError = outcome.error ? errmsg(outcome.error) : "empty prompt response"; + const response = outcome.value; + if (response.error || !response.data) { + lastError = response.error ? errmsg(response.error) : "empty prompt response"; return null; } - const data = outcome.data as { parts?: Part[] }; + const data = response.data as { parts?: Part[] }; return replyText(Array.isArray(data.parts) ? data.parts : []); } catch (error) { lastError = errmsg(error); return null; - } finally { - if (timer) clearTimeout(timer); } } @@ -443,6 +509,7 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { if (!cleanedOrphans) { cleanedOrphans = true; await deleteOrphans(); + if (stopped || !leaseHeld()) return; } let prompts = 0; while ( @@ -478,7 +545,7 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { prompts++; const summaries = await promptBatchFor(cards); - if (!leaseHeld() || stopped) { + if (stopped || !leaseHeld()) { for (const id of batchIds) inFlight.delete(id); break; } @@ -547,10 +614,24 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { ); }, - stop(): void { + stop(): Promise { + if (stopPromise) return stopPromise; stopped = true; for (const timer of debounceTimers.values()) clearTimeout(timer); debounceTimers.clear(); + pending.length = 0; + pendingSet.clear(); + const active = drainPromise; + stopPromise = active + ? settleWithin(active, shutdownTimeoutMs).then((result) => { + if (result.timedOut) { + logMsg( + `shutdown timed out after ${shutdownTimeoutMs}ms; late SDK work is lease-guarded and detached`, + ); + } + }) + : Promise.resolve(); + return stopPromise; }, status() { diff --git a/test/cards.test.ts b/test/cards.test.ts index 206da85..db9de3f 100644 --- a/test/cards.test.ts +++ b/test/cards.test.ts @@ -428,6 +428,43 @@ describe("cards semantic persistence", () => { db.close(); }); + it("cancels deferred semantic warm-up when disposed", async () => { + let ready = false; + let resolveReady!: () => void; + const readyPromise = new Promise((resolve) => { + resolveReady = () => { + ready = true; + resolve(); + }; + }); + const embed = vi.fn(() => Float32Array.from([1, 0])); + const embedder = { + get ready() { + return ready; + }, + embed, + }; + const { db, store } = await freshStore(); + store.upsertCard(makeCard("s1", { inventory: `alpha topic ${SUBST}` })); + store.setMeta("cards_rev", "1"); + const runtime = createCardsRuntime({ + source: persistentSource(store), + embedder, + semanticWeight: 0.5, + semanticModel: "model-x", + semanticReady: readyPromise, + }); + runtime.rank(parseQuery("alpha"), {}); // load the warm store before model readiness + + runtime.dispose(); + db.close(); + resolveReady(); + await readyPromise; + await Promise.resolve(); + + expect(embed).not.toHaveBeenCalled(); + }); + it("reloads on a cross-process vectors_rev bump, but not on its own vector write", async () => { // Two handles on one store file (mixed-version skew): the runtime reads // through storeB; storeA stands in for another process. Vector writes do NOT diff --git a/test/distill.test.ts b/test/distill.test.ts index 02b5e6e..1ef1f84 100644 --- a/test/distill.test.ts +++ b/test/distill.test.ts @@ -1,4 +1,4 @@ -import { afterAll, describe, expect, it } from "vitest"; +import { afterAll, describe, expect, it, vi } from "vitest"; import { mkdtempSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -66,7 +66,11 @@ type SdkCalls = { function makeDistillFake( graph: Graph, - opts: { throwOnce?: Set } = {}, + opts: { + throwOnce?: Set; + nonArray?: Set; + metadataErrorOnce?: Set; + } = {}, ): { client: OpencodeClient; sdk: SdkCalls } { const sdk: SdkCalls = { list: 0, get: 0, messages: [] }; const threw = new Set(); @@ -78,6 +82,10 @@ function makeDistillFake( }, get: async ({ sessionID }: { sessionID: string }) => { sdk.get++; + if (opts.metadataErrorOnce?.has(sessionID) && !threw.has(`meta:${sessionID}`)) { + threw.add(`meta:${sessionID}`); + return { error: apiFailure(`metadata transport failed: ${sessionID}`) }; + } const found = graph.sessions.find((s) => s.id === sessionID); return found ? { data: found } : { error: apiFailure(`not found: ${sessionID}`) }; }, @@ -91,6 +99,9 @@ function makeDistillFake( threw.add(params.sessionID); throw new Error(`throw once: ${params.sessionID}`); } + if (opts.nonArray?.has(params.sessionID)) { + return { data: { messages: "not-an-array" } }; + } const data = graph.messagesBySession[params.sessionID] ?? []; const { items, nextCursor } = paginateBundles( data, @@ -543,6 +554,322 @@ describe("fetchMessagePage", () => { // ── Cold pass ──────────────────────────────────────────────────────────────── describe("cold pass", () => { + function malformedBundle(sessionId: string): MessageBundle { + return { + info: userMessage(`m-${sessionId}`, sessionId, 100), + parts: undefined, + } as unknown as MessageBundle; + } + + it("quarantines a malformed session and continues processing the pass", async () => { + const bad = session("bad", "Bad", PROJECT_DIR, 3000); + const good = session("good", "Good", PROJECT_DIR, 2000); + const graph: Graph = { + sessions: [bad, good], + messagesBySession: { + bad: [malformedBundle("bad")], + good: [ + bundle(userMessage("m-good", "good", 100), [ + textPart("p-good", "good", "m-good", "valid session content"), + ]), + ], + }, + }; + const { client } = makeDistillFake(graph); + const { db, store } = await freshStore(); + const { gate } = makeSpyGate(); + const logs: string[] = []; + const distiller = createDistiller({ + client, + store, + gate, + limits: { ...TEST_LIMITS, distillConcurrency: 1 }, + instanceId: "malformed-continue", + log: (message) => logs.push(message), + }); + + distiller.start(); + await waitFor(() => distiller.status().coldPass === "done"); + + expect(store.getCard("bad")).toBeUndefined(); + expect(store.getCard("good")?.distillState).toBe("full"); + expect(logs.some((line) => line.includes("session bad quarantined at timeUpdated 3000:"))).toBe( + true, + ); + await distiller.stop(); + db.close(); + }); + + it("skips an unchanged quarantined session on a later cold-pass retry", async () => { + const graph: Graph = { + sessions: [ + session("bad", "Bad", PROJECT_DIR, 3000), + session("flaky", "Flaky", PROJECT_DIR, 2000), + session("good", "Good", PROJECT_DIR, 1000), + ], + messagesBySession: { + bad: [malformedBundle("bad")], + flaky: [ + bundle(userMessage("m-flaky", "flaky", 100), [ + textPart("p-flaky", "flaky", "m-flaky", "flaky content"), + ]), + ], + good: [ + bundle(userMessage("m-good", "good", 100), [ + textPart("p-good", "good", "m-good", "good content"), + ]), + ], + }, + }; + const { client, sdk } = makeDistillFake(graph, { throwOnce: new Set(["flaky"]) }); + const { db, store } = await freshStore(); + const { gate } = makeSpyGate(); + const distiller = createDistiller({ + client, + store, + gate, + limits: { ...TEST_LIMITS, distillConcurrency: 1 }, + instanceId: "malformed-skip", + coldPassRetryMs: 15, + }); + + distiller.start(); + await waitFor(() => distiller.status().coldPass === "done"); + + expect(sdk.list).toBe(2); + expect(sdk.messages.filter((call) => call.sessionID === "bad")).toHaveLength(1); + expect(store.getCard("good")?.distillState).toBe("full"); + await distiller.stop(); + db.close(); + }); + + it("preserves an existing card and FTS rows for a non-array message payload", async () => { + const current = session("bad", "Bad", PROJECT_DIR, 2000); + const old = session("bad", "Bad", PROJECT_DIR, 1000); + const oldMessages = [ + bundle(userMessage("m-old", "bad", 100), [ + textPart("p-old", "bad", "m-old", "preserved_fts_marker"), + ]), + ]; + const graph: Graph = { sessions: [current], messagesBySession: { bad: oldMessages } }; + const { client } = makeDistillFake(graph, { nonArray: new Set(["bad"]) }); + const { db, store } = await freshStore(); + const seeded = deriveCard({ + session: metaFromSession(old), + messages: oldMessages, + parentById: new Map([["bad", null]]), + }); + store.replaceSessionParts("bad", seeded.rows, seeded.card); + const { gate } = makeSpyGate(); + const distiller = createDistiller({ + client, + store, + gate, + limits: { ...TEST_LIMITS, distillConcurrency: 1 }, + instanceId: "non-array-preserve", + }); + + distiller.start(); + await waitFor(() => distiller.status().coldPass === "done"); + + expect(store.getCard("bad")).toEqual(seeded.card); + expect(ftsSessions(store.ftsSearch({ strong: ["preserved_fts_marker"], weak: [] }))).toEqual([ + "bad", + ]); + await distiller.stop(); + db.close(); + }); + + it("does not arm a cold-pass retry when discovery fails after stop", async () => { + vi.useFakeTimers(); + const { db, store } = await freshStore(); + try { + let rejectDiscovery!: (error: Error) => void; + const discovery = new Promise((_, reject) => { + rejectDiscovery = reject; + }); + const discover = vi.fn(() => discovery); + const { client } = makeDistillFake({ sessions: [], messagesBySession: {} }); + const { gate } = makeSpyGate(); + const distiller = createDistiller({ + client, + store, + gate, + limits: TEST_LIMITS, + instanceId: "late-discovery-failure", + coldPassRetryMs: 100, + discover, + }); + + distiller.start(); + await Promise.resolve(); + await Promise.resolve(); + expect(discover).toHaveBeenCalledOnce(); + const stopping = distiller.stop(); + rejectDiscovery(new Error("late discovery failure")); + await stopping; + + expect(vi.getTimerCount()).toBe(0); + expect(discover).toHaveBeenCalledOnce(); + } finally { + db.close(); + vi.useRealTimers(); + } + }); + + it("does not fetch another message page or write progress after stop during a delay", async () => { + vi.useFakeTimers(); + const { db, store } = await freshStore(); + try { + const meta = session("s1", "Paged", PROJECT_DIR, 2000); + let messageCalls = 0; + const first = bundle(userMessage("m1", "s1", 100), [ + textPart("p1", "s1", "m1", "first page"), + ]); + const client = { + session: { + messages: async () => { + messageCalls++; + return messagesResponse( + messageCalls === 1 ? [first] : [], + messageCalls === 1 ? "next" : null, + ); + }, + }, + } as unknown as OpencodeClient; + const { gate } = makeSpyGate(); + const distiller = createDistiller({ + client, + store, + gate, + limits: { ...TEST_LIMITS, distillDelayMs: 100 }, + instanceId: "stop-pagination", + discover: async () => [meta], + }); + + distiller.start(); + for (let i = 0; i < 8 && messageCalls === 0; i++) await Promise.resolve(); + expect(messageCalls).toBe(1); + const stopping = distiller.stop(); + await vi.advanceTimersByTimeAsync(100); + await stopping; + + expect(messageCalls).toBe(1); + expect(store.getCard("s1")).toBeUndefined(); + expect(store.getMeta("coldpass_cursor")).toBeUndefined(); + } finally { + db.close(); + vi.useRealTimers(); + } + }); + + it("does not start a message request that was queued in the gate when stopped", async () => { + const { db, store } = await freshStore(); + try { + const meta = session("s1", "Queued", PROJECT_DIR, 2000); + let messageCalls = 0; + let releaseQuery!: () => void; + const queryBlocked = new Promise((resolve) => { + releaseQuery = resolve; + }); + let queryStarted!: () => void; + const queryIsRunning = new Promise((resolve) => { + queryStarted = resolve; + }); + let messageQueued!: () => void; + const messageIsQueued = new Promise((resolve) => { + messageQueued = resolve; + }); + const realGate = createFetchGate({ concurrency: 1 }); + let backgroundCalls = 0; + const gate: FetchGate = { + runQuery: (fn) => realGate.runQuery(fn), + runBackground: (fn) => { + backgroundCalls++; + if (backgroundCalls === 2) messageQueued(); + return realGate.runBackground(fn); + }, + activeQueries: () => realGate.activeQueries(), + }; + const client = { + session: { + messages: async () => { + messageCalls++; + return messagesResponse([], null); + }, + }, + } as unknown as OpencodeClient; + const distiller = createDistiller({ + client, + store, + gate, + limits: TEST_LIMITS, + instanceId: "stop-gate-queue", + discover: async () => { + void gate.runQuery(async () => { + queryStarted(); + await queryBlocked; + }); + return [meta]; + }, + }); + + distiller.start(); + await queryIsRunning; + await messageIsQueued; + const stopping = distiller.stop(); + releaseQuery(); + await stopping; + + expect(messageCalls).toBe(0); + expect(store.getCard("s1")).toBeUndefined(); + expect(store.getMeta("coldpass_cursor")).toBeUndefined(); + } finally { + db.close(); + } + }); + + it("retries a quarantined session after its update timestamp changes", async () => { + const bad = session("bad", "Bad", PROJECT_DIR, 3000); + const graph: Graph = { + sessions: [bad, session("flaky", "Flaky", PROJECT_DIR, 2000)], + messagesBySession: { + bad: [malformedBundle("bad")], + flaky: [ + bundle(userMessage("m-flaky", "flaky", 100), [ + textPart("p-flaky", "flaky", "m-flaky", "flaky content"), + ]), + ], + }, + }; + const { client, sdk } = makeDistillFake(graph, { throwOnce: new Set(["flaky"]) }); + const { db, store } = await freshStore(); + const { gate } = makeSpyGate(); + const distiller = createDistiller({ + client, + store, + gate, + limits: { ...TEST_LIMITS, distillConcurrency: 1 }, + instanceId: "malformed-update", + coldPassRetryMs: 100, + }); + + distiller.start(); + await waitFor(() => distiller.status().lastError != null); + bad.time.updated = 4000; + graph.messagesBySession.bad = [ + bundle(userMessage("m-bad-fixed", "bad", 100), [ + textPart("p-bad-fixed", "bad", "m-bad-fixed", "repaired content"), + ]), + ]; + await waitFor(() => distiller.status().coldPass === "done"); + + expect(sdk.messages.filter((call) => call.sessionID === "bad")).toHaveLength(2); + expect(store.getCard("bad")?.timeUpdated).toBe(4000); + await distiller.stop(); + db.close(); + }); + it("distills newest-updated-first, skips up-to-date cards, routes every fetch through the gate", async () => { const s1 = session("s1", "Alpha", PROJECT_DIR, 3000); const s2 = session("s2", "Bravo", PROJECT_DIR, 2000); @@ -922,6 +1249,61 @@ describe("lease", () => { // ── Incremental ────────────────────────────────────────────────────────────── describe("incremental", () => { + it("logs metadata transport errors without changing a valid card and retries on a later event", async () => { + const original = session("s1", "Alpha", PROJECT_DIR, 3000); + const graph: Graph = { + sessions: [original], + messagesBySession: { + s1: [ + bundle(userMessage("m1", "s1", 100), [ + textPart("p1", "s1", "m1", "preserved metadata transport marker"), + ]), + ], + }, + }; + const { client, sdk } = makeDistillFake(graph, { + metadataErrorOnce: new Set(["s1"]), + }); + const { db, store } = await freshStore(); + const { gate } = makeSpyGate(); + const logs: string[] = []; + const distiller = createDistiller({ + client, + store, + gate, + limits: TEST_LIMITS, + instanceId: "metadata-transport", + idleDebounceMs: 5, + log: (message) => logs.push(message), + }); + distiller.start(); + await waitFor(() => distiller.status().coldPass === "done"); + const preserved = store.getCard("s1"); + + graph.sessions[0] = session("s1", "Alpha updated", PROJECT_DIR, 4000); + graph.messagesBySession.s1 = [ + bundle(userMessage("m2", "s1", 200), [ + textPart("p2", "s1", "m2", "fresh metadata transport marker"), + ]), + ]; + distiller.onEvent(idleEvent("s1")); + await waitFor(() => sdk.get === 1); + await new Promise((resolve) => setTimeout(resolve, 10)); + + expect(store.getCard("s1")).toEqual(preserved); + expect(logs.some((line) => line.includes("session s1 re-distill failed:"))).toBe(true); + expect(logs.some((line) => line.includes("metadata transport failed: s1"))).toBe(true); + + // A later idle event retries normally; the transport failure was neither + // interpreted as deletion nor quarantined as malformed session data. + distiller.onEvent(idleEvent("s1")); + await waitFor(() => store.getCard("s1")?.timeUpdated === 4000); + expect(sdk.get).toBe(2); + expect(store.getCard("s1")?.summaryHead).toContain("fresh metadata transport marker"); + await distiller.stop(); + db.close(); + }); + it("coalesces an idle burst into a single re-distill", async () => { const graph: Graph = { sessions: [session("s1", "Alpha", PROJECT_DIR, 3000)], diff --git a/test/plugin.test.ts b/test/plugin.test.ts index 9b5660b..e5fdf18 100644 --- a/test/plugin.test.ts +++ b/test/plugin.test.ts @@ -1,12 +1,47 @@ -import { beforeEach, describe, expect, it, vi } from "vitest"; -import { tool, type PluginInput, type ToolDefinition } from "@opencode-ai/plugin"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { tool, type Hooks, type PluginInput, type ToolDefinition } from "@opencode-ai/plugin"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { TOOLS } from "../src/types.js"; -import { PROJECT_DIR } from "./helpers.js"; +import { openSqlite } from "../src/sqlite.js"; +import { openStore } from "../src/store.js"; +import { bundle, PROJECT_DIR, textPart, userMessage } from "./helpers.js"; const createOpencodeClient = vi.hoisted(() => vi.fn((options: unknown) => options)); +const sqliteLifecycle = vi.hoisted(() => ({ closes: 0, postCloseCalls: 0 })); vi.mock("@opencode-ai/sdk/v2", () => ({ createOpencodeClient })); +vi.mock("../src/sqlite.js", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + openSqlite: async (...args: Parameters) => { + const db = await actual.openSqlite(...args); + if (!db) return db; + let closed = false; + return new Proxy(db, { + get(target, property, receiver) { + const value = Reflect.get(target, property, receiver); + if (typeof value !== "function") return value; + if (property === "close") { + return () => { + sqliteLifecycle.closes++; + closed = true; + return value.call(target); + }; + } + return (...methodArgs: unknown[]) => { + if (closed) sqliteLifecycle.postCloseCalls++; + return value.apply(target, methodArgs); + }; + }, + }); + }, + }; +}); + // Mock the semantic embedder so the wiring test never downloads a model or // touches the network: init resolves immediately and the model stays unready. vi.mock("../src/semantic/embedder.js", () => ({ @@ -22,12 +57,19 @@ vi.mock("../src/semantic/embedder.js", () => ({ })); const plugin = await import("../src/opencode-session-recall.js"); +const activeHooks: Hooks[] = []; // Every entry call opens a card store and (with coldPass) starts a background // distiller. Default tests use an in-memory store with the cold pass off so they // exercise only wiring, with no filesystem side effect or leaked timers. -function server(input: PluginInput, opts: Record = {}) { - return plugin.default.server(input, { storePath: ":memory:", coldPass: false, ...opts }); +async function server(input: PluginInput, opts: Record = {}) { + const hooks = await plugin.default.server(input, { + storePath: ":memory:", + coldPass: false, + ...opts, + }); + activeHooks.push(hooks); + return hooks; } function mustTool(definition: ToolDefinition | undefined): ToolDefinition { @@ -66,6 +108,12 @@ function ctx(config: { describe("plugin entry", () => { beforeEach(() => { createOpencodeClient.mockClear(); + sqliteLifecycle.closes = 0; + sqliteLifecycle.postCloseCalls = 0; + }); + + afterEach(async () => { + await Promise.all(activeHooks.splice(0).map((hooks) => hooks.dispose?.())); }); it("registers all tools and strips project scoping only from the unscoped client", async () => { @@ -214,6 +262,281 @@ describe("plugin entry", () => { expect(() => sessionsArgs.parse({ limit: 5 })).toThrow(); }); + it("waits for an in-flight cold-pass fetch before closing SQLite", async () => { + let resolveMessages: ((value: { data: [] }) => void) | undefined; + const messages = vi.fn( + () => + new Promise<{ data: [] }>((resolve) => { + resolveMessages = resolve; + }), + ); + createOpencodeClient + .mockImplementationOnce((options: unknown) => ({ + ...(options as object), + session: { messages, list: vi.fn(async () => ({ data: [] })) }, + })) + .mockImplementationOnce((options: unknown) => ({ + ...(options as object), + experimental: { + session: { + list: vi.fn(async () => ({ + data: [ + { + id: "s1", + title: "T", + directory: PROJECT_DIR, + time: { created: 1, updated: 2 }, + }, + ], + })), + }, + }, + })); + const hooks = await server(ctx({ fetch: vi.fn() }), { coldPass: true }); + await vi.waitFor(() => expect(messages).toHaveBeenCalled()); + + let disposed = false; + const stopping = hooks.dispose?.().then(() => { + disposed = true; + }); + await Promise.resolve(); + expect(disposed).toBe(false); + expect(sqliteLifecycle.closes).toBe(0); + + resolveMessages?.({ data: [] }); + await stopping; + expect(sqliteLifecycle.closes).toBe(1); + expect(sqliteLifecycle.postCloseCalls).toBe(0); + }); + + it("disposes idempotently and rejects tools without touching SQLite afterwards", async () => { + const hooks = await server(ctx({ fetch: vi.fn() }), {}); + + const first = hooks.dispose?.(); + const second = hooks.dispose?.(); + expect(second).toBe(first); + await first; + expect(sqliteLifecycle.closes).toBe(1); + + await hooks.event?.({ + event: { type: "session.idle", properties: { sessionID: "s" } }, + } as never); + await expect(mustTool(hooks.tool?.recall_sessions).execute({}, {} as never)).rejects.toThrow( + "opencode-session-recall: plugin has been disposed", + ); + expect(sqliteLifecycle.closes).toBe(1); + expect(sqliteLifecycle.postCloseCalls).toBe(0); + }); + + it("cancels scheduler timers before closing SQLite", async () => { + vi.useFakeTimers(); + try { + createOpencodeClient + .mockImplementationOnce((options: unknown) => ({ + ...(options as object), + session: { messages: vi.fn(async () => ({ data: [] })) }, + })) + .mockImplementationOnce((options: unknown) => ({ + ...(options as object), + experimental: { session: { list: vi.fn(async () => ({ data: [] })) } }, + })); + const hooks = await server(ctx({ fetch: vi.fn() }), { coldPass: true }); + + await hooks.dispose?.(); + expect(sqliteLifecycle.closes).toBe(1); + await vi.advanceTimersByTimeAsync(20_000); + + expect(sqliteLifecycle.closes).toBe(1); + expect(sqliteLifecycle.postCloseCalls).toBe(0); + } finally { + vi.useRealTimers(); + } + }); + + it("holds the distill lease until active summarizer cleanup finishes", async () => { + const dir = mkdtempSync(join(tmpdir(), "recall-plugin-handoff-")); + const storePath = join(dir, "recall.sqlite"); + let resolvePrompt: ((value: unknown) => void) | undefined; + const prompt = vi.fn( + () => + new Promise((resolve) => { + resolvePrompt = resolve; + }), + ); + const deleteWorker = vi.fn(async () => ({ data: true })); + const messages = vi.fn(async () => ({ + data: [ + bundle(userMessage("m1", "s1", 1), [ + textPart("p1", "s1", "m1", "Summarize this lease handoff session."), + ]), + ], + })); + createOpencodeClient + .mockImplementationOnce((options: unknown) => ({ + ...(options as object), + session: { + messages, + list: vi.fn(async () => ({ data: [] })), + create: vi.fn(async () => ({ data: { id: "worker-1" } })), + prompt, + delete: deleteWorker, + abort: vi.fn(async () => ({ data: true })), + }, + })) + .mockImplementationOnce((options: unknown) => ({ + ...(options as object), + experimental: { + session: { + list: vi.fn(async () => ({ + data: [ + { + id: "s1", + title: "Lease handoff", + directory: PROJECT_DIR, + time: { created: 1, updated: 2 }, + }, + ], + })), + }, + }, + })); + + let rivalDb: Awaited> = null; + try { + const hooks = await server(ctx({ fetch: vi.fn() }), { + storePath, + coldPass: true, + summaries: { enabled: true, model: "test/cheap" }, + }); + await vi.waitFor(() => expect(prompt).toHaveBeenCalledOnce()); + + const stopping = hooks.dispose?.(); + await Promise.resolve(); + rivalDb = await openSqlite(storePath); + if (!rivalDb) throw new Error("failed to open rival store"); + const rival = openStore(rivalDb); + if (!rival) throw new Error("failed to initialize rival store"); + expect(rival.acquireLease("rival", 60_000, "test", 1)).toBe(false); + + resolvePrompt?.({ data: { info: {}, parts: [{ type: "text", text: "[]" }] } }); + await stopping; + expect(deleteWorker).toHaveBeenCalledWith({ sessionID: "worker-1" }); + expect(rival.acquireLease("rival", 60_000, "test", 1)).toBe(true); + } finally { + rivalDb?.close(); + rmSync(dir, { recursive: true, force: true }); + } + }); + + it("quiesces immediately but renews the lease only until blocked cleanup times out", async () => { + vi.useFakeTimers({ now: 1_000_000 }); + const dir = mkdtempSync(join(tmpdir(), "recall-plugin-bounded-cleanup-")); + const storePath = join(dir, "recall.db"); + let resolveDelete: ((value: { data: true }) => void) | undefined; + try { + const coldSession = { + id: "s1", + title: "Cold session", + slug: "cold-session", + directory: PROJECT_DIR, + projectID: "p", + time: { created: 1000, updated: 2000 }, + }; + const getSession = vi.fn(async () => ({ data: coldSession })); + const messages = vi.fn(async () => ({ + data: [ + bundle(userMessage("m1", "s1", 1100), [ + textPart("p1", "s1", "m1", "bounded cleanup source"), + ]), + ], + })); + const deleteWorker = vi.fn( + () => + new Promise<{ data: true }>((resolve) => { + resolveDelete = resolve; + }), + ); + const scopedClient = { + session: { + get: getSession, + messages, + list: vi.fn(async () => ({ data: [] })), + create: vi.fn(async () => ({ data: { id: "blocked-worker" } })), + prompt: vi.fn(async () => ({ + data: { + info: { role: "assistant" }, + parts: [{ type: "text", text: "[]" }], + }, + })), + delete: deleteWorker, + abort: vi.fn(async () => ({ data: true })), + }, + }; + const discover = vi.fn(async () => ({ data: [coldSession] })); + const unscopedClient = { experimental: { session: { list: discover } } }; + createOpencodeClient.mockReturnValueOnce(scopedClient).mockReturnValueOnce(unscopedClient); + + const hooks = await server(ctx({ fetch: vi.fn() }), { + storePath, + coldPass: true, + summaries: { enabled: true, model: "test/cheap" }, + }); + for (let i = 0; i < 100 && deleteWorker.mock.calls.length === 0; i++) { + await Promise.resolve(); + } + expect(deleteWorker).toHaveBeenCalledWith({ sessionID: "blocked-worker" }); + + // Queue incremental work immediately before disposal. Quiescence must + // cancel it synchronously even though summarizer cleanup is still blocked. + await hooks.event?.({ + event: { type: "session.idle", properties: { sessionID: "s1" } }, + } as Parameters>[0]); + let disposed = false; + const stopping = hooks.dispose?.().then(() => { + disposed = true; + }); + expect(stopping).toBeDefined(); + + const rivalDb = await openSqlite(storePath); + if (!rivalDb) throw new Error("openSqlite returned null for rival"); + const rival = openStore(rivalDb); + if (!rival) throw new Error("openStore returned null for rival"); + const initialLease = rival.leaseStatus(); + expect(initialLease).toBeDefined(); + expect(rival.acquireLease("rival", 60_000, "test", 1)).toBe(false); + + await vi.advanceTimersByTimeAsync(10_000); + const renewedLease = rival.leaseStatus(); + expect(renewedLease?.heartbeat).toBeGreaterThan(initialLease?.heartbeat ?? 0); + expect(disposed).toBe(false); + expect(rival.acquireLease("rival", 60_000, "test", 1)).toBe(false); + expect(discover).toHaveBeenCalledTimes(1); + expect(messages).toHaveBeenCalledTimes(1); + expect(getSession).not.toHaveBeenCalled(); + + // Summarizer shutdown is bounded. Once its timeout expires, distiller + // finalization releases the lease and disposal closes the old DB. + await vi.advanceTimersByTimeAsync(5_001); + await stopping; + expect(disposed).toBe(true); + expect(rival.acquireLease("rival", 60_000, "test", 1)).toBe(true); + expect(discover).toHaveBeenCalledTimes(1); + expect(messages).toHaveBeenCalledTimes(1); + expect(getSession).not.toHaveBeenCalled(); + expect(sqliteLifecycle.postCloseCalls).toBe(0); + + // The SDK promise itself cannot be cancelled. Its late settlement only + // releases the fetch-gate permit and cannot touch SQLite or another worker. + resolveDelete?.({ data: true }); + await Promise.resolve(); + expect(sqliteLifecycle.postCloseCalls).toBe(0); + rivalDb.close(); + } finally { + vi.useRealTimers(); + rmSync(dir, { recursive: true, force: true }); + } + }); + it("fails clearly if SDK internals needed for transport extraction change", async () => { await expect(server({ client: {} } as unknown as PluginInput, {})).rejects.toThrow( "SDK internals changed", diff --git a/test/summarize.test.ts b/test/summarize.test.ts index 9f9009a..e6d594e 100644 --- a/test/summarize.test.ts +++ b/test/summarize.test.ts @@ -1,4 +1,4 @@ -import { afterAll, describe, expect, it } from "vitest"; +import { afterAll, describe, expect, it, vi } from "vitest"; import { mkdtempSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -16,6 +16,7 @@ import { createSummarizer, parseModelId, parseSummaryReply, + summarizerWorkerTitle, type Summarizer, } from "../src/summarize.js"; import { @@ -119,6 +120,7 @@ function makeSummarizer( store, gate, config: { providerID: "test", modelID: "cheap" }, + ownerToken: "test-owner", leaseHeld: () => true, politenessMs: 0, idleDebounceMs: 0, @@ -306,6 +308,11 @@ describe("summarizer worker lifecycle", () => { expect(client.calls.prompts.length).toBe(3); expect(client.calls.creates.length).toBe(3); // one worker per batch + expect(client.calls.creates.map((call) => call.title)).toEqual([ + summarizerWorkerTitle("test-owner"), + summarizerWorkerTitle("test-owner"), + summarizerWorkerTitle("test-owner"), + ]); expect(client.calls.deletes.length).toBe(3); // each disposed expect(client.liveWorkers()).toHaveLength(0); }); @@ -325,6 +332,46 @@ describe("summarizer worker lifecycle", () => { expect(client.liveWorkers()).toHaveLength(0); // orphans + this batch's worker all gone }); + it("does not delete a new holder's worker when an orphan list returns after lease loss", async () => { + const store = await freshStore(); + store.upsertCard(fullCard("c1")); + const gate = createFetchGate({ concurrency: 2 }); + let leaseHeld = true; + let resolveList!: (value: { data: unknown[] }) => void; + const listResult = new Promise<{ data: unknown[] }>((resolve) => { + resolveList = resolve; + }); + const deleteWorker = vi.fn(async () => ({ data: true })); + const createWorker = vi.fn(async () => ({ data: { id: "old-worker" } })); + const listWorkers = vi.fn(async () => listResult); + const client = { + session: { + list: listWorkers, + delete: deleteWorker, + create: createWorker, + }, + } as unknown as OpencodeClient; + const summarizer = makeSummarizer(store, client, gate, { + ownerToken: "old-owner", + leaseHeld: () => leaseHeld, + shutdownTimeoutMs: 100, + }); + + const run = summarizer.runColdPass(); + while (listWorkers.mock.calls.length === 0) { + await Promise.resolve(); + } + leaseHeld = false; + resolveList({ + data: [session("new-worker", summarizerWorkerTitle("new-owner"), PROJECT_DIR, 4000)], + }); + await run; + + expect(deleteWorker).not.toHaveBeenCalled(); + expect(createWorker).not.toHaveBeenCalled(); + await summarizer.stop(); + }); + it("disables tools and applies a deny-all permission on the worker prompt", async () => { const store = await freshStore(); store.upsertCard(fullCard("c1")); @@ -365,6 +412,28 @@ describe("summarizer worker lifecycle", () => { }); describe("summarizer drain (budget, latch, gating)", () => { + it("waits for an active prompt to settle when stopped", async () => { + const store = await freshStore(); + store.upsertCard(fullCard("c1")); + const gate = createFetchGate({ concurrency: 2 }); + const client = makeSummarizerClient(() => ({ text: "[]", delayMs: 30 })); + const summarizer = makeSummarizer(store, client.client, gate); + + const run = summarizer.runColdPass(); + while (client.calls.prompts.length === 0) + await new Promise((resolve) => setTimeout(resolve, 1)); + let stopped = false; + const stopping = summarizer.stop().then(() => { + stopped = true; + }); + await Promise.resolve(); + + expect(stopped).toBe(false); + await Promise.all([run, stopping]); + expect(stopped).toBe(true); + expect(store.getCard("c1")?.nlSummary).toBe(""); + }); + it("summarizes every needing card, then skips them on the content-hash gate", async () => { const store = await freshStore(); store.upsertCard(fullCard("c1")); diff --git a/test/tools.test.ts b/test/tools.test.ts index c993d9d..d7ef17f 100644 --- a/test/tools.test.ts +++ b/test/tools.test.ts @@ -163,14 +163,13 @@ describe("recall_messages", () => { ); expect(errorOut.error).toContain("Unauthorized"); - // A session with no data now returns an empty page rather than an error. + // A successful response without an array body is malformed, not an empty page. const noData = makeFakeHarness({ noMessageData: new Set(["s-current"]) }); - const noDataOut = await runTool( + const noDataOut = await runTool( messagesTool(noData.client, gate, TEST_LIMITS), {}, ); - expect(noDataOut.ok).toBe(true); - expect(noDataOut.pagination.returned).toBe(0); + expect(noDataOut.error).toBe("successful message response was not an array"); }); it("survives raw MCP-bypass args (undefined role/limit must not filter everything)", async () => { @@ -383,7 +382,7 @@ describe("recall_context", () => { sessionID: "s-current", messageID: "m-current-1", }); - expect(noDataOut.error).toBe("No messages returned"); + expect(noDataOut.error).toBe("successful message response was not an array"); }); it("survives raw MCP-bypass args (undefined window must not break slice bounds)", async () => { From edf4738a8c80f35adb7e3a0b91128fef788c8236 Mon Sep 17 00:00:00 2001 From: Rafi Khardalian Date: Tue, 25 Aug 2026 21:52:27 -0700 Subject: [PATCH 2/5] fix(shutdown): address review blockers and issues on PR #3's quiesce/finalize work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - B1: drop the engines.opencode constraint — opencode hard-enforces engines and would refuse to load on older hosts; dispose is an optional hook, so older hosts simply never call it. peerDependencies range unchanged. - B2: bound dispose(). settleWithin moved to types.ts and wraps both the distiller stop and the tracked-operations wait (2.5s each, after the summarizer's own 15s bound: <20s worst case). A timeout does NOT skip db.close(); detached continuations are fenced by stopped/ finalized/lease guards before every store read/write. New test proves dispose completes when an SDK request never settles. - B3: fetchMessagePage strictness is opt-in (strict?: boolean). Only distiller call sites pass strict:true; the query path keeps the documented "non-array data = empty page, ok:true" contract. Restored the original recall_messages/recall_context expectations and added strict/lenient unit tests. - B4: owned-worker cleanup is no longer lease-gated. Remote worker ownership is a separate authority from the SQLite writer lease; delete/abort of an ownedWorkers member goes through a bounded ungated runner, while creates, prompts, and orphan sweeps stay lease-gated. Test: lease lost mid-batch still deletes the worker. - B5: stale-writer window closed. Every distiller write batch (full/append/cold-pass replace, rollups, deletes) re-verifies authoritative ownership via ownsLease() immediately before the write, one leaseStatus() read per batch. Test simulates a rival takeover between fetch and write. - I1: ownsLease() no longer self-demotes on its own stale-looking heartbeat; ownership is lost only when the row names someone else or is gone. Heartbeat freshness is the rival's acquire-side concern. - I2: the deriveCard quarantine catch is narrowed to data-shape errors (MalformedSessionError/TypeError); anything else aborts the pass and surfaces in lastError. DistillStatus gains quarantinedCount. - I3: status() reports the cached lease flag (side-effect-free); ownsLease() is reserved for write gates. - I4: bumped @opencode-ai/plugin to 1.18.23, which declares dispose in Hooks, and deleted the local module augmentation. Test-side fallout (execute now returns ToolResult) handled with a toolResultText narrowing helper. - I5: disposed-tool calls return the JSON { ok:false, error } shape instead of rejecting. - I6: ownedWorkers entries are removed once cleanup settles (a timed-out delete keeps the id owned for the next holder's orphan sweep). - I7: fetchNewMessages wraps its page walk in the same MalformedSessionError conversion as fetchSessionMessages. - I8: CHANGELOG Unreleased entry for the change-set. Based on PR #3 by @kernel-oops with maintainer fixes. --- CHANGELOG.md | 49 +++++ package-lock.json | 308 +++++++++++++++++++++++++++- package.json | 5 +- src/distill.ts | 92 +++++++-- src/opencode-session-recall.ts | 50 +++-- src/summarize.ts | 74 ++++--- src/types.ts | 29 +++ test/distill.test.ts | 149 ++++++++++++++ test/eval/harness.ts | 3 +- test/eval/relevance.test.ts | 3 +- test/eval/semantic-plumbing.test.ts | 4 +- test/eval/semantic.test.ts | 5 +- test/helpers.ts | 12 +- test/perf.test.ts | 3 +- test/plugin.test.ts | 66 +++++- test/summarize.test.ts | 31 ++- test/tools.test.ts | 10 +- 17 files changed, 798 insertions(+), 95 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c2288a..6a4d112 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,55 @@ All notable changes to this project are documented here. This project follows [Conventional Commits](https://www.conventionalcommits.org/) and [Semantic Versioning](https://semver.org/). +## Unreleased + +Based on PR #3 by @kernel-oops with maintainer fixes. + +### Added + +- **Bounded shutdown via the optional `dispose` hook.** On hosts that call + `dispose` (opencode ≥ 1.15.11), the plugin now shuts down in two phases: + an immediate quiesce (no new distill/summary/hook work starts) followed by a + bounded drain of in-flight work before SQLite closes. Every wait is + timeout-capped — a never-settling SDK request is detached, not awaited + forever — so plugin disposal can never hang the host's shutdown. Detached + work is fenced by stop/finalize/lease guards and cannot touch the closed + store. There is **no new host requirement**: older hosts simply never call + `dispose` and the plugin behaves exactly as before (no `engines` constraint + was added). +- **Cold-pass quarantine for malformed legacy sessions.** A session whose + message payload is structurally malformed (non-array body, unwalkable parts) + is sidelined for that `timeUpdated` and logged instead of aborting the whole + pass; it is retried automatically when the session changes and is dropped + from quarantine on deletion. The quarantine catches only data-shape errors — + a distiller regression still surfaces as a pass failure. `status()` exposes + `quarantinedCount`. +- **Transport-vs-absence distinction in the incremental path.** A failed + session-metadata fetch is now a logged, retryable transport error rather + than being conflated with "session gone", so a flaky server no longer makes + the distiller silently skip re-distills. + +### Fixed + +- **Stale-writer window closed.** Distiller writes (full/append/cold-pass + replaces, rollups, deletes) now re-verify authoritative lease ownership + against the live lease row immediately before each write transaction, so a + process suspended past the lease TTL cannot clobber the new holder's rows + when its paused fetch resumes. The self-check demotes only when another + holder's name is on the row — an expired-looking own heartbeat is not a + loss, so a long event-loop hiccup no longer costs a lease-retry stall. +- **Summarizer worker handoff safety.** Worker sessions are title-stamped with + their owner instance; normal cleanup only ever targets workers this instance + created, and orphan sweeps (other holders' leftovers) stay lease-gated. + Deleting/aborting a worker this instance owns is deliberately NOT + lease-gated — losing the writer lease mid-batch no longer leaks the worker + session. Worker ids are released from the owned set once cleanup completes. +- **No tool-contract change:** `fetchMessagePage` stays lenient on the query + path — a successful response without an array body is still an empty page + (`ok: true`) for `recall_messages`/`recall_context`/`recall_get`/drill. + Only the distiller opts into strict mode (where a non-array body must be + distinguishable from an empty session for quarantine). + ## 2.1.0 ### Added diff --git a/package-lock.json b/package-lock.json index 877dc98..0ba04b3 100644 --- a/package-lock.json +++ b/package-lock.json @@ -16,7 +16,7 @@ }, "devDependencies": { "@eslint/js": "^10.0.1", - "@opencode-ai/plugin": "^1.4.3", + "@opencode-ai/plugin": "^1.18.23", "@types/node": "^25.6.0", "eslint": "^10.2.0", "husky": "^9.1.7", @@ -26,13 +26,23 @@ "typescript-eslint": "^8.58.1", "vitest": "^4.1.5" }, - "engines": { - "opencode": ">=1.15.11" - }, "peerDependencies": { "@opencode-ai/plugin": ">=1.2.0" } }, + "node_modules/@ai-sdk/provider": { + "version": "3.0.8", + "resolved": "https://registry.npmjs.org/@ai-sdk/provider/-/provider-3.0.8.tgz", + "integrity": "sha512-oGMAgGoQdBXbZqNG0Ze56CHjDZ1IDYOwGYxYjO5KLSlz5HiNQ9udIXsPZ61VWaHGZ5XW/jyjmr6t2xz2jGVwbQ==", + "dev": true, + "license": "Apache-2.0", + "dependencies": { + "json-schema": "^0.4.0" + }, + "engines": { + "node": ">=18" + } + }, "node_modules/@emnapi/core": { "version": "1.10.0", "resolved": "https://registry.npmjs.org/@emnapi/core/-/core-1.10.0.tgz", @@ -692,6 +702,90 @@ "@jridgewell/sourcemap-codec": "^1.4.14" } }, + "node_modules/@msgpackr-extract/msgpackr-extract-darwin-arm64": { + "version": "3.0.4", + "resolved": "https://registry.npmjs.org/@msgpackr-extract/msgpackr-extract-darwin-arm64/-/msgpackr-extract-darwin-arm64-3.0.4.tgz", + "integrity": "sha512-LCkGo6JDfaBhgST7UpPWgNgLINpcpabaHfyz5OBx75nUYxBsaEPxjnyNjWpeb/xBup/682QnBfRBy2/LvPutZQ==", + "cpu": [ + "arm64" + ], + "dev": true, + "license": "MIT", + "optional": true, + "os": [ + "darwin" + ] + }, + "node_modules/@msgpackr-extract/msgpackr-extract-darwin-x64": { + "version": "3.0.4", + "resolved": "https://registry.npmjs.org/@msgpackr-extract/msgpackr-extract-darwin-x64/-/msgpackr-extract-darwin-x64-3.0.4.tgz", + "integrity": "sha512-zExlW9zUJKZH/tOtVMttwjKa4Xm/3KcNjnE3dPN92uCktwavMxpgCA3MoJK/DOnTWsQgo224OaST27/mPNAf+w==", + "cpu": [ + "x64" + ], + "dev": true, + "license": "MIT", + "optional": true, + "os": [ + "darwin" + ] + }, + "node_modules/@msgpackr-extract/msgpackr-extract-linux-arm": { + "version": "3.0.4", + "resolved": "https://registry.npmjs.org/@msgpackr-extract/msgpackr-extract-linux-arm/-/msgpackr-extract-linux-arm-3.0.4.tgz", + "integrity": "sha512-Tg3yX65f5GbtXLkrYEHE5oibZG9epyYWas7FogTTEJeDEF9JlXJzKgXaNhT3UXlTOeA+AfZpYZYZ0uPj7Cfquw==", + "cpu": [ + "arm" + ], + "dev": true, + "license": "MIT", + "optional": true, + "os": [ + "linux" + ] + }, + "node_modules/@msgpackr-extract/msgpackr-extract-linux-arm64": { + "version": "3.0.4", + "resolved": "https://registry.npmjs.org/@msgpackr-extract/msgpackr-extract-linux-arm64/-/msgpackr-extract-linux-arm64-3.0.4.tgz", + "integrity": "sha512-dgX0P/9wGPJeHFBG+ZmhgE6bmtMt7NP5CRBGyyktpopdk/mW4POnrpQsSLtKI1dwpc+pPLuXHDh6vvskyQE/sw==", + "cpu": [ + "arm64" + ], + "dev": true, + "license": "MIT", + "optional": true, + "os": [ + "linux" + ] + }, + "node_modules/@msgpackr-extract/msgpackr-extract-linux-x64": { + "version": "3.0.4", + "resolved": "https://registry.npmjs.org/@msgpackr-extract/msgpackr-extract-linux-x64/-/msgpackr-extract-linux-x64-3.0.4.tgz", + "integrity": "sha512-8TNXMEjJc3QEy7R/x1INhgiU+XakDAFUzBhaz7+Rbrs8NH5UQeHQxxmzsSBJGyV6I1jW79undiQm8tOI+D+8FQ==", + "cpu": [ + "x64" + ], + "dev": true, + "license": "MIT", + "optional": true, + "os": [ + "linux" + ] + }, + "node_modules/@msgpackr-extract/msgpackr-extract-win32-x64": { + "version": "3.0.4", + "resolved": "https://registry.npmjs.org/@msgpackr-extract/msgpackr-extract-win32-x64/-/msgpackr-extract-win32-x64-3.0.4.tgz", + "integrity": "sha512-CmCXPQrkbwExx3j946/PtHWHbYJiCRBRDl4BlkRQcJB/YOwQxJRTpoo7aTsortjgoJ1x7opzTSxn7C+ASSLVjQ==", + "cpu": [ + "x64" + ], + "dev": true, + "license": "MIT", + "optional": true, + "os": [ + "win32" + ] + }, "node_modules/@napi-rs/wasm-runtime": { "version": "1.1.4", "resolved": "https://registry.npmjs.org/@napi-rs/wasm-runtime/-/wasm-runtime-1.1.4.tgz", @@ -712,21 +806,29 @@ } }, "node_modules/@opencode-ai/plugin": { - "version": "1.4.3", + "version": "1.18.23", + "resolved": "https://registry.npmjs.org/@opencode-ai/plugin/-/plugin-1.18.23.tgz", + "integrity": "sha512-a+LbJe+wyyiwOQ7DeYOY4MkI/6VL45wkgYunVNZGC93UQgaj6sMGemS45CsLXrjyF4bvJMtqGd66GJvgjzbXAg==", "dev": true, "license": "MIT", "dependencies": { - "@opencode-ai/sdk": "1.4.3", + "@ai-sdk/provider": "3.0.8", + "@opencode-ai/sdk": "1.18.23", + "effect": "4.0.0-beta.83", "zod": "4.1.8" }, "peerDependencies": { - "@opentui/core": ">=0.1.97", - "@opentui/solid": ">=0.1.97" + "@opentui/core": ">=0.4.5", + "@opentui/keymap": ">=0.4.5", + "@opentui/solid": ">=0.4.5" }, "peerDependenciesMeta": { "@opentui/core": { "optional": true }, + "@opentui/keymap": { + "optional": true + }, "@opentui/solid": { "optional": true } @@ -741,7 +843,9 @@ } }, "node_modules/@opencode-ai/sdk": { - "version": "1.4.3", + "version": "1.18.23", + "resolved": "https://registry.npmjs.org/@opencode-ai/sdk/-/sdk-1.18.23.tgz", + "integrity": "sha512-VouYbL8O2ynLq0atr5fjzCv8YLtFi/zKLQ/uYkCvTJ4CmUdA01XwPn8om+u3TvxnGEfQsBfmThGU141vt3sg5w==", "license": "MIT", "dependencies": { "cross-spawn": "7.0.6" @@ -1993,6 +2097,25 @@ "node": ">=8" } }, + "node_modules/effect": { + "version": "4.0.0-beta.83", + "resolved": "https://registry.npmjs.org/effect/-/effect-4.0.0-beta.83.tgz", + "integrity": "sha512-0wsak8RtgGAr9UWSbVDgJHZcUqMSvicHcvaZv1MbMM7MCGgW4Rn/137J1MHQbwYPcwYGxT/IqehFd+UbYuj78w==", + "dev": true, + "license": "MIT", + "dependencies": { + "@standard-schema/spec": "^1.1.0", + "fast-check": "^4.8.0", + "find-my-way-ts": "^0.1.6", + "ini": "^7.0.0", + "kubernetes-types": "^1.30.0", + "msgpackr": "^2.0.1", + "multipasta": "^0.2.7", + "toml": "^4.1.1", + "uuid": "^14.0.0", + "yaml": "^2.9.0" + } + }, "node_modules/es-module-lexer": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/es-module-lexer/-/es-module-lexer-2.0.0.tgz", @@ -2207,6 +2330,29 @@ "node": ">=12.0.0" } }, + "node_modules/fast-check": { + "version": "4.9.0", + "resolved": "https://registry.npmjs.org/fast-check/-/fast-check-4.9.0.tgz", + "integrity": "sha512-7ms6T7SybUev/PQITciI0yLM2pOSFy5zpG8Ty7tQofcVaQUvrMXp6CBwqF6fThLCLOrfBtuHAtwq6Yu4XPCllg==", + "dev": true, + "funding": [ + { + "type": "individual", + "url": "https://github.com/sponsors/dubzzz" + }, + { + "type": "opencollective", + "url": "https://opencollective.com/fast-check" + } + ], + "license": "MIT", + "dependencies": { + "pure-rand": "^8.0.0" + }, + "engines": { + "node": ">=12.17.0" + } + }, "node_modules/fast-deep-equal": { "version": "3.1.3", "dev": true, @@ -2258,6 +2404,13 @@ "node": ">=16.0.0" } }, + "node_modules/find-my-way-ts": { + "version": "0.1.6", + "resolved": "https://registry.npmjs.org/find-my-way-ts/-/find-my-way-ts-0.1.6.tgz", + "integrity": "sha512-a85L9ZoXtNAey3Y6Z+eBWW658kO/MwR7zIafkIUPUMf3isZG0NCs2pjW2wtjxAKuJPxMAsHUIP4ZPGv0o5gyTA==", + "dev": true, + "license": "MIT" + }, "node_modules/find-up": { "version": "5.0.0", "dev": true, @@ -2355,6 +2508,16 @@ "node": ">=0.8.19" } }, + "node_modules/ini": { + "version": "7.0.0", + "resolved": "https://registry.npmjs.org/ini/-/ini-7.0.0.tgz", + "integrity": "sha512-ifK0CgjALofS5bkrcTy4RaQ9Vx2Knf/eLeIO+NaswQEpH1UblrtTSCIvN71qQDMq0PeQ/SSPojvEJp9vvvfr+w==", + "dev": true, + "license": "ISC", + "engines": { + "node": "^22.22.2 || ^24.15.0 || >=26.0.0" + } + }, "node_modules/is-extglob": { "version": "2.1.1", "dev": true, @@ -2391,6 +2554,13 @@ "dev": true, "license": "MIT" }, + "node_modules/json-schema": { + "version": "0.4.0", + "resolved": "https://registry.npmjs.org/json-schema/-/json-schema-0.4.0.tgz", + "integrity": "sha512-es94M3nTIfsEPisRafak+HDLfHXnKBhV3vU5eqPcS3flIWqcxJWgXHXiey3YrpaNsanY5ei1VoYEbOzijuq9BA==", + "dev": true, + "license": "(AFL-2.1 OR BSD-3-Clause)" + }, "node_modules/json-schema-traverse": { "version": "0.4.1", "dev": true, @@ -2409,6 +2579,13 @@ "json-buffer": "3.0.1" } }, + "node_modules/kubernetes-types": { + "version": "1.30.0", + "resolved": "https://registry.npmjs.org/kubernetes-types/-/kubernetes-types-1.30.0.tgz", + "integrity": "sha512-Dew1okvhM/SQcIa2rcgujNndZwU8VnSapDgdxlYoB84ZlpAD43U6KLAFqYo17ykSFGHNPrg0qry0bP+GJd9v7Q==", + "dev": true, + "license": "Apache-2.0" + }, "node_modules/levn": { "version": "0.4.1", "dev": true, @@ -2776,6 +2953,46 @@ "dev": true, "license": "MIT" }, + "node_modules/msgpackr": { + "version": "2.0.6", + "resolved": "https://registry.npmjs.org/msgpackr/-/msgpackr-2.0.6.tgz", + "integrity": "sha512-plGul/tqjt9vqWFR9zyqyLZls6gb5KTLvnsRO2B9+TZ8tNiXiVI/CR8LiDWlAX4IUeVkQ9gsmzKA6voC2QOciw==", + "dev": true, + "license": "MIT", + "optionalDependencies": { + "msgpackr-extract": "^3.0.4" + } + }, + "node_modules/msgpackr-extract": { + "version": "3.0.4", + "resolved": "https://registry.npmjs.org/msgpackr-extract/-/msgpackr-extract-3.0.4.tgz", + "integrity": "sha512-4kmO/MdyUIkLIvTPr8VHLil4AtoKIoniWPIEk5+CDy0xnWC84azhSFmuJ7PxZdsYtiP5kEeQsORAVIeMgxT+Hw==", + "dev": true, + "hasInstallScript": true, + "license": "MIT", + "optional": true, + "dependencies": { + "node-gyp-build-optional-packages": "5.2.2" + }, + "bin": { + "download-msgpackr-prebuilds": "bin/download-prebuilds.js" + }, + "optionalDependencies": { + "@msgpackr-extract/msgpackr-extract-darwin-arm64": "3.0.4", + "@msgpackr-extract/msgpackr-extract-darwin-x64": "3.0.4", + "@msgpackr-extract/msgpackr-extract-linux-arm": "3.0.4", + "@msgpackr-extract/msgpackr-extract-linux-arm64": "3.0.4", + "@msgpackr-extract/msgpackr-extract-linux-x64": "3.0.4", + "@msgpackr-extract/msgpackr-extract-win32-x64": "3.0.4" + } + }, + "node_modules/multipasta": { + "version": "0.2.8", + "resolved": "https://registry.npmjs.org/multipasta/-/multipasta-0.2.8.tgz", + "integrity": "sha512-ZPWuMKyv0cSO29f7hozp+k6+crZbQijV8ipMvxNxRf2SwtYGTX1ZX89Kd20VV4H9Znonx+EQn+iy1wGQsJ+b+Q==", + "dev": true, + "license": "MIT" + }, "node_modules/mz": { "version": "2.7.0", "dev": true, @@ -2810,6 +3027,22 @@ "dev": true, "license": "MIT" }, + "node_modules/node-gyp-build-optional-packages": { + "version": "5.2.2", + "resolved": "https://registry.npmjs.org/node-gyp-build-optional-packages/-/node-gyp-build-optional-packages-5.2.2.tgz", + "integrity": "sha512-s+w+rBWnpTMwSFbaE0UXsRlg7hU4FjekKU4eyAih5T8nJuNZT1nNsskXpxmeqSK9UzkBl6UgRlnKc8hz8IEqOw==", + "dev": true, + "license": "MIT", + "optional": true, + "dependencies": { + "detect-libc": "^2.0.1" + }, + "bin": { + "node-gyp-build-optional-packages": "bin.js", + "node-gyp-build-optional-packages-optional": "optional.js", + "node-gyp-build-optional-packages-test": "build-test.js" + } + }, "node_modules/object-assign": { "version": "4.1.1", "dev": true, @@ -3029,6 +3262,23 @@ "node": ">=6" } }, + "node_modules/pure-rand": { + "version": "8.4.2", + "resolved": "https://registry.npmjs.org/pure-rand/-/pure-rand-8.4.2.tgz", + "integrity": "sha512-vvuOGgcuPJAirlHvuQw1TrOiw7ptaIXXmIbNuiNOY6lNGJJH49PQ1Kj4nd783nPdQhQdicgOjVI2yI/9BD6/Ng==", + "dev": true, + "funding": [ + { + "type": "individual", + "url": "https://github.com/sponsors/dubzzz" + }, + { + "type": "opencollective", + "url": "https://opencollective.com/fast-check" + } + ], + "license": "MIT" + }, "node_modules/readdirp": { "version": "4.1.2", "dev": true, @@ -3270,6 +3520,16 @@ "node": ">=14.0.0" } }, + "node_modules/toml": { + "version": "4.3.0", + "resolved": "https://registry.npmjs.org/toml/-/toml-4.3.0.tgz", + "integrity": "sha512-lVb8X9BsPVuH0M4BKeS91tXAmJvCjQ5UIyAbQFaxkKGyUFK2RPkhwaFSQH8vbpl1d23eu/IBH+dwVMHWaq9A5A==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=20" + } + }, "node_modules/tree-kill": { "version": "1.2.2", "dev": true, @@ -3418,6 +3678,20 @@ "punycode": "^2.1.0" } }, + "node_modules/uuid": { + "version": "14.0.2", + "resolved": "https://registry.npmjs.org/uuid/-/uuid-14.0.2.tgz", + "integrity": "sha512-xZe/16rV4aa+HGSOCiY2YeLT1OybRLrrkL/Rqaq7p7GMVXjFh+6wN4oMYgjFmnSnhY8t6Xpdl2l9qmnHYuMHwQ==", + "dev": true, + "funding": [ + "https://github.com/sponsors/broofa", + "https://github.com/sponsors/ctavan" + ], + "license": "MIT", + "bin": { + "uuid": "dist-node/bin/uuid" + } + }, "node_modules/vite": { "version": "8.0.10", "resolved": "https://registry.npmjs.org/vite/-/vite-8.0.10.tgz", @@ -3634,6 +3908,22 @@ "node": ">=0.10.0" } }, + "node_modules/yaml": { + "version": "2.9.0", + "resolved": "https://registry.npmjs.org/yaml/-/yaml-2.9.0.tgz", + "integrity": "sha512-2AvhNX3mb8zd6Zy7INTtSpl1F15HW6Wnqj0srWlkKLcpYl/gMIMJiyuGq2KeI2YFxUPjdlB+3Lc10seMLtL4cA==", + "dev": true, + "license": "ISC", + "bin": { + "yaml": "bin.mjs" + }, + "engines": { + "node": ">= 14.6" + }, + "funding": { + "url": "https://github.com/sponsors/eemeli" + } + }, "node_modules/yocto-queue": { "version": "0.1.0", "dev": true, diff --git a/package.json b/package.json index 99ecaf6..a13979e 100644 --- a/package.json +++ b/package.json @@ -62,9 +62,6 @@ "url": "https://github.com/rmk40/opencode-session-recall/issues" }, "license": "MIT", - "engines": { - "opencode": ">=1.15.11" - }, "peerDependencies": { "@opencode-ai/plugin": ">=1.2.0" }, @@ -76,7 +73,7 @@ }, "devDependencies": { "@eslint/js": "^10.0.1", - "@opencode-ai/plugin": "^1.4.3", + "@opencode-ai/plugin": "^1.18.23", "@types/node": "^25.6.0", "eslint": "^10.2.0", "husky": "^9.1.7", diff --git a/src/distill.ts b/src/distill.ts index d54718c..dcd6423 100644 --- a/src/distill.ts +++ b/src/distill.ts @@ -101,6 +101,9 @@ export type DistillStatus = { coldPass: "idle" | "running" | "done"; distilledCount: number; knownCount: number; + /** Sessions currently sidelined as malformed (see the quarantine mechanism); + * observability for "the pass is done but N sessions were skipped". */ + quarantinedCount: number; lastError?: string; /** Current distill-lease holder info (this process or whichever build holds it), * from {@link Store.leaseStatus}. Undefined with no store or no lease row yet — @@ -566,10 +569,16 @@ function readNextCursor(response: unknown): string | null { * without `limit` is a 400. The next-page cursor rides the `X-Next-Cursor` * header; the body is the `{ info, parts }` array. Throws on an SDK error so the * caller's try/catch handles fetch failures uniformly. + * + * A successful response whose body is not an array is ambiguous: the query path + * (browse/context/drill) treats it as a deliberate "empty page, ok:true" — a + * documented decision for sessions with no data — while the distiller passes + * `strict: true` so its quarantine can distinguish a malformed session from an + * empty one. */ export async function fetchMessagePage( client: OpencodeClient, - opts: { sessionID: string; limit: number; before?: string }, + opts: { sessionID: string; limit: number; before?: string; strict?: boolean }, ): Promise { const params = opts.before != null @@ -578,7 +587,10 @@ export async function fetchMessagePage( const resp = await client.session.messages(params); if (resp.error) throw new Error(errmsg(resp.error)); if (!Array.isArray(resp.data)) { - throw new MalformedSessionError("successful message response was not an array"); + if (opts.strict) { + throw new MalformedSessionError("successful message response was not an array"); + } + return { items: [], nextCursor: readNextCursor(resp.response) }; } const items = resp.data as MsgWithParts[]; return { items, nextCursor: readNextCursor(resp.response) }; @@ -614,6 +626,7 @@ const noopStatus = (): DistillStatus => ({ coldPass: "idle", distilledCount: 0, knownCount: 0, + quarantinedCount: 0, }); export function createDistiller(options: DistillerOptions): Distiller { @@ -739,7 +752,7 @@ export function createDistiller(options: DistillerOptions): Distiller { const before = cursor; const page = await gate.runBackground(() => { if (stopped || !leaseHeld) return Promise.resolve({ items: [], nextCursor: null }); - return fetchMessagePage(client, { sessionID, limit: pageMessages, before }); + return fetchMessagePage(client, { sessionID, limit: pageMessages, before, strict: true }); }); try { for (const msg of page.items) { @@ -777,14 +790,18 @@ export function createDistiller(options: DistillerOptions): Distiller { const before = cursor; const page = await gate.runBackground(() => { if (stopped || !leaseHeld) return Promise.resolve({ items: [], nextCursor: null }); - return fetchMessagePage(client, { sessionID, limit: pageMessages, before }); + return fetchMessagePage(client, { sessionID, limit: pageMessages, before, strict: true }); }); - for (const msg of page.items) { - if (msg.info.id === checkpoint) { - reached = true; - break; + try { + for (const msg of page.items) { + if (msg.info.id === checkpoint) { + reached = true; + break; + } + collected.push(msg); } - collected.push(msg); + } catch (error) { + throw new MalformedSessionError(errmsg(error)); } cursor = reached ? undefined : (page.nextCursor ?? undefined); } while (cursor); @@ -820,6 +837,10 @@ export function createDistiller(options: DistillerOptions): Distiller { } function recomputeRootRollup(rootId: string): void { + // Rollup writes replace the root card row; verify authoritative ownership + // once per rollup batch (callers reach here after awaited fetches, so the + // cached flag alone can be stale after a >TTL suspension). + if (!ownsLease()) return; const root = store.getCard(rootId); if (!root) return; const children = store @@ -830,6 +851,10 @@ export function createDistiller(options: DistillerOptions): Distiller { } function recomputeAllRootRollups(): void { + // One authoritative ownership read for the whole rollup sweep (see + // recomputeRootRollup); per-row checks would multiply SQLite reads for + // no additional safety. + if (!ownsLease()) return; const rootCards = new Map(); const childrenByRoot = new Map(); // Load embeddings too: the rollup upsert below rewrites every card column, so @@ -857,13 +882,21 @@ export function createDistiller(options: DistillerOptions): Distiller { async function distillFull(session: DistillSessionMeta): Promise { const messages = await fetchSessionMessages(session.id, caps.ftsRowsPerSession); + // Cheap flag check before parentChainOf touches the store: a detached + // continuation (the fetch settled after dispose's bounded wait) must not + // read SQLite, which may already be closed. + if (stopped || !leaseHeld) return; const { card, rows } = deriveCard({ session, messages, parentById: parentChainOf(session), caps, }); - if (stopped || !leaseHeld) return; + // Authoritative re-check (live lease row, not the cached flag): a process + // suspended past the TTL can lose the lease to a rival before its heartbeat + // callback ever runs; the resumed fetch must not replace the new holder's + // rows. One leaseStatus() read per write batch. + if (stopped || !ownsLease()) return; store.replaceSessionParts(session.id, rows, card); bumpCardsRev(); } @@ -875,7 +908,9 @@ export function createDistiller(options: DistillerOptions): Distiller { return; } const { messages: newMessages, reached } = await fetchNewMessages(session.id, checkpoint); - if (stopped || !leaseHeld) return; + // Authoritative ownership check before the append write (see distillFull); + // only synchronous derivation sits between here and appendSessionParts. + if (stopped || !ownsLease()) return; // If the checkpoint message was never found, the newest pages are NOT a clean // append tail (they'd re-insert already-stored parts and hit a UNIQUE // violation, wedging the session). Fall back to a full re-distill. @@ -1012,15 +1047,25 @@ export function createDistiller(options: DistillerOptions): Distiller { try { ({ card, rows } = deriveCard({ session, messages, parentById, caps })); } catch (error) { + // Quarantine only data-shape failures (mirroring the fetch path + // above): a TypeError walking malformed legacy parts is this + // session's problem; anything else is a deriveCard regression that + // must surface as a pass failure, not silently sideline sessions. + if (!(error instanceof MalformedSessionError) && !(error instanceof TypeError)) { + throw error; + } quarantine(session, error); examined++; recordProgress(session.timeUpdated); return; } // The lease can drop (heartbeat takeover) during the fetch above; a - // non-holder must never write. Re-check right before the write and - // skip it, counting the session as not distilled. - if (stopped || !leaseHeld) return; + // non-holder must never write. Re-check right before the write — + // authoritatively, against the live lease row, because a suspension + // past the TTL loses the lease before the heartbeat callback flips + // the cached flag — and skip it, counting the session as not + // distilled. + if (stopped || !ownsLease()) return; store.replaceSessionParts(session.id, rows, card); bumpCardsRev(); distilled++; @@ -1134,6 +1179,10 @@ export function createDistiller(options: DistillerOptions): Distiller { let succeeded = false; try { const session = await fetchSessionMeta(sessionID); + // A request already in flight at stop time settles late, past dispose's + // bounded wait — by then SQLite may be closed, so a detached continuation + // must not reach the store reads below. + if (stopped || !leaseHeld) return; // The summarizer's worker session emits idle events as it is prompted; it // is never distilled or carded (its prompts embed card digests). if (session && isSummarizerTitle(session.title)) return; @@ -1163,7 +1212,7 @@ export function createDistiller(options: DistillerOptions): Distiller { } function handleDeleted(sessionID: string): void { - if (!leaseHeld) return; + if (!ownsLease()) return; // authoritative: this path deletes rows immediately const rootId = store.getCard(sessionID)?.rootId; store.deleteSession(sessionID); bumpCardsRev(); @@ -1229,8 +1278,14 @@ export function createDistiller(options: DistillerOptions): Distiller { function ownsLease(): boolean { if (!leaseHeld || finalized) return false; + // Authoritative check against the live lease row. Ownership is lost only + // when someone else's name is on the row (a takeover happened) or the row + // is gone — NOT when our own heartbeat merely looks stale: an expired + // heartbeat means a rival COULD take over, and heartbeat freshness is the + // ACQUIRE-side concern of that rival. Self-demoting on staleness would cost + // a full lease-retry stall after any >TTL event-loop hiccup. const current = store.leaseStatus(); - const owned = current?.holder === instanceId && current.heartbeat + current.ttl > now(); + const owned = current?.holder === instanceId; if (!owned) { leaseHeld = false; clearTimer(heartbeatTimer); @@ -1318,11 +1373,14 @@ export function createDistiller(options: DistillerOptions): Distiller { status(): DistillStatus { const lease = store.leaseStatus(); + // Report the cached flag: status() must be side-effect-free (ownsLease() + // mutates lease state and re-arms timers; it is for write gates only). return { - leaseHeld: ownsLease(), + leaseHeld, coldPass: coldPassState, distilledCount, knownCount, + quarantinedCount: quarantined.size, ...(lastError !== undefined ? { lastError } : {}), ...(lease ? { lease } : {}), }; diff --git a/src/opencode-session-recall.ts b/src/opencode-session-recall.ts index 8f796cf..1676ecc 100644 --- a/src/opencode-session-recall.ts +++ b/src/opencode-session-recall.ts @@ -24,16 +24,24 @@ import { createCardsRuntime, cardsLiteFromSessions, type CardSource } from "./ca import { createDrill } from "./drill.js"; import { createDistiller } from "./distill.js"; import { createSummarizer, parseModelId, type Summarizer } from "./summarize.js"; -import { TOOLS, DEFAULTS, optionalString, errmsg, type Limits } from "./types.js"; +import { + TOOLS, + DEFAULTS, + optionalString, + errmsg, + settleWithin, + type ErrorOutput, + type Limits, +} from "./types.js"; -// `dispose` was added to the host/plugin contract in @opencode-ai/plugin 1.15.11. -// Keep the source compatible with this repository's older tool-result typings -// while declaring the exact minimum-host hook that the package engine requires. -declare module "@opencode-ai/plugin" { - interface Hooks { - dispose?: () => Promise; - } -} +/** Per-phase bound on dispose()'s awaited waits. The host awaits dispose as a + * shutdown finalizer with no timeout of its own, so a never-settling in-flight + * SDK request must not hang shutdown forever. Two bounded phases (distiller + * stop, tracked operations) after the summarizer's own 15s bound keep total + * dispose under ~20s worst case. Timed-out work is detached, not cancelled; + * every store write path re-checks stopped/finalized/lease ownership before + * writing, so closing SQLite with a detached fetch pending is safe. */ +const SHUTDOWN_TIMEOUT_MS = 2_500; /** Guarded, Node-free logger: `console` is a std global, but `src/` declares no * types, so reach it defensively. */ @@ -337,7 +345,13 @@ const server: Plugin = async (ctx, options) => { ...definition, execute: (args, context) => { if (disposed) { - return Promise.reject(new Error("opencode-session-recall: plugin has been disposed")); + // Match every other failure in this codebase: a JSON error output, not + // a rejection. + const err: ErrorOutput = { + ok: false, + error: "opencode-session-recall: plugin has been disposed", + }; + return Promise.resolve(JSON.stringify(err)); } return track(definition.execute(args, context)); }, @@ -400,12 +414,18 @@ const server: Plugin = async (ctx, options) => { distiller.quiesce(); cards.dispose(); disposePromise = (async () => { - // Summarizer.stop() is bounded. Its late SDK promises are detached and - // ownership/lease guarded, so phase 2 may safely release the lease once - // this settles even when an SDK request itself never does. + // Summarizer.stop() is internally bounded (its own 15s shutdown + // timeout). Its late SDK promises are detached and ownership/lease + // guarded, so phase 2 may safely release the lease once this settles + // even when an SDK request itself never does. if (summarizer) await Promise.allSettled([summarizer.stop()]); - await distiller.stop(); - await Promise.allSettled([...operations]); + // Bound the remaining waits: an SDK request that never settles would + // otherwise hang the host's shutdown (dispose is awaited untimed). + // Timing out does NOT skip db.close() — every distiller write path is + // finalized/lease-guarded, so a detached fetch that settles later can + // no longer reach SQLite. + await settleWithin(distiller.stop(), SHUTDOWN_TIMEOUT_MS); + await settleWithin(Promise.allSettled([...operations]), SHUTDOWN_TIMEOUT_MS); db?.close(); db = null; })(); diff --git a/src/summarize.ts b/src/summarize.ts index bcf97e0..8b1aeb4 100644 --- a/src/summarize.ts +++ b/src/summarize.ts @@ -2,7 +2,7 @@ import type { OpencodeClient, Part, PermissionRuleset, Session } from "@opencode import type { Card, Store } from "./store.js"; import { SUMMARY_REV_KEY } from "./store.js"; import type { FetchGate } from "./fetch-gate.js"; -import { errmsg } from "./types.js"; +import { errmsg, settleWithin } from "./types.js"; import { tokenizeAll } from "./normalize.js"; import { SUMMARIZER_SENTINEL, isSummarizerTitle } from "./extract.js"; @@ -126,23 +126,6 @@ export type Summarizer = { type Timer = ReturnType; -type TimedResult = { timedOut: false; value: T } | { timedOut: true }; - -async function settleWithin(promise: Promise, timeoutMs: number): Promise> { - let timer: Timer | undefined; - const timeout = new Promise>((resolve) => { - timer = setTimeout(() => resolve({ timedOut: true }), timeoutMs); - }); - try { - return await Promise.race([ - promise.then((value): TimedResult => ({ timedOut: false, value })), - timeout, - ]); - } finally { - if (timer) clearTimeout(timer); - } -} - export function summarizerWorkerTitle(ownerToken: string): string { return `${SUMMARIZER_SENTINEL} owner=${ownerToken}`; } @@ -303,16 +286,12 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { // A fresh worker per batch (create, prompt once, delete): create/delete are // unbilled and this keeps every batch's context clean with zero accumulation. - async function leaseSdk( - label: string, - allowWhileStopped: boolean, - operation: () => Promise, - ): Promise { - if ((!allowWhileStopped && stopped) || !leaseHeld()) return undefined; + async function leaseSdk(label: string, operation: () => Promise): Promise { + if (stopped || !leaseHeld()) return undefined; const result = await settleWithin( gate.runBackground(() => { // A gate permit may arrive after shutdown or an involuntary lease loss. - if ((!allowWhileStopped && stopped) || !leaseHeld()) return Promise.resolve(undefined); + if (stopped || !leaseHeld()) return Promise.resolve(undefined); return operation(); }), shutdownTimeoutMs, @@ -324,13 +303,32 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { return result.value; } + /** Bounded SDK runner for destroying a worker THIS instance created. Remote + * worker ownership is a separate authority from the SQLite writer lease: + * losing the lease (or stopping) must never leak a worker we uniquely own, + * so this deliberately checks neither `stopped` nor `leaseHeld()` — only the + * caller's `ownedWorkers` membership scopes it. Returns whether the request + * settled in time (a timed-out delete leaves the id owned for a later retry + * by the next holder's orphan sweep). */ + async function ownedWorkerSdk( + label: string, + operation: () => Promise, + ): Promise { + const result = await settleWithin(gate.runBackground(operation), shutdownTimeoutMs); + if (result.timedOut) { + logMsg(`${label} timed out after ${shutdownTimeoutMs}ms; late SDK settlement detached`); + return false; + } + return true; + } + async function createWorker(): Promise { try { // Probe the deny-all permission ruleset once; if the server rejects the // shape, remember that and create plainly thereafter (never let a rejected // ruleset silently disable summaries). if (permissionMode !== "without") { - const resp = await leaseSdk("worker create", false, () => + const resp = await leaseSdk("worker create", () => client.session.create({ title: workerTitle, permission: DENY_ALL_PERMISSION }), ); if (!resp) return null; @@ -345,7 +343,7 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { logMsg("worker permission ruleset rejected; relying on tool-disable + exclusion"); } } - const resp = await leaseSdk("worker create", false, () => + const resp = await leaseSdk("worker create", () => client.session.create({ title: workerTitle }), ); if (!resp) return null; @@ -361,16 +359,26 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { async function deleteOwnedWorker(sessionID: string): Promise { if (!ownedWorkers.has(sessionID)) return; try { - await leaseSdk("worker delete", true, () => client.session.delete({ sessionID })); + // NOT lease-gated: this instance created the worker and uniquely owns it; + // losing the SQLite writer lease mid-batch must not leak the session. + const settled = await ownedWorkerSdk("worker delete", () => + client.session.delete({ sessionID }), + ); + if (settled) ownedWorkers.delete(sessionID); } catch { - // Best-effort; a lingering sentinel session is excluded everywhere. + // Best-effort; a lingering sentinel session is excluded everywhere and + // swept by the next holder's orphan cleanup. + ownedWorkers.delete(sessionID); } } async function abortOwnedWorker(sessionID: string): Promise { if (!ownedWorkers.has(sessionID)) return; try { - await leaseSdk("worker abort", true, () => client.session.abort({ sessionID })); + // NOT lease-gated, same as deleteOwnedWorker: the abort stops OUR + // worker's spend. It does not remove the id — the delete that follows + // owns the ownedWorkers cleanup. + await ownedWorkerSdk("worker abort", () => client.session.abort({ sessionID })); } catch { // Best-effort. } @@ -380,7 +388,7 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { * than adopting one whose accumulated context is unknown. Runs once. */ async function deleteOrphans(): Promise { try { - const resp = await leaseSdk("worker orphan list", false, () => + const resp = await leaseSdk("worker orphan list", () => client.session.list({ search: SUMMARIZER_SENTINEL, limit: 100 }), ); if (!resp || stopped || !leaseHeld()) return; @@ -390,7 +398,9 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { // every await and immediately before each destructive request. if (stopped || !leaseHeld()) return; if (isSummarizerTitle(row.title) && typeof row.id === "string" && row.id) { - await leaseSdk("orphan worker delete", false, () => + // Orphan sweeps target OTHER holders' leftovers, so they stay + // lease-gated (unlike ownedWorkers cleanup above). + await leaseSdk("orphan worker delete", () => client.session.delete({ sessionID: row.id }), ); if (stopped || !leaseHeld()) return; diff --git a/src/types.ts b/src/types.ts index cff384b..f9bb650 100644 --- a/src/types.ts +++ b/src/types.ts @@ -449,3 +449,32 @@ export function coerceInt(value: unknown, fallback: number, min: number, max: nu if (typeof value !== "number" || !Number.isFinite(value)) return fallback; return Math.min(max, Math.max(min, Math.trunc(value))); } + +/** Result of {@link settleWithin}: the settled value, or a timeout marker. */ +export type TimedResult = { timedOut: false; value: T } | { timedOut: true }; + +/** + * Await a promise for at most `timeoutMs`, then detach it. The promise itself + * is never cancelled (SDK requests cannot be); a late settlement is simply no + * longer awaited. Callers must ensure detached work is guarded (lease checks, + * finalized flags) so its late completion cannot write anywhere. Used to bound + * summarizer SDK calls and the plugin's dispose() so a never-settling request + * cannot hang the host's shutdown. + */ +export async function settleWithin( + promise: Promise, + timeoutMs: number, +): Promise> { + let timer: ReturnType | undefined; + const timeout = new Promise>((resolve) => { + timer = setTimeout(() => resolve({ timedOut: true }), timeoutMs); + }); + try { + return await Promise.race([ + promise.then((value): TimedResult => ({ timedOut: false, value })), + timeout, + ]); + } finally { + if (timer) clearTimeout(timer); + } +} diff --git a/test/distill.test.ts b/test/distill.test.ts index 1ef1f84..f51edcb 100644 --- a/test/distill.test.ts +++ b/test/distill.test.ts @@ -549,6 +549,24 @@ describe("fetchMessagePage", () => { const { client } = clientReturning(() => ({ error: apiFailure("boom") })); await expect(fetchMessagePage(client, { sessionID: "s", limit: 5 })).rejects.toThrow("boom"); }); + + it("treats a successful non-array body as an empty page by default (query path)", async () => { + // Documented decision: a `{}` response (no data field) means "empty page, + // ok:true" for recall_messages/recall_context/recall_get/drill. Only the + // distiller opts into strict (below) so its quarantine can distinguish + // malformed from empty. + const { client } = clientReturning(() => ({})); + const page = await fetchMessagePage(client, { sessionID: "s", limit: 5 }); + expect(page.items).toEqual([]); + expect(page.nextCursor).toBeNull(); + }); + + it("throws MalformedSessionError on a successful non-array body under strict", async () => { + const { client } = clientReturning(() => ({ data: { messages: "not-an-array" } })); + await expect( + fetchMessagePage(client, { sessionID: "s", limit: 5, strict: true }), + ).rejects.toThrow("successful message response was not an array"); + }); }); // ── Cold pass ──────────────────────────────────────────────────────────────── @@ -596,6 +614,102 @@ describe("cold pass", () => { expect(logs.some((line) => line.includes("session bad quarantined at timeUpdated 3000:"))).toBe( true, ); + expect(distiller.status().quarantinedCount).toBe(1); + await distiller.stop(); + db.close(); + }); + + it("does not quarantine on a non-shape deriveCard error; the pass aborts and surfaces it", async () => { + // A deriveCard REGRESSION (anything that is not a data-shape failure) must + // not sideline healthy sessions while reporting "done". The message below + // passes the fetch stage untouched (fetchSessionMessages only walks parts) + // and then throws a plain Error inside deriveCard's walk. + const boobyTrapped = { + info: { + id: "m-boom", + role: "user", + get time(): { created: number } { + throw new Error("synthetic deriveCard regression"); + }, + }, + parts: [textPart("p-boom", "boom", "m-boom", "content")], + } as unknown as MessageBundle; + const graph: Graph = { + sessions: [session("boom", "Boom", PROJECT_DIR, 3000)], + messagesBySession: { boom: [boobyTrapped] }, + }; + const { client } = makeDistillFake(graph); + const { db, store } = await freshStore(); + const { gate } = makeSpyGate(); + const distiller = createDistiller({ + client, + store, + gate, + limits: { ...TEST_LIMITS, distillConcurrency: 1 }, + instanceId: "regression-surface", + coldPassRetryMs: 60_000, + }); + + distiller.start(); + await waitFor(() => distiller.status().lastError != null); + + expect(distiller.status().lastError).toContain("synthetic deriveCard regression"); + expect(distiller.status().quarantinedCount).toBe(0); // NOT quarantined + expect(distiller.status().coldPass).toBe("idle"); // aborted, not "done" + await distiller.stop(); + db.close(); + }); + + it("skips the write when the lease is taken over between fetch and write (stale writer)", async () => { + // A process suspended past the TTL can lose the lease before its heartbeat + // callback ever runs. The resumed fetch must not replace the new holder's + // rows: the write gate re-verifies ownership against the live lease row. + let clock = 1_000_000; + let resolveMessages!: (value: unknown) => void; + const gatePromise = new Promise((resolve) => { + resolveMessages = resolve; + }); + const s1 = session("s1", "Alpha", PROJECT_DIR, 3000); + const client = { + session: { + list: async () => ({ data: [s1] }), + messages: async (params: { sessionID: string; limit?: number }) => { + if (params.limit == null) throw new Error("unbounded fetch"); + await gatePromise; + return messagesResponse( + [bundle(userMessage("m1", "s1", 100), [textPart("p1", "s1", "m1", "alpha")])], + null, + ); + }, + }, + } as unknown as OpencodeClient; + const { db, store } = await freshStore(() => clock); + const { gate } = makeSpyGate(); + const distiller = createDistiller({ + client, + store, + gate, + limits: { ...TEST_LIMITS, distillConcurrency: 1 }, + instanceId: "suspended", + now: () => clock, + leaseRetryMs: 600_000, + }); + + distiller.start(); + await new Promise((r) => setTimeout(r, 10)); // fetch now blocked in flight + + // Simulated suspension past the TTL: a rival takes the lease over. + clock += 31_000; + expect(store.acquireLease("rival", 30_000, "rivalbuild", 4)).toBe(true); + + // status() reports the CACHED flag (side-effect-free): the heartbeat + // callback has not run, so this instance still believes it holds the lease. + expect(distiller.status().leaseHeld).toBe(true); + + resolveMessages({}); + await waitFor(() => distiller.status().coldPass !== "running"); + + expect(store.getCard("s1")).toBeUndefined(); // the stale write was skipped await distiller.stop(); db.close(); }); @@ -1244,6 +1358,40 @@ describe("lease", () => { distiller.stop(); db.close(); }); + + it("ownsLease does not self-demote on its own stale heartbeat", async () => { + // If the row still names THIS instance, nobody has taken over — an expired + // heartbeat only means someone COULD. Heartbeat freshness is the + // ACQUIRE-side concern of rivals; self-demoting would cost a full + // lease-retry stall after any >TTL event-loop hiccup. + let clock = 1_000_000; + const { client } = makeDistillFake({ sessions: [], messagesBySession: {} }); + const { db, store } = await freshStore(() => clock); + const { gate } = makeSpyGate(); + const distiller = createDistiller({ + client, + store, + gate, + limits: { ...TEST_LIMITS, coldPass: false }, + instanceId: "hiccup", + now: () => clock, + }); + + distiller.start(); + expect(distiller.ownsLease()).toBe(true); + + clock += 31_000; // heartbeat now looks expired, but the row still names us + expect(distiller.ownsLease()).toBe(true); + expect(distiller.status().leaseHeld).toBe(true); + + // An actual takeover (someone else's name on the row) IS a loss. + expect(store.acquireLease("rival", 30_000, "rivalbuild", 4)).toBe(true); + expect(distiller.ownsLease()).toBe(false); + expect(distiller.status().leaseHeld).toBe(false); + + await distiller.stop(); + db.close(); + }); }); // ── Incremental ────────────────────────────────────────────────────────────── @@ -1589,6 +1737,7 @@ describe("null store", () => { coldPass: "idle", distilledCount: 0, knownCount: 0, + quarantinedCount: 0, }); }); }); diff --git a/test/eval/harness.ts b/test/eval/harness.ts index 8fe92a0..4c57362 100644 --- a/test/eval/harness.ts +++ b/test/eval/harness.ts @@ -21,6 +21,7 @@ import { strictNoLimit, setStrictNoLimitMessages, UNBOUNDED_MESSAGES_ERROR, + toolResultText, } from "../helpers.js"; import { openSqlite } from "../../src/sqlite.js"; import { openStore } from "../../src/store.js"; @@ -264,7 +265,7 @@ export async function runCase( ): Promise { const caseCtx = c.ctxSessionID ? evalContext(c.ctxSessionID) : (ctx ?? evalContext()); const raw = await searchTool.execute(c.args as Parameters[0], caseCtx); - const parsed = JSON.parse(raw) as SearchOutput | { ok: false; error: string }; + const parsed = JSON.parse(toolResultText(raw)) as SearchOutput | { ok: false; error: string }; const returnedSessionIDs: string[] = []; const topClasses: (string | undefined)[] = []; diff --git a/test/eval/relevance.test.ts b/test/eval/relevance.test.ts index f02eb94..37dbb13 100644 --- a/test/eval/relevance.test.ts +++ b/test/eval/relevance.test.ts @@ -3,6 +3,7 @@ import type { ToolDefinition } from "@opencode-ai/plugin"; import type { SearchOutput } from "../../src/types.js"; import { EVAL_CASES } from "./cases.js"; import { evalContext, makeDegradedEvalSearch, makeEvalSearch, runEval } from "./harness.js"; +import { toolResultText } from "../helpers.js"; import BASELINE from "./baseline.json" with { type: "json" }; /** Run one recall query through a tool and parse it (throws on a non-ok body). */ @@ -11,7 +12,7 @@ async function runQuery( args: Record, ): Promise { const raw = await tool.execute(args as Parameters[0], evalContext()); - const parsed = JSON.parse(raw) as SearchOutput | { ok: false; error: string }; + const parsed = JSON.parse(toolResultText(raw)) as SearchOutput | { ok: false; error: string }; if (!("ok" in parsed) || !parsed.ok) { throw new Error(`query failed: ${JSON.stringify(parsed)}`); } diff --git a/test/eval/semantic-plumbing.test.ts b/test/eval/semantic-plumbing.test.ts index f719b96..14b4bfe 100644 --- a/test/eval/semantic-plumbing.test.ts +++ b/test/eval/semantic-plumbing.test.ts @@ -1,7 +1,7 @@ import { afterAll, beforeAll, describe, expect, it } from "vitest"; import type { ToolDefinition } from "@opencode-ai/plugin"; import type { SearchOutput } from "../../src/types.js"; -import { TEST_LIMITS } from "../helpers.js"; +import { TEST_LIMITS, toolResultText } from "../helpers.js"; import { PROJECT_DIR, assistantMessage, @@ -178,7 +178,7 @@ async function run( } as Parameters[0], evalContext("plumbing-external"), ); - const parsed = JSON.parse(raw) as SearchOutput | { ok: false; error: string }; + const parsed = JSON.parse(toolResultText(raw)) as SearchOutput | { ok: false; error: string }; if (!("ok" in parsed) || !parsed.ok) throw new Error(`query failed: ${JSON.stringify(parsed)}`); return parsed; } diff --git a/test/eval/semantic.test.ts b/test/eval/semantic.test.ts index 2323586..db5df33 100644 --- a/test/eval/semantic.test.ts +++ b/test/eval/semantic.test.ts @@ -11,6 +11,7 @@ import { globalSessionFrom, session, textPart, + toolResultText, userMessage, } from "../helpers.js"; import type { EvalCorpus, MessageBundle } from "./corpus.js"; @@ -82,7 +83,7 @@ describe.skipIf(!process.env.RECALL_EVAL_SEMANTIC)( >[0], evalContext("sem-external"), ); - const out = JSON.parse(raw) as SearchOutput; + const out = JSON.parse(toolResultText(raw)) as SearchOutput; const top3 = out.results.slice(0, 3).map((r) => r.sessionID); expect(top3, `results: ${JSON.stringify(out.results.map((r) => r.sessionID))}`).toContain( "sem-work", @@ -275,7 +276,7 @@ describe.skipIf(!process.env.RECALL_EVAL_SEMANTIC)( } as Parameters[0], evalContext("soup-external"), ); - return JSON.parse(raw) as SearchOutput; + return JSON.parse(toolResultText(raw)) as SearchOutput; }; // Lexical-only baseline (no embedder) vs. the production default weight diff --git a/test/helpers.ts b/test/helpers.ts index aa50682..5bce5a9 100644 --- a/test/helpers.ts +++ b/test/helpers.ts @@ -842,6 +842,14 @@ function guardUnbounded(parsed: { ok: boolean; error?: unknown }): void { } } +/** Narrow a ToolResult to the JSON string every tool in this plugin returns. + * (Since @opencode-ai/plugin 1.18 `execute` may also return a structured + * object; this codebase never does.) */ +export function toolResultText(raw: unknown): string { + if (typeof raw !== "string") throw new Error(`tool returned a non-string result: ${typeof raw}`); + return raw; +} + export async function runTool( definition: ToolDefinition, rawArgs: Record, @@ -849,7 +857,7 @@ export async function runTool( ): Promise { const parsedArgs = tool.schema.object(definition.args).parse(rawArgs); const raw = await definition.execute(parsedArgs, ctx); - const parsed = JSON.parse(raw) as T; + const parsed = JSON.parse(toolResultText(raw)) as T; expect(parsed).toHaveProperty("ok"); guardUnbounded(parsed as { ok: boolean; error?: unknown }); return parsed; @@ -865,7 +873,7 @@ export async function runToolRaw( ctx = makeContext().ctx, ): Promise { const raw = await definition.execute(rawArgs as Parameters[0], ctx); - const parsed = JSON.parse(raw) as T; + const parsed = JSON.parse(toolResultText(raw)) as T; expect(parsed).toHaveProperty("ok"); guardUnbounded(parsed as { ok: boolean; error?: unknown }); return parsed; diff --git a/test/perf.test.ts b/test/perf.test.ts index 96d4ebd..dfb8052 100644 --- a/test/perf.test.ts +++ b/test/perf.test.ts @@ -46,6 +46,7 @@ import { paginateBundles, session, textPart, + toolResultText, userMessage, } from "./helpers.js"; @@ -524,7 +525,7 @@ describe.skipIf(!ENABLED)("perf: distill-then-search gates", () => { } as Parameters[0], perfContext(), ); - return JSON.parse(raw) as SearchOutput; + return JSON.parse(toolResultText(raw)) as SearchOutput; }; // Warm up (cold LRU/index), then measure a fresh drilled query. diff --git a/test/plugin.test.ts b/test/plugin.test.ts index e5fdf18..5fb016f 100644 --- a/test/plugin.test.ts +++ b/test/plugin.test.ts @@ -309,6 +309,62 @@ describe("plugin entry", () => { expect(sqliteLifecycle.postCloseCalls).toBe(0); }); + it("dispose completes within its bound when an SDK request never settles", async () => { + // The host awaits dispose as a shutdown finalizer with no timeout of its + // own; a never-settling in-flight fetch must not hang shutdown forever. + // The timeout still closes SQLite: every write path is stopped/finalized- + // guarded, so the detached fetch can never reach the store. + vi.useFakeTimers(); + try { + const messages = vi.fn(() => new Promise(() => {})); + createOpencodeClient + .mockImplementationOnce((options: unknown) => ({ + ...(options as object), + session: { messages, list: vi.fn(async () => ({ data: [] })) }, + })) + .mockImplementationOnce((options: unknown) => ({ + ...(options as object), + experimental: { + session: { + list: vi.fn(async () => ({ + data: [ + { + id: "s1", + title: "T", + directory: PROJECT_DIR, + time: { created: 1, updated: 2 }, + }, + ], + })), + }, + }, + })); + const hooks = await server(ctx({ fetch: vi.fn() }), { coldPass: true }); + for (let i = 0; i < 100 && messages.mock.calls.length === 0; i++) { + await Promise.resolve(); + } + expect(messages).toHaveBeenCalled(); + + let disposed = false; + const stopping = hooks.dispose?.().then(() => { + disposed = true; + }); + await Promise.resolve(); + expect(disposed).toBe(false); + + // Total dispose stays under ~20s worst case even though the SDK request + // never settles; SQLite is still closed exactly once, with no post-close + // access from the detached fetch. + await vi.advanceTimersByTimeAsync(20_000); + await stopping; + expect(disposed).toBe(true); + expect(sqliteLifecycle.closes).toBe(1); + expect(sqliteLifecycle.postCloseCalls).toBe(0); + } finally { + vi.useRealTimers(); + } + }); + it("disposes idempotently and rejects tools without touching SQLite afterwards", async () => { const hooks = await server(ctx({ fetch: vi.fn() }), {}); @@ -321,9 +377,13 @@ describe("plugin entry", () => { await hooks.event?.({ event: { type: "session.idle", properties: { sessionID: "s" } }, } as never); - await expect(mustTool(hooks.tool?.recall_sessions).execute({}, {} as never)).rejects.toThrow( - "opencode-session-recall: plugin has been disposed", - ); + // Disposed-tool calls return the codebase's JSON error-output shape, not a + // rejection (every other failure path resolves `{ ok:false, error }`). + const out = await mustTool(hooks.tool?.recall_sessions).execute({}, {} as never); + expect(JSON.parse(out as string)).toEqual({ + ok: false, + error: "opencode-session-recall: plugin has been disposed", + }); expect(sqliteLifecycle.closes).toBe(1); expect(sqliteLifecycle.postCloseCalls).toBe(0); }); diff --git a/test/summarize.test.ts b/test/summarize.test.ts index e6d594e..55320d8 100644 --- a/test/summarize.test.ts +++ b/test/summarize.test.ts @@ -30,6 +30,7 @@ import { paginateBundles, session, textPart, + toolResultText, userMessage, type SummaryPromptCall, type SummaryPromptResult, @@ -291,7 +292,7 @@ describe("summarizer worker-session exclusion", () => { { scope: "global", since: "30d" } as Parameters[0], ctx, ); - const out = JSON.parse(raw) as { sessions: Array<{ id: string }> }; + const out = JSON.parse(toolResultText(raw)) as { sessions: Array<{ id: string }> }; const ids = out.sessions.map((s) => s.id); expect(ids).toContain("real"); expect(ids).not.toContain("worker"); @@ -372,6 +373,30 @@ describe("summarizer worker lifecycle", () => { await summarizer.stop(); }); + it("still deletes its own worker when the lease is lost mid-batch (no leak)", async () => { + // Remote worker ownership is a separate authority from the SQLite writer + // lease: this instance created the worker and uniquely owns it, so losing + // the lease between create and cleanup must not leak the session. + const store = await freshStore(); + store.upsertCard(fullCard("c1")); + const gate = createFetchGate({ concurrency: 2 }); + let leaseHeld = true; + const client = makeSummarizerClient((call) => { + void call; + leaseHeld = false; // takeover lands while the prompt is in flight + return { text: "[]" }; + }); + const summarizer = makeSummarizer(store, client.client, gate, { + leaseHeld: () => leaseHeld, + }); + + await summarizer.runColdPass(); + + expect(client.calls.deletes).toEqual(["worker-1"]); + expect(client.liveWorkers()).toHaveLength(0); + await summarizer.stop(); + }); + it("disables tools and applies a deny-all permission on the worker prompt", async () => { const store = await freshStore(); store.upsertCard(fullCard("c1")); @@ -607,7 +632,9 @@ describe("nl_summary consumption", () => { { scope: "global", since: "30d" } as Parameters[0], ctx, ); - const out = JSON.parse(raw) as { sessions: Array<{ id: string; digest?: string }> }; + const out = JSON.parse(toolResultText(raw)) as { + sessions: Array<{ id: string; digest?: string }>; + }; expect(out.sessions.find((s) => s.id === "s1")?.digest).toBe("LLM summary about widgets."); }); }); diff --git a/test/tools.test.ts b/test/tools.test.ts index 7183d23..4ac3c40 100644 --- a/test/tools.test.ts +++ b/test/tools.test.ts @@ -165,13 +165,15 @@ describe("recall_messages", () => { ); expect(errorOut.error).toContain("Unauthorized"); - // A successful response without an array body is malformed, not an empty page. + // A session with no data returns an empty page rather than an error: the + // query path is deliberately lenient (only the distiller opts into strict). const noData = makeFakeHarness({ noMessageData: new Set(["s-current"]) }); - const noDataOut = await runTool( + const noDataOut = await runTool( messagesTool(noData.client, gate, TEST_LIMITS), {}, ); - expect(noDataOut.error).toBe("successful message response was not an array"); + expect(noDataOut.ok).toBe(true); + expect(noDataOut.pagination.returned).toBe(0); }); it("survives raw MCP-bypass args (undefined role/limit must not filter everything)", async () => { @@ -384,7 +386,7 @@ describe("recall_context", () => { sessionID: "s-current", messageID: "m-current-1", }); - expect(noDataOut.error).toBe("successful message response was not an array"); + expect(noDataOut.error).toBe("No messages returned"); }); it("survives raw MCP-bypass args (undefined window must not break slice bounds)", async () => { From 45b280573cab2a35496b2bef7cd16cbe3977e412 Mon Sep 17 00:00:00 2001 From: Rafi Khardalian Date: Tue, 25 Aug 2026 22:43:29 -0700 Subject: [PATCH 3/5] fix(shutdown): keep SQLite open when foreground operations outlive dispose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review fixes on the bounded-shutdown change-set. - Blocker: dispose no longer closes the DB when the tracked-operations wait times out. `operations` tracks FOREGROUND tool executions with no finalized/lease guards, so a recall paused in an SDK fetch could resume into store reads on a closed handle. A distiller-stop timeout still closes (its continuations are finalized/lease-guarded); an operations timeout skips the close — the process is exiting and the derived store is rebuildable, so an unclosed handle is harmless where a use-after-close is not. Comment rewritten to state what protects each path. New tests: a detached distiller fetch resolved AFTER dispose asserts zero post-close DB calls; a foreground tool operation outliving the bound asserts the close was skipped and the late resumption does not crash. - recordProgress (coldpass_cursor) is now gated on authoritative ownsLease() — it was the one distiller store write outside the stale-writer fence — checked only when the progress floor moves, so the hot skip path costs nothing extra. The cold-pass "done" transition and onColdPassDone() are gated the same way, so a demoted instance whose rollup recompute no-oped cannot declare success. - Page-walk catches in fetchSessionMessages/fetchNewMessages narrowed to TypeError (rethrow the rest), matching the quarantine principle. The deriveCard catch keeps MalformedSessionError|TypeError with a comment acknowledging the accepted tradeoff. - status().leaseHeld is now derived from the lease row status() already reads (holder === instanceId): authoritative AND side-effect-free, no extra SQLite read. Stale-writer test expectation updated. - Inter-page politeness sleeps are tracked and cancelled by quiesce(), so a long distillDelayMs cannot keep the runtime alive past dispose. - ownedWorkerSdk returns the SDK response; deleteOwnedWorker prunes the ownedWorkers id only on CONFIRMED success (resp without error) — a failed delete keeps the id for this instance's own retry. Comment corrected: the next holder's sweep deletes by sentinel regardless. - db.close() wrapped in try/catch so a throwing close cannot reject disposePromise into the host; redundant Promise.allSettled around summarizer.stop() replaced with a plain catch. - Orphan sweep comment documents the accepted winner-deletes-loser's- in-flight-worker race (benign: loser's results were lease-gated out). - fetchSessionMeta classifies a recognizable not-found (v2 `_tag` SessionNotFoundError or response.status 404) as absence (null), not a transport error. - CHANGELOG: "opencode >= 1.15.11" softened to "recent opencode versions"; bounded-shutdown entry describes the skip-close behavior; stale-writer entry now truthfully says every distiller store write is fenced (recordProgress included). --- CHANGELOG.md | 16 ++++--- src/distill.ts | 83 +++++++++++++++++++++++++++++++--- src/opencode-session-recall.ts | 31 +++++++++---- src/summarize.ts | 30 +++++++----- test/distill.test.ts | 9 ++-- test/plugin.test.ts | 83 +++++++++++++++++++++++++++++++--- 6 files changed, 211 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a4d112..ebf3405 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,13 +11,16 @@ Based on PR #3 by @kernel-oops with maintainer fixes. ### Added - **Bounded shutdown via the optional `dispose` hook.** On hosts that call - `dispose` (opencode ≥ 1.15.11), the plugin now shuts down in two phases: + `dispose` (recent opencode versions), the plugin now shuts down in two phases: an immediate quiesce (no new distill/summary/hook work starts) followed by a bounded drain of in-flight work before SQLite closes. Every wait is timeout-capped — a never-settling SDK request is detached, not awaited forever — so plugin disposal can never hang the host's shutdown. Detached - work is fenced by stop/finalize/lease guards and cannot touch the closed - store. There is **no new host requirement**: older hosts simply never call + distiller work is fenced by stop/finalize/lease guards and cannot touch the + closed store; if a foreground tool execution has not settled within the + bound, the store handle is deliberately left open (the process is exiting + and the store is derived/rebuildable) rather than risking a use-after-close. + There is **no new host requirement**: older hosts simply never call `dispose` and the plugin behaves exactly as before (no `engines` constraint was added). - **Cold-pass quarantine for malformed legacy sessions.** A session whose @@ -34,9 +37,10 @@ Based on PR #3 by @kernel-oops with maintainer fixes. ### Fixed -- **Stale-writer window closed.** Distiller writes (full/append/cold-pass - replaces, rollups, deletes) now re-verify authoritative lease ownership - against the live lease row immediately before each write transaction, so a +- **Stale-writer window closed.** Every distiller store write (full/append/ + cold-pass replaces, rollups, deletes, the cold-pass progress cursor) now + re-verifies authoritative lease ownership against the live lease row + immediately before the write, so a process suspended past the lease TTL cannot clobber the new holder's rows when its paused fetch resumes. The self-check demotes only when another holder's name is on the row — an expired-looking own heartbeat is not a diff --git a/src/distill.ts b/src/distill.ts index dcd6423..187206d 100644 --- a/src/distill.ts +++ b/src/distill.ts @@ -686,7 +686,31 @@ export function createDistiller(options: DistillerOptions): Distiller { * the next re-distill takes the full path rather than appending. */ const removalSince = new Set(); - const sleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)); + /** Cancellable inter-page politeness sleep. The timers are tracked so + * quiesce() can cancel them: an untracked setTimeout with a long + * distillDelayMs would otherwise keep the runtime alive past dispose. A + * cancelled sleep resolves immediately; the caller's stopped/lease check + * right after it does the actual bail-out. */ + const pageDelayTimers = new Map void>(); + const sleep = (ms: number): Promise => + new Promise((resolve) => { + if (stopped) { + resolve(); + return; + } + const timer = setTimeout(() => { + pageDelayTimers.delete(timer); + resolve(); + }, ms); + pageDelayTimers.set(timer, resolve); + }); + function cancelPageDelays(): void { + for (const [timer, resolve] of pageDelayTimers) { + clearTimeout(timer); + resolve(); + } + pageDelayTimers.clear(); + } const discover = options.discover ?? @@ -716,6 +740,25 @@ export function createDistiller(options: DistillerOptions): Distiller { return sessions.map(toMeta).filter((meta) => !isSummarizerTitle(meta.title)); } + /** Whether an SDK error return means "session does not exist" (absence) + * rather than a transport failure. Best-effort on both signals the SDK + * exposes: the v2 error body's `_tag` discriminant (`SessionNotFoundError`) + * and the fields-style `response.status` (404). Either matching → absence. */ + function isNotFoundError(error: unknown, response: unknown): boolean { + if ( + error && + typeof error === "object" && + (error as { _tag?: unknown })._tag === "SessionNotFoundError" + ) { + return true; + } + return ( + response != null && + typeof response === "object" && + (response as { status?: unknown }).status === 404 + ); + } + async function fetchSessionMeta(sessionID: string): Promise { const resp = await gate.runBackground(() => { if (stopped || !leaseHeld) return Promise.resolve(null); @@ -723,6 +766,9 @@ export function createDistiller(options: DistillerOptions): Distiller { }); if (!resp) return null; if (resp.error) { + // A recognizable not-found is ABSENCE (session gone → nothing to + // distill), not a transport failure to retry. + if (isNotFoundError(resp.error, resp.response)) return null; throw new SessionMetadataTransportError( `session ${sessionID} metadata fetch failed: ${errmsg(resp.error)}`, ); @@ -760,6 +806,10 @@ export function createDistiller(options: DistillerOptions): Distiller { for (const part of msg.parts) rowCount += distillFields(part).length; } } catch (error) { + // Only a TypeError here is a data-shape failure (e.g. `parts` missing + // on a malformed legacy message); anything else is a code regression + // that must abort the pass, not quarantine the session. + if (!(error instanceof TypeError)) throw error; throw new MalformedSessionError(errmsg(error)); } cursor = page.nextCursor ?? undefined; @@ -801,6 +851,10 @@ export function createDistiller(options: DistillerOptions): Distiller { collected.push(msg); } } catch (error) { + // TypeError only, matching fetchSessionMessages: a data-shape failure + // (`info` missing on a malformed message) quarantines; anything else + // is a regression and must surface. + if (!(error instanceof TypeError)) throw error; throw new MalformedSessionError(errmsg(error)); } cursor = reached ? undefined : (page.nextCursor ?? undefined); @@ -1051,6 +1105,9 @@ export function createDistiller(options: DistillerOptions): Distiller { // above): a TypeError walking malformed legacy parts is this // session's problem; anything else is a deriveCard regression that // must surface as a pass failure, not silently sideline sessions. + // Accepted tradeoff: a deriveCard TypeError REGRESSION quarantines + // rather than aborts; quarantinedCount + the 1000-entry cap bound + // the damage and make it diagnosable. if (!(error instanceof MalformedSessionError) && !(error instanceof TypeError)) { throw error; } @@ -1077,8 +1134,10 @@ export function createDistiller(options: DistillerOptions): Distiller { // Losing the lease (a takeover) or stopping mid-pass must NOT finish the // pass or recompute rollups as if complete — leave it idle so the new - // holder (or a restart) redoes the remainder. - if (stopped || !leaseHeld) { + // holder (or a restart) redoes the remainder. Authoritative check: a + // demoted instance whose rollup recompute silently no-oped must not + // declare "done" or kick the summarizer. + if (stopped || !ownsLease()) { coldPassState = "idle"; return; } @@ -1139,6 +1198,11 @@ export function createDistiller(options: DistillerOptions): Distiller { function recordProgress(timeUpdated: number): void { if (stopped || !leaseHeld) return; if (progressFloor == null || timeUpdated < progressFloor) { + // Authoritative ownership gate (same as every other distiller store + // write): this is reached from the quarantine branches and the common + // up-to-date skip path, all after awaited fetches. Checked only when the + // floor actually moves, so the hot skip path usually costs nothing extra. + if (!ownsLease()) return; progressFloor = timeUpdated; store.setMeta("coldpass_cursor", String(timeUpdated)); } @@ -1305,6 +1369,10 @@ export function createDistiller(options: DistillerOptions): Distiller { for (const timer of debounceTimers.values()) clearTimeout(timer); debounceTimers.clear(); pendingRerun.clear(); + // Wake any inter-page politeness sleep immediately: with a long + // distillDelayMs those timers would otherwise keep the runtime alive past + // dispose. The woken fetch loop bails on its stopped check. + cancelPageDelays(); // Deliberately retain heartbeatTimer: summary worker cleanup is lease-owned // and plugin disposal finalizes this distiller only after that cleanup has // settled or reached its shutdown bound. @@ -1373,10 +1441,13 @@ export function createDistiller(options: DistillerOptions): Distiller { status(): DistillStatus { const lease = store.leaseStatus(); - // Report the cached flag: status() must be side-effect-free (ownsLease() - // mutates lease state and re-arms timers; it is for write gates only). + // Derive leaseHeld from the lease row already read for the `lease` field: + // authoritative AND side-effect-free (unlike ownsLease(), which mutates + // lease state and re-arms timers — that one is for write gates only), and + // no extra SQLite read. `leaseHeld &&` keeps a released/finalized + // instance reporting false even if the row still names it briefly. return { - leaseHeld, + leaseHeld: leaseHeld && lease?.holder === instanceId, coldPass: coldPassState, distilledCount, knownCount, diff --git a/src/opencode-session-recall.ts b/src/opencode-session-recall.ts index 1676ecc..4036fe0 100644 --- a/src/opencode-session-recall.ts +++ b/src/opencode-session-recall.ts @@ -417,17 +417,32 @@ const server: Plugin = async (ctx, options) => { // Summarizer.stop() is internally bounded (its own 15s shutdown // timeout). Its late SDK promises are detached and ownership/lease // guarded, so phase 2 may safely release the lease once this settles - // even when an SDK request itself never does. - if (summarizer) await Promise.allSettled([summarizer.stop()]); + // even when an SDK request itself never does. The catch covers the rare + // rejecting drain (e.g. a store write threw) — shutdown must not care. + if (summarizer) await summarizer.stop().catch(() => {}); // Bound the remaining waits: an SDK request that never settles would // otherwise hang the host's shutdown (dispose is awaited untimed). - // Timing out does NOT skip db.close() — every distiller write path is - // finalized/lease-guarded, so a detached fetch that settles later can - // no longer reach SQLite. + // + // The two waits differ in what protects a detached continuation: + // - Distiller work is finalized/lease-guarded before every store + // read/write, so a distiller fetch that settles after the timeout can + // never reach SQLite. Closing after a distiller timeout is safe. + // - `operations` tracks FOREGROUND tool executions, which have no such + // guards: a paused recall/drill fetch can resume straight into card + // coverage reads. If they have not all settled, do NOT close — the + // process is exiting anyway, the store is derived/rebuildable, and an + // unclosed handle is harmless, whereas a use-after-close is not. await settleWithin(distiller.stop(), SHUTDOWN_TIMEOUT_MS); - await settleWithin(Promise.allSettled([...operations]), SHUTDOWN_TIMEOUT_MS); - db?.close(); - db = null; + const ops = await settleWithin(Promise.allSettled([...operations]), SHUTDOWN_TIMEOUT_MS); + if (!ops.timedOut) { + try { + db?.close(); + } catch { + // Best-effort: a throwing close must not reject disposePromise + // into the host's shutdown finalizer. + } + db = null; + } })(); return disposePromise; }, diff --git a/src/summarize.ts b/src/summarize.ts index 8b1aeb4..31a7521 100644 --- a/src/summarize.ts +++ b/src/summarize.ts @@ -307,19 +307,19 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { * worker ownership is a separate authority from the SQLite writer lease: * losing the lease (or stopping) must never leak a worker we uniquely own, * so this deliberately checks neither `stopped` nor `leaseHeld()` — only the - * caller's `ownedWorkers` membership scopes it. Returns whether the request - * settled in time (a timed-out delete leaves the id owned for a later retry - * by the next holder's orphan sweep). */ - async function ownedWorkerSdk( + * caller's `ownedWorkers` membership scopes it. Returns the SDK response, or + * undefined on timeout, so the caller can distinguish confirmed success + * (`resp` without `error`) from a failed or detached request. */ + async function ownedWorkerSdk( label: string, - operation: () => Promise, - ): Promise { + operation: () => Promise, + ): Promise { const result = await settleWithin(gate.runBackground(operation), shutdownTimeoutMs); if (result.timedOut) { logMsg(`${label} timed out after ${shutdownTimeoutMs}ms; late SDK settlement detached`); - return false; + return undefined; } - return true; + return result.value; } async function createWorker(): Promise { @@ -361,14 +361,17 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { try { // NOT lease-gated: this instance created the worker and uniquely owns it; // losing the SQLite writer lease mid-batch must not leak the session. - const settled = await ownedWorkerSdk("worker delete", () => + const resp = await ownedWorkerSdk("worker delete", () => client.session.delete({ sessionID }), ); - if (settled) ownedWorkers.delete(sessionID); + // Prune only on CONFIRMED success: an SDK error, rejection, or timeout + // keeps the id owned so this instance's own later cleanup can retry. + // (Retention only helps THIS instance — any next lease holder's orphan + // sweep deletes by sentinel title regardless of our bookkeeping.) + if (resp && !resp.error) ownedWorkers.delete(sessionID); } catch { // Best-effort; a lingering sentinel session is excluded everywhere and // swept by the next holder's orphan cleanup. - ownedWorkers.delete(sessionID); } } @@ -385,7 +388,10 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { } /** Delete any sentinel worker sessions left by a crashed prior holder, rather - * than adopting one whose accumulated context is unknown. Runs once. */ + * than adopting one whose accumulated context is unknown. Runs once. + * Accepted race: the sweep matches by sentinel title, so a fresh lease + * winner can delete a demoted loser's still-in-flight worker — benign, since + * the loser's results were lease-gated out of persistence anyway. */ async function deleteOrphans(): Promise { try { const resp = await leaseSdk("worker orphan list", () => diff --git a/test/distill.test.ts b/test/distill.test.ts index f51edcb..6c0d3d9 100644 --- a/test/distill.test.ts +++ b/test/distill.test.ts @@ -702,9 +702,12 @@ describe("cold pass", () => { clock += 31_000; expect(store.acquireLease("rival", 30_000, "rivalbuild", 4)).toBe(true); - // status() reports the CACHED flag (side-effect-free): the heartbeat - // callback has not run, so this instance still believes it holds the lease. - expect(distiller.status().leaseHeld).toBe(true); + // status() is authoritative WITHOUT side effects: it derives leaseHeld from + // the lease row it already reads for the `lease` field, so it reports the + // takeover even though the heartbeat callback has not run and the internal + // cached flag still says held. + expect(distiller.status().leaseHeld).toBe(false); + expect(distiller.status().lease?.holder).toBe("rival"); resolveMessages({}); await waitFor(() => distiller.status().coldPass !== "running"); diff --git a/test/plugin.test.ts b/test/plugin.test.ts index 5fb016f..9ef79ec 100644 --- a/test/plugin.test.ts +++ b/test/plugin.test.ts @@ -309,14 +309,22 @@ describe("plugin entry", () => { expect(sqliteLifecycle.postCloseCalls).toBe(0); }); - it("dispose completes within its bound when an SDK request never settles", async () => { + it("dispose completes within its bound when a distiller SDK request never settles", async () => { // The host awaits dispose as a shutdown finalizer with no timeout of its // own; a never-settling in-flight fetch must not hang shutdown forever. - // The timeout still closes SQLite: every write path is stopped/finalized- - // guarded, so the detached fetch can never reach the store. + // A distiller timeout still closes SQLite: every distiller path is + // stopped/finalized-guarded, so the detached fetch can never reach the + // store — proven below by resolving it AFTER dispose and asserting no + // post-close DB access. vi.useFakeTimers(); try { - const messages = vi.fn(() => new Promise(() => {})); + let resolveMessages: ((value: { data: [] }) => void) | undefined; + const messages = vi.fn( + () => + new Promise<{ data: [] }>((resolve) => { + resolveMessages = resolve; + }), + ); createOpencodeClient .mockImplementationOnce((options: unknown) => ({ ...(options as object), @@ -353,13 +361,76 @@ describe("plugin entry", () => { expect(disposed).toBe(false); // Total dispose stays under ~20s worst case even though the SDK request - // never settles; SQLite is still closed exactly once, with no post-close - // access from the detached fetch. + // has not settled; SQLite is still closed exactly once. await vi.advanceTimersByTimeAsync(20_000); await stopping; expect(disposed).toBe(true); expect(sqliteLifecycle.closes).toBe(1); expect(sqliteLifecycle.postCloseCalls).toBe(0); + + // The detached fetch resolving AFTER dispose must not reach the closed + // store: the resumed continuation bails on its stopped/finalized guards + // before any store read/write. + resolveMessages?.({ data: [] }); + for (let i = 0; i < 20; i++) await Promise.resolve(); + expect(sqliteLifecycle.postCloseCalls).toBe(0); + } finally { + vi.useRealTimers(); + } + }); + + it("skips the DB close when a foreground tool operation outlives the dispose bound", async () => { + // `operations` tracks FOREGROUND tool executions, which have no + // finalized/lease guards: a recall paused in an SDK fetch can resume into + // card/store reads. When that wait times out, dispose deliberately does + // NOT close SQLite (the process is exiting; the derived store rebuilds) so + // the late resumption cannot use-after-close. + vi.useFakeTimers(); + try { + let resolveMessages: ((value: { data: [] }) => void) | undefined; + const messages = vi.fn( + () => + new Promise<{ data: [] }>((resolve) => { + resolveMessages = resolve; + }), + ); + const get = vi.fn(async () => ({ data: undefined })); + createOpencodeClient + .mockImplementationOnce((options: unknown) => ({ + ...(options as object), + session: { messages, get, list: vi.fn(async () => ({ data: [] })) }, + })) + .mockImplementationOnce((options: unknown) => ({ + ...(options as object), + experimental: { session: { list: vi.fn(async () => ({ data: [] })) } }, + })); + const hooks = await server(ctx({ fetch: vi.fn() }), {}); + + // Start a foreground tool call that parks inside the SDK fetch. + const toolRun = mustTool(hooks.tool?.recall_messages).execute({ sessionID: "s-park" }, { + sessionID: "s-park", + metadata: () => {}, + } as never); + for (let i = 0; i < 100 && messages.mock.calls.length === 0; i++) { + await Promise.resolve(); + } + expect(messages).toHaveBeenCalled(); + + let disposed = false; + const stopping = hooks.dispose?.().then(() => { + disposed = true; + }); + await vi.advanceTimersByTimeAsync(20_000); + await stopping; + expect(disposed).toBe(true); + // The operations wait timed out → the close was skipped. + expect(sqliteLifecycle.closes).toBe(0); + + // The parked tool resuming afterwards must not crash or hit a closed + // handle (there is none to hit — the close was skipped). + resolveMessages?.({ data: [] }); + await toolRun; + expect(sqliteLifecycle.postCloseCalls).toBe(0); } finally { vi.useRealTimers(); } From 953f0469d4441b200d330bec31d2b658c468831f Mon Sep 17 00:00:00 2001 From: Rafi Khardalian Date: Thu, 27 Aug 2026 18:24:18 -0700 Subject: [PATCH 4/5] fix(shutdown): defer the DB close behind outliving foreground operations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3 review fixes. - Blocker: replace skip-close with deferred close. Skipping leaked the SQLite handle when opencode disposes a cached per-directory instance without the process exiting (cache eviction/reload). When the operations wait times out, dispose now schedules the close behind a fresh Promise.allSettled of the stragglers: it fires after disposePromise resolved (cannot hang the host), never under a live reader, and closes eventually. A truly never-settling straggler degrades to the old skip behavior. Test extended: after the parked tool resolves, the deferred close fires (closes === 1) with zero post-close calls; a comment notes recall_messages is a proxy and the sqlite mock proxies every store method, so any tool's post-close store call would trip the counter. - isNotFoundError: session.get's real 404 body is the generic NotFoundError discriminated by name:"NotFoundError" (SessionGetErrors in the v2 typings), not _tag:"SessionNotFoundError" (another endpoint's shape). Added the name check as primary; _tag and response.status 404 remain as fallbacks; docstring corrected. New tests: name:"NotFoundError" → absence (no lastError, no quarantine, card untouched, no retry noise); apiFailure with neither signal → transport path with lastError surfaced. - ownedWorkers retry is now real: drainOwnedWorkers() at the start of each batch best-effort re-deletes leftover ids (bounded — each delete is already settleWithin-capped), pruning on success and keeping on failure. Comment updated to name the actual retry path. Test: a failed delete's id is retried and pruned by the next batch's drain, nothing leaked. - TypeError quarantine breadth held deliberately (gpt5's objection acknowledged, not converted): both page-walk catches now cross- reference the deriveCard accepted-tradeoff comment, which is strengthened to name the residual risk explicitly — a TypeError regression in distillFields/deriveCard quarantines while the pass reports done; bounded by quarantinedCount observability and the 1000-entry cap; accepted because annotating every field access is worse. Perf (RECALL_PERF=1, after the leaseStatus() reads landed): tier-1 rank p95 10.45ms (<50 budget), distill+replace p50 0.85ms (<150), ftsSearch p95 0.96ms (<100), e2e drilled query 20.0ms (<1500), heap 57.9MB (<150). --- src/distill.ts | 39 +++++++++++++++-------- src/opencode-session-recall.ts | 23 +++++++++----- src/summarize.ts | 20 ++++++++++-- test/distill.test.ts | 58 ++++++++++++++++++++++++++++++++++ test/plugin.test.ts | 28 ++++++++++++---- test/summarize.test.ts | 36 +++++++++++++++++++++ 6 files changed, 172 insertions(+), 32 deletions(-) diff --git a/src/distill.ts b/src/distill.ts index 187206d..1400b9d 100644 --- a/src/distill.ts +++ b/src/distill.ts @@ -741,16 +741,17 @@ export function createDistiller(options: DistillerOptions): Distiller { } /** Whether an SDK error return means "session does not exist" (absence) - * rather than a transport failure. Best-effort on both signals the SDK - * exposes: the v2 error body's `_tag` discriminant (`SessionNotFoundError`) - * and the fields-style `response.status` (404). Either matching → absence. */ + * rather than a transport failure. `session.get`'s 404 body is the generic + * `NotFoundError` discriminated by `name: "NotFoundError"` (SessionGetErrors + * in the v2 typings) — that is the primary check. The `_tag: + * "SessionNotFoundError"` shape belongs to OTHER endpoints' 404s and the + * fields-style `response.status` is transport-level; both are kept as + * belt-and-suspenders fallbacks. Any matching signal → absence. */ function isNotFoundError(error: unknown, response: unknown): boolean { - if ( - error && - typeof error === "object" && - (error as { _tag?: unknown })._tag === "SessionNotFoundError" - ) { - return true; + if (error && typeof error === "object") { + const shaped = error as { name?: unknown; _tag?: unknown }; + if (shaped.name === "NotFoundError") return true; + if (shaped._tag === "SessionNotFoundError") return true; } return ( response != null && @@ -808,7 +809,9 @@ export function createDistiller(options: DistillerOptions): Distiller { } catch (error) { // Only a TypeError here is a data-shape failure (e.g. `parts` missing // on a malformed legacy message); anything else is a code regression - // that must abort the pass, not quarantine the session. + // that must abort the pass, not quarantine the session. TypeError is a + // proxy, not a proof — see the accepted-tradeoff note at the cold + // pass's deriveCard catch for the residual risk this carries. if (!(error instanceof TypeError)) throw error; throw new MalformedSessionError(errmsg(error)); } @@ -853,7 +856,8 @@ export function createDistiller(options: DistillerOptions): Distiller { } catch (error) { // TypeError only, matching fetchSessionMessages: a data-shape failure // (`info` missing on a malformed message) quarantines; anything else - // is a regression and must surface. + // is a regression and must surface. Same residual risk as there — see + // the accepted-tradeoff note at the cold pass's deriveCard catch. if (!(error instanceof TypeError)) throw error; throw new MalformedSessionError(errmsg(error)); } @@ -1105,9 +1109,16 @@ export function createDistiller(options: DistillerOptions): Distiller { // above): a TypeError walking malformed legacy parts is this // session's problem; anything else is a deriveCard regression that // must surface as a pass failure, not silently sideline sessions. - // Accepted tradeoff: a deriveCard TypeError REGRESSION quarantines - // rather than aborts; quarantinedCount + the 1000-entry cap bound - // the damage and make it diagnosable. + // + // ACCEPTED TRADEOFF (referenced by the page-walk catches above): + // TypeError is a proxy for "malformed data", not a proof. A + // TypeError REGRESSION in distillFields/deriveCard — a bug of ours + // that happens to throw TypeError — quarantines sessions while the + // pass reports "done" instead of aborting. Residual risk accepted + // because the alternative (annotating every field access to + // distinguish data-shape from code-bug TypeErrors) is worse; + // bounded by quarantinedCount observability in status() and the + // MAX_QUARANTINED_SESSIONS (1000) cap. if (!(error instanceof MalformedSessionError) && !(error instanceof TypeError)) { throw error; } diff --git a/src/opencode-session-recall.ts b/src/opencode-session-recall.ts index 4036fe0..8e142f3 100644 --- a/src/opencode-session-recall.ts +++ b/src/opencode-session-recall.ts @@ -429,20 +429,27 @@ const server: Plugin = async (ctx, options) => { // never reach SQLite. Closing after a distiller timeout is safe. // - `operations` tracks FOREGROUND tool executions, which have no such // guards: a paused recall/drill fetch can resume straight into card - // coverage reads. If they have not all settled, do NOT close — the - // process is exiting anyway, the store is derived/rebuildable, and an - // unclosed handle is harmless, whereas a use-after-close is not. - await settleWithin(distiller.stop(), SHUTDOWN_TIMEOUT_MS); - const ops = await settleWithin(Promise.allSettled([...operations]), SHUTDOWN_TIMEOUT_MS); - if (!ops.timedOut) { + // coverage reads. If they have not all settled within the bound, + // DEFER the close instead: it fires after disposePromise has resolved + // (cannot hang the host), only once every straggler has settled + // (never closes under a live reader), and so closes eventually — no + // handle leak when opencode disposes a cached per-directory instance + // without the process exiting. A straggler that truly never settles + // degrades to an unclosed handle on a derived, rebuildable store — + // harmless, unlike a use-after-close. + const closeDb = (): void => { try { db?.close(); } catch { // Best-effort: a throwing close must not reject disposePromise - // into the host's shutdown finalizer. + // (or escape the detached deferral) into the host. } db = null; - } + }; + await settleWithin(distiller.stop(), SHUTDOWN_TIMEOUT_MS); + const ops = await settleWithin(Promise.allSettled([...operations]), SHUTDOWN_TIMEOUT_MS); + if (ops.timedOut) void Promise.allSettled([...operations]).then(closeDb); + else closeDb(); })(); return disposePromise; }, diff --git a/src/summarize.ts b/src/summarize.ts index 31a7521..f87a211 100644 --- a/src/summarize.ts +++ b/src/summarize.ts @@ -365,9 +365,10 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { client.session.delete({ sessionID }), ); // Prune only on CONFIRMED success: an SDK error, rejection, or timeout - // keeps the id owned so this instance's own later cleanup can retry. - // (Retention only helps THIS instance — any next lease holder's orphan - // sweep deletes by sentinel title regardless of our bookkeeping.) + // keeps the id owned so drainOwnedWorkers retries it at the start of the + // next batch. (Retention only helps THIS instance — any next lease + // holder's orphan sweep deletes by sentinel title regardless of our + // bookkeeping.) if (resp && !resp.error) ownedWorkers.delete(sessionID); } catch { // Best-effort; a lingering sentinel session is excluded everywhere and @@ -375,6 +376,17 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { } } + /** Best-effort retry of leftover owned ids (deletes that failed or timed out + * in earlier batches). Runs at the start of each batch, before creating the + * new worker. Bounded: at most a few ids, each delete already capped by + * settleWithin inside ownedWorkerSdk; success prunes, failure keeps the id + * for the next batch's drain. */ + async function drainOwnedWorkers(): Promise { + for (const sessionID of [...ownedWorkers]) { + await deleteOwnedWorker(sessionID); + } + } + async function abortOwnedWorker(sessionID: string): Promise { if (!ownedWorkers.has(sessionID)) return; try { @@ -423,6 +435,8 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { * permit that long would only starve foreground recall for no concurrency * benefit. Worker create/delete/list/abort stay gated (quick server fetches). */ async function promptBatchFor(cards: Card[]): Promise> { + // Retry any leftover owned workers from earlier batches before adding one. + await drainOwnedWorkers(); const workerId = await createWorker(); if (!workerId) return new Map(); try { diff --git a/test/distill.test.ts b/test/distill.test.ts index 6c0d3d9..cc16b3a 100644 --- a/test/distill.test.ts +++ b/test/distill.test.ts @@ -70,6 +70,8 @@ function makeDistillFake( throwOnce?: Set; nonArray?: Set; metadataErrorOnce?: Set; + /** session.get returns the v2 404 body (`name: "NotFoundError"`). */ + metadataNotFound?: Set; } = {}, ): { client: OpencodeClient; sdk: SdkCalls } { const sdk: SdkCalls = { list: 0, get: 0, messages: [] }; @@ -86,6 +88,12 @@ function makeDistillFake( threw.add(`meta:${sessionID}`); return { error: apiFailure(`metadata transport failed: ${sessionID}`) }; } + if (opts.metadataNotFound?.has(sessionID)) { + // The real SessionGetErrors[404] shape from the v2 typings. + return { + error: { name: "NotFoundError", data: { message: `session not found: ${sessionID}` } }, + }; + } const found = graph.sessions.find((s) => s.id === sessionID); return found ? { data: found } : { error: apiFailure(`not found: ${sessionID}`) }; }, @@ -1444,6 +1452,9 @@ describe("incremental", () => { expect(store.getCard("s1")).toEqual(preserved); expect(logs.some((line) => line.includes("session s1 re-distill failed:"))).toBe(true); expect(logs.some((line) => line.includes("metadata transport failed: s1"))).toBe(true); + // The apiFailure shape carries neither not-found signal (no name/_tag/404 + // status), so it takes the transport path and surfaces in lastError. + expect(distiller.status().lastError).toContain("metadata transport failed: s1"); // A later idle event retries normally; the transport failure was neither // interpreted as deletion nor quarantined as malformed session data. @@ -1455,6 +1466,53 @@ describe("incremental", () => { db.close(); }); + it("treats a NotFoundError metadata response as absence: silent, card preserved", async () => { + // session.get's real 404 body (`name: "NotFoundError"`, per the v2 + // SessionGetErrors typing) means the session is GONE — absence, not a + // transport failure: no lastError, no quarantine, existing card untouched, + // no retry noise in the logs. + const graph: Graph = { + sessions: [session("s1", "Alpha", PROJECT_DIR, 3000)], + messagesBySession: { + s1: [ + bundle(userMessage("m1", "s1", 100), [ + textPart("p1", "s1", "m1", "preserved not-found marker"), + ]), + ], + }, + }; + const { client, sdk } = makeDistillFake(graph, { + metadataNotFound: new Set(["s1"]), + }); + const { db, store } = await freshStore(); + const { gate } = makeSpyGate(); + const logs: string[] = []; + const distiller = createDistiller({ + client, + store, + gate, + limits: TEST_LIMITS, + instanceId: "metadata-not-found", + idleDebounceMs: 5, + log: (message) => logs.push(message), + }); + distiller.start(); + await waitFor(() => distiller.status().coldPass === "done"); + const preserved = store.getCard("s1"); + expect(preserved).toBeDefined(); + + distiller.onEvent(idleEvent("s1")); + await waitFor(() => sdk.get === 1); + await new Promise((resolve) => setTimeout(resolve, 10)); + + expect(store.getCard("s1")).toEqual(preserved); // untouched + expect(distiller.status().lastError).toBeUndefined(); // no error surfaced + expect(distiller.status().quarantinedCount).toBe(0); // not quarantined + expect(logs.some((line) => line.includes("re-distill failed"))).toBe(false); // no retry noise + await distiller.stop(); + db.close(); + }); + it("coalesces an idle burst into a single re-distill", async () => { const graph: Graph = { sessions: [session("s1", "Alpha", PROJECT_DIR, 3000)], diff --git a/test/plugin.test.ts b/test/plugin.test.ts index 9ef79ec..94d7e90 100644 --- a/test/plugin.test.ts +++ b/test/plugin.test.ts @@ -379,12 +379,14 @@ describe("plugin entry", () => { } }); - it("skips the DB close when a foreground tool operation outlives the dispose bound", async () => { + it("defers the DB close until a foreground tool operation outliving dispose settles", async () => { // `operations` tracks FOREGROUND tool executions, which have no // finalized/lease guards: a recall paused in an SDK fetch can resume into - // card/store reads. When that wait times out, dispose deliberately does - // NOT close SQLite (the process is exiting; the derived store rebuilds) so - // the late resumption cannot use-after-close. + // card/store reads. When that wait times out, dispose DEFERS the close + // until every tracked operation has settled, so the late resumption can + // never use-after-close, and the handle still closes eventually (no leak + // when opencode disposes a cached per-directory instance without the + // process exiting). vi.useFakeTimers(); try { let resolveMessages: ((value: { data: [] }) => void) | undefined; @@ -407,6 +409,11 @@ describe("plugin entry", () => { const hooks = await server(ctx({ fetch: vi.fn() }), {}); // Start a foreground tool call that parks inside the SDK fetch. + // recall_messages is a proxy for the riskier session-scoped recall/drill + // path (wiring the full search tool through this fake needs a card + // corpus); fidelity holds because the sqlite mock proxies EVERY store + // method, so ANY post-close store call from any tool's resumption would + // trip postCloseCalls. const toolRun = mustTool(hooks.tool?.recall_messages).execute({ sessionID: "s-park" }, { sessionID: "s-park", metadata: () => {}, @@ -423,14 +430,21 @@ describe("plugin entry", () => { await vi.advanceTimersByTimeAsync(20_000); await stopping; expect(disposed).toBe(true); - // The operations wait timed out → the close was skipped. + // The operations wait timed out → the close is DEFERRED, not yet fired: + // dispose resolved without closing under the still-parked reader. expect(sqliteLifecycle.closes).toBe(0); - // The parked tool resuming afterwards must not crash or hit a closed - // handle (there is none to hit — the close was skipped). + // The parked tool resumes and completes without hitting a closed handle + // (the deferred close only fires after it settles) … resolveMessages?.({ data: [] }); await toolRun; expect(sqliteLifecycle.postCloseCalls).toBe(0); + + // … and once every straggler has settled, the deferred close DOES fire: + // no handle leak on instance disposal without process exit. + for (let i = 0; i < 20; i++) await Promise.resolve(); + expect(sqliteLifecycle.closes).toBe(1); + expect(sqliteLifecycle.postCloseCalls).toBe(0); } finally { vi.useRealTimers(); } diff --git a/test/summarize.test.ts b/test/summarize.test.ts index 55320d8..38cb312 100644 --- a/test/summarize.test.ts +++ b/test/summarize.test.ts @@ -397,6 +397,42 @@ describe("summarizer worker lifecycle", () => { await summarizer.stop(); }); + it("retries a failed owned-worker delete on the next batch's drain and prunes it", async () => { + // A delete that fails keeps the id in ownedWorkers; the start-of-batch + // drain retries it before creating the next worker, and prunes on success. + const store = await freshStore(); + store.upsertCard(fullCard("c1", { timeUpdated: 2000 })); + store.upsertCard(fullCard("c2", { timeUpdated: 3000 })); + const gate = createFetchGate({ concurrency: 2 }); + const client = makeSummarizerClient(replyKeys(() => "s")); + const realDelete = ( + client.client as unknown as { + session: { delete: (p: { sessionID: string }) => Promise }; + } + ).session.delete; + let failNext = true; + const deletes: string[] = []; + ( + client.client as unknown as { + session: { delete: (p: { sessionID: string }) => Promise }; + } + ).session.delete = async (params) => { + deletes.push(params.sessionID); + if (failNext) { + failNext = false; + return { error: { data: { message: "delete failed" } } }; + } + return realDelete(params); + }; + + await makeSummarizer(store, client.client, gate, { batchSize: 1 }).runColdPass(); + + // Batch 1's delete of worker-1 failed; batch 2's drain retried worker-1 + // (successfully) before its own worker-2 create+delete. + expect(deletes).toEqual(["worker-1", "worker-1", "worker-2"]); + expect(client.liveWorkers()).toHaveLength(0); // nothing leaked + }); + it("disables tools and applies a deny-all permission on the worker prompt", async () => { const store = await freshStore(); store.upsertCard(fullCard("c1")); From 8d8f733f323c1ff8f63a844eeb0aad4a9fbc29e0 Mon Sep 17 00:00:00 2001 From: Rafi Khardalian Date: Thu, 27 Aug 2026 18:33:40 -0700 Subject: [PATCH 5/5] fix(summarize): bound the owned-worker drain in aggregate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-4 review fix: drainOwnedWorkers was bounded per delete but unbounded in aggregate — persistent failures grew the retained set one id per batch (O(n²) deletes across a pass; with timeouts instead of errors, (k-1)×15s serial waits wedging the pass while holding the lease). - Cap the drain at MAX_DRAIN_PER_BATCH (3) ids per batch. - Give-up counter: after MAX_DELETE_ATTEMPTS (2) failed deletes an id is dropped from ownedWorkers and the attempts map (logged). Safe: the next holder's sentinel-based orphan sweep reaps it, and sentinel-titled sessions are excluded from every recall path meanwhile. Attempts are also cleared on successful delete. - Bounded-backlog policy: a backlog still at/over MAX_DRAIN_PER_BATCH after the drain means deletes are persistently failing — skip the batch (no new worker) instead of adding to the leak; the give-up shrinks the backlog so later batches proceed. - pluginLog on the deferred-close branch in dispose (count + bound, plus a line when the deferred close fires) so a never-settling straggler's open handle is diagnosable instead of silent. - Drain comment states the real bounds (per-batch cap, 2-attempt give-up, sentinel sweep backstop). - Tests: persistent failure → no id delete-attempted more than twice, give-up logged; saturated backlog (give-up delayed via the new maxDeleteAttempts test affordance) → batches skip worker creation (creates capped at 3). Added the missing await summarizer.stop() in the round-3 retry test. --- src/opencode-session-recall.ts | 15 ++++++- src/summarize.ts | 58 +++++++++++++++++++++++---- test/summarize.test.ts | 72 +++++++++++++++++++++++++++++++++- 3 files changed, 134 insertions(+), 11 deletions(-) diff --git a/src/opencode-session-recall.ts b/src/opencode-session-recall.ts index 8e142f3..bf0bc42 100644 --- a/src/opencode-session-recall.ts +++ b/src/opencode-session-recall.ts @@ -448,8 +448,19 @@ const server: Plugin = async (ctx, options) => { }; await settleWithin(distiller.stop(), SHUTDOWN_TIMEOUT_MS); const ops = await settleWithin(Promise.allSettled([...operations]), SHUTDOWN_TIMEOUT_MS); - if (ops.timedOut) void Promise.allSettled([...operations]).then(closeDb); - else closeDb(); + if (ops.timedOut) { + // Diagnosable, not silent: until the stragglers settle, the SQLite + // handle stays open — this line is the marker if it never closes. + pluginLog( + `dispose: ${operations.size} operation(s) outlived the ${SHUTDOWN_TIMEOUT_MS}ms bound; deferring SQLite close until they settle`, + ); + void Promise.allSettled([...operations]).then(() => { + pluginLog("dispose: deferred SQLite close firing (stragglers settled)"); + closeDb(); + }); + } else { + closeDb(); + } })(); return disposePromise; }, diff --git a/src/summarize.ts b/src/summarize.ts index f87a211..e7a4910 100644 --- a/src/summarize.ts +++ b/src/summarize.ts @@ -44,6 +44,14 @@ const DEFAULT_IDLE_DEBOUNCE_MS = 3_000; /** Abort a drain after this many prompts fail in a row: a misconfigured model * must not burn the whole per-pass budget. Latches until the next cold pass. */ const MAX_CONSECUTIVE_FAILURES = 3; +/** Leftover owned-worker deletes retried per batch (aggregate bound: without + * it, persistent failures make batch k drain k-1 ids — O(n²) deletes, or + * worse, serial 15s timeouts wedging the pass while holding the lease). */ +const MAX_DRAIN_PER_BATCH = 3; +/** Failed delete attempts per owned worker before giving up on it. The next + * holder's sentinel-based orphan sweep is the backstop, and sentinel-titled + * sessions are excluded from every recall path, so giving up is safe. */ +const MAX_DELETE_ATTEMPTS = 2; const INVENTORY_TOKENS = 24; const FILES_SHOWN = 6; const TOOLS_SHOWN = 8; @@ -110,6 +118,9 @@ export type SummarizerDeps = { promptTimeoutMs?: number; shutdownTimeoutMs?: number; idleDebounceMs?: number; + /** Failed delete attempts per owned worker before the give-up drops it + * (spec: MAX_DELETE_ATTEMPTS). */ + maxDeleteAttempts?: number; }; export type Summarizer = { @@ -256,6 +267,7 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { const politenessMs = Math.max(0, deps.politenessMs ?? DEFAULT_POLITENESS_MS); const promptTimeoutMs = Math.max(1, deps.promptTimeoutMs ?? DEFAULT_PROMPT_TIMEOUT_MS); const shutdownTimeoutMs = Math.max(1, deps.shutdownTimeoutMs ?? DEFAULT_SHUTDOWN_TIMEOUT_MS); + const maxDeleteAttempts = Math.max(1, deps.maxDeleteAttempts ?? MAX_DELETE_ATTEMPTS); const idleDebounceMs = Math.max(0, deps.idleDebounceMs ?? DEFAULT_IDLE_DEBOUNCE_MS); const maxPromptsPerPass = Math.max(1, config.maxPromptsPerPass ?? DEFAULT_MAX_PROMPTS_PER_PASS); @@ -280,6 +292,8 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { let stopPromise: Promise | undefined; const debounceTimers = new Map(); const ownedWorkers = new Set(); + /** Failed delete attempts per owned worker id (see MAX_DELETE_ATTEMPTS). */ + const deleteAttempts = new Map(); const workerTitle = summarizerWorkerTitle(ownerToken); // ── Worker session lifecycle ── @@ -358,6 +372,7 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { async function deleteOwnedWorker(sessionID: string): Promise { if (!ownedWorkers.has(sessionID)) return; + let succeeded = false; try { // NOT lease-gated: this instance created the worker and uniquely owns it; // losing the SQLite writer lease mid-batch must not leak the session. @@ -366,23 +381,42 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { ); // Prune only on CONFIRMED success: an SDK error, rejection, or timeout // keeps the id owned so drainOwnedWorkers retries it at the start of the - // next batch. (Retention only helps THIS instance — any next lease - // holder's orphan sweep deletes by sentinel title regardless of our - // bookkeeping.) - if (resp && !resp.error) ownedWorkers.delete(sessionID); + // next batch (up to MAX_DELETE_ATTEMPTS failures). + succeeded = resp != null && !resp.error; } catch { // Best-effort; a lingering sentinel session is excluded everywhere and // swept by the next holder's orphan cleanup. } + if (succeeded) { + ownedWorkers.delete(sessionID); + deleteAttempts.delete(sessionID); + return; + } + const attempts = (deleteAttempts.get(sessionID) ?? 0) + 1; + if (attempts >= maxDeleteAttempts) { + // Give up: drop the id so the retained set stays bounded. Safe because + // the next lease holder's sentinel-based orphan sweep deletes it, and + // sentinel-titled sessions are excluded from every recall path meanwhile. + ownedWorkers.delete(sessionID); + deleteAttempts.delete(sessionID); + logMsg( + `worker ${sessionID} delete gave up after ${attempts} attempts; orphan sweep will reap it`, + ); + } else { + deleteAttempts.set(sessionID, attempts); + } } /** Best-effort retry of leftover owned ids (deletes that failed or timed out * in earlier batches). Runs at the start of each batch, before creating the - * new worker. Bounded: at most a few ids, each delete already capped by - * settleWithin inside ownedWorkerSdk; success prunes, failure keeps the id - * for the next batch's drain. */ + * new worker. Bounds: at most MAX_DRAIN_PER_BATCH deletes per batch (each + * already settleWithin-capped), and each id is retried at most + * MAX_DELETE_ATTEMPTS times before deleteOwnedWorker drops it — the next + * holder's sentinel-based orphan sweep is the backstop for dropped ids. + * Success prunes; a failure under the attempt cap keeps the id for the next + * batch's drain. */ async function drainOwnedWorkers(): Promise { - for (const sessionID of [...ownedWorkers]) { + for (const sessionID of [...ownedWorkers].slice(0, MAX_DRAIN_PER_BATCH)) { await deleteOwnedWorker(sessionID); } } @@ -437,6 +471,14 @@ export function createSummarizer(deps: SummarizerDeps): Summarizer { async function promptBatchFor(cards: Card[]): Promise> { // Retry any leftover owned workers from earlier batches before adding one. await drainOwnedWorkers(); + // Bounded-backlog policy: a still-saturated backlog after the drain means + // deletes are persistently failing. Skip this batch rather than creating + // yet another worker that will likely join the leak; the per-id give-up in + // deleteOwnedWorker shrinks the backlog so a later batch proceeds. + if (ownedWorkers.size >= MAX_DRAIN_PER_BATCH) { + logMsg(`skipping batch: ${ownedWorkers.size} owned workers still undeleted after drain`); + return new Map(); + } const workerId = await createWorker(); if (!workerId) return new Map(); try { diff --git a/test/summarize.test.ts b/test/summarize.test.ts index 38cb312..ba5a1a3 100644 --- a/test/summarize.test.ts +++ b/test/summarize.test.ts @@ -32,6 +32,7 @@ import { textPart, toolResultText, userMessage, + type SummarizerClient, type SummaryPromptCall, type SummaryPromptResult, } from "./helpers.js"; @@ -425,12 +426,81 @@ describe("summarizer worker lifecycle", () => { return realDelete(params); }; - await makeSummarizer(store, client.client, gate, { batchSize: 1 }).runColdPass(); + const summarizer = makeSummarizer(store, client.client, gate, { batchSize: 1 }); + await summarizer.runColdPass(); // Batch 1's delete of worker-1 failed; batch 2's drain retried worker-1 // (successfully) before its own worker-2 create+delete. expect(deletes).toEqual(["worker-1", "worker-1", "worker-2"]); expect(client.liveWorkers()).toHaveLength(0); // nothing leaked + await summarizer.stop(); + }); + + /** Replace the fake client's delete with one that always fails, recording ids. */ + function failAllDeletes(client: SummarizerClient): string[] { + const deletes: string[] = []; + ( + client.client as unknown as { + session: { delete: (p: { sessionID: string }) => Promise }; + } + ).session.delete = async (params) => { + deletes.push(params.sessionID); + return { error: { data: { message: "delete always fails" } } }; + }; + return deletes; + } + + it("gives up on a persistently failing delete after 2 attempts (set stays bounded)", async () => { + // Persistent delete failures must not grow ownedWorkers one id per batch + // (O(n^2) deletes / serial timeout wedge): each id is dropped after + // MAX_DELETE_ATTEMPTS (2) failures — the sentinel orphan sweep reaps it. + const store = await freshStore(); + for (let i = 0; i < 4; i++) store.upsertCard(fullCard(`c${i}`, { timeUpdated: 2000 + i })); + const gate = createFetchGate({ concurrency: 2 }); + const client = makeSummarizerClient(replyKeys(() => "s")); + const deletes = failAllDeletes(client); + + const logs: string[] = []; + const summarizer = makeSummarizer(store, client.client, gate, { + batchSize: 1, + log: (message) => logs.push(message), + }); + await summarizer.runColdPass(); + + // No id is delete-attempted more than twice (its own batch's finally plus + // at most one drain retry) before the give-up drops it. + const countsById = new Map(); + for (const id of deletes) countsById.set(id, (countsById.get(id) ?? 0) + 1); + for (const [id, count] of countsById) { + expect(count, `delete attempts for ${id}`).toBeLessThanOrEqual(2); + } + expect(logs.some((line) => line.includes("delete gave up after 2 attempts"))).toBe(true); + await summarizer.stop(); + }); + + it("skips worker creation while the retained backlog stays saturated after a drain", async () => { + // Bounded-backlog policy: with the give-up delayed (high maxDeleteAttempts) + // the retained set grows to MAX_DRAIN_PER_BATCH (3); from then on each + // batch drains at most 3 and — still saturated — skips creating another + // worker instead of adding to the leak. + const store = await freshStore(); + for (let i = 0; i < 6; i++) store.upsertCard(fullCard(`c${i}`, { timeUpdated: 2000 + i })); + const gate = createFetchGate({ concurrency: 2 }); + const client = makeSummarizerClient(replyKeys(() => "s")); + failAllDeletes(client); + + const logs: string[] = []; + const summarizer = makeSummarizer(store, client.client, gate, { + batchSize: 1, + maxDeleteAttempts: 100, // keep ids retained so the backlog saturates + log: (message) => logs.push(message), + }); + await summarizer.runColdPass(); + + expect(logs.some((line) => line.includes("skipping batch"))).toBe(true); + // Only the pre-saturation batches created workers (3 = MAX_DRAIN_PER_BATCH). + expect(client.calls.creates.length).toBe(3); + await summarizer.stop(); }); it("disables tools and applies a deny-all permission on the worker prompt", async () => {