Skip to content

fix(gateway): skip systemd timing check for non-loaded units - #36766

Open
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:fix/systemd-load-state-check-v2
Open

fix(gateway): skip systemd timing check for non-loaded units#36766
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:fix/systemd-load-state-check-v2

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes false-positive 'stale systemd unit' warning when the gateway runs as a system-level unit but the user manager is reachable. The warning would appear even when the system unit has the correct TimeoutStopSec because systemctl --user show returns compiled-in defaults (90s) for nonexistent units.

Related Issue

Fixes #36755

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • gateway/shutdown_forensics.py: Add LoadState to systemctl query and skip iterations where LoadState != loaded
  • gateway/run.py: Update warning message to reference correct CLI command

How to Test

  1. (Linux only) Deploy hermes-gateway as a system-level systemd unit with TimeoutStopSec=240s
  2. Ensure the gateway user has a reachable user manager (e.g., XDG_RUNTIME_DIR set for rootless podman)
  3. Start the gateway via systemctl
  4. Verify no false-positive warning appears in the logs (previously would warn "stale unit" with 90s timeout)

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass (1 pre-existing flaky test failure unrelated to this change)
  • I've added tests for my changes (N/A - existing tests cover the function)
  • I've tested on my platform: macOS (no systemd - code path unchanged)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — Linux-only function, no impact on Windows/macOS
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Code Intelligence

⚠️ GitNexus unavailable — grep-based fallback used.

  • Analyzed: gateway/shutdown_forensics.py:check_systemd_timing_alignment() (callers: gateway/run.py:check_systemd_timing_alignment())
  • Blast radius: LOW — only affects Linux systems under systemd, function is defensive and never raises
  • Related patterns: subprocess error handling, systemctl query parsing, systemd unit property retrieval

Fixes NousResearch#36755

When the gateway runs as a system-level unit but the user manager is reachable
(e.g., with XDG_RUNTIME_DIR set for rootless podman), `systemctl --user show`
returns rc=0 and emits compiled-in defaults (TimeoutStopUSec=1min 30s) for
nonexistent units. This produces a false-positive 'stale unit' warning even when
the system unit has the correct TimeoutStopSec.

Add LoadState to the systemctl query and skip iterations where LoadState != loaded.

Also update the warning message to reference the correct CLI command
(hermes gateway install --replace, not hermes gateway service install --replace).
@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have labels Jun 1, 2026

@mxnstrexgl mxnstrexgl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated PR Review

Security Scan

  • ✓ No hardcoded secrets, injection sinks, unsafe deserialization, or dependency red flags found by this automated scan.

Code Quality

  • ✓ No blocking code-quality issues found by this automated scan.
  • ℹ️ No test file changes detected; verify existing coverage exercises this behavior.

Summary

Status: APPROVE — security findings: 0, quality suggestions: 0.

Automated review; raw diff content intentionally omitted.

@kristian-dott-42

Copy link
Copy Markdown

Verified this fix in production on v0.18.0 (Ubuntu 24.04, systemd 255): a root system unit installed by hermes gateway install --system --run-as-user root plus Linger=yes for root reproduces the false positive on every start — no custom unit needed, since _ensure_user_systemd_env() sets XDG_RUNTIME_DIR at runtime and the user manager then answers LoadState=not-found + default TimeoutStopUSec=1min 30s (details in #36755). With the LoadState skip applied, the check falls through to the system manager (LoadState=loaded, 210s), mismatch is false, no warning, and the gateway/adapters are unaffected.

One rebase note: the gateway/run.py hunk looks stale — current main already reads Run `hermes gateway install --force` (gateway/run.py:6462), so that hunk will conflict or regress the wording back to --replace.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating the systemd manager-selection failure. The LoadState gate addresses the current-main path in gateway/shutdown_forensics.py:367-389, which otherwise accepts the first user-manager timeout before consulting the system manager.

Problems

  • gateway/run.py:3994 restores hermes gateway install --replace. Current main intentionally uses hermes gateway install --force at gateway/run.py:6739; commit c9df4bc094fb records that the old service/install --replace command does not exist.
  • No regression test accompanies the gate. The existing alignment coverage at tests/gateway/test_shutdown_forensics.py:237-250 covers only early-return paths, not a LoadState=not-found user response followed by a loaded system response.

Suggested changes

  • Salvage only the gateway/shutdown_forensics.py behavior and retain current main's --force warning text.
  • Add a mocked two-manager regression test proving the loaded system unit is selected after the non-loaded user unit is skipped.

Automated hermes-sweeper review.

Comment thread gateway/run.py
"Stale systemd unit detected: %s has TimeoutStopSec=%.0fs but "
"drain_timeout=%.0fs (expected >=%.0fs). systemd may SIGKILL the "
"gateway mid-drain. Run `hermes gateway service install --replace` "
"gateway mid-drain. Run `hermes gateway install --replace` "

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--replace is stale here: commit c9df4bc094fb intentionally changed this remediation to hermes gateway install --force because the former command no longer exists. Please omit this hunk during salvage and retain current main's wording.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(diagnostics): check_systemd_timing_alignment false-positive — systemctl show returns default 90s for nonexistent --user unit

5 participants