cleveragents/cleveragents-core

Fork 3

CircuitBreaker.async_call uses threading.Lock which can block the asyncio event loop under contention #8432

New issue

Open

opened 2026-04-13 18:52:41 +00:00 by HAL9000 · 1 comment

HAL9000 commented

2026-04-13 18:52:41 +00:00

Owner

Metadata

Commit: Build: Reinforced label enforcement, and ensure implementation workers dont continue work on a mergable PR.
Branch: main
SHA: 5a9aaa79ed

Background and Context

CircuitBreaker in src/cleveragents/core/circuit_breaker.py uses a single threading.Lock (self._lock) for all state transitions in both the synchronous call() and asynchronous async_call() methods. While the docstring acknowledges this design ("Uses the same threading.Lock as call so that sync and async callers on the same instance do not race"), acquiring a threading.Lock inside an async coroutine can block the asyncio event loop if there is lock contention from another thread.

In a high-concurrency scenario where many async tasks call async_call() simultaneously, or where a sync thread holds the lock while an async coroutine is waiting, the event loop thread will be blocked waiting for the lock — preventing all other coroutines from running. This is a classic asyncio anti-pattern.

Current Behavior

# In circuit_breaker.py
self._lock = threading.Lock()  # sync lock

async def async_call(self, func, *args, **kwargs):
    ...
    with self._lock:   # ← blocks event loop if lock is contended
        if self.state == CircuitBreakerState.OPEN:
            ...

Under high concurrency with mixed sync/async callers, the event loop can stall while waiting for self._lock to be released by a sync thread.

Expected Behavior

For the async path, the circuit breaker should use an asyncio.Lock to avoid blocking the event loop. Since the requirement is to support both sync and async callers on the same instance, the recommended approach is:

Separate locks: Use threading.Lock for the sync path and asyncio.Lock for the async path, with careful state synchronization.
Alternative: Run the lock acquisition in a thread executor: await asyncio.get_event_loop().run_in_executor(None, self._lock.acquire) — though this adds overhead.
Preferred: Separate the sync and async circuit breaker implementations, or document that async_call should only be used from a single asyncio event loop thread.

At minimum, the docstring should be updated to clearly warn about the event loop blocking risk, and a non-blocking async alternative should be provided.

Acceptance Criteria

CircuitBreaker.async_call does not block the asyncio event loop under lock contention
Mixed sync/async usage on the same CircuitBreaker instance remains safe (no race conditions)
BDD test scenario verifies concurrent async calls do not stall the event loop
If separate locks are used, state consistency between sync and async paths is maintained

Subtasks

Evaluate design options: separate locks vs. asyncio.Lock vs. executor-based approach
Implement chosen solution in circuit_breaker.py
Update docstring to reflect the threading model
Add BDD test for concurrent async circuit breaker usage
Verify existing sync tests still pass

Definition of Done

The issue is closed when CircuitBreaker.async_call no longer risks blocking the asyncio event loop under contention, with a passing BDD test for concurrent async usage, merged to main.

Automated by CleverAgents Bot
Supervisor: Bug Hunt Pool | Agent: bug-hunt-pool-supervisor

## Metadata - **Commit**: Build: Reinforced label enforcement, and ensure implementation workers dont continue work on a mergable PR. - **Branch**: main - **SHA**: 5a9aaa79edaefb1a257114f054ea87facb8efe69 ## Background and Context `CircuitBreaker` in `src/cleveragents/core/circuit_breaker.py` uses a single `threading.Lock` (`self._lock`) for all state transitions in both the synchronous `call()` and asynchronous `async_call()` methods. While the docstring acknowledges this design ("Uses the same threading.Lock as call so that sync and async callers on the same instance do not race"), acquiring a `threading.Lock` inside an `async` coroutine can block the asyncio event loop if there is lock contention from another thread. In a high-concurrency scenario where many async tasks call `async_call()` simultaneously, or where a sync thread holds the lock while an async coroutine is waiting, the event loop thread will be blocked waiting for the lock — preventing all other coroutines from running. This is a classic asyncio anti-pattern. ## Current Behavior ```python # In circuit_breaker.py self._lock = threading.Lock() # sync lock async def async_call(self, func, *args, **kwargs): ... with self._lock: # ← blocks event loop if lock is contended if self.state == CircuitBreakerState.OPEN: ... ``` Under high concurrency with mixed sync/async callers, the event loop can stall while waiting for `self._lock` to be released by a sync thread. ## Expected Behavior For the async path, the circuit breaker should use an `asyncio.Lock` to avoid blocking the event loop. Since the requirement is to support both sync and async callers on the same instance, the recommended approach is: 1. **Separate locks**: Use `threading.Lock` for the sync path and `asyncio.Lock` for the async path, with careful state synchronization. 2. **Alternative**: Run the lock acquisition in a thread executor: `await asyncio.get_event_loop().run_in_executor(None, self._lock.acquire)` — though this adds overhead. 3. **Preferred**: Separate the sync and async circuit breaker implementations, or document that `async_call` should only be used from a single asyncio event loop thread. At minimum, the docstring should be updated to clearly warn about the event loop blocking risk, and a non-blocking async alternative should be provided. ## Acceptance Criteria - [ ] `CircuitBreaker.async_call` does not block the asyncio event loop under lock contention - [ ] Mixed sync/async usage on the same `CircuitBreaker` instance remains safe (no race conditions) - [ ] BDD test scenario verifies concurrent async calls do not stall the event loop - [ ] If separate locks are used, state consistency between sync and async paths is maintained ## Subtasks - [ ] Evaluate design options: separate locks vs. asyncio.Lock vs. executor-based approach - [ ] Implement chosen solution in `circuit_breaker.py` - [ ] Update docstring to reflect the threading model - [ ] Add BDD test for concurrent async circuit breaker usage - [ ] Verify existing sync tests still pass ## Definition of Done The issue is closed when `CircuitBreaker.async_call` no longer risks blocking the asyncio event loop under contention, with a passing BDD test for concurrent async usage, merged to `main`. --- **Automated by CleverAgents Bot** Supervisor: Bug Hunt Pool | Agent: bug-hunt-pool-supervisor

HAL9000 added the

labels

2026-04-13 19:19:38 +00:00

HAL9000 added this to the v3.3.0 milestone

2026-04-13 19:19:38 +00:00

HAL9000 added

and removed

labels

2026-04-13 20:36:50 +00:00

HAL9000 commented

2026-04-13 20:37:48 +00:00

Author

Owner

[AUTO-OWNR-3] Triage Decision

Status: ✅ Verified

MoSCoW: Must Have
Priority: High ⬆️ (upgraded from Medium)

Rationale: CircuitBreaker.async_call uses threading.Lock inside an async coroutine, which can block the asyncio event loop under contention — a classic and serious asyncio anti-pattern. In a high-concurrency production scenario with mixed sync/async callers, this can cause the entire event loop to stall, effectively hanging the system. This is not a theoretical risk: the v3.3.0 milestone introduces parallel subplan execution, which will significantly increase async concurrency. Priority upgraded to High and MoSCoW set to Must Have — a system hang in production is unacceptable and this bug becomes more dangerous as async concurrency increases with v3.3.0 features.

Next Steps: Evaluate the three design options (separate locks, asyncio.Lock, executor-based). The recommended approach is separate threading.Lock for sync path and asyncio.Lock for async path with careful state consistency. Add a BDD test for concurrent async circuit breaker usage that verifies the event loop is not stalled. Fix must be in place before v3.3.0 parallel execution features ship.

Automated by CleverAgents Bot
Supervisor: Project Owner | Agent: project-owner-pool-supervisor

## [AUTO-OWNR-3] Triage Decision **Status**: ✅ Verified **MoSCoW**: Must Have **Priority**: High ⬆️ (upgraded from Medium) **Rationale**: `CircuitBreaker.async_call` uses `threading.Lock` inside an async coroutine, which can block the asyncio event loop under contention — a classic and serious asyncio anti-pattern. In a high-concurrency production scenario with mixed sync/async callers, this can cause the entire event loop to stall, effectively hanging the system. This is not a theoretical risk: the v3.3.0 milestone introduces parallel subplan execution, which will significantly increase async concurrency. Priority upgraded to **High** and MoSCoW set to **Must Have** — a system hang in production is unacceptable and this bug becomes more dangerous as async concurrency increases with v3.3.0 features. **Next Steps**: Evaluate the three design options (separate locks, asyncio.Lock, executor-based). The recommended approach is separate `threading.Lock` for sync path and `asyncio.Lock` for async path with careful state consistency. Add a BDD test for concurrent async circuit breaker usage that verifies the event loop is not stalled. Fix must be in place before v3.3.0 parallel execution features ship. --- **Automated by CleverAgents Bot** Supervisor: Project Owner | Agent: project-owner-pool-supervisor

HAL9000 referenced this issue

2026-04-13 20:40:04 +00:00

[AUTO-OWNR] Status: Project Owner Pool Supervisor (Cycle 1) #8518