Skip to content

Guard the counter value caches with a lock - #1871

Merged
SimonCropp merged 1 commit into
mainfrom
fix-counter-thread-safety
Aug 17, 2026
Merged

Guard the counter value caches with a lock#1871
SimonCropp merged 1 commit into
mainfrom
fix-counter-thread-safety

Conversation

@SimonCropp

Copy link
Copy Markdown
Member

Each Counter value cache (Guid, DateTime, DateTimeOffset, Date, Time, numeric id) is a plain Dictionary written through Extensions.GetOrAdd, while the number behind the name came from Interlocked.Increment. The mix reads as thread safe but is not: the interlocked part guards only the counter, not the dictionary that stores the result.

Counter.Current is public and reachable from user code, so parallel work inside a single test hits it concurrently. On main that reliably produces:

System.InvalidOperationException : Operations that change non-concurrent collections
must have exclusive access. A concurrent update was performed on this collection and
corrupted its state.

and sometimes Destination array was not long enough from a resize racing a read.

All the caches now share one lock. ConcurrentDictionary would have been the smaller diff, but its GetOrAdd can run the factory more than once under contention, which would burn counter numbers and hand out a name that is then discarded. A lock keeps the numbering tight, and it also covers the numeric id path, which mutates two dictionaries (the per entity cache and the per entity counter) that no single concurrent collection would make atomic together.

With the caches locked the interlocked increments are redundant, so they become plain increments, and the Build* methods are commented as being called under the lock.

CounterConcurrencyTests drives 500 distinct values through Parallel.ForEach for guids and numeric ids, asserting one name per input and that each input keeps its name. Both fail on main — 3 runs out of 3 while checking.

Verify.Tests passes on net11.0 (1301) and net48 (1222); StaticSettingsTests and ApplyScrubbersTests pass.

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.
@SimonCropp SimonCropp added this to the 32.0.0 milestone Aug 17, 2026
@SimonCropp
SimonCropp merged commit c41ddce into main Aug 17, 2026
7 checks passed
@SimonCropp
SimonCropp deleted the fix-counter-thread-safety branch August 17, 2026 13:20
This was referenced Aug 26, 2026
This was referenced Aug 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant