fix(docker): relax vLLM pull stall watchdog

[chengjiew]

Fixes #6399

Problem

Fresh DGX Spark installs can spend more than 120 seconds without a new recognized docker pull progress line while Docker finishes a large managed vLLM image pull. The existing stall watchdog treated that quiet period as a failure and killed a pull that can otherwise complete successfully.

Solution

Increase the default Docker pull stall watchdog window from 120 seconds to 15 minutes while keeping the existing 12-hour maximum safety budget. This keeps protection against genuinely stuck pulls but avoids aborting valid large-image pulls during quiet Docker finalization periods.

Verification

• npx vitest run --project cli src/lib/adapters/docker/pull.test.ts
• npx vitest run --project cli src/lib/inference/vllm.test.ts
• git diff --check
• npm run typecheck:cli

Notes

A live DGX Spark cold-pull repro was not run from this workstation; the regression test covers the reported boundary where the old 120-second idle window killed the pull after Docker had emitted pull progress.

Summary by CodeRabbit

• Bug Fixes
  • Increased the default Docker image pull watchdog “stall” timeout to 15 minutes to reduce premature termination of quiet pulls that produce minimal output during finalization.
• Tests
  • Added Vitest coverage for quiet Docker pulls, verifying successful completion within the stall window (no termination signal sent) and that pulls are terminated with the expected timeout behavior both for the default setting and a custom 5-minute stall timeout.

Signed-off-by: Chengjie Wang chengjiew@nvidia.com

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

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: c492747b-055d-490b-859d-dda376d07869

📥 Commits

Reviewing files that changed from the base of the PR and between e949bfe and 8cb9ea9.

📒 Files selected for processing (2)

• src/lib/adapters/docker/pull.test.ts
• src/lib/inference/vllm.ts

✅ Files skipped from review due to trivial changes (1)

• src/lib/inference/vllm.ts

---

📝 Walkthrough

Walkthrough

The Docker pull watchdog now waits 15 minutes before treating a quiet pull as stalled, and tests cover completion before that threshold plus timeout behavior at the default and custom limits.

Changes

Docker Pull Stall Timeout

Layer / File(s)	Summary
Stall timeout constant and comments src/lib/adapters/docker/pull.ts, src/lib/inference/vllm.ts	The default stall timeout is now 15 minutes, is file-local, and related comments describe quiet vLLM pull behavior and the shared stall limit.
Quiet pull watchdog tests src/lib/adapters/docker/pull.test.ts	Three watchdog tests cover quiet pulls that complete before the timeout, exceed the default stall timeout, and exceed a custom stall timeout.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 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 summarizes the main change: increasing the Docker pull stall watchdog for vLLM pulls.
Linked Issues check	✅ Passed	The PR raises the stall timeout, preserves the max budget, and adds coverage for quiet but healthy pulls, matching the issue.
Out of Scope Changes check	✅ Passed	The changes are limited to the watchdog timeout, related tests, and explanatory comments, with no unrelated functionality added.
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 fix/6399_vllm_pull_timeout

---

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

[github-code-quality]

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the fix/6399_vllm_pull_t... branch is 96%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.

File	main	fix/6399_vllm_pull_t... 8cb9ea9	+/-
nemoclaw/src/se...cret-scanner.ts	—	100%	—
nemoclaw/src/commands/slash.ts	—	100%	—
nemoclaw/src/li...bprocess-env.ts	—	100%	—
nemoclaw/src/bl...eprint/state.ts	—	98%	—
nemoclaw/src/onboard/config.ts	—	98%	—
nemoclaw/src/bl...int/snapshot.ts	—	97%	—
nemoclaw/src/bl...print/runner.ts	—	95%	—
nemoclaw/src/co...ration-state.ts	—	94%	—
nemoclaw/src/bl...ate-networks.ts	—	94%	—
nemoclaw/src/index.ts	—	94%	—

TypeScript / code-coverage/cli

The overall coverage in the fix/6399_vllm_pull_t... branch is 76%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.

File	main	fix/6399_vllm_pull_t... 8cb9ea9	+/-
src/lib/onboard/preflight.ts	—	82%	—
src/lib/state/o...oard-session.ts	—	82%	—
src/lib/actions...all/run-plan.ts	—	81%	—
src/lib/actions...licy-channel.ts	—	79%	—
src/lib/actions...box/snapshot.ts	—	79%	—
src/lib/state/sandbox.ts	—	75%	—
src/lib/onboard...er-gpu-patch.ts	—	69%	—
src/lib/policy/index.ts	—	65%	—
src/lib/shields/index.ts	—	61%	—
src/lib/onboard.ts	—	28%	—

---

_{Updated July 08, 2026 08:55 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.}

[github-actions]

PR Review Advisor — No blocking findings

Merge posture: No blocking advisor findings
Primary next action: Add or justify PRA-T1 and any related test follow-ups.
Open items: 0 required · 0 warnings · 0 suggestions · 8 test follow-ups
Since last review: 0 prior items resolved · 1 still applies · 0 new items found

Action checklist

• PRA-T1 Add or justify test follow-up: Runtime validation
• PRA-T2 Add or justify test follow-up: Runtime validation
• PRA-T3 Add or justify test follow-up: Runtime validation
• PRA-T4 Add or justify test follow-up: Acceptance clause
• PRA-T5 Add or justify test follow-up: Acceptance clause
• PRA-T6 Add or justify test follow-up: Acceptance clause
• PRA-T7 Add or justify test follow-up: Acceptance clause
• PRA-T8 Add or justify test follow-up: Acceptance clause

Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

• PRA-T1 Runtime validation — Add or identify targeted runtime validation for a fresh DGX Spark Ubuntu 24.04 aarch64 pull of `nvcr.io/nvidia/vllm:26.05.post1-py3` where Docker is quiet for longer than 120 seconds after a recognized progress line and completes before the 15 minute stall timeout.. Unit coverage is focused and covers the changed watchdog boundary, custom override behavior, and the Spark caller contract. Because the reported failure depends on real DGX Spark, Docker, NGC, and cold-pull output timing, targeted runtime validation would still improve confidence without changing the code review result.
• PRA-T2 Runtime validation — Add or identify install-flow evidence that both DGX Spark Express and interactive non-Express managed vLLM paths reach the shared `pullImage()` helper and continue past image pull success to the next install phase.. Unit coverage is focused and covers the changed watchdog boundary, custom override behavior, and the Spark caller contract. Because the reported failure depends on real DGX Spark, Docker, NGC, and cold-pull output timing, targeted runtime validation would still improve confidence without changing the code review result.
• PRA-T3 Runtime validation — If live DGX Spark validation is not practical, cite the unit-contract evidence that Spark `pullImage()` calls `dockerPullWithProgressWatchdog()` without `stallTimeoutMs`, quiet finalization before 15 minutes resolves, and a quiet pull after the default boundary is still killed.. Unit coverage is focused and covers the changed watchdog boundary, custom override behavior, and the Spark caller contract. Because the reported failure depends on real DGX Spark, Docker, NGC, and cold-pull output timing, targeted runtime validation would still improve confidence without changing the code review result.
• PRA-T4 Acceptance clause — On a fresh DGX Spark host, NemoClaw v0.0.75 installation exits before the managed local vLLM image pull finishes. — add test evidence or identify existing coverage. `src/lib/inference/vllm.ts` still routes managed vLLM pulls through `pullImage()`, which calls `dockerPullWithProgressWatchdog()` without a `stallTimeoutMs` override, so the new default in `src/lib/adapters/docker/pull.ts` applies. `src/lib/adapters/docker/pull.test.ts` covers quiet completion before the 15 minute default; no fresh DGX Spark install was run in this review.
• PRA-T5 Acceptance clause — This reproduced in both Express and interactive non-Express installation. — add test evidence or identify existing coverage. The changed code is in the shared managed vLLM `pullImage()` path. The diff does not add separate Express and interactive non-Express flow regressions.
• PRA-T6 Acceptance clause — A direct pull of the same image completed successfully in about 5m24s, and installation completed once the image was cached, so fresh DGX Spark installation is blocked by the installer aborting a valid long-running pull. — add test evidence or identify existing coverage. The default stall window is now 15 minutes, above the reported approximately 5m24s pull. The new unit test allows quiet finalization within that default and a paired negative test still kills after the default boundary. No live NGC cold pull was run.
• PRA-T7 Acceptance clause — Device: DGX Spark (GB10) — add test evidence or identify existing coverage. Existing `src/lib/inference/vllm.test.ts` asserts that `detectVllmProfile({ platform: "spark" })` selects the DGX Spark profile and image. This review did not validate on GB10 hardware.
• PRA-T8 Acceptance clause — OS: Ubuntu 24.04 — add test evidence or identify existing coverage. The changed code is platform-neutral Node timer/subprocess logic, but no Ubuntu 24.04 runtime validation evidence is present in the diff.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

[github-actions]

PR Review Advisor (Nemotron Ultra) — Changes requested

Merge posture: Do not merge yet
Primary next action: Fix PRA-2: Stall watchdog timeout relaxed from 2min to 15min — defense-in-depth boundary weakened; then add or justify PRA-T1.
Open items: 1 required · 4 warnings · 1 suggestion · 6 test follow-ups
Since last review: 1 prior item resolved · 3 still apply · 0 new items found

Action checklist

• PRA-2 Fix: Stall watchdog timeout relaxed from 2min to 15min — defense-in-depth boundary weakened in src/lib/adapters/docker/pull.ts:19
• PRA-1 Resolve or justify: Source-of-truth review needed: src/lib/adapters/docker/pull.ts:19 + src/lib/inference/vllm.ts:227-231
• PRA-3 Resolve or justify: vLLM pullImage() inherits 15min stall timeout for all profiles without explicit override in src/lib/inference/vllm.ts:230
• PRA-4 Resolve or justify: Runtime validation of vLLM image pull on DGX Spark not performed in src/lib/adapters/docker/pull.test.ts:1
• PRA-6 Resolve or justify: Source-of-truth workaround: 15min stall default applied to all profiles including Generic Linux in src/lib/inference/vllm.ts:227
• PRA-T1 Add or justify test follow-up: Runtime validation
• PRA-T2 Add or justify test follow-up: Runtime validation
• PRA-T3 Add or justify test follow-up: Runtime validation
• PRA-T4 Add or justify test follow-up: No test for profile-specific stall timeout in vLLM installer
• PRA-T5 Add or justify test follow-up: Acceptance clause
• PRA-T6 Add or justify test follow-up: src/lib/adapters/docker/pull.ts:19 + src/lib/inference/vllm.ts:227-231
• PRA-5 In-scope improvement: No test for profile-specific stall timeout in vLLM installer in src/lib/adapters/docker/pull.test.ts:1

Findings index

ID	Severity	Category	Location	Required action
PRA-1	Resolve/justify	architecture	—	Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2	Required	security	src/lib/adapters/docker/pull.ts:19	Risk is acknowledged and documented in code comment (DGX Spark vLLM pulls observed ~5.5min quiet period). 15min provides ~2.7x safety margin over observed maximum. Acceptable with explicit justification documented. Consider adding metric/alert for frequent stall timeouts as follow-up.
PRA-3	Resolve/justify	scope	src/lib/inference/vllm.ts:230	Add stallTimeoutSec field to VllmProfile type and pass explicit stallTimeoutMs override in pullImage() call for each profile. If 15min is confirmed suitable for all profiles, document the rationale in a comment at the pullImage() call site.
PRA-4	Resolve/justify	acceptance	src/lib/adapters/docker/pull.test.ts:1	Run integration test: real docker pull of nvcr.io/nvidia/vllm:26.05.post1-py3 on DGX Spark to validate end-to-end behavior with multi-minute quiet period. Document measured quiet period duration. This is out of unit test scope but recommended before merge for full confidence.
PRA-5	Improvement	tests	src/lib/adapters/docker/pull.test.ts:1	When stallTimeoutSec is added to VllmProfile, add a test that mocks dockerPullWithProgressWatchdog and verifies the correct stallTimeoutMs is passed for each profile. Can be deferred to follow-up PR that adds the profile field.
PRA-6	Resolve/justify	security	src/lib/inference/vllm.ts:227	Add stallTimeoutSec to VllmProfile (PRA-2) to make workaround profile-specific. Document removal condition: when profile-specific timeouts added or evidence shows 2min suffices universally.

🚨 Required before merge

Address these before merging unless a maintainer explicitly overrides the advisor with rationale.

PRA-2 Required — Stall watchdog timeout relaxed from 2min to 15min — defense-in-depth boundary weakened

• Location: src/lib/adapters/docker/pull.ts:19
• Category: security
• Problem: DEFAULT_DOCKER_PULL_STALL_TIMEOUT_MS increased from 120,000ms (2min) to 900,000ms (15min). This watchdog is the early detection layer for stuck/malicious docker pulls consuming sandbox resources (CPU, memory, network, disk I/O) without progress. The 12-hour maxTimeoutMs remains as ultimate backstop, but stall detection now allows up to 15 minutes of no-progress resource consumption before SIGTERM.
• Impact: A buggy or malicious image pull could hold sandbox resources for up to 15 minutes without progress before stall detection triggers. This is an intentional relaxation for vLLM large-layer extraction but weakens a security boundary.
• Required action: Risk is acknowledged and documented in code comment (DGX Spark vLLM pulls observed ~5.5min quiet period). 15min provides ~2.7x safety margin over observed maximum. Acceptable with explicit justification documented. Consider adding metric/alert for frequent stall timeouts as follow-up.
• Expected follow-up: Fix before merge or get explicit maintainer override.
• Verification: Review pull.ts:19 comment and pull.ts:100 where stallTimeoutMs is used in watchdog check (now - lastProgressAt >= stallTimeoutMs). Confirm maxTimeoutMs (12h) still bounds total runtime at pull.ts:101.
• Missing regression test: Existing test 'kills a quiet pull after the default 15 minute stall timeout' validates stall detection triggers at the new boundary with default timeouts.
• Done when: The required change is committed and verification passes: Review pull.ts:19 comment and pull.ts:100 where stallTimeoutMs is used in watchdog check (now - lastProgressAt >= stallTimeoutMs). Confirm maxTimeoutMs (12h) still bounds total runtime at pull.ts:101.
• Evidence: pull.ts:19 constant change; pull.ts:100-101 watchdog uses stallTimeoutMs and maxTimeoutMs; pull.test.ts:133-147 new test advances 16min and expects timeoutKind='stall'

Review findings by urgency: 1 required fix, 4 items to resolve/justify, 1 in-scope improvement

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Source-of-truth review needed: src/lib/adapters/docker/pull.ts:19 + src/lib/inference/vllm.ts:227-231

• Location: not file-specific
• Category: architecture
• Problem: The advisor marked localized patch analysis as needs_followup.
• Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
• Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
• Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
• Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
• Missing regression test: Unit test 'allows quiet pull finalization within 15 minute stall timeout' validates logic boundary with fake timers. Does not prove real Docker behavior won't regress — needs integration test
• Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
• Evidence: pull.ts:19 constant + rationale comment; vllm.ts:227-231 comment acknowledging shared default; issue [DGX Spark][Install] v0.0.75 installation aborts during managed vLLM image pull #6399 logs show 5m24s control pull

PRA-3 Resolve/justify — vLLM pullImage() inherits 15min stall timeout for all profiles without explicit override

