Repository navigation
Conversation
When running under systemd (INVOCATION_ID set), treat SIGTERM as a planned stop so the unit exits cleanly (code 0) instead of code 1. Only signals from outside the service manager (external kill, OOM, container signal) exit non-zero so Restart=on-failure can revive. Previously, systemctl stop caused the gateway to exit 1, which put the unit in a failed state despite systemd having intentionally stopped it. Closes NousResearch#41631
Code Review — Positive VerificationReviewed the full diff (211 lines: Correctness:
Edge cases handled:
Test coverage: 163-line test file covers the key scenarios. LGTM. |
|
✅ Verified — systemd SIGTERM detection via INVOCATION_ID Reviewed the diff in
LGTM — clean gateway stability fix. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing the systemd shutdown failure. The direct systemctl stop premise remains valid on current main: generated units have no ExecStop marker writer (hermes_cli/gateway.py:2761-2767, :2795-2801), while unmarked SIGTERM deliberately exits 1 (gateway/run.py:20611-20623, :20912-20925).
Problems
gateway/run.py:19893usesINVOCATION_IDto infer who sent SIGTERM. Current forensics defines it as evidence that the process runs under systemd, not signal provenance (gateway/shutdown_forensics.py:134-143). An externalkill -TERMagainst a systemd-managed gateway would therefore be misclassified as planned and suppress the intended recovery exit.tests/test_issue_41631_fix.py:34-58duplicates the proposed branch instead of exercisingshutdown_signal_handler, so the tests do not verify the production integration.
Suggested changes
- Re-scope to the existing planned-stop marker path: a non-recursive systemd
ExecStophelper should mark$MAINPIDbefore systemd signals it. This is the marker-based direction documented in #42517 and open PR #42555. - Cover both generated unit scopes and the real marker-driven classification.
Automated hermes-sweeper review.
| not planned_stop | ||
| and received_signal == signal.SIGTERM | ||
| and os.environ.get("INVOCATION_ID") | ||
| ): |
There was a problem hiding this comment.
INVOCATION_ID identifies a systemd-managed process, not the sender of this SIGTERM. An external kill -TERM against the service retains that environment variable and would be treated as planned here, suppressing the existing non-zero recovery path. Please use the planned-stop marker path from systemd ExecStop instead.
| # --------------------------------------------------------------------------- | ||
| # Helper: simulate the signal-handler logic for the planned-stop detection | ||
| # path. We don't invoke the full gateway runner; instead we replicate the | ||
| # relevant branching from shutdown_signal_handler to verify the guard. |
There was a problem hiding this comment.
This helper reimplements the proposed condition rather than exercising shutdown_signal_handler, so these tests cannot catch divergence in the real signal path. Test the actual marker-driven classification or an observable handler outcome instead.
Summary
When running under systemd (detected via
INVOCATION_ID), treat SIGTERM as a planned stop so the unit exits cleanly (code 0) instead of code 1.Only signals from outside the service manager (external kill, OOM, container signal) exit non-zero so
Restart=on-failurecan revive the gateway.Problem
systemctl stop hermes-gateway.servicesends SIGTERM to the gateway. The gateway didn't have a "planned stop marker" (onlyhermes gateway stopcreates one), so it treated the signal as unexpected and exited 1 — putting the unit in a failed state despite systemd having intentionally stopped it.Fix
In
shutdown_signal_handler, after checking for takeover/planned-stop markers, we now check: ifreceived_signal == SIGTERMandINVOCATION_IDis set in the environment, setplanned_stop = Trueso_signal_initiated_shutdownstaysFalseand the gateway exits 0.This is safe because:
INVOCATION_IDis only set by systemd (not external kill, not container signal)INVOCATION_ID) still exits 1 forRestart=on-failureChanges
gateway/run.py: Added systemd-initiated SIGTERM detection in signal handlertests/test_issue_41631_fix.py: 9 regression tests covering all scenariosTest Plan
Closes #41631