diff --git a/src/servers/api/chat/chat.ts b/src/servers/api/chat/chat.ts index 18bc972f..3b78ea61 100644 --- a/src/servers/api/chat/chat.ts +++ b/src/servers/api/chat/chat.ts @@ -417,6 +417,16 @@ chatRouter.delete('/sessions/:id', async (ctx) => { return ctx.json({ ok: true }); } + /* + Stop the harness BEFORE removing its file, and wait for it. + + The `claude` CLI owns the transcript. Deleting it while a live session is still writing removes a file + that is written straight back, so the conversation reappears untitled on the next refresh — see + `killClaudeAndWait`. No live session means nothing to wait for, which is the ordinary case here. + */ + const liveKey = sessionKeyForTranscript(id); + if (liveKey) await sidecar.killClaudeAndWait(liveKey, ctx.get('user').id); + // `.length`, not truthiness: this returns the IDS it removed and an empty array is truthy in JavaScript, // so `if (removed)` would report a delete that found nothing as a success. const removed = deleteClaudeSession(who, cwd, id); @@ -431,7 +441,6 @@ chatRouter.delete('/sessions/:id', async (ctx) => { `chat_session_events` is keyed on officer's session key. Undefined here is the normal case for an old conversation — no live session holds it, so nothing is keyed on a key to remove. */ - const liveKey = sessionKeyForTranscript(id); await forgetChatSessions({ transcriptIds: removed, sessionKeys: liveKey ? [liveKey] : [] }); return ctx.json({ ok: true }); }); diff --git a/src/servers/api/command-hq/router.ts b/src/servers/api/command-hq/router.ts index 1091c4a8..0f5622b4 100644 --- a/src/servers/api/command-hq/router.ts +++ b/src/servers/api/command-hq/router.ts @@ -6,7 +6,7 @@ import * as errors from '@@/custom-errors'; import { resolveHomeDir } from '@@/user-home'; import { getCommandersDir, toShellUsername } from '@@/data-path'; import { forgetChatSessions, getChatEventsSince, ingestSpawnedChildren } from 'officerdb'; -import { killClaudeSession } from '@@/sidecar/claude/claude-manager'; +import * as sidecar from '@@/sidecar-registry'; import { findSessionKeyByClaudeSession } from '@@/sidecar/claude/state'; import { sessionKeyForTranscript } from '../chat/websocket'; import { @@ -560,7 +560,18 @@ commandHqRouter.delete('/commanders/:sessionId', async (ctx) => { // Same mapping problem as the briefing: the sidecar's map is not visible from here, so the platform's own // is asked first and the sidecar's is kept as a fallback for a session that predates this process. const sessionKey = sessionKeyForTranscript(sessionId) ?? findSessionKeyByClaudeSession(sessionId, user.id); - const sessionKilled = sessionKey ? killClaudeSession(sessionKey, user.id) : false; + /* + Across the socket, and AWAITED — both halves matter. + + `killClaudeSession` from `claude-manager` was called directly here, and it could only ever return false: + the session map lives in the `officer-claude-code` process and this route runs in `officer`, so it was + reading an empty map in the wrong process. The kill silently never happened. + + Awaited because the next thing this route does is delete the transcript, and the `claude` CLI owns that + file. Deleting it while the CLI is still unwinding removes a file that is written straight back — which + is exactly how a deleted commander came back as an untitled ordinary chat. + */ + const sessionKilled = sessionKey ? await sidecar.killClaudeAndWait(sessionKey, user.id) : false; const stopped = stopCommanderProcesses(who.email, sessionId); // 2. Everything it spawned. Best-effort per child: one transcript that has already been cleaned up must diff --git a/src/servers/sidecar-registry.ts b/src/servers/sidecar-registry.ts index 6b8e0491..05833590 100644 --- a/src/servers/sidecar-registry.ts +++ b/src/servers/sidecar-registry.ts @@ -330,6 +330,26 @@ export function killClaude(sessionKey: string, userId: number): void { sendFire('claude', { type: 'claude:kill', id: nextId(), sessionKey, userId }); } +/** + * Kill a session and wait for the agent to confirm its child process is gone. + * + * For callers that then delete the session's FILES. The transcript belongs to the `claude` CLI, not to + * officer, so removing it while the CLI is still unwinding deletes a file that is written straight back — + * which resurrects a conversation the user deleted, as an untitled ordinary chat that nothing filters out. + * + * Never fatal: a sidecar that is down or slow must not block a delete. The caller proceeds and accepts the + * race it was already running, which is no worse than before this existed. + */ +export async function killClaudeAndWait(sessionKey: string, userId: number): Promise { + try { + const sc = await waitForHandler('claude'); + const res = await sendCommandToSidecar(sc, { type: 'claude:kill', id: nextId(), sessionKey, userId }); + return res.type === 'claude:killed'; + } catch { + return false; + } +} + // Interrupt the current turn but keep the persistent session warm (the "stop" button). export function interruptClaude(sessionKey: string, userId: number): void { sendFire('claude', { type: 'claude:interrupt', id: nextId(), sessionKey, userId }); diff --git a/src/servers/sidecar/claude/claude-manager.ts b/src/servers/sidecar/claude/claude-manager.ts index 5b1824c8..a7158c8e 100644 --- a/src/servers/sidecar/claude/claude-manager.ts +++ b/src/servers/sidecar/claude/claude-manager.ts @@ -247,6 +247,18 @@ type PersistentSession = { interrupted: boolean; /** What we passed as `--resume`, so a failure can name it and decide whether it was to blame. */ resumeId?: string; + /** + * Resolves when this session's child process is actually gone. + * + * Killing is not the same as being dead, and the difference is visible on disk: `abort()` returns at once + * while the `claude` CLI keeps writing for as long as it takes to unwind. Officer does not own the + * transcript — the CLI does — so a delete that races this window removes the file and the CLI writes it + * straight back, resurrecting a conversation the user deleted. Anything that deletes a session's FILES + * has to await this first. + */ + exited: Promise; + /** Called by the exit hook. Resolving twice is a no-op, which is what makes the double-fire safe. */ + markExited: () => void; /** * The model this session is currently answering as, `harness/tier`. * @@ -362,6 +374,10 @@ function spawnClaudeAsOwner({ command, args, cwd, env, signal }: SpawnOptions): * transcript by id. */ function dropDeadSession(session: PersistentSession, detail: string): void { + // Before the identity guard, and deliberately: this fires on the real process exit, and a waiter must be + // released even when a newer session has already taken this key. Otherwise a replaced session's waiter + // blocks until its timeout for a process that died on schedule. + session.markExited(); // 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; @@ -562,7 +578,13 @@ function createSession( interrupted: false, stderrTail: '', sawOutput: false, + // Assigned immediately below — the resolver has to escape the executor before the object is usable. + exited: undefined as unknown as Promise, + markExited: () => {}, }; + session.exited = new Promise((resolve) => { + session.markExited = resolve; + }); // Strip the nested-session guard vars so the SDK can spawn `claude` (mirrors the old spawn env clean). const cleanEnv = envWithoutParentSession(); @@ -1236,6 +1258,33 @@ export async function interruptClaudeSession(sessionKey: string, userId: number) } /** Fully tear the session down (the "disconnect" action / idle GC): abort the query + close input. */ +/** + * Kill a session and wait for its child process to actually be gone. + * + * The bounded wait is the point. `killClaudeSession` returns as soon as it has aborted, which is before the + * `claude` CLI has finished writing — and the CLI, not officer, owns the transcript file. A caller that + * deletes files has to await this or it deletes them out from under a process still writing them. + * + * The timeout resolves rather than throws: a child that will not die within the window is a worse problem + * than a stale transcript, and blocking a user's delete on it forever helps nobody. The caller gets `false` + * and can say so. + */ +export async function killClaudeSessionAndWait(sessionKey: string, userId: number, timeoutMs = 5000): Promise { + const session = ownedSession(sessionKey, userId); + if (!session) return false; + const { exited } = session; + killClaudeSession(sessionKey, userId); + let timer: ReturnType | undefined; + const timedOut = await Promise.race([ + exited.then(() => false), + new Promise((resolve) => { + timer = setTimeout(() => resolve(true), timeoutMs); + }), + ]); + if (timer) clearTimeout(timer); + return !timedOut; +} + export function killClaudeSession(sessionKey: string, userId: number): boolean { const session = ownedSession(sessionKey, userId); if (!session) return false; diff --git a/src/servers/sidecar/claude/user-instance.ts b/src/servers/sidecar/claude/user-instance.ts index 96881020..16458cea 100644 --- a/src/servers/sidecar/claude/user-instance.ts +++ b/src/servers/sidecar/claude/user-instance.ts @@ -294,11 +294,20 @@ async function handleCommand(cmd: SidecarCommand, reply: ReplyFn) { break; } - case 'claude:kill': - claudeManager.killClaudeSession(cmd.sessionKey, cmd.userId); + case 'claude:kill': { + /* + Reply only once the child is actually gone. + + The caller deleting a session's transcript has to know the CLI has stopped writing it, and this + reply is the only signal it can get — officer's side of the socket has no view of the process. + Bounded inside `killClaudeSessionAndWait`, so a child that will not die delays this by seconds + rather than hanging the caller's request. + */ + await claudeManager.killClaudeSessionAndWait(cmd.sessionKey, cmd.userId); sessionLog.drop(cmd.sessionKey); reply({ type: 'claude:killed', id: cmd.id }); break; + } case 'claude:interrupt': await claudeManager.interruptClaudeSession(cmd.sessionKey, cmd.userId);