fix(slm): a healthy oneshot and a templated unit must not report unknown

[mrveiss]

Closes #16019.

Single-issue rationale: #16020 is an Ansible role change that runs against live fleet nodes; this is a Python status-mapping change in the SLM backend. Different blast radius. They were one commit until the pre-push sys.modules leak guard fired on the cross-tree collection (#16024).

Thinking Path

The operator reported /slm/orchestration/per-node showing redis, slm-admin-ui and postgres as unknown while all three were healthy. Measured on that node:

postgresql            LoadState=loaded     active/exited     <- oneshot wrapper, completed
postgresql@16-main    LoadState=loaded     active/running    <- the unit doing the work
slm-admin-ui          LoadState=loaded     active/exited     <- oneshot, completed
redis-stack-server    LoadState=loaded     active/running
redis                 LoadState=not-found  inactive
redis-server          LoadState=not-found  inactive

Three independent defects, one symptom.

1. Templated units were discarded. health_collector.py:233 returned None for any unit whose name contains @. postgresql@16-main.service is the running PostgreSQL — so the filter threw away the unit doing the work and kept the wrapper that exits. A @ means instantiated from a template, not not a real service.

2. active (exited) fell off the end of the mapping. A oneshot that ran to completion matched no branch and returned unknown. That is slm-admin-ui and postgresql on every healthy node.

3. The redis unit name was not in the map. services.py listed redis-server and redis; roles/redis installs redis-stack-server, pinning the repo to jammy on Noble because Noble ships no such build.

The reading error worth recording

I first reported defect 3 as "a stale redis.service stub exists and is inactive". It does not exist. dpkg -l shows only redis-stack-server 7.4.0-v8, and there is no redis.service unit file on the node.

systemctl is-active answers inactive for a unit that does not exist — identical to one that is merely stopped. I read that as evidence of a stopped stub. LoadState is the field that separates them: not-found vs loaded.

So the same defect this PR fixes — a state the instrument cannot express folded into a neighbouring one — was in my own diagnosis of it. The operator caught it: "we were not supposed to have redis ordinary." They were right, and my evidence had never said otherwise.

What Changed

• health_collector.py — templated units are reported with their instance in the name; active/exited maps to completed; every other active, activating and deactivating sub-state maps to a live status instead of falling through.
• services.py — redis-stack-server added. The two legacy names stay: roles/redis records that a host with no Redis Stack build falls back to stock redis-server, and such a node must still be identified.

unknown now means exactly one thing: the probe got no usable answer. It previously meant that and "systemd told me something I have no branch for", which the UI renders identically — so a oneshot that succeeded was indistinguishable from an unreachable node.

Verification

health_collector_unit_states_16019_test.py — 8 passed locally under the pre-push hook.

The mapping test walks 14 (active_state, sub_state) pairs systemd actually emits and asserts none returns unknown, with a floor on the pairs examined — a shrunken table would report "nothing maps to unknown" for the same reason a correct one does.

Beside it sits the contrast, which is the one that matters: failed → failed, auto-restart → crash-loop, inactive → stopped. A mapping that answered "running" to everything would pass the first test while destroying the only signal the page exists to show.

Same for the filter: a templated unit is now kept, and a not-found phantom is still dropped. Removing the @ filter must not weaken the LoadState one.

Model Used

Opus 5

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: ff6ce842-cce5-4577-bd42-eb04f5362424

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No new hardcoded values of either class — ssot and other both block.

Known backlog in pipeline-scripts/hardcoded_values_baseline.txt is suppressed and tracked in #14371.

[mrveiss]

Review: three real defects, correctly separated, and the diagnosis is right. The new vocabulary stops one layer short of the number the operator reads.

The three-defects-one-symptom split is the right decomposition, and @ meaning instantiated from a template, not not a real service, is the kind of reading error that survives precisely because the filter looks deliberate. Recording your own is-active/LoadState misreading in the PR body — a state the instrument cannot express folded into a neighbouring one, inside the diagnosis of that same bug — is worth more than the fix.

I also checked the pytest_ignored change in tools/git-hooks/pre-push, because a hook edit that makes a guard stop firing deserves scrutiny. It is correct: --ignore genuinely does not apply to paths named on the command line, the ignore list is two narrow entries, and the change rests on pipeline-scripts/detect-agent-code-drift.sh keeping the mirror byte-identical — which it does, diff -r over the pair with a PAIRS_COMPARED floor. Aligning the hook with what CI actually collects, not weakening it.

1. ServiceStatus does not contain the values this now emits

autobot-slm-backend/models/database.py:302:

class ServiceStatus(str, enum.Enum):
    RUNNING = "running"; STOPPED = "stopped"; FAILED = "failed"
    CRASH_LOOP = "crash-loop"; UNKNOWN = "unknown"

_map_status_from_states now returns completed, starting and stopping. None are in the enum, and the PR does not touch the file.

Nothing will tell you, because the column is status = Column(String(20), ...) (:499) rather than an Enum column. All three fit in 20 characters, so they store, round-trip, and render with no error anywhere. The enum is the declared vocabulary for this field and it is now a description of what the collector used to emit.

Adding them is in scope here — this PR is what makes them reachable.

2. A completed oneshot is counted as neither running nor failed

api/monitoring.py:286 and again at :426:

if row.status == ServiceStatus.RUNNING.value:
    services_by_node[row.node_id]["running"] = row.count
elif row.status == ServiceStatus.FAILED.value:
    services_by_node[row.node_id]["failed"] = row.count

Two buckets, no else. slm-admin-ui and the postgresql wrapper report completed on every healthy node, so they land in neither.

This is not a regression — they reported unknown before and were excluded then too, so the aggregate is unchanged. That is exactly why it is worth raising: the per-node view now correctly says completed, while the count the operator reads still says those services are not running. The symptom that opened #16019 was a healthy service reported as not-healthy, and after this PR that is still true one layer up.

It is the same shape as the bug being fixed — an if/elif over an enumerated set, with everything unenumerated falling into a silent bucket. A check that enumerates conditions is blind to the conditions it does not enumerate, and completed is now one of them.

Whether that belongs in this PR is your call: it is a small, clearly-correct change (completed counts toward healthy, starting/stopping are transitional), but it is the monitoring API rather than the collector, and you have already argued blast radius for splitting #16020. I am happy to file it if you would rather keep this one narrow — say the word and I will, cross-linked to #16019.

3. Minor: the ignore filter fails open silently

ignores=$(grep -oE '^[[:space:]]*--ignore=[^[:space:]]+' pytest.ini 2>/dev/null | sed 's/.*--ignore=//')
if [ -z "$ignores" ]; then cat; return 0; fi

pytest.ini is read bare from the working directory while the hook computes REPO_ROOT="$(git rev-parse --show-toplevel)" at :15 and uses $REPO_ROOT/$f elsewhere. Git runs hooks from the toplevel so this works today, but if it ever does not, ignores is empty and the function passes everything through — restoring precisely the behaviour this change removes, with no message. $REPO_ROOT/pytest.ini costs nothing; if the file is genuinely missing, saying so is better than silently reverting.

Verdict

Fix 1 before merge — it is a few lines and the enum is the only place the vocabulary is written down. 2 either here or as a follow-up I will file. 3 is a one-word path change. The fix itself is right and the pre-push change is justified.

[mrveiss]

Review: both findings from my earlier pass are properly addressed, and the extraction is sound. One finding, and it is on the part most likely to regress.

The = → += change is the good catch here, and it is easy to miss. Once two statuses feed one bucket, assignment silently keeps whichever row arrives last — a node with 3 running and 2 completed would have reported 2. Nothing in the issue asked for that; you found it by doing the change properly.

The extraction reasoning is right and well recorded. models/__init__.py pulling in user_management → autobot_shared, which the migration runner lacks, is exactly the kind of constraint that is invisible until it fires, and putting it in the module docstring means the next person moving this file does not rediscover it by breaking Postgres 16. Re-exporting from models.database so the six importers are untouched is the right call.

test_every_state_the_collector_emits_is_a_declared_status is the closure test I would have asked for — collector output ⊆ declared enum, so a fifth state cannot appear without failing something.

The finding: the aggregates have no behavioural test at all

autobot-slm-backend/tests/api/test_completed_services_are_counted_16019.py imports exactly two things:

import ast
from pathlib import Path

It is entirely static, and _get_services_by_node / get_node_metrics are called by nothing in the test suite — git grep -l "_get_services_by_node" -- autobot-slm-backend/tests returns empty.

So the assertion protecting the aggregate fix is:

assert source.count("ServiceStatus.COMPLETED.value") == 2

Revert += to = and that still passes. The source still mentions COMPLETED twice; only the arithmetic changed. The subtlest and most valuable part of this PR — the one thing here that a careful person could plausibly undo while "simplifying" — is the one thing no test can see.

This is the shape you and I have both been holding each other to today, and I would not raise it except that the fix is genuinely good and deserves a test that can defend it. The behavioural test is small: feed _get_services_by_node a fake result with running=3, completed=2, failed=1 for one node and assert {"running": 5, "failed": 1}. That fails on =, fails if COMPLETED is dropped from the tuple, and passes only on what you actually wrote.

Question, not a finding: STARTING and STOPPING

Both are now declared and emitted, and both are counted in neither bucket. A node mid-restart reports services that are neither running nor failed, so the aggregate does not sum to the total.

That may be exactly right — a starting service genuinely is not running yet. But the code path being fixed here is one that dropped a state into a silent bucket, and the fix enumerates one of the three new states while leaving two in the same position. If it is deliberate, a line saying so where the buckets are built would stop the next reader re-deriving it.

Minor

The bucket is named running and now contains completed oneshots, so the operator-facing label is slightly untrue — it means "healthy". Not worth renaming a response field for, but worth knowing it is a count of health, not of resident processes.

Verdict

Merge-ready on correctness. I would add the four-line behavioural test first, because without it the += is undefended and it is the part I would most expect to be lost.