From 7454b23ae6542f5a46dc2b161aab9527a3f5e3cf Mon Sep 17 00:00:00 2001
From: Hunter Bown <hmbown@gmail.com>
Date: Mon, 11 May 2026 16:34:00 -0500
Subject: [PATCH] fix(shell): kill process group in collect_output to prevent
 UI freeze
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Cherry-picked from PR #1475 by CrepuscularIRIS (autoghclaw/issue-828).

When a shell command spawns background subprocesses (nohup, sleep &, etc.),
those subprocesses inherit the pipe write-ends. After the shell exits, the
subprocesses keep those write-ends open, causing handle.join() on reader
threads to block indefinitely in read(). Since list_jobs() calls poll() →
collect_output() on every TUI render tick, the entire UI event loop blocks.

Fix: kill the process group (PGID = child PID) before joining reader threads,
so orphaned subprocesses release their pipe write-ends. Also wires the
previously dead-coded cleanup() into list_jobs() with a 1-hour eviction
window to bound process table growth.

Fixes #828.
---
 crates/tui/src/tools/shell.rs       | 12 +++++++-
 crates/tui/src/tools/shell/tests.rs | 46 +++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/crates/tui/src/tools/shell.rs b/crates/tui/src/tools/shell.rs
index 0c93fd86..d567ed03 100644
--- a/crates/tui/src/tools/shell.rs
+++ b/crates/tui/src/tools/shell.rs
@@ -329,6 +329,15 @@ impl BackgroundShell {
 
     /// Collect output from the background threads
     fn collect_output(&mut self) {
+        // Kill the whole process group before joining reader threads.
+        // When the shell spawned persistent background jobs (e.g. `nohup curl`),
+        // those subprocesses keep the pipe write-ends open after the shell exits.
+        // Without this kill, handle.join() blocks indefinitely, freezing the UI
+        // event loop that calls list_jobs() → poll() → collect_output().
+        #[cfg(unix)]
+        if let Some(ShellChild::Process(ref mut proc)) = self.child {
+            let _ = kill_child_process_group(proc);
+        }
         if let Some(handle) = self.stdout_thread.take() {
             let _ = handle.join();
         }
@@ -1299,6 +1308,8 @@ impl ShellManager {
         for shell in self.processes.values_mut() {
             shell.poll();
         }
+        // Evict completed processes older than 1 hour to bound memory growth.
+        self.cleanup(Duration::from_secs(3600));
 
         let mut jobs = self
             .processes
@@ -1346,7 +1357,6 @@ impl ShellManager {
     }
 
     /// Clean up completed processes older than the given duration
-    #[allow(dead_code)]
     pub fn cleanup(&mut self, max_age: Duration) {
         let _now = Instant::now();
         self.processes.retain(|_, shell| {
diff --git a/crates/tui/src/tools/shell/tests.rs b/crates/tui/src/tools/shell/tests.rs
index a01018ba..4e71c29c 100644
--- a/crates/tui/src/tools/shell/tests.rs
+++ b/crates/tui/src/tools/shell/tests.rs
@@ -743,3 +743,49 @@ fn test_macos_provenance_not_triggered_on_unrelated_eperm() {
     let result = make_failed_result("open /some/other/path: operation not permitted");
     assert!(!looks_like_macos_provenance_failure(&result));
 }
+
+// Regression test for #828: shell spawns an orphaned background subprocess
+// (simulating `nohup curl`) that keeps the pipe write-end open after the shell
+// exits. collect_output() must not block indefinitely — it kills the whole
+// process group first, allowing reader threads to get EOF and exit.
+#[cfg(unix)]
+#[test]
+fn test_orphaned_subprocess_does_not_block_collect_output() {
+    let tmp = tempdir().expect("tempdir");
+    let mut manager = ShellManager::new(tmp.path().to_path_buf());
+
+    // sh spawns `sleep 100 &` and exits; the sleep subprocess inherits the
+    // pipe write-ends and would keep reader threads blocked without the fix.
+    let result = manager
+        .execute("sh -c 'sleep 100 &'", None, 5000, true)
+        .expect("execute");
+    let task_id = result.task_id.expect("task id");
+
+    // Drive to completion with a tight timeout — must not hang.
+    let done = manager
+        .get_output(&task_id, true, 3000)
+        .expect("get_output must complete, not hang");
+    assert_eq!(done.status, ShellStatus::Completed);
+}
+
+#[test]
+fn test_list_jobs_cleans_up_completed_old_processes() {
+    let tmp = tempdir().expect("tempdir");
+    let mut manager = ShellManager::new(tmp.path().to_path_buf());
+
+    let bg = manager
+        .execute(&echo_command("bg"), None, 5000, true)
+        .expect("execute bg");
+    let bg_id = bg.task_id.expect("bg task id");
+    manager.get_output(&bg_id, true, 3000).expect("bg done");
+
+    // Both the completed job and any tracking state should be present.
+    assert!(!manager.processes.is_empty());
+
+    // cleanup(ZERO) removes all completed processes immediately.
+    manager.cleanup(Duration::ZERO);
+    assert!(
+        manager.processes.is_empty(),
+        "completed processes should be evicted by cleanup"
+    );
+}