fix(bypass): scope browser cleanup to the calling session (#1232)

The orphan sweep ran a container-wide 'pkill -9 -f
chrome|chromium|Xvfb|ffmpeg', so it also matched browsers another bypass
was still driving. Scope it by process group: kill only our own group
and groups whose leader has died. Spawn the helper with
start_new_session so its browser tree is identifiable, tear the whole
group down after every run (a timed-out helper used to leak its Chrome
and Xvfb), and have an orphaned helper take its browser down with it.
Fixes #1231.
This commit is contained in:
CaliBrain
2026-08-18 23:24:43 -04:00
committed by GitHub
parent 63133097e4
commit fae6140c6a
3 changed files with 330 additions and 48 deletions
+5 -3
View File
@@ -3,7 +3,7 @@
<img src="src/frontend/public/logo.png" alt="Shelfmark" width="200">
> [!NOTE]
> This project is in a stable state as of May 2026 but is not under active maintenance.
> Shelfmark is feature stable and maintained on a best-effort basis. Bug fixes, security updates, and small quality-of-life improvements are still shipped, and pull requests are reviewed — including new features. There is no roadmap for new features for now.
Shelfmark is a self-hosted web interface for searching and requesting books and audiobooks across multiple sources. Bring your own sources, metadata providers, and download clients to build a single hub for your digital library. Supports multiple users with a built-in request system, so you can share your instance with others and let them browse and request books on their own.
@@ -238,9 +238,11 @@ These are non-goals, not missing features.
## Contributing
Shelfmark's core feature set is complete. Development focuses on stability, bug fixes, quality-of-life improvements, and refining the search experience. Contributions in these areas are welcome, please file issues or submit pull requests on GitHub.
Shelfmark's core feature set is complete.
Feature requests that fall outside the project scope (library integration, automation, collection management) will be closed. If you're unsure whether something fits, open a discussion first.
Pull requests are welcome and all of them get reviewed, new features included. If you want a feature, the fastest path is to send a PR for it rather than to file a request.
Feature requests that fall outside the project scope (library integration, automation, collection management) will be closed, and PRs implementing them won't be merged. If you're unsure whether something fits, open a discussion first.
## Health Monitoring
+155 -45
View File
@@ -5,7 +5,6 @@ import asyncio
import json
import os
import random
import shutil
import signal
import socket
import stat
@@ -62,6 +61,7 @@ _BYPASS_SUBPROCESS_TIMEOUT_SECONDS = 420.0
# branches of get() are bounded the same way.
_IN_PROCESS_BYPASS_TIMEOUT_SECONDS = _BYPASS_SUBPROCESS_TIMEOUT_SECONDS
_BYPASS_CHILD_ENV = "SHELFMARK_INTERNAL_BYPASSER_CHILD"
_PARENT_WATCHDOG_INTERVAL_SECONDS = 5.0
# Challenge detection indicators
CLOUDFLARE_INDICATORS = [
@@ -98,8 +98,8 @@ DISPLAY: _DisplayState = {
"ffmpeg_output": None,
}
LOCKED = threading.Lock()
_PGREP_PATH = shutil.which("pgrep")
_PKILL_PATH = shutil.which("pkill")
_PROC_ROOT = Path("/proc")
_BROWSER_PROCESS_PATTERNS = ("chrome", "chromium", "Xvfb", "ffmpeg")
_RNG = random.SystemRandom()
_CDP_OPERATION_ERRORS = (
@@ -260,61 +260,110 @@ async def _extract_cookies_from_cdp(driver: Any, page: Any, url: str) -> None:
logger.debug("Failed to extract cookies: %s", e)
def _read_process_cmdline(proc_dir: Path) -> str:
"""Return a process's full command line, or "" when it cannot be read."""
try:
raw = (proc_dir / "cmdline").read_bytes()
except OSError:
return ""
return raw.replace(b"\x00", b" ").decode("utf-8", "replace").strip()
def _read_process_pgid(proc_dir: Path) -> int | None:
"""Return a process's group id from /proc/<pid>/stat, or None when unreadable."""
try:
stat_line = (proc_dir / "stat").read_text(encoding="utf-8", errors="replace")
except OSError:
return None
# Field 2 (comm) is parenthesised and may itself contain spaces and parens, so the
# fields are only unambiguous after the last ')': state, ppid, pgrp, ...
fields = stat_line.rpartition(")")[2].split()
pgrp_index = 2
if len(fields) <= pgrp_index:
return None
try:
return int(fields[pgrp_index])
except ValueError:
return None
def _find_browser_processes() -> list[tuple[int, int, str]]:
"""Return (pid, pgid, cmdline) for every browser-ish process visible in /proc."""
found: list[tuple[int, int, str]] = []
try:
entries = list(_PROC_ROOT.iterdir())
except OSError as e:
logger.debug("Could not list %s: %s", _PROC_ROOT, e)
return found
for entry in entries:
if not entry.name.isdigit():
continue
cmdline = _read_process_cmdline(entry)
if not cmdline or not any(name in cmdline for name in _BROWSER_PROCESS_PATTERNS):
continue
pgid = _read_process_pgid(entry)
if pgid is None:
continue
found.append((int(entry.name), pgid, cmdline))
return found
def _kill_process(pid: int, cmdline: str) -> bool:
"""SIGKILL one process, reporting whether it was actually signalled."""
try:
os.kill(pid, signal.SIGKILL)
except ProcessLookupError:
return False
except OSError as e:
logger.warning("Failed to kill pid %s: %s", pid, e)
return False
logger.debug("Killed leftover process %s: %s", pid, cmdline[:120])
return True
def _cleanup_orphan_processes() -> int:
"""Kill orphan Chrome/Xvfb/ffmpeg processes. Only runs in Docker mode."""
"""Kill leftover Chrome/Xvfb/ffmpeg processes. Only runs in Docker mode.
Scoped to this bypass session's process group plus groups whose leader has died.
A container-wide sweep (the old `pkill -9 -f chrome`) also matched the browsers a
concurrently running bypass was still driving, so with MAX_CONCURRENT_DOWNLOADS > 1
every worker that started a solve killed the others' browsers (#1231).
"""
if not env.DOCKERMODE:
return 0
_stop_ffmpeg_recording()
processes_to_kill = ["chrome", "chromium", "Xvfb", "ffmpeg"]
total_killed = 0
logger.debug("Checking for orphan processes...")
logger.debug("Checking for leftover browser processes...")
logger.log_resource_usage()
if _PGREP_PATH is None or _PKILL_PATH is None:
logger.warning("Skipping orphan-process cleanup because pgrep/pkill are unavailable")
if not _PROC_ROOT.is_dir():
logger.warning("Skipping browser-process cleanup because %s is unavailable", _PROC_ROOT)
return 0
for proc_name in processes_to_kill:
try:
result = subprocess.run(
[_PGREP_PATH, "-f", proc_name],
capture_output=True,
check=False,
text=True,
timeout=5,
)
if result.returncode != 0 or not result.stdout.strip():
continue
own_pid = os.getpid()
own_pgid = os.getpgrp()
total_killed = 0
pids = result.stdout.strip().split("\n")
count = len(pids)
logger.info("Found %s orphan %s process(es), killing...", count, proc_name)
kill_result = subprocess.run(
[_PKILL_PATH, "-9", "-f", proc_name],
capture_output=True,
check=False,
timeout=5,
)
if kill_result.returncode == 0:
total_killed += count
else:
logger.warning("pkill for %s returned %s", proc_name, kill_result.returncode)
except subprocess.TimeoutExpired:
logger.warning("Timeout while checking for %s processes", proc_name)
except _SUBPROCESS_OPERATION_ERRORS as e:
logger.debug("Error checking for %s processes: %s", proc_name, e)
for pid, pgid, cmdline in _find_browser_processes():
if pid == own_pid:
continue
# Another live process group means another bypass session: its browsers are in
# use, not orphans. Only our own group and groups whose leader is gone (a helper
# that died or was killed, leaving its browser behind) are ours to clean up.
if pgid != own_pgid and (_PROC_ROOT / str(pgid)).exists():
logger.debug("Leaving pid %s to its live bypass session (pgid %s)", pid, pgid)
continue
if _kill_process(pid, cmdline):
total_killed += 1
if total_killed > 0:
time.sleep(1)
logger.info("Cleaned up %s orphan process(es)", total_killed)
logger.info("Cleaned up %s leftover browser process(es)", total_killed)
logger.log_resource_usage()
else:
logger.debug("No orphan processes found")
logger.debug("No leftover browser processes found")
return total_killed
@@ -804,6 +853,21 @@ def _prepare_child_browser_env(env_vars: dict[str, str]) -> dict[str, str]:
return env_vars
def _terminate_helper_session(proc: subprocess.Popen[str]) -> None:
"""Kill the bypass helper and every process it spawned.
start_new_session makes the helper a session leader, so its pid doubles as the
process-group id of the browser tree underneath it and one killpg reaches all of it.
"""
if hasattr(os, "killpg"):
with suppress(OSError):
os.killpg(proc.pid, signal.SIGKILL)
with suppress(OSError):
proc.kill()
with suppress(OSError, subprocess.SubprocessError):
proc.wait(timeout=5)
def _get_via_subprocess(url: str, retry: int, cancel_flag: Event | None = None) -> str:
"""Run the browser bypass in a helper process isolated from gunicorn/gevent."""
_check_cancellation(cancel_flag, "Bypass cancelled before helper process")
@@ -830,14 +894,25 @@ def _get_via_subprocess(url: str, retry: int, cancel_flag: Event | None = None)
stdin=subprocess.PIPE,
text=True,
env=env_vars,
# Give the helper its own session: Chrome, Xvfb and ffmpeg inherit its process
# group, which is what lets the cleanup sweep tell this bypass's browsers apart
# from a concurrent worker's (#1231) and lets us kill the whole tree below.
start_new_session=True,
)
timed_out = False
try:
proc.communicate(json.dumps(payload), timeout=_BYPASS_SUBPROCESS_TIMEOUT_SECONDS)
except subprocess.TimeoutExpired:
proc.kill()
proc.wait()
timed_out = True
finally:
# Always tear the session down, not just on timeout: killing the helper alone
# leaves its Chrome and Xvfb running, and those leftovers are what made the next
# worker's browser fail to start in the first place.
_terminate_helper_session(proc)
if timed_out:
msg = "Internal bypasser helper process timed out"
raise TimeoutError(msg) from None
raise TimeoutError(msg)
try:
result = json.loads(result_path.read_text())
@@ -1207,6 +1282,38 @@ def _apply_parent_dns_config(dns_config: dict[str, Any]) -> None:
logger.warning("Could not apply parent DNS config (%s): %s", provider, exc)
def _terminate_own_session() -> None:
"""SIGKILL this process and every process it spawned, browser included."""
if hasattr(os, "killpg") and os.getpgrp() == os.getpid():
with suppress(OSError):
os.killpg(os.getpgrp(), signal.SIGKILL)
# A thread cannot end the process any other way; sys.exit would only end itself.
os._exit(1)
def _watch_parent_process(original_ppid: int, interval: float) -> None:
"""Take the browser down with us once the app process that spawned us is gone.
Cleanup only reclaims process groups whose leader has died, so a helper that outlives
its parent (worker restart, OOM kill) would sit there holding a browser that no later
bypass is allowed to touch.
"""
while os.getppid() == original_ppid:
time.sleep(interval)
logger.warning("Bypass helper lost its parent process; taking the browser down")
_terminate_own_session()
def _start_parent_watchdog() -> None:
"""Watch the spawning process in the background for the life of this helper."""
threading.Thread(
target=_watch_parent_process,
args=(os.getppid(), _PARENT_WATCHDOG_INTERVAL_SECONDS),
daemon=True,
name="BypassParentWatchdog",
).start()
def _run_child_process() -> int:
"""CLI entrypoint used by the Docker helper subprocess."""
request = json.loads(sys.stdin.read() or "{}")
@@ -1243,4 +1350,7 @@ def _run_child_process() -> int:
if __name__ == "__main__":
# Started here rather than in _run_child_process() so it only ever watches a real
# spawned helper, never a test or an embedded call.
_start_parent_watchdog()
raise SystemExit(_run_child_process())
+170
View File
@@ -1,5 +1,8 @@
import asyncio
import json
import subprocess
import threading
from pathlib import Path
import pytest
@@ -488,3 +491,170 @@ def test_run_bypass_in_current_process_bounds_its_wait(monkeypatch):
assert result == "html"
assert observed["timeout"] == internal_bypasser._IN_PROCESS_BYPASS_TIMEOUT_SECONDS
def _write_fake_proc_entry(proc_root, pid: int, pgid: int, argv: list[str]) -> None:
"""Create a /proc-shaped entry for a fake process."""
entry = proc_root / str(pid)
entry.mkdir()
(entry / "cmdline").write_bytes(b"\0".join(arg.encode() for arg in argv) + b"\0")
# pid (comm) state ppid pgrp ... - comm is parenthesised and may contain spaces.
(entry / "stat").write_text(f"{pid} (some (odd) name) S 1 {pgid} {pgid} 0 -1 4194304 0 0")
def test_cleanup_only_kills_own_and_abandoned_browser_sessions(monkeypatch, tmp_path):
"""Regression test for issue #1231: the sweep used a container-wide `pkill -f chrome`,
so every worker that started a bypass killed the browsers the other workers were
still driving. Only our own process group and groups whose leader is gone are ours."""
import shelfmark.bypass.internal_bypasser as internal_bypasser
proc_root = tmp_path / "proc"
proc_root.mkdir()
_write_fake_proc_entry(proc_root, 1000, 1000, ["python", "-m", "shelfmark.bypass"])
_write_fake_proc_entry(proc_root, 1001, 1000, ["/usr/bin/chromium", "--headless"])
_write_fake_proc_entry(proc_root, 1002, 1000, ["Xvfb", ":99"])
# Live sibling session: another worker is solving a challenge with these right now.
_write_fake_proc_entry(proc_root, 2000, 2000, ["python", "-m", "shelfmark.bypass"])
_write_fake_proc_entry(proc_root, 2001, 2000, ["/usr/bin/chromium", "--headless"])
# Abandoned session: its leader (pid 3000) is gone, so its browser really is an orphan.
_write_fake_proc_entry(proc_root, 3001, 3000, ["/usr/bin/chromium", "--headless"])
killed: list[int] = []
monkeypatch.setattr(internal_bypasser.env, "DOCKERMODE", True)
monkeypatch.setattr(internal_bypasser, "_PROC_ROOT", proc_root)
monkeypatch.setattr(internal_bypasser.os, "getpid", lambda: 1000)
monkeypatch.setattr(internal_bypasser.os, "getpgrp", lambda: 1000)
monkeypatch.setattr(internal_bypasser.os, "kill", lambda pid, _sig: killed.append(pid))
monkeypatch.setattr(internal_bypasser.time, "sleep", lambda _seconds: None)
assert internal_bypasser._cleanup_orphan_processes() == 3
assert sorted(killed) == [1001, 1002, 3001]
def test_cleanup_is_skipped_without_proc(monkeypatch, tmp_path):
"""Without /proc there is no way to tell sessions apart, so kill nothing."""
import shelfmark.bypass.internal_bypasser as internal_bypasser
monkeypatch.setattr(internal_bypasser.env, "DOCKERMODE", True)
monkeypatch.setattr(internal_bypasser, "_PROC_ROOT", tmp_path / "missing")
monkeypatch.setattr(
internal_bypasser.os, "kill", lambda *_args: pytest.fail("must not kill anything")
)
assert internal_bypasser._cleanup_orphan_processes() == 0
class _FakeHelperProcess:
"""Stand-in for the bypass helper subprocess."""
def __init__(self, *_args, **kwargs):
self.kwargs = kwargs
self.pid = 4242
self.returncode = 0
self.timed_out = False
self.killed = False
self.waited = False
def communicate(self, payload, timeout=None):
if self.timed_out:
raise subprocess.TimeoutExpired(cmd="helper", timeout=timeout)
request = json.loads(payload)
result = {"ok": True, "html": "<html>solved</html>", "cookies": {}, "user_agents": {}}
Path(request["result_path"]).write_text(json.dumps(result), encoding="utf-8")
return "", ""
def kill(self):
self.killed = True
def wait(self, timeout=None):
self.waited = True
return self.returncode
def _patch_helper_subprocess(monkeypatch, internal_bypasser, process, killed_groups):
monkeypatch.setattr(internal_bypasser.subprocess, "Popen", lambda *a, **kw: process(*a, **kw))
monkeypatch.setattr(internal_bypasser.network, "get_dns_config", dict)
monkeypatch.setattr(
internal_bypasser.os, "killpg", lambda pgid, _sig: killed_groups.append(pgid)
)
def test_helper_runs_in_its_own_session_and_is_torn_down(monkeypatch):
"""Regression test for issue #1231: the helper's Chrome and Xvfb must belong to the
helper's own process group, and the whole group must die with it - otherwise the
leftovers break the next worker's browser and can only be cleared by a sweep broad
enough to kill a concurrent worker's browser too."""
import shelfmark.bypass.internal_bypasser as internal_bypasser
processes: list[_FakeHelperProcess] = []
killed_groups: list[int] = []
def _make_process(*args, **kwargs):
process = _FakeHelperProcess(*args, **kwargs)
processes.append(process)
return process
_patch_helper_subprocess(monkeypatch, internal_bypasser, _make_process, killed_groups)
assert internal_bypasser._get_via_subprocess("https://example.com", 1) == "<html>solved</html>"
assert processes[0].kwargs["start_new_session"] is True
assert killed_groups == [processes[0].pid]
def test_helper_timeout_kills_the_whole_session(monkeypatch):
"""A timed-out solve must not leave a live browser behind for the next worker."""
import shelfmark.bypass.internal_bypasser as internal_bypasser
processes: list[_FakeHelperProcess] = []
killed_groups: list[int] = []
def _make_process(*args, **kwargs):
process = _FakeHelperProcess(*args, **kwargs)
process.timed_out = True
processes.append(process)
return process
_patch_helper_subprocess(monkeypatch, internal_bypasser, _make_process, killed_groups)
with pytest.raises(TimeoutError):
internal_bypasser._get_via_subprocess("https://example.com", 1)
assert killed_groups == [processes[0].pid]
assert processes[0].killed is True
def test_helper_takes_the_browser_down_when_its_parent_dies(monkeypatch):
"""Cleanup only reclaims process groups whose leader is gone (#1231), so an orphaned
helper must not sit there holding a browser no later bypass is allowed to touch."""
import shelfmark.bypass.internal_bypasser as internal_bypasser
terminated: list[str] = []
monkeypatch.setattr(internal_bypasser.os, "getppid", lambda: 1)
monkeypatch.setattr(
internal_bypasser, "_terminate_own_session", lambda: terminated.append("terminated")
)
monkeypatch.setattr(
internal_bypasser.time, "sleep", lambda _seconds: pytest.fail("should not wait")
)
internal_bypasser._watch_parent_process(999, interval=0.0)
assert terminated == ["terminated"]
def test_helper_watchdog_waits_while_its_parent_is_alive(monkeypatch):
"""The watchdog must only fire on a changed ppid, not on every poll."""
import shelfmark.bypass.internal_bypasser as internal_bypasser
ppids = iter([999, 999, 1])
sleeps: list[float] = []
monkeypatch.setattr(internal_bypasser.os, "getppid", lambda: next(ppids))
monkeypatch.setattr(internal_bypasser, "_terminate_own_session", lambda: None)
monkeypatch.setattr(internal_bypasser.time, "sleep", sleeps.append)
internal_bypasser._watch_parent_process(999, interval=0.5)
assert sleeps == [0.5, 0.5]