[runtime] Do not retry or persist interrupted tool calls on cancellation - #1111
Open
Ashfaqbs wants to merge 1 commit into
Open
[runtime] Do not retry or persist interrupted tool calls on cancellation#1111Ashfaqbs wants to merge 1 commit into
Ashfaqbs wants to merge 1 commit into
Conversation
ToolCallAction.executeSequentially/executeParallel caught InterruptedException the same as any ordinary tool failure, recorded it as a tool error, and let the loop continue: processToolRequest still sent a ToolResponseEvent afterward, driving another chat call and letting ActionExecutionOperator persist the action as completed. On recovery, isCompleted() then skipped re-execution and replayed the stale output instead of re-running the interrupted call - the same failure mode apache#1071 closed for the chat-call path, reached here through the tool-call path apache#1071 didn't cover. executeParallel's batch path had the same gap one layer down too: RunnerContextImpl.durableExecuteAllAsync's per-callable catch (Exception e) folded an interruption into Outcome.failure and kept scheduling the rest of the batch, even though durableExecute itself (fixed in apache#1071) already rethrows InterruptedException correctly - this outer catch was just re-swallowing it before it could propagate. Fixed both the same way apache#1071 fixed the chat-call path: restore the interrupt flag and rethrow immediately, so the loop stops, no ToolResponseEvent goes out, and the action is never persisted as completed off the back of a cancelled call. Fixes apache#1088 Generated-by: Claude Code 2.1.226 (Claude Sonnet 5)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked issue: #1088
Purpose of change
ToolCallAction.executeSequentially/executeParallelcaughtInterruptedExceptionthe same as any ordinary tool failure, recorded it as a tool error, and let the loop continue —processToolRequeststill sent aToolResponseEventafterward, driving another chat call and lettingActionExecutionOperatorpersist the action as completed. On recovery,isCompleted()then skipped re-execution and replayed the stale output instead of re-running the interrupted call — the same failure mode #1071 closed for the chat-call path, reached here through the tool-call path #1071 didn't cover (out of that PR's scope, filed separately per the discussion on #1071).executeParallel's batch path had the same gap one layer down too:RunnerContextImpl.durableExecuteAllAsync's per-callablecatch (Exception e)folded an interruption intoOutcome.failureand kept scheduling the rest of the batch, even thoughdurableExecuteitself (fixed in #1071) already rethrowsInterruptedExceptioncorrectly — this outer catch was just re-swallowing it before it could propagate.Behavioral Semantics
Fixed both the same way #1071 fixed the chat-call path: an explicit
catch (InterruptedException e)ahead of the generic failure handling restores the interrupt flag and rethrows immediately, in three places:RunnerContextImpl.durableExecuteAllAsync— stops scheduling the remaining callables in the batch and propagates, instead of recording a failedOutcomeand continuing.ToolCallAction.executeParallel— propagates instead of recording every execution in the batch as a tool error.ToolCallAction.executeSequentially— propagates instead of recording the current execution as a tool error and moving on to the next one in the loop.processToolRequestnow declaresthrows InterruptedExceptionand lets it propagate, sosendEventfor theToolResponseEventis skipped entirely on a cancellation — no further chat call gets driven off a cancelled tool call, and the action is never persisted as completed on the back of it. Ordinary tool failures (missing tool, execution exceptions, infrastructure errors) are unaffected — they still go through the existing recording/reporting paths untouched.Tests
ToolCallActionTest: an interrupted single (sequential) call propagates and sends noToolResponseEvent; a second sequential tool call does not run once the first is interrupted; an interrupted parallel batch propagates and sends noToolResponseEvent. All three assert the interrupt status is restored on the thread.RunnerContextImplDurableExecuteTest: in a 3-callable batch where the 2nd is interrupted, the 1st (already-succeeded) callable's result is correctly persisted, the 2nd's interruption is not recorded as a failed outcome, and the 3rd never runs.ToolCallActionTest: 19/19 passing (16 pre-existing + 3 new).RunnerContextImplDurableExecuteTest: 14/14 passing (13 pre-existing + 1 new).planmodule suite: 313/316 passing; the 3 failures are the pre-existingBashToolTestcases (require a working WSL bash on this Windows environment, fail identically onmainbefore this diff — confirmed viagit stash).runtimemodule suite: 847/854 passing; all 14 failures are pre-existing, environment-specific (Fluss coordinator server not starting, Windows path/file-locking issues in unrelated skill-loading and Python-environment tests) — none inToolCallActionTestorRunnerContextImplDurableExecuteTest.spotless:checkclean on both modules.API
No public API changes.
ToolCallAction.processToolRequest/executeSequentially/executeParallelnow declarethrows InterruptedException, but these are internal, reflectively-invoked action handlers (JavaFunction), not part of the public surface.Documentation
doc-neededdoc-not-neededdoc-includedWas this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code 2.1.226 (Claude Sonnet 5)