Skip to content

Make the browser pool un-pinnable by hung crawls - #2214

Open
SohamKukreti wants to merge 1 commit into
developfrom
fix/pool-unpinnable-2202
Open

Make the browser pool un-pinnable by hung crawls#2214
SohamKukreti wants to merge 1 commit into
developfrom
fix/pool-unpinnable-2202

Conversation

@SohamKukreti

Copy link
Copy Markdown
Collaborator

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:

  • Cancellation-safe release: release_crawler no longer takes the pool LOCK and has no
    await points, so a request cancelled mid-finally (client disconnect, deadline) can never
    skip the busy-counter decrement.
  • Janitor stale-lease backstop: a browser reporting "busy" that nothing has touched for
    longer than crawler.pool.stale_lease_s (default 0 = auto max(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.
  • Closes never block the pool: the janitor and close_all() route browser closes through
    a fire-and-forget background task (60 s cap, logged on timeout) instead of awaiting
    close() while holding the pool LOCK — a wedged browser previously froze get_crawler()
    and all future janitor passes for the whole server. close_all() drains pending closes
    (65 s cap) so shutdown is clean.
  • Per-crawl deadline on by default: limits.wall_clock_s is now 1800 (was 0). A
    non-streaming crawl exceeding it returns 504 and releases its browser. 0 remains the
    documented 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-free release_crawler, stale-lease
    backstop in both janitor sweeps, _close_in_background helper, close_all drain,
    ceiling config with safe fallback (bad YAML values, including true, fall back to auto).
  • deploy/docker/config.ymlwall_clock_s: 1800 default; new pool.stale_lease_s knob.
  • deploy/docker/MIGRATION.md — documents the new 504 default and the janitor force-close
    behavior, including the "raise stale_lease_s if your crawls legitimately run > 6 h" note.
  • deploy/docker/ARCHITECTURE.md — janitor pseudo-code and pool config reference updated to
    match the new design.
  • deploy/docker/utils.pydns.resolver import deferred into verify_email_domain() so
    the 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 real crawler_pool module
    instead 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?

  • Unit: 28 tests against the real module (no Docker needed):
    python -m pytest tests/docker/test_pool_release.py — release decrements without the LOCK
    and 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_all waits
    for pending background closes and never holds the LOCK while one hangs; ceiling resolution
    rejects nonsense values (-1, "30m", true, null).
  • Live acceptance (scripted): full suite against a locally built image plus a control run
    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: 0 the
    backstop 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).
  • Both shutdown orderings verified live: stop before the janitor reaps (close_all closes a
    live pooled browser via the background path) and stop after a background reap.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added/updated unit tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

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

1 participant