Repository navigation
feat(tools): make managed-agent tool timeout configurable - #1956
kunwar-vikrant wants to merge 2 commits into
Conversation
chrikrah
left a comment
There was a problem hiding this comment.
@kunwar-vikrant the threading is right and the 150 s default holds. main moved under you first: 9c60a53 reworded the max_idle docstring in AsyncWork.worker, and GitHub has the branch CONFLICTING on that paragraph alone, so a rebase drops your tool_timeout: lines straight back in. One guard I would change before merge.
Head 2eb7b27, merge base 4421d56, Python 3.12.3, pytest 9.1.1:
# outside ./scripts/test, which wants the project's own lockfile, so -o addopts=
# replaces the repo default and keeps -p tests._alias_httpx
$ PYTHONPATH=$HEAD/src python -m pytest tests/lib/tools/test_session_runner.py \
tests/lib/environments/test_worker.py -q -o addopts="--tb=short -p tests._alias_httpx"
90 passed in 6.89s
# same two suites, lib/ and resources/ restored to 4421d56, your tests kept
$ PYTHONPATH=$BASE/src python -m pytest tests/lib/tools/test_session_runner.py \
tests/lib/environments/test_worker.py -q -k timeout -rf \
-o addopts="--tb=line -p tests._alias_httpx"
FFFFF. [100%]
=================================== FAILURES ===================================
E TypeError: SessionToolRunner.__init__() got an unexpected keyword argument 'tool_timeout'
[4 more identical E lines, plus 5 traceback lines at :290, :1397 and :1407, cut]
=========================== short test summary info ============================
FAILED tests/lib/tools/test_session_runner.py::test_tool_timeout - TypeError:...
FAILED tests/lib/tools/test_session_runner.py::test_tool_timeout_can_be_disabled
FAILED tests/lib/tools/test_session_runner.py::test_tool_timeout_rejects_nonpositive_values
FAILED tests/lib/tools/test_session_runner.py::test_tool_timeout_must_exceed_bash_default
FAILED tests/lib/tools/test_session_runner.py::test_tool_timeout_fires_for_blocking_sync_tool
5 failed, 1 passed, 84 deselected in 0.19s
# not run: the rest of tests/lib
blocking: a NaN tool_timeout clears both new guards. nan <= 0 is False at _beta_session_runner.py:454, nan <= BASH_DEFAULT_TIMEOUT is False at :461, and anyio.fail_after(nan) then bounds nothing. _memories.py:59 writes its own floor as if not interval >= MIN_MEMORY_SYNC_INTERVAL with a comment on this exact trap, and _worker.py:495 calls it two lines below your self._tool_timeout =. Inverting both comparisons the same way keeps inf and refuses NaN.
$ PYTHONPATH=$HEAD/src python nan.py # SessionToolRunner(c, "s_1", tools=[FakeBash()],
# tool_timeout=float("nan"))
accepted, self.tool_timeout = nan
fail_after(nan): slept 0.4s, no timeout, 0.40s elapsed
non-blocking: Fixes #1948 closes it on merge, but hlee-cb also asked there for the margin made enforceable for a custom bash tool. Your guard imports the bundled 120 s constant, so a tool named bash carrying a longer default of its own still passes. Refs #1948 would leave that half open.
@dtmeadows-ant you merged the recent fix(tools): work on these files and nobody has answered hlee-cb since 09-24. Does this scope close #1948, or should the per-call sizing clause stay open on its own?
2eb7b27 to
4b0dfee
Compare
|
@chrikrah thanks for the detailed review, rebased onto current main, preserved the updated docstring, and changed both timeout comparisons to reject NaNs. the library suite passes: 1,583 passed, 8 skipped, 1 xfailed. custom bash timeout sizing remains outside this patch. will use Refs #1948 pending the maintainer’s scope decision. |
chrikrah
left a comment
There was a problem hiding this comment.
@kunwar-vikrant approving at 4b0dfee: both guards now refuse NaN and still accept inf, and your two NaN cases fail with the old comparisons put back.
# Python 3.12.3, pytest 9.1.1, tree from git archive 4b0dfee, merge base 18f2554
$ PYTHONPATH=$HEAD/src python -m pytest tests/lib/tools/test_session_runner.py \
tests/lib/environments/test_worker.py -q -o addopts="--tb=line -p tests._alias_httpx"
112 passed in 11.62s
# _beta_session_runner.py from 5a211bf, the commit before the NaN fix, tests kept
2 failed, 110 passed in 10.55s
FAILED tests/lib/tools/test_session_runner.py::test_tool_timeout_rejects_nan[bash]
FAILED tests/lib/tools/test_session_runner.py::test_tool_timeout_rejects_nan[echo]
# lib/ and resources/ from 18f2554, tests kept
75 failed, 37 passed in 7.11s
E TypeError: SessionToolRunner.__init__() got an unexpected keyword argument 'tool_timeout'
# not run: the rest of tests/lib
non-blocking: you mentioned switching to Refs #1948, and the body still says Fixes, so a merge would close it.
@dtmeadows-ant the NaN guard from my last review is fixed and tested at 4b0dfee, so this is ready for a maintainer pass.
Fixes #1948. Self-hosted Managed Agents currently stop tool calls after 150 seconds, even when a bash call requests longer. This adds a configurable tool_timeout to the worker and session runner while preserving the 150-second default. For example, worker(tool_timeout=300) allows a bash call with timeout_ms=240000 to use its requested budget.
The affected tools and environments tests, lint, and type checks pass. This changes Python SDK behavior only; it does not change the wire API. Go already supports a timeout option, while TypeScript remains fixed.