Skip to content

Fix builtin browser CDP startup crash in multi-threaded workers (gunicorn/uvicorn) - #2240

Open
chelsealong wants to merge 1 commit into
unclecode:mainfrom
chelsealong:fix/builtin-browser-preexec-fn-segfault
Open

Fix builtin browser CDP startup crash in multi-threaded workers (gunicorn/uvicorn)#2240
chelsealong wants to merge 1 commit into
unclecode:mainfrom
chelsealong:fix/builtin-browser-preexec-fn-segfault

Conversation

@chelsealong

Copy link
Copy Markdown

Fixes #2238

Root cause

ManagedBrowser.start() launches Chromium with:

subprocess.Popen(args, stdout=PIPE, stderr=PIPE, preexec_fn=os.setpgrp)

preexec_fn runs arbitrary Python code between fork() and exec() in the
child process. Python's own docs warn this is unsafe in a multi-threaded
process:

The preexec_fn parameter is NOT SAFE to use in the presence of threads
in your application. The child process could deadlock before exec is
called. If you must use it, keep it trivial! Minimize the number of
libraries you call into.

When fork() happens inside a multi-threaded parent (a gunicorn UvicornWorker
running the asyncio event loop plus worker threads), the child process only
inherits the thread that called fork(). Any lock held by another thread at
that 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 SIGSEGV reported in
the 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.setpgrp with start_new_session=True. This achieves
the same goal (put the browser in its own session/process group, so
os.killpg(os.getpgid(pid), SIGKILL) in _cleanup still works unchanged)
but the equivalent setsid() call is made natively by the subprocess
module'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_fn for this exact scenario, and
mirrors the Windows branch just above it, which already avoids running any
code in the child (it uses creationflags instead).

Testing

Added tests/browser/test_managed_browser_start_new_session.py, which mocks
subprocess.Popen and asserts ManagedBrowser.start() calls it with
start_new_session=True and never with preexec_fn.

Confirmed the test fails without the fix:

$ git checkout HEAD~1 -- crawl4ai/browser_manager.py
$ python -m pytest tests/browser/test_managed_browser_start_new_session.py -v
...
FAILED tests/browser/test_managed_browser_start_new_session.py::test_start_uses_start_new_session_not_preexec_fn
AssertionError: assert None is True
 +  where None = <built-in method get of dict object>('start_new_session')
 +    where <built-in method get of dict object> = {'stdout': -1, 'stderr': -1, 'preexec_fn': <built-in function setpgrp>}.get

And passes with the fix restored:

$ git checkout HEAD -- crawl4ai/browser_manager.py
$ python -m pytest tests/browser/test_managed_browser_start_new_session.py -v
...
tests/browser/test_managed_browser_start_new_session.py::test_start_uses_start_new_session_not_preexec_fn PASSED
1 passed in 0.83s

Also ran the broader browser test suite (with Playwright's Chromium
installed) to check for regressions:

$ python -m pytest tests/browser/test_context_leak_fix.py tests/browser/test_browser_manager_close.py \
    tests/browser/test_managed_browser_start_new_session.py tests/browser/test_profile_shrink.py \
    tests/browser/test_resource_filtering.py -v
...
93 passed in 19.25s

(A handful of unrelated tests/browser/ files fail to even collect on a
clean checkout — missing optional deps like websockets/colorama, or a
crawl4ai.browser module that doesn't exist in this version of the repo —
pre-existing and unrelated to this change.)

Files changed

  • crawl4ai/browser_manager.py — swap preexec_fn=os.setpgrp for
    start_new_session=True in ManagedBrowser.start()'s Unix branch.
  • tests/browser/test_managed_browser_start_new_session.py — new regression
    test.

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.

…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.
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.

[Bug]: browser_mode=builtin fails with "CDP endpoint not ready" — Chromium segfaults (SIGSEGV) when spawned from gunicorn worker (v0.9.3)

1 participant