refactor(cli): replace spawnSync("sleep") with native wait utility#2027
refactor(cli): replace spawnSync("sleep") with native wait utility#2027brandonpelfrey merged 16 commits intoNVIDIA:mainfrom
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes synchronous blocking sleep into Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/lib/wait.ts (1)
14-15: Avoid per-callSharedArrayBufferallocation in hot polling paths.Reuse one module-level buffer to reduce allocation churn.
♻️ Proposed refactor
+const SLEEP_BUFFER = new Int32Array(new SharedArrayBuffer(4)); + export function sleepMs(ms: number): void { if (!Number.isFinite(ms) || ms <= 0) return; - const buffer = new Int32Array(new SharedArrayBuffer(4)); - Atomics.wait(buffer, 0, 0, ms); + Atomics.wait(SLEEP_BUFFER, 0, 0, ms); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/wait.ts` around lines 14 - 15, Replace the per-call allocation of "buffer" (new Int32Array(new SharedArrayBuffer(4))) with a single module-level Int32Array instance (e.g., const WAIT_BUFFER = new Int32Array(new SharedArrayBuffer(4))) and have the wait function call Atomics.wait(WAIT_BUFFER, 0, 0, ms) instead of creating a new SharedArrayBuffer each invocation; update references to "buffer" in the function to use WAIT_BUFFER so allocation churn is removed while keeping Atomics.wait semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/deploy.ts`:
- Line 336: The module calls the helper sleepSeconds but never imports it,
causing type/runtime failures; fix by adding an import for sleepSeconds from the
wait module at the top of the deploy module (i.e., add an import statement for
sleepSeconds) so the calls to sleepSeconds() in this file resolve correctly and
TypeScript compiles.
In `@src/lib/onboard.ts`:
- Around line 54-56: The file still uses the local sleep() helper which invokes
spawnSync("sleep") instead of the imported sleepSeconds from ./wait; replace the
local spawnSync-based sleep implementation (and all its callers) to use the
exported sleepSeconds utility: remove or refactor the sleep() function to call
or return sleepSeconds(seconds) and make callers await sleepSeconds(...) (or
adapt to its promise-based API) so the native wait utility is actually used;
ensure any references to spawnSync("sleep") are removed and tests/flows that
relied on synchronous behavior are adjusted to the async Promise-based contract.
In `@src/lib/wait.ts`:
- Around line 12-16: sleepMs currently calls Atomics.wait with ms even when ms
is NaN or Infinity (which bypasses the ms <= 0 check and can block
indefinitely); update the guard in sleepMs to first verify the timeout is a
finite number (use Number.isFinite or equivalent) and only proceed to create the
SharedArrayBuffer and call Atomics.wait when ms is finite and greater than
0—otherwise return early (or reject) to avoid passing non-finite timeouts to
Atomics.wait; refer to the sleepMs function and the Atomics.wait/buffer usage to
locate and apply this change.
In `@test/wait.test.ts`:
- Around line 9-28: The tests for sleepMs and sleepSeconds use Date.now() and a
brittle upper bound of 200ms; switch to a monotonic timer (performance.now()) in
both tests and relax the upper bound to something CI-friendly (e.g., assert
duration >= 100 and duration < 500) so the assertions for functions sleepMs and
sleepSeconds are stable on loaded runners; update both test blocks ("sleepMs
blocks for approximately the requested time" and "sleepSeconds blocks for
approximately the requested time") to use performance.now() and the wider upper
bound.
---
Nitpick comments:
In `@src/lib/wait.ts`:
- Around line 14-15: Replace the per-call allocation of "buffer" (new
Int32Array(new SharedArrayBuffer(4))) with a single module-level Int32Array
instance (e.g., const WAIT_BUFFER = new Int32Array(new SharedArrayBuffer(4)))
and have the wait function call Atomics.wait(WAIT_BUFFER, 0, 0, ms) instead of
creating a new SharedArrayBuffer each invocation; update references to "buffer"
in the function to use WAIT_BUFFER so allocation churn is removed while keeping
Atomics.wait semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ac39f487-def3-49eb-bf0d-9bdac481c3b6
📒 Files selected for processing (7)
src/lib/agent-onboard.tssrc/lib/deploy.tssrc/lib/nim.tssrc/lib/onboard.tssrc/lib/wait.tssrc/nemoclaw.tstest/wait.test.ts
wscurran
added
NemoClaw CLI
|
✨ Thanks for submitting this PR that proposes a refactor to replace spawnSync('sleep') with a native wait utility, which could help improve the performance and robustness of the CLI. Possibly related open issues: |
|
Hey @ksapru — nice work on this! I rebased on main and ran CI on a parallel PR (#2111) and hit two issues you'll want to fix before this can merge: 1. - import { sleepMs, sleepSeconds } from "../src/lib/wait.ts";
+ import { sleepMs, sleepSeconds } from "../src/lib/wait.js";The CLI tsconfig doesn't have 2. The old line was - // eslint-disable-next-line @typescript-eslint/no-require-imports
sleepSeconds(intervalSec);Once those are in, this should be green. I'll close #2111 in favor of yours once you've got it. 👍 |
|
Sounds good, thanks @jyaunches. I'll get this done shortly. |
|
Approved @brandonpelfrey can you merge? |
5 participants
Summary
This PR refactors the NemoClaw CLI to replace inefficient and platform-dependent
spawnSync("sleep")subprocess calls with a native Node.jswaitutility. By usingAtomics.wait, we provide a reliable synchronous sleep mechanism that avoids the overhead of spawning new processes, improving performance and robustness during onboarding and deployment.Related Issue
Phase #2001
Changes
sleepMs()andsleepSeconds()primitives.spawnSync("sleep")for nativesleepSeconds()insrc/nemoclaw.ts,src/lib/onboard.ts,src/lib/deploy.ts,src/lib/agent-onboard.ts, andsrc/lib/nim.ts.test/wait.test.tsto verify timing accuracy and updatedrunner.test.ts.Type of Change
Verification
npx prek run --all-filespasses (Several pre-existing unrelated flakes detected in the full suite)npm testpasses (Verified focused suites:wait,onboard,cli,deploy,agent-onboard, andnim)make docsbuilds without warnings (doc changes only)AI Disclosure
Signed-off-by: Krish Sapru ksapru@bu.edu
Summary by CodeRabbit
New Features
Refactor
Tests