Skip to content

Never give up in socket transport #7616

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 3, 2025
Merged

Never give up in socket transport #7616

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 4 additions & 19 deletions packages/cloud/src/bridge/SocketTransport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export class SocketTransport {
private hasConnectedOnce: boolean = false

private readonly retryConfig: RetryConfig = {
maxInitialAttempts: 10,
maxInitialAttempts: Infinity,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is setting this to Infinity intentional? The while loop in connectWithRetry() will never exit naturally now, which could lead to resource exhaustion if the connection continuously fails. Consider:

  1. Making this configurable through the constructor's retryConfig parameter
  2. Adding a mechanism to cancel retry attempts (e.g., an abort signal)
  3. Implementing exponential backoff with a maximum retry duration instead of infinite attempts

initialDelay: 1_000,
maxDelay: 15_000,
backoffMultiplier: 2,
Expand Down Expand Up @@ -67,12 +67,8 @@ export class SocketTransport {
await this.connectWithRetry()
} catch (error) {
console.error(
`[SocketTransport] Initial connection attempts failed: ${error instanceof Error ? error.message : String(error)}`,
`[SocketTransport] Unexpected error in connection loop: ${error instanceof Error ? error.message : String(error)}`,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error message might be misleading now. With infinite retries, errors in the connection loop aren't necessarily "unexpected" anymore. Could we update this to be more descriptive, like "Error in connection retry loop" or "Connection retry loop terminated unexpectedly"?

)

// If we've never connected successfully, we've exhausted our retry attempts
// The user will need to manually retry or fix the issue
this.connectionState = ConnectionState.FAILED
}
}

Expand All @@ -83,9 +79,7 @@ export class SocketTransport {
try {
this.connectionState = this.retryAttempt === 0 ? ConnectionState.CONNECTING : ConnectionState.RETRYING

console.log(
`[SocketTransport] Connection attempt ${this.retryAttempt + 1} / ${this.retryConfig.maxInitialAttempts}`,
)
console.log(`[SocketTransport] Connection attempt ${this.retryAttempt + 1}`)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With maxInitialAttempts now set to Infinity, this while condition will always be true. Should we add an additional exit condition here, such as checking for a cancellation signal or a maximum retry duration?

Also, the removed code that threw an error after max attempts provided a clear exit condition. Without it, the only way to exit this loop is through a successful connection or an unexpected error.


await this.connectSocket()

Expand All @@ -111,12 +105,6 @@ export class SocketTransport {
this.socket = null
}

if (this.retryAttempt >= this.retryConfig.maxInitialAttempts) {
this.connectionState = ConnectionState.FAILED

throw new Error(`Failed to connect after ${this.retryConfig.maxInitialAttempts} attempts`)
}

console.log(`[SocketTransport] Waiting ${delay}ms before retry...`)

await this.delay(delay)
Expand Down Expand Up @@ -204,10 +192,7 @@ export class SocketTransport {
this.socket.on("reconnect_failed", () => {
console.error(`[SocketTransport] Socket.IO reconnection failed after all attempts`)

this.connectionState = ConnectionState.FAILED

// Socket.IO has exhausted its reconnection attempts
// The connection is now permanently failed until manual intervention
this.connectionState = ConnectionState.RETRYING
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is setting the state to RETRYING here intentional? Socket.IO has already exhausted its reconnection attempts at this point. This might confuse consumers checking the connection state, as it implies retrying is still happening when it's not. Should this remain as FAILED or perhaps a new state like AWAITING_MANUAL_RECONNECT?

})

this.socket.on("error", (error) => {
Expand Down
Loading