diff --git a/CHANGELOG.md b/CHANGELOG.md index 616a5945c..eb553a1be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -318,6 +318,16 @@ ensuring data is stored with proper parameter values. infinite recursion at runtime. Added Behave regression tests (`features/actor_subgraph_cycle_detection.feature`) and a Robot Framework integration test (`robot/actor_compiler.robot`) to prevent regressions. + +- **ActorLoader.list_actors TOCTOU race condition** (#8588): Moved the namespace + filter inside the ``with self._lock:`` block so that the actors dictionary is read + and filtered atomically. Previously an actor ``.clear()`` could run between the + dictionary read and the filter step, causing the loader to return stale results or + raise ``RuntimeError: dictionary changed size during iteration``. Added a + ``@unit @actor @concurrency`` Behave scenario in ``features/actor_loading.feature`` + that exercises ``list_actors(namespace=...)`` and ``clear()`` on multiple threads + via ``threading.Barrier``. + - **Devcontainer auto-discovery wired into `git-checkout`/`fs-directory` handlers** (#4740): `GitCheckoutHandler.discover_children()` and `FsDirectoryHandler.discover_children()` now call `discover_devcontainers()` after scanning for `fs-directory` children. Any diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 98dc7f4d0..a5c90c67f 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -35,6 +35,7 @@ Below are some of the specific details of various contributions. * HAL 9000 has contributed the architecture-pool-supervisor milestone assignment feature (PR #8188 / issue #7521): added `forgejo_update_pull_request` permission and documented the PR workflow for major spec changes, enabling automatic milestone assignment for specification PRs. * HAL 9000 has contributed the git worktree TOCTOU race condition fix (PR #8178 / issue #7507): replaced the unsafe mkdtemp() + rmdir() pattern with a parent-directory approach to eliminate the race window in concurrent git worktree operations. * HAL 9000 has contributed the git_tools TOCTOU race condition fix (PR #8255 / issue #7619): eliminated the Time-Of-Check-To-Time-Of-Use race in `_get_base_env()` by adding double-checked locking with a module-level `threading.Lock`, preventing concurrent threads from writing conflicting environment snapshots. +* HAL 9000 has contributed the ActorLoader.list_actors TOCTOU race condition fix (PR #8660 / issue #8588): moved the namespace filter inside the ``with self._lock:`` block in ``list_actors()`` so that dictionary reads and filtering are atomic, eliminating stale results and potential ``RuntimeError`` under concurrent access. Added concurrency BDD coverage via Behave test scenarios using ``threading.Barrier``. * HAL 9000 has contributed the mandatory PR compliance checklist to `implementation-supervisor.md` (#9824): added an 8-item checklist to the worker prompt body with concrete items covering CHANGELOG.md, CONTRIBUTORS.md, commit footer, CI verification, BDD tests, Epic reference, labels, and milestone assignment to eliminate systemic PR merge blockers. * HAL 9000 has contributed the PlanResult.success derivation fix (PR #8214 / issue #7501): replaced the incorrect `error_message is None` heuristic with a dedicated `result_success` column in the plans table, ensuring plans with historical build errors are not incorrectly marked as failed after a successful apply. * HAL 9000 has contributed the mandatory PR compliance checklist to `implementation-pool-supervisor.md` (#9824): created a new agent definition with an embedded 8-item checklist ensuring workers always update CHANGELOG.md, CONTRIBUTORS.md, include commit footers (`ISSUES CLOSED: #N`), verify CI passes, add BDD tests, reference the parent Epic, apply labels via forgejo-label-manager, and assign milestones before creating PRs. Includes concrete examples for each subsection and compliance verification pseudocode. diff --git a/features/actor_loading.feature b/features/actor_loading.feature index 514aea06e..677a5a7b2 100644 --- a/features/actor_loading.feature +++ b/features/actor_loading.feature @@ -228,6 +228,22 @@ Feature: Actor registry and loader Then the loader should contain actor "utilities/ops" And the loader should have a warning about unresolved tool "missing/nonexistent" + # ──────────────────────────────────────────────────────────── + # Concurrency and race conditions + # ──────────────────────────────────────────────────────────── + + @unit @actor @concurrency + Scenario: list_actors with namespace filter is thread-safe under concurrent clear/discover + Given a temporary actors directory with files + | filename | name | type | model | + | rev.yaml | assistants/reviewer | llm | gpt-4 | + | fmt.yaml | utilities/formatter | llm | gpt-4 | + | ana.yaml | assistants/analyzer | llm | gpt-4 | + When I create an actor loader with that directory as search root + And I run discovery + And 5 threads concurrently call list_actors with namespace "assistants" while another thread calls clear + Then all concurrent list_actors calls should return consistent results + # ──────────────────────────────────────────────────────────── # Edge cases for coverage # ──────────────────────────────────────────────────────────── diff --git a/features/steps/actor_loading_steps.py b/features/steps/actor_loading_steps.py index bffafb854..aef633234 100644 --- a/features/steps/actor_loading_steps.py +++ b/features/steps/actor_loading_steps.py @@ -3,6 +3,8 @@ from __future__ import annotations import tempfile +import threading +import time from pathlib import Path from behave import given, then, when @@ -386,3 +388,81 @@ def step_given_list_yaml(context: Context, filename: str) -> None: d = _fresh_dir(context, "listyaml") context._actor_dir = d (d / filename).write_text("- item_one\n- item_two\n") + + +# ──────────────────────────────────────────────────────────── +# Concurrency test steps +# ──────────────────────────────────────────────────────────── + + +@when( + "{n:d} threads concurrently call list_actors with namespace " + '"{namespace}" while another thread calls clear' +) +def step_when_concurrent_list_actors_with_clear( + context: Context, n: int, namespace: str +) -> None: + """Test race condition: list_actors with namespace filter under concurrent clear.""" + + loader: ActorLoader = context._loader + results: list[list[ActorConfigSchema]] = [] + errors: list[Exception] = [] + barrier = threading.Barrier(n + 1) # n list_actors threads + 1 clear thread + + def list_actors_worker() -> None: + try: + barrier.wait() # All threads start simultaneously + result = loader.list_actors(namespace=namespace) + results.append(result) + except Exception as exc: + errors.append(exc) + + def clear_worker() -> None: + try: + barrier.wait() # All threads start simultaneously + # Small delay to let list_actors threads acquire the lock first + time.sleep(0.001) + loader.clear() + except Exception as exc: + errors.append(exc) + + # Create n threads for list_actors calls + list_threads = [threading.Thread(target=list_actors_worker) for _ in range(n)] + # Create 1 thread for clear + clear_thread = threading.Thread(target=clear_worker) + + # Start all threads + for t in list_threads: + t.start() + clear_thread.start() + + # Wait for all threads to complete + for t in list_threads: + t.join() + clear_thread.join() + + assert not errors, f"Threads raised exceptions: {errors}" + + # Store results and namespace for assertion + context._concurrent_list_results = results + context._concurrent_namespace = namespace + + +@then("all concurrent list_actors calls should return consistent results") +def step_then_concurrent_results_consistent(context: Context) -> None: + """Verify that all concurrent list_actors calls returned consistent results.""" + results: list[list[ActorConfigSchema]] = context._concurrent_list_results # type: ignore[assignment] + namespace: str = context._concurrent_namespace # type: ignore[assignment] + + # Each result should only contain actors from the requested namespace. + # When clear() runs concurrently, some threads may see the full list while + # others see an empty list, but no result should have stale/partial data. + if not results: + return + + # Check that each result only contains actors from the requested namespace + for i, result in enumerate(results): + for actor_config in result: + assert actor_config.name.startswith(f"{namespace}/"), ( + f"Result {i} contains actor from wrong namespace: {actor_config.name}" + ) diff --git a/src/cleveragents/actor/loader.py b/src/cleveragents/actor/loader.py index b0f900bb3..ee24ccea9 100644 --- a/src/cleveragents/actor/loader.py +++ b/src/cleveragents/actor/loader.py @@ -241,10 +241,9 @@ class ActorLoader: """ with self._lock: configs = [e.config for e in self._actors.values()] - - if namespace is not None: - prefix = f"{namespace}/" - configs = [c for c in configs if c.name.startswith(prefix)] + if namespace is not None: + prefix = f"{namespace}/" + configs = [c for c in configs if c.name.startswith(prefix)] return configs