fix(relay): take NIP-43 snapshot reconcile off the startup path

[TheSentinel454]

Summary

Before the relay opened its listener, it reconciled the NIP-43 membership snapshot for every active community. Each community costs two writer round trips, one after the other. On a production deployment with about 80k communities, this one step averaged 174s of a roughly 180s boot (max 324s). Pods in an availability zone farther from the writer took 3–4× longer than pods in the writer's zone.

The step doesn't need to block startup. The snapshots live in the shared database, and the replicas that are already running serve the same snapshots. A new replica has nothing of its own to repair before it serves. The sweep before bind did the same work as the background maintenance sweep; only the metric labels differed.

This PR:

• Removes the NIP-43 sweep that ran before bind.
• Moves the background loop into side_effects::spawn_nip43_membership_reconciler. Its first sweep now runs as soon as the task starts. Before, the loop skipped its first tick, so the first maintenance sweep waited a full BUZZ_NIP43_RECONCILE_INTERVAL_SECS.
• Runs that loop on one replica only. The replica holding the NIP43_RECONCILE_LOCK_KEY session advisory lock sweeps; the others retry the lock every NIP43_STANDBY_RETRY (5s, capped at the interval). This reuses the usage-metrics leader lease (a detached Postgres session), renamed from UsageMetricsLeader/try_lock_usage_metrics to the generic SessionLeader/try_lock_session_leader.
• Waits a full interval after each sweep (sleep after sweep), so a sweep longer than the interval never runs straight into the next one.
• reconcile_nip43_membership_snapshots is now the only entry point and keeps its original signature (-> Result<usize>). It's no longer deprecated.

What got simpler

Deleted:

• Nip43ReconciliationPurpose (the Bootstrap/Maintenance purposes) and reconcile_nip43_membership_snapshots_with_purpose
• Db::bootstrap_community_hosts and Db::nip43_membership_snapshot_needs_reconciliation_for_bootstrap
• The startup progress log (NIP-43 startup reconciliation progress, nip43_progress_due) and its test
• The nip43_reconcile startup phase: StartupStep::ALL goes from 19 to 18 steps

The leader lock reuses the existing usage-metrics lease rather than adding a second leader primitive. StorageAccountingLeader is still a near-duplicate of it; folding that in is left as identified debt.

Trade-offs

• Repair before serving. A newly started replica no longer guarantees it has repaired stale snapshots before serving. The running fleet doesn't guarantee that today either: the background sweep is what repairs drift, and the leader starts it as soon as it takes the lock.
• Rollout load. Before, every replica swept before binding, and every replica also ran the background loop. Now there is one sweeper in steady state, so writer load from this job drops from one sweep per replica to one per deployment, and rollouts no longer stack boot sweeps. Standbys add one lock attempt every 5s each.
• Leader handoff. When the leader's process exits (including SIGKILL), its connection closes, Postgres releases the session lock, and a standby takes it within NIP43_STANDBY_RETRY (5s) and sweeps at once. Before this, a standby waited a full interval (3600s in production), so after a rollout replaced the leader nothing swept for up to an hour. If the leader is lost with its host or network instead, the lock is held until the database server's TCP keepalive drops the session, and nobody sweeps meanwhile. The usage-metrics leader already has the same exposure. The lease is checked only between sweeps, so a leader that loses its session mid-sweep finishes that sweep while the new leader starts one; the per-community publication lock keeps that overlap safe.
• Snapshot window. A missing or stale kind:13534 snapshot is served until the leader's sweep reaches that community; before, a booting replica blocked until it was repaired. Admission reads canonical relay_members, so this window doesn't change who's admitted. It does affect clients that read the snapshot: desktop onboarding shows the membership-denied page to a member missing from it, the desktop/mobile member lists and join alerts lag, and desktop mesh discovery keeps its existing allowlist when the snapshot is missing. In production the inline publications that keep it current have not failed since 2026-10-07, and the sweep repaired no snapshots in the last 30 days.
• Dashboards and alerts. The buzz_startup_phase_seconds{phase="nip43_reconcile"} series stops reporting. Anything charting or alerting on it should move to the sweep's existing counters (buzz_nip43_membership_reconciliations_total, buzz_nip43_membership_reconciliation_failures_total).

Related issue

None found.

Merge order

#8468 is stacked on this PR. Both remove a startup phase, and with the old fixed-length StartupStep::ALL array, merging both would have built a 17-entry [Self; 18] that doesn't compile. Merge this PR first; #8468 makes ALL a slice.

Testing

• background_reconciler_sweeps_immediately (private scratch database): spawns the production reconciler with a 24h interval and 24h standby retry, and requires the first sweep to publish a missing snapshot. Fails if the skipped first tick is restored.
• background_reconciler_standby_takes_over_on_short_retry (private scratch database): with a 24h interval and 100ms standby retry, nothing publishes while another session holds the leader lock; once it's released the standby must publish within 10s. Fails if the standby sleeps the full interval again (checked by reverting that line).
• test_usage_metrics_lock_has_single_owner_and_releases_on_drop covers the renamed lease.
• maintenance_sweep_skips_archived_and_deleted_communities now calls the single entry point.
• Runtime E2E at 574b28b3: two real relays sharing one database, 3600s sweep interval, production 5s standby retry. While the leader was alive, the standby made only lock attempts (no sweep or snapshot-comparison reads). SIGKILL of the actual lock holder, 3/3 trials: the survivor took the lock in 4.81–4.86s and repaired the snapshot within 10ms after that. Both replicas served authenticated reads while the initial publication was stalled.
• Blox, at 574b28b3: just fmt-check clippy security-review-check file-size-check passes. Every test-unit step and just test (118 suites) passes except the buzz-acp git_bootstrap native-git tests, which fail the same way at base b6aa5539 on Blox (environment, not this change).

[github-actions]

🔐 Codex Security Review

Note: This is an automated, security-focused review generated by Codex.
Use it as a supplement to human review; false positives are possible.

Scope

• Exact PR diff: 6e6173acd58029bac800583c4cc7482dafed693a...574b28b3347f46440fed2c3ec695212c4c8b54c5
• Model: gpt-6.1-sol

💡 Click "edited" above to see earlier reviews for this PR.

---

Review Summary

Overall Risk: NONE

No concrete security, correctness, or reliability findings identified in the authorized PR range after tracing membership authorization, snapshot publication, tenant scoping, and leader lifecycle.

Findings

No concrete security, correctness, or reliability findings were identified.

Notes

• No additional limitations were reported.

---

_{Generated by Codex Security Review |
Requested by: @TheSentinel454 |
Workflow run}

[loganj]

Reviewed at 13af7cad7. The change looks correct to me. I found no blocking problems.

What I checked

• The sweep that ran before bind and the background sweep did the same work. Only the WriterOperation label and the progress log were different. So removing the startup copy does not lose any repair.
• tokio::time::interval completes its first tick at once. Without the extra interval.tick().await, the first sweep runs as soon as the task starts. The new test proves this with a 24h interval.
• No other code in the tree uses the deleted items (Nip43ReconciliationPurpose, reconcile_nip43_membership_snapshots_with_purpose, bootstrap_community_hosts, ..._for_bootstrap, the nip43_reconcile phase). ReconcileSummary is still used by the large-roster reconcile, so it should stay. Other bootstrap callers still use WriterOperation::Bootstrap, so that variant should stay too.
• CI runs the new #[ignore] Postgres test (postgres-test-run.sh uses --run-ignored ignored-only), and PostgreSQL Tests passed at this head.

1. Non-blocking: boot sweeps will overlap during a rollout. The trade-offs section says "Rollouts replace one pod at a time, so these sweeps don't stack." That was true when the sweep held up readiness, because the next surge pod could not start until the sweep finished. After this PR a pod becomes available after about 10s boot plus minReadySeconds: 60. With maxSurge: 1, maxUnavailable: 0, the next pod starts about every 70s. One sweep takes 174s on average and up to 324s. So 3 to 5 full sweeps run at the same time on the writer during a rollout, and the old pods' own periodic sweeps run as well.

The per-sweep load is probably acceptable: each sweep is sequential and uses one connection at a time. But the PR text should say that sweeps overlap. If you do not want the overlap, the cheap fix is a pg_try_advisory_lock around each sweep, like try_lock_usage_metrics. Then a pod skips its sweep when another pod is already sweeping. This is fleet-wide work, so one sweeper at a time is enough.

2. Optional, existing behavior: missed-tick bursts. Now that spawn_nip43_membership_reconciler owns the loop, this is a good place to set interval.set_missed_tick_behavior(MissedTickBehavior::Delay). With the default Burst behavior and the default 60s interval, a 174s sweep is followed at once by catch-up ticks. On a large deployment with default settings, the sweep then never stops. bb-public sets 1h, so it is not affected. This problem existed before this PR and does not block it.

Residual risk (no change needed): a single-replica deployment has no peer that already serves repaired snapshots. For such a deployment, a missing kind:13534 snapshot can now be read for the length of the first sweep. That was the reason the sweep ran before bind. I think the trade-off is acceptable, but a single-replica operator would notice it.

[loganj]

One more item from reviewing #8468: this PR and #8468 both change StartupStep::ALL from [Self; 19] to [Self; 18]. Git merges the two cleanly (I checked with git merge-tree), and the result has 17 entries in an [Self; 18] array, so main will not compile. main does not require branches to be up to date, so whichever PR merges second needs a rebase to [Self; 17] and a fresh CI run before merge. Details: #8468

[wpfleger96]

🤖 Reviewed at 13af7cad7. Thanks for this one. The change is correct and I found nothing that blocks it.

Checked:

• The startup sweep and the background sweep did the same work. Only the WriterOperation label and the progress log differed, so deleting the startup copy loses no repair.
• Admission still reads relay_members directly (is_relay_member, get_relay_member). Serving before the first sweep finishes changes what clients see in kind:13534, not who gets in.
• Overlapping sweeps are safe. publish_nip43_membership_list holds the per-community advisory lock across read, build and write, and the sweep only republishes when the snapshot differs.
• background_reconciler_sweeps_immediately passed in the PostgreSQL Tests job at this head.

Agree with @loganj on the three items in his review: the trade-offs text should say boot sweeps now overlap during a rollout, the reconciler loop should set a missed-tick behavior, and whichever of this PR and #8468 merges second needs StartupStep::ALL rebased to [Self; 17]. The missed-tick fix and a doc note are inline.

On the advisory-lock option: if the pod holding the lock is an old pod that gets terminated mid-sweep, every other pod has already skipped, and nobody sweeps again for a full interval (1h on bb-public). If you adopt it, retry the lock on a short backoff instead of skipping until the next tick. Fixing the PR text is enough on its own.

Related, not caused by this PR: desktop doesn't recover when it reads a missing snapshot. When the membership lookup returns snapshotFound: false, Settings hides the member and invite controls and shows "Relay recovery may still be in progress", but nothing ever reads the snapshot again. The live subscription in useCommunityJoinAlerts only mounts once an owner or admin role is known, the lookup in community-members/hooks.ts has no refetch interval, window-focus refetch is off globally, and "Try again" only appears on a query error. So once the sweep publishes the snapshot, an open Settings view stays without controls until it remounts. This already happens today whenever a membership change commits but its snapshot publish fails, which lasts until the next periodic sweep. In a multi-replica fleet, this PR doesn't add exposure, since the old pods serve the same missing snapshot while the new one boots. It only adds exposure for single-replica deployments. Worth a separate desktop fix: retry the lookup on a short backoff while the snapshot is missing.

[TheSentinel454]

Thanks @loganj and @wpfleger96. What changed at 77c4e96:

• Overlapping boot sweeps / advisory lock: went with a leader. Only the replica holding a session advisory lock (NIP43_RECONCILE_LOCK_KEY) sweeps; this reuses the usage-metrics leader lease, renamed to the generic SessionLeader. The lock lives for the leader's whole session, not per sweep, so when the leader's process exits (including SIGKILL) its connection closes, the lock is released, and a standby takes over on its next attempt, at most one interval later. If the leader is lost with its host or network instead, the lock is held until the database server's TCP keepalive drops the session, and nobody sweeps until then; the existing usage-metrics leader has the same exposure. Since the sweep only repairs drift in derived snapshots, either delay is acceptable. (Edited at 8c0c9ec to narrow the handoff claim; the code comment and PR body say the same.) New test: background_reconciler_sweeps_only_while_holding_leader_lock.
• Missed ticks: sleep a full interval after each sweep (inline thread).
• Trade-offs text: rewritten for leader-only sweeps, plus the snapshot-window sentence.
• StartupStep::ALL: fix(relay): delete the startup NIP-33 d_tag backfill #8468 is now stacked on this PR and makes ALL a slice. Merge this one first.
• Desktop not re-reading a missing snapshot: agreed, that predates this PR, so I'm keeping it out of scope here.

[wesbillman]

🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency) and Groot (product behavior) reviews.

No actionable source defects found at head 8c0c9ec against base/merge-base b6aa553. The startup call no longer awaits reconciliation, the first background attempt is immediate, and subsequent attempts sleep after completion. Per-community transaction locking still serializes canonical membership reads and snapshot replacement. Mid-sweep leader-session loss can briefly overlap sweeps, but the source does not establish a correctness regression; strict fencing during a sweep would be an optional follow-up.

Optional description clarification: “only affects what’s advertised” also includes desktop roster/membership displays (desktop/src-tauri/src/commands/relay_members.rs:46–101) and mesh discovery’s member filter (desktop/src-tauri/src/commands/mesh_llm.rs:189–210). A missing snapshot makes mesh discovery return an error and retain its existing allowlist; a stale snapshot supplies stale membership inputs. Naming those consumers would make the accepted snapshot-repair delay clearer. This is not a demonstrated relay-admission bypass or a requested redesign.

Validation: source inspection only; no tests, builds, relay/database execution, fault injection or device workflows were run by this review. One CI snapshot at this head showed 57 successful and 30 skipped checks. The PR body still describes its runtime exercise as pre-leader and its local test run as pending, so current-head leader-loss/failover, load and operational timing remain unverified here. Public PR description inspected; no attached images. This COMMENT is not an approval or merge authorization.

[wpfleger96]

🤖 Automated review of 8c0c9ecb7bce314a48dd5f522c0350c6261a4452 (base b6aa5539e3a44fe32a4bfe2963dfb23f10316985): one blocking issue.

Thanks for the leader change. The rename is complete: run_usage_metrics_tick behaves exactly as before apart from the two renamed symbols, and is_live() is unchanged. NIP43_RECONCILE_LOCK_KEY doesn't collide with any other fixed advisory-lock key in the tree. Lock and sweep errors both fall through to the sleep, so nothing can spin or end the loop. Per-community publication still holds its advisory lock across read, build and write. The hosted PostgreSQL Tests job reports both new reconciler tests, the active-community sweep test and the renamed lease test as PASS, and removing the leader.is_some() gate would make the leader test publish during the held-lock phase and fail.

Blocking: after the leader exits, nobody retries the lock for up to a full interval

A replica that doesn't get the lock returns Ok(None), skips the sweep, and sleeps the whole every before trying again (crates/buzz-relay/src/handlers/side_effects.rs:3440-3463). Nothing gives standbys a shorter retry. During a rollout the leader is an old pod. New pods try the lock at boot, fail while it's alive, and go to sleep. When the old leader exits, the fleet has no sweeper until the earliest standby wakes. With BUZZ_NIP43_RECONCILE_INTERVAL_SECS=3600, which bb-public uses, that gap can approach an hour, and a leader killed mid-sweep leaves the rest of its communities unrepaired for that time too. My earlier review asked for exactly this: "If you adopt it, retry the lock on a short backoff instead of skipping until the next tick." The PR body describes an interval-long handoff and accepts it, but that's a different choice.

Fix: while not leader, retry the lock on a short bounded delay, and keep every between completed sweeps. Add a test through spawn_nip43_membership_reconciler with a long every: hold the lock through the standby's first attempt, release it, and require the snapshot within the short retry budget. The current leader test uses every = 100ms (side_effects.rs:4444-4459), so it can't tell a prompt takeover from waiting a full interval.

Optional

• The leader checks the lease only before a sweep (side_effects.rs:3434-3453). The sweep runs on pooled connections, so if the lease backend is terminated or Postgres restarts mid-sweep, a standby can take the lock while the old sweep finishes. That's safe, because publication is still serialized per community, and it's less load than base, where every replica swept. But the doc comment at side_effects.rs:3415-3420 ("Only the replica holding the lock_key session advisory lock sweeps") and the body's one-sweep-per-deployment claim should say "one sweeper in steady state; losing the lease can overlap sweeps until the old one finishes." An is_live() check every N communities that stops the sweep would close it, if you want it.
• "It now starts right after boot" is true for the first lock attempt, but a sweep only starts if that attempt wins. Once the retry fix is in, the body can say how fast a standby takes over.
• background_reconciler_sweeps_immediately uses the shared test database. maintenance_sweep_skips_archived_and_deleted_communities sweeps every active community in that database, and in the hosted run they started 20ms apart. A concurrent sweep can publish the snapshot this test waits for, so restoring the skipped first tick may not turn it red. Running it in a private database, like the leader test, would fix that. Pulling the scratch-database setup into a helper would let both tests share it.
• The leader test says "Twenty lock attempts' worth of standby" but sleeps every * 20 without counting attempts. Removing only the is_live() demotion check isn't caught by any test.
• The Testing section still marks the runtime E2E as pre-leader and the local just runs as pending. The description doesn't include buzz-review-completed.

[TheSentinel454]

Thanks @wpfleger96. You're right about the standby retry. Fixed at 574b28b:

• Standby retry: a replica that doesn't hold the lock now retries it every NIP43_STANDBY_RETRY (5s, capped at every). The leader still waits every between sweeps, so after the old leader's session ends a standby takes over within 5s and sweeps right away.
• Test: background_reconciler_standby_takes_over_on_short_retry replaces the leader test. It uses a 24h every and a 100ms retry, holds the lock through about 20 standby retries (nothing may publish), then releases it and requires the snapshot within 10s. I confirmed it fails when the standby sleeps every again.
• Doc comment and PR body: both now say one sweeper in steady state. If the leader loses its session mid-sweep, that sweep can overlap with the new leader's until it finishes, and the per-community publication lock keeps that safe.
• Shared test database: background_reconciler_sweeps_immediately now runs in a private scratch database too. Both tests share one ScratchDb helper.
• Not done: there's still no test that pins the is_live() demotion check alone.
• Testing section: updated. Full local runs at this head are in progress.

#8468 is rebased onto this. Its own commits are unchanged.

[wesbillman]

🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/concurrency) and Groot (product behavior) lanes.

No actionable source defects found at head 574b28b against base/merge-base b6aa553. This follow-up checks the actual changes since reviewed head 8c0c9ec and their surrounding call paths.

The standby uses min(standby_retry, every), while a leader retains the full post-sweep interval. Acquisition success enters the sweep immediately; unsuccessful acquisition returns the pooled connection. The production caller supplies a five-second retry and clamps the sweep interval to at least one second. The existing per-community transaction locks before reading canonical members and retains that lock through snapshot replacement, preserving serialization when sweeps overlap. The isolated tests use a day-long interval and short observation budgets to distinguish immediate startup and standby takeover from a full-interval sleep.

Source assessment: minimalism 9/10, elegance 9/10, correctness 9/10; runtime validation remains separate.

The description now names the snapshot-dependent clients and honestly distinguishes earlier live failover evidence from this revision. Public description inspected; no attached images.

Validation: source inspection only; no PR code, tests, builds, database operations or application workflows were executed. A single current-head CI snapshot showed 46 successful, 30 skipped and four in-progress checks. Current-head live failover, database load, deployment settings and client recovery remain unverified by this review. This COMMENT is not approval or merge authorization.

[TheSentinel454]

@wpfleger96 the standby-retry fix from your review is in at 574b28b33: a standby now retries the lock every 5s, so after the leader exits, another replica takes over within about 5s instead of waiting up to an hour. At that head, fmt, clippy and the full just test suite pass. The only failures are the two buzz-acp git_bootstrap native-git tests, which fail the same way at the base commit in that environment. I've resolved both inline threads.

@wesbillman the Trade-offs section now lists the clients that read the snapshot: desktop onboarding, the member lists and join alerts, and mesh discovery.

Follow-up: removing this sweep. A separate PR will write the kind:13534 member list in the same transaction as each membership change. That covers add, claim, invite, remove, role change, owner bootstrap and transfer, allowlist backfill, and community creation. With the list always committed alongside the member rows, it can't drift, so that PR deletes the reconciler, the leader lock, the standby retry, BUZZ_NIP43_RECONCILE_INTERVAL_SECS and the reconcile metrics. Merge order is this PR, then #8468, then the follow-up.

[wpfleger96]

🤖 Automated review of 574b28b3347f46440fed2c3ec695212c4c8b54c5 (base b6aa5539e3a44fe32a4bfe2963dfb23f10316985): no blocking issues.

Thanks for the quick turnaround. This pass covers the one commit since 8c0c9ecb.

The blocker from my last review is fixed. A replica without the lock now sleeps min(NIP43_STANDBY_RETRY, every), the leader still sleeps every after each sweep, and a leader that fails its lease check retries the lock at once and then drops to the short delay (crates/buzz-relay/src/handlers/side_effects.rs:3441-3476). Nothing can spin: production passes a fixed 5s retry and main.rs clamps the interval to at least 1s. A standby that loses the race returns its connection to the pool and logs nothing (crates/buzz-db/src/store/usage.rs:615-631), so each standby adds at most 12 lock queries a minute.

Both reconciler tests now catch the regressions they're named for, checked by running them against modified code at the exact head. Putting the full-interval standby sleep back makes background_reconciler_standby_takes_over_on_short_retry fail at its takeover assertion. Sleeping before the first lock attempt makes background_reconciler_sweeps_immediately fail at its immediate-sweep assertion. Both pass again once the code is restored. Moving the immediate-sweep test into its own database closes the shared-database gap from last time. The hosted PostgreSQL Tests job lists both as PASS.

Optional

• The doc comment (side_effects.rs:3420-3422) and the description say a standby takes over "within" 5s. The 5s bounds the sleep between attempts. The attempt after it still waits for a writer connection and the lock query, so under database pressure takeover can take longer. Something like "retries at least every 5s; takeover also waits on a writer connection" would be exact.
• No test pins the leader's own every cadence. A leader that slept the standby delay between sweeps would pass both tests. As you noted, the is_live() demotion check also has no test of its own.
• spawn_nip43_membership_reconciler accepts zero durations. No caller passes one, but a debug_assert! or a doc line would keep a future caller from writing a hot loop.
• The description doesn't include buzz-review-completed.