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<K, Lazy<T>>.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 <noreply@anthropic.com>
This commit is contained in:
parent
3b5e099241
commit
dc0468cc2b
2 changed files with 50 additions and 7 deletions
|
|
@ -488,7 +488,23 @@ tree is recoverable from git history at `844cf092^`.
|
||||||
|
|
||||||
## #255 — Two RetailDatLoader concurrency tests measured the thread pool, not the loader
|
## #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<K, Lazy<T>>.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
|
guarantee; `AnimationCache_Coalesces…` still fails under full-suite load on
|
||||||
Windows. Reopened independently by two sessions on the same day; both evidence
|
Windows. Reopened independently by two sessions on the same day; both evidence
|
||||||
sets are kept at the end of this issue.
|
sets are kept at the end of this issue.
|
||||||
|
|
|
||||||
|
|
@ -56,6 +56,7 @@ public sealed class RetailDatLoaderTests
|
||||||
private int _activeReads;
|
private int _activeReads;
|
||||||
private int _maxConcurrentReads;
|
private int _maxConcurrentReads;
|
||||||
private int _totalReads;
|
private int _totalReads;
|
||||||
|
private Barrier? _concurrencyGate;
|
||||||
|
|
||||||
public DatDatabase Db => null!;
|
public DatDatabase Db => null!;
|
||||||
public int Iteration => 0;
|
public int Iteration => 0;
|
||||||
|
|
@ -63,6 +64,23 @@ public sealed class RetailDatLoaderTests
|
||||||
public int MaxConcurrentReads => Volatile.Read(ref _maxConcurrentReads);
|
public int MaxConcurrentReads => Volatile.Read(ref _maxConcurrentReads);
|
||||||
public int TotalReads => Volatile.Read(ref _totalReads);
|
public int TotalReads => Volatile.Read(ref _totalReads);
|
||||||
public void Add(uint id, byte[] bytes) => _entries[id] = bytes;
|
public void Add(uint id, byte[] bytes) => _entries[id] = bytes;
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Forces exactly <paramref name="participantCount"/> concurrent
|
||||||
|
/// <see cref="TryGetFileBytes(uint, out byte[])"/> 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 <see cref="ReadDelayMilliseconds"/>
|
||||||
|
/// 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.
|
||||||
|
/// </summary>
|
||||||
|
public void ArmConcurrencyGate(int participantCount) =>
|
||||||
|
_concurrencyGate = new Barrier(participantCount);
|
||||||
|
|
||||||
public IEnumerable<uint> GetAllIdsOfType<T>() where T : IDBObj => _entries.Keys;
|
public IEnumerable<uint> GetAllIdsOfType<T>() where T : IDBObj => _entries.Keys;
|
||||||
public bool TryGet<T>(uint fileId, [MaybeNullWhen(false)] out T value) where T : IDBObj
|
public bool TryGet<T>(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);
|
&& Interlocked.CompareExchange(ref _maxConcurrentReads, active, observed) != observed);
|
||||||
try
|
try
|
||||||
{
|
{
|
||||||
|
_concurrencyGate?.SignalAndWait();
|
||||||
if (ReadDelayMilliseconds > 0)
|
if (ReadDelayMilliseconds > 0)
|
||||||
Thread.Sleep(ReadDelayMilliseconds);
|
Thread.Sleep(ReadDelayMilliseconds);
|
||||||
return _entries.TryGetValue(fileId, out value);
|
return _entries.TryGetValue(fileId, out value);
|
||||||
|
|
@ -293,7 +312,7 @@ public sealed class RetailDatLoaderTests
|
||||||
{
|
{
|
||||||
const uint firstDid = 0x03010024u;
|
const uint firstDid = 0x03010024u;
|
||||||
const uint secondDid = 0x03010025u;
|
const uint secondDid = 0x03010025u;
|
||||||
var portal = new RawDatabase { ReadDelayMilliseconds = 40 };
|
var portal = new RawDatabase();
|
||||||
portal.Add(firstDid, AnimationBytes(firstDid));
|
portal.Add(firstDid, AnimationBytes(firstDid));
|
||||||
portal.Add(secondDid, AnimationBytes(secondDid));
|
portal.Add(secondDid, AnimationBytes(secondDid));
|
||||||
var loader = new RetailAnimationLoader(portal);
|
var loader = new RetailAnimationLoader(portal);
|
||||||
|
|
@ -304,11 +323,15 @@ public sealed class RetailDatLoaderTests
|
||||||
Assert.Same(firstPair[0], firstPair[1]);
|
Assert.Same(firstPair[0], firstPair[1]);
|
||||||
Assert.Equal(1, portal.TotalReads);
|
Assert.Equal(1, portal.TotalReads);
|
||||||
|
|
||||||
await InParallel(
|
// secondDid and 0x03010026u are unrelated keys, so the loader must
|
||||||
() => loader.LoadAnimation(secondDid),
|
// not serialize their reads against each other. Prove that with a
|
||||||
() => loader.LoadAnimation(0x03010026u));
|
// 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);
|
Assert.Equal(3, portal.TotalReads);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -317,13 +340,17 @@ public sealed class RetailDatLoaderTests
|
||||||
{
|
{
|
||||||
const uint firstDid = 0x33010010u;
|
const uint firstDid = 0x33010010u;
|
||||||
const uint secondDid = 0x33010011u;
|
const uint secondDid = 0x33010011u;
|
||||||
var portal = new RawDatabase { ReadDelayMilliseconds = 40 };
|
var portal = new RawDatabase();
|
||||||
byte[] first = ProjectileVfxDatFixtures.OrdinaryPhysicsScript.ToArray();
|
byte[] first = ProjectileVfxDatFixtures.OrdinaryPhysicsScript.ToArray();
|
||||||
byte[] second = ProjectileVfxDatFixtures.OrdinaryPhysicsScript.ToArray();
|
byte[] second = ProjectileVfxDatFixtures.OrdinaryPhysicsScript.ToArray();
|
||||||
System.Buffers.Binary.BinaryPrimitives.WriteUInt32LittleEndian(first, firstDid);
|
System.Buffers.Binary.BinaryPrimitives.WriteUInt32LittleEndian(first, firstDid);
|
||||||
System.Buffers.Binary.BinaryPrimitives.WriteUInt32LittleEndian(second, secondDid);
|
System.Buffers.Binary.BinaryPrimitives.WriteUInt32LittleEndian(second, secondDid);
|
||||||
portal.Add(firstDid, first);
|
portal.Add(firstDid, first);
|
||||||
portal.Add(secondDid, second);
|
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);
|
var loader = new RetailPhysicsScriptLoader(portal);
|
||||||
|
|
||||||
await InParallel(
|
await InParallel(
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue