fix(inference): recover dual-Station runtime ownership - #8158
Conversation
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
📝 WalkthroughWalkthroughDual-Station runtime ownership now uses host-global state. Onboarding recovers and revalidates existing managed pairs. Uninstall discovers receipts across supported locations, preserves related credentials, and validates filesystem state before cleanup. ChangesDual-Station ownership lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Onboarding
participant RuntimeReceipt
participant Peer
participant Uninstall
Onboarding->>RuntimeReceipt: recover installed receipt
RuntimeReceipt->>Peer: revalidate stored binding and runtime identity
Peer-->>RuntimeReceipt: validation result
RuntimeReceipt-->>Onboarding: ready, not-installed, or unsafe
Uninstall->>RuntimeReceipt: discover and validate receipt
RuntimeReceipt-->>Uninstall: cleanup plan
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 06c41bc in the TypeScript / code-coverage/cliThe overall coverage in commit 06c41bc in the Show a code coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-8158.docs.buildwithfern.com/nemoclaw |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/lib/actions/uninstall/run-plan-dual-station.test.ts (1)
88-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the preserved dual-Station entries.
This test proves discovery and cleanup for a legacy receipt location. The PR also adds
DUAL_STATION_VLLM_API_KEY_FILEtoSHARED_HOST_STATE_ENTRIESand to the gateway-scoped preserve list insrc/lib/actions/uninstall/run-plan.ts. No test in this file asserts that a gateway-scoped uninstall keepsdual-station-vllm-api-key,dual-station-vllm-runtime.json, anddual-station-vllm-runtime.json.ssh-bindingwhile sibling gateways remain. Add one test that runs the scoped path with a realrmSyncagainst the temporary state directory and asserts those three entries still exist.🤖 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/actions/uninstall/run-plan-dual-station.test.ts` around lines 88 - 148, Add a test in the uninstall plan suite covering gateway-scoped cleanup with a real temporary state directory and rmSync implementation. Create the dual-Station API key, runtime receipt, and runtime SSH-binding entries alongside a sibling gateway’s entries, run the scoped uninstall path, and assert the three dual-Station entries remain while sibling entries are removed. Anchor the test setup and assertions around runUninstallPlan.src/lib/inference/vllm-station-runtime-receipt.ts (1)
189-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOne receipt-location contract is implemented twice. Both files encode the supported dual-Station receipt locations: the host-global state directory plus numeric directories under
GATEWAYS_SUBDIR, with symlink and non-directory rejection and a 1-65535 port range. The copies already diverge in error handling, so uninstall and onboarding can disagree about which receipts exist.
src/lib/inference/vllm-station-runtime-receipt.ts#L189-L232: export the discovery helper (for exampledualStationVllmReceiptStateDirs) so it becomes the single owner of the layout rules.src/lib/actions/uninstall/run-plan.ts#L1218-L1258: import that helper inremoveManagedDualStationRuntime, delete the local regex, port-range, and symlink checks, and stop swallowingENOENTraised duringfs.readdirSynciteration so discovery failures still fail closed.🤖 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/inference/vllm-station-runtime-receipt.ts` around lines 189 - 232, Export receiptStateDirs under a shared discovery-helper name so it owns all dual-Station receipt location, validation, and error-handling rules. In src/lib/inference/vllm-station-runtime-receipt.ts lines 189-232, preserve the existing host-global and numeric gateway-directory behavior. In src/lib/actions/uninstall/run-plan.ts lines 1218-1258, import and use that helper from removeManagedDualStationRuntime, removing the duplicated regex, port-range, and symlink checks and ensuring ENOENT from readdirSync is not swallowed.src/lib/inference/vllm-api-key.ts (1)
12-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused re-export.
No module imports
DUAL_STATION_VLLM_API_KEY_FILEfrom./vllm-api-key. Keepvllm-station-runtime-receipt-path.tsas the authoritative import path.🤖 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/inference/vllm-api-key.ts` around lines 12 - 14, Remove the unused re-export of DUAL_STATION_VLLM_API_KEY_FILE from vllm-api-key.ts, while preserving the direct import and authoritative export from vllm-station-runtime-receipt-path.ts.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.
Nitpick comments:
In `@src/lib/actions/uninstall/run-plan-dual-station.test.ts`:
- Around line 88-148: Add a test in the uninstall plan suite covering
gateway-scoped cleanup with a real temporary state directory and rmSync
implementation. Create the dual-Station API key, runtime receipt, and runtime
SSH-binding entries alongside a sibling gateway’s entries, run the scoped
uninstall path, and assert the three dual-Station entries remain while sibling
entries are removed. Anchor the test setup and assertions around
runUninstallPlan.
In `@src/lib/inference/vllm-api-key.ts`:
- Around line 12-14: Remove the unused re-export of
DUAL_STATION_VLLM_API_KEY_FILE from vllm-api-key.ts, while preserving the direct
import and authoritative export from vllm-station-runtime-receipt-path.ts.
In `@src/lib/inference/vllm-station-runtime-receipt.ts`:
- Around line 189-232: Export receiptStateDirs under a shared discovery-helper
name so it owns all dual-Station receipt location, validation, and
error-handling rules. In src/lib/inference/vllm-station-runtime-receipt.ts lines
189-232, preserve the existing host-global and numeric gateway-directory
behavior. In src/lib/actions/uninstall/run-plan.ts lines 1218-1258, import and
use that helper from removeManagedDualStationRuntime, removing the duplicated
regex, port-range, and symlink checks and ensuring ENOENT from readdirSync is
not swallowed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 04a21c8f-02de-4fe3-a26c-7b7e56c5fa65
📒 Files selected for processing (10)
docs/inference/set-up-vllm-on-two-dgx-stations.mdxdocs/reference/host-files-and-state.mdxsrc/lib/actions/uninstall/run-plan-dual-station.test.tssrc/lib/actions/uninstall/run-plan.tssrc/lib/inference/vllm-api-key.tssrc/lib/inference/vllm-dual-station.test.tssrc/lib/inference/vllm-station-runtime-receipt-path.tssrc/lib/inference/vllm-station-runtime-receipt.test.tssrc/lib/inference/vllm-station-runtime-receipt.tssrc/lib/inference/vllm.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
4 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
1 additional E2E selection from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
|
Addressed all three CodeRabbit follow-ups in |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/lib/actions/uninstall/run-plan-dual-station.test.ts`:
- Around line 172-185: In the sibling-uninstall test, add an assertion that the
mocked runDualStationRuntimeCleanup function from the injected runtime is not
called. Keep the existing success and fixture-preservation assertions unchanged,
using the visible runDualStationRuntimeCleanup mock to verify shared endpoint
cleanup does not start.
🪄 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: 1ae049a9-cb73-460a-ba01-17089eb938e2
📒 Files selected for processing (5)
src/lib/actions/uninstall/run-plan-dual-station.test.tssrc/lib/actions/uninstall/run-plan.tssrc/lib/inference/vllm-api-key.tssrc/lib/inference/vllm-station-runtime-receipt-path.tssrc/lib/inference/vllm-station-runtime-receipt.ts
💤 Files with no reviewable changes (1)
- src/lib/inference/vllm-api-key.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/actions/uninstall/run-plan.ts
- src/lib/inference/vllm-station-runtime-receipt.ts
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
cv
left a comment
There was a problem hiding this comment.
Documentation writer review: changes requested for commit 37a11c9.\n\nProduct scope and security review pass. One blocking accuracy issue remains in docs/reference/host-files-and-state.mdx.\n\nThe paragraph after the dual-Station file table says all three files could exist under the legacy per-port state root. That incorrectly includes dual-station-vllm-api-key. NemoClaw v0.0.97 already stored the API key at the host-global default state root, and the current implementation discovers only the legacy runtime receipt and SSH-binding directory. Revise the paragraph to distinguish those two ownership artifacts from the host-global API key.\n\nBecause this statement concerns a credential location, repeat the documentation writer review at the new commit and refresh the receipt metadata. Then let GitHub CI validate the new commit.\n\nThe current aggregate check failure is inherited from the repository-wide reviewed dependency audit; it is not caused by this PR.
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
|
@cv Addressed the credential-location accuracy issue in |
apurvvkumaria
left a comment
There was a problem hiding this comment.
Re-reviewed the current revision. The credential-location text now correctly keeps the API key host-global while limiting legacy per-port discovery to the runtime receipt and SSH binding. That matches the implementation and recovery search paths, the documentation receipt passes, and I found no blocking correctness, security, compatibility, or regression defect. The current image-build failures are from repository-wide newly disclosed dependency advisories, not this documentation change.
Superseded: the host-global API-key and legacy per-port receipt distinction was corrected, independently re-reviewed, and approved on the refreshed revision.
…hind (#7901) <!-- markdownlint-disable MD041 --> ## Summary Adds an explicit whole-host uninstall mode for hosts with NemoClaw environments on more than one gateway port. Ordinary uninstall remains scoped to the selected port; the opt-in sweep processes discovered ports independently, keeps shared resources after any incomplete cleanup, and reports a nonzero outcome when work remains. ## Related Issue Related to #7791. ## Changes - Add `--all-gateway-ports` and `NEMOCLAW_UNINSTALL_ALL_GATEWAY_PORTS=1` as explicit sweep controls while preserving the existing one-port default. - Run non-selected ports in isolated child processes and the selected port last so port-scoped state, gateway names, and Docker resources resolve from the correct environment. - Reject a mismatched `--gateway` before cleanup and fail closed when gateway-process cleanup is incomplete or an unenumerated environment remains. - Preserve shared host resources after partial failure, continue independent port cleanup, exit nonzero, and document that completed cleanup is not rolled back. - Update command, uninstall, state, and troubleshooting documentation for data preservation, external supervision, recovery, and one-port versus multi-port confirmation. - Ratchet the source-architecture budget to the measured current-diff values and add focused behavior/security coverage. ## Product Scope - Status: `approved` by the current maintainer in the PR preparation task on 2026-08-03. - Approved contract: the opt-in sweep may remove all discovered gateway-port environments, while ordinary uninstall remains sticky and port-scoped; incomplete or unidentified cleanup fails closed and preserves shared resources. It rejects a mismatched gateway before cleanup, respects OpenShell/external-supervisor and user-data preservation controls, and does not roll back cleanup completed before an independent port failure. - Decision record: [issue #7791 maintainer comment](#7791 (comment)). - This approval is recorded independently of `mergeStateStatus`; it is not inferred from passing tests, review, or CI. The refresh to base `a5562015029fd8cdbebdce5664e8b8bfda9d6ba8` did not change the intended behavior; the stable patch ID and all six original PR commits remain unchanged. ## Automated Review Disposition - All seven GitHub automated-review threads are resolved; six documentation findings were corrected and the CodeQL clear-text logging report was a false positive because the environment value only selects allowlisted branding and is not logged. - CodeRabbit timeout nit: declined. There is no accepted cleanup timeout contract, and terminating a child at an arbitrary deadline could interrupt destructive cleanup mid-step; the foreground process remains operator-interruptible. - CodeRabbit direct-default test nit: nonblocking. Child arguments/environment, exit mapping, state-root enumeration, coordinator behavior, and failure paths are covered without exposing internal production defaults solely for tests. - Current-commit security review: PASS in all nine categories with no PR-diff findings. Updated uninstall, dual-Station, gateway-process, and release coverage passed 111/111, and the localhost gateway identity/release runtime case passed 1/1; the three earlier findings (incomplete process cleanup, delayed gateway-name validation, and unenumerated surviving environments) remain fixed and covered. - Documentation-review blocker after #8158: resolved. A whole-host sweep selected on a non-default port now removes the host-global dual-Station bearer key only after full cleanup is authorized and any managed-pair cleanup succeeds. Scoped cleanup, failed swept ports, and failed managed-pair cleanup preserve the key. The exact reproduction now reports `keyRemains:false` and `selectedRootRemains:false` with exit code 0. - Current-commit GPT-5.6 Terra and optional Nemotron advisor jobs succeeded. Terra recommends `merge_as_is` with medium confidence, Nemotron recommends `merge_as_is` with high confidence, and both canonical finding ledgers are empty. Their runtime-validation recommendation is covered by the required E2E gates tracked below. - Documentation-writer suggestion: no change required. The two temporal uses of “once” are unambiguous, preserve the approved behavior and recovery meaning, and are advisory under the writing policy. - Documentation-writer test-title suggestion: no change required. The plural “credentials” describes the host-global credential class even though the regression asserts the current API-key artifact; the test body and failure contract are exact. - Advisor terminology candidates: no change required. `sweep`, `scoped uninstall`, `sibling gateway-port environment`, `host-global`, and `gateway port` are used consistently with the current controlled vocabulary; neither advisor recorded a terminology finding. - Base-only corporate-CA test warning: nonblocking and unrelated to this PR. On macOS, the new base's GNU-`base64` capability probe accepts FreeBSD `base64`, so three tests reach a later `awk` rejection instead of the expected early diagnostic; every invalid payload still fails nonzero. Linux CI is authoritative, and the probe should be tightened in a separate base follow-up rather than adding unrelated work here. - Base-only HTTP-probe test warning: nonblocking and unrelated to this PR. One broader focused test times out while its test server's SIGTERM handler waits for `server.close()`; the bounded-response implementation and all #7901 interaction assertions pass. This teardown issue belongs in a separate base follow-up. - Fern warnings: nonblocking and unrelated to this patch. Redirect verification was skipped because this local run had no Fern authentication, and the existing light-mode accent color has a 2.41:1 contrast ratio. Fern reported zero errors. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Codex Desktop security specialist reviewed commit `489a368a5d0ada868cd01c0dacfc1a99e64a15f1` against base `a5562015029fd8cdbebdce5664e8b8bfda9d6ba8`; no PR-diff security finding, 112 focused tests passed, and unrelated base-test portability warnings were dispositioned above. This is an agent review, not a human sensitive-path owner review or hardware review. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: `docs/manage-sandboxes/uninstall-nemoclaw.mdx`, `docs/reference/commands.mdx`, `docs/reference/host-files-and-state.mdx`, and `docs/reference/troubleshooting.mdx`; all 15 changed files and all 12 OpenClaw, Hermes, and Deep Agents generated pages were reviewed; `npm run docs` passed with 0 errors and 2 unchanged nonblocking Fern warnings; focused documentation/uninstall validation passed 28/28; the non-default-selected sweep reproduction removed the host-global dual-Station key. - Agent: Codex Desktop <!-- docs-review-head-sha: 489a368 --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: the canonical 13-file uninstall and host-process regression passed 156/156 with a 30-second runner timeout for unrelated local-host CPU contention; uninstall integration passed 6 host-applicable tests with 4 platform-gated PTY skips; independent updated security coverage passed 112 tests; CLI build and post-build CLI typecheck passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: not applicable; the change is confined to uninstall coordination and uses focused CLI/integration suites plus repository hooks. CI is authoritative. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) ## Current Commit and Base Evidence - Commit: `489a368a5d0ada868cd01c0dacfc1a99e64a15f1` - Base: `a5562015029fd8cdbebdce5664e8b8bfda9d6ba8` - Refresh: signed, non-force merge commit `e9b83f35b395d76409a14d4bd036f841841124a6` from current `upstream/main`, followed by signed fix commit `489a368a5d0ada868cd01c0dacfc1a99e64a15f1`; both raw commit objects contain SSH signatures. - Patch identity: stable patch ID `6e980dca7f59b4af87c4e402dffc2d0b447d3884`; current binary-diff SHA-256 `f4c2ed0334f2490f5b7fa5471f88520f679544afcc32721f41913e3c5b18cf86`. The prior six original PR commits remain unchanged in range-diff; the new fix commit resolves the #8158 composition finding without rewriting existing history. - GitHub commit audit: all 24 commits displayed by GitHub are `Verified` (`verified=true`, reason `valid`) after the non-force push. - Cross-issue sweep: no medium/high-confidence adjacent fix or contradiction was found; #7791 remains the primary issue. Merged #8158 now composes safely with the whole-host sweep: scoped or failed cleanup preserves host-global dual-Station ownership and successful full cleanup removes the bearer key. Merged #8167 does not change uninstall ownership. Open PR #8129 remains a sequencing overlap only and must preserve this invariant if it lands after #7901. --- Signed-off-by: Tinson Lai <tinsonl@nvidia.com> --------- Signed-off-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Senthil Ravichandran <senthilr@nvidia.com>
Summary
Later onboarding can now recover and revalidate the owner-only cleanup state for an existing managed dual-Station vLLM endpoint without requiring
NEMOCLAW_DGX_STATION_PEERin the new process. The running pair is neither restarted nor rewritten, and unsafe, changed, or ambiguous ownership still fails closed.Related Issue
Fixes #8117
Changes
Type of Change
Quality Gates
Documentation Writer Review
docs-updateddocs/inference/set-up-vllm-on-two-dgx-stations.mdxanddocs/reference/host-files-and-state.mdx; v0.0.97 host-global API-key and legacy per-port receipt locations verified;npm run docspassed with 0 errors and 2 pre-existing warnings; focused tests passed 44/44 after the current-main refresh.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablevitestpassed 44 focused tests after the current-main refresh;npm run build:cli,npm run typecheck:cli, andnpm run checks:repositorypassed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only) — passed with 0 errors and 2 pre-existing warningsSigned-off-by: Senthil Ravichandran senthilr@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation