Fix builtin browser CDP startup crash in multi-threaded workers (gunicorn/uvicorn) - #2240
Open
chelsealong wants to merge 1 commit into
Open
Conversation
…nclecode#2238) ManagedBrowser.start() used preexec_fn=os.setpgrp to give the launched Chromium its own process group. preexec_fn runs Python between fork() and exec() in the child; in a multi-threaded parent (a gunicorn/uvicorn worker) the forked child only inherits the calling thread, so a lock held by another thread at fork time can leave the child deadlocked or crash before exec ever runs. Switch to start_new_session=True, which gets the same process-group isolation via setsid() natively, without running Python in the forked child.
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
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.
Fixes #2238
Root cause
ManagedBrowser.start()launches Chromium with:preexec_fnruns arbitrary Python code betweenfork()andexec()in thechild process. Python's own docs warn this is unsafe in a multi-threaded
process:
When
fork()happens inside a multi-threaded parent (a gunicornUvicornWorkerrunning the asyncio event loop plus worker threads), the child process only
inherits the thread that called
fork(). Any lock held by another thread atthat instant (CPython's allocator lock, import lock, etc.) is inherited
already-locked and never released in the child, since the threads that would
release it don't exist there. Executing
os.setpgrp— a Python-level call —in that child can therefore hit a held lock and crash before
execve()ever replaces the process image, which is exactly the
SIGSEGVreported inthe issue. It's also exactly why the same command line runs fine from a
shell or a standalone single-threaded script: those parents have no other
thread holding a lock at fork time.
This matches the issue's own report precisely: 100% failure when the parent
is the gunicorn worker (multi-threaded), 100% success from any other parent
(single-threaded shell / script), same args, same image, same container.
Fix
Replace
preexec_fn=os.setpgrpwithstart_new_session=True. This achievesthe same goal (put the browser in its own session/process group, so
os.killpg(os.getpgid(pid), SIGKILL)in_cleanupstill works unchanged)but the equivalent
setsid()call is made natively by thesubprocessmodule's C implementation rather than by running Python bytecode in the
forked child — so it doesn't touch the interpreter state that can be
mid-mutation in another thread at fork time. This is the same fix pattern
CPython itself recommends over
preexec_fnfor this exact scenario, andmirrors the Windows branch just above it, which already avoids running any
code in the child (it uses
creationflagsinstead).Testing
Added
tests/browser/test_managed_browser_start_new_session.py, which mockssubprocess.Popenand assertsManagedBrowser.start()calls it withstart_new_session=Trueand never withpreexec_fn.Confirmed the test fails without the fix:
And passes with the fix restored:
Also ran the broader browser test suite (with Playwright's Chromium
installed) to check for regressions:
(A handful of unrelated
tests/browser/files fail to even collect on aclean checkout — missing optional deps like
websockets/colorama, or acrawl4ai.browsermodule that doesn't exist in this version of the repo —pre-existing and unrelated to this change.)
Files changed
crawl4ai/browser_manager.py— swappreexec_fn=os.setpgrpforstart_new_session=TrueinManagedBrowser.start()'s Unix branch.tests/browser/test_managed_browser_start_new_session.py— new regressiontest.
AI assistance disclosure
This PR was prepared with the help of an AI coding agent (Claude), which
investigated the issue, identified the root cause, implemented the fix, and
wrote/verified the regression test. All changes were reviewed before
submission.