fix(docker): recycle pooled browser contexts by pages served (#2231) - #2232
fix(docker): recycle pooled browser contexts by pages served (#2231)#2232ntohidi wants to merge 2 commits into
Conversation
Pooled browsers get slower with sustained use and the janitor never recycles them, because it only closes browsers that have been *idle* past a TTL — and a server under continuous load never has an idle one. Measured where the slowdown actually lives: after 1000 real page loads on one pooled browser, a fresh context inside the SAME chromium process navigated as fast as a brand-new process (7.1ms vs 7.0ms), while the worn context took 32.0ms. Wiping that context's cookies/localStorage/ service workers in place recovered half of it (16.3ms). A control run with pages that leave no state behind stayed flat over 2000 pages. So the rot is context state, not the browser process, and the mechanism to bound it already exists in browser_manager (version-based recycling, which replaces the context under load without waiting for a quiet moment). It was simply never enabled for the Docker server. Enabling it is one config line — no second lifetime policy in the pool's janitor, which would put the same rule in two places. Measured effect at max_pages_before_recycle=200: penalty drops from ~30ms to ~10-14ms. It bounds the damage to one recycle window rather than removing it, so the threshold is the knob. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019EQGr8ey8riKFr4inp9B8f
…kwargs The previous commit put max_pages_before_recycle in config.yml's crawler.browser.kwargs, which does not reach the endpoints in the report: /crawl and /crawl/stream build their BrowserConfig from the request body (api.py:687, api.py:903) and nothing merges the server's browser kwargs into it. Only the handful of endpoints that call get_default_browser_config() would have picked it up. Move the setting to crawler.pool (it is a pool policy, next to max_pages and idle_ttl_sec) and apply it in get_crawler()/init_permanent(), which every endpoint goes through. Pooling is what makes a browser long-lived, so the pool is the right owner of its recycling policy. Applied before _sig() so all requests still share one signature and pooling is unchanged; an explicit per-request value wins. Tests cover the call site too, not only the helper — dropping _apply_pool_defaults() from get_crawler() now fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019EQGr8ey8riKFr4inp9B8f
|
Thanks for digging into this — the context-vs-process split is a much better answer than anything I got to, and the fresh-context-in-the-same-process measurement is the part that settles it. On the gap you flagged between your ~25 ms and my ~5 s: I think that is scale in the same mechanism rather than two different effects. My workload crawls several hundred distinct origins through one pooled context; you wore out one origin. Cookie jar entries, localStorage, service worker registrations and CacheStorage all accumulate per origin inside that single context, so the number of origins is a multiplier on exactly the state you identified. It also fits the origin-independence I saw. My control was a fresh browser in the same container hitting example.com and iana.org while the worn pooled one was slow to both. If the cost lived in the visited origin's own state, the worn browser would have been fast on origins it had never touched, and it was not. A context-wide cost that every navigation pays is consistent with both sets of numbers. Offered as an explanation that fits the data, not something I have proven. Two review notes. The new test file errors at collection when the deps are absent, rather than skipping.
On confirmation at my scale: yes, I will run it and post numbers back here. The allowlist point above makes that cheaper than I expected, since I can set the field per request against the pinned image I already run and keep the recycling behaviour as the only thing under test. I will report whatever I get, including if it fails to reproduce your improvement. |
|
Ran the knob against a local rig. Three results, one of them a null. 1. The field applies from an untrusted request body on stock 0.9.2. Sending
and 15 more across a 3000-page run. That is worth a line in the PR: operators can adopt this today without waiting for a release, and it makes the knob A/B-able against an image they already run. I checked the log rather than the status code, since an unrecognised key is dropped silently with a 200. 2. Recycling is free. 3000 pages per arm, fresh container per arm,
15 context replacements cost nothing measurable — the recycling arm was, if anything, marginally faster. That seems like the result you would want in hand before defaulting it on, and it supports doing so. 3. I did not reproduce the decay, and my rig cannot refute your 25 ms. 3000 page loads across 99 distinct origins, each setting 5 cookies, ~20 KB of localStorage and an IndexedDB store, left the probe flat: +1.9 ms of drift. Two reasons not to read that as a negative result:
So the honest reading is that my synthetic rig failed to reproduce the mechanism, not that the mechanism is absent. The real-workload confirmation is still what I owe you, and I have not run it: the deployment that shows the effect is in production and I am not going to experiment on it. I will get it onto a staging copy and post numbers. If it is useful sooner, I can rebuild the local rig over HTTPS so service workers actually register, which would test your third ingredient directly — say the word and I will run that next. |
Fixes #2231.
The problem
A pooled browser in the Docker server gets slower with sustained use, and
janitor()never recycles it — it only closes browsers that have been idle past a TTL, and a server under continuous load never has an idle one. The workload that causes the degradation is exactly the workload that prevents recycling.Where the slowdown actually lives
Measured, because the reporter could not reproduce it in-process and neither could I at first. After 1000 real page loads on one pooled browser, timing the same navigation three ways:
A fresh context inside the same process is as fast as a fresh process, so the browser process is not the problem — the context is. A control run with pages that leave no state behind stayed flat over 2000 pages; the same pages with cookies + localStorage + a service worker degraded 4-5x from ~500 pages on.
That matches the code:
browser_manager.pykeeps one context per config for the life of the pooled browser, makes a new page per crawl, and never clears that context's cookies or storage. It also explains why the effect needs real pages —raw:pages leave no state.The change
browser_manageralready has version-based recycling that replaces the context under load without waiting for a quiet moment (no idleness required, old contexts drain naturally). It was simply never enabled for the Docker server. This turns it on:config.yml:crawler.pool.max_pages_before_recycle: 200, next to the other pool policy (max_pages,idle_ttl_sec).0disables.crawler_pool.py: applied inget_crawler()/init_permanent().It is applied in the pool rather than in
crawler.browser.kwargsbecause/crawland/crawl/streambuild theirBrowserConfigfrom the request body — the server's browser kwargs never reach them. Every endpoint goes throughget_crawler(), and pooling is what makes a browser long-lived, so the pool is the right owner of the policy.Applied before
_sig(), so all requests still share one signature and pooling is unchanged. An explicit per-request value wins.No new lifetime logic in
janitor()— that would put the same rule in two places.Effect
It bounds the rot to one recycle window rather than removing it. The threshold is the knob.
Caveats, stated plainly
/healthsuggestion in the issue is not addressed here; it is a separate concern.Tests
tests/docker/test_pool_recycle_config.py— covers the config value, the pool applying it to a request-supplied config, an explicit caller value winning, signature stability, and theget_crawler()call site. Removing the config line fails 2 tests; removing the call site fails 1.🤖 Generated with Claude Code
https://claude.ai/code/session_019EQGr8ey8riKFr4inp9B8f