* fix(gateway): auto-start after container restart via planned-stop marker
On Docker (s6-overlay), the gateway runs as a dynamically-registered s6
service. When the container stops/restarts/upgrades, s6 sends the gateway
a plain SIGTERM. The shutdown path (_stop_impl) ended with an
unconditional _update_runtime_status("stopped"), persisting
gateway_state=stopped to the volume. container_boot.py reads that on the
next boot and only auto-starts gateways whose last state was "running"
(_AUTOSTART_STATES) — so after a routine `docker compose up
--force-recreate` the gateway stays down and messaging channels silently
go dark, with no error surfaced (issue #42675).
The codebase already distinguishes intentional stops from unexpected
signals via the planned-stop marker (write_planned_stop_marker /
consume_planned_stop_marker_for_self): `hermes gateway stop`,
systemd/launchd ExecStop, and Ctrl+C write a marker before signalling,
so the handler classifies them as planned. An unmarked SIGTERM
(container/s6 restart, OOM, bare kill) is signal-initiated.
This wires that existing classification through to the state persist,
rather than adding unreliable signal-source inference:
- run.py: GatewayRunner._signal_initiated_shutdown, set in
shutdown_signal_handler's unmarked-signal branch. In _stop_impl, a
signal-initiated (non-restart) teardown now persists "running" instead
of "stopped" — preserving the operator's run-intent and overwriting the
mid-shutdown "draining" marker so _AUTOSTART_STATES matches on reboot.
Operator stops and restarts persist "stopped" as before.
- service_manager.py: S6ServiceManager.stop() now writes the planned-stop
marker for the supervised PID (read from s6-svstat) before `s6-svc -d`,
so an in-container `hermes gateway stop` is correctly classified as
intentional (parity with the systemd/launchd/host stop paths, which
already mark). Best-effort: a marker-write failure falls back to the
safe signal-initiated path.
Tests: shutdown persist-decision table (signal→running, operator→stopped,
restart→stopped), s6 stop marker write + svstat PID parse + failure
tolerance. The signal→running and s6-marker tests fail without the
respective source change. Verified end-to-end against a container built
from this branch: an unmarked SIGTERM to the live gateway leaves
gateway_state=running (shutdown-context log confirms signal path);
existing real container-restart suite still green.
* docs(docker): clarify gateway autostart distinguishes operator-stop from container-kill
The per-profile-supervision section described the autostart-across-restart
contract as "running gateways come back, stopped stay stopped" without
spelling out what records 'stopped'. That contract was the source of
#42675 confusion: users expected a restart to bring the gateway back and
it didn't. With the write-side fix, only an explicit `hermes gateway stop`
records 'stopped'; container/s6 restart SIGTERMs (incl. image upgrades and
unexpected exits) leave the state 'running' so the gateway auto-starts.
Make that distinction explicit in both the multi-profile and
per-profile-supervision sections.
* test(docker): real-restart autostart E2E for #42675
Adds test_live_gateway_autostarts_after_real_restart_without_manual_state_stamp:
a live s6-supervised gateway is killed by an actual `docker restart`
SIGTERM (no manual gateway_state stamp, no planned-stop marker) and must
auto-start on the next boot. Exercises the WRITE side of the fix that the
existing stamp-based tests bypass.
Verified to FAIL against an origin/main image (reconciler logs
prior_state=stopped action=registered — the #42675 bug) and PASS against
the fixed image (prior_state=running action=started).
This commit is contained in:
@@ -66,6 +66,7 @@ def make_restart_runner(
|
||||
runner._background_tasks = set()
|
||||
runner._draining = False
|
||||
runner._restart_requested = False
|
||||
runner._signal_initiated_shutdown = False
|
||||
runner._restart_task_started = False
|
||||
runner._restart_detached = False
|
||||
runner._restart_via_service = False
|
||||
|
||||
@@ -358,3 +358,90 @@ async def test_gateway_stop_kills_tool_subprocesses_on_graceful_path(monkeypatch
|
||||
|
||||
# Only the final catch-all fires on the graceful path.
|
||||
assert kill_count == 1
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# gateway_state persistence on shutdown (issue #42675)
|
||||
#
|
||||
# On Docker/s6, container_boot.py only auto-starts gateways whose last
|
||||
# persisted gateway_state was "running". An unexpected external signal
|
||||
# (the SIGTERM s6/Docker sends on `docker compose up --force-recreate`,
|
||||
# OOM, bare kill) must NOT persist "stopped" — otherwise the gateway
|
||||
# stays down after every container restart. An operator-initiated stop
|
||||
# writes a planned-stop marker first, so it is NOT signal-initiated and
|
||||
# DOES persist "stopped", respecting the explicit intent.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _persisted_states(runner) -> list:
|
||||
"""All gateway_state values passed to _update_runtime_status, in order."""
|
||||
states = []
|
||||
for call in runner._update_runtime_status.call_args_list:
|
||||
args, kwargs = call
|
||||
state = kwargs.get("gateway_state", args[0] if args else None)
|
||||
states.append(state)
|
||||
return states
|
||||
|
||||
|
||||
def _stopped_state_persisted(runner) -> bool:
|
||||
"""True iff _update_runtime_status was called with gateway_state='stopped'."""
|
||||
return "stopped" in _persisted_states(runner)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_signal_initiated_shutdown_persists_running_not_stopped(tmp_path, monkeypatch):
|
||||
"""Unexpected SIGTERM (container restart / OOM / kill) must persist
|
||||
gateway_state=running — NOT stopped, and NOT leave the mid-shutdown
|
||||
'draining' marker — so container_boot auto-starts on next boot (#42675)."""
|
||||
monkeypatch.setattr(gateway_run, "_hermes_home", tmp_path)
|
||||
runner, adapter = make_restart_runner()
|
||||
adapter.disconnect = AsyncMock()
|
||||
runner._signal_initiated_shutdown = True # set by handler on unmarked signal
|
||||
|
||||
with patch("gateway.status.remove_pid_file"), patch("gateway.status.write_runtime_status"):
|
||||
await runner.stop()
|
||||
|
||||
assert not _stopped_state_persisted(runner), (
|
||||
"signal-initiated shutdown must NOT persist gateway_state=stopped"
|
||||
)
|
||||
# The FINAL terminal write must be 'running' so container_boot's
|
||||
# _AUTOSTART_STATES check passes (it only auto-starts 'running').
|
||||
assert _persisted_states(runner)[-1] == "running", (
|
||||
f"final state must be 'running', got: {_persisted_states(runner)}"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_operator_initiated_stop_persists_stopped(tmp_path, monkeypatch):
|
||||
"""A planned stop (marker written → not signal-initiated) must persist
|
||||
gateway_state=stopped so an explicit `hermes gateway stop` stays down."""
|
||||
monkeypatch.setattr(gateway_run, "_hermes_home", tmp_path)
|
||||
runner, adapter = make_restart_runner()
|
||||
adapter.disconnect = AsyncMock()
|
||||
runner._signal_initiated_shutdown = False # planned stop classification
|
||||
|
||||
with patch("gateway.status.remove_pid_file"), patch("gateway.status.write_runtime_status"):
|
||||
await runner.stop()
|
||||
|
||||
assert _stopped_state_persisted(runner), (
|
||||
"operator-initiated stop must persist gateway_state=stopped"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_signal_initiated_restart_still_persists_stopped(tmp_path, monkeypatch):
|
||||
"""A restart is not a 'stay down' — it persists normally (the new
|
||||
process/container brings the gateway back up itself). The suppression
|
||||
only applies to a terminal signal-initiated stop, not a restart."""
|
||||
monkeypatch.setattr(gateway_run, "_hermes_home", tmp_path)
|
||||
runner, adapter = make_restart_runner()
|
||||
adapter.disconnect = AsyncMock()
|
||||
runner._signal_initiated_shutdown = True
|
||||
runner._launch_systemd_restart_shortcut = MagicMock()
|
||||
|
||||
with patch("gateway.status.remove_pid_file"), patch("gateway.status.write_runtime_status"):
|
||||
await runner.stop(restart=True, service_restart=True)
|
||||
|
||||
assert _stopped_state_persisted(runner), (
|
||||
"a restart must persist gateway_state=stopped via the normal path"
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user