From ef73f8c1ddfa889db93408c641f4f21e74ca3aa8 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Fri, 31 Jul 2026 21:56:26 -0700 Subject: [PATCH 01/16] docs(code-reviews): session-03 plan review rounds 1-3 Plan-review checkpoint for TD-2026-07-09 (and, in rounds 1-2, TD-2026-07-08). 25 agents plus a verification pass across three rounds. Round 1 invalidated the plan's central premise: timeout_is_outage_signal (mod.rs:504) is a duration comparison, not header provenance, so every client deadline at or above the global collapsed into the "no header" bucket. Round 2 found four regressions introduced by round 1's own remediation, two of them recreating the anti-patterns the session exists to remove: a stored outage_signal bool with no checkable invariant, and a metrics hoist that let racing transitions leave the Prometheus gauge permanently wrong. It also established that TD-08 and TD-09 are mis-sequenced rather than mis-specified. Session was re-scoped to TD-09 only on that finding; TD-08 moves to session 04 and must be read against these artifacts. Round 3 found no architectural regressions and did not indicate a round 4. Artifacts carry two TD-08 findings absent from the TD record: a rejected header remains indistinguishable from an absent one, and timeout.rs:129-131 drops a non-UTF-8 header value with no log at all. --- docs/code-reviews/session-03-plan-round1.md | 525 ++++++++++++++++++++ docs/code-reviews/session-03-plan-round2.md | 506 +++++++++++++++++++ docs/code-reviews/session-03-plan-round3.md | 305 ++++++++++++ 3 files changed, 1336 insertions(+) create mode 100644 docs/code-reviews/session-03-plan-round1.md create mode 100644 docs/code-reviews/session-03-plan-round2.md create mode 100644 docs/code-reviews/session-03-plan-round3.md diff --git a/docs/code-reviews/session-03-plan-round1.md b/docs/code-reviews/session-03-plan-round1.md new file mode 100644 index 0000000..6c5cf9a --- /dev/null +++ b/docs/code-reviews/session-03-plan-round1.md @@ -0,0 +1,525 @@ +# Session 03 — Plan Review, Round 1 + +**Target:** the session-03 plan (TD-2026-07-08 client-visible `X-Request-Timeout` +feedback + TD-2026-07-09 circuit-breaker enum-with-payloads and RAII `ProbePermit`), +reviewed against the tree at `tech-debt/session-03` @ `e12a513`. Scope = plan; no +implementation exists yet. The plan itself is a working artifact and is not committed +(this repo has never committed a plan file). + +**Provenance:** config: full — step-0 gate attested by Maxim this session (8-agent +suite, both rounds, all agents plus the verifier pinned to Opus-class). Agent +fallbacks: none; all eight roster agents ran as their native types. Verification +tally: 20 claims re-read against cited `file:line`, **19 kept / 0 discarded**, 1 +`drifted` (corrected inline, theme T10) and one embedded count correction (T15). + +Reviewers cited in brackets: [consistency] general-purpose, [architect] +feature-dev:code-architect, [reviewer] feature-dev:code-reviewer, [types] +pr-review-toolkit:type-design-analyzer, [silent] pr-review-toolkit:silent-failure-hunter, +[comments] pr-review-toolkit:comment-analyzer, [tests] pr-review-toolkit:pr-test-analyzer, +[simplifier] pr-review-toolkit:code-simplifier. + +--- + +## T1 — CRITICAL — The plan's central premise is false: `timeout_is_outage_signal` is not provenance + +*[consistency] [architect] [reviewer] [types] [silent] [tests] [simplifier] — 7 of 8 agents, independently.* + +Plan lines 54-57 assert that "provenance is already fully determined by the existing +`timeout_is_outage_signal` + `timeout` pair, so no new parameter is required." It is not. + +`src/iggy_client/mod.rs:504`: +```rust +let timeout_is_outage_signal = self.op_deadline >= self.config.operation_timeout; +``` + +That is a **duration comparison**, not "did the client send a header". `with_timeout` +clamps at `mod.rs:1047` via `clamp_deadline` (`mod.rs:194-196`, `requested.min(global)`), +so every requested deadline ≥ the global collapses onto the global. + +Concrete failure, with stock config (`OPERATION_TIMEOUT_SECS` default 30, `config.rs:196`; +`MAX_REQUEST_TIMEOUT_MS = 300_000`, `middleware/timeout.rs:68`): + +| Client sends | Enforced | `timeout_is_outage_signal` | Plan's `client_deadline_ms` | Correct answer | +|---|---|---|---|---| +| *(nothing)* | 30 s | true | `None` | `None` ✓ | +| `5000` | 5 s | false | `Some(5000)` | `Some(5000)` ✓ | +| `30000` | 30 s | **true** | **`None`** ✗ | `Some(30000)` | +| `300000` | 30 s (clamped) | **true** | **`None`** ✗ | requested 300000, enforced 30000 | + +The whole 30 s–300 s acceptance band — which includes the largest value a client can +legally send — renders a 504 byte-identical to a request that sent no header at all. +That is TD-08's problem statement item 2 (`TD-2026-07-08.md:14-16`) surviving the fix +intended to close it, and the clamped case is precisely where a diagnostic is most +load-bearing: the client asked for five minutes, silently got thirty seconds. + +A second defect rides along: the value available at both construction sites +(`resilience.rs:186-189`, `:234-237`) is the **enforced** duration. TD-08's trigger +requires details that "name the client-requested deadline" — which is not the number +in scope. + +**Remediation:** provenance must be **carried**, not inferred. Q-A stops being a +preference and becomes a requirement, and the type must hold both numbers. + +--- + +## T2 — CRITICAL — The obvious fix for T1 would silently reverse an anti-DoS property + +*[architect] [reviewer] [types] [simplifier].* + +If `Deadline { Global, Client }` is introduced and the discriminant drives the breaker +predicate at `resilience.rs:157`, then every request carrying a header ≥ global stops +feeding the breaker — today it does feed it. `resilience.rs:56-65` already documents +that when client-deadline traffic dominates, the only guaranteed breaker feeder is the +background stats refresher and convergence to Open drops to minutes-scale. This would +widen that hole to *all* header-bearing traffic, so one client (hostile, or just an SDK +that always sets a large header) could blind outage detection for everyone. + +A deadline expiring at exactly the global carries identical evidentiary value to the +global expiring — so the value-based predicate is the correct one and must stay. + +**Remediation:** the type carries three independent projections; the breaker predicate +stays duration-derived. Never one discriminant serving both questions. + +--- + +## T3 — HIGH — TD-08 is not fully discharged, yet M3 marks it resolved + +*[consistency] [architect] [silent] [types].* + +Three distinct holes; the plan closes roughly one and a half and then flips the record: + +1. **The clamped/equal band stays silent** — T1. +2. **The reconnect-wait path reports the client's own expiry as a broker outage.** + `mod.rs:478-481`, inside `reconnect_bounded`, bounds the wait with `self.op_deadline` + (`:469` — the possibly client-scoped value) and on expiry returns + `AppError::ConnectionFailed("Reconnection did not complete within {:?} …")`, which + renders as **503 `connection_failed`** ("Message broker is temporarily unavailable"), + not a 504. M1 only touches `OperationTimeout`. So a client whose own 100 ms deadline + expired is affirmatively told the broker is down — TD-08 item 1 in a strictly worse + form than the generic 504 it complains about. +3. **A rejected header is indistinguishable from an absent one.** Out-of-range + (`timeout.rs:142-147`) and malformed (`:153-156`) both warn-log and drop the value; + under M2 both yield `X-Effective-Timeout: ` — exactly what a header-less + request gets. A client that typos `X-Request-Timeout: 5s` gets a 200 and never learns + its integration is broken. TD-08 item 2 asks for confirmation the header was + *honored*; the echo confirms only the enforced number. + +Marking a TD resolved while its named silences are live is the deferral-without-a-plan +failure mode. + +--- + +## T4 — HIGH — "on every response" is unachievable where the layer sits: 401 and 429 cannot carry the header + +*[architect] [reviewer] [silent] [comments] [tests] — 5 of 8.* + +`src/routes.rs` applies the timeout middleware at `:154`, auth at `:172`, rate limiting +at `:186`. Layers apply bottom-to-top, which the file states itself at `:136` ("order +matters - applied bottom to top") and `:177-178` ("applied last, so it runs FIRST … +outermost layer"). Both outer layers short-circuit without invoking the inner service — +`rate_limit.rs:403-414` returns the 429 directly, `auth.rs:273`/`:282` the 401 — so +`extract_request_timeout` never runs and cannot stamp anything. + +The result is inverted from what a client needs: **present** on 404 (axum's `Router::layer` +wraps the fallback), **absent** on exactly the two rejection paths a client most wants to +diagnose. This is the same pre-existing reason 401s lack `X-Request-Id`. + +Shipping an attested decision whose stated scope the wiring silently contradicts is the +class of client-facing lie TD-08 exists to remove. + +--- + +## T5 — HIGH — The header would advertise a deadline on responses where nothing was enforced + +*[architect] [reviewer] [silent] [simplifier].* + +`health_check` (`handlers/health.rs:46`), `readiness_check` (`:79`) and `stats` (`:109`) +take no `Option` and never call `*_scoped`; they read a cached flag +(`:47`, `:80`) or the background stats cache (`:110`). Under "every response", +`GET /health` with `X-Request-Timeout: 100` returns `X-Effective-Timeout: 100` while +nothing whatsoever was bounded by 100 ms. + +Note this is **not** the divergence Q-B is guarding against, and a shared clamp function +cannot detect it: the two computations agree perfectly: the mismatch is between the +middleware's input and whether any handler built a scoped client at all. The plan's +proposed test matrix (absent / in-range / clamped / out-of-range) contains no case that +would catch it. + +Related, from [architect] and [silent]: the header names a **per-attempt** bound. +`resilience.rs:42-46` documents the reconnect path costing up to 3× the deadline, and +`handlers/topics.rs:38`+`:40` issue *two* scoped operations per request — so a response +can legitimately take ~6× the number in the header. + +--- + +## T6 — HIGH — `ProbePermit` as sketched closes 2 of TD-09's 3 leaks, not 3 + +*[consistency] [architect] [reviewer] [types] [simplifier] — 5 of 8.* + +Plan lines 121-124 claim all three named accounting leaks become "unrepresentable". +Checked against `TD-2026-07-09.md:29-35`: + +- **Phantom release** (admitted while Closed, releases a token it never consumed) — only + fixed if `allow_request` stops returning `bool`. `circuit_breaker.rs:230` returns + `true` on the Closed path with no token behind it; a uniform permit whose `Drop` calls + release would mint a token from nothing. Fix: `fn allow_request(&self) -> Option` + where `Admission::{Ungated, Probe(ProbePermit)}` — `Ungated` has no release path, so + the Closed case cannot phantom-release. +- **Dropped future** (client disconnect mid-probe) — genuinely fixed, and only fixable + by `Drop`. This is the honest justification for RAII. +- **Straggler re-grant** — **not fixed.** `release_probe` (`:329-338`) checks only + `state == HalfOpen` and the budget cap; it has no notion of *which* window a token + belonged to. A permit minted in window N and dropped in window N+1 still credits N+1. + +`granted_at: Instant` cannot serve as the window identity: every relevant test runs +`start_paused = true` and only moves the clock via `tokio::time::advance`, so two grants +without an intervening advance compare equal. + +**Remediation:** add a monotonic `u64` generation to the `HalfOpen` payload, stamp it +into the permit, and release only on a generation match. + +*Credit where due [simplifier]:* the permit also deletes the documented double-release at +`resilience.rs:67-71` — a stronger argument than the three leaks, and one the plan omits. + +--- + +## T7 — HIGH — RAII release over a non-reentrant `std::sync::Mutex` is a self-deadlock hazard + +*[architect] [reviewer] [types] [simplifier].* + +`ProbePermit::drop` must take the same mutex that `record_success` (`:343-344`) and +`record_failure` (`:385-386`) take as their first statement. If a consuming API takes the +lock and *then* drops the permit — the most natural way to write it — it self-deadlocks. +Under `tokio::sync::RwLock` that hangs one task; under `std::sync::Mutex` it parks an OS +**worker thread**, and repeats exhaust the runtime on the path that gates every Iggy +operation. `panic = "abort"` (`Cargo.toml:100`) means there is no unwind to bail out. + +Rust's drop order (locals before params) accidentally rescues the most obvious shape, +which makes this worse — it will work until someone restructures. + +**Remediation:** the disarm must be a **lock-free** flag flip performed *before* any +guard is acquired (`breaker: Option<&'a CircuitBreaker>`, `consume()` sets it to `None`), +with a documented invariant "never drop a `ProbePermit` while a state guard is live", +plus a regression test. Borrow rather than `Arc` so a permit cannot be smuggled into a +detached `tokio::spawn`. + +--- + +## T8 — HIGH — Both breaking-change commits will fail the "Conventional Commits" CI job + +*[reviewer]; verified `present`.* + +`.github/workflows/pr.yml:77`: +``` +PATTERN="^(feat|fix|docs|style|refactor|perf|test|build|ci|chore|revert)(\(.+\))?: .+" +``` +There is no `!?` before the colon, and the check at `:84` exits 1 on no-match. +`refactor(error)!: …` cannot match. `.commitlintrc.json` *does* permit `!` — but no +workflow anywhere runs commitlint (confirmed repo-wide; no husky, no pre-commit), so the +stricter regex is the only enforcer and it wins. Plan commits 1 and 4 both fail. + +**Remediation:** land a `ci:` commit relaxing the pattern to `…(\(.+\))?!?: .+` first, or +drop `!` from subjects and declare breaks in a `BREAKING CHANGE:` footer (the check reads +only `%s`). + +--- + +## T9 — MEDIUM-HIGH — Q-C and Q-D are both answered "yes", but the plan's stated reasons are factually wrong + +*[architect] [reviewer] [silent].* + +The plan justifies both with "every critical section is a handful of field assignments +with no panic path and no `await`". Verified false: the guarded regions call `tracing` +macros and the global metrics recorder throughout — `:264`, `:363`, `:446`, `:469-470`, +and `:318` inside `reject_request`, which is called at `:238` **while the read guard +taken at `:228` is still live**. `record_circuit_breaker_rejection` builds a labeled key +per call: hashing, a registry shard lock inside `metrics-exporter-prometheus`, and an +allocation. + +Concrete consequence a panic there would leave: `open_now` (`:465-471`) sets internal +state to Open, then panics in `record_circuit_breaker_open()` before +`set_circuit_breaker_state(2)` — breaker Open, Prometheus gauge permanently reading 0 +(closed). With `into_inner` and no logging, nobody is ever told. + +**The conclusions survive, on better grounds:** +- **Q-C — yes.** The strong argument is `Cargo.toml:100` `panic = "abort"`: poisoning is + structurally unreachable in release and `unwrap_or_else(PoisonError::into_inner)` only + ever executes under `cargo test`. `clippy::unwrap_used` does not fire on + `unwrap_or_else`, so `Cargo.toml:84-89` is satisfied with no `#[allow]`. In a `Drop` + impl `into_inner` also avoids the panic-during-unwind → abort that `.unwrap()` risks. +- **Q-D — yes.** [architect] corrects the framing: tokio's `RwLock::read()` is a fair + semaphore permit acquisition (atomic RMW, possible task park behind a queued writer), + not a free shared read, and the HalfOpen path currently takes **two** locks (`:228` + then `:246`) where a `Mutex` takes one. The change is a *win*, not a cost. + +**Remediation (required for both answers to hold):** hoist logging and metrics out of the +critical sections — compute a small `Effect` value, drop the guard, then emit. That makes +"no panic path" true by construction, shrinks exclusive hold time, and removes the only +realistic poisoning source. If `into_inner` is retained it must `error!` + increment a +counter, not swallow. + +--- + +## T10 — MEDIUM — Q-B's mitigation is tautological, targets the wrong divergence, and duplicates an existing function + +*[architect] [types] [silent] [tests] [simplifier] [comments] — 6 of 8.* + +Three separate problems with "extract a single `effective_deadline(client, global)`": + +1. **It already exists.** `clamp_deadline` at `mod.rs:194-196` is that function. + *(Verifier correction: the plan-review citation `192-194` was drifted; `192-193` are + the doc comment.)* +2. **The test is vacuous.** If both sides call the same helper, "header == enforced" + holds by construction and can never fail. +3. **It clamps against the wrong operand.** `with_timeout` clamps against + `self.op_deadline` (`:1047`), deliberately — the comment at `:1044-1046` and the test + at `:1111-1112` pin that re-scoping is shrink-only. A middleware clamping against + `config.operation_timeout` agrees only while the handler scopes the *root* wrapper. + Nothing enforces that. + +**Remediation:** make the **value** the shared artifact, not a function. Build the +`Deadline` once in the middleware, put it in request extensions, stamp the header from +it, and pass the same value into the wrapper. The header cannot lie because it *is* what +was enforced. At least one assertion must *observe* the enforced deadline rather than +recompute it. + +--- + +## T11 — MEDIUM — Commit 4 bundles three-to-four independent changes and rewrites its own oracle + +*[consistency] [architect] [simplifier].* + +`refactor(circuit-breaker)!: per-state payloads and RAII probe permit` contains: the +sync-lock conversion, the enum reshape, `allow_request`'s return-type change across 28 +assert sites, and new `Drop` semantics — while mechanically rewriting the 17 breaker and +16 resilience tests that are the *only* specification of this behavior. A single diff +that changes semantics and rewrites every assertion cannot be reviewed for behavior +preservation, and `pr.yml` warns above 500 changed lines. `refactor` is also the wrong +conventional type for a behavioral delta, contradicting `TD-2026-07-09.md:42`'s own +"shape-only with no behavioral delta" framing. + +**Remediation — split into three:** +- **4a** `refactor(circuit-breaker): flat struct → state enum with payloads` — `RwLock` + retained, **all 17 tests byte-identical**. That invariance *is* the proof. +- **4b** `refactor(circuit-breaker)!: sync Mutex, breaker methods become sync` — purely + mechanical `.await` removal. +- **4c** `feat(circuit-breaker)!: RAII ProbePermit replaces explicit release_probe` — the + only commit carrying a behavioral delta. + +--- + +## T12 — MEDIUM — M4 has zero documentation deliverables, and the breaker's usage example rots invisibly + +*[comments] (lead) [architect] [types] [silent] [tests].* + +M3 lists four doc items; **M4 lists none** — yet M4 falsifies the two largest prose blocks +in the crate (~150 lines). + +- `circuit_breaker.rs:57-82` is a `rust,ignore` fenced example calling + `cb.allow_request().await` (`:61`), `cb.record_success().await` (`:69`), + `cb.record_failure().await` (`:73`), `cb.release_probe().await` (`:78`). Under M4 all + four are wrong and the final arm teaches an API RAII deletes. **`ignore` means + `cargo test --doc` will never catch it** — permanent silent rot. Drop `,ignore` after + rewriting so the compiler pins it thereafter. +- `resilience.rs:1-71` — semantics #5 names `timeout_is_outage_signal` by identifier + (`:21-29`); `:96-103` documents the six parameters by name; `:66-71` documents the + double-release that M4 makes unrepresentable. +- Per-method docs at `circuit_breaker.rs:175-183` ("Uses RwLock internally"), `:201-223` + (the re-grant "anti-wedge guarantee" the permit supersedes), `:293-297`, `:303-308`, + `:322-328`, `:340-342`, `:381-384`, `:420-423`, `:435-448`, `:450-453`. +- `TD-2026-07-03.md`'s resolution recorded a **two-site invariant** — the breaker module + doc *and* the `IggyClientWrapper` struct doc (`mod.rs:28`, `:139-146`) must move + together. The plan names neither. + +M3 additions [comments] [reviewer]: `error.rs:163` (whose `// Never expose internal +details to clients` becomes the policy M1 breaks), `README.md:46`, `README.md:611`, +`routes.rs:25` + `:152-153`, `timeout.rs:28-34` + `:42-49` + `:140-141`, `CLAUDE.md:142`. + +--- + +## T13 — MEDIUM — Test-coverage gaps at exactly the places the plan changes + +*[tests] (lead) [comments].* + +- **`src/error.rs` has no test module at all** (verified: zero `cfg(test)` matches in 207 + lines; no `AppError` reference in `tests/`). M1 rewrites its rendering — status, + message, and the `details` gating — with nothing checking any of it. +- **Nothing in the suite produces a 504.** `tests/integration_tests.rs:988-1035` exercises + four header paths and asserts status only (`:1001`, `:1013`, `:1024`, `:1034`) — no + header reads, no timeout path. TD-08's headline case is untested. +- **The concurrency test cannot survive the sync switch.** `circuit_breaker.rs:722-739` + uses `tokio::join!(cb.allow_request(), cb.allow_request())`, and its own comment + (`:723-726`) names the write-lock yield point as the mechanism. Sync methods aren't + futures — `join!` won't compile, and the obvious rewrite (two sequential calls) passes + even with the cap removed. `start_paused` also forbids `multi_thread`, and `std::thread` + racers escape the frozen clock. +- **Two of TD-09's three leaks have no planned test** — phantom-release-after-transition + and straggler-after-re-grant. The plan's three tests map to the dropped-future leak plus + one new property. +- **"Permit released on drop" tests the wrong thing** — a scope-exit drop proves `Drop` is + wired; the leak TD-09 names is *cancellation*. Make it a mid-flight future drop. + Relatedly, `reconnect().await?` at `resilience.rs:140`/`:179` exits with a live token and + no release today; RAII changes that — an observable delta, and both existing tests of + that path use Closed-state breakers so neither would notice. +- **Churn is understated in the direction that matters.** Of 123 breaker `.await` sites, + **105 are inside `#[cfg(test)]`** — ~85% of the churn is a mechanical edit pass across + the suites that are this feature's only specification. That is exactly where "make it + compile" quietly weakens an assertion. +- **Every breaker Prometheus transition is rewritten and none is asserted** — the + `"open"` vs `"half_open"` rejection label, documented at `:313-315` as distinguishing + "materially different situations", has no coverage at all. + +--- + +## T14 — MEDIUM — Q-E is moot: both accessors have zero callers + +*[consistency] [architect] [reviewer] [types] [tests] [simplifier] — 6 of 8, unanimous on the answer.* + +`circuit_breaker_state()` (`mod.rs:1052`), `force_close_circuit()` (`:1067`) and +`circuit_breaker_metrics()` (`:1059`) have **no callers anywhere** in `src/`, `tests/`, or +`fuzz/`. `src/state.rs` contains zero breaker references, so the plan's "keep `AppState`'s +surface stable" rationale (line 162) is unsupported — `AppState` never touches them. The +crate is `publish = false` (`Cargo.toml:10`) and `cargo semver-checks` is +`continue-on-error` (`pr.yml:154`). + +**Answer: make them sync** (or delete them). An `async fn` that awaits nothing advertises +a suspension point that does not exist, and `clippy::unused_async` is not enabled so CI +will never flag it. Do not spend a "second breaking change" budget defending a caller set +of size zero. + +--- + +## T15 — MEDIUM — Count drift in the plan, and two stale counts in the repo + +*[consistency] [comments] [tests]; resolved by the verifier.* + +- Resilience tests: plan says 15; actual **16** (14 composition `:277`-`:712` + 2 + classifier `:718`, `:729`). +- Breaker tests: plan says 17; **correct** (the naive grep returns 7 because 10 carry + `(start_paused = true)`). +- Call sites: plan says 124; actual **123**, split **105 test / 18 non-test** — and 4 of + those 18 are the doc-comment lines of T12's example, so **14 genuine production call + sites**. (Reviewer figures of 124 and 131 are both wrong.) +- `README.md:611` documents the timeout error as `operation_timeout` / **503**; the code + renders `"timeout"` / **504** (`error.rs:128-132`). Both columns wrong, and M1 edits + that row's body. +- `README.md:46` states "183 unit tests, 30 integration tests, 18 model tests" — a live + claim this session invalidates. `CHANGELOG.md:40` carries the same numbers inside the + released `## [0.3.0]` section and must **not** be touched. + +--- + +## T16 — MEDIUM — The enum refactor will silently drop a recorded probe success + +*[simplifier]; verified `present`. Single-source but high-value.* + +`grant_probe_tokens` (`:298-301`) writes only `half_open_probes_remaining` and +`half_open_granted_at`. Its two callers differ on the third field: + +- HalfOpen **entry** (`:255-257`) sets `consecutive_successes = 0` as a separate statement + before calling it. +- **Re-grant** (`:284-285`) does not — it must *preserve* the accumulated count. + +Under `State::HalfOpen { probes_remaining, granted_at, consecutive_successes }` you can no +longer mutate two fields in isolation. The obvious port — +`*state = State::HalfOpen { …, consecutive_successes: 0 }` at both sites — **drops a +recorded success at the re-grant**, and no existing test catches it: +`test_half_open_probe_tokens_regrant_after_open_duration` (`:646-668`) never calls +`record_success`. + +**Remediation:** two constructors — `enter_half_open(budget)` (successes = 0) and a +payload-preserving `regrant(...)`. Add the missing test (success → advance past window → +re-grant → second success closes) **before** the refactor, so it pins current behavior. + +--- + +## T17 — MEDIUM — The plan edits 100% of the `run_resilient`/`retry_once` duplication without collapsing it + +*[simplifier].* + +The two functions perform identical breaker bookkeeping on the same three outcome classes +(`:133-136`/`:210-213` success, `:138`/`:216` connection failure, `:147`/`:220` release, +`:157-169`/`:225-233` timeout); the only real difference is escalate-vs-give-up. M1 +rewrites both `OperationTimeout` constructions and Q-A rewrites both signatures and all +four flag reads — so the plan already touches essentially the whole duplicated surface. +Consolidating is close to free now and will not be free later. + +**Remediation:** extract bookkeeping only, no control flow, as its own commit *before* M1. +The 16-test matrix passing untouched is the proof the extraction was behavior-preserving. + +--- + +## T18 — LOW/MEDIUM — Accumulated smaller findings + +- **Browser clients cannot read the new header.** Neither CORS branch sets + `expose_headers` (`routes.rs:214-217`, `:239-242`); the token appears nowhere in the + repo. The header's entire purpose is client visibility. Same pre-existing gap affects + `X-Request-Id`. *[consistency] [reviewer]* +- **`error.rs:163` is the single enforcement point for "never expose internals".** Widen + the match at `:76-158` to a 4-tuple so every arm must state its exposure decision, and a + new variant cannot compile without deciding. *[types]* +- **`OperationTimeout { message: String, … }` has zero encapsulation** — enum struct-variant + fields are as public as the enum (`lib.rs:75`), and the message duplicates the deadline + already in the payload. Compare `RequestTimeout` (`timeout.rs:79-102`): private field, + one constructor, invalid states unrepresentable. Prefer `OperationTimeout(TimeoutContext)` + with private fields, and make "on retry" a field rather than a substring asserted at + `resilience.rs:545`, `:703`. *[types] [simplifier]* +- **The Prometheus gauge is a third encoding of breaker state**, set with bare `0`/`1`/`2` + at four sites — the same convention-maintained pattern TD-09 exists to kill. Give + `CircuitState` a `gauge()` projection. *[types]* +- **`probe_budget()`'s `.max(1)` is the sole guard against a zero budget**; + `CIRCUIT_BREAKER_SUCCESS_THRESHOLD=0` is reachable (`config.rs:203-206`, `parse_env` has + no range check, `validate()` at `:244-281` checks neither breaker threshold). In release, + overflow-checks-off turns the underflow into `u32::MAX` — the probe cap silently + disappears. `failure_threshold` has no `.max(1)` equivalent at all. *[reviewer] [types]* +- **`OPERATION_TIMEOUT_SECS=0` is unvalidated** — every operation times out instantly and, + being global-provenance, records a breaker failure. A validating `Deadline` constructor + is the natural place to make that unrepresentable. *[types]* +- **Pass `Duration`, not `AppState`, to the timeout middleware.** `AppState::new` requires + a live wrapper and spawns background tasks (`state.rs:132-133`), which would push the + parity test into the testcontainers suite; `config.operation_timeout` keeps it a unit + test next to `timeout.rs:163-233`. *[architect] [tests]* +- **`release_probe` should become private** once permits exist — leaving it `pub(super)` + preserves a way to release a token nobody holds, the exact hole T6 closes. *[types]* +- **`ProbePermit::drop` deletes the operator's only stalled-recovery signal.** The + re-grant `info!` at `:284` is today the sole indication that probes are being lost; once + the permit hands tokens back it stops firing and the underlying condition goes + invisible. Add a `debug!` + counter on the Drop path; keep the re-grant `info!` as a + wedge alarm. *[silent]* +- **TD records need a `## Resolution (session NN)` section**, per the convention in + `TD-2026-07-03.md` and `TD-2026-07-04.md` — a bare status flip leaves TD-08's + "Mitigations in place (session 02)" reading as current state. *[comments]* +- **"Regenerate the registry index" implies tooling that does not exist** — no justfile, + no Makefile, no scripts dir. `docs/tech-debt/README.md` is a hand-maintained table + (TD-08 and TD-09 are rows `:15` and `:16`). *[consistency] [comments]* +- **Review artifacts are missing from the commit sequence.** Session 02 shipped + `docs(code-reviews): session-02 round-{1,2} review artifact` as commits; the plan's five + contain no equivalent. *[consistency]* +- **`panic = "abort"` makes half of M4's safety story profile-dependent.** `Drop` does not + run on panic in release, so `ProbePermit` has no panic-safety in production — the paths + that matter are *cancellation* (`tokio::time::timeout` dropping the operation future at + `resilience.rs:132`, `:209`, client disconnect, `CancellationToken` shutdown). Document + that, and test the cancellation path specifically. *[silent]* + +--- + +## Verdict + +**Do not begin implementation.** T1 invalidates M1's design premise, T2 shows the obvious +repair carries an anti-DoS regression, T3 means the plan would close TD-08 while three of +its silences remain live, and T4/T5 mean the attested "every response" scope is partly +unimplementable and partly untruthful. T6 and T7 mean M4 as written under-delivers its +headline claim and carries a runtime-wedge hazard. + +Remediation of Round 1 proceeds in batched edits to the plan, followed by Round 2 against +the remediated plan — where regressions introduced by these edits are the expected find. + +**Answers to the five open questions, as resolved by this round:** + +| Q | Answer | +|---|---| +| **Q-A** | **Yes — required, not optional.** But not `{ Global(Duration), Client(Duration) }`: the type must carry `requested` and `enforced` separately, be **constructed** where provenance is known, and expose three independent projections so the breaker predicate stays duration-derived (T1, T2). | +| **Q-B** | **No — insufficient.** Share the *value*, not a function; `clamp_deadline` already exists; and the real gap is handlers that enforce nothing at all (T5, T10). | +| **Q-C** | **Yes**, but on the `panic = "abort"` argument, and only after metrics/tracing move out of the critical sections (T9). | +| **Q-D** | **Yes** — and it is a performance *win*, not a cost; the plan's justification and its claimed arm-deletion are both wrong (T9). | +| **Q-E** | **Moot — make them sync.** Zero callers repo-wide (T14). | diff --git a/docs/code-reviews/session-03-plan-round2.md b/docs/code-reviews/session-03-plan-round2.md new file mode 100644 index 0000000..ec688c8 --- /dev/null +++ b/docs/code-reviews/session-03-plan-round2.md @@ -0,0 +1,506 @@ +# Session 03 — Plan Review, Round 2 + +**Target:** the Round-1-remediated session-03 plan, reviewed against the tree at +`tech-debt/session-03` @ `e12a513`. Scope = plan. Round 2's purpose is to catch +regressions introduced by Round 1's remediation — and it did. + +**Provenance:** config: full — step-0 gate attested by Maxim this session (8-agent +suite, both rounds, Opus-class). Agent fallbacks: none. Verification: this round's +agents each re-read their own citations; the comment lens independently re-verified +**47 citations (45 exact, 2 drifted, both LOW)** and re-derived all four disputed +counts from scratch — all four confirmed. Round 1's one drift correction +(`mod.rs:192-194` → `:194-196`) was absorbed correctly in both places. + +Reviewers cited in brackets as in Round 1. + +**Headline:** Round 1's structural findings are genuinely closed — T1, T2, T4, T6, +T7, T8, T11, T14, T15, T16, T17 all verified resolved. But the remediation created +four new defects of its own, two of them the same anti-patterns the session exists to +remove. **Do not begin implementation.** + +--- + +## R2-A — CRITICAL — TD-08's third silence was dropped in the rewrite, and M5 would pin it as a contract + +*[consistency] [architect] [reviewer] [silent] [tests] — 5 of 8.* + +Round 1 T3 named three holes. The remediation closed items 1 and 2 and **silently +dropped item 3**, while M3 still flips TD-08 to resolved. Grepping the remediated plan +for `malform|reject|ignor`, the only hit is a comment-rewrite line in M3. + +With the stock 30 s global, four distinct client intents produce one byte-identical +response: + +| Client sends | Parse result | `X-Effective-Timeout` | +|---|---|---| +| *(nothing)* | `None` | `30000` | +| `"5s"` (malformed, `timeout.rs:153-156`) | `None` | `30000` | +| `50` (below MIN, `timeout.rs:142-147`) | `None` | `30000` | +| `300000` (valid, clamped at `mod.rs:1047`) | `Some` → 30 s | `30000` | + +That is `TD-2026-07-08.md:14-16` verbatim, including its own parenthetical +*"malformed headers are warn-logged server-side as of session 02, but the client still +sees nothing."* + +**It is now worse than unfixed.** M5's `extract_request_timeout` table test enumerates +`< MIN`, `0`, `"-1"`, non-numeric, whitespace-padded, non-ASCII — and every one of +those rows would assert the *same* observable output as the absent case. Round 1 warned +about relabelling silences as contracts; a test that pins them converts an +acknowledged gap into intended behavior. + +**A second consequence [consistency]:** M1's `details` fix fires only on the 504 path, +so on a **200** response a clamped header is still indistinguishable from no header. +TD-08 item 2 asks a client to confirm its header was *honored*; a clamped header was +not honored, and a single-number echo cannot say so. + +**Root cause is type-level, one variant from fixed:** `client_requested: Option` +makes `None` mean both "absent" and "rejected". + +**Remediation options, strongest first:** (1) `400 Bad Request` on a present-but-unusable +header — v0.4.0 is already breaking, and `AppError::BadRequest` echoes its message +(`error.rs:157`); (2) a three-state source projection plus a second header +(`X-Timeout-Source: client|clamped|default|rejected-range|rejected-malformed`), +added to `expose_headers`; (3) minimum acceptable — **do not resolve TD-08**; rename +commit 6, and add a `## Residual (session 03)` section with a binding trigger. + +**Also newly found [tests]:** `timeout.rs:129-131` — the `warn!` at `:153` sits *inside* +the `to_str()` success branch, so a non-UTF-8 header value is dropped with **no log at +all**. A fourth silence class, quieter than the two already named. + +--- + +## R2-B — CRITICAL — `Deadline.outage_signal` as a stored bool re-creates the anti-pattern TD-09 exists to delete + +*[architect] [types] [simplifier].* + +`outage_signal` is *defined* as `per_attempt >= global`, but `global` is not a field. +So given a `Deadline` in isolation, nothing — not the type, not a `debug_assert`, not a +test — can check the field against its own definition. Consistency is maintained by +convention at every construction site. Round 1's own T18 condemned exactly this shape +for the Prometheus gauge ("a third encoding … the convention-maintained pattern TD-09 +exists to kill"), and the remediation introduced it in the commit that is supposed to be +the type-design win. Round 1 asked for three **projections**; the plan delivered three +**fields**. + +Both drift directions are one assignment away and both are security-relevant: + +- `outage_signal: true` with `per_attempt: 5s` (global 30 s) → a client's short deadline + feeds the shared breaker. N slow short-deadline clients open the circuit for everyone + — **T2's DoS in mirror image, introduced by T2's fix.** +- `outage_signal: false` with `per_attempt == global` → T2's exemption hole verbatim. + +Not reachable in today's call graph, but **unenforced** — and M2 adds the second builder +that makes agreement a convention. It *is* reachable today in the test module: +`mod.rs:1085-1092` builds `IggyClientWrapper` by struct literal and would gain the new +field, and M5 designates those tests as the specification. + +**Remediation:** store `global`, delete the bool. + +```rust +pub(crate) struct Deadline { + global: Duration, // non-zero by constructor + client_requested: Option, // unclamped + per_attempt: Duration, // enforced +} +impl Deadline { + pub(crate) fn is_outage_signal(&self) -> bool { self.per_attempt >= self.global } + pub(crate) fn was_clamped(&self) -> bool { + self.client_requested.is_some_and(|r| r > self.per_attempt) + } + #[must_use] pub(crate) fn narrow_to(self, d: Duration) -> Self { /* only mutator */ } +} +``` + +Smart constructors alone are **not** sufficient — they make the first derivation correct; +`with_timeout`'s clamp is where it rots. `was_clamped()` is also exactly the fact R2-A +needs, and a validating constructor discharges the `OPERATION_TIMEOUT_SECS=0` item the +plan parked in Out-of-scope. + +--- + +## R2-C — CRITICAL — M1 and M2 specify mutually exclusive construction sites, the root path was missed, and one branch deletes a pinned security property + +*[consistency] [architect] [reviewer] [types] [silent] [simplifier] — 6 of 8.* + +The plan says all three of: built **once** in `with_timeout`; carried "next to +`op_deadline`"; and `IggyClientWrapper` "gains `client_requested`". Those are three +different layouts, and M2 adds a fourth builder (the middleware). The choice is +load-bearing: M2's headline claim — *"the header cannot diverge from enforcement because +it **is** what was enforced"* — holds only under one reading. + +**Two construction sites the plan missed entirely:** +- **The root wrapper** is a struct literal at `mod.rs:226-232` inside `new()`, reached + from `main.rs:70`. It never calls `with_timeout`. +- **`unconnected_wrapper()`** at `mod.rs:1085-1092` — the fixture for + `test_with_timeout_wiring_clamps_and_is_shrink_only`. `session-02-round2.md:30-32` + records that deleting the `.min` clamp passed all 180 tests *until this fixture + existed*. It breaks on the field change and is in no test count. + +**The shrink-only property is silently at stake.** `with_timeout` clamps against +`self.op_deadline` (`mod.rs:1047`), deliberately — documented at `:1044-1046`, pinned at +`:1109-1112`, and a recorded `TD-2026-07-04` security property. If the wrapper consumes +the middleware's value verbatim, that property and its test die. If it re-clamps, the +already-stamped header can exceed what was enforced — the one failure M2 exists to +prevent — and M5's "header parity observes enforcement, not a recomputation" names a +test the architecture forbids. + +**Two further blockers on this path:** `clamp_deadline` is **private** (`mod.rs:194`), so +M2's "do not mint a twin" is unimplementable as written; and threading a new type through +`*_scoped` ripples to **13 handler signatures** plus the `OptionalFromRequestParts` impl +(`timeout.rs:109-121`) — uncounted. + +**Remediation — one construction rule, written down.** Two private constructors +(`Deadline::global`, `Deadline::for_request`) plus one narrower (`narrow_to`), routed +through by `new()`, `with_timeout`, the fixture, and the middleware. Then pick a single +authority for the header: either (a) *enforcement is authoritative* — the middleware +seeds a slot extension, `*_scoped` writes the post-narrow value into it, the middleware +stamps from the slot on the response path (this also **closes** T5 for free, because +`/health`, `/ready`, `/stats` never fill the slot and therefore omit the header rather +than advertising a fiction); or (b) *middleware is authoritative* with +`debug_assert!` that the second clamp is a no-op. **[simplifier] offers a cheaper (c):** +keep `Option` everywhere (zero handler churn), make `clamp_deadline` +`pub(crate)`, and satisfy T10 with a test that reads back the wrapper's deadline +accessor after `iggy_scoped` — ~16 fewer changed sites. + +--- + +## R2-D — HIGH — M4c's metrics hoist introduces a gauge race that leaves Prometheus permanently wrong + +*[reviewer] [silent] — both traced the interleaving independently.* + +Today every gauge write happens inside the exclusive guard (`:264` →1, `:363` →0, +`:446` →0, `:469-470` →2). The lock serializes them, so gauge writes are totally +ordered consistently with state transitions and last-writer-wins is always correct. +Under "compute an `Effect`, drop the guard, then emit": + +``` +T1: lock → Closed→Open, Effect(gauge=2) → unlock → [preempted inside metrics registry] +T2: lock → Open→HalfOpen, Effect(gauge=1) → unlock → emits 1 +T1: resumes → emits 2 +``` + +Final state HalfOpen, gauge 2. **Nothing corrects it** — the gauge is written only on +transitions, and a HalfOpen breaker with an exhausted budget may take no further +transition during a stalled recovery. The mirror case is worse: state Open, gauge 0 → +the dashboard reads "healthy" while every request gets a 503 `circuit_open`, silently. + +This is not exotic: `allow_request`'s Open→HalfOpen (`:246-265`) races +`record_failure`'s HalfOpen→Open (`:409`) on the request path during exactly the outage +the gauge exists to display, and `gauge!().set()` takes a registry shard lock — a real +preemption window. + +**And the hazard T9 was removing is unreachable in release.** `panic = "abort"` +(`Cargo.toml:96`, `:100`) means a panic under the lock kills the process, so no surviving +process holds a stale gauge. **The remediation trades a release-impossible failure for a +release-normal one.** + +**The distinction the plan is missing: counters commute, gauges do not.** +`record_circuit_breaker_open`, `record_circuit_breaker_rejection`, and the two atomics +are monotonic — hoisting them is free. `set_circuit_breaker_state` is a +last-writer-wins register and is the only order-sensitive emission in the module. + +**Remediation (preferred):** hoist tracing and the counters; **keep the single +`gauge!().set()` inside the guard.** T9's goal is 95% met and ordering is preserved by +construction. Q-C does not depend on the gauge moving. If it must come out, stamp each +`Effect` with a monotonic sequence taken under the lock and publish through a +`fetch_max` CAS guard. + +**Two docs assert the invariant being traded away and are in no doc list:** +`circuit_breaker.rs:462-464` (`open_now` — "keeping internal counters and Prometheus +metrics in lockstep … so the gauge cannot drift from the atomics") and `:310-315` +(`reject_request` — "single site … so no rejection path can forget the metrics half"). + +--- + +## R2-E — HIGH — `generation` inside `HalfOpen` cannot be monotone, and "payload-preserving regrant" points the wrong way + +*[consistency] [types].* + +Two independent defects in the T6 fix: + +1. **`Open { opened_at }` carries no generation.** On `HalfOpen(gen=N) → Open → + HalfOpen(?)`, `enter_half_open` has no previous generation to read. Every available + implementation reintroduces the leak: start at 0 → every window is generation 0; + derive from `Instant` → already ruled out by T6 (paused clock, two grants without + `advance` compare equal); read from the previous state → correct only on the regrant + path, resets to 0 through `Open`. +2. **"Payload-preserving `regrant`" is backwards on the one field that must NOT be + preserved.** The regrant path fires precisely when the budget is exhausted *and* the + window expired — i.e. when outstanding probes are presumed lost. Preserving + `generation` there lets a straggler from the old window credit the fresh budget: + T6's leak in its purest form, surviving the fix. + +**Remediation:** move the counter to `CircuitBreaker` as `probe_generation: AtomicU64` +— the struct already holds `times_opened: AtomicU32` and `requests_rejected: AtomicU64` +at `:185-187`, exact precedent — and write the disposition table into the plan: + +| field | `enter_half_open` | `regrant` | +|---|---|---| +| `probes_remaining` | `probe_budget()` | `probe_budget()` | +| `granted_at` | `now()` | `now()` | +| `consecutive_successes` | `0` | **preserve** (T16) | +| `generation` | **bump** | **bump** | + +M0.5's pin covers only the third row; add a fourth-row test. + +--- + +## R2-F — HIGH — M4d inverts 4 breaker tests, hollows 5 more, and is the only M4 commit with no stated review discipline + +*[tests].* + +Under `allow_request() -> Option`, `assert!(cb.allow_request().is_some())` +binds the `Admission` to a **temporary** that drops at the end of the statement — `Drop` +runs and the token goes straight back. Every test that spends tokens across statements +changes meaning: + +| Test | Line | Outcome | +|---|---|---| +| `test_half_open_limits_probes_to_success_threshold` | `:636-642` | budget never exhausts → `assert!(!…)` **fails** | +| `test_half_open_probe_tokens_regrant_after_open_duration` | `:656-657` | same inversion; re-grant premise collapses | +| `test_release_probe_returns_token_capped_at_budget` | `:742-766` | `release_probe` goes private → **won't compile** | +| `test_half_open_concurrent_probes_admit_exactly_the_budget` | `:733` | broken by M4c *and* M4d | +| **M0.5's own re-grant pin** | new | **fails the same way — the T16 pin dies at M4d** | + +Silently hollowed (green, testing nothing): `:533`, `:547`, `:568`, `:679`, `:699-705`. +`test_half_open_recovery_within_probe_budget` is worst — its stated property is +"probed back to Closed *without any rejection*", which passes vacuously once the budget +is never pressured. + +M4b gets `byte-identical`; M4c gets `mechanical-only`; **M4d gets nothing** — and M4d is +where semantics change *and* the oracle is rewritten, on the same lines. That is T11's +original complaint, relocated one level down. + +**Remediation:** (1) every `allow_request()` result binds to a named local — a review +checklist line, not a convention; (2) **split M4d in two** — types-and-bindings first +(`Drop` a no-op stub, all assertions still pass under today's semantics), then the +3-line behavioral delta; (3) re-assert the T16 pin after M4d with permits held. + +--- + +## R2-G — HIGH — The reconnect reclassification is stated unconditionally, would misreport a real outage, and has zero coverage + +*[silent] [tests].* + +**Unconditional is wrong.** `reconnect_bounded` bounds the wait with `self.op_deadline` +(`mod.rs:469`). On the **root** wrapper that *is* `config.operation_timeout` — the +background stats refresher (`state.rs:210-220`) and every header-less request run there. +Today they get 503 `connection_failed`, which is **correct**: a 30 s reconnect wait that +expired is strong outage evidence. M1's wording would tell them "Operation timed out. +Please try again." during a genuine broker outage — **TD-08's complaint inverted by +TD-08's fix.** Condition the branch on `client_requested.is_some()`. + +**Zero coverage, and two tests give a false green.** +`reconnect_failure_propagates_without_retry` (`:480`) and +`timeout_branch_reconnect_failure_propagates` (`:606`) both inject +`fake_reconnect(… ConnectionFailed("reconnect exhausted"))` — a closure, never +`reconnect_bounded`. They stay green through M1 and *read* as covering the path. Nothing +in the suite exercises `mod.rs:468-483` in either classification. + +**Remediation:** extract `bound_reconnect_wait(deadline, session)` — the same move +TD-2026-07-01 made for `run_resilient` — so a test can pass `std::future::pending()` +deterministically instead of racing real I/O; assert the variant *and* the 504 for both +provenances. + +*Verified safe [silent]:* propagation and classification do **not** silently change. +`is_connection_error` is applied only to the operation's error (`:137`, `:215`), never +the reconnect step's; both `reconnect().await?` sites propagate straight out. + +**One prose casualty [comments]:** `mod.rs:495-498` reads as an iff — *"A timeout counts +as a circuit-breaker failure only when this view runs at the global deadline."* After M1 +there exists a global-deadline `OperationTimeout` that records no breaker failure +(it escapes via `?` before any breaker call), which that sentence excludes. Unlisted. + +--- + +## R2-H — HIGH — `pub(super)` on the new types is a CI build break + +*[consistency] [architect] [types] [tests] — 4 of 8.* + +`AppState::{producer,consumer,iggy}_scoped` are `pub` on a re-exported type +(`state.rs:148-169`, `lib.rs:78`), and `allow_request` is `pub` on a re-exported +`CircuitBreaker` (`mod.rs:80`). Threading a crate-visible `Deadline`/`Admission`/ +`ProbePermit` through them trips rustc's `private_interfaces`, which +`ci.yml:58` (`clippy --all-targets -- -D warnings`) and `RUSTDOCFLAGS: -D warnings` +turn into failures. `TimeoutContext` inside the **public** `AppError::OperationTimeout` +must be `pub` outright. + +Note also [types]: `pub(super)` declared in `mod.rs` resolves to the crate root +(effectively `pub(crate)`), but declared in a child file it stops at `iggy_client` and +M2 cannot compile. Put `Deadline` in its own file at `pub(crate)` with private fields — +which also removes `mod.rs`'s test module's raw field access. + +**Related, [tests]:** dropping `,ignore` from the breaker usage example is infeasible as +written — doc tests compile as external crates, and the example calls `release_probe` +(`pub(super)`) and `is_connection_error` (`pub(super)`). Rewrite to the public surface +only. + +--- + +## R2-I — MEDIUM — Q-F answered unanimously: split M1 + +Every lens that addressed it said split. Measured surface: `error.rs` (new type, +variant reshape, 15-arm match widening ≈ 90 lines), `mod.rs` (field, root literal, +`reconnect_bounded` **503→504 user-visible**, `with_reconnect`, `with_timeout`, four doc +blocks, fixture, tests), `resilience.rs` (module docs, both signatures, four flag reads, +two constructions, all 14 composition tests, the `"on retry"` asserts, and the +classifier fixture at `:740` that constructs `OperationTimeout(String)`). That is +**~20 tests across three files, not 16** — and it bundles a user-visible status-code +change inside a commit typed `refactor`. + +**Recommended split:** `refactor(error)!` (error.rs only) → `refactor(iggy-client)` +(Deadline threading, behavior-identical) → `fix(iggy-client)!` (the reconnect +reclassification, ~15 lines, its own test, its own CHANGELOG line). + +--- + +## R2-J — MEDIUM — Adopt `Rejected(CircuitState)`; it is a smaller diff, not a larger one + +*[architect] [types] [simplifier].* + +`Option` preserves both defects at `resilience.rs:120-129`: two lock +acquisitions on the fail-fast path (worse once M4c makes it a `Mutex`), and the +self-documented inaccuracy at `:121-123` — the reported state may not be the one that +rejected. `reject_request` already computes the correct label under the lock at `:238`, +`:267`, `:280`, and discards it. + +`Result, CircuitState>` (or a third variant) **deletes** `resilience.rs:124` +and the `state_label` parameter, makes the client-visible message structurally accurate, +and lets the metric label plus the Prometheus gauge become `CircuitState` projections — +landing T18's dropped `gauge()` item at zero extra cost. Test churn is identical +(`.is_some()` → `.is_ok()`). Rename `allow_request` → `admit`, since a non-boolean named +"allow" misleads. + +--- + +## R2-K — MEDIUM — The plan is mis-sequenced; M4a is in the worst position, and the TD-09→TD-08 seam is clean + +*[simplifier], with [architect] concurring on M4a.* + +**Size is not the problem.** Session 02 shipped **35 files, +2503/−622 = 3125 lines** +(`268a9e8`), and `pr.yml:50-56` only *warns* above 500 — it says so in a comment. +(`CLAUDE.md`'s "error >1000" claim is stale; M3 should fix it.) + +**Ordering is the problem.** M4d deletes all four `release_probe()` sites +(`resilience.rs:147`, `:164`, `:220`, `:232`) and collapses the non-connection-error arm +— the same arms M1 edits. So M4a-at-commit-3 authors a helper that is then re-edited by +M1, M4c, **and** M4d, whose deletion erases the primary duplication the helper existed +to absorb. Its stated proof ("the 16-test matrix passing untouched") is spent on the +least valuable version, and is invalid at any later position because M1/M4c/M4d all +legitimately change those tests. + +**Move M4a after M4d — then ask whether it is needed at all.** Post-M4d the residual +duplication is three one-line calls plus one `if outage { … }` block. + +**And the strategic option: split the session on the TD-09→TD-08 seam, in that order.** +TD-09 first shrinks the `resilience.rs` surface M1 must edit; TD-09's binding trigger is +the harder one and gets discharged outright; and 03a stands alone coherently (pr.yml → +pinning test → enum → sync → RAII → TD-09 resolution). Cost to state explicitly: both +halves are breaking, so it is v0.4.0 + v0.5.0, or v0.4.0 waits for both. + +--- + +## R2-L — MEDIUM — Two lenses disagree on scope; recorded rather than resolved + +**[simplifier] says cut, [types] says keep:** + +- **The 4-tuple `error.rs` match.** [simplifier] S1: 14 mechanical edits across + unrelated arms to guard against *under*-exposure, which is the safe direction; and + `error.rs:142-155` already shows the cheaper early-return pattern. [types] M3/F3: + the 4-tuple is what makes exposure a compile-time decision — but concedes + [reviewer] F3's point that the early-return arm **escapes the discipline anyway**, + so the guarantee is partial unless the message slot widens to `Cow`. +- **`TimeoutContext` as a separate type.** [simplifier] S4: it duplicates `Deadline`'s + two core fields and names no invariant, so it is encapsulation without a purpose in a + `publish = false` crate. [types] F9: it reaches `RequestTimeout`-grade encapsulation + **only if** the constructor takes `Deadline`, in which case it is infallible and + correct. + +**Convergence:** both accept `OperationTimeout(TimeoutContext)` where +`TimeoutContext { deadline: Deadline, attempt: Attempt }` — one type, no duplicated +fields, and the invariant lives in `Deadline`. That resolves the disagreement; it needs +Maxim's call only if the 4-tuple is kept. + +--- + +## R2-M — MEDIUM — Accumulated smaller regressions and corrections + +- **M5 has no commit slot.** Its ~14 tests are distributed across commits 4-9 with no + mapping, so nobody can tell at review time whether a commit's tests were written for + it or backfilled. At minimum the 504/boundary/negative tests must precede commit 6's + TD-08 resolution. *[consistency] [simplifier]* +- **The M0.5 `error.rs` pin is hollow for the variant it names** and the plan describes + its value backwards. The load-bearing half is the **13 control arms** that must stay + byte-identical across the widening. Isolate the changing constructor behind one + test-local helper so M1 edits a helper body, never an assertion. *[consistency] [tests]* +- **The concurrency-test decision cannot be deferred** — `tokio::join!` on sync methods + does not compile, so it blocks M4c. [tests] verified both premises of a working design + (`tokio::time::Instant::now()` outside a runtime falls back to the real clock; + `std::thread::scope` works in a plain `#[test]`) and recommends a `Barrier` + 200-iteration + real-thread racer with `open_duration = ZERO`. It also rules out `spawn_blocking` + + `start_paused`, whose auto-advance fires while blocking tasks run. +- **`debug!` on the Drop path is invisible in production.** CLAUDE.md's own tiering puts + production at `RUST_LOG=info`, and this *replaces* an `info!` (`:284`). Correct levels: + no log when consumed; **`warn!`** when a probe is abandoned unrecorded (volume bounded + by the probe budget); **`warn!`** with both generations on a stale-generation discard. + The counter has no home — `src/metrics.rs` is in no commit's file list, and an + undescribed metric ships with no HELP text. *[silent]* +- **The poison branch is production-dead and the counter is false comfort.** A counter + that structurally cannot increment reads as "poisoning never happened". Use + `debug_assert!(false, …)` — which fails at the true origin in tests, the one profile + where the branch runs — and add + `#[cfg(all(not(debug_assertions), panic = "unwind"))] compile_error!(…)` so flipping + the profile is a build error rather than silent decay. *[silent]* +- **Breaker metric assertions need a new dev-dependency and a sequencing constraint.** + `metrics.rs:215-218` asserts nothing today because no recorder is installed, and + `metrics_smoke_test.rs` exists precisely because the recorder is process-global. + `metrics-util` with `debugging` gives `with_local_recorder`, which is **thread-local + and closure-scoped** — wrong for async methods, right for sync ones. So these tests + must land after M4c, and that is a genuine extra argument for Q-D the plan has not + claimed. *[tests]* +- **`tower = "0.5"` declares no features**; `ServiceExt` resolves only via axum's + feature unification. Pin `features = ["util"]` while adding the crate's first + `ServiceExt` consumer. *[tests]* +- **401/429 negative tests cannot be unit tests.** `build_router` takes `AppState` by + value and `IggyClientWrapper::new` connects (`mod.rs:234-242`), so they belong in + `tests/integration_tests.rs` under `SecureTestFixture` (`:1340-1394`) — the standard + fixture sets `api_key: None, rate_limit_rps: 0` and cannot reach either code. *[tests]* +- **Four new prose falsifications** the remediation created, all unlisted: + `mod.rs:495-498` (R2-G), `circuit_breaker.rs:462-464` and `:310-315` (R2-D), + `timeout.rs:123-126` (the doc on the function M2 rewrites most). Three are + guarantee-bearing sentences a "mechanical-only" M4c pass is instructed not to touch. + Also unlisted: `CONTRIBUTING.md:28-55` (the only human-facing statement of the commit + convention M0 changes, and it shows no `!`), `routes.rs:199-208` (the CORS doc + immediately above M2's edit), `mod.rs:1023-1040` + `:177-184`. *[comments]* +- **Corrections:** `circuit_breaker_metrics` (`mod.rs:1059`) is **already sync** — only + two accessors change, not three. M4b's proof count is **18**, not 17 (M0.5 adds one). + Commit 2's subject is **77 chars**, over `.commitlintrc.json`'s 72. `architecture.md:293/:297/:301` + carry test counts off by up to 90 and `:19-22` omits the Timeout layer entirely — worse + than the `README.md:46` drift the plan does sweep. TD-09 gets no `## Resolution` section, + and `TD-2026-07-09.md:42`'s "shape-only with no behavioral delta" will sit directly under + a resolution that disproves it. M4d names only the Closed arm at `:230`; there are two + (`:230` and `:250`). *[comments] [reviewer] [tests]* +- **Three Round-1 items resolved in code but with no test:** T5 (`/health` advertises an + unenforced deadline — documented, untested), T7 (the deadlock regression test was part + of the remediation and is absent; note a deadlock hangs rather than fails, so it needs + a watchdog), and R2-A's rejected-header case. *[tests]* +- **T18's `CircuitState::gauge()` projection was dropped** in remediation without a note. + Four bare `0`/`1`/`2` literals remain at `:264`, `:363`, `:446`, `:470`. R2-D and R2-J + both make it *more* valuable. *[silent] [tests] [simplifier]* + +--- + +## Verdict + +Round 1's remediation genuinely closed 11 of 18 themes, and every one of the 20 +orientation facts now holds against the code. But Round 2 surfaced **four +CRITICAL/HIGH regressions introduced by the remediation itself** (R2-A, R2-B, R2-C, +R2-D), two of which recreate the exact anti-patterns this session exists to remove, plus +three further HIGH findings (R2-E, R2-F, R2-G) and a CI build break (R2-H). + +Per `docs/quality-assurance.md § Double-review protocol`, CRITICAL-class regressions in +Round 2 indicate **Round 3**. Before spending it, the scope question R2-K raises should +be settled: the plan is mis-sequenced rather than merely mis-specified, and splitting on +the TD-09→TD-08 seam would remove most of the interaction that produced R2-B, R2-C, and +R2-K in the first place. A Round 3 against a re-scoped plan is a materially different — +and much cheaper — review than a Round 3 against this one. diff --git a/docs/code-reviews/session-03-plan-round3.md b/docs/code-reviews/session-03-plan-round3.md new file mode 100644 index 0000000..fb84362 --- /dev/null +++ b/docs/code-reviews/session-03-plan-round3.md @@ -0,0 +1,305 @@ +# Session 03 — Plan Review, Round 3 + +**Target:** the plan re-scoped to **TD-2026-07-09 only** (TD-2026-07-08 deferred to +session 04), reviewed against the tree at `tech-debt/session-03` @ `e12a513`. +Round 3 was triggered by Round 2's CRITICAL-class regressions, and run against a +materially smaller plan after Maxim's re-scope decision. + +**Provenance:** config: full — step-0 attestation carried from this session's gate +(8-agent suite, Opus-class). Agent fallbacks: none. Agents were asked for terse, +findings-only output this round; each verified its own citations, and the +consistency and comment lenses independently re-verified **all 40+ plan citations** +against the tree — no regressions from Rounds 1-2, no new drift. + +**Character of this round:** qualitatively different from Round 2. **Zero new +architectural regressions.** Every finding is implementation-order, test-writability, +or a mechanical detail. The design has converged; the plan has not yet. + +--- + +## R3-1 — CRITICAL — M1's second pin cannot pass at commit 1 and duplicates an M7 test + +*[consistency] [reviewer] [tests] — three independent traces, same conclusion.* + +The pin reads "mint a permit, force a re-grant, drop the permit, assert the budget did +not grow." Two independent reasons it cannot be written at commit 1: + +1. **`ProbePermit` does not exist until commit 5.** The only commit-1 mechanism is + `allow_request()` + `release_probe()`. +2. **Expressed that way it asserts the *fixed* behavior.** `release_probe` is + window-blind (`:329-338`): after a re-grant, `remaining(0) < cap(1)` → `remaining = 1`. + The budget **does** grow today. It is a red test parked for five commits. + +It is also the *same test* as M7's "straggler-after-re-grant". + +**Remediation:** delete it from M1; M7 is its only correct home. If a commit-1 signal is +wanted, it must assert today's leak with a `// KNOWN LEAK — inverts at M6` marker, and +M6's checklist must name the inversion. + +## R3-2 — HIGH — M1's first pin is vacuous as written + +*[consistency] [tests].* + +With `success_threshold = 2`, the Open→HalfOpen transition leaves `remaining = 1` +(`:257`, `:263`), so the next `allow_request` fails the `remaining == 0` test at `:270` +and **skips the re-grant block entirely** — it just spends the second token. +`consecutive_successes` is then preserved trivially because no re-grant occurred. The +test passes today and after M3 for the wrong reason, over exactly the row M3's +disposition table changes. + +**Remediation:** exhaust first — admit → `record_success` → admit → **assert the third +admit is rejected** → advance past `open_duration` → admit (this is the re-grant) → +`record_success` → assert `Closed`. Without the rejection assertion the pin cannot fail. + +## R3-3 — HIGH — The redesigned concurrency test fails as specified, and no clock value fixes it + +*[tests].* + +`open_duration = Duration::ZERO` **deletes the cap it is meant to test.** +`Instant::elapsed()` is never negative, so the re-grant guard +`granted_at.elapsed() >= ZERO` (`:273-275`) is always true: thread A takes the only +token, thread B finds `remaining == 0`, re-grants unconditionally, and is admitted. +`a.is_ok() ^ b.is_ok()` fails every iteration. + +Worse, **no real-clock value fixes it** — `open_duration` gates *both* the Open→HalfOpen +transition and the re-grant window, so any duration short enough to reach HalfOpen +without `advance()` is short enough to re-grant. + +**Remediation:** add `#[cfg(test)] fn force_half_open(&self)` granting a window under a +long `open_duration`, then race two threads *inside* HalfOpen over the token — the +actually-contended resource. Construct a fresh breaker per iteration (unstated in the +plan). + +## R3-4 — HIGH — The commit order is wrong again: M2 and M3 author code that M4 deletes + +*[architect] [simplifier], with different corrections.* + +M4 removes the read-lock pre-check block `:227-243`, which contains the `:230` Closed arm +**and** the `:238` reject site. So under the current order M2 must plumb an `Effect` +across *two* guard scopes with a third "fall through to the write lock" outcome (a +`ControlFlow`, unstated), and M3 must port the same match twice — both thrown away at M4. +This is precisely the "each pays the other's churn" failure used to justify dropping +TD-08. + +The two lenses agree the order is wrong and disagree on the fix: +- **[architect]:** M0, M1, **M4**, M2, M3, M5, M6, M7 — M4 is not purely mechanical today + anyway (poisoning helper, `compile_error!` guard, redesigned concurrency test), so the + "keep M4 mechanical" argument is already spent. Post-M4 there is one guard per method, + the "helper must never read state" invariant is trivially true, and M7's gauge tests — + the only verification M2's hoist ever gets — land one commit later instead of five. +- **[simplifier]:** M1 → **M3** → M2 → M4 — M2's only hard constraint is *before M4* + (self-deadlock under a non-reentrant `Mutex`), not before M3; merging M2 into M3 would + bury R2-D's gauge decision inside a state reshape. + +Both orders beat the current one. [architect]'s is stronger on the `:227-243` point, +which is the deletion actually causing the churn. + +## R3-5 — HIGH — The plan contradicts itself on "both Closed arms" + +*[architect].* + +M4/Q-D says the pre-check block `:227-243` disappears; M5 says `Ungated` covers "**both** +`:230` and `:250`". `:230` lives *inside* `:227-243`. Post-M4 there is exactly **one** +Closed arm. A reviewer auditing "did we cover both?" will hunt for something that no +longer exists. + +*[types] supplies the correct post-M4 mapping:* 7 return sites pre-M4, **5** after — +`Ungated` ×1 (`:250`), `Probe` ×2 (`:265`, `:288`), `Err` ×2 (`:267` Open, `:280` budget +exhausted). The plan only *implies* `:265`, which is the likeliest miss because its +decrement at `:263` is textually separated from the grant. + +## R3-6 — HIGH — M5's named permit locals will fail `-D warnings` + +*[simplifier], verified by building a scratch crate.* + +`unused_variables` fires on `Drop`-typed bindings too. With `Drop` inert and no +`consume()` in M5, every `permit` local is write-only → clippy `-D warnings` errors. + +**Remediation:** land `consume()` in M5 at all ten outcome sites; M6 then adds only +`impl Drop`. Preserves the M5/M6 split and makes M5 compile. + +*[architect] adds a related point:* with `Drop` inert, a correctly-bound site and a +mis-bound temporary are **indistinguishable**, so M5 cannot prove its own binding +discipline. Enforce it mechanically — `#[must_use]` on `Admission`, plus a grep-able ban +on `admit()` inside `assert!`/`matches!`. + +## R3-7 — HIGH — The `compile_error!` guard breaks `cargo bench` and `cargo test --release` + +*[reviewer] [simplifier].* + +Cargo forces `-C panic=unwind` for **all test and bench units regardless of profile**, so +`cfg(all(not(debug_assertions), panic = "unwind"))` fires under +`cargo bench --all-features` — which `extended-tests.yml:58` runs weekly — and under +`cargo test --release`. `cfg(panic = ...)` is stable (1.60) and does evaluate correctly; +the guard simply catches more than intended. + +**Remediation:** add `not(test)` to the `cfg`, or delete the guard — a comment at +`Cargo.toml:100` carries the same information at zero risk. [simplifier] prefers deletion. + +## R3-8 — HIGH — Projecting the metric label through `CircuitState` silently renames it + +*[architect] [types] [comments] [simplifier] — 4 of 8.* + +`circuit_breaker.rs:109` renders `HalfOpen` as `"half-open"` (hyphen); the shipped +Prometheus label at `:280` is `"half_open"` (underscore). M5's "the label becomes a +`CircuitState` projection" would rename the exported label value, falsify +`metrics.rs:13` and `:150-153`, and M7's own new label test would codify the wrong value +as intended. `Display` must stay hyphenated — it feeds the user-visible `CircuitOpen` +message at `resilience.rs:126`. + +**Remediation:** a `fn metric_label(&self) -> &'static str` explicitly distinct from +`Display`; state in M5 that label values are unchanged. + +## R3-9 — HIGH — M8's `CLAUDE.md` task is a phantom, deleted by this session's own PR #30 + +*[consistency] [comments].* + +"Fix `CLAUDE.md`'s stale 'error >1000' PR-size claim" — that text no longer exists. +Commit `9d645aa` (this session's CLAUDE.md trim) removed the entire CI/CD section; +`grep -E "1000|>500|PR size"` returns nothing across all 233 lines, in the worktree and +at `HEAD`. Round 2 asserted it from a pre-trim reading. + +**Remediation:** delete the M8 clause — an implementer "fixing" it may re-add a CI section +the trim deliberately removed. (The orientation-table row is still correct: +`pr.yml:50-56` warns only, and `:55` says so in a comment.) + +## R3-10 — MEDIUM-HIGH — No release-prep commit, and TD-08's new findings have no durable home + +*[consistency].* + +- The sequence ends at commit 8 + artifacts, then "PR; v0.4.0" — but `Cargo.toml:3` is + still `0.3.0`, `CHANGELOG.md` has a live `## [Unreleased]` Security entry, and the + repo's precedent (`274bbc3 chore(release): prepare v0.3.0`) touches `CHANGELOG.md`, + `Cargo.toml`, `Cargo.lock`, `README.md`. Add a `chore(release): prepare v0.4.0` slot. +- **The two new TD-08 findings are recorded only in untracked artifacts.** The plan points + session 04 at `docs/code-reviews/session-03-plan-round{1,2}.md`, which are committed + only at the tail slots, and the plan itself lives in scratchpad and is never committed. + `docs/tech-debt/TD-2026-07-08.md` — the record reachable from the registry index — is + untouched and still lists only two silences. **M8 must append both findings there.** + +## R3-11 — MEDIUM — Accumulated corrections + +- **`HalfOpen.generation` is a redundant copy** of `probe_generation`: both constructors + bump the atomic and write the variant under the same lock, so while the state *is* + HalfOpen the field is provably equal to the atomic. The tell is the plan's own + disposition table — `generation` is the one row whose two cells are identical. Delete + the field; compare the permit's `u64` against the atomic. *[types]* +- **`Err(CircuitState)` has no correct `Closed` arm** — `Err(Closed)` is representable and + meaningless, and `metric_label()` would need a bogus arm. Prefer + `enum Rejection { Open, ProbeBudgetExhausted }` with an exhaustive `label()` plus + `From for CircuitState` for the message. *[types]* +- **`allow_request` has three early `return`s** (`:230`, `:238`, `:265`); hoisting the + emit to a post-guard tail silently drops any Effect whose arm still returns early. + `:238` is the trap — it both bumps `requests_rejected` and returns. M2 must state that + the method becomes single-tail, and add a rejection-count assertion for the pre-check + path, which no test distinguishes from `:267` today. *[silent]* +- **M2's `:399-403` capture rationale is wrong but the action is right for a better + reason:** `open_now` never touches `consecutive_failures` — the real point is that M3's + `Open { opened_at }` **deletes the field**, so uncaptured, the path of least resistance + in M3 is to drop `failures =` from the log, losing the only report of the threshold + count at the moment the circuit opens. Make capture an **M3 prerequisite**. Same shape + at `:258-261` vs the `-= 1` at `:263`. *[silent]* +- **The re-grant `info!` rewording is factually wrong.** Under RAII the re-grant path + stays reachable for a legitimate reason — probes still **in flight** past + `open_duration`, which is the normal HalfOpen case during an outage and exactly what + `generation` exists to handle. Labelling it an invariant violation misdiagnoses a hang + and cries wolf on every real outage. Keep it as `info!` reporting outstanding probes; + the RAII-leak alarm is the *stale-generation discard*, a distinct event. *[silent]* +- **The abandoned-only counter reproduces the structural-zero defect** that killed the + poison counter: `probes_abandoned_total == 0` cannot distinguish "healthy" from "the + Drop release path is dead code". Use a disposition denominator — + `probe_dispositions_total{disposition=consumed|released|stale}`. *[silent]* +- **`debug_assert!(false)` in a `Drop`-reachable `lock()` defeats its own rationale.** + Poisoning requires a panic under the guard; in dev that panic unwinds, unwinding drops a + live permit, `Drop` calls `lock()`, the assert panics *during unwind* → **abort**, + losing the original panic's origin and the whole libtest report. Gate on + `!std::thread::panicking()`. *[reviewer] [silent]* +- **`warn!` is wrong for the routine abandon path** — drop-without-consume with a matching + generation *is* today's `release_probe` path (currently `debug!`) and fires on every + non-connection error in half-open. `debug!` + counter there; reserve `warn!` for the + stale-generation discard. Also: `CIRCUIT_BREAKER_OPEN_DURATION_SECS` is unvalidated + (`config.rs:207-209`), so `open_duration = 0` makes the re-grant path fire on every + request — "bounded by `probe_budget`" is false when the window is zero. *[silent] [simplifier]* +- **`counter!` is invoked nowhere outside `metrics.rs`** — every emitter is a `record_*` + wrapper — so a `record_circuit_breaker_probe_*` function in the `:144-156` block is + **required**, not optional, and the module-doc counter inventory at `:9-13` goes stale. + Five bare gauge literals exist, not four: `main.rs:63` also passes a bare `0`. *[silent] [types] [simplifier]* +- **Forbid `Clone` on `ProbePermit`/`Admission`** — `Copy` is already impossible + (mutually exclusive with `Drop`), but a `Clone` derive double-releases and is one line + away from reopening the leak M6 closes. Also state that `probe_generation` is **never** + reset by `force_close`/`force_open`, or M3's deletion of the hygiene-reset instinct + returns and an ancient permit matches a recycled generation. *[types]* +- **`test_release_probe_returns_token_capped_at_budget` (`:742-766`) is still unaddressed.** + Correcting Round 2: it does *not* fail on visibility — `mod tests` is a child module and + can call a fully private fn. It fails on **arity** once `release_probe` takes a + generation, and post-M6 its over-release branch is unreachable through the public path. + Keep it, renamed to signal "defensive-only branch". *[reviewer] [types] [tests]* +- **`resilience.rs:670`** is `assert!(breaker.allow_request().await, …)` — a temporary, + the exact pattern M5 bans — and sits outside the plan's audit set ("the 18 breaker + tests"). Its comment at `:667-668` also dies at M6. *[types] [comments]* +- **Doc sites attributed to the wrong commit**, each shipping one milestone stale: + `:201-223` and `:310-315` die at **M5** (rename + `Result` + the deleted `state_label`), + `:293-297` dies at **M3** (split into two constructors), and `:49`'s intra-doc link + `[CircuitBreaker::allow_request]` dies at M5. Also unlisted: `TD-2026-07-03.md:24` + (whose Resolution prose names `allow_request()` and the anti-wedge mechanism), + `pr.yml:86-87` (the contributor-facing failure message, which still shows no `!`), and + `metrics.rs:8-13`. `mod.rs:28` is an orphan — it stays true. *[comments]* +- **The `,ignore` drop is unimplementable.** `mod.rs:59` declares `mod circuit_breaker` + **private** and only three items are re-exported; after M5, `admit`/`Admission`/ + `ProbePermit` are `pub(super)`, so a doctest — which compiles as an external crate — + cannot demonstrate admission at all. Keep `,ignore`. *[reviewer] [types]* +- **Counts:** M1 adds tests, so M2's "17 byte-identical" and M3's "18" should both read + **19**. Commit #5's subject is **88** chars (not 89) and the shortened form is **72** + (not 71) — exactly at the limit, zero margin. `README.md:46` and `CLAUDE.md`'s "183 + tests" are newly falsified by this session and are unmentioned now that TD-08's doc + milestone is gone. *[consistency] [reviewer] [comments] [tests]* +- **`metrics-util` may be unnecessary** — the new risk M3 introduces is the + `CircuitState → 0/1/2` mapping, covered by a four-assert pure unit test on `gauge()` + with zero dependencies. Dropping it also deletes the artificial "must land after M4" + constraint. Check `deny.toml` before adding it either way. *[simplifier] [tests]* +- **The deadlock watchdog guards an unreachable hang** — `record_success()` releases its + guard on return, so dropping the permit afterwards cannot self-deadlock. Either use a + plain `#[test]`, or redesign to drop a permit *while* a guard is live — the shape that + can actually deadlock. *[simplifier]* +- **Make the deadlock invariant structural:** in `admit`, compute `Option` under the + guard, **drop the guard, then construct the permit**. Then no permit can exist while a + guard is live, and a future `?` added mid-function cannot deadlock. *[types] [architect]* +- **The real constraint forcing M4 before M6 is `Send`, not lifetimes** — no + `std::sync::MutexGuard` may cross an `.await`, or every handler future stops being + `Send`. Say so. *[reviewer]* + +## R3-12 — The one open design question: this session need not be breaking at all + +*[simplifier], with supporting verification from [consistency] and [types].* + +`CircuitBreaker`, `CircuitBreakerConfig`, `CircuitState` and the three +`IggyClientWrapper` accessors have **zero users outside `src/iggy_client/`** — verified +across `src`, `tests`, and `fuzz`. If the breaker methods narrow to `pub(super)` and the +three dead accessors are deleted (folded into M3), then **M4, M5 and M6 all become +non-breaking**: ~20 lines disappear, M4's Q-E vanishes entirely (converting zero-caller +dead code to sync is cost for no benefit), and v0.4.0 gets one honest changelog line +instead of three. + +The counter-consideration: narrowing `mod.rs:80`'s `pub use` is *itself* a semver break, +so a version bump is still owed — but one deliberate narrowing beats three incidental +signature breaks, and it also dissolves R3-6's visibility pressure and lets +`Admission`/`ProbePermit` be plain `pub(crate)`. + +This is a scope decision, not a defect. It is the last thing standing between this plan +and implementation. + +--- + +## Verdict + +**No new architectural regressions.** Rounds 1 and 2 each invalidated a design premise; +Round 3 found none — every finding is ordering, test-writability, or a mechanical detail, +and the two lenses that disagreed (R3-4) disagreed only about *which* better order to +adopt. That is convergence. + +**Round 4 is not indicated.** R3-1 is CRITICAL by severity but is a test-writability error +in a newly-added item, not a regression in the design; it and every other finding here is +remediable by editing the plan text, with no design question reopened except the +optional R3-12. + +Remediate Round 3, settle R3-12, and implementation may begin. From 1628224e57bf8b66a4a5777dcf5343d849083cfc Mon Sep 17 00:00:00 2001 From: mlevkov Date: Fri, 31 Jul 2026 22:42:55 -0700 Subject: [PATCH 02/16] ci(pr): accept ! breaking-change marker in commit subjects The Conventional Commits check rejected the spec's breaking-change marker: the pattern had no `!?` between the optional scope group and the colon, so `refactor(scope)!: description` could not match on either backtrack path and the job exited 1. .commitlintrc.json permits `!` via config-conventional, but no workflow runs commitlint, so this regex was the only enforcer and the two configs disagreed. A BREAKING CHANGE: footer is not a workaround. This check reads only `%s` (pr.yml:74), and release.yml:178 formats the generated release notes the same way, so a footer-declared break would be invisible in both. Verified the relaxed pattern against 14 conventional subjects plus bare and bang variants (all accepted) and 8 malformed forms -- capitalized type, no space after the colon, bang before the scope, spaced bang, double bang, empty description, unknown type, prose -- all still rejected. Also updates the two places that document the convention to humans: the job's own failure message, and CONTRIBUTING.md, whose grammar and examples showed no `!` and whose type list omitted `style` and `revert` that the regex accepts. --- .github/workflows/pr.yml | 11 ++++++++--- CONTRIBUTING.md | 11 ++++++++++- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index c0049eb..4f43386 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -73,8 +73,12 @@ jobs: # Get all commits in this PR (exclude merge commits) COMMITS=$(git log --format="%s" --no-merges origin/${{ github.base_ref }}..HEAD) - # Regex for conventional commits - PATTERN="^(feat|fix|docs|style|refactor|perf|test|build|ci|chore|revert)(\(.+\))?: .+" + # Regex for conventional commits. The optional `!` marks a breaking + # change (type[(scope)][!]: description) - it must be accepted here + # because this check reads only the subject line, so a BREAKING CHANGE + # footer would be invisible to it (and to release.yml's notes, which + # also format with %s). + PATTERN="^(feat|fix|docs|style|refactor|perf|test|build|ci|chore|revert)(\(.+\))?!?: .+" FAILED=0 while IFS= read -r commit; do @@ -83,8 +87,9 @@ jobs: if [[ ! "$commit" =~ $PATTERN ]]; then echo "::error::Non-conventional commit: '$commit'" - echo "Expected format: type(scope): description" + echo "Expected format: type(scope)!: description (scope and ! are optional)" echo "Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore, revert" + echo "Use ! to mark a breaking change, e.g. refactor(api)!: rename the client trait" FAILED=1 fi done <<< "$COMMITS" diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e9bcdda..4ce4c4f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -30,30 +30,39 @@ By participating in this project, you agree to maintain a respectful and inclusi This project uses [Conventional Commits](https://www.conventionalcommits.org/): ``` -(): +()!: [optional body] [optional footer(s)] ``` +The scope and the `!` are both optional. Use `!` to mark a breaking change — +CI checks only the subject line, so a `BREAKING CHANGE:` footer alone will not +be recognized (and would not appear in the generated release notes either). + **Types:** - `feat`: New feature - `fix`: Bug fix - `docs`: Documentation changes +- `style`: Formatting only, no code change - `refactor`: Code refactoring (no functional change) - `test`: Adding or updating tests - `chore`: Maintenance tasks - `perf`: Performance improvements - `ci`: CI/CD changes +- `revert`: Reverting a previous commit **Examples:** ``` feat(messages): add batch message validation fix(auth): handle empty API key gracefully docs(readme): update configuration examples +refactor(circuit-breaker)!: narrow the public surface ``` +Keep the subject line at 72 characters or fewer (`.commitlintrc.json`). + ## Development Setup ### Prerequisites From d3dd082b0e1ca1bc43d05a017115f4e0f92dd5ef Mon Sep 17 00:00:00 2001 From: mlevkov Date: Fri, 31 Jul 2026 23:07:56 -0700 Subject: [PATCH 03/16] test(circuit-breaker): pin half-open re-grant success accounting grant_probe_tokens has two callers with opposite intent for a field it does not touch. The HalfOpen ENTRY path zeroes consecutive_successes in a separate statement before calling it; the RE-GRANT path must preserve the count, and today does so only because nothing there writes it. TD-2026-07-09 replaces those loose field writes with whole-variant enum construction, where the obvious port sets consecutive_successes: 0 at both sites and silently discards a recorded probe success -- the breaker would then need a fresh success_threshold run after every window expiry. The test exhausts the probe budget before advancing the clock, which is what makes it discriminating: with success_threshold = 2 the entry transition leaves one token, so without spending it and asserting the rejection the re-grant branch is never reached and the assertion passes vacuously. Verified by mutation: injecting consecutive_successes = 0 at the re-grant site fails this test on the exact assertion, while all 17 pre-existing breaker tests still pass -- the gap was real and unguarded. 184 lib tests pass; fmt and clippy --all-targets -D warnings clean. --- src/iggy_client/circuit_breaker.rs | 42 ++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/iggy_client/circuit_breaker.rs b/src/iggy_client/circuit_breaker.rs index fa2cb12..ece3791 100644 --- a/src/iggy_client/circuit_breaker.rs +++ b/src/iggy_client/circuit_breaker.rs @@ -667,6 +667,48 @@ mod tests { assert_eq!(cb.state().await, CircuitState::HalfOpen); } + #[tokio::test(start_paused = true)] + async fn test_half_open_regrant_preserves_consecutive_successes() { + // TD-2026-07-09 pin. `grant_probe_tokens` is shared by two callers + // with different intent: the HalfOpen ENTRY path zeroes + // `consecutive_successes` in a separate statement before calling it, + // while the RE-GRANT path must PRESERVE the count. Replacing those + // loose field writes with whole-variant construction makes the + // obvious port zero the count at both sites, silently discarding a + // recorded probe success. Pinned here before the shape changes. + let config = CircuitBreakerConfig::new(1, 2, Duration::from_secs(30)); + let cb = CircuitBreaker::new(config); + + cb.record_failure().await; + tokio::time::advance(Duration::from_secs(30)).await; + + // Entry consumes one of the two tokens; record a success against it. + assert!(cb.allow_request().await); + cb.record_success().await; + assert_eq!(cb.state().await, CircuitState::HalfOpen); + + // Spend the second token, then exhaust the budget. Without this the + // re-grant branch is never reached and the rest passes vacuously. + assert!(cb.allow_request().await); + assert!( + !cb.allow_request().await, + "budget must be exhausted for the re-grant branch to be exercised" + ); + + // Window expiry re-grants; the success recorded above must survive. + tokio::time::advance(Duration::from_secs(30)).await; + assert!(cb.allow_request().await); + + // This second success reaches success_threshold only if the first one + // survived the re-grant - a resetting re-grant leaves it HalfOpen. + cb.record_success().await; + assert_eq!( + cb.state().await, + CircuitState::Closed, + "re-grant must preserve consecutive_successes" + ); + } + #[tokio::test(start_paused = true)] async fn test_half_open_reentry_grants_fresh_probe_tokens() { let config = CircuitBreakerConfig::new(1, 1, Duration::from_secs(30)); From 6100591d127be6104a7f263ef516e8f5fba74212 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Fri, 31 Jul 2026 23:14:32 -0700 Subject: [PATCH 04/16] refactor(circuit-breaker)!: sync mutex, breaker methods become sync Replaces tokio's RwLock with std::sync::Mutex and drops async from allow_request, record_success, record_failure, release_probe, state, force_close and force_open. The motivation is TD-2026-07-09's RAII probe permit, which must return its token from Drop. Drop cannot await, so release_probe().await can never run from a destructor; a blocking guard is the prerequisite, not a preference. The corresponding constraint is that no guard may cross an .await or every handler future stops being Send -- clippy::await_holding_lock is warn-by- default and CI denies warnings, so that is enforced rather than remembered. This is also a small win rather than a cost. tokio's RwLock::read() is a semaphore permit acquisition, not a free shared read, and the HalfOpen path previously took two locks where a mutex takes one. The read-lock fast path and its double-checked re-match both disappear, including the arm that existed only to handle another task closing the circuit during the read->write upgrade -- a race a single acquisition cannot have. Poisoning is handled by one private lock() helper. clippy::unwrap_used is denied here, and unwrap_or_else does not trip it, so no #[allow] is needed. The recovery branch is unreachable in release because panic = "abort" turns a panic under the guard into process death rather than a poisoned lock; it runs only under cargo test, where continuing on half-updated state would surface as a baffling failure in a later test. Hence the debug_assert -- and hence the !std::thread::panicking() gate around it, because poisoning implies an in-flight unwind and asserting during unwind double-panics to abort, destroying the report the assertion exists to sharpen. Cargo.toml records that the profile setting is load-bearing. The concurrency test could not be ported mechanically: a sync allow_request is not a future, so tokio::join! does not compile, and the obvious sequential rewrite would pass with the cap removed entirely. It is now two OS threads released by a Barrier, looped to amplify the window, with a fresh breaker per iteration and an open_duration long enough that the re-grant window cannot fire mid-race. Staging that race needs a test-only force_half_open, because the production path reaches HalfOpen only through allow_request, which consumes the very token under contention on the way in. Verified by mutation: forcing window_expired = true (the shape an open_duration of zero would produce) fails the redesigned test along with four others. All 17 pre-existing breaker tests keep their tokio runtime and pausable clock; only the concurrency test changed attribute. The breaker types are still publicly re-exported at this commit, so this is marked breaking. The next commits narrow that surface to pub(crate), after which the release's net public delta is that single deliberate narrowing. 184 lib tests pass; fmt, clippy --all-targets -D warnings, and rustdoc -D warnings all clean. --- Cargo.toml | 5 + src/iggy_client/circuit_breaker.rs | 372 ++++++++++++++++------------- src/iggy_client/mod.rs | 4 +- src/iggy_client/resilience.rs | 54 ++--- 4 files changed, 242 insertions(+), 193 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 97fbc1b..5460c49 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -97,4 +97,9 @@ unsafe_code = "warn" debug = true codegen-units = 1 lto = true +# Load-bearing beyond binary size: CircuitBreaker::lock recovers a poisoned +# state mutex rather than propagating, and its justification is that a panic +# under the guard aborts here instead of unwinding into a poisoned lock. The +# recovery path is therefore unreachable in release and exercised only by +# `cargo test`. Changing this means revisiting that reasoning. panic = "abort" diff --git a/src/iggy_client/circuit_breaker.rs b/src/iggy_client/circuit_breaker.rs index ece3791..4b10c65 100644 --- a/src/iggy_client/circuit_breaker.rs +++ b/src/iggy_client/circuit_breaker.rs @@ -57,8 +57,9 @@ //! ```rust,ignore //! let cb = CircuitBreaker::new(CircuitBreakerConfig::default()); //! -//! // Check if request should be allowed -//! if !cb.allow_request().await { +//! // Check if request should be allowed. The gate is synchronous; only the +//! // guarded operation itself awaits. +//! if !cb.allow_request() { //! return Err(AppError::CircuitOpen); //! } //! @@ -66,29 +67,29 @@ //! // breaker (see `resilience::run_resilient` for the real composition): //! match operation().await { //! Ok(result) => { -//! cb.record_success().await; +//! cb.record_success(); //! Ok(result) //! } //! Err(e) if is_connection_error(&e) => { -//! cb.record_failure().await; +//! cb.record_failure(); //! Err(e) //! } //! // Other errors record neither; release any half-open probe token. //! Err(e) => { -//! cb.release_probe().await; +//! cb.release_probe(); //! Err(e) //! } //! } //! ``` use std::sync::atomic::{AtomicU32, AtomicU64, Ordering}; +use std::sync::{Mutex, MutexGuard}; use std::time::Duration; -use tokio::sync::RwLock; // tokio's Instant (a thin wrapper over std's) so breaker timing follows the // pausable test clock; identical behavior in production. use tokio::time::Instant; -use tracing::{debug, info, warn}; +use tracing::{debug, error, info, warn}; /// Circuit breaker state. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -175,12 +176,17 @@ impl CircuitBreakerState { /// Thread-safe circuit breaker implementation. /// /// Prevents cascading failures by failing fast when a service is unavailable. -/// Uses RwLock internally for thread-safe state management. +/// +/// State is guarded by a synchronous [`std::sync::Mutex`] rather than an async +/// lock: every critical section is a short run of field updates that never +/// awaits, and a blocking guard is what lets a future RAII probe permit release +/// its token from `Drop`, which cannot await. No guard may be held across an +/// `.await` — `clippy::await_holding_lock` enforces that, and CI denies warnings. pub struct CircuitBreaker { /// Configuration parameters. config: CircuitBreakerConfig, - /// Internal state protected by RwLock. - state: RwLock, + /// Internal state protected by a synchronous mutex. + state: Mutex, /// Total number of times the circuit has been opened (for metrics). times_opened: AtomicU32, /// Total number of requests rejected due to open circuit (for metrics). @@ -192,12 +198,34 @@ impl CircuitBreaker { pub fn new(config: CircuitBreakerConfig) -> Self { Self { config, - state: RwLock::new(CircuitBreakerState::new()), + state: Mutex::new(CircuitBreakerState::new()), times_opened: AtomicU32::new(0), requests_rejected: AtomicU64::new(0), } } + /// Acquire the state lock, recovering the inner value if it was poisoned. + /// + /// Poisoning requires a panic while the guard is held. The release profile + /// sets `panic = "abort"` (see `Cargo.toml`), so such a panic kills the + /// process and this branch is unreachable in production; it is reachable + /// only under `cargo test`, where silently continuing on half-updated state + /// would surface as a baffling failure in some later test instead of at its + /// origin — hence the `debug_assert!`. + /// + /// The `panicking()` guard is load-bearing: poisoning implies an in-flight + /// unwind, and asserting during unwind double-panics straight to abort, + /// destroying the very test report the assertion exists to sharpen. + fn lock(&self) -> MutexGuard<'_, CircuitBreakerState> { + self.state.lock().unwrap_or_else(|poisoned| { + if !std::thread::panicking() { + debug_assert!(false, "circuit breaker state lock poisoned"); + } + error!("Circuit breaker state lock poisoned; recovering inner state"); + poisoned.into_inner() + }) + } + /// Check if a request should be allowed through the circuit breaker. /// /// Returns `true` if the request can proceed, `false` if it should be rejected. @@ -221,32 +249,15 @@ impl CircuitBreaker { /// (e.g. the operation failed with a non-connection error, which by /// design touches neither breaker counter) would otherwise leave the /// breaker half-open with zero tokens forever. - pub async fn allow_request(&self) -> bool { - // First, check with a read lock for the common cases that don't - // mutate state (Closed passes, still-Open rejects). - { - let state = self.state.read().await; - match state.state { - CircuitState::Closed => return true, - // Consuming a probe token requires the write lock below. - CircuitState::HalfOpen => {} - CircuitState::Open => { - // Check if timeout has expired - if let Some(opened_at) = state.opened_at - && opened_at.elapsed() < self.config.open_duration - { - return self.reject_request("open"); - } - // Timeout expired - need to transition to half-open - } - } - } - - // Write lock: Open -> HalfOpen transition, or HalfOpen token use. - let mut state = self.state.write().await; + pub fn allow_request(&self) -> bool { + // One exclusive acquisition covers every case. The former read-lock + // fast path existed to avoid an async write lock on the common Closed + // path; a synchronous mutex makes it unnecessary, and dropping it also + // removes the read->write upgrade race that forced the state to be + // re-matched after the second acquisition. + let mut state = self.lock(); match state.state { - // Another task closed the circuit while we waited for the lock. CircuitState::Closed => true, CircuitState::Open => { if let Some(opened_at) = state.opened_at @@ -307,6 +318,21 @@ impl CircuitBreaker { self.config.success_threshold.max(1) } + /// Test-only: enter HalfOpen with a full probe budget, spending nothing. + /// + /// The production path reaches HalfOpen only through [`Self::allow_request`], + /// which consumes the transitioning caller's token on the way in. A race + /// over the budget therefore cannot be staged through it — the setup call + /// would take the very token under contention. Granting the window directly + /// leaves the budget as the only contended resource. + #[cfg(test)] + fn force_half_open(&self) { + let mut state = self.lock(); + state.state = CircuitState::HalfOpen; + state.consecutive_successes = 0; + self.grant_probe_tokens(&mut state); + } + /// Record a rejection (counter + state-labeled metric) and return `false`. /// /// Single site for the bookkeeping so no rejection path can forget the @@ -326,8 +352,8 @@ impl CircuitBreaker { /// round-trip proving transport health — would permanently consume /// tokens and starve recovery until the re-grant window. No-op outside /// HalfOpen; capped at the granted budget. - pub(super) async fn release_probe(&self) { - let mut state = self.state.write().await; + pub(super) fn release_probe(&self) { + let mut state = self.lock(); if state.state == CircuitState::HalfOpen { let cap = self.probe_budget(); if state.half_open_probes_remaining < cap { @@ -340,8 +366,8 @@ impl CircuitBreaker { /// Record a successful operation. /// /// In HalfOpen state, consecutive successes can close the circuit. - pub async fn record_success(&self) { - let mut state = self.state.write().await; + pub fn record_success(&self) { + let mut state = self.lock(); match state.state { CircuitState::Closed => { @@ -382,8 +408,8 @@ impl CircuitBreaker { /// /// In Closed state, consecutive failures can open the circuit. /// In HalfOpen state, any failure reopens the circuit. - pub async fn record_failure(&self) { - let mut state = self.state.write().await; + pub fn record_failure(&self) { + let mut state = self.lock(); match state.state { CircuitState::Closed => { @@ -418,8 +444,8 @@ impl CircuitBreaker { } /// Get the current circuit state. - pub async fn state(&self) -> CircuitState { - self.state.read().await.state + pub fn state(&self) -> CircuitState { + self.lock().state } /// Get the number of times the circuit has been opened. @@ -433,8 +459,8 @@ impl CircuitBreaker { } /// Force the circuit to close (for testing or manual recovery). - pub async fn force_close(&self) { - let mut state = self.state.write().await; + pub fn force_close(&self) { + let mut state = self.lock(); state.state = CircuitState::Closed; state.opened_at = None; state.consecutive_failures = 0; @@ -451,8 +477,8 @@ impl CircuitBreaker { /// /// A no-op when already Open, preserving the no-refresh policy for /// `opened_at` (see `record_failure`) and keeping `times_opened` honest. - pub async fn force_open(&self) { - let mut state = self.state.write().await; + pub fn force_open(&self) { + let mut state = self.lock(); if state.state != CircuitState::Open { self.open_now(&mut state); warn!("Circuit breaker forcibly opened"); @@ -485,8 +511,8 @@ mod tests { #[tokio::test] async fn test_circuit_breaker_starts_closed() { let cb = CircuitBreaker::default(); - assert_eq!(cb.state().await, CircuitState::Closed); - assert!(cb.allow_request().await); + assert_eq!(cb.state(), CircuitState::Closed); + assert!(cb.allow_request()); } #[tokio::test] @@ -495,13 +521,13 @@ mod tests { let cb = CircuitBreaker::new(config); // Record failures below threshold - cb.record_failure().await; - cb.record_failure().await; - assert_eq!(cb.state().await, CircuitState::Closed); + cb.record_failure(); + cb.record_failure(); + assert_eq!(cb.state(), CircuitState::Closed); // One more failure should open the circuit - cb.record_failure().await; - assert_eq!(cb.state().await, CircuitState::Open); + cb.record_failure(); + assert_eq!(cb.state(), CircuitState::Open); assert_eq!(cb.times_opened(), 1); } @@ -510,11 +536,11 @@ mod tests { let config = CircuitBreakerConfig::new(1, 1, Duration::from_secs(30)); let cb = CircuitBreaker::new(config); - cb.record_failure().await; - assert_eq!(cb.state().await, CircuitState::Open); + cb.record_failure(); + assert_eq!(cb.state(), CircuitState::Open); // Requests should be rejected - assert!(!cb.allow_request().await); + assert!(!cb.allow_request()); assert_eq!(cb.requests_rejected(), 1); } @@ -523,15 +549,15 @@ mod tests { let config = CircuitBreakerConfig::new(1, 1, Duration::from_millis(10)); let cb = CircuitBreaker::new(config); - cb.record_failure().await; - assert_eq!(cb.state().await, CircuitState::Open); + cb.record_failure(); + assert_eq!(cb.state(), CircuitState::Open); // Advance the paused clock past the open window tokio::time::advance(Duration::from_millis(20)).await; // Should allow request and transition to half-open - assert!(cb.allow_request().await); - assert_eq!(cb.state().await, CircuitState::HalfOpen); + assert!(cb.allow_request()); + assert_eq!(cb.state(), CircuitState::HalfOpen); } #[tokio::test(start_paused = true)] @@ -540,19 +566,19 @@ mod tests { let cb = CircuitBreaker::new(config); // Open the circuit - cb.record_failure().await; + cb.record_failure(); tokio::time::advance(Duration::from_millis(20)).await; // Transition to half-open - assert!(cb.allow_request().await); - assert_eq!(cb.state().await, CircuitState::HalfOpen); + assert!(cb.allow_request()); + assert_eq!(cb.state(), CircuitState::HalfOpen); // Record successes - cb.record_success().await; - assert_eq!(cb.state().await, CircuitState::HalfOpen); + cb.record_success(); + assert_eq!(cb.state(), CircuitState::HalfOpen); - cb.record_success().await; - assert_eq!(cb.state().await, CircuitState::Closed); + cb.record_success(); + assert_eq!(cb.state(), CircuitState::Closed); } #[tokio::test(start_paused = true)] @@ -561,16 +587,16 @@ mod tests { let cb = CircuitBreaker::new(config); // Open the circuit - cb.record_failure().await; + cb.record_failure(); tokio::time::advance(Duration::from_millis(20)).await; // Transition to half-open - assert!(cb.allow_request().await); - assert_eq!(cb.state().await, CircuitState::HalfOpen); + assert!(cb.allow_request()); + assert_eq!(cb.state(), CircuitState::HalfOpen); // Failure should reopen - cb.record_failure().await; - assert_eq!(cb.state().await, CircuitState::Open); + cb.record_failure(); + assert_eq!(cb.state(), CircuitState::Open); assert_eq!(cb.times_opened(), 2); } @@ -579,43 +605,43 @@ mod tests { let config = CircuitBreakerConfig::new(3, 1, Duration::from_secs(30)); let cb = CircuitBreaker::new(config); - cb.record_failure().await; - cb.record_failure().await; + cb.record_failure(); + cb.record_failure(); // Success should reset the counter - cb.record_success().await; + cb.record_success(); // Now we need 3 more failures to open - cb.record_failure().await; - cb.record_failure().await; - assert_eq!(cb.state().await, CircuitState::Closed); + cb.record_failure(); + cb.record_failure(); + assert_eq!(cb.state(), CircuitState::Closed); - cb.record_failure().await; - assert_eq!(cb.state().await, CircuitState::Open); + cb.record_failure(); + assert_eq!(cb.state(), CircuitState::Open); } #[tokio::test] async fn test_force_close() { let cb = CircuitBreaker::default(); - cb.record_failure().await; - cb.record_failure().await; - cb.record_failure().await; - cb.record_failure().await; - cb.record_failure().await; - assert_eq!(cb.state().await, CircuitState::Open); + cb.record_failure(); + cb.record_failure(); + cb.record_failure(); + cb.record_failure(); + cb.record_failure(); + assert_eq!(cb.state(), CircuitState::Open); - cb.force_close().await; - assert_eq!(cb.state().await, CircuitState::Closed); - assert!(cb.allow_request().await); + cb.force_close(); + assert_eq!(cb.state(), CircuitState::Closed); + assert!(cb.allow_request()); } #[tokio::test] async fn test_force_open() { let cb = CircuitBreaker::default(); - assert_eq!(cb.state().await, CircuitState::Closed); + assert_eq!(cb.state(), CircuitState::Closed); - cb.force_open().await; - assert_eq!(cb.state().await, CircuitState::Open); - assert!(!cb.allow_request().await); + cb.force_open(); + assert_eq!(cb.state(), CircuitState::Open); + assert!(!cb.allow_request()); } // ========================================================================= @@ -627,18 +653,18 @@ mod tests { let config = CircuitBreakerConfig::new(1, 2, Duration::from_secs(30)); let cb = CircuitBreaker::new(config); - cb.record_failure().await; - assert_eq!(cb.state().await, CircuitState::Open); + cb.record_failure(); + assert_eq!(cb.state(), CircuitState::Open); tokio::time::advance(Duration::from_secs(30)).await; // success_threshold = 2 probe tokens: two callers pass, the third // is rejected instead of piling onto the recovering server. - assert!(cb.allow_request().await); - assert!(cb.allow_request().await); - assert_eq!(cb.state().await, CircuitState::HalfOpen); + assert!(cb.allow_request()); + assert!(cb.allow_request()); + assert_eq!(cb.state(), CircuitState::HalfOpen); let rejected_before = cb.requests_rejected(); - assert!(!cb.allow_request().await); + assert!(!cb.allow_request()); assert_eq!(cb.requests_rejected(), rejected_before + 1); } @@ -647,24 +673,24 @@ mod tests { let config = CircuitBreakerConfig::new(1, 1, Duration::from_secs(30)); let cb = CircuitBreaker::new(config); - cb.record_failure().await; + cb.record_failure(); tokio::time::advance(Duration::from_secs(30)).await; // Single token consumed by the transitioning caller; its outcome is // never recorded (the leaked-probe case) so the breaker sits in // HalfOpen with zero tokens. - assert!(cb.allow_request().await); - assert!(!cb.allow_request().await); + assert!(cb.allow_request()); + assert!(!cb.allow_request()); // Just below the window boundary the budget must stay exhausted - // an unconditional re-grant would defeat the probe cap entirely. tokio::time::advance(Duration::from_secs(29)).await; - assert!(!cb.allow_request().await); + assert!(!cb.allow_request()); // The re-grant window keeps the breaker from wedging permanently. tokio::time::advance(Duration::from_secs(1)).await; - assert!(cb.allow_request().await); - assert_eq!(cb.state().await, CircuitState::HalfOpen); + assert!(cb.allow_request()); + assert_eq!(cb.state(), CircuitState::HalfOpen); } #[tokio::test(start_paused = true)] @@ -679,31 +705,31 @@ mod tests { let config = CircuitBreakerConfig::new(1, 2, Duration::from_secs(30)); let cb = CircuitBreaker::new(config); - cb.record_failure().await; + cb.record_failure(); tokio::time::advance(Duration::from_secs(30)).await; // Entry consumes one of the two tokens; record a success against it. - assert!(cb.allow_request().await); - cb.record_success().await; - assert_eq!(cb.state().await, CircuitState::HalfOpen); + assert!(cb.allow_request()); + cb.record_success(); + assert_eq!(cb.state(), CircuitState::HalfOpen); // Spend the second token, then exhaust the budget. Without this the // re-grant branch is never reached and the rest passes vacuously. - assert!(cb.allow_request().await); + assert!(cb.allow_request()); assert!( - !cb.allow_request().await, + !cb.allow_request(), "budget must be exhausted for the re-grant branch to be exercised" ); // Window expiry re-grants; the success recorded above must survive. tokio::time::advance(Duration::from_secs(30)).await; - assert!(cb.allow_request().await); + assert!(cb.allow_request()); // This second success reaches success_threshold only if the first one // survived the re-grant - a resetting re-grant leaves it HalfOpen. - cb.record_success().await; + cb.record_success(); assert_eq!( - cb.state().await, + cb.state(), CircuitState::Closed, "re-grant must preserve consecutive_successes" ); @@ -714,18 +740,18 @@ mod tests { let config = CircuitBreakerConfig::new(1, 1, Duration::from_secs(30)); let cb = CircuitBreaker::new(config); - cb.record_failure().await; + cb.record_failure(); tokio::time::advance(Duration::from_secs(30)).await; // Consume the only token, then fail the probe: back to Open. - assert!(cb.allow_request().await); - cb.record_failure().await; - assert_eq!(cb.state().await, CircuitState::Open); + assert!(cb.allow_request()); + cb.record_failure(); + assert_eq!(cb.state(), CircuitState::Open); // Next half-open entry starts with a fresh token budget. tokio::time::advance(Duration::from_secs(30)).await; - assert!(cb.allow_request().await); - assert_eq!(cb.state().await, CircuitState::HalfOpen); + assert!(cb.allow_request()); + assert_eq!(cb.state(), CircuitState::HalfOpen); } #[tokio::test(start_paused = true)] @@ -735,16 +761,16 @@ mod tests { let config = CircuitBreakerConfig::new(1, 2, Duration::from_secs(30)); let cb = CircuitBreaker::new(config); - cb.record_failure().await; + cb.record_failure(); tokio::time::advance(Duration::from_secs(30)).await; - assert!(cb.allow_request().await); - cb.record_success().await; - assert!(cb.allow_request().await); - cb.record_success().await; + assert!(cb.allow_request()); + cb.record_success(); + assert!(cb.allow_request()); + cb.record_success(); - assert_eq!(cb.state().await, CircuitState::Closed); - assert!(cb.allow_request().await); + assert_eq!(cb.state(), CircuitState::Closed); + assert!(cb.allow_request()); } #[tokio::test(start_paused = true)] @@ -754,30 +780,48 @@ mod tests { let config = CircuitBreakerConfig::new(1, 0, Duration::from_secs(30)); let cb = CircuitBreaker::new(config); - cb.record_failure().await; + cb.record_failure(); tokio::time::advance(Duration::from_secs(30)).await; - assert!(cb.allow_request().await); - } - - #[tokio::test(start_paused = true)] - async fn test_half_open_concurrent_probes_admit_exactly_the_budget() { - // Two callers race allow_request at the Open->HalfOpen boundary with - // a single-token budget: exactly one may pass. On the deterministic - // paused single-thread runtime, join! interleaves both futures - // through the same write-lock protocol the production path uses. - let config = CircuitBreakerConfig::new(1, 1, Duration::from_secs(30)); - let cb = CircuitBreaker::new(config); - - cb.record_failure().await; - tokio::time::advance(Duration::from_secs(30)).await; - - let (a, b) = tokio::join!(cb.allow_request(), cb.allow_request()); - assert!( - a ^ b, - "exactly one of two racing probes may pass, got ({a}, {b})" - ); - assert_eq!(cb.state().await, CircuitState::HalfOpen); + assert!(cb.allow_request()); + } + + #[test] + fn test_half_open_concurrent_probes_admit_exactly_the_budget() { + // Two OS threads race allow_request inside HalfOpen over a one-token + // budget: exactly one may pass. + // + // This deliberately no longer uses tokio::join!. A synchronous + // allow_request is not a future, and the obvious sequential rewrite + // would still pass with the cap removed entirely - it takes two + // genuinely concurrent callers to test a cap. A Barrier releases both + // threads at once and the loop amplifies a narrow window; a fresh + // breaker per iteration keeps them independent. open_duration is far + // longer than an iteration, so the re-grant window cannot fire + // mid-race and hand the loser a second token. + for i in 0..200 { + let cb = CircuitBreaker::new(CircuitBreakerConfig::new(1, 1, Duration::from_secs(30))); + cb.force_half_open(); + + let gate = std::sync::Barrier::new(2); + let (a, b) = std::thread::scope(|s| { + let first = s.spawn(|| { + gate.wait(); + cb.allow_request() + }); + let second = s.spawn(|| { + gate.wait(); + cb.allow_request() + }); + (first.join().unwrap(), second.join().unwrap()) + }); + + assert!( + a ^ b, + "iteration {i}: exactly one of two racing probes may pass, got ({a}, {b})" + ); + assert_eq!(cb.state(), CircuitState::HalfOpen); + } } #[tokio::test(start_paused = true)] @@ -786,24 +830,24 @@ mod tests { let cb = CircuitBreaker::new(config); // No-op while Closed. - cb.release_probe().await; - assert!(cb.allow_request().await); + cb.release_probe(); + assert!(cb.allow_request()); - cb.record_failure().await; + cb.record_failure(); tokio::time::advance(Duration::from_secs(30)).await; // Consume the only token, release it, and it must admit again. - assert!(cb.allow_request().await); - assert!(!cb.allow_request().await); - cb.release_probe().await; - assert!(cb.allow_request().await); + assert!(cb.allow_request()); + assert!(!cb.allow_request()); + cb.release_probe(); + assert!(cb.allow_request()); // Releases never exceed the granted budget (single token here). - cb.release_probe().await; - cb.release_probe().await; - assert!(cb.allow_request().await); + cb.release_probe(); + cb.release_probe(); + assert!(cb.allow_request()); assert!( - !cb.allow_request().await, + !cb.allow_request(), "budget cap must hold after over-release" ); } @@ -812,10 +856,10 @@ mod tests { async fn test_force_open_when_already_open_does_not_double_count() { let cb = CircuitBreaker::default(); - cb.force_open().await; - cb.force_open().await; + cb.force_open(); + cb.force_open(); - assert_eq!(cb.state().await, CircuitState::Open); + assert_eq!(cb.state(), CircuitState::Open); assert_eq!( cb.times_opened(), 1, diff --git a/src/iggy_client/mod.rs b/src/iggy_client/mod.rs index 29baa17..f1d29af 100644 --- a/src/iggy_client/mod.rs +++ b/src/iggy_client/mod.rs @@ -1050,7 +1050,7 @@ impl IggyClientWrapper { /// Get the current circuit breaker state. pub async fn circuit_breaker_state(&self) -> CircuitState { - self.circuit_breaker.state().await + self.circuit_breaker.state() } /// Get circuit breaker metrics. @@ -1065,7 +1065,7 @@ impl IggyClientWrapper { /// Force close the circuit breaker (for manual recovery). pub async fn force_close_circuit(&self) { - self.circuit_breaker.force_close().await; + self.circuit_breaker.force_close(); } } diff --git a/src/iggy_client/resilience.rs b/src/iggy_client/resilience.rs index b71e4dd..96b16ec 100644 --- a/src/iggy_client/resilience.rs +++ b/src/iggy_client/resilience.rs @@ -117,11 +117,11 @@ where RFut: Future>, { // Check circuit breaker before attempting operation - if !breaker.allow_request().await { + if !breaker.allow_request() { // The state is re-read after the rejection, so under a concurrent // transition it reports the CURRENT state, not necessarily the one // that rejected the request. - let state = breaker.state().await; + let state = breaker.state(); return Err(AppError::CircuitOpen(format!( "Circuit breaker rejected the request (current state: {}) - service temporarily unavailable", state @@ -131,11 +131,11 @@ where // First attempt with timeout match tokio::time::timeout(timeout, operation()).await { Ok(Ok(value)) => { - breaker.record_success().await; + breaker.record_success(); Ok(value) } Ok(Err(e)) if is_connection_error(&e) => { - breaker.record_failure().await; + breaker.record_failure(); warn!(error = %e, "Operation failed due to connection error, attempting reconnect"); reconnect().await?; retry_once(breaker, timeout, timeout_is_outage_signal, &operation).await @@ -144,7 +144,7 @@ where // Non-connection error - record neither success nor failure, // but hand back any half-open probe token this request consumed // so an unrecorded outcome cannot starve recovery. - breaker.release_probe().await; + breaker.release_probe(); Err(e) } Err(_) => { @@ -155,13 +155,13 @@ where // failure. A client-shortened deadline expiring is not outage // evidence; hand back any consumed probe token instead. if timeout_is_outage_signal { - breaker.record_failure().await; + breaker.record_failure(); warn!( timeout = ?timeout, "Operation timed out at the global deadline (recorded as circuit-breaker failure)" ); } else { - breaker.release_probe().await; + breaker.release_probe(); debug!( timeout = ?timeout, "Operation timed out at a client-scoped deadline (not a breaker failure)" @@ -208,28 +208,28 @@ where { match tokio::time::timeout(timeout, operation()).await { Ok(Ok(value)) => { - breaker.record_success().await; + breaker.record_success(); Ok(value) } Ok(Err(e)) => { if is_connection_error(&e) { - breaker.record_failure().await; + breaker.record_failure(); warn!(error = %e, "Retry failed with a connection error (recorded as breaker failure)"); } else { // Unrecorded outcome: hand back any consumed probe token. - breaker.release_probe().await; + breaker.release_probe(); } Err(e) } Err(_) => { if timeout_is_outage_signal { - breaker.record_failure().await; + breaker.record_failure(); warn!( timeout = ?timeout, "Retry timed out at the global deadline (recorded as breaker failure)" ); } else { - breaker.release_probe().await; + breaker.release_probe(); } Err(AppError::OperationTimeout(format!( "Operation timed out after {:?} on retry", @@ -277,7 +277,7 @@ mod tests { #[tokio::test(start_paused = true)] async fn breaker_open_fails_fast_without_running_operation() { let breaker = breaker_with(5); - breaker.force_open().await; + breaker.force_open(); let calls = Arc::new(AtomicU32::new(0)); let reconnects = Arc::new(AtomicU32::new(0)); @@ -310,7 +310,7 @@ mod tests { // observable: the single success must close the circuit. Zero // open_duration makes the Open->HalfOpen transition immediate. let breaker = CircuitBreaker::new(CircuitBreakerConfig::new(1, 1, Duration::ZERO)); - breaker.force_open().await; + breaker.force_open(); let reconnects = Arc::new(AtomicU32::new(0)); let result: AppResult = run_resilient( @@ -324,7 +324,7 @@ mod tests { .await; assert_eq!(result.unwrap(), 42); - assert_eq!(breaker.state().await, CircuitState::Closed); + assert_eq!(breaker.state(), CircuitState::Closed); assert_eq!(reconnects.load(Ordering::SeqCst), 0); } @@ -346,7 +346,7 @@ mod tests { assert!(matches!(result, Err(AppError::OperationTimeout(_)))); assert_eq!(reconnects.load(Ordering::SeqCst), 0, "must not reconnect"); // The timeout still counts as a breaker failure (threshold 1 -> Open). - assert_eq!(breaker.state().await, CircuitState::Open); + assert_eq!(breaker.state(), CircuitState::Open); } #[tokio::test(start_paused = true)] @@ -410,7 +410,7 @@ mod tests { assert_eq!(reconnects.load(Ordering::SeqCst), 1); assert_eq!(calls.load(Ordering::SeqCst), 2); // Failure then success: the success resets the consecutive count. - assert_eq!(breaker.state().await, CircuitState::Closed); + assert_eq!(breaker.state(), CircuitState::Closed); } #[tokio::test(start_paused = true)] @@ -442,7 +442,7 @@ mod tests { assert!(matches!(result, Err(AppError::ConnectionFailed(_)))); assert_eq!(calls.load(Ordering::SeqCst), 2, "single retry, no loop"); assert_eq!(reconnects.load(Ordering::SeqCst), 1, "single reconnect"); - assert_eq!(breaker.state().await, CircuitState::Open); + assert_eq!(breaker.state(), CircuitState::Open); } #[tokio::test(start_paused = true)] @@ -471,7 +471,7 @@ mod tests { .await; assert!(matches!(result, Err(AppError::BadRequest(_)))); - assert_eq!(breaker.state().await, CircuitState::Closed); + assert_eq!(breaker.state(), CircuitState::Closed); assert_eq!(calls.load(Ordering::SeqCst), 1, "no retry"); assert_eq!(reconnects.load(Ordering::SeqCst), 0, "no reconnect"); } @@ -544,7 +544,7 @@ mod tests { assert!( matches!(&result, Err(AppError::OperationTimeout(msg)) if msg.contains("on retry")) ); - assert_eq!(breaker.state().await, CircuitState::Open); + assert_eq!(breaker.state(), CircuitState::Open); } #[tokio::test(start_paused = true)] @@ -576,7 +576,7 @@ mod tests { .await; assert!(matches!(result, Err(AppError::BadRequest(_)))); - assert_eq!(breaker.state().await, CircuitState::Closed); + assert_eq!(breaker.state(), CircuitState::Closed); } #[tokio::test(start_paused = true)] @@ -598,7 +598,7 @@ mod tests { .await; assert!(matches!(result, Err(AppError::OperationTimeout(_)))); - assert_eq!(breaker.state().await, CircuitState::Closed); + assert_eq!(breaker.state(), CircuitState::Closed); assert_eq!(reconnects.load(Ordering::SeqCst), 0); } @@ -647,8 +647,8 @@ mod tests { // error records neither counter but must hand its token back, or // recovery starves until the re-grant window. let breaker = breaker_with(1); // success_threshold 1 => single token - breaker.record_failure().await; - assert_eq!(breaker.state().await, CircuitState::Open); + breaker.record_failure(); + assert_eq!(breaker.state(), CircuitState::Open); tokio::time::advance(Duration::from_secs(30)).await; let reconnects = Arc::new(AtomicU32::new(0)); @@ -663,11 +663,11 @@ mod tests { .await; assert!(matches!(result, Err(AppError::NotFound(_)))); - assert_eq!(breaker.state().await, CircuitState::HalfOpen); + assert_eq!(breaker.state(), CircuitState::HalfOpen); // Without release_probe the single token would be gone and this // would be rejected until the re-grant window. assert!( - breaker.allow_request().await, + breaker.allow_request(), "released token must admit the next probe" ); } @@ -705,7 +705,7 @@ mod tests { assert_eq!(reconnects.load(Ordering::SeqCst), 1, "recovery still fires"); assert_eq!(calls.load(Ordering::SeqCst), 2, "single retry"); assert_eq!( - breaker.state().await, + breaker.state(), CircuitState::Closed, "scoped timeouts on both attempts must not feed the breaker" ); From 03130a7ec159dc0e13eacd7500367339bdd635c4 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Fri, 31 Jul 2026 23:21:57 -0700 Subject: [PATCH 05/16] refactor(circuit-breaker)!: narrow to crate-internal surface The breaker is a mechanism of the resilience executor, not part of this crate's API. `pub use` becomes `pub(crate) use`, and the three IggyClientWrapper accessors that exposed it -- circuit_breaker_state, circuit_breaker_metrics, force_close_circuit -- are deleted. All three had zero callers anywhere in src, tests or fuzz; they were an admin surface that was never wired to an endpoint. This is the release's single deliberate semver break. Every remaining commit in TD-2026-07-09 changes signatures on these types -- Admission, ProbePermit, the state enum -- and without the narrowing each would be a public break for no consumer's benefit. Two consequences worth naming rather than papering over: CircuitState leaves the re-export entirely. Its only crate-level user was the deleted accessor; resilience.rs's tests import it by module path. times_opened, requests_rejected, force_close and force_open become #[cfg(test)]. Deleting the accessors left them with only test callers, and dead_code is a warning CI denies. Marking them test-only states what they now are; the alternative was #[allow(dead_code)], which CLAUDE.md rules out for production code. force_close/force_open lose their "or manual recovery" doc claim in the process -- accurate, since the manual path was the accessor just removed. Verified by rustdoc: no public page is generated for CircuitBreaker or CircuitState, while IggyClientWrapper still has one. 184 lib tests pass; fmt, clippy --all-targets -D warnings and rustdoc -D warnings all clean. --- src/iggy_client/circuit_breaker.rs | 22 ++++++++++++++++++---- src/iggy_client/mod.rs | 26 +++++--------------------- 2 files changed, 23 insertions(+), 25 deletions(-) diff --git a/src/iggy_client/circuit_breaker.rs b/src/iggy_client/circuit_breaker.rs index 4b10c65..45e5396 100644 --- a/src/iggy_client/circuit_breaker.rs +++ b/src/iggy_client/circuit_breaker.rs @@ -449,16 +449,28 @@ impl CircuitBreaker { } /// Get the number of times the circuit has been opened. + /// + /// Test-only. The counters are reported to operators through the + /// Prometheus gauge and counters emitted on each transition, not through + /// this getter; it exists so tests can assert on transitions directly. + #[cfg(test)] pub fn times_opened(&self) -> u32 { self.times_opened.load(Ordering::Relaxed) } /// Get the number of requests rejected due to open circuit. + /// + /// Test-only, for the same reason as [`Self::times_opened`]. + #[cfg(test)] pub fn requests_rejected(&self) -> u64 { self.requests_rejected.load(Ordering::Relaxed) } - /// Force the circuit to close (for testing or manual recovery). + /// Force the circuit to close. + /// + /// Test-only: there is no manual-recovery path into the breaker, so the + /// only callers are tests staging a known state. + #[cfg(test)] pub fn force_close(&self) { let mut state = self.lock(); state.state = CircuitState::Closed; @@ -473,10 +485,12 @@ impl CircuitBreaker { info!("Circuit breaker forcibly closed"); } - /// Force the circuit to open (for testing or manual intervention). + /// Force the circuit to open. /// - /// A no-op when already Open, preserving the no-refresh policy for - /// `opened_at` (see `record_failure`) and keeping `times_opened` honest. + /// Test-only, as [`Self::force_close`]. A no-op when already Open, + /// preserving the no-refresh policy for `opened_at` (see `record_failure`) + /// and keeping `times_opened` honest. + #[cfg(test)] pub fn force_open(&self) { let mut state = self.lock(); if state.state != CircuitState::Open { diff --git a/src/iggy_client/mod.rs b/src/iggy_client/mod.rs index f1d29af..2627475 100644 --- a/src/iggy_client/mod.rs +++ b/src/iggy_client/mod.rs @@ -77,7 +77,11 @@ use crate::error::{AppError, AppResult}; use crate::models::Event; // Re-exports for public API -pub use circuit_breaker::{CircuitBreaker, CircuitBreakerConfig, CircuitState}; +// Crate-internal: the breaker is a mechanism of the resilience executor, not +// part of this crate's surface. Nothing outside `src/iggy_client/` referenced +// these, and keeping them public would force every subsequent signature change +// in TD-2026-07-09 to be a semver break for no consumer's benefit. +pub(crate) use circuit_breaker::{CircuitBreaker, CircuitBreakerConfig}; pub use connection::ConnectionState; pub use helpers::{rand_jitter, to_identifier}; pub use params::PollParams; @@ -1047,26 +1051,6 @@ impl IggyClientWrapper { scoped.op_deadline = clamp_deadline(timeout, self.op_deadline); scoped } - - /// Get the current circuit breaker state. - pub async fn circuit_breaker_state(&self) -> CircuitState { - self.circuit_breaker.state() - } - - /// Get circuit breaker metrics. - /// - /// Returns a tuple of (times_opened, requests_rejected). - pub fn circuit_breaker_metrics(&self) -> (u32, u64) { - ( - self.circuit_breaker.times_opened(), - self.circuit_breaker.requests_rejected(), - ) - } - - /// Force close the circuit breaker (for manual recovery). - pub async fn force_close_circuit(&self) { - self.circuit_breaker.force_close(); - } } #[cfg(test)] From 833348219eb95bb7506516a93204ae02dba5821e Mon Sep 17 00:00:00 2001 From: mlevkov Date: Fri, 31 Jul 2026 23:27:26 -0700 Subject: [PATCH 06/16] refactor(circuit-breaker): hoist tracing and counters out of the lock Each method now computes an Effect under the state guard, releases it, then emits. Logging reaches a global tracing subscriber and the metrics recorder takes a registry shard lock and allocates a label key; neither belongs inside the mutex that gates every Iggy operation, and both were the only realistic way that mutex could ever be poisoned. The Prometheus state gauge deliberately does NOT move. Unlike the counters it is a last-writer-wins register, so two racing transitions emitting after release could land out of order and leave it permanently disagreeing with the breaker -- reading "closed" while every request is rejected -- until the next transition, which during a stalled recovery may never come. set_gauge is therefore called with the guard still held, which makes gauge order match transition order by construction. The counters are monotonic and commute, so hoisting them is free. That distinction, counters commute and gauges do not, is the whole design of this commit. set_gauge also replaces four hand-written 0/1/2 literals with one exhaustive match, so a new state cannot be added without deciding its gauge value. (main.rs seeds the gauge at startup with a bare 0 and is left alone; it runs before any breaker exists.) allow_request becomes single-tail. It previously had three early returns, and a tail emit would silently skip the Effect on any arm that still returned -- including the rejection path, whose counter a test asserts. record_failure captures consecutive_failures BEFORE open_now. The state enum this refactor is preparing for carries no failure count in its Open variant, so reading it after the transition would not survive that change, and the path of least resistance would be to drop the field from the log -- losing the only report of the threshold count at the moment the circuit opens. Same reason the half-open probe budget is captured before its decrement. open_now's doc claimed counters and metrics move "in lockstep"; that is now split deliberately, so it says which half stays under the guard and why. force_close and force_open keep their logs but emit them after their guard scope closes. Verified: every tracing and metrics call in the module now sits in emit, count_open, set_gauge or the poison path; emit and count_open never take the lock, so no re-entrancy is possible; all four gauge writes remain inside a live guard and cover the same four transitions as before. 184 lib tests pass unchanged; fmt, clippy --all-targets -D warnings and rustdoc -D warnings all clean. --- src/iggy_client/circuit_breaker.rs | 423 +++++++++++++++++++---------- 1 file changed, 287 insertions(+), 136 deletions(-) diff --git a/src/iggy_client/circuit_breaker.rs b/src/iggy_client/circuit_breaker.rs index 45e5396..5902e7d 100644 --- a/src/iggy_client/circuit_breaker.rs +++ b/src/iggy_client/circuit_breaker.rs @@ -173,6 +173,43 @@ impl CircuitBreakerState { } } +/// What a completed state transition should report, emitted once the state +/// guard has been released. +/// +/// Logging reaches a global `tracing` subscriber and the metrics recorder takes +/// a registry shard lock and allocates a label key; neither belongs inside the +/// mutex that gates every Iggy operation. Everything carried here is a log line +/// or a **monotonic counter**, so reordering two concurrent transitions' +/// emissions is harmless. +/// +/// The Prometheus state gauge is deliberately NOT here — see +/// [`CircuitBreaker::set_gauge`]. +enum Effect { + /// Nothing to report. + None, + /// Open -> HalfOpen. `probes` is the budget as granted, captured before the + /// transitioning caller takes its own token. + EnteredHalfOpen { probes: u32 }, + /// A request was turned away. The label separates "circuit is open" from + /// "half-open probe budget exhausted" — materially different situations. + Rejected { + label: &'static str, + budget_exhausted: bool, + }, + /// A probe window elapsed with no recorded outcome and was re-granted. + RegrantedProbes, + /// A probe token was handed back because its outcome was never recorded. + ReleasedProbe, + /// A success landed in HalfOpen; `closed` if it reached the threshold. + HalfOpenSuccess { successes: u32, closed: bool }, + /// A success arrived after another probe's failure had reopened the circuit. + SuccessWhileOpen, + /// A failure landed in Closed; `opened` if it reached the threshold. + Failure { failures: u32, opened: bool }, + /// A failure in HalfOpen reopened the circuit. + ReopenedFromHalfOpen, +} + /// Thread-safe circuit breaker implementation. /// /// Prevents cascading failures by failing fast when a service is unavailable. @@ -226,6 +263,101 @@ impl CircuitBreaker { }) } + /// Write the Prometheus state gauge. Called **while the guard is held**. + /// + /// This is the one emission that cannot be hoisted out of the critical + /// section. The gauge is a last-writer-wins register, so if two racing + /// transitions each computed a value and emitted it after releasing the + /// guard, the older value could land second and the gauge would disagree + /// with the breaker until the next transition — which, during a stalled + /// recovery, may never come. Holding the guard makes the write order match + /// the transition order by construction. + /// + /// The single exhaustive mapping also replaces four hand-written `0`/`1`/`2` + /// literals, so a new state cannot be added without deciding its gauge value. + fn set_gauge(&self, state: CircuitState) { + crate::metrics::set_circuit_breaker_state(match state { + CircuitState::Closed => 0, + CircuitState::HalfOpen => 1, + CircuitState::Open => 2, + }); + } + + /// Monotonic bookkeeping for an Open transition, emitted after the guard is + /// released. Both are counters, so ordering against other transitions is + /// immaterial. + fn count_open(&self) { + self.times_opened.fetch_add(1, Ordering::Relaxed); + crate::metrics::record_circuit_breaker_open(); + } + + /// Emit the logs and monotonic counters for a completed transition. + /// + /// Always called with the state guard already dropped. + fn emit(&self, effect: Effect) { + match effect { + Effect::None => {} + Effect::EnteredHalfOpen { probes } => { + info!( + probes, + "Circuit breaker transitioning from Open to HalfOpen" + ); + } + Effect::Rejected { + label, + budget_exhausted, + } => { + if budget_exhausted { + debug!("Circuit breaker rejected request: half-open probe budget exhausted"); + } + self.requests_rejected.fetch_add(1, Ordering::Relaxed); + crate::metrics::record_circuit_breaker_rejection(label); + } + Effect::RegrantedProbes => { + // info: a full probe window elapsed without a recorded outcome + // - recovery is stalling, not progressing. + info!("Circuit breaker re-granted half-open probe tokens"); + } + Effect::ReleasedProbe => { + debug!("Circuit breaker released a half-open probe token (outcome not recorded)"); + } + Effect::HalfOpenSuccess { successes, closed } => { + debug!( + consecutive_successes = successes, + threshold = self.config.success_threshold, + "Circuit breaker recorded success in HalfOpen state" + ); + if closed { + info!("Circuit breaker closed after successful recovery"); + } + } + Effect::SuccessWhileOpen => { + debug!( + "Success recorded while Open (in-flight probe finished after reopen); discarded" + ); + } + Effect::Failure { failures, opened } => { + debug!( + consecutive_failures = failures, + threshold = self.config.failure_threshold, + "Circuit breaker recorded failure" + ); + if opened { + self.count_open(); + warn!( + failures, + open_duration = ?self.config.open_duration, + "Circuit breaker opened due to consecutive failures" + ); + } + } + Effect::ReopenedFromHalfOpen => { + self.count_open(); + warn!("Circuit breaker reopened after failure in HalfOpen state"); + } + } + } + /// Check if a request should be allowed through the circuit breaker. /// /// Returns `true` if the request can proceed, `false` if it should be rejected. @@ -255,50 +387,64 @@ impl CircuitBreaker { // path; a synchronous mutex makes it unnecessary, and dropping it also // removes the read->write upgrade race that forced the state to be // re-matched after the second acquisition. - let mut state = self.lock(); - - match state.state { - CircuitState::Closed => true, - CircuitState::Open => { - if let Some(opened_at) = state.opened_at - && opened_at.elapsed() >= self.config.open_duration - { - state.state = CircuitState::HalfOpen; - state.consecutive_successes = 0; - self.grant_probe_tokens(&mut state); - info!( - probes = state.half_open_probes_remaining, - "Circuit breaker transitioning from Open to HalfOpen" - ); - // The transitioning caller takes the first probe token. - state.half_open_probes_remaining -= 1; - crate::metrics::set_circuit_breaker_state(1); - return true; + // Single-tail: every arm yields (allowed, effect) rather than returning + // early, so no path can skip the emit below. + let (allowed, effect) = { + let mut state = self.lock(); + + match state.state { + CircuitState::Closed => (true, Effect::None), + CircuitState::Open => { + if let Some(opened_at) = state.opened_at + && opened_at.elapsed() >= self.config.open_duration + { + state.state = CircuitState::HalfOpen; + state.consecutive_successes = 0; + self.grant_probe_tokens(&mut state); + // Captured before the transitioning caller takes its + // token, so the log reports the budget as granted. + let probes = state.half_open_probes_remaining; + state.half_open_probes_remaining -= 1; + self.set_gauge(CircuitState::HalfOpen); + (true, Effect::EnteredHalfOpen { probes }) + } else { + ( + false, + Effect::Rejected { + label: "open", + budget_exhausted: false, + }, + ) + } } - self.reject_request("open") - } - CircuitState::HalfOpen => { - if state.half_open_probes_remaining == 0 { - // Re-grant after open_duration so leaked probes cannot - // wedge the breaker in HalfOpen (see doc above). - let window_expired = state + CircuitState::HalfOpen => { + if state.half_open_probes_remaining > 0 { + state.half_open_probes_remaining -= 1; + (true, Effect::None) + } else if state .half_open_granted_at - .is_none_or(|granted| granted.elapsed() >= self.config.open_duration); - if !window_expired { - debug!( - "Circuit breaker rejected request: half-open probe budget exhausted" - ); - return self.reject_request("half_open"); + .is_none_or(|granted| granted.elapsed() >= self.config.open_duration) + { + // Re-grant after open_duration so leaked probes cannot + // wedge the breaker in HalfOpen (see doc above). + self.grant_probe_tokens(&mut state); + state.half_open_probes_remaining -= 1; + (true, Effect::RegrantedProbes) + } else { + ( + false, + Effect::Rejected { + label: "half_open", + budget_exhausted: true, + }, + ) } - // info: a full probe window elapsed without a recorded - // outcome - recovery is stalling, not progressing. - info!("Circuit breaker re-granted half-open probe tokens"); - self.grant_probe_tokens(&mut state); } - state.half_open_probes_remaining -= 1; - true } - } + }; + + self.emit(effect); + allowed } /// Grant a fresh window of half-open probe tokens. @@ -333,18 +479,6 @@ impl CircuitBreaker { self.grant_probe_tokens(&mut state); } - /// Record a rejection (counter + state-labeled metric) and return `false`. - /// - /// Single site for the bookkeeping so no rejection path can forget the - /// metrics half; the label lets operators distinguish "circuit is open" - /// from "half-open probe budget exhausted" — materially different - /// situations. - fn reject_request(&self, state_label: &'static str) -> bool { - self.requests_rejected.fetch_add(1, Ordering::Relaxed); - crate::metrics::record_circuit_breaker_rejection(state_label); - false - } - /// Hand back a half-open probe token whose outcome was deliberately not /// recorded (non-connection errors touch neither breaker counter). /// @@ -353,55 +487,57 @@ impl CircuitBreaker { /// tokens and starve recovery until the re-grant window. No-op outside /// HalfOpen; capped at the granted budget. pub(super) fn release_probe(&self) { - let mut state = self.lock(); - if state.state == CircuitState::HalfOpen { - let cap = self.probe_budget(); - if state.half_open_probes_remaining < cap { + let effect = { + let mut state = self.lock(); + if state.state == CircuitState::HalfOpen + && state.half_open_probes_remaining < self.probe_budget() + { state.half_open_probes_remaining += 1; - debug!("Circuit breaker released a half-open probe token (outcome not recorded)"); + Effect::ReleasedProbe + } else { + Effect::None } - } + }; + self.emit(effect); } /// Record a successful operation. /// /// In HalfOpen state, consecutive successes can close the circuit. pub fn record_success(&self) { - let mut state = self.lock(); - - match state.state { - CircuitState::Closed => { - // Reset failure counter on success - state.consecutive_failures = 0; - } - CircuitState::HalfOpen => { - state.consecutive_successes += 1; - debug!( - consecutive_successes = state.consecutive_successes, - threshold = self.config.success_threshold, - "Circuit breaker recorded success in HalfOpen state" - ); + let effect = { + let mut state = self.lock(); - if state.consecutive_successes >= self.config.success_threshold { - state.state = CircuitState::Closed; - state.opened_at = None; + match state.state { + CircuitState::Closed => { + // Reset failure counter on success state.consecutive_failures = 0; - crate::metrics::set_circuit_breaker_state(0); - info!("Circuit breaker closed after successful recovery"); + Effect::None + } + CircuitState::HalfOpen => { + state.consecutive_successes += 1; + let successes = state.consecutive_successes; + let closed = successes >= self.config.success_threshold; + if closed { + state.state = CircuitState::Closed; + state.opened_at = None; + state.consecutive_failures = 0; + self.set_gauge(CircuitState::Closed); + } + Effect::HalfOpenSuccess { successes, closed } + } + CircuitState::Open => { + // Reachable through legitimate interleavings: a half-open + // probe (or its post-reconnect retry, which bypasses the + // gate) can complete successfully after another probe's + // failure reopened the circuit. The success is deliberately + // discarded - recovery restarts from the next half-open + // window's probes. + Effect::SuccessWhileOpen } } - CircuitState::Open => { - // Reachable through legitimate interleavings: a half-open - // probe (or its post-reconnect retry, which bypasses the - // gate) can complete successfully after another probe's - // failure reopened the circuit. The success is deliberately - // discarded - recovery restarts from the next half-open - // window's probes. - debug!( - "Success recorded while Open (in-flight probe finished after reopen); discarded" - ); - } - } + }; + self.emit(effect); } /// Record a failed operation. @@ -409,38 +545,38 @@ impl CircuitBreaker { /// In Closed state, consecutive failures can open the circuit. /// In HalfOpen state, any failure reopens the circuit. pub fn record_failure(&self) { - let mut state = self.lock(); - - match state.state { - CircuitState::Closed => { - state.consecutive_failures += 1; - debug!( - consecutive_failures = state.consecutive_failures, - threshold = self.config.failure_threshold, - "Circuit breaker recorded failure" - ); - - if state.consecutive_failures >= self.config.failure_threshold { + let effect = { + let mut state = self.lock(); + + match state.state { + CircuitState::Closed => { + state.consecutive_failures += 1; + // Captured before open_now: the state enum this refactor is + // preparing for carries no failure count in its Open + // variant, so reading it after the transition would not + // survive that change. + let failures = state.consecutive_failures; + let opened = failures >= self.config.failure_threshold; + if opened { + self.open_now(&mut state); + } + Effect::Failure { failures, opened } + } + CircuitState::HalfOpen => { + // Any failure in half-open state reopens the circuit + state.consecutive_successes = 0; self.open_now(&mut state); - warn!( - failures = state.consecutive_failures, - open_duration = ?self.config.open_duration, - "Circuit breaker opened due to consecutive failures" - ); + Effect::ReopenedFromHalfOpen + } + CircuitState::Open => { + // Already open. Deliberately do NOT refresh opened_at: + // straggler failures from in-flight requests would otherwise + // extend the open window indefinitely and delay recovery. + Effect::None } } - CircuitState::HalfOpen => { - // Any failure in half-open state reopens the circuit - state.consecutive_successes = 0; - self.open_now(&mut state); - warn!("Circuit breaker reopened after failure in HalfOpen state"); - } - CircuitState::Open => { - // Already open. Deliberately do NOT refresh opened_at: - // straggler failures from in-flight requests would otherwise - // extend the open window indefinitely and delay recovery. - } - } + }; + self.emit(effect); } /// Get the current circuit state. @@ -472,16 +608,18 @@ impl CircuitBreaker { /// only callers are tests staging a known state. #[cfg(test)] pub fn force_close(&self) { - let mut state = self.lock(); - state.state = CircuitState::Closed; - state.opened_at = None; - state.consecutive_failures = 0; - state.consecutive_successes = 0; - // Hygiene: half-open probe fields are re-granted on every HalfOpen - // entry, but stale values should not outlive a manual reset. - state.half_open_probes_remaining = 0; - state.half_open_granted_at = None; - crate::metrics::set_circuit_breaker_state(0); + { + let mut state = self.lock(); + state.state = CircuitState::Closed; + state.opened_at = None; + state.consecutive_failures = 0; + state.consecutive_successes = 0; + // Hygiene: half-open probe fields are re-granted on every HalfOpen + // entry, but stale values should not outlive a manual reset. + state.half_open_probes_remaining = 0; + state.half_open_granted_at = None; + self.set_gauge(CircuitState::Closed); + } info!("Circuit breaker forcibly closed"); } @@ -492,22 +630,35 @@ impl CircuitBreaker { /// and keeping `times_opened` honest. #[cfg(test)] pub fn force_open(&self) { - let mut state = self.lock(); - if state.state != CircuitState::Open { - self.open_now(&mut state); + let opened = { + let mut state = self.lock(); + let changed = state.state != CircuitState::Open; + if changed { + self.open_now(&mut state); + } + changed + }; + if opened { + self.count_open(); warn!("Circuit breaker forcibly opened"); } } - /// Transition to Open, keeping internal counters and Prometheus metrics - /// in lockstep. Shared by the threshold, half-open-failure, and forced - /// open transitions so the gauge cannot drift from the atomics. + /// Transition to Open under the state guard. + /// + /// Shared by the threshold, half-open-failure and forced-open paths so the + /// gauge cannot drift from the state: [`Self::set_gauge`] runs here, still + /// holding the guard, which is what keeps gauge writes ordered with + /// transitions. + /// + /// The `times_opened` atomic and the `circuit_breaker_open` counter are + /// deliberately NOT bumped here — they are monotonic, so they move to + /// [`Self::count_open`] and are emitted after the guard is released. + /// Callers must pair this with the matching [`Effect`]. fn open_now(&self, state: &mut CircuitBreakerState) { state.state = CircuitState::Open; state.opened_at = Some(Instant::now()); - self.times_opened.fetch_add(1, Ordering::Relaxed); - crate::metrics::record_circuit_breaker_open(); - crate::metrics::set_circuit_breaker_state(2); + self.set_gauge(CircuitState::Open); } } From 50612ac220f44c518f7ed8423a0f6a1d40c6faa7 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Fri, 31 Jul 2026 23:35:01 -0700 Subject: [PATCH 07/16] refactor(circuit-breaker): flat struct to state enum with payloads Each variant now owns exactly the data meaningful while the breaker is in it: Closed { consecutive_failures }, Open { opened_at }, HalfOpen { probes_remaining, granted_at, consecutive_successes }. The flat struct kept every field in every state, so opened_at was Some only in Open, the two half-open fields only in HalfOpen, and consecutive_failures only in Closed -- invariants maintained by convention at each mutation site and re-established by hand on every transition. Deleted outright: both Options, the is_none_or window guard, force_close's six-field hygiene reset (one assignment now; the enum drops stale data with the variant), the three defensive resets on entry/close/reopen, and grant_probe_tokens, whose two callers wanted different things from it. That difference is now documented as a table on the type and expressed at both sites. Entering HalfOpen is a NEW recovery attempt, so consecutive_successes starts at zero; re-granting an expired window is the SAME attempt continuing, so it must be preserved. Writing a zero at both sites is the tempting port in either shape, and it silently discards a recorded probe success. CircuitBreakerState disappears entirely -- the mutex now guards State directly rather than a wrapper whose only remaining field was the state itself. CircuitState gains gauge(), an exhaustive projection with no _ arm, so a new state cannot be added without deciding what operators see. It is deliberately separate from Display, whose "half-open" rendering is user-facing prose and differs from the metric vocabulary. From<&State> for CircuitState is likewise exhaustive, making a new internal variant a compile error rather than a silent mis-projection. Behavior preservation: no test assertion changed. The diff touches the test module in exactly one place -- a comment on the M1 pin that described the pre-refactor shape as if it were current, which this commit falsified. Every other line in the 18-test module is untouched, verified by inspecting the diff hunks rather than by eye. Verified by mutation in the new shape: zeroing consecutive_successes on the re-grant arm fails the pin on its own assertion, so the enum port did not quietly lose the distinction the pin was written to guard. 184 lib tests pass; fmt, clippy --all-targets -D warnings and rustdoc -D warnings all clean. --- src/iggy_client/circuit_breaker.rs | 297 +++++++++++++++++------------ 1 file changed, 176 insertions(+), 121 deletions(-) diff --git a/src/iggy_client/circuit_breaker.rs b/src/iggy_client/circuit_breaker.rs index 5902e7d..f1bb318 100644 --- a/src/iggy_client/circuit_breaker.rs +++ b/src/iggy_client/circuit_breaker.rs @@ -112,6 +112,24 @@ impl std::fmt::Display for CircuitState { } } +impl CircuitState { + /// Value for the `iggy_circuit_breaker_state` Prometheus gauge. + /// + /// The single mapping for what used to be four hand-written `0`/`1`/`2` + /// literals; exhaustive with no `_` arm, so a new state cannot be added + /// without deciding what operators should see. + /// + /// Deliberately not [`Display`], whose rendering is user-facing prose + /// (`"half-open"`, hyphenated) and differs from the metric vocabulary. + fn gauge(self) -> u8 { + match self { + CircuitState::Closed => 0, + CircuitState::HalfOpen => 1, + CircuitState::Open => 2, + } + } +} + /// Configuration for the circuit breaker. #[derive(Debug, Clone)] pub struct CircuitBreakerConfig { @@ -144,31 +162,64 @@ impl CircuitBreakerConfig { } } -/// Internal state for the circuit breaker. -struct CircuitBreakerState { - /// Current circuit state. - state: CircuitState, - /// When the circuit was opened (for timeout calculation). - opened_at: Option, - /// Number of consecutive failures (in closed state). - consecutive_failures: u32, - /// Number of consecutive successes (in half-open state). - consecutive_successes: u32, - /// Probe tokens remaining in the current half-open window. - half_open_probes_remaining: u32, - /// When the current half-open probe window was granted (for re-grant). - half_open_granted_at: Option, +/// Internal state, with each variant owning exactly the data that is +/// meaningful while the breaker is in it. +/// +/// The flat struct this replaces kept every field in every state, so +/// `opened_at` was `Some` only in Open, the two half-open fields only in +/// HalfOpen, and `consecutive_failures` only in Closed — invariants maintained +/// by convention at each mutation site, and re-established by hand on every +/// transition. Payload variants make a stale field unrepresentable instead. +/// +/// # Entering versus re-granting a probe window +/// +/// Both paths refill `probes_remaining` and stamp `granted_at`, and they differ +/// on exactly one field: +/// +/// | field | enter (Open -> HalfOpen) | re-grant (within HalfOpen) | +/// |---|---|---| +/// | `probes_remaining` | budget | budget | +/// | `granted_at` | now | now | +/// | `consecutive_successes` | **0** | **preserved** | +/// +/// Entering is a new recovery attempt, so probes recorded against the previous +/// one must not count toward closing. A re-grant is the *same* attempt +/// continuing — its window simply expired with probes unaccounted for — so a +/// success already recorded still counts. Zeroing it there would silently +/// discard a probe success and force a full fresh run after every window +/// expiry. Pinned by `test_half_open_regrant_preserves_consecutive_successes`. +enum State { + /// Normal operation. Consecutive failures accumulate toward the threshold. + Closed { consecutive_failures: u32 }, + /// Failing fast until `open_duration` elapses from `opened_at`. + Open { opened_at: Instant }, + /// Token-limited recovery probing. + HalfOpen { + probes_remaining: u32, + granted_at: Instant, + consecutive_successes: u32, + }, } -impl CircuitBreakerState { - fn new() -> Self { - Self { - state: CircuitState::Closed, - opened_at: None, +impl State { + /// The initial state: closed, with no failures recorded. + fn initial() -> Self { + State::Closed { consecutive_failures: 0, - consecutive_successes: 0, - half_open_probes_remaining: 0, - half_open_granted_at: None, + } + } +} + +/// Projection to the public, data-less state. +/// +/// Deliberately exhaustive with no `_` arm, so adding an internal variant is a +/// compile error here rather than a silent mis-projection. +impl From<&State> for CircuitState { + fn from(state: &State) -> Self { + match state { + State::Closed { .. } => CircuitState::Closed, + State::Open { .. } => CircuitState::Open, + State::HalfOpen { .. } => CircuitState::HalfOpen, } } } @@ -223,7 +274,7 @@ pub struct CircuitBreaker { /// Configuration parameters. config: CircuitBreakerConfig, /// Internal state protected by a synchronous mutex. - state: Mutex, + state: Mutex, /// Total number of times the circuit has been opened (for metrics). times_opened: AtomicU32, /// Total number of requests rejected due to open circuit (for metrics). @@ -235,7 +286,7 @@ impl CircuitBreaker { pub fn new(config: CircuitBreakerConfig) -> Self { Self { config, - state: Mutex::new(CircuitBreakerState::new()), + state: Mutex::new(State::initial()), times_opened: AtomicU32::new(0), requests_rejected: AtomicU64::new(0), } @@ -253,7 +304,7 @@ impl CircuitBreaker { /// The `panicking()` guard is load-bearing: poisoning implies an in-flight /// unwind, and asserting during unwind double-panics straight to abort, /// destroying the very test report the assertion exists to sharpen. - fn lock(&self) -> MutexGuard<'_, CircuitBreakerState> { + fn lock(&self) -> MutexGuard<'_, State> { self.state.lock().unwrap_or_else(|poisoned| { if !std::thread::panicking() { debug_assert!(false, "circuit breaker state lock poisoned"); @@ -273,14 +324,10 @@ impl CircuitBreaker { /// recovery, may never come. Holding the guard makes the write order match /// the transition order by construction. /// - /// The single exhaustive mapping also replaces four hand-written `0`/`1`/`2` - /// literals, so a new state cannot be added without deciding its gauge value. + /// The value itself comes from [`CircuitState::gauge`], the single + /// exhaustive mapping that replaced four hand-written literals. fn set_gauge(&self, state: CircuitState) { - crate::metrics::set_circuit_breaker_state(match state { - CircuitState::Closed => 0, - CircuitState::HalfOpen => 1, - CircuitState::Open => 2, - }); + crate::metrics::set_circuit_breaker_state(state.gauge()); } /// Monotonic bookkeeping for an Open transition, emitted after the guard is @@ -390,23 +437,26 @@ impl CircuitBreaker { // Single-tail: every arm yields (allowed, effect) rather than returning // early, so no path can skip the emit below. let (allowed, effect) = { - let mut state = self.lock(); - - match state.state { - CircuitState::Closed => (true, Effect::None), - CircuitState::Open => { - if let Some(opened_at) = state.opened_at - && opened_at.elapsed() >= self.config.open_duration - { - state.state = CircuitState::HalfOpen; - state.consecutive_successes = 0; - self.grant_probe_tokens(&mut state); - // Captured before the transitioning caller takes its - // token, so the log reports the budget as granted. - let probes = state.half_open_probes_remaining; - state.half_open_probes_remaining -= 1; + let mut guard = self.lock(); + let budget = self.probe_budget(); + + match &mut *guard { + State::Closed { .. } => (true, Effect::None), + State::Open { opened_at } => { + if opened_at.elapsed() >= self.config.open_duration { + // A NEW recovery attempt: the success count starts at + // zero (contrast the re-grant arm below). The + // transitioning caller takes the first of the granted + // tokens as it passes, so the window opens at + // budget - 1; probe_budget() floors at 1, so this + // cannot underflow. + *guard = State::HalfOpen { + probes_remaining: budget - 1, + granted_at: Instant::now(), + consecutive_successes: 0, + }; self.set_gauge(CircuitState::HalfOpen); - (true, Effect::EnteredHalfOpen { probes }) + (true, Effect::EnteredHalfOpen { probes: budget }) } else { ( false, @@ -417,18 +467,21 @@ impl CircuitBreaker { ) } } - CircuitState::HalfOpen => { - if state.half_open_probes_remaining > 0 { - state.half_open_probes_remaining -= 1; + State::HalfOpen { + probes_remaining, + granted_at, + .. + } => { + if *probes_remaining > 0 { + *probes_remaining -= 1; (true, Effect::None) - } else if state - .half_open_granted_at - .is_none_or(|granted| granted.elapsed() >= self.config.open_duration) - { + } else if granted_at.elapsed() >= self.config.open_duration { // Re-grant after open_duration so leaked probes cannot - // wedge the breaker in HalfOpen (see doc above). - self.grant_probe_tokens(&mut state); - state.half_open_probes_remaining -= 1; + // wedge the breaker in HalfOpen (see doc above). This + // is the SAME recovery attempt continuing, so + // consecutive_successes is deliberately untouched. + *probes_remaining = budget - 1; + *granted_at = Instant::now(); (true, Effect::RegrantedProbes) } else { ( @@ -447,16 +500,6 @@ impl CircuitBreaker { allowed } - /// Grant a fresh window of half-open probe tokens. - /// - /// `success_threshold` tokens (at least one, so a zero threshold cannot - /// deadlock the breaker) — exactly enough probes to close the circuit - /// if all of them succeed. - fn grant_probe_tokens(&self, state: &mut CircuitBreakerState) { - state.half_open_probes_remaining = self.probe_budget(); - state.half_open_granted_at = Some(Instant::now()); - } - /// Half-open probe budget: `success_threshold`, floored at one so a /// zero threshold cannot deadlock the breaker. Single definition shared /// by the grant and release paths so the cap cannot drift. @@ -473,10 +516,12 @@ impl CircuitBreaker { /// leaves the budget as the only contended resource. #[cfg(test)] fn force_half_open(&self) { - let mut state = self.lock(); - state.state = CircuitState::HalfOpen; - state.consecutive_successes = 0; - self.grant_probe_tokens(&mut state); + let mut guard = self.lock(); + *guard = State::HalfOpen { + probes_remaining: self.probe_budget(), + granted_at: Instant::now(), + consecutive_successes: 0, + }; } /// Hand back a half-open probe token whose outcome was deliberately not @@ -488,11 +533,14 @@ impl CircuitBreaker { /// HalfOpen; capped at the granted budget. pub(super) fn release_probe(&self) { let effect = { - let mut state = self.lock(); - if state.state == CircuitState::HalfOpen - && state.half_open_probes_remaining < self.probe_budget() + let mut guard = self.lock(); + let budget = self.probe_budget(); + if let State::HalfOpen { + probes_remaining, .. + } = &mut *guard + && *probes_remaining < budget { - state.half_open_probes_remaining += 1; + *probes_remaining += 1; Effect::ReleasedProbe } else { Effect::None @@ -506,27 +554,32 @@ impl CircuitBreaker { /// In HalfOpen state, consecutive successes can close the circuit. pub fn record_success(&self) { let effect = { - let mut state = self.lock(); + let mut guard = self.lock(); - match state.state { - CircuitState::Closed => { + match &mut *guard { + State::Closed { + consecutive_failures, + } => { // Reset failure counter on success - state.consecutive_failures = 0; + *consecutive_failures = 0; Effect::None } - CircuitState::HalfOpen => { - state.consecutive_successes += 1; - let successes = state.consecutive_successes; + State::HalfOpen { + consecutive_successes, + .. + } => { + *consecutive_successes += 1; + let successes = *consecutive_successes; let closed = successes >= self.config.success_threshold; if closed { - state.state = CircuitState::Closed; - state.opened_at = None; - state.consecutive_failures = 0; + *guard = State::Closed { + consecutive_failures: 0, + }; self.set_gauge(CircuitState::Closed); } Effect::HalfOpenSuccess { successes, closed } } - CircuitState::Open => { + State::Open { .. } => { // Reachable through legitimate interleavings: a half-open // probe (or its post-reconnect retry, which bypasses the // gate) can complete successfully after another probe's @@ -546,29 +599,31 @@ impl CircuitBreaker { /// In HalfOpen state, any failure reopens the circuit. pub fn record_failure(&self) { let effect = { - let mut state = self.lock(); + let mut guard = self.lock(); - match state.state { - CircuitState::Closed => { - state.consecutive_failures += 1; + match &mut *guard { + State::Closed { + consecutive_failures, + } => { + *consecutive_failures += 1; // Captured before open_now: the state enum this refactor is // preparing for carries no failure count in its Open // variant, so reading it after the transition would not // survive that change. - let failures = state.consecutive_failures; + let failures = *consecutive_failures; let opened = failures >= self.config.failure_threshold; if opened { - self.open_now(&mut state); + self.open_now(&mut guard); } Effect::Failure { failures, opened } } - CircuitState::HalfOpen => { - // Any failure in half-open state reopens the circuit - state.consecutive_successes = 0; - self.open_now(&mut state); + State::HalfOpen { .. } => { + // Any failure in half-open state reopens the circuit; the + // accumulated success count dies with the variant. + self.open_now(&mut guard); Effect::ReopenedFromHalfOpen } - CircuitState::Open => { + State::Open { .. } => { // Already open. Deliberately do NOT refresh opened_at: // straggler failures from in-flight requests would otherwise // extend the open window indefinitely and delay recovery. @@ -581,7 +636,7 @@ impl CircuitBreaker { /// Get the current circuit state. pub fn state(&self) -> CircuitState { - self.lock().state + CircuitState::from(&*self.lock()) } /// Get the number of times the circuit has been opened. @@ -609,15 +664,13 @@ impl CircuitBreaker { #[cfg(test)] pub fn force_close(&self) { { - let mut state = self.lock(); - state.state = CircuitState::Closed; - state.opened_at = None; - state.consecutive_failures = 0; - state.consecutive_successes = 0; - // Hygiene: half-open probe fields are re-granted on every HalfOpen - // entry, but stale values should not outlive a manual reset. - state.half_open_probes_remaining = 0; - state.half_open_granted_at = None; + let mut guard = self.lock(); + // One assignment replaces six field resets. The old flat struct + // needed explicit hygiene so stale half-open values could not + // outlive the reset; the enum drops them with the variant. + *guard = State::Closed { + consecutive_failures: 0, + }; self.set_gauge(CircuitState::Closed); } info!("Circuit breaker forcibly closed"); @@ -631,10 +684,10 @@ impl CircuitBreaker { #[cfg(test)] pub fn force_open(&self) { let opened = { - let mut state = self.lock(); - let changed = state.state != CircuitState::Open; + let mut guard = self.lock(); + let changed = !matches!(*guard, State::Open { .. }); if changed { - self.open_now(&mut state); + self.open_now(&mut guard); } changed }; @@ -655,9 +708,10 @@ impl CircuitBreaker { /// deliberately NOT bumped here — they are monotonic, so they move to /// [`Self::count_open`] and are emitted after the guard is released. /// Callers must pair this with the matching [`Effect`]. - fn open_now(&self, state: &mut CircuitBreakerState) { - state.state = CircuitState::Open; - state.opened_at = Some(Instant::now()); + fn open_now(&self, state: &mut State) { + *state = State::Open { + opened_at: Instant::now(), + }; self.set_gauge(CircuitState::Open); } } @@ -860,13 +914,14 @@ mod tests { #[tokio::test(start_paused = true)] async fn test_half_open_regrant_preserves_consecutive_successes() { - // TD-2026-07-09 pin. `grant_probe_tokens` is shared by two callers - // with different intent: the HalfOpen ENTRY path zeroes - // `consecutive_successes` in a separate statement before calling it, - // while the RE-GRANT path must PRESERVE the count. Replacing those - // loose field writes with whole-variant construction makes the - // obvious port zero the count at both sites, silently discarding a - // recorded probe success. Pinned here before the shape changes. + // TD-2026-07-09. Entering HalfOpen resets `consecutive_successes`; + // re-granting an expired window inside HalfOpen must PRESERVE it. The + // two paths sat one statement apart in the flat struct and are now two + // arms of the state enum, and in both shapes the tempting port writes + // a zero at both sites - silently discarding a recorded probe success + // and forcing a full fresh run after every window expiry. Written + // before the enum landed so it pins the behavior across that change; + // see the `State` docs for the full disposition table. let config = CircuitBreakerConfig::new(1, 2, Duration::from_secs(30)); let cb = CircuitBreaker::new(config); From 6f94bb478f73d29275a9f905739f82d4b861d501 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Fri, 31 Jul 2026 23:45:06 -0700 Subject: [PATCH 08/16] refactor(circuit-breaker): Admission types, permits bound at call sites allow_request() -> bool becomes admit() -> Result. Ungated carries no permit by construction. The Closed path consumes no probe token, so there is nothing it could later hand back -- which is what makes a phantom release unrepresentable rather than merely guarded against. Only the two paths that actually take a token mint a ProbePermit. Rejection rather than Err(CircuitState): Closed never rejects, so an Err(Closed) would be representable and meaningless, and a label projection would need a bogus arm for it. It also deletes a real defect. run_resilient used to re-read state() after a rejection to name it in the error, and the comment there conceded the reported state "is not necessarily the one that rejected the request". Rejection is captured under the guard that made the decision, so that second acquisition and its staleness caveat both go. Rejection::metric_label is deliberately separate from CircuitState's Display. Display renders "half-open" as user-facing prose; the exported Prometheus label is "half_open", and routing one through the other would silently rename a label value and break existing queries. That also retires the hand-passed &'static str the rejection path used to thread through. Drop is an inert stub here. Landing the release separately is the point: with Drop doing nothing, every existing assertion still holds, so ~30 call-site rewrites can be reviewed as mechanical rather than as behavior, and the next commit's behavioral delta is a handful of lines. consume() lands now rather than with Drop, because unused_variables fires on Drop-typed bindings and every call site must therefore already dispose of its permit explicitly. The permit is minted AFTER the guard is released, via a local Decision enum. That is structural deadlock safety rather than a convention: once Drop takes the same non-reentrant mutex, a permit existing while a guard is live would hang a worker thread, and deferring the mint makes that unrepresentable even if someone later adds a ? mid-function. Four tests bind their permits to named locals, with a comment at each saying why: their assertions are that a window stays exhausted, which inverts the moment a temporary hands its token straight back. The other admissions are binding-insensitive and left as they are. The permit docs state plainly that this rule is review-enforced, not compiler-enforced -- #[must_use] on Admission does not survive .is_ok(). CircuitBreaker::state() joins the test-only set; the rejection message was its last production caller. 184 lib tests pass; fmt, clippy --all-targets -D warnings and rustdoc -D warnings all clean. --- src/iggy_client/circuit_breaker.rs | 277 +++++++++++++++++++++-------- src/iggy_client/resilience.rs | 48 +++-- 2 files changed, 236 insertions(+), 89 deletions(-) diff --git a/src/iggy_client/circuit_breaker.rs b/src/iggy_client/circuit_breaker.rs index f1bb318..1e9c779 100644 --- a/src/iggy_client/circuit_breaker.rs +++ b/src/iggy_client/circuit_breaker.rs @@ -46,7 +46,7 @@ //! rejected so a recovering server never receives a thundering herd of //! probes. Tokens re-grant after `open_duration` elapses in half-open, //! guaranteeing the breaker cannot wedge if a probe's outcome is never -//! recorded. See [`CircuitBreaker::allow_request`]. +//! recorded. See [`CircuitBreaker::admit`]. //! //! One consumed token can cover up to two server operations: the //! post-reconnect retry in `resilience::run_resilient` deliberately does @@ -59,7 +59,7 @@ //! //! // Check if request should be allowed. The gate is synchronous; only the //! // guarded operation itself awaits. -//! if !cb.allow_request() { +//! if cb.admit().is_err() { //! return Err(AppError::CircuitOpen); //! } //! @@ -82,6 +82,7 @@ //! } //! ``` +use std::marker::PhantomData; use std::sync::atomic::{AtomicU32, AtomicU64, Ordering}; use std::sync::{Mutex, MutexGuard}; use std::time::Duration; @@ -224,6 +225,115 @@ impl From<&State> for CircuitState { } } +/// Why the gate turned a request away. +/// +/// Captured under the same guard that made the decision, so — unlike re-reading +/// `state()` afterwards — it always names the state that actually rejected, +/// even under a concurrent transition. +/// +/// Deliberately has no `Closed` variant: Closed never rejects, so an +/// `Err(Closed)` would be representable and meaningless. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum Rejection { + /// The circuit is open and its open window has not yet elapsed. + Open, + /// Half-open, but this window's probe budget is already spent. + ProbeBudgetExhausted, +} + +impl Rejection { + /// Value for the `state` label on the rejection counter. + /// + /// Deliberately not [`CircuitState`]'s `Display`, which renders + /// `"half-open"` as user-facing prose. The metric vocabulary is + /// `"half_open"`, and routing the label through `Display` would silently + /// rename an exported label value and break existing queries. + fn metric_label(self) -> &'static str { + match self { + Rejection::Open => "open", + Rejection::ProbeBudgetExhausted => "half_open", + } + } +} + +impl From for CircuitState { + fn from(rejection: Rejection) -> Self { + match rejection { + Rejection::Open => CircuitState::Open, + Rejection::ProbeBudgetExhausted => CircuitState::HalfOpen, + } + } +} + +impl std::fmt::Display for Rejection { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + CircuitState::from(*self).fmt(f) + } +} + +/// Ownership of one half-open probe token. +/// +/// Minted only where a token is actually taken, so the permit's *existence* is +/// the proof that this request holds one. That is what makes a phantom release +/// — handing back a token the request never took — unrepresentable rather than +/// merely guarded against. +/// +/// In this commit the permit is a marker: it makes ownership explicit at every +/// call site and forces each to bind it, but dropping one does nothing and the +/// explicit `release_probe()` calls remain. The next commit moves the release +/// into `Drop`, at which point these bindings become load-bearing. +/// +/// The lifetime borrows the breaker, so a permit cannot outlive it or be +/// smuggled into a detached task. Deliberately not `Clone` — a cloned permit +/// would release twice. +/// +/// One caveat worth stating rather than implying: binding a permit to a named +/// local is a review rule, not a compiler-enforced one. `#[must_use]` on +/// [`Admission`] catches a discarded admission, but `admit().is_ok()` and +/// `let _ = admit()` both silence it while dropping the token immediately — +/// `clippy::let_underscore_must_use` would catch the latter and is a pedantic +/// lint this crate does not enable. Tests whose assertion depends on a token +/// staying held say so at the binding. +pub(crate) struct ProbePermit<'a> { + breaker: PhantomData<&'a CircuitBreaker>, +} + +impl ProbePermit<'_> { + fn new() -> Self { + Self { + breaker: PhantomData, + } + } + + /// The outcome was recorded, so the token must NOT be handed back. + /// + /// Consumes by value, so use-after-consume is a compile error rather than a + /// silent no-op. The disarm is a move with no lock involved, which is what + /// makes it safe to call from anywhere — including, once `Drop` is wired, + /// from a path where a state guard might still be live. + pub(crate) fn consume(self) { + let _ = std::mem::ManuallyDrop::new(self); + } +} + +impl Drop for ProbePermit<'_> { + /// Inert in this commit. Landing the release separately is deliberate: + /// with `Drop` doing nothing, every existing assertion still holds, so the + /// call-site rewrites here can be reviewed as mechanical rather than as + /// behavior. + fn drop(&mut self) {} +} + +/// Outcome of the circuit-breaker gate for one request. +#[must_use = "an admission decides whether the operation may run"] +pub(crate) enum Admission<'a> { + /// Closed: the request passes and consumes no probe token, so there is + /// nothing it could later hand back. + Ungated, + /// HalfOpen: the request holds one of the window's probe tokens. + Probe(ProbePermit<'a>), +} + /// What a completed state transition should report, emitted once the state /// guard has been released. /// @@ -241,12 +351,8 @@ enum Effect { /// Open -> HalfOpen. `probes` is the budget as granted, captured before the /// transitioning caller takes its own token. EnteredHalfOpen { probes: u32 }, - /// A request was turned away. The label separates "circuit is open" from - /// "half-open probe budget exhausted" — materially different situations. - Rejected { - label: &'static str, - budget_exhausted: bool, - }, + /// A request was turned away. + Rejected(Rejection), /// A probe window elapsed with no recorded outcome and was re-granted. RegrantedProbes, /// A probe token was handed back because its outcome was never recorded. @@ -350,15 +456,12 @@ impl CircuitBreaker { "Circuit breaker transitioning from Open to HalfOpen" ); } - Effect::Rejected { - label, - budget_exhausted, - } => { - if budget_exhausted { + Effect::Rejected(rejection) => { + if rejection == Rejection::ProbeBudgetExhausted { debug!("Circuit breaker rejected request: half-open probe budget exhausted"); } self.requests_rejected.fetch_add(1, Ordering::Relaxed); - crate::metrics::record_circuit_breaker_rejection(label); + crate::metrics::record_circuit_breaker_rejection(rejection.metric_label()); } Effect::RegrantedProbes => { // info: a full probe window elapsed without a recorded outcome @@ -405,9 +508,11 @@ impl CircuitBreaker { } } - /// Check if a request should be allowed through the circuit breaker. + /// Ask the breaker to admit one request. /// - /// Returns `true` if the request can proceed, `false` if it should be rejected. + /// `Ok` carries an [`Admission`] describing what the caller now holds: + /// nothing (Closed) or a [`ProbePermit`] (HalfOpen). `Err` carries the + /// [`Rejection`] reason, captured under the guard that made the decision. /// /// # State Transitions /// @@ -428,20 +533,29 @@ impl CircuitBreaker { /// (e.g. the operation failed with a non-connection error, which by /// design touches neither breaker counter) would otherwise leave the /// breaker half-open with zero tokens forever. - pub fn allow_request(&self) -> bool { - // One exclusive acquisition covers every case. The former read-lock - // fast path existed to avoid an async write lock on the common Closed - // path; a synchronous mutex makes it unnecessary, and dropping it also - // removes the read->write upgrade race that forced the state to be - // re-matched after the second acquisition. - // Single-tail: every arm yields (allowed, effect) rather than returning - // early, so no path can skip the emit below. - let (allowed, effect) = { + pub(crate) fn admit(&self) -> Result, Rejection> { + /// Decided under the guard; the permit is minted only afterwards. + /// + /// Keeping construction outside the critical section is structural + /// deadlock safety: once `Drop` releases a token it must take the same + /// non-reentrant mutex, so no permit may exist while a guard is live. + /// Deferring the mint makes that impossible rather than merely + /// discouraged, and a `?` added mid-function later cannot reintroduce it. + enum Decision { + Ungated, + Probe, + Rejected(Rejection), + } + + // One exclusive acquisition covers every case, and every arm yields + // (decision, effect) rather than returning early, so no path can skip + // the emit below. + let (decision, effect) = { let mut guard = self.lock(); let budget = self.probe_budget(); match &mut *guard { - State::Closed { .. } => (true, Effect::None), + State::Closed { .. } => (Decision::Ungated, Effect::None), State::Open { opened_at } => { if opened_at.elapsed() >= self.config.open_duration { // A NEW recovery attempt: the success count starts at @@ -456,14 +570,11 @@ impl CircuitBreaker { consecutive_successes: 0, }; self.set_gauge(CircuitState::HalfOpen); - (true, Effect::EnteredHalfOpen { probes: budget }) + (Decision::Probe, Effect::EnteredHalfOpen { probes: budget }) } else { ( - false, - Effect::Rejected { - label: "open", - budget_exhausted: false, - }, + Decision::Rejected(Rejection::Open), + Effect::Rejected(Rejection::Open), ) } } @@ -474,7 +585,7 @@ impl CircuitBreaker { } => { if *probes_remaining > 0 { *probes_remaining -= 1; - (true, Effect::None) + (Decision::Probe, Effect::None) } else if granted_at.elapsed() >= self.config.open_duration { // Re-grant after open_duration so leaked probes cannot // wedge the breaker in HalfOpen (see doc above). This @@ -482,14 +593,11 @@ impl CircuitBreaker { // consecutive_successes is deliberately untouched. *probes_remaining = budget - 1; *granted_at = Instant::now(); - (true, Effect::RegrantedProbes) + (Decision::Probe, Effect::RegrantedProbes) } else { ( - false, - Effect::Rejected { - label: "half_open", - budget_exhausted: true, - }, + Decision::Rejected(Rejection::ProbeBudgetExhausted), + Effect::Rejected(Rejection::ProbeBudgetExhausted), ) } } @@ -497,7 +605,11 @@ impl CircuitBreaker { }; self.emit(effect); - allowed + match decision { + Decision::Ungated => Ok(Admission::Ungated), + Decision::Probe => Ok(Admission::Probe(ProbePermit::new())), + Decision::Rejected(rejection) => Err(rejection), + } } /// Half-open probe budget: `success_threshold`, floored at one so a @@ -509,7 +621,7 @@ impl CircuitBreaker { /// Test-only: enter HalfOpen with a full probe budget, spending nothing. /// - /// The production path reaches HalfOpen only through [`Self::allow_request`], + /// The production path reaches HalfOpen only through [`Self::admit`], /// which consumes the transitioning caller's token on the way in. A race /// over the budget therefore cannot be staged through it — the setup call /// would take the very token under contention. Granting the window directly @@ -635,6 +747,12 @@ impl CircuitBreaker { } /// Get the current circuit state. + /// + /// Test-only. Production code used to call this immediately after a + /// rejection to name the state in the error message; [`Rejection`] now + /// carries that, captured under the guard that rejected, so the last + /// non-test caller is gone along with its second lock acquisition. + #[cfg(test)] pub fn state(&self) -> CircuitState { CircuitState::from(&*self.lock()) } @@ -731,7 +849,7 @@ mod tests { async fn test_circuit_breaker_starts_closed() { let cb = CircuitBreaker::default(); assert_eq!(cb.state(), CircuitState::Closed); - assert!(cb.allow_request()); + assert!(cb.admit().is_ok()); } #[tokio::test] @@ -759,7 +877,7 @@ mod tests { assert_eq!(cb.state(), CircuitState::Open); // Requests should be rejected - assert!(!cb.allow_request()); + assert!(cb.admit().is_err()); assert_eq!(cb.requests_rejected(), 1); } @@ -775,7 +893,7 @@ mod tests { tokio::time::advance(Duration::from_millis(20)).await; // Should allow request and transition to half-open - assert!(cb.allow_request()); + assert!(cb.admit().is_ok()); assert_eq!(cb.state(), CircuitState::HalfOpen); } @@ -789,7 +907,7 @@ mod tests { tokio::time::advance(Duration::from_millis(20)).await; // Transition to half-open - assert!(cb.allow_request()); + assert!(cb.admit().is_ok()); assert_eq!(cb.state(), CircuitState::HalfOpen); // Record successes @@ -810,7 +928,7 @@ mod tests { tokio::time::advance(Duration::from_millis(20)).await; // Transition to half-open - assert!(cb.allow_request()); + assert!(cb.admit().is_ok()); assert_eq!(cb.state(), CircuitState::HalfOpen); // Failure should reopen @@ -850,7 +968,7 @@ mod tests { cb.force_close(); assert_eq!(cb.state(), CircuitState::Closed); - assert!(cb.allow_request()); + assert!(cb.admit().is_ok()); } #[tokio::test] @@ -860,7 +978,7 @@ mod tests { cb.force_open(); assert_eq!(cb.state(), CircuitState::Open); - assert!(!cb.allow_request()); + assert!(cb.admit().is_err()); } // ========================================================================= @@ -878,12 +996,16 @@ mod tests { // success_threshold = 2 probe tokens: two callers pass, the third // is rejected instead of piling onto the recovering server. - assert!(cb.allow_request()); - assert!(cb.allow_request()); + // + // The permits are bound to named locals deliberately. Once Drop + // releases the token, letting these fall as temporaries would hand + // both tokens straight back and invert the rejection asserted below. + let _probe_1 = cb.admit().expect("first probe admitted"); + let _probe_2 = cb.admit().expect("second probe admitted"); assert_eq!(cb.state(), CircuitState::HalfOpen); let rejected_before = cb.requests_rejected(); - assert!(!cb.allow_request()); + assert!(cb.admit().is_err()); assert_eq!(cb.requests_rejected(), rejected_before + 1); } @@ -897,18 +1019,20 @@ mod tests { // Single token consumed by the transitioning caller; its outcome is // never recorded (the leaked-probe case) so the breaker sits in - // HalfOpen with zero tokens. - assert!(cb.allow_request()); - assert!(!cb.allow_request()); + // HalfOpen with zero tokens. Held in a named local: a temporary would + // return the token once Drop releases, and the whole point of this + // test is a window that stays exhausted. + let _leaked_probe = cb.admit().expect("transitioning caller admitted"); + assert!(cb.admit().is_err()); // Just below the window boundary the budget must stay exhausted - // an unconditional re-grant would defeat the probe cap entirely. tokio::time::advance(Duration::from_secs(29)).await; - assert!(!cb.allow_request()); + assert!(cb.admit().is_err()); // The re-grant window keeps the breaker from wedging permanently. tokio::time::advance(Duration::from_secs(1)).await; - assert!(cb.allow_request()); + assert!(cb.admit().is_ok()); assert_eq!(cb.state(), CircuitState::HalfOpen); } @@ -929,21 +1053,21 @@ mod tests { tokio::time::advance(Duration::from_secs(30)).await; // Entry consumes one of the two tokens; record a success against it. - assert!(cb.allow_request()); + assert!(cb.admit().is_ok()); cb.record_success(); assert_eq!(cb.state(), CircuitState::HalfOpen); // Spend the second token, then exhaust the budget. Without this the // re-grant branch is never reached and the rest passes vacuously. - assert!(cb.allow_request()); + assert!(cb.admit().is_ok()); assert!( - !cb.allow_request(), + cb.admit().is_err(), "budget must be exhausted for the re-grant branch to be exercised" ); // Window expiry re-grants; the success recorded above must survive. tokio::time::advance(Duration::from_secs(30)).await; - assert!(cb.allow_request()); + assert!(cb.admit().is_ok()); // This second success reaches success_threshold only if the first one // survived the re-grant - a resetting re-grant leaves it HalfOpen. @@ -964,13 +1088,13 @@ mod tests { tokio::time::advance(Duration::from_secs(30)).await; // Consume the only token, then fail the probe: back to Open. - assert!(cb.allow_request()); + assert!(cb.admit().is_ok()); cb.record_failure(); assert_eq!(cb.state(), CircuitState::Open); // Next half-open entry starts with a fresh token budget. tokio::time::advance(Duration::from_secs(30)).await; - assert!(cb.allow_request()); + assert!(cb.admit().is_ok()); assert_eq!(cb.state(), CircuitState::HalfOpen); } @@ -984,13 +1108,13 @@ mod tests { cb.record_failure(); tokio::time::advance(Duration::from_secs(30)).await; - assert!(cb.allow_request()); + assert!(cb.admit().is_ok()); cb.record_success(); - assert!(cb.allow_request()); + assert!(cb.admit().is_ok()); cb.record_success(); assert_eq!(cb.state(), CircuitState::Closed); - assert!(cb.allow_request()); + assert!(cb.admit().is_ok()); } #[tokio::test(start_paused = true)] @@ -1003,16 +1127,16 @@ mod tests { cb.record_failure(); tokio::time::advance(Duration::from_secs(30)).await; - assert!(cb.allow_request()); + assert!(cb.admit().is_ok()); } #[test] fn test_half_open_concurrent_probes_admit_exactly_the_budget() { - // Two OS threads race allow_request inside HalfOpen over a one-token + // Two OS threads race admit() inside HalfOpen over a one-token // budget: exactly one may pass. // // This deliberately no longer uses tokio::join!. A synchronous - // allow_request is not a future, and the obvious sequential rewrite + // admit() is not a future, and the obvious sequential rewrite // would still pass with the cap removed entirely - it takes two // genuinely concurrent callers to test a cap. A Barrier releases both // threads at once and the loop amplifies a narrow window; a fresh @@ -1027,11 +1151,11 @@ mod tests { let (a, b) = std::thread::scope(|s| { let first = s.spawn(|| { gate.wait(); - cb.allow_request() + cb.admit().is_ok() }); let second = s.spawn(|| { gate.wait(); - cb.allow_request() + cb.admit().is_ok() }); (first.join().unwrap(), second.join().unwrap()) }); @@ -1051,25 +1175,26 @@ mod tests { // No-op while Closed. cb.release_probe(); - assert!(cb.allow_request()); + assert!(cb.admit().is_ok()); cb.record_failure(); tokio::time::advance(Duration::from_secs(30)).await; // Consume the only token, release it, and it must admit again. - assert!(cb.allow_request()); - assert!(!cb.allow_request()); + let held = cb.admit().expect("only token admitted"); + assert!(cb.admit().is_err()); cb.release_probe(); - assert!(cb.allow_request()); + let held_2 = cb.admit().expect("released token re-admits"); // Releases never exceed the granted budget (single token here). cb.release_probe(); cb.release_probe(); - assert!(cb.allow_request()); + let held_3 = cb.admit().expect("capped release admits exactly once more"); assert!( - !cb.allow_request(), + cb.admit().is_err(), "budget cap must hold after over-release" ); + drop((held, held_2, held_3)); } #[tokio::test] diff --git a/src/iggy_client/resilience.rs b/src/iggy_client/resilience.rs index 96b16ec..3dbecf5 100644 --- a/src/iggy_client/resilience.rs +++ b/src/iggy_client/resilience.rs @@ -74,7 +74,7 @@ use std::time::Duration; use tracing::{debug, warn}; -use super::circuit_breaker::CircuitBreaker; +use super::circuit_breaker::{Admission, CircuitBreaker, ProbePermit}; use crate::error::{AppError, AppResult}; /// Check if an error is a connection-related error that warrants reconnection. @@ -88,6 +88,18 @@ pub(super) fn is_connection_error(error: &AppError) -> bool { ) } +/// Give up an owned probe token because the outcome WAS recorded. +/// +/// A no-op for `None` (the request was admitted while Closed and holds no +/// token). Consuming rather than dropping is what keeps a recorded outcome +/// from also handing the token back — once `Drop` releases, doing both would +/// mint a token from nothing. +fn consume(permit: Option>) { + if let Some(permit) = permit { + permit.consume(); + } +} + /// Execute `operation` under the full resilience composition. /// /// Generic over its collaborators so the composition is testable without a @@ -116,26 +128,33 @@ where R: FnOnce() -> RFut, RFut: Future>, { - // Check circuit breaker before attempting operation - if !breaker.allow_request() { - // The state is re-read after the rejection, so under a concurrent - // transition it reports the CURRENT state, not necessarily the one - // that rejected the request. - let state = breaker.state(); - return Err(AppError::CircuitOpen(format!( - "Circuit breaker rejected the request (current state: {}) - service temporarily unavailable", - state - ))); - } + // Check circuit breaker before attempting operation. A half-open + // admission hands back a permit representing the probe token this request + // now owns; Closed admissions consume no token and so carry none. + let permit = match breaker.admit() { + Ok(Admission::Ungated) => None, + Ok(Admission::Probe(permit)) => Some(permit), + Err(rejection) => { + // `rejection` was captured under the guard that made the decision, + // so it names the state that actually rejected - no second lock + // acquisition, and no chance of reporting a state a concurrent + // transition has already moved past. + return Err(AppError::CircuitOpen(format!( + "Circuit breaker rejected the request (state: {rejection}) - service temporarily unavailable" + ))); + } + }; // First attempt with timeout match tokio::time::timeout(timeout, operation()).await { Ok(Ok(value)) => { breaker.record_success(); + consume(permit); Ok(value) } Ok(Err(e)) if is_connection_error(&e) => { breaker.record_failure(); + consume(permit); warn!(error = %e, "Operation failed due to connection error, attempting reconnect"); reconnect().await?; retry_once(breaker, timeout, timeout_is_outage_signal, &operation).await @@ -145,6 +164,7 @@ where // but hand back any half-open probe token this request consumed // so an unrecorded outcome cannot starve recovery. breaker.release_probe(); + drop(permit); Err(e) } Err(_) => { @@ -156,12 +176,14 @@ where // evidence; hand back any consumed probe token instead. if timeout_is_outage_signal { breaker.record_failure(); + consume(permit); warn!( timeout = ?timeout, "Operation timed out at the global deadline (recorded as circuit-breaker failure)" ); } else { breaker.release_probe(); + drop(permit); debug!( timeout = ?timeout, "Operation timed out at a client-scoped deadline (not a breaker failure)" @@ -667,7 +689,7 @@ mod tests { // Without release_probe the single token would be gone and this // would be rejected until the re-grant window. assert!( - breaker.allow_request(), + breaker.admit().is_ok(), "released token must admit the next probe" ); } From 412e67558f88513ea0f137e186ae70c2a835ba5a Mon Sep 17 00:00:00 2001 From: mlevkov Date: Fri, 31 Jul 2026 23:55:12 -0700 Subject: [PATCH 09/16] feat(circuit-breaker): Drop releases the half-open probe token The behavioral delta of TD-2026-07-09, and the only one in this session. A ProbePermit now owns a real reference and the id of the window it was minted in. Dropping it returns the token; consume() gives it up without returning, for outcomes that were recorded. A request can no longer both record an outcome and refund its token, nor release the same token twice -- both of which resilience.rs previously documented as accepted slack. More to the point, a request future dropped mid-probe, which is a client disconnect during an outage, now returns the token it was holding instead of stranding it until the re-grant window. The window id closes the third leak. release_probe now compares the permit's generation against the live one and discards a token whose window has already been replaced, rather than crediting it to the current budget. The id lives in the guarded struct beside the state, not as an atomic on the breaker: every access is already under the guard, and an atomic would advertise lock-free access that is not safe to use that way. It cannot live inside State::HalfOpen either -- passing through Open would leave the next entry with nothing to read, and any restart lets a straggler match a recycled id. grant_window is the sole place a window is granted, so the bump and the grant cannot come apart. release_probe is private and reachable only from Drop. Observability. The counter is a disposition counter, consumed | released | stale, not an abandoned-only one: a counter that can only increment on the failure path reads identically whether the system is healthy or the release path is dead code, whereas released staying flat while consumed climbs is a visible signature. Routine releases log at debug, since every non-connection error in half-open lands there; the stale discard logs at warn, because it means a probe outran its window. The re-grant info was NOT reworded into an alarm: it stays reachable for a legitimate reason -- probes still in flight past open_duration, the normal half-open case during an outage -- so calling it an invariant violation would cry wolf on every real outage. It now reports how many were outstanding. Two tests failed on the first run, which is the M5/M6 split working as intended. Both admitted a probe as a temporary and then asserted the budget was exhausted; with Drop inert in M5 that passed, and with Drop live it does not, because the token goes straight back. The concurrency test was the sharper of the two -- testing is_ok() inside the racing threads dropped each permit there and let both racers through. Both now carry their admissions out to the assertion. The direct test of release_probe is replaced by two that drive the same property through the public path: dropping a permit returns its token, and consuming one does not. Docs corrected where this commit falsified them: the resilience module's double-release paragraph, its non-connection-error semantics, the breaker's probe-limiting prose and usage example, and both sites of the two-site invariant TD-2026-07-03 recorded, including a superseded-in-part note on that record. 185 lib tests pass; fmt, clippy --all-targets -D warnings and rustdoc -D warnings all clean. --- docs/tech-debt/TD-2026-07-03.md | 6 + src/iggy_client/circuit_breaker.rs | 368 +++++++++++++++++++++-------- src/iggy_client/mod.rs | 5 +- src/iggy_client/resilience.rs | 41 ++-- src/metrics.rs | 20 ++ 5 files changed, 315 insertions(+), 125 deletions(-) diff --git a/docs/tech-debt/TD-2026-07-03.md b/docs/tech-debt/TD-2026-07-03.md index 925b89c..9fd49f5 100644 --- a/docs/tech-debt/TD-2026-07-03.md +++ b/docs/tech-debt/TD-2026-07-03.md @@ -20,6 +20,12 @@ breaker configuration is next extended. ## Resolution (session 02) +> **Superseded in part by TD-2026-07-09 (session 03):** `allow_request()` is +> now `admit()`, returning a `ProbePermit` that owns the token, and the +> anti-wedge re-grant below is no longer the only thing that recovers a leaked +> probe — a permit returns its token on drop. The budget cap itself is +> unchanged. + Implemented token-limited probing: entering HalfOpen grants `success_threshold` probe tokens (minimum one), each `allow_request()` consumes one, and exhausted budgets reject with a breaker rejection. diff --git a/src/iggy_client/circuit_breaker.rs b/src/iggy_client/circuit_breaker.rs index 1e9c779..c034714 100644 --- a/src/iggy_client/circuit_breaker.rs +++ b/src/iggy_client/circuit_breaker.rs @@ -42,11 +42,16 @@ //! # Half-Open Probe Limiting //! //! Entering half-open grants `success_threshold` probe tokens (minimum one); -//! each allowed request consumes one, and requests beyond the budget are -//! rejected so a recovering server never receives a thundering herd of -//! probes. Tokens re-grant after `open_duration` elapses in half-open, -//! guaranteeing the breaker cannot wedge if a probe's outcome is never -//! recorded. See [`CircuitBreaker::admit`]. +//! each admitted request receives a `ProbePermit` owning one, and requests +//! beyond the budget are rejected so a recovering server never receives a +//! thundering herd of probes. See [`CircuitBreaker::admit`]. +//! +//! A permit returns its token unless the outcome was recorded, so a probe that +//! ends without feeding the breaker — an unrecorded error, or a request future +//! dropped mid-flight — no longer strands its token. Windows still re-grant +//! after `open_duration`, which now covers only genuinely long-running probes +//! rather than leaked ones; a token from an expired window is discarded on +//! release rather than credited to the live one. //! //! One consumed token can cover up to two server operations: the //! post-reconnect retry in `resilience::run_resilient` deliberately does @@ -57,11 +62,12 @@ //! ```rust,ignore //! let cb = CircuitBreaker::new(CircuitBreakerConfig::default()); //! -//! // Check if request should be allowed. The gate is synchronous; only the -//! // guarded operation itself awaits. -//! if cb.admit().is_err() { -//! return Err(AppError::CircuitOpen); -//! } +//! // The gate is synchronous; only the guarded operation itself awaits. +//! // A half-open admission carries a permit owning one probe token. +//! let _permit = match cb.admit() { +//! Ok(admission) => admission, +//! Err(rejection) => return Err(AppError::CircuitOpen(rejection.to_string())), +//! }; //! //! // Execute the operation. Only CONNECTION-CLASS outcomes feed the //! // breaker (see `resilience::run_resilient` for the real composition): @@ -74,15 +80,11 @@ //! cb.record_failure(); //! Err(e) //! } -//! // Other errors record neither; release any half-open probe token. -//! Err(e) => { -//! cb.release_probe(); -//! Err(e) -//! } +//! // Other errors record neither. Dropping the permit returns the token. +//! Err(e) => Err(e), //! } //! ``` -use std::marker::PhantomData; use std::sync::atomic::{AtomicU32, AtomicU64, Ordering}; use std::sync::{Mutex, MutexGuard}; use std::time::Duration; @@ -225,6 +227,76 @@ impl From<&State> for CircuitState { } } +/// Everything the state mutex protects. +/// +/// `probe_generation` lives here rather than as an atomic on +/// [`CircuitBreaker`] deliberately: every access already happens under this +/// guard, and an atomic field would advertise lock-free access that is not +/// actually safe to use that way. Keeping it beside `state` also makes +/// "bumped on every grant" checkable in one place. +struct Guarded { + state: State, + /// Monotonic id of the current half-open probe window. + /// + /// Bumped on EVERY grant — both entering HalfOpen and re-granting within + /// it — so a token minted in one window is distinguishable from the window + /// it is dropped into. It cannot live inside `State::HalfOpen`: passing + /// through Open would leave the next entry with nothing to read, and any + /// restart lets a straggler match a recycled id. + /// + /// Never reset. `force_close`/`force_open` deliberately leave it alone for + /// the same reason. + probe_generation: u64, +} + +/// Open a fresh half-open probe window and return its id. +/// +/// The single place a window is granted, so the generation cannot advance +/// without a grant, nor a grant happen without advancing it — the pairing is +/// structural rather than a convention repeated at three call sites. +/// +/// Takes the two guarded fields separately rather than `&mut Guarded` because +/// callers hold them through a split borrow: the state must stay mutably +/// borrowed by the match arm that decided to grant. +/// +/// `consecutive_successes` is the caller's decision, and is exactly what +/// separates entering from re-granting — see the [`State`] docs. +fn grant_window( + state: &mut State, + probe_generation: &mut u64, + probes_remaining: u32, + consecutive_successes: u32, +) -> u64 { + *probe_generation += 1; + *state = State::HalfOpen { + probes_remaining, + granted_at: Instant::now(), + consecutive_successes, + }; + *probe_generation +} + +/// How a half-open probe token ended. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Disposition { + /// The outcome was recorded, so the token was not handed back. + Consumed, + /// The outcome was never recorded; the token returned to its own window. + Released, + /// Dropped into a different window than it was minted in, and discarded. + Stale, +} + +impl Disposition { + fn label(self) -> &'static str { + match self { + Disposition::Consumed => "consumed", + Disposition::Released => "released", + Disposition::Stale => "stale", + } + } +} + /// Why the gate turned a request away. /// /// Captured under the same guard that made the decision, so — unlike re-reading @@ -278,10 +350,12 @@ impl std::fmt::Display for Rejection { /// — handing back a token the request never took — unrepresentable rather than /// merely guarded against. /// -/// In this commit the permit is a marker: it makes ownership explicit at every -/// call site and forces each to bind it, but dropping one does nothing and the -/// explicit `release_probe()` calls remain. The next commit moves the release -/// into `Drop`, at which point these bindings become load-bearing. +/// Dropping a permit hands its token back; [`ProbePermit::consume`] gives it up +/// without releasing, for outcomes that WERE recorded. That is the whole +/// mechanism: a request cannot both record an outcome and return its token, +/// and a request future dropped mid-probe — a client disconnect during an +/// outage — returns the token it was holding instead of leaking it until the +/// re-grant window. /// /// The lifetime borrows the breaker, so a permit cannot outlive it or be /// smuggled into a detached task. Deliberately not `Clone` — a cloned permit @@ -295,13 +369,16 @@ impl std::fmt::Display for Rejection { /// lint this crate does not enable. Tests whose assertion depends on a token /// staying held say so at the binding. pub(crate) struct ProbePermit<'a> { - breaker: PhantomData<&'a CircuitBreaker>, + breaker: &'a CircuitBreaker, + /// The probe window this token was taken from. + generation: u64, } -impl ProbePermit<'_> { - fn new() -> Self { +impl<'a> ProbePermit<'a> { + fn new(breaker: &'a CircuitBreaker, generation: u64) -> Self { Self { - breaker: PhantomData, + breaker, + generation, } } @@ -312,16 +389,18 @@ impl ProbePermit<'_> { /// makes it safe to call from anywhere — including, once `Drop` is wired, /// from a path where a state guard might still be live. pub(crate) fn consume(self) { + self.breaker.record_disposition(Disposition::Consumed); + // Suppress the release without running Drop. ManuallyDrop rather than + // mem::forget only for clarity - the permit owns a shared reference + // and a u64, so neither leaks anything. let _ = std::mem::ManuallyDrop::new(self); } } impl Drop for ProbePermit<'_> { - /// Inert in this commit. Landing the release separately is deliberate: - /// with `Drop` doing nothing, every existing assertion still holds, so the - /// call-site rewrites here can be reviewed as mechanical rather than as - /// behavior. - fn drop(&mut self) {} + fn drop(&mut self) { + self.breaker.release_probe(self.generation); + } } /// Outcome of the circuit-breaker gate for one request. @@ -353,8 +432,11 @@ enum Effect { EnteredHalfOpen { probes: u32 }, /// A request was turned away. Rejected(Rejection), - /// A probe window elapsed with no recorded outcome and was re-granted. - RegrantedProbes, + /// A probe window elapsed with probes still unaccounted for, and was + /// re-granted so recovery cannot wedge. + RegrantedProbes { outstanding: u32 }, + /// A token was dropped into a different window than it was minted in. + StaleProbeDiscarded { minted: u64, current: u64 }, /// A probe token was handed back because its outcome was never recorded. ReleasedProbe, /// A success landed in HalfOpen; `closed` if it reached the threshold. @@ -380,7 +462,7 @@ pub struct CircuitBreaker { /// Configuration parameters. config: CircuitBreakerConfig, /// Internal state protected by a synchronous mutex. - state: Mutex, + state: Mutex, /// Total number of times the circuit has been opened (for metrics). times_opened: AtomicU32, /// Total number of requests rejected due to open circuit (for metrics). @@ -392,7 +474,10 @@ impl CircuitBreaker { pub fn new(config: CircuitBreakerConfig) -> Self { Self { config, - state: Mutex::new(State::initial()), + state: Mutex::new(Guarded { + state: State::initial(), + probe_generation: 0, + }), times_opened: AtomicU32::new(0), requests_rejected: AtomicU64::new(0), } @@ -410,7 +495,7 @@ impl CircuitBreaker { /// The `panicking()` guard is load-bearing: poisoning implies an in-flight /// unwind, and asserting during unwind double-panics straight to abort, /// destroying the very test report the assertion exists to sharpen. - fn lock(&self) -> MutexGuard<'_, State> { + fn lock(&self) -> MutexGuard<'_, Guarded> { self.state.lock().unwrap_or_else(|poisoned| { if !std::thread::panicking() { debug_assert!(false, "circuit breaker state lock poisoned"); @@ -463,14 +548,33 @@ impl CircuitBreaker { self.requests_rejected.fetch_add(1, Ordering::Relaxed); crate::metrics::record_circuit_breaker_rejection(rejection.metric_label()); } - Effect::RegrantedProbes => { - // info: a full probe window elapsed without a recorded outcome - // - recovery is stalling, not progressing. - info!("Circuit breaker re-granted half-open probe tokens"); + Effect::RegrantedProbes { outstanding } => { + // Reachable for a legitimate reason even with RAII release: + // probes still IN FLIGHT past open_duration, which is the + // normal half-open case during an outage. Not an invariant + // violation, so not a warning - the leak alarm is the stale + // discard below. + info!( + outstanding, + "Circuit breaker re-granted half-open probe tokens (previous window still outstanding)" + ); } Effect::ReleasedProbe => { + // Routine: every non-connection error in half-open lands here. + self.record_disposition(Disposition::Released); debug!("Circuit breaker released a half-open probe token (outcome not recorded)"); } + Effect::StaleProbeDiscarded { minted, current } => { + // The token outlived its window. Bounded and self-correcting, + // but it means a probe ran longer than open_duration, so it is + // worth an operator's attention. + self.record_disposition(Disposition::Stale); + warn!( + minted_generation = minted, + current_generation = current, + "Circuit breaker discarded a probe token from an expired window" + ); + } Effect::HalfOpenSuccess { successes, closed } => { debug!( consecutive_successes = successes, @@ -543,7 +647,8 @@ impl CircuitBreaker { /// discouraged, and a `?` added mid-function later cannot reintroduce it. enum Decision { Ungated, - Probe, + /// Admitted holding a token from this probe window. + Probe(u64), Rejected(Rejection), } @@ -553,8 +658,14 @@ impl CircuitBreaker { let (decision, effect) = { let mut guard = self.lock(); let budget = self.probe_budget(); - - match &mut *guard { + // Split the borrow so a grant can bump the window id while the + // match still holds `state` mutably. + let Guarded { + state, + probe_generation, + } = &mut *guard; + + match state { State::Closed { .. } => (Decision::Ungated, Effect::None), State::Open { opened_at } => { if opened_at.elapsed() >= self.config.open_duration { @@ -564,13 +675,17 @@ impl CircuitBreaker { // tokens as it passes, so the window opens at // budget - 1; probe_budget() floors at 1, so this // cannot underflow. - *guard = State::HalfOpen { - probes_remaining: budget - 1, - granted_at: Instant::now(), - consecutive_successes: 0, - }; + // A NEW recovery attempt, so the success count starts + // at zero (contrast the re-grant arm below). The + // transitioning caller takes the first of the granted + // tokens as it passes, so the window opens at + // budget - 1; probe_budget() floors at 1. + let generation = grant_window(state, probe_generation, budget - 1, 0); self.set_gauge(CircuitState::HalfOpen); - (Decision::Probe, Effect::EnteredHalfOpen { probes: budget }) + ( + Decision::Probe(generation), + Effect::EnteredHalfOpen { probes: budget }, + ) } else { ( Decision::Rejected(Rejection::Open), @@ -581,19 +696,25 @@ impl CircuitBreaker { State::HalfOpen { probes_remaining, granted_at, - .. + consecutive_successes, } => { if *probes_remaining > 0 { *probes_remaining -= 1; - (Decision::Probe, Effect::None) + (Decision::Probe(*probe_generation), Effect::None) } else if granted_at.elapsed() >= self.config.open_duration { - // Re-grant after open_duration so leaked probes cannot - // wedge the breaker in HalfOpen (see doc above). This - // is the SAME recovery attempt continuing, so - // consecutive_successes is deliberately untouched. - *probes_remaining = budget - 1; - *granted_at = Instant::now(); - (Decision::Probe, Effect::RegrantedProbes) + // Re-grant after open_duration so outstanding probes + // cannot wedge the breaker in HalfOpen. This is the + // SAME recovery attempt continuing, so the success + // count carries over. + let successes = *consecutive_successes; + let generation = + grant_window(state, probe_generation, budget - 1, successes); + ( + Decision::Probe(generation), + Effect::RegrantedProbes { + outstanding: budget, + }, + ) } else { ( Decision::Rejected(Rejection::ProbeBudgetExhausted), @@ -607,7 +728,7 @@ impl CircuitBreaker { self.emit(effect); match decision { Decision::Ungated => Ok(Admission::Ungated), - Decision::Probe => Ok(Admission::Probe(ProbePermit::new())), + Decision::Probe(generation) => Ok(Admission::Probe(ProbePermit::new(self, generation))), Decision::Rejected(rejection) => Err(rejection), } } @@ -628,12 +749,13 @@ impl CircuitBreaker { /// leaves the budget as the only contended resource. #[cfg(test)] fn force_half_open(&self) { + let budget = self.probe_budget(); let mut guard = self.lock(); - *guard = State::HalfOpen { - probes_remaining: self.probe_budget(), - granted_at: Instant::now(), - consecutive_successes: 0, - }; + let Guarded { + state, + probe_generation, + } = &mut *guard; + grant_window(state, probe_generation, budget, 0); } /// Hand back a half-open probe token whose outcome was deliberately not @@ -643,24 +765,40 @@ impl CircuitBreaker { /// round-trip proving transport health — would permanently consume /// tokens and starve recovery until the re-grant window. No-op outside /// HalfOpen; capped at the granted budget. - pub(super) fn release_probe(&self) { + fn release_probe(&self, generation: u64) { let effect = { let mut guard = self.lock(); let budget = self.probe_budget(); - if let State::HalfOpen { - probes_remaining, .. - } = &mut *guard - && *probes_remaining < budget - { - *probes_remaining += 1; - Effect::ReleasedProbe - } else { - Effect::None + let current = guard.probe_generation; + + match &mut guard.state { + // A token minted in an earlier window must not credit this + // one: the re-grant already replaced the budget it belonged + // to, so crediting it here would inflate the live window. + State::HalfOpen { .. } if current != generation => Effect::StaleProbeDiscarded { + minted: generation, + current, + }, + State::HalfOpen { + probes_remaining, .. + } if *probes_remaining < budget => { + *probes_remaining += 1; + Effect::ReleasedProbe + } + // Already at budget, or the breaker has left HalfOpen entirely + // and the window died with the variant. + _ => Effect::None, } }; self.emit(effect); } + /// Count how a probe token ended. Touches no state and takes no lock, so + /// it is safe from [`ProbePermit::consume`], which may run anywhere. + fn record_disposition(&self, disposition: Disposition) { + crate::metrics::record_circuit_breaker_probe_disposition(disposition.label()); + } + /// Record a successful operation. /// /// In HalfOpen state, consecutive successes can close the circuit. @@ -668,7 +806,7 @@ impl CircuitBreaker { let effect = { let mut guard = self.lock(); - match &mut *guard { + match &mut guard.state { State::Closed { consecutive_failures, } => { @@ -684,7 +822,7 @@ impl CircuitBreaker { let successes = *consecutive_successes; let closed = successes >= self.config.success_threshold; if closed { - *guard = State::Closed { + guard.state = State::Closed { consecutive_failures: 0, }; self.set_gauge(CircuitState::Closed); @@ -713,7 +851,7 @@ impl CircuitBreaker { let effect = { let mut guard = self.lock(); - match &mut *guard { + match &mut guard.state { State::Closed { consecutive_failures, } => { @@ -725,14 +863,14 @@ impl CircuitBreaker { let failures = *consecutive_failures; let opened = failures >= self.config.failure_threshold; if opened { - self.open_now(&mut guard); + self.open_now(&mut guard.state); } Effect::Failure { failures, opened } } State::HalfOpen { .. } => { // Any failure in half-open state reopens the circuit; the // accumulated success count dies with the variant. - self.open_now(&mut guard); + self.open_now(&mut guard.state); Effect::ReopenedFromHalfOpen } State::Open { .. } => { @@ -754,7 +892,7 @@ impl CircuitBreaker { /// non-test caller is gone along with its second lock acquisition. #[cfg(test)] pub fn state(&self) -> CircuitState { - CircuitState::from(&*self.lock()) + CircuitState::from(&self.lock().state) } /// Get the number of times the circuit has been opened. @@ -786,7 +924,7 @@ impl CircuitBreaker { // One assignment replaces six field resets. The old flat struct // needed explicit hygiene so stale half-open values could not // outlive the reset; the enum drops them with the variant. - *guard = State::Closed { + guard.state = State::Closed { consecutive_failures: 0, }; self.set_gauge(CircuitState::Closed); @@ -803,9 +941,9 @@ impl CircuitBreaker { pub fn force_open(&self) { let opened = { let mut guard = self.lock(); - let changed = !matches!(*guard, State::Open { .. }); + let changed = !matches!(guard.state, State::Open { .. }); if changed { - self.open_now(&mut guard); + self.open_now(&mut guard.state); } changed }; @@ -1053,13 +1191,15 @@ mod tests { tokio::time::advance(Duration::from_secs(30)).await; // Entry consumes one of the two tokens; record a success against it. - assert!(cb.admit().is_ok()); + // Permits are held in named locals throughout: dropping one returns + // its token, and this test is entirely about a budget that stays spent. + let _probe_1 = cb.admit().expect("first probe admitted"); cb.record_success(); assert_eq!(cb.state(), CircuitState::HalfOpen); // Spend the second token, then exhaust the budget. Without this the // re-grant branch is never reached and the rest passes vacuously. - assert!(cb.admit().is_ok()); + let _probe_2 = cb.admit().expect("second probe admitted"); assert!( cb.admit().is_err(), "budget must be exhausted for the re-grant branch to be exercised" @@ -1067,7 +1207,7 @@ mod tests { // Window expiry re-grants; the success recorded above must survive. tokio::time::advance(Duration::from_secs(30)).await; - assert!(cb.admit().is_ok()); + let _probe_3 = cb.admit().expect("re-granted window admits"); // This second success reaches success_threshold only if the first one // survived the re-grant - a resetting re-grant leaves it HalfOpen. @@ -1148,53 +1288,73 @@ mod tests { cb.force_half_open(); let gate = std::sync::Barrier::new(2); + // The admissions are carried OUT of the threads and held until the + // assertion. Testing `is_ok()` inside a thread would drop the + // permit there, hand the token straight back, and let both racers + // pass - which is the bug this test exists to catch. let (a, b) = std::thread::scope(|s| { let first = s.spawn(|| { gate.wait(); - cb.admit().is_ok() + cb.admit() }); let second = s.spawn(|| { gate.wait(); - cb.admit().is_ok() + cb.admit() }); (first.join().unwrap(), second.join().unwrap()) }); assert!( - a ^ b, - "iteration {i}: exactly one of two racing probes may pass, got ({a}, {b})" + a.is_ok() ^ b.is_ok(), + "iteration {i}: exactly one of two racing probes may pass, got ({}, {})", + a.is_ok(), + b.is_ok() ); assert_eq!(cb.state(), CircuitState::HalfOpen); } } #[tokio::test(start_paused = true)] - async fn test_release_probe_returns_token_capped_at_budget() { + async fn test_dropping_a_permit_returns_its_token() { + // Replaces the old direct test of release_probe, which is now private + // and reachable only through Drop. Same property, driven through the + // public path: an unrecorded outcome hands its token back. let config = CircuitBreakerConfig::new(1, 1, Duration::from_secs(30)); let cb = CircuitBreaker::new(config); - // No-op while Closed. - cb.release_probe(); - assert!(cb.admit().is_ok()); - cb.record_failure(); tokio::time::advance(Duration::from_secs(30)).await; - // Consume the only token, release it, and it must admit again. - let held = cb.admit().expect("only token admitted"); - assert!(cb.admit().is_err()); - cb.release_probe(); - let held_2 = cb.admit().expect("released token re-admits"); + let probe = cb.admit().expect("only token admitted"); + assert!( + cb.admit().is_err(), + "budget exhausted while the probe is held" + ); - // Releases never exceed the granted budget (single token here). - cb.release_probe(); - cb.release_probe(); - let held_3 = cb.admit().expect("capped release admits exactly once more"); + drop(probe); + let _readmitted = cb.admit().expect("released token re-admits"); assert!( cb.admit().is_err(), - "budget cap must hold after over-release" + "release must not push the budget above its cap" ); - drop((held, held_2, held_3)); + } + + #[tokio::test(start_paused = true)] + async fn test_consuming_a_permit_does_not_return_its_token() { + // The other half of the mechanism: a RECORDED outcome must not also + // hand the token back, or a request would both count and refund. + let config = CircuitBreakerConfig::new(1, 1, Duration::from_secs(30)); + let cb = CircuitBreaker::new(config); + + cb.record_failure(); + tokio::time::advance(Duration::from_secs(30)).await; + + let Admission::Probe(probe) = cb.admit().expect("only token admitted") else { + panic!("half-open admission must carry a permit"); + }; + probe.consume(); + + assert!(cb.admit().is_err(), "a consumed token must stay consumed"); } #[tokio::test] diff --git a/src/iggy_client/mod.rs b/src/iggy_client/mod.rs index 2627475..0e48285 100644 --- a/src/iggy_client/mod.rs +++ b/src/iggy_client/mod.rs @@ -146,8 +146,9 @@ fn backoff_delay_ms(attempt: u32, base_ms: u64, max_ms: u64, jitter_unit: f64) - /// - **Closed** (normal): All requests pass through /// - **Open** (failing): Requests fail fast without attempting the operation /// - **Half-Open** (recovery): Probes limited to `success_threshold` tokens -/// per `open_duration` window; excess requests fail fast (see -/// `circuit_breaker` module docs for the token re-grant rules) +/// per `open_duration` window; excess requests fail fast. Each admitted +/// probe owns its token and returns it unless its outcome was recorded (see +/// `circuit_breaker` module docs for ownership and the re-grant rules) /// /// # Performance Considerations /// diff --git a/src/iggy_client/resilience.rs b/src/iggy_client/resilience.rs index 3dbecf5..8836530 100644 --- a/src/iggy_client/resilience.rs +++ b/src/iggy_client/resilience.rs @@ -14,10 +14,9 @@ //! 3. **Classified connection error** → record breaker failure, run the //! `reconnect` step, then retry the operation exactly once. //! 4. **Non-connection error** → returned as-is; the breaker records -//! neither success nor failure (bad requests must not open the circuit), -//! but any half-open probe token the request consumed is RELEASED so an -//! unrecorded outcome cannot starve recovery (see -//! `CircuitBreaker::release_probe`). +//! neither success nor failure (bad requests must not open the circuit). +//! Any half-open probe token the request consumed is returned when its +//! permit drops, so an unrecorded outcome cannot starve recovery. //! 5. **Timeout** → recorded as a breaker failure only when //! `timeout_is_outage_signal` is true, i.e. the deadline is the global //! operation timeout (the SDK's internal transport reconnection swallows @@ -64,11 +63,19 @@ //! immediately. This is the accepted trade against the one-client-DoS the //! exemption prevents. //! -//! Note also that a single request may release its probe token twice -//! (scoped first-attempt timeout, then an unrecorded retry outcome); the -//! budget cap bounds the effect to transiently admitting one extra -//! concurrent probe — the same order of slack as the documented -//! retry-gate bypass. +//! # Probe token ownership +//! +//! A half-open admission hands back a `ProbePermit`. Recording an outcome +//! CONSUMES it; anything else — an unrecorded error, a client-scoped timeout, +//! or the request future being dropped mid-flight — returns the token when the +//! permit falls out of scope. A request can therefore no longer both record an +//! outcome and refund its token, nor release the same token twice, both of +//! which this module previously documented as bounded slack. +//! +//! The retry path holds no permit: the first attempt has already disposed of +//! it by the time a reconnect leads there. That is deliberate and unchanged — +//! the retry deliberately bypasses the gate (above), so one token still covers +//! up to two server operations. use std::time::Duration; @@ -160,10 +167,10 @@ where retry_once(breaker, timeout, timeout_is_outage_signal, &operation).await } Ok(Err(e)) => { - // Non-connection error - record neither success nor failure, - // but hand back any half-open probe token this request consumed - // so an unrecorded outcome cannot starve recovery. - breaker.release_probe(); + // Non-connection error - record neither success nor failure. + // Dropping the permit hands back any half-open probe token this + // request consumed, so an unrecorded outcome cannot starve + // recovery. drop(permit); Err(e) } @@ -182,7 +189,6 @@ where "Operation timed out at the global deadline (recorded as circuit-breaker failure)" ); } else { - breaker.release_probe(); drop(permit); debug!( timeout = ?timeout, @@ -237,10 +243,9 @@ where if is_connection_error(&e) { breaker.record_failure(); warn!(error = %e, "Retry failed with a connection error (recorded as breaker failure)"); - } else { - // Unrecorded outcome: hand back any consumed probe token. - breaker.release_probe(); } + // No token to hand back: the permit was disposed on the first + // attempt, before the reconnect that led here. Err(e) } Err(_) => { @@ -250,8 +255,6 @@ where timeout = ?timeout, "Retry timed out at the global deadline (recorded as breaker failure)" ); - } else { - breaker.release_probe(); } Err(AppError::OperationTimeout(format!( "Operation timed out after {:?} on retry", diff --git a/src/metrics.rs b/src/metrics.rs index 99bcd59..aee76e4 100644 --- a/src/metrics.rs +++ b/src/metrics.rs @@ -11,6 +11,7 @@ //! - `iggy_connection_reconnects_total` - Total reconnection attempts //! - `iggy_circuit_breaker_opens_total` - Times the circuit breaker opened //! - `iggy_circuit_breaker_rejections_total` - Requests rejected by circuit breaker (label: state = open | half_open) +//! - `iggy_circuit_breaker_probe_dispositions_total` - How half-open probe tokens ended (label: disposition = consumed | released | stale) //! //! ## Histograms //! - `iggy_send_duration_seconds` - Message send duration @@ -45,6 +46,8 @@ pub mod names { pub const CONNECTION_RECONNECTS_TOTAL: &str = "iggy_connection_reconnects_total"; pub const CIRCUIT_BREAKER_OPENS_TOTAL: &str = "iggy_circuit_breaker_opens_total"; pub const CIRCUIT_BREAKER_REJECTIONS_TOTAL: &str = "iggy_circuit_breaker_rejections_total"; + pub const CIRCUIT_BREAKER_PROBE_DISPOSITIONS_TOTAL: &str = + "iggy_circuit_breaker_probe_dispositions_total"; pub const SEND_DURATION_SECONDS: &str = "iggy_send_duration_seconds"; pub const POLL_DURATION_SECONDS: &str = "iggy_poll_duration_seconds"; pub const CONNECTION_STATUS: &str = "iggy_connection_status"; @@ -91,6 +94,11 @@ pub fn init_metrics(metrics_addr: SocketAddr) -> Result<(), String> { names::CIRCUIT_BREAKER_REJECTIONS_TOTAL, "Total number of requests rejected by circuit breaker" ); + describe_counter!( + names::CIRCUIT_BREAKER_PROBE_DISPOSITIONS_TOTAL, + "How half-open probe tokens ended: consumed (outcome recorded), \ + released (returned unrecorded), or stale (dropped into a later window)" + ); describe_histogram!( names::SEND_DURATION_SECONDS, @@ -155,6 +163,18 @@ pub fn record_circuit_breaker_rejection(state: &'static str) { counter!(names::CIRCUIT_BREAKER_REJECTIONS_TOTAL, "state" => state).increment(1); } +/// Record how a half-open probe token ended. +/// +/// Deliberately a disposition counter rather than an abandoned-only one. A +/// counter that only ever increments on the failure path reads identically +/// whether the system is healthy or the release path is dead code; with +/// `consumed` as a denominator, `released` flat while `consumed` climbs is a +/// visible signature rather than an absence. +pub fn record_circuit_breaker_probe_disposition(disposition: &'static str) { + counter!(names::CIRCUIT_BREAKER_PROBE_DISPOSITIONS_TOTAL, "disposition" => disposition) + .increment(1); +} + // ============================================================================= // Histogram Recording Functions // ============================================================================= From bc7848a9ce74cf2df35eda2f2e242c710ddfc00d Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sat, 1 Aug 2026 00:19:22 -0700 Subject: [PATCH 10/16] test(circuit-breaker): cover the three probe-accounting leaks One test per leak named in TD-2026-07-09, plus two guards. Leak 2, straggler re-grant: a window is re-granted while an earlier window's token is still outstanding, and the straggler must be discarded rather than credited to the live window. Ownership alone does not fix this - it is what the window id exists for. Leak 1, phantom release, respecified. The original shape - a request admitted while Closed handing back a token it never took - is unrepresentable now that Ungated carries no permit, so there is nothing left to assert. The test covers the same hazard one step later: a permit that outlives its window entirely, via HalfOpen -> Open -> a fresh HalfOpen. Leak 3, cancellation. The only leak Drop is strictly required for: a client disconnecting during an outage drops the request future mid-probe, and there is no code path left on which an explicit release could run. Driven with tokio::spawn plus abort rather than manual polling, since futures is not a dependency; the permit is held across a suspension point as a real operation would hold it. Also added: a resilience-level test that a scoped-deadline timeout returns its token even when the following reconnect fails and returns early. Both pre-existing reconnect-failure tests drive Closed breakers, where a release is a no-op, so neither would notice either way. Two guards rather than detectors, labelled as such. The deadlock ordering test would HANG rather than fail on regression - a deadlocked thread cannot assert its own deadlock - so it is an executable statement of the invariant, which holds structurally anyway. The projection test pins gauge() and metric_label() and asserts they differ from Display, which is the trap: Display renders "half-open" and the exported Prometheus label is "half_open". Each leak test was mutation-checked against the mechanism it guards. Removing the generation comparison fails exactly the two window-identity tests and nothing else; making Drop inert fails exactly the two release tests and the resilience invariance test. No leak test passes without the mechanism it was written for. 191 lib tests pass; fmt, clippy --all-targets -D warnings and rustdoc -D warnings all clean. --- src/iggy_client/circuit_breaker.rs | 135 +++++++++++++++++++++++++++++ src/iggy_client/resilience.rs | 37 ++++++++ 2 files changed, 172 insertions(+) diff --git a/src/iggy_client/circuit_breaker.rs b/src/iggy_client/circuit_breaker.rs index c034714..286176b 100644 --- a/src/iggy_client/circuit_breaker.rs +++ b/src/iggy_client/circuit_breaker.rs @@ -981,6 +981,8 @@ impl Default for CircuitBreaker { #[cfg(test)] #[allow(clippy::unwrap_used, clippy::expect_used)] mod tests { + use std::sync::Arc; + use super::*; #[tokio::test] @@ -1339,6 +1341,139 @@ mod tests { ); } + // ========================================================================= + // TD-2026-07-09: the three probe-accounting leaks, one test each + // ========================================================================= + + #[tokio::test(start_paused = true)] + async fn test_straggler_from_an_expired_window_does_not_inflate_the_new_one() { + // Leak 2. A re-grant replaces the budget while an earlier window's + // token is still outstanding; when that straggler finally returns it + // must be discarded, not credited to the live window. Ownership alone + // does not fix this - it needs the window id. + let cb = CircuitBreaker::new(CircuitBreakerConfig::new(1, 1, Duration::from_secs(30))); + cb.record_failure(); + tokio::time::advance(Duration::from_secs(30)).await; + + let stale = cb.admit().expect("window 1 token"); + assert!(cb.admit().is_err(), "window 1 holds a single token"); + + // Window 2, granted while window 1's token is still out. + tokio::time::advance(Duration::from_secs(30)).await; + let _current = cb.admit().expect("window 2 re-granted"); + assert!(cb.admit().is_err(), "window 2 also holds a single token"); + + drop(stale); + assert!( + cb.admit().is_err(), + "a token from an expired window must not inflate the live one" + ); + } + + #[tokio::test(start_paused = true)] + async fn test_permit_dropped_after_leaving_half_open_does_not_seed_the_next_window() { + // Leak 1, respecified. The original phantom release - a request + // admitted while Closed handing back a token it never took - is + // unrepresentable now that Ungated carries no permit, so there is + // nothing left to assert about it. What remains testable is the same + // hazard one step later: a permit that outlives its window entirely. + let cb = CircuitBreaker::new(CircuitBreakerConfig::new(1, 1, Duration::from_secs(30))); + cb.record_failure(); + tokio::time::advance(Duration::from_secs(30)).await; + + let stale = cb.admit().expect("window 1 token"); + + // Fail the probe: back to Open, then into a brand new window. + cb.record_failure(); + assert_eq!(cb.state(), CircuitState::Open); + tokio::time::advance(Duration::from_secs(30)).await; + let _fresh = cb.admit().expect("window 2 token"); + assert!(cb.admit().is_err(), "window 2 holds a single token"); + + drop(stale); + assert!( + cb.admit().is_err(), + "a permit that outlived its window must not seed the next one" + ); + } + + #[tokio::test] + async fn test_a_probe_cancelled_mid_flight_returns_its_token() { + // Leak 3, and the only one Drop is strictly required for: a client + // disconnecting during an outage drops the request future while its + // probe is in flight. An explicit release call cannot cover this - + // there is no code path left to run it on. + let cb = Arc::new(CircuitBreaker::new(CircuitBreakerConfig::new( + 1, + 1, + Duration::from_secs(30), + ))); + cb.force_half_open(); + + let task_cb = Arc::clone(&cb); + let in_flight = tokio::spawn(async move { + let _probe = task_cb.admit().expect("token admitted"); + // Hold the permit across a suspension point, as a real operation + // awaiting the server would. + std::future::pending::<()>().await; + }); + + tokio::task::yield_now().await; + assert!( + cb.admit().is_err(), + "the in-flight probe holds the only token" + ); + + in_flight.abort(); + let _ = in_flight.await; + + assert!( + cb.admit().is_ok(), + "a probe cancelled mid-flight must return its token" + ); + } + + #[test] + fn test_dropping_a_permit_after_recording_does_not_deadlock() { + // Drop takes the same non-reentrant mutex that record_* takes, so the + // ordering is load-bearing. It holds structurally rather than by + // convention: record_success releases its guard before returning, and + // admit mints the permit only after releasing its own, so no permit + // can exist while a guard is live. + // + // A regression here HANGS rather than fails - a deadlocked thread + // cannot assert its own deadlock - so this is an executable statement + // of the ordering rather than a detector. + let cb = CircuitBreaker::new(CircuitBreakerConfig::new(1, 1, Duration::from_secs(30))); + cb.force_half_open(); + + let probe = cb.admit().expect("token admitted"); + cb.record_success(); + drop(probe); + + assert_eq!(cb.state(), CircuitState::Closed); + } + + #[test] + fn test_metric_projections_are_distinct_from_display() { + assert_eq!(CircuitState::Closed.gauge(), 0); + assert_eq!(CircuitState::HalfOpen.gauge(), 1); + assert_eq!(CircuitState::Open.gauge(), 2); + + assert_eq!(Rejection::Open.metric_label(), "open"); + assert_eq!(Rejection::ProbeBudgetExhausted.metric_label(), "half_open"); + + // The distinction is the point: Display is user-facing prose and + // renders a hyphen, while the exported Prometheus label uses an + // underscore. Routing the label through Display would silently rename + // it and break existing queries. + assert_eq!(CircuitState::HalfOpen.to_string(), "half-open"); + assert_ne!( + CircuitState::HalfOpen.to_string(), + Rejection::ProbeBudgetExhausted.metric_label() + ); + } + #[tokio::test(start_paused = true)] async fn test_consuming_a_permit_does_not_return_its_token() { // The other half of the mechanism: a RECORDED outcome must not also diff --git a/src/iggy_client/resilience.rs b/src/iggy_client/resilience.rs index 8836530..6042234 100644 --- a/src/iggy_client/resilience.rs +++ b/src/iggy_client/resilience.rs @@ -736,6 +736,43 @@ mod tests { ); } + #[tokio::test(start_paused = true)] + async fn scoped_timeout_returns_the_probe_token_even_when_reconnect_fails() { + // The permit is disposed before the reconnect step runs, so an early + // return from a failed reconnect cannot strand the token. Both + // pre-existing reconnect-failure tests drive Closed breakers, where a + // release is a no-op, so neither would notice either way. + let breaker = breaker_with(1); + breaker.record_failure(); + tokio::time::advance(Duration::from_secs(30)).await; + + let reconnects = Arc::new(AtomicU32::new(0)); + let result: AppResult = run_resilient( + &breaker, + TIMEOUT, + false, // client-scoped deadline: must not feed the breaker + || false, // disconnected, so the reconnect path is taken + fake_reconnect( + &reconnects, + Err(AppError::ConnectionFailed("reconnect exhausted".into())), + ), + || async { std::future::pending().await }, + ) + .await; + + assert!(matches!(&result, Err(AppError::ConnectionFailed(_)))); + assert_eq!(reconnects.load(Ordering::SeqCst), 1); + assert_eq!( + breaker.state(), + CircuitState::HalfOpen, + "a scoped deadline expiring is not outage evidence" + ); + assert!( + breaker.admit().is_ok(), + "the probe token must be back despite the early return" + ); + } + // ========================================================================= // Error classifier // ========================================================================= From 7d0513c616f460206e7f78a8413eaef9b6dc453b Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sat, 1 Aug 2026 00:21:38 -0700 Subject: [PATCH 11/16] docs(tech-debt): resolve TD-09, sync registry, carry TD-08 findings forward TD-2026-07-09 flips to resolved with a Resolution section covering all six commits: the state enum and what it deleted, the three leaks and the distinct mechanism that closes each, the sync lock as a prerequisite rather than a preference, the observability split, and the surface narrowing. It also corrects its own deferral note. "The enum refactor is shape-only with no behavioral delta" held for the enum and for the sync conversion it turned out to require, but not for the permit -- releasing on drop is a real behavior change and the only one this TD carried. Left uncorrected, that sentence would sit directly above a resolution disproving it. TD-2026-07-08 gains two silences the plan review found that its Problem section did not name: a rejected header is indistinguishable from an absent one, so the echo alone cannot close its item 2; and a non-UTF-8 header value is dropped with no log at all, because the malformed-value warn sits inside the to_str success branch. Recorded on the TD rather than only in the review artifacts, since the registry index is what a future session actually reaches. Its deferral to session 04 is noted with the reason -- the two records were mis-sequenced, not oversized. Registry row synced by hand; this repo has no index generator. README's test count moves 183 -> 191, verified by running the suite rather than by arithmetic. CLAUDE.md needs no equivalent edit: PR #30 removed the section that carried counts. --- README.md | 2 +- docs/tech-debt/README.md | 2 +- docs/tech-debt/TD-2026-07-08.md | 30 ++++++++++++++ docs/tech-debt/TD-2026-07-09.md | 73 ++++++++++++++++++++++++++++++++- 4 files changed, 104 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index ad24ef1..1abf440 100644 --- a/README.md +++ b/README.md @@ -43,7 +43,7 @@ Apache Iggy is capable of processing millions of messages per second with ultra- ### Development & Testing - Docker Compose setup for local development -- Comprehensive test suite (183 unit tests, 30 integration tests, 18 model tests, plus a metrics exporter smoke test) +- Comprehensive test suite (191 unit tests, 30 integration tests, 18 model tests, plus a metrics exporter smoke test) - Integration tests with testcontainers (auto-spins Iggy server) - Fuzz testing for input validation functions diff --git a/docs/tech-debt/README.md b/docs/tech-debt/README.md index 6cc4be3..e3c2835 100644 --- a/docs/tech-debt/README.md +++ b/docs/tech-debt/README.md @@ -13,4 +13,4 @@ condition under which the record MUST be resolved (not "someday"). | [TD-2026-07-06](TD-2026-07-06.md) | Durable-storage guide config re-validation | Review session 01 (consistency #10) | Next server image bump past 0.8.x | resolved (session 02) | | [TD-2026-07-07](TD-2026-07-07.md) | Pin third-party GitHub Actions to commit SHAs | Security review on v0.2.0 release PR | Next CI-focused change, or any new repo secret | resolved (session 02) | | [TD-2026-07-08](TD-2026-07-08.md) | Client-visible feedback for X-Request-Timeout | Review session 02 (silentfail M3) | Before documenting the header in any external API reference | open | -| [TD-2026-07-09](TD-2026-07-09.md) | Breaker per-state data as enum with payloads | Review session 02 (types MEDIUM) | Next behavioral change to CircuitBreakerState/allow_request | open | +| [TD-2026-07-09](TD-2026-07-09.md) | Breaker per-state data as enum with payloads | Review session 02 (types MEDIUM) | Next behavioral change to CircuitBreakerState/allow_request | resolved (session 03) | diff --git a/docs/tech-debt/TD-2026-07-08.md b/docs/tech-debt/TD-2026-07-08.md index a62b1a4..54907f6 100644 --- a/docs/tech-debt/TD-2026-07-08.md +++ b/docs/tech-debt/TD-2026-07-08.md @@ -21,6 +21,36 @@ Malformed AND out-of-range header values log at `warn!`; the middleware docs and describe the ignore-and-fall-back behavior accurately; server-side traces carry the timeout duration. +## Additional silences found in the session-03 plan review + +Recorded here rather than only in the review artifacts, because this record is +what a future session reaches from the registry index. Full analysis in +`docs/code-reviews/session-03-plan-round{1,2}.md`. + +3. **A rejected header is indistinguishable from an absent one.** Out-of-range + (`middleware/timeout.rs`, the `warn!` on the range check) and malformed + (the `warn!` on the parse failure) both log and drop the value, so the + request proceeds under the global timeout exactly as a header-less request + does. Echoing the *enforced* deadline does not fix this: a client that types + `X-Request-Timeout: 5s` still gets a 200 and never learns its integration is + broken. Item 2 above asks a client to confirm its header was **honored**, + and a single echoed number cannot say whether it was honored, clamped, or + rejected. Closing item 2 therefore needs a second signal (a status/source + header, or a `400` on a present-but-unusable value) — not just the echo. + +4. **A non-UTF-8 header value is dropped with no log at all.** The `warn!` for + a malformed value sits inside the `to_str()` success branch, so a header + whose bytes are not visible ASCII short-circuits the whole chain silently — + quieter than either silence item 2 names. + +## Sequencing note (session 03) + +This record was originally scoped into session 03 alongside TD-2026-07-09 and +was deferred to session 04 — not for size, but because the two were +mis-sequenced. TD-09's permit work deletes all four `release_probe()` call +sites in `resilience.rs`, which are the same arms this record's `Deadline` +threading edits. Landing TD-09 first leaves that file in its final shape. + ## Binding trigger Before the header is documented in any external API reference (README API diff --git a/docs/tech-debt/TD-2026-07-09.md b/docs/tech-debt/TD-2026-07-09.md index fe17ba3..e97f633 100644 --- a/docs/tech-debt/TD-2026-07-09.md +++ b/docs/tech-debt/TD-2026-07-09.md @@ -1,7 +1,7 @@ # TD-2026-07-09: Circuit-breaker per-state data as an enum with payloads **Source:** Review session 02, Round 1 — type-design-analyzer (MEDIUM). -**Status:** open +**Status:** resolved (session 03) ## Problem @@ -42,9 +42,80 @@ and pinned by the breaker unit-test suite (17+ tests) including concurrency and degenerate-config cases. The enum refactor is shape-only with no behavioral delta and deserves its own change with the full matrix re-run. +> **Corrected in session 03.** "Shape-only with no behavioral delta" held for +> the enum, and for the sync-lock conversion it turned out to require, but not +> for the permit: releasing on drop is a real behavior change, and it is the +> only one this TD carried. The session split accordingly — see the Resolution +> below. + ## Binding trigger The next behavioral change to `CircuitBreakerState` or `allow_request` (any new state, counter, or transition rule) MUST land as/with the enum-with-payloads restructuring rather than adding another convention-maintained field. + +## Resolution (session 03) + +Landed across six commits so each claim could be reviewed on its own evidence. + +**Shape.** `CircuitBreakerState` is gone. The mutex guards a `State` enum whose +variants own exactly what is meaningful in them — `Closed { consecutive_failures }`, +`Open { opened_at }`, `HalfOpen { probes_remaining, granted_at, consecutive_successes }` +— which deleted both `Option`s, the `is_none_or` guard, `force_close`'s +six-field hygiene reset, and three defensive resets on entry/close/reopen. The +public data-less `CircuitState` remains as an exhaustive projection with no `_` +arm, so a new internal variant is a compile error rather than a silent +mis-projection. Every one of the 18 breaker tests stayed byte-identical across +that commit; no assertion changed, and the only test-module edit was a comment +the refactor itself falsified. + +**Ownership.** `allow_request() -> bool` became +`admit() -> Result`. All three named leaks are closed, +each by a different mechanism and each with its own mutation-checked test: + +- *Phantom release* — `Admission::Ungated` carries no permit, because a request + admitted while Closed consumes no token. Unrepresentable rather than guarded. +- *Straggler re-grant* — a permit carries the id of the window that minted it, + and a token whose window has since been replaced is discarded instead of + credited to the live one. Ownership alone does not close this. +- *Dropped future* — `Drop` returns the token, so a client disconnecting + mid-probe no longer strands it. This is the leak that required RAII: there is + no code path left on which an explicit release could have run. + +**Lock.** `Drop` cannot await, so the release forced `tokio::sync::RwLock` to +`std::sync::Mutex` and every breaker method to sync. Not an independent +preference — a prerequisite. It also removed the read-lock fast path and the +read-to-write upgrade race that made the state re-match necessary. Poisoning is +recovered behind one `lock()` helper, justified by `panic = "abort"` in release +and gated on `!thread::panicking()` so an assert during unwind cannot abort the +test report. + +**Observability.** Tracing and the monotonic counters moved out of the critical +section; the Prometheus *gauge* deliberately did not, because it is +last-writer-wins and emitting it after releasing the guard would let racing +transitions leave it permanently disagreeing with the breaker. New: +`iggy_circuit_breaker_probe_dispositions_total{disposition}` — a disposition +counter with `consumed` as a denominator, not an abandoned-only counter, which +could not distinguish a healthy system from a dead release path. The re-grant +`info!` was deliberately NOT reworded into a leak alarm: it stays reachable for +probes still in flight past `open_duration`, the normal half-open case during an +outage. + +**Surface.** The breaker types narrowed to `pub(crate)` and three zero-caller +`IggyClientWrapper` accessors were deleted. That is the release's only semver +break; it left `times_opened`, `requests_rejected`, `force_close`, `force_open` +and `state` as `#[cfg(test)]`, which is what they had become. + +**Docs moved** (per the two-site invariant TD-2026-07-03 recorded): the breaker +module docs and usage example, its per-method docs, `resilience.rs`'s +composition semantics and the double-release paragraph this deletes, the +`IggyClientWrapper` circuit-breaker doc, and a superseded-in-part note on +TD-2026-07-03 itself. + +Plan review ran three rounds before any code (25 agents; artifacts under +`docs/code-reviews/session-03-plan-round{1,2,3}.md`). Round 1 found the original +plan's premise false; round 2 found four regressions in round 1's own +remediation and established that TD-2026-07-08 was mis-sequenced with this +record rather than merely mis-specified, which is why that TD moved to session +04. From 67c2f9cb36923ab1c7f07a1d082a7e35876e8a87 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sat, 1 Aug 2026 00:23:03 -0700 Subject: [PATCH 12/16] chore(release): prepare v0.4.0 Version bump, lockfile, and the 0.4.0 changelog entry; the Unreleased crossbeam-epoch security note moves into it. Marked a minor bump rather than a patch because the release carries one deliberate semver break: the circuit-breaker types are crate-internal and the three zero-caller IggyClientWrapper accessors are gone. That narrowing is what keeps the rest of the release non-breaking -- without it, every signature change in TD-2026-07-09 would have been a public break for no consumer's benefit. The entry names the sequencing decision too, since it explains an absence: TD-2026-07-08 was scoped into this session and is not in this release. Round 2 of the plan review established the two records were mis-sequenced rather than oversized -- TD-09's permit work deletes the same resilience.rs call sites TD-08's threading would edit -- so it moves to session 04 against a file in its final shape. --- CHANGELOG.md | 48 +++++++++++++++++++++++++++++++++++++++++++++++- Cargo.lock | 2 +- Cargo.toml | 2 +- 3 files changed, 49 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a756f3a..ec434b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,51 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.4.0] - 2026-08-01 + +Session-03 tech-debt sweep: TD-2026-07-09 resolved. Plan review ran three +rounds before any code was written (25 agents; artifacts under +`docs/code-reviews/session-03-plan-round{1,2,3}.md`), which is why +TD-2026-07-08 moved to session 04 — round 2 established the two records were +mis-sequenced rather than merely mis-specified. + +### Added + +- Ownership of half-open circuit-breaker probe tokens. `admit()` returns a + `ProbePermit` that returns its token on drop and is consumed when the + outcome is recorded, closing the three accounting leaks TD-2026-07-09 + named: a request admitted while closed can no longer release a token it + never took, a token from an expired probe window is discarded instead of + credited to the live one, and a request future dropped mid-probe — a client + disconnecting during an outage — returns its token instead of stranding it + until the re-grant window +- `iggy_circuit_breaker_probe_dispositions_total{disposition}` — how probe + tokens end, as `consumed` / `released` / `stale`. A disposition counter + rather than an abandoned-only one, so a flat failure count is + distinguishable from a dead release path + +### Changed + +- Circuit-breaker state is an enum whose variants own their own data, so a + field belonging to another state is unrepresentable. Deletes two `Option`s, + a window guard, a six-field hygiene reset and three defensive resets that + were previously maintained by convention at each mutation site +- The breaker's state is guarded by `std::sync::Mutex` and its methods are + synchronous. Not a preference: `Drop` cannot await, so a blocking guard is + what makes the probe permit's release possible at all. Also removes the + read-lock fast path and the read-to-write upgrade race it required +- Tracing and monotonic counters moved out of the breaker's critical section. + The Prometheus state gauge deliberately stays inside it: it is + last-writer-wins, and emitting it after releasing the guard would let racing + transitions leave it permanently disagreeing with the breaker +- **Breaking**: `CircuitBreaker`, `CircuitBreakerConfig` and `CircuitState` + are crate-internal, and `IggyClientWrapper`'s `circuit_breaker_state`, + `circuit_breaker_metrics` and `force_close_circuit` accessors are removed. + All had zero callers; narrowing the surface is what keeps the rest of this + release non-breaking +- CI accepts the Conventional Commits `!` breaking-change marker, which its + regex previously rejected outright + ### Security - Bumped transitive `crossbeam-epoch` 0.9.18 -> 0.9.20 (lockfile-only) to @@ -197,7 +242,8 @@ triggers (`docs/tech-debt/`): - Trusted proxy configuration for X-Forwarded-For validation - Input validation to prevent injection attacks -[Unreleased]: https://github.com/mlevkov/iggy_sample/compare/v0.3.0...HEAD +[Unreleased]: https://github.com/mlevkov/iggy_sample/compare/v0.4.0...HEAD +[0.4.0]: https://github.com/mlevkov/iggy_sample/compare/v0.3.0...v0.4.0 [0.3.0]: https://github.com/mlevkov/iggy_sample/compare/v0.2.0...v0.3.0 [0.2.0]: https://github.com/mlevkov/iggy_sample/compare/v0.1.0...v0.2.0 [0.1.0]: https://github.com/mlevkov/iggy_sample/releases/tag/v0.1.0 diff --git a/Cargo.lock b/Cargo.lock index 40c2748..c6e0d58 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2283,7 +2283,7 @@ dependencies = [ [[package]] name = "iggy_sample" -version = "0.3.0" +version = "0.4.0" dependencies = [ "anyhow", "axum", diff --git a/Cargo.toml b/Cargo.toml index 5460c49..40327fb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "iggy_sample" -version = "0.3.0" +version = "0.4.0" edition = "2024" rust-version = "1.93.0" description = "A comprehensive demonstration of Apache Iggy message streaming with Axum" From 671a27a5b73f17ddd332abeb0a8f97b272832467 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sat, 1 Aug 2026 00:41:12 -0700 Subject: [PATCH 13/16] fix(config): reject a zero open duration and operation timeout Both were already accepted by validate(); this session made the first one materially worse, which is why it is fixed here rather than deferred again. CIRCUIT_BREAKER_OPEN_DURATION_SECS=0 disables the breaker outright: Open never rejects because the window has always elapsed, and every admission past the budget re-grants. The re-grant bumps the probe generation, so each outstanding permit then comes back as a stale discard -- an info plus a warn per request, during exactly the outage the breaker exists to damp. The warn is new in this session, so the flood is a regression on top of a pre-existing hole. OPERATION_TIMEOUT_SECS=0 expires every request on its first poll. Because that deadline equals the global one it counts as outage evidence, so the breaker opens on a service that is perfectly healthy and never closes. --- src/config.rs | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/src/config.rs b/src/config.rs index e78900d..e8db356 100644 --- a/src/config.rs +++ b/src/config.rs @@ -277,6 +277,28 @@ impl Config { )); } + // A zero open window makes the breaker a no-op AND a log source: Open + // never rejects because the window has always elapsed, and every + // admission past the budget re-grants, bumping the probe generation so + // each outstanding permit then returns as a stale discard. That is an + // info plus a warn per request during the outage the breaker exists to + // damp. + if self.circuit_breaker_open_duration.is_zero() { + return Err(AppError::ConfigError( + "CIRCUIT_BREAKER_OPEN_DURATION_SECS must be greater than 0".to_string(), + )); + } + + // A zero operation timeout expires every request on its first poll, + // and because that deadline equals the global one it counts as outage + // evidence - so the breaker opens on a service that is perfectly + // healthy and never closes. + if self.operation_timeout.is_zero() { + return Err(AppError::ConfigError( + "OPERATION_TIMEOUT_SECS must be greater than 0".to_string(), + )); + } + Ok(()) } @@ -516,6 +538,45 @@ mod tests { assert!(result.unwrap_err().to_string().contains("BATCH_MAX_SIZE")); } + #[test] + fn test_validate_circuit_breaker_open_duration_zero() { + // A zero window disables the breaker and turns every request during an + // outage into an info + a warn. + let config = Config { + circuit_breaker_open_duration: Duration::ZERO, + ..Config::default() + }; + + let result = config.validate(); + assert!(result.is_err()); + assert!( + result + .unwrap_err() + .to_string() + .contains("CIRCUIT_BREAKER_OPEN_DURATION_SECS") + ); + } + + #[test] + fn test_validate_operation_timeout_zero() { + // A zero deadline opens the breaker on a healthy service: every + // request expires on first poll, at the global deadline, which counts + // as outage evidence. + let config = Config { + operation_timeout: Duration::ZERO, + ..Config::default() + }; + + let result = config.validate(); + assert!(result.is_err()); + assert!( + result + .unwrap_err() + .to_string() + .contains("OPERATION_TIMEOUT_SECS") + ); + } + #[test] fn test_validate_poll_max_count_zero() { let config = Config { From be74e312695ba812a516dc4646563af136a829d2 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sat, 1 Aug 2026 00:41:12 -0700 Subject: [PATCH 14/16] fix(circuit-breaker): remediate session-end review round 1 Eight agents reviewed the finished diff. Findings worth their own note: The disposition counter was not a partition. A permit dropped after the breaker left HalfOpen fell through a silent catch-all -- no counter, no log -- and that is the COMMON recovery path, not a corner: with the default budget of two, one probe failing reopens the circuit while its sibling is still in flight. So consumed+released+stale was less than the tokens minted, which breaks the very denominator argument used to choose a disposition counter over an abandoned-only one. Five of eight agents found it independently. The catch-all is now three explicit arms: a fourth `abandoned` label for a window that closed under a live probe, and a debug_assert plus warn for a release into a full window, which cannot happen unless the accounting is broken. The module usage example demonstrated the exact bug this session fixed. It bound the admission to a live local, called record_success, and let the permit drop -- recording an outcome AND refunding the token. It is rust,ignore, so the compiler could not catch it, and it is the first thing a reader copies. A pre-existing test was silently weakened. test_half_open_recovery_within_probe_budget admitted through temporaries, which refund on drop, so it passed with the probe budget hard-coded to one -- vacuous against the exact property it names. Now holds named locals, and fails under that mutation. The Rejection to error-message path had no coverage at all. Mapping ProbeBudgetExhausted to the wrong state left all 191 tests green, so the entire rationale for returning Rejection instead of re-reading state() was unpinned. Also pinned the four disposition label values, which are exported Prometheus labels: renaming one silently breaks queries. Prose: admit()'s re-grant rationale still argued the anti-wedge case for leaked tokens, which RAII removes, and contradicted the module doc two screens up. Three comments were written in the tense of work that has since landed. One paragraph shipped duplicated verbatim. RegrantedProbes logged the granted budget under the field name "outstanding", a number that can never be wrong-side-low. Records: TD-09's Resolution claimed all three leaks have their own mutation-checked test. Leak 1 is closed by construction and has no test -- what was written for it guards leak 2's mechanism. Corrected, along with the commit count, the plan deviation on the deleted release_probe test, and the CHANGELOG disposition list. One reported CRITICAL was a false positive: an unresolved [Display] intra-doc link failing the docs job. That agent had no shell; rustdoc with -D warnings passes. 194 lib tests; fmt, clippy --all-targets -D warnings and rustdoc -D warnings all clean. --- CHANGELOG.md | 17 ++++- README.md | 2 +- docs/tech-debt/README.md | 2 +- docs/tech-debt/TD-2026-07-09.md | 40 ++++++++-- src/iggy_client/circuit_breaker.rs | 113 ++++++++++++++++++++--------- src/iggy_client/resilience.rs | 54 ++++++++++++++ 6 files changed, 182 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ec434b4..8e0939d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,9 +26,10 @@ mis-sequenced rather than merely mis-specified. disconnecting during an outage — returns its token instead of stranding it until the re-grant window - `iggy_circuit_breaker_probe_dispositions_total{disposition}` — how probe - tokens end, as `consumed` / `released` / `stale`. A disposition counter - rather than an abandoned-only one, so a flat failure count is - distinguishable from a dead release path + tokens end, as `consumed` / `released` / `stale` / `abandoned`. The four + labels partition every admitted token, which is what makes `consumed` usable + as a denominator; an abandoned-only counter could not distinguish a healthy + system from a dead release path ### Changed @@ -52,6 +53,16 @@ mis-sequenced rather than merely mis-specified. - CI accepts the Conventional Commits `!` breaking-change marker, which its regex previously rejected outright +### Fixed + +- `Config::validate` rejects a zero `CIRCUIT_BREAKER_OPEN_DURATION_SECS`, which + disabled the breaker entirely — Open never rejected, and every admission past + the budget re-granted — and a zero `OPERATION_TIMEOUT_SECS`, which opened the + circuit on a healthy service and never closed it +- The 503 body for a rejected request now names the state that actually + rejected. It previously re-read the breaker after the fact and could report a + state a concurrent transition had already moved past + ### Security - Bumped transitive `crossbeam-epoch` 0.9.18 -> 0.9.20 (lockfile-only) to diff --git a/README.md b/README.md index 1abf440..0253374 100644 --- a/README.md +++ b/README.md @@ -43,7 +43,7 @@ Apache Iggy is capable of processing millions of messages per second with ultra- ### Development & Testing - Docker Compose setup for local development -- Comprehensive test suite (191 unit tests, 30 integration tests, 18 model tests, plus a metrics exporter smoke test) +- Comprehensive test suite (194 unit tests, 30 integration tests, 18 model tests, plus a metrics exporter smoke test) - Integration tests with testcontainers (auto-spins Iggy server) - Fuzz testing for input validation functions diff --git a/docs/tech-debt/README.md b/docs/tech-debt/README.md index e3c2835..d10d9e9 100644 --- a/docs/tech-debt/README.md +++ b/docs/tech-debt/README.md @@ -13,4 +13,4 @@ condition under which the record MUST be resolved (not "someday"). | [TD-2026-07-06](TD-2026-07-06.md) | Durable-storage guide config re-validation | Review session 01 (consistency #10) | Next server image bump past 0.8.x | resolved (session 02) | | [TD-2026-07-07](TD-2026-07-07.md) | Pin third-party GitHub Actions to commit SHAs | Security review on v0.2.0 release PR | Next CI-focused change, or any new repo secret | resolved (session 02) | | [TD-2026-07-08](TD-2026-07-08.md) | Client-visible feedback for X-Request-Timeout | Review session 02 (silentfail M3) | Before documenting the header in any external API reference | open | -| [TD-2026-07-09](TD-2026-07-09.md) | Breaker per-state data as enum with payloads | Review session 02 (types MEDIUM) | Next behavioral change to CircuitBreakerState/allow_request | resolved (session 03) | +| [TD-2026-07-09](TD-2026-07-09.md) | Breaker per-state data as enum with payloads | Review session 02 (types MEDIUM) | Next behavioral change to the breaker state or admission gate | resolved (session 03) | diff --git a/docs/tech-debt/TD-2026-07-09.md b/docs/tech-debt/TD-2026-07-09.md index e97f633..76ab750 100644 --- a/docs/tech-debt/TD-2026-07-09.md +++ b/docs/tech-debt/TD-2026-07-09.md @@ -57,7 +57,7 @@ convention-maintained field. ## Resolution (session 03) -Landed across six commits so each claim could be reviewed on its own evidence. +Landed across eight commits so each claim could be reviewed on its own evidence. **Shape.** `CircuitBreakerState` is gone. The mutex guards a `State` enum whose variants own exactly what is meaningful in them — `Closed { consecutive_failures }`, @@ -72,16 +72,22 @@ the refactor itself falsified. **Ownership.** `allow_request() -> bool` became `admit() -> Result`. All three named leaks are closed, -each by a different mechanism and each with its own mutation-checked test: - -- *Phantom release* — `Admission::Ungated` carries no permit, because a request - admitted while Closed consumes no token. Unrepresentable rather than guarded. +each by a different mechanism — but not each by its own test, and the +distinction is worth recording: + +- *Phantom release* — closed **by construction**, not by a test. + `Admission::Ungated` carries no permit, because a request admitted while + Closed consumes no token, so there is no longer a state of affairs to assert + against. What was written instead is the same hazard one step later: a permit + that outlives its window entirely. - *Straggler re-grant* — a permit carries the id of the window that minted it, and a token whose window has since been replaced is discarded instead of - credited to the live one. Ownership alone does not close this. + credited to the live one. Ownership alone does not close this. Two tests + guard the generation comparison; removing it fails both and nothing else. - *Dropped future* — `Drop` returns the token, so a client disconnecting mid-probe no longer strands it. This is the leak that required RAII: there is - no code path left on which an explicit release could have run. + no code path left on which an explicit release could have run. Covered by a + cancellation test; making `Drop` inert fails it and two others. **Lock.** `Drop` cannot await, so the release forced `tokio::sync::RwLock` to `std::sync::Mutex` and every breaker method to sync. Not an independent @@ -97,11 +103,29 @@ last-writer-wins and emitting it after releasing the guard would let racing transitions leave it permanently disagreeing with the breaker. New: `iggy_circuit_breaker_probe_dispositions_total{disposition}` — a disposition counter with `consumed` as a denominator, not an abandoned-only counter, which -could not distinguish a healthy system from a dead release path. The re-grant +could not distinguish a healthy system from a dead release path. The four +labels partition every minted token: the session-end review found the first cut +left the common recovery case (a sibling probe closing or reopening the circuit +while this one is in flight) falling through a silent catch-all, which would +have broken the denominator the metric is built on. The re-grant `info!` was deliberately NOT reworded into a leak alarm: it stays reachable for probes still in flight past `open_duration`, the normal half-open case during an outage. +**Also fixed here**, because this session's new warn-level alarm made a +pre-existing gap worse: `Config::validate` now rejects a zero +`CIRCUIT_BREAKER_OPEN_DURATION_SECS` (which disabled the breaker *and* emitted +an info plus a warn per request) and a zero `OPERATION_TIMEOUT_SECS` (which +opened the circuit on a healthy service and never closed it). + +**Deviation from the plan**, recorded rather than buried: +`test_release_probe_returns_token_capped_at_budget` was to be kept and renamed; +it was replaced instead by two tests driving the same property through the +public path, because `release_probe` became private and generation-aware. The +`probes_remaining < budget` cap it exercised is now unreachable through that +path — reaching it means the accounting is broken, so it is guarded by a +`debug_assert!` rather than a test. + **Surface.** The breaker types narrowed to `pub(crate)` and three zero-caller `IggyClientWrapper` accessors were deleted. That is the release's only semver break; it left `times_opened`, `requests_rejected`, `force_close`, `force_open` diff --git a/src/iggy_client/circuit_breaker.rs b/src/iggy_client/circuit_breaker.rs index 286176b..f2be349 100644 --- a/src/iggy_client/circuit_breaker.rs +++ b/src/iggy_client/circuit_breaker.rs @@ -63,24 +63,30 @@ //! let cb = CircuitBreaker::new(CircuitBreakerConfig::default()); //! //! // The gate is synchronous; only the guarded operation itself awaits. -//! // A half-open admission carries a permit owning one probe token. -//! let _permit = match cb.admit() { -//! Ok(admission) => admission, +//! // A half-open admission carries a permit owning one probe token; a Closed +//! // one consumes no token and so carries none. +//! let permit = match cb.admit() { +//! Ok(Admission::Ungated) => None, +//! Ok(Admission::Probe(permit)) => Some(permit), //! Err(rejection) => return Err(AppError::CircuitOpen(rejection.to_string())), //! }; //! -//! // Execute the operation. Only CONNECTION-CLASS outcomes feed the -//! // breaker (see `resilience::run_resilient` for the real composition): +//! // Execute the operation. Only CONNECTION-CLASS outcomes feed the breaker +//! // (see `resilience::run_resilient` for the real composition). Recording an +//! // outcome must CONSUME the permit: letting it drop as well would return a +//! // token the request already accounted for. //! match operation().await { //! Ok(result) => { //! cb.record_success(); +//! permit.map(ProbePermit::consume); //! Ok(result) //! } //! Err(e) if is_connection_error(&e) => { //! cb.record_failure(); +//! permit.map(ProbePermit::consume); //! Err(e) //! } -//! // Other errors record neither. Dropping the permit returns the token. +//! // Neither counter moves, so the token goes back: just drop the permit. //! Err(e) => Err(e), //! } //! ``` @@ -278,6 +284,9 @@ fn grant_window( /// How a half-open probe token ended. #[derive(Debug, Clone, Copy, PartialEq, Eq)] +/// Every minted token ends in exactly one of these, so the four together +/// partition the admissions that carried a permit. That totality is the point: +/// `consumed` is only usable as a denominator if nothing ends uncounted. enum Disposition { /// The outcome was recorded, so the token was not handed back. Consumed, @@ -285,6 +294,11 @@ enum Disposition { Released, /// Dropped into a different window than it was minted in, and discarded. Stale, + /// The breaker left HalfOpen while the probe was in flight, so the window + /// died with the variant and there was nothing to return the token to. + /// The common shape during recovery: a sibling probe closes or reopens the + /// circuit while this one is still running. + Abandoned, } impl Disposition { @@ -293,6 +307,7 @@ impl Disposition { Disposition::Consumed => "consumed", Disposition::Released => "released", Disposition::Stale => "stale", + Disposition::Abandoned => "abandoned", } } } @@ -386,8 +401,8 @@ impl<'a> ProbePermit<'a> { /// /// Consumes by value, so use-after-consume is a compile error rather than a /// silent no-op. The disarm is a move with no lock involved, which is what - /// makes it safe to call from anywhere — including, once `Drop` is wired, - /// from a path where a state guard might still be live. + /// makes it safe to call from anywhere, including a path where a state + /// guard is still live — which `Drop` itself is not. pub(crate) fn consume(self) { self.breaker.record_disposition(Disposition::Consumed); // Suppress the release without running Drop. ManuallyDrop rather than @@ -433,10 +448,17 @@ enum Effect { /// A request was turned away. Rejected(Rejection), /// A probe window elapsed with probes still unaccounted for, and was - /// re-granted so recovery cannot wedge. - RegrantedProbes { outstanding: u32 }, + /// re-granted so recovery cannot wedge. `granted` is the size of the NEW + /// window - not a count of what is still in flight, which the breaker does + /// not track. + RegrantedProbes { granted: u32 }, /// A token was dropped into a different window than it was minted in. StaleProbeDiscarded { minted: u64, current: u64 }, + /// A token outlived its window entirely - the breaker is no longer HalfOpen. + ProbeWindowGone, + /// A token came back to a window that is already whole. Unreachable unless + /// the accounting is wrong. + ProbeOverRelease, /// A probe token was handed back because its outcome was never recorded. ReleasedProbe, /// A success landed in HalfOpen; `closed` if it reached the threshold. @@ -455,8 +477,8 @@ enum Effect { /// /// State is guarded by a synchronous [`std::sync::Mutex`] rather than an async /// lock: every critical section is a short run of field updates that never -/// awaits, and a blocking guard is what lets a future RAII probe permit release -/// its token from `Drop`, which cannot await. No guard may be held across an +/// awaits, and a blocking guard is what lets [`ProbePermit::drop`] return its +/// token, which it could not do if releasing required an await. No guard may be held across an /// `.await` — `clippy::await_holding_lock` enforces that, and CI denies warnings. pub struct CircuitBreaker { /// Configuration parameters. @@ -548,15 +570,15 @@ impl CircuitBreaker { self.requests_rejected.fetch_add(1, Ordering::Relaxed); crate::metrics::record_circuit_breaker_rejection(rejection.metric_label()); } - Effect::RegrantedProbes { outstanding } => { + Effect::RegrantedProbes { granted } => { // Reachable for a legitimate reason even with RAII release: // probes still IN FLIGHT past open_duration, which is the // normal half-open case during an outage. Not an invariant // violation, so not a warning - the leak alarm is the stale // discard below. info!( - outstanding, - "Circuit breaker re-granted half-open probe tokens (previous window still outstanding)" + granted, + "Circuit breaker re-granted half-open probe tokens (previous window did not complete)" ); } Effect::ReleasedProbe => { @@ -564,6 +586,21 @@ impl CircuitBreaker { self.record_disposition(Disposition::Released); debug!("Circuit breaker released a half-open probe token (outcome not recorded)"); } + Effect::ProbeWindowGone => { + // Routine during recovery, so debug rather than warn - but it + // is counted, because an uncounted ending would break the + // partition the disposition metric depends on. + self.record_disposition(Disposition::Abandoned); + debug!("Circuit breaker probe ended after its window closed"); + } + Effect::ProbeOverRelease => { + self.record_disposition(Disposition::Abandoned); + debug_assert!(false, "probe token released into a full window"); + warn!( + "Circuit breaker saw a probe token released into a full window; \ + token accounting is inconsistent" + ); + } Effect::StaleProbeDiscarded { minted, current } => { // The token outlived its window. Bounded and self-correcting, // but it means a probe ran longer than open_duration, so it is @@ -632,11 +669,11 @@ impl CircuitBreaker { /// are rejected, which caps the probe load on a recovering server /// instead of letting every concurrent caller through at once. /// - /// Tokens re-grant after `open_duration` elapses in HalfOpen. This is - /// the anti-wedge guarantee: a probe whose outcome is never recorded - /// (e.g. the operation failed with a non-connection error, which by - /// design touches neither breaker counter) would otherwise leave the - /// breaker half-open with zero tokens forever. + /// Tokens re-grant after `open_duration` elapses in HalfOpen. A probe + /// whose outcome is never recorded now returns its token when its permit + /// drops, so the re-grant no longer covers leaked tokens — what remains is + /// a probe still IN FLIGHT past the window, which the breaker cannot + /// distinguish from a lost one and must not wait on forever. pub(crate) fn admit(&self) -> Result, Rejection> { /// Decided under the guard; the permit is minted only afterwards. /// @@ -675,11 +712,6 @@ impl CircuitBreaker { // tokens as it passes, so the window opens at // budget - 1; probe_budget() floors at 1, so this // cannot underflow. - // A NEW recovery attempt, so the success count starts - // at zero (contrast the re-grant arm below). The - // transitioning caller takes the first of the granted - // tokens as it passes, so the window opens at - // budget - 1; probe_budget() floors at 1. let generation = grant_window(state, probe_generation, budget - 1, 0); self.set_gauge(CircuitState::HalfOpen); ( @@ -711,9 +743,7 @@ impl CircuitBreaker { grant_window(state, probe_generation, budget - 1, successes); ( Decision::Probe(generation), - Effect::RegrantedProbes { - outstanding: budget, - }, + Effect::RegrantedProbes { granted: budget }, ) } else { ( @@ -785,9 +815,15 @@ impl CircuitBreaker { *probes_remaining += 1; Effect::ReleasedProbe } - // Already at budget, or the breaker has left HalfOpen entirely - // and the window died with the variant. - _ => Effect::None, + // Same window, already at full budget. Since remaining plus + // outstanding always equals the budget, no permit can exist to + // reach this arm - getting here means the token accounting is + // broken, not that a release was merely redundant. + State::HalfOpen { .. } => Effect::ProbeOverRelease, + // The breaker left HalfOpen while this probe was in flight: a + // sibling probe closed or reopened the circuit. The window died + // with the variant, so there is nothing to return the token to. + State::Closed { .. } | State::Open { .. } => Effect::ProbeWindowGone, } }; self.emit(effect); @@ -1250,9 +1286,13 @@ mod tests { cb.record_failure(); tokio::time::advance(Duration::from_secs(30)).await; - assert!(cb.admit().is_ok()); + // Permits held in named locals: as temporaries they would refund + // their tokens before the next admit, and the test would pass with the + // budget capped at one - which is exactly the property it claims to + // check. + let _probe_1 = cb.admit().expect("first probe admitted"); cb.record_success(); - assert!(cb.admit().is_ok()); + let _probe_2 = cb.admit().expect("second probe admitted"); cb.record_success(); assert_eq!(cb.state(), CircuitState::Closed); @@ -1463,6 +1503,13 @@ mod tests { assert_eq!(Rejection::Open.metric_label(), "open"); assert_eq!(Rejection::ProbeBudgetExhausted.metric_label(), "half_open"); + // Exported Prometheus label values; renaming one silently breaks + // existing queries, so they are pinned rather than trusted. + assert_eq!(Disposition::Consumed.label(), "consumed"); + assert_eq!(Disposition::Released.label(), "released"); + assert_eq!(Disposition::Stale.label(), "stale"); + assert_eq!(Disposition::Abandoned.label(), "abandoned"); + // The distinction is the point: Display is user-facing prose and // renders a hyphen, while the exported Prometheus label uses an // underscore. Routing the label through Display would silently rename diff --git a/src/iggy_client/resilience.rs b/src/iggy_client/resilience.rs index 6042234..a560c19 100644 --- a/src/iggy_client/resilience.rs +++ b/src/iggy_client/resilience.rs @@ -773,6 +773,60 @@ mod tests { ); } + #[tokio::test(start_paused = true)] + async fn rejection_message_names_the_state_that_actually_rejected() { + // The whole point of returning Rejection instead of re-reading state() + // is that the message names the state that made the decision. Nothing + // asserted on the message before, so mapping ProbeBudgetExhausted to + // the wrong state left every test green. + let breaker = breaker_with(1); + let calls = Arc::new(AtomicU32::new(0)); + let reconnects = Arc::new(AtomicU32::new(0)); + + // Hard-open: the request never runs. + breaker.force_open(); + let op_calls = Arc::clone(&calls); + let result: AppResult = run_resilient( + &breaker, + TIMEOUT, + true, + || true, + fake_reconnect(&reconnects, Ok(())), + move || { + let calls = Arc::clone(&op_calls); + async move { + calls.fetch_add(1, Ordering::SeqCst); + Ok(1) + } + }, + ) + .await; + let Err(AppError::CircuitOpen(message)) = result else { + panic!("an open circuit must reject"); + }; + assert!(message.contains("open"), "got: {message}"); + assert!(!message.contains("half-open"), "got: {message}"); + assert_eq!(calls.load(Ordering::SeqCst), 0, "operation must not run"); + + // Half-open with the budget spent: a different rejection reason, and + // the message must say so rather than reporting the enclosing state. + tokio::time::advance(Duration::from_secs(30)).await; + let _probe = breaker.admit().expect("the single probe token"); + let result: AppResult = run_resilient( + &breaker, + TIMEOUT, + true, + || true, + fake_reconnect(&reconnects, Ok(())), + || async { Ok(2) }, + ) + .await; + let Err(AppError::CircuitOpen(message)) = result else { + panic!("an exhausted probe budget must reject"); + }; + assert!(message.contains("half-open"), "got: {message}"); + } + // ========================================================================= // Error classifier // ========================================================================= From 58db0a270fbe3dc66d48f813aca82cbac318d57e Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sat, 1 Aug 2026 00:53:59 -0700 Subject: [PATCH 15/16] fix(circuit-breaker): remediate session-end review round 2 Round 2's job is finding regressions in round 1's remediation, and it found one, plus a set of records that drifted again. The regression: round 1 added `debug_assert!(false)` to the new over-release arm in emit(), which is reachable from ProbePermit::drop -> release_probe -> emit. Under cargo test that means a panic anywhere with a live permit unwinds, drops the permit, and double-panics to abort -- destroying the test report including the failure that started it. That is verbatim the hazard the lock() helper is gated against seventy lines above, and round 1 reintroduced it while citing the same reasoning elsewhere. Now guarded on !thread::panicking(), and the warn! moved ahead of the assert so the diagnostic survives the build that catches the bug. Five of six agents found it independently. The over-release arm no longer shares the `abandoned` label. Round 1 collapsed an invariant violation into the same Prometheus label as the routine recovery case, which is high-volume -- so in release, where the assert compiles out, a broken invariant's only trace was one warn line inside a climbing counter. `inconsistent` is now its own label, which makes it alertable. The exported metric HELP text still listed three dispositions. That string ships to /metrics and renders in Grafana, so operators would have seen a label domain that did not match what the code emits. The stated unreachability invariant was wrong: "remaining plus outstanding always equals the budget" is false after any consume, since a consumed token never comes back. The real relation is remaining + outstanding + consumed == budget, from which remaining == budget implies outstanding == 0. Same conclusion, sound argument. Records: the TD Resolution's mutation-failure counts were wrong again in both directions, so they are now measured rather than asserted -- removing the generation comparison fails three tests, making Drop inert fails four. The deviation paragraph contradicted itself in consecutive sentences (claiming the replacement tests drive "the same property" and then that the property is unreachable); it now says plainly that the budget cap has no test. The Binding trigger still named two symbols this record's own resolution deleted, so it carries a discharge note. The test-module banner still said "one test each" after the record was corrected to say leak 1 has no test. Also: a resilience test built CircuitBreakerConfig with a zero open duration -- the configuration production now refuses. Rewritten with a real window and an advance, so no test rests on a config that cannot exist. 194 lib tests, 18 model, 1 doc; fmt, clippy --all-targets -D warnings and rustdoc -D warnings all clean. --- CHANGELOG.md | 10 +++--- docs/tech-debt/TD-2026-07-09.md | 30 ++++++++++++------ src/iggy_client/circuit_breaker.rs | 50 +++++++++++++++++++++++------- src/iggy_client/resilience.rs | 10 ++++-- src/metrics.rs | 8 +++-- 5 files changed, 78 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e0939d..4b27fa8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,10 +26,12 @@ mis-sequenced rather than merely mis-specified. disconnecting during an outage — returns its token instead of stranding it until the re-grant window - `iggy_circuit_breaker_probe_dispositions_total{disposition}` — how probe - tokens end, as `consumed` / `released` / `stale` / `abandoned`. The four - labels partition every admitted token, which is what makes `consumed` usable - as a denominator; an abandoned-only counter could not distinguish a healthy - system from a dead release path + tokens end, as `consumed` / `released` / `stale` / `abandoned` / + `inconsistent`. The labels partition every admitted token, which is what + makes `consumed` usable as a denominator; an abandoned-only counter could not + distinguish a healthy system from a dead release path. `inconsistent` is + separate on purpose — it means the token accounting is wrong, and it must not + hide inside the routine `abandoned` volume ### Changed diff --git a/docs/tech-debt/TD-2026-07-09.md b/docs/tech-debt/TD-2026-07-09.md index 76ab750..9e172a8 100644 --- a/docs/tech-debt/TD-2026-07-09.md +++ b/docs/tech-debt/TD-2026-07-09.md @@ -55,9 +55,17 @@ The next behavioral change to `CircuitBreakerState` or `allow_request` enum-with-payloads restructuring rather than adding another convention-maintained field. +> **Discharged in session 03.** Both symbols named above are gone — +> `CircuitBreakerState` is the `State` enum and `allow_request` is `admit`. The +> restructuring the trigger demanded is what closed this record, so the trigger +> has no successor: per-state data now lives in the variant that owns it, and +> there is no flat struct left to accrete a convention-maintained field. + ## Resolution (session 03) -Landed across eight commits so each claim could be reviewed on its own evidence. +Landed incrementally so each claim could be reviewed on its own evidence: nine +commits touch the breaker, one behaviour-preserving step at a time, with the +single behavioural delta isolated in the one that wires `Drop`. **Shape.** `CircuitBreakerState` is gone. The mutex guards a `State` enum whose variants own exactly what is meaningful in them — `Closed { consecutive_failures }`, @@ -82,12 +90,13 @@ distinction is worth recording: that outlives its window entirely. - *Straggler re-grant* — a permit carries the id of the window that minted it, and a token whose window has since been replaced is discarded instead of - credited to the live one. Ownership alone does not close this. Two tests - guard the generation comparison; removing it fails both and nothing else. + credited to the live one. Ownership alone does not close this. Removing the + generation comparison fails three tests — the two window-identity ones and + the pre-existing re-grant test — and nothing outside that set. - *Dropped future* — `Drop` returns the token, so a client disconnecting mid-probe no longer strands it. This is the leak that required RAII: there is no code path left on which an explicit release could have run. Covered by a - cancellation test; making `Drop` inert fails it and two others. + cancellation test; making `Drop` inert fails four tests across both modules. **Lock.** `Drop` cannot await, so the release forced `tokio::sync::RwLock` to `std::sync::Mutex` and every breaker method to sync. Not an independent @@ -120,11 +129,14 @@ opened the circuit on a healthy service and never closed it). **Deviation from the plan**, recorded rather than buried: `test_release_probe_returns_token_capped_at_budget` was to be kept and renamed; -it was replaced instead by two tests driving the same property through the -public path, because `release_probe` became private and generation-aware. The -`probes_remaining < budget` cap it exercised is now unreachable through that -path — reaching it means the accounting is broken, so it is guarded by a -`debug_assert!` rather than a test. +it was deleted instead, because `release_probe` became private and +generation-aware. Two new tests cover what remains testable — that dropping a +permit returns its token and consuming one does not — but they exercise the +generation branch, *not* the `probes_remaining < budget` cap the old test drove. +That cap is now unreachable through the public path (`remaining == budget` +implies no permit is outstanding), so it has no test at all; reaching it means +the accounting is broken, and it is guarded by a `debug_assert!` plus its own +`inconsistent` metric label instead. **Surface.** The breaker types narrowed to `pub(crate)` and three zero-caller `IggyClientWrapper` accessors were deleted. That is the release's only semver diff --git a/src/iggy_client/circuit_breaker.rs b/src/iggy_client/circuit_breaker.rs index f2be349..03739cc 100644 --- a/src/iggy_client/circuit_breaker.rs +++ b/src/iggy_client/circuit_breaker.rs @@ -283,10 +283,11 @@ fn grant_window( } /// How a half-open probe token ended. +/// +/// Every minted token ends in exactly one of these, so they partition the +/// admissions that carried a permit. That totality is the point: `consumed` is +/// only usable as a denominator if nothing ends uncounted. #[derive(Debug, Clone, Copy, PartialEq, Eq)] -/// Every minted token ends in exactly one of these, so the four together -/// partition the admissions that carried a permit. That totality is the point: -/// `consumed` is only usable as a denominator if nothing ends uncounted. enum Disposition { /// The outcome was recorded, so the token was not handed back. Consumed, @@ -299,6 +300,11 @@ enum Disposition { /// The common shape during recovery: a sibling probe closes or reopens the /// circuit while this one is still running. Abandoned, + /// A token came back to a window that was already whole. Unreachable + /// unless the accounting is wrong, and kept as its own label precisely so + /// it cannot hide inside the routine `abandoned` volume — this one is + /// alertable, that one is not. + Inconsistent, } impl Disposition { @@ -308,6 +314,7 @@ impl Disposition { Disposition::Released => "released", Disposition::Stale => "stale", Disposition::Abandoned => "abandoned", + Disposition::Inconsistent => "inconsistent", } } } @@ -594,12 +601,21 @@ impl CircuitBreaker { debug!("Circuit breaker probe ended after its window closed"); } Effect::ProbeOverRelease => { - self.record_disposition(Disposition::Abandoned); - debug_assert!(false, "probe token released into a full window"); + self.record_disposition(Disposition::Inconsistent); + // The structured line first: in a debug build the assert below + // ends the process, and this is the diagnostic worth keeping. warn!( "Circuit breaker saw a probe token released into a full window; \ token accounting is inconsistent" ); + // Same reasoning as `lock()`: this is reachable from + // `ProbePermit::drop`, so under test a panic elsewhere can drop + // a live permit mid-unwind and land here. Asserting during an + // unwind double-panics straight to abort, taking the whole test + // report with it - including the failure that started it. + if !std::thread::panicking() { + debug_assert!(false, "probe token released into a full window"); + } } Effect::StaleProbeDiscarded { minted, current } => { // The token outlived its window. Bounded and self-correcting, @@ -815,10 +831,12 @@ impl CircuitBreaker { *probes_remaining += 1; Effect::ReleasedProbe } - // Same window, already at full budget. Since remaining plus - // outstanding always equals the budget, no permit can exist to - // reach this arm - getting here means the token accounting is - // broken, not that a release was merely redundant. + // Same window, already at full budget. The real invariant is + // remaining + outstanding + consumed == budget (a consumed + // token never comes back), so remaining == budget implies + // outstanding == 0 - there is no permit left that could reach + // this arm. Getting here means the accounting is broken, not + // that a release was merely redundant. State::HalfOpen { .. } => Effect::ProbeOverRelease, // The breaker left HalfOpen while this probe was in flight: a // sibling probe closed or reopened the circuit. The window died @@ -829,8 +847,10 @@ impl CircuitBreaker { self.emit(effect); } - /// Count how a probe token ended. Touches no state and takes no lock, so - /// it is safe from [`ProbePermit::consume`], which may run anywhere. + /// Count how a probe token ended. Touches no breaker state and takes no + /// breaker lock, so it is safe from [`ProbePermit::consume`], which may run + /// anywhere. (The metrics recorder takes its own registry lock — that is + /// why this is called outside the state guard, not why it is safe.) fn record_disposition(&self, disposition: Disposition) { crate::metrics::record_circuit_breaker_probe_disposition(disposition.label()); } @@ -1382,7 +1402,12 @@ mod tests { } // ========================================================================= - // TD-2026-07-09: the three probe-accounting leaks, one test each + // TD-2026-07-09: the three probe-accounting leaks. + // + // Not one test each. Leak 1 (phantom release) is closed by construction - + // Ungated carries no permit - so there is no state of affairs left to + // assert; its slot here holds the successor hazard, a permit outliving its + // window. See the record's Resolution. // ========================================================================= #[tokio::test(start_paused = true)] @@ -1509,6 +1534,7 @@ mod tests { assert_eq!(Disposition::Released.label(), "released"); assert_eq!(Disposition::Stale.label(), "stale"); assert_eq!(Disposition::Abandoned.label(), "abandoned"); + assert_eq!(Disposition::Inconsistent.label(), "inconsistent"); // The distinction is the point: Display is user-facing prose and // renders a hyphen, while the exported Prometheus label uses an diff --git a/src/iggy_client/resilience.rs b/src/iggy_client/resilience.rs index a560c19..df87550 100644 --- a/src/iggy_client/resilience.rs +++ b/src/iggy_client/resilience.rs @@ -332,10 +332,14 @@ mod tests { #[tokio::test(start_paused = true)] async fn success_passes_through_and_records_breaker_success() { // Enter HalfOpen (success_threshold = 1) so record_success is - // observable: the single success must close the circuit. Zero - // open_duration makes the Open->HalfOpen transition immediate. - let breaker = CircuitBreaker::new(CircuitBreakerConfig::new(1, 1, Duration::ZERO)); + // observable: the single success must close the circuit. + // + // A real 30s window plus an advance, rather than a zero window: zero is + // a configuration Config::validate now rejects, and a test resting on + // one production refuses would drift from reality. + let breaker = CircuitBreaker::new(CircuitBreakerConfig::new(1, 1, Duration::from_secs(30))); breaker.force_open(); + tokio::time::advance(Duration::from_secs(30)).await; let reconnects = Arc::new(AtomicU32::new(0)); let result: AppResult = run_resilient( diff --git a/src/metrics.rs b/src/metrics.rs index aee76e4..142e2b4 100644 --- a/src/metrics.rs +++ b/src/metrics.rs @@ -11,7 +11,7 @@ //! - `iggy_connection_reconnects_total` - Total reconnection attempts //! - `iggy_circuit_breaker_opens_total` - Times the circuit breaker opened //! - `iggy_circuit_breaker_rejections_total` - Requests rejected by circuit breaker (label: state = open | half_open) -//! - `iggy_circuit_breaker_probe_dispositions_total` - How half-open probe tokens ended (label: disposition = consumed | released | stale) +//! - `iggy_circuit_breaker_probe_dispositions_total` - How half-open probe tokens ended (label: disposition = consumed | released | stale | abandoned | inconsistent) //! //! ## Histograms //! - `iggy_send_duration_seconds` - Message send duration @@ -97,7 +97,11 @@ pub fn init_metrics(metrics_addr: SocketAddr) -> Result<(), String> { describe_counter!( names::CIRCUIT_BREAKER_PROBE_DISPOSITIONS_TOTAL, "How half-open probe tokens ended: consumed (outcome recorded), \ - released (returned unrecorded), or stale (dropped into a later window)" + released (returned unrecorded), stale (dropped into a later window), \ + abandoned (the window closed while the probe was still running), or \ + inconsistent (returned to a window that was already whole - a bug). \ + The labels partition every admitted token, so consumed is usable as a \ + denominator." ); describe_histogram!( From b72db84940c7f6be1b91f67042ee61e05ed40060 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sat, 1 Aug 2026 00:55:41 -0700 Subject: [PATCH 16/16] docs(code-reviews): session-03 code review rounds 1-2 The session-end double-review on the finished diff, as distinct from the three plan rounds already recorded. Round 1 found four HIGH defects, two of which were invisible to a fully green suite: the disposition counter was not a partition, so the common recovery path ended uncounted and broke the denominator the metric was designed around; and one pre-existing test had been silently weakened by the permit semantics into passing vacuously against the property it names. Both were found by agents that ran their own mutations rather than by reading. Also: the module usage example demonstrated the exact double-accounting bug the session fixed, and the Rejection-to-message path had no coverage at all. Round 2 found that round 1's remediation reintroduced, in a new place, the very hazard it cites elsewhere -- a debug_assert on a Drop-reachable path, which aborts the test binary mid-unwind instead of reporting. Five of six agents found it, and one proved it rather than arguing it. Both artifacts record what was NOT fixed, with triggers: the abandoned disposition has no test because no metrics recorder is installed under cargo test; one pre-existing test is vacuous for its name; and one commit subject exceeds the declared length limit. Round 1's artifact also records a false positive -- a confidently reported CRITICAL that verification disproved -- because the difference between it and the four real HIGHs was the verification step, not the plausibility of the claim. --- docs/code-reviews/session-03-round1.md | 155 +++++++++++++++++++++++++ docs/code-reviews/session-03-round2.md | 144 +++++++++++++++++++++++ 2 files changed, 299 insertions(+) create mode 100644 docs/code-reviews/session-03-round1.md create mode 100644 docs/code-reviews/session-03-round2.md diff --git a/docs/code-reviews/session-03-round1.md b/docs/code-reviews/session-03-round1.md new file mode 100644 index 0000000..4bb72cc --- /dev/null +++ b/docs/code-reviews/session-03-round1.md @@ -0,0 +1,155 @@ +# Session 03 — Code Review, Round 1 + +**Target:** branch `tech-debt/session-03` vs `main` (12 commits at review time, 14 +files, +1384/−419 excluding review artifacts) — TD-2026-07-09: circuit-breaker +state enum with payloads, RAII probe permit, synchronous mutex, surface +narrowing, and the v0.4.0 release prep. + +**Provenance:** config: full — step-0 gate attested by Maxim this session (8-agent +suite, both rounds, Opus-class). Agent fallbacks: none. Several agents ran their +own mutation experiments against the tree; all reverted. Two agents had no shell +and said so, which mattered once (see the false positive below). + +Reviewers cited in brackets: [consistency] general-purpose, [architect] +feature-dev:code-architect, [reviewer] feature-dev:code-reviewer, [types] +type-design-analyzer, [silent] silent-failure-hunter, [comments] comment-analyzer, +[tests] pr-test-analyzer, [simplifier] code-simplifier. + +Remediated in `671a27a` and `be74e31`. + +--- + +## T1 — HIGH — The disposition counter was not a partition + +*[architect] [types] [silent] [comments] [consistency] — 5 of 8.* + +A permit dropped after the breaker left HalfOpen fell through +`release_probe`'s `_ => Effect::None`: no counter, no log. And that is the +**common** recovery path, not a corner — with the default budget of two, one +probe failing reopens the circuit while its sibling is still in flight. + +So `consumed + released + stale` was strictly less than the tokens minted, which +breaks the exact denominator argument used to choose a disposition counter over +an abandoned-only one. The metric's own `describe_counter!` text and the +CHANGELOG both asserted totality. + +**Remediated:** the catch-all became three explicit arms — a fourth `abandoned` +label for a window that closed under a live probe, and a `debug_assert!` plus +`warn!` for a release into a full window, which cannot happen unless the +accounting is broken. + +## T2 — HIGH — The module usage example demonstrated the bug this session fixed + +*[comments].* + +The rewritten example bound the admission to a live local, called +`record_success()`, and let the permit drop at end of scope — recording an +outcome **and** refunding the token. That is precisely what `ProbePermit`'s own +doc says is now impossible. It is `rust,ignore`, so the compiler cannot catch +it, and it is the first thing a reader copies. + +**Remediated:** the example destructures `Admission` and consumes the permit on +both recorded arms, mirroring `run_resilient`. + +## T3 — HIGH — A pre-existing test was silently weakened + +*[architect] [tests].* + +`test_half_open_recovery_within_probe_budget` admitted through temporaries, +which refund their tokens on drop, so it passed with `probe_budget()` hard-coded +to 1 — vacuous against the exact property its name claims. [tests] confirmed by +mutation: capping the budget left it green while two neighbours failed. + +This is the one place the session's "no pre-existing test was weakened" claim +did not hold, and it was found by systematic comparison against `main` rather +than by reading. + +**Remediated:** named locals, and it now fails under that mutation. + +## T4 — HIGH — The `Rejection` → error-message path had no coverage + +*[tests].* + +Mapping `ProbeBudgetExhausted` to the wrong `CircuitState` left all 191 tests +green. The entire rationale for returning `Rejection` instead of re-reading +`state()` — that the message names the state which actually rejected — was +unpinned, and no test drove `run_resilient` against a budget-exhausted +half-open breaker at all. + +**Remediated:** a resilience test asserting both messages, plus pinned label +values for the disposition enum. Bidirectionally mutation-checked. + +## T5 — MEDIUM — Prose the final code falsified + +*[comments] [consistency] [architect] [simplifier].* + +- `admit()`'s re-grant rationale still argued the anti-wedge case for *leaked* + tokens, which RAII removes — and contradicted the module doc two screens up. +- Three comments were written in the tense of work that had since landed + ("a **future** RAII probe permit", "once `Drop` **is wired**", "the state enum + this refactor **is preparing for**"). +- One paragraph shipped duplicated verbatim inside `admit()`. +- `Effect::RegrantedProbes` logged the granted budget under the field name + `outstanding` — a number that can never be wrong-side-low. + +## T6 — MEDIUM — A config gap this session made worse + +*[architect] [reviewer].* + +`CIRCUIT_BREAKER_OPEN_DURATION_SECS=0` disables the breaker outright: Open never +rejects, every admission past the budget re-grants, and each re-grant bumps the +generation so every outstanding permit returns as a stale discard. The stale +`warn!` is new this session, so a pre-existing hole became a per-request warn +flood during exactly the outage the breaker damps. `OPERATION_TIMEOUT_SECS=0` +likewise opens the circuit on a healthy service and never closes it. + +**Remediated in `671a27a`** — both rejected by `Config::validate`, with tests. + +## T7 — MEDIUM — Records overclaimed + +*[consistency].* + +TD-09's Resolution said all three leaks are closed "each with its own +mutation-checked test". Leak 1 is closed **by construction** — `Ungated` carries +no permit — and the test written in its slot guards leak 2's mechanism. A +permanent record asserting coverage that does not exist is worse than a gap. + +Also corrected: the commit count, the CHANGELOG disposition list, and the +deviation on the deleted `release_probe` test. + +## T8 — LOW/MEDIUM — Accumulated + +- Commit `7d0513c`'s subject is 75 chars against `.commitlintrc.json`'s 72. No + workflow runs commitlint, so CI will not catch it. *[consistency] [reviewer]* +- The `stale` `warn!` is near-dead under default config — it needs + `OPERATION_TIMEOUT_SECS > CIRCUIT_BREAKER_OPEN_DURATION_SECS` to be reachable, + an undocumented ordering dependency. *[silent]* +- `Rejection::Open` emits no log at any level, only the counter, while + `ProbeBudgetExhausted` gets a `debug!`. Pre-existing asymmetry. *[silent]* +- `circuit_breaker.rs` grew 783 → 1509 lines, split roughly evenly between + production, docs and tests. [simplifier] judged `Effect` a genuine value type + rather than disguised flags, and the `run_resilient`/`retry_once` deferral + correct — this session made extraction *harder*, not easier, so the residual + ~15 duplicated lines beat the plumbing a shared helper would need. +- Several doc blocks narrate the diff rather than the steady state ("what used + to be four literals", "the old flat struct"). *[simplifier]* + +## One false positive, recorded + +*[reviewer]* reported as CRITICAL that an unresolved `[Display]` intra-doc link +would fail the docs job. It does not: `rustdoc` with `-D warnings` passes, and +the module is private so those links are not rendered. That agent had no shell +and said so. Worth recording because the finding was specific, plausible, and +wrong — the verification step is what separated it from T1-T4. + +--- + +## Verdict + +Four HIGH findings, all remediated. Two of them — the disposition gap and the +weakened test — were invisible to the full suite passing, and both were found by +agents that ran their own mutations rather than reading. The permit lifecycle, +generation mechanism, deadlock freedom, `Send`/`Sync` and gauge coverage were all +independently traced and came back clean. + +Round 2 follows against the remediated tree. diff --git a/docs/code-reviews/session-03-round2.md b/docs/code-reviews/session-03-round2.md new file mode 100644 index 0000000..1467097 --- /dev/null +++ b/docs/code-reviews/session-03-round2.md @@ -0,0 +1,144 @@ +# Session 03 — Code Review, Round 2 + +**Target:** `tech-debt/session-03` after Round 1's remediation (`671a27a`, +`be74e31`). Round 2's purpose is regressions introduced by that remediation. + +**Provenance:** config: full — same step-0 attestation. Six agents (the lenses +with live findings to re-check); no fallbacks. Agents again ran their own +mutations; several transiently observed each other's, which is why two reported +the tree dirty — it was clean at HEAD before and after. + +Remediated in `58db0a2`. + +--- + +## R2-1 — HIGH — The remediation reintroduced the exact hazard it cited elsewhere + +*[consistency] [architect] [silent] [comments] [reviewer] — 5 of 6.* + +Round 1's fix added `debug_assert!(false)` to the new over-release arm in +`emit()`. That arm is reachable from `ProbePermit::drop` → `release_probe` → +`emit`. Under `cargo test` — which unwinds, with debug assertions on — a panic +anywhere with a live permit drops that permit mid-unwind and lands here. +Asserting during an unwind double-panics straight to abort, destroying the test +report **including the failure that started it**. + +This is verbatim the hazard the `lock()` helper is gated against seventy lines +above, with the reasoning spelled out in its doc comment. Round 1 reproduced the +hazard while citing the rule. + +[consistency] proved it rather than arguing it: with the generation check +removed, a leak test fails, unwinds, drops a permit, and the run ends in SIGABRT +rather than a test failure. + +**Remediated:** guarded on `!std::thread::panicking()`, and the `warn!` moved +ahead of the assert so the diagnostic survives the build that catches the bug. + +## R2-2 — HIGH — An invariant violation was given the same label as routine recovery + +*[architect] [silent] [comments] [tests].* + +Round 1 recorded the over-release arm as `Disposition::Abandoned` — the same +label as a window closing under a live probe, which is the common, high-volume +recovery case. In release the `debug_assert!` compiles out, so a broken +invariant's only trace was one `warn!` line inside a counter that is already +climbing. Nothing alertable. + +**Remediated:** `inconsistent` is its own label. `rate(...{disposition="inconsistent"}) > 0` +is now a real alert, and the partition still holds. + +## R2-3 — HIGH — The exported metric description still listed three labels + +*[silent] [reviewer] [architect] [consistency].* + +`describe_counter!`'s HELP text ships to `/metrics` and renders in Grafana. Round 1 +added `abandoned` to the enum, the CHANGELOG and the TD record — but not to the +string operators actually read, nor to the module inventory. The label domain +shown to operators did not match what the code emits, for the very metric whose +value depends on totality. + +**Remediated:** both updated, and all five labels pinned by test. + +## R2-4 — MEDIUM — The stated unreachability invariant was false + +*[architect] [comments].* + +The new comment justified the over-release arm with "remaining plus outstanding +always equals the budget". That is false after any `consume()` — a consumed +token never comes back, so the sum is `budget − consumed`. Under the stated +equality the arm would be reachable whenever anything had been consumed. + +The conclusion survives on a correct argument: `remaining + outstanding + +consumed == budget`, so `remaining == budget` implies `outstanding == 0`, and no +permit exists to reach the arm. A maintainer checking the stated invariant would +have found it violated on the commonest path. + +## R2-5 — MEDIUM — Records drifted again + +*[consistency] [comments] [tests].* + +Round 1 corrected TD-09's counts and immediately introduced new ones that were +also wrong, in both directions: "two tests guard the generation comparison…and +nothing else" (three), "making `Drop` inert fails it and two others" (four), +"eight commits" (nine touch the breaker). The counts were written before the +commits they describe. + +Worse, the deviation paragraph contradicted itself in consecutive sentences — +the replacement tests drive "the same property", then that property is +"unreachable through that path". Both cannot hold. + +And the **Binding trigger** still named `CircuitBreakerState` and +`allow_request`, both deleted by this record's own resolution — the same two-site +drift TD-2026-07-03 exists to prevent. + +**Remediated:** numbers measured rather than asserted; the deviation says +plainly that the budget cap now has no test; the trigger carries a discharge +note. + +## R2-6 — MEDIUM — Leftovers + +- The test-module banner still read "the three probe-accounting leaks, one test + each" after the record was corrected to say leak 1 has none. *[comments]* +- `record_disposition`'s doc claimed it "takes no lock" while the `Effect` doc in + the same file says the metrics recorder takes a registry lock — and the false + claim was the stated licence for calling it near a guard. Now scoped to "no + *breaker* lock". *[comments] [architect]* +- The `Disposition` doc paragraph was inserted *after* `#[derive(...)]`, so + rustdoc collapsed summary and body. *[consistency] [comments]* +- A resilience test built `CircuitBreakerConfig::new(1, 1, Duration::ZERO)` — a + configuration `Config::validate` now refuses. Rewritten with a real window and + an advance, so no test rests on a config that cannot exist. *[architect]* + +## Open, not remediated — recorded deliberately + +- **`Effect::ProbeWindowGone` has no test.** Reverting it to `Effect::None` — the + catch-all Round 1 removed — leaves all 194 green. The arm *is* executed (by the + deadlock ordering test), but nothing asserts on it, because + `record_disposition` only calls `metrics::counter!` and no recorder is + installed under `cargo test`. Closing this needs a `#[cfg(test)]` tally on the + breaker. **Trigger:** add it with the next change to disposition accounting. + *[tests]* +- **`test_half_open_reentry_grants_fresh_probe_tokens` is vacuous for its name** — + with `success_threshold = 1` the granted budget is unobservable. Pre-existing + on `main`, not a session weakening. **Trigger:** fix when that test is next + touched. *[tests]* +- **Commit `7d0513c` is 75 chars** against the declared 72. Fixing it means a + rebase of merged-in history on the branch; left for Maxim's call at merge time. + *[reviewer] [consistency]* + +--- + +## Verdict + +One HIGH regression from Round 1's remediation, found by five of six agents and +proven rather than argued, plus two HIGH observability defects and a set of +records that drifted a second time. All remediated in `58db0a2`. + +No CRITICAL-class regression and no design question reopened, so **Round 3 is not +indicated**. Three items are recorded above as open with triggers rather than +silently dropped. + +The permit lifecycle, disposition partition, arm ordering and unreachability +argument were each re-derived independently this round and hold. 194 lib tests, +18 model, 1 doc; fmt, clippy `--all-targets --all-features -D warnings` and +rustdoc `-D warnings` clean.