Skip to content

Comments

Always send lowlevel_error response to client (#2731)#2

Open
MitchLewis930 wants to merge 1 commit intopr_052_beforefrom
pr_052_after
Open

Always send lowlevel_error response to client (#2731)#2
MitchLewis930 wants to merge 1 commit intopr_052_beforefrom
pr_052_after

Conversation

@MitchLewis930
Copy link

@MitchLewis930 MitchLewis930 commented Jan 30, 2026

PR_052


Note

Medium Risk
Touches core request/response and error-handling paths; behavior should be more consistent, but mistakes could impact error responses and connection handling under failure conditions.

Overview
Ensures Puma always sends the lowlevel_error Rack response back to the client for request/app failures, instead of using client.write_error or potentially dropping the response.

This refactors response writing by extracting Request#write_response and reusing it from both Request#handle_request and Server#client_error, and hardens lowlevel_error to tolerate handlers returning nil status/headers/body. Tests are updated to expect HTTP/1.0 error status lines and add coverage for a custom lowlevel_error_handler response on internal failures.

Written by Cursor Bugbot for commit f8acac1. This will update automatically on new commits. Configure here.

* Always send lowlevel_error response to client

* Add spec for lowlever error handler

* Make sure we have a clean buffer when starting response

* Simplify test

* Rename spec

* Add method docs

* Tweak the test

* Return 500 from lowlevel_error_handler in test

Co-authored-by: Patrik Ragnarsson <patrik@starkast.net>
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.

uncork_socket io

body.close
client.body.close if client.body
Copy link

Choose a reason for hiding this comment

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

After-reply callbacks may be invoked twice on error

Medium Severity

When an exception occurs during write_response (e.g., when the app returns invalid headers like nil or an integer key), the ensure block at line 194 runs after_reply.each { |o| o.call }. The exception then propagates to process_client, which calls client_error, which calls write_response again. The second call's ensure block will invoke the same callbacks again because env[RACK_AFTER_REPLY] still contains the same array. This violates the Rack specification that callbacks are invoked once after the response is sent, and can cause issues for non-idempotent callbacks like logging, metrics, or cleanup operations.

Additional Locations (1)

Fix in Cursor Fix in Web


status, headers, res_body = lowlevel_error(e, env, 500)
end
return false if closed_socket?(io)
Copy link

Choose a reason for hiding this comment

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

Early return bypasses resource cleanup in ensure block

Medium Severity

The new closed_socket?(io) check at line 113 returns false before entering the begin/ensure block. If the socket closes after the app executes but before write_response starts, resources are not cleaned up: client.body is not closed (file handle leak), res_body.close is not called, and rack.after_reply callbacks are not invoked (Rack spec violation). In the old code, response writing was inside the begin/ensure block of handle_request, so cleanup always ran.

Fix in Cursor Fix in Web

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