let an idle browser go without taking the agent with it
Officer's hour-long idle 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. It could not do the first without the second, because `unsub` was a closure reachable only through `kill`. So a browser that went away killed a live agent an hour later — including one the sidecar had deliberately protected. The sidecar already refuses to collect a session that is mid-turn or holding background tasks: `task:started` disarms its idle GC, and `armIdle` re-checks and re-arms rather than firing once. Officer had no view of any of that. A laptop running out of battery overnight took a `run_in_background` job with it for no reason. `detach` now sits beside `kill` on both streaming handles, and `_sidecarUnsub` — declared and called for a long time, never once assigned — is populated at all three sites. `releaseSession` unsubscribes and forgets the record without killing; the idle timer points at it. `deleteSession` is unchanged, so an explicit disconnect still ends the session. The third assignment site was not in the plan: `adoptOrphanedSession` sets `_claudeKill` but nothing else, so an adopted session that later idled out would have dropped its record while the listener stayed subscribed — a leak of one per adopt-then-leave. No double subscription: releasing unsubscribes first, so a returning browser either adopts with a fresh listener or starts a first turn with none behind it. Step 1 of docs/chat-session-lifetime.md. Step 2 (a list verb, so running sessions can be found after a restart) is still open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -20,9 +20,9 @@ until morning. Officer would have killed it about an hour after the browser sock
|
|||||||
- `pendingTasks: Set<string>` (`:174`) holds background tasks started but not yet notified.
|
- `pendingTasks: Set<string>` (`:174`) holds background tasks started but not yet notified.
|
||||||
`task:started` adds the id and **clears the idle timer outright** (`:406-411`); `task:notification`
|
`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`).
|
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
|
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
|
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.
|
"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` +
|
- 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
|
`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
|
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
|
- Expiry runs `deleteSession` (`session-manager.ts:74-102`), which calls `_sidecarUnsub()` then
|
||||||
`_claudeKill()` → `sidecar.killClaude(sessionId)`.
|
`_claudeKill()` → `sidecar.killClaude(sessionId)`.
|
||||||
- `attachWs` clears the timer (`session-manager.ts:104-116`), so reconnecting cancels it cleanly.
|
- `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
|
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.
|
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
|
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.
|
sidecar keeps and officer kills.
|
||||||
|
|
||||||
### What is already fine
|
### 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
|
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.
|
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
|
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
|
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:
|
**The blocker.** `src/servers/channels/send-claude-code.ts:65-69` welds them together:
|
||||||
|
|
||||||
```ts
|
```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
|
**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.
|
`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
|
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.
|
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
|
## 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).
|
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
|
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
|
it cannot be serialised. The durable part — the output — is already in `chat_session_events`, which is
|
||||||
what makes replay work.
|
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.
|
survivable; `officer-agent` restarts are not.
|
||||||
|
|
||||||
## Deliberately not decided
|
## Deliberately not decided
|
||||||
|
|||||||
@@ -1,5 +1,5 @@
|
|||||||
import type { UserSession } from "./types";
|
import type { UserSession } from './types';
|
||||||
import { logger } from "./logger";
|
import { logger } from './logger';
|
||||||
|
|
||||||
class SessionManager {
|
class SessionManager {
|
||||||
private sessions = new Map<string, UserSession>();
|
private sessions = new Map<string, UserSession>();
|
||||||
@@ -26,13 +26,13 @@ class SessionManager {
|
|||||||
ws: null,
|
ws: null,
|
||||||
lastActivity: Date.now(),
|
lastActivity: Date.now(),
|
||||||
idleTimer: null,
|
idleTimer: null,
|
||||||
streamBuffer: "",
|
streamBuffer: '',
|
||||||
isGenerating: false,
|
isGenerating: false,
|
||||||
systemContextSent: false,
|
systemContextSent: false,
|
||||||
messages: [],
|
messages: [],
|
||||||
meta: {
|
meta: {
|
||||||
id: sessionId,
|
id: sessionId,
|
||||||
title: "",
|
title: '',
|
||||||
model,
|
model,
|
||||||
cwd,
|
cwd,
|
||||||
groupSlug: groupSlug || null,
|
groupSlug: groupSlug || null,
|
||||||
@@ -66,19 +66,36 @@ class SessionManager {
|
|||||||
|
|
||||||
getUserSessions(email: string): UserSession[] {
|
getUserSessions(email: string): UserSession[] {
|
||||||
const sessionIds = this.userSessions.get(email) || [];
|
const sessionIds = this.userSessions.get(email) || [];
|
||||||
return sessionIds
|
return sessionIds.map((id) => this.sessions.get(id)).filter((s): s is UserSession => s !== undefined);
|
||||||
.map((id) => this.sessions.get(id))
|
|
||||||
.filter((s): s is UserSession => s !== undefined);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
deleteSession(sessionId: string): void {
|
/** Drop this process's record of a session. Shared by `deleteSession` and `releaseSession`. */
|
||||||
const session = this.sessions.get(sessionId);
|
private forget(session: UserSession): void {
|
||||||
if (!session) return;
|
|
||||||
|
|
||||||
if (session.idleTimer) {
|
if (session.idleTimer) {
|
||||||
clearTimeout(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
|
// Clean up sidecar subscriptions
|
||||||
if (session._sidecarUnsub) {
|
if (session._sidecarUnsub) {
|
||||||
session._sidecarUnsub();
|
session._sidecarUnsub();
|
||||||
@@ -87,19 +104,35 @@ class SessionManager {
|
|||||||
session._claudeKill();
|
session._claudeKill();
|
||||||
}
|
}
|
||||||
|
|
||||||
this.sessions.delete(sessionId);
|
this.forget(session);
|
||||||
|
}
|
||||||
|
|
||||||
const userSessionIds = this.userSessions.get(session.email);
|
/**
|
||||||
if (userSessionIds) {
|
* Let go WITHOUT killing: unsubscribe, forget the record, leave the agent running.
|
||||||
const filtered = userSessionIds.filter(
|
*
|
||||||
(id) => id !== sessionId
|
* 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
|
||||||
if (filtered.length > 0) {
|
* sidecar's. The sidecar already refuses to collect a session that is mid-turn or holding background
|
||||||
this.userSessions.set(session.email, filtered);
|
* tasks (`claude-manager.ts` → `pendingTasks`, and an `armIdle` that re-checks rather than firing
|
||||||
} else {
|
* once); officer knew none of that and killed anyway. A `run_in_background` job outliving the browser
|
||||||
this.userSessions.delete(session.email);
|
* — 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 {
|
attachWs(sessionId: string, ws: any): void {
|
||||||
@@ -132,8 +165,11 @@ class SessionManager {
|
|||||||
}
|
}
|
||||||
|
|
||||||
session.idleTimer = setTimeout(() => {
|
session.idleTimer = setTimeout(() => {
|
||||||
logger.info('Session idle timeout reached, cleaning up', { sessionId, timeoutMs });
|
logger.info('Session idle timeout reached, releasing binding (agent left running)', {
|
||||||
this.deleteSession(sessionId);
|
sessionId,
|
||||||
|
timeoutMs,
|
||||||
|
});
|
||||||
|
this.releaseSession(sessionId);
|
||||||
}, timeoutMs);
|
}, timeoutMs);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -344,7 +344,9 @@ async function handleClaudeCodeChat(
|
|||||||
try {
|
try {
|
||||||
if (!session._claudeKill) {
|
if (!session._claudeKill) {
|
||||||
// First turn of this session: open the persistent session + a SESSION-scoped event subscription
|
// 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({
|
const handle = await sendClaudeCodeStreaming({
|
||||||
userId,
|
userId,
|
||||||
email,
|
email,
|
||||||
@@ -359,6 +361,7 @@ async function handleClaudeCodeChat(
|
|||||||
});
|
});
|
||||||
session.piProcess = sessionId as any;
|
session.piProcess = sessionId as any;
|
||||||
session._claudeKill = handle.kill;
|
session._claudeKill = handle.kill;
|
||||||
|
session._sidecarUnsub = handle.detach;
|
||||||
} else {
|
} else {
|
||||||
// Session already live: push this turn onto the existing persistent session (no new subscription).
|
// Session already live: push this turn onto the existing persistent session (no new subscription).
|
||||||
await sidecar.spawnClaudeStreaming({
|
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).
|
// Store the abort handle so handleStop can end the turn (OpenCode is aborted via this handle).
|
||||||
session.piProcess = sessionId as any;
|
session.piProcess = sessionId as any;
|
||||||
session._claudeKill = handle.kill;
|
session._claudeKill = handle.kill;
|
||||||
|
session._sidecarUnsub = handle.detach;
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
logger.error('Failed to start OpenCode streaming', { sessionId, error: String(err) });
|
logger.error('Failed to start OpenCode streaming', { sessionId, error: String(err) });
|
||||||
sendToClient(ws, { type: 'error', message: 'Failed to start OpenCode' });
|
sendToClient(ws, { type: 'error', message: 'Failed to start OpenCode' });
|
||||||
@@ -583,6 +587,10 @@ function adoptOrphanedSession(ws: ServerWebSocket<WSData>, sessionId: string, mo
|
|||||||
else sidecar.killOpenCode(sessionId);
|
else sidecar.killOpenCode(sessionId);
|
||||||
unsub();
|
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 });
|
logger.info('Adopted orphaned chat session after restart', { sessionId, model });
|
||||||
return session;
|
return session;
|
||||||
|
|||||||
@@ -45,7 +45,18 @@ type ClaudeCodeStreamingParams = {
|
|||||||
};
|
};
|
||||||
|
|
||||||
type ClaudeCodeStreamingHandle = {
|
type ClaudeCodeStreamingHandle = {
|
||||||
|
/** End the agent's session upstream and stop listening. For an explicit disconnect. */
|
||||||
kill: () => void;
|
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<ClaudeCodeStreamingHandle> {
|
export async function sendClaudeCodeStreaming(params: ClaudeCodeStreamingParams): Promise<ClaudeCodeStreamingHandle> {
|
||||||
@@ -67,5 +78,6 @@ export async function sendClaudeCodeStreaming(params: ClaudeCodeStreamingParams)
|
|||||||
sidecar.killClaude(params.sessionKey);
|
sidecar.killClaude(params.sessionKey);
|
||||||
unsub();
|
unsub();
|
||||||
},
|
},
|
||||||
|
detach: unsub,
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -25,7 +25,10 @@ type OpenCodeStreamingParams = {
|
|||||||
};
|
};
|
||||||
|
|
||||||
type OpenCodeStreamingHandle = {
|
type OpenCodeStreamingHandle = {
|
||||||
|
/** End the agent's session upstream and stop listening. For an explicit disconnect. */
|
||||||
kill: () => void;
|
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<OpenCodeStreamingHandle> {
|
export async function sendOpenCodeStreaming(params: OpenCodeStreamingParams): Promise<OpenCodeStreamingHandle> {
|
||||||
@@ -67,5 +70,6 @@ export async function sendOpenCodeStreaming(params: OpenCodeStreamingParams): Pr
|
|||||||
sidecar.killOpenCode(params.sessionKey);
|
sidecar.killOpenCode(params.sessionKey);
|
||||||
unsub();
|
unsub();
|
||||||
},
|
},
|
||||||
|
detach: unsub,
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user