fix(preflight): auto-create swap on low-memory VMs to prevent OOM during sandbox build - #419
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds Linux memory inspection and conditional swap remediation: new Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant Onboard as onboard.js
participant Preflight as bin/lib/preflight
participant OS as OS/Shell
participant Sandbox as Sandbox Builder
User->>Onboard: run "nemoclaw onboard"
Onboard->>Preflight: call getMemoryInfo()
Preflight->>OS: read /proc/meminfo or run sysctl
OS-->>Preflight: return memory+swap totals
Preflight-->>Onboard: return totals
alt Linux and total < threshold
Onboard->>User: prompt to create 4 GB swap (interactive)
User-->>Onboard: consent or decline
Onboard->>Preflight: call ensureSwap(12000)
Preflight->>OS: check /swapfile, create/enable swap, update /etc/fstab
OS-->>Preflight: creation/activation result
Preflight-->>Onboard: {ok, swapCreated?, reason?}
else Memory OK or non-Linux
Onboard-->>Onboard: log "Memory OK" / skip swap
end
Onboard->>Sandbox: continue sandbox creation
Sandbox-->>User: sandbox create result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/setup.sh (1)
176-176: Risk of duplicate/etc/fstabentries on re-runs.If
/swapfileis manually deleted and the script is re-run, the/etc/fstabappend will create duplicate entries. Consider checking for existing entry first.Proposed fix
- echo '/swapfile none swap sw 0 0' | sudo tee -a /etc/fstab + grep -q '/swapfile' /etc/fstab || echo '/swapfile none swap sw 0 0' | sudo tee -a /etc/fstab🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup.sh` at line 176, Avoid appending a duplicate fstab entry by checking /etc/fstab for the exact line '/swapfile none swap sw 0 0' before running the echo/tee append; in the setup.sh code where you currently call "echo '/swapfile none swap sw 0 0' | sudo tee -a /etc/fstab", add a conditional that greps /etc/fstab for that exact string and only performs the append when not found, ensuring idempotent behavior on re-runs.bin/lib/preflight.js (1)
218-221: Potential duplicate/etc/fstabentry if swapfile was previously removed manually.Similar to the shell script, if a user manually removes
/swapfilebut the fstab entry remains from a previous run, this will append a duplicate entry.Proposed fix
- runCapture( - "echo '/swapfile none swap sw 0 0' | sudo tee -a /etc/fstab", - { ignoreError: false } - ); + runCapture( + "grep -q '/swapfile' /etc/fstab || echo '/swapfile none swap sw 0 0' | sudo tee -a /etc/fstab", + { ignoreError: false } + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/preflight.js` around lines 218 - 221, The current runCapture call unconditionally appends "/swapfile none swap sw 0 0" to /etc/fstab which can create duplicate entries; update the runCapture invocation that contains the command string "echo '/swapfile none swap sw 0 0' | sudo tee -a /etc/fstab" to first check /etc/fstab for the exact line and only append if missing (for example using a grep check or similar conditional) so the line is not added twice if it already exists; keep the change local to the runCapture call in preflight.js.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/onboard.js`:
- Around line 333-340: The current log treats `{ ok: true, swapCreated: false }`
as "Memory OK" even when memory is low; update the conditional handling around
swapResult and mem in the onboard.js branch that prints `Memory OK:
${mem.totalRamMB} MB RAM + ${mem.totalSwapMB} MB swap` so it distinguishes a
pre-existing swap from a successful creation and reports accurate status: if
swapResult.swapCreated is false and total RAM+swap is still below the threshold
(e.g., 12000 MB) log a warning that swap already existed but memory is still low
(include mem.totalRamMB and mem.totalSwapMB and swapResult.reason/state),
otherwise keep the positive message when totals are sufficient; adjust messages
around ensureSwap/swapResult to reflect swap pre-existence vs newly created
swap.
In `@scripts/setup.sh`:
- Line 166: The MIN_TOTAL_MB constant in setup.sh is set to 6144 which conflicts
with the 12000 MB requirement enforced elsewhere (ensureSwap and preflight
defaults); update MIN_TOTAL_MB to 12000 so the shell setup check aligns with
bin/lib/onboard.js's ensureSwap(12000) and bin/lib/preflight.js defaults,
ensuring the VM memory/swap preflight threshold is consistent across scripts.
---
Nitpick comments:
In `@bin/lib/preflight.js`:
- Around line 218-221: The current runCapture call unconditionally appends
"/swapfile none swap sw 0 0" to /etc/fstab which can create duplicate entries;
update the runCapture invocation that contains the command string "echo
'/swapfile none swap sw 0 0' | sudo tee -a /etc/fstab" to first check /etc/fstab
for the exact line and only append if missing (for example using a grep check or
similar conditional) so the line is not added twice if it already exists; keep
the change local to the runCapture call in preflight.js.
In `@scripts/setup.sh`:
- Line 176: Avoid appending a duplicate fstab entry by checking /etc/fstab for
the exact line '/swapfile none swap sw 0 0' before running the echo/tee append;
in the setup.sh code where you currently call "echo '/swapfile none swap sw 0 0'
| sudo tee -a /etc/fstab", add a conditional that greps /etc/fstab for that
exact string and only performs the append when not found, ensuring idempotent
behavior on re-runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ddd2bc0-4f6c-4a6f-b97a-5585af63ea2c
📒 Files selected for processing (5)
bin/lib/onboard.jsbin/lib/preflight.jsdocs/reference/troubleshooting.mdscripts/setup.shtest/preflight.test.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
bin/lib/preflight.js (1)
228-245: Consider cleanup on partial failure.If swap creation fails partway (e.g.,
ddsucceeds butmkswapfails), a 4 GB/swapfileremains on disk without being formatted as swap. The helpful error message guides manual recovery, but the orphaned file could be confusing.🧹 Optional: Clean up on failure
// Create 4 GB swap file try { runCapture("sudo dd if=/dev/zero of=/swapfile bs=1M count=4096 status=none", { ignoreError: false }); runCapture("sudo chmod 600 /swapfile", { ignoreError: false }); runCapture("sudo mkswap /swapfile", { ignoreError: false }); runCapture("sudo swapon /swapfile", { ignoreError: false }); runCapture( "grep -q '/swapfile' /etc/fstab || echo '/swapfile none swap sw 0 0' | sudo tee -a /etc/fstab", { ignoreError: false } ); return { ok: true, totalMB: mem.totalMB + 4096, swapCreated: true }; } catch (err) { + // Attempt cleanup of partial state + try { + runCapture("sudo swapoff /swapfile 2>/dev/null || true", { ignoreError: true }); + runCapture("sudo rm -f /swapfile", { ignoreError: true }); + } catch { + // Best effort cleanup + } return { ok: false, reason: `swap creation failed: ${err.message}. Create swap manually:\n` +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/preflight.js` around lines 228 - 245, The try/catch that creates swap via runCapture may leave a partial /swapfile if a later step fails; modify the catch in the swap-creation block to attempt cleanup before returning: call runCapture to sudo swapoff /swapfile (ignore failures), remove any /swapfile entry from /etc/fstab (e.g. via sudo sed -i to delete lines containing '/swapfile', ignore failures), and remove the file with sudo rm -f /swapfile (ignore failures), then return the existing error response; reference the swap creation sequence using runCapture and the catch block surrounding runCapture("sudo dd ..."), runCapture("sudo mkswap /swapfile", ...) etc., to implement the cleanup steps.uninstall.sh (1)
409-411: Sed pattern may miss fstab entries with variant whitespace.The pattern
/swapfile none swap sw 0 0requires exact spacing. If/etc/fstabwas manually edited or if future code changes the format (e.g., tabs instead of spaces), this cleanup would silently leave the entry behind.Consider a more permissive pattern:
🔧 Proposed fix for more robust fstab cleanup
if grep -q '/swapfile' /etc/fstab 2>/dev/null; then - sudo sed -i '\|/swapfile none swap sw 0 0|d' /etc/fstab + sudo sed -i '\|^/swapfile[[:space:]]|d' /etc/fstab info "Removed /swapfile entry from /etc/fstab" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@uninstall.sh` around lines 409 - 411, The sed removal currently uses an exact string match ('/swapfile none swap sw 0 0') which will miss lines with tabs or extra/missing spaces; update the sed invocation that targets /etc/fstab (the current sed -i '\|/swapfile none swap sw 0 0|d' call referencing '/swapfile') to use a whitespace-tolerant regex (e.g., match '/swapfile' followed later by 'swap' and/or allow '\s\+' between fields) or match any line starting with '/swapfile' containing 'swap' so entries with tabs/variant spacing are removed reliably, and keep the info "Removed /swapfile entry from /etc/fstab" message intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/lib/preflight.js`:
- Around line 228-245: The try/catch that creates swap via runCapture may leave
a partial /swapfile if a later step fails; modify the catch in the swap-creation
block to attempt cleanup before returning: call runCapture to sudo swapoff
/swapfile (ignore failures), remove any /swapfile entry from /etc/fstab (e.g.
via sudo sed -i to delete lines containing '/swapfile', ignore failures), and
remove the file with sudo rm -f /swapfile (ignore failures), then return the
existing error response; reference the swap creation sequence using runCapture
and the catch block surrounding runCapture("sudo dd ..."), runCapture("sudo
mkswap /swapfile", ...) etc., to implement the cleanup steps.
In `@uninstall.sh`:
- Around line 409-411: The sed removal currently uses an exact string match
('/swapfile none swap sw 0 0') which will miss lines with tabs or extra/missing
spaces; update the sed invocation that targets /etc/fstab (the current sed -i
'\|/swapfile none swap sw 0 0|d' call referencing '/swapfile') to use a
whitespace-tolerant regex (e.g., match '/swapfile' followed later by 'swap'
and/or allow '\s\+' between fields) or match any line starting with '/swapfile'
containing 'swap' so entries with tabs/variant spacing are removed reliably, and
keep the info "Removed /swapfile entry from /etc/fstab" message intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b4398e22-34c5-44d5-82cf-b9e9db825de3
📒 Files selected for processing (5)
bin/lib/onboard.jsbin/lib/preflight.jsdocs/reference/troubleshooting.mdscripts/setup.shuninstall.sh
✅ Files skipped from review due to trivial changes (1)
- docs/reference/troubleshooting.md
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/lib/onboard.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
bin/lib/preflight.js (2)
239-247: Remove redundantdryRuncheck in cleanup block.The
if (!o.dryRun)check on line 241 is unreachable dead code. This catch block can only be reached whendryRunis false, since line 223 returns early whendryRunis true.♻️ Proposed simplification
} catch (err) { // Attempt cleanup of partial state try { - if (!o.dryRun) { - runCapture("sudo swapoff /swapfile 2>/dev/null || true", { ignoreError: true }); - runCapture("sudo rm -f /swapfile", { ignoreError: true }); - } + runCapture("sudo swapoff /swapfile 2>/dev/null || true", { ignoreError: true }); + runCapture("sudo rm -f /swapfile", { ignoreError: true }); } catch { // Best effort cleanup }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/preflight.js` around lines 239 - 247, The cleanup try/catch contains an unnecessary conditional around o.dryRun because the function returns earlier when o.dryRun is true; remove the redundant "if (!o.dryRun)" guard and have the try block directly call runCapture("sudo swapoff /swapfile 2>/dev/null || true", { ignoreError: true }) and runCapture("sudo rm -f /swapfile", { ignoreError: true }) so the cleanup is attempted unconditionally in that catch path (keep the surrounding try/catch and the ignoreError options intact); refer to the runCapture calls and the o.dryRun variable to locate and update the code.
182-205: Consider handling inactive swapfile edge case.The check at line 185 verifies
/swapfileexists but doesn't confirm it's actually activated as swap. If someone previously created the file but never ranswapon, or if it was disabled, the system could still be low on virtual memory.This is a minor edge case, and the current behavior (not recreating an existing file) is reasonable to avoid data loss. You could optionally verify activation via
/proc/swaps:♻️ Optional enhancement to verify swap is active
// Check if swap file already exists if (!o.dryRun) { try { fs.accessSync("/swapfile"); + // Optionally verify it's actually in use + const swaps = fs.readFileSync("/proc/swaps", "utf-8"); + if (!swaps.includes("/swapfile")) { + // File exists but isn't active — attempt to activate it + runCapture("sudo swapon /swapfile", { ignoreError: true }); + } return { ok: true, totalMB: mem.totalMB, swapCreated: false, reason: "/swapfile already exists", };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/preflight.js` around lines 182 - 205, The current early-return when fs.accessSync("/swapfile") succeeds does not verify the file is actually active as swap; after detecting the file exists (fs.accessSync) (and in dry-run using o.swapfileExists), read /proc/swaps and check for an entry matching "/swapfile" and only return the "already exists" result if that entry is present (i.e., swap is active); if the file exists but is not listed in /proc/swaps, fall through so the code will enable/create the swap (and in dry-run allow simulating this via a new o.swapfileActive flag), preserving the existing returned shape (ok, totalMB, swapCreated, reason) when appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/lib/preflight.js`:
- Around line 239-247: The cleanup try/catch contains an unnecessary conditional
around o.dryRun because the function returns earlier when o.dryRun is true;
remove the redundant "if (!o.dryRun)" guard and have the try block directly call
runCapture("sudo swapoff /swapfile 2>/dev/null || true", { ignoreError: true })
and runCapture("sudo rm -f /swapfile", { ignoreError: true }) so the cleanup is
attempted unconditionally in that catch path (keep the surrounding try/catch and
the ignoreError options intact); refer to the runCapture calls and the o.dryRun
variable to locate and update the code.
- Around line 182-205: The current early-return when fs.accessSync("/swapfile")
succeeds does not verify the file is actually active as swap; after detecting
the file exists (fs.accessSync) (and in dry-run using o.swapfileExists), read
/proc/swaps and check for an entry matching "/swapfile" and only return the
"already exists" result if that entry is present (i.e., swap is active); if the
file exists but is not listed in /proc/swaps, fall through so the code will
enable/create the swap (and in dry-run allow simulating this via a new
o.swapfileActive flag), preserving the existing returned shape (ok, totalMB,
swapCreated, reason) when appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6e4b3a53-b82e-4b2f-97a1-a2633abf691f
📒 Files selected for processing (2)
bin/lib/preflight.jsuninstall.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- uninstall.sh
vl43den
force-pushed
the
fix/swap-preflight
branch
from
March 19, 2026 21:30
06233c7 to
f4a6a07
Compare
Contributor
|
Thanks for working on a real issue here. The low-memory OOM problem is valid, and the 12 GB total-memory check is directionally reasonable, but I’m not comfortable merging this as written. The main problem is that the PR expands the fix into automatic host swap lifecycle management in ways that are too risky for product behavior.
I’d be happy to re-review if you narrow this so the OOM mitigation is preserved without silent host-state mutation. Concretely, the next revision should:
The underlying issue is worth fixing. This just needs a tighter product boundary around host mutation before it can merge. |
Contributor
Author
|
Thank you very much for the review! In order to ensure a more robust solution I have:
I documented the main anchors of the process by spinning up another droplet and installing the adjusted version, I'm eager to help tighten this more with your help, if needed, so this can reach production grade: Changed prompt requiring y: Swap space being allocated after prompting y: Situation after ending deployment: 
Uninstalling deployment with 
Testing behavior with a manually created marker: 
|
prekshivyas
reviewed
Mar 26, 2026
Collaborator
There was a problem hiding this comment.
@kjw3's. three concerns from the previous review look addressed:
- Interactive consent now defaults to
[y/N]with explicitstartsWith("y")— non-interactive mode skips swap creation entirely. scripts/setup.shonly warns, never auto-creates swap. Correct.- Uninstall checks
~/.nemoclaw/managed_swapmarker before touching/swapfile.
Needs a rebase — docs/reference/troubleshooting.md conflicts with #911 (merged last week, added reconnect-after-reboot section under Runtime).
One minor nit: bin/lib/preflight.js ~L230 loads require("path") and require("os") inside ensureSwap() — move them to the top of the file with the other requires.
@vl43den
… build On DigitalOcean Ubuntu 24.04 droplets (and similar VMs) with 8 GB RAM and no swap, the sandbox image push during onboarding gets OOM-killed (exit 137). Changes: - Add getMemoryInfo() and ensureSwap() to bin/lib/preflight.js - Integrate memory/swap check into onboard preflight step (onboard.js) - Add equivalent swap check to scripts/setup.sh - Add unit tests for getMemoryInfo and ensureSwap - Document OOM troubleshooting in docs/reference/troubleshooting.md The preflight now detects low memory (<6 GB RAM+swap) and auto-creates a 4 GB swap file via sudo. If swap creation fails, it warns the user with manual steps instead of hard-failing.
prevent misleading "Memory OK" logs when RAM is low but an existing swapfile is safely providing coverage Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
align the MIN_TOTAL_MB constant with the 12000MB threshold enforced in the JS preflight checks Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…nd proper uninstaller cleanup
vl43den
force-pushed
the
fix/swap-preflight
branch
from
March 26, 2026 18:33
a75107c to
df9f073
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
uninstall.sh (1)
494-526: Well-structured ownership verification before swap cleanup.The function correctly implements the ownership proof pattern requested in PR feedback:
- Verifies
/swapfileexists before attempting cleanup- Validates the marker file at
$NEMOCLAW_STATE_DIR/managed_swapexists and contains/swapfile- Skips cleanup in non-interactive mode (consistent with
remove_file_with_optional_sudo)- Uses
|| trueafterswapofffor graceful handling if already deactivatedThe sed pattern
\|^/swapfile[[:space:]]|dcorrectly matches the fstab entry format/swapfile none swap sw 0 0written bypreflight.js.One minor robustness consideration: after
sudo rm -f /swapfileon line 519, there's no verification the removal succeeded. Ifrmfails (e.g., immutable attribute set), the function reports success but the file remains.Optional: Add verification after removal
sudo swapoff /swapfile 2>/dev/null || true - sudo rm -f /swapfile + if ! sudo rm -f /swapfile; then + warn "Failed to remove /swapfile" + return 1 + fi # Clean fstab entry🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@uninstall.sh` around lines 494 - 526, The remove_nemoclaw_swap function currently deletes /swapfile with sudo rm -f but doesn't verify the removal; add a post-removal check immediately after the sudo rm -f in remove_nemoclaw_swap to test whether /swapfile still exists (e.g., [ -f /swapfile ]), and if it does, log an error or warn via warn/processLogger and return a non-zero status (or set a failure flag) so the caller knows cleanup failed; if the file is gone, continue to clean fstab and report success as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/preflight.test.js`:
- Around line 207-214: The test currently relies on memoryInfo: null which
causes ensureSwap to call getMemoryInfo and makes the test
environment-dependent; instead, mock or stub getMemoryInfo to deterministically
simulate "unavailable" memory info and then call ensureSwap with memoryInfo
explicitly set to null (or left undefined) to force the error branch.
Specifically, in test/preflight.test.js replace the current case with a stub of
the getMemoryInfo function (e.g., using sinon.stub or your test framework's
mocking) so getMemoryInfo returns null or throws, call ensureSwap(...) (the
symbol ensureSwap) with platform "linux" and memoryInfo null, assert result.ok
is false and reason matches /could not read memory info/, and finally restore
the stubbed getMemoryInfo to avoid affecting other tests.
---
Nitpick comments:
In `@uninstall.sh`:
- Around line 494-526: The remove_nemoclaw_swap function currently deletes
/swapfile with sudo rm -f but doesn't verify the removal; add a post-removal
check immediately after the sudo rm -f in remove_nemoclaw_swap to test whether
/swapfile still exists (e.g., [ -f /swapfile ]), and if it does, log an error or
warn via warn/processLogger and return a non-zero status (or set a failure flag)
so the caller knows cleanup failed; if the file is gone, continue to clean fstab
and report success as before.
🪄 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
Run ID: afe3dfaf-ef85-4a89-a392-46585b36662a
📒 Files selected for processing (6)
bin/lib/onboard.jsbin/lib/preflight.jsdocs/reference/troubleshooting.mdscripts/setup.shtest/preflight.test.jsuninstall.sh
✅ Files skipped from review due to trivial changes (1)
- docs/reference/troubleshooting.md
🚧 Files skipped from review as they are similar to previous changes (2)
- bin/lib/onboard.js
- bin/lib/preflight.js
vl43den
force-pushed
the
fix/swap-preflight
branch
from
March 26, 2026 18:46
b880a0f to
1cf4142
Compare
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/preflight.test.js (1)
207-221:⚠️ Potential issue | 🟠 MajorMake the unavailable-memory test deterministic (current stub likely doesn’t affect
ensureSwap).At Line 210, mutating
preflight.getMemoryInfomay not intercept the lexicalgetMemoryInfo(...)call insideensureSwap, so this can still depend on host/proc/meminfoand become flaky.Proposed deterministic fix
+import { assert, describe, expect, it, vi } from "vitest"; ... it("returns error when memory info is unavailable", () => { - const preflight = require("../bin/lib/preflight"); - const originalGetMemoryInfo = preflight.getMemoryInfo; - preflight.getMemoryInfo = () => null; + const fs = require("node:fs"); + const readSpy = vi.spyOn(fs, "readFileSync").mockImplementation((path, ...args) => { + if (path === "/proc/meminfo") { + throw new Error("EACCES"); + } + return ""; + }); try { - const result = preflight.ensureSwap(6144, { + const result = ensureSwap(6144, { platform: "linux", - memoryInfo: null, }); assert.equal(result.ok, false); assert.match(result.reason, /could not read memory info/); } finally { - preflight.getMemoryInfo = originalGetMemoryInfo; + readSpy.mockRestore(); } });#!/bin/bash # Verify whether ensureSwap calls lexical getMemoryInfo (not exported property). rg -n -C3 'function ensureSwap|const mem = o\.memoryInfo \|\| getMemoryInfo\(' bin/lib/preflight.jsExpected result:
ensureSwapcontainsgetMemoryInfo({ platform })directly, confirming export-object reassignment in the test is not a reliable hook point.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/preflight.test.js` around lines 207 - 221, The test mutates preflight.getMemoryInfo but ensureSwap calls the lexical getMemoryInfo directly, so the stub doesn't take effect and the test is flaky; fix by loading the module with a test-time stub for the internal getMemoryInfo used by ensureSwap (e.g., use proxyquire/rewire to require the module while replacing the internal getMemoryInfo to return null) so ensureSwap sees the deterministic null result; target the ensureSwap function and the module's internal getMemoryInfo when replacing the implementation rather than assigning preflight.getMemoryInfo after require.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/preflight.test.js`:
- Around line 207-221: The test mutates preflight.getMemoryInfo but ensureSwap
calls the lexical getMemoryInfo directly, so the stub doesn't take effect and
the test is flaky; fix by loading the module with a test-time stub for the
internal getMemoryInfo used by ensureSwap (e.g., use proxyquire/rewire to
require the module while replacing the internal getMemoryInfo to return null) so
ensureSwap sees the deterministic null result; target the ensureSwap function
and the module's internal getMemoryInfo when replacing the implementation rather
than assigning preflight.getMemoryInfo after require.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46000274-8fbb-4cdd-aab6-b70e1dccf6bc
📒 Files selected for processing (2)
test/preflight.test.jsuninstall.sh
Contributor
Author
|
Thank you so much for the review @prekshivyas, I've adressed all the points so far:
Let me know if we need to rework something or lend anything a finishing touch! |
mafueee
pushed a commit
to mafueee/NemoClaw
that referenced
this pull request
Mar 28, 2026
cv
approved these changes
Mar 30, 2026
Collaborator
|
@vl43den fya The CI failure ( Root cause: The test doesn't pass Fix — add it("reports swap would be created in dry-run mode when below threshold", () => {
const result = ensureSwap(6144, {
platform: "linux",
memoryInfo: { totalRamMB: 4000, totalSwapMB: 0, totalMB: 4000 },
dryRun: true,
+ swapfileExists: false,
});This makes the test deterministic regardless of CI host state — same pattern used by the "skips swap creation when /swapfile already exists" test on line 191. |
ensures test is deterministic regardless of CI host state after proposed fix from review
Contributor
Author
|
Thanks for the quick heads up/root cause analysis @prekshivyas ! |
quanticsoul4772
pushed a commit
to quanticsoul4772/NemoClaw
that referenced
this pull request
Mar 30, 2026
…ing sandbox build (NVIDIA#419) * fix: auto-create swap on low-memory VMs to prevent OOM during sandbox build On DigitalOcean Ubuntu 24.04 droplets (and similar VMs) with 8 GB RAM and no swap, the sandbox image push during onboarding gets OOM-killed (exit 137). Changes: - Add getMemoryInfo() and ensureSwap() to bin/lib/preflight.js - Integrate memory/swap check into onboard preflight step (onboard.js) - Add equivalent swap check to scripts/setup.sh - Add unit tests for getMemoryInfo and ensureSwap - Document OOM troubleshooting in docs/reference/troubleshooting.md The preflight now detects low memory (<6 GB RAM+swap) and auto-creates a 4 GB swap file via sudo. If swap creation fails, it warns the user with manual steps instead of hard-failing. * fix: increase memory threshold for swap creation to 12000MB * Update bin/lib/onboard.js prevent misleading "Memory OK" logs when RAM is low but an existing swapfile is safely providing coverage Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update scripts/setup.sh align the MIN_TOTAL_MB constant with the 12000MB threshold enforced in the JS preflight checks Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: harden swap creation with size checks, interactive prompt, dd, and proper uninstaller cleanup * fix: address CodeRabbit feedback for swap cleanup and inactive checks * fix(preflight): address swap management feedback * fix(preflight): fix swap cleanup ordering and clean up comments * fix(preflight): move require('path') and require('os') to top-level imports * fix: address CodeRabbit feedback on preflight tests and uninstaller verification * fix(preflight): harden swap check to handle reactivation of orphaned swap files * fix: add swapfileExists false to the test options ensures test is deterministic regardless of CI host state after proposed fix from review --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
laitingsheng
pushed a commit
that referenced
this pull request
Apr 2, 2026
…ing sandbox build (#419) * fix: auto-create swap on low-memory VMs to prevent OOM during sandbox build On DigitalOcean Ubuntu 24.04 droplets (and similar VMs) with 8 GB RAM and no swap, the sandbox image push during onboarding gets OOM-killed (exit 137). Changes: - Add getMemoryInfo() and ensureSwap() to bin/lib/preflight.js - Integrate memory/swap check into onboard preflight step (onboard.js) - Add equivalent swap check to scripts/setup.sh - Add unit tests for getMemoryInfo and ensureSwap - Document OOM troubleshooting in docs/reference/troubleshooting.md The preflight now detects low memory (<6 GB RAM+swap) and auto-creates a 4 GB swap file via sudo. If swap creation fails, it warns the user with manual steps instead of hard-failing. * fix: increase memory threshold for swap creation to 12000MB * Update bin/lib/onboard.js prevent misleading "Memory OK" logs when RAM is low but an existing swapfile is safely providing coverage Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update scripts/setup.sh align the MIN_TOTAL_MB constant with the 12000MB threshold enforced in the JS preflight checks Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: harden swap creation with size checks, interactive prompt, dd, and proper uninstaller cleanup * fix: address CodeRabbit feedback for swap cleanup and inactive checks * fix(preflight): address swap management feedback * fix(preflight): fix swap cleanup ordering and clean up comments * fix(preflight): move require('path') and require('os') to top-level imports * fix: address CodeRabbit feedback on preflight tests and uninstaller verification * fix(preflight): harden swap check to handle reactivation of orphaned swap files * fix: add swapfileExists false to the test options ensures test is deterministic regardless of CI host state after proposed fix from review --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
lakamsani
pushed a commit
to lakamsani/NemoClaw
that referenced
this pull request
Apr 4, 2026
…ing sandbox build (NVIDIA#419) * fix: auto-create swap on low-memory VMs to prevent OOM during sandbox build On DigitalOcean Ubuntu 24.04 droplets (and similar VMs) with 8 GB RAM and no swap, the sandbox image push during onboarding gets OOM-killed (exit 137). Changes: - Add getMemoryInfo() and ensureSwap() to bin/lib/preflight.js - Integrate memory/swap check into onboard preflight step (onboard.js) - Add equivalent swap check to scripts/setup.sh - Add unit tests for getMemoryInfo and ensureSwap - Document OOM troubleshooting in docs/reference/troubleshooting.md The preflight now detects low memory (<6 GB RAM+swap) and auto-creates a 4 GB swap file via sudo. If swap creation fails, it warns the user with manual steps instead of hard-failing. * fix: increase memory threshold for swap creation to 12000MB * Update bin/lib/onboard.js prevent misleading "Memory OK" logs when RAM is low but an existing swapfile is safely providing coverage Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update scripts/setup.sh align the MIN_TOTAL_MB constant with the 12000MB threshold enforced in the JS preflight checks Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: harden swap creation with size checks, interactive prompt, dd, and proper uninstaller cleanup * fix: address CodeRabbit feedback for swap cleanup and inactive checks * fix(preflight): address swap management feedback * fix(preflight): fix swap cleanup ordering and clean up comments * fix(preflight): move require('path') and require('os') to top-level imports * fix: address CodeRabbit feedback on preflight tests and uninstaller verification * fix(preflight): harden swap check to handle reactivation of orphaned swap files * fix: add swapfileExists false to the test options ensures test is deterministic regardless of CI host state after proposed fix from review --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
gemini2026
pushed a commit
to gemini2026/NemoClaw
that referenced
this pull request
Apr 14, 2026
…ing sandbox build (NVIDIA#419) * fix: auto-create swap on low-memory VMs to prevent OOM during sandbox build On DigitalOcean Ubuntu 24.04 droplets (and similar VMs) with 8 GB RAM and no swap, the sandbox image push during onboarding gets OOM-killed (exit 137). Changes: - Add getMemoryInfo() and ensureSwap() to bin/lib/preflight.js - Integrate memory/swap check into onboard preflight step (onboard.js) - Add equivalent swap check to scripts/setup.sh - Add unit tests for getMemoryInfo and ensureSwap - Document OOM troubleshooting in docs/reference/troubleshooting.md The preflight now detects low memory (<6 GB RAM+swap) and auto-creates a 4 GB swap file via sudo. If swap creation fails, it warns the user with manual steps instead of hard-failing. * fix: increase memory threshold for swap creation to 12000MB * Update bin/lib/onboard.js prevent misleading "Memory OK" logs when RAM is low but an existing swapfile is safely providing coverage Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update scripts/setup.sh align the MIN_TOTAL_MB constant with the 12000MB threshold enforced in the JS preflight checks Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: harden swap creation with size checks, interactive prompt, dd, and proper uninstaller cleanup * fix: address CodeRabbit feedback for swap cleanup and inactive checks * fix(preflight): address swap management feedback * fix(preflight): fix swap cleanup ordering and clean up comments * fix(preflight): move require('path') and require('os') to top-level imports * fix: address CodeRabbit feedback on preflight tests and uninstaller verification * fix(preflight): harden swap check to handle reactivation of orphaned swap files * fix: add swapfileExists false to the test options ensures test is deterministic regardless of CI host state after proposed fix from review --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
6 tasks
wscurran
added
area: install
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR addresses Out-of-Memory (OOM) crashes encountered during sandbox creation on VMs with 8GB RAM (such as my tested instance on Ubuntu 24.04 droplets).
Previously, the preflight memory check only triggered swap space creation if the total memory was below 6GB. However, the OpenShell sandbox creation process realistically requires more than 8GB total virtual memory (RAM + swap) to handle image compression and buffering without being OOM killed.
This change raises the automatic swap creation threshold from
6000MBto12000MB. Now, systems with exactly 8GB RAM will correctly trigger the preflight check to naturally provision a 4GB swap space, ensuring the sandbox build survives without returningExit 137.Fixes
Production Hardening (Added in latest commits)
[Y/n]confirmation prompt before running any sudo swap commands./proc/swapsbefore short-circuiting.fallocatetoddfor broader filesystem compatibility (e.g., XFS/btrfs)./swapfileunmount and robustfstabremoval to uninstall.sh, along with best-effort cleanup on partial creation failure.Environment Tested
Verification Results
nemoclaw onboardconsistently failed during "Pushing image" withCommand failed (exit 137).free -hshowed 0B swap.Note: In the latest commits an interactive
[Y/n]prompt was added for safety. Verified the interactive prompt and swap allocation flow on a fresh 8GB droplet.After prompting "Y" Swap was checked:
Swap Space Usage [check at Step 22/22]:
Success step, proceeding to next step:
Summary by CodeRabbit
New Features
Documentation
Tests
Chores