fix(cli): detach a failed Session instance before its cleanup terminate

.agents/notes/implemented/bug-fix/2026-09-11-failed-session-create-lifecycle-events.md

@@ -0,0 +1,76 @@
+# Detach a failed Session instance before its cleanup terminate
+
+Status: implemented
+Translation: pending
+
+## Abstract
+
+A session restored after idle GC lost an entire turn of agent output when the ACP
+resume attempt failed and the execution service fell back to a history-replay
+session. `SessionManager` registers a `Session` instance's lifecycle events before
+`createAgent`, so the cleanup `terminate` of the failed instance published
+`terminated` as if the live turn's agent had died; `MessageHandler` finalized the
+turn, and every update from the replacement agent was dropped without a target.
+The manager now detaches the instance from its listeners and the live map before
+that terminate, so lifecycle events publish only for instances a caller received.
+The trade-off is that a startup crash no longer triggers the `exit`-driven idle
+status write; the rejected `createSession` promise is the sole signal and the
+caller owns recovery.
+
+## Problem
+
+Observed on 2026-09-11 in two dsh (`@deepseek-ai/dsh-acp-demo@0.1.1-rc.2`)
+sessions on the same machine. dsh advertises neither `loadSession` nor `resume`,
+so any continuation after the idle GC evicts the agent takes this path:
+
+1. `createSessionInnerWithAgent` calls `createSessionInner`, which constructs the
+   `Session`, calls `registerSessionEvents`, and stores it in `sessions`.
+2. `createAgent` throws `[ACP_RESUME_UNSUPPORTED]`. The catch block calls
+   `session.terminate(true)`, which emits `terminated`.
+3. The manager forwards it. `MessageHandler`'s `terminated` listener runs
+   `finalizeACPState`, which clears the transient turn state to `idle` and stamps
+   the assistant entry finished.
+4. `SessionExecutionService` catches the resume error and, by design, creates a
+   fresh session with a replay prompt. `activateTurnACPUpdateTarget` is a no-op on
+   an idle turn, so `enqueueACPUpdate` logged
+   `Dropping ACP update without an active/finalized assistant entry target` for
+   34,551 thought chunks and 48 message chunks over six minutes.
+5. The prompt resolved `completed`; the turn was recorded through
+   `recordSilentTurnFailure`. The agent's own transcript held the full answer.
+
+The defect is independent of dsh: any agent whose resume fails would lose the
+fallback's output. dsh's missing `loadSession` is a capability gap that only
+degrades context after GC; it caused no loss.
+
+## Decision
+
+Fix at the source of the spurious event rather than downstream:
+
+- `registerSessionEvents` keeps its handlers and stores a per-instance detacher in
+  a `WeakMap`. The `createAgent` catch block calls `detachSession(session)` before
+  `terminate(true)`: listeners are removed and the instance is dropped from
+  `sessions` only if it is still the mapped instance for that id. The identity
+  check matters because a recovery path may already be creating the replacement
+  under the same session id.
+
+Alternatives considered:
+
+- Re-`beginTurn` in the execution service's fallback path. Rejected: the handler's
+  `terminated` listener is fire-and-forget and clears turn state in a `finally`
+  after `waitUntilSynced`, so it can land after the replacement prompt started and
+  wipe a live turn; it also stamps `finished` on the entry and registers a late
+  update target that would then need undoing.
+- Guard in `MessageHandler`'s listener. Rejected: the event carries only a session
+  id, so the handler cannot distinguish the turn's bound instance from one that
+  never prompted without new plumbing across three layers.
+
+## Verification
+
+`apps/cli/src/session/session-manager.test.ts` drives the public `createSession`
+with a custom ACP launch and `Session.prototype.createAgent` rejecting with
+`[ACP_RESUME_UNSUPPORTED]`: no `terminated`/`exit` reaches manager consumers, the
+session is absent from the map, and a replacement created under the same id is the
+mapped instance whose later `terminateSession` does publish `terminated`.
+
+Not verified: an end-to-end restore against a real agent that lacks resume, and
+the `SessionExecutionService` fallback path itself, whose behavior is unchanged.

apps/cli/AGENTS.md

@@ -56,6 +56,11 @@ execution/consent rules. These rules also bind CLI callers outside that director
   runtime rejections in debug diagnostics: Codex/Claude mismatches for model, effort, Fast, or Plan
   never become visible `agent_warning` notices, while other rejections still do. Claude Fable
   models omit Fast, so `fast=false` is skipped as a no-op while `fast=true` is dispatched.
+- INVARIANT: `SessionManager` publishes `exit`/`terminated` only for `Session` instances a caller
+  received. `MessageHandler` treats them as "the live turn's agent died" and finalizes the turn, so
+  a `createAgent` failure detaches the instance BEFORE its cleanup `terminate`; otherwise a
+  recovery such as the resume-to-replay fallback loses every update of the replacement agent
+  ([note](../../.agents/notes/implemented/bug-fix/2026-09-11-failed-session-create-lifecycle-events.md)).
 - `lody feedback` and MCP `lody_feedback` submit only caller-provided suggestion text plus CLI
   version, platform, and architecture — never cwd, paths, hostname, environment, logs, prompts,
   history, or file contents. Keep obvious-secret rejection in the CLI and the hosted API boundary.

apps/cli/src/session/AGENTS.md

@@ -2,7 +2,7 @@
 
 `CLAUDE.md` is a symlink to this file. Edit `AGENTS.md` only.
 
-Rules only; responsibilities and reasoning: [README.md](README.md). Worktrees and git
+Rules only; rationale: [README.md](README.md). Worktrees and git
 credentials: [worktree/AGENTS.md](worktree/AGENTS.md). Architecture: context/message-flow.md.
 Contract: specs/session-orchestration.md.
 

apps/cli/src/session/session-manager.test.ts

@@ -8,6 +8,7 @@ import {
   buildSessionPreparationRequestKey,
   buildSessionLaunchConfig,
   normalizeSessionPreparationRunConfigForDedup,
+  type ACPSessionId,
   type AgentConfigId,
   type LocalProjectId,
   type MachineId,
@@ -1319,3 +1320,79 @@ describe('SessionManager preparation resource accounting', () => {
     expect(preparationApplyLimits).toHaveBeenCalledTimes(1);
   });
 });
+
+describe('SessionManager failed agent creation', () => {
+  let tempHome: string;
+
+  beforeEach(() => {
+    tempHome = mkdtempSync(path.join(os.tmpdir(), 'lody-session-manager-'));
+    vi.stubEnv('HOME', tempHome);
+    vi.stubEnv('LODY_LOCKS_DIR', path.join(tempHome, 'locks'));
+  });
+
+  afterEach(() => {
+    vi.restoreAllMocks();
+    vi.unstubAllEnvs();
+    rmSync(tempHome, { recursive: true, force: true });
+  });
+
+  it('publishes no lifecycle events for an instance whose agent never started', async () => {
+    const sourceDir = createLocalRepo(tempHome);
+    const sessionId = 'agent-start-failed' as SessionId;
+    const docs = new Map<SessionId, FakeSessionDoc>();
+    const manager = new SessionManager(
+      createLogger(),
+      'token',
+      'machine-1' as MachineId,
+      'workspace-1' as WorkspaceId,
+      createWorkspaceDocument(docs),
+      {
+        sessionSandboxFactory: async () => createNoopSessionSandbox(),
+        cloudPort: createTestCloudPort(),
+      }
+    );
+    const terminated = vi.fn();
+    const exit = vi.fn();
+    manager.on('terminated', terminated);
+    manager.on('exit', exit);
+    const config = createSessionConfig({
+      sessionId,
+      agentCliType: 'custom',
+      agentType: 'custom-agent',
+      customAcp: { command: 'agent', args: [] },
+      workdir: sourceDir,
+    });
+    const createAgent = vi
+      .spyOn(Session.prototype, 'createAgent')
+      .mockRejectedValueOnce(
+        new Error('[ACP_RESUME_UNSUPPORTED] agent_did_not_advertise_resume_or_loadSession')
+      );
+
+    // The resume attempt fails after the instance was registered. A consumer
+    // that treats `terminated` as "the session running this turn died" would
+    // finalize the live turn here and lose the replacement agent's output.
+    await expect(
+      manager.createSession(config, { resumeSessionId: 'acp-old' as ACPSessionId })
+    ).rejects.toThrow('ACP_RESUME_UNSUPPORTED');
+    expect(createAgent).toHaveBeenCalledTimes(1);
+    expect(manager.getSession(sessionId)).toBeNull();
+    expect(terminated).not.toHaveBeenCalled();
+    expect(exit).not.toHaveBeenCalled();
+
+    // The replacement under the same id is a normal live session: it is the
+    // one in the map and its termination still reaches consumers.
+    createAgent.mockResolvedValueOnce('acp-replacement');
+    const sessionDoc = docs.get(sessionId) as FakeSessionDoc & {
+      setACPSessionId?: (id: ACPSessionId) => Promise<void>;
+    };
+    sessionDoc.setACPSessionId = vi.fn(async () => undefined);
+    const replacement = await manager.createSession(config);
+    expect(manager.getSession(sessionId)).toBe(replacement);
+    expect(terminated).not.toHaveBeenCalled();
+
+    await manager.terminateSession(sessionId, true);
+    expect(terminated).toHaveBeenCalledTimes(1);
+    expect(terminated).toHaveBeenCalledWith(expect.objectContaining({ sessionId }));
+    expect(manager.getSession(sessionId)).toBeNull();
+  });
+});

