Skip to content

fix(process): drop OOMPolicy from systemd-run --scope argv (breaks cron worker dispatch on Linux) - #102357

Closed
gkd2323c wants to merge 1 commit into
NousResearch:mainfrom
gkd2323c:fix/cron-systemd-scope-oompolicy
Closed

gkd2323c wants to merge 1 commit into
NousResearch:mainfrom
gkd2323c:fix/cron-systemd-scope-oompolicy

Conversation

@gkd2323c

@gkd2323c gkd2323c commented Sep 3, 2026

Copy link
Copy Markdown

Problem

Since #70716's restart-safe worker isolation landed (and 83efdf5 widened the Linux path to supervised gateways via not _IS_LINUX), every cron job dispatched from a supervised systemd gateway fails with:

cannot create restart-safe systemd scope for gateway child: systemd-run --user --scope is unavailable

Root cause

_systemd_run_user_scope_available() probes with systemd-run --user --scope passing --property OOMPolicy=kill. But OOMPolicy is a service-unit property — transient scopes reject it:

$ systemd-run --user --scope --property OOMPolicy=kill -- /bin/true
Unknown assignment: OOMPolicy=kill

The probe therefore always returns non-zero in any environment where systemd-run --user --scope actually works, the negative result is cached (TTL), and restart_safe_gateway_child_argv() raises — failing every restart-safe cron worker dispatch.

Same invalid property is in _build_systemd_scope_argv(), so even if the probe were bypassed the real worker spawn would fail identically.

Fix

Drop OOMPolicy=kill from both the probe and the scope argv builder. MemoryAccounting=yes + MemoryMax remain; a transient scope has no service manager acting on the OOM event, so the property was a no-op intended for service units only.

Verified: probe returns True after the change, cron dispatch completes (job history: failed at 01:19/01:22 → ok at 01:26).

OOMPolicy is a service-unit property; systemd-run --user --scope
rejects it with 'Unknown assignment: OOMPolicy=kill'. The availability
probe therefore always failed in supervised Linux gateways, making
restart-safe cron worker dispatch (systemd scope) permanently
unavailable and falling back to in-cgroup workers.

MemoryMax + MemoryAccounting remain; OOMPolicy adds nothing for a
transient scope (no service manager to act on the OOM event).
@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery comp/tools Tool registry, model_tools, toolsets backend/local Local shell execution sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Sep 3, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

PR #102357 — drop OOMPolicy from systemd-run --scope argv

Verdict: Reasonable one-line-class fix (unsupported property fails the whole systemd-run, breaking cron dispatch on Linux). Non-blocking notes below.

What the change does

  • tools/process_registry.py:235,316 — removes --property OOMPolicy=kill from both the availability probe argv and the worker scope argv. MemoryMax containment stays.

Non-blocking

  1. No test in this diff — understandable (depends on host systemd), but flagging that verification is manual: confirm dispatch works on an affected host (user-scope systemd without OOMPolicy support) and that scopes still get MemoryMax applied.
  2. Behavior nuance: without an explicit policy, OOM handling falls back to the manager default for scopes (stop, typically) rather than kill. Slightly different kill semantics for runaway workers, but strictly better than dispatch failing outright. If per-worker OOM kill matters, consider re-adding it conditionally behind a support probe later.

Tests: None; manual host verification is the appropriate gate.

@andrexibiza andrexibiza 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.

Reviewed exact head 280fbe97e2ae8e1d9d957ab75abdd06e9e065582 against current main 63279301bcbdc185c1b07b98a9312eb0c862f26d.

The production change is the correct narrow carrier for #102486. It removes OOMPolicy=kill from both the required systemd-run --user --scope probe and the real worker-scope argv while leaving MemoryAccounting=yes, finite MemoryMax, and the fail-closed RuntimeError branches untouched. That separates rejection of an optional property from actual absence of the required process-isolation substrate. This should compose before #102431; that PR asks a different policy question about genuinely unavailable isolation and is not a substitute for fixing this false-negative probe.

BLOCKER — the exact-head acceptance contract still requires the deleted property, so this PR's own deterministic test is red.

tests/tools/test_process_registry.py::TestSystemdCgroupIsolation::test_wraps_in_systemd_scope_when_supervisor_and_available still contains:

assert "OOMPolicy=kill" in properties

This head necessarily violates that assertion. The exact-head Python tests / Run tests check is failed, which in turn leaves All required checks pass failed. The passing lint, E2E, Windows, macOS, Docker, Nix, supply-chain, and attribution checks do not supersede that red required check.

Please update the existing scope-argv test to retain the required assertions for MemoryAccounting=yes and finite MemoryMax, and explicitly assert that OOMPolicy=kill is absent. Also make test_systemd_run_user_scope_available_caches_after_probe inspect its captured probe argv and assert the same absence there; otherwise only the real builder site is protected and the probe can regress independently. These are host-independent argv tests and directly encode the capability boundary this fix establishes.

Graph / closure: add Fixes #102486 to the PR body so the affected-host report closes through this implementation carrier. Keep #102431 separate: genuine failure to establish systemd-run --user --scope must continue to fail closed unless that broader safety policy is resolved on its own merits.

Once the deterministic test contract is repaired and every check on the resulting exact head is green, I see no remaining blocker in this narrow change.

kiwipaulrob added a commit to kiwipaulrob/hermes-agent that referenced this pull request Sep 6, 2026
A systemd-supervised gateway (INVOCATION_ID set) with no user D-Bus
session — containers, minimal LXCs, macOS-style supervisors — fails
EVERY scheduled job at dispatch: restart_safe_gateway_child_argv()
raises and run_one_job() records a failure. The only symptom is
silently skipped executions (a missed nightly backup, dead watchdogs).

Degrade to a direct external subprocess with a warning instead of
raising, unless HERMES_GATEWAY_CHILD_REQUIRE_SCOPE=1 re-enables
fail-closed. Degraded jobs keep process separation and the full
NousResearch#101940 ownership handoff; only cgroup isolation is lost.

The dispatch is a GatewayChildDispatch (in_process / scoped /
degraded) so the degraded case can never collapse into the
'not managed, stay in-process' sentinel — the exact failure mode
that would recreate the restart-interruption edge NousResearch#101940 closed.

Also drops OOMPolicy=kill from scope argv: it is a service-unit
property that transient scopes reject ('Unknown assignment'), so the
probe always failed where scopes actually work (NousResearch#102357, by gkd2323c,
incorporated here with credit).

Co-authored-by: gkd2323c (probe fix from NousResearch#102357)
kshitijk4poor added a commit to kshitijk4poor/hermes-agent that referenced this pull request Sep 6, 2026
…MPolicy too

Requested in the review of NousResearch#102357: the spawn builder was pinned, the probe
was not, and the probe is where the rejection was cached as "unavailable".
kshitijk4poor added a commit that referenced this pull request Sep 6, 2026
…MPolicy too

Requested in the review of #102357: the spawn builder was pinned, the probe
was not, and the probe is where the rejection was cached as "unavailable".
@kshitijk4poor

Copy link
Copy Markdown

Merged via #104152 (rebase) — your commit is on main as 611ee856c5 with your authorship intact. It landed as a single deletion: after the #102117 refactor both the probe and the real spawn build their argv through one shared _systemd_scope_argv helper, so the two-site change you wrote against the older layout collapsed to one line. On top I flipped the spawn-argv test to assert no OOMPolicy= property is emitted, added the same pin on the captured probe argv, and put the WHY in the helper's docstring.

@andrexibiza — both requests from your review are in the merged head: the spawn test keeps its MemoryAccounting=yes / finite MemoryMax assertions and asserts OOMPolicy= is absent (04885c25cf), and test_systemd_run_user_scope_available_caches_after_probe now inspects the captured probe argv for the same absence (3513a3b922). Fixes #102486 was in the carrier body; #102486 and #103693 closed on merge. #102431 was left open and separate, as you argued.

Verified before merge with an A/B probe of _systemd_scope_argv(...): origin/main emitted ['MemoryAccounting=yes', 'MemoryMax=…', 'OOMPolicy=kill'], the branch emits the first two only. Thank you @gkd2323c — earliest and the minimal fix in a six-PR cluster.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend/local Local shell execution comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery comp/tools Tool registry, model_tools, toolsets P1 High — major feature broken, no workaround sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants