Skip to content

refactor(cli): replace spawnSync("sleep") with native wait utility#2111

Closed
jyaunches wants to merge 12 commits into
NVIDIA:mainfrom
jyaunches:pr-2027-native-sleep
Closed

refactor(cli): replace spawnSync("sleep") with native wait utility#2111
jyaunches wants to merge 12 commits into
NVIDIA:mainfrom
jyaunches:pr-2027-native-sleep

Conversation

@jyaunches
Copy link
Copy Markdown
Contributor

@jyaunches jyaunches commented Apr 20, 2026

Summary

Credit: This PR is based on the original work by @ksapru in #2027. Rebased on current main to incorporate recent security hardening (#1915), NGC login prompt (#2043), installer integrity check (#2046), and docs fixes (#2106).

Replaces all spawnSync("sleep", ...) subprocess calls with a native Node.js sleepSeconds() utility backed by Atomics.wait. This eliminates the overhead of spawning a child process for every delay and removes a platform dependency on the sleep binary.

Related Issue

Refs #2001 (Phase 2: replace sleeps with event-driven waits)
Supersedes #2027

Changes

  • New: src/lib/wait.tssleepMs() and sleepSeconds() primitives using Atomics.wait(SharedArrayBuffer) for CPU-friendly synchronous blocking
  • Refactored: Replaced spawnSync("sleep", ...) in src/nemoclaw.ts, src/lib/onboard.ts, src/lib/deploy.ts, src/lib/agent-onboard.ts, and src/lib/nim.ts
  • Tests: test/wait.test.ts — timing accuracy, edge cases (zero, negative, NaN, Infinity)

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Verification

  • npx prek run --all-files passes
  • npm test passes
  • Tests added or updated for new or changed behavior
  • No secrets, API keys, or credentials committed
  • Docs updated for user-facing behavior changes

AI Disclosure

  • AI-assisted — tool: Claude (pi agent)

Signed-off-by: Test User test@example.com

Summary by CodeRabbit

  • Refactor

    • Replaced ad-hoc OS-level sleeps with a single internal wait utility to standardize delays used in polling and retry flows across the CLI.
  • Tests

    • Added tests verifying the wait utility's timing and edge-case behavior to ensure predictable delays during operations.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 20, 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 Plus

Run ID: a1122f33-3cbe-453f-8436-619c90afeea1

📥 Commits

Reviewing files that changed from the base of the PR and between b3ec7c4 and 821bfa9.

📒 Files selected for processing (1)
  • src/lib/nim.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/lib/nim.ts

📝 Walkthrough

Walkthrough

Replaces ad-hoc OS-level sleep calls with a shared blocking sleep utility (sleepMs / sleepSeconds implemented with SharedArrayBuffer + Atomics.wait) and updates callers to use sleepSeconds() for delays in health/probe and retry loops.

Changes

Cohort / File(s) Summary
Wait Module
src/lib/wait.ts
Adds sleepMs(ms: number): void and sleepSeconds(seconds: number): void implemented with SharedArrayBuffer + Atomics.wait(), including input validation for non-positive and non-finite values.
Sleep Implementation Migration
src/lib/agent-onboard.ts, src/lib/deploy.ts, src/lib/nim.ts, src/lib/onboard.ts, src/nemoclaw.ts
Replaces local/inline spawnSync("sleep", ...) invocations with imported sleepSeconds(); preserves existing loop bounds, retry logic, and timeouts.
Tests
test/wait.test.ts
Adds Vitest suite validating timing behavior for sleepMs() and sleepSeconds() (positive durations) and quick-return behavior for non-positive/non-finite inputs.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I tap the buffer, still and small,

Atomics hush — I hear no call.
No spawned shells to wake the night,
One quiet wait keeps timing right.
Hops in sync, the code feels light.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: replacing spawnSync("sleep") calls with a native wait utility, which is the primary objective of this refactor.

✏️ 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.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/lib/nim.ts (1)

276-277: Remove stale ESLint suppression near the new sleep call.

The suppression targets no-require-imports, but this line no longer uses require.

🧹 Proposed cleanup
-    // eslint-disable-next-line `@typescript-eslint/no-require-imports`
     sleepSeconds(intervalSec);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/lib/nim.ts` around lines 276 - 277, Remove the stale ESLint suppression
placed directly above the sleepSeconds call: delete the comment "//
eslint-disable-next-line `@typescript-eslint/no-require-imports`" that precedes
sleepSeconds(intervalSec); in the code (referencing the sleepSeconds invocation)
so the file no longer silences a rule that isn’t relevant; if other nearby lines
still require suppressions, replace with the appropriate rule only where needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/lib/nim.ts`:
- Around line 276-277: Remove the stale ESLint suppression placed directly above
the sleepSeconds call: delete the comment "// eslint-disable-next-line
`@typescript-eslint/no-require-imports`" that precedes sleepSeconds(intervalSec);
in the code (referencing the sleepSeconds invocation) so the file no longer
silences a rule that isn’t relevant; if other nearby lines still require
suppressions, replace with the appropriate rule only where needed.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: a25186c7-8d8b-4293-92f3-8fd79ab6378f

📥 Commits

Reviewing files that changed from the base of the PR and between ed38d45 and 7fda280.

📒 Files selected for processing (7)
  • src/lib/agent-onboard.ts
  • src/lib/deploy.ts
  • src/lib/nim.ts
  • src/lib/onboard.ts
  • src/lib/wait.ts
  • src/nemoclaw.ts
  • test/wait.test.ts

@wscurran wscurran added refactor This is a refactor of the code and/or architecture. NemoClaw CLI Use this label to identify issues with the NemoClaw command-line interface (CLI). labels Apr 20, 2026
@jyaunches jyaunches closed this Apr 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NemoClaw CLI Use this label to identify issues with the NemoClaw command-line interface (CLI). refactor This is a refactor of the code and/or architecture.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants