From 9b41c0b2ebad7a9b023882456fc5eb84709a729a Mon Sep 17 00:00:00 2001 From: Nathan Flurry Date: Sun, 19 Jul 2026 00:52:47 -0700 Subject: [PATCH] fix(native-sidecar): always answer sync-RPC waiters on handler failure Three sites could complete or abandon guest-driven work without ever answering the guest, leaving it blocked until the ~31s bridge deadline: - service.rs: the deferred fd wakes ran with `?` BEFORE response delivery, so a wake failure discarded an already-successful result. The wakes are a side effect for OTHER waiters and must not fail this request. - service.rs: the chmod shadow mirror had the same pre-delivery `?`. - process_events.rs: an internal event is leased (popped) before dispatch, so a handler error destroyed it outright; when it was a sync-RPC request the waiter ceased to exist anywhere. The waiter is now answered with the errno. Answering is preferred over requeueing: a deterministic handler failure would otherwise be retried forever, and the guest is owed the errno rather than silence. --- .../src/execution/process_events.rs | 51 ++++++++++++- crates/native-sidecar/src/service.rs | 71 ++++++++++++++----- 2 files changed, 103 insertions(+), 19 deletions(-) diff --git a/crates/native-sidecar/src/execution/process_events.rs b/crates/native-sidecar/src/execution/process_events.rs index 777f8a8b2c..70e46312af 100644 --- a/crates/native-sidecar/src/execution/process_events.rs +++ b/crates/native-sidecar/src/execution/process_events.rs @@ -518,8 +518,55 @@ where // process events. Handle them immediately so a sibling // process can service sync RPCs while another request // waits on VM-local networking. - self.handle_execution_event(&vm_id, &process_id, event.into_event()) - .await?; + // + // The event was already LEASED (popped) by the poll + // above, so propagating a handler error with `?` + // consumed and destroyed it. When the event was a + // JavascriptSyncRpcRequest that request then ceased to + // exist anywhere, and the guest blocked on it could + // never be answered — it hung until the bridge deadline + // (~31 s). Capture the waiter's id before handing the + // event over and, on failure, deliver the error to that + // waiter. Answering is preferable to requeueing here: a + // deterministic handler failure would otherwise be + // retried forever, and the guest is owed the errno + // rather than silence. + let stranded_waiter = match event.event() { + ActiveExecutionEvent::JavascriptSyncRpcRequest(request) => { + Some(request.id) + } + _ => None, + }; + if let Err(error) = self + .handle_execution_event(&vm_id, &process_id, event.into_event()) + .await + { + if let Some(request_id) = stranded_waiter { + if let Some(process) = self + .vms + .get_mut(&vm_id) + .and_then(|vm| vm.active_processes.get_mut(&process_id)) + { + if let Err(respond_error) = process + .execution + .respond_javascript_sync_rpc_error( + request_id, + javascript_sync_rpc_error_code(&error), + error.to_string(), + ) + .or_else(ignore_stale_javascript_sync_rpc_response) + { + tracing::error!( + vm = %vm_id, + process = %process_id, + error = %respond_error, + "failed to answer a sync RPC stranded by a process-event handler error" + ); + } + } + } + return Err(error); + } } else { let PolledExecutionEvent { event, reservation } = event; let envelope = ProcessEventEnvelope { diff --git a/crates/native-sidecar/src/service.rs b/crates/native-sidecar/src/service.rs index aa420019c8..2fb32febd3 100644 --- a/crates/native-sidecar/src/service.rs +++ b/crates/native-sidecar/src/service.rs @@ -2811,16 +2811,38 @@ where other => other, }; + // Waking OTHER parked readers/writers is a side effect on behalf of + // other waiters; it is not this request's result. Propagating a wake + // failure with `?` here aborted the function BEFORE the response below + // was delivered, so the guest that just had its work completed + // successfully was never answered and blocked until the bridge deadline + // (~31 s) — while the side effect had already been applied. Capture the + // failure, always deliver this request's response, and report the wake + // error afterwards so it is never silently swallowed. + let mut wake_failure = None; if response.is_ok() && javascript_sync_rpc_may_make_fd_readable(&request) { if let Some(vm) = self.vms.get_mut(vm_id) { - Self::wake_ready_deferred_fd_reads(vm)?; + if let Err(error) = Self::wake_ready_deferred_fd_reads(vm) { + wake_failure = Some(("deferred fd reads", error)); + } } } if response.is_ok() && javascript_sync_rpc_may_make_fd_writable(&request) { if let Some(vm) = self.vms.get_mut(vm_id) { - Self::wake_ready_deferred_fd_writes(vm)?; + if let Err(error) = Self::wake_ready_deferred_fd_writes(vm) { + wake_failure = Some(("deferred fd writes", error)); + } } } + if let Some((phase, error)) = &wake_failure { + tracing::error!( + vm = %vm_id, + process = %process_id, + phase = %phase, + error = %error, + "waking parked fd waiters failed; answering the originating request anyway" + ); + } let Some(vm) = self.vms.get_mut(vm_id) else { log_stale_process_event( @@ -2848,21 +2870,36 @@ where "fs.chmodSync" | "fs.promises.chmod" ) { - let guest_path = - javascript_sync_rpc_arg_str(&request.args, 0, "filesystem chmod path")?; - let mode = - javascript_sync_rpc_arg_u32(&request.args, 1, "filesystem chmod mode")? & 0o7777; - let host_path = - shadow_host_path_for_process(&shadow_root, &process.guest_cwd, guest_path); - if host_path.exists() { - fs::set_permissions(&host_path, fs::Permissions::from_mode(mode)).map_err( - |error| { - SidecarError::Io(format!( - "failed to mirror chmod to shadow path {}: {error}", - host_path.display() - )) - }, - )?; + // Same rule as the fd wakes above: mirroring into the shadow root + // must not abort the function before the response is delivered, or + // a guest whose chmod already succeeded blocks until the bridge + // deadline. Report the mirror failure instead of propagating it. + let mirror = (|| -> Result<(), SidecarError> { + let guest_path = + javascript_sync_rpc_arg_str(&request.args, 0, "filesystem chmod path")?; + let mode = javascript_sync_rpc_arg_u32(&request.args, 1, "filesystem chmod mode")? + & 0o7777; + let host_path = + shadow_host_path_for_process(&shadow_root, &process.guest_cwd, guest_path); + if host_path.exists() { + fs::set_permissions(&host_path, fs::Permissions::from_mode(mode)).map_err( + |error| { + SidecarError::Io(format!( + "failed to mirror chmod to shadow path {}: {error}", + host_path.display() + )) + }, + )?; + } + Ok(()) + })(); + if let Err(error) = mirror { + tracing::error!( + vm = %vm_id, + process = %process_id, + error = %error, + "mirroring chmod into the shadow root failed; answering the originating request anyway" + ); } }