From 6e8140980a32c4c9b02d8d02bb38398940e8e94a Mon Sep 17 00:00:00 2001 From: Shaurya Kesarwani Date: Tue, 13 Oct 2026 10:24:16 +0530 Subject: [PATCH] test(cluster/etcd): stop asserting that the undisturbed node never rejoins Two rejoin tests ended with `assert.Zero(t, a.Rejoins())`, a negative about a background watcher the design permits to fire. `follow` ends on either of two things, and only one of them is node-b's deletion: the other is the keep-alive channel closing, which a loaded runner can produce on any node at any time, and `maintain` documents the response as ordinary, a new lease and a new key rather than an exit. So a rejoin by node-a is correct behaviour, and the assertion read it as a defect. It failed on the windows runner for exactly that reason (#509). What the tests are about is still asserted: requireConverged checks that node-b's loss evicted nobody and left one key per node, and node-a is now checked not to believe itself absent. On which lease it got there is not the tests' business. The port-reuse half of #509 is fixed on main by cc0abbf (etcdtest binds to port 0), so this change no longer carries its own port handling. --- cluster/etcd/rejoin_test.go | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/cluster/etcd/rejoin_test.go b/cluster/etcd/rejoin_test.go index d00cfa96..2ce8add6 100644 --- a/cluster/etcd/rejoin_test.go +++ b/cluster/etcd/rejoin_test.go @@ -143,7 +143,19 @@ func TestMembershipRejoinsAfterLeaseExpiry(t *testing.T) { // registration rather than adding a second one, and node-a picked it back up. requireConverged(t, client, nodes, "node-a", "node-b") assert.NotEqual(t, lost, b.LeaseID(), "node-b registered under a fresh lease") - assert.Zero(t, a.Rejoins(), "the undisturbed node did not re-register") + + // There is deliberately no `assert.Zero(t, a.Rejoins())` here. It asserted a negative about + // a background watcher that the design permits to fire: `follow` ends on either of two + // things, and only one of them is node-b's deletion. The other is the keep-alive channel + // closing, which a loaded runner can produce on any node at any time, and `maintain` + // documents the response as ordinary — a new lease and a new key rather than an exit. So a + // rejoin by node-a is correct behaviour, not evidence of a defect, and the assertion read it + // as one; it failed on the windows runner for exactly that reason (#509). + // + // What matters is asserted by requireConverged above: node-b's loss must not evict node-a + // from the member set, nor leave it holding a registration the cluster cannot see. Whether + // node-a got there on its original lease or a replacement one is not this test's business. + assert.False(t, a.SelfAbsent(), "node-a still holds a registration the cluster can see") } // TestMembershipRejoinsAfterWholeRingLeaseLoss reproduces the reported end state directly: @@ -196,7 +208,8 @@ func TestMembershipRejoinsAfterExternalKeyDelete(t *testing.T) { requireConverged(t, client, nodes, "node-a", "node-b") assert.NotEqual(t, held, b.LeaseID(), "the stale lease is not reused") - assert.Zero(t, a.Rejoins(), "the undisturbed node did not re-register") + // No `assert.Zero(t, a.Rejoins())`: see TestMembershipRejoinsAfterLeaseExpiry. + assert.False(t, a.SelfAbsent(), "node-a still holds a registration the cluster can see") } // TestMembershipRejoinIsIdempotent hits the same node repeatedly. Each loss must produce