Tests: don't assume endpoint discovery is synchronous with connect - #3197
Merged
Conversation
EndpointPruningUnitTests.NodeAbsentFromTopologyIsPrunedAfterThreeGenerations fails on CI roughly one run in four, at the Assert.Contains *before* any pruning is involved - so the test never reached the behaviour it exists to check. A node learned from the CLUSTER SLOTS reply during handshake is registered on the connection's own path, and that does not have to have finished by the time ConnectAsync returns; GetEndPoints() is a snapshot. The invariant these tests care about is that a discovered node appears *promptly*, not synchronously, so they now poll for it. Reproducing this locally needs the CPU constraint, not load: `taskset -c 0,1` plus `-c Release /p:CI=true` reproduces it at about the CI rate, while no amount of parallel load on 14 cores ever did. Verified on main both ways - failing 1-in-4 before, 0-in-6 after. Swept the sibling suites for the same assumption. Most assertions on GetEndPoints() are about *configured* endpoints (present from the start) or follow awaited topology work, so only the two that assert on discovery needed changing: the one above, and NewNodeIsDialledByTheAdvertisedForm.
Poll.UntilAsync had its own 5s timeout and ignored the framework's, so a cancelled or timed-out test would keep polling to its own schedule, and a predicate that never comes true reported false on our clock rather than surfacing the framework's cancellation. The token goes to Task.Delay rather than being probed between polls, so cancellation lands inside the current interval instead of after it. The local timeout stays as a backstop for any caller with no ambient limit.
mgravell
added a commit
that referenced
this pull request
Aug 25, 2026
…3198) #3197 fixed one flaw in NodeAbsentFromTopologyIsPrunedAfterThreeGenerations - it asserted discovery synchronously. The failure then moved one line down, to "two absences is not yet evidence", still around one two-core run in four. The reason is in the primitive: OnMissingFromTopology returns `generation - absentSince + 1`, which is the number of generations *elapsed*, not the number of times this server was observed absent. So any topology application, from anywhere, advances it - and a test that applies two generations and then asserts "not pruned yet" is really asserting that nothing else applied one. It cannot control that, which is why it passes on a fast machine and fails on a two-core runner. Suppressing the heartbeat was tried first and did not fix it, so the assertion is the problem rather than any particular background path. Split accordingly: - AbsenceIsCountedAsGenerationsElapsed drives the counter directly with explicit generation numbers. Deterministic, and it documents the elapsed-versus-observed distinction - including that a two-generation gap counts as two, which is the thing an end-to-end test cannot control. - NodeAbsentFromTopologyIsEventuallyPruned keeps what it can honestly own: an absent, idle, cluster-discovered node is eventually pruned. Bounded, so a regression fails rather than hangs. Verified with `taskset -c 0,1` and -c Release /p:CI=true: 6 clean whole-suite runs, against roughly 1 in 4 failing before. No library change - the product behaviour is fine, and arguably intended: three generations elapsed is three generations elapsed, whoever applied them.
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
1 participant
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.
EndpointPruningUnitTests.NodeAbsentFromTopologyIsPrunedAfterThreeGenerationsfails on CI roughly one run in four, and it fails at theAssert.Containsbefore any pruning happens - so the test was never reaching the behaviour it exists to check. Seen on #3193; confirmed to reproduce onmain, so it is pre-existing rather than from that stack.Cause
A node learned from the
CLUSTER SLOTSreply during handshake is registered on the connection's own path, and that does not have to have completed by the timeConnectAsyncreturns -GetEndPoints()is a snapshot. The test asserted the discovered node was present the instant connect returned, which is an ordering the library never promised. The invariant it actually cares about is that a discovered node appears promptly, so it now polls.Reproducing it, which is the useful part
It needs CPU constraint, not load:
Two cores is what a GitHub runner has. On
main, that fails about 1 run in 4; with this change, 0 in 6. Six whole-suite runs under heavy parallel load on 14 cores never reproduced it once, which is why it looked like a mystery - and is worth remembering for the next runner-only failure.Scope
Swept the sibling suites (
EndpointPruningUnitTests,EndpointResolutionUnitTests,ServerRetirementUnitTests) for the same assumption. Most assertions onGetEndPoints()are about configured endpoints - present from the start, so not racy - or follow awaited topology work. Only two assert on discovery: the one above, andNewNodeIsDialledByTheAdvertisedForm. Both now poll.Poll.UntilAsyncis new (Helpers/Poll.cs);TestBase.UntilConditionAsyncalready exists but isprotected, and these suites don't derive fromTestBase.Test-only change; no library code touched.
Note in passing: the same two-core setup also turned up a different pre-existing flake,
ScriptingTests.SimpleRawScriptEvaluate (RESP3)(1 run in 6, fails in 3ms). Not touched here.