fix(onboard): clean up invalid llama.cpp port state - #8565
Conversation
The managed installer pins the fixed loopback host port that every downstream llama.cpp endpoint assumes, while the lifecycle validator required a dynamic assignment, so managed onboarding always failed on the inspection that follows container creation. The validator now compares the configured host port against the port the bindings pin, and the lifecycle input requires that port instead of accepting a dynamic binding. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe lifecycle validates Docker’s loopback host port against ChangesManaged llama.cpp port handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Startup
participant inspectContainer
participant Docker
Startup->>inspectContainer: inspect with lifecycle bindings
inspectContainer->>Docker: read container network and port bindings
Docker-->>inspectContainer: return published host port
inspectContainer-->>Startup: accept matching loopback port or report mismatch
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
laitingsheng
added
area: integrations
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 4428002 in the TypeScript / code-coverage/cliThe overall coverage in commit 4428002 in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.ts`:
- Around line 312-317: Add a cleanup inspection mode to parseInspection that
skips only configured host/port endpoint checks while preserving ownership and
hardening validation. Update rollbackExact and its post-create mismatch path to
use this mode so managed containers and networks can always be removed and the
creating journal cleared. Extend the mismatch test to verify container, network,
and journal cleanup, and ensure resume/repair bridge handling remains idempotent
and converges through the same authoritative path.
🪄 Autofix
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: a6e1ddda-4a09-4848-aab3-848733a818aa
📒 Files selected for processing (5)
src/lib/inference/llama-cpp/host-local-runtime.test.tssrc/lib/inference/llama-cpp/managed-installer.test.tssrc/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.test.tssrc/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.tssrc/lib/onboard/runtime-provider/host-local-inference.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
5 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. Live E2E does not run automatically for pull requests. 1 semantic terminology decisionTerminology 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: 1 optional E2E recommendation
1 warning · 0 suggestionsWarningsWarnings do not block.
|
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
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.test.ts (1)
842-856: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd an isolated non-loopback binding case.
The
0.0.0.0case also sets the published port to"8082". The lifecycle can reject that fixture only for port drift. This test does not prove that a non-loopback published binding is rejected.Add a case where both configured and published ports equal
HOST_PORT, butpublishedHostIpis"0.0.0.0".Proposed test input
for (const args of [ + [HOST_PORT, HOST_PORT, "0.0.0.0", 1], ["8081", "8082", "0.0.0.0", 1], ["8081", "invalid", "127.0.0.1", 1],As per path instructions, tests must prove the claimed behavior through observable outcomes.
🤖 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.test.ts` around lines 842 - 856, Add an isolated fixture case in the parameterized inputs for the lifecycle test where the configured and published ports both equal HOST_PORT (8081) while publishedHostIp is "0.0.0.0". Keep the existing rejection and cleanup assertions unchanged so the test specifically verifies rejection of the non-loopback binding without port drift.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.test.ts`:
- Around line 842-856: Add an isolated fixture case in the parameterized inputs
for the lifecycle test where the configured and published ports both equal
HOST_PORT (8081) while publishedHostIp is "0.0.0.0". Keep the existing rejection
and cleanup assertions unchanged so the test specifically verifies rejection of
the non-loopback binding without port drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7f8dbbc2-66e0-4755-94da-3ed79810175b
📒 Files selected for processing (2)
src/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.test.tssrc/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.ts
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
changed the title
|
Addressed the current outside-diff review finding. The cleanup regression now includes an isolated |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
3 participants
Summary
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 #8544
Changes
parseInspectioninsrc/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.tscompared the configuredHostPortagainst"", which requires Docker to choose the port. The only caller,lifecycleForinsrc/lib/inference/llama-cpp/managed-installer.ts, always pinsLLAMA_CPP_PORT, sobuildLlamaCppHostLocalDockerArgvemits--publish 127.0.0.1:8081:8081and Docker reportsHostPort: "8081". The two requirements could not both hold.HostIpthat is not127.0.0.1still reportsDocker 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 correct127.0.0.1binding, which is what the issue reported.parseInspectionandinspectContainertake 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 passedoptions.bindings.network.name.HostLocalLlamaCppLifecycleInput.bindingsinsrc/lib/onboard/runtime-provider/host-local-inference.tsnow requireshostPort. The shared bindings type leaves it optional becausebuildLlamaCppHostLocalDockerArgvis also called directly byscripts/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:clipasses unchanged, which confirms no current caller omits it.providers.tsandsetup-nim-flow.tssetendpointUrlfrom the hardcodedLLAMA_CPP_HOST_OPENAI_BASE_URL,llama-cpp/index.tsrefuses attachment on any other port, andcore/ports.tsreserves 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.tsran a configuration the product never produces: its bindings omittedhostPortand its Docker inspection fixture hard-codedHostPort: ""with an ephemeral published port. PinningLLAMA_CPP_PORTin 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.src/lib/inference/llama-cpp/host-local-runtime.test.tsasserted only the dynamic127.0.0.1::8081publish argument. A case now covers the pinned127.0.0.1:8081:8081form the product uses.src/lib/inference/llama-cpp/managed-installer.test.tsnow asserts that the bindings handed to the lifecycle pinLLAMA_CPP_PORT, so the installer and the validator cannot drift apart again without a test failing.Type of Change
Quality Gates
docs/inference/choose-local-inference-server.mdxstates that onboarding "Publishes port8081on127.0.0.1" and that "The Docker port output must contain127.0.0.1:8081", anddocs/reference/commands.mdxrecords that port8081is reserved for the managed llama.cpp runtime. Those statements were correct and unreachable; they are now reachable. No documented error string changes, anddocs/reference/troubleshooting.mdxhas no llama.cpp entry to keep consistent.Documentation Writer Review
no-docs-needed127.0.0.1:8081endpoint, authenticated unfinished-journal rollback, and retry behavior; the completed change restores those documented contracts without adding a user-facing surface.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 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.tsneeds 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, andnpm run validate:prpasses.npm testfor broad runtime/test-harness changes;npm run checkfor 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:repositorypassed, including the layer import boundary and the source architecture budget with zero cycles.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit