From 0312e9248d6f66faf9e3c00581ad0226d323d9ea Mon Sep 17 00:00:00 2001 From: Xiao Deshi Date: Mon, 19 Oct 2026 15:41:52 +0800 Subject: [PATCH] fix(tandem): stop handing the same loopback port out twice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestTandemPersistentTxnCompareTargets failed with rqlite failed readiness fatal: HTTP and Raft addresses must differ testAddress picked a free port by listening on 127.0.0.1:0 and closing before returning it. The second of two back-to-back calls could therefore receive the same port, because the allocator recycles what was just released. rqlite, given one address for both its HTTP API and its Raft listener, refuses to start, and the fixture only sees a readiness timeout — a message that names a clock problem rather than the port collision it actually is. Record each handed-out port and retry on a collision. Entries are never released, so the HTTP/Raft pair and every member of a multi-node fixture stay distinct for the whole process run. Test added: TestTestAddressNeverRepeatsAPort covers both shapes that matter — the rqlite pair, and the per-member address sequence. Honest note on the test: I could not make the old implementation fail here, on this host, across 10 consecutive runs of 200 calls each. Whether the kernel recycles a just-freed port is allocation detail, not something a test can force from outside. So this asserts the invariant rather than reproducing the failure, and the production CI flake is the evidence the collision is real. It is stated in the test comment so nobody reads a green run as proof of a repro. Call sites now deduped rather than colliding: persistence_integration_test.go:289 address, raft := testAddress(t), testAddress(t) persistence_integration_test.go:81 the readiness timeout that surfaced it cluster_integration_test.go:168-169 per-member httpAddr/raftAddr pair cluster_integration_test.go:261-262 same, in the single-member fixture --- pkg/tandem/persistence_integration_test.go | 100 +++++++++++++++++++-- 1 file changed, 95 insertions(+), 5 deletions(-) diff --git a/pkg/tandem/persistence_integration_test.go b/pkg/tandem/persistence_integration_test.go index dec0c051b..fbb9ba5a0 100644 --- a/pkg/tandem/persistence_integration_test.go +++ b/pkg/tandem/persistence_integration_test.go @@ -10,6 +10,7 @@ import ( "os" "os/exec" "path/filepath" + "strconv" "sync" "sync/atomic" "testing" @@ -216,14 +217,103 @@ func TestTandemPersistenceConcurrentTxn(t *testing.T) { } } +var ( + testPortMu sync.Mutex + testPortsUsed = map[int]bool{} +) + +// testAddress returns a localhost host:port this process has not handed out +// before. +// +// Picking a free port is done by listening on port 0 and closing, which frees +// it immediately. Two such calls in a row can therefore return the SAME port: +// the allocator recycles what was just released. rqlite, told to serve its HTTP +// API and its Raft listener on one address, refuses to start — +// "fatal: HTTP and Raft addresses must differ" — and the caller only sees a +// readiness timeout. Same story for the addresses of different cluster members. +// +// So the port is recorded and a collision retries. Registry entries are never +// released: a test's addresses stay distinct for the whole process run, which +// is what keeps the pair, and the members of a multi-node fixture, apart. func testAddress(t *testing.T) string { t.Helper() - l, err := net.Listen("tcp", "127.0.0.1:0") - if err != nil { - t.Fatal(err) + for attempt := 0; attempt < 1000; attempt++ { + l, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + addr := l.Addr().String() + l.Close() + + _, portText, err := net.SplitHostPort(addr) + if err != nil { + t.Fatal(err) + } + port, err := strconv.Atoi(portText) + if err != nil { + t.Fatal(err) + } + + testPortMu.Lock() + duplicate := testPortsUsed[port] + if !duplicate { + testPortsUsed[port] = true + } + testPortMu.Unlock() + if !duplicate { + return addr + } + } + t.Fatal("could not allocate a unique loopback address after 1000 attempts") + return "" +} + +// TestTestAddressNeverRepeatsAPort guards the allocator the fixtures depend on. +// +// The old implementation could return the same port twice, because each call +// released its port before returning it; that is what made rqlite abort with +// "HTTP and Raft addresses must differ" and the test report a readiness timeout +// instead. +// +// This is a structural guard rather than a reproduction: whether the kernel +// recycles a just-freed port is an allocation detail. I could not make the old +// implementation fail here on this host (10 consecutive runs, 200 calls each, +// no duplicate), so the old implementation was not inherently broken; the +// registry removes the possibility of the collision, and this test stands +// behind the guarantee that it does. +// +// The two cases that matter are the ones this test distinguishes: +// +// - Two consecutive calls, as in `address, raft := testAddress(t), +// testAddress(t)` (the rqlite fixtures) +// - Members of a multi-node cluster fixture, each taking its own pair +// +// So the assertions run the pair repeatedly and the cluster-sized sequence, and +// fail loudly on any repeat. Removing the registry in favour of the +// old two-line body does not necessarily fail this test on this host, but it +// reintroduces the state that made the production test flaky, which is the +// part worth pinning. +func TestTestAddressNeverRepeatsAPort(t *testing.T) { + // A single allocator serving two callers back to back is the rqlite shape. + for i := 0; i < 50; i++ { + address := testAddress(t) + raft := testAddress(t) + if address == raft { + t.Fatalf("iteration %d: HTTP and Raft addresses are both %s; rqlite will refuse to start", i, address) + } + } + + // A sequence for every member of the largest fixture in the suite. + seen := map[string]struct{}{} + for i := 0; i < 3; i++ { + for j := 0; j < 2; j++ { + addr := testAddress(t) + if _, dup := seen[addr]; dup { + t.Fatalf("member %d probe %d: address %s already handed out", i, j, addr) + } + seen[addr] = struct{}{} + } } - defer l.Close() - return l.Addr().String() } // Every test owns its rqlite process and data; no pre-existing daemon is used.