fix(framework): refuse start() and addModule() while a stop() is still running

changelog.d/start-during-stop.fixed.md

@@ -0,0 +1 @@
+- `start()` and `addModule()` made while a `stop()` is still running are now refused: `start()` throws `start() refused: the framework is stopping`, and `addModule()` rejects with `addModule() refused: the framework is stopping` (#315). Before, `start()` began a second lifecycle under the stop. If the stop hadn't yet seen its event loop end, it waited on the new loop instead: with no budget, which is the default, nothing ended that wait, and with one, it failed with `pending: event loop` and left the new loop running. If the stop had got past that wait, it settled after tearing down modules and an owned store under the new loop, and every later `stop()` returned that settled promise, so nothing could end the loop. A module added after the stop had taken its list of modules to stop stayed started and registered past it. `start()` and `addModule()` after a `stop()` has settled behave as before.

docs/tool-result-guard.md

@@ -242,7 +242,7 @@ Requests sent by the framework or the direct Agent inference API are compiled af
 
 Concurrent starts on one Agent, a direct inference call beside a framework-driven turn, are not made safe here: an Agent compiles its request while its status still reads idle (or ready), as before this change, and the wait for annotations lengthens that window, by up to the batch's deadline. `abortInference()` returns false during a direct call's initial assembly; a caller's own abort signal is checked before the request is sent, and again after the refusal retry's annotation wait. The direct API reads its unguarded tool results once, after the first compile, and sends the same ones with the refusal retry.
 
-At stop, the framework lets running annotations finish while the workspace is up. An annotation that starts after that drain writes no file and is saved before the store closes. A framework-driven turn that compiled across stop is discarded before its stream is driven, even if the same framework has since restarted. A `start()` after a finished `stop()` starts a new lifecycle, with the spill gate open again; a `start()` made while a `stop()` is still draining isn't a supported order. These lifecycle gates cover framework-driven turns; direct Agent calls use their caller's abort signal.
+At stop, the framework lets running annotations finish while the workspace is up. An annotation that starts after that drain writes no file and is saved before the store closes. A framework-driven turn that compiled across stop is discarded before its stream is driven, even if the same framework has since restarted. A `start()` after a finished `stop()` starts a new lifecycle, with the spill gate open again; a `start()` made while a `stop()` is still running is refused. These lifecycle gates cover framework-driven turns; direct Agent calls use their caller's abort signal.
 
 For example, an operator can inspect records without changing the agent's
 view:

src/framework.ts

@@ -3085,15 +3085,21 @@ export class AgentFramework {
   }
 
   /**
-   * Start the event loop.
+   * Start the event loop. A start() on a running framework does nothing.
+   * Refused, with an error, from a stop() call until that stop() settles,
+   * since that stop would wait on the lifecycle a start() begins, or tear it
+   * down under it. What a start() after a settled stop() should do is #315's
+   * question.
    */
   start(): void {
+    // Before the running check: stop() claims its attempt before it clears
+    // running, so a start() made in its first steps, such as from a trace
+    // listener, is refused too.
+    this.resetCompletedStop('start()');
     if (this.running) {
       return;
     }
 
-    this.resetCompletedStop();
-
     this.running = true;
     this.providerAdmissionClosed = false;
     // A new lifecycle: a turn compiled before an earlier stop() never drives,
@@ -3125,9 +3131,10 @@ export class AgentFramework {
   }
 
   /**
-   * Stop the event loop. Each step of the teardown runs whatever an earlier
-   * one did: one that throws, such as a host stream's cancel(), fails the
-   * stop() once the rest has run. When a step or a wait up to and including
+   * Stop the event loop. Until it settles, start() and addModule() are
+   * refused. Each step of the teardown runs whatever an earlier one did: one
+   * that throws, such as a host stream's cancel(), fails the stop() once the
+   * rest has run. When a step or a wait up to and including
    * the wait on awareness drains fails or times out, storage stays open and
    * stop() can be called again; one that fails after it, in the storage work
    * that finishes the stop, fails the stop() without keeping storage open.
@@ -3160,7 +3167,8 @@ export class AgentFramework {
    *   land in the run's namespace after them.
    * - an event loop still in a call, such as a module's onProcess, finishes
    *   that call and the rest of its iteration, and then exits. A start()
-   *   made meanwhile runs its own loop beside that last iteration.
+   *   made once that stop() has failed runs its own loop beside that last
+   *   iteration.
    * - a later stop() waits again, within its own budget, for any stream or
    *   loop this one left running, so the store never closes under it.
    */
@@ -3187,8 +3195,15 @@ export class AgentFramework {
     return attempt;
   }
 
-  private resetCompletedStop(): void {
+  /** What start() and addModule() pass before either begins work of a
+   * lifecycle. A stop() still running refuses both, from the moment it
+   * claims its attempt until that attempt settles. A failed stop() has
+   * cleared its attempt, unless it closed an owned store, which is refused
+   * first. A completed stop() is cleared, and the call goes on as before;
+   * whether a lifecycle should begin after a stop() is #315's question. */
+  private resetCompletedStop(verb: 'start()' | 'addModule()'): void {
     if (this.ownedStoreClosed) throw new Error('Cannot start a new lifecycle: owned store is closed');
+    if (this.stopPromise && !this.stopCompleted) throw new Error(`${verb} refused: the framework is stopping`);
     if (this.stopCompleted) {
       this.stopPromise = null;
       this.stopCompleted = false;
@@ -3269,7 +3284,7 @@ export class AgentFramework {
     for (const wake of [...this.retryWaitWakers]) step(wake);
     this.lifecycleGeneration++;
     // The generation this stop ends; withheld spills close for it alone, so a
-    // start() made before this stop finishes reopens them for its own.
+    // start() after this stop has settled reopens them for its own.
     const stoppingGeneration = this.lifecycleGeneration;
     step(() => this.queue.close());
     step(() => this.tuneOutCoordinator?.stop());
@@ -3413,10 +3428,8 @@ export class AgentFramework {
     // hold, and a run whose stream is still running would otherwise wait for
     // its idle watchdog. Its end stores its queue, as at any run's end; that
     // stream's late events are dropped once the run has left, and a later
-    // stop() waits for the stream itself (leftRunning). (A start() during a
-    // stop() is #315's. One made before the loop ended keeps a loop running,
-    // so this stop waits for it, up to its budget; one made just after,
-    // before this point, has its runs ended here too.)
+    // stop() waits for the stream itself (leftRunning). (A start() is refused
+    // until this stop() settles, so no new lifecycle begins beside it.)
     for (const [name, run] of this.ephemeralRuns) run.settle.reject(ephemeralRunStopped(name));
 
     // Admission closed when stop() began. What admitted operations start now,
@@ -3510,10 +3523,8 @@ export class AgentFramework {
     // No stream is left, unless its wait above ran out or its cancel threw:
     // from the start of stop() none registers and no agent turn starts
     // (providerAdmissionClosed, checked for every agent), and every stream
-    // registered before was cancelled and awaited above.
-    // (A start() made before this stop finishes reopens admission, so a turn
-    // it begins can be alive here and take one of these entries mid-stream;
-    // #315 asks how start() after stop() should behave.)
+    // registered before was cancelled and awaited above. (A start(), which
+    // would reopen admission, is refused until this stop() settles.)
     // What is still deferred (a message held for a turn's end whose flush
     // found the target busy again, or one deferred behind its target's own
     // state) would wait for a flush that may never come. Land each at its
@@ -4593,10 +4604,13 @@ export class AgentFramework {
   }
 
   /**
-   * Add a module at runtime.
+   * Add a module at runtime. Refused, with a rejection, while a stop() is
+   * still running: a module added then would either be stopped by that stop
+   * or, once the stop has taken its list of modules to stop, stay started
+   * past it.
    */
   async addModule(module: Module): Promise<void> {
-    this.resetCompletedStop();
+    this.resetCompletedStop('addModule()');
     await this.moduleRegistry.addModule(module);
     this.emitTrace({ type: 'module:added', moduleName: module.name });
   }
@@ -5145,7 +5159,7 @@ export class AgentFramework {
    *  is open again for an instance kept with a store it doesn't own and that
    *  is still open (start() doesn't restore stopped modules or MCPL
    *  connections, and refuses an owned store that stop() closed). A start()
-   *  made while a stop() is still draining isn't a supported order. */
+   *  made while a stop() is still running is refused. */
   private guardSpillsClosedFor: number | null = null;
 
   /** A new agent's guard: its persisted setting, the tool-results spill its

src/gate/event-gate.ts

@@ -2003,10 +2003,9 @@ export class EventGate {
     // a turn whose exit didn't end it (a retry stop() cancelled, a successor
     // stop() refused) would make every debounced batch buffer until that
     // agent's next turn ended. A turn that continues after the start() takes
-    // its entry again (onInferenceResumed). A turn that a start() during the
-    // stop() began before this ran loses its entry too, and runs on without
-    // it, so debounced batches are delivered while it runs: nothing refuses
-    // a start() during a stop() yet.
+    // its entry again (onInferenceResumed). A start() made during the stop()
+    // is refused, so no turn of a new lifecycle can have begun before this
+    // runs.
     this.inferring.clear();
   }
 }

test/start-during-stop.test.ts

@@ -0,0 +1,165 @@
+/**
+ * start() and addModule() made while a stop() is still running are refused
+ * (#315's in-flight half). Before, a start() then left the stop waiting on
+ * the new lifecycle's loop, for good with no budget, or let the stop tear
+ * down modules and an owned store under a loop that no later stop() could
+ * end; a module added after the stop took its list of modules stayed started
+ * past it. Once the stop has settled, this refusal no longer fires: what a
+ * start() after a settled stop() should do is #315's question, and the last
+ * test leaves it open. Each test waits on the event it needs (a module's
+ * stop() being called), not on a fixed delay.
+ */
+
+import { test } from 'node:test';
+import assert from 'node:assert/strict';
+import { mkdtempSync, rmSync } from 'node:fs';
+import { tmpdir } from 'node:os';
+import { join } from 'node:path';
+import { JsStore } from '@animalabs/chronicle';
+import { AgentFramework, type Module } from '../src/index.js';
+import { MockMembrane } from './helpers/mock-membrane.js';
+
+const STOPPING = /^(start|addModule)\(\) refused: the framework is stopping$/;
+
+type Internals = { running: boolean; loopPromise: Promise<void> | null; moduleRegistry: { modules: Map<string, Module> } };
+
+function module(name: string, hooks: { start?: Module['start']; stop?: Module['stop'] } = {}): Module {
+  return {
+    name,
+    start: hooks.start ?? (async () => {}),
+    stop: hooks.stop ?? (async () => {}),
+    async onProcess() { return {}; },
+    getTools: () => [],
+    async handleToolCall() { throw new Error('No tool calls expected'); },
+  };
+}
+
+function deferred(): { promise: Promise<void>; resolve: () => void } {
+  let resolve!: () => void;
+  const promise = new Promise<void>((done) => { resolve = done; });
+  return { promise, resolve };
+}
+
+/** A module whose stop() says when it has been called and waits for the test. */
+function slowToStop() {
+  const called = deferred();
+  const release = deferred();
+  return { called, release, module: module('slow', { stop: async () => { called.resolve(); await release.promise; } }) };
+}
+
+async function fixture(t: import('node:test').TestContext, modules: Module[] = [], borrowStore = false) {
+  const dir = mkdtempSync(join(tmpdir(), 'start-during-stop-'));
+  const store = borrowStore ? JsStore.openOrCreate({ path: join(dir, 'store') }) : undefined;
+  const framework = await AgentFramework.create({
+    ...(store ? { store } : { storePath: join(dir, 'store') }),
+    membrane: new MockMembrane().asMembrane(),
+    agents: [], modules, syncIntervalMs: 0, maintenanceIntervalMs: 0,
+  });
+  const internals = framework as unknown as Internals;
+  const own = framework.getStore();
+  t.after(async () => {
+    // Where a refusal is missing, a second loop can still be running, and a
+    // stop waiting on it: end both before the store goes.
+    internals.running = false;
+    await framework.stop().catch(() => {});
+    await internals.loopPromise?.catch(() => {});
+    try { if (!own.isClosed()) own.close(); } finally { rmSync(dir, { recursive: true, force: true }); }
+  });
+  return { framework, internals };
+}
+
+test('a start() made right after stop() is refused, and that stop settles with its loop ended', { timeout: 30_000 }, async (t) => {
+  const { framework, internals } = await fixture(t);
+  framework.start();
+  const loop = internals.loopPromise!;
+  let loopEnded = false;
+  void loop.then(() => { loopEnded = true; });
+  const stopping = framework.stop();
+  assert.throws(() => framework.start(), { message: 'start() refused: the framework is stopping' });
+  await stopping;
+  assert.equal(loopEnded, true, 'the loop had ended when the stop settled');
+  assert.equal(internals.running, false);
+  assert.equal(internals.loopPromise, loop, 'no second loop was started');
+});
+
+test("a start() made in stop()'s first synchronous steps is refused too", { timeout: 30_000 }, async (t) => {
+  // stop() claims its attempt, then runs steps that can emit traces, the push
+  // coalescer's suspend() first, before it clears running. A stand-in for the
+  // coalescer calls start() there, as a trace listener could.
+  const { framework, internals } = await fixture(t);
+  framework.start();
+  let refusal: unknown = 'start() was never called';
+  (framework as unknown as { pushCoalescer: { suspend(): void } }).pushCoalescer = {
+    suspend: () => {
+      try { framework.start(); refusal = 'start() returned'; } catch (error) { refusal = error; }
+    },
+  };
+  await framework.stop();
+  assert.ok(refusal instanceof Error && refusal.message === 'start() refused: the framework is stopping', String(refusal));
+  assert.equal(internals.running, false);
+});
+
+test('a start() on a running framework does nothing', { timeout: 30_000 }, async (t) => {
+  const { framework, internals } = await fixture(t);
+  framework.start();
+  const loop = internals.loopPromise;
+  framework.start();
+  assert.equal(internals.loopPromise, loop, 'no second loop was started');
+  await framework.stop();
+});
+
+test("a start() made while a module's stop() runs is refused, and no loop outlives that stop", { timeout: 30_000 }, async (t) => {
+  const slow = slowToStop();
+  const { framework, internals } = await fixture(t, [slow.module]);
+  framework.start();
+  const loop = internals.loopPromise!;
+  let loopEnded = false;
+  void loop.then(() => { loopEnded = true; });
+  const stopping = framework.stop();
+  try {
+    await slow.called.promise;
+    assert.throws(() => framework.start(), { message: 'start() refused: the framework is stopping' });
+  } finally {
+    slow.release.resolve();
+  }
+  await stopping;
+  assert.equal(loopEnded, true, 'the loop had ended when the stop settled');
+  assert.equal(internals.running, false);
+  assert.equal(internals.loopPromise, loop, 'no second loop was started');
+  assert.equal(framework.getStore().isClosed(), true, 'the stop closed the store it owns, with nothing running over it');
+});
+
+test("an addModule() made while a module's stop() runs is refused, and its module never starts", { timeout: 30_000 }, async (t) => {
+  const slow = slowToStop();
+  const { framework, internals } = await fixture(t, [slow.module]);
+  framework.start();
+  const stopping = framework.stop();
+  const started: string[] = [];
+  try {
+    await slow.called.promise;
+    await assert.rejects(
+      framework.addModule(module('late', { start: async () => { started.push('late'); } })),
+      { message: 'addModule() refused: the framework is stopping' },
+    );
+  } finally {
+    slow.release.resolve();
+  }
+  await stopping;
+  assert.deepEqual(started, []);
+  assert.equal(internals.moduleRegistry.modules.has('late'), false);
+});
+
+test('once a stop() has settled, the stopping refusal no longer fires', { timeout: 30_000 }, async (t) => {
+  // A store the framework doesn't own, so its close doesn't refuse first.
+  // Only the stopping refusal is asserted: whether start() and addModule()
+  // then begin a working lifecycle is #315's.
+  const { framework } = await fixture(t, [], true);
+  framework.start();
+  await framework.stop();
+  const refusal = (run: () => unknown): unknown => { try { run(); return null; } catch (error) { return error; } };
+  const afterStart = refusal(() => framework.start());
+  assert.ok(!(afterStart instanceof Error && STOPPING.test(afterStart.message)), String(afterStart));
+  await framework.stop().catch(() => {});
+  const added = await framework.addModule(module('after')).then(() => null, (error: unknown) => error);
+  assert.ok(!(added instanceof Error && STOPPING.test(added.message)), String(added));
+});