Fix flaky cluster tests - #39
Merged
Merged
Conversation
The cluster suite failed in roughly one of six matrix cells per push, and fail-fast turned that single flake into a fully red run. TestMasterOnly asserted on a debug event within a fixed 800ms window, which races the background control loop: an iteration that read the master-only set before the test unset it emits the event after the events were reset. Both fallback-to-slave tests read a value from a replica right after writing it to the master, without waiting for asynchronous replication. TestAllReturns_ GoodMoving counted any error as a failure while running MIGRATE in a loop, although MIGRATE blocks both servers well beyond the 200ms IO timeout the test used. Test_justToCover relied on a hostname in a real TLD resolving to nothing. Cluster behaviour does not depend on the go version, so the cluster stage now runs on the oldest and the newest go only; the other stages keep the full matrix.
TestAllReturns_Good and TestAllReturns_Bad read with MasterAndSlaves policy keys that fillMany wrote to masters 10ms earlier, so a replica lagging behind answered with a miss. In TestAllReturns_Good such a goroutine returned without signalling the channel the test waits on, which turned the mismatch into a 10 minute test binary timeout instead of a failed assertion. Revert the cluster node timeout back to 200ms: the replication race explains the observed failures, and slower failure detection only prolongs the tests that stop nodes.
A shard has no connection to serve a request for a short while after the slot it serves moves to a node the client has not connected to yet, so TestAllReturns_GoodMoving now retries such requests instead of counting them as wrong answers. Its load is also lowered: with 400 goroutines saturating the runner, redis did not converge between the moves and the testbed gave up waiting. An explicit ReconnectPause keeps the dead window after a broken connection short, since it otherwise follows IOTimeout, which both TestAllReturns tests raise to survive a MIGRATE blocking the server they talk to.
The keys a goroutine walks are derived from the goroutine count, so lowering it also lowers how often the migrating slots are requested — with 100 the test stopped seeing MOVED at all.
With a 200ms node timeout the TLS cluster failed to take the seventh node in within a minute, since nodes declare each other failed faster than the bus finishes its handshakes when the runner is busy. For the same reason a TLS connection could not answer PING within the 200ms the plain tests use. WaitClusterOk now reports what every node thought about the cluster before it gives up, which is otherwise unrecoverable from CI logs.
Linux hands out 32768-60999 to outgoing connections, and the tests open hundreds of them, so a client socket could take 43216 before the seventh node tried to listen on it. Ports are chosen below the range now, cluster bus ports (port + 10000) included.
The dump of every node's view does not say which of the conditions WaitClusterOk polls for was not met, and the ones it polls for are not observable after the fact.
Only the source and the destination were told, so a following migration of the same slot could outrun the gossip carrying the previous one. The cluster then had masters pointing at the new owner and the new owner pointing back, with nobody claiming the slot, and it never agreed on the configuration again.
g7r
changed the title
g7r
marked this pull request as ready for review
September 11, 2026 19:40
isopov
self-requested a review
September 11, 2026 19:44
isopov
approved these changes
Sep 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The cluster test suite failed in about one of six matrix cells per push, which
fail-fast then turned into a fully red run.
Slot migration left the cluster permanently inconsistent: FinishMoveSlot told
only the source and the destination who owns the slot now, so a following
migration of the same slot could outrun the gossip carrying the previous one,
after which neither node claimed the slot and no node agreed with another.
The fixed ports the testbed listens on were inside the range Linux hands out to
outgoing connections, and the tests open hundreds of those, so a client socket
could take the port a node was about to listen on.
Several tests raced the cluster instead of waiting for it: reads with the
MasterAndSlaves policy went to replicas that had not received the writes yet,
and TestMasterOnly asserted on an event the background control loop emits
within a window of two check intervals, which an in-flight iteration can miss.
A goroutine of TestAllReturns_Good also left the channel the test waits on
unsignalled when an assertion failed, turning a mismatch into a test binary
timeout.
Timeouts assumed an idle machine: 200ms of IO timeout does not cover a MIGRATE
that blocks the server it is sent to, and 200ms of cluster node timeout is
below what the TLS cluster bus needs to take a new node in when the runner is
busy. Requests are also allowed to fail while a slot has moved to a node the
client has not connected to yet, so the migration test retries them.
WaitClusterOk now reports which of the conditions it polls for was not met,
since none of them is observable after the fact.
The cluster stage runs on the oldest and the newest go only — its behaviour
does not depend on the go version, while each cell costs seven minutes — and
fail-fast is off so one failure no longer hides the state of the other cells.