fix: a baseline that disappears mid-test raises instead of passing green (#217) - #278
Conversation
…een (#217) Two concurrently-running tests asserting the SAME screenshot name race on one set of files -- every artifact path derives from the name alone. The winner's `archive_baseline!` moves the baseline the loser just checked out; the loser's `need_to_compare?` then finds none, records the screenshot as NEW, and returns without comparing. Green, having verified nothing. The parallel audit measured 18-34 such skips out of 64. "No baseline was ever committed" is a legitimate state with its own warning. "The baseline I just checked out has disappeared" is impossible in a correct run. They were indistinguishable at the point where it matters, so the second was recorded as the first. `Snap#checkout_base_screenshot` now tells the session when git really handed it a baseline, and `AssertionRegistry#record_new_screenshot` raises rather than recording a name it knows had one. The record is per-session (fiber-local) and cleared by `reset`: checkout and comparison happen in one call stack, and the session outlives the test, so a name read in one test must not raise in the next. test/unit/baseline_disappeared_test.rb reproduces the race with two free-running threads on one name -- silent skips in 4 of 5 runs before the fix, 0 in 10 runs after. docs: state that `parallelize(with: :threads)` is verified clean subject to name uniqueness, and that the vanished baseline is now an error.
Reviewer's GuideThe PR closes the silent comparison-skip race by recording successful baseline checkouts in the session and raising when such a baseline disappears before comparison, with reset-safe tracking, race-focused regression tests, and updated thread-safety documentation. Sequence diagram for detecting a disappeared screenshot baselinesequenceDiagram
participant Test
participant Snap
participant Git as SnapshotManager
participant Session as AssertionRegistry
participant Archive
Test->>Snap: checkout_base_screenshot()
Snap->>Git: checkout_file(path, base_path)
Git-->>Snap: checked_out
Snap->>Session: record_baseline_checkout(full_name)
Test->>Session: need_to_compare?
Archive->>Git: archive_baseline!
Git-->>Session: baseline missing
Session->>Session: record_new_screenshot(name)
alt baseline was checked out
Session-->>Test: raise SnapDiff::Error
else no baseline was ever committed
Session-->>Test: record new screenshot
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reachedNext included review available in 25 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…289) `jruby-10.0/rails80` timed out on master run 32763256451 and took the whole run red. Tests were still printing dots when SIGTERM landed, so the cell was SLOW, not hung -- and the same cell had passed the previous three runs. The 15-minute per-attempt budget was sized from run 32643567648, where a clean JRuby attempt was 545-713s: "~26% headroom over the slowest", as the comment says. That measurement is stale. Re-measured on run 32758898367, the last green one before this bit: rails71 652s rails81 740s rails80 870s rails72 881s <- against a 900s cap The suite grew from 646 to 757 runs in between (#274, #277, #278, #279, #283), and the headroom went with it: 881/900 is 2%. The cells have been passing by seconds, which is why this looked stable for three runs and then was not. 20 minutes restores the ~26% margin this was originally sized for, and the job cap follows to keep the arithmetic true: 1 + 20 + 20 = 41. Both numbers move together on purpose -- a per-attempt timeout that does not fit the cap kills the last attempt mid-run and reports `cancelled`, which reads as an absence rather than a failure. MRI is unchanged: 128s against 3 minutes. Costs nothing on a green run; it is a ceiling, not a sleep.
Closes the 2.0 subset of #217: the one silently wrong finding from the parallel-execution audit.
The bug
Every artifact path derives from the screenshot name alone. Two concurrently-running tests asserting the same name therefore race on one set of files: the winner's
archive_baseline!moves the baseline the loser just checked out, and the loser'sneed_to_compare?then sees no baseline, records the screenshot as new, and returns without comparing. The test passes green having verified nothing. The audit measured 18–34 skipped comparisons out of 64.The fix
"No baseline was ever committed" is a legitimate state, already warned about (#255). "The baseline I just checked out has disappeared" is impossible in a correct run, and was indistinguishable from the first at the point where it matters.
Snap#checkout_base_screenshotrecords on the session that git really handed it a baseline.AssertionRegistry#record_new_screenshotraises for a name it knows had one, instead of recording it as never-committed.The record is per-session (fiber-local — checkout and comparison happen in one call stack) and cleared by
reset, since the session outlives the test.Additive only — no behaviour change for any correct run (ADR-010: 2.0 breaks nothing).
Evidence
test/unit/baseline_disappeared_test.rb— two free-running threads, one name, no injected deletion:The audit's other, already-loud failure mode (the baseline vanishing after
need_to_compare?, giving aVips::Erroron a missing file) still appears in the tally and is untouched — it was never silent.Mutation transcript (each restored with a targeted edit; both files verified byte-identical by sha256 afterwards):
record_new_screenshotguard →if false && …Snap#checkout_base_screenshotrecording →if checked_out && false@checked_out_baselines.clearfromresetGates:
rake test:unit(698 runs) andrake test:canonical(587 runs) green locally and underCI=true;standardrb lib testclean.Docs
docs/thread_safety.mdanddocs/reporters.mdalready stated thatparallelize(with: :threads)is clean and that deprecation notices print once per fork worker. Updated here: the threads row now names the screenshot-name-uniqueness precondition, and the name-collision section says the vanished baseline is now an error rather than a silent pass.Left on #217
fail_if_new_screenshotunderrecord: :none— the victim of the same race raises the misleading "No existing screenshot found … git add" message. Loud, not silent, so not the bug this PR closes; the seam is inscreenshot_matcher.rb, owned by another lane today.Refs #217. Do not merge yet.
Summary by Sourcery
Prevent concurrent screenshot-name collisions from silently skipping baseline comparisons by distinguishing vanished checked-out baselines from genuinely new screenshots.
Bug Fixes:
Enhancements:
Documentation:
Tests: