Skip to content

fix: explicitly exit process on successful command completion#8032

Open
tobq wants to merge 1 commit intonetlify:mainfrom
tobq:fix/process-exit-on-success
Open

fix: explicitly exit process on successful command completion#8032
tobq wants to merge 1 commit intonetlify:mainfrom
tobq:fix/process-exit-on-success

Conversation

@tobq
Copy link

@tobq tobq commented Mar 13, 2026

Summary

Fixes #8031

On success, onEnd() relied on the Node.js event loop draining naturally to exit the process. This fails when stdio pipes are closed by the parent process (e.g. CI runners, editor integrations, or tools that spawn the CLI with piped stdio and then close the connection).

On Windows specifically, the orphaned process never exits and continuously leaks OS handles, eventually accumulating 400,000+ handles per process. This exhausts the Windows kernel nonpaged pool (~3.4 GB vs normal ~500 MB), causing full system freezes including mouse/keyboard input.

Root cause

onEnd() calls exit(1) on error but does nothing on success — it just returns, expecting the event loop to drain. However:

  1. Node.js 19+ default HTTP agents use keepAlive: true, keeping sockets in the pool
  2. When parent processes close stdio pipes (normal behavior for CI, editors, etc.), the CLI process becomes orphaned on Windows
  3. The orphaned process can't exit because it has active handles (keepAlive sockets, broken pipe references)
  4. Handles accumulate indefinitely until the system runs out of kernel resources

Changes

  • src/commands/base-command.ts: Add exit() call at the end of onEnd() for the success path (the error path already had exit(1))
  • bin/run.js: await the onEnd() call in both success and error paths so telemetry tracking completes before exit

Reproduction

The issue can be reproduced by spawning the CLI with piped stdio, then closing the pipes:

const child = spawn('node', ['bin/run.js', 'api', 'listSiteDeploys', '--data', '...'], { stdio: 'pipe' });
child.stdout.on('data', () => {});
// After API response, close pipes (simulating parent abandonment)
setTimeout(() => {
  child.stdout.destroy();
  child.stderr.destroy();
  child.stdin.destroy();
  child.unref();
  // Without fix: process stays alive indefinitely, leaking handles
  // With fix: process exits cleanly via process.exit(0)
}, 1500);

Before fix: Process stays alive indefinitely after pipe closure, accumulating handles
After fix: Process exits cleanly via process.exit(0) before pipes are even abandoned

On success, onEnd() relied on the Node.js event loop draining naturally
to exit the process. This fails when stdio pipes are closed by the parent
(e.g. CI runners, editor integrations, or tools that spawn the CLI with
piped output and then close the connection). On Windows in particular,
the orphaned process never exits and continuously leaks OS handles,
eventually exhausting the kernel nonpaged pool and freezing the system.

The fix:
- Add exit() call in onEnd() for the success path (error path already
  had exit(1))
- Await onEnd() in bin/run.js so telemetry completes before exit

Fixes netlify#8031
@tobq tobq requested a review from a team as a code owner March 13, 2026 02:45
@coderabbitai
Copy link

coderabbitai bot commented Mar 13, 2026

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes
    • Improved error propagation and shutdown reliability by ensuring asynchronous cleanup operations complete before application exit.

Walkthrough

The pull request modifies the process lifecycle handling to ensure proper termination and resource cleanup. In bin/run.js, two synchronous calls to program.onEnd() are converted to awaited calls to ensure the method resolves before proceeding. In src/commands/base-command.ts, an unconditional exit() call is added after telemetry completion, triggering process termination on both success and error paths rather than only on error conditions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding explicit process exit on successful command completion, which is the core fix for the issue.
Description check ✅ Passed The description is well-detailed and directly relates to the changeset, explaining the root cause, changes made, and reproduction steps.
Linked Issues check ✅ Passed The PR successfully addresses issue #8031 by adding process.exit() calls to prevent orphaned processes and handle leaks on Windows as required.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the handle leak issue: awaiting onEnd() calls and adding exit() on success path.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

CodeRabbit can enforce grammar and style rules using `languagetool`.

Configure the reviews.tools.languagetool setting to enable/disable rules and categories. Refer to the LanguageTool Community to learn more.

Copy link

@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.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/commands/base-command.ts (1)

436-441: ⚠️ Potential issue | 🟡 Minor

Use else branch to prevent exit code fallthrough when process.exit is mocked.

The onEnd method at line 414 can fall through to the unconditional exit() at line 441 when process.exit is mocked to return (as in tests). When an error occurs and exit(1) is called at line 438, a mocked exit returns control flow, allowing line 441 to execute exit() with code 0, masking the error status.

🔧 Proposed fix
     if (error_ !== undefined) {
       logError(error_ instanceof Error ? error_ : format(error_))
       exit(1)
     }
-
-    exit()
+    else {
+      exit()
+    }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/commands/base-command.ts` around lines 436 - 441, The onEnd method
currently calls exit(1) when error_ is set but then unconditionally calls exit()
afterwards, which can mask failure if process.exit is mocked; update the control
flow in onEnd (referencing error_, logError(...) and exit(...)) so that the
unconditional exit() is only called in the else branch when error_ is
undefined—i.e., after logging the error and calling exit(1) do not fall through
to the success exit path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/commands/base-command.ts`:
- Around line 436-441: The onEnd method currently calls exit(1) when error_ is
set but then unconditionally calls exit() afterwards, which can mask failure if
process.exit is mocked; update the control flow in onEnd (referencing error_,
logError(...) and exit(...)) so that the unconditional exit() is only called in
the else branch when error_ is undefined—i.e., after logging the error and
calling exit(1) do not fall through to the success exit path.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4af0590c-9fcd-4fc7-838c-a5728cf05d00

📥 Commits

Reviewing files that changed from the base of the PR and between 8dae2c5 and bcb9cf9.

📒 Files selected for processing (2)
  • bin/run.js
  • src/commands/base-command.ts

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.

netlify api listSiteDeploys leaks hundreds of thousands of OS handles on Windows, eventually freezing the system

1 participant