Fix #4491: route Target._random through Random.Shared#4492
Merged
Conversation
src/Marten.Testing/Documents/Target.cs declared `_random` as a
static `new Random(67)` shared across the test assembly. `Random`
is not thread-safe, and xUnit parallelizes across test classes,
so concurrent calls into `Random.Next(int, int)` could corrupt
internal state and start returning 0 indefinitely. That mapped
every Target.Color to the `0` arm of the switch (Blue) and made
tests that rely on a color distribution fail intermittently with
Shouldly.ShouldAssertException : greens
should not be empty but was
Surfaced on the PGLatest + STJ + NET10 matrix (the fastest of the
four, so widest concurrency overlap) for both
DocumentDbTests.Reading.query_plans.use_as_batch and other tests
generating bulk Target data. Re-running the matrix passed cleanly
— classic narrow concurrency-window flake.
Switch the default source to `System.Random.Shared` (per-thread,
thread-safe, .NET 6+). Preserve the existing `ResetRandomSeed`
contract via a [ThreadStatic] overlay: tests that pin a specific
seed get a per-thread Random seeded to that value; other code
falls back to Random.Shared. The overlay keeps seeding from
leaking across xUnit-parallelized test classes the way the old
shared-static field did.
System.Random.Shared has to be fully qualified because Target
itself has a static `Random()` factory method that shadows the
type name in this scope.
Tradeoff: this removes the (already-fragile) process-wide
deterministic stream the old static seed implied. The previous
comment block on the static field acknowledged that the
"determinism" depended on xUnit test discovery order and was a
known source of CI flakes — i.e. it was deterministic in name
only. Random.Shared trades that for true thread-safety, which
matters more in practice.
Closes #4491.
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #4491.
src/Marten.Testing/Documents/Target.csdeclared_randomas a staticnew Random(67)shared across the test assembly.Randomis not thread-safe; xUnit parallelizes across test classes, so concurrent calls intoRandom.Next(int, int)could corrupt the internal state and start returning 0 indefinitely. That mapped everyTarget.Colorto the0arm of the switch (Blue) and made tests that rely on a color distribution fail intermittently:Surfaced on the PGLatest + STJ + NET10 CI matrix (the fastest of the four, so widest concurrency overlap) on PRs #4488 and #4490 for
DocumentDbTests.Reading.query_plans.use_as_batch. Re-running the matrix passed cleanly both times — classic narrow concurrency-window flake.Fix
Switch the default source to
System.Random.Shared(per-thread, thread-safe, .NET 6+). Preserve the existingResetRandomSeed(int)contract via a[ThreadStatic]overlay: tests that pin a specific seed get a per-threadRandomseeded to that value; other code falls back toRandom.Shared. The overlay also keeps seeding from leaking across xUnit-parallelized test classes the way the old shared-static field did.System.Random.Sharedis fully qualified at the call site becauseTargetitself has a staticRandom()factory method that shadows the type name.Tradeoff
This removes the (already-fragile) process-wide deterministic stream the old static seed implied. The previous comment block on the static field acknowledged that the "determinism" depended on xUnit test discovery order and was a known source of CI flakes — i.e. it was deterministic in name only.
Random.Sharedtrades that for true thread-safety, which matters more in practice.ResetRandomSeedhas no callers in the current codebase, but the method is preserved for source compatibility. Tests that want a deterministic stream still get one on their own thread.Test plan
dotnet build src/Marten.slnx -c Release— 0 errorsDocumentDbTests.Reading.query_plans.*— 2/2 pass on both Newtonsoft and STJDocumentDbTests.Reading.*— 126/126 pass on net10.0ResetRandomSeedcallers exist in the codebase — the overlay path is exercised only by future seeded-determinism callers🤖 Generated with Claude Code