diff --git a/src/servers/sidecar/claude/claude-manager.ts b/src/servers/sidecar/claude/claude-manager.ts index d9cee15c..147e0f55 100644 --- a/src/servers/sidecar/claude/claude-manager.ts +++ b/src/servers/sidecar/claude/claude-manager.ts @@ -6,6 +6,7 @@ import { query, type Query, type SpawnOptions, type SpawnedProcess } from '@anth import type { ChatEvent, PromptImage } from '../../api/chat/types'; import type { ClaudeSpawnParams, ClaudeSpawnStreamingParams, ClaudeCodeResult, LiveClaudeSession } from '../protocol'; import { setClaudeSession, clearClaudeSession, getClaudeSession } from './state'; +import { diagnoseExit, stderrReason } from './exit-diagnosis'; import { createParseState, processMessage } from './stream-parser'; import { spawnClaudeAsMember } from './spawn-as-member'; import { claudeBinIn } from '@@/os-user-claude'; @@ -217,6 +218,12 @@ type PersistentSession = { * side that called `interrupt()` knows better, so it says so here. */ interrupted: boolean; + /** What we passed as `--resume`, so a failure can name it and decide whether it was to blame. */ + resumeId?: string; + /** The tail of the child's stderr, so its exit can carry a reason instead of just a number. */ + stderrTail: string; + /** Whether the child ever produced a single message. False plus a non-zero exit means it never started. */ + sawOutput: boolean; /** * When the `PreCompact` hook fired, so the `compact_boundary` that closes it can carry how long the * silence lasted. The harness reports the boundary but not the duration, and the duration is the part @@ -300,12 +307,29 @@ function dropDeadSession(session: PersistentSession, detail: string): void { // 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.', + const diagnosis = diagnoseExit({ + sawOutput: session.sawOutput, + resumeId: session.resumeId, + stderrTail: session.stderrTail, }); + const reason = stderrReason(session.stderrTail); + + console.error( + `[claude:exit:${session.sessionKey}] ${detail} resume=${session.resumeId ?? 'none'} ` + + `sawOutput=${session.sawOutput}${reason ? ` stderr="${reason}"` : ' stderr='}`, + ); + + // Dropping the mapping is what makes the thread usable again — until this, the failing id was retried on + // every message forever. See `diagnoseExit` for why `sawOutput` is the thing that decides it. + if (diagnosis.clearResume) { + clearClaudeSession(session.sessionKey); + console.error( + `[claude:exit:${session.sessionKey}] cleared the stored resume id — the next message starts a new session`, + ); + } + + if (!wasGenerating) return; + session.emit({ type: 'error', message: diagnosis.message }); } /** Wrap a spawn so the session self-heals when its child goes away. */ @@ -434,6 +458,8 @@ function createSession(params: ClaudeSpawnStreamingParams, onEvent: (event: Chat isGenerating: false, pendingTasks: new Set(), interrupted: false, + stderrTail: '', + sawOutput: false, }; // Strip the nested-session guard vars so the SDK can spawn `claude` (mirrors the old spawn env clean). @@ -441,6 +467,23 @@ function createSession(params: ClaudeSpawnStreamingParams, onEvent: (event: Chat const resumeId = getClaudeSession(sessionKey, params.userId) ?? params.resumeSessionId; const subModel = params.model?.split('/')[1]; + session.resumeId = resumeId; + + /** + * What we are about to run, before we run it. + * + * A member's turn died on `exit code=1` with no stderr and nothing else logged, and the resume id — the + * one input most likely to be wrong — appeared in no log anywhere. Every hypothesis about it therefore + * had to be tested by hand on the server, which took an evening and produced two wrong answers before + * the right question. One line here would have replaced all of it. + * + * The resume id in particular is not the id in the URL: it comes from the stored map, so the two can + * disagree, and only this line can show that they have. + */ + console.log( + `[claude:spawn:${sessionKey}] resume=${resumeId ?? 'none'} cwd=${params.cwd ?? 'default'} ` + + `as=${params.member?.osUser ?? 'owner'} model=${params.model ?? 'default'}`, + ); const q = query({ prompt: input.gen as AsyncIterable, @@ -495,7 +538,11 @@ function createSession(params: ClaudeSpawnStreamingParams, onEvent: (event: Chat settingSources: ['user', 'project', 'local'], env: cleanEnv as Record, stderr: (d: string) => { - if (d.trim()) console.error(`[claude:stream:${sessionKey}] ${d.slice(0, 300)}`); + if (!d.trim()) return; + console.error(`[claude:stream:${sessionKey}] ${d.slice(0, 300)}`); + // Kept as well as logged, so the exit can carry it. A reason that only exists in the server's log + // is a reason the person who hit it cannot see, and they are the one who has to decide what to do. + session.stderrTail = `${session.stderrTail}${d}`.slice(-2000); }, ...(subModel ? { model: subModel } : {}), ...(resumeId ? { resume: resumeId } : {}), @@ -577,6 +624,9 @@ function createSession(params: ClaudeSpawnStreamingParams, onEvent: (event: Chat }; try { for await (const msg of q as AsyncGenerator>) { + // One message — any message — means the child got far enough to talk. That distinguishes a resume + // that could not even load from a turn that broke halfway, which want opposite handling. + session.sawOutput = true; processMessage(msg, state, { onEvent: emit, onSessionId: (id: string) => setClaudeSession(sessionKey, id, params.userId), diff --git a/src/servers/sidecar/claude/exit-diagnosis.test.ts b/src/servers/sidecar/claude/exit-diagnosis.test.ts new file mode 100644 index 00000000..14760e77 --- /dev/null +++ b/src/servers/sidecar/claude/exit-diagnosis.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, test } from 'bun:test'; +import { diagnoseExit, stderrReason } from './exit-diagnosis'; + +// The bug these encode: a member's chat answered "Claude Code process exited with code 1" four times in a +// row and could never recover, because every message retried the same `--resume` id and nothing ever +// reconsidered it. A fresh chat worked throughout, which is what identifies the resume as the difference. + +describe('diagnoseExit', () => { + test('a resuming turn that died before saying anything gives up on the resume', () => { + const d = diagnoseExit({ sawOutput: false, resumeId: '49527b33', stderrTail: '' }); + expect(d.clearResume).toBe(true); + expect(d.message).toContain('could not reopen'); + // The cost has to be stated: the next turn genuinely will not have the history. + expect(d.message).toContain('will not remember'); + }); + + // The important negative. A turn that spoke and then broke has a working transcript, so discarding the + // resume would turn one failed turn into a permanently amnesiac conversation. + test('a turn that produced output first keeps its conversation', () => { + const d = diagnoseExit({ sawOutput: true, resumeId: '49527b33', stderrTail: 'boom' }); + expect(d.clearResume).toBe(false); + expect(d.message).toContain('conversation is safe'); + }); + + test('a NEW session that dies has no resume to blame, so nothing is cleared', () => { + const d = diagnoseExit({ sawOutput: false, resumeId: undefined, stderrTail: '' }); + expect(d.clearResume).toBe(false); + expect(d.message).toContain('exited unexpectedly'); + }); + + test("the child's own words are carried to the person, not just to the log", () => { + const d = diagnoseExit({ sawOutput: false, resumeId: 'abc', stderrTail: 'No conversation found with session ID' }); + expect(d.message).toContain('No conversation found with session ID'); + }); + + // A silent exit is the case that started this: code 1, nothing on stderr. The sentence still has to read. + test('a silent death produces a clean sentence rather than a dangling colon', () => { + const d = diagnoseExit({ sawOutput: false, resumeId: 'abc', stderrTail: ' \n \n' }); + expect(d.message).not.toContain(':'); + expect(d.message).toContain('could not reopen this conversation.'); + }); +}); + +describe('stderrReason', () => { + test('keeps the last lines, where the actual error is', () => { + expect(stderrReason('one\ntwo\nthree\nfour')).toBe('two three four'); + }); + + test('is empty for whitespace, so callers can test it as a condition', () => { + expect(stderrReason('')).toBe(''); + expect(stderrReason(' \n \t ')).toBe(''); + }); + + test('stays short enough to sit inside a sentence', () => { + expect(stderrReason('x'.repeat(5000)).length).toBeLessThanOrEqual(300); + }); +}); diff --git a/src/servers/sidecar/claude/exit-diagnosis.ts b/src/servers/sidecar/claude/exit-diagnosis.ts new file mode 100644 index 00000000..d30df2d1 --- /dev/null +++ b/src/servers/sidecar/claude/exit-diagnosis.ts @@ -0,0 +1,62 @@ +/** + * What to tell someone whose agent process just died, and whether to stop trying to resume. + * + * Split out of `dropDeadSession` because it is the part with branches and the part a person reads, and + * because everything around it needs the SDK to construct — which is how the old version's single sentence + * went unexamined: it could not be exercised without spawning a real `claude`. + * + * ── Why the distinction it draws is the whole point ── + * + * A member's chat failed with `exited with code 1`, four times, identically, and could never recover. A + * fresh chat worked. Everything else — the process, the Linux account, the credential, the binary — is + * shared between those two cases, so the only input a resumed turn has that a new one does not is + * `--resume`. Nothing reconsidered it, so every message retried the same failing id forever. + * + * `sawOutput` is what separates that from an ordinary crash. A child that produced not one message never + * got as far as loading the conversation; a child that died after speaking had a working transcript and a + * turn that broke later. Clearing the resume on the second would trade one bad turn for a lost + * conversation, which is the wrong direction. + */ +export type ExitDiagnosis = { + /** Forget the stored transcript id, so the next message starts a new session instead of failing again. */ + clearResume: boolean; + /** What the person sees. Carries the child's own words when it left any. */ + message: string; +}; + +export type ExitFacts = { + /** Whether the child produced a single message before dying. */ + sawOutput: boolean; + /** The transcript id passed as `--resume`, if this turn was continuing something. */ + resumeId?: string | undefined; + /** Whatever the child wrote to stderr. Often empty — a silent exit is the case that started all this. */ + stderrTail: string; +}; + +/** The last few lines of stderr, short enough to put in a sentence. Empty when the child said nothing. */ +export function stderrReason(stderrTail: string): string { + return stderrTail.trim().split('\n').slice(-3).join(' ').slice(-300).trim(); +} + +export function diagnoseExit(facts: ExitFacts): ExitDiagnosis { + const reason = stderrReason(facts.stderrTail); + const clearResume = !facts.sawOutput && Boolean(facts.resumeId); + + if (clearResume) { + return { + clearResume, + // Says what will happen next rather than what went wrong, because what happens next is the part the + // reader has to decide about — and "it will not remember" is the cost they are being asked to accept. + message: + `The agent could not reopen this conversation${reason ? `: ${reason}` : ''}. ` + + `Send again — it will start a fresh session, so it will not remember what was said above.`, + }; + } + + return { + clearResume, + message: + `The agent process exited unexpectedly${reason ? `: ${reason}` : ''}. ` + + `Your conversation is safe — send again to continue.`, + }; +}