diff --git a/features/providers/test_registry_thread_safety.feature b/features/providers/test_registry_thread_safety.feature new file mode 100644 index 000000000..6e58f82eb --- /dev/null +++ b/features/providers/test_registry_thread_safety.feature @@ -0,0 +1,20 @@ +@tdd_issue @tdd_issue_10409 +Feature: TDD Issue #10409 — Non-thread-safe singleton in get_provider_registry() + The function get_provider_registry() 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 across the application. + + This TDD issue captures the failing Behave scenario that proves the race + condition exists. It must be merged to master BEFORE the corresponding bug + fix issue is worked on. + + 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/features/steps/registry_thread_safety_steps.py b/features/steps/registry_thread_safety_steps.py new file mode 100644 index 000000000..943abcfcc --- /dev/null +++ b/features/steps/registry_thread_safety_steps.py @@ -0,0 +1,173 @@ +"""Step definitions for TDD Issue #10409 — get_provider_registry() thread-safety race condition. + +This module implements the Behave steps that prove the race condition in +``cleveragents.providers.registry.get_provider_registry``. Two concurrent +callers can each observe ``_registry is None`` and independently construct a +``ProviderRegistry``, violating the singleton contract. + +The implementation already holds ``_registry_lock`` around all reads and +writes, so this scenario passes rather than fails; ``@tdd_expected_fail`` +is intentionally omitted. The test serves as a regression guard ensuring +the lock is never removed. +""" + +from __future__ import annotations + +import contextlib +import threading +from typing import Any +from unittest.mock import MagicMock, patch + +from behave import given, then, when # type: ignore[import-untyped] + +import cleveragents.providers.registry as _registry_module +from cleveragents.providers.registry import ( + ProviderRegistry, + get_provider_registry, + reset_provider_registry, +) + + +def _make_settings() -> Any: + """Return a minimal fake Settings object for ProviderRegistry construction.""" + settings = MagicMock() + settings.openai_api_key = None + settings.anthropic_api_key = None + settings.google_api_key = None + settings.gemini_api_key = None + settings.azure_api_key = None + settings.openrouter_api_key = None + settings.cohere_api_key = None + settings.groq_api_key = None + settings.together_api_key = None + settings.default_provider = None + settings.default_model = None + settings.azure_openai_endpoint = None + settings.azure_openai_api_version = None + settings.azure_openai_deployment = None + settings.openrouter_organization = None + return settings + + +@given("the global provider registry has been reset") +def step_reset_global_registry(context: Any) -> None: + """Reset the global _registry singleton to None to simulate a fresh start.""" + reset_provider_registry() + # Verify it is actually None + assert _registry_module._registry is None, ( + "Expected _registry to be None after reset" + ) + + +@when("two threads call get_provider_registry() simultaneously") +def step_two_threads_call_get_provider_registry(context: Any) -> None: + """Spawn two threads that call get_provider_registry() simultaneously. + + A threading.Barrier is used to ensure both threads enter the function + body at the same time, maximising the chance of exposing the race + condition in the non-thread-safe singleton implementation. + + The ProviderRegistry constructor is patched to introduce a small delay + (via a second barrier) so that both threads can observe ``_registry is + None`` before either has finished constructing the instance. + """ + results: list[ProviderRegistry] = [] + construction_count: list[int] = [0] + errors: list[Exception] = [] + + # Barrier to synchronise both threads at the entry point of + # get_provider_registry() so they both observe _registry is None. + entry_barrier = threading.Barrier(2, timeout=10) + + # Barrier inside the ProviderRegistry constructor to ensure both threads + # have passed the ``if _registry is None`` check before either assigns + # the new instance to ``_registry``. + construction_barrier = threading.Barrier(2, timeout=10) + + original_init = ProviderRegistry.__init__ + + def _slow_init(self: ProviderRegistry, settings: Any = None) -> None: + """Wrap __init__ to synchronise both threads mid-construction.""" + construction_count[0] += 1 + # Wait for the other thread to also enter __init__ before proceeding. + # This guarantees both threads have passed the ``_registry is None`` + # guard in get_provider_registry() before either assigns the result. + with contextlib.suppress(threading.BrokenBarrierError): + construction_barrier.wait(timeout=10) + if settings is None: + settings = _make_settings() + original_init(self, settings) + + def _thread_body() -> None: + try: + # Synchronise both threads at the entry of get_provider_registry() + with contextlib.suppress(threading.BrokenBarrierError): + entry_barrier.wait(timeout=10) + # Call without settings to trigger the race condition + registry = get_provider_registry() + results.append(registry) + except Exception as exc: # pragma: no cover — defensive + errors.append(exc) + + with ( + patch.object(ProviderRegistry, "__init__", _slow_init), + patch( + "cleveragents.providers.registry.get_settings", + return_value=_make_settings(), + ), + ): + t1 = threading.Thread( + target=_thread_body, name="registry-thread-1", daemon=False + ) + t2 = threading.Thread( + target=_thread_body, name="registry-thread-2", daemon=False + ) + t1.start() + t2.start() + t1.join(timeout=15) + t2.join(timeout=15) + + if errors: # pragma: no cover — defensive + raise errors[0] + + context.registry_results = results + context.registry_construction_count = construction_count[0] + + +@then("both threads should receive the identical registry instance") +def step_both_threads_same_instance(context: Any) -> None: + """Assert that both threads received the exact same ProviderRegistry object. + + With ``_registry_lock`` in place the singleton contract holds, so this + assertion passes. It acts as a regression guard: removing the lock would + let two independent instances be created and trip this AssertionError. + """ + results: list[ProviderRegistry] = context.registry_results + assert len(results) == 2, f"Expected 2 results from threads, got {len(results)}" + first_id = id(results[0]) + second_id = id(results[1]) + assert first_id == second_id, ( + f"Race condition detected: two different ProviderRegistry instances were " + f"created (id={first_id} vs id={second_id}). The singleton is not " + f"thread-safe — both threads observed _registry is None and each " + f"independently constructed a new ProviderRegistry." + ) + + +@then("only one ProviderRegistry should have been constructed") +def step_only_one_registry_constructed(context: Any) -> None: + """Assert that ProviderRegistry.__init__ was called exactly once. + + With ``_registry_lock`` in place only one thread can enter the + ``if _registry is None`` branch, so the constructor runs exactly once and + this assertion passes. It acts as a regression guard: removing the lock + would let both threads call ProviderRegistry() and trip this + AssertionError. + """ + count: int = context.registry_construction_count + assert count == 1, ( + f"Race condition detected: ProviderRegistry was constructed {count} " + f"time(s) instead of exactly 1. Both threads entered the " + f"``if _registry is None`` branch simultaneously and each called " + f"ProviderRegistry(), violating the singleton contract." + ) diff --git a/features/steps/tdd_registry_thread_safety_steps.py b/features/steps/tdd_registry_thread_safety_steps.py deleted file mode 100644 index 5e98d49c5..000000000 --- a/features/steps/tdd_registry_thread_safety_steps.py +++ /dev/null @@ -1,80 +0,0 @@ -"""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 deleted file mode 100644 index 164ac9fb1..000000000 --- a/features/tdd_registry_thread_safety.feature +++ /dev/null @@ -1,20 +0,0 @@ -@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