feat: add circuit breaker for cascading failure protection

[bnusunny]

Adds a thread-safe circuit breaker implementation to protect against cascading failures in external service calls.

[cortex-assistant]

Circuit Breaker Review

The implementation is clean and well-structured — good use of time.monotonic(), enum-based state modeling, and clear docstrings. However, there are a couple of correctness issues and missing elements that should be addressed before merging.

🔴 Critical Issues

1. Race condition in allow_request() for HALF_OPEN state — allow_request() calls self.state (which acquires/releases the lock), then returns the result. Between the lock release inside the state property and the caller acting on the return value, another thread can also read HALF_OPEN and get True. This allows multiple threads to pass through simultaneously, violating the "one probe call" contract. Fix: allow_request() should atomically check the state and, if HALF_OPEN, set a flag to block subsequent callers until the probe completes.

2. No tests — This is a concurrency-sensitive component where bugs are subtle. Unit tests should cover: all state transitions, threshold boundary, recovery timeout behavior, HALF_OPEN success/failure paths, and thread-safety under contention.

🟡 Medium Issues

3. record_success() unconditionally resets to CLOSED — Calling record_success() while in OPEN state (before timeout expires) would incorrectly force the breaker closed, bypassing recovery timeout. Consider guarding so it only transitions from HALF_OPEN → CLOSED.

4. No logging — Other utils in this project use LOG = logging.getLogger(__name__). State transitions (CLOSED→OPEN, OPEN→HALF_OPEN, HALF_OPEN→CLOSED) are critical operational events that should be logged.

5. No integration point — The circuit breaker is added but nothing uses it. Consider adding a decorator or context-manager interface to make it practical.

🟢 Minor

6. No input validation on failure_threshold (could be 0 or negative) and recovery_timeout (could be negative).

What's Good

• time.monotonic() over time.time() ✅
• Clean enum-based state machine ✅
• Clear class docstring describing the contract ✅
• Proper threading.Lock usage ✅

[cortex-assistant]

🔴 Race condition: allow_request() reads self.state which acquires and releases the lock, but between that release and the caller acting on the result, another thread can also see HALF_OPEN and get True. This violates the "one probe call" contract.

Suggested fix — make the check-and-transition atomic:

def allow_request(self) -> bool:
    with self._lock:
        if self._state == CircuitState.OPEN:
            if time.monotonic() - self._last_failure_time >= self._recovery_timeout:
                self._state = CircuitState.HALF_OPEN
            else:
                return False
        if self._state == CircuitState.HALF_OPEN:
            # Block further callers until probe completes
            self._state = CircuitState.OPEN
            return True  # Only this caller gets through
        return self._state == CircuitState.CLOSED

[cortex-assistant]

🟡 Guard against misuse: record_success() unconditionally resets to CLOSED. If called while OPEN (before timeout), it bypasses the recovery timeout entirely. Consider:

def record_success(self) -> None:
    with self._lock:
        self._failure_count = 0
        if self._state == CircuitState.HALF_OPEN:
            self._state = CircuitState.CLOSED

[cortex-assistant]

🟢 Minor: Add input validation to prevent nonsensical configurations:

if failure_threshold < 1:
    raise ValueError("failure_threshold must be >= 1")
if recovery_timeout <= 0:
    raise ValueError("recovery_timeout must be > 0")

[cortex-assistant]

I've completed the review of PR #20. Here's a summary of what I did:

Review submitted: REQUEST_CHANGES with 3 inline comments:

1. 🔴 Race condition in allow_request() (line 55) — The HALF_OPEN state check is not atomic, allowing multiple threads to pass through simultaneously when only one probe call should be permitted. Provided a suggested fix that makes the check-and-transition atomic under a single lock acquisition.

2. 🟡 record_success() guard (line 44) — Currently unconditionally resets to CLOSED, which could bypass the recovery timeout if called while OPEN. Suggested guarding the transition to only apply from HALF_OPEN.

3. 🟢 Input validation (line 26) — No validation on constructor parameters; negative/zero values would cause nonsensical behavior.

The review body also calls out the missing tests (critical for a concurrency primitive), missing logging for state transitions, and the lack of any integration point or decorator/context-manager API.

Label added: size:S (1 file, +55 lines)