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

PR_052

* 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>
@greptile-apps
Copy link

greptile-apps bot commented Jan 30, 2026

Greptile Overview

Greptile Summary

Refactored error response handling to ensure lowlevel_error responses are always sent to clients

  • Extracted write_response method from handle_request to enable reuse in error paths
  • Modified client_error in lib/puma/server.rb to call lowlevel_error + write_response instead of client.write_error
  • Ensured custom error handlers return normalized Rack tuples [status || default_status, headers || {}, body || []]
  • Fixed indentation for chunked transfer encoding code
  • Added nil-safety check client.body.close if client.body in cleanup
  • Updated tests to verify custom error handler responses are sent correctly

Confidence Score: 5/5

  • Safe to merge - well-tested refactoring that improves error handling consistency
  • Code follows existing patterns, adds proper test coverage, and fixes a bug where custom error handlers weren't being sent to clients. The refactoring reduces code duplication and improves maintainability.
  • No files require special attention

Important Files Changed

Filename Overview
lib/puma/request.rb Extracted write_response method to enable reuse in error handling paths; simplified request flow by removing nested begin blocks
lib/puma/server.rb Modified client_error to send proper error responses via lowlevel_error + write_response; ensured handler return values are normalized to proper Rack tuple format

Sequence Diagram

sequenceDiagram
    participant Client
    participant Server
    participant Request
    participant App
    
    Client->>Server: HTTP Request
    Server->>Server: process_client()
    Server->>Request: handle_request()
    
    alt Normal Flow
        Request->>App: @app.call(env)
        App-->>Request: [status, headers, body]
        Request->>Request: write_response()
        Request->>Client: HTTP Response
    else ThreadPool::ForceShutdown
        Request->>Request: lowlevel_error(503)
        Request->>Request: write_response()
        Request->>Client: 503 Error Response
    else Exception in App
        Request->>Request: lowlevel_error(500)
        Request->>Request: write_response()
        Request->>Client: 500 Error Response
    else Client I/O Error (HttpParserError)
        Server->>Server: client_error()
        Server->>Server: lowlevel_error(400)
        Server->>Request: write_response()
        Request->>Client: 400 Error Response
    else Client I/O Error (Other)
        Server->>Server: client_error()
        Server->>Server: lowlevel_error(500)
        Server->>Request: write_response()
        Request->>Client: 500 Error Response
    end
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

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