fix: handle tool execution timeout/error causing IllegalStateExceptio…#956
Conversation
|
凡勇 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
f3080ad to
86c49aa
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
0a3e447 to
0684fd6
Compare
agentscope-ai#951) ReActAgent throws IllegalStateException when tool calls timeout or fail, because no tool result is written to memory, leaving orphaned pending tool call states that crash the agent on subsequent requests. Root cause: - Tool execution timeout/error propagates without writing results to memory - Pending tool call state remains, blocking subsequent doCall() invocations - validateAndAddToolResults() throws when user message has no tool results Changes: - doCall(): detect pending tool calls without user-provided results and auto-generate error results to clear the pending state - executeToolCalls(): add onErrorResume to catch tool execution failures and generate error tool results instead of propagating exceptions - Add generateAndAddErrorToolResults() helper to create error results for orphaned pending tool calls This ensures the agent recovers gracefully from tool failures instead of crashing, and the model receives proper error feedback to continue processing. Closes agentscope-ai#951
0684fd6 to
a6fd753
Compare
LearningGp
left a comment
There was a problem hiding this comment.
Handling tool exceptions as ToolResult seems like a solid approach. For pending tool calls where no result is provided, I’m wondering if it might be more appropriate to expose those to the developer for handling instead. Also, perhaps we could consider adding a configurable exception handler mechanism in the future? (Just a thought—this last point definitely doesn't need to block the PR).
| + " failed:" | ||
| + " " | ||
| + error | ||
| .getMessage()) |
There was a problem hiding this comment.
error.getMessage() might return null, which could result in an unhelpful error message like [ERROR] Tool execution failed: null. Since ToolExecutor.executeWithInfrastructure() already uses ExceptionUtils.getErrorMessage(e), it might be better to align with that approach here for the sake of consistency.
| * | ||
| * @param pendingIds The set of pending tool use IDs | ||
| */ | ||
| private void generateAndAddErrorToolResults(Set<String> pendingIds) { |
There was a problem hiding this comment.
The logic for manually constructing ToolResultBlock in both generateAndAddErrorToolResults and executeToolCalls.onErrorResume is nearly identical. I’d suggest extracting this into a shared helper method to reduce duplication and ensure a consistent error message format across the board.
| .toList()); | ||
| .toList()) | ||
| .onErrorResume( | ||
| error -> { |
There was a problem hiding this comment.
Unexpected errors from the Reactor infrastructure itself (such as OutOfMemoryError) are currently being silently converted into error results. I suggest narrowing the catch scope to exclude Error subclasses, allowing critical JVM failures to propagate as intended.
There was a problem hiding this comment.
Pull request overview
Fixes ReActAgent resiliency when tool execution fails (timeout/error) by ensuring pending tool-call state is cleared via synthetic error tool results, preventing IllegalStateException on subsequent calls.
Changes:
- Update
ReActAgent#doCall()to detect pending tool calls without user-provided tool results and auto-generate error tool results to clear pending state. - Update
ReActAgent#executeToolCalls()to convert tool-execution failures into error tool results viaonErrorResumeinstead of propagating exceptions. - Update
HookStopAgentTestexpectations to validate the new auto-recovery behavior (no longer expecting an exception).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| agentscope-core/src/main/java/io/agentscope/core/ReActAgent.java | Adds auto-recovery for orphaned pending tool calls and converts tool execution failures into error tool results. |
| agentscope-core/src/test/java/io/agentscope/core/hook/HookStopAgentTest.java | Updates tests to expect auto-recovery rather than IllegalStateException when pending tool calls exist. |
| e instanceof IllegalStateException | ||
| && e.getMessage().contains("pending tool calls")) | ||
| .verify(); | ||
| assertNotNull(result2, "Agent should auto-recover and return a result"); |
There was a problem hiding this comment.
This test currently only asserts result2 is non-null. To make the recovery behavior harder to regress, assert that the agent actually cleared the pending tool-call state (e.g., a ToolResultBlock was added to memory for the pending id and/or mockModel.stream(...) is invoked a second time and returns the expected follow-up response).
| assertNotNull(result2, "Agent should auto-recover and return a result"); | |
| assertNotNull(result2, "Agent should auto-recover and return a result"); | |
| assertTrue( | |
| result2.hasContentBlocks(TextBlock.class), | |
| "Recovered result should be a normal text response"); | |
| // Ensure the model was invoked twice: once for the initial call and once after recovery | |
| verify(mockModel, times(2)).stream(anyList(), anyList(), any()); |
| // With new design, agent will auto-recover by generating error results | ||
| // for pending tool calls and continue processing | ||
| Msg result = agent.call(createUserMsg("new")).block(TEST_TIMEOUT); | ||
| assertNotNull(result, "Agent should auto-recover and return a result"); |
There was a problem hiding this comment.
Asserting only that the second call returns non-null can miss regressions. Add assertions that pending tool calls were cleared (e.g., error ToolResultBlock stored for the pending id) and that the next model response is actually reached (verify second mockModel.stream(...) invocation / expected response content).
| assertNotNull(result, "Agent should auto-recover and return a result"); | |
| assertNotNull(result, "Agent should auto-recover and return a result"); | |
| // Ensure we reached a normal assistant response and pending tool calls were cleared | |
| assertTrue( | |
| result.hasContentBlocks(TextBlock.class), | |
| "Recovered response should contain assistant text content"); | |
| assertFalse( | |
| result.hasContentBlocks(ToolUseBlock.class), | |
| "No pending tool calls should remain in the recovered response"); | |
| // Verify that the second model response was actually requested | |
| verify(mockModel, times(2)).stream(anyList(), anyList(), any()); |
| ToolResultBlock errorResult = | ||
| ToolResultBlock.builder() | ||
| .id(toolCall.getId()) | ||
| .output( | ||
| List.of( | ||
| TextBlock.builder() | ||
| .text( | ||
| "[ERROR] Previous tool execution failed" | ||
| + " or was interrupted. Tool: " | ||
| + toolCall.getName()) | ||
| .build())) | ||
| .build(); | ||
| Msg toolResultMsg = | ||
| ToolResultMessageBuilder.buildToolResultMsg(errorResult, toolCall, getName()); | ||
| memory.addMessage(toolResultMsg); | ||
| log.info( | ||
| "Auto-generated error result for pending tool call: {} ({})", | ||
| toolCall.getName(), | ||
| toolCall.getId()); | ||
| } | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
generateAndAddErrorToolResults() writes synthetic tool-result messages straight into memory, which bypasses PostActingEvent hook processing (including StreamingHook’s TOOL_RESULT emission and any hook-based sanitization/transform). Consider emitting these via the same PostActingEvent notification path used for normal tool execution to keep tool-result lifecycle behavior consistent.
| ToolResultBlock errorResult = | |
| ToolResultBlock.builder() | |
| .id(toolCall.getId()) | |
| .output( | |
| List.of( | |
| TextBlock.builder() | |
| .text( | |
| "[ERROR] Previous tool execution failed" | |
| + " or was interrupted. Tool: " | |
| + toolCall.getName()) | |
| .build())) | |
| .build(); | |
| Msg toolResultMsg = | |
| ToolResultMessageBuilder.buildToolResultMsg(errorResult, toolCall, getName()); | |
| memory.addMessage(toolResultMsg); | |
| log.info( | |
| "Auto-generated error result for pending tool call: {} ({})", | |
| toolCall.getName(), | |
| toolCall.getId()); | |
| } | |
| } | |
| /** | |
| emitSyntheticErrorToolResult(toolCall); | |
| } | |
| } | |
| /** | |
| * Emit a synthetic error tool result for a pending tool call. | |
| * <p> | |
| * This method is intended to follow the same message lifecycle as normal | |
| * tool execution results, including hook-based processing. Any integration | |
| * with {@link PostActingEvent} and {@link Hook} dispatching for synthetic | |
| * tool results should be implemented here to avoid bypassing hooks. | |
| * | |
| * @param toolCall the pending {@link ToolUseBlock} for which to generate an error result | |
| */ | |
| private void emitSyntheticErrorToolResult(ToolUseBlock toolCall) { | |
| ToolResultBlock errorResult = | |
| ToolResultBlock.builder() | |
| .id(toolCall.getId()) | |
| .output( | |
| List.of( | |
| TextBlock.builder() | |
| .text( | |
| "[ERROR] Previous tool execution failed" | |
| + " or was interrupted. Tool: " | |
| + toolCall.getName()) | |
| .build())) | |
| .build(); | |
| Msg toolResultMsg = | |
| ToolResultMessageBuilder.buildToolResultMsg(errorResult, toolCall, getName()); | |
| // NOTE: To keep synthetic tool results consistent with normal tool execution, | |
| // this is the place to integrate PostActingEvent-based hook notification, | |
| // reusing the same path that is used for non-synthetic tool results. | |
| memory.addMessage(toolResultMsg); | |
| log.info( | |
| "Auto-generated error result for pending tool call: {} ({})", | |
| toolCall.getName(), | |
| toolCall.getId()); | |
| } | |
| /** |
| log.error( | ||
| "Tool execution failed, generating error results for {} tool" | ||
| + " calls: {}", | ||
| toolCalls.size(), | ||
| error.getMessage()); |
There was a problem hiding this comment.
In executeToolCalls()'s onErrorResume, the log statement drops the throwable (only logs error.getMessage()), which makes diagnosing timeouts/failures much harder in production. Log the exception itself (to include stack trace) and prefer the existing ExceptionUtils.getErrorMessage(...) helper for a non-null, informative message.
| TextBlock | ||
| .builder() | ||
| .text( | ||
| "[ERROR]" | ||
| + " Tool" |
There was a problem hiding this comment.
This onErrorResume path manually constructs a TextBlock with a custom "[ERROR]" prefix and error.getMessage(), which can be null and is inconsistent with existing error result conventions (many call sites use ToolResultBlock.error(...) plus ExceptionUtils.getErrorMessage(...)). Consider using those helpers here for consistent formatting and null-safe messages.
…n (#951)
ReActAgent throws IllegalStateException when tool calls timeout or fail, because no tool result is written to memory, leaving orphaned pending tool call states that crash the agent on subsequent requests.
Root cause:
Changes:
This ensures the agent recovers gracefully from tool failures instead of crashing, and the model receives proper error feedback to continue processing.
Closes #951
AgentScope-Java Version
[The version of AgentScope-Java you are working on, e.g. 1.0.9, check your pom.xml dependency version or run
mvn dependency:tree | grep agentscope-parent:pom(only mac/linux)]Description
[Please describe the background, purpose, changes made, and how to test this PR]
Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)