Fix race condition in static validator cache

[kamilprzyb2]

Fix race condition in static ViesManager.GetValidator cache

Problem

ViesManager.IsValid(countryCode, vatNumber) can throw ArgumentException: An item with the same key has already been added. Key: <code> when called concurrently before the static VatValidators cache is warm.

Minimal repro:

Parallel.For(0, 16, _ => ViesManager.IsValid("FR", "40303265045"));

On a cold process, at least one iteration throws intermittently. I hit this in an ASP.NET service that validates VAT in request handlers — parallel integration tests surface it routinely, and any real workload that calls IsValid from multiple threads before the cache is populated is exposed.

Stack trace from a real failure:

System.ArgumentException : An item with the same key has already been added. Key: FR
   at System.Collections.Generic.Dictionary`2.TryInsert(...)
   at System.Collections.Generic.Dictionary`2.Add(TKey, TValue)
   at Padi.Vies.ViesManager.GetValidator(String countryCode) in src/vies-dotnet-api/ViesManager.cs:line 88
   at Padi.Vies.ViesManager.IsValid(String countryCode, String vatNumber) in src/vies-dotnet-api/ViesManager.cs:line 145

Cause

VatValidators is a Dictionary<string, IVatValidator>, which is documented as not safe for concurrent reads with writes. GetValidator does the classic check-then-act:

if (VatValidators.TryGetValue(countryCode, out var v)) return v;
v = countryCode.AsSpan() switch { /* construct per country */ };
if (v == null) return null;
VatValidators.Add(countryCode, v);   // races with another thread doing the same

Two threads can both miss TryGetValue, both build their validator, and both call Add — the loser throws. Concurrent Adds with different keys can also corrupt the bucket array; the duplicate-key throw is just the louder failure mode.

Fix

Swap Dictionary for ConcurrentDictionary and use GetOrAdd(key, value). Behaviour is identical for serial callers; concurrent first-access is now safe. The unknown-country-code branch still returns null without polluting the cache.

- private static readonly Dictionary<string, IVatValidator> VatValidators =
-     new(StringComparer.OrdinalIgnoreCase);
+ private static readonly ConcurrentDictionary<string, IVatValidator> VatValidators =
+     new(StringComparer.OrdinalIgnoreCase);

- VatValidators.Add(countryCode, validator);
+ VatValidators.GetOrAdd(countryCode, validator);

Plus using System.Collections.Concurrent; at the top.

Why not a lock?

A lock would serialise every cache miss. ConcurrentDictionary is lock-free on the hit path and uses fine-grained striping for inserts — a better fit for what's effectively read-mostly state.

Why not eager initialisation?

Populating all 28 validators in a static constructor would also fix the race, but pays the construction cost unconditionally at first use of the class. The lazy + concurrent approach preserves the current "only pay for what you use" behaviour.

Caveat: redundant constructions under contention

Under heavy first-access contention for the same country, two threads can both build a validator; the loser's instance is GC'd. Acceptable here because validator constructors are cheap and side-effect-free.

Tests

Added ViesManagerConcurrencyTests that parallelises IsValid over all 28 supported country codes from multiple threads. Fails reliably on master, passes on this branch.

Caveat: because VatValidators is process-static, once any other test in the same xUnit process touches IsValid, the cache is warm and the race window is gone for that run. The new test is best-effort rather than deterministic; a fully deterministic regression check would need either reflection-based dict-clearing or a refactor to make the cache injectable. Happy to follow up with whichever you'd prefer if that's a blocker.