Skip to content

Tests: pin the pruning threshold deterministically, not end-to-end - #3198

Merged
mgravell merged 1 commit into
mainfrom
marc/fix-pruning-generation-race
Aug 25, 2026
Merged

Tests: pin the pruning threshold deterministically, not end-to-end#3198
mgravell merged 1 commit into
mainfrom
marc/fix-pruning-generation-race

Conversation

@mgravell

Copy link
Copy Markdown
Collaborator

Follow-up to #3197, which fixed one flaw in NodeAbsentFromTopologyIsPrunedAfterThreeGenerations - it asserted discovery synchronously with ConnectAsync returning. The failure then moved one line down, to Assert.Contains(doomed, ...) after two generations, still failing about one two-core run in four.

Why

The primitive counts elapsed generations, not observed absences:

internal int OnMissingFromTopology(int generation)
{
    if (AbsentSinceGeneration < 0) { AbsentSinceGeneration = generation; return 1; }
    return generation - AbsentSinceGeneration + 1;
}

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

  • AbsenceIsCountedAsGenerationsElapsed drives 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.
  • NodeAbsentFromTopologyIsEventuallyPruned keeps 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,1 with -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.SimpleRawScriptEvaluate and ConnectionFailureErrorsTests.SocketFailureError. Not touched here; worth a separate sweep.

#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.
@mgravell
mgravell merged commit 322688f into main Aug 25, 2026
6 checks passed
@mgravell
mgravell deleted the marc/fix-pruning-generation-race branch August 25, 2026 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant