feat(core): reapChatSession — close a managed session the reap policy never releases - #441
Conversation
… never releases `decideReap` keeps a streaming session alive for exactly as long as it holds live background work, with no idle timer and no max-lifetime backstop. That is the right default — reaping a session with work in flight kills the work — but it left consumers with no way out when a session becomes permanently unreapable, which happens in at least two ordinary ways: - A background task never exits, so `backgroundTasks` never drains. - A re-invocation turn dies without firing a Stop hook (a subscription usage limit, say). `activity` has already cleared `awaitingTasks`, so every later `background_tasks_changed` returns early at the guard, and only a `turn_end` could re-arm it or reap. None comes. In both cases the session's message stream never ends, so a consumer rendering that stream shows the session as running until the process restarts. `SessionReaper.forceReap(sessionId)` closes a live managed session regardless of what it holds; `FleetManager.reapChatSession(sessionId)` exposes it, since the lifecycle manager is private on the fleet. Both are idempotent and return false for an unknown, unmanaged or already-reaped id. This belongs on the reaper rather than being a `close()` the consumer calls on the RuntimeSession it already holds: closing the query directly leaves `liveById` holding a stale entry, so `whenSessionReaped` never resolves (a later resume stalls until its ceiling, #403) and `WakeRegistry` skips that session's wakes forever. `forceReap` routes through the same private `reap` the policy uses, so the id is unregistered, reap waiters drain, and the consumer just sees an ordinary end-of-stream. Policy is unchanged: nothing reaps on its own that didn't before. Tests cover the stranded-session shape above, the waiter drain, and idempotency. Unblocks edspencer/paddock#528. Co-Authored-By: Claude <noreply@anthropic.com>
Deploying herdctl with
|
| Latest commit: |
2aede3a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0727402f.herdctl.pages.dev |
| Branch Preview URL: | https://feat-force-reap-session.herdctl.pages.dev |
📝 WalkthroughWalkthroughThis change adds ChangesManaged session force-reaping
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant FleetManager
participant SessionReaper
participant RuntimeSession
Caller->>FleetManager: reapChatSession(sessionId)
FleetManager->>SessionReaper: forceReap(sessionId)
SessionReaper->>RuntimeSession: close through normal reap cleanup
SessionReaper-->>FleetManager: return boolean result
FleetManager-->>Caller: return boolean result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/fleet-manager/__tests__/reap-chat-session.test.ts (1)
11-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMock the file system in this test.
The test creates and removes real directories through
node:fs/promises. This makes the lifecycle test depend on host filesystem behavior.Use the repository's in-memory filesystem utility or mock the filesystem calls. If this must remain an integration test, move it to a location with an explicit integration-test exemption.
As per coding guidelines, “Mock external dependencies (SDK, file system, GitHub API) in tests.” <coding_guidelines>
Also applies to: 56-78
🤖 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 `@packages/core/src/fleet-manager/__tests__/reap-chat-session.test.ts` around lines 11 - 13, Update the test’s filesystem setup and cleanup around the lifecycle test to use the repository’s in-memory filesystem utility or mocks instead of real node:fs/promises calls; replace the mkdir, mkdtemp, rm, and writeFile usage while preserving the existing test behavior and assertions.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.
Nitpick comments:
In `@packages/core/src/fleet-manager/__tests__/reap-chat-session.test.ts`:
- Around line 11-13: Update the test’s filesystem setup and cleanup around the
lifecycle test to use the repository’s in-memory filesystem utility or mocks
instead of real node:fs/promises calls; replace the mkdir, mkdtemp, rm, and
writeFile usage while preserving the existing test behavior and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 74465938-fee0-48e4-84a8-10a75a7cfdf1
📒 Files selected for processing (7)
.changeset/force-reap-session.mddocs/src/content/docs/concepts/sessions.mddocs/src/content/docs/library-reference/fleet-manager.mdxpackages/core/src/fleet-manager/__tests__/reap-chat-session.test.tspackages/core/src/fleet-manager/fleet-manager.tspackages/core/src/session/__tests__/session-reaper.test.tspackages/core/src/session/session-reaper.ts
Skipping this one — the premise doesn't hold on two counts:
The guideline it cites ("mock external dependencies") is aimed at the SDK, network and GitHub API — non-deterministic or costly dependencies. A Happy to revisit if an in-memory fs helper is introduced and the other 14 files move over — but this one test shouldn't be the odd one out. |
The reapChatSession/forceReap API this fix depends on shipped in edspencer/herdctl#441, released as core 5.31.0. package.json already asked for it; this refreshes the lockfile off the published tarball, so CI installs the same package the tests were re-verified against rather than a local build. Co-Authored-By: Claude <noreply@anthropic.com>
…662) * fix(server): make Stop work while a chat runs background work (#528) A chat could sit with the spinner and Stop showing forever. Stop did nothing — no error, no frame, no log line. The composer silently queued anything typed instead of sending it, and reloading didn't help (the state is server-authoritative and replays as running). Only a server restart cleared it. Two independent things had to be wrong at once, and both were. **No cancellable identity.** Once a session-mode turn's primary `result` lands, the session can stay open — the reaper holds it while the turn's background work runs — and autonomous re-invocation turns keep arriving on the same stream. `makeBackgroundTurnSink` renders that stretch as one hub turn but never called `setJobId`; it was called at only two of the five turn-start sites, and this was one of the three that missed. So every frame and every `chat:active` carried `jobId: null`, the client's deferred cancel (#196) waited for an id that never arrived, and clicking Stop put nothing on the wire at all — hence silent rather than errored. The sink now mints a synthetic job id and publishes it when the turn opens, as the foreground path does via `onJobCreated`. **Nothing to route to.** `cancel` knew a live turn in `liveSessions` (→ `interrupt()`) and a batch job (→ `cancelJob`). The primary turn's `liveSessions` entry is dropped the moment it returns, so a background-phase id matched neither and fell through to `cancelJob(<synthetic uuid>)` → JobNotFoundError → false, discarded by the WS layer. `interrupt()` was the wrong primitive anyway: it ends an in-flight model turn, and this session is idle holding background work. Cancel now routes these to `fleet.reapChatSession()`, so the stream ends and the existing unwind emits `chat:complete`. Easiest to hit on a subscription usage limit: sub-agents die, the parent's re-invocation turn dies without a Stop hook, and the reaper's `awaitingTasks` (cleared by that turn's `activity`) means no later signal can reap the session. Also covers the originally reported trigger — a model-authored `until` loop whose sentinel never arrives. Requires @herdctl/core >= 5.31.0 (edspencer/herdctl#441), so this must land after that release. Co-Authored-By: Claude <noreply@anthropic.com> * chore: lock @herdctl/core at the released 5.31.0 The reapChatSession/forceReap API this fix depends on shipped in edspencer/herdctl#441, released as core 5.31.0. package.json already asked for it; this refreshes the lockfile off the published tarball, so CI installs the same package the tests were re-verified against rather than a local build. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Paddock <paddock@valfenda.net> Co-authored-by: Claude <noreply@anthropic.com>
1 participant
The gap
decideReapkeeps a streaming session alive for exactly as long as it holds live background work, and says so plainly in its own header: "No idle timer, no max-lifetime backstop, no idle-concurrency cap."That is the right default — reaping a session with work in flight kills the work. But it left consumers with no way out when a session becomes permanently unreapable, and there are at least two ordinary ways that happens:
untilloop whose sentinel never arrives, sobackgroundTasksnever drains.activityhas already clearedawaitingTasks, so every laterbackground_tasks_changedreturns early at theisAwaitingTasks()guard, and only aturn_endcould re-arm it or reap. None comes. The session is stranded live with no pending reap.In both cases the session's message stream never ends, so a consumer rendering that stream shows the session as running until the process restarts. There was no API to end it.
The change
SessionReaper.forceReap(sessionId): boolean— close a live managed session regardless of what it is holding.FleetManager.reapChatSession(sessionId): boolean— public exposure, sincesessionLifecycleis private on the fleet.Both idempotent; both return
falsefor an unknown, unmanaged or already-reaped id.Why on the reaper, and not just
close()A consumer holds the
RuntimeSessionand could callclose()itself. That is not equivalent — it closes the query behind the reaper's bookkeeping:liveByIdkeeps a stale entry, sowhenSessionReapednever resolves and a later resume of that id stalls until its 5-minute ceiling (openChatSession(resume) spawns a second subprocess for an already-live session → SDK self-interrupt (double-resume class) #403).WakeRegistryskips that session's wakes indefinitely.forceReaproutes through the same privatereapthe policy uses, somarkDone()unregisters the id and drains the reap waiters beforeclose(). The consumer observes an ordinary end-of-stream and unwinds through its ordinary path — no new teardown contract to get right.Worth stating explicitly:
interrupt()is not a substitute. It targets an in-flight model turn, and a session held open purely for background work has none.Scope
Policy is untouched — nothing reaps on its own that didn't before. This only adds a door that can be opened deliberately.
The reap log line now carries a reason;
Reaping idle session …is unchanged, a forced one readsReaping force-reaped on request session ….Tests
Four new reaper tests and three FleetManager tests. The one that matters is
rescues a session stranded live by a re-invocation turn that never ends— it drives the real signal sequence (turn_endkeepAlive →background_tasks_changeddrained →activity, then nothing) and asserts the session is genuinely stuck (onReapnever fires,close()never called) before showingforceReapreleases it.pnpm typecheckgreen across the monorepo; core suite passes (3675) and the docs site builds. One pre-existing failure locally,directory.test.ts > throws StateDirectoryCreateError when parent directory is not writable, is environmental — it fails onmaintoo because this box runs as uid 0, so a chmod-unwritable parent is still writable.Docs
library-reference/fleet-manager.mdxgains areapChatSession()section;concepts/sessions.mdnotes that the keep-alive rule has no backstop and points at it.Unblocks edspencer/paddock#528.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests