Skip to content

fix(gateway): don't accept systemd's not-found default for TimeoutStopUSec - #72943

Closed
henryberliand-design wants to merge 2 commits into
NousResearch:mainfrom
aerodeck-ai:fix/systemd-timing-alignment-not-found-default
Closed

fix(gateway): don't accept systemd's not-found default for TimeoutStopUSec#72943
henryberliand-design wants to merge 2 commits into
NousResearch:mainfrom
aerodeck-ai:fix/systemd-timing-alignment-not-found-default

Conversation

@henryberliand-design

Copy link
Copy Markdown

What does this PR do?

Fixes a false-positive warning in the gateway's shutdown-timing diagnostics (gateway/shutdown_forensics.py::check_systemd_timing_alignment) for any Hermes gateway installed as a system-managed systemd unit (e.g. /etc/systemd/system/hermes-gateway-*.service) that carries a real TimeoutStopSec= override.

Root cause: systemctl show <unit> --property=TimeoutStopUSec does not error (rc=0) when the unit isn't loaded under the manager you queried — it silently returns systemd's compiled-in template default (TimeoutStopUSec=1min 30s) instead. The lookup tried --user first (the common case), took that rc=0 response at face value, and never fell through to the --system manager that actually owns the unit and holds the real timeout override. The result: gateways with a longer, deliberately-configured TimeoutStopSec got flagged with a bogus timing-misalignment warning based on the generic 90s default, not their real value.

The fix: also request --property=LoadState in the same systemctl show call, and only trust a TimeoutStopUSec reading whose LoadState is actually loaded under the manager that answered. If both --user and --system report not-found, the function now correctly returns None ("can't determine") instead of manufacturing a mismatch from the template default.

Related Issue

No existing issue — found this via a false-positive warning in production on a system-managed gateway unit with a real TimeoutStopSec=240 override.

Type of Change

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

Changes Made

  • gateway/shutdown_forensics.py: fetch LoadState alongside TimeoutStopUSec in the systemctl show call; skip any result where LoadState=not-found, falling through to the next manager instead of accepting the compiled-in default.
  • tests/gateway/test_shutdown_forensics.py: added TestCheckSystemdTimingAlignmentManagerSelection with 3 regression tests:
    • a system-managed unit with a real TimeoutStopSec=240 override is correctly read from the --system manager instead of accepting the --user manager's bogus not-found default (mirrors a real production unit),
    • a false-positive control: a unit genuinely unmanaged under either manager (both report LoadState=not-found) must return None, never a manufactured mismatch — this proves the fix doesn't just move the false positive somewhere else,
    • a control for the already-working path: a unit genuinely loaded under --user is still accepted directly without needlessly falling through to --system.

How to Test

  1. pytest tests/gateway/test_shutdown_forensics.py -v — 33 tests pass, including the 3 new ones above.
  2. To reproduce the original bug: install a service as a system unit with TimeoutStopSec=240 (no matching --user unit loaded), then call check_systemd_timing_alignment() under that unit's cgroup — before this fix it reports a mismatch against the wrong 90s default; after, it correctly reads 240s from the system manager.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass (ran the full tests/gateway/ suite; 33/33 pass)
  • I've added tests for my changes
  • I've tested on my platform: Ubuntu 24.04 (this is a Linux/systemd-specific code path)

Documentation & Housekeeping

  • N/A — no docs, config keys, or tool schemas changed by this fix

…pUSec

`systemctl show <unit>` never errors for a unit that isn't loaded under
the manager you queried -- it returns rc=0 plus the compiled-in template
defaults (LoadState=not-found, TimeoutStopUSec=1min 30s). Gateways
installed as system-managed units (confirmed live in production with
real TimeoutStopSec overrides) had their --user query "succeed" with
this bogus default and never reached the system manager holding the
real value, producing false-positive timeout-misalignment warnings.

Now also fetch LoadState and only trust a TimeoutStopUSec whose unit is
actually loaded under the manager that answered.
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/gateway Gateway runner, session dispatch, delivery duplicate This issue or pull request already exists labels Jul 27, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #34734: both add a LoadState gate before trusting TimeoutStopUSec from systemctl. #34734 is the broader canonical guard because it rejects every non-loaded state, not only not-found.

@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 adding focused manager-selection regression coverage; the false-positive premise is verified on current main at gateway/shutdown_forensics.py:367-389.

Problems

  • gateway/shutdown_forensics.py:399 rejects only LoadState=not-found, while the new comment at gateway/shutdown_forensics.py:374-375 says the result must be trusted only when loaded. This accepts any other or absent LoadState if TimeoutStopUSec parses. The existing canonical duplicate, #34734, uses the stricter load_state != "loaded" rejection.

Suggested changes

  • Require LoadState == "loaded" before assigning timeout_us, matching #34734.
  • Preserve these useful mock tests and add a non-loaded LoadState fallback case alongside tests/gateway/test_shutdown_forensics.py:283-360.

Automated hermes-sweeper review.

timeout_us = _parse_systemd_duration_to_us(value)
if timeout_us is not None:
break
if load_state == "not-found" or value is None:

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.

This should require load_state == "loaded", not merely reject not-found: the surrounding comment says values are trusted only for loaded units, and canonical duplicate #34734 uses that stricter guard. Otherwise any other or absent LoadState with a parseable timeout is accepted.

@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 30, 2026
Unblocks contributor-check on PR #2 — the only failing gate; all 27
substantive checks pass.
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 duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants