From 6829ac16c2aa49a521c016bbaf9cde013ba78dcf Mon Sep 17 00:00:00 2001 From: Matias Palma Date: Tue, 21 Apr 2026 02:00:30 -0400 Subject: [PATCH] fix(pty): make reader thread joinable so old-session output cannot bleed into the next tab MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `spawn_pty` started a reader thread with `thread::spawn` and dropped the `JoinHandle`. When `PtyState` was replaced (either via `kill_pty` or a second `spawn_pty`), the old reader kept running on its cloned reader until it hit EOF. Anything the old shell had already pushed to the PTY — or any prompt re-drawn by an editor like `vim` as the slave side flushed — surfaced as `pty-output` events inside the new session's tab. Fix: - `PtyState` now owns an `Arc` shutdown flag and an `Option>`. The reader thread checks the flag after every read and bails before emitting if it's set. - `PtyState::shutdown_and_join(self)` flips the flag, drops the `master` (which closes the native handle and unblocks the reader's blocking `read`), then joins the thread. - `spawn_pty` and `kill_pty` both take the old state *out of the mutex first* and only then run `shutdown_and_join`. Joining inside the lock would serialize PTY teardown with every other command and could deadlock on a reader re-entering via `AppHandle::emit`. Deliberately no `Drop` impl: dropping by `&mut self` cannot move `master` out before `handle.join()`, which would deadlock (the reader is parked inside `read` and only master closure wakes it up). Teardown is instead a single explicit path that both callers always go through. No new unit tests — the change is wiring that only manifests against a real PTY. Verified manually: spawn → type → kill_pty → spawn → no stray output in the new session; also with the `vim` / prompt-redraw repro from the issue. Fixes #79 --- apps/desktop/src-tauri/src/pty.rs | 113 +++++++++++++++++++++++++----- 1 file changed, 94 insertions(+), 19 deletions(-) diff --git a/apps/desktop/src-tauri/src/pty.rs b/apps/desktop/src-tauri/src/pty.rs index a80e4beb..db0e009c 100644 --- a/apps/desktop/src-tauri/src/pty.rs +++ b/apps/desktop/src-tauri/src/pty.rs @@ -2,14 +2,55 @@ use log::error; use portable_pty::{native_pty_system, CommandBuilder, PtySize}; use std::{ io::{Read, Write}, - sync::{Arc, Mutex}, - thread, + sync::{ + atomic::{AtomicBool, Ordering}, + Arc, Mutex, + }, + thread::{self, JoinHandle}, }; use tauri::{AppHandle, Emitter, Runtime}; pub struct PtyState { pub writer: Arc>>, pub master: Box, + /// Set by `kill_pty` (or the `Drop` impl) to tell the reader thread it + /// must stop emitting events. The thread checks this flag between + /// reads so bytes still in flight from a previous shell cannot land in + /// the tab of the next one. + shutdown: Arc, + /// Owned handle to the reader thread. Held in an `Option` so + /// `shutdown_and_join` can take it out (joining consumes the handle) + /// without needing `&mut self`. + thread_handle: Option>, +} + +impl PtyState { + /// Flags the reader for shutdown and waits for it to finish. Called + /// explicitly by `spawn_pty` (when replacing an old session) and + /// `kill_pty` (when the user closes the terminal) so the next session + /// cannot receive stale `pty-output` events from the old reader. + /// + /// Both callers always go through this path; there is no implicit + /// `Drop`-based teardown, because `Drop` would have to run on + /// `&mut self` and could not drop `master` before `handle.join()`, + /// which would deadlock (the reader is blocked inside `read`, and + /// only dropping the master wakes it up). + fn shutdown_and_join(self) { + self.shutdown.store(true, Ordering::Release); + // Destructure so we can drop `master` *before* joining. Dropping + // the master closes the native PTY handle and unblocks the + // reader's pending `read` call with `Ok(0)` / `Err` on all + // supported platforms; the join then returns promptly. + let PtyState { + master, + thread_handle, + .. + } = self; + drop(master); + if let Some(handle) = thread_handle { + let _ = handle.join(); + } + } } #[tauri::command] @@ -20,7 +61,18 @@ pub fn spawn_pty( rows: Option, cols: Option, ) -> Result<(), String> { - // If session exists, it will be replaced (dropping old resources kills the previous PTY) + // Take any previous session out of the mutex *before* tearing it down. + // Holding the lock across `shutdown_and_join` would serialize the join + // with every other PTY command and could deadlock if the reader was + // trying to re-enter (e.g. via `emit`). Dropping `old` here flushes + // the old thread to completion with the lock released. + let old = { + let mut guard = state.lock().map_err(|e| e.to_string())?; + guard.take() + }; + if let Some(old_state) = old { + old_state.shutdown_and_join(); + } let pty_system = native_pty_system(); @@ -85,24 +137,25 @@ pub fn spawn_pty( err })?; - let pty_state = PtyState { - writer: Arc::new(Mutex::new(writer)), - master: pair.master, - }; + let shutdown = Arc::new(AtomicBool::new(false)); + let shutdown_for_thread = shutdown.clone(); - *state.lock().map_err(|e| { - let err = e.to_string(); - error!("PTY state lock failed: {}", err); - err - })? = Some(pty_state); - - // Spawn a thread to read from the PTY and emit to the frontend - thread::spawn(move || { + // Spawn a thread to read from the PTY and emit to the frontend. The + // thread owns its cloned reader; when the master is dropped on + // shutdown the native handle closes and the next `read` returns + // `Ok(0)` / `Err`, breaking the loop. + let handle = thread::spawn(move || { let mut buffer = [0u8; 4096]; loop { match reader.read(&mut buffer) { Ok(0) => break, Ok(n) => { + // Observe the shutdown flag *before* emitting so bytes + // read from a shell that the user already killed + // cannot surface inside a new session's tab. + if shutdown_for_thread.load(Ordering::Acquire) { + break; + } let data = String::from_utf8_lossy(&buffer[..n]).to_string(); let _ = app.emit("pty-output", data); } @@ -114,6 +167,19 @@ pub fn spawn_pty( } }); + let pty_state = PtyState { + writer: Arc::new(Mutex::new(writer)), + master: pair.master, + shutdown, + thread_handle: Some(handle), + }; + + *state.lock().map_err(|e| { + let err = e.to_string(); + error!("PTY state lock failed: {}", err); + err + })? = Some(pty_state); + Ok(()) } @@ -153,11 +219,20 @@ pub fn resize_pty( Ok(()) } -/// Kills the currently active PTY session (if any). -/// Dropping the PtyState closes the master PTY handle which signals the shell to exit. +/// Kills the currently active PTY session (if any). Flags the reader +/// thread for shutdown, drops the master to unblock it, and joins the +/// thread before returning so `spawn_pty` can safely reuse the slot. #[tauri::command] pub fn kill_pty(state: tauri::State<'_, Arc>>>) -> Result<(), String> { - let mut guard = state.lock().map_err(|e| e.to_string())?; - *guard = None; // Drops PtyState — closes master PTY and terminates child + // Take the state out of the mutex *before* tearing it down. Joining + // inside the lock would block every other PTY command and could + // deadlock on a reader that re-enters via `emit`. + let taken = { + let mut guard = state.lock().map_err(|e| e.to_string())?; + guard.take() + }; + if let Some(old_state) = taken { + old_state.shutdown_and_join(); + } Ok(()) }