Close sessions to free them, and auto-close idle ones (SPEC-29) - #157
Merged
Conversation
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_5f2a0f9c-9131-42f1-a93b-56bcb7208ac3) |
Contributor
📝 WalkthroughWalkthroughThe PR replaces archived sessions with closed, reopenable sessions across the protocol, server, persistence layer, adapters, Flutter client, documentation, and tests. It also adds graceful process shutdown and configurable idle-session auto-close. ChangesClosed session lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant WebSocket
participant Manager
participant Adapter
participant Storage
Client->>WebSocket: session.close
WebSocket->>Manager: closeSession(id)
Manager->>Adapter: close()
Manager->>Adapter: kill()
Manager->>Storage: persist closed=true
Storage-->>Manager: closed session
Manager-->>WebSocket: acknowledgement
WebSocket-->>Client: close result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
leduckhc
force-pushed
the
feat/close-session
branch
from
August 10, 2026 20:24
7749511 to
47fcbfa
Compare
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_d2a269b3-4645-4220-9ee1-5d7f2ae104a5) |
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/manager.ts (1)
1235-1252: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd the
closedearly return that the idempotency claim implies.The doc comment states
reopenSessionis idempotent, but the body runs unconditionally. On a session that is already open, it callsliveWorktreePaths(agit worktree listshell-out) and can callsession.detachToRoot(), which clears the worktree binding of a live session. A client can reach this through thesession.reopencommand with any session id.♻️ Proposed guard
async reopenSession(id: string): Promise<void> { const session = this.sessions.get(id); if (!session) throw new Error(`no such session: ${id}`); + if (!session.closed) return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/manager.ts` around lines 1235 - 1252, Update reopenSession to return immediately when the session is already open, before any liveWorktreePaths lookup or detachToRoot call. Preserve the existing orphaned-worktree restoration and setClosed(false) behavior only for closed sessions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/desktop/chat/closed_sidebar_view.dart`:
- Around line 43-45: Initialize `_lastActiveIds` in `initState` from the current
`sessionsProvider` active-session IDs before registering the live-sync listener,
matching the baseline setup in `closed_screen.dart`; preserve the listener’s
existing change-detection behavior for subsequent close/reopen updates.
In `@app/lib/store/fake_server.dart`:
- Around line 782-787: Implement explicit session.close and session.reopen
handling in the fake server’s session message dispatch, tracking closed state in
the fake session model. On close, remove the session from active _sessions and
add it to the closed-session state; on reopen, restore it to _sessions and
remove it from that state, updating any repository snapshots consistently.
Ensure _closedSessions() reflects the tracked state so session.listClosed
returns the current closed sessions rather than only static fixtures.
In `@app/lib/store/store.dart`:
- Around line 864-870: Update subscribeSession() so it remains replay-only and
does not attach resumable sessions with SessionStatus.exited, including sessions
reopened by reopenSession(). Move live-agent restoration to the input handling
path, reusing or extending the server-side ensureLiveForInput flow so opening a
reopened session starts the agent only when input is sent.
In `@app/test/ui/home/closed_screen_test.dart`:
- Around line 33-45: Update the fake store methods listClosedSessions and
reopenSession so reopened session IDs are excluded from subsequent list results:
record the ID in reopenSession and filter those IDs from closed in
listClosedSessions. Extend the reopen test assertion around find.text('sess-a')
to verify the row is absent after the screen reloads, confirming the refreshed
list is rendered.
In `@docs/specs/2026-07-26-SPEC-29-session-lifecycle-resume-list-delete.md`:
- Around line 26-48: Replace the superseded frozen Decision 6 with the amended
lifecycle contract: define session.close as invoking AgentAdapter.close()
followed by guaranteed AgentAdapter.kill() process reaping, while retaining the
session record, transcript, and resume handle for reopening. Rename the
persisted archived state to closed and update the related APIs and terminology
to session.reopen and session.listClosed, removing detached-only shutdown and
archive-based requirements.
In `@server/src/adapters/child_transport.ts`:
- Around line 290-309: The SIGKILL escalation in the child termination flow must
only be scheduled when child.kill("SIGTERM") returns true. In
server/src/adapters/child_transport.ts lines 290-309, store the SIGTERM result
and return without creating killTimer when it is false; in
server/src/adapters/child_transport.test.ts lines 35-39, make the
successful-kill test double return true and add coverage verifying a false
SIGTERM result does not schedule or invoke SIGKILL.
In `@server/src/adapters/stub.ts`:
- Around line 212-220: Update SessionManager.closeSession to mark the session as
closing before awaiting adapter.close(), and clear or reject any queued input
during that state. Modify Session.bindAdapter/flushNext so idle events cannot
deliver queued messages while closing, while preserving normal flushing
otherwise. Add a regression test covering a busy stub session with one queued
message, asserting that closing it does not trigger a second send().
In `@server/src/adapters/subprocess-adapter.ts`:
- Around line 55-61: Update the doc comment for SubprocessAdapter.close so it no
longer claims the method must never throw; document that close may reject or
hang and that SessionManager.closeSession bounds and handles the outcome,
consistent with the AgentAdapter.close contract. Preserve the existing no-op
default and subclass behavior description.
In `@server/src/idle_reaper.test.ts`:
- Around line 128-165: Add tests covering IdleReaper’s implicit sweep interval
and overlapping sweep protection. Verify the default interval is idleCloseMs
divided by four but never below the 30-second MIN_SWEEP_MS floor, and verify a
concurrent sweep returns an empty result without duplicating the session close
while the first sweep is pending. Use the existing IdleReaper, session, and
timer injection patterns in the neighboring tests.
In `@server/src/manager.ts`:
- Around line 1203-1228: The closeSession teardown must be serialized with
concurrent input: record a closing state or promise before the first await, and
update ensureLiveForInput to await that teardown before reopening or reattaching
the session. Ensure send.message cannot use the live adapter while closeSession
is closing it, and add a regression test covering this interleaving.
In `@server/src/storage/sqlite_event_store.test.ts`:
- Around line 239-245: Update the saveSession round-trip test to close the
SqliteEventStore in a finally block, ensuring cleanup occurs whether assertions
pass or fail. Keep the existing setup and assertions unchanged, and use the
store instance created by new SqliteEventStore().
---
Outside diff comments:
In `@server/src/manager.ts`:
- Around line 1235-1252: Update reopenSession to return immediately when the
session is already open, before any liveWorktreePaths lookup or detachToRoot
call. Preserve the existing orphaned-worktree restoration and setClosed(false)
behavior only for closed sessions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d63ed9c1-5604-4e28-9165-395d3d222281
📒 Files selected for processing (73)
app/lib/app/router.dartapp/lib/app/routes.dartapp/lib/desktop/chat/closed_sidebar_view.dartapp/lib/desktop/chat/desktop_sidebar.dartapp/lib/desktop/chat/panes/pane_header.dartapp/lib/desktop/chat/panes/workspace_controller.dartapp/lib/desktop/chat/selected_session.dartapp/lib/desktop/chat/split_view.dartapp/lib/desktop/desktop_app.dartapp/lib/store/fake_server.dartapp/lib/store/models.dartapp/lib/store/store.dartapp/lib/transport/codec.dartapp/lib/ui/home/closed_screen.dartapp/lib/ui/home/home_screen.dartapp/lib/ui/home/session_tile.dartapp/lib/ui/ports/ports_filter.dartapp/lib/ui/ports/ports_screen.dartapp/lib/ui/session/chat_transcript.dartapp/lib/ui/session/session_screen.dartapp/lib/ui/session/timing_labels.dartapp/lib/ui/widgets/pr_signals.dartapp/lib/ui/widgets/wrap_up.dartapp/test/app/router_ports_test.dartapp/test/desktop/chat/closed_sidebar_view_test.dartapp/test/desktop/chat/github_budget_button_test.dartapp/test/desktop/chat/groups/group_bar_test.dartapp/test/desktop/chat/groups/groups_controller_test.dartapp/test/desktop/chat/pr_bar_test.dartapp/test/desktop/chat/selected_session_test.dartapp/test/desktop/chat/split_tree_view_test.dartapp/test/desktop/chat/split_view_test.dartapp/test/desktop/desktop_chat_pane_menu_test.dartapp/test/desktop/desktop_chat_pane_test.dartapp/test/desktop/desktop_session_prune_test.dartapp/test/desktop/desktop_sidebar_test.dartapp/test/desktop/keymap_scope_test.dartapp/test/fake_server_demo_data_test.dartapp/test/session_resumable_test.dartapp/test/tool_summary_test.dartapp/test/ui/home/closed_screen_sync_test.dartapp/test/ui/home/closed_screen_test.dartapp/test/ui/home/home_screen_ports_test.dartapp/test/ui/home/session_tile_test.dartapp/test/ui/session/session_actions_menu_test.dartapp/test/ui/widgets/pr_signals_test.dartdocs/DEVELOPMENT.mddocs/MOBILE-PARITY.mddocs/specs/2026-07-26-SPEC-29-session-lifecycle-resume-list-delete.mdserver/src/adapters/acp.test.tsserver/src/adapters/acp.tsserver/src/adapters/adapter.tsserver/src/adapters/child_transport.test.tsserver/src/adapters/child_transport.tsserver/src/adapters/codex.test.tsserver/src/adapters/codex.tsserver/src/adapters/deadline.tsserver/src/adapters/detached.tsserver/src/adapters/stub.tsserver/src/adapters/subprocess-adapter.tsserver/src/idle_reaper.test.tsserver/src/idle_reaper.tsserver/src/manager.test.tsserver/src/manager.tsserver/src/protocol.tsserver/src/repo_service.tsserver/src/server.tsserver/src/session.tsserver/src/storage/event_store.tsserver/src/storage/sqlite_event_store.test.tsserver/src/storage/sqlite_event_store.tsserver/src/ws/commands/session.tsserver/test/ws/send_message_attachments.test.ts
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_27e005da-719a-4fd5-becf-a032cc0ef304) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_40b74fa1-8751-49a0-9878-bcad385874ac) |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/test/fake_server_demo_data_test.dart`:
- Around line 92-93: Update the comment near the listener setup in the test to
accurately state that FakeServer.start() schedules _pushInitialState() after 150
ms rather than emitting synchronously, and that subscribing before start()
ensures the listener receives this later initial snapshot.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 11420812-6e47-42e5-a31f-048da4593ea6
📒 Files selected for processing (2)
app/test/desktop/chat/closed_sidebar_sync_test.dartapp/test/fake_server_demo_data_test.dart
Owner
Author
|
All CodeRabbit findings have been addressed in these commits:
Verified: server tests (1299/1299), app analysis clean, e2e on real app/agent confirmed close→process death, idle reaper active, transparent resume working. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_654ad6fb-80e3-42ee-8edc-31ff40eea99e) |
leduckhc
added a commit
that referenced
this pull request
Aug 10, 2026
…red SIGTERM CodeRabbit review on PR #157, server-side findings. Serialize input with teardown (#10). `closeSession` left `closed === false` with the live adapter installed while awaiting `adapter.close()` and `kill()`, so a concurrent `send.message` could pass `ensureLiveForInput` and prompt an agent mid-reap — the turn lost or errored. Teardown is now published as a per-session promise: concurrent closes collapse onto one, and `ensureLiveForInput` awaits it before reopening. Drop queued input on close (#7). An adapter's `close()` cancels its turn and reports `idle`, which is exactly the signal `flushNext()` waits for — so a queued mid-turn message was prompted into the process about to be reaped. Closing now clears the queue first, matching `cancel`'s "stop means stop" (SPEC-35). Escalate only a delivered SIGTERM (#6). `ChildProcess.kill()` returns false when the signal was not delivered (the child is already gone); scheduling the SIGKILL anyway could fire it at a pid the OS had since recycled, hitting an unrelated process. The test double now reports delivery like Node does, and covers the false path. Correct the close() contract in subprocess-adapter's doc (#8): it said "must never throw", contradicting `AgentAdapter.close`, which states the opposite now that the manager owns bounding and swallowing. Cover the reaper's untested paths (#9): the default sweep interval and its 30s floor (which stops a short window busy-looping), and the overlapping-sweep guard. Both races are red-then-green: each new test reproduces the interleaving before the fix. 1304 server tests pass.
leduckhc
added a commit
that referenced
this pull request
Aug 10, 2026
…ision 6 CodeRabbit review on PR #157. Both reopen tests asserted only that the request went out: their fakes returned the same closed list forever, so the tests passed even if the row never left the view — which is precisely the user-visible failure worth guarding. Each fake now behaves like the server and drops a reopened session from `session.listClosed`, and both tests assert the row is gone (mobile ClosedScreen and the desktop closed sidebar). SPEC-29's Decision 6 still specified `session.archive`, the `archived` flag and a bare `adapter.kill()` while the amendment at the top of the spec described the close/reopen contract — one spec, two contradictory lifecycles. Decision 6 is now marked superseded and restates the shipped contract: graceful agent-side release then a verified reap, queued input dropped, teardown serialized against input, `closed` persisted, `session.listClosed`/`session.reopen`, transparent reopen on a message, `sub` never respawning, and the idle-sweep guards that keep every auto-close reversible.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_f3de0b76-af3e-43fd-9a01-2968461daf1a) |
leduckhc
added a commit
that referenced
this pull request
Aug 10, 2026
start() schedules _pushInitialState() on a 150ms timer rather than emitting synchronously. The reason to subscribe before start() stands — a listener attached afterwards can miss that emission — but the stated mechanism was wrong. Review follow-up on PR #157.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_8334f2d1-7ab6-4f2f-bc53-18272f515d63) |
`dispose()` sent one SIGTERM, never verified it, and never escalated. An agent that ignored or outlived it stayed resident forever, holding its whole RSS — measured on a dev host as 19 agent processes / ~0.95 GB, the oldest five days old, several of them children of makit daemons whose sessions were long gone. Escalate to SIGKILL after a grace period (default 3 s, `killGraceMs`), cancel the escalation when the child settles, and skip signalling entirely once it has — a queued SIGKILL against a recycled pid would hit an unrelated process. Scheduled at most once, so repeated dispose() calls can't stack timers. This is the half that actually returns memory: releasing a session agent-side frees nothing while its process is still alive.
Archiving was a soft hide that stopped the agent; it never asked the back end to release the session, and nothing ever reclaimed a session left idle. Since makit runs ONE agent process per session, idle agents simply accumulated. Replace archive with a real close: session.archive -> session.close session.unarchive -> session.reopen session.listArchived -> session.listClosed flag `archived` -> `closed` Closing is two steps, both required. First the back end's own release primitive (ACP `session/close`, gated on the new `sessionCapabilities.close`; codex `thread/unsubscribe`), which cancels the in-flight turn and frees agent-side state while leaving the session listable and resumable. Then the process is reaped. Step one alone reclaims no memory; step two alone is a SIGTERM to an agent mid-write. Neither can block the other — a wedged agent must never be able to keep its RSS. Idle sessions are now swept automatically (`MAKIT_IDLE_CLOSE_MIN`, default 60, `0` disables). A stale timestamp alone is never enough: sessions that are mid-turn, awaiting input or approval, still drafts, already cold, or lacking a native resume handle are all held — auto-close must always be reversible. Recovery is invisible: `send.message` routes through `ensureLiveForInput`, which reopens and resumes a closed session, while `sub` deliberately does not, so reading a closed transcript never respawns an agent. The session record, transcript and resume handle are kept throughout, so reopen resumes exactly as before. `capabilities.archive` stays (codex has a real `thread/archive`) but is now clearly a different concept from close. BREAKING CHANGE: the three `session.*` commands above are renamed and the DTO field `archived` becomes `closed`; server and app must ship together. Existing databases migrate via `ALTER TABLE sessions RENAME COLUMN archived TO closed` — adding a new column instead would have silently returned every already-closed session to the active list and respawned its agent on the next subscribe.
Code review caught four misses. All of them survived because the sweeps that drove the rename were case-sensitive, so a lowercase `grep archiv` never matched `listArchived`, `'ARCHIVED'` or `"An closed"`. - The desktop sidebar's closed-view header still read ARCHIVED on screen (with two tests happily asserting it). Now CLOSED. - `SessionDTO.orphaned` still pointed readers at `session.listArchived`. - The `session.close` handler comment still described hiding a session rather than releasing its agent. - `Session.closed` was documented as "a soft, recoverable hide", which is what archive did; close releases the agent and reclaims its process. Reworded, and the "An closed" grammar fixed. Also drops a redundant `as bool?` + `?? false` on `sidebarClosedProvider`: it is a `StateProvider<bool>` with a `false` default, so both were dead. Re-audited case-insensitively; the only remaining "archive" mentions are the back end's genuinely distinct native archive capability (codex `thread/archive`) and the SQLite migration that must still name the old column.
…e reaping Two high-severity bugs from review of the close/auto-close work, both in the path that is supposed to reclaim memory. 1. The idle sweep evaluated its safety guards once, when building the candidate list, then closed each session behind an `await`. Every close waits for an agent round-trip plus up to the SIGTERM grace period, so across several sessions the loop runs for tens of seconds — ample time for a message to arrive and start a turn on a session further down the list, which was then closed anyway on the strength of a stale check. That tears the agent out from under a live turn: exactly the data loss the guards exist to prevent. The predicate is now `isIdleClosable()` and is re-evaluated against a fresh clock immediately before each close, with a skip logged. 2. `closeSession()` awaited `adapter.close()` with no deadline, and the ACP adapter's `session/close` had none either (codex's `thread/unsubscribe` was already bounded). A wedged agent is precisely the one that needs killing, and it never answers — so teardown stranded before `kill()`, holding the RSS this whole branch exists to free, and left `sweeping` true for the lifetime of the daemon, silently disabling idle auto-close entirely. Bounded in both places, defence in depth: the ACP adapter now deadlines its own `session/close` (mirroring codex), and the manager independently bounds `adapter.close()` via `closeGraceMs` (default 10s) because the "must not hang" half of the adapter contract cannot be left to implementations. Either way the reap still runs. `withDeadline` moves out of codex.ts into `adapters/deadline.ts` alongside a new `bestEffort()` for teardown steps that are courtesies with a hard fallback. Regression tests cover a message landing mid-sweep, a close() that never settles, and a subsequent sweep still working after one hangs. The hang cases previously hung the test process rather than failing.
…er's Nested deadlines only work if the inner one is strictly tighter, and mine were inverted: the ACP adapter's `session/close` defaulted to ACP_HANDSHAKE_TIMEOUT (15s) while the manager's `bestEffort` backstop was 10s. So the adapter's own deadline could never fire on the normal closeSession path — the manager always abandoned the call first, then `kill()` disposed the transport out from under a timer still pending for another 5s. Defence in depth that only looked layered. `session/close` now has its own constant, ACP_CLOSE_TIMEOUT (8s), instead of borrowing the handshake timeout it has nothing to do with, and the manager's backstop is named DEFAULT_CLOSE_GRACE_MS. A test asserts inner < outer so a future tweak to either constant cannot silently invert the ordering again. Also makes closeSession's log ids consistent: the kill-failure warning used the full session id while every other line in the file (and the close warning right above it) uses the 8-char prefix, so no single grep matched both. Found by open-code-review on the previous two commits.
Quality audit of my own feature. Four structural problems, all self-inflicted. 1. The idle sweeper was inlined into SessionManager: +204 lines onto an already-1472-line file, plus 4 constructor options (idleCloseMs, idleSweepMs, setTimer, clearTimer), 6 private fields and its own re-entrancy flag. It is a periodic policy collaborator, which this codebase already has a shape for — MetricsCollector and the ports service both own their window, clock and injected timer and are wired in server.ts. Now `idle_reaper.ts` does the same, reaching the session world through a two-function seam. manager.ts drops back to +59 over base, ManagerOpts loses 4 options, and serve.ts returns to untouched (the env parsing lives with the module that owns the policy). The tests got the bigger win: the reaper reads a handful of session fields, so they now use plain object doubles instead of standing up a SessionManager, SQLite store and temp dir per case. Each case states exactly the session shape it is about and nothing else can affect the outcome. 2. Two nested close deadlines with one caller between them. The adapter bounded its own `session/close` AND the manager bounded `adapter.close()` — which is how the previous commit shipped them inverted, and why it needed an invariant test to hold them in order. Defence in depth against nobody: `closeSession` is the only caller. Deleted the adapter-level deadline, the constant, the option, the invariant test, and `bestEffort()` (a wrapper with one call site once the redundancy was gone). Adapters now issue the plain request and MAY reject or hang; the manager bounds and swallows it in one place, for every back end, and reaps either way. The contract is stated on `AgentAdapter.close` instead of half-implemented in three adapters. 3. `deadline.ts` was created as the canonical home for bounded agent waits and then acp.ts's three pre-existing ad-hoc timeouts were left in place — four ways to do one thing. Converged onto `withDeadline`, deleting the local `withTimeout` closure and two raw `Promise.race` blocks; acp.ts shrinks despite gaining the close path. 4. A triple-negative status check (`!== "running" && !== "awaiting-input" && !== "awaiting-approval"`) was the only place asking "is this agent mid-flight?" — a missing model, not a formatting problem. Now `isBusy(status)` next to the SessionStatus type it interrogates, and `Session.cold` replaces two `instanceof DetachedAdapter` checks. Also caught by the type checker mid-refactor: I had added a second `allSessions()` returning Iterable when a canonical one returning Session[] already existed, which broke its existing callers. Removed; the reaper reuses the canonical accessor. No behaviour change. 1297 server tests green, and the stub e2e still proves close -> not active -> in the Closed list -> a message transparently reopens.
Formatter output only: collapses a few single-argument calls now that the surrounding lines changed. No behaviour or logic change.
…red SIGTERM CodeRabbit review on PR #157, server-side findings. Serialize input with teardown (#10). `closeSession` left `closed === false` with the live adapter installed while awaiting `adapter.close()` and `kill()`, so a concurrent `send.message` could pass `ensureLiveForInput` and prompt an agent mid-reap — the turn lost or errored. Teardown is now published as a per-session promise: concurrent closes collapse onto one, and `ensureLiveForInput` awaits it before reopening. Drop queued input on close (#7). An adapter's `close()` cancels its turn and reports `idle`, which is exactly the signal `flushNext()` waits for — so a queued mid-turn message was prompted into the process about to be reaped. Closing now clears the queue first, matching `cancel`'s "stop means stop" (SPEC-35). Escalate only a delivered SIGTERM (#6). `ChildProcess.kill()` returns false when the signal was not delivered (the child is already gone); scheduling the SIGKILL anyway could fire it at a pid the OS had since recycled, hitting an unrelated process. The test double now reports delivery like Node does, and covers the false path. Correct the close() contract in subprocess-adapter's doc (#8): it said "must never throw", contradicting `AgentAdapter.close`, which states the opposite now that the manager owns bounding and swallowing. Cover the reaper's untested paths (#9): the default sweep interval and its 30s floor (which stops a short window busy-looping), and the overlapping-sweep guard. Both races are red-then-green: each new test reproduces the interleaving before the fix. 1304 server tests pass.
…ision 6 CodeRabbit review on PR #157. Both reopen tests asserted only that the request went out: their fakes returned the same closed list forever, so the tests passed even if the row never left the view — which is precisely the user-visible failure worth guarding. Each fake now behaves like the server and drops a reopened session from `session.listClosed`, and both tests assert the row is gone (mobile ClosedScreen and the desktop closed sidebar). SPEC-29's Decision 6 still specified `session.archive`, the `archived` flag and a bare `adapter.kill()` while the amendment at the top of the spec described the close/reopen contract — one spec, two contradictory lifecycles. Decision 6 is now marked superseded and restates the shipped contract: graceful agent-side release then a verified reap, queued input dropped, teardown serialized against input, `closed` persisted, `session.listClosed`/`session.reopen`, transparent reopen on a message, `sub` never respawning, and the idle-sweep guards that keep every auto-close reversible.
start() schedules _pushInitialState() on a 150ms timer rather than emitting synchronously. The reason to subscribe before start() stands — a listener attached afterwards can miss that emission — but the stated mechanism was wrong. Review follow-up on PR #157.
leduckhc
force-pushed
the
feat/close-session
branch
from
August 10, 2026 22:03
2907915 to
8c12222
Compare
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_d1042a28-91e8-4ab6-aedf-db6706f7be90) |
Owner
Author
|
@coderabbitai review |
Contributor
|
Owner
Author
|
@coderabbitai help |
Contributor
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Owner
Author
|
@coderabbitai rate limit |
Contributor
|
Your plan includes PR reviews subject to rate limits. More reviews will be available in 13 minutes. |
Owner
Author
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/manager.ts (1)
1262-1278: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWait for an in-flight close before reopening.
A
session.reopenrequest can run whilecloseSession()awaitsadapter.close(). At that point,session.closedis stillfalse.reopenSession()then returns, andrunClose()later setsclosedtotrue. The reopen request is lost.Wait for
closeInFlight.get(id)before reading the session state. Add a regression test with a gated close, a concurrent reopen, and a final assertion thatclosed === false.Proposed fix
async reopenSession(id: string): Promise<void> { + const closing = this.closeInFlight.get(id); + if (closing) await closing.catch(() => {}); const session = this.sessions.get(id); if (!session) throw new Error(`no such session: ${id}`);As per coding guidelines: “Use test-driven development: write a failing test before production logic, then make it pass and refactor.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/manager.ts` around lines 1262 - 1278, Update reopenSession to await any promise from closeInFlight.get(id) before reading or mutating the session state, then retain the existing orphaned-worktree handling and reopening logic. Add a regression test using a gated close and concurrent reopen, asserting the final session.closed value is false; write the test before the production change.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/store/fake_server.dart`:
- Around line 783-820: The fake server uses separate immutable fixtures for
closed sessions and leaves runtime-closed sessions in active repository
snapshots. Move seeded closed sessions into the mutable _sessions model so
session.reopen updates and re-emits them, update _pushRepos() to exclude
sessions where closed is true, and extend the regression test to reopen a seeded
fixture and verify that closing a runtime session removes it from both active
session and repository snapshots.
In `@server/src/adapters/child_transport.ts`:
- Around line 300-316: Update the termination state in dispose() so firing the
escalation timer records that SIGKILL has already been sent independently of
clearing killTimer; subsequent dispose() calls before the child emits exit must
not send SIGTERM or schedule another escalation. Add a regression test covering
dispose() after the first SIGKILL and before exit, asserting exactly one
SIGKILL.
---
Outside diff comments:
In `@server/src/manager.ts`:
- Around line 1262-1278: Update reopenSession to await any promise from
closeInFlight.get(id) before reading or mutating the session state, then retain
the existing orphaned-worktree handling and reopening logic. Add a regression
test using a gated close and concurrent reopen, asserting the final
session.closed value is false; write the test before the production change.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ddb469f6-e9c5-466d-a4df-1b64750238e0
📒 Files selected for processing (16)
app/lib/desktop/chat/closed_sidebar_view.dartapp/lib/store/fake_server.dartapp/lib/ui/session/chat_transcript.dartapp/test/desktop/chat/closed_sidebar_view_test.dartapp/test/fake_server_demo_data_test.dartapp/test/tool_summary_test.dartapp/test/ui/home/closed_screen_test.dartdocs/DEVELOPMENT.mddocs/specs/2026-07-26-SPEC-29-session-lifecycle-resume-list-delete.mdserver/src/adapters/child_transport.test.tsserver/src/adapters/child_transport.tsserver/src/adapters/stub.tsserver/src/adapters/subprocess-adapter.tsserver/src/idle_reaper.test.tsserver/src/manager.test.tsserver/src/manager.ts
…real server Third CodeRabbit round on PR #157. All three findings verified still-valid against current code; each is red-then-green. `reopenSession` did not await an in-flight close (server/src/manager.ts). Teardown sets `closed = true` only after awaiting the agent, so a reopen that landed in between cleared a flag the close then set again — the session stayed closed although the user asked for it back. It now waits for `closeInFlight`, mirroring the `ensureLiveForInput` guard. While there, the documented idempotency is now real: an already-open session returns early instead of shelling out to `git worktree list` and possibly calling `detachToRoot()`, which would clear a LIVE session's worktree binding (that was the earlier outside-diff comment on the same method). `dispose()` could signal twice (server/src/adapters/child_transport.ts). The escalation timer clears `killTimer` when it fires, so a later dispose() — which the API documents as safe — sent a second SIGTERM and scheduled a second SIGKILL at a process already being force-killed. Tracked with a separate `sigkilled` flag. The FakeServer diverged from the real server (app/lib/store/fake_server.dart). The demo's closed rows were immutable literals, so `session.reopen` could ack but never restore them and they appeared in no snapshot; they now live in `_sessions` like every other session, and `session.listClosed` derives from the model. Repo snapshots also exclude closed sessions, matching `repo_service.ts`, so a closed session stops counting against its worktree. The regression test now reopens a seeded fixture and checks the repo snapshot as well as the session one — it is what caught the first pass filtering the wrong loop.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_386f25b4-906f-4ac1-a440-0369e5e46a10) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Idle sessions were never reclaimed. Measured on the dev host while writing this:
Two root causes:
child_transport.dispose()sent a single unverifiedSIGTERMwith no escalation. Any agent that ignored or outlived it survived forever.archiveSessiondid already callkill()— it just didn't work.archivewas a soft hide: it never asked the back end to release the session, and nothing ever reclaimed one left idle.Since makit runs one agent process per session (60–450 MB each), this compounds until the daemon dies.
What changed
Reaping actually reaps.
dispose()escalatesSIGTERM → SIGKILLafter a grace period (3s,killGraceMs), cancels the escalation once the child settles, and refuses to signal a settled child whose pid may have been recycled.archivebecomes a realclose. Renamed across the wire protocol, server and app:session.archivesession.closesession.unarchivesession.reopensession.listArchivedsession.listClosedarchivedclosedClosing is two steps, both required: the back end's own release primitive (ACP
session/close, gated on the newsessionCapabilities.close; codexthread/unsubscribe) and then the process reap. Step one alone reclaims no memory; step two alone is aSIGTERMto an agent mid-write. Neither can block the other — a wedged agent must never keep its RSS — socloseSessionbounds the graceful call (closeGraceMs, 10s) and reaps regardless.Idle sessions close themselves. New
IdleReaper(MAKIT_IDLE_CLOSE_MIN, default 60,0disables). A stale timestamp is never enough: it skips sessions that are mid-turn, awaiting input or approval, still drafts, already cold, or lacking a native resume handle — every auto-close must be reversible.Recovery is invisible.
send.messageroutes throughensureLiveForInput, which reopens and resumes a closed session.subdeliberately does not, so reading a closed transcript never respawns an agent.The session record, transcript and resume handle are kept throughout, so reopen resumes exactly as before.
capabilities.archivestays — codex has a realthread/archive— but is now clearly a different concept from close.Migration
Existing databases migrate with
ALTER TABLE sessions RENAME COLUMN archived TO closed. Adding a new column instead would have silently returned every already-closed session to the active list and respawned its agent on the next subscribe. Covered by a test that builds a pre-rename schema and asserts the values survive.Breaking
The three
session.*commands are renamed and the DTO fieldarchivedbecomesclosed, so server and app must ship together.How it was tested
pnpm typecheckclean,flutter analyzeclean.SIGTERMis still reclaimed:session.listClosedwith its resume handle → a message reopens it → out of the closed list.ppid=1.Flutter widget tests are flaky on this machine (
flutter_tester"Invalid WebSocket upgrade request" on a random subset each run). Confirmed pre-existing by running the same suite on an unmodified checkout in a separate worktree; every affected file passes individually.Review history
Three independent passes, each finding what the previous missed:
open-code-reviewon the feature → 4 rename/doc misses, including a user-visibleARCHIVEDheader still on screen. My sweeps had been case-sensitive, sogrep archivnever matchedlistArchivedor'ARCHIVED'.codexon all commits → 2 high severity: the sweep evaluated its safety guards once and then closed behind anawait(a message arriving mid-sweep got its agent killed = data loss); andadapter.close()was awaited with no deadline, so a wedged agent stranded teardown beforekill()and left the sweeper permanently disabled — defeating the feature in the one case that matters most.open-code-reviewon the fixes → the fix's own defect: nested deadlines in the wrong order (inner 15s > outer 10s), making the inner one dead code.Plus a structural audit of my own work (
d37560c), which extracted the sweeper out ofSessionManager(it had grown +204 lines onto an already-1472-line file, ignoring theMetricsCollectorpattern the codebase already had), deleted one of the two redundant close deadlines outright, converged four ad-hoc timeout implementations onto one helper, and replaced a triple-negative status check with a canonicalisBusy().Known follow-ups (not in this PR)
ppid=1daemons had accumulated over ~5 days, each holding agents no reaper could reach. This PR stops agents piling up inside a live daemon; it does nothing for a daemon whose app has quit.resumable. The sweeper skips non-resumable sessions so auto-close is always reversible, butcloseSessionhas no such gate — so a manual close of a session with no native handle leaves it transcript-only, while the confirm dialog promises you can "resume where you left off". Rare (any started ACP/codex session captures an id) but the copy shouldn't overpromise.Note
High Risk
Changes core session lifecycle, agent teardown, SQLite migration, and wire protocol; incorrect close/reaper behavior could kill live turns or leak processes, and a mismatched deploy would break clients.
Overview
Replaces the archive lifecycle with close/reopen: wire commands (
session.close/session.reopen/session.listClosed), the persistedclosedflag (SQLitearchived→closed), and matching Flutter store/UI (ClosedScreen, closed sidebar, “Close session” / “Reopen”). Server and app must ship together — the DTO field and RPC names changed.Closing now releases memory: adapters call a graceful back-end release (ACP
session/close, codexthread/unsubscribe) before reap;child_transport.dispose()escalates SIGTERM → SIGKILL after a grace period. AnIdleReaperauto-closes idle sessions (MAKIT_IDLE_CLOSE_MIN, default 60) with guards so busy/draft/non-resumable sessions are skipped.App fixes include live-sync baselines for closed lists (desktop + mobile) and fake-server support so demo/offline paths exercise close/reopen. Docs/spec SPEC-29 and DEVELOPMENT.md describe the new semantics.
Reviewed by Cursor Bugbot for commit 382aedd. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes