From 7d625770920e8b3c8524b5a39b1a1ba83f33c347 Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Wed, 9 Sep 2026 13:57:38 +0100 Subject: [PATCH] Provision each sandbox environment once, atomically AblySandboxFixture.GetSettings cached into a plain Dictionary from a static method, with an await between the ContainsKey check and the write. xunit.runner.json sets parallelizeTestCollections, and the sandbox is reached from several collections at once - ChannelSubscriptionsTests declares none, so it gets an implicit one of its own - so those writes race. A concurrently written Dictionary can leave its indexer returning null for a key ContainsKey has just accepted, and callers dereference what they are handed: System.NullReferenceException at IO.Ably.Tests.SandboxSpecs.GetRestClient (SandboxSpecs.cs:85) at IO.Ably.Tests.Push.PushAdminSandboxTests.ChannelSubscriptionsTests .ShouldSuccessfullySetAndUpdateChannelSubscription The same window also let two collections each provision an app. A Lazy over a ConcurrentDictionary runs Initialise once per environment however many callers race, and hands all of them the same task. It also passes the environment through, which the old code dropped: Initialise defaults to "sandbox", so asking for anything else provisioned a sandbox app and cached it under the other environment's name. Nothing does that today, so that half is a latent trap rather than a live bug. A failed attempt is evicted rather than cached. The Dictionary this replaces assigned only on success, so a provisioning attempt that threw was retried by whoever asked next; keeping the faulted task would have failed every remaining sandbox test in the run with the same exception, which is worse than the race it fixes. Removal compares key and value together, so a caller that has already raced in a replacement keeps it. The NullReferenceException is intermittent by nature and did not reproduce locally in two full sandbox runs, so this removes the race by construction rather than by a failing test. Those runs did confirm it changes nothing else: 5 failed / 230 passed / 12 skipped both with and without the change, four of the five the same tests, all pre-existing flakes under parallel sandbox load. --- .../Infrastructure/AblySandboxFixture.cs | 43 ++++++++++++++++--- 1 file changed, 37 insertions(+), 6 deletions(-) diff --git a/src/IO.Ably.Tests.Shared/Infrastructure/AblySandboxFixture.cs b/src/IO.Ably.Tests.Shared/Infrastructure/AblySandboxFixture.cs index 29076cbb9..bba429c99 100644 --- a/src/IO.Ably.Tests.Shared/Infrastructure/AblySandboxFixture.cs +++ b/src/IO.Ably.Tests.Shared/Infrastructure/AblySandboxFixture.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Net.Http; using System.Threading.Tasks; @@ -11,18 +12,48 @@ public class AblySandboxFixture { private static readonly DateTimeOffset StartInterval = DateHelper.CreateDate(DateTimeOffset.UtcNow.Year - 1, 2, 3, 15, 5); - private static readonly Dictionary Settings = new Dictionary(); + // One provisioning task per environment, created atomically. xunit.runner.json sets + // parallelizeTestCollections, and the sandbox is reached from several collections at once - + // ChannelSubscriptionsTests declares none at all, so it gets its own - which meant + // concurrent writes to a plain Dictionary. That can leave the indexer returning null for a + // key ContainsKey has just accepted, and the caller dereferences it: SandboxSpecs + // .GetRestClient throws NullReferenceException on the settings it was handed. The await + // between the check and the write also let two collections provision an app each. + // + // Lazy over a ConcurrentDictionary gives one Initialise per environment however many + // callers race, and hands every one of them the same task. + private static readonly ConcurrentDictionary>> Settings = + new ConcurrentDictionary>>(); public static async Task GetSettings(string environment = null) { environment = environment ?? "sandbox"; - if (Settings.ContainsKey(environment)) + + // Passed through, where it used to be dropped: Initialise defaults to "sandbox", so + // asking for any other environment provisioned a sandbox app and cached it under that + // environment's name. + var provisioning = Settings.GetOrAdd( + environment, + env => new Lazy>(() => Initialise(env))); + + try { - return Settings[environment]; + return await provisioning.Value; + } + catch + { + // A failure must not be what gets cached. The Dictionary this replaced assigned only + // on success, so a provisioning attempt that threw was retried by whoever asked + // next; holding the faulted task instead would fail every remaining sandbox test in + // the run with the same exception. + // + // Removed by key and value together, so a caller that has already raced in a + // replacement keeps it - ConcurrentDictionary's ICollection.Remove compares both, + // and the value comparison is reference equality on this exact Lazy. + ((ICollection>>>)Settings) + .Remove(new KeyValuePair>>(environment, provisioning)); + throw; } - - Settings[environment] = await Initialise(); - return Settings[environment]; } private static async Task Initialise(string environment = "sandbox")