diff --git a/XE-Local-AI-Engine.Tests/AgentHome/AgentHomeServiceTests.cs b/XE-Local-AI-Engine.Tests/AgentHome/AgentHomeServiceTests.cs index aa3b4c290..3458dbc22 100644 --- a/XE-Local-AI-Engine.Tests/AgentHome/AgentHomeServiceTests.cs +++ b/XE-Local-AI-Engine.Tests/AgentHome/AgentHomeServiceTests.cs @@ -353,7 +353,7 @@ public async Task RunLifecycleAsync_WhenDifferentOwnerNode_NotBlockedByConcurren identity.NodeId = "node-2"; using var secondCancellation = new CancellationTokenSource(); var second = harness.Service.RunLifecycleAsync(NewLifecycle(folderId), secondCancellation.Token); - await WaitForInFlightCommandCountAsync(provider, count: 2); + await WaitForInFlightCommandCountAsync(provider, count: 2, first, second); // The real assertion: the second run got PAST the guard (two in-flight commands exist) instead of being // rejected with AgentHomeBusy. Both runs are still blocking, so neither has faulted. @@ -876,19 +876,38 @@ private static async Task SwallowAsync(Task task) } } - private static async Task WaitForInFlightCommandCountAsync(FakeSandboxRuntimeProvider provider, int count) + private static async Task WaitForInFlightCommandCountAsync(FakeSandboxRuntimeProvider provider, + int count, + params Task[] runs) { - for (var attempt = 0; attempt < 200; attempt++) + // Deliberately NOT TestBudgets.Contended. The commands this waits on are registered blocking, so they stay + // in flight until the test cancels them: if the count has not been reached, waiting longer does not reach it. + // A 120s budget here bought nothing and made the same failure take two minutes to surface on CI. + var deadline = DateTimeOffset.UtcNow + TimeSpan.FromSeconds(30); + while (DateTimeOffset.UtcNow < deadline) { if (InFlightExecutionIds(provider).Count >= count) { return; } + // A run that faulted will never produce its command, so report why instead of polling out with a + // count that hides the real exception. + foreach (var run in runs) + { + if (run.IsFaulted) + { + throw new InvalidOperationException( + $"A run faulted before {count} command(s) were in flight.", + run.Exception); + } + } + await Task.Delay(10); } - throw new InvalidOperationException($"Expected {count} in-flight command(s) but the fake never reached it."); + throw new InvalidOperationException($"Expected {count} in-flight command(s) but the fake never reached it; " + + $"observed {InFlightExecutionIds(provider).Count}."); } private static IReadOnlyList InFlightExecutionIds(FakeSandboxRuntimeProvider provider) diff --git a/XE-Local-AI-Engine.Tests/Connection/WorkerHubConnectionSignalRIntegrationTests.cs b/XE-Local-AI-Engine.Tests/Connection/WorkerHubConnectionSignalRIntegrationTests.cs index 49f047744..a0c2e287c 100644 --- a/XE-Local-AI-Engine.Tests/Connection/WorkerHubConnectionSignalRIntegrationTests.cs +++ b/XE-Local-AI-Engine.Tests/Connection/WorkerHubConnectionSignalRIntegrationTests.cs @@ -260,7 +260,7 @@ public async Task EncryptedRuntimePackageRoundTrip_WhenWorkerReceivesInvocationA await fixture.SendInvocationAssignedAsync(runtimePackage); - var receivedPackage = await invocationAssigned.Task.WaitAsync(TimeSpan.FromSeconds(5)); + var receivedPackage = await invocationAssigned.Task.WaitAsync(TestBudgets.Contended); AssertEncryptedRuntimePackageEqual(runtimePackage, receivedPackage); var chunkPayload = new EncryptedChunkEnvelopeV1 @@ -495,7 +495,7 @@ public async Task EncryptedRuntimePackageRoundTrip_WhenWorkerReceivesMultiMessag await fixture.SendInvocationAssignedAsync(runtimePackage); - var receivedPackage = await invocationAssigned.Task.WaitAsync(TimeSpan.FromSeconds(5)); + var receivedPackage = await invocationAssigned.Task.WaitAsync(TestBudgets.Contended); AssertEncryptedRuntimePackageEqual(runtimePackage, receivedPackage); await capabilityReporter.Received(1).ReportToApiAsync(Arg.Any()); } @@ -590,7 +590,7 @@ public async Task OnReconnected_WhenConnectionDrops_ReSendsWorkerHelloAndReports await fixture.FireTransportLevelConnectionDropAsync(); - await reconnected.Task.WaitAsync(TimeSpan.FromSeconds(30)); + await reconnected.Task.WaitAsync(TestBudgets.Contended); // OnReconnectedAsync re-sends WorkerHello over the hub and re-reports capabilities via the // capability reporter. The hub observes the second WorkerHello; the reporter observes the @@ -662,7 +662,7 @@ public async Task OnReconnected_WhenCredentialsRevoked_StopsAndTransitionsToErro await fixture.FireTransportLevelConnectionDropAsync(); - await errorReached.Task.WaitAsync(TimeSpan.FromSeconds(30)); + await errorReached.Task.WaitAsync(TestBudgets.Contended); AssertEx.Equal(WorkerConnectionState.Error, connection.State); AssertEx.True(reconnectingObserved, "Expected the connection to attempt at least one reconnect before erroring."); @@ -723,7 +723,7 @@ public async Task Reconnect_WhenTransientRefreshFailure_KeepsReconnecting() await fixture.FireTransportLevelConnectionDropAsync(); - await reconnecting.Task.WaitAsync(TimeSpan.FromSeconds(30)); + await reconnecting.Task.WaitAsync(TestBudgets.Contended); // The transient failure keeps the worker reconnecting; it must not transition to Error. Hold the // observation window open briefly to assert the negative (no Error) deterministically. @@ -775,7 +775,7 @@ public async Task OnReconnected_WhenTokenRefreshFails_TransitionsToError() await fixture.FireTransportLevelConnectionDropAsync(); - await errorReached.Task.WaitAsync(TimeSpan.FromSeconds(30)); + await errorReached.Task.WaitAsync(TestBudgets.Contended); AssertEx.Equal(WorkerConnectionState.Error, connection.State); } diff --git a/XE-Local-AI-Engine.Tests/Hosting/DesktopBootstrapTests.cs b/XE-Local-AI-Engine.Tests/Hosting/DesktopBootstrapTests.cs index 48f566ff2..bf16ec5d0 100644 --- a/XE-Local-AI-Engine.Tests/Hosting/DesktopBootstrapTests.cs +++ b/XE-Local-AI-Engine.Tests/Hosting/DesktopBootstrapTests.cs @@ -14,9 +14,10 @@ namespace XE_Local_AI_Engine.Tests.Hosting; /// // EnsureLocalDataConfiguration resolves the operator secret process-env-first (XE_NODE_SQLITE_KEY) via // NodeOperatorSecretProvider, so the "neither env set" cases require that process-global var to be UNSET while other -// suites (the CUDA env-scrub test, the ranker-registration build) set/read it. The shared NotInParallel key serializes -// this class against them, and the constructor/Dispose save-then-clear-then-restore the var so a serialized-but-leaky -// sibling can never poison the next test's "neither set" premise. +// suites (the CUDA env-scrub test, the ranker-registration build) set/read it. The bare NotInParallel below runs this +// class exclusively — not merely against a shared key — which serializes it against them, and the constructor/Dispose +// save-then-clear-then-restore the var so a serialized-but-leaky sibling can never poison the next test's "neither +// set" premise. [NotInParallel] public sealed class DesktopBootstrapTests : IDisposable { diff --git a/XE-Local-AI-Engine.Tests/Hubs/ServerPushHubTests.cs b/XE-Local-AI-Engine.Tests/Hubs/ServerPushHubTests.cs index 704adcbe9..27ea51864 100644 --- a/XE-Local-AI-Engine.Tests/Hubs/ServerPushHubTests.cs +++ b/XE-Local-AI-Engine.Tests/Hubs/ServerPushHubTests.cs @@ -81,7 +81,7 @@ public async Task KnowledgeIndexingNotifier_PushIsReceivedByAnAuthorizedClient() await Factory.Services.GetRequiredService() .NotifyDocumentChangedAsync(documentId, KnowledgeDocumentStatus.Embedding); - var evt = await received.Task.WaitAsync(TimeSpan.FromSeconds(10)); + var evt = await received.Task.WaitAsync(TestBudgets.Contended); AssertEx.Equal(KnowledgeBaseHubEvents.DocumentChanged, evt.EventType); AssertEx.Equal(documentId, evt.DocumentId); AssertEx.Equal(KnowledgeDocumentStatus.Embedding, evt.Status); @@ -102,7 +102,7 @@ public async Task GgufDownloadEventPublisher_PushIsReceivedByAnAuthorizedClient( SanitizedError: null); await Factory.Services.GetRequiredService().PublishStatusAsync(published); - var evt = await received.Task.WaitAsync(TimeSpan.FromSeconds(10)); + var evt = await received.Task.WaitAsync(TestBudgets.Contended); AssertEx.Equal(published.ModelName, evt.ModelName); AssertEx.Equal("Running", evt.Phase); AssertEx.Equal(expected: 512L, evt.CompletedBytes); @@ -128,7 +128,7 @@ public async Task RuntimeAcquisitionEventPublisher_PushIsReceivedByAnAuthorizedC SanitizedError: null); await Factory.Services.GetRequiredService().PublishStatusAsync(published); - var evt = await received.Task.WaitAsync(TimeSpan.FromSeconds(10)); + var evt = await received.Task.WaitAsync(TestBudgets.Contended); AssertEx.Equal(expected: 7L, evt.Sequence); AssertEx.Equal(nameof(RuntimeAcquisitionPhase.Downloading), evt.Phase); AssertEx.Equal("Cuda", evt.Variant); diff --git a/XE-Local-AI-Engine.Tests/Providers/LlamaServer/LlamaCppSourceBuildServiceTests.cs b/XE-Local-AI-Engine.Tests/Providers/LlamaServer/LlamaCppSourceBuildServiceTests.cs index a9b6410cb..124a17d6e 100644 --- a/XE-Local-AI-Engine.Tests/Providers/LlamaServer/LlamaCppSourceBuildServiceTests.cs +++ b/XE-Local-AI-Engine.Tests/Providers/LlamaServer/LlamaCppSourceBuildServiceTests.cs @@ -560,7 +560,13 @@ public async Task Start_OfficialCpu_UsesPinnedCommitScrubbedGitAndCpuMatrix() AssertEx.False(environment.Contains("must-not-leak", StringComparison.Ordinal)); AssertEx.True(environment.Contains("GIT_CONFIG_NOSYSTEM=1", StringComparison.Ordinal)); AssertEx.True(environment.Contains($"HOME={Path.Combine(temp.Path, "llama.cpp", "source-build", ".work", ".home")}", StringComparison.Ordinal)); - AssertEx.Null(activity.ActiveBuildId); + + // The reservation is released in the build task's finally, AFTER the phase goes terminal, so the + // Terminal wait above does not imply it has happened yet. Asserting it directly races that release + // and loses whenever the build task is descheduled between the two. + await AssertEx.EventuallyAsync(() => activity.ActiveBuildId is null, + TestBudgets.Contended, + "The source build stayed reserved after reaching a terminal phase."); } finally { diff --git a/XE-Local-AI-Engine.Tests/Testing/TestBudgets.cs b/XE-Local-AI-Engine.Tests/Testing/TestBudgets.cs new file mode 100644 index 000000000..d6ea04bac --- /dev/null +++ b/XE-Local-AI-Engine.Tests/Testing/TestBudgets.cs @@ -0,0 +1,25 @@ +namespace XE_Local_AI_Engine.Tests.Testing; + +/// +/// Wall-clock budgets for waits that must survive a contended CI runner. +/// +/// +/// +/// CI runs this module as four concurrent group processes with coverage instrumentation on a four-core +/// runner (see the TEST_GROUPS note in scripts/run-tests-memory-safe.sh), so a wait that is comfortable +/// on an idle developer box can be starved for seconds at a time. Budgets sized against local timings +/// are what turn that starvation into a red build on work that changed nothing. +/// +/// +/// These are failure deadlines, not sleeps: every consumer polls or awaits a completion and returns the +/// moment the condition holds, so a generous budget costs nothing on a green run and only decides how +/// long a genuinely stuck test waits before reporting. +/// +/// +internal static class TestBudgets +{ + /// + /// Deadline for an asynchronous condition that is expected within milliseconds when the machine is idle. + /// + public static readonly TimeSpan Contended = TimeSpan.FromSeconds(120); +} diff --git a/docs/agent-knowledge.md b/docs/agent-knowledge.md index 3c9f2bf20..6865c7344 100644 --- a/docs/agent-knowledge.md +++ b/docs/agent-knowledge.md @@ -179,10 +179,20 @@ A targeted `--treenode-filter "/*/*//*"` run is great for a fast loop ### The full Tests module is flaky under parallelism — verify suspects in isolation -Running the entire `XE-Local-AI-Engine.Tests` module concurrently produces a **non-deterministic** failure count (observed 1/4/5/21) from tests that mutate **process-global** state racing each other — chiefly `DesktopBootstrapTests` (`EnsureLocalDataConfiguration_*`, which set/clear `XE_NODE_SQLITE_KEY` and write key files) and `EmbeddingPlaybookRetrievalRankerTests`. They **pass in isolation** (`--treenode-filter` per class). So a red full-module run is not automatically a real failure — re-run the named suspects alone before believing it. Related trap: the module has **conflicting env premises** — `DesktopBootstrapTests` `WhenNeitherEnvSet_*` require `XE_NODE_SQLITE_KEY` **unset**, while `PlaybookRetrievalRankerRegistrationTests.AddServices_ResolvesBoth…` requires it **set** (base64 of 32 bytes, not hex) — so you **cannot** satisfy the whole module with one ambient env var. (That follow-up is done where it is safe: both named suspects now carry `[NotInParallel("XE_NODE_SQLITE_KEY")]`.) +Running the entire `XE-Local-AI-Engine.Tests` module concurrently produces a **non-deterministic** failure count (observed 1/4/5/21) from tests that mutate **process-global** state racing each other — chiefly `DesktopBootstrapTests` (`EnsureLocalDataConfiguration_*`, which set/clear `XE_NODE_SQLITE_KEY` and write key files) and `EmbeddingPlaybookRetrievalRankerTests`. They **pass in isolation** (`--treenode-filter` per class). So a red full-module run is not automatically a real failure — re-run the named suspects alone before believing it. Related trap: the module has **conflicting env premises** — `DesktopBootstrapTests` `WhenNeitherEnvSet_*` require `XE_NODE_SQLITE_KEY` **unset**, while `PlaybookRetrievalRankerRegistrationTests.AddServices_ResolvesBoth…` requires it **set** (base64 of 32 bytes, not hex) — so you **cannot** satisfy the whole module with one ambient env var. (That follow-up is done where it is safe, though **not** in the shape this line used to claim: only `PlaybookRetrievalRankerRegistrationTests` carries the keyed `[NotInParallel("XE_NODE_SQLITE_KEY")]`. `DesktopBootstrapTests` carries a **bare** `[NotInParallel]`, which is stronger — see the next section — so the serialization is real and must not be "fixed" into a key. Corrected 2026-08-23. While you are here: only **two** classes actually write that process-global var, `CudaBuildServiceTests` and `LlamaCppSourceBuildServiceTests`, and both already run exclusively. Grepping the name finds ~8 files, but the rest either mention it in a comment, seed it into a test-host **configuration dictionary**, or set it on a **child** `ProcessStartInfo.Environment` — none of which touches the parent process. Do not "harden" those.) **Do not "optimize" the remaining bare `[NotInParallel]` attributes into keyed groups.** Verified 2026-08-15: in TUnit a bare `[NotInParallel]` means *run exclusively* (a terminal serial phase after everything else), and for most of the ~28 bare sites that exclusivity is load-bearing, not laziness. The meter-capture classes (`NodeMeterCapture` copies in `Telemetry/`, `Invocation/`, `Mcp/`, `Agents/`) subscribe by **meter name** (`XE.Node`) — several with no instrument filter at all — so *any* concurrently running test that drives product code emitting on that meter (every `TestServerWebAppFactory` host, every `McpAgentRunMetrics` instantiation) contaminates the capture window. A keyed group cannot express "nothing else may emit while I listen"; only run-alone can, unless every emitter in the module were keyed too. Likewise the `PATH`-stub suites (`CudaBuildServiceTests` and friends) must not overlap *any* test that spawns a real `git`/`cmake`, not just each other. The parallelism win comes from process- and module-level parallelism instead (see the memory-safe runner's `JOBS` below — CI itself no longer overlaps modules, see the per-project loop above), which these attributes do not constrain. +### A wall-clock budget sized on an idle box is a CI flake waiting to happen — use `TestBudgets.Contended` + +CI runs this module as **four concurrent group processes with coverage instrumentation on a four-core runner**, so a wait that completes in milliseconds locally can be starved for tens of seconds there. On 2026-08-23 the nuget-remaining bump PR went red on **four consecutive** CI runs with **five different** tests — `GgufDownloadEventPublisher_*` and `KnowledgeIndexingNotifier_*` (10 s), `Reconnect_WhenTransientRefreshFailure_KeepsReconnecting` (30 s), `RunLifecycleAsync_WhenDifferentOwnerNode_*` (a 200 × 10 ms poll = 2 s), and `Start_OfficialCpu_*` — while the same commit ran green five times locally. None was a product defect; every budget was simply sized against a quiet machine. Different tests failing each run is the signature: a real regression fails the *same* test. + +`TestBudgets.Contended` (Testing/TestBudgets.cs) is the deadline to use. These are **failure deadlines, not sleeps** — each consumer polls or awaits and returns the instant the condition holds, so a generous value costs nothing on a green run. Never raise one that a test *expects* to expire (a negative assertion held open by `Task.Delay`, or a `WaitAsync` whose `TimeoutException` is the assertion) — that trades a flake for minutes of dead wall-clock. + +**Raise a budget only where the thing waited on can still arrive.** `RunLifecycleAsync_WhenDifferentOwnerNode_NotBlockedByConcurrentRun` looked like the same class of problem — a 200 × 10 ms poll, failing at 2s368ms — so it was moved to the 120s budget too. It then failed again at exactly **2m00s**. Its commands are registered *blocking*, so they stay in flight until the test cancels them: if the second one has not arrived, more waiting will not make it arrive, and the only thing the bigger budget bought was a two-minute-slower red. It is back on 30s and now inspects the run tasks for a fault first, so a run that died on its way to the sandbox reports *its* exception instead of an unhelpful count. That underlying non-arrival is still open — treat this test as a known suspect, not as fixed. + +**The related trap is asserting a post-condition the wait above did not actually synchronize.** `Start_OfficialCpu_*` waited for `GetStatus().Terminal` and then asserted `activity.ActiveBuildId is null` directly — 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. When a wait and an assertion observe *different* signals, the assertion needs its own `AssertEx.EventuallyAsync`. + ### Leftover build daemons starve the timing-sensitive tests — and the packaging gate is where you notice The third reason a red run may not be real, and the one that bites at exactly the worst moment. It is neither of the two below it: no process-global state is racing, and no assembly is being rewritten. The machine is simply **busy**, and a handful of tests spawn real child processes against fixed wall-clock budgets.