diff --git a/docs/chat-session-lifetime.md b/docs/chat-session-lifetime.md index 9b14eed7..577d4501 100644 --- a/docs/chat-session-lifetime.md +++ b/docs/chat-session-lifetime.md @@ -20,9 +20,9 @@ until morning. Officer would have killed it about an hour after the browser sock - `pendingTasks: Set` (`:174`) holds background tasks started but not yet notified. `task:started` adds the id and **clears the idle timer outright** (`:406-411`); `task:notification` removes it and re-arms only if nothing else is outstanding (`:413-414`). -- Its own comment states the intent: *"never GC a session that's mid-turn or still has background tasks +- Its own comment states the intent: _"never GC a session that's mid-turn or still has background tasks running — a long silent `run_in_background` job would otherwise be killed along with its pending - `task_notification`."* + `task_notification`."_ So **the sidecar already detects long-running background work with no declaration from the user.** The "mark a session permanent" feature is not needed for this case. @@ -38,7 +38,7 @@ There is also `stallTimer` for a turn that claims to be generating but has emitt - Armed by the socket **closing**, not by it being unresponsive: `close(ws)` → `detachWs` + `setIdleTimeout` (`websocket.ts:151-163`). Unresponsiveness matters only upstream — Bun closes a WS idle for 60s and there is a per-connection heartbeat, so a dead laptop loses the socket about a minute - in, and *that* starts the hour. + in, and _that_ starts the hour. - Expiry runs `deleteSession` (`session-manager.ts:74-102`), which calls `_sidecarUnsub()` then `_claudeKill()` → `sidecar.killClaude(sessionId)`. - `attachWs` clears the timer (`session-manager.ts:104-116`), so reconnecting cancels it cleanly. @@ -49,7 +49,7 @@ There is also `stallTimer` for a turn that claims to be generating but has emitt 2. It does not survive `pm2 restart officer` — a `setTimeout` on an in-memory record, in the process designed to bounce. If the browser never returns, nothing re-arms it. 3. On a flat battery the sidecar's 30 minutes normally fires first anyway, so officer's hour is mostly - redundant *except* in the one case where it does damage — a session with background work, which the + redundant _except_ in the one case where it does damage — a session with background work, which the sidecar keeps and officer kills. ### What is already fine @@ -59,7 +59,7 @@ agent keeps generating and keeps committing to `chat_session_events`. Only offic dies, and `adoptOrphanedSession` (`websocket.ts:559`) rebuilds it — including the session-scoped subscription, without which a reconnected client replays and then goes silent for the rest of the turn. -## Step 1 — officer's timer releases instead of kills +## Step 1 — officer's timer releases instead of kills — **DONE** Officer's idle timer is doing two unrelated jobs: garbage-collecting its own binding (its business) and terminating the agent (the sidecar's). Split them and leave the agent's lifetime to the process that @@ -71,10 +71,15 @@ the sidecar's heartbeat in a second place, which is how these two drifted apart **The blocker.** `src/servers/channels/send-claude-code.ts:65-69` welds them together: ```ts -return { kill: () => { sidecar.killClaude(params.sessionKey); unsub(); } } +return { + kill: () => { + sidecar.killClaude(params.sessionKey); + unsub(); + }, +}; ``` -`unsub` is a closure reachable only *through* `kill`, so officer cannot let go without killing. +`unsub` is a closure reachable only _through_ `kill`, so officer cannot let go without killing. **The seam already exists.** `_sidecarUnsub` is declared (`types.ts:310`) and called separately in `deleteSession` (`session-manager.ts:83-84`) — and **nothing ever assigns it**. Populating it is the fix. @@ -96,9 +101,19 @@ Make sure a released-then-readopted session cannot land in that state. Consequence to accept: after an hour a returning browser goes through the adopt path rather than finding a live record. That path already runs on every officer restart. +**As built.** `detach` added beside `kill` on both streaming handles; `_sidecarUnsub` populated at all +three sites (Claude first turn, OpenCode, and `adoptOrphanedSession` — that last one was not in the +original list and would have leaked a listener per adopt-then-idle); `forget` factored out of +`deleteSession`; `releaseSession` added; the idle timer points at it. `deleteSession` still kills, so an +explicit disconnect is unchanged. + +The double-subscription trap does not arise: `releaseSession` unsubscribes and drops the record, so a +returning browser either adopts (fresh single subscription) or starts a first turn with no stale +listener behind it. `unsub` is a `Set.delete`, so `deleteSession` calling it twice is harmless. + ## Step 2 — enumerate live sessions -After an officer restart a live session is invisible until a browser reconnects to it *by id*; adoption +After an officer restart a live session is invisible until a browser reconnects to it _by id_; adoption is on-demand only. **There is no list verb in the sidecar protocol** (checked). Add `claude:list` returning each live `sessionKey` with `isGenerating` and `pendingTasks.size`, so @@ -108,7 +123,7 @@ officer can answer "what is running right now" and surface it. it cannot be serialised. The durable part — the output — is already in `chat_session_events`, which is what makes replay work. -**Hard limit:** if the *agent sidecar* restarts, a mid-turn is lost regardless. Officer restarts are +**Hard limit:** if the _agent sidecar_ restarts, a mid-turn is lost regardless. Officer restarts are survivable; `officer-agent` restarts are not. ## Deliberately not decided diff --git a/src/servers/api/chat/session-manager.ts b/src/servers/api/chat/session-manager.ts index 27552dec..af4d4531 100644 --- a/src/servers/api/chat/session-manager.ts +++ b/src/servers/api/chat/session-manager.ts @@ -1,5 +1,5 @@ -import type { UserSession } from "./types"; -import { logger } from "./logger"; +import type { UserSession } from './types'; +import { logger } from './logger'; class SessionManager { private sessions = new Map(); @@ -26,13 +26,13 @@ class SessionManager { ws: null, lastActivity: Date.now(), idleTimer: null, - streamBuffer: "", + streamBuffer: '', isGenerating: false, systemContextSent: false, messages: [], meta: { id: sessionId, - title: "", + title: '', model, cwd, groupSlug: groupSlug || null, @@ -66,19 +66,36 @@ class SessionManager { getUserSessions(email: string): UserSession[] { const sessionIds = this.userSessions.get(email) || []; - return sessionIds - .map((id) => this.sessions.get(id)) - .filter((s): s is UserSession => s !== undefined); + return sessionIds.map((id) => this.sessions.get(id)).filter((s): s is UserSession => s !== undefined); } - deleteSession(sessionId: string): void { - const session = this.sessions.get(sessionId); - if (!session) return; - + /** Drop this process's record of a session. Shared by `deleteSession` and `releaseSession`. */ + private forget(session: UserSession): void { if (session.idleTimer) { clearTimeout(session.idleTimer); } + this.sessions.delete(session.sessionId); + + const userSessionIds = this.userSessions.get(session.email); + if (userSessionIds) { + const filtered = userSessionIds.filter((id) => id !== session.sessionId); + if (filtered.length > 0) { + this.userSessions.set(session.email, filtered); + } else { + this.userSessions.delete(session.email); + } + } + } + + /** + * End the session for good: kill the agent upstream, drop the subscription, forget the record. This is + * what an explicit "disconnect" means — the user said stop. + */ + deleteSession(sessionId: string): void { + const session = this.sessions.get(sessionId); + if (!session) return; + // Clean up sidecar subscriptions if (session._sidecarUnsub) { session._sidecarUnsub(); @@ -87,19 +104,35 @@ class SessionManager { session._claudeKill(); } - this.sessions.delete(sessionId); + this.forget(session); + } - const userSessionIds = this.userSessions.get(session.email); - if (userSessionIds) { - const filtered = userSessionIds.filter( - (id) => id !== sessionId - ); - if (filtered.length > 0) { - this.userSessions.set(session.email, filtered); - } else { - this.userSessions.delete(session.email); - } + /** + * Let go WITHOUT killing: unsubscribe, forget the record, leave the agent running. + * + * This is what the idle GC should always have done. Officer's timer was doing two unrelated jobs — + * collecting its own in-memory binding, which is its business, and terminating the agent, which is the + * sidecar's. The sidecar already refuses to collect a session that is mid-turn or holding background + * tasks (`claude-manager.ts` → `pendingTasks`, and an `armIdle` that re-checks rather than firing + * once); officer knew none of that and killed anyway. A `run_in_background` job outliving the browser + * — a laptop that ran out of battery — died an hour later for no reason. + * + * Nothing is stranded by forgetting the record. The sidecar keeps committing to `chat_session_events`, + * and a returning browser goes through `adoptOrphanedSession`, which rebuilds the record and a fresh + * subscription. That is the same path every `pm2 restart officer` already takes. + */ + releaseSession(sessionId: string): void { + const session = this.sessions.get(sessionId); + if (!session) return; + + // Only the listener goes. `_claudeKill` is deliberately NOT called — and must not be left behind + // either: the record is being dropped, so the next turn starts from `adoptOrphanedSession`, which + // installs its own kill and subscription. + if (session._sidecarUnsub) { + session._sidecarUnsub(); } + + this.forget(session); } attachWs(sessionId: string, ws: any): void { @@ -132,8 +165,11 @@ class SessionManager { } session.idleTimer = setTimeout(() => { - logger.info('Session idle timeout reached, cleaning up', { sessionId, timeoutMs }); - this.deleteSession(sessionId); + logger.info('Session idle timeout reached, releasing binding (agent left running)', { + sessionId, + timeoutMs, + }); + this.releaseSession(sessionId); }, timeoutMs); } diff --git a/src/servers/api/chat/websocket.ts b/src/servers/api/chat/websocket.ts index 583c30ed..4bdca57a 100644 --- a/src/servers/api/chat/websocket.ts +++ b/src/servers/api/chat/websocket.ts @@ -344,7 +344,9 @@ async function handleClaudeCodeChat( try { if (!session._claudeKill) { // First turn of this session: open the persistent session + a SESSION-scoped event subscription - // (survives turn-end so background task:notifications keep flowing). handle.kill tears both down. + // (survives turn-end so background task:notifications keep flowing). `kill` tears both down for an + // explicit disconnect; `detach` drops only the listener, which is what the idle GC uses so an + // absent browser stops taking a live agent with it. const handle = await sendClaudeCodeStreaming({ userId, email, @@ -359,6 +361,7 @@ async function handleClaudeCodeChat( }); session.piProcess = sessionId as any; session._claudeKill = handle.kill; + session._sidecarUnsub = handle.detach; } else { // Session already live: push this turn onto the existing persistent session (no new subscription). await sidecar.spawnClaudeStreaming({ @@ -450,6 +453,7 @@ async function handleOpenCodeChat( // Store the abort handle so handleStop can end the turn (OpenCode is aborted via this handle). session.piProcess = sessionId as any; session._claudeKill = handle.kill; + session._sidecarUnsub = handle.detach; } catch (err) { logger.error('Failed to start OpenCode streaming', { sessionId, error: String(err) }); sendToClient(ws, { type: 'error', message: 'Failed to start OpenCode' }); @@ -583,6 +587,10 @@ function adoptOrphanedSession(ws: ServerWebSocket, sessionId: string, mo else sidecar.killOpenCode(sessionId); unsub(); }; + // An adopted session can idle out and be released like any other, and releasing detaches through this + // field alone. Leaving it unset would drop the record while the listener stayed subscribed — a leak + // that grows by one every time a browser adopts a session and then goes away. + session._sidecarUnsub = unsub; logger.info('Adopted orphaned chat session after restart', { sessionId, model }); return session; diff --git a/src/servers/channels/send-claude-code.ts b/src/servers/channels/send-claude-code.ts index 3b2c5296..a6793c9a 100644 --- a/src/servers/channels/send-claude-code.ts +++ b/src/servers/channels/send-claude-code.ts @@ -45,7 +45,18 @@ type ClaudeCodeStreamingParams = { }; type ClaudeCodeStreamingHandle = { + /** End the agent's session upstream and stop listening. For an explicit disconnect. */ kill: () => void; + /** + * Stop listening and leave the agent running. + * + * These are separate because officer's idle GC and a user's "disconnect" want different things, and + * for a long time they could not have them: `unsub` was a closure reachable only through `kill`, so + * letting go of a session necessarily killed it. That is why an idle browser took a live agent down + * with it — including one the sidecar had deliberately protected because background work was still + * in flight. + */ + detach: () => void; }; export async function sendClaudeCodeStreaming(params: ClaudeCodeStreamingParams): Promise { @@ -67,5 +78,6 @@ export async function sendClaudeCodeStreaming(params: ClaudeCodeStreamingParams) sidecar.killClaude(params.sessionKey); unsub(); }, + detach: unsub, }; } diff --git a/src/servers/channels/send-opencode.ts b/src/servers/channels/send-opencode.ts index 52f1ece9..4be73c23 100644 --- a/src/servers/channels/send-opencode.ts +++ b/src/servers/channels/send-opencode.ts @@ -25,7 +25,10 @@ type OpenCodeStreamingParams = { }; type OpenCodeStreamingHandle = { + /** End the agent's session upstream and stop listening. For an explicit disconnect. */ kill: () => void; + /** Stop listening and leave the agent running — see the same pair in `send-claude-code.ts`. */ + detach: () => void; }; export async function sendOpenCodeStreaming(params: OpenCodeStreamingParams): Promise { @@ -67,5 +70,6 @@ export async function sendOpenCodeStreaming(params: OpenCodeStreamingParams): Pr sidecar.killOpenCode(params.sessionKey); unsub(); }, + detach: unsub, }; }