Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions src/git/reap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,18 @@ pub struct CwdProcess {

/// Timeout for the `lsof` / `ps` probes. Discovery is opt-in and off the hot
/// path, but a hung probe should still not stall removal indefinitely.
const PROBE_TIMEOUT: Duration = Duration::from_secs(5);
///
/// Tests override via `WORKTRUNK_TEST_PROBE_TIMEOUT_MS` (set generously in
/// `STATIC_TEST_ENV_VARS`): under suite load a probe's spawn alone can stall
/// past the production bound, and the timeout's fail-safe empty result then
/// reads as "no processes to reap" — a load-dependent test outcome.
fn probe_timeout() -> Duration {
let ms = std::env::var("WORKTRUNK_TEST_PROBE_TIMEOUT_MS")
.ok()
.and_then(|s| s.parse().ok())
.unwrap_or(5_000);
Duration::from_millis(ms)
}

/// Parse `lsof -d cwd -F pcn` field output into one [`CwdProcess`] per process.
///
Expand Down Expand Up @@ -153,7 +164,7 @@ fn without_controlling_terminal(pids: &[u32]) -> HashSet<u32> {
// partial-output shape `lsof` has in `processes_under`).
let Ok(output) = Cmd::new("ps")
.args(["-o", "pid=,tty=", "-p", &pid_list])
.timeout(PROBE_TIMEOUT)
.timeout(probe_timeout())
.run()
else {
return HashSet::new();
Expand Down Expand Up @@ -182,7 +193,7 @@ pub fn processes_under(worktree_path: &Path) -> Vec<CwdProcess> {
// exit status. Only a spawn failure (lsof missing) means "no data".
let Ok(output) = Cmd::new("lsof")
.args(["-d", "cwd", "-F", "pcn"])
.timeout(PROBE_TIMEOUT)
.timeout(probe_timeout())
.run()
else {
return Vec::new();
Expand Down
95 changes: 90 additions & 5 deletions src/testing/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ use path_slash::PathExt;

use self::mock_commands::{MockConfig, MockResponse, tea_api_include_stderr};

/// Path to the `wt` binary built by Cargo.
/// Path to the `wt` binary built by Cargo, pinned against concurrent builds.
///
/// Resolved at runtime from `CARGO_BIN_EXE_wt`, which cargo and nextest both
/// provide to integration-test processes, naming the binary the same
Expand All @@ -76,13 +76,93 @@ use self::mock_commands::{MockConfig, MockResponse, tea_api_include_stderr};
/// code that spawns `wt` — `mock_commands` included — belongs in integration
/// tests.
///
/// The returned path is not `CARGO_BIN_EXE_wt` itself but a hardlink to it
/// under `target/<profile>/wt-test-bin/<key>/` (see [`pin_test_binary`]):
/// cargo uplifts `target/debug/wt` by removing the path and recreating it, so
/// a concurrent `cargo build` in the same tree leaves the uplifted path absent
/// for a fraction of a millisecond per rebuild, failing whatever spawn is in
/// flight with `NotFound`. The hardlink keeps the observed binary's inode
/// alive whatever a concurrent cargo does to the uplifted path. Pinned once
/// per test process; every spawn helper routes through here, so no test
/// spawns the unlinkable path directly (`test_wt_spawns_are_pinned`).
///
/// Panics when the variable is absent (the test binary run outside a cargo
/// runner) rather than deriving a path that could name a stale binary.
pub fn wt_bin() -> PathBuf {
PathBuf::from(
std::env::var("CARGO_BIN_EXE_wt")
.expect("CARGO_BIN_EXE_wt not set — only available during `cargo test`"),
)
static PINNED: std::sync::OnceLock<PathBuf> = std::sync::OnceLock::new();
PINNED
.get_or_init(|| {
let uplifted = PathBuf::from(
std::env::var("CARGO_BIN_EXE_wt")
.expect("CARGO_BIN_EXE_wt not set — only available during `cargo test`"),
);
pin_test_binary(&uplifted)
})
.clone()
}

/// Pin `src` where a concurrent cargo can't unlink it, returning the pinned
/// path.
///
/// Hardlinks `src` into a sibling `wt-test-bin/<mtime>-<len>/` directory named
/// by the observed binary's identity, keeping `src`'s basename. The hardlink
/// pins the inode: cargo's uplift only unlinks the *path*, so the pinned entry
/// keeps serving the observed binary through any number of concurrent
/// rebuilds. A link costs no space while the `deps/` artifact it shares an
/// inode with exists, and everything under `wt-test-bin/` dies with
/// `cargo clean`, so nothing sweeps it.
///
/// Two runs observing the same binary converge on the same entry (`link`
/// returning `AlreadyExists` is success — the key names the content); a run
/// observing a rebuilt binary creates a new entry beside the old one, exactly
/// as it would have spawned the rebuilt binary before pinning existed. A
/// `NotFound` from `stat` or `link` is the uplift window itself — the binary
/// is absent for well under a millisecond — so it's polled through rather than
/// surfaced (bounded; a genuinely missing binary still panics, with the path).
pub fn pin_test_binary(src: &Path) -> PathBuf {
let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10);
loop {
match try_pin(src) {
Ok(pinned) => return pinned,
Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
assert!(
std::time::Instant::now() < deadline,
"binary to pin stayed absent for 10s: {}",
src.display()
);
std::thread::sleep(std::time::Duration::from_millis(10));
}
Err(e) => panic!("failed to pin test binary {}: {e}", src.display()),
}
}
}

/// One pin attempt. `NotFound` means `src` was observed mid-uplift (or the
/// pin directory's ancestors vanished under a `cargo clean`); the caller
/// retries those.
fn try_pin(src: &Path) -> std::io::Result<PathBuf> {
let meta = std::fs::metadata(src)?;
let mtime = meta
.modified()?
.duration_since(std::time::UNIX_EPOCH)
.unwrap_or_default()
.as_nanos();
let dir = src
.parent()
.expect("binary path has a parent")
.join("wt-test-bin")
.join(format!("{mtime:x}-{:x}", meta.len()));
let pinned = dir.join(src.file_name().expect("binary path has a file name"));
if pinned.exists() {
return Ok(pinned);
}
std::fs::create_dir_all(&dir)?;
match std::fs::hard_link(src, &pinned) {
// A concurrent test process pinned the same key first; the key names
// the content, so its entry is ours.
Err(e) if e.kind() != std::io::ErrorKind::AlreadyExists => Err(e),
_ => Ok(pinned),
}
}

use tempfile::TempDir;
Expand Down Expand Up @@ -472,6 +552,11 @@ pub const STATIC_TEST_ENV_VARS: &[(&str, &str)] = &[
// Disable delayed streaming for deterministic output across platforms.
// Without this, slow CI triggers progress messages that don't appear on faster systems.
("WORKTRUNK_TEST_DELAYED_STREAM_MS", "-1"),
// Give the `--reap` probes (`git::reap::probe_timeout`) a load-proof
// bound. At the production 5s, a probe spawn stalling under suite load
// trips the timeout, whose fail-safe empty result turns "reap the child"
// into "No processes to reap" — a load-dependent outcome.
("WORKTRUNK_TEST_PROBE_TIMEOUT_MS", "60000"),
// Treat shells as not installed by default so the "Skipped …; rc not found"
// filter in scan_shell_configs is deterministic across hosts. Tests that need
// a shell to count as installed (e.g., to assert the Skipped path) set "1".
Expand Down
5 changes: 3 additions & 2 deletions tests/CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ cargo test --test integration # integration
cargo test --test integration --features shell-integration-tests # + shell tests
```

Every binary the suite spawns is `wt` itself — the mock commands are the same binary linked under other names, dispatching on argv[0] (`testing::mock_stub`) — so no run can spawn missing or stale code: cargo rebuilds a package's own binaries whenever its integration tests build, under every runner and filter, and the `CARGO_BIN_EXE_wt` the harness resolves at runtime names that just-built binary; outside a cargo runner the suite panics ("CARGO_BIN_EXE_wt not set") rather than guessing a path. `cargo build --bin wt` recompiling right after a test run is the bin-only build being a separate cached unit (a different feature graph), not evidence the tests ran stale code.
Every binary the suite spawns is `wt` itself — the mock commands are the same binary linked under other names, dispatching on argv[0] (`testing::mock_stub`) — so no run can spawn missing or stale code: cargo rebuilds a package's own binaries whenever its integration tests build, under every runner and filter, and `wt_bin()` resolves `CARGO_BIN_EXE_wt` — naming that just-built binary — into a hardlink pinned under `target/debug/wt-test-bin/`, which a concurrent `cargo build`'s uplift can't unlink mid-run (see No Retries); outside a cargo runner the suite panics ("CARGO_BIN_EXE_wt not set") rather than guessing a path. `cargo build --bin wt` recompiling right after a test run is the bin-only build being a separate cached unit (a different feature graph), not evidence the tests ran stale code.

**Claude Code web:** `task setup-web` installs zsh/fish/nushell, `gh`, and dev tools. Install `task` first if needed: `sh -c "$(curl --location https://taskfile.dev/install.sh)" -- -d -b ~/bin` then `export PATH="$HOME/bin:$PATH"`. The permission tests (`test_permission_error_prevents_save`, `test_approval_prompt_permission_error`) skip automatically when running as root.

Expand Down Expand Up @@ -116,7 +116,7 @@ cmd.args(["switch", "--create", "feature"])

```rust
// ❌ BAD: Missing isolation - inherits host environment
let output = Command::new(env!("CARGO_BIN_EXE_wt"))
let output = Command::new(wt_bin())
.args(["switch", "--create", "feature"])
.current_dir(repo.root_path())
.output()?;
Expand Down Expand Up @@ -389,6 +389,7 @@ Tests run once. Worktrunk configures no nextest `retries` and writes no retry lo
- A racy assertion is a timing bug. Make it deterministic, per Timing Tests above: poll for the event, or drive it causally through a callback.
- Resource pressure is a concurrency bug. Windows process creation intermittently fails with STATUS_DLL_INIT_FAILED (exit `-1073741502`) when many tests spawn git/wt children at once. Bound how many heavy tests run together; that removes the pressure instead of retrying past it.
- A shared channel with more than one producer is an attribution bug. Counting events drained from a channel between steps only measures the step that produced them if nothing *else* can produce them: a background task's event landing after its step's drain is charged to the next step, so the assertion that fails names the wrong step and the wrong cause. The events are usually indistinguishable (skim's `Event::RunPreview` carries no payload), so identity assertions aren't available — quiesce the other producers instead, before the step sequence arms whatever they'd match. `on_update_pokes_run_preview_only_when_the_visible_pane_changes` is the worked example: `PreviewOrchestrator::wait_for_idle()` before each `note_awaiting`, so the skeleton precompute lands while no key matches it.
- A spawn that fails `NotFound` is a concurrent-build bug. Cargo uplifts `target/debug/wt` by removing the path and recreating it, so a second `cargo` against the same target directory leaves the binary absent for a fraction of a millisecond per rebuild, failing whatever spawn is in flight with `Os { code: 2, kind: NotFound }` — anywhere: an `insta_cmd` snapshot, a PTY wrapper's `wt config shell init`, a hook whose marker file then never appears — and passing on re-run, which is the signature. `wt_bin()` closes the window by spawning a hardlink pinned under `target/debug/wt-test-bin/` instead of the uplifted path (`testing::pin_test_binary`), so route every `wt` spawn through it or a helper that does; `test_wt_spawns_are_pinned` makes a direct `CARGO_BIN_EXE_wt` spawn fail the suite.
- A shared namespace is a collision bug. **Never `NamedTempFile::new()`** for a file a test needs by name: `tempfile` retries a name collision only when it surfaces as `AlreadyExists`, and on Windows `create_new` against a name already held by a *directory* — or by a file in delete-pending state — comes back `PermissionDenied`, which it hands straight back to the caller. A full suite run leaves the temp directory full of `.tmpXXXXXX` entries (every `TestRepo` makes one), and under that load the call fails ~1% of the time with `Access is denied.` — a panic that has nothing to do with what the test asserts. Take a `TempDir` and give the files fixed names inside it (`worktrunk::testing::directive_files` is the pattern); a directory collision surfaces as `AlreadyExists`, which tempfile retries.

**Reproducing a Windows-only flake** means reproducing its *neighbours*, not starving it: run the suspect tests in a loop on a Windows runner with a full `cargo nextest run` going alongside. Pinning the CPU instead models the wrong thing whenever the failing run's own timing was normal (compare its duration against the same test passing — nextest prints it, and the `test` job uploads `junit.xml` with every test's time). Measured both ways on the same five tests: 80 iterations under a concurrent full suite reproduced a 1-in-100 Windows failure that no local run had ever shown, while pinning all four cores with busy loops instead just pushed nearly every iteration past the 30s waits — artificial failures that say nothing about the flake.
Expand Down
80 changes: 76 additions & 4 deletions tests/common/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1128,8 +1128,14 @@ fn setup_snapshot_settings_for_paths_with_home(
// debug builds and crane/release builds (notably the nightly nix-flake
// sandbox). The pattern is anchored on `/target/.../wt` so it matches
// the bin path in "Invoked as:" / "Binary invoked as:" / diagnostic
// hint output without touching unrelated `target/` paths.
settings.add_filter(r"/target/(debug|release)/wt", "/target/[BUILD_MODE]/wt");
// hint output without touching unrelated `target/` paths. The optional
// segment collapses the pinned spawn path (`testing::pin_test_binary`'s
// `wt-test-bin/<mtime>-<len>/wt`), whose key would otherwise leak a
// build-specific value into snapshots.
settings.add_filter(
r"/target/(?:debug|release)/(?:wt-test-bin/[0-9a-f]+-[0-9a-f]+/)?wt",
"/target/[BUILD_MODE]/wt",
);

// Normalize shell probe binary paths
// Shell probe reports the actual binary location which varies by system
Expand Down Expand Up @@ -1322,9 +1328,11 @@ pub fn add_pty_binary_path_filters(settings: &mut insta::Settings) {
//
// Include the literal `[BUILD_MODE]` placeholder so this filter still
// collapses to `[BIN]` when the prelude's `target/(debug|release)/wt`
// → `target/[BUILD_MODE]/wt` rewrite has already run.
// → `target/[BUILD_MODE]/wt` rewrite has already run. The optional
// segment covers the pinned spawn path (`testing::pin_test_binary`'s
// `wt-test-bin/<mtime>-<len>/wt`).
settings.add_filter(
r"[^\s]+/target/(?:[^/\s]+/)*(?:debug|release|\[BUILD_MODE\])/wt",
r"[^\s]+/target/(?:[^/\s]+/)*(?:debug|release|\[BUILD_MODE\])/(?:wt-test-bin/[0-9a-f]+-[0-9a-f]+/)?wt",
"[BIN]",
);
}
Expand All @@ -1345,6 +1353,70 @@ mod tests {
/// an identity on top, never `pty_env_vars`. `useConfigOnly` fires only
/// where an identity is missing, so no wrapper assertion would notice the
/// floor going missing — this pins it instead.
/// The uplifted `target/debug/wt` is removed and recreated by any
Comment thread
max-sixty marked this conversation as resolved.
Resolved
/// concurrent `cargo build`, so the suite spawns a pinned hardlink
/// instead (`testing::pin_test_binary`). The pin must keep serving the
/// observed binary through that unlink, converge across processes
/// observing the same binary, and track a rebuild as a new entry.
#[test]
fn pin_test_binary_tracks_generations_and_survives_unlink() {
let dir = worktrunk::testing::test_tempdir();
let src = dir.path().join("wt");
std::fs::write(&src, "generation A").unwrap();

let pinned_a = worktrunk::testing::pin_test_binary(&src);
assert_ne!(pinned_a, src);
assert_eq!(worktrunk::testing::pin_test_binary(&src), pinned_a);

// The uplift: the observed path vanishes; the pin doesn't.
std::fs::remove_file(&src).unwrap();
assert_eq!(std::fs::read(&pinned_a).unwrap(), b"generation A");

// A rebuilt binary (the lengths differ, so the key differs whatever
// the filesystem's mtime granularity) pins beside the old entry,
// which keeps serving processes that observed the old binary.
std::fs::write(&src, "generation B, rebuilt").unwrap();
let pinned_b = worktrunk::testing::pin_test_binary(&src);
assert_ne!(pinned_b, pinned_a);
assert_eq!(std::fs::read(&pinned_b).unwrap(), b"generation B, rebuilt");
assert_eq!(std::fs::read(&pinned_a).unwrap(), b"generation A");
}

/// `wt_bin()` pins the binary against concurrent-build uplifts. A spawn
/// naming the `CARGO_BIN_EXE_wt` path directly bypasses the pin and can
/// hit the uplift's `NotFound` window, so the variable has exactly one
/// reader: `wt_bin()` itself.
#[test]
fn test_wt_spawns_are_pinned() {
// Built from parts so this file doesn't match its own needle.
let needle = ["CARGO_BIN_EXE_", "wt\""].concat();
let root = Path::new(env!("CARGO_MANIFEST_DIR"));
let mut offenders = Vec::new();
for dir in ["src", "tests"] {
scan_for_needle(&root.join(dir), &needle, root, &mut offenders);
}
assert_eq!(
offenders,
vec![PathBuf::from("src/testing/mod.rs")],
"spawn wt via wt_bin() (or a helper that does), never via \
CARGO_BIN_EXE_wt directly — the uplifted path can vanish under \
a concurrent cargo build"
);
}

fn scan_for_needle(dir: &Path, needle: &str, root: &Path, offenders: &mut Vec<PathBuf>) {
for entry in std::fs::read_dir(dir).unwrap().flatten() {
let path = entry.path();
if path.is_dir() {
scan_for_needle(&path, needle, root, offenders);
} else if path.extension().and_then(|s| s.to_str()) == Some("rs")
&& std::fs::read_to_string(&path).unwrap().contains(needle)
{
offenders.push(path.strip_prefix(root).unwrap().to_path_buf());
}
}
}

#[test]
fn configure_pty_command_carries_the_git_config_floor() {
let mut cmd = portable_pty::CommandBuilder::new("true");
Expand Down
Loading
Loading