fix(gateway): collect the tombstones the cluster has finished with

[kvinwang]

Problem

Nothing in this repository ever called wavekv's tombstone collector. Every delete leaves a tombstone, and they are never removed:

$ grep -rn "cleanup_expired_tombstones\|collect_tombstone_garbage" dstack/ --include=*.rs
(no results, before this PR)

wavekv 2.1 says the same thing from its side, on collect_tombstone_garbage:

No embedder in this repository calls this method, so the cost above is latent.

So a gateway accumulates, permanently and on disk, one inst/ tombstone per deregistered CVM — plus one per operator override the same delete withdraws. They are replicated state: each one also rides in every sync digest and in every full re-exchange, on every node. A gateway fronting a fleet that churns CVMs pays this for the life of the deployment.

Fix

Run collect_tombstone_garbage on both stores, spawned beside the periodic persist and WAL-sync tasks — the same set of work the store needs on a schedule whether or not this node has peers. A single-node gateway has nobody to resurrect from, so wavekv collects every tombstone it holds there, and it is the deployment where nothing else ever would.

Safety comes from wavekv's ack watermark, not from the trigger: a tombstone goes only once every known peer has acknowledged the delete, at which point no peer holds the live record and nothing can resurrect it. The trigger only decides when each node stops to scan — which is a cluster-coordination problem, not a correctness one, and is the part worth reviewing.

The trigger counts replicated writes; no node reads a clock. The state digest covers tombstones — it has to, or it could not catch a replica that lost one — so a node that has collected and a node that has not report different digests while both are correct. wavekv's detector reads that as silent divergence after digest_check_rounds quiescent rounds and forces a full re-exchange, after which the laggard's tombstone comes back and the collector drops it again next cycle. wavekv documents this oscillation as an accepted cost and does not fix it, so the embedder has to keep the window in which nodes disagree small.

Free-running per-node timers put two nodes half an interval apart on average — far past that threshold. A wall-clock-aligned tick (this PR's first shape) closes the window, but only while every host's clock stays stepped-together: alignment computed once at startup is undone for good by a later NTP step, e.g. a gateway starting before chrony's first sync. So the trigger is instead a pure function of replicated state: collect once every tombstone_gc_writes replicated writes (default 10000, zero disables), per store, where the count is the sum of every origin's ack watermark. That count converges on every node within a sync round, so the cluster crosses each boundary together — the window is one check period plus one sync round, with no assumption about time. The cadence also follows write volume, which is what produces tombstones in the first place: the busy ephemeral store collects roughly hourly, the quiet persistent one much more rarely.

A node that has not collected since its task started treats the next check as due, so a restarted node and a store that never writes again both shed the backlog they already hold. In a cluster that backlog collection runs alone, so anything it frees comes back via one digest repair and is shed for good at the next shared boundary -- one bounded repair per restart.

Peer removal resets the baseline. RemovePeer drops the removed origin's ack watermark, so the write count steps back permanently -- a baseline earned before the removal would gate collection until the cluster re-earns writes it no longer remembers, which on a quiet store means never. The task watches the peer set and starts over from the backlog case when it shrinks; removal replicates, so every node's reset lands within a sync round of the others. (Transient dips from digest repair are just waited out -- retransmission restores them.)

The pace is one number cluster-wide, stored in the KV

Nodes collecting on different boundaries are back to collecting on their own phase. So the config-file value (core.sync.tombstone_gc_writes) is only a default: an operator override stored in the KV itself (SetTombstoneGcConfig / GetTombstoneGcConfig admin RPCs, global/tombstone_gc_config) replicates to every node and takes precedence. A corrupt override fails closed — the round is skipped rather than collected on the local default, which would be exactly the phase drift the shared pace exists to prevent.

Why there is no TTL

The obvious shape for this knob is a tombstone TTL. That API existed in wavekv 1.x as cleanup_expired_tombstones(ttl) and 2.0 deleted it, naming this repository's failure mode:

is_expired_tombstone(ttl_ms, now_ms) was removed in 2.0 along with v1's local-clock tombstone TTL. Collecting a tombstone on a local timer is precisely what lets a lagging replica resurrect a deleted key (RFC 0001 Section 6) — in gateway terms, a deregistered CVM reappearing in every peer's WireGuard config.

The replacement gates on coverage instead of on time: a tombstone authored by origin n at seq s may go only once every known peer reports peer_acks[p][n] >= s. The supporting invariants are what make that safe rather than optimistic:

• acks[n] is "the largest seq of origin n's writes this node provably covers", not an estimate.
• peer_acks is documented as "volatile, and safe to lose or under-report" — losing one delays collection, never advances it.
• membership is durable precisely so this cannot be weakened by a crash: "A peer forgotten in a crash would let this node collect tombstones that peer never covered, and the peer's next round would then resurrect every key it still held as a live value."
• a peer that has never been heard from still counts as a member, and pins collection.

So a freshly written tombstone is collected only when every peer has already applied the delete — at which point no peer holds the live record and there is nothing to push back. A TTL is strictly weaker: it drops the tombstone on a guess about elapsed time, and a peer offline longer than the TTL resurrects the record.

tombstone_gc_writes therefore trades disk against how often each node stops to scan its data map. It has no bearing on correctness.

Verification

cargo test -p dstack-gateway — 280 passed (after rebasing onto next, which grew the suite).

Nine new tests:

test	property
a_single_node_collects_the_tombstones_it_has_finished_with	a delete leaves a tombstone that survives load_all_instances returning empty; collection removes it
a_tombstone_a_peer_has_not_acknowledged_is_kept	adding a peer that has said nothing pins collection
a_tombstone_is_collected_on_coverage_not_on_age	two real KvStores exchanging real sync envelopes: the peer holds the live record, the delete is not collectable, after the round trip it is, and a further round trip does not bring it back
tombstone_collection_triggers_on_write_count_boundaries_not_on_time	inside a boundary nothing is due; crossing it (either store) is; two nodes whose views are a beat apart agree on every boundary; a count lowered by digest repair is not due until it crosses again
the_replicated_write_count_advances_with_writes_and_converges_between_peers	the trigger's input behaves like replicated state: writes advance it, converged nodes read the same value
removing_a_peer_permanently_lowers_the_replicated_write_count	RemovePeer deletes the removed origin's watermark from the count — the permanent regression the baseline reset exists for
a_removed_node_returning_with_old_state_resurrects_collected_deletes	the known limitation, demonstrated (see below): ordinary rounds do not leak the zombie back, the divergence repair does
a_tombstone_gc_override_is_absent_by_default_and_replicates	absent means "config-file default"; a stored pace replicates and every node reads the operator's number
a_corrupt_tombstone_gc_override_does_not_read_as_the_default	fail-closed, mirroring the certbot config

Open questions for review

1. Is 10000 writes the right default pace? Any value is correct; it trades disk against scan frequency. On the busy ephemeral store it works out to roughly hourly under a ~100-instance fleet; on the quiet persistent store, much rarer — which also means the persistent backlog mostly drains via the collect-on-start path rather than boundary crossings.

2. I could not construct a resurrection through a supported path. Answered: there is one, and it is now pinned by a test. The construction is a node removed via RemoveNode that later returns with its old data directory. Removal vacates the watermark, collection drops the tombstone, and the returning node still holds the record live. Ordinary rounds do not leak it back — its request re-teaches the responder coverage of its origin, so nothing it authored is re-sent. What completes the route is wavekv's own divergence repair: the digests disagree for as long as the zombie exists (no tombstone and no ack filter can reconcile them), so after digest_check_rounds the repair fires reset_peer_coverage on the returning node and its next request is a full dump, which merges the record back as live — a deregistered CVM re-entering every node's WireGuard config. a_removed_node_returning_with_old_state_resurrects_collected_deletes demonstrates both halves.

   Two consequences. Operationally, removal must mean decommission: a removed node's data directory must never come back (note that a merely-restarted node re-registers itself via register_peer_url, so "removed but still powered" is an accident waiting to happen; a wiped data directory or a fresh node id is safe). Structurally, the fix is to lock a removed sender out at the sync boundary — refused before handle_envelope, it cannot push the dump. That needs a removal marker that survives collection, i.e. a live record rather than the __peer_addr tombstone (which this very GC eventually collects); it is a self-contained node-retirement feature and lands as fix(gateway): keep a removed node out until an operator re-admits it #1100.

3. Retired peers make some tombstones uncollectable forever. wavekv documents it: remove_peer drops acks[removed], and once every node has retired a peer, the watermark for tombstones that peer authored is unknowable. Out of scope here, but it means collection is best-effort, not a bound.

[Copilot]

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