Skip to content

fix(justdummies): serialize draws on a seeded random source - #311

Merged
Reefact merged 8 commits into
mainfrom
claude/justdummies-serialize-random-draws
Jul 27, 2026
Merged

fix(justdummies): serialize draws on a seeded random source#311
Reefact merged 8 commits into
mainfrom
claude/justdummies-serialize-random-draws

Conversation

@Reefact

@Reefact Reefact commented Jul 27, 2026

Copy link
Copy Markdown
Owner

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

  • Bug fix
  • New feature
  • Breaking change
  • Refactoring
  • Analyzer / diagnostic change
  • Tests
  • Documentation
  • Build / CI / tooling

Changes

  • SeededRandom no longer hands out its System.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).
  • The ~35 call sites and the internal signatures that carried a RandomGenerateOrdinal, CountSpec.Resolve, StringSpec.BuildCandidate, RegexGenerationContext, the UriSpec helpers, the RandomSampling extension methods — now carry a SeededRandom. Every hunk is a type substitution; no generation logic changed.
  • ConcurrentDrawTests pins 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 shared AnyContext); five more cover the composed paths that route through the same lock (As, Combine, ListOf, SetOf, StringMatching). One property in SeedDeterminismProperties adds 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 (an OrNull coin, a bare Double) were dropped rather than shipped as false-confidence tests, with the reasoning recorded in the test file.
  • The concurrency contract is stated once and now holds for both sources: XML docs on IAny<T>.Generate — scoped, after review, to the library's own serialized draw, with supplied factories, composers, comparers and foreign IAny<T> implementations left as the caller's responsibility — plus Any.WithSeed and Any.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 — no System.HashCode).
  • The NuGet Description is narrowed from "any run is reproducible from a reported seed" to "any sequential run", so the shipped metadata stops claiming a reproducibility the package cannot deliver under parallelism.
  • ADR-0042 records the decision, drafted as 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.UseSeed scope 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.sln
  • dotnet test FirstClassErrors.sln — full solution green, 0 failures, including FirstClassErrors.Testing.UnitTests, which draws from JustDummies per ADR-0026.
  • Analyzer tests pass (FirstClassErrors.Analyzers.UnitTests) — as part of the solution-wide run above.

Additionally:

  • The three JustDummies suites: 543 + 216 + 14 = 773 tests green (762 before; +10 examples, +1 property).
  • Every new test was confirmed red before the fix by stripping the lock from an out-of-tree copy: the scalar example suite failed on 7 of 8 runs and the property was falsified 3/3, and each composed-path example (As, Combine, ListOf, SetOf, StringMatching) tripped 5/5. Two candidates that missed 5/5 — an OrNull coin flip and a bare Double, 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.csproj under the CI warning ratchet (GITHUB_ACTIONS=true, --no-incremental): 0 warnings.
  • The public API baseline is untouched (git diff -- JustDummies/PublicAPI/ is empty).
  • The original benchmark that surfaced the defect now reports 100% distinct values at every thread/draw configuration, and sequential draws taken after a parallel burst are varied again.

The net472 support floor was not exercised locally: the framework-floor job 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>.Generate remark over-promising for user-supplied code — agreed and fixed by the narrowing above; and removing the French ADR — declined, as adr/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

  • Public API / error documentation updated
  • README / doc/ updated
  • French translation updated if user-facing behavior changed — ArbitraryTestValues.fr.md and the ADR's FR twin
  • No documentation change required

JustDummies/CHANGELOG.md is deliberately untouched: its Unreleased section is drafted automatically from merged pull requests by .github/workflows/changelog.yml.

Architecture decisions

  • No architectural decision in this pull request
  • New decision recorded — ADR drafted as Proposed: ADR-0042
  • Supersedes an existing ADR — successor proposed, status not flipped: ADR-____
  • ⚠️ Conflicts with an existing ADR — flagged for the maintainer: ADR-____

ADR-0006 named this hazard verbatim — "a single shared, mutable System.Random is not thread-safe and would produce cross-test interference and non-reproducible values" — and chose AsyncLocal context-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

claude added 6 commits July 27, 2026 08:01
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
@Reefact

Reefact commented Jul 27, 2026

Copy link
Copy Markdown
Owner Author

@codex

Please perform a thorough and skeptical code review of the fix that was implemented for the following issue.

Implemented fix

Your 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 objectives

1. Understand the original defect

  • Identify the exact behavior that was incorrect.
  • Locate its root cause in the previous implementation.
  • Confirm whether the issue was a real bug, a design weakness, a missing validation, or another type of problem.
  • Describe the concrete scenarios in which the defect could occur.

2. Analyze the implemented fix

  • Explain precisely how the new implementation attempts to solve the issue.
  • Verify that it fixes the root cause rather than merely hiding the observed symptom.
  • Check that every relevant execution path is covered.
  • Detect any incomplete, fragile, overly specific, or accidental behavior.
  • Verify that the fix is consistent with the surrounding architecture and existing JustDummies conventions.

3. Reproduce and validate the behavior

When technically possible:

  • Demonstrate that the previous implementation could reproduce the original bug.
  • Demonstrate that the corrected implementation no longer reproduces it.
  • Run the relevant existing tests.
  • Add or execute targeted tests for the original failing scenario.
  • Test adjacent and boundary scenarios.
  • Run the complete relevant test suite to detect regressions.

Do not rely only on the fact that the existing tests pass.

4. Review the tests

Verify that the tests added or modified for this fix:

  • fail against the implementation before the fix;
  • pass against the corrected implementation;
  • test observable behavior rather than implementation details;
  • cover the actual root cause;
  • include meaningful boundary and edge cases;
  • would detect a future regression;
  • are deterministic and maintainable;
  • do not pass accidentally because their assertions are too weak.

Identify any missing tests and provide concrete test cases when appropriate.

5. Search for regressions

Examine whether the fix could introduce regressions involving:

  • existing public behavior;
  • API compatibility;
  • source or binary compatibility;
  • nullability;
  • exception behavior;
  • validation order;
  • random generation and deterministic seeded generation;
  • shared or mutable state;
  • concurrency or thread safety;
  • performance;
  • allocations;
  • recursive generation;
  • collections and nested generators;
  • configuration reuse;
  • unexpected changes to default behavior.

Only retain the categories that are actually relevant to this fix, but actively investigate the surrounding code for less obvious consequences.

6. Check edge cases

Actively search for inputs and execution paths that the implementation author may have overlooked.

Pay particular attention to:

  • empty values;
  • null values where applicable;
  • minimum and maximum boundaries;
  • contradictory constraints;
  • repeated calls;
  • reused builder or generator instances;
  • different ordering of fluent configuration calls;
  • deterministic seeds;
  • nested generators;
  • collections containing constrained generators;
  • invalid user configuration;
  • culture-sensitive behavior;
  • integer overflow or invalid ranges;
  • unexpected exceptions.

Do not limit the review to this list.

7. Assess design quality

Evaluate whether the fix:

  • is as simple as reasonably possible;
  • preserves encapsulation;
  • remains consistent with the existing public API;
  • respects JustDummies’ purpose as a unit-test dummy generation library;
  • avoids unnecessary abstraction;
  • avoids duplicating existing mechanisms;
  • remains understandable and maintainable;
  • does not create technical debt disproportionate to the corrected issue.

If you believe a better implementation exists, explain it concretely and compare its trade-offs with the current fix.

8. Inspect the complete change scope

Review every file changed as part of this fix.

Detect:

  • unrelated modifications;
  • accidental formatting changes;
  • dead code;
  • obsolete code left behind;
  • misleading comments;
  • documentation no longer matching behavior;
  • public API changes that are not explicitly justified;
  • changes that exceed the scope of the original issue.

Required output

Start with a concise explanation of:

  1. the original root cause;
  2. how the fix works;
  3. whether the fix actually addresses the root cause.

Then list all findings, ordered by severity:

  • Critical
  • High
  • Medium
  • Low
  • Informational

For every finding, include:

  • the affected file and code location;
  • the problematic scenario;
  • why it matters;
  • a concrete recommendation;
  • a suggested test when relevant.

Finish with the following verdict:

Final verdict

  • Correctness: Correct / Partially correct / Incorrect / Unable to prove
  • Completeness: Complete / Missing cases / Substantially incomplete
  • Regression risk: Low / Moderate / High
  • Test quality: Sufficient / Improvements required / Insufficient
  • Scope discipline: Appropriate / Contains unrelated changes
  • Recommendation: Approve / Approve with minor changes / Request changes / Reject

Important constraints

  • Be skeptical and evidence-driven.
  • Do not approve the fix solely because the test suite passes.
  • Do not report hypothetical concerns without tracing them to a realistic code path.
  • Clearly distinguish proven defects from risks or improvement suggestions.
  • Cite exact code locations for every actionable finding.
  • Do not modify the implementation unless explicitly requested.
  • If you cannot prove that the fix is correct, state exactly what prevents that conclusion.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread JustDummies/IAny.cs Outdated
claude added 2 commits July 27, 2026 09:17
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
@Reefact
Reefact enabled auto-merge July 27, 2026 09:42
@Reefact
Reefact merged commit 512b51c into main Jul 27, 2026
16 checks passed
@Reefact
Reefact deleted the claude/justdummies-serialize-random-draws branch July 27, 2026 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JustDummies: concurrent draws from one seeded source corrupt it permanently

2 participants