Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
123 changes: 103 additions & 20 deletions apps/server/src/browser/backends/cdp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ import {
type RawSend,
} from '@browseros/cdp-protocol/create-api'
import type { ProtocolApi } from '@browseros/cdp-protocol/protocol-api'
import { EXIT_CODES } from '@browseros/shared/constants/exit-codes'
import { CDP_LIMITS } from '@browseros/shared/constants/limits'
import { TIMEOUTS } from '@browseros/shared/constants/timeouts'
import { logger } from '../../lib/logger'
import type { CdpTarget, CdpBackend as ICdpBackend } from './types'

interface PendingRequest {
Expand All @@ -20,6 +24,8 @@ class CdpBackend implements ICdpBackend {
private messageId = 0
private pending = new Map<number, PendingRequest>()
private connected = false
private disconnecting = false
private reconnecting = false
private eventHandlers = new Map<string, ((params: unknown) => void)[]>()
private sessionCache = new Map<string, ProtocolApi>()

Expand All @@ -32,37 +38,114 @@ class CdpBackend implements ICdpBackend {
}

async connect(): Promise<void> {
const versionResponse = await fetch(
`http://localhost:${this.port}/json/version`,
)
const version = (await versionResponse.json()) as {
webSocketDebuggerUrl: string
const maxRetries = CDP_LIMITS.CONNECT_MAX_RETRIES
const retryDelay = TIMEOUTS.CDP_CONNECT_RETRY_DELAY

for (let attempt = 1; attempt <= maxRetries; attempt++) {
try {
await this.attemptConnect()
return
} catch (error) {
const msg = error instanceof Error ? error.message : String(error)
if (attempt < maxRetries) {
logger.warn(
`CDP connection attempt ${attempt}/${maxRetries} failed: ${msg}. Retrying in ${retryDelay}ms...`,
)
await Bun.sleep(retryDelay)
} else {
throw new Error(
`CDP connection failed after ${maxRetries} attempts: ${msg}`,
)
}
}
}
const wsUrl = version.webSocketDebuggerUrl
}

private attemptConnect(): Promise<void> {
return new Promise<void>((resolve, reject) => {
this.ws = new WebSocket(wsUrl)
fetch(`http://localhost:${this.port}/json/version`)
.then((res) => res.json())
.then((version) => {
const wsUrl = (version as { webSocketDebuggerUrl: string })
.webSocketDebuggerUrl
let opened = false
const ws = new WebSocket(wsUrl)

this.ws.onopen = () => {
this.connected = true
resolve()
}
ws.onopen = () => {
opened = true
this.ws = ws
this.connected = true
this.disconnecting = false
resolve()
}

this.ws.onerror = (event) => {
reject(new Error(`CDP WebSocket error: ${event}`))
}
ws.onerror = (event) => {
if (!opened) reject(new Error(`CDP WebSocket error: ${event}`))
}

this.ws.onclose = () => {
this.connected = false
}
ws.onclose = () => {
this.connected = false
this.ws = null
if (opened) this.handleUnexpectedClose()
}

this.ws.onmessage = (event) => {
this.handleMessage(event.data as string)
}
ws.onmessage = (event) => {
this.handleMessage(event.data as string)
}
})
.catch(reject)
})
}

private handleUnexpectedClose(): void {
if (this.disconnecting || this.reconnecting) return

this.rejectPendingRequests()

logger.error(
'CDP WebSocket closed unexpectedly, attempting reconnection...',
)
this.reconnecting = true
this.reconnectOrCrash().finally(() => {
this.reconnecting = false
})
Comment on lines +108 to 111
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

private rejectPendingRequests(): void {
const error = new Error('CDP connection lost')
for (const request of this.pending.values()) {
request.reject(error)
}
this.pending.clear()
}
Comment on lines 100 to 120
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines 100 to 120
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


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}`,
)
}
}

logger.error(
`CDP reconnection failed after ${maxRetries} attempts, exiting for restart`,
)
process.exit(EXIT_CODES.GENERAL_ERROR)
}
Comment on lines 122 to 145
Copy link
Contributor

Choose a reason for hiding this comment

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

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 122 to 145
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


async disconnect(): Promise<void> {
this.disconnecting = true
if (this.ws) {
this.ws.close()
this.ws = null
Expand Down
4 changes: 4 additions & 0 deletions packages/shared/src/constants/limits.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ export const PAGINATION = {
DEFAULT_PAGE_SIZE: 20,
} as const

export const CDP_LIMITS = {
CONNECT_MAX_RETRIES: 3,
} as const

export const CONTENT_LIMITS = {
BODY_CONTEXT_SIZE: 10_000,
MAX_QUEUE_SIZE: 1_000,
Expand Down
1 change: 1 addition & 0 deletions packages/shared/src/constants/timeouts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export const TIMEOUTS = {

// CDP connection
CDP_CONNECT: 10_000,
CDP_CONNECT_RETRY_DELAY: 1_000,

// External API calls
KLAVIS_FETCH: 30_000,
Expand Down
Loading