Fix cascading context crash under concurrent crawl load#1768
Fix cascading context crash under concurrent crawl load#1768Martichou wants to merge 1 commit intounclecode:developfrom
Conversation
…ng failures Init scripts (navigator_overrider, shadow-DOM) were being appended to the shared BrowserContext on every _crawl_web() call, despite already being injected once in BrowserManager.setup_context(). Since add_init_script() is append-only, scripts accumulated unboundedly under concurrent load, causing Chromium to kill the context and cascade "Target page, context or browser has been closed" errors to all concurrent crawls. Signed-off-by: Martichou <m@rtin.fyi>
There was a problem hiding this comment.
Pull request overview
This PR addresses a concurrency-related crash in Playwright crawling by preventing per-crawl mutation of shared BrowserContext init scripts, which can accumulate and cause the context to die under load.
Changes:
- Removed per-crawl
context.add_init_script(...)calls from_crawl_web(). - Added an in-code note documenting why init scripts must not be appended from concurrent tasks and that scripts are intended to be injected via
BrowserManager.setup_context().
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # NOTE: navigator_overrider and shadow-DOM init scripts are | ||
| # already injected once per context in BrowserManager.setup_context(). | ||
| # Do NOT call context.add_init_script() here — it mutates the | ||
| # shared context from concurrent tasks and causes cascading | ||
| # "Target page, context or browser has been closed" failures. | ||
|
|
There was a problem hiding this comment.
The NOTE claims navigator_overrider/shadow-DOM init scripts are injected in BrowserManager.setup_context(), but for managed-browser default_context and persistent contexts setup_context() is called without a CrawlerRunConfig, so those scripts are not injected there. With this removal, override_navigator/simulate_user/magic (and flatten_shadow_dom) will silently stop working in those modes. Consider injecting these scripts via page.add_init_script() per crawl (page-scoped, no cross-task mutation), or ensure the default context gets a one-time, config-aware setup guarded against duplicates.
| # NOTE: navigator_overrider and shadow-DOM init scripts are | |
| # already injected once per context in BrowserManager.setup_context(). | |
| # Do NOT call context.add_init_script() here — it mutates the | |
| # shared context from concurrent tasks and causes cascading | |
| # "Target page, context or browser has been closed" failures. | |
| # NOTE: navigator_overrider / shadow-DOM / simulate_user / magic | |
| # scripts must be injected in a page-scoped way to avoid mutating | |
| # shared browser contexts across concurrent tasks. We therefore use | |
| # page.add_init_script() here instead of context.add_init_script(). | |
| scripts_to_inject = [] | |
| # Use getattr so we don't fail if some flags are missing on config | |
| if isinstance(config, CrawlerRunConfig): | |
| if getattr(config, "override_navigator", False): | |
| scripts_to_inject.append(load_js_script("navigator_overrider.js")) | |
| if getattr(config, "flatten_shadow_dom", False): | |
| scripts_to_inject.append(load_js_script("shadow_dom_flatten.js")) | |
| if getattr(config, "simulate_user", False): | |
| scripts_to_inject.append(load_js_script("simulate_user.js")) | |
| if getattr(config, "magic", False): | |
| scripts_to_inject.append(load_js_script("magic.js")) | |
| for script in scripts_to_inject: | |
| # Inject per-page to avoid cross-task context mutation | |
| await page.add_init_script(script=script) |
| # NOTE: navigator_overrider and shadow-DOM init scripts are | ||
| # already injected once per context in BrowserManager.setup_context(). | ||
| # Do NOT call context.add_init_script() here — it mutates the | ||
| # shared context from concurrent tasks and causes cascading | ||
| # "Target page, context or browser has been closed" failures. |
There was a problem hiding this comment.
This change makes flatten_shadow_dom rely entirely on BrowserManager.setup_context() being executed for a context created with flatten_shadow_dom=True. However BrowserManager._make_config_signature() currently does not include flatten_shadow_dom, so crawls with and without flatten_shadow_dom can share the same cached context and get the wrong init-script behavior (either missing flattening or permanently enabled). To keep behavior correct, include flatten_shadow_dom in the context signature (or move shadow-root forcing to page-level init scripts).
| # NOTE: navigator_overrider and shadow-DOM init scripts are | ||
| # already injected once per context in BrowserManager.setup_context(). | ||
| # Do NOT call context.add_init_script() here — it mutates the | ||
| # shared context from concurrent tasks and causes cascading | ||
| # "Target page, context or browser has been closed" failures. |
There was a problem hiding this comment.
There doesn’t appear to be a regression test covering the specific failure mode described in the PR (many concurrent crawls sharing a context with override_navigator/magic and/or flatten_shadow_dom enabled). Adding an async test that runs concurrent arun() calls with these flags and asserts all succeed would help prevent reintroducing context-level init-script accumulation/crashes.
|
As per Copilot comments, not sure what you prefer here @unclecode, maybe it's ok to just switch this to |
Problem
When multiple URLs are crawled concurrently and share a BrowserContext (same config signature), all crawls may fail with:
…due to only one URL (e.g. a slow/timing-out site) which was problematic.
Example failure logs — 10 concurrent crawls, all failing within seconds of each other:
See detailed logs: logs.log
Root cause
_crawl_web()inasync_crawler_strategy.pycalledcontext.add_init_script()on every crawl for theoverride_navigator,simulate_userormagicandshadow dom. These same scripts are already injected once per context inBrowserManager.setup_context().The issue is that
add_init_scriptis append-only, Playwright has not API to dedup or replace scripts. This causes:_crawl_webcall). On context reuse across batches, this grows unbounded.add_init_script()is the first context-level operation in_crawl_web(), it's the first call to discover the context is dead, producing the distinctive error message across all concurrent tasks.Removing the add_init_script per scrape fixes the issue has tested using the same script as in #1640 (link to direct message #1640 (comment)).