fix: handling unexpected browser context closed error #498
Conversation
WalkthroughThe update modifies the Changes
Sequence Diagram(s)sequenceDiagram
participant RB as RemoteBrowser
participant B as Browser
participant C as Context
participant L as Logger
RB->>B: Attempt to launch browser (Attempt 1)
alt Launch successful
RB->>C: Initiate context creation (with 15s timeout)
alt Context creation successful
RB->>L: Log success and return browser/context
else Timeout/Error
RB->>L: Log context error and close browser
Note over RB,B: Retry if attempt count < 3
end
else Launch failed
RB->>L: Log launch failure and wait briefly
Note over RB,B: Retry if attempt count < 3
end
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
server/src/browser-management/classes/RemoteBrowser.ts (3)
248-251: Consider adding an exponential backoff to retry mechanism.
This would help spread out retry attempts, especially in scenarios where the environment might temporarily be under high load. Currently, the short, fixed 1-second delay might be insufficient or overly aggressive in some edge cases.- await new Promise(resolve => setTimeout(resolve, 1000)); + const backoffDelay = Math.pow(2, retryCount) * 1000; // exponential backoff + await new Promise(resolve => setTimeout(resolve, backoffDelay));
303-321: Make the 15-second timeout configurable.
A hardcoded 15s timeout may cause issues in slower network or resource-constrained environments. By allowing a configurable timeout, you can better accommodate various runtime conditions.
322-345: Review the WebDriver patch script for ongoing maintenance.
Embedding a multi-line script is effective but can be brittle if browser APIs change. Consider isolating this logic in a separate function or file, with accompanying tests or documentation, to simplify maintenance and future updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/browser-management/classes/RemoteBrowser.ts(1 hunks)
🔇 Additional comments (1)
server/src/browser-management/classes/RemoteBrowser.ts (1)
252-272: Ensure concurrency safety for browser initialization.
Ifinitializecan be called while another initialization is in progress, there could be concurrency issues (e.g., multiple browsers launching simultaneously). Consider adding a lock or a guard to prevent concurrentinitializecalls on the same instance.
| retryCount++; | ||
| logger.log('error', `Browser initialization failed (attempt ${retryCount}/${MAX_RETRIES}): ${error.message}`); | ||
|
|
||
| if (this.browser) { | ||
| try { | ||
| await this.browser.close(); | ||
| } catch (closeError) { | ||
| logger.log('warn', `Failed to close browser during cleanup: ${closeError}`); | ||
| } | ||
| this.browser = null; | ||
| } | ||
|
|
||
| if (retryCount >= MAX_RETRIES) { | ||
| throw new Error(`Failed to initialize browser after ${MAX_RETRIES} attempts: ${error.message}`); | ||
| } | ||
|
|
||
| await new Promise(resolve => setTimeout(resolve, 1000)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve cleanup logic in the catch block.
When cleaning up after a failed attempt, ensure that partial initializations (e.g., a context that started to initialize) are also explicitly closed if needed. Currently, only the browser is gracefully closed. If a browser context was partially created and left behind, it might lead to resource leaks.
amhsirak
added
Type: Bug
Closes: #492
Summary by CodeRabbit