Skip to content

fix(nim): use published Docker port in status checks#1022

Open
cr7258 wants to merge 2 commits intoNVIDIA:mainfrom
cr7258:fix-nim-port
Open

fix(nim): use published Docker port in status checks#1022
cr7258 wants to merge 2 commits intoNVIDIA:mainfrom
cr7258:fix-nim-port

Conversation

@cr7258
Copy link
Copy Markdown
Contributor

@cr7258 cr7258 commented Mar 27, 2026

Summary

nimStatusByName() previously hardcoded its health check to localhost:8000, which caused false unhealthy reports when a NIM container was published on a non-default host port. This change uses the explicit port when provided, otherwise resolves the published port via docker port command.

Related Issue

Fixes #1016

Changes

  • fix nimStatusByName() so it no longer hardcodes localhost:8000 for health checks
  • use the explicit port when provided, otherwise resolve the published Docker port with docker port <container> 8000
  • fall back to port 8000 if Docker port lookup fails and skip health checks when the container is not running
  • rename the resolved port variable for clarity and add unit coverage for explicit port, published port detection, fallback behavior, and non-running containers

Type of Change

  • Code change for a new feature, bug fix, or refactor.
  • Code change with doc updates.
  • Doc only. Prose changes without code sample modifications.
  • Doc only. Includes code sample changes.

Testing

  • npx prek run --all-files passes (or equivalently make check).
  • npm test passes.
  • make docs builds without warnings. (for doc-only changes)
npx vitest run test/nim.test.js

 RUN  v4.1.0 /Users/sevenc/code/ai/agent/NemoClaw

 ✓  cli  test/nim.test.js (13 tests) 3751ms
   ✓ nim (13)
     ✓ listModels (2)
       ✓ returns 5 models 1ms
       ✓ each model has name, image, and minGpuMemoryMB 0ms
     ✓ getImageForModel (2)
       ✓ returns correct image for known model 0ms
       ✓ returns null for unknown model 0ms
     ✓ containerName (1)
       ✓ prefixes with nemoclaw-nim- 0ms
     ✓ detectGpu (3)
       ✓ returns object or null  1149ms
       ✓ nvidia type is nimCapable  1150ms
       ✓ apple type is not nimCapable  1180ms
     ✓ nimStatus (1)
       ✓ returns not running for nonexistent container 265ms
     ✓ nimStatusByName (4)
       ✓ uses provided port directly 1ms
       ✓ uses published docker port when no port is provided 1ms
       ✓ falls back to 8000 when docker port lookup fails 0ms
       ✓ does not run health check when container is not running 1ms

 Test Files  1 passed (1)
      Tests  13 passed (13)
   Start at  12:34:32
   Duration  3.91s (transform 19ms, setup 0ms, import 75ms, tests 3.75s, environment 0ms)

Checklist

General

Code Changes

  • Formatters applied — npx prek run --all-files auto-fixes formatting (or make format for targeted runs).
  • Tests added or updated for new or changed behavior.
  • No secrets, API keys, or credentials committed.
  • Doc pages updated for any user-facing behavior changes (new commands, changed defaults, new features, bug fixes that contradict existing docs).

Doc Changes

  • Follows the style guide. Try running the update-docs agent skill to draft changes while complying with the style guide. For example, prompt your agent with "/update-docs catch up the docs for the new changes I made in this PR."
  • New pages include SPDX license header and frontmatter, if creating a new page.
  • Cross-references and links verified.

Summary by CodeRabbit

Release Notes

  • New Features

    • Added support for configurable ports in NIM health checks and status monitoring operations.
  • Tests

    • Expanded test coverage for port resolution and health check scenarios, including Docker port mapping resolution and fallback behavior.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 27, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d0874b23-d819-4ce5-a059-5577801a5393

📥 Commits

Reviewing files that changed from the base of the PR and between 5c269c1 and b266924.

📒 Files selected for processing (2)
  • bin/lib/nim.js
  • test/nim.test.js

📝 Walkthrough

Walkthrough

Added port parameter support to nimStatus and nimStatusByName functions. nimStatusByName now resolves the correct host port either from the provided parameter or by querying Docker port mappings, enabling health checks on custom ports instead of hardcoded port 8000.

Changes

Cohort / File(s) Summary
Port Resolution for NIM Status
bin/lib/nim.js
nimStatus and nimStatusByName now accept a port parameter. nimStatusByName determines the host port from the provided parameter or by executing docker port to query the container's port mapping, then performs health checks against the resolved port instead of hardcoded localhost:8000. waitForNimHealth refactored to use intervalSec constant for sleep duration.
Test Coverage for Port Resolution
test/nim.test.js
Added loadNimWithMockedRunner helper for dynamic module reloading with mocked runner functions. New nimStatusByName test suite with four behavioral tests covering: explicit port parameter handling, automatic port discovery via Docker, fallback to port 8000 when unavailable, and stopped container detection.

Sequence Diagram

sequenceDiagram
    participant Caller as Caller
    participant nimStatusByName as nimStatusByName()
    participant Docker as Docker CLI
    participant HealthCheck as HTTP Health Check

    Caller->>nimStatusByName: nimStatusByName(containerName, port?)
    
    nimStatusByName->>Docker: docker inspect containerName
    Docker-->>nimStatusByName: container state (running/stopped)
    
    alt Container Running
        alt Port Parameter Provided
            nimStatusByName->>HealthCheck: curl http://localhost:PORT/v1/models
            HealthCheck-->>nimStatusByName: health status
        else Port Parameter Not Provided
            nimStatusByName->>Docker: docker port containerName 8000
            Docker-->>nimStatusByName: hostPort mapping
            
            alt Mapping Found
                nimStatusByName->>HealthCheck: curl http://localhost:MAPPED_PORT/v1/models
                HealthCheck-->>nimStatusByName: health status
            else No Mapping
                nimStatusByName->>HealthCheck: curl http://localhost:8000/v1/models
                HealthCheck-->>nimStatusByName: health status (fallback)
            end
        end
    else Container Stopped
        nimStatusByName-->>Caller: { running: false, healthy: false }
    end
    
    nimStatusByName-->>Caller: { running: boolean, healthy: boolean }
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A rabbit hops through ports galore,
No more hardcoded eight-zero-zero!
Docker's secrets now revealed with care,
Health checks bloom on custom ports everywhere.
Port resolution, swift and true—
The NIM saga runs brand new! 🚀

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. 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 'fix(nim): use published Docker port in status checks' directly describes the main change: updating nimStatusByName() to use the correct Docker port instead of hardcoding localhost:8000.
Linked Issues check ✅ Passed The PR successfully addresses all coding requirements from issue #1016: nimStatusByName() now accepts a port parameter, retrieves published port via docker port command, and performs health checks against the resolved port.
Out of Scope Changes check ✅ Passed Changes focus on fixing the hardcoded port issue in nimStatusByName() and related functions, with comprehensive test coverage. Updates to waitForNimHealth() and nimStatus() for port parameter consistency are in scope.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[NemoClaw] nimStatusByName() hardcodes port 8000 — health check fails for containers started on custom ports

1 participant