fix: prevent file descriptor leak in acquireOnboardLock#1052
fix: prevent file descriptor leak in acquireOnboardLock#1052vl43den wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/lib/onboard-session.js (1)
228-270: Consider adding a test for write-failure scenarios.The existing tests (per the provided snippets) use real filesystem operations and don't mock
fs.writeFileSync. This means failure paths like ENOSPC during lock acquisition aren't exercised. While the fd leak fix is correct, a unit test that mocksfs.writeFileSyncto throw ENOSPC would confirm the error propagates correctly without leaking descriptors.This is a pre-existing gap and not a blocker for this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard-session.js` around lines 228 - 270, Add a unit test that simulates write failures by mocking fs.writeFileSync to throw an ENOSPC error and asserting the lock-acquisition code (the logic around LOCK_FILE that calls fs.writeFileSync and uses parseLockFile/isProcessAlive) propagates the error without leaking resources; specifically, mock fs.writeFileSync to throw { code: "ENOSPC" }, invoke the function that attempts to create the lock (the block that writes LOCK_FILE), verify the thrown error bubbles up (or is handled as expected by your API), and ensure no leftover file descriptor or lock file remains after the test (restore the mock and clean up).
🤖 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/onboard-session.js`:
- Around line 228-270: Add a unit test that simulates write failures by mocking
fs.writeFileSync to throw an ENOSPC error and asserting the lock-acquisition
code (the logic around LOCK_FILE that calls fs.writeFileSync and uses
parseLockFile/isProcessAlive) propagates the error without leaking resources;
specifically, mock fs.writeFileSync to throw { code: "ENOSPC" }, invoke the
function that attempts to create the lock (the block that writes LOCK_FILE),
verify the thrown error bubbles up (or is handled as expected by your API), and
ensure no leftover file descriptor or lock file remains after the test (restore
the mock and clean up).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 911f1946-f84c-4bd9-a721-a63682ac5e5e
📒 Files selected for processing (1)
bin/lib/onboard-session.js
Summary
Replace manual fd lifecycle (
openSync/writeFileSync/closeSync) inacquireOnboardLockwith a singlewriteFileSynccall. If the write threw (e.g.ENOSPC), the catch block re-threw because the error code is notEEXIST, bypassingcloseSyncand leaking the descriptor. Repeated failures accumulate leaked fds until the process crashes withEMFILE.Please note that npm test and prek linters currently fail due to pre-existing issues on main (131 unrelated test failures, and pre-existing shellcheck/shebang/Vitest plugin failures on Windows). These changes should introduce zero new failures, and the relevant onboard-session tests pass successfully.
Changes
fs.openSync/fs.closeSyncfd management inbin/lib/onboard-session.jsfs.writeFileSync(LOCK_FILE, payload, { flag: "wx", mode: 0o600 })which lets Node.js manage the fd internally, guaranteeing cleanup regardless of write outcomeType of Change
Testing
npx prek run --all-filespasses (or equivalentlymake check).npm testpasses.make docsbuilds without warnings. (for doc-only changes)Checklist
General
Code Changes
npx prek run --all-filesauto-fixes formatting (ormake formatfor targeted runs).Doc Changes
update-docsagent skill to draft changes while complying with the style guide. For example, prompt your agent with "/update-docscatch up the docs for the new changes I made in this PR."Summary by CodeRabbit
Release Notes