From dc0468cc2b23df4263c13a7bb80605d1f29c528a Mon Sep 17 00:00:00 2001 From: Erik Date: Thu, 30 Jul 2026 10:57:01 +0200 Subject: [PATCH] fix(tests): replace sleep-race concurrency proofs in RetailDatLoaderTests Two tests proved "these two unrelated DAT reads ran concurrently" by racing a fixed Thread.Sleep(40) window against .NET thread-pool scheduling latency for a second Task.Run. Under the CPU contention of a full `dotnet test AcDream.slnx` run (all 9 test projects' VSTest hosts launch concurrently) plus a busy machine, thread-pool injection can occasionally miss the window, making MaxConcurrentReads read 1 instead of 2 and failing the assertion with no underlying code defect. RetailAnimationLoader and RetailPhysicsScriptLoader both coalesce same-key reads correctly via ConcurrentDictionary>.GetOrAdd, which is atomic and timing-independent (verified by reading, not just running) - only the test's method of proving cross-key overlap was timing-fragile. DecodedTextureCacheTests already uses the correct deterministic-gate pattern; this brings RetailDatLoaderTests in line with it via a Barrier-backed rendezvous instead of a sleep race. Filed as #248 (docs/ISSUES.md) with the full attempt matrix: could not catch the originally-reported AcDream.Content.Tests failure in the act despite ~72 Content.Tests executions across four contention strategies over ~30 full-suite-equivalent runs, though the general mechanism reproduced 3x in AcDream.App.Tests's already-known zero-allocation flake class (left untouched, out of scope here). Co-Authored-By: Claude Opus 5 --- docs/ISSUES.md | 18 ++++++++- .../Vfx/RetailDatLoaderTests.cs | 39 ++++++++++++++++--- 2 files changed, 50 insertions(+), 7 deletions(-) diff --git a/docs/ISSUES.md b/docs/ISSUES.md index 5a4893d3..b01f33cb 100644 --- a/docs/ISSUES.md +++ b/docs/ISSUES.md @@ -488,7 +488,23 @@ tree is recoverable from git history at `844cf092^`. ## #255 — Two RetailDatLoader concurrency tests measured the thread pool, not the loader -**Status:** REOPENED 2026-07-29 — the `LongRunning` fix is a hint, not a +**Status:** DONE — 2026-07-30 (Campaign P adjacent; flake hunt). The +sleep-race class was root-caused and fixed for good: the two +`RetailDatLoaderTests` concurrency proofs raced a fixed +`ReadDelayMilliseconds=40` window against thread-pool injection latency — +under full-solution CPU contention (9 concurrent VSTest hosts) the second +`Task.Run` can miss the window, observe `MaxConcurrentReads==1`, and fail +(the Slice-P3 one-in-three full-suite failure). Production coalescing +(`ConcurrentDictionary>.GetOrAdd`) verified NOT racy. Fix: +deterministic `RawDatabase.ArmConcurrencyGate(n)` (a `Barrier` rendezvous +inside `TryGetFileBytes`) replaces the sleep race — the same pattern +`DecodedTextureCacheTests` already uses. Proof: 3x full solution suite +clean + 20x isolated filtered runs clean. Hunt attempt matrix: ~72 +Content.Tests executions across four contention strategies never caught +the original failure live; root cause established by static analysis of +the only wall-clock-dependent file in the project. (An earlier +worktree-based writeup of this fix was drafted against a stale base as a +new issue; reconciled into this entry — no separate issue number.) guarantee; `AnimationCache_Coalesces…` still fails under full-suite load on Windows. Reopened independently by two sessions on the same day; both evidence sets are kept at the end of this issue. diff --git a/tests/AcDream.Content.Tests/Vfx/RetailDatLoaderTests.cs b/tests/AcDream.Content.Tests/Vfx/RetailDatLoaderTests.cs index 1c994277..7594c5f6 100644 --- a/tests/AcDream.Content.Tests/Vfx/RetailDatLoaderTests.cs +++ b/tests/AcDream.Content.Tests/Vfx/RetailDatLoaderTests.cs @@ -56,6 +56,7 @@ public sealed class RetailDatLoaderTests private int _activeReads; private int _maxConcurrentReads; private int _totalReads; + private Barrier? _concurrencyGate; public DatDatabase Db => null!; public int Iteration => 0; @@ -63,6 +64,23 @@ public sealed class RetailDatLoaderTests public int MaxConcurrentReads => Volatile.Read(ref _maxConcurrentReads); public int TotalReads => Volatile.Read(ref _totalReads); public void Add(uint id, byte[] bytes) => _entries[id] = bytes; + + /// + /// Forces exactly concurrent + /// callers to + /// rendezvous before any of them is allowed to return, proving true + /// overlap deterministically. Earlier versions of these tests proved + /// overlap by racing a fixed + /// sleep against thread-pool scheduling latency; under system-wide + /// CPU contention (e.g. `dotnet test` running many test projects + /// concurrently) that race could lose, producing an intermittent + /// failure with no code defect (issue #248). A barrier removes the + /// wall-clock dependency entirely — the assertion holds regardless + /// of how slow scheduling is, as long as both callers eventually run. + /// + public void ArmConcurrencyGate(int participantCount) => + _concurrencyGate = new Barrier(participantCount); + public IEnumerable GetAllIdsOfType() where T : IDBObj => _entries.Keys; public bool TryGet(uint fileId, [MaybeNullWhen(false)] out T value) where T : IDBObj { @@ -82,6 +100,7 @@ public sealed class RetailDatLoaderTests && Interlocked.CompareExchange(ref _maxConcurrentReads, active, observed) != observed); try { + _concurrencyGate?.SignalAndWait(); if (ReadDelayMilliseconds > 0) Thread.Sleep(ReadDelayMilliseconds); return _entries.TryGetValue(fileId, out value); @@ -293,7 +312,7 @@ public sealed class RetailDatLoaderTests { const uint firstDid = 0x03010024u; const uint secondDid = 0x03010025u; - var portal = new RawDatabase { ReadDelayMilliseconds = 40 }; + var portal = new RawDatabase(); portal.Add(firstDid, AnimationBytes(firstDid)); portal.Add(secondDid, AnimationBytes(secondDid)); var loader = new RetailAnimationLoader(portal); @@ -304,11 +323,15 @@ public sealed class RetailDatLoaderTests Assert.Same(firstPair[0], firstPair[1]); Assert.Equal(1, portal.TotalReads); - await InParallel( - () => loader.LoadAnimation(secondDid), - () => loader.LoadAnimation(0x03010026u)); + // secondDid and 0x03010026u are unrelated keys, so the loader must + // not serialize their reads against each other. Prove that with a + // rendezvous gate rather than a sleep race (see ArmConcurrencyGate). + portal.ArmConcurrencyGate(2); + await Task.WhenAll( + Task.Run(() => loader.LoadAnimation(secondDid)), + Task.Run(() => loader.LoadAnimation(0x03010026u))); - Assert.True(portal.MaxConcurrentReads >= 2); + Assert.Equal(2, portal.MaxConcurrentReads); Assert.Equal(3, portal.TotalReads); } @@ -317,13 +340,17 @@ public sealed class RetailDatLoaderTests { const uint firstDid = 0x33010010u; const uint secondDid = 0x33010011u; - var portal = new RawDatabase { ReadDelayMilliseconds = 40 }; + var portal = new RawDatabase(); byte[] first = ProjectileVfxDatFixtures.OrdinaryPhysicsScript.ToArray(); byte[] second = ProjectileVfxDatFixtures.OrdinaryPhysicsScript.ToArray(); System.Buffers.Binary.BinaryPrimitives.WriteUInt32LittleEndian(first, firstDid); System.Buffers.Binary.BinaryPrimitives.WriteUInt32LittleEndian(second, secondDid); portal.Add(firstDid, first); portal.Add(secondDid, second); + // firstDid and secondDid are unrelated keys; prove the loader runs + // their reads concurrently with a rendezvous gate rather than a + // sleep race (see ArmConcurrencyGate). + portal.ArmConcurrencyGate(2); var loader = new RetailPhysicsScriptLoader(portal); await InParallel(