diff --git a/features/providers/test_registry_thread_safety.feature b/features/providers/test_registry_thread_safety.feature index c910d278a..6e58f82eb 100644 --- a/features/providers/test_registry_thread_safety.feature +++ b/features/providers/test_registry_thread_safety.feature @@ -13,7 +13,6 @@ Feature: TDD Issue #10409 — Non-thread-safe singleton in get_provider_registry See CONTRIBUTING.md > Bug Fix Workflow > TDD Issue Test Tags. - @tdd_expected_fail 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 diff --git a/features/steps/registry_thread_safety_steps.py b/features/steps/registry_thread_safety_steps.py index 73a8cfa7b..70f2d8831 100644 --- a/features/steps/registry_thread_safety_steps.py +++ b/features/steps/registry_thread_safety_steps.py @@ -109,10 +109,17 @@ def step_two_threads_call_get_provider_registry(context: Any) -> None: with ( patch.object(ProviderRegistry, "__init__", _slow_init), - patch("cleveragents.providers.registry.get_settings", return_value=_make_settings()), + 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 = 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) @@ -134,9 +141,7 @@ def step_both_threads_same_instance(context: Any) -> None: created. The @tdd_expected_fail tag inverts the result so CI passes. """ results: list[ProviderRegistry] = context.registry_results - assert len(results) == 2, ( - f"Expected 2 results from threads, got {len(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, ( 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 diff --git a/scripts/fix_registry_steps_tmp.py b/scripts/fix_registry_steps_tmp.py deleted file mode 100644 index 48d1363c2..000000000 --- a/scripts/fix_registry_steps_tmp.py +++ /dev/null @@ -1,14 +0,0 @@ -import re - -with open('/tmp/registry_steps_orig.py', 'r') as f: - content = f.read() - -content = content.replace('daemon=False)', 'daemon=True)') -content = content.replace('threading.Barrier(2, timeout=10)', 'threading.Barrier(2, timeout=5)') -content = content.replace('construction_barrier.wait(timeout=10)', 'construction_barrier.wait(timeout=5)') -content = content.replace('entry_barrier.wait(timeout=10)', 'entry_barrier.wait(timeout=5)') - -with open('/tmp/implementation-worker-1777034444/repo/features/steps/registry_thread_safety_steps.py', 'w') as f: - f.write(content) - -print('Done')