From 876b39b301c9d9e1d17fe44dc0c9877e261ca3aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Fri, 7 Aug 2026 01:43:01 +0000 Subject: [PATCH] end a turn whose agent has gone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit restarting officer-agent takes every persistent session with it and nothing downstream notices: the browser's socket is healthy, officer's subscription is a bus filter, and there is simply never another event. the spinner ran forever and a refresh didn't help, because the transcript has no ending to read. keyed off the agent *registering*, not disconnecting — a disconnect fires on every `pm2 restart officer`, when the turn is fine. a registration socket dies with its process, so an agent appearing on it is a new one. covers the sitting tab; the reconnect path covers the rest, with the client now sending its belief that a turn is in flight and officer checking it against the agent over a new claude:is-generating. the check fails toward alive. Co-Authored-By: Claude Opus 5 --- docs/chat-ui-walkthrough.md | 42 ++++++++++++++ src/servers/api/chat/types.ts | 5 ++ src/servers/api/chat/websocket.ts | 57 ++++++++++++++++++- src/servers/sidecar-registry.ts | 45 +++++++++++++++ src/servers/sidecar/claude/claude-manager.ts | 12 ++++ src/servers/sidecar/claude/user-instance.ts | 4 ++ src/servers/sidecar/protocol.ts | 4 ++ .../officerdev/src/hooks/useChat.ts | 7 +++ 8 files changed, 173 insertions(+), 3 deletions(-) diff --git a/docs/chat-ui-walkthrough.md b/docs/chat-ui-walkthrough.md index d7ab0cb1..d4e52d1f 100644 --- a/docs/chat-ui-walkthrough.md +++ b/docs/chat-ui-walkthrough.md @@ -522,6 +522,48 @@ ends. --- +## 19. The turn that never ends + +**The symptom you described:** the last thing on screen is a tool call, the spinner runs forever, and +refreshing puts you back on the same dead conversation. It looked like a cancel you didn't press. + +**What actually happens.** It is the agent sidecar restarting. `officer-agent` holds the persistent +`query()` for each session, so when that process goes, so does every turn it was running — and nothing +downstream finds out. The browser's socket is to `officer`, which is fine; officer's subscription is an +event-bus filter, which is also fine; there is simply never another event. The `pm2` logs make the two +cases plain: `[sidecar] disconnected from API server` with no `SIGINT` beside it is officer restarting +underneath a healthy agent (turn survives — that's the documented design), while `[agent] SIGINT +received` is the case that kills turns. + +So the fix could not key off the sidecar _disconnecting_ — that fires on every `pm2 restart officer`, +when nothing is wrong. It keys off a **registration**: a registration socket lives and dies with its +process, so an agent appearing on it is an agent that has just started, and anything it was mid-turn on +is gone. + +Two paths, because the tab can be in two states: + +- **The tab is sitting there with a live socket.** The new agent registers, and every session officer + still believes is generating gets checked and ended. On a freshly-restarted _officer_ this loop is + empty — no sessions yet — which is right, because that case belongs to the other path. +- **The tab reconnects** (socket blip, or a refresh, or officer itself restarted). The client now sends + `generating` in its resume handshake — its belief that a turn is in flight. Officer can't confirm that + from its own memory, which died with the process, so it asks the agent over a new `claude:is-generating` + command. The agent is the only party that knows. + +Either way you get a line saying the agent restarted and the turn was cut off, and it's written to +`chat_session_events` — so a reload afterwards shows the same explanation rather than a conversation that +trails off mid-tool-call. + +The liveness check **fails toward alive**: a timeout, or no answer, is read as "still running". Telling +you a turn died while it is quietly typing would be a worse lie than a spinner that stays up a bit +longer. Only a registered agent answering "no", or no agent at all, counts as dead. OpenCode sessions are +left alone — that harness runs a turn per invocation and has no equivalent question. + +**Not verified:** the browser, and the restart itself. The mechanism is reasoned from the code plus the +`pm2` logs that pinned the cause; typecheck and the full 365-test suite are clean. + +--- + ## Things noticed and deliberately left alone - **`useChatWebSocket` silently ignores unparseable frames.** That one is intentional and the comment diff --git a/src/servers/api/chat/types.ts b/src/servers/api/chat/types.ts index 8f312db5..efeae242 100644 --- a/src/servers/api/chat/types.ts +++ b/src/servers/api/chat/types.ts @@ -87,6 +87,11 @@ export type ClientMessage = // client is the only party that still remembers them — officer's copy died with the process. model?: string; cwd?: string; + /** + * The client still shows a turn in flight. Officer cannot know this on its own after a restart, and + * it is the one claim worth checking against the agent — see `endTurnIfAgentIsGone`. + */ + generating?: boolean; }; // The `Task` tool's id, when this piece of output came from a subagent rather than the agent you are diff --git a/src/servers/api/chat/websocket.ts b/src/servers/api/chat/websocket.ts index cc67178b..a99d61da 100644 --- a/src/servers/api/chat/websocket.ts +++ b/src/servers/api/chat/websocket.ts @@ -8,7 +8,7 @@ import { ensureGeneralChatSessionsCwd } from './claude-sessions'; import * as sidecar from '@@/sidecar-registry'; import { join } from 'path'; import { getOwnerHomeDir, getEmailAccountsDir } from '../../../servers/data-path'; -import { getUserSettings, getEmailAccounts, getChatEventsSince } from 'officerdb'; +import { getUserSettings, getEmailAccounts, getChatEventsSince, appendChatEvent } from 'officerdb'; import { mkdirSync } from 'node:fs'; import { logger } from './logger'; @@ -579,11 +579,12 @@ function adoptOrphanedSession(ws: ServerWebSocket, sessionId: string, mo // task:notification. attachWs cancels the pending idle-GC. async function handleResumeCursor( ws: ServerWebSocket, - msg: { sessionId: string; cursor: number; model?: string; cwd?: string }, + msg: { sessionId: string; cursor: number; model?: string; cwd?: string; generating?: boolean }, ): Promise { const { sessionId, cursor } = msg; + const model = msg.model || DEFAULT_MODEL; if (!sessionManager.getSession(sessionId)) { - adoptOrphanedSession(ws, sessionId, msg.model || DEFAULT_MODEL, msg.cwd ?? ''); + adoptOrphanedSession(ws, sessionId, model, msg.cwd ?? ''); } sessionManager.attachWs(sessionId, ws); wsToSessionMap.set(ws as any, sessionId); @@ -595,8 +596,58 @@ async function handleResumeCursor( } catch (err) { logger.error('Failed to replay chat events on resume-cursor', { sessionId, error: String(err) }); } + if (msg.generating) await endTurnIfAgentIsGone(ws, sessionId, model); } +/** + * The client came back still believing a turn is running. Check whether it is, and if it isn't, say so. + * + * A restart of the agent sidecar takes its persistent sessions with it, and nothing downstream notices: + * the turn simply stops emitting. The browser's socket is fine, the conversation looks alive, and the + * spinner runs forever — a refresh doesn't help either, because there is no ending in the transcript to + * read. This is the one moment we can catch it, so the answer is written durably: a reload after this + * shows the same explanation rather than a conversation that trails off mid-tool-call. + * + * Only the claude harness is asked. OpenCode runs a turn per invocation and has no equivalent question, + * so its sessions are left alone rather than guessed at. + */ +async function endTurnIfAgentIsGone( + ws: ServerWebSocket | null, + sessionId: string, + model: string, +): Promise { + if (!isClaudeModel(model)) return; + if (await sidecar.isClaudeGenerating(sessionId)) return; + + const session = sessionManager.getSession(sessionId); + if (session) session.isGenerating = false; + + const event: ServerMessage = { + type: 'error', + message: 'The agent restarted while this turn was running, so it was cut off. The conversation is intact.', + }; + try { + const seq = await appendChatEvent(sessionId, event); + sendToClient(ws, event, seq); + } catch (err) { + // Still tell this client — an un-replayable explanation beats a spinner that never stops. + logger.error('Failed to persist cut-off notice', { sessionId, error: String(err) }); + sendToClient(ws, event); + } + logger.info('Ended a turn whose agent had gone', { sessionId }); +} + +// The other half of the same problem: the agent restarts while the browser sits there with a healthy +// socket, so nothing ever reconnects and nothing ever asks. A fresh agent process means every turn we +// still believe is running belongs to a process that no longer exists. On a fresh officer this loop is +// empty — it has no sessions yet — which is exactly right, because that case is the reconnect's to catch. +sidecar.onClaudeSidecarStarted(() => { + for (const session of sessionManager.getAllSessions()) { + if (!session.isGenerating) continue; + void endTurnIfAgentIsGone(session.ws as ServerWebSocket | null, session.sessionId, session.model); + } +}); + export const chatWebsocket = { open, message, diff --git a/src/servers/sidecar-registry.ts b/src/servers/sidecar-registry.ts index beef8d53..48c6a66a 100644 --- a/src/servers/sidecar-registry.ts +++ b/src/servers/sidecar-registry.ts @@ -65,9 +65,31 @@ export function registerSidecar(ws: ServerWebSocket, registration: SidecarR console.log( `[registry] registered sidecar "${registration.name}" (id=${id}, capabilities=[${registration.capabilities.join(', ')}])`, ); + + // A registration socket lives and dies with its process, so an agent showing up here is an agent that + // has just started — and whatever it was running before is gone. Announced rather than inferred from + // the *disconnect*, which is the wrong signal entirely: every `pm2 restart officer` drops these sockets + // while the sidecars, and their turns, carry on perfectly well. + if (registration.capabilities.includes('claude')) { + for (const handler of claudeRestartHandlers) { + try { + handler(); + } catch (err) { + console.error('[registry] claude-restart handler failed', err); + } + } + } return id; } +const claudeRestartHandlers = new Set<() => void>(); + +/** Notified when the agent sidecar registers — i.e. when a new agent process has come up. */ +export function onClaudeSidecarStarted(handler: () => void): () => void { + claudeRestartHandlers.add(handler); + return () => claudeRestartHandlers.delete(handler); +} + export function unregisterSidecar(id: string): void { const sc = sidecars.get(id); if (!sc) return; @@ -273,6 +295,29 @@ export function interruptClaude(sessionKey: string): void { sendFire('claude', { type: 'claude:interrupt', id: nextId(), sessionKey }); } +/** + * Is the agent still running a turn for this session? + * + * Asked of the sidecar because it is the only party that can answer: officer's own memory of a turn dies + * with `pm2 restart officer` while the turn itself carries on, so "we don't remember it" is not evidence + * of anything. Used on reconnect to tell those two apart — a turn that outlived a restart from one whose + * process is gone and is never going to produce another token. + * + * Fails *toward alive*: no answer means we don't know, and wrongly telling someone their turn died while + * it is still typing is worse than leaving a spinner up a little longer. Only a registered agent that + * says "no", or no agent at all, counts as dead. + */ +export async function isClaudeGenerating(sessionKey: string): Promise { + const sc = findSidecarByCapability('claude'); + if (!sc) return false; + try { + const res = await sendCommandToSidecar(sc, { type: 'claude:is-generating', id: nextId(), sessionKey }); + return res.type === 'claude:generating' ? res.generating : true; + } catch { + return true; + } +} + export function clearClaudeSession(sessionKey: string): void { sendFire('claude', { type: 'claude:clear-session', id: nextId(), sessionKey }); } diff --git a/src/servers/sidecar/claude/claude-manager.ts b/src/servers/sidecar/claude/claude-manager.ts index 980b75ef..3e5d412d 100644 --- a/src/servers/sidecar/claude/claude-manager.ts +++ b/src/servers/sidecar/claude/claude-manager.ts @@ -379,3 +379,15 @@ export function clearSession(sessionKey: string): void { export function getActiveSessionKeys(): string[] { return Array.from(sessions.keys()); } + +/** + * Is a turn actually in flight for this session, right now, in this process? + * + * This is the ground truth a reconnecting browser has no way to work out for itself. Officer's own view + * dies with `pm2 restart officer` while the turn keeps running here, so "officer doesn't remember" means + * nothing — and if *this* process was the one that restarted, the session is simply absent and the turn + * it was running is gone, however alive the client still believes it to be. + */ +export function isSessionGenerating(sessionKey: string): boolean { + return sessions.get(sessionKey)?.isGenerating ?? false; +} diff --git a/src/servers/sidecar/claude/user-instance.ts b/src/servers/sidecar/claude/user-instance.ts index dafc7ad2..bf01495a 100644 --- a/src/servers/sidecar/claude/user-instance.ts +++ b/src/servers/sidecar/claude/user-instance.ts @@ -190,6 +190,10 @@ async function handleCommand(cmd: SidecarCommand, reply: ReplyFn) { reply({ type: 'claude:interrupted', id: cmd.id }); break; + case 'claude:is-generating': + reply({ type: 'claude:generating', id: cmd.id, generating: claudeManager.isSessionGenerating(cmd.sessionKey) }); + break; + case 'claude:clear-session': claudeManager.clearSession(cmd.sessionKey); sessionLog.drop(cmd.sessionKey); diff --git a/src/servers/sidecar/protocol.ts b/src/servers/sidecar/protocol.ts index 3974fcdf..7ee07bdb 100644 --- a/src/servers/sidecar/protocol.ts +++ b/src/servers/sidecar/protocol.ts @@ -17,6 +17,9 @@ export type SidecarCommand = | { type: 'claude:kill'; id: string; sessionKey: string } | { type: 'claude:interrupt'; id: string; sessionKey: string } | { type: 'claude:clear-session'; id: string; sessionKey: string } + // Is a turn still running for this session? Only the process that owns the session can say, which is + // exactly why it is asked over the wire — see `isClaudeGenerating` in sidecar-registry. + | { type: 'claude:is-generating'; id: string; sessionKey: string } // OpenCode — drive a turn via `opencode run … --format json` (tools re-anchored to cwd via --dir) | { type: 'opencode:run-streaming'; id: string; params: OpenCodeRunParams } | { type: 'opencode:kill'; id: string; sessionKey: string } @@ -43,6 +46,7 @@ export type SidecarEvent = | { type: 'claude:killed'; id: string } | { type: 'claude:interrupted'; id: string } | { type: 'claude:session-cleared'; id: string } + | { type: 'claude:generating'; id: string; generating: boolean } // VNC | { type: 'vnc:started'; id: string; port: number; display: number } | { type: 'vnc:password'; id: string; password: string } diff --git a/src/workspaces/officerdev/src/hooks/useChat.ts b/src/workspaces/officerdev/src/hooks/useChat.ts index 706c3b31..76b449d1 100644 --- a/src/workspaces/officerdev/src/hooks/useChat.ts +++ b/src/workspaces/officerdev/src/hooks/useChat.ts @@ -93,6 +93,9 @@ export function useChat(initialSessionId?: string, initialModel?: string | null, const onTurnCompleteRef = useRef(onTurnComplete); onTurnCompleteRef.current = onTurnComplete; const resumeSummaryRef = useRef(initialResumeSummary); + // Mirror for the reconnect handshake, which runs from a stable callback and cannot read state. + const isGeneratingRef = useRef(false); + isGeneratingRef.current = isGenerating; const client = useClient(); @@ -393,6 +396,10 @@ export function useChat(initialSessionId?: string, initialModel?: string | null, cursor: cursorRef.current, ...(modelRef.current ? { model: modelRef.current } : {}), ...(cwdRef.current ? { cwd: cwdRef.current } : {}), + // Whether we still think a turn is running. If the agent restarted under us there is nothing left + // to produce output, and this is the only moment anyone can notice — the socket is healthy, the + // transcript just stops. The server checks the claim with the agent and answers if it's false. + generating: isGeneratingRef.current, }); }, []);