fix: cdp retry on disconnect and crash#357
Merged
shadowfax92 merged 3 commits intomainfrom Feb 23, 2026
Merged
Conversation
Contributor
Greptile SummaryThis PR adds robust retry logic for CDP connection failures and unexpected disconnections. The implementation properly handles connection state with flags to prevent concurrent operations, rejects pending requests when connection is lost, and implements a crash-recovery pattern that exits the process for external restart after retries are exhausted. Key improvements:
Review notes:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CDP Connection] -->|Close Event| B{Check Flags}
B -->|disconnecting=true| C[Ignore]
B -->|reconnecting=true| C
B -->|Both false| D[Set reconnecting=true]
D --> E[Reject Pending Requests]
E --> F[Start Reconnection Loop]
F --> G{Attempt < Max Retries?}
G -->|Yes| H[Wait Retry Delay]
H --> I[Attempt Connect]
I -->|Success| J[Log Success]
J --> K[Clear reconnecting flag]
I -->|Fail| G
G -->|No| L[Log Error]
L --> M[Exit Process]
N[Initial Connection] --> O{Attempt < Max Retries?}
O -->|Yes| P[Attempt Connect]
P -->|Success| Q[Return]
P -->|Fail| R[Wait Retry Delay]
R --> O
O -->|No| S[Throw Error]
Last reviewed commit: 5861e0a |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| logger.error( | ||
| 'CDP WebSocket closed unexpectedly, attempting reconnection...', | ||
| ) | ||
| this.reconnectOrCrash() |
Contributor
There was a problem hiding this comment.
reconnectOrCrash() is not awaited, allowing multiple concurrent reconnection attempts if close events fire rapidly
Suggested change
| this.reconnectOrCrash() | |
| await this.reconnectOrCrash() |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/browser/backends/cdp.ts
Line: 104
Comment:
`reconnectOrCrash()` is not awaited, allowing multiple concurrent reconnection attempts if close events fire rapidly
```suggestion
await this.reconnectOrCrash()
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
107
to
130
| private async reconnectOrCrash(): Promise<void> { | ||
| const maxRetries = TIMEOUTS.CDP_CONNECT_MAX_RETRIES | ||
| const retryDelay = TIMEOUTS.CDP_CONNECT_RETRY_DELAY | ||
|
|
||
| for (let attempt = 1; attempt <= maxRetries; attempt++) { | ||
| try { | ||
| logger.info(`CDP reconnection attempt ${attempt}/${maxRetries}...`) | ||
| await sleep(retryDelay) | ||
| await this.attemptConnect() | ||
| logger.info('CDP reconnected successfully') | ||
| return | ||
| } catch (error) { | ||
| const msg = error instanceof Error ? error.message : String(error) | ||
| logger.warn( | ||
| `CDP reconnection attempt ${attempt}/${maxRetries} failed: ${msg}`, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| this.ws.onmessage = (event) => { | ||
| this.handleMessage(event.data as string) | ||
| } | ||
| }) | ||
| logger.error( | ||
| `CDP reconnection failed after ${maxRetries} attempts, exiting for restart`, | ||
| ) | ||
| process.exit(EXIT_CODES.GENERAL_ERROR) | ||
| } |
Contributor
There was a problem hiding this comment.
pending requests are never rejected when reconnection starts, causing them to hang indefinitely if the connection drops mid-request
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/browser/backends/cdp.ts
Line: 107-130
Comment:
pending requests are never rejected when reconnection starts, causing them to hang indefinitely if the connection drops mid-request
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
98
to
105
| private handleUnexpectedClose(): void { | ||
| if (this.disconnecting) return | ||
|
|
||
| this.ws.onerror = (event) => { | ||
| reject(new Error(`CDP WebSocket error: ${event}`)) | ||
| } | ||
| logger.error( | ||
| 'CDP WebSocket closed unexpectedly, attempting reconnection...', | ||
| ) | ||
| this.reconnectOrCrash() | ||
| } |
Contributor
There was a problem hiding this comment.
add guard to prevent multiple concurrent reconnection attempts
Suggested change
| private handleUnexpectedClose(): void { | |
| if (this.disconnecting) return | |
| this.ws.onerror = (event) => { | |
| reject(new Error(`CDP WebSocket error: ${event}`)) | |
| } | |
| logger.error( | |
| 'CDP WebSocket closed unexpectedly, attempting reconnection...', | |
| ) | |
| this.reconnectOrCrash() | |
| } | |
| private reconnecting = false | |
| private handleUnexpectedClose(): void { | |
| if (this.disconnecting || this.reconnecting) return | |
| logger.error( | |
| 'CDP WebSocket closed unexpectedly, attempting reconnection...', | |
| ) | |
| this.reconnecting = true | |
| this.reconnectOrCrash().finally(() => { | |
| this.reconnecting = false | |
| }) | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/browser/backends/cdp.ts
Line: 98-105
Comment:
add guard to prevent multiple concurrent reconnection attempts
```suggestion
private reconnecting = false
private handleUnexpectedClose(): void {
if (this.disconnecting || this.reconnecting) return
logger.error(
'CDP WebSocket closed unexpectedly, attempting reconnection...',
)
this.reconnecting = true
this.reconnectOrCrash().finally(() => {
this.reconnecting = false
})
}
```
How can I resolve this? If you propose a fix, please make it concise.
Contributor
Author
|
@greptileai review |
| logger.error( | ||
| 'CDP WebSocket closed unexpectedly, attempting reconnection...', | ||
| ) | ||
| this.reconnectOrCrash() |
Contributor
There was a problem hiding this comment.
reconnectOrCrash() not awaited - if multiple close events fire, multiple concurrent reconnection attempts will run
Suggested change
| this.reconnectOrCrash() | |
| await this.reconnectOrCrash() |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/browser/backends/cdp.ts
Line: 105
Comment:
`reconnectOrCrash()` not awaited - if multiple close events fire, multiple concurrent reconnection attempts will run
```suggestion
await this.reconnectOrCrash()
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
108
to
131
| private async reconnectOrCrash(): Promise<void> { | ||
| const maxRetries = CDP_LIMITS.CONNECT_MAX_RETRIES | ||
| const retryDelay = TIMEOUTS.CDP_CONNECT_RETRY_DELAY | ||
|
|
||
| for (let attempt = 1; attempt <= maxRetries; attempt++) { | ||
| try { | ||
| logger.info(`CDP reconnection attempt ${attempt}/${maxRetries}...`) | ||
| await Bun.sleep(retryDelay) | ||
| await this.attemptConnect() | ||
| logger.info('CDP reconnected successfully') | ||
| return | ||
| } catch (error) { | ||
| const msg = error instanceof Error ? error.message : String(error) | ||
| logger.warn( | ||
| `CDP reconnection attempt ${attempt}/${maxRetries} failed: ${msg}`, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| this.ws.onmessage = (event) => { | ||
| this.handleMessage(event.data as string) | ||
| } | ||
| }) | ||
| logger.error( | ||
| `CDP reconnection failed after ${maxRetries} attempts, exiting for restart`, | ||
| ) | ||
| process.exit(EXIT_CODES.GENERAL_ERROR) | ||
| } |
Contributor
There was a problem hiding this comment.
No guard prevents concurrent reconnection attempts. Add a reconnecting flag similar to disconnecting:
private reconnecting = false
private async reconnectOrCrash(): Promise<void> {
if (this.reconnecting) return
this.reconnecting = true
try {
const maxRetries = CDP_LIMITS.CONNECT_MAX_RETRIES
const retryDelay = TIMEOUTS.CDP_CONNECT_RETRY_DELAY
for (let attempt = 1; attempt <= maxRetries; attempt++) {
// ... existing retry logic
}
logger.error(
`CDP reconnection failed after ${maxRetries} attempts, exiting for restart`,
)
process.exit(EXIT_CODES.GENERAL_ERROR)
} finally {
this.reconnecting = false
}
}Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/browser/backends/cdp.ts
Line: 108-131
Comment:
No guard prevents concurrent reconnection attempts. Add a `reconnecting` flag similar to `disconnecting`:
```typescript
private reconnecting = false
private async reconnectOrCrash(): Promise<void> {
if (this.reconnecting) return
this.reconnecting = true
try {
const maxRetries = CDP_LIMITS.CONNECT_MAX_RETRIES
const retryDelay = TIMEOUTS.CDP_CONNECT_RETRY_DELAY
for (let attempt = 1; attempt <= maxRetries; attempt++) {
// ... existing retry logic
}
logger.error(
`CDP reconnection failed after ${maxRetries} attempts, exiting for restart`,
)
process.exit(EXIT_CODES.GENERAL_ERROR)
} finally {
this.reconnecting = false
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
99
to
106
| private handleUnexpectedClose(): void { | ||
| if (this.disconnecting) return | ||
|
|
||
| this.ws.onerror = (event) => { | ||
| reject(new Error(`CDP WebSocket error: ${event}`)) | ||
| } | ||
| logger.error( | ||
| 'CDP WebSocket closed unexpectedly, attempting reconnection...', | ||
| ) | ||
| this.reconnectOrCrash() | ||
| } |
Contributor
There was a problem hiding this comment.
Pending requests never rejected when connection drops - they hang indefinitely. Reject all pending requests before reconnecting:
private handleUnexpectedClose(): void {
if (this.disconnecting) return
// Reject all pending requests
for (const [id, request] of this.pending.entries()) {
request.reject(new Error('CDP connection lost'))
}
this.pending.clear()
logger.error(
'CDP WebSocket closed unexpectedly, attempting reconnection...',
)
await this.reconnectOrCrash()
}Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/browser/backends/cdp.ts
Line: 99-106
Comment:
Pending requests never rejected when connection drops - they hang indefinitely. Reject all pending requests before reconnecting:
```typescript
private handleUnexpectedClose(): void {
if (this.disconnecting) return
// Reject all pending requests
for (const [id, request] of this.pending.entries()) {
request.reject(new Error('CDP connection lost'))
}
this.pending.clear()
logger.error(
'CDP WebSocket closed unexpectedly, attempting reconnection...',
)
await this.reconnectOrCrash()
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+108
to
111
| this.reconnecting = true | ||
| this.reconnectOrCrash().finally(() => { | ||
| this.reconnecting = false | ||
| }) |
Contributor
There was a problem hiding this comment.
reconnecting flag should be set inside the promise chain to prevent race condition. If reconnectOrCrash() Promise is created but .finally() runs synchronously before async work starts, the flag could be cleared prematurely.
Suggested change
| this.reconnecting = true | |
| this.reconnectOrCrash().finally(() => { | |
| this.reconnecting = false | |
| }) | |
| logger.error( | |
| 'CDP WebSocket closed unexpectedly, attempting reconnection...', | |
| ) | |
| void (async () => { | |
| this.reconnecting = true | |
| try { | |
| await this.reconnectOrCrash() | |
| } finally { | |
| this.reconnecting = false | |
| } | |
| })() |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/browser/backends/cdp.ts
Line: 108-111
Comment:
`reconnecting` flag should be set inside the promise chain to prevent race condition. If `reconnectOrCrash()` Promise is created but `.finally()` runs synchronously before async work starts, the flag could be cleared prematurely.
```suggestion
logger.error(
'CDP WebSocket closed unexpectedly, attempting reconnection...',
)
void (async () => {
this.reconnecting = true
try {
await this.reconnectOrCrash()
} finally {
this.reconnecting = false
}
})()
```
How can I resolve this? If you propose a fix, please make it concise.
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
1 participant
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.
No description provided.