fix(slots): route slot artefact names through the naming seam (#1436) - #2298
thinmintdev wants to merge 1 commit into
Conversation
…through the naming seam (#1436) The two /api/slots/{name}/logs routes, the post-unload reset-failed in backends.py, cached_aie_columns in npu.py and collect_local in metrics_collect.py formatted the mutable slot NAME into hal0-slot@<name>.service / hal0-slot-<name>. On an id-keyed box the real artefacts are keyed by id, so logs came back empty, reset-failed was a silent no-op and the NPU probe missed. Add slot_token_for() to the seam (reads the slot config, falls back to the name) and use slot_unit_name / slot_container_name at every site. Add a grep-guard test forbidding such f-strings outside slots/naming.py. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017bDysVfpfD2VTJfPcQwCcM
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
thinmintdev
left a comment
There was a problem hiding this comment.
Independent review of 39a2606. Verdict: APPROVE. (Posted as a comment review because GitHub does not let the PR author account approve its own PR.)
1. The four reported sites, plus one more
All four sites named in #1436 now get their names from the naming seam (slot_unit_name / slot_container_name applied to slot_token_for(...)):
src/hal0/api/routes/slots.py:1796:GET /{name}/logssrc/hal0/api/routes/slots.py:1867:/{name}/logs/stream. The unit is resolved before the generator runs.src/hal0/api/routes/backends.py:436: reset-failed. The token is resolved beforedelete(), which matters because the config is gone after the delete. The test checks this ordering by makingget_configraise after the delete.src/hal0/api/routes/npu.py:241: the AIE-column probe.
The fifth site, src/hal0/slots/metrics_collect.py:389,401, has the same root cause: the name is formatted into hal0-slot@/hal0-slot-, which gives empty memory and uptime on id-keyed boxes. The issue explicitly asked for a grep guard that would catch this kind of site, so I don't count it as scope creep.
grep -rn 'hal0-slot[@-]' src/hal0 outside naming.py now finds only these:
- docstrings and comments
- operator hint and error text, which the guard allow-lists
capacity.py:384, which already goes through its own token resolution (_artefact_token,capacity.py:455)- plain string constants for the seeded
img/flmslots (see the follow-ups below)
2. Root cause
slot_token_for (src/hal0/slots/naming.py:79) calls manager.get_config(name) and passes the result to slot_instance_token. This is the same shape as #1431 and the existing callers (slot_view/__init__.py:689, services_health.py:135, manager.py:1567).
The PR uses the config's id rather than Slot.slot_id, and that is the right choice. _stamp_id (manager.py:941) stamps an id even on name-keyed boxes. capacity._artefact_token (capacity.py:455-469) documents the same trap.
3. The tests fail without the fix
I restored the four changed route and metrics source files from origin/main and kept the test file and naming.py from the PR head. Result: 6 failed, including the guard listing api/routes/backends.py:447: f-string 'hal0-slot@'. With HEAD restored: 6 passed.
4. Regressions
- On a name-keyed slot, the config has no
id, soslot_instance_tokenfalls back tocfg["name"]or[slot].name. The result is the same unit as before. For an alias such asprimary, the result is now the canonical name, which is the real unit. - If
get_configraises (SlotNotFound, a manager withoutget_config), the code falls back to the bare name, which is the old behaviour. get_configuses an mtime-keyed parse cache, so the extra read per slot incollect_localandnpu_occupancyis cheap.- The guard walks the AST and only inspects
JoinedStrconstant parts. Docstrings, comments and plain strings cannot trip it. The allow-list matches on a (file, fragment) pair, not on a line number, so it will not break when lines move.
5-6. Scope and forbidden areas
The diff touches 5 files under src/hal0 and 1 new test file. It does not touch CHANGELOG.md, .github/, migrations, credentials or infrastructure.
7. Gates
ruff format --check src tests: 1305 files already formattedruff check src tests: All checks passed!pytest tests/ -q -m "not integration":8 failed, 13191 passed, 26 skipped, 1 xfailed. All 8 failures (doctor ports, the installer unwritable-dir and kfd tests, seam_check non-root wrapper, updater privileged_seam/wrapper_refresh) also fail onorigin/mainin this sandbox. They come from running as uid 0 and are unrelated to this diff.- DCO:
Signed-off-by:is present.
Non-blocking follow-ups (suggest a separate issue)
- Some plain string constants still hard-code the seeded slot name and would miss on an id-keyed box:
comfyui.py:62_IMG_UNIT,installer.py:558,flm_catalog.py:54hal0-slot-flm. The guard does not catch these because it only looks at f-strings and they are plain strings. cli/slot_commands.py:223,529printjournalctl -u hal0-slot@{name}/restart of hal0-slot@{name}.servicehints. On an id-keyed box those hints point at a unit that doesn't exist. The guard allow-lists them rather than fixing them.- No test covers the name-keyed fallback of
slot_token_for(a config withoutid, orget_configraising). The fallback is simple, but a two-line unit test would lock it in.
Generated by Claude Code
What changed
Every remaining site that formatted the mutable slot NAME into a unit/container name now goes through the naming seam (
slot_unit_name/slot_container_nameover the instance token):src/hal0/api/routes/slots.py/{name}/logsand/{name}/logs/stream(weref"hal0-slot@{name}.service")src/hal0/api/routes/backends.pyunload_npu_modelreset-failed (wasf"hal0-slot@{slot_name}.service"). The token is resolved BEFOREdelete(), since the config is gone afterwards.src/hal0/api/routes/npu.pycached_aie_columns(...)(wasf"hal0-slot-{s.name}")src/hal0/slots/metrics_collect.pycollect_local(hal0-slot@{slot.name}.serviceandhal0-slot-{slot.name}) — a fifth site not listed in the issue; the grep-guard flagged it and it has the same root cause (empty memory/uptime on id-keyed boxes).New helper
slot_token_for(manager, name)insrc/hal0/slots/naming.py: readsmanager.get_config(name), appliesslot_instance_token, falls back to the name when the config cannot be read (pre-migration / deleted slot).Deviation from the issue text: npu.py and metrics_collect read the config rather than
Slot.as_dict()["id"].Slot.slot_idis stamped from the identity store (manager.py:_stamp_id) even on name-keyed boxes whose artefacts have not been migrated yet, so using it would target non-existenthal0-slot@<id>artefacts before migration. The configid(_slot_id_if_id_keyed) is the correct signal.Why
Closes the second half of #1417 / #1431: on an id-keyed box the real artefacts are
hal0-slot@<id>.service/hal0-slot-<id>, so logs were empty, reset-failed was a silent no-op and the NPU probe missed.Tests
tests/api/test_slot_artefact_names.py: one test per site with an id-keyed config (name="brain",id=7) assertinghal0-slot@7.service/hal0-slot-<id>, plus an AST-based grep guard (f-strings only; docstrings, comments and plain strings are ignored) forbiddinghal0-slot@/hal0-slot-f-strings outsideslots/naming.py. Allow-list (each commented in the test): CLI hint text incli/slot_commands.py, the watchdog task label, validation error text insystem/seam.py, and log text inbench/harness.py(id parsed from a reallist-unitsname).src/:uv run pytest tests/api/test_slot_artefact_names.py -q-> 6 faileduv run ruff format --check src tests-> 1305 files already formatteduv run ruff check src tests-> All checks passed!uv run pytest tests/ -q -x -m "not integration"-> 4201 passed, then stopped attests/cli/test_doctor.py::test_preflight_ports_soft_mode_downgrades_to_warning; that test andtests/system/test_seam_check.py::test_non_root_owned_wrapper_is_reportedfail for sandbox reasons (running as root / no port listener detection) in files this PR does not touch. Of the directories aftertests/cli, only cli, slots, slot_view, slot_config, providers, system and dispatcher were re-run (3292 passed, with the doctor test deselected).Closes #1436
🤖 Generated with Claude Code
https://claude.ai/code/session_017bDysVfpfD2VTJfPcQwCcM
Generated by Claude Code