Guard the counter value caches with a lock (#1871)

Each Counter cache is a plain Dictionary, written through GetOrAdd, while the
number backing the name came from Interlocked.Increment. That mix only reads as
thread safe: user code can reach Counter.Current from parallel work inside one
test, and concurrent calls corrupted the dictionary.

All the caches now share one lock, which makes the Interlocked increments
redundant, so they become plain increments under that lock.

Author: SimonCropp

src/Verify.Tests/CounterConcurrencyTests.cs

@@ -0,0 +1,50 @@
+// User code can reach Counter.Current from parallel work inside one test, so the
+// value caches have to survive concurrent access
+public class CounterConcurrencyTests
+{
+    [Fact]
+    public void ConcurrentGuids()
+    {
+        using var counter = Counter.Start();
+
+        var guids = Enumerable.Range(0, 500)
+            .Select(_ => Guid.NewGuid())
+            .ToList();
+
+        var names = new ConcurrentBag<string>();
+        Parallel.ForEach(guids, guid => names.Add(counter.NextString(guid)));
+
+        // one name per distinct input, and no name handed out twice
+        Assert.Equal(guids.Count, names.Distinct().Count());
+
+        // and the same input keeps the name it was given
+        foreach (var guid in guids)
+        {
+            Assert.Contains(counter.NextString(guid), names);
+        }
+
+        Assert.Equal(guids.Count, names.Distinct().Count());
+    }
+
+    [Fact]
+    public void ConcurrentNumericIds()
+    {
+        using var counter = Counter.Start();
+
+        var ids = Enumerable.Range(0, 500)
+            .Select(_ => (long) _)
+            .ToList();
+
+        var names = new ConcurrentBag<string>();
+        Parallel.ForEach(ids, id => names.Add(counter.NextNumericIdString("TheEntity", id)));
+
+        Assert.Equal(ids.Count, names.Distinct().Count());
+
+        foreach (var id in ids)
+        {
+            Assert.Contains(counter.NextNumericIdString("TheEntity", id), names);
+        }
+
+        Assert.Equal(ids.Count, names.Distinct().Count());
+    }
+}

src/Verify/Counter.cs

@@ -10,6 +10,10 @@ public partial class Counter :
     Dictionary<DateTime, string> namedDateTimes;
     Dictionary<Guid, string> namedGuids;
     Dictionary<DateTimeOffset, string> namedDateTimeOffsets;
+    // Guards every value cache below. User code can reach Counter.Current from
+    // parallel work inside one test, and a Dictionary does not survive that.
+    internal object cacheLock = new();
+
     public bool DateCounting { get; }
     public bool ScrubDateTimes { get; }
     public bool ScrubGuids { get; }

src/Verify/Counter_Date.cs

@@ -29,14 +29,18 @@ public string NextString(Date input) =>
             return new(0, name);
         }
 
-        return dateCache.GetOrAdd(
-            input,
-            _ => BuildDateValue());
+        lock (cacheLock)
+        {
+            return dateCache.GetOrAdd(
+                input,
+                _ => BuildDateValue());
+        }
     }
 
+    // Called under cacheLock
     (int intValue, string stringValue) BuildDateValue()
     {
-        var value = Interlocked.Increment(ref currentDate);
+        var value = ++currentDate;
 
         if (DateCounting)
         {
@@ -54,9 +58,10 @@ public partial class Counter
     Dictionary<DateTime, (int intValue, string stringValue)> dateCache = new(dateTimeComparer);
     int currentDate;
 
+    // Called under cacheLock
     (int intValue, string stringValue) BuildDateValue()
     {
-        var value = Interlocked.Increment(ref currentDate);
+        var value = ++currentDate;
 
         if (DateCounting)
         {
@@ -78,10 +83,13 @@ internal string ConvertDate(DateTime date)
             return "Date_MinValue";
         }
 
-        return dateCache.GetOrAdd(
-            date,
-            _ => BuildDateValue())
-            .stringValue;
+        lock (cacheLock)
+        {
+            return dateCache.GetOrAdd(
+                    date,
+                    _ => BuildDateValue())
+                .stringValue;
+        }
     }
 }
 #endif

src/Verify/Counter_DateTime.cs

@@ -48,14 +48,18 @@ public string NextString(DateTime input) =>
             return new(0, name);
         }
 
