From b6d5c2ecdc9bf33446fbbcef3aca016f6c952753 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Hou=C5=A1ka?= Date: Wed, 28 Oct 2026 20:26:25 +0100 Subject: [PATCH] fix(env): kill the daemon's commands when it is terminated Connection.close() ends stdin and sends SIGTERM at once. The daemon did not handle SIGTERM, so it died before its end-of-input kill_all, and running commands were reparented to init. The daemon now blocks SIGTERM, SIGINT and SIGHUP before starting any thread and waits for them on its own thread, which kills every process group it started and exits. On Windows the job object already kills commands with the daemon. --- packages/env/CHANGELOG.md | 1 + packages/env/daemon/src/main.rs | 11 ++++++++++- packages/env/daemon/src/sys/unix.rs | 23 +++++++++++++++++++++++ packages/env/daemon/src/sys/windows.rs | 3 +++ packages/env/docs/protocol.md | 3 ++- packages/env/test/remote.test.ts | 25 +++++++++++++++++++++++++ 6 files changed, 64 insertions(+), 2 deletions(-) diff --git a/packages/env/CHANGELOG.md b/packages/env/CHANGELOG.md index c484db660..96bd4e91e 100644 --- a/packages/env/CHANGELOG.md +++ b/packages/env/CHANGELOG.md @@ -9,6 +9,7 @@ ### Fixed - `exec` and `pty` fail with `spawn_error` instead of hanging when the daemon cannot start a helper thread (for example at a cgroup task limit); the started command is killed and reaped ([#11433](https://github.com/earendil-works/pi/issues/11433)). +- `Connection.close()` on Unix no longer leaves running commands behind: the daemon used to die from the client's `SIGTERM` before killing its process groups. It now kills them on `SIGTERM`, `SIGINT` or `SIGHUP` before exiting. ## [1.4.0] - 2026-10-21 diff --git a/packages/env/daemon/src/main.rs b/packages/env/daemon/src/main.rs index 4457fef4d..ea720eabb 100644 --- a/packages/env/daemon/src/main.rs +++ b/packages/env/daemon/src/main.rs @@ -470,6 +470,15 @@ fn serve_frames(server: &Arc, input: &mut impl Read) -> io::Result<()> { } fn serve(token: &str) -> io::Result<()> { + let groups: exec::Groups = Arc::new(Mutex::new(Default::default())); + { + // A client that stops the daemon (Node's `child.kill()`), or a hangup, leaves nothing running either. + let groups = groups.clone(); + sys::on_exit_signal(move |signal| { + kill_all(&groups); + std::process::exit(128 + signal); + }); + } { let mut stdout = io::stdout().lock(); writeln!(stdout, "PI-ENV {token}")?; @@ -488,7 +497,7 @@ fn serve(token: &str) -> io::Result<()> { handles: Mutex::new(HashMap::new()), next_handle: AtomicU64::new(1), controls: Mutex::new(HashMap::new()), - groups: Arc::new(Mutex::new(Default::default())), + groups, terminals: Arc::new(Mutex::new(HashMap::new())), }); let last_seen = Arc::new(AtomicU64::new(now_ms())); diff --git a/packages/env/daemon/src/sys/unix.rs b/packages/env/daemon/src/sys/unix.rs index 764cad1d1..4b0637f0e 100644 --- a/packages/env/daemon/src/sys/unix.rs +++ b/packages/env/daemon/src/sys/unix.rs @@ -283,6 +283,29 @@ pub fn configure( pub fn after_spawn(_child: &Child) {} +/// Run `on_signal` with the signal on its own thread when `SIGTERM`, `SIGINT` or `SIGHUP` arrives, instead of dying +/// at once. Call before starting any other thread: threads inherit the blocked signals, so only this one receives +/// them. Commands reset their signal mask before they run. +pub fn on_exit_signal(on_signal: impl FnOnce(i32) + Send + 'static) { + // SAFETY: a zeroed sigset_t initialized by sigemptyset, and plain signal mask calls. + let set = unsafe { + let mut set: libc::sigset_t = std::mem::zeroed(); + libc::sigemptyset(&mut set); + for signal in [libc::SIGTERM, libc::SIGINT, libc::SIGHUP] { + libc::sigaddset(&mut set, signal); + } + libc::pthread_sigmask(libc::SIG_BLOCK, &set, std::ptr::null_mut()); + set + }; + std::thread::spawn(move || { + let mut signal = 0; + // SAFETY: waits for one of the signals blocked above. + if unsafe { libc::sigwait(&set, &mut signal) } == 0 { + on_signal(signal); + } + }); +} + /// Kill the command's process group, or the process if the group is gone. pub fn kill_tree(pid: u32) { let pid = pid as i32; diff --git a/packages/env/daemon/src/sys/windows.rs b/packages/env/daemon/src/sys/windows.rs index f43a4bacf..de39fd4c3 100644 --- a/packages/env/daemon/src/sys/windows.rs +++ b/packages/env/daemon/src/sys/windows.rs @@ -550,6 +550,9 @@ pub fn after_spawn(child: &Child) { } } +/// Nothing to do: the daemon cannot catch `TerminateProcess`, and the job object kills its commands with it. +pub fn on_exit_signal(_on_signal: impl FnOnce(i32) + Send + 'static) {} + /// Node's `killProcessTree` on Windows: `taskkill /F /T`, not waited for. pub fn kill_tree(pid: u32) { let system_root = std::env::var("SystemRoot").unwrap_or_else(|_| "C:\\Windows".into()); diff --git a/packages/env/docs/protocol.md b/packages/env/docs/protocol.md index ae43fb767..f8ca5b4e1 100644 --- a/packages/env/docs/protocol.md +++ b/packages/env/docs/protocol.md @@ -62,7 +62,8 @@ command as a full pipe would. ## Liveness Each side sends `ping` every 5 seconds. The daemon kills every process group it started and exits after 30 seconds -without receiving any bytes, or when stdin ends. A dropped connection therefore stops remote commands. The client +without receiving any bytes, when stdin ends, or on `SIGTERM`, `SIGINT` or `SIGHUP` (on Windows, a job object kills +its commands with it). A dropped connection or a stopped daemon therefore stops remote commands. The client fails requests of a lost connection with `{ code: "unknown", lost: true }` and starts a new daemon on the next request; handles belong to the daemon that opened them, so requests on them fail instead of reaching the new one. diff --git a/packages/env/test/remote.test.ts b/packages/env/test/remote.test.ts index 4b1241148..4f7370679 100644 --- a/packages/env/test/remote.test.ts +++ b/packages/env/test/remote.test.ts @@ -117,6 +117,31 @@ describe("RemoteExecutionEnv", () => { expect(Date.now() - started).toBeLessThan(3000); }); + // The daemon used to die on Connection.close()'s SIGTERM before killing its commands. + it.skipIf(process.platform === "win32")("kills running commands when the connection closes", async () => { + const { env, connection } = environment(); + const pid = Promise.withResolvers(); + const running = env.exec( + ["sh", "-c", "echo $$; exec sleep 30"], + { onOutput: (text: string) => pid.resolve(Number.parseInt(text, 10)) }, + context, + ); + const command = await pid.promise; + connection.close(); + await running; + const alive = () => { + try { + process.kill(command, 0); + return true; + } catch { + return false; + } + }; + const deadline = Date.now() + 5000; + while (alive() && Date.now() < deadline) await new Promise((done) => setTimeout(done, 50)); + expect(alive()).toBe(false); + }); + it("fails requests on handles of a lost connection instead of reaching the new daemon", async () => { const { env, connection } = environment(); getOrThrow(await env.writeFile("a.txt", "AAA", context));