test(cluster/etcd): absorb the port race, and stop asserting a legal rejoin

[shaurya703]

Fixes #509

Both halves, and the second one turned out to have an answer in the code rather than needing a judgement call.

(1) The port race — absorbed, not narrowed

Your diagnosis is exactly right, including that a fixed port range would trade one flake for another. embed.StartEtcd takes URLs and will not accept an already-bound listener, so the window genuinely cannot be closed — startEtcdOnFreshPorts re-picks and retries instead, bounded at 5 attempts.

The bound matters: anything that is not a bind collision is reported on the first attempt rather than burning the retries and then being described as a lost port, which it wouldn't be.

A second bug in the same three lines. Calling the old one-at-a-time helper twice did not guarantee two ports — the first listener was closed before the second opened, so the OS was free to hand the same port straight back, and etcd would then have been asked to bind its client and peer to one address. freeAddrs(t, n) holds every listener open until all n addresses have been chosen, which makes that structurally impossible rather than merely unlikely.

The rejoin fixture keeps its address on restart, deliberately — a client that survives the rollout is the whole point of it — so only its first boot re-picks. A lost address on restart is now a named failure rather than a bare require, because it means something took an address we were actively using rather than one we had merely reserved. Different situation, different message.

A bug of my own, worth naming

My first version had startEtcdOnFreshPorts create its own t.TempDir() and startEtcdServer create a second one for s.dir. Every restart would then have reopened an empty directory — quietly discarding the state that test exists to check, while still passing. It now returns the directory it settled on, so the restart reuses it.

(2) The rejoin assertion — legal, so the assertion goes

You asked whether a spurious rejoin by an undisturbed node is legal before relaxing anything. It is, and the code says so.

follow ends on either of two things, and only one of them is node-b's deletion:

the clientv3 keep-alive channel closing, the proactive signal that the lease is gone; and the watch [seeing its own key deleted]

A keep-alive channel closing is not specific to node-b — a loaded runner produces it on any node. And maintain documents the response as routine:

Losing it is therefore an ordinary event, answered with a new lease and a new key rather than with an exit.

So assert.Zero(t, a.Rejoins()) asserted a negative about a watcher the design explicitly permits to fire. That is why it failed on windows, and chasing it as a defect would have found nothing to fix.

Removed rather than relaxed, and replaced with what actually matters — deleting one node's key must not evict another:

assert.False(t, a.SelfAbsent(), "node-a still holds a registration the cluster can see")
assert.Contains(t, memberKeys(client), testPrefix+"node-a",
    "deleting one node's key does not evict another")

Whether node-a got there on its original lease or a replacement one isn't this test's business. If you'd rather treat a spurious rejoin as illegal and chase it, say so and I'll revert this half — but then the fix belongs in follow, not here.

Verification

The classifier is checked in both directions, since a missed match reinstates the flake and a false positive spends every attempt on an error that should have been reported:

mutation	result
isAddrInUse matches nothing real	red
isAddrInUse matches everything	red

The collision it classifies is produced by binding a held address, not constructed from a string. The Windows wording is asserted directly, since this machine cannot produce that error.

TestStartEtcdOnFreshPortsSucceedsUnderPortContention boots etcd while a goroutine continuously takes and releases ports, so the retry meets real contention rather than a stub.

An honest limit: the distinctness test cannot tell the old helper from the new one. Sequential :0 listens usually return different ports anyway, so that bug isn't reliably reproducible — the guarantee is structural, from holding the listeners open, and I'd rather say that than imply the test proves more than it does.

./cluster/etcd passes with -count=1, with -race, and the rejoin test 8×. gofmt and go vet clean. No sleeps added — per CLAUDE.md, and because the port race isn't a timing problem that waiting solves.

Not addressed

Instance (3), TestOwnershipReconcileMinimalMoveAndHandoffPlan, which you'd already recorded as a report rather than a confirmed instance. It didn't reproduce here either (20× with -race), and with #504 having removed the swallowing require.NoError, a genuine failure there should now name its own cause. Left alone rather than changed speculatively.

[shaurya703]

Rebuilt on current main (a5d12e78d-era is long gone; this is on top of 7d7016b) as a single commit that carries only the second half of #509.

The first half, the freeAddr probe-then-bind race, is fixed on main by cc0abbf: etcdtest binds the embedded server to port 0, so there is no address to lose and nothing for startEtcdOnFreshPorts to retry. That code is gone from this branch.

What remains is the rejoin assertion. main still ends TestMembershipRejoinsAfterLeaseExpiry and TestMembershipRejoinsAfterExternalKeyDelete with assert.Zero(t, a.Rejoins()), and the argument from the original description stands unchanged: follow also ends when the keep-alive channel closes, maintain answers that with a new lease and key as ordinary behaviour, so a rejoin by the undisturbed node is correct and the assertion reads it as a defect. That is the windows failure in #509. Both sites now keep requireConverged as the real check and add assert.False(t, a.SelfAbsent()), with the reasoning in a comment at the first site.

go vet ./cluster/etcd/ clean; go test ./cluster/etcd/ -run TestMembershipRejoins -count=1 passes (12.9s, embedded etcd).

If you would rather keep the negative assertion as a canary now that the shared server removed the port pressure, say so and I will close this instead.