Make the browser pool un-pinnable by hung crawls - #2214
Open
SohamKukreti wants to merge 1 commit into
Open
Conversation
Lock-free release, janitor stale-lease backstop (stale_lease_s, auto 6h floor), closes off-LOCK in background tasks, wall_clock_s=1800 default; docs and tests.
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.
Summary
Fixes #2202
A crawl that hangs inside the browser (e.g. a page whose JS thread never yields — see #2205)
used to pin its pooled browser forever: the busy counter never decremented, the janitor
refused to touch a "busy" browser unconditionally, and the per-crawl deadline shipped
disabled. Weeks of light traffic accumulated dozens of chromium processes and ~1 GB RSS
(reporter's container: 46 chrome processes, 35 never-closed pages).
This PR makes the server survive hung or cancelled crawls without leaking browsers:
release_crawlerno longer takes the pool LOCK and has noawait points, so a request cancelled mid-
finally(client disconnect, deadline) can neverskip the busy-counter decrement.
longer than
crawler.pool.stale_lease_s(default0= automax(2 × wall_clock_s, 21600);6 h floor because streaming has no deadline) is treated as pinned by a leaked request and
force-closed with an ERROR log naming the signature.
close_all()route browser closes througha fire-and-forget background task (60 s cap, logged on timeout) instead of awaiting
close()while holding the pool LOCK — a wedged browser previously frozeget_crawler()and all future janitor passes for the whole server.
close_all()drains pending closes(65 s cap) so shutdown is clean.
limits.wall_clock_sis now1800(was0). Anon-streaming crawl exceeding it returns 504 and releases its browser.
0remains thedocumented opt-out; the backstop above covers that case.
Out of scope (recorded for follow-up): fixing why crawls hang (#2205), the permanent
browser fingerprint (#2204), deadlines for the streaming path and the other pooled
endpoints, and
monitor_routes.py's closes-under-LOCK.List of files changed and why
deploy/docker/crawler_pool.py— the fix: lock-freerelease_crawler, stale-leasebackstop in both janitor sweeps,
_close_in_backgroundhelper,close_alldrain,ceiling config with safe fallback (bad YAML values, including
true, fall back to auto).deploy/docker/config.yml—wall_clock_s: 1800default; newpool.stale_lease_sknob.deploy/docker/MIGRATION.md— documents the new 504 default and the janitor force-closebehavior, including the "raise
stale_lease_sif your crawls legitimately run > 6 h" note.deploy/docker/ARCHITECTURE.md— janitor pseudo-code and pool config reference updated tomatch the new design.
deploy/docker/utils.py—dns.resolverimport deferred intoverify_email_domain()sothe module imports without dnspython (needed for the unit tests; dnspython ships in the
image, so no runtime change).
tests/docker/test_pool_release.py— rewritten to exercise the realcrawler_poolmoduleinstead of a standalone copy of the logic; covers release/cancellation safety, both sweep
paths, the backstop, the ceiling config resolution, and shutdown draining.
How Has This Been Tested?
python -m pytest tests/docker/test_pool_release.py— release decrements without the LOCKand survives cancellation; janitor force-closes busy-past-ceiling browsers (cold and hot),
skips genuinely busy ones, and is not blocked by a
close()that hangs;close_allwaitsfor pending background closes and never holds the LOCK while one hangs; ceiling resolution
rejects nonsense values (
-1,"30m",true,null).on the unfixed image — 14/14 pass. Hung crawl 504s within deadline+10 s and the janitor
reaps the browser (renderers back to baseline); 20 client cancellations and 40 concurrent
hung crawls never wedge the server; 60 healthy crawls leak nothing and trigger zero
force-closes; a genuinely busy browser is never touched; with
wall_clock_s: 0thebackstop force-closes the pinned browser, kills the wedged renderer, and the server keeps
serving; container shutdown exits 0 with zero destroyed-task warnings. The unfixed image
reproduces the original bug (pinned browser never reclaimed).
live pooled browser via the background path) and stop after a background reap.
Checklist: