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"