feat(ws): add an upgrade establishment timeout - #172
DavidIlie wants to merge 1 commit into
Conversation
Sorry, something went wrong.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: faf12e7e-683c-430a-b763-9f12f1e968e3 📥 CommitsReviewing files that changed from the base of the PR and between 54e4263 and 60932bf. 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 Walkthrough WalkthroughThe PR adds an optional establishmentTimeout to WebSocket proxy APIs. It enforces the deadline for upstream response headers or upgrades, cleans up timed-out connections, updates documentation and types, and adds coverage for timeout, cancellation, agent, response, and invalid-value behavior. ChangesWebSocket establishment timeout
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Client
participant Proxy
participant TimeoutHelper
participant Upstream
Client->>Proxy: request WebSocket upgrade
Proxy->>TimeoutHelper: setUpgradeTimeout
TimeoutHelper->>Upstream: monitor upgrade request and socket
Upstream-->>TimeoutHelper: response, upgrade, error, or close
TimeoutHelper-->>Proxy: report timeout or socket failure
Proxy-->>Client: close or complete connection
Suggested reviewers: pi0 Merge Risk: ⚪ Minimal · up to 60932 The opt-in WebSocket establishment timeout is typed, documented, enforced across both proxy paths, and covered for timeout, cleanup, cancellation, and successful establishment behavior. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
Explanation Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 6 files. (2 skipped: 2 unsupported.)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
Summary
Adds optional establishmentTimeout support to proxyUpgrade and ProxyServer.ws().
The deadline covers connection setup through upstream upgrade or final response headers. It is disabled by default and does not limit established tunnels or non-upgrade response bodies.
Timeouts report ERR_UPSTREAM_UPGRADE_TIMEOUT with statusCode: 504 and destroy the pending upstream request and client socket without writing an HTTP 504 response. Failure is reported even when a custom agent is waiting for a socket. Downstream cancellation preserves the original error where available, and deferred request errors do not cause duplicate delivery.
Verification
Summary by CodeRabbit
New Features
Documentation