Repository navigation
fix: lower timeout for validating connection errors and guard against locking - #2148
Conversation
🦋 Changeset detectedLatest commit: ee591cf The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.
🔍 Devin Review: 1 flag
Not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
size-limit report 📦
|
xianshijing-lk
left a comment
xianshijing-lk
left a comment
There was a problem hiding this comment.
it looks good to me regarding the deadlock issue.
two questions
| @@ -434,15 +435,23 @@ export class SignalClient { | |||
| ): Promise<JoinResponse | ReconnectResponse | undefined> { | |||
| const unlock = await this.connectionLock.lock(); | |||
There was a problem hiding this comment.
just found out Connect() and close() are using different locks, why is it ?
There was a problem hiding this comment.
They guard different things, and one shared lock would deadlock. connectionLock makes connect attempts run one at a time. closingLock makes teardowns run one at a time, and it lets a new join()/reconnect() wait for a close that is still running (settleInFlightClose).
connect() calls close() while it holds connectionLock. Examples are the abort handler (abort signal or connect timeout) and the missing-first-message path. In the timeout case, the attempt is waiting on ws.opened. Only close() makes opened settle, because it closes the socket. If close() needed connectionLock, it would wait for the attempt to finish, and the attempt would wait for close(). A shared lock would also make room.disconnect() wait out a stalled connect instead of cancelling it.
There was a problem hiding this comment.
from what I am seeing, close take two parans, reason is the second one:
async close(updateState: boolean = true, reason = 'Close method called on signal client') {
// when the lifecycle is already shutting down (or another close owns it), only the transport
// teardown below still applies — it is idempotent, so callers can always await a close
const drivesLifecycle = updateState && this.sendLifecycleInput({ type: 'close', reason });
const unlock = await this.closingLock.lock();
try {
await this.teardownTransport(reason);
} finally {
if (drivesLifecycle) {
this.sendLifecycleInput({ type: 'closeComplete' });
}
unlock();
}
}
Or I miss something here ?
There was a problem hiding this comment.
Good catch, this is a bug. this.close(reason) passes the reason as updateState. The string is truthy, so the lifecycle still closes, but reason falls back to the default and the abort reason is lost. TypeScript does not catch it: getAbortReasonAsString returns signal.reason, which is any, so the inferred return type is any. The bug predates this PR. I will fix it here with this.close(true, reason), pass the reason in the two other close() calls in the abort handler too, and give the helper an explicit : string return type so the compiler flags calls like this.
close(reason) passed the reason as updateState, so the abort reason was dropped. getAbortReasonAsString inferred `any` from signal.reason, which hid the mistake from the compiler; it now declares a string return type. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| }); | ||
| } else { | ||
| this.close(); | ||
| this.close(true, reason); |
There was a problem hiding this comment.
🔴 Long abort reasons leave sockets open
When join receives an abort reason exceeding 123 UTF-8 bytes, close passes it to WebSocket.close, which throws. teardownTransport catches that error before clearing the socket, leaving the aborted transport open.
Learn more
An AbortSignal supplied to join can carry an arbitrary string reason. getAbortReasonAsString preserves that string, and teardownTransport passes it directly to the browser's WebSocket.close. WebSocket close reasons have a 123-byte UTF-8 limit. An oversized reason throws before teardownTransport clears ws, and the catch swallows the error without closing the socket. The same oversized reason can reach the send-leave success and failure branches.
Example: A caller aborts a pending join with 'x'.repeat(124). The join rejects as cancelled, but closing the socket throws, so the pending transport remains attached to the client.
Recommended fix: Keep the full abort reason for diagnostics and the rejected error, but pass a bounded WebSocket close reason to close in every abort branch. Truncate by UTF-8 byte length, or use a fixed short close reason.
Was this helpful? React with 👍 or 👎 to provide feedback.
WebSocket.close throws a SyntaxError for a reason over 123 UTF-8 bytes. The connect abort handler passes the caller's abort reason, so a long one made teardownTransport throw before it released the socket, leaving the aborted transport open. WebSocketStream.close truncates the reason on code point boundaries, and teardownTransport releases the transport before it closes it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Since an abandoned connect skips the validate request, the hanging- validate regression test from livekit#2148 no longer consumes its queued fetch mocks, which leaked into the handleConnectionError tests. Assert that no validation happens and reset the mocks afterwards. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
2 participants
SignalClient.connect()usesconnectionLockto run one connect attempt at a time. An attempt releases the lock when its executor finishes, not when the caller gets the result. When the websocket fails to open, the attempt asks the validate endpoint why, and it waits for thatfetchwith no deadline. A timed-out attempt already reports its failure to the engine, but it keeps the lock until the fetch settles. On a network that drops packets without a reset, the fetch can hang for minutes. A pooled connection that went stale during a short Wi-Fi outage is one example. The next reconnect attempt waits on the lock with no deadline. The engine still counts a reconnect as in progress, so it ignores other triggers and never asks the reconnect policy again. The room stays inSignalReconnectingand never emitsDisconnected.A second path leaks the lock for good.
connect()takes the lock and then builds the join request and the URLs outside thetry/finallythat releases it. A malformed URL makesnew URL()throw, so every laterjoin()orreconnect()hangs.This change gives the validate
fetcha 3 s abort signal. A timeout becomes the existingServerUnreachableerror, soRTCEngine.joinretries it like any other unreachable server. The setup code before the executor releases the lock if it throws, then re-throws the error.The failure case is a validate endpoint that answers after more than 3 s. The attempt cannot classify the websocket error, so a 401 or a missing room is reported as
ServerUnreachable. The engine retries instead of stopping. This affects only the path where the websocket fails on its own. When the connect timeout fires, the caller already has aTimeouterror and the validate result is discarded.Impact
A user reported a room stuck in
SignalReconnectingfor more than 7 minutes after a short Wi-Fi outage. The network came back after about 1 minute. The room never retried and never emittedDisconnected, and onlyroom.disconnect()ended it. With this change, a failed attempt holds the lock for at most 3 s after its websocket closes, and the next attempt then runs under the normal connect timeout. The new testdoes not let a timed-out attempt block the next onereproduces the report: onmainthe second attempt never opens a websocket. A second test covers the malformed-URL leak.