chore(apr-cli): untrack the generated 28 MB test.apr — and fix the race fix it was hiding - #3059
Merged
Merged
Conversation
…ce fix it was hiding `crates/apr-cli/playbooks/snapshots/test.apr` is generated on demand by pixel_regression.rs::test_apr_file(), which creates it only when it is missing. #3053 tracked it via `git add -A crates/apr-cli`. 28 MB of git history for a file the tests write themselves — and, worse, a present fixture means the creation path is NEVER taken, so #3051's atomic-publish race fix was dead code. Measured, on this tree: fixture tracked+present, `panic!()` at the top of publish_atomically -> 5 passed. The fix cannot be reached; any bug in it is invisible. fixture untracked+absent, same panic -> 5 failed, all five reaching publish_atomically. That is the #3051 race. fixture untracked+absent, panic removed (the code as shipped) -> 4 of 5 FAILED: rename ENOENT. The third line is a real defect that shipped green because of the second. The temp name was `apr.tmp.{process::id()}` — unique per PROCESS. #3051 was written against nextest, which gives each test its own process; plain `cargo test --test pixel_regression` runs the five as THREADS in one process, so they shared one temp path, the first thread renamed it away, and the rest died on `rename` with ENOENT. Fixed with a monotonic counter beside the pid: the pid separates processes, the counter separates threads. `publish_atomically_is_unique_per_call_not_per_process` gates it directly — 8 threads publish concurrently, the result must be one complete file with no temp left behind. Reverting to the pid-only name turns it RED (verified). pixel_regression was also absent from ci.yml's `--test` line, so this whole target has never run in CI and the gate above would have been born dark. Added. Cargo `exclude` already kept the fixture out of the published package, so this costs nothing at publish time and removes 28 MB from the repo. Verified: `cargo test -p apr-cli --test pixel_regression` 6 passed / 0 failed with the fixture absent; cargo fmt clean. Refs #3051, #3053 Pmat-Ticket: PMAT-1095
|
§13.11 rung 1 — quorum shadow verdict Shadow mode: this records a verdict and merges nothing. A refusal |
…ion into ci.yml scripts/tree_reader_tests.txt is DERIVED from ci.yml's `--test` line, so adding `cargo test -p apr-cli --test pixel_regression` to it made the registry stale and ci_test_tier.sh refused (exit 2, naming its own remedy). Regenerated with `check_tree_reader_tests.sh --update`: 41 targets, 38 unwired. The tier decision now resolves for this PR's diff: tier=quick, targets include apr-cli:--test:pixel_regression which is the point -- without it the new race gate would run nowhere. Pmat-Ticket: PMAT-1095
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
1 participant
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.
What
crates/apr-cli/playbooks/snapshots/test.apris generated on demand bypixel_regression.rs::test_apr_file(), which creates it only when it is missing. #3053 tracked it viagit add -A crates/apr-cli. That is 28 MB of git history for a file the tests write themselves — and, worse, a present fixture means the creation path is never taken, so #3051's atomic-publish race fix was dead code.The measurement
publish_atomicallypanic!()injected at toppanic!()renameENOENTThe third row is a real defect that shipped green because of the second.
Root cause
The temp name was
apr.tmp.{process::id()}— unique per process. #3051 was written against nextest, which gives each test its own process. Plaincargo test --test pixel_regressionruns the five as threads in one process, so they shared one temp path: the first thread renamed it away and the rest died onrenamewith ENOENT.Fixed with a monotonic counter beside the pid — the pid separates processes, the counter separates threads.
Gate
publish_atomically_is_unique_per_call_not_per_processgates it directly: 8 threads publish concurrently, the result must be one complete file with no temp left behind. Reverting to the pid-only name turns it RED (verified).pixel_regressionwas also absent fromci.yml's--testline, so this target has never run in CI and the new gate would have been born dark. Added.Cargo
excludealready kept the fixture out of the published package, so this costs nothing at publish time and removes 28 MB from the repo.Verification
cargo test -p apr-cli --test pixel_regression— 6 passed / 0 failed with the fixture absentcargo fmt --all -- --checkcleanRefs #3051, #3053
🤖 Generated with Claude Code
https://claude.ai/code/session_018RouwmUL7vFfJyCx9qLEoH