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/CHANGELOG.md b/CHANGELOG.md index a756f3a..4b27fa8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,64 @@ 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` / `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 + +- 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 + +### 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 @@ -197,7 +255,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/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 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 97fbc1b..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" @@ -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/README.md b/README.md index ad24ef1..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 (183 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/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. 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. diff --git a/docs/tech-debt/README.md b/docs/tech-debt/README.md index 6cc4be3..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 | 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 the breaker state or admission gate | resolved (session 03) | 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/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..9e172a8 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,116 @@ 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. + +> **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 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 }`, +`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 — 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. 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 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 +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 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 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 +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. 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 { diff --git a/src/iggy_client/circuit_breaker.rs b/src/iggy_client/circuit_breaker.rs index fa2cb12..03739cc 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::allow_request`]. +//! 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,38 +62,43 @@ //! ```rust,ignore //! let cb = CircuitBreaker::new(CircuitBreakerConfig::default()); //! -//! // Check if request should be allowed -//! if !cb.allow_request().await { -//! 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; 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().await; +//! cb.record_success(); +//! permit.map(ProbePermit::consume); //! Ok(result) //! } //! Err(e) if is_connection_error(&e) => { -//! cb.record_failure().await; -//! Err(e) -//! } -//! // Other errors record neither; release any half-open probe token. -//! Err(e) => { -//! cb.release_probe().await; +//! cb.record_failure(); +//! permit.map(ProbePermit::consume); //! Err(e) //! } +//! // Neither counter moves, so the token goes back: just drop the permit. +//! Err(e) => 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)] @@ -111,6 +121,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 { @@ -143,44 +171,327 @@ 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). +/// 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 State { + /// The initial state: closed, with no failures recorded. + fn initial() -> Self { + State::Closed { + consecutive_failures: 0, + } + } +} + +/// 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, + } + } +} + +/// 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, - /// 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, +) -> u64 { + *probe_generation += 1; + *state = State::HalfOpen { + probes_remaining, + granted_at: Instant::now(), + consecutive_successes, + }; + *probe_generation +} + +/// 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)] +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, + /// 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, + /// 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 { + fn label(self) -> &'static str { + match self { + Disposition::Consumed => "consumed", + Disposition::Released => "released", + Disposition::Stale => "stale", + Disposition::Abandoned => "abandoned", + Disposition::Inconsistent => "inconsistent", + } + } +} + +/// 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. +/// +/// 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 +/// 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: &'a CircuitBreaker, + /// The probe window this token was taken from. + generation: u64, } -impl CircuitBreakerState { - fn new() -> Self { +impl<'a> ProbePermit<'a> { + fn new(breaker: &'a CircuitBreaker, generation: u64) -> Self { Self { - state: CircuitState::Closed, - opened_at: None, - consecutive_failures: 0, - consecutive_successes: 0, - half_open_probes_remaining: 0, - half_open_granted_at: None, + breaker, + generation, } } + + /// 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 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 + // 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<'_> { + fn drop(&mut self) { + self.breaker.release_probe(self.generation); + } +} + +/// 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. +/// +/// 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. + Rejected(Rejection), + /// A probe window elapsed with probes still unaccounted for, and was + /// 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. + 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. -/// 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 [`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. 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,15 +503,173 @@ impl CircuitBreaker { pub fn new(config: CircuitBreakerConfig) -> Self { Self { config, - state: RwLock::new(CircuitBreakerState::new()), + state: Mutex::new(Guarded { + state: State::initial(), + probe_generation: 0, + }), times_opened: AtomicU32::new(0), requests_rejected: AtomicU64::new(0), } } - /// Check if a request should be allowed through the circuit breaker. + /// 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<'_, Guarded> { + 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() + }) + } + + /// 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 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(state.gauge()); + } + + /// 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(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(rejection.metric_label()); + } + 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!( + granted, + "Circuit breaker re-granted half-open probe tokens (previous window did not complete)" + ); + } + 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::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::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, + // 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, + 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"); + } + } + } + + /// 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 /// @@ -216,88 +685,98 @@ 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. - 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 - } - } + /// 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. + /// + /// 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, + /// Admitted holding a token from this probe window. + Probe(u64), + Rejected(Rejection), } - // Write lock: Open -> HalfOpen transition, or HalfOpen token use. - let mut state = self.state.write().await; - - 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 - && 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; + // 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(); + // 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 { + // 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. + let generation = grant_window(state, probe_generation, budget - 1, 0); + self.set_gauge(CircuitState::HalfOpen); + ( + Decision::Probe(generation), + Effect::EnteredHalfOpen { probes: budget }, + ) + } else { + ( + Decision::Rejected(Rejection::Open), + Effect::Rejected(Rejection::Open), + ) + } } - 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 - .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"); + State::HalfOpen { + probes_remaining, + granted_at, + consecutive_successes, + } => { + if *probes_remaining > 0 { + *probes_remaining -= 1; + (Decision::Probe(*probe_generation), Effect::None) + } else if granted_at.elapsed() >= self.config.open_duration { + // 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 { granted: budget }, + ) + } else { + ( + Decision::Rejected(Rejection::ProbeBudgetExhausted), + Effect::Rejected(Rejection::ProbeBudgetExhausted), + ) } - // 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 } - } - } + }; - /// 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()); + self.emit(effect); + match decision { + Decision::Ungated => Ok(Admission::Ungated), + Decision::Probe(generation) => Ok(Admission::Probe(ProbePermit::new(self, generation))), + Decision::Rejected(rejection) => Err(rejection), + } } /// Half-open probe budget: `success_threshold`, floored at one so a @@ -307,16 +786,22 @@ impl CircuitBreaker { self.config.success_threshold.max(1) } - /// Record a rejection (counter + state-labeled metric) and return `false`. + /// Test-only: enter HalfOpen with a full probe budget, spending nothing. /// - /// 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 + /// 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 + /// 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(); + 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 @@ -326,148 +811,220 @@ 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; - if state.state == CircuitState::HalfOpen { - let cap = self.probe_budget(); - if state.half_open_probes_remaining < cap { - state.half_open_probes_remaining += 1; - debug!("Circuit breaker released a half-open probe token (outcome not recorded)"); + fn release_probe(&self, generation: u64) { + let effect = { + let mut guard = self.lock(); + let budget = self.probe_budget(); + 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 + } + // 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 + // with the variant, so there is nothing to return the token to. + State::Closed { .. } | State::Open { .. } => Effect::ProbeWindowGone, } - } + }; + self.emit(effect); + } + + /// 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()); } /// 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; - - 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" - ); - - if state.consecutive_successes >= self.config.success_threshold { - state.state = CircuitState::Closed; - state.opened_at = None; - state.consecutive_failures = 0; - crate::metrics::set_circuit_breaker_state(0); - info!("Circuit breaker closed after successful recovery"); + pub fn record_success(&self) { + let effect = { + let mut guard = self.lock(); + + match &mut guard.state { + State::Closed { + consecutive_failures, + } => { + // Reset failure counter on success + *consecutive_failures = 0; + Effect::None + } + State::HalfOpen { + consecutive_successes, + .. + } => { + *consecutive_successes += 1; + let successes = *consecutive_successes; + let closed = successes >= self.config.success_threshold; + if closed { + guard.state = State::Closed { + consecutive_failures: 0, + }; + self.set_gauge(CircuitState::Closed); + } + Effect::HalfOpenSuccess { successes, closed } + } + 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 + // 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. /// /// 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; - - 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 { - self.open_now(&mut state); - warn!( - failures = state.consecutive_failures, - open_duration = ?self.config.open_duration, - "Circuit breaker opened due to consecutive failures" - ); + pub fn record_failure(&self) { + let effect = { + let mut guard = self.lock(); + + match &mut guard.state { + 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 = *consecutive_failures; + let opened = failures >= self.config.failure_threshold; + if opened { + 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.state); + Effect::ReopenedFromHalfOpen + } + 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. + 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. - pub async fn state(&self) -> CircuitState { - self.state.read().await.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().state) } /// 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). - pub async fn force_close(&self) { - let mut state = self.state.write().await; - 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); + /// 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 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 = State::Closed { + consecutive_failures: 0, + }; + self.set_gauge(CircuitState::Closed); + } 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. - pub async fn force_open(&self) { - let mut state = self.state.write().await; - if state.state != CircuitState::Open { - self.open_now(&mut state); + /// 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 opened = { + let mut guard = self.lock(); + let changed = !matches!(guard.state, State::Open { .. }); + if changed { + self.open_now(&mut guard.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. - 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); + /// 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 State) { + *state = State::Open { + opened_at: Instant::now(), + }; + self.set_gauge(CircuitState::Open); } } @@ -480,13 +1037,15 @@ impl Default for CircuitBreaker { #[cfg(test)] #[allow(clippy::unwrap_used, clippy::expect_used)] mod tests { + use std::sync::Arc; + use super::*; #[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.admit().is_ok()); } #[tokio::test] @@ -495,13 +1054,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 +1069,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.admit().is_err()); assert_eq!(cb.requests_rejected(), 1); } @@ -523,15 +1082,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.admit().is_ok()); + assert_eq!(cb.state(), CircuitState::HalfOpen); } #[tokio::test(start_paused = true)] @@ -540,19 +1099,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.admit().is_ok()); + 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 +1120,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.admit().is_ok()); + 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 +1138,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.force_close().await; - assert_eq!(cb.state().await, CircuitState::Closed); - assert!(cb.allow_request().await); + cb.record_failure(); + cb.record_failure(); + cb.record_failure(); + cb.record_failure(); + cb.record_failure(); + assert_eq!(cb.state(), CircuitState::Open); + + cb.force_close(); + assert_eq!(cb.state(), CircuitState::Closed); + assert!(cb.admit().is_ok()); } #[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.admit().is_err()); } // ========================================================================= @@ -627,18 +1186,22 @@ 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); + // + // 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().await); + assert!(cb.admit().is_err()); assert_eq!(cb.requests_rejected(), rejected_before + 1); } @@ -647,24 +1210,71 @@ 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); + // 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().await); + 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().await); - assert_eq!(cb.state().await, CircuitState::HalfOpen); + assert!(cb.admit().is_ok()); + assert_eq!(cb.state(), CircuitState::HalfOpen); + } + + #[tokio::test(start_paused = true)] + async fn test_half_open_regrant_preserves_consecutive_successes() { + // 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); + + cb.record_failure(); + tokio::time::advance(Duration::from_secs(30)).await; + + // Entry consumes one of the two tokens; record a success against it. + // 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. + 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" + ); + + // Window expiry re-grants; the success recorded above must survive. + tokio::time::advance(Duration::from_secs(30)).await; + 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. + cb.record_success(); + assert_eq!( + cb.state(), + CircuitState::Closed, + "re-grant must preserve consecutive_successes" + ); } #[tokio::test(start_paused = true)] @@ -672,18 +1282,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.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().await); - assert_eq!(cb.state().await, CircuitState::HalfOpen); + assert!(cb.admit().is_ok()); + assert_eq!(cb.state(), CircuitState::HalfOpen); } #[tokio::test(start_paused = true)] @@ -693,16 +1303,20 @@ 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_eq!(cb.state().await, CircuitState::Closed); - assert!(cb.allow_request().await); + // 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(); + let _probe_2 = cb.admit().expect("second probe admitted"); + cb.record_success(); + + assert_eq!(cb.state(), CircuitState::Closed); + assert!(cb.admit().is_ok()); } #[tokio::test(start_paused = true)] @@ -712,68 +1326,253 @@ 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); + assert!(cb.admit().is_ok()); + } + + #[test] + fn test_half_open_concurrent_probes_admit_exactly_the_budget() { + // 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 + // 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 + // 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); + // 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() + }); + let second = s.spawn(|| { + gate.wait(); + cb.admit() + }); + (first.join().unwrap(), second.join().unwrap()) + }); + + assert!( + 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_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. + 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); - cb.record_failure().await; + cb.record_failure(); tokio::time::advance(Duration::from_secs(30)).await; - let (a, b) = tokio::join!(cb.allow_request(), cb.allow_request()); + let probe = cb.admit().expect("only token admitted"); + assert!( + cb.admit().is_err(), + "budget exhausted while the probe is held" + ); + + drop(probe); + let _readmitted = cb.admit().expect("released token re-admits"); assert!( - a ^ b, - "exactly one of two racing probes may pass, got ({a}, {b})" + cb.admit().is_err(), + "release must not push the budget above its cap" ); - assert_eq!(cb.state().await, CircuitState::HalfOpen); } + // ========================================================================= + // 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)] - async fn test_release_probe_returns_token_capped_at_budget() { - let config = CircuitBreakerConfig::new(1, 1, Duration::from_secs(30)); - let cb = CircuitBreaker::new(config); + 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"); - // No-op while Closed. - cb.release_probe().await; - assert!(cb.allow_request().await); + // 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" + ); + } - cb.record_failure().await; + #[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; - // 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); + 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"); - // Releases never exceed the granted budget (single token here). - cb.release_probe().await; - cb.release_probe().await; - assert!(cb.allow_request().await); + drop(stale); assert!( - !cb.allow_request().await, - "budget cap must hold after over-release" + 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"); + + // 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"); + 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 + // 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 + // 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] 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..0e48285 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; @@ -142,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 /// @@ -1047,26 +1052,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().await - } - - /// 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().await; - } } #[cfg(test)] diff --git a/src/iggy_client/resilience.rs b/src/iggy_client/resilience.rs index b71e4dd..df87550 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,17 +63,25 @@ //! 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; 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 +95,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,35 +135,43 @@ where R: FnOnce() -> RFut, RFut: Future>, { - // Check circuit breaker before attempting operation - if !breaker.allow_request().await { - // 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; - 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().await; + breaker.record_success(); + consume(permit); Ok(value) } Ok(Err(e)) if is_connection_error(&e) => { - breaker.record_failure().await; + 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 } 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().await; + // 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) } Err(_) => { @@ -155,13 +182,14 @@ 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(); + consume(permit); warn!( timeout = ?timeout, "Operation timed out at the global deadline (recorded as circuit-breaker failure)" ); } else { - breaker.release_probe().await; + drop(permit); debug!( timeout = ?timeout, "Operation timed out at a client-scoped deadline (not a breaker failure)" @@ -208,28 +236,25 @@ 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; } + // No token to hand back: the permit was disposed on the first + // attempt, before the reconnect that led here. 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; } Err(AppError::OperationTimeout(format!( "Operation timed out after {:?} on retry", @@ -277,7 +302,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)); @@ -307,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)); - breaker.force_open().await; + // 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( @@ -324,7 +353,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 +375,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 +439,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 +471,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 +500,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 +573,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 +605,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 +627,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 +676,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 +692,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.admit().is_ok(), "released token must admit the next probe" ); } @@ -705,12 +734,103 @@ 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" ); } + #[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" + ); + } + + #[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 // ========================================================================= diff --git a/src/metrics.rs b/src/metrics.rs index 99bcd59..142e2b4 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 | abandoned | inconsistent) //! //! ## 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,15 @@ 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), 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!( names::SEND_DURATION_SECONDS, @@ -155,6 +167,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 // =============================================================================