fix(actor): move namespace filter inside lock in ActorLoader.list_actors #8660
@@ -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
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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
|
||||
# ────────────────────────────────────────────────────────────
|
||||
|
||||
@@ -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
|
||||
|
|
||||
@@ -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."""
|
||||
|
HAL9000
commented
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:
This preserves static type safety without suppression. Automated by CleverAgents Bot 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(
|
||||
|
HAL9000
commented
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:
This resolves the type mismatch without suppression. Automated by CleverAgents Bot 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}"
|
||||
)
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user
BLOCKING — Import ordering violation (lint failure root cause)
This
from typing import castimport is placed after third-partybehaveimports, violating ruff/isort ordering rules. stdlib imports must always precede third-party imports.Current (incorrect):
Required (correct):
This is the root cause of the
CI / lintfailure on HEAD47d0fb15.Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
BLOCKER: Import misplaced — isort/ruff violation causing CI lint failure.
from typing import castis 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:
This one-line reordering will fix the
CI / lintfailure.BLOCKING (lint violation — import ordering):
from typing import cast(stdlib) is placed on line 12, afterfrom behave import given, then, when(line 10) andfrom behave.runner import Context(line 11). This violates ruff/isort import ordering: stdlib imports must precede third-party imports. This is whyCI / lintis failing after 1m9s on the current HEAD.Fix: Move
from typing import castinto the stdlib block, immediately afterfrom pathlib import Pathand before the blank line separating stdlib from third-party:Run
nox -s lintlocally to confirm the violation is resolved before pushing.Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker