stop the harness before deleting its transcript, and wait for it to actually stop

Deleting a commander removed its transcript and the `claude` CLI wrote it
straight back. Verified rather than inferred: the file returned with 22 lines,
every one timestamped after the delete, and the deleted commander was sitting in
/chat as "(untitled)" — no longer in `command_hq_sessions`, so nothing filtered
it out. Delete a commander, get an untitled chat back.

Two causes, and the first one hid the second.

The kill never happened. `killClaudeSession` was imported straight from
`claude-manager` into a route that runs in `officer`, while the session map lives
in `officer-claude-code`. It read an empty map in the wrong process and returned
false every time. `claude:kill` and `killClaude` already existed for this; the
route just wasn't using them.

And killing is not being dead. `abort()` returns while the CLI is still
unwinding, and officer does not own that file — the CLI does. So a session now
carries an `exited` promise resolved by the same exit hook that already reaps it,
`claude:kill` replies only once that resolves, and both delete routes await the
reply before touching the filesystem.

Bounded at 5s and never fatal. A child that will not die delays a delete by
seconds instead of hanging it, and a sidecar that is down leaves the caller with
exactly the race it already had.

`markExited` fires before `dropDeadSession`'s identity guard, deliberately: a
waiter has to be released even when a newer session has taken its key, or it
blocks for the full timeout on a process that died on schedule.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
2026-08-22 22:55:29 +00:00
co-authored by Claude Opus 5
parent 8ac3ca7fe5
commit eefcb06e35
5 changed files with 103 additions and 5 deletions
+10 -1
View File
@@ -417,6 +417,16 @@ chatRouter.delete('/sessions/:id', async (ctx) => {
return ctx.json({ ok: true }); 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, // `.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. // so `if (removed)` would report a delete that found nothing as a success.
const removed = deleteClaudeSession(who, cwd, id); 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 `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. 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] : [] }); await forgetChatSessions({ transcriptIds: removed, sessionKeys: liveKey ? [liveKey] : [] });
return ctx.json({ ok: true }); return ctx.json({ ok: true });
}); });
+13 -2
View File
@@ -6,7 +6,7 @@ import * as errors from '@@/custom-errors';
import { resolveHomeDir } from '@@/user-home'; import { resolveHomeDir } from '@@/user-home';
import { getCommandersDir, toShellUsername } from '@@/data-path'; import { getCommandersDir, toShellUsername } from '@@/data-path';
import { forgetChatSessions, getChatEventsSince, ingestSpawnedChildren } from 'officerdb'; 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 { findSessionKeyByClaudeSession } from '@@/sidecar/claude/state';
import { sessionKeyForTranscript } from '../chat/websocket'; import { sessionKeyForTranscript } from '../chat/websocket';
import { 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 // 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. // 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 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); const stopped = stopCommanderProcesses(who.email, sessionId);
// 2. Everything it spawned. Best-effort per child: one transcript that has already been cleaned up must // 2. Everything it spawned. Best-effort per child: one transcript that has already been cleaned up must
+20
View File
@@ -330,6 +330,26 @@ export function killClaude(sessionKey: string, userId: number): void {
sendFire('claude', { type: 'claude:kill', id: nextId(), sessionKey, userId }); 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<boolean> {
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). // Interrupt the current turn but keep the persistent session warm (the "stop" button).
export function interruptClaude(sessionKey: string, userId: number): void { export function interruptClaude(sessionKey: string, userId: number): void {
sendFire('claude', { type: 'claude:interrupt', id: nextId(), sessionKey, userId }); sendFire('claude', { type: 'claude:interrupt', id: nextId(), sessionKey, userId });
@@ -247,6 +247,18 @@ type PersistentSession = {
interrupted: boolean; interrupted: boolean;
/** What we passed as `--resume`, so a failure can name it and decide whether it was to blame. */ /** What we passed as `--resume`, so a failure can name it and decide whether it was to blame. */
resumeId?: string; 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<void>;
/** 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`. * 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. * transcript by id.
*/ */
function dropDeadSession(session: PersistentSession, detail: string): void { 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 // 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. // its predecessor's process exited would break the live conversation instead of a dead one.
if (sessions.get(session.sessionKey) !== session) return; if (sessions.get(session.sessionKey) !== session) return;
@@ -562,7 +578,13 @@ function createSession(
interrupted: false, interrupted: false,
stderrTail: '', stderrTail: '',
sawOutput: false, sawOutput: false,
// Assigned immediately below — the resolver has to escape the executor before the object is usable.
exited: undefined as unknown as Promise<void>,
markExited: () => {},
}; };
session.exited = new Promise<void>((resolve) => {
session.markExited = resolve;
});
// Strip the nested-session guard vars so the SDK can spawn `claude` (mirrors the old spawn env clean). // Strip the nested-session guard vars so the SDK can spawn `claude` (mirrors the old spawn env clean).
const cleanEnv = envWithoutParentSession(); 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. */ /** 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<boolean> {
const session = ownedSession(sessionKey, userId);
if (!session) return false;
const { exited } = session;
killClaudeSession(sessionKey, userId);
let timer: ReturnType<typeof setTimeout> | undefined;
const timedOut = await Promise.race([
exited.then(() => false),
new Promise<boolean>((resolve) => {
timer = setTimeout(() => resolve(true), timeoutMs);
}),
]);
if (timer) clearTimeout(timer);
return !timedOut;
}
export function killClaudeSession(sessionKey: string, userId: number): boolean { export function killClaudeSession(sessionKey: string, userId: number): boolean {
const session = ownedSession(sessionKey, userId); const session = ownedSession(sessionKey, userId);
if (!session) return false; if (!session) return false;
+11 -2
View File
@@ -294,11 +294,20 @@ async function handleCommand(cmd: SidecarCommand, reply: ReplyFn) {
break; break;
} }
case 'claude:kill': case 'claude:kill': {
claudeManager.killClaudeSession(cmd.sessionKey, cmd.userId); /*
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); sessionLog.drop(cmd.sessionKey);
reply({ type: 'claude:killed', id: cmd.id }); reply({ type: 'claude:killed', id: cmd.id });
break; break;
}
case 'claude:interrupt': case 'claude:interrupt':
await claudeManager.interruptClaudeSession(cmd.sessionKey, cmd.userId); await claudeManager.interruptClaudeSession(cmd.sessionKey, cmd.userId);