• Location: src/lib/inference/vllm.ts:230
• Category: scope
• Problem: vLLM pullImage() calls dockerPullWithProgressWatchdog with custom maxTimeoutMs (12h from profile) but does not override stallTimeoutMs, so it inherits the new 15min default. This applies to all three profiles: DGX Spark, DGX Station, and Generic Linux. Generic Linux uses a smaller image (26.03.post1 + Nemotron-Nano-4B) and may not need a 15min stall window.
• Impact: If any deployment profile has different stall characteristics (e.g., slower storage, different layer sizes), the 15min default may be too short or too long. Generic Linux profile may unnecessarily allow 15min of no-progress resource consumption, violating least privilege.
• Recommended action: Add stallTimeoutSec field to VllmProfile type and pass explicit stallTimeoutMs override in pullImage() call for each profile. If 15min is confirmed suitable for all profiles, document the rationale in a comment at the pullImage() call site.
• Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
• Verification: Review vllm.ts:230-231 — only maxTimeoutMs is passed. Check VllmProfile type at vllm.ts:50 for stallTimeoutSec field (does not exist).
• Missing regression test: Integration test with real vLLM image pull on each profile to validate end-to-end stall behavior. Unit test verifying pullImage() passes profile-specific stallTimeoutMs when field is added.
• Done when: The risk is fixed or explicitly justified in the PR. Verification: Review vllm.ts:230-231 — only maxTimeoutMs is passed. Check VllmProfile type at vllm.ts:50 for stallTimeoutSec field (does not exist).
• Evidence: vllm.ts:230-231 passes maxTimeoutMs only; VllmProfile has pullTimeoutSec but no stallTimeoutSec

PRA-4 Resolve/justify — Runtime validation of vLLM image pull on DGX Spark not performed

• Location: src/lib/adapters/docker/pull.test.ts:1
• Category: acceptance
• Problem: PR author notes 'live DGX Spark cold-pull repro was not run from this workstation'. Unit tests with fake timers cover logic boundaries (15min stall, 12h max, 1s watchdog interval) but don't exercise real Docker daemon behavior end-to-end with multi-GB layer extraction and multi-minute quiet periods.
• Impact: End-to-end behavior with real Docker daemon, network, disk I/O, and vLLM image layer structure not validated in this PR. The 2.7x margin (15min vs observed 5.5min) is based on a single control pull observation.
• Recommended action: Run integration test: real docker pull of nvcr.io/nvidia/vllm:26.05.post1-py3 on DGX Spark to validate end-to-end behavior with multi-minute quiet period. Document measured quiet period duration. This is out of unit test scope but recommended before merge for full confidence.
• Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
• Verification: Execute 'docker pull nvcr.io/nvidia/vllm:26.05.post1-py3' on DGX Spark and measure quiet period duration after last progress line.
• Missing regression test: Integration test with real vLLM image pull on DGX Spark hardware (PRA-T1 from prior review).
• Done when: The risk is fixed or explicitly justified in the PR. Verification: Execute 'docker pull nvcr.io/nvidia/vllm:26.05.post1-py3' on DGX Spark and measure quiet period duration after last progress line.
• Evidence: PR body: 'A live DGX Spark cold-pull repro was not run from this workstation'; issue [DGX Spark][Install] v0.0.75 installation aborts during managed vLLM image pull #6399 logs show 5m24s successful control pull

PRA-6 Resolve/justify — Source-of-truth workaround: 15min stall default applied to all profiles including Generic Linux

• Location: src/lib/inference/vllm.ts:227
• Category: security
• Problem: The workaround (15min default for all profiles) handles 'invalid state' = slow layer finalization on Spark. Root cause = Docker layer extraction latency on large NGC images. Source fix would be profile-specific stall timeout (PRA-2) or upstream Docker/registry optimization. Workaround removal condition: when profile-specific timeouts added or evidence shows 2min suffices for all profiles.
• Impact: Generic Linux profile unnecessarily inherits 15min stall window, weakening sandbox resource-exhaustion boundary for that profile. Workaround masks root cause (lack of profile-specific configuration) rather than fixing it at source.
• Recommended action: Add stallTimeoutSec to VllmProfile (PRA-2) to make workaround profile-specific. Document removal condition: when profile-specific timeouts added or evidence shows 2min suffices universally.
• Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
• Verification: Review vllm.ts:227-231 comment acknowledging shared default; confirm VllmProfile lacks stallTimeoutSec.
• Missing regression test: Unit test verifying pullImage() passes profile-specific stallTimeoutMs when field is added (depends on PRA-2).
• Done when: The risk is fixed or explicitly justified in the PR. Verification: Review vllm.ts:227-231 comment acknowledging shared default; confirm VllmProfile lacks stallTimeoutSec.
• Evidence: pull.ts:19 constant change; vllm.ts:227-231 comment; VllmProfile type at vllm.ts:50 lacks stallTimeoutSec

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

PRA-5 Improvement — No test for profile-specific stall timeout in vLLM installer

• Location: src/lib/adapters/docker/pull.test.ts:1
• Category: tests
• Problem: If VllmProfile gains a stallTimeoutSec field (per finding above), there's no test verifying pullImage() passes it correctly to dockerPullWithProgressWatchdog.
• Impact: Without this test, a future change adding profile-specific stall timeout could regress silently.
• Suggested action: When stallTimeoutSec is added to VllmProfile, add a test that mocks dockerPullWithProgressWatchdog and verifies the correct stallTimeoutMs is passed for each profile. Can be deferred to follow-up PR that adds the profile field.
• Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
• Verification: Search pull.test.ts for mock of dockerPullWithProgressWatchdog verifying stallTimeoutMs per profile — no such test exists.
• Missing regression test: New test: mock dockerPullWithProgressWatchdog, call pullImage() for each profile, verify correct stallTimeoutMs passed.
• Done when: The local improvement is applied, or the PR notes why it should be deferred.
• Evidence: vllm.ts:230-231 passes maxTimeoutMs only; no profile-specific stall timeout field exists yet

Simplification opportunities: 5 possible cuts

These are safe simplification checks only. Do not remove validation, security controls, data-loss prevention, or required tests.

• PRA-2 delete (src/lib/adapters/docker/pull.ts:19): None — constant change is minimal and intentional; no abstraction to simplify
  • Replacement: N/A
  • Net: 0 lines
  • Safety boundary: Stall watchdog boundary must remain; do not remove the constant or watchdog logic
• PRA-3 native (src/lib/inference/vllm.ts:230): Shared 15min default assumption across all profiles
  • Replacement: Profile-specific stallTimeoutSec field in VllmProfile with explicit per-profile values
  • Net: 5 lines
  • Safety boundary: Profile-specific stall timeouts must not exceed maxTimeoutMs; 12h max remains ultimate backstop
• PRA-4 delete (src/lib/adapters/docker/pull.test.ts:1): N/A — this is a validation gap, not code to simplify
  • Replacement: N/A
  • Net: 0 lines
  • Safety boundary: Real hardware validation cannot be replaced by unit tests; must run on target DGX Spark
• PRA-5 yagni (src/lib/adapters/docker/pull.test.ts:1): N/A — test depends on PRA-2 fix first
  • Replacement: N/A
  • Net: 0 lines
  • Safety boundary: Test must verify security boundary (stall timeout) is correctly applied per profile
• PRA-6 shrink (src/lib/inference/vllm.ts:227): Shared 15min default comment and assumption
  • Replacement: Profile-specific stallTimeoutSec values with documented rationale per profile
  • Net: 5 lines
  • Safety boundary: Stall watchdog boundary must remain for each profile; maxTimeoutMs (12h) remains ultimate backstop

Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

• PRA-T1 Runtime validation — Integration: real DGX Spark cold pull of nvcr.io/nvidia/vllm:26.05.post1-py3 measuring quiet period duration (validates 2.7× margin). Unit tests with fake timers comprehensively cover logic boundaries (stall/max timeouts, custom override, repeated output, output tail, sub-ms clamping, spawn failure). Runtime/sandbox/infrastructure paths need behavioral runtime validation on DGX Spark hardware to confirm 2.7× margin holds under real Docker daemon, network, disk I/O, and vLLM image layer structure.
• PRA-T2 Runtime validation — Integration: Station profile large image pull validating 15min suffices. Unit tests with fake timers comprehensively cover logic boundaries (stall/max timeouts, custom override, repeated output, output tail, sub-ms clamping, spawn failure). Runtime/sandbox/infrastructure paths need behavioral runtime validation on DGX Spark hardware to confirm 2.7× margin holds under real Docker daemon, network, disk I/O, and vLLM image layer structure.
• PRA-T3 Runtime validation — Unit: profile-specific stall timeout — mock dockerPullWithProgressWatchdog, verify correct stallTimeoutMs passed per profile (requires VllmProfile.stallTimeoutSec first). Unit tests with fake timers comprehensively cover logic boundaries (stall/max timeouts, custom override, repeated output, output tail, sub-ms clamping, spawn failure). Runtime/sandbox/infrastructure paths need behavioral runtime validation on DGX Spark hardware to confirm 2.7× margin holds under real Docker daemon, network, disk I/O, and vLLM image layer structure.
• PRA-T4 No test for profile-specific stall timeout in vLLM installer — When stallTimeoutSec is added to VllmProfile, add a test that mocks dockerPullWithProgressWatchdog and verifies the correct stallTimeoutMs is passed for each profile. Can be deferred to follow-up PR that adds the profile field.
• PRA-T5 Acceptance clause — Fresh DGX Spark installs blocked — should now work — add test evidence or identify existing coverage. Unit tests pass; no live DGX Spark validation (PR body: 'was not run from this workstation')
• PRA-T6 src/lib/adapters/docker/pull.ts:19 + src/lib/inference/vllm.ts:227-231 — Unit test 'allows quiet pull finalization within 15 minute stall timeout' validates logic boundary with fake timers. Does not prove real Docker behavior won't regress — needs integration test. pull.ts:19 constant + rationale comment; vllm.ts:227-231 comment acknowledging shared default; issue [DGX Spark][Install] v0.0.75 installation aborts during managed vLLM image pull #6399 logs show 5m24s control pull

Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: src/lib/adapters/docker/pull.ts:19 + src/lib/inference/vllm.ts:227-231

• Location: not file-specific
• Category: architecture
• Problem: The advisor marked localized patch analysis as needs_followup.
• Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
• Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
• Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
• Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
• Missing regression test: Unit test 'allows quiet pull finalization within 15 minute stall timeout' validates logic boundary with fake timers. Does not prove real Docker behavior won't regress — needs integration test
• Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
• Evidence: pull.ts:19 constant + rationale comment; vllm.ts:227-231 comment acknowledging shared default; issue [DGX Spark][Install] v0.0.75 installation aborts during managed vLLM image pull #6399 logs show 5m24s control pull

