Skip to content

fix(providers): prevent TOCTOU race in _discover_providers() - #24696

Closed
wesleysimplicio wants to merge 1 commit into
NousResearch:mainfrom
wesleysimplicio:fix/cx13-issue-24694-providers-discover-toctou
Closed

wesleysimplicio wants to merge 1 commit into
NousResearch:mainfrom
wesleysimplicio:fix/cx13-issue-24694-providers-discover-toctou

Conversation

@wesleysimplicio

@wesleysimplicio wesleysimplicio commented May 13, 2026 •

Copy link
Copy Markdown

What does this PR do?

Fixes a time-of-check/time-of-use (TOCTOU) race condition in providers/_discover_providers().

Root cause

The detailed rationale from the original PR body is preserved below. This template update keeps the review structure consistent with #29640.

Fix

  • providers/__init__.py: add _discover_lock = threading.Lock(); indent all three discovery sections inside with _discover_lock:; move _discovered = True to after all work completes
  • tests/providers/test_plugin_discovery.py: add TestDiscoverProvidersToctouRace with a 50-thread threading.Barrier contention test and a spy test asserting the flag stays False while plugins are being imported

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 #24694

Original body

Summary

Fixes a time-of-check/time-of-use (TOCTOU) race condition in providers/_discover_providers().

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?

Summary

Fixes a time-of-check/time-of-use (TOCTOU) race condition in providers/_discover_providers().

Root cause: _discovered was set to True at the top of the function, before any plugin directory was scanned or any module was imported. A second thread entering concurrently could observe _discovered=False, pass the early-return guard, then race the first thread through the three discovery phases — resulting in a partially populated registry visible to callers.

Fix: Double-checked locking pattern:

  • Fast lock-free early-return path (if _discovered: return) remains for the already-initialised steady state
  • Slow path acquires _discover_lock, re-checks the flag inside the lock, runs all three discovery phases (bundled plugins → user plugins → legacy per-file modules), then sets _discovered = True as the last statement inside the lock

This matches the pattern already applied to agent/transports/__init__.py (#24692) and plugins/platforms/google_chat/adapter.py (#24679).

Impact

get_provider_profile() has no retry-on-miss logic — it returns None directly if the name is absent from the registry. Under the old code, a race could cause valid provider names to return None, silently falling back to generic behaviour for the lifetime of the process.

Changes

  • providers/__init__.py: add _discover_lock = threading.Lock(); indent all three discovery sections inside with _discover_lock:; move _discovered = True to after all work completes
  • tests/providers/test_plugin_discovery.py: add TestDiscoverProvidersToctouRace with a 50-thread threading.Barrier contention test and a spy test asserting the flag stays False while plugins are being imported

Test plan

  • uv run pytest tests/providers/ — 94 passed
  • New TestDiscoverProvidersToctouRace class: 2 new tests (concurrent consistency + flag-ordering invariant)

Closes #24694

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 #24694

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

_discovered was set to True before any plugin directory was scanned or
imported, so a second thread could observe _discovered=False, enter the
discovery work, and race the first thread — leaving the registry
partially populated or empty for callers that checked between the flag
write and the actual scan.

Apply double-checked locking: acquire _discover_lock, re-check the flag
inside the lock, run all three discovery phases (bundled plugins, user
plugins, legacy per-file modules), then set _discovered=True as the
final step inside the lock.

Adds a 50-thread barrier regression test and a spy test that asserts
_discovered remains False while plugins are being imported.

Closes NousResearch#24694

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 13, 2026 01:18
@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins P2 Medium — degraded but workaround exists labels May 13, 2026

Copilot AI left a comment

Copy link
Copy Markdown

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

concurrency: providers/_discover_providers() sets _discovered=True before registry is populated (TOCTOU)

3 participants