Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 23 additions & 4 deletions XE-Local-AI-Engine.Tests/AgentHome/AgentHomeServiceTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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<string> InFlightExecutionIds(FakeSandboxRuntimeProvider provider)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<CancellationToken>());
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.");
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);
}
Expand Down
7 changes: 4 additions & 3 deletions XE-Local-AI-Engine.Tests/Hosting/DesktopBootstrapTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,10 @@ namespace XE_Local_AI_Engine.Tests.Hosting;
/// </summary>
// 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
{
Expand Down
6 changes: 3 additions & 3 deletions XE-Local-AI-Engine.Tests/Hubs/ServerPushHubTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ public async Task KnowledgeIndexingNotifier_PushIsReceivedByAnAuthorizedClient()
await Factory.Services.GetRequiredService<IKnowledgeIndexingNotifier>()
.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);
Expand All @@ -102,7 +102,7 @@ public async Task GgufDownloadEventPublisher_PushIsReceivedByAnAuthorizedClient(
SanitizedError: null);
await Factory.Services.GetRequiredService<IGgufDownloadEventPublisher>().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);
Expand All @@ -128,7 +128,7 @@ public async Task RuntimeAcquisitionEventPublisher_PushIsReceivedByAnAuthorizedC
SanitizedError: null);
await Factory.Services.GetRequiredService<IRuntimeAcquisitionEventPublisher>().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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down
25 changes: 25 additions & 0 deletions XE-Local-AI-Engine.Tests/Testing/TestBudgets.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
namespace XE_Local_AI_Engine.Tests.Testing;

/// <summary>
/// Wall-clock budgets for waits that must survive a contended CI runner.
/// </summary>
/// <remarks>
/// <para>
/// 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.
/// </para>
/// <para>
/// 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.
/// </para>
/// </remarks>
internal static class TestBudgets
{
/// <summary>
/// Deadline for an asynchronous condition that is expected within milliseconds when the machine is idle.
/// </summary>
public static readonly TimeSpan Contended = TimeSpan.FromSeconds(120);
}
12 changes: 11 additions & 1 deletion docs/agent-knowledge.md
Original file line number Diff line number Diff line change
Expand Up @@ -179,10 +179,20 @@ A targeted `--treenode-filter "/*/*/<YourClass>/*"` 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.
Expand Down