diff --git a/ctl.sh b/ctl.sh index 2e827aad..f4a1a426 100755 --- a/ctl.sh +++ b/ctl.sh @@ -385,11 +385,25 @@ _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 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,152 @@ _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). 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' "${host}" ;; + *:*) printf '[%s]' "${host}" ;; + *) printf '%s' "${host}" ;; + 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. --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 --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 + 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="" + # 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}')" || 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}')" || true + fi + if [[ -n "${line}" ]]; then + printf '%s' "${line}" + fi + 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 + # 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 unit_port + 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 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) + 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 + ;; + esac + done + return 1 +} + start_cmd() { ensure_home _load_repo_dotenv_preserving_env @@ -446,6 +606,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 +643,33 @@ 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 + # 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 + 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 direct >/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 +678,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() { @@ -477,11 +707,15 @@ stop_cmd() { local pid if ! pid="$(_pid_from_file 2>/dev/null)"; then echo "[ctl] Hermes WebUI is stopped" + # 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 + _warn_if_unmanaged_instance_serving _clear_stale_pid return 0 fi @@ -551,11 +785,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/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 new file mode 100644 index 00000000..fd1f219c --- /dev/null +++ b/tests/test_ctl_foreign_server_guard.py @@ -0,0 +1,800 @@ +"""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, + 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( + f""" + #!/usr/bin/env bash + for arg in "$@"; do + 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 + """ + ).lstrip(), + encoding="utf-8", + ) + 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" + 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_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" + 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") +@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() + 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) + + +@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_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 + 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) diff --git a/tests/test_ctl_script.py b/tests/test_ctl_script.py index 27ba3267..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, ): @@ -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, )