[plan][runtime] Do not retry or persist interrupted chat calls on cancellation

[Ashfaqbs]

Fixes #1070.

Problem

When a job cancellation interrupts an in-flight chat-model call, the resulting InterruptedException was handled the same as any other model failure:

• ChatModelInvoker.chatWithRetries() caught it with the generic catch (Exception e) block and, under ERROR_HANDLING_STRATEGY=RETRY, could apply retry backoff and issue another model call after cancellation had already been requested — delaying task shutdown and firing unnecessary external requests.
• RunnerContextImpl (durableExecuteCompletionOnly and executeAndFinalizeCurrentCall) recorded the interruption as a completed durable success/failure before rethrowing it. With an ActionStateStore configured, that meant a stale interruption from a cancelled attempt could be replayed as a terminal result after recovery instead of the call being re-executed.

Fix

Added an InterruptedException-specific catch ahead of the generic failure handling in both places:

• ChatModelInvoker.chatWithRetries() now restores the thread's interrupt status and rethrows immediately, skipping the retry/backoff branch entirely, regardless of FAIL, RETRY, or IGNORE.
• RunnerContextImpl.durableExecuteCompletionOnly() and executeAndFinalizeCurrentCall() now restore the interrupt status and rethrow without calling recordDurableCompletion/finalizeCurrentCall, so the durable slot is left unfinished (or pending) for recovery to re-execute or reconcile instead of replaying a stale interruption.

Ordinary provider/network failures are unaffected — they still go through the existing retry and durable-completion paths untouched.

Testing

• Added ChatModelInvokerTest (new file) covering: an interrupted call is not retried (durableExecute invoked exactly once) and the interrupt status is restored; an ordinary RuntimeException still consumes the full retry budget, to confirm normal retry behavior is unchanged.
• Added two cases to RunnerContextImplDurableExecuteTest covering the legacy completion-only path and the pending-slot resume path: an interruption is not persisted as a durable result and the interrupt status is restored.
• Verified RED before the fix (git stash on the two source files, keeping the new tests) — all four new/updated cases failed as expected, then GREEN after restoring the fix.
• Full plan module suite: 288/290 passing; the 2 failures are pre-existing BashToolTest cases unrelated to this change (they require a working WSL bash on this Windows environment and fail the same way on main before this diff).
• Full runtime module suite: passing.
• spotless:check clean on both modules.

Generated-by: Claude Code 2.1.226 (Claude Opus 4.6)

---

This fix follows from my own diagnosis of the bug. I used AI tooling for exploration and testing, directing it toward the change I had in mind.

[weiqingy]

Thanks for taking this on. A few questions inline.

[weiqingy]

Thread.sleep on line 203 sits inside this catch (Exception e) block, so a cancel during the backoff wait throws from in here rather than from the call above. A catch block is not covered by its own sibling catch, and Thread.sleep clears the interrupt status when it throws. The call still stops, so this is only about the flag, but on this one path it ends up cleared rather than restored. RETRY_WAIT_INTERVAL defaults to 1, so under RETRY there is a one second window on every retry.

Something like this, if useful:

try {
    Thread.sleep(currentWaitSec * 1000L);
} catch (InterruptedException ie) {
    Thread.currentThread().interrupt();
    throw ie;
}

Worth restoring the flag there too?

[Ashfaqbs]

Confirmed, good catch — the sleep sits in the sibling catch (Exception e) block, not the InterruptedException handler above it, so it wasn't covered by that fix. Wrapped it the same way (restore flag, rethrow) in c30ba89.

[weiqingy]

The new block has no test. Both tests pass retryWaitIntervalSec = 0 (lines 80 and 112), so the guard on line 202 skips lines 203 to 208.

A cheap deterministic test, in case it helps. If the stub sets the flag before it throws, Thread.sleep throws at once and costs no wall time:

when(ctx.durableExecute(any()))
        .thenAnswer(
                inv -> {
                    Thread.currentThread().interrupt();
                    throw new RuntimeException("transient failure");
                });

with RETRY, numRetries = 1, retryWaitIntervalSec = 1.

One catch. assertThrows and the times(1) verify pass on the old code too, so only assertTrue(Thread.interrupted()) proves the fix. Is a test here worth adding?

[Ashfaqbs]

Added, in 5803bb1. Set the interrupt flag from within the mocked durableExecute call itself so Thread.sleep throws immediately at no wall-clock cost, with RETRY / numRetries=1 / retryWaitIntervalSec=1 as you suggested. Verified it's the flag assertion alone that distinguishes the fix — temporarily restored the pre-c30ba894 sleep block and confirmed this new test fails there (times(1)/InterruptedException-thrown still pass either way) while the two existing tests stay green.

