TD-2026-07-09: circuit-breaker state enum and RAII probe permit - #31
Merged
Conversation
Plan-review checkpoint for TD-2026-07-09 (and, in rounds 1-2, TD-2026-07-08). 25 agents plus a verification pass across three rounds. Round 1 invalidated the plan's central premise: timeout_is_outage_signal (mod.rs:504) is a duration comparison, not header provenance, so every client deadline at or above the global collapsed into the "no header" bucket. Round 2 found four regressions introduced by round 1's own remediation, two of them recreating the anti-patterns the session exists to remove: a stored outage_signal bool with no checkable invariant, and a metrics hoist that let racing transitions leave the Prometheus gauge permanently wrong. It also established that TD-08 and TD-09 are mis-sequenced rather than mis-specified. Session was re-scoped to TD-09 only on that finding; TD-08 moves to session 04 and must be read against these artifacts. Round 3 found no architectural regressions and did not indicate a round 4. Artifacts carry two TD-08 findings absent from the TD record: a rejected header remains indistinguishable from an absent one, and timeout.rs:129-131 drops a non-UTF-8 header value with no log at all.
The Conventional Commits check rejected the spec's breaking-change marker: the pattern had no `!?` between the optional scope group and the colon, so `refactor(scope)!: description` could not match on either backtrack path and the job exited 1. .commitlintrc.json permits `!` via config-conventional, but no workflow runs commitlint, so this regex was the only enforcer and the two configs disagreed. A BREAKING CHANGE: footer is not a workaround. This check reads only `%s` (pr.yml:74), and release.yml:178 formats the generated release notes the same way, so a footer-declared break would be invisible in both. Verified the relaxed pattern against 14 conventional subjects plus bare and bang variants (all accepted) and 8 malformed forms -- capitalized type, no space after the colon, bang before the scope, spaced bang, double bang, empty description, unknown type, prose -- all still rejected. Also updates the two places that document the convention to humans: the job's own failure message, and CONTRIBUTING.md, whose grammar and examples showed no `!` and whose type list omitted `style` and `revert` that the regex accepts.
grant_probe_tokens has two callers with opposite intent for a field it does not touch. The HalfOpen ENTRY path zeroes consecutive_successes in a separate statement before calling it; the RE-GRANT path must preserve the count, and today does so only because nothing there writes it. TD-2026-07-09 replaces those loose field writes with whole-variant enum construction, where the obvious port sets consecutive_successes: 0 at both sites and silently discards a recorded probe success -- the breaker would then need a fresh success_threshold run after every window expiry. The test exhausts the probe budget before advancing the clock, which is what makes it discriminating: with success_threshold = 2 the entry transition leaves one token, so without spending it and asserting the rejection the re-grant branch is never reached and the assertion passes vacuously. Verified by mutation: injecting consecutive_successes = 0 at the re-grant site fails this test on the exact assertion, while all 17 pre-existing breaker tests still pass -- the gap was real and unguarded. 184 lib tests pass; fmt and clippy --all-targets -D warnings clean.
Replaces tokio's RwLock with std::sync::Mutex and drops async from allow_request, record_success, record_failure, release_probe, state, force_close and force_open. The motivation is TD-2026-07-09's RAII probe permit, which must return its token from Drop. Drop cannot await, so release_probe().await can never run from a destructor; a blocking guard is the prerequisite, not a preference. The corresponding constraint is that no guard may cross an .await or every handler future stops being Send -- clippy::await_holding_lock is warn-by- default and CI denies warnings, so that is enforced rather than remembered. This is also a small win rather than a cost. tokio's RwLock::read() is a semaphore permit acquisition, not a free shared read, and the HalfOpen path previously took two locks where a mutex takes one. The read-lock fast path and its double-checked re-match both disappear, including the arm that existed only to handle another task closing the circuit during the read->write upgrade -- a race a single acquisition cannot have. Poisoning is handled by one private lock() helper. clippy::unwrap_used is denied here, and unwrap_or_else does not trip it, so no #[allow] is needed. The recovery branch is unreachable in release because panic = "abort" turns a panic under the guard into process death rather than a poisoned lock; it runs only under cargo test, where continuing on half-updated state would surface as a baffling failure in a later test. Hence the debug_assert -- and hence the !std::thread::panicking() gate around it, because poisoning implies an in-flight unwind and asserting during unwind double-panics to abort, destroying the report the assertion exists to sharpen. Cargo.toml records that the profile setting is load-bearing. The concurrency test could not be ported mechanically: a sync allow_request is not a future, so tokio::join! does not compile, and the obvious sequential rewrite would pass with the cap removed entirely. It is now two OS threads released by a Barrier, looped to amplify the window, with a fresh breaker per iteration and an open_duration long enough that the re-grant window cannot fire mid-race. Staging that race needs a test-only force_half_open, because the production path reaches HalfOpen only through allow_request, which consumes the very token under contention on the way in. Verified by mutation: forcing window_expired = true (the shape an open_duration of zero would produce) fails the redesigned test along with four others. All 17 pre-existing breaker tests keep their tokio runtime and pausable clock; only the concurrency test changed attribute. The breaker types are still publicly re-exported at this commit, so this is marked breaking. The next commits narrow that surface to pub(crate), after which the release's net public delta is that single deliberate narrowing. 184 lib tests pass; fmt, clippy --all-targets -D warnings, and rustdoc -D warnings all clean.
The breaker is a mechanism of the resilience executor, not part of this crate's API. `pub use` becomes `pub(crate) use`, and the three IggyClientWrapper accessors that exposed it -- circuit_breaker_state, circuit_breaker_metrics, force_close_circuit -- are deleted. All three had zero callers anywhere in src, tests or fuzz; they were an admin surface that was never wired to an endpoint. This is the release's single deliberate semver break. Every remaining commit in TD-2026-07-09 changes signatures on these types -- Admission, ProbePermit, the state enum -- and without the narrowing each would be a public break for no consumer's benefit. Two consequences worth naming rather than papering over: CircuitState leaves the re-export entirely. Its only crate-level user was the deleted accessor; resilience.rs's tests import it by module path. times_opened, requests_rejected, force_close and force_open become #[cfg(test)]. Deleting the accessors left them with only test callers, and dead_code is a warning CI denies. Marking them test-only states what they now are; the alternative was #[allow(dead_code)], which CLAUDE.md rules out for production code. force_close/force_open lose their "or manual recovery" doc claim in the process -- accurate, since the manual path was the accessor just removed. Verified by rustdoc: no public page is generated for CircuitBreaker or CircuitState, while IggyClientWrapper still has one. 184 lib tests pass; fmt, clippy --all-targets -D warnings and rustdoc -D warnings all clean.
Each method now computes an Effect under the state guard, releases it, then emits. Logging reaches a global tracing subscriber and the metrics recorder takes a registry shard lock and allocates a label key; neither belongs inside the mutex that gates every Iggy operation, and both were the only realistic way that mutex could ever be poisoned. The Prometheus state gauge deliberately does NOT move. Unlike the counters it is a last-writer-wins register, so two racing transitions emitting after release could land out of order and leave it permanently disagreeing with the breaker -- reading "closed" while every request is rejected -- until the next transition, which during a stalled recovery may never come. set_gauge is therefore called with the guard still held, which makes gauge order match transition order by construction. The counters are monotonic and commute, so hoisting them is free. That distinction, counters commute and gauges do not, is the whole design of this commit. set_gauge also replaces four hand-written 0/1/2 literals with one exhaustive match, so a new state cannot be added without deciding its gauge value. (main.rs seeds the gauge at startup with a bare 0 and is left alone; it runs before any breaker exists.) allow_request becomes single-tail. It previously had three early returns, and a tail emit would silently skip the Effect on any arm that still returned -- including the rejection path, whose counter a test asserts. record_failure captures consecutive_failures BEFORE open_now. The state enum this refactor is preparing for carries no failure count in its Open variant, so reading it after the transition would not survive that change, and the path of least resistance would be to drop the field from the log -- losing the only report of the threshold count at the moment the circuit opens. Same reason the half-open probe budget is captured before its decrement. open_now's doc claimed counters and metrics move "in lockstep"; that is now split deliberately, so it says which half stays under the guard and why. force_close and force_open keep their logs but emit them after their guard scope closes. Verified: every tracing and metrics call in the module now sits in emit, count_open, set_gauge or the poison path; emit and count_open never take the lock, so no re-entrancy is possible; all four gauge writes remain inside a live guard and cover the same four transitions as before. 184 lib tests pass unchanged; fmt, clippy --all-targets -D warnings and rustdoc -D warnings all clean.
Each variant now owns exactly the data meaningful while the breaker is in it:
Closed { consecutive_failures }, Open { opened_at }, HalfOpen
{ probes_remaining, granted_at, consecutive_successes }.
The flat struct kept every field in every state, so opened_at was Some only in
Open, the two half-open fields only in HalfOpen, and consecutive_failures only
in Closed -- invariants maintained by convention at each mutation site and
re-established by hand on every transition. Deleted outright: both Options,
the is_none_or window guard, force_close's six-field hygiene reset (one
assignment now; the enum drops stale data with the variant), the three
defensive resets on entry/close/reopen, and grant_probe_tokens, whose two
callers wanted different things from it.
That difference is now documented as a table on the type and expressed at both
sites. Entering HalfOpen is a NEW recovery attempt, so consecutive_successes
starts at zero; re-granting an expired window is the SAME attempt continuing,
so it must be preserved. Writing a zero at both sites is the tempting port in
either shape, and it silently discards a recorded probe success.
CircuitBreakerState disappears entirely -- the mutex now guards State directly
rather than a wrapper whose only remaining field was the state itself.
CircuitState gains gauge(), an exhaustive projection with no _ arm, so a new
state cannot be added without deciding what operators see. It is deliberately
separate from Display, whose "half-open" rendering is user-facing prose and
differs from the metric vocabulary. From<&State> for CircuitState is likewise
exhaustive, making a new internal variant a compile error rather than a silent
mis-projection.
Behavior preservation: no test assertion changed. The diff touches the test
module in exactly one place -- a comment on the M1 pin that described the
pre-refactor shape as if it were current, which this commit falsified. Every
other line in the 18-test module is untouched, verified by inspecting the diff
hunks rather than by eye.
Verified by mutation in the new shape: zeroing consecutive_successes on the
re-grant arm fails the pin on its own assertion, so the enum port did not
quietly lose the distinction the pin was written to guard.
184 lib tests pass; fmt, clippy --all-targets -D warnings and rustdoc
-D warnings all clean.
allow_request() -> bool becomes admit() -> Result<Admission, Rejection>. Ungated carries no permit by construction. The Closed path consumes no probe token, so there is nothing it could later hand back -- which is what makes a phantom release unrepresentable rather than merely guarded against. Only the two paths that actually take a token mint a ProbePermit. Rejection rather than Err(CircuitState): Closed never rejects, so an Err(Closed) would be representable and meaningless, and a label projection would need a bogus arm for it. It also deletes a real defect. run_resilient used to re-read state() after a rejection to name it in the error, and the comment there conceded the reported state "is not necessarily the one that rejected the request". Rejection is captured under the guard that made the decision, so that second acquisition and its staleness caveat both go. Rejection::metric_label is deliberately separate from CircuitState's Display. Display renders "half-open" as user-facing prose; the exported Prometheus label is "half_open", and routing one through the other would silently rename a label value and break existing queries. That also retires the hand-passed &'static str the rejection path used to thread through. Drop is an inert stub here. Landing the release separately is the point: with Drop doing nothing, every existing assertion still holds, so ~30 call-site rewrites can be reviewed as mechanical rather than as behavior, and the next commit's behavioral delta is a handful of lines. consume() lands now rather than with Drop, because unused_variables fires on Drop-typed bindings and every call site must therefore already dispose of its permit explicitly. The permit is minted AFTER the guard is released, via a local Decision enum. That is structural deadlock safety rather than a convention: once Drop takes the same non-reentrant mutex, a permit existing while a guard is live would hang a worker thread, and deferring the mint makes that unrepresentable even if someone later adds a ? mid-function. Four tests bind their permits to named locals, with a comment at each saying why: their assertions are that a window stays exhausted, which inverts the moment a temporary hands its token straight back. The other admissions are binding-insensitive and left as they are. The permit docs state plainly that this rule is review-enforced, not compiler-enforced -- #[must_use] on Admission does not survive .is_ok(). CircuitBreaker::state() joins the test-only set; the rejection message was its last production caller. 184 lib tests pass; fmt, clippy --all-targets -D warnings and rustdoc -D warnings all clean.
The behavioral delta of TD-2026-07-09, and the only one in this session. A ProbePermit now owns a real reference and the id of the window it was minted in. Dropping it returns the token; consume() gives it up without returning, for outcomes that were recorded. A request can no longer both record an outcome and refund its token, nor release the same token twice -- both of which resilience.rs previously documented as accepted slack. More to the point, a request future dropped mid-probe, which is a client disconnect during an outage, now returns the token it was holding instead of stranding it until the re-grant window. The window id closes the third leak. release_probe now compares the permit's generation against the live one and discards a token whose window has already been replaced, rather than crediting it to the current budget. The id lives in the guarded struct beside the state, not as an atomic on the breaker: every access is already under the guard, and an atomic would advertise lock-free access that is not safe to use that way. It cannot live inside State::HalfOpen either -- passing through Open would leave the next entry with nothing to read, and any restart lets a straggler match a recycled id. grant_window is the sole place a window is granted, so the bump and the grant cannot come apart. release_probe is private and reachable only from Drop. Observability. The counter is a disposition counter, consumed | released | stale, not an abandoned-only one: a counter that can only increment on the failure path reads identically whether the system is healthy or the release path is dead code, whereas released staying flat while consumed climbs is a visible signature. Routine releases log at debug, since every non-connection error in half-open lands there; the stale discard logs at warn, because it means a probe outran its window. The re-grant info was NOT reworded into an alarm: it stays reachable for a legitimate reason -- probes still in flight past open_duration, the normal half-open case during an outage -- so calling it an invariant violation would cry wolf on every real outage. It now reports how many were outstanding. Two tests failed on the first run, which is the M5/M6 split working as intended. Both admitted a probe as a temporary and then asserted the budget was exhausted; with Drop inert in M5 that passed, and with Drop live it does not, because the token goes straight back. The concurrency test was the sharper of the two -- testing is_ok() inside the racing threads dropped each permit there and let both racers through. Both now carry their admissions out to the assertion. The direct test of release_probe is replaced by two that drive the same property through the public path: dropping a permit returns its token, and consuming one does not. Docs corrected where this commit falsified them: the resilience module's double-release paragraph, its non-connection-error semantics, the breaker's probe-limiting prose and usage example, and both sites of the two-site invariant TD-2026-07-03 recorded, including a superseded-in-part note on that record. 185 lib tests pass; fmt, clippy --all-targets -D warnings and rustdoc -D warnings all clean.
One test per leak named in TD-2026-07-09, plus two guards. Leak 2, straggler re-grant: a window is re-granted while an earlier window's token is still outstanding, and the straggler must be discarded rather than credited to the live window. Ownership alone does not fix this - it is what the window id exists for. Leak 1, phantom release, respecified. The original shape - a request admitted while Closed handing back a token it never took - is unrepresentable now that Ungated carries no permit, so there is nothing left to assert. The test covers the same hazard one step later: a permit that outlives its window entirely, via HalfOpen -> Open -> a fresh HalfOpen. Leak 3, cancellation. The only leak Drop is strictly required for: a client disconnecting during an outage drops the request future mid-probe, and there is no code path left on which an explicit release could run. Driven with tokio::spawn plus abort rather than manual polling, since futures is not a dependency; the permit is held across a suspension point as a real operation would hold it. Also added: a resilience-level test that a scoped-deadline timeout returns its token even when the following reconnect fails and returns early. Both pre-existing reconnect-failure tests drive Closed breakers, where a release is a no-op, so neither would notice either way. Two guards rather than detectors, labelled as such. The deadlock ordering test would HANG rather than fail on regression - a deadlocked thread cannot assert its own deadlock - so it is an executable statement of the invariant, which holds structurally anyway. The projection test pins gauge() and metric_label() and asserts they differ from Display, which is the trap: Display renders "half-open" and the exported Prometheus label is "half_open". Each leak test was mutation-checked against the mechanism it guards. Removing the generation comparison fails exactly the two window-identity tests and nothing else; making Drop inert fails exactly the two release tests and the resilience invariance test. No leak test passes without the mechanism it was written for. 191 lib tests pass; fmt, clippy --all-targets -D warnings and rustdoc -D warnings all clean.
…orward TD-2026-07-09 flips to resolved with a Resolution section covering all six commits: the state enum and what it deleted, the three leaks and the distinct mechanism that closes each, the sync lock as a prerequisite rather than a preference, the observability split, and the surface narrowing. It also corrects its own deferral note. "The enum refactor is shape-only with no behavioral delta" held for the enum and for the sync conversion it turned out to require, but not for the permit -- releasing on drop is a real behavior change and the only one this TD carried. Left uncorrected, that sentence would sit directly above a resolution disproving it. TD-2026-07-08 gains two silences the plan review found that its Problem section did not name: a rejected header is indistinguishable from an absent one, so the echo alone cannot close its item 2; and a non-UTF-8 header value is dropped with no log at all, because the malformed-value warn sits inside the to_str success branch. Recorded on the TD rather than only in the review artifacts, since the registry index is what a future session actually reaches. Its deferral to session 04 is noted with the reason -- the two records were mis-sequenced, not oversized. Registry row synced by hand; this repo has no index generator. README's test count moves 183 -> 191, verified by running the suite rather than by arithmetic. CLAUDE.md needs no equivalent edit: PR #30 removed the section that carried counts.
Version bump, lockfile, and the 0.4.0 changelog entry; the Unreleased crossbeam-epoch security note moves into it. Marked a minor bump rather than a patch because the release carries one deliberate semver break: the circuit-breaker types are crate-internal and the three zero-caller IggyClientWrapper accessors are gone. That narrowing is what keeps the rest of the release non-breaking -- without it, every signature change in TD-2026-07-09 would have been a public break for no consumer's benefit. The entry names the sequencing decision too, since it explains an absence: TD-2026-07-08 was scoped into this session and is not in this release. Round 2 of the plan review established the two records were mis-sequenced rather than oversized -- TD-09's permit work deletes the same resilience.rs call sites TD-08's threading would edit -- so it moves to session 04 against a file in its final shape.
Both were already accepted by validate(); this session made the first one materially worse, which is why it is fixed here rather than deferred again. CIRCUIT_BREAKER_OPEN_DURATION_SECS=0 disables the breaker outright: Open never rejects because the window has always elapsed, and every admission past the budget re-grants. The re-grant bumps the probe generation, so each outstanding permit then comes back as a stale discard -- an info plus a warn per request, during exactly the outage the breaker exists to damp. The warn is new in this session, so the flood is a regression on top of a pre-existing hole. OPERATION_TIMEOUT_SECS=0 expires every request on its first poll. Because that deadline equals the global one it counts as outage evidence, so the breaker opens on a service that is perfectly healthy and never closes.
Eight agents reviewed the finished diff. Findings worth their own note: The disposition counter was not a partition. A permit dropped after the breaker left HalfOpen fell through a silent catch-all -- no counter, no log -- and that is the COMMON recovery path, not a corner: with the default budget of two, one probe failing reopens the circuit while its sibling is still in flight. So consumed+released+stale was less than the tokens minted, which breaks the very denominator argument used to choose a disposition counter over an abandoned-only one. Five of eight agents found it independently. The catch-all is now three explicit arms: a fourth `abandoned` label for a window that closed under a live probe, and a debug_assert plus warn for a release into a full window, which cannot happen unless the accounting is broken. The module usage example demonstrated the exact bug this session fixed. It bound the admission to a live local, called record_success, and let the permit drop -- recording an outcome AND refunding the token. It is rust,ignore, so the compiler could not catch it, and it is the first thing a reader copies. A pre-existing test was silently weakened. test_half_open_recovery_within_probe_budget admitted through temporaries, which refund on drop, so it passed with the probe budget hard-coded to one -- vacuous against the exact property it names. Now holds named locals, and fails under that mutation. The Rejection to error-message path had no coverage at all. Mapping ProbeBudgetExhausted to the wrong state left all 191 tests green, so the entire rationale for returning Rejection instead of re-reading state() was unpinned. Also pinned the four disposition label values, which are exported Prometheus labels: renaming one silently breaks queries. Prose: admit()'s re-grant rationale still argued the anti-wedge case for leaked tokens, which RAII removes, and contradicted the module doc two screens up. Three comments were written in the tense of work that has since landed. One paragraph shipped duplicated verbatim. RegrantedProbes logged the granted budget under the field name "outstanding", a number that can never be wrong-side-low. Records: TD-09's Resolution claimed all three leaks have their own mutation-checked test. Leak 1 is closed by construction and has no test -- what was written for it guards leak 2's mechanism. Corrected, along with the commit count, the plan deviation on the deleted release_probe test, and the CHANGELOG disposition list. One reported CRITICAL was a false positive: an unresolved [Display] intra-doc link failing the docs job. That agent had no shell; rustdoc with -D warnings passes. 194 lib tests; fmt, clippy --all-targets -D warnings and rustdoc -D warnings all clean.
Round 2's job is finding regressions in round 1's remediation, and it found one, plus a set of records that drifted again. The regression: round 1 added `debug_assert!(false)` to the new over-release arm in emit(), which is reachable from ProbePermit::drop -> release_probe -> emit. Under cargo test that means a panic anywhere with a live permit unwinds, drops the permit, and double-panics to abort -- destroying the test report including the failure that started it. That is verbatim the hazard the lock() helper is gated against seventy lines above, and round 1 reintroduced it while citing the same reasoning elsewhere. Now guarded on !thread::panicking(), and the warn! moved ahead of the assert so the diagnostic survives the build that catches the bug. Five of six agents found it independently. The over-release arm no longer shares the `abandoned` label. Round 1 collapsed an invariant violation into the same Prometheus label as the routine recovery case, which is high-volume -- so in release, where the assert compiles out, a broken invariant's only trace was one warn line inside a climbing counter. `inconsistent` is now its own label, which makes it alertable. The exported metric HELP text still listed three dispositions. That string ships to /metrics and renders in Grafana, so operators would have seen a label domain that did not match what the code emits. The stated unreachability invariant was wrong: "remaining plus outstanding always equals the budget" is false after any consume, since a consumed token never comes back. The real relation is remaining + outstanding + consumed == budget, from which remaining == budget implies outstanding == 0. Same conclusion, sound argument. Records: the TD Resolution's mutation-failure counts were wrong again in both directions, so they are now measured rather than asserted -- removing the generation comparison fails three tests, making Drop inert fails four. The deviation paragraph contradicted itself in consecutive sentences (claiming the replacement tests drive "the same property" and then that the property is unreachable); it now says plainly that the budget cap has no test. The Binding trigger still named two symbols this record's own resolution deleted, so it carries a discharge note. The test-module banner still said "one test each" after the record was corrected to say leak 1 has no test. Also: a resilience test built CircuitBreakerConfig with a zero open duration -- the configuration production now refuses. Rewritten with a real window and an advance, so no test rests on a config that cannot exist. 194 lib tests, 18 model, 1 doc; fmt, clippy --all-targets -D warnings and rustdoc -D warnings all clean.
The session-end double-review on the finished diff, as distinct from the three plan rounds already recorded. Round 1 found four HIGH defects, two of which were invisible to a fully green suite: the disposition counter was not a partition, so the common recovery path ended uncounted and broke the denominator the metric was designed around; and one pre-existing test had been silently weakened by the permit semantics into passing vacuously against the property it names. Both were found by agents that ran their own mutations rather than by reading. Also: the module usage example demonstrated the exact double-accounting bug the session fixed, and the Rejection-to-message path had no coverage at all. Round 2 found that round 1's remediation reintroduced, in a new place, the very hazard it cites elsewhere -- a debug_assert on a Drop-reachable path, which aborts the test binary mid-unwind instead of reporting. Five of six agents found it, and one proved it rather than arguing it. Both artifacts record what was NOT fixed, with triggers: the abandoned disposition has no test because no metrics recorder is installed under cargo test; one pre-existing test is vacuous for its name; and one commit subject exceeds the declared length limit. Round 1's artifact also records a false positive -- a confidently reported CRITICAL that verification disproved -- because the difference between it and the four real HIGHs was the verification step, not the plausibility of the claim.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes TD-2026-07-09: the circuit breaker's per-state data becomes an enum
whose variants own it, and half-open probe tokens gain real ownership via an RAII
ProbePermit. That closes the three accounting leaks the record named — arequest could release a token it never took, a token from an expired probe window
could inflate the live one, and a request future dropped mid-probe stranded its
token until the re-grant window.
Ships as v0.4.0 with one deliberate semver break: the breaker types are
crate-internal and three zero-caller accessors are gone. That narrowing is what
keeps everything else non-breaking.
TD-2026-07-08 is not in this PR. It was scoped into this session and moved to
session 04 — round 2 of the plan review established the two records were
mis-sequenced, not oversized: this work deletes the same
resilience.rscallsites TD-08's threading would edit, so TD-08 now lands on a file in its final
shape. The reasoning is recorded on both TD records.
Type of Change
!Changes Made
Sixteen commits, ordered so that all but one are behavior-preserving and the
single behavioral delta is isolated:
ci(pr)!?, so every breaking-change subject failed CItest(circuit-breaker)refactor!std::sync::Mutex;Dropcannot await, so this is a prerequisite, not a preferencerefactor!refactorrefactorrefactorAdmission/Rejectiontypes, permits bound at call sitesfeatDropreleases the token — the only behavioral delta, ~3 linestestdocs(tech-debt)chore(release)fix(config)fix×2docs(code-reviews)×2Testing
194 lib (+11), 30 integration, 18 model, 1 metrics smoke, 1 doc.
fmt,clippy --all-targets --all-features -D warningsandrustdoc -D warningsallclean.
Every load-bearing claim in this PR was mutation-checked, not asserted:
Dropinert fails four tests across both modulesRejectionfails exactly the new message test, in both directionsThe enum commit's proof is that no test assertion changed — the only
test-module edit in it is a comment the refactor itself falsified.
Integration tests— n/a, no request-path behavior changedChecklist
Code Quality
cargo fmt--all-targets --all-features -D warnings)unwrap; the poisoning path usesunwrap_or_elseTesting
Documentation
Security
open it on a healthy service are now rejected at startup
Related Issues
None. Registry record:
docs/tech-debt/TD-2026-07-09.md(resolved).Additional Notes
Review depth. Three plan-review rounds ran before any code and two more on
the finished diff — 39 agents total, artifacts under
docs/code-reviews/. Thatwas not ceremony:
mis-sequencing that moved TD-08 out
hazard it cites elsewhere
Recorded as open, with triggers (in
session-03-round2.md) rather thansilently dropped: the
abandoneddisposition has no test because no metricsrecorder is installed under
cargo test; one pre-existing test is vacuous forits name; and commit
7d0513c's subject is 75 chars against the declared 72 —fixing that means rebasing branch history, so it is left as a merge-time call.
One false positive is recorded too — a confidently reported CRITICAL that
verification disproved. The difference between it and the real findings was the
verification step, not the plausibility of the claim.
PR size: large, but ~40% is review artifacts and ~a third of the code diff is
tests and doc comments. The behavioral change is one commit of roughly three lines.