Skip to content

fix(web-incoming): emit econnreset on client disconnect#115

Merged
pi0 merged 1 commit intomainfrom
fix/client-disconnect-error
Mar 25, 2026
Merged

fix(web-incoming): emit econnreset on client disconnect#115
pi0 merged 1 commit intomainfrom
fix/client-disconnect-error

Conversation

@pi0
Copy link
Copy Markdown
Member

@pi0 pi0 commented Mar 25, 2026

Summary

  • Fix race condition where client disconnect ECONNRESET errors were incorrectly routed to the error event instead of econnreset
  • Replace req.socket.destroyed check with !req.socket?.writable which is set earlier during socket teardown
  • Add regression test that simulates the race (socket non-writable but not yet destroyed)

Upstream ref: http-party/node-http-proxy#1542

Detail

The createErrorHandler in web-incoming.ts checks whether an ECONNRESET error is caused by a client disconnect. The original check (req.socket.destroyed) can be false during a timing race where the socket is closing but not yet fully destroyed. The upstream fix used req.aborted, but that property is deprecated since Node.js 17.

This fix uses !req.socket?.writable instead, which:

  • Is false when the client has disconnected (socket closing/closed)
  • Is true when the client is still connected (e.g. proxy timeout)
  • Works reliably across modern Node.js versions
  • Also handles the edge case where req.socket is null via optional chaining

Test plan

  • New regression test reproduces the race condition and verifies econnreset event fires
  • Existing proxyTimeout test still passes (timeout ECONNRESET correctly goes to error event)
  • Full test suite passes (229 tests)

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced detection and handling of connection reset errors to improve proxy reliability and error reporting.
  • Tests

    • Added test coverage for client disconnect race conditions to ensure proper event emission during connection failures.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 25, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 74e1aa67-3940-42f3-9c2f-62ab7bbb3c63

📥 Commits

Reviewing files that changed from the base of the PR and between b7075ab and b12a68f.

📒 Files selected for processing (2)
  • src/middleware/web-incoming.ts
  • test/http-proxy.test.ts

📝 Walkthrough

Walkthrough

The ECONNRESET error handling in the stream proxy middleware was updated to check socket writability instead of destruction status. When connection reset occurs with a non-writable socket, the middleware emits an econnreset event. A test case validates this behavior under client disconnect race conditions.

Changes

Cohort / File(s) Summary
Error Handling Logic
src/middleware/web-incoming.ts
Modified ECONNRESET condition from checking req.socket?.destroyed to !req.socket?.writable. When true, the middleware now emits server.emit("econnreset", ...) and destroys proxyReq.
Test Coverage
test/http-proxy.test.ts
Added new test case under client disconnect that simulates a race condition by intercepting proxyReq event, toggling socket state (writable to false, destroyed to false), emitting ECONNRESET error, and asserting econnreset event emission.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A socket's writable state, now we test with care,
Race conditions caught before they snare,
One line changed, but logic refined,
To handle resets with peace of mind! 🔌✨

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/client-disconnect-error

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 25, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.11%. Comparing base (b7075ab) to head (b12a68f).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #115   +/-   ##
=======================================
  Coverage   96.11%   96.11%           
=======================================
  Files           8        8           
  Lines         644      644           
  Branches      244      244           
=======================================
  Hits          619      619           
  Misses         24       24           
  Partials        1        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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Check `!req.socket?.writable` instead of `req.socket.destroyed` in the
proxy error handler. The socket `writable` flag is set to false earlier
than `destroyed` during client disconnection, avoiding a race condition
where ECONNRESET errors were incorrectly routed to the `error` event.

Ref: http-party/node-http-proxy#1542
@pi0 pi0 force-pushed the fix/client-disconnect-error branch from 444b1db to b12a68f Compare March 25, 2026 20:05
@pi0 pi0 merged commit 5efcaba into main Mar 25, 2026
6 of 7 checks passed
@pi0 pi0 deleted the fix/client-disconnect-error branch March 25, 2026 20:06
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.

1 participant