Conversation
…rror concurrent.futures.as_completed raises TimeoutError from its iterator, so the except clause guarding future.result() never handled a real timeout and the exception escaped agent.step() through the streaming path. Handle the iterator timeout and log the per-tool timeout for pending futures, matching the async tool execution branch.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| logger.info( | ||
| f"Function output: {tool_call_record.result}" | ||
| ) | ||
| except concurrent.futures.TimeoutError: |
There was a problem hiding this comment.
The outer handler is the right shape and it does stop the escape. One thing that came with the move: the inner except concurrent.futures.TimeoutError here survives, and on Python 3.11+ it is not dead code, it is reachable for the wrong reason.
concurrent.futures.TimeoutError became an alias of the builtin TimeoutError in 3.11, and TimeoutError is an OSError subclass, so a tool that hits its own network deadline lands in this branch instead of the generic one below it. I ran the handler structure from this diff (not the agent itself) at 05ad013 in clean containers, with a tool that raises TimeoutError("the tool's own network deadline"):
python:3.10-slim -> ERROR Error executing tool 'slow_tool': the tool's own network deadline
python:3.12-slim -> WARNING Function 'slow_tool' timed out after 30.0 seconds
pyproject is >=3.10,<3.15, so both of those ship. On 3.12 the warning names a budget the tool never exceeded, and the tool's own message is dropped.
Now that the outer handler owns the budget timeout, deleting this inner one and letting except Exception report the tool's error would keep that case accurate.
There was a problem hiding this comment.
The Python facts here check out, but the conclusion does not survive the real call path. I ran it through the agent rather than a copy of the handler, and a tool that raises TimeoutError is reported as the tool's error — the branch is never entered.
_execute_tool_from_stream_data — the callable submitted to the executor at chat_agent.py:5061 — wraps the whole tool invocation in except Exception as e: (:5185) and returns a record with {"error": error_msg}, with a second except Exception at :5242 returning None. So future.result() cannot raise for a tool failure. Only a BaseException (KeyboardInterrupt, SystemExit, asyncio.CancelledError) could escape, and TimeoutError is not one. The inner except concurrent.futures.TimeoutError at :5082 is therefore unreachable from a tool error, which is presumably why it has sat there as a catch for a future.result() call that is not given a timeout.
Confirmed by driving a streamed tool that raises TimeoutError("the tool's own network deadline") with tool_execution_timeout=30.0:
WARNING camel.camel.agents.chat_agent:chat_agent.py:5190 Error executing tool
'tool_with_own_deadline': Execution of function tool_with_own_deadline failed
with no arguments. Error: the tool's own network deadline
The tool's message survives and no timeout is claimed. af311904 adds that as a regression test so the reporting cannot start being swallowed unnoticed. I left :5082 alone rather than deleting it, since removing genuinely unreachable code is a separate change from this fix and would be a no-op diff here — happy to drop it if you would rather have it gone.
There was a problem hiding this comment.
You are right, and the way I checked could not have caught it: I ran the handler shape on its own rather than the executor path, so I never saw that future.result() cannot raise here.
Reading it at af311904: _execute_tools_sync_with_status_accumulator submits only _execute_tool_from_stream_data (camel/agents/chat_agent.py:5060), and that function's whole body is a single try with except Exception at :5185 and :5242 and no raise anywhere inside it. TimeoutError is an Exception, so a tool's own deadline is caught at :5185 and comes back as a record. The branch at :5082 is not reachable from a tool error, and the outer handler you added at :5092 is the one that fires. Sorry for the noise.
Separately, your reply above posted as the literal text @/tmp/reply_4337.md, so the body did not come through. I went by the commit message.
…rror A tool that hits its own network deadline raises TimeoutError, which is an alias of concurrent.futures.TimeoutError on Python 3.11+, so the executor loop's inner handler looks like it would blame the agent's tool budget and drop the tool's own message. It cannot: _execute_tool_from_stream_data catches every Exception it can and returns a record, so future.result() never raises and that branch is unreachable from a tool error. This drives a streamed tool that raises TimeoutError well inside tool_execution_timeout and pins the reporting, so the behaviour cannot regress silently.
2 participants
Related Issue
Closes #4336
Description
In streaming mode the sync path is the only one that mishandles
tool_execution_timeout: when a streamed tool call runs past the deadline, an uncaughtconcurrent.futures.TimeoutErrorpropagates out ofagent.step()instead of the per-tool warning that_execute_tools_async_with_status_accumulatoralready logs for the same input.Root cause —
camel/agents/chat_agent.py:5066(_execute_tools_sync_with_status_accumulator):The deadline is handed to
as_completed(), and CPython raisesTimeoutErrorfrom that iterator itself — i.e. at theforstatement, outside thetry. The handler at line 5081 therefore never runs: no warning is logged,future.cancel()is never called, noToolCallingRecordis produced, and the exception unwinds through_stream_response/_streamand out ofstep(). Passingtimeouttoas_completedstill matters, which is why the fix wraps the iteration rather than removing the deadline.For contrast, the async branch (
camel/agents/chat_agent.py:6056-6100) wraps each task inasyncio.wait_for(timeout=self.tool_execution_timeout), catchesasyncio.TimeoutError, logsFunction timed out after {N} secondsand continues — the intended behaviour for this option, which the constructor docstring atcamel/agents/chat_agent.py:441-442documents as a per-tool budget.Changes
camel/agents/chat_agent.py: wrap theas_completed(..., timeout=...)loop intry/except concurrent.futures.TimeoutError. On timeout, log the existing warning and callfuture.cancel()for each future still pending, matching the async branch. Completed futures are still drained and recorded as before.test/agents/test_chat_agent.py: addtest_chat_agent_stream_tool_timeout_is_reported_not_raised, an offline regression test (stub model yielding two pre-built chunks carrying oneslow_toolcall,tool_execution_timeout=0.2) asserting thatstep()completes, that the timeout warning is logged, and that no tool call is recorded.Testing
Reproduced first on unpatched
master(8c791b7b) with only the new test added:After the fix:
The 4 failures (
test_tool_calling_async,test_chat_agent_creation_methods,test_chat_agent_stream_with_structured_output,test_chat_agent_async_stream_with_structured_output) also fail on unpatchedmasterin this environment — they need a live backend / real API key. With the new test added, unpatchedmastergives5 failed, 34 passed, the patched tree gives4 failed, 35 passed; no existing test changes state.Lint and type check, using the versions the repo pins (
.pre-commit-config.yaml,pyproject.toml):The mypy count is byte-identical to unpatched
masterin this environment (missing optional deps such astree_sitter,jinja2,sklearn); neither changed file reports an error.What is the purpose of this pull request?
Checklist
pyproject.tomland runuv lock— no dependency changesAI assistance disclosure: this patch was drafted with AI assistance and reviewed by a human; the test output quoted above was produced by running the commands shown against this branch and unpatched
master.