From 5d775ff8eef1cabbaa33fc48ef82fce4e3fc77bd Mon Sep 17 00:00:00 2001 From: John Paul Soliva Date: Sun, 20 Sep 2026 23:29:29 +0900 Subject: [PATCH] perf(gateway): read a PID's command line with psutil before forking ps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_read_process_cmdline` tried `/proc`, then `ps`, then psutil. macOS and BSD have no `/proc`, so every call there forked `ps -p -o command=` while psutil — a pinned core dependency, which pyproject calls "the canonical answer" for PID questions — sat unused behind it, returning the same string in-process. That call is the gateway-identity check guarding against PID reuse, reached for every profile with a live gateway, and `list_profiles()` runs it per profile. `list_profiles` is the shared body of `GET /api/profiles` and `profiles.list`, which the Bots roster polls every 5s per connection, so the forks repeat forever on an idle machine. Measured on macOS: one call 4.17ms via `ps` against 0.022ms via psutil. End to end against a home whose profiles each hold a live gateway lock, A/B in one process state, two rounds: 4 gateways 23.9ms -> 4.0ms, 8 gateways 49.0ms -> 9.3ms. `ps` stays as the fallback rather than being replaced: on macOS psutil raises AccessDenied for a process owned by another user, which `ps` still reports — real when a dashboard probes root-owned LaunchDaemon gateways. That path is unchanged and simply pays a cheap failed lookup first. For a readable process both sources return byte-identical strings, so the command-line matchers see no difference, and Linux still answers from /proc without reaching either. Regressions: psutil answers without any fork (a raising `subprocess.run` proves it); the `ps` fallback still answers when psutil refuses, as it does across users; a dead PID reports nothing from either; and both sources agree on a live gateway-shaped process. Restoring the old order fails the first. Fixes #117270 --- gateway/status.py | 18 ++-- tests/gateway/test_process_cmdline_source.py | 103 +++++++++++++++++++ 2 files changed, 115 insertions(+), 6 deletions(-) create mode 100644 tests/gateway/test_process_cmdline_source.py diff --git a/gateway/status.py b/gateway/status.py index 0aa8214244..c1e326d2c9 100644 --- a/gateway/status.py +++ b/gateway/status.py @@ -450,11 +450,22 @@ def get_process_start_time(pid: int) -> Optional[int]: def _read_process_cmdline(pid: int) -> Optional[str]: - """Process command line as one string: /proc, then ``ps``, then psutil (Windows).""" + """Process command line as one string: /proc, then psutil, then ``ps``. + + Order is by cost, and this runs per live gateway on every roster/status poll. ``psutil`` reads + the process table in-process (a ``sysctl`` on macOS) where ``ps`` costs a fork+exec — measured + 0.02ms against 4.2ms on macOS for the same string. It cannot always answer: on macOS it raises + ``AccessDenied`` for a process owned by another user, which ``ps`` still reports, so ``ps`` + stays as the fallback rather than being replaced.""" with contextlib.suppress(OSError): raw = Path(f"/proc/{pid}/cmdline").read_bytes() if raw: return raw.replace(b"\x00", b" ").decode("utf-8", errors="ignore").strip() + with contextlib.suppress(Exception): + import psutil # type: ignore + cmdline_parts = psutil.Process(pid).cmdline() + if cmdline_parts: + return " ".join(cmdline_parts) if not _IS_WINDOWS: with contextlib.suppress(OSError, subprocess.TimeoutExpired): result = subprocess.run( @@ -463,11 +474,6 @@ def _read_process_cmdline(pid: int) -> Optional[str]: ) if result.returncode == 0 and result.stdout.strip(): return result.stdout.strip() - with contextlib.suppress(Exception): - import psutil # type: ignore - cmdline_parts = psutil.Process(pid).cmdline() - if cmdline_parts: - return " ".join(cmdline_parts) return None diff --git a/tests/gateway/test_process_cmdline_source.py b/tests/gateway/test_process_cmdline_source.py new file mode 100644 index 0000000000..e45accbddc --- /dev/null +++ b/tests/gateway/test_process_cmdline_source.py @@ -0,0 +1,103 @@ +"""``_read_process_cmdline`` asks the cheapest source that can answer, and still answers. + +The gateway-identity check reads a live PID's command line, and `list_profiles` runs it per profile +with a live gateway — on every 5s roster poll, per connection. On macOS there is no ``/proc``, so +this used to fork ``ps`` every time (measured 4.2ms) when psutil answers the same string in 0.02ms. +psutil cannot always answer (macOS raises ``AccessDenied`` across users), so ``ps`` must remain the +fallback, not be replaced. +""" +from __future__ import annotations + +import os +import subprocess +import sys +import time + +import pytest + +from gateway import status + + +@pytest.fixture +def no_proc(monkeypatch): + """Force the non-Linux shape: /proc unreadable, so the source choice is psutil vs ps.""" + real_read_bytes = status.Path.read_bytes + + def _read_bytes(self, *a, **kw): + if str(self).startswith("/proc/"): + raise OSError("no /proc") + return real_read_bytes(self, *a, **kw) + + monkeypatch.setattr(status.Path, "read_bytes", _read_bytes) + + +def _no_fork(monkeypatch): + """Fail loudly if anything forks a subprocess.""" + def _boom(*a, **kw): + raise AssertionError(f"forked a subprocess: {a[0] if a else kw}") + + monkeypatch.setattr(status.subprocess, "run", _boom) + + +def test_psutil_answers_without_forking_ps(no_proc, monkeypatch): + _no_fork(monkeypatch) + + cmdline = status._read_process_cmdline(os.getpid()) + + assert cmdline and sys.executable.split("/")[-1] in cmdline + + +def test_ps_still_answers_when_psutil_cannot(no_proc, monkeypatch): + """macOS raises AccessDenied across users; ps still reports those, so it stays the fallback.""" + import psutil + + def _denied(_pid): + raise psutil.AccessDenied(_pid) + + monkeypatch.setattr(psutil, "Process", _denied) + forked: list = [] + real_run = status.subprocess.run + + def _watching(cmd, *a, **kw): + forked.append(cmd) + return real_run(cmd, *a, **kw) + + monkeypatch.setattr(status.subprocess, "run", _watching) + + cmdline = status._read_process_cmdline(os.getpid()) + + assert cmdline, "the ps fallback must still answer when psutil refuses" + assert forked and forked[0][:2] == ["ps", "-p"] + + +def test_a_dead_pid_reports_nothing_from_either_source(no_proc): + dead = subprocess.Popen([sys.executable, "-c", "pass"]) + dead.wait() + + assert status._read_process_cmdline(dead.pid) in (None, "") + + +def test_both_sources_agree_on_a_live_process(no_proc, monkeypatch): + """The consumer matches on this string, so the two sources must not disagree.""" + import psutil + + proc = subprocess.Popen([sys.executable, "-c", "import time; time.sleep(30)", "gateway", "run"]) + try: + for _ in range(50): + if psutil.pid_exists(proc.pid): + break + time.sleep(0.05) + via_psutil = status._read_process_cmdline(proc.pid) + + denied = psutil.AccessDenied + + def _raise(_pid): + raise denied(_pid) + + monkeypatch.setattr(psutil, "Process", _raise) + via_ps = status._read_process_cmdline(proc.pid) + finally: + proc.kill() + proc.wait() + + assert via_psutil == via_ps