From 58265b2233df204003f68a7c62fb5862c4ee8604 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Fri, 18 Sep 2026 02:37:39 +0000 Subject: [PATCH 1/2] fix(tests): the fixture template is verified before it is handed out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- crates/batten/tests/it/common/mod.rs | 56 +++++++++++++++++++++-- crates/batten/tests/it/primitives.rs | 66 ++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 4 deletions(-) diff --git a/crates/batten/tests/it/common/mod.rs b/crates/batten/tests/it/common/mod.rs index 0ea0dea99..ae3306282 100644 --- a/crates/batten/tests/it/common/mod.rs +++ b/crates/batten/tests/it/common/mod.rs @@ -830,7 +830,7 @@ fn git_init_template() -> &'static Path { static TEMPLATE: std::sync::OnceLock = std::sync::OnceLock::new(); TEMPLATE.get_or_init(|| { let published = target_tmp().join(format!("git-init-template-{}", git_stamp())); - if published.join("HEAD").is_file() && published.join("config").is_file() { + if is_template(&published) { return published; } let staging = target_tmp().join(format!( @@ -847,9 +847,30 @@ fn git_init_template() -> &'static Path { // fixture copies is a repository directory, and lifting it here keeps // `init_repo` from having to know the template's internal layout. if fs::rename(staging.join(".git"), &published).is_err() { - // Another process published first, which is the whole point of the - // rename. Its copy is complete; ours is not needed. + // A FAILED RENAME IS NOT A REPORT THAT SOMEBODY ELSE WON (CLOUD-1832). + // That is the LIKELY reading — `rename(2)` onto a non-empty directory + // is `ENOTEMPTY` and the winner's copy is complete — but it is not the + // only one. A `staging/.git` that was never created, a cross-device + // error, a publish interrupted mid-flight: every one of them arrives + // here, and returning the path unasked turns "I could not establish a + // template" into "here is a template". `init_repo` then copies + // whatever is at that path into a fixture, and the first thing to + // notice is a `git add -A` three calls later reporting a directory + // that is not a repository — a could-not-look wearing a result's + // clothes, which is the one thing this repository refuses. + // + // So the loser's branch asserts what it assumed. let _ = fs::remove_dir_all(&staging); + assert!( + is_template(&published), + "the template publish at {} failed and {} is not a complete \ + template, so this process could not establish one — see \ + CLOUD-1832. Every fixture under CARGO_TARGET_TMPDIR copies this \ + path, so continuing would hand them a `.git` git does not \ + recognise", + staging.display(), + published.display(), + ); return published; } let _ = fs::remove_dir_all(&staging); @@ -857,6 +878,17 @@ fn git_init_template() -> &'static Path { }) } +/// Whether `dir` is a published template this module may copy from. +/// +/// The same two files [`git_init_template`] checks before it stages, named once +/// so the pre-check and the post-check cannot drift into disagreeing about what +/// "published" means. Deliberately cheap and structural rather than a `git` +/// fork: the question is whether the directory is a repository at all, and a +/// fork per fixture is the cost this template exists to remove. +pub(crate) fn is_template(dir: &Path) -> bool { + dir.join("HEAD").is_file() && dir.join("config").is_file() +} + /// The resolved `git` binary's length and mtime, as one path-safe token. /// /// Falls back to a literal when `PATH` carries no `git` this can stat — the @@ -932,7 +964,23 @@ pub(crate) fn init_repo(dir: &Path) { dir.display() ); if dir.starts_with(target_tmp()) { - copy_tree(git_init_template(), &dir.join(".git")); + let template = git_init_template(); + copy_tree(template, &dir.join(".git")); + // CHECK WHAT THE COPY PRODUCED, HERE (CLOUD-1832). `copy_tree` creates + // its destination and copies whatever it finds, so a template that was + // not a repository yields a `.git` that is not one either — silently. + // The failure then surfaces wherever the fixture first runs git, which + // in the measured case was `base_commit`'s `git add -A` reporting + // "not a git repository" in a case about config loading. Asserting at + // the point of creation is the difference between a defect that names + // itself and one that reads as an unrelated test being broken. + assert!( + is_template(&dir.join(".git")), + "copying the template {} into {} did not produce a repository — see \ + CLOUD-1832", + template.display(), + dir.display(), + ); } else { git_in(dir, &["init", "-q"]); } diff --git a/crates/batten/tests/it/primitives.rs b/crates/batten/tests/it/primitives.rs index 17d5bf415..3dae4a334 100644 --- a/crates/batten/tests/it/primitives.rs +++ b/crates/batten/tests/it/primitives.rs @@ -1900,3 +1900,69 @@ fn no_case_names_the_template_directory() { `make_empty` would wipe it: {named:?}" ); } + +// THE PREDICATE BOTH TEMPLATE ASSERTIONS REST ON (CLOUD-1832). +// +// `git_init_template` publishes by atomic rename and, when that rename failed, +// used to return the published path unasked — reading "rename failed" as +// "somebody else won and their copy is complete". Every other cause of a failed +// rename arrived at the same `return`, so a process that could not establish a +// template handed one back anyway, and `init_repo` copied it into a fixture. The +// first thing to notice was a `git add -A` several calls later reporting a +// directory that is not a repository: a could-not-look rendered as a result. +// +// Both halves of the repair — the loser's branch, and `init_repo` checking what +// its copy produced — are straight-line uses of `common::is_template`, so this is +// where the discrimination has to be shown. 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 a case that +// corrupted it would red its concurrent siblings — which is precisely the +// cross-process hazard this row is about. +#[test] +fn a_directory_is_a_template_only_when_it_carries_a_repositorys_own_files() { + let root = common::scratch("is-template"); + + assert!( + !common::is_template(&root.join("never-created")), + "a path that does not exist is not a template" + ); + + let empty = root.join("empty"); + std::fs::create_dir_all(&empty).expect("create the empty candidate"); + assert!( + !common::is_template(&empty), + "an empty directory is not a template — that is the shape a failed \ + publish leaves behind, and the one that used to be handed back as \ + though it were a repository" + ); + + let headless = root.join("no-head"); + std::fs::create_dir_all(&headless).expect("create the headless candidate"); + std::fs::write(headless.join("config"), "[core]\n").expect("write config"); + assert!( + !common::is_template(&headless), + "config alone is not a repository" + ); + + let configless = root.join("no-config"); + std::fs::create_dir_all(&configless).expect("create the configless candidate"); + std::fs::write(configless.join("HEAD"), "ref: refs/heads/main\n").expect("write HEAD"); + assert!( + !common::is_template(&configless), + "HEAD alone is not a repository" + ); + + // The positive case comes from a repository this suite actually built, not + // from two files this test wrote: a predicate checked only against hand-made + // directories could agree with itself and disagree with git. + let real = common::Fixture::new("is-template-real") + .file("a.txt", "a\n") + .git() + .base_commit() + .build(); + assert!( + common::is_template(&real.join(".git")), + "a repository this suite just built must satisfy the predicate its own \ + fixtures are checked against" + ); +} From ed1eb115758a45691d06d56a8399cb7b3f71df01 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Fri, 18 Sep 2026 06:30:26 +0000 Subject: [PATCH 2/2] fix(tests): a template that cannot be established is a cost, not a failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- crates/batten/tests/it/common/mod.rs | 172 +++++++++++++++++++-------- crates/batten/tests/it/primitives.rs | 43 +++++++ 2 files changed, 163 insertions(+), 52 deletions(-) diff --git a/crates/batten/tests/it/common/mod.rs b/crates/batten/tests/it/common/mod.rs index ae3306282..0fa5397c9 100644 --- a/crates/batten/tests/it/common/mod.rs +++ b/crates/batten/tests/it/common/mod.rs @@ -807,6 +807,15 @@ pub(crate) fn git_command(dir: &Path, args: &[&str]) -> Command { /// of redundant `git init`s is the wrong trade for a once-per-filesystem cost, /// and a lock file is one more thing to leak. /// +/// # `None` is an answer, not a failure (CLOUD-1832) +/// +/// Every reading of a failed publish that does NOT leave a complete template +/// behind ends here, and the honest report is that this process has no template +/// — not a path it hopes is one. The caller's fallback is the `git init` fork +/// this template exists to avoid, which is a cost rather than a defect, so the +/// suite stays green on a filesystem where the publish cannot land. The state is +/// printed, not swallowed: the next run that hits it says why. +/// /// # The stamp is the `git` binary's own metadata /// /// A hand-bumped version constant would leave a stale template behind whenever @@ -826,56 +835,108 @@ pub(crate) fn git_command(dir: &Path, args: &[&str]) -> Command { /// `.git/config` is the same values by a cheaper route. A fixture that wants a /// DIFFERENT identity still sets it after the copy, and one whose subject is an /// UNSET identity unsets it — `attribution.rs` already does exactly that. -fn git_init_template() -> &'static Path { - static TEMPLATE: std::sync::OnceLock = std::sync::OnceLock::new(); - TEMPLATE.get_or_init(|| { - let published = target_tmp().join(format!("git-init-template-{}", git_stamp())); - if is_template(&published) { - return published; - } - let staging = target_tmp().join(format!( - "git-init-template-{}.staging-{}", - git_stamp(), - std::process::id() - )); +fn git_init_template() -> Option<&'static Path> { + static TEMPLATE: std::sync::OnceLock> = std::sync::OnceLock::new(); + TEMPLATE.get_or_init(build_git_init_template).as_deref() +} + +/// Establish the template once per process, or report that this process could +/// not — see [`git_init_template`] for why the answer is an `Option`. +#[allow(clippy::print_stderr)] +fn build_git_init_template() -> Option { + let published = target_tmp().join(format!("git-init-template-{}", git_stamp())); + if is_template(&published) { + return Some(published); + } + let staging = target_tmp().join(format!( + "git-init-template-{}.staging-{}", + git_stamp(), + std::process::id() + )); + let _ = fs::remove_dir_all(&staging); + fs::create_dir_all(&staging).expect("create the template staging directory"); + fork_the_template_into(&staging); + // Publish the `.git` itself rather than the work tree around it: what a + // fixture copies is a repository directory, and lifting it here keeps + // `init_repo` from having to know the template's internal layout. + if let Err(error) = fs::rename(staging.join(".git"), &published) { + // A FAILED RENAME IS NOT A REPORT THAT SOMEBODY ELSE WON (CLOUD-1832). + // That is the LIKELY reading — `rename(2)` onto a non-empty directory + // is `ENOTEMPTY` and the winner's copy is complete — but it is not the + // only one. A `staging/.git` that was never created, a cross-device + // error, a publish interrupted mid-flight: every one of them arrives + // here, and returning the path unasked turns "I could not establish a + // template" into "here is a template". `init_repo` then copies + // whatever is at that path into a fixture, and the first thing to + // notice is a `git add -A` three calls later reporting a directory + // that is not a repository — a could-not-look wearing a result's + // clothes, which is the one thing this repository refuses. + // + // So the loser's branch reports what it could not establish rather + // than assuming it. It does NOT fail the case: the template is a + // COST optimisation over a `git init` this module still knows how to + // fork, so the sound answer is `None` and one fork for this process. + // Failing here would red a suite whose subject is elsewhere for a + // reason that is purely about how the fixture was built — the + // measured CI failure of exactly that shape is why this is a + // fallback and not an assertion. + let listing = fs::read_dir(&published).map_or_else( + |error| format!("unreadable: {error}"), + |entries| { + let mut names: Vec = entries + .filter_map(Result::ok) + .map(|entry| entry.file_name().to_string_lossy().into_owned()) + .collect(); + names.sort(); + format!("[{}]", names.join(" ")) + }, + ); + let staged_git = staging.join(".git").is_dir(); let _ = fs::remove_dir_all(&staging); - fs::create_dir_all(&staging).expect("create the template staging directory"); - git_in(&staging, &["init", "-q"]); - git_in(&staging, &["config", "user.email", "t@example.com"]); - git_in(&staging, &["config", "user.name", "t"]); - // Publish the `.git` itself rather than the work tree around it: what a - // fixture copies is a repository directory, and lifting it here keeps - // `init_repo` from having to know the template's internal layout. - if fs::rename(staging.join(".git"), &published).is_err() { - // A FAILED RENAME IS NOT A REPORT THAT SOMEBODY ELSE WON (CLOUD-1832). - // That is the LIKELY reading — `rename(2)` onto a non-empty directory - // is `ENOTEMPTY` and the winner's copy is complete — but it is not the - // only one. A `staging/.git` that was never created, a cross-device - // error, a publish interrupted mid-flight: every one of them arrives - // here, and returning the path unasked turns "I could not establish a - // template" into "here is a template". `init_repo` then copies - // whatever is at that path into a fixture, and the first thing to - // notice is a `git add -A` three calls later reporting a directory - // that is not a repository — a could-not-look wearing a result's - // clothes, which is the one thing this repository refuses. - // - // So the loser's branch asserts what it assumed. - let _ = fs::remove_dir_all(&staging); - assert!( - is_template(&published), - "the template publish at {} failed and {} is not a complete \ - template, so this process could not establish one — see \ - CLOUD-1832. Every fixture under CARGO_TARGET_TMPDIR copies this \ - path, so continuing would hand them a `.git` git does not \ - recognise", - staging.display(), - published.display(), - ); - return published; + if is_template(&published) { + return Some(published); } - let _ = fs::remove_dir_all(&staging); - published - }) + // Diagnostics rather than a verdict: this names the state that the + // assertion used to only assert, so the next run that hits it says + // WHY instead of that it happened. + // + // `print_stderr` is allowed here and only here. The lint exists so the + // BINARY never writes an ungoverned line to a channel its output + // contract owns (house-style §6); this is the test harness, whose + // stderr nextest already captures per process and prints on failure. + // The alternative is a fallback that leaves no trace at all — a silent + // change of route, which is the shape that made this defect expensive + // to see. The allowance sits on the function rather than here because + // a statement-scoped one does not reach this macro — clippy reports it + // as an unused attribute. + eprintln!( + "note: the template publish at {} failed ({error}; staged .git \ + present: {staged_git}) and {} is not a complete template \ + (contents: {listing}), so this process forks `git init` per \ + fixture instead — see CLOUD-1832", + staging.display(), + published.display(), + ); + return None; + } + let _ = fs::remove_dir_all(&staging); + Some(published) +} + +/// Build the repository [`git_init_template`] publishes, by forking, in `dir`. +/// +/// The route [`init_repo`] falls back to when no template could be established, +/// and the reason it is a named function rather than three lines inside that +/// `else`: the two `config` calls are not decoration. The template bakes the +/// identity into its own `config`, the binary under test reads it through +/// `git::config_value`, and a fallback that forked `init` alone would hand the +/// fixture an UNSET identity — a difference no fixture asks about and every +/// attribution case would feel. Same commands, same order, same values as the +/// staging build above. +pub(crate) fn fork_the_template_into(dir: &Path) { + git_in(dir, &["init", "-q"]); + git_in(dir, &["config", "user.email", "t@example.com"]); + git_in(dir, &["config", "user.name", "t"]); } /// Whether `dir` is a published template this module may copy from. @@ -963,8 +1024,17 @@ pub(crate) fn init_repo(dir: &Path) { template copy is not, so a second initialisation has to be deliberate", dir.display() ); - if dir.starts_with(target_tmp()) { - let template = git_init_template(); + if !dir.starts_with(target_tmp()) { + git_in(dir, &["init", "-q"]); + return; + } + let Some(template) = git_init_template() else { + // No template could be established (CLOUD-1832): fork what it stands + // for. + fork_the_template_into(dir); + return; + }; + { copy_tree(template, &dir.join(".git")); // CHECK WHAT THE COPY PRODUCED, HERE (CLOUD-1832). `copy_tree` creates // its destination and copies whatever it finds, so a template that was @@ -981,8 +1051,6 @@ pub(crate) fn init_repo(dir: &Path) { template.display(), dir.display(), ); - } else { - git_in(dir, &["init", "-q"]); } } diff --git a/crates/batten/tests/it/primitives.rs b/crates/batten/tests/it/primitives.rs index 3dae4a334..806eb1afd 100644 --- a/crates/batten/tests/it/primitives.rs +++ b/crates/batten/tests/it/primitives.rs @@ -1966,3 +1966,46 @@ fn a_directory_is_a_template_only_when_it_carries_a_repositorys_own_files() { fixtures are checked against" ); } + +// CLOUD-1832, the other half: what a process does when it could NOT establish a +// template. +// +// The first repair asserted, and that is what CI then failed on — the assertion +// fired on a cold musl scratch root and reddened a case whose subject was +// elsewhere. A missing template is a COST, not a defect: `common` still knows +// how to fork the repository the template stands for, so the sound answer is to +// fork it. `init_repo` takes that route whenever `git_init_template` returns +// `None`, and this pins what the route has to produce. +// +// Driving it through `init_repo` itself is not available — the template is a +// process-wide `OnceLock` and a case cannot un-establish it without redding its +// concurrent siblings, the same cross-process hazard the case above declines. So +// the fallback's BODY is the named function both routes call, and it is checked +// directly: a repository, with the identity the template bakes into its config. +// A fallback that forked `init` alone would pass `is_template` and still hand +// every fixture an unset identity, so the identity is asserted, not assumed. +#[test] +fn the_route_taken_when_no_template_exists_builds_the_repository_the_template_would_have() { + let dir = common::scratch("template-fallback").join("repo"); + std::fs::create_dir_all(&dir).expect("create the fallback candidate"); + + common::fork_the_template_into(&dir); + + assert!( + common::is_template(&dir.join(".git")), + "the fallback has to produce a repository — it stands in for a template \ + whose whole content is one" + ); + assert_eq!( + common::git_in(&dir, &["config", "user.email"]), + "t@example.com", + "the template bakes the identity into its own config, so a fallback \ + that left it unset would differ from the copy route in a way the \ + binary under test can read" + ); + assert_eq!( + common::git_in(&dir, &["config", "user.name"]), + "t", + "both halves of the identity, for the same reason" + ); +}