Skip to content

fix: AddCache concurrency race (#55) and FluentAssertions dependency leak (#56) - #60

Open
poxet wants to merge 8 commits into
masterfrom
fix/issue-55-56-registration-race-and-fluentassertions
Open

fix: AddCache concurrency race (#55) and FluentAssertions dependency leak (#56)#60
poxet wants to merge 8 commits into
masterfrom
fix/issue-55-56-registration-race-and-fluentassertions

Conversation

@poxet

@poxet poxet commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Fixes #55
Fixes #56

Two consumer-reported defects in the published packages, plus the dependency work the feature workflow requires up front.

#55AddCache registration race

CacheRegistrationExtensions merged every AddCache call into a process-wide static Dictionary without synchronization. ToArray() sizes its array from Count and then copies, so a concurrent TryAdd from another host build overran it.

The reporter suggested three directions; this takes the third, and for a stronger reason than the race alone. The dictionary was process-wide state behind a per-container API — one host's type registrations leaked into an unrelated host's merged options. The tests already worked around that, calling an internal ResetRegistrations() from their constructor and Dispose purely to stop cross-test bleed.

AddCache already replaces IOptions<CacheOptions> on every call, so the previous call's options are sitting in the collection as an ImplementationInstance before the removal:

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 gone, and AddCacheIdempotencyTests no longer needs IDisposable or the reset hooks.

New AddCacheConcurrencyTests — 3 tests, 64 concurrent hosts. Against the old code all three failed with the reporter's exact ArgumentException: Destination array is not long enough at Dictionary.CopyToEnumerable.ToArray, plus a concurrent-mutation InvalidOperationException and the cross-collection leak.

One deliberate semantic change: in a three-or-more-call sequence, a 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.

Consumers can delete [assembly: CollectionBehavior(DisableTestParallelization = true)] if they added it for this.

#56 — FluentAssertions shipped as a public dependency

Tharga.Cache.csproj declared FluentAssertions 8.10.0 with no PrivateAssets, propagating the Xceed Community License to every consumer and forcing NU1605 on anyone pinning an older version.

The reference was unused — no using FluentAssertions, no .Should() anywhere in the library's sources — so it is removed outright rather than marked private. Verified by packing and reading the nuspec out of the .nupkg: the net10.0 group now lists only Microsoft.Extensions.Hosting.Abstractions.

It was a single stray line, not inherited from a shared Directory.Build.props, but it reached all six published packages because the siblings ProjectReference the core.

Microsoft.Testing.Platform migration

Not optional — xunit.v3 4.0.0 drops the VSTest bridge on the .NET 10 SDK, so dotnet test fails outright until the projects opt in.

  • global.json selects the MTP runner.
  • <OutputType>Exe</OutputType> on all four test projects; xunit v3 4.x refuses to build a library test project.
  • Dropped Microsoft.NET.Test.Sdk, xunit.runner.visualstudio, coverlet.collector, coverlet.msbuild — all VSTest-only here. Added Microsoft.Testing.Extensions.CodeCoverage.
  • CI test step rewritten: --filter "(Category!=Integration)&(Category!=TimeCritical)"--filter-not-trait pairs; --collect:"XPlat Code Coverage"--coverage --coverage-output-format cobertura. Four cobertura files still land in ./coverage for Codecov.
  • coverage/ added to .gitignore.

Also bumped Tharga.Blazor 2.3.0 → 2.3.1.

A test that asserted the opposite of its name

AddCache_CalledTwice_WithSameType_FirstRegistrationWins checked only that the cache resolved non-null. The merged options have always let the later registration win, so the name was wrong and nothing caught it. Renamed to ..._LatestRegistrationWins with a real assertion on DefaultFreshSpan.

Verification

CI-gate command green on every run: 467 tests, 465 passed, 2 skipped.

The full unfiltered suite shows failures only in FetchDataThrottleTests — all TimeCritical-traited and excluded from CI. That class passes in isolation; this is the pre-existing flakiness already tracked in the backlog, and the new 64-host concurrency tests make it easier to hit locally by loading the CPU. Flagged rather than papered over.

Version

CI computes 1.0.x from MAJOR_MINOR: '1.0', so this lands as a patch. Worth considering 1.1.0 instead: removing the transitive FluentAssertions is a compile-time break for anyone who was unknowingly relying on it, and the registration-accumulation semantics changed. Say the word and I'll bump MAJOR_MINOR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant