Circuit breaker: model per-host state as (failures, restrain-until) and derive the rest — removes the stateTtl footgun (design note, targets next)

[nyanrus]

Where

packages/fedify/src/federation/circuit-breaker.ts — the per-host record shape (CircuitBreakerKvState: state / failures / opened / halfOpened) and the stateTtl option.

Context

Not a bug report — #917 is correct and green, and this builds on it. While reviewing #916/#917 I wrote the circuit breaker out as a small timed state machine, and a few of its fields turn out to be derivable. Filing the observation in case it's useful for a future next cycle; entirely happy for it to be closed if the simplification isn't worth the churn.

The observation

Writing the decision logic (beforeSend :248, recordFailure :380) as a transition system, three things fall out:

1. opened is redundant. A circuit opens exactly at the failure that trips it, and failures are ignored while open (:387), so opened == maximum(failures) in every reachable state.
2. The record is advisory. Losing it only ever turns a hold into a send (beforeSend treats a missing record as closed, :269) — never a wrong delivery, never a drop. So its exact lifetime is a soft concern.
3. The correct TTL is derived, not free. A record can influence a decision only within max(recoveryDelay, failureWindow) of its last write (an open record must survive to opened + recoveryDelay; an accumulating one until its failures age out of the window). heldActivityTtl doesn't belong here — it governs the message-side circuitHeldSince (:258, :595), a separate persistence layer. Today stateTtl is a free knob that can be set below that floor, which is the root of the Circuit breaker never expires its per-host KV state, so a dead/unresponsive host leaks one KV entry forever #916-adjacent footgun: stateTtl < recoveryDelay silently defeats the breaker for the recoveryDelay − stateTtl gap.

Proposal (for next)

Persist only two fields per host — failures and restrainedUntil (nothing = closed); everything else (state, opened, halfOpened) is derived. In Julia-ish pseudocode:

# configuration — these are the existing CircuitBreakerOptions (defaults shown)
failureThreshold = 5           # this many failures trip the circuit
failureWindow    = Minute(10)  # failures older than this stop counting
recoveryDelay    = Minute(30)  # wait this long before allowing a probe
ttlMargin        = Second(5)   # margin for backend TTL granularity / clock skew

# per-host record — state (open/half-open), opened, halfOpened are all derived, not stored
mutable struct Circuit
    failures::Vector{Instant}                 # the failure timestamps that still count
    restrainedUntil::Union{Instant,Nothing}   # hold deliveries until this instant; nothing ⇒ closed
end

# drop failures outside the window [now-failureWindow, now], then keep at most the newest few
prune(failures, now) =
    last(failureThreshold, [f for f in failures if f ≥ now - failureWindow])

# tripped ⇔ enough failures exist AND the newest `failureThreshold` all fit in one window
tripped(failures) =
    length(failures) ≥ failureThreshold &&
    maximum(failures) - failures[end - failureThreshold + 1] ≤ failureWindow

function deliver(circuit, now)
    circuit.restrainedUntil === nothing && return :send   # not restrained ⇒ deliver (closed)
    now < circuit.restrainedUntil       && return :hold   # inside the restraint window ⇒ hold
    circuit.restrainedUntil = now + recoveryDelay         # restraint elapsed: open a fresh window…
    return :probe                                         # …and let one trial delivery through
end

function fail!(circuit, now)
    circuit.failures = prune([circuit.failures; now], now)  # record failure, drop stale ones
    if circuit.restrainedUntil !== nothing                  # was restrained ⇒ a probe just failed:
        circuit.restrainedUntil = now + recoveryDelay       #   re-arm restraint (half-open strike)
    elseif tripped(circuit.failures)                        # just crossed the threshold ⇒ open:
        circuit.restrainedUntil = now + recoveryDelay       #   restrain for recoveryDelay
    else                                                    # still under the threshold ⇒
        circuit.restrainedUntil = nothing                   #   stay closed, keep counting
    end
end

# any success wipes history and un-restrains ⇒ back to closed
success!(circuit) = (empty!(circuit.failures); circuit.restrainedUntil = nothing)

# a record can only affect a decision for max(recoveryDelay, failureWindow); +margin for clock slop
stateTtl = max(recoveryDelay, failureWindow) + ttlMargin

This reproduces the current decisions — including the half-open single-strike reopen that the failure history alone can't express: with recoveryDelay > failureWindow the tripping failures have aged out of the window by probe time (so tripped(failures) is false), yet restrainedUntil still carries the open-ness (the restrainedUntil !== nothing branch in fail!). I checked the equivalence two ways: an invariant induction (R's restrainedUntil is always ≥ the current design's, so the only divergence is a rarer, strictly-more-restrictive hold under a concurrent race — never a send where the current design holds), and a small simulation (realistic event streams agree exactly; adversarial race injection diverges ~1% of steps, all in the safe direction).

On the TTL floor. max(recoveryDelay, failureWindow) is tight only under an idealized single clock — the open record's expiry then lands exactly on the probe boundary opened + recoveryDelay. Real KV backends expire on their own clock at coarse granularity (Cloudflare KV is second-level, others differ), so the record can vanish a hair before beforeSend reaches the probe. That turns the round's would-be half-open probe into a plain send: if it fails, recordFailure sees no record and merely re-accumulates (tripped([now]) is false when failureThreshold > 1) instead of the single-strike reopen (:396). Bounded — it self-heals within failureThreshold / failureWindow — but not purely advisory. So in practice use stateTtl = max(recoveryDelay, failureWindow) + ttlMargin. Any stateTtl ≥ max(recoveryDelay, failureWindow) keeps the invariant; the current default recoveryDelay + max(failureWindow, heldActivityTtl) already sits well above it, which is why this boundary doesn't bite today.

What it buys

• The stateTtl footgun disappears by construction — the TTL is derived from recoveryDelay and failureWindow (plus a margin), not a knob that can undercut recovery.
• Smaller, more stable record (2 fields vs 4) → less migration surface across state-format versions.
• Fewer transition races — fail! / success! become set operations; no state / opened / halfOpened to keep mutually consistent under CAS.
• restrainedUntil does triple duty — the restraint check (now < restrainedUntil), the probe timing (now ≥ restrainedUntil), and the held-activity retry target (the current staleAt in capHeldRetryAt, :583) are all just restrainedUntil, which the current code spreads across opened / halfOpened / a recomputed staleAt.

Costs / risks

• Behaviour-equivalent only up to the advisory race slack above — a core state-model change, so next, not a maintenance branch.
• deliver computes tripped(prune(failures, now)) (O(failureThreshold)) instead of reading state.
• Decisions are equivalent, but two observable details are not, from (failures, restrainedUntil) alone. restrainedUntil cannot distinguish open from half-open (both are just now < restrainedUntil), so (a) onStateChange can't label the exact previousState on a strike, and (b) the held-activity retry cadence differs (open retries at opened + recoveryDelay; half-open retries every releaseInterval, :277 vs :311). Restoring full fidelity needs one extra probing flag — i.e. (failures, restrainedUntil, probing), still 3 fields vs the current 4. The two-field record is the minimal decision-equivalent form; adding probing is the minimal fully observable-equivalent one.
• Custom failure policies have no derivable time-TTL. Their pruning is count-based (slice(-100), :734), with no window, so a failure can persist unboundedly and there's no window to floor the TTL from — which is why the shipped code leaves stateTtl undefined (no TTL) for them today, an independent leak vector from Circuit breaker never expires its per-host KV state, so a dead/unresponsive host leaks one KV entry forever #916. A black-box predicate's lookback can't be inferred, so something has to be user-declared. The minimal fix is to require an explicit stateTtl for custom policies — turning today's silent leak into a clear error — which bounds the record without touching predicate semantics. Fully deriving the TTL would instead need a time bound on the history itself (a maxFailureAge), but that also time-bounds what the predicate sees, so it's a larger, semantics-changing move — better kept as its own change than folded into this reduction.

Relationship to #917

The two surgical changes I've suggested on #917 — a single claim key for the legacy sweep, and throwing when stateTtl < recoveryDelay — would be a light stepping-stone toward this: the sweep driver would carry over verbatim, and that throw would enforce the exact invariant this model makes structural. To be clear, neither has landed. The shipped normalizer still validates an explicit stateTtl with assertPositiveDuration only (positive, not ≥ recoveryDelay), so the #916 footgun is still open for explicit overrides today — which is part of why closing it by construction here is worth doing.

About this issue

Drafted by Shiro (Claude Opus 4.8), an AI assistant working with @nyanrus. The equivalence is my own analysis plus a simulation, not a mechanized proof — if I've misread how opened / halfOpened are meant to be used (surfaced to users somewhere, or load-bearing for a case I missed), please say so; that assumption is what the whole reduction rests on.