fix(model-router): stop/recover router on uninstall & onboard; extend gateway health wait - #5230
Closed
tyeth-ai-assisted wants to merge 4 commits into
Closed
Conversation
Fixes NVIDIA#5169: uninstall.sh left a running model-router process on port 4000, causing reinstall to fail with "Port 4000 already has a healthy router endpoint, but its credential state is unknown." Changes: - Add stop-model-router action to the Stopping services uninstall step; reads routerPid from onboard-session.json and falls back to lsof :4000 scan, mirroring the Ollama auth proxy stop pattern (NVIDIA#2759) - Raise wait_for_hermes_gateway_internal attempts from 45 to 60 in agents/hermes/start.sh so the gateway health wait budget covers ~120s of effective curl timeout (was ~90s) on slow WSL2 hosts Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
When onboarding starts fresh (no recorded routerPid) but a model-router process is already healthy on port 4000 from a previous failed install, reconcileModelRouter now scans /proc to find and stop the orphan before starting a new router — instead of throwing "credential state is unknown" and requiring a manual stop-and-retry. See issue NVIDIA#5169. Adds findModelRouterPidForPort to model-router-process.ts with injectable deps (listProcPids, readProcCommandLine) for unit testing, guarded by the existing isModelRouterCommandLineForPort check so non-router services on the same port are never killed. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…d ps (NVIDIA#5169) model-router runs as a Python venv entry-point script, so the OS interposes the interpreter: /proc cmdline is 'python /path/model-router proxy --port …' with args[0]=python and args[1]=model-router. The previous isModelRouterCommandLineForPort check only tested args[0], causing both the uninstall orphan-stop and the onboard orphan-recovery to silently skip the running process. Fix: - isModelRouterCommandLineForPort: check args[0] OR args[1] basename === model-router - isModelRouterPid (uninstall run-plan): same dual-position check via ps - Update all relevant tests to use the realistic Python-interpreter cmdline (python /path/model-router proxy --port 4000) to exercise the fixed path Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughModel-router detection and shutdown added to uninstall and onboarding reconciliation; Hermes startup now ensures an API_SERVER_KEY in its .env and increases gateway health-probe retries. ChangesModel Router Lifecycle Management
Hermes Gateway API Server Key Injection
Sequence Diagram(s)sequenceDiagram
participant ComponentA
participant ComponentB
ComponentA->>ComponentB: observable interaction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
2 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agents/hermes/start.sh (1)
676-691:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAccept
401as a healthy Hermes gateway response.The loop still uses
curl -sf, so a live gateway that returns401from/healthwill burn through all 60 retries and abort startup.src/lib/verify-deployment.tsalready treats200and401as alive for this endpoint, so the retry increase does not fix that false-negative path.Suggested fix
wait_for_hermes_gateway_internal() { local gateway_pid="$1" local attempts=0 + local http_code while [ "$attempts" -lt 60 ]; do - if curl -sf --max-time 2 "http://127.0.0.1:${INTERNAL_PORT}/health" >/dev/null 2>&1; then + http_code="$( + curl -so /dev/null -w '%{http_code}' --max-time 2 \ + "http://127.0.0.1:${INTERNAL_PORT}/health" 2>/dev/null || echo 000 + )" + if [ "$http_code" = "200" ] || [ "$http_code" = "401" ]; then return 0 fi if ! kill -0 "$gateway_pid" 2>/dev/null; then wait "$gateway_pid" return $?As per coding guidelines, Hermes health-wait changes should stay aligned with the nightly E2E expectations, and the downstream gateway probe already counts
401as alive.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agents/hermes/start.sh` around lines 676 - 691, The health check in function wait_for_hermes_gateway_internal incorrectly treats any 4xx as failure because it uses curl -sf; change the check to capture the HTTP status (e.g., use curl -s --max-time 2 -o /dev/null -w '%{http_code}' "http://127.0.0.1:${INTERNAL_PORT}/health") and consider the gateway healthy if the returned status is 200 or 401; keep the existing kill -0/gateway_pid logic and return behavior unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agents/hermes/start.sh`:
- Around line 972-1003: Record the initial ownership/mode of the .env (and
compat_hash/hash_file if relevant) before you mutate them and restore that
original state at the end instead of always chowning/chmodding to sandbox:
sandbox and 640; specifically, in the start.sh block that writes API_SERVER_KEY
(variables env_file, hash_file, compat_hash, HERMES_DIR), capture the original
owner/group and permission bits into local vars (e.g., original_owner,
original_group, original_mode) before any chown/chmod, and when the script
finishes the sha256/write section restore those saved values (use chown/chmod to
the saved owner/group/mode) so that hermes_config_root_is_locked() and
restore_hermes_config_permissions_after_dashboard_start() see the preserved
locked posture rather than always reverting to sandbox:640.
In `@src/lib/onboard/model-router-process.test.ts`:
- Around line 57-68: The test title and implementation diverge: "skips
non-integer entries from the PID list" currently uses an empty list and only
checks that readProcCommandLine isn't called. Update the test for
findModelRouterPidForPort to supply a mixed PID array (e.g., include non-numeric
values like NaN/"abc"/Infinity alongside a valid PID like 100) via listProcPids
and have readProcCommandLine record which PIDs were invoked and return a
matching command line for the valid PID; then assert the function returns the
valid PID and that only the valid finite PID(s) were passed to
readProcCommandLine, thereby exercising the Number->isFinite filtering in
model-router-process.ts.
---
Outside diff comments:
In `@agents/hermes/start.sh`:
- Around line 676-691: The health check in function
wait_for_hermes_gateway_internal incorrectly treats any 4xx as failure because
it uses curl -sf; change the check to capture the HTTP status (e.g., use curl -s
--max-time 2 -o /dev/null -w '%{http_code}'
"http://127.0.0.1:${INTERNAL_PORT}/health") and consider the gateway healthy if
the returned status is 200 or 401; keep the existing kill -0/gateway_pid logic
and return behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 62e571bf-d881-437c-a154-d4db814b776f
📒 Files selected for processing (8)
agents/hermes/start.shsrc/lib/actions/uninstall/run-plan.test.tssrc/lib/actions/uninstall/run-plan.tssrc/lib/domain/uninstall/plan.test.tssrc/lib/domain/uninstall/plan.tssrc/lib/onboard/model-router-process.test.tssrc/lib/onboard/model-router-process.tssrc/lib/onboard/model-router.ts
Comment on lines
+972
to
+1003
| if [ "$(id -u)" -eq 0 ]; then | ||
| chown root:sandbox "$env_file" || return 1 | ||
| chmod 640 "$env_file" || return 1 | ||
| chmod u+w "$hash_file" || return 1 | ||
| [ ! -f "$compat_hash" ] || chmod u+w "$compat_hash" 2>/dev/null || true | ||
| elif [ ! -w "$env_file" ]; then | ||
| echo "[config] Cannot inject API_SERVER_KEY — .env not writable (non-root mode); Hermes api_server will fail" >&2 | ||
| return 0 | ||
| fi | ||
|
|
||
| local new_key | ||
| new_key=$(python3 -c "import secrets; print(secrets.token_hex(32))") | ||
| printf 'API_SERVER_KEY=%s\n' "$new_key" >>"$env_file" | ||
| echo "[config] Generated missing API_SERVER_KEY for Hermes API server (Hermes v0.16.0+ requirement)" >&2 | ||
|
|
||
| local _write_rc=0 | ||
| if sha256sum "${HERMES_DIR}/config.yaml" "${HERMES_DIR}/.env" >"$hash_file"; then | ||
| chown root:root "$hash_file" 2>/dev/null || true | ||
| chmod 444 "$hash_file" 2>/dev/null || true | ||
| if [ -f "$compat_hash" ]; then | ||
| sha256sum "${HERMES_DIR}/config.yaml" "${HERMES_DIR}/.env" >"$compat_hash" || _write_rc=$? | ||
| chown sandbox:sandbox "$compat_hash" 2>/dev/null || true | ||
| chmod 600 "$compat_hash" 2>/dev/null || true | ||
| fi | ||
| else | ||
| _write_rc=$? | ||
| fi | ||
|
|
||
| if [ "$(id -u)" -eq 0 ]; then | ||
| chown sandbox:sandbox "$env_file" 2>/dev/null || true | ||
| chmod 640 "$env_file" 2>/dev/null || true | ||
| fi |
Contributor
There was a problem hiding this comment.
Preserve the shields-up lock state when rewriting .env.
If .env starts in the locked posture (root:root and non-writable), this helper always restores it to sandbox:sandbox 640. That makes hermes_config_root_is_locked() go false and restore_hermes_config_permissions_after_dashboard_start() then downgrades the whole config root back to mutable 3770, so a post-upgrade boot silently disables shields-up on previously locked sandboxes.
Suggested fix
ensure_hermes_api_server_key() {
local env_file="${HERMES_DIR}/.env"
local hash_file="${HERMES_HASH_FILE}"
local compat_hash="${HERMES_DIR}/.config-hash"
+ local env_was_locked=0
[ -f "$env_file" ] || return 0
+ if hermes_config_path_is_locked "$env_file"; then
+ env_was_locked=1
+ fi
+
grep -q "^API_SERVER_KEY=" "$env_file" 2>/dev/null && return 0
@@
if [ "$(id -u)" -eq 0 ]; then
- chown sandbox:sandbox "$env_file" 2>/dev/null || true
- chmod 640 "$env_file" 2>/dev/null || true
+ if [ "$env_was_locked" -eq 1 ]; then
+ chown root:root "$env_file" 2>/dev/null || true
+ chmod 444 "$env_file" 2>/dev/null || true
+ else
+ chown sandbox:sandbox "$env_file" 2>/dev/null || true
+ chmod 640 "$env_file" 2>/dev/null || true
+ fi
fiBased on learnings, the normal 640 sandbox:sandbox mutable posture is intentional here, but the locked integrity posture is supposed to remain distinct and preserved across startup mutations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agents/hermes/start.sh` around lines 972 - 1003, Record the initial
ownership/mode of the .env (and compat_hash/hash_file if relevant) before you
mutate them and restore that original state at the end instead of always
chowning/chmodding to sandbox: sandbox and 640; specifically, in the start.sh
block that writes API_SERVER_KEY (variables env_file, hash_file, compat_hash,
HERMES_DIR), capture the original owner/group and permission bits into local
vars (e.g., original_owner, original_group, original_mode) before any
chown/chmod, and when the script finishes the sha256/write section restore those
saved values (use chown/chmod to the saved owner/group/mode) so that
hermes_config_root_is_locked() and
restore_hermes_config_permissions_after_dashboard_start() see the preserved
locked posture rather than always reverting to sandbox:640.
Source: Learnings
Comment on lines
+57
to
+68
| it("skips non-integer entries from the PID list", () => { | ||
| let called = false; | ||
| const pid = findModelRouterPidForPort(4000, { | ||
| readProcCommandLine: () => { | ||
| called = true; | ||
| return null; | ||
| }, | ||
| listProcPids: () => [], | ||
| }); | ||
| expect(pid).toBe(null); | ||
| expect(called).toBe(false); | ||
| }); |
Contributor
There was a problem hiding this comment.
Test name doesn't match implementation.
The test is named "skips non-integer entries from the PID list" but it provides an empty listProcPids: () => [] and verifies that readProcCommandLine is never called. This tests empty-list behavior, not non-integer filtering.
The production code's non-integer filtering happens at model-router-process.ts:159-162 where /proc entries are mapped to Number then filtered by isFinite. To properly test that logic, provide a mixed PID list like [1, NaN, 100] or ["abc", 100, "def"] and verify only valid PIDs are processed.
📝 Suggested test improvement
- it("skips non-integer entries from the PID list", () => {
+ it("returns null when the PID list is empty", () => {
let called = false;
const pid = findModelRouterPidForPort(4000, {Or add a separate test:
it("skips non-integer entries from the PID list", () => {
const calledPids: number[] = [];
const pid = findModelRouterPidForPort(4000, {
readProcCommandLine: (p) => {
calledPids.push(p);
return p === 100
? ["/opt/model-router", "proxy", "--port", "4000"]
: null;
},
listProcPids: () => [NaN, 100, Infinity, -1, 0] as any,
});
expect(pid).toBe(100);
expect(calledPids).toEqual([100]); // Only the valid finite positive integer
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/onboard/model-router-process.test.ts` around lines 57 - 68, The test
title and implementation diverge: "skips non-integer entries from the PID list"
currently uses an empty list and only checks that readProcCommandLine isn't
called. Update the test for findModelRouterPidForPort to supply a mixed PID
array (e.g., include non-numeric values like NaN/"abc"/Infinity alongside a
valid PID like 100) via listProcPids and have readProcCommandLine record which
PIDs were invoked and return a matching command line for the valid PID; then
assert the function returns the valid PID and that only the valid finite PID(s)
were passed to readProcCommandLine, thereby exercising the Number->isFinite
filtering in model-router-process.ts.
Thabhelo
added a commit
to Thabhelo/NemoClaw
that referenced
this pull request
Jun 11, 2026
Uninstall and onboard both failed to stop routers started via the venv interpreter (python /path/model-router proxy). Also auto-recover orphaned routers during reconcileModelRouter when session PID is stale. Incorporates NVIDIA#5169-relevant pieces from NVIDIA#5230; Hermes gateway changes left out of scope.
12 tasks
Contributor
|
Thanks @tyeth-ai-assisted — incorporated the #5169-relevant pieces (Python venv cmdline detection, Left the Hermes |
wscurran
added
area: cli
Contributor
|
✨ Thanks for addressing the model-router cleanup on uninstall and the orphaned router recovery during onboard. This proposes a way to stop the router on uninstall, auto-recover orphaned routers on fresh onboard, and extend the Hermes gateway health wait with three focused commits. Related open issues: |
Collaborator
|
@tyeth-ai-assisted could you add a DCO to the PR body text, please? |
Contributor
Author
@cv you still want this despite merging the alternative? Edit - sorry my mobile app didn't update the issue content until I replied, DOH! |
This was referenced Jul 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the model-router failing to stop/recover during uninstall and on fresh onboard, plus a longer Hermes gateway health wait. Three clean commits on top of upstream
main.Commits
Testing
npm install && npm test && make checkunder Node 22 before merge.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests