From 6f10e4d48b706c6384014f01e608d4f9bcfb8408 Mon Sep 17 00:00:00 2001 From: ai-ag2026 <261867348+ai-ag2026@users.noreply.github.com> Date: Sat, 11 Jul 2026 19:32:47 +0200 Subject: [PATCH 1/7] fix(ctl): detect foreign/supervised WebUI instances instead of double-starting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ctl.sh start only guarded against launchd (macOS). On Linux, with a systemd-supervised WebUI serving the port and a stale PID file, stop reported 'stopped', start spawned a bootstrap that died ~2s later on server.py's 'already responding' check — after the 0.15s aliveness gate had already printed 'Started' and recorded the doomed PID. Killing the foreign server by hand then put its supervisor's auto-restart into a race with ctl.sh's start, ending in a permanent RestartSec crash loop. - start: refuse when anything answers HTTP(S) on the target port (any response bytes, matching server.py's abort semantics — a 404 squatter still dooms our server), and when the hermes-webui systemd unit is active on our port or mid-auto-restart (activating). Port scoping mirrors the launchd #3291 over-block fix; overrides: HERMES_WEBUI_CTL_ALLOW_SYSTEMD_CONFLICT / _ALLOW_PORT_CONFLICT, unit name via HERMES_WEBUI_SYSTEMD_UNIT. - start: watch the child through a startup grace window (HERMES_WEBUI_START_GRACE, default 3s) — report failure and clean the PID file when it dies during startup; break early once /health answers. - status/stop: when ctl.sh owns no PID but the port answers, say 'running (not managed by ctl.sh)' with listener diagnostics instead of 'stopped', and never touch the foreign process. - _pid_listens_on_port: ss fallback for Linux hosts without lsof. Co-Authored-By: Claude Fable 5 --- ctl.sh | 212 +++++++++++++- tests/test_ctl_foreign_server_guard.py | 367 +++++++++++++++++++++++++ tests/test_ctl_script.py | 5 + 3 files changed, 578 insertions(+), 6 deletions(-) create mode 100644 tests/test_ctl_foreign_server_guard.py diff --git a/ctl.sh b/ctl.sh index 2e827aad..e062356f 100755 --- a/ctl.sh +++ b/ctl.sh @@ -390,6 +390,20 @@ _pid_listens_on_port() { fi return 1 # PID is alive but NOT listening on that port → no conflict fi + # Linux hosts (where the systemd guard runs) often lack lsof but ship ss. + if command -v ss >/dev/null 2>&1; then + local ss_out rows + if ! ss_out="$(ss -tlnp 2>/dev/null)"; then + return 2 + fi + rows="$(printf '%s\n' "${ss_out}" | awk -v p="${port}" '$4 ~ (":" p "$")')" + [[ -z "${rows}" ]] && return 1 # nothing listens on that port at all + printf '%s\n' "${rows}" | grep -q "pid=${pid}," && return 0 + # Listener rows exist; when they carry pid= attribution and none is ours, + # some OTHER process owns the port — not a conflict for this pid. + printf '%s\n' "${rows}" | grep -q "pid=" && return 1 + return 2 # listener present but unattributable + fi return 2 # can't determine } @@ -424,6 +438,105 @@ _launchd_webui_pid() { esac } +_probe_target_host() { + # A bind-all host (0.0.0.0 / ::) is not a connectable probe target; probe + # loopback instead (mirrors server.py _abort_if_already_serving). + case "${1:-}" in + 0.0.0.0 | '' | ::) printf '127.0.0.1' ;; + *) printf '%s' "$1" ;; + esac +} + +_port_answers_http() { + # True when ANYTHING answers an HTTP(S) request on host:port — mirrors + # server.py's _abort_if_already_serving, which treats any response bytes + # (including an error status from a foreign app squatting the port) as a + # conflict. Deliberately broader than hermes_webui_probe_health, which + # requires a 200 from /health. + local host="$1" port="$2" url rc scheme + for scheme in http https; do + url="${scheme}://${host}:${port}/health" + if command -v curl >/dev/null 2>&1; then + # No -f: an HTTP error status is still a responder. -k: a self-signed + # cert is still a responder. + curl -sS -o /dev/null -k --max-time 2 "${url}" 2>/dev/null + rc=$? + [[ ${rc} -eq 0 ]] && return 0 + elif command -v wget >/dev/null 2>&1; then + wget -qO /dev/null --no-check-certificate "--timeout=2" --tries=1 "${url}" 2>/dev/null + rc=$? + # 0 = OK; 8 = server answered with an error status — both are responders. + [[ ${rc} -eq 0 || ${rc} -eq 8 ]] && return 0 + else + return 1 + fi + done + return 1 +} + +_port_listener_diag() { + # Best-effort one-liner describing what listens on TCP port $1. Used in + # error/status messages only — never fails the caller. + local port="$1" line="" + if command -v ss >/dev/null 2>&1; then + line="$(ss -tlnp 2>/dev/null | awk -v p="${port}" '$4 ~ (":" p "$") {print; exit}')" + fi + if [[ -z "${line}" ]] && command -v lsof >/dev/null 2>&1; then + line="$(lsof -nP -iTCP:"${port}" -sTCP:LISTEN 2>/dev/null | awk 'NR==2 {print; exit}')" + fi + [[ -n "${line}" ]] && printf '%s' "${line}" +} + +_systemd_webui_conflict() { + # Linux analog of _launchd_webui_pid: echo a short conflict descriptor and + # return 0 when a systemd unit (default hermes-webui.service, override via + # HERMES_WEBUI_SYSTEMD_UNIT) effectively owns the instance we are about to + # start. Two conflict shapes: + # - ActiveState=active and the unit's MainPID listens on our port. + # - ActiveState=activating/reloading (Restart= backoff between attempts): + # the port is briefly silent, but the unit WILL respawn and re-bind; a + # ctl.sh-started server would trap it in a permanent crash loop + # (each systemd attempt aborts on the "already responding" check, 5s + # later the next one dies the same way — observed in the field). + # Port scoping mirrors the launchd #3291 fix: when the unit's port cannot + # be determined, only the default port is guarded, so alternate-port test + # instances are never wrongly blocked. + [[ "${HERMES_WEBUI_CTL_ALLOW_SYSTEMD_CONFLICT:-0}" == "1" ]] && return 1 + command -v systemctl >/dev/null 2>&1 || return 1 + local unit="${HERMES_WEBUI_SYSTEMD_UNIT:-hermes-webui.service}" + [[ -n "${unit}" ]] || return 1 + local want_port="${CTL_PORT:-${HERMES_WEBUI_PORT:-8787}}" + local scope state main_pid + for scope in --system --user; do + state="$(systemctl "${scope}" show -p ActiveState --value "${unit}" 2>/dev/null)" || continue + case "${state}" in + active) + main_pid="$(systemctl "${scope}" show -p MainPID --value "${unit}" 2>/dev/null)" + [[ "${main_pid}" =~ ^[0-9]+$ ]] || main_pid=0 + if (( main_pid > 0 )); then + _pid_listens_on_port "${main_pid}" "${want_port}" + case "$?" in + 0) printf 'unit %s is active (MainPID %s listens on port %s)' "${unit}" "${main_pid}" "${want_port}"; return 0 ;; + 1) continue ;; # active on a different port → no conflict + esac + fi + # Port ownership unknown: guard only the default port (see #3291). + if [[ "${want_port}" == "8787" ]]; then + printf 'unit %s is active' "${unit}" + return 0 + fi + ;; + activating | reloading) + if [[ "${want_port}" == "8787" ]]; then + printf 'unit %s is %s (auto-restart pending)' "${unit}" "${state}" + return 0 + fi + ;; + esac + done + return 1 +} + start_cmd() { ensure_home _load_repo_dotenv_preserving_env @@ -446,6 +559,28 @@ start_cmd() { echo "[ctl] Use launchctl kickstart -k gui/$(id -u)/${HERMES_WEBUI_LAUNCHD_LABEL:-${DEFAULT_LAUNCHD_LABEL}} or disable the launchd job before using ctl.sh start." >&2 return 2 fi + local systemd_conflict + if systemd_conflict="$(_systemd_webui_conflict 2>/dev/null)"; then + echo "[ctl] Refusing to start a second Hermes WebUI: systemd ${systemd_conflict}." >&2 + echo "[ctl] Manage that instance with systemctl instead (e.g. sudo systemctl restart ${HERMES_WEBUI_SYSTEMD_UNIT:-hermes-webui.service}), or disable the unit before using ctl.sh start. Set HERMES_WEBUI_CTL_ALLOW_SYSTEMD_CONFLICT=1 to override." >&2 + return 2 + fi + # Generic duplicate guard: whatever supervises it, if a live server already + # answers on the target host:port, a second one is doomed — server.py aborts + # on its "already responding" startup check a few seconds in, AFTER ctl.sh's + # old 0.15s aliveness check had reported success and recorded a PID file + # that goes stale on exit. Refuse up front instead. + local probe_host + probe_host="$(_probe_target_host "${CTL_HOST}")" + if [[ "${HERMES_WEBUI_CTL_ALLOW_PORT_CONFLICT:-0}" != "1" ]] \ + && _port_answers_http "${probe_host}" "${CTL_PORT}"; then + echo "[ctl] Refusing to start: a live server is already responding on ${probe_host}:${CTL_PORT}." >&2 + local listener_diag + listener_diag="$(_port_listener_diag "${CTL_PORT}")" + [[ -n "${listener_diag}" ]] && echo "[ctl] listener: ${listener_diag}" >&2 + echo "[ctl] Stop that instance first (its own supervisor may restart it — check systemctl/launchctl)." >&2 + return 2 + fi _clear_stale_pid >/dev/null 2>&1 || true local python_exe pid @@ -461,7 +596,30 @@ start_cmd() { printf '%s\n' "${pid}" > "${PID_FILE}" _write_state "${pid}" "${CTL_HOST}" "${CTL_PORT}" "${python_exe}" - sleep 0.15 + # Watch the child through its startup window instead of a single 0.15s + # aliveness check. A server that dies during startup (bad venv, import + # error, port stolen between guard and bind) exits after ~1-3s — after the + # old check had already printed success and left a stale PID file behind. + # Break early as soon as /health answers; report failure the moment the + # process dies. HERMES_WEBUI_START_GRACE (integer seconds, default 3) + # bounds the wait for servers that need longer before /health responds — + # on timeout we keep the optimistic legacy behavior and say so. + local grace="${HERMES_WEBUI_START_GRACE:-3}" + [[ "${grace}" =~ ^[0-9]+$ ]] || grace=3 + local grace_steps=$(( grace * 4 )) step=0 healthy=0 + while (( step < grace_steps )); do + if ! _is_alive "${pid}"; then + echo "[ctl] Hermes WebUI failed to stay running. Log: ${LOG_FILE}" >&2 + rm -f "${PID_FILE}" "${STATE_FILE}" + return 1 + fi + if hermes_webui_probe_health "${probe_host}" "${CTL_PORT}" "/health" 1 >/dev/null 2>&1; then + healthy=1 + break + fi + sleep 0.25 + step=$(( step + 1 )) + done if ! _is_alive "${pid}"; then echo "[ctl] Hermes WebUI failed to stay running. Log: ${LOG_FILE}" >&2 rm -f "${PID_FILE}" "${STATE_FILE}" @@ -470,6 +628,28 @@ start_cmd() { echo "[ctl] Started Hermes WebUI (PID ${pid})" echo "[ctl] Bound: ${CTL_HOST}:${CTL_PORT}" echo "[ctl] Log: ${LOG_FILE}" + if (( ! healthy )); then + echo "[ctl] Note: /health did not respond within ${grace}s; check '$(basename "$0") status' shortly." + fi +} + +_warn_if_unmanaged_instance_serving() { + # After stop concluded "nothing to do", check whether a server is STILL + # answering on the configured port. Silently reporting "stopped" while a + # foreign (e.g. systemd-supervised) instance keeps serving is how operators + # end up starting doomed duplicates. + _load_state_if_present + local host="${HOST:-${HERMES_WEBUI_HOST:-127.0.0.1}}" + local port="${PORT:-${HERMES_WEBUI_PORT:-8787}}" + local probe_host + probe_host="$(_probe_target_host "${host}")" + if _port_answers_http "${probe_host}" "${port}"; then + echo "[ctl] Warning: an instance NOT managed by ctl.sh is still serving ${probe_host}:${port} — not touching it." >&2 + local listener_diag + listener_diag="$(_port_listener_diag "${port}")" + [[ -n "${listener_diag}" ]] && echo "[ctl] listener: ${listener_diag}" >&2 + echo "[ctl] If it is supervised (systemd/launchd), stop or restart it there instead." >&2 + fi } stop_cmd() { @@ -478,11 +658,13 @@ stop_cmd() { if ! pid="$(_pid_from_file 2>/dev/null)"; then echo "[ctl] Hermes WebUI is stopped" rm -f "${PID_FILE}" "${STATE_FILE}" + _warn_if_unmanaged_instance_serving return 0 fi if ! _is_alive "${pid}" || ! _is_owned_webui_pid "${pid}"; then _clear_stale_pid + _warn_if_unmanaged_instance_serving return 0 fi @@ -551,11 +733,29 @@ status_cmd() { echo " Health: ${health}" else [[ -f "${PID_FILE}" ]] && _clear_stale_pid >/dev/null 2>&1 || true - echo "● hermes-webui — stopped" - echo " PID: -" - echo " Bound: ${host}:${port}" - echo " Log: ${log_path}" - echo " Health: not checked" + local probe_host + probe_host="$(_probe_target_host "${host}")" + if _port_answers_http "${probe_host}" "${port}"; then + # Something serves the port but ctl.sh does not own it (systemd unit, + # launchd job, manual run). Saying "stopped" here is what leads + # operators to start a doomed duplicate. + health="$(_health_line "${probe_host}" "${port}")" + echo "● hermes-webui — running (not managed by ctl.sh)" + local listener_diag + listener_diag="$(_port_listener_diag "${port}")" + echo " PID: -" + [[ -n "${listener_diag}" ]] && echo " Listener: ${listener_diag}" + echo " Bound: ${host}:${port}" + echo " Log: ${log_path}" + echo " Health: ${health}" + echo " Note: manage it via its own supervisor (systemctl/launchctl) or the process directly." + else + echo "● hermes-webui — stopped" + echo " PID: -" + echo " Bound: ${host}:${port}" + echo " Log: ${log_path}" + echo " Health: not checked" + fi fi } diff --git a/tests/test_ctl_foreign_server_guard.py b/tests/test_ctl_foreign_server_guard.py new file mode 100644 index 00000000..c0d05e5b --- /dev/null +++ b/tests/test_ctl_foreign_server_guard.py @@ -0,0 +1,367 @@ +"""ctl.sh must not double-start against a foreign/supervised WebUI instance. + +Field failure this pins down: a systemd-supervised WebUI (Restart=always) was +serving :8787 while ctl.sh's PID file was stale. `ctl.sh stop` said "stopped", +`ctl.sh restart` spawned a bootstrap that died ~2s later on server.py's +"already responding" startup check — AFTER ctl.sh's old 0.15s aliveness gate +had printed "Started" and recorded the doomed PID. Killing the foreign server +by hand then put systemd's auto-restart into a race with ctl.sh's own start, +ending in a permanent 5s crash loop (each systemd attempt aborting against the +ctl.sh-started server). + +Guards under test: +- start refuses when anything already answers HTTP(S) on the target port. +- start refuses when the hermes-webui systemd unit is active on our port or + mid-auto-restart (activating), instead of racing its next respawn. +- start reports failure (and cleans the PID file) when the spawned server dies + during the startup window instead of printing success after 0.15s. +- status/stop surface a running unmanaged instance instead of claiming + "stopped" and never touch a process ctl.sh does not own. +""" + +import os +import socket +import subprocess +import sys +import textwrap +import time +from pathlib import Path + +import pytest + +from tests.test_ctl_script import ( + bash_path, + run_ctl, + write_fake_python, +) + + +def _free_port() -> int: + with socket.socket() as s: + s.bind(("127.0.0.1", 0)) + return s.getsockname()[1] + + +def _start_dummy_http_server(port: int, status: int = 200) -> subprocess.Popen: + """A minimal HTTP server answering every request with `status`.""" + code = textwrap.dedent( + f""" + from http.server import BaseHTTPRequestHandler, HTTPServer + + class H(BaseHTTPRequestHandler): + def do_GET(self): + self.send_response({status}) + body = b'{{"status": "ok", "sessions": 0, "active_streams": 0}}' + self.send_header("Content-Type", "application/json") + self.send_header("Content-Length", str(len(body))) + self.end_headers() + self.wfile.write(body) + + def log_message(self, *args): + pass + + HTTPServer(("127.0.0.1", {port}), H).serve_forever() + """ + ) + proc = subprocess.Popen([sys.executable, "-c", code]) + deadline = time.time() + 5 + while time.time() < deadline: + try: + with socket.create_connection(("127.0.0.1", port), timeout=0.2): + return proc + except OSError: + time.sleep(0.05) + proc.terminate() + raise AssertionError("dummy HTTP server did not come up") + + +def _stop_proc(proc: subprocess.Popen) -> None: + proc.terminate() + try: + proc.wait(timeout=3) + except subprocess.TimeoutExpired: + proc.kill() + + +def _write_fake_systemctl(fake_bin: Path, active_state: str, main_pid: int = 0) -> None: + """Fake systemctl answering `show -p ActiveState|MainPID --value `.""" + systemctl = fake_bin / "systemctl" + systemctl.write_text( + textwrap.dedent( + f""" + #!/usr/bin/env bash + for arg in "$@"; do + case "${{arg}}" in + *ActiveState*) echo "{active_state}"; exit 0 ;; + *MainPID*) echo "{main_pid}"; exit 0 ;; + esac + done + exit 0 + """ + ).lstrip(), + encoding="utf-8", + ) + systemctl.chmod(0o755) + + +def _write_fake_port_tools(fake_bin: Path, pid_listens: bool) -> None: + """Fake lsof/ss so _pid_listens_on_port resolves deterministically.""" + lsof = fake_bin / "lsof" + lsof.write_text( + "#!/usr/bin/env bash\n" + ("exit 0\n" if pid_listens else "exit 1\n"), + encoding="utf-8", + ) + lsof.chmod(0o755) + # _pid_listens_on_port prefers lsof; keep a fake ss anyway so the shim + # works on hosts without lsof, where the ss branch runs instead. + ss = fake_bin / "ss" + if pid_listens: + ss.write_text( + '#!/usr/bin/env bash\n' + 'echo "LISTEN 0 64 0.0.0.0:8787 0.0.0.0:* users:((\\"python\\",pid=4242,fd=7))"\n', + encoding="utf-8", + ) + else: + ss.write_text("#!/usr/bin/env bash\nexit 0\n", encoding="utf-8") + ss.chmod(0o755) + + +def _guard_env(fake_bin: Path | None = None, **extra: str) -> dict[str, str]: + env = { + "HERMES_WEBUI_CTL_ALLOW_LAUNCHD_CONFLICT": "1", + } + if fake_bin is not None: + env["PATH"] = f"{bash_path(fake_bin)}{os.pathsep}{os.environ.get('PATH', '')}" + env.update(extra) + return env + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX daemon guards") +def test_start_refuses_when_port_already_serving(tmp_path): + port = _free_port() + server = _start_dummy_http_server(port) + try: + result = run_ctl( + tmp_path, + "start", + env=_guard_env( + HERMES_WEBUI_PORT=str(port), + HERMES_WEBUI_CTL_ALLOW_SYSTEMD_CONFLICT="1", + ), + timeout=15, + ) + combined = result.stdout + result.stderr + assert result.returncode == 2, combined + assert "already responding" in combined + assert not (tmp_path / ".hermes" / "webui.pid").exists() + finally: + _stop_proc(server) + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX daemon guards") +def test_start_refuses_even_when_squatter_answers_http_errors(tmp_path): + """server.py treats ANY response bytes as a conflict; the guard must match + (a 404 from a foreign app squatting the port still dooms our server).""" + port = _free_port() + server = _start_dummy_http_server(port, status=404) + try: + result = run_ctl( + tmp_path, + "start", + env=_guard_env( + HERMES_WEBUI_PORT=str(port), + HERMES_WEBUI_CTL_ALLOW_SYSTEMD_CONFLICT="1", + ), + timeout=15, + ) + combined = result.stdout + result.stderr + assert result.returncode == 2, combined + assert "already responding" in combined + finally: + _stop_proc(server) + + +@pytest.mark.skipif(sys.platform == "win32", reason="systemd is a Linux/POSIX path") +def test_start_refuses_when_systemd_unit_is_auto_restarting(tmp_path): + """ActiveState=activating means the unit is between Restart= attempts: the + port is briefly silent, but starting now traps the unit in a crash loop.""" + fake_bin = tmp_path / "bin" + fake_bin.mkdir() + _write_fake_systemctl(fake_bin, "activating") + + result = run_ctl( + tmp_path, + "start", + env=_guard_env(fake_bin), + timeout=15, + ) + combined = result.stdout + result.stderr + assert result.returncode == 2, combined + assert "activating" in combined + assert "systemctl" in combined + assert not (tmp_path / ".hermes" / "webui.pid").exists() + + +@pytest.mark.skipif(sys.platform == "win32", reason="systemd is a Linux/POSIX path") +def test_start_refuses_when_systemd_unit_active_on_our_port(tmp_path): + fake_bin = tmp_path / "bin" + fake_bin.mkdir() + _write_fake_systemctl(fake_bin, "active", main_pid=4242) + _write_fake_port_tools(fake_bin, pid_listens=True) + + result = run_ctl( + tmp_path, + "start", + env=_guard_env(fake_bin), + timeout=15, + ) + combined = result.stdout + result.stderr + assert result.returncode == 2, combined + assert "4242" in combined + assert not (tmp_path / ".hermes" / "webui.pid").exists() + + +@pytest.mark.skipif(sys.platform == "win32", reason="systemd is a Linux/POSIX path") +def test_start_allows_alternate_port_while_systemd_unit_auto_restarts(tmp_path): + """Mirror of the launchd #3291 over-block fix: an auto-restarting default + unit must not block a test instance on a different port.""" + fake_bin = tmp_path / "bin" + fake_bin.mkdir() + _write_fake_systemctl(fake_bin, "activating") + + fake_python = tmp_path / "fake-python" + fake_log = tmp_path / "fake-python.log" + write_fake_python(fake_python) + + port = _free_port() + started_pid = None + try: + result = run_ctl( + tmp_path, + "start", + env=_guard_env( + fake_bin, + HERMES_WEBUI_PORT=str(port), + HERMES_WEBUI_PYTHON=str(fake_python), + FAKE_PYTHON_LOG=str(fake_log), + ), + timeout=15, + ) + combined = result.stdout + result.stderr + assert "Refusing to start" not in combined, combined + assert result.returncode == 0, combined + pid_file = tmp_path / ".hermes" / "webui.pid" + if pid_file.exists(): + started_pid = int(pid_file.read_text().strip()) + finally: + if started_pid: + try: + os.kill(started_pid, 9) + except ProcessLookupError: + pass + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX daemon guards") +def test_start_proceeds_when_systemd_unit_inactive(tmp_path): + fake_bin = tmp_path / "bin" + fake_bin.mkdir() + _write_fake_systemctl(fake_bin, "inactive") + + fake_python = tmp_path / "fake-python" + fake_log = tmp_path / "fake-python.log" + write_fake_python(fake_python) + + port = _free_port() + started_pid = None + try: + result = run_ctl( + tmp_path, + "start", + env=_guard_env( + fake_bin, + HERMES_WEBUI_PORT=str(port), + HERMES_WEBUI_PYTHON=str(fake_python), + FAKE_PYTHON_LOG=str(fake_log), + ), + timeout=15, + ) + combined = result.stdout + result.stderr + assert "Refusing to start" not in combined, combined + assert result.returncode == 0, combined + started_pid = int((tmp_path / ".hermes" / "webui.pid").read_text().strip()) + finally: + if started_pid: + try: + os.kill(started_pid, 9) + except ProcessLookupError: + pass + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX daemon guards") +def test_start_reports_failure_when_server_dies_during_startup(tmp_path): + """A server that exits ~0.5s in (import error, stolen port) must yield a + failure and no stale PID file — not '[ctl] Started'.""" + dying_python = tmp_path / "dying-python" + dying_python.write_text( + "#!/usr/bin/env bash\nsleep 0.5\nexit 1\n", + encoding="utf-8", + ) + dying_python.chmod(0o755) + + port = _free_port() + result = run_ctl( + tmp_path, + "start", + env=_guard_env( + HERMES_WEBUI_PORT=str(port), + HERMES_WEBUI_PYTHON=str(dying_python), + HERMES_WEBUI_CTL_ALLOW_SYSTEMD_CONFLICT="1", + ), + timeout=15, + ) + combined = result.stdout + result.stderr + assert result.returncode == 1, combined + assert "failed to stay running" in combined + assert "Started Hermes WebUI" not in combined + assert not (tmp_path / ".hermes" / "webui.pid").exists() + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX daemon guards") +def test_status_reports_unmanaged_running_instance(tmp_path): + port = _free_port() + server = _start_dummy_http_server(port) + try: + result = run_ctl( + tmp_path, + "status", + env=_guard_env(HERMES_WEBUI_PORT=str(port)), + timeout=15, + ) + combined = result.stdout + result.stderr + assert result.returncode == 0, combined + assert "running (not managed by ctl.sh)" in combined + assert "stopped" not in combined + finally: + _stop_proc(server) + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX daemon guards") +def test_stop_warns_about_unmanaged_instance_and_leaves_it_alone(tmp_path): + port = _free_port() + server = _start_dummy_http_server(port) + try: + result = run_ctl( + tmp_path, + "stop", + env=_guard_env(HERMES_WEBUI_PORT=str(port)), + timeout=15, + ) + combined = result.stdout + result.stderr + assert result.returncode == 0, combined + assert "NOT managed by ctl.sh" in combined + assert server.poll() is None, "ctl.sh stop must not kill a foreign server" + # The foreign server must still answer after stop returned. + with socket.create_connection(("127.0.0.1", port), timeout=1): + pass + finally: + _stop_proc(server) diff --git a/tests/test_ctl_script.py b/tests/test_ctl_script.py index 27ba3267..31e1bd44 100644 --- a/tests/test_ctl_script.py +++ b/tests/test_ctl_script.py @@ -269,6 +269,11 @@ def test_start_can_ignore_repo_dotenv_for_authoritative_test_env(tmp_path): "HERMES_WEBUI_PYTHON": str(fake_python), "FAKE_PYTHON_LOG": str(fake_log), "HERMES_WEBUI_CTL_ALLOW_LAUNCHD_CONFLICT": "1", + # This test exercises dotenv precedence on the DEFAULT port; keep + # it hermetic on developer machines where a real WebUI (systemd + # unit or manual run) is serving 8787. + "HERMES_WEBUI_CTL_ALLOW_SYSTEMD_CONFLICT": "1", + "HERMES_WEBUI_CTL_ALLOW_PORT_CONFLICT": "1", }, repo_root=repo_root, ) From 1272ccb4eb572a579d92690b3a013f6f9c30bf86 Mon Sep 17 00:00:00 2001 From: ai-ag2026 <261867348+ai-ag2026@users.noreply.github.com> Date: Sat, 11 Jul 2026 19:58:35 +0200 Subject: [PATCH 2/7] fix(ctl): harden the foreign-instance guards per review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four fixes from the Greptile review on #5944: - Bracket IPv6 literals in the probe target ('::1' -> '[::1]') so the URL-based responder checks don't silently miss a running instance. - Force direct connections in the local ownership probes: --noproxy '*' (curl) / --no-proxy (wget) in _port_answers_http, and neutralized proxy env around the startup-grace health probe — a configured http(s)_proxy would report the proxy instead of the port. - Clamp HERMES_WEBUI_START_GRACE=0 to the default: a zero window would skip startup monitoring entirely and restore the stale-PID behavior the window exists to prevent. - stop: warn about an unmanaged instance BEFORE deleting the state file — it carries the saved host/port binding the probe needs when the instance was started off-default. Co-Authored-By: Claude Fable 5 --- ctl.sh | 31 ++++-- tests/test_ctl_foreign_server_guard.py | 147 +++++++++++++++++++++++++ 2 files changed, 169 insertions(+), 9 deletions(-) diff --git a/ctl.sh b/ctl.sh index e062356f..f693aaee 100755 --- a/ctl.sh +++ b/ctl.sh @@ -440,10 +440,15 @@ _launchd_webui_pid() { _probe_target_host() { # A bind-all host (0.0.0.0 / ::) is not a connectable probe target; probe - # loopback instead (mirrors server.py _abort_if_already_serving). - case "${1:-}" in + # loopback instead (mirrors server.py _abort_if_already_serving). IPv6 + # literals must be bracketed for URL interpolation ("http://[::1]:8787"), + # or curl/wget reject the URL and an existing instance is missed. + local host="${1:-}" + case "${host}" in 0.0.0.0 | '' | ::) printf '127.0.0.1' ;; - *) printf '%s' "$1" ;; + \[*\]) printf '%s' "${host}" ;; + *:*) printf '[%s]' "${host}" ;; + *) printf '%s' "${host}" ;; esac } @@ -458,12 +463,14 @@ _port_answers_http() { url="${scheme}://${host}:${port}/health" if command -v curl >/dev/null 2>&1; then # No -f: an HTTP error status is still a responder. -k: a self-signed - # cert is still a responder. - curl -sS -o /dev/null -k --max-time 2 "${url}" 2>/dev/null + # cert is still a responder. --noproxy: this is a LOCAL ownership + # check — routed through an http(s)_proxy it would report the proxy, + # not the port (false block or, with a dead proxy, a missed conflict). + curl -sS -o /dev/null -k --noproxy '*' --max-time 2 "${url}" 2>/dev/null rc=$? [[ ${rc} -eq 0 ]] && return 0 elif command -v wget >/dev/null 2>&1; then - wget -qO /dev/null --no-check-certificate "--timeout=2" --tries=1 "${url}" 2>/dev/null + wget -qO /dev/null --no-check-certificate --no-proxy "--timeout=2" --tries=1 "${url}" 2>/dev/null rc=$? # 0 = OK; 8 = server answered with an error status — both are responders. [[ ${rc} -eq 0 || ${rc} -eq 8 ]] && return 0 @@ -606,6 +613,9 @@ start_cmd() { # on timeout we keep the optimistic legacy behavior and say so. local grace="${HERMES_WEBUI_START_GRACE:-3}" [[ "${grace}" =~ ^[0-9]+$ ]] || grace=3 + # 0 would skip startup monitoring entirely and restore the stale-PID + # behavior this window exists to prevent; treat it like any invalid value. + (( grace > 0 )) || grace=3 local grace_steps=$(( grace * 4 )) step=0 healthy=0 while (( step < grace_steps )); do if ! _is_alive "${pid}"; then @@ -613,7 +623,8 @@ start_cmd() { rm -f "${PID_FILE}" "${STATE_FILE}" return 1 fi - if hermes_webui_probe_health "${probe_host}" "${CTL_PORT}" "/health" 1 >/dev/null 2>&1; then + if http_proxy='' https_proxy='' HTTP_PROXY='' HTTPS_PROXY='' \ + hermes_webui_probe_health "${probe_host}" "${CTL_PORT}" "/health" 1 >/dev/null 2>&1; then healthy=1 break fi @@ -657,14 +668,16 @@ stop_cmd() { local pid if ! pid="$(_pid_from_file 2>/dev/null)"; then echo "[ctl] Hermes WebUI is stopped" - rm -f "${PID_FILE}" "${STATE_FILE}" + # Warn BEFORE deleting the state file: it carries the saved host/port + # binding the probe needs when the instance was started off-default. _warn_if_unmanaged_instance_serving + rm -f "${PID_FILE}" "${STATE_FILE}" return 0 fi if ! _is_alive "${pid}" || ! _is_owned_webui_pid "${pid}"; then - _clear_stale_pid _warn_if_unmanaged_instance_serving + _clear_stale_pid return 0 fi diff --git a/tests/test_ctl_foreign_server_guard.py b/tests/test_ctl_foreign_server_guard.py index c0d05e5b..6fe402c8 100644 --- a/tests/test_ctl_foreign_server_guard.py +++ b/tests/test_ctl_foreign_server_guard.py @@ -365,3 +365,150 @@ def test_stop_warns_about_unmanaged_instance_and_leaves_it_alone(tmp_path): pass finally: _stop_proc(server) + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX daemon guards") +def test_port_guard_ignores_http_proxy_env(tmp_path): + """The ownership probe must force a direct connection: a configured + http_proxy would report the proxy, not the local port (Greptile P1 on + #5944) — a dead proxy hides an occupied port and start proceeds doomed.""" + port = _free_port() + server = _start_dummy_http_server(port) + try: + result = run_ctl( + tmp_path, + "start", + env=_guard_env( + HERMES_WEBUI_PORT=str(port), + HERMES_WEBUI_CTL_ALLOW_SYSTEMD_CONFLICT="1", + # Dead proxy: any probe routed through it sees no responder. + http_proxy="http://127.0.0.1:1", + https_proxy="http://127.0.0.1:1", + HTTP_PROXY="http://127.0.0.1:1", + HTTPS_PROXY="http://127.0.0.1:1", + ), + timeout=15, + ) + combined = result.stdout + result.stderr + assert result.returncode == 2, combined + assert "already responding" in combined + finally: + _stop_proc(server) + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX daemon guards") +def test_ipv6_host_probe_builds_bracketed_url(tmp_path): + """HERMES_WEBUI_HOST='::1' must probe http://[::1]:port — the unbracketed + literal is rejected by curl/wget and the running instance is missed.""" + if not socket.has_ipv6: + pytest.skip("no IPv6 support") + port = _free_port() + code = textwrap.dedent( + f""" + import socket + from http.server import BaseHTTPRequestHandler, HTTPServer + + class V6Server(HTTPServer): + address_family = socket.AF_INET6 + + class H(BaseHTTPRequestHandler): + def do_GET(self): + body = b'{{"status": "ok"}}' + self.send_response(200) + self.send_header("Content-Length", str(len(body))) + self.end_headers() + self.wfile.write(body) + + def log_message(self, *args): + pass + + V6Server(("::1", {port}), H).serve_forever() + """ + ) + try: + server = subprocess.Popen([sys.executable, "-c", code]) + except OSError: + pytest.skip("cannot bind ::1") + try: + deadline = time.time() + 5 + up = False + while time.time() < deadline: + if server.poll() is not None: + pytest.skip("IPv6 loopback unavailable") + try: + with socket.create_connection(("::1", port), timeout=0.2): + up = True + break + except OSError: + time.sleep(0.05) + assert up, "IPv6 dummy server did not come up" + + result = run_ctl( + tmp_path, + "start", + env=_guard_env( + HERMES_WEBUI_HOST="::1", + HERMES_WEBUI_PORT=str(port), + HERMES_WEBUI_CTL_ALLOW_SYSTEMD_CONFLICT="1", + ), + timeout=15, + ) + combined = result.stdout + result.stderr + assert result.returncode == 2, combined + assert "already responding" in combined + finally: + _stop_proc(server) + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX daemon guards") +def test_zero_start_grace_still_monitors_startup(tmp_path): + """HERMES_WEBUI_START_GRACE=0 must not disable the startup watch — that + would restore the exact stale-PID-on-early-death behavior being fixed.""" + dying_python = tmp_path / "dying-python" + dying_python.write_text( + "#!/usr/bin/env bash\nsleep 0.5\nexit 1\n", + encoding="utf-8", + ) + dying_python.chmod(0o755) + + port = _free_port() + result = run_ctl( + tmp_path, + "start", + env=_guard_env( + HERMES_WEBUI_PORT=str(port), + HERMES_WEBUI_PYTHON=str(dying_python), + HERMES_WEBUI_CTL_ALLOW_SYSTEMD_CONFLICT="1", + HERMES_WEBUI_START_GRACE="0", + ), + timeout=15, + ) + combined = result.stdout + result.stderr + assert result.returncode == 1, combined + assert "failed to stay running" in combined + assert not (tmp_path / ".hermes" / "webui.pid").exists() + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX daemon guards") +def test_stop_warns_using_saved_binding_from_state_file(tmp_path): + """With no PID file but a saved off-default binding in the state file, + stop must probe THAT binding (before deleting the file) and still warn + about the unmanaged instance.""" + port = _free_port() + server = _start_dummy_http_server(port) + hermes_home = tmp_path / ".hermes" + hermes_home.mkdir() + (hermes_home / "webui.ctl.env").write_text( + f"PID=999999\nHOST=127.0.0.1\nPORT={port}\n", + encoding="utf-8", + ) + try: + # No HERMES_WEBUI_PORT in the environment: the probe target must come + # from the saved state file. + result = run_ctl(tmp_path, "stop", env=_guard_env(), timeout=15) + combined = result.stdout + result.stderr + assert result.returncode == 0, combined + assert "NOT managed by ctl.sh" in combined + assert server.poll() is None + finally: + _stop_proc(server) From ee7af858845695c370f60789b0ad62eea450248e Mon Sep 17 00:00:00 2001 From: ai-ag2026 <261867348+ai-ag2026@users.noreply.github.com> Date: Sun, 12 Jul 2026 02:34:24 +0200 Subject: [PATCH 3/7] fix(ctl): keep listener diagnostics best-effort --- ctl.sh | 5 +++- tests/test_ctl_foreign_server_guard.py | 34 ++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/ctl.sh b/ctl.sh index f693aaee..fe26185f 100755 --- a/ctl.sh +++ b/ctl.sh @@ -491,7 +491,10 @@ _port_listener_diag() { if [[ -z "${line}" ]] && command -v lsof >/dev/null 2>&1; then line="$(lsof -nP -iTCP:"${port}" -sTCP:LISTEN 2>/dev/null | awk 'NR==2 {print; exit}')" fi - [[ -n "${line}" ]] && printf '%s' "${line}" + if [[ -n "${line}" ]]; then + printf '%s' "${line}" + fi + return 0 } _systemd_webui_conflict() { diff --git a/tests/test_ctl_foreign_server_guard.py b/tests/test_ctl_foreign_server_guard.py index 6fe402c8..b095cf05 100644 --- a/tests/test_ctl_foreign_server_guard.py +++ b/tests/test_ctl_foreign_server_guard.py @@ -345,6 +345,40 @@ def test_status_reports_unmanaged_running_instance(tmp_path): _stop_proc(server) +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX daemon guards") +@pytest.mark.parametrize( + ("command", "expected_output"), + [ + ("status", "running (not managed by ctl.sh)"), + ("stop", "NOT managed by ctl.sh"), + ], +) +def test_unmanaged_instance_commands_survive_missing_listener_diagnostics( + tmp_path, command, expected_output +): + """Best-effort listener diagnostics must not abort status/stop under set -e + when ss/lsof are unavailable or return no matching listener row.""" + fake_bin = tmp_path / "bin" + fake_bin.mkdir() + _write_fake_port_tools(fake_bin, pid_listens=False) + port = _free_port() + server = _start_dummy_http_server(port) + try: + result = run_ctl( + tmp_path, + command, + env=_guard_env(fake_bin, HERMES_WEBUI_PORT=str(port)), + timeout=15, + ) + combined = result.stdout + result.stderr + assert result.returncode == 0, combined + assert expected_output in combined + assert "Listener:" not in combined + assert server.poll() is None, "ctl.sh must leave a foreign server untouched" + finally: + _stop_proc(server) + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX daemon guards") def test_stop_warns_about_unmanaged_instance_and_leaves_it_alone(tmp_path): port = _free_port() From 7ab9f6ee5c1caa8eeba5a38564a75446cf242d46 Mon Sep 17 00:00:00 2001 From: ai-ag2026 <261867348+ai-ag2026@users.noreply.github.com> Date: Sat, 18 Jul 2026 10:39:37 +0200 Subject: [PATCH 4/7] fix: bypass proxies in ctl startup health probe --- ctl.sh | 3 +- scripts/lib/health_probe.sh | 46 ++++++++++++------ tests/test_ctl_foreign_server_guard.py | 67 ++++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 16 deletions(-) diff --git a/ctl.sh b/ctl.sh index fe26185f..95164be8 100755 --- a/ctl.sh +++ b/ctl.sh @@ -626,8 +626,7 @@ start_cmd() { rm -f "${PID_FILE}" "${STATE_FILE}" return 1 fi - if http_proxy='' https_proxy='' HTTP_PROXY='' HTTPS_PROXY='' \ - hermes_webui_probe_health "${probe_host}" "${CTL_PORT}" "/health" 1 >/dev/null 2>&1; then + if hermes_webui_probe_health "${probe_host}" "${CTL_PORT}" "/health" 1 direct >/dev/null 2>&1; then healthy=1 break fi diff --git a/scripts/lib/health_probe.sh b/scripts/lib/health_probe.sh index 89141231..477adbaa 100644 --- a/scripts/lib/health_probe.sh +++ b/scripts/lib/health_probe.sh @@ -58,31 +58,49 @@ _hermes_webui_warn_self_signed() { "$1" >&2 } -# _hermes_webui_http_get +# _hermes_webui_http_get [direct] # mode: "insecure" disables certificate verification, anything else verifies. +# direct: "direct" bypasses every configured proxy. # Prints the response body to stdout; returns the underlying client exit code. _hermes_webui_http_get() { - local url="$1" max_time="$2" mode="$3" + local url="$1" max_time="$2" mode="$3" direct="${4:-}" if command -v curl >/dev/null 2>&1; then if [[ "${mode}" == "insecure" ]]; then - curl -fsS -k --max-time "${max_time}" "${url}" 2>/dev/null + if [[ "${direct}" == "direct" ]]; then + curl -fsS -k --noproxy '*' --max-time "${max_time}" "${url}" 2>/dev/null + else + curl -fsS -k --max-time "${max_time}" "${url}" 2>/dev/null + fi else - curl -fsS --max-time "${max_time}" "${url}" 2>/dev/null + if [[ "${direct}" == "direct" ]]; then + curl -fsS --noproxy '*' --max-time "${max_time}" "${url}" 2>/dev/null + else + curl -fsS --max-time "${max_time}" "${url}" 2>/dev/null + fi fi return $? elif command -v wget >/dev/null 2>&1; then if [[ "${mode}" == "insecure" ]]; then - wget -qO- --no-check-certificate "--timeout=${max_time}" --tries=1 "${url}" 2>/dev/null + if [[ "${direct}" == "direct" ]]; then + wget -qO- --no-proxy --no-check-certificate "--timeout=${max_time}" --tries=1 "${url}" 2>/dev/null + else + wget -qO- --no-check-certificate "--timeout=${max_time}" --tries=1 "${url}" 2>/dev/null + fi else - wget -qO- "--timeout=${max_time}" --tries=1 "${url}" 2>/dev/null + if [[ "${direct}" == "direct" ]]; then + wget -qO- --no-proxy "--timeout=${max_time}" --tries=1 "${url}" 2>/dev/null + else + wget -qO- "--timeout=${max_time}" --tries=1 "${url}" 2>/dev/null + fi fi return $? fi return 127 } -# hermes_webui_probe_health [path] [max_time] -# Prints the response body to stdout on success; warnings go to stderr. +# hermes_webui_probe_health [path] [max_time] [direct] +# Pass "direct" to bypass every configured proxy for all probe attempts. +# Prints the response body to stdout; warnings go to stderr. # Returns 0 if the server answered, 1 otherwise. # # Side effect: sets the global _HERMES_WEBUI_PROBE_SCHEME to the scheme that @@ -91,14 +109,14 @@ _hermes_webui_http_get() { # back to plain HTTP when the cert/key are unloadable — so the configured # scheme can be https:// while the live server speaks http://. hermes_webui_probe_health() { - local host="$1" port="$2" path="${3:-/health}" max_time="${4:-2}" + local host="$1" port="$2" path="${3:-/health}" max_time="${4:-2}" direct="${5:-}" local scheme body scheme="$(hermes_webui_probe_scheme)" local http_url="http://${host}:${port}${path}" if [[ "${scheme}" == "http" ]]; then - if body="$(_hermes_webui_http_get "${http_url}" "${max_time}" "")"; then + if body="$(_hermes_webui_http_get "${http_url}" "${max_time}" "" "${direct}")"; then _HERMES_WEBUI_PROBE_SCHEME="http" printf '%s' "${body}" return 0 @@ -111,20 +129,20 @@ hermes_webui_probe_health() { if _hermes_webui_truthy "${HERMES_WEBUI_TLS_INSECURE_PROBE:-}"; then # Explicit opt-in: skip verification, stay silent by contract. - if body="$(_hermes_webui_http_get "${https_url}" "${max_time}" "insecure")"; then + if body="$(_hermes_webui_http_get "${https_url}" "${max_time}" "insecure" "${direct}")"; then _HERMES_WEBUI_PROBE_SCHEME="https" printf '%s' "${body}" return 0 fi else # 1) Verified HTTPS. - if body="$(_hermes_webui_http_get "${https_url}" "${max_time}" "")"; then + if body="$(_hermes_webui_http_get "${https_url}" "${max_time}" "" "${direct}")"; then _HERMES_WEBUI_PROBE_SCHEME="https" printf '%s' "${body}" return 0 fi # 2) Self-signed fallback: verification failed, retry unverified + warn. - if body="$(_hermes_webui_http_get "${https_url}" "${max_time}" "insecure")"; then + if body="$(_hermes_webui_http_get "${https_url}" "${max_time}" "insecure" "${direct}")"; then _hermes_webui_warn_self_signed "${https_url}" _HERMES_WEBUI_PROBE_SCHEME="https" printf '%s' "${body}" @@ -133,7 +151,7 @@ hermes_webui_probe_health() { fi # 3) server.py may have fallen back to plain HTTP (cert/key unloadable). - if body="$(_hermes_webui_http_get "${http_url}" "${max_time}" "")"; then + if body="$(_hermes_webui_http_get "${http_url}" "${max_time}" "" "${direct}")"; then _HERMES_WEBUI_PROBE_SCHEME="http" printf '%s' "${body}" return 0 diff --git a/tests/test_ctl_foreign_server_guard.py b/tests/test_ctl_foreign_server_guard.py index b095cf05..3a24ff8e 100644 --- a/tests/test_ctl_foreign_server_guard.py +++ b/tests/test_ctl_foreign_server_guard.py @@ -430,6 +430,73 @@ def test_port_guard_ignores_http_proxy_env(tmp_path): _stop_proc(server) +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX daemon guards") +def test_start_health_probe_ignores_all_proxy_env(tmp_path): + """The startup watch must reach its own local health server directly. + + Unlike the preflight ownership guard above, this drives the post-launch + health loop: a dead ALL_PROXY must not make ctl wait out its grace period + and print the misleading health-timeout note. + """ + port = _free_port() + health_server = tmp_path / "health-server" + health_server.write_text( + textwrap.dedent( + """ + #!/usr/bin/env python3 + import sys + from http.server import BaseHTTPRequestHandler, HTTPServer + + class HealthHandler(BaseHTTPRequestHandler): + def do_GET(self): + body = b'{"status": "ok"}' + self.send_response(200) + self.send_header("Content-Length", str(len(body))) + self.end_headers() + self.wfile.write(body) + + def log_message(self, *args): + pass + + HTTPServer(("127.0.0.1", int(sys.argv[-1])), HealthHandler).serve_forever() + """ + ).lstrip(), + encoding="utf-8", + ) + health_server.chmod(0o755) + + started_pid = None + try: + result = run_ctl( + tmp_path, + "start", + env=_guard_env( + HERMES_WEBUI_PORT=str(port), + HERMES_WEBUI_PYTHON=str(health_server), + HERMES_WEBUI_CTL_ALLOW_SYSTEMD_CONFLICT="1", + # A live local server must still win when both proxy spellings + # route clients to a dead endpoint. + ALL_PROXY="http://127.0.0.1:1", + all_proxy="http://127.0.0.1:1", + NO_PROXY="", + no_proxy="", + ), + timeout=15, + ) + combined = result.stdout + result.stderr + pid_file = tmp_path / ".hermes" / "webui.pid" + if pid_file.exists(): + started_pid = int(pid_file.read_text().strip()) + assert result.returncode == 0, combined + assert "/health did not respond" not in combined + finally: + if started_pid: + try: + os.kill(started_pid, 9) + except ProcessLookupError: + pass + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX daemon guards") def test_ipv6_host_probe_builds_bracketed_url(tmp_path): """HERMES_WEBUI_HOST='::1' must probe http://[::1]:port — the unbracketed From 5d3b76bd13aae341fa2c9635a33a4793f5c5e25d Mon Sep 17 00:00:00 2001 From: ai-ag2026 <261867348+ai-ag2026@users.noreply.github.com> Date: Sat, 18 Jul 2026 10:43:05 +0200 Subject: [PATCH 5/7] test: allow startup monitor cleanup in dotenv test --- tests/test_ctl_script.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/test_ctl_script.py b/tests/test_ctl_script.py index 31e1bd44..ebfe6fbd 100644 --- a/tests/test_ctl_script.py +++ b/tests/test_ctl_script.py @@ -354,6 +354,9 @@ def test_start_loads_dotenv_double_quoted_port_with_trailing_comment(tmp_path): }, repo_root=repo_root, load_dotenv=True, + # The legitimate three-second startup monitor plus suite cleanup can + # exceed run_ctl's narrow default budget on a loaded test worker. + timeout=15, ) assert result.returncode == 0, result.stderr + result.stdout From 96bb78b1c16bb9b08b320f80f6037b8c7545c047 Mon Sep 17 00:00:00 2001 From: ai-ag2026 <261867348+ai-ag2026@users.noreply.github.com> Date: Sat, 18 Jul 2026 10:46:23 +0200 Subject: [PATCH 6/7] test: give ctl start fixtures startup grace --- tests/test_ctl_script.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/test_ctl_script.py b/tests/test_ctl_script.py index ebfe6fbd..47b3b437 100644 --- a/tests/test_ctl_script.py +++ b/tests/test_ctl_script.py @@ -27,7 +27,7 @@ def run_ctl( home: Path, *args: str, env: dict[str, str] | None = None, - timeout: float = 5.0, + timeout: float = 15.0, repo_root: Path = REPO_ROOT, load_dotenv: bool = False, ): @@ -354,9 +354,6 @@ def test_start_loads_dotenv_double_quoted_port_with_trailing_comment(tmp_path): }, repo_root=repo_root, load_dotenv=True, - # The legitimate three-second startup monitor plus suite cleanup can - # exceed run_ctl's narrow default budget on a loaded test worker. - timeout=15, ) assert result.returncode == 0, result.stderr + result.stdout From a248aa52de157abc11f3995b627759bb2a2cb4cd Mon Sep 17 00:00:00 2001 From: ai-ag2026 <261867348+ai-ag2026@users.noreply.github.com> Date: Sat, 18 Jul 2026 20:47:03 +0200 Subject: [PATCH 7/7] fix(ctl): close all three 12.07 re-gate findings on the systemd guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. inherit_errexit: the listener-diagnostic assignments in _port_listener_diag abort status/stop when errexit is inherited into command substitutions (shopt inherit_errexit or a BASHOPTS env from the invoking shell). Guard both assignments with || true; regression test runs status/stop under BASHOPTS=inherit_errexit. 2. PID/port AND: _pid_listens_on_port called lsof -p PID -iTCP:PORT without -a, which OR-combines the selectors — any socket of the PID or any listener on the port matched, so an active unit could be blamed for a port its MainPID does not listen on. Add -a; the new fake lsof mimics real OR/AND semantics so a missing -a fails the test. 3. Binding-aware collision: an active/activating unit whose ownership could not be attributed via MainPID was assumed to own port 8787 unconditionally. Resolve the unit's configured binding first (HERMES_WEBUI_PORT from Environment=, then --port from ExecStart=) and refuse only on actual overlap; the default-port guard remains solely for undeterminable bindings (#3291 semantics). Tests cover the reverse alternate-port case (unit on 9999, start on 8787 proceeds) and the overlap case (unit on the requested port refuses). Co-Authored-By: Claude Fable 5 --- ctl.sh | 51 ++++++- tests/test_ctl_foreign_server_guard.py | 189 ++++++++++++++++++++++++- 2 files changed, 231 insertions(+), 9 deletions(-) diff --git a/ctl.sh b/ctl.sh index 95164be8..f4a1a426 100755 --- a/ctl.sh +++ b/ctl.sh @@ -385,7 +385,7 @@ _pid_listens_on_port() { local pid="$1" port="$2" [[ "${pid}" =~ ^[0-9]+$ && "${port}" =~ ^[0-9]+$ ]] || return 2 if command -v lsof >/dev/null 2>&1; then - if lsof -nP -p "${pid}" -iTCP:"${port}" -sTCP:LISTEN >/dev/null 2>&1; then + if lsof -nP -a -p "${pid}" -iTCP:"${port}" -sTCP:LISTEN >/dev/null 2>&1; then return 0 # PID is listening on that port → real conflict fi return 1 # PID is alive but NOT listening on that port → no conflict @@ -485,11 +485,14 @@ _port_listener_diag() { # Best-effort one-liner describing what listens on TCP port $1. Used in # error/status messages only — never fails the caller. local port="$1" line="" + # The || true keeps the assignments non-failing when errexit is inherited + # into command substitutions (shopt inherit_errexit, or a BASHOPTS env from + # the invoking shell) — without it a no-match awk/ss aborts status/stop. if command -v ss >/dev/null 2>&1; then - line="$(ss -tlnp 2>/dev/null | awk -v p="${port}" '$4 ~ (":" p "$") {print; exit}')" + line="$(ss -tlnp 2>/dev/null | awk -v p="${port}" '$4 ~ (":" p "$") {print; exit}')" || true fi if [[ -z "${line}" ]] && command -v lsof >/dev/null 2>&1; then - line="$(lsof -nP -iTCP:"${port}" -sTCP:LISTEN 2>/dev/null | awk 'NR==2 {print; exit}')" + line="$(lsof -nP -iTCP:"${port}" -sTCP:LISTEN 2>/dev/null | awk 'NR==2 {print; exit}')" || true fi if [[ -n "${line}" ]]; then printf '%s' "${line}" @@ -497,6 +500,25 @@ _port_listener_diag() { return 0 } +_systemd_unit_effective_port() { + # Best-effort resolution of the port a systemd unit is configured to bind: + # HERMES_WEBUI_PORT in its Environment=, then an explicit --port on its + # ExecStart=. Prints nothing when the binding cannot be determined so the + # caller can fall back to the conservative default-port guard. + local scope="$1" unit="$2" env_block="" exec_block="" port="" + env_block="$(systemctl "${scope}" show -p Environment --value "${unit}" 2>/dev/null)" || true + if [[ "${env_block}" =~ HERMES_WEBUI_PORT=([0-9]+) ]]; then + port="${BASH_REMATCH[1]}" + fi + if [[ -z "${port}" ]]; then + exec_block="$(systemctl "${scope}" show -p ExecStart --value "${unit}" 2>/dev/null)" || true + if [[ "${exec_block}" =~ --port[=[:space:]]([0-9]+) ]]; then + port="${BASH_REMATCH[1]}" + fi + fi + printf '%s' "${port}" +} + _systemd_webui_conflict() { # Linux analog of _launchd_webui_pid: echo a short conflict descriptor and # return 0 when a systemd unit (default hermes-webui.service, override via @@ -516,7 +538,7 @@ _systemd_webui_conflict() { local unit="${HERMES_WEBUI_SYSTEMD_UNIT:-hermes-webui.service}" [[ -n "${unit}" ]] || return 1 local want_port="${CTL_PORT:-${HERMES_WEBUI_PORT:-8787}}" - local scope state main_pid + local scope state main_pid unit_port for scope in --system --user; do state="$(systemctl "${scope}" show -p ActiveState --value "${unit}" 2>/dev/null)" || continue case "${state}" in @@ -530,14 +552,29 @@ _systemd_webui_conflict() { 1) continue ;; # active on a different port → no conflict esac fi - # Port ownership unknown: guard only the default port (see #3291). - if [[ "${want_port}" == "8787" ]]; then + # Port ownership unknown via PID: resolve the unit's CONFIGURED + # binding and refuse only on actual overlap. Only when the binding + # cannot be determined fall back to guarding the default port + # (see #3291) so alternate-port instances are never wrongly blocked. + unit_port="$(_systemd_unit_effective_port "${scope}" "${unit}")" || true + if [[ -n "${unit_port}" ]]; then + if [[ "${unit_port}" == "${want_port}" ]]; then + printf 'unit %s is active (configured for port %s)' "${unit}" "${unit_port}" + return 0 + fi + elif [[ "${want_port}" == "8787" ]]; then printf 'unit %s is active' "${unit}" return 0 fi ;; activating | reloading) - if [[ "${want_port}" == "8787" ]]; then + unit_port="$(_systemd_unit_effective_port "${scope}" "${unit}")" || true + if [[ -n "${unit_port}" ]]; then + if [[ "${unit_port}" == "${want_port}" ]]; then + printf 'unit %s is %s (auto-restart pending on port %s)' "${unit}" "${state}" "${unit_port}" + return 0 + fi + elif [[ "${want_port}" == "8787" ]]; then printf 'unit %s is %s (auto-restart pending)' "${unit}" "${state}" return 0 fi diff --git a/tests/test_ctl_foreign_server_guard.py b/tests/test_ctl_foreign_server_guard.py index 3a24ff8e..fd1f219c 100644 --- a/tests/test_ctl_foreign_server_guard.py +++ b/tests/test_ctl_foreign_server_guard.py @@ -83,8 +83,15 @@ def _stop_proc(proc: subprocess.Popen) -> None: proc.kill() -def _write_fake_systemctl(fake_bin: Path, active_state: str, main_pid: int = 0) -> None: - """Fake systemctl answering `show -p ActiveState|MainPID --value `.""" +def _write_fake_systemctl( + fake_bin: Path, + active_state: str, + main_pid: int = 0, + environment: str = "", + exec_start: str = "", +) -> None: + """Fake systemctl answering `show -p --value ` for the + ActiveState/MainPID/Environment/ExecStart properties the guard inspects.""" systemctl = fake_bin / "systemctl" systemctl.write_text( textwrap.dedent( @@ -94,6 +101,8 @@ def _write_fake_systemctl(fake_bin: Path, active_state: str, main_pid: int = 0) case "${{arg}}" in *ActiveState*) echo "{active_state}"; exit 0 ;; *MainPID*) echo "{main_pid}"; exit 0 ;; + *Environment*) echo "{environment}"; exit 0 ;; + *ExecStart*) echo "{exec_start}"; exit 0 ;; esac done exit 0 @@ -104,6 +113,32 @@ def _write_fake_systemctl(fake_bin: Path, active_state: str, main_pid: int = 0) systemctl.chmod(0o755) +def _write_fake_lsof_with_real_and_semantics(fake_bin: Path) -> None: + """Fake lsof mimicking real -a semantics: without -a, `-p PID -iTCP:PORT` + OR-combines its selectors (matching whenever EITHER hits); with -a they + AND. Exit codes come from FAKE_LSOF_OR_EXIT / FAKE_LSOF_AND_EXIT so a test + can model 'the pid is alive with sockets, but nothing of it listens on the + requested port' — the case a missing -a falsely reports as a conflict.""" + lsof = fake_bin / "lsof" + lsof.write_text( + textwrap.dedent( + """ + #!/usr/bin/env bash + has_a=0 + for arg in "$@"; do + [[ "${arg}" == "-a" ]] && has_a=1 + done + if [[ "${has_a}" == 1 ]]; then + exit "${FAKE_LSOF_AND_EXIT:-1}" + fi + exit "${FAKE_LSOF_OR_EXIT:-0}" + """ + ).lstrip(), + encoding="utf-8", + ) + lsof.chmod(0o755) + + def _write_fake_port_tools(fake_bin: Path, pid_listens: bool) -> None: """Fake lsof/ss so _pid_listens_on_port resolves deterministically.""" lsof = fake_bin / "lsof" @@ -261,6 +296,156 @@ def test_start_allows_alternate_port_while_systemd_unit_auto_restarts(tmp_path): pass +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX daemon guards") +def test_systemd_pid_port_check_requires_and_semantics(tmp_path): + """lsof without -a OR-combines -p/-i, so an active unit whose MainPID has + sockets but does NOT listen on the requested port must not be mistaken + for a conflict (12.07 re-gate finding 2).""" + fake_bin = tmp_path / "bin" + fake_bin.mkdir() + _write_fake_systemctl(fake_bin, "active", main_pid=4242) + _write_fake_lsof_with_real_and_semantics(fake_bin) + + fake_python = tmp_path / "fake-python" + fake_log = tmp_path / "fake-python.log" + write_fake_python(fake_python) + + port = _free_port() + started_pid = None + try: + result = run_ctl( + tmp_path, + "start", + env=_guard_env( + fake_bin, + HERMES_WEBUI_PORT=str(port), + HERMES_WEBUI_PYTHON=str(fake_python), + FAKE_PYTHON_LOG=str(fake_log), + FAKE_LSOF_OR_EXIT="0", + FAKE_LSOF_AND_EXIT="1", + ), + timeout=15, + ) + combined = result.stdout + result.stderr + assert "Refusing to start" not in combined, combined + assert result.returncode == 0, combined + pid_file = tmp_path / ".hermes" / "webui.pid" + if pid_file.exists(): + started_pid = int(pid_file.read_text().strip()) + finally: + if started_pid: + try: + os.kill(started_pid, 9) + except ProcessLookupError: + pass + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX daemon guards") +def test_start_on_default_port_allowed_when_unit_configured_elsewhere(tmp_path): + """Reverse alternate-port case (12.07 re-gate finding 3): a unit whose + CONFIGURED binding is a non-default port must not block a start on 8787 + just because its ownership cannot be attributed via MainPID.""" + with socket.socket() as probe: + if probe.connect_ex(("127.0.0.1", 8787)) == 0: + pytest.skip("default port 8787 already occupied on this host") + fake_bin = tmp_path / "bin" + fake_bin.mkdir() + _write_fake_systemctl( + fake_bin, "activating", environment="HERMES_WEBUI_PORT=9999" + ) + + fake_python = tmp_path / "fake-python" + fake_log = tmp_path / "fake-python.log" + write_fake_python(fake_python) + + started_pid = None + try: + result = run_ctl( + tmp_path, + "start", + env=_guard_env( + fake_bin, + HERMES_WEBUI_PORT="8787", + HERMES_WEBUI_PYTHON=str(fake_python), + FAKE_PYTHON_LOG=str(fake_log), + ), + timeout=15, + ) + combined = result.stdout + result.stderr + assert "Refusing to start" not in combined, combined + assert result.returncode == 0, combined + pid_file = tmp_path / ".hermes" / "webui.pid" + if pid_file.exists(): + started_pid = int(pid_file.read_text().strip()) + finally: + if started_pid: + try: + os.kill(started_pid, 9) + except ProcessLookupError: + pass + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX daemon guards") +def test_start_refuses_when_unit_configured_for_requested_alternate_port(tmp_path): + """Counterpart of the reverse case: when the unit's configured binding + matches the requested (non-default) port, the conflict must be reported.""" + fake_bin = tmp_path / "bin" + fake_bin.mkdir() + port = _free_port() + _write_fake_systemctl( + fake_bin, "activating", environment=f"HERMES_WEBUI_PORT={port}" + ) + + result = run_ctl( + tmp_path, + "start", + env=_guard_env(fake_bin, HERMES_WEBUI_PORT=str(port)), + timeout=15, + ) + combined = result.stdout + result.stderr + assert "Refusing to start" in combined, combined + assert result.returncode != 0, combined + assert str(port) in combined + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX daemon guards") +@pytest.mark.parametrize( + ("command", "expected_output"), + [ + ("status", "running (not managed by ctl.sh)"), + ("stop", "NOT managed by ctl.sh"), + ], +) +def test_unmanaged_instance_commands_survive_inherit_errexit( + tmp_path, command, expected_output +): + """12.07 re-gate finding 1: with errexit inherited into command + substitutions (BASHOPTS=inherit_errexit from the invoking shell), the + no-match listener diagnostics must still not abort status/stop.""" + fake_bin = tmp_path / "bin" + fake_bin.mkdir() + _write_fake_port_tools(fake_bin, pid_listens=False) + port = _free_port() + server = _start_dummy_http_server(port) + try: + result = run_ctl( + tmp_path, + command, + env=_guard_env( + fake_bin, + HERMES_WEBUI_PORT=str(port), + BASHOPTS="inherit_errexit", + ), + timeout=15, + ) + combined = result.stdout + result.stderr + assert result.returncode == 0, combined + assert expected_output in combined + assert server.poll() is None, "ctl.sh must leave a foreign server untouched" + finally: + _stop_proc(server) + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX daemon guards") def test_start_proceeds_when_systemd_unit_inactive(tmp_path): fake_bin = tmp_path / "bin"