Skip to content

[SLES-2666] handle_end_invocation race condition #1024

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

Draft
pablomartinezbernardo wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

[SLES-2666] handle_end_invocation race condition #1024

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
16 changes: 8 additions & 8 deletions bottlecap/src/lifecycle/listener.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,16 +164,16 @@ impl Listener {
State((invocation_processor_handle, _, tasks)): State<ListenerState>,
request: Request,
) -> Response {
let (parts, body) = match extract_request_body(request).await {
Copy link
Member

@lucaspimentel lucaspimentel Feb 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Consider leaving a comment to explain why this should not be async (so somebody doesn't come and try to "optimize" it again later).

Something like:

extract_request_body must complete BEFORE returning 200 OK
to avoid a race condition. See SLES-2666.

Ok(r) => r,
Err(e) => {
error!("Failed to extract request body: {e}");
return (StatusCode::OK, json!({}).to_string()).into_response();
Copy link
Member

@lucaspimentel lucaspimentel Feb 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

handle_end_invocation returns StatusCode::OK on error but handle_start_invocation returns StatusCode::BAD_REQUEST (line 129). Should these be consistent?

If this was intentional, consider leaving a comment.

Copy link
Author

@pablomartinezbernardo pablomartinezbernardo Feb 13, 2026

Choose a reason for hiding this comment

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

This was intentional as in this is the behavior that does not introduce regressions: handle_end_invocation was never able to return anything other than 200, and consumers of this endpoint may not be expecting something other than 200. But yeah, a comment explaining why (or better yet, making consumers aware that this may not return 200) is a good idea.

}
};

let mut join_set = tasks.lock().await;
join_set.spawn(async move {
let (parts, body) = match extract_request_body(request).await {
Ok(r) => r,
Err(e) => {
error!("Failed to extract request body: {e}");
return;
}
};

Self::universal_instrumentation_end(&parts.headers, body, invocation_processor_handle)
.await;
});
Expand Down
Loading