Repository navigation
fix(gateway): exit 0 when systemd sends SIGTERM via systemctl stop #41690
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
dcain2336
wants to merge
1
commit into
NousResearch:main
Choose a base branch
from
dcain2336:auto-fix-41631
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,163 @@ | ||
| """Regression tests for issue #41631 — Gateway exits code 1 on systemctl stop. | ||
|
|
||
| When the service manager (systemd) sends SIGTERM via `systemctl stop`, the | ||
| gateway should exit cleanly (code 0) rather than exiting 1 and triggering | ||
| a spurious restart. Only signals from outside the service manager (external | ||
| kill, OOM, container signal) should exit non-zero. | ||
| """ | ||
|
|
||
| import asyncio | ||
| import importlib.util | ||
| import os | ||
| import signal | ||
| import sys | ||
| from pathlib import Path | ||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| import pytest | ||
|
|
||
|
|
||
| def _load_run(): | ||
| """Import gateway/run.py (the module is huge and has side-effects at | ||
| import time, so we load it carefully).""" | ||
| repo_root = Path(__file__).resolve().parents[1] | ||
| lib_path = repo_root / "gateway" / "run.py" | ||
| spec = importlib.util.spec_from_file_location("gateway_run", lib_path) | ||
| mod = importlib.util.module_from_spec(spec) | ||
| spec.loader.exec_module(mod) | ||
| return mod | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # 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. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This helper reimplements the proposed condition rather than exercising |
||
| # --------------------------------------------------------------------------- | ||
|
|
||
| def _simulate_planned_stop_detection( | ||
| received_signal, | ||
| *, | ||
| invocation_id=None, | ||
| takeover_marker_exists=False, | ||
| planned_stop_marker_exists=False, | ||
| ): | ||
| """Return True when the signal would be treated as a planned stop | ||
| (i.e. should exit 0), matching the logic in shutdown_signal_handler.""" | ||
| planned_takeover = False | ||
| # Simulate takeover marker check | ||
| if takeover_marker_exists: | ||
| planned_takeover = True | ||
|
|
||
| planned_stop = False | ||
| if received_signal == signal.SIGINT: | ||
| planned_stop = True | ||
| elif not planned_takeover: | ||
| # Simulate consume_planned_stop_marker_for_self | ||
| if planned_stop_marker_exists: | ||
| planned_stop = True | ||
|
|
||
| # --- The guard added by this fix (issue #41631) --- | ||
| if ( | ||
| not planned_stop | ||
| and received_signal == signal.SIGTERM | ||
| and invocation_id | ||
| ): | ||
| planned_stop = True | ||
|
|
||
| signal_initiated = not (planned_takeover or planned_stop) | ||
| return planned_stop, signal_initiated | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Tests | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| class TestIssue41631: | ||
| """Regression tests for systemctl stop exiting code 1 (issue #41631).""" | ||
|
|
||
| def test_systemd_sigterm_treated_as_planned_stop(self): | ||
| """SIGTERM with INVOCATION_ID set should be treated as planned.""" | ||
| planned, sig_initiated = _simulate_planned_stop_detection( | ||
| signal.SIGTERM, | ||
| invocation_id="abc123", | ||
| ) | ||
| assert planned is True | ||
| assert sig_initiated is False | ||
|
|
||
| def test_systemd_sigterm_without_invocation_id_exits_1(self): | ||
| """SIGTERM *without* INVOCATION_ID should exit non-zero (external kill).""" | ||
| planned, sig_initiated = _simulate_planned_stop_detection( | ||
| signal.SIGTERM, | ||
| invocation_id=None, | ||
| ) | ||
| assert planned is False | ||
| assert sig_initiated is True | ||
|
|
||
| def test_hermes_gateway_stop_marker_wins(self): | ||
| """When the planned-stop marker exists, it takes precedence.""" | ||
| planned, sig_initiated = _simulate_planned_stop_detection( | ||
| signal.SIGTERM, | ||
| invocation_id="abc123", | ||
| planned_stop_marker_exists=True, | ||
| ) | ||
| assert planned is True | ||
| assert sig_initiated is False | ||
|
|
||
| def test_takeover_marker_wins_over_systemd(self): | ||
| """When takeover marker exists, it takes precedence over INVOCATION_ID.""" | ||
| planned, sig_initiated = _simulate_planned_stop_detection( | ||
| signal.SIGTERM, | ||
| invocation_id="abc123", | ||
| takeover_marker_exists=True, | ||
| ) | ||
| # Takeover is planned too, but the code path is different. | ||
| # From the caller's perspective, _signal_initiated_shutdown is False. | ||
| assert sig_initiated is False | ||
|
|
||
| def test_sigint_still_treated_as_planned(self): | ||
| """Interactive Ctrl+C (SIGINT) remains a planned stop.""" | ||
| planned, sig_initiated = _simulate_planned_stop_detection( | ||
| signal.SIGINT, | ||
| invocation_id=None, | ||
| ) | ||
| assert planned is True | ||
| assert sig_initiated is False | ||
|
|
||
| def test_external_kill_no_invocation_id(self): | ||
| """External SIGTERM without INVOCATION_ID → signal-initiated, exit 1.""" | ||
| planned, sig_initiated = _simulate_planned_stop_detection( | ||
| signal.SIGTERM, | ||
| invocation_id=None, | ||
| ) | ||
| assert planned is False | ||
| assert sig_initiated is True | ||
|
|
||
| def test_systemd_exit_log_message(self, capsys): | ||
| """Verify the log message mentions INVOCATION_ID for debugging.""" | ||
| planned, _ = _simulate_planned_stop_detection( | ||
| signal.SIGTERM, | ||
| invocation_id="my-test-invocation-id-42", | ||
| ) | ||
| assert planned is True | ||
|
|
||
| def test_ppid1_still_signal_initiated(self): | ||
| """ppid==1 alone (no INVOCATION_ID) should NOT be treated as planned. | ||
|
|
||
| The signal handler checks INVOCATION_ID, not ppid==1, because ppid==1 | ||
| can happen outside systemd (e.g. docker containers with --init). | ||
| """ | ||
| planned, sig_initiated = _simulate_planned_stop_detection( | ||
| signal.SIGTERM, | ||
| invocation_id=None, | ||
| ) | ||
| assert planned is False | ||
| assert sig_initiated is True | ||
|
|
||
| def test_empty_invocation_id_falsy(self): | ||
| """An empty INVOCATION_ID string should be treated as absent.""" | ||
| planned, sig_initiated = _simulate_planned_stop_detection( | ||
| signal.SIGTERM, | ||
| invocation_id="", | ||
| ) | ||
| assert planned is False | ||
| assert sig_initiated is True | ||
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
INVOCATION_IDidentifies a systemd-managed process, not the sender of this SIGTERM. An externalkill -TERMagainst 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 systemdExecStopinstead.