fix(serve): keep a public endpoint authenticated across respawns

[volen-silo]

• If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. (No EAI-7409 rows exist; endpoint auth has never had e2e coverage.)

Problem

Public-endpoint authentication (EAI-7409, added in #124) held for the initial rocm serve, but not for any path that respawns a recorded service. Those paths treated "a key file exists" as the source of truth. The real invariant is a property of the recorded host: a non-loopback bind must always be authenticated.

Reachable through first-class commands:

rocm serve --host 0.0.0.0 --allow-public-bind   # key minted, stored 0600, printed
rocm services stop <id>                         # key file deleted
rocm services restart <id>                      # comes back with NO key

restart_internal_managed_service captured None for the preserved key, stored nothing, and spawned the engine child with no ROCM_SERVE_API_KEY_FILE, so the service returned on 0.0.0.0 completely unauthenticated. rocm services restart documents its argument as an id from rocm services list --all, which includes stopped records, so this is the documented flow rather than an edge case. rocmd's recovery supervisor had the same gap.

Approach

Enforce the invariant at each spawn site and fail closed, following the existing ensure_public_bind_engine_supported precedent. Fail-closed rather than mint-and-print a replacement key: restart_server is also an automation/sandbox tool whose JSON result can land in agent transcripts, so regenerating would add a secret-disclosure surface.

• rocm_engine_protocol::is_public_bind_host is now the single classification, so rocm and rocmd cannot disagree about a recorded host. Both crates' is_loopback_host delegate to it.
• Refuse a keyless public respawn in restart_internal_managed_service (before the stop, so a refused restart leaves a running service running), in spawn_managed_engine_child, and in rocmd's supervise_service (before the manifest write, so a refusal does not clobber restart_count/timestamps).
• The guards check key validity, not file existence — the engine adapters resolve the key with endpoint_api_key_from_file and enforce nothing when it yields None, so an empty file would otherwise pass the guard and still serve anonymously.
• A stop drops the key only once it has confirmed termination. An unconfirmed stop may have left the engine alive and still enforcing the key, and discarding the only copy locked the CLI's own probes, chat, and discovery out of a healthy service. The deferred cleanup lands on the liveness refresh that later observes the process dead, so no plaintext secret is stranded.
• Daemon recovery refuses at the recovery boundary with a recorded restart_managed_service_refused event, rather than letting a permanent failure propagate out of evaluate_watchers and take every watcher down on each 30s tick.

Loopback behavior is unchanged: it stays credential-free.

Behavior change

A public-host service whose key is gone (including records predating #124) is no longer restartable in place; relaunch it with rocm serve --host <host> --allow-public-bind to issue a new key. The refusal message says so. This is the intended cost of fail-closed.

Known gap

rocmd's own stop_managed_service still clears the key unconditionally: its terminate_process reports only that a signal was accepted, so it has no confirmed-termination notion to gate on. Giving it one is a larger change than this fix.

Testing

cargo fmt --all -- --check and cargo clippy --workspace --all-targets -- -D warnings clean. cargo test --workspace --no-fail-fast passes except two pre-existing rocm-core proc_lifecycle::tests::tree_* failures, which fail on WSL2 for process-tree signalling reasons unrelated to this change (untouched file).

Not run locally: GPU acceptance (scripts/vllm_therock_gpu_test.py) and the GPU e2e lanes — no GPU on this host. The end-to-end assertion that a restarted public server actually rejects an unauthenticated request needs a live engine; endpoint auth has no e2e coverage today, so these paths are unit-tested only. Filters are listed in docs/testing.md.

Closes EAI-7409.

[rominf]

Review notes (automated review pass)

Reviewed b77c6609 against ca9f2974. All CI green. To be clear up front: the guard design itself is good and I could not break it — the fail-closed choice over mint-and-print, the conservative fail-public default in is_public_bind_host, the pre-stop ordering, and the validity-not-existence fix all hold up under adversarial reading, and the doc comments explaining why are unusually good. The one issue below is not in the guard; it's a separate line in the same PR that deletes the artifact the guard depends on.

---

1. Blocking — a crashed public service becomes permanently unrecoverable

The new deferred cleanup at apps/rocm/src/main.rs:13783 fires more eagerly than the comment above it suggests, and I think it defeats the PR's own goal for the recovery path.

Call path, please check my reasoning:

1. refresh_managed_service_runtime_liveness early-returns for non-live records at main.rs:13747-13749 (if !managed_service_is_live(record)). Good — an already-stopped record never re-enters. But a crashed service still carries status running/ready (nothing updated it), so managed_service_is_live is true and it does enter.
2. The endpoint probe fails, both tracked pids are dead, so has_tracked_pid && !has_live_pid at main.rs:13778 is true and clear_endpoint_api_key runs at main.rs:13783.
3. That function is called on effectively every read: load_managed_service at main.rs:12637 and load_managed_services at main.rs:13831.
4. So the key file is gone after the first rocm services list following the crash — and restart_internal_managed_service deletes it itself, because its own load_managed_service at main.rs:12864 runs the refresh before endpoint_api_key reads the key at main.rs:12869.
5. Both refusal sites then fire forever: main.rs:12872 (rocm services restart) and apps/rocmd/src/lib.rs:3089 (supervise_service).

Concrete scenario: a public service on 0.0.0.0 is running with a minted key. Engine and supervisor both die (OOM kill, host reboot of the process tree, panic). Operator runs rocm services list. Key deleted. Every restart and every daemon recovery attempt is refused from then on.

This is the part I'd flag hardest: the rocmd guard's own comment at apps/rocmd/src/lib.rs:3124-3128 says it exists so a previously-authenticated public service does not "come back up anonymous after a crash/recover cycle" — which presupposes that cycle works with the key intact. Before this PR a crash left the key file in place and recovery restored an authenticated service; after it, the same crash guarantees refusal. The "Behavior change" section only mentions stopped services, so this looks unintended.

It also mostly nullifies the new all_stopped gating at main.rs:12817-12820 — the key survives an unconfirmed stop only until the next liveness refresh observes the dead pids.

Suggestion (take or leave): defer the clear to a genuinely terminal state — when the record is pruned/deleted, or only for records already marked stopped by a confirmed stop — rather than on first observation of a dead pid. A crashed public service still has a valid recorded host and a legitimate need for its key. Happy to be told I've misread the liveness gate.

2. The recovery refusal loops every ~30s with no cap

handle_server_recover_event_with_record returns Ok via record_event at apps/rocmd/src/lib.rs:4646. That bumps last_event_unix_ms (lib.rs:4901-4904), and server_recover_due (lib.rs:4709-4720) re-arms after SERVER_RECOVER_BACKOFF_MS = 30s (lib.rs:51). Returning Ok is the right call — it correctly avoids taking every watcher down — but a missing key never self-heals and there's no dedupe, escalating backoff, or watcher-disable for this mode. Combined with #1, one crash yields an error-level restart_managed_service_refused entry in both the automation event log and the audit log every 30s indefinitely (~2,880 paired entries/day per service). Worth a terminal state for known-permanent failures.

3. No coverage for the two riskiest new behaviors

Four of the five new/changed tests call the pure ensure_public_service_has_endpoint_key(host, bool) directly. Nothing covers the conditional clear on stop (main.rs:12817-12820), the deferred clear in liveness refresh (main.rs:13783 — the source of #1), or the handle_server_recover_event_with_record refusal branch (rocmd/lib.rs:4646), which is the actual wiring behind the headline claim. A change that moved the guard after restart_managed_service would pass CI today.

restart_refuses_a_public_service_without_a_key_before_stopping_it (main.rs:20667) is sound, for the record — I checked that terminate_recorded_service_pids (main.rs:12732-12750) skips the caller's own pid, so the assertion holds for the reason the comment gives.

4. The "single classification" claim has one gap (pre-existing)

A third is_loopback_host lives at crates/rocm-dash-tui/src/llm.rs:168, matching case-insensitive localhost, ::1, and all of 127.0.0.0/8 — broader than is_public_bind_host, and not unified (dash-tui has no dependency on the protocol crate). A service on 127.0.0.2 is public to serve (key minted and enforced) but loopback to the TUI chat client (llm.rs:105-110, ui/tabs/chat.rs:370), which strips credentials and gets a 401. Pre-existing, not introduced here — only flagging it because the description asserts the classification is now singular.

---

Non-blocking

• Duplicated guard. ensure_public_service_has_endpoint_key is defined twice with divergent message text (main.rs:4459, rocmd/lib.rs:9033) while the classification it wraps was deliberately centralized. A shared helper next to is_public_bind_host would match the PR's own principle.
• Comment overstates protection. rocmd/lib.rs:3086-3089 says the guard keeps restart_count/timestamps intact — true on the refusal path, but every successful recovery has ManagedServiceRecord::new reset restart_count: 0 / last_restart_unix_ms: None before the write at lib.rs:3096. Pre-existing (confirmed against ca9f2974); just noting the new comment describes a protection that mostly isn't there.

Verified clean

Guard ordering in restart_internal_managed_service; validity-vs-existence consistent end to end (endpoint_api_key_file_if_valid, endpoint_api_key, and the engine adapters all route through endpoint_api_key_from_file); the "cannot fire on fresh launch" claim in spawn_managed_engine_child (both call sites are downstream of the key write at main.rs:4204-4206, so fresh rocm serve --host 0.0.0.0 --allow-public-bind is unaffected); both is_loopback_host rewrites are exact semantic no-ops; the supervise_service double guard is not redundant and has no TOCTOU window; the unguarded apply_endpoint_key_env in engine_request (rocmd/lib.rs:9070) is correctly exempt (stdio probe, no listener); all docs/testing.md filters resolve to real test names; dropping "and cleanup on stop" from the coverage sentence is accurate.

[volen-silo]

@rominf Thanks — #1 is a real regression and your call path is correct end to end. Fixed in 462ef46; the rest of the notes are triaged below.

1 — fixed

Confirmed exactly as you traced it: the deferred clear fired on any observation of dead tracked pids, refresh_managed_service_runtime_liveness runs on every record read, and a crashed service still carries running, so it entered the refresh and lost its key on the first rocm services list after the crash. Both refusal sites then held forever. Unintended — the "Behavior change" section only contemplated a stopped service, and as you note it contradicted the rocmd guard's own rationale.

The root cause is that dead pids alone cannot distinguish a stopped service from a crashed one, so the fix records the intent rather than inferring it:

• New ManagedServiceRecord::stop_requested_unix_ms (#[serde(default)]; None on older records, which is the safe default — keep the key).
• stop_internal_managed_service sets it when all_stopped is false, and clears it on a confirmed stop, which still drops the key inline as before.
• The liveness refresh's cleanup is now settle_pending_stop_key_cleanup, gated on that marker. The dead-pid branch only reconciles status. So a crash keeps its key and stays restartable/recoverable; an unconfirmed stop still gets its deferred cleanup once the processes are observed gone.
• It runs before the managed_service_is_live early return, so a record that reached stopped by another route still gets cleaned up instead of stranding the key.
• restart_internal_managed_service resets the marker, so a restart cannot have its own key deleted by the next refresh.

This also restores the all_stopped gating you flagged as nullified: the key now survives an unconfirmed stop until that specific stop's cleanup runs, not until any refresh observes dead pids.

The cost, stated deliberately: a crashed public service's 0600 key file outlives the process until the service is stopped or successfully restarted. That is the pre-#124 behavior and the price of keeping it recoverable — the alternative is exactly the permanent refusal you found.

3 — partially addressed

Three tests on the fixed path: a crashed public service keeps its key and still demotes to stopped; a marked record clears it once the pids are gone; the marked cleanup still runs on a record already reading stopped (that one pins the ordering against the liveness gate, so a future move below it fails). The conditional clear on a confirmed stop and the handle_server_recover_event_with_record refusal branch remain uncovered — agreed they should be, not in this commit.

2 — acknowledged, not fixed here

Your reading is right. Note the loop is narrower after the fix above, since a crashed public service no longer becomes keyless: it now requires a record that is genuinely keyless and in a recoverable status (e.g. one predating #124). The terminal state is still the right answer and it is not in this commit.

4 and non-blocking notes

• dash-tui is_loopback_host — won't-fix here. That one is a client-side credential-stripping decision over arbitrary user-configured base URLs, not a bind classification, and unifying it would change 127.0.0.0/8 handling in the TUI chat path on its own merits. You are right that the description overclaims; the sentence should read as the rocm/rocmd bind classification specifically.
• Duplicated guard — agreed, it contradicts the PR's own principle. It belongs next to is_public_bind_host with a single message.
• Comment overstating restart_count protection — agreed, the comment describes the refusal path but reads as a general claim.

Thanks for the unusually precise trace on #1 — the call path and the "mostly nullifies all_stopped" observation were both load-bearing.