Skip to content

fix: unref BrowserController.close() fallback timer to avoid keeping Node alive longer than needed#3671

Merged
barjin merged 2 commits into
apify:masterfrom
simshaun:browser-pool-close-unref
May 26, 2026
Merged

fix: unref BrowserController.close() fallback timer to avoid keeping Node alive longer than needed#3671
barjin merged 2 commits into
apify:masterfrom
simshaun:browser-pool-close-unref

Conversation

@simshaun
Copy link
Copy Markdown
Contributor

Summary

This PR fixes an event-loop retention issue in browser-pool where BrowserController.close() schedules a 5 second fallback kill timer that can keep Node alive after crawl work has already finished.

The timer is created and left referenced. This change keeps the fallback behavior intact but calls unref() on that timer so it does not block process exit.

Why this is needed

In current behavior, a minimal crawl can complete and all user work can be done, but process exit is delayed due to the fallback timer.

Minimal reproduction

Run any short crawler that exits immediately after crawler.run() and cleanup. Observe that process exit occurs ~5 seconds later than expected.

@simshaun simshaun changed the title [browser-pool] unref BrowserController.close() fallback timer to avoid keeping Node alive longer than needed fix: unref BrowserController.close() fallback timer to avoid keeping Node alive longer than needed May 22, 2026
Copy link
Copy Markdown
Member

@barjin barjin left a comment

Choose a reason for hiding this comment

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

Thank you for spotting this @simshaun! The fix looks reasonable to me 👍

I'm not really convinced about the test, as it's imo checking this specific implementation. Perhaps we don't need the test for this?

@janbuchar
Copy link
Copy Markdown
Contributor

Thank you for spotting this @simshaun! The fix looks reasonable to me 👍

I'm not really convinced about the test, as it's imo checking this specific implementation. Perhaps we don't need the test for this?

Agree on this one, please get rid of the test

@barjin barjin merged commit eef94d4 into apify:master May 26, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants