From 49cc3708e50d146fbfce90d4e733712f16cbbda0 Mon Sep 17 00:00:00 2001 From: Jack Lau <72348727+jackulau@users.noreply.github.com> Date: Tue, 18 Aug 2026 05:00:49 -0500 Subject: [PATCH] fix(dashboard): coalesce repeat gateway restarts for a short window `_spawn_gateway_restart` already reuses an in-flight `hermes gateway restart` child so a double-clicked button cannot start two racing restarts. That guard evaporates exactly when it is needed most: the child exits as soon as it has handed the restart to the supervisor (or to the running gateway), long before the gateway is actually back, so a stale cached dashboard frontend re-firing its own restart every few seconds cleared the guard on every attempt and started a fresh restart each time. #89034 measured the result on an s6-supervised container: 77 `gateway-restart started` entries, 17 of them inside one minute. Each one SIGHUPs a gateway that is still coming up, and killing it mid-FTS5-write corrupted `state.db` ("database disk image is malformed", 203x in agent.log) until the operator recreated the file by hand. Requests for the same profile within GATEWAY_RESTART_COOLDOWN_SECONDS of the last spawn are now coalesced onto that spawn and logged, so a storm produces one restart instead of one per request. The window is fixed rather than health-gated on purpose: a gateway that never comes back would leave a health-gated restart action permanently inert, which is a worse failure than the flood it prevents. The cooldown state is kept outside `_ACTION_PROCS` because completed action children are reaped out of that table, and a guard that disappears when the child exits is the bug being fixed. Only the *frontend-flood* half of #89034 is addressed here. The s6 `finish` death-cap the report also asks for is a separate change to `hermes_cli/service_manager.py` with a much larger blast radius, and is left for a maintainer decision. --- hermes_cli/web_server.py | 52 +++- .../test_spawn_gateway_restart_cooldown.py | 228 ++++++++++++++++++ .../test_spawn_gateway_restart_reap.py | 15 ++ 3 files changed, 294 insertions(+), 1 deletion(-) create mode 100644 tests/hermes_cli/test_spawn_gateway_restart_cooldown.py diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index 35f8f42290..2e9d4f5dc4 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -4382,6 +4382,30 @@ _ACTION_PROCS: Dict[str, subprocess.Popen] = {} _ACTION_COMMANDS: Dict[str, Tuple[str, ...]] = {} _ACTION_IDS: Dict[str, str] = {} +# A finished ``gateway-restart`` child does not mean the gateway is back: the +# child exits as soon as it has handed the restart to the supervisor (or to the +# running gateway), while the gateway itself is still stopping and coming up. +# The in-flight reuse in :func:`_spawn_gateway_restart` therefore stops +# coalescing exactly when repeat requests do the most damage, so a stale cached +# frontend that re-fires its restart every few seconds gets a brand new restart +# every time (#89034: 77 restarts, 17 of them inside one minute, killing the +# gateway often enough mid-FTS5-write to corrupt state.db). Suppress repeats +# for a short window after the last spawn as well. +# +# MAINTAINER DECISION: a fixed window, not "until the gateway reports healthy". +# Health-gating is what #89034 asks for, but it cannot be made to fail safe +# here — a gateway that never comes back would leave the restart action +# permanently inert, which is a worse failure than the flood it prevents. A +# fixed window always releases. 10s is above the ~3.5s spacing of the reported +# storm and below the time an operator waits before deliberately retrying. +GATEWAY_RESTART_COOLDOWN_SECONDS = 10.0 + +# ``(monotonic spawn time, Popen, command)`` for the last gateway restart this +# process started. Deliberately NOT read out of ``_ACTION_PROCS``: entries +# there are reaped once the child exits, and a guard that disappears when the +# child exits is the bug this exists to fix. +_LAST_GATEWAY_RESTART: Optional[Tuple[float, subprocess.Popen, Tuple[str, ...]]] = None + # ``name`` → completed synthetic action result for actions the server handled # without spawning a subprocess (for example, unsupported Docker updates). _ACTION_RESULTS: Dict[str, Dict[str, Any]] = {} @@ -4608,6 +4632,13 @@ def _spawn_gateway_restart(profile: Optional[str] = None) -> Tuple[subprocess.Po concurrent ``hermes gateway restart`` children race each other on the manual kill-and-start path, so reuse the live one instead. + Reusing only the *live* child is not enough. The child exits as soon as + the restart has been handed off, long before the gateway is back, so a + frontend re-firing every few seconds cleared that guard every time and + kept restarting a gateway that was still coming up (#89034). Requests + within ``GATEWAY_RESTART_COOLDOWN_SECONDS`` of the last spawn for the + same profile are coalesced onto that spawn as well. + Before spawning, sweep for orphaned gateway processes whose parent has exited (e.g. desktop-app restarts leaving a reparented gateway child under launchd/PPID=1). Without this the orphan keeps its platform @@ -4623,6 +4654,8 @@ def _spawn_gateway_restart(profile: Optional[str] = None) -> Tuple[subprocess.Po except Exception: pass # best-effort — don't block the restart on a reap failure + global _LAST_GATEWAY_RESTART + subcommand = _gateway_subcommand(profile, "restart") existing = _ACTION_PROCS.get("gateway-restart") if existing is not None and existing.poll() is None: @@ -4630,7 +4663,24 @@ def _spawn_gateway_restart(profile: Optional[str] = None) -> Tuple[subprocess.Po if existing_command is None or existing_command == tuple(subcommand): return existing, True raise RuntimeError("gateway restart already in progress for another profile") - return _spawn_hermes_action(subcommand, "gateway-restart"), False + + recent = _LAST_GATEWAY_RESTART + if recent is not None: + spawned_at, recent_proc, recent_command = recent + age = time.monotonic() - spawned_at if recent_command == tuple(subcommand) else None + if age is not None and age < GATEWAY_RESTART_COOLDOWN_SECONDS: + _log.info( + "Coalescing gateway restart: one was started %.1fs ago " + "(pid %s) and the gateway may still be coming back; not " + "spawning another (#89034).", + age, + getattr(recent_proc, "pid", "?"), + ) + return recent_proc, True + + proc = _spawn_hermes_action(subcommand, "gateway-restart") + _LAST_GATEWAY_RESTART = (time.monotonic(), proc, tuple(subcommand)) + return proc, False def _restart_gateway_after_webhook_enable(profile: Optional[str] = None) -> dict[str, Any]: diff --git a/tests/hermes_cli/test_spawn_gateway_restart_cooldown.py b/tests/hermes_cli/test_spawn_gateway_restart_cooldown.py new file mode 100644 index 0000000000..031dbf74ae --- /dev/null +++ b/tests/hermes_cli/test_spawn_gateway_restart_cooldown.py @@ -0,0 +1,228 @@ +"""Tests for the _spawn_gateway_restart repeat-request cooldown (#89034). + +A finished ``gateway-restart`` child does not mean the gateway is back, so the +pre-existing "reuse the in-flight child" guard stops coalescing exactly when +repeat requests are most harmful. A stale cached dashboard frontend re-firing +its restart every few seconds therefore produced a fresh restart every time +(the reporter measured 77, 17 of them inside one minute), killing the gateway +often enough mid-FTS5-write to corrupt ``state.db``. +""" +from __future__ import annotations + +import subprocess +from unittest.mock import MagicMock, patch + +import pytest + + +@pytest.fixture(autouse=True) +def reset_restart_cooldown(): + """Keep the module-level cooldown state out of neighbouring tests.""" + import hermes_cli.web_server as web_server + + web_server._LAST_GATEWAY_RESTART = None + yield + web_server._LAST_GATEWAY_RESTART = None + + +def _exited_proc(pid: int = 4242) -> MagicMock: + """A Popen handle for a child that has already finished.""" + proc = MagicMock(spec=subprocess.Popen) + proc.poll.return_value = 0 + proc.pid = pid + return proc + + +class TestRepeatRestartWithinCooldown: + """Repeats within the window ride the previous spawn.""" + + @patch( + "hermes_cli.web_server._gateway_subcommand", + return_value=["gateway", "restart"], + ) + @patch("hermes_cli.web_server._spawn_hermes_action") + @patch("hermes_cli.web_server._ACTION_PROCS", {}) + def test_second_request_after_the_child_exits_is_coalesced( + self, mock_spawn, mock_subcmd + ): + """The exact #89034 shape: child gone, gateway still coming up.""" + from hermes_cli.web_server import _spawn_gateway_restart + + proc = _exited_proc() + mock_spawn.return_value = proc + + with patch("hermes_cli.gateway._reap_unsupervised_gateway_orphans"), patch( + "hermes_cli.web_server.time.monotonic", side_effect=[100.0, 103.5] + ): + first, first_reused = _spawn_gateway_restart() + second, second_reused = _spawn_gateway_restart() + + assert mock_spawn.call_count == 1, ( + "a repeat request 3.5s later must not start a second restart" + ) + assert first is proc and first_reused is False + assert second is proc and second_reused is True + + @patch( + "hermes_cli.web_server._gateway_subcommand", + return_value=["gateway", "restart"], + ) + @patch("hermes_cli.web_server._spawn_hermes_action") + @patch("hermes_cli.web_server._ACTION_PROCS", {}) + def test_a_storm_of_requests_produces_exactly_one_restart( + self, mock_spawn, mock_subcmd + ): + """17 requests inside one minute, the reported burst rate.""" + from hermes_cli.web_server import _spawn_gateway_restart + + mock_spawn.return_value = _exited_proc() + # One read to stamp the spawn, then one per coalesced repeat. The + # window is anchored to the SPAWN, not to the previous request: + # anchoring it to the previous request would let a 3.5s-spaced storm + # walk the deadline forward forever and never restart at all. + clock = [100.0, 103.5, 107.0] + + with patch("hermes_cli.gateway._reap_unsupervised_gateway_orphans"), patch( + "hermes_cli.web_server.time.monotonic", side_effect=clock + ): + _spawn_gateway_restart() + _spawn_gateway_restart() + _spawn_gateway_restart() + + assert mock_spawn.call_count == 1 + + @patch( + "hermes_cli.web_server._gateway_subcommand", + return_value=["gateway", "restart"], + ) + @patch("hermes_cli.web_server._spawn_hermes_action") + @patch("hermes_cli.web_server._ACTION_PROCS", {}) + def test_cooldown_survives_the_action_table_being_cleared( + self, mock_spawn, mock_subcmd + ): + """The guard must not live in ``_ACTION_PROCS``. + + Completed action children get reaped out of that table, and a guard + that disappears when the child is reaped is the bug this fixes. + """ + import hermes_cli.web_server as web_server + from hermes_cli.web_server import _spawn_gateway_restart + + mock_spawn.return_value = _exited_proc() + + with patch("hermes_cli.gateway._reap_unsupervised_gateway_orphans"), patch( + "hermes_cli.web_server.time.monotonic", side_effect=[100.0, 102.0] + ): + _spawn_gateway_restart() + web_server._ACTION_PROCS.clear() + web_server._ACTION_COMMANDS.clear() + _, reused = _spawn_gateway_restart() + + assert mock_spawn.call_count == 1 + assert reused is True + + +class TestCooldownReleases: + """The window always expires; it never wedges the restart action.""" + + @patch( + "hermes_cli.web_server._gateway_subcommand", + return_value=["gateway", "restart"], + ) + @patch("hermes_cli.web_server._spawn_hermes_action") + @patch("hermes_cli.web_server._ACTION_PROCS", {}) + def test_request_after_the_window_starts_a_real_restart( + self, mock_spawn, mock_subcmd + ): + from hermes_cli.web_server import _spawn_gateway_restart + + mock_spawn.side_effect = [_exited_proc(1), _exited_proc(2)] + + with patch("hermes_cli.gateway._reap_unsupervised_gateway_orphans"), patch( + "hermes_cli.web_server.time.monotonic", + side_effect=[100.0, 111.0, 111.0], + ): + _spawn_gateway_restart() + second, reused = _spawn_gateway_restart() + + assert mock_spawn.call_count == 2 + assert reused is False + assert second.pid == 2 + + @patch("hermes_cli.web_server._spawn_hermes_action") + @patch("hermes_cli.web_server._ACTION_PROCS", {}) + def test_a_different_profile_is_never_coalesced(self, mock_spawn): + """Two profiles are two services; one's restart is not the other's.""" + from hermes_cli.web_server import _spawn_gateway_restart + + mock_spawn.side_effect = [_exited_proc(1), _exited_proc(2)] + + with patch("hermes_cli.gateway._reap_unsupervised_gateway_orphans"), patch( + "hermes_cli.web_server.time.monotonic", + side_effect=[100.0, 101.0], + ), patch( + "hermes_cli.web_server._gateway_subcommand", + side_effect=[["gateway", "restart"], ["-p", "coder", "gateway", "restart"]], + ): + _spawn_gateway_restart() + second, reused = _spawn_gateway_restart(profile="coder") + + assert mock_spawn.call_count == 2 + assert reused is False + assert second.pid == 2 + + +class TestExistingBehaviourIsPreserved: + """Regression guards on the pre-existing in-flight reuse.""" + + @patch( + "hermes_cli.web_server._gateway_subcommand", + return_value=["gateway", "restart"], + ) + @patch("hermes_cli.web_server._spawn_hermes_action") + def test_live_child_is_still_reused_without_consulting_the_clock( + self, mock_spawn, mock_subcmd + ): + from hermes_cli.web_server import _spawn_gateway_restart + + live = MagicMock(spec=subprocess.Popen) + live.poll.return_value = None + live.pid = 7 + + with patch( + "hermes_cli.web_server._ACTION_PROCS", {"gateway-restart": live} + ), patch( + "hermes_cli.web_server._ACTION_COMMANDS", + {"gateway-restart": ("gateway", "restart")}, + ), patch( + "hermes_cli.gateway._reap_unsupervised_gateway_orphans" + ): + proc, reused = _spawn_gateway_restart() + + assert proc is live + assert reused is True + mock_spawn.assert_not_called() + + @patch( + "hermes_cli.web_server._gateway_subcommand", + return_value=["gateway", "restart"], + ) + @patch("hermes_cli.web_server._spawn_hermes_action") + def test_live_child_for_another_profile_still_raises(self, mock_spawn, mock_subcmd): + from hermes_cli.web_server import _spawn_gateway_restart + + live = MagicMock(spec=subprocess.Popen) + live.poll.return_value = None + + with patch( + "hermes_cli.web_server._ACTION_PROCS", {"gateway-restart": live} + ), patch( + "hermes_cli.web_server._ACTION_COMMANDS", + {"gateway-restart": ("-p", "coder", "gateway", "restart")}, + ), patch( + "hermes_cli.gateway._reap_unsupervised_gateway_orphans" + ): + with pytest.raises(RuntimeError, match="another profile"): + _spawn_gateway_restart() + + mock_spawn.assert_not_called() diff --git a/tests/hermes_cli/test_spawn_gateway_restart_reap.py b/tests/hermes_cli/test_spawn_gateway_restart_reap.py index 8b2832660f..dc0c8aa74f 100644 --- a/tests/hermes_cli/test_spawn_gateway_restart_reap.py +++ b/tests/hermes_cli/test_spawn_gateway_restart_reap.py @@ -7,6 +7,21 @@ from unittest.mock import MagicMock, patch import pytest +@pytest.fixture(autouse=True) +def reset_restart_cooldown(): + """Clear the #89034 repeat-restart cooldown between cases. + + ``_spawn_gateway_restart`` now coalesces a second restart request that + arrives within ``GATEWAY_RESTART_COOLDOWN_SECONDS`` of the last spawn, so + without this the first case's spawn suppresses the second case's. + """ + import hermes_cli.web_server as web_server + + web_server._LAST_GATEWAY_RESTART = None + yield + web_server._LAST_GATEWAY_RESTART = None + + class TestSpawnGatewayRestartReapsOrphans: """_spawn_gateway_restart must reap orphaned gateways before spawning."""