perf(llm): keep agent subprocesses alive across chat turns#10554
perf(llm): keep agent subprocesses alive across chat turns#10554eliandoran wants to merge 2 commits into
Conversation
Every turn spawned `copilot --acp`, ran the ACP handshake, created a
session and then disposed the client. Measured on Windows that is ~785
ms to spawn plus ~2.4 s for the first session/new (it refreshes auth and
fetches the model list) -- ~3.2 s before the agent starts generating,
none of it model latency, repeated on every message.
ACP is built for the opposite: one connection hosts many sessions, each
addressed by sessionId, and prompting is a call on a session that
already exists. So the client is now shared process-wide and kept warm
between turns, reaped after 5 minutes idle. A turn whose session is
still loaded issues nothing but session/prompt.
Live against the real CLI (see copilot_agent.live.spec.ts):
cold 6912 ms [initialize, session/new, session/set_model, prompt]
warm 3078 ms [session/set_model, prompt]
Notable consequences:
- Cancellation is now in-band. The client is shared, so disposing it on
abort would kill every other chat's turn; only the pool reaps.
- Session mappings record the process generation that created them. A
session lives inside its process, so after a reap or crash the mapping
is stale and the turn falls back to session/load. Prompting a live
session directly is what makes the warm path cheap -- session/load on
an already-loaded session is rejected by the CLI ("already loaded").
- Flipping note access starts a new session: MCP servers are fixed at
session/new and cannot be reconfigured afterwards.
- AcpClient gained an onExit hook so a dead subprocess evicts itself and
the next turn transparently starts a replacement.
Tests cover process reuse across turns and chats, a shared spawn for
concurrent turns, per-session update routing, crash recovery, the
note-tools reseed and the idle reap. The live test is opt-in via
TRILIUM_COPILOT_LIVE_TEST=1 -- it spends premium-request budget.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`query({ prompt: "text" })` binds the Claude Code subprocess to the
returned iterator: it spawns on call and dies when iteration ends, so
every turn re-paid the spawn. Measured on Windows that is ~2.3 s to the
first token cold against ~0.9 s once warm.
Switch to the SDK's streaming input mode -- pass an AsyncIterable as the
prompt and push each turn's message onto it -- keeping one subprocess
per chat. That is also the only mode in which the Query control methods
work, which is what makes a mid-chat model switch a ~0 ms setModel()
call instead of a cold start.
Live against the real CLI (see claude_agent.live.spec.ts):
cold first token 2265 ms -> warm 866 ms
Iteration is deliberately manual (`query.next()` in a loop, stopping at
the turn's `result`) rather than `for await ... break`: breaking out of
a for-await calls iterator.return(), which closes the query and kills
the process this exists to keep warm.
The design follows zed's claude-code-acp adapter, which solves the same
problem, minus its persistent-consumer and turn-deferred machinery --
that exists to interleave queued and autonomous turns, whereas a Trilium
chat is strictly one turn at a time.
Notable decisions:
- Options fixed at query() construction (system prompt, note tools,
thinking) are fingerprinted; a change retires the session and builds a
new one, since they cannot be applied to a live query.
- An aborted turn closes the session rather than returning it to the
pool: it stops mid-stream, so the session's state is unknown and the
SDK can be wedged mid-next() (interrupt() is not guaranteed to yield --
agentclientprotocol/claude-agent-acp#680). The next turn pays a cold start.
- Warm sessions are capped and reaped after 5 minutes idle. Unlike the
Copilot pool this is one subprocess *per chat*, so the reaper bounds
real memory rather than just being tidy.
- Pushable.push after end() is a no-op, not a hang: enqueueing onto a
dead stream would otherwise wait on a promise nobody settles, the
failure behind claude-agent-acp#338.
The live test is opt-in via TRILIUM_CLAUDE_LIVE_TEST=1.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces keep-alive session pooling for both the Claude and Copilot LLM providers to significantly reduce latency by reusing warm subprocesses across chat turns. For Claude, a new session pool keeps a subprocess alive per chat using streaming input mode, while for Copilot, a shared process-wide client multiplexes multiple sessions. Additionally, live integration and unit tests are added to verify keep-alive behavior, model switching, and idle reaping. Feedback is provided regarding the evictOldest logic in the Claude session pool, which could fail to evict idle sessions if the oldest session is busy, potentially leading to a resource leak.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| function evictOldest(): void { | ||
| while (sessionsByChatNote.size > MAX_WARM_SESSIONS) { | ||
| const [oldestId, oldest] = [...sessionsByChatNote.entries()][0]; | ||
| // Never evict a session mid-turn; stop at the first busy one so a | ||
| // pathological all-busy map degrades to "no eviction" instead of | ||
| // killing a live reply. | ||
| if (oldest.busy) { | ||
| break; | ||
| } | ||
| closeSession(oldestId, oldest); | ||
| } | ||
| } |
There was a problem hiding this comment.
The evictOldest function stops at the first busy session it encounters. If the oldest session in the map is busy, no other idle sessions will be evicted, even if the pool size exceeds MAX_WARM_SESSIONS. This can lead to a resource leak of warm subprocesses. Instead, iterate through the map and evict any non-busy sessions until the pool size is within the limit.
function evictOldest(): void {
for (const [chatNoteId, session] of sessionsByChatNote.entries()) {
if (sessionsByChatNote.size <= MAX_WARM_SESSIONS) {
break;
}
if (!session.busy) {
closeSession(chatNoteId, session);
}
}
}
Greptile SummaryThis PR eliminates the per-turn subprocess spawn overhead for both the Claude and Copilot agent providers by keeping subprocesses warm across chat turns, cutting time-to-first-token by 55–62% on Windows.
Confidence Score: 3/5The warm-session logic in
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Manual iteration on purpose: `for await (…) { break }` calls | ||
| // iterator.return(), which closes the query and kills the process | ||
| // this whole pool exists to keep warm. | ||
| let turnComplete = false; |
There was a problem hiding this comment.
turnComplete is declared inside the try block and is therefore invisible to the finally block. When active.query.next() throws (subprocess crash, protocol error, rate-limit, etc.) the turn exits via catch without ever setting turnComplete. In finally, held.closed remains false and signal?.aborted is false, so reusable evaluates to true — the broken session is handed back to the pool via releaseSession. The next turn picks it up as warm, pushes a new user message into a dead stream, and either gets an immediate error or hangs on query.next().
The fix is to hoist turnComplete to the outer scope (alongside held) so finally can gate on it: const reusable = … && !signal?.aborted && turnComplete;.
| function evictOldest(): void { | ||
| while (sessionsByChatNote.size > MAX_WARM_SESSIONS) { | ||
| const [oldestId, oldest] = [...sessionsByChatNote.entries()][0]; | ||
| // Never evict a session mid-turn; stop at the first busy one so a | ||
| // pathological all-busy map degrades to "no eviction" instead of | ||
| // killing a live reply. | ||
| if (oldest.busy) { | ||
| break; | ||
| } | ||
| closeSession(oldestId, oldest); | ||
| } | ||
| } |
There was a problem hiding this comment.
The while loop retrieves only the oldest map entry and breaks immediately if it is busy. When the oldest session belongs to a long-running conversation, all younger idle sessions behind it are never examined and the map grows beyond MAX_WARM_SESSIONS indefinitely — one extra slot per new session added while the oldest is busy. The idle reaper eventually recovers, but the stated memory bound isn't enforced during active use.
Scanning past busy entries toward the first idle one would honour the bound more faithfully without risking a live reply.
| } else if (resumable !== undefined) { | ||
| // The session outlived its process (reaped, crashed, or the | ||
| // server restarted). session/load replays its history from | ||
| // disk; no listener is registered yet, so that replay is | ||
| // dropped rather than streamed to the user as a fresh reply. | ||
| const mcpServers = noteToolsEnabled ? await buildMcpServersConfig() : []; | ||
| try { | ||
| await client.request("session/load", { sessionId: resume, cwd: getAgentCwd(), mcpServers }, SESSION_TIMEOUT_MS); | ||
| sessionId = resume; | ||
| await client.request("session/load", { sessionId: resumable, cwd: getAgentCwd(), mcpServers }, SESSION_TIMEOUT_MS); | ||
| resolved = resumable; | ||
| } catch (err) { | ||
| getLog().info(`Copilot Agent provider: session/load failed (${describeError(err)}); reseeding a fresh session.`); | ||
| } finally { | ||
| collector.muted = false; | ||
| } | ||
| } | ||
|
|
||
| if (!sessionId) { | ||
| if (!resolved) { | ||
| const mcpServers = noteToolsEnabled ? await buildMcpServersConfig() : []; |
There was a problem hiding this comment.
buildMcpServersConfig() is an async call that is invoked a second time unconditionally in the session/new branch even when it was already awaited in the session/load branch and failed. Extract it into a shared variable to avoid the redundant network round-trip on the failure path.
| } else if (resumable !== undefined) { | |
| // The session outlived its process (reaped, crashed, or the | |
| // server restarted). session/load replays its history from | |
| // disk; no listener is registered yet, so that replay is | |
| // dropped rather than streamed to the user as a fresh reply. | |
| const mcpServers = noteToolsEnabled ? await buildMcpServersConfig() : []; | |
| try { | |
| await client.request("session/load", { sessionId: resume, cwd: getAgentCwd(), mcpServers }, SESSION_TIMEOUT_MS); | |
| sessionId = resume; | |
| await client.request("session/load", { sessionId: resumable, cwd: getAgentCwd(), mcpServers }, SESSION_TIMEOUT_MS); | |
| resolved = resumable; | |
| } catch (err) { | |
| getLog().info(`Copilot Agent provider: session/load failed (${describeError(err)}); reseeding a fresh session.`); | |
| } finally { | |
| collector.muted = false; | |
| } | |
| } | |
| if (!sessionId) { | |
| if (!resolved) { | |
| const mcpServers = noteToolsEnabled ? await buildMcpServersConfig() : []; | |
| const mcpServers = noteToolsEnabled ? await buildMcpServersConfig() : []; | |
| if (resumable !== undefined) { | |
| // The session outlived its process (reaped, crashed, or the | |
| // server restarted). session/load replays its history from | |
| // disk; no listener is registered yet, so that replay is | |
| // dropped rather than streamed to the user as a fresh reply. | |
| try { | |
| await client.request("session/load", { sessionId: resumable, cwd: getAgentCwd(), mcpServers }, SESSION_TIMEOUT_MS); | |
| resolved = resumable; | |
| } catch (err) { | |
| getLog().info(`Copilot Agent provider: session/load failed (${describeError(err)}); reseeding a fresh session.`); | |
| } | |
| } | |
| if (!resolved) { |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Both agent providers spawned a CLI subprocess, ran a handshake, and threw it all away on every chat turn. This keeps the process warm between turns, cutting seconds of pure overhead off each message — none of it model latency.
Measured on Windows against the real CLIs (times to first token):
Two commits, one per provider — they solve the same problem but the mechanisms have nothing in common.
Copilot: one shared client, many sessions
ACP is built for this — one connection hosts many sessions addressed by
sessionId, and prompting is a call on a session that already exists. The client is now shared process-wide and reaped after 5 minutes idle. The protocol trace tells the story:The
session/newalone was ~2.4 s: it refreshes auth and fetches the model list.Consequences worth reviewing:
session/load. Prompting a live session directly is what makes the warm path cheap —session/loadon an already-loaded session is rejected by the CLI ("already loaded").session/new.AcpClientgained anonExithook so a dead subprocess evicts itself.Claude: streaming input mode
query({ prompt: "text" })binds the subprocess to the returned iterator, so it dies when iteration ends. Passing anAsyncIterableinstead keeps one subprocess per chat. That is also the only mode in which theQuerycontrol methods work — which makes a mid-chat model switch a ~0 mssetModel()instead of a cold start.Iteration is deliberately manual (
query.next()in a loop, stopping at the turn'sresult) rather thanfor await … break: breaking out of a for-await callsiterator.return(), which closes the query and kills the process this exists to keep warm.The design follows zed's claude-code-acp adapter, which solves the same problem — minus its persistent-consumer and turn-deferred machinery, which exists to interleave queued and autonomous turns. A Trilium chat is strictly one turn at a time.
Decisions worth reviewing:
query()construction (system prompt, note tools, thinking) are fingerprinted; a change retires the session.next()(interrupt()is not guaranteed to yield — claude-agent-acp#680). The next turn pays a cold start.Pushable.pushafterend()is a no-op, not a hang — enqueueing onto a dead stream would otherwise wait on a promise nobody settles (claude-agent-acp#338).Testing
359 server LLM tests pass. New unit coverage: process reuse across turns and chats, a shared spawn for concurrent turns, per-session update routing, crash recovery, config-change reseeds, cancellation, and the idle reap.
Two opt-in live integration tests drive the real CLIs and assert the latency drop — skipped by default since they spend subscription budget:
Notes for the reviewer
worktree-copilot-agent-provider, notmain— the Copilot provider isn't inmainyet, so this stacks on that branch.session/promptrather thansession/load. Those assertions were updated, not deleted.🤖 Generated with Claude Code