fix(inference): honor declared llama.cpp host port - #8564
Merged
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
ericksoa
added
area: inference
Contributor
📝 WalkthroughWalkthroughThe managed llama.cpp installer now derives its host port from the selected recipe. Docker lifecycle inspection validates configured and published bindings against that port during installation, recovery, rollback, authorization, destruction, and startup. Tests cover custom ports, receipt round trips, and port drift. ChangesLlama.cpp port validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ManagedInstaller
participant DockerLlamaCppManagedLifecycle
participant Docker
ManagedInstaller->>DockerLlamaCppManagedLifecycle: provide recipe serving port
DockerLlamaCppManagedLifecycle->>Docker: inspect container bindings
Docker-->>DockerLlamaCppManagedLifecycle: return configured and published ports
DockerLlamaCppManagedLifecycle-->>ManagedInstaller: return validated lifecycle state
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit d07506f in the TypeScript / code-coverage/cliThe overall coverage in commit d07506f in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
2 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.ts (1)
312-327: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake port-drift rejection recoverable.
rollbackExactcalls strictinspectContainerbefore ownership validation. Configured or published port drift therefore prevents container removal, network removal, and journal retirement. Add a cleanup inspection path that skips only port-binding validation, while retaining ownership and security checks. Use it in rollback and add tests that assert both drift cases remove resources and retire the journal.🤖 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/runtime-provider/docker-llama-cpp-managed-lifecycle.ts` around lines 312 - 327, Make the strict port checks in inspectContainer optionally skippable for cleanup while preserving ownership and security validation; use this cleanup inspection mode in rollbackExact so configured or published port drift still removes the container/network and retires the journal. Update the lifecycle tests covering both drift cases to assert resource removal and journal retirement. Apply the implementation in src/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.ts at lines 312-327 and 935-966, and the corresponding assertions in src/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.test.ts at lines 814-820.Source: Path instructions
🤖 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.
Outside diff comments:
In `@src/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.ts`:
- Around line 312-327: Make the strict port checks in inspectContainer
optionally skippable for cleanup while preserving ownership and security
validation; use this cleanup inspection mode in rollbackExact so configured or
published port drift still removes the container/network and retires the
journal. Update the lifecycle tests covering both drift cases to assert resource
removal and journal retirement. Apply the implementation in
src/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.ts at lines
312-327 and 935-966, and the corresponding assertions in
src/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.test.ts at
lines 814-820.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4b0b15ee-09c0-4575-83f4-35daf61ffaf0
📒 Files selected for processing (4)
src/lib/inference/llama-cpp/managed-installer.test.tssrc/lib/inference/llama-cpp/managed-installer.tssrc/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.test.tssrc/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
23 tasks
ericksoa
added a commit
that referenced
this pull request
Aug 7, 2026
<!-- markdownlint-disable MD041 --> ## Summary <!-- 1-3 plain sentences: what changes and why. Describe before-and-after behavior when it applies. Follow the NemoClaw Writing Guide: https://github.com/NVIDIA/NemoClaw/blob/main/WRITING.md. Do not add unrelated prose cleanup. --> The managed llama.cpp installer publishes the fixed loopback host port that every downstream endpoint assumes, while the lifecycle validator required a dynamic host-port assignment, so the inspection that follows container creation always rejected the container the installer had just created and managed onboarding could never finish. The validator now compares the configured host port against the port the bindings pin, and the lifecycle input requires that port rather than accepting a dynamic binding. Since #8564 landed the primary fixed-port behavior, this PR now carries the distinct rollback protection: malformed, non-loopback, or drifting endpoint metadata can be rejected while strongly authenticated managed containers, networks, and journals are still cleaned up for a safe retry. ## Related Issue <!-- Fixes #NNN or Closes #NNN. Remove this section if none. --> Fixes #8544 ## Changes <!-- List concrete changes. If this adds an abstraction, configuration, fallback, migration, or compatibility path, name its current requirement and consumer, explain why a direct change is insufficient, and identify the test that protects it. --> - `parseInspection` in `src/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.ts` compared the configured `HostPort` against `""`, which requires Docker to choose the port. The only caller, `lifecycleFor` in `src/lib/inference/llama-cpp/managed-installer.ts`, always pins `LLAMA_CPP_PORT`, so `buildLlamaCppHostLocalDockerArgv` emits `--publish 127.0.0.1:8081:8081` and Docker reports `HostPort: "8081"`. The two requirements could not both hold. - The loopback check and the port check are now separate. A `HostIp` that is not `127.0.0.1` still reports `Docker llama.cpp configured host port is not loopback-only.`; a port that disagrees with the bindings reports that instead. The previous single condition raised the loopback message for a correct `127.0.0.1` binding, which is what the issue reported. - `parseInspection` and `inspectContainer` take the runtime bindings rather than a bare network name, so the expected host port travels with the network identity it is checked alongside. The ten call sites already passed `options.bindings.network.name`. - Rollback now uses a cleanup inspection mode after an endpoint mismatch. It skips only endpoint validation, still requires the recorded container ownership and hardening to match before deletion, verifies container and network absence, and retires the journal so a retry can proceed. - `HostLocalLlamaCppLifecycleInput.bindings` in `src/lib/onboard/runtime-provider/host-local-inference.ts` now requires `hostPort`. The shared bindings type leaves it optional because `buildLlamaCppHostLocalDockerArgv` is also called directly by `scripts/checks/run-llama-cpp-dgx-spark-qualification.mts`, which legitimately omits it and never enters this lifecycle. Requiring it at the lifecycle boundary states that the managed path publishes a fixed port and makes a dynamic binding a compile error rather than a runtime check. `npm run typecheck:cli` passes unchanged, which confirms no current caller omits it. - The fix is on the lifecycle, not the installer. Publishing a dynamic port would satisfy the old condition and then break onboarding silently: `providers.ts` and `setup-nim-flow.ts` set `endpointUrl` from the hardcoded `LLAMA_CPP_HOST_OPENAI_BASE_URL`, `llama-cpp/index.ts` refuses attachment on any other port, and `core/ports.ts` reserves 8081 against env-var conflicts. The configured endpoint would point at 8081 while the container listened elsewhere. ### Tests - `src/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.test.ts` ran a configuration the product never produces: its bindings omitted `hostPort` and its Docker inspection fixture hard-coded `HostPort: ""` with an ephemeral published port. Pinning `LLAMA_CPP_PORT` in the bindings and moving the fixture to the shape Docker returns for that binding makes the existing suite cover the shipped path. Reverting only the source change and rerunning the suite fails 22 of 34 tests with the exact error from the issue. - The host-port mismatch regression now proves rejection, journal retirement, an idempotent recovery pass, and a successful fresh start. The retry would fail at the existing container-name or network-name checks if either resource remained. It distinguishes configured-port and published-port diagnostics and includes an isolated non-loopback case with no simultaneous port drift. - `src/lib/inference/llama-cpp/host-local-runtime.test.ts` asserted only the dynamic `127.0.0.1::8081` publish argument. A case now covers the pinned `127.0.0.1:8081:8081` form the product uses. - `src/lib/inference/llama-cpp/managed-installer.test.ts` now asserts that the bindings handed to the lifecycle pin `LLAMA_CPP_PORT`, so the installer and the validator cannot drift apart again without a test failing. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: no flag, prompt, configuration key, default, or success-path output changes. The published documentation already describes the pinned behavior this change restores: `docs/inference/choose-local-inference-server.mdx` states that onboarding "Publishes port `8081` on `127.0.0.1`" and that "The Docker port output must contain `127.0.0.1:8081`", and `docs/reference/commands.mdx` records that port `8081` is reserved for the managed llama.cpp runtime. Those statements were correct and unreachable; they are now reachable. No documented error string changes, and `docs/reference/troubleshooting.mdx` has no llama.cpp entry to keep consistent. - [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 independently completed all nine security categories on the current branch revision; no findings. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review <!-- Required for code and documentation changes after the changes and applicable validation are complete. Keep one review checkbox and one instance of each visible or hidden field. For Evidence, list changed documentation paths. For documentation-only changes, also state that the writing rules and documentation style were reviewed. For other results, explain why no documentation change is needed or why the review is blocked. For Agent, use a consistent product and surface name, such as Codex Desktop, Codex CLI, Claude Code, or Cursor. After committing all review changes, put `git rev-parse --short HEAD` and `git rev-parse --short HEAD:AGENTS.md` in the hidden metadata below. Rerun the review and refresh that metadata after any new commit. This receipt is advisory during the data-collection pilot. --> - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: No documentation paths changed. The independent review confirmed that existing managed llama.cpp documentation already specifies the fixed `127.0.0.1:8081` endpoint, authenticated unfinished-journal rollback, and retry behavior; the completed change restores those documented contracts without adding a user-facing surface. - Agent: Codex Desktop <!-- docs-review-head-sha: 4428002 --> <!-- docs-review-agents-blob-sha: 12ad395 --> ## DGX Station Hardware Evidence <!-- Required only when scripts/prepare-dgx-station-host.sh changes. Maintainers must review the linked evidence before approving or merging. This is human-reviewed evidence, not authenticated hardware provenance. Exceptional bypasses use existing repository governance and must be documented on the PR. --> - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [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: `npx vitest run --project cli src/lib/onboard/runtime-provider src/lib/inference/llama-cpp` — 295 passed across 18 files. `npx vitest run --project integration test/cli/list-share-live-inference.test.ts test/sandbox-connect-inference` — 23 passed. `npx vitest run --project e2e-support` — 2142 passed, 10 skipped. `npm run typecheck:cli` — clean. `test/e2e/live/llama-cpp-dgx-spark-qualification.test.ts` needs DGX Spark hardware and was not run. Maintainer follow-up on the current branch revision: the three directly affected files pass 76/76 tests, CLI type-checking passes, and `npm run validate:pr` passes. - [ ] 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. This changes one validator and one lifecycle input type; it adds no harness mechanics, Vitest config, project glob, or registration topology. `npm run checks:repository` passed, including the layer import boundary and the source architecture budget with zero cycles. - [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) - [ ] 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) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Tinson Lai <tinsonl@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved llama.cpp container lifecycle handling for malformed, non-loopback, duplicate, or unexpectedly changed published ports. * Ensured cleanup and recovery flows reliably remove affected containers and networks. * Enforced consistent use of the configured local host port during runtime validation and Docker publishing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Co-authored-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Aaron Erickson <aerickson@nvidia.com>
22 tasks
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
1 participant
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
Managed llama.cpp onboarding now accepts the exact loopback host port declared by the selected YAML recipe. Docker lifecycle inspection still rejects non-loopback or mismatched bindings, and isolated qualification retains dynamic loopback allocation.
Related Issue
Fixes #8544.
Advances #8144.
Changes
recipe.spec.serve.port.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededserve.port, validates Docker's published port against that declared binding, and preserves exact-owned rollback for valid loopback port drift. The shipped recipe remains on loopback port8081, which current documentation already describes. No command, public configuration, default, support status, or user workflow changes.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 unavailablenpx vitest run --project cli src/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.test.ts src/lib/inference/llama-cpp/managed-installer.test.ts(2 files, 60 tests);npm run typecheck:cli;npm run test-size:check;npm run checks:repository; andnpm run lintpassed.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)Signed-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit