diff --git a/features/steps/tdd_registry_thread_safety_steps.py b/features/steps/tdd_registry_thread_safety_steps.py new file mode 100644 index 000000000..5e98d49c5 --- /dev/null +++ b/features/steps/tdd_registry_thread_safety_steps.py @@ -0,0 +1,80 @@ +"""Step definitions for TDD Issue #10409 — get_provider_registry() thread safety. + +Verifies that get_provider_registry() is safe for concurrent access from +multiple threads, preventing the race condition where two threads can each +observe _registry is None and independently construct a ProviderRegistry, +violating the singleton contract. +""" + +from __future__ import annotations + +import threading +from typing import Any + +from behave import given, then, when + +from cleveragents.providers.registry import ( + get_provider_registry, + reset_provider_registry, +) + +__all__: list[str] = [] + + +@given("the global provider registry has been reset") +def step_given_registry_reset(context: Any) -> None: + """Reset the global provider registry to None before the test.""" + reset_provider_registry() + context.registry_instances: list[object] = [] + context.construction_count = 0 + + +@when("two threads call get_provider_registry() simultaneously") +def step_when_two_threads_call_simultaneously(context: Any) -> None: + """Spawn two threads that call get_provider_registry() at the same time.""" + results: list[object] = [] + results_lock = threading.Lock() + barrier = threading.Barrier(2) + + def worker() -> None: + barrier.wait() # Synchronise both threads to maximise race window + result = get_provider_registry() + with results_lock: + results.append(result) + + threads = [threading.Thread(target=worker) for _ in range(2)] + for t in threads: + t.start() + for t in threads: + t.join() + + context.registry_instances = results + # Count is always 1 with the lock fix; without the fix it could be 2 + # We infer construction count from whether instances are identical + context.construction_count = ( + 1 if (len(results) == 2 and results[0] is results[1]) else 2 + ) + + +@then("both threads should receive the identical registry instance") +def step_then_same_instance(context: Any) -> None: + """Assert both threads received the exact same ProviderRegistry object.""" + instances = context.registry_instances + assert len(instances) == 2, ( + f"Expected 2 registry instances from threads, got {len(instances)}" + ) + assert instances[0] is instances[1], ( + f"Threads received different ProviderRegistry instances: " + f"id(instances[0])={id(instances[0])}, id(instances[1])={id(instances[1])}. " + f"This indicates a thread-safety race condition in get_provider_registry()." + ) + + +@then("only one ProviderRegistry should have been constructed") +def step_then_one_construction(context: Any) -> None: + """Assert that only one ProviderRegistry was constructed.""" + assert context.construction_count == 1, ( + f"Expected exactly 1 ProviderRegistry construction, " + f"got {context.construction_count}. " + f"This indicates a thread-safety race condition in get_provider_registry()." + ) diff --git a/features/tdd_registry_thread_safety.feature b/features/tdd_registry_thread_safety.feature new file mode 100644 index 000000000..164ac9fb1 --- /dev/null +++ b/features/tdd_registry_thread_safety.feature @@ -0,0 +1,20 @@ +@tdd_issue @tdd_issue_10409 @mock_only +Feature: TDD Issue #10409 — get_provider_registry() singleton is not thread-safe + As a developer running concurrent agent workers + I want get_provider_registry() to be thread-safe + So that only one ProviderRegistry instance is ever created under concurrent access + + This test captures bug #10478. The get_provider_registry() function in + cleveragents.providers.registry implements a singleton pattern without + thread-safety guards. Under concurrent access, two threads can each observe + _registry is None before either has finished constructing the ProviderRegistry, + causing both to independently instantiate a new registry. This violates the + singleton contract and can lead to inconsistent provider state. + + See CONTRIBUTING.md > Bug Fix Workflow > TDD Issue Test Tags. + + Scenario: Concurrent calls to get_provider_registry return the same instance + Given the global provider registry has been reset + When two threads call get_provider_registry() simultaneously + Then both threads should receive the identical registry instance + And only one ProviderRegistry should have been constructed diff --git a/src/cleveragents/providers/registry.py b/src/cleveragents/providers/registry.py index 848ee7676..d41f2421b 100644 --- a/src/cleveragents/providers/registry.py +++ b/src/cleveragents/providers/registry.py @@ -14,6 +14,7 @@ Following ADR-008 (Provider Plugin Architecture), this registry provides: from __future__ import annotations import os +import threading from dataclasses import dataclass from enum import StrEnum from typing import TYPE_CHECKING, Any, ClassVar @@ -794,13 +795,18 @@ def resolve_provider_by_name( return registry.create_ai_provider(provider_type=provider_type) -# Global registry instance +# Global registry instance and its thread-safety lock _registry: ProviderRegistry | None = None +_registry_lock: threading.Lock = threading.Lock() def get_provider_registry(settings: Settings | None = None) -> ProviderRegistry: """Get the global provider registry instance. + This function is thread-safe: all reads and writes to ``_registry`` are + protected by ``_registry_lock``, ensuring only one ``ProviderRegistry`` + instance is ever created under concurrent access. + Args: settings: Optional settings instance. Creates new registry if provided. @@ -808,15 +814,20 @@ def get_provider_registry(settings: Settings | None = None) -> ProviderRegistry: The global ProviderRegistry instance. """ global _registry - if _registry is None or settings is not None: - _registry = ProviderRegistry(settings) - return _registry + with _registry_lock: + if _registry is None or settings is not None: + _registry = ProviderRegistry(settings) + return _registry def reset_provider_registry() -> None: """Reset the global provider registry. + This function is thread-safe: the write to ``_registry`` is protected by + ``_registry_lock``. + Useful for testing to ensure clean state between tests. """ global _registry - _registry = None + with _registry_lock: + _registry = None