fix: fall back to the base transport when websocket handshakes are rejected outright - #2285
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesWebSocket connectivity fallback
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ConnectionManager
participant WebSocket
participant BaseTransport
ConnectionManager->>WebSocket: Check connectivity once
WebSocket-->>ConnectionManager: Return availability
alt Connectivity unavailable
ConnectionManager->>BaseTransport: Start connection
else Connectivity available
ConnectionManager-->>ConnectionManager: Transition to disconnected
end
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
github-actions
Bot
temporarily deployed
to
staging/pull/2285/features
August 10, 2026 14:00
Inactive
github-actions
Bot
temporarily deployed
to
staging/pull/2285/features
August 10, 2026 14:13
Inactive
github-actions
Bot
temporarily deployed
to
staging/pull/2285/bundle-report
August 10, 2026 14:13
Inactive
VeskeR
force-pushed
the
fix-ws-fast-fail-base-transport-fallback
branch
from
August 10, 2026 14:53
192db23 to
ecc6608
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2285/features
August 10, 2026 14:54
Inactive
github-actions
Bot
temporarily deployed
to
staging/pull/2285/bundle-report
August 10, 2026 14:54
Inactive
VeskeR
force-pushed
the
fix-ws-fast-fail-base-transport-fallback
branch
from
August 10, 2026 15:02
ecc6608 to
26e9bce
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2285/features
August 10, 2026 15:03
Inactive
github-actions
Bot
temporarily deployed
to
staging/pull/2285/bundle-report
August 10, 2026 15:03
Inactive
VeskeR
marked this pull request as ready for review
August 10, 2026 15:14
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/common/lib/transport/connectionmanager.ts`:
- Around line 1510-1512: Add a concise, concrete remediation to the ErrorInfo
created in the wsAvailable branch of the connection fallback flow, guiding users
to investigate WebSocket connectivity for the configured hosts. Verify the
wording matches the documented Ably error path and preserves the existing
message, code, and status passed to giveUp().
In `@test/realtime/transports.test.js`:
- Around line 258-266: Update the ws_rejected_immediately_with_no_base_transport
test and its rejectingWebSocket helper to record connectivity-check URLs, then
assert that no connectivity checks were initiated when only the web_socket
transport is configured. Keep the existing disconnected cleanup flow unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c986c20-ede5-46ff-8421-0ff02560a8f9
📒 Files selected for processing (2)
src/common/lib/transport/connectionmanager.tstest/realtime/transports.test.js
Comment on lines
+1510
to
+1512
| if (wsAvailable) { | ||
| giveUp(new ErrorInfo('Unable to connect (and no more fallback hosts to try)', 80003, 404)); | ||
| } else { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a verified remediation to this public error.
giveUp() sends this SDK-originating ErrorInfo through notifyState, so application code can receive it on the public connection-state boundary. Add a concise remediation that helps users investigate WebSocket failures for the configured hosts. Verify the wording against the documented Ably error path before shipping it.
As per coding guidelines: Add a concrete remediation to every publicly reachable SDK-originating throw site when it provides actionable value beyond the message.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/common/lib/transport/connectionmanager.ts` around lines 1510 - 1512, Add
a concise, concrete remediation to the ErrorInfo created in the wsAvailable
branch of the connection fallback flow, guiding users to investigate WebSocket
connectivity for the configured hosts. Verify the wording matches the documented
Ably error path and preserves the existing message, code, and status passed to
giveUp().
Source: Coding guidelines
Comment on lines
+258
to
+266
| it('ws_rejected_immediately_with_no_base_transport', function (done) { | ||
| const helper = this.test.helper; | ||
| Config.WebSocket = rejectingWebSocket(); | ||
| const realtime = helper.AblyRealtime({ transports: ['web_socket'] }); | ||
|
|
||
| realtime.connection.once('disconnected', function () { | ||
| helper.closeAndFinish(done, realtime); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert that no connectivity check starts without a base transport.
This test passes if checkWsConnectivity() runs, because rejectingWebSocket() rejects that check immediately and the client still becomes disconnected. Record connectivity-check URLs in rejectingWebSocket() and assert that the check count is zero here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/realtime/transports.test.js` around lines 258 - 266, Update the
ws_rejected_immediately_with_no_base_transport test and its rejectingWebSocket
helper to record connectivity-check URLs, then assert that no connectivity
checks were initiated when only the web_socket transport is configured. Keep the
existing disconnected cleanup flow unchanged.
Closed
owenpearson
approved these changes
Aug 10, 2026
| 'ConnectionManager.abandonWebSocketForBaseTransport()', | ||
| 'websocket connectivity appears to be unavailable; trying base transport', | ||
| ); | ||
| this.abandonedWebSocket = true; |
Member
There was a problem hiding this comment.
nit: we should remove the duplication between webSocketGiveUpTimer and this
Contributor
Author
There was a problem hiding this comment.
Merged
…right The websocket -> base transport failover was only ever triggered by the webSocketGiveUpTimer, which fires webSocketConnectTimeout after a connection attempt begins. That only helps when a websocket attempt is slow. A network that actively rejects the upgrade request fails every candidate host within milliseconds, so tryTransportWithFallbacks exhausts the host list and notifies `disconnected` before either the slow timer or the give-up timer can fire, and notifyState cancels both. Such a client rotates its fallback hosts on web_socket, gives up with 80003, and repeats forever, never attempting the base transport. Since 2.0 replaced connect-on-base-then-upgrade with connect-on-websocket-then-fall-back there is no other route to it, so these clients simply never connect. Give the failover a second trigger: exhausting every host on websocket now runs the websocket connectivity check and decides on its result rather than writing the attempt off. If the check fails we take up the base transport immediately; if it hangs the give-up timer still does so as before; and if it succeeds - websockets do work here, so it is the hosts that are unhealthy - we go to `disconnected` as we always have, since the base transport talks to those same hosts.
VeskeR
force-pushed
the
fix-ws-fast-fail-base-transport-fallback
branch
from
August 10, 2026 16:32
26e9bce to
2f34fdb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A browser behind a proxy that rejects the WebSocket upgrade — a 403, say — never connects. It rotates through its fallback hosts on
web_socket, reports80003 Unable to connect (and no more fallback hosts to try), and repeats indefinitely.xhr_pollingis never attempted.If the proxy instead silently drops the handshake, the same client connects over
xhr_pollingin ~10s. Only the fast-failure case is broken.The failover to the base transport hangs off
webSocketGiveUpTimer(webSocketConnectTimeout, 10s), which only helps when a websocket attempt is slow. On fast rejection,tryFallbackHosts()exhausts the host list in milliseconds and callsnotifyState('disconnected')— which cancels that timer. Since 2.0 replaced connect-on-base-then-upgrade with connect-on-websocket-then-fall-back, the timers are the only route to the base transport.Fix
Exhausting every host on
web_socketnow runs the websocket connectivity check and decides on its result, instead of writing the attempt off:webSocketConnectTimeoutdisconnected, since websockets work here and the base transport talks to the same unhealthy hostsThat last row is what keeps
try_fallback_hosts_on_placement_constraintpassing: a client told to move datacenter must not hop back to the primary host on a different transport. It is also the condition the give-up timer already applies, so the two paths now agree.This only affects attempts that fail on every host over websocket without the transport becoming viable. Clients with no base transport available skip the check entirely.
Testing
Three new tests in
test/realtime/transports.test.js, using arejectingWebSocket()drop-in that firesonerrorthenclose(1006)immediately — what a proxy 403 looks like to the browser WebSocket API:ws_rejected_immediately— falls back to the base transport. Times out at 60s onmain.ws_rejected_immediately_but_ws_connectivity_available— does not switch transport when websockets work on the network.ws_rejected_immediately_with_no_base_transport— goes straight todisconnected.All nine tests in the file pass on Chromium, Firefox and WebKit. The red CI legs are the pre-existing flaky suites (
presence,rest/message-operations,channel_backoff_*), each verified to fail onmaintoo.🤖 Generated with Claude Code
Summary by CodeRabbit