Skip to content

fix(test): serialise the stats tests that share the global revert map - #431

Merged
avrabe merged 4 commits into
mainfrom
fix/stats-test-race
Oct 22, 2026
Merged

avrabe merged 4 commits into
mainfrom
fix/stats-test-race

Conversation

@avrabe

@avrabe avrabe commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

What failed

PR #428 changes only a workflow file, yet its Verification Gate went red on a Rust unit test:

thread 'stats::tests::record_and_read' panicked at loom-core/src/stats.rs:197:9:
assertion `left == right` failed
  left: None
 right: Some(3)

Why

record_and_read writes its own pass name three times and then reads it back. Its comment says the unique name prevents races. It doesn't: take_resets in the same module calls take_revert_summary(), which clears the whole process-global map. When the two tests run on parallel threads, the clear can land between the writes and the read, and the read returns None.

Evidence

runs of stats::tests --test-threads=8 failures
main 400 1
this PR 2000 0

The counts alone don't prove the fix: one failure in 400 runs is a single observation, not a known rate. The argument rests on the mechanism. Both tests now hold one test-only lock, so the clear and the read can't interleave. The lock recovers from poisoning, so one failing test can't take the other down with it.

Is that the only race on these globals? I checked every reader of the global stats in the workspace:

  • take_resets is the only test that clears the revert map.
  • record_and_read is the only test that reads it.
  • Every other test adds keys through record_revert, and an added key can't disturb a read of a unique key.
  • The coverage tests build local CoverageSummary values and never touch the global.

So the lock covers every test pair that can race. cargo clippy --all-targets -D warnings and cargo fmt --check are clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01RZof1M5HMBSYVZPeoT4MEn

`record_and_read` used a unique pass name to avoid racing other tests, but
`take_resets` clears the WHOLE process-global map. On a parallel test
thread the clear can land between `record_and_read`'s writes and its read,
which then sees `None`:

    assertion `left == right` failed
      left: None
     right: Some(3)

It turned PR #428's Verification Gate red, on a PR that changes only a
workflow file. Reproduced on main: 1 failure in 400 runs of
`stats::tests --test-threads=8`. With both tests holding a shared test
lock: 0 in 2000. The lock recovers from poison, so one failing test
cannot cascade into the other.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RZof1M5HMBSYVZPeoT4MEn
@github-actions

Copy link
Copy Markdown

Rivet verification gate

✅ 30/38 passed

count
Passed 30
Failed 0
Skipped (no steps) 8

Filter: (and (= type "feature") (matches id "^TEST-"))

Failed artifacts

(none)

Updated automatically by tools/post_verification_comment.py. Source of truth: artifacts/verification.yaml.

@avrabe
avrabe merged commit 63b7c71 into main Oct 22, 2026
25 of 26 checks passed
@avrabe
avrabe deleted the fix/stats-test-race branch October 22, 2026 23:47
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.

1 participant