fix: catch unhandled ws signal errors - #2065
Conversation
🦋 Changeset detectedLatest commit: b14d3ba 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 |
size-limit report 📦
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ent-sdk-js into lukas/catch-unhandled-ws-rej
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| } catch (e) { | ||
| this.log.error(`error reading from signal stream`, { error: e }); | ||
| await this.close(false, 'error in reading loop'); | ||
| break; | ||
| } |
There was a problem hiding this comment.
🟡 Stale read loop can close the live connection
On any read error the loop calls close(false, ...), which tears down whatever this.ws currently is, with no check that the failing reader still owns it. When an earlier attempt's socket closes uncleanly after a newer attempt has installed a new this.ws, the stale loop closes the healthy new connection. handleOnClose guards this cross-attempt case with attemptId; the read loop does not.
Prompt for agents
The signal read loop in startReadingLoop (src/api/SignalClient.ts) now calls this.close(false, 'error in reading loop') on any read error. close() -> teardownTransport() unconditionally closes the current this.ws, but the read loop is not scoped to the connection attempt it belongs to. handleSignalConnected (around line 1207) starts the loop with connection.readable.getReader() but does not pass attemptId, whereas handleOnClose guards transport teardown with attemptId to avoid tearing down a transport that a newer attempt has since installed. Because teardownTransport can return before the old socket is actually CLOSED (Promise.race with MAX_WS_CLOSE_TIME around line 729-735), an old reader can reject after this.ws has been replaced, and the old loop's close(false) would then tear down the new, healthy connection. Consider making the read loop attempt-aware (pass attemptId into startReadingLoop and guard the close call with it, similar to handleOnClose), or route the read-error teardown through handleOnClose so the existing attemptId guard applies.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
closes #2062