test(circuit-breaker): drive breaker transitions deterministically in the rapid-transition race test (LAB-3612) - #292
Conversation
… the rapid-transition race test (LAB-3612) test_rapid_concurrent_state_transitions asserted that post-hoc, unlocked reads of breaker._state saw at least two distinct states after 10 threads slept 10 ms between calls against a breaker with a 50 ms recovery timeout. The fifth call lands 51-54 ms after the last failure, so the assertion rested on a 1-4 ms scheduling margin; under load it fails (1 in 50 locally, and the 3.13 leg of the main-branch run at 284fa7e). Freeze the clock with time_machine and run the threads in lockstep rounds: a Barrier action advances the clock past the timeout between rounds, and each admitted operation waits at a second barrier until every thread has been admitted or rejected, so the HALF_OPEN probe holds its permit while the other threads contend for it. Assert the exact admitted count per round (everyone in failure and CLOSED rounds, exactly half_open_requests in the round after each trip) and that both OPEN and CLOSED are observed. The test now fails against a breaker with the half-open cap removed, one that never leaves OPEN, and one that never closes; the old version passed the first of those. Test-only change; no src/ change.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. WalkthroughChangesCircuit breaker race-condition tests
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Other Merge Risk: ⚪ Minimal · up to The deterministic test update has no identified merge-blocking risk. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the failure, the deterministic test changes, and the validation results. However, it omits the required template sections, including Type of Change, Security Checklist, Documentation Validation Checklist, Testing checklist, Backward Compatibility, and Additional Notes. Resolution Update the description to use the repository template. Add the required headings and complete the applicable checkboxes. State that this is a test-only change, confirm that no public API or documentation changes are required, record the relevant test commands and results, and complete the security and backward compatibility checks. Use the template headings instead of only Why, What, and Proof.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
|
@kody start-review |
1 participant
Summary
This PR rewrites the
test_rapid_concurrent_state_transitionstest to make circuit breaker state transitions deterministic rather than relying on OS thread scheduling.Problem
The previous test used
time.sleep(0.01)between operations and a shorttimeout_secondsto trigger state transitions, then only asserted weakly that "at least 2 states were observed." This made the test non-deterministic — whether the breaker actually tripped and recovered depended on how the OS happened to schedule threads andtime.sleep, so transition coverage was never guaranteed.Changes
The test now drives the breaker through controlled, reproducible transitions:
time_machine.travel(0, tick=False)so time only advances deliberately, never during a breaker operation.arrivalsbarrier ensures an admitted operation doesn't complete until every thread has been admitted or rejected — so a HALF_OPEN probe holds its permit while other threads contend for it.round_endbarrier advances the clock past the recovery timeout between rounds (via a barrier action, guaranteeing the clock only moves when no thread is inside the breaker).half_open_requestsprobes and recovers — verifying the double-checked OPEN → HALF_OPEN transition and the permit cap under contention.admitted_per_round == expectedassertion and confirms both OPEN and CLOSED states are always observed (breaker both trips and recovers on every run).BackendError) from admitted-but-failed calls (RuntimeError).Impact
The rapid-transition race test is now deterministic and reliable, verifying that the double-checked locking pattern correctly handles concurrent state checks and permit limits — instead of leaving coverage to chance.