Skip to content

fix(swim): fast-restart rejoin — persist incarnation, echo refutations, ping liveness - #243

Closed
EnRaiha wants to merge 2 commits into
NodeDB-Lab:mainfrom
EnRaiha:fix/swim-fast-restart-rejoin
Closed

fix(swim): fast-restart rejoin — persist incarnation, echo refutations, ping liveness#243
EnRaiha wants to merge 2 commits into
NodeDB-Lab:mainfrom
EnRaiha:fix/swim-fast-restart-rejoin

Conversation

@EnRaiha

@EnRaiha EnRaiha commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Closes the SWIM fast-restart rejoin stick (epic #165, Medium #9): a node that crashes while peers hold Dead(A, N) restarts at incarnation 0, announces Alive(0) exactly once, and can diverge forever when the probabilistic refutation round-trip fails.

  • F4 (primary): persist local incarnation in the catalog (KEY_SWIM_INCARNATION); bootstrap resumes at persisted+1 so the first announcement dominates any lingering Dead rumour; every self-refutation bump persisted fire-and-forget.
  • F1: refutations echoed deterministically on Ack + forwarded PingReq piggyback.
  • F2: a ping IS liveness evidence — sender Alive claim applied before ack; self-advertise rate-limited to 500ms.
  • F3: fresh Alive apply cancels the pending suspicion timer (kills the re-kill race).

6 regression tests incl. deterministic recovery loop (A holds Dead(B,5) → echo → self-refute → converge Alive). Verification: swim 129/129, cluster lib 1023, clippy -D warnings 0, maya-gate L1 clean.

Closes #165 item: Medium — SWIM fast-restart rejoin.

…s, ping liveness

Fixes the SWIM fast-restart rejoin stick (epic NodeDB-Lab#165): a node that
crashes while peers hold Dead(A, N) restarts at incarnation 0,
announces Alive(0) exactly once, and can diverge from the cluster
forever when the probabilistic refutation round-trip fails.

- F4 (primary): persist the local incarnation in the catalog
  (KEY_SWIM_INCARNATION, u64 LE, same pattern as cluster_epoch).
  Bootstrap resumes at persisted + 1, so the first announcement
  already dominates any lingering Dead(A, N) rumour. Every
  self-refutation bump is persisted (fire-and-forget, never stalls
  the probe loop). New IncarnationStore trait: catalog-backed in
  production, in-memory in tests. Wired through SwimSubsystemConfig,
  spawn_with_subscribers, and register_default_subsystems (load +
  bump on start).
- F1: ingest_piggyback returns refutations — whenever an update is
  refuted, the stored (newer) view is echoed deterministically on the
  reply piggyback (Ack + forwarded PingReq), instead of relying on
  probabilistic gossip fanout.
- F2: a ping IS liveness evidence (SWIM paper §3) — the sender's
  Alive claim is applied before acking, clearing Dead/Suspect views;
  self-advertisement on the ack path is rate-limited to 500ms so a
  heavily-pinged node cannot flood its dissemination queue.
- F3: a fresh Alive apply cancels the pending suspicion timer, so a
  stale expiry can no longer promote a just-seen-live node to Dead.

Tests: 6 new regression tests pinning each fix + the combined
deterministic recovery loop (A holds Dead(B,5) → echo → self-refute
→ converge Alive), 129/129 swim, 1015 cluster lib, clippy -D 0,
maya-gate L1 clean.

Refactor note (deferred): IncarnationTracker consolidation
(P2-GLM53-REVIEW-RESOLUTION.md Improvement 1) — incarnation state is
still split between detector/local_incarnation and the store; a
single-owner tracker with atomic persist would remove the
fire-and-forget save. MemberState::Left has no production sender yet;
when graceful-leave lands, restart must resume above TerminalLeft or
clean-shutdown restarts will stick forever.
Copilot AI lite review requested due to automatic review settings August 23, 2026 22:23

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

EnRaiha added a commit to EnRaiha/nodedb that referenced this pull request Aug 24, 2026
Drill/fix-plan/GLM-resolution docs (commit dd0c316) written pre-fix are
now stale. Add resolution banners pointing to the implemented fixes:
SWIM 8886846 (PR NodeDB-Lab#243), wire window e60a853 (PR NodeDB-Lab#244), epoch fence
4f92959 (PR NodeDB-Lab#245), lease GC 16d9916 (PR NodeDB-Lab#246). Full verification +
refactor code in P2-REPORT.md.
load_swim_incarnation errors on corrupted metadata (len != 8) instead
of silent Ok(None); register_default_subsystems propagates read errors
instead of unwrap_or(None) — bootstrap no longer hides catalog
IO/corruption problems at startup.
EnRaiha added a commit to EnRaiha/nodedb that referenced this pull request Aug 24, 2026
/NodeDB-Lab#246

- catalog: load_swim_incarnation returns an ERROR on corrupted metadata
  (len != 8) instead of silently Ok(None); register_default_subsystems
  propagates the read error instead of unwrap_or(None) — bootstrap no
  longer hides catalog IO/corruption problems (PR NodeDB-Lab#243 critical items).
- server: handle_stream drops StalePeerEpoch frames explicitly with a
  warn! log instead of ending the stream via generic ? — fenced frames
  are not transport failures; connection stays open for exempt traffic
  (PR NodeDB-Lab#245 critical item).
- lease_gc: proposal failures logged at warn (operators can notice GC
  not happening); drain_propose doc comment translated to English
  (PR NodeDB-Lab#246 review items).
@EnRaiha

EnRaiha commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the two critical review items in 1876085:

  1. register_default_subsystems now propagates catalog read errors (?) instead of unwrap_or(None) — bootstrap fails on IO/corruption instead of silently restarting at incarnation 0.
  2. load_swim_incarnation now returns an error (with key + length) when stored metadata length != 8 — corrupted catalogs surface early.

Thanks for the review!

@farhan-syah

Copy link
Copy Markdown
Member

The diagnosis was right, and the fix is actually stronger than the description claims. ProbeScheduler::reshuffle builds the probe queue from snapshot().alive() only, so a node marked Dead is never probed again — peers holding Dead(A, N) send A nothing at all. Once the rumour ages out of piggyback buffers there is no path left for A to learn it is considered dead, so this is not just a probabilistic stick, it is a permanent one-way split.

F1 and F2 together are the part I would highlight: A pings with its stale Alive, the peer refutes and echoes its stored Dead record on the reply, A self-refutes above it, converged. That is a deterministic recovery loop and it works without persistence at all — F4 just makes the first announcement win outright. I checked whether F2 lets a stale claim bypass incarnation ordering and it does not: the sender's claim goes through apply_and_notify like any other update.

Landed in 47f890d, with you as co-author.

Four corrections went in on top, as 40ea609 and one hunk in the first commit.

  1. save_swim_incarnation was a blind overwrite. Bumps are persisted off the probe path, so two of them can reach the catalog out of order and leave the lower value on disk. The node then restarts at an incarnation it has already advertised and loses to a peer still holding a Dead record at that number — the exact stick the key exists to break. The write now reads inside its own transaction and discards anything at or below what is stored. swim_incarnation_never_moves_backwards covers it.

  2. The persistence ran in tokio::spawn, but save is a synchronous redb commit — a blocking call on a reactor worker. Now spawn_blocking. Note these two interlock: taking the write off the probe path is what makes bumps land out of order, so it is only safe because the store is monotonic now.

  3. spawn_with_subscribers gained a parameter but nodedb-cluster-tests/tests/swim_routing_invalidation.rs was left on the old arity, so the branch does not compile against the integration suite.

  4. CatalogIncarnationStore held Arc<ClusterCatalog>. The failure detector outlives the shutdown sequence that closes the catalog, so the redb file lock survived it and the next open failed with "Database already open. Cannot acquire lock." A node could not restart against its own data directory — four restart tests fail on it. It now holds a Weak, matching what LeaseRenewalLoop already does with Weak<SharedState> for the same reason.

Items 3 and 4 both surface immediately in the second test stage (-p nodedb-cluster-tests), which the verification note does not mention — swim 129/129 and cluster lib 1023 are both first-stage. Worth running both stages before posting results; item 4 in particular is a real availability bug, not a test artifact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Epic] P2 — Cluster Consensus Safety (v0.6)

3 participants