Add support for rack.response_finished (#3681)

[MitchLewis930]

User description

PR_059

---

PR Type

Enhancement

---

Description

• Add support for rack.response_finished callback mechanism

• Implement reverse-order callback invocation per Rack spec

• Pass environment, status, headers, and error to callbacks

• Rename exception variable e to error for clarity

---

Diagram Walkthrough

flowchart LR
  A["Request handling"] --> B["Initialize rack.response_finished array"]
  B --> C["Execute Rack app"]
  C --> D["Capture status, headers, error"]
  D --> E["Invoke callbacks in reverse order"]
  E --> F["Pass env, status, headers, error to each callback"]

Loading

File Walkthrough

	Relevant files
Enhancement	const.rb Add rack.response_finished constant                                            lib/puma/const.rb Add RACK_RESPONSE_FINISHED constant definition Define constant as "rack.response_finished" string +1/-0      binder.rb Initialize rack callback extensions in environment              lib/puma/binder.rb Initialize RACK_AFTER_REPLY and RACK_RESPONSE_FINISHED to nil in default environment Set both callbacks as available rack extensions in request environment +4/-1      request.rb Implement rack.response_finished callback handling              lib/puma/request.rb Add error variable initialization to capture exceptions during request handling Initialize RACK_RESPONSE_FINISHED array in request environment Implement reverse-order callback invocation for rack.response_finished callbacks Pass environment, status, headers, and error to each callback Rename exception variable e to error throughout exception handlers for consistency +18/-6
Tests	test_rack_server.rb Add tests for rack.response_finished callback                        test/test_rack_server.rb Add test_rack_response_finished test verifying callbacks invoked in reverse order Add test_rack_response_finished_on_error test verifying error is passed to callbacks Update server initialization to use Puma::LogWriter.null for cleaner test output Verify callback receives correct environment, status, headers, and error parameters +53/-1

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Async/unknown status: rack.response_finished callbacks are invoked in the ensure block and may receive status/headers that are unset (e.g., early return :async or other atypical flows), so edge-case behavior needs confirmation against intended Rack/Puma semantics. Referred Code if response_finished = env[RACK_RESPONSE_FINISHED] response_finished.reverse_each do |o| begin o.call(env, status, headers, error) rescue StandardError => e @log_writer.debug_error e end end end Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Ensure callbacks run on all errors Initialize status, headers, and res_body to nil at the start of handle_request to prevent a NameError in the ensure block if an error occurs before they are assigned. lib/puma/request.rb [50-159] def handle_request(client, requests) env = client.env io_buffer = client.io_buffer socket = client.io # io may be a MiniSSL::Socket app_body = nil error = nil - - return false if closed_socket?(socket) - - if client.http_content_length_limit_exceeded - return prepare_response(413, {}, ["Payload Too Large"], requests, client) - end - - # ... - - req_env_post_parse env + status = headers = res_body = nil # A rack extension. If the app writes #call'ables to this # array, we will invoke them when the request is done. # env[RACK_AFTER_REPLY] ||= [] env[RACK_RESPONSE_FINISHED] ||= [] begin + return false if closed_socket?(socket) + + if client.http_content_length_limit_exceeded + return prepare_response(413, {}, ["Payload Too Large"], requests, client) + end + + # ... + + req_env_post_parse env + if @supported_http_methods == :any || @supported_http_methods.key?(env[REQUEST_METHOD]) status, headers, app_body = @thread_pool.with_force_shutdown do @app.call(env) end res_body = app_body else # ... end if status == -1 # ... end + prepare_response(status, headers, res_body, requests, client) rescue ThreadPool::ForceShutdown => error # ... status, headers, res_body = lowlevel_error(error, env, 503) + prepare_response(status, headers, res_body, requests, client) rescue Exception => error # ... status, headers, res_body = lowlevel_error(error, env, 500) - end - prepare_response(status, headers, res_body, requests, client) -ensure - io_buffer.reset - uncork_socket client.io - app_body.close if app_body.respond_to? :close - client&.tempfile_close - if after_reply = env[RACK_AFTER_REPLY] - after_reply.each do |o| - begin - o.call - rescue StandardError => e - @log_writer.debug_error e + prepare_response(status, headers, res_body, requests, client) + ensure + io_buffer.reset + uncork_socket client.io + app_body.close if app_body.respond_to? :close + client&.tempfile_close + if after_reply = env[RACK_AFTER_REPLY] + after_reply.each do |o| + begin + o.call + rescue StandardError => e + @log_writer.debug_error e + end end end - end - if response_finished = env[RACK_RESPONSE_FINISHED] - response_finished.reverse_each do |o| - begin - o.call(env, status, headers, error) - rescue StandardError => e - @log_writer.debug_error e + if response_finished = env[RACK_RESPONSE_FINISHED] + response_finished.reverse_each do |o| + begin + o.call(env, status, headers, error) + rescue StandardError => e + @log_writer.debug_error e + end end end end end [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that if an exception occurs before the main begin block, status and headers will be uninitialized, causing a NameError in the ensure block. Initializing these variables to nil is a valid fix for this bug. However, the proposed improved_code is an unnecessarily large refactoring of the entire method, when a single line addition would suffice.	Medium
General	Extract callback invocation helper Refactor the RACK_AFTER_REPLY and RACK_RESPONSE_FINISHED callback loops into a single, reusable helper method to reduce code duplication. lib/puma/request.rb [150-158] -if response_finished = env[RACK_RESPONSE_FINISHED] - response_finished.reverse_each do |o| - begin - o.call(env, status, headers, error) - rescue StandardError => e - @log_writer.debug_error e - end - end -end +invoke_callbacks(env, RACK_RESPONSE_FINISHED, [env, status, headers, error], reverse: true) Apply / Chat Suggestion importance[1-10]: 4 __ Why: The suggestion proposes a valid refactoring to reduce code duplication by extracting the callback invocation logic into a helper method. This would improve maintainability, but the two loops are not identical (one calls with arguments, the other does not), so the abstraction is not as straightforward as suggested.	Low
	Align callback array initialization Align the ||= operator for env[RACK_RESPONSE_FINISHED] with the line above it to improve code style and readability. lib/puma/request.rb [95-96] -env[RACK_AFTER_REPLY] ||= [] -env[RACK_RESPONSE_FINISHED] ||= [] +env[RACK_AFTER_REPLY] ||= [] +env[RACK_RESPONSE_FINISHED] ||= [] Apply / Chat Suggestion importance[1-10]: 2 __ Why: This is a minor code style suggestion for alignment that improves readability but has no functional impact.	Low
More