fix: make the init-repo test fixture collision-proof under parallel threads - #25
Open
ashproto wants to merge 2 commits into
Open
fix: make the init-repo test fixture collision-proof under parallel threads#25ashproto wants to merge 2 commits into
ashproto wants to merge 2 commits into
Conversation
…hreads
TempFolder::new() named its directory git-it-init-test-{pid}-{nanos}. The
four initialize_repository tests run on parallel threads in one process, so
the pid is shared and two threads could read the same nanosecond; fs::create_dir
then failed with AlreadyExists and the unwrap() panicked. A different one of
the four failed on each run, which is what made it look random.
Replace the wall-clock stamp with a process-wide AtomicU32 counter, matching
the TempRepo fixtures in ops.rs and ops_worktree.rs. pid + counter cannot
collide by construction, so create_dir is kept over create_dir_all: a
collision should now be impossible, and would be a real bug worth panicking on.
Verified with five consecutive full cargo test runs, all green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4480001d54
ℹ️ 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".
The previous commit claimed pid + counter "cannot collide by construction" and kept `create_dir` over `create_dir_all` on that basis. That reasoning only holds among LIVE processes. The counter restarts at zero every run, so uniqueness rests entirely on the pid never repeating — and a pid is unique only while its process is alive. A run killed before `Drop` (Ctrl-C on cargo test, or a panic=abort) leaves its directories behind; once the OS recycles that pid, a fresh process counting from zero reproduces the exact same path and `create_dir(...).unwrap()` panics with AlreadyExists. Not hypothetical: this machine currently holds 36 orphaned `gte-*` fixture directories spanning 18 distinct pids. Remove any leftover first, then `create_dir_all` — the same two lines the TempRepo fixtures in ops.rs, ops_worktree.rs, ops_merge.rs, ops_rewrite.rs and graph.rs already use. Those fixtures were right and this one was the outlier; the extra line is the point, not redundancy. Reported by Codex review on #25. cargo test -p git-core: 183 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
cargo testintermittently failed one of the fourgit_ops::tests::initialize_repository_*tests. Pre-existing flake, unrelated to any feature work.Cause
TempFolder::new()built its path asgit-it-init-test-{pid}-{nanos}and then calledfs::create_dir(&path).unwrap(). All four tests run on parallel threads in one process, so the pid is shared and two threads can read the same nanosecond;create_dirthen fails withAlreadyExistsand theunwrap()panics. A different one of the four failed on each run, which is what made it look random.Fix
Replace the wall-clock stamp with a process-wide
AtomicU32counter — the pattern theTempRepofixtures inops.rsandops_worktree.rsalready use.pid + countercannot collide by construction.Kept
create_dirovercreate_dir_alldeliberately: a collision is now impossible, so if one ever happens it should panic loudly rather than be absorbed. The now-unusedSystemTime/UNIX_EPOCHimport is removed.Test plan
cargo test -p git-core initialize_repository -- --test-threads=1— 4/4 passcargo testruns, all green (the flake needed repetition to surface)git_may_fail,read_file) are pre-existing dead test helpers in other filesNote
Other
TempRepofixtures (ops_rewrite.rs,ops_remote.rs,graph.rs,ops_merge.rs) each define their own. Any that derive uniqueness from a timestamp rather than a counter could flake the same way under enough parallelism — not touched here since none are currently failing.🤖 Generated with Claude Code