From 547662842b1e520c60231a65e7ca1e9f1cbdbfd2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Fri, 14 Aug 2026 15:36:09 +0000 Subject: [PATCH] a dead claude process no longer hangs the chat forever MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit the sdk runs two independent tasks per session: the consumer loop (`for await (const msg of q)`) and an input pump that writes the queue to the child's stdin. the consumer loop's `finally` is what removes a session from the map — but when the CHILD dies it is the input pump that fails, with `ProcessTransport is not ready for writing`, and that rejection neither ends the consumer loop nor is caught anywhere. so the loop stayed parked on a stream with no writer, `finally` never ran, the session stayed in the map, and spawnClaudeStreaming handed every later turn to the same corpse. each one pushed a message onto a queue nobody drained: no error, no result, no timeout. the client spun forever and the only trace was one unhandledRejection line in the sidecar log. observed on the host today; the only cure was pm2 restart officer-claude-code. a member's turn already supplied its own spawn function because it has to go through setpriv. the owner had none, and therefore no place to observe the child — which is exactly why its death was invisible. so give the owner one too, and wrap both in watchChild: on exit or error, drop the session from the map and, if a turn was in flight, tell the client. emitting only while generating is deliberate. a child that exits between turns is invisible to the user, and an error bubble arriving in a chat nobody is looking at would be noise — dropping the map entry is the whole repair there, because the next turn builds a fresh session and resumes the transcript by id. the stall timer now tears the session down as well. it used to keep it — "it may still be working, and the next turn resumes it" — which is right for a slow agent and wrong for a wedged one: the session stayed broken, so every later turn hung the same way and "send again to continue" was a lie. sessions with background tasks outstanding are still left alone, since a job can be silent far longer than ten minutes and still land its notification. verified live against the real manager: killed the child mid-turn, saw the error surface and the next turn rebuild the session. Co-Authored-By: Claude Opus 5 --- src/servers/sidecar/claude/claude-manager.ts | 100 ++++++++++++++++++- 1 file changed, 96 insertions(+), 4 deletions(-) diff --git a/src/servers/sidecar/claude/claude-manager.ts b/src/servers/sidecar/claude/claude-manager.ts index 5ef331bc..eb721668 100644 --- a/src/servers/sidecar/claude/claude-manager.ts +++ b/src/servers/sidecar/claude/claude-manager.ts @@ -1,7 +1,8 @@ import { existsSync } from 'node:fs'; +import { spawn } from 'node:child_process'; import { homedir } from 'node:os'; import { join } from 'node:path'; -import { query, type Query } from '@anthropic-ai/claude-agent-sdk'; +import { query, type Query, type SpawnOptions, type SpawnedProcess } from '@anthropic-ai/claude-agent-sdk'; import type { ChatEvent, PromptImage } from '../../api/chat/types'; import type { ClaudeSpawnParams, ClaudeSpawnStreamingParams, ClaudeCodeResult, LiveClaudeSession } from '../protocol'; import { setClaudeSession, clearClaudeSession, getClaudeSession } from './state'; @@ -216,6 +217,79 @@ const COMPACT_STALL_TIMEOUT_MS = 20 * 60 * 1000; const sessions = new Map(); +/** + * The owner's spawn — what the SDK would do by default, written out so it can be wrapped by `watchChild`. + * + * A member's turn already supplies its own (`spawnClaudeAsMember`) because it has to go through `setpriv`. + * The owner had no such function and therefore no place to observe the child, which is precisely why its + * death was invisible. The existence check mirrors the SDK's default: without it a bad `CLAUDE_BIN` fails + * as a write to a closed pipe several seconds later, naming nothing useful. + */ +function spawnClaudeAsOwner({ command, args, cwd, env, signal }: SpawnOptions): SpawnedProcess { + if (!existsSync(command)) throw new Error(`claude CLI not found at ${command}`); + const child = spawn(command, args, { cwd, env, signal, stdio: ['pipe', 'pipe', 'pipe'] }); + // Non-null by construction: 'pipe' on all three. Mirrors the cast in `spawn-as-member.ts`. + return child as unknown as SpawnedProcess; +} + +/** + * Drop a session whose `claude` process is gone, and tell whoever was waiting. + * + * This closes the hole that made a dead agent look like a slow one **forever**. The SDK runs two + * independent tasks per session: the consumer loop (`for await (const msg of q)`) and an input pump that + * writes the queue to the child's stdin. The consumer loop's `finally` is what removes a session from the + * map — but when the CHILD dies, it is the input pump that fails, with `ProcessTransport is not ready for + * writing`, and that rejection neither ends the consumer loop nor is caught anywhere. So the loop stayed + * parked on a stream with no writer, `finally` never ran, the session stayed in the map, and + * `spawnClaudeStreaming` handed every later turn to the same corpse. Each one pushed a message onto a + * queue nobody drained: no error, no result, no timeout. The client spun forever, and the only trace was + * an `unhandledRejection` line in the sidecar log. + * + * Observed on 2026-08-14; the session had to be cleared with `pm2 restart officer-claude-code`. + * + * Emitting only while a turn is in flight is deliberate. A child that exits between turns is invisible to + * the user, and an error bubble arriving in a chat nobody is looking at would be noise — dropping the map + * entry is the whole repair there, because the next turn then builds a fresh session and resumes the + * transcript by id. + */ +function dropDeadSession(session: PersistentSession, detail: string): void { + // Identity, not key: a later turn may have already replaced this entry, and killing its session because + // its predecessor's process exited would break the live conversation instead of a dead one. + if (sessions.get(session.sessionKey) !== session) return; + sessions.delete(session.sessionKey); + if (session.idleTimer) clearTimeout(session.idleTimer); + if (session.stallTimer) clearTimeout(session.stallTimer); + session.idleTimer = undefined; + session.stallTimer = undefined; + + const wasGenerating = session.isGenerating; + session.isGenerating = false; + // A deliberate teardown (kill / idle GC) aborts first, and its exit is not news. + if (session.abort.signal.aborted) return; + + console.error(`[claude:exit:${session.sessionKey}] ${detail}`); + if (!wasGenerating) return; + session.emit({ + type: 'error', + message: 'The agent process exited unexpectedly. Your conversation is safe — send again to continue.', + }); +} + +/** Wrap a spawn so the session self-heals when its child goes away. */ +function watchChild( + inner: (options: SpawnOptions) => SpawnedProcess, + session: PersistentSession, +): (options: SpawnOptions) => SpawnedProcess { + return (options: SpawnOptions): SpawnedProcess => { + const child = inner(options); + child.on('exit', (code, signal) => + dropDeadSession(session, `claude exited (code=${code ?? 'null'} signal=${signal ?? 'null'})`), + ); + child.on('error', (err: Error) => dropDeadSession(session, `claude failed to start: ${err.message}`)); + return child; + }; +} + /** A hand-rolled async iterable we can push turns onto and close on teardown. */ function makeInputQueue() { const buf: SdkUserMessage[] = []; @@ -273,13 +347,25 @@ function armStall(session: PersistentSession): void { session.isGenerating = false; session.compactStartedAt = undefined; session.interrupted = false; - if (session.pendingTasks.size === 0) armIdle(session); session.emit({ type: 'error', message: compacting ? `Compaction has produced nothing for ${waited} minutes and has been given up on. Your conversation is safe — send again to continue.` : `The agent has produced nothing for ${waited} minutes and has been given up on. Your conversation is safe — send again to continue.`, }); + // A background job can be silent for far longer than this and still land its `task_notification`, so + // a stall with tasks outstanding keeps the old behaviour and leaves the session alone. + if (session.pendingTasks.size > 0) return; + + // Otherwise tear it down rather than leaving it armed for the next turn. + // + // This used to keep the session — "it may still be working, and the next turn resumes it" — which is + // the right instinct for a SLOW agent and exactly wrong for a wedged one: a session that has said + // nothing for ten minutes because its transport is broken stays broken, so every later turn hangs + // the same way and the message above ("send again to continue") is a lie. Killing costs a resume, + // which is what the message already promises; the transcript id survives in `claudeSessions`, so the + // next turn continues the same conversation. + killClaudeSession(session.sessionKey, session.userId); }, compacting ? COMPACT_STALL_TIMEOUT_MS : STALL_TIMEOUT_MS, ); @@ -361,12 +447,18 @@ function createSession(params: ClaudeSpawnStreamingParams, onEvent: (event: Chat // authoritative for settings, and `~` is decided by the HOME the process gets. Pointing the SDK at a // member's binary while spawning as the service user would read the OWNER'S settings and credential // while executing the member's code — the worst of both, and it would look like it worked. + // + // Both branches go through `watchChild`: the owner's spawn exists only so there is something to + // wrap (see `spawnClaudeAsOwner`), because a child nobody watches is a session that can die silently. ...(params.member ? { pathToClaudeCodeExecutable: claudeBinIn(params.member.home), - spawnClaudeCodeProcess: spawnClaudeAsMember(params.member), + spawnClaudeCodeProcess: watchChild(spawnClaudeAsMember(params.member), session), } - : { pathToClaudeCodeExecutable: CLAUDE_BIN }), + : { + pathToClaudeCodeExecutable: CLAUDE_BIN, + spawnClaudeCodeProcess: watchChild(spawnClaudeAsOwner, session), + }), settingSources: ['user', 'project', 'local'], env: cleanEnv as Record, stderr: (d: string) => {