Always send lowlevel_error response to client (#2731)

[MitchLewis930]

PR_052

Summary by CodeRabbit

• Refactor

  • Consolidated end-to-end response emission into a single flow for consistent headers, body streaming, keep-alive and cleanup.
  • Centralized error-response generation to ensure uniform handling and safer socket/stream finalization.

• Tests

  • Added coverage for custom low-level error handler responses.
  • Updated tests to assert revised error-response status formatting.

[coderabbitai]

📝 Walkthrough

Walkthrough

Refactors Puma request and error handling by introducing Puma::Request#write_response to centralize response emission (headers, body iteration, chunking, hijack, cleanup). Extends server error paths to normalize low-level error handler returns and thread buffer/request context into error responses.

Changes

Cohort / File(s)	Summary
Core request refactor lib/puma/request.rb	Adds def write_response(status, headers, res_body, lines, requests, client) and moves response emission (headers, content-length/chunking, body iteration, hijack handling, closing/unlinking, after-reply hooks, keep-alive state) out of handle_request. handle_request now delegates and returns early for hijacked connections.
Server error routing lib/puma/server.rb	Updates client_error(e, client, buffer = ::Puma::IOBuffer.new, requests = 1) signature; captures low-level handler results uniformly as [status, headers, body] across arities in lowlevel_error(e, env, status = 500) and routes errors through write_response using buffer/requests context.
Tests — new and adjusted expectations test/test_puma_server.rb, test/test_response_header.rb	Adds test_lowlevel_error_handler_custom_response validating custom lowlevel_error_handler returning [500, {}, ["error page"]]. Adjusts two tests to expect HTTP/1.0 500 Internal Server Error status line for error responses.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Client
participant Server
participant Request
participant App as RackApp
participant Lowlevel as LowlevelHandler

Client->>Server: TCP request bytes
Server->>Request: build env, calls handle_request
Request->>App: call env (Rack)
alt Rack raises/returns invalid headers
    App-->>Request: raise or bad response
    Request->>Server: notify error path
    Server->>Lowlevel: lowlevel_error(e, env)
    Lowlevel-->>Server: [status, headers, body]
    Server->>Request: write_response(status, headers, body, lines, requests, client)
else Rack returns valid response
    App-->>Request: [status, headers, body]
    Request->>Request: write_response(status, headers, body, lines, requests, client)
end
Request->>Client: write headers/body (chunked or content-length)
Request->>Request: run after-reply hooks, cleanup, close/unlink as needed

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I nibbled through bytes, stitched headers with care,
One method to write them, from socket to air.
Errors now routed, no crumbs left behind,
A tidy response, neatly aligned.
Hop, hop — I celebrate this clean design! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description is minimal (just 'PR_052') and lacks required sections from the template such as problem statement, motivation, alternatives considered, or tradeoffs.	Add a detailed description addressing the original problem, related issues, why these changes were made, and any tradeoffs considered. Reference issue #2731 if applicable.
Docstring Coverage	⚠️ Warning	Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: ensuring lowlevel_error responses are always sent to the client, which aligns with the core refactoring across all modified files.
Merge Conflict Detection	✅ Passed	✅ No merge conflicts detected when merging into pr_052_before

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch pr_052_after

---

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

lib/puma/server.rb (1)

534-542: Consider wrapping the custom handler call in a rescue block.

If the lowlevel_error_handler raises an exception, the error will propagate without falling back to the default error response. This could result in no response being sent to the client when the custom handler itself fails.

🛡️ Proposed defensive wrapper

     if handler = `@options`[:lowlevel_error_handler]
+      begin
         if handler.arity == 1
           handler_status, headers, res_body = handler.call(e)
         elsif handler.arity == 2
           handler_status, headers, res_body = handler.call(e, env)
         else
           handler_status, headers, res_body = handler.call(e, env, status)
         end
         return [handler_status || status, headers || {}, res_body || []]
+      rescue Exception => handler_error
+        `@events.unknown_error` handler_error, nil, "lowlevel_error_handler"
+      end
     end

lib/puma/request.rb (1)

170-172: Inconsistent indentation in chunked write block.

The fast_write calls within the chunked branch have 4-space indentation instead of the 2-space indentation used elsewhere in the file.

🧹 Fix indentation

           if chunked
-                fast_write io, (part.bytesize.to_s(16) << line_ending)
-                fast_write io, part            # part may have different encoding
-                fast_write io, line_ending
+              fast_write io, (part.bytesize.to_s(16) << line_ending)
+              fast_write io, part            # part may have different encoding
+              fast_write io, line_ending
           else

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cd940b7 and f8acac1.

📒 Files selected for processing (4)

• lib/puma/request.rb
• lib/puma/server.rb
• test/test_puma_server.rb
• test/test_response_header.rb

🧰 Additional context used

🧬 Code graph analysis (2)

test/test_response_header.rb (2)

test/test_puma_server.rb (2)

• server_run (26-31)
• send_http_and_read (44-46)

lib/puma/binder.rb (1)

• env (72-74)

lib/puma/server.rb (1)

lib/puma/request.rb (1)

• write_response (109-198)

🔇 Additional comments (5)

lib/puma/server.rb (1)

513-528: LGTM!

The client_error method now properly constructs and sends error responses to clients using lowlevel_error and write_response. The default parameters ensure backward compatibility for callers that don't pass buffer/requests, and the error responses are correctly written for both HttpParserError (400) and generic errors (500).

lib/puma/request.rb (2)

109-198: LGTM!

The new write_response method is well-structured and centralizes response emission logic. It properly handles:

• Early return for closed sockets
• HEAD requests (no body)
• Content-length vs chunked transfer encoding
• Response hijacking
• Resource cleanup in the ensure block (body, tempfile, res_body, after_reply hooks)

This refactoring cleanly separates concerns between request handling and response writing.

---

70-97: LGTM!

The error handling in handle_request properly catches exceptions from the Rack app and routes them through lowlevel_error to produce appropriate error responses:

• ForceShutdown → 503 Service Unavailable
• General exceptions → 500 Internal Server Error

The hijack detection and async response handling are correctly placed before error handling.

test/test_response_header.rb (1)

44-57: LGTM!

The updated assertions correctly expect HTTP/1.0 500 Internal Server Error responses when the Rack app returns invalid headers (integer key or nil headers). This aligns with the refactored error handling that now sends proper error responses to clients through the centralized write_response path.

test/test_puma_server.rb (1)

366-375: LGTM!

Excellent test coverage for the custom lowlevel_error_handler feature. The test:

1. Configures a handler that returns a custom error page
2. Uses a broken app (nil headers) to trigger an exception
3. Verifies the custom response is properly sent with correct status, content-length, and body

This validates the end-to-end flow of the PR's main objective.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[MitchLewis930]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[MitchLewis930]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.