Skip to content

fix(inference): adopt an interrupted managed vLLM container holding the serving port - #11587

Merged
prekshivyas merged 7 commits into
mainfrom
pull-request/11530
Sep 12, 2026
Merged

fix(inference): adopt an interrupted managed vLLM container holding the serving port#11587
prekshivyas merged 7 commits into
mainfrom
pull-request/11530

Conversation

@jyaunches

@jyaunches jyaunches commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Outcome

Managed vLLM onboarding now reclaims an interrupted NemoClaw container only when the container is on the same local Docker boundary and one published binding covers the probed loopback endpoint on the requested port. This same-repository continuation adopts the exact reviewed candidate from #11530 so required SDK CI and trusted E2E can run.

Reason

The source contribution fixes the recovery gap in #11426, but fork heads cannot consume the base-controlled OpenShell SDK package and are rejected by manual PR E2E. The copy-pr-bot branch preserves the exact signed candidate while providing the repository-owned source identity those gates require.

Related issues

Fixes #11426

Adopts #11530. Do not merge both pull requests.

Changes

  • Recognize an interrupted, running NemoClaw-managed vLLM container as the owner of a failed serving port only through the sanitized local Docker environment.
  • Require a published container-port 8000 binding on 127.0.0.1 or 0.0.0.0 at the failed serving port before replacement.
  • Reuse that same Docker environment for preflight inspection, removal, and relaunch, while failing closed for foreign ownership, mismatched ports or addresses, unreadable inspection, distributed profiles, stopped containers, and container-ID drift.
  • Add regressions for ambient remote Docker routing, another host port, and bindings away from the probed loopback address.

Verification

  • Focused vLLM serving-port suite — 18 tests passed.
  • npm run test:changed — 17 files and 391 tests passed.
  • NODE_OPTIONS=--max-old-space-size=8192 npm run typecheck:cli — passed.
  • NODE_OPTIONS=--max-old-space-size=8192 npm run validate:pr — passed against canonical main validation surfaces.
  • Guarded pre-push validation on the source contribution — passed.
  • GitHub commit verification on fix(inference): adopt an interrupted managed vLLM container holding the serving port #11530 — all six commits Verified.
  • The diff contains no secrets, API keys, or credentials.

Review notes

Sensitive paths: src/lib/inference/vllm.ts and src/lib/inference/vllm-serving-port.test.ts. The complete two-file candidate at bf1b1d8 was reviewed through #11530, including correctness, Docker locality, destructive-removal guards, binding parsing, sibling recovery paths, regression coverage, and credential boundaries. CodeRabbit findings on remote Docker routing and binding addresses were reproduced and repaired. The local PR Advisor was attempted on each repaired candidate but could not start its first specialist sandbox because the local OpenShell relay refused the connection; cleanup succeeded and no artifacts were produced. Alternative review completed; this is not an Advisor clearance.


Signed-off-by: Julie Yaunches jyaunches@nvidia.com

Summary by CodeRabbit

  • Bug Fixes
    • Improved vLLM serving recovery after interrupted installations or restarts.
    • Existing managed containers can now be safely reused when they are running locally and serving the expected port.
    • Added safeguards to reject containers with incorrect ownership, port mappings, or network addresses.
    • Container replacement now preserves the required runtime configuration for a reliable relaunch.

HOYALIM and others added 6 commits September 11, 2026 00:49
…he serving port

Host-local lifecycle recovery admits only a completed authenticated install:
it requires the runtime receipt written after startup and the auth label a
profile adds only for managed bearer auth. An install interrupted before
either exists leaves its own managed container running on the serving port,
so the guard reported that port as held by another process and neither
onboard --resume nor a fresh install could make progress.

Classify the port holder by its ownership labels when recovery cannot claim
it, and reuse the existing replacement path for a running managed container.
A foreign or unlabeled holder, an ambiguous inspection, a distributed head or
worker, and a container that is not running all remain conflicts.

Fixes #11426

Signed-off-by: Ho Lim <subhoya@gmail.com>
Review found that ownership alone did not prove the managed container was the
process holding the serving port: a managed container published on another
host port would have been adopted and removed while an unrelated process kept
the port, freeing nothing and destroying an unrelated runtime.

Compare the container's published binding for the fixed container port with
the port that failed, and keep the conflict outcome when they differ or the
binding cannot be read.

Signed-off-by: Ho Lim <subhoya@gmail.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
@jyaunches jyaunches self-assigned this Sep 11, 2026
@copy-pr-bot

copy-pr-bot Bot commented Sep 11, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: a8124cd4-b1ab-4c42-bf87-fc376eb3eb25

📥 Commits

Reviewing files that changed from the base of the PR and between e8071e8 and bf1b1d8.

📒 Files selected for processing (2)
  • src/lib/inference/vllm-serving-port.test.ts
  • src/lib/inference/vllm.ts

Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review.


📝 Walkthrough

Walkthrough

The installer now adopts eligible managed vLLM containers that hold the configured loopback serving port. It preserves the recovered Docker context and validates ownership during replacement. Tests cover recovery, foreign containers, incorrect ports, non-loopback bindings, and replacement races.

Changes

Managed vLLM container recovery

Layer / File(s) Summary
Serving-port adoption and replacement
src/lib/inference/vllm.ts
The installer adopts running, managed containers with matching loopback bindings. It reuses the recovered Docker environment and validates ownership before replacement.
Recovery and ownership test coverage
src/lib/inference/vllm-serving-port.test.ts
Tests cover default-context recovery, foreign containers, incorrect published ports, non-loopback bindings, and replacement races.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant vLLMInstaller
  participant PortProbe
  participant DockerDefaultContext
  vLLMInstaller->>PortProbe: detect configured serving port
  vLLMInstaller->>DockerDefaultContext: inspect port holder
  DockerDefaultContext-->>vLLMInstaller: return ownership and binding data
  vLLMInstaller->>DockerDefaultContext: adopt or replace eligible container
Loading

Suggested reviewers: ericksoa, prekshivyas

Merge Risk: ⚪ Minimal · up to bf1b1

The recovery flow validates managed ownership, exact loopback port binding, and Docker context before replacement, with focused coverage for foreign, mismatched, and remote-context cases.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adopting an interrupted managed vLLM container that holds the serving port.
Linked Issues check ✅ Passed The changes implement #11426. They inspect the default local Docker environment, require managed ownership, a running container, and a loopback binding for host port 8000. They preserve that Docker en…
Out of Scope Changes check ✅ Passed The diff is limited to managed vLLM recovery logic and regression tests for #11426. The Docker ownership and binding checks directly support safe recovery and fail-closed behavior.
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch pull-request/11530

Comment @coderabbitai help to get the list of available commands.

@github-code-quality

github-code-quality Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall line coverage in commit 55b330e in the pull-request/11530 branch remains at 96%, unchanged from commit 10b283b in the main branch.

TypeScript / code-coverage/cli

The overall line coverage in commit 55b330e in the pull-request/11530 branch remains at 83%, unchanged from commit 10b283b in the main branch.

Show a line coverage summary of the most impacted files.
File main 10b283b pull-request/11530 55b330e +/-
src/lib/inference/vllm.ts 90% 90% 0%
src/lib/sandbox...rce-identity.ts 82% 82% 0%
src/lib/onboard...uild-context.ts 74% 75% +1%
src/lib/onboard...outer-python.ts 84% 96% +12%
src/lib/onboard...uter-command.ts 52% 65% +13%

Updated September 11, 2026 21:03 UTC

@jyaunches
jyaunches marked this pull request as ready for review September 11, 2026 20:24
@jyaunches jyaunches added the v0.0.124 Release target label Sep 11, 2026
@jyaunches
jyaunches marked this pull request as draft September 11, 2026 20:27
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
@jyaunches
jyaunches marked this pull request as ready for review September 11, 2026 20:50

@rsliter rsliter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact commit 55b330e against issue #11426. The implementation keeps adoption on the physical host Docker boundary, requires managed running ownership plus a loopback-covering binding on the failed port, and revalidates the exact container ID before removal. Negative coverage includes foreign ownership, remote Docker selectors, mismatched ports and addresses, stopped state, inspection ambiguity, and ID drift. The focused suite passes 18/18 locally. The repository-owned CI and self-hosted qualification runs are green. Approval applies to the implementation; merge remains separately gated.

