Skip to content

Tool timeouts - #736

Merged
jpodivin merged 2 commits into
packit:mainfrom
jpodivin:tool_timeouts
Aug 6, 2026
Merged

Tool timeouts#736
jpodivin merged 2 commits into
packit:mainfrom
jpodivin:tool_timeouts

Conversation

@jpodivin

@jpodivin jpodivin commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

All tools handled by MCP gateway now have a timeout parameter inherited from the CloneableTool class.
The timeout itself is handled by a TimeSignal passed through the run method. In case that an additional signal is passed to the method from other source, for example from the agent context, both signals are preserved.

Timeouts for tools have been derived using existing timeouts and tool characteristics. There may be a good reason to change some of them.

Besides the tool timeout itself, several tools now use timeout for their HTTP sessions. This brings them in line with majority of tools.

RELEASE NOTES BEGIN

Tools handled by MPC gateway have timeouts.

RELEASE NOTES END

@qodo-for-packit

Copy link
Copy Markdown

PR Summary by Qodo

Add per-tool execution timeouts via CloneableTool AbortSignal injection

✨ Enhancement 🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

Grey Divider

AI Description

• Add a CloneableTool.timeout contract and enforce it via AbortSignal in run().
• Assign pragmatic default timeouts across privileged/unprivileged MCP gateway tools.
• Align several aiohttp-based tools with shared HTTP session timeouts and add unit coverage.
Diagram

graph TD
  A((Caller/Agent)) --> B["CloneableTool.run()"] --> C{"timeout set?"} --> F["Concrete Tool._run()"] --> G{{"External services"}}
  C -->|"yes"| D["AbortSignal.timeout()"] --> E["ToolRunOptions.signal"] --> F
  E --> H["register_signals()"]
  F --> I[("HTTP session")]
  subgraph Legend
    direction LR
    _act((Caller)) ~~~ _cls[Tool/Base] ~~~ _dec{"Decision"} ~~~ _ext{{External}} ~~~ _db[(Network/IO)]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Enforce timeouts only at MCP gateway layer
  • ➕ Centralized control; no need to touch every tool class
  • ➕ One place to tune defaults and observe metrics
  • ➖ Harder to preserve/merge tool-specific cancellation semantics with caller signals
  • ➖ Gateway may not have enough context to set sensible per-tool defaults
  • ➖ Doesn’t help non-gateway invocations of the same tools
2. Use per-tool wrapper/decorator instead of overriding `run()`
  • ➕ Keeps Tool base behavior unchanged; explicit opt-in per tool or per registration
  • ➖ Easy to miss tools (coverage gaps)
  • ➖ More boilerplate and harder to enforce uniformly across the fleet
3. Extend/standardize a framework-level `timeout` option in ToolRunOptions
  • ➕ Timeout becomes a first-class run option rather than an ad-hoc class attribute
  • ➕ Potentially integrates better with other framework features/telemetry
  • ➖ Requires framework API changes or deeper coupling
  • ➖ Longer lead time vs. the local, backwards-compatible approach in this PR

Recommendation: The current approach (a shared CloneableTool override of run() that injects an AbortSignal timeout and merges it with any caller-provided signal) is a good balance of uniform enforcement and minimal per-tool code. Reviewers should focus on correctness of signal merging semantics, and whether individual timeout values match operational expectations for the slowest legitimate calls.

Files changed (25) +329 / -13

Enhancement (21) +122 / -5
base.pyIntroduce CloneableTool timeout injection and signal merging in run() +27/-1

Introduce CloneableTool timeout injection and signal merging in run()

• Adds a 'timeout' ClassVar and overrides 'run()' to create an AbortSignal-based deadline per tool. If the caller already provided a signal, both signals are merged via an AbortController before delegating to the framework run path.

ymir/tools/base.py

copr.pyAdd execution timeouts for Copr build and artifact download tools +3/-0

Add execution timeouts for Copr build and artifact download tools

• Defines a long timeout for build polling to exceed the internal polling budget and a shorter timeout for artifact downloads. These timeouts are enforced through the new CloneableTool mechanism (where applicable).

ymir/tools/privileged/copr.py

distgit.pySet long timeout for dist-git Z-stream branch creation +1/-0

Set long timeout for dist-git Z-stream branch creation

• Adds a 3-hour tool timeout to reflect the potentially long-running branch creation workflow.

ymir/tools/privileged/distgit.py

errata.pySwitch Errata tools to CloneableTool and add uniform timeouts +12/-1

Switch Errata tools to CloneableTool and add uniform timeouts

• Rebinds 'Tool' to 'CloneableTool' and assigns 120s execution timeouts across Errata-related operations to prevent indefinite hangs while preserving caller cancellation semantics.

ymir/tools/privileged/errata.py

gitlab.pyAdd per-operation GitLab tool timeouts (short ops vs git clone/push/fetch) +19/-0

Add per-operation GitLab tool timeouts (short ops vs git clone/push/fetch)

• Defines 120s timeouts for API-style operations and 3600s timeouts for repository clone/push/fetch tasks. This standardizes runtime bounds across the GitLab tool suite.

ymir/tools/privileged/gitlab.py

jira.pyAdd 120s execution timeouts across Jira tools +15/-0

Add 120s execution timeouts across Jira tools

• Applies a consistent 120s timeout to Jira read/write/search operations, reducing risk of stuck runs while retaining compatibility with caller-provided AbortSignals.

ymir/tools/privileged/jira.py

lookaside.pyAdd 5-minute timeouts for lookaside source download/upload +2/-0

Add 5-minute timeouts for lookaside source download/upload

• Sets 300s tool timeouts for operations expected to involve larger transfers while still bounding execution.

ymir/tools/privileged/lookaside.py

maintainer_rules.pyAdopt CloneableTool and add timeout for maintainer rules fetch +3/-1

Adopt CloneableTool and add timeout for maintainer rules fetch

• Switches the tool base to CloneableTool and adds a 120s execution timeout for fetching maintainer-defined rules content.

ymir/tools/privileged/maintainer_rules.py

testing_farm.pyAdopt CloneableTool and add timeouts to Testing Farm tools +4/-1

Adopt CloneableTool and add timeouts to Testing Farm tools

• Rebinds Tool to CloneableTool and adds 120s timeouts to Testing Farm request retrieval/reproduction operations.

ymir/tools/privileged/testing_farm.py

zstream_search.pyAdd timeout to Z-stream search tool +1/-0

Add timeout to Z-stream search tool

• Sets a 120s execution timeout for z-stream search operations.

ymir/tools/privileged/zstream_search.py

analyze_ewa_testrun.pyAdopt CloneableTool and add timeout for EWA testrun analysis +4/-1

Adopt CloneableTool and add timeout for EWA testrun analysis

• Switches to CloneableTool and bounds execution with a 120s timeout.

ymir/tools/unprivileged/analyze_ewa_testrun.py

commands.pyAdd outer execution timeout to shell command tool +2/-0

Add outer execution timeout to shell command tool

• Adds a backstop tool timeout slightly above the inner asyncio timeout to prevent indefinite command execution if inner safeguards fail.

ymir/tools/unprivileged/commands.py

distgit_detector.pyAdd short timeout to distgit URL detector +1/-0

Add short timeout to distgit URL detector

• Sets a 30s timeout for a quick detection tool to keep agent flows responsive.

ymir/tools/unprivileged/distgit_detector.py

filesystem.pyAdd short timeouts to filesystem helper tools +2/-0

Add short timeouts to filesystem helper tools

• Applies 30s timeouts to lightweight local filesystem tools like cwd and remove, preventing unexpected stalls.

ymir/tools/unprivileged/filesystem.py

greenwave.pyAdd timeouts to Greenwave/Testing Farm results fetch tools +2/-0

Add timeouts to Greenwave/Testing Farm results fetch tools

• Adds 120s timeouts to unprivileged fetch tools that scrape or download results pages/artifacts.

ymir/tools/unprivileged/greenwave.py

specfile.pyAdd short timeouts to specfile parsing/editing tools +3/-0

Add short timeouts to specfile parsing/editing tools

• Adds 30s timeouts to specfile inspection and editing helpers to bound local operations.

ymir/tools/unprivileged/specfile.py

text.pyAdd short timeouts to text/file editing tools +6/-0

Add short timeouts to text/file editing tools

• Adds 30s timeouts across create/view/insert/replace/search helpers to prevent long-running file operations from blocking agent execution.

ymir/tools/unprivileged/text.py

upstream_search.pyAdd timeout to upstream repository search tool +1/-0

Add timeout to upstream repository search tool

• Sets a 300s timeout for upstream search which can legitimately take longer than simple API calls.

ymir/tools/unprivileged/upstream_search.py

upstream_tools.pyAdd timeouts to upstream repo extraction/clone and patch application tools +6/-0

Add timeouts to upstream repo extraction/clone and patch application tools

• Adds timeouts tuned to operation cost: 120s for metadata extraction, 3600s for clone, 30s for base commit checkout, and 300s for patch/cherry-pick operations.

ymir/tools/unprivileged/upstream_tools.py

version_mapper.pyAdd short timeout to version mapping tool +1/-0

Add short timeout to version mapping tool

• Sets a 30s timeout for a small mapping helper to keep flows bounded.

ymir/tools/unprivileged/version_mapper.py

wicked_git.pyAdd timeouts to local git/package prep tools +7/-0

Add timeouts to local git/package prep tools

• Applies 300s timeouts across git/package preparation and patch workflows to bound potentially heavy local operations without allowing indefinite hangs.

ymir/tools/unprivileged/wicked_git.py

Bug fix (3) +17 / -8
read_logfile.pyAdopt CloneableTool, add timeout, and enforce aiohttp session timeout +5/-2

Adopt CloneableTool, add timeout, and enforce aiohttp session timeout

• Switches to CloneableTool with a 120s execution timeout and configures aiohttp ClientSession with the shared AIOHTTP_TIMEOUT to bound HTTP waits.

ymir/tools/unprivileged/read_logfile.py

read_readme.pyAdopt CloneableTool, add timeout, and enforce aiohttp session timeout +7/-3

Adopt CloneableTool, add timeout, and enforce aiohttp session timeout

• Switches to CloneableTool with a 120s execution timeout and applies AIOHTTP_TIMEOUT to the aiohttp ClientSession while preserving User-Agent headers.

ymir/tools/unprivileged/read_readme.py

search_resultsdb.pyAdopt CloneableTool, add timeout, and enforce aiohttp session timeout +5/-3

Adopt CloneableTool, add timeout, and enforce aiohttp session timeout

• Switches to CloneableTool, adds a 120s tool timeout, and uses AIOHTTP_TIMEOUT on the aiohttp session used for resultsdb queries.

ymir/tools/unprivileged/search_resultsdb.py

Tests (1) +190 / -0
test_base.pyAdd unit tests for CloneableTool timeout and signal composition +190/-0

Add unit tests for CloneableTool timeout and signal composition

• Introduces async tests verifying: no-timeout passthrough, options creation when missing, abort behavior on slow runs, caller-signal vs tool-timeout precedence, and rough timing expectations for abort deadlines.

ymir/tools/privileged/tests/unit/test_base.py

@qodo-for-packit

qodo-for-packit Bot commented Aug 5, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. HTTP timeout undercuts tool ✗ Dismissed 🐞 Bug ☼ Reliability
Description
ReadLogfileTool/ReadReadmeTool/SearchResultsdbTool set tool timeout=120s but use
AIOHTTP_TIMEOUT(total=30s) for aiohttp.ClientSession, so requests can fail at ~30s even though the
tool should be allowed to run up to 120s. This makes these tools unreliable for slow servers or
large payloads and defeats the intended per-tool timeout budget.
Code

ymir/tools/unprivileged/read_logfile.py[R41-42]

+                aiohttp.ClientSession(timeout=AIOHTTP_TIMEOUT) as session,
                aiohttp_get_with_retries(session, input.logfile_url) as response,
Relevance

●● Moderate

Team precedent mixed on aiohttp timeout policy; no close precedent about matching per-tool timeout
budgets.

PR-#410
PR-#526

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The tools’ timeout = 120 is inconsistent with the aiohttp.ClientSession(timeout=AIOHTTP_TIMEOUT)
total timeout of 30s, so network requests can terminate 90s earlier than the tool budget.

ymir/tools/constants.py[1-4]
ymir/tools/unprivileged/read_logfile.py[20-49]
ymir/tools/unprivileged/read_readme.py[29-61]
ymir/tools/unprivileged/search_resultsdb.py[51-63]
ymir/tools/unprivileged/search_resultsdb.py[122-131]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Several tools now set a tool-level timeout of 120s, but their aiohttp sessions use `AIOHTTP_TIMEOUT` (`ClientTimeout(total=30)`), meaning HTTP operations can time out after ~30s and fail the tool early.

### Issue Context
- `AIOHTTP_TIMEOUT` is a global `aiohttp.ClientTimeout(total=30)`.
- These tools explicitly set `timeout = 120`, implying the operation is expected to have up to ~120 seconds.
- Using a shorter aiohttp *total* timeout makes the tool timeout largely irrelevant for network calls.

### Fix Focus Areas
- Update these tools to derive the aiohttp `ClientTimeout(total=...)` from the tool’s configured timeout (or otherwise make it consistent), e.g. `aiohttp.ClientTimeout(total=self.timeout)` or `aiohttp.ClientTimeout(total=max(AIOHTTP_TIMEOUT.total, self.timeout))`.
- For `search_resultsdb()`, pass an explicit timeout derived from `SearchResultsdbTool.timeout` (or accept a parameter from the tool) instead of hard-coding the global 30s.

- ymir/tools/constants.py[1-4]
- ymir/tools/unprivileged/read_logfile.py[20-49]
- ymir/tools/unprivileged/read_readme.py[29-61]
- ymir/tools/unprivileged/search_resultsdb.py[51-63]
- ymir/tools/unprivileged/search_resultsdb.py[122-131]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context used
✅ Compliance rules (platform): 7 rules

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

Comment thread ymir/tools/unprivileged/read_logfile.py
@nforro

nforro commented Aug 5, 2026

Copy link
Copy Markdown
Member

Tools handled by MPC gateway have timeouts.

Does this really affect only MCP tools? Unprivileged tools also use CloneableTool as a base.

@jpodivin

jpodivin commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Tools handled by MPC gateway have timeouts.

Does this really affect only MCP tools? Unprivileged tools also use CloneableTool as a base.

Yep, those too.

@nforro nforro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, individual tool timeouts can be adjusted as needed in the future.

Signed-off-by: Jiri Podivin <jpodivin@redhat.com>
All gateway tools are now derived from CloneableTool class.
The CloneableTool timeout attribute sets timeout of the tool execution
in seconds. This value is used to set the signal.

If signal is already set on the tool, both are registered.

Signed-off-by: Jiri Podivin <jpodivin@redhat.com>
@jpodivin
jpodivin merged commit ccbf8cb into packit:main Aug 6, 2026
11 checks passed
@jpodivin
jpodivin deleted the tool_timeouts branch August 7, 2026 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants