diff --git a/Cargo.lock b/Cargo.lock index 33c749f3..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", @@ -703,8 +704,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 +1842,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 +3403,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 +5283,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 +5556,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..096f5eb8 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 } @@ -41,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/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..4d6d9df1 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,101 @@ 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 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 +/// 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 + /// 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) + } +} + +/// 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() { + 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(child) = self.child.take() else { + return; + }; + if self.reaped { + return; + } + // 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 = 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); + } + } + } +} + impl LocalTerminal { /// Marks the terminal as no longer holding its session id. pub fn disown(&self) { @@ -527,7 +622,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 +644,108 @@ 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)); + } + } + + /// 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)); + } + } + + /// 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] @@ -562,9 +756,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 +767,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 +787,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 +797,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"); } }