fix(ctl): harden the foreign-instance guards per review
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 <noreply@anthropic.com>
This commit is contained in:
ai-ag2026
parent
6f10e4d48b
commit
1272ccb4eb
2 changed files with 169 additions and 9 deletions
31
ctl.sh
31
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
|
||||
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
Loading…
Reference in a new issue