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; } diff --git a/crates/native-sidecar/src/plugins/js_bridge.rs b/crates/native-sidecar/src/plugins/js_bridge.rs index 06c98f6bb..1391b74b3 100644 --- a/crates/native-sidecar/src/plugins/js_bridge.rs +++ b/crates/native-sidecar/src/plugins/js_bridge.rs @@ -517,6 +517,18 @@ impl VirtualFileSystem for JsBridgeFilesystem { } fn realpath(&self, path: &str) -> VfsResult { + // The mount root always exists — it IS the mount point — so resolve it + // locally like `host_dir` does instead of round-tripping the bridge. + // Host-side drivers frequently cannot canonicalize their own root and + // return ENOENT for it, and because a js_bridge mount root is not a + // symlink leaf, `MountTable::realpath` propagates that ENOENT instead + // of falling back to the lexical path. The kernel permission wrapper + // resolves every operation's subject through `realpath` first, so that + // ENOENT aborts e.g. a readdir of the mount root before the readDir + // bridge call is ever issued. + if path.is_empty() || path == "/" { + return Ok(String::from("/")); + } self.parse_required( "realpath", path, diff --git a/crates/native-sidecar/tests/service.rs b/crates/native-sidecar/tests/service.rs index 6ea4daf03..cf0c6fbfb 100644 --- a/crates/native-sidecar/tests/service.rs +++ b/crates/native-sidecar/tests/service.rs @@ -992,6 +992,19 @@ ykAheWCsAteSEWVc0w==\n\ ) -> ( Arc>, Arc>>, + ) { + install_memory_js_bridge_handler_with_options(sidecar, false) + } + + /// `fail_realpath` simulates host-side drivers that cannot canonicalize + /// their own paths and answer every `realpath` bridge call with ENOENT + /// — the shape that used to break readdir of a js_bridge mount root. + fn install_memory_js_bridge_handler_with_options( + sidecar: &mut NativeSidecar, + fail_realpath: bool, + ) -> ( + Arc>, + Arc>>, ) { let filesystem = Arc::new(Mutex::new(MemoryFileSystem::new())); let calls = Arc::new(Mutex::new(Vec::::new())); @@ -1111,11 +1124,15 @@ ykAheWCsAteSEWVc0w==\n\ .map_err(|error| format!("{}: {error}", error.code())) } "realpath" => { - let path = call_args["path"].as_str().expect("realpath path"); - filesystem - .realpath(path) - .map(|resolved| Some(json!(resolved))) - .map_err(|error| format!("{}: {error}", error.code())) + if fail_realpath { + Err(String::from("ENOENT: no such file or directory")) + } else { + let path = call_args["path"].as_str().expect("realpath path"); + filesystem + .realpath(path) + .map(|resolved| Some(json!(resolved))) + .map_err(|error| format!("{}: {error}", error.code())) + } } "symlink" => { let target = call_args["target"].as_str().expect("symlink target"); @@ -7458,6 +7475,92 @@ ykAheWCsAteSEWVc0w==\n\ .expect_err("stat should fail"); assert_eq!(stat_error.code(), "EIO"); } + + fn configure_vm_js_bridge_mount_readdir_of_mount_root_survives_broken_driver_realpath() { + // Regression: readdir of a js_bridge mount root used to fail with + // ENOENT before any readDir bridge call was issued. The kernel + // permission wrapper resolves every subject through `realpath` + // first, and host-side drivers that cannot canonicalize their own + // root answer the mount-root realpath with ENOENT — which + // `MountTable::realpath` propagates for non-symlink-leaf mounts. + // The mount root must resolve locally, without a bridge realpath. + let mut sidecar = create_test_sidecar(); + let (filesystem, calls) = + install_memory_js_bridge_handler_with_options(&mut sidecar, true); + filesystem + .lock() + .expect("lock js bridge fs") + .write_file("/hello.txt", b"hi".to_vec()) + .expect("seed js bridge fs"); + + let (connection_id, session_id) = + authenticate_and_open_session(&mut sidecar).expect("authenticate and open session"); + let vm_id = create_vm( + &mut sidecar, + &connection_id, + &session_id, + PermissionsPolicy::allow_all(), + ) + .expect("create vm"); + + sidecar + .dispatch_blocking(request( + 4, + OwnershipScope::vm(&connection_id, &session_id, &vm_id), + RequestPayload::ConfigureVm(ConfigureVmRequest { + mounts: vec![MountDescriptor { + guest_path: String::from("/workspace"), + read_only: false, + plugin: MountPluginDescriptor { + id: String::from("js_bridge"), + config: json!({ "mountId": "mount-root" }).to_string(), + }, + }], + software: Vec::new(), + permissions: None, + module_access_cwd: None, + instructions: Vec::new(), + projected_modules: Vec::new(), + command_permissions: std::collections::HashMap::new(), + loopback_exempt_ports: Vec::new(), + packages: Vec::new(), + packages_mount_at: String::new(), + bootstrap_commands: Vec::new(), + tool_shim_commands: Vec::new(), + }), + )) + .expect("configure js_bridge mount"); + + let vm = sidecar.vms.get_mut(&vm_id).expect("configured vm"); + let entries = vm + .kernel + .filesystem_mut() + .read_dir("/workspace") + .expect("readdir of js_bridge mount root"); + assert!( + entries.iter().any(|entry| entry == "hello.txt"), + "mount-root readdir should list seeded entries, got {entries:?}" + ); + + let calls = calls.lock().expect("lock js bridge calls"); + assert!( + calls + .iter() + .any(|call| call.operation == "readDir" + || call.operation == "readDirWithTypes"), + "readdir must reach the bridge; recorded operations: {:?}", + calls + .iter() + .map(|call| call.operation.clone()) + .collect::>() + ); + } + + #[test] + fn configure_vm_js_bridge_mount_root_readdir_without_bridge_realpath() { + configure_vm_js_bridge_mount_readdir_of_mount_root_survives_broken_driver_realpath(); + } + fn configure_vm_instantiates_sandbox_agent_mounts_through_the_plugin_registry() { let server = MockSandboxAgentServer::start("agentos-native-sidecar-sandbox", None); fs::write(server.root().join("hello.txt"), "hello from sandbox") @@ -21012,6 +21115,7 @@ console.log(JSON.stringify({ configure_vm_js_bridge_mount_rejects_oversized_read_payloads(); configure_vm_js_bridge_mount_rejects_pread_payloads_above_requested_length(); configure_vm_js_bridge_mount_maps_callback_errors_to_errno_codes(); + configure_vm_js_bridge_mount_readdir_of_mount_root_survives_broken_driver_realpath(); configure_vm_instantiates_sandbox_agent_mounts_through_the_plugin_registry(); configure_vm_instantiates_s3_mounts_through_the_plugin_registry(); configure_vm_instantiates_object_s3_mounts_through_the_plugin_registry();