Skip to content

Re-panic http.ErrAbortHandler in recovery middleware#4065

Merged
amirejaz merged 1 commit intostacklok:mainfrom
gkatz2:fix/recovery-abort-handler
Mar 11, 2026
Merged

Re-panic http.ErrAbortHandler in recovery middleware#4065
amirejaz merged 1 commit intostacklok:mainfrom
gkatz2:fix/recovery-abort-handler

Conversation

@gkatz2
Copy link
Copy Markdown
Contributor

@gkatz2 gkatz2 commented Mar 10, 2026

Summary

  • The recovery middleware catches all panics indiscriminately, including
    http.ErrAbortHandler. This is a sentinel that Go's httputil.ReverseProxy
    panics with intentionally when a streaming response breaks mid-copy
    (reverseproxy.go:612). Go's HTTP server has built-in handling for it:
    catch it silently, close the connection, move on.
  • By intercepting this sentinel, the middleware logs noisy stack traces for
    normal proxy behavior, attempts to write a 500 to an already-in-flight
    response, and prevents Go's HTTP server from cleanly aborting the connection.
  • Check for http.ErrAbortHandler and re-panic it, letting Go's HTTP server
    handle it as designed.

Fixes #4064

Type of change

  • Bug fix

Test plan

  • Unit tests (task test)
  • Linting (task lint-fix)
  • Manual testing (describe below)

Deployed a local ToolHive build with this fix and tested with mcp-optimizer
traffic. Before the fix: 150 abort Handler panic stack traces in the log
over ~5 hours. After: zero.

Go's httputil.ReverseProxy intentionally panics with http.ErrAbortHandler
when a streaming response breaks mid-copy. Go's HTTP server has built-in
handling for this sentinel: catch it silently, close the connection, move on.

By intercepting ErrAbortHandler, the recovery middleware was:
- Logging noisy stack traces for something that isn't a bug
- Attempting to write http.Error() to an already-in-flight response
- Preventing Go's HTTP server from cleanly aborting the connection

This fix re-panics the sentinel, letting Go's HTTP server handle it as designed.

Fixes stacklok#4064

Signed-off-by: Greg Katz <gkatz@indeed.com>
@github-actions github-actions bot added the size/XS Extra small PR: < 100 lines changed label Mar 10, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 10, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.68%. Comparing base (406cb85) to head (ee55da1).
⚠️ Report is 30 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4065      +/-   ##
==========================================
+ Coverage   68.63%   68.68%   +0.04%     
==========================================
  Files         446      446              
  Lines       45435    45437       +2     
==========================================
+ Hits        31185    31208      +23     
+ Misses      11842    11820      -22     
- Partials     2408     2409       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@amirejaz amirejaz merged commit 7fce517 into stacklok:main Mar 11, 2026
37 of 39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Extra small PR: < 100 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Recovery middleware catches http.ErrAbortHandler, corrupting proxy responses

3 participants