Skip to content

fix(google_chat): double-checked locking for _load_google_modules() TOCTOU - #24679

Closed
wesleysimplicio wants to merge 1 commit into
NousResearch:mainfrom
wesleysimplicio:fix/cx13-issue-24673-google-modules-toctou
Closed

wesleysimplicio wants to merge 1 commit into
NousResearch:mainfrom
wesleysimplicio:fix/cx13-issue-24673-google-modules-toctou

Conversation

@wesleysimplicio

@wesleysimplicio wesleysimplicio commented May 13, 2026 •

Copy link
Copy Markdown

What does this PR do?

Fixes a TOCTOU race condition in plugins/platforms/google_chat/adapter.py::_load_google_modules().

Root cause

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

Fix

  • 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

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

Original body

Summary

Fixes a TOCTOU race condition in plugins/platforms/google_chat/adapter.py::_load_google_modules().

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 TOCTOU race condition in plugins/platforms/google_chat/adapter.py::_load_google_modules().

Bug: _google_modules_loaded = True was set on line 95 before any imports ran and before any module globals (httplib2, pubsub_v1, service_account, etc.) were assigned. A concurrent thread entering on the fast path (if _google_modules_loaded: return GOOGLE_CHAT_AVAILABLE) would see the flag as True but read stale None globals and GOOGLE_CHAT_AVAILABLE=False, causing downstream NoneType errors or silently degraded behaviour.

Fix: Added _google_modules_lock = threading.Lock() and applied the standard double-checked locking pattern:

  • Fast lock-free path for the already-initialized case (no contention after first call)
  • Slow path acquires lock and re-checks before doing work
  • All module globals are assigned before GOOGLE_CHAT_AVAILABLE = True
  • _google_modules_loaded = True is set last, after all writes are visible
  • ImportError path also sets _google_modules_loaded = True so missing-deps environments don't retry the import on every call

Test plan

  • tests/gateway/test_google_chat.py::TestLoadGoogleModulesToctouRace::test_concurrent_calls_return_consistent_result — 50 threads race on first call, all must see same return value
  • tests/gateway/test_google_chat.py::TestLoadGoogleModulesToctouRace::test_flag_set_after_globals_assigned — ImportError path still sets flag (no infinite retry)
  • Full tests/gateway/test_google_chat.py suite: 159 passed, 0 failed

Closes #24673

🤖 Generated with Claude Code

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

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

_google_modules_loaded was set True before the actual imports completed
and before module globals (httplib2, pubsub_v1, etc.) were assigned.
A concurrent thread on the fast path could read stale None globals and
GOOGLE_CHAT_AVAILABLE=False.

Fix: add _google_modules_lock (threading.Lock) and apply double-checked
locking — fast lock-free path after init, slow path acquires lock and
re-checks, all globals assigned before flag is set.

Also sets _google_modules_loaded=True in the ImportError branch so a
missing-deps environment doesn't retry the import on every call.

Closes NousResearch#24673

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 13, 2026 00:43

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 P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

concurrency: _load_google_modules() TOCTOU — _google_modules_loaded set before imports complete

3 participants