AddCache is not safe to call concurrently — static registration dictionary races

[poxet]

CacheRegistrationExtensions merges each AddCache call into a process-wide static dictionary without synchronization, so two hosts built concurrently in one process can throw.

private static readonly Dictionary<Type, CacheTypeOptions> _configuredPersistTypes = new();

private static void AppendPreviousRegistrations(CacheOptions o)
{
    var previouslyRegisteredTypes = _configuredPersistTypes.ToArray();   // sizes its array from Count
    foreach (var item in o.GetRegistered())
    {
        _configuredPersistTypes.TryAdd(item.Key, item.Value);            // grows it from another thread
    }
    ...
}

ToArray() on a Dictionary allocates from Count and then copies. If another thread is inside TryAdd on the same static instance, the copy overruns the array it just sized:

System.ArgumentException : Destination array is not long enough to copy all the items in the collection.
   at System.Collections.Generic.Dictionary`2.CopyTo(KeyValuePair`2[] array, Int32 index)
   at System.Linq.Enumerable.ICollectionToArray[TSource](ICollection`1 collection)
   at Tharga.Cache.CacheRegistrationExtensions.AppendPreviousRegistrations(CacheOptions o)
   at Tharga.Cache.CacheRegistrationExtensions.AddCache(IServiceCollection serviceCollection, Action`1 options)

How we hit it

An xUnit integration-test assembly where each test constructs its own WebApplicationFactory<Program>. xUnit runs test classes in parallel, so several hosts build at once, each calling AddCache (for us transitively, via AddQuilt4NetApplicationInsightsClientRemote). It appeared only after the assembly grew past a handful of concurrent boots, which made it read as a flaky test rather than as a race — the failure names an array copy, and nothing in the failing test mentions caching.

Worked around with [assembly: CollectionBehavior(DisableTestParallelization = true)].

Why it seems worth fixing rather than documenting

AddCache is a registration API, and registration is normally safe to call from anywhere. Nothing in the signature suggests process-wide shared state, so a consumer has no reason to serialize it. Any host that builds two service providers concurrently — parallel tests, a multi-tenant host spinning up isolated containers, a WebApplicationFactory used from more than one fixture — can hit it, and it will present as an unrelated intermittent failure.

Possible directions

• A lock around the read-and-merge in AppendPreviousRegistrations, which is the smallest fix.
• ConcurrentDictionary — note this alone is not sufficient: ToArray() on a ConcurrentDictionary is atomic, so it would fix this particular throw, but the read-then-merge would still interleave.
• Scoping the accumulated registrations to the IServiceCollection rather than to the process, which removes the shared state entirely. This looks like the real fix, though it presumably changes behaviour for hosts that deliberately rely on cross-call accumulation.

Observed on Tharga.Cache 1.0.0, .NET 10.

[poxet]

Fixed in #60.

Took the third of your three suggested directions — scoping the accumulated registrations to the IServiceCollection — and your instinct that it was the real fix was right, for a reason beyond the race itself. The dictionary was process-wide state behind a per-container API, so one host's type registrations leaked into an unrelated host's merged options. The test suite was already working around that: AddCacheIdempotencyTests called an internal ResetRegistrations() from its constructor and Dispose purely to stop one test's registrations reaching the next.

It turned out no new plumbing was needed. AddCache already replaces IOptions<CacheOptions> on every call, so the previous call's options are still in the collection as an ImplementationInstance at the point the merge runs:

var previous = serviceCollection
    .LastOrDefault(x => x.ServiceType == typeof(IOptions<CacheOptions>))?
    .ImplementationInstance as IOptions<CacheOptions>;

No lock, no shared state, no cross-host leak. _configuredPersistTypes and ResetRegistrations are deleted, and the idempotency tests now pass without the reset hooks.

On your note that it read as a flaky test rather than a race — that was worth acting on. A new AddCacheConcurrencyTests builds 64 hosts in parallel and asserts both that nothing throws and that no registration crosses between collections. Against the old code all three tests failed, reproducing your exact ArgumentException: Destination array is not long enough at Dictionary.CopyTo ← Enumerable.ToArray, plus an InvalidOperationException about concurrent mutation.

You can delete [assembly: CollectionBehavior(DisableTestParallelization = true)] once you are on the release carrying this.

One deliberate semantic change worth knowing about, since you called out that scoping might alter behaviour for hosts relying on cross-call accumulation: within a single service collection, a third-or-later AddCache call that does not mention a type now inherits the most recent value for it rather than the first-ever one. Two-call behaviour is unchanged.