Skip to content

Return 5XX for requests that land in a reused session that crashes#540

Merged
ulyssa merged 1 commit intomainfrom
ulyssa/reused-session-5xx
Oct 7, 2025
Merged

Return 5XX for requests that land in a reused session that crashes#540
ulyssa merged 1 commit intomainfrom
ulyssa/reused-session-5xx

Conversation

@ulyssa
Copy link
Contributor

@ulyssa ulyssa commented Oct 7, 2025

This PR moves the logic for turning an error into a 500 response to happen right after the guest crashes by calling Session::close_downstream_response_sender so that it will still apply to reused sessions. Fortunately, the ways that a guest can exit here are much simpler than in Compute, so this ends up being a simpler change.

@ulyssa ulyssa requested review from acw and dgohman-fastly and removed request for acw October 7, 2025 22:13
Copy link
Contributor

@acw acw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine, although I have one question about a panic.

anyhow_response(e)
} else {
resp
panic!("failed to run guest: {err}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why panic here, rather than turning the execution error into an anyhow response?

Copy link
Contributor Author

@ulyssa ulyssa Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm doing this to continue matching the behaviour from here:

Viceroy/src/execute.rs

Lines 592 to 606 in f2ea186

Ok(_) => (Response::new(Body::empty()), None),
Err(ExecutionError::WasmTrap(e)) => {
event!(
Level::ERROR,
"There was an error handling the request {}",
e.to_string()
);
#[allow(unused_mut)]
let mut response = Response::builder()
.status(hyper::StatusCode::INTERNAL_SERVER_ERROR)
.body(Body::empty())
.unwrap();
(response, Some(e))
}
Err(e) => panic!("failed to run guest: {}", e),

@ulyssa ulyssa merged commit 0d07ae2 into main Oct 7, 2025
13 checks passed
@ulyssa ulyssa deleted the ulyssa/reused-session-5xx branch October 7, 2025 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants