diff --git a/readme.md b/readme.md index cdb2c07..aa5434d 100644 --- a/readme.md +++ b/readme.md @@ -3,7 +3,7 @@ Shelfmark > [!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 diff --git a/shelfmark/bypass/internal_bypasser.py b/shelfmark/bypass/internal_bypasser.py index ed7f499..429820e 100644 --- a/shelfmark/bypass/internal_bypasser.py +++ b/shelfmark/bypass/internal_bypasser.py @@ -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//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()) diff --git a/tests/bypass/test_internal_bypasser.py b/tests/bypass/test_internal_bypasser.py index 1ec4c6b..16f13fd 100644 --- a/tests/bypass/test_internal_bypasser.py +++ b/tests/bypass/test_internal_bypasser.py @@ -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": "solved", "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) == "solved" + 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]