Skip to content

Map proxy errors to specific status codes instead of always 502#131

Open
alexspeller wants to merge 1 commit into
basecamp:mainfrom
alexspeller:client-closed-request-status
Open

Map proxy errors to specific status codes instead of always 502#131
alexspeller wants to merge 1 commit into
basecamp:mainfrom
alexspeller:client-closed-request-status

Conversation

@alexspeller

Copy link
Copy Markdown
Contributor

Problem

httputil.ReverseProxy's ErrorHandler is invoked for several distinct failure modes, but Thruster collapses all of them into a 502 (logged at info level as "Unable to proxy request").

The most damaging case is client disconnection. When a client closes its connection before the response is ready — an aborted fetch, a Turbo Frame swapping its src, a navigation away, an upstream load balancer giving up — the proxy is called with a context.Canceled error. The client is already gone, so the 502 is written to a closed socket and discarded, but the access log still records status=502. Ordinary client cancellations therefore show up as server errors and pollute 5xx dashboards.

Change

Distinguish the error cases, mirroring the handling in kamal-proxy's handleProxyError:

Condition Before After
Client disconnect (context.Canceled) 502 499 Client Closed Request (nginx convention), logged at debug
Upstream timeout (net.Error with Timeout()) 502 504 Gateway Timeout
Malformed chunked encoding from client 502 400 Bad Request
Connection refused / EOF / other 502 502 (unchanged)
Request entity too large 413 413 (unchanged)

The client-disconnect case still sets a status code (rather than skipping the write) so the request is recorded in the access log — just as a 499 rather than a 502, keeping it out of the 5xx bucket. Only genuine upstream failures now land there.

Tests

Adds proxy_handler_test.go covering each branch, including an end-to-end test that performs a real mid-flight client cancellation through the proxy and asserts it is recorded as 499 rather than 502.

Copilot AI review requested due to automatic review settings June 4, 2026 11:07

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds more nuanced reverse-proxy error handling so client cancellations are recorded as 499 (instead of 5xx), and certain upstream/transport errors map to more appropriate HTTP status codes.

Changes:

  • Introduces StatusClientClosedRequest (499) and treats context.Canceled as a client-closed request without logging it as a proxy error.
  • Classifies timeout errors as 504 Gateway Timeout and chunked-encoding parse failures as 400 Bad Request.
  • Adds unit + end-to-end tests covering the new error classification behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
internal/proxy_handler.go Adds 499 status constant and expands ProxyErrorHandler to classify cancellations/timeouts/chunked-encoding errors.
internal/proxy_handler_test.go Adds tests validating status codes and logging behavior, including an end-to-end disconnect case.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/proxy_handler_test.go
Comment thread internal/proxy_handler_test.go Outdated
httputil.ReverseProxy's ErrorHandler is invoked for several distinct
failure modes, but Thruster collapsed all of them into a 502 (logged at
info level as "Unable to proxy request"). The most damaging case is client
disconnection: when a client closes its connection before the response is
ready (an aborted fetch, a Turbo Frame swapping its src, a navigation away,
an upstream load balancer giving up), the proxy is called with a
context.Canceled error. The client is already gone so the 502 is written to
a closed socket and discarded, but the access log still records status=502,
so ordinary client cancellations show up as server errors and pollute 5xx
dashboards.

Distinguish the error cases, mirroring the handling in kamal-proxy:

  * client disconnect (context.Canceled) -> 499 Client Closed Request
    (nginx convention), logged at debug rather than info
  * upstream timeout (net.Error with Timeout()) -> 504 Gateway Timeout
  * malformed chunked encoding from the client -> 400 Bad Request
  * everything else (connection refused, EOF, ...) -> 502 as before

Request-entity-too-large continues to return 413. Only genuine upstream
failures now land in the 5xx bucket.
@alexspeller alexspeller force-pushed the client-closed-request-status branch from 4dbb767 to 2e9120b Compare June 4, 2026 11:24
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.

3 participants