Tests: pin the pruning threshold deterministically, not end-to-end - #3198
Merged
Conversation
#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
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.
Follow-up to #3197, which fixed one flaw in
NodeAbsentFromTopologyIsPrunedAfterThreeGenerations- it asserted discovery synchronously withConnectAsyncreturning. The failure then moved one line down, toAssert.Contains(doomed, ...)after two generations, still failing about one two-core run in four.Why
The primitive counts elapsed generations, not observed absences:
So any topology application advances it. A test that applies two generations and then asserts "not pruned yet" is really asserting that nothing else applied one - which it does not control. Hence fast machine passes, two-core runner fails.
I tried suppressing the heartbeat first, on the theory that a background pass was the culprit; it did not fix it, so the assertion is the problem rather than any particular background path.
The split
AbsenceIsCountedAsGenerationsElapseddrives the counter directly with explicit generation numbers - deterministic, no connection timing involved. It also documents the elapsed-versus-observed distinction, including that a two-generation gap counts as two, which is precisely what an end-to-end test cannot control.NodeAbsentFromTopologyIsEventuallyPrunedkeeps what it can honestly own: an absent, idle, cluster-discovered node is eventually pruned. Bounded loop, so a regression fails rather than hangs.Verification
taskset -c 0,1with-c Release /p:CI=true(two cores is what a runner has): 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. If we ever want "three observed absences" instead, that is a deliberate change to the primitive and its own PR.
Note in passing: the same two-core setup keeps turning up unrelated pre-existing flakes of the same family (assert-before-ready) -
ScriptingTests.SimpleRawScriptEvaluateandConnectionFailureErrorsTests.SocketFailureError. Not touched here; worth a separate sweep.