apps/cli/src/session/session-manager.ts

@@ -457,6 +457,8 @@ export class SessionManager extends EventEmitter<SessionManagerEvents> {
   private githubTokenManager: CloudGithubTokenManager | null = null;
   private gitCredentialBroker: GitCredentialBroker | null = null;
   private readonly sessions = new Map<SessionId, Session>();
+  /** Per-instance listener teardown for `detachSession`; see `registerSessionEvents`. */
+  private readonly sessionEventDetachers = new WeakMap<Session, () => void>();
   private readonly pendingSessionCreates = new Map<SessionId, Promise<ISession>>();
   private readonly pendingTerminationPromises = new Map<SessionId, Promise<void>>();
   private readonly preparationSessions = new Map<SessionId, Session>();
@@ -1447,6 +1449,19 @@ export class SessionManager extends EventEmitter<SessionManagerEvents> {
       this.logger.error(
         `[${config.sessionId}] Failed to create agent: ${error instanceof Error ? error.message : 'Unknown error'}`
       );
+      // This instance never left `createSession`, so nobody upstream holds it
+      // and nobody may observe its death. Detach it BEFORE terminate: the
+      // Session still emits `terminated`/`exit` while it kills its processes,
+      // and with the manager listeners attached those events reach
+      // MessageHandler as "the session running turn X died". The handler then
+      // finalizes the live turn (clears its ACP update target, stamps the
+      // assistant entry finished) even though the caller is about to recover —
+      // e.g. the execution service falls back from a failed ACP resume to a
+      // history-replay session — and every update from the replacement agent
+      // is dropped for lack of a target. The trade-off: a startup crash no
+      // longer produces an `exit`-driven idle-status write; the rejected
+      // promise is the only signal, and the caller owns the recovery.
+      this.detachSession(session);
       try {
         await session.terminate(true);
       } catch (terminateError) {
@@ -2260,31 +2275,54 @@ export class SessionManager extends EventEmitter<SessionManagerEvents> {
   }
 
   private registerSessionEvents(session: Session): void {
-    session.on('output', (event: SessionOutputEvent) => {
+    const onOutput = (event: SessionOutputEvent): void => {
       this.emit('output', event);
-    });
-
-    session.on('error', (event: SessionErrorEvent) => {
+    };
+    const onError = (event: SessionErrorEvent): void => {
       this.emit('error', event);
-    });
-
-    session.on('exit', (event: SessionExitEvent) => {
+    };
+    const onExit = (event: SessionExitEvent): void => {
       this.sessions.delete(event.sessionId);
       void this.rebalanceSessionSandboxes();
       this.emit('exit', event);
-    });
-
-    session.on('terminated', (event: SessionExitEvent) => {
+    };
+    const onTerminated = (event: SessionExitEvent): void => {
       this.sessions.delete(event.sessionId);
       void this.rebalanceSessionSandboxes();
       const terminatedEvent: SessionTerminatedEvent = {
         sessionId: event.sessionId,
         exitCode: event.exitCode,
       };
       this.emit('terminated', terminatedEvent);
+    };
+    session.on('output', onOutput);
+    session.on('error', onError);
+    session.on('exit', onExit);
+    session.on('terminated', onTerminated);
+    this.sessionEventDetachers.set(session, () => {
+      session.off('output', onOutput);
+      session.off('error', onError);
+      session.off('exit', onExit);
+      session.off('terminated', onTerminated);
     });
   }
 
+  /**
+   * Stop publishing a Session instance's lifecycle events and drop it from the
+   * live map. Only for an instance that was registered by `createSessionInner`
+   * but whose creation then failed: it was never returned to a caller, so from
+   * the outside it never existed. Keyed by instance, not session id, because a
+   * recovery path may already be creating the replacement under the same id.
+   */
+  private detachSession(session: Session): void {
+    this.sessionEventDetachers.get(session)?.();
+    this.sessionEventDetachers.delete(session);
+    if (this.sessions.get(session.sessionId) === session) {
+      this.sessions.delete(session.sessionId);
+      void this.rebalanceSessionSandboxes();
+    }
+  }
+
   private async rebalanceSessionSandboxes(): Promise<void> {
     const run = async (): Promise<void> => {
       const activeSessions = [...this.sessions.values(), ...this.preparationSessions.values()];