Skip to content

fix: prevent file descriptor leak in acquireOnboardLock#1052

Merged
cv merged 1 commit intoNVIDIA:mainfrom
vl43den:fix-acquireOnboardLock-leak
Mar 30, 2026
Merged

fix: prevent file descriptor leak in acquireOnboardLock#1052
cv merged 1 commit intoNVIDIA:mainfrom
vl43den:fix-acquireOnboardLock-leak

Conversation

@vl43den
Copy link
Copy Markdown
Contributor

@vl43den vl43den commented Mar 28, 2026

Summary

Replace manual fd lifecycle (openSync/writeFileSync/closeSync) in acquireOnboardLock with a single writeFileSync call. If the write threw (e.g. ENOSPC), the catch block re-threw because the error code is not EEXIST, bypassing closeSync and leaking the descriptor. Repeated failures accumulate leaked fds until the process crashes with EMFILE.

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

  • Removed manual fs.openSync / fs.closeSync fd management in bin/lib/onboard-session.js
  • Replaced with fs.writeFileSync(LOCK_FILE, payload, { flag: "wx", mode: 0o600 }) which lets Node.js manage the fd internally, guaranteeing cleanup regardless of write outcome

Type of Change

  • Code change for a new feature, bug fix, or refactor.
  • Code change with doc updates.
  • Doc only. Prose changes without code sample modifications.
  • Doc only. Includes code sample changes.

Testing

  • npx prek run --all-files passes (or equivalently make check).
  • npm test passes.
  • make docs builds without warnings. (for doc-only changes)

Checklist

General

Code Changes

  • Formatters applied — npx prek run --all-files auto-fixes formatting (or make format for targeted runs).
  • Tests added or updated for new or changed behavior.
  • No secrets, API keys, or credentials committed.
  • Doc pages updated for any user-facing behavior changes (new commands, changed defaults, new features, bug fixes that contradict existing docs).

Doc Changes

  • Follows the style guide. Try running the update-docs agent skill to draft changes while complying with the style guide. For example, prompt your agent with "/update-docs catch up the docs for the new changes I made in this PR."
  • New pages include SPDX license header and frontmatter, if creating a new page.
  • Cross-references and links verified.

Summary by CodeRabbit

Release Notes

  • Chores
    • Optimized internal lock file handling for improved code efficiency.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 28, 2026

📝 Walkthrough

Walkthrough

The acquireOnboardLock function in the onboarding session module is refactored to simplify lock file writing. Instead of separately opening a file descriptor, writing through it, and closing it, the code now writes the lock payload directly using a single synchronous file write operation with atomic flags.

Changes

Cohort / File(s) Summary
Lock Acquisition Simplification
bin/lib/onboard-session.js
Streamlined lock file creation in acquireOnboardLock by replacing the three-step file descriptor approach (open, write, close) with a direct synchronous write call, maintaining atomic file creation semantics and file permissions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A lock writes swift and clean,
No descriptors in between,
One call does what three once made,
Simplicity—a code upgrade! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title 'fix: prevent file descriptor leak in acquireOnboardLock' directly and specifically describes the main change — replacing manual file descriptor management to prevent leaks when write operations fail.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ 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)
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 mocks fs.writeFileSync to 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

📥 Commits

Reviewing files that changed from the base of the PR and between eb4ba8c and f454f6f.

📒 Files selected for processing (1)
  • bin/lib/onboard-session.js

@cv cv merged commit 13461f8 into NVIDIA:main Mar 30, 2026
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants