fix(uninstall): remove orphaned managed vllm

[prekshivyas]

Summary

• remove an orphaned nemoclaw-vllm container during full uninstall when Docker verifies its NemoClaw managed label
• retain fail-closed behavior for a missing or mismatched label, malformed inspection, or failed removal
• add focused uninstall regression coverage for the labeled and unlabeled cases

Root cause

The host-local vLLM cleanup only ran when its local key state existed. A legacy/orphaned managed container with the reserved name and managed label but no local state was skipped, then caused the residual-container guard to abort the rest of uninstall.

Validation

• npx vitest run src/lib/actions/uninstall/run-plan-local-model-profile.test.ts
• npm run typecheck:cli
• npx @biomejs/biome check src/lib/actions/uninstall/run-plan.ts src/lib/actions/uninstall/run-plan-local-model-profile.test.ts
• live DGX Spark full uninstall: completed successfully after removing the reproduced nemoclaw-vllm orphan and all NemoClaw resources

Closes #8981

Summary by CodeRabbit

• Bug Fixes
  • Uninstall now cleans up orphaned host-local vLLM containers when they are verified as managed by the application.
  • Unmanaged or unlabeled containers are preserved to prevent accidental removal.
  • If container verification or removal fails, the uninstall process reports the error and stops subsequent cleanup steps.

[copy-pr-bot]

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

[coderabbitai]

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: 2c656340-a0a5-4408-8489-94c7cd272cb0

📥 Commits

Reviewing files that changed from the base of the PR and between 9f9f483 and 6362136.

📒 Files selected for processing (2)

• src/lib/actions/uninstall/run-plan-local-model-profile.test.ts
• src/lib/actions/uninstall/run-plan.ts

---

📝 Walkthrough

Walkthrough

The uninstall plan now detects orphaned host-local vLLM containers, removes only verified managed containers, and stops on removal failure. Tests cover labeled and unlabeled containers plus Docker inspection of reserved inference names.

Changes

Host-local vLLM cleanup

Layer / File(s)	Summary
Managed container cleanup src/lib/actions/uninstall/run-plan.ts	The no-runtime-state path inspects nemoclaw-vllm, verifies its managed label and output shape, and force-removes verified containers. Removal failures stop the uninstall.
Cleanup behavior validation src/lib/actions/uninstall/run-plan-local-model-profile.test.ts	Tests cover removal of labeled containers, preservation and error reporting for unlabeled containers, and Docker inspection during reserved-name cleanup.

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

Mergeability Score: ⚪ Minimal · up to 63621

The PR narrowly updates full uninstall cleanup for verified managed orphan containers and adds regression coverage; no actionable merge-blocking risk remains beyond normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant UninstallPlan
  participant Docker
  participant HostLocalVLLM
  UninstallPlan->>Docker: inspect nemoclaw-vllm
  Docker-->>UninstallPlan: return container metadata
  UninstallPlan->>Docker: force-remove verified container
  Docker->>HostLocalVLLM: remove managed container

Loading

Possibly related PRs

• NVIDIA/NemoClaw#8919: Extends the same uninstall logic and test coverage with related model-store cleanup.
• NVIDIA/NemoClaw#8976: Modifies Docker cleanup matching in the same uninstall plan.

Suggested labels: area: sandbox, bug-fix

Suggested reviewers: ericksoa, cv, senthilr-nv

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 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 primary change: removing an orphaned managed vLLM container during uninstall.
Linked Issues check	✅ Passed	The changes address issue #8981 by verifying ownership and removing the managed vLLM container while failing closed for unsafe cleanup conditions.
Out of Scope Changes check	✅ Passed	The implementation and regression tests remain focused on orphaned managed vLLM container cleanup during uninstall.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch agent/fix-orphaned-vllm-uninstall

---

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

[rsliter]

The pull request cannot enter the merge train yet. Commit 6362136a91ab8f4bd9feb13ca4520327ab94d1fd and the PR description do not include the contributor’s Signed-off-by: declaration. Please add your DCO sign-off to the commit and PR description, then push the verified commit. I will recheck the exact revision and repository gates after that update.

[prekshivyas]

Superseded by the signed-off replacement PR opened after this branch could not be force-pushed under repository rules.

[github-code-quality]

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit 6362136 in the agent/fix-orphaned-v... branch remains at 96%, unchanged from commit 0b03d49 in the main branch.

[github-actions]

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: Review the warnings below.
Findings: 0 blockers · 1 warning · 0 suggestions

Model lanes

• GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 1 warning · 0 suggestions
• Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
• Model comparison: normalized findings differ; normalized terminology decisions differ; normalized E2E selections differ; Nemotron reported the same number of blockers, 1 fewer warning, the same number of suggestions.

1 additional E2E selection from the second opinion

Advisory only. The primary lane did not select these E2E jobs or targets.

• spark-install: The completed second-opinion lane identified E2E coverage that the primary lane omitted.

Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests.

1 semantic terminology decision

Terminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.

• established — orphaned at src/lib/actions/uninstall/run-plan.ts:1561: Keep “orphaned” for the container that remains without local ownership state.

E2E guidance

Advisory only. A maintainer can dispatch the default E2E suite for the commit under review.

Recommended E2E: None

1 optional E2E recommendation

• vllm-docker-storage

1 warning · 0 suggestions

Warnings

Warnings do not block.

PRA-1 Warning — Validate the inspected Docker container ID before removal

• Location: src/lib/actions/uninstall/run-plan.ts:1553
• Category: security
• Problem: The orphaned vLLM cleanup accepts any nonempty inspection field as a container ID and passes it to `docker rm -f`.
• Impact: Malformed inspection output can reach the Docker argument boundary, unlike the existing state-backed cleanup path.
• Recommendation: Require `containerId` to match `/^[a-f0-9]{12,64}$/u` before calling `docker rm -f`; otherwise preserve the container for the residual cleanup guard.
• Verification: Read `removeOrphanedManagedHostLocalVllm` and run the focused unit test with a malformed first inspection field.
• Test coverage: Add a test where inspection returns a non-hex or option-like first field with managed label `true`, then assert that no Docker removal occurs and uninstall stops at the residual guard.
• Evidence: src/lib/actions/uninstall/run-plan.ts:1552-1555 splits Docker inspection output, checks only nonemptiness, and invokes `docker rm -f` with the result. src/lib/inference/local-model-profile/cleanup.ts:113-126 validates state-backed container IDs with `/^[a-f0-9]{12,64}$/u` before removal.

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.