prekshivyas pushed a commit that referenced this pull request Sep 12, 2026
## Outcome

Configuration export can read a NemoClaw-managed gateway from the
canonical owner-private default state directory even when that directory
predates explicit ownership markers. The selected inference-routing E2E
also accepts OpenShell's current scope-bound credential placeholder and
continues to verify delegated bearer resolution.

## Reason

Exact-candidate run 34649953836 and exact-base replay 34651044220
reproduced the same two failures: default-gateway SDK reads stopped
before the sandbox inventory RPC, and TC-INF-12 rejected the valid
OpenShell 0.0.116 scope-bound placeholder. These inherited failures
block deterministic qualification of #11587.

### Related issues

Refs #11421

## Changes

- Preserve strict ownership-marker requirements for explicit gateway
state overrides while recognizing the canonical owner-private default
state used by installations created before markers.
- Cover both the legacy-default connection path and rejection of an
unmarked explicit override.
- Share the bounded OpenShell placeholder grammar for unscoped,
generation-scoped, and scope-bound credentials between TC-INF-12 and its
mapped fast test.
- Integrate current main so trusted E2E compares the candidate against
the exact PR base instead of treating intervening main changes as
candidate-managed-image inputs.

## Verification

- `npx vitest run --project cli
src/lib/adapters/openshell/sandbox-command-sdk.test.ts
src/lib/adapters/config/live-export-source.test.ts` — 93 tests passed.
- `npx vitest run --project e2e-support
test/e2e/support/hosted-inference.test.ts` — 19 tests passed.
- `npx tsx scripts/checks/e2e-mock-parity.mts --base origin/main --head
HEAD` — passed.
- `NODE_OPTIONS=--max-old-space-size=8192 npm run typecheck:cli` —
passed.
- `npm run test:changed` — the growth-guardrail preflight passed; no
additional source tests were selected after base integration.
- `NODE_OPTIONS=--max-old-space-size=8192 npm run validate:pr` — passed
at `1f5269c49138fea461d3351f5d5c39c73f5d1ba1` against canonical main
`37261efb99c0dcbfcef6551efa0c59357e96fcbe`.
- The pre-repair exact-head hosted CI run
[34666334600](https://github.com/NVIDIA/NemoClaw/actions/runs/34666334600)
passed every selected job, including all 12 CLI shards. Current-head run
[34668234877](https://github.com/NVIDIA/NemoClaw/actions/runs/34668234877)
is qualifying the one-line E2E repair.
- Trusted manual PR E2E run
[34667270786](https://github.com/NVIDIA/NemoClaw/actions/runs/34667270786)
ran `inference-routing,network-policy` in mock mode against head
`092fdefc472882cd135a4f839d2285da4969b467`, with correlation
`57e03ed4-c1ad-4a14-9f45-112ad6c8d10a`. Network policy passed. TC-INF-12
failed after credential rotation because its second assertion still
required only the generation-scoped placeholder. Cleanup and artifact
upload passed. Commit `1f5269c49138fea461d3351f5d5c39c73f5d1ba1` reuses
the bounded credential-reference matcher for the rotated placeholder.
- The diff contains no secrets, API keys, or credentials.

## Review notes

Alternative review completed for
`1f5269c49138fea461d3351f5d5c39c73f5d1ba1`. Local Advisor failed before
specialist analysis because its OpenShell sandbox never passed
supervisor-relay readiness (`sandbox is not ready`, SSH status 255);
cleanup succeeded, but no specialist artifacts were produced. Manual
review of the full diff and the one-line E2E repair found no remaining
actionable issue. The repair reuses the bounded matcher already
exercised by the fast support test. Focused tests and canonical
validation passed. The residual limitation is the absence of Advisor
specialist artifacts; current-head CodeRabbit and trusted E2E evidence
remain pending.

## Dependency

- [#11598](#11598) merged as
`37261efb99c0dcbfcef6551efa0c59357e96fcbe`; that fix is integrated into
the current head, so qualification has resumed.

---
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>

---------

Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
@prekshivyas
prekshivyas merged commit 75c437f into main Sep 12, 2026
71 of 74 checks passed
@prekshivyas
prekshivyas deleted the pull-request/11530 branch September 12, 2026 05:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v0.0.124 Release target

Projects

None yet

4 participants