From 16f87d8bc94fed17095ef0b32d4b5ca4ba895cbc Mon Sep 17 00:00:00 2001 From: ciregenz Date: Wed, 26 Aug 2026 13:25:48 -0700 Subject: [PATCH] [eric] router: a stale-router kill is scoped to our own port and never fires under test (ENG-393) --- backend/apps/nine_router/process.py | 52 +++++++++++++++--- backend/tests/conftest.py | 2 + backend/tests/test_router_kill_is_scoped.py | 60 +++++++++++++++++++++ 3 files changed, 107 insertions(+), 7 deletions(-) create mode 100644 backend/tests/test_router_kill_is_scoped.py diff --git a/backend/apps/nine_router/process.py b/backend/apps/nine_router/process.py index 8df0c34a..5f30b15c 100644 --- a/backend/apps/nine_router/process.py +++ b/backend/apps/nine_router/process.py @@ -16,18 +16,48 @@ import logging import os import secrets import shutil +import signal import socket import stat import subprocess +import sys import tempfile import time -from typing import Any +from typing import Any, List, Optional import httpx logger = logging.getLogger(__name__) NINE_ROUTER_PORT = 20128 + + +def stale_router_pids() -> List[int]: + """PIDs LISTENING on our port, and only those. + + This was `pkill -f next-server`, which matches by process NAME and therefore killed every + next-server on the machine: the user's own packaged OpenSwarm, another worktree's dev stack, + an unrelated Next.js project. Measured 2026-08-24: a `pytest backend/tests` run killed the + running app's router (ENG-393). A destructive action must be scoped to the thing it owns. + """ + import subprocess as p_sp + try: + p_out = p_sp.run( + ["lsof", "-nP", f"-iTCP:{NINE_ROUTER_PORT}", "-sTCP:LISTEN", "-t"], + capture_output=True, text=True, timeout=3, + ).stdout + except Exception: + return [] + return [int(x) for x in p_out.split() if x.strip().isdigit()] + + +def router_kill_held_because() -> Optional[str]: + """Why this process must not kill a router it did not start, or None to proceed.""" + if os.environ.get("OSW_NEVER_KILL_ROUTER") == "1": + return "this process was told never to" + if "pytest" in sys.modules: + return "a test run has no business killing a live router" + return None NINE_ROUTER_URL = f"http://localhost:{NINE_ROUTER_PORT}" NINE_ROUTER_API = f"{NINE_ROUTER_URL}/api" NINE_ROUTER_V1 = f"{NINE_ROUTER_URL}/v1" @@ -521,13 +551,21 @@ async def p_ensure_running_impl(): return import subprocess as p_sp try: - result = p_sp.run( - ["pgrep", "-f", "next-server"], - capture_output=True, text=True, timeout=3, - ) - if result.stdout.strip(): + p_stale = stale_router_pids() + if p_stale and router_kill_held_because(): + logger.warning( + "9Router on port %d is not ours and %s, so it is being left alone; " + "this run will use whatever is already listening", + NINE_ROUTER_PORT, router_kill_held_because(), + ) + return + if p_stale: logger.info("Dev mode: killing stale standalone 9Router to use next dev instead") - p_sp.run(["pkill", "-f", "next-server"], timeout=5) + for p_pid in p_stale: + try: + os.kill(p_pid, signal.SIGTERM) + except OSError: + pass # The port is about to go dead; drop the positive-cache so the start-loop below actually re-probes instead of trusting the killed server's stale "ready". p_is_running_last_ok = 0.0 await asyncio.sleep(2) diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index ec57954a..1ce1a075 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -37,6 +37,8 @@ def _isolate_browser_state(monkeypatch): # whatever tree the suite runs from (ENG-388). The pytest-in-sys.modules fallback still exists, # but a gate that depends on an accident fails silently the day the accident changes. monkeypatch.setenv("OSW_DISABLE_AUTO_RESUME", "1") + # A suite run must never kill a router it did not start; that is the user's app (ENG-393). + monkeypatch.setenv("OSW_NEVER_KILL_ROUTER", "1") monkeypatch.setenv("OSW_PRESTAGE", "0") monkeypatch.setenv("OSW_FASTREAD_HOP", "0") monkeypatch.setenv("OSW_PRELUDE_TRIM", "0") diff --git a/backend/tests/test_router_kill_is_scoped.py b/backend/tests/test_router_kill_is_scoped.py new file mode 100644 index 00000000..0950c43c --- /dev/null +++ b/backend/tests/test_router_kill_is_scoped.py @@ -0,0 +1,60 @@ +"""The suite must never kill a router it did not start. That router is the user's running app. + +Measured 2026-08-24 (ENG-393): `pytest backend/tests` reached `ensure_running()`, which did +`pkill -f next-server` whenever OPENSWARM_PACKAGED was unset. 9router runs as a bare `next-server`, +so that command matched by NAME and killed every one on the machine: the packaged OpenSwarm the +developer had open, another worktree's dev stack, an unrelated Next.js project. Same batch, same +commit: backend up = hang at test 22 or 3 failures; ports clear = 159/161 passed. +""" + +import os +import sys + +from backend.apps.nine_router import process as p_proc + +SRC = "backend/apps/nine_router/process.py" + + +def test_the_kill_is_scoped_to_our_own_port(): + src = open(SRC).read() + # The docstring still names the old command; what must be gone is any INVOCATION of it, and a + # subprocess call has to quote its argv. + for bad in ('"pkill"', "'pkill'", '"pgrep"', "'pgrep'"): + assert bad not in src, f"{bad} matches by process NAME and reaches processes we do not own" + i = src.index("def stale_router_pids") + body = src[i:i + 900] + assert 'f"-iTCP:{NINE_ROUTER_PORT}"' in body and '"-sTCP:LISTEN"' in body + + +def test_a_test_run_is_held_by_a_declared_signal_first(): + why = p_proc.router_kill_held_because() + assert why, "under pytest it must always be held" + assert os.environ.get("OSW_NEVER_KILL_ROUTER") == "1", "conftest sets the DECLARED signal" + + +def test_the_declared_signal_holds_it_without_pytest(monkeypatch): + # The incidental signal stays a fallback: the day pytest is importable somewhere unexpected must + # not be the day this behaviour silently changes. + monkeypatch.setenv("OSW_NEVER_KILL_ROUTER", "1") + monkeypatch.delitem(sys.modules, "pytest", raising=False) + assert p_proc.router_kill_held_because() == "this process was told never to" + + +def test_a_real_dev_boot_can_still_replace_a_stale_router(monkeypatch): + # The feature exists so `next dev` can take over from a stale standalone build. Holding it + # always would be the opposite bug: a dev stack stuck on yesterday's router, silently. + monkeypatch.delenv("OSW_NEVER_KILL_ROUTER", raising=False) + monkeypatch.delitem(sys.modules, "pytest", raising=False) + assert p_proc.router_kill_held_because() is None + + +def test_the_hold_says_which_router_it_just_left_alone(): + src = open(SRC).read() + i = src.index("is not ours and %s") + assert "logger.warning" in src[i - 200:i], "a guard may never disable itself in silence" + assert "NINE_ROUTER_PORT" in src[i:i + 300] + + +def test_finding_no_listener_is_not_an_error(): + # lsof missing, or nothing listening, must read as "nothing to kill", never as a crash on boot. + assert isinstance(p_proc.stale_router_pids(), list)