Skip to content

shutdown_forensics: don't trust 'systemctl --user' defaults for system units - #34734

Open
rp-keeran wants to merge 1 commit into
NousResearch:mainfrom
rp-keeran:fix/systemd-timing-check-loadstate
Open

shutdown_forensics: don't trust 'systemctl --user' defaults for system units#34734
rp-keeran wants to merge 1 commit into
NousResearch:mainfrom
rp-keeran:fix/systemd-timing-check-loadstate

Conversation

@rp-keeran

Copy link
Copy Markdown

check_systemd_timing_alignment queried systemctl --user show <unit> first, which returns built-in defaults (TimeoutStopUSec=90s) with exit 0 even when the unit isn't loaded for the user manager. For system-managed units this produced a false "stale unit / TimeoutStopSec=90s" warning. Now requires LoadState=loaded before trusting the value.

…m units

check_systemd_timing_alignment queried 'systemctl --user show <unit>'
first, which returns built-in defaults (TimeoutStopUSec=90s) with exit 0
even when the unit isn't loaded for the user manager. For system-managed
units this produced a false 'stale unit / TimeoutStopSec=90s' warning.
Require LoadState=loaded before trusting the value.
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating the systemd manager-scope false positive. The production guard is narrowly targeted and its diff applies cleanly to the current gateway/shutdown_forensics.py blob.

Problems

  • tests/gateway/test_shutdown_forensics.py:237-250 only verifies early returns. It does not mock subprocess.run to cover the new behavior: reject LoadState=not-found from the first manager, then accept the loaded fallback manager.

Suggested changes

  • Add regression tests for an unloaded --user result followed by a loaded system result, plus a loaded user-scope happy path. Assert the selected timeout comes only from the loaded manager.

Automated hermes-sweeper review.

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 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants