diff --git a/changelog.d/389-failed-create-releases.fixed.md b/changelog.d/389-failed-create-releases.fixed.md new file mode 100644 index 000000000..0d63464b8 --- /dev/null +++ b/changelog.d/389-failed-create-releases.fixed.md @@ -0,0 +1,4 @@ +- A failed `AgentFramework.create()` now releases what it had acquired before its error reaches the caller, so the same process can call `create()` again (#389). Before, a `create()` that failed after opening its store, such as on an invalid `timeZone`, an MCPL `toolPrefix` collision, a module whose `start()` threw or an out-of-range `toolResultInlineMaxChars`, left the store locked: every later `create()` on that `storePath` got `Store is locked by another process` until the process exited. It also left running what it had started, the liveness watchdog among it, which could then kill a process that carried on after the failure. Now the framework it had begun is stopped as `stop()` stops one (its modules, MCPL servers, event gate, tune-out coordinator and watchdog), and a store it opened from `storePath` is closed. A store passed in as `store` stays open. + - The caller of a failed `create()` has no framework to give `stop()` a deadline, so `create()` gives it one: it gives up after 30 s on each of `stop()`'s two cleanup waits, for an operator's operations already running and then for its modules and MCPL servers to stop, where `stop()`'s default waits with no deadline. A module whose `stop()` never settles would otherwise turn a failed boot that exits into one that hangs. + - When stopping them fails or runs out of time, or closing the store fails, `create()` rejects with an `AggregateError` holding its own error first and then those failures. Its message begins `AgentFramework.create() failed:` and says what didn't finish: that part of what it started may still be running, that stopping failed only after everything it started had stopped, or that its store may still be locked. It closes a store it opened even when stopping failed. When the release finishes cleanly, `create()`'s own error is rethrown unchanged, as before. + - A `WorkspaceModule` that stops before its `initStore()` has applied the state it was handed, as one does when a `create()` that started it fails, or after an `initStore()` that threw partway, no longer saves over its stored state. Its `stop()` saved empty or default mounts there, losing the branch last materialized to disk, so on the next start `materialize` no longer refused a branch that had diverged from it. diff --git a/src/framework.ts b/src/framework.ts index 81b0c2d40..e397a16ee 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -1027,6 +1027,15 @@ const WITHHELD_SPILL_DEADLINE_MS = 30_000; const DEFAULT_MAINTENANCE_INTERVAL_MS = 5000; /** Bound one pass so a large backlog yields to inference and other agents. */ const MAINTENANCE_TICKS_PER_PASS = 8; +/** The teardownTimeoutMs a failed create() gives stop(): how long each of its + * cleanup waits, on operator operations and then on modules and MCPL servers + * together, may take. stop() leaves its deadline to its caller, who owns the + * framework, but the caller of a failed create() never got one: unbounded, a + * module whose stop() never settles would turn a failed boot that exits into + * one that hangs. stop()'s other waits are on streams, the event loop, + * maintenance and guard annotations, which a framework create() never + * started has none of. */ +const FAILED_CREATE_TEARDOWN_TIMEOUT_MS = 30_000; /** Local per-residence provider admission (AF #114 bounded first slice). */ interface LocalProviderGate { @@ -1602,6 +1611,11 @@ export class AgentFramework { private stopPromise: Promise | null = null; private stopCompleted = false; private ownedStoreClosed = false; + /** Whether the latest stop() stopped in time everything it waits on with + * its deadline (an operator's operations, modules and MCPL servers), + * whatever failed after that. create()'s release reads it to say whether + * part of what a failed create() started may still be running. */ + private stopFinishedTeardown = false; /** An operator's operations in flight, and the config materializations they * start. stop() waits until this set is empty before it tears down, unless * its wait budget runs out. */ @@ -2107,7 +2121,13 @@ export class AgentFramework { } /** - * Create and start the framework. + * Create and start the framework. A create() that fails releases what it + * had acquired before its error reaches the caller, so that the caller can + * try again in the same process: the framework it had begun is stopped, as + * stop() stops one, but giving up after 30 s on each of stop()'s two cleanup + * waits, for an operator's operations already running and then for its + * modules and MCPL servers, and a store it opened is closed. A store the + * caller passed in is left open. */ static async create(config: FrameworkConfig): Promise { // Before anything opens or starts: a refused config must leave nothing behind. @@ -2127,6 +2147,70 @@ export class AgentFramework { throw new Error('Either storePath or store must be provided'); } + const building: { framework?: AgentFramework } = {}; + try { + return await AgentFramework.createOn(store, ownsStore, config, building); + } catch (error) { + throw await AgentFramework.releaseFailedCreate(error, building.framework, store, ownsStore); + } + } + + /** + * The error a failed create() rejects with, once what it acquired is + * released: the framework it had begun is stopped, with + * FAILED_CREATE_TEARDOWN_TIMEOUT_MS as stop()'s teardownTimeoutMs, and a + * store create() opened is closed even when that stop failed or timed out. + * stop() keeps an owned store open when its waits on operator operations, + * modules or MCPL servers fail or time out, so that it can be called again, + * but no one can call it on a framework that create() never returned. When + * the release doesn't finish cleanly, the rejection carries create()'s error + * and then the release's, and its message says what didn't finish. + */ + private static async releaseFailedCreate( + error: unknown, + framework: AgentFramework | undefined, + store: JsStore, + ownsStore: boolean, + ): Promise { + const failures: unknown[] = []; + const unfinished: string[] = []; + if (framework) { + try { + await framework.stop(FAILED_CREATE_TEARDOWN_TIMEOUT_MS); + } catch (stopError) { + failures.push(stopError); + // A stop can fail after everything it waits on has stopped, as stop() + // counts it (an MCPL server once its transport has sent the kill): + // finishing a guard's storage work, in its final sync, or closing the + // store. The message then doesn't say part of it may still be running. + unfinished.push(framework.stopFinishedTeardown + ? 'What it had started stopped, but finishing the stop failed: ' + shutdownErrorMessage(stopError) + : 'Stopping what it had started didn\'t finish, so part of it may still be running: ' + + shutdownErrorMessage(stopError)); + } + } + if (ownsStore && !store.isClosed()) { + try { + store.close(); + } catch (closeError) { + failures.push(closeError); + unfinished.push('Closing its store failed, so the store may still be locked: ' + shutdownErrorMessage(closeError)); + } + } + if (failures.length === 0) return error; + return new AggregateError([error, ...failures], + `AgentFramework.create() failed: ${shutdownErrorMessage(error)}. ${unfinished.join('. ')}`); + } + + /** create()'s work once it holds its store. The framework is named in + * `building` as soon as it exists, so that create() can stop it if a later + * step fails. */ + private static async createOn( + store: JsStore, + ownsStore: boolean, + config: FrameworkConfig, + building: { framework?: AgentFramework }, + ): Promise { // Register framework states try { store.registerState({ id: FRAMEWORK_STATE_ID, strategy: 'snapshot' }); @@ -2201,7 +2285,7 @@ export class AgentFramework { ); } - const framework = new AgentFramework( + const framework = building.framework = new AgentFramework( store, ownsStore, config.membrane, @@ -2684,6 +2768,7 @@ export class AgentFramework { } private async stopOwned(teardownTimeoutMs: number | undefined): Promise { + this.stopFinishedTeardown = false; this.pushCoalescer?.suspend(); this.flushCoalescingSnapshot(); this.running = false; @@ -2806,6 +2891,7 @@ export class AgentFramework { } const teardownSucceeded = shutdownErrors.length === 0; + this.stopFinishedTeardown = teardownSucceeded; // Those later annotations write nothing, so they finish at once; let them // record and edit their stubs before the store closes. @@ -16363,10 +16449,9 @@ export class AgentFramework { // awareness generation. await this.connectMcplServerInternal(config, true); } catch (error) { - if (error instanceof DiscordAwarenessAccountingError) { - await this.mcplServerRegistry.closeAll(); - throw error; - } + // An accounting failure ends startup. create()'s release closes every + // server connected so far, within its deadline. + if (error instanceof DiscordAwarenessAccountingError) throw error; // Fail-open: log and continue with remaining servers const err = error instanceof Error ? error : new Error(String(error)); console.error(`Failed to connect MCPL server "${config.id}": ${err.message}${withStderr(err)}`); @@ -16389,15 +16474,13 @@ export class AgentFramework { // Start the durable drain only after all startup connections are staged. // The awareness tools/call is written before control-plane registration is // flushed, preserving the server-side queue -> registration -> call order. + // A barrier that fails ends startup the same way, and create()'s release + // closes the servers. (A lazy initialization from connectMcplServer has + // connected none yet.) const startupBarrier = this.installMcplDataPlaneGate(); this.releaseMcplDataPlaneGate(startupBarrier); - try { - await startupBarrier.promise; - this.completeMcplDataPlaneGate(startupBarrier); - } catch (error) { - await this.mcplServerRegistry.closeAll(); - throw error; - } + await startupBarrier.promise; + this.completeMcplDataPlaneGate(startupBarrier); // Discover tools from all connected servers await this.refreshMcplTools(); @@ -16880,13 +16963,15 @@ export class AgentFramework { try { await awarenessBarrier.promise; } catch (error) { - // Startup cannot fail open on a broken awareness ledger. Disable the + // A connect cannot fail open on a broken awareness ledger. Disable the // reconnecting stub/connection before propagating the distinct error to - // initializeMcpl, which tears down any other servers and aborts create(). + // the runtime connect or restart that asked for it. (A startup connect + // defers awareness and returns before this gate; a startup that fails + // ends in create()'s release, which closes its servers.) // Ordering matters: awareness accounting settles BEFORE the §5.3 // policy round-trip below, so an accounting failure aborts with the // grant still empty and every buffered event still behind the gate — - // a failed startup never reports a released data plane. (The reverse + // a failed connect never reports a released data plane. (The reverse // order let a push arrive during the policy await and get flushed by // teardown.) await connection.close().catch(() => {}); diff --git a/src/modules/workspace/index.ts b/src/modules/workspace/index.ts index 479000375..3becf2b44 100644 --- a/src/modules/workspace/index.ts +++ b/src/modules/workspace/index.ts @@ -1050,9 +1050,16 @@ export class WorkspaceModule implements Module { // the pins and watermarks as the last work left them. this.stopped = true; - // Persist state - if (this.ctx) { - const activeBranchId = this.store ? this.store.currentBranch().id : undefined; + // Persist state, once initStore() has run and the state start() was + // handed has been applied (savedState is cleared then). Before that this + // session has made no pass or write, so it has nothing to save, and the + // stored state stays as it was: saving would put empty or default mounts + // over the saved pins. A create() that fails after starting this module + // stops it before the host can call initStore() (#389), and an + // initStore() that throws after taking the store leaves the state + // unapplied. + if (this.ctx && this.store && this.savedState === null) { + const activeBranchId = this.store.currentBranch().id; const state: WorkspaceModuleState = { mounts: {}, activeBranchId }; for (const [name, mount] of this.mounts) { state.mounts[name] = { diff --git a/src/types/module.ts b/src/types/module.ts index a6fda9ed2..ca9cb3816 100644 --- a/src/types/module.ts +++ b/src/types/module.ts @@ -53,7 +53,12 @@ export interface Module { /** * Stop the module. - * Called when the framework stops or when the module is removed. + * Called when the framework stops or when the module is removed. Also called + * when a create() that started the module fails (#389): create() stops the + * framework before it rejects, so before the host has it and before anything + * a host wires up after create() returns, such as a WorkspaceModule's + * initStore(). A stop() that saves state can therefore run before that + * wiring, and shouldn't then save over state it was handed and hasn't used. */ stop(): Promise; diff --git a/test/failed-create-releases.test.ts b/test/failed-create-releases.test.ts new file mode 100644 index 000000000..7d063defc --- /dev/null +++ b/test/failed-create-releases.test.ts @@ -0,0 +1,340 @@ +/** + * A create() that fails releases what it had acquired before its error reaches + * the caller, so the caller can retry it in the same process (#389). + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { spawn } from 'node:child_process'; +import { closeSync, mkdirSync, mkdtempSync, openSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { fileURLToPath, pathToFileURL } from 'node:url'; +import { JsStore } from '@animalabs/chronicle'; +import { AgentFramework, type Module } from '../src/index.js'; +import { WorkspaceModule } from '../src/modules/workspace/index.js'; +import { DiscordAwarenessOutbox } from '../src/recovery/discord-awareness-outbox.js'; +import { MockMembrane } from './helpers/mock-membrane.js'; + +function recordingModule(name: string, stop: () => Promise = async () => {}, start: () => Promise = async () => {}) { + const calls: string[] = []; + const module: Module = { + name, + async start() { calls.push('start'); await start(); }, + async stop() { calls.push('stop'); await stop(); }, + async onProcess() { return {}; }, + getTools: () => [], + async handleToolCall() { throw new Error('No tool calls expected'); }, + }; + return { module, calls }; +} + +function tempDir(t: import('node:test').TestContext): string { + const dir = mkdtempSync(join(tmpdir(), 'failed-create-')); + t.after(() => rmSync(dir, { recursive: true, force: true })); + return dir; +} + +function baseConfig(storePath: string) { + return { storePath, membrane: new MockMembrane().asMembrane(), agents: [], modules: [] as Module[], + syncIntervalMs: 0, maintenanceIntervalMs: 0 }; +} + +/** A release that finished cleanly rejects with create()'s own error, unwrapped. */ +function rejectsWithItsOwnError(creating: Promise, message: string | RegExp) { + return assert.rejects(creating, (error: unknown) => { + assert.ok(error instanceof Error && !(error instanceof AggregateError), `expected create()'s own error, got ${String(error)}`); + if (typeof message === 'string') assert.equal(error.message, message); + else assert.match(error.message, message); + return true; + }); +} + +// Two servers on one prefix: refused after the modules have started, before +// any MCPL server is spawned (the reproduction in #389). +const COLLIDING_SERVERS = [ + { id: 'first', command: process.execPath, args: ['-e', ''], toolPrefix: 'shared' }, + { id: 'second', command: process.execPath, args: ['-e', ''], toolPrefix: 'shared' }, +]; +const COLLISION = 'MCPL server "second" toolPrefix "shared" collides with server "first"'; + +test('a create() refused after its modules started stops them and frees its store', async t => { + const storePath = join(tempDir(t), 'store'); + const { module, calls } = recordingModule('started'); + await rejectsWithItsOwnError( + AgentFramework.create({ ...baseConfig(storePath), modules: [module], mcplServers: COLLIDING_SERVERS }), + COLLISION, + ); + assert.deepEqual(calls, ['start', 'stop']); + const retried = await AgentFramework.create(baseConfig(storePath)); + await retried.stop(); +}); + +test('a create() whose module fails to start stops every module it started, that one included', async t => { + const storePath = join(tempDir(t), 'store'); + const first = recordingModule('first'); + const failing = recordingModule('failing', undefined, async () => { throw new Error('the port is taken'); }); + await rejectsWithItsOwnError( + AgentFramework.create({ ...baseConfig(storePath), modules: [first.module, failing.module] }), + 'the port is taken', + ); + assert.deepEqual(first.calls, ['start', 'stop']); + assert.deepEqual(failing.calls, ['start', 'stop']); + const retried = await AgentFramework.create(baseConfig(storePath)); + await retried.stop(); +}); + +test('a create() that fails after its MCPL server connected closes the server and frees its store', async t => { + const dir = tempDir(t); + const outboxPath = join(dir, 'awareness.json'); + // A pending Discord awareness batch makes startup run the awareness barrier + // after the server connects; its second reconciliation fails. + const outbox = new DiscordAwarenessOutbox(outboxPath); + const batch = outbox.prepare({ agentName: 'assistant', sourceBranch: 'source', targetBranch: 'main', + activationPolicy: 'explicit', + refs: [{ serverId: 'discord', channelId: 'discord:guild:startup', messageId: 'message-1' }] }); + assert.ok(batch); + outbox.activate(batch.id); + const config = { + ...baseConfig(join(dir, 'store')), + agents: [{ name: 'assistant', model: 'test', systemPrompt: 'test' }], + discordAwarenessOutboxPath: outboxPath, + discordAwarenessDeadlineMs: 1_000, + mcplServers: [{ + id: 'discord', command: process.execPath, + args: [fileURLToPath(new URL('./fixtures/awareness-barrier-mcpl-server.mjs', import.meta.url))], + requestTimeoutMs: 1_000, enabledFeatureSets: ['chat'], + env: { STATUS_PATH: join(dir, 'status.jsonl'), LEDGER_PATH: outboxPath }, + }], + }; + const reconcile = DiscordAwarenessOutbox.prototype.reconcileForBranch; + let reconciliations = 0; + const injected = t.mock.method(DiscordAwarenessOutbox.prototype, 'reconcileForBranch', + function (this: DiscordAwarenessOutbox, ...args: Parameters) { + if (++reconciliations === 2) throw new Error('injected reconcile failure'); + return reconcile.apply(this, args); + }); + await rejectsWithItsOwnError(AgentFramework.create(config), + /^Discord awareness accounting failed during .*: injected reconcile failure$/); + injected.mock.restore(); + const initialized = readFileSync(join(dir, 'status.jsonl'), 'utf8').split('\n').filter(Boolean) + .map(line => JSON.parse(line) as { event: string; pid?: number }) + .find(record => record.event === 'initialized'); + assert.ok(initialized?.pid, 'the MCPL server had finished its handshake before the failure'); + // Wait on that child itself, with room for a loaded machine: only a child + // still running after 10 s fails this. + const running = (pid: number) => { + try { process.kill(pid, 0); return true; } + catch (error) { return (error as NodeJS.ErrnoException).code === 'EPERM'; } + }; + for (let waits = 0; running(initialized.pid); waits++) { + assert.ok(waits < 1000, 'the MCPL server is still running after create() rejected'); + await new Promise(done => setTimeout(done, 10)); + } + const retried = await AgentFramework.create(config); + await retried.stop(); +}); + +test('a create() refused before the framework is built frees its store', async t => { + const storePath = join(tempDir(t), 'store'); + await rejectsWithItsOwnError( + AgentFramework.create({ ...baseConfig(storePath), timeZone: 'Not/AZone' }), + 'Invalid IANA time zone: "Not/AZone"', + ); + const retried = await AgentFramework.create(baseConfig(storePath)); + await retried.stop(); +}); + +test('a failed create() leaves a store the caller owns open, and stops what it started', async t => { + const dir = mkdtempSync(join(tmpdir(), 'failed-create-')); + const store = JsStore.openOrCreate({ path: join(dir, 'store') }); + t.after(() => { try { store.close(); } finally { rmSync(dir, { recursive: true, force: true }); } }); + const { module, calls } = recordingModule('started'); + const { storePath: _unused, ...config } = baseConfig(''); + await rejectsWithItsOwnError( + AgentFramework.create({ ...config, store, modules: [module], mcplServers: COLLIDING_SERVERS }), + COLLISION, + ); + assert.deepEqual(calls, ['start', 'stop']); + assert.equal(store.isClosed(), false); + await rejectsWithItsOwnError( + AgentFramework.create({ ...config, store, timeZone: 'Not/AZone' }), + 'Invalid IANA time zone: "Not/AZone"', + ); + assert.equal(store.isClosed(), false); + const retried = await AgentFramework.create({ ...config, store }); + await retried.stop(); +}); + +test('a failed create() whose module fails to stop reports both, and still frees its store', async t => { + const storePath = join(tempDir(t), 'store'); + const { module } = recordingModule('stuck', async () => { throw new Error('the socket would not close'); }); + await assert.rejects( + AgentFramework.create({ ...baseConfig(storePath), modules: [module], mcplServers: COLLIDING_SERVERS }), + (error: unknown) => { + assert.ok(error instanceof AggregateError, `expected an AggregateError, got ${String(error)}`); + assert.equal(error.message, `AgentFramework.create() failed: ${COLLISION}. Stopping what it had started ` + + `didn't finish, so part of it may still be running: the socket would not close`); + assert.equal((error.errors[0] as Error).message, COLLISION); + assert.equal(error.errors.length, 2); + return true; + }, + ); + const retried = await AgentFramework.create(baseConfig(storePath)); + await retried.stop(); +}); + +test('a failed create() whose store fails to close says so, after its own error', async t => { + const storePath = join(tempDir(t), 'store'); + const { module } = recordingModule('stuck', async () => { throw new Error('the socket would not close'); }); + // The store does close, so the retry below can open it; close() then reports + // a failure, as a close whose final sync fails would. + const close = JsStore.prototype.close; + const failing = t.mock.method(JsStore.prototype, 'close', function (this: JsStore) { + failing.mock.restore(); + close.call(this); + throw new Error('the final sync failed'); + }); + await assert.rejects( + AgentFramework.create({ ...baseConfig(storePath), modules: [module], mcplServers: COLLIDING_SERVERS }), + (error: unknown) => { + assert.ok(error instanceof AggregateError, `expected an AggregateError, got ${String(error)}`); + assert.equal(error.message, `AgentFramework.create() failed: ${COLLISION}. Stopping what it had started ` + + `didn't finish, so part of it may still be running: the socket would not close. Closing its store ` + + `failed, so the store may still be locked: the final sync failed`); + assert.deepEqual(error.errors.map(e => (e as Error).message), + [COLLISION, 'the socket would not close', 'the final sync failed']); + return true; + }, + ); + const retried = await AgentFramework.create(baseConfig(storePath)); + await retried.stop(); +}); + +test('a failed create() whose stop fails only at the end says nothing it started is still running', async t => { + const storePath = join(tempDir(t), 'store'); + const { module, calls } = recordingModule('clean'); + // Once everything it waits on has stopped, stop() closes the store itself. + // That close fails without closing, as a close whose final sync fails before + // the handle goes would, and the release's own close then succeeds. + const failing = t.mock.method(JsStore.prototype, 'close', function (this: JsStore) { + failing.mock.restore(); + throw new Error('the final sync failed'); + }); + await assert.rejects( + AgentFramework.create({ ...baseConfig(storePath), modules: [module], mcplServers: COLLIDING_SERVERS }), + (error: unknown) => { + assert.ok(error instanceof AggregateError, `expected an AggregateError, got ${String(error)}`); + assert.equal(error.message, `AgentFramework.create() failed: ${COLLISION}. What it had started stopped, ` + + `but finishing the stop failed: the final sync failed`); + assert.deepEqual(error.errors.map(e => (e as Error).message), [COLLISION, 'the final sync failed']); + return true; + }, + ); + assert.deepEqual(calls, ['start', 'stop']); + const retried = await AgentFramework.create(baseConfig(storePath)); + await retried.stop(); +}); + +test('a failed create() gives up on a module\'s stop after 30 s, and says what did not finish', async t => { + const storePath = join(tempDir(t), 'store'); + let release!: () => void; + const stopping = new Promise(done => { release = done; }); + t.after(() => release()); + const { module, calls } = recordingModule('hung', () => stopping); + t.mock.timers.enable({ apis: ['setTimeout'] }); + let settled = false; + const rejection = AgentFramework.create({ ...baseConfig(storePath), modules: [module], mcplServers: COLLIDING_SERVERS }) + .then(() => assert.fail('create() should fail'), (error: unknown) => error) + .finally(() => { settled = true; }); + // The module's stop() is called once the release's deadline is armed. + for (let turns = 0; !calls.includes('stop'); turns++) { + assert.ok(turns < 1000, 'the release never reached the module'); + await new Promise(done => setImmediate(done)); + } + t.mock.timers.tick(29_999); + await new Promise(done => setImmediate(done)); + assert.equal(settled, false); + t.mock.timers.tick(1); + for (let turns = 0; !settled; turns++) { + assert.ok(turns < 1000, 'create() still waits on its release after 30 s'); + await new Promise(done => setImmediate(done)); + } + const error = await rejection; + assert.ok(error instanceof AggregateError, `expected an AggregateError, got ${String(error)}`); + assert.equal(error.message, `AgentFramework.create() failed: ${COLLISION}. Stopping what it had started ` + + `didn't finish, so part of it may still be running: Shutdown timed out after 30000 ms; pending: module:hung`); + t.mock.timers.reset(); + const retried = await AgentFramework.create(baseConfig(storePath)); + await retried.stop(); +}); + +// The watchdog's worker kills the whole process once the main thread stops +// beating for its threshold. One a failed create() left running would kill a +// process that carried on after the failure. +test('a create() refused after it started the watchdog leaves none to kill the process later', async t => { + const dir = tempDir(t); + const script = join(dir, 'child.mjs'); + const index = pathToFileURL(join(import.meta.dirname, '../src/index.js')).href; + const membrane = pathToFileURL(join(import.meta.dirname, 'helpers/mock-membrane.js')).href; + writeFileSync(script, ` + import { AgentFramework } from ${JSON.stringify(index)}; + import { MockMembrane } from ${JSON.stringify(membrane)}; + const refusal = await AgentFramework.create({ + storePath: ${JSON.stringify(join(dir, 'store'))}, membrane: new MockMembrane().asMembrane(), + agents: [], modules: [], syncIntervalMs: 0, maintenanceIntervalMs: 0, + watchdog: { enabled: true, thresholdMs: 2000, action: 'exit' }, + toolResultInlineMaxChars: 1, + }).then(() => 'none', error => error.message); + console.error('refused: ' + refusal); + await new Promise(done => setTimeout(done, 200)); + const end = Date.now() + 5000; + while (Date.now() < end) {} + process.exit(0); + `); + // A file, not a pipe: writes to it are synchronous on every platform, so a + // killed child's lines are all there. + const logPath = join(dir, 'child.log'); + const log = openSync(logPath, 'w'); + const { code, signal } = await new Promise<{ code: number | null; signal: string | null }>(resolve => { + const child = spawn(process.execPath, [script], { stdio: ['ignore', log, log] }); + const safety = setTimeout(() => child.kill('SIGTERM'), 20_000); + child.on('exit', (code, signal) => { clearTimeout(safety); resolve({ code, signal }); }); + }); + closeSync(log); + const output = readFileSync(logPath, 'utf8'); + assert.match(output, /\[liveness-watchdog\] armed/, 'the watchdog started before the refusal'); + assert.match(output, /refused: FrameworkConfig\.toolResultInlineMaxChars must be a number >= 1000/); + assert.deepEqual({ code, signal }, { code: 0, signal: null }); +}); + +test('a failed create() on a restart leaves the workspace\'s saved pins as they were', async t => { + const dir = tempDir(t); + const storePath = join(dir, 'store'); + const mountDir = join(dir, 'mount'); + mkdirSync(mountDir); + const workspace = () => new WorkspaceModule({ mounts: [{ name: 'work', path: mountDir, mode: 'read-write', watch: 'never' }] }); + const savedState = () => { + const store = JsStore.openOrCreate({ path: storePath }); + try { return store.getStateJson('modules/workspace/state'); } finally { store.close(); } + }; + + // A session that materializes, so its stop() saves the branch it pinned. + const first = workspace(); + const framework = await AgentFramework.create({ ...baseConfig(storePath), modules: [first] }); + first.initStore(framework.getStore()); + let result = await first.handleToolCall({ id: 'w', name: 'write', input: { path: 'work/one.txt', content: 'one' } }); + assert.equal(result.success, true, `write failed: ${result.error}`); + result = await first.handleToolCall({ id: 'm', name: 'materialize', input: {} }); + assert.equal(result.success, true, `materialize failed: ${result.error}`); + await framework.stop(); + const pinned = savedState() as { mounts: { work?: { lastMaterializedBranchId?: string } } }; + assert.ok(pinned.mounts.work?.lastMaterializedBranchId, 'the session saved a pin'); + + // A restart refused after the workspace module started, so before the host + // could call its initStore(). + await rejectsWithItsOwnError( + AgentFramework.create({ ...baseConfig(storePath), modules: [workspace()], mcplServers: COLLIDING_SERVERS }), + COLLISION, + ); + assert.deepEqual(savedState(), pinned, 'the saved state is as the session left it'); +}); diff --git a/test/fixtures/awareness-barrier-mcpl-server.mjs b/test/fixtures/awareness-barrier-mcpl-server.mjs index f8789d22d..b83fff1a5 100644 --- a/test/fixtures/awareness-barrier-mcpl-server.mjs +++ b/test/fixtures/awareness-barrier-mcpl-server.mjs @@ -161,7 +161,7 @@ function sendDataRequest(id, phase) { function beginServerTraffic() { if (initialized) return; initialized = true; - log('initialized'); + log('initialized', { pid: process.pid }); request(200, 'channels/register', { channels: [channel('startup')] }); // The reconnect generation carries an inference-bearing event in the same // registration window. Initial-start tests use generation 1 as well. diff --git a/test/workspace-restore-order.test.ts b/test/workspace-restore-order.test.ts index 4cf8401a0..85c0bbf1a 100644 --- a/test/workspace-restore-order.test.ts +++ b/test/workspace-restore-order.test.ts @@ -20,6 +20,7 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { JsStore } from '@animalabs/chronicle'; import { WorkspaceModule } from '../src/modules/workspace/index.js'; +import { DiskAgreement } from '../src/modules/workspace/disk-agreement.js'; import type { WorkspaceModuleState } from '../src/modules/workspace/types.js'; import type { ModuleContext } from '../src/types/module.js'; @@ -166,3 +167,27 @@ test('a fresh (non-restart) start still initializes mounts at zero', async (t) = assert.equal(status.lastMaterializedBranch, null); assert.equal(status.canMaterialize, true); }); + +test('a restart stopped before its saved state is applied leaves the stored state as it was', async (t) => { + const { store, makeModule } = setup(t); + const { saved } = await runFirstSession(store, makeModule); + const saves: unknown[] = []; + const ctx = { ...makeCtx({ isRestart: true, saved }), setState: (state: unknown) => { saves.push(state); } } as unknown as ModuleContext; + + // Stopped before initStore(), as a create() that fails after starting it + // stops it (#389). + const notWired = makeModule(); + await notWired.start(ctx); + await notWired.stop(); + + // Stopped after an initStore() that threw once it had taken the store and + // made the mounts, before it applied the saved state. + const failedInit = makeModule(); + await failedInit.start(ctx); + const open = t.mock.method(DiskAgreement.prototype, 'open', () => { throw new Error('the journal is unreadable'); }); + assert.throws(() => failedInit.initStore(store), /the journal is unreadable/); + open.mock.restore(); + await failedInit.stop(); + + assert.deepEqual(saves, [], 'neither stop saved over the stored state'); +});