BUG-HUNT: [concurrency] ServiceRetryPolicyRegistry.apply_overrides has TOCTOU race condition between policy read and write #6415

New issue

Open

opened 2026-04-09 21:01:36 +00:00 by HAL9000 · 0 comments

HAL9000 commented

2026-04-09 21:01:36 +00:00

Owner

Bug Report: [concurrency] — `ServiceRetryPolicyRegistry.apply_overrides` has a TOCTOU race condition between reading the base policy and writing the merged policy

Severity Assessment

Impact: In a multi-threaded environment, a concurrent call to register() made between apply_overrides's self.get() and the final self._policies[...] = write will be silently overwritten. This causes lost configuration updates, potentially leaving the registry in an inconsistent state.
Likelihood: Low in normal usage, but the module explicitly claims thread-safety and the ThreadSafeOrgCostAccumulator class shows intent for concurrent access
Priority: Medium

Location

File: src/cleveragents/domain/models/core/retry_policy.py
Function/Class: ServiceRetryPolicyRegistry.apply_overrides
Lines: 527–604

Description

The ServiceRetryPolicyRegistry uses a threading.Lock for thread safety, but apply_overrides is NOT atomic: it calls self.get(service_name) (which acquires and RELEASES the lock), performs the merge outside the lock, then re-acquires the lock for the final write. This creates a classic time-of-check-time-of-use (TOCTOU) race condition.

The race scenario:

Thread A calls apply_overrides({"plan_service": {"retry": {"max_attempts": 5}}})
Thread A calls self.get("plan_service") → reads base policy (max_attempts=3), lock released
Thread B calls registry.register(new_policy) → writes updated policy (max_attempts=10), lock acquired/released
Thread A merges overrides onto its stale copy (max_attempts=5), re-acquires lock
Thread A writes back → Thread B's update is silently lost, now max_attempts=5 instead of 10

Evidence

# src/cleveragents/domain/models/core/retry_policy.py, lines 564-598

# Step 1: Read base (acquires then RELEASES lock):
try:
    base = self.get(service_name)    # ← Lock acquired AND RELEASED here
except (ValidationError, ValueError):
    ...
    continue

# ← LOCK IS NOT HELD HERE. Thread B can modify _policies between here...

merged = base.model_dump()
# ... merge logic runs outside the lock ...
for key in ("retry", "circuit_breaker"):
    if key in override_data:
        ...

try:
    with self._lock:    # ← Lock acquired HERE, too late
        self._policies[service_name] = ServiceRetryPolicy.model_validate(merged)
except ValidationError as exc:
    ...

The get() method, which is called at line 565, acquires and releases self._lock:

# lines 483-490
def get(self, service_name: str) -> ServiceRetryPolicy:
    ...
    with self._lock:   # ← acquires lock
        if service_name in self._policies:
            return self._policies[service_name].model_copy(deep=True)
    # ← LOCK IS RELEASED HERE

So the entire merge operation at lines 573–594 runs without holding any lock.

Expected Behavior

The read-merge-write sequence in apply_overrides should be atomic: the lock should be held for the entire duration of reading the base, merging, and writing back, preventing any concurrent modification.

Actual Behavior

The lock is released between reading the base policy and writing the merged policy, creating a window for concurrent modifications to be silently overwritten.

Suggested Fix

Hold the lock for the entire read-merge-write sequence for each service name:

def apply_overrides(self, overrides: dict[str, dict[str, Any]]) -> None:
    ...
    for service_name, override_data in overrides.items():
        service_name = service_name.strip()
        ...
        try:
            with self._lock:  # Hold lock for entire read-merge-write
                base = self._policies.get(service_name)
                if base is None:
                    base = ServiceRetryPolicy(
                        service_name=service_name,
                        ...
                    )
                merged = base.model_copy(deep=True).model_dump()
                # ... merge logic ...
                self._policies[service_name] = ServiceRetryPolicy.model_validate(merged)
        except (ValidationError, ValueError) as exc:
            ...

This eliminates the TOCTOU window by keeping the lock held throughout the entire operation.

TDD Note

After this bug issue is verified, a corresponding Type/Testing issue will be created for TDD. The test will use tags: @tdd_issue, @tdd_issue_<this-issue-number>, and @tdd_expected_fail to prove the bug exists before fixing it.

Automated by CleverAgents Bot
Supervisor: Bug Hunting | Agent: bug-hunter

