feat(net)!: rolling-window (error-rate) circuit breaker (ADR-0031 Am#3)#119
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Swap the CircuitBreaker's Closed-state trip trigger from a consecutive transport-failure counter to the count-based error-rate rolling window built in the prior commit (ADR-0031 Amendment #3): a sustained failure rate over a window now trips the breaker, not a run of back-to-back failures a lone interleaved success used to reset. Breaking: CircuitBreakerConfig drops `failure_threshold` and gains `failure_rate_threshold: u8`, `window_size: NonZeroU32`, and `minimum_calls: NonZeroU32`; boot validation (`stack::validate_config`) rejects an out-of-range threshold and a `minimum_calls > window_size` config via two new BuildError variants. Open/Half-Open/probe-guard/ Retry-After behavior is unchanged. Every construction site (doctests, test helpers, the hyper example) is updated to the new fields.
Records ADR-0031 Amendment #3 (consecutive-count trip -> rolling error-rate window) and the corresponding CHANGELOG entry under [Unreleased] -> Changed.
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR replaces the net-http CircuitBreaker's consecutive-failure trip policy with a rolling error-rate window. It adds a ChangesRolling error-rate breaker
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/adapter/net/http/api/src/stack.rs (1)
211-225: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider bounding
window_sizeat boot to avoid a large eager allocation.
validate_configchecksminimum_calls <= window_sizeand the threshold range, but places no ceiling onwindow_sizeitself.RateWindow::new(rate_window.rs) doesVecDeque::with_capacity(window_size.get() as usize)— an eager allocation that scales directly with a misconfigured value (up to ~4 billionOutcomeslots forNonZeroU32::MAX), and this reallocates on every Half-Open→Closed recovery, not just at boot.Since this is boot-time operator config rather than attacker-controlled input, this is a defensive suggestion rather than a security bug — a sane upper bound (e.g. a few thousand) would catch a units/typo mistake before it causes a large or failing allocation.
🛡️ Example additional guard
if cfg.circuit_breaker.minimum_calls.get() > cfg.circuit_breaker.window_size.get() { return Err(BuildError::MinCallsExceedWindow( cfg.circuit_breaker.minimum_calls.get(), cfg.circuit_breaker.window_size.get(), )); } + // Sanity ceiling: guards against a units/typo mistake causing a very large + // eager `VecDeque` allocation in `RateWindow::new`. + const MAX_WINDOW_SIZE: u32 = 10_000; + if cfg.circuit_breaker.window_size.get() > MAX_WINDOW_SIZE { + return Err(BuildError::WindowSizeTooLarge( + cfg.circuit_breaker.window_size.get(), + MAX_WINDOW_SIZE, + )); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/adapter/net/http/api/src/stack.rs` around lines 211 - 225, Add a sane upper bound check for circuit breaker window size in validate_config alongside the existing threshold and minimum_calls validation, so oversized values are rejected before RateWindow::new allocates a large VecDeque. Use the existing config fields in stack.rs and the RateWindow::new constructor in rate_window.rs as the key symbols to update, and return a BuildError for values above the chosen boot-time limit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/adapter/net/http/api/src/stack.rs`:
- Around line 211-225: Add a sane upper bound check for circuit breaker window
size in validate_config alongside the existing threshold and minimum_calls
validation, so oversized values are rejected before RateWindow::new allocates a
large VecDeque. Use the existing config fields in stack.rs and the
RateWindow::new constructor in rate_window.rs as the key symbols to update, and
return a BuildError for values above the chosen boot-time limit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f205b71-832a-4bda-a444-7e6400f6895b
📒 Files selected for processing (12)
CHANGELOG.mdcrates/adapter/net/http/api/src/circuit_breaker.rscrates/adapter/net/http/api/src/lib.rscrates/adapter/net/http/api/src/meter.rscrates/adapter/net/http/api/src/rate.rscrates/adapter/net/http/api/src/rate_window.rscrates/adapter/net/http/api/src/stack.rscrates/adapter/net/http/hyper/examples/client_with_directives.rscrates/adapter/net/http/hyper/src/build.rsdocs/adr/0031-http-resilience-venue-pacing.mddocs/superpowers/plans/2026-07-09-net-http-rolling-window-breaker.mddocs/superpowers/specs/2026-07-09-net-http-rolling-window-breaker-design.md
Reject a window_size above a 10k sanity ceiling in validate_config (new BuildError::WindowSizeTooLarge) so a units/typo mistake can't force a huge eager RateWindow VecDeque alloc — re-allocated on every recovery. Consistent with the existing fail-closed boot validation. Addresses the CodeRabbit review on PR #119. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Closes #118. Second Tier-2 hardening item split from #102 (after Retry-After honoring, #117).
What
Replaces the
CircuitBreaker's consecutive-count trip trigger with a count-based rolling error-rate window (ADR-0031 Amendment #3). Today a singleSuccessresets the failure streak, so a venue failing ~50 % of interleaved traffic (F S F S…) never trips — the top resilience-detection hole from the deep-review §2B. ADR-0031 §5 always planned this ("consecutive-count for v1; rolling-window later").How
RateWindowunit (rate_window.rs) — a fixed-capacityVecDequering of the last-NFailure/Successoutcomes with a running failure count; zero per-request allocation, clock-free, integer trip math (u64-widened to avoid overflow).Failure):len ≥ minimum_calls && failures*100 ≥ failure_rate_threshold*len(≥trips).Successnow dilutes the rate instead of resetting;Ignored(4xx/Auth) is never a sample; a venue 429 (TripNow) still trips immediately onretry_after_fallback/honored value (Amendment chore: Fix cargo-deny workspace wildcards #2, unchanged).Open/HalfOpen/probe-guard/Retry-Afterare unchanged; recovery builds a fresh window onHalfOpen → Closed.http_circuit_breaker_transitions_total{to="open"}counter gains a boundedreasonlabel (rate/throttle/probe_failed/abandoned) so a rate-degradation trip is distinguishable from a 429 penalty-box trip.tower-resilience-circuitbreaker(cited in the ADR; not adopted — OATH keeps its RPITIT&self/no-dynService).Breaking (pre-release)
CircuitBreakerConfigdropsfailure_thresholdand gainsfailure_rate_threshold: u8(1..=100),window_size: NonZeroU32, andminimum_calls: NonZeroU32, validated at boot instack::validate_config(BuildError::RateThresholdRange/MinCallsExceedWindow). Recommended v1 profile:50 % / N=50 / min_calls=10(tunable, not hardcoded).Tests
TDD throughout: the
RateWindowunit, the pureBreaker(incl. the motivating interleaved-50 % case, the min-calls floor, ignored-not-sampled, a discriminating reset-on-close), the boot-validation rejections, and thereasontelemetry.just ci+just msrvgreen.Reviews
Four per-task reviews (spec + quality) + a final whole-branch review (opus): Ready to merge, 0 Critical / 0 Important. Two non-blocking nice-to-haves deferred: a doc note that
should_tripassumesmin_calls ≥ 1(moot —NonZeroU32, validated≤ window_size), and a sentence on theservice_tests::cfg100/N/N no-interleave assumption.Spec:
docs/superpowers/specs/2026-07-09-net-http-rolling-window-breaker-design.md· Plan:docs/superpowers/plans/2026-07-09-net-http-rolling-window-breaker.md🤖 Generated with Claude Code
Summary by CodeRabbit
Breaking Changes
New Features
Bug Fixes