Skip to content

Commit 61d0372

Browse files
matt2eclaude
andauthored
fix(acp): send SIGINT to blox acp proxy for graceful shutdown (#454)
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 937d1f7 commit 61d0372

File tree

8 files changed

+92
-28
lines changed

8 files changed

+92
-28
lines changed

Cargo.lock

Lines changed: 18 additions & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

apps/staged/src-tauri/Cargo.lock

Lines changed: 14 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

apps/staged/src-tauri/src/session_runner.rs

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -397,9 +397,7 @@ pub fn start_session(
397397
.map(|q| !q.is_empty())
398398
.unwrap_or(false);
399399

400-
if has_queued {
401-
log::info!("Skipping auto review for branch {branch_id}: queued sessions pending");
402-
} else {
400+
if !has_queued {
403401
let store_for_auto = Arc::clone(&store_for_status);
404402
let registry_for_auto = Arc::clone(&registry);
405403
let app_handle_for_auto = app_handle.clone();

crates/acp-client/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,3 +14,4 @@ anyhow = "1"
1414
async-trait = "0.1"
1515
log = "0.4"
1616
blox-cli = { path = "../blox-cli" }
17+
nix = { version = "0.29", features = ["signal"] }

crates/acp-client/src/driver.rs

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,9 @@ use tokio::sync::Mutex;
2727
use tokio_util::compat::{TokioAsyncReadCompatExt, TokioAsyncWriteCompatExt};
2828
use tokio_util::sync::CancellationToken;
2929

30+
use nix::sys::signal::{self, Signal};
31+
use nix::unistd::Pid;
32+
3033
use crate::types::blox_acp_command;
3134

3235
// =============================================================================
@@ -283,6 +286,13 @@ impl AgentDriver for AcpDriver {
283286
// the actions executor does) and logging it to aid debugging.
284287
.stderr(Stdio::null())
285288
.kill_on_drop(true);
289+
// Put remote proxies in their own process group so we can send SIGINT
290+
// to the entire group (sq + its child processes) for graceful shutdown.
291+
// Without this, only `sq` receives SIGINT and hangs waiting for its
292+
// child (the blox acp proxy) which never got the signal.
293+
if self.is_remote {
294+
cmd.process_group(0);
295+
}
286296
// For local shells extra_env is set on the clean environment; for
287297
// remote spawns it augments the inherited environment.
288298
for (k, v) in &self.extra_env {
@@ -397,6 +407,7 @@ impl AgentDriver for AcpDriver {
397407
_ = cancel_token.cancelled() => {
398408
log::info!("Session {session_id} cancelled");
399409
writer.finalize().await;
410+
graceful_stop(&mut child, self.is_remote).await;
400411
return Ok(());
401412
}
402413
result = run_acp_protocol(
@@ -406,12 +417,45 @@ impl AgentDriver for AcpDriver {
406417
};
407418

408419
writer.finalize().await;
409-
let _ = child.kill().await;
420+
graceful_stop(&mut child, self.is_remote).await;
410421

411422
protocol_result
412423
}
413424
}
414425

426+
/// Gracefully stop the ACP child process.
427+
///
428+
/// For remote blox proxies, sends SIGINT so the Go process can run its
429+
/// deferred cleanup (calling `StopProcess` on the bloxlet, which marks the
430+
/// remote command as COMPLETE). Falls back to SIGKILL after a timeout.
431+
///
432+
/// For local processes, sends SIGKILL immediately.
433+
async fn graceful_stop(child: &mut tokio::process::Child, is_remote: bool) {
434+
if is_remote {
435+
let Some(pid) = child.id() else {
436+
// Process already exited on its own — no cleanup needed.
437+
return;
438+
};
439+
let Ok(pid) = i32::try_from(pid) else {
440+
let _ = child.kill().await;
441+
return;
442+
};
443+
// Send SIGINT to the process group (negative PID) so both `sq`
444+
// and its child processes (the blox acp proxy) receive the signal.
445+
// The process was spawned with process_group(0) so its PGID == PID.
446+
if signal::kill(Pid::from_raw(-pid), Signal::SIGINT).is_ok() {
447+
if let Ok(Ok(_status)) =
448+
tokio::time::timeout(Duration::from_secs(5), child.wait()).await
449+
{
450+
return;
451+
}
452+
}
453+
let _ = child.kill().await;
454+
return;
455+
}
456+
let _ = child.kill().await;
457+
}
458+
415459
#[derive(Debug, PartialEq, Eq)]
416460
enum RemoteLineOutcome {
417461
Emit(String),

crates/acp-client/src/lib.rs

Lines changed: 2 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -16,21 +16,6 @@
1616
//! full session orchestration, streaming, and database integration
1717
//! 2. **Simple one-shot interface** (`run_acp_prompt`) - For simple prompting
1818
//! without session management
19-
//!
20-
//! # Example (Simple)
21-
//!
22-
//! ```rust,no_run
23-
//! use acp_client::{find_acp_agent, run_acp_prompt};
24-
//! use std::path::Path;
25-
//!
26-
//! #[tokio::main(flavor = "current_thread")]
27-
//! async fn main() -> anyhow::Result<()> {
28-
//! let agent = find_acp_agent().ok_or_else(|| anyhow::anyhow!("No ACP agent found"))?;
29-
//! let response = run_acp_prompt(&agent, Path::new("."), "Hello!").await?;
30-
//! println!("Agent response: {}", response);
31-
//! Ok(())
32-
//! }
33-
//! ```
3419
3520
mod driver;
3621
mod simple;
@@ -43,6 +28,6 @@ pub use driver::{
4328
};
4429
pub use simple::run_acp_prompt;
4530
pub use types::{
46-
discover_providers, find_acp_agent, find_acp_agent_by_id, find_command, AcpAgent,
47-
AcpProviderInfo,
31+
discover_providers, find_acp_agent, find_acp_agent_by_id, find_command, known_agent_commands,
32+
AcpAgent, AcpProviderInfo,
4833
};

crates/acp-client/src/types.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,11 @@ pub fn find_acp_agent_by_id(provider_id: &str) -> Option<AcpAgent> {
111111
})
112112
}
113113

114+
/// Return the CLI command names of all known agents (for error messages).
115+
pub fn known_agent_commands() -> Vec<&'static str> {
116+
KNOWN_AGENTS.iter().map(|a| a.command).collect()
117+
}
118+
114119
/// Find the first available ACP agent.
115120
///
116121
/// Tries each known agent in order and returns the first one found.

crates/builderbot-actions/src/acp_provider.rs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,12 @@ impl AcpAiProvider {
2626
/// An ACP provider, or an error if no ACP agent is available
2727
pub fn new(working_dir: PathBuf) -> Result<Self> {
2828
// Verify an agent is available
29-
acp_client::find_acp_agent()
30-
.ok_or_else(|| anyhow::anyhow!("No ACP agent found (tried goose, claude-agent-acp)"))?;
29+
acp_client::find_acp_agent().ok_or_else(|| {
30+
anyhow::anyhow!(
31+
"No ACP agent found (tried {})",
32+
acp_client::known_agent_commands().join(", ")
33+
)
34+
})?;
3135
Ok(Self { working_dir })
3236
}
3337

0 commit comments

Comments
 (0)