diff --git a/crates/agentos-sidecar-core/src/engine.rs b/crates/agentos-sidecar-core/src/engine.rs index 3d1235d40..8c7ab2558 100644 --- a/crates/agentos-sidecar-core/src/engine.rs +++ b/crates/agentos-sidecar-core/src/engine.rs @@ -227,13 +227,25 @@ impl AcpCore { }; let _ = host.close_stdin(&session.process_id); - let _ = host.kill_agent(&session.process_id, "SIGTERM"); - if host - .wait_for_exit(&session.process_id, SESSION_CLOSE_TIMEOUT_MS)? - .is_none() + // Mirror of the native `close_session` short-circuit: an adapter that + // already exited (crash / OOM / idle eviction, recorded on the session + // as `closed`) has no future exit to wait for, and signalling its + // reaped PID fails with a process-gone error. Skip the SIGTERM → wait + // → SIGKILL → wait dance (~2× `SESSION_CLOSE_TIMEOUT_MS` of dead + // waiting) in that case. + let adapter_already_gone = session.closed || { + let sigterm = host.kill_agent(&session.process_id, "SIGTERM"); + matches!(&sigterm, Err(error) if is_process_already_gone_error(error)) + }; + if !adapter_already_gone + && host + .wait_for_exit(&session.process_id, SESSION_CLOSE_TIMEOUT_MS)? + .is_none() { - let _ = host.kill_agent(&session.process_id, "SIGKILL"); - let _ = host.wait_for_exit(&session.process_id, SESSION_CLOSE_TIMEOUT_MS)?; + let sigkill = host.kill_agent(&session.process_id, "SIGKILL"); + if !matches!(&sigkill, Err(error) if is_process_already_gone_error(error)) { + let _ = host.wait_for_exit(&session.process_id, SESSION_CLOSE_TIMEOUT_MS)?; + } } Ok(AcpResponse::AcpSessionClosedResponse( @@ -1121,6 +1133,18 @@ fn is_unknown_session_error(response: &Value) -> bool { .is_some_and(|kind| kind == "unknown_session") } +/// True when a signal/kill request failed because the target process no longer +/// exists — the process-table `ESRCH` / "no such process" / "has no active +/// process" errors surfaced for an already-reaped PID. `close_session` uses +/// this to skip the exit wait when the adapter is already gone. Mirrors the +/// native sidecar's `is_process_already_gone_error`. +fn is_process_already_gone_error(error: &AcpCoreError) -> bool { + let message = error.to_string().to_ascii_lowercase(); + message.contains("esrch") + || message.contains("no such process") + || message.contains("has no active process") +} + /// Write a JSON-RPC message as a single newline-terminated line to the agent's /// stdin (no waiting). Used by the resumable handshake. fn write_json_line( @@ -1329,6 +1353,104 @@ mod tests { assert_eq!(host.killed, vec![("proc-s1".into(), "SIGTERM".into())]); } + /// Host whose adapter process is already gone: `wait_for_exit` would time + /// out (returns `None`), so any wait the engine performs is dead time. The + /// wait counter proves the short-circuit: a regression re-enters the + /// SIGTERM → wait → SIGKILL → wait sequence and records 2 waits. + #[derive(Default)] + struct GoneAdapterHost { + kill_error: Option, + killed: Vec<(String, String)>, + waits: usize, + } + + impl AcpHost for GoneAdapterHost { + fn spawn_agent(&mut self, _: SpawnAgentRequest) -> Result { + unreachable!("close_session does not spawn") + } + fn bind_session(&mut self, _: &str, _: &str) -> Result<(), AcpCoreError> { + Ok(()) + } + fn write_stdin(&mut self, _: &str, _: &[u8]) -> Result<(), AcpCoreError> { + unreachable!() + } + fn close_stdin(&mut self, _: &str) -> Result<(), AcpCoreError> { + Ok(()) + } + fn poll_output(&mut self, _: &str) -> Result, AcpCoreError> { + Ok(None) + } + fn kill_agent(&mut self, process_id: &str, signal: &str) -> Result<(), AcpCoreError> { + self.killed + .push((process_id.to_string(), signal.to_string())); + match &self.kill_error { + Some(message) => Err(AcpCoreError::InvalidState(message.clone())), + None => Ok(()), + } + } + fn wait_for_exit(&mut self, _: &str, _: u64) -> Result, AcpCoreError> { + self.waits += 1; + Ok(None) // the exit event was already drained; a wait can only time out + } + fn write_file(&mut self, _: &str, _: &[u8]) -> Result<(), AcpCoreError> { + Ok(()) + } + fn read_file(&mut self, _: &str) -> Result, AcpCoreError> { + Ok(Vec::new()) + } + fn now_ms(&self) -> u64 { + 0 + } + } + + #[test] + fn close_session_skips_teardown_wait_when_session_already_closed() { + let mut core = AcpCore::new(); + let mut dead = record("s1", "conn-a"); + dead.closed = true; + dead.exit_code = Some(137); + core.insert_session(dead); + let mut host = GoneAdapterHost::default(); + let resp = core + .close_session( + &mut host, + "conn-a", + &AcpCloseSessionRequest { + session_id: "s1".into(), + }, + ) + .expect("close"); + assert!(matches!(resp, AcpResponse::AcpSessionClosedResponse(_))); + assert_eq!(core.session_count(), 0); + // Already-observed exit: no signals sent, no dead waiting. + assert!(host.killed.is_empty()); + assert_eq!(host.waits, 0); + } + + #[test] + fn close_session_skips_teardown_wait_when_sigterm_reports_process_gone() { + let mut core = AcpCore::new(); + core.insert_session(record("s1", "conn-a")); + let mut host = GoneAdapterHost { + kill_error: Some(String::from("process proc-s1: no such process (ESRCH)")), + ..GoneAdapterHost::default() + }; + let resp = core + .close_session( + &mut host, + "conn-a", + &AcpCloseSessionRequest { + session_id: "s1".into(), + }, + ) + .expect("close"); + assert!(matches!(resp, AcpResponse::AcpSessionClosedResponse(_))); + // SIGTERM was attempted, classified as process-gone, and both waits + // (and the SIGKILL escalation) were skipped. + assert_eq!(host.killed, vec![("proc-s1".into(), "SIGTERM".into())]); + assert_eq!(host.waits, 0); + } + #[test] fn session_request_enforces_ownership_without_side_effects() { // A non-owner prompt fails closed with the same unknown-session error and diff --git a/crates/agentos-sidecar/src/acp_extension.rs b/crates/agentos-sidecar/src/acp_extension.rs index 9cedadb7e..4f2ef4149 100644 --- a/crates/agentos-sidecar/src/acp_extension.rs +++ b/crates/agentos-sidecar/src/acp_extension.rs @@ -626,37 +626,38 @@ impl AcpExtension { process_id: session.process_id.clone(), }) .await; - let sigterm = ctx - .kill_process_wire(KillProcessRequest { - process_id: session.process_id.clone(), - signal: String::from("SIGTERM"), - }) - .await; - // The adapter process may already be gone: it can exit out-of-band (crash / - // OOM, or a lazily-observed idle-time crash) *before* the client sends - // close_session. When it does, its `ProcessExited` event has already been - // emitted and drained, so `wait_for_process_exit` — which only polls the - // event stream and never checks current process state — can never observe - // it and blocks the full `SESSION_CLOSE_TIMEOUT` for SIGTERM *and* again - // after SIGKILL (~2× the timeout) before we ever reach resource disposal. - // Because close_session is serial per sidecar, that stall also blocks a - // `create_session` the host issues right after it (session recovery / a - // returning user whose idle session was evicted eats the full stall on - // their next turn). Detect the already-gone case from the SIGTERM result — - // reusing the same "adapter is gone" classification the prompt path uses — - // and skip straight to resource disposal. - let already_gone = matches!(&sigterm, Err(error) if is_process_already_gone(error)); - if !already_gone + // The adapter may already be gone: it can crash, OOM, or idle-evict + // before the client sends close_session, and its `ProcessExitedEvent` + // has then already been drained from the shared per-ownership event + // queue (usually by the prompt exchange loop, which records it as + // `session.closed`). `wait_for_process_exit` only observes *future* + // events, so without a short-circuit an already-dead adapter burns + // `SESSION_CLOSE_TIMEOUT` twice (~10s) signalling a PID that no longer + // exists — and because extension dispatch is serialized, a + // `create_session` issued right after (session recovery for a + // returning user) stalls behind the dead wait. + let adapter_already_gone = session.closed || { + let sigterm = ctx + .kill_process_wire(KillProcessRequest { + process_id: session.process_id.clone(), + signal: String::from("SIGTERM"), + }) + .await; + matches!(&sigterm, Err(error) if is_process_already_gone_error(error)) + }; + if !adapter_already_gone && !wait_for_process_exit(&mut ctx, &session.process_id, SESSION_CLOSE_TIMEOUT).await { - let _ = ctx + let sigkill = ctx .kill_process_wire(KillProcessRequest { process_id: session.process_id.clone(), signal: String::from("SIGKILL"), }) .await; - let _ = - wait_for_process_exit(&mut ctx, &session.process_id, SESSION_CLOSE_TIMEOUT).await; + if !matches!(&sigkill, Err(error) if is_process_already_gone_error(error)) { + let _ = wait_for_process_exit(&mut ctx, &session.process_id, SESSION_CLOSE_TIMEOUT) + .await; + } } let _ = ctx .dispose_session_resources_wire(&request.session_id) @@ -2571,14 +2572,13 @@ fn is_adapter_gone_error(error: &SidecarError) -> bool { matches!(error, SidecarError::InvalidState(message) if message.contains(ADAPTER_NO_ACTIVE_PROCESS_MARKER)) } -/// True when a kill/signal request failed because the target process no longer -/// exists — i.e. it already exited before we tried to terminate it. Covers both -/// the adapter-gone classification (`is_adapter_gone_error`) and the lower-level -/// process-table `ESRCH` / "no such process" the signal path returns for a PID -/// that has already been reaped. Used by `close_session` to skip the -/// `wait_for_process_exit` dance (which can only observe a *future* exit event) -/// when the process is already gone. -fn is_process_already_gone(error: &SidecarError) -> bool { +/// True when a signal/kill request failed because the target process no longer +/// exists: either the adapter-gone classification the prompt path uses +/// (`is_adapter_gone_error`) or the lower-level process-table `ESRCH` / +/// "no such process" error the signal path returns for an already-reaped PID. +/// `close_session` uses this to skip `wait_for_process_exit` — which can only +/// observe a *future* exit event — when the process is already gone. +fn is_process_already_gone_error(error: &SidecarError) -> bool { if is_adapter_gone_error(error) { return true; }