-        return dateTimeCache.GetOrAdd(
-            input,
-            _ => BuildDateTimeValue());
+        lock (cacheLock)
+        {
+            return dateTimeCache.GetOrAdd(
+                input,
+                _ => BuildDateTimeValue());
+        }
     }
 
+    // Called under cacheLock
     (int intValue, string stringValue) BuildDateTimeValue()
     {
-        var value = Interlocked.Increment(ref currentDateTime);
+        var value = ++currentDateTime;
 
         if (DateCounting)
         {

src/Verify/Counter_DateTimeOffset.cs

@@ -48,14 +48,18 @@ public string NextString(DateTimeOffset input) =>
             return new(0, name);
         }
 
-        return dateTimeOffsetCache.GetOrAdd(
-            input,
-            _ => BuildDateTimeOffsetValue());
+        lock (cacheLock)
+        {
+            return dateTimeOffsetCache.GetOrAdd(
+                input,
+                _ => BuildDateTimeOffsetValue());
+        }
     }
 
+    // Called under cacheLock
     (int intValue, string stringValue) BuildDateTimeOffsetValue()
     {
-        var value = Interlocked.Increment(ref currentDateTimeOffset);
+        var value = ++currentDateTimeOffset;
 
         if (DateCounting)
         {

src/Verify/Counter_Guid.cs

@@ -28,14 +28,18 @@ public string NextString(Guid input) =>
             return new(0, name);
         }
 
-        return guidCache.GetOrAdd(
-            input,
-            _ => BuildGuidValue());
+        lock (cacheLock)
+        {
+            return guidCache.GetOrAdd(
+                input,
+                _ => BuildGuidValue());
+        }
     }
 
+    // Called under cacheLock
     (int intValue, string stringValue) BuildGuidValue()
     {
-        var value = Interlocked.Increment(ref currentGuid);
+        var value = ++currentGuid;
         return (value, $"Guid_{value}");
     }
 }

src/Verify/Counter_NumericId.cs

@@ -33,17 +33,21 @@ static string ToKey(object value)
 
     (int intValue, string stringValue) NextNumericIdValue(string entityName, string input)
     {
-        if (!numericIdCache.TryGetValue(entityName, out var cache))
+        lock (cacheLock)
         {
-            cache = [];
-            numericIdCache[entityName] = cache;
+            if (!numericIdCache.TryGetValue(entityName, out var cache))
+            {
+                cache = [];
+                numericIdCache[entityName] = cache;
+            }
+
+            return cache.GetOrAdd(
+                input,
+                _ => BuildNumericIdValue(entityName));
         }
-
-        return cache.GetOrAdd(
-            input,
-            _ => BuildNumericIdValue(entityName));
     }
 
+    // Called under cacheLock
     (int intValue, string stringValue) BuildNumericIdValue(string entityName)
     {
         numericIdCounters.TryGetValue(entityName, out var current);

src/Verify/Counter_Time.cs

@@ -43,14 +43,18 @@ public string NextString(Time input) =>
             return new(0, name);
         }
 
-        return timeCache.GetOrAdd(
-            input,
-            _ => BuildTimeValue());
+        lock (cacheLock)
+        {
+            return timeCache.GetOrAdd(
+                input,
+                _ => BuildTimeValue());
+        }
     }
 
+    // Called under cacheLock
     (int intValue, string stringValue) BuildTimeValue()
     {
-        var value = Interlocked.Increment(ref currentTime);
+        var value = ++currentTime;
         return (value, $"Time_{value}");
     }
 }

src/todo.md

@@ -82,5 +82,5 @@ All six resolved 2026-08-16 (five fixed here; the inline item resolved as by-des
 - [x] **`PrefixUnique` set is case-sensitive on case-insensitive filesystems.**
   `Verify/Naming/PrefixUnique.cs:3` — methods `Foo` and `foo` map to the same files on NTFS/APFS but pass the uniqueness check and silently clobber each other.
 
-- [ ] **`Counter` caches mix `Interlocked` counters with unsynchronized `Dictionary` writes.**
+- [x] **`Counter` caches mix `Interlocked` counters with unsynchronized `Dictionary` writes.**
   `Verify/Counter_*.cs` → `Extensions.cs:155-164` — concurrent `Counter.Current.Next(...)` calls from parallel user code inside one test can corrupt the plain `Dictionary`.