## Bug Report: [concurrency] — `ServiceRetryPolicyRegistry.apply_overrides` has a TOCTOU race condition between reading the base policy and writing the merged policy ### Severity Assessment - **Impact**: In a multi-threaded environment, a concurrent call to `register()` made between `apply_overrides`'s `self.get()` and the final `self._policies[...] =` write will be silently overwritten. This causes lost configuration updates, potentially leaving the registry in an inconsistent state. - **Likelihood**: Low in normal usage, but the module explicitly claims thread-safety and the `ThreadSafeOrgCostAccumulator` class shows intent for concurrent access - **Priority**: Medium ### Location - **File**: `src/cleveragents/domain/models/core/retry_policy.py` - **Function/Class**: `ServiceRetryPolicyRegistry.apply_overrides` - **Lines**: 527–604 ### Description The `ServiceRetryPolicyRegistry` uses a `threading.Lock` for thread safety, but `apply_overrides` is NOT atomic: it calls `self.get(service_name)` (which acquires and RELEASES the lock), performs the merge outside the lock, then re-acquires the lock for the final write. This creates a classic time-of-check-time-of-use (TOCTOU) race condition. The race scenario: 1. Thread A calls `apply_overrides({"plan_service": {"retry": {"max_attempts": 5}}})` 2. Thread A calls `self.get("plan_service")` → reads base policy (max_attempts=3), lock released 3. Thread B calls `registry.register(new_policy)` → writes updated policy (max_attempts=10), lock acquired/released 4. Thread A merges overrides onto its stale copy (max_attempts=5), re-acquires lock 5. Thread A writes back → **Thread B's update is silently lost**, now max_attempts=5 instead of 10 ### Evidence ```python # src/cleveragents/domain/models/core/retry_policy.py, lines 564-598 # Step 1: Read base (acquires then RELEASES lock): try: base = self.get(service_name) # ← Lock acquired AND RELEASED here except (ValidationError, ValueError): ... continue # ← LOCK IS NOT HELD HERE. Thread B can modify _policies between here... merged = base.model_dump() # ... merge logic runs outside the lock ... for key in ("retry", "circuit_breaker"): if key in override_data: ... try: with self._lock: # ← Lock acquired HERE, too late self._policies[service_name] = ServiceRetryPolicy.model_validate(merged) except ValidationError as exc: ... ``` The `get()` method, which is called at line 565, acquires and releases `self._lock`: ```python # lines 483-490 def get(self, service_name: str) -> ServiceRetryPolicy: ... with self._lock: # ← acquires lock if service_name in self._policies: return self._policies[service_name].model_copy(deep=True) # ← LOCK IS RELEASED HERE ``` So the entire merge operation at lines 573–594 runs without holding any lock. ### Expected Behavior The read-merge-write sequence in `apply_overrides` should be atomic: the lock should be held for the entire duration of reading the base, merging, and writing back, preventing any concurrent modification. ### Actual Behavior The lock is released between reading the base policy and writing the merged policy, creating a window for concurrent modifications to be silently overwritten. ### Suggested Fix Hold the lock for the entire read-merge-write sequence for each service name: ```python def apply_overrides(self, overrides: dict[str, dict[str, Any]]) -> None: ... for service_name, override_data in overrides.items(): service_name = service_name.strip() ... try: with self._lock: # Hold lock for entire read-merge-write base = self._policies.get(service_name) if base is None: base = ServiceRetryPolicy( service_name=service_name, ... ) merged = base.model_copy(deep=True).model_dump() # ... merge logic ... self._policies[service_name] = ServiceRetryPolicy.model_validate(merged) except (ValidationError, ValueError) as exc: ... ``` This eliminates the TOCTOU window by keeping the lock held throughout the entire operation. ### Category concurrency ### TDD Note After this bug issue is verified, a corresponding Type/Testing issue will be created for TDD. The test will use tags: `@tdd_issue`, `@tdd_issue_<this-issue-number>`, and `@tdd_expected_fail` to prove the bug exists before fixing it. --- **Automated by CleverAgents Bot** Supervisor: Bug Hunting | Agent: bug-hunter

HAL9000 added the

labels

2026-04-09 21:07:17 +00:00

HAL9000 added this to the v3.2.0 milestone

2026-04-09 21:09:07 +00:00

HAL9000 added