pastilhasandClaude Opus 5
be266da9e2
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>
135 lines
7.3 KiB
Markdown
135 lines
7.3 KiB
Markdown
# Chat session lifetime — findings and plan
|
|
|
|
Written 2026-08-09, before implementation. The investigation is done; the code is not.
|
|
|
|
**The trigger.** A laptop ran out of battery overnight while an agent on the home lab was running a
|
|
15-minute background loop (review new commits, restart the server). The session should have survived
|
|
until morning. Officer would have killed it about an hour after the browser socket dropped.
|
|
|
|
## What is actually there — verified, not remembered
|
|
|
|
**Two independent idle policies exist, in two processes, answering different questions.**
|
|
|
|
### The sidecar's — the real one
|
|
|
|
`src/servers/sidecar/claude/claude-manager.ts`
|
|
|
|
- `IDLE_TIMEOUT_MS = 30 * 60 * 1000` (`:39`) — thirty minutes with **no new turn**.
|
|
- `armIdle` (`:284-295`) is a **heartbeat, not a one-shot**: on expiry it re-checks and re-arms if
|
|
`isGenerating || pendingTasks.size > 0`, so it can never collect a session that is still working.
|
|
- `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`
|
|
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
|
|
running — a long silent `run_in_background` job would otherwise be killed along with its pending
|
|
`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.
|
|
|
|
There is also `stallTimer` for a turn that claims to be generating but has emitted nothing, and
|
|
`SEND_TIMEOUT_MS = 30 * 60 * 1000` (`:10`).
|
|
|
|
### Officer's — the blunt one
|
|
|
|
`src/servers/api/chat/websocket.ts`, `src/servers/api/chat/session-manager.ts`
|
|
|
|
- `IDLE_TIMEOUT_MS = 60 * 60 * 1000` (`websocket.ts:48`).
|
|
- 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.
|
|
- 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.
|
|
|
|
**Three defects follow:**
|
|
|
|
1. It kills sessions the sidecar has deliberately protected. Officer has no view of `pendingTasks`.
|
|
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
|
|
sidecar keeps and officer kills.
|
|
|
|
### What is already fine
|
|
|
|
`pm2 restart officer` **does not kill running sessions.** Sidecars are PM2 peers, not children; the
|
|
agent keeps generating and keeps committing to `chat_session_events`. Only officer's in-memory binding
|
|
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 — **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
|
|
already reasons about it correctly.
|
|
|
|
**Rejected alternative:** teaching officer about `pendingTasks` over the protocol. That re-implements
|
|
the sidecar's heartbeat in a second place, which is how these two drifted apart to begin with.
|
|
|
|
**The blocker.** `src/servers/channels/send-claude-code.ts:65-69` welds them together:
|
|
|
|
```ts
|
|
return {
|
|
kill: () => {
|
|
sidecar.killClaude(params.sessionKey);
|
|
unsub();
|
|
},
|
|
};
|
|
```
|
|
|
|
`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.
|
|
|
|
Work:
|
|
|
|
1. `send-claude-code.ts` — return `{ kill, detach }`; `kill` stays as-is, `detach` is `unsub` alone.
|
|
2. Same for the OpenCode path (`websocket.ts:452` assigns `handle.kill` there too).
|
|
3. `websocket.ts:361` / `:452` — also assign `session._sidecarUnsub = handle.detach`.
|
|
4. `session-manager.ts` — add `releaseSession(sessionId)`: clear the idle timer, call `_sidecarUnsub`,
|
|
drop from `sessions`/`userSessions`. **Do not** call `_claudeKill`.
|
|
5. `setIdleTimeout`'s callback → `releaseSession`, not `deleteSession`.
|
|
6. Leave `deleteSession` alone — explicit disconnect (`handleDisconnect`) must still kill.
|
|
|
|
Check while doing it: `adoptOrphanedSession` notes that `handleChat` treats an absent `_claudeKill` as
|
|
"first turn of this session" and would open a **second** subscription, delivering every message twice.
|
|
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
|
|
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
|
|
officer can answer "what is running right now" and surface it.
|
|
|
|
**Do not try to persist a running session.** A live session is a running `query()` with an open stream;
|
|
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
|
|
survivable; `officer-agent` restarts are not.
|
|
|
|
## Deliberately not decided
|
|
|
|
Keeping an **idle** session alive — nothing generating, no background work — is a separate decision and
|
|
the only part that would need the user to mark anything. Steps 1 and 2 cover sessions with work in
|
|
flight, which is the case that actually bit. A 15-minute loop re-arms the sidecar's 30-minute timer on
|
|
every turn, so it never idles out on its own.
|