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] 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)