feat(mesh): causally-stable tombstone GC driven by per-peer ack watermarks#1686
feat(mesh): causally-stable tombstone GC driven by per-peer ack watermarks#1686CatherineSue wants to merge 6 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 causally-stable tombstone garbage collection (GC) for CRDTs by tracking per-peer acknowledgment watermarks and only collecting tombstones that every live peer has acknowledged. Key changes include adding gc_stable_tombstones to the CRDT engines, introducing watermark coverage checks, and updating the gossip controller and service to share watermark tables. The review feedback highlights potential lock nesting and contention risks: specifically, holding DashMap shard write locks while calling the stable closure, and repeatedly acquiring read locks on peer watermarks inside the closure. The reviewer suggests pre-filtering candidates outside of the DashMap locks and snapshotting the watermarks once before executing the GC.
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.
| .is_none_or(|winner| { | ||
| winner.is_tombstone | ||
| && now.saturating_duration_since(winner.created_at) >= grace | ||
| && stable(&key, winner.version_key()) |
There was a problem hiding this comment.
Holding the DashMap shard write lock (via remove_if) while calling the stable closure—which internally acquires read locks on multiple RwLock<CrdtWatermark>s—introduces a lock nesting risk. If other threads attempt to write to those watermarks while holding other locks, or if there is high contention, this can block the DashMap shard and degrade performance.
Consider refactoring gc_tombstones_where to pre-filter the candidates and evaluate stable outside of the DashMap locks.
References
- To avoid holding
DashMapshard locks during slow operations, collect the keys and/or values into a temporary collection (e.g.,Vec) before iterating and performing the operations.
There was a problem hiding this comment.
Addressed via the sibling thread's restructure (59b08ae): stable() now reads pre-cloned snapshots, so no watermark lock is ever taken inside remove_if. For the record, the nesting as-was could not deadlock — the ack paths take only the watermark lock over a pre-materialized op snapshot and never touch engine shard locks, so the shard->watermark order had no inversion partner — but eliminating the nesting class entirely is strictly better than relying on that argument.
| entry | ||
| .tombstoned_at | ||
| .is_some_and(|at| now.saturating_duration_since(at) >= grace) | ||
| && stable(&key, (version.timestamp, version.replica_id)) |
There was a problem hiding this comment.
Holding the DashMap shard write lock (via remove_if) while calling the stable closure—which internally acquires read locks on multiple RwLock<CrdtWatermark>s—introduces a lock nesting risk. If other threads attempt to write to those watermarks while holding other locks, or if there is high contention, this can block the DashMap shard and degrade performance.
Consider refactoring gc_tombstones_where to pre-filter the candidates and evaluate stable outside of the DashMap locks.
References
- To avoid holding
DashMapshard locks during slow operations, collect the keys and/or values into a temporary collection (e.g.,Vec) before iterating and performing the operations.
There was a problem hiding this comment.
Same fix as the LWW twin: 59b08ae snapshots the watermarks before the GC pass, so the RateLimitEngine predicate also runs lock-free under remove_if.
| let stable = |key: &str, version: (u64, ReplicaId)| { | ||
| handles | ||
| .iter() | ||
| .all(|acked| acked.read().covers(key, version)) | ||
| }; |
There was a problem hiding this comment.
Evaluating acked.read() inside the stable closure for every single tombstone candidate acquires and releases a read lock on each peer's RwLock<CrdtWatermark> per candidate. If there are many tombstones, this can cause significant lock contention with the stream tasks that frequently write to these watermarks.
We can optimize this by cloning/snapshotting the watermarks of all peers once before running the GC, completely eliminating per-candidate lock acquisition.
let watermarks: Vec<CrdtWatermark> = handles
.iter()
.map(|acked| acked.read().clone())
.collect();
let stable = move |key: &str, version: (u64, ReplicaId)| {
watermarks
.iter()
.all(|wm| wm.covers(key, version))
};There was a problem hiding this comment.
Adopted in 59b08ae — the driver now clones each registered watermark once per GC run (60s cadence, ~O(keys) HashMap clone per peer, transient), so the stability predicate evaluates against plain snapshots with zero lock acquisition under the engine shard locks. Acks landing mid-run defer coverage to the next run, which is conservative by construction.
58650ce to
dbdb96e
Compare
The stability predicate ran under engine shard locks while acquiring every peer's watermark RwLock per tombstone candidate. Cloning each registered watermark once per run (60s cadence) removes all lock acquisition from the predicate; acks landing mid-run defer coverage to the next run, which is conservative. Suggested by review on #1686. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
8880ea6 to
0baf890
Compare
The stability predicate ran under engine shard locks while acquiring every peer's watermark RwLock per tombstone candidate. Cloning each registered watermark once per run (60s cadence) removes all lock acquisition from the predicate; acks landing mid-run defer coverage to the next run, which is conservative. Suggested by review on #1686. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
59b08ae to
e7c841b
Compare
![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: e7c841b7de
ℹ️ 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 *attribution == DeadKeyAttribution::AuthorReplica { | ||
| let swept = self.sweep_authored_by(prefix, &absent_replicas); |
There was a problem hiding this comment.
When a removed node uses DeadKeyAttribution::NodeNameSuffix, this retirement path deletes the last registry entry without running sweep_node_suffix. If a stale survivor relays a suffix-owned key after the final holddown re-sweep but before retire_absent_replica_entries runs, the key survives this pass; once the registry is deleted and the name is no longer held, the controller has no later opportunity to call handle_node_removed("dead-node"), so the ghost key can remain indefinitely. The retirement pass should apply the suffix sweep for the absent node names before deleting their registry entries, just as it does author-replica sweeps here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 76d2f1b. retire_absent_replica_entries now also runs the NodeNameSuffix sweep — for each absent node name it calls sweep_node_suffix across the suffix-attributed prefixes before deleting the registry entries, so a late-relayed rl: shard is tombstoned in the same (last) pass that can attribute it rather than stranded. Regression test absent_entry_retirement_sweeps_suffix_attributed_ghosts.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// is paused, so a partitioned-but-alive node returning inside the window | ||
| /// finds every tombstone intact and merges them instead of replaying its | ||
| /// stale inserts around collected ones. Must exceed the tombstone grace. | ||
| const REMOVAL_QUARANTINE: Duration = Duration::from_secs(600); |
There was a problem hiding this comment.
🟡 Nit: The safety of the quarantine depends on REMOVAL_QUARANTINE > DEFAULT_TOMBSTONE_GRACE (defined in crdt.rs), but nothing enforces this at compile time. If someone bumps the grace past 600s without updating this constant, the returning-node replay protection silently breaks. A compile-time assertion would lock the invariant:
| const REMOVAL_QUARANTINE: Duration = Duration::from_secs(600); | |
| const REMOVAL_QUARANTINE: Duration = Duration::from_secs(600); | |
| const _: () = assert!( | |
| REMOVAL_QUARANTINE.as_secs() > crate::crdt_kv::DEFAULT_TOMBSTONE_GRACE.as_secs(), | |
| "quarantine must exceed tombstone grace for returning-node replay safety" | |
| ); |
The stability predicate ran under engine shard locks while acquiring every peer's watermark RwLock per tombstone candidate. Cloning each registered watermark once per run (60s cadence) removes all lock acquisition from the predicate; acks landing mid-run defer coverage to the next run, which is conservative. Suggested by review on #1686. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
retire_absent_replica_entries swept only AuthorReplica prefixes before deleting a departed node's registry entries; a NodeNameSuffix key (e.g. an rl: shard) relayed after the node left holddown was never swept and, with the registry entry gone and the name no longer held, no later handle_node_removed could attribute it — a permanent ghost. Retirement now also sweeps NodeNameSuffix prefixes by the absent node names (which need no registry entry) in the same pass. Reported by review on #1686. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
e7c841b to
d001a57
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d001a57fa7
ℹ️ 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 absent_keys.is_empty() { | ||
| return 0; |
There was a problem hiding this comment.
When a departed node has no live replica: entry (for example an older/partially upgraded node, or one whose registry entry was already tombstoned) but a late DeadKeyAttribution::NodeNameSuffix key is relayed after the holddown re-sweeps, this early return skips suffix cleanup entirely. At that point the controller has already dropped the node name from removed_holddown, so there is no later handle_node_removed(node) call to tombstone rl:*:<node> ghosts; suffix-attributed cleanup needs to scan/sweep absent suffixes even when there are no registry entries to retire.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferring with rationale. The suffix sweep here is driven by the replica registry, so a departed node that left NO trace (no replica: entry — pre-upgrade, or an entry already retired in a prior pass) gives retire_absent_replica_entries no node name to sweep by, and handle_node_removed no longer fires for it. Closing this fully needs a membership-diff sweep (rl:*: where is in no live member), which is a different mechanism with its own live-node-sweep risk and belongs with the SWIM membership work. It's also enforcement-inert in the meantime: get_aggregate only sums max-epoch shards, so a stale-epoch ghost from a dead node contributes 0 to the cluster total — the leftover is a bounded, inert shard, not a counted one. Tracking as a known limitation of suffix-attribution cleanup rather than expanding this PR.
ddc7ae8 to
023cf62
Compare
The stability predicate ran under engine shard locks while acquiring every peer's watermark RwLock per tombstone candidate. Cloning each registered watermark once per run (60s cadence) removes all lock acquisition from the predicate; acks landing mid-run defer coverage to the next run, which is conservative. Suggested by review on #1686. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
retire_absent_replica_entries swept only AuthorReplica prefixes before deleting a departed node's registry entries; a NodeNameSuffix key (e.g. an rl: shard) relayed after the node left holddown was never swept and, with the registry entry gone and the name no longer held, no later handle_node_removed could attribute it — a permanent ghost. Retirement now also sweeps NodeNameSuffix prefixes by the absent node names (which need no registry entry) in the same pass. Reported by review on #1686. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
d001a57 to
d2a78eb
Compare
|
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! |
…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>
…tamps A legacy timestamp-only ack maps to (v, ReplicaId::MAX), which allows() deliberately overstates to suppress resends — but covers() reused it as a causal-stability proof, claiming coverage of a same-timestamp tombstone from another replica that the sentinel also suppresses from ever being sent. Collecting on that proof and a later replay of the peer's retained insert would silently resurrect the deleted key. Sentinel entries now prove coverage only below their timestamp. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
…Leaving Adversarial review proved the removal path reopened the replay hole stable GC exists to close: removing a node dropped its GC veto, yet the stack explicitly supports that node returning (refutation + holddown lift) with its full in-memory log — after grace + collection, its replayed dominated inserts landed on vacant metadata and resurrected deleted keys cluster-wide; a survivor that removed every peer even degenerated to vacuously-true stability (pure time-based GC). Removals now sit in a 600s quarantine (longer than the tombstone grace) during which stable GC is paused entirely, so a node returning inside the window finds every tombstone intact and merges them; the residual — a live node partitioned past the quarantine that then heals — needs incarnation numbers (SWIM hardening) and is documented at the gate. Leaving now expires from membership like Down (spec 5.2 removes on either; an exited process has no in-memory state to replay), closing the review's other confirmed leak: a graceful scale-down used to strand its name in membership and its frozen watermark in the table, stalling stable GC permanently. Watermark registrations are now stream-scoped (Drop deregisters unless a replacement re-registered), so a dead stream blocks GC only until reconnect instead of forever. Registry retirement sweeps ghost keys (late-relayed after the quarantine) in the same pass instead of retaining their attribution indefinitely. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
The stability predicate ran under engine shard locks while acquiring every peer's watermark RwLock per tombstone candidate. Cloning each registered watermark once per run (60s cadence) removes all lock acquisition from the predicate; acks landing mid-run defer coverage to the next run, which is conservative. Suggested by review on #1686. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
retire_absent_replica_entries swept only AuthorReplica prefixes before deleting a departed node's registry entries; a NodeNameSuffix key (e.g. an rl: shard) relayed after the node left holddown was never swept and, with the registry entry gone and the name no longer held, no later handle_node_removed could attribute it — a permanent ghost. Retirement now also sweeps NodeNameSuffix prefixes by the absent node names (which need no registry entry) in the same pass. Reported by review on #1686. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
The under-lock re-check guarding against sweeping a revived node matched only Down, but this branch expires Leaving nodes too; a Leaving node that reached dead_timeout would fail the re-check and never be removed or swept. Match is_departed, consistent with expire_down_nodes. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
023cf62 to
496ca0d
Compare
d2a78eb to
8db7cce
Compare
|
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! |
1 participant
Description
Problem
Tombstone GC was deliberately undriven: time-based collection is unsound, because a peer absent past the grace replays its retained insert against vacant metadata and resurrects the deleted key. Without any driven GC, tombstone metadata and the Remove ops they pin accumulate forever — dead-node sweeps (#1674) made this acute by becoming the first mass-delete producer.
Solution
Collect a tombstone only at causal stability — when every member has provably acked past it:
CrdtWatermark::covers(key, version)turns the per-peer send watermarks (feat(mesh): per-key CRDT send watermark + ack + retry #1586) into stability proofs; engines gaingc_tombstones_where(grace, stable)(time-basedgc_tombstonesis the always-true special case).stable = all peers cover it. A peer acked tombstone ⇒ it holds and has compacted against it ⇒ it can never relay the dominated insert.Found by adversarial review, fixed in-PR (3 confirmed must-fixes):
Leavingdeparture stalled GC permanently (name stranded in membership, watermark frozen in the table). Leaving now expires from membership like Down — spec §5.2 removes on either, and an exited process has no in-memory state to replay, so removal + sweep is unconditionally safe.ReplicaId::MAXsentinel) claimed coverage of same-timestamp tombstones they also suppress from ever being sent. Sentinel entries now prove coverage only strictly below their timestamp.It also delivers the promise from the #1674 review threads: dead-node sweeps no longer retire replica-registry entries; quarantined names are re-swept each tick (late-relayed keys tombstoned with attribution intact), and retirement sweeps any remaining ghost keys in the same pass before deleting the entries.
Known limitations (disclosed)
gc_grace+ rebootstrap rule is the operational precedent). Documented at the GC gate.Changes
crates/mesh/src/crdt_kv/watermark.rs—covers(sentinel-aware)crates/mesh/src/crdt_kv/engine/{mod,lww,rate_limit}.rs—gc_tombstones_wherecrates/mesh/src/crdt_kv/crdt.rs,crates/mesh/src/kv.rs—gc_stable_tombstones; retirement sweeps ghostscrates/mesh/src/gossip_controller.rs—PeerWatermarkstable +WatermarkRegistration, quarantine, Leaving expiry, GC drivercrates/mesh/src/gossip_service.rs— server-side registrationcrates/mesh/src/service.rs— table shared controller → serviceTest Plan
cargo test -p smg-meshgreen (214);cargo clippy --workspace --all-targets --all-features -- -D warningsclean.coverssemantics + sentinel strictness; Leaving expires like Down; ghost-sweeping retirement; existing dead-node/holddown/refutation suites updated and green.Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspasses🤖 Generated with Claude Code