[weiqingy]

nit: Thread.interrupted() on this line clears the flag, but it only runs if the verify above it passes. If that verify ever fails, the flag stays set on the JUnit thread. ChatModelActionRetryTest.chatRetriesWithExponentialBackoff is in this same package and drives a real one second backoff, so it can then fail with an unrelated InterruptedException and send someone chasing the wrong test. RunnerContextImplDurableExecuteTest:91 and :115 have the same shape. Would an @AfterEach calling Thread.interrupted() be worth adding, so the cleanup runs either way?

[Ashfaqbs]

Confirmed — assertTrue only consumes the flag when it passes, so a failing assertion here would leave it set on the JUnit thread. Added an @AfterEach in both this test class and RunnerContextImplDurableExecuteTest to clear it unconditionally, in c30ba89.

[weiqingy]

ModelRoutingResolver.java:97 runs the routing strategy through ctx.durableExecute, so an interrupt there reaches this rethrow. ChatModelAction.java:609 then catches it, and under IGNORE line 621 returns normally. I could not trigger this with in-tree code. It needs a MODEL_ROUTER resource, and the only routing strategy in the repo today does no I/O. But RoutingStrategy is a user extension point, and the comment on line 612 already expects strategies that do I/O. The issue's first bullet asks for cancellation to propagate under IGNORE too. Should that catch let InterruptedException through before the IGNORE check?

[Ashfaqbs]

Traced it through — matches what you described. It's real but needs a RoutingStrategy that does I/O, which nothing in-tree does today, so it's about the extension point rather than this PR's own code. Given the issue is scoped to the chat/tool call paths, I'd rather keep this PR to what it already touches and file a follow-up for the routing-resolver + IGNORE interaction rather than pull it in here — let me know if you'd rather it go in this PR instead.

[weiqingy]

Agreed, leave it out of this PR. I would skip the follow-up issue too. #1042 already has the same fix. It adds ModelRoutingResolver.isCancellation(...) and calls it in ChatModelAction.processChatRequest just before the IGNORE check.

#1042 is also what makes this path reachable. Strategies.llm(...) runs a judge chat call through chatWithRetries, so the strategy that does I/O arrives with its own guard.

It is still open though, so this only holds if it lands as it stands. Does that look right to you?

[Ashfaqbs]

That reasoning matches what I see in #1042: ModelRoutingResolver.isCancellation(...) gets added and called from ChatModelAction.processChatRequest right before the IGNORE check, and Strategies.llm(...)'s judge chat call through chatWithRetries is what would make this path reachable in the first place. Agreed on skipping a separate follow-up issue, conditional on #1042 landing as-is — I'll keep an eye on it and flag here (or file the issue myself) if it merges in a materially different shape.

[weiqingy]

Tool calls run through the two methods you patched, so this rethrow reaches them too, but the tool path still finishes normally after a cancel. ToolCallAction.java:256 catches the InterruptedException, line 257 records it as a tool error, and the loop moves on to the next tool. Line 85 then sends the ToolResponseEvent anyway, which drives another chat call, and the action is persisted as finished. executeParallel has the same shape at line 209. That catch predates this PR. Is the tool path meant to be in scope here, or is it worth a separate issue?

[Ashfaqbs]

Confirmed, same shape in both executeSequentially and executeParallel, and it predates this PR. Tool-call cancellation handling looks like its own piece of work (the response still gets sent and the action still finishes), not a natural extension of the chat-retry fix here. I'd lean toward a separate issue/PR for it rather than scope-creeping this one — open to doing it here instead if you'd prefer to keep it together.

[weiqingy]

Agreed, a separate issue is right. Nothing open covers tool-call cancellation, so it will not duplicate anything.

One thing that might be worth adding to it. Because the action returns normally, ActionExecutionOperator persists it as completed (line 491), and on recovery line 437 replays the output events without running the call again. That is problem 2 in #1070, carried by the action state rather than the durable slot. The chat path escapes it because the raw InterruptedException is rethrown on line 482, before the persist. Worth folding in?

[Ashfaqbs]

Good catch, worth folding in. Traced it: ActionExecutionOperator persists the action as completed at line ~491 (durableExecManager.maybePersistTaskResult) right after the tool-call catch swallows the interruption, and on recovery line ~437 (actionState.isCompleted()) skips re-execution and replays the stale output. Confirms it's the same class of problem as #1070's problem 2, just reached through the action-state path instead of the durable-slot path since the chat path's raw rethrow at line 482 escapes before the persist call. I'll fold this into the follow-up issue's description when I file it.

[Ashfaqbs]

Filed the tool-call cancellation follow-up: #1088, including the persistence angle from the review thread.