fix(dashboard): coalesce repeat gateway restarts for a short window - #89088
Closed
jackulau wants to merge 1 commit into
Closed
fix(dashboard): coalesce repeat gateway restarts for a short window#89088jackulau wants to merge 1 commit into
jackulau wants to merge 1 commit into
Conversation
`_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. NousResearch#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 NousResearch#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.
19 tasks
Collaborator
|
Merged via #90275 using rebase-merge — your commit cherry-picked with authorship preserved. Your fix was excellent: correct root cause diagnosis (child exits before gateway is back, so Closes #89034 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Stops the dashboard from starting a fresh gateway restart every time a stale
frontend asks for one, which is the flood at the top of #89034's causal chain.
_spawn_gateway_restartalready refuses to start a second restart while thefirst
hermes gateway restartchild is alive — that is what makes adouble-clicked button safe. The guard evaporates exactly when it matters most.
The child exits as soon as it has handed the restart off (to the supervisor, or
to the running gateway), which is long before the gateway is back, so:
a cached frontend re-firing every few seconds clears
poll() is Noneon everyattempt and gets a brand new restart each time. The function's own docstring
already names that frontend as a known caller ("a stale cached frontend firing
its own restart after the server already auto-restarted post-onboarding") — it
just guards the wrong half of the problem.
The reporter measured the consequence on an s6-supervised container: 77
gateway-restart startedentries, 17 of them inside one minute. Each oneSIGHUPs a gateway that is still coming up, and enough of those landed
mid-FTS5-write to corrupt
state.db—database disk image is malformed203xin
agent.log, auto-repair failed, operator recreated the file by hand.This PR coalesces same-profile requests that arrive within
GATEWAY_RESTART_COOLDOWN_SECONDSof the last spawn onto that spawn, and logseach one. A storm becomes one restart plus a run of coalesce lines that say so.
Two design decisions worth naming, both maintainer calls if you disagree:
finishwith no backoff → state.db FTS5 corruption ("database disk image is malformed") #89034'sExpected section asks for the health-gated version, and it is the more
precise rule. I did not implement it because it cannot be made to fail safe
here: a gateway that never comes back would leave the restart action
permanently inert, and "the restart button no longer works" 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 what an operator
waits before deliberately retrying.
_ACTION_PROCS. Completed actionchildren are reaped out of that table, and a guard that disappears when the
child exits is precisely the bug being fixed. There is a regression test for
this, and it matters for fix(dashboard): reap action subprocesses without status polling #89060 specifically (see Overlap below).
Related Issue
Fixes #89034
Scope: this fixes Defect 1 only. #89034 reports two compounding defects and
I am deliberately not touching the second one. Defect 2 is the s6
finishscript having no death-cap, generated by
hermes_cli.service_manager.S6ServiceManager._render_finish_script. I looked atit and did not write it, for a reason I would rather state than bury: the
suggested cap makes s6 stop supervising after N unclean exits in a window, so a
container that legitimately crash-loops for a transient reason (bad token,
unreachable provider) would stay permanently down instead of recovering when
the cause clears. That is a real product tradeoff on a shipped image and it
belongs to a maintainer, not to me. It is also worth noting that s6-supervise
already floors respawns at roughly one per second, so Defect 2 alone does not
produce the observed 3.5s-spaced storm — Defect 1 does. Fixing this half
removes the driver; the death-cap would be a second layer of protection under
it.
Overlap with open PRs
Three open PRs touch this area. I read all three; none of them rate-limits
restart requests, and I have tried to leave room for each.
_spawn_gateway_restartdelegates to launchd/systemd/s6 when supervised, instead of spawning a childPOST /api/system/restart-hermessends SIGUSR1 instead of spawning/api/gateway/restart(the one the reported storm used) is unchanged by it_ACTION_PROCSwithout a status poll_ACTION_PROCS— the obvious way — their PR would silently re-open this exact hole.test_cooldown_survives_the_action_table_being_clearedpins that. We do both edittests/hermes_cli/test_spawn_gateway_restart_reap.py, so expect a small conflict there.Changes Made
hermes_cli/web_server.pyGATEWAY_RESTART_COOLDOWN_SECONDS(10.0) and_LAST_GATEWAY_RESTART, with the reasoning for both, next to_ACTION_PROCS._spawn_gateway_restart: after the existing in-flight reuse, coalesce asame-profile request made within the window onto the last spawn, log it at
INFO, and record
(monotonic, proc, command)on every real spawn. Adifferent profile is never coalesced — two profiles are two services.
tests/hermes_cli/test_spawn_gateway_restart_cooldown.py(new, 7 tests).tests/hermes_cli/test_spawn_gateway_restart_reap.py: added an autousefixture clearing the new module state. Required — without it the first case's
spawn suppresses the second case's, and that file fails. It is a fixture
rather than a per-test decorator so it collides as little as possible with
fix(dashboard): reap action subprocesses without status polling #89060.
How to Test
The new tests pass:
7 passedSabotage proof. Delete the
recent = _LAST_GATEWAY_RESTARTblock from_spawn_gateway_restartand re-run — the three regression tests fail withthe storm itself, while the four guardrail tests keep passing (so they are
not just asserting the fix back to itself):
Restore the block and all 7 pass. The
3 == 1is the reported behaviour inminiature: three requests, three restarts.
Neighbouring suites, unchanged:
170 passed, 4 skippedBaseline comparison over every test file in the repo that mentions
web_server(85 files, ~1060 tests), run serially with and without thischange: 31 failures both times, identical sets (pre-existing
Windows/environment failures —
os.geteuid, resource limits, anon-Windows-only platform test). Zero new failures, zero fixed by accident.
ruff checkclean on all three files.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass. Not the full suite —tests/hermes_cli/cannot be collected on Windows (test_doctor_journal_modes.pycallsos.geteuid). Ran the 85-fileweb_serverslice instead, with a with/without baseline: identical failure sets, zero new failures. CI covers the restDocumentation & Housekeeping
docs/, docstrings)._spawn_gateway_restart's docstring now explains why in-flight reuse alone was insufficient; no user-facing docs affectedsys.platformbranching in_spawn_hermes_actionand applies equally to launchd, systemd, s6 and unsupervised hostsOn the constant vs. a config key: I kept it a module constant to stay minimal.
If you would rather it be tunable,
agent.gateway_restart_cooldownalongsiderestart_drain_timeoutis the obvious home and I am happy to add it — say theword. I specifically did not reuse
restart_drain_timeouteven though itlooked like the principled choice, because it defaults to
0, which would makethe guard a no-op for everyone who has not configured it.
Screenshots / Logs
What the storm looks like after the change — one restart, and the suppressed
repeats say so instead of vanishing:
From #89034, what it looked like before (excerpt):