Repository navigation
fix(net)!: stop one silent dial from stalling the reconnect loop - #6608
Conversation
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # js/net/MIGRATE.md
Main's @moq/qmux (#6528) refuses an empty negotiated subprotocol instead of falling back to the legacy framing, so the stubs name qmux-01. The comment on the advertised bare ALPNs drops the removed webtransport entry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 14 minutes. View limit details
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…ove the migration note to the PR body Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Replaces moq-dev/moq#6540 by Matt Sheppard, whose design and code this is. That branch lives on an organization-owned fork, which GitHub does not let maintainers push to, so this branch carries Matt's commit unchanged, a merge of
main, and one test fix. History is intact (no rebase), and Matt keeps authorship of the original commit.Changes since #6540
main; the only conflict was theMIGRATE.mdUnreleased list.@moq/qmuxonmain(moq-dev/moq#6528) now refuses an empty negotiated subprotocol instead of falling back to legacy framing. The new tests'WebSocketStreamstubs negotiated"", so they now negotiateqmux-01. The comment listing the bare ALPNs the WebSocket fallback advertises dropped the removedwebtransportentry.mainagain.@moq/clock'sinstall()now returns anInstalledhandle (moq-dev/moq#6510), so the new tests close it instead of calling it. Thejs/net/MIGRATE.mdentry moved into Impact below, since PRs no longer writeMIGRATE.md.Problem
connect()has no deadline, andReloadhands it only its teardown signal. So a dial the network never answers holds the reconnect loop until the browser or OS gives up.delay.timeoutis only checked after an attempt fails, so it doesn't help.This happens when a client reconnects through a layer-4 load balancer.
In Chrome 152 on macOS, against an address that never answers:
Promise.anywaits until both fail, so an attempt whose dials all land on the dead target waits 75 s. Where WebSocket is the only transport (Safari, Firefox before 153, Node, Bun, Deno), one dead dial is enough. A browser page in this situation took 80 s to reconnect.Approach
Four parts, all in
connect(), soReload, the pool's shared and private loops, GOAWAY migration and a one-shotconnect()all get them.ConnectProps.timeoutandConnectionProps.timeout(default 30 s;Milli(0)for none) cover the dial and the MoQ handshake.Error.ConnectTimeout, whichReloadretries like any other failure.connect.timeoutandError::ConnectTimeout(moq-dev/moq#2622). The pairtimeout/delay.timeoutmirrorsconnect.timeout/connect.backoff.timeout.allowPooling: false), so each dial is a new flow.no transport connected: …keeps one reason per transport. A browser words every handshake failure the same way.WebSocketStreamandWebSocket:WebSocket), a second WebSocket dialed 1.8 s in failed 75.0 s after the first was closed (at 5.8 s), not 75 s after its own dial. So it had been queued too.Consumers:
<moq-watch>and<moq-publish>use the shared pool, so they get all of this through the defaults. Thetimeoutoptions are the escape hatch described under Impact.Impact
ConnectProps.timeoutandConnectionProps.timeout.share: truerefusestimeout, as it doesdelay.Error.ConnectTimeout, withtimeoutin milliseconds.ConnectProps.timeout,ConnectionProps.timeout, and the exportedError.ConnectTimeout. The hedge and redial delays stay private constants.connect()and eachConnectionattempt reject withError.ConnectTimeoutafter 30 s. A deadline set throughsignalthat is longer than that no longer takes effect.connect(), and each attempt of aConnection, now rejects withError.ConnectTimeoutonce the dial and the MoQ handshake together take longer than 30 s, closing what it had opened. Before, an attempt nothing answered waited until the browser or the OS gave up, which for a WebSocket can be minutes, and a suppliedtransportwhose handshake was never answered waited forever. Passtimeoutto change the bound, ortimeout: Milli(0)for no deadline. A deadline set throughsignal, for exampleAbortSignal.timeout(60_000), now only takes effect when it is shorter thantimeout.timeout(negative, NaN, infinite) is aRangeError:connect()rejects, and aConnectionthrows when it is built.Alternatives
connect()calls insideReload: a near-tie wires both sessions into the origins and announces on both, an auth rejection gets lost in theAggregateError, and the WebSocket gains nothing because of the per-address limit.websocketWonis: it would halve the throttle cost in a long outage, but slow recovery when both of an attempt's dials happen to land on dead targets.Follow-ups
accept()has no SETUP deadline on the server side either. That's a different problem (what a silent peer can make a server hold); to be planned as a quest.Decisions (settled)
connect.timeout.ConnectPropsnext totimeout.Checks
just checkpasses, and thejs/netsuite passed 3 runs in a row (2732 tests).quest check --base origin/mainpasses. No wire, moq-ffi, or gateway change, so no interop run.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)