From da12e78fb143e3a6c928689571a3b769b7e01e9f Mon Sep 17 00:00:00 2001 From: NotAProfDev <84450364+NotAProfDev@users.noreply.github.com> Date: Thu, 9 Jul 2026 15:29:03 +0000 Subject: [PATCH 1/7] docs(net): rolling-window error-rate breaker design spec Design for replacing the CircuitBreaker's consecutive-count trip trigger with a count-based rolling error-rate window (ADR-0031 Amendment #3). Tier-2 hardening item split from #102. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...-net-http-rolling-window-breaker-design.md | 256 ++++++++++++++++++ 1 file changed, 256 insertions(+) create mode 100644 docs/superpowers/specs/2026-07-09-net-http-rolling-window-breaker-design.md diff --git a/docs/superpowers/specs/2026-07-09-net-http-rolling-window-breaker-design.md b/docs/superpowers/specs/2026-07-09-net-http-rolling-window-breaker-design.md new file mode 100644 index 0000000..c2f6c8d --- /dev/null +++ b/docs/superpowers/specs/2026-07-09-net-http-rolling-window-breaker-design.md @@ -0,0 +1,256 @@ +# net-http rolling-window (error-rate) circuit breaker — design + +## Context + +The net-http resilience stack (ADR-0031) shipped its Tier-1 remediation in #104–#114, +and the first Tier-2 hardening item (`Retry-After` honoring) in #117. This spec covers +the next **Tier-2 hardening** item tracked in +[issue #102](https://github.com/NotAProfDev/oath/issues/102): replace the breaker's +**consecutive-count** trip trigger with a **rolling error-rate window**. + +Today `CircuitBreaker` +([circuit_breaker.rs](../../../crates/adapter/net/http/api/src/circuit_breaker.rs)) +trips `Closed → Open` after `failure_threshold` **consecutive** `Failure` outcomes +([circuit_breaker.rs:232-243](../../../crates/adapter/net/http/api/src/circuit_breaker.rs#L232-L243)). +The load-bearing word is *consecutive*: a single `Success` resets the streak to zero +([:253-257](../../../crates/adapter/net/http/api/src/circuit_breaker.rs#L253-L257)). So +a venue failing **50 %** of requests in an interleaved `F S F S …` pattern **never +trips** — the streak never reaches the threshold. For a trading venue this is the worst +regime: a half-broken gateway keeps swallowing orders into ambiguity while every +intervening success silences the alarm. ADR-0031 §5 already anticipated this +(*"consecutive-count for v1; rolling-window later"*, +[0031 §5](../../adr/0031-http-resilience-venue-pacing.md#L135)); the deep-review §2B +ranked it the top resilience-detection hole. + +The existing anti-masking on `Class::Ignored` (a 4xx neither trips **nor resets**, +[:252](../../../crates/adapter/net/http/api/src/circuit_breaker.rs#L252)) is a *partial* +pre-emptive step. This spec generalizes it: by removing the reset-on-success cliff +entirely, interleaved **2xx** can no longer mask a building failure rate either. + +### Research grounding (prior art) + +- **resilience4j / Polly / Finagle / Envoy outlier detection** all reject + consecutive-count for exactly this reason and trip on a **failure rate over a sliding + window**, gated by a **minimum number of calls** (don't act on `1/1 = 100 %`). +- **`tower-resilience-circuitbreaker`** + ([joshrotenberg/tower-resilience](https://github.com/joshrotenberg/tower-resilience)) + is direct prior art: `failure_rate_threshold(0.5)` + `sliding_window_size(N)` + the + same three states, with named presets (`standard` = 50 %/100, `fast_fail` = 25 %/20, + `tolerant` = 75 %/200). We deliberately **do not adopt** it — it is built on + `tower::Service` (`poll_ready`, `&mut self`, associated `Future`), whereas OATH's + stack is the RPITIT `oath_adapter_net_api::Service` (`&self`, no `poll_ready`, no + `dyn`), which the deep-review §0 judged *superior for this use case*. Adopting it + would mean either a full tower rewrite (rejected by three clean-slate architects) or a + bridge shim more complex than the surgical change below — and its **generic** breaker + cannot express OATH's venue semantics (two open-durations by trip cause, `Retry-After` + honoring, the C1 local-`Throttled`-vs-venue-`429` `classify`). It is cited as prior + art confirming the window/rate/min-calls shape and preset framing. + +### Governing ADRs + +- **ADR-0031 §5** — the `CircuitBreaker` backstop; states the v1 consecutive-count with + *"rolling-window later"*. **Amendment #1** (C1) fixed `classify`; **Amendment #2** + (#117) added `Retry-After` honoring + the `retry_after_fallback`/`retry_after_cap` + fields. This feature is **ADR-0031 Amendment #3**. +- **ADR-0029** — `Timer` exposes only monotonic `now() -> Instant`. The count-based + window is **clock-free** and needs no new `Timer` surface (unlike the time-based + alternative — see D2). + +## Goal + +Replace the Closed-state trip trigger with a count-based rolling error-rate window: the +pure `Breaker` gains a small `RateWindow` in its `Closed` payload; `record` feeds each +`Failure`/`Success` into it and trips when the failure **rate** crosses a threshold once +a **minimum sample count** is present. `Open`, `HalfOpen`, the probe guard, `Retry-After` +honoring, and the single-per-host sharing all stay **exactly as they are**. One PR: a new +`RateWindow` unit, a `CircuitBreakerConfig` field swap, boot validation, an `open`-reason +telemetry label, tests, and an ADR amendment. **No new dependency; RPITIT/`&self`/no-`dyn` +Service preserved; `stack()`/`build()` signatures unchanged.** + +## Design decisions (locked) + +| # | Decision | Rationale | +| --- | --- | --- | +| **D1** | **Pure rate window replaces consecutive-count** (not a hybrid "rate OR N-consecutive"). | Single clean semantic + one config surface + one trip reason to reason about and table-test. Matches resilience4j/Polly/tower-resilience, which have no consecutive path. | +| **D2** | **Count-based window (last `N` outcomes)**, not time-based. | The window is **clock-free** — no monotonic-seconds plumbing derived from `Instant` (the time-based design's one genuinely new seam). Fully deterministic to table-test. It also *trips* a low-volume-but-persistently-failing venue (the ring fills over time), where a time-based window might never reach `minimum_calls` within its span. | +| **D3** | **Accepted trade-off of D2: no idle-time self-heal.** A count-based ring forgets only via **new calls**, not the clock, so a stale failure patch lingers until fresh successes flush it (slow recovery registration at low volume). | Bounded in practice: within a `Closed` episode the window either trips (→ `HalfOpen` → **reset to a clean slate**, D8) or is flushed by new traffic. Acceptable for a venue backstop; documented in the ADR so it is a deliberate choice, not an oversight. | +| **D4** | **Sample set = `Failure` (failure + call) and `Success` (call only).** `Class::Ignored` (4xx/Auth/Unknown) **never enters** the window; a venue `429` (`Class::TripNow`) trips **immediately** and is **not** a window sample. | The rate reflects only transport + 5xx host-health. Preserves today's anti-masking (a 4xx can neither trip nor dilute) and matches resilience4j's `ignoreExceptions`. `429`/penalty-box is an explicit venue directive, orthogonal to silent degradation. | +| **D5** | **Trip test:** `len ≥ minimum_calls && failures * 100 ≥ failure_rate_threshold * len`. Integer cross-multiply, **no float**; boundary is `≥` (rate *equal to* threshold trips). | Deterministic, cheap, and keeps float out of a resilience path. A rate trip reopens on the normal `cooldown` (unchanged) — the two open-durations by cause (rate → `cooldown`; `429` → `retry_after_*`) are preserved. | +| **D6** | **`CircuitBreakerConfig` swap:** drop `failure_threshold`; add `failure_rate_threshold: u8`, `window_size: NonZeroU32`, `minimum_calls: NonZeroU32`. Keep `cooldown`, `retry_after_fallback`, `retry_after_cap`, `half_open_probes`. | Breaking config change is cheap pre-release. `NonZeroU32` carries "≥ 1" for the two counts as a type invariant, as the two existing count fields already do. | +| **D7** | **Validate at boot in `stack::validate_config`**, *not* by making `CircuitBreakerLayer::new` fallible. Checks: `failure_rate_threshold ∈ 1..=100`; `minimum_calls ≤ window_size`. | Consistent with where the breaker's sibling `retry_after_*` zero-checks already live (#117) and #113's Duration validation — one boot gate, no constructor-signature churn across callers. | +| **D8** | **`RateWindow` resets to empty on `HalfOpen → Closed`.** | A recovered host earns a clean slate; a pre-trip failure patch must not instantly re-trip it. | +| **D9** | **Fixed inline ring; O(1) push; zero per-request allocation.** | Matches the codebase's no-alloc-hot-path ethos (M8, fixed in #111). The window lives once inside the single `Arc>`; `admit`/`record` mutate in place. | +| **D10** | **Single global per-host breaker unchanged.** Per-key breakers stay a **separate** #102 item. | ADR-0031 §5: IBKR's penalty box is per-IP/venue-wide. Windowing is per-breaker regardless of how many breakers exist. | +| **D11** | **Telemetry:** add a `reason` label to the `to="open"` transition (`reason ∈ {rate, throttle, probe_failed, abandoned}`). **Defer** a continuous failure-rate gauge (YAGNI v1). | The deep-review §2C flagged breaker-transition observability; a rate-degradation trip and a `429` penalty-box trip demand different operator responses and today look identical. A gauge needs periodic sampling; the trip transition is what you alert on. | + +## Architecture + +The pure `Breaker` state machine is unchanged **except** the `Closed` payload; `Open`, +`HalfOpen`, `admit`, the probe guard, `on_abandoned_probe`, and `Retry-After` all carry +over verbatim. + +```text +BreakerState::Closed { consecutive_failures: u32 } ─────► Closed { window: RateWindow } + +record(class, now, retry_after): + Closed: + Failure → window.push(Failure); if window.should_trip(cfg) → Open{cooldown} (reason=rate) + Success → window.push(Success) (no reset cliff) + Ignored → no-op (never a sample — anti-masking preserved) + TripNow → Open{ min(retry_after, cap) or fallback } (reason=throttle) + HalfOpen / Open: unchanged (probe budget, reopen, Retry-After) + HalfOpen → Closed on probe success: window = RateWindow::empty() (D8) +``` + +### New unit — `RateWindow` + +Its own small, table-testable type (feed it a sequence of outcomes; assert `rate()` / +`should_trip()`), isolated from the state machine exactly as the pure `Breaker` is +isolated from the async shell. + +```rust +/// A fixed-capacity ring of the last `N` breaker-relevant outcomes, tracking the +/// running failure rate. Clock-free: only `Failure`/`Success` enter (a 4xx/Auth is +/// never pushed; a 429 trips immediately elsewhere). Resets to empty on recovery. +struct RateWindow { + slots: Box<[Outcome]>, // capacity = window_size, allocated ONCE with the breaker + head: usize, // next write index (ring) + len: u32, // fills 0..=window_size + failures: u32, // running count over the live slots +} + +#[derive(Clone, Copy, PartialEq, Eq)] +enum Outcome { Failure, Success } + +impl RateWindow { + /// Push one outcome (O(1)); evict the oldest once full, keeping `failures` exact. + fn push(&mut self, o: Outcome) { + if self.len == self.slots.len() as u32 { + if self.slots[self.head] == Outcome::Failure { self.failures -= 1; } + } else { + self.len += 1; + } + if o == Outcome::Failure { self.failures += 1; } + self.slots[self.head] = o; + self.head = (self.head + 1) % self.slots.len(); + } + + /// Trip iff enough samples AND rate ≥ threshold. Integer cross-multiply (D5). + fn should_trip(&self, min_calls: u32, threshold_pct: u32) -> bool { + self.len >= min_calls && self.failures * 100 >= threshold_pct * self.len + } +} +``` + +> `slots` is a `Box<[Outcome]>` sized from `window_size` at construction — a **single** +> allocation living inside `Arc>`, never per request. (An `enum Outcome` +> is one byte; `window_size = 50` ⇒ 50 bytes for the whole venue.) The exact indexing +> shown is illustrative — the implementation must satisfy the no-indexing lint (helper +> accessors / iterators), per the repo's `#[deny]` posture. + +### Config change — `CircuitBreakerConfig` + +```rust +pub struct CircuitBreakerConfig { + pub failure_rate_threshold: u8, // NEW: trip at ≥ this % (1..=100), validated at boot + pub window_size: NonZeroU32, // NEW: N = ring capacity (last-N outcomes) + pub minimum_calls: NonZeroU32, // NEW: floor before the rate can trip (≤ window_size) + pub cooldown: Duration, // rate-trip reopen (UNCHANGED) + pub retry_after_fallback: Duration, // 429 reopen w/o header (UNCHANGED, Amendment #2) + pub retry_after_cap: Duration, // 429 Retry-After ceiling (UNCHANGED, Amendment #2) + pub half_open_probes: NonZeroU32, // (UNCHANGED) +} +``` + +`failure_threshold` is **removed**. `stack::validate_config` +([stack.rs:118](../../../crates/adapter/net/http/api/src/stack.rs#L118)) adds +`failure_rate_threshold ∈ 1..=100` and `minimum_calls ≤ window_size` checks alongside +the existing `retry_after_*` zero-checks (D7). `HttpConfig` nests +`CircuitBreakerConfig`, so `stack()`/`build()` signatures are unchanged; the field swap +is the only caller-visible break (pre-release, no external users). + +**v1 `build()` defaults** (tunable per deployment, not hardcoded): `failure_rate_threshold += 50`, `window_size = 50`, `minimum_calls = 10`. N = 50 ≈ 5 s of traffic at IBKR's +~10 req/s pacing — responsive for a backstop while giving a stable rate. Reference +profiles from tower-resilience: `standard` (50 %/100), `fast_fail` (25 %/20). + +### Telemetry — extend the #112 facade (D11) + +`record`/`on_abandoned_probe` surface *why* a transition to `Open` happened (derivable +from `(class, prior phase)`), so [meter.rs](../../../crates/adapter/net/http/api/src/meter.rs)'s +`breaker_transition` attaches a `reason` label on `to="open"` only: +`reason ∈ {rate, throttle, probe_failed, abandoned}` — low, bounded cardinality. The +existing `http_circuit_breaker_transitions_total{to}` counter is otherwise unchanged. +No continuous rate gauge in v1. + +## Testing + +TDD, table-first, in the existing inline `#[cfg(test)]` modules (`MockTimer` + +`ScriptLeaf`, as the current suite does). Each test must fail if its guard regresses. + +**`RateWindow` unit:** +- rate math: integer cross-multiply; boundary `failures*100 == threshold*len` trips (`≥`). +- ring eviction/wrap: pushing `> N` outcomes keeps `failures` exact and `len == N`. +- `minimum_calls` gate: below it, even `100 %` failures do **not** trip. +- all-failure saturation trips **exactly at** `minimum_calls`. + +**Pure `Breaker` (table tests):** +- **Motivating case:** an interleaved `F S F S …` 50 % venue **trips** once `len ≥ min_calls` + (the exact scenario consecutive-count missed). +- No-regression: a hard outage (`F F F …`) trips at `minimum_calls`; a healthy stream + never trips. +- `Ignored` burst: a flood of 4xx neither trips nor dilutes (not a sample). +- Reset-on-close: fail into a trip → probe succeeds → `Closed` with an **empty** window + (a pre-trip patch does not instantly re-trip). +- 429 `TripNow` still trips immediately on `retry_after_fallback`/honored value + (Amendment #2 regression guard), independent of window state. + +**Service (`service_tests`):** a `ScriptLeaf` sequence that produces a sub-threshold +mix stays `Pass`; a mix crossing the threshold fast-rejects (`CircuitOpen`) without +touching the leaf; a tripping-transition emits `to="open", reason="rate"`. + +## ADR impact + +- **ADR-0031 Amendment #3** (append-only, decision text unedited): consecutive-count → + count-based rolling error-rate window; the trip formula and sample-set semantics (D4/D5); + the config swap (D6) and boot validation (D7); the count-based **no-idle-self-heal** + trade-off (D3) as accepted; single global breaker unchanged (D10); the `open`-reason + telemetry label (D11); cites `tower-resilience-circuitbreaker` as prior art; + supersedes the `failure_threshold` field in §5's config sketch. + +## Scope (in) + +- New `RateWindow` unit in `net-http-api` + wiring into `BreakerState::Closed`. +- `record` Closed-arm rewrite (rate window in place of the streak); `Success` no longer + hard-resets; `Ignored` still a no-op; `TripNow`/`HalfOpen`/`Open` arms unchanged. +- Window reset on `HalfOpen → Closed`. +- `CircuitBreakerConfig` field swap + `stack::validate_config` checks + updated + `build()` defaults. +- `breaker_transition` `reason` label on `to="open"`. +- Tests (`RateWindow`, `Breaker`, service) + ADR-0031 Amendment #3 + `CHANGELOG.md` + `[Unreleased]`. + +## Non-goals (deferred — each its own follow-up) + +| Deferred | Why | +| --- | --- | +| **Time-based sliding window** (last `T` seconds) | Needs a monotonic-seconds seam derived from `Instant`; count-based (D2) is simpler, clock-free, and detects low-volume sustained failure. The config could grow a `window` enum later without another ADR. | +| **Per-key circuit breakers** | Separate #102 item; independent of windowing (D10). | +| **Hybrid rate-OR-consecutive fast-trip** | Rejected (D1) for semantic/config simplicity; `minimum_calls` low keeps hard-outage trip latency acceptable. | +| **Continuous failure-rate gauge** | YAGNI v1 (D11); the trip transition + reason is the alertable signal. | +| **Slow-call-rate threshold** (resilience4j) — treating slow-but-2xx calls as failures | `Timeout` already covers the hard-timeout case; slow-success accounting is a separate concern. | + +## Delivery + +One issue (split from tracking #102), one branch `feat/rolling-window-breaker` in a +worktree under `.claude/worktrees/`, one squash-merged PR (`Closes #`, references +#102). `just ci` + `just msrv` green; the spec and plan are committed on the feature +branch (per the one-issue-one-PR rule — not to `main`). + +## Open points for review + +1. **v1 defaults** — `50 % / N=50 / min_calls=10`. Match tower-resilience `standard` + (50 %/100) or lean `fast_fail` (25 %/20) instead? (Tunable, so low-stakes.) +2. **`reason` telemetry scope** — it's the one part touching the service shell (not just + the pure core). Keep it in this PR (recommended — the observability is the point) or + split it into a trivial follow-up to keep the core change minimal? From c773c5f21700a59a388d40b1f060ec4bd811deb1 Mon Sep 17 00:00:00 2001 From: NotAProfDev <84450364+NotAProfDev@users.noreply.github.com> Date: Thu, 9 Jul 2026 15:44:18 +0000 Subject: [PATCH 2/7] docs(net): rolling-window breaker implementation plan (#118) Co-Authored-By: Claude Opus 4.8 (1M context) --- ...6-07-09-net-http-rolling-window-breaker.md | 1007 +++++++++++++++++ 1 file changed, 1007 insertions(+) create mode 100644 docs/superpowers/plans/2026-07-09-net-http-rolling-window-breaker.md diff --git a/docs/superpowers/plans/2026-07-09-net-http-rolling-window-breaker.md b/docs/superpowers/plans/2026-07-09-net-http-rolling-window-breaker.md new file mode 100644 index 0000000..f396359 --- /dev/null +++ b/docs/superpowers/plans/2026-07-09-net-http-rolling-window-breaker.md @@ -0,0 +1,1007 @@ +# Rolling-Window (Error-Rate) Circuit Breaker — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Replace `CircuitBreaker`'s consecutive-count trip trigger with a count-based rolling error-rate window, so a venue failing a sustained fraction of interleaved traffic is detected (ADR-0031 Amendment #3). + +**Architecture:** A new clock-free `RateWindow` (ring of the last-N `Failure`/`Success` outcomes with a running failure count) lives in `BreakerState::Closed`. `Breaker::record` feeds each host-health outcome into it and trips `Open` when `failures/len ≥ threshold` once `len ≥ minimum_calls`. `Open`/`HalfOpen`/probe-guard/`Retry-After`/single-per-host sharing are unchanged. A `reason` label is added to the `to="open"` transition metric. + +**Tech Stack:** Rust (edition 2024), `oath-adapter-net-http-api` crate, `std::collections::VecDeque`, `metrics` facade, `just` task runner, `nextest`. + +## Global Constraints + +- Edition **2024**, MSRV **1.90** (validate with `just msrv`). +- **No `unsafe`** (`unsafe_code = "deny"`). +- **No `unwrap`/`expect`/indexing** in non-test code (warned) — return `Result`, model errors with `thiserror`. Test code is exempt. +- **Document public items** (`missing_docs` warned). `pub(crate)`/private items are exempt but doc them where it aids clarity. +- Clippy **`all` is deny-level** — no new warnings. +- **Conventional Commits** (`commit-msg` hook), e.g. `feat(net):`, `test(net):`, `docs(net):`. +- **Per-task verification** must run `just check`, `just test`, `just lint`, **and `just doc`** (rustdoc intra-doc links break silently otherwise). `just ci` + `just msrv` green before the PR. +- Work happens in the existing worktree `.claude/worktrees/rolling-window-breaker` on branch `feat/rolling-window-breaker`. One squash-merged PR, `Closes #118`, references #102. +- Tests are **inline `#[cfg(test)]` modules** (this repo uses no `tests/` dirs). + +--- + +### Task 1: `RateWindow` unit + +A standalone, clock-free rolling window. No dependency on the rest of the change; fully unit-tested on its own. + +**Files:** +- Create: `crates/adapter/net/http/api/src/rate_window.rs` +- Modify: `crates/adapter/net/http/api/src/lib.rs` (add `mod rate_window;`) + +**Interfaces:** +- Produces: `pub(crate) enum Outcome { Failure, Success }`; `pub(crate) struct RateWindow` with `fn new(window_size: NonZeroU32) -> Self`, `fn push(&mut self, o: Outcome)`, `fn reset(&mut self)`, `fn len(&self) -> u32`, `fn should_trip(&self, min_calls: u32, threshold_pct: u32) -> bool`. + +- [ ] **Step 1: Declare the module.** In `crates/adapter/net/http/api/src/lib.rs`, add the private module declaration next to the other private modules (`mod clock;`, `mod retry_after;`): + +```rust +mod rate_window; +``` + +- [ ] **Step 2: Write the failing tests.** Create `crates/adapter/net/http/api/src/rate_window.rs` with the tests first (the type does not exist yet): + +```rust +//! A fixed-capacity rolling window of recent breaker outcomes tracking the failure +//! rate for the error-rate trip policy (ADR-0031 Amendment #3). +//! +//! Clock-free: only host-health outcomes enter — a transport failure / `5xx` +//! ([`Outcome::Failure`]) or a reached-host `2xx`/`3xx` ([`Outcome::Success`]). A +//! `4xx`/`Auth` (`Class::Ignored`) is never pushed, and a venue `429` trips the breaker +//! immediately without a window sample. Reset to empty when the breaker recovers. + +use std::collections::VecDeque; +use std::num::NonZeroU32; + +/// One breaker-relevant, host-health-bearing outcome that enters the window. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum Outcome { + /// A transport failure (`Connection`/`Timeout`) or a `5xx` response. + Failure, + /// A reached-host success (`2xx`/`3xx`). + Success, +} + +/// The last-`N` outcomes as a ring, with a running failure count so the rate is O(1). +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct RateWindow { + cap: usize, + samples: VecDeque, + failures: u32, +} + +#[cfg(test)] +mod tests { + use super::{Outcome, RateWindow}; + use std::num::NonZeroU32; + + fn win(cap: u32) -> RateWindow { + RateWindow::new(NonZeroU32::new(cap).unwrap()) + } + + fn push_n(w: &mut RateWindow, o: Outcome, n: u32) { + for _ in 0..n { + w.push(o); + } + } + + #[test] + fn empty_window_never_trips() { + assert!(!win(50).should_trip(10, 50), "no samples < min_calls"); + } + + #[test] + fn below_min_calls_never_trips_even_at_full_failure() { + let mut w = win(50); + push_n(&mut w, Outcome::Failure, 9); // 100% failure, but only 9 < min_calls 10 + assert!(!w.should_trip(10, 50)); + } + + #[test] + fn all_failures_trips_exactly_at_min_calls() { + let mut w = win(50); + push_n(&mut w, Outcome::Failure, 9); + assert!(!w.should_trip(10, 50), "9 samples"); + w.push(Outcome::Failure); // 10th + assert!(w.should_trip(10, 50), "reached min_calls at 100%"); + } + + #[test] + fn interleaved_fifty_percent_trips_at_threshold_fifty() { + let mut w = win(50); + for _ in 0..10 { + w.push(Outcome::Failure); + w.push(Outcome::Success); + } // 10 F + 10 S = 20 samples, rate 50% + assert!( + w.should_trip(10, 50), + "50% failure rate meets the 50% threshold (>= trips)" + ); + } + + #[test] + fn just_below_threshold_does_not_trip() { + let mut w = win(100); + push_n(&mut w, Outcome::Failure, 49); + push_n(&mut w, Outcome::Success, 51); // 49/100 = 49% < 50% + assert!(!w.should_trip(10, 50)); + } + + #[test] + fn eviction_keeps_the_failure_count_exact() { + let mut w = win(10); + push_n(&mut w, Outcome::Failure, 10); // window full, all failures + assert!(w.should_trip(10, 50)); + push_n(&mut w, Outcome::Success, 10); // evicts all 10 failures + assert_eq!(w.len(), 10, "capacity holds"); + assert!(!w.should_trip(10, 50), "window is now all successes"); + } + + #[test] + fn reset_clears_to_empty() { + let mut w = win(50); + push_n(&mut w, Outcome::Failure, 20); + w.reset(); + assert_eq!(w.len(), 0); + assert!(!w.should_trip(1, 1)); + } +} +``` + +- [ ] **Step 3: Run the tests to verify they fail.** + +Run: `just test -p oath-adapter-net-http-api rate_window` +Expected: FAIL — `RateWindow::new`, `push`, `len`, `should_trip`, `reset` not found. + +- [ ] **Step 4: Implement `RateWindow`.** Add the `impl` block after the struct (before the `#[cfg(test)]` module): + +```rust +impl RateWindow { + /// An empty window of capacity `window_size`. The single backing allocation is + /// sized once here; `push` never reallocates (it evicts before exceeding `cap`). + pub(crate) fn new(window_size: NonZeroU32) -> Self { + let cap = window_size.get() as usize; + Self { + cap, + samples: VecDeque::with_capacity(cap), + failures: 0, + } + } + + /// Record one outcome (O(1)); evict the oldest once full, keeping `failures` exact. + pub(crate) fn push(&mut self, o: Outcome) { + if self.samples.len() == self.cap { + // Window full: drop the oldest. It was counted, so if it was a Failure the + // running count is >= 1 here and the decrement cannot underflow. + if self.samples.pop_front() == Some(Outcome::Failure) { + self.failures -= 1; + } + } + if o == Outcome::Failure { + self.failures += 1; + } + self.samples.push_back(o); + } + + /// Reset to empty — a recovered host earns a clean slate (ADR-0031 Amendment #3). + pub(crate) fn reset(&mut self) { + self.samples.clear(); + self.failures = 0; + } + + /// The current live sample count. + pub(crate) fn len(&self) -> u32 { + self.samples.len() as u32 + } + + /// Trip iff at least `min_calls` samples **and** failure rate >= `threshold_pct`. + /// Integer cross-multiply — no float in the resilience path; `>=` trips. Widen to + /// `u64` so `failures * 100` cannot overflow for a large `window_size`. + pub(crate) fn should_trip(&self, min_calls: u32, threshold_pct: u32) -> bool { + let len = self.len(); + len >= min_calls + && u64::from(self.failures) * 100 >= u64::from(threshold_pct) * u64::from(len) + } +} +``` + +- [ ] **Step 5: Run the tests to verify they pass.** + +Run: `just test -p oath-adapter-net-http-api rate_window` +Expected: PASS (7 tests). + +- [ ] **Step 6: Verify lint + doc.** + +Run: `just lint && just doc` +Expected: no warnings; docs build. + +- [ ] **Step 7: Commit.** + +```bash +git add crates/adapter/net/http/api/src/rate_window.rs crates/adapter/net/http/api/src/lib.rs +git commit -m "feat(net): add RateWindow rolling error-rate unit (ADR-0031 Am#3)" +``` + +--- + +### Task 2: Swap the breaker Closed-state to the error-rate window + +Atomic core change: the config field swap, boot validation, and the `Breaker` logic all move together (they share a compile unit). This is a refactor — write the new pure-`Breaker` tests that encode the target behavior, swap config + logic + every construction site + tests, and land green. + +**Files:** +- Modify: `crates/adapter/net/http/api/src/circuit_breaker.rs` (config struct, `Breaker`, module rustdoc, both test modules) +- Modify: `crates/adapter/net/http/api/src/rate.rs:126-152` (add two `BuildError` variants) +- Modify: `crates/adapter/net/http/api/src/stack.rs` (`validate_config` + doctest + test helper) +- Modify: `crates/adapter/net/http/hyper/src/build.rs` (doctest at :41 + `http_cfg` test at :127) +- Modify: `crates/adapter/net/http/hyper/examples/client_with_directives.rs:65` + +**Interfaces:** +- Consumes: `RateWindow`, `Outcome` from Task 1. +- Produces: `CircuitBreakerConfig { failure_rate_threshold: u8, window_size: NonZeroU32, minimum_calls: NonZeroU32, cooldown: Duration, retry_after_fallback: Duration, retry_after_cap: Duration, half_open_probes: NonZeroU32 }` (no `failure_threshold`); `BuildError::RateThresholdRange(u8)` and `BuildError::MinCallsExceedWindow(u32, u32)`; `Breaker::record` unchanged signature (`record(&mut self, class: Class, now: Instant, retry_after: Option)`). + +- [ ] **Step 1: Swap the config struct.** In `circuit_breaker.rs`, replace the `failure_threshold` field ([:40-56](../../../crates/adapter/net/http/api/src/circuit_breaker.rs#L40-L56)). New struct: + +```rust +/// The circuit breaker's thresholds, as plain `Copy` data (ADR-0031 §5, Amendment #3). +/// +/// `window_size`, `minimum_calls`, and `half_open_probes` are `NonZeroU32` ("≥ 1" is a +/// type invariant); `failure_rate_threshold` and the `minimum_calls ≤ window_size` +/// relationship are validated at boot by `stack::validate_config`. +#[derive(Debug, Clone, Copy)] +pub struct CircuitBreakerConfig { + /// Failure-rate percentage (`1..=100`) that trips the circuit over the rolling + /// window; a 50 % host trips at `50`. Validated at boot. + pub failure_rate_threshold: u8, + /// Rolling window size: the last-N outcomes the failure rate is computed over. + pub window_size: NonZeroU32, + /// Minimum window samples before the rate can trip (cold-start floor). Must be + /// `≤ window_size` (validated at boot). + pub minimum_calls: NonZeroU32, + /// The cooldown before Half-Open probing after a **rate** trip. + pub cooldown: Duration, + /// The `429` reopen wait when the response carries no usable `Retry-After` + /// (the penalty-box fallback; ≈ 10–15 min for IBKR) — Amendment #2. + pub retry_after_fallback: Duration, + /// Ceiling on an honored `429` `Retry-After`: `reopen = min(retry_after, cap)` — + /// Amendment #2. + pub retry_after_cap: Duration, + /// Probes admitted per Half-Open episode; all must reach the host to close. + pub half_open_probes: NonZeroU32, +} +``` + +- [ ] **Step 2: Import the window + swap the `Closed` payload.** Near the top of `circuit_breaker.rs`, add to the `use` block: + +```rust +use crate::rate_window::{Outcome, RateWindow}; +``` + +Change `BreakerState`'s derive ([:115](../../../crates/adapter/net/http/api/src/circuit_breaker.rs#L115)) to drop `Copy` (`RateWindow` is not `Copy`), and swap the `Closed` variant ([:116-127](../../../crates/adapter/net/http/api/src/circuit_breaker.rs#L116-L127)): + +```rust +#[derive(Debug, Clone, PartialEq, Eq)] +enum BreakerState { + /// Passing requests; `window` accumulates outcomes toward the rate trip. + Closed { window: RateWindow }, + /// Rejecting fast until `reopen_at`; then the next admit begins Half-Open. + Open { reopen_at: Instant }, + /// Probing: `probes_left` may still be admitted, `successes_needed` must reach + /// the host before the circuit closes. + HalfOpen { + probes_left: u32, + successes_needed: u32, + }, +} +``` + +- [ ] **Step 3: Update `Breaker::new` (drop `const`) and rewrite `record`.** `RateWindow::new` allocates, so `Breaker::new` can no longer be `const`. Replace `Breaker::new` ([:184-191](../../../crates/adapter/net/http/api/src/circuit_breaker.rs#L184-L191)) and `record` ([:227-294](../../../crates/adapter/net/http/api/src/circuit_breaker.rs#L227-L294)): + +```rust +/// A fresh breaker starts Closed with an empty window. +pub(crate) fn new(cfg: CircuitBreakerConfig) -> Self { + Self { + state: BreakerState::Closed { + window: RateWindow::new(cfg.window_size), + }, + cfg, + } +} +``` + +```rust +/// Record a classified outcome, transitioning as ADR-0031 §5 (Amendment #3) dictates. +/// +/// In Closed, `Failure`/`Success` feed the rolling window and a `Failure` trips when +/// the rate crosses the threshold (with enough samples); `Ignored` is never a sample; +/// a `429` `TripNow` trips immediately (unchanged). `retry_after` is consulted only in +/// the `TripNow` arms, clamped to `retry_after_cap`. +pub(crate) fn record(&mut self, class: Class, now: Instant, retry_after: Option) { + // Hoist config reads (all `Copy`) so the `&mut self.state` match below borrows only + // `state`, and compute the next state, applying it after the borrow ends. + let min_calls = self.cfg.minimum_calls.get(); + let threshold = u32::from(self.cfg.failure_rate_threshold); + let window_size = self.cfg.window_size; + let rate_reopen = deadline(now, self.cfg.cooldown); + let tripnow_reopen = deadline( + now, + retry_after.map_or(self.cfg.retry_after_fallback, |ra| { + ra.min(self.cfg.retry_after_cap) + }), + ); + + let next: Option = match &mut self.state { + BreakerState::Closed { window } => match class { + Class::Failure => { + window.push(Outcome::Failure); + window + .should_trip(min_calls, threshold) + .then_some(BreakerState::Open { + reopen_at: rate_reopen, + }) + }, + Class::Success => { + window.push(Outcome::Success); // dilutes the rate; no reset cliff + None + }, + Class::Ignored => None, // a 4xx/Auth is not a host-health sample + Class::TripNow => Some(BreakerState::Open { + reopen_at: tripnow_reopen, + }), + }, + BreakerState::HalfOpen { + probes_left, + successes_needed, + } => match class { + Class::Failure => Some(BreakerState::Open { + reopen_at: rate_reopen, + }), + Class::TripNow => Some(BreakerState::Open { + reopen_at: tripnow_reopen, + }), + // A reached-host probe (2xx/3xx or 4xx/Auth) resolves; the last one closes + // to a fresh window. + Class::Ignored | Class::Success => Some(if *successes_needed <= 1 { + BreakerState::Closed { + window: RateWindow::new(window_size), + } + } else { + BreakerState::HalfOpen { + probes_left: *probes_left, + successes_needed: *successes_needed - 1, + } + }), + }, + // A stale outcome from a call admitted before a concurrent trip; drop it. + BreakerState::Open { .. } => None, + }; + if let Some(state) = next { + self.state = state; + } +} +``` + +> `admit`, `on_abandoned_probe`, and `phase` are **unchanged** — they already match `&mut self.state` / read discriminants only, which works for the now-non-`Copy` state. + +- [ ] **Step 4: Update the module rustdoc.** In `circuit_breaker.rs`, the crate-module doc ([:1-22](../../../crates/adapter/net/http/api/src/circuit_breaker.rs#L1-L22)) references `failure_threshold`/"consecutive" — the intra-doc link would break `just doc`. Replace the first sentences of the module doc: + +```rust +//! The `CircuitBreaker` resilience layer (ADR-0031 §5, Amendment #3): the reactive +//! 429/outage backstop to `RateLimit`'s proactive pacing. +//! +//! `RateLimit` tries never to hit a 429; `CircuitBreaker` stops cold if the host +//! fails anyway. It trips **Open** when the **failure rate** over the last +//! [`CircuitBreakerConfig::window_size`] outcomes reaches +//! [`CircuitBreakerConfig::failure_rate_threshold`] (once at least +//! [`CircuitBreakerConfig::minimum_calls`] samples are present), or **immediately** on a +//! venue **429 response** with the long [`CircuitBreakerConfig::retry_after_fallback`] +//! (IBKR's ~15-minute penalty box). A `Throttled` *error* and a `4xx`/`Auth` are local +//! or client-side and never enter the window. +``` + +Also fix the `Closed` variant doc comment ([:117](../../../crates/adapter/net/http/api/src/circuit_breaker.rs#L117)) — done in Step 2 above. + +- [ ] **Step 5: Add the `BuildError` variants.** In `rate.rs`, inside `enum BuildError` ([:128-152](../../../crates/adapter/net/http/api/src/rate.rs#L128-L152)), before the closing brace, add: + +```rust + /// `circuit_breaker.failure_rate_threshold` is outside `1..=100` — a `0` would trip + /// on the first sample and a value `> 100` could never trip. + #[error("config field `circuit_breaker.failure_rate_threshold` must be in 1..=100, but is {0}")] + RateThresholdRange(u8), + /// `circuit_breaker.minimum_calls` (`{0}`) exceeds `window_size` (`{1}`) — the window + /// can never hold enough samples to reach the floor, so the breaker could never trip + /// on rate. + #[error("config field `circuit_breaker.minimum_calls` ({0}) must be <= `window_size` ({1})")] + MinCallsExceedWindow(u32, u32), +``` + +- [ ] **Step 6: Write the failing `validate_config` tests.** In `stack.rs`'s `#[cfg(test)] mod tests`, add (after the existing config tests): + +```rust +#[test] +fn rejects_a_zero_failure_rate_threshold() { + let mut cfg = http_cfg_for_validation(); + cfg.circuit_breaker.failure_rate_threshold = 0; + assert_eq!( + stack(Leaf, cfg, MockTimer::new(), NoAuth, total_rates()).err(), + Some(BuildError::RateThresholdRange(0)), + ); +} + +#[test] +fn rejects_an_over_100_failure_rate_threshold() { + let mut cfg = http_cfg_for_validation(); + cfg.circuit_breaker.failure_rate_threshold = 101; + assert_eq!( + stack(Leaf, cfg, MockTimer::new(), NoAuth, total_rates()).err(), + Some(BuildError::RateThresholdRange(101)), + ); +} + +#[test] +fn rejects_min_calls_greater_than_window() { + let mut cfg = http_cfg_for_validation(); + cfg.circuit_breaker.window_size = NonZeroU32::new(10).unwrap(); + cfg.circuit_breaker.minimum_calls = NonZeroU32::new(11).unwrap(); + assert_eq!( + stack(Leaf, cfg, MockTimer::new(), NoAuth, total_rates()).err(), + Some(BuildError::MinCallsExceedWindow(11, 10)), + ); +} +``` + +> Use the existing test's `Leaf`, `total_rates()`, and a small helper `http_cfg_for_validation()` that returns a valid `HttpConfig` — add it in this module mirroring the existing valid config already used by the passing `stack` test, with the new breaker fields (Step 8). If a valid `HttpConfig` builder already exists in this module, reuse it instead of adding one. + +- [ ] **Step 7: Run the validation tests to verify they fail.** + +Run: `just test -p oath-adapter-net-http-api validation` +Expected: FAIL — variants unused / config still has `failure_threshold` (won't compile until Step 8). + +- [ ] **Step 8: Extend `validate_config` and swap every construction site.** In `stack.rs`, add the two checks to the `const fn validate_config` ([:192-208](../../../crates/adapter/net/http/api/src/stack.rs#L192-L208)) before `Ok(())`: + +```rust + if cfg.circuit_breaker.failure_rate_threshold == 0 + || cfg.circuit_breaker.failure_rate_threshold > 100 + { + return Err(BuildError::RateThresholdRange( + cfg.circuit_breaker.failure_rate_threshold, + )); + } + 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(), + )); + } +``` + +Then replace the `failure_threshold: NonZeroU32::new(...).unwrap(),` line with the three new fields at **every** construction site. Use the recommended v1 profile `50 % / N=50 / min_calls=10` for the doc/example sites: + +```rust + failure_rate_threshold: 50, + window_size: NonZeroU32::new(50).unwrap(), + minimum_calls: NonZeroU32::new(10).unwrap(), +``` + +Sites (all currently `failure_threshold: NonZeroU32::new(3).unwrap()` unless noted): +- `stack.rs:129-135` (doctest in `stack()` doc) +- `stack.rs` `tests` module config helper (the `circuit_breaker: CircuitBreakerConfig { ... }` in `mod tests`) +- `build.rs:41` (single-line doctest — replace `failure_threshold: NonZeroU32::new(3).unwrap()` with the three fields inline) +- `build.rs:127-133` (`http_cfg()` test helper) +- `hyper/examples/client_with_directives.rs:65` +- `circuit_breaker.rs:345-352` (the `CircuitBreakerLayer::new` doctest) + +- [ ] **Step 9: Rewrite the breaker test helpers + tests.** In `circuit_breaker.rs` `mod breaker_tests`, replace the `cfg(threshold, probes)` helper ([:611-619](../../../crates/adapter/net/http/api/src/circuit_breaker.rs#L611-L619)) with two helpers: + +```rust +// General rate config for policy-specific tests. +fn rate_cfg(threshold_pct: u8, window: u32, min_calls: u32, probes: u32) -> CircuitBreakerConfig { + CircuitBreakerConfig { + failure_rate_threshold: threshold_pct, + window_size: NonZeroU32::new(window).unwrap(), + minimum_calls: NonZeroU32::new(min_calls).unwrap(), + cooldown: Duration::from_secs(30), + retry_after_fallback: Duration::from_secs(900), + retry_after_cap: Duration::from_secs(1800), + half_open_probes: NonZeroU32::new(probes).unwrap(), + } +} + +// A config that trips on the FIRST failure (100% over a 1-sample window) — the analogue +// of the old `failure_threshold = 1`, for the Open/HalfOpen/probe tests whose behavior +// is independent of the trip policy. +fn first(probes: u32) -> CircuitBreakerConfig { + rate_cfg(100, 1, 1, probes) +} +``` + +Apply this **mechanical substitution** to the existing tests (behavior unchanged — only the trip-policy config differs): + +| Test (`mod breaker_tests`) | Old call | New call | +| --- | --- | --- | +| `throttle_trips_immediately_on_the_long_cooldown` | `cfg(3, 1)` | `first(1)` | +| `open_rejects_until_cooldown_then_admits_one_probe` | `cfg(1, 1)` | `first(1)` | +| `half_open_probe_success_closes` | `cfg(1, 1)` | `first(1)` | +| `half_open_probe_ignored_also_closes` | `cfg(1, 1)` | `first(1)` | +| `half_open_probe_failure_reopens` | `cfg(1, 1)` | `first(1)` | +| `half_open_probe_429_reopens_on_the_long_cooldown` | `cfg(1, 1)` | `first(1)` | +| `multi_probe_half_open_requires_all_to_close` | `cfg(1, 2)` | `first(2)` | +| `abandoned_probe_reopens_half_open` | `cfg(1, 1)` | `first(1)` | +| `abandoned_probe_is_a_noop_in_open` | `cfg(1, 1)` | `first(1)` | +| `record_while_open_never_untrips` | `cfg(1, 1)` | `first(1)` | +| `admit_distinguishes_a_probe_from_a_normal_pass` | `cfg(1, 1)` | `first(1)` | +| `a_429_retry_after_reopens_on_the_honored_value` | `cfg(1, 1)` | `first(1)` | +| `a_429_retry_after_is_clamped_to_the_cap` | `cfg(1, 1)` | `first(1)` | +| `a_429_without_retry_after_uses_the_fallback` | `cfg(1, 1)` | `first(1)` | + +**Delete** these three now-obsolete consecutive-count tests: `closed_trips_after_threshold_consecutive_failures`, `a_success_resets_the_failure_streak`, `ignored_does_not_reset_the_streak`. + +**Reframe** `abandoned_probe_is_a_noop_in_closed` (its intent — an abandoned call must not advance the Closed accumulator — survives) to: + +```rust +#[test] +fn abandoned_probe_is_a_noop_in_closed() { + let now = Instant::now(); + // Trip at 100% over a 3-sample window (min_calls 3). + let mut b = Breaker::new(rate_cfg(100, 3, 3, 1)); + b.record(Class::Failure, now, None); // 1 sample + b.record(Class::Failure, now, None); // 2 samples, < min 3 → not tripped + b.on_abandoned_probe(now); // must NOT add a sample + assert_eq!( + b.admit(now), + Admit::Pass, + "2 real failures < min_calls 3 — abandon was a no-op" + ); + b.record(Class::Failure, now, None); // 3rd real failure → 100% of 3 → trips + assert_eq!(b.admit(now), Admit::Reject, "3rd real failure → tripped"); +} +``` + +- [ ] **Step 10: Add the new rate-policy tests.** In `mod breaker_tests`, add: + +```rust +#[test] +fn closed_trips_when_failure_rate_reaches_threshold() { + let now = Instant::now(); + // 50% over a 20-window, min_calls 10. + let mut b = Breaker::new(rate_cfg(50, 20, 10, 1)); + for _ in 0..5 { + b.record(Class::Success, now, None); + b.record(Class::Failure, now, None); + } // 5F + 5S = 10 samples, 50% + assert_eq!( + b.admit(now), + Admit::Reject, + "50% over 10 samples meets the 50% threshold" + ); +} + +#[test] +fn interleaved_successes_do_not_prevent_a_rate_trip() { + // The motivating case: consecutive-count never tripped this; the rate window does. + let now = Instant::now(); + let mut b = Breaker::new(rate_cfg(50, 20, 10, 1)); + for _ in 0..10 { + b.record(Class::Failure, now, None); + b.record(Class::Success, now, None); // a success no longer resets an alarm + } + assert_eq!(b.admit(now), Admit::Reject, "sustained 50% degradation trips"); +} + +#[test] +fn below_min_calls_never_trips() { + let now = Instant::now(); + let mut b = Breaker::new(rate_cfg(50, 20, 10, 1)); + for _ in 0..9 { + b.record(Class::Failure, now, None); // 100% but only 9 < min 10 + } + assert_eq!(b.admit(now), Admit::Pass, "under the min-calls floor"); +} + +#[test] +fn ignored_is_not_a_window_sample() { + let now = Instant::now(); + let mut b = Breaker::new(rate_cfg(50, 20, 10, 1)); + for _ in 0..30 { + b.record(Class::Ignored, now, None); // a flood of 4xx: neither trips nor counts + } + assert_eq!(b.admit(now), Admit::Pass, "4xx never enters the window"); +} + +#[test] +fn window_resets_to_a_clean_slate_on_close() { + let now = Instant::now(); + let mut b = Breaker::new(first(1)); // trips on the first failure + b.record(Class::Failure, now, None); // → Open + let after = now + Duration::from_secs(30); + assert_eq!(b.admit(after), Admit::Probe); + b.record(Class::Success, after, None); // probe closes → fresh empty window + // The pre-trip failure did not carry over: one new failure at 100%/1 trips again, + // proving the window is a clean slate (not still holding the old failure). + assert_eq!(b.admit(after), Admit::Pass, "closed with an empty window"); +} +``` + +- [ ] **Step 11: Update the `service_tests` helper.** In `mod service_tests`, replace the `cfg(threshold, cooldown, fallback, probes)` helper ([:1014-1027](../../../crates/adapter/net/http/api/src/circuit_breaker.rs#L1014-L1027)) so its first parameter drives the rate policy. Keep the same call arity by mapping `threshold` → a "trip after `threshold` consecutive failures at 100%" config (window = threshold, min_calls = threshold, rate = 100): + +```rust +fn cfg( + trip_after: u32, // consecutive failures needed at 100% (window = min_calls = trip_after) + cooldown: Duration, + fallback: Duration, + probes: u32, +) -> CircuitBreakerConfig { + CircuitBreakerConfig { + failure_rate_threshold: 100, + window_size: NonZeroU32::new(trip_after).unwrap(), + minimum_calls: NonZeroU32::new(trip_after).unwrap(), + cooldown, + retry_after_fallback: fallback, + retry_after_cap: Duration::from_secs(1800), + half_open_probes: NonZeroU32::new(probes).unwrap(), + } +} +``` + +> This preserves every existing `service_tests` behavior: `cfg(3, …)` still trips after 3 straight failures (3/3 = 100% ≥ 100%), `cfg(2, …)` after 2, `cfg(1, …)` after 1 — because with no interleaved successes the rate stays 100%. No `service_tests` bodies change; only the helper does. + +- [ ] **Step 12: Run the full crate tests.** + +Run: `just test -p oath-adapter-net-http-api` +Expected: PASS. If any `service_tests` fails, it relied on a success *not* resetting — re-check against the helper mapping above. + +- [ ] **Step 13: Full check + lint + doc.** + +Run: `just check && just lint && just doc` +Expected: clean (the hyper crate's doctest/example + `build.rs` compile with the new fields). + +- [ ] **Step 14: Commit.** + +```bash +git add crates/adapter/net/http/api/src/circuit_breaker.rs crates/adapter/net/http/api/src/rate.rs crates/adapter/net/http/api/src/stack.rs crates/adapter/net/http/hyper/src/build.rs crates/adapter/net/http/hyper/examples/client_with_directives.rs +git commit -m "feat(net)!: rolling error-rate breaker replaces consecutive-count (ADR-0031 Am#3)" +``` + +--- + +### Task 3: `open`-transition `reason` telemetry + +Attach *why* the breaker opened, so a rate-degradation trip is distinguishable from a 429 penalty-box trip. + +**Files:** +- Modify: `crates/adapter/net/http/api/src/circuit_breaker.rs` (`record`/`on_abandoned_probe` return a reason; three service call sites) +- Modify: `crates/adapter/net/http/api/src/meter.rs:60-63` + its test + +**Interfaces:** +- Consumes: `Breaker::record`, `Breaker::on_abandoned_probe` from Task 2. +- Produces: `pub(crate) enum TripReason { Rate, Throttle, ProbeFailed, Abandoned }` with `const fn label(self) -> &'static str`; `record` and `on_abandoned_probe` return `Option`; `meter::breaker_transition(to: &'static str, reason: Option<&'static str>)`. + +- [ ] **Step 1: Write the failing meter test.** In `meter.rs` `mod tests`, replace `breaker_transition_increments_a_phase_labelled_counter` so it asserts the `reason` label: + +```rust +#[test] +fn breaker_transition_carries_to_and_optional_reason() { + let recorder = DebuggingRecorder::new(); + let snap = recorder.snapshotter(); + metrics::with_local_recorder(&recorder, || { + breaker_transition("open", Some("rate")); + breaker_transition("half_open", None); + }); + let snapshot = snap.snapshot().into_vec(); + let opened = snapshot.iter().any(|(k, _, _, v)| { + k.key().name() == "http_circuit_breaker_transitions_total" + && k.key().labels().any(|l| l.key() == "to" && l.value() == "open") + && k.key().labels().any(|l| l.key() == "reason" && l.value() == "rate") + && matches!(v, DebugValue::Counter(n) if *n == 1) + }); + assert!(opened, "to=open carries reason=rate"); +} +``` + +- [ ] **Step 2: Run it to verify it fails.** + +Run: `just test -p oath-adapter-net-http-api breaker_transition` +Expected: FAIL — `breaker_transition` takes one argument. + +- [ ] **Step 3: Extend the meter fn.** In `meter.rs`, replace `breaker_transition` ([:60-63](../../../crates/adapter/net/http/api/src/meter.rs#L60-L63)): + +```rust +/// Record a circuit-breaker phase transition into `to`, with an optional `reason` +/// (present only for `to = "open"`: `"rate" | "throttle" | "probe_failed" | "abandoned"`). +pub(crate) fn breaker_transition(to: &'static str, reason: Option<&'static str>) { + match reason { + Some(reason) => { + metrics::counter!(CIRCUIT_TRANSITIONS, "to" => to, "reason" => reason).increment(1); + }, + None => metrics::counter!(CIRCUIT_TRANSITIONS, "to" => to).increment(1), + } +} +``` + +- [ ] **Step 4: Return a reason from the breaker core.** In `circuit_breaker.rs`, add the enum near `Phase`: + +```rust +/// Why the breaker transitioned to `Open` — a low-cardinality telemetry reason. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum TripReason { + /// The rolling failure rate crossed the threshold. + Rate, + /// A venue `429` response (`TripNow`) — the penalty box. + Throttle, + /// A Half-Open probe failed. + ProbeFailed, + /// A Half-Open probe was abandoned (cancelled / panicked). + Abandoned, +} + +impl TripReason { + /// The stable, low-cardinality telemetry label. + const fn label(self) -> &'static str { + match self { + Self::Rate => "rate", + Self::Throttle => "throttle", + Self::ProbeFailed => "probe_failed", + Self::Abandoned => "abandoned", + } + } +} +``` + +Change `record` to return `Option` (Some only when it enters `Open`). Update the arms from Task 2: the Closed `Failure` rate-trip yields `Some(TripReason::Rate)`, Closed `TripNow` and HalfOpen `TripNow` yield `Some(TripReason::Throttle)`, HalfOpen `Failure` yields `Some(TripReason::ProbeFailed)`; all non-opening arms yield `None`. Concretely, make each arm return `(Option, Option)` — or simpler, compute `next: Option` as before and derive the reason alongside: + +```rust +pub(crate) fn record( + &mut self, + class: Class, + now: Instant, + retry_after: Option, +) -> Option { + let min_calls = self.cfg.minimum_calls.get(); + let threshold = u32::from(self.cfg.failure_rate_threshold); + let window_size = self.cfg.window_size; + let rate_reopen = deadline(now, self.cfg.cooldown); + let tripnow_reopen = deadline( + now, + retry_after.map_or(self.cfg.retry_after_fallback, |ra| { + ra.min(self.cfg.retry_after_cap) + }), + ); + + // (next state, trip reason). Reason is Some only when entering Open. + let (next, reason): (Option, Option) = match &mut self.state { + BreakerState::Closed { window } => match class { + Class::Failure => { + window.push(Outcome::Failure); + if window.should_trip(min_calls, threshold) { + ( + Some(BreakerState::Open { reopen_at: rate_reopen }), + Some(TripReason::Rate), + ) + } else { + (None, None) + } + }, + Class::Success => { + window.push(Outcome::Success); + (None, None) + }, + Class::Ignored => (None, None), + Class::TripNow => ( + Some(BreakerState::Open { reopen_at: tripnow_reopen }), + Some(TripReason::Throttle), + ), + }, + BreakerState::HalfOpen { + probes_left, + successes_needed, + } => match class { + Class::Failure => ( + Some(BreakerState::Open { reopen_at: rate_reopen }), + Some(TripReason::ProbeFailed), + ), + Class::TripNow => ( + Some(BreakerState::Open { reopen_at: tripnow_reopen }), + Some(TripReason::Throttle), + ), + Class::Ignored | Class::Success => ( + Some(if *successes_needed <= 1 { + BreakerState::Closed { window: RateWindow::new(window_size) } + } else { + BreakerState::HalfOpen { + probes_left: *probes_left, + successes_needed: *successes_needed - 1, + } + }), + None, + ), + }, + BreakerState::Open { .. } => (None, None), + }; + if let Some(state) = next { + self.state = state; + } + reason +} +``` + +Change `on_abandoned_probe` ([:304-310](../../../crates/adapter/net/http/api/src/circuit_breaker.rs#L304-L310)) to return `Option`: + +```rust +pub(crate) fn on_abandoned_probe(&mut self, now: Instant) -> Option { + if matches!(self.state, BreakerState::HalfOpen { .. }) { + self.state = BreakerState::Open { + reopen_at: deadline(now, self.cfg.cooldown), + }; + Some(TripReason::Abandoned) + } else { + None + } +} +``` + +- [ ] **Step 5: Thread the reason through the three service call sites.** In `circuit_breaker.rs`, the three `breaker_transition(to)` calls become two-arg. The `record` site ([:530-542](../../../crates/adapter/net/http/api/src/circuit_breaker.rs#L530-L542)) captures the reason: + +```rust +let (transition, reason) = { + let now = self.timer.now(); + let mut breaker = self + .breaker + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let before = breaker.phase(); + let reason = breaker.record(class, now, retry_after); + (transition_label(before, breaker.phase()), reason) +}; +if let Some(to) = transition { + crate::meter::breaker_transition(to, reason.map(super::TripReason::label)); +} +``` + +The `admit` site ([:489-501](../../../crates/adapter/net/http/api/src/circuit_breaker.rs#L489-L501)) can only enter Half-Open (never Open), so it passes `None`: + +```rust +if let Some(to) = transition { + crate::meter::breaker_transition(to, None); +} +``` + +The `ProbeGuard::drop` site ([:455-466](../../../crates/adapter/net/http/api/src/circuit_breaker.rs#L455-L466)) captures the abandoned reason: + +```rust +let (transition, reason) = { + let mut breaker = self + .breaker + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let before = breaker.phase(); + let reason = breaker.on_abandoned_probe(now); + (transition_label(before, breaker.phase()), reason) +}; +if let Some(to) = transition { + crate::meter::breaker_transition(to, reason.map(super::TripReason::label)); +} +``` + +> `super::TripReason::label` is a `fn(TripReason) -> &'static str` used as `Option::map`'s closure. If the call sites are inside a nested `impl`, reference it as `TripReason::label` with a `use super::TripReason;` already in scope (`TripReason` is defined at module level). + +- [ ] **Step 6: Update the existing `service_tests` reason assertion.** The `tripping_the_breaker_emits_an_open_transition_metric` test ([:1055-1079](../../../crates/adapter/net/http/api/src/circuit_breaker.rs#L1055-L1079)) trips via a `Connection` error → a **rate** trip (with `cfg(1, …)` = trip at first failure). Add a `reason=rate` assertion to it: + +```rust + let opened = snap.snapshot().into_vec().into_iter().any(|(k, _, _, v)| { + k.key().name() == "http_circuit_breaker_transitions_total" + && k.key().labels().any(|l| l.key() == "to" && l.value() == "open") + && k.key().labels().any(|l| l.key() == "reason" && l.value() == "rate") + && matches!(v, DebugValue::Counter(n) if n >= 1) + }); + assert!(opened, "a rate trip emits to=open, reason=rate"); +``` + +- [ ] **Step 7: Run tests + lint + doc.** + +Run: `just test -p oath-adapter-net-http-api && just lint && just doc` +Expected: PASS, clean. + +- [ ] **Step 8: Commit.** + +```bash +git add crates/adapter/net/http/api/src/circuit_breaker.rs crates/adapter/net/http/api/src/meter.rs +git commit -m "feat(net): label breaker open-transitions with a trip reason" +``` + +--- + +### Task 4: ADR-0031 Amendment #3, CHANGELOG, final CI + +**Files:** +- Modify: `docs/adr/0031-http-resilience-venue-pacing.md` (append Amendment #3) +- Modify: `CHANGELOG.md` (`[Unreleased]`) + +- [ ] **Step 1: Append ADR-0031 Amendment #3.** After Amendment #2 in `docs/adr/0031-http-resilience-venue-pacing.md`, add: + +```markdown +3. **Consecutive-count trip → count-based rolling error-rate window.** §5 shipped the + breaker with `failure_threshold` consecutive failures (*"consecutive-count for v1; + rolling-window later"*). That is blind to mixed-traffic degradation — a single + `Success` resets the streak, so a venue failing ~50 % of interleaved requests never + trips (deep-review §2B). **Changed:** in Closed, `CircuitBreaker` now trips when the + **failure rate** over the last `window_size` host-health outcomes reaches + `failure_rate_threshold` (%), once at least `minimum_calls` samples are present — + `len ≥ minimum_calls && failures*100 ≥ failure_rate_threshold*len` (integer, `≥` + trips). Only `Failure` (transport/`5xx`) and `Success` (`2xx`/`3xx`) enter the window; + a `4xx`/`Auth` (`Class::Ignored`) is never a sample, and a venue `429` (`TripNow`) + still trips **immediately** on `retry_after_fallback`/honored value (Amendment #2, + unchanged). The window is **count-based** (clock-free), chosen over time-based to + avoid a monotonic-seconds `Timer` seam; the accepted trade-off is **no idle-time + self-heal** — a stale failure patch lingers until flushed by new calls or a trip, + which resets the window to a clean slate on recovery. `Open`, `HalfOpen`, the + probe-guard, and single-per-host sharing are unchanged; per-key breakers remain + deferred (#102). **Config:** `CircuitBreakerConfig` drops `failure_threshold` and + gains `failure_rate_threshold: u8`, `window_size: NonZeroU32`, `minimum_calls: + NonZeroU32`, validated at boot (`failure_rate_threshold ∈ 1..=100`, `minimum_calls ≤ + window_size`). The `to="open"` transition metric gains a `reason` label + (`rate`/`throttle`/`probe_failed`/`abandoned`). Prior art: resilience4j / Polly / + `tower-resilience-circuitbreaker` (rate + sliding window + minimum-calls); **not** + adopted — it is built on `tower::Service`, whereas OATH keeps its RPITIT `&self` + `Service`. Spec: + `docs/superpowers/specs/2026-07-09-net-http-rolling-window-breaker-design.md`. +``` + +- [ ] **Step 2: Add the CHANGELOG entry.** Under `## [Unreleased]` → `### Changed` in `CHANGELOG.md`, add: + +```markdown +- **Breaking (pre-release) — net-http circuit-breaker trip policy.** The `CircuitBreaker` + now trips on a **rolling error-rate window** (last-N outcomes) rather than consecutive + failures, so a venue failing a sustained fraction of interleaved traffic is detected + (a 50 %-error host no longer resets an alarm on every success) — ADR-0031 Amendment #3. + `CircuitBreakerConfig` drops `failure_threshold` and gains `failure_rate_threshold` + (percent, `1..=100`), `window_size`, and `minimum_calls` (validated at boot). The + `http_circuit_breaker_transitions_total{to="open"}` metric gains a `reason` label + (`rate`/`throttle`/`probe_failed`/`abandoned`). Prior art: tower-resilience (not + adopted — OATH keeps its RPITIT `Service`). +``` + +- [ ] **Step 3: Full local CI + MSRV.** + +Run: `just ci && just msrv` +Expected: all green (fmt, lint, test, doc, deny, typos, MSRV). + +- [ ] **Step 4: Commit.** + +```bash +git add docs/adr/0031-http-resilience-venue-pacing.md CHANGELOG.md +git commit -m "docs(net): record ADR-0031 Amendment #3 + CHANGELOG (rolling-window breaker)" +``` + +- [ ] **Step 5: Push and open the PR.** + +```bash +git push -u origin feat/rolling-window-breaker +gh pr create --fill --base main \ + --title "feat(net)!: rolling-window (error-rate) circuit breaker (ADR-0031 Am#3)" \ + --body "Closes #118. Replaces the CircuitBreaker's consecutive-count trip trigger with a count-based rolling error-rate window. References #102. + +🤖 Generated with [Claude Code](https://claude.com/claude-code)" +``` + +--- + +## Self-Review + +**Spec coverage** (each spec decision → task): +- D1 pure rate window (no hybrid) → Task 2 Step 3 (`record` rewrite). +- D2 count-based → Task 1 (`RateWindow`, `VecDeque`, clock-free). +- D3 no-idle-self-heal accepted → documented, Task 4 Step 1 (ADR). +- D4 sample set (Failure/Success in; Ignored out; 429 immediate) → Task 2 Steps 3, 10 (`ignored_is_not_a_window_sample`). +- D5 integer trip math, `≥` → Task 1 Step 4 (`should_trip`), Task 1 Step 2 (boundary test). +- D6 config swap → Task 2 Steps 1, 8. +- D7 boot validation → Task 2 Steps 5–8. +- D8 reset-on-close → Task 2 Step 3 (HalfOpen→Closed arm), Step 10 (`window_resets_to_a_clean_slate_on_close`). +- D9 zero-alloc ring → Task 1 (`VecDeque::with_capacity`, evict-before-exceed). +- D10 single global breaker unchanged → no layer/sharing change (untouched). +- D11 `reason` telemetry → Task 3. +- ADR-0031 Amendment #3 + CHANGELOG → Task 4. +- Motivating 50 %-venue test → Task 2 Step 10 (`interleaved_successes_do_not_prevent_a_rate_trip`). +- Regression guards (429 immediate, Retry-After) → preserved by Task 2 substitution table (`first(1)` tests). + +**Placeholder scan:** none — all code shown; the only `<...>` is the PR number resolved by `#118`. + +**Type consistency:** `RateWindow::{new,push,reset,len,should_trip}` and `Outcome::{Failure,Success}` are consistent across Tasks 1–2; `record(&mut self, Class, Instant, Option) -> Option` (Task 3) and `on_abandoned_probe(&mut self, Instant) -> Option` match their three call sites; `breaker_transition(&'static str, Option<&'static str>)` matches all call sites and the meter test; `TripReason::label(self) -> &'static str` matches the `.map(...)` usage. From 0bd71b7ce47a2ba46dc8485fb34aaf772a492424 Mon Sep 17 00:00:00 2001 From: NotAProfDev <84450364+NotAProfDev@users.noreply.github.com> Date: Thu, 9 Jul 2026 16:00:38 +0000 Subject: [PATCH 3/7] feat(net): add RateWindow rolling error-rate unit (ADR-0031 Am#3) --- crates/adapter/net/http/api/src/lib.rs | 1 + .../adapter/net/http/api/src/rate_window.rs | 166 ++++++++++++++++++ 2 files changed, 167 insertions(+) create mode 100644 crates/adapter/net/http/api/src/rate_window.rs diff --git a/crates/adapter/net/http/api/src/lib.rs b/crates/adapter/net/http/api/src/lib.rs index 9453f6f..1d09f78 100644 --- a/crates/adapter/net/http/api/src/lib.rs +++ b/crates/adapter/net/http/api/src/lib.rs @@ -40,6 +40,7 @@ pub mod error; pub mod meter; pub mod rate; pub mod rate_limit; +mod rate_window; pub mod retry; mod retry_after; pub mod service; diff --git a/crates/adapter/net/http/api/src/rate_window.rs b/crates/adapter/net/http/api/src/rate_window.rs new file mode 100644 index 0000000..1312bf7 --- /dev/null +++ b/crates/adapter/net/http/api/src/rate_window.rs @@ -0,0 +1,166 @@ +//! A fixed-capacity rolling window of recent breaker outcomes tracking the failure +//! rate for the error-rate trip policy (ADR-0031 Amendment #3). +//! +//! Clock-free: only host-health outcomes enter — a transport failure / `5xx` +//! ([`Outcome::Failure`]) or a reached-host `2xx`/`3xx` ([`Outcome::Success`]). A +//! `4xx`/`Auth` (`Class::Ignored`) is never pushed, and a venue `429` trips the breaker +//! immediately without a window sample. A recovered host earns a fresh window via +//! [`RateWindow::new`]. +//! +//! Not yet wired into [`crate::circuit_breaker`] — that lands in the next commit on +//! this branch, which is why the `#[expect(dead_code, …)]` below exists. + +// Standalone unit, landed ahead of its call site (`Breaker::record`, next commit on +// this branch) so it can be built and reviewed test-first in isolation. `expect` (not +// `allow`) makes the suppression self-clearing: once `circuit_breaker.rs` consumes the +// items in the non-test build the lint stops firing and `expect` reports itself as +// unfulfilled, forcing its own removal in Task 2. Scoped to `not(test)` because the +// unit tests below already exercise every item, so `dead_code` never fires in the test +// build — an unscoped `expect` there would be unfulfilled today. +#![cfg_attr( + not(test), + expect( + dead_code, + reason = "not yet wired into circuit_breaker.rs; removed once RateWindow/Outcome are consumed in Task 2" + ) +)] + +use std::collections::VecDeque; +use std::num::NonZeroU32; + +/// One breaker-relevant, host-health-bearing outcome that enters the window. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +// `pub(crate)` in a private module: clippy's nursery `redundant_pub_crate` wants +// `pub`, but that would trip the workspace `unreachable_pub` lint instead (a `pub` +// item not reachable outside the crate). `pub(crate)` states the true visibility; +// silence the losing alternative, matching `clock.rs`/`retry_after.rs`. +#[allow(clippy::redundant_pub_crate)] +pub(crate) enum Outcome { + /// A transport failure (`Connection`/`Timeout`) or a `5xx` response. + Failure, + /// A reached-host success (`2xx`/`3xx`). + Success, +} + +/// The last-`N` outcomes as a ring, with a running failure count so the rate is O(1). +#[derive(Debug, Clone, PartialEq, Eq)] +#[allow(clippy::redundant_pub_crate)] // see `Outcome` above +pub(crate) struct RateWindow { + cap: usize, + samples: VecDeque, + failures: u32, +} + +impl RateWindow { + /// An empty window of capacity `window_size`. The single backing allocation is + /// sized once here; `push` never reallocates (it evicts before exceeding `cap`). + pub(crate) fn new(window_size: NonZeroU32) -> Self { + let cap = window_size.get() as usize; + Self { + cap, + samples: VecDeque::with_capacity(cap), + failures: 0, + } + } + + /// Record one outcome (O(1)); evict the oldest once full, keeping `failures` exact. + pub(crate) fn push(&mut self, o: Outcome) { + if self.samples.len() == self.cap { + // Window full: drop the oldest. It was counted, so if it was a Failure the + // running count is >= 1 here and the decrement cannot underflow. + if self.samples.pop_front() == Some(Outcome::Failure) { + self.failures -= 1; + } + } + if o == Outcome::Failure { + self.failures += 1; + } + self.samples.push_back(o); + } + + /// The current live sample count. + // `cap` (and so `samples.len()`, which never exceeds it — `push` evicts before + // growing past `cap`) originates from `NonZeroU32::get()` in `new`, so the value + // always fits back in a `u32`; the cast cannot truncate. + #[allow(clippy::cast_possible_truncation)] + pub(crate) fn len(&self) -> u32 { + self.samples.len() as u32 + } + + /// Trip iff at least `min_calls` samples **and** failure rate >= `threshold_pct`. + /// Integer cross-multiply — no float in the resilience path; `>=` trips. Widen to + /// `u64` so `failures * 100` cannot overflow for a large `window_size`. + pub(crate) fn should_trip(&self, min_calls: u32, threshold_pct: u32) -> bool { + let len = self.len(); + len >= min_calls + && u64::from(self.failures) * 100 >= u64::from(threshold_pct) * u64::from(len) + } +} + +#[cfg(test)] +mod tests { + use super::{Outcome, RateWindow}; + use std::num::NonZeroU32; + + fn win(cap: u32) -> RateWindow { + RateWindow::new(NonZeroU32::new(cap).unwrap()) + } + + fn push_n(w: &mut RateWindow, o: Outcome, n: u32) { + for _ in 0..n { + w.push(o); + } + } + + #[test] + fn empty_window_never_trips() { + assert!(!win(50).should_trip(10, 50), "no samples < min_calls"); + } + + #[test] + fn below_min_calls_never_trips_even_at_full_failure() { + let mut w = win(50); + push_n(&mut w, Outcome::Failure, 9); // 100% failure, but only 9 < min_calls 10 + assert!(!w.should_trip(10, 50)); + } + + #[test] + fn all_failures_trips_exactly_at_min_calls() { + let mut w = win(50); + push_n(&mut w, Outcome::Failure, 9); + assert!(!w.should_trip(10, 50), "9 samples"); + w.push(Outcome::Failure); // 10th + assert!(w.should_trip(10, 50), "reached min_calls at 100%"); + } + + #[test] + fn interleaved_fifty_percent_trips_at_threshold_fifty() { + let mut w = win(50); + for _ in 0..10 { + w.push(Outcome::Failure); + w.push(Outcome::Success); + } // 10 F + 10 S = 20 samples, rate 50% + assert!( + w.should_trip(10, 50), + "50% failure rate meets the 50% threshold (>= trips)" + ); + } + + #[test] + fn just_below_threshold_does_not_trip() { + let mut w = win(100); + push_n(&mut w, Outcome::Failure, 49); + push_n(&mut w, Outcome::Success, 51); // 49/100 = 49% < 50% + assert!(!w.should_trip(10, 50)); + } + + #[test] + fn eviction_keeps_the_failure_count_exact() { + let mut w = win(10); + push_n(&mut w, Outcome::Failure, 10); // window full, all failures + assert!(w.should_trip(10, 50)); + push_n(&mut w, Outcome::Success, 10); // evicts all 10 failures + assert_eq!(w.len(), 10, "capacity holds"); + assert!(!w.should_trip(10, 50), "window is now all successes"); + } +} From e3754f13456e74825a6d7694d1191da7314dc73e Mon Sep 17 00:00:00 2001 From: NotAProfDev <84450364+NotAProfDev@users.noreply.github.com> Date: Thu, 9 Jul 2026 16:34:41 +0000 Subject: [PATCH 4/7] feat(net)!: rolling error-rate breaker replaces consecutive-count 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. --- .../net/http/api/src/circuit_breaker.rs | 322 ++++++++++-------- crates/adapter/net/http/api/src/rate.rs | 9 + .../adapter/net/http/api/src/rate_window.rs | 19 +- crates/adapter/net/http/api/src/stack.rs | 76 ++++- .../hyper/examples/client_with_directives.rs | 4 +- crates/adapter/net/http/hyper/src/build.rs | 10 +- 6 files changed, 275 insertions(+), 165 deletions(-) diff --git a/crates/adapter/net/http/api/src/circuit_breaker.rs b/crates/adapter/net/http/api/src/circuit_breaker.rs index ef6154e..f01c52d 100644 --- a/crates/adapter/net/http/api/src/circuit_breaker.rs +++ b/crates/adapter/net/http/api/src/circuit_breaker.rs @@ -1,13 +1,14 @@ -//! The `CircuitBreaker` resilience layer (ADR-0031 §5): the reactive 429/outage -//! backstop to `RateLimit`'s proactive pacing. +//! The `CircuitBreaker` resilience layer (ADR-0031 §5, Amendment #3): the reactive +//! 429/outage backstop to `RateLimit`'s proactive pacing. //! //! `RateLimit` tries never to hit a 429; `CircuitBreaker` stops cold if the host -//! fails anyway. It trips **Open** after [`CircuitBreakerConfig::failure_threshold`] -//! consecutive transport failures (`HttpError::{Connection, Timeout}` or a `5xx` -//! response), or **immediately** on a venue **429 response** with the long -//! [`CircuitBreakerConfig::retry_after_fallback`] (IBKR's ~15-minute penalty box). A -//! `Throttled` *error* is a local pacing decision (the request was never sent) and -//! is ignored — it never trips the breaker. +//! fails anyway. It trips **Open** when the **failure rate** over the last +//! [`CircuitBreakerConfig::window_size`] outcomes reaches +//! [`CircuitBreakerConfig::failure_rate_threshold`] (once at least +//! [`CircuitBreakerConfig::minimum_calls`] samples are present), or **immediately** on a +//! venue **429 response** with the long [`CircuitBreakerConfig::retry_after_fallback`] +//! (IBKR's ~15-minute penalty box). A `Throttled` *error* and a `4xx`/`Auth` are local +//! or client-side and never enter the window. //! While Open it **fast-rejects** every request with a non-retryable //! [`HttpError::CircuitOpen`] — the inner stack is //! never touched. After the cooldown a bounded number of **Half-Open** probes test @@ -22,6 +23,7 @@ //! — `http::Response` is forwarded untouched. use crate::clock::deadline; +use crate::rate_window::{Outcome, RateWindow}; use crate::{HttpError, Service}; use bytes::Bytes; use oath_adapter_net_api::{ErrorKind, HasErrorKind, Layer, Timer}; @@ -31,25 +33,28 @@ use std::num::NonZeroU32; use std::sync::{Arc, Mutex}; use std::time::{Duration, Instant}; -/// The circuit breaker's thresholds, as plain `Copy` data (ADR-0031 §5). +/// The circuit breaker's thresholds, as plain `Copy` data (ADR-0031 §5, Amendment #3). /// -/// `failure_threshold` and `half_open_probes` are `NonZeroU32`: "≥ 1" is a type -/// invariant, so [`CircuitBreakerLayer::new`] needs no -/// `Result` (a `0` threshold is nonsense and `0` probes would leave a tripped -/// circuit stuck Open forever). This types §5's `u32` sketch more precisely. +/// `window_size`, `minimum_calls`, and `half_open_probes` are `NonZeroU32` ("≥ 1" is a +/// type invariant); `failure_rate_threshold` and the `minimum_calls ≤ window_size` +/// relationship are validated at boot by `stack::validate_config`. #[derive(Debug, Clone, Copy)] pub struct CircuitBreakerConfig { - /// Consecutive failures in the Closed state that trip the circuit Open. - pub failure_threshold: NonZeroU32, - /// The cooldown before Half-Open probing after a failure-threshold trip. + /// Failure-rate percentage (`1..=100`) that trips the circuit over the rolling + /// window; a 50 % host trips at `50`. Validated at boot. + pub failure_rate_threshold: u8, + /// Rolling window size: the last-N outcomes the failure rate is computed over. + pub window_size: NonZeroU32, + /// Minimum window samples before the rate can trip (cold-start floor). Must be + /// `≤ window_size` (validated at boot). + pub minimum_calls: NonZeroU32, + /// The cooldown before Half-Open probing after a **rate** trip. pub cooldown: Duration, - /// The `429` reopen wait when the response carries **no** usable `Retry-After` - /// (the penalty-box fallback; ≈ 10–15 min for IBKR). Renamed for clarity - /// (ADR-0031 Amendment #2). + /// The `429` reopen wait when the response carries no usable `Retry-After` + /// (the penalty-box fallback; ≈ 10–15 min for IBKR) — Amendment #2. pub retry_after_fallback: Duration, - /// Ceiling on an **honored** `429` `Retry-After`: `reopen = min(retry_after, cap)`. - /// May be set `≥ retry_after_fallback` to honor a directive *longer* than the - /// default box; also bounds a hostile/absurd `Retry-After` (ADR-0031 Amendment #2). + /// Ceiling on an honored `429` `Retry-After`: `reopen = min(retry_after, cap)` — + /// Amendment #2. pub retry_after_cap: Duration, /// Probes admitted per Half-Open episode; all must reach the host to close. pub half_open_probes: NonZeroU32, @@ -112,10 +117,10 @@ pub(crate) fn classify(outcome: &Result, HttpError>) -> Cla /// The breaker's state (ADR-0031 §5). `Instant` deadlines are compared against /// `Timer::now()` by the async shell — the core itself never reads a clock. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq)] enum BreakerState { - /// Passing requests; `consecutive_failures` counts toward the trip threshold. - Closed { consecutive_failures: u32 }, + /// Passing requests; `window` accumulates outcomes toward the rate trip. + Closed { window: RateWindow }, /// Rejecting fast until `reopen_at`; then the next admit begins Half-Open. Open { reopen_at: Instant }, /// Probing: `probes_left` may still be admitted, `successes_needed` must reach @@ -180,11 +185,11 @@ pub(crate) struct Breaker { } impl Breaker { - /// A fresh breaker starts Closed with no failures. - pub(crate) const fn new(cfg: CircuitBreakerConfig) -> Self { + /// A fresh breaker starts Closed with an empty window. + pub(crate) fn new(cfg: CircuitBreakerConfig) -> Self { Self { state: BreakerState::Closed { - consecutive_failures: 0, + window: RateWindow::new(cfg.window_size), }, cfg, } @@ -219,77 +224,73 @@ impl Breaker { } } - /// Record a classified outcome, transitioning as ADR-0031 §5 dictates. + /// Record a classified outcome, transitioning as ADR-0031 §5 (Amendment #3) dictates. /// - /// `retry_after` is the delay-seconds value parsed from a `429` response's - /// `Retry-After` header (Amendment #2); it is consulted only in the - /// [`Class::TripNow`] arms below, clamped to `retry_after_cap`. + /// In Closed, `Failure`/`Success` feed the rolling window and a `Failure` trips when + /// the rate crosses the threshold (with enough samples); `Ignored` is never a sample; + /// a `429` `TripNow` trips immediately (unchanged). `retry_after` is consulted only in + /// the `TripNow` arms, clamped to `retry_after_cap`. pub(crate) fn record(&mut self, class: Class, now: Instant, retry_after: Option) { - match self.state { - BreakerState::Closed { - consecutive_failures, - } => match class { + // Hoist config reads (all `Copy`) so the `&mut self.state` match below borrows only + // `state`, and compute the next state, applying it after the borrow ends. + let min_calls = self.cfg.minimum_calls.get(); + let threshold = u32::from(self.cfg.failure_rate_threshold); + let window_size = self.cfg.window_size; + let rate_reopen = deadline(now, self.cfg.cooldown); + let tripnow_reopen = deadline( + now, + retry_after.map_or(self.cfg.retry_after_fallback, |ra| { + ra.min(self.cfg.retry_after_cap) + }), + ); + + let next: Option = match &mut self.state { + BreakerState::Closed { window } => match class { Class::Failure => { - let n = consecutive_failures.saturating_add(1); - self.state = if n >= self.cfg.failure_threshold.get() { - BreakerState::Open { - reopen_at: deadline(now, self.cfg.cooldown), - } - } else { - BreakerState::Closed { - consecutive_failures: n, - } - }; + window.push(Outcome::Failure); + window + .should_trip(min_calls, threshold) + .then_some(BreakerState::Open { + reopen_at: rate_reopen, + }) }, - Class::TripNow => { - let cooldown = retry_after.map_or(self.cfg.retry_after_fallback, |ra| { - ra.min(self.cfg.retry_after_cap) - }); - self.state = BreakerState::Open { - reopen_at: deadline(now, cooldown), - }; - }, - Class::Ignored => {}, // streak untouched — a 4xx/Auth neither trips nor resets Class::Success => { - self.state = BreakerState::Closed { - consecutive_failures: 0, - }; + window.push(Outcome::Success); // dilutes the rate; no reset cliff + None }, + Class::Ignored => None, // a 4xx/Auth is not a host-health sample + Class::TripNow => Some(BreakerState::Open { + reopen_at: tripnow_reopen, + }), }, BreakerState::HalfOpen { probes_left, successes_needed, } => match class { - Class::Failure => { - self.state = BreakerState::Open { - reopen_at: deadline(now, self.cfg.cooldown), - }; - }, - Class::TripNow => { - let cooldown = retry_after.map_or(self.cfg.retry_after_fallback, |ra| { - ra.min(self.cfg.retry_after_cap) - }); - self.state = BreakerState::Open { - reopen_at: deadline(now, cooldown), - }; - }, - // A reached-host probe (2xx/3xx or 4xx/Auth) resolves; the last one closes. - Class::Ignored | Class::Success => { - self.state = if successes_needed <= 1 { - BreakerState::Closed { - consecutive_failures: 0, - } - } else { - BreakerState::HalfOpen { - probes_left, - successes_needed: successes_needed - 1, - } - }; - }, + Class::Failure => Some(BreakerState::Open { + reopen_at: rate_reopen, + }), + Class::TripNow => Some(BreakerState::Open { + reopen_at: tripnow_reopen, + }), + // A reached-host probe (2xx/3xx or 4xx/Auth) resolves; the last one closes + // to a fresh window. + Class::Ignored | Class::Success => Some(if *successes_needed <= 1 { + BreakerState::Closed { + window: RateWindow::new(window_size), + } + } else { + BreakerState::HalfOpen { + probes_left: *probes_left, + successes_needed: *successes_needed - 1, + } + }), }, // A stale outcome from a call admitted before a concurrent trip; drop it. - // Never un-trips a freshly-opened circuit (single global v1 breaker). - BreakerState::Open { .. } => {}, + BreakerState::Open { .. } => None, + }; + if let Some(state) = next { + self.state = state; } } @@ -343,7 +344,9 @@ impl CircuitBreakerLayer { /// use std::time::Duration; /// /// let cfg = CircuitBreakerConfig { - /// failure_threshold: NonZeroU32::new(3).unwrap(), + /// failure_rate_threshold: 50, + /// window_size: NonZeroU32::new(50).unwrap(), + /// minimum_calls: NonZeroU32::new(10).unwrap(), /// cooldown: Duration::from_secs(30), /// retry_after_fallback: Duration::from_secs(900), /// retry_after_cap: Duration::from_secs(1800), @@ -608,9 +611,17 @@ mod breaker_tests { use std::num::NonZeroU32; use std::time::{Duration, Instant}; - fn cfg(threshold: u32, probes: u32) -> CircuitBreakerConfig { + // General rate config for policy-specific tests. + fn rate_cfg( + threshold_pct: u8, + window: u32, + min_calls: u32, + probes: u32, + ) -> CircuitBreakerConfig { CircuitBreakerConfig { - failure_threshold: NonZeroU32::new(threshold).unwrap(), + failure_rate_threshold: threshold_pct, + window_size: NonZeroU32::new(window).unwrap(), + minimum_calls: NonZeroU32::new(min_calls).unwrap(), cooldown: Duration::from_secs(30), retry_after_fallback: Duration::from_secs(900), retry_after_cap: Duration::from_secs(1800), @@ -618,53 +629,89 @@ mod breaker_tests { } } + // A config that trips on the FIRST failure (100% over a 1-sample window) — the analogue + // of the old `failure_threshold = 1`, for the Open/HalfOpen/probe tests whose behavior + // is independent of the trip policy. + fn first(probes: u32) -> CircuitBreakerConfig { + rate_cfg(100, 1, 1, probes) + } + #[test] - fn closed_trips_after_threshold_consecutive_failures() { + fn closed_trips_when_failure_rate_reaches_threshold() { let now = Instant::now(); - let mut b = Breaker::new(cfg(3, 1)); - assert_eq!(b.admit(now), Admit::Pass); - b.record(Class::Failure, now, None); - b.record(Class::Failure, now, None); - assert_eq!(b.admit(now), Admit::Pass, "still closed after 2 failures"); - b.record(Class::Failure, now, None); + // 50% over a 20-window, min_calls 10. + let mut b = Breaker::new(rate_cfg(50, 20, 10, 1)); + for _ in 0..5 { + b.record(Class::Success, now, None); + b.record(Class::Failure, now, None); + } // 5F + 5S = 10 samples, 50% assert_eq!( b.admit(now), Admit::Reject, - "3rd consecutive failure → Open rejects" + "50% over 10 samples meets the 50% threshold" ); } #[test] - fn a_success_resets_the_failure_streak() { + fn interleaved_successes_do_not_prevent_a_rate_trip() { + // The motivating case: consecutive-count never tripped this; the rate window does. let now = Instant::now(); - let mut b = Breaker::new(cfg(3, 1)); - b.record(Class::Failure, now, None); - b.record(Class::Failure, now, None); - b.record(Class::Success, now, None); // reset - b.record(Class::Failure, now, None); - b.record(Class::Failure, now, None); - assert_eq!(b.admit(now), Admit::Pass, "streak reset → not tripped"); + let mut b = Breaker::new(rate_cfg(50, 20, 10, 1)); + for _ in 0..10 { + b.record(Class::Failure, now, None); + b.record(Class::Success, now, None); // a success no longer resets an alarm + } + assert_eq!( + b.admit(now), + Admit::Reject, + "sustained 50% degradation trips" + ); } #[test] - fn ignored_does_not_reset_the_streak() { + fn below_min_calls_never_trips() { let now = Instant::now(); - let mut b = Breaker::new(cfg(3, 1)); - b.record(Class::Failure, now, None); - b.record(Class::Ignored, now, None); // a 4xx does NOT reset — anti-masking - b.record(Class::Failure, now, None); - b.record(Class::Failure, now, None); // 3rd failure overall → trips + let mut b = Breaker::new(rate_cfg(50, 20, 10, 1)); + for _ in 0..9 { + b.record(Class::Failure, now, None); // 100% but only 9 < min 10 + } + assert_eq!(b.admit(now), Admit::Pass, "under the min-calls floor"); + } + + #[test] + fn ignored_is_not_a_window_sample() { + let now = Instant::now(); + let mut b = Breaker::new(rate_cfg(50, 20, 10, 1)); + for _ in 0..30 { + b.record(Class::Ignored, now, None); // a flood of 4xx: neither trips nor counts + } + assert_eq!(b.admit(now), Admit::Pass, "4xx never enters the window"); + } + + #[test] + fn window_resets_to_a_clean_slate_on_close() { + let now = Instant::now(); + // Trips at 2/2 failures (100% over a 2-sample window). + let mut b = Breaker::new(rate_cfg(100, 2, 2, 1)); + b.record(Class::Failure, now, None); // [F] — 1 < min 2, no trip + b.record(Class::Failure, now, None); // [F,F] — 100% of 2 → Open + let after = now + Duration::from_secs(30); + assert_eq!(b.admit(after), Admit::Probe); + b.record(Class::Success, after, None); // probe closes → FRESH empty window + // One failure into a fresh window is 1/1 = 100% but only 1 sample < min 2 → no trip. + // Had the pre-trip [F,F] carried over, this failure would keep it tripping. + b.record(Class::Failure, after, None); assert_eq!( - b.admit(now), - Admit::Reject, - "ignored left the streak intact → trips" + b.admit(after), + Admit::Pass, + "fresh window after close: one failure is below min_calls, so no trip" ); } #[test] fn throttle_trips_immediately_on_the_long_cooldown() { let now = Instant::now(); - let mut b = Breaker::new(cfg(3, 1)); + let mut b = Breaker::new(first(1)); b.record(Class::TripNow, now, None); // one throttle → Open, no threshold needed assert_eq!(b.admit(now), Admit::Reject); assert_eq!( @@ -682,7 +729,7 @@ mod breaker_tests { #[test] fn open_rejects_until_cooldown_then_admits_one_probe() { let now = Instant::now(); - let mut b = Breaker::new(cfg(1, 1)); // trips on the first failure + let mut b = Breaker::new(first(1)); // trips on the first failure b.record(Class::Failure, now, None); assert_eq!(b.admit(now), Admit::Reject); let after = now + Duration::from_secs(30); @@ -701,7 +748,7 @@ mod breaker_tests { #[test] fn half_open_probe_success_closes() { let now = Instant::now(); - let mut b = Breaker::new(cfg(1, 1)); + let mut b = Breaker::new(first(1)); b.record(Class::Failure, now, None); let after = now + Duration::from_secs(30); assert_eq!(b.admit(after), Admit::Probe); @@ -712,7 +759,7 @@ mod breaker_tests { #[test] fn half_open_probe_ignored_also_closes() { let now = Instant::now(); - let mut b = Breaker::new(cfg(1, 1)); + let mut b = Breaker::new(first(1)); b.record(Class::Failure, now, None); let after = now + Duration::from_secs(30); assert_eq!(b.admit(after), Admit::Probe); @@ -727,7 +774,7 @@ mod breaker_tests { #[test] fn half_open_probe_failure_reopens() { let now = Instant::now(); - let mut b = Breaker::new(cfg(1, 1)); + let mut b = Breaker::new(first(1)); b.record(Class::Failure, now, None); let after = now + Duration::from_secs(30); assert_eq!(b.admit(after), Admit::Probe); @@ -747,7 +794,7 @@ mod breaker_tests { // NOT the 30s cooldown a failing probe would use. Distinguish the two: at // reopen+30s still Reject; only at reopen+900s does the next probe admit. let now = Instant::now(); - let mut b = Breaker::new(cfg(1, 1)); // trips on the first failure + let mut b = Breaker::new(first(1)); // trips on the first failure b.record(Class::Failure, now, None); // Closed → Open (30s cooldown) let probe_at = now + Duration::from_secs(30); assert_eq!(b.admit(probe_at), Admit::Probe, "cooldown elapsed → probe"); @@ -767,7 +814,7 @@ mod breaker_tests { #[test] fn multi_probe_half_open_requires_all_to_close() { let now = Instant::now(); - let mut b = Breaker::new(cfg(1, 2)); // 2 probes per episode + let mut b = Breaker::new(first(2)); // 2 probes per episode b.record(Class::Failure, now, None); let after = now + Duration::from_secs(30); assert_eq!(b.admit(after), Admit::Probe, "probe 1"); @@ -786,7 +833,7 @@ mod breaker_tests { #[test] fn abandoned_probe_reopens_half_open() { let now = Instant::now(); - let mut b = Breaker::new(cfg(1, 1)); + let mut b = Breaker::new(first(1)); b.record(Class::Failure, now, None); // → Open let after = now + Duration::from_secs(30); assert_eq!( @@ -810,23 +857,24 @@ mod breaker_tests { #[test] fn abandoned_probe_is_a_noop_in_closed() { let now = Instant::now(); - let mut b = Breaker::new(cfg(3, 1)); - b.record(Class::Failure, now, None); // streak = 1 - b.record(Class::Failure, now, None); // streak = 2 - b.on_abandoned_probe(now); // must NOT advance the streak + // Trip at 100% over a 3-sample window (min_calls 3). + let mut b = Breaker::new(rate_cfg(100, 3, 3, 1)); + b.record(Class::Failure, now, None); // 1 sample + b.record(Class::Failure, now, None); // 2 samples, < min 3 → not tripped + b.on_abandoned_probe(now); // must NOT add a sample assert_eq!( b.admit(now), Admit::Pass, - "2 real failures < threshold 3 — abandon was a no-op" + "2 real failures < min_calls 3 — abandon was a no-op" ); - b.record(Class::Failure, now, None); // the 3rd REAL failure trips it + b.record(Class::Failure, now, None); // 3rd real failure → 100% of 3 → trips assert_eq!(b.admit(now), Admit::Reject, "3rd real failure → tripped"); } #[test] fn abandoned_probe_is_a_noop_in_open() { let now = Instant::now(); - let mut b = Breaker::new(cfg(1, 1)); + let mut b = Breaker::new(first(1)); b.record(Class::Failure, now, None); // → Open { reopen_at: now + 30s } b.on_abandoned_probe(now + Duration::from_secs(5)); // must not push the deadline out assert_eq!( @@ -844,7 +892,7 @@ mod breaker_tests { #[test] fn record_while_open_never_untrips() { let now = Instant::now(); - let mut b = Breaker::new(cfg(1, 1)); + let mut b = Breaker::new(first(1)); b.record(Class::Failure, now, None); // → Open b.record(Class::Success, now, None); // a stale success from a pre-trip admit assert_eq!( @@ -857,7 +905,7 @@ mod breaker_tests { #[test] fn admit_distinguishes_a_probe_from_a_normal_pass() { let now = Instant::now(); - let mut b = Breaker::new(cfg(1, 1)); + let mut b = Breaker::new(first(1)); assert_eq!( b.admit(now), Admit::Pass, @@ -875,7 +923,7 @@ mod breaker_tests { #[test] fn a_429_retry_after_reopens_on_the_honored_value() { let now = Instant::now(); - let mut b = Breaker::new(cfg(1, 1)); // trips on the first outcome + let mut b = Breaker::new(first(1)); // trips on the first outcome // 429 carrying Retry-After: 2 → reopen at now+2s (honored), under the 900s // fallback and the 1800s cap. b.record(Class::TripNow, now, Some(Duration::from_secs(2))); @@ -894,7 +942,7 @@ mod breaker_tests { #[test] fn a_429_retry_after_is_clamped_to_the_cap() { let now = Instant::now(); - let mut b = Breaker::new(cfg(1, 1)); // retry_after_cap = 1800s + let mut b = Breaker::new(first(1)); // retry_after_cap = 1800s b.record(Class::TripNow, now, Some(Duration::from_secs(100_000))); // absurd assert_eq!( b.admit(now + Duration::from_secs(1799)), @@ -911,7 +959,7 @@ mod breaker_tests { #[test] fn a_429_without_retry_after_uses_the_fallback() { let now = Instant::now(); - let mut b = Breaker::new(cfg(1, 1)); // retry_after_fallback = 900s + let mut b = Breaker::new(first(1)); // retry_after_fallback = 900s b.record(Class::TripNow, now, None); assert_eq!( b.admit(now + Duration::from_secs(899)), @@ -1012,13 +1060,15 @@ mod service_tests { } fn cfg( - threshold: u32, + trip_after: u32, // consecutive failures needed at 100% (window = min_calls = trip_after) cooldown: Duration, fallback: Duration, probes: u32, ) -> CircuitBreakerConfig { CircuitBreakerConfig { - failure_threshold: NonZeroU32::new(threshold).unwrap(), + failure_rate_threshold: 100, + window_size: NonZeroU32::new(trip_after).unwrap(), + minimum_calls: NonZeroU32::new(trip_after).unwrap(), cooldown, retry_after_fallback: fallback, retry_after_cap: Duration::from_secs(1800), diff --git a/crates/adapter/net/http/api/src/rate.rs b/crates/adapter/net/http/api/src/rate.rs index 9407cca..0d04d58 100644 --- a/crates/adapter/net/http/api/src/rate.rs +++ b/crates/adapter/net/http/api/src/rate.rs @@ -149,6 +149,15 @@ pub enum BuildError { /// state). Symmetric with the pacing-parameter validation (deep review §2A). #[error("config field `{0}` must be a positive Duration, but is zero")] ZeroDuration(&'static str), + /// `circuit_breaker.failure_rate_threshold` is outside `1..=100` — a `0` would trip + /// on the first sample and a value `> 100` could never trip. + #[error("config field `circuit_breaker.failure_rate_threshold` must be in 1..=100, but is {0}")] + RateThresholdRange(u8), + /// `circuit_breaker.minimum_calls` (`{0}`) exceeds `window_size` (`{1}`) — the window + /// can never hold enough samples to reach the floor, so the breaker could never trip + /// on rate. + #[error("config field `circuit_breaker.minimum_calls` ({0}) must be <= `window_size` ({1})")] + MinCallsExceedWindow(u32, u32), } /// Validate that `cfg` is a **total**, param-sane pacing configuration. diff --git a/crates/adapter/net/http/api/src/rate_window.rs b/crates/adapter/net/http/api/src/rate_window.rs index 1312bf7..6ce43ea 100644 --- a/crates/adapter/net/http/api/src/rate_window.rs +++ b/crates/adapter/net/http/api/src/rate_window.rs @@ -7,23 +7,8 @@ //! immediately without a window sample. A recovered host earns a fresh window via //! [`RateWindow::new`]. //! -//! Not yet wired into [`crate::circuit_breaker`] — that lands in the next commit on -//! this branch, which is why the `#[expect(dead_code, …)]` below exists. - -// Standalone unit, landed ahead of its call site (`Breaker::record`, next commit on -// this branch) so it can be built and reviewed test-first in isolation. `expect` (not -// `allow`) makes the suppression self-clearing: once `circuit_breaker.rs` consumes the -// items in the non-test build the lint stops firing and `expect` reports itself as -// unfulfilled, forcing its own removal in Task 2. Scoped to `not(test)` because the -// unit tests below already exercise every item, so `dead_code` never fires in the test -// build — an unscoped `expect` there would be unfulfilled today. -#![cfg_attr( - not(test), - expect( - dead_code, - reason = "not yet wired into circuit_breaker.rs; removed once RateWindow/Outcome are consumed in Task 2" - ) -)] +//! Wired into [`crate::circuit_breaker`]'s `Breaker::record` as the Closed-state trip +//! policy. use std::collections::VecDeque; use std::num::NonZeroU32; diff --git a/crates/adapter/net/http/api/src/stack.rs b/crates/adapter/net/http/api/src/stack.rs index 95b0fd1..de18c31 100644 --- a/crates/adapter/net/http/api/src/stack.rs +++ b/crates/adapter/net/http/api/src/stack.rs @@ -127,7 +127,9 @@ impl fmt::Debug for HttpConfig { /// seed: 1, /// }, /// circuit_breaker: CircuitBreakerConfig { -/// failure_threshold: NonZeroU32::new(3).unwrap(), +/// failure_rate_threshold: 50, +/// window_size: NonZeroU32::new(50).unwrap(), +/// minimum_calls: NonZeroU32::new(10).unwrap(), /// cooldown: Duration::from_secs(30), /// retry_after_fallback: Duration::from_secs(900), /// retry_after_cap: Duration::from_secs(1800), @@ -182,13 +184,15 @@ where /// Reject degenerate zero `Duration`s that would silently defeat the layer they /// configure: `timeout == 0` (every send instantly `Timeout`) and the breaker's /// `cooldown`/`retry_after_fallback`/`retry_after_cap == 0` (Open collapses straight -/// back to Half-Open). +/// back to Half-Open). Also rejects an out-of-range `failure_rate_threshold` (must be +/// `1..=100`) and a `minimum_calls` that exceeds `window_size` (ADR-0031 Amendment #3). /// /// `rate_limit_max_wait` and the retry backoff (`base`/`cap`) may legitimately be /// zero (throttle-immediately / no-backoff), so they are **not** checked. /// /// # Errors -/// [`BuildError::ZeroDuration`] naming the first offending field. +/// [`BuildError::ZeroDuration`] naming the first offending field, +/// [`BuildError::RateThresholdRange`], or [`BuildError::MinCallsExceedWindow`]. const fn validate_config(cfg: &HttpConfig) -> Result<(), BuildError> { if cfg.timeout.is_zero() { return Err(BuildError::ZeroDuration("timeout")); @@ -204,6 +208,19 @@ const fn validate_config(cfg: &HttpConfig) -> Result<(), BuildError> { if cfg.circuit_breaker.retry_after_cap.is_zero() { return Err(BuildError::ZeroDuration("circuit_breaker.retry_after_cap")); } + if cfg.circuit_breaker.failure_rate_threshold == 0 + || cfg.circuit_breaker.failure_rate_threshold > 100 + { + return Err(BuildError::RateThresholdRange( + cfg.circuit_breaker.failure_rate_threshold, + )); + } + 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(), + )); + } Ok(()) } @@ -384,8 +401,16 @@ mod tests { cap: Duration::ZERO, seed: 1, }, + // 100% over a 3-sample window, min_calls 3: trips on the 3rd consecutive + // `Class::Failure` record — the rate-policy analogue of the old + // `failure_threshold: NonZeroU32::new(3)`, preserving + // `circuit_opens_and_fast_rejects_without_touching_the_leaf`'s "3 straight + // failures trip" assertion below (the CircuitBreaker sits outside Retry, so it + // records one outcome per top-level `svc.call`, not per retry attempt). circuit_breaker: CircuitBreakerConfig { - failure_threshold: NonZeroU32::new(3).unwrap(), + failure_rate_threshold: 100, + window_size: NonZeroU32::new(3).unwrap(), + minimum_calls: NonZeroU32::new(3).unwrap(), cooldown: Duration::from_secs(30), retry_after_fallback: Duration::from_secs(900), retry_after_cap: Duration::from_secs(1800), @@ -538,6 +563,43 @@ mod tests { ); } + #[test] + fn rejects_a_zero_failure_rate_threshold() { + let timer = MockTimer::new(); + let leaf = ScriptLeaf::new(timer.clone(), vec![Step::Status(200)]); + let mut cfg = http_cfg(1, Duration::from_secs(1), Duration::ZERO); + cfg.circuit_breaker.failure_rate_threshold = 0; + assert_eq!( + stack(leaf, cfg, timer, NoAuth, rate_cfg()).err(), + Some(BuildError::RateThresholdRange(0)), + ); + } + + #[test] + fn rejects_an_over_100_failure_rate_threshold() { + let timer = MockTimer::new(); + let leaf = ScriptLeaf::new(timer.clone(), vec![Step::Status(200)]); + let mut cfg = http_cfg(1, Duration::from_secs(1), Duration::ZERO); + cfg.circuit_breaker.failure_rate_threshold = 101; + assert_eq!( + stack(leaf, cfg, timer, NoAuth, rate_cfg()).err(), + Some(BuildError::RateThresholdRange(101)), + ); + } + + #[test] + fn rejects_min_calls_greater_than_window() { + let timer = MockTimer::new(); + let leaf = ScriptLeaf::new(timer.clone(), vec![Step::Status(200)]); + let mut cfg = http_cfg(1, Duration::from_secs(1), Duration::ZERO); + cfg.circuit_breaker.window_size = NonZeroU32::new(10).unwrap(); + cfg.circuit_breaker.minimum_calls = NonZeroU32::new(11).unwrap(); + assert_eq!( + stack(leaf, cfg, timer, NoAuth, rate_cfg()).err(), + Some(BuildError::MinCallsExceedWindow(11, 10)), + ); + } + // ---- Task 2 tests ----------------------------------------------------- // 1. CircuitBreaker OUTSIDE Retry — an open circuit fast-rejects; the leaf is @@ -809,15 +871,15 @@ mod tests { // 6. C1 regression — a purely LOCAL pacing rejection (a `Throttled` error, the // request never sent) must NEVER trip the venue-wide breaker. Fire more local - // throttles than failure_threshold, then a well-formed request must still - // reach the leaf (breaker stayed Closed), not be fast-rejected as CircuitOpen. + // throttles than the breaker's minimum_calls, then a well-formed request must + // still reach the leaf (breaker stayed Closed), not be fast-rejected as CircuitOpen. #[tokio::test] async fn repeated_local_throttle_never_opens_the_breaker() { let timer = MockTimer::new(); let leaf = ScriptLeaf::new(timer.clone(), vec![Step::Status(200)]); let svc = stack( leaf.clone(), - http_cfg(1, Duration::from_secs(30), Duration::ZERO), // failure_threshold = 3 + http_cfg(1, Duration::from_secs(30), Duration::ZERO), // window=min_calls=3 timer, NoAuth, rate_cfg(), diff --git a/crates/adapter/net/http/hyper/examples/client_with_directives.rs b/crates/adapter/net/http/hyper/examples/client_with_directives.rs index e650420..68cb3a2 100644 --- a/crates/adapter/net/http/hyper/examples/client_with_directives.rs +++ b/crates/adapter/net/http/hyper/examples/client_with_directives.rs @@ -63,7 +63,9 @@ async fn main() { seed: 1, }, circuit_breaker: CircuitBreakerConfig { - failure_threshold: NonZeroU32::new(3).unwrap(), + failure_rate_threshold: 50, + window_size: NonZeroU32::new(50).unwrap(), + minimum_calls: NonZeroU32::new(10).unwrap(), cooldown: Duration::from_secs(30), retry_after_fallback: Duration::from_secs(900), retry_after_cap: Duration::from_secs(1800), diff --git a/crates/adapter/net/http/hyper/src/build.rs b/crates/adapter/net/http/hyper/src/build.rs index f5a7ac1..33acf4c 100644 --- a/crates/adapter/net/http/hyper/src/build.rs +++ b/crates/adapter/net/http/hyper/src/build.rs @@ -38,7 +38,7 @@ use std::fmt; /// let cfg = HttpConfig { /// timeout: Duration::from_secs(5), /// retry: RetryConfig { max_attempts: NonZeroU32::new(3).unwrap(), base: Duration::from_millis(50), cap: Duration::from_secs(1), seed: 1 }, -/// circuit_breaker: CircuitBreakerConfig { failure_threshold: NonZeroU32::new(3).unwrap(), cooldown: Duration::from_secs(30), retry_after_fallback: Duration::from_secs(900), retry_after_cap: Duration::from_secs(1800), half_open_probes: NonZeroU32::new(1).unwrap() }, +/// circuit_breaker: CircuitBreakerConfig { failure_rate_threshold: 50, window_size: NonZeroU32::new(50).unwrap(), minimum_calls: NonZeroU32::new(10).unwrap(), cooldown: Duration::from_secs(30), retry_after_fallback: Duration::from_secs(900), retry_after_cap: Duration::from_secs(1800), half_open_probes: NonZeroU32::new(1).unwrap() }, /// headers: http::HeaderMap::new(), /// rate_limit_max_wait: Duration::from_secs(0), /// }; @@ -125,7 +125,9 @@ mod tests { seed: 1, }, circuit_breaker: CircuitBreakerConfig { - failure_threshold: NonZeroU32::new(3).unwrap(), + failure_rate_threshold: 50, + window_size: NonZeroU32::new(50).unwrap(), + minimum_calls: NonZeroU32::new(10).unwrap(), cooldown: Duration::from_secs(30), retry_after_fallback: Duration::from_secs(900), retry_after_cap: Duration::from_secs(1800), @@ -297,8 +299,8 @@ mod tests { // A real venue 429 arrives as Ok(status = 429). A single 429 maps to // Class::TripNow in the breaker (circuit_breaker.rs classify), which trips - // immediately on the long retry_after_fallback regardless of failure_threshold; the - // next call fast-rejects with CircuitOpen without a send. + // immediately on the long retry_after_fallback regardless of the rate-window + // policy; the next call fast-rejects with CircuitOpen without a send. #[tokio::test] async fn a_real_429_trips_the_breaker_through_the_full_stack() { let base = spawn_status(429).await; From 2067c15e6617db075f760da1ef181a422933fd8c Mon Sep 17 00:00:00 2001 From: NotAProfDev <84450364+NotAProfDev@users.noreply.github.com> Date: Thu, 9 Jul 2026 16:55:33 +0000 Subject: [PATCH 5/7] feat(net): label breaker open-transitions with a trip reason --- .../net/http/api/src/circuit_breaker.rs | 135 ++++++++++++------ crates/adapter/net/http/api/src/meter.rs | 45 +++--- 2 files changed, 119 insertions(+), 61 deletions(-) diff --git a/crates/adapter/net/http/api/src/circuit_breaker.rs b/crates/adapter/net/http/api/src/circuit_breaker.rs index f01c52d..6041c5a 100644 --- a/crates/adapter/net/http/api/src/circuit_breaker.rs +++ b/crates/adapter/net/http/api/src/circuit_breaker.rs @@ -167,6 +167,31 @@ impl Phase { } } +/// Why the breaker transitioned to `Open` — a low-cardinality telemetry reason. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum TripReason { + /// The rolling failure rate crossed the threshold. + Rate, + /// A venue `429` response (`TripNow`) — the penalty box. + Throttle, + /// A Half-Open probe failed. + ProbeFailed, + /// A Half-Open probe was abandoned (cancelled / panicked). + Abandoned, +} + +impl TripReason { + /// The stable, low-cardinality telemetry label. + const fn label(self) -> &'static str { + match self { + Self::Rate => "rate", + Self::Throttle => "throttle", + Self::ProbeFailed => "probe_failed", + Self::Abandoned => "abandoned", + } + } +} + /// The label for a phase change, or `None` when the phase is unchanged — the shell /// emits a `http_circuit_breaker_transitions_total{to}` count for each `Some`. fn transition_label(before: Phase, after: Phase) -> Option<&'static str> { @@ -230,7 +255,12 @@ impl Breaker { /// the rate crosses the threshold (with enough samples); `Ignored` is never a sample; /// a `429` `TripNow` trips immediately (unchanged). `retry_after` is consulted only in /// the `TripNow` arms, clamped to `retry_after_cap`. - pub(crate) fn record(&mut self, class: Class, now: Instant, retry_after: Option) { + pub(crate) fn record( + &mut self, + class: Class, + now: Instant, + retry_after: Option, + ) -> Option { // Hoist config reads (all `Copy`) so the `&mut self.state` match below borrows only // `state`, and compute the next state, applying it after the borrow ends. let min_calls = self.cfg.minimum_calls.get(); @@ -244,54 +274,73 @@ impl Breaker { }), ); - let next: Option = match &mut self.state { + // (next state, trip reason). Reason is Some only when entering Open. + let (next, reason): (Option, Option) = match &mut self.state { BreakerState::Closed { window } => match class { Class::Failure => { window.push(Outcome::Failure); - window - .should_trip(min_calls, threshold) - .then_some(BreakerState::Open { - reopen_at: rate_reopen, - }) + if window.should_trip(min_calls, threshold) { + ( + Some(BreakerState::Open { + reopen_at: rate_reopen, + }), + Some(TripReason::Rate), + ) + } else { + (None, None) + } }, Class::Success => { window.push(Outcome::Success); // dilutes the rate; no reset cliff - None + (None, None) }, - Class::Ignored => None, // a 4xx/Auth is not a host-health sample - Class::TripNow => Some(BreakerState::Open { - reopen_at: tripnow_reopen, - }), + Class::Ignored => (None, None), // a 4xx/Auth is not a host-health sample + Class::TripNow => ( + Some(BreakerState::Open { + reopen_at: tripnow_reopen, + }), + Some(TripReason::Throttle), + ), }, BreakerState::HalfOpen { probes_left, successes_needed, } => match class { - Class::Failure => Some(BreakerState::Open { - reopen_at: rate_reopen, - }), - Class::TripNow => Some(BreakerState::Open { - reopen_at: tripnow_reopen, - }), + Class::Failure => ( + Some(BreakerState::Open { + reopen_at: rate_reopen, + }), + Some(TripReason::ProbeFailed), + ), + Class::TripNow => ( + Some(BreakerState::Open { + reopen_at: tripnow_reopen, + }), + Some(TripReason::Throttle), + ), // A reached-host probe (2xx/3xx or 4xx/Auth) resolves; the last one closes // to a fresh window. - Class::Ignored | Class::Success => Some(if *successes_needed <= 1 { - BreakerState::Closed { - window: RateWindow::new(window_size), - } - } else { - BreakerState::HalfOpen { - probes_left: *probes_left, - successes_needed: *successes_needed - 1, - } - }), + Class::Ignored | Class::Success => ( + Some(if *successes_needed <= 1 { + BreakerState::Closed { + window: RateWindow::new(window_size), + } + } else { + BreakerState::HalfOpen { + probes_left: *probes_left, + successes_needed: *successes_needed - 1, + } + }), + None, + ), }, // A stale outcome from a call admitted before a concurrent trip; drop it. - BreakerState::Open { .. } => None, + BreakerState::Open { .. } => (None, None), }; if let Some(state) = next { self.state = state; } + reason } /// Resolve a Half-Open probe whose call was **abandoned** (the future was @@ -302,11 +351,14 @@ impl Breaker { /// cancelled call is not a host-health signal, so it must not advance the trip /// streak) and in `Open` (already tripped). This is what makes "every admitted /// probe reaches a decisive resolution" hold even under cancellation. - pub(crate) fn on_abandoned_probe(&mut self, now: Instant) { + pub(crate) fn on_abandoned_probe(&mut self, now: Instant) -> Option { if matches!(self.state, BreakerState::HalfOpen { .. }) { self.state = BreakerState::Open { reopen_at: deadline(now, self.cfg.cooldown), }; + Some(TripReason::Abandoned) + } else { + None } } @@ -455,17 +507,17 @@ impl Drop for ProbeGuard<'_, T> { fn drop(&mut self) { if self.armed { let now = self.timer.now(); - let transition = { + let (transition, reason) = { let mut breaker = self .breaker .lock() .unwrap_or_else(std::sync::PoisonError::into_inner); let before = breaker.phase(); - breaker.on_abandoned_probe(now); - transition_label(before, breaker.phase()) + let reason = breaker.on_abandoned_probe(now); + (transition_label(before, breaker.phase()), reason) }; if let Some(to) = transition { - crate::meter::breaker_transition(to); + crate::meter::breaker_transition(to, reason.map(TripReason::label)); } } } @@ -500,7 +552,7 @@ where (admit, transition_label(before, breaker.phase())) }; if let Some(to) = transition { - crate::meter::breaker_transition(to); + crate::meter::breaker_transition(to, None); } let is_probe = match admit { Admit::Reject => return Err(HttpError::CircuitOpen), // fast reject — leaf untouched @@ -530,18 +582,18 @@ where }, _ => None, }; - let transition = { + let (transition, reason) = { let now = self.timer.now(); let mut breaker = self .breaker .lock() .unwrap_or_else(std::sync::PoisonError::into_inner); let before = breaker.phase(); - breaker.record(class, now, retry_after); - transition_label(before, breaker.phase()) + let reason = breaker.record(class, now, retry_after); + (transition_label(before, breaker.phase()), reason) }; if let Some(to) = transition { - crate::meter::breaker_transition(to); + crate::meter::breaker_transition(to, reason.map(TripReason::label)); } if retry_after.is_some() { crate::meter::retry_after_honored("breaker"); @@ -1123,9 +1175,12 @@ mod service_tests { && k.key() .labels() .any(|l| l.key() == "to" && l.value() == "open") + && k.key() + .labels() + .any(|l| l.key() == "reason" && l.value() == "rate") && matches!(v, DebugValue::Counter(n) if n >= 1) }); - assert!(opened, "a trip emits a to=open transition counter"); + assert!(opened, "a rate trip emits to=open, reason=rate"); } #[tokio::test] diff --git a/crates/adapter/net/http/api/src/meter.rs b/crates/adapter/net/http/api/src/meter.rs index a63fb1e..d254130 100644 --- a/crates/adapter/net/http/api/src/meter.rs +++ b/crates/adapter/net/http/api/src/meter.rs @@ -57,9 +57,15 @@ pub(crate) fn route_label(req: &http::Request) -> Cow<'static, str> { .map_or(Cow::Borrowed("other"), |t| t.0.clone()) } -/// Record a circuit-breaker phase transition into `to`. -pub(crate) fn breaker_transition(to: &'static str) { - metrics::counter!(CIRCUIT_TRANSITIONS, "to" => to).increment(1); +/// Record a circuit-breaker phase transition into `to`, with an optional `reason` +/// (present only for `to = "open"`: `"rate" | "throttle" | "probe_failed" | "abandoned"`). +pub(crate) fn breaker_transition(to: &'static str, reason: Option<&'static str>) { + match reason { + Some(reason) => { + metrics::counter!(CIRCUIT_TRANSITIONS, "to" => to, "reason" => reason).increment(1); + }, + None => metrics::counter!(CIRCUIT_TRANSITIONS, "to" => to).increment(1), + } } /// Count one local pacing rejection for `route`. @@ -111,28 +117,25 @@ mod tests { } #[test] - fn breaker_transition_increments_a_phase_labelled_counter() { + fn breaker_transition_carries_to_and_optional_reason() { let recorder = DebuggingRecorder::new(); let snap = recorder.snapshotter(); metrics::with_local_recorder(&recorder, || { - breaker_transition("open"); - breaker_transition("open"); + breaker_transition("open", Some("rate")); + breaker_transition("half_open", None); }); - let counter = snap - .snapshot() - .into_vec() - .into_iter() - .find(|(k, _, _, _)| k.key().name() == "http_circuit_breaker_transitions_total") - .expect("counter emitted"); - assert!( - counter - .0 - .key() - .labels() - .any(|l| l.key() == "to" && l.value() == "open"), - "labelled to=open" - ); - assert_eq!(counter.3, DebugValue::Counter(2), "two transitions counted"); + let snapshot = snap.snapshot().into_vec(); + let opened = snapshot.iter().any(|(k, _, _, v)| { + k.key().name() == "http_circuit_breaker_transitions_total" + && k.key() + .labels() + .any(|l| l.key() == "to" && l.value() == "open") + && k.key() + .labels() + .any(|l| l.key() == "reason" && l.value() == "rate") + && matches!(v, DebugValue::Counter(n) if *n == 1) + }); + assert!(opened, "to=open carries reason=rate"); } #[test] From d6a6a4507b2de16ab43a43e3e46963fc6a7c6f32 Mon Sep 17 00:00:00 2001 From: NotAProfDev <84450364+NotAProfDev@users.noreply.github.com> Date: Thu, 9 Jul 2026 17:04:47 +0000 Subject: [PATCH 6/7] docs(net): ADR-0031 Amendment #3 + CHANGELOG (rolling-window breaker) Records ADR-0031 Amendment #3 (consecutive-count trip -> rolling error-rate window) and the corresponding CHANGELOG entry under [Unreleased] -> Changed. --- CHANGELOG.md | 9 +++++++ docs/adr/0031-http-resilience-venue-pacing.md | 26 +++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 14882ba..503ccbb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -67,6 +67,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 renamed `retry_after_fallback` (the `429` reopen wait when no usable `Retry-After` is present), and a new `retry_after_cap` bounds an honored `Retry-After` (both validated non-zero at `stack()`/`build()`). +- **Breaking (pre-release) — net-http circuit-breaker trip policy.** The `CircuitBreaker` + now trips on a **rolling error-rate window** (last-N outcomes) rather than consecutive + failures, so a venue failing a sustained fraction of interleaved traffic is detected + (a 50 %-error host no longer resets an alarm on every success) — ADR-0031 Amendment #3. + `CircuitBreakerConfig` drops `failure_threshold` and gains `failure_rate_threshold` + (percent, `1..=100`), `window_size`, and `minimum_calls` (validated at boot). The + `http_circuit_breaker_transitions_total{to="open"}` metric gains a `reason` label + (`rate`/`throttle`/`probe_failed`/`abandoned`). Prior art: tower-resilience (not + adopted — OATH keeps its RPITIT `Service`). ### Added diff --git a/docs/adr/0031-http-resilience-venue-pacing.md b/docs/adr/0031-http-resilience-venue-pacing.md index ef009bf..184bdb9 100644 --- a/docs/adr/0031-http-resilience-venue-pacing.md +++ b/docs/adr/0031-http-resilience-venue-pacing.md @@ -229,3 +229,29 @@ Recorded append-only (the decision text above is unedited). wall-clock `Timer` seam (ADR-0029). Lands the `delay-seconds` half of ADR-0034 Amendment #8's deferred "`Retry-After` parsing". Spec: `docs/superpowers/specs/2026-07-08-net-http-retry-after-design.md`. + +3. **Consecutive-count trip → count-based rolling error-rate window.** §5 shipped the + breaker with `failure_threshold` consecutive failures (*"consecutive-count for v1; + rolling-window later"*). That is blind to mixed-traffic degradation — a single + `Success` resets the streak, so a venue failing ~50 % of interleaved requests never + trips (deep-review §2B). **Changed:** in Closed, `CircuitBreaker` now trips when the + **failure rate** over the last `window_size` host-health outcomes reaches + `failure_rate_threshold` (%), once at least `minimum_calls` samples are present — + `len ≥ minimum_calls && failures*100 ≥ failure_rate_threshold*len` (integer, `≥` + trips). Only `Failure` (transport/`5xx`) and `Success` (`2xx`/`3xx`) enter the window; + a `4xx`/`Auth` (`Class::Ignored`) is never a sample, and a venue `429` (`TripNow`) + still trips **immediately** on `retry_after_fallback`/honored value (Amendment #2, + unchanged). The window is **count-based** (clock-free), chosen over time-based to + avoid a monotonic-seconds `Timer` seam; the accepted trade-off is **no idle-time + self-heal** — a stale failure patch lingers until flushed by new calls or a trip, + which resets the window to a clean slate on recovery. `Open`, `HalfOpen`, the + probe-guard, and single-per-host sharing are unchanged; per-key breakers remain + deferred (#102). **Config:** `CircuitBreakerConfig` drops `failure_threshold` and + gains `failure_rate_threshold: u8`, `window_size: NonZeroU32`, `minimum_calls: + NonZeroU32`, validated at boot (`failure_rate_threshold ∈ 1..=100`, `minimum_calls ≤ + window_size`). The `to="open"` transition metric gains a `reason` label + (`rate`/`throttle`/`probe_failed`/`abandoned`). Prior art: resilience4j / Polly / + `tower-resilience-circuitbreaker` (rate + sliding window + minimum-calls); **not** + adopted — it is built on `tower::Service`, whereas OATH keeps its RPITIT `&self` + `Service`. Spec: + `docs/superpowers/specs/2026-07-09-net-http-rolling-window-breaker-design.md`. From 3e301097e4ef9531e137e4e74efaac057dacdbb9 Mon Sep 17 00:00:00 2001 From: NotAProfDev <84450364+NotAProfDev@users.noreply.github.com> Date: Thu, 9 Jul 2026 18:02:47 +0000 Subject: [PATCH 7/7] feat(net): bound circuit-breaker window_size at boot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- crates/adapter/net/http/api/src/rate.rs | 6 ++++ crates/adapter/net/http/api/src/stack.rs | 29 +++++++++++++++++-- docs/adr/0031-http-resilience-venue-pacing.md | 3 +- 3 files changed, 35 insertions(+), 3 deletions(-) diff --git a/crates/adapter/net/http/api/src/rate.rs b/crates/adapter/net/http/api/src/rate.rs index 0d04d58..7603d81 100644 --- a/crates/adapter/net/http/api/src/rate.rs +++ b/crates/adapter/net/http/api/src/rate.rs @@ -158,6 +158,12 @@ pub enum BuildError { /// on rate. #[error("config field `circuit_breaker.minimum_calls` ({0}) must be <= `window_size` ({1})")] MinCallsExceedWindow(u32, u32), + /// `circuit_breaker.window_size` (`{0}`) exceeds the sanity ceiling (`{1}`) — a + /// window far larger than any sensible "recent health" span is almost always a + /// units/typo mistake, and `RateWindow::new` would eagerly allocate (and + /// re-allocate on every Half-Open→Closed recovery) a `VecDeque` of that size. + #[error("config field `circuit_breaker.window_size` ({0}) exceeds the sanity ceiling ({1})")] + WindowSizeTooLarge(u32, u32), } /// Validate that `cfg` is a **total**, param-sane pacing configuration. diff --git a/crates/adapter/net/http/api/src/stack.rs b/crates/adapter/net/http/api/src/stack.rs index de18c31..b4f44fc 100644 --- a/crates/adapter/net/http/api/src/stack.rs +++ b/crates/adapter/net/http/api/src/stack.rs @@ -185,15 +185,21 @@ where /// configure: `timeout == 0` (every send instantly `Timeout`) and the breaker's /// `cooldown`/`retry_after_fallback`/`retry_after_cap == 0` (Open collapses straight /// back to Half-Open). Also rejects an out-of-range `failure_rate_threshold` (must be -/// `1..=100`) and a `minimum_calls` that exceeds `window_size` (ADR-0031 Amendment #3). +/// `1..=100`), a `minimum_calls` that exceeds `window_size`, and a `window_size` +/// above a sanity ceiling (ADR-0031 Amendment #3). /// /// `rate_limit_max_wait` and the retry backoff (`base`/`cap`) may legitimately be /// zero (throttle-immediately / no-backoff), so they are **not** checked. /// /// # Errors /// [`BuildError::ZeroDuration`] naming the first offending field, -/// [`BuildError::RateThresholdRange`], or [`BuildError::MinCallsExceedWindow`]. +/// [`BuildError::RateThresholdRange`], [`BuildError::MinCallsExceedWindow`], or +/// [`BuildError::WindowSizeTooLarge`]. const fn validate_config(cfg: &HttpConfig) -> Result<(), BuildError> { + // Sanity ceiling for `window_size`: a window far larger than any sensible "recent + // health" span is almost always a units/typo mistake, and `RateWindow::new` would + // eagerly allocate (and re-allocate on every recovery) a `VecDeque` of that size. + const MAX_WINDOW_SIZE: u32 = 10_000; if cfg.timeout.is_zero() { return Err(BuildError::ZeroDuration("timeout")); } @@ -221,6 +227,12 @@ const fn validate_config(cfg: &HttpConfig) -> Result<(), BuildError> { cfg.circuit_breaker.window_size.get(), )); } + if cfg.circuit_breaker.window_size.get() > MAX_WINDOW_SIZE { + return Err(BuildError::WindowSizeTooLarge( + cfg.circuit_breaker.window_size.get(), + MAX_WINDOW_SIZE, + )); + } Ok(()) } @@ -600,6 +612,19 @@ mod tests { ); } + #[test] + fn rejects_an_oversized_window() { + let timer = MockTimer::new(); + let leaf = ScriptLeaf::new(timer.clone(), vec![Step::Status(200)]); + let mut cfg = http_cfg(1, Duration::from_secs(1), Duration::ZERO); + // A units/typo mistake: a window far beyond any sane "recent health" span. + cfg.circuit_breaker.window_size = NonZeroU32::new(10_001).unwrap(); + assert_eq!( + stack(leaf, cfg, timer, NoAuth, rate_cfg()).err(), + Some(BuildError::WindowSizeTooLarge(10_001, 10_000)), + ); + } + // ---- Task 2 tests ----------------------------------------------------- // 1. CircuitBreaker OUTSIDE Retry — an open circuit fast-rejects; the leaf is diff --git a/docs/adr/0031-http-resilience-venue-pacing.md b/docs/adr/0031-http-resilience-venue-pacing.md index 184bdb9..5160f88 100644 --- a/docs/adr/0031-http-resilience-venue-pacing.md +++ b/docs/adr/0031-http-resilience-venue-pacing.md @@ -249,7 +249,8 @@ Recorded append-only (the decision text above is unedited). deferred (#102). **Config:** `CircuitBreakerConfig` drops `failure_threshold` and gains `failure_rate_threshold: u8`, `window_size: NonZeroU32`, `minimum_calls: NonZeroU32`, validated at boot (`failure_rate_threshold ∈ 1..=100`, `minimum_calls ≤ - window_size`). The `to="open"` transition metric gains a `reason` label + window_size`, and `window_size ≤` a sanity ceiling so a units/typo mistake can't + force a huge `RateWindow` allocation). The `to="open"` transition metric gains a `reason` label (`rate`/`throttle`/`probe_failed`/`abandoned`). Prior art: resilience4j / Polly / `tower-resilience-circuitbreaker` (rate + sliding window + minimum-calls); **not** adopted — it is built on `tower::Service`, whereas OATH keeps its RPITIT `&self`