Contain source-upgrade process trees

crates/homeboy-cli/src/commands/agent_task/review.rs

@@ -1838,6 +1838,7 @@ mod tests {
             candidate_ref: "deadbeef".to_string(),
             ai_model: Some("openai/gpt-5.6-terra".to_string()),
             replace_interrupted: false,
+            accept_inherited_failures: false,
             full: false,
         };
 

crates/homeboy-upgrade/src/upgrade/execution.rs

@@ -3,6 +3,9 @@ use homeboy_core::engine::shell::quote_path;
 use homeboy_core::error::{Error, Result};
 use homeboy_core::git::{run_git, run_git_output};
 use homeboy_core::stream_capture::StreamCaptureMetadata;
+use homeboy_engine_primitives::command::{
+    terminate_process_tree_and_reap, terminate_remaining_process_group, ControllerChildGuard,
+};
 use std::env;
 use std::path::{Path, PathBuf};
 use std::process::{Command, Stdio};
@@ -21,6 +24,7 @@ use super::types::InstallMethod;
 /// `agent_task_promotion` / runner exec captures (#5297).
 const UPGRADE_CAPTURE_LIMIT_BYTES: usize = 65_536;
 const SOURCE_UPGRADE_TIMEOUT: Duration = Duration::from_secs(20 * 60);
+const CLEANUP_ERROR_CONTEXT_LIMIT_CHARS: usize = 1_024;
 
 /// Environment variable set in the shell child's environment and checked at the
 /// start of `execute_upgrade` to prevent nested / re-entrant source upgrades.
@@ -375,55 +379,87 @@ fn run_source_upgrade_command(
         .current_dir(workspace_root)
         .env(REENTRANCY_GUARD_ENV, "1")
         .stdin(Stdio::null());
-    #[cfg(unix)]
-    {
-        use std::os::unix::process::CommandExt;
-        child_command.process_group(0);
-    }
+    let guard = ControllerChildGuard::prepare(&mut child_command).map_err(|error| {
+        Error::internal_io(error.to_string(), Some("run source upgrade".to_string()))
+    })?;
     let mut child = child_command
         .spawn()
         .map_err(|e| Error::internal_io(e.to_string(), Some("run source upgrade".to_string())))?;
+    if let Err(error) = guard.attach(&child) {
+        let primary = Error::internal_io(
+            format!("failed to attach source-upgrade process guard: {error}"),
+            Some("run source upgrade".to_string()),
+        );
+        return Err(append_cleanup_failure_context(
+            primary,
+            terminate_process_tree_and_reap(&mut child).err(),
+        ));
+    }
     let start = Instant::now();
     loop {
         match child.try_wait() {
-            Ok(Some(status)) if status.success() => return Ok(()),
             Ok(Some(status)) => {
-                return Err(upgrade_failure_error(
-                    InstallMethod::Source,
-                    &format!("source upgrade command exited with {}", status),
-                    None,
+                let cleanup = terminate_remaining_process_group(child.id());
+                if status.success() {
+                    return cleanup.map_err(|error| {
+                        Error::internal_io(
+                            error.to_string(),
+                            Some("run source upgrade".to_string()),
+                        )
+                    });
+                }
+                return Err(append_cleanup_failure_context(
+                    upgrade_failure_error(
+                        InstallMethod::Source,
+                        &format!("source upgrade command exited with {}", status),
+                        None,
+                    ),
+                    cleanup.err(),
                 ));
             }
             Ok(None) if start.elapsed() >= timeout => {
-                terminate_upgrade_child(&mut child);
-                return Err(Error::internal_io(
-                    format!(
-                        "source upgrade timed out after {}s; child process group was terminated",
-                        timeout.as_secs()
-                    ),
+                let primary = Error::internal_io(
+                    format!("source upgrade timed out after {}s", timeout.as_secs()),
                     Some("run source upgrade".to_string()),
+                );
+                return Err(append_cleanup_failure_context(
+                    primary,
+                    terminate_process_tree_and_reap(&mut child).err(),
                 ));
             }
             Ok(None) => std::thread::sleep(Duration::from_millis(25)),
             Err(e) => {
-                terminate_upgrade_child(&mut child);
-                return Err(Error::internal_io(
-                    e.to_string(),
-                    Some("wait for source upgrade".to_string()),
+                let primary =
+                    Error::internal_io(e.to_string(), Some("wait for source upgrade".to_string()));
+                return Err(append_cleanup_failure_context(
+                    primary,
+                    terminate_process_tree_and_reap(&mut child).err(),
                 ));
             }
         }
     }
 }
 
-fn terminate_upgrade_child(child: &mut std::process::Child) {
-    #[cfg(unix)]
-    unsafe {
-        libc::kill(-(child.id() as i32), libc::SIGKILL);
+/// Keep the command failure actionable while retaining bounded cleanup evidence.
+fn append_cleanup_failure_context(
+    mut primary: Error,
+    cleanup_error: Option<std::io::Error>,
+) -> Error {
+    let Some(cleanup_error) = cleanup_error else {
+        return primary;
+    };
+    let cleanup_message = cleanup_error.to_string();
+    let mut bounded = cleanup_message
+        .chars()
+        .take(CLEANUP_ERROR_CONTEXT_LIMIT_CHARS)
+        .collect::<String>();
+    if cleanup_message.chars().count() > CLEANUP_ERROR_CONTEXT_LIMIT_CHARS {
+        bounded.push_str("... [truncated]");
     }
-    #[cfg(not(unix))]
-    let _ = child.kill();
-    let _ = child.wait();
+    primary.message.push_str(&format!(
+        "; source-upgrade process cleanup also failed: {bounded}"
+    ));
+    primary
 }
 
 /// Detect a source upgrade whose command exited successfully but left the

crates/homeboy-upgrade/src/upgrade/execution/tests/part_a.rs

@@ -95,6 +95,80 @@ fn source_upgrade_command_returns_after_same_binary_success() {
     .expect("source command completes");
 }
 
+#[test]
+fn cleanup_context_preserves_the_primary_upgrade_error_contract() {
+    let primary = upgrade_failure_error(
+        InstallMethod::Binary,
+        "curl: (22) The requested URL returned error: 404",
+        None,
+    );
+    let expected = primary.clone();
+    let error = append_cleanup_failure_context(
+        primary,
+        Some(std::io::Error::other("cleanup process group failed")),
+    );
+
+    assert_eq!(error.code, expected.code);
+    assert_eq!(error.details, expected.details);
+    assert_eq!(
+        error
+            .hints
+            .iter()
+            .map(|hint| hint.message.as_str())
+            .collect::<Vec<_>>(),
+        expected
+            .hints
+            .iter()
+            .map(|hint| hint.message.as_str())
+            .collect::<Vec<_>>()
+    );
+    assert!(error.message.starts_with(&expected.message));
+    assert!(error.message.contains("cleanup process group failed"));
+}
+
+#[test]
+fn cleanup_context_is_bounded() {
+    let error = append_cleanup_failure_context(
+        Error::internal_io("primary failure", Some("source upgrade".to_string())),
+        Some(std::io::Error::other("x".repeat(2_000))),
+    );
+
+    assert!(error.message.len() < 1_200);
+    assert!(error.message.ends_with("... [truncated]"));
+}
+
+#[cfg(unix)]
+#[test]
+fn source_upgrade_completion_reaps_background_process_group() {
+    let workspace = tempfile::tempdir().expect("workspace");
+    let pid_file = workspace.path().join("child.pid");
+    let command = format!(
+        "sleep 30 & echo $! > {}; printf built",
+        quote_path(&pid_file.display().to_string())
+    );
+
+    run_source_upgrade_command(&command, workspace.path(), Duration::from_secs(1))
+        .expect("source command completes");
+
+    let child_pid = std::fs::read_to_string(&pid_file)
+        .expect("background child pid")
+        .trim()
+        .parse::<libc::pid_t>()
+        .expect("numeric pid");
+    let state = Command::new("ps")
+        .args(["-o", "stat=", "-p", &child_pid.to_string()])
+        .output()
+        .expect("inspect background child state");
+    assert!(
+        state.stdout.is_empty()
+            || String::from_utf8_lossy(&state.stdout)
+                .trim_start()
+                .starts_with('Z'),
+        "background child {child_pid} remained runnable: {}",
+        String::from_utf8_lossy(&state.stdout)
+    );
+}
+
 #[cfg(unix)]
 #[test]
 fn source_upgrade_timeout_terminates_the_entire_child_process_group() {