fix(ctl): close all three 12.07 re-gate findings on the systemd guard

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 <noreply@anthropic.com>
This commit is contained in:
ai-ag2026andClaude Fable 5 committed 2026-07-18 20:47:03 +02:00
1 parent 96bb78b1c1
commit a248aa52de
2 files changed
+231 -9

No files matched your search

+44 -7
View File
@@ -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
+187 -2
View File
@@ -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 <unit>`."""
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 <prop> --value <unit>` 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"