Skip to content

fix(native-sidecar): always answer sync-RPC waiters on handler failure #1809

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
NathanFlurry wants to merge 1 commit into
base: stack/fix-native-sidecar-tolerate-guest-filesystem-quota-errors-in-shadow-sync-muvrpwuz
Choose a base branch
from
Open

fix(native-sidecar): always answer sync-RPC waiters on handler failure #1809

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 49 additions & 2 deletions crates/native-sidecar/src/execution/process_events.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
71 changes: 54 additions & 17 deletions crates/native-sidecar/src/service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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"
);
}
}

Expand Down