test: test-isolation hardening for the group2 contended-runner races - #28
Conversation
CI runs this module as four concurrent group processes with coverage
instrumentation on a four-core runner, so waits sized against a quiet
developer box get starved there. The nuget-remaining bump PR went red on
four consecutive runs with five different tests while the same commit ran
green five times locally -- different tests each run, which is a race, not
a regression.
One of the five was a genuine test bug rather than a tight budget.
Start_OfficialCpu_UsesPinnedCommitScrubbedGitAndCpuMatrix waited for
GetStatus().Terminal and then asserted activity.ActiveBuildId was null,
but LlamaCppSourceBuildService sets the terminal phase and only releases
the reservation afterwards, in the build task's finally. The assertion
raced that release and lost whenever the task was descheduled between the
two, so it now waits for the signal it actually depends on.
The rest were budgets: a 200 x 10ms poll (two seconds, which is what the
2s368ms failure had spent), three ten-second hub pushes, and six worker
hub waits. They move to TestBudgets.Contended. These are failure
deadlines, not sleeps -- each site polls or awaits and returns as soon as
the condition holds, so a generous value costs a green run nothing. Every
one was checked to be a positive wait; a budget whose expiry IS the
assertion must keep its tight value.
Also correct two claims that no longer matched the code: only
PlaybookRetrievalRankerRegistrationTests carries the keyed
NotInParallel("XE_NODE_SQLITE_KEY"), and only two classes actually write
that process-global var -- both already run exclusively under a bare
NotInParallel, which is the stronger guarantee and must not be traded for
a key. Grepping the name finds eight files; the other six use a
configuration dictionary or a child process environment.
Raising this poll to the 120s contended budget was wrong. Its commands are registered blocking, so they stay in flight until the test cancels them: if the second command has not arrived, waiting longer does not make it arrive. CI proved it -- the same failure came back at exactly 2m00s instead of 2s368ms, so the only thing the bigger budget bought was a two-minute-slower red. Back to 30s, and check the run tasks for a fault while polling. A run that died on its way to the sandbox never produces its command, and the old message blamed "the fake" for it; now it surfaces that run's exception. The count is included in the timeout message too, so the next occurrence says how far it actually got. The non-arrival itself is still unexplained and stays open.
|
CI caught a mistake in the first commit, fixed in I had moved It is back on 30s and now inspects the run tasks for a fault while polling, so a run that died on its way to the sandbox reports its exception rather than blaming the fake, and the timeout message now includes how many commands were actually observed. To be clear about what this does and does not fix: the non-arrival of that second command under group2 contention is still unexplained and still open. This PR stops it wasting two minutes and makes the next occurrence diagnostic; it does not claim to fix it. The knowledge-doc entry says the same and flags the test as a known suspect. The other budget changes stand — those sites are genuine timeout-shaped failures where the awaited event can still arrive. |
1 participant
Test-code and docs only. No product code changes.
Why
XE-Local-AI-Engine.Testsruns in CI as four concurrent group processes with coverage instrumentation on a four-core runner. Waits sized against a quiet developer box get starved there.PR #19 (the nuget-remaining bump) went red on four consecutive CI runs with five different tests, while the same commit ran the full suite green five times locally:
GgufDownloadEventPublisher_PushIsReceivedByAnAuthorizedClientRunLifecycleAsync_WhenDifferentOwnerNode_NotBlockedByConcurrentRunStart_OfficialCpu_UsesPinnedCommitScrubbedGitAndCpuMatrixReconnect_WhenTransientRefreshFailure_KeepsReconnectingKnowledgeIndexingNotifier_PushIsReceivedByAnAuthorizedClientDifferent tests each run is the signature of a race, not a regression — a regression fails the same test every time. Two of the five sites (
AgentHomeServiceTests,LlamaCppSourceBuildServiceTests) cannot touch any package #19 bumps, and #19 showed no slowdown (18m14s/18m53s against 18m29s–19m10s for its green siblings).What changed
1. A real test bug, not a tight budget.
Start_OfficialCpu_*waited forGetStatus().Terminal, then assertedactivity.ActiveBuildId is nullimmediately. ButLlamaCppSourceBuildServicesets the terminal phase (SetTerminalAsync, line 516) and only releases the reservation afterwards, in the build task'sfinally(line 541). The assertion raced that release and lost whenever the task was descheduled between the two. It now waits for the signal it actually depends on. This is why it failed at 30ms — an assertion, not a timeout.2.
Testing/TestBudgets.cs— one documentedContendeddeadline, with the reasoning for why a local-timed budget is a CI flake waiting to happen.3. Budgets moved onto it — the 200 × 10 ms poll in
AgentHomeServiceTests.WaitForInFlightCommandCountAsync(exactly 2 s, which is precisely what the 2s368ms failure had spent), the three 10 s pushes inServerPushHubTests, and six waits inWorkerHubConnectionSignalRIntegrationTests.These are failure deadlines, not sleeps — every site polls or awaits and returns the instant the condition holds, so a generous value costs a green run nothing. I checked that all ten are positive waits followed by assertions on the received value. A budget whose expiry is the assertion must keep its tight value, so the ~20 other short budgets in
InvocationRunnerTests,StreamIdleWatchdogTestsand friends are deliberately untouched.4. Two documentation claims corrected.
docs/agent-knowledge.mdsaid both named suspects carry the keyed[NotInParallel("XE_NODE_SQLITE_KEY")]; onlyPlaybookRetrievalRankerRegistrationTestsdoes.DesktopBootstrapTestscarries a bare[NotInParallel]— which is stronger (exclusive), so it must not be "fixed" into a key — and its own header comment claimed the opposite. Also recorded: grepping that variable finds eight files, but only two actually write the process-global one (CudaBuildServiceTests,LlamaCppSourceBuildServiceTests, both already exclusive); the rest use a test-host configuration dictionary or a childProcessStartInfo.Environment.No
[NotInParallel]attribute was changed. The audit found no isolation gap to close.Validation
--no-incremental): 0 errors, 0 warnings.TEST_GROUPS=4,JOBS=4, coverage, pinned to 4 CPUs): 2/2 green, 6865 pass.Honest caveat on the A/B. I tried to demonstrate the fix holds where the race fires by running clean develop and this branch under six extra CPU spinners. The baseline stayed green (0/2) — the race is low-rate and did not reproduce on demand locally, so that comparison demonstrates nothing either way. This PR rests on the mechanism analysis above, in particular the provable
Terminal-vs-release ordering bug and the 2 s poll budget exactly matching its observed failure duration — not on a local reproduction.