From 13ddda9c8a04b87a196c14a719d4c79f30c956ab Mon Sep 17 00:00:00 2001 From: Ada-1017 Date: Fri, 16 Oct 2026 01:50:51 -0400 Subject: [PATCH 1/3] fix(store): let every state registration fail for anything but "exists" Twelve of the seventeen places that register a state wrapped registerState in a catch that swallowed every failure as "already registered". Only a state that already exists is safe to swallow (a reopened store holds it). isStateExistsError said so, three sites used it and two more tested its message inline, but the twelve also swallowed a closed store or an I/O error. Each such failure then surfaced at a later read or write of that state, under that call's name. ensureStateRegistered, beside isStateExistsError, registers a state and rethrows every failure but "exists", and every registration in src now goes through it: - create(): the framework, deferred-write, coalescing, inference-log, turn-checkpoint, process-log and conversation-router states; - ModuleRegistry.addModule() and a module's registerLogState(); - CheckpointManager's checkpoint state and a feature set's host state; - ChannelRegistry's lifecycle and label-history logs (which tested the message inline) and the tune-out coordinator's state; - the workspace's tree and intent states; - the tool-result guard's audit log. A failure is now thrown where it happened. Chronicle's error codes (chronicle#31) aren't released, so the helper still matches the message; when they are, it changes in one place. Tests: each entry point fails with an injected I/O error for its state; the helper accepts an existing state and rethrows anything else; and a scan of src finds no registerState call outside the helper. --- .../state-registration-errors.fixed.md | 1 + src/framework.ts | 65 ++---- src/mcpl/channel-registry.ts | 25 +-- src/mcpl/checkpoint-manager.ts | 13 +- src/module-registry.ts | 45 ++-- src/modules/workspace/index.ts | 18 +- src/tool-result-guard.ts | 8 +- test/state-registration-errors.test.ts | 202 ++++++++++++++++++ 8 files changed, 262 insertions(+), 115 deletions(-) create mode 100644 changelog.d/state-registration-errors.fixed.md create mode 100644 test/state-registration-errors.test.ts diff --git a/changelog.d/state-registration-errors.fixed.md b/changelog.d/state-registration-errors.fixed.md new file mode 100644 index 000000000..b90b59811 --- /dev/null +++ b/changelog.d/state-registration-errors.fixed.md @@ -0,0 +1 @@ +- A state that can't be registered, for any reason but that it already exists, now fails the call that registers it (#385). Those calls are `AgentFramework.create()`, `addModule()`, the workspace's `initStore()`, and the MCPL checkpoint, host-state and tune-out coordinator states. Their registrations swallowed every failure, a closed store or an I/O error alike, so the failure surfaced at a later read or write of that state, under that call's name. A state that already exists, as it does when a store is reopened, is still registered without complaint. diff --git a/src/framework.ts b/src/framework.ts index 94f1035ff..8dff9bf9b 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -72,7 +72,7 @@ import { ProcessQueueImpl } from './queue.js'; import { DEFAULT_SHUTDOWN_TIMEOUT_MS, validateShutdownTimeout, shutdownErrorMessage, waitForShutdown, ShutdownTimeoutError } from './shutdown.js'; import { REFUSAL_REACTIONS, REFUSAL_REACTION_FALLBACK, REFUSAL_REACTION_BASELINE } from './refusal-reactions.js'; import { Agent } from './agent.js'; -import { ModuleRegistry, checkedToolResult, isStateExistsError, storableToolResult, thrownResult } from './module-registry.js'; +import { ModuleRegistry, checkedToolResult, ensureStateRegistered, storableToolResult, thrownResult } from './module-registry.js'; import { McplServerRegistry } from './mcpl/server-registry.js'; import { logValue } from './mcpl/log-value.js'; import { FeatureSetManager } from './mcpl/feature-set-manager.js'; @@ -2128,42 +2128,21 @@ export class AgentFramework { } // Register framework states - try { - store.registerState({ id: FRAMEWORK_STATE_ID, strategy: 'snapshot' }); - } catch { - // Already registered - } - try { - store.registerState({ id: DEFERRED_WRITES_ID, strategy: 'snapshot' }); - } catch { - // Already registered - } + ensureStateRegistered(store, { id: FRAMEWORK_STATE_ID, strategy: 'snapshot' }); + ensureStateRegistered(store, { id: DEFERRED_WRITES_ID, strategy: 'snapshot' }); for (const id of [COALESCING_STATE_ID, COALESCING_RECENT_ID]) { - try { - store.registerState({ id, strategy: 'snapshot' }); - } catch { - // Already registered - } - } - - try { - store.registerState({ - id: INFERENCE_LOG_ID, - strategy: 'append_log', - deltaSnapshotEvery: 100, - fullSnapshotEvery: 20, - }); - } catch { - // Already registered + ensureStateRegistered(store, { id, strategy: 'snapshot' }); } + ensureStateRegistered(store, { + id: INFERENCE_LOG_ID, + strategy: 'append_log', + deltaSnapshotEvery: 100, + fullSnapshotEvery: 20, + }); // The legacy single-map checkpoint state (TURN_CHECKPOINTS_ID) is no longer // registered for new stores — it's read-only fallback data in old ones. - try { - store.registerState({ id: TURN_CHECKPOINTS_TREE_ID, strategy: 'tree' }); - } catch (error) { - if (!isStateExistsError(error)) throw error; - } + ensureStateRegistered(store, { id: TURN_CHECKPOINTS_TREE_ID, strategy: 'tree' }); // Process logging config (default: disabled) const processLoggingPersist = config.processLogging?.persist ?? false; @@ -2171,16 +2150,12 @@ export class AgentFramework { // Register process log state only if persistence is enabled if (processLoggingPersist) { - try { - store.registerState({ - id: PROCESS_LOG_ID, - strategy: 'append_log', - deltaSnapshotEvery: 100, - fullSnapshotEvery: 20, - }); - } catch { - // Already registered - } + ensureStateRegistered(store, { + id: PROCESS_LOG_ID, + strategy: 'append_log', + deltaSnapshotEvery: 100, + fullSnapshotEvery: 20, + }); } const discordAwarenessOutboxPath = config.discordAwarenessOutboxPath @@ -2293,11 +2268,7 @@ export class AgentFramework { // Generation counters persist across restarts — reusing generation 1's // agent name after a restart would reopen (and re-seed) the previous // engagement's Chronicle namespace. - try { - store.registerState({ id: CONVERSATION_ROUTER_STATE_ID, strategy: 'snapshot' }); - } catch { - // Already registered - } + ensureStateRegistered(store, { id: CONVERSATION_ROUTER_STATE_ID, strategy: 'snapshot' }); try { const data = store.getStateJson(CONVERSATION_ROUTER_STATE_ID) as { generations?: Record } | null; diff --git a/src/mcpl/channel-registry.ts b/src/mcpl/channel-registry.ts index 082e6ed9b..d43aa732a 100644 --- a/src/mcpl/channel-registry.ts +++ b/src/mcpl/channel-registry.ts @@ -16,6 +16,7 @@ import type { ContentBlock } from '@animalabs/membrane'; import { INLINE_WITHHELD_TEXT, isInlineContradiction, referenceStubOrNull } from './references.js'; import type { JsStore } from '@animalabs/chronicle'; +import { ensureStateRegistered } from '../module-registry.js'; import type { ChannelDescriptor, @@ -1549,13 +1550,7 @@ export class ChannelRegistry { private initializeLifecycleStore(): void { if (!this.store) return; - try { - this.store.registerState({ id: CHANNEL_LIFECYCLE_LOG_ID, strategy: 'append_log' }); - } catch (error) { - if (!(error instanceof Error) || !error.message.includes('State already exists')) { - throw error; - } - } + ensureStateRegistered(this.store, { id: CHANNEL_LIFECYCLE_LOG_ID, strategy: 'append_log' }); const raw = this.store.getStateJson(CHANNEL_LIFECYCLE_LOG_ID); if (!Array.isArray(raw)) return; @@ -1616,13 +1611,7 @@ export class ChannelRegistry { private initializeLabelHistoryStore(): void { if (!this.store) return; - try { - this.store.registerState({ id: CHANNEL_LABEL_HISTORY_LOG_ID, strategy: 'append_log' }); - } catch (error) { - if (!(error instanceof Error) || !error.message.includes('State already exists')) { - throw error; - } - } + ensureStateRegistered(this.store, { id: CHANNEL_LABEL_HISTORY_LOG_ID, strategy: 'append_log' }); const raw = this.store.getStateJson(CHANNEL_LABEL_HISTORY_LOG_ID); if (!Array.isArray(raw)) return; @@ -2069,17 +2058,13 @@ export class ChannelRegistry { */ readCoordinatorState(stateId: string): unknown { if (!this.store) return null; - try { - this.store.registerState({ id: stateId, strategy: 'snapshot' }); - } catch { /* already registered */ } + ensureStateRegistered(this.store, { id: stateId, strategy: 'snapshot' }); return this.store.getStateJson(stateId); } writeCoordinatorState(stateId: string, value: unknown): void { if (!this.store) return; - try { - this.store.registerState({ id: stateId, strategy: 'snapshot' }); - } catch { /* already registered */ } + ensureStateRegistered(this.store, { id: stateId, strategy: 'snapshot' }); this.store.setStateJson(stateId, value); } diff --git a/src/mcpl/checkpoint-manager.ts b/src/mcpl/checkpoint-manager.ts index 4c7a26b42..be822dd95 100644 --- a/src/mcpl/checkpoint-manager.ts +++ b/src/mcpl/checkpoint-manager.ts @@ -13,6 +13,7 @@ */ import type { JsStore } from '@animalabs/chronicle'; +import { ensureStateRegistered } from '../module-registry.js'; import type { StateCheckpoint, JsonPatchOperation } from './types.js'; @@ -190,11 +191,7 @@ export class CheckpointManager { this.emitTraceFn = emitTraceFn; // Register the checkpoint metadata state slot - try { - store.registerState({ id: CHECKPOINTS_STATE_ID, strategy: 'snapshot' }); - } catch { - // Already registered (e.g. framework restart) - } + ensureStateRegistered(store, { id: CHECKPOINTS_STATE_ID, strategy: 'snapshot' }); // Load existing tree from Chronicle this.loadFromStore(); @@ -227,11 +224,7 @@ export class CheckpointManager { // Register per-feature-set state slot for host-managed state if (opts.hostState) { const stateId = this.stateId(serverId, featureSet); - try { - this.store.registerState({ id: stateId, strategy: 'snapshot' }); - } catch { - // Already registered - } + ensureStateRegistered(this.store, { id: stateId, strategy: 'snapshot' }); // Load existing state if any const existing = this.store.getStateJson(stateId); if (existing !== null && existing !== undefined) { diff --git a/src/module-registry.ts b/src/module-registry.ts index 6997da00b..71869e8b2 100644 --- a/src/module-registry.ts +++ b/src/module-registry.ts @@ -1,4 +1,4 @@ -import type { JsStore } from '@animalabs/chronicle'; +import type { JsStateRegistration, JsStore } from '@animalabs/chronicle'; import type { ContentBlock } from '@animalabs/membrane'; import type { MessageId, @@ -44,6 +44,22 @@ export function isStateExistsError(err: unknown): boolean { return err instanceof Error && err.message.includes('State already exists'); } +/** + * Register a state, treating "already exists" as done: a store reopened after + * a restart already holds it. Every other failure is thrown here, where it + * happened. Each state the framework registers goes through this. + */ +export function ensureStateRegistered( + store: Pick, + registration: JsStateRegistration, +): void { + try { + store.registerState(registration); + } catch (error) { + if (!isStateExistsError(error)) throw error; + } +} + /** * Hard ceiling for a single module's gatherContext budget, regardless of the * `contextTimeoutMs` it declares. gatherContext runs on the critical path of @@ -146,14 +162,7 @@ export class ModuleRegistry { // Register module state in store const stateId = this.getStateId(module.name); - try { - this.store.registerState({ - id: stateId, - strategy: 'snapshot', - }); - } catch { - // State already registered (from previous run) - } + ensureStateRegistered(this.store, { id: stateId, strategy: 'snapshot' }); // Check if this is a restart (state exists) const existingState = this.store.getStateJson(stateId); @@ -640,18 +649,12 @@ class ModuleContextImpl implements ModuleContext { name: string, opts?: { deltaSnapshotEvery?: number; fullSnapshotEvery?: number } ): void { - const id = this.logStateId(name); - try { - this.store.registerState({ - id, - strategy: 'append_log', - deltaSnapshotEvery: opts?.deltaSnapshotEvery ?? 100, - fullSnapshotEvery: opts?.fullSnapshotEvery ?? 20, - }); - } catch (err) { - if (!isStateExistsError(err)) throw err; - // Already registered (previous run) — idempotent path. - } + ensureStateRegistered(this.store, { + id: this.logStateId(name), + strategy: 'append_log', + deltaSnapshotEvery: opts?.deltaSnapshotEvery ?? 100, + fullSnapshotEvery: opts?.fullSnapshotEvery ?? 20, + }); this.registeredLogStates.add(name); } diff --git a/src/modules/workspace/index.ts b/src/modules/workspace/index.ts index 3be26091f..b74c1249c 100644 --- a/src/modules/workspace/index.ts +++ b/src/modules/workspace/index.ts @@ -40,7 +40,7 @@ import { hashContent, DEFAULT_MAX_FILE_SIZE } from './sync.js'; import { DiskAgreement, sameRootIdentity } from './disk-agreement.js'; import { BranchIntents } from './branch-intent.js'; import { coveredByIgnore, filesystemTrustsCtime, locateRoot } from './observe.js'; -import { TOOL_BATCH_HOOK_TIMEOUT_MS } from '../../module-registry.js'; +import { TOOL_BATCH_HOOK_TIMEOUT_MS, ensureStateRegistered } from '../../module-registry.js'; import { type MountRuntime, type PassOptions, @@ -706,16 +706,12 @@ export class WorkspaceModule implements Module { const treeStateId = `workspace/${mount.name}/tree`; const intentTreeStateId = `workspace/${mount.name}/intent`; for (const id of [treeStateId, intentTreeStateId]) { - try { - store.registerState({ - id, - strategy: 'tree', - deltaSnapshotEvery: this.config.deltaSnapshotEvery ?? 50, - fullSnapshotEvery: this.config.fullSnapshotEvery ?? 10, - }); - } catch { - // State already registered (restart scenario) - } + ensureStateRegistered(store, { + id, + strategy: 'tree', + deltaSnapshotEvery: this.config.deltaSnapshotEvery ?? 50, + fullSnapshotEvery: this.config.fullSnapshotEvery ?? 10, + }); } const mountState: MountState = { diff --git a/src/tool-result-guard.ts b/src/tool-result-guard.ts index 6a901835d..2a71f2ab3 100644 --- a/src/tool-result-guard.ts +++ b/src/tool-result-guard.ts @@ -3,7 +3,7 @@ import { isDeepStrictEqual } from 'node:util'; import type { ContextManager, MessageId, MessageMetadata } from '@animalabs/context-manager'; import type { ContentBlock, NormalizedMessage, ToolResult } from '@animalabs/membrane'; import type { CompletedToolCall } from './types/index.js'; -import { isStateExistsError } from './module-registry.js'; +import { ensureStateRegistered } from './module-registry.js'; import { toolResultDataToHistoryString } from './tool-result-history.js'; import { safeSlice } from './safe-slice.js'; @@ -295,11 +295,7 @@ export class ToolResultGuard { private write(record: Record): void { const store = this.cm.getStore(); if (!this.registered) { - try { - store.registerState({ id: TOOL_RESULT_GUARD_AUDIT_STATE, strategy: 'append_log' }); - } catch (error) { - if (!isStateExistsError(error)) throw error; - } + ensureStateRegistered(store, { id: TOOL_RESULT_GUARD_AUDIT_STATE, strategy: 'append_log' }); this.registered = true; } store.appendToStateJson(TOOL_RESULT_GUARD_AUDIT_STATE, record); diff --git a/test/state-registration-errors.test.ts b/test/state-registration-errors.test.ts new file mode 100644 index 000000000..bc4d2cf64 --- /dev/null +++ b/test/state-registration-errors.test.ts @@ -0,0 +1,202 @@ +/** + * Registering a state treats "already exists" as done, since a store reopened + * after a restart already holds the state. Every other failure, such as a + * closed store or an I/O error, reaches the caller of the call that + * registered it, rather than surfacing at a later write with another call's + * name in its error. + */ +import { describe, it } from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdtempSync, readdirSync, readFileSync, rmSync, statSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { JsStore } from '@animalabs/chronicle'; +import type { + FrameworkConfig, + EventResponse, + Module, + ModuleContext, + ProcessEvent, + ProcessState, + ToolCall, + ToolDefinition, + ToolResult, +} from '../src/index.js'; +import { AgentFramework } from '../src/index.js'; +import { ensureStateRegistered } from '../src/module-registry.js'; +import { CheckpointManager } from '../src/mcpl/checkpoint-manager.js'; +import { ChannelRegistry } from '../src/mcpl/channel-registry.js'; +import type { McplServerRegistry } from '../src/mcpl/server-registry.js'; +import type { FeatureSetManager } from '../src/mcpl/feature-set-manager.js'; +import { WorkspaceModule } from '../src/modules/workspace/index.js'; +import { MockMembrane } from './helpers/mock-membrane.js'; + +/** assert.throws and assert.rejects test a RegExp against the error's toString(). */ +function injected(id: string): RegExp { + return new RegExp(`^Error: injected EIO registering ${id.replace(/[/.]/g, '\\$&')}$`); +} + +/** A real store whose registerState fails for one id, as an I/O error would. */ +function failingOn(store: JsStore, failId: string): JsStore { + return new Proxy(store, { + get(target, prop) { + if (prop === 'registerState') { + return (registration: Parameters[0]) => { + if (registration.id === failId) { + throw Object.assign(new Error(`injected EIO registering ${registration.id}`), { code: 'EIO' }); + } + target.registerState(registration); + }; + } + const value = (target as unknown as Record)[prop]; + return typeof value === 'function' ? (value as (...a: unknown[]) => unknown).bind(target) : value; + }, + }); +} + +function withStore(prefix: string, body: (store: JsStore, dir: string) => Promise | void) { + return async () => { + const dir = mkdtempSync(join(tmpdir(), prefix)); + const store = JsStore.openOrCreate({ path: join(dir, 'store') }); + try { + await body(store, dir); + } finally { + store.close(); + rmSync(dir, { recursive: true, force: true }); + } + }; +} + +function probeModule(name: string, onStart?: (ctx: ModuleContext) => void): Module { + return { + name, + async start(ctx: ModuleContext) { onStart?.(ctx); }, + async stop() {}, + getTools(): ToolDefinition[] { return []; }, + async handleToolCall(_call: ToolCall): Promise { return { success: true }; }, + async onProcess(_event: ProcessEvent, _state: ProcessState): Promise { return {}; }, + }; +} + +describe('ensureStateRegistered', () => { + it('treats a state that already exists as registered', withStore('state-reg-exists-', (store) => { + ensureStateRegistered(store, { id: 'probe/state', strategy: 'snapshot' }); + ensureStateRegistered(store, { id: 'probe/state', strategy: 'snapshot' }); + store.setStateJson('probe/state', { ok: true }); + assert.deepEqual(store.getStateJson('probe/state'), { ok: true }); + })); + + it('throws every other failure', withStore('state-reg-other-', (store) => { + assert.throws( + () => ensureStateRegistered(failingOn(store, 'probe/state'), { id: 'probe/state', strategy: 'snapshot' }), + injected('probe/state'), + ); + })); +}); + +describe('AgentFramework.create() lets a failed registration through', () => { + const trunk = { name: 'trunk', model: 'test-model', systemPrompt: 'You are the trunk.' }; + const cases: Array<{ id: string; config?: Partial }> = [ + { id: 'framework/state' }, + { id: 'framework/deferred-writes' }, + { id: 'mcpl/coalescing' }, + { id: 'mcpl/coalescing-recent' }, + { id: 'framework/inference-log' }, + { id: 'framework/turn-checkpoints/tree' }, + { id: 'framework/process-log', config: { processLogging: { persist: true } } }, + { id: 'framework/conversation-router', config: { agents: [trunk], conversations: { templateAgent: 'trunk' } } }, + { id: 'modules/probe/state', config: { modules: [probeModule('probe')] } }, + { id: 'modules/probe/events', config: { modules: [probeModule('probe', (ctx) => ctx.registerLogState('events'))] } }, + ]; + for (const { id, config } of cases) { + it(`fails when ${id} can't be registered`, withStore('state-reg-create-', async (store) => { + let framework: AgentFramework | undefined; + try { + await assert.rejects( + async () => { + framework = await AgentFramework.create({ + store: failingOn(store, id), + membrane: new MockMembrane().asMembrane(), + agents: [], + modules: [], + syncIntervalMs: 0, + ...config, + }); + }, + injected(id), + ); + } finally { + await framework?.stop(); + } + })); + } +}); + +describe('MCPL state lets a failed registration through', () => { + it('a checkpoint manager', withStore('state-reg-checkpoints-', (store) => { + assert.throws(() => new CheckpointManager(failingOn(store, 'mcpl/checkpoints'), () => {}), injected('mcpl/checkpoints')); + })); + + it("a feature set's host state", withStore('state-reg-hoststate-', (store) => { + const manager = new CheckpointManager(failingOn(store, 'mcpl/state/srv/fs'), () => {}); + assert.throws( + () => manager.registerFeatureSet('srv', 'fs', { hostState: true, rollback: false }), + injected('mcpl/state/srv/fs'), + ); + })); + + const channelRegistry = (store: JsStore) => new ChannelRegistry( + { getServer: (_id: string) => null } as unknown as McplServerRegistry, + {} as FeatureSetManager, + () => {}, + () => {}, + { store }, + ); + + for (const id of ['mcpl/channel-lifecycle', 'mcpl/channel-label-history']) { + it(`a channel registry's ${id}`, withStore('state-reg-channels-', (store) => { + assert.throws(() => channelRegistry(failingOn(store, id)), injected(id)); + })); + } + + it("the tune-out coordinator's state, read and written", withStore('state-reg-coordinator-', (store) => { + const registry = channelRegistry(failingOn(store, 'tune-out/probe')); + assert.throws(() => registry.readCoordinatorState('tune-out/probe'), injected('tune-out/probe')); + assert.throws(() => registry.writeCoordinatorState('tune-out/probe', { a: 'b' }), injected('tune-out/probe')); + })); +}); + +describe("the workspace's mount state lets a failed registration through", () => { + for (const id of ['workspace/files/tree', 'workspace/files/intent']) { + it(id, withStore('state-reg-workspace-', (store, dir) => { + const workspace = new WorkspaceModule({ + mounts: [{ name: 'files', path: join(dir, 'files'), mode: 'read-write', watch: 'never' }], + }); + assert.throws(() => workspace.initStore(failingOn(store, id)), injected(id)); + })); + } +}); + +it('every state the framework registers goes through ensureStateRegistered', () => { + // A registration written as its own try/catch is how the catch-alls this + // file tests came about, beside a helper that already did it right. The one + // registerState call in src is the helper's own. + const src = fileURLToPath(new URL('../../src/', import.meta.url)); + const calls: string[] = []; + const walk = (dir: string): void => { + for (const name of readdirSync(dir)) { + const full = join(dir, name); + if (statSync(full).isDirectory()) { walk(full); continue; } + if (!name.endsWith('.ts')) continue; + readFileSync(full, 'utf8').split('\n').forEach((line, i) => { + const code = line.trim(); + if (code.startsWith('*') || code.startsWith('//') || code.startsWith('/*')) return; + if (/\.registerState\(/.test(code)) calls.push(`${full.slice(src.length)}:${i + 1}`); + }); + } + }; + walk(src); + assert.equal(calls.length, 1, `registerState calls outside ensureStateRegistered: ${calls.join(', ')}`); + assert.match(calls[0], /^module-registry\.ts:/); +}); From 1c6e2058708165aa9348eb13192628cb9e6a8168 Mon Sep 17 00:00:00 2001 From: Ada-1017 Date: Fri, 16 Oct 2026 21:43:02 -0400 Subject: [PATCH 2/3] fix(store): a failed registration names its state, and two of its calls leave nothing behind From our internal review of the first commit. The helper's rethrow now names the state, "Registering state '' failed: ", with the store's error kept as its cause. Before, create(), addModule() and initStore() rejected with the store's bare message, which named no state. CheckpointManager.registerFeatureSet registers a feature set's host-state slot, and reads its stored value, before it records the feature set. It recorded it first, so a registration that failed left the record behind, and every later call, a reconnect's re-run included, returned at the already-registered check without registering the slot or loading the stored state. The workspace's initStore registers every mount's states before it takes the store or any mount. It took them one mount at a time, so a failure at a later mount left the store and the earlier mounts set, with no agreement, saved state or watchers. registerLogState's doc says its cadence applies when the state is first registered, and that a failed registration leaves the name unregistered. Tests: the helper names the state and keeps the cause; a real closed store's failure is thrown, not taken for "already exists"; the store's own answer to a second registration is the one the helper takes as done; a feature set registered after a failed call, with its stored state; an initStore that fails takes no mount, and the next takes them all; a module's log state and the tool-result guard's audit state, each registered on first use, register on the next use after a failure. The fragment says which two calls leave nothing behind, that the tune-out coordinator's state fails where it's written, and that a failure surfaced later only if a later call came. Co-Authored-By: Claude Opus 5.5 --- .../state-registration-errors.fixed.md | 2 +- src/mcpl/checkpoint-manager.ts | 25 ++- src/module-registry.ts | 7 +- src/modules/workspace/index.ts | 17 +- src/types/module.ts | 7 +- test/state-registration-errors.test.ts | 145 ++++++++++++++++-- 6 files changed, 171 insertions(+), 32 deletions(-) diff --git a/changelog.d/state-registration-errors.fixed.md b/changelog.d/state-registration-errors.fixed.md index b90b59811..c27577d32 100644 --- a/changelog.d/state-registration-errors.fixed.md +++ b/changelog.d/state-registration-errors.fixed.md @@ -1 +1 @@ -- A state that can't be registered, for any reason but that it already exists, now fails the call that registers it (#385). Those calls are `AgentFramework.create()`, `addModule()`, the workspace's `initStore()`, and the MCPL checkpoint, host-state and tune-out coordinator states. Their registrations swallowed every failure, a closed store or an I/O error alike, so the failure surfaced at a later read or write of that state, under that call's name. A state that already exists, as it does when a store is reopened, is still registered without complaint. +- A state that can't be registered, for any reason but that it already exists, now fails the call that registers it, with an error that names the state (#385). Those calls are `AgentFramework.create()`, `addModule()`, the workspace's `initStore()`, the MCPL checkpoint and host states, and the tune-out coordinator's state when it's written. Most of the registrations behind them swallowed every failure, a closed store or an I/O error alike, so the failure surfaced at a later read or write of that state, if one came, under that call's name. Two of them leave nothing behind when they fail this way: `initStore()` takes no mount until a call succeeds, and a feature set whose host state couldn't be registered is registered by the next call for it, such as a reconnect's. A registration the store answers with "already exists", as on a reopened store, is still taken as done. diff --git a/src/mcpl/checkpoint-manager.ts b/src/mcpl/checkpoint-manager.ts index be822dd95..047965dd8 100644 --- a/src/mcpl/checkpoint-manager.ts +++ b/src/mcpl/checkpoint-manager.ts @@ -213,26 +213,25 @@ export class CheckpointManager { const key = this.key(serverId, featureSet); if (this.trees.has(key)) return; // Already registered (e.g. from loaded state) + // The host-managed state's slot is registered, and its stored value read, + // before the feature set is recorded: a registration that fails leaves + // nothing behind, so the next call (a reconnect's re-run) tries again. + let currentState: unknown = undefined; + if (opts.hostState) { + const stateId = this.stateId(serverId, featureSet); + ensureStateRegistered(this.store, { id: stateId, strategy: 'snapshot' }); + const existing = this.store.getStateJson(stateId); + if (existing !== null && existing !== undefined) currentState = existing; + } + this.trees.set(key, { hostState: opts.hostState, rollback: opts.rollback, currentCheckpoint: null, - currentState: undefined, + currentState, nodes: new Map(), }); - // Register per-feature-set state slot for host-managed state - if (opts.hostState) { - const stateId = this.stateId(serverId, featureSet); - ensureStateRegistered(this.store, { id: stateId, strategy: 'snapshot' }); - // Load existing state if any - const existing = this.store.getStateJson(stateId); - if (existing !== null && existing !== undefined) { - const tree = this.trees.get(key)!; - tree.currentState = existing; - } - } - this.emitTraceFn({ type: 'mcpl:state_registered', serverId, diff --git a/src/module-registry.ts b/src/module-registry.ts index 71869e8b2..fb89a5517 100644 --- a/src/module-registry.ts +++ b/src/module-registry.ts @@ -47,7 +47,8 @@ export function isStateExistsError(err: unknown): boolean { /** * Register a state, treating "already exists" as done: a store reopened after * a restart already holds it. Every other failure is thrown here, where it - * happened. Each state the framework registers goes through this. + * happened, naming the state, with the store's own error as its cause. Each + * state the framework registers goes through this. */ export function ensureStateRegistered( store: Pick, @@ -56,7 +57,9 @@ export function ensureStateRegistered( try { store.registerState(registration); } catch (error) { - if (!isStateExistsError(error)) throw error; + if (isStateExistsError(error)) return; + const reason = error instanceof Error ? error.message : String(error); + throw new Error(`Registering state '${registration.id}' failed: ${reason}`, { cause: error }); } } diff --git a/src/modules/workspace/index.ts b/src/modules/workspace/index.ts index b74c1249c..563649fdf 100644 --- a/src/modules/workspace/index.ts +++ b/src/modules/workspace/index.ts @@ -698,7 +698,11 @@ export class WorkspaceModule implements Module { * second. `ensureRunning()` gates on `ctx && mounts-populated`. */ initStore(store: JsStore): void { - this.store = store; + // Every mount's states are registered before the module takes the store + // or any mount, so a registration that fails leaves the module as it was + // before the call, knowing no mount, and a later initStore starts clean. + const mounts = new Map(); + const intents = new Map(); // Register tree states for each mount: the workspace tree, and the // branch-local intent that qualifies it. @@ -714,7 +718,7 @@ export class WorkspaceModule implements Module { }); } - const mountState: MountState = { + mounts.set(mount.name, { config: mount, treeStateId, intentTreeStateId, @@ -724,11 +728,14 @@ export class WorkspaceModule implements Module { lastMaterializedBranchId: null, watcherReadyAt: null, watcherError: null, - }; - this.mounts.set(mount.name, mountState); - this.intents.set(mount.name, new BranchIntents(store, intentTreeStateId)); + }); + intents.set(mount.name, new BranchIntents(store, intentTreeStateId)); } + this.store = store; + for (const [name, state] of mounts) this.mounts.set(name, state); + for (const [name, intent] of intents) this.intents.set(name, intent); + // What disk last agreed with, per mount and path: global records, so a // branch switch never rewinds it. this.agreement = new DiskAgreement(store); diff --git a/src/types/module.ts b/src/types/module.ts index a6fda9ed2..e79b714a0 100644 --- a/src/types/module.ts +++ b/src/types/module.ts @@ -184,8 +184,11 @@ export interface ModuleContext { * `modules//`. Idempotent — safe to call on every start(). * Items are appended with `appendToLog` (one O(item) record per append) and * point-edited with `editLogItem`; Chronicle's append_log snapshot cadence - * handles consolidation. `name` must not be 'state' (reserved for the main - * snapshot state) and must not contain '/'. + * handles consolidation. The cadence applies when the state is first + * registered: a store that already holds the state keeps the cadence it was + * registered with. `name` must not be 'state' (reserved for the main + * snapshot state) and must not contain '/'. A registration that fails + * throws, naming the state, and leaves the name unregistered. */ registerLogState( name: string, diff --git a/test/state-registration-errors.test.ts b/test/state-registration-errors.test.ts index bc4d2cf64..44253b70f 100644 --- a/test/state-registration-errors.test.ts +++ b/test/state-registration-errors.test.ts @@ -12,6 +12,7 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { fileURLToPath } from 'node:url'; import { JsStore } from '@animalabs/chronicle'; +import { ContextManager } from '@animalabs/context-manager'; import type { FrameworkConfig, EventResponse, @@ -24,7 +25,8 @@ import type { ToolResult, } from '../src/index.js'; import { AgentFramework } from '../src/index.js'; -import { ensureStateRegistered } from '../src/module-registry.js'; +import { ensureStateRegistered, isStateExistsError } from '../src/module-registry.js'; +import { ToolResultGuard, TOOL_RESULT_GUARD_AUDIT_STATE } from '../src/tool-result-guard.js'; import { CheckpointManager } from '../src/mcpl/checkpoint-manager.js'; import { ChannelRegistry } from '../src/mcpl/channel-registry.js'; import type { McplServerRegistry } from '../src/mcpl/server-registry.js'; @@ -32,18 +34,29 @@ import type { FeatureSetManager } from '../src/mcpl/feature-set-manager.js'; import { WorkspaceModule } from '../src/modules/workspace/index.js'; import { MockMembrane } from './helpers/mock-membrane.js'; -/** assert.throws and assert.rejects test a RegExp against the error's toString(). */ +/** assert.throws and assert.rejects test a RegExp against the error's + * toString(): the failure names the state, then gives the store's error. */ function injected(id: string): RegExp { - return new RegExp(`^Error: injected EIO registering ${id.replace(/[/.]/g, '\\$&')}$`); + const escaped = id.replace(/[/.]/g, '\\$&'); + return new RegExp(`^Error: Registering state '${escaped}' failed: injected EIO registering ${escaped}$`); } -/** A real store whose registerState fails for one id, as an I/O error would. */ -function failingOn(store: JsStore, failId: string): JsStore { +type FailingStore = JsStore & { readonly registrations: string[] }; + +/** A real store whose registerState fails for one id, as an I/O error would: + * every time, or only the first `times` times. `registrations` lists each + * id registerState was called with, in order. */ +function failingOn(store: JsStore, failId: string, times = Infinity): FailingStore { + const registrations: string[] = []; + let failures = 0; return new Proxy(store, { get(target, prop) { + if (prop === 'registrations') return registrations; if (prop === 'registerState') { return (registration: Parameters[0]) => { - if (registration.id === failId) { + registrations.push(registration.id); + if (registration.id === failId && failures < times) { + failures++; throw Object.assign(new Error(`injected EIO registering ${registration.id}`), { code: 'EIO' }); } target.registerState(registration); @@ -52,7 +65,7 @@ function failingOn(store: JsStore, failId: string): JsStore { const value = (target as unknown as Record)[prop]; return typeof value === 'function' ? (value as (...a: unknown[]) => unknown).bind(target) : value; }, - }); + }) as FailingStore; } function withStore(prefix: string, body: (store: JsStore, dir: string) => Promise | void) { @@ -82,17 +95,41 @@ function probeModule(name: string, onStart?: (ctx: ModuleContext) => void): Modu describe('ensureStateRegistered', () => { it('treats a state that already exists as registered', withStore('state-reg-exists-', (store) => { ensureStateRegistered(store, { id: 'probe/state', strategy: 'snapshot' }); + // The store's own answer to a second registration is the one the helper + // takes as done. + assert.throws( + () => store.registerState({ id: 'probe/state', strategy: 'snapshot' }), + (error: unknown) => isStateExistsError(error), + ); ensureStateRegistered(store, { id: 'probe/state', strategy: 'snapshot' }); store.setStateJson('probe/state', { ok: true }); assert.deepEqual(store.getStateJson('probe/state'), { ok: true }); })); - it('throws every other failure', withStore('state-reg-other-', (store) => { + it("throws every other failure, naming the state, with the store's error as its cause", withStore('state-reg-other-', (store) => { assert.throws( () => ensureStateRegistered(failingOn(store, 'probe/state'), { id: 'probe/state', strategy: 'snapshot' }), - injected('probe/state'), + (error: unknown) => injected('probe/state').test(String(error)) + && (error as Error).cause instanceof Error + && ((error as Error).cause as { code?: string }).code === 'EIO', ); })); + + it("throws a closed store's failure, rather than taking it for 'already exists'", () => { + const dir = mkdtempSync(join(tmpdir(), 'state-reg-closed-')); + try { + const store = JsStore.openOrCreate({ path: join(dir, 'store') }); + store.close(); + assert.throws( + () => ensureStateRegistered(store, { id: 'probe/closed', strategy: 'snapshot' }), + (error: unknown) => error instanceof Error + && error.message.startsWith("Registering state 'probe/closed' failed: ") + && !isStateExistsError(error.cause), + ); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); }); describe('AgentFramework.create() lets a failed registration through', () => { @@ -146,6 +183,23 @@ describe('MCPL state lets a failed registration through', () => { ); })); + it("a feature set whose host state failed to register is registered by the next call, with its stored state", withStore('state-reg-hoststate-retry-', (store) => { + // A host state stored before, as a reconnect's re-run finds it. + store.registerState({ id: 'mcpl/state/srv/fs', strategy: 'snapshot' }); + store.setStateJson('mcpl/state/srv/fs', { cursor: 7 }); + const flaky = failingOn(store, 'mcpl/state/srv/fs', 1); + const manager = new CheckpointManager(flaky, () => {}); + assert.throws( + () => manager.registerFeatureSet('srv', 'fs', { hostState: true, rollback: false }), + injected('mcpl/state/srv/fs'), + ); + assert.equal(manager.isHostManaged('srv', 'fs'), false, 'a failed call records nothing'); + manager.registerFeatureSet('srv', 'fs', { hostState: true, rollback: false }); + assert.equal(manager.isHostManaged('srv', 'fs'), true); + assert.deepEqual(manager.getCurrentState('srv', 'fs'), { cursor: 7 }); + assert.equal(flaky.registrations.filter((id) => id === 'mcpl/state/srv/fs').length, 2); + })); + const channelRegistry = (store: JsStore) => new ChannelRegistry( { getServer: (_id: string) => null } as unknown as McplServerRegistry, {} as FeatureSetManager, @@ -176,6 +230,79 @@ describe("the workspace's mount state lets a failed registration through", () => assert.throws(() => workspace.initStore(failingOn(store, id)), injected(id)); })); } + + it('a failed initStore leaves the module knowing no mount, and the next initStore takes them all', withStore('state-reg-workspace-whole-', (store, dir) => { + const workspace = new WorkspaceModule({ + mounts: [ + { name: 'files', path: join(dir, 'files'), mode: 'read-write', watch: 'never' }, + { name: 'more', path: join(dir, 'more'), mode: 'read-write', watch: 'never' }, + ], + }); + assert.throws(() => workspace.initStore(failingOn(store, 'workspace/more/tree')), injected('workspace/more/tree')); + assert.equal(workspace.resolveAbsolutePath('files/a.txt'), null, 'the first mount was not taken either'); + workspace.initStore(store); + assert.equal(workspace.resolveAbsolutePath('files/a.txt'), join(dir, 'files', 'a.txt')); + assert.equal(workspace.resolveAbsolutePath('more/b.txt'), join(dir, 'more', 'b.txt')); + })); +}); + +describe('a state registered on first use lets a failed registration through, and registers on the next use', () => { + it("a module's log state", withStore('state-reg-logstate-', async (store) => { + let first: unknown; + let beforeRegistered: unknown; + let framework: AgentFramework | undefined; + try { + framework = await AgentFramework.create({ + store: failingOn(store, 'modules/probe/events', 1), + membrane: new MockMembrane().asMembrane(), + agents: [], + syncIntervalMs: 0, + modules: [probeModule('probe', (ctx) => { + try { ctx.registerLogState('events'); } catch (error) { first = error; } + try { ctx.appendToLog('events', 1); } catch (error) { beforeRegistered = error; } + ctx.registerLogState('events'); + ctx.appendToLog('events', 2); + })], + }); + assert.match(String(first), injected('modules/probe/events')); + assert.match(String(beforeRegistered), /Module log state 'events' is not registered/); + assert.deepEqual(store.getStateJson('modules/probe/events'), [2]); + } finally { + await framework?.stop(); + } + })); + + it("the tool-result guard's audit state", withStore('state-reg-guard-', async (store) => { + const cm = await ContextManager.open({ store, namespace: 'agents/probe' }); + const flaky = failingOn(store, TOOL_RESULT_GUARD_AUDIT_STATE, 1); + (cm as unknown as { getStore(): JsStore }).getStore = () => flaky; + cm.addMessage('probe', [{ type: 'tool_use', id: 'tu-1', name: 'search', input: {} }]); + const guard = new ToolResultGuard('probe', cm, true); + const logged: unknown[][] = []; + const realError = console.error; + console.error = (...args: unknown[]) => { logged.push(args); }; + try { + guard.storeResults( + [{ type: 'tool_result', toolUseId: 'tu-1', content: 'out' }], + [{ toolUseId: 'tu-1', content: 'out' }], + [], + undefined, + undefined, + ); + } finally { + console.error = realError; + } + // The staged record's write failed with the registration, naming the + // state; the record stayed queued, and the next write registered the + // state again before writing both records. + assert.ok( + logged.some((args) => args.some((arg) => injected(TOOL_RESULT_GUARD_AUDIT_STATE).test(String(arg)))), + 'the failed registration is logged, naming the state', + ); + assert.equal(flaky.registrations.filter((id) => id === TOOL_RESULT_GUARD_AUDIT_STATE).length, 2); + const audit = store.getStateJson(TOOL_RESULT_GUARD_AUDIT_STATE) as Array<{ type: string }>; + assert.deepEqual(audit.map((record) => record.type), ['staged', 'linked']); + })); }); it('every state the framework registers goes through ensureStateRegistered', () => { From 5a8192983eadcc476001e6dfd0cceac7d859af72 Mon Sep 17 00:00:00 2001 From: Ada-1017 Date: Sun, 18 Oct 2026 00:20:30 -0400 Subject: [PATCH 3/3] fix(store): a feature set's host state that can't be registered says what that stops loadHostState's refusal says what a failed read of a host-managed state stops: before a connect, that create() and connectMcplServer() don't go on; at registration, that the server's connect fails. Its registration ran ahead of that, so a failed registration threw only the helper's "Registering state '...' failed: ...", naming the state without saying what it stopped. It now runs inside the same try, and the refusal reads "could not be registered or read", with the helper's error as its cause and in its text. The two tests that pin the refusal follow: the host-state registration tests now check the refusal and its cause, and the read refusal's expected text says "registered or read". Co-Authored-By: Claude Opus 5.5 --- src/mcpl/checkpoint-manager.ts | 15 ++++++++------- test/boot-state-read-errors.test.ts | 2 +- test/state-registration-errors.test.ts | 15 +++++++++++++-- 3 files changed, 22 insertions(+), 10 deletions(-) diff --git a/src/mcpl/checkpoint-manager.ts b/src/mcpl/checkpoint-manager.ts index 713177f02..63974f92c 100644 --- a/src/mcpl/checkpoint-manager.ts +++ b/src/mcpl/checkpoint-manager.ts @@ -557,11 +557,12 @@ export class CheckpointManager { } /** A host-managed feature set's stored state, read into its tree; nothing - * stored leaves it unset. A read that fails throws, naming the state: - * taken as nothing stored, the next patch would apply to nothing and - * store over it. `when` says what the refusal stops: a restored tree's - * read comes before its server connects, and a feature set with no tree - * is read when it registers, once its server has connected. */ + * stored leaves it unset. A registration or read that fails throws, + * naming the state: taken as nothing stored, the next patch would apply + * to nothing and store over it. `when` says what the refusal stops: a + * restored tree's read comes before its server connects, and a feature + * set with no tree is read when it registers, once its server has + * connected. */ private loadHostState( serverId: string, featureSet: string, @@ -569,14 +570,14 @@ export class CheckpointManager { when: 'before-connect' | 'registration', ): void { const stateId = this.stateId(serverId, featureSet); - ensureStateRegistered(this.store, { id: stateId, strategy: 'snapshot' }); let existing: unknown; try { + ensureStateRegistered(this.store, { id: stateId, strategy: 'snapshot' }); existing = this.store.getStateJson(stateId); } catch (error) { throw new Error( `the host-managed state of feature set "${featureSet}" on MCPL server "${serverId}" (${stateId}) could not ` + - `be read: ${thrownText(error)}. ` + + `be registered or read: ${thrownText(error)}. ` + (when === 'before-connect' ? `Neither create() with the server configured nor a connectMcplServer() of it goes on without it, since ` + `the feature set's next checkpoint would store over it. It's unchanged, and they go on once it can be read.` diff --git a/test/boot-state-read-errors.test.ts b/test/boot-state-read-errors.test.ts index c532cdd23..deff1b50d 100644 --- a/test/boot-state-read-errors.test.ts +++ b/test/boot-state-read-errors.test.ts @@ -611,7 +611,7 @@ function seedHostManaged(storePath: string, server: string, featureSet: string, /** The refusal of a host-managed state read before its server connects, or, with `registration`, when its feature set registers. */ const hostStateUnreadable = (server: string, featureSet: string, when: 'before-connect' | 'registration' = 'before-connect'): RegExp => new RegExp( `^the host-managed state of feature set "${featureSet}" on MCPL server "${server}" \\(mcpl/state/${server}/${featureSet}\\) could not ` + - `be read: injected EIO reading mcpl/state/${server}/${featureSet}\\. ` + + `be registered or read: injected EIO reading mcpl/state/${server}/${featureSet}\\. ` + (when === 'before-connect' ? `Neither create\\(\\) with the server configured nor a connectMcplServer\\(\\) of it goes on without it, since the feature set's ` + `next checkpoint would store over it\\. It's unchanged, and they go on once it can be read\\.$` diff --git a/test/state-registration-errors.test.ts b/test/state-registration-errors.test.ts index 44253b70f..821855e6f 100644 --- a/test/state-registration-errors.test.ts +++ b/test/state-registration-errors.test.ts @@ -41,6 +41,17 @@ function injected(id: string): RegExp { return new RegExp(`^Error: Registering state '${escaped}' failed: injected EIO registering ${escaped}$`); } +/** loadHostState's refusal for a feature set's host state at registration: + * it says what the failure stops, and its cause is the helper's error. */ +function hostStateRefused(error: unknown): boolean { + assert.match( + String(error), + /^Error: the host-managed state of feature set "fs" on MCPL server "srv" \(mcpl\/state\/srv\/fs\) could not be registered or read: Registering state 'mcpl\/state\/srv\/fs' failed: injected EIO registering mcpl\/state\/srv\/fs\. The feature set isn't registered without it, /, + ); + assert.match(String((error as Error).cause), injected('mcpl/state/srv/fs')); + return true; +} + type FailingStore = JsStore & { readonly registrations: string[] }; /** A real store whose registerState fails for one id, as an I/O error would: @@ -179,7 +190,7 @@ describe('MCPL state lets a failed registration through', () => { const manager = new CheckpointManager(failingOn(store, 'mcpl/state/srv/fs'), () => {}); assert.throws( () => manager.registerFeatureSet('srv', 'fs', { hostState: true, rollback: false }), - injected('mcpl/state/srv/fs'), + hostStateRefused, ); })); @@ -191,7 +202,7 @@ describe('MCPL state lets a failed registration through', () => { const manager = new CheckpointManager(flaky, () => {}); assert.throws( () => manager.registerFeatureSet('srv', 'fs', { hostState: true, rollback: false }), - injected('mcpl/state/srv/fs'), + hostStateRefused, ); assert.equal(manager.isHostManaged('srv', 'fs'), false, 'a failed call records nothing'); manager.registerFeatureSet('srv', 'fs', { hostState: true, rollback: false });