From 658ea6e4ea2db35d06b5645d8735b567544e233b Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 2 Oct 2026 16:10:17 +0100 Subject: [PATCH 01/19] fix: a devpod up no longer outlives a dl that dies without running its handler The own-group child (devpod up) now gets PR_SET_PDEATHSIG(SIGKILL) in its pre_exec, and checks getppid against the pid read before the fork so a parent that died in between is not missed. aid's --boot-up child gets the same with SIGINT, so the boot's own handler still kills its group and unlinks its token. Linux only; a no-op elsewhere. --- rust/aid/src/interactive.rs | 11 ++++-- rust/devlaunch-runner/public-api.txt | 1 + rust/devlaunch-runner/src/interrupt.rs | 55 ++++++++++++++++++++++++++ rust/devlaunch-runner/src/lib.rs | 6 +++ rust/dl/src/lib.rs | 15 +++++++ 5 files changed, 85 insertions(+), 3 deletions(-) diff --git a/rust/aid/src/interactive.rs b/rust/aid/src/interactive.rs index 583ea0b2..55052156 100644 --- a/rust/aid/src/interactive.rs +++ b/rust/aid/src/interactive.rs @@ -77,13 +77,18 @@ impl BootChild { return None; } }; - let spawned = Command::new(me) + let mut command = Command::new(me); + command .arg(BOOT_WORD) .args(boot_args) .stdin(Stdio::null()) .stdout(Stdio::from(out)) - .stderr(Stdio::from(err)) - .spawn(); + .stderr(Stdio::from(err)); + // The boot must not outlive this aid. An aid that is SIGKILLed never gets + // to `cancel`, and its boot then goes on holding devpod's workspace lock + // with nobody left to wait on it. + dl::interrupted_with_this_process(&mut command); + let spawned = command.spawn(); let Ok(child) = spawned else { // The fallback path must not litter: the log was created for a boot // that never started. diff --git a/rust/devlaunch-runner/public-api.txt b/rust/devlaunch-runner/public-api.txt index 46ab7094..5b0915c5 100644 --- a/rust/devlaunch-runner/public-api.txt +++ b/rust/devlaunch-runner/public-api.txt @@ -6,6 +6,7 @@ pub fn devlaunch_runner::interrupt::Registration::fmt(&self, &mut core::fmt::For impl core::ops::drop::Drop for devlaunch_runner::interrupt::Registration pub fn devlaunch_runner::interrupt::Registration::drop(&mut self) pub unsafe fn devlaunch_runner::interrupt::cleanup_and_exit(i32) -> never +pub fn devlaunch_runner::interrupt::ends_with_this_process(&mut std::process::Command, i32) pub fn devlaunch_runner::interrupt::register_dir(&std::path::Path) -> core::option::Option pub fn devlaunch_runner::interrupt::register_file(&std::path::Path) -> core::option::Option pub fn devlaunch_runner::interrupt::register_pid(i32) -> core::option::Option diff --git a/rust/devlaunch-runner/src/interrupt.rs b/rust/devlaunch-runner/src/interrupt.rs index ed02dbfe..8a1b4903 100644 --- a/rust/devlaunch-runner/src/interrupt.rs +++ b/rust/devlaunch-runner/src/interrupt.rs @@ -179,6 +179,61 @@ fn register_in(slots: &'static [AtomicPtr], path: &Path) -> Option None } +/// Have the child `command` spawns take `signal` when this process dies, however +/// it dies. +/// +/// The handler below is what tears a `devpod up` down when `dl` is *told* to stop, +/// and it is not enough on its own. A SIGKILL runs no handler at all, and a +/// `devpod up` that takes the handler's SIGTERM and does not act on it outlives +/// the `_exit` behind it. Either way the child is reparented to init still holding +/// devpod's workspace flock, and every later `dl `, `rm` and `devpod delete` +/// waits on it. That was measured on a host: two such orphans of an `aid +/// --boot-up` sat for four hours, and the kernel log showed no OOM kill. Linux's +/// `PR_SET_PDEATHSIG` is the kernel's own answer, and it does not depend on this +/// process getting to run anything. +/// +/// **The race it leaves, and how it is closed.** The parent can die after the fork +/// and before the `prctl`, and then the child is already init's and no death is +/// left to signal it. So the pid is read here, before the fork, and the child +/// compares its parent with it once the `prctl` is in: a mismatch means the parent +/// is gone, and the child `_exit`s instead of `exec`ing. +/// +/// **The kernel watches the parent *thread*, not the process.** The signal fires +/// when the thread that forked exits, so a child spawned from a short-lived thread +/// would be killed when that thread ends. Every child this is set on is spawned +/// from `main` in both binaries: the launch's `devpod up` and `aid`'s boot child. +/// A caller that spawns from another thread has to keep that thread alive for as +/// long as the child should live. +/// +/// A no-op off Linux, where there is no `PR_SET_PDEATHSIG`: the handler's group +/// kill is all those hosts get. +pub fn ends_with_this_process(command: &mut std::process::Command, signal: i32) { + #[cfg(target_os = "linux")] + { + use std::os::unix::process::CommandExt as _; + // SAFETY: `getpid` reads a value and touches nothing. + let parent = unsafe { libc::getpid() }; + // SAFETY: `prctl`, `getppid` and `_exit` are bare syscalls, which is all a + // pre-exec hook may make: no allocation, no locks. The closure captures two + // integers by value. + unsafe { + command.pre_exec(move || { + if libc::prctl(libc::PR_SET_PDEATHSIG, signal as libc::c_ulong, 0, 0, 0) == -1 { + return Err(std::io::Error::last_os_error()); + } + if libc::getppid() != parent { + libc::_exit(1); + } + Ok(()) + }); + } + } + #[cfg(not(target_os = "linux"))] + { + let _ = (command, signal); + } +} + /// Record the process group of the foreground child now being waited on, so the /// interrupt handler can tear it down. Paired with [`clear_foreground_child`] /// once the child is reaped. diff --git a/rust/devlaunch-runner/src/lib.rs b/rust/devlaunch-runner/src/lib.rs index 49699dcb..55182c45 100644 --- a/rust/devlaunch-runner/src/lib.rs +++ b/rust/devlaunch-runner/src/lib.rs @@ -998,6 +998,12 @@ fn start( Ok(()) }); } + // The child that leads its own group is the `devpod up`, and it is the one + // that holds devpod's workspace flock. SIGKILL rather than SIGTERM: the + // orphans this exists for are `devpod up`s that already had a SIGTERM + // from the interrupt handler and did not stop. See + // [`interrupt::ends_with_this_process`]. + interrupt::ends_with_this_process(&mut command, libc::SIGKILL); } command .spawn() diff --git a/rust/dl/src/lib.rs b/rust/dl/src/lib.rs index f40e2f81..dc46623b 100644 --- a/rust/dl/src/lib.rs +++ b/rust/dl/src/lib.rs @@ -525,6 +525,21 @@ pub fn interrupt(pid: u32) { } } +/// Have the child `command` spawns take a SIGINT when this process dies. +/// +/// For `aid`'s background boot, which is `aid --boot-up` and runs a whole launch: +/// an `aid` that is SIGKILLed, or that dies any other way that runs no handler, +/// used to leave the boot running with nobody to cancel it, and the boot's +/// `devpod up` behind it. SIGINT because it is what `cancel` already sends, so the +/// boot's own handler kills its `devpod up` group and unlinks its staged token, as +/// a cancelled boot does. A SIGKILL here would skip that handler and leave the +/// token file on disk. See +/// [`devlaunch_runner::interrupt::ends_with_this_process`], and its note on +/// threads: `aid` spawns the boot from `main`. +pub fn interrupted_with_this_process(command: &mut std::process::Command) { + devlaunch_runner::interrupt::ends_with_this_process(command, libc::SIGINT); +} + pub use render::ClaudeProfileOffer; /// The Claude logins this host can launch with, for `aid`'s account picker: the From fb8220f48cb1bcf90dca6ef2716a6704079284b3 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 2 Oct 2026 16:10:17 +0100 Subject: [PATCH 02/19] test: a SIGKILLed parent takes its own-group child with it, at the runner and at dl --- rust/devlaunch-runner/tests/parent_death.rs | 101 ++++++++++++++++++++ rust/dl/tests/up_blocked.rs | 59 ++++++++++++ 2 files changed, 160 insertions(+) create mode 100644 rust/devlaunch-runner/tests/parent_death.rs diff --git a/rust/devlaunch-runner/tests/parent_death.rs b/rust/devlaunch-runner/tests/parent_death.rs new file mode 100644 index 00000000..9dd7b3c2 --- /dev/null +++ b/rust/devlaunch-runner/tests/parent_death.rs @@ -0,0 +1,101 @@ +//! A child that leads its own group does not outlive a parent that was SIGKILLed. +//! +//! The child that leads its own group is `devpod up`, and it holds devpod's +//! workspace flock for as long as it lives. A `dl` that is SIGKILLed runs no +//! handler, so before `PR_SET_PDEATHSIG` the `up` was reparented to init and every +//! later `dl `, `rm` and `devpod delete` waited on its flock. Two of them were +//! found on a host after four hours. +//! +//! A SIGKILL cannot be sent to the test process itself, so this binary is +//! re-executed as the parent: with [`ROLE`] set it is the copy that spawns the +//! child and blocks on it, and the test kills that copy. The same shape as +//! `tests/terminal.rs`, for the same coverage reason given there. + +#![cfg(target_os = "linux")] + +use std::path::Path; +use std::process::{Command, Stdio}; +use std::time::{Duration, Instant}; + +use devlaunch_runner::{Invocation, ProcessRunner, Runner, SpawnSpec}; + +/// Set, this copy is the parent and the value is where its child writes its pid. +const ROLE: &str = "DEVLAUNCH_TEST_PARENT_DEATH_PIDFILE"; + +/// The parent's side: one own-group passthrough that would block for a minute. +/// +/// A no-op in an ordinary run, where [`ROLE`] is unset. +#[test] +fn parent_side() { + let Ok(pidfile) = std::env::var(ROLE) else { + return; + }; + let script = format!("echo $$ > {pidfile}; exec sleep 60"); + let spec = SpawnSpec::new(Invocation::new("/bin/sh").with_arg("-c").with_arg(script)) + .leading_its_own_group(); + let _ = ProcessRunner.passthrough(&spec); +} + +fn read_pid(pidfile: &Path) -> Option { + std::fs::read_to_string(pidfile).ok()?.trim().parse().ok() +} + +/// Whether `pid` is a live process. A zombie is not one: it has already died and +/// waits only for its new parent to reap it. +fn alive(pid: i32) -> bool { + match std::fs::read_to_string(format!("/proc/{pid}/stat")) { + Ok(stat) => !stat + .rsplit_once(')') + .is_some_and(|(_, rest)| rest.trim_start().starts_with('Z')), + Err(_) => false, + } +} + +fn wait_for(mut ready: impl FnMut() -> bool) -> bool { + let deadline = Instant::now() + Duration::from_secs(20); + while Instant::now() < deadline { + if ready() { + return true; + } + std::thread::sleep(Duration::from_millis(25)); + } + false +} + +#[test] +fn an_own_group_child_dies_with_a_parent_that_was_sigkilled() { + let scratch = tempfile::tempdir().expect("a scratch directory"); + let pidfile = scratch.path().join("child.pid"); + let mut parent = Command::new(std::env::current_exe().expect("this test binary")) + .args(["--exact", "parent_side", "--nocapture", "--test-threads=1"]) + .env(ROLE, &pidfile) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("the parent copy starts"); + + let started = wait_for(|| read_pid(&pidfile).is_some_and(alive)); + if !started { + let _ = parent.kill(); + let _ = parent.wait(); + } + assert!(started, "the parent never started its child"); + let child = read_pid(&pidfile).expect("the child's pid"); + + parent.kill().expect("SIGKILL the parent"); + parent.wait().expect("reap the parent"); + + let died = wait_for(|| !alive(child)); + if !died { + // Do not leave a minute-long sleep behind a failed test. + // SAFETY: `kill` on a pid this test's own copy started. + unsafe { + libc::kill(child, libc::SIGKILL); + } + } + assert!( + died, + "the own-group child {child} outlived the parent that was SIGKILLed" + ); +} diff --git a/rust/dl/tests/up_blocked.rs b/rust/dl/tests/up_blocked.rs index 76f76234..c79aaceb 100644 --- a/rust/dl/tests/up_blocked.rs +++ b/rust/dl/tests/up_blocked.rs @@ -147,12 +147,17 @@ impl World { .lines() .find(|line| line.starts_with("exec ")) .expect("the delegate exec line"); + // The `up` writes its pid before it blocks, and the `exec` keeps that pid + // for the `sleep`: the process dl spawned is the one that blocks. + let up_pid = root.join("up.pid"); + let up_pid = up_pid.display(); let script = format!( "#!/bin/sh\n\ if [ \"$1\" = \"up\" ]; then\n\ \x20 echo '{BUILD_LINE}' >&2\n\ \x20 echo '{DEVPOD_LINE}'\n\ \x20 echo '{DEVPOD_LINE}'\n\ + \x20 echo $$ > '{up_pid}'\n\ \x20 exec sleep 30\n\ fi\n\ {delegate}\n" @@ -524,6 +529,60 @@ fn a_launch_blocked_behind_an_orphan_clears_it_and_gets_past_the_up() { ); } +/// The orphan itself, prevented at the source: a `dl` that is SIGKILLed while its +/// `devpod up` is blocked takes the `up` with it. +/// +/// A SIGKILL runs no handler, so the interrupt drain that a Ctrl-C reaches never +/// runs, and before `PR_SET_PDEATHSIG` the `up` was reparented to init still +/// holding devpod's flock. That is the holder every test above has to sweep, and +/// two of them were found on a host after four hours behind a dead `aid --boot-up`. +#[cfg(target_os = "linux")] +#[test] +fn a_launch_that_is_sigkilled_mid_up_takes_its_devpod_up_with_it() { + let _serialized = one_at_a_time(); + let world = World::blocked_up(); + let root = world.root.display().to_string(); + let up_pid = world.root.join("up.pid"); + + let mut child = Command::new(env!("CARGO_BIN_EXE_dl")) + .arg("blooop/devlaunch@cold") + .env_clear() + .keeping_coverage() + .env("PATH", format!("{root}/bin:/usr/bin:/bin")) + .env("HOME", format!("{root}/home")) + .env("XDG_CACHE_HOME", format!("{root}/cache")) + .env("XDG_CONFIG_HOME", format!("{root}/config")) + .env("DEVPOD_HOME", format!("{root}/devpod")) + .env("DEVPOD_SHIM_STATE", format!("{root}/shim-state.json")) + .env("DEVPOD_SHIM_LOG", format!("{root}/shim-log.jsonl")) + .env("DEVPOD_SHIM_CONFIG", format!("{root}/shim-config.json")) + .env("GIT_SSH_COMMAND", "false") + .env("GIT_CONFIG_GLOBAL", "/dev/null") + .env("GIT_CONFIG_SYSTEM", "/dev/null") + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("the dl binary runs"); + + let blocked = wait_for(|| read_pid(&up_pid).is_some_and(alive)); + if !blocked { + let _ = child.kill(); + let _ = child.wait(); + } + assert!(blocked, "the launch never reached its blocked up"); + let up = read_pid(&up_pid).expect("the up's pid"); + + child.kill().expect("SIGKILL dl"); + child.wait().expect("reap dl"); + + let gone = wait_for(|| !alive(up)); + if !gone { + let _ = Command::new("kill").args(["-KILL", &up.to_string()]).output(); + } + assert!(gone, "the devpod up {up} outlived the dl that was SIGKILLed"); +} + /// The fixture's own promise, since nothing else checks it: an [`Orphan`] that /// goes out of scope takes its process with it. /// From 2b2615756faafda2b32bce018ab2ba46a1fb697d Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 2 Oct 2026 16:16:22 +0100 Subject: [PATCH 03/19] fix: rm and rme clear an orphan holding devpod's lock instead of naming kill A delete blocked on devpod's workspace lock now runs the sweep a launch runs (kill::release_the_lock): orphans holding the workspace are signalled, a holder somebody is waiting on is spared, and the line says what the sweep found. Both the launch and the delete sweep again every 12 lock lines (about a minute), since a holder spared once can lose its parent later; a repeat sweep speaks only when it signalled something. Tests: the delete sweeps an orphan (core), the lock-wait count, the launch's repeat sweep, and the rm binary test now expects the sweep's finding. --- README.md | 9 +- docs/cli.md | 19 +++- rust/devlaunch-core/public-api.api.txt | 4 +- rust/devlaunch-core/src/clients/devpod.rs | 63 ++++++++++++++ rust/devlaunch-core/src/flows/kill.rs | 12 +++ rust/devlaunch-core/src/flows/launch.rs | 87 +++++++++++++++---- .../src/flows/lifecycle/delete.rs | 39 +++++++-- .../src/flows/lifecycle/tests.rs | 85 +++++++++++++++++- .../tests/api_removal_is_self_sufficient.rs | 2 +- rust/dl/src/commands.rs | 7 +- rust/dl/src/render.rs | 79 ++++++++++------- rust/dl/tests/lifecycle.rs | 29 +++++-- rust/dl/tests/up_blocked.rs | 9 +- 13 files changed, 362 insertions(+), 82 deletions(-) diff --git a/README.md b/README.md index 359743d9..f411e129 100644 --- a/README.md +++ b/README.md @@ -294,10 +294,11 @@ outlived SIGKILL, and it says which. `rm` is the happy path and keeps its guard and its `--force`: use it whenever the workspace might still be wanted, and `kill` when it is stuck and finished with. An `rm` that devpod cannot -get the workspace's lock for now says so while it waits, and names the `kill` that clears it. -So does a launch: `dl `, `up`, `restart`, `recreate`, `reset`, `code` and `dotfiles` all -say the same thing while their `devpod up` sits behind the lock, and add that `kill` deletes -the workspace, so the launch is typed again once it has. +get the workspace's lock for now says so while it waits, then clears every holder that nothing +is waiting on and carries on. So does a launch: `dl `, `up`, `restart`, `recreate`, `reset`, +`code` and `dotfiles` all sweep the lock while their `devpod up` sits behind it. Only a holder +somebody is still waiting on is left, and the line names the `kill` that ends it. See +[docs/cli.md](docs/cli.md) for the details. [docs/cli.md](docs/cli.md) has the rest: what the delete asks of devpod, what stands it down, and why `kill` is the one command in dl with a deadline on it. diff --git a/docs/cli.md b/docs/cli.md index a4ef87c0..c82ba600 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -930,9 +930,17 @@ waits for as long as whatever holds the lock lives. The usual holder is a `devpo up` that outlived the `dl` that started it: reparented to init, sleeping, no children, and nothing on the machine is ever going to reap it. -dl watches for that line. An `rm` behind the lock says so while it waits and names -the `kill` that clears it; the terminal it is printed in is busy holding the command -the advice is about, so the advice names another one on purpose. +On Linux dl makes that orphan hard to create. Each `devpod up` it starts is set to +take a SIGKILL from the kernel when its `dl` dies (`PR_SET_PDEATHSIG`), so a `dl` +that is SIGKILLed, or whose interrupt handler signals an `up` that does not stop, +takes the `up` with it. `aid`'s background boot gets a SIGINT the same way, so an +`aid` that dies cancels its boot as a Ctrl-C would. A holder started some other way, +or on another host, can still wedge the workspace, and the sweep below is for that. + +dl watches for that line. **An `rm`, `rme` or `--rm` behind the lock does not wait +for you either.** It says devpod is waiting, then runs the same sweep a launch runs, +described next, and the delete goes on once the holder lets go. A holder that +somebody is still waiting on is spared, and the line names the `kill` that ends it. **A launch behind the lock does not wait for you.** It says devpod is waiting and that the wait has no deadline, and then it clears the lock itself: the same sweep @@ -942,7 +950,10 @@ restarted and not abandoned. devpod's acquire polls behind that five second line the `up` that was blocked takes the freed flock itself and goes on to build, about a second later, measured. Every verb that brings a workspace up is covered, `dl ` itself, `up`, `restart`, `recreate`, `reset`, `code` and `dotfiles`, because -they all run the same `devpod up`. +they all run the same `devpod up`. The sweep runs on devpod's first lock line and +again once a minute for as long as the wait goes on, because a holder that a live +`dl` was behind can lose that `dl` later. A repeat sweep prints a line only when it +signalled something. Three things it will not do, and they are the reason a launch may do this at all. It never signals a holder somebody is waiting on: a `devpod up` with a live `dl` diff --git a/rust/devlaunch-core/public-api.api.txt b/rust/devlaunch-core/public-api.api.txt index bb4a77bf..988b7e30 100644 --- a/rust/devlaunch-core/public-api.api.txt +++ b/rust/devlaunch-core/public-api.api.txt @@ -12,6 +12,7 @@ pub fn devlaunch_core::flows::launch::ColdRefused::fmt(&self, &mut core::fmt::Fo impl core::marker::StructuralPartialEq for devlaunch_core::flows::launch::ColdRefused pub enum devlaunch_core::api::DeleteStalled pub devlaunch_core::api::DeleteStalled::OnTheLock +pub devlaunch_core::api::DeleteStalled::Swept(devlaunch_core::flows::kill::Released) impl core::clone::Clone for devlaunch_core::flows::lifecycle::DeleteStalled pub fn devlaunch_core::flows::lifecycle::DeleteStalled::clone(&self) -> devlaunch_core::flows::lifecycle::DeleteStalled impl core::cmp::Eq for devlaunch_core::flows::lifecycle::DeleteStalled @@ -19,7 +20,6 @@ impl core::cmp::PartialEq for devlaunch_core::flows::lifecycle::DeleteStalled pub fn devlaunch_core::flows::lifecycle::DeleteStalled::eq(&self, &devlaunch_core::flows::lifecycle::DeleteStalled) -> bool impl core::fmt::Debug for devlaunch_core::flows::lifecycle::DeleteStalled pub fn devlaunch_core::flows::lifecycle::DeleteStalled::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result -impl core::marker::Copy for devlaunch_core::flows::lifecycle::DeleteStalled impl core::marker::StructuralPartialEq for devlaunch_core::flows::lifecycle::DeleteStalled pub enum devlaunch_core::api::Insistence pub devlaunch_core::api::Insistence::Insisted @@ -749,6 +749,7 @@ pub fn devlaunch_core::flows::launch::ToolProvisioning<'_>::remembered_claude(&s pub fn devlaunch_core::flows::launch::ToolProvisioning<'_>::stage_missing_for_this_launch(&self, &str) -> bool pub enum devlaunch_core::flows::lifecycle::DeleteStalled pub devlaunch_core::flows::lifecycle::DeleteStalled::OnTheLock +pub devlaunch_core::flows::lifecycle::DeleteStalled::Swept(devlaunch_core::flows::kill::Released) impl core::clone::Clone for devlaunch_core::flows::lifecycle::DeleteStalled pub fn devlaunch_core::flows::lifecycle::DeleteStalled::clone(&self) -> devlaunch_core::flows::lifecycle::DeleteStalled impl core::cmp::Eq for devlaunch_core::flows::lifecycle::DeleteStalled @@ -756,7 +757,6 @@ impl core::cmp::PartialEq for devlaunch_core::flows::lifecycle::DeleteStalled pub fn devlaunch_core::flows::lifecycle::DeleteStalled::eq(&self, &devlaunch_core::flows::lifecycle::DeleteStalled) -> bool impl core::fmt::Debug for devlaunch_core::flows::lifecycle::DeleteStalled pub fn devlaunch_core::flows::lifecycle::DeleteStalled::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result -impl core::marker::Copy for devlaunch_core::flows::lifecycle::DeleteStalled impl core::marker::StructuralPartialEq for devlaunch_core::flows::lifecycle::DeleteStalled pub enum devlaunch_core::flows::lifecycle::Insistence pub devlaunch_core::flows::lifecycle::Insistence::Insisted diff --git a/rust/devlaunch-core/src/clients/devpod.rs b/rust/devlaunch-core/src/clients/devpod.rs index 2c266588..570d5ae0 100644 --- a/rust/devlaunch-core/src/clients/devpod.rs +++ b/rust/devlaunch-core/src/clients/devpod.rs @@ -279,6 +279,49 @@ pub(crate) fn says_it_is_blocked(line: &str) -> bool { line.contains("Trying to lock workspace") } +/// How many of devpod's lock lines pass between one sweep of the lock and the +/// next: twelve, which is a minute at devpod's five-second timer. +/// +/// A sweep that ran once could find nothing to take and then never look again, +/// while the holder it spared lost its parent a minute later. An attended `devpod +/// up` whose `dl` dies mid-wait is exactly that holder. Not every line, because a +/// sweep reads the process table and spends a grace period whenever it signals. +pub(crate) const SWEEP_AGAIN_EVERY: u32 = 12; + +/// What one of devpod's lines means to a call that is watching for its lock wait. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum LockLine { + /// The first lock line: say the call is blocked, and sweep. + First, + /// Another [`SWEEP_AGAIN_EVERY`] lines have passed: sweep again. + SweepAgain, + /// A lock line between sweeps, or any other line: nothing to do. + Nothing, +} + +/// The count of devpod's lock lines one call has seen, which decides when the +/// call sweeps the lock. One per `devpod up` or `devpod delete`. +#[derive(Debug, Default)] +pub(crate) struct LockWait { + seen: u32, +} + +impl LockWait { + pub(crate) fn read(&mut self, line: &str) -> LockLine { + if !says_it_is_blocked(line) { + return LockLine::Nothing; + } + self.seen = self.seen.saturating_add(1); + if self.seen == 1 { + LockLine::First + } else if (self.seen - 1).is_multiple_of(SWEEP_AGAIN_EVERY) { + LockLine::SweepAgain + } else { + LockLine::Nothing + } + } +} + /// The host path a devcontainer asked for and this machine does not have, if that /// is what this line is about. /// @@ -2725,4 +2768,24 @@ mod tests { StatusUnreadable::NotRun(NotRun::NotInstalled) ); } + + /// The first lock line sweeps, and then every [`SWEEP_AGAIN_EVERY`]th line + /// after it, for as long as devpod goes on waiting. Other lines never count. + #[test] + fn a_lock_wait_sweeps_first_and_then_once_a_minute() { + let lock = "info Trying to lock workspace, seems like another process is running \ + machine_client.go:311"; + let mut wait = LockWait::default(); + assert_eq!(wait.read("info creating devcontainer"), LockLine::Nothing); + assert_eq!(wait.read(lock), LockLine::First); + let every = usize::try_from(SWEEP_AGAIN_EVERY).expect("a small count"); + let mut sweeps = Vec::new(); + for line in 2..=(1 + 2 * every) { + assert_eq!(wait.read("info another build line"), LockLine::Nothing); + if wait.read(lock) == LockLine::SweepAgain { + sweeps.push(line); + } + } + assert_eq!(sweeps, vec![1 + every, 1 + 2 * every]); + } } diff --git a/rust/devlaunch-core/src/flows/kill.rs b/rust/devlaunch-core/src/flows/kill.rs index d0f71bd0..e1672b94 100644 --- a/rust/devlaunch-core/src/flows/kill.rs +++ b/rust/devlaunch-core/src/flows/kill.rs @@ -588,6 +588,18 @@ pub enum Released { Unavailable(HostCannot), } +impl Released { + /// Whether this sweep signalled anything at all. + /// + /// What decides whether a *repeated* sweep is worth a line. The first sweep of + /// a wait is always reported, because its finding is news; a later one that + /// found the same thing would only repeat it once a minute, so it speaks only + /// when it acted. + pub(crate) fn signalled_any(&self) -> bool { + matches!(self, Released::Swept(release) if !release.signalled.is_empty()) + } +} + /// What one launch's sweep found and what it did about it. /// /// [`Sweep`] minus the two halves a launch has no business in. The pair is kept diff --git a/rust/devlaunch-core/src/flows/launch.rs b/rust/devlaunch-core/src/flows/launch.rs index 0d124cb1..1018abe0 100644 --- a/rust/devlaunch-core/src/flows/launch.rs +++ b/rust/devlaunch-core/src/flows/launch.rs @@ -1861,9 +1861,14 @@ fn up_under_stage( // holder letting go, measured on the host in devlaunch#602. Nothing here waits // for that: the sweep returns and the `up` this is watching goes on to build. // - // Once per launch, which is the guard `said` was already keeping: the sweep - // reads the process table and spends the escalation's grace, and devpod's line - // arrives every five seconds for as long as anything is still held. + // Said once per launch, and swept on the first line and then again every + // [`devpod::SWEEP_AGAIN_EVERY`] lines, which is [`devpod::LockWait`]'s count. + // Not on every line: the sweep reads the process table and spends the + // escalation's grace, and devpod's line arrives every five seconds for as + // long as anything is still held. But not only once either: a holder the + // first sweep spared, because a live `dl` was behind it, is an orphan the + // moment that `dl` dies, and a launch that never looks again waits on it + // forever. A later sweep speaks only when it signalled something. // // The runner is lifted out of `context` first because the closure needs it too // — `runner()` hands back a borrow of the command's lifetime rather than of @@ -1877,7 +1882,7 @@ fn up_under_stage( .identity() .unwrap_or(request.source) .to_owned(); - let mut said = false; + let mut lock_wait = devpod::LockWait::default(); let exit = devpod::run_watching( runner, &Call::new(args) @@ -1897,22 +1902,27 @@ fn up_under_stage( notices.say(LaunchNotice::MountSourceEmpty); return; } - if said || !devpod::says_it_is_blocked(line) { - return; + let first = match lock_wait.read(line) { + devpod::LockLine::Nothing => return, + devpod::LockLine::First => true, + devpod::LockLine::SweepAgain => false, + }; + if first { + notices.say(LaunchNotice::UpBlockedOnTheLock { + workspace_id: blocked_on.clone(), + }); } - said = true; - notices.say(LaunchNotice::UpBlockedOnTheLock { - workspace_id: blocked_on.clone(), - }); // The grace period, really spent, as the binary spends it for `kill`: // this call site is core's own, and there is no launch-side clock to // thread through eight parameters for the sake of two seconds that // only elapse when an orphan actually has to be signalled. let released = kill::release_the_lock(runner, &blocked_on, &mut std::thread::sleep); - notices.say(LaunchNotice::SweptTheLockHolders { - workspace_id: blocked_on.clone(), - released, - }); + if first || released.signalled_any() { + notices.say(LaunchNotice::SweptTheLockHolders { + workspace_id: blocked_on.clone(), + released, + }); + } }, )?; // `up` creates and starts workspaces, so any snapshot of `devpod list` taken @@ -6561,10 +6571,16 @@ mod tests { /// One `up` of `myws` that devpod parks on its workspace lock, against a host /// whose process table is `table`. fn up_blocked_against(table: &str) -> Blocked { + up_blocked_for_lines(table, 1) + } + + /// The same, with devpod saying its lock line `lines` times before it gets + /// the lock. + fn up_blocked_for_lines(table: &str, lines: usize) -> Blocked { let scene = Scene::new(); scene.runner.script( ["devpod", "up"], - Response::exited(0).and_stdout(BLOCKED_ON_THE_LOCK), + Response::exited(0).and_stdout(BLOCKED_ON_THE_LOCK.repeat(lines)), ); scene .runner @@ -6705,6 +6721,38 @@ mod tests { ); } + /// A wait that goes on is swept again, once a minute of devpod's lines. A + /// holder the first sweep spared because a live `dl` was behind it becomes an + /// orphan the moment that `dl` dies, and a launch that swept only once would + /// then wait on it for good. The repeat says nothing when it took nothing: the + /// first sweep's finding has already been said. + #[test] + fn an_up_that_stays_blocked_sweeps_again_and_repeats_no_finding() { + let lines = 1 + usize::try_from(devpod::SWEEP_AGAIN_EVERY).expect("a small count"); + let blocked = up_blocked_for_lines( + " 1 0 /sbin/init\n 5000 1 dl myws\n 5001 5000 devpod up myws\n", + lines, + ); + + assert_eq!( + blocked.tables_read, 2, + "the launch swept on the first lock line and once more a minute later", + ); + assert!(blocked.signals.is_empty(), "{:?}", blocked.signals); + assert_eq!( + blocked_notices(&blocked.notices), + 1, + "{:?}", + blocked.notices + ); + assert_eq!( + releases(&blocked.notices).len(), + 1, + "a repeat that took nothing is not said: {:?}", + blocked.notices, + ); + } + /// The launch's own `devpod up` is not something holding the workspace: it is /// the process *waiting* for it. It names the workspace in its own argv and /// its parent is this live `dl`, so the sweep's own reading finds it, calls it @@ -6748,10 +6796,11 @@ mod tests { ); } - /// Once per launch, however long devpod goes on repeating its line. The sweep - /// reads the process table and spends the escalation's grace, so a sweep on - /// devpod's five-second timer would spend the whole of a long wait signalling - /// things it had already signalled. + /// Not on every line devpod repeats. The sweep reads the process table and + /// spends the escalation's grace, so a sweep on devpod's five-second timer + /// would spend the whole of a long wait signalling things it had already + /// signalled. Three lines is well inside one [`devpod::SWEEP_AGAIN_EVERY`], + /// so they get one sweep. #[test] fn an_up_that_stays_blocked_sweeps_once() { let scene = Scene::new(); diff --git a/rust/devlaunch-core/src/flows/lifecycle/delete.rs b/rust/devlaunch-core/src/flows/lifecycle/delete.rs index ffb39120..2663e750 100644 --- a/rust/devlaunch-core/src/flows/lifecycle/delete.rs +++ b/rust/devlaunch-core/src/flows/lifecycle/delete.rs @@ -15,6 +15,7 @@ use crate::clients::docker; use crate::domain::metadata::MetadataStorage; use crate::domain::workspace_state::NonEmpty; use crate::flows::kept_copies::{self, KeptCopies}; +use crate::flows::kill; use crate::flows::listing::CommandContext; use crate::flows::workspace_clone::{RemoveWorkspaceError, Removed, WorkspaceCloneManager}; use crate::notices::Notices; @@ -273,15 +274,32 @@ pub(crate) fn workspace_delete( // names live. Named afterwards, this would find nothing every time and look // like a working cleanup. let named = devcontainer_volumes(devpod_home, workspace_id); - let mut said = false; + // The lock line is answered the way a launch answers it (devlaunch#602): said, + // and then *cleared*, by the same sweep `up` runs. It used to be said and + // nothing more, with advice to run `kill` in another terminal, which left an + // `rm` behind an orphaned `devpod up` waiting until somebody did. The sweep + // takes only orphans, so a holder somebody is still waiting on is spared here + // as it is there, and this delete's own `devpod delete` is this process's + // child and is never one of them. Swept again on [`devpod::LockWait`]'s count, + // for the launch's reason: a holder spared once can lose its parent later. + let runner = context.runner(); + let mut lock_wait = devpod::LockWait::default(); let exit = match devpod::run_watching( - context.runner(), + runner, &delete_call(workspace_id, insistence, persistence), &mut |line| { - if !said && devpod::says_it_is_blocked(line) { - said = true; + let first = match lock_wait.read(line) { + devpod::LockLine::Nothing => return, + devpod::LockLine::First => true, + devpod::LockLine::SweepAgain => false, + }; + if first { stalled(DeleteStalled::OnTheLock); } + let released = kill::release_the_lock(runner, workspace_id, &mut std::thread::sleep); + if first || released.signalled_any() { + stalled(DeleteStalled::Swept(released)); + } }, ) { Ok(exit) => exit, @@ -471,14 +489,19 @@ pub(super) const WEDGED_DELETE: Duration = Duration::from_secs(60); /// so a delete that hits this returns when the holder dies and not before. By the /// time a `Result` could carry the fact, the fact is hours stale. /// -/// Reported once per call however many times devpod says it. The line repeats -/// every five seconds for as long as the holder lives, and advice repeated on that -/// timer buries itself. -#[derive(Clone, Copy, Debug, PartialEq, Eq)] +/// [`OnTheLock`](Self::OnTheLock) is reported once per call however many times +/// devpod says it. The line repeats every five seconds for as long as the holder +/// lives, and a notice repeated on that timer buries itself. +#[derive(Clone, Debug, PartialEq, Eq)] pub enum DeleteStalled { /// Something else holds this workspace's lock, and devpod is waiting on it /// with no deadline. OnTheLock, + /// What this delete's own sweep of that lock came to, which is the launch's + /// [`kill::release_the_lock`] verbatim. Said straight after + /// [`OnTheLock`](Self::OnTheLock), and again only when a later sweep of the + /// same wait signalled something. + Swept(kill::Released), } /// How hard devpod is pushed to let go of the workspace. diff --git a/rust/devlaunch-core/src/flows/lifecycle/tests.rs b/rust/devlaunch-core/src/flows/lifecycle/tests.rs index 1a1fb51c..5f5e8f6b 100644 --- a/rust/devlaunch-core/src/flows/lifecycle/tests.rs +++ b/rust/devlaunch-core/src/flows/lifecycle/tests.rs @@ -61,6 +61,7 @@ use crate::domain::workspace_state::{self, NonEmpty}; use crate::flows::agent_worktrees::{self, Standing, Verdict}; use crate::flows::completion_cache; use crate::flows::kept_copies::KeptCopies; +use crate::flows::kill; use crate::flows::launch_locks::LaunchLocks; use crate::flows::listing::CommandContext; use crate::flows::repo_manager::tests::{refusing_reads, refusing_writes, run_git}; @@ -265,8 +266,12 @@ impl Devpod { /// `docker` is on the list for the reason `devpod` is: a delete spawns it now, /// and a unit test that reached the developer's own docker daemon would be /// removing real volumes named after a fixture. +/// +/// `ps` and `kill` for the same reason: a delete blocked on devpod's lock sweeps +/// the lock now, as a launch does, and a unit test that read the host's process +/// table could signal a real process that happens to name a fixture's workspace. fn faked(program: &str) -> bool { - program == devpod::PROGRAM || program == docker::PROGRAM + [devpod::PROGRAM, docker::PROGRAM, "ps", "kill"].contains(&program) } impl Runner for Devpod { @@ -1900,7 +1905,11 @@ fn a_delete_blocked_on_the_workspace_lock_says_so_while_it_is_blocked() { "myws", Insistence::NotInsisted, Persistence::Ordinary, - &mut |DeleteStalled::OnTheLock| stalls += 1, + &mut |stalled| { + if stalled == DeleteStalled::OnTheLock { + stalls += 1; + } + }, &mut ignoring(), ) .expect("devpod ran"); @@ -1940,7 +1949,11 @@ fn a_delete_blocked_on_the_workspace_lock_sees_the_line_on_stdout() { "myws", Insistence::NotInsisted, Persistence::Ordinary, - &mut |DeleteStalled::OnTheLock| stalls += 1, + &mut |stalled| { + if stalled == DeleteStalled::OnTheLock { + stalls += 1; + } + }, &mut ignoring(), ) .expect("devpod ran"); @@ -1977,7 +1990,11 @@ fn a_delete_that_stays_blocked_says_it_once() { "myws", Insistence::NotInsisted, Persistence::Ordinary, - &mut |DeleteStalled::OnTheLock| stalls += 1, + &mut |stalled| { + if stalled == DeleteStalled::OnTheLock { + stalls += 1; + } + }, &mut ignoring(), ) .expect("devpod ran"); @@ -1985,6 +2002,66 @@ fn a_delete_that_stays_blocked_says_it_once() { assert_eq!(stalls, 1); } +/// A delete parked behind an orphan **clears it**, as a launch does +/// (devlaunch#602). Before this the delete said the right thing and told the +/// reader to run `kill` in another terminal, then waited for as long as the +/// orphan lived, which for an init-reparented `devpod up` is until the machine +/// reboots. Two of them held a host for four hours that way. +#[test] +fn a_delete_blocked_behind_an_orphan_sweeps_it_and_reports_the_sweep() { + let world = a_stopping_world(); + world.devpod.fake.script( + ["devpod", "delete"], + Response::exited(0).and_stdout( + "info Trying to lock workspace, seems like another process is running that \ + blocks this workspace machine_client.go:311\n", + ), + ); + world.devpod.fake.script( + ["ps"], + Response::stdout( + " 1 0 /sbin/init\n732721 1 devpod up myws --ide none\n".to_owned(), + ), + ); + let mut world_cache = World::empty(); + let clones = clones_for(&world_cache.repos_dir, &world_cache.devpod); + let mut context = CommandContext::new(&world.devpod); + let mut refresh = Refresh::new(&world.updater, &world.cache_path); + let mut said = Vec::new(); + + let copies = world_cache.copies(); + workspace_delete( + &mut context, + &mut refresh, + &clones, + &mut world_cache.storage, + None, + &copies, + "myws", + Insistence::NotInsisted, + Persistence::Ordinary, + &mut |stalled| said.push(stalled), + &mut ignoring(), + ) + .expect("devpod ran"); + + assert_eq!( + world.devpod.fake.args_to("kill").first().map(Vec::as_slice), + Some(["-TERM".to_owned(), "732721".to_owned()].as_slice()), + "the delete signalled the orphan holding its workspace", + ); + assert!( + matches!( + said.as_slice(), + [ + DeleteStalled::OnTheLock, + DeleteStalled::Swept(kill::Released::Swept(_)) + ] + ), + "the block is said, then its sweep: {said:?}", + ); +} + /// The deadline firing is a devpod that *ran* — for a minute, and was then /// SIGKILLed by the runner — so it may have got far enough to unlink the /// workspace record before it went. The two lines that answer for that are diff --git a/rust/devlaunch-core/tests/api_removal_is_self_sufficient.rs b/rust/devlaunch-core/tests/api_removal_is_self_sufficient.rs index d55a3501..6dfdfd92 100644 --- a/rust/devlaunch-core/tests/api_removal_is_self_sufficient.rs +++ b/rust/devlaunch-core/tests/api_removal_is_self_sufficient.rs @@ -379,7 +379,7 @@ impl Machine { &KeptCopies::under(&self.cache), WORKSPACE, removal, - &mut |DeleteStalled::OnTheLock| {}, + &mut |_: DeleteStalled| {}, said, ) .expect("devpod ran") diff --git a/rust/dl/src/commands.rs b/rust/dl/src/commands.rs index c2db1b8a..69b49b2a 100644 --- a/rust/dl/src/commands.rs +++ b/rust/dl/src/commands.rs @@ -1161,7 +1161,12 @@ fn remove_addressed<'r>( // Printed from inside the call rather than said through the sink beside it, // because the whole value of the sentence is its timing: the delete it is // about has not returned and, until somebody acts on this, is not going to. - &mut |DeleteStalled::OnTheLock| eprintln!("{}", render::delete_blocked(workspace_id, word)), + &mut |stalled| match stalled { + DeleteStalled::OnTheLock => eprintln!("{}", render::delete_blocked(workspace_id)), + DeleteStalled::Swept(released) => { + eprintln!("{}", render::delete_swept(workspace_id, word, &released)); + } + }, &mut notices, ); match removed { diff --git a/rust/dl/src/render.rs b/rust/dl/src/render.rs index 83db1efe..ef5beab4 100644 --- a/rust/dl/src/render.rs +++ b/rust/dl/src/render.rs @@ -632,21 +632,25 @@ pub(crate) fn devpod_not_run(call: &str, refused: &NotRun) -> String { /// /// Printed while the command is still running, which is the only time it is worth /// anything: this is the one failure with no downstream to report it, because -/// devpod's acquire returns when the holder dies and not before. Somebody watching -/// the five-second line repeat has two choices, wait or intervene, and until now -/// dl said nothing about either. -/// -/// **It names another terminal**, because this one is busy holding the command the -/// advice is about, and Ctrl-C is the alternative it saves people from finding on -/// their own. `word` is the verb that was typed, so a `--rm` firing at the end of a -/// session offers the same `kill` the `rm` verb does rather than a word that is not -/// on the line. -pub(crate) fn delete_blocked(workspace_id: &str, word: &str) -> String { - format!( - "dl: devpod is waiting for another process to let go of {workspace_id}, and it will wait \ - for as long as that takes. In another terminal, 'dl {workspace_id} kill' clears whatever \ - is holding it and deletes it. (This {word} is still waiting.)" - ) +/// devpod's acquire returns when the holder dies and not before. +/// +/// **The launch's own sentence**, because the delete now does what the launch does +/// about it (devlaunch#602): it sweeps the lock, and [`delete_swept`] says how that +/// went. The line used to tell the reader to run `'dl kill'` in another +/// terminal, which is advice to do by hand what dl is about to do unasked, and +/// `kill` was the only way out of an `rm` behind an orphaned `devpod up`. +pub(crate) fn delete_blocked(workspace_id: &str) -> String { + launch_notice(&LaunchNotice::UpBlockedOnTheLock { + workspace_id: workspace_id.to_owned(), + }) + .unwrap_or_default() +} + +/// How a blocked delete's own sweep of devpod's lock ended: the launch's report, +/// [`swept_the_lock`], ending with where *this delete* stands. `word` is the verb +/// that was typed, so a `--rm` at the end of a session is not called an `rm`. +pub(crate) fn delete_swept(workspace_id: &str, word: &str, released: &Released) -> String { + swept_the_lock(workspace_id, released, Waiter::Delete(word)) } /// The delete that was still running when its deadline ran out. @@ -1928,7 +1932,8 @@ pub(crate) fn kill_delete_withheld(workspace_id: &str) -> String { ) } -/// How a blocked launch's own sweep of devpod's lock ended (devlaunch#602). +/// How a blocked launch's or delete's own sweep of devpod's lock ended +/// (devlaunch#602). /// /// One line, because a notice is one line, and the whole of the report `dl /// kill` spreads over several has to fit in it. What survives the compression is @@ -1937,9 +1942,9 @@ pub(crate) fn kill_delete_withheld(workspace_id: &str) -> String { /// something on their behalf, and "cleared 1 process" asks them to go and work /// out what it was, from a process that no longer exists. /// -/// **Every arm ends by saying where the launch now stands**, which is the one +/// **Every arm ends by saying where the [`Waiter`] now stands**, which is the one /// thing the reader cannot see for themselves: the terminal is still sitting in -/// the same `devpod up`, and whether that is about to finish or about to wait +/// the same `devpod up` or `devpod delete`, and whether that is about to finish or about to wait /// forever is exactly what this is for. /// /// The match is over [`Freed`] rather than over the two halves of [`Release`] @@ -1948,28 +1953,32 @@ pub(crate) fn kill_delete_withheld(workspace_id: &str) -> String { /// kill a second said the lock was free and dropped the survivor from the /// report. `Freed` answers both questions at once, and its arms are the four /// sentences. -fn swept_the_lock(workspace_id: &str, released: &Released) -> String { +fn swept_the_lock(workspace_id: &str, released: &Released, waiter: Waiter<'_>) -> String { + let (noun, call) = match waiter { + Waiter::Launch => ("launch", "devpod up"), + Waiter::Delete(word) => (word, "devpod delete"), + }; let release = match released { // The sweep never ran. The reason is `kill`'s own, shared through // `cannot_sweep` so the two verbs cannot describe one broken host - // differently; the frame round it is this launch's, because this launch - // has not failed and is about to go on waiting in its own `up`. + // differently; the frame round it is the waiter's, because it has not + // failed and is about to go on waiting in its own devpod call. Released::Unavailable(cannot) => { return format!( - "dl: {}, so nothing was cleared and this launch is still waiting.", + "dl: {}, so nothing was cleared and this {noun} is still waiting.", cannot_sweep(cannot) ); } Released::Swept(release) => release, }; match release.freed() { - // The good ending, and the one the whole ticket is for. It says the `up` - // carries on rather than telling anybody to launch again, because the - // blocked `up` is still running and takes the flock itself: devpod's + // The good ending, and the one the whole ticket is for. It says the call + // carries on rather than telling anybody to run it again, because the + // blocked call is still running and takes the flock itself: devpod's // acquire is a poll behind that five-second line. Freed::Entirely { cleared } => format!( "dl: cleared what was holding {workspace_id}, and nothing was waiting on it — {}. \ - This launch's own devpod up takes the lock from here.", + This {noun}'s own {call} takes the lock from here.", named(cleared.iter().copied().map(cleared_line)) ), // Half of it. Both halves are said and neither is allowed to imply the @@ -1979,7 +1988,7 @@ fn swept_the_lock(workspace_id: &str, released: &Released) -> String { cleared, still_held, } => format!( - "dl: cleared {} from {workspace_id}, and {}, so this launch is still waiting.{}", + "dl: cleared {} from {workspace_id}, and {}, so this {noun} is still waiting.{}", named(cleared.iter().copied().map(cleared_line)), what_is_left(&still_held), what_is_left_to_do(workspace_id, &still_held), @@ -1989,7 +1998,7 @@ fn swept_the_lock(workspace_id: &str, released: &Released) -> String { // build beside an unstoppable orphan is both findings, and asking // `any_attended` reported it as only the first. Freed::Nothing { still_held } => format!( - "dl: {workspace_id} {}. This launch is still waiting.{}", + "dl: {workspace_id} {}. This {noun} is still waiting.{}", what_is_left(&still_held), what_is_left_to_do(workspace_id, &still_held), ), @@ -1997,11 +2006,21 @@ fn swept_the_lock(workspace_id: &str, released: &Released) -> String { // and the one that says the wait is not an orphan and not dl's to clear. Freed::NothingHeldIt => format!( "dl: nothing on this host is holding {workspace_id}, so whatever devpod is waiting \ - on is out of dl's reach. This launch is still waiting." + on is out of dl's reach. This {noun} is still waiting." ), } } +/// Who is waiting behind the lock a sweep was run for: what the sweep's report +/// ends by saying is still waiting, and which devpod call takes the lock next. +#[derive(Clone, Copy)] +enum Waiter<'a> { + /// A launch, in its `devpod up`. + Launch, + /// A delete, in its `devpod delete`, named by the verb that was typed. + Delete(&'a str), +} + /// What a release left holding the workspace, as one clause. /// /// Total over [`StillHeld`], so the mixed case has a sentence of its own instead @@ -3237,7 +3256,7 @@ pub(crate) fn launch_notice(notice: &LaunchNotice) -> Option { LaunchNotice::SweptTheLockHolders { workspace_id, released, - } => swept_the_lock(workspace_id, released), + } => swept_the_lock(workspace_id, released, Waiter::Launch), // --- the terminal title (no level at all: not a sentence) // diff --git a/rust/dl/tests/lifecycle.rs b/rust/dl/tests/lifecycle.rs index 7d153864..b71ace98 100644 --- a/rust/dl/tests/lifecycle.rs +++ b/rust/dl/tests/lifecycle.rs @@ -1422,9 +1422,13 @@ fn a_kill_names_the_work_it_is_about_to_destroy_and_destroys_it() { /// it waits on with no deadline, logging the same line every five seconds. There /// is no exit to inspect and no timeout on `rm`'s delete, so dl said nothing at /// all and the run had to be Ctrl-C'd. Now the line is read as it arrives and -/// answered while the command is still blocked. +/// answered while the command is still blocked, by the sweep a launch runs. +/// +/// Nothing on this host holds the workspace, so the sweep finds nothing, says so, +/// and names no `kill`: the line used to send the reader to another terminal to do +/// what dl now does itself. #[test] -fn an_rm_devpod_cannot_get_the_lock_for_names_the_kill_that_clears_it() { +fn an_rm_devpod_cannot_get_the_lock_for_sweeps_it_and_says_what_it_found() { let world = World::base(); world.devpod_answers( &["delete"], @@ -1436,7 +1440,7 @@ fn an_rm_devpod_cannot_get_the_lock_for_names_the_kill_that_clears_it() { let run = world.dl(&["devlaunch-main-legacy", "rm"]); // devpod's own line is still forwarded verbatim: reading it must not consume - // it, or the reader loses the evidence the advice is about. + // it, or the reader loses the evidence the notice is about. assert!( run.err.contains("info Trying to lock workspace"), "devpod's line was swallowed: {}", @@ -1445,10 +1449,21 @@ fn an_rm_devpod_cannot_get_the_lock_for_names_the_kill_that_clears_it() { assert!( run.err.contains( "dl: devpod is waiting for another process to let go of devlaunch-main-legacy" - ) && run - .err - .contains("'dl devlaunch-main-legacy kill' clears whatever is holding it"), - "the blocked delete offered no way out: {}", + ), + "the blocked delete said nothing: {}", + run.err + ); + assert!( + run.err.contains( + "dl: nothing on this host is holding devlaunch-main-legacy, so whatever devpod is \ + waiting on is out of dl's reach. This rm is still waiting." + ), + "the blocked delete never reported its sweep: {}", + run.err + ); + assert!( + !run.err.contains("another terminal"), + "dl still tells the reader to run kill by hand: {}", run.err ); } diff --git a/rust/dl/tests/up_blocked.rs b/rust/dl/tests/up_blocked.rs index c79aaceb..8ac14086 100644 --- a/rust/dl/tests/up_blocked.rs +++ b/rust/dl/tests/up_blocked.rs @@ -578,9 +578,14 @@ fn a_launch_that_is_sigkilled_mid_up_takes_its_devpod_up_with_it() { let gone = wait_for(|| !alive(up)); if !gone { - let _ = Command::new("kill").args(["-KILL", &up.to_string()]).output(); + let _ = Command::new("kill") + .args(["-KILL", &up.to_string()]) + .output(); } - assert!(gone, "the devpod up {up} outlived the dl that was SIGKILLed"); + assert!( + gone, + "the devpod up {up} outlived the dl that was SIGKILLed" + ); } /// The fixture's own promise, since nothing else checks it: an [`Orphan`] that From 8363c45f3e03500c7ef7b83567d09ab03e68a31d Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 2 Oct 2026 16:18:17 +0100 Subject: [PATCH 04/19] fix: the interrupt drain SIGKILLs a devpod up group that sits through its SIGTERM The drain sent SIGTERM to the group and _exited at once. A devpod up that ignored it, or a child of it that did, outlived dl holding the workspace flock. Now the drain waits up to two seconds for the leader (waitpid WNOHANG and poll, both async-signal-safe), then SIGKILLs the group. The wait also gives devpod up its unwind before the kernel's parent-death SIGKILL lands. Test: a Ctrl-C mid-up kills an up child that ignores SIGTERM. --- docs/cli.md | 8 +-- rust/devlaunch-runner/src/interrupt.rs | 71 ++++++++++++++++--- rust/dl/tests/up_blocked.rs | 98 ++++++++++++++++++++++++++ 3 files changed, 165 insertions(+), 12 deletions(-) diff --git a/docs/cli.md b/docs/cli.md index c82ba600..1ec434ac 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -388,10 +388,10 @@ removal (a signal handler may not allocate or lock, and this one `_exit`s): What all three *do* run is the cleanup the removal is not: the staged plaintext `GH_TOKEN` file is unlinked and the `devpod up` child is killed, so none of these three -leaves a credential on disk or a build running behind you. The one exception is a run -whose SIGTERM was disarmed before it started. The drain fells the build with a -`killpg(…, SIGTERM)`, so disarming that signal disarms its own reach into the child too. -Ctrl-\ (SIGQUIT) is not one of them and still does mean "die now and dump core", where +leaves a credential on disk or a build running behind you. The drain sends the build's +process group a SIGTERM, gives the `devpod up` up to two seconds to unwind, and then sends +the group a SIGKILL. So a run whose SIGTERM was disarmed before it started, and whose child +inherits that, still loses the build, two seconds later. Ctrl-\ (SIGQUIT) is not one of them and still does mean "die now and dump core", where tidying up first is not what it asks for. The workspace is what stays, still there under its name, and `dl rm` is how it goes. diff --git a/rust/devlaunch-runner/src/interrupt.rs b/rust/devlaunch-runner/src/interrupt.rs index 8a1b4903..d925e920 100644 --- a/rust/devlaunch-runner/src/interrupt.rs +++ b/rust/devlaunch-runner/src/interrupt.rs @@ -15,8 +15,9 @@ //! //! This module is the missing `finally`, expressed as the only things a handler //! is allowed to do about a file and a child: `unlink(2)`/`rmdir(2)` a path, and -//! `killpg(2)`/`kill(2)` a process group or a single detached child. All four are -//! on POSIX's async-signal-safe list. +//! `killpg(2)`/`kill(2)` a process group or a single detached child, with +//! `waitpid(2)` and `poll(2)` to give the group's leader a moment to act on its +//! SIGTERM before the SIGKILL. All of them are on POSIX's async-signal-safe list. //! //! # The registry is lock-free by construction //! @@ -251,9 +252,10 @@ pub(crate) fn clear_foreground_child() { /// /// # Safety /// -/// Async-signal-safe, and only that: it calls `killpg`, `unlink`, `rmdir` and -/// `_exit`, all on POSIX's async-signal-safe list, and reads only lock-free -/// atomics. It must be called **only** from a signal handler (it never +/// Async-signal-safe, and only that: it calls `killpg`, `waitpid`, `poll`, +/// `unlink`, `rmdir` and `_exit`, all on POSIX's async-signal-safe list, and reads +/// only lock-free atomics. It can take up to two seconds, while a foreground +/// child that took its SIGTERM unwinds. It must be called **only** from a signal handler (it never /// returns). Calling it from ordinary code would end the process without /// flushing anything. pub unsafe fn cleanup_and_exit(code: i32) -> ! { @@ -278,9 +280,9 @@ unsafe fn drain() { // `isatty`, `write`, and no allocation. // // Its position among the three is arbitrary and deliberately not argued for: - // the child below is signalled, not waited for, so it can still be writing - // whatever it likes to this terminal after `_exit` either way. Repairing - // after the kill would not close that, and nothing here can. + // the child below is waited for only briefly, and what is left of its group + // can still be writing to this terminal as it dies. Repairing after the kill + // would not close that, and nothing here can. crate::terminal::restore(); // The child next: killing the `devpod up` group before unlinking means the // build is already on its way down by the time the token it was handed is @@ -303,6 +305,24 @@ unsafe fn drain() { unsafe { libc::killpg(pgid, libc::SIGTERM); } + // Then a short wait for the group's leader, and SIGKILL for whatever is + // left. Two reasons, and both are about the `devpod up` that holds the + // workspace's flock. On Linux that child also takes a SIGKILL from the + // kernel the moment this process exits ([`ends_with_this_process`]), so + // an `_exit` straight after the SIGTERM would give devpod no time to + // unwind at all, and an unwind is what takes its busy marker with it. And + // a child that ignores SIGTERM, which is how the orphans this was written + // for came about, is stopped here rather than left for the kernel or for + // nobody, off Linux. The rest of the group goes with it: devpod's own + // children are in it. + // + // SAFETY: `waitpid`, `poll` and `killpg` are async-signal-safe. The wait + // reaps the leader, which is this process's own child, so the pid cannot + // be reused while the group is signalled after it. + unsafe { + wait_for_the_leader(pgid); + libc::killpg(pgid, libc::SIGKILL); + } } // The detached children next, for the same reason the foreground child is // signalled above: `dl` is about to `_exit`, and one of these holds a remote @@ -335,6 +355,41 @@ unsafe fn drain() { } } +/// How long the drain gives the foreground child to act on its SIGTERM before it +/// is SIGKILLed: two seconds, `kill`'s own SIGTERM grace for the same `devpod +/// up`, in steps of [`UNWIND_STEP_MS`]. +const UNWIND_STEPS: u32 = 40; + +/// One step of [`UNWIND_STEPS`], in milliseconds. +const UNWIND_STEP_MS: libc::c_int = 50; + +/// Wait, for [`UNWIND_STEPS`] at most, for the foreground child that leads group +/// `pgid` to exit, and reap it. +/// +/// The leader and not the group, because the leader is the one this process can +/// wait for: a reaped zombie is how its exit is seen here, and a group whose +/// leader is a zombie still answers `killpg(pgid, 0)`. The rest of the group is +/// SIGKILLed after this returns either way. `ECHILD` is the main thread reaping it +/// first, which is the same answer. +/// +/// # Safety +/// +/// Same contract as [`drain`]: `waitpid` and `poll` are async-signal-safe. +unsafe fn wait_for_the_leader(pgid: i32) { + for _ in 0..UNWIND_STEPS { + let mut status: libc::c_int = 0; + // SAFETY: async-signal-safe; see the function contract. + let reaped = unsafe { libc::waitpid(pgid, &mut status, libc::WNOHANG) }; + if reaped != 0 { + return; + } + // SAFETY: a `poll` of no descriptors is a sleep, and async-signal-safe. + unsafe { + libc::poll(ptr::null_mut(), 0, UNWIND_STEP_MS); + } + } +} + /// `SIGTERM` every registered detached child. /// /// Its own function so a test can call it without the rest of [`drain`], which diff --git a/rust/dl/tests/up_blocked.rs b/rust/dl/tests/up_blocked.rs index 8ac14086..76aba37d 100644 --- a/rust/dl/tests/up_blocked.rs +++ b/rust/dl/tests/up_blocked.rs @@ -212,6 +212,39 @@ impl World { } } +impl World { + /// The blocked world, with an `up` that ignores SIGTERM and has a child that + /// ignores it too, the way a `devpod up` and its own children can. The child's + /// pid goes to `up-child.pid`. + fn blocked_up_deaf_to_sigterm() -> Self { + let world = Self::blocked_up(); + let devpod = world.root.join("bin/devpod"); + let original = std::fs::read_to_string(&devpod).expect("the scenario's devpod"); + let delegate = original + .lines() + .find(|line| line.starts_with("exec ")) + .expect("the delegate exec line"); + let child_pid = world.root.join("up-child.pid"); + let child_pid = child_pid.display(); + let script = format!( + "#!/bin/sh\n\ + if [ \"$1\" = \"up\" ]; then\n\ + \x20 trap '' TERM\n\ + \x20 echo '{DEVPOD_LINE}'\n\ + \x20 sleep 30 &\n\ + \x20 echo $! > '{child_pid}'\n\ + \x20 wait\n\ + fi\n\ + {delegate}\n" + ); + std::fs::write(&devpod, script).expect("rewrite devpod"); + use std::os::unix::fs::PermissionsExt as _; + std::fs::set_permissions(&devpod, std::fs::Permissions::from_mode(0o755)) + .expect("keep devpod executable"); + world + } +} + /// A process that looks to `ps` exactly like the orphan devlaunch#602 was opened /// about, owned so that it cannot outlive the test that started it. /// @@ -588,6 +621,71 @@ fn a_launch_that_is_sigkilled_mid_up_takes_its_devpod_up_with_it() { ); } +/// A Ctrl-C does not leave behind a `devpod up` group that ignores SIGTERM. +/// +/// The interrupt drain used to SIGTERM the group and `_exit` at once. A leader +/// that ignored it was reparented to init holding devpod's flock, and so was any +/// child of it that ignored it too. The drain now waits briefly for the leader and +/// then SIGKILLs the group, so the child here goes as well as the leader. +#[test] +fn a_ctrl_c_mid_up_kills_a_group_that_ignores_sigterm() { + let _serialized = one_at_a_time(); + let world = World::blocked_up_deaf_to_sigterm(); + let root = world.root.display().to_string(); + let child_pid = world.root.join("up-child.pid"); + + let mut dl = Command::new(env!("CARGO_BIN_EXE_dl")) + .arg("blooop/devlaunch@cold") + .env_clear() + .keeping_coverage() + .env("PATH", format!("{root}/bin:/usr/bin:/bin")) + .env("HOME", format!("{root}/home")) + .env("XDG_CACHE_HOME", format!("{root}/cache")) + .env("XDG_CONFIG_HOME", format!("{root}/config")) + .env("DEVPOD_HOME", format!("{root}/devpod")) + .env("DEVPOD_SHIM_STATE", format!("{root}/shim-state.json")) + .env("DEVPOD_SHIM_LOG", format!("{root}/shim-log.jsonl")) + .env("DEVPOD_SHIM_CONFIG", format!("{root}/shim-config.json")) + .env("GIT_SSH_COMMAND", "false") + .env("GIT_CONFIG_GLOBAL", "/dev/null") + .env("GIT_CONFIG_SYSTEM", "/dev/null") + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("the dl binary runs"); + + let blocked = wait_for(|| read_pid(&child_pid).is_some_and(alive)); + if !blocked { + let _ = dl.kill(); + let _ = dl.wait(); + } + assert!(blocked, "the launch never reached its blocked up"); + let child = read_pid(&child_pid).expect("the up's child's pid"); + + assert!( + Command::new("kill") + .args(["-INT", &dl.id().to_string()]) + .status() + .expect("kill is installed") + .success(), + "sending SIGINT to dl" + ); + let status = dl.wait().expect("dl exits"); + assert_eq!(status.code(), Some(130), "a Ctrl-C mid-up drains at 130"); + + let gone = wait_for(|| !alive(child)); + if !gone { + let _ = Command::new("kill") + .args(["-KILL", &child.to_string()]) + .output(); + } + assert!( + gone, + "the up's child {child} ignored SIGTERM and outlived the Ctrl-C" + ); +} + /// The fixture's own promise, since nothing else checks it: an [`Orphan`] that /// goes out of scope takes its process with it. /// From 0da526300228a5821205632b2cc10dbcdb350e1b Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 2 Oct 2026 16:18:29 +0100 Subject: [PATCH 05/19] docs: changelog entries for the orphaned devpod up fixes --- CHANGELOG.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ff07bb7..d8174b04 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **A `devpod up` no longer outlives the `dl` or `aid` that started it.** Two `devpod up` + processes were found on a host reparented to init after their `aid --boot-up` parent died, + holding devpod's workspace lock for four hours, and every later `dl `, `rm` and + `devpod delete` waited on them with no end. On Linux each `devpod up` now takes a SIGKILL + from the kernel when its `dl` dies, however it dies (`PR_SET_PDEATHSIG`), and `aid`'s + background boot takes a SIGINT when `aid` dies, so it cancels as a Ctrl-C would. The + interrupt handler also gives the `devpod up` group two seconds to act on its SIGTERM and + then SIGKILLs it, where it used to send the SIGTERM and exit at once. +- **`rm`, `rme` and `--rm` clear an orphan holding devpod's lock, as a launch does.** A + delete blocked on the lock used to print advice to run `dl kill` in another + terminal and then wait. It now runs the launch's sweep: orphans holding the workspace are + signalled, a holder somebody is still waiting on is spared, and the line says what was + found. Both a launch and a delete also sweep again about once a minute while the wait + goes on, because a holder spared once can lose its parent later. + ## [0.59.2] - 2026-10-01 ### Fixed From ce93aebeb4aa6879b67b625a5596f915bed0202a Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 2 Oct 2026 16:22:02 +0100 Subject: [PATCH 06/19] test: a run whose SIGTERM was disarmed now loses its build to a Ctrl-C too --- rust/dl/tests/interrupt.rs | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/rust/dl/tests/interrupt.rs b/rust/dl/tests/interrupt.rs index e5275c37..70cf81ee 100644 --- a/rust/dl/tests/interrupt.rs +++ b/rust/dl/tests/interrupt.rs @@ -308,9 +308,8 @@ enum Ignored { /// The inherited ignore loses: the signal drains anyway, exiting this code. StillDrains(i32), /// The inherited ignore wins: the signal ends nothing, and the run is left for - /// a Ctrl-C to finish — which drains at 130 and reaches the `devpod up` child, - /// unless `up_survives` says the disarming reached the child too. - Honoured { up_survives: bool }, + /// a Ctrl-C to finish — which drains at 130 and reaches the `devpod up` child. + Honoured, } /// The inherited-ignore rule stated as data, one row per signal `dl` handles — @@ -326,14 +325,12 @@ enum Ignored { /// while the set stays put is caught by nothing but review. const INHERITED_IGNORE: [(&str, Ignored); 3] = [ ("INT", Ignored::StillDrains(130)), - // The one row whose child outlives the Ctrl-C, and not because of anything - // `dl` decides: `trap '' TERM` is inherited by everything `dl` spawns, and the - // drain fells the build with a `killpg(…, SIGTERM)`. Disarming SIGTERM for the - // run therefore disarms the drain's own reach into the child — inherent to - // killing a group with the signal the caller switched off, and true of any - // program that tears its children down that way. - ("TERM", Ignored::Honoured { up_survives: true }), - ("HUP", Ignored::Honoured { up_survives: false }), + // `trap '' TERM` is inherited by everything `dl` spawns, so the drain's + // SIGTERM does not reach the build here. This row's child used to outlive the + // Ctrl-C for that reason. The drain now SIGKILLs the group after a short wait, + // so the build goes either way. + ("TERM", Ignored::Honoured), + ("HUP", Ignored::Honoured), ]; #[test] @@ -355,7 +352,7 @@ fn an_inherited_ignore_wins_for_the_two_signals_that_mean_it_and_loses_for_ctrl_ // For the two signals this branch adds, an inherited ignore is a // statement: `nohup dl …` disarms SIGHUP precisely so the run outlives // the terminal, and draining on it would take that away. - Ignored::Honoured { up_survives } => { + Ignored::Honoured => { run.send(signal); assert!( run.survives(Duration::from_millis(500)), @@ -368,7 +365,7 @@ fn an_inherited_ignore_wins_for_the_two_signals_that_mean_it_and_loses_for_ctrl_ Aftermath { code: Some(130), token_left: false, - up_alive: up_survives, + up_alive: false, }, "after SIG{signal} was disarmed the run must still answer Ctrl-C" ); From f7224e7b1e085126b016c3855b9d95e942e167be Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 2 Oct 2026 16:33:05 +0100 Subject: [PATCH 07/19] test: a Ctrl-C mid-up gives devpod its grace to unwind before the SIGKILL Removing the drain's wait for the group leader left every interrupt test green. The new test's up writes a marker 0.5 s into its SIGTERM unwind; it fails with the wait removed. --- rust/dl/tests/up_blocked.rs | 102 ++++++++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/rust/dl/tests/up_blocked.rs b/rust/dl/tests/up_blocked.rs index 76aba37d..fc7bc2a6 100644 --- a/rust/dl/tests/up_blocked.rs +++ b/rust/dl/tests/up_blocked.rs @@ -243,6 +243,40 @@ impl World { .expect("keep devpod executable"); world } + + /// The blocked world, with an `up` that takes half a second to unwind on + /// SIGTERM, the way devpod removes its busy marker before it exits. The + /// marker it writes once that unwind is done is `unwound`, and its pid goes to + /// `up.pid` once the trap is set. + fn blocked_up_slow_to_unwind() -> Self { + let world = Self::blocked_up(); + let devpod = world.root.join("bin/devpod"); + let original = std::fs::read_to_string(&devpod).expect("the scenario's devpod"); + let delegate = original + .lines() + .find(|line| line.starts_with("exec ")) + .expect("the delegate exec line"); + let up_pid = world.root.join("up.pid"); + let up_pid = up_pid.display(); + let unwound = world.root.join("unwound"); + let unwound = unwound.display(); + let script = format!( + "#!/bin/sh\n\ + if [ \"$1\" = \"up\" ]; then\n\ + \x20 trap 'sleep 0.5; echo done > \"{unwound}\"; exit 0' TERM\n\ + \x20 echo '{DEVPOD_LINE}'\n\ + \x20 echo $$ > '{up_pid}'\n\ + \x20 sleep 30 &\n\ + \x20 wait\n\ + fi\n\ + {delegate}\n" + ); + std::fs::write(&devpod, script).expect("rewrite devpod"); + use std::os::unix::fs::PermissionsExt as _; + std::fs::set_permissions(&devpod, std::fs::Permissions::from_mode(0o755)) + .expect("keep devpod executable"); + world + } } /// A process that looks to `ps` exactly like the orphan devlaunch#602 was opened @@ -686,6 +720,74 @@ fn a_ctrl_c_mid_up_kills_a_group_that_ignores_sigterm() { ); } +/// A Ctrl-C mid-up gives the `devpod up` time to finish unwinding before the +/// SIGKILL. +/// +/// devpod removes its busy marker as it unwinds from SIGTERM. A drain that +/// SIGKILLed the group straight after the SIGTERM, or `_exit`ed into the +/// kernel's parent-death SIGKILL, would cut that unwind off and leave the marker +/// behind for the next launch to trip on. +#[test] +fn a_ctrl_c_mid_up_lets_the_up_finish_unwinding_from_sigterm() { + let _serialized = one_at_a_time(); + let world = World::blocked_up_slow_to_unwind(); + let root = world.root.display().to_string(); + let up_pid = world.root.join("up.pid"); + let unwound = world.root.join("unwound"); + + let mut dl = Command::new(env!("CARGO_BIN_EXE_dl")) + .arg("blooop/devlaunch@cold") + .env_clear() + .keeping_coverage() + .env("PATH", format!("{root}/bin:/usr/bin:/bin")) + .env("HOME", format!("{root}/home")) + .env("XDG_CACHE_HOME", format!("{root}/cache")) + .env("XDG_CONFIG_HOME", format!("{root}/config")) + .env("DEVPOD_HOME", format!("{root}/devpod")) + .env("DEVPOD_SHIM_STATE", format!("{root}/shim-state.json")) + .env("DEVPOD_SHIM_LOG", format!("{root}/shim-log.jsonl")) + .env("DEVPOD_SHIM_CONFIG", format!("{root}/shim-config.json")) + .env("GIT_SSH_COMMAND", "false") + .env("GIT_CONFIG_GLOBAL", "/dev/null") + .env("GIT_CONFIG_SYSTEM", "/dev/null") + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("the dl binary runs"); + + let blocked = wait_for(|| read_pid(&up_pid).is_some_and(alive)); + if !blocked { + let _ = dl.kill(); + let _ = dl.wait(); + } + assert!(blocked, "the launch never reached its blocked up"); + let up = read_pid(&up_pid).expect("the up's pid"); + + assert!( + Command::new("kill") + .args(["-INT", &dl.id().to_string()]) + .status() + .expect("kill is installed") + .success(), + "sending SIGINT to dl" + ); + let status = dl.wait().expect("dl exits"); + assert_eq!(status.code(), Some(130), "a Ctrl-C mid-up drains at 130"); + + let finished = wait_for(|| unwound.exists() || !alive(up)); + if alive(up) { + let _ = Command::new("kill") + .args(["-KILL", &up.to_string()]) + .output(); + } + assert!(finished, "the up {up} was still running after the Ctrl-C"); + assert!( + unwound.exists(), + "the up was killed before it finished unwinding from SIGTERM" + ); +} + /// The fixture's own promise, since nothing else checks it: an [`Orphan`] that /// goes out of scope takes its process with it. /// From c203d31b4726eb841297e00e7dd9b9234e09be59 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 2 Oct 2026 16:36:49 +0100 Subject: [PATCH 08/19] test: a SIGKILLed aid takes its boot down and the boot unlinks its token Removing the boot's parent-death SIGINT left every aid test green. aid runs under a shell that leads the pty's session and outlives it, so the kernel's hangup cannot stand in for the parent-death signal. The test fails with the ends_with_this_process call removed. --- rust/aid/tests/interactive.rs | 130 +++++++++++++++++++++++++++++++++- 1 file changed, 129 insertions(+), 1 deletion(-) diff --git a/rust/aid/tests/interactive.rs b/rust/aid/tests/interactive.rs index 81509362..826fabd6 100644 --- a/rust/aid/tests/interactive.rs +++ b/rust/aid/tests/interactive.rs @@ -102,6 +102,29 @@ struct PtyAid { impl PtyAid { fn spawn(world: &World, args: &[&str], extra: &[(&str, &str)]) -> Self { + Self::spawn_behind(world, &[env!("CARGO_BIN_EXE_aid")], args, extra) + } + + /// `aid` as a child of a shell that leads the pty's session and stays for a + /// minute after aid ends. Neither aid's death nor the shell's is then a + /// session leader's exit within that minute, so the kernel sends the + /// foreground group no SIGHUP, and only what aid itself arranged reaches its + /// children. + fn spawn_under_a_shell(world: &World, args: &[&str], extra: &[(&str, &str)]) -> Self { + Self::spawn_behind( + world, + &[ + "sh", + "-c", + "\"$0\" \"$@\"; exec sleep 60", + env!("CARGO_BIN_EXE_aid"), + ], + args, + extra, + ) + } + + fn spawn_behind(world: &World, lead: &[&str], args: &[&str], extra: &[(&str, &str)]) -> Self { let pty = native_pty_system() .openpty(PtySize { rows: 24, @@ -111,7 +134,8 @@ impl PtyAid { }) .expect("a pty"); let root = world.root.display().to_string(); - let mut command = CommandBuilder::new(env!("CARGO_BIN_EXE_aid")); + let mut command = CommandBuilder::new(lead[0]); + command.args(&lead[1..]); command.args(args); command.env_clear(); // `KeepingCoverage` by hand: the trait extends `std::process::Command`, @@ -761,6 +785,110 @@ fn a_ctrl_c_at_the_editor_tears_the_whole_boot_down() { ); } +/// An aid that is SIGKILLed at the editor takes its boot with it, and the boot +/// still unlinks its staged token. +/// +/// A SIGKILL runs no handler, so aid never reaches the `cancel` a Ctrl-C does. +/// The kernel's parent-death signal is what reaches the boot instead, and it is a +/// SIGINT so the boot's own handler runs: it kills the `devpod up` and unlinks the +/// token file, as the Ctrl-C above has it do. +#[cfg(target_os = "linux")] +#[test] +fn a_sigkilled_aid_takes_its_boot_down_and_the_token_with_it() { + let world = World::with(&["--gh"]); + let devpod = world.root.join("bin/devpod"); + let original = std::fs::read_to_string(&devpod).expect("the scenario's devpod"); + let delegate = original + .lines() + .find(|line| line.starts_with("exec ")) + .expect("the delegate exec line"); + let script = format!( + "#!/bin/sh\n\ + if [ \"$1\" = \"up\" ]; then\n\ + \x20 echo \"$$\" > \"$DL_UP_PID\"\n\ + \x20 : > \"$DL_UP_STARTED\"\n\ + \x20 exec sleep 120\n\ + fi\n\ + {delegate}\n" + ); + std::fs::write(&devpod, script).expect("rewrite devpod"); + use std::os::unix::fs::PermissionsExt as _; + std::fs::set_permissions(&devpod, std::fs::Permissions::from_mode(0o755)) + .expect("keep devpod executable"); + let tmpdir = world.root.join("tmp"); + std::fs::create_dir_all(&tmpdir).expect("a scratch TMPDIR"); + let up_pid = world.root.join("up.pid"); + let up_started = world.root.join("up.started"); + + let session = PtyAid::spawn_under_a_shell( + &world, + &["blooop/devlaunch@cold"], + &[ + ("TMPDIR", &tmpdir.display().to_string()), + ("DL_UP_PID", &up_pid.display().to_string()), + ("DL_UP_STARTED", &up_started.display().to_string()), + ], + ); + session.reach_the_editor(); + assert!( + wait_for(|| up_started.exists() && token_file(&tmpdir).is_some()), + "devpod up never blocked with a token staged" + ); + let up = std::fs::read_to_string(&up_pid).expect("the up pid"); + let up = up.trim().to_owned(); + let only_child = |parent: &str| { + let children = Command::new("ps") + .args(["-o", "pid=", "--ppid", parent]) + .output() + .expect("ps is installed"); + String::from_utf8_lossy(&children.stdout) + .split_whitespace() + .next() + .expect("a child") + .to_owned() + }; + let shell = session + .child + .process_id() + .expect("the shell's pid") + .to_string(); + let aid = only_child(&shell); + let boot = only_child(&aid); + let alive = |pid: &str| { + Command::new("kill") + .args(["-0", pid]) + .output() + .expect("kill is installed") + .status + .success() + }; + + assert!( + Command::new("kill") + .args(["-KILL", &aid]) + .status() + .expect("kill is installed") + .success(), + "sending SIGKILL to aid" + ); + let aid_gone = wait_for(|| !alive(&aid)); + + let boot_gone = wait_for(|| !alive(&boot)); + let token_gone = wait_for(|| token_file(&tmpdir).is_none()); + let up_gone = wait_for(|| !alive(&up)); + for pid in [&boot, &up, &shell] { + let _ = Command::new("kill").args(["-KILL", pid]).output(); + } + let _ = session.wait(); + assert!(aid_gone, "aid (pid {aid}) outlived its SIGKILL"); + assert!( + boot_gone, + "the boot (pid {boot}) outlived the SIGKILLed aid" + ); + assert!(token_gone, "the boot left its token file behind"); + assert!(up_gone, "the boot left its devpod up (pid {up}) behind"); +} + /// The one staged GitHub-token file under `dir`, if any. fn token_file(dir: &Path) -> Option { std::fs::read_dir(dir).ok()?.flatten().find_map(|entry| { From de3f0937e859f289fbc7076899bfc84665a2204d Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 2 Oct 2026 16:37:11 +0100 Subject: [PATCH 09/19] docs: a blocked rm sweeps the lock rather than naming kill once docs/cli.md still said a delete behind devpod's lock answered the first lock line by naming dl kill, and said it once. It now prints the launch's notice and sweeps on the first line and every twelfth after, and kill's own delete does the same. --- docs/cli.md | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/docs/cli.md b/docs/cli.md index 1ec434ac..44a7e123 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -1075,10 +1075,13 @@ on screen above it. A `dl rm` that devpod cannot get the lock for is the harder half, because it never refuses: devpod waits on that lock with no deadline, logging the five second line at the top of this section for as long as the holder lives, so there is no -exit code for anything downstream to read. dl reads devpod's stderr as it arrives -instead, and answers the first of those lines while the command is still blocked, -naming the `dl kill` to run in another terminal. It says it once, however many -times devpod says it. +exit code for anything downstream to read. dl reads devpod's output as it arrives +instead, and answers the first of those lines while the command is still blocked: +it prints the launch's notice, the one that ends "Looking for what is holding +it...", and runs the sweep described above. It sweeps again on every twelfth line +after that, about once a minute, and prints a repeat sweep only when it signalled +something. `dl kill` ends in this same delete, so a holder that arrives after +its own sweep is swept here too. ## When devpod is missing or will not answer From fcb69d8ef5714e1b9912632206b0c06bd866efd9 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 2 Oct 2026 16:34:17 +0100 Subject: [PATCH 10/19] test: a delete that stays blocked on the lock sweeps it again No delete test sent more than one sweep's worth of lock lines, so a delete that swept once and never again passed the suite. This one sends 1 + SWEEP_AGAIN_EVERY lines and counts the process-table reads. --- .../src/flows/lifecycle/tests.rs | 139 ++++++++++++++++++ 1 file changed, 139 insertions(+) diff --git a/rust/devlaunch-core/src/flows/lifecycle/tests.rs b/rust/devlaunch-core/src/flows/lifecycle/tests.rs index 5f5e8f6b..2a8a0e38 100644 --- a/rust/devlaunch-core/src/flows/lifecycle/tests.rs +++ b/rust/devlaunch-core/src/flows/lifecycle/tests.rs @@ -2062,6 +2062,145 @@ fn a_delete_blocked_behind_an_orphan_sweeps_it_and_reports_the_sweep() { ); } +/// A runner that answers `ps` with `tables` in turn, the last one for every read +/// after it, and hands every other spawn to `rest`. For a sweep whose second +/// reading of the host is not its first. +struct TablesInTurn<'a> { + rest: &'a dyn Runner, + tables: Vec, + read: std::sync::Mutex, +} + +impl<'a> TablesInTurn<'a> { + fn new(rest: &'a dyn Runner, tables: &[&str]) -> Self { + assert!(!tables.is_empty(), "at least one table to answer with"); + Self { + rest, + tables: tables + .iter() + .map(|table| FakeRunner::new().with_script(["ps"], Response::stdout(*table))) + .collect(), + read: std::sync::Mutex::new(0), + } + } + + fn tables_read(&self) -> usize { + *self + .read + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + } +} + +impl Runner for TablesInTurn<'_> { + fn capture(&self, spec: &SpawnSpec) -> Outcome { + if spec.invocation.program != "ps" { + return self.rest.capture(spec); + } + let mut read = self + .read + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let table = &self.tables[(*read).min(self.tables.len() - 1)]; + *read += 1; + table.capture(spec) + } + + fn passthrough(&self, spec: &SpawnSpec) -> Outcome { + self.rest.passthrough(spec) + } + + fn session(&self, spec: &SpawnSpec, on_stderr_line: &mut dyn FnMut(&str)) -> Outcome { + self.rest.session(spec, on_stderr_line) + } + + fn watched(&self, spec: &SpawnSpec, on_line: &mut dyn FnMut(&str)) -> Outcome { + self.rest.watched(spec, on_line) + } + + fn detach(&self, what: &RawInvocation) -> DetachOutcome { + self.rest.detach(what) + } +} + +/// What one blocked delete did, beyond deleting. +struct DeleteBlocked { + said: Vec, + /// Every `kill` the delete ran, in order. + signals: Vec>, + /// How many times it read the host's process table. + tables_read: usize, +} + +/// One delete of `myws` that devpod parks on its workspace lock for `lines` of +/// its lock line, against a host whose process table reads `tables` in turn. +fn delete_blocked_for_lines(tables: &[&str], lines: usize) -> DeleteBlocked { + let world = a_stopping_world(); + world.devpod.fake.script( + ["devpod", "delete"], + Response::exited(0).and_stdout( + "info Trying to lock workspace, seems like another process is running that \ + blocks this workspace machine_client.go:311\n" + .repeat(lines), + ), + ); + let runner = TablesInTurn::new(&world.devpod, tables); + let mut world_cache = World::empty(); + let clones = clones_for(&world_cache.repos_dir, &world_cache.devpod); + let mut context = CommandContext::new(&runner); + let mut refresh = Refresh::new(&world.updater, &world.cache_path); + let mut said = Vec::new(); + + let copies = world_cache.copies(); + workspace_delete( + &mut context, + &mut refresh, + &clones, + &mut world_cache.storage, + None, + &copies, + "myws", + Insistence::NotInsisted, + Persistence::Ordinary, + &mut |stalled| said.push(stalled), + &mut ignoring(), + ) + .expect("devpod ran"); + + DeleteBlocked { + said, + signals: world.devpod.fake.args_to("kill"), + tables_read: runner.tables_read(), + } +} + +/// Somebody else's live `dl`, building `myws` through its own `devpod up`. +const A_LIVE_BUILD_HOLDING_MYWS: &str = + " 1 0 /sbin/init\n 5000 1 dl myws\n 5001 5000 devpod up myws\n"; + +/// A wait that goes on is swept again, once a minute of devpod's lines, as a +/// launch's is. The repeat says nothing when it took nothing. +#[test] +fn a_delete_that_stays_blocked_sweeps_again_and_repeats_no_finding() { + let lines = 1 + usize::try_from(devpod::SWEEP_AGAIN_EVERY).expect("a small count"); + + let blocked = delete_blocked_for_lines(&[A_LIVE_BUILD_HOLDING_MYWS], lines); + + assert_eq!( + blocked.tables_read, 2, + "the delete swept on the first lock line and once more a minute later", + ); + assert!(blocked.signals.is_empty(), "{:?}", blocked.signals); + assert!( + matches!( + blocked.said.as_slice(), + [DeleteStalled::OnTheLock, DeleteStalled::Swept(_)] + ), + "a repeat that took nothing is not said: {:?}", + blocked.said, + ); +} + /// The deadline firing is a devpod that *ran* — for a minute, and was then /// SIGKILLed by the runner — so it may have got far enough to unlink the /// workspace record before it went. The two lines that answer for that are From b88eacf4c44967cec6ba787cc122f1a709b8a579 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 2 Oct 2026 16:36:37 +0100 Subject: [PATCH 11/19] test: a later sweep that takes a once-spared holder is reported The first sweep spares a holder that a live dl is behind, and a later sweep takes it once that dl is gone. Nothing tested that the later sweep is said, so a gate reduced to `if first` passed in both the launch and the delete. Each now reads a different process table per sweep. --- rust/devlaunch-core/src/flows/launch.rs | 104 +++++++++++++++++- .../src/flows/lifecycle/tests.rs | 40 +++++++ 2 files changed, 139 insertions(+), 5 deletions(-) diff --git a/rust/devlaunch-core/src/flows/launch.rs b/rust/devlaunch-core/src/flows/launch.rs index 1018abe0..e2251e7e 100644 --- a/rust/devlaunch-core/src/flows/launch.rs +++ b/rust/devlaunch-core/src/flows/launch.rs @@ -6577,15 +6577,18 @@ mod tests { /// The same, with devpod saying its lock line `lines` times before it gets /// the lock. fn up_blocked_for_lines(table: &str, lines: usize) -> Blocked { + up_blocked_through(&[table], lines) + } + + /// The same, against a host whose process table reads `tables` in turn. + fn up_blocked_through(tables: &[&str], lines: usize) -> Blocked { let scene = Scene::new(); scene.runner.script( ["devpod", "up"], Response::exited(0).and_stdout(BLOCKED_ON_THE_LOCK.repeat(lines)), ); - scene - .runner - .script(["ps"], Response::stdout(table.to_owned())); - let mut context = CommandContext::new(&scene.runner); + let runner = TablesInTurn::new(&scene.runner, tables); + let mut context = CommandContext::new(&runner); let token = HostToken::new(); let request = UpRequest::new( "owner/repo", @@ -6610,7 +6613,61 @@ mod tests { Blocked { notices, signals: scene.runner.args_to("kill"), - tables_read: scene.runner.calls_to("ps").len(), + tables_read: runner.tables_read(), + } + } + + /// A runner that answers `ps` with `tables` in turn, the last one for every + /// read after it, and hands every other spawn to `rest`. + struct TablesInTurn<'a> { + rest: &'a FakeRunner, + tables: Vec, + read: Mutex, + } + + impl<'a> TablesInTurn<'a> { + fn new(rest: &'a FakeRunner, tables: &[&str]) -> Self { + assert!(!tables.is_empty(), "at least one table to answer with"); + Self { + rest, + tables: tables + .iter() + .map(|table| FakeRunner::new().with_script(["ps"], Response::stdout(*table))) + .collect(), + read: Mutex::new(0), + } + } + + fn tables_read(&self) -> usize { + *self.read.lock().unwrap_or_else(PoisonError::into_inner) + } + } + + impl Runner for TablesInTurn<'_> { + fn capture(&self, spec: &SpawnSpec) -> Outcome { + if spec.program() != "ps" { + return self.rest.capture(spec); + } + let mut read = self.read.lock().unwrap_or_else(PoisonError::into_inner); + let table = &self.tables[(*read).min(self.tables.len() - 1)]; + *read += 1; + table.capture(spec) + } + + fn passthrough(&self, spec: &SpawnSpec) -> Outcome { + self.rest.passthrough(spec) + } + + fn session(&self, spec: &SpawnSpec, on_stderr_line: &mut dyn FnMut(&str)) -> Outcome { + self.rest.session(spec, on_stderr_line) + } + + fn watched(&self, spec: &SpawnSpec, on_line: &mut dyn FnMut(&str)) -> Outcome { + self.rest.watched(spec, on_line) + } + + fn detach(&self, what: &Invocation) -> DetachOutcome { + self.rest.detach(what) } } @@ -6753,6 +6810,43 @@ mod tests { ); } + /// A holder the first sweep spared, because a live `dl` was behind it, is an + /// orphan once that `dl` dies. The later sweep that takes it is said, because + /// this time the sweep did something. + #[test] + fn an_up_whose_spared_holder_is_orphaned_later_reports_the_later_sweep() { + let lines = 1 + usize::try_from(devpod::SWEEP_AGAIN_EVERY).expect("a small count"); + + let blocked = up_blocked_through( + &[ + " 1 0 /sbin/init\n 5000 1 dl myws\n 5001 5000 devpod up myws\n", + " 1 0 /sbin/init\n 5001 1 devpod up myws\n", + " 1 0 /sbin/init\n", + ], + lines, + ); + + assert_eq!( + blocked.signals.first().map(Vec::as_slice), + Some(["-TERM".to_owned(), "5001".to_owned()].as_slice()), + "the second sweep signalled the holder its `dl` left behind: {:?}", + blocked.signals, + ); + let reported = releases(&blocked.notices); + let [kill::Released::Swept(spared), kill::Released::Swept(taken)] = reported.as_slice() + else { + panic!( + "the first sweep, then the sweep that took something: {:?}", + blocked.notices + ); + }; + assert!(spared.signalled.is_empty(), "{spared:?}"); + assert!( + matches!(taken.freed(), kill::Freed::Entirely { .. }), + "{taken:?}" + ); + } + /// The launch's own `devpod up` is not something holding the workspace: it is /// the process *waiting* for it. It names the workspace in its own argv and /// its parent is this live `dl`, so the sweep's own reading finds it, calls it diff --git a/rust/devlaunch-core/src/flows/lifecycle/tests.rs b/rust/devlaunch-core/src/flows/lifecycle/tests.rs index 2a8a0e38..235d725b 100644 --- a/rust/devlaunch-core/src/flows/lifecycle/tests.rs +++ b/rust/devlaunch-core/src/flows/lifecycle/tests.rs @@ -2201,6 +2201,46 @@ fn a_delete_that_stays_blocked_sweeps_again_and_repeats_no_finding() { ); } +/// A holder the first sweep spared, because a live `dl` was behind it, is an +/// orphan once that `dl` dies. The later sweep that takes it is said, because +/// this time the sweep did something. +#[test] +fn a_delete_whose_spared_holder_is_orphaned_later_reports_the_later_sweep() { + let lines = 1 + usize::try_from(devpod::SWEEP_AGAIN_EVERY).expect("a small count"); + + let blocked = delete_blocked_for_lines( + &[ + A_LIVE_BUILD_HOLDING_MYWS, + " 1 0 /sbin/init\n 5001 1 devpod up myws\n", + " 1 0 /sbin/init\n", + ], + lines, + ); + + assert_eq!( + blocked.signals.first().map(Vec::as_slice), + Some(["-TERM".to_owned(), "5001".to_owned()].as_slice()), + "the second sweep signalled the holder its `dl` left behind: {:?}", + blocked.signals, + ); + let [ + DeleteStalled::OnTheLock, + DeleteStalled::Swept(kill::Released::Swept(spared)), + DeleteStalled::Swept(kill::Released::Swept(taken)), + ] = blocked.said.as_slice() + else { + panic!( + "the block, its first sweep, then the sweep that took something: {:?}", + blocked.said + ); + }; + assert!(spared.signalled.is_empty(), "{spared:?}"); + assert!( + matches!(taken.freed(), kill::Freed::Entirely { .. }), + "{taken:?}" + ); +} + /// The deadline firing is a devpod that *ran* — for a minute, and was then /// SIGKILLed by the runner — so it may have got far enough to unlink the /// workspace record before it went. The two lines that answer for that are From 48c9a6ab1f83d05fa490c51d2aefc9ea608a0cea Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 2 Oct 2026 16:38:02 +0100 Subject: [PATCH 12/19] test: a delete blocked behind a live build signals nothing Every delete test fed the sweep an empty table or an orphan, so a sweep that signalled attended holders too passed the delete suite. This one puts a devpod up with a live dl behind it on the lock. --- .../src/flows/lifecycle/tests.rs | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/rust/devlaunch-core/src/flows/lifecycle/tests.rs b/rust/devlaunch-core/src/flows/lifecycle/tests.rs index 235d725b..5d894d89 100644 --- a/rust/devlaunch-core/src/flows/lifecycle/tests.rs +++ b/rust/devlaunch-core/src/flows/lifecycle/tests.rs @@ -2241,6 +2241,36 @@ fn a_delete_whose_spared_holder_is_orphaned_later_reports_the_later_sweep() { ); } +/// Somebody else's `devpod up`, with a live `dl` behind it, is a build in flight. +/// A delete that wants its lock waits for it, as a launch does, and says the +/// sweep found it and signalled nothing. +#[test] +fn a_delete_blocked_behind_somebody_elses_live_build_signals_nothing() { + let blocked = delete_blocked_for_lines(&[A_LIVE_BUILD_HOLDING_MYWS], 1); + + assert!( + blocked.signals.is_empty(), + "a live build must not be signalled by a delete that wants its lock: {:?}", + blocked.signals, + ); + assert_eq!(blocked.tables_read, 1, "the sweep ran and spared the build"); + let [ + DeleteStalled::OnTheLock, + DeleteStalled::Swept(kill::Released::Swept(release)), + ] = blocked.said.as_slice() + else { + panic!("the block, then its sweep: {:?}", blocked.said); + }; + assert!( + matches!(release.freed(), kill::Freed::Nothing { .. }), + "{release:?}" + ); + assert!( + release.holding.any_attended(), + "the build is reported as the live holder it is: {release:?}", + ); +} + /// The deadline firing is a devpod that *ran* — for a minute, and was then /// SIGKILLed by the runner — so it may have got far enough to unlink the /// workspace record before it went. The two lines that answer for that are From 6de25f942c97578a635e0b80664b6762375bd392 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 2 Oct 2026 16:42:35 +0100 Subject: [PATCH 13/19] release: 0.59.3 All four copies of the version move together: Cargo.toml, Cargo.lock, and the README conda badge and dl --version transcript. The bump rides on #668 so the merge that lands the fix is the one that publishes it. --- CHANGELOG.md | 6 ++++-- README.md | 4 ++-- rust/Cargo.lock | 10 +++++----- rust/Cargo.toml | 2 +- 4 files changed, 12 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d8174b04..c8da2525 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,9 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.59.3] - 2026-10-02 + ### Fixed -- **A `devpod up` no longer outlives the `dl` or `aid` that started it.** Two `devpod up` +- **A `devpod up` no longer outlives the `dl` or `aid` that started it** (#668). Two `devpod up` processes were found on a host reparented to init after their `aid --boot-up` parent died, holding devpod's workspace lock for four hours, and every later `dl `, `rm` and `devpod delete` waited on them with no end. On Linux each `devpod up` now takes a SIGKILL @@ -17,7 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 background boot takes a SIGINT when `aid` dies, so it cancels as a Ctrl-C would. The interrupt handler also gives the `devpod up` group two seconds to act on its SIGTERM and then SIGKILLs it, where it used to send the SIGTERM and exit at once. -- **`rm`, `rme` and `--rm` clear an orphan holding devpod's lock, as a launch does.** A +- **`rm`, `rme` and `--rm` clear an orphan holding devpod's lock, as a launch does** (#668). A delete blocked on the lock used to print advice to run `dl kill` in another terminal and then wait. It now runs the launch's sweep: orphans holding the workspace are signalled, a holder somebody is still waiting on is spared, and the line says what was diff --git a/README.md b/README.md index f411e129..6ac712f1 100644 --- a/README.md +++ b/README.md @@ -19,7 +19,7 @@ one argument instead of a clone, a config file and a build command. [![GitHub pull-requests merged](https://badgen.net/github/merged-prs/blooop/devlaunch)](https://github.com/blooop/devlaunch/pulls?q=is%3Amerged) [![GitHub release](https://img.shields.io/github/release/blooop/devlaunch.svg)](https://GitHub.com/blooop/devlaunch/releases/) [![PyPI](https://img.shields.io/pypi/v/devlaunch)](https://pypi.org/project/devlaunch/) -[![Conda](https://img.shields.io/badge/conda-v0.59.2-brightgreen?logo=anaconda)](https://prefix.dev/channels/blooop/packages/devlaunch) +[![Conda](https://img.shields.io/badge/conda-v0.59.3-brightgreen?logo=anaconda)](https://prefix.dev/channels/blooop/packages/devlaunch) [![License](https://img.shields.io/github/license/blooop/devlaunch)](https://opensource.org/license/mit/) [![Platform](https://img.shields.io/badge/platform-linux--64-blue)](https://github.com/blooop/devlaunch/releases) [![Pixi Badge](https://img.shields.io/endpoint?url=https://raw.githubusercontent.com/prefix-dev/pixi/main/assets/badge/v0.json)](https://pixi.sh) @@ -342,7 +342,7 @@ clone, and [docs/cleanup.md](docs/cleanup.md) says what it carries one past and ```bash $ dl --version -dl 0.59.2 +dl 0.59.3 ``` `--devcontainer ` picks a non-default `devcontainer.json`. A bare name means diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 7f656542..bf5417b8 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -13,7 +13,7 @@ dependencies = [ [[package]] name = "aid" -version = "0.59.2" +version = "0.59.3" dependencies = [ "devlaunch-test-support", "dl", @@ -438,7 +438,7 @@ dependencies = [ [[package]] name = "devlaunch-core" -version = "0.59.2" +version = "0.59.3" dependencies = [ "devlaunch-runner", "devlaunch-test-support", @@ -457,7 +457,7 @@ dependencies = [ [[package]] name = "devlaunch-runner" -version = "0.59.2" +version = "0.59.3" dependencies = [ "libc", "portable-pty", @@ -466,7 +466,7 @@ dependencies = [ [[package]] name = "devlaunch-test-support" -version = "0.59.2" +version = "0.59.3" dependencies = [ "devlaunch-runner", "serde", @@ -508,7 +508,7 @@ dependencies = [ [[package]] name = "dl" -version = "0.59.2" +version = "0.59.3" dependencies = [ "clap", "devlaunch-core", diff --git a/rust/Cargo.toml b/rust/Cargo.toml index 2249937e..603f55e2 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -11,7 +11,7 @@ members = [ # The single source of the version (docs/rust-rewrite-plan.md: cutover ships # 0.1.0, version read from Cargo.toml). [workspace.package] -version = "0.59.2" +version = "0.59.3" edition = "2024" license = "MIT" repository = "https://github.com/blooop/devlaunch" From 03c59f8d97fc493a5b5fac0bf797b7a972bc72df Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 2 Oct 2026 17:39:36 +0100 Subject: [PATCH 14/19] fix: a blocked rm told the user kill ends a live build it cannot end An rm, rme or --rm whose sweep spared somebody's live `devpod up` ended its line with "'dl kill' ends it and deletes the workspace". kill classifies that holder as Standing::ABuild, spares it in the same sweep, and withholds its delete (kill_delete_withheld), so the advice sent the reader to a command that does neither. The delete waiter now says to stop the build in its own terminal and that this rm deletes the workspace once it lets go. The README and docs/cli.md copies this branch added are corrected to match. The launch waiter's sentence is unchanged here. --- README.md | 2 +- docs/cli.md | 4 ++- rust/dl/src/render.rs | 69 ++++++++++++++++++++++++++++++++++++------- 3 files changed, 63 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 6ac712f1..1ecd73f8 100644 --- a/README.md +++ b/README.md @@ -297,7 +297,7 @@ might still be wanted, and `kill` when it is stuck and finished with. An `rm` th get the workspace's lock for now says so while it waits, then clears every holder that nothing is waiting on and carries on. So does a launch: `dl `, `up`, `restart`, `recreate`, `reset`, `code` and `dotfiles` all sweep the lock while their `devpod up` sits behind it. Only a holder -somebody is still waiting on is left, and the line names the `kill` that ends it. See +somebody is still waiting on is left, and the line says what is left to do. See [docs/cli.md](docs/cli.md) for the details. [docs/cli.md](docs/cli.md) has the rest: what the delete asks of devpod, what stands it down, diff --git a/docs/cli.md b/docs/cli.md index 44a7e123..39f5e43c 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -940,7 +940,9 @@ or on another host, can still wedge the workspace, and the sweep below is for th dl watches for that line. **An `rm`, `rme` or `--rm` behind the lock does not wait for you either.** It says devpod is waiting, then runs the same sweep a launch runs, described next, and the delete goes on once the holder lets go. A holder that -somebody is still waiting on is spared, and the line names the `kill` that ends it. +somebody is still waiting on is spared. When that is a live build, which `kill` +spares too, the line says to stop it in its own terminal, and the delete goes on +once it lets go. **A launch behind the lock does not wait for you.** It says devpod is waiting and that the wait has no deadline, and then it clears the lock itself: the same sweep diff --git a/rust/dl/src/render.rs b/rust/dl/src/render.rs index ef5beab4..2d4da0c5 100644 --- a/rust/dl/src/render.rs +++ b/rust/dl/src/render.rs @@ -1991,7 +1991,7 @@ fn swept_the_lock(workspace_id: &str, released: &Released, waiter: Waiter<'_>) - "dl: cleared {} from {workspace_id}, and {}, so this {noun} is still waiting.{}", named(cleared.iter().copied().map(cleared_line)), what_is_left(&still_held), - what_is_left_to_do(workspace_id, &still_held), + what_is_left_to_do(workspace_id, &still_held, waiter), ), // Nothing was dl's to take. Which of the three sentences is a match on // `StillHeld` rather than a fold to `bool` over the holders: a spared @@ -2000,7 +2000,7 @@ fn swept_the_lock(workspace_id: &str, released: &Released, waiter: Waiter<'_>) - Freed::Nothing { still_held } => format!( "dl: {workspace_id} {}. This {noun} is still waiting.{}", what_is_left(&still_held), - what_is_left_to_do(workspace_id, &still_held), + what_is_left_to_do(workspace_id, &still_held, waiter), ), // The sweep looked and found no holder. A finding rather than a failure, // and the one that says the wait is not an orphan and not dl's to clear. @@ -2064,17 +2064,29 @@ fn what_is_left(still_held: &StillHeld<'_>) -> String { /// waiting with no command at all. /// /// The two arms need different answers, which is why it is not one sentence. -/// Against a spared build, `kill` works and the sentence has to say what it -/// costs, because the build is somebody else's and `kill` deletes the workspace -/// under it. Against an orphan that sat through SIGKILL, `kill` would fail +/// Against a spared build, a launch is told the `kill` and what it costs, because +/// the build is somebody else's and `kill` deletes the workspace under it. A +/// delete is not: `kill` spares that same build and then withholds its own +/// delete, so the only way past it is the build ending, from the terminal of +/// whoever started it, after which this delete goes on. Against an orphan that sat through SIGKILL, `kill` would fail /// exactly as this sweep just did, for the same reason -- it is another user's /// and dl has no privilege to add -- so naming it would send the reader round /// the same loop. -fn what_is_left_to_do(workspace_id: &str, still_held: &StillHeld<'_>) -> String { - let end_the_build = format!( - " If that build is not wanted, 'dl {workspace_id} kill' ends it and deletes the \ - workspace." - ); +fn what_is_left_to_do( + workspace_id: &str, + still_held: &StillHeld<'_>, + waiter: Waiter<'_>, +) -> String { + let end_the_build = match waiter { + Waiter::Launch => format!( + " If that build is not wanted, 'dl {workspace_id} kill' ends it and deletes the \ + workspace." + ), + Waiter::Delete(word) => format!( + " If that build is not wanted, stop it in its own terminal, and this {word} deletes \ + the workspace once it lets go." + ), + }; let not_ours = " Only whoever owns that process, or root, can end it."; match still_held { StillHeld::Attended(_) => end_the_build, @@ -5306,6 +5318,43 @@ mod tests { ); } + /// A delete behind somebody's live build has no `kill` to be sent to: `kill` + /// spares that build exactly as this sweep did, then withholds its own delete + /// (`kill_delete_withheld`). The only way past it is the build ending, which + /// whoever started it does from their own terminal. + #[test] + fn a_delete_behind_a_spared_build_does_not_name_a_kill_that_cannot_end_it() { + for word in ["rm", "rme", "--rm"] { + let line = delete_swept( + "my-ws", + word, + &Released::Swept(Release { + signalled: Vec::new(), + holding: Holding::StillHeld { + holders: vec![Standing::ABuild(HostProcess { + pid: 5001, + parent: 5000, + command: "devpod up my-ws".to_owned(), + })], + }, + }), + ); + + assert!( + !line.contains("kill"), + "kill spares a live build and keeps its delete back: {line}" + ); + assert!( + line.contains("in its own terminal"), + "the reader is told where that build can be stopped: {line}" + ); + assert!( + line.contains(&format!("this {word} deletes the workspace")), + "and that this {word} then goes on with the delete: {line}" + ); + } + } + /// An orphan dl signalled and could not stop is almost always another user's, /// so `dl kill` is not the way out -- it would fail the same way, for the /// same reason. Saying who *can* end it is what stops the reader retrying the From 40128b2e8f10a9e29d7d2344d97814d6463298f2 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 2 Oct 2026 17:42:53 +0100 Subject: [PATCH 15/19] test: the TERM row of the inherited-ignore table passed with the drain's SIGKILL gone The fake up was `exec sleep 30`, which pdeathsig kills when dl exits, so up_alive: false held without the drain's killpg(SIGKILL). The up now forks a child that records its pid and blocks on it; a forked child does not inherit pdeathsig, and under the TERM row it inherits the ignored SIGTERM, so only the drain's SIGKILL ends it. --- rust/dl/tests/interrupt.rs | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/rust/dl/tests/interrupt.rs b/rust/dl/tests/interrupt.rs index 70cf81ee..ee8890cb 100644 --- a/rust/dl/tests/interrupt.rs +++ b/rust/dl/tests/interrupt.rs @@ -59,8 +59,8 @@ impl World { String::from_utf8_lossy(&built.stderr) ); - // Replace the fake `devpod`: `up` records its own pid and then blocks, - // every other subcommand delegates to the shim the scenario installed. + // Replace the fake `devpod`: `up` records its own pid, forks a child that + // records its pid, and blocks on it; every other subcommand delegates to the shim the scenario installed. // The original is `#!/bin/sh` + one `exec "$@"` line, and // the delegate reuses that exact line so `status`/`list`/`ssh` behave as // before. @@ -74,8 +74,10 @@ impl World { "#!/bin/sh\n\ if [ \"$1\" = \"up\" ]; then\n\ \x20 echo \"$$\" > \"$DL_UP_PID\"\n\ + \x20 sleep 30 &\n\ + \x20 echo \"$!\" > \"$DL_UP_CHILD_PID\"\n\ \x20 : > \"$DL_UP_STARTED\"\n\ - \x20 exec sleep 30\n\ + \x20 wait\n\ fi\n\ {delegate}\n" ); @@ -147,7 +149,9 @@ struct Aftermath { code: Option, /// Whether the plaintext GitHub-token file is still on disk. token_left: bool, - /// Whether the `devpod up` child outlived the `dl` that started it. + /// Whether the `devpod up` child, or the child it forked, outlived the `dl` + /// that started it. The forked one is what pins the drain's group-wide + /// SIGKILL: unlike the `up`, it does not die with `dl` by pdeathsig. up_alive: bool, } @@ -159,6 +163,7 @@ struct MidUp { child: std::process::Child, tmpdir: PathBuf, up: String, + up_child: String, } impl MidUp { @@ -195,6 +200,7 @@ impl MidUp { let tmpdir = world.path("tmp"); let up_pid = world.path("up.pid"); let up_started = world.path("up.started"); + let up_child_pid = world.path("up-child.pid"); let child = command .env_clear() @@ -213,6 +219,7 @@ impl MidUp { .env("TMPDIR", tmpdir.display().to_string()) .env("DL_UP_PID", up_pid.display().to_string()) .env("DL_UP_STARTED", up_started.display().to_string()) + .env("DL_UP_CHILD_PID", up_child_pid.display().to_string()) .env("GIT_SSH_COMMAND", "false") .env("GIT_CONFIG_GLOBAL", "/dev/null") .env("GIT_CONFIG_SYSTEM", "/dev/null") @@ -231,11 +238,13 @@ impl MidUp { "the token is on disk before the signal" ); let up = std::fs::read_to_string(&up_pid).expect("the up pid"); + let up_child = std::fs::read_to_string(&up_child_pid).expect("the up's child's pid"); MidUp { _world: world, child, tmpdir, up: up.trim().to_string(), + up_child: up_child.trim().to_string(), } } @@ -267,7 +276,7 @@ impl MidUp { Aftermath { code: status.code(), token_left: token_file(&self.tmpdir).is_some(), - up_alive: !is_dead(&self.up), + up_alive: !is_dead(&self.up) || !is_dead(&self.up_child), } } } @@ -328,7 +337,8 @@ const INHERITED_IGNORE: [(&str, Ignored); 3] = [ // `trap '' TERM` is inherited by everything `dl` spawns, so the drain's // SIGTERM does not reach the build here. This row's child used to outlive the // Ctrl-C for that reason. The drain now SIGKILLs the group after a short wait, - // so the build goes either way. + // so the build goes either way. The `up`'s forked child is what proves it: + // pdeathsig ends the `up` itself when `dl` exits, but not a child it forked. ("TERM", Ignored::Honoured), ("HUP", Ignored::Honoured), ]; From 0307b6ba212309a492df037494b6c992d9d17730 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 2 Oct 2026 17:44:06 +0100 Subject: [PATCH 16/19] test: a --rm whose sweep freed the lock is told its own delete takes it --- rust/dl/src/render.rs | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/rust/dl/src/render.rs b/rust/dl/src/render.rs index 2d4da0c5..1c1030b9 100644 --- a/rust/dl/src/render.rs +++ b/rust/dl/src/render.rs @@ -5355,6 +5355,35 @@ mod tests { } } + /// A delete whose sweep freed the lock is still sitting in its own `devpod + /// delete`, and that is the call that takes the lock next. Saying "this + /// launch's own devpod up" sends the reader looking for a launch that is not + /// there. + #[test] + fn a_delete_whose_sweep_freed_the_lock_is_told_its_own_delete_takes_it() { + for word in ["--rm", "rm"] { + let line = delete_swept( + "my-ws", + word, + &Released::Swept(Release { + signalled: vec![Signalled { + process: an_orphan(732_721), + ending: Ending::Terminated, + }], + holding: Holding::Free, + }), + ); + + assert!( + line.contains(&format!( + "This {word}'s own devpod delete takes the lock from here." + )), + "the {word} is told its own delete carries on: {line}" + ); + assert!(!line.contains("launch"), "{line}"); + } + } + /// An orphan dl signalled and could not stop is almost always another user's, /// so `dl kill` is not the way out -- it would fail the same way, for the /// same reason. Saying who *can* end it is what stops the reader retrying the From 33f88d605a14568fa9db1e3c46e046a6f287c320 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 2 Oct 2026 17:44:52 +0100 Subject: [PATCH 17/19] test: a delete does not sweep its own blocked devpod delete --- .../src/flows/lifecycle/tests.rs | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/rust/devlaunch-core/src/flows/lifecycle/tests.rs b/rust/devlaunch-core/src/flows/lifecycle/tests.rs index 5d894d89..5311436e 100644 --- a/rust/devlaunch-core/src/flows/lifecycle/tests.rs +++ b/rust/devlaunch-core/src/flows/lifecycle/tests.rs @@ -2271,6 +2271,39 @@ fn a_delete_blocked_behind_somebody_elses_live_build_signals_nothing() { ); } +/// The delete's own `devpod delete` is the process waiting for the lock, not +/// one holding it. It names the workspace in its own argv and its parent is +/// this live `dl`, so the sweep's own reading finds it unless the sweep leaves +/// out this process's children, as a launch leaves out its own `devpod up`. +#[test] +fn a_delete_does_not_count_its_own_blocked_delete_among_the_holders() { + let table = format!( + " 1 0 /sbin/init\n{:>7} {:>7} devpod delete myws\n", + 4711, + std::process::id(), + ); + + let blocked = delete_blocked_for_lines(&[&table], 1); + + assert!( + blocked.signals.is_empty(), + "a delete must never signal its own devpod delete: {:?}", + blocked.signals, + ); + let [ + DeleteStalled::OnTheLock, + DeleteStalled::Swept(kill::Released::Swept(release)), + ] = blocked.said.as_slice() + else { + panic!("the block, then its sweep: {:?}", blocked.said); + }; + assert_eq!( + release.holding, + kill::Holding::Free, + "the delete's own devpod delete was reported as holding the workspace it waits for", + ); +} + /// The deadline firing is a devpod that *ran* — for a minute, and was then /// SIGKILLed by the runner — so it may have got far enough to unlink the /// workspace record before it went. The two lines that answer for that are From bee0b5c17e7f64f02a890539a85eed30665b9243 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 2 Oct 2026 17:45:43 +0100 Subject: [PATCH 18/19] docs: the drain's pid-reuse note credited the reap with what the live group guarantees --- rust/devlaunch-runner/src/interrupt.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/rust/devlaunch-runner/src/interrupt.rs b/rust/devlaunch-runner/src/interrupt.rs index d925e920..97f52286 100644 --- a/rust/devlaunch-runner/src/interrupt.rs +++ b/rust/devlaunch-runner/src/interrupt.rs @@ -316,9 +316,13 @@ unsafe fn drain() { // nobody, off Linux. The rest of the group goes with it: devpod's own // children are in it. // - // SAFETY: `waitpid`, `poll` and `killpg` are async-signal-safe. The wait - // reaps the leader, which is this process's own child, so the pid cannot - // be reused while the group is signalled after it. + // SAFETY: `waitpid`, `poll` and `killpg` are async-signal-safe. Reaping + // the leader frees its pid, so the reap is not what keeps the SIGKILL on + // target. The live group is: Linux does not hand out a pgid while any + // member of that group lives, and an empty group makes `killpg` fail + // ESRCH. The window left is the one the SIGTERM above has: the group + // empties and a new group takes the number before the SIGKILL, which + // needs the pid space to wrap inside the wait. unsafe { wait_for_the_leader(pgid); libc::killpg(pgid, libc::SIGKILL); From 5faba368960fd7e53c8a6058492ed82647b961bd Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 2 Oct 2026 17:46:07 +0100 Subject: [PATCH 19/19] docs: kill's module doc still called the orphan's cause an open question --- rust/devlaunch-core/src/flows/kill.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/rust/devlaunch-core/src/flows/kill.rs b/rust/devlaunch-core/src/flows/kill.rs index e1672b94..77d4bae3 100644 --- a/rust/devlaunch-core/src/flows/kill.rs +++ b/rust/devlaunch-core/src/flows/kill.rs @@ -32,9 +32,11 @@ //! //! Why the orphan exists at all. Something killed a `dl` and left its child //! running, which is either a path outside #304's SIGTERM drain or a signal that -//! drain cannot catch. That is a different question with a different fix, and a -//! verb that treats the symptom does not stop being worth having while it is -//! open. +//! drain cannot catch. Its fix is in `devlaunch-runner`: on Linux the launch's +//! `devpod up` takes `PR_SET_PDEATHSIG` +//! (`devlaunch_runner::interrupt::ends_with_this_process`), so it dies with its +//! `dl`. This verb still covers what that does not: an orphan left by a `dl` that +//! predates it, a host that is not Linux, and a holder that `dl` did not start. use std::path::PathBuf; use std::time::Duration;