Skip to content

fix(gateway): keep a removed node out until an operator re-admits it - #1100

Merged
kvinwang merged 1 commit into
nextfrom
fix/gateway-removed-node-lockout
Aug 23, 2026
Merged

fix(gateway): keep a removed node out until an operator re-admits it#1100
kvinwang merged 1 commit into
nextfrom
fix/gateway-removed-node-lockout

Conversation

@kvinwang

@kvinwang kvinwang commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

Stacked on #1099; review only the last commit until that merges (base is set to its branch).

Problem

A node removed via RemoveNode silently rejoined the cluster the next time it started. Every node re-registers its own sync address on boot (register_peer_url), the sync endpoints authenticate app identity (RA-TLS) but not membership, and neither handle_envelope nor apply_envelope asks whether the sender is still a member. So "removed but still powered" was never really removed.

With #1099 collecting tombstones, that comeback stops being an annoyance and becomes a resurrection. A removed node returning with its old data directory still holds, live, records whose deletes the cluster collected in its absence. Ordinary rounds do not leak them back — its request re-teaches the responder coverage of its origin, so nothing it authored is re-sent. But the digests disagree for as long as the zombies exist (no tombstone and no ack filter can reconcile them), and after digest_check_rounds wavekv's divergence repair answers with a full re-exchange that merges them in as live values: deregistered CVMs re-entering every node's WireGuard config. #1099's a_removed_node_returning_with_old_state_resurrects_collected_deletes pins that route. The conclusion it forces: the door, not the merge, is where a removed sender has to stop.

Fix

RemoveNode now writes a durable removal marker, and both sync routes enforce it: an envelope from a marked sender is refused with 403 before anything merges.

The marker is a live record, deliberately. The obvious marker — the __peer_addr tombstone the removal already writes — is food for the very GC this defends against: once collected, the key reads as "never registered", which is indistinguishable from an early-bootstrap peer and must be admitted. The lockout would evaporate at exactly the moment it starts to matter, because the resurrection window opens after collection (while the tombstone survives, LWW kills the zombies for free). A fact that must outlive every tombstone cannot be expressed as a deletion, so it is stored as data: __peer_removed/{id}{removed_at, removed_by}.

This is also the embedder-side instance of the fix wavekv's own docs name but decline to build — "the collector needs to remember what it collected". For this one key family, the memory is the marker.

The rest of the surface

  • prune_removed_peers reads the marker too. The address tombstone still counts — it is the only signal a removal by an older binary leaves, until the collector eats it. A second watcher prunes on marker replication alone: a node that was offline for the removal may only ever receive the marker, the tombstone having been collected everywhere else before it returned.
  • A corrupt marker fails closed as "removed". Refusing sync is recoverable — the operator overwrites or clears the record; admitting a removed node's full dump is not.
  • RemoveNode reports membership from a pre-write read. Both the marker and the address tombstone wake watchers whose prune races the call — the marker is the sharper race, being the very first write — so removed_from_peer_set is captured before anything is written, and the racing-watcher guard test now drives both watchers at full speed.
  • Re-admission is one explicit operator decision: SetNodeUrl clears the marker. That is the escape hatch for a mistaken removal; the proto comment now spells out when re-admitting an old data directory is safe (never absent across a collected deletion; when in doubt, wipe first).
  • A node that finds its own marker at startup warns and proceeds. Best-effort only: the marker rarely replicates to its own victim — the refusals it drives are what keep it from arriving — the local copy may be stale after a re-admission, and the lockout is enforced by the peers either way. Making the refusal properly observable from both ends (metrics on the refusing peers and on the refused node itself, which can only learn of its removal from the 403s) is split into a follow-up PR stacked on this one.

Verification

cargo test -p dstack-gateway — 285 passed (after rebasing onto the updated #1099 branch). Five new tests, plus assertions added to two existing ones:

test property
a_removal_marker_survives_the_collector the full removal sequence plus a collection pass: the address tombstone is gone, the marker is what remains
a_marked_peer_is_pruned_even_when_the_address_tombstone_is_long_gone the post-collection state prunes; an early-bootstrap peer (no marker, no tombstone) is left alone
clearing_the_marker_re_admits_the_node clear → admitted again; idempotent, and the retry reports there was nothing to clear
a_corrupt_removal_marker_reads_as_removed fail-closed
a_removed_nodes_envelopes_are_refused_at_the_door route-level: both sync routes answer 403 to a marked sender, and clearing the marker opens the door again

Plus: the existing RemoveNode test asserts removal leaves the marker; the racing-watcher guard test (remove_node_reports_peer_membership_despite_a_racing_watcher) now also drives the marker watcher, guarding the pre-write membership capture against the race the marker itself introduces.

Residual risk

The lockout takes effect where the marker has replicated. In the round or so between RemoveNode and the marker reaching a given peer, that peer still answers the removed node — the same window the removal itself has always had, and it closes by replication rather than staying open forever. The protocol-level fix (wavekv remembering collected watermarks, making resurrection impossible regardless of membership games) remains upstream work.

Copilot AI lite review requested due to automatic review settings August 23, 2026 06:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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.

@kvinwang
kvinwang force-pushed the fix/gateway-removed-node-lockout branch from 23ab3c3 to c1935f3 Compare August 23, 2026 06:33
Base automatically changed from fix/gateway-kv-tombstone-gc to next August 23, 2026 07:21
@kvinwang
kvinwang force-pushed the fix/gateway-removed-node-lockout branch 4 times, most recently from 0a6d23c to c2673c5 Compare August 23, 2026 07:37
A node removed via RemoveNode silently rejoined the cluster the next
time it started: every node re-registers its own sync address on boot,
the sync endpoints authenticate app identity but not membership, and
nothing on the receiving side asks whether the sender is still a
member.

With tombstone GC collecting, that comeback is worse than an
annoyance. A removed node returning with its old data directory still
holds records whose deletes the cluster collected in its absence;
ordinary rounds do not leak them back, but the digests disagree for as
long as the zombies exist, and wavekv's divergence repair answers with
a full re-exchange that merges them in as live values -- deregistered
CVMs re-entering every node's WireGuard config
(`a_removed_node_returning_with_old_state_resurrects_collected_deletes`
pins the route). So the door, not the merge, is where a removed sender
has to stop.

RemoveNode now writes a durable removal marker that both sync routes
enforce: an envelope from a marked sender is refused with 403 before
anything merges. The marker is a **live** record, deliberately -- the
`__peer_addr` tombstone the removal also writes is food for the very
GC this defends against, and a marker the collector eventually eats
reads as "never registered" at precisely the moment the lockout
matters. A fact that must outlive every tombstone cannot be expressed
as a deletion.

RemoveNode reports membership from a read taken before its first
write. Both the marker and the address tombstone wake watchers whose
prune races the call -- the marker is the sharper race, being the very
first write -- so an answer read after either signal exists would
depend on scheduling. The racing-watcher guard test now drives both
watchers.

`prune_removed_peers` reads the marker too (the address tombstone
still counts, for removals performed by older binaries), and a second
watcher prunes on marker replication alone -- a node that was offline
for the removal may only ever see the marker, the tombstone having
been collected everywhere else. A corrupt marker fails closed as
"removed": refusing sync is recoverable by overwriting the record;
admitting a removed node's full dump is not.

Re-admission is one explicit operator decision: SetNodeUrl clears the
marker. A node that finds its own marker at startup logs a warning and
otherwise proceeds -- the marker rarely replicates to its own victim
(the refusals it drives are what keep it from arriving), the local
copy may be stale after a re-admission this node has not yet learned
about, and if it is current, every envelope it sends is refused
anyway. Making the refusal properly observable from both ends is a
follow-up.
@kvinwang
kvinwang force-pushed the fix/gateway-removed-node-lockout branch from c2673c5 to 0d7aeb3 Compare August 23, 2026 07:46
@kvinwang
kvinwang merged commit c7af44a into next Aug 23, 2026
16 checks passed
@kvinwang
kvinwang deleted the fix/gateway-removed-node-lockout branch August 23, 2026 08:08
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.

2 participants