From ee5e140f3275c0ea26b1baf007c1393f0e15dab9 Mon Sep 17 00:00:00 2001 From: Jakub Date: Thu, 23 Jul 2026 12:57:29 +0200 Subject: [PATCH 01/11] fix(logging): give headless/ui-owned its own log file The single-binary daemon (`okena --headless [--ui-owned]`) reuses the same src/main.rs logging init as the GUI, so both processes rotated and wrote the same okena.log, clobbering each other's history. The standalone okena-daemon.log tee only exists in the separate okena-daemon binary, so ui-owned mode produced no daemon log at all. Pick the log filename before the rotate/create block: when headless (or Linux --listen/--remote with no display), write okena-headless.log with its own .1 rotation, leaving the GUI's okena.log legible. --- src/main.rs | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/src/main.rs b/src/main.rs index 869276d9f..c818c2415 100644 --- a/src/main.rs +++ b/src/main.rs @@ -406,8 +406,34 @@ fn main() { }; // SAFETY: called before any threads are spawned; no concurrent reads of the environment. unsafe { std::env::set_var("OKENA_PROFILE", &profile_paths.id) }; - let profile_log = profile_paths.log_path(); - let profile_log_prev = profile_paths.root.join("okena.log.1"); + // Pick the log filename BEFORE rotating/creating it. A single-binary daemon + // (`okena --headless [--ui-owned]`) reuses this same `src/main.rs` logging + // init as the GUI, so if both wrote `okena.log` they would rotate+clobber + // each other's history (and the standalone `okena-daemon.log` tee — which + // only exists in the separate `okena-daemon` binary — is never produced in + // ui-owned mode). Give the headless process its own `okena-headless.log` + // (with its own `.1` rotation) so the GUI's `okena.log` stays legible. This + // mirrors the headless detection performed in full further down (explicit + // `--headless`, or Linux `--listen`/`--remote` with no display); it is + // recomputed here only because logging is initialized before that block. + let log_is_headless = { + let explicit_headless = args.iter().any(|a| a == "--headless"); + let wants_listen = args.iter().any(|a| a == "--listen" || a == "--remote"); + let has_display = + std::env::var("DISPLAY").is_ok() || std::env::var("WAYLAND_DISPLAY").is_ok(); + explicit_headless || (cfg!(target_os = "linux") && wants_listen && !has_display) + }; + let (profile_log, profile_log_prev) = if log_is_headless { + ( + profile_paths.root.join("okena-headless.log"), + profile_paths.root.join("okena-headless.log.1"), + ) + } else { + ( + profile_paths.log_path(), + profile_paths.root.join("okena.log.1"), + ) + }; profiles::init_profile(profile_paths); // Migrate legacy flat-layout state into profiles/default/ if needed. From cbeffa2b0fc00907304d59a4e56a3a2ee79bfe4d Mon Sep 17 00:00:00 2001 From: Jakub Date: Thu, 23 Jul 2026 14:37:20 +0200 Subject: [PATCH 02/11] fix(terminal): avoid blocking teardown on stuck reader --- crates/okena-terminal/src/pty_manager.rs | 285 +++++++++++++++++++++-- 1 file changed, 263 insertions(+), 22 deletions(-) diff --git a/crates/okena-terminal/src/pty_manager.rs b/crates/okena-terminal/src/pty_manager.rs index 9b28b1f95..06731cbbb 100644 --- a/crates/okena-terminal/src/pty_manager.rs +++ b/crates/okena-terminal/src/pty_manager.rs @@ -256,6 +256,18 @@ fn format_panic(payload: &dyn std::any::Any) -> String { } } +fn join_reader_handle(reader_handle: Option>, terminal_id: &str) { + if let Some(handle) = reader_handle + && let Err(error) = handle.join() + { + log::warn!( + "PTY reader thread for {} panicked on join: {}", + terminal_id, + format_panic(&*error) + ); + } +} + /// Number of shared teardown worker threads. Bounds how many PTY teardowns /// (thread joins + `lsof`/`tmux kill-session`/SIGTERM subprocess calls) can run /// concurrently. On bulk shutdown we enqueue N jobs but only this many run at once, @@ -1449,7 +1461,7 @@ impl PtyManager { /// Perform coordinated shutdown of a single PTY handle fn shutdown_handle(mut handle: PtyHandle) { - let id = &handle.shutdown.terminal_id; + let id = handle.shutdown.terminal_id.clone(); // 1. Signal shutdown to threads handle.shutdown.mark_broken(); @@ -1465,28 +1477,57 @@ impl PtyManager { // 4. Drop master - safety net to unblock reader if still stuck drop(handle.master.take()); - // 5. Join writer thread (should exit quickly after input_tx drop) + // 5. Join writer thread (should exit quickly after input_tx drop), then + // close the manager's synchronous-response writer clone. Keeping this + // clone alive while waiting for the child leaves the PTY master open and + // can prevent session clients such as dtach from completing their exit. if let Some(h) = handle.writer_handle.take() && let Err(e) = h.join() { log::warn!("PTY writer thread panicked on join: {}", format_panic(&*e)); } - - // 6. Join reader thread (should exit after child kill + master drop) - if let Some(h) = handle.reader_handle.take() - && let Err(e) = h.join() - { - log::warn!("PTY reader thread panicked on join: {}", format_panic(&*e)); - } - - // 7. Reap the child to prevent a zombie. The reader normally reaps via - // `wait_for_exit_code` on EOF, but that is a bounded `WNOHANG` poll that - // gives up if the SIGKILL'd child is briefly unreapable (e.g. stuck in - // D-state on slow IO). Now that the reader has joined, a blocking wait - // guarantees the PID is reaped. If the reader already reaped it via raw - // `waitpid`, this just returns ECHILD, which is harmless. - if let Err(e) = handle.child.wait() { - log::debug!("PTY child {} already reaped or wait failed: {}", id, e); + drop(handle.writer.take()); + + // 6. Decide whether the child has exited BEFORE joining the reader. A + // child that ignores termination can retain its slave PTY indefinitely, + // which in turn keeps the reader blocked; joining it here would consume + // one of the bounded teardown workers forever. + match handle.child.try_wait() { + Ok(Some(_)) => { + // Exit is confirmed, so EOF should be available after every + // manager-held master clone above was dropped. Joining here keeps + // normal teardown synchronous and catches reader panics. + join_reader_handle(handle.reader_handle.take(), &id); + } + Err(e) => { + // ECHILD commonly means the reader already reaped it. There is + // no child left to wait for, so join the reader normally. + log::debug!("PTY child {} already reaped or wait failed: {}", id, e); + join_reader_handle(handle.reader_handle.take(), &id); + } + Ok(None) => { + // Keep BOTH ownership pieces in a rare detached reaper. It waits + // for child termination first, then joins the reader that EOF + // unblocks, leaving the shared teardown worker available. + let id = id.to_string(); + let short_id = id[..8.min(id.len())].to_string(); + let reaper_id = id.clone(); + if let Err(e) = std::thread::Builder::new() + .name(format!("pty-reaper-{short_id}")) + .spawn(move || { + if let Err(e) = handle.child.wait() { + log::debug!("PTY child {} reaper wait failed: {}", reaper_id, e); + } + join_reader_handle(handle.reader_handle.take(), &reaper_id); + }) + { + // Thread creation failure is exceptional. Dropping the + // handles is still non-blocking and avoids pinning a shared + // teardown worker; the child/reader Drop backstops have + // already been signalled above. + log::warn!("Failed to spawn PTY child reaper for {}: {}", id, e); + } + } } } @@ -2256,6 +2297,202 @@ mod tests { manager.flush_teardown(); } + #[cfg(unix)] + #[derive(Clone, Debug)] + struct DelayedTerminationChild { + release: Arc<(Mutex, Condvar)>, + } + + #[cfg(unix)] + impl portable_pty::ChildKiller for DelayedTerminationChild { + fn kill(&mut self) -> std::io::Result<()> { + Ok(()) + } + + fn clone_killer(&self) -> Box { + Box::new(self.clone()) + } + } + + #[cfg(unix)] + impl Child for DelayedTerminationChild { + fn try_wait(&mut self) -> std::io::Result> { + let released = *self.release.0.lock(); + Ok(released.then(|| portable_pty::ExitStatus::with_exit_code(0))) + } + + fn wait(&mut self) -> std::io::Result { + let mut released = self.release.0.lock(); + while !*released { + self.release.1.wait(&mut released); + } + Ok(portable_pty::ExitStatus::with_exit_code(0)) + } + + fn process_id(&self) -> Option { + None + } + } + + #[cfg(unix)] + #[test] + fn shutdown_does_not_block_on_a_child_or_reader_that_ignores_termination() { + let child_release = Arc::new((Mutex::new(false), Condvar::new())); + let reader_release = Arc::new((Mutex::new(false), Condvar::new())); + let reader_wait = reader_release.clone(); + let (reader_started_tx, reader_started_rx) = mpsc::channel(); + let (reader_done_tx, reader_done_rx) = mpsc::channel(); + let reader_handle = std::thread::spawn(move || { + reader_started_tx.send(()).expect("reader starts"); + let mut released = reader_wait.0.lock(); + while !*released { + reader_wait.1.wait(&mut released); + } + reader_done_tx.send(()).expect("reader finishes"); + }); + reader_started_rx + .recv_timeout(Duration::from_secs(1)) + .expect("reader waits before teardown"); + let handle = PtyHandle { + generation: PtyGeneration(1), + master: None, + child: Box::new(DelayedTerminationChild { + release: child_release.clone(), + }), + input_tx: None, + writer: None, + reader_handle: Some(reader_handle), + writer_handle: None, + shutdown: Arc::new(PtyShutdownState::new( + "non-terminating".to_string(), + PtyGeneration(1), + )), + }; + let (done_tx, done_rx) = mpsc::channel(); + + std::thread::spawn(move || { + PtyManager::shutdown_handle(handle); + let _ = done_tx.send(()); + }); + + done_rx + .recv_timeout(Duration::from_secs(1)) + .expect("PTY teardown must not wait forever for child or reader"); + assert!( + reader_done_rx.try_recv().is_err(), + "reader is still owned by detached reaper until child/reader release" + ); + *child_release.0.lock() = true; + child_release.1.notify_all(); + *reader_release.0.lock() = true; + reader_release.1.notify_all(); + reader_done_rx + .recv_timeout(Duration::from_secs(1)) + .expect("detached reaper joins released reader"); + } + + #[cfg(unix)] + #[test] + fn shutdown_drops_manager_writer_clone_before_waiting_for_reader_eof() { + struct DropNotifyingWriter(Option>); + + impl Write for DropNotifyingWriter { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + Ok(buf.len()) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } + } + + impl Drop for DropNotifyingWriter { + fn drop(&mut self) { + if let Some(sender) = self.0.take() { + let _ = sender.send(()); + } + } + } + + let child_release = Arc::new((Mutex::new(true), Condvar::new())); + let (writer_dropped_tx, writer_dropped_rx) = mpsc::channel(); + let (reader_done_tx, reader_done_rx) = mpsc::channel(); + let reader_handle = std::thread::spawn(move || { + writer_dropped_rx + .recv_timeout(Duration::from_secs(1)) + .expect("manager writer clone is dropped before reader join"); + reader_done_tx.send(()).expect("reader reaches EOF"); + }); + let handle = PtyHandle { + generation: PtyGeneration(1), + master: None, + child: Box::new(DelayedTerminationChild { + release: child_release, + }), + input_tx: None, + writer: Some(Arc::new(Mutex::new(Box::new(DropNotifyingWriter(Some( + writer_dropped_tx, + )))))), + reader_handle: Some(reader_handle), + writer_handle: None, + shutdown: Arc::new(PtyShutdownState::new( + "writer-clone".to_string(), + PtyGeneration(1), + )), + }; + + PtyManager::shutdown_handle(handle); + reader_done_rx + .recv_timeout(Duration::from_secs(1)) + .expect("reader joins after observing writer clone closure"); + } + + #[cfg(unix)] + #[test] + fn dtach_kill_does_not_stall_teardown_flush() { + if SessionBackend::Dtach.resolve() != ResolvedBackend::Dtach { + return; + } + + let (manager, _events) = PtyManager::new(SessionBackend::Dtach); + let cwd = std::env::temp_dir().to_string_lossy().into_owned(); + let plan = TerminalLaunchPlan { + route: ShellType::Default, + initial_command: Some(crate::backend::TerminalLaunchCommand { + program: "/bin/sh".to_string(), + args: vec!["-c".to_string(), "sleep 30".to_string()], + }), + environment: Vec::new(), + }; + let terminal_id = manager + .create_terminal_with_plan(&cwd, &plan) + .expect("create dtach-backed PTY"); + let session_name = ResolvedBackend::Dtach.session_name(&terminal_id); + let socket_path = ResolvedBackend::Dtach + .socket_path(&session_name) + .expect("dtach socket path"); + let socket_deadline = std::time::Instant::now() + Duration::from_secs(2); + while !socket_path.exists() && std::time::Instant::now() < socket_deadline { + std::thread::sleep(Duration::from_millis(10)); + } + assert!(socket_path.exists(), "dtach session socket was not created"); + + manager.kill(&terminal_id); + let (done_tx, done_rx) = mpsc::channel(); + std::thread::spawn(move || { + manager.flush_teardown(); + let _ = done_tx.send(()); + }); + + done_rx + .recv_timeout(Duration::from_secs(3)) + .expect("dtach teardown flush must complete"); + assert!( + !socket_path.exists(), + "dtach session socket must be removed" + ); + } + #[derive(Default)] struct RecordingSink { published: Mutex)>>, @@ -2301,18 +2538,22 @@ mod tests { let (done_tx, done_rx) = mpsc::channel(); let waiter_tracker = Arc::clone(&tracker); let waiter = std::thread::spawn(move || { - started_tx.send(()).unwrap(); + started_tx + .send(()) + .expect("announce teardown flush waiter start"); waiter_tracker.flush(); - done_tx.send(()).unwrap(); + done_tx + .send(()) + .expect("announce teardown flush waiter completion"); }); - started_rx.recv().unwrap(); + started_rx.recv().expect("teardown flush waiter must start"); assert!(matches!(done_rx.try_recv(), Err(mpsc::TryRecvError::Empty))); tracker.completed(); done_rx .recv_timeout(Duration::from_secs(1)) .expect("flush returns after completion"); - waiter.join().unwrap(); + waiter.join().expect("teardown flush waiter must join"); } #[test] From f0bb33b70459f7a5f0c579a6f813803e65deaa2a Mon Sep 17 00:00:00 2001 From: Jakub Date: Thu, 23 Jul 2026 14:44:09 +0200 Subject: [PATCH 03/11] fix(daemon): recover vanished worktree close hooks --- crates/okena-daemon-core/src/daemon.rs | 10 + crates/okena-daemon-core/src/lib.rs | 1 + crates/okena-daemon-core/src/pty_loop.rs | 252 +++++++++++++++++- .../src/worktree_close_watchdog.rs | 239 +++++++++++++++++ crates/okena-workspace/src/lifecycle.rs | 19 ++ crates/okena-workspace/src/state.rs | 98 +++++++ 6 files changed, 613 insertions(+), 6 deletions(-) create mode 100644 crates/okena-daemon-core/src/worktree_close_watchdog.rs diff --git a/crates/okena-daemon-core/src/daemon.rs b/crates/okena-daemon-core/src/daemon.rs index 864530fb4..da40f0171 100644 --- a/crates/okena-daemon-core/src/daemon.rs +++ b/crates/okena-daemon-core/src/daemon.rs @@ -473,6 +473,16 @@ impl DaemonCore { reactor.hook_monitor.clone(), soft_close_deadlines.clone(), )); + // Missing-PTY reconciliation deliberately has no hook-duration + // timeout: it only aborts a pending worktree close after the PTY + // manager itself no longer owns that hook terminal. + tokio::task::spawn_local(crate::worktree_close_watchdog::run_worktree_close_watchdog( + reactor.workspace.clone(), + pty_manager.clone(), + reactor.workspace_tick.clone(), + reactor.hook_runner.clone(), + reactor.hook_monitor.clone(), + )); // The command loop is the "main" task; it runs until the bridge // closes. Race it against ctrl-c so the daemon can shut down cleanly. diff --git a/crates/okena-daemon-core/src/lib.rs b/crates/okena-daemon-core/src/lib.rs index 8609770a2..4897d0c21 100644 --- a/crates/okena-daemon-core/src/lib.rs +++ b/crates/okena-daemon-core/src/lib.rs @@ -40,6 +40,7 @@ pub mod service_cx; pub mod soft_close; pub mod toast_poll; pub mod workspace_cx; +pub mod worktree_close_watchdog; #[cfg(test)] mod test_support; diff --git a/crates/okena-daemon-core/src/pty_loop.rs b/crates/okena-daemon-core/src/pty_loop.rs index 4cc9015dd..5d0476cb8 100644 --- a/crates/okena-daemon-core/src/pty_loop.rs +++ b/crates/okena-daemon-core/src/pty_loop.rs @@ -181,7 +181,22 @@ pub async fn run_pty_loop( // independent of any PTY `Exit`. Mirror the GUI's post-batch dirty-title // scan. (Runs whether or not there were exits.) if !dirty_terminal_ids.is_empty() { - process_osc_hook_exits(&dirty_terminal_ids, &terminals, &reactor); + let osc_hook_exits = process_osc_hook_exits(&dirty_terminal_ids, &terminals, &reactor); + if !osc_hook_exits.is_empty() { + resolve_osc_worktree_closes( + &osc_hook_exits, + &terminals, + &pty_manager, + &service_manager, + &service_tick, + &runtime, + &reactor, + ); + // The workspace tick carries the authoritative mutation to the + // state observer, but bump the coarse version here too so a + // client resync is not delayed behind another PTY event. + state_version.send_modify(|v| *v += 1); + } // Activity edges — OSC 133 ;D command-finish, bell, and OSC 9/777 // notification — stamp `last_activity_at` on the owning project so the // activity-sorted sidebar floats it up. Bump `state_version` if @@ -262,16 +277,16 @@ fn process_event( /// Hook-exit-via-OSC-title: for any terminal that produced output this batch and /// IS a hook terminal, if its title is `__okena_hook_exit:`, set the hook -/// status and HookMonitor execution to Succeeded (code 0) / Failed otherwise. +/// status and HookMonitor execution to Succeeded (code 0) / Failed otherwise, +/// and return its authoritative result for pending worktree-close resolution. /// -/// Mirrors the GUI's post-batch dirty-title scan (`app/mod.rs`). This happens for -/// keep-alive hooks whose command finished but whose PTY stays alive as an -/// interactive shell, so there is no PTY `Exit` to drive the status. +/// This happens for keep-alive hooks whose command finished but whose PTY stays +/// alive as an interactive shell, so there is no PTY `Exit` to drive completion. fn process_osc_hook_exits( dirty_terminal_ids: &[String], terminals: &TerminalsRegistry, reactor: &PtyLoopReactor, -) { +) -> Vec<(String, i32)> { // Collect status updates under the registry + workspace read locks, then // apply them under a single workspace write lock (matching the GUI's split). let mut status_updates: Vec<(String, HookTerminalStatus, Option)> = Vec::new(); @@ -296,6 +311,7 @@ fn process_osc_hook_exits( } } } + let mut results = Vec::with_capacity(status_updates.len()); if !status_updates.is_empty() { let mut cx = reactor.workspace_cx(); let mut ws = reactor.workspace.lock(); @@ -303,7 +319,88 @@ fn process_osc_hook_exits( if let Some(monitor) = reactor.hook_monitor.as_ref() { monitor.finish_by_terminal_id(&tid, exit_code); } + let code = match &status { + HookTerminalStatus::Succeeded => 0, + HookTerminalStatus::Failed { exit_code } => *exit_code, + HookTerminalStatus::Running => unreachable!("OSC produces a completed hook status"), + }; ws.update_hook_terminal_status(&tid, status, &mut cx); + results.push((tid, code)); + } + } + results +} + +/// Resolve before-remove hooks that reported an authoritative result through +/// OSC while their PTY remains alive. The pending map is the exactly-once claim: +/// a late PTY Exit or repeated title observes no pending entry and cannot delete +/// a project or overwrite the completed hook state. +#[allow(clippy::too_many_arguments)] +fn resolve_osc_worktree_closes( + osc_hook_exits: &[(String, i32)], + terminals: &TerminalsRegistry, + pty_manager: &PtyManager, + service_manager: &Arc>, + service_tick: &watch::Sender, + runtime: &Handle, + reactor: &PtyLoopReactor, +) { + let global_hooks = reactor.settings.lock().hooks.clone(); + for (terminal_id, exit_code) in osc_hook_exits { + let mut cx = reactor.workspace_cx(); + let mut ws = reactor.workspace.lock(); + let Some(pending) = ws.take_pending_worktree_close(terminal_id) else { + continue; + }; + + if *exit_code == 0 { + // A keep-alive shell can retain the worktree CWD after reporting + // success. Tear down its PTY before starting the canonical removal; + // unlike the watchdog this result is authoritative, not inferred. + ws.remove_hook_terminal(terminal_id, &mut cx); + match ws.begin_worktree_removal(&pending.project_id, &global_hooks, &mut cx) { + Ok(plan) => { + let operation_epoch = ws.data_replacement_epoch(); + drop(ws); + pty_manager.kill(terminal_id); + terminals.lock().remove(terminal_id); + let _ = crate::command_loop::spawn_background_worktree_removal( + plan, + operation_epoch, + false, + &global_hooks, + &reactor.workspace, + &reactor.workspace_tick, + &reactor.hook_runner, + &reactor.hook_monitor, + &reactor.backend, + terminals, + &reactor.settings, + service_manager, + service_tick, + runtime, + ); + } + Err(error) => { + let project_name = ws + .project(&pending.project_id) + .map(|project| project.name.clone()) + .unwrap_or_else(|| pending.project_id.clone()); + ws.finish_closing_project(&pending.project_id); + cx.notify(); + if let Some(monitor) = reactor.hook_monitor.as_ref() { + monitor.push_toast(okena_state::Toast::error(format!( + "\"{project_name}\" was not closed: {error}" + ))); + } + } + } + } else { + ws.finish_closing_project(&pending.project_id); + cx.notify(); + // `process_osc_hook_exits` already completed the HookMonitor with + // this authoritative nonzero code, which queues its single failure + // toast. Do not enqueue a second toast for the same hook result. } } } @@ -1085,6 +1182,76 @@ mod tests { assert_eq!(monitor.drain_pending_toasts().len(), 1); } + #[tokio::test] + async fn nonzero_osc_hook_exit_aborts_pending_worktree_close_once() { + let repo = std::env::temp_dir().join("okena-osc-hook-failure-main"); + let worktree = std::env::temp_dir().join("okena-osc-hook-failure-worktree"); + let reactor = test_reactor( + workspace_with_pending_close(&repo, &worktree, "hook-osc"), + AppSettings::default(), + ); + let monitor = reactor.hook_monitor.clone().expect("hook monitor"); + monitor.record_start( + "before_worktree_remove", + "exit 7", + "Feature", + Some("hook-osc".into()), + ); + let terminals: TerminalsRegistry = Arc::new(Mutex::new(Default::default())); + let terminal = Arc::new(Terminal::new( + "hook-osc".into(), + terminal_size(), + reactor.backend.transport(), + worktree.to_string_lossy().into_owned(), + )); + terminal.process_output(b"\x1b]0;__okena_hook_exit:7\x07"); + terminals.lock().insert("hook-osc".into(), terminal); + let osc_results = process_osc_hook_exits(&["hook-osc".into()], &terminals, &reactor); + assert_eq!(osc_results, vec![("hook-osc".into(), 7)]); + + let (pty_manager, _events) = PtyManager::new(SessionBackend::None); + let services = Arc::new(Mutex::new(ServiceManager::new( + reactor.backend.clone(), + terminals.clone(), + ))); + let (service_tick, _service_rx) = watch::channel(0u64); + resolve_osc_worktree_closes( + &osc_results, + &terminals, + &pty_manager, + &services, + &service_tick, + &Handle::current(), + &reactor, + ); + let workspace = reactor.workspace.lock(); + let project = workspace + .project("wt1") + .expect("failed hook retains worktree"); + assert!(!workspace.is_project_closing("wt1")); + assert!(!project.is_closing); + assert!(matches!( + project.hook_terminals["hook-osc"].status, + HookTerminalStatus::Failed { exit_code: 7 } + )); + drop(workspace); + assert_eq!(monitor.drain_pending_toasts().len(), 1); + + resolve_osc_worktree_closes( + &osc_results, + &terminals, + &pty_manager, + &services, + &service_tick, + &Handle::current(), + &reactor, + ); + assert!( + monitor.drain_pending_toasts().is_empty(), + "late OSC is a no-op" + ); + } + async fn drive_hook_exit_through_pty_loop( reactor: PtyLoopReactor, terminals: TerminalsRegistry, @@ -1244,6 +1411,79 @@ mod tests { std::fs::remove_dir_all(repo).ok(); } + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn successful_osc_hook_exit_removes_worktree_once() { + let (repo, worktree) = real_git_worktree(); + let (pty_manager, _events) = PtyManager::new(SessionBackend::None); + let hook_terminal_id = pty_manager + .create_terminal_with_shell( + worktree.to_str().expect("utf-8 worktree path"), + Some(&ShellType::for_command("sleep 30".to_string())), + ) + .expect("create keep-alive before-remove hook PTY"); + let pty_manager = Arc::new(pty_manager); + let reactor = test_reactor_with_manager( + workspace_with_pending_close(&repo, &worktree, &hook_terminal_id), + AppSettings::default(), + pty_manager.clone(), + ); + let workspace = reactor.workspace.clone(); + let terminals: TerminalsRegistry = Arc::new(Mutex::new(Default::default())); + let terminal = Arc::new(Terminal::new( + hook_terminal_id.clone(), + terminal_size(), + pty_manager.clone(), + worktree.to_string_lossy().into_owned(), + )); + terminal.process_output(b"\x1b]0;__okena_hook_exit:0\x07"); + terminals.lock().insert(hook_terminal_id.clone(), terminal); + let osc_results = process_osc_hook_exits( + std::slice::from_ref(&hook_terminal_id), + &terminals, + &reactor, + ); + assert_eq!(osc_results, vec![(hook_terminal_id.clone(), 0)]); + let service_manager = Arc::new(Mutex::new(ServiceManager::new( + reactor.backend.clone(), + terminals.clone(), + ))); + let (service_tick, _service_rx) = watch::channel(0u64); + let local = tokio::task::LocalSet::new(); + local + .run_until(async { + resolve_osc_worktree_closes( + &osc_results, + &terminals, + &pty_manager, + &service_manager, + &service_tick, + &Handle::current(), + &reactor, + ); + tokio::time::timeout(Duration::from_secs(3), async { + while workspace.lock().project("wt1").is_some() { + tokio::time::sleep(Duration::from_millis(10)).await; + } + }) + .await + .expect("OSC success removes worktree through canonical path"); + resolve_osc_worktree_closes( + &osc_results, + &terminals, + &pty_manager, + &service_manager, + &service_tick, + &Handle::current(), + &reactor, + ); + }) + .await; + + assert!(!worktree.exists(), "checkout was physically removed once"); + assert!(workspace.lock().project("wt1").is_none()); + std::fs::remove_dir_all(repo).ok(); + } + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn failed_hook_pty_exit_aborts_pending_close() { let repo = std::env::temp_dir().join("okena-hook-failure-main"); diff --git a/crates/okena-daemon-core/src/worktree_close_watchdog.rs b/crates/okena-daemon-core/src/worktree_close_watchdog.rs new file mode 100644 index 000000000..f7e7cbb83 --- /dev/null +++ b/crates/okena-daemon-core/src/worktree_close_watchdog.rs @@ -0,0 +1,239 @@ +//! Daemon-side reconciliation for worktree closes whose before-remove hook PTY +//! disappeared without an authoritative exit event. +//! +//! A current PTY generation is the only liveness signal. There is deliberately +//! no elapsed-time policy: a hook is allowed to run indefinitely until it exits, +//! reports an authoritative result, or its PTY actually vanishes. + +use std::sync::Arc; +use std::time::Duration; + +use okena_hooks::{HookMonitor, HookRunner}; +use okena_terminal::pty_manager::PtyManager; +use okena_workspace::state::Workspace; +use parking_lot::Mutex; +use tokio::sync::watch; + +use crate::workspace_cx::DaemonWorkspaceCx; + +const POLL_INTERVAL: Duration = Duration::from_millis(200); + +/// Periodically abort worktree closes whose hook PTY has disappeared. This is a +/// liveness reconciler, not a hook timeout: live PTYs remain pending forever. +pub async fn run_worktree_close_watchdog( + workspace: Arc>, + pty_manager: Arc, + workspace_tick: watch::Sender, + hook_runner: Option, + hook_monitor: Option, +) { + loop { + tokio::time::sleep(POLL_INTERVAL).await; + reconcile_orphaned_worktree_closes_once( + &workspace, + &pty_manager, + &workspace_tick, + &hook_runner, + &hook_monitor, + ); + } +} + +/// Run one orphan reconciliation pass. Returns the number of atomically claimed +/// pending closes. A candidate can safely disappear between snapshot and claim: +/// the workspace operation is idempotent and loses races to normal exits. +pub fn reconcile_orphaned_worktree_closes_once( + workspace: &Arc>, + pty_manager: &PtyManager, + workspace_tick: &watch::Sender, + hook_runner: &Option, + hook_monitor: &Option, +) -> usize { + let candidates = workspace.lock().pending_worktree_close_terminal_ids(); + let mut reconciled = 0; + + for terminal_id in candidates { + // Do not use the terminals registry and never apply an age deadline: + // only PTY ownership is authoritative for liveness. + if pty_manager.current_generation(&terminal_id).is_some() { + continue; + } + + let aborted = { + let mut cx = DaemonWorkspaceCx::new(workspace_tick, hook_runner, hook_monitor); + let mut ws = workspace.lock(); + ws.abort_orphaned_worktree_close(&terminal_id, &mut cx) + }; + let Some(aborted) = aborted else { + continue; + }; + + if let Some(monitor) = hook_monitor { + // `None` records the unknown exit as a failure, decrements the + // running count, and queues the hook-failure toast exactly once. + monitor.finish_by_terminal_id(&terminal_id, None); + } + log::error!( + "worktree-close: before-remove hook PTY disappeared; close aborted for \"{}\" ({}) terminal {}", + aborted.project_name, + aborted.project_id, + terminal_id + ); + reconciled += 1; + } + + reconciled +} + +#[cfg(test)] +mod tests { + use super::*; + use okena_hooks::HookStatus; + use okena_state::{HookTerminalEntry, HookTerminalStatus, ProjectData, WorkspaceData}; + use okena_terminal::session_backend::SessionBackend; + use std::collections::HashMap; + + fn workspace_with_pending_close(terminal_id: &str) -> Workspace { + let mut project = ProjectData { + id: "worktree".into(), + name: "Feature".into(), + path: std::env::temp_dir().to_string_lossy().into_owned(), + layout: None, + terminal_names: HashMap::from([(terminal_id.into(), "Before remove".into())]), + hidden_terminals: Default::default(), + worktree_info: None, + worktree_ids: Vec::new(), + folder_color: Default::default(), + hooks: Default::default(), + is_remote: false, + connection_id: None, + service_terminals: Default::default(), + default_shell: None, + hook_terminals: Default::default(), + pinned: false, + last_activity_at: None, + is_creating: false, + is_closing: false, + }; + project.hook_terminals.insert( + terminal_id.into(), + HookTerminalEntry { + label: "Before remove".into(), + status: HookTerminalStatus::Running, + hook_type: "before_worktree_remove".into(), + command: "true".into(), + cwd: project.path.clone(), + }, + ); + let mut workspace = Workspace::new(WorkspaceData { + version: 1, + projects: vec![project], + project_order: vec!["worktree".into()], + folders: Vec::new(), + service_panel_heights: Default::default(), + hook_panel_heights: Default::default(), + main_window: Default::default(), + extra_windows: Vec::new(), + }); + workspace.register_pending_worktree_close(okena_state::PendingWorktreeClose { + project_id: "worktree".into(), + hook_terminal_id: terminal_id.into(), + branch: "feature".into(), + main_repo_path: std::env::temp_dir().to_string_lossy().into_owned(), + }); + workspace + } + + #[test] + fn vanished_hook_aborts_close_and_heals_monitor_once() { + let workspace = Arc::new(Mutex::new(workspace_with_pending_close("hook-1"))); + let (manager, _events) = PtyManager::new(SessionBackend::None); + let (tick, tick_rx) = watch::channel(0u64); + let monitor = HookMonitor::new(); + monitor.record_start( + "before_worktree_remove", + "true", + "Feature", + Some("hook-1".into()), + ); + + assert_eq!( + reconcile_orphaned_worktree_closes_once( + &workspace, + &manager, + &tick, + &None, + &Some(monitor.clone()), + ), + 1 + ); + assert!(tick_rx.has_changed().expect("tick remains open")); + let ws = workspace.lock(); + let project = ws.project("worktree").expect("project retained"); + assert!(!ws.is_project_closing("worktree")); + assert!(!project.is_closing); + assert!(matches!( + project.hook_terminals["hook-1"].status, + HookTerminalStatus::Failed { exit_code: -1 } + )); + drop(ws); + assert!(matches!( + monitor.history()[0].status, + HookStatus::Failed { exit_code: -1, .. } + )); + assert_eq!(monitor.drain_pending_toasts().len(), 1); + assert_eq!( + reconcile_orphaned_worktree_closes_once( + &workspace, + &manager, + &tick, + &None, + &Some(monitor.clone()), + ), + 0, + "late/duplicate reconciliation must not rewrite status or toast" + ); + assert!(monitor.drain_pending_toasts().is_empty()); + } + + #[test] + fn live_hook_never_times_out() { + let workspace = Arc::new(Mutex::new(workspace_with_pending_close("hook-live"))); + let (manager, _events) = PtyManager::new(SessionBackend::None); + let cwd = std::env::temp_dir().to_string_lossy().into_owned(); + manager + .create_or_reconnect_terminal(Some("hook-live"), &cwd) + .expect("create current PTY"); + let (tick, _tick_rx) = watch::channel(0u64); + let monitor = HookMonitor::new(); + monitor.record_start( + "before_worktree_remove", + "sleep 60", + "Feature", + Some("hook-live".into()), + ); + + for _ in 0..3 { + assert_eq!( + reconcile_orphaned_worktree_closes_once( + &workspace, + &manager, + &tick, + &None, + &Some(monitor.clone()), + ), + 0 + ); + } + let ws = workspace.lock(); + assert!(ws.is_project_closing("worktree")); + assert!(ws.project("worktree").unwrap().is_closing); + assert!(matches!( + ws.project("worktree").unwrap().hook_terminals["hook-live"].status, + HookTerminalStatus::Running + )); + drop(ws); + assert!(matches!(monitor.history()[0].status, HookStatus::Running)); + manager.kill("hook-live"); + } +} diff --git a/crates/okena-workspace/src/lifecycle.rs b/crates/okena-workspace/src/lifecycle.rs index 3c845a107..ed47e9605 100644 --- a/crates/okena-workspace/src/lifecycle.rs +++ b/crates/okena-workspace/src/lifecycle.rs @@ -132,6 +132,13 @@ impl ProjectLifecycleTracker { .insert(pending.hook_terminal_id.clone(), pending); } + /// Snapshot hook terminal IDs with a worktree close awaiting authoritative + /// completion. Callers must still claim a particular ID through + /// [`Self::cancel_pending_close`] because the snapshot can become stale. + pub fn pending_close_terminal_ids(&self) -> Vec { + self.pending_worktree_closes.keys().cloned().collect() + } + /// Take a pending worktree close for the given hook terminal ID (removes it). pub fn take_pending_close(&mut self, hook_terminal_id: &str) -> Option { self.pending_worktree_closes.remove(hook_terminal_id) @@ -214,6 +221,18 @@ mod tests { assert!(!tracker.is_closing("p1")); } + #[test] + fn pending_close_terminal_ids_is_a_snapshot() { + let mut tracker = ProjectLifecycleTracker::new(); + tracker.register_pending_close(pending("p1", "hook1")); + tracker.register_pending_close(pending("p2", "hook2")); + let mut ids = tracker.pending_close_terminal_ids(); + ids.sort(); + assert_eq!(ids, ["hook1", "hook2"]); + tracker.cancel_pending_close("hook1"); + assert_eq!(ids, ["hook1", "hook2"], "snapshot remains independent"); + } + #[test] fn runtime_quiesce_claims_are_atomic_and_generation_fenced() { let mut tracker = ProjectLifecycleTracker::new(); diff --git a/crates/okena-workspace/src/state.rs b/crates/okena-workspace/src/state.rs index ba9cea967..0ffbed6a0 100644 --- a/crates/okena-workspace/src/state.rs +++ b/crates/okena-workspace/src/state.rs @@ -28,6 +28,13 @@ pub use okena_state::{ WorkspaceData, WorktreeMetadata, }; +/// Diagnostics returned after atomically aborting a vanished before-remove hook. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct AbortedWorktreeClose { + pub project_id: String, + pub project_name: String, +} + #[derive(Clone, Debug, PartialEq, Eq)] enum FilesystemObjectIdentity { #[cfg(unix)] @@ -1756,6 +1763,48 @@ impl Workspace { } } + /// Snapshot before-remove hook terminal IDs awaiting authoritative completion. + /// + /// The returned IDs are only candidates; callers must use + /// [`Self::abort_orphaned_worktree_close`] to atomically claim an orphan. + pub fn pending_worktree_close_terminal_ids(&self) -> Vec { + self.lifecycle.pending_close_terminal_ids() + } + + /// Abort a pending close whose before-remove hook PTY vanished without an + /// authoritative exit result. This is intentionally state-only: it retains + /// the project and worktree, does not run removal hooks, and is idempotent. + /// + /// The lifecycle record, in-memory closing marker, wire-facing closing flag, + /// and still-running hook entry are healed in one workspace mutation. A + /// caller that sees `None` lost the race to a normal exit, rerun, data + /// replacement, or another watchdog pass. + pub fn abort_orphaned_worktree_close( + &mut self, + terminal_id: &str, + cx: &mut impl WorkspaceCx, + ) -> Option { + let project_id = self.lifecycle.cancel_pending_close(terminal_id)?; + let project = self.data.projects.iter_mut().find(|p| p.id == project_id); + let project_name = project + .as_ref() + .map(|project| project.name.clone()) + .unwrap_or_else(|| project_id.clone()); + if let Some(project) = project { + project.is_closing = false; + if let Some(entry) = project.hook_terminals.get_mut(terminal_id) + && entry.status == HookTerminalStatus::Running + { + entry.status = HookTerminalStatus::Failed { exit_code: -1 }; + } + } + cx.notify(); + Some(AbortedWorktreeClose { + project_id, + project_name, + }) + } + /// Check if a project is currently being closed (hook running or removal in progress). pub fn is_project_closing(&self, project_id: &str) -> bool { self.lifecycle.is_closing(project_id) @@ -2102,6 +2151,55 @@ mod workspace_tests { } } + #[test] + fn orphaned_worktree_close_aborts_atomically_and_idempotently() { + let mut project = make_project("wt1"); + project.name = "Feature".into(); + project.hook_terminals.insert( + "hook-1".into(), + HookTerminalEntry { + label: "Before remove".into(), + status: HookTerminalStatus::Running, + hook_type: "before_worktree_remove".into(), + command: "true".into(), + cwd: "/tmp".into(), + }, + ); + let mut workspace = Workspace::new(make_workspace_data(vec![project], vec!["wt1"])); + workspace.register_pending_worktree_close(crate::state::PendingWorktreeClose { + project_id: "wt1".into(), + hook_terminal_id: "hook-1".into(), + branch: "feature".into(), + main_repo_path: "/tmp".into(), + }); + assert!(workspace.is_project_closing("wt1")); + assert!(workspace.project("wt1").unwrap().is_closing); + + let mut cx = RecordingCx::default(); + assert_eq!( + workspace.abort_orphaned_worktree_close("hook-1", &mut cx), + Some(crate::state::AbortedWorktreeClose { + project_id: "wt1".into(), + project_name: "Feature".into(), + }) + ); + assert!(!workspace.is_project_closing("wt1")); + let project = workspace.project("wt1").expect("project retained"); + assert!(!project.is_closing); + assert!(matches!( + project.hook_terminals["hook-1"].status, + HookTerminalStatus::Failed { exit_code: -1 } + )); + assert_eq!(cx.notifications, 1); + assert!(workspace.pending_worktree_close_terminal_ids().is_empty()); + assert!( + workspace + .abort_orphaned_worktree_close("hook-1", &mut cx) + .is_none() + ); + assert_eq!(cx.notifications, 1, "second claim is a no-op"); + } + #[test] fn terminal_backend_migration_gate_is_exclusive_and_epoch_fenced() { let mut workspace = Workspace::new(WorkspaceData::empty()); From 302d0c14308c88f0b9c9c35cc459c7727959e35c Mon Sep 17 00:00:00 2001 From: Jakub Date: Thu, 23 Jul 2026 15:11:51 +0200 Subject: [PATCH 04/11] fix(worktree): reconcile close teardown recovery --- crates/okena-daemon-core/src/command_loop.rs | 14 +- .../src/worktree_close_watchdog.rs | 21 ++- crates/okena-git/src/repository/worktree.rs | 119 ++++++++++++++--- crates/okena-terminal/src/backend.rs | 11 ++ crates/okena-terminal/src/pty_manager.rs | 120 ++++++++++++++---- crates/okena-terminal/src/session_backend.rs | 63 ++++++++- src/main.rs | 74 ++++++++++- 7 files changed, 374 insertions(+), 48 deletions(-) diff --git a/crates/okena-daemon-core/src/command_loop.rs b/crates/okena-daemon-core/src/command_loop.rs index db94a7d93..295fce669 100644 --- a/crates/okena-daemon-core/src/command_loop.rs +++ b/crates/okena-daemon-core/src/command_loop.rs @@ -39,6 +39,7 @@ use std::collections::{HashMap, HashSet}; use std::path::PathBuf; use std::sync::Arc; +use std::time::Duration; use okena_app_core::remote_snapshot::build_state_response; #[cfg(test)] @@ -1901,9 +1902,16 @@ pub(crate) fn spawn_background_worktree_removal( let monitor = hook_monitor.clone(); let teardown_backend = backend.clone(); let outcome = tokio::task::spawn_blocking(move || { - // `kill` is asynchronous for local PTYs. Wait off-reactor until the - // queued handles and persistent sessions release their checkout CWD. - teardown_backend.flush_teardown(); + // `kill` is asynchronous for local PTYs. Do not race destructive + // removal with a process that may still own the checkout CWD: a + // bounded failure restores the project instead of deleting it. + if !teardown_backend.flush_teardown_with_timeout(Duration::from_secs(5)) { + return ( + plan, + Err("terminal teardown did not release the worktree in time; checkout preserved".to_string()), + None, + ); + } let worktree_path = plan.worktree_path.clone(); // force_remove = is_dirty && !did_stash — same condition the sync // close_worktree path uses to fire the dirty-close safety net. Runs diff --git a/crates/okena-daemon-core/src/worktree_close_watchdog.rs b/crates/okena-daemon-core/src/worktree_close_watchdog.rs index f7e7cbb83..f967d8ed8 100644 --- a/crates/okena-daemon-core/src/worktree_close_watchdog.rs +++ b/crates/okena-daemon-core/src/worktree_close_watchdog.rs @@ -145,9 +145,16 @@ mod tests { } #[test] - fn vanished_hook_aborts_close_and_heals_monitor_once() { + fn vanished_hook_pty_aborts_close_heals_once_and_allows_immediate_retry() { let workspace = Arc::new(Mutex::new(workspace_with_pending_close("hook-1"))); let (manager, _events) = PtyManager::new(SessionBackend::None); + manager + .create_or_reconnect_terminal(Some("hook-1"), &std::env::temp_dir().to_string_lossy()) + .expect("create hook PTY"); + // Deliberately do not dispatch a PtyEvent::Exit: this reproduces the + // daemon teardown race where the hook PTY disappears first. + manager.kill("hook-1"); + manager.flush_teardown(); let (tick, tick_rx) = watch::channel(0u64); let monitor = HookMonitor::new(); monitor.record_start( @@ -194,6 +201,18 @@ mod tests { "late/duplicate reconciliation must not rewrite status or toast" ); assert!(monitor.drain_pending_toasts().is_empty()); + workspace + .lock() + .register_pending_worktree_close(okena_state::PendingWorktreeClose { + project_id: "worktree".into(), + hook_terminal_id: "hook-retry".into(), + branch: "feature".into(), + main_repo_path: std::env::temp_dir().to_string_lossy().into_owned(), + }); + assert!( + workspace.lock().is_project_closing("worktree"), + "retry can claim close immediately" + ); } #[test] diff --git a/crates/okena-git/src/repository/worktree.rs b/crates/okena-git/src/repository/worktree.rs index d27474b03..d3f1b41c5 100644 --- a/crates/okena-git/src/repository/worktree.rs +++ b/crates/okena-git/src/repository/worktree.rs @@ -151,6 +151,56 @@ fn revalidate_verified_worktree(verified: &VerifiedWorktree) -> GitResult<()> { Ok(()) } +/// Remove only a directory that is absent, empty, or contains regular +/// `.DS_Store` files. This handles Finder metadata recreated after the verified +/// checkout was quarantined without ever deleting a replacement directory. +fn remove_benign_residual(path: &Path) -> std::io::Result { + let entries = match std::fs::read_dir(path) { + Ok(entries) => entries, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(true), + Err(error) => return Err(error), + }; + + let mut ds_store_files = Vec::new(); + for entry in entries { + let entry = entry?; + let file_type = entry.file_type()?; + if entry.file_name() != ".DS_Store" || !file_type.is_file() || file_type.is_symlink() { + return Ok(false); + } + ds_store_files.push(entry.path()); + } + for ds_store in ds_store_files { + match std::fs::remove_file(ds_store) { + Ok(()) => {} + Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} + Err(error) => return Err(error), + } + } + match std::fs::remove_dir(path) { + Ok(()) => Ok(true), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(true), + // A concurrent Finder write is harmless only if a subsequent inspection + // again proves the residual is exclusively benign metadata. + Err(error) if error.kind() == std::io::ErrorKind::DirectoryNotEmpty => Ok(false), + Err(error) => Err(error), + } +} + +fn cleanup_benign_residual(path: &Path) -> GitResult<()> { + match remove_benign_residual(path) { + Ok(true) => Ok(()), + Ok(false) => Err(unsafe_worktree( + path, + "checkout path was recreated with non-benign content; preserved it", + )), + Err(source) => Err(GitError::RemoveFailed { + path: path.to_path_buf(), + source, + }), + } +} + /// Refuse every existing target. An unregistered directory is not proof that /// Okena owns its contents, so create must never remove it speculatively. fn require_absent_worktree_target(target_path: &Path) -> GitResult<()> { @@ -333,25 +383,37 @@ pub fn remove_worktree_fast(verified: &VerifiedWorktree) -> GitResult<()> { match std::fs::remove_dir_all(&quarantine) { Ok(()) => {} - Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} - Err(e) => { - let source = match std::fs::rename(&quarantine, worktree_path) { - Ok(()) => e, - Err(restore_error) => std::io::Error::new( - e.kind(), - format!( - "{e}; remaining checkout preserved at '{}'; restore failed: {restore_error}", - quarantine.display() + Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} + Err(error) => { + // `remove_dir_all` can have already removed the checkout and leave + // only Finder metadata behind. Delete that narrow, verified class of + // debris; otherwise restore the still-owned quarantine and fail closed. + if let Err(cleanup_error) = cleanup_benign_residual(&quarantine) { + let source = match std::fs::rename(&quarantine, worktree_path) { + Ok(()) => std::io::Error::other(format!( + "{error}; residual cleanup refused: {cleanup_error}" + )), + Err(restore_error) => std::io::Error::new( + error.kind(), + format!( + "{error}; residual cleanup refused: {cleanup_error}; remaining checkout preserved at '{}'; restore failed: {restore_error}", + quarantine.display() + ), ), - ), - }; - return Err(GitError::RemoveFailed { - path: worktree_path.to_path_buf(), - source, - }); + }; + return Err(GitError::RemoveFailed { + path: worktree_path.to_path_buf(), + source, + }); + } } } + // A process such as Finder can recreate the old path after the atomic + // quarantine. It is safe to delete only an empty directory or `.DS_Store`; + // any other replacement is foreign data and must survive without pruning. + cleanup_benign_residual(worktree_path)?; + // Prune stale worktree entries from the main repo let main_str = path_str(&verified.parent_path)?; let output = safe_output(command("git").args(["-C", main_str, "worktree", "prune"]))?; @@ -510,6 +572,33 @@ mod tests { ); } + #[test] + fn benign_residual_cleanup_accepts_ds_store_and_absence() { + let parent = tempfile::tempdir().expect("create residual parent"); + let residual = parent.path().join("worktree"); + std::fs::create_dir(&residual).expect("create residual"); + std::fs::write(residual.join(".DS_Store"), "finder metadata").expect("write metadata"); + + assert!(remove_benign_residual(&residual).expect("remove benign metadata")); + assert!(!residual.exists()); + assert!(remove_benign_residual(&residual).expect("already absent is benign")); + } + + #[test] + fn benign_residual_cleanup_preserves_foreign_replacement() { + let parent = tempfile::tempdir().expect("create residual parent"); + let residual = parent.path().join("worktree"); + std::fs::create_dir(&residual).expect("create residual"); + let sentinel = residual.join("must-survive.txt"); + std::fs::write(&sentinel, "foreign data").expect("write sentinel"); + + assert!(!remove_benign_residual(&residual).expect("inspect foreign residual")); + assert_eq!( + std::fs::read_to_string(sentinel).expect("sentinel survives"), + "foreign data" + ); + } + #[test] fn guarded_fast_removal_rejects_a_replaced_checkout() { let (_tmp, repo) = init_temp_repo(); diff --git a/crates/okena-terminal/src/backend.rs b/crates/okena-terminal/src/backend.rs index 9cd5dc440..534c89652 100644 --- a/crates/okena-terminal/src/backend.rs +++ b/crates/okena-terminal/src/backend.rs @@ -8,6 +8,7 @@ use anyhow::Result; use std::collections::HashMap; use std::path::PathBuf; use std::sync::Arc; +use std::time::Duration; /// Exact startup command carried separately from the shell used to route it. #[derive(Clone, Debug, PartialEq, Eq)] @@ -113,6 +114,12 @@ pub trait TerminalBackend: Send + Sync { } /// Wait for teardown work queued before this call to finish. fn flush_teardown(&self) {} + /// Bounded teardown wait for destructive operations. `false` means a + /// terminal/session may still own its former working directory. + fn flush_teardown_with_timeout(&self, _timeout: Duration) -> bool { + self.flush_teardown(); + true + } /// Whether this backend can switch persistence routes without replacement. fn supports_session_backend_reconfiguration(&self) -> bool { false @@ -211,6 +218,10 @@ impl TerminalBackend for LocalBackend { self.pty_manager.flush_teardown() } + fn flush_teardown_with_timeout(&self, timeout: Duration) -> bool { + self.pty_manager.flush_teardown_with_timeout(timeout) + } + fn supports_session_backend_reconfiguration(&self) -> bool { true } diff --git a/crates/okena-terminal/src/pty_manager.rs b/crates/okena-terminal/src/pty_manager.rs index 06731cbbb..d51bd125d 100644 --- a/crates/okena-terminal/src/pty_manager.rs +++ b/crates/okena-terminal/src/pty_manager.rs @@ -15,6 +15,7 @@ use std::sync::Arc; use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; use std::sync::mpsc; use std::thread::JoinHandle; +use std::time::Duration; #[cfg(windows)] fn append_wsl_environment(cmd: &mut CommandBuilder, environment: &[(String, Option)]) { @@ -333,6 +334,7 @@ impl Drop for PendingSessionKill { struct TeardownTracker { pending: Mutex, drained: Condvar, + failed: AtomicBool, } impl TeardownTracker { @@ -353,6 +355,24 @@ impl TeardownTracker { while *pending != 0 { self.drained.wait(&mut pending); } + self.failed.store(false, Ordering::Release); + } + + fn mark_failed(&self) { + self.failed.store(true, Ordering::Release); + } + + fn flush_timeout(&self, timeout: Duration) -> bool { + let deadline = std::time::Instant::now() + timeout; + let mut pending = self.pending.lock(); + while *pending != 0 { + let now = std::time::Instant::now(); + if now >= deadline { + return false; + } + self.drained.wait_for(&mut pending, deadline - now); + } + !self.failed.swap(false, Ordering::AcqRel) } } @@ -471,7 +491,7 @@ impl PtyManager { // sender, then `recv_blocking` returns Err once buffered jobs run). while let Ok(job) = rx.recv_blocking() { if let Err(panic) = std::panic::catch_unwind(AssertUnwindSafe(|| { - Self::run_teardown_job(job); + Self::run_teardown_job(job, &tracker); })) { log::error!("PTY teardown worker panicked: {}", format_panic(&*panic)); } @@ -505,13 +525,13 @@ impl PtyManager { ) } - /// Execute one teardown job on a worker thread. This is exactly what the old - /// per-call detached closures did: reap the handle's reader/writer threads (if a - /// handle is present), then run the session kill (only for `KillSession` jobs). - fn run_teardown_job(mut job: TeardownJob) { - if let Some(handle) = job.handle { - Self::shutdown_handle(handle); - } + /// Execute one teardown job on a worker thread. A persistent session is + /// stopped first, then the attach handle is reaped without blocking a worker + /// indefinitely on a child that ignores termination. + fn run_teardown_job(mut job: TeardownJob, tracker: &Arc) { + // End persistent sessions before waiting for the attach client. In + // particular, dtach can otherwise keep its shell (and checkout CWD) + // alive after the client has been signalled. match job.kind { // Process already EOF'd; nothing to SIGTERM from our side. TeardownKind::ReapOnly => {} @@ -533,12 +553,21 @@ impl PtyManager { wsl_distro.as_deref(), &session_name, ); - return; + } else { + if !session_backend.kill_session(&session_name) { + tracker.mark_failed(); + } } } - session_backend.kill_session(&session_name); + #[cfg(not(windows))] + if !session_backend.kill_session(&session_name) { + tracker.mark_failed(); + } } } + if let Some(handle) = job.handle.take() { + Self::shutdown_handle(handle, Some(tracker)); + } drop(job.pending_session_kill.take()); } @@ -1443,9 +1472,9 @@ impl PtyManager { } fn run_tracked_teardown(&self, job: TeardownJob) { - if let Err(panic) = - std::panic::catch_unwind(AssertUnwindSafe(|| Self::run_teardown_job(job))) - { + if let Err(panic) = std::panic::catch_unwind(AssertUnwindSafe(|| { + Self::run_teardown_job(job, &self.teardown_tracker) + })) { log::error!("PTY teardown panicked: {}", format_panic(&*panic)); } self.teardown_tracker.completed(); @@ -1459,8 +1488,14 @@ impl PtyManager { self.teardown_tracker.flush(); } + /// Wait only a bounded interval for queued teardown and detached reapers. + /// `false` means a process may still own its former working directory. + pub fn flush_teardown_with_timeout(&self, timeout: Duration) -> bool { + self.teardown_tracker.flush_timeout(timeout) + } + /// Perform coordinated shutdown of a single PTY handle - fn shutdown_handle(mut handle: PtyHandle) { + fn shutdown_handle(mut handle: PtyHandle, tracker: Option<&Arc>) { let id = handle.shutdown.terminal_id.clone(); // 1. Signal shutdown to threads @@ -1512,20 +1547,61 @@ impl PtyManager { let id = id.to_string(); let short_id = id[..8.min(id.len())].to_string(); let reaper_id = id.clone(); + let reaper_tracker = tracker.cloned(); + if let Some(tracker) = &reaper_tracker { + // A teardown job is not complete until this child has exited + // and its reader has joined. `flush_teardown_with_timeout` + // uses this count to prevent destructive CWD removal races. + tracker.queued(); + } + // Retain the handle outside the closure until thread creation + // succeeds. `Builder::spawn` drops a moved closure on failure, + // which would otherwise trigger PtyHandle's intentionally + // non-killing Drop path. + let reap_handle = Arc::new(Mutex::new(Some(handle))); + let reaper_handle = Arc::clone(&reap_handle); if let Err(e) = std::thread::Builder::new() .name(format!("pty-reaper-{short_id}")) .spawn(move || { + let Some(mut handle) = reaper_handle.lock().take() else { + log::error!("PTY child {} reaper lost its handle", reaper_id); + if let Some(tracker) = reaper_tracker { + tracker.completed(); + } + return; + }; if let Err(e) = handle.child.wait() { log::debug!("PTY child {} reaper wait failed: {}", reaper_id, e); } join_reader_handle(handle.reader_handle.take(), &reaper_id); + if let Some(tracker) = reaper_tracker { + tracker.completed(); + } }) { - // Thread creation failure is exceptional. Dropping the - // handles is still non-blocking and avoids pinning a shared - // teardown worker; the child/reader Drop backstops have - // already been signalled above. - log::warn!("Failed to spawn PTY child reaper for {}: {}", id, e); + // Do not drop a live `PtyHandle`: its Drop intentionally + // neither kills nor waits. A worker already owns this + // exceptional path, so synchronously retain and reap it. + if let Some(tracker) = tracker { + tracker.completed(); + } + log::warn!( + "Failed to spawn PTY child reaper for {}: {}; reaping inline", + id, + e + ); + if let Some(mut handle) = reap_handle.lock().take() { + if let Err(wait_error) = handle.child.wait() { + log::debug!( + "PTY child {} inline reaper wait failed: {}", + id, + wait_error + ); + } + join_reader_handle(handle.reader_handle.take(), &id); + } else { + log::error!("PTY child {} failed reaper lost its handle", id); + } } } } @@ -1543,7 +1619,7 @@ impl PtyManager { } drop(instances); for handle in handles { - Self::shutdown_handle(handle); + Self::shutdown_handle(handle, None); } } @@ -2371,7 +2447,7 @@ mod tests { let (done_tx, done_rx) = mpsc::channel(); std::thread::spawn(move || { - PtyManager::shutdown_handle(handle); + PtyManager::shutdown_handle(handle, None); let _ = done_tx.send(()); }); @@ -2441,7 +2517,7 @@ mod tests { )), }; - PtyManager::shutdown_handle(handle); + PtyManager::shutdown_handle(handle, None); reader_done_rx .recv_timeout(Duration::from_secs(1)) .expect("reader joins after observing writer clone closure"); diff --git a/crates/okena-terminal/src/session_backend.rs b/crates/okena-terminal/src/session_backend.rs index 51a46fd91..d525e3473 100644 --- a/crates/okena-terminal/src/session_backend.rs +++ b/crates/okena-terminal/src/session_backend.rs @@ -426,10 +426,11 @@ impl ResolvedBackend { } } - /// Kill a session - pub fn kill_session(&self, session_name: &str) { + /// Stop a persistent session. Returns false only when a verified dtach + /// holder survives bounded TERM/KILL escalation and may retain its CWD. + pub fn kill_session(&self, session_name: &str) -> bool { match self { - Self::None => {} + Self::None => true, Self::Tmux => { #[cfg(target_os = "macos")] let _ = crate::process::safe_output( @@ -444,6 +445,7 @@ impl ResolvedBackend { "-t", session_name, ])); + true } Self::Screen => { #[cfg(target_os = "macos")] @@ -460,6 +462,7 @@ impl ResolvedBackend { "-X", "quit", ])); + true } Self::Dtach => { let socket_path = get_dtach_socket_path(session_name); @@ -480,6 +483,7 @@ impl ResolvedBackend { let holders = crate::pty_manager::find_pids_for_unix_sockets( std::slice::from_ref(&socket_path), ); + let mut signalled = Vec::new(); for &pid in holders.get(&socket_path).into_iter().flatten() { let pid = pid as i32; if pid == my_pid { @@ -504,24 +508,77 @@ impl ResolvedBackend { unsafe { libc::kill(pid, libc::SIGTERM); } + signalled.push(pid); log::debug!( "Sent SIGTERM to dtach process {} for session {}", pid, session_name ); } + if !wait_for_pids_to_exit(&signalled) { + log::error!( + "dtach session {} still has a live holder after SIGKILL; preserving dependent checkout", + session_name + ); + return false; + } } let _ = std::fs::remove_file(&socket_path); log::debug!("Removed dtach socket: {:?}", socket_path); } + true } Self::Psmux => { let mut cmd = crate::process::command("psmux"); cmd.args(["kill-session", "-t", session_name]); let _ = crate::process::safe_output(&mut cmd); log::debug!("Killed psmux session {}", session_name); + true + } + } + } +} + +#[cfg(unix)] +fn process_is_live(pid: i32) -> bool { + if unsafe { libc::kill(pid, 0) } == 0 { + return true; + } + std::io::Error::last_os_error().raw_os_error() == Some(libc::EPERM) +} + +#[cfg(unix)] +fn wait_for_pids_to_exit(pids: &[i32]) -> bool { + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(2); + loop { + let live: Vec = pids + .iter() + .copied() + .filter(|pid| process_is_live(*pid)) + .collect(); + if live.is_empty() { + return true; + } + if std::time::Instant::now() >= deadline { + for pid in live { + // The PID was verified as a holder of this Okena-owned socket + // immediately before TERM; this is the bounded escalation path. + unsafe { libc::kill(pid, libc::SIGKILL) }; } + break; + } + std::thread::sleep(std::time::Duration::from_millis(20)); + } + let deadline = std::time::Instant::now() + std::time::Duration::from_millis(500); + loop { + let any_live = pids.iter().any(|pid| process_is_live(*pid)); + if !any_live { + return true; + } + if std::time::Instant::now() >= deadline { + return false; } + std::thread::sleep(std::time::Duration::from_millis(20)); } } diff --git a/src/main.rs b/src/main.rs index c818c2415..76c8e2294 100644 --- a/src/main.rs +++ b/src/main.rs @@ -89,6 +89,52 @@ impl std::io::Write for TeeWriter { } } +/// Rotate one log without relying on platform-specific replacement semantics. +/// Windows does not let `rename` overwrite an existing destination, so remove +/// the old rotation target first and fail rather than truncating the active log. +fn rotate_log_file(active: &std::path::Path, previous: &std::path::Path) -> std::io::Result<()> { + if !active.exists() { + return Ok(()); + } + match std::fs::remove_file(previous) { + Ok(()) => {} + Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} + Err(error) => return Err(error), + } + std::fs::rename(active, previous) +} + +#[cfg(test)] +mod log_rotation_tests { + use super::rotate_log_file; + + #[test] + fn rotation_replaces_existing_previous_file_without_truncating_active() { + let directory = std::env::temp_dir().join(format!( + "okena-log-rotation-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("system clock is after epoch") + .as_nanos() + )); + std::fs::create_dir(&directory).expect("create log directory"); + let active = directory.join("okena-headless.log"); + let previous = directory.join("okena-headless.log.1"); + std::fs::write(&active, "active log").expect("write active log"); + std::fs::write(&previous, "old rotation").expect("write old rotation"); + + rotate_log_file(&active, &previous).expect("rotate log"); + + assert!(!active.exists()); + assert_eq!( + std::fs::read_to_string(&previous).expect("read rotation"), + "active log" + ); + std::fs::remove_dir_all(directory).expect("remove log directory"); + } +} + use crate::assets::{Assets, embedded_fonts}; use okena_app::app::Okena; use okena_app::keybindings; @@ -482,11 +528,31 @@ fn main() { // Set up file logging: rotate previous log, write to both stderr and file let log_target = (|| -> Option { let root = &profiles::current().root; - std::fs::create_dir_all(root).ok()?; - if profile_log.exists() { - let _ = std::fs::rename(&profile_log, &profile_log_prev); + if let Err(error) = std::fs::create_dir_all(root) { + eprintln!( + "Warning: could not create log directory '{}': {error}", + root.display() + ); + return None; + } + if let Err(error) = rotate_log_file(&profile_log, &profile_log_prev) { + eprintln!( + "Warning: could not rotate log '{}' to '{}'; leaving the active log intact: {error}", + profile_log.display(), + profile_log_prev.display() + ); + return None; } - let file = std::fs::File::create(&profile_log).ok()?; + let file = match std::fs::File::create(&profile_log) { + Ok(file) => file, + Err(error) => { + eprintln!( + "Warning: could not create log '{}': {error}", + profile_log.display() + ); + return None; + } + }; Some(env_logger::fmt::Target::Pipe(Box::new(TeeWriter { stderr: std::io::stderr(), file, From 92623b9b9de517edeb610e65cafe8ea7f8975682 Mon Sep 17 00:00:00 2001 From: Jakub Date: Thu, 23 Jul 2026 16:04:07 +0200 Subject: [PATCH 05/11] fix(teardown): bound reaping and verify session exits --- crates/okena-daemon-core/src/command_loop.rs | 27 +- crates/okena-daemon-core/src/daemon.rs | 8 +- crates/okena-git/src/repository/worktree.rs | 20 ++ crates/okena-terminal/src/pty_manager.rs | 255 +++++++++++++------ crates/okena-terminal/src/session_backend.rs | 172 ++++++++++--- 5 files changed, 359 insertions(+), 123 deletions(-) diff --git a/crates/okena-daemon-core/src/command_loop.rs b/crates/okena-daemon-core/src/command_loop.rs index 295fce669..352178fbe 100644 --- a/crates/okena-daemon-core/src/command_loop.rs +++ b/crates/okena-daemon-core/src/command_loop.rs @@ -521,10 +521,15 @@ async fn flush_project_runtime_teardown( for teardown in &teardown_sessions { backend.kill_session(teardown); } - backend.flush_teardown(); + if backend.flush_teardown_with_timeout(Duration::from_secs(5)) { + Ok(()) + } else { + Err("terminal teardown did not release project paths in time; checkout preserved") + } }) .await - .map_err(|error| format!("terminal teardown task failed: {error}")) + .map_err(|error| format!("terminal teardown task failed: {error}"))? + .map_err(str::to_string) } #[allow(clippy::too_many_arguments)] @@ -6511,6 +6516,7 @@ mod tests { killed: std::sync::atomic::AtomicBool, flush_started: std::sync::atomic::AtomicBool, release: Mutex>, + timeout_result: Option, } struct RenameRecordingBackend { @@ -6679,6 +6685,21 @@ mod tests { .expect("test releases teardown barrier"); } + fn flush_teardown_with_timeout(&self, _timeout: Duration) -> bool { + if let Some(result) = self.timeout_result { + assert!( + self.killed.load(std::sync::atomic::Ordering::SeqCst), + "project PTYs must be killed before bounded teardown verification" + ); + self.flush_started + .store(true, std::sync::atomic::Ordering::SeqCst); + result + } else { + self.flush_teardown(); + true + } + } + fn capture_buffer(&self, _terminal_id: &str) -> Option { None } @@ -7107,6 +7128,7 @@ mod tests { killed: std::sync::atomic::AtomicBool::new(false), flush_started: std::sync::atomic::AtomicBool::new(false), release: Mutex::new(teardown_release_rx), + timeout_result: None, }); let terminals: TerminalsRegistry = Arc::new(Mutex::new(HashMap::new())); let service_manager = Arc::new(Mutex::new(ServiceManager::new( @@ -8209,6 +8231,7 @@ mod tests { killed: std::sync::atomic::AtomicBool::new(false), flush_started: std::sync::atomic::AtomicBool::new(false), release: Mutex::new(release_rx), + timeout_result: None, }); let backend: Arc = barrier_backend.clone(); let terminals: TerminalsRegistry = Arc::new(Mutex::new(Default::default())); diff --git a/crates/okena-daemon-core/src/daemon.rs b/crates/okena-daemon-core/src/daemon.rs index da40f0171..389344bfa 100644 --- a/crates/okena-daemon-core/src/daemon.rs +++ b/crates/okena-daemon-core/src/daemon.rs @@ -551,7 +551,13 @@ impl DaemonCore { &*shutdown_backend, &shutdown_terminals, || shutdown_autosaves.flush(), - || shutdown_pty_manager.flush_teardown(), + || { + if !shutdown_pty_manager + .flush_teardown_with_timeout(std::time::Duration::from_secs(5)) + { + log::warn!("terminal teardown still owns a process at daemon shutdown"); + } + }, persistence::save_workspace, )?; remote_server.stop(); diff --git a/crates/okena-git/src/repository/worktree.rs b/crates/okena-git/src/repository/worktree.rs index d3f1b41c5..1f3f4f580 100644 --- a/crates/okena-git/src/repository/worktree.rs +++ b/crates/okena-git/src/repository/worktree.rs @@ -497,15 +497,25 @@ pub fn list_linked_worktree_paths(repo_path: &Path) -> Vec { let Some(repo) = crate::gix_helpers::open(repo_path) else { return Vec::new(); }; + // macOS exposes `/var` through `/private/var`. gix may report either spelling + // for the main worktree, so compare existing paths by canonical filesystem + // identity instead of lexical components. Missing paths retain the portable + // lexical fallback used elsewhere in this module. + let main_worktree = repo.workdir().map(path_identity); let Ok(worktrees) = repo.worktrees() else { return Vec::new(); }; worktrees .into_iter() .filter_map(|proxy| proxy.base().ok()) + .filter(|path| main_worktree.as_ref() != Some(&path_identity(path))) .collect() } +fn path_identity(path: &Path) -> PathBuf { + std::fs::canonicalize(path).unwrap_or_else(|_| crate::repository::normalize_path(path)) +} + #[cfg(test)] mod tests { use super::*; @@ -534,6 +544,16 @@ mod tests { assert_eq!(branches, vec!["feat", "main"]); } + #[test] + fn path_identity_prefers_canonical_filesystem_path() { + let directory = tempfile::tempdir().expect("create identity directory"); + let dotted = directory.path().join("."); + assert_eq!( + path_identity(&dotted), + directory.path().canonicalize().unwrap() + ); + } + #[test] fn list_linked_worktree_paths_excludes_main_worktree() { let (_tmp, repo) = init_temp_repo(); diff --git a/crates/okena-terminal/src/pty_manager.rs b/crates/okena-terminal/src/pty_manager.rs index d51bd125d..991376ff7 100644 --- a/crates/okena-terminal/src/pty_manager.rs +++ b/crates/okena-terminal/src/pty_manager.rs @@ -12,6 +12,8 @@ use std::collections::HashMap; use std::io::{Read, Write}; use std::panic::AssertUnwindSafe; use std::sync::Arc; +#[cfg(test)] +use std::sync::atomic::AtomicUsize; use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; use std::sync::mpsc; use std::thread::JoinHandle; @@ -275,6 +277,12 @@ fn join_reader_handle(reader_handle: Option>, terminal_id: &str) /// instead of spawning one detached OS thread per `kill()`/`cleanup_exited()` call. const TEARDOWN_WORKERS: usize = 4; +/// Number of workers allowed to wait for children that ignored termination. +/// This is deliberately separate from `TEARDOWN_WORKERS`: a stuck child must +/// not consume the normal teardown pool, and N stuck children must not create +/// N OS threads. +const REAPER_WORKERS: usize = 2; + #[derive(Clone, Copy)] struct SessionBackendSelection { preference: SessionBackend, @@ -315,6 +323,12 @@ struct TeardownJob { pending_session_kill: Option, } +/// A child that ignored termination. The reaper retains the full handle until +/// the child exits and the reader can be joined; it is never dropped live. +struct ReaperJob { + handle: PtyHandle, +} + struct PendingSessionKill { terminal_id: String, instances: Arc>, @@ -451,7 +465,14 @@ pub struct PtyManager { /// only so `Drop` can `take()` it and close the channel, signaling workers to /// drain remaining jobs and exit. teardown_tx: Option>, + /// Manager-owned fixed reaper pool for children that survive initial teardown. + /// The unbounded queue holds ownership, while `REAPER_WORKERS` bounds threads + /// that can wait forever. This keeps ordinary shutdown non-blocking while a + /// destructive flush can observe every live CWD-owning child through the tracker. + reaper_tx: Option>, teardown_tracker: Arc, + #[cfg(test)] + reaper_worker_count: Arc, } impl PtyManager { @@ -476,14 +497,47 @@ impl PtyManager { log::warn!("failed to spawn dtach cleanup thread: {e}"); } + let teardown_tracker = Arc::new(TeardownTracker::default()); + + // A distinct fixed pool waits for stubborn children. Unlike the normal + // teardown workers, these may wait forever, so no job is allowed to make + // a new thread here. The tracker remains pending until each handle exits. + let (reaper_tx, reaper_rx) = async_channel::unbounded::(); + #[cfg(test)] + let reaper_worker_count = Arc::new(AtomicUsize::new(0)); + for i in 0..REAPER_WORKERS { + let rx = reaper_rx.clone(); + let tracker = Arc::clone(&teardown_tracker); + #[cfg(test)] + let worker_count = Arc::clone(&reaper_worker_count); + if let Err(e) = std::thread::Builder::new() + .name(format!("pty-reaper-{i}")) + .spawn(move || { + #[cfg(test)] + worker_count.fetch_add(1, Ordering::Release); + while let Ok(mut job) = rx.recv_blocking() { + let id = job.handle.shutdown.terminal_id.clone(); + if let Err(wait_error) = job.handle.child.wait() { + log::debug!("PTY child {} reaper wait failed: {}", id, wait_error); + } + join_reader_handle(job.handle.reader_handle.take(), &id); + tracker.completed(); + } + }) + { + log::error!("failed to spawn PTY reaper worker {i}: {e}"); + } + } + drop(reaper_rx); + // Shared teardown worker pool. `async-channel` is MPMC, so all workers share // one `Receiver` and pull jobs via `recv_blocking`. Unbounded so enqueuing // never blocks the GPUI thread; concurrency is bounded by the worker count. let (teardown_tx, teardown_rx) = async_channel::unbounded::(); - let teardown_tracker = Arc::new(TeardownTracker::default()); for i in 0..TEARDOWN_WORKERS { let rx = teardown_rx.clone(); let tracker = Arc::clone(&teardown_tracker); + let reaper_tx = reaper_tx.clone(); if let Err(e) = std::thread::Builder::new() .name(format!("pty-teardown-{i}")) .spawn(move || { @@ -491,7 +545,7 @@ impl PtyManager { // sender, then `recv_blocking` returns Err once buffered jobs run). while let Ok(job) = rx.recv_blocking() { if let Err(panic) = std::panic::catch_unwind(AssertUnwindSafe(|| { - Self::run_teardown_job(job, &tracker); + Self::run_teardown_job(job, &tracker, Some(&reaper_tx)); })) { log::error!("PTY teardown worker panicked: {}", format_panic(&*panic)); } @@ -519,7 +573,10 @@ impl PtyManager { output_sink: Arc::new(Mutex::new(None)), extra_env: Mutex::new(Vec::new()), teardown_tx: Some(teardown_tx), + reaper_tx: Some(reaper_tx), teardown_tracker, + #[cfg(test)] + reaper_worker_count, }, rx, ) @@ -528,7 +585,11 @@ impl PtyManager { /// Execute one teardown job on a worker thread. A persistent session is /// stopped first, then the attach handle is reaped without blocking a worker /// indefinitely on a child that ignores termination. - fn run_teardown_job(mut job: TeardownJob, tracker: &Arc) { + fn run_teardown_job( + mut job: TeardownJob, + tracker: &Arc, + reaper_tx: Option<&Sender>, + ) { // End persistent sessions before waiting for the attach client. In // particular, dtach can otherwise keep its shell (and checkout CWD) // alive after the client has been signalled. @@ -566,7 +627,7 @@ impl PtyManager { } } if let Some(handle) = job.handle.take() { - Self::shutdown_handle(handle, Some(tracker)); + Self::shutdown_handle(handle, Some(tracker), reaper_tx); } drop(job.pending_session_kill.take()); } @@ -1473,7 +1534,7 @@ impl PtyManager { fn run_tracked_teardown(&self, job: TeardownJob) { if let Err(panic) = std::panic::catch_unwind(AssertUnwindSafe(|| { - Self::run_teardown_job(job, &self.teardown_tracker) + Self::run_teardown_job(job, &self.teardown_tracker, self.reaper_tx.as_ref()) })) { log::error!("PTY teardown panicked: {}", format_panic(&*panic)); } @@ -1495,7 +1556,11 @@ impl PtyManager { } /// Perform coordinated shutdown of a single PTY handle - fn shutdown_handle(mut handle: PtyHandle, tracker: Option<&Arc>) { + fn shutdown_handle( + mut handle: PtyHandle, + tracker: Option<&Arc>, + reaper_tx: Option<&Sender>, + ) { let id = handle.shutdown.terminal_id.clone(); // 1. Signal shutdown to threads @@ -1541,67 +1606,26 @@ impl PtyManager { join_reader_handle(handle.reader_handle.take(), &id); } Ok(None) => { - // Keep BOTH ownership pieces in a rare detached reaper. It waits - // for child termination first, then joins the reader that EOF - // unblocks, leaving the shared teardown worker available. - let id = id.to_string(); - let short_id = id[..8.min(id.len())].to_string(); - let reaper_id = id.clone(); - let reaper_tracker = tracker.cloned(); - if let Some(tracker) = &reaper_tracker { - // A teardown job is not complete until this child has exited - // and its reader has joined. `flush_teardown_with_timeout` - // uses this count to prevent destructive CWD removal races. + // Transfer the still-live handle to the manager-owned fixed reaper + // pool. The extra tracker count is the destructive-operation gate: + // worktree removal may proceed only after this child exits and its + // reader has joined. Normal manager Drop never flushes this count. + if let Some(tracker) = tracker { tracker.queued(); } - // Retain the handle outside the closure until thread creation - // succeeds. `Builder::spawn` drops a moved closure on failure, - // which would otherwise trigger PtyHandle's intentionally - // non-killing Drop path. - let reap_handle = Arc::new(Mutex::new(Some(handle))); - let reaper_handle = Arc::clone(&reap_handle); - if let Err(e) = std::thread::Builder::new() - .name(format!("pty-reaper-{short_id}")) - .spawn(move || { - let Some(mut handle) = reaper_handle.lock().take() else { - log::error!("PTY child {} reaper lost its handle", reaper_id); - if let Some(tracker) = reaper_tracker { - tracker.completed(); - } - return; - }; - if let Err(e) = handle.child.wait() { - log::debug!("PTY child {} reaper wait failed: {}", reaper_id, e); - } - join_reader_handle(handle.reader_handle.take(), &reaper_id); - if let Some(tracker) = reaper_tracker { - tracker.completed(); - } - }) - { - // Do not drop a live `PtyHandle`: its Drop intentionally - // neither kills nor waits. A worker already owns this - // exceptional path, so synchronously retain and reap it. - if let Some(tracker) = tracker { - tracker.completed(); - } - log::warn!( - "Failed to spawn PTY child reaper for {}: {}; reaping inline", - id, - e - ); - if let Some(mut handle) = reap_handle.lock().take() { - if let Err(wait_error) = handle.child.wait() { - log::debug!( - "PTY child {} inline reaper wait failed: {}", - id, - wait_error - ); - } - join_reader_handle(handle.reader_handle.take(), &id); - } else { - log::error!("PTY child {} failed reaper lost its handle", id); + if let Some(tx) = reaper_tx { + if let Err(error) = tx.send_blocking(ReaperJob { handle }) { + // A live handle must never fall through to PtyHandle::Drop. + // A closed reaper queue can only occur during manager drop; + // retain it until process exit rather than blocking shutdown. + log::error!("PTY reaper queue closed for {}; retaining live handle", id); + std::mem::forget(error.into_inner()); } + } else { + // This is only reachable from direct unit-test helpers. Keep the + // handle alive rather than silently dropping a CWD-owning child. + log::error!("PTY reaper unavailable for {}; retaining live handle", id); + std::mem::forget(handle); } } } @@ -1619,7 +1643,7 @@ impl PtyManager { } drop(instances); for handle in handles { - Self::shutdown_handle(handle, None); + Self::shutdown_handle(handle, None, self.reaper_tx.as_ref()); } } @@ -2012,6 +2036,7 @@ impl Drop for PtyManager { // teardown of already-enqueued jobs is best-effort at quit — the process may // exit before slow jobs finish, which is acceptable for graceful detach. drop(self.teardown_tx.take()); + drop(self.reaper_tx.take()); } } @@ -2444,19 +2469,20 @@ mod tests { PtyGeneration(1), )), }; - let (done_tx, done_rx) = mpsc::channel(); - - std::thread::spawn(move || { - PtyManager::shutdown_handle(handle, None); - let _ = done_tx.send(()); - }); - - done_rx - .recv_timeout(Duration::from_secs(1)) - .expect("PTY teardown must not wait forever for child or reader"); + let (manager, _events) = PtyManager::new(SessionBackend::None); + let started = std::time::Instant::now(); + PtyManager::shutdown_handle( + handle, + Some(&manager.teardown_tracker), + manager.reaper_tx.as_ref(), + ); + assert!( + started.elapsed() < Duration::from_secs(1), + "PTY teardown must not wait forever for child or reader" + ); assert!( reader_done_rx.try_recv().is_err(), - "reader is still owned by detached reaper until child/reader release" + "reader is still owned by the manager reaper until child/reader release" ); *child_release.0.lock() = true; child_release.1.notify_all(); @@ -2464,7 +2490,8 @@ mod tests { reader_release.1.notify_all(); reader_done_rx .recv_timeout(Duration::from_secs(1)) - .expect("detached reaper joins released reader"); + .expect("manager reaper joins released reader"); + assert!(manager.flush_teardown_with_timeout(Duration::from_secs(1))); } #[cfg(unix)] @@ -2517,12 +2544,84 @@ mod tests { )), }; - PtyManager::shutdown_handle(handle, None); + PtyManager::shutdown_handle(handle, None, None); reader_done_rx .recv_timeout(Duration::from_secs(1)) .expect("reader joins after observing writer clone closure"); } + #[cfg(unix)] + #[test] + fn stuck_children_use_fixed_reaper_pool_and_keep_flush_pending() { + const STUCK_CHILDREN: usize = REAPER_WORKERS + 5; + let (manager, _events) = PtyManager::new(SessionBackend::None); + let worker_deadline = std::time::Instant::now() + Duration::from_secs(1); + while manager.reaper_worker_count.load(Ordering::Acquire) != REAPER_WORKERS { + assert!( + std::time::Instant::now() < worker_deadline, + "fixed reaper workers did not start" + ); + std::thread::sleep(Duration::from_millis(5)); + } + + let child_release = Arc::new((Mutex::new(false), Condvar::new())); + let reader_release = Arc::new((Mutex::new(false), Condvar::new())); + let (reader_done_tx, reader_done_rx) = mpsc::channel(); + for index in 0..STUCK_CHILDREN { + let reader_wait = Arc::clone(&reader_release); + let reader_done_tx = reader_done_tx.clone(); + let reader_handle = std::thread::spawn(move || { + let mut released = reader_wait.0.lock(); + while !*released { + reader_wait.1.wait(&mut released); + } + reader_done_tx.send(index).expect("reader finishes"); + }); + let handle = PtyHandle { + generation: PtyGeneration(index as u64 + 1), + master: None, + child: Box::new(DelayedTerminationChild { + release: Arc::clone(&child_release), + }), + input_tx: None, + writer: None, + reader_handle: Some(reader_handle), + writer_handle: None, + shutdown: Arc::new(PtyShutdownState::new( + format!("stuck-{index}"), + PtyGeneration(index as u64 + 1), + )), + }; + PtyManager::shutdown_handle( + handle, + Some(&manager.teardown_tracker), + manager.reaper_tx.as_ref(), + ); + } + + assert_eq!( + manager.reaper_worker_count.load(Ordering::Acquire), + REAPER_WORKERS, + "N stuck children must not create N reaper threads" + ); + assert_eq!(*manager.teardown_tracker.pending.lock(), STUCK_CHILDREN); + assert!( + !manager.flush_teardown_with_timeout(Duration::from_millis(50)), + "destructive flush must remain blocked while children may own a CWD" + ); + + *child_release.0.lock() = true; + child_release.1.notify_all(); + *reader_release.0.lock() = true; + reader_release.1.notify_all(); + for _ in 0..STUCK_CHILDREN { + reader_done_rx + .recv_timeout(Duration::from_secs(1)) + .expect("all reaper-owned readers finish"); + } + assert!(manager.flush_teardown_with_timeout(Duration::from_secs(1))); + } + #[cfg(unix)] #[test] fn dtach_kill_does_not_stall_teardown_flush() { diff --git a/crates/okena-terminal/src/session_backend.rs b/crates/okena-terminal/src/session_backend.rs index d525e3473..e3f04a75e 100644 --- a/crates/okena-terminal/src/session_backend.rs +++ b/crates/okena-terminal/src/session_backend.rs @@ -426,44 +426,27 @@ impl ResolvedBackend { } } - /// Stop a persistent session. Returns false only when a verified dtach - /// holder survives bounded TERM/KILL escalation and may retain its CWD. + /// Stop a persistent session. Success means both the kill command and a + /// bounded liveness probe confirm that the session no longer exists. pub fn kill_session(&self, session_name: &str) -> bool { match self { Self::None => true, - Self::Tmux => { - #[cfg(target_os = "macos")] - let _ = crate::process::safe_output( - crate::process::command("tmux") - .args(["kill-session", "-t", session_name]) - .env("PATH", get_extended_path()), - ); - - #[cfg(all(unix, not(target_os = "macos")))] - let _ = crate::process::safe_output(crate::process::command("tmux").args([ - "kill-session", - "-t", - session_name, - ])); - true - } - Self::Screen => { - #[cfg(target_os = "macos")] - let _ = crate::process::safe_output( - crate::process::command("screen") - .args(["-S", session_name, "-X", "quit"]) - .env("PATH", get_extended_path()), - ); - - #[cfg(all(unix, not(target_os = "macos")))] - let _ = crate::process::safe_output(crate::process::command("screen").args([ - "-S", - session_name, - "-X", - "quit", - ])); - true - } + Self::Tmux => verify_session_kill( + session_backend_output("tmux", &["kill-session", "-t", session_name]), + || { + session_backend_output("tmux", &["has-session", "-t", session_name]) + .map(|output| output.status.success()) + }, + std::time::Duration::from_secs(2), + ), + Self::Screen => verify_session_kill( + session_backend_output("screen", &["-S", session_name, "-X", "quit"]), + || { + session_backend_output("screen", &["-S", session_name, "-Q", "select", "."]) + .map(|output| output.status.success()) + }, + std::time::Duration::from_secs(2), + ), Self::Dtach => { let socket_path = get_dtach_socket_path(session_name); if socket_path.exists() { @@ -522,18 +505,81 @@ impl ResolvedBackend { ); return false; } + let remaining = crate::pty_manager::find_pids_for_unix_sockets( + std::slice::from_ref(&socket_path), + ); + if remaining + .get(&socket_path) + .into_iter() + .flatten() + .any(|pid| *pid as i32 != my_pid) + { + log::error!( + "dtach session {} still has a socket owner; preserving dependent checkout", + session_name + ); + return false; + } + } + if let Err(error) = std::fs::remove_file(&socket_path) + && error.kind() != std::io::ErrorKind::NotFound + { + log::error!("failed to remove dtach socket {:?}: {}", socket_path, error); + return false; } - let _ = std::fs::remove_file(&socket_path); log::debug!("Removed dtach socket: {:?}", socket_path); } true } - Self::Psmux => { - let mut cmd = crate::process::command("psmux"); - cmd.args(["kill-session", "-t", session_name]); - let _ = crate::process::safe_output(&mut cmd); - log::debug!("Killed psmux session {}", session_name); - true + Self::Psmux => verify_session_kill( + session_backend_output("psmux", &["kill-session", "-t", session_name]), + || { + session_backend_output("psmux", &["has-session", "-t", session_name]) + .map(|output| output.status.success()) + }, + std::time::Duration::from_secs(2), + ), + } + } +} + +fn session_backend_output(program: &str, args: &[&str]) -> std::io::Result { + let mut command = crate::process::command(program); + command.args(args); + #[cfg(target_os = "macos")] + command.env("PATH", get_extended_path()); + crate::process::safe_output(&mut command) +} + +fn verify_session_kill( + kill_result: std::io::Result, + mut session_is_live: impl FnMut() -> std::io::Result, + timeout: std::time::Duration, +) -> bool { + match kill_result { + Ok(output) if output.status.success() => {} + Ok(output) => { + log::error!("session kill command exited with {}", output.status); + return false; + } + Err(error) => { + log::error!("failed to run session kill command: {error}"); + return false; + } + } + + let deadline = std::time::Instant::now() + timeout; + loop { + match session_is_live() { + Ok(false) => return true, + Ok(true) if std::time::Instant::now() >= deadline => { + log::error!("session survived bounded kill verification"); + return false; + } + Ok(true) => std::thread::sleep(std::time::Duration::from_millis(20)), + Err(error) => { + log::error!("failed to verify session liveness: {error}"); + return false; } } } @@ -1233,6 +1279,48 @@ fn is_screen_available() -> bool { mod tests { use super::*; + #[cfg(unix)] + fn successful_command_output() -> std::process::Output { + std::process::Command::new("true") + .output() + .expect("run true") + } + + #[cfg(unix)] + #[test] + fn verified_session_kill_rejects_command_failure() { + assert!(!verify_session_kill( + Err(std::io::Error::other("kill command failed")), + || panic!("liveness must not be checked after command failure"), + std::time::Duration::ZERO, + )); + } + + #[cfg(unix)] + #[test] + fn verified_session_kill_rejects_session_that_survives() { + let mut probes = 0; + assert!(!verify_session_kill( + Ok(successful_command_output()), + || { + probes += 1; + Ok(true) + }, + std::time::Duration::ZERO, + )); + assert_eq!(probes, 1, "the live session was probed before failure"); + } + + #[cfg(unix)] + #[test] + fn verified_session_kill_accepts_confirmed_disappearance() { + assert!(verify_session_kill( + Ok(successful_command_output()), + || Ok(false), + std::time::Duration::ZERO, + )); + } + #[test] fn test_parse_backend() { assert_eq!(SessionBackend::parse_str("tmux"), SessionBackend::Tmux); From 991835740d2ad5685281908bbcf0945076c3c400 Mon Sep 17 00:00:00 2001 From: Jakub Date: Thu, 23 Jul 2026 16:47:09 +0200 Subject: [PATCH 06/11] fix(teardown): retain indeterminate child reaping --- crates/okena-git/src/repository/worktree.rs | 77 ++++++++- crates/okena-terminal/src/pty_manager.rs | 29 +++- crates/okena-terminal/src/session_backend.rs | 161 +++++++++++++++---- 3 files changed, 231 insertions(+), 36 deletions(-) diff --git a/crates/okena-git/src/repository/worktree.rs b/crates/okena-git/src/repository/worktree.rs index 1f3f4f580..b3567e589 100644 --- a/crates/okena-git/src/repository/worktree.rs +++ b/crates/okena-git/src/repository/worktree.rs @@ -350,6 +350,13 @@ pub fn remove_worktree(verified: &VerifiedWorktree, force: bool) -> GitResult<() /// This is safe because prune only acts on entries whose directories no longer exist, /// and we only delete the single target directory before pruning. pub fn remove_worktree_fast(verified: &VerifiedWorktree) -> GitResult<()> { + remove_worktree_fast_with(verified, |path| std::fs::remove_dir_all(path)) +} + +fn remove_worktree_fast_with( + verified: &VerifiedWorktree, + remove_dir_all: impl FnOnce(&Path) -> std::io::Result<()>, +) -> GitResult<()> { revalidate_verified_worktree(verified)?; let worktree_path = &verified.checkout_path; let parent = worktree_path @@ -381,7 +388,7 @@ pub fn remove_worktree_fast(verified: &VerifiedWorktree) -> GitResult<()> { return Err(unsafe_worktree(worktree_path, reason)); } - match std::fs::remove_dir_all(&quarantine) { + match remove_dir_all(&quarantine) { Ok(()) => {} Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} Err(error) => { @@ -554,6 +561,18 @@ mod tests { ); } + #[cfg(unix)] + #[test] + fn path_identity_matches_a_directory_symlink_alias() { + let parent = tempfile::tempdir().expect("create identity parent"); + let actual = parent.path().join("actual"); + let alias = parent.path().join("alias"); + std::fs::create_dir(&actual).expect("create actual directory"); + std::os::unix::fs::symlink(&actual, &alias).expect("create directory alias"); + + assert_eq!(path_identity(&actual), path_identity(&alias)); + } + #[test] fn list_linked_worktree_paths_excludes_main_worktree() { let (_tmp, repo) = init_temp_repo(); @@ -564,7 +583,13 @@ mod tests { &["worktree", "add", wt_path.to_str().unwrap(), "-b", "feat"], ); - assert_eq!(list_linked_worktree_paths(&repo), vec![wt_path]); + assert_eq!( + list_linked_worktree_paths(&repo) + .iter() + .map(|path| path_identity(path)) + .collect::>(), + vec![path_identity(&wt_path)] + ); } #[test] @@ -619,6 +644,54 @@ mod tests { ); } + #[test] + fn fast_removal_cleans_partial_ds_store_residual_and_preserves_old_path_replacement() { + let (_tmp, repo) = init_temp_repo(); + let wt_tmp = tempfile::tempdir().expect("create worktree tempdir"); + let wt_path = wt_tmp.path().join("wt-feat"); + git_in( + &repo, + &["worktree", "add", wt_path.to_str().unwrap(), "-b", "feat"], + ); + let verified = verify_linked_worktree_fresh(&repo, &wt_path).expect("verify worktree"); + let replacement_path = wt_path.clone(); + + let result = remove_worktree_fast_with(&verified, |quarantine| { + std::fs::remove_dir_all(quarantine).expect("remove quarantined checkout contents"); + std::fs::create_dir(quarantine).expect("recreate partial quarantine residual"); + std::fs::write(quarantine.join(".DS_Store"), "finder metadata") + .expect("write partial residual"); + std::fs::create_dir(&replacement_path).expect("recreate old checkout path"); + std::fs::write(replacement_path.join("must-survive.txt"), "foreign data") + .expect("write replacement sentinel"); + Err(std::io::Error::other( + "simulated partial remove_dir_all failure", + )) + }); + + assert!( + result.is_err(), + "foreign old-path replacement must stop pruning" + ); + assert!( + !wt_tmp + .path() + .read_dir() + .expect("inspect worktree parent") + .filter_map(Result::ok) + .any(|entry| entry + .file_name() + .to_string_lossy() + .starts_with(".okena-removing-")), + "benign .DS_Store quarantine residual must be removed" + ); + assert_eq!( + std::fs::read_to_string(wt_path.join("must-survive.txt")) + .expect("foreign replacement survives"), + "foreign data" + ); + } + #[test] fn guarded_fast_removal_rejects_a_replaced_checkout() { let (_tmp, repo) = init_temp_repo(); diff --git a/crates/okena-terminal/src/pty_manager.rs b/crates/okena-terminal/src/pty_manager.rs index 991376ff7..8b913f020 100644 --- a/crates/okena-terminal/src/pty_manager.rs +++ b/crates/okena-terminal/src/pty_manager.rs @@ -1600,10 +1600,22 @@ impl PtyManager { join_reader_handle(handle.reader_handle.take(), &id); } Err(e) => { - // ECHILD commonly means the reader already reaped it. There is - // no child left to wait for, so join the reader normally. - log::debug!("PTY child {} already reaped or wait failed: {}", id, e); - join_reader_handle(handle.reader_handle.take(), &id); + // An indeterminate child status must not synchronously join the + // reader: a transient wait error can still leave both child and + // reader live. Retain the handle in the bounded reaper instead. + log::debug!("PTY child {} status is indeterminate: {}", id, e); + if let Some(tracker) = tracker { + tracker.queued(); + } + if let Some(tx) = reaper_tx { + if let Err(error) = tx.send_blocking(ReaperJob { handle }) { + log::error!("PTY reaper queue closed for {}; retaining live handle", id); + std::mem::forget(error.into_inner()); + } + } else { + log::error!("PTY reaper unavailable for {}; retaining live handle", id); + std::mem::forget(handle); + } } Ok(None) => { // Transfer the still-live handle to the manager-owned fixed reaper @@ -2402,6 +2414,7 @@ mod tests { #[derive(Clone, Debug)] struct DelayedTerminationChild { release: Arc<(Mutex, Condvar)>, + try_wait_error: bool, } #[cfg(unix)] @@ -2418,6 +2431,9 @@ mod tests { #[cfg(unix)] impl Child for DelayedTerminationChild { fn try_wait(&mut self) -> std::io::Result> { + if self.try_wait_error { + return Err(std::io::Error::other("indeterminate child status")); + } let released = *self.release.0.lock(); Ok(released.then(|| portable_pty::ExitStatus::with_exit_code(0))) } @@ -2437,7 +2453,7 @@ mod tests { #[cfg(unix)] #[test] - fn shutdown_does_not_block_on_a_child_or_reader_that_ignores_termination() { + fn shutdown_transfers_try_wait_errors_with_blocking_wait_to_reaper() { let child_release = Arc::new((Mutex::new(false), Condvar::new())); let reader_release = Arc::new((Mutex::new(false), Condvar::new())); let reader_wait = reader_release.clone(); @@ -2459,6 +2475,7 @@ mod tests { master: None, child: Box::new(DelayedTerminationChild { release: child_release.clone(), + try_wait_error: true, }), input_tx: None, writer: None, @@ -2531,6 +2548,7 @@ mod tests { master: None, child: Box::new(DelayedTerminationChild { release: child_release, + try_wait_error: false, }), input_tx: None, writer: Some(Arc::new(Mutex::new(Box::new(DropNotifyingWriter(Some( @@ -2582,6 +2600,7 @@ mod tests { master: None, child: Box::new(DelayedTerminationChild { release: Arc::clone(&child_release), + try_wait_error: false, }), input_tx: None, writer: None, diff --git a/crates/okena-terminal/src/session_backend.rs b/crates/okena-terminal/src/session_backend.rs index e3f04a75e..969a18661 100644 --- a/crates/okena-terminal/src/session_backend.rs +++ b/crates/okena-terminal/src/session_backend.rs @@ -431,20 +431,9 @@ impl ResolvedBackend { pub fn kill_session(&self, session_name: &str) -> bool { match self { Self::None => true, - Self::Tmux => verify_session_kill( - session_backend_output("tmux", &["kill-session", "-t", session_name]), - || { - session_backend_output("tmux", &["has-session", "-t", session_name]) - .map(|output| output.status.success()) - }, - std::time::Duration::from_secs(2), - ), - Self::Screen => verify_session_kill( - session_backend_output("screen", &["-S", session_name, "-X", "quit"]), - || { - session_backend_output("screen", &["-S", session_name, "-Q", "select", "."]) - .map(|output| output.status.success()) - }, + Self::Tmux | Self::Screen | Self::Psmux => self.kill_session_with_executor( + session_name, + session_backend_output, std::time::Duration::from_secs(2), ), Self::Dtach => { @@ -531,16 +520,39 @@ impl ResolvedBackend { } true } - Self::Psmux => verify_session_kill( - session_backend_output("psmux", &["kill-session", "-t", session_name]), - || { - session_backend_output("psmux", &["has-session", "-t", session_name]) - .map(|output| output.status.success()) - }, - std::time::Duration::from_secs(2), - ), } } + + fn kill_session_with_executor( + &self, + session_name: &str, + mut execute: impl FnMut(&str, &[&str]) -> std::io::Result, + timeout: std::time::Duration, + ) -> bool { + let (program, kill_args, probe_args) = match self { + Self::Tmux => ( + "tmux", + vec!["kill-session", "-t", session_name], + vec!["has-session", "-t", session_name], + ), + Self::Screen => ( + "screen", + vec!["-S", session_name, "-X", "quit"], + vec!["-S", session_name, "-Q", "select", "."], + ), + Self::Psmux => ( + "psmux", + vec!["kill-session", "-t", session_name], + vec!["has-session", "-t", session_name], + ), + Self::None | Self::Dtach => unreachable!("backend does not use command verification"), + }; + verify_session_kill( + execute(program, &kill_args), + || execute(program, &probe_args).map(|output| output.status.success()), + timeout, + ) + } } fn session_backend_output(program: &str, args: &[&str]) -> std::io::Result { @@ -1279,14 +1291,22 @@ fn is_screen_available() -> bool { mod tests { use super::*; - #[cfg(unix)] + fn command_output(success: bool) -> std::process::Output { + #[cfg(windows)] + let mut command = { + let mut command = std::process::Command::new("cmd.exe"); + command.args(["/C", if success { "exit 0" } else { "exit 1" }]); + command + }; + #[cfg(not(windows))] + let mut command = std::process::Command::new(if success { "true" } else { "false" }); + command.output().expect("run command") + } + fn successful_command_output() -> std::process::Output { - std::process::Command::new("true") - .output() - .expect("run true") + command_output(true) } - #[cfg(unix)] #[test] fn verified_session_kill_rejects_command_failure() { assert!(!verify_session_kill( @@ -1296,7 +1316,6 @@ mod tests { )); } - #[cfg(unix)] #[test] fn verified_session_kill_rejects_session_that_survives() { let mut probes = 0; @@ -1311,7 +1330,6 @@ mod tests { assert_eq!(probes, 1, "the live session was probed before failure"); } - #[cfg(unix)] #[test] fn verified_session_kill_accepts_confirmed_disappearance() { assert!(verify_session_kill( @@ -1321,6 +1339,91 @@ mod tests { )); } + #[test] + fn command_backends_issue_exact_kill_and_probe_commands() { + let cases = [ + ( + ResolvedBackend::Tmux, + "tmux", + ["kill-session", "-t", "tm-test"].as_slice(), + ["has-session", "-t", "tm-test"].as_slice(), + ), + ( + ResolvedBackend::Screen, + "screen", + ["-S", "tm-test", "-X", "quit"].as_slice(), + ["-S", "tm-test", "-Q", "select", "."].as_slice(), + ), + ( + ResolvedBackend::Psmux, + "psmux", + ["kill-session", "-t", "tm-test"].as_slice(), + ["has-session", "-t", "tm-test"].as_slice(), + ), + ]; + + for (backend, program, kill_args, probe_args) in cases { + let mut calls = Vec::new(); + let mut invocation = 0; + assert!(backend.kill_session_with_executor( + "tm-test", + |actual_program, actual_args| { + calls.push(( + actual_program.to_string(), + actual_args + .iter() + .map(ToString::to_string) + .collect::>(), + )); + invocation += 1; + Ok(command_output(invocation == 1)) + }, + std::time::Duration::ZERO, + )); + assert_eq!( + calls, + vec![ + ( + program.to_string(), + kill_args.iter().map(ToString::to_string).collect(), + ), + ( + program.to_string(), + probe_args.iter().map(ToString::to_string).collect(), + ), + ], + "{backend:?} must verify its exact session after killing it" + ); + } + } + + #[test] + fn command_backend_kill_failure_preserves_dependent_checkout() { + for backend in [ + ResolvedBackend::Tmux, + ResolvedBackend::Screen, + ResolvedBackend::Psmux, + ] { + let mut calls = Vec::new(); + assert!(!backend.kill_session_with_executor( + "tm-test", + |program, args| { + calls.push(( + program.to_string(), + args.iter().map(ToString::to_string).collect::>(), + )); + Err(std::io::Error::other("kill failed")) + }, + std::time::Duration::ZERO, + )); + assert_eq!( + calls.len(), + 1, + "{backend:?} must not probe after kill failure" + ); + } + } + #[test] fn test_parse_backend() { assert_eq!(SessionBackend::parse_str("tmux"), SessionBackend::Tmux); From 7854350426f0aa3c9a12c3003a16a65db75ffd94 Mon Sep 17 00:00:00 2001 From: Jakub Date: Thu, 23 Jul 2026 17:17:28 +0200 Subject: [PATCH 07/11] fix(terminal): reap orphaned persistent sessions Reconcile dtach sessions against authoritative post-lock workspace state, isolate named-profile sockets, and tear down verified descendant trees before removing their masters. --- .../views/overlays/profile_manager/actions.rs | 6 +- crates/okena-core/src/profiles.rs | 105 ++- crates/okena-daemon-core/src/daemon.rs | 160 +++- crates/okena-terminal/CLAUDE.md | 3 +- crates/okena-terminal/src/macos_proc.rs | 6 + crates/okena-terminal/src/session_backend.rs | 748 +++++++++++++++--- 6 files changed, 927 insertions(+), 101 deletions(-) diff --git a/crates/okena-app/src/views/overlays/profile_manager/actions.rs b/crates/okena-app/src/views/overlays/profile_manager/actions.rs index 12f658bb9..d81b67b69 100644 --- a/crates/okena-app/src/views/overlays/profile_manager/actions.rs +++ b/crates/okena-app/src/views/overlays/profile_manager/actions.rs @@ -45,7 +45,11 @@ impl ProfileManager { } pub(super) fn delete_profile(&mut self, id: &str, cx: &mut Context) { - match okena_core::profiles::delete_profile(id) { + match okena_core::profiles::delete_profile_with_cleanup(id, || { + okena_terminal::session_backend::reap_dtach_profile_sessions(id) + .map(|_| ()) + .map_err(anyhow::Error::from) + }) { Ok(()) => { self.show_delete_confirmation = None; self.refresh_profiles(); diff --git a/crates/okena-core/src/profiles.rs b/crates/okena-core/src/profiles.rs index d307ec2e7..4fb5d6a04 100644 --- a/crates/okena-core/src/profiles.rs +++ b/crates/okena-core/src/profiles.rs @@ -308,11 +308,88 @@ pub fn all_profiles() -> Result> { Ok(ProfileIndex::load(&root)?.profiles) } -/// Delete a profile. Refuses to delete the active profile, the default profile, or a -/// profile whose `remote.json` points to a live PID. Removes the profile directory and -/// updates `profiles.json` (index written first so partial FS failure leaves index clean). -/// Claude credentials at `~/.claude-/` are intentionally preserved. +/// Runtime root used by persistent dtach sessions. +pub fn dtach_socket_base_dir() -> PathBuf { + if let Ok(runtime_dir) = std::env::var("XDG_RUNTIME_DIR") { + PathBuf::from(runtime_dir).join("okena") + } else { + #[cfg(unix)] + { + // SAFETY: `getuid(2)` takes no arguments, dereferences no pointers, + // and is documented as never failing. + let uid = unsafe { libc::getuid() }; + PathBuf::from(format!("/tmp/okena-{uid}")) + } + #[cfg(not(unix))] + { + std::env::temp_dir().join("okena") + } + } +} + +/// Profile-isolated dtach directory. The default retains the legacy root for +/// backward compatibility; named profiles use a nested runtime directory. +pub fn dtach_socket_dir_for_profile(profile_id: &str) -> PathBuf { + let base = dtach_socket_base_dir(); + if profile_id == "default" { + base + } else { + base.join("profiles").join(profile_id) + } +} + +#[cfg(unix)] +fn ensure_profile_runtime_removable(runtime_dir: &Path, profile_id: &str) -> Result<()> { + let Ok(entries) = std::fs::read_dir(runtime_dir) else { + return Ok(()); + }; + for path in entries.flatten().map(|entry| entry.path()) { + let is_dtach_socket = path + .file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| name.starts_with("tm-") && name.ends_with(".sock")); + if !is_dtach_socket { + continue; + } + match std::os::unix::net::UnixStream::connect(&path) { + Ok(_) => { + bail!( + "Cannot delete profile '{profile_id}' while persistent terminal sessions are still running; open the profile and close its terminals first" + ); + } + Err(error) + if matches!( + error.kind(), + std::io::ErrorKind::NotFound | std::io::ErrorKind::ConnectionRefused + ) => + { + let _ = std::fs::remove_file(path); + } + Err(error) => { + bail!("Cannot verify terminal cleanup for profile '{profile_id}': {error}"); + } + } + } + Ok(()) +} + +#[cfg(not(unix))] +fn ensure_profile_runtime_removable(_runtime_dir: &Path, _profile_id: &str) -> Result<()> { + Ok(()) +} + +/// Delete a profile. Refuses to delete the active profile, the default profile, a +/// profile whose `remote.json` points to a live PID, or one with live persistent +/// terminal sessions. Removes the profile directory and updates `profiles.json` +/// (index written first so partial FS failure leaves index clean). Claude +/// credentials at `~/.claude-/` are intentionally preserved. pub fn delete_profile(id: &str) -> Result<()> { + delete_profile_with_cleanup(id, || Ok(())) +} + +/// Delete a profile after running a caller-provided terminal cleanup, but only +/// after the usual default/active/running guards have succeeded. +pub fn delete_profile_with_cleanup(id: &str, cleanup: impl FnOnce() -> Result<()>) -> Result<()> { let root = config_root(); let mut index = ProfileIndex::load(&root)?; @@ -335,6 +412,9 @@ pub fn delete_profile(id: &str) -> Result<()> { if is_profile_running(&paths) { bail!("Profile '{id}' is currently in use by another Okena instance"); } + cleanup()?; + let runtime_dir = dtach_socket_dir_for_profile(id); + ensure_profile_runtime_removable(&runtime_dir, id)?; index.profiles.retain(|p| p.id != id); if index.last_used.as_deref() == Some(id) { @@ -343,6 +423,7 @@ pub fn delete_profile(id: &str) -> Result<()> { index.save(&root)?; let _ = std::fs::remove_dir_all(&paths.root); + let _ = std::fs::remove_dir_all(runtime_dir); Ok(()) } @@ -1153,6 +1234,22 @@ mod tests { assert_eq!(loaded.profiles.len(), idx.profiles.len()); } + #[cfg(unix)] + #[test] + fn profile_runtime_guard_refuses_live_sessions_and_removes_dead_sockets() { + let dir = temp_root(); + let socket_path = dir.path().join("tm-live.sock"); + let listener = std::os::unix::net::UnixListener::bind(&socket_path).unwrap(); + + let error = ensure_profile_runtime_removable(dir.path(), "work").unwrap_err(); + assert!(error.to_string().contains("persistent terminal sessions")); + assert!(socket_path.exists()); + + drop(listener); + ensure_profile_runtime_removable(dir.path(), "work").unwrap(); + assert!(!socket_path.exists()); + } + #[test] fn test_delete_profile_refuses_default() { let dir = temp_root(); diff --git a/crates/okena-daemon-core/src/daemon.rs b/crates/okena-daemon-core/src/daemon.rs index 389344bfa..7b5026f0f 100644 --- a/crates/okena-daemon-core/src/daemon.rs +++ b/crates/okena-daemon-core/src/daemon.rs @@ -67,7 +67,7 @@ use okena_remote_server::server::RemoteServer; use okena_terminal::TerminalsRegistry; use okena_terminal::backend::{LocalBackend, TerminalBackend, TerminalSessionTeardown}; use okena_terminal::pty_manager::{PtyEvent, PtyManager}; -use okena_terminal::session_backend::SessionBackend; +use okena_terminal::session_backend::{SessionBackend, reconcile_dtach_sessions}; use okena_workspace::persistence::{self, AppSettings, LockGuard, acquire_instance_lock}; use okena_workspace::state::{Workspace, WorkspaceData}; use parking_lot::Mutex; @@ -76,6 +76,77 @@ use tokio::sync::{mpsc, watch}; use crate::daemon_config::DaemonConfig; use crate::reactor::DaemonReactor; +fn workspace_terminal_ids(data: &WorkspaceData) -> HashSet { + data.projects + .iter() + .flat_map(|project| { + let mut ids = project + .layout + .as_ref() + .map_or_else(Vec::new, okena_state::LayoutNode::collect_terminal_ids); + ids.extend(project.service_terminals.values().cloned()); + ids.extend(project.hook_terminals.keys().cloned()); + ids + }) + .collect() +} + +/// The default profile owns the pre-profile shared dtach directory. During the +/// migration window, preserve terminals referenced by every profile before +/// classifying a legacy socket as orphaned. Named profiles reconcile only their +/// isolated directories, so they need no cross-profile state. +fn reconciliation_terminal_ids(data: &WorkspaceData) -> Option> { + let mut retained = workspace_terminal_ids(data); + let Some(active_profile) = okena_core::profiles::try_current() else { + return Some(retained); + }; + if active_profile.id != "default" { + return Some(retained); + } + + let index = match okena_core::profiles::ProfileIndex::load(&active_profile.config_root) { + Ok(index) => index, + Err(error) => { + log::warn!("Skipping dtach reconciliation: cannot read profile index: {error:#}"); + return None; + } + }; + for profile in index + .profiles + .iter() + .filter(|profile| profile.id != active_profile.id) + { + let path = active_profile + .config_root + .join("profiles") + .join(&profile.id) + .join("workspace.json"); + let content = match std::fs::read_to_string(&path) { + Ok(content) => content, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => continue, + Err(error) => { + log::warn!( + "Skipping dtach reconciliation: cannot read {}: {error}", + path.display() + ); + return None; + } + }; + let profile_workspace: WorkspaceData = match serde_json::from_str(&content) { + Ok(workspace) => workspace, + Err(error) => { + log::warn!( + "Skipping dtach reconciliation: cannot parse {}: {error}", + path.display() + ); + return None; + } + }; + retained.extend(workspace_terminal_ids(&profile_workspace)); + } + Some(retained) +} + fn kill_stale_terminal_sessions( backend: &dyn TerminalBackend, sessions: &[TerminalSessionTeardown], @@ -203,6 +274,7 @@ impl DaemonCore { /// The reactor tasks are NOT started here — that is [`run`](DaemonCore::run)'s /// job (they need a `LocalSet`). pub fn new(params: DaemonParams) -> anyhow::Result { + let mut params = params; // ── 0. Acquire the single-writer instance lock FIRST ───────────────── // §5: exactly one process owns the profile's persistence + lock. The // daemon is that process; the `--daemon-client` GUI deliberately skips @@ -211,6 +283,38 @@ impl DaemonCore { // lifetime (dropped at the end of `run`). let instance_lock = acquire_instance_lock()?; + // The caller loads before it can acquire this lock. Re-read now so an + // outgoing owner cannot save newer authoritative state between that + // initial snapshot and our reconciliation pass. + let workspace_revalidated = match persistence::load_workspace_with_cleanup_for_shell( + params.session_backend, + ¶ms.settings.default_shell, + ) { + Ok(latest) => { + params.workspace_data = latest.data; + params.stale_terminal_ids = latest.stale_terminal_ids; + true + } + Err(error) => { + // The caller snapshot predates this lock and is therefore not + // safe authority for destructive reconciliation. + log::warn!( + "Could not revalidate workspace after acquiring the instance lock; skipping dtach reconciliation and using the caller snapshot: {error:#}" + ); + false + } + }; + + // Reconcile only after acquiring the profile's single-writer lock and + // successfully reloading authoritative state, before starting a PTY + // manager. This closes the crash window where workspace state no longer + // owns a terminal but its persistent dtach process tree survived. + if workspace_revalidated + && let Some(retained_terminal_ids) = reconciliation_terminal_ids(¶ms.workspace_data) + { + reconcile_dtach_sessions(&retained_terminal_ids); + } + // ── 1. Multi-thread tokio runtime backing the reactor ──────────────── let runtime = tokio::runtime::Builder::new_multi_thread() .enable_all() @@ -672,6 +776,60 @@ mod shutdown_tests { } } + #[test] + fn startup_retains_every_workspace_owned_terminal_kind() { + let mut data = WorkspaceData::empty(); + let mut project = okena_state::ProjectData { + id: "p1".to_string(), + name: "Project".to_string(), + path: "/tmp".to_string(), + layout: Some(okena_state::LayoutNode::Terminal { + terminal_id: Some("layout".to_string()), + minimized: false, + detached: false, + shell_type: Default::default(), + zoom_level: 1.0, + }), + terminal_names: HashMap::new(), + hidden_terminals: HashMap::new(), + worktree_info: None, + worktree_ids: Vec::new(), + folder_color: Default::default(), + hooks: Default::default(), + is_remote: false, + connection_id: None, + service_terminals: HashMap::from([("web".to_string(), "service".to_string())]), + default_shell: None, + hook_terminals: HashMap::from([( + "hook".to_string(), + okena_state::HookTerminalEntry { + label: "Hook".to_string(), + status: okena_state::HookTerminalStatus::Running, + hook_type: "on_project_open".to_string(), + command: "true".to_string(), + cwd: "/tmp".to_string(), + }, + )]), + pinned: false, + last_activity_at: None, + is_creating: false, + is_closing: false, + }; + project + .terminal_names + .insert("layout".to_string(), "Shell".to_string()); + data.projects.push(project); + + assert_eq!( + workspace_terminal_ids(&data), + HashSet::from([ + "layout".to_string(), + "service".to_string(), + "hook".to_string(), + ]) + ); + } + #[test] fn startup_kills_sessions_owned_by_discarded_worktrees() { let killed = Arc::new(Mutex::new(Vec::new())); diff --git a/crates/okena-terminal/CLAUDE.md b/crates/okena-terminal/CLAUDE.md index df94f8c4c..e8e807187 100644 --- a/crates/okena-terminal/CLAUDE.md +++ b/crates/okena-terminal/CLAUDE.md @@ -36,5 +36,6 @@ See the doc comments on `pub struct Terminal` in `terminal.rs` for per-field thr - **`TerminalsRegistry`**: `Arc>>>` — shared registry for PTY event routing. - **Batched PTY processing**: The PTY reader thread sends `PtyEvent::Data` via `async_channel`. The GPUI thread drains all pending events before notifying, avoiding per-byte UI updates. -- **Remote output decoupling**: Remote tokio reader calls `enqueue_output` (just appends to `pending_output` + sets `dirty`). The GPUI thread drains via `drain_pending_output` inside `with_content`, so `term.lock()` is never held on the tokio thread. +- **Remote output decoupling**: Remote tokio readers call `enqueue_output` (append to `pending_output` + set `dirty`) and ring the manager's capacity-1 activity doorbell. The GPUI-thread activity pump drains/parses output, then emits targeted pane/sidebar notifications; `with_content` remains the fallback drain. Never restore per-pane polling or hold `term.lock()` on the tokio thread. +- **Persistent dtach teardown**: `SIGTERM` to the dtach master does not propagate to its PTY child tree. Teardown must keep the socket discoverable, revalidate PID birth markers, quiesce/reap descendants before the master, and unlink only after socket death is verified. - **Shell detection**: Auto-detects available shells on the system. On Windows, detects WSL distros and converts paths (`C:\` → `/mnt/c/`). diff --git a/crates/okena-terminal/src/macos_proc.rs b/crates/okena-terminal/src/macos_proc.rs index 6abdc8530..f37505b5d 100644 --- a/crates/okena-terminal/src/macos_proc.rs +++ b/crates/okena-terminal/src/macos_proc.rs @@ -54,6 +54,12 @@ pub fn process_tree() -> HashMap> { tree } +/// Stable process birth marker used to revalidate a PID before signalling it. +pub fn process_start_time(pid: u32) -> Option<(u64, u64)> { + let info = pidinfo::(pid as i32, 0).ok()?; + Some((info.pbi_start_tvsec, info.pbi_start_tvusec)) +} + /// Map each given unix-socket path to the pids that have it open — equivalent to /// `lsof `. Scans every process's socket fds and matches the bound /// unix-domain address against the requested paths (exact match, like the lsof diff --git a/crates/okena-terminal/src/session_backend.rs b/crates/okena-terminal/src/session_backend.rs index 969a18661..2c2728fd0 100644 --- a/crates/okena-terminal/src/session_backend.rs +++ b/crates/okena-terminal/src/session_backend.rs @@ -440,76 +440,13 @@ impl ResolvedBackend { let socket_path = get_dtach_socket_path(session_name); if socket_path.exists() { #[cfg(unix)] - { - let my_pid = std::process::id() as i32; - // Discover the PIDs holding the dtach socket open. This is a - // best-effort, point-in-time snapshot (now via the /proc socket - // scan instead of an `lsof -t` spawn), with an inherent TOCTOU - // window between reading it here and signalling below. By the - // time we call `kill`, the dtach process may have already exited - // and its PID been recycled onto an unrelated process. We accept - // this risk because there is no portable, race-free way to - // atomically "signal whoever holds this socket"; the window is - // short and the dtach socket is user-private (see - // get_dtach_socket_dir). - let holders = crate::pty_manager::find_pids_for_unix_sockets( - std::slice::from_ref(&socket_path), - ); - let mut signalled = Vec::new(); - for &pid in holders.get(&socket_path).into_iter().flatten() { - let pid = pid as i32; - if pid == my_pid { - log::debug!( - "Skipping own PID {} when killing dtach session {}", - pid, - session_name - ); - continue; - } - // SAFETY: `libc::kill` is a thin FFI wrapper over the - // `kill(2)` syscall. It takes two plain `i32` values - // (a pid and a signal number) by value, dereferences no - // pointers, and has no memory-safety preconditions, so - // the call itself cannot cause UB regardless of the - // argument values. The only hazard is *logical*, not a - // memory-safety one: per the TOCTOU note above, `pid` - // may have been recycled since the scan, so we could - // signal an unrelated process. We tolerate that as - // best-effort cleanup and intentionally ignore the - // return value (the process may already be gone). - unsafe { - libc::kill(pid, libc::SIGTERM); - } - signalled.push(pid); - log::debug!( - "Sent SIGTERM to dtach process {} for session {}", - pid, - session_name - ); - } - if !wait_for_pids_to_exit(&signalled) { - log::error!( - "dtach session {} still has a live holder after SIGKILL; preserving dependent checkout", - session_name - ); - return false; - } - let remaining = crate::pty_manager::find_pids_for_unix_sockets( - std::slice::from_ref(&socket_path), - ); - if remaining - .get(&socket_path) - .into_iter() - .flatten() - .any(|pid| *pid as i32 != my_pid) - { - log::error!( - "dtach session {} still has a socket owner; preserving dependent checkout", - session_name - ); - return false; - } + if !terminate_dtach_process_tree(&socket_path, session_name) { + // Keep the socket path as the durable retry handle. Unlinking + // it while either the master or its child tree survives is + // what made leaked agent trees invisible to later cleanup. + return false; } + if let Err(error) = std::fs::remove_file(&socket_path) && error.kind() != std::io::ErrorKind::NotFound { @@ -640,10 +577,319 @@ fn wait_for_pids_to_exit(pids: &[i32]) -> bool { } } -/// Minimum age before a `tm-*.sock` file is a GC candidate. A socket created -/// just before this scan may not yet appear in the `/proc` snapshot, so treat -/// recent files as live (TOCTOU defense-in-depth on top of the name filter). #[cfg(unix)] +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] +struct TrackedProcess { + pid: i32, + /// Platform process-birth marker. Revalidating this before every signal + /// prevents a recycled PID from targeting an unrelated process. + start_marker: Option, +} + +#[cfg(unix)] +fn raw_process_is_alive(pid: i32) -> bool { + // SAFETY: signal 0 performs existence/permission checking only and takes no + // pointers. All callers pass a positive PID discovered from process state. + unsafe { libc::kill(pid, 0) == 0 } +} + +#[cfg(target_os = "macos")] +fn process_start_marker(pid: i32) -> Option { + let (seconds, micros) = crate::macos_proc::process_start_time(pid as u32)?; + Some(((seconds as u128) << 64) | micros as u128) +} + +#[cfg(target_os = "linux")] +fn linux_process_stat(pid: i32) -> Option<(i32, u64)> { + let stat = std::fs::read_to_string(format!("/proc/{pid}/stat")).ok()?; + // `comm` is parenthesized and may itself contain spaces or `)`; the final + // ") " delimiter is the only safe place to begin fixed-field parsing. + let suffix = stat.rsplit_once(") ")?.1; + let fields: Vec<&str> = suffix.split_whitespace().collect(); + let parent_pid = fields.get(1)?.parse().ok()?; // field 4 + let start_ticks = fields.get(19)?.parse().ok()?; // field 22 + Some((parent_pid, start_ticks)) +} + +#[cfg(target_os = "linux")] +fn process_start_marker(pid: i32) -> Option { + linux_process_stat(pid).map(|(_, start_ticks)| start_ticks as u128) +} + +#[cfg(all(unix, not(target_os = "linux"), not(target_os = "macos")))] +fn process_start_marker(_pid: i32) -> Option { + None +} + +#[cfg(unix)] +fn tracked_process(pid: i32) -> Option { + let start_marker = process_start_marker(pid); + (start_marker.is_some() || raw_process_is_alive(pid)) + .then_some(TrackedProcess { pid, start_marker }) +} + +#[cfg(unix)] +fn same_process_is_alive(process: TrackedProcess) -> bool { + match process.start_marker { + Some(marker) => process_start_marker(process.pid) == Some(marker), + None => raw_process_is_alive(process.pid), + } +} + +#[cfg(target_os = "macos")] +fn process_tree_snapshot() -> std::collections::HashMap> { + crate::macos_proc::process_tree() + .into_iter() + .map(|(parent, children)| { + ( + parent as i32, + children.into_iter().map(|pid| pid as i32).collect(), + ) + }) + .collect() +} + +#[cfg(target_os = "linux")] +fn process_tree_snapshot() -> std::collections::HashMap> { + let mut tree = std::collections::HashMap::new(); + let Ok(entries) = std::fs::read_dir("/proc") else { + return tree; + }; + for entry in entries.flatten() { + let Some(pid) = entry + .file_name() + .to_str() + .and_then(|name| name.parse::().ok()) + else { + continue; + }; + if let Some((parent_pid, _)) = linux_process_stat(pid) { + tree.entry(parent_pid).or_insert_with(Vec::new).push(pid); + } + } + tree +} + +#[cfg(all(unix, not(target_os = "linux"), not(target_os = "macos")))] +fn process_tree_snapshot() -> std::collections::HashMap> { + let mut tree = std::collections::HashMap::new(); + let Ok(output) = crate::process::command("ps") + .args(["-axo", "pid=,ppid="]) + .output() + else { + return tree; + }; + for line in String::from_utf8_lossy(&output.stdout).lines() { + let mut fields = line.split_whitespace(); + let (Some(pid), Some(parent)) = (fields.next(), fields.next()) else { + continue; + }; + if let (Ok(pid), Ok(parent)) = (pid.parse::(), parent.parse::()) { + tree.entry(parent).or_insert_with(Vec::new).push(pid); + } + } + tree +} + +#[cfg(unix)] +fn tracked_descendants(roots: &[TrackedProcess]) -> Vec { + fn visit( + pid: i32, + tree: &std::collections::HashMap>, + visited: &mut std::collections::HashSet, + descendants: &mut Vec, + ) { + let Some(children) = tree.get(&pid) else { + return; + }; + for &child in children { + if !visited.insert(child) { + continue; + } + visit(child, tree, visited, descendants); + if let Some(process) = tracked_process(child) { + descendants.push(process); + } + } + } + + let tree = process_tree_snapshot(); + let mut visited: std::collections::HashSet = + roots.iter().map(|process| process.pid).collect(); + let mut descendants = Vec::new(); + for &root in roots { + if same_process_is_alive(root) { + visit(root.pid, &tree, &mut visited, &mut descendants); + } + } + descendants +} + +#[cfg(unix)] +fn signal_tracked_processes(processes: &[TrackedProcess], signal: i32, session_name: &str) { + for &process in processes { + if !same_process_is_alive(process) { + continue; + } + // SAFETY: `kill(2)` takes plain integer values and no pointers. The PID + // has just been revalidated against its platform birth marker. + unsafe { + libc::kill(process.pid, signal); + } + log::debug!( + "Sent signal {signal} to process {} for dtach session {session_name}", + process.pid + ); + } +} + +#[cfg(unix)] +fn dtach_socket_holders(socket_path: &std::path::PathBuf) -> Vec { + let my_pid = std::process::id() as i32; + crate::pty_manager::find_pids_for_unix_sockets(std::slice::from_ref(socket_path)) + .remove(socket_path) + .unwrap_or_default() + .into_iter() + .map(|pid| pid as i32) + .filter(|pid| *pid != my_pid) + .collect() +} + +#[cfg(unix)] +fn tracked_dtach_socket_holders(socket_path: &std::path::PathBuf) -> Vec { + let tracked: Vec = dtach_socket_holders(socket_path) + .into_iter() + .filter_map(tracked_process) + .collect(); + let current: std::collections::HashSet = + dtach_socket_holders(socket_path).into_iter().collect(); + tracked + .into_iter() + .filter(|process| current.contains(&process.pid) && same_process_is_alive(*process)) + .collect() +} + +#[cfg(unix)] +fn dtach_socket_is_definitively_dead(socket_path: &std::path::Path) -> bool { + match std::os::unix::net::UnixStream::connect(socket_path) { + Ok(_) => false, + Err(error) => matches!( + error.kind(), + std::io::ErrorKind::NotFound | std::io::ErrorKind::ConnectionRefused + ), + } +} + +#[cfg(unix)] +fn terminate_dtach_process_tree(socket_path: &std::path::PathBuf, session_name: &str) -> bool { + let mut holders = tracked_dtach_socket_holders(socket_path); + if holders.is_empty() { + if dtach_socket_is_definitively_dead(socket_path) { + return true; + } + log::warn!( + "Refusing to unlink live dtach session {session_name}: no socket holder PID was discoverable" + ); + return false; + } + + // Freeze verified holders first. Besides pinning their PID identities, this + // keeps the dtach master as a stable parentage anchor while descendants are + // discovered and frozen below. + signal_tracked_processes(&holders, libc::SIGSTOP, session_name); + let stopped_holders = holders.clone(); + std::thread::sleep(std::time::Duration::from_millis(10)); + let confirmed_holders: std::collections::HashSet = + tracked_dtach_socket_holders(socket_path) + .into_iter() + .collect(); + holders.retain(|holder| confirmed_holders.contains(holder)); + if holders.is_empty() { + signal_tracked_processes(&stopped_holders, libc::SIGCONT, session_name); + return dtach_socket_is_definitively_dead(socket_path); + } + + // dtach exits on SIGTERM without forwarding it to the child PTY process + // group. Iteratively freeze descendants parent-first until two snapshots are + // stable. Once every anchored process is SIGSTOPed, none can fork during the + // destructive pass and the socket remains a durable retry handle on failure. + let mut descendants = Vec::new(); + let mut seen = std::collections::HashSet::new(); + let mut stable_snapshots = 0; + for _ in 0..8 { + let snapshot = tracked_descendants(&holders); + let mut newly_seen: Vec = snapshot + .into_iter() + .filter(|process| seen.insert(*process)) + .collect(); + if newly_seen.is_empty() { + stable_snapshots += 1; + if stable_snapshots == 2 { + break; + } + } else { + stable_snapshots = 0; + // `tracked_descendants` is child-first; reverse it so spawning + // parents are stopped before their children. + newly_seen.reverse(); + signal_tracked_processes(&newly_seen, libc::SIGSTOP, session_name); + descendants.extend(newly_seen); + } + std::thread::sleep(std::time::Duration::from_millis(10)); + } + + if stable_snapshots < 2 { + signal_tracked_processes(&descendants, libc::SIGCONT, session_name); + signal_tracked_processes(&stopped_holders, libc::SIGCONT, session_name); + log::error!( + "Dtach session {session_name} descendant tree did not quiesce; preserving {:?} for retry", + socket_path + ); + return false; + } + + signal_tracked_processes(&descendants, libc::SIGKILL, session_name); + std::thread::sleep(std::time::Duration::from_millis(50)); + let surviving_descendants = descendants + .iter() + .filter(|process| same_process_is_alive(**process)) + .count(); + if surviving_descendants > 0 { + signal_tracked_processes(&descendants, libc::SIGCONT, session_name); + signal_tracked_processes(&stopped_holders, libc::SIGCONT, session_name); + log::error!( + "Dtach session {session_name} still has {surviving_descendants} live descendant(s); preserving {:?} for retry", + socket_path + ); + return false; + } + + // SIGTERM is queued while holders are stopped; SIGCONT lets dtach run its + // normal exit/unlink path. Escalate only freshly revalidated survivors. + signal_tracked_processes(&stopped_holders, libc::SIGTERM, session_name); + signal_tracked_processes(&stopped_holders, libc::SIGCONT, session_name); + std::thread::sleep(std::time::Duration::from_millis(50)); + + let surviving_holders = tracked_dtach_socket_holders(socket_path); + if !surviving_holders.is_empty() { + signal_tracked_processes(&surviving_holders, libc::SIGKILL, session_name); + std::thread::sleep(std::time::Duration::from_millis(50)); + } + + let terminated = dtach_socket_holders(socket_path).is_empty() + && dtach_socket_is_definitively_dead(socket_path); + if !terminated { + log::error!( + "Dtach session {session_name} is still live after teardown; preserving {:?} for retry", + socket_path + ); + } + terminated +} + +/// Minimum age before a `tm-*.sock` file is a GC candidate. A socket created +/// just before this scan may not yet appear in the process/socket snapshot, so +/// treat recent files as live. const DTACH_SOCKET_GC_MIN_AGE: std::time::Duration = std::time::Duration::from_secs(60); /// Whether a filename matches the dtach/tmux socket naming scheme (`tm-*.sock`). @@ -670,6 +916,105 @@ fn socket_age(path: &std::path::Path) -> Option { Some(modified.elapsed().unwrap_or(std::time::Duration::ZERO)) } +#[cfg(unix)] +fn dtach_session_name_from_path(path: &std::path::Path) -> Option { + let file_name = path.file_name()?.to_str()?; + is_stale_gc_candidate(file_name) + .then(|| file_name.strip_suffix(".sock").map(str::to_owned)) + .flatten() +} + +#[cfg(unix)] +fn orphaned_dtach_session_names<'a>( + socket_paths: impl IntoIterator, + retained_session_names: &std::collections::HashSet, +) -> Vec { + let mut orphaned: Vec = socket_paths + .into_iter() + .filter_map(|path| dtach_session_name_from_path(path)) + .filter(|name| !retained_session_names.contains(name)) + .collect(); + orphaned.sort(); + orphaned.dedup(); + orphaned +} + +/// Reconcile live dtach sessions against the workspace that owns this profile. +/// Sessions absent from authoritative state are leftovers from an interrupted or +/// incomplete close and must not survive another daemon start. +#[cfg(unix)] +pub fn reconcile_dtach_sessions(retained_terminal_ids: &std::collections::HashSet) { + // Always reconcile dtach artifacts, even when the newly selected backend is + // tmux/screen/none or Auto now resolves differently. + let backend = ResolvedBackend::Dtach; + let dir = get_dtach_socket_dir(); + let Ok(entries) = std::fs::read_dir(&dir) else { + return; + }; + let socket_paths: Vec = entries + .flatten() + .map(|entry| entry.path()) + .filter(|path| { + path.file_name() + .and_then(|name| name.to_str()) + .is_some_and(is_stale_gc_candidate) + }) + // A recently-created socket may belong to a terminal added after the + // caller's last disk snapshot. Reconciliation never destroys it. + .filter(|path| socket_age(path).is_some_and(|age| !is_too_fresh_to_gc(age))) + .collect(); + let retained_session_names: std::collections::HashSet = retained_terminal_ids + .iter() + .map(|terminal_id| backend.session_name(terminal_id)) + .collect(); + let orphaned = orphaned_dtach_session_names(socket_paths.iter(), &retained_session_names); + + for session_name in &orphaned { + backend.kill_session(session_name); + } + if !orphaned.is_empty() { + log::info!( + "Reconciled {} orphaned dtach session(s) in {:?}", + orphaned.len(), + dir + ); + } +} + +#[cfg(not(unix))] +pub fn reconcile_dtach_sessions(_retained_terminal_ids: &std::collections::HashSet) {} + +/// Tear down every persistent dtach session in a stopped profile before its +/// authoritative profile directory is deleted. +#[cfg(unix)] +pub fn reap_dtach_profile_sessions(profile_id: &str) -> std::io::Result { + let dir = okena_core::profiles::dtach_socket_dir_for_profile(profile_id); + let entries = match std::fs::read_dir(&dir) { + Ok(entries) => entries, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(0), + Err(error) => return Err(error), + }; + let mut reaped = 0; + for path in entries.flatten().map(|entry| entry.path()) { + let Some(session_name) = dtach_session_name_from_path(&path) else { + continue; + }; + if !terminate_dtach_process_tree(&path, &session_name) { + return Err(std::io::Error::other(format!( + "persistent terminal session {session_name} did not terminate" + ))); + } + let _ = std::fs::remove_file(path); + reaped += 1; + } + Ok(reaped) +} + +#[cfg(not(unix))] +pub fn reap_dtach_profile_sessions(_profile_id: &str) -> std::io::Result { + Ok(0) +} + /// Remove dtach socket files whose dtach process is no longer running. /// Called once at startup to clean up after crashes or ungraceful exits. /// @@ -705,7 +1050,7 @@ pub fn cleanup_stale_dtach_sockets() { let mut removed = 0; for path in &socket_paths { let has_listener = holders.get(path).map(|v| !v.is_empty()).unwrap_or(false); - if !has_listener { + if !has_listener && dtach_socket_is_definitively_dead(path) { let _ = std::fs::remove_file(path); removed += 1; } @@ -983,34 +1328,71 @@ fn shell_escape(s: &str) -> String { /// Get the socket directory for dtach sessions #[allow(dead_code)] +fn profile_scoped_dtach_socket_dir( + base: std::path::PathBuf, + profile_id: Option<&str>, +) -> std::path::PathBuf { + let Some(profile_id) = profile_id else { + return base; + }; + let safe = !profile_id.is_empty() + && !profile_id.contains('/') + && !profile_id.contains('\\') + && !profile_id.contains("..") + && !profile_id.contains('\0'); + if profile_id == "default" || !safe { + base + } else { + base.join("profiles").join(profile_id) + } +} + +fn dtach_socket_base_dir() -> std::path::PathBuf { + okena_core::profiles::dtach_socket_base_dir() +} + +fn active_profile_id() -> Option { + okena_core::profiles::try_current() + .map(|profile| profile.id.clone()) + .or_else(|| std::env::var("OKENA_PROFILE").ok()) +} + fn get_dtach_socket_dir() -> std::path::PathBuf { - // Use XDG_RUNTIME_DIR if available (Linux), otherwise fall back to temp dir - // XDG_RUNTIME_DIR is preferred as it's user-specific and cleaned on logout - if let Ok(runtime_dir) = std::env::var("XDG_RUNTIME_DIR") { - std::path::PathBuf::from(runtime_dir).join("okena") + // Keep the default profile in the legacy root so existing sessions survive + // the profile migration. Every named profile gets an isolated socket pool. + profile_scoped_dtach_socket_dir(dtach_socket_base_dir(), active_profile_id().as_deref()) +} + +fn profile_dtach_socket_path( + base: &std::path::Path, + profile_id: Option<&str>, + session_name: &str, +) -> std::path::PathBuf { + let file_name = format!("{session_name}.sock"); + let scoped = profile_scoped_dtach_socket_dir(base.to_path_buf(), profile_id).join(&file_name); + if scoped.exists() { + return scoped; + } + + let legacy = base.join(file_name); + if scoped != legacy && legacy.exists() { + legacy } else { - // Fallback: /tmp/okena- for security - #[cfg(unix)] - { - // SAFETY: `libc::getuid` is a thin FFI wrapper over the `getuid(2)` - // syscall. It takes no arguments, dereferences no pointers, always - // succeeds (it is documented as never failing), and returns a plain - // `uid_t` by value. There are no memory-safety preconditions, so the - // call cannot cause UB. - let uid = unsafe { libc::getuid() }; - std::path::PathBuf::from(format!("/tmp/okena-{}", uid)) - } - #[cfg(not(unix))] - { - std::env::temp_dir().join("okena") - } + scoped } } -/// Get the socket path for a specific dtach session +/// Get the socket path for a specific dtach session. A named profile first looks +/// in its isolated directory, then falls back to the pre-upgrade shared root so +/// retained legacy sessions remain attachable and closable. New sessions are +/// created in the isolated path once no legacy socket exists. #[allow(dead_code)] fn get_dtach_socket_path(session_name: &str) -> std::path::PathBuf { - get_dtach_socket_dir().join(format!("{}.sock", session_name)) + profile_dtach_socket_path( + &dtach_socket_base_dir(), + active_profile_id().as_deref(), + session_name, + ) } /// Extract directory name from a path for use as window name @@ -1465,6 +1847,184 @@ mod tests { assert!(!is_stale_gc_candidate("remote.json")); } + #[cfg(unix)] + #[test] + fn orphan_reconciliation_preserves_retained_sessions() { + let sockets = [ + std::path::PathBuf::from("/runtime/tm-keep1234.sock"), + std::path::PathBuf::from("/runtime/tm-drop5678.sock"), + std::path::PathBuf::from("/runtime/daemon.sock"), + ]; + let retained = std::collections::HashSet::from(["tm-keep1234".to_string()]); + + assert_eq!( + orphaned_dtach_session_names(sockets.iter(), &retained), + vec!["tm-drop5678".to_string()] + ); + } + + #[cfg(unix)] + #[test] + fn non_default_profiles_get_isolated_dtach_socket_directories() { + let base = std::path::PathBuf::from("/tmp/okena-501"); + + assert_eq!(profile_scoped_dtach_socket_dir(base.clone(), None), base); + assert_eq!( + profile_scoped_dtach_socket_dir(base.clone(), Some("default")), + base + ); + assert_eq!( + profile_scoped_dtach_socket_dir(base.clone(), Some("work-client")), + base.join("profiles").join("work-client") + ); + } + + #[cfg(unix)] + #[test] + fn named_profile_reuses_legacy_socket_before_creating_an_isolated_one() { + let base = std::env::temp_dir().join(format!( + "okena-profile-socket-test-{}-{}", + std::process::id(), + uuid::Uuid::new_v4() + )); + std::fs::create_dir_all(&base).unwrap(); + let legacy = base.join("tm-legacy.sock"); + std::fs::write(&legacy, b"").unwrap(); + + assert_eq!( + profile_dtach_socket_path(&base, Some("work"), "tm-legacy"), + legacy + ); + std::fs::remove_file(&legacy).unwrap(); + assert_eq!( + profile_dtach_socket_path(&base, Some("work"), "tm-legacy"), + base.join("profiles/work/tm-legacy.sock") + ); + let _ = std::fs::remove_dir_all(base); + } + + #[cfg(unix)] + #[test] + fn profile_reaping_refuses_live_sessions_and_removes_dead_sockets() { + let profile_id = format!("test-profile-{}", uuid::Uuid::new_v4()); + let dir = okena_core::profiles::dtach_socket_dir_for_profile(&profile_id); + std::fs::create_dir_all(&dir).unwrap(); + let socket_path = dir.join("tm-profile-test.sock"); + let listener = std::os::unix::net::UnixListener::bind(&socket_path).unwrap(); + + assert!(reap_dtach_profile_sessions(&profile_id).is_err()); + assert!(socket_path.exists()); + + drop(listener); + assert_eq!(reap_dtach_profile_sessions(&profile_id).unwrap(), 1); + assert!(!socket_path.exists()); + let _ = std::fs::remove_dir_all(dir); + } + + #[cfg(unix)] + #[test] + fn dtach_teardown_preserves_socket_until_its_master_is_dead() { + let session_name = format!("tm-live-test-{}", std::process::id()); + let socket_path = get_dtach_socket_path(&session_name); + std::fs::create_dir_all(socket_path.parent().expect("socket parent")).unwrap(); + let _ = std::fs::remove_file(&socket_path); + let listener = std::os::unix::net::UnixListener::bind(&socket_path).unwrap(); + + ResolvedBackend::Dtach.kill_session(&session_name); + assert!( + socket_path.exists(), + "a socket that still accepts connections must stay discoverable" + ); + + drop(listener); + ResolvedBackend::Dtach.kill_session(&session_name); + assert!( + !socket_path.exists(), + "a dead socket should be removed once liveness is verified" + ); + } + + #[cfg(unix)] + #[test] + fn dtach_teardown_reaps_the_real_child_process_tree() { + if std::process::Command::new("dtach") + .arg("--help") + .output() + .is_err() + { + return; + } + + let unique = format!("{}-{}", std::process::id(), uuid::Uuid::new_v4()); + let session_name = format!("tm-tree-test-{unique}"); + let socket_path = get_dtach_socket_path(&session_name); + std::fs::create_dir_all(socket_path.parent().expect("socket parent")).unwrap(); + let _ = std::fs::remove_file(&socket_path); + let temp_dir = std::env::temp_dir().join(format!("okena-dtach-tree-{unique}")); + std::fs::create_dir_all(&temp_dir).unwrap(); + let shell_pid_file = temp_dir.join("shell.pid"); + let child_pid_file = temp_dir.join("child.pid"); + let command = format!( + "echo $$ > {}; sleep 30 & echo $! > {}; wait", + shell_escape(&shell_pid_file.to_string_lossy()), + shell_escape(&child_pid_file.to_string_lossy()) + ); + let status = std::process::Command::new("dtach") + .args([ + "-n", + socket_path.to_str().unwrap(), + "-E", + "sh", + "-c", + &command, + ]) + .status() + .unwrap(); + assert!(status.success()); + + for _ in 0..100 { + if socket_path.exists() && shell_pid_file.exists() && child_pid_file.exists() { + break; + } + std::thread::sleep(std::time::Duration::from_millis(20)); + } + let shell_pid: i32 = std::fs::read_to_string(&shell_pid_file) + .unwrap() + .trim() + .parse() + .unwrap(); + let child_pid: i32 = std::fs::read_to_string(&child_pid_file) + .unwrap() + .trim() + .parse() + .unwrap(); + + ResolvedBackend::Dtach.kill_session(&session_name); + for _ in 0..100 { + if !raw_process_is_alive(shell_pid) && !raw_process_is_alive(child_pid) { + break; + } + std::thread::sleep(std::time::Duration::from_millis(20)); + } + let shell_alive = raw_process_is_alive(shell_pid); + let child_alive = raw_process_is_alive(child_pid); + if shell_alive { + // SAFETY: positive PID was written by this test-owned shell; kill(2) + // takes no pointers and cleanup ignores a concurrent exit. + unsafe { libc::kill(shell_pid, libc::SIGKILL) }; + } + if child_alive { + // SAFETY: positive PID was written by this test-owned child; kill(2) + // takes no pointers and cleanup ignores a concurrent exit. + unsafe { libc::kill(child_pid, libc::SIGKILL) }; + } + let _ = std::fs::remove_file(&socket_path); + let _ = std::fs::remove_dir_all(&temp_dir); + + assert!(!shell_alive, "dtach shell survived teardown"); + assert!(!child_alive, "dtach grandchild survived teardown"); + } + #[cfg(unix)] #[test] fn too_fresh_to_gc_respects_min_age() { From 5c411929dd005cddf9de26a8b72378689ec863bb Mon Sep 17 00:00:00 2001 From: Jakub Date: Thu, 23 Jul 2026 17:17:29 +0200 Subject: [PATCH 08/11] perf(terminal): replace pane polling with activity events Route remote terminal activity through the existing doorbell and app-wide pane registry instead of per-pane 8 ms timers. --- crates/okena-app/src/app/mod.rs | 11 +++- crates/okena-app/src/views/window/mod.rs | 64 +++++++++++++++++-- crates/okena-remote-client/src/manager.rs | 36 ++++------- crates/okena-terminal/src/terminal/io.rs | 7 +- .../src/layout/terminal_pane/mod.rs | 35 ---------- .../src/overlays/detached_terminal.rs | 32 +--------- src/main.rs | 4 +- 7 files changed, 90 insertions(+), 99 deletions(-) diff --git a/crates/okena-app/src/app/mod.rs b/crates/okena-app/src/app/mod.rs index dbfdcfc6e..fbe0fcf07 100644 --- a/crates/okena-app/src/app/mod.rs +++ b/crates/okena-app/src/app/mod.rs @@ -7,7 +7,9 @@ mod notifications; pub use detached_overlays::open_detached_overlay; use crate::remote_client::manager::{RemoteConnectionManager, RemoteManagerEvent}; -use crate::views::window::{TerminalsRegistry, WindowView}; +use crate::views::window::{ + TerminalsRegistry, WindowView, content_pane_registry, notify_registered_panes, +}; use crate::workspace::state::{GlobalWorkspace, WindowId, Workspace, WorkspaceData}; use gpui::*; use std::collections::{HashMap, HashSet}; @@ -359,6 +361,13 @@ impl Okena { cx.subscribe(&remote_manager, |this, _rm, event, cx| match event { RemoteManagerEvent::TerminalActivity(terminal_ids) => { if !terminal_ids.is_empty() { + // One app-wide fan-out refreshes panes in every main, extra, + // and detached window. Keeping this out of WindowView avoids + // duplicate notifications and does not depend on a specific + // OS window entity remaining alive. + let mut registry = content_pane_registry().lock(); + notify_registered_panes(&mut registry, terminal_ids, cx); + drop(registry); this.process_terminal_notifications(terminal_ids, cx); // Answer (or, when disabled, drop) OSC 52 clipboard *read* // requests for remote terminals. The clipboard physically diff --git a/crates/okena-app/src/views/window/mod.rs b/crates/okena-app/src/views/window/mod.rs index 92f3086de..30ab5b76e 100644 --- a/crates/okena-app/src/views/window/mod.rs +++ b/crates/okena-app/src/views/window/mod.rs @@ -61,6 +61,31 @@ pub fn notify_pane_weaks(weaks: &mut Vec>, cx: &mut Ap any_alive } +/// Notify panes for terminals whose remote content actually advanced. Empty +/// registrations are removed so repeated activity cannot grow stale weak lists. +pub fn notify_registered_panes( + registry: &mut HashMap>>, + terminal_ids: &[String], + cx: &mut App, +) -> usize { + let mut notified = 0; + let mut empty = Vec::new(); + for terminal_id in terminal_ids { + if let Some(weaks) = registry.get_mut(terminal_id) { + if notify_pane_weaks(weaks, cx) { + notified += 1; + } + if weaks.is_empty() { + empty.push(terminal_id.clone()); + } + } + } + for terminal_id in empty { + registry.remove(&terminal_id); + } + notified +} + /// Per-window view of the application: one instance per OS window. /// /// Owns the per-window UI state (sidebar, overlays, toasts, scroll handles, @@ -569,9 +594,8 @@ impl WindowView { // observer above. let sidebar_for_activity = self.sidebar.clone(); cx.subscribe(&manager, move |_this, _rm, event, cx| match event { - // The payload (advanced terminal ids) is for `Okena`'s - // notification drain; the sidebar re-reads every terminal's - // bell/idle flags, so it just repaints. + // The app-wide manager subscription performs targeted terminal + // pane fan-out once; each WindowView only owns its sidebar repaint. RemoteManagerEvent::TerminalActivity(_) => { sidebar_for_activity.update(cx, |_, cx| cx.notify()); } @@ -883,11 +907,43 @@ impl EventEmitter for WindowView {} #[cfg(test)] mod tests { - use super::notify_pane_weaks; + use super::{notify_pane_weaks, notify_registered_panes}; use gpui::AppContext as _; + use std::collections::HashMap; struct Stub; + #[gpui::test] + fn activity_notifies_only_registered_terminal_panes(cx: &mut gpui::TestAppContext) { + let (target, other, mut registry) = cx.update(|cx| { + let target = cx.new(|_| Stub); + let other = cx.new(|_| Stub); + let registry = HashMap::from([ + ("target".to_string(), vec![target.downgrade()]), + ("other".to_string(), vec![other.downgrade()]), + ]); + (target, other, registry) + }); + + cx.update(|cx| { + assert_eq!( + notify_registered_panes(&mut registry, &["target".to_string()], cx), + 1 + ); + assert_eq!(registry.len(), 2, "unrelated registrations stay intact"); + }); + + drop(target); + cx.update(|cx| { + assert_eq!( + notify_registered_panes(&mut registry, &["target".to_string()], cx), + 0 + ); + assert!(!registry.contains_key("target"), "dead pane key is pruned"); + }); + drop(other); + } + #[gpui::test] fn fans_out_to_every_alive_weak_and_prunes_dead(cx: &mut gpui::TestAppContext) { let (a, b, mut weaks) = cx.update(|cx| { diff --git a/crates/okena-remote-client/src/manager.rs b/crates/okena-remote-client/src/manager.rs index 6c46186a4..7fd76e90b 100644 --- a/crates/okena-remote-client/src/manager.rs +++ b/crates/okena-remote-client/src/manager.rs @@ -250,28 +250,17 @@ impl RemoteConnectionManager { /// woken by the `activity_rx` doorbell rather than by polling. /// /// Remote output arrives on a tokio task that only buffers bytes via - /// `Terminal::enqueue_output` — it never touches GPUI. The per-pane dirty - /// loop (`TerminalPane::start_remote_dirty_check_loop`) repaints the - /// *focused* terminal grid, but two server-driven indicators are left - /// stale until unrelated local input forces a global repaint (issue #128): + /// `Terminal::enqueue_output`; it cannot touch GPUI directly. Each enqueue + /// rings a capacity-1 doorbell (`try_send`, so bursts coalesce). On every + /// wake this drains and parses pending output for all remote terminals on + /// the GPUI thread, then watches `content_generation` to identify which + /// terminals advanced. /// - /// 1. **Background (unmounted) terminals never get parsed.** A sidebar - /// entry whose pane isn't mounted has no per-pane loop, so its pending - /// bytes are never drained — `has_bell()` stays false and the bell - /// badge never appears. - /// 2. **The sidebar is never notified.** It reads bell/idle straight from - /// the `TerminalsRegistry` (a plain `Arc>`, invisible to - /// GPUI's automatic per-entity dependency tracking), so nothing tells - /// it to re-render when a terminal's derived state changes. - /// - /// Each `enqueue_output` rings the capacity-1 doorbell (`try_send`, so - /// bursts coalesce). On every wake this drains+parses pending output for all - /// remote terminals on the GPUI thread (fixing #1) and watches - /// `content_generation` to confirm something actually advanced — regardless - /// of whether the per-pane loop also drained it. When so it emits - /// `RemoteManagerEvent::TerminalActivity`, which repaints every window's - /// sidebar via the subscription in `WindowView::set_remote_manager` - /// (fixing #2). Idle ⇒ the task simply parks on `recv()`, no CPU. + /// `RemoteManagerEvent::TerminalActivity` carries those terminal ids to + /// `WindowView`, which directly notifies their registered content panes and + /// repaints each window's sidebar. This keeps mounted and background bell / + /// idle state current without a per-pane 8 ms polling task. While idle this + /// task parks on `recv()`, consuming no CPU. fn start_terminal_activity_pump( &self, activity_rx: async_channel::Receiver<()>, @@ -304,9 +293,12 @@ impl RemoteConnectionManager { // fire. let mut advanced: Vec = Vec::new(); for (id, terminal) in &terminals { + // Consume the edge-triggered dirty marker before parsing. + // Any bytes arriving after this point enqueue another + // activity wake, so no per-pane polling is needed. + terminal.take_dirty(); // Parse on the GPUI thread so bell/idle flags are // current even for terminals with no mounted pane. - // No-op when the pending buffer is empty. terminal.process_pending_output(); let generation = terminal.content_generation(); if last_generations.get(id) != Some(&generation) { diff --git a/crates/okena-terminal/src/terminal/io.rs b/crates/okena-terminal/src/terminal/io.rs index 23317eb9c..601a283ff 100644 --- a/crates/okena-terminal/src/terminal/io.rs +++ b/crates/okena-terminal/src/terminal/io.rs @@ -86,10 +86,9 @@ impl Terminal { /// sidebar bell/idle indicators read `has_bell()` / `is_waiting_for_input()` /// *before* the `TerminalContent` child drains. For local terminals the /// equivalent state is set eagerly in `process_output`; remote terminals only - /// buffer via `enqueue_output`, so without an eager parse those indicators - /// render one frame stale and only appear once unrelated local input forces a - /// second repaint. The remote dirty loop calls this so the flags are current - /// when the frame is built. GPUI thread only. + /// buffer via `enqueue_output`. The remote manager's activity pump calls this + /// before emitting targeted pane/sidebar notifications, so derived state is + /// current when the frame is built without per-pane polling. GPUI thread only. pub fn process_pending_output(&self) { self.drain_pending_output(); } diff --git a/crates/okena-views-terminal/src/layout/terminal_pane/mod.rs b/crates/okena-views-terminal/src/layout/terminal_pane/mod.rs index 630ea7ea1..bc8cf9720 100644 --- a/crates/okena-views-terminal/src/layout/terminal_pane/mod.rs +++ b/crates/okena-views-terminal/src/layout/terminal_pane/mod.rs @@ -137,13 +137,6 @@ impl TerminalPane { pane.create_new_terminal(cx); } - if pane - .terminal_id - .as_deref() - .is_some_and(|id| id.starts_with("remote:")) - { - pane.start_remote_dirty_check_loop(cx); - } pane.start_cursor_blink_loop(cx); pane.start_idle_check_loop(cx); @@ -206,34 +199,6 @@ impl TerminalPane { } } - fn start_remote_dirty_check_loop(&self, cx: &mut Context) { - cx.spawn(async move |this: WeakEntity>, cx| { - let interval = Duration::from_millis(8); - loop { - smol::Timer::after(interval).await; - let result = this.update(cx, |pane, cx| { - if let Some(terminal) = pane.terminal.as_ref() - && terminal.take_dirty() - { - // Parse the freshly-arrived bytes up front so derived - // state (bell, waiting) is current before the frame is - // built. Otherwise the lazy parse inside the content - // child's `with_content` runs *after* the pane border - // and sidebar read `has_bell()`, leaving those server- - // driven indicators stale until local input forces a - // second repaint (issue #128). - terminal.process_pending_output(); - pane.content.update(cx, |_, cx| cx.notify()); - } - }); - if result.is_err() { - break; - } - } - }) - .detach(); - } - fn start_cursor_blink_loop(&self, cx: &mut Context) { cx.spawn(async move |this: WeakEntity>, cx| { let interval = Duration::from_millis(500); diff --git a/crates/okena-views-terminal/src/overlays/detached_terminal.rs b/crates/okena-views-terminal/src/overlays/detached_terminal.rs index 593d08690..7b75bb4a4 100644 --- a/crates/okena-views-terminal/src/overlays/detached_terminal.rs +++ b/crates/okena-views-terminal/src/overlays/detached_terminal.rs @@ -75,6 +75,7 @@ impl DetachedTerminalView { request_broker, terminal.clone(), ); + crate::register_content_pane(terminal_id.clone(), content.downgrade()); // Observe workspace for changes (to detect when re-attached) let terminal_id_for_observer = terminal_id.clone(); @@ -90,37 +91,6 @@ impl DetachedTerminalView { }) .detach(); - // Refresh timer - checks terminal dirty flag and notifies only when content changed - let terminal_for_refresh = terminal.clone(); - cx.spawn(async move |this: WeakEntity, cx| { - loop { - smol::Timer::after(std::time::Duration::from_millis(8)).await; // ~120fps check rate - - // Only notify if terminal has new content - if terminal_for_refresh.take_dirty() { - let should_continue = this.update(cx, |this, cx| { - if this.should_close { - return false; - } - cx.notify(); - true - }); - match should_continue { - Ok(true) => continue, - _ => break, - } - } else { - // Check if view still exists - let should_continue = this.update(cx, |this, _| !this.should_close); - match should_continue { - Ok(true) => continue, - _ => break, - } - } - } - }) - .detach(); - Self { workspace, terminal, diff --git a/src/main.rs b/src/main.rs index 76c8e2294..637df3492 100644 --- a/src/main.rs +++ b/src/main.rs @@ -935,14 +935,14 @@ fn main() { }) .detach(); - // Wire up content pane registration so PTY events can notify terminal views + // Wire up content pane registration so remote activity events can notify terminal views okena_views_terminal::set_register_content_pane_fn(Box::new(|terminal_id, weak_content| { let mut registry = okena_app::views::window::content_pane_registry().lock(); let panes = registry.entry(terminal_id).or_default(); // Re-layouts (e.g. workspace switch) re-register the same // terminal, minting fresh panes. Drop dead weaks and skip an // entity already present so the vec stays bounded by live - // viewers and a live pane isn't notified twice per PTY event. + // viewers and a live pane isn't notified twice per activity event. let new_id = weak_content.entity_id(); panes.retain(|w| w.upgrade().is_some()); if !panes.iter().any(|w| w.entity_id() == new_id) { From 79ced68b0e5ca193bfa71d0cd2441fc0a2f03e8e Mon Sep 17 00:00:00 2001 From: Jakub Date: Thu, 23 Jul 2026 17:58:14 +0200 Subject: [PATCH 09/11] fix(terminal): treat Linux zombies as terminated Preserve birth-marker validation while allowing a stopped dtach master to reap killed children after teardown verification. --- crates/okena-terminal/src/session_backend.rs | 84 ++++++++------------ 1 file changed, 34 insertions(+), 50 deletions(-) diff --git a/crates/okena-terminal/src/session_backend.rs b/crates/okena-terminal/src/session_backend.rs index 2c2728fd0..cbaaec06d 100644 --- a/crates/okena-terminal/src/session_backend.rs +++ b/crates/okena-terminal/src/session_backend.rs @@ -534,49 +534,6 @@ fn verify_session_kill( } } -#[cfg(unix)] -fn process_is_live(pid: i32) -> bool { - if unsafe { libc::kill(pid, 0) } == 0 { - return true; - } - std::io::Error::last_os_error().raw_os_error() == Some(libc::EPERM) -} - -#[cfg(unix)] -fn wait_for_pids_to_exit(pids: &[i32]) -> bool { - let deadline = std::time::Instant::now() + std::time::Duration::from_secs(2); - loop { - let live: Vec = pids - .iter() - .copied() - .filter(|pid| process_is_live(*pid)) - .collect(); - if live.is_empty() { - return true; - } - if std::time::Instant::now() >= deadline { - for pid in live { - // The PID was verified as a holder of this Okena-owned socket - // immediately before TERM; this is the bounded escalation path. - unsafe { libc::kill(pid, libc::SIGKILL) }; - } - break; - } - std::thread::sleep(std::time::Duration::from_millis(20)); - } - let deadline = std::time::Instant::now() + std::time::Duration::from_millis(500); - loop { - let any_live = pids.iter().any(|pid| process_is_live(*pid)); - if !any_live { - return true; - } - if std::time::Instant::now() >= deadline { - return false; - } - std::thread::sleep(std::time::Duration::from_millis(20)); - } -} - #[cfg(unix)] #[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] struct TrackedProcess { @@ -599,21 +556,27 @@ fn process_start_marker(pid: i32) -> Option { Some(((seconds as u128) << 64) | micros as u128) } -#[cfg(target_os = "linux")] -fn linux_process_stat(pid: i32) -> Option<(i32, u64)> { - let stat = std::fs::read_to_string(format!("/proc/{pid}/stat")).ok()?; +#[cfg(any(target_os = "linux", test))] +fn parse_linux_process_stat(stat: &str) -> Option<(i32, u8, u64)> { // `comm` is parenthesized and may itself contain spaces or `)`; the final // ") " delimiter is the only safe place to begin fixed-field parsing. let suffix = stat.rsplit_once(") ")?.1; let fields: Vec<&str> = suffix.split_whitespace().collect(); + let state = *fields.first()?.as_bytes().first()?; // field 3 let parent_pid = fields.get(1)?.parse().ok()?; // field 4 let start_ticks = fields.get(19)?.parse().ok()?; // field 22 - Some((parent_pid, start_ticks)) + Some((parent_pid, state, start_ticks)) +} + +#[cfg(target_os = "linux")] +fn linux_process_stat(pid: i32) -> Option<(i32, u8, u64)> { + let stat = std::fs::read_to_string(format!("/proc/{pid}/stat")).ok()?; + parse_linux_process_stat(&stat) } #[cfg(target_os = "linux")] fn process_start_marker(pid: i32) -> Option { - linux_process_stat(pid).map(|(_, start_ticks)| start_ticks as u128) + linux_process_stat(pid).map(|(_, _, start_ticks)| start_ticks as u128) } #[cfg(all(unix, not(target_os = "linux"), not(target_os = "macos")))] @@ -628,7 +591,18 @@ fn tracked_process(pid: i32) -> Option { .then_some(TrackedProcess { pid, start_marker }) } -#[cfg(unix)] +#[cfg(target_os = "linux")] +fn same_process_is_alive(process: TrackedProcess) -> bool { + let Some((_, state, start_ticks)) = linux_process_stat(process.pid) else { + return false; + }; + // A killed child remains in /proc as a zombie until its stopped dtach + // parent resumes and reaps it. It is already dead and must not make a + // verified teardown fail merely because the birth marker still matches. + state != b'Z' && process.start_marker == Some(start_ticks as u128) +} + +#[cfg(all(unix, not(target_os = "linux")))] fn same_process_is_alive(process: TrackedProcess) -> bool { match process.start_marker { Some(marker) => process_start_marker(process.pid) == Some(marker), @@ -663,7 +637,7 @@ fn process_tree_snapshot() -> std::collections::HashMap> { else { continue; }; - if let Some((parent_pid, _)) = linux_process_stat(pid) { + if let Some((parent_pid, _, _)) = linux_process_stat(pid) { tree.entry(parent_pid).or_insert_with(Vec::new).push(pid); } } @@ -1689,6 +1663,16 @@ mod tests { command_output(true) } + #[test] + fn linux_process_stat_parser_reports_zombies_and_birth_markers() { + let mut fields = vec!["Z", "42"]; + fields.extend(std::iter::repeat_n("0", 17)); + fields.push("987"); + let stat = format!("123 (worker ) name) {}", fields.join(" ")); + + assert_eq!(parse_linux_process_stat(&stat), Some((42, b'Z', 987))); + } + #[test] fn verified_session_kill_rejects_command_failure() { assert!(!verify_session_kill( From aaf1054c112dceaaefb495ae0d7ecb686c795755 Mon Sep 17 00:00:00 2001 From: Jakub Date: Thu, 23 Jul 2026 18:01:11 +0200 Subject: [PATCH 10/11] test(core): wait for profile socket shutdown --- crates/okena-core/src/profiles.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/crates/okena-core/src/profiles.rs b/crates/okena-core/src/profiles.rs index 4fb5d6a04..cee823fc0 100644 --- a/crates/okena-core/src/profiles.rs +++ b/crates/okena-core/src/profiles.rs @@ -1246,7 +1246,19 @@ mod tests { assert!(socket_path.exists()); drop(listener); - ensure_profile_runtime_removable(dir.path(), "work").unwrap(); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(1); + loop { + match ensure_profile_runtime_removable(dir.path(), "work") { + Ok(()) => break, + Err(error) + if error.to_string().contains("persistent terminal sessions") + && std::time::Instant::now() < deadline => + { + std::thread::sleep(std::time::Duration::from_millis(10)); + } + Err(error) => panic!("dead socket did not become removable: {error}"), + } + } assert!(!socket_path.exists()); } From 0c89ca4381f189d170a927738b4124bb3a03530d Mon Sep 17 00:00:00 2001 From: Jakub Date: Fri, 24 Jul 2026 13:16:33 +0200 Subject: [PATCH 11/11] fix(ci): move log rotation tests after runtime items --- src/main.rs | 62 ++++++++++++++++++++++++++--------------------------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/src/main.rs b/src/main.rs index 637df3492..8dabf2871 100644 --- a/src/main.rs +++ b/src/main.rs @@ -104,37 +104,6 @@ fn rotate_log_file(active: &std::path::Path, previous: &std::path::Path) -> std: std::fs::rename(active, previous) } -#[cfg(test)] -mod log_rotation_tests { - use super::rotate_log_file; - - #[test] - fn rotation_replaces_existing_previous_file_without_truncating_active() { - let directory = std::env::temp_dir().join(format!( - "okena-log-rotation-{}-{}", - std::process::id(), - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .expect("system clock is after epoch") - .as_nanos() - )); - std::fs::create_dir(&directory).expect("create log directory"); - let active = directory.join("okena-headless.log"); - let previous = directory.join("okena-headless.log.1"); - std::fs::write(&active, "active log").expect("write active log"); - std::fs::write(&previous, "old rotation").expect("write old rotation"); - - rotate_log_file(&active, &previous).expect("rotate log"); - - assert!(!active.exists()); - assert_eq!( - std::fs::read_to_string(&previous).expect("read rotation"), - "active log" - ); - std::fs::remove_dir_all(directory).expect("remove log directory"); - } -} - use crate::assets::{Assets, embedded_fonts}; use okena_app::app::Okena; use okena_app::keybindings; @@ -985,3 +954,34 @@ fn main() { }); } + +#[cfg(test)] +mod log_rotation_tests { + use super::rotate_log_file; + + #[test] + fn rotation_replaces_existing_previous_file_without_truncating_active() { + let directory = std::env::temp_dir().join(format!( + "okena-log-rotation-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("system clock is after epoch") + .as_nanos() + )); + std::fs::create_dir(&directory).expect("create log directory"); + let active = directory.join("okena-headless.log"); + let previous = directory.join("okena-headless.log.1"); + std::fs::write(&active, "active log").expect("write active log"); + std::fs::write(&previous, "old rotation").expect("write old rotation"); + + rotate_log_file(&active, &previous).expect("rotate log"); + + assert!(!active.exists()); + assert_eq!( + std::fs::read_to_string(&previous).expect("read rotation"), + "active log" + ); + std::fs::remove_dir_all(directory).expect("remove log directory"); + } +}