From b0affa020d251d2099cd4b7b44daf6caa22c1684 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Fri, 11 Sep 2026 13:39:33 +0800 Subject: [PATCH] fix(cli): detach a failed Session instance before its cleanup terminate `SessionManager` registers a `Session`'s lifecycle events before `createAgent`, so the cleanup `terminate(true)` of an instance whose agent never started published `terminated` as if the live turn's agent had died. `MessageHandler` then finalized the turn and cleared its ACP update target, and when the execution service fell back from a failed ACP resume to a history-replay session every update from the replacement agent was dropped; the turn was recorded as "completed without any agent output". Detach the instance from the manager listeners and the live map before the cleanup terminate. Lifecycle events now publish only for instances a caller received; the rejected `createSession` promise is the sole signal for a startup failure and the caller owns the recovery. Regression test drives the public `createSession` with `createAgent` rejecting `[ACP_RESUME_UNSUPPORTED]` and asserts no `terminated`/`exit` reaches consumers while a replacement under the same id still publishes. Model: claude-fable-5-1 Co-Authored-By: Claude Fable 5.1 --- ...-failed-session-create-lifecycle-events.md | 76 ++++++++++++++++++ apps/cli/AGENTS.md | 5 ++ apps/cli/src/session/AGENTS.md | 2 +- apps/cli/src/session/session-manager.test.ts | 77 +++++++++++++++++++ apps/cli/src/session/session-manager.ts | 58 +++++++++++--- 5 files changed, 207 insertions(+), 11 deletions(-) create mode 100644 .agents/notes/implemented/bug-fix/2026-09-11-failed-session-create-lifecycle-events.md diff --git a/.agents/notes/implemented/bug-fix/2026-09-11-failed-session-create-lifecycle-events.md b/.agents/notes/implemented/bug-fix/2026-09-11-failed-session-create-lifecycle-events.md new file mode 100644 index 000000000..cc3ccfd98 --- /dev/null +++ b/.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. diff --git a/apps/cli/AGENTS.md b/apps/cli/AGENTS.md index c23152ce6..809e2cd6c 100644 --- a/apps/cli/AGENTS.md +++ b/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. diff --git a/apps/cli/src/session/AGENTS.md b/apps/cli/src/session/AGENTS.md index 7ebe6819e..da502aa6d 100644 --- a/apps/cli/src/session/AGENTS.md +++ b/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. diff --git a/apps/cli/src/session/session-manager.test.ts b/apps/cli/src/session/session-manager.test.ts index 6a4bf24f0..90e32cc9f 100644 --- a/apps/cli/src/session/session-manager.test.ts +++ b/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(); + 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; + }; + 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(); + }); +}); diff --git a/apps/cli/src/session/session-manager.ts b/apps/cli/src/session/session-manager.ts index c004cb581..1e5968ce8 100644 --- a/apps/cli/src/session/session-manager.ts +++ b/apps/cli/src/session/session-manager.ts @@ -457,6 +457,8 @@ export class SessionManager extends EventEmitter { private githubTokenManager: CloudGithubTokenManager | null = null; private gitCredentialBroker: GitCredentialBroker | null = null; private readonly sessions = new Map(); + /** Per-instance listener teardown for `detachSession`; see `registerSessionEvents`. */ + private readonly sessionEventDetachers = new WeakMap void>(); private readonly pendingSessionCreates = new Map>(); private readonly pendingTerminationPromises = new Map>(); private readonly preparationSessions = new Map(); @@ -1447,6 +1449,19 @@ export class SessionManager extends EventEmitter { 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,21 +2275,18 @@ export class SessionManager extends EventEmitter { } 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 = { @@ -2282,9 +2294,35 @@ export class SessionManager extends EventEmitter { 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 { const run = async (): Promise => { const activeSessions = [...this.sessions.values(), ...this.preparationSessions.values()];