From 8f71baa4d83289dfc7c029cc5b1a9db7e8317ff8 Mon Sep 17 00:00:00 2001 From: Marc-Merino <35137847+Marc-Merino@users.noreply.github.com> Date: Mon, 12 Oct 2026 08:55:49 -0500 Subject: [PATCH] Isolate every test workspace's landing locks and shorten run checkout paths Windows CI failed in three ways: parallel tests that share project `demo` grabbed the same landing ownership lock in the shared temp directory (`os error 80` while another test held it); the run checkout directory under `.run.checkouts/<64-character hash>` grew past Windows' path limit (`fatal: '$GIT_DIR' too big'`); and the live-output streaming test gave `sh` five seconds to start, which Windows runners regularly miss. The test harness now gives every spawned knit process a temp directory private to its workspace (TMP/TEMP/TMPDIR keyed by cwd), so landing locks are shared only inside a single test; revert_landed's local wrappers for the same problem fold back into the harness. Run checkout directories use a twelve-character hash prefix, like plan fingerprints already do. The streaming test's deadline rises to thirty seconds; the step's own timeout still bounds the command. Knit-Group: kg_20261012_bbe991 Knit-Bundle: isolate-test-landing-locks-per-workspace --- src/commands/land/v2/runtime.rs | 6 ++++- tests/common/mod.rs | 40 +++++++++++++++++++++++++++++++++ tests/land.rs | 9 ++++---- tests/land_fork_lifecycle.rs | 9 ++++---- tests/landing_gates.rs | 9 ++++---- tests/landing_sequence.rs | 9 ++++---- tests/landing_v2.rs | 13 ++++++----- tests/revert_fallback.rs | 1 + tests/revert_landed.rs | 40 +++------------------------------ 9 files changed, 77 insertions(+), 59 deletions(-) diff --git a/src/commands/land/v2/runtime.rs b/src/commands/land/v2/runtime.rs index e46e96d..4d99874 100644 --- a/src/commands/land/v2/runtime.rs +++ b/src/commands/land/v2/runtime.rs @@ -674,10 +674,14 @@ fn pinned_roots(step: &Value, roots: &Roots, journal: &Journal, phase: &str) -> .get(&repo) .context("pinned checkout binding missing")?; let rev = rev.as_str().context("revision must be string")?; + // Keep the checkout directory short: this path sits deep under the + // workspace, and a full 64-character hash pushes it past Windows' + // 260-character limit (`fatal: '$GIT_DIR' too big`). Twelve hex + // characters still separate every repo and revision in one run. let checkout = journal .path .with_extension("checkouts") - .join(canonical_hash(&json!([repo, rev]))); + .join(&canonical_hash(&json!([repo, rev]))[..12]); if !checkout.exists() { if git(root, &["cat-file", "-e", &format!("{rev}^{{commit}}")]).is_err() { let identity = super::mergeability::repo_identity(&snapshot["sourceBundle"], &repo); diff --git a/tests/common/mod.rs b/tests/common/mod.rs index fbce102..b2d8548 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -437,6 +437,39 @@ pub fn isolated_git_config_global() -> String { dir.join("gitconfig").to_string_lossy().to_string() } +/// A stable temp directory private to one workspace path: the same cwd always +/// maps to the same directory, and different workspaces map apart. +fn workspace_temp_dir(cwd: &Path) -> PathBuf { + use std::collections::hash_map::DefaultHasher; + use std::hash::{Hash, Hasher}; + let mut hasher = DefaultHasher::new(); + cwd.hash(&mut hasher); + let dir = dunce_canonicalize_or(std::env::temp_dir()).join(format!( + "knit-test-{}-{:x}", + std::process::id(), + hasher.finish() + )); + fs::create_dir_all(&dir).unwrap(); + dir +} + +/// Give a spawned `knit` process a temp directory private to its workspace. +/// +/// Landing ownership locks live in `std::env::temp_dir()` and are keyed only +/// by project id, which most fixtures share (`demo`): parallel tests that land +/// at the same time would otherwise fight over one lock file (`create_new` +/// fails while another test holds it, `os error 80` on Windows). A temp dir +/// keyed by workspace keeps locks shared only inside a single test. Windows +/// resolves the temp dir from `TMP`/`TEMP` and Unix from `TMPDIR`, so set all +/// three. Applied before test-provided env, a test can still override. +pub fn private_temp_env(cwd: &Path, command: &mut Command) { + let temp = workspace_temp_dir(cwd).into_os_string(); + command + .env("TMPDIR", &temp) + .env("TMP", &temp) + .env("TEMP", &temp); +} + /// Ambient identity overrides (exported by editor/agent harnesses such as a /// T3 Code session) would silently override the per-repo `git config` identity /// every fixture sets and inject actor attribution, breaking actor/author @@ -470,6 +503,7 @@ where .env("GIT_CONFIG_GLOBAL", isolated_git_config_global()) .env_remove("KNIT_BUNDLE") .env_remove("KNIT_SESSION"); + private_temp_env(cwd, &mut command); scrub_ambient_git_identity(&mut command); run(command) } @@ -487,6 +521,7 @@ where .env("GIT_CONFIG_GLOBAL", isolated_git_config_global()) .env_remove("KNIT_BUNDLE") .env_remove("KNIT_SESSION"); + private_temp_env(cwd, &mut command); scrub_ambient_git_identity(&mut command); // Test-provided env wins, so a test can still point KNIT_HOME at its own dir. for (key, value) in env { @@ -508,6 +543,7 @@ where .env("GIT_CONFIG_GLOBAL", isolated_git_config_global()) .env_remove("KNIT_BUNDLE") .env_remove("KNIT_SESSION"); + private_temp_env(cwd, &mut command); scrub_ambient_git_identity(&mut command); let output = command.output().unwrap(); assert!(!output.status.success()); @@ -531,6 +567,7 @@ where .env("GIT_CONFIG_GLOBAL", isolated_git_config_global()) .env_remove("KNIT_BUNDLE") .env_remove("KNIT_SESSION"); + private_temp_env(cwd, &mut command); scrub_ambient_git_identity(&mut command); for (key, value) in env { command.env(key, value); @@ -578,6 +615,7 @@ where .env_remove("KNIT_SESSION") .env("PATH", path) .env("GH_FAKE_DIR", fake_gh_dir); + private_temp_env(cwd, &mut command); scrub_ambient_git_identity(&mut command); for (key, value) in env { command.env(key, value); @@ -624,6 +662,7 @@ where .env_remove("KNIT_SESSION") .env("PATH", path) .env("GH_FAKE_DIR", fake_gh_dir); + private_temp_env(cwd, &mut command); scrub_ambient_git_identity(&mut command); for (key, value) in env { command.env(key, value); @@ -663,6 +702,7 @@ where .env_remove("KNIT_SESSION") .env("PATH", path) .env("FORGE_FAKE_DIR", fake_dir); + private_temp_env(cwd, &mut command); scrub_ambient_forge_env(&mut command); scrub_ambient_git_identity(&mut command); for (key, value) in env { diff --git a/tests/land.rs b/tests/land.rs index 61d66b2..dc0e7dc 100644 --- a/tests/land.rs +++ b/tests/land.rs @@ -1283,16 +1283,17 @@ fn terminating_knit_cancels_deployment_tree_and_records_failure() { knit(&workspace, ["bundle", "cancelled deploy"]); knit(&workspace, ["land", "plan"]); - let child = Command::new(env!("CARGO_BIN_EXE_knit")) + let mut child = Command::new(env!("CARGO_BIN_EXE_knit")); + child .args(["land", "apply"]) .current_dir(&workspace) .env("KNIT_HOME", isolated_knit_home()) .env_remove("KNIT_BUNDLE") .env_remove("KNIT_SESSION") .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .spawn() - .unwrap(); + .stderr(Stdio::piped()); + private_temp_env(&workspace, &mut child); + let child = child.spawn().unwrap(); let marker_deadline = Instant::now() + Duration::from_secs(5); while !child_pid_path.exists() && Instant::now() < marker_deadline { diff --git a/tests/land_fork_lifecycle.rs b/tests/land_fork_lifecycle.rs index a6c5b90..fa3d7df 100644 --- a/tests/land_fork_lifecycle.rs +++ b/tests/land_fork_lifecycle.rs @@ -189,7 +189,8 @@ impl Fixture { } } fn cmd(&self, args: &[&str]) -> std::process::Output { - Command::new(env!("CARGO_BIN_EXE_knit")) + let mut command = Command::new(env!("CARGO_BIN_EXE_knit")); + command .current_dir(&self.root) .args(args) .env_remove("KNIT_BUNDLE") @@ -198,9 +199,9 @@ impl Fixture { .env("GIT_CONFIG_GLOBAL", isolated_git_config_global()) .env("GH_TOKEN", "synthetic") .env("KNIT_GITHUB_API_TRANSPORT", "native") - .env("KNIT_GITHUB_API_BASE", &self.api) - .output() - .unwrap() + .env("KNIT_GITHUB_API_BASE", &self.api); + private_temp_env(&self.root, &mut command); + command.output().unwrap() } fn ok(&self, args: &[&str]) -> String { let o = self.cmd(args); diff --git a/tests/landing_gates.rs b/tests/landing_gates.rs index f73a779..54104f3 100644 --- a/tests/landing_gates.rs +++ b/tests/landing_gates.rs @@ -133,7 +133,8 @@ else: raise Exception('unexpected forge call '+str(a)) .chain(std::env::split_paths(&std::env::var_os("PATH").unwrap())), ) .unwrap(); - Command::new(env!("CARGO_BIN_EXE_knit")) + let mut command = Command::new(env!("CARGO_BIN_EXE_knit")); + command .current_dir(&self.workspace) .args(args) .env("PATH", path) @@ -142,9 +143,9 @@ else: raise Exception('unexpected forge call '+str(a)) .env_remove("KNIT_BUNDLE") .env_remove("KNIT_SESSION") .env_remove("GH_TOKEN") - .env_remove("GITHUB_TOKEN") - .output() - .unwrap() + .env_remove("GITHUB_TOKEN"); + private_temp_env(&self.workspace, &mut command); + command.output().unwrap() } fn run_path(&self) -> PathBuf { fs::read_dir(self.workspace.join(".knit/land-runs")) diff --git a/tests/landing_sequence.rs b/tests/landing_sequence.rs index 840b9f4..63feb4d 100644 --- a/tests/landing_sequence.rs +++ b/tests/landing_sequence.rs @@ -203,14 +203,15 @@ impl Fixture { } fn cmd(&self, args: &[&str]) -> std::process::Output { - Command::new(env!("CARGO_BIN_EXE_knit")) + let mut command = Command::new(env!("CARGO_BIN_EXE_knit")); + command .current_dir(&self.root) .args(args) .env_remove("KNIT_BUNDLE") .env_remove("KNIT_SESSION") - .env("KNIT_HOME", self.root.join("home")) - .output() - .unwrap() + .env("KNIT_HOME", self.root.join("home")); + private_temp_env(&self.root, &mut command); + command.output().unwrap() } fn plan(&self) -> Value { diff --git a/tests/landing_v2.rs b/tests/landing_v2.rs index 1bbb48e..ab39b8b 100644 --- a/tests/landing_v2.rs +++ b/tests/landing_v2.rs @@ -91,14 +91,15 @@ elif sys.argv[1]=='verify': assert state.read_text()==json.loads(os.environ['KNI } } fn cmd(&self, args: &[&str]) -> std::process::Output { - Command::new(env!("CARGO_BIN_EXE_knit")) + let mut command = Command::new(env!("CARGO_BIN_EXE_knit")); + command .current_dir(&self.root) .args(args) .env_remove("KNIT_BUNDLE") .env_remove("KNIT_SESSION") - .env("KNIT_HOME", self.root.join("home")) - .output() - .unwrap() + .env("KNIT_HOME", self.root.join("home")); + private_temp_env(&self.root, &mut command); + command.output().unwrap() } } impl Drop for Fixture { @@ -181,7 +182,9 @@ fn ordinary_commands_stream_before_exit_and_keep_receipts_in_both_output_modes() drop(send); let mut stdout = Vec::new(); let mut stderr = Vec::new(); - let deadline = Instant::now() + Duration::from_secs(5); + // Windows runners start `sh`/Python slowly enough that five seconds + // was flaky; the step's own timeout still bounds the command. + let deadline = Instant::now() + Duration::from_secs(30); while !(String::from_utf8_lossy(if json_mode { &stderr } else { &stdout }) .contains("LIVE-STDOUT") && String::from_utf8_lossy(&stderr).contains("LIVE-STDERR")) diff --git a/tests/revert_fallback.rs b/tests/revert_fallback.rs index a31dc88..f8b1780 100644 --- a/tests/revert_fallback.rs +++ b/tests/revert_fallback.rs @@ -164,6 +164,7 @@ impl Landed { .env("GIT_CONFIG_GLOBAL", isolated_git_config_global()) .env_remove("KNIT_BUNDLE") .env_remove("KNIT_SESSION"); + private_temp_env(&self.workspace, &mut command); scrub_ambient_forge_env(&mut command); for (key, value) in &self.env { command.env(key, value); diff --git a/tests/revert_landed.rs b/tests/revert_landed.rs index e5c2e9c..d3cfd0a 100644 --- a/tests/revert_landed.rs +++ b/tests/revert_landed.rs @@ -9,43 +9,9 @@ use std::path::{Path, PathBuf}; const BUNDLE: &str = "venue-capacity"; // Landing ownership locks live in the temp directory and are keyed by project, -// and every test here uses project `demo`; a private temp directory keeps -// parallel tests apart. Windows takes it from TMP/TEMP rather than TMPDIR. -fn private_tmp(fake_bin: &Path) -> String { - let dir = fake_bin.parent().unwrap().join("tmp"); - fs::create_dir_all(&dir).unwrap(); - dir.to_string_lossy().into_owned() -} - -fn knit_with_fake_gh(cwd: &Path, args: I, fake_bin: &Path, fake_gh_dir: &Path) -> String -where - I: IntoIterator, - S: AsRef, -{ - let tmp = private_tmp(fake_bin); - knit_with_fake_gh_env( - cwd, - args, - fake_bin, - fake_gh_dir, - &[("TMPDIR", &tmp), ("TMP", &tmp), ("TEMP", &tmp)], - ) -} - -fn knit_fails_with_fake_gh(cwd: &Path, args: I, fake_bin: &Path, fake_gh_dir: &Path) -> String -where - I: IntoIterator, - S: AsRef, -{ - let tmp = private_tmp(fake_bin); - knit_fails_with_fake_gh_env( - cwd, - args, - fake_bin, - fake_gh_dir, - &[("TMPDIR", &tmp), ("TMP", &tmp), ("TEMP", &tmp)], - ) -} +// and every test here uses project `demo`; the common test harness gives each +// workspace its own temp directory (TMP/TEMP/TMPDIR), keeping parallel tests +// apart on every platform. /// Two-repo project whose landing merges backend before frontend, published /// as `venue-capacity`.