From 2e09f404446dcc862dfd22de798d7cebf4e84159 Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 29 Jul 2026 01:57:32 +0700 Subject: [PATCH 1/3] Fix the audit job: resolve a lockfile, and stop compiling cargo-audit Two problems with the job as first written. Cargo.lock is not committed here, per library convention, so cargo audit had nothing to read and failed with "Couldn't load ./Cargo.lock". Resolving one in CI is also the more meaningful check for a library: consumers resolve their own graph, so what matters is whether a fresh resolution pulls in an advisory, not whether some pinned snapshot did. rustsec/audit-check compiles cargo-audit from source on every run, which took 2m53s against a ~40s build for the crate itself. On a runner billed by vCPU-minute that is most of the job's cost spent on the tool rather than the work, so this installs a prebuilt binary instead. Verified locally first: 42 dependencies, no advisories, so the original failure was the missing lockfile and not a real vulnerability. --- .github/workflows/ci.yml | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 53e75f8..ae3a4f6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -57,14 +57,27 @@ jobs: runs-on: ubicloud-standard-2 permissions: contents: read - # `audit-check` reports advisories as check annotations. - checks: write - issues: write steps: - uses: actions/checkout@v4 - # Worth having now that the crate is published: an advisory against tokio - # or serde reaches every consumer, and nothing else here would notice. - - uses: rustsec/audit-check@v2 + - uses: dtolnay/rust-toolchain@stable + + # `Cargo.lock` is not committed, per library convention, so there is + # nothing for audit to read until one is resolved. That is also the more + # meaningful check for a library: consumers resolve their own graph, so + # what matters is whether a *fresh* resolution pulls in an advisory, not + # whether some pinned snapshot did. + - name: Resolve a dependency graph to audit + run: cargo generate-lockfile + + # A prebuilt binary rather than `rustsec/audit-check`, which compiles + # cargo-audit from source on every run. That took 2m53s here, against a + # ~40s build for the crate itself, and Ubicloud bills by vCPU-minute. + - uses: taiki-e/install-action@v2 with: - token: ${{ secrets.GITHUB_TOKEN }} + tool: cargo-audit + + # Worth having now that the crate is published: an advisory against tokio + # or serde reaches every consumer, and nothing else in CI would notice. + - name: Audit dependencies + run: cargo audit From e9658d6f2b1629e32011a8b47c0cdcc80bfb75e3 Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 29 Jul 2026 02:01:57 +0700 Subject: [PATCH 2/3] Fix the flaky drop test, which is red on master `dropping_a_run_kills_the_agent_and_its_children` asserted the grandchild was gone after a fixed 300ms sleep. That held locally and on the PR runner, and failed on master's, which is exactly the shape of a timing bet rather than a test. The bet was wrong in principle, not just unlucky. `Drop` cannot await, so it aborts the driver task and the teardown runs when the runtime next polls it: prompt, but not synchronous. A loaded 2-vCPU runner beat the sleep. The contract is "killed promptly", so the test now polls for that with a 10s ceiling, which still fails a genuine leak rather than hanging on it. The cancel test keeps its immediate, unpolled assertion. That asymmetry is the point: `cancel` awaits its own teardown, so if the tree is not already gone when it returns, the contract really is broken. Documented the distinction where callers will read it, in `Run`'s docs and the README: both kill the tree, only `cancel` tells you when. --- README.md | 6 +++++- src/run.rs | 9 +++++++-- tests/process.rs | 43 ++++++++++++++++++++++++++++++++----------- 3 files changed, 44 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 8bcc2ca..4e95bab 100644 --- a/README.md +++ b/README.md @@ -142,7 +142,11 @@ running.detach(); // opt out: keep running unsupervised `cancel` is cooperative rather than an abort: the driver signals the process group, reaps the child and joins its readers before returning `Error::Cancelled`. So when it returns the tree really is gone, which matters if the next thing you do touches the files it was working -on. `drop` cannot await, so it signals and then aborts as a backstop. +on. + +`drop` cannot await, so it signals and aborts as a backstop. That is prompt but **not +synchronous**: teardown runs when the runtime next polls the aborted task. Both kill the +tree; only `cancel` tells you when. Each run gets its own process group on Unix, and cancellation, drop and timeout all tear down the whole group. Killing only the CLI would orphan the commands *it* started, which diff --git a/src/run.rs b/src/run.rs index a13e6c9..f34fea7 100644 --- a/src/run.rs +++ b/src/run.rs @@ -77,8 +77,13 @@ const EVENT_BUFFER: usize = 256; /// this crate targets: closing a window or cancelling a request should stop the /// work, not leave an agent running invisibly, spending quota and touching /// files with nobody watching. Call [`Run::detach`] when background execution is -/// genuinely what you want, or [`Run::cancel`] to stop one deterministically and -/// wait for it to die. +/// genuinely what you want. +/// +/// Dropping is prompt but not synchronous: `Drop` cannot await, so it asks the +/// driver to stop and aborts it, and the teardown runs when the runtime next +/// polls that task. If you need to *know* the tree is gone before doing +/// something else, such as touching the files it was working on, use +/// [`Run::cancel`], which waits for exactly that. #[derive(Debug)] pub struct Run { events: mpsc::Receiver, diff --git a/tests/process.rs b/tests/process.rs index 4c2511e..8327b25 100644 --- a/tests/process.rs +++ b/tests/process.rs @@ -74,11 +74,29 @@ async fn grandchild_pid(dir: &Path) -> i32 { panic!("the fake agent never spawned its grandchild"); } -/// Give the OS a moment to reap after a kill. -async fn settle() { - tokio::time::sleep(Duration::from_millis(300)).await; +/// Wait for a pid to disappear, up to `limit`. Returns whether it did. +/// +/// `Drop` cannot await, so it aborts the driver task and the actual teardown +/// happens when the runtime next polls it. That is prompt but not synchronous, +/// so asserting after a fixed sleep is a bet on scheduler timing: it held +/// locally and on a quiet runner, and lost on a loaded 2-vCPU CI runner. What +/// the contract actually promises is "killed promptly", so that is what this +/// waits for. +async fn wait_until_dead(pid: i32, limit: Duration) -> bool { + let deadline = tokio::time::Instant::now() + limit; + while tokio::time::Instant::now() < deadline { + if !alive(pid) { + return true; + } + tokio::time::sleep(Duration::from_millis(25)).await; + } + !alive(pid) } +/// Long enough to absorb a loaded CI runner, short enough that a genuine leak +/// still fails the test rather than hanging it. +const TEARDOWN_GRACE: Duration = Duration::from_secs(10); + /// The default that matters for a GUI: closing a window must stop the agent, /// not leave it running invisibly and spending quota. #[tokio::test] @@ -92,10 +110,9 @@ async fn dropping_a_run_kills_the_agent_and_its_children() { assert!(alive(grandchild), "the grandchild should be running"); drop(running); - settle().await; assert!( - !alive(grandchild), + wait_until_dead(grandchild, TEARDOWN_GRACE).await, "dropping the run left a grandchild ({grandchild}) alive; \ killing only the CLI orphans whatever it spawned" ); @@ -115,8 +132,9 @@ async fn cancel_stops_the_whole_tree_before_returning() { let err = running.cancel().await.unwrap_err(); assert!(err.is_cancelled(), "cancel should report itself: {err:?}"); - // No settle(): cooperative cancellation must have reaped the tree before - // returning, so the check is immediate rather than after a grace period. + // Checked immediately, with no polling, unlike the drop test above. That + // asymmetry is the point: `cancel` awaits its own teardown, so if the tree + // is not already gone when it returns, the contract is broken. assert!( !alive(grandchild), "cancel returned while a grandchild was still alive, so it is not \ @@ -142,9 +160,10 @@ async fn a_timed_out_run_kills_its_children() { matches!(err, agent_abstraction::Error::Timeout { .. }), "got {err:?}" ); - settle().await; - - assert!(!alive(grandchild), "the timeout left a grandchild alive"); + assert!( + wait_until_dead(grandchild, TEARDOWN_GRACE).await, + "the timeout left a grandchild alive" + ); std::fs::remove_dir_all(&dir).ok(); } @@ -159,7 +178,9 @@ async fn detach_lets_a_run_outlive_its_handle() { let grandchild = grandchild_pid(&dir).await; running.detach(); - settle().await; + // A brief pause is right here rather than a poll: the assertion is that + // nothing kills it, so the test has to give something the chance to. + tokio::time::sleep(Duration::from_millis(500)).await; assert!( alive(grandchild), From 38d1e4ed3a5106294eb3af685db98c6a49c375c9 Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 29 Jul 2026 02:05:42 +0700 Subject: [PATCH 3/3] Treat a zombie as dead when checking whether a process survived My previous commit diagnosed this as flaky timing and added polling. That was wrong: it failed the same way on the next run, deterministically on Linux while passing on macOS, which is a platform difference rather than a race. The liveness check was `kill -0`, which succeeds for a zombie: a process that has exited but whose parent has not reaped it. On a developer machine init reaps orphans immediately so the distinction never shows. A CI container's PID 1 is frequently not a real init, so an orphaned grandchild sits as a zombie and `kill -0` reports it alive indefinitely, long after the kill worked. So the process-group teardown was correct the whole time and the test was measuring the wrong thing. Now it reads the state from `ps` and treats `Z` as dead, which it is: a killed process is dead whether or not anyone collected its exit status. Failures now report the actual process state, so the next surprise here diagnoses itself instead of costing another round of guessing. The polling from the previous commit stays. It was aimed at the wrong cause but is still correct on its own terms, since Drop cannot await and its teardown is prompt rather than synchronous. --- tests/process.rs | 40 ++++++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/tests/process.rs b/tests/process.rs index 8327b25..9f478e9 100644 --- a/tests/process.rs +++ b/tests/process.rs @@ -50,14 +50,27 @@ fn fake_agent(dir: &Path) -> PathBuf { script } -/// Whether a pid is still alive, via a null signal. +/// The process state as `ps` reports it, or `None` if the pid is gone. +fn process_state(pid: i32) -> Option { + let out = std::process::Command::new("ps") + .args(["-o", "stat=", "-p", &pid.to_string()]) + .output() + .ok()?; + let state = String::from_utf8_lossy(&out.stdout).trim().to_string(); + (!state.is_empty()).then_some(state) +} + +/// Whether a pid is still running. +/// +/// Deliberately not `kill -0`, which succeeds for a **zombie**: a process that +/// has exited but whose parent has not reaped it. That distinction does not +/// matter on a developer machine, where init reaps orphans immediately, but a +/// CI container's PID 1 is often not a real init, so an orphan can sit as a +/// zombie indefinitely and `kill -0` keeps reporting it alive long after it +/// died. A killed process is dead whether or not anyone collected its exit +/// status. fn alive(pid: i32) -> bool { - // `kill -0` reports liveness without actually signalling. - std::process::Command::new("kill") - .args(["-0", &pid.to_string()]) - .stderr(std::process::Stdio::null()) - .status() - .is_ok_and(|s| s.success()) + process_state(pid).is_some_and(|state| !state.starts_with('Z')) } /// Wait for the grandchild to record its pid, then return it. @@ -113,8 +126,9 @@ async fn dropping_a_run_kills_the_agent_and_its_children() { assert!( wait_until_dead(grandchild, TEARDOWN_GRACE).await, - "dropping the run left a grandchild ({grandchild}) alive; \ - killing only the CLI orphans whatever it spawned" + "dropping the run left a grandchild ({grandchild}) alive in state {:?}; \ + killing only the CLI orphans whatever it spawned", + process_state(grandchild) ); std::fs::remove_dir_all(&dir).ok(); } @@ -137,8 +151,9 @@ async fn cancel_stops_the_whole_tree_before_returning() { // is not already gone when it returns, the contract is broken. assert!( !alive(grandchild), - "cancel returned while a grandchild was still alive, so it is not \ - awaiting its own cleanup" + "cancel returned while a grandchild was still alive (state {:?}), so it \ + is not awaiting its own cleanup", + process_state(grandchild) ); std::fs::remove_dir_all(&dir).ok(); } @@ -162,7 +177,8 @@ async fn a_timed_out_run_kills_its_children() { ); assert!( wait_until_dead(grandchild, TEARDOWN_GRACE).await, - "the timeout left a grandchild alive" + "the timeout left a grandchild alive in state {:?}", + process_state(grandchild) ); std::fs::remove_dir_all(&dir).ok(); }