PRA-2 Required — Stall watchdog timeout relaxed from 2min to 15min — defense-in-depth boundary weakened

• Location: src/lib/adapters/docker/pull.ts:19
• Category: security
• Problem: DEFAULT_DOCKER_PULL_STALL_TIMEOUT_MS increased from 120,000ms (2min) to 900,000ms (15min). This watchdog is the early detection layer for stuck/malicious docker pulls consuming sandbox resources (CPU, memory, network, disk I/O) without progress. The 12-hour maxTimeoutMs remains as ultimate backstop, but stall detection now allows up to 15 minutes of no-progress resource consumption before SIGTERM.
• Impact: A buggy or malicious image pull could hold sandbox resources for up to 15 minutes without progress before stall detection triggers. This is an intentional relaxation for vLLM large-layer extraction but weakens a security boundary.
• Required action: Risk is acknowledged and documented in code comment (DGX Spark vLLM pulls observed ~5.5min quiet period). 15min provides ~2.7x safety margin over observed maximum. Acceptable with explicit justification documented. Consider adding metric/alert for frequent stall timeouts as follow-up.
• Expected follow-up: Fix before merge or get explicit maintainer override.
• Verification: Review pull.ts:19 comment and pull.ts:100 where stallTimeoutMs is used in watchdog check (now - lastProgressAt >= stallTimeoutMs). Confirm maxTimeoutMs (12h) still bounds total runtime at pull.ts:101.
• Missing regression test: Existing test 'kills a quiet pull after the default 15 minute stall timeout' validates stall detection triggers at the new boundary with default timeouts.
• Done when: The required change is committed and verification passes: Review pull.ts:19 comment and pull.ts:100 where stallTimeoutMs is used in watchdog check (now - lastProgressAt >= stallTimeoutMs). Confirm maxTimeoutMs (12h) still bounds total runtime at pull.ts:101.
• Evidence: pull.ts:19 constant change; pull.ts:100-101 watchdog uses stallTimeoutMs and maxTimeoutMs; pull.test.ts:133-147 new test advances 16min and expects timeoutKind='stall'

PRA-3 Resolve/justify — vLLM pullImage() inherits 15min stall timeout for all profiles without explicit override

• Location: src/lib/inference/vllm.ts:230
• Category: scope
• Problem: vLLM pullImage() calls dockerPullWithProgressWatchdog with custom maxTimeoutMs (12h from profile) but does not override stallTimeoutMs, so it inherits the new 15min default. This applies to all three profiles: DGX Spark, DGX Station, and Generic Linux. Generic Linux uses a smaller image (26.03.post1 + Nemotron-Nano-4B) and may not need a 15min stall window.
• Impact: If any deployment profile has different stall characteristics (e.g., slower storage, different layer sizes), the 15min default may be too short or too long. Generic Linux profile may unnecessarily allow 15min of no-progress resource consumption, violating least privilege.
• Recommended action: Add stallTimeoutSec field to VllmProfile type and pass explicit stallTimeoutMs override in pullImage() call for each profile. If 15min is confirmed suitable for all profiles, document the rationale in a comment at the pullImage() call site.
• Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
• Verification: Review vllm.ts:230-231 — only maxTimeoutMs is passed. Check VllmProfile type at vllm.ts:50 for stallTimeoutSec field (does not exist).
• Missing regression test: Integration test with real vLLM image pull on each profile to validate end-to-end stall behavior. Unit test verifying pullImage() passes profile-specific stallTimeoutMs when field is added.
• Done when: The risk is fixed or explicitly justified in the PR. Verification: Review vllm.ts:230-231 — only maxTimeoutMs is passed. Check VllmProfile type at vllm.ts:50 for stallTimeoutSec field (does not exist).
• Evidence: vllm.ts:230-231 passes maxTimeoutMs only; VllmProfile has pullTimeoutSec but no stallTimeoutSec

PRA-4 Resolve/justify — Runtime validation of vLLM image pull on DGX Spark not performed

• Location: src/lib/adapters/docker/pull.test.ts:1
• Category: acceptance
• Problem: PR author notes 'live DGX Spark cold-pull repro was not run from this workstation'. Unit tests with fake timers cover logic boundaries (15min stall, 12h max, 1s watchdog interval) but don't exercise real Docker daemon behavior end-to-end with multi-GB layer extraction and multi-minute quiet periods.
• Impact: End-to-end behavior with real Docker daemon, network, disk I/O, and vLLM image layer structure not validated in this PR. The 2.7x margin (15min vs observed 5.5min) is based on a single control pull observation.
• Recommended action: Run integration test: real docker pull of nvcr.io/nvidia/vllm:26.05.post1-py3 on DGX Spark to validate end-to-end behavior with multi-minute quiet period. Document measured quiet period duration. This is out of unit test scope but recommended before merge for full confidence.
• Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
• Verification: Execute 'docker pull nvcr.io/nvidia/vllm:26.05.post1-py3' on DGX Spark and measure quiet period duration after last progress line.
• Missing regression test: Integration test with real vLLM image pull on DGX Spark hardware (PRA-T1 from prior review).
• Done when: The risk is fixed or explicitly justified in the PR. Verification: Execute 'docker pull nvcr.io/nvidia/vllm:26.05.post1-py3' on DGX Spark and measure quiet period duration after last progress line.
• Evidence: PR body: 'A live DGX Spark cold-pull repro was not run from this workstation'; issue [DGX Spark][Install] v0.0.75 installation aborts during managed vLLM image pull #6399 logs show 5m24s successful control pull

PRA-5 Improvement — No test for profile-specific stall timeout in vLLM installer

• Location: src/lib/adapters/docker/pull.test.ts:1
• Category: tests
• Problem: If VllmProfile gains a stallTimeoutSec field (per finding above), there's no test verifying pullImage() passes it correctly to dockerPullWithProgressWatchdog.
• Impact: Without this test, a future change adding profile-specific stall timeout could regress silently.
• Suggested action: When stallTimeoutSec is added to VllmProfile, add a test that mocks dockerPullWithProgressWatchdog and verifies the correct stallTimeoutMs is passed for each profile. Can be deferred to follow-up PR that adds the profile field.
• Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
• Verification: Search pull.test.ts for mock of dockerPullWithProgressWatchdog verifying stallTimeoutMs per profile — no such test exists.
• Missing regression test: New test: mock dockerPullWithProgressWatchdog, call pullImage() for each profile, verify correct stallTimeoutMs passed.
• Done when: The local improvement is applied, or the PR notes why it should be deferred.
• Evidence: vllm.ts:230-231 passes maxTimeoutMs only; no profile-specific stall timeout field exists yet

PRA-6 Resolve/justify — Source-of-truth workaround: 15min stall default applied to all profiles including Generic Linux

• Location: src/lib/inference/vllm.ts:227
• Category: security
• Problem: The workaround (15min default for all profiles) handles 'invalid state' = slow layer finalization on Spark. Root cause = Docker layer extraction latency on large NGC images. Source fix would be profile-specific stall timeout (PRA-2) or upstream Docker/registry optimization. Workaround removal condition: when profile-specific timeouts added or evidence shows 2min suffices for all profiles.
• Impact: Generic Linux profile unnecessarily inherits 15min stall window, weakening sandbox resource-exhaustion boundary for that profile. Workaround masks root cause (lack of profile-specific configuration) rather than fixing it at source.
• Recommended action: Add stallTimeoutSec to VllmProfile (PRA-2) to make workaround profile-specific. Document removal condition: when profile-specific timeouts added or evidence shows 2min suffices universally.
• Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
• Verification: Review vllm.ts:227-231 comment acknowledging shared default; confirm VllmProfile lacks stallTimeoutSec.
• Missing regression test: Unit test verifying pullImage() passes profile-specific stallTimeoutMs when field is added (depends on PRA-2).
• Done when: The risk is fixed or explicitly justified in the PR. Verification: Review vllm.ts:227-231 comment acknowledging shared default; confirm VllmProfile lacks stallTimeoutSec.
• Evidence: pull.ts:19 constant change; vllm.ts:227-231 comment; VllmProfile type at vllm.ts:50 lacks stallTimeoutSec

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

[github-actions]

E2E Advisor Recommendation

Required E2E: None
Optional E2E: gpu-e2e, inference-routing

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

• None.

Optional E2E

• gpu-e2e (high): Closest existing live GPU/local-inference job. It validates Docker, GPU host setup, local inference onboarding, sandbox inference.local routing, and auth-proxy behavior, but it uses Ollama rather than vLLM and therefore does not directly cover the changed vLLM Docker pull watchdog path.
• inference-routing (medium): Adjacent confidence for inference selection/routing and sandbox inference.local behavior. It does not exercise managed vLLM image pulls, so it is useful but not merge-blocking for this specific watchdog timeout change.

New E2E recommendations

• managed-vllm-local-inference-install (high): No existing live E2E job appears to run the managed vLLM install path or assert that large vLLM Docker image pulls tolerate long quiet finalization while remaining bounded by the profile max timeout.
  • Suggested test: Add a dedicated vllm-install live E2E target that runs non-interactive managed vLLM setup on a suitable NVIDIA GPU runner, validates the Docker pull watchdog behavior/artifacts, confirms the vLLM container starts, and sends a PONG request through sandbox inference.local.
• docker-image-pull-watchdog (medium): The watchdog is currently covered by unit tests with fake timers, but the production failure mode involves real Docker CLI output and quiet layer finalization during large image pulls.
  • Suggested test: Add an integration/E2E-style Docker pull watchdog smoke that exercises dockerPullWithProgressWatchdog against a controlled local registry/image or scripted Docker-compatible pull output under real subprocess IO, verifying stall and max-timeout diagnostics without requiring a multi-hour NGC pull.

[github-actions]

E2E Target Recommendation

Required E2E targets: None
Optional E2E targets: None

Workflow run

Full E2E target advisor summary

E2E Target Advisor

Base: origin/main
Head: HEAD
Confidence: medium

Required E2E targets

• None. The PR changes the Docker pull watchdog and vLLM image-pull path, but the current e2e.yaml live targets/jobs do not wire a vLLM/local-vLLM install path that would exercise this changed surface. The accompanying test change is outside test/e2e/ and is covered by unit tests rather than the E2E target workflow.

Optional E2E targets

• None.

Relevant changed files

• src/lib/adapters/docker/pull.ts
• src/lib/inference/vllm.ts

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/lib/adapters/docker/pull.test.ts (1)

110-129: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Boundary test doesn't reach the boundary it claims to cover.

This test asserts a quiet pull survives "within the 15 minute stall timeout," but only advances fake timers by 121 seconds — well inside the old 120s threshold, not anywhere near the new 900s (15-minute) one. It would pass even if the stall timeout were still much shorter than 15 minutes, so it doesn't actually exercise the new default's boundary the way the commit message ("cover vllm pull stall boundary") and the sibling test (130-147, advancing 16 minutes) intend.

Advance closer to the actual threshold (e.g. just under 900_000ms) so the assertion is meaningful:

🧪 Suggested boundary-accurate advance

-    await vi.advanceTimersByTimeAsync(121_000);
+    // Just under the 15-minute default stall timeout; must not be killed.
+    await vi.advanceTimersByTimeAsync(15 * 60_000 - 1_000);

As per path instructions, tests should be flagged for "conditionals that make a test pass without exercising its claim."

🤖 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/adapters/docker/pull.test.ts` around lines 110 - 129, The quiet-pull
boundary test in dockerPullWithProgressWatchdog is not exercising the claimed
15-minute stall window because it only advances timers by 121_000ms. Update this
test to advance fake time much closer to the actual 900_000ms threshold so it
meaningfully verifies the new default boundary, while keeping the existing
assertions on child.kill and the resolved pull result.

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/adapters/docker/pull.test.ts`:
- Around line 110-129: The quiet-pull boundary test in
dockerPullWithProgressWatchdog is not exercising the claimed 15-minute stall
window because it only advances timers by 121_000ms. Update this test to advance
fake time much closer to the actual 900_000ms threshold so it meaningfully
verifies the new default boundary, while keeping the existing assertions on
child.kill and the resolved pull result.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 775c19a8-15e7-41e2-97ce-c1fe73f84fda

📥 Commits

Reviewing files that changed from the base of the PR and between 91ffb67 and fce6878.

📒 Files selected for processing (2)

• src/lib/adapters/docker/pull.test.ts
• src/lib/adapters/docker/pull.ts

[prekshivyas]

Advisor follow-up / maintainer rationale:

• PRA-1 / PRA-3: The 15-minute stall window is an intentional bounded tradeoff. The reported successful control pull was quiet for about 5m24s, so 15 minutes provides roughly 2.8x headroom for large-layer finalization while the independent 12-hour maximum still caps total wall time. Keeping this as the Docker-pull default is appropriate across vLLM profiles because the quiet period comes from Docker layer finalization rather than profile-specific application behavior; callers can still override stallTimeoutMs if a future profile needs a different budget.
• PRA-2 / PRA-T2: Addressed in e949bfe by widening the success-boundary test margin from 1 second to 10 seconds. Focused watchdog tests pass (10/10), and the normal pre-push TypeScript/hooks pass.
• PRA-T1 / PRA-4: A live cold pull on DGX Spark is not available in this environment. The E2E Advisor marked required E2E as None (optional: gpu-e2e, spark-install). Existing coverage verifies that the managed vLLM path inherits this default, that quiet finalization succeeds below it, and that a truly quiet pull is terminated at the new boundary. The issue's measured 5m24s control pull provides the runtime evidence motivating the budget.

On that evidence, I accept the bounded delay in stall detection for this fix; no unbounded-hang behavior is introduced.

[prekshivyas]

PR Advisor source-of-truth disposition and explicit maintainer override:

• Invalid state: a valid Docker pull enters quiet large-layer finalization for longer than the old 2-minute stall budget and is misclassified as stalled.
• Source boundary: progress output is owned by the Docker daemon/registry layer-finalization path; NemoClaw cannot force upstream Docker to emit progress while finalizing a layer.
• Why not source-fix here: the observed control pull completed successfully after about 5m24s, and this repository can only choose a watchdog budget around that external behavior.
• Regression evidence: focused fake-timer tests now cover successful quiet finalization below 15 minutes, termination at the 15-minute default, and a caller-provided custom stall timeout. The full CLI suite and typecheck are running in CI.
• Scope: all vLLM profiles intentionally share the Docker-layer finalization budget; the profile-specific maximum remains the hard total-runtime cap. Callers retain a tested stallTimeoutMs override.
• Removal/tightening condition: replace the shared budget with profile-specific values when measured runtime evidence exists for each profile or Docker exposes a reliable finalization-progress signal.

I explicitly accept the bounded increase in no-progress detection latency for this bug fix. The 15-minute stall guard and independent total-runtime cap remain enforced; this is not an unbounded resource-consumption path. Live DGX hardware is unavailable here, so the reported 5m24s successful control pull plus the deterministic boundary tests are the available evidence.

[prekshivyas]

Reviewed the watchdog implementation, vLLM call path, and tests. All CI and dispatched sandbox E2Es are green; GPT-5.5 reports no blocking findings. I accept the bounded 15-minute stall-detection tradeoff documented in the PR: the observed valid quiet pull took ~5m24s, deterministic tests cover success/default/custom timeout boundaries, profile max time still caps total runtime, and the source-boundary/removal conditions are explicitly recorded. Nemotron’s remaining item is therefore dispositioned by maintainer override.