Skip to content

fix(proxy): handle client-aborted streaming requests#209

Merged
bnema merged 1 commit into
mainfrom
fix/sse-client-abort-proxy-errors
Jun 2, 2026
Merged

fix(proxy): handle client-aborted streaming requests#209
bnema merged 1 commit into
mainfrom
fix/sse-client-abort-proxy-errors

Conversation

@bnema

@bnema bnema commented Jun 2, 2026

Copy link
Copy Markdown
Owner

Summary

  • Treat client-canceled reverse proxy requests as client disconnects instead of upstream failures.
  • Preserve real upstream failures as 503 responses while returning 499 for canceled proxy requests.
  • Let http.ErrAbortHandler panics escape Gordon panic recovery so Go suppresses benign streaming abort logs.

Test Plan

  • go test ./internal/adapters/in/http/proxy ./internal/adapters/in/http/middleware
  • go test ./...
  • golangci-lint run ./...
  • go vet ./...

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Improved handling of aborted reverse proxy requests to prevent incorrect error logging and 500 responses.
    • Added client disconnection detection for proxied requests, responding with appropriate status code instead of treating as service failure.

Copilot AI review requested due to automatic review settings June 2, 2026 18:02
@coderabbitai

coderabbitai Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@bnema, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 34 minutes and 21 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: c54942b0-3f3e-4335-9052-2805cfc527b4

📥 Commits

Reviewing files that changed from the base of the PR and between afb5326 and 8823050.

📒 Files selected for processing (4)
  • internal/adapters/in/http/middleware/logging.go
  • internal/adapters/in/http/middleware/logging_test.go
  • internal/adapters/in/http/proxy/forwarder.go
  • internal/adapters/in/http/proxy/forwarder_test.go
📝 Walkthrough

Walkthrough

The PR improves HTTP error handling across middleware and proxy layers. The middleware now correctly re-panics http.ErrAbortHandler instead of converting it to a 500 response. The proxy detects client disconnection/cancellation errors and returns a 499 status instead of treating them as upstream failures.

Changes

Client Abort and Disconnect Handling

Layer / File(s) Summary
Middleware panic recovery for handler abort
internal/adapters/in/http/middleware/logging.go, internal/adapters/in/http/middleware/logging_test.go
PanicRecovery adds detection of http.ErrAbortHandler panics via isAbortHandlerPanic helper and re-panics the abort handler instead of logging and converting to 500; test verifies re-panic behavior.
Proxy client disconnect detection and 499 response
internal/adapters/in/http/proxy/forwarder.go, internal/adapters/in/http/proxy/forwarder_test.go
Both forwardToTarget and forwardToRegistry error handlers now detect client cancellation/abort via isClientDisconnectError helper and return HTTP 499 status; request overflow detection refactored to errors.AsType; test stubs transport to return context.Canceled and verifies 499 response.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A client that fled or panics mid-stream,
No longer becomes the middleware's dream;
Now 499 shines where errors once loomed,
And abort handlers hop free from their gloom! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title "fix(proxy): handle client-aborted streaming requests" accurately summarizes the main change: handling client-aborted requests in the proxy layer with appropriate status codes and panic recovery.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/sse-client-abort-proxy-errors

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

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

This PR improves reverse-proxy handling of client-aborted streaming requests by distinguishing client cancellations from true upstream failures, and by letting http.ErrAbortHandler panics propagate so Go’s HTTP server suppresses benign abort logging.

Changes:

  • Map proxy errors caused by request cancellation (context.Canceled) to HTTP 499 (Client Closed Request) instead of returning 503.
  • Preserve 503 responses for genuine upstream/proxy failures while downgrading cancellation logging to debug level.
  • Update panic recovery middleware to rethrow http.ErrAbortHandler, with new tests covering both behaviors.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
internal/adapters/in/http/proxy/forwarder.go Returns 499 for canceled proxy requests and adjusts error classification/logging.
internal/adapters/in/http/proxy/forwarder_test.go Adds a unit test asserting 499 is returned on context.Canceled from the transport.
internal/adapters/in/http/middleware/logging.go Updates panic recovery to rethrow http.ErrAbortHandler so Go suppresses abort logs.
internal/adapters/in/http/middleware/logging_test.go Adds coverage ensuring http.ErrAbortHandler panics are rethrown (not converted to 500).

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

Comment on lines +323 to +325
func clientClosedRequest(w http.ResponseWriter) {
w.WriteHeader(statusClientClosedRequest)
}
@bnema bnema force-pushed the fix/sse-client-abort-proxy-errors branch 2 times, most recently from 5d7fa7d to 8823050 Compare June 2, 2026 18:12
@bnema bnema merged commit b59533b into main Jun 2, 2026
7 checks passed
@bnema bnema deleted the fix/sse-client-abort-proxy-errors branch June 2, 2026 18:21
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