Skip to content

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

Merged
zxch3n merged 1 commit into
mainfrom
fix/detach-failed-session-before-terminate
Sep 11, 2026
Merged

zxch3n merged 1 commit into
mainfrom
fix/detach-failed-session-before-terminate

Conversation

@zxch3n

@zxch3n zxch3n commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Related issue

None. Same-repository branch; diagnosed from local daemon logs of two affected sessions on 2026-09-11.

Problem / pressure

A session restored after idle GC whose ACP resume attempt fails (dsh advertises neither loadSession nor resume) lost an entire turn of agent output. SessionManager registers a Session instance's lifecycle events before createAgent, so the cleanup terminate(true) of that never-started instance published terminated as if the live turn's agent had died. MessageHandler finalized the turn and cleared its ACP update target; the execution service then fell back, by design, to a history-replay session, and every update from the replacement agent was dropped (Dropping ACP update without an active/finalized assistant entry target, 34,551 thought chunks and 48 message chunks over six minutes). The turn ended as completed without any agent output while the agent's own transcript held the full answer.

Summary

  • SessionManager.registerSessionEvents keeps a per-instance detacher in a WeakMap; detachSession removes the listeners and drops the instance from sessions only when it is still the mapped instance for that id.
  • The createAgent catch block calls detachSession before session.terminate(true). Lifecycle events now publish only for instances a caller received.
  • The rule is recorded in apps/cli/AGENTS.md (cross-entry contracts) since it binds both the producer (session-manager.ts) and the consumer (message-handler.ts); the decision and rejected alternatives are in the new bug-fix note.
  • Regression test in session-manager.test.ts.

Visual explanation

sequenceDiagram
  participant X as SessionExecutionService
  participant M as SessionManager
  participant S as Session (attempt 1)
  participant H as MessageHandler
  participant S2 as Session (fallback)
  X->>M: createSession(resume=acp-old)
  M->>S: new + registerSessionEvents
  S-->>M: createAgent throws ACP_RESUME_UNSUPPORTED
  rect rgb(255,235,235)
    Note over M,S: before: terminate(true) → 'terminated' → H.finalizeACPState → turn idle
  end
  rect rgb(235,255,235)
    Note over M,S: after: detachSession(S) then terminate(true) and nothing reaches H
  end
  M-->>X: reject
  X->>M: createSession() (history replay fallback)
  M->>S2: new + registerSessionEvents
  S2-->>H: agent updates → routed to the live turn
Loading

Before / after

Before After
Failed resume attempt's cleanup terminate reaches MessageHandler; the live turn is finalized; fallback agent output is dropped; turn recorded as silent failure Failed instance is detached first; the turn stays prompting; fallback agent output lands in the assistant entry; a replacement session under the same id still publishes terminated normally
Startup crash also produced an exit-driven idle-status write Only the rejected createSession promise signals the failure; the caller owns recovery

Test plan

  • apps/cli: vitest run src/session/session-manager.test.ts — 24 passed, including the new failed agent creation test.
  • Ablation: the new test fails against the pre-fix source (terminated called once).
  • apps/cli: vitest run tests/session-execution-service.test.ts — 102 passed (owner of the fallback path, unchanged).
  • apps/cli pnpm typecheck exit 0; oxlint --type-aware no findings on changed lines; prettier clean; pnpm run docs check no errors.
  • Not run: the full workspace pnpm test:ci; no end-to-end restore against a real resume-less agent.

Context handoff

Instructions for reviewing agents

  • Review focus: session-manager.ts detachSession and the createAgent catch block; confirm no other consumer of exit/terminated (machine-runtime.ts, lody-fleet.ts terminal cleanup) needs the event for an instance that never left createSession.
  • Decisions to challenge: fixing at the producer instead of re-owning the turn in the execution-service fallback or guarding in MessageHandler's listener (rationale in the note); the prepared-session path shares the same catch block.
  • Plausible failures / evidence gaps: a startup crash no longer writes idle status via exit; verified only by unit tests, not by an end-to-end restore against a real agent.

Authoring context

  • User goal / directives: diagnose why a dsh session's continuation turn ran for minutes and failed with no output, then fix the root cause in session-manager with the trade-off explained in code.
  • Constraints / non-goals: no change to the execution-service fallback or to MessageHandler; dsh's missing loadSession capability is an upstream gap and out of scope.
  • Risk-bearing decisions: lifecycle events are suppressed for a Session instance whose createAgent failed; the map entry is removed only if it is still that instance.
  • Destructive or irreversible behavior: none; the failed instance is still terminated and its processes killed as before.
  • Deliberately not done or tested: full workspace test run and end-to-end agent restore; the note's Chinese translation is pending.
  • Unknowns / confidence: high confidence in the mechanism (log-verified on two sessions and reproduced by the ablated test); moderate on unobserved consumers that might have relied on the spurious event.

🤖 Generated with Claude Code

`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 <noreply@anthropic.com>
@zxch3n
zxch3n merged commit 32cf745 into main Sep 11, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant