From 50359190baf2229ece92938ebce4880d0f6112b1 Mon Sep 17 00:00:00 2001 From: schickling-assistant <261620128+schickling-assistant@users.noreply.github.com> Date: Thu, 27 Aug 2026 10:30:43 +0200 Subject: [PATCH 1/3] fix(tests): synchronize Darwin lifecycle tests on the event they measure (#356) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three tests spent their behavioral deadline on fixture scheduling, so a loaded aarch64-darwin host failed them while asserting nothing about the behavior: - `input_write_failure_terminates_and_reaps_the_child` gave EPIPE a 1s budget that also had to cover fork+exec of the fixture shell, and reported `timed out after 1.0s` instead. - `the_group_kill_reaps_a_descendant_that_outlives_the_direct_child` read the descendant pidfile after a 2s deadline that the child had not yet met. - `deferred_delivery_backoff_bounds_short_lived_pty_attempts` stopped the DING loop after a fixed 250ms window that ended before its first poll. All three now synchronize on the lifecycle event first, then apply a bounded assertion. `run_captured` already starts the child deadline after `on_spawn` returns, so `on_spawn` is the readiness barrier: `await_fixture_ready` blocks there until the fixture published an atomically renamed marker, and fork+exec is paid outside the deadline rather than out of it. The DING test stops on the loop's own poll counter instead of wall clock. Production cleanup semantics are unchanged; both call sites pass a no-op observer. That ordering is load-bearing and was proven by nothing, so `the_spawn_observer_runs_before_the_child_deadline_starts` pins it: its barrier outlasts the timeout by construction, so only the order decides the outcome. `input_write_obeys_the_child_deadline` had the same latent defect — it timed from before the call, charging fork+exec to the 1s budget it polices — and now times from the observed spawn instant. It cannot use a barrier: it exists to cover the case where the child is never scheduled at all. Counter-tests verified by mutation: dropping `kill(-pid, SIGKILL)`, dropping the `terminate_and_reap_before` on the stdin-failure path, reordering the deadline before `on_spawn`, and dropping the DING backoff each fail their exact test. Adds a hosted `macos-15` job running `nix build .#st2`, which is the surface #356 lives on. st2 drives process groups, signals, pipes and reaping — OS behavior no Linux run can gate — and the 3-core hosted runner is a stricter scheduler than any Mac in the fleet. Closes #356 Co-Authored-By: Claude Opus 5 agent-identity: unknown agent-persona: generalist agent-supervisor: unavailable agent-tool: Claude Code agent-tool-version: 2.1.237 agent-runtime: Claude Code 2.1.237 tooling-profile: dotfiles@11eaf2d-dirty --- .github/workflows/nix.yml | 24 +++++++ src/ding/mod.rs | 22 +++++- src/run.rs | 145 +++++++++++++++++++++++++++++++------- 3 files changed, 166 insertions(+), 25 deletions(-) diff --git a/.github/workflows/nix.yml b/.github/workflows/nix.yml index a40438e1..2e654ef1 100644 --- a/.github/workflows/nix.yml +++ b/.github/workflows/nix.yml @@ -4,6 +4,12 @@ on: push: branches: [main] +# One in-flight run per ref: a push that supersedes an earlier one should not keep +# two ~35m Linux checks and two macOS builds competing for the same runner pool. +concurrency: + group: nix-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + jobs: check: runs-on: ubuntu-latest @@ -17,3 +23,21 @@ jobs: # evaluates every `checks.*`, so fmt, clippy, the hermetic test suite, and # the `--help` smoke test all gate the PR from one entrypoint. - run: nix flake check --print-build-logs + + # Darwin gate. st2 drives real processes — process groups, signals, pipes, reaping — + # and those paths are OS behaviour, not portable Rust: issue #356 was two aarch64-darwin + # test failures that no Linux run could have caught. The hosted macOS runner is arm64 + # with 3 cores, so it is also a *stricter* scheduler than any Mac in the fleet, which is + # what makes it a useful detector for load-sensitive lifecycle tests. + # + # Scope is deliberately `nix build .#st2`, not `nix flake check`: that builds the package + # and runs its hermetic suite via `doCheck` — exactly the surface #356 lives on — without + # also demanding cold aarch64-darwin builds of the `pty` and `otelite` dependencies that + # `checks.parked-recovery` and `checks.otel-export` pull in. Widen once this is stable. + build-darwin: + runs-on: macos-15 + timeout-minutes: 90 + steps: + - uses: actions/checkout@v4 + - uses: DeterminateSystems/determinate-nix-action@v3 + - run: nix build .#st2 --no-link --print-build-logs diff --git a/src/ding/mod.rs b/src/ding/mod.rs index 07409b88..ba48a4cf 100644 --- a/src/ding/mod.rs +++ b/src/ding/mod.rs @@ -3741,9 +3741,24 @@ Enter to select · ↑/↓ to navigate · Esc to cancel"; status_refresh: Duration::from_secs(60), }; + // Synchronize on the loop's own progress rather than a wall-clock window. `session_alive` + // runs exactly once per iteration, so `probes` counts polls directly: stop only once the + // first poke landed AND the loop polled several more times, which is precisely when a + // missing backoff would poke again. A fixed window instead ends wherever the host's + // scheduler leaves it — on a loaded machine before the first poll, so the test observed zero + // pokes and failed while asserting nothing about backoff. + const POLLS_AFTER_FIRST_POKE: usize = 5; + let barrier = Duration::from_secs(30); std::thread::scope(|scope| { scope.spawn(|| { - std::thread::sleep(Duration::from_millis(250)); + let deadline = Instant::now() + barrier; + while poker.calls.lock().unwrap().is_empty() && Instant::now() < deadline { + std::thread::sleep(Duration::from_millis(2)); + } + let polled = poker.probes.load(Ordering::SeqCst) + POLLS_AFTER_FIRST_POKE; + while poker.probes.load(Ordering::SeqCst) < polled && Instant::now() < deadline { + std::thread::sleep(Duration::from_millis(2)); + } stop.store(true, Ordering::SeqCst); }); run_ding( @@ -3761,6 +3776,11 @@ Enter to select · ↑/↓ to navigate · Esc to cancel"; .unwrap(); }); + // Without this the counter-test is vacuous: one poke across one poll proves no backoff. + assert!( + poker.probes.load(Ordering::SeqCst) > POLLS_AFTER_FIRST_POKE, + "the loop did not poll again, so no backoff was exercised" + ); assert_eq!( poker.calls.lock().unwrap().len(), 1, diff --git a/src/run.rs b/src/run.rs index fcdb3d5d..b10a1888 100644 --- a/src/run.rs +++ b/src/run.rs @@ -209,6 +209,11 @@ fn output_with_input_timeout( /// that pid is also its process group id — the group this function signals on every failure path. /// Tests need it to assert the child was reaped, and the child cannot supply it: a test whose /// deadline expires before the child is first scheduled would never see anything the child wrote. +/// +/// It runs BEFORE the child deadline starts, and tests rely on that: a lifecycle test blocks in +/// `on_spawn` until the fixture reached the state it wants to measure, so fork+exec scheduling is +/// paid outside the deadline instead of out of it. See +/// `tests::the_spawn_observer_runs_before_the_child_deadline_starts`. fn output_with_input_timeout_observed( command: &mut Command, timeout: Duration, @@ -261,6 +266,9 @@ fn run_captured( let mut child = command.spawn()?; let pid = child.id() as i32; on_spawn(pid); + // Load-bearing order: the deadline starts after `on_spawn` returns, so a test that blocks there + // as a readiness barrier spends none of `timeout` on fork+exec. Moving this line above + // `on_spawn` is silent in production and makes every barrier test load-sensitive again. let deadline = Instant::now() + timeout; if let Some(input) = input { let Some(stdin) = child.stdin.take() else { @@ -2821,6 +2829,31 @@ mod tests { .next() } + /// Block until the fixture publishes `marker`, which its script creates by an atomic rename so + /// the barrier never observes a half-written file. Called from `on_spawn`, which runs before + /// [`run_captured`] starts the child deadline: fork+exec scheduling is therefore paid here and + /// not out of the deadline the test then measures. The ceiling is deliberately far larger than + /// any plausible fork+exec — it bounds a fixture that never ran at all, and is not itself the + /// behaviour under test, so a loaded host cannot turn it into a failure. + fn await_fixture_ready(pid: i32, marker: &Path, what: &str) { + const CEILING: Duration = Duration::from_secs(30); + let deadline = Instant::now() + CEILING; + while !marker.exists() { + if Instant::now() >= deadline { + // Do not leak the fixture's long sleeper into the test host on the way out. + unsafe { + libc::kill(-pid, libc::SIGKILL); + libc::kill(pid, libc::SIGKILL); + } + panic!( + "{what} within {CEILING:?}: {} never appeared", + marker.display() + ); + } + std::thread::sleep(Duration::from_millis(5)); + } + } + fn process_can_retain_cleanup_resources(pid: i32) -> bool { #[cfg(target_os = "linux")] if linux_process_state(pid) == Some('Z') { @@ -3855,19 +3888,29 @@ mod tests { let temporary = tempfile::tempdir().unwrap(); let executable = temporary.path().join("close-stdin"); - std::fs::write(&executable, "#!/bin/sh\nexec 0<&-\nsleep 60\n").unwrap(); + let stdin_closed = temporary.path().join("stdin-closed"); + // The script signals only AFTER closing its stdin, so the barrier below returns exactly when + // the read end is gone and the parent's very next write must fail with EPIPE. + std::fs::write( + &executable, + "#!/bin/sh\nexec 0<&-\n: > \"$READY.tmp\"\nmv \"$READY.tmp\" \"$READY\"\nsleep 60\n", + ) + .unwrap(); std::fs::set_permissions(&executable, std::fs::Permissions::from_mode(0o755)).unwrap(); let input = vec![b'x'; 1024 * 1024]; - // The pid comes from the parent at spawn. Reading it from a file the child writes made the - // case depend on the child being scheduled inside the deadline: miss that and the read - // panics with `NotFound` before either assertion runs, naming neither the pipe nor the - // deadline. The `on_spawn` seam removes the dependency rather than widening the window. + // The pid comes from the parent at spawn, and the barrier makes the deadline measure only + // the behaviour under test. Without it the 1s budget also had to cover fork+exec of the + // shell, so a loaded host reported `timed out after 1.0s` instead of `Broken pipe` — the + // fixture's scheduling consumed the deadline the assertion is about. let mut spawned = None; let error = output_with_input_timeout_observed( - &mut Command::new(&executable), + Command::new(&executable).env("READY", &stdin_closed), Duration::from_secs(1), Some(input), - |pid| spawned = Some(pid), + |pid| { + spawned = Some(pid); + await_fixture_ready(pid, &stdin_closed, "the child never closed its stdin"); + }, ) .unwrap_err(); let pid = spawned.expect("the child was spawned before the input write failed"); @@ -3896,16 +3939,29 @@ mod tests { let descendant_pidfile = temporary.path().join("descendant.pid"); // The descendant inherits stdout/stderr and outlives the direct child, which is exactly the // shape the docstring describes. `child.kill()` cannot reach it; only the group signal can. + // It publishes its pid by atomic rename, so the barrier never reads a truncated file. std::fs::write( &executable, - "#!/bin/sh\nsh -c 'printf \"%s\" \"$$\" > \"$DESCENDANT_PIDFILE\"; sleep 60' &\nsleep 60\n", + "#!/bin/sh\nsh -c 'printf \"%s\" \"$$\" > \"$DESCENDANT_PIDFILE.tmp\"; mv \"$DESCENDANT_PIDFILE.tmp\" \"$DESCENDANT_PIDFILE\"; sleep 60' &\nsleep 60\n", ) .unwrap(); std::fs::set_permissions(&executable, std::fs::Permissions::from_mode(0o755)).unwrap(); - let error = output_with_timeout( + // This test *requires* the child to have run — a descendant it never forked is nothing to + // reap. Waiting for the pidfile inside `on_spawn` makes that a barrier instead of a race: + // the deadline then only has to outlast a `sleep`, never a fork+exec, so a loaded host can + // no longer end the run before the fixture has built the thing under test. + let error = output_with_input_timeout_observed( Command::new(&executable).env("DESCENDANT_PIDFILE", &descendant_pidfile), - Duration::from_secs(2), + Duration::from_millis(500), + None, + |pid| { + await_fixture_ready( + pid, + &descendant_pidfile, + "the child never forked a descendant, so this case would test nothing", + ) + }, ) .unwrap_err(); assert!( @@ -3913,16 +3969,15 @@ mod tests { "unexpected error: {error:#}" ); - // Unlike the deadline case, this test *requires* the child to have run — a descendant it - // never forked is nothing to reap — so reading the pid it recorded is sound here. Two - // seconds against a fork+exec is a wide margin, and the failure is named rather than a bare - // `NotFound`. let descendant = std::fs::read_to_string(&descendant_pidfile) - .expect("the child never forked a descendant, so this case tested nothing") + .expect("the readiness barrier returned without a pidfile") .parse::() .unwrap(); - let deadline = Instant::now() + Duration::from_secs(1); + // Generous on purpose: the descendant is orphaned by the same group kill, so its exit is + // observable only once the reparenting init reaps it. That latency is not the behaviour + // under test, and waiting longer costs nothing when the kill did reach it. + let deadline = Instant::now() + Duration::from_secs(5); while process_can_retain_cleanup_resources(descendant) && Instant::now() < deadline { std::thread::sleep(Duration::from_millis(10)); } @@ -3975,21 +4030,23 @@ mod tests { std::fs::write(&executable, "#!/bin/sh\nsleep 60\n").unwrap(); std::fs::set_permissions(&executable, std::fs::Permissions::from_mode(0o755)).unwrap(); let input = vec![b'x'; 1024 * 1024]; - let started = Instant::now(); - // The pid comes from the parent at spawn, not from the child. This case is precisely the one - // where the child may never be scheduled: the write blocks as soon as the pipe buffer fills, - // which needs no execution by the child at all, and the deadline then terminates the whole - // group. Anything the child was supposed to record would never be written, so a test that - // waits for it fails on exactly the condition it exists to cover. + // The pid comes from the parent at spawn, not from the child, and this case cannot use a + // readiness barrier: it is precisely the one where the child may never be scheduled. The + // write blocks as soon as the pipe buffer fills, which needs no execution by the child at + // all, and the deadline then terminates the whole group. Anything the child was supposed to + // record would never be written, so a test that waits for it fails on exactly the condition + // it exists to cover. The observed spawn instant is therefore also the clock: timing from + // before the call would charge fork+exec to the 1s budget this assertion polices. let mut spawned = None; let error = output_with_input_timeout_observed( &mut Command::new(&executable), Duration::from_millis(100), Some(input), - |pid| spawned = Some(pid), + |pid| spawned = Some((pid, Instant::now())), ) .unwrap_err(); - let pid = spawned.expect("the child was spawned before the input deadline expired"); + let (pid, started) = + spawned.expect("the child was spawned before the input deadline expired"); assert!( format!("{error:#}").contains("timed out"), @@ -4006,6 +4063,46 @@ mod tests { assert!(!crate::host_lock::process_alive(pid)); } + /// The two lifecycle tests above block in `on_spawn` until their fixture reached the state under + /// test, which only keeps them load-insensitive because [`run_captured`] starts the child + /// deadline AFTER `on_spawn` returns. Nothing else proves that order: reversing it leaves every + /// other test green on an idle host and silently puts both back on a race with the scheduler. + #[test] + fn the_spawn_observer_runs_before_the_child_deadline_starts() { + use std::os::unix::fs::PermissionsExt as _; + + let temporary = tempfile::tempdir().unwrap(); + let executable = temporary.path().join("close-stdin"); + let stdin_closed = temporary.path().join("stdin-closed"); + std::fs::write( + &executable, + "#!/bin/sh\nexec 0<&-\n: > \"$READY.tmp\"\nmv \"$READY.tmp\" \"$READY\"\nsleep 60\n", + ) + .unwrap(); + std::fs::set_permissions(&executable, std::fs::Permissions::from_mode(0o755)).unwrap(); + let timeout = Duration::from_millis(200); + + // The barrier deliberately outlasts `timeout`, so the outcome depends on the order alone and + // on nothing the host's scheduler does. Deadline after `on_spawn`: the write meets a closed + // read end and fails with EPIPE at once. Deadline before `on_spawn`: it has already expired + // when the barrier returns, so the write never runs and the call reports a timeout instead. + let error = output_with_input_timeout_observed( + Command::new(&executable).env("READY", &stdin_closed), + timeout, + Some(vec![b'x'; 1024]), + |pid| { + await_fixture_ready(pid, &stdin_closed, "the child never closed its stdin"); + std::thread::sleep(timeout * 2); + }, + ) + .unwrap_err(); + + assert!( + format!("{error:#}").contains("Broken pipe"), + "the child deadline started before `on_spawn` returned: {error:#}" + ); + } + #[test] fn bounded_capture_keeps_the_tail_of_an_oversized_stream() { use std::os::unix::fs::PermissionsExt as _; From 7980aa1429056f7fdd630efb98ee8d4d82a00c49 Mon Sep 17 00:00:00 2001 From: schickling-assistant <261620128+schickling-assistant@users.noreply.github.com> Date: Thu, 27 Aug 2026 10:41:23 +0200 Subject: [PATCH 2/3] ci: name the Nix jobs for the flake system each one proves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check` and `build-darwin` described different things — one the verb, one the platform — so neither name said which system it gates. `check-x86_64-linux` and `check-aarch64-darwin` use the flake's own system identifiers, and the size difference between them moves into the comment where it can be explained. No required-status-check rule references the old name, so the rename is inert for branch protection. Co-Authored-By: Claude Opus 5 agent-identity: unknown agent-persona: generalist agent-supervisor: unavailable agent-tool: Claude Code agent-tool-version: 2.1.237 agent-runtime: Claude Code 2.1.237 tooling-profile: dotfiles@11eaf2d-dirty --- .github/workflows/nix.yml | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/.github/workflows/nix.yml b/.github/workflows/nix.yml index 2e654ef1..5b6a9baf 100644 --- a/.github/workflows/nix.yml +++ b/.github/workflows/nix.yml @@ -10,8 +10,10 @@ concurrency: group: nix-${{ github.workflow }}-${{ github.ref }} cancel-in-progress: ${{ github.event_name == 'pull_request' }} +# Both jobs are named for the flake system they prove, not for the runner that hosts them. +# They are deliberately not the same size — see the comment on the Darwin job. jobs: - check: + check-x86_64-linux: runs-on: ubuntu-latest # A healthy full `nix flake check` baseline is ~35m; 90m provides # headroom while still failing boundedly. @@ -24,17 +26,17 @@ jobs: # the `--help` smoke test all gate the PR from one entrypoint. - run: nix flake check --print-build-logs - # Darwin gate. st2 drives real processes — process groups, signals, pipes, reaping — - # and those paths are OS behaviour, not portable Rust: issue #356 was two aarch64-darwin - # test failures that no Linux run could have caught. The hosted macOS runner is arm64 - # with 3 cores, so it is also a *stricter* scheduler than any Mac in the fleet, which is - # what makes it a useful detector for load-sensitive lifecycle tests. + # st2 drives real processes — process groups, signals, pipes, reaping — and those paths + # are OS behaviour, not portable Rust: #356 and #255 were aarch64-darwin failures no Linux + # run could have caught. The hosted macOS runner is arm64 with 3 cores, so it is also a + # stricter scheduler than any Mac in the fleet, which makes it a good detector for + # load-sensitive lifecycle tests. # - # Scope is deliberately `nix build .#st2`, not `nix flake check`: that builds the package - # and runs its hermetic suite via `doCheck` — exactly the surface #356 lives on — without - # also demanding cold aarch64-darwin builds of the `pty` and `otelite` dependencies that + # Narrower than its Linux sibling on purpose: `nix build .#st2` builds the package and runs + # its hermetic suite via `doCheck` — the surface those issues live on — without also + # demanding cold aarch64-darwin builds of the `pty` and `otelite` inputs that # `checks.parked-recovery` and `checks.otel-export` pull in. Widen once this is stable. - build-darwin: + check-aarch64-darwin: runs-on: macos-15 timeout-minutes: 90 steps: From 3c20ae432793a5796cb416eba03a1abcf4324fe8 Mon Sep 17 00:00:00 2001 From: schickling-assistant <261620128+schickling-assistant@users.noreply.github.com> Date: Thu, 27 Aug 2026 12:39:52 +0200 Subject: [PATCH 3/3] fix(tests): give the DING startup-backlog barriers their own budget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `startup_backlog_gets_one_generic_recovery_then_new_arrivals_poke` waits for two events — the loop's first poll, then two delivered pokes — but shared one 3s deadline across both. Under sustained load the first wait spent it, so the test stopped the loop after a single poke and failed on the count while naming neither barrier. Each wait now has its own budget, and the ceiling is generous: it bounds a loop that never ran at all, which is not the behaviour under test. The barriers report rather than panic, because an unwind from inside `thread::scope` would leave `run_ding` without the `stop` flag that ends it, turning a failed assertion into a hang. Found by the loaded aarch64-darwin rebuild added in this branch — the fourth instance of the same defect, and the reason the new Darwin job is worth having. Counter-test: dropping the `new_arrivals` queueing so post-start messages never poke fails it with `the ding loop never delivered both notices`. Co-Authored-By: Claude Opus 5 agent-identity: unknown agent-persona: generalist agent-supervisor: unavailable agent-tool: Claude Code agent-tool-version: 2.1.237 agent-runtime: Claude Code 2.1.237 tooling-profile: dotfiles@11eaf2d-dirty --- src/ding/mod.rs | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/src/ding/mod.rs b/src/ding/mod.rs index ba48a4cf..89f74436 100644 --- a/src/ding/mod.rs +++ b/src/ding/mod.rs @@ -1301,6 +1301,22 @@ mod tests { ); } + /// Block until `ready` holds, reporting whether it did. The ceiling bounds a DING loop that + /// made no progress at all; it is not the behaviour under test, so it is far larger than any + /// plausible scheduling delay and a loaded host cannot turn it into a failure. Callers report + /// the result after their scope ends rather than panicking inside it, because an unwind from a + /// scoped thread leaves `run_ding` without the `stop` flag that ends it. + fn await_ding_progress(ready: impl Fn() -> bool) -> bool { + let deadline = Instant::now() + Duration::from_secs(30); + while !ready() { + if Instant::now() >= deadline { + return false; + } + std::thread::sleep(Duration::from_millis(2)); + } + true + } + #[derive(Default)] struct RecordingPoker { alive: AtomicBool, @@ -3690,16 +3706,18 @@ Enter to select · ↑/↓ to navigate · Esc to cancel"; status_refresh: Duration::from_secs(60), }; + // Two separate barriers, each with its own generous budget. Sharing one 3s deadline + // across both meant the first wait could spend it: the loop then stopped after a single + // poke and the count assertion failed while naming neither barrier. The ceiling bounds a + // loop that never ran at all, so a loaded host cannot turn it into a failure — and `stop` + // is set on every path, because panicking here would leave `run_ding` looping forever. + let mut polled = false; + let mut poked_twice = false; std::thread::scope(|scope| { scope.spawn(|| { - let deadline = Instant::now() + Duration::from_secs(3); - while poker.probes.load(Ordering::SeqCst) == 0 && Instant::now() < deadline { - std::thread::yield_now(); - } + polled = await_ding_progress(|| poker.probes.load(Ordering::SeqCst) > 0); send_to_inbox(&inbox, "new", Some("post-start"), None, &[], "new").unwrap(); - while poker.calls.lock().unwrap().len() < 2 && Instant::now() < deadline { - std::thread::sleep(Duration::from_millis(2)); - } + poked_twice = await_ding_progress(|| poker.calls.lock().unwrap().len() >= 2); stop.store(true, Ordering::SeqCst); }); @@ -3718,6 +3736,8 @@ Enter to select · ↑/↓ to navigate · Esc to cancel"; .unwrap(); }); + assert!(polled, "the ding loop never reached its first poll"); + assert!(poked_twice, "the ding loop never delivered both notices"); let calls = poker.calls.lock().unwrap(); assert_eq!(calls.len(), 2); assert_eq!(calls[0], RECOVERY_POKE);