Skip to content

feat(registry): add per-tool timeout enforcement - #8123

Open
chinadbo wants to merge 3 commits into
NousResearch:mainfrom
chinadbo:feat/tool-timeout-enforcement
Open

chinadbo wants to merge 3 commits into
NousResearch:mainfrom
chinadbo:feat/tool-timeout-enforcement

Conversation

@chinadbo

Copy link
Copy Markdown
Contributor

Summary

  • Add timeout field to ToolEntry and registry.register() — tools can now declare a wall-clock timeout in seconds
  • registry.dispatch() runs sync handlers in a thread when a timeout is set, returning a clear JSON error if execution exceeds the limit
  • 9 tests covering ToolEntry.timeout, register() pass-through, and dispatch timeout enforcement (fast completion, timeout exceeded, no-timeout default, error message content)

Problem

Only environment-layer commands and MCP tool calls had timeout guards. Regular tool dispatch had no wall-clock limit — a misbehaving or slow handler could block the agent loop indefinitely with no feedback.

Test plan

  • pytest tests/run_agent/test_tool_timeout_enforcement.py -v -o addopts="" — 9/9 passed
  • ToolEntry.timeout defaults to None, stores custom values
  • register() passes timeout to ToolEntry
  • Fast tool completes within timeout and returns result
  • Slow tool exceeds timeout and returns JSON error
  • Tool with timeout=None runs without a timeout guard
  • Timeout error includes tool name and timeout value

@chinadbo
chinadbo force-pushed the feat/tool-timeout-enforcement branch from 1f991f5 to 1fc6e18 Compare April 27, 2026 07:32
@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have comp/tools Tool registry, model_tools, toolsets labels Apr 27, 2026
ToolEntry now accepts an optional `timeout` field (seconds). When set,
registry.dispatch() runs sync handlers in a thread and cancels execution
if it exceeds the limit, returning a clear JSON timeout error instead of
hanging indefinitely. Tools without a timeout (the default) are unaffected.

Previously, only environment-layer commands and MCP tool calls had timeout
guards. Regular tool dispatch had no wall-clock limit — a misbehaving
handler could block the agent loop indefinitely.
- Fix ThreadPoolExecutor blocking on timeout by using shutdown(wait=False)
- Fix async tools bypassing timeout by checking timeout before is_async
- Guard against None return from handler in _dispatch_with_timeout
- Add 3 new tests verifying prompt return, async coverage, and normal completion
…shutdown in timeout path

cancel_futures=True only cancels pending futures, not running ones — the
comment claiming the thread is "abandoned immediately" was misleading since
the OS thread continues running to completion.  Replace the dual shutdown
calls (one in the except block, one in finally) with a single shutdown in
finally, using wait=not timed_out so the success path waits for the thread
(already done) while the timeout path returns promptly without blocking.
@chinadbo
chinadbo force-pushed the feat/tool-timeout-enforcement branch from f0d0551 to 7283121 Compare April 28, 2026 02:04

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing a real gap: current ToolRegistry.dispatch() directly invokes synchronous handlers at tools/registry.py:584-588.

Problems

  • tools/registry.py:342 submits a bare worker. Current tools/thread_context.py:4-18 documents that this loses approval/session ContextVars and thread-local approval/sudo callbacks; existing thread fan-out sites wrap targets with propagate_context_to_thread().
  • The timeout does not stop execution: the PR documents that the worker continues at tools/registry.py:323-328 and returns via shutdown(wait=False) at line 351. The synthetic sleep tests verify prompt return, not cancellation or side-effect cleanup.
  • The PR adds no timeout= registration for a bundled tool. A current-tree AST scan found zero existing registrations using this API, so there is no production handler whose behavior changes.

Suggested changes

  • Start from a concrete handler and test its real dispatch path.
  • Preserve thread context and use a handler/resource-specific cancellation mechanism, or narrow the contract to an explicitly non-cancelling caller deadline.
  • Reconcile the conflicting registry changes with current dynamic_schema_overrides, override-policy, generation, and error-sanitization behavior.

Automated hermes-sweeper review.

Comment thread tools/registry.py
result_holder[0] = entry.handler(args, **kwargs)

pool = concurrent.futures.ThreadPoolExecutor(max_workers=1)
future = pool.submit(_target)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bare worker drops the current ContextVars and thread-local approval/sudo callbacks. tools/thread_context.py:4-18 documents the security consequence, and current worker call sites use propagate_context_to_thread(...); wrap _target before submitting it.

Comment thread tools/registry.py
logger.warning("Tool %s timed out after %ss", entry.name, entry.timeout)
finally:
# Do not wait for the thread on timeout — it may run indefinitely.
pool.shutdown(wait=not timed_out)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait=False only lets dispatch return; it leaves the timed-out handler running and able to perform later side effects. Please use a handler-specific cancellation/cleanup mechanism or narrow the feature contract so it does not claim execution is terminated.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants