From adb1b790eb67473d30ae29589658c05b3bd5bfbf Mon Sep 17 00:00:00 2001 From: Azer Sadykhzadeh Date: Fri, 16 Oct 2026 22:55:50 +0000 Subject: [PATCH 1/4] End and reap a local shell when its tab closes, and drop the unused os and process plugins Removing a local terminal dropped its portable-pty child without waiting on it, so every closed tab left a zombie for as long as the app ran, and a shell that ignored the PTY hangup kept running. LocalShell now sends the hangup itself and reaps the shell on a thread of its own. A shell whose exit try_wait already collected is never signalled, because its pid may belong to another process by then. tauri-plugin-os and tauri-plugin-process were registered and os:default was granted, but nothing in the frontend imports either plugin, and restarts go through AppHandle::request_restart. They only added code and let the page read OS details it has no use for. --- Cargo.lock | 65 --------------- desktop/src-tauri/Cargo.toml | 2 - desktop/src-tauri/capabilities/default.json | 3 +- desktop/src-tauri/src/commands.rs | 2 +- desktop/src-tauri/src/main.rs | 2 - desktop/src-tauri/src/state.rs | 90 +++++++++++++++++---- 6 files changed, 77 insertions(+), 87 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 33c749f3..a9812e3a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -703,8 +703,6 @@ dependencies = [ "tauri", "tauri-build", "tauri-plugin-dialog", - "tauri-plugin-os", - "tauri-plugin-process", "tauri-plugin-shell", "tauri-plugin-single-instance", "tauri-plugin-updater", @@ -1843,16 +1841,6 @@ dependencies = [ "typenum", ] -[[package]] -name = "gethostname" -version = "1.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1bd49230192a3797a9a4d6abe9b3eed6f7fa4c8a8a4947977c6f80025f92cbd8" -dependencies = [ - "rustix", - "windows-link", -] - [[package]] name = "getrandom" version = "0.2.17" @@ -3414,22 +3402,6 @@ dependencies = [ "pin-project-lite", ] -[[package]] -name = "os_info" -version = "3.15.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9cf20a545b305cf1da722b236b5155c9bb35f1d5ceb28c048bd96ca842f41b5b" -dependencies = [ - "android_system_properties", - "log", - "nix 0.31.3", - "objc2", - "objc2-foundation", - "objc2-ui-kit", - "serde", - "windows-sys 0.61.2", -] - [[package]] name = "os_pipe" version = "1.2.3" @@ -5310,15 +5282,6 @@ dependencies = [ "syn 2.0.119", ] -[[package]] -name = "sys-locale" -version = "0.3.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8eab9a99a024a169fe8a903cf9d4a3b3601109bcc13bd9e3c6fff259138626c4" -dependencies = [ - "libc", -] - [[package]] name = "system-configuration" version = "0.7.0" @@ -5592,34 +5555,6 @@ dependencies = [ "url", ] -[[package]] -name = "tauri-plugin-os" -version = "2.4.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4bd69d272ab772b30462779de12a40f837616831c794c71531bea91474e1061f" -dependencies = [ - "gethostname", - "log", - "os_info", - "serde", - "serde_json", - "serialize-to-javascript", - "sys-locale", - "tauri", - "tauri-plugin", - "thiserror 2.0.21", -] - -[[package]] -name = "tauri-plugin-process" -version = "2.4.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "74ae3d8549ae564c0397377373e262f340908eb23a5b9fbe03f2a00007dc0038" -dependencies = [ - "tauri", - "tauri-plugin", -] - [[package]] name = "tauri-plugin-shell" version = "2.4.0" diff --git a/desktop/src-tauri/Cargo.toml b/desktop/src-tauri/Cargo.toml index be8044fc..df10a732 100644 --- a/desktop/src-tauri/Cargo.toml +++ b/desktop/src-tauri/Cargo.toml @@ -13,9 +13,7 @@ clavyn-core = { path = "../../core" } tauri = { version = "2", features = [] } tauri-plugin-shell = "2" tauri-plugin-dialog = "2" -tauri-plugin-os = "2" tauri-plugin-updater = "2" -tauri-plugin-process = "2" tauri-plugin-single-instance = "2" serde = { workspace = true } serde_json = { workspace = true } diff --git a/desktop/src-tauri/capabilities/default.json b/desktop/src-tauri/capabilities/default.json index 962531c1..f269fef5 100644 --- a/desktop/src-tauri/capabilities/default.json +++ b/desktop/src-tauri/capabilities/default.json @@ -5,7 +5,6 @@ "windows": ["main"], "permissions": [ "core:default", - "shell:allow-open", - "os:default" + "shell:allow-open" ] } diff --git a/desktop/src-tauri/src/commands.rs b/desktop/src-tauri/src/commands.rs index 38c153bd..42c23612 100644 --- a/desktop/src-tauri/src/commands.rs +++ b/desktop/src-tauri/src/commands.rs @@ -766,7 +766,7 @@ pub async fn create_local_terminal( let terminal = LocalTerminal { writer, master, - child, + child: crate::state::LocalShell::new(child), owns_session_id: owns_session_id.clone(), }; let refused = { diff --git a/desktop/src-tauri/src/main.rs b/desktop/src-tauri/src/main.rs index 9024747e..988255e6 100644 --- a/desktop/src-tauri/src/main.rs +++ b/desktop/src-tauri/src/main.rs @@ -121,8 +121,6 @@ fn main() { builder .plugin(tauri_plugin_shell::init()) .plugin(tauri_plugin_dialog::init()) - .plugin(tauri_plugin_os::init()) - .plugin(tauri_plugin_process::init()) .plugin(tauri_plugin_updater::Builder::new().build()) .setup(|app| { // State lives in the non-roaming directory, so a roaming profile diff --git a/desktop/src-tauri/src/state.rs b/desktop/src-tauri/src/state.rs index 76c7d1df..6c559a71 100644 --- a/desktop/src-tauri/src/state.rs +++ b/desktop/src-tauri/src/state.rs @@ -356,9 +356,9 @@ pub struct LocalTerminal { /// which take the same lock to admit a session id. pub writer: Arc>>, pub master: Box, - /// Kept alive so the shell is not reaped, and polled with `try_wait` to - /// tell a live terminal from one whose shell has already exited. - pub child: Box, + /// Polled with `try_wait` to tell a live terminal from one whose shell + /// has already exited. Dropping the terminal ends and reaps the shell. + pub child: LocalShell, /// Cleared when this terminal stops holding its session id. /// /// The thread draining the PTY outlives the map entry: it is detached, and @@ -371,6 +371,56 @@ pub struct LocalTerminal { pub owns_session_id: Arc, } +/// The shell process behind a local terminal. +/// +/// Dropping it ends the shell — SIGHUP on Unix, as closing a terminal window +/// does, and process termination on Windows — and reaps it on a thread of its +/// own, so a closed tab leaves neither a running shell nor a zombie behind. +/// Background jobs the user detached from the shell are not touched. +pub struct LocalShell { + child: Option>, + /// Set once `try_wait` has collected the exit status. The process id may + /// belong to an unrelated process from then on, so it is never signalled. + reaped: bool, +} + +impl LocalShell { + pub fn new(child: Box) -> Self { + Self { + child: Some(child), + reaped: false, + } + } + + pub fn try_wait(&mut self) -> std::io::Result> { + let Some(child) = self.child.as_mut() else { + return Ok(None); + }; + let status = child.try_wait()?; + self.reaped |= status.is_some(); + Ok(status) + } +} + +impl Drop for LocalShell { + fn drop(&mut self) { + let Some(mut child) = self.child.take() else { + return; + }; + if self.reaped { + return; + } + let _ = child.kill(); + // `wait` blocks until the shell is gone, which a shell that ignores + // SIGHUP may never be; a thread of its own keeps that off the caller. + let _ = std::thread::Builder::new() + .name("local-shell-reaper".into()) + .spawn(move || { + let _ = child.wait(); + }); + } +} + impl LocalTerminal { /// Marks the terminal as no longer holding its session id. pub fn disown(&self) { @@ -527,7 +577,7 @@ mod local_terminal_ownership_tests { LocalTerminal { writer, master: pair.master, - child, + child: LocalShell::new(child), owns_session_id: owns_session_id.clone(), }, owns_session_id, @@ -549,9 +599,25 @@ mod local_terminal_ownership_tests { // The id is free for reuse from here, so the reader still draining the // old shell must stop addressing it. - let mut closed = locals.remove("local-0").expect("the terminal was live"); + let _closed = locals.remove("local-0").expect("the terminal was live"); assert!(!owns.load(Ordering::Acquire)); - let _ = closed.child.kill(); + } + + /// Closing a tab ends its shell and collects it, rather than leaving a + /// running shell or a zombie for as long as the app runs. + #[cfg(target_os = "linux")] + #[test] + fn dropping_a_terminal_ends_and_reaps_its_shell() { + let (terminal, _) = terminal(); + let pid = terminal.child.child.as_ref().and_then(|c| c.process_id()).expect("pid"); + let proc_entry = std::path::PathBuf::from(format!("/proc/{pid}")); + assert!(proc_entry.exists()); + drop(terminal); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); + while proc_entry.exists() { + assert!(std::time::Instant::now() < deadline, "the shell is still around"); + std::thread::sleep(std::time::Duration::from_millis(20)); + } } #[test] @@ -562,9 +628,6 @@ mod local_terminal_ownership_tests { let reaped = locals.reap_exited(|_| true); assert_eq!(reaped.len(), 1); assert!(!owns.load(Ordering::Acquire)); - for mut terminal in reaped { - let _ = terminal.child.kill(); - } } #[test] @@ -576,8 +639,7 @@ mod local_terminal_ownership_tests { assert!(locals.remove("other").is_none()); assert!(owns.load(Ordering::Acquire)); - let mut open = locals.remove("local-0").expect("the terminal was live"); - let _ = open.child.kill(); + let _open = locals.remove("local-0").expect("the terminal was live"); } /// The window the token closes: a close drops the first open's @@ -597,10 +659,9 @@ mod local_terminal_ownership_tests { // And its fulfil installs nothing — the caller tears the shell down. let (stale_terminal, _) = terminal(); - let mut refused = locals + let _refused = locals .fulfil("local-0", stale, stale_terminal) .expect_err("the reservation is no longer the first request's"); - let _ = refused.child.kill(); assert!(locals.contains("local-0")); assert!(locals.get_live("local-0").is_none()); @@ -608,8 +669,7 @@ mod local_terminal_ownership_tests { assert!(locals.fulfil("local-0", current, live_terminal).is_ok()); assert!(owns.load(Ordering::Acquire)); - let mut open = locals.remove("local-0").expect("the terminal is live"); - let _ = open.child.kill(); + let _open = locals.remove("local-0").expect("the terminal is live"); } } From 524778f761b036ac8f7d79f2d28d49094263bdc9 Mon Sep 17 00:00:00 2001 From: Azer Sadykhzadeh Date: Fri, 16 Oct 2026 23:13:44 +0000 Subject: [PATCH 2/4] Give a closed tab's shell time to exit, and end it off the map lock portable-pty's kill() sends SIGHUP, polls for up to 200 ms and then kills outright, and the terminal is dropped under the local terminal map's lock. Closing a tab whose shell took its time therefore held up every other local terminal operation, and cut short whatever the shell does on hangup. The whole shutdown now runs on the reaper thread: the shell gets two seconds to exit after the PTY hangs up, and only then is it signalled. If no thread can be started, the shell is ended inline rather than left as a zombie. --- desktop/src-tauri/src/state.rs | 96 ++++++++++++++++++++++++++++++---- 1 file changed, 86 insertions(+), 10 deletions(-) diff --git a/desktop/src-tauri/src/state.rs b/desktop/src-tauri/src/state.rs index 6c559a71..d4294fa0 100644 --- a/desktop/src-tauri/src/state.rs +++ b/desktop/src-tauri/src/state.rs @@ -371,12 +371,21 @@ pub struct LocalTerminal { pub owns_session_id: Arc, } +/// How long a closed tab's shell gets to exit on its own after the terminal +/// hangs up, before it is signalled. +const SHELL_EXIT_GRACE: std::time::Duration = std::time::Duration::from_secs(2); + /// The shell process behind a local terminal. /// -/// Dropping it ends the shell — SIGHUP on Unix, as closing a terminal window -/// does, and process termination on Windows — and reaps it on a thread of its -/// own, so a closed tab leaves neither a running shell nor a zombie behind. -/// Background jobs the user detached from the shell are not touched. +/// Dropping it ends the shell and reaps it, so a closed tab leaves neither a +/// running shell nor a zombie behind. All of that happens on a thread of its +/// own, because a terminal is dropped under the terminal map's lock and the +/// shutdown can take seconds. Dropping the PTY hangs the terminal up, which +/// ends an ordinary shell, so the shell first gets [`SHELL_EXIT_GRACE`] to +/// finish whatever it does on hangup. Only then is it signalled: SIGHUP with +/// portable-pty's short grace and a forced kill after it on Unix, process +/// termination on Windows. Background jobs the user detached from the shell +/// are not touched. pub struct LocalShell { child: Option>, /// Set once `try_wait` has collected the exit status. The process id may @@ -402,22 +411,48 @@ impl LocalShell { } } +/// End `child` and collect its exit status, blocking until it is gone. +fn end_shell(mut child: Box) { + let deadline = std::time::Instant::now() + SHELL_EXIT_GRACE; + loop { + match child.try_wait() { + Ok(Some(_)) => return, + Ok(None) if std::time::Instant::now() < deadline => { + std::thread::sleep(std::time::Duration::from_millis(50)); + } + _ => break, + } + } + let _ = child.kill(); + let _ = child.wait(); +} + impl Drop for LocalShell { fn drop(&mut self) { - let Some(mut child) = self.child.take() else { + let Some(child) = self.child.take() else { return; }; if self.reaped { return; } - let _ = child.kill(); - // `wait` blocks until the shell is gone, which a shell that ignores - // SIGHUP may never be; a thread of its own keeps that off the caller. - let _ = std::thread::Builder::new() + // The slot lets this thread take the child back if no reaper thread + // can be started, and end it here rather than leave a zombie. + let slot = Arc::new(std::sync::Mutex::new(Some(child))); + let reaper_slot = slot.clone(); + let spawned = std::thread::Builder::new() .name("local-shell-reaper".into()) .spawn(move || { - let _ = child.wait(); + let child = reaper_slot.lock().unwrap_or_else(|e| e.into_inner()).take(); + if let Some(child) = child { + end_shell(child); + } }); + if spawned.is_err() { + let child = slot.lock().unwrap_or_else(|e| e.into_inner()).take(); + if let Some(child) = child { + end_shell(child); + } + } } } @@ -620,6 +655,47 @@ mod local_terminal_ownership_tests { } } + /// A shell that ignores the hangup is still ended, after its grace period, + /// and the drop that starts all this returns at once: it runs under the + /// terminal map's lock. + #[cfg(target_os = "linux")] + #[test] + fn a_shell_that_ignores_the_hangup_is_ended_without_blocking_the_drop() { + let pair = native_pty_system() + .openpty(PtySize { + rows: 24, + cols: 80, + pixel_width: 0, + pixel_height: 0, + }) + .expect("a pty can be opened"); + let mut command = CommandBuilder::new("/bin/sh"); + command.args(["-c", "trap '' HUP; sleep 60"]); + let child = pair.slave.spawn_command(command).expect("a shell can be spawned"); + let pid = child.process_id().expect("pid"); + let terminal = LocalTerminal { + writer: Arc::new(std::sync::Mutex::new( + pair.master.take_writer().expect("the master has a writer"), + )), + master: pair.master, + child: LocalShell::new(child), + owns_session_id: Arc::new(AtomicBool::new(true)), + }; + // Let the trap take effect before the hangup arrives. + std::thread::sleep(std::time::Duration::from_millis(200)); + + let started = std::time::Instant::now(); + drop(terminal); + assert!(started.elapsed() < std::time::Duration::from_millis(100)); + + let proc_entry = std::path::PathBuf::from(format!("/proc/{pid}")); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); + while proc_entry.exists() { + assert!(std::time::Instant::now() < deadline, "the shell is still around"); + std::thread::sleep(std::time::Duration::from_millis(50)); + } + } + #[test] fn reaping_an_exited_terminal_hands_its_session_id_back() { let mut locals = LocalTerminals::default(); From 2fa9d19e829ca30328cdfb42b864627fbefeacaa Mon Sep 17 00:00:00 2001 From: Azer Sadykhzadeh Date: Sat, 17 Oct 2026 07:04:32 +0000 Subject: [PATCH 3/4] Send a closed tab's shell SIGHUP directly The PTY hangup only reaches the shell once every copy of the master is closed, and the output reader keeps one open while it is blocked reading. A shell busy with a foreground command therefore heard nothing until the grace period ran out and portable-pty's kill took over. It now gets SIGHUP as soon as the tab closes, as with a terminal window, and the grace period starts from there. --- Cargo.lock | 1 + desktop/src-tauri/Cargo.toml | 5 +++ desktop/src-tauri/src/state.rs | 64 ++++++++++++++++++++++++++++++---- 3 files changed, 64 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a9812e3a..49fbaf5a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -691,6 +691,7 @@ dependencies = [ "core-foundation-sys", "keyring", "keyring-core", + "libc", "objc2-local-authentication", "portable-pty", "reqwest 0.12.28", diff --git a/desktop/src-tauri/Cargo.toml b/desktop/src-tauri/Cargo.toml index df10a732..096f5eb8 100644 --- a/desktop/src-tauri/Cargo.toml +++ b/desktop/src-tauri/Cargo.toml @@ -39,6 +39,11 @@ tauri = { version = "2", features = ["test"] } # the known hosts store takes; the workspace pin keeps it the same russh. russh = { workspace = true } +[target.'cfg(unix)'.dependencies] +# Sends a closed tab's shell SIGHUP directly; already in the tree through +# portable-pty and tokio. +libc = "0.2" + [target.'cfg(target_os = "linux")'.dependencies] # Resolves the D-Bus session address before the single-instance plugin does, # because the plugin resolves it with `unwrap` and `panic = "abort"` turns that diff --git a/desktop/src-tauri/src/state.rs b/desktop/src-tauri/src/state.rs index d4294fa0..4d6d9df1 100644 --- a/desktop/src-tauri/src/state.rs +++ b/desktop/src-tauri/src/state.rs @@ -380,12 +380,11 @@ const SHELL_EXIT_GRACE: std::time::Duration = std::time::Duration::from_secs(2); /// Dropping it ends the shell and reaps it, so a closed tab leaves neither a /// running shell nor a zombie behind. All of that happens on a thread of its /// own, because a terminal is dropped under the terminal map's lock and the -/// shutdown can take seconds. Dropping the PTY hangs the terminal up, which -/// ends an ordinary shell, so the shell first gets [`SHELL_EXIT_GRACE`] to -/// finish whatever it does on hangup. Only then is it signalled: SIGHUP with -/// portable-pty's short grace and a forced kill after it on Unix, process -/// termination on Windows. Background jobs the user detached from the shell -/// are not touched. +/// shutdown can take seconds. On Unix the shell is sent SIGHUP, as closing a +/// terminal window does, and gets [`SHELL_EXIT_GRACE`] to finish whatever it +/// does on hangup. Only then is it forced: portable-pty's kill, which ends in +/// SIGKILL on Unix and process termination on Windows. Background jobs the +/// user detached from the shell are not touched. pub struct LocalShell { child: Option>, /// Set once `try_wait` has collected the exit status. The process id may @@ -413,6 +412,17 @@ impl LocalShell { /// End `child` and collect its exit status, blocking until it is gone. fn end_shell(mut child: Box) { + // The PTY hangup only reaches the shell once every copy of the master is + // closed, and the output reader holds one until it next wakes up, so the + // shell is sent SIGHUP directly. It has not been waited on yet, so its + // pid still names it. + #[cfg(unix)] + if let Some(pid) = child.process_id().and_then(|pid| libc::pid_t::try_from(pid).ok()) { + // SAFETY: kill(2) takes plain integers and has no memory effects. + unsafe { + libc::kill(pid, libc::SIGHUP); + } + } let deadline = std::time::Instant::now() + SHELL_EXIT_GRACE; loop { match child.try_wait() { @@ -696,6 +706,48 @@ mod local_terminal_ownership_tests { } } + /// The output reader keeps its own copy of the PTY master, so closing the + /// terminal does not hang the tty up while the reader is blocked. The shell + /// still hears about the close at once rather than after the grace period. + #[cfg(target_os = "linux")] + #[test] + fn a_busy_shell_gets_the_hangup_without_waiting_for_the_reader() { + let pair = native_pty_system() + .openpty(PtySize { + rows: 24, + cols: 80, + pixel_width: 0, + pixel_height: 0, + }) + .expect("a pty can be opened"); + let mut command = CommandBuilder::new("/bin/sh"); + command.args(["-c", "trap 'exit 0' HUP; while :; do sleep 0.05; done"]); + let child = pair.slave.spawn_command(command).expect("a shell can be spawned"); + let pid = child.process_id().expect("pid"); + let reader = pair.master.try_clone_reader().expect("a reader"); + let terminal = LocalTerminal { + writer: Arc::new(std::sync::Mutex::new( + pair.master.take_writer().expect("the master has a writer"), + )), + master: pair.master, + child: LocalShell::new(child), + owns_session_id: Arc::new(AtomicBool::new(true)), + }; + std::thread::sleep(std::time::Duration::from_millis(200)); + + drop(terminal); + let proc_entry = std::path::PathBuf::from(format!("/proc/{pid}")); + let deadline = std::time::Instant::now() + std::time::Duration::from_millis(1500); + while proc_entry.exists() { + assert!( + std::time::Instant::now() < deadline, + "the shell did not get the hangup while the reader held the master" + ); + std::thread::sleep(std::time::Duration::from_millis(20)); + } + drop(reader); + } + #[test] fn reaping_an_exited_terminal_hands_its_session_id_back() { let mut locals = LocalTerminals::default(); From 8a95bfbff08a0a4e2792ed12b2d21ee4ed80bfaa Mon Sep 17 00:00:00 2001 From: computerbox124 Date: Thu, 12 Nov 2026 01:13:09 +0000 Subject: [PATCH 4/4] Never signal a shell whose status is unreadable, and tear terminals down off the map lock A try_wait error was treated as a shell still running, so the reaper went on to SIGHUP and SIGKILL its raw pid. That error is how a process something else already collected shows up (ECHILD), and its pid may then belong to an unrelated process. Every signal now follows a try_wait that reports the shell as running, and an unreadable status leaves it alone. close_session and the vault teardown still dropped terminals under the terminal-map lock, and the terminal's fields dropped the writer before the shell. The writer's drop writes EOF to the master, which blocks while the tty input queue is full, so one stuck tab could hold the lock every other session needs, with the hangup that would unblock it queued behind it. Both paths now drop with the lock released, and the shell is hung up first. The HUP-ignoring test execs its sleep, so the process it kills is the sleeper rather than a shell that left a sleep 60 running after every run. --- desktop/src-tauri/src/commands.rs | 8 +- desktop/src-tauri/src/state.rs | 228 +++++++++++++++++++++--- desktop/src-tauri/src/vault_commands.rs | 7 +- 3 files changed, 213 insertions(+), 30 deletions(-) diff --git a/desktop/src-tauri/src/commands.rs b/desktop/src-tauri/src/commands.rs index 42c23612..748bbc27 100644 --- a/desktop/src-tauri/src/commands.rs +++ b/desktop/src-tauri/src/commands.rs @@ -962,9 +962,11 @@ pub async fn close_session(state: State<'_, Arc>, session_id: String) if state.sessions.list().await.contains(&session_id) { return state.sessions.close(&session_id).await.map_err(err); } - // Try local terminal - let mut locals = state.local_terminals.lock().await; - locals.remove(&session_id); + // Try local terminal. The terminal is dropped with the lock released: + // dropping its writer writes EOF to the PTY, which blocks while the tty + // input queue is full, and every other session needs this lock. + let closed = state.local_terminals.lock().await.remove(&session_id); + drop(closed); Ok(()) } diff --git a/desktop/src-tauri/src/state.rs b/desktop/src-tauri/src/state.rs index 4d6d9df1..1a0f4986 100644 --- a/desktop/src-tauri/src/state.rs +++ b/desktop/src-tauri/src/state.rs @@ -315,15 +315,19 @@ impl LocalTerminals { removed } - /// Drops every live terminal and forgets every reservation. Live terminals - /// are disowned first so a still-running reader cannot speak for an id a - /// later session reuses, and a reservation left behind would hold a slot - /// for a terminal nobody wants any more. - pub fn clear(&mut self) { - for (_, terminal) in self.live.drain() { - terminal.disown(); - } + /// Takes out every live terminal and forgets every reservation, returning + /// the terminals so the caller can drop them with the lock released. Live + /// terminals are disowned here so a still-running reader cannot speak for + /// an id a later session reuses, and a reservation left behind would hold a + /// slot for a terminal nobody wants any more. + #[must_use = "dropping the terminals under the map lock stalls every other session"] + pub fn clear(&mut self) -> Vec { self.opening.clear(); + self.live + .drain() + .map(|(_, terminal)| terminal) + .inspect(LocalTerminal::disown) + .collect() } /// Takes out the terminals `exited` reports as finished, returning them so @@ -348,6 +352,14 @@ impl LocalTerminals { /// A local terminal session backed by portable-pty. pub struct LocalTerminal { + /// Polled with `try_wait` to tell a live terminal from one whose shell + /// has already exited. Dropping the terminal ends and reaps the shell. + /// + /// Declared first so it is dropped first: the shell gets its hangup before + /// the writer is dropped. The writer's drop writes EOF to the master, which + /// blocks while the tty input queue is full until the shell's side of the + /// tty closes. + pub child: LocalShell, /// Behind its own lock so a write never runs under the terminal map's /// lock. A write to a PTY master blocks once the tty input buffer fills and /// the foreground process is not reading its stdin, and holding the map @@ -356,9 +368,6 @@ pub struct LocalTerminal { /// which take the same lock to admit a session id. pub writer: Arc>>, pub master: Box, - /// Polled with `try_wait` to tell a live terminal from one whose shell - /// has already exited. Dropping the terminal ends and reaps the shell. - pub child: LocalShell, /// Cleared when this terminal stops holding its session id. /// /// The thread draining the PTY outlives the map entry: it is detached, and @@ -379,8 +388,8 @@ const SHELL_EXIT_GRACE: std::time::Duration = std::time::Duration::from_secs(2); /// /// Dropping it ends the shell and reaps it, so a closed tab leaves neither a /// running shell nor a zombie behind. All of that happens on a thread of its -/// own, because a terminal is dropped under the terminal map's lock and the -/// shutdown can take seconds. On Unix the shell is sent SIGHUP, as closing a +/// own, because the shutdown can take seconds and a terminal is dropped by an +/// async command handler. On Unix the shell is sent SIGHUP, as closing a /// terminal window does, and gets [`SHELL_EXIT_GRACE`] to finish whatever it /// does on hangup. Only then is it forced: portable-pty's kill, which ends in /// SIGKILL on Unix and process termination on Windows. Background jobs the @@ -410,12 +419,28 @@ impl LocalShell { } } +/// Whether `try_wait` reports `child` as still running. +/// +/// A status that cannot be read counts as not running. On Unix that is how a +/// process something else has already collected shows up (`ECHILD`), and its +/// pid may name an unrelated process from then on, so nothing may signal it. +fn shell_still_running(child: &mut (dyn portable_pty::Child + Send + Sync)) -> bool { + matches!(child.try_wait(), Ok(None)) +} + /// End `child` and collect its exit status, blocking until it is gone. +/// +/// Every signal goes to a raw pid, so each one is sent only right after +/// [`shell_still_running`] has confirmed the shell is still this process's +/// uncollected child. A shell that has exited, or whose status cannot be +/// read, is left alone. fn end_shell(mut child: Box) { + if !shell_still_running(&mut *child) { + return; + } // The PTY hangup only reaches the shell once every copy of the master is - // closed, and the output reader holds one until it next wakes up, so the - // shell is sent SIGHUP directly. It has not been waited on yet, so its - // pid still names it. + // closed, and the output reader holds one until the slave side closes, so + // the shell is sent SIGHUP directly. #[cfg(unix)] if let Some(pid) = child.process_id().and_then(|pid| libc::pid_t::try_from(pid).ok()) { // SAFETY: kill(2) takes plain integers and has no memory effects. @@ -425,13 +450,13 @@ fn end_shell(mut child: Box) { } let deadline = std::time::Instant::now() + SHELL_EXIT_GRACE; loop { - match child.try_wait() { - Ok(Some(_)) => return, - Ok(None) if std::time::Instant::now() < deadline => { - std::thread::sleep(std::time::Duration::from_millis(50)); - } - _ => break, + if !shell_still_running(&mut *child) { + return; } + if std::time::Instant::now() >= deadline { + break; + } + std::thread::sleep(std::time::Duration::from_millis(50)); } let _ = child.kill(); let _ = child.wait(); @@ -666,8 +691,8 @@ mod local_terminal_ownership_tests { } /// A shell that ignores the hangup is still ended, after its grace period, - /// and the drop that starts all this returns at once: it runs under the - /// terminal map's lock. + /// and the drop that starts all this returns at once: it runs on an async + /// command handler. #[cfg(target_os = "linux")] #[test] fn a_shell_that_ignores_the_hangup_is_ended_without_blocking_the_drop() { @@ -679,8 +704,10 @@ mod local_terminal_ownership_tests { pixel_height: 0, }) .expect("a pty can be opened"); + // `exec` makes the HUP-ignoring sleep the process that is ended. Forked + // instead, it would survive its shell and outlive the test. let mut command = CommandBuilder::new("/bin/sh"); - command.args(["-c", "trap '' HUP; sleep 60"]); + command.args(["-c", "trap '' HUP; exec sleep 60"]); let child = pair.slave.spawn_command(command).expect("a shell can be spawned"); let pid = child.process_id().expect("pid"); let terminal = LocalTerminal { @@ -691,7 +718,7 @@ mod local_terminal_ownership_tests { child: LocalShell::new(child), owns_session_id: Arc::new(AtomicBool::new(true)), }; - // Let the trap take effect before the hangup arrives. + // Let the trap and the exec take effect before the hangup arrives. std::thread::sleep(std::time::Duration::from_millis(200)); let started = std::time::Instant::now(); @@ -748,6 +775,155 @@ mod local_terminal_ownership_tests { drop(reader); } + /// Stands in for the real writer, whose drop writes EOF to the master and + /// blocks while the tty input queue is full. This one blocks until the + /// shell is gone, up to a deadline, and records whether it got there. + #[cfg(target_os = "linux")] + struct WriterBlockedUntilShellExits { + proc_entry: std::path::PathBuf, + unblocked: Arc, + } + + #[cfg(target_os = "linux")] + impl std::io::Write for WriterBlockedUntilShellExits { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + Ok(buf.len()) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } + } + + #[cfg(target_os = "linux")] + impl Drop for WriterBlockedUntilShellExits { + fn drop(&mut self) { + let deadline = std::time::Instant::now() + std::time::Duration::from_millis(1500); + while self.proc_entry.exists() && std::time::Instant::now() < deadline { + std::thread::sleep(std::time::Duration::from_millis(20)); + } + self.unblocked + .store(!self.proc_entry.exists(), Ordering::Release); + } + } + + /// Only the shell's side of the tty closing unblocks the writer's drop, so + /// the shell is hung up before the writer is dropped rather than after. + #[cfg(target_os = "linux")] + #[test] + fn a_terminal_hangs_its_shell_up_before_dropping_its_writer() { + let pair = native_pty_system() + .openpty(PtySize { + rows: 24, + cols: 80, + pixel_width: 0, + pixel_height: 0, + }) + .expect("a pty can be opened"); + let mut command = CommandBuilder::new("/bin/sh"); + command.args(["-c", "while :; do sleep 0.05; done"]); + let child = pair.slave.spawn_command(command).expect("a shell can be spawned"); + let pid = child.process_id().expect("pid"); + let unblocked = Arc::new(AtomicBool::new(false)); + let writer: Box = Box::new(WriterBlockedUntilShellExits { + proc_entry: std::path::PathBuf::from(format!("/proc/{pid}")), + unblocked: unblocked.clone(), + }); + let terminal = LocalTerminal { + child: LocalShell::new(child), + writer: Arc::new(std::sync::Mutex::new(writer)), + master: pair.master, + owns_session_id: Arc::new(AtomicBool::new(true)), + }; + + drop(terminal); + assert!( + unblocked.load(Ordering::Acquire), + "the writer was dropped before the shell was hung up" + ); + } + + /// A child whose status cannot be read, which is how a shell that + /// something else has already collected reports itself, while its pid + /// names another process. + #[cfg(unix)] + #[derive(Debug)] + struct CollectedElsewhere { + pid: u32, + killed: Arc, + } + + #[cfg(unix)] + impl portable_pty::ChildKiller for CollectedElsewhere { + fn kill(&mut self) -> std::io::Result<()> { + self.killed.store(true, Ordering::Release); + Ok(()) + } + + fn clone_killer(&self) -> Box { + Box::new(Self { + pid: self.pid, + killed: self.killed.clone(), + }) + } + } + + #[cfg(unix)] + impl portable_pty::Child for CollectedElsewhere { + fn try_wait(&mut self) -> std::io::Result> { + Err(std::io::Error::from_raw_os_error(libc::ECHILD)) + } + + fn wait(&mut self) -> std::io::Result { + Err(std::io::Error::from_raw_os_error(libc::ECHILD)) + } + + fn process_id(&self) -> Option { + Some(self.pid) + } + } + + /// Once something else has collected the shell, its pid may belong to an + /// unrelated process, so ending the shell signals nothing at all. + #[cfg(unix)] + #[test] + fn a_shell_whose_status_cannot_be_read_is_never_signalled() { + // The unrelated process that now holds the shell's old pid. + let mut bystander = std::process::Command::new("sleep") + .arg("30") + .spawn() + .expect("sleep can be spawned"); + let killed = Arc::new(AtomicBool::new(false)); + + end_shell(Box::new(CollectedElsewhere { + pid: bystander.id(), + killed: killed.clone(), + })); + // A SIGHUP sent to the pid would end the bystander within this time. + std::thread::sleep(std::time::Duration::from_millis(200)); + let bystander_alive = bystander + .try_wait() + .expect("the bystander can be polled") + .is_none(); + let _ = bystander.kill(); + let _ = bystander.wait(); + + assert!(!killed.load(Ordering::Acquire), "the shell was force-killed"); + assert!(bystander_alive, "the process holding the pid was signalled"); + } + + #[test] + fn clearing_hands_back_every_terminal_and_forgets_reservations() { + let mut locals = LocalTerminals::default(); + let owns = live(&mut locals, "local-0"); + let _pending = locals.reserve("local-1".to_string()); + + let closed = locals.clear(); + assert_eq!(closed.len(), 1); + assert!(!owns.load(Ordering::Acquire)); + assert_eq!(locals.len(), 0); + } + #[test] fn reaping_an_exited_terminal_hands_its_session_id_back() { let mut locals = LocalTerminals::default(); diff --git a/desktop/src-tauri/src/vault_commands.rs b/desktop/src-tauri/src/vault_commands.rs index c7ff8f08..d7b9edf4 100644 --- a/desktop/src-tauri/src/vault_commands.rs +++ b/desktop/src-tauri/src/vault_commands.rs @@ -39,7 +39,12 @@ async fn contain_open_sessions( // session reachable once the budget lapses. tokio::time::timeout(budget, async { tokio::join!( - async { local_terminals.lock().await.clear() }, + async { + // Dropped once the guard is gone, so a terminal whose teardown + // blocks does not hold the map lock while it does. + let closed = local_terminals.lock().await.clear(); + drop(closed); + }, sftp.close_all(), sessions.close_all(), );