Handle Puppeteer browser disconnects and relaunch - #1137
Conversation
Add cleanup logic to safely close and reset the browser instance, then automatically relaunch when Puppeteer emits `disconnected` or when the current browser is no longer connected.
|
Warning Review limit reached
Next review available in: 51 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesBrowser recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant BrowserToPdfRenderer
participant Browser
participant Puppeteer
Browser->>BrowserToPdfRenderer: report disconnection
BrowserToPdfRenderer->>Browser: clean up disconnected browser
BrowserToPdfRenderer->>Puppeteer: launch replacement browser
Puppeteer-->>BrowserToPdfRenderer: return replacement browser
BrowserToPdfRenderer-->>BrowserToPdfRenderer: share recovery promise
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/binary/puppeteer/src/lib/render-to.ts`:
- Around line 33-40: The cleanupHandler registered in the render-to browser
lifecycle must not relaunch after an intentional shutdown. Update the close()
flow and cleanupHandler around this.launchedBrowser.on('disconnected') to track
intentional closing or disable the handler before this.launchedBrowser.close(),
while preserving recovery for unexpected disconnections; add a test verifying
close() does not invoke puppeteer.launch() again.
- Around line 33-40: Update the browser recovery logic around the cleanupHandler
and launchedBrowser lifecycle so shared renderers register only one disconnect
handler per browser, serialize concurrent recovery through a single in-flight
promise, and avoid duplicate cleanup/launch operations. Make cleanup
instance-safe by clearing launchedBrowser only when it still references the
browser being cleaned, while preserving browser() recovery behavior for
disconnected instances.
- Around line 33-36: Update the asynchronous cleanupHandler disconnect listener
to catch and handle rejections from cleanup or browser relaunch, ensuring
failures are not left as ignored Promise rejections. Keep the existing cleanup
and this.browser({ timeout }) recovery flow, but route errors through the
appropriate existing error-handling mechanism inside the callback.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f5043c50-51da-41d0-aae2-0be584c7e37e
📒 Files selected for processing (1)
libs/binary/puppeteer/src/lib/render-to.ts
Add a regression test to ensure `close()` does not trigger a new Puppeteer launch after shutdown. Track an `isClosing` state and bypass disconnect cleanup relaunch logic while closing, delegating `close()` to `cleanup()` safely.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
libs/binary/puppeteer/src/lib/render-to.spec.ts (1)
45-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInvoke the registered disconnect handler in this test.
The test records
disconnectedHandlers, but it never calls a handler. Lines 47-51 only test the synchronousactiveBrowser.connectedfallback inbrowser().Invoke
disconnectedHandlers[0]()and await recovery completion. This validates the event-driven recovery path and preserves coverage for the rejection handler atrender-to.tsLines 57-61.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/binary/puppeteer/src/lib/render-to.spec.ts` around lines 45 - 60, Update the recovery test around the registered disconnectedHandlers to invoke disconnectedHandlers[0]() and await its completion before asserting recovered browsers. Preserve the existing activeBrowser.connected fallback assertions while also exercising the event-driven recovery and its rejection handling in renderer.browser().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/binary/puppeteer/src/lib/render-to.ts`:
- Around line 14-27: Update recoverBrowser and the browser launch path so
recovery does not reuse this.recoveringBrowser when browser() is called from the
current recovery operation. Use a non-reentrant launch path or track the browser
being recovered, ensuring an immediately disconnected replacement starts a new
recovery and returns a connected browser rather than creating a promise cycle.
- Around line 79-85: Update close() and the recovery flow around
recoveringBrowser so shutdown awaits any in-flight recovery before resolving.
Recheck isClosing after recovery cleanup and immediately before relaunching the
browser, preventing a recovery that was already in progress from launching after
close() completes. Add an interleaving test that pauses recovery during cleanup,
starts close(), and verifies no browser relaunch occurs after shutdown.
---
Nitpick comments:
In `@libs/binary/puppeteer/src/lib/render-to.spec.ts`:
- Around line 45-60: Update the recovery test around the registered
disconnectedHandlers to invoke disconnectedHandlers[0]() and await its
completion before asserting recovered browsers. Preserve the existing
activeBrowser.connected fallback assertions while also exercising the
event-driven recovery and its rejection handling in renderer.browser().
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e176ae4-ae47-4e29-b1a0-53cee61969bf
📒 Files selected for processing (2)
libs/binary/puppeteer/src/lib/render-to.spec.tslibs/binary/puppeteer/src/lib/render-to.ts
Add a regression test for close interleaving with recovery cleanup and make shutdown wait for any in-flight recovery before and after cleanup to avoid launching a new browser while closing.
Track the disconnected source browser for in-flight recovery so only matching recoveries are reused. Start a fresh recovery when a replacement browser is already disconnected, and add a regression test for this case.
1 participant
Summary by CodeRabbit