ouyangbo/hermes-agent
1
0
Fork 0
Code Issues Pull Requests Actions 9 Packages Projects Releases Wiki Activity

fix(dashboard): coalesce repeat gateway restarts for a short window

Browse Source 
`_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.
This commit is contained in:
Jack Lau
2026-08-18 05:00:49 -05:00
committed by kshitij
parent 657550716f
commit 49cc3708e5
3 changed files with 294 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files
hermes_cli/web_server.py
+51 -1
View File
@@ -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]:
tests/hermes_cli/test_spawn_gateway_restart_cooldown.py
+228
View File
@@ -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()
tests/hermes_cli/test_spawn_gateway_restart_reap.py
+15
View File
@@ -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."""