fix(justdummies): serialize draws on a seeded random source - #311
Conversation
Drawing from one seeded source on several threads at once collapses it: the unsynchronized Random's two internal indices converge under contention and it returns zero for ever, so every generator settles on the minimum of its range (0, "", Guid.Empty, int.MinValue) and stays there for the rest of the scope. Nothing throws. Five examples pin the regression at the coordinates where it was found — the ambient scope, a bounded generator collapsing onto its lower bound, the sibling generators the dead source poisons, the sequential draws taken afterwards, and a shared AnyContext. One property adds the seed as the input space, since nothing about the collapse depended on which seed was pinned. Refs: #310
SeededRandom stops handing out its System.Random and becomes the only door to it: the four primitives actually drawn (Next, Next(min,max), NextBytes, NextDouble) now go through it, each serialized on the source's own lock. Keeping the instance private is what makes the guarantee hold — a synchronized subclass would leak any member left un-overridden, whereas here a draw that bypasses the lock does not compile. An uncontended lock does not change the order a single thread consumes the stream, so a pinned seed replays bit-identically: verified against a captured fingerprint of 39 generator shapes over 7 seeds, unchanged before and after. What this does not buy is a value-level guarantee across threads: the lock is per primitive draw, so two threads interleave inside a multi-draw generation. That narrower contract is what the documentation states. Refs: #310
The two sources said different things and neither was true. AnyContext claimed to be "not thread-safe" with nothing enforcing it; the ambient source said nothing, while the user guide's only statement on the subject covered the cross-test axis and read as general safety. One contract now holds for both: safe to draw from concurrently, no value-level guarantee across threads, and a per-work-item Any.UseSeed scope for a parallel run that must replay — a recipe the public surface already allowed and nothing documented. Refs: #310
ADR-0006 named this hazard verbatim and chose AsyncLocal context-locality as the remedy — which answers which source is in effect, never how it is touched. It is superseded by ADR-0026, which does not revisit the question, so the concurrency contract is recorded as a new decision rather than as an edit to a superseded one. Drafted as Proposed for @Reefact to accept. Refs: #310
The NuGet Description promised "any run is reproducible from a reported seed" without qualification, which the package's own README now contradicts three files away: concurrent draws interleave, so a seed replays a run only while its draws are taken one at a time. Scope the headline promise to "any sequential run" so the shipped artifact stops claiming a reproducibility it cannot deliver under parallelism — the honest boundary the concurrency contract established. Refs: #310
Adversarial review of the concurrency docs found the parallel-reproducibility recipe misleading in a section whose subject is reproducibility: - it wrapped the loop in a seedless Any.Reproducibly(() => ...), whose reported seed replays nothing because every draw happens inside a per-item UseSeed scope that overrides the ambient source — dropped it, and show runSeed as a recorded constant instead, so the reader keeps the number that actually replays; - it derived the sub-seed with System.HashCode.Combine, which does not exist on netstandard2.0 / net472 (the floor ADR-0022 sets and ADR-0042 cites) — a consumer copying it verbatim would not compile; replaced with a floor-safe derivation, verified reproducible and collision-free over the illustrated range. Also fixes an article-agreement slip in the package README. English and French user guides and the Any.UseSeed XML example move in step. Refs: #310
|
Please perform a thorough and skeptical code review of the fix that was implemented for the following issue. Implemented fixYour task is to verify that the fix is genuinely correct, complete, and safe. Do not assume that the implementation is correct merely because the original issue appears to have been addressed. Inspect the actual code, the full diff, the surrounding implementation, and the relevant tests. Review objectives1. Understand the original defect
2. Analyze the implemented fix
3. Reproduce and validate the behaviorWhen technically possible:
Do not rely only on the fact that the existing tests pass. 4. Review the testsVerify that the tests added or modified for this fix:
Identify any missing tests and provide concrete test cases when appropriate. 5. Search for regressionsExamine whether the fix could introduce regressions involving:
Only retain the categories that are actually relevant to this fix, but actively investigate the surrounding code for less obvious consequences. 6. Check edge casesActively search for inputs and execution paths that the implementation author may have overlooked. Pay particular attention to:
Do not limit the review to this list. 7. Assess design qualityEvaluate whether the fix:
If you believe a better implementation exists, explain it concretely and compare its trade-offs with the current fix. 8. Inspect the complete change scopeReview every file changed as part of this fix. Detect:
Required outputStart with a concise explanation of:
Then list all findings, ordered by severity:
For every finding, include:
Finish with the following verdict: Final verdict
Important constraints
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8075501fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review flagged the unconditional "safe to call concurrently" remark on the public IAny<T>.Generate contract as an over-promise: the interface is user-implementable, and As/Combine/OrNull/ContainingAny run caller-supplied factories, composers, element generators and comparers without synchronization. A stateful user factory drawn concurrently races in the caller's code even though the random source stays healthy. Narrow the guarantee to what the library actually delivers — its own serialized draw, which cannot corrupt the source or breach the declared constraints — and state that supplied delegates and foreign IAny<T> implementations must be concurrency-safe themselves. Refs: #310
The #310 regression suite pinned only the scalar draw paths, yet the fix (a lock at the SeededRandom choke point) protects every path. Extend it to the paths with the most moving parts, each routing its draws through that choke point: As and Combine (derived generators wrapping a draw), ListOf (the collection fill loop), SetOf (the bounded distinct dedup-draw), and StringMatching (the regex context). Each was confirmed red before the fix by stripping the lock from a copy and watching it collapse — 5/5 lock-stripped runs. Deliberately omitted, with the reasoning recorded in the test file: OrNull's null/value coin and a bare Any.Double(). Corruption is provoked only by NextBytes-heavy draws; a lone Next(2)/NextDouble() never builds enough contention to corrupt within a bounded run, so a lock-stripped mutant leaves them green (measured: 5/5 missed). Shipping a test that cannot fail on the broken code would only manufacture false confidence. Refs: #310
Summary
Drawing from one seeded random source on several threads at once corrupted it permanently, so every generator silently collapsed to the minimum of its declared range —
0,"",Guid.Empty,int.MinValue— for the rest of the scope. Draws are now serialized on the source's own lock, and the reproducibility promise is stated at the width it actually holds.Type of change
Changes
SeededRandomno longer hands out itsSystem.Random. It becomes the only door to it, exposing the four primitives actually drawn —Next(int),Next(int, int),NextBytes(byte[]),NextDouble()— each serialized on the instance's own gate. Keeping the instance private is what makes the guarantee hold: a synchronized subclass would leak any member left un-overridden, whereas here a draw that bypasses the lock does not compile (the same "make the rule un-break-able rather than merely checked" reasoning as ADR-0031 and ADR-0035).Random—GenerateOrdinal,CountSpec.Resolve,StringSpec.BuildCandidate,RegexGenerationContext, theUriSpechelpers, theRandomSamplingextension methods — now carry aSeededRandom. Every hunk is a type substitution; no generation logic changed.ConcurrentDrawTestspins the regression across every draw path, not just the scalar ones. Five examples cover the scalar paths at the coordinates where the defect was found (the ambient scope, a bounded generator collapsing onto its lower bound, the sibling generators a dead source poisons, the sequential draws taken afterwards, a sharedAnyContext); five more cover the composed paths that route through the same lock (As,Combine,ListOf,SetOf,StringMatching). One property inSeedDeterminismPropertiesadds the seed as the input space. Every one was confirmed red before the fix by stripping the lock from a copy; two further candidates whose light single-sample draw could not be made to fail on the broken code (anOrNullcoin, a bareDouble) were dropped rather than shipped as false-confidence tests, with the reasoning recorded in the test file.IAny<T>.Generate— scoped, after review, to the library's own serialized draw, with supplied factories, composers, comparers and foreignIAny<T>implementations left as the caller's responsibility — plusAny.WithSeedandAny.UseSeed;AnyContext's unenforced "not thread-safe" remark replaced; the user guide (EN + FR) and the package README given the intra-test axis and a verified per-work-item recipe (floor-safe on netstandard2.0 — noSystem.HashCode).Proposed.What the fix does and does not promise
Serializing removes the corruption; it does not make a parallel run replayable. The lock is taken per primitive draw and one generated value may consume many (a string draws once per character), so two threads interleave inside a single generation and neither the sequence nor the multiset of values is stable under parallelism. A promise of parallel reproducibility would be false, so the documentation states the narrower one — the same standard the library already applies when it withholds a full-replay claim for a foreign generator.
The wider guarantee is already reachable with the existing public surface, and is now documented rather than built: a
Any.UseSeedscope opened inside a parallel loop body is private to its worker, so deriving one seed per unit of work from the run's seed makes the whole run replay. Verified — identical results across runs for the same master seed, 64/64 distinct values, no degenerate value.Behavioural non-regression
An uncontended lock does not change the order in which a single thread consumes the stream, so every pinned seed replays exactly as before. Verified against a captured fingerprint of 39 generator shapes over 7 seeds plus the ambient path — byte-identical before and after (
md5 f07467d087cd88fe706fcd436f145280).Testing
dotnet build FirstClassErrors.slndotnet test FirstClassErrors.sln— full solution green, 0 failures, includingFirstClassErrors.Testing.UnitTests, which draws from JustDummies per ADR-0026.FirstClassErrors.Analyzers.UnitTests) — as part of the solution-wide run above.Additionally:
As,Combine,ListOf,SetOf,StringMatching) tripped 5/5. Two candidates that missed 5/5 — anOrNullcoin flip and a bareDouble, whose lone light sample never builds enough contention to corrupt the source — were dropped rather than shipped green-on-broken-code.dotnet build JustDummies/JustDummies.csprojunder the CI warning ratchet (GITHUB_ACTIONS=true,--no-incremental): 0 warnings.git diff -- JustDummies/PublicAPI/is empty).The net472 support floor was not exercised locally: the
framework-floorjob needs Windows, and this container has neither .NET Framework nor Mono. It runs in CI.Review
An adversarial pre-PR review on three dimensions (completeness/soundness, behavioural equivalence, tests/docs) returned two SHIP IT and one SHIP WITH NITS (documentation only, folded in). Codex then reviewed the branch and raised two threads: the
IAny<T>.Generateremark over-promising for user-supplied code — agreed and fixed by the narrowing above; and removing the French ADR — declined, asadr/README.md(l.45-48) mandates the bilingual twin and all 41 existing ADRs ship one, arbitrated by @Reefact ("ADR in EN + FR"). The composed-path test coverage was then added in response to the question of how to be confident no draw path was left unguarded.Documentation
doc/updatedArbitraryTestValues.fr.mdand the ADR's FR twinJustDummies/CHANGELOG.mdis deliberately untouched: its Unreleased section is drafted automatically from merged pull requests by.github/workflows/changelog.yml.Architecture decisions
Proposed: ADR-0042ADR-0006 named this hazard verbatim — "a single shared, mutable
System.Randomis not thread-safe and would produce cross-test interference and non-reproducible values" — and choseAsyncLocalcontext-locality as the remedy. That remedy answers which source is in effect, never how it is touched: it covers the cross-test axis, and the mechanism it selects is what propagates the shared instance into the threads a single test spawns. The code is faithful to the ADR and still carried the defect the ADR set out to avoid.ADR-0006 is already superseded by ADR-0026, which does not revisit the question, so this is recorded as a new decision rather than as an edit to a superseded one. It does not conflict with ADR-0006: it supplies the property that was believed to follow from it, and leaves its scoping decision intact.
Related issues
Closes #310
🤖 Generated with Claude Code