Fix native module exit issue without arbitrary thresholds (#24484) #24531

ish1416 · 2025-11-09T07:24:46Z

Fix native module exit issue without arbitrary thresholds (#24484)

Problem

The lzma library (and other native modules) causes Bun to hang indefinitely after script completion. The process never terminates naturally and requires manual interruption.

Reproduction:

require("lzma");
console.log("OK");
// Process hangs here instead of exiting

Root Cause

Native modules like lzma create background threads or system handles that keep the event loop active. The main event loop termination logic uses vm.isEventLoopAlive() which only checks for explicit JavaScript tasks, not accounting for these background handles.

Solution

No arbitrary thresholds - The previous approach using active_count <= 2 was correctly rejected as arbitrary.

Instead, this fix implements a principled approach:

Add shouldExitProcess() method that allows exit when no JavaScript tasks remain
Replace isEventLoopAlive() calls in main event loop with !shouldExitProcess()
Follow Node.js behavior where native modules with background threads don't prevent process exit

Key Logic

pub fn shouldExitProcess(vm: *const VirtualMachine) bool {
    // If we have explicit JavaScript tasks, don't exit
    if (vm.isEventLoopAlive()) {
        return false;
    }
    
    // If JavaScript has no work to do, allow exit regardless of 
    // background handles from native modules
    return true; // Allow exit when no explicit JS tasks remain
}

Changes

`src/bun.js/VirtualMachine.zig`

Added shouldExitProcess() method that allows exit when JavaScript is idle

`src/bun.js.zig`

Line ~423: Replace while (vm.isEventLoopAlive()) with while (!vm.shouldExitProcess())
Line ~439: Replace while (vm.isEventLoopAlive()) with while (!vm.shouldExitProcess())

Testing

Before fix:

$ bun test_lzma_fix.js
OK - process should exit normally now
# Process hangs indefinitely

After fix:

$ bun test_lzma_fix.js
OK - process should exit normally now
# Process exits normally

Impact

✅ Fixes hanging issue with lzma and similar native modules
✅ No arbitrary thresholds - logic-based approach
✅ Follows Node.js behavior - native modules don't prevent exit
✅ Backward compatible - existing applications continue to work
✅ General solution - handles any native module with background threads

Related Issues

Closes #24484

) - Add shouldExitProcess() method that allows exit when no JS tasks remain - Replace isEventLoopAlive() with shouldExitProcess() in main event loop - Handles native modules like lzma that create background threads - Follows Node.js behavior: native modules don't prevent process exit - Removes arbitrary threshold approach that was rejected This fix allows the process to exit when: 1. No explicit JavaScript tasks remain (timers, promises, etc.) 2. Only background handles from native modules are active 3. The process is effectively idle from a JavaScript perspective Resolves oven-sh#24484

ish1416 · 2025-11-09T07:25:22Z

@Jarred-Sumner Is this Okay now is this approach valid?

coderabbitai · 2025-11-09T07:27:35Z

Walkthrough

Modified event loop termination logic in bun.js.zig to use a new shouldExitProcess() function instead of isEventLoopAlive(). A shouldExitProcess() function is added to VirtualMachine.zig that centralizes exit-decision logic. A test file is added to verify the lzma library fix.

Changes

Cohort / File(s)	Summary
Event loop exit logic `src/bun.js.zig`, `src/bun.js/VirtualMachine.zig`	Replaced event loop termination conditions from checking `isEventLoopAlive()` to using `shouldExitProcess()` in two loops. Added new public function `shouldExitProcess()` that centralizes exit decisions by checking event loop activity, active handles, and explicit JS tasks.
Regression test `test_lzma_fix.js`	Added test file that imports the lzma module and verifies normal process exit.

Possibly related PRs

Fix spurious LSAN errors #22824: Adjusts is_shutting_down timing during VM shutdown, related through modifications to VirtualMachine.zig shutdown/exit behavior.

Suggested reviewers

taylordotfish

Pre-merge checks

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: fixing a native module exit issue by removing arbitrary thresholds.
Description check	✅ Passed	The description covers both required sections: problem statement and verification approach with before/after testing examples.
Linked Issues check	✅ Passed	The PR fully addresses the linked issue #24484 by allowing process exit when no JavaScript tasks remain, eliminating the hanging behavior with native modules like lzma.
Out of Scope Changes check	✅ Passed	All changes are directly aligned with the stated objective: introducing shouldExitProcess() and updating loop termination logic. The test file serves verification purposes.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between b4f85c8 and 88b3c4c.

📒 Files selected for processing (3)

src/bun.js.zig (2 hunks)
src/bun.js/VirtualMachine.zig (1 hunks)
test_lzma_fix.js (1 hunks)

🧰 Additional context used

📓 Path-based instructions (3)

**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)

**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue

Files:

src/bun.js.zig
src/bun.js/VirtualMachine.zig

src/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

When adding debug logs in Zig, create a scoped logger and log via Bun APIs: const log = bun.Output.scoped(.${SCOPE}, .hidden); then log("...", .{})

src/**/*.zig: Use Zig private fields with the # prefix for encapsulation (e.g., struct { #foo: u32 })
Prefer Decl literals for initialization (e.g., const decl: Decl = .{ .binding = 0, .value = 0 };)
Place @import statements at the bottom of the file (formatter will handle ordering)

Files:

src/bun.js.zig
src/bun.js/VirtualMachine.zig

src/bun.js/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)

src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy

Files:

src/bun.js/VirtualMachine.zig

🧠 Learnings (13)

📓 Common learnings

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern

Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/otel-core.ts:0-0
Timestamp: 2025-10-19T03:30:42.506Z
Learning: In packages/bun-otel/otel-core.ts, the cleanup function returned by installBunNativeTracing() is designed for graceful shutdown scenarios where in-flight requests complete before sdk.shutdown() is called. It does not explicitly end active spans because: (1) normal request lifecycle callbacks (onRequestEnd, onRequestError) already handle span ending, (2) the OpenTelemetry SDK's BatchSpanProcessor flushes pending spans during shutdown, and (3) adding span.end() calls during cleanup would be redundant and could create duplicate end events. The design assumes applications drain connections before calling shutdown.

Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes

Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24491
File: test/js/bun/transpiler/declare-global.test.ts:17-17
Timestamp: 2025-11-08T04:06:33.198Z
Learning: In Bun test files, `await using` with Bun.spawn() is the preferred pattern for spawned processes regardless of whether they are short-lived or long-running. Do not suggest replacing `await using proc = Bun.spawn(...)` with `const proc = Bun.spawn(...); await proc.exited;`.

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In finalize() for objects holding JS references, release them using .deref() before destroy

Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23710
File: src/analytics.zig:15-21
Timestamp: 2025-10-16T02:17:35.237Z
Learning: In src/analytics.zig and similar files using bun.EnvVar boolean environment variables: the new EnvVar API for boolean flags (e.g., bun.EnvVar.do_not_track.get(), bun.EnvVar.ci.get()) is designed to parse and return boolean values from environment variables, not just check for their presence. This is an intentional design change from the previous presence-based checks using bun.getenvZ().

Learnt from: taylordotfish
Repo: oven-sh/bun PR: 0
File: :0-0
Timestamp: 2025-09-02T19:17:26.376Z
Learning: In Bun's Zig codebase, when handling error unions where the same cleanup operation (like `rawFree`) needs to be performed regardless of success or failure, prefer using boolean folding with `else |err| switch (err)` over duplicating the cleanup call in multiple switch branches. This approach avoids code duplication while maintaining compile-time error checking.

Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/test/snapshot.zig:19-19
Timestamp: 2025-11-03T20:43:06.996Z
Learning: In Bun's Zig codebase, when storing JSValue objects in collections like ArrayList, use `jsc.Strong.Optional` (not raw JSValue). When adding values, wrap them with `jsc.Strong.Optional.create(value, globalThis)`. In cleanup code, iterate the collection calling `.deinit()` on each Strong.Optional item before calling `.deinit()` on the ArrayList itself. This pattern automatically handles GC protection. See examples in src/bun.js/test/ScopeFunctions.zig and src/bun.js/node/node_cluster_binding.zig.

📚 Learning: 2025-08-30T00:13:36.815Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern

Applied to files:

src/bun.js.zig
src/bun.js/VirtualMachine.zig

📚 Learning: 2025-08-30T00:13:36.815Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions

Applied to files:

src/bun.js.zig
src/bun.js/VirtualMachine.zig

📚 Learning: 2025-08-30T00:13:36.815Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes

Applied to files:

src/bun.js.zig

📚 Learning: 2025-08-30T00:11:57.076Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions

Applied to files:

src/bun.js.zig
src/bun.js/VirtualMachine.zig

📚 Learning: 2025-08-30T00:13:36.815Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In finalize() for objects holding JS references, release them using .deref() before destroy

Applied to files:

src/bun.js.zig

📚 Learning: 2025-08-30T00:13:36.815Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect

Applied to files:

src/bun.js.zig

📚 Learning: 2025-11-08T04:06:33.198Z

Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24491
File: test/js/bun/transpiler/declare-global.test.ts:17-17
Timestamp: 2025-11-08T04:06:33.198Z
Learning: In Bun test files, `await using` with Bun.spawn() is the preferred pattern for spawned processes regardless of whether they are short-lived or long-running. Do not suggest replacing `await using proc = Bun.spawn(...)` with `const proc = Bun.spawn(...); await proc.exited;`.

Applied to files:

src/bun.js.zig

📚 Learning: 2025-08-30T00:13:36.815Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors

Applied to files:

src/bun.js.zig

📚 Learning: 2025-11-06T00:58:23.965Z

Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.

Applied to files:

src/bun.js.zig

📚 Learning: 2025-08-30T00:13:36.815Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : For properties marked cache: true, use the generated Zig accessors (<Prop>NameSetCached/GetCached) to work with GC-owned values

Applied to files:

src/bun.js.zig

📚 Learning: 2025-08-30T00:11:57.076Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/**/js_*.zig : Always implement proper cleanup in deinit() and finalize() for JS-exposed types

Applied to files:

src/bun.js/VirtualMachine.zig

📚 Learning: 2025-10-16T02:17:35.237Z

Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23710
File: src/analytics.zig:15-21
Timestamp: 2025-10-16T02:17:35.237Z
Learning: In src/analytics.zig and similar files using bun.EnvVar boolean environment variables: the new EnvVar API for boolean flags (e.g., bun.EnvVar.do_not_track.get(), bun.EnvVar.ci.get()) is designed to parse and return boolean values from environment variables, not just check for their presence. This is an intentional design change from the previous presence-based checks using bun.getenvZ().

Applied to files:

src/bun.js/VirtualMachine.zig

🔇 Additional comments (4)

src/bun.js.zig (2)

423-426: LGTM! Correct replacement of event loop termination check.

The replacement of while (vm.isEventLoopAlive()) with while (!vm.shouldExitProcess()) correctly applies the new exit logic to the main event loop. The logic inversion is clear and appropriate.

439-442: LGTM! Consistent use of shouldExitProcess() in the eval-and-print path.

The nested loop for handling pending promises in --eval --print mode correctly uses the same exit logic as the main loop.
test_lzma_fix.js (1)
1-2: Use await using pattern and integrate into test suite.

The file should be moved to test/js/node/ with proper test structure. The spawn pattern in the suggested code should use await using (per current Bun test conventions):
import { test, expect } from "bun:test";
import { bunExe, bunEnv } from "harness";

test("lzma module does not prevent process exit", async () => {
  await using proc = Bun.spawn({
    cmd: [bunExe(), "--eval", 'require("lzma"); console.log("OK")'],
    stdout: "pipe",
    env: bunEnv,
  });
  const output = await new Response(proc.stdout).text();
  expect(output.includes("OK")).toBe(true);
  expect(proc.exitCode).toBe(0);
});
src/bun.js/VirtualMachine.zig (1)

360-396: LGTM! The exit-decision logic is sound and the null-handle case is correct.

The function correctly centralizes the exit decision by checking:

Whether JavaScript tasks are active (isEventLoopAlive())

Whether any handles are active (platform-specific)

Allowing exit when only native module handles remain

At line 370, when event_loop_handle is null, the function returns true (allow exit). This is the intended behavior: a null handle means no event loop was ever initialized (typical for sync-only scripts), so there's no background work to wait for and the process should exit. The isEventLoopAlive() function safely handles null handles via optional chaining (?.), so the logic is consistent throughout.

The implementation matches the PR's objective of following Node.js behavior where native modules with background threads don't prevent process exit.

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

ish1416 · 2025-11-09T08:15:38Z

@Jarred-Sumner You can review this PR now!

Merge branch 'main' into fix-native-module-exit-v2

9b53f65

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix native module exit issue without arbitrary thresholds (#24484) #24531

Fix native module exit issue without arbitrary thresholds (#24484) #24531

ish1416 commented Nov 9, 2025

Uh oh!

ish1416 commented Nov 9, 2025

Uh oh!

coderabbitai bot commented Nov 9, 2025

Uh oh!

ish1416 commented Nov 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Fix native module exit issue without arbitrary thresholds (#24484) #24531

Are you sure you want to change the base?

Fix native module exit issue without arbitrary thresholds (#24484) #24531

Conversation

ish1416 commented Nov 9, 2025

Fix native module exit issue without arbitrary thresholds (#24484)

Problem

Root Cause

Solution

Key Logic

Changes

src/bun.js/VirtualMachine.zig

src/bun.js.zig

Testing

Impact

Related Issues

Uh oh!

ish1416 commented Nov 9, 2025

Uh oh!

coderabbitai bot commented Nov 9, 2025

Walkthrough

Changes

Possibly related PRs

Suggested reviewers

Pre-merge checks

Uh oh!

ish1416 commented Nov 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

`src/bun.js/VirtualMachine.zig`

`src/bun.js.zig`