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..748bbc27 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 = { @@ -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/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..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, - /// 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, /// Cleared when this terminal stops holding its session id. /// /// The thread draining the PTY outlives the map entry: it is detached, and @@ -371,6 +380,117 @@ 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 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 +/// 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) + } +} + +/// 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 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. + unsafe { + libc::kill(pid, libc::SIGHUP); + } + } + let deadline = std::time::Instant::now() + SHELL_EXIT_GRACE; + loop { + 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(); +} + +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 +647,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 +669,259 @@ 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 on an async + /// command handler. + #[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"); + // `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; 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 { + 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 and the exec 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); + } + + /// 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] @@ -562,9 +932,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 +943,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 +963,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 +973,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"); } } 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(), );