feat(mesh): dead-node key cleanup — replica attribution, membership removal, sweep#1674
feat(mesh): dead-node key cleanup — replica attribution, membership removal, sweep#1674CatherineSue wants to merge 11 commits into
Conversation
github-actions
Bot
added
tests
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements SWIM §5.2 dead-node removal and key sweeping in the mesh gossip protocol. It introduces tracking for Down nodes, holddown mechanisms to prevent stale re-insertions, and key sweeping (by replica authorship or node name suffix) when a node is removed. Additionally, it adds SWIM refutation to allow nodes to refute stale rumors about themselves. The review feedback suggests offloading the CPU-bound operations handle_node_removed and reconcile_replica_registry to tokio::task::spawn_blocking to avoid blocking the async executor threads.
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.
| if let Some(mesh_kv) = &self.mesh_kv { | ||
| let swept = mesh_kv.handle_node_removed(&name); | ||
| log::info!("Dead-node sweep for {name} tombstoned {swept} keys"); | ||
| } |
There was a problem hiding this comment.
The handle_node_removed function performs CPU-bound operations, including cloning and iterating over all keys in the store and scanning the entire operation log. Running this directly on the async executor thread can block the executor. Offload this operation to tokio::task::spawn_blocking to keep the async executor responsive. Note that data passed to the spawned task must have a 'static lifetime, so name must be cloned before being moved into the closure.
if let Some(mesh_kv) = &self.mesh_kv {
let mesh_kv = mesh_kv.clone();
let name = name.clone();
tokio::task::spawn_blocking(move || {
let swept = mesh_kv.handle_node_removed(&name);
log::info!("Dead-node sweep for {name} tombstoned {swept} keys");
});
}References
- Offload CPU-bound operations, such as large-scale bincode serialization or complex tree walks, to
tokio::task::spawn_blockingto prevent blocking the async executor threads. - Data passed to spawned background tasks must have a
'staticlifetime. Use owned types or reference-counted pointers likeArcinstead of passing references to ensure the data outlives the task.
There was a problem hiding this comment.
Declining the spawn_blocking here after quantifying: handle_node_removed is two full key scans plus one op-log snapshot scan and K tombstone writes — single-digit milliseconds at the design scale (~10k keys), and it fires once per dead node (the holddown prevents re-fires). The event loop it runs on sleeps 1s between rounds, and ms-scale work on one runtime worker doesn't stall the multi-threaded executor. spawn_blocking would add fire-and-forget semantics (sweep racing the next round's membership view) for a path that isn't hot. If a future profile shows otherwise we can offload then.
| if let Some(mesh_kv) = &self.mesh_kv { | ||
| mesh_kv.reconcile_replica_registry(); | ||
| } |
There was a problem hiding this comment.
The reconcile_replica_registry function is called periodically (every 60 rounds) and performs CPU-bound operations, including cloning and filtering all keys in the store. To prevent blocking the async executor thread, offload this call to tokio::task::spawn_blocking.
if let Some(mesh_kv) = &self.mesh_kv {
let mesh_kv = mesh_kv.clone();
tokio::task::spawn_blocking(move || {
mesh_kv.reconcile_replica_registry();
});
}References
- Offload CPU-bound operations, such as large-scale bincode serialization or complex tree walks, to
tokio::task::spawn_blockingto prevent blocking the async executor threads.
There was a problem hiding this comment.
Same reasoning as the sibling thread: reconcile_replica_registry is a key scan + op-log winner scan, milliseconds at design scale, once per 60 rounds on a loop that sleeps 1s per round — measured against that, spawn_blocking buys nothing and costs ordering (the reconcile re-assert racing the same loop's sweep decisions). Declining for now.
CatherineSue
force-pushed
the
chang/mesh-dead-node-sweep
branch
from
eac747c to
c500af4
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR adds dead-node expiration and holddown handling to gossip, refutes stale self failure rumors, and wires CRDT cleanup so removed nodes trigger key tombstoning and replica-registry reconciliation. It also updates tests and gateway wiring for the new sweep behavior. ChangesSWIM-style dead-node detection and cleanup
Estimated code review effort: 4 (Complex) | ~50 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eac747c8da
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for key in self.replica_keys_of(&self.server_name) { | ||
| if key != own_key { | ||
| self.delete_with_notify(&key); |
There was a problem hiding this comment.
When a node crash-restarts before tombstoning its worker: keys, this reconciliation deletes the previous incarnation's replica:{id} -> node entry while those old keys can still be live. If the node later dies, handle_node_removed() builds dead_replicas only from the remaining registry entries, so any live worker: keys whose winning op was authored by the retired replica ID are no longer attributable and will survive the dead-node sweep indefinitely. Keep prior replica entries until their authored keys have been removed, or sweep/migrate those keys before deleting the attribution record.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 6748401: reconcile now retires a prior incarnation's entry only when its replica no longer authors the winning op of any live key in an author-attributed namespace (winner scan over the op-log snapshot, gated on the key being live). A crash-restart's un-re-asserted keys keep their attribution until they're gone, and the registry stays bounded at one entry per replica still authoring keys. Regression test: reconcile_keeps_prior_entry_while_it_still_authors_live_keys.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c500af4ba0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for key in &replica_keys { | ||
| self.delete_with_notify(key); |
There was a problem hiding this comment.
A separate delayed-gossip case still hits this deletion path: if the timeout fires before this survivor has learned every worker: op from the dead replica, or if another survivor later relays an older batch, the registry tombstone here prevents replica:{id} from becoming live again while removed_holddown also avoids another sweep. The late worker: key can then merge in with the dead replica's author id, but future handle_node_removed() calls reconstruct dead_replicas only from live registry keys, so that key is no longer attributable and can survive indefinitely.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, but the sound fix needs machinery this stack's next PR introduces. A key relayed after the sweep retired the dead node's entries is exactly a causal-stability violation: the per-peer ack watermarks (from the #1586 sender loop) prove when no peer still holds unacked ops authored by the dead replicas — the same condition the deferred tombstone GC needs. The follow-up PR gates both on it: registry entries and tombstones retire only once every live peer has acked past them, which closes this window without guessing at delays. Until then the exposure is a key that arrives after removal + sweep (60s+ of lag) yet was never re-asserted by its owner — disclosed as a known limitation rather than patched with a timer here.
…ibuted Review on #1674 found the reconcile traded bounded registry growth for an attribution hole: a crash-restart can leave live keys whose winning op was authored by the prior incarnation, and retiring that replica entry orphans those keys at the node's real death (dead_replicas is rebuilt from live entries only). A stale entry now survives reconcile while it still authors some live key in an author-attributed namespace and retires on the first pass after the key is gone — bounding the registry at one entry per replica still authoring keys. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6748401ad5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if node.name == self.self_name && node.status != NodeStatus::Alive as i32 { | ||
| let refuted_version = node.version + 1; |
There was a problem hiding this comment.
When this node is in the graceful-shutdown path, peers can echo its Leaving state back during the 1s propagation window after MeshServerHandler::graceful_shutdown broadcasts it. This branch treats any non-Alive self entry, including NodeStatus::Leaving, as a stale failure rumor and rewrites it to a higher-version Alive entry; that higher version can then be gossiped back and make peers discard the intended Leaving state, so the graceful leave degrades into a failure/dead-timeout removal. Limit the refutation to failure states such as Suspected/Down, or preserve intentional Leaving.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 3e0f70b: refutation now covers Suspected and Down only, so a peer echoing the node's own graceful-shutdown Leaving broadcast is merged normally instead of being rewritten to Alive. Test: merge_state_preserves_leaving_rumor_about_self.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e0f70b06d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| held.retain(|_, since| now.saturating_duration_since(*since) < holddown); | ||
| held.retain(|name, _| match state.get(name) { | ||
| // Genuine return: lift the hold so normal tracking resumes. | ||
| Some(node) if node.status == NodeStatus::Alive as i32 => false, |
There was a problem hiding this comment.
When a removed peer is still listed as Alive on a slower survivor, that survivor's full-state gossip can reinsert the node after we have deleted its membership entry. This branch then treats the stale Alive as a genuine return and drops the holddown, so the dead node remains in membership until it is probed through Suspected/Down and waits another dead_timeout (and repeated stale Alive rumors can keep resurrecting it). The hold should only be lifted by a freshness proof such as a version newer than the removed state or a successful direct contact, not by any reoffered Alive entry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in bb00b85: the hold now records the membership version at removal and lifts only for an Alive entry strictly newer than it. A genuine return clears that bar via SWIM refutation (which bumps past the Down rumor's version); a stale Alive re-offer from a survivor that never saw the Down transition is purged like any other stale rumor, with the hold kept. Tests: holddown_lifts_on_alive_newer_than_removal, holddown_keeps_holding_on_stale_alive_rumor.
…marks Time-based tombstone GC was deliberately undriven: a peer absent past the grace replays its retained insert against empty metadata and resurrects the deleted key. Collection is sound only at causal stability — every live peer acked past the tombstone — which the per-peer send watermarks prove. CrdtWatermark::covers reports whether a peer acked a key at a version; engines gain gc_tombstones_where(grace, stable), with time-based gc_tombstones as the always-true special case. Both sender paths (client- and server-side) register their per-peer watermark in a controller-owned table, replaced on reconnect (fresh = conservative) and dropped on node removal (spec 5.2 step 4). The 60-round housekeeping tick collects tombstones every member covers, skipping collection while any member lacks a registered watermark. The same tick closes the late-relayed-key window from #1674 review: dead-node sweeps no longer retire replica-registry entries; held (recently removed) names are re-swept so late keys are tombstoned with attribution intact, and entries retire only once their node is neither in membership nor held and nothing live is attributed to them. The GC primitive and its driver land together because the primitive alone is dead code under -D warnings. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb00b85ecc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // intentional and must survive the merge. | ||
| let failure_rumor = node.status == NodeStatus::Suspected as i32 | ||
| || node.status == NodeStatus::Down as i32; | ||
| if node.name == self.self_name && failure_rumor { |
There was a problem hiding this comment.
Fresh evidence after the Leaving-rumor fix is that this branch still refutes any incoming Suspected/Down rumor about self without checking whether the local self entry is already Leaving. If a peer echoes a newer failure rumor during the graceful-shutdown propagation window after this node has accepted/broadcast Leaving, lines 206-212 rewrite the local state to higher-version Alive, and that higher version can be gossiped back so peers discard the intentional leave. Skip refutation when the current self status is Leaving.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 8880ea6: refutation now checks the local self entry first and stands down while it is Leaving, so a peer escalating the departing node's unresponsiveness can no longer be refuted into overriding the leave. Test: merge_state_does_not_refute_failure_rumors_while_leaving. (#1686 is rebased on top.)
…marks Time-based tombstone GC was deliberately undriven: a peer absent past the grace replays its retained insert against empty metadata and resurrects the deleted key. Collection is sound only at causal stability — every live peer acked past the tombstone — which the per-peer send watermarks prove. CrdtWatermark::covers reports whether a peer acked a key at a version; engines gain gc_tombstones_where(grace, stable), with time-based gc_tombstones as the always-true special case. Both sender paths (client- and server-side) register their per-peer watermark in a controller-owned table, replaced on reconnect (fresh = conservative) and dropped on node removal (spec 5.2 step 4). The 60-round housekeeping tick collects tombstones every member covers, skipping collection while any member lacks a registered watermark. The same tick closes the late-relayed-key window from #1674 review: dead-node sweeps no longer retire replica-registry entries; held (recently removed) names are re-swept so late keys are tombstoned with attribution intact, and entries retire only once their node is neither in membership nor held and nothing live is attributed to them. The GC primitive and its driver land together because the primitive alone is dead code under -D warnings. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
…marks Time-based tombstone GC was deliberately undriven: a peer absent past the grace replays its retained insert against empty metadata and resurrects the deleted key. Collection is sound only at causal stability — every live peer acked past the tombstone — which the per-peer send watermarks prove. CrdtWatermark::covers reports whether a peer acked a key at a version; engines gain gc_tombstones_where(grace, stable), with time-based gc_tombstones as the always-true special case. Both sender paths (client- and server-side) register their per-peer watermark in a controller-owned table, replaced on reconnect (fresh = conservative) and dropped on node removal (spec 5.2 step 4). The 60-round housekeeping tick collects tombstones every member covers, skipping collection while any member lacks a registered watermark. The same tick closes the late-relayed-key window from #1674 review: dead-node sweeps no longer retire replica-registry entries; held (recently removed) names are re-swept so late keys are tombstoned with attribution intact, and entries retire only once their node is neither in membership nor held and nothing live is attributed to them. The GC primitive and its driver land together because the primitive alone is dead code under -D warnings. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8880ea69dc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if name == self_name || node.status != NodeStatus::Down as i32 { | ||
| continue; |
There was a problem hiding this comment.
When a node exits via MeshServerHandler::graceful_shutdown, peers merge it as NodeStatus::Leaving; this new expiration path only tracks Down entries, and Leaving peers are excluded from probe selection, so a graceful leaver is never promoted to Down, never removed from membership, and mesh_kv.handle_node_removed() never runs. In clusters where that node published worker: or rl: CRDT keys, those keys and worker imports can linger indefinitely after a clean shutdown; treat Leaving as removable after a bounded delay or invoke the same sweep when the Leaving state is accepted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real gap, though not a regression of this PR — on main today a Leaving node also lingers forever (no expiry path of any kind), and this PR adds the Down removal without yet touching Leaving. The fix exists in the stacked #1686 (expire_down_nodes tracks is_departed = Down OR Leaving, test leaving_node_expires_like_down), where it pairs with the analysis that motivated it: a Leaving process has exited, so its in-memory CRDT state can never replay, making removal + sweep unconditionally safe — and without the expiry, its frozen watermark would stall #1686's causally-stable GC permanently. If you'd rather have per-PR completeness I can fold the Leaving-expiry commit down into this branch; as stacked, merging #1674 then #1686 closes the gap in order.
|
Hi @CatherineSue, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
…ibuted Review on #1674 found the reconcile traded bounded registry growth for an attribution hole: a crash-restart can leave live keys whose winning op was authored by the prior incarnation, and retiring that replica entry orphans those keys at the node's real death (dead_replicas is rebuilt from live entries only). A stale entry now survives reconcile while it still authors some live key in an author-attributed namespace and retires on the first pass after the key is gone — bounding the registry at one entry per replica still authoring keys. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
CatherineSue
force-pushed
the
chang/mesh-dead-node-sweep
branch
from
8880ea6 to
0baf890
Compare
…marks Time-based tombstone GC was deliberately undriven: a peer absent past the grace replays its retained insert against empty metadata and resurrects the deleted key. Collection is sound only at causal stability — every live peer acked past the tombstone — which the per-peer send watermarks prove. CrdtWatermark::covers reports whether a peer acked a key at a version; engines gain gc_tombstones_where(grace, stable), with time-based gc_tombstones as the always-true special case. Both sender paths (client- and server-side) register their per-peer watermark in a controller-owned table, replaced on reconnect (fresh = conservative) and dropped on node removal (spec 5.2 step 4). The 60-round housekeeping tick collects tombstones every member covers, skipping collection while any member lacks a registered watermark. The same tick closes the late-relayed-key window from #1674 review: dead-node sweeps no longer retire replica-registry entries; held (recently removed) names are re-swept so late keys are tombstoned with attribution intact, and entries retire only once their node is neither in membership nor held and nothing live is attributed to them. The GC primitive and its driver land together because the primitive alone is dead code under -D warnings. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
mergify
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0baf890889
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Some(node) | ||
| if node.status == NodeStatus::Alive as i32 && node.version > hold.removed_version => |
There was a problem hiding this comment.
When a removed node really restarts with the same name during the holddown window, it normally advertises a fresh Alive membership entry starting back at version 1 via its join/init-peer ping. Because removal already deleted the old Down entry from state, that restarted node does not necessarily receive a Down rumor to self-refute past removed_version; this condition then treats the genuine Alive v1 as stale and the Some(_) arm purges it again until the whole holddown expires. This makes a live replacement unable to reliably rejoin for 3 * dead_timeout after its predecessor was swept; use a direct-contact/join freshness proof or bump the accepted version instead of requiring the incoming Alive version to exceed the removed version.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged as a known, bounded tradeoff. The version fence is what stops a slower survivor's STALE Alive re-offer from lifting the hold; it cannot distinguish that from a genuine same-name restart at v1, because membership (NodeState) carries no incarnation/restart epoch — both look like 'Alive, version <= removed_version' to a survivor. The proper fix is SWIM incarnation numbers, already tracked as the residual on this stack's GC gate (a returning live node also needs them to fence replay). Until then the hold deliberately favors correctness (no resurrection / no live-node sweep) over re-admission latency, bounded by the quarantine window after which normal tracking resumes. Noted as a documented limitation rather than patched with a heuristic here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@crates/mesh/src/gossip_controller.rs`:
- Around line 241-279: expired is computed from a read snapshot but the loop
unconditionally removes and sweeps names which can delete a revived peer; fix by
taking the init_state write lock and verifying the peer is still absent/down
before performing removal and sweeping: inside the loop acquire let mut state =
init_state.write(); if state.contains_key(&name) { continue; } else { let
removed_version = state.remove(&name).map_or(0, |node| node.version); ... } and
only then proceed to retry_managers.remove(&name), down_since.remove(&name),
removed_holddown.insert(...), abort sync_connections via
self.sync_connections.lock().await.remove(&name), and call
mesh_kv.handle_node_removed(&name); this ensures merge_state/other writers
cannot revive the peer between the read snapshot and destructive removal.
In `@crates/mesh/src/gossip_service.rs`:
- Around line 201-205: The failure rumor handling must ignore incoming failure
rumors for the local node when self_is_leaving is true: after computing
self_is_leaving (the check node.name == self.self_name &&
state.get(&node.name).is_some_and(|entry| entry.status == NodeStatus::Leaving as
i32)) add an early skip (e.g., continue or return) whenever failure_rumor is
true and self_is_leaving is true so the code does not fall through into the
generic merge block that updates state (thus preventing a Suspected/Down rumor
from overwriting the deliberate Leaving state); keep the existing refutation
logic unchanged and only suppress the generic merge for self when
NodeStatus::Leaving.
In `@crates/mesh/src/kv.rs`:
- Around line 519-531: The code publishes a replica id keyed by
store.replica_id() but MeshKV still uses derive_replica_id(server_name), causing
a mismatch; make a single source of truth by deriving the replica id once and
using it everywhere: obtain let replica_id = store.replica_id() (or call
derive_replica_id(server_name) and pass it into the store so store.replica_id()
returns that value), then replace uses in
store.insert(format!("{REPLICA_PREFIX}{}", store.replica_id()), ...) and the
MeshKV::replica_id() field / Debug output to use that same replica_id variable
so the registry key, MeshKV::replica_id(), and any Debug displays are
consistent; update the code paths that currently call
derive_replica_id(server_name) to use the chosen replica_id instead.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1fe60702-dbef-4f89-bb40-9150ec8bae1d
📒 Files selected for processing (6)
crates/mesh/src/gossip_controller.rscrates/mesh/src/gossip_service.rscrates/mesh/src/kv.rscrates/mesh/src/lib.rscrates/mesh/src/tests/crdt_integration.rsmodel_gateway/src/mesh/wiring.rs
…weep The expired set is computed from a read snapshot; merge_state on the gRPC task could revive a peer to Alive in the gap before the removal write, and the unconditional remove+sweep would then delete the fresh entry and sweep a live node's keys — the exact hazard this path guards against. Remove and sweep only if the entry is still Down under the write lock; a revived peer is left in membership and dropped from down_since so it re-arms cleanly. Also drops the synthetic version-0 fallback (we proceed only with the real removed version). Reported by review on #1674. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
…g them The previous guard only skipped REFUTATION when self was Leaving, then fell through to the generic version-wins merge — so a higher-version Down/Suspected rumor overwrote the deliberate Leaving entry and ping_server began advertising Down, degrading a graceful leave into a failure removal. A self failure rumor is now ignored outright while Leaving (never refuted, never merged); when not Leaving it is still refuted to Alive. Reported by review on #1674. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
…marks Time-based tombstone GC was deliberately undriven: a peer absent past the grace replays its retained insert against empty metadata and resurrects the deleted key. Collection is sound only at causal stability — every live peer acked past the tombstone — which the per-peer send watermarks prove. CrdtWatermark::covers reports whether a peer acked a key at a version; engines gain gc_tombstones_where(grace, stable), with time-based gc_tombstones as the always-true special case. Both sender paths (client- and server-side) register their per-peer watermark in a controller-owned table, replaced on reconnect (fresh = conservative) and dropped on node removal (spec 5.2 step 4). The 60-round housekeeping tick collects tombstones every member covers, skipping collection while any member lacks a registered watermark. The same tick closes the late-relayed-key window from #1674 review: dead-node sweeps no longer retire replica-registry entries; held (recently removed) names are re-swept so late keys are tombstoned with attribution intact, and entries retire only once their node is neither in membership nor held and nothing live is attributed to them. The GC primitive and its driver land together because the primitive alone is dead code under -D warnings. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
…ibuted Review on #1674 found the reconcile traded bounded registry growth for an attribution hole: a crash-restart can leave live keys whose winning op was authored by the prior incarnation, and retiring that replica entry orphans those keys at the node's real death (dead_replicas is rebuilt from live entries only). A stale entry now survives reconcile while it still authors some live key in an author-attributed namespace and retires on the first pass after the key is gone — bounding the registry at one entry per replica still authoring keys. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
…weep The expired set is computed from a read snapshot; merge_state on the gRPC task could revive a peer to Alive in the gap before the removal write, and the unconditional remove+sweep would then delete the fresh entry and sweep a live node's keys — the exact hazard this path guards against. Remove and sweep only if the entry is still Down under the write lock; a revived peer is left in membership and dropped from down_since so it re-arms cleanly. Also drops the synthetic version-0 fallback (we proceed only with the real removed version). Reported by review on #1674. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
…g them The previous guard only skipped REFUTATION when self was Leaving, then fell through to the generic version-wins merge — so a higher-version Down/Suspected rumor overwrote the deliberate Leaving entry and ping_server began advertising Down, degrading a graceful leave into a failure removal. A self failure rumor is now ignored outright while Leaving (never refuted, never merged); when not Leaving it is still refuted to Alive. Reported by review on #1674. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
CatherineSue
force-pushed
the
chang/mesh-dead-node-sweep
branch
from
ddc7ae8 to
023cf62
Compare
…marks Time-based tombstone GC was deliberately undriven: a peer absent past the grace replays its retained insert against empty metadata and resurrects the deleted key. Collection is sound only at causal stability — every live peer acked past the tombstone — which the per-peer send watermarks prove. CrdtWatermark::covers reports whether a peer acked a key at a version; engines gain gc_tombstones_where(grace, stable), with time-based gc_tombstones as the always-true special case. Both sender paths (client- and server-side) register their per-peer watermark in a controller-owned table, replaced on reconnect (fresh = conservative) and dropped on node removal (spec 5.2 step 4). The 60-round housekeeping tick collects tombstones every member covers, skipping collection while any member lacks a registered watermark. The same tick closes the late-relayed-key window from #1674 review: dead-node sweeps no longer retire replica-registry entries; held (recently removed) names are re-swept so late keys are tombstoned with attribution intact, and entries retire only once their node is neither in membership nor held and nothing live is attributed to them. The GC primitive and its driver land together because the primitive alone is dead code under -D warnings. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/mesh/src/gossip_service.rs`:
- Around line 220-241: Replace the arithmetic operation `node.version + 1` with
`node.version.saturating_add(1)` to prevent overflow when node.version is at its
maximum value. In the `or_insert_with` closure that creates a new NodeState
entry, explicitly set the address field to `self.advertise_addr` instead of
inheriting it from the rumor via `..node`, ensuring the refutation uses the
current node's advertise address. Additionally, add two regression tests: one
that sends an incoming self-Down rumor with `version: u64::MAX` to verify
saturating addition behavior, and another where the rumor's address differs from
the node's current advertise_addr to verify the correct address is used in
refutation.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3bac668f-4c43-41da-acec-a487a7218641
📒 Files selected for processing (6)
crates/mesh/src/gossip_controller.rscrates/mesh/src/gossip_service.rscrates/mesh/src/kv.rscrates/mesh/src/lib.rscrates/mesh/src/tests/crdt_integration.rsmodel_gateway/src/mesh/wiring.rs
| let refuted_version = node.version + 1; | ||
| log::warn!( | ||
| "Refuting status {} rumor about self (v{}); re-asserting Alive v{refuted_version}", | ||
| node.status, | ||
| node.version | ||
| ); | ||
| state | ||
| .entry(node.name.clone()) | ||
| .and_modify(|entry| { | ||
| if entry.version < refuted_version { | ||
| entry.status = NodeStatus::Alive as i32; | ||
| entry.version = refuted_version; | ||
| updated = true; | ||
| } | ||
| }) | ||
| .or_insert_with(|| { | ||
| updated = true; | ||
| NodeState { | ||
| status: NodeStatus::Alive as i32, | ||
| version: refuted_version, | ||
| ..node | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the gossip_service.rs file
fd gossip_service.rs
# Check the file size and structure
wc -l crates/mesh/src/gossip_service.rsRepository: lightseekorg/smg
Length of output: 132
🏁 Script executed:
# Read the relevant section and surrounding context (lines 200-250)
sed -n '200,250p' crates/mesh/src/gossip_service.rs | cat -nRepository: lightseekorg/smg
Length of output: 2832
🏁 Script executed:
# Find the NodeState struct definition
rg -A 10 "struct NodeState" crates/mesh/src/Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Find the GossipService struct definition and look for advertise_addr
rg -B 5 -A 15 "struct GossipService" crates/mesh/src/Repository: lightseekorg/smg
Length of output: 1884
🏁 Script executed:
# Check the version field type in NodeState
rg "version\s*:" crates/mesh/src/gossip_service.rsRepository: lightseekorg/smg
Length of output: 262
🏁 Script executed:
# Find NodeState struct definition and its fields
rg -B 2 -A 20 "struct NodeState" crates/mesh/src/Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Look at the broader context to see what function this is in and if self is available
sed -n '150,250p' crates/mesh/src/gossip_service.rs | cat -nRepository: lightseekorg/smg
Length of output: 5406
🏁 Script executed:
# Check if there are any tests for the refutation logic
rg -B 5 -A 10 "refut" crates/mesh/src/gossip_service.rsRepository: lightseekorg/smg
Length of output: 6570
🏁 Script executed:
# Look for where NodeState is defined (might be in a proto or another file)
fd -e rs -e proto | xargs rg "struct NodeState|message NodeState"Repository: lightseekorg/smg
Length of output: 115
🏁 Script executed:
# Read the NodeState proto definition
cat crates/mesh/src/proto/gossip.protoRepository: lightseekorg/smg
Length of output: 4483
🏁 Script executed:
# Check what type version field is in the proto and generated code
rg -A 5 "message NodeState" crates/mesh/src/proto/gossip.protoRepository: lightseekorg/smg
Length of output: 203
🏁 Script executed:
# Look for overflow behavior or u64 usage with addition
rg "u64|uint64" crates/mesh/src/gossip_service.rsRepository: lightseekorg/smg
Length of output: 150
🏁 Script executed:
# Check if there are tests for the u64::MAX version case
rg -B 5 -A 10 "u64::MAX\|0xFFFFFFFFFFFFFFFF" crates/mesh/src/Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Verify overflow behavior of u64::saturating_add by checking Rust std docs
# Let's check if saturating_add is used elsewhere in the codebase
rg "saturating_add|saturating_sub" crates/Repository: lightseekorg/smg
Length of output: 2318
🏁 Script executed:
# Look more closely at the test to understand if it tests address handling
sed -n '780,850p' crates/mesh/src/gossip_service.rs | cat -nRepository: lightseekorg/smg
Length of output: 3039
🏁 Script executed:
# Check what happens to node.address in the current or_insert_with path
rg -B 10 -A 5 "or_insert_with" crates/mesh/src/gossip_service.rs | head -40Repository: lightseekorg/smg
Length of output: 1213
Harden self-refutation against max versions and stale rumor addresses.
At line 71, node.version + 1 overflows when node.version is u64::MAX (peer-supplied gossip values can reach max). Additionally, the or_insert_with path at lines 88-92 inherits the rumor's address via ..node, allowing a restarted node to re-advertise an old address instead of its current self.advertise_addr.
Use saturating_add(1) for overflow safety and explicitly set the address to self.advertise_addr to ensure refutation asserts the live node's current endpoint.
Add regression tests: one with an incoming self-Down rumor at version: u64::MAX, and another where the rumor address differs from the node's current advertise address.
🤖 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 `@crates/mesh/src/gossip_service.rs` around lines 220 - 241, Replace the
arithmetic operation `node.version + 1` with `node.version.saturating_add(1)` to
prevent overflow when node.version is at its maximum value. In the
`or_insert_with` closure that creates a new NodeState entry, explicitly set the
address field to `self.advertise_addr` instead of inheriting it from the rumor
via `..node`, ensuring the refutation uses the current node's advertise address.
Additionally, add two regression tests: one that sends an incoming self-Down
rumor with `version: u64::MAX` to verify saturating addition behavior, and
another where the rumor's address differs from the node's current advertise_addr
to verify the correct address is used in refutation.
|
This pull request has been automatically marked as stale because it has not had any activity within 14 days. It will be automatically closed if no further activity occurs within 16 days. Leave a comment if you feel this pull request should remain open. Thank you! |
Each node publishes replica:{replica_id} -> node_name at construction
(one entry per incarnation, since op-author ids are minted per boot).
handle_node_removed tombstones a dead node's keys in sweep-registered
namespaces — AuthorReplica matches the key's winning op author against
the dead node's registry entries (single-writer namespaces);
NodeNameSuffix matches the key's last :-segment (rl: shards) — then
retires the registry entries themselves. Every survivor sweeps;
concurrent tombstones converge and a live partitioned owner's
reconcile re-assert overrules a premature sweep.
Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
SWIM 5.2 prescribes Down -> removed after dead_timeout (60s) but Down entries lived in ClusterState forever, only excluded from peer selection — so no trigger existed for dead-node key cleanup in any namespace. The event loop now tracks when each peer was first seen Down, and past dead_timeout removes it from membership and retry state and calls MeshKV::handle_node_removed to sweep its keys. A peer that revives mid-grace clears its clock; a re-Down restarts it. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
MeshAdapters::start declares the attribution rule per namespace: worker: keys sweep by the publisher's replica id (single-writer); rl: shard keys sweep by their node-name suffix. The swept worker tombstones flow through the existing inbound loop, evicting the dead node's imported workers from the registry. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Adversarial review of the removal path found it was local-only while every ping carries full state and the receive path re-inserts absent nodes unconditionally — so removed Down entries ping-ponged between survivors (re-arming a fresh dead_timeout and re-firing the sweep forever), and a restarted node lost the version race to its own circulating Down rumor and was destructively swept while serving. Two fixes: merge_state now refutes any non-Alive rumor about self by re-asserting Alive past the rumor's version (the SWIM refutation subset this failure needs), and the controller holds removed names against re-insertion for 3x dead_timeout — stale re-offers are purged each round without re-tracking, while an Alive return lifts the hold. Removal also aborts the dead peer's sync-stream task instead of letting it tick into a dead channel until the idle timeout. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
…als sweeps Adversarial review found two registry lifecycle gaps: entries were written once per boot and retired only on node death, so every restart of a live node leaked one immortal key cluster-wide (unbounded with deploy count); and a premature sweep of a partitioned-but-alive node tombstoned its entry with nothing re-asserting it, permanently disarming author attribution for that node's real death. reconcile_replica_registry — run at boot and on the gossip loop's 60-round housekeeping tick — re-asserts the current incarnation's entry (winning over a sweep tombstone once the clock has observed it) and retires entries left by prior incarnations of this node, keeping the registry at ~one entry per live node. handle_node_removed also warns when author-attributed sweeps run with no registry entries (rolling upgrade from binaries that never published one). Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
…ibuted Review on #1674 found the reconcile traded bounded registry growth for an attribution hole: a crash-restart can leave live keys whose winning op was authored by the prior incarnation, and retiring that replica entry orphans those keys at the node's real death (dead_replicas is rebuilt from live entries only). A stale entry now survives reconcile while it still authors some live key in an author-attributed namespace and retires on the first pass after the key is gone — bounding the registry at one entry per replica still authoring keys. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
…eaving A peer echoing this node's own graceful-shutdown Leaving broadcast within the propagation window was treated as a stale failure rumor and rewritten to a higher-version Alive, degrading the graceful leave into a dead-timeout removal. Refutation now covers Suspected and Down only. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
A survivor that never saw the Down transition can re-offer a stale Alive entry for a removed node; treating any Alive as a genuine return lifted the hold and parked the dead node back in membership for another Suspected -> Down -> dead_timeout walk, repeatably. The hold now records the membership version at removal and lifts only for an Alive entry strictly newer — which a genuine return produces via SWIM refutation (it bumps past the Down rumor); stale Alive re-offers are purged like any other stale rumor. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
A node mid-graceful-shutdown that heard a peer escalate its unresponsiveness to Suspected/Down refuted the rumor to a higher-version Alive, overriding its own Leaving broadcast and degrading the graceful leave into a failure-path removal. Refutation now checks the local self entry and stands down while it is Leaving. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
…weep The expired set is computed from a read snapshot; merge_state on the gRPC task could revive a peer to Alive in the gap before the removal write, and the unconditional remove+sweep would then delete the fresh entry and sweep a live node's keys — the exact hazard this path guards against. Remove and sweep only if the entry is still Down under the write lock; a revived peer is left in membership and dropped from down_since so it re-arms cleanly. Also drops the synthetic version-0 fallback (we proceed only with the real removed version). Reported by review on #1674. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
…g them The previous guard only skipped REFUTATION when self was Leaving, then fell through to the generic version-wins merge — so a higher-version Down/Suspected rumor overwrote the deliberate Leaving entry and ping_server began advertising Down, degrading a graceful leave into a failure removal. A self failure rumor is now ignored outright while Leaving (never refuted, never merged); when not Leaving it is still refuted to Alive. Reported by review on #1674. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
CatherineSue
force-pushed
the
chang/mesh-dead-node-sweep
branch
from
023cf62 to
496ca0d
Compare
…marks Time-based tombstone GC was deliberately undriven: a peer absent past the grace replays its retained insert against empty metadata and resurrects the deleted key. Collection is sound only at causal stability — every live peer acked past the tombstone — which the per-peer send watermarks prove. CrdtWatermark::covers reports whether a peer acked a key at a version; engines gain gc_tombstones_where(grace, stable), with time-based gc_tombstones as the always-true special case. Both sender paths (client- and server-side) register their per-peer watermark in a controller-owned table, replaced on reconnect (fresh = conservative) and dropped on node removal (spec 5.2 step 4). The 60-round housekeeping tick collects tombstones every member covers, skipping collection while any member lacks a registered watermark. The same tick closes the late-relayed-key window from #1674 review: dead-node sweeps no longer retire replica-registry entries; held (recently removed) names are re-swept so late keys are tombstoned with attribution intact, and entries retire only once their node is neither in membership nor held and nothing live is attributed to them. The GC primitive and its driver land together because the primitive alone is dead code under -D warnings. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 496ca0ddb4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // id (single-writer namespace); rl shard keys carry the node name as | ||
| // their last segment. | ||
| mesh_kv.configure_dead_node_sweep("worker:", DeadKeyAttribution::AuthorReplica); | ||
| mesh_kv.configure_dead_node_sweep("rl:", DeadKeyAttribution::NodeNameSuffix); |
There was a problem hiding this comment.
Registering rl: for unconditional suffix sweeping means a transient partition or other false Down decision will tombstone the still-live node's rl:*:{node} shards. MeshKV::handle_node_removed treats premature sweeps as recoverable via owner reassert, but RateLimitSyncAdapter::start only observes tombstones/backfills for logging and the only writer is sync_counter, so the current window's count can disappear from cluster aggregates until that node sees another increment. That under-enforces rate limits after a false removal; either gate rl: sweeps on a stronger death proof or add an owner-side shard reassert.
Useful? React with 👍 / 👎.
1 participant
Description
Problem
A permanently-dead node's keys lived forever cluster-wide — the documented "known gap" of the worker-sync design: only the owner may tombstone its keys, and a dead owner never does. Its imported workers stayed registered on every gateway (demoted only by local health probes), its
rl:shards accumulated, and nothing in any namespace ever cleaned up. The trigger half was also missing at the membership layer: SWIM §5.2 prescribes Down → removed afterdead_timeout, but Down entries sat inClusterStateforever, only excluded from peer selection.Solution
One shared mechanism, per-namespace attribution:
replica:{replica_id}→ node name): every node publishes its per-boot op-author id at construction, giving survivors the attribution map from ops to nodes that random-per-boot replica ids otherwise destroy.dead_timeout(60s, spec default), removes it from membership/retry state, aborts its sync-stream task, and fires the sweep.MeshKV::handle_node_removed): tombstones the dead node's live keys in sweep-registered namespaces, then retires its registry entries. Attribution is declared per namespace inMeshAdapters::start:worker:sweeps by the key's winning-op author (single-writer namespace);rl:sweeps by the key's node-name suffix. Swept worker tombstones flow through the existing inbound loop, evicting the dead node's imports from every registry.Found by adversarial review, fixed in-PR (3 confirmed must-fixes):
merge_statere-asserts Alive past any non-Alive rumor about self) plus a removal holddown (stale re-offers are purged each round without re-arming the clock; an Alive return lifts the hold).reconcile_replica_registry(boot + 60-round cadence) retires prior incarnations' entries, bounding the registry at ~one entry per live node.Known limitations (disclosed)
rl:premature-sweep healing waits for the node's next local increment (no periodic re-publish yet — the d-3b middleware will publish on a cadence). Epoch-scoped aggregation makes the lost stale count mostly moot.dead_timeoutis the spec default (60s), not yet a config knob.Changes
crates/mesh/src/kv.rs—REPLICA_PREFIXregistry (auto-registered LWW),DeadKeyAttribution,configure_dead_node_sweep,handle_node_removed,reconcile_replica_registrycrates/mesh/src/gossip_controller.rs—expire_down_nodes+dead_timeoutremoval wired into the event loop; removal holddown; sync-stream abort; reconcile drive on the 60-round tickcrates/mesh/src/gossip_service.rs— SWIM self-refutation inmerge_statemodel_gateway/src/mesh/wiring.rs— sweep registration forworker:/rl:crates/mesh/src/lib.rs— exportDeadKeyAttributionTest Plan
cargo test -p smg-mesh(206) andcargo test -p smggreen;cargo clippy --workspace --all-targets --all-features -- -D warningsclean.(key, None)subscriber tombstones; gateway end-to-end (worker evicted from registry + rl shard zeroed);expire_down_nodestiming/revival/self/departure; holddown purge/lift/expiry;merge_staterefutation (and non-regression for peer rumors); reconcile retires prior incarnations + re-asserts after premature sweep.Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspasses🤖 Generated with Claude Code
Summary by CodeRabbit