Repository navigation
Conversation
The heaviest project in the suite and the one that sets the shard floor
(shard-assign.sh: "The floor for ANY sharding is the heaviest single project").
threads=1 512 tests, 0 failed, 314.7s
threads=4 512 tests, 0 failed, 122.5s 2.57x
Held back in #979 because threads=4 red two tests. Both are now resolved, and
neither by suppression:
1. StaleActivationDurableFirstSeedTest (durable-seed / routing-cache) — GONE.
#980 closed it as already fixed by the store-level write CAS (#971 / PR #988).
The read-back of a value BELOW the one written was never a visibility lag; it
was a concurrent rollback, which the CAS now refuses at the store. Verified:
it does not reproduce at threads=4 on current main.
2. The GC-reachability family — SCOPED, and this is the part worth reading.
## Why a serial collection here is scoping, not a band-aid
Four classes assert PROCESS-WIDE reachability — "after disposing X, nothing still
roots it" — with WeakReference plus a forced full GC.Collect:
MeshHubDisposalLeakTest, NodeTypeAssemblyLeakTest,
KernelScriptMemoryLeakTest, ConsoleCaptureExecutionContextLeakTest
That assertion is only meaningful in a quiet process. With neighbours allocating
and holding live graphs, a forced collect cannot be relied on to reclaim the
target, so the test reports a "leak" that is really a busy heap. Under parallelism
such a test is not flaky — it is WRONG, because its precondition no longer holds.
Declaring that precondition via [CollectionDefinition(DisableParallelization=true)]
IS the fix. A retry, a widened timeout or a [Skip] would have been the band-aid.
All four are tagged, not only the one that failed locally: NodeTypeAssemblyLeakTest
red here, while MeshHubDisposalLeakTest red identically on CI run 31300348689 this
morning and passed this run purely on timing. They are the same defect class.
🚨 The collection is for reachability assertions ONLY. A test that is merely slow,
or that races on shared STATE, must not be added — that is a real defect and
serialising it would hide it. The type doc says so.
Everything else in the assembly still runs at maxParallelThreads: 4.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Enables intra-project test parallelism for MeshWeaver.Hosting.Monolith.Test to reduce CI wall-clock time, while explicitly isolating GC reachability/leak-assertion tests into a non-parallel xUnit collection so their process-wide WeakReference + forced GC.Collect assertions remain valid under parallel execution.
Changes:
- Opts
test/MeshWeaver.Hosting.Monolith.Testinto xUnit parallel execution (maxParallelThreads: 4,parallelizeTestCollections: true). - Introduces a dedicated
GcReachabilityCollectionwithDisableParallelization = truefor process-wide GC reachability assertions. - Tags the existing reachability/leak tests to run under that collection so they don’t execute alongside allocating “neighbour” tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/MeshWeaver.Hosting.Monolith.Test/xunit.runner.json | Opts this test project into 4-thread parallel execution. |
| test/MeshWeaver.Hosting.Monolith.Test/GcReachabilityCollection.cs | Adds a non-parallel collection definition for process-wide GC reachability assertions. |
| test/MeshWeaver.Hosting.Monolith.Test/NodeTypeAssemblyLeakTest.cs | Runs the reachability/leak test in the dedicated non-parallel collection. |
| test/MeshWeaver.Hosting.Monolith.Test/MeshHubDisposalLeakTest.cs | Runs the reachability/leak test in the dedicated non-parallel collection. |
| test/MeshWeaver.Hosting.Monolith.Test/KernelScriptMemoryLeakTest.cs | Runs the reachability/leak test in the dedicated non-parallel collection. |
| test/MeshWeaver.Hosting.Monolith.Test/ConsoleCaptureExecutionContextLeakTest.cs | Runs the reachability/leak test in the dedicated non-parallel collection. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Test Results (shard 1)1 199 tests 1 198 ✅ 8m 40s ⏱️ For more details on these failures, see this check. Results for commit 5ac02b0. |
Test Results (shard 0)685 tests 685 ✅ 13m 14s ⏱️ Results for commit 5ac02b0. |
Test Results (shard 2)1 912 tests 1 909 ✅ 8m 59s ⏱️ For more details on these failures, see this check. Results for commit 5ac02b0. |
Test Results (shard 4)1 284 tests 1 279 ✅ 6m 46s ⏱️ Results for commit 5ac02b0. |
Test Results (shard 5)1 725 tests 1 533 ✅ 5m 52s ⏱️ Results for commit 5ac02b0. |
Test Results (shard 3) 11 files 11 suites 5m 42s ⏱️ Results for commit 5ac02b0. |
Test Results 65 files 65 suites 49m 15s ⏱️ For more details on these failures, see this check. Results for commit 5ac02b0. |
|
Closing — not ready. My GC scoping was necessary but not sufficient, and CI found two more parallelism-caused failures that my local run did not. What CI red on (run 31327487555)Neither is a GC-reachability test, so neither is covered by the Why my local run missed themLocal: 512 tests, 0 failed, 122.5s at That is a reusable lesson for this repo: a local parallel green on a many-core dev machine is not evidence for a 4-vCPU runner. It is the same mistake shape as trusting a macOS green for The two failures are worth someone's attention, separately
What survivesThe measurement stands and is worth keeping: 314.7s → 122.5s (2.57x) on the heaviest project in the suite, and the two blockers from #979 are now one — the durable-seed failure is genuinely gone, fixed by the write CAS.
The branch Meanwhile#1035 parallelises |
Fixes the root cause behind #1040 and re-lands #1035. ## The defect `MonolithMeshTestBase.TestPartition` was `public const string = "TestData"`, so EVERY test class in EVERY project wrote into one partition — and a partition has a single owning hub that serialises every write routed through it. Harmless while classes run one at a time; a contention point the moment they do not. That is why #1035 (AI.Test parallel) red on CI: waits that never emitted, in a class that is green in isolation under identical CPU pressure (ThreadTokenUsageTest: 8/8 in 5.6 s alone, failing alongside 1,130 neighbours). Not a timeout budget — 🚨 do not "fix" that shape by widening the wait. ## The fix `TestPartition` becomes `public virtual`, DEFAULT UNCHANGED ("TestData"), so no other project moves. Isolation is opt-in per project base — here, one override: // AITestBase public override string TestPartition => $"TestData{GetType().Name}"; giving all 39 AI.Test classes their own partition from a single line. ## Cascade the compiler forced (each is a hard error, never a silent change) 42 static-qualified refs (MonolithMeshTestBase.TestPartition) -> instance, 15 files 3 hardcoded node paths in ThreadComposerFlowTest -> $"{TestPartition}/…" 2 CS1736 default-parameter values -> null-default + `?? TestPartition` 2 CS0236 static field initialisers -> instance properties The 20 other "TestData" strings in AI.Test are `Path.Combine(AppContext.BaseDirectory, "TestData")` — on-disk directories, unrelated to the partition — and are untouched. 🚨 The CS1736 sites needed the `?? TestPartition` coalesce: `string? ns = null` alone would have passed null where the partition used to be — a silent behaviour change the compiler could NOT have caught. ## Verified on a representative machine, not this one A dev Mac has 18 cores; the runner has 4, so "maxParallelThreads: 4" means something completely different on each — which is exactly why #1033 and #1035 passed locally and red on CI. `DOTNET_PROCESSOR_COUNT=4` makes .NET size its thread pool and GC as if it had 4 cores: before fix, DOTNET_PROCESSOR_COUNT=4 : 1138 tests, 2 FAILED (ThreadTokenUsageTest) after fix, DOTNET_PROCESSOR_COUNT=4 : 1138 tests, 0 failed, 117 s Serial baseline for reference: 198.2 s (2.11x). Hosting.Monolith.Test (#1033, measured 2.57x) is NOT re-enabled here — it also needs GcReachabilityCollection from branch perf/share-mesh-cluster. One project at a time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The heaviest project in the suite and the one that sets the shard floor
(shard-assign.sh: "The floor for ANY sharding is the heaviest single project").
threads=1 513 tests, 0 failed, 317.6s
threads=4 513 tests, 0 failed, 110.2s 2.88x
Both measured with DOTNET_PROCESSOR_COUNT=4 — the runner's core count, not this
18-core dev box. That distinction is why the earlier attempt (#1033) passed
locally and red on CI: "4 threads" is four competing for four cores there and
four spread across eighteen here.
## GcReachabilityCollection — scoping, not a band-aid
Four classes assert PROCESS-WIDE reachability ("after disposing X, nothing still
roots it") with WeakReference + a forced full GC.Collect:
MeshHubDisposalLeakTest, NodeTypeAssemblyLeakTest,
KernelScriptMemoryLeakTest, ConsoleCaptureExecutionContextLeakTest
Test classes do NOT share a mesh (ShareMeshClusterEnabled is false, so every
[Fact] builds its own), but they DO share the heap. With neighbours allocating,
a forced collect cannot be relied on to reclaim the target, so the test reports a
"leak" that is really a busy heap. Under parallelism such a test is not flaky —
it is WRONG, because its precondition no longer holds.
[CollectionDefinition(DisableParallelization = true)] declares that precondition.
A retry, a widened timeout or a [Skip] would have been the band-aid.
🚨 The collection is for reachability assertions ONLY. A test that is merely slow,
or races on shared STATE, must not be added — that is a real defect and
serialising it would hide it. The type doc says so.
## The two other #1033 failures are gone
PixelFidelityOfferedOnlyWhenAvailableTest and StoreLevelWriteConflictTest do not
reproduce on current main at threads=4. Consistent with the store-level write CAS
(#971/#988) having hardened the path StoreLevelWriteConflictTest exercises — the
same fix that closed #980.
Verified: full solution build clean under CI's flags
(MW_ALLOW_SOLUTION_BUILD=1 dotnet build MeshWeaver.slnx -c Release -p:CIRun=true
-warnaserror). Building only the touched project is what let CS0236 reach CI on an
earlier attempt.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The heaviest project in the suite, and the one that sets the shard floor —
shard-assign.sh: "The floor for ANY sharding is the heaviest single project."Held back in #979 because
threads=4red two tests. Both are now resolved, neither by suppression.1. The durable-seed failure is gone
StaleActivationDurableFirstSeedTestwas the blocker in #979. #980 closed it as already fixed by the store-level write CAS (#971 / PR #988): the read-back of a value below the one written was never a visibility lag, it was a concurrent rollback, which the CAS now refuses at the store. Verified — it does not reproduce atthreads=4on current main.2. The GC-reachability family is scoped — and this is the part worth reading
Four classes assert process-wide reachability ("after disposing X, nothing still roots it") using
WeakReference+ a forced fullGC.Collect:MeshHubDisposalLeakTestNodeTypeAssemblyLeakTestKernelScriptMemoryLeakTestConsoleCaptureExecutionContextLeakTestThat assertion is only meaningful in a quiet process. With neighbours concurrently allocating and holding live object graphs, a forced collect cannot be relied on to reclaim the target — so the test reports a "leak" that is really a busy heap.
Under parallelism such a test is not flaky, it is wrong: its precondition no longer holds. Declaring that precondition with
[CollectionDefinition(DisableParallelization = true)]is the fix. A retry, a widened timeout, or a[Skip]would have been the band-aid.All four are tagged, not just the one that failed locally —
NodeTypeAssemblyLeakTestred here, whileMeshHubDisposalLeakTestred identically on CI run 31300348689 this morning and passed this run purely on timing. Same defect class; scoping one and waiting for the others to bite would be worse than useless.🚨 The collection is documented as being for reachability assertions only. A test that is merely slow, or that races on shared state, must not be added — that is a real defect, and serialising it would hide it.
Everything else in the assembly still runs at
maxParallelThreads: 4.Why this matters beyond one project
Total measured test work is 36.7 min across 8,817 tests. Monolith is the single largest block and the floor under any sharding change — cutting it 2.57x is what makes reducing shard count viable later (ideal per-shard is 5.4 min at 6 shards today, 8.1 at 4; that arithmetic only improves once the 36.7 min shrinks).
No What's New entry
Test infrastructure, no user-visible effect — the
pullrequestskill's step 0.5 says to skip and say so.