fix(actor): move namespace filter inside lock in ActorLoader.list_actors #8660

Merged
HAL9000 merged 4 commits from fix/actor-loader-list-actors-race-condition into master 2026-06-02 09:12:04 +00:00
5 changed files with 113 additions and 4 deletions
+10
View File
@@ -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
+1
View File
@@ -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.
+16
View File
@@ -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
# ────────────────────────────────────────────────────────────
+83
View File
@@ -3,7 +3,10 @@
from __future__ import annotations
import tempfile
import threading
import time
from pathlib import Path
from typing import cast
from behave import given, then, when
from behave.runner import Context
Outdated
Review

BLOCKING — Import ordering violation (lint failure root cause)

This from typing import cast import is placed after third-party behave imports, violating ruff/isort ordering rules. stdlib imports must always precede third-party imports.

Current (incorrect):

from behave import given, then, when
from behave.runner import Context
from typing import cast          # ← stdlib placed after third-party

Required (correct):

import tempfile
import threading
import time
from pathlib import Path
from typing import cast           # ← move here, before behave imports

from behave import given, then, when
from behave.runner import Context

This is the root cause of the CI / lint failure on HEAD 47d0fb15.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKING — Import ordering violation (lint failure root cause)** This `from typing import cast` import is placed after third-party `behave` imports, violating ruff/isort ordering rules. stdlib imports must always precede third-party imports. **Current (incorrect):** ```python from behave import given, then, when from behave.runner import Context from typing import cast # ← stdlib placed after third-party ``` **Required (correct):** ```python import tempfile import threading import time from pathlib import Path from typing import cast # ← move here, before behave imports from behave import given, then, when from behave.runner import Context ``` This is the root cause of the `CI / lint` failure on HEAD `47d0fb15`. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
Outdated
Review

BLOCKER: Import misplaced — isort/ruff violation causing CI lint failure.

from typing import cast is a stdlib import and must be placed in the stdlib group, not between the third-party imports (from behave import ...) and the local imports.

The correct ordering per ruff/isort:

# stdlib
import tempfile
import threading
import time
from pathlib import Path
from typing import cast    # ← must be here, with other stdlib imports

# third-party
from behave import given, then, when
from behave.runner import Context

# local
from cleveragents.actor.loader import ActorLoader
# ...

This one-line reordering will fix the CI / lint failure.

**BLOCKER: Import misplaced — isort/ruff violation causing CI lint failure.** `from typing import cast` is a **stdlib** import and must be placed in the stdlib group, not between the third-party imports (`from behave import ...`) and the local imports. The correct ordering per ruff/isort: ```python # stdlib import tempfile import threading import time from pathlib import Path from typing import cast # ← must be here, with other stdlib imports # third-party from behave import given, then, when from behave.runner import Context # local from cleveragents.actor.loader import ActorLoader # ... ``` This one-line reordering will fix the `CI / lint` failure.
Outdated
Review

BLOCKING (lint violation — import ordering): from typing import cast (stdlib) is placed on line 12, after from behave import given, then, when (line 10) and from behave.runner import Context (line 11). This violates ruff/isort import ordering: stdlib imports must precede third-party imports. This is why CI / lint is failing after 1m9s on the current HEAD.

Fix: Move from typing import cast into the stdlib block, immediately after from pathlib import Path and before the blank line separating stdlib from third-party:

import tempfile
import threading
import time
from pathlib import Path
from typing import cast        # ← move here

from behave import given, then, when
from behave.runner import Context

Run nox -s lint locally to confirm the violation is resolved before pushing.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKING (lint violation — import ordering):** `from typing import cast` (stdlib) is placed on line 12, **after** `from behave import given, then, when` (line 10) and `from behave.runner import Context` (line 11). This violates ruff/isort import ordering: stdlib imports must precede third-party imports. This is why `CI / lint` is failing after 1m9s on the current HEAD. **Fix:** Move `from typing import cast` into the stdlib block, immediately after `from pathlib import Path` and before the blank line separating stdlib from third-party: ```python import tempfile import threading import time from pathlib import Path from typing import cast # ← move here from behave import given, then, when from behave.runner import Context ``` Run `nox -s lint` locally to confirm the violation is resolved before pushing. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
2
@@ -386,3 +389,83 @@ 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."""
Outdated
Review

BLOCKING: type: ignore is prohibited (zero tolerance)

This # type: ignore[assignment] comment must be removed. Per CONTRIBUTING.md, # type: ignore is absolutely prohibited with no exceptions.

Fix using cast() from the typing module:

from typing import cast
results = cast(list[list[ActorConfigSchema]], getattr(context, "_concurrent_list_results", []))

This preserves static type safety without suppression.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

BLOCKING: type: ignore is prohibited (zero tolerance) This # type: ignore[assignment] comment must be removed. Per CONTRIBUTING.md, # type: ignore is absolutely prohibited with no exceptions. Fix using cast() from the typing module: from typing import cast results = cast(list[list[ActorConfigSchema]], getattr(context, "_concurrent_list_results", [])) This preserves static type safety without suppression. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
results = cast(
Outdated
Review

BLOCKING: type: ignore is prohibited (zero tolerance)

This # type: ignore[assignment] comment must be removed. Per CONTRIBUTING.md, # type: ignore is absolutely prohibited with no exceptions.

Fix using cast() from the typing module:

namespace = cast(str, getattr(context, "_concurrent_namespace", ""))

This resolves the type mismatch without suppression.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

BLOCKING: type: ignore is prohibited (zero tolerance) This # type: ignore[assignment] comment must be removed. Per CONTRIBUTING.md, # type: ignore is absolutely prohibited with no exceptions. Fix using cast() from the typing module: namespace = cast(str, getattr(context, "_concurrent_namespace", "")) This resolves the type mismatch without suppression. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
list[list[ActorConfigSchema]], getattr(context, "_concurrent_list_results", [])
)
namespace = cast(str, getattr(context, "_concurrent_namespace", ""))
# 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}"
)
+3 -4
View File
1
@@ -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