fix(tests): verify the fixture git template before handing it out - #978
Merged
Merged
Conversation
|
Warning Review limit reachedNext included review available in 49 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
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 |
wenzowski
marked this pull request as ready for review
September 18, 2026 05:08
wenzowski
marked this pull request as draft
September 18, 2026 05:10
wenzowski
force-pushed
the
claude/glacial-ci-regression-d9qtr3
branch
from
September 18, 2026 06:31
9d21d85 to
54be583
Compare
`git_init_template` publishes by atomic rename, and read a failed rename as "another process published first; its copy is complete". That is the likely cause and not the only one: a `staging/.git` that was never created, a cross-device error, an interrupted publish all arrive at the same `return`, handing back a path nobody checked. `init_repo` then copies whatever is there into a fixture and checks nothing either, so the first thing to notice is a `git add -A` several calls later reporting a directory that is not a repository — a could-not-look wearing a result's clothes. Both ends now assert. The loser's branch requires the published path to be a complete template before returning it, and `init_repo` requires the copy to have produced a repository, naming the template when it did not. `is_template` is the one predicate both rest on, and `a_directory_is_a_template_only_when_it_carries_a_repositorys_own_files` is where it is shown to discriminate: absent, empty, HEAD-only and config-only directories are all refused, and a repository this suite built is accepted. Shown able to fail — with the predicate stubbed to `true` it reds on the absent case. NOT REPRODUCED LOCALLY, stated because it bounds what this claims. The `musl` job on the v0.0.170 release PR reddened with exactly this message on two fixtures in the same millisecond; a full musl `binary(it)` run with the scratch root wiped first passed 3525/3525, and thirty consecutive cold-start races through the same path failed none. So this closes a branch that is unsound on reading and is the only code that emits that message, rather than a race I can demonstrate. Poisoning the real published template would be the stronger arm and is deliberately not taken: that path is shared by every test process under one `CARGO_TARGET_TMPDIR`, so the case would red its concurrent siblings — the cross-process hazard this fix is about. Refs: CLOUD-1832
…ilure The first repair asserted on the branch where the publish rename fails and the published path is not a complete template. CI then failed on exactly that assertion, on a cold musl scratch root, in a case whose subject was elsewhere: the diagnosis was confirmed and the suite was still red. A missing template is a COST. `common` still knows how to fork the repository the template stands for, so `git_init_template` now answers `Option` and `init_repo` forks when the answer is `None` — one `git init` per fixture for that process, which is the behaviour that predates the template. The state that used to be only asserted is printed instead: the rename's error, whether a staged `.git` existed, and what the published path actually contains. Both routes now build the repository through one named function, so the identity the template bakes into its own config cannot drift out of the fallback — a fixture reading it through `git::config_value` cannot tell which route built its repository, and a test pins that. Refs: CLOUD-1832
wenzowski
force-pushed
the
claude/glacial-ci-regression-d9qtr3
branch
from
September 18, 2026 10:49
54be583 to
ed1eb11
Compare
wenzowski
marked this pull request as ready for review
September 18, 2026 10:49
Contributor
Author
|
/fast-forward |
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.
DO-NOT-CLOSE CLOUD-1832
Why
The
musljob on release PR #977 (run 35292062763) reddened with two fixtures failing in the same millisecond:Both call
Fixture::…git().base_commit().init_reporan;git add -Athen found no repository. Not a verdict failure and not a musl divergence —windowsandmacosran the same suite green on the identical head, whose only delta frommainis the version bump.The unsound branch
git_init_templatepublishes by atomic rename and reads a failed rename as "another process published first; its copy is complete". That is the likely cause and not the only one — astaging/.gitthat was never created, a cross-device error, an interrupted publish all reach the samereturn, handing back a path nobody checked.init_repothen copies whatever is there into the fixture and checks nothing either, so the first thing to notice is agit addseveral calls later. A could-not-look wearing a result's clothes.The change
init_reporequires its copy to have produced a repository, naming the template when it did not.is_templateis the one predicate both rest on, anda_directory_is_a_template_only_when_it_carries_a_repositorys_own_filesshows it discriminating: absent, empty, HEAD-only and config-only are refused; a repository this suite built is accepted. Shown able to fail — stubbing the predicate totruereds it on the absent case.What this does not claim
Not reproduced locally. A full musl
binary(it)run with the scratch root wiped first passed 3525/3525, and thirty consecutive cold-start races through the same path failed none. This closes a branch that is unsound on reading and is the only code that emits that message; it is not a race I can demonstrate on demand.Poisoning the real published template would be the stronger end-to-end arm and is deliberately not taken: that path is shared by every test process under one
CARGO_TARGET_TMPDIR, so such a case would red its concurrent siblings — the cross-process hazard this fix is about.CI's musl job runs with a cold scratch root every time, because the
musl-cache family is new; that is the condition under which the first wave of test processes all race to publish.🤖 Generated with Claude Code
https://claude.ai/code/session_01F1kFtyX6Fr37ANwTjr7yEV
Generated by Claude Code