Skip to content

Commit 070116a

Browse files
authored
fix(agent): tighten idle-drain lifecycle from PR review
Address Greptile review comments on the idle drainer: - interrupt(): await settleIdleDrainStop() after query.interrupt() so a drainer that is mid-forward (awaiting client.sessionUpdate) fully exits before interrupt() returns, closing the window where it could emit an event for an already-cancelled session. - startIdleDrain scheduling: capture the session at schedule time and bail in the microtask if this.session was swapped (e.g. by refreshSession) before it runs, so the drainer can't tag new-query events with the old session id. - Document the single-drainer invariant on the startIdleDrain guard: because a drainer holding a handoff keeps this.idleDrain non-null until settleIdleDrainStop clears it, no second drainer can be assigned in that window (the third review comment was self-correcting, not a bug). Adds a test asserting cancel() while idle-draining awaits the drainer's exit. Generated-By: PostHog Code Task-Id: 39548658-f313-445b-bd1c-d07eaf9281cf
1 parent 1a98164 commit 070116a

2 files changed

Lines changed: 41 additions & 2 deletions

File tree

packages/agent/src/adapters/claude/claude-agent.idle-drain.test.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,4 +232,21 @@ describe("ClaudeAcpAgent — between-turns idle drain (autonomous /loop turns)",
232232
// follow-up prompt drained the backlog).
233233
expect(texts.filter((t) => t === "autonomous update")).toHaveLength(1);
234234
});
235+
236+
it("cancel while idle-draining stops the drainer and returns cleanly", async () => {
237+
const { agent } = makeAgent();
238+
const sessionId = "s-idle-3";
239+
const { query, input } = installFakeSession(agent, sessionId);
240+
241+
await runTurn(agent, query, input, sessionId, "loop", "answer", "r1");
242+
// Drainer starts and blocks on query.next().
243+
await tick();
244+
const drainSlot = () =>
245+
(agent as unknown as { idleDrain: unknown }).idleDrain;
246+
expect(drainSlot()).not.toBeNull();
247+
248+
// Cancelling must await the drainer's exit (not return with it still live).
249+
await agent.cancel({ sessionId });
250+
expect(drainSlot()).toBeNull();
251+
});
235252
});

packages/agent/src/adapters/claude/claude-agent.ts

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1223,15 +1223,25 @@ export class ClaudeAcpAgent extends BaseAcpAgent {
12231223
!this.session.cancelled
12241224
) {
12251225
const sessionId = params.sessionId;
1226-
queueMicrotask(() => this.startIdleDrain(sessionId));
1226+
const session = this.session;
1227+
queueMicrotask(() => {
1228+
// Bail if the session was swapped (e.g. by refreshSession) between
1229+
// scheduling and running: startIdleDrain reads this.session at
1230+
// execution time, so without this it could drain the new query while
1231+
// tagging events with the old session id.
1232+
if (this.session === session) {
1233+
this.startIdleDrain(sessionId);
1234+
}
1235+
});
12271236
}
12281237
}
12291238
}
12301239

12311240
// Called by BaseAcpAgent#cancel() to interrupt the session
12321241
protected async interrupt(): Promise<void> {
12331242
// A cancel/close may land while the between-turns idle drainer owns the
1234-
// query; stop it so `query.interrupt()` below isn't racing a live reader.
1243+
// query; flag it now so it stops at its next boundary, and fence its exit
1244+
// after `query.interrupt()` below (see end of method).
12351245
this.requestIdleDrainStop();
12361246
this.session.cancelled = true;
12371247
for (const [, pending] of this.session.pendingMessages) {
@@ -1255,6 +1265,13 @@ export class ClaudeAcpAgent extends BaseAcpAgent {
12551265
}
12561266

12571267
await this.session.query.interrupt();
1268+
1269+
// Fence the drainer's exit. `query.interrupt()` unblocks a drainer suspended
1270+
// in `query.next()`, but one mid-forward only exits after its in-flight
1271+
// `client.sessionUpdate()` settles. Awaiting here keeps interrupt() from
1272+
// returning while the drainer can still emit an event for a cancelled
1273+
// session. No-op when no drainer is running.
1274+
await this.settleIdleDrainStop();
12581275
}
12591276

12601277
/**
@@ -1355,6 +1372,11 @@ export class ClaudeAcpAgent extends BaseAcpAgent {
13551372
* (reverting to prior behavior), never breaks the session.
13561373
*/
13571374
private startIdleDrain(sessionId: string): void {
1375+
// At most one drainer at a time. Combined with "drainers are only scheduled
1376+
// after a completed turn", this is what keeps the slot unambiguous: while a
1377+
// drainer holds a handoff for an in-flight prompt(), `this.idleDrain` stays
1378+
// non-null (its finally skips clearing), so no second drainer can be
1379+
// assigned before settleIdleDrainStop() clears it.
13581380
if (this.idleDrain) return;
13591381
if (this.session.promptRunning) return;
13601382
if (this.session.abortController.signal.aborted) return;

0 commit comments

Comments
 (0)