Expire circuit breaker state

[dahlia]

Why this is needed

Fedify 2.3.0 introduced per-host circuit breaker state, but those entries were written without a TTL. That made sense for the immediate delivery logic, where the state is just a small record of recent failures, but it gave old entries no natural way to disappear when a remote host never recovered. Over time, a busy server could keep circuit breaker keys for hosts it no longer needed to track.

This PR treats circuit breaker state as soft state. The default failure policy has a bounded time window, so Fedify can derive a safe retention period from the failure window, recovery delay, and held activity TTL. Custom failure callbacks are different: Fedify cannot inspect how far back they want to look, so those states stay unexpired unless the application opts in with stateTtl.

Closes #916.

How the fix works

New circuit breaker writes now include a TTL when the normalized options have one. The stored value also gets a small internal version marker. Public reads still return the same CircuitBreakerKvState shape, so the marker stays an implementation detail rather than becoming part of the API.

The legacy cleanup path is deliberately conservative. Fedify 2.3.0 and 2.3.1 may already have valid open or half-open circuits in the configured KV store, so the migration does not delete every unversioned value. It parses valid legacy state and rewrites it with the internal marker and TTL. Malformed values under the circuit prefix are removed only when a CAS check confirms that they have not changed since listing.

The sweep is guarded with a marker under the circuit prefix. That avoids two upgraded workers racing through the same prefix, and it leaves room for a second sweep after a short grace window so rolling deployments can catch no-TTL state written by still-running old workers. If another worker is already sweeping, the current delivery path skips the migration instead of waiting behind a full prefix scan. The migration is useful cleanup, not a reason to stall outbound delivery.

Stores without CAS are left out of the legacy sweep. They still get TTLs on new state writes, but they cannot safely migrate or delete legacy entries after a prefix listing without risking stale writes. The non-CAS path therefore avoids extra marker writes on every circuit breaker operation.

Tests

The regression tests in packages/fedify/src/federation/circuit-breaker.test.ts cover TTL writes with and without CAS, custom failure policy retention, legacy state migration, rolling-upgrade retry behavior, transient sweep failures, and the case where another worker already holds the sweep marker. The middleware tests that seed circuit state now mark the migration as complete so they keep testing the intended delivery paths.

I verified the branch with:

deno test --config deno.json --allow-env packages/fedify/src/federation/circuit-breaker.test.ts
mise run check
git diff --check

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 08623ae0-3c2a-4c20-a67b-49e068d85d11

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[dahlia]

@codex review

[gemini-code-assist]

Code Review

This pull request introduces a TTL for outbound delivery circuit breaker states in the key-value store to prevent remote host states from being retained indefinitely. It also implements a legacy sweep mechanism to automatically clean up stale, no-TTL states from previous versions. The review feedback highlights a critical performance and reliability issue: the legacy sweep is currently executed synchronously on every outbound delivery path, which can block active requests and propagate transient database errors. The reviewer recommends running the sweep asynchronously in the background and exposing a helper to await its completion in unit tests.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c1ac7e8bd6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[coderabbitai]

Actionable comments posted: 4

♻️ Duplicate comments (1)

packages/fedify/src/federation/middleware.test.ts (1)

83-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate of markLegacySweepDone in circuit-breaker.test.ts.

Same key array and payload as markLegacySweepDone in circuit-breaker.test.ts. Consider consolidating into a shared test helper to keep the sweep-meta key in sync across both files.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/fedify/src/federation/middleware.test.ts` around lines 83 - 91,
`markCircuitBreakerLegacySweepDone` duplicates `markLegacySweepDone` from
`circuit-breaker.test.ts` with the same sweep-meta key and payload, so
consolidate this into a shared test helper. Move the common helper to a shared
test utility and have both `markCircuitBreakerLegacySweepDone` and
`markLegacySweepDone` call it, keeping the
`_fedify/circuit/__fedify_meta/circuit_breaker_state_ttl_sweep_v1` key
definition in one place.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/fedify/src/federation/circuit-breaker.test.ts`:
- Around line 77-90: The migration test currently only verifies the rewritten
value shape and does not confirm that TTL is actually applied during the
legacy-state rewrite. Extend CountingSweepKvStore in circuit-breaker.test.ts to
capture write options by overriding set/cas and recording KvStoreSetOptions,
then update the “migrates legacy states without TTL once” test to assert the
migration write for stale-a.example and stale-b.example includes a TTL. Use the
existing CountingSweepKvStore and migration test block to locate the change.
- Around line 129-136: The `markLegacySweepDone` helper duplicates the same
sweep-marker logic used by `markCircuitBreakerLegacySweepDone` in
`middleware.test.ts`, including the magic key path and payload. Refactor this
test setup into a shared helper/utility that both tests import, and update
`markLegacySweepDone` to delegate to that shared function so the sweep meta key
only lives in one place.
- Around line 852-889: `CircuitBreaker.#sweepLegacyStates()` is currently
causing transient kv.list() failures to reject the main flow, so update the
`CircuitBreaker` sweep path to catch and log sweep errors instead of propagating
them from `beforeSend`, `recordFailure`, `recordSuccess`, and `getState`. Keep
the primary state transition logic running after a failed sweep, and use the
existing `CircuitBreaker`/`#sweepLegacyStates` symbols to locate the shared
sweep call site and error handling.

In `@packages/fedify/src/federation/circuit-breaker.ts`:
- Around line 512-516: The `#deleteIfUnchanged` flow can leave
LEGACY_SWEEP_DELETING_MARKER behind permanently if the delete step fails or the
worker stops between the CAS and delete. Update the `#deleteIfUnchanged` method in
circuit-breaker.ts so the this.#kv.cas call writes the marker with an explicit
TTL/expiration option, ensuring the temporary deleting state self-expires even
when `#kv.delete` is never reached. Keep the change localized to
`#deleteIfUnchanged` and reuse the existing LEGACY_SWEEP_DELETING_MARKER logic.

---

Duplicate comments:
In `@packages/fedify/src/federation/middleware.test.ts`:
- Around line 83-91: `markCircuitBreakerLegacySweepDone` duplicates
`markLegacySweepDone` from `circuit-breaker.test.ts` with the same sweep-meta
key and payload, so consolidate this into a shared test helper. Move the common
helper to a shared test utility and have both
`markCircuitBreakerLegacySweepDone` and `markLegacySweepDone` call it, keeping
the `_fedify/circuit/__fedify_meta/circuit_breaker_state_ttl_sweep_v1` key
definition in one place.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 19e226d1-703b-489e-88cd-b780b0cadbee

📥 Commits

Reviewing files that changed from the base of the PR and between 2d18272 and c1ac7e8.

📒 Files selected for processing (4)

• CHANGES.md
• packages/fedify/src/federation/circuit-breaker.test.ts
• packages/fedify/src/federation/circuit-breaker.ts
• packages/fedify/src/federation/middleware.test.ts

[dahlia]

/gemini review

[dahlia]

@codex review

[gemini-code-assist]

Code Review

This pull request addresses an issue where the outbound delivery circuit breaker retains per-host state in the key-value store indefinitely. It introduces a TTL on writes for the default failure policy, adds a new stateTtl option for custom policies, and implements a background legacy sweep to clean up or migrate older, no-TTL state. The review feedback highlights a potential issue where parsing the stored retryUntil timestamp using Temporal.Instant.from is not guarded by a try/catch block. If the stored value is malformed, it could throw a RangeError and disrupt the background sweep process. It is recommended to wrap these parsing calls in try/catch blocks to handle invalid dates gracefully.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7f0db201af

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[codecov]

Codecov Report

❌ Patch coverage is 98.67110% with 4 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
packages/fedify/src/federation/circuit-breaker.ts	98.61%	2 Missing and 2 partials ⚠️

Files with missing lines	Coverage Δ
...edify/src/federation/circuit-breaker-test-utils.ts	100.00% <100.00%> (ø)
packages/postgres/src/kv.ts	96.96% <100.00%> (ø)
packages/fedify/src/federation/circuit-breaker.ts	94.57% <98.61%> (ø)

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[dahlia]

@codex review

[dahlia]

/gemini review

[gemini-code-assist]

Code Review

This pull request addresses an issue where the outbound delivery circuit breaker retained per-host state in the key-value store indefinitely if a remote host never recovered. To resolve this, circuit breaker states are now written with a Time-To-Live (TTL) by default, and custom failure policies can opt-in using the new stateTtl option. Additionally, a background migration sweep has been introduced to automatically clean up or migrate legacy, TTL-less state entries written by previous 2.3 releases on CAS-backed stores. Comprehensive unit tests have been added to verify the new TTL behavior, custom policy options, and the legacy sweep migration logic. There are no review comments to address, and I have no further feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2c1efef678

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[dahlia]

@codex review

[dahlia]

/gemini review

[gemini-code-assist]

Code Review

This pull request resolves an issue where the outbound delivery circuit breaker retained per-host state indefinitely in the key-value store for hosts that never recovered. It introduces a TTL on writes made with the default failure policy, allows custom failure policies to opt in via a new stateTtl option, and automatically sweeps stale state written by earlier 2.3 releases on CAS-backed stores. The review feedback highlights two key improvements: first, replacing optional chaining with non-null assertions inside the legacy sweep loop to prevent potential infinite loops if the CAS guard is bypassed; second, adding a key length check during the sweep to defensively skip non-host keys under the prefix.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 38c3e7fffb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[dahlia]

@codex review

[dahlia]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces state TTLs for the outbound delivery circuit breaker to prevent per-host state from being retained indefinitely, along with a background legacy sweep mechanism to migrate or clear old no-TTL states. Additionally, the PostgreSQL key-value store was updated to refresh the created timestamp on updates to ensure correct TTL calculations. The review feedback highlights a potential deadlock if a worker crashes during a legacy sweep, leaving a stale sweeping marker, and suggests verifying if the sweep has timed out using LEGACY_SWEEP_LOCK_TTL.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7c66ba1874

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. 👍

Reviewed commit: d76e21daf9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[dahlia]

@nyanrus If you have some spare time, could you review this pull request? Thanks!

[sij411]

One small concern. it looks like malformed legacy sweep markers appear to permanently suppress the legacy cleanup sweep. i'm not sure this is intentional, if so maybe a short comment near this branch would help explain that malformed markers are deliberately treated as "do not sweep" rather than "retry cleanup."

[dahlia]

@sij411 Addressed in 1a352bf. Malformed legacy sweep markers are now treated as replaceable migration metadata, so cleanup can retry instead of being permanently suppressed.

[dahlia]

@codex review

[dahlia]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a state TTL option and a background legacy sweep mechanism to clean up stale, no-TTL circuit breaker states from previous releases. It also updates the PostgreSQL KV store to refresh the creation timestamp on upserts. Feedback highlights a potential database syntax error in the PostgreSQL KV store when stringifying a Temporal.DurationLike object directly, and identifies a mathematically impossible conditional block in the legacy sweep logic that results in dead code.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1a352bf84b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[dahlia]

@codex review

[dahlia]

/gemini review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: e77306d70c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[gemini-code-assist]

Code Review

This pull request introduces automatic expiration (TTL) for outbound delivery circuit breaker states in the key-value store to prevent retaining per-host state indefinitely. It adds a stateTtl option for custom failure policies, automatically derives a default TTL for the default numeric policy, and implements a background migration sweep to clean up or migrate legacy, no-TTL states from earlier 2.3 releases. Additionally, the PostgreSQL KV store implementation is updated to refresh the created timestamp on upsert to correctly calculate TTL expiration. No review comments were provided, so there is no additional feedback to address.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[nyanrus]

Thank you for taking #916 seriously — this is a far more thorough fix than the one-line TTL I sketched in the issue. I traced the concurrency paths and ran the suite locally (deno test … circuit-breaker.test.ts → 24/24 green, typecheck clean). The core is sound: new writes carry a TTL, the marker/version scheme keeps the public CircuitBreakerKvState shape stable, and the migration is idempotent and CAS-guarded, so I couldn't find any way to corrupt or lose live circuit state. Nice work preserving open/half-open circuits instead of wiping the prefix.

The notes below are not correctness bugs — the suite is right to be green. They're about the robustness and cost of the legacy sweep on exactly the deployments it's meant to help: the servers whose circuit prefix already grew large under 2.3.0/2.3.1. I modeled the marker protocol to be sure, so I'll show the evidence rather than just assert.

(Re-checked against the current head, e77306d: the PR since gained a fix for malformed legacy-sweep markers and a Postgres Duration-like TTL normalization, plus a docs section on stateTtl for custom failure policies — none of which touch the three points below, which still stand at the line numbers linked here.)

---

1. The sweep's mutual exclusion weakens precisely on large stores (unheartbeated lock + unconditional completion set)

Two facts compound:

• The sweeping marker is written with { ttl: LEGACY_SWEEP_LOCK_TTL } (5 min) in #acquireLegacySweep but is never refreshed during the pass in #sweepLegacyStatesImpl. On any TTL-honoring backend (sqlite/postgres/redis all honor ttl), the lock key simply disappears mid-pass once a list() + migrate pass runs longer than 5 minutes — which is likely on the multi-million-key prefixes this migration targets. A second worker then sees no marker, CAS-claims it, and starts another full list() from the top.
• The completion write is an unconditional this.#kv.set(markerKey, finishedMarker), not a CAS. A worker that claimed the lock from an expired marker always carries a future retryUntil, so #finishLegacySweepMarker hands it back a done marker — and its unconditional set can overwrite a final marker a faster peer already wrote, resurrecting the retry cycle.

I ported the marker protocol (acquire / finish / the isLegacySweep{Active,InProgress,Done,RetryDue} predicates) to a small Go harness with a TTL-honoring in-memory KV and 4 workers, with the pass deliberately outlasting the lock (then shrinking, to model entries aging out). Result:

full sweeps executed   : 11    (ideal: 1, or 2 incl. the scheduled retry)
~entries touched total : 1615  (~10× inflation from redundant re-lists)
final markers written  : 3
final -> done resurrect: 4     (a slow pass overwrote a peer's `final`)
terminal marker state  : "done"   (never durably terminated within the window)

It does self-limit — once the store drains enough that a pass fits inside 5 min, a single clean pass writes final and it settles. But on the worst-hit stores you can get many redundant full-prefix scans and several 7-day retry cycles before that happens. This is the same asymmetry as issue #916 itself: the mechanism is weakest exactly where the leak is biggest.

Smallest fix I can see: make the completion a CAS against our own sweeping marker (so a worker that lost the lock doesn't stomp a peer's final), and heartbeat the lock (re-set the sweeping marker with a fresh TTL every N entries), or bail the pass early if the marker is no longer ours. Happy to open a follow-up PR for this if it's useful.

2. kv.list(this.#prefix) materializes the whole circuit prefix in memory

The sweep streams with for await (… of this.#kv.list(this.#prefix)) (here), but the bundled stores don't stream: @fedify/sqlite's list does .prepare(…).all(now) — synchronous, so it materializes every matching row and blocks the event loop while doing so; @fedify/postgres awaits the full result array. Neither has a LIMIT/cursor, and KvStore.list(prefix?) exposes no pagination to chunk through. On a store with millions of leaked circuit keys — the target case — the first sweep loads the entire prefix at once (and on sqlite, stalls the loop synchronously). This is also the first list() consumer over an unbounded, host-cardinality prefix; the existing callers (otel/exporter.ts) list bounded prefixes, so this hasn't bitten before.

No clean fix from inside circuit-breaker.ts given the interface. Options are a cursor/limit on KvStore.list (streaming the sqlite/postgres impls), or leaning on plain TTL going forward + a bounded, lazy migration instead of an eager full scan.

3. Custom failure policies run the full sweep but reclaim nothing (and still leak)

For a custom failure callback with no stateTtl, stateTtl stays undefined (normalize), so both new writes and migrated writes go out without a TTL (#setOptions). But #sweepLegacyStates doesn't check stateTtl — it runs anyway and rewrites every legacy entry from unversioned-immortal to versioned-immortal. So a custom-policy deployment pays the full scan/rewrite cost (incl. point 1 and point 2 above) for zero reclamation, and remains leaked. The new docs section now tells custom-policy users to set stateTtl themselves, which helps — but doesn't change the sweep's behavior. Two small touches would still help: skip the sweep entirely when stateTtl == null, and warn once at construction when a custom failure is set without stateTtl, so those who miss the docs still find out.

Minor

• Reclaim latency. Most Circuit breaker never expires its per-host KV state, so a dead/unresponsive host leaks one KV entry forever #916 leakage is closed state with a short failure list (a few failures never cross the threshold), so migration rewrites them with a ~7-day TTL rather than freeing them — full reclamation lands ~stateTtl after the sweep. Deleting closed entries outright (missing == closed) would reclaim immediately; the trade-off is dropping in-flight failure accumulation, which is why rewriting is defensible. The dominant cost is the list() in point 2 either way, so this is low.
• Unbounded retry loop. #acquireLegacySweep is while (true) with only a 10 ms delay; every other retry loop in the file caps at 10 attempts. It converges on a strongly-consistent store, but an eventually-consistent backend that can't read-its-own-writes could spin in the background indefinitely. A cap (best-effort return) would match the rest of the file.

---

None of this blocks the fix in my view — the TTL-on-write change already closes the default-policy leak, which is the main thing. Point 1 and point 3 are the two I'd weigh most, since they land on the large/custom deployments that most need the cleanup. I'm glad to send follow-up patches for point 1 (CAS completion + heartbeat) and point 3 (gate + warn) if you'd like.

_{Reviewed by Shiro (Claude Opus 4.8), an AI assistant working with @nyanrus — I filed #916. I traced these paths and ran both the suite and the Go marker-protocol model myself; if I've misread the sweep's intent anywhere, please tell me and I'll correct it.}

[dahlia]

@nyanrus Thanks for the detailed review. I addressed the actionable parts in ebf154c: the legacy sweep now skips custom failure policies without a state TTL, finishes the sweep marker with CAS instead of an unconditional write, and bounds marker acquisition retries so the best-effort cleanup does not spin indefinitely under contention.

[nyanrus]

A few more things I noticed while re-reading the sweep after ebf154c8 — none of these block anything, just flagging for visibility.

@fedify/postgres and @fedify/redis don't implement cas(), so the legacy sweep never runs for them

#sweepLegacyStates returns immediately when this.#kv.cas == null. Checking PostgresKvStore and RedisKvStore, neither defines a cas method at all (only set/delete/list, plus initialize/drop for postgres) — KvStore.cas is optional, so this compiles fine, it's just never exercised for these two backends. That matches what the PR description already says ("stores without CAS are left out of the legacy sweep"), so this is a known, intentional tradeoff, not a bug. But it means for two of Fedify's own bundled, production-grade adapters, any circuit-breaker state written without a TTL under 2.3.0/2.3.1 (the actual #916 leak) will never be migrated or cleaned up — only brand-new writes get a TTL going forward. The CHANGES.md entry says stale state "is cleared automatically on CAS-backed stores after upgrade," which is accurate but easy to misread as "on upgrade" full stop. Might be worth a line in docs/manual/circuit-breaker.md telling @fedify/postgres/@fedify/redis users that pre-upgrade leaked entries need a manual cleanup (e.g. a one-off script or TTL added directly), since the automatic path silently doesn't apply to them.

kv.list() doesn't stream in the two bundled stores that matter most for this migration

The sweep does for await (... of this.#kv.list(this.#prefix)), but @fedify/sqlite's list() does .prepare(...).all(now) — synchronous, so it materializes every matching row and blocks the event loop while doing so — and @fedify/postgres's list() awaits the full result array before yielding anything. Neither has a LIMIT/cursor, and KvStore.list(prefix?) has no pagination to chunk through. On a store with the large legacy-circuit-key backlog this migration specifically targets, the first post-upgrade sweep loads the entire prefix into memory at once (and on sqlite, stalls the loop synchronously doing it). Not something fixable from inside circuit-breaker.ts given the current interface — would need a cursor/limit on KvStore.list to really fix.

Dead code: isEqualKvKey(key, markerKey) never has anything to catch

isEqualKvKey(key, markerKey) — markerKey is [...prefix, "__fedify_meta", "circuit_breaker_state_ttl_sweep_v1"], always two segments so always prefix.length + 2 long, while the very next line only lets keys of length prefix.length + 1 through. Any key isEqualKvKey would match is already excluded by that length check, so it's dead regardless of the comparison result — and it's the only call site of isEqualKvKey (defined here) in the file, so both the check and the helper could go. Not a behavior change either way.

Minor

• New shared test helper lives outside the documented testing-utilities locations. packages/fedify/src/federation/circuit-breaker-test-utils.ts is imported from both circuit-breaker.test.ts and middleware.test.ts, so it's cross-file shared test infrastructure, but this repo's CLAUDE.md says to use packages/testing/ or packages/fedify/src/testing/ for that.
• delay() is a new copy of a helper that already exists four times over: packages/denokv/src/mod.ts, packages/mysql/src/mq.ts, packages/sqlite/src/mq.ts, and packages/postgres/src/mq.ts all have the same new Promise((resolve) => setTimeout(resolve, ms)) one-liner.
• TTL Duration coercion is inconsistent across KV adapters. PostgresKvStore.set() now defensively does Temporal.Duration.from(options.ttl) before use, but MemoryKvStore, SqliteKvStore, and RedisKvStore all call .round()/.total() directly on options.ttl. Low risk given the type is declared as Temporal.Duration, but worth knowing they'd behave differently if a plain duration-like object ever reached them.

_{Written by Shiro (Claude Opus 4.8), an AI assistant working with @nyanrus. If any of these are off-base or I've misread something, let me know and I'll correct it.}

[nyanrus]

When I asked to review and fix with looping, bugs comes a lot, so I think the original structure would have something not for clean logic. Researching about alts

[dahlia]

@nyanrus Addressed your latest review feedback in efd6d01.

[nyanrus]

Two small follow-ups from the same review — both optional, and both shrink code rather than add it.

1. The legacy sweep only needs one claim key, not the marker state machine. The sweeping → done → final marker + lock TTL + retry window + completion CAS (~200 lines) coordinate N workers for what is a one-time, idempotent migration. Since #migrateLegacyState is already per-key CAS (re-runs are safe), the coordination collapses to a single cas(claimKey, undefined, claimed, { ttl }): whoever wins runs the pass, everyone else skips, and a crashed sweeper is retried once the claim lapses. On non-CAS stores (@fedify/postgres, @fedify/redis) the same shape can run the pass unlocked instead of returning early at :467 — which is what currently leaves those backends' 2.3.0/2.3.1 entries unreclaimed.

2. Throw when stateTtl < recoveryDelay. A shorter stateTtl expires the open record before opened + recoveryDelay, silently defeating the breaker for the gap. The derived default already keeps stateTtl ≥ recoveryDelay; a RangeError in normalizeCircuitBreakerOptions next to the existing assertPositiveDuration closes the override path, consistent with how the other durations are validated.

Both are drop-in for 2.3-maintenance. They're also a clean stepping-stone toward a smaller (failures, restrain-until) state model I've written up separately in #919 — the sweep driver and the TTL invariant carry straight over — but they stand on their own.

Drafted by Shiro (Claude Opus 4.8), working with @nyanrus; tell me if I've misjudged the sweep's retry-window intent.

[dahlia]

@nyanrus Addressed the stateTtl/recoveryDelay validation feedback in 873ba9b.

I left the sweep marker state machine in place for this PR. The done/final retry window is there to catch no-TTL states written by old workers during a rolling upgrade, and running the sweep unlocked on non-CAS stores would lose the per-key CAS guard that prevents overwriting state changed after listing. I think that simplification is better handled with the larger state model follow-up rather than in this maintenance fix.

[nyanrus]

Thanks for landing the TTL validation in 873ba9b — that closes the explicit-override case cleanly.

And agreed on leaving the sweep as-is here. You're right that running it unlocked on non-CAS stores drops the per-key CAS guard — I'd missed that a stale migrate-write could clobber a state change made after the list(), which skipping non-CAS deliberately avoids. Thanks too for confirming the retry window's rolling-upgrade purpose; that matches what I'd inferred. The claim-key simplification does belong in the larger state-model follow-up (#919), not this maintenance fix.

That's everything from my side: the corruption fixes and the TTL guard all look good, and the remaining items (non-CAS reclamation, list() streaming, the sweep reduction) are better tracked as follow-ups or in #919. LGTM — thanks for the careful back-and-forth on this one.

— Shiro (Claude Opus 4.8), working with @nyanrus