mirror of
https://github.com/agentclientprotocol/claude-agent-acp.git
synced 2026-10-27 04:30:59 +00:00
fix: end a held turn cancelled mid-followup at its interrupt's idle (#1229)
Found while reviewing #1226. It affects ACP v1 and v2. ## Problem A turn held open for its background subagents (`Turn.deferredSettle`, issues #864–#866) was settled `cancelled` inside `cancel()`, before the interrupt. The followup cycle can be live at that point, writing the subagents' summary or blocked on a permission request. The interrupt still flushes that cycle's output, so: - **v1:** the output arrives after `session/prompt` has answered `cancelled`, when many clients have stopped listening. - **v2 (#1226):** the output arrives after `idle cancelled`. The v2 draft says updates after a cancel MUST come before it. ## Change `cancel()` now settles inline only where the consumer can't: - **a settling turn,** blocked in its checkpoint preview, which can't process the interrupt's trailing idle (unchanged); - **a held turn whose session sits idle,** or has reported no state yet: the interrupt emits nothing, so no trailing idle comes (unchanged). **A held turn with a live followup** (`running` or `requires_action`) is now left to the consumer, like any live turn. It ends at the interrupt's trailing idle, or at the next prompt's echo if that idle lags, after the output the interrupt flushed. The force-cancel backstop is armed for it meanwhile. Two consequences, both handled: - **The trailing-idle debt:** the interrupted followup's result is autonomous, so it records its trailing idle as owed. When that idle ends the cancelled held turn, it now also pays the debt. Otherwise the debt would absorb a later issue-#825 signal. - **Usage:** a held turn can now be ended by the trailing idle, the echo hand-off, stream end or the backstop, not only by `cancel()`. All of them take the outcome from one rule, `cancelledOutcome`: a turn that already has its result keeps that result's usage and `_meta`, as the inline settle did (issue #844). Today the session's tally equals that recorded outcome anyway, since followup results aren't accumulated. Stream end already treats the stored one as authoritative. ## Testing - **New:** `src/tests/turn-events.test.ts` cancels a held turn mid-followup. The interrupt flushes more of the summary, then the followup's result and idle arrive. It asserts: - the flushed output comes before the turn ends `cancelled`; - a wedged next turn still fails at its un-owed idle (issue #825), so no trailer debt leaked. - **Updated:** the two existing tests that cancel a held turn in a live cycle (running, and blocked on a permission request) expected the prompt to resolve at `cancel()`. Their scripts send the interrupt's trailer only after the next prompt's echo, so the turn now ends at that echo hand-off. They keep their issue #825 assertion, and one now also checks the usage the turn keeps (issue #844). - **Unchanged:** the tests for an idle hold, which settles at `cancel()` without pre-counting a trailer, pass as before. - **Mutations:** settling a live held turn inline again, settling an idle one at an idle that never comes, or skipping the debt payment each fail at least one test. `npm run build`, `npm run check`, `npm run test:run` (2244 passed).
This commit is contained in:
Ben Brandt
@@ -4756,7 +4756,7 @@ export class ClaudeAcpAgent {
|
||||
}
|
||||
}
|
||||
await compaction.interrupt();
|
||||
await settleActive(turnOutcome(session, "cancelled"));
|
||||
await settleActive(cancelledOutcome(session, session.activeTurn));
|
||||
// The cancelled turn's result may never come (that's why the
|
||||
// backstop fired) — close its delivery stretch here so partial
|
||||
// streamed text can't suppress the next turn's issue-#453 fallback.
|
||||
@@ -4821,7 +4821,7 @@ export class ClaudeAcpAgent {
|
||||
);
|
||||
await settleActive(
|
||||
session.cancelled
|
||||
? turnOutcome(session, "cancelled")
|
||||
? cancelledOutcome(session, inFlight)
|
||||
: (inFlight?.deferredSettle ?? turnOutcome(session, stopReason)),
|
||||
);
|
||||
// Queued turns the SDK never started never ran, so reject them rather
|
||||
@@ -5146,7 +5146,13 @@ export class ClaudeAcpAgent {
|
||||
// the interrupted turn's tokens entirely (issue #844). Zero
|
||||
// when the cancel pre-empted the result (wedge/force-cancel).
|
||||
if (session.cancelled && session.activeTurn && !session.activeTurn.settled) {
|
||||
await settleActive(turnOutcome(session, "cancelled"));
|
||||
// A held turn's result passed long ago; this idle is the
|
||||
// trailer of the followup cycle the cancel interrupted,
|
||||
// whose result (an autonomous one) recorded it as owed.
|
||||
if (isHeldOpen(session.activeTurn) && session.owedTrailingIdles > 0) {
|
||||
session.owedTrailingIdles--;
|
||||
}
|
||||
await settleActive(cancelledOutcome(session, session.activeTurn));
|
||||
// An interrupt can pre-empt the turn's result entirely
|
||||
// (nothing ran the result-case `finally`), so close the
|
||||
// delivery stretch here: idle is the SDK's authoritative
|
||||
@@ -6560,7 +6566,7 @@ export class ClaudeAcpAgent {
|
||||
session.owedTrailingIdles++;
|
||||
// Before activateTurn resets the accumulator, so the
|
||||
// usage still belongs to the cancelled turn.
|
||||
await settleActive(turnOutcome(session, "cancelled"));
|
||||
await settleActive(cancelledOutcome(session, session.activeTurn));
|
||||
} else if (isHeldOpen(session.activeTurn)) {
|
||||
// A turn held open for its background subagents (see
|
||||
// Turn.deferredSettle) hands off with the real outcome
|
||||
@@ -7257,15 +7263,26 @@ export class ClaudeAcpAgent {
|
||||
|
||||
// A deferred active turn (see Turn.deferredSettle) already has its result
|
||||
// and is only held open for subagents. A settling turn likewise has its
|
||||
// result but is awaiting the bounded checkpoint preview. Settle either one
|
||||
// "cancelled" NOW: the consumer cannot process the interrupt's trailing
|
||||
// idle while blocked in that preview, and a held turn may already be idle.
|
||||
// The captured outcome preserves usage and metadata (issue #844), while
|
||||
// reporter state makes a late checkpoint response harmless.
|
||||
// result but is awaiting the bounded checkpoint preview. Settle it
|
||||
// "cancelled" NOW where the consumer cannot: it cannot process the
|
||||
// interrupt's trailing idle while blocked in that preview, and a held turn
|
||||
// whose session sits idle gets no trailing idle (the interrupt emits
|
||||
// nothing; with no state events consumed yet, the same is assumed).
|
||||
// A held turn whose followup cycle is live (running, or blocked on a
|
||||
// permission request) is left to that idle, like any live turn: the
|
||||
// interrupt still flushes the cycle's output, which must reach the client
|
||||
// before the turn ends cancelled. The captured outcome preserves usage and
|
||||
// metadata (issue #844), while reporter state makes a late checkpoint
|
||||
// response harmless.
|
||||
{
|
||||
const active = session.activeTurn;
|
||||
const pendingOutcome = active?.deferredSettle ?? active?.settlingOutcome;
|
||||
if (active && pendingOutcome && (isHeldOpen(active) || active.settling)) {
|
||||
const followupLive =
|
||||
session.lastSessionState === "running" || session.lastSessionState === "requires_action";
|
||||
if (
|
||||
active &&
|
||||
(active.settling || (isHeldOpen(active) && !followupLive)) &&
|
||||
(active.deferredSettle ?? active.settlingOutcome)
|
||||
) {
|
||||
session.fileChangeReporter?.finish(active.fileChangeReport, "cancelled");
|
||||
active.settled = true;
|
||||
// Mirror settleActive's invariants (it is consumer-scoped and
|
||||
@@ -7279,17 +7296,16 @@ export class ClaudeAcpAgent {
|
||||
// text since the last boundary was its followups', and left latched
|
||||
// it would suppress a following replayed turn's issue-#453 fallback.
|
||||
session.emittedAssistantText = false;
|
||||
// When the interrupt below pre-empts a live cycle — running, or
|
||||
// blocked on a permission request (requires_action, the #866 shape
|
||||
// users cancel out of) — it produces a trailer idle with no counted
|
||||
// result; with the hold's own trailer typically already absorbed,
|
||||
// that idle would be un-owed and could lag past the next prompt's
|
||||
// echo — read as the fresh turn ending without a result (issue #825
|
||||
// false-fail). Pre-count it unless the session sits idle: there the
|
||||
// interrupt emits nothing, and a debt that never drains would mask
|
||||
// one future #825 detection. (lastSessionState is last-CONSUMED, so
|
||||
// both stale reads exist and both are accepted one-cycle windows: a
|
||||
// running transition still in the backlog reads as stale idle and
|
||||
// When the interrupt below pre-empts a live cycle (a settling turn's,
|
||||
// or one no state event reported), it produces a trailer idle with no
|
||||
// counted result; that idle would be un-owed and could lag past the
|
||||
// next prompt's echo — read as the fresh turn ending without a result
|
||||
// (issue #825 false-fail).
|
||||
// Pre-count it unless the session sits idle: there the interrupt
|
||||
// emits nothing, and a debt that never drains would mask one future
|
||||
// #825 detection. (lastSessionState is last-CONSUMED, so both stale
|
||||
// reads exist and both are accepted one-cycle windows: a running
|
||||
// transition still in the backlog reads as stale idle and
|
||||
// under-counts — that false-fail additionally needs the trailer to
|
||||
// lag past the next echo — while a cycle already completed into the
|
||||
// backlog reads as stale non-idle and over-counts, masking one
|
||||
@@ -7299,14 +7315,7 @@ export class ClaudeAcpAgent {
|
||||
if (session.lastSessionState !== "idle") {
|
||||
session.owedTrailingIdles++;
|
||||
}
|
||||
// Carries the held outcome's `_meta` too: a deferred outcome's only
|
||||
// metadata is its quota breakdown, the counterpart of the usage taken
|
||||
// from it here.
|
||||
active.resolve({
|
||||
stopReason: "cancelled",
|
||||
usage: pendingOutcome.usage,
|
||||
...(pendingOutcome._meta ? { _meta: pendingOutcome._meta } : {}),
|
||||
});
|
||||
active.resolve(cancelledOutcome(session, active));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -9907,6 +9916,20 @@ function turnOutcome(
|
||||
};
|
||||
}
|
||||
|
||||
/** The outcome of `turn` when a cancel ends it. A turn that already has its
|
||||
* result, because it is held open for its background subagents (see
|
||||
* Turn.deferredSettle) or is settling (see Turn.settlingOutcome), keeps the
|
||||
* usage and metadata that result recorded (issue #844). */
|
||||
function cancelledOutcome(session: Session, turn: Turn | null | undefined): TurnOutcome {
|
||||
const recorded = turn?.deferredSettle ?? turn?.settlingOutcome;
|
||||
if (!recorded) return turnOutcome(session, "cancelled");
|
||||
return {
|
||||
stopReason: "cancelled",
|
||||
usage: recorded.usage,
|
||||
...(recorded._meta ? { _meta: recorded._meta } : {}),
|
||||
};
|
||||
}
|
||||
|
||||
/** `_meta.quota` for a prompt response: what the turn spent, shaped like
|
||||
* codex-acp's (snake_case container keys, camelCase counters) so a client
|
||||
* reads one shape from either agent.
|
||||
|
||||
@@ -17705,13 +17705,21 @@ describe("deferred settlement for live background subagents (issues #864/#866)",
|
||||
});
|
||||
await waitFor(() => !!agent.sessions["test-session"]?.activeTurn?.deferredSettle);
|
||||
await agent.cancel({ sessionId: "test-session" });
|
||||
await expect(first).resolves.toEqual(expect.objectContaining({ stopReason: "cancelled" }));
|
||||
// The followup is live, so the turn ends at the interrupt's trailer, or,
|
||||
// as here where that trailer lags, at the next prompt's echo.
|
||||
releaseAfterCancel();
|
||||
|
||||
const second = await agent.prompt({
|
||||
sessionId: "test-session",
|
||||
prompt: [{ type: "text", text: "next" }],
|
||||
});
|
||||
// With the usage its own result recorded (issue #844).
|
||||
await expect(first).resolves.toEqual(
|
||||
expect.objectContaining({
|
||||
stopReason: "cancelled",
|
||||
usage: expect.objectContaining({ totalTokens: 15 }),
|
||||
}),
|
||||
);
|
||||
expect(second.stopReason).toBe("end_turn");
|
||||
await agent.sessions["test-session"]?.consumer;
|
||||
});
|
||||
@@ -17998,13 +18006,15 @@ describe("deferred settlement for live background subagents (issues #864/#866)",
|
||||
});
|
||||
await waitFor(() => !!agent.sessions["test-session"]?.activeTurn?.deferredSettle);
|
||||
await agent.cancel({ sessionId: "test-session" });
|
||||
await expect(first).resolves.toEqual(expect.objectContaining({ stopReason: "cancelled" }));
|
||||
// A live cycle, so the turn ends at the interrupt's trailer, or here, where
|
||||
// that trailer lags, at the next prompt's echo.
|
||||
releaseAfterCancel();
|
||||
|
||||
const second = await agent.prompt({
|
||||
sessionId: "test-session",
|
||||
prompt: [{ type: "text", text: "next" }],
|
||||
});
|
||||
await expect(first).resolves.toEqual(expect.objectContaining({ stopReason: "cancelled" }));
|
||||
expect(second.stopReason).toBe("end_turn");
|
||||
await agent.sessions["test-session"]?.consumer;
|
||||
});
|
||||
|
||||
@@ -513,6 +513,86 @@ describe("turn events", () => {
|
||||
]);
|
||||
});
|
||||
|
||||
it("ends a held turn cancelled mid-followup after the output its interrupt flushes", async () => {
|
||||
const trace: string[] = [];
|
||||
onSessionUpdate = ({ update }) => {
|
||||
if (
|
||||
update.sessionUpdate === "agent_message_chunk" &&
|
||||
update.content.type === "text" &&
|
||||
update.messageId?.startsWith("msg_")
|
||||
) {
|
||||
trace.push(`chunk ${update.content.text}`);
|
||||
}
|
||||
};
|
||||
const interrupted = Promise.withResolvers<void>();
|
||||
const controls = {
|
||||
interrupt: async () => {
|
||||
trace.push("interrupt");
|
||||
interrupted.resolve();
|
||||
},
|
||||
};
|
||||
scriptQuery(async function* (input, options) {
|
||||
const sessionId = options.sessionId!;
|
||||
const idle = () => system("session_state_changed", sessionId, { state: "idle" });
|
||||
const running = () => system("session_state_changed", sessionId, { state: "running" });
|
||||
const first = (await input.next()).value;
|
||||
yield echo(first, sessionId);
|
||||
yield running();
|
||||
yield system("task_started", sessionId, {
|
||||
task_id: "agent-1",
|
||||
tool_use_id: "toolu_agent-1",
|
||||
description: "Explore the project",
|
||||
subagent_type: "Explore",
|
||||
});
|
||||
yield result(sessionId); // held for the subagent
|
||||
yield idle();
|
||||
yield system("task_notification", sessionId, {
|
||||
task_id: "agent-1",
|
||||
tool_use_id: "toolu_agent-1",
|
||||
status: "completed",
|
||||
output_file: "",
|
||||
summary: "done",
|
||||
});
|
||||
// The followup cycle writes the promised summary when the cancel comes.
|
||||
yield running();
|
||||
yield* streamedText("msg_followup", "Summary so far", sessionId);
|
||||
await interrupted.promise;
|
||||
yield stream(
|
||||
{ type: "content_block_delta", index: 0, delta: { type: "text_delta", text: ", stopped" } },
|
||||
sessionId,
|
||||
);
|
||||
yield { ...result(sessionId), origin: { kind: "task-notification" } };
|
||||
yield idle();
|
||||
// A wedged next turn: an idle without a result is the issue #825 signal,
|
||||
// which a leaked trailer debt would absorb.
|
||||
const second = (await input.next()).value;
|
||||
yield echo(second, sessionId);
|
||||
yield idle();
|
||||
yield result(sessionId);
|
||||
await input.next();
|
||||
}, controls);
|
||||
const sessionId = await newSession();
|
||||
const first = recordEvents(trace, "first");
|
||||
const second = recordEvents(trace, "second");
|
||||
|
||||
await agent.startTurn(prompt(sessionId, "explore"), first.events);
|
||||
await vi.waitFor(() => expect(trace).toContain("chunk Summary so far"));
|
||||
await agent.cancel({ sessionId });
|
||||
await first.done;
|
||||
await agent.startTurn(prompt(sessionId, "next"), second.events);
|
||||
await second.done;
|
||||
|
||||
expect(trace).toEqual([
|
||||
expect.stringMatching(/^first inserted /),
|
||||
"chunk Summary so far",
|
||||
"interrupt",
|
||||
"chunk , stopped",
|
||||
"first ended cancelled",
|
||||
expect.stringMatching(/^second inserted /),
|
||||
expect.stringMatching(/^second failed /),
|
||||
]);
|
||||
});
|
||||
|
||||
it("does not insert a turn that a cancel ended while the previous turn was handed off", async () => {
|
||||
scriptTurns([
|
||||
async function* (options) {
|
||||
|
||||
Reference in New Issue
Block a user