fix(create): a failed create() releases what it had acquired

changelog.d/389-failed-create-releases.fixed.md

@@ -0,0 +1,4 @@
+- A failed `AgentFramework.create()` now releases what it had acquired before its error reaches the caller, so the same process can call `create()` again (#389). Before, a `create()` that failed after opening its store, such as on an invalid `timeZone`, an MCPL `toolPrefix` collision, a module whose `start()` threw or an out-of-range `toolResultInlineMaxChars`, left the store locked: every later `create()` on that `storePath` got `Store is locked by another process` until the process exited. It also left running what it had started, the liveness watchdog among it, which could then kill a process that carried on after the failure. Now the framework it had begun is stopped as `stop()` stops one (its modules, MCPL servers, event gate, tune-out coordinator and watchdog), and a store it opened from `storePath` is closed. A store passed in as `store` stays open.
+  - The caller of a failed `create()` has no framework to give `stop()` a deadline, so `create()` gives it one: it gives up after 30 s on each of `stop()`'s two cleanup waits, for an operator's operations already running and then for its modules and MCPL servers to stop, where `stop()`'s default waits with no deadline. A module whose `stop()` never settles would otherwise turn a failed boot that exits into one that hangs.
+  - When stopping them fails or runs out of time, or closing the store fails, `create()` rejects with an `AggregateError` holding its own error first and then those failures. Its message begins `AgentFramework.create() failed:` and says what didn't finish: that part of what it started may still be running, that stopping failed only after everything it started had stopped, or that its store may still be locked. It closes a store it opened even when stopping failed. When the release finishes cleanly, `create()`'s own error is rethrown unchanged, as before.
+  - A `WorkspaceModule` that stops before its `initStore()` has applied the state it was handed, as one does when a `create()` that started it fails, or after an `initStore()` that threw partway, no longer saves over its stored state. Its `stop()` saved empty or default mounts there, losing the branch last materialized to disk, so on the next start `materialize` no longer refused a branch that had diverged from it.

src/framework.ts

@@ -1027,6 +1027,15 @@ const WITHHELD_SPILL_DEADLINE_MS = 30_000;
 const DEFAULT_MAINTENANCE_INTERVAL_MS = 5000;
 /** Bound one pass so a large backlog yields to inference and other agents. */
 const MAINTENANCE_TICKS_PER_PASS = 8;
+/** The teardownTimeoutMs a failed create() gives stop(): how long each of its
+ *  cleanup waits, on operator operations and then on modules and MCPL servers
+ *  together, may take. stop() leaves its deadline to its caller, who owns the
+ *  framework, but the caller of a failed create() never got one: unbounded, a
+ *  module whose stop() never settles would turn a failed boot that exits into
+ *  one that hangs. stop()'s other waits are on streams, the event loop,
+ *  maintenance and guard annotations, which a framework create() never
+ *  started has none of. */
+const FAILED_CREATE_TEARDOWN_TIMEOUT_MS = 30_000;
 
 /** Local per-residence provider admission (AF #114 bounded first slice). */
 interface LocalProviderGate {
@@ -1602,6 +1611,11 @@ export class AgentFramework {
   private stopPromise: Promise<void> | null = null;
   private stopCompleted = false;
   private ownedStoreClosed = false;
+  /** Whether the latest stop() stopped in time everything it waits on with
+   * its deadline (an operator's operations, modules and MCPL servers),
+   * whatever failed after that. create()'s release reads it to say whether
+   * part of what a failed create() started may still be running. */
+  private stopFinishedTeardown = false;
   /** An operator's operations in flight, and the config materializations they
    * start. stop() waits until this set is empty before it tears down, unless
    * its wait budget runs out. */
@@ -2107,7 +2121,13 @@ export class AgentFramework {
   }
 
   /**
-   * Create and start the framework.
+   * Create and start the framework. A create() that fails releases what it
+   * had acquired before its error reaches the caller, so that the caller can
+   * try again in the same process: the framework it had begun is stopped, as
+   * stop() stops one, but giving up after 30 s on each of stop()'s two cleanup
+   * waits, for an operator's operations already running and then for its
+   * modules and MCPL servers, and a store it opened is closed. A store the
+   * caller passed in is left open.
    */
   static async create(config: FrameworkConfig): Promise<AgentFramework> {
     // Before anything opens or starts: a refused config must leave nothing behind.
@@ -2127,6 +2147,70 @@ export class AgentFramework {
       throw new Error('Either storePath or store must be provided');
     }
 
+    const building: { framework?: AgentFramework } = {};
+    try {
+      return await AgentFramework.createOn(store, ownsStore, config, building);
+    } catch (error) {
+      throw await AgentFramework.releaseFailedCreate(error, building.framework, store, ownsStore);
+    }
+  }
+
+  /**
+   * The error a failed create() rejects with, once what it acquired is
+   * released: the framework it had begun is stopped, with
+   * FAILED_CREATE_TEARDOWN_TIMEOUT_MS as stop()'s teardownTimeoutMs, and a
+   * store create() opened is closed even when that stop failed or timed out.
+   * stop() keeps an owned store open when its waits on operator operations,
+   * modules or MCPL servers fail or time out, so that it can be called again,
+   * but no one can call it on a framework that create() never returned. When
+   * the release doesn't finish cleanly, the rejection carries create()'s error
+   * and then the release's, and its message says what didn't finish.
+   */
+  private static async releaseFailedCreate(
+    error: unknown,
+    framework: AgentFramework | undefined,
+    store: JsStore,
+    ownsStore: boolean,
+  ): Promise<unknown> {
+    const failures: unknown[] = [];
+    const unfinished: string[] = [];
+    if (framework) {
+      try {
+        await framework.stop(FAILED_CREATE_TEARDOWN_TIMEOUT_MS);
+      } catch (stopError) {
+        failures.push(stopError);
+        // A stop can fail after everything it waits on has stopped, as stop()
+        // counts it (an MCPL server once its transport has sent the kill):
+        // finishing a guard's storage work, in its final sync, or closing the
+        // store. The message then doesn't say part of it may still be running.
+        unfinished.push(framework.stopFinishedTeardown
+          ? 'What it had started stopped, but finishing the stop failed: ' + shutdownErrorMessage(stopError)
+          : 'Stopping what it had started didn\'t finish, so part of it may still be running: ' +
+            shutdownErrorMessage(stopError));
+      }
+    }
+    if (ownsStore && !store.isClosed()) {
+      try {
+        store.close();
+      } catch (closeError) {
+        failures.push(closeError);
+        unfinished.push('Closing its store failed, so the store may still be locked: ' + shutdownErrorMessage(closeError));
+      }
+    }
+    if (failures.length === 0) return error;
+    return new AggregateError([error, ...failures],
+      `AgentFramework.create() failed: ${shutdownErrorMessage(error)}. ${unfinished.join('. ')}`);
+  }
+
+  /** create()'s work once it holds its store. The framework is named in
+   *  `building` as soon as it exists, so that create() can stop it if a later
+   *  step fails. */
+  private static async createOn(
+    store: JsStore,
+    ownsStore: boolean,
+    config: FrameworkConfig,
+    building: { framework?: AgentFramework },
+  ): Promise<AgentFramework> {
     // Register framework states
     try {
       store.registerState({ id: FRAMEWORK_STATE_ID, strategy: 'snapshot' });
@@ -2201,7 +2285,7 @@ export class AgentFramework {
       );
     }
 
-    const framework = new AgentFramework(
+    const framework = building.framework = new AgentFramework(
       store,
       ownsStore,
       config.membrane,
@@ -2684,6 +2768,7 @@ export class AgentFramework {
   }
 
   private async stopOwned(teardownTimeoutMs: number | undefined): Promise<void> {
+    this.stopFinishedTeardown = false;
     this.pushCoalescer?.suspend();
     this.flushCoalescingSnapshot();
     this.running = false;
@@ -2806,6 +2891,7 @@ export class AgentFramework {
     }
 
     const teardownSucceeded = shutdownErrors.length === 0;
+    this.stopFinishedTeardown = teardownSucceeded;
 
     // Those later annotations write nothing, so they finish at once; let them
     // record and edit their stubs before the store closes.
@@ -16363,10 +16449,9 @@ export class AgentFramework {
         // awareness generation.
         await this.connectMcplServerInternal(config, true);
       } catch (error) {
-        if (error instanceof DiscordAwarenessAccountingError) {
-          await this.mcplServerRegistry.closeAll();
-          throw error;
-        }
+        // An accounting failure ends startup. create()'s release closes every
+        // server connected so far, within its deadline.
+        if (error instanceof DiscordAwarenessAccountingError) throw error;
         // Fail-open: log and continue with remaining servers
         const err = error instanceof Error ? error : new Error(String(error));
         console.error(`Failed to connect MCPL server "${config.id}": ${err.message}${withStderr(err)}`);
@@ -16389,15 +16474,13 @@ export class AgentFramework {
     // Start the durable drain only after all startup connections are staged.
     // The awareness tools/call is written before control-plane registration is
     // flushed, preserving the server-side queue -> registration -> call order.
+    // A barrier that fails ends startup the same way, and create()'s release
+    // closes the servers. (A lazy initialization from connectMcplServer has
+    // connected none yet.)
     const startupBarrier = this.installMcplDataPlaneGate();
     this.releaseMcplDataPlaneGate(startupBarrier);
-    try {
-      await startupBarrier.promise;
-      this.completeMcplDataPlaneGate(startupBarrier);
-    } catch (error) {
-      await this.mcplServerRegistry.closeAll();
-      throw error;
-    }
+    await startupBarrier.promise;
+    this.completeMcplDataPlaneGate(startupBarrier);
 
     // Discover tools from all connected servers
     await this.refreshMcplTools();
@@ -16880,13 +16963,15 @@ export class AgentFramework {
     try {
       await awarenessBarrier.promise;
     } catch (error) {
-      // Startup cannot fail open on a broken awareness ledger. Disable the
+      // A connect cannot fail open on a broken awareness ledger. Disable the
       // reconnecting stub/connection before propagating the distinct error to
-      // initializeMcpl, which tears down any other servers and aborts create().
+      // the runtime connect or restart that asked for it. (A startup connect
+      // defers awareness and returns before this gate; a startup that fails
+      // ends in create()'s release, which closes its servers.)
       // Ordering matters: awareness accounting settles BEFORE the §5.3
       // policy round-trip below, so an accounting failure aborts with the
       // grant still empty and every buffered event still behind the gate —
-      // a failed startup never reports a released data plane. (The reverse
+      // a failed connect never reports a released data plane. (The reverse
       // order let a push arrive during the policy await and get flushed by
       // teardown.)
       await connection.close().catch(() => {});

src/modules/workspace/index.ts

@@ -1050,9 +1050,16 @@ export class WorkspaceModule implements Module {
     // the pins and watermarks as the last work left them.
     this.stopped = true;
 
-    // Persist state
-    if (this.ctx) {
-      const activeBranchId = this.store ? this.store.currentBranch().id : undefined;
+    // Persist state, once initStore() has run and the state start() was
+    // handed has been applied (savedState is cleared then). Before that this
+    // session has made no pass or write, so it has nothing to save, and the
+    // stored state stays as it was: saving would put empty or default mounts
+    // over the saved pins. A create() that fails after starting this module
+    // stops it before the host can call initStore() (#389), and an
+    // initStore() that throws after taking the store leaves the state
+    // unapplied.
+    if (this.ctx && this.store && this.savedState === null) {
+      const activeBranchId = this.store.currentBranch().id;
       const state: WorkspaceModuleState = { mounts: {}, activeBranchId };
       for (const [name, mount] of this.mounts) {
         state.mounts[name] = {

src/types/module.ts

@@ -53,7 +53,12 @@ export interface Module {
 
   /**
    * Stop the module.
-   * Called when the framework stops or when the module is removed.
+   * Called when the framework stops or when the module is removed. Also called
+   * when a create() that started the module fails (#389): create() stops the
+   * framework before it rejects, so before the host has it and before anything
+   * a host wires up after create() returns, such as a WorkspaceModule's
+   * initStore(). A stop() that saves state can therefore run before that
+   * wiring, and shouldn't then save over state it was handed and hasn't used.
    */
   stop(): Promise<void>;