From 03151f909a354dc205b7d621894479fdbc918c75 Mon Sep 17 00:00:00 2001 From: Nell-1783 Date: Tue, 13 Oct 2026 08:16:46 -0400 Subject: [PATCH 01/21] fix(framework): stop() waits for an operator's operation already running stop() refuses a host/command that arrives once it has begun, but an operation admitted before that kept running while stop() tore down what it used. A resume waiting on a maintenance pass or its feasibility previews went on to leave quiesce while the host stopped: it cleared the durable quiesce mark, opened the event gate and reopened the MCPL data planes that stop() was closing, and answered ok. A quiesce kept polling its drain for up to its ten-minute window after stop() returned. A store surgery (rollbackToMessage, suppressMessages, message-granular undo) could find the store closed while it synced awareness markers after its branch switch, and a host/command could answer after the connections it answers on had gone. stop() now waits, once the event loop has ended and before the event gate, modules, MCPL connections and store go, for the operations it tracks: quiesce(), resume(), each host/command, a store surgery until it releases the store, and syncDiscordAwarenessMarkers(), which an operator's undo, redo or branch switch runs. Each stops waiting once the queue is closed. A quiesce leaves its drain loop and an abandon's grace, so the host stays quiesced and boots quiesced. A resume refuses if stop() began before it left quiesce; one already past that point finishes before the final sync. quiesce() and resume() called once stop() has begun are refused, as a host/command already was. A failed awareness drain no longer recycles the MCPL connections of a host that is stopping. teardownTimeoutMs bounds this wait as it bounds module and MCPL cleanup: an operation that outlasts it fails stop() with a ShutdownTimeoutError naming it, and the store stays open so that stop() can be called again. test/stop-with-host-operations.test.ts holds each kind of operation across stop() and checks the order of its end against the modules' teardown, so the verdicts don't rest on timing. CI runs the file unforced with the other work-in-flight files. --- .github/workflows/ci.yml | 2 +- ...top-waits-for-operator-operations.fixed.md | 24 ++ src/framework.ts | 101 +++++- test/stop-with-host-operations.test.ts | 326 ++++++++++++++++++ 4 files changed, 441 insertions(+), 12 deletions(-) create mode 100644 changelog.d/stop-waits-for-operator-operations.fixed.md create mode 100644 test/stop-with-host-operations.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 630e417e9..2bd45fc08 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -56,4 +56,4 @@ jobs: # closed"). These files run unforced so that class fails here, not in a # later PR's qualification. - name: Test unforced (work in flight at stop) - run: node --test dist/test/stop-with-work-in-flight.test.js dist/test/conversation-routing.test.js + run: node --test dist/test/stop-with-work-in-flight.test.js dist/test/stop-with-host-operations.test.js dist/test/conversation-routing.test.js diff --git a/changelog.d/stop-waits-for-operator-operations.fixed.md b/changelog.d/stop-waits-for-operator-operations.fixed.md new file mode 100644 index 000000000..af6bf5d3d --- /dev/null +++ b/changelog.d/stop-waits-for-operator-operations.fixed.md @@ -0,0 +1,24 @@ +- **`stop()` waits for an operator's operation that is already running before + it tears down what that operation uses.** A quiesce, a resume, a + `host/command`, a store surgery (`rollbackToMessage`, `suppressMessages`) + or an awareness sync (`syncDiscordAwarenessMarkers`) admitted before + `stop()` began now finishes before the event gate, the modules, the MCPL + connections and the store go. Before, such an operation kept running + through the teardown: a resume could clear the durable quiesce mark and + reopen the data planes after the final sync, a surgery could find its + store closed partway, and a `host/command` answer could miss its + connection. + - A quiesce stops waiting for its drain, or for an abandoned turn's grace, + once `stop()` begins. `stop()` ends the turns itself. The host stays + quiesced, so it boots quiesced. + - A resume that hasn't left quiesce when `stop()` begins is refused with + an error, and the host stays quiesced. One that has left it finishes, + its durable clear included, before the final sync. + - `quiesce()` and `resume()` called once `stop()` has begun are refused + with an error, as a `host/command` already was. + - A failed awareness drain no longer recycles the MCPL connections of a + host that is stopping, since `stop()` is closing them. + - **For operators:** `stop(teardownTimeoutMs)` bounds this wait too. An + operation that outlasts it fails `stop()` with a `ShutdownTimeoutError` + that names it, and the store stays open, as for a module whose teardown + timed out, so `stop()` can be called again. diff --git a/src/framework.ts b/src/framework.ts index dbb2e4033..223dc3264 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -61,7 +61,7 @@ import type { SameRoundThinkTextPolicy, } from './types/index.js'; import { ProcessQueueImpl } from './queue.js'; -import { DEFAULT_SHUTDOWN_TIMEOUT_MS, validateShutdownTimeout, shutdownErrorMessage } from './shutdown.js'; +import { DEFAULT_SHUTDOWN_TIMEOUT_MS, validateShutdownTimeout, shutdownErrorMessage, waitForShutdown } from './shutdown.js'; import { REFUSAL_REACTIONS, REFUSAL_REACTION_FALLBACK } from './refusal-reactions.js'; import { Agent } from './agent.js'; import { ModuleRegistry, isStateExistsError } from './module-registry.js'; @@ -1138,6 +1138,10 @@ export class AgentFramework { private stopPromise: Promise | null = null; private stopCompleted = false; private ownedStoreClosed = false; + /** An operator's operations in flight, which stop() waits for before it + * tears down what they use: quiesce(), resume(), each host/command, a + * store surgery until it releases the store, and an awareness sync. */ + private readonly operatorOperations = new Set<{ label: string; promise: Promise }>(); private membrane: Membrane; private queue: ProcessQueueImpl; private agents: Map = new Map(); @@ -2132,9 +2136,11 @@ export class AgentFramework { /** * Stop the event loop. Failed module/MCPL cleanup can be retried with its * original context; storage stays open until that cleanup succeeds. - * @param teardownTimeoutMs Optional module/MCPL wait budget. Omit to wait - * until cleanup settles, as required by existing module/child contracts. This - * does not bound earlier stream or event-loop quiescence, or cancel cleanup. + * @param teardownTimeoutMs Optional budget for each wait on cleanup: an + * operator's operations already running, then modules and MCPL together. + * Omit to wait until cleanup settles, as required by existing module/child + * contracts. This does not bound earlier stream or event-loop quiescence, or + * cancel cleanup. */ stop(teardownTimeoutMs = DEFAULT_SHUTDOWN_TIMEOUT_MS): Promise { try { validateShutdownTimeout(teardownTimeoutMs); } @@ -2159,6 +2165,15 @@ export class AgentFramework { } } + /** Count an operator's operation as in flight until it settles, so that + * stop() can wait for it (see stopOwned). */ + private trackOperatorOperation(label: string, operation: Promise): Promise { + const entry = { label, promise: operation.then(() => {}, () => {}) }; + this.operatorOperations.add(entry); + void entry.promise.then(() => { this.operatorOperations.delete(entry); }); + return operation; + } + private async stopOwned(teardownTimeoutMs: number | undefined): Promise { this.pushCoalescer?.suspend(); this.flushCoalescingSnapshot(); @@ -2230,6 +2245,20 @@ export class AgentFramework { await this.loopPromise; } + // An operator's operation admitted before stop() began (a quiesce + // draining, a resume, an undo) finishes before what it uses is torn down: + // the event gate, the MCPL connections and the store. Once the queue is + // closed, a quiesce stops waiting for its drain and a resume that hasn't + // left quiesce refuses; the rest finish their work. One that outlasts the + // budget may still write, so the store stays open, as it does for a + // module whose teardown timed out. + let operatorOperationsTimedOut: unknown; + try { + await waitForShutdown([...this.operatorOperations], teardownTimeoutMs); + } catch (error) { + operatorOperationsTimedOut = error; + } + // Stop typing indicators this.channelRegistry?.stopAll(); @@ -2244,7 +2273,7 @@ export class AgentFramework { } // Timed-out or failed teardown retains its context and may still write. // Sync available state best-effort, but leave its store open for a retry. - const shutdownErrors: unknown[] = []; + const shutdownErrors: unknown[] = operatorOperationsTimedOut === undefined ? [] : [operatorOperationsTimedOut]; for (const result of await Promise.allSettled(shutdownPromises)) { if (result.status === 'rejected') shutdownErrors.push(result.reason); } @@ -3367,11 +3396,21 @@ export class AgentFramework { * dedicated cancel kind that settles the turn without feeding the * inference-failure accounting. */ - async quiesce(opts?: { + quiesce(opts?: { + reason?: string; + timeoutMs?: number; + abandon?: boolean; + }): Promise { + return this.trackOperatorOperation('host:quiesce', this.runQuiesce(opts)); + } + + private async runQuiesce(opts?: { reason?: string; timeoutMs?: number; abandon?: boolean; }): Promise { + // stop() closes the queue first: a stopping host takes no host command. + if (this.queue?.isClosed) throw new Error('quiesce refused: the framework is stopping'); if (this.quiesced) { // Idempotent — EXCEPT abandon escalation: a first quiesce that timed // out undrained must be escalatable with a second quiesce({abandon}) @@ -3426,6 +3465,7 @@ export class AgentFramework { // continuation's quiesce recheck requeues it — wait for that to happen). while ( this.quiesced && + !this.queue?.isClosed && (this.activeTurnTokens.size > 0 || this.parkedAdmissionCount() > 0) && Date.now() < deadline ) { @@ -3440,6 +3480,13 @@ export class AgentFramework { console.error('[host-mode] quiesce superseded by a concurrent resume during drain'); return this.getHostModeStatus(); } + // stop() began while we waited. It ends every turn itself, and it waits + // for this call before it tears down. The host stays quiesced, so it + // boots quiesced. + if (this.queue?.isClosed) { + console.error('[host-mode] quiesce stopped waiting for its drain: the framework is stopping'); + return this.getHostModeStatus(); + } let abandoned = false; let unabandonable: string[] = []; @@ -3499,9 +3546,10 @@ export class AgentFramework { ); } } - // Bounded grace for the cancelled streams' teardown to settle. + // Bounded grace for the cancelled streams' teardown to settle. A stopping + // framework ends the turns itself and waits for this call, so stop waiting. const grace = Date.now() + 10_000; - while (this.quiesced && this.activeTurnTokens.size > 0 && Date.now() < grace) { + while (this.quiesced && !this.queue?.isClosed && this.activeTurnTokens.size > 0 && Date.now() < grace) { await new Promise((resolve) => setTimeout(resolve, 250)); } // Only turns still alive after the grace are worth reporting. @@ -3517,7 +3565,11 @@ export class AgentFramework { * Unavailable previews warn and pass — refusing to resume because the * strategy cannot preview would hold hosts hostage to a diagnostic. */ - async resume(opts?: { force?: boolean }): Promise { + resume(opts?: { force?: boolean }): Promise { + return this.trackOperatorOperation('host:resume', this.runResume(opts)); + } + + private async runResume(opts?: { force?: boolean }): Promise { if (!this.quiesced) return this.getHostModeStatus(); // The canonical operator flow is `maintain; resume`, and a timer-driven @@ -3545,6 +3597,12 @@ export class AgentFramework { ); } } + // stop() closes the queue first. If it has begun, before this call or + // during the waits above, leave the host quiesced, as it will boot, + // rather than reopen what stop() is closing. + if (this.queue?.isClosed) { + throw new Error('resume refused: the framework is stopping, so the host stays quiesced'); + } if (failing.length > 0 && !opts?.force) { const detail = failing.map(({ agentName, preview }) => `${agentName}: folded floor ${preview.effective!.finalTokens} > hard budget ` + @@ -5391,7 +5449,15 @@ export class AgentFramework { * host-status — report the quiesce state (drained / parked wakes / * running background scripts). */ - private async handleHostCommand( + private handleHostCommand( + serverId: string, + params: HostCommandParams, + ): ReturnType { + return this.trackOperatorOperation(`host/command:${String(params.command)}`, this.runHostCommand(serverId, params)); + } + + /** handleHostCommand's work, run where stop() can wait for it. */ + private async runHostCommand( serverId: string, params: HostCommandParams, ): Promise<{ @@ -6225,10 +6291,16 @@ export class AgentFramework { // arrivals during the awaited switch (see surgeryHold). const hold = { verb, agentName, since: Date.now() }; this.surgeryHold = hold; + // stop() waits for the release (see stopOwned): a surgery admitted before + // stop() began finishes, flush included, before the store closes. + let settle!: () => void; + void this.trackOperatorOperation(`surgery:${verb}`, new Promise((resolve) => { settle = resolve; })); let released = false; return () => { if (released) return; released = true; + // The release runs to its end before stop() can go on. + settle(); if (this.surgeryHold === hold) this.surgeryHold = null; for (const [name, token] of reserved) { if (this.activeTurnTokens.get(name) === token) this.activeTurnTokens.delete(name); @@ -13888,7 +13960,11 @@ export class AgentFramework { } /** Reconcile the durable ledger with Chronicle, then deliver every server's work. */ - async syncDiscordAwarenessMarkers(_onlyServerId?: string): Promise { + syncDiscordAwarenessMarkers(onlyServerId?: string): Promise { + return this.trackOperatorOperation('awareness:sync', this.runDiscordAwarenessSync(onlyServerId)); + } + + private async runDiscordAwarenessSync(_onlyServerId?: string): Promise { if (!this.discordAwarenessOutbox) return; // A targeted retry may discover pending work for other Discord servers // during reconciliation. Safety is global, so one generation accounts for @@ -14220,6 +14296,9 @@ export class AgentFramework { error: `Discord awareness accounting unhealthy: ${err.message}`, }); } + // A stopping host closes these connections itself. Recycling them now + // would start reconnects only for stop() to close them again. + if (this.queue?.isClosed) return; await Promise.all(connections.map(async (connection) => { await connection.reconnectAfterFailure().catch((closeError) => { console.error( diff --git a/test/stop-with-host-operations.test.ts b/test/stop-with-host-operations.test.ts new file mode 100644 index 000000000..9f5366217 --- /dev/null +++ b/test/stop-with-host-operations.test.ts @@ -0,0 +1,326 @@ +/** + * stop() with an operator's operation still running. + * + * stop() refuses a host/command that arrives once it has begun. These tests + * cover the operations admitted before that: a quiesce draining, a resume, a + * host/command, a store surgery and an awareness sync, whether they came over + * MCPL or from a direct call. stop() waits for each before it tears down what + * it uses: the event gate, the modules, the MCPL connections and the store. + * A quiesce stops waiting for its drain, and a resume that hasn't committed + * refuses, so the wait stays short and the host boots in the mode it had. + * + * Where a test holds an operation open, it lets it go once stop() reaches its + * modules' teardown, or after a short while if stop() is waiting short of it. + * The order of the two events is the verdict, so it doesn't depend on timing. + */ + +import { describe, it, beforeEach, afterEach } from 'node:test'; +import assert from 'node:assert/strict'; +import { EventEmitter } from 'node:events'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import type { EventResponse, Module, ModuleContext, ToolDefinition, ToolResult } from '../src/index.js'; +import { AgentFramework, ShutdownTimeoutError } from '../src/index.js'; +import { MockMembrane } from './helpers/mock-membrane.js'; + +/** Resolve once `cond` holds; fail, naming `what`, after `ms`. */ +async function until(cond: () => boolean, what: string, ms = 5000): Promise { + const deadline = Date.now() + ms; + while (!cond()) { + if (Date.now() > deadline) throw new Error(`timed out waiting for ${what}`); + await new Promise((r) => setTimeout(r, 5)); + } +} + +/** Records when stop() reaches its modules' teardown, which comes after its wait for operations. */ +class TeardownProbe implements Module { + readonly name = 'teardown-probe'; + constructor(private readonly order: string[]) {} + async start(_ctx: ModuleContext): Promise {} + async stop(): Promise { this.order.push('modules stopped'); } + getTools(): ToolDefinition[] { return []; } + async handleToolCall(): Promise { return { success: false, error: 'no tools' }; } + async onProcess(): Promise { return {}; } +} + +/** The private state these tests set up or hold. */ +interface Internals { + activeTurnTokens: Map; + maintenancePass: Promise | null; + mcplServerRegistry: unknown; + beginDiscordAwarenessBarrier: () => { requiresBarrier: boolean; promise: Promise }; + wireMcplEvents(connection: unknown): void; +} +const internals = (framework: AgentFramework): Internals => framework as unknown as Internals; + +describe('stop() with an operator operation in flight', () => { + let tempDir: string; + let storePath: string; + let logged: string[]; + let rejections: unknown[]; + let order: string[]; + const onRejection = (reason: unknown): void => { rejections.push(reason); }; + const consoleError = console.error; + const consoleWarn = console.warn; + + beforeEach(() => { + tempDir = mkdtempSync(join(tmpdir(), 'af-stop-host-ops-')); + storePath = join(tempDir, 'store'); + logged = []; + rejections = []; + order = []; + process.on('unhandledRejection', onRejection); + console.error = (...args: unknown[]) => { logged.push(args.map(String).join(' ')); }; + console.warn = (...args: unknown[]) => { logged.push(args.map(String).join(' ')); }; + }); + + afterEach(() => { + console.error = consoleError; + console.warn = consoleWarn; + process.off('unhandledRejection', onRejection); + rmSync(tempDir, { recursive: true, force: true }); + }); + + function create(): Promise { + return AgentFramework.create({ + storePath, + membrane: new MockMembrane().asMembrane(), + agents: [{ name: 'agent', model: 'test-model', systemPrompt: 'test' }], + modules: [new TeardownProbe(order)], + }); + } + + /** Let the held operation go once stop() reaches its modules, or after 300 ms if it waits short of them. */ + async function releaseWhenStopWaits(release: () => void): Promise { + const deadline = Date.now() + 300; + while (!order.includes('modules stopped') && Date.now() < deadline) { + await new Promise((r) => setTimeout(r, 5)); + } + release(); + } + + async function bootsQuiesced(): Promise { + const restarted = await create(); + try { + return restarted.getHostModeStatus().quiesced; + } finally { + await restarted.stop(); + } + } + + it('a quiesce waiting for its drain stops waiting when stop() begins, and stop() waits for it', { timeout: 20_000 }, async () => { + const framework = await create(); + framework.start(); + // A turn between dequeue and stream registration holds its token with no + // stream to cancel, so nothing but stop() can end this drain. + const tokens = internals(framework).activeTurnTokens; + tokens.set('agent', -1); + let settled = false; + const quiescing = framework.quiesce({ reason: 'surgery', timeoutMs: 600_000 }); + void quiescing.then(() => { settled = true; }, () => { settled = true; }); + try { + await framework.stop(); + assert.equal(settled, true, 'stop() returned while the quiesce was still waiting'); + const status = await quiescing; + assert.equal(status.quiesced, true); + assert.equal(status.drained, false); + assert.ok(logged.some((line) => line.includes('quiesce stopped waiting for its drain: the framework is stopping')), logged.join('\n')); + } finally { + tokens.delete('agent'); + await quiescing.catch(() => {}); + } + assert.equal(await bootsQuiesced(), true, 'the host boots quiesced'); + assert.deepEqual(rejections, []); + }); + + it("an abandon waiting on a turn it can't cancel stops waiting when stop() begins, and stop() waits for it", { timeout: 20_000 }, async () => { + const framework = await create(); + framework.start(); + const tokens = internals(framework).activeTurnTokens; + tokens.set('agent', -1); + let settled = false; + const quiescing = framework.quiesce({ timeoutMs: 1_000, abandon: true }); + void quiescing.then(() => { settled = true; }, () => { settled = true; }); + try { + // After its one-second window, abandon finds no stream to cancel and + // gives the turn ten seconds to settle on its own. + await until(() => logged.some((line) => line.includes('cannot abandon turn for agent')), 'abandon to find the held token'); + const began = Date.now(); + await framework.stop(); + const took = Date.now() - began; + assert.equal(settled, true, 'stop() returned while the abandon was still waiting'); + assert.ok(took < 5_000, `stop() took ${took} ms, waiting out the abandon's ten seconds`); + assert.deepEqual((await quiescing).unabandonable, ['agent']); + } finally { + tokens.delete('agent'); + await quiescing.catch(() => {}); + } + assert.deepEqual(rejections, []); + }); + + it('a resume waiting when stop() begins refuses, and the host stays quiesced', async () => { + const framework = await create(); + framework.start(); + await framework.quiesce({ reason: 'surgery' }); + // resume() waits for a running maintenance pass before its feasibility + // gate, and so does stop(): both go on when the pass ends. + let endPass!: () => void; + internals(framework).maintenancePass = new Promise((resolve) => { endPass = resolve; }); + const resuming = framework.resume(); + const stopping = framework.stop(); + endPass(); + await assert.rejects(resuming, /^Error: resume refused: the framework is stopping, so the host stays quiesced$/); + await stopping; + assert.equal(await bootsQuiesced(), true, 'the host boots quiesced'); + assert.deepEqual(rejections, []); + }); + + it('a quiesce called after stop() is refused, and persists nothing', async () => { + const framework = await create(); + await framework.stop(); + await assert.rejects(framework.quiesce({ reason: 'too late' }), /^Error: quiesce refused: the framework is stopping$/); + assert.equal(await bootsQuiesced(), false, 'the host boots serving'); + }); + + it('a resume called after stop() is refused, and the host stays quiesced', async () => { + const framework = await create(); + await framework.quiesce(); + await framework.stop(); + await assert.rejects(framework.resume(), /^Error: resume refused: the framework is stopping, so the host stays quiesced$/); + assert.equal(await bootsQuiesced(), true, 'the host boots quiesced'); + }); + + it('stop() waits for a resume past its commit point, and a reopen that fails recycles no connection', async () => { + const framework = await create(); + const connection = { + id: 'srv', + recycled: 0, + pauseDataPlane(): void {}, + readyControlPlane(): void {}, + ready(): void {}, + async reconnectAfterFailure(): Promise { this.recycled++; }, + }; + const fw = internals(framework); + fw.mcplServerRegistry = { getAllServers: () => [connection], closeAll: async () => {} }; + framework.start(); + await framework.quiesce(); + // The reopen waits on the awareness barrier, which fails while stop() waits. + let failBarrier: ((error: Error) => void) | undefined; + fw.beginDiscordAwarenessBarrier = () => ({ + requiresBarrier: true, + promise: new Promise((_, reject) => { failBarrier = reject; }), + }); + const resuming = framework.resume().then((status) => { order.push('resume settled'); return status; }); + await until(() => failBarrier !== undefined, 'the resume to reach its reopen'); + const stopping = framework.stop(); + await releaseWhenStopWaits(() => failBarrier!(new Error('the awareness drain failed'))); + await stopping; + assert.deepEqual(order, ['resume settled', 'modules stopped']); + assert.equal((await resuming).quiesced, false, 'past its commit point the resume completes'); + assert.equal(connection.recycled, 0, 'a stopping host recycles no connection'); + assert.equal(await bootsQuiesced(), false, 'its durable clear landed'); + assert.deepEqual(rejections, []); + }); + + it("stop() waits for a host/command, so its answer goes out before the connections close", async () => { + const framework = await create(); + const connection = Object.assign(new EventEmitter(), { id: 'srv' }); + internals(framework).wireMcplEvents(connection); + const cm = framework.getAgent('agent')!.getContextManager(); + for (const n of [1, 2]) { + cm.addMessage('user', [{ type: 'text', text: `m${n}` }], { serverId: 'srv', channelId: 'discord:g1:c1', messageId: `d${n}` }); + } + // hide's answer carries a preview of what the agent sees now; hold it. + let release!: () => void; + const held = new Promise((resolve) => { release = resolve; }); + const preview = framework.previewActivation.bind(framework); + let previewing = false; + framework.previewActivation = (async (...args: Parameters) => { + previewing = true; + await held; + return preview(...args); + }) as AgentFramework['previewActivation']; + const answers: unknown[] = []; + connection.emit('host-command', { command: 'hide', agentName: 'agent', fromMessageId: 'd1' }, { + id: 7, + respond: (result: unknown) => { answers.push(result); order.push('host/command answered'); }, + respondError: (code: number, message: string) => { answers.push({ code, message }); order.push('host/command answered'); }, + }); + await until(() => previewing, 'hide to reach its preview'); + const stopping = framework.stop(); + await releaseWhenStopWaits(release); + await stopping; + assert.deepEqual(order, ['host/command answered', 'modules stopped']); + assert.equal((answers[0] as { ok?: boolean; hidden?: number }).ok, true, JSON.stringify(answers)); + assert.equal((answers[0] as { hidden?: number }).hidden, 1); + assert.deepEqual(rejections, []); + }); + + it('stop() waits for a store surgery to release the store', async () => { + const framework = await create(); + const cm = framework.getAgent('agent')!.getContextManager(); + const ids = [1, 2, 3].map((n) => cm.addMessage('user', [{ type: 'text', text: `m${n}` }])); + // The rollback syncs awareness markers while it holds the store; hold that. + let release!: () => void; + const held = new Promise((resolve) => { release = resolve; }); + let syncing = false; + framework.syncDiscordAwarenessMarkers = async () => { + syncing = true; + await held; + order.push('markers synced'); + }; + const rolling = framework.rollbackToMessage('agent', { messageId: ids[0]! }); + await until(() => syncing, 'the rollback to reach its marker sync'); + const stopping = framework.stop(); + await releaseWhenStopWaits(release); + await stopping; + assert.deepEqual(order, ['markers synced', 'modules stopped']); + assert.equal((await rolling).messagesRemoved, 2); + assert.deepEqual(rejections, []); + }); + + it("stop() waits for an awareness sync, which an operator's undo or branch switch runs", async () => { + const framework = await create(); + let deliver!: () => void; + internals(framework).beginDiscordAwarenessBarrier = () => ({ + requiresBarrier: true, + promise: new Promise((resolve) => { + deliver = () => resolve({ status: 'delivered', delivered: 1, failed: 0 }); + }), + }); + const syncing = framework.syncDiscordAwarenessMarkers().then(() => { order.push('awareness synced'); }); + await until(() => deliver !== undefined, 'the sync to wait on its barrier'); + const stopping = framework.stop(); + await releaseWhenStopWaits(() => deliver()); + await stopping; + await syncing; + assert.deepEqual(order, ['awareness synced', 'modules stopped']); + assert.deepEqual(rejections, []); + }); + + it("an operation that outlasts stop()'s budget fails the stop and keeps the store open for a retry", async () => { + const framework = await create(); + let deliver: (() => void) | undefined; + internals(framework).beginDiscordAwarenessBarrier = () => ({ + requiresBarrier: true, + promise: new Promise((resolve) => { + deliver = () => resolve({ status: 'delivered', delivered: 1, failed: 0 }); + }), + }); + const syncing = framework.syncDiscordAwarenessMarkers(); + await until(() => deliver !== undefined, 'the sync to wait on its barrier'); + await assert.rejects( + framework.stop(100), + (error: unknown) => error instanceof ShutdownTimeoutError + && error.pending.includes('awareness:sync') + && /^Shutdown timed out after 100 ms; pending: awareness:sync$/.test(error.message), + ); + assert.equal(framework.getStore().isClosed(), false, 'the operation may still write'); + deliver!(); + await syncing; + await framework.stop(); + assert.equal(framework.getStore().isClosed(), true); + assert.deepEqual(rejections, []); + }); +}); From 6759b9c4af96343c52edd80161ca7d6db219a38b Mon Sep 17 00:00:00 2001 From: Nell-1783 Date: Tue, 13 Oct 2026 08:41:46 -0400 Subject: [PATCH 02/21] test(discord-awareness-outbox): give the stub framework its operator-operations set The message-granular undo test builds its framework with Object.create(AgentFramework.prototype) and sets the fields the live rollback path uses. That path now counts its host/command and its store surgery as operations stop() waits for, so the stub needs the set they go into, as it already has the turn tokens and deferred writes the reservation uses. --- test/discord-awareness-outbox.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/discord-awareness-outbox.test.ts b/test/discord-awareness-outbox.test.ts index 4a35a21f5..707a10ab3 100644 --- a/test/discord-awareness-outbox.test.ts +++ b/test/discord-awareness-outbox.test.ts @@ -263,6 +263,8 @@ test('message-granular undo prepares markers before switching branches', async ( framework.nextTurnToken = 1; framework.deferredMessages = []; framework.pendingAssistantBlocks = new Map(); + // A host/command and a store surgery are operations stop() waits for. + framework.operatorOperations = new Set(); framework.operatorLog = new OperatorLog(undefined); const result = await framework.handleHostCommand('discord', { From db6033605e7632f45eaf93d1369e5d86ba164252 Mon Sep 17 00:00:00 2001 From: Sebastian-2380 Date: Tue, 13 Oct 2026 09:25:25 -0400 Subject: [PATCH 03/21] fix(lifecycle): close operator admission and drain work admitted before stop Refuse quiesce, resume, rollback, suppression, undo, redo, direct awareness sync, and MCPL lifecycle calls once the queue closes. Surgery checks precede validation and the catch that records an operator action, so a late request neither reads a closed store nor appends a failure record. Resume checks admission even when it would otherwise return an already-serving status, and retains the check after its awaited preparation. Register each operation before invoking its body in the same stack. The previous promise-argument form ran the body up to its first await before registration; an undo completion trace could synchronously start stop(), which then saw an empty set and tore down before the undo's held awareness sync answered. The regression observes this order through the real host-command listener. Drain the operation set until empty, with one monotonic deadline across rounds. Admitted work can start config materialization after the first snapshot. Undo, redo, rollback and suppression now share a tracked materialization helper while preserving fire-and-forget behavior, so a surgery releases its reservation without holding it through file IO. Already-admitted surgeries and host undo use the private awareness continuation, and an admitted restart uses private disconnect/connect continuations. A successful barrier retires without readying a data plane during stop; the existing failure path still records the failure while skipping recycling. Keep the explicit bounded-stop contract: when the operator wait times out, teardown proceeds while pending work may still act on torn-down components. The store stays open, another still-held retry can time out, and a retry after settlement can close it. Timeout diagnostics name current pending work and the original budget. Text now distinguishes cancelled streams from turns with no stream, drain interruption from abandon-grace interruption, and a single timeout from combined cleanup errors. Resume's stop-time log says parked wakes will not run. Replace the fixed-delay ordering release with observation of a nonempty wait or module teardown. An empty set is not evidence of waiting: treating it as one allowed the untracked MCPL and materialization cases to pass the first fixture. Add late-admission, continuation, reentrant-registration, successful-barrier, child-work and controlled-clock deadline cases. The constructor-bypassing MCPL reconnection fixture receives the operation set that real instances initialize. VPS checks before commit, with Bun's transpiler cache disabled: the six requested files passed 138 tests; MCPL lifecycle/reconnection/coalescing consumers passed 77. The expanded operator file passed 35 cases, and the final three-case deadline matrix passed after adding later-round timer expiry. Final frozen-tree, main-source red and removal receipts follow this commit. No native-platform or live-provider claim is made by these controlled fixtures. Co-authored-by: GPT-6 --- ...top-waits-for-operator-operations.fixed.md | 31 +- src/framework.ts | 164 +++++--- test/mcpl-reconnect-reregistration.test.ts | 2 + test/stop-with-host-operations.test.ts | 381 +++++++++++++++++- 4 files changed, 478 insertions(+), 100 deletions(-) diff --git a/changelog.d/stop-waits-for-operator-operations.fixed.md b/changelog.d/stop-waits-for-operator-operations.fixed.md index af6bf5d3d..7a5a0aa8e 100644 --- a/changelog.d/stop-waits-for-operator-operations.fixed.md +++ b/changelog.d/stop-waits-for-operator-operations.fixed.md @@ -1,24 +1,7 @@ -- **`stop()` waits for an operator's operation that is already running before - it tears down what that operation uses.** A quiesce, a resume, a - `host/command`, a store surgery (`rollbackToMessage`, `suppressMessages`) - or an awareness sync (`syncDiscordAwarenessMarkers`) admitted before - `stop()` began now finishes before the event gate, the modules, the MCPL - connections and the store go. Before, such an operation kept running - through the teardown: a resume could clear the durable quiesce mark and - reopen the data planes after the final sync, a surgery could find its - store closed partway, and a `host/command` answer could miss its - connection. - - A quiesce stops waiting for its drain, or for an abandoned turn's grace, - once `stop()` begins. `stop()` ends the turns itself. The host stays - quiesced, so it boots quiesced. - - A resume that hasn't left quiesce when `stop()` begins is refused with - an error, and the host stays quiesced. One that has left it finishes, - its durable clear included, before the final sync. - - `quiesce()` and `resume()` called once `stop()` has begun are refused - with an error, as a `host/command` already was. - - A failed awareness drain no longer recycles the MCPL connections of a - host that is stopping, since `stop()` is closing them. - - **For operators:** `stop(teardownTimeoutMs)` bounds this wait too. An - operation that outlasts it fails `stop()` with a `ShutdownTimeoutError` - that names it, and the store stays open, as for a module whose teardown - timed out, so `stop()` can be called again. +- **`stop()` waits for admitted operator work before teardown, subject to its optional wait budget.** Quiesce, resume, each `host/command`, store surgeries, awareness syncs, MCPL connect/disconnect/restart calls, and config materializations are tracked. When the wait completes, their work finishes before the event gate, modules, MCPL connections, final sync, and store close. Previously, an operation could keep writing or open a connection after teardown. + - A quiesce stops waiting for its drain or abandon grace when stop begins. Stop cancels streams and waits for the event loop; a turn with a token but no stream settles on its own. The host stays durably quiesced. A drain cut short returns its current status (possibly `drained: false`), logs the stopping message, and omits the `host:quiesce` trace. If abandon was requested but had not begun, stream cancellation is left to stop. An abandon whose grace is cut short still reports the abandon through its usual trace and log. + - A resume refuses at entry once the queue is closed, even on an unquiesced host. An admitted resume checks again after its maintenance and preview waits, before leaving quiesce. A resume past that commit completes its flush and durable clear. Its log names parked wakes that will not run during stop. + - The public quiesce, resume, rollback, suppression, undo, redo, awareness-sync, and MCPL lifecycle entry points refuse after queue closure, before reads or mutations, with ` refused: the framework is stopping`. Already-admitted operations use private continuations for their remaining work. Registration precedes the body's synchronous callbacks, so a trace listener can reenter stop without escaping its wait. + - Undo, redo, rollback, and suppression still start config materialization without awaiting file IO. Stop waits for that work too, including a materialization started by a surgery while stop is already waiting. One deadline covers every round of that operator wait. + - The MCPL barrier funnel applies the stop boundary to every completion path. A failed barrier recycles no connection during stop. A successful barrier is retired but readies no data plane. + - **For operators:** an admitted awareness drain finishes its deliveries, each within its own deadline. `stop(teardownTimeoutMs)` also bounds the operator wait. On timeout, teardown proceeds while the operation may still act on torn-down components; the store stays open for another stop attempt. The timeout is a `ShutdownTimeoutError` naming pending work, either alone or inside an `AggregateError` if other cleanup failed. A retry while the work remains pending can time out again; a retry after it settles can close the store. diff --git a/src/framework.ts b/src/framework.ts index 223dc3264..f7ecacc16 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -61,7 +61,7 @@ import type { SameRoundThinkTextPolicy, } from './types/index.js'; import { ProcessQueueImpl } from './queue.js'; -import { DEFAULT_SHUTDOWN_TIMEOUT_MS, validateShutdownTimeout, shutdownErrorMessage, waitForShutdown } from './shutdown.js'; +import { DEFAULT_SHUTDOWN_TIMEOUT_MS, validateShutdownTimeout, shutdownErrorMessage, waitForShutdown, ShutdownTimeoutError } from './shutdown.js'; import { REFUSAL_REACTIONS, REFUSAL_REACTION_FALLBACK } from './refusal-reactions.js'; import { Agent } from './agent.js'; import { ModuleRegistry, isStateExistsError } from './module-registry.js'; @@ -1138,9 +1138,8 @@ export class AgentFramework { private stopPromise: Promise | null = null; private stopCompleted = false; private ownedStoreClosed = false; - /** An operator's operations in flight, which stop() waits for before it - * tears down what they use: quiesce(), resume(), each host/command, a - * store surgery until it releases the store, and an awareness sync. */ + /** Admitted operator work, including child config materializations. + * stop() drains this set before teardown, unless its wait budget expires. */ private readonly operatorOperations = new Set<{ label: string; promise: Promise }>(); private membrane: Membrane; private queue: ProcessQueueImpl; @@ -2134,8 +2133,9 @@ export class AgentFramework { } /** - * Stop the event loop. Failed module/MCPL cleanup can be retried with its - * original context; storage stays open until that cleanup succeeds. + * Stop the event loop. Failed operator/module/MCPL waits can be retried; + * storage stays open. A wait timeout permits teardown to continue while + * that work may still act on the components being torn down. * @param teardownTimeoutMs Optional budget for each wait on cleanup: an * operator's operations already running, then modules and MCPL together. * Omit to wait until cleanup settles, as required by existing module/child @@ -2165,15 +2165,45 @@ export class AgentFramework { } } - /** Count an operator's operation as in flight until it settles, so that - * stop() can wait for it (see stopOwned). */ - private trackOperatorOperation(label: string, operation: Promise): Promise { + /** Register before invoking the body: its synchronous trace callbacks can + * reenter stop(). Invocation stays in this stack, preserving immediate + * mode changes; the returned promise carries its result to the caller. */ + private trackOperatorOperation(label: string, run: () => Promise): Promise { + let resolve!: (value: T | PromiseLike) => void; + let reject!: (error: unknown) => void; + const operation = new Promise((yes, no) => { resolve = yes; reject = no; }); const entry = { label, promise: operation.then(() => {}, () => {}) }; this.operatorOperations.add(entry); void entry.promise.then(() => { this.operatorOperations.delete(entry); }); + try { resolve(run()); } catch (error) { reject(error); } return operation; } + private refuseOperatorAfterStop(verb: string): void { + if (this.queue?.isClosed) throw new Error(verb + ' refused: the framework is stopping'); + } + + /** Admission is closed, but admitted work can still start child work. + * All rounds share the caller's deadline rather than renewing its budget. */ + private async waitForOperatorOperations(timeoutMs: number | undefined): Promise { + const deadline = timeoutMs === undefined ? undefined : performance.now() + timeoutMs; + while (this.operatorOperations.size > 0) { + const remaining = deadline === undefined ? undefined : Math.ceil(deadline - performance.now()); + const timedOut = () => new ShutdownTimeoutError( + [...this.operatorOperations].map(({ label }) => label), timeoutMs!, [], + ); + if (remaining !== undefined && remaining < 1) throw timedOut(); + try { + await waitForShutdown([...this.operatorOperations], remaining); + } catch (error) { + // Report the original budget and everything still pending, including + // child work added after this round's snapshot. + if (error instanceof ShutdownTimeoutError) throw timedOut(); + throw error; + } + } + } + private async stopOwned(teardownTimeoutMs: number | undefined): Promise { this.pushCoalescer?.suspend(); this.flushCoalescingSnapshot(); @@ -2245,16 +2275,15 @@ export class AgentFramework { await this.loopPromise; } - // An operator's operation admitted before stop() began (a quiesce - // draining, a resume, an undo) finishes before what it uses is torn down: - // the event gate, the MCPL connections and the store. Once the queue is - // closed, a quiesce stops waiting for its drain and a resume that hasn't - // left quiesce refuses; the rest finish their work. One that outlasts the - // budget may still write, so the store stays open, as it does for a - // module whose teardown timed out. + // Admission closed with the queue. Only admitted operations can add + // work now (for example a surgery's config materialization), so drain + // until the set is empty. This follows stream cancellation and the event + // loop, allowing a quiesce or surgery waiting on them to settle. + // If the budget expires, teardown proceeds while the operation may still + // act on torn-down components. Keep the store open for a later stop(). let operatorOperationsTimedOut: unknown; try { - await waitForShutdown([...this.operatorOperations], teardownTimeoutMs); + await this.waitForOperatorOperations(teardownTimeoutMs); } catch (error) { operatorOperationsTimedOut = error; } @@ -3396,12 +3425,13 @@ export class AgentFramework { * dedicated cancel kind that settles the turn without feeding the * inference-failure accounting. */ - quiesce(opts?: { + async quiesce(opts?: { reason?: string; timeoutMs?: number; abandon?: boolean; }): Promise { - return this.trackOperatorOperation('host:quiesce', this.runQuiesce(opts)); + this.refuseOperatorAfterStop('quiesce'); + return this.trackOperatorOperation('host:quiesce', () => this.runQuiesce(opts)); } private async runQuiesce(opts?: { @@ -3409,8 +3439,6 @@ export class AgentFramework { timeoutMs?: number; abandon?: boolean; }): Promise { - // stop() closes the queue first: a stopping host takes no host command. - if (this.queue?.isClosed) throw new Error('quiesce refused: the framework is stopping'); if (this.quiesced) { // Idempotent — EXCEPT abandon escalation: a first quiesce that timed // out undrained must be escalatable with a second quiesce({abandon}) @@ -3480,9 +3508,9 @@ export class AgentFramework { console.error('[host-mode] quiesce superseded by a concurrent resume during drain'); return this.getHostModeStatus(); } - // stop() began while we waited. It ends every turn itself, and it waits - // for this call before it tears down. The host stays quiesced, so it - // boots quiesced. + // Stop cuts this drain short, including any requested abandon. It cancels + // streams and waits for the event loop; a token with no stream settles + // on its own. Return the status as it stands and keep the durable quiesce. if (this.queue?.isClosed) { console.error('[host-mode] quiesce stopped waiting for its drain: the framework is stopping'); return this.getHostModeStatus(); @@ -3546,8 +3574,9 @@ export class AgentFramework { ); } } - // Bounded grace for the cancelled streams' teardown to settle. A stopping - // framework ends the turns itself and waits for this call, so stop waiting. + // Bounded grace for cancelled streams. Stop cancels streams too; a turn + // with no stream settles on its own. Report the abandon that already ran, + // even if stop cuts its grace short. const grace = Date.now() + 10_000; while (this.quiesced && !this.queue?.isClosed && this.activeTurnTokens.size > 0 && Date.now() < grace) { await new Promise((resolve) => setTimeout(resolve, 250)); @@ -3565,8 +3594,9 @@ export class AgentFramework { * Unavailable previews warn and pass — refusing to resume because the * strategy cannot preview would hold hosts hostage to a diagnostic. */ - resume(opts?: { force?: boolean }): Promise { - return this.trackOperatorOperation('host:resume', this.runResume(opts)); + async resume(opts?: { force?: boolean }): Promise { + this.refuseOperatorAfterStop('resume'); + return this.trackOperatorOperation('host:resume', () => this.runResume(opts)); } private async runResume(opts?: { force?: boolean }): Promise { @@ -3657,9 +3687,9 @@ export class AgentFramework { await barrier.promise; this.completeMcplDataPlaneGate(barrier); } catch (error) { - // The host IS resumed — don't rethrow. failMcplDataPlaneGate - // recycles the connections, and their reconnect flows re-run the - // funnel with quiesced=false, self-healing the data planes. + // The host has resumed. Report the failed barrier without undoing + // that commit. The funnel recycles while serving; during stop it + // neither recycles on failure nor readies data planes on success. await this.failMcplDataPlaneGate(barrier, 'quiesce resume', error); } } @@ -3673,7 +3703,9 @@ export class AgentFramework { // mid-quiesce boots quiesced but with these queues empty (sources like // heartbeats re-deliver on their own cadence; one-shot wakes are lost). // Deferred context WRITES are persisted while quiesced and survive. - console.error(`[host-mode] resumed — ${releasedRequests} parked wake(s) released`); + console.error(this.queue?.isClosed + ? `[host-mode] resumed — ${releasedRequests} parked wake(s) won't run because the framework is stopping` + : `[host-mode] resumed — ${releasedRequests} parked wake(s) released`); } return this.getHostModeStatus(); } @@ -5453,7 +5485,7 @@ export class AgentFramework { serverId: string, params: HostCommandParams, ): ReturnType { - return this.trackOperatorOperation(`host/command:${String(params.command)}`, this.runHostCommand(serverId, params)); + return this.trackOperatorOperation(`host/command:${String(params.command)}`, () => this.runHostCommand(serverId, params)); } /** handleHostCommand's work, run where stop() can wait for it. */ @@ -5754,7 +5786,7 @@ export class AgentFramework { return { ok: true, undone: 0, requested, lastVisible: null }; } - await this.syncDiscordAwarenessMarkers(); + await this.runDiscordAwarenessSync(); return { ok: true, undone, requested, lastVisible: await this.lastVisiblePreview(agentName) }; } @@ -6077,6 +6109,7 @@ export class AgentFramework { fromBranch?: string; toBranch?: string; } { + this.refuseOperatorAfterStop('undo'); const agent = this.agents.get(agentName); if (!agent) { throw new Error(`Unknown agent: ${agentName}`); @@ -6099,11 +6132,7 @@ export class AgentFramework { this.store.createBranchAt(undoBranchName, currentBranch.name, checkpoint.sequenceBefore); this.store.switchBranch(undoBranchName); - // Materialize config files from the new branch (fire-and-forget; gate picks up via mtime) - const wsUndo = this.moduleRegistry.getModule('workspace'); - if (wsUndo && 'materializeMount' in wsUndo) { - (wsUndo as any).materializeMount('_config').catch(() => {}); - } + this.materializeConfigMountAfterBranchSwitch(); // Push onto redo stack let redoStack = this.redoStacks.get(agentName); @@ -6147,6 +6176,7 @@ export class AgentFramework { fromBranch?: string; toBranch?: string; } { + this.refuseOperatorAfterStop('redo'); const agent = this.agents.get(agentName); if (!agent) { throw new Error(`Unknown agent: ${agentName}`); @@ -6165,11 +6195,7 @@ export class AgentFramework { this.store.switchBranch(branchName); - // Materialize config files from the restored branch (fire-and-forget; gate picks up via mtime) - const wsRedo = this.moduleRegistry.getModule('workspace'); - if (wsRedo && 'materializeMount' in wsRedo) { - (wsRedo as any).materializeMount('_config').catch(() => {}); - } + this.materializeConfigMountAfterBranchSwitch(); // Restore the checkpoint const checkpoints = this.getTurnCheckpoints(agentName); @@ -6294,7 +6320,7 @@ export class AgentFramework { // stop() waits for the release (see stopOwned): a surgery admitted before // stop() began finishes, flush included, before the store closes. let settle!: () => void; - void this.trackOperatorOperation(`surgery:${verb}`, new Promise((resolve) => { settle = resolve; })); + void this.trackOperatorOperation(`surgery:${verb}`, () => new Promise((resolve) => { settle = resolve; })); let released = false; return () => { if (released) return; @@ -6379,6 +6405,7 @@ export class AgentFramework { removedRefs: Array<{ serverId: string; channelId: string; messageId: string }>; lastVisible: { participant?: string; role?: string; preview?: string } | null; }> { + this.refuseOperatorAfterStop('roll back'); const logBase: OperatorLogInput = { kind: 'rollback', agent: agentName, @@ -6448,7 +6475,7 @@ export class AgentFramework { ); } if (markerBatch) this.discordAwarenessOutbox!.activate(markerBatch.id); - await this.syncDiscordAwarenessMarkers(); + await this.runDiscordAwarenessSync(); this.materializeConfigMountAfterBranchSwitch(); } finally { release(); @@ -6514,6 +6541,7 @@ export class AgentFramework { removedRefs: Array<{ serverId: string; channelId: string; messageId: string }>; lastVisible: { participant?: string; role?: string; preview?: string } | null; }> { + this.refuseOperatorAfterStop('suppress'); const requestedIds = [...new Set(opts.messageIds.map(String))]; const logBase: OperatorLogInput = { kind: 'suppress', @@ -6620,7 +6648,7 @@ export class AgentFramework { { cause: error }, ); } - await this.syncDiscordAwarenessMarkers(); + await this.runDiscordAwarenessSync(); this.materializeConfigMountAfterBranchSwitch(); } finally { release(); @@ -6657,14 +6685,14 @@ export class AgentFramework { } } - /** Re-materialize config files from the (new) active branch — fire and - * forget; the gate picks changes up via mtime. Mirrors undoLastTurn. */ + /** Start config file IO without holding the caller or a surgery's store + * reservation. The gate picks it up by mtime; stop() still waits for it. */ private materializeConfigMountAfterBranchSwitch(): void { const ws = this.moduleRegistry.getModule('workspace'); if (ws && 'materializeMount' in ws) { - (ws as { materializeMount: (m: string) => Promise }) - .materializeMount('_config') - .catch(() => {}); + void this.trackOperatorOperation('config:materialize', + () => (ws as { materializeMount: (m: string) => Promise }).materializeMount('_config'), + ).catch(() => {}); } } @@ -13960,8 +13988,9 @@ export class AgentFramework { } /** Reconcile the durable ledger with Chronicle, then deliver every server's work. */ - syncDiscordAwarenessMarkers(onlyServerId?: string): Promise { - return this.trackOperatorOperation('awareness:sync', this.runDiscordAwarenessSync(onlyServerId)); + async syncDiscordAwarenessMarkers(onlyServerId?: string): Promise { + this.refuseOperatorAfterStop('awareness sync'); + return this.trackOperatorOperation('awareness:sync', () => this.runDiscordAwarenessSync(onlyServerId)); } private async runDiscordAwarenessSync(_onlyServerId?: string): Promise { @@ -14264,6 +14293,9 @@ export class AgentFramework { // check sits AFTER this guard: readyControlPlane can also synchronously // flush a control event that installs a newer barrier.) if (this.discordAwarenessBarrier !== null) return false; + // This generation is retired above, but a stopping host reopens no + // data plane. Recheck for each connection: a ready() can reenter stop(). + if (this.queue?.isClosed) return true; // Host quiesce (issue #122): this is the single point every barrier // completion and reconnect flow funnels through, so holding here keeps // data planes paused across awareness drains and reconnects for the @@ -14712,6 +14744,13 @@ export class AgentFramework { */ async connectMcplServer( config: import('./mcpl/types.js').McplServerConfig, + ): Promise { + this.refuseOperatorAfterStop('connect MCPL server'); + return this.trackOperatorOperation('mcpl:connect', () => this.runConnectMcplServer(config)); + } + + private async runConnectMcplServer( + config: import('./mcpl/types.js').McplServerConfig, ): Promise { // Lazily bring up the MCPL subsystem — a framework that started with zero // configured servers can still deploy its first one at runtime. @@ -14748,6 +14787,11 @@ export class AgentFramework { * No-op-ish if the server is not connected (still clears routing state). */ async disconnectMcplServer(id: string): Promise { + this.refuseOperatorAfterStop('disconnect MCPL server'); + return this.trackOperatorOperation('mcpl:disconnect', () => this.runDisconnectMcplServer(id)); + } + + private async runDisconnectMcplServer(id: string): Promise { if (!this.mcplServerRegistry) { throw new Error('MCPL subsystem is not initialized'); } @@ -14781,13 +14825,21 @@ export class AgentFramework { async restartMcplServer( id: string, newConfig?: import('./mcpl/types.js').McplServerConfig, + ): Promise { + this.refuseOperatorAfterStop('restart MCPL server'); + return this.trackOperatorOperation('mcpl:restart', () => this.runRestartMcplServer(id, newConfig)); + } + + private async runRestartMcplServer( + id: string, + newConfig?: import('./mcpl/types.js').McplServerConfig, ): Promise { const config = newConfig ?? this.mcplServerConfigs.get(id); if (!config) { throw new Error(`MCPL server "${id}" is not configured`); } - await this.disconnectMcplServer(id); - await this.connectMcplServer(config); + await this.runDisconnectMcplServer(id); + await this.runConnectMcplServer(config); } /** diff --git a/test/mcpl-reconnect-reregistration.test.ts b/test/mcpl-reconnect-reregistration.test.ts index a7627a77f..2484f610a 100644 --- a/test/mcpl-reconnect-reregistration.test.ts +++ b/test/mcpl-reconnect-reregistration.test.ts @@ -98,6 +98,8 @@ async function makeHarness() { const pushed: unknown[] = []; const fw = Object.create(AgentFramework.prototype) as any; + // Public lifecycle calls use the constructor's in-flight operation set. + fw.operatorOperations = new Set(); fw.traceListeners = []; fw.consecutiveInferenceFailures = new Map(); fw.exhaustionRewinds = new Map(); diff --git a/test/stop-with-host-operations.test.ts b/test/stop-with-host-operations.test.ts index 9f5366217..bebcf7b27 100644 --- a/test/stop-with-host-operations.test.ts +++ b/test/stop-with-host-operations.test.ts @@ -4,20 +4,20 @@ * stop() refuses a host/command that arrives once it has begun. These tests * cover the operations admitted before that: a quiesce draining, a resume, a * host/command, a store surgery and an awareness sync, whether they came over - * MCPL or from a direct call. stop() waits for each before it tears down what - * it uses: the event gate, the modules, the MCPL connections and the store. - * A quiesce stops waiting for its drain, and a resume that hasn't committed - * refuses, so the wait stays short and the host boots in the mode it had. + * MCPL or from a direct call. A completed wait precedes the event gate, + * modules, MCPL connections and store teardown; an explicit budget can cut + * the wait short and leave the store open. Quiesce releases its drain, and + * a resume that has not committed preserves the durable quiesce. * * Where a test holds an operation open, it lets it go once stop() reaches its - * modules' teardown, or after a short while if stop() is waiting short of it. + * modules' teardown or reads a nonempty operations set to wait for them. * The order of the two events is the verdict, so it doesn't depend on timing. */ import { describe, it, beforeEach, afterEach } from 'node:test'; import assert from 'node:assert/strict'; import { EventEmitter } from 'node:events'; -import { mkdtempSync, rmSync } from 'node:fs'; +import { existsSync, mkdtempSync, readFileSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import type { EventResponse, Module, ModuleContext, ToolDefinition, ToolResult } from '../src/index.js'; @@ -51,6 +51,10 @@ interface Internals { mcplServerRegistry: unknown; beginDiscordAwarenessBarrier: () => { requiresBarrier: boolean; promise: Promise }; wireMcplEvents(connection: unknown): void; + operatorOperations: Set<{ label: string; promise: Promise }>; + moduleRegistry: { getModule(name: string): unknown }; + saveTurnCheckpoints(name: string, checkpoints: unknown[]): void; + discordAwarenessBarrier: unknown; } const internals = (framework: AgentFramework): Internals => framework as unknown as Internals; @@ -60,6 +64,7 @@ describe('stop() with an operator operation in flight', () => { let logged: string[]; let rejections: unknown[]; let order: string[]; + let waitReads: number; const onRejection = (reason: unknown): void => { rejections.push(reason); }; const consoleError = console.error; const consoleWarn = console.warn; @@ -70,6 +75,7 @@ describe('stop() with an operator operation in flight', () => { logged = []; rejections = []; order = []; + waitReads = 0; process.on('unhandledRejection', onRejection); console.error = (...args: unknown[]) => { logged.push(args.map(String).join(' ')); }; console.warn = (...args: unknown[]) => { logged.push(args.map(String).join(' ')); }; @@ -82,22 +88,29 @@ describe('stop() with an operator operation in flight', () => { rmSync(tempDir, { recursive: true, force: true }); }); - function create(): Promise { - return AgentFramework.create({ + async function create(): Promise { + const framework = await AgentFramework.create({ storePath, membrane: new MockMembrane().asMembrane(), agents: [{ name: 'agent', model: 'test-model', systemPrompt: 'test' }], modules: [new TeardownProbe(order)], }); + internals(framework).operatorOperations = new class extends Set<{ label: string; promise: Promise }> { + override [Symbol.iterator](): SetIterator<{ label: string; promise: Promise }> { + if (this.size > 0) waitReads++; + return super[Symbol.iterator](); + } + }(); + return framework; } - /** Let the held operation go once stop() reaches its modules, or after 300 ms if it waits short of them. */ + /** A positive observation releases the hold. Main reaches module teardown + * without reading the set, making the wrong order observable too. */ async function releaseWhenStopWaits(release: () => void): Promise { - const deadline = Date.now() + 300; - while (!order.includes('modules stopped') && Date.now() < deadline) { - await new Promise((r) => setTimeout(r, 5)); - } - release(); + try { + await until(() => waitReads > 0 || order.includes('modules stopped'), + 'stop to read pending operations or stop its modules', 30_000); + } finally { release(); } } async function bootsQuiesced(): Promise { @@ -113,10 +126,12 @@ describe('stop() with an operator operation in flight', () => { const framework = await create(); framework.start(); // A turn between dequeue and stream registration holds its token with no - // stream to cancel, so nothing but stop() can end this drain. + // stream to cancel. Stop cuts the drain short; the token remains alive. const tokens = internals(framework).activeTurnTokens; tokens.set('agent', -1); let settled = false; + const traces: string[] = []; + framework.onTrace((event) => { traces.push(event.type); }); const quiescing = framework.quiesce({ reason: 'surgery', timeoutMs: 600_000 }); void quiescing.then(() => { settled = true; }, () => { settled = true; }); try { @@ -125,6 +140,7 @@ describe('stop() with an operator operation in flight', () => { const status = await quiescing; assert.equal(status.quiesced, true); assert.equal(status.drained, false); + assert.ok(!traces.includes('host:quiesce'), 'an interrupted drain reports no completed quiesce'); assert.ok(logged.some((line) => line.includes('quiesce stopped waiting for its drain: the framework is stopping')), logged.join('\n')); } finally { tokens.delete('agent'); @@ -150,7 +166,8 @@ describe('stop() with an operator operation in flight', () => { await framework.stop(); const took = Date.now() - began; assert.equal(settled, true, 'stop() returned while the abandon was still waiting'); - assert.ok(took < 5_000, `stop() took ${took} ms, waiting out the abandon's ten seconds`); + // Only waiting out the ten-second grace should consume this bound. + assert.ok(took < 8_000, `stop() took ${took} ms, waiting out the abandon's ten seconds`); assert.deepEqual((await quiescing).unabandonable, ['agent']); } finally { tokens.delete('agent'); @@ -187,7 +204,7 @@ describe('stop() with an operator operation in flight', () => { const framework = await create(); await framework.quiesce(); await framework.stop(); - await assert.rejects(framework.resume(), /^Error: resume refused: the framework is stopping, so the host stays quiesced$/); + await assert.rejects(framework.resume(), /^Error: resume refused: the framework is stopping$/); assert.equal(await bootsQuiesced(), true, 'the host boots quiesced'); }); @@ -265,10 +282,12 @@ describe('stop() with an operator operation in flight', () => { let release!: () => void; const held = new Promise((resolve) => { release = resolve; }); let syncing = false; - framework.syncDiscordAwarenessMarkers = async () => { + internals(framework).beginDiscordAwarenessBarrier = () => { syncing = true; - await held; - order.push('markers synced'); + return { requiresBarrier: true, promise: held.then(() => { + order.push('markers synced'); + return { status: 'delivered', delivered: 0, failed: 0 }; + }) }; }; const rolling = framework.rollbackToMessage('agent', { messageId: ids[0]! }); await until(() => syncing, 'the rollback to reach its marker sync'); @@ -317,10 +336,332 @@ describe('stop() with an operator operation in flight', () => { && /^Shutdown timed out after 100 ms; pending: awareness:sync$/.test(error.message), ); assert.equal(framework.getStore().isClosed(), false, 'the operation may still write'); + assert.ok(order.includes('modules stopped'), 'timeout still proceeds with teardown'); + await assert.rejects(framework.stop(100), (error: unknown) => + error instanceof ShutdownTimeoutError && error.pending.includes('awareness:sync')); + assert.equal(framework.getStore().isClosed(), false, 'a still-running retry keeps the store open too'); deliver!(); await syncing; await framework.stop(); assert.equal(framework.getStore().isClosed(), true); assert.deepEqual(rejections, []); }); + const lateCalls: Array<[string, (framework: AgentFramework) => unknown]> = [ + ['quiesce', (f) => f.quiesce()], + ['resume', (f) => f.resume()], + ['roll back', (f) => f.rollbackToMessage('agent', { messageId: 'invalid' })], + ['suppress', (f) => f.suppressMessages('agent', { messageIds: ['invalid'] })], + ['undo', (f) => f.undoLastTurn('agent')], + ['redo', (f) => f.redo('agent')], + ['awareness sync', (f) => f.syncDiscordAwarenessMarkers()], + ['connect MCPL server', (f) => f.connectMcplServer({ id: 'late', command: 'unused' })], + ['disconnect MCPL server', (f) => f.disconnectMcplServer('late')], + ['restart MCPL server', (f) => f.restartMcplServer('late')], + ]; + for (const [verb, call] of lateCalls) { + it('round: ' + verb + ' refuses during teardown and after stop, without writes or operator logs', async () => { + const framework = await create(); + let backendCalls = 0; + Object.assign(internals(framework), { + mcplHostCapabilities: {}, + mcplServerRegistry: { + getAllServers: () => [], closeAll: async () => {}, + removeServer: async () => { backendCalls++; }, + }, + connectMcplServerInternal: async () => { backendCalls++; }, + refreshMcplTools: async () => { backendCalls++; }, + emitMcplToolDiff: () => { backendCalls++; }, + }); + let finishModules!: () => void; + const hold = new Promise((resolve) => { finishModules = resolve; }); + framework.getModule('teardown-probe')!.stop = async () => { + order.push('modules stopped'); + await hold; + }; + const stopping = framework.stop(); + await until(() => order.includes('modules stopped'), 'module teardown'); + const sequence = framework.getStore().currentSequence(); + const branch = framework.getStore().currentBranch().name; + const logPath = framework.getOperatorLogPath()!; + const log = () => existsSync(logPath) ? readFileSync(logPath, 'utf8') : ''; + const beforeLog = log(); + const refused = async () => assert.rejects( + Promise.resolve().then(() => call(framework)), + (error: unknown) => error instanceof Error && error.message === verb + ' refused: the framework is stopping', + ); + try { + await refused(); + assert.equal(framework.getStore().currentSequence(), sequence); + assert.equal(framework.getStore().currentBranch().name, branch); + assert.equal(log(), beforeLog); + assert.equal(backendCalls, 0); + } finally { finishModules(); await stopping; } + await refused(); + assert.equal(log(), beforeLog); + }); + } + + it('round: resume admitted after closure refuses before a held maintenance pass ends', async () => { + const framework = await create(); + await framework.quiesce(); + let endPass!: () => void; + internals(framework).maintenancePass = new Promise((resolve) => { endPass = resolve; }); + const stopping = framework.stop(); + let settled = false; + const resuming = framework.resume().finally(() => { settled = true; }); + void resuming.catch(() => {}); + try { + // Admission is synchronous; promise settlement needs only its microtasks. + await Promise.resolve(); + await Promise.resolve(); + assert.equal(settled, true, 'refusal must not wait for maintenance'); + await assert.rejects(resuming, /resume refused: the framework is stopping/); + } finally { endPass(); await stopping; await resuming.catch(() => {}); } + }); + + it('round: resume rechecks closure after preview retries', async () => { + const framework = await create(); + await framework.quiesce(); + let previewed = false; + framework.previewAgentRuntimeSettings = (() => { + previewed = true; + return { available: false, reason: 'preview_in_flight', path: 'none', liveBudgetTokens: 0, transition: 'stable' }; + }) as typeof framework.previewAgentRuntimeSettings; + const resuming = framework.resume(); + void resuming.catch(() => {}); + assert.equal(previewed, true); + const stopping = framework.stop(); + await assert.rejects(resuming, /resume refused: the framework is stopping/); + await stopping; + assert.equal(await bootsQuiesced(), true); + }); + + it('round: a successful resume barrier across stop retires without readying a data plane', async () => { + const framework = await create(); + let ready = 0; + const connection = { + id: 'srv', pauseDataPlane() {}, readyControlPlane() {}, + ready() { ready++; }, + }; + const fw = internals(framework); + fw.mcplServerRegistry = { getAllServers: () => [connection], closeAll: async () => {} }; + await framework.quiesce(); + let deliver!: () => void; + fw.beginDiscordAwarenessBarrier = () => ({ + requiresBarrier: true, + promise: new Promise((resolve) => { deliver = () => resolve({ status: 'delivered', delivered: 0, failed: 0 }); }), + }); + const resuming = framework.resume(); + await until(() => deliver !== undefined, 'resume barrier'); + const stopping = framework.stop(); + await releaseWhenStopWaits(deliver); + await resuming; + await stopping; + assert.equal(ready, 0); + assert.equal(fw.discordAwarenessBarrier, null); + assert.ok(logged.some((line) => line.includes("parked wake(s) won't run") && line.includes('stopping'))); + assert.equal(await bootsQuiesced(), false); + }); + + for (const action of ['connect', 'disconnect', 'restart'] as const) { + it('round: an admitted MCPL ' + action + ' settles before connection teardown', async () => { + const framework = await create(); + const fw = framework as unknown as Internals & { + mcplHostCapabilities: object; + mcplServerConfigs: Map; + connectMcplServerInternal(config: unknown): Promise; + refreshMcplTools(): Promise; + emitMcplToolDiff(...args: unknown[]): void; + }; + let release!: () => void; + const held = new Promise((resolve) => { release = resolve; }); + let entered = false; + let connected = action !== 'connect'; + fw.mcplHostCapabilities = {}; + fw.mcplServerConfigs.set('srv', { id: 'srv', command: 'unused' }); + fw.connectMcplServerInternal = async () => { + entered = true; + await held; + connected = true; + order.push('connection opened'); + }; + fw.refreshMcplTools = async () => {}; + fw.emitMcplToolDiff = () => {}; + fw.mcplServerRegistry = { + getAllServers: () => [], + removeServer: async () => { + entered = true; + await held; + connected = false; + order.push('connection removed'); + }, + closeAll: async () => { connected = false; order.push('connections closed'); }, + }; + const running = action === 'connect' ? framework.connectMcplServer({ id: 'srv', command: 'unused' }) + : action === 'disconnect' ? framework.disconnectMcplServer('srv') : framework.restartMcplServer('srv'); + void running.catch(() => {}); + await until(() => entered, 'MCPL operation'); + const stopping = framework.stop(); + await releaseWhenStopWaits(release); + await running; + await stopping; + assert.equal(connected, false); + assert.ok(order.indexOf(action === 'disconnect' ? 'connection removed' : 'connection opened') < order.indexOf('modules stopped'), order.join(', ')); + }); + } + + for (const action of ['undo', 'redo', 'rollback', 'suppress'] as const) { + it('round: ' + action + ' config materialization stays fire-and-forget but finishes before teardown', async () => { + const framework = await create(); + const fw = internals(framework); + const cm = framework.getAgent('agent')!.getContextManager(); + const sequenceBefore = framework.getStore().currentSequence(); + const ids = [1, 2, 3].map((n) => cm.addMessage('user', [{ type: 'text', text: 'm' + n }])); + fw.saveTurnCheckpoints('agent', [{ agentName: 'agent', turnIndex: 1, sequenceBefore, branchName: 'main', timestamp: Date.now() }]); + if (action === 'redo') framework.undoLastTurn('agent'); + let release!: () => void; + const held = new Promise((resolve) => { release = resolve; }); + let materializing = false; + const getModule = fw.moduleRegistry.getModule.bind(fw.moduleRegistry); + fw.moduleRegistry.getModule = (name) => name === 'workspace' ? { + materializeMount: async () => { materializing = true; await held; order.push('config materialized'); }, + } : getModule(name); + if (action === 'undo') framework.undoLastTurn('agent'); + else if (action === 'redo') framework.redo('agent'); + else if (action === 'rollback') await framework.rollbackToMessage('agent', { messageId: ids[0]! }); + else await framework.suppressMessages('agent', { messageIds: [ids[1]!] }); + assert.equal(materializing, true, 'the caller returned before file IO settled'); + const stopping = framework.stop(); + await releaseWhenStopWaits(release); + await stopping; + assert.ok(order.indexOf('config materialized') < order.indexOf('modules stopped'), order.join(', ')); + }); + } + + for (const action of ['rollback', 'suppress'] as const) it('round: stop also waits for materialization an admitted ' + action + ' starts after its first snapshot', async () => { + const framework = await create(); + const fw = internals(framework); + const cm = framework.getAgent('agent')!.getContextManager(); + const ids = [1, 2, 3].map((n) => cm.addMessage('user', [{ type: 'text', text: 'm' + n }])); + let finishSwitch!: () => void; + const switchHold = new Promise((resolve) => { finishSwitch = resolve; }); + if (action === 'rollback') { + const switchBranch = cm.switchBranch.bind(cm); + cm.switchBranch = async (...args: Parameters) => { await switchHold; return switchBranch(...args); }; + } else { + const fork = cm.fork.bind(cm); + cm.fork = async (...args: Parameters) => { await switchHold; return fork(...args); }; + } + let finishMaterialize!: () => void; + const materializeHold = new Promise((resolve) => { finishMaterialize = resolve; }); + let materializing = false; + const getModule = fw.moduleRegistry.getModule.bind(fw.moduleRegistry); + fw.moduleRegistry.getModule = (name) => name === 'workspace' ? { + materializeMount: async () => { materializing = true; await materializeHold; order.push('config materialized'); }, + } : getModule(name); + const rolling = action === 'rollback' + ? framework.rollbackToMessage('agent', { messageId: ids[0]! }) + : framework.suppressMessages('agent', { messageIds: [ids[1]!] }); + void rolling.catch(() => {}); + const stopping = framework.stop(); + await releaseWhenStopWaits(finishSwitch); + try { + await rolling; + assert.equal(materializing, true); + await until(() => waitReads >= 2 || order.includes('modules stopped'), 'stop to discover child work', 30_000); + assert.equal(order.includes('modules stopped'), false, 'the original snapshot is not the whole wait'); + } finally { finishMaterialize(); await stopping; } + assert.ok(order.indexOf('config materialized') < order.indexOf('modules stopped')); + }); + + it('round: a synchronous host trace can begin stop without losing the operation that emitted it', async () => { + const framework = await create(); + const fw = internals(framework); + const cm = framework.getAgent('agent')!.getContextManager(); + const sequenceBefore = framework.getStore().currentSequence(); + cm.addMessage('user', [{ type: 'text', text: 'a turn' }]); + fw.saveTurnCheckpoints('agent', [{ agentName: 'agent', turnIndex: 1, sequenceBefore, branchName: 'main', timestamp: Date.now() }]); + let deliver!: () => void; + fw.beginDiscordAwarenessBarrier = () => ({ + requiresBarrier: true, + promise: new Promise((resolve) => { deliver = () => resolve({ status: 'delivered', delivered: 0, failed: 0 }); }), + }); + let stopping: Promise | undefined; + framework.onTrace((event) => { + if (event.type === 'undo:completed') stopping = framework.stop(); + }); + const connection = Object.assign(new EventEmitter(), { id: 'srv' }); + fw.wireMcplEvents(connection); + connection.emit('host-command', { command: 'undo', agentName: 'agent', turns: 1 }, { + id: 9, + respond: () => { order.push('undo answered'); }, + respondError: () => { order.push('undo failed'); }, + }); + await until(() => stopping !== undefined && (deliver !== undefined || order.includes('undo failed')), 'reentrant stop and awareness sync'); + assert.ok(deliver, 'the admitted undo must finish its awareness sync'); + await releaseWhenStopWaits(deliver); + await stopping; + await until(() => order.includes('undo answered') || order.includes('undo failed'), 'undo answer'); + assert.ok(order.indexOf('undo answered') >= 0 && order.indexOf('undo answered') < order.indexOf('modules stopped'), order.join(', ')); + assert.deepEqual(rejections, []); + }); + + for (const elapsed of [6_000, 9_999, 11_000]) it('round: child work shares the operator deadline after ' + elapsed + ' ms of clock input', async () => { + const framework = await create(); + const fw = internals(framework); + const cm = framework.getAgent('agent')!.getContextManager(); + const ids = [1, 2, 3].map((n) => cm.addMessage('user', [{ type: 'text', text: 'm' + n }])); + let finishSwitch!: () => void; + const switchHold = new Promise((resolve) => { finishSwitch = resolve; }); + const switchBranch = cm.switchBranch.bind(cm); + cm.switchBranch = async (...args: Parameters) => { await switchHold; return switchBranch(...args); }; + let finishMaterialize!: () => void; + const materializeHold = new Promise((resolve) => { finishMaterialize = resolve; }); + const getModule = fw.moduleRegistry.getModule.bind(fw.moduleRegistry); + fw.moduleRegistry.getModule = (name) => name === 'workspace' ? { + materializeMount: async () => { await materializeHold; order.push('config materialized'); }, + } : getModule(name); + const rolling = framework.rollbackToMessage('agent', { messageId: ids[0]! }); + void rolling.catch(() => {}); + const nowProperty = Object.getOwnPropertyDescriptor(performance, 'now'); + const timer = globalThis.setTimeout; + const budgets: number[] = []; + let now = 0; + let stopping: Promise | undefined; + try { + Object.defineProperty(performance, 'now', { configurable: true, value: () => now }); + globalThis.setTimeout = ((...args: Parameters) => { + if (args[1] !== undefined && args[1] > 100) budgets.push(args[1]); + return timer(...args); + }) as typeof setTimeout; + stopping = framework.stop(10_000); + void stopping.catch(() => {}); + await releaseWhenStopWaits(() => { now = elapsed; finishSwitch(); }); + await rolling; + await until(() => waitReads >= 2 || order.includes('modules stopped'), 'the second operator wait'); + // Clock input controls the remaining budget. In the timeout cases, + // work stays held until the assertion, so elapsed wall time is no verdict. + if (elapsed === 6_000) { + assert.deepEqual(budgets.slice(0, 2), [10_000, 4_000]); + finishMaterialize(); + await stopping; + } + else { + await assert.rejects(stopping, (error: unknown) => + error instanceof ShutdownTimeoutError && error.pending.includes('config:materialize') + && error.message.startsWith('Shutdown timed out after 10000 ms;')); + assert.equal(framework.getStore().isClosed(), false); + } + } finally { + finishSwitch(); + finishMaterialize(); + globalThis.setTimeout = timer; + if (nowProperty) Object.defineProperty(performance, 'now', nowProperty); + else Reflect.deleteProperty(performance, 'now'); + await rolling.catch(() => {}); + await stopping?.catch(() => {}); + await framework.stop(); + } + }); + }); From 091f31a7b1794b6b29e411fafe3463bcb317e25b Mon Sep 17 00:00:00 2001 From: Sebastian-2380 Date: Tue, 13 Oct 2026 09:34:00 -0400 Subject: [PATCH 04/21] test(lifecycle): let a broken drain assertion release its held turn Wait for the positive quiesce-settlement or module-teardown observation before awaiting stop. If neither occurs, fail within the test's own safety bound and release the artificial token in finally, then await cleanup. Removing the drain's closure condition can otherwise leave the test callback awaiting stop until the runner cancels it, without executing that cleanup. The successful path retains the same order and trace assertions. The focused case passes on Node 24. Production source is unchanged; final main-source and removal checks use this fixture. Co-authored-by: GPT-6 --- test/stop-with-host-operations.test.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/stop-with-host-operations.test.ts b/test/stop-with-host-operations.test.ts index bebcf7b27..a3ae1848d 100644 --- a/test/stop-with-host-operations.test.ts +++ b/test/stop-with-host-operations.test.ts @@ -134,8 +134,10 @@ describe('stop() with an operator operation in flight', () => { framework.onTrace((event) => { traces.push(event.type); }); const quiescing = framework.quiesce({ reason: 'surgery', timeoutMs: 600_000 }); void quiescing.then(() => { settled = true; }, () => { settled = true; }); + const stopping = framework.stop(); try { - await framework.stop(); + await until(() => settled || order.includes('modules stopped'), 'stop to interrupt the quiesce or reach teardown', 10_000); + await stopping; assert.equal(settled, true, 'stop() returned while the quiesce was still waiting'); const status = await quiescing; assert.equal(status.quiesced, true); @@ -145,6 +147,7 @@ describe('stop() with an operator operation in flight', () => { } finally { tokens.delete('agent'); await quiescing.catch(() => {}); + await stopping; } assert.equal(await bootsQuiesced(), true, 'the host boots quiesced'); assert.deepEqual(rejections, []); From 0ab701928b6e785a58cd87d669265fe383363a31 Mon Sep 17 00:00:00 2001 From: Nell-1783 Date: Tue, 13 Oct 2026 10:23:04 -0400 Subject: [PATCH 05/21] fix(lifecycle): let a sync finish a change made during stop, release failed barriers while stopping, admit puppet and maintain A further round on the admission change, from a review of it. - syncDiscordAwarenessMarkers() is no longer refused once stop() has begun. Its callers have already changed the store when they call it: the API server's branch switch moves the branch itself, and its undo and redo call it after undoLastTurn() or redo(), whose own trace can start stop(). Refusing it left a move that had happened unreconciled and reported as failed. It stays tracked, so stop() waits for one that starts while stop() is waiting. One that starts later can fail to deliver, which leaves that work in the durable outbox for the next start, whose startup barrier reconciles with the branch. - completeMcplDataPlaneGate no longer returns early while the host stops. Keeping the planes paused stranded what the pause had buffered, server requests included, until the transport closed. Readied, those requests reach the listeners' stopping refusal (-32000), which is how the listeners answer traffic during stop. The quiesce hold still applies. This also removes the early return's "true", which let a reconnect announce itself without readying anything. - failMcplDataPlaneGate, while the host stops, still reports the failure and recycles nothing, and now releases the barrier through completeMcplDataPlaneGate. A rejected barrier left installed made later inbound requests await it and answer as failed instead of refused. - puppetToolCall() and maintenanceTick() refuse once the queue is closed, like the other operator entry points. puppetToolCall() is also tracked, so stop() waits for a puppet call already running, whose pair could otherwise be stored after the store closed. A maintain already running needs no tracking: stop() awaits the maintenance pass itself. - The config materialization's catch logs a failure instead of discarding it. - Comments and the changelog fragment now say which entry points refuse and why the sync doesn't, what stop() waits for, that it doesn't wait for a turn holding its token with no stream, what a multi-turn host/command undo does when its own trace starts stop(), and what a barrier does while the host stops. Tests: the refusal cases gain maintain and puppet and lose the sync. New cases cover a sync called while stop() waits, a puppet call held in its tool, and a failed materialization's log line. The reopen cases check that the barrier is released and the plane readied, and that the failure is still traced. The materialization and MCPL lifecycle cases assert the whole order, and the test names lose an internal prefix. Checks (VPS, Node 24.21.0, transpiler cache off): typecheck and build clean; stop-with-host-operations 40/40 unforced. With this commit's tests and the previous commit's src, 7 of 40 fail, each at its own assertion; with main's src, all 40 fail. Co-Authored-By: Claude Opus 5.5 --- ...top-waits-for-operator-operations.fixed.md | 61 ++++++- src/framework.ts | 81 ++++++--- test/stop-with-host-operations.test.ts | 154 +++++++++++++++--- 3 files changed, 236 insertions(+), 60 deletions(-) diff --git a/changelog.d/stop-waits-for-operator-operations.fixed.md b/changelog.d/stop-waits-for-operator-operations.fixed.md index 7a5a0aa8e..a03a0f58c 100644 --- a/changelog.d/stop-waits-for-operator-operations.fixed.md +++ b/changelog.d/stop-waits-for-operator-operations.fixed.md @@ -1,7 +1,54 @@ -- **`stop()` waits for admitted operator work before teardown, subject to its optional wait budget.** Quiesce, resume, each `host/command`, store surgeries, awareness syncs, MCPL connect/disconnect/restart calls, and config materializations are tracked. When the wait completes, their work finishes before the event gate, modules, MCPL connections, final sync, and store close. Previously, an operation could keep writing or open a connection after teardown. - - A quiesce stops waiting for its drain or abandon grace when stop begins. Stop cancels streams and waits for the event loop; a turn with a token but no stream settles on its own. The host stays durably quiesced. A drain cut short returns its current status (possibly `drained: false`), logs the stopping message, and omits the `host:quiesce` trace. If abandon was requested but had not begun, stream cancellation is left to stop. An abandon whose grace is cut short still reports the abandon through its usual trace and log. - - A resume refuses at entry once the queue is closed, even on an unquiesced host. An admitted resume checks again after its maintenance and preview waits, before leaving quiesce. A resume past that commit completes its flush and durable clear. Its log names parked wakes that will not run during stop. - - The public quiesce, resume, rollback, suppression, undo, redo, awareness-sync, and MCPL lifecycle entry points refuse after queue closure, before reads or mutations, with ` refused: the framework is stopping`. Already-admitted operations use private continuations for their remaining work. Registration precedes the body's synchronous callbacks, so a trace listener can reenter stop without escaping its wait. - - Undo, redo, rollback, and suppression still start config materialization without awaiting file IO. Stop waits for that work too, including a materialization started by a surgery while stop is already waiting. One deadline covers every round of that operator wait. - - The MCPL barrier funnel applies the stop boundary to every completion path. A failed barrier recycles no connection during stop. A successful barrier is retired but readies no data plane. - - **For operators:** an admitted awareness drain finishes its deliveries, each within its own deadline. `stop(teardownTimeoutMs)` also bounds the operator wait. On timeout, teardown proceeds while the operation may still act on torn-down components; the store stays open for another stop attempt. The timeout is a `ShutdownTimeoutError` naming pending work, either alone or inside an `AggregateError` if other cleanup failed. A retry while the work remains pending can time out again; a retry after it settles can close the store. +- **`stop()` waits for an operator's operation that is already running, and + refuses new ones.** A quiesce, a resume, a `host/command`, a store surgery, + an undo or redo's config materialization, a puppet tool call, an awareness + sync, or an MCPL connect, disconnect or restart admitted before `stop()` + began now finishes before the event gate, the modules, the MCPL + connections and the store go. Before, such an operation kept running + through the teardown. A resume waiting on its maintenance pass or previews + could go on to leave quiesce while the host stopped, so the host booted + unquiesced; a puppet call could find the store closed partway; a surgery + could still be running at the final sync; a `host/command` could answer + after its connection closed; and a connect could leave a connection that + nothing closed. + - Once `stop()` has begun, `quiesce()`, `resume()`, `rollbackToMessage()`, + `suppressMessages()`, `undoLastTurn()`, `redo()`, `maintenanceTick()`, + `puppetToolCall()`, `connectMcplServer()`, `disconnectMcplServer()` and + `restartMcplServer()` are refused with " refused: the framework is + stopping" before they read or change anything, as a `host/command` + already was. `syncDiscordAwarenessMarkers()` isn't refused, because its + callers, such as the API server's branch switch, have already changed + the store when they call it. `stop()` waits for one that starts while + it is waiting. + - `stop()` also waits for what those operations start while it waits: the + config materialization that undo, redo and the two surgeries start + without waiting for it themselves. A failed materialization is logged. + - A quiesce stops waiting for its drain, or for an abandon's grace, once + `stop()` begins. `stop()` cancels the streams and waits for the event + loop, but not for a turn that holds its token with no stream to cancel, + so a quiesce cut short can report `drained: false`. The host stays + quiesced, so it boots quiesced. A drain cut short logs that it stopped + waiting and emits no `host:quiesce` trace, and an abandon it hadn't + begun is left to `stop()`, which cancels the streams. An abandon whose + grace is cut short reports its abandon as usual. + - A resume is refused at its start once `stop()` has begun, whether or not + the host is quiesced, and checks again after its maintenance and preview + waits, before it leaves quiesce, so a quiesced host stays quiesced. A + resume past that point finishes, its durable clear included, and its log + says that the parked wakes it released won't run. + - A host's trace listener that calls `stop()` from inside an operation, + such as on its completion trace, doesn't lose that operation: `stop()` + waits for it. A multi-turn `host/command` undo stopped that way is + refused at its next turn and answers with the turns it undid. + - While the host stops, an MCPL awareness barrier that fails no longer + recycles the connections, which `stop()` is closing. It is released as + a completed one is, so nothing waits on a rejected barrier, and unless + the host is quiesced, what it held back reaches the listeners' stopping + refusal. + - **For operators:** `stop(teardownTimeoutMs)` bounds this wait too, with + one budget for the whole wait, however many operations join it. An + operation that outlasts it fails `stop()` with a `ShutdownTimeoutError` + that names it, on its own or inside an `AggregateError` when other + cleanup failed too. `stop()` still tears down the rest, while that + operation may still run, and keeps the store open, so `stop()` can be + called again. An awareness drain it waits for finishes its deliveries, + each within its own deadline. diff --git a/src/framework.ts b/src/framework.ts index f7ecacc16..958201c1a 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -1138,8 +1138,9 @@ export class AgentFramework { private stopPromise: Promise | null = null; private stopCompleted = false; private ownedStoreClosed = false; - /** Admitted operator work, including child config materializations. - * stop() drains this set before teardown, unless its wait budget expires. */ + /** 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. */ private readonly operatorOperations = new Set<{ label: string; promise: Promise }>(); private membrane: Membrane; private queue: ProcessQueueImpl; @@ -2133,9 +2134,10 @@ export class AgentFramework { } /** - * Stop the event loop. Failed operator/module/MCPL waits can be retried; - * storage stays open. A wait timeout permits teardown to continue while - * that work may still act on the components being torn down. + * Stop the event loop. After a failed or timed-out wait on operator, module + * or MCPL cleanup, storage stays open and stop() can be called again. A + * timed-out wait lets teardown go on while that work may still act on what + * is being torn down. * @param teardownTimeoutMs Optional budget for each wait on cleanup: an * operator's operations already running, then modules and MCPL together. * Omit to wait until cleanup settles, as required by existing module/child @@ -2183,8 +2185,9 @@ export class AgentFramework { if (this.queue?.isClosed) throw new Error(verb + ' refused: the framework is stopping'); } - /** Admission is closed, but admitted work can still start child work. - * All rounds share the caller's deadline rather than renewing its budget. */ + /** Wait in rounds until the set is empty: admitted work can still start work + * of its own, and an awareness sync can still join. All rounds share the + * caller's deadline rather than renewing its budget. */ private async waitForOperatorOperations(timeoutMs: number | undefined): Promise { const deadline = timeoutMs === undefined ? undefined : performance.now() + timeoutMs; while (this.operatorOperations.size > 0) { @@ -2275,12 +2278,13 @@ export class AgentFramework { await this.loopPromise; } - // Admission closed with the queue. Only admitted operations can add - // work now (for example a surgery's config materialization), so drain - // until the set is empty. This follows stream cancellation and the event - // loop, allowing a quiesce or surgery waiting on them to settle. - // If the budget expires, teardown proceeds while the operation may still - // act on torn-down components. Keep the store open for a later stop(). + // Closing the queue closed admission, except for an awareness sync (see + // syncDiscordAwarenessMarkers). What admitted operations start now, such + // as a surgery's config materialization, joins the set, and so does such + // a sync, so wait until the set is empty. This comes after the streams and + // the event loop, which a quiesce or a surgery may be waiting on. If the + // budget runs out, teardown goes on while the operation may still act on + // what it tears down, and the store stays open for another stop(). let operatorOperationsTimedOut: unknown; try { await this.waitForOperatorOperations(teardownTimeoutMs); @@ -3687,9 +3691,10 @@ export class AgentFramework { await barrier.promise; this.completeMcplDataPlaneGate(barrier); } catch (error) { - // The host has resumed. Report the failed barrier without undoing - // that commit. The funnel recycles while serving; during stop it - // neither recycles on failure nor readies data planes on success. + // The host has resumed, so report the failed barrier without undoing + // that. While serving, failMcplDataPlaneGate recycles the + // connections and their reconnect flows reopen the data planes; + // while the host stops, it releases the barrier instead. await this.failMcplDataPlaneGate(barrier, 'quiesce resume', error); } } @@ -4002,6 +4007,8 @@ export class AgentFramework { * true in quiesce mode precisely so this machinery stays hot. */ async maintenanceTick(): Promise { + // A pass already running is stop()'s to wait for; a new one isn't started. + this.refuseOperatorAfterStop('maintain'); if (!this.running) { // startQueuedMaintenance no-ops on a stopped/never-started framework — // a bare snapshot would masquerade as a completed pass, so say so. @@ -6692,7 +6699,12 @@ export class AgentFramework { if (ws && 'materializeMount' in ws) { void this.trackOperatorOperation('config:materialize', () => (ws as { materializeMount: (m: string) => Promise }).materializeMount('_config'), - ).catch(() => {}); + ).catch((error) => { + console.error( + '[workspace] re-materializing the config mount after a branch switch failed:', + error instanceof Error ? error.message : error, + ); + }); } } @@ -11653,6 +11665,16 @@ export class AgentFramework { agentName: string, toolName: string, input: Record, + ): Promise<{ toolUseId: string; result: ToolResult }> { + this.refuseOperatorAfterStop('puppet'); + return this.trackOperatorOperation('puppet:tool-call', () => this.runPuppetToolCall(agentName, toolName, input)); + } + + /** puppetToolCall's work, run where stop() can wait for it. */ + private async runPuppetToolCall( + agentName: string, + toolName: string, + input: Record, ): Promise<{ toolUseId: string; result: ToolResult }> { const agent = this.agents.get(agentName); if (!agent) throw new Error(`Unknown agent: ${agentName}`); @@ -13987,9 +14009,13 @@ export class AgentFramework { await this.refreshMcplTools(); } - /** Reconcile the durable ledger with Chronicle, then deliver every server's work. */ - async syncDiscordAwarenessMarkers(onlyServerId?: string): Promise { - this.refuseOperatorAfterStop('awareness sync'); + /** Reconcile the durable ledger with Chronicle, then deliver every server's work. + * Unlike the other operator entry points, this one isn't refused once stop() + * has begun: its callers, such as the API server's branch switch, have + * already changed the store and call it to finish that change. stop() waits + * for one that starts while stop() is waiting. One that starts later can + * fail to deliver, and the outbox keeps that work for the next start. */ + syncDiscordAwarenessMarkers(onlyServerId?: string): Promise { return this.trackOperatorOperation('awareness:sync', () => this.runDiscordAwarenessSync(onlyServerId)); } @@ -14293,9 +14319,6 @@ export class AgentFramework { // check sits AFTER this guard: readyControlPlane can also synchronously // flush a control event that installs a newer barrier.) if (this.discordAwarenessBarrier !== null) return false; - // This generation is retired above, but a stopping host reopens no - // data plane. Recheck for each connection: a ready() can reenter stop(). - if (this.queue?.isClosed) return true; // Host quiesce (issue #122): this is the single point every barrier // completion and reconnect flow funnels through, so holding here keeps // data planes paused across awareness drains and reconnects for the @@ -14328,9 +14351,15 @@ export class AgentFramework { error: `Discord awareness accounting unhealthy: ${err.message}`, }); } - // A stopping host closes these connections itself. Recycling them now - // would start reconnects only for stop() to close them again. - if (this.queue?.isClosed) return; + // A stopping host closes these connections itself, so recycling them now + // would start reconnects only for stop() to close them again. Release the + // barrier instead, as a completed one is released: nothing waits on a + // rejected barrier, and unless the host is quiesced, what the pause held + // back reaches the listeners' stopping refusal. + if (this.queue?.isClosed) { + this.completeMcplDataPlaneGate(barrier); + return; + } await Promise.all(connections.map(async (connection) => { await connection.reconnectAfterFailure().catch((closeError) => { console.error( diff --git a/test/stop-with-host-operations.test.ts b/test/stop-with-host-operations.test.ts index a3ae1848d..0d38a6812 100644 --- a/test/stop-with-host-operations.test.ts +++ b/test/stop-with-host-operations.test.ts @@ -1,17 +1,19 @@ /** - * stop() with an operator's operation still running. + * stop() and an operator's operations. * - * stop() refuses a host/command that arrives once it has begun. These tests - * cover the operations admitted before that: a quiesce draining, a resume, a - * host/command, a store surgery and an awareness sync, whether they came over - * MCPL or from a direct call. A completed wait precedes the event gate, - * modules, MCPL connections and store teardown; an explicit budget can cut - * the wait short and leave the store open. Quiesce releases its drain, and - * a resume that has not committed preserves the durable quiesce. + * Once stop() has begun, an operator's entry points refuse: quiesce, resume, + * the store surgeries, undo, redo, maintain, puppet and the MCPL lifecycle + * calls (a host/command is refused by its listener). The awareness sync is + * the exception, since its callers have already changed the store. These + * tests cover those refusals and the operations admitted before them, over + * MCPL or from a direct call: stop() waits for each, and for what each starts + * while it waits, before the event gate, the modules, the MCPL connections + * and the store go. A budget can cut that wait short, leaving the store open. * - * Where a test holds an operation open, it lets it go once stop() reaches its - * modules' teardown or reads a nonempty operations set to wait for them. - * The order of the two events is the verdict, so it doesn't depend on timing. + * Where a test holds an operation open, it lets it go once stop() reads a + * nonempty set of operations to wait on, or reaches its modules' teardown. + * main reaches teardown without reading any such set, so the order of the + * two events is the verdict, and it doesn't depend on timing. */ import { describe, it, beforeEach, afterEach } from 'node:test'; @@ -211,18 +213,23 @@ describe('stop() with an operator operation in flight', () => { assert.equal(await bootsQuiesced(), true, 'the host boots quiesced'); }); - it('stop() waits for a resume past its commit point, and a reopen that fails recycles no connection', async () => { + it('stop() waits for a resume past its commit point, and a reopen that fails is released, not recycled', async () => { const framework = await create(); const connection = { id: 'srv', recycled: 0, + readied: 0, pauseDataPlane(): void {}, readyControlPlane(): void {}, - ready(): void {}, + ready(): void { this.readied++; }, async reconnectAfterFailure(): Promise { this.recycled++; }, }; const fw = internals(framework); fw.mcplServerRegistry = { getAllServers: () => [connection], closeAll: async () => {} }; + const failures: string[] = []; + framework.onTrace((event) => { + if (event.type === 'mcpl:server-error') failures.push(String((event as { serverId?: unknown }).serverId)); + }); framework.start(); await framework.quiesce(); // The reopen waits on the awareness barrier, which fails while stop() waits. @@ -238,7 +245,10 @@ describe('stop() with an operator operation in flight', () => { await stopping; assert.deepEqual(order, ['resume settled', 'modules stopped']); assert.equal((await resuming).quiesced, false, 'past its commit point the resume completes'); + assert.deepEqual(failures, ['srv'], 'the failure is still reported'); assert.equal(connection.recycled, 0, 'a stopping host recycles no connection'); + assert.equal(fw.discordAwarenessBarrier, null, 'the failed barrier is released'); + assert.equal(connection.readied, 1, 'its data plane opens, so the listeners answer what it held back'); assert.equal(await bootsQuiesced(), false, 'its durable clear landed'); assert.deepEqual(rejections, []); }); @@ -321,6 +331,47 @@ describe('stop() with an operator operation in flight', () => { assert.deepEqual(rejections, []); }); + it('an awareness sync called while stop() waits is not refused, and stop() waits for it too', { timeout: 20_000 }, async () => { + // The API server moves the branch on the store itself and then syncs, so + // a sync that arrives during stop() finishes a change already made. + const framework = await create(); + const fw = framework as unknown as Internals & { refreshMcplTools(): Promise; emitMcplToolDiff(...args: unknown[]): void }; + let releaseFirst!: () => void; + const first = new Promise((resolve) => { releaseFirst = resolve; }); + fw.refreshMcplTools = async () => {}; + fw.emitMcplToolDiff = () => {}; + fw.mcplServerRegistry = { + getAllServers: () => [], + removeServer: async () => { await first; order.push('server removed'); }, + closeAll: async () => {}, + }; + const disconnecting = framework.disconnectMcplServer('srv'); + let deliver: (() => void) | undefined; + fw.beginDiscordAwarenessBarrier = () => ({ + requiresBarrier: true, + promise: new Promise((resolve) => { + deliver = () => resolve({ status: 'delivered', delivered: 1, failed: 0 }); + }), + }); + const stopping = framework.stop(); + await until(() => waitReads > 0 || order.includes('modules stopped'), 'stop to wait on the disconnect', 10_000); + const syncing = framework.syncDiscordAwarenessMarkers().then(() => { order.push('awareness synced'); }); + try { + await until(() => deliver !== undefined, 'the late sync to wait on its barrier'); + releaseFirst(); + await disconnecting; + // stop()'s next round reads the set with the late sync in it. + await until(() => waitReads >= 2 || order.includes('modules stopped'), 'stop to wait on the late sync', 10_000); + } finally { + releaseFirst(); + deliver?.(); + } + await syncing; + await stopping; + assert.deepEqual(order, ['server removed', 'awareness synced', 'modules stopped']); + assert.deepEqual(rejections, []); + }); + it("an operation that outlasts stop()'s budget fails the stop and keeps the store open for a retry", async () => { const framework = await create(); let deliver: (() => void) | undefined; @@ -356,13 +407,14 @@ describe('stop() with an operator operation in flight', () => { ['suppress', (f) => f.suppressMessages('agent', { messageIds: ['invalid'] })], ['undo', (f) => f.undoLastTurn('agent')], ['redo', (f) => f.redo('agent')], - ['awareness sync', (f) => f.syncDiscordAwarenessMarkers()], + ['maintain', (f) => f.maintenanceTick()], + ['puppet', (f) => f.puppetToolCall('agent', 'unused', {})], ['connect MCPL server', (f) => f.connectMcplServer({ id: 'late', command: 'unused' })], ['disconnect MCPL server', (f) => f.disconnectMcplServer('late')], ['restart MCPL server', (f) => f.restartMcplServer('late')], ]; for (const [verb, call] of lateCalls) { - it('round: ' + verb + ' refuses during teardown and after stop, without writes or operator logs', async () => { + it(verb + ' is refused during teardown and after stop(), with no writes and no operator log', async () => { const framework = await create(); let backendCalls = 0; Object.assign(internals(framework), { @@ -404,7 +456,7 @@ describe('stop() with an operator operation in flight', () => { }); } - it('round: resume admitted after closure refuses before a held maintenance pass ends', async () => { + it('a resume called once stop() has begun refuses before a held maintenance pass ends', async () => { const framework = await create(); await framework.quiesce(); let endPass!: () => void; @@ -422,7 +474,7 @@ describe('stop() with an operator operation in flight', () => { } finally { endPass(); await stopping; await resuming.catch(() => {}); } }); - it('round: resume rechecks closure after preview retries', async () => { + it('a resume checks again after its preview retries, and refuses when stop() began during them', async () => { const framework = await create(); await framework.quiesce(); let previewed = false; @@ -439,7 +491,9 @@ describe('stop() with an operator operation in flight', () => { assert.equal(await bootsQuiesced(), true); }); - it('round: a successful resume barrier across stop retires without readying a data plane', async () => { + it('a resume whose reopen succeeds while stop() waits releases the barrier and opens the data plane', async () => { + // The listeners answer whatever the pause held back with their stopping + // refusal, so the stopping host opens its planes as a serving one does. const framework = await create(); let ready = 0; const connection = { @@ -460,14 +514,42 @@ describe('stop() with an operator operation in flight', () => { await releaseWhenStopWaits(deliver); await resuming; await stopping; - assert.equal(ready, 0); + assert.equal(ready, 1); assert.equal(fw.discordAwarenessBarrier, null); assert.ok(logged.some((line) => line.includes("parked wake(s) won't run") && line.includes('stopping'))); assert.equal(await bootsQuiesced(), false); }); + it('stop() waits for a puppet tool call, so it finishes before the modules stop', { timeout: 20_000 }, async () => { + const framework = await create(); + const fw = framework as unknown as Internals & { + getToolsForAgent(name: string): Array<{ name: string }>; + executeToolCall(call: unknown): Promise; + }; + let release!: () => void; + const held = new Promise((resolve) => { release = resolve; }); + let called = false; + fw.getToolsForAgent = () => [{ name: 'probe' }]; + fw.executeToolCall = async () => { + called = true; + await held; + order.push('tool returned'); + return { success: true, data: 'done' }; + }; + const puppeting = framework.puppetToolCall('agent', 'probe', {}); + void puppeting.catch(() => {}); + await until(() => called, 'the puppet call to reach its tool'); + const stopping = framework.stop(); + await releaseWhenStopWaits(release); + const { toolUseId } = await puppeting; + await stopping; + assert.match(toolUseId, /^toolu_01/); + assert.deepEqual(order, ['tool returned', 'modules stopped']); + assert.deepEqual(rejections, []); + }); + for (const action of ['connect', 'disconnect', 'restart'] as const) { - it('round: an admitted MCPL ' + action + ' settles before connection teardown', async () => { + it('stop() waits for an MCPL ' + action + ' already running before it closes the connections', async () => { const framework = await create(); const fw = framework as unknown as Internals & { mcplHostCapabilities: object; @@ -509,12 +591,13 @@ describe('stop() with an operator operation in flight', () => { await running; await stopping; assert.equal(connected, false); - assert.ok(order.indexOf(action === 'disconnect' ? 'connection removed' : 'connection opened') < order.indexOf('modules stopped'), order.join(', ')); + const settled = action === 'disconnect' ? 'connection removed' : 'connection opened'; + assert.ok(order.includes(settled) && order.indexOf(settled) < order.indexOf('modules stopped'), order.join(', ')); }); } for (const action of ['undo', 'redo', 'rollback', 'suppress'] as const) { - it('round: ' + action + ' config materialization stays fire-and-forget but finishes before teardown', async () => { + it(action + "'s config materialization doesn't hold its caller, and stop() waits for it", { timeout: 20_000 }, async () => { const framework = await create(); const fw = internals(framework); const cm = framework.getAgent('agent')!.getContextManager(); @@ -537,11 +620,28 @@ describe('stop() with an operator operation in flight', () => { const stopping = framework.stop(); await releaseWhenStopWaits(release); await stopping; - assert.ok(order.indexOf('config materialized') < order.indexOf('modules stopped'), order.join(', ')); + assert.deepEqual(order, ['config materialized', 'modules stopped']); }); } - for (const action of ['rollback', 'suppress'] as const) it('round: stop also waits for materialization an admitted ' + action + ' starts after its first snapshot', async () => { + it('a config materialization that fails is logged, and its undo still succeeds', async () => { + const framework = await create(); + const fw = internals(framework); + const cm = framework.getAgent('agent')!.getContextManager(); + const sequenceBefore = framework.getStore().currentSequence(); + cm.addMessage('user', [{ type: 'text', text: 'a turn' }]); + fw.saveTurnCheckpoints('agent', [{ agentName: 'agent', turnIndex: 1, sequenceBefore, branchName: 'main', timestamp: Date.now() }]); + const getModule = fw.moduleRegistry.getModule.bind(fw.moduleRegistry); + fw.moduleRegistry.getModule = (name) => name === 'workspace' ? { + materializeMount: async () => { throw new Error('disk full'); }, + } : getModule(name); + assert.equal(framework.undoLastTurn('agent').undone, true); + await framework.stop(); + assert.ok(logged.some((line) => line.includes('re-materializing the config mount after a branch switch failed') && line.includes('disk full')), logged.join('\n')); + assert.deepEqual(rejections, []); + }); + + for (const action of ['rollback', 'suppress'] as const) it('stop() also waits for the materialization a ' + action + ' in flight starts after stop() first reads the set', { timeout: 20_000 }, async () => { const framework = await create(); const fw = internals(framework); const cm = framework.getAgent('agent')!.getContextManager(); @@ -574,10 +674,10 @@ describe('stop() with an operator operation in flight', () => { await until(() => waitReads >= 2 || order.includes('modules stopped'), 'stop to discover child work', 30_000); assert.equal(order.includes('modules stopped'), false, 'the original snapshot is not the whole wait'); } finally { finishMaterialize(); await stopping; } - assert.ok(order.indexOf('config materialized') < order.indexOf('modules stopped')); + assert.deepEqual(order, ['config materialized', 'modules stopped']); }); - it('round: a synchronous host trace can begin stop without losing the operation that emitted it', async () => { + it("a trace listener that calls stop() doesn't lose the operation whose trace it heard", async () => { const framework = await create(); const fw = internals(framework); const cm = framework.getAgent('agent')!.getContextManager(); @@ -609,7 +709,7 @@ describe('stop() with an operator operation in flight', () => { assert.deepEqual(rejections, []); }); - for (const elapsed of [6_000, 9_999, 11_000]) it('round: child work shares the operator deadline after ' + elapsed + ' ms of clock input', async () => { + for (const elapsed of [6_000, 9_999, 11_000]) it("work an operation starts shares stop()'s deadline, " + elapsed + ' ms in', async () => { const framework = await create(); const fw = internals(framework); const cm = framework.getAgent('agent')!.getContextManager(); From e1bab7d6b7473b61a6bb7f8cab6f6f466b633ef7 Mon Sep 17 00:00:00 2001 From: Nell-1783 Date: Tue, 13 Oct 2026 10:32:34 -0400 Subject: [PATCH 06/21] test(puppet-tool-call): give the stub framework its operator-operations set The puppet tests build their framework with Object.create(AgentFramework.prototype) and set the fields the real path uses. puppetToolCall() now counts as an operation stop() waits for, so the stub needs the set it goes into, as it already has the turn tokens and deferred writes. At the previous commit the file's ten tests failed on Node 20 and 24 with a TypeError in trackOperatorOperation; with the field they pass. Co-Authored-By: Claude Opus 5.5 --- test/puppet-tool-call.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/puppet-tool-call.test.ts b/test/puppet-tool-call.test.ts index 08e34088a..412110b6c 100644 --- a/test/puppet-tool-call.test.ts +++ b/test/puppet-tool-call.test.ts @@ -54,6 +54,8 @@ function puppetHarness(opts?: { fw.deferredSeq = 0; fw.deferredScanFrom = new Map(); fw.pendingAssistantBlocks = new Map(); + // A puppet call is an operation stop() waits for. + fw.operatorOperations = new Set(); fw.primaryAgentName = 'princess'; // Cross-turn writer path (what a channel message goes through): defers // while a turn is alive, else stores — the real guard, reduced. From 5bf4e39536e26c33bacb99a8b54938fe2e1032cb Mon Sep 17 00:00:00 2001 From: Nell-1783 Date: Tue, 13 Oct 2026 10:38:52 -0400 Subject: [PATCH 07/21] docs(changelog): plainer wording for the stop() fragment Active voice where the fragment said what the framework does in the passive (the refusals, the logged materialization, the released barrier, the abandon a cut-short drain leaves to stop()), and a plain sentence for the trace listener that calls stop(). No change in what it says. Co-Authored-By: Claude Opus 5.5 --- ...top-waits-for-operator-operations.fixed.md | 31 ++++++++++--------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/changelog.d/stop-waits-for-operator-operations.fixed.md b/changelog.d/stop-waits-for-operator-operations.fixed.md index a03a0f58c..ee4c32aa7 100644 --- a/changelog.d/stop-waits-for-operator-operations.fixed.md +++ b/changelog.d/stop-waits-for-operator-operations.fixed.md @@ -13,37 +13,38 @@ - Once `stop()` has begun, `quiesce()`, `resume()`, `rollbackToMessage()`, `suppressMessages()`, `undoLastTurn()`, `redo()`, `maintenanceTick()`, `puppetToolCall()`, `connectMcplServer()`, `disconnectMcplServer()` and - `restartMcplServer()` are refused with " refused: the framework is + `restartMcplServer()` refuse with " refused: the framework is stopping" before they read or change anything, as a `host/command` - already was. `syncDiscordAwarenessMarkers()` isn't refused, because its + already did. `syncDiscordAwarenessMarkers()` doesn't refuse, because its callers, such as the API server's branch switch, have already changed the store when they call it. `stop()` waits for one that starts while it is waiting. - `stop()` also waits for what those operations start while it waits: the config materialization that undo, redo and the two surgeries start - without waiting for it themselves. A failed materialization is logged. + without waiting for it themselves. The framework now logs a failed + materialization. - A quiesce stops waiting for its drain, or for an abandon's grace, once `stop()` begins. `stop()` cancels the streams and waits for the event loop, but not for a turn that holds its token with no stream to cancel, so a quiesce cut short can report `drained: false`. The host stays quiesced, so it boots quiesced. A drain cut short logs that it stopped - waiting and emits no `host:quiesce` trace, and an abandon it hadn't - begun is left to `stop()`, which cancels the streams. An abandon whose - grace is cut short reports its abandon as usual. - - A resume is refused at its start once `stop()` has begun, whether or not + waiting, emits no `host:quiesce` trace, and leaves an abandon it hadn't + begun to `stop()`, which cancels the streams. An abandon whose grace is + cut short reports its abandon as usual. + - A resume refuses at its start once `stop()` has begun, whether or not the host is quiesced, and checks again after its maintenance and preview waits, before it leaves quiesce, so a quiesced host stays quiesced. A resume past that point finishes, its durable clear included, and its log says that the parked wakes it released won't run. - - A host's trace listener that calls `stop()` from inside an operation, - such as on its completion trace, doesn't lose that operation: `stop()` - waits for it. A multi-turn `host/command` undo stopped that way is - refused at its next turn and answers with the turns it undid. + - A host's trace listener can call `stop()` from inside an operation, such + as on its completion trace, and `stop()` still waits for that operation. + A multi-turn `host/command` undo stopped that way refuses at its next + turn and answers with the turns it undid. - While the host stops, an MCPL awareness barrier that fails no longer - recycles the connections, which `stop()` is closing. It is released as - a completed one is, so nothing waits on a rejected barrier, and unless - the host is quiesced, what it held back reaches the listeners' stopping - refusal. + recycles the connections, which `stop()` is closing. The framework + releases it as it releases a completed one, so nothing waits on a + rejected barrier, and unless the host is quiesced, what it held back + reaches the listeners' stopping refusal. - **For operators:** `stop(teardownTimeoutMs)` bounds this wait too, with one budget for the whole wait, however many operations join it. An operation that outlasts it fails `stop()` with a `ShutdownTimeoutError` From 3affa5d1babf9950df5db136388ee3d558df06d4 Mon Sep 17 00:00:00 2001 From: Nell-1783 Date: Tue, 13 Oct 2026 11:05:41 -0400 Subject: [PATCH 08/21] feat(framework): switchBranch() as an operator operation; refuse an awareness sync once stop() has finished waiting From the review of the previous round, and two readers' reads of it. - New: AgentFramework.switchBranch(name) switches the store to an existing branch, reconciles the Discord awareness markers with it, and re-materializes the config mount, then resolves. Those are the steps the API server's branch switch ran itself, on the store, outside the framework, so a switch requested once stop() had begun moved the branch, mounted its config after the modules had stopped and the store had closed, and answered success. As an operator operation, switchBranch() refuses once stop() has begun, and stop() waits for one already running. The API server's cmdBranchSwitch now calls it and keeps only its own bookkeeping and broadcast. Its current branch follows the store even when a later step fails after the store has moved. Branch create and delete stay in the API server: each is one synchronous store write that either completes or throws. - syncDiscordAwarenessMarkers() is admitted once stop() has begun only until stop() has finished waiting on the operator set. stop() can wait for a sync admitted while it waits, which is what its callers need, having already changed the store. One that starts after that can't be waited for, so it's refused instead of running its barrier against connections being closed. stopOwned marks the point (operatorWaitEnded), whether the wait completed or timed out. - The surgeries' and host/command undo's awareness syncs call the public syncDiscordAwarenessMarkers() again, as on main. With the sync no longer refused while stop() waits, the private continuation they used behaved the same, and its removal probes showed no test that told them apart. A restart keeps its private disconnect and connect. - Text: the changelog fragment and comments say which entry points refuse, and when the sync is the exception. They say that stop() waits for puppet calls among the operations, though not for other turns that hold a token with no stream. They say that a puppet call holds stop() for as long as its tool runs, and that a budget is given to the operator wait and then again to the modules' and MCPL's teardown. A separate changelog.d fragment announces switchBranch(). Tests: the refusal cases gain switch branch. New cases cover an API branch switch held in its config mount, which stop() waits for; one requested once stop() has begun, which is refused with the branch left where it was; the API server's current branch after a switch fails past the store's move; and a sync called once stop() has finished waiting, which is refused and installs no barrier. The puppet wait case now asserts that the stored pair's two writes land before the modules stop. Checks (VPS, Node 24.21.0, transpiler cache off): typecheck and build clean; stop-with-host-operations 45/45 unforced. With these tests and the previous commit's src, 4 of 45 fail, each at its own assertion; the bookkeeping case passes there, because the old code set the API server's branch before its awaits. Removing each of this commit's seven pieces makes a test fail: the late-sync refusal, marking the wait's end, the switch's refusal, its tracking, its awaited mount, the API server's delegation, and its bookkeeping after a failure. Co-Authored-By: Claude Opus 5.5 --- ...top-waits-for-operator-operations.fixed.md | 73 +++++++----- changelog.d/switch-branch.added.md | 5 + src/api/server.ts | 21 ++-- src/framework.ts | 70 ++++++++--- test/stop-with-host-operations.test.ts | 110 +++++++++++++++++- 5 files changed, 214 insertions(+), 65 deletions(-) create mode 100644 changelog.d/switch-branch.added.md diff --git a/changelog.d/stop-waits-for-operator-operations.fixed.md b/changelog.d/stop-waits-for-operator-operations.fixed.md index ee4c32aa7..c872b53e0 100644 --- a/changelog.d/stop-waits-for-operator-operations.fixed.md +++ b/changelog.d/stop-waits-for-operator-operations.fixed.md @@ -1,36 +1,43 @@ - **`stop()` waits for an operator's operation that is already running, and - refuses new ones.** A quiesce, a resume, a `host/command`, a store surgery, - an undo or redo's config materialization, a puppet tool call, an awareness - sync, or an MCPL connect, disconnect or restart admitted before `stop()` - began now finishes before the event gate, the modules, the MCPL - connections and the store go. Before, such an operation kept running - through the teardown. A resume waiting on its maintenance pass or previews - could go on to leave quiesce while the host stopped, so the host booted - unquiesced; a puppet call could find the store closed partway; a surgery - could still be running at the final sync; a `host/command` could answer - after its connection closed; and a connect could leave a connection that - nothing closed. + refuses new ones, with one exception given below.** A quiesce, a resume, a + `host/command`, a store surgery, a branch switch, an undo or redo's config + materialization, a puppet tool call, an awareness sync, or an MCPL + connect, disconnect or restart admitted before `stop()` began now + finishes before the event gate, the modules, the MCPL connections and the + store go, unless a budget runs out first (see the operators' note below). + Before, such an operation kept running through the teardown. A resume + waiting on its maintenance pass or previews could go on to leave quiesce + while the host stopped, so the host booted unquiesced; a puppet call + could find the store closed partway; a surgery could still be running at + the final sync; a `host/command` could answer after its connection + closed; an API server branch switch requested during `stop()` moved the + branch, mounted its config after the store had closed and answered + success; and a connect could leave a connection that nothing closed. - Once `stop()` has begun, `quiesce()`, `resume()`, `rollbackToMessage()`, - `suppressMessages()`, `undoLastTurn()`, `redo()`, `maintenanceTick()`, - `puppetToolCall()`, `connectMcplServer()`, `disconnectMcplServer()` and - `restartMcplServer()` refuse with " refused: the framework is - stopping" before they read or change anything, as a `host/command` - already did. `syncDiscordAwarenessMarkers()` doesn't refuse, because its - callers, such as the API server's branch switch, have already changed - the store when they call it. `stop()` waits for one that starts while - it is waiting. + `suppressMessages()`, `switchBranch()`, `undoLastTurn()`, `redo()`, + `maintenanceTick()`, `puppetToolCall()`, `connectMcplServer()`, + `disconnectMcplServer()` and `restartMcplServer()` refuse with " + refused: the framework is stopping" before they read or change + anything, as a `host/command` already did. + - The exception is `syncDiscordAwarenessMarkers()`, which `stop()` admits + until it has finished waiting, because its callers have already changed + the store when they call it: the API server's undo and redo call it + after `undoLastTurn()` or `redo()`, sometimes from inside a trace + listener that started `stop()`. `stop()` waits for a sync admitted then, + and refuses one that starts once it can no longer wait for it. - `stop()` also waits for what those operations start while it waits: the config materialization that undo, redo and the two surgeries start without waiting for it themselves. The framework now logs a failed materialization. - A quiesce stops waiting for its drain, or for an abandon's grace, once `stop()` begins. `stop()` cancels the streams and waits for the event - loop, but not for a turn that holds its token with no stream to cancel, - so a quiesce cut short can report `drained: false`. The host stays - quiesced, so it boots quiesced. A drain cut short logs that it stopped - waiting, emits no `host:quiesce` trace, and leaves an abandon it hadn't - begun to `stop()`, which cancels the streams. An abandon whose grace is - cut short reports its abandon as usual. + loop and the operations above, puppet calls included. It doesn't wait + for any other turn that holds its token with no stream to cancel, so a + quiesce cut short can report `drained: false`. The host stays quiesced, + so it boots quiesced. A drain cut short logs that it stopped waiting, + emits no `host:quiesce` trace, and leaves an abandon it hadn't begun to + `stop()`, which cancels the streams. An abandon whose grace is cut short + reports its abandon as usual. - A resume refuses at its start once `stop()` has begun, whether or not the host is quiesced, and checks again after its maintenance and preview waits, before it leaves quiesce, so a quiesced host stays quiesced. A @@ -45,11 +52,17 @@ releases it as it releases a completed one, so nothing waits on a rejected barrier, and unless the host is quiesced, what it held back reaches the listeners' stopping refusal. - - **For operators:** `stop(teardownTimeoutMs)` bounds this wait too, with - one budget for the whole wait, however many operations join it. An - operation that outlasts it fails `stop()` with a `ShutdownTimeoutError` + - **For operators:** `stop()` waits as long as an operation's own work + runs. An awareness drain finishes its deliveries, each within its own + deadline, and a puppet call waits for its tool, so a tool on an MCPL + server that never answers holds `stop()` until the request times out + (60 s by default, never with `requestTimeoutMs: 0`). + `stop(teardownTimeoutMs)` gives this wait its own budget, shared by all + its rounds, and then gives the modules' and MCPL's teardown the budget + again, so a budgeted `stop()` can take up to twice the budget, beside + the stream and event-loop waits that no budget bounds. An operation + that outlasts the budget fails `stop()` with a `ShutdownTimeoutError` that names it, on its own or inside an `AggregateError` when other cleanup failed too. `stop()` still tears down the rest, while that operation may still run, and keeps the store open, so `stop()` can be - called again. An awareness drain it waits for finishes its deliveries, - each within its own deadline. + called again. diff --git a/changelog.d/switch-branch.added.md b/changelog.d/switch-branch.added.md new file mode 100644 index 000000000..39779373e --- /dev/null +++ b/changelog.d/switch-branch.added.md @@ -0,0 +1,5 @@ +- `AgentFramework.switchBranch(name)` switches the store to an existing + branch, reconciles the Discord awareness markers with it, re-materializes + the config mount, and then resolves. The API server's branch switch used to + run these steps itself and now calls it. Once `stop()` has begun it + refuses, and `stop()` waits for one already running. diff --git a/src/api/server.ts b/src/api/server.ts index 58664705b..9cc1b53c5 100644 --- a/src/api/server.ts +++ b/src/api/server.ts @@ -536,20 +536,17 @@ export class ApiServer { throw new Error('name is required'); } - const store = this.framework.getStore(); const previousBranch = this.currentBranch; - store.switchBranch(params.name); - this.currentBranch = params.name; - - // Reversible Discord awareness markers are part of the branch projection. - // Do not acknowledge the switch until add/remove operations were attempted. - await this.framework.syncDiscordAwarenessMarkers(); - - // Materialize config files from the new branch - const ws = this.framework.getModule('workspace'); - if (ws && 'materializeMount' in ws) { - await (ws as any).materializeMount('_config'); + // The framework switches the store, reconciles the Discord awareness + // markers and re-materializes the config mount. Once stop() has begun it + // refuses, and stop() waits for a switch already running. + try { + await this.framework.switchBranch(params.name); + } finally { + // A later step can fail after the store has moved. + const store = this.framework.getStore(); + if (!store.isClosed()) this.currentBranch = store.currentBranch().name; } this.broadcast('branch:switched', { diff --git a/src/framework.ts b/src/framework.ts index 958201c1a..768e6e362 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -1142,6 +1142,10 @@ export class AgentFramework { * start. stop() waits until this set is empty before it tears down, unless * its wait budget runs out. */ private readonly operatorOperations = new Set<{ label: string; promise: Promise }>(); + /** Set once stop() has finished waiting on operatorOperations, whether the + * wait completed or timed out: an awareness sync, which is admitted after + * stop() has begun, is refused from here on (see syncDiscordAwarenessMarkers). */ + private operatorWaitEnded = false; private membrane: Membrane; private queue: ProcessQueueImpl; private agents: Map = new Map(); @@ -2278,19 +2282,21 @@ export class AgentFramework { await this.loopPromise; } - // Closing the queue closed admission, except for an awareness sync (see - // syncDiscordAwarenessMarkers). What admitted operations start now, such - // as a surgery's config materialization, joins the set, and so does such - // a sync, so wait until the set is empty. This comes after the streams and - // the event loop, which a quiesce or a surgery may be waiting on. If the - // budget runs out, teardown goes on while the operation may still act on - // what it tears down, and the store stays open for another stop(). + // Closing the queue closed admission, except for an awareness sync, which + // is admitted until this wait ends (see syncDiscordAwarenessMarkers). What + // admitted operations start now, such as a surgery's config + // materialization, joins the set, and so does such a sync, so wait until + // the set is empty. This comes after the streams and the event loop, which + // a quiesce or a surgery may be waiting on. If the budget runs out, + // teardown goes on while the operation may still act on what it tears + // down, and the store stays open for another stop(). let operatorOperationsTimedOut: unknown; try { await this.waitForOperatorOperations(teardownTimeoutMs); } catch (error) { operatorOperationsTimedOut = error; } + this.operatorWaitEnded = true; // Stop typing indicators this.channelRegistry?.stopAll(); @@ -4010,8 +4016,9 @@ export class AgentFramework { // A pass already running is stop()'s to wait for; a new one isn't started. this.refuseOperatorAfterStop('maintain'); if (!this.running) { - // startQueuedMaintenance no-ops on a stopped/never-started framework — - // a bare snapshot would masquerade as a completed pass, so say so. + // startQueuedMaintenance no-ops on a framework that was never started + // (a stopping one is refused above): a bare snapshot would masquerade + // as a completed pass, so say so. console.warn('[host-mode] maintenanceTick: framework is not running — no pass executed'); return { ...this.getContextMaintenanceSnapshot(), ran: false }; } @@ -5793,7 +5800,7 @@ export class AgentFramework { return { ok: true, undone: 0, requested, lastVisible: null }; } - await this.runDiscordAwarenessSync(); + await this.syncDiscordAwarenessMarkers(); return { ok: true, undone, requested, lastVisible: await this.lastVisiblePreview(agentName) }; } @@ -6097,9 +6104,32 @@ export class AgentFramework { } // ========================================================================== - // Undo / Redo + // Branch switch, Undo / Redo // ========================================================================== + /** + * Switch the store to an existing branch, reconcile Discord awareness + * markers with it, and re-materialize the config mount from it. Resolves + * once all three have run. An operator's operation: refused once stop() has + * begun, and waited for by stop() while it runs. + */ + async switchBranch(name: string): Promise { + this.refuseOperatorAfterStop('switch branch'); + return this.trackOperatorOperation('branch:switch', () => this.runSwitchBranch(name)); + } + + /** switchBranch's work, run where stop() can wait for it. */ + private async runSwitchBranch(name: string): Promise { + this.store.switchBranch(name); + // Reversible Discord awareness markers are part of the branch projection. + // Don't acknowledge the switch until add/remove operations were attempted. + await this.syncDiscordAwarenessMarkers(); + const ws = this.moduleRegistry.getModule('workspace'); + if (ws && 'materializeMount' in ws) { + await (ws as { materializeMount: (m: string) => Promise }).materializeMount('_config'); + } + } + /** * Undo the last inference turn for an agent. * @@ -6482,7 +6512,7 @@ export class AgentFramework { ); } if (markerBatch) this.discordAwarenessOutbox!.activate(markerBatch.id); - await this.runDiscordAwarenessSync(); + await this.syncDiscordAwarenessMarkers(); this.materializeConfigMountAfterBranchSwitch(); } finally { release(); @@ -6655,7 +6685,7 @@ export class AgentFramework { { cause: error }, ); } - await this.runDiscordAwarenessSync(); + await this.syncDiscordAwarenessMarkers(); this.materializeConfigMountAfterBranchSwitch(); } finally { release(); @@ -14010,12 +14040,14 @@ export class AgentFramework { } /** Reconcile the durable ledger with Chronicle, then deliver every server's work. - * Unlike the other operator entry points, this one isn't refused once stop() - * has begun: its callers, such as the API server's branch switch, have - * already changed the store and call it to finish that change. stop() waits - * for one that starts while stop() is waiting. One that starts later can - * fail to deliver, and the outbox keeps that work for the next start. */ - syncDiscordAwarenessMarkers(onlyServerId?: string): Promise { + * Unlike the other operator entry points, this one is admitted after stop() + * has begun, until stop() has finished waiting on the operator set: its + * callers, such as the API server's undo and redo, have already changed the + * store and call it to finish that change, sometimes from inside a trace + * listener that started stop(). stop() waits for one admitted then. Once + * stop() can no longer wait for it, it's refused. */ + async syncDiscordAwarenessMarkers(onlyServerId?: string): Promise { + if (this.operatorWaitEnded) throw new Error('awareness sync refused: the framework is stopping'); return this.trackOperatorOperation('awareness:sync', () => this.runDiscordAwarenessSync(onlyServerId)); } diff --git a/test/stop-with-host-operations.test.ts b/test/stop-with-host-operations.test.ts index 0d38a6812..56c77c1a0 100644 --- a/test/stop-with-host-operations.test.ts +++ b/test/stop-with-host-operations.test.ts @@ -2,9 +2,10 @@ * stop() and an operator's operations. * * Once stop() has begun, an operator's entry points refuse: quiesce, resume, - * the store surgeries, undo, redo, maintain, puppet and the MCPL lifecycle - * calls (a host/command is refused by its listener). The awareness sync is - * the exception, since its callers have already changed the store. These + * the store surgeries, a branch switch, undo, redo, maintain, puppet and the + * MCPL lifecycle calls (a host/command is refused by its listener). The + * awareness sync is the exception: it's admitted until stop() has finished + * waiting, since its callers have already changed the store. These * tests cover those refusals and the operations admitted before them, over * MCPL or from a direct call: stop() waits for each, and for what each starts * while it waits, before the event gate, the modules, the MCPL connections @@ -24,6 +25,7 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import type { EventResponse, Module, ModuleContext, ToolDefinition, ToolResult } from '../src/index.js'; import { AgentFramework, ShutdownTimeoutError } from '../src/index.js'; +import { ApiServer } from '../src/api/server.js'; import { MockMembrane } from './helpers/mock-membrane.js'; /** Resolve once `cond` holds; fail, naming `what`, after `ms`. */ @@ -372,6 +374,29 @@ describe('stop() with an operator operation in flight', () => { assert.deepEqual(rejections, []); }); + it('an awareness sync called once stop() has finished waiting is refused, and installs no barrier', async () => { + const framework = await create(); + let barriers = 0; + internals(framework).beginDiscordAwarenessBarrier = () => { + barriers++; + return { requiresBarrier: false, promise: Promise.resolve({ status: 'delivered', delivered: 0, failed: 0 }) }; + }; + let finishModules!: () => void; + const hold = new Promise((resolve) => { finishModules = resolve; }); + framework.getModule('teardown-probe')!.stop = async () => { + order.push('modules stopped'); + await hold; + }; + const stopping = framework.stop(); + try { + await until(() => order.includes('modules stopped'), 'module teardown'); + await assert.rejects(framework.syncDiscordAwarenessMarkers(), /^Error: awareness sync refused: the framework is stopping$/); + assert.equal(barriers, 0); + } finally { finishModules(); await stopping; } + await assert.rejects(framework.syncDiscordAwarenessMarkers(), /^Error: awareness sync refused: the framework is stopping$/); + assert.equal(barriers, 0); + }); + it("an operation that outlasts stop()'s budget fails the stop and keeps the store open for a retry", async () => { const framework = await create(); let deliver: (() => void) | undefined; @@ -409,6 +434,7 @@ describe('stop() with an operator operation in flight', () => { ['redo', (f) => f.redo('agent')], ['maintain', (f) => f.maintenanceTick()], ['puppet', (f) => f.puppetToolCall('agent', 'unused', {})], + ['switch branch', (f) => f.switchBranch('main')], ['connect MCPL server', (f) => f.connectMcplServer({ id: 'late', command: 'unused' })], ['disconnect MCPL server', (f) => f.disconnectMcplServer('late')], ['restart MCPL server', (f) => f.restartMcplServer('late')], @@ -536,6 +562,14 @@ describe('stop() with an operator operation in flight', () => { order.push('tool returned'); return { success: true, data: 'done' }; }; + // The pair's two writes are what the wait protects. + const cm = framework.getAgent('agent')!.getContextManager(); + const addMessage = cm.addMessage.bind(cm); + cm.addMessage = ((...args: Parameters) => { + const id = addMessage(...args); + order.push('message stored'); + return id; + }) as typeof cm.addMessage; const puppeting = framework.puppetToolCall('agent', 'probe', {}); void puppeting.catch(() => {}); await until(() => called, 'the puppet call to reach its tool'); @@ -544,10 +578,78 @@ describe('stop() with an operator operation in flight', () => { const { toolUseId } = await puppeting; await stopping; assert.match(toolUseId, /^toolu_01/); - assert.deepEqual(order, ['tool returned', 'modules stopped']); + assert.deepEqual(order, ['tool returned', 'message stored', 'message stored', 'modules stopped']); assert.deepEqual(rejections, []); }); + it("stop() waits for a branch switch from the API server, its config mount included", { timeout: 20_000 }, async () => { + const framework = await create(); + const api = new ApiServer(framework) as unknown as { + cmdBranchCreate(params: { name: string }): Promise; + cmdBranchSwitch(params: { name: string }): Promise; + }; + await api.cmdBranchCreate({ name: 'side' }); + const fw = internals(framework); + let release!: () => void; + const held = new Promise((resolve) => { release = resolve; }); + let mounting = false; + const getModule = fw.moduleRegistry.getModule.bind(fw.moduleRegistry); + fw.moduleRegistry.getModule = (name) => name === 'workspace' ? { + materializeMount: async () => { mounting = true; await held; order.push('config materialized'); }, + } : getModule(name); + const switching = api.cmdBranchSwitch({ name: 'side' }); + void switching.catch(() => {}); + await until(() => mounting, 'the switch to reach its config mount'); + const stopping = framework.stop(); + await releaseWhenStopWaits(release); + await switching; + await stopping; + assert.deepEqual(order, ['config materialized', 'modules stopped']); + assert.deepEqual(rejections, []); + }); + + it("the API server's current branch follows the store when a switch fails after moving it", async () => { + const framework = await create(); + const api = new ApiServer(framework) as unknown as { + cmdBranchCreate(params: { name: string }): Promise; + cmdBranchSwitch(params: { name: string }): Promise; + cmdBranchCurrent(): Promise<{ name: string }>; + }; + await api.cmdBranchCreate({ name: 'side' }); + const fw = internals(framework); + const getModule = fw.moduleRegistry.getModule.bind(fw.moduleRegistry); + fw.moduleRegistry.getModule = (name) => name === 'workspace' ? { + materializeMount: async () => { throw new Error('disk full'); }, + } : getModule(name); + await assert.rejects(api.cmdBranchSwitch({ name: 'side' }), /disk full/); + assert.equal(framework.getStore().currentBranch().name, 'side'); + assert.equal((await api.cmdBranchCurrent()).name, 'side'); + await framework.stop(); + }); + + it('a branch switch from the API server once stop() has begun is refused, and the branch stays', async () => { + const framework = await create(); + const api = new ApiServer(framework) as unknown as { + cmdBranchCreate(params: { name: string }): Promise; + cmdBranchSwitch(params: { name: string }): Promise; + cmdBranchCurrent(): Promise<{ name: string }>; + }; + await api.cmdBranchCreate({ name: 'side' }); + let finishModules!: () => void; + const hold = new Promise((resolve) => { finishModules = resolve; }); + framework.getModule('teardown-probe')!.stop = async () => { + order.push('modules stopped'); + await hold; + }; + const stopping = framework.stop(); + try { + await until(() => order.includes('modules stopped'), 'module teardown'); + await assert.rejects(api.cmdBranchSwitch({ name: 'side' }), /^Error: switch branch refused: the framework is stopping$/); + assert.equal(framework.getStore().currentBranch().name, 'main'); + assert.equal((await api.cmdBranchCurrent()).name, 'main'); + } finally { finishModules(); await stopping; } + }); + for (const action of ['connect', 'disconnect', 'restart'] as const) { it('stop() waits for an MCPL ' + action + ' already running before it closes the connections', async () => { const framework = await create(); From 0b9dc1937751cf665f2c251dc91abbf337b3a0ee Mon Sep 17 00:00:00 2001 From: Nell-1783 Date: Tue, 13 Oct 2026 11:18:42 -0400 Subject: [PATCH 09/21] fix(framework): switchBranch() reserves the store; admitted operations run their own awareness sync From a read of the previous commit. - An operation already admitted calls the private runDiscordAwarenessSync() again, and only the public syncDiscordAwarenessMarkers() is refused once stop() has finished waiting. That covers a surgery's sync, a host/command undo's, and switchBranch()'s. With the previous commit, a rollback or suppression admitted before a stop() whose budget ran out reached its sync after the wait had ended, and was refused there, after its branch had moved. The operator saw a failure for a change that had happened. The private call is covered by the enclosing operation's tracking. The host/command undo and switchBranch() call their sync with no await before it, so it can't reach the refusal today. They call the private one too, so that there's one rule and an await added later can't reintroduce the failure. - switchBranch() reserves the store as rollbackToMessage() and suppressMessages() do, since it moves the branch under every agent sharing the store. Every agent must be idle, or it refuses with OperatorActionError('agent-busy'), and no turn starts until the switch, the sync and the mount are done. The reservation is also what stop() waits for, replacing the separate tracked wrapper. The API server's branch switch, which calls it, now refuses during a live turn instead of moving the branch beneath it. Its old path had no gate. Tests: a rollback and a suppression whose branch move is held past stop(100)'s budget still finish, their sync included, and the next stop() closes the store. A branch switch during a live turn is refused, both directly and through the API server, and the branch stays. Checks (VPS, Node 24.21.0, transpiler cache off): typecheck and build clean; stop-with-host-operations 48/48 unforced; operator-surgery 18/18, discord-awareness-outbox 6/6, host-quiesce 31/31. With these tests and the previous commit's src, the three new cases fail, each at its own assertion (the two timeout cases with "awareness sync refused", the live-turn case with a missing rejection). Co-Authored-By: Claude Opus 5.5 --- ...top-waits-for-operator-operations.fixed.md | 4 +- changelog.d/switch-branch.added.md | 11 ++-- src/framework.ts | 41 +++++++++------ test/stop-with-host-operations.test.ts | 51 +++++++++++++++++++ 4 files changed, 87 insertions(+), 20 deletions(-) diff --git a/changelog.d/stop-waits-for-operator-operations.fixed.md b/changelog.d/stop-waits-for-operator-operations.fixed.md index c872b53e0..77857eae3 100644 --- a/changelog.d/stop-waits-for-operator-operations.fixed.md +++ b/changelog.d/stop-waits-for-operator-operations.fixed.md @@ -24,7 +24,9 @@ the store when they call it: the API server's undo and redo call it after `undoLastTurn()` or `redo()`, sometimes from inside a trace listener that started `stop()`. `stop()` waits for a sync admitted then, - and refuses one that starts once it can no longer wait for it. + and refuses one that starts once it can no longer wait for it. An + operation already admitted runs its own sync without that check, so a + surgery whose branch move outlasted a budget still finishes. - `stop()` also waits for what those operations start while it waits: the config materialization that undo, redo and the two surgeries start without waiting for it themselves. The framework now logs a failed diff --git a/changelog.d/switch-branch.added.md b/changelog.d/switch-branch.added.md index 39779373e..dc08a256d 100644 --- a/changelog.d/switch-branch.added.md +++ b/changelog.d/switch-branch.added.md @@ -1,5 +1,10 @@ - `AgentFramework.switchBranch(name)` switches the store to an existing branch, reconciles the Discord awareness markers with it, re-materializes - the config mount, and then resolves. The API server's branch switch used to - run these steps itself and now calls it. Once `stop()` has begun it - refuses, and `stop()` waits for one already running. + the config mount, and then resolves. The branch moves under every agent + sharing the store, so, as for `rollbackToMessage()`, every agent must be + idle: during a live turn it refuses with `OperatorActionError` + (`agent-busy`), and no turn starts until it's done. Once `stop()` has + begun it refuses, and `stop()` waits for one already running. + - The API server's branch switch used to run these steps itself, on the + store, and now calls `switchBranch()`. So it refuses during a live turn + instead of moving the branch beneath it. diff --git a/src/framework.ts b/src/framework.ts index 768e6e362..75987e9bf 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -5800,7 +5800,7 @@ export class AgentFramework { return { ok: true, undone: 0, requested, lastVisible: null }; } - await this.syncDiscordAwarenessMarkers(); + await this.runDiscordAwarenessSync(); return { ok: true, undone, requested, lastVisible: await this.lastVisiblePreview(agentName) }; } @@ -6112,21 +6112,26 @@ export class AgentFramework { * markers with it, and re-materialize the config mount from it. Resolves * once all three have run. An operator's operation: refused once stop() has * begun, and waited for by stop() while it runs. + * + * The branch moves under every agent sharing the store, so the switch + * reserves the store as a surgery does: every agent must be idle, or it is + * refused (OperatorActionError 'agent-busy'), and no turn starts until all + * three steps are done. */ async switchBranch(name: string): Promise { this.refuseOperatorAfterStop('switch branch'); - return this.trackOperatorOperation('branch:switch', () => this.runSwitchBranch(name)); - } - - /** switchBranch's work, run where stop() can wait for it. */ - private async runSwitchBranch(name: string): Promise { - this.store.switchBranch(name); - // Reversible Discord awareness markers are part of the branch projection. - // Don't acknowledge the switch until add/remove operations were attempted. - await this.syncDiscordAwarenessMarkers(); - const ws = this.moduleRegistry.getModule('workspace'); - if (ws && 'materializeMount' in ws) { - await (ws as { materializeMount: (m: string) => Promise }).materializeMount('_config'); + const release = this.reserveStoreForSurgery('switch branch', 'every agent'); + try { + this.store.switchBranch(name); + // Reversible Discord awareness markers are part of the branch projection. + // Don't acknowledge the switch until add/remove operations were attempted. + await this.runDiscordAwarenessSync(); + const ws = this.moduleRegistry.getModule('workspace'); + if (ws && 'materializeMount' in ws) { + await (ws as { materializeMount: (m: string) => Promise }).materializeMount('_config'); + } + } finally { + release(); } } @@ -6512,7 +6517,7 @@ export class AgentFramework { ); } if (markerBatch) this.discordAwarenessOutbox!.activate(markerBatch.id); - await this.syncDiscordAwarenessMarkers(); + await this.runDiscordAwarenessSync(); this.materializeConfigMountAfterBranchSwitch(); } finally { release(); @@ -6685,7 +6690,7 @@ export class AgentFramework { { cause: error }, ); } - await this.syncDiscordAwarenessMarkers(); + await this.runDiscordAwarenessSync(); this.materializeConfigMountAfterBranchSwitch(); } finally { release(); @@ -14045,7 +14050,11 @@ export class AgentFramework { * callers, such as the API server's undo and redo, have already changed the * store and call it to finish that change, sometimes from inside a trace * listener that started stop(). stop() waits for one admitted then. Once - * stop() can no longer wait for it, it's refused. */ + * stop() can no longer wait for it, it's refused. An operation already + * admitted (a surgery, a host/command undo, a branch switch) calls + * runDiscordAwarenessSync() instead, which its own tracking covers, so a + * stop() whose budget ran out doesn't refuse it partway, after its branch + * has moved. */ async syncDiscordAwarenessMarkers(onlyServerId?: string): Promise { if (this.operatorWaitEnded) throw new Error('awareness sync refused: the framework is stopping'); return this.trackOperatorOperation('awareness:sync', () => this.runDiscordAwarenessSync(onlyServerId)); diff --git a/test/stop-with-host-operations.test.ts b/test/stop-with-host-operations.test.ts index 56c77c1a0..5d3fbc6bd 100644 --- a/test/stop-with-host-operations.test.ts +++ b/test/stop-with-host-operations.test.ts @@ -397,6 +397,37 @@ describe('stop() with an operator operation in flight', () => { assert.equal(barriers, 0); }); + for (const action of ['rollback', 'suppress'] as const) it('a ' + action + " whose branch move outlasts stop()'s budget still finishes, its awareness sync included", { timeout: 20_000 }, async () => { + // A stop() whose budget ran out has finished waiting, and a sync called + // from outside is refused from then on; one inside an operation already + // admitted isn't, since its branch has moved. + const framework = await create(); + const cm = framework.getAgent('agent')!.getContextManager(); + const ids = [1, 2, 3].map((n) => cm.addMessage('user', [{ type: 'text', text: 'm' + n }])); + let finishMove!: () => void; + const moveHold = new Promise((resolve) => { finishMove = resolve; }); + if (action === 'rollback') { + const switchBranch = cm.switchBranch.bind(cm); + cm.switchBranch = async (...args: Parameters) => { await moveHold; return switchBranch(...args); }; + } else { + const fork = cm.fork.bind(cm); + cm.fork = async (...args: Parameters) => { await moveHold; return fork(...args); }; + } + const surgery = action === 'rollback' + ? framework.rollbackToMessage('agent', { messageId: ids[0]! }) + : framework.suppressMessages('agent', { messageIds: [ids[1]!] }); + void surgery.catch(() => {}); + try { + await assert.rejects(framework.stop(100), (error: unknown) => + error instanceof ShutdownTimeoutError && error.pending.includes('surgery:' + (action === 'rollback' ? 'roll back' : 'suppress'))); + } finally { finishMove(); } + const result = await surgery; + assert.ok(result.messagesRemoved > 0, JSON.stringify(result)); + await framework.stop(); + assert.equal(framework.getStore().isClosed(), true); + assert.deepEqual(rejections, []); + }); + it("an operation that outlasts stop()'s budget fails the stop and keeps the store open for a retry", async () => { const framework = await create(); let deliver: (() => void) | undefined; @@ -627,6 +658,26 @@ describe('stop() with an operator operation in flight', () => { await framework.stop(); }); + it('a branch switch during a live turn is refused, and the branch stays', async () => { + const framework = await create(); + const api = new ApiServer(framework) as unknown as { + cmdBranchCreate(params: { name: string }): Promise; + cmdBranchSwitch(params: { name: string }): Promise; + cmdBranchCurrent(): Promise<{ name: string }>; + }; + await api.cmdBranchCreate({ name: 'side' }); + // A turn holds its token from dequeue, before its stream registers. + const tokens = internals(framework).activeTurnTokens; + tokens.set('agent', -1); + try { + await assert.rejects(framework.switchBranch('side'), /^OperatorActionError: Cannot switch branch while agent is idle\+turn-alive/); + await assert.rejects(api.cmdBranchSwitch({ name: 'side' }), /Cannot switch branch while agent is idle\+turn-alive/); + assert.equal(framework.getStore().currentBranch().name, 'main'); + assert.equal((await api.cmdBranchCurrent()).name, 'main'); + } finally { tokens.delete('agent'); } + await framework.stop(); + }); + it('a branch switch from the API server once stop() has begun is refused, and the branch stays', async () => { const framework = await create(); const api = new ApiServer(framework) as unknown as { From 439129258fa881c26e9aef089db288de1f6aa8d4 Mon Sep 17 00:00:00 2001 From: Nell-1783 Date: Tue, 13 Oct 2026 11:42:55 -0400 Subject: [PATCH 10/21] fix(framework): admit an awareness sync per stop() attempt; keep the API server's branch in step with a switch From the review of the switchBranch() round. - The awareness sync's admission is now a window for each stop() attempt (stopWaitPending), not a one-way latch. Each attempt opens it at its start, and closes it in the same step as its wait's last look at the operator set (a finally in waitForOperatorOperations), whether the wait completed or timed out. Before, the latch was set by the first attempt and never cleared, so a stop() retried after a timeout refused a sync its own wait would have covered. It was also set one await after the loop's last check, so a sync could join the set unseen in that step. The sync is refused while the queue is closed and no attempt's wait is open, as the other entry points are after a failed or completed stop(). No test pins the single-step gap: making it observable would mean scheduling a microtask between two awaits inside stop(). - switchBranch()'s doc now states what it guarantees: the store has moved by the time the call returns, before its promise settles, a refusal moves nothing, and if the sync or the mount fails it rejects with the store moved. The API server's branch switch reads the store's branch right after the call. Before, it read it only once the whole switch had settled, so during the awaited sync and mount it reported the old branch, and its delete guard would pass for the branch the store was on. It also broadcasts branch:switched whenever the store moved, even if a later step failed, as well as on success. - Text: the added fragment says the switch can reject with the branch moved, and how the API server's current branch and broadcast follow it. The fixed fragment says each stop() attempt admits the sync. A test name and a test comment no longer describe the API's switch as calling the sync itself. Tests: a stop() retried after a timeout admits a sync during its own wait and waits for it. While a switch is held in its mount, the API server reports the new branch and refuses to delete it. A switch whose mount fails leaves the API on the new branch, and clients hear of the move. switchBranch() reconciles the markers on the branch it moved to. The API's refused switch is refused with the store closed too. The failure-path test stops its framework in a finally. Checks (VPS, Node 24.21.0, transpiler cache off): typecheck and build clean; unforced, stop-with-host-operations 51/51, operator-surgery 18/18, discord-awareness-outbox 6/6, host-quiesce 31/31, puppet-tool-call 10/10, api-admin-gate 4/4. With these tests and the previous commit's src, three fail, each at its own assertion: the retried stop()'s sync is refused, the API reports the old branch during the switch, and no broadcast follows a failed switch. The markers and closed-store cases pass there, since they pin behaviour that commit already had. Co-Authored-By: Claude Opus 5.5 --- ...top-waits-for-operator-operations.fixed.md | 17 +-- changelog.d/switch-branch.added.md | 7 +- src/api/server.ts | 35 +++-- src/framework.ts | 61 +++++--- test/stop-with-host-operations.test.ts | 131 +++++++++++++++--- 5 files changed, 188 insertions(+), 63 deletions(-) diff --git a/changelog.d/stop-waits-for-operator-operations.fixed.md b/changelog.d/stop-waits-for-operator-operations.fixed.md index 77857eae3..1cf41f107 100644 --- a/changelog.d/stop-waits-for-operator-operations.fixed.md +++ b/changelog.d/stop-waits-for-operator-operations.fixed.md @@ -19,14 +19,15 @@ `disconnectMcplServer()` and `restartMcplServer()` refuse with " refused: the framework is stopping" before they read or change anything, as a `host/command` already did. - - The exception is `syncDiscordAwarenessMarkers()`, which `stop()` admits - until it has finished waiting, because its callers have already changed - the store when they call it: the API server's undo and redo call it - after `undoLastTurn()` or `redo()`, sometimes from inside a trace - listener that started `stop()`. `stop()` waits for a sync admitted then, - and refuses one that starts once it can no longer wait for it. An - operation already admitted runs its own sync without that check, so a - surgery whose branch move outlasted a budget still finishes. + - The exception is `syncDiscordAwarenessMarkers()`, which each `stop()` + attempt admits until it has finished waiting, because its callers have + already changed the store when they call it: the API server's undo and + redo call it after `undoLastTurn()` or `redo()`, sometimes from inside a + trace listener that started `stop()`. `stop()` waits for a sync + admitted then, and refuses one that starts once it can no longer wait + for it. An operation already admitted runs its own sync without that + check, so a surgery whose branch move outlasted a budget still + finishes. - `stop()` also waits for what those operations start while it waits: the config materialization that undo, redo and the two surgeries start without waiting for it themselves. The framework now logs a failed diff --git a/changelog.d/switch-branch.added.md b/changelog.d/switch-branch.added.md index dc08a256d..22c7abcab 100644 --- a/changelog.d/switch-branch.added.md +++ b/changelog.d/switch-branch.added.md @@ -1,10 +1,13 @@ - `AgentFramework.switchBranch(name)` switches the store to an existing branch, reconciles the Discord awareness markers with it, re-materializes - the config mount, and then resolves. The branch moves under every agent + the config mount, and then resolves. If the sync or the mount fails, it + rejects with the branch already moved. The branch moves under every agent sharing the store, so, as for `rollbackToMessage()`, every agent must be idle: during a live turn it refuses with `OperatorActionError` (`agent-busy`), and no turn starts until it's done. Once `stop()` has begun it refuses, and `stop()` waits for one already running. - The API server's branch switch used to run these steps itself, on the store, and now calls `switchBranch()`. So it refuses during a live turn - instead of moving the branch beneath it. + instead of moving the branch beneath it. Its current branch follows the + store from the moment it moves, and clients hear of a move even when a + later step fails. diff --git a/src/api/server.ts b/src/api/server.ts index 9cc1b53c5..ed60e058a 100644 --- a/src/api/server.ts +++ b/src/api/server.ts @@ -539,24 +539,37 @@ export class ApiServer { const previousBranch = this.currentBranch; // The framework switches the store, reconciles the Discord awareness - // markers and re-materializes the config mount. Once stop() has begun it - // refuses, and stop() waits for a switch already running. + // markers and re-materializes the config mount. It refuses during a live + // turn or once stop() has begun, and stop() waits for one already running. + // The store has moved when the call returns, before its promise settles + // (a refusal moves nothing), so the current branch follows it through the + // awaited steps. + const switching = this.framework.switchBranch(params.name); + this.followStoreBranch(); + let switched = false; try { - await this.framework.switchBranch(params.name); + await switching; + switched = true; } finally { - // A later step can fail after the store has moved. - const store = this.framework.getStore(); - if (!store.isClosed()) this.currentBranch = store.currentBranch().name; + // A later step can fail after the store has moved: clients still hear + // where it is. + if (switched || this.currentBranch !== previousBranch) { + this.broadcast('branch:switched', { + from: previousBranch, + to: this.currentBranch, + }); + } } - this.broadcast('branch:switched', { - from: previousBranch, - to: params.name, - }); - return { switched: true }; } + /** Take the current branch from the store, while it's open. */ + private followStoreBranch(): void { + const store = this.framework.getStore(); + if (!store.isClosed()) this.currentBranch = store.currentBranch().name; + } + private async cmdBranchCurrent(): Promise<{ name: string }> { return { name: this.currentBranch }; } diff --git a/src/framework.ts b/src/framework.ts index 75987e9bf..66270c801 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -1142,10 +1142,11 @@ export class AgentFramework { * start. stop() waits until this set is empty before it tears down, unless * its wait budget runs out. */ private readonly operatorOperations = new Set<{ label: string; promise: Promise }>(); - /** Set once stop() has finished waiting on operatorOperations, whether the - * wait completed or timed out: an awareness sync, which is admitted after - * stop() has begun, is refused from here on (see syncDiscordAwarenessMarkers). */ - private operatorWaitEnded = false; + /** True from the start of each stop() attempt until it has finished waiting + * on operatorOperations, whether the wait completed or timed out: the window + * in which an awareness sync is admitted although the queue is closed (see + * syncDiscordAwarenessMarkers). */ + private stopWaitPending = false; private membrane: Membrane; private queue: ProcessQueueImpl; private agents: Map = new Map(); @@ -2191,27 +2192,37 @@ export class AgentFramework { /** Wait in rounds until the set is empty: admitted work can still start work * of its own, and an awareness sync can still join. All rounds share the - * caller's deadline rather than renewing its budget. */ + * caller's deadline rather than renewing its budget. Ends the window in + * which a sync is admitted (stopWaitPending). */ private async waitForOperatorOperations(timeoutMs: number | undefined): Promise { const deadline = timeoutMs === undefined ? undefined : performance.now() + timeoutMs; - while (this.operatorOperations.size > 0) { - const remaining = deadline === undefined ? undefined : Math.ceil(deadline - performance.now()); - const timedOut = () => new ShutdownTimeoutError( - [...this.operatorOperations].map(({ label }) => label), timeoutMs!, [], - ); - if (remaining !== undefined && remaining < 1) throw timedOut(); - try { - await waitForShutdown([...this.operatorOperations], remaining); - } catch (error) { - // Report the original budget and everything still pending, including - // child work added after this round's snapshot. - if (error instanceof ShutdownTimeoutError) throw timedOut(); - throw error; + try { + while (this.operatorOperations.size > 0) { + const remaining = deadline === undefined ? undefined : Math.ceil(deadline - performance.now()); + const timedOut = () => new ShutdownTimeoutError( + [...this.operatorOperations].map(({ label }) => label), timeoutMs!, [], + ); + if (remaining !== undefined && remaining < 1) throw timedOut(); + try { + await waitForShutdown([...this.operatorOperations], remaining); + } catch (error) { + // Report the original budget and everything still pending, including + // child work added after this round's snapshot. + if (error instanceof ShutdownTimeoutError) throw timedOut(); + throw error; + } } + } finally { + // In the same step as the last look at the set, so no awareness sync + // can join it unseen. + this.stopWaitPending = false; } } private async stopOwned(teardownTimeoutMs: number | undefined): Promise { + // An awareness sync is admitted until this attempt has finished waiting on + // the operator set (waitForOperatorOperations ends the window). + this.stopWaitPending = true; this.pushCoalescer?.suspend(); this.flushCoalescingSnapshot(); this.running = false; @@ -2296,7 +2307,6 @@ export class AgentFramework { } catch (error) { operatorOperationsTimedOut = error; } - this.operatorWaitEnded = true; // Stop typing indicators this.channelRegistry?.stopAll(); @@ -6110,8 +6120,11 @@ export class AgentFramework { /** * Switch the store to an existing branch, reconcile Discord awareness * markers with it, and re-materialize the config mount from it. Resolves - * once all three have run. An operator's operation: refused once stop() has - * begun, and waited for by stop() while it runs. + * once all three have run; if the sync or the mount fails, it rejects with + * the store already moved. The store has moved by the time the call + * returns, before its promise settles, and a refusal moves nothing. An + * operator's operation: refused once stop() has begun, and waited for by + * stop() while it runs. * * The branch moves under every agent sharing the store, so the switch * reserves the store as a surgery does: every agent must be idle, or it is @@ -14050,13 +14063,15 @@ export class AgentFramework { * callers, such as the API server's undo and redo, have already changed the * store and call it to finish that change, sometimes from inside a trace * listener that started stop(). stop() waits for one admitted then. Once - * stop() can no longer wait for it, it's refused. An operation already + * stop() can no longer wait for it, it's refused, and so it is while no + * stop() is running on a closed queue (after a failed or completed stop()), + * as the other entry points are. An operation already * admitted (a surgery, a host/command undo, a branch switch) calls * runDiscordAwarenessSync() instead, which its own tracking covers, so a * stop() whose budget ran out doesn't refuse it partway, after its branch * has moved. */ async syncDiscordAwarenessMarkers(onlyServerId?: string): Promise { - if (this.operatorWaitEnded) throw new Error('awareness sync refused: the framework is stopping'); + if (this.queue?.isClosed && !this.stopWaitPending) throw new Error('awareness sync refused: the framework is stopping'); return this.trackOperatorOperation('awareness:sync', () => this.runDiscordAwarenessSync(onlyServerId)); } diff --git a/test/stop-with-host-operations.test.ts b/test/stop-with-host-operations.test.ts index 5d3fbc6bd..54199bdd9 100644 --- a/test/stop-with-host-operations.test.ts +++ b/test/stop-with-host-operations.test.ts @@ -314,7 +314,7 @@ describe('stop() with an operator operation in flight', () => { assert.deepEqual(rejections, []); }); - it("stop() waits for an awareness sync, which an operator's undo or branch switch runs", async () => { + it('stop() waits for an awareness sync already running', async () => { const framework = await create(); let deliver!: () => void; internals(framework).beginDiscordAwarenessBarrier = () => ({ @@ -334,8 +334,8 @@ describe('stop() with an operator operation in flight', () => { }); it('an awareness sync called while stop() waits is not refused, and stop() waits for it too', { timeout: 20_000 }, async () => { - // The API server moves the branch on the store itself and then syncs, so - // a sync that arrives during stop() finishes a change already made. + // The API server's undo and redo change the store and then sync, so a + // sync that arrives during stop() finishes a change already made. const framework = await create(); const fw = framework as unknown as Internals & { refreshMcplTools(): Promise; emitMcplToolDiff(...args: unknown[]): void }; let releaseFirst!: () => void; @@ -374,6 +374,48 @@ describe('stop() with an operator operation in flight', () => { assert.deepEqual(rejections, []); }); + it("a retried stop() admits an awareness sync during its own wait, and waits for it", { timeout: 20_000 }, async () => { + const framework = await create(); + const fw = framework as unknown as Internals & { + getToolsForAgent(name: string): Array<{ name: string }>; + executeToolCall(call: unknown): Promise; + }; + // A puppet call held in its tool outlasts the first stop()'s budget. + let releaseTool!: () => void; + const toolHeld = new Promise((resolve) => { releaseTool = resolve; }); + let called = false; + fw.getToolsForAgent = () => [{ name: 'probe' }]; + fw.executeToolCall = async () => { called = true; await toolHeld; return { success: true, data: 'done' }; }; + const puppeting = framework.puppetToolCall('agent', 'probe', {}); + void puppeting.catch(() => {}); + await until(() => called, 'the puppet call to reach its tool'); + let deliver: (() => void) | undefined; + fw.beginDiscordAwarenessBarrier = () => ({ + requiresBarrier: true, + promise: new Promise((resolve) => { deliver = () => resolve({ status: 'delivered', delivered: 1, failed: 0 }); }), + }); + let syncSettled = false; + try { + await assert.rejects(framework.stop(100), (error: unknown) => error instanceof ShutdownTimeoutError); + const readsBefore = waitReads; + const retrying = framework.stop(); + void retrying.catch(() => {}); + await until(() => waitReads > readsBefore, 'the retry to wait on the puppet call'); + const syncing = framework.syncDiscordAwarenessMarkers().then(() => { syncSettled = true; }); + await until(() => deliver !== undefined, 'the sync to wait on its barrier'); + releaseTool(); + await puppeting; + await until(() => waitReads > readsBefore + 1, 'the retry to wait on the sync'); + assert.equal(framework.getStore().isClosed(), false, 'the retry waits for the sync'); + deliver!(); + await syncing; + await retrying; + } finally { releaseTool(); deliver?.(); } + assert.equal(syncSettled, true); + assert.equal(framework.getStore().isClosed(), true); + assert.deepEqual(rejections, []); + }); + it('an awareness sync called once stop() has finished waiting is refused, and installs no barrier', async () => { const framework = await create(); let barriers = 0; @@ -639,23 +681,71 @@ describe('stop() with an operator operation in flight', () => { assert.deepEqual(rejections, []); }); - it("the API server's current branch follows the store when a switch fails after moving it", async () => { + it("the API server's current branch follows the store when a switch fails after moving it, and clients hear of the move", async () => { const framework = await create(); - const api = new ApiServer(framework) as unknown as { - cmdBranchCreate(params: { name: string }): Promise; - cmdBranchSwitch(params: { name: string }): Promise; - cmdBranchCurrent(): Promise<{ name: string }>; - }; - await api.cmdBranchCreate({ name: 'side' }); - const fw = internals(framework); - const getModule = fw.moduleRegistry.getModule.bind(fw.moduleRegistry); - fw.moduleRegistry.getModule = (name) => name === 'workspace' ? { - materializeMount: async () => { throw new Error('disk full'); }, - } : getModule(name); - await assert.rejects(api.cmdBranchSwitch({ name: 'side' }), /disk full/); - assert.equal(framework.getStore().currentBranch().name, 'side'); - assert.equal((await api.cmdBranchCurrent()).name, 'side'); - await framework.stop(); + try { + const api = new ApiServer(framework) as unknown as { + cmdBranchCreate(params: { name: string }): Promise; + cmdBranchSwitch(params: { name: string }): Promise; + cmdBranchCurrent(): Promise<{ name: string }>; + broadcast(event: string, data: unknown): void; + }; + await api.cmdBranchCreate({ name: 'side' }); + const broadcasts: Array<[string, unknown]> = []; + api.broadcast = (event, data) => { broadcasts.push([event, data]); }; + const fw = internals(framework); + const getModule = fw.moduleRegistry.getModule.bind(fw.moduleRegistry); + fw.moduleRegistry.getModule = (name) => name === 'workspace' ? { + materializeMount: async () => { throw new Error('disk full'); }, + } : getModule(name); + await assert.rejects(api.cmdBranchSwitch({ name: 'side' }), /disk full/); + assert.equal(framework.getStore().currentBranch().name, 'side'); + assert.equal((await api.cmdBranchCurrent()).name, 'side'); + assert.deepEqual(broadcasts, [['branch:switched', { from: 'main', to: 'side' }]]); + } finally { await framework.stop(); } + }); + + it("the API server's current branch is the store's while a switch is still running", { timeout: 20_000 }, async () => { + const framework = await create(); + try { + const api = new ApiServer(framework) as unknown as { + cmdBranchCreate(params: { name: string }): Promise; + cmdBranchSwitch(params: { name: string }): Promise; + cmdBranchCurrent(): Promise<{ name: string }>; + cmdBranchDelete(params: { name: string }): Promise; + }; + await api.cmdBranchCreate({ name: 'side' }); + const fw = internals(framework); + let release!: () => void; + const held = new Promise((resolve) => { release = resolve; }); + let mounting = false; + const getModule = fw.moduleRegistry.getModule.bind(fw.moduleRegistry); + fw.moduleRegistry.getModule = (name) => name === 'workspace' ? { + materializeMount: async () => { mounting = true; await held; }, + } : getModule(name); + const switching = api.cmdBranchSwitch({ name: 'side' }); + try { + await until(() => mounting, 'the switch to reach its config mount'); + assert.equal((await api.cmdBranchCurrent()).name, 'side'); + await assert.rejects(api.cmdBranchDelete({ name: 'side' }), /Cannot delete current branch/); + } finally { release(); } + await switching; + } finally { await framework.stop(); } + }); + + it('switchBranch() reconciles the awareness markers on the branch it moved to', async () => { + const framework = await create(); + try { + const api = new ApiServer(framework) as unknown as { cmdBranchCreate(params: { name: string }): Promise }; + await api.cmdBranchCreate({ name: 'side' }); + const reconciledOn: string[] = []; + internals(framework).beginDiscordAwarenessBarrier = () => { + reconciledOn.push(framework.getStore().currentBranch().name); + return { requiresBarrier: false, promise: Promise.resolve({ status: 'delivered', delivered: 0, failed: 0 }) }; + }; + await framework.switchBranch('side'); + assert.deepEqual(reconciledOn, ['side']); + } finally { await framework.stop(); } }); it('a branch switch during a live turn is refused, and the branch stays', async () => { @@ -699,6 +789,9 @@ describe('stop() with an operator operation in flight', () => { assert.equal(framework.getStore().currentBranch().name, 'main'); assert.equal((await api.cmdBranchCurrent()).name, 'main'); } finally { finishModules(); await stopping; } + // With the store closed too, the refusal is what the caller sees. + await assert.rejects(api.cmdBranchSwitch({ name: 'side' }), /^Error: switch branch refused: the framework is stopping$/); + assert.equal((await api.cmdBranchCurrent()).name, 'main'); }); for (const action of ['connect', 'disconnect', 'restart'] as const) { From eb7d978bbd7c1fb74c6a03f0a5acc4d9e2dfbe87 Mon Sep 17 00:00:00 2001 From: Nell-1783 Date: Tue, 13 Oct 2026 12:04:11 -0400 Subject: [PATCH 11/21] fix(framework): close a failed stop()'s sync window; undo and redo respect the store hold; a host undo finishes its turns From the review of the previous round. - The awareness sync's window is now a set of open stop() waits (openStopWaits), one per attempt. stop() adds its attempt's entry before it starts. The attempt's wait removes it in the same step as its last look at the operator set. If the attempt fails before reaching its wait, its rejection handler removes it. Before, a stop() that threw after closing the queue but before its wait left the window open, so a sync was admitted that no stop() would wait for. One shared flag also let a second attempt, started by a trace listener inside the first one's synchronous start, close the window while the first still waited. - undoLastTurn() and redo() refuse while a surgery or a branch switch holds the store, as puppet calls and ephemeral agents already do. Before, an undo or redo could move the branch while a switch awaited its sync or mount, or while a rollback awaited its branch switch. - A multi-turn host/command undo calls a private runUndoLastTurn() for its turns, so a stop() begun by one turn's trace doesn't refuse the next. The undo, already admitted, finishes every turn it was asked for, and stop() waits for it. Before, it stopped after the first turn. - The API server's branch switch announces branch:switched only when its own call moved the store or succeeded, comparing the store's branch before and after the call, and names the store's branch as "from". Before, a refused switch whose cached branch was stale (after a rollback made outside the API server) announced a move that hadn't happened. The switch promise gets a handler before the bookkeeping runs, so a throw there can't leave its rejection unhandled. - Text: the fragments say that undo and redo refuse during a hold, that a multi-turn host undo finishes, that a refused switch announces nothing, and that a create with switchTo creates the branch before a switch that can be refused. Tests: a sync after a stop() that failed before its wait is refused. A two-turn host/command undo whose first turn starts stop() undoes both turns. Undo and redo are refused while a switch holds the store. A refused switch with a stale cache broadcasts nothing. After a timed-out wait, with no retry running, a sync is refused. The rollback and suppression timeout cases now check that their sync ran. Checks (VPS, Node 24.21.0, transpiler cache off): typecheck and build clean; stop-with-host-operations 53/53 unforced. With these tests and the previous commit's src, four fail, each at its own assertion: the stale broadcast, the one-turn undo, the admitted sync after a failed stop(), and the undo during a switch. The timed-out sync and the surgeries' sync count pass there, since they pin behaviour that commit already had. Co-Authored-By: Claude Opus 5.5 --- ...top-waits-for-operator-operations.fixed.md | 6 +- changelog.d/switch-branch.added.md | 4 +- src/api/server.ts | 11 ++-- src/framework.ts | 56 ++++++++++++------ test/stop-with-host-operations.test.ts | 59 +++++++++++++++++++ 5 files changed, 112 insertions(+), 24 deletions(-) diff --git a/changelog.d/stop-waits-for-operator-operations.fixed.md b/changelog.d/stop-waits-for-operator-operations.fixed.md index 1cf41f107..9e2e37e2d 100644 --- a/changelog.d/stop-waits-for-operator-operations.fixed.md +++ b/changelog.d/stop-waits-for-operator-operations.fixed.md @@ -48,8 +48,10 @@ says that the parked wakes it released won't run. - A host's trace listener can call `stop()` from inside an operation, such as on its completion trace, and `stop()` still waits for that operation. - A multi-turn `host/command` undo stopped that way refuses at its next - turn and answers with the turns it undid. + A multi-turn `host/command` undo stopped that way still undoes every + turn it was asked for. + - `undoLastTurn()` and `redo()` refuse while a surgery or a branch switch + holds the store, instead of moving the branch beneath it. - While the host stops, an MCPL awareness barrier that fails no longer recycles the connections, which `stop()` is closing. The framework releases it as it releases a completed one, so nothing waits on a diff --git a/changelog.d/switch-branch.added.md b/changelog.d/switch-branch.added.md index 22c7abcab..01ee63939 100644 --- a/changelog.d/switch-branch.added.md +++ b/changelog.d/switch-branch.added.md @@ -10,4 +10,6 @@ store, and now calls `switchBranch()`. So it refuses during a live turn instead of moving the branch beneath it. Its current branch follows the store from the moment it moves, and clients hear of a move even when a - later step fails. + later step fails, but not of a refused switch. A branch create with + `switchTo` creates the branch first, so during a live turn it creates + it and answers the switch's refusal. diff --git a/src/api/server.ts b/src/api/server.ts index ed60e058a..bf3378aa7 100644 --- a/src/api/server.ts +++ b/src/api/server.ts @@ -536,7 +536,8 @@ export class ApiServer { throw new Error('name is required'); } - const previousBranch = this.currentBranch; + const store = this.framework.getStore(); + const from = store.isClosed() ? this.currentBranch : store.currentBranch().name; // The framework switches the store, reconciles the Discord awareness // markers and re-materializes the config mount. It refuses during a live @@ -545,17 +546,19 @@ export class ApiServer { // (a refusal moves nothing), so the current branch follows it through the // awaited steps. const switching = this.framework.switchBranch(params.name); + void switching.catch(() => {}); // awaited below; handled even if the next line throws this.followStoreBranch(); + const moved = this.currentBranch !== from; let switched = false; try { await switching; switched = true; } finally { // A later step can fail after the store has moved: clients still hear - // where it is. - if (switched || this.currentBranch !== previousBranch) { + // where it is. A refusal moved nothing, so there's nothing to tell. + if (switched || moved) { this.broadcast('branch:switched', { - from: previousBranch, + from, to: this.currentBranch, }); } diff --git a/src/framework.ts b/src/framework.ts index 4819a505b..5e071c946 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -1143,11 +1143,13 @@ export class AgentFramework { * start. stop() waits until this set is empty before it tears down, unless * its wait budget runs out. */ private readonly operatorOperations = new Set<{ label: string; promise: Promise }>(); - /** True from the start of each stop() attempt until it has finished waiting - * on operatorOperations, whether the wait completed or timed out: the window - * in which an awareness sync is admitted although the queue is closed (see - * syncDiscordAwarenessMarkers). */ - private stopWaitPending = false; + /** stop() attempts whose wait on operatorOperations hasn't ended: one is + * added as an attempt starts, and removed when its wait completes or times + * out, or when the attempt fails before reaching it. While any is here, an + * awareness sync is admitted although the queue is closed (see + * syncDiscordAwarenessMarkers). A set, because a trace listener can start a + * second attempt inside the first's synchronous start. */ + private readonly openStopWaits = new Set(); private membrane: Membrane; private queue: ProcessQueueImpl; private agents: Map = new Map(); @@ -2154,12 +2156,16 @@ export class AgentFramework { try { validateShutdownTimeout(teardownTimeoutMs); } catch (error) { return Promise.reject(error); } if (this.stopPromise) return this.stopPromise; - const attempt = this.stopOwned(teardownTimeoutMs); + const stopWait = {}; + this.openStopWaits.add(stopWait); + const attempt = this.stopOwned(teardownTimeoutMs, stopWait); this.stopPromise = attempt; void attempt.then(() => { this.stopCompleted = true; }, () => {}); // Failed cleanup can be retried while its original store is open. A // terminal storage failure after successful close must remain a failure. void attempt.catch(() => { + // An attempt that failed before its wait leaves no wait open. + this.openStopWaits.delete(stopWait); if (!this.ownedStoreClosed && this.stopPromise === attempt) this.stopPromise = null; }); return attempt; @@ -2193,9 +2199,9 @@ export class AgentFramework { /** Wait in rounds until the set is empty: admitted work can still start work * of its own, and an awareness sync can still join. All rounds share the - * caller's deadline rather than renewing its budget. Ends the window in - * which a sync is admitted (stopWaitPending). */ - private async waitForOperatorOperations(timeoutMs: number | undefined): Promise { + * caller's deadline rather than renewing its budget. Ends this attempt's + * window for an awareness sync (openStopWaits). */ + private async waitForOperatorOperations(timeoutMs: number | undefined, stopWait: object): Promise { const deadline = timeoutMs === undefined ? undefined : performance.now() + timeoutMs; try { while (this.operatorOperations.size > 0) { @@ -2216,14 +2222,11 @@ export class AgentFramework { } finally { // In the same step as the last look at the set, so no awareness sync // can join it unseen. - this.stopWaitPending = false; + this.openStopWaits.delete(stopWait); } } - private async stopOwned(teardownTimeoutMs: number | undefined): Promise { - // An awareness sync is admitted until this attempt has finished waiting on - // the operator set (waitForOperatorOperations ends the window). - this.stopWaitPending = true; + private async stopOwned(teardownTimeoutMs: number | undefined, stopWait: object): Promise { this.pushCoalescer?.suspend(); this.flushCoalescingSnapshot(); this.running = false; @@ -2304,7 +2307,7 @@ export class AgentFramework { // down, and the store stays open for another stop(). let operatorOperationsTimedOut: unknown; try { - await this.waitForOperatorOperations(teardownTimeoutMs); + await this.waitForOperatorOperations(teardownTimeoutMs, stopWait); } catch (error) { operatorOperationsTimedOut = error; } @@ -5790,7 +5793,7 @@ export class AgentFramework { let undone = 0; try { for (let i = 0; i < requested; i++) { - const r = this.undoLastTurn(agentName, hostCommandRequester(serverId, params)); + const r = this.runUndoLastTurn(agentName, hostCommandRequester(serverId, params)); if (!r.undone) break; undone++; } @@ -6166,6 +6169,17 @@ export class AgentFramework { toBranch?: string; } { this.refuseOperatorAfterStop('undo'); + return this.runUndoLastTurn(agentName, requester); + } + + /** undoLastTurn's work, for a host/command undo already admitted: a stop() + * begun by a trace of one of its turns doesn't refuse the next. */ + private runUndoLastTurn(agentName: string, requester?: OperatorRequester): { + undone: boolean; + turnIndex?: number; + fromBranch?: string; + toBranch?: string; + } { const agent = this.agents.get(agentName); if (!agent) { throw new Error(`Unknown agent: ${agentName}`); @@ -6173,6 +6187,10 @@ export class AgentFramework { if (agent.state.status !== 'idle') { throw new Error(`Cannot undo while agent is ${agent.state.status}`); } + // A surgery or a branch switch holds the store while it awaits its steps. + if (this.surgeryHold) { + throw new Error(`Cannot undo: the store is under live ${this.surgeryHold.verb} for ${this.surgeryHold.agentName}`); + } const checkpoints = this.getTurnCheckpoints(agentName); if (checkpoints.length === 0) { @@ -6240,6 +6258,10 @@ export class AgentFramework { if (agent.state.status !== 'idle') { throw new Error(`Cannot redo while agent is ${agent.state.status}`); } + // A surgery or a branch switch holds the store while it awaits its steps. + if (this.surgeryHold) { + throw new Error(`Cannot redo: the store is under live ${this.surgeryHold.verb} for ${this.surgeryHold.agentName}`); + } const redoStack = this.redoStacks.get(agentName); if (!redoStack || redoStack.length === 0) { @@ -14075,7 +14097,7 @@ export class AgentFramework { * stop() whose budget ran out doesn't refuse it partway, after its branch * has moved. */ async syncDiscordAwarenessMarkers(onlyServerId?: string): Promise { - if (this.queue?.isClosed && !this.stopWaitPending) throw new Error('awareness sync refused: the framework is stopping'); + if (this.queue?.isClosed && this.openStopWaits.size === 0) throw new Error('awareness sync refused: the framework is stopping'); return this.trackOperatorOperation('awareness:sync', () => this.runDiscordAwarenessSync(onlyServerId)); } diff --git a/test/stop-with-host-operations.test.ts b/test/stop-with-host-operations.test.ts index 54199bdd9..e394283ba 100644 --- a/test/stop-with-host-operations.test.ts +++ b/test/stop-with-host-operations.test.ts @@ -416,6 +416,19 @@ describe('stop() with an operator operation in flight', () => { assert.deepEqual(rejections, []); }); + it('an awareness sync after a stop() that failed before its wait is refused', async () => { + const framework = await create(); + const fw = framework as unknown as Internals & { codeExecutionRunners: Map }; + // A runner whose dispose throws fails stop() after the queue has closed, + // before stop() reaches its wait on the operator set. + fw.codeExecutionRunners.set('agent', { dispose() { throw new Error('dispose failed'); } }); + await assert.rejects(framework.stop(), /dispose failed/); + await assert.rejects(framework.syncDiscordAwarenessMarkers(), /^Error: awareness sync refused: the framework is stopping$/); + fw.codeExecutionRunners.clear(); + await framework.stop(); + assert.equal(framework.getStore().isClosed(), true); + }); + it('an awareness sync called once stop() has finished waiting is refused, and installs no barrier', async () => { const framework = await create(); let barriers = 0; @@ -455,6 +468,11 @@ describe('stop() with an operator operation in flight', () => { const fork = cm.fork.bind(cm); cm.fork = async (...args: Parameters) => { await moveHold; return fork(...args); }; } + let barriers = 0; + internals(framework).beginDiscordAwarenessBarrier = () => { + barriers++; + return { requiresBarrier: false, promise: Promise.resolve({ status: 'delivered', delivered: 0, failed: 0 }) }; + }; const surgery = action === 'rollback' ? framework.rollbackToMessage('agent', { messageId: ids[0]! }) : framework.suppressMessages('agent', { messageIds: [ids[1]!] }); @@ -465,6 +483,7 @@ describe('stop() with an operator operation in flight', () => { } finally { finishMove(); } const result = await surgery; assert.ok(result.messagesRemoved > 0, JSON.stringify(result)); + assert.equal(barriers, 1, 'its awareness sync ran'); await framework.stop(); assert.equal(framework.getStore().isClosed(), true); assert.deepEqual(rejections, []); @@ -489,6 +508,8 @@ describe('stop() with an operator operation in flight', () => { ); assert.equal(framework.getStore().isClosed(), false, 'the operation may still write'); assert.ok(order.includes('modules stopped'), 'timeout still proceeds with teardown'); + await assert.rejects(framework.syncDiscordAwarenessMarkers(), /^Error: awareness sync refused: the framework is stopping$/, + 'no stop() is waiting now, so a new sync is refused'); await assert.rejects(framework.stop(100), (error: unknown) => error instanceof ShutdownTimeoutError && error.pending.includes('awareness:sync')); assert.equal(framework.getStore().isClosed(), false, 'a still-running retry keeps the store open too'); @@ -728,6 +749,9 @@ describe('stop() with an operator operation in flight', () => { await until(() => mounting, 'the switch to reach its config mount'); assert.equal((await api.cmdBranchCurrent()).name, 'side'); await assert.rejects(api.cmdBranchDelete({ name: 'side' }), /Cannot delete current branch/); + // The switch holds the store through its mount. + assert.throws(() => framework.undoLastTurn('agent'), /^Error: Cannot undo: the store is under live switch branch for every agent$/); + assert.throws(() => framework.redo('agent'), /^Error: Cannot redo: the store is under live switch branch for every agent$/); } finally { release(); } await switching; } finally { await framework.stop(); } @@ -756,6 +780,10 @@ describe('stop() with an operator operation in flight', () => { cmdBranchCurrent(): Promise<{ name: string }>; }; await api.cmdBranchCreate({ name: 'side' }); + const broadcasts: unknown[] = []; + (api as unknown as { broadcast(event: string, data: unknown): void }).broadcast = (event, data) => { broadcasts.push([event, data]); }; + // A stale cache, as after a rollback made outside the API server. + (api as unknown as { currentBranch: string }).currentBranch = 'stale'; // A turn holds its token from dequeue, before its stream registers. const tokens = internals(framework).activeTurnTokens; tokens.set('agent', -1); @@ -764,6 +792,7 @@ describe('stop() with an operator operation in flight', () => { await assert.rejects(api.cmdBranchSwitch({ name: 'side' }), /Cannot switch branch while agent is idle\+turn-alive/); assert.equal(framework.getStore().currentBranch().name, 'main'); assert.equal((await api.cmdBranchCurrent()).name, 'main'); + assert.deepEqual(broadcasts, [], 'a refused switch moved nothing, so it announces nothing'); } finally { tokens.delete('agent'); } await framework.stop(); }); @@ -955,6 +984,36 @@ describe('stop() with an operator operation in flight', () => { assert.deepEqual(rejections, []); }); + it('a multi-turn host/command undo whose first turn starts stop() still undoes every turn it was asked for', async () => { + const framework = await create(); + const fw = internals(framework); + const cm = framework.getAgent('agent')!.getContextManager(); + // Two turns, each checkpointed as a real turn's start records it. + const record = (fw as unknown as { recordTurnCheckpoint(name: string): void }).recordTurnCheckpoint.bind(fw); + record('agent'); + cm.addMessage('user', [{ type: 'text', text: 'turn one' }]); + record('agent'); + cm.addMessage('user', [{ type: 'text', text: 'turn two' }]); + let stopping: Promise | undefined; + framework.onTrace((event) => { + if (event.type === 'undo:completed' && !stopping) stopping = framework.stop(); + }); + const connection = Object.assign(new EventEmitter(), { id: 'srv' }); + fw.wireMcplEvents(connection); + let answer: { ok?: boolean; undone?: number; requested?: number } | undefined; + connection.emit('host-command', { command: 'undo', agentName: 'agent', turns: 2 }, { + id: 10, + respond: (result: typeof answer) => { answer = result; }, + respondError: () => { order.push('undo failed'); }, + }); + await until(() => answer !== undefined || order.includes('undo failed'), 'the undo to answer'); + assert.equal(answer?.ok, true, JSON.stringify(answer)); + assert.equal(answer?.undone, 2, JSON.stringify(answer)); + assert.ok(stopping, 'the first turn started stop()'); + await stopping; + assert.deepEqual(rejections, []); + }); + for (const elapsed of [6_000, 9_999, 11_000]) it("work an operation starts shares stop()'s deadline, " + elapsed + ' ms in', async () => { const framework = await create(); const fw = internals(framework); From 08214453448bec3b02dde120875924292c27c4f8 Mon Sep 17 00:00:00 2001 From: Nell-1783 Date: Tue, 13 Oct 2026 12:52:59 -0400 Subject: [PATCH 12/21] fix(framework): a stop() called from inside stop()'s start gets the running attempt stop() now creates its attempt's promise and assigns it to stopPromise before it starts stopOwned. It still starts the body in the same call, so the queue still closes before stop() returns. The body's first step, the push coalescer's suspend(), emits an mcpl:coalescing trace for a render in flight, synchronously. Before, stopPromise was assigned only after stopOwned returned at its first await, so a trace listener that called stop() there found no attempt and started a second teardown. The inner one closed the store, the outer one's final sync then failed, and stopPromise stayed on the failed outer attempt, kept there because the owned store was closed. Every later stop() then rejected with "Store has been closed" for a framework that had stopped. main does the same (found with a probe by a reader of the previous commit). It's the rule this change applies to operator operations, register before the body runs, now applied to stop() itself. openStopWaits stays a set: one attempt runs at a time now, but each keeps its own entry so that cleaning up after one can't close another's window, and its comment says so. Test: a coalescer whose suspend() emits a trace, and a listener that calls stop() on it. The two calls get the same promise, one teardown runs, and a later stop() resolves. Checks (VPS, Node 24.21.0, transpiler cache off): typecheck and build clean; unforced, stop-with-host-operations 54/54, stop-with-work-in-flight 25/25, shutdown-errors 28/28, host-quiesce 31/31, conversation-routing 15/15, operator-surgery 18/18. Co-Authored-By: Claude Opus 5.5 --- ...top-waits-for-operator-operations.fixed.md | 6 ++++ src/framework.ts | 14 +++++++-- test/stop-with-host-operations.test.ts | 29 +++++++++++++++++++ 3 files changed, 46 insertions(+), 3 deletions(-) diff --git a/changelog.d/stop-waits-for-operator-operations.fixed.md b/changelog.d/stop-waits-for-operator-operations.fixed.md index 9e2e37e2d..41db28021 100644 --- a/changelog.d/stop-waits-for-operator-operations.fixed.md +++ b/changelog.d/stop-waits-for-operator-operations.fixed.md @@ -52,6 +52,12 @@ turn it was asked for. - `undoLastTurn()` and `redo()` refuse while a surgery or a branch switch holds the store, instead of moving the branch beneath it. + - A `stop()` called from a trace emitted inside `stop()`'s own start, as + the push coalescer's suspension emits for a render in flight, gets the + attempt already running. Before, it started a second teardown beside + the first, which then failed at its final sync, and every later + `stop()` rejected with "Store has been closed" for a framework that had + stopped. - While the host stops, an MCPL awareness barrier that fails no longer recycles the connections, which `stop()` is closing. The framework releases it as it releases a completed one, so nothing waits on a diff --git a/src/framework.ts b/src/framework.ts index 5e071c946..49e41608f 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -1147,8 +1147,8 @@ export class AgentFramework { * added as an attempt starts, and removed when its wait completes or times * out, or when the attempt fails before reaching it. While any is here, an * awareness sync is admitted although the queue is closed (see - * syncDiscordAwarenessMarkers). A set, because a trace listener can start a - * second attempt inside the first's synchronous start. */ + * syncDiscordAwarenessMarkers). One attempt runs at a time; each has its own + * entry so that cleaning up after one can't close another's window. */ private readonly openStopWaits = new Set(); private membrane: Membrane; private queue: ProcessQueueImpl; @@ -2158,7 +2158,12 @@ export class AgentFramework { if (this.stopPromise) return this.stopPromise; const stopWait = {}; this.openStopWaits.add(stopWait); - const attempt = this.stopOwned(teardownTimeoutMs, stopWait); + // Claim the attempt before any of its body runs: stopOwned's synchronous + // start can emit traces (the push coalescer's suspend() does), and a + // listener that calls stop() there must get this attempt, not start a + // second teardown beside it. + let settle!: { resolve: () => void; reject: (error: unknown) => void }; + const attempt = new Promise((resolve, reject) => { settle = { resolve, reject }; }); this.stopPromise = attempt; void attempt.then(() => { this.stopCompleted = true; }, () => {}); // Failed cleanup can be retried while its original store is open. A @@ -2168,6 +2173,9 @@ export class AgentFramework { this.openStopWaits.delete(stopWait); if (!this.ownedStoreClosed && this.stopPromise === attempt) this.stopPromise = null; }); + // The body still starts in this call, so its synchronous steps (closing + // the queue among them) happen before stop() returns. + this.stopOwned(teardownTimeoutMs, stopWait).then(settle.resolve, settle.reject); return attempt; } diff --git a/test/stop-with-host-operations.test.ts b/test/stop-with-host-operations.test.ts index e394283ba..a37a4620f 100644 --- a/test/stop-with-host-operations.test.ts +++ b/test/stop-with-host-operations.test.ts @@ -416,6 +416,35 @@ describe('stop() with an operator operation in flight', () => { assert.deepEqual(rejections, []); }); + it("a stop() called from a trace inside stop()'s own start gets the same attempt, and one teardown runs", async () => { + const framework = await create(); + const fw = framework as unknown as { + pushCoalescer: { suspend(): void } | null; + emitTrace(event: unknown): void; + }; + // A coalescer with a render in flight reports it from suspend(), the first + // thing stop() does, as a trace. + let suspended = false; + fw.pushCoalescer = { + suspend: () => { + if (suspended) return; + suspended = true; + fw.emitTrace({ type: 'mcpl:coalescing', phase: 'suspended' }); + }, + }; + let inner: Promise | undefined; + framework.onTrace((event) => { + if ((event as { type?: string }).type === 'mcpl:coalescing' && !inner) inner = framework.stop(); + }); + const outer = framework.stop(); + assert.equal(inner, outer, 'the reentrant stop() gets the attempt already running'); + await outer; + assert.deepEqual(order, ['modules stopped'], 'one teardown'); + assert.equal(framework.getStore().isClosed(), true); + await framework.stop(); + assert.deepEqual(rejections, []); + }); + it('an awareness sync after a stop() that failed before its wait is refused', async () => { const framework = await create(); const fw = framework as unknown as Internals & { codeExecutionRunners: Map }; From 1feebb970c455b1d70353d196411903d13f9c4ba Mon Sep 17 00:00:00 2001 From: Nell-1783 Date: Tue, 13 Oct 2026 20:45:41 -0400 Subject: [PATCH 13/21] test(framework): bound the budget test's late-sync refusal check The check that a public awareness sync is refused once a timed-out stop() has stopped waiting awaited the sync's rejection directly. With the refusal removed, the sync was admitted and waited on a barrier of its own that nothing released, so the whole file hung rather than failing. It now races the sync's outcome against a one-second timer (unref'd) and asserts the refusal's text, so a missing refusal fails in about a second. Checks (VPS, Node 24.21.0, transpiler cache off): stop-with-host-operations 54/54 unforced. With the refusal removed from syncDiscordAwarenessMarkers, the file exits in 7 s with 3 failures (this test among them) instead of hanging. Co-Authored-By: Claude Opus 5.5 --- test/stop-with-host-operations.test.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/stop-with-host-operations.test.ts b/test/stop-with-host-operations.test.ts index a37a4620f..591dd7080 100644 --- a/test/stop-with-host-operations.test.ts +++ b/test/stop-with-host-operations.test.ts @@ -537,7 +537,11 @@ describe('stop() with an operator operation in flight', () => { ); assert.equal(framework.getStore().isClosed(), false, 'the operation may still write'); assert.ok(order.includes('modules stopped'), 'timeout still proceeds with teardown'); - await assert.rejects(framework.syncDiscordAwarenessMarkers(), /^Error: awareness sync refused: the framework is stopping$/, + // Bounded: a sync admitted here would wait on a barrier of its own, so it + // fails the test rather than hanging it. + const late = framework.syncDiscordAwarenessMarkers().then(() => 'admitted', (error: unknown) => String(error)); + const stillRunning = new Promise((resolve) => { setTimeout(() => resolve('still running'), 1_000).unref(); }); + assert.equal(await Promise.race([late, stillRunning]), 'Error: awareness sync refused: the framework is stopping', 'no stop() is waiting now, so a new sync is refused'); await assert.rejects(framework.stop(100), (error: unknown) => error instanceof ShutdownTimeoutError && error.pending.includes('awareness:sync')); From 64c39f7a789179d211bbd6add39b9ce33e1c316b Mon Sep 17 00:00:00 2001 From: Nell-1783 Date: Tue, 13 Oct 2026 21:45:45 -0400 Subject: [PATCH 14/21] fix(framework): refuse operator calls from inside stop()'s start; keep the sync window open past stop()'s own call From the review of the previous round. - refuseOperatorAfterStop() refuses once a stop() attempt is in flight (stopPromise, now set before the attempt's body runs) as well as once the queue is closed. Before, a call made from a trace emitted inside stopOwned's synchronous start (the push coalescer's suspend() emits one for a render in flight) came before the queue closed, so it was admitted after stop() had begun. stop() did wait for it, but the documented rule is that these calls refuse once stop() has begun. - waitForOperatorOperations() yields once before its first look at the operator set. When nothing in stopOwned awaited before the wait (no streams, no maintenance pass, no event loop, as in a framework that was never started), the wait's finally closed the awareness-sync window inside stop()'s own call. So the API server's undo, whose trace listener started stop(), had its sync refused right after the undo moved the branch. The last look and the window's close still happen in one step. - undoLastTurn() and redo() refuse a store hold with OperatorActionError('agent-busy'), as a surgery or a switch does for the same hold, and a host/command undo in turns mode forwards that code when its first turn is refused. - A comment in the API server's switch says why the branch it reads right after the call stays current: the switch holds the store until it's done. - Text: the fragment says the refusal covers a call from inside stop()'s start, that a trace-started stop() admits the API's sync even before anything awaited, and that the hold refusal is agent-busy. Tests: the API server's undo whose trace starts stop() syncs the markers, and stop() waits for that sync. An undo called from a trace inside stop()'s start is refused. The hold refusals are OperatorActionError('agent-busy'). A successful switch from a stale cache names the store's branch as where it moved from. The multi-turn host/command undo test checks, as its answer goes out, that both turns' checkpoints and messages are gone. Checks (VPS, Node 24.21.0, transpiler cache off): typecheck and build clean; unforced, stop-with-host-operations 56/56, operator-surgery 18/18, host-quiesce 31/31, stop-with-work-in-flight 25/25, shutdown-errors 28/28, discord-awareness-outbox 6/6, puppet-tool-call 10/10. With these tests and the previous head's src (c1b7ea0d), three fail, each at its own assertion: the undo inside stop()'s start isn't refused, the hold refusal is a plain Error, and the API undo's sync is refused. The stale-cache broadcast and the undo's state checks pass there, since they pin behaviour that head already had. Co-Authored-By: Claude Opus 5.5 --- ...top-waits-for-operator-operations.fixed.md | 12 ++- src/api/server.ts | 4 +- src/framework.ts | 16 +++- test/stop-with-host-operations.test.ts | 79 +++++++++++++++++-- 4 files changed, 96 insertions(+), 15 deletions(-) diff --git a/changelog.d/stop-waits-for-operator-operations.fixed.md b/changelog.d/stop-waits-for-operator-operations.fixed.md index 41db28021..4b315fd56 100644 --- a/changelog.d/stop-waits-for-operator-operations.fixed.md +++ b/changelog.d/stop-waits-for-operator-operations.fixed.md @@ -18,12 +18,15 @@ `maintenanceTick()`, `puppetToolCall()`, `connectMcplServer()`, `disconnectMcplServer()` and `restartMcplServer()` refuse with " refused: the framework is stopping" before they read or change - anything, as a `host/command` already did. + anything, as a `host/command` already did. That includes a call made + from a trace emitted inside `stop()`'s own start, before its queue has + closed. - The exception is `syncDiscordAwarenessMarkers()`, which each `stop()` attempt admits until it has finished waiting, because its callers have already changed the store when they call it: the API server's undo and redo call it after `undoLastTurn()` or `redo()`, sometimes from inside a - trace listener that started `stop()`. `stop()` waits for a sync + trace listener that started `stop()`, so such a sync is admitted even + when nothing in `stop()` had awaited yet. `stop()` waits for a sync admitted then, and refuses one that starts once it can no longer wait for it. An operation already admitted runs its own sync without that check, so a surgery whose branch move outlasted a budget still @@ -50,8 +53,9 @@ as on its completion trace, and `stop()` still waits for that operation. A multi-turn `host/command` undo stopped that way still undoes every turn it was asked for. - - `undoLastTurn()` and `redo()` refuse while a surgery or a branch switch - holds the store, instead of moving the branch beneath it. + - `undoLastTurn()` and `redo()` refuse with `OperatorActionError` + (`agent-busy`) while a surgery or a branch switch holds the store, + instead of moving the branch beneath it. - A `stop()` called from a trace emitted inside `stop()`'s own start, as the push coalescer's suspension emits for a render in flight, gets the attempt already running. Before, it started a second teardown beside diff --git a/src/api/server.ts b/src/api/server.ts index bf3378aa7..f749423ed 100644 --- a/src/api/server.ts +++ b/src/api/server.ts @@ -543,8 +543,8 @@ export class ApiServer { // markers and re-materializes the config mount. It refuses during a live // turn or once stop() has begun, and stop() waits for one already running. // The store has moved when the call returns, before its promise settles - // (a refusal moves nothing), so the current branch follows it through the - // awaited steps. + // (a refusal moves nothing), and the switch holds the store until it's + // done, so the branch read here is current through the awaited steps. const switching = this.framework.switchBranch(params.name); void switching.catch(() => {}); // awaited below; handled even if the next line throws this.followStoreBranch(); diff --git a/src/framework.ts b/src/framework.ts index 1825fd3eb..dd94195be 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -2212,8 +2212,11 @@ export class AgentFramework { return operation; } + /** From the moment stop() has begun: an attempt in flight (stopPromise is + * set before its body runs, which emits traces before the queue closes) or + * a closed queue (after a failed or completed stop()). */ private refuseOperatorAfterStop(verb: string): void { - if (this.queue?.isClosed) throw new Error(verb + ' refused: the framework is stopping'); + if (this.stopPromise || this.queue?.isClosed) throw new Error(verb + ' refused: the framework is stopping'); } /** Wait in rounds until the set is empty: admitted work can still start work @@ -2223,6 +2226,11 @@ export class AgentFramework { private async waitForOperatorOperations(timeoutMs: number | undefined, stopWait: object): Promise { const deadline = timeoutMs === undefined ? undefined : performance.now() + timeoutMs; try { + // Yield once, even with nothing to wait for: when nothing before this + // wait awaited, the window would otherwise close inside stop()'s own + // call, and a sync its caller makes next (the API server's, after an + // undo whose trace started stop()) would be refused. + await null; while (this.operatorOperations.size > 0) { const remaining = deadline === undefined ? undefined : Math.ceil(deadline - performance.now()); const timedOut = () => new ShutdownTimeoutError( @@ -5913,7 +5921,7 @@ export class AgentFramework { // e.g. "Cannot undo while agent is streaming" — report what happened, // including any turns already undone before the failure. const msg = error instanceof Error ? error.message : String(error); - if (undone === 0) return { ok: false, error: msg }; + if (undone === 0) return { ok: false, error: msg, ...(error instanceof OperatorActionError ? { code: error.code } : {}) }; console.error(`[host-command] undo partially failed after ${undone}/${requested}: ${msg}`); } @@ -6301,7 +6309,7 @@ export class AgentFramework { } // A surgery or a branch switch holds the store while it awaits its steps. if (this.surgeryHold) { - throw new Error(`Cannot undo: the store is under live ${this.surgeryHold.verb} for ${this.surgeryHold.agentName}`); + throw new OperatorActionError('agent-busy', `Cannot undo: the store is under live ${this.surgeryHold.verb} for ${this.surgeryHold.agentName}`); } const checkpoints = this.getTurnCheckpoints(agentName); @@ -6372,7 +6380,7 @@ export class AgentFramework { } // A surgery or a branch switch holds the store while it awaits its steps. if (this.surgeryHold) { - throw new Error(`Cannot redo: the store is under live ${this.surgeryHold.verb} for ${this.surgeryHold.agentName}`); + throw new OperatorActionError('agent-busy', `Cannot redo: the store is under live ${this.surgeryHold.verb} for ${this.surgeryHold.agentName}`); } const redoStack = this.redoStacks.get(agentName); diff --git a/test/stop-with-host-operations.test.ts b/test/stop-with-host-operations.test.ts index 591dd7080..1d8874711 100644 --- a/test/stop-with-host-operations.test.ts +++ b/test/stop-with-host-operations.test.ts @@ -24,7 +24,7 @@ import { existsSync, mkdtempSync, readFileSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import type { EventResponse, Module, ModuleContext, ToolDefinition, ToolResult } from '../src/index.js'; -import { AgentFramework, ShutdownTimeoutError } from '../src/index.js'; +import { AgentFramework, OperatorActionError, ShutdownTimeoutError } from '../src/index.js'; import { ApiServer } from '../src/api/server.js'; import { MockMembrane } from './helpers/mock-membrane.js'; @@ -58,6 +58,7 @@ interface Internals { operatorOperations: Set<{ label: string; promise: Promise }>; moduleRegistry: { getModule(name: string): unknown }; saveTurnCheckpoints(name: string, checkpoints: unknown[]): void; + getTurnCheckpoints(name: string): unknown[]; discordAwarenessBarrier: unknown; } const internals = (framework: AgentFramework): Internals => framework as unknown as Internals; @@ -433,11 +434,16 @@ describe('stop() with an operator operation in flight', () => { }, }; let inner: Promise | undefined; + let undoInside: unknown; framework.onTrace((event) => { - if ((event as { type?: string }).type === 'mcpl:coalescing' && !inner) inner = framework.stop(); + if ((event as { type?: string }).type !== 'mcpl:coalescing' || inner) return; + inner = framework.stop(); + // stop() has begun, though its queue isn't closed yet. + try { framework.undoLastTurn('agent'); } catch (error) { undoInside = error; } }); const outer = framework.stop(); assert.equal(inner, outer, 'the reentrant stop() gets the attempt already running'); + assert.match(String(undoInside), /^Error: undo refused: the framework is stopping$/); await outer; assert.deepEqual(order, ['modules stopped'], 'one teardown'); assert.equal(framework.getStore().isClosed(), true); @@ -759,6 +765,24 @@ describe('stop() with an operator operation in flight', () => { } finally { await framework.stop(); } }); + it("a successful switch from the API server names the store's branch as where it moved from, even with a stale cache", async () => { + const framework = await create(); + try { + const api = new ApiServer(framework) as unknown as { + cmdBranchCreate(params: { name: string }): Promise; + cmdBranchSwitch(params: { name: string }): Promise; + broadcast(event: string, data: unknown): void; + }; + await api.cmdBranchCreate({ name: 'side' }); + const broadcasts: Array<[string, unknown]> = []; + api.broadcast = (event, data) => { broadcasts.push([event, data]); }; + // As after a rollback made outside the API server. + (api as unknown as { currentBranch: string }).currentBranch = 'stale'; + await api.cmdBranchSwitch({ name: 'side' }); + assert.deepEqual(broadcasts, [['branch:switched', { from: 'main', to: 'side' }]]); + } finally { await framework.stop(); } + }); + it("the API server's current branch is the store's while a switch is still running", { timeout: 20_000 }, async () => { const framework = await create(); try { @@ -783,8 +807,10 @@ describe('stop() with an operator operation in flight', () => { assert.equal((await api.cmdBranchCurrent()).name, 'side'); await assert.rejects(api.cmdBranchDelete({ name: 'side' }), /Cannot delete current branch/); // The switch holds the store through its mount. - assert.throws(() => framework.undoLastTurn('agent'), /^Error: Cannot undo: the store is under live switch branch for every agent$/); - assert.throws(() => framework.redo('agent'), /^Error: Cannot redo: the store is under live switch branch for every agent$/); + assert.throws(() => framework.undoLastTurn('agent'), (error: unknown) => error instanceof OperatorActionError + && error.code === 'agent-busy' && error.message === 'Cannot undo: the store is under live switch branch for every agent'); + assert.throws(() => framework.redo('agent'), (error: unknown) => error instanceof OperatorActionError + && error.code === 'agent-busy' && error.message === 'Cannot redo: the store is under live switch branch for every agent'); } finally { release(); } await switching; } finally { await framework.stop(); } @@ -1017,6 +1043,40 @@ describe('stop() with an operator operation in flight', () => { assert.deepEqual(rejections, []); }); + it("the API server's undo whose trace starts stop() still syncs the markers, and stop() waits for that sync", async () => { + const framework = await create(); + const fw = internals(framework); + const cm = framework.getAgent('agent')!.getContextManager(); + const record = (fw as unknown as { recordTurnCheckpoint(name: string): void }).recordTurnCheckpoint.bind(fw); + record('agent'); + cm.addMessage('user', [{ type: 'text', text: 'a turn' }]); + let deliver: (() => void) | undefined; + fw.beginDiscordAwarenessBarrier = () => ({ + requiresBarrier: true, + promise: new Promise((resolve) => { deliver = () => resolve({ status: 'delivered', delivered: 0, failed: 0 }); }), + }); + let stopping: Promise | undefined; + framework.onTrace((event) => { + if (event.type === 'undo:completed' && !stopping) stopping = framework.stop(); + }); + const api = new ApiServer(framework) as unknown as { + cmdUndo(params: { agentName: string }): Promise<{ undone: boolean }>; + broadcast(event: string, data: unknown): void; + }; + api.broadcast = () => {}; + // Nothing in this framework's stop() awaits before its wait on the + // operator set, so only the wait's own yield keeps the sync admitted. + const undoing = api.cmdUndo({ agentName: 'agent' }); + void undoing.catch(() => {}); + await until(() => deliver !== undefined || order.includes('modules stopped'), 'the sync to reach its barrier'); + assert.ok(deliver, "the undo's sync was admitted"); + await releaseWhenStopWaits(deliver); + assert.equal((await undoing).undone, true); + await stopping; + assert.ok(order.includes('modules stopped')); + assert.deepEqual(rejections, []); + }); + it('a multi-turn host/command undo whose first turn starts stop() still undoes every turn it was asked for', async () => { const framework = await create(); const fw = internals(framework); @@ -1034,14 +1094,23 @@ describe('stop() with an operator operation in flight', () => { const connection = Object.assign(new EventEmitter(), { id: 'srv' }); fw.wireMcplEvents(connection); let answer: { ok?: boolean; undone?: number; requested?: number } | undefined; + // Read as the answer goes out: stop() closes the store once the undo is done. + let checkpoints: unknown[] | undefined; + let texts: string[] = []; connection.emit('host-command', { command: 'undo', agentName: 'agent', turns: 2 }, { id: 10, - respond: (result: typeof answer) => { answer = result; }, + respond: (result: typeof answer) => { + answer = result; + checkpoints = fw.getTurnCheckpoints('agent'); + texts = cm.getAllMessages().map((m) => JSON.stringify(m.content)); + }, respondError: () => { order.push('undo failed'); }, }); await until(() => answer !== undefined || order.includes('undo failed'), 'the undo to answer'); assert.equal(answer?.ok, true, JSON.stringify(answer)); assert.equal(answer?.undone, 2, JSON.stringify(answer)); + assert.deepEqual(checkpoints, [], 'both turns were rolled back'); + assert.ok(!texts.some((t) => t.includes('turn one') || t.includes('turn two')), texts.join(' | ')); assert.ok(stopping, 'the first turn started stop()'); await stopping; assert.deepEqual(rejections, []); From 801d9349935a6eb73117505047c6b149e21a4e2d Mon Sep 17 00:00:00 2001 From: Nell-1783 Date: Tue, 13 Oct 2026 21:47:39 -0400 Subject: [PATCH 15/21] test(framework): a host/command undo in turns mode answers a store hold's refusal with its code While a branch switch holds the store in its config mount, a host/command undo with turns: 1 answers { ok: false, error: "Cannot undo: the store is under live switch branch for every agent", code: "agent-busy" }. This pins the turns-mode code forwarding the previous commit added. Checks (VPS, Node 24.21.0, transpiler cache off): typecheck clean; stop-with-host-operations 56/56 unforced. Co-Authored-By: Claude Opus 5.5 --- test/stop-with-host-operations.test.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/test/stop-with-host-operations.test.ts b/test/stop-with-host-operations.test.ts index 1d8874711..eb98c0981 100644 --- a/test/stop-with-host-operations.test.ts +++ b/test/stop-with-host-operations.test.ts @@ -811,6 +811,21 @@ describe('stop() with an operator operation in flight', () => { && error.code === 'agent-busy' && error.message === 'Cannot undo: the store is under live switch branch for every agent'); assert.throws(() => framework.redo('agent'), (error: unknown) => error instanceof OperatorActionError && error.code === 'agent-busy' && error.message === 'Cannot redo: the store is under live switch branch for every agent'); + // A host/command undo in turns mode answers the refusal with its code. + const connection = Object.assign(new EventEmitter(), { id: 'srv' }); + fw.wireMcplEvents(connection); + let answer: unknown; + connection.emit('host-command', { command: 'undo', agentName: 'agent', turns: 1 }, { + id: 11, + respond: (result: unknown) => { answer = result; }, + respondError: (error: unknown) => { answer = { respondError: String(error) }; }, + }); + await until(() => answer !== undefined, 'the host/command undo to answer'); + assert.deepEqual(answer, { + ok: false, + error: 'Cannot undo: the store is under live switch branch for every agent', + code: 'agent-busy', + }); } finally { release(); } await switching; } finally { await framework.stop(); } From d98c595cbabb14040a1730de90ade77258a48d60 Mon Sep 17 00:00:00 2001 From: Nell-1783 Date: Tue, 13 Oct 2026 21:55:47 -0400 Subject: [PATCH 16/21] test(framework): the API undo test checks that stop() waits for the undo's sync The test named "the API server's undo whose trace starts stop() still syncs the markers, and stop() waits for that sync" checked that the sync was admitted and that the undo resolved, not the order. On main's src, which never refuses a sync and never waits for one, it passed: stop() reached module teardown, and only then did the test release the sync. The barrier stub now records "sync delivered", and the test asserts it comes before "modules stopped". Checks (VPS, Node 24.21.0, transpiler cache off): stop-with-host-operations 56/56 unforced. With these tests and main da01e800's src, 55 of the 56 fail. The one that passes, the multi-turn host/command undo, pins what main already does, since main's undo has no stop() refusal to cut it short. Co-Authored-By: Claude Opus 5.5 --- test/stop-with-host-operations.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/stop-with-host-operations.test.ts b/test/stop-with-host-operations.test.ts index eb98c0981..0e3c6da01 100644 --- a/test/stop-with-host-operations.test.ts +++ b/test/stop-with-host-operations.test.ts @@ -1068,7 +1068,9 @@ describe('stop() with an operator operation in flight', () => { let deliver: (() => void) | undefined; fw.beginDiscordAwarenessBarrier = () => ({ requiresBarrier: true, - promise: new Promise((resolve) => { deliver = () => resolve({ status: 'delivered', delivered: 0, failed: 0 }); }), + promise: new Promise((resolve) => { + deliver = () => { order.push('sync delivered'); resolve({ status: 'delivered', delivered: 0, failed: 0 }); }; + }), }); let stopping: Promise | undefined; framework.onTrace((event) => { @@ -1088,7 +1090,7 @@ describe('stop() with an operator operation in flight', () => { await releaseWhenStopWaits(deliver); assert.equal((await undoing).undone, true); await stopping; - assert.ok(order.includes('modules stopped')); + assert.ok(order.indexOf('sync delivered') >= 0 && order.indexOf('sync delivered') < order.indexOf('modules stopped'), order.join(', ')); assert.deepEqual(rejections, []); }); From 2d7ff94dea36fdf229772a889fbd473273c32c8b Mon Sep 17 00:00:00 2001 From: Nell-1783 Date: Tue, 13 Oct 2026 22:06:13 -0400 Subject: [PATCH 17/21] fix(framework): the host/command listener refuses from the moment stop() has begun The operator entry points refuse once a stop() attempt is in flight (stopPromise) or the queue is closed. The MCPL host/command listener still refused only on a closed queue. So a host/command delivered from a trace emitted inside stop()'s own start, before the queue closes, was admitted. A reader's probe showed an undo answering ok: true, undone: 1 and moving the store to its undo branch there. stopHasBegun() now holds the predicate, and refuseOperatorAfterStop() and the listener both use it. The listener keeps its 'refused' answer (-32000, "the host is stopping"), and a command admitted before stop() began keeps its private continuations. Test: the re-entrant stop() test records a turn, wires a connection, and from the coalescer's trace inside stop()'s start delivers a host/command undo too. It's answered -32000 "the host is stopping", and the branch stays on main. Checks (VPS, Node 24.21.0, transpiler cache off): typecheck clean; unforced, stop-with-host-operations 56/56, host-quiesce 31/31, operator-surgery 18/18. With the listener's old closed-queue check, the re-entrant test fails at the host/command's answer. Co-Authored-By: Claude Opus 5.5 --- .../stop-waits-for-operator-operations.fixed.md | 3 ++- src/framework.ts | 17 +++++++++++------ test/stop-with-host-operations.test.ts | 13 +++++++++++++ 3 files changed, 26 insertions(+), 7 deletions(-) diff --git a/changelog.d/stop-waits-for-operator-operations.fixed.md b/changelog.d/stop-waits-for-operator-operations.fixed.md index 4b315fd56..e274f97bc 100644 --- a/changelog.d/stop-waits-for-operator-operations.fixed.md +++ b/changelog.d/stop-waits-for-operator-operations.fixed.md @@ -20,7 +20,8 @@ refused: the framework is stopping" before they read or change anything, as a `host/command` already did. That includes a call made from a trace emitted inside `stop()`'s own start, before its queue has - closed. + closed, and the MCPL listener now refuses a `host/command` from that + moment too. - The exception is `syncDiscordAwarenessMarkers()`, which each `stop()` attempt admits until it has finished waiting, because its callers have already changed the store when they call it: the API server's undo and diff --git a/src/framework.ts b/src/framework.ts index de53b70c3..8c39c4630 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -2214,11 +2214,16 @@ export class AgentFramework { return operation; } - /** From the moment stop() has begun: an attempt in flight (stopPromise is - * set before its body runs, which emits traces before the queue closes) or - * a closed queue (after a failed or completed stop()). */ + /** Whether stop() has begun: an attempt in flight (stopPromise is set before + * its body runs, which emits traces before the queue closes) or a closed + * queue (after a failed or completed stop()). The admission rule for every + * operator entry point, the host/command listener included. */ + private stopHasBegun(): boolean { + return !!this.stopPromise || !!this.queue?.isClosed; + } + private refuseOperatorAfterStop(verb: string): void { - if (this.stopPromise || this.queue?.isClosed) throw new Error(verb + ' refused: the framework is stopping'); + if (this.stopHasBegun()) throw new Error(verb + ' refused: the framework is stopping'); } /** Wait in rounds until the set is empty: admitted work can still start work @@ -15522,8 +15527,8 @@ export class AgentFramework { const barrier = this.discordAwarenessBarrier; if (barrier) await barrier.promise; // A stopping host runs no host command: refused like the requests - // above, before its handler runs. - if (this.queue?.isClosed) { + // above, before its handler runs, from the moment stop() has begun. + if (this.stopHasBegun()) { this.refuseServerItem('host/command', connection.id, answer, { kind: 'refused' }); return; } diff --git a/test/stop-with-host-operations.test.ts b/test/stop-with-host-operations.test.ts index 0e3c6da01..3bfa5938d 100644 --- a/test/stop-with-host-operations.test.ts +++ b/test/stop-with-host-operations.test.ts @@ -433,17 +433,30 @@ describe('stop() with an operator operation in flight', () => { fw.emitTrace({ type: 'mcpl:coalescing', phase: 'suspended' }); }, }; + // A turn to undo, so an admitted undo would move the branch. + const record = (framework as unknown as { recordTurnCheckpoint(name: string): void }).recordTurnCheckpoint.bind(framework); + record('agent'); + framework.getAgent('agent')!.getContextManager().addMessage('user', [{ type: 'text', text: 'a turn' }]); + const connection = Object.assign(new EventEmitter(), { id: 'srv' }); + internals(framework).wireMcplEvents(connection); let inner: Promise | undefined; let undoInside: unknown; + let hostUndo: unknown; framework.onTrace((event) => { if ((event as { type?: string }).type !== 'mcpl:coalescing' || inner) return; inner = framework.stop(); // stop() has begun, though its queue isn't closed yet. try { framework.undoLastTurn('agent'); } catch (error) { undoInside = error; } + connection.emit('host-command', { command: 'undo', agentName: 'agent', turns: 1 }, { + respond: (result: unknown) => { hostUndo = { respond: result }; }, + respondError: (code: number, message: string) => { hostUndo = { code, message }; }, + }); }); const outer = framework.stop(); assert.equal(inner, outer, 'the reentrant stop() gets the attempt already running'); assert.match(String(undoInside), /^Error: undo refused: the framework is stopping$/); + assert.deepEqual(hostUndo, { code: -32000, message: 'the host is stopping' }); + assert.equal(framework.getStore().currentBranch().name, 'main', 'nothing moved the branch'); await outer; assert.deepEqual(order, ['modules stopped'], 'one teardown'); assert.equal(framework.getStore().isClosed(), true); From 11b201630dae9d91b86a5835972cf3cc4c4b30f4 Mon Sep 17 00:00:00 2001 From: Nell-1783 Date: Tue, 13 Oct 2026 22:06:44 -0400 Subject: [PATCH 18/21] docs(framework): why an operator operation registers before its body, now that the wait yields first Removing the early registration no longer fails a test: the operator wait now yields once before its first look, so a stop() begun by a trace inside the body finds the operation either way. The registration stays so that the set holds the operation for its whole life, and the trace-started stop() doesn't rest on the wait's yield alone. The comment now says both. Co-Authored-By: Claude Opus 5.5 --- src/framework.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/framework.ts b/src/framework.ts index 8c39c4630..1b5122d22 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -2200,9 +2200,12 @@ export class AgentFramework { } } - /** Register before invoking the body: its synchronous trace callbacks can - * reenter stop(). Invocation stays in this stack, preserving immediate - * mode changes; the returned promise carries its result to the caller. */ + /** Register before invoking the body, so the set holds the operation for + * its whole life: its synchronous trace callbacks can reenter stop(), and + * whether that stop() finds it then doesn't rest on when the wait first + * looks (it yields once first, too). Invocation stays in this stack, + * preserving immediate mode changes; the returned promise carries its + * result to the caller. */ private trackOperatorOperation(label: string, run: () => Promise): Promise { let resolve!: (value: T | PromiseLike) => void; let reject!: (error: unknown) => void; From 4efc3b379c0080fbe4030cc24ff9ddef0d830b70 Mon Sep 17 00:00:00 2001 From: Nell-1783 Date: Tue, 13 Oct 2026 22:53:16 -0400 Subject: [PATCH 19/21] test(framework): stop() doesn't wait for a started delivery, and one it cuts off raises no alert The test that stop() doesn't wait for the marks' delivery a branch switch started now stops with a one-second budget, so a stop() that waited for the never-ending delivery fails at once instead of hanging the file. A new test fails a switch's delivery after stop() has begun, and checks that the failure is logged and raises no discord-awareness-delivery ops alert. Checks (VPS, Node 24.21.0, transpiler cache off): typecheck clean; stop-with-host-operations 57/57 unforced. Making the delivery an operator operation (the public sync) fails the first test in a second. Dropping the quiet path in the delivery's catch fails the second. Co-Authored-By: Claude Opus 5.5 --- test/stop-with-host-operations.test.ts | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/test/stop-with-host-operations.test.ts b/test/stop-with-host-operations.test.ts index 4d17a465c..3f84ea380 100644 --- a/test/stop-with-host-operations.test.ts +++ b/test/stop-with-host-operations.test.ts @@ -793,12 +793,34 @@ describe('stop() with an operator operation in flight', () => { }; await framework.switchBranch('side'); assert.equal(delivering, true, 'the switch started the delivery and answered without it'); - await framework.stop(); + // A budget, so a stop() that waited for the delivery would fail rather than hang. + await framework.stop(1_000); assert.ok(order.includes('modules stopped')); assert.equal(framework.getStore().isClosed(), true); assert.deepEqual(rejections, []); }); + it('a delivery that stop() cuts off is logged without an ops alert', async () => { + const framework = await create(); + const api = new ApiServer(framework) as unknown as { cmdBranchCreate(params: { name: string }): Promise }; + await api.cmdBranchCreate({ name: 'side' }); + let fail!: (error: Error) => void; + internals(framework).beginDiscordAwarenessBarrier = () => ({ + requiresBarrier: true, + promise: new Promise((_resolve, reject) => { fail = reject; }), + }); + const alerts: string[] = []; + framework.onTrace((event) => { + if (event.type === 'ops:alert') alerts.push((event as { kind?: string }).kind ?? ''); + }); + await framework.switchBranch('side'); + const stopping = framework.stop(); + fail(new Error('connection closed')); + await stopping; + await until(() => logged.some((line) => line.includes('delivery after branch switch agent=framework failed')), 'the failure to be logged'); + assert.deepEqual(alerts.filter((kind) => kind === 'discord-awareness-delivery'), []); + }); + it('a branch switch during a live turn is refused, and the branch stays', async () => { const framework = await create(); const api = new ApiServer(framework) as unknown as { From aa6c1a5fc442c91210976bcab64decfe7c98fa3f Mon Sep 17 00:00:00 2001 From: Nell-1783 Date: Tue, 13 Oct 2026 23:22:29 -0400 Subject: [PATCH 20/21] fix(framework): a started delivery's failure always alerts; the delivery tests reach stop()'s start From the review of the merge with #246. - startDiscordAwarenessDelivery() raises its ops alert for every failure again, as #246 wrote it. The quiet path the merge added for a drain that stop() cut off keyed on stop() having begun, not on the cause. What reaches that catch are the ledger's and the gate's own failures, which the alert exists for: a send that fails because its connection closed is recorded by the drain, which then resolves. So the quiet path hid real incidents and spared no cut-off. Its test, which injected a rejection no real cut-off produces, is removed. - The delivery's doc comment says that a drain already running when stop() begins runs on while the connections close, with what it can't deliver staying pending, and why none is started once stop() has begun. It also says that a failed generation releases the gate rather than recycling the connections once the host is stopping. - The re-entrant stop() test also calls the public sync and starts a delivery from the trace inside stop()'s start. The sync is refused, the delivery isn't started (its log line), and neither reaches a barrier. Before, only an undo and a host/command checked that window. - The test that stop() doesn't wait for a switch's delivery has a test timeout, so a switch that awaited the never-ending delivery fails rather than hangs. Its stop() budget is five seconds, leaving room for an ordinary teardown on a loaded runner. - Text: the test file's header no longer calls the awareness sync an exception, and the fragment names who starts a delivery (a surgery, a branch switch, a host/command undo, the API server's undo and redo) and drops the quiet-failure sentence. Checks (VPS, Node 24.21.0, transpiler cache off): typecheck clean; unforced, stop-with-host-operations 56/56, surgery-marks-receipt 15/15, discord-awareness-outbox 6/6. Co-Authored-By: Claude Opus 5.5 --- ...top-waits-for-operator-operations.fixed.md | 13 +++-- src/framework.ts | 21 ++++---- test/stop-with-host-operations.test.ts | 50 ++++++++----------- 3 files changed, 38 insertions(+), 46 deletions(-) diff --git a/changelog.d/stop-waits-for-operator-operations.fixed.md b/changelog.d/stop-waits-for-operator-operations.fixed.md index bc37dc229..a6e546767 100644 --- a/changelog.d/stop-waits-for-operator-operations.fixed.md +++ b/changelog.d/stop-waits-for-operator-operations.fixed.md @@ -23,13 +23,12 @@ from a trace emitted inside `stop()`'s own start, before its queue has closed, and the MCPL listener now refuses a `host/command` from that moment too. - - The awareness delivery that a surgery, a branch switch, an undo or a - redo starts (`startDiscordAwarenessDelivery()`) runs like a server's - drain: `stop()` doesn't wait for it, and once `stop()` has begun none is - started, since its drain would race the teardown that closes its - connections. The durable outbox keeps the marks, and the next start - delivers them. A drain that `stop()` cuts off is logged without an ops - alert. + - The awareness delivery that a surgery, a branch switch, a `host/command` + undo, or the API server's undo or redo starts + (`startDiscordAwarenessDelivery()`) runs like a server's drain: `stop()` + doesn't wait for it, and once `stop()` has begun none is started, since + the teardown would cut it off. The durable outbox keeps the marks, and + the next start delivers them. - `stop()` also waits for what those operations start while it waits: the config materialization that undo, redo and the two surgeries start without waiting for it themselves. The framework now logs a failed diff --git a/src/framework.ts b/src/framework.ts index e898ff46c..646e22c66 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -7057,17 +7057,20 @@ export class AgentFramework { * (queued when a surgery began, or arriving during its switch or fork): the * surgery's store reservation, which used to hold such a turn through the * drain, now ends once the gate is up. A failure is accounted by the gate - * itself: the current generation's failure recycles the connections, and a - * superseded one leaves that to the newer generation, whose own pass meets - * the same ledger. Here it is logged and raised as an ops alert, and the - * next sync, reconnect or start delivers what is still pending. + * itself: the current generation's failure recycles the connections (or, + * once the host is stopping, releases the gate instead), and a superseded + * one leaves that to the newer generation, whose own pass meets the same + * ledger. Here it is logged and raised as an ops alert, and the next sync, + * reconnect or start delivers what is still pending. * * Like a server's drain, the delivery runs apart from the operation that * started it: it isn't an operator's operation, so stop() doesn't wait for - * it, and the durable outbox keeps what fails. Once stop() has begun, none - * is started, since its drain would race the teardown that closes its - * connections: the marks stay pending, and the next start's barrier - * reconciles the outbox with the branch and delivers them. + * it. A drain already running when stop() begins runs on while the + * connections close, and what it can't deliver stays pending in the + * durable outbox. Once stop() has begun, none is started, since there's + * no point opening a drain that the teardown is about to cut off: the + * marks stay pending, and the next start's barrier reconciles the outbox + * with the branch and delivers them. * * @param after What moved the branch, for the log and the alert * ("rollback", "branch switch", "undo" …). @@ -7086,8 +7089,6 @@ export class AgentFramework { this.runDiscordAwarenessSync().catch((error) => { const detail = error instanceof Error ? error.message : String(error); console.error(`[discord-awareness] delivery after ${after} agent=${agentName} failed: ${detail}`); - // A drain cut off by stop() isn't an incident: the outbox keeps its marks. - if (this.stopHasBegun()) return; this.opsAlert( 'discord-awareness-delivery', agentName, diff --git a/test/stop-with-host-operations.test.ts b/test/stop-with-host-operations.test.ts index 3f84ea380..3b03b28ce 100644 --- a/test/stop-with-host-operations.test.ts +++ b/test/stop-with-host-operations.test.ts @@ -2,11 +2,11 @@ * stop() and an operator's operations. * * Once stop() has begun, an operator's entry points refuse: quiesce, resume, - * the store surgeries, a branch switch, undo, redo, maintain, puppet and the - * MCPL lifecycle calls (a host/command is refused by its listener). The - * awareness sync is the exception: it's admitted until stop() has finished - * waiting, since its callers have already changed the store. These - * tests cover those refusals and the operations admitted before them, over + * the store surgeries, a branch switch, undo, redo, maintain, puppet, the + * awareness sync and the MCPL lifecycle calls (a host/command is refused by + * its listener). The marks' delivery a branch move starts isn't one of these + * operations: stop() doesn't wait for it, and starts none once it has begun. + * These tests cover those refusals and the operations admitted before them, over * MCPL or from a direct call: stop() waits for each, and for what each starts * while it waits, before the event gate, the modules, the MCPL connections * and the store go. A budget can cut that wait short, leaving the store open. @@ -359,14 +359,23 @@ describe('stop() with an operator operation in flight', () => { framework.getAgent('agent')!.getContextManager().addMessage('user', [{ type: 'text', text: 'a turn' }]); const connection = Object.assign(new EventEmitter(), { id: 'srv' }); internals(framework).wireMcplEvents(connection); + let barriers = 0; + internals(framework).beginDiscordAwarenessBarrier = () => { + barriers++; + return { requiresBarrier: false, promise: Promise.resolve({ status: 'delivered', delivered: 0, failed: 0 }) }; + }; let inner: Promise | undefined; let undoInside: unknown; + let syncInside: Promise | undefined; let hostUndo: unknown; framework.onTrace((event) => { if ((event as { type?: string }).type !== 'mcpl:coalescing' || inner) return; inner = framework.stop(); // stop() has begun, though its queue isn't closed yet. try { framework.undoLastTurn('agent'); } catch (error) { undoInside = error; } + syncInside = framework.syncDiscordAwarenessMarkers(); + void syncInside.catch(() => {}); + framework.startDiscordAwarenessDelivery('probe'); connection.emit('host-command', { command: 'undo', agentName: 'agent', turns: 1 }, { respond: (result: unknown) => { hostUndo = { respond: result }; }, respondError: (code: number, message: string) => { hostUndo = { code, message }; }, @@ -375,6 +384,9 @@ describe('stop() with an operator operation in flight', () => { const outer = framework.stop(); assert.equal(inner, outer, 'the reentrant stop() gets the attempt already running'); assert.match(String(undoInside), /^Error: undo refused: the framework is stopping$/); + await assert.rejects(syncInside!, /^Error: awareness sync refused: the framework is stopping$/); + assert.ok(logged.some((line) => line.includes('delivery after probe agent=framework not started')), logged.join('\n')); + assert.equal(barriers, 0, 'neither the sync nor the delivery reached a barrier'); assert.deepEqual(hostUndo, { code: -32000, message: 'the host is stopping' }); assert.equal(framework.getStore().currentBranch().name, 'main', 'nothing moved the branch'); await outer; @@ -781,7 +793,7 @@ describe('stop() with an operator operation in flight', () => { } finally { await framework.stop(); } }); - it("stop() doesn't wait for the marks' delivery a branch switch started", async () => { + it("stop() doesn't wait for the marks' delivery a branch switch started", { timeout: 20_000 }, async () => { const framework = await create(); const api = new ApiServer(framework) as unknown as { cmdBranchCreate(params: { name: string }): Promise }; await api.cmdBranchCreate({ name: 'side' }); @@ -793,34 +805,14 @@ describe('stop() with an operator operation in flight', () => { }; await framework.switchBranch('side'); assert.equal(delivering, true, 'the switch started the delivery and answered without it'); - // A budget, so a stop() that waited for the delivery would fail rather than hang. - await framework.stop(1_000); + // A budget, so a stop() that waited for the delivery would fail rather than + // hang, with room for an ordinary teardown on a loaded runner. + await framework.stop(5_000); assert.ok(order.includes('modules stopped')); assert.equal(framework.getStore().isClosed(), true); assert.deepEqual(rejections, []); }); - it('a delivery that stop() cuts off is logged without an ops alert', async () => { - const framework = await create(); - const api = new ApiServer(framework) as unknown as { cmdBranchCreate(params: { name: string }): Promise }; - await api.cmdBranchCreate({ name: 'side' }); - let fail!: (error: Error) => void; - internals(framework).beginDiscordAwarenessBarrier = () => ({ - requiresBarrier: true, - promise: new Promise((_resolve, reject) => { fail = reject; }), - }); - const alerts: string[] = []; - framework.onTrace((event) => { - if (event.type === 'ops:alert') alerts.push((event as { kind?: string }).kind ?? ''); - }); - await framework.switchBranch('side'); - const stopping = framework.stop(); - fail(new Error('connection closed')); - await stopping; - await until(() => logged.some((line) => line.includes('delivery after branch switch agent=framework failed')), 'the failure to be logged'); - assert.deepEqual(alerts.filter((kind) => kind === 'discord-awareness-delivery'), []); - }); - it('a branch switch during a live turn is refused, and the branch stays', async () => { const framework = await create(); const api = new ApiServer(framework) as unknown as { From 28a0828737320a3603f53258a8a843adc33b8535 Mon Sep 17 00:00:00 2001 From: Nell-1783 Date: Wed, 14 Oct 2026 00:00:14 -0400 Subject: [PATCH 21/21] fix(api): each branch switch's event names its own destination Greptile found that two branch.switch requests handled together could announce the wrong move. Without a workspace module, switchBranch() holds the store for no await, so a second switch can move the store before the first one's continuation runs. The first switch's branch:switched event then read this.currentBranch after its await and named the second switch's destination: moves main -> a -> b were announced as main -> b and a -> b. cmdBranchSwitch() now keeps the branch it read as the store moved and names it in the event. The comment that said the switch holds the store through its awaited steps, which is true only with a workspace module, now says why the event uses the value read then. A new test sends two switches together and checks that each announces its own move, and that the API server's current branch is the last one. It fails before this change (the first event named b) and passes after. main's own switch also passes it, since there the event names the requested branch. Checks (VPS, Node 24.21.0, transpiler cache off): typecheck clean; stop-with-host-operations 57/57, surgery-marks-receipt 15/15, unforced. Co-Authored-By: Claude Opus 5.5 --- src/api/server.ts | 15 +++++++-------- test/stop-with-host-operations.test.ts | 25 +++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/src/api/server.ts b/src/api/server.ts index 79c53241a..e2ec45bf8 100644 --- a/src/api/server.ts +++ b/src/api/server.ts @@ -544,24 +544,23 @@ export class ApiServer { // config mount. It refuses during a live turn or once stop() has begun, // and stop() waits for one already running. // The store has moved when the call returns, before its promise settles - // (a refusal moves nothing), and the switch holds the store until it's - // done, so the branch read here is current through the awaited steps. + // (a refusal moves nothing). The branch read then is this switch's + // destination: another switch can move the store before this one's + // awaited steps end, so the event names what was read here. const switching = this.framework.switchBranch(params.name); void switching.catch(() => {}); // awaited below; handled even if the next line throws this.followStoreBranch(); - const moved = this.currentBranch !== from; + const to = this.currentBranch; + const moved = to !== from; let switched = false; try { await switching; switched = true; } finally { // A later step can fail after the store has moved: clients still hear - // where it is. A refusal moved nothing, so there's nothing to tell. + // where it went. A refusal moved nothing, so there's nothing to tell. if (switched || moved) { - this.broadcast('branch:switched', { - from, - to: this.currentBranch, - }); + this.broadcast('branch:switched', { from, to }); } } diff --git a/test/stop-with-host-operations.test.ts b/test/stop-with-host-operations.test.ts index 3b03b28ce..cfd9feba5 100644 --- a/test/stop-with-host-operations.test.ts +++ b/test/stop-with-host-operations.test.ts @@ -730,6 +730,31 @@ describe('stop() with an operator operation in flight', () => { } finally { await framework.stop(); } }); + it('two API switches handled together each announce their own move', async () => { + // With no workspace module, a switch holds the store for no await, so a + // second one can move the store before the first's continuation runs. + const framework = await create(); + try { + const api = new ApiServer(framework) as unknown as { + cmdBranchCreate(params: { name: string }): Promise; + cmdBranchSwitch(params: { name: string }): Promise; + cmdBranchCurrent(): Promise<{ name: string }>; + broadcast(event: string, data: unknown): void; + }; + await api.cmdBranchCreate({ name: 'a' }); + await api.cmdBranchCreate({ name: 'b' }); + const broadcasts: Array<[string, unknown]> = []; + api.broadcast = (event, data) => { broadcasts.push([event, data]); }; + await Promise.all([api.cmdBranchSwitch({ name: 'a' }), api.cmdBranchSwitch({ name: 'b' })]); + assert.deepEqual(broadcasts, [ + ['branch:switched', { from: 'main', to: 'a' }], + ['branch:switched', { from: 'a', to: 'b' }], + ]); + assert.equal(framework.getStore().currentBranch().name, 'b'); + assert.equal((await api.cmdBranchCurrent()).name, 'b'); + } finally { await framework.stop(); } + }); + it("the API server's current branch is the store's while a switch is still running", { timeout: 20_000 }, async () => { const framework = await create(); try {