perf(agents): keep active sessions warm and reap idle state - #195
perf(agents): keep active sessions warm and reap idle state#195Waishnav wants to merge 7 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change adds a warm Claude streaming runtime, routes Claude through the shared runtime pool, passes agent IDs through local-agent execution, and adds idle-session reaping for ACP, Codex, and Pi runtimes. ChangesLocal-agent runtime lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LocalAgentManager
participant LocalAgentRuntimeRegistry
participant RuntimePool
participant ClaudeWarmRuntime
participant ClaudeSDK
LocalAgentManager->>LocalAgentRuntimeRegistry: executeTurn with agentId
LocalAgentRuntimeRegistry->>RuntimePool: route Claude run
RuntimePool->>ClaudeWarmRuntime: acquire pooled runtime
ClaudeWarmRuntime->>ClaudeSDK: stream prompt or resume session
ClaudeSDK-->>ClaudeWarmRuntime: result message and session ID
ClaudeWarmRuntime-->>RuntimePool: return run result
RuntimePool-->>LocalAgentRuntimeRegistry: return provider response
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Greptile SummaryThe PR adds a pooled warm Claude SDK runtime and provider-level idle-session cleanup while preserving durable conversation identifiers.
Confidence Score: 4/5The idle-reaping race should be fixed before merging because it can terminate a provider runtime after a new turn has already started. The new awaited per-provider cleanup separates the pool's activity check from eviction, allowing a concurrent run to make the slot active before the reaper deletes and closes it. Files Needing Attention: src/local-agent-runtime-pool.ts
|
| Filename | Overview |
|---|---|
| src/local-agent-claude/runtime.ts | Adds a persistent Claude SDK query, prompt queue, model switching, resumable session handling, and runtime driver. |
| src/local-agent-runtime-pool.ts | Adds provider-level idle cleanup, but the awaited cleanup creates a race that can close a newly active runtime. |
| src/local-agent-acp/runtime.ts | Tracks ACP session activity and closes idle sessions when the provider advertises support. |
| src/local-agent-codex/runtime.ts | Tracks thread activity and unsubscribes idle Codex threads without shutting down the shared app server. |
| src/local-agent-pi/runtime.ts | Wraps Pi sessions with activity metadata and disposes idle in-memory sessions for later durable reopening. |
| src/local-agent-runtime-registry.ts | Registers Claude as a pooled harness driver alongside Codex, ACP, and Pi. |
Sequence Diagram
sequenceDiagram
participant Timer as Idle reaper
participant Pool as Runtime pool
participant Run as New turn
participant Runtime as Provider runtime
Timer->>Pool: reapIdle()
Pool->>Pool: "observe activeRuns == 0"
Pool->>Runtime: await reapIdleSessions(now)
Run->>Pool: acquire same ready slot
Pool->>Pool: activeRuns++
Run->>Runtime: run(input)
Runtime-->>Pool: cleanup returns
Pool->>Pool: delete slot
Pool->>Runtime: close()
Runtime-->>Run: active turn fails
Reviews (1): Last reviewed commit: "perf(agents): reclaim idle provider sess..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
5f76ffb to
c7233f2
Compare
Waishnav
changed the title
c7233f2 to
4529736
Compare
4529736 to
0b3d090
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
0b3d090 to
f9086d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/local-agent-claude/runtime.test.ts (1)
83-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the failure and stream-end paths.
The test covers reuse, model change, effort-triggered replacement, and close. It does not cover the two paths in
ClaudeWarmRuntime.runthat callresetQueryon failure:
next()resolves withdone: true, which must throw "Claude session ended before returning a result." and discard the live query.- A result message with
is_error: true, which must throw and discard the live query.Both paths must leave the runtime able to start a fresh query and resume from the persisted provider session id. That recovery behavior is the reason the warm runtime is safe to pool. Add a case that forces a failure and then asserts the next run creates a new query with
resumeset to the persisted session id.Based on learnings, add and maintain behavior and regression tests for affected contracts and lifecycle behavior.
🤖 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 `@src/local-agent-claude/runtime.test.ts` around lines 83 - 100, Add regression coverage around ClaudeWarmRuntime.run for both resetQuery failure paths: a stream ending before a result and an is_error result message. Assert each throws the expected error, discards the live query, and allows the following run to create a fresh query resumed with the persisted provider session id.Source: Learnings
src/local-agent-claude/runtime.ts (1)
42-85: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReject concurrent turns on one warm runtime.
runpushes a prompt and then drainslive.query.next()in a loop. Two overlappingruncalls share one iterator. Each call would consume messages belonging to the other turn, and both results would be wrong.The manager serializes turns per agent, and the pool key is the agent id, so the current call path is safe. The runtime itself does not enforce this.
CodexAppServerRuntime.runand the ACP runtime both reject a second concurrent turn with an explicit error. Add the same guard here to keep the invariant local and testable.♻️ Proposed guard
export class ClaudeWarmRuntime implements HarnessRuntime { private live: LiveClaudeQuery | undefined; private providerSessionId: string | undefined; private closed = false; + private turnInProgress = false; constructor(private readonly createQuery: ClaudeQueryFactory) {} async run(input: LocalAgentRunInput): Promise<LocalAgentRunResult> { if (this.closed) throw new Error("Claude runtime is closed."); + if (this.turnInProgress) { + throw new Error("Claude runtime already has a turn in progress."); + } + this.turnInProgress = true; + try {Close the
trywith afinally { this.turnInProgress = false; }around the existing body.🤖 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 `@src/local-agent-claude/runtime.ts` around lines 42 - 85, Update the Claude runtime class and its run method to reject a second concurrent turn with an explicit error before mutating the shared query state. Track turn ownership with a turnInProgress guard, set it when entering run, and clear it in a finally block covering the existing query-draining logic, including error and success paths.package.json (1)
31-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the manual test chain with Node.js test discovery.
The package requires Node.js 22.19 or newer, and the current script lists all 31 test files manually. Use the test runner to include new suites automatically and report failures from later suites:
Example replacement
- "test": "tsx src/config.test.ts && ... && tsx src/cli.test.ts", + "test": "tsx --test \"src/**/*.test.ts\"",Node.js process isolation keeps the existing top-level assertion suites and their environment changes isolated.
🤖 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 `@package.json` at line 31, Replace the manual test command in the package scripts with Node.js 22.19+ test discovery, using process isolation so the existing top-level assertion suites and environment changes remain isolated. Configure discovery to include the current test files automatically and ensure failures from later suites are still reported.
🤖 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 `@src/local-agent-claude/runtime.ts`:
- Around line 97-109: Update ensureQuery to bind each live query to
input.workspace: store the workspace when creating this.live, compare it
alongside thinking before reusing the existing query, and reset/recreate (or
explicitly reject) when the workspace differs. Preserve reuse only when both
workspace and effort match, ensuring createQuery receives the current workspace
context.
In `@src/local-agent-runtime-pool.ts`:
- Around line 83-85: The cleanup loop in maintainSlot must not use runtime-wide
activeRuns to suppress reaping of unrelated sessions. Add session-level
coordination so idle sessions can be cleaned while another PiHarnessRuntime
session is active, or defer cleanup until the runtime becomes idle, and add
coverage for the mixed idle/active-session case across the process and subagent
lifecycle.
---
Nitpick comments:
In `@package.json`:
- Line 31: Replace the manual test command in the package scripts with Node.js
22.19+ test discovery, using process isolation so the existing top-level
assertion suites and environment changes remain isolated. Configure discovery to
include the current test files automatically and ensure failures from later
suites are still reported.
In `@src/local-agent-claude/runtime.test.ts`:
- Around line 83-100: Add regression coverage around ClaudeWarmRuntime.run for
both resetQuery failure paths: a stream ending before a result and an is_error
result message. Assert each throws the expected error, discards the live query,
and allows the following run to create a fresh query resumed with the persisted
provider session id.
In `@src/local-agent-claude/runtime.ts`:
- Around line 42-85: Update the Claude runtime class and its run method to
reject a second concurrent turn with an explicit error before mutating the
shared query state. Track turn ownership with a turnInProgress guard, set it
when entering run, and clear it in a finally block covering the existing
query-draining logic, including error and success paths.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 73394a35-e7e3-4495-994c-10da19faa050
📒 Files selected for processing (16)
docs/agent-profile-schema.mdpackage.jsonsrc/local-agent-acp/runtime.tssrc/local-agent-adapters.tssrc/local-agent-claude/runtime.test.tssrc/local-agent-claude/runtime.tssrc/local-agent-codex/runtime.test.tssrc/local-agent-codex/runtime.tssrc/local-agent-manager.test.tssrc/local-agent-manager.tssrc/local-agent-pi/runtime.test.tssrc/local-agent-pi/runtime.tssrc/local-agent-runtime-pool.test.tssrc/local-agent-runtime-pool.tssrc/local-agent-runtime-registry.tssrc/local-agent-runtime.ts
f9086d1 to
a3da077
Compare
a3da077 to
fbc19e6
Compare
|
Closing this Sol stack PR in favor of the Luna-based implementation. |
1 participant
Claude still restarts its CLI-backed query between turns, and multiplexed runtimes can retain provider sessions long after those sessions stop being useful. This keeps one streaming Claude query warm per active DevSpace agent and completes the pool lifecycle with provider-owned idle-session reaping: Codex can unsubscribe threads, ACP can close sessions when supported, and Pi can dispose idle AgentSessions without tearing down a shared runtime.\n\nThe runtime pool remains provider-agnostic: it only asks a runtime to reap idle session state and still treats every live runtime as disposable. Claude stays intentionally per-agent rather than being multiplexed across unrelated conversations. Stacked on #194.
Summary by CodeRabbit
New Features
Documentation
Tests