Skip to content

fix(web_tools): prevent TOCTOU races in _get_parallel_client / _get_async_parallel_client / _get_exa_client - #24741

Closed
wesleysimplicio wants to merge 1 commit into
NousResearch:mainfrom
wesleysimplicio:fix/cx13-issue-24736-web-tools-client-toctou
Closed

wesleysimplicio wants to merge 1 commit into
NousResearch:mainfrom
wesleysimplicio:fix/cx13-issue-24736-web-tools-client-toctou

Conversation

@wesleysimplicio

@wesleysimplicio wesleysimplicio commented May 13, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Three module-level lazy-init singletons in tools/web_tools.py had no lock:

Root cause

Three module-level lazy-init singletons in tools/web_tools.py had no lock:

Singleton Guard (line) Function
_parallel_client (360) if _parallel_client is None: _get_parallel_client()
_async_parallel_client (361) if _async_parallel_client is None: _get_async_parallel_client()
_exa_client (1009) if _exa_client is None: _get_exa_client()

Under concurrent tool dispatch (MoA, parallel tool calls) two threads can simultaneously see None, both call the constructor, and the losing thread's client — with its underlying connection pool or API session — is orphaned and never closed.

Fix

Added import threading and one threading.Lock() per singleton. Applied double-checked locking to each function:

_parallel_client = None
_parallel_client_lock = threading.Lock()

def _get_parallel_client():
    ...
    global _parallel_client
    if _parallel_client is None:
        with _parallel_client_lock:
            if _parallel_client is None:
                ...
                _parallel_client = Parallel(api_key=api_key)
    return _parallel_client

Same pattern applied to _get_async_parallel_client() and _get_exa_client().

Why this shape

This shape mirrors #29640 so reviewers can quickly compare scope, root cause, fix, tests, and related context without having to decode a custom PR description.

Tests

  • Veja a descrição original preservada abaixo para detalhes de validação, testes e notas de verificação.
Original body

Related PRs / issues

Closes #24736

Original body

Summary

Three module-level lazy-init singletons in tools/web_tools.py had no lock:

What Changed

  • Standardized this PR body to the current Hermes Turbo template.
  • Preserved the original detailed description below for reference.

Fluxo

A mudança continua seguindo o fluxo original descrito na seção preservada abaixo, sem ampliar o escopo funcional deste PR.

Visão

A padronização melhora a revisão, reduz ruído e evita deriva de formatação entre PRs abertos.

Test Plan

  • Veja a descrição original preservada abaixo para detalhes de validação, testes e notas de verificação.
Original body

What does this PR do?

Problem

Three module-level lazy-init singletons in tools/web_tools.py had no lock:

Singleton Guard (line) Function
_parallel_client (360) if _parallel_client is None: _get_parallel_client()
_async_parallel_client (361) if _async_parallel_client is None: _get_async_parallel_client()
_exa_client (1009) if _exa_client is None: _get_exa_client()

Under concurrent tool dispatch (MoA, parallel tool calls) two threads can simultaneously see None, both call the constructor, and the losing thread's client — with its underlying connection pool or API session — is orphaned and never closed.

Fix

Added import threading and one threading.Lock() per singleton. Applied double-checked locking to each function:

_parallel_client = None
_parallel_client_lock = threading.Lock()

def _get_parallel_client():
    ...
    global _parallel_client
    if _parallel_client is None:
        with _parallel_client_lock:
            if _parallel_client is None:
                ...
                _parallel_client = Parallel(api_key=api_key)
    return _parallel_client

Same pattern applied to _get_async_parallel_client() and _get_exa_client().

Tests

Added to tests/tools/test_web_tools_config.py:

  • TestParallelClientToctouRace — 50-thread Barrier; asserts all callers get the same object and Parallel() is called exactly once.
  • TestAsyncParallelClientToctouRace — same for AsyncParallel.
  • TestExaClientToctouRace — same for Exa.

All 6 new tests pass (uv run python -m pytest tests/tools/test_web_tools_config.py::TestParallelClientToctouRace tests/tools/test_web_tools_config.py::TestAsyncParallelClientToctouRace tests/tools/test_web_tools_config.py::TestExaClientToctouRace -v).

Closes #24736

Solution Sketch

  • fix the root cause in the touched subsystem instead of layering a broad workaround around the symptom
  • keep surrounding behavior stable and avoid unrelated refactors while the area is under review
  • prove the change with focused checks on the exact path that regressed

Related Issue

Closes #24736

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • preserved the existing technical rationale and validation notes inside the template body
  • scoped this PR description to the implementation already present on the branch
  • aligned the delivery format with .github/PULL_REQUEST_TEMPLATE.md

How to Test

  1. Review the existing validation notes preserved in this PR body.
  2. Run the focused checks for the touched area.
  3. Confirm the scoped change still behaves as described above.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform:

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

  • N/A.

Generated by Hermes Turbo


Generated by Hermes Turbo

…sync_parallel_client / _get_exa_client

Three module-level lazy-init singletons had no lock: two concurrent
threads could each see None, both construct a client, and the loser's
connection pool is orphaned.

Adds import threading and per-singleton Lock. Applies double-checked
locking (fast lock-free early-return + re-check inside lock) to all
three functions.

Adds TestParallelClientToctouRace, TestAsyncParallelClientToctouRace,
TestExaClientToctouRace in test_web_tools_config.py — each with a
50-thread Barrier that asserts identity of returned client and that
the underlying constructor is called exactly once.

Closes NousResearch#24736
Copilot AI review requested due to automatic review settings May 13, 2026 02:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/tools Tool registry, model_tools, toolsets tool/web Web search and extraction labels May 13, 2026
@wesleysimplicio

Copy link
Copy Markdown
Contributor Author

Closing — PR has merge conflicts that can't be auto-resolved. The codebase has evolved past this fix. Re-opening with a fresh rebase welcome if the issue is still open.

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 tool/web Web search and extraction type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(web_tools): TOCTOU race in _get_parallel_client / _get_async_parallel_client / _get_exa_client singleton init

3 participants