Repository navigation
fix(windows): close worker libuv loop - #24811
Merged
Merged
Conversation
Collaborator
|
Updated 6:03 PM PT - Nov 19th, 2025
❌ @zackradisic, your commit 7b32474 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 24811That installs a local version of the PR into your bun-24811 --bun |
Contributor
WalkthroughCall sequence for libuv loop shutdown was changed: loop shutdown is now a thread-local guarded workflow that walks and closes remaining handles before running and clearing the loop. Web worker exit calls the new shutdown on Windows (Environment.isWindows) prior to GC deinit and thread exit. Changes
Suggested reviewers
Pre-merge checks✅ Passed checks (4 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 1
📜 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.
📒 Files selected for processing (2)
src/bun.js/web_worker.zig(1 hunks)src/deps/libuv.zig(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 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/web_worker.zig
📚 Learning: 2025-10-19T03:30:42.506Z
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.
Applied to files:
src/bun.js/web_worker.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/web_worker.zig
📚 Learning: 2025-09-24T05:48:59.872Z
Learnt from: nektro
Repo: oven-sh/bun PR: 22806
File: scripts/runner.node.mjs:687-689
Timestamp: 2025-09-24T05:48:59.872Z
Learning: In the Bun codebase, the `startGroup` utility function in scripts/runner.node.mjs automatically closes any previously open group when called, so `startGroup("End")` correctly closes the final test group and keeps subsequent output ungrouped.
Applied to files:
src/bun.js/web_worker.zig
📚 Learning: 2025-09-02T05:33:37.517Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22323
File: test/js/web/websocket/websocket-subprotocol.test.ts:74-75
Timestamp: 2025-09-02T05:33:37.517Z
Learning: In Bun's runtime, `await using` with Node.js APIs like `net.createServer()` is properly supported and should not be replaced with explicit cleanup. Bun has extended Node.js APIs with proper async dispose support.
Applied to files:
src/bun.js/web_worker.zig
📚 Learning: 2025-09-02T06:10:17.252Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22323
File: test/js/web/websocket/websocket-subprotocol-strict.test.ts:80-91
Timestamp: 2025-09-02T06:10:17.252Z
Learning: Bun's WebSocket implementation includes a terminate() method, which is available even though it's not part of the standard WebSocket API. This method can be used in Bun test files and applications for immediate WebSocket closure.
Applied to files:
src/bun.js/web_worker.zig
📚 Learning: 2025-10-17T20:50:58.644Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23755
File: src/bun.js/api/bun/socket/Handlers.zig:154-159
Timestamp: 2025-10-17T20:50:58.644Z
Learning: In Bun socket configuration error messages (src/bun.js/api/bun/socket/Handlers.zig), use the user-facing JavaScript names "data" and "drain" instead of internal field names "onData" and "onWritable", as these are the names users see in the API according to SocketConfig.bindv2.ts.
Applied to files:
src/bun.js/web_worker.zig
📚 Learning: 2025-09-12T18:16:50.754Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22606
File: src/glob/GlobWalker.zig:449-452
Timestamp: 2025-09-12T18:16:50.754Z
Learning: For Bun codebase: prefer using `std.fs.path.sep` over manual platform separator detection, and use `bun.strings.lastIndexOfChar` instead of `std.mem.lastIndexOfScalar` for string operations.
Applied to files:
src/bun.js/web_worker.zig
🔇 Additional comments (1)
src/bun.js/web_worker.zig (1)
625-625: Verified: Thread-local libuv loop cleanup on all worker exit paths.All worker thread termination paths have been verified to call
exitAndDeinit(), which is markednoreturnand serves as the single exit point for the thread. The new cleanup call is correctly placed beforebun.exitThread()and follows all other resource cleanup. No additional fixes needed.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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.
📒 Files selected for processing (2)
src/bun.js/web_worker.zig(2 hunks)src/deps/libuv.zig(2 hunks)
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
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.
📚 Learning: 2025-11-03T20:43:06.996Z
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.
Applied to files:
src/bun.js/web_worker.zig
📚 Learning: 2025-10-14T04:04:17.132Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23636
File: src/shell/interpreter.zig:672-0
Timestamp: 2025-10-14T04:04:17.132Z
Learning: In Bun's shell interpreter (src/shell/interpreter.zig), the JavaScript garbage collector's memory reporting API only allows reporting extra memory allocation once at object creation time. Therefore, `estimated_size_for_gc` is intentionally set once during initialization and not updated afterward, even though fields like `root_io`, `vm_args_utf8`, and `export_env` may be mutated later. The `estimatedSize()` method returns this cached initial value, while `memoryCost()` computes the size on-demand.
Applied to files:
src/bun.js/web_worker.zig
📚 Learning: 2025-09-02T05:33:37.517Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22323
File: test/js/web/websocket/websocket-subprotocol.test.ts:74-75
Timestamp: 2025-09-02T05:33:37.517Z
Learning: In Bun's runtime, `await using` with Node.js APIs like `net.createServer()` is properly supported and should not be replaced with explicit cleanup. Bun has extended Node.js APIs with proper async dispose support.
Applied to files:
src/bun.js/web_worker.zig
📚 Learning: 2025-09-20T03:39:41.770Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 22534
File: test/regression/issue/21830.fixture.ts:14-63
Timestamp: 2025-09-20T03:39:41.770Z
Learning: Bun's test runner supports async describe callbacks, unlike Jest/Vitest where describe callbacks must be synchronous. The syntax `describe("name", async () => { ... })` is valid in Bun.
Applied to files:
src/bun.js/web_worker.zig
📚 Learning: 2025-10-04T21:17:53.040Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23253
File: test/js/valkey/valkey.failing-subscriber-no-ipc.ts:40-59
Timestamp: 2025-10-04T21:17:53.040Z
Learning: In Bun runtime, the global `console` object is an AsyncIterable that yields lines from stdin. The pattern `for await (const line of console)` is valid and documented in Bun for reading input line-by-line from the child process stdin.
Applied to files:
src/bun.js/web_worker.zig
📚 Learning: 2025-10-08T13:48:02.430Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23373
File: test/js/bun/tarball/extract.test.ts:107-111
Timestamp: 2025-10-08T13:48:02.430Z
Learning: In Bun's test runner, use `expect(async () => { await ... }).toThrow()` to assert async rejections. Unlike Jest/Vitest, Bun does not require `await expect(...).rejects.toThrow()` - the async function wrapper with `.toThrow()` is the correct pattern for async error assertions in Bun tests.
Applied to files:
src/bun.js/web_worker.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/web_worker.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/web_worker.zig
📚 Learning: 2025-10-26T01:32:04.844Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.
Applied to files:
src/bun.js/web_worker.zig
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
src/bun.js/web_worker.zig
📚 Learning: 2025-10-01T22:13:08.081Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/codegen/bindgenv2/internal/base.ts:120-125
Timestamp: 2025-10-01T22:13:08.081Z
Learning: Iterator.some() and other Iterator helper methods (map, filter, etc.) are supported in Bun and modern Node.js (22+) runtimes and can be safely used on string iterators and other iterator objects.
Applied to files:
src/bun.js/web_worker.zig
📚 Learning: 2025-10-30T03:48:10.513Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 24063
File: packages/bun-otel/test/context-propagation.test.ts:1-7
Timestamp: 2025-10-30T03:48:10.513Z
Learning: In Bun test files, `using` declarations at the describe block level execute during module load/parsing, not during test execution. This means they acquire and dispose resources before any tests run. For test-scoped resource management, use beforeAll/afterAll hooks instead. The pattern `beforeAll(beforeUsingEchoServer); afterAll(afterUsingEchoServer);` is correct for managing ref-counted test resources like the EchoServer in packages/bun-otel/test/ - the using block pattern should not be used at describe-block level for test resources.
<!-- [/add_learning]
Applied to files:
src/bun.js/web_worker.zig
📚 Learning: 2025-09-02T19:17:26.376Z
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.
Applied to files:
src/deps/libuv.zig
📚 Learning: 2025-10-24T10:43:09.398Z
Learnt from: fmguerreiro
Repo: oven-sh/bun PR: 23774
File: src/install/PackageManager/updatePackageJSONAndInstall.zig:548-548
Timestamp: 2025-10-24T10:43:09.398Z
Learning: In Bun's Zig codebase, the `as(usize, intCast(...))` cast pattern triggers a Zig compiler bug that causes compilation to hang indefinitely when used in complex control flow contexts (loops + short-circuit operators + optional unwrapping). Avoid this pattern and use simpler alternatives like just `intCast(...)` if type casting is necessary.
Applied to files:
src/deps/libuv.zig
📚 Learning: 2025-09-02T18:09:21.647Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/allocators/allocation_scope.zig:79-85
Timestamp: 2025-09-02T18:09:21.647Z
Learning: In Bun's Zig codebase, prefer simple, direct error handling patterns. When an error type has only one variant (like FreeError with only NotAllocated), use a switch statement rather than generic catch blocks to leverage compile-time knowledge and avoid unnecessary complexity. Avoid duplicating function calls across error paths when a cleaner structure is possible.
Applied to files:
src/deps/libuv.zig
📚 Learning: 2025-11-03T20:40:59.655Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/bindings/JSValue.zig:545-586
Timestamp: 2025-11-03T20:40:59.655Z
Learning: In Bun's Zig codebase, JSErrors (returned as `bun.JSError!T`) must always be properly handled. Using `catch continue` or `catch { break; }` to silently suppress JSErrors is a bug. Errors should either be explicitly handled or propagated with `try`. This applies to snapshot serializer error handling where Jest's behavior is to throw when serializers throw.
Applied to files:
src/deps/libuv.zig
🔇 Additional comments (4)
src/bun.js/web_worker.zig (3)
614-618: LGTM: Proper sequencing prevents double-close.The early
gc_controller.deinit()beforeLoop.shutdown()correctly prevents doubleuv_closeon the GC timer handle. The comment clearly documents the rationale, and the placement aligns with the past review feedback.
649-649: LGTM: Import required for comptime check.The
Environmentimport is correctly added to support thecomptime Environment.isWindowscheck at line 620.
620-622: Verify whether non-Windows platforms require Loop cleanup.The
Loop.shutdown()call properly handles libuv cleanup (closes handles, closes loop, nullifies threadlocal reference) and is correctly positioned aftergc_controller.deinit(). However, the Windows-specific guard lacks documentation. Since threadlocal loops exist on all platforms and the shutdown logic is generic, verify with the original issue whether macOS and Linux should also callLoop.shutdown()to prevent stale thread-local loops, or if this is a Windows-only libuv issue.src/deps/libuv.zig (1)
2132-2132: LGTM: ReturnCode provides type-safe error handling.Changing
uv_loop_closereturn type fromc_inttoReturnCodeimproves type safety and enables the.errEnum()method used inshutdown(). This is consistent with other libuv function signatures in this file.
Comment on lines
+657
to
663
| fn closeWalkCb(handle: ?*uv_handle_t, data: ?*anyopaque) callconv(.c) void { | ||
| _ = data; | ||
| const h = handle.?; | ||
| if (uv_is_closing(h) == 0) { | ||
| uv_close(h, null); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM: Cleanup callback correctly guards against double-close.
The closeWalkCb properly checks uv_is_closing before calling uv_close, preventing double-close attempts. The null callback is acceptable for cleanup scenarios.
Consider adding debug-only logging to track which handles are being cleaned up, which could aid troubleshooting:
fn closeWalkCb(handle: ?*uv_handle_t, data: ?*anyopaque) callconv(.c) void {
_ = data;
const h = handle.?;
if (uv_is_closing(h) == 0) {
+ if (comptime bun.Environment.isDebug) {
+ log("closeWalkCb: closing handle type {d}", .{h.type});
+ }
uv_close(h, null);
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/deps/libuv.zig around lines 657 to 663, the cleanup callback currently
skips logging; add debug-only logging before calling uv_close to aid
troubleshooting by printing which handle is being closed (e.g., pointer address
and/or handle type) using a comptime debug guard so it’s only emitted in debug
builds (use std.debug.print guarded by @compileError? no — use if (builtin.mode
== .Debug) or a comptime std.build.Mode check appropriate for this repo), ensure
std is imported if needed, and keep the existing uv_is_closing check and null
callback behavior unchanged.
Comment on lines
+665
to
679
| pub fn shutdown() void { | ||
| const loop = threadlocal_loop orelse { | ||
| return; | ||
| }; | ||
|
|
||
| if (uv_loop_close(loop).errEnum()) |err| { | ||
| if (err == .BUSY) { | ||
| uv_walk(loop, &closeWalkCb, null); | ||
| _ = uv_run(loop, .default); | ||
| bun.debugAssert(uv_loop_close(loop) == .zero); | ||
| } | ||
| } | ||
|
|
||
| pub fn delete(ptr: *Loop) void { | ||
| close(ptr); | ||
| bun.default_allocator.destroy(ptr); | ||
| threadlocal_loop = null; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider release-build error handling for final close failure.
The shutdown sequence correctly follows the libuv pattern: close → walk handles → run loop → close again. The debugAssert at line 674 assumes the second close will succeed after cleanup.
In release builds where assertions are disabled, if uv_loop_close unexpectedly fails after cleanup, the error is silently ignored and threadlocal_loop is still cleared. Consider whether release builds should log or handle this case:
if (uv_loop_close(loop).errEnum()) |err| {
if (err == .BUSY) {
uv_walk(loop, &closeWalkCb, null);
_ = uv_run(loop, .default);
if (uv_loop_close(loop).errEnum()) |final_err| {
if (comptime bun.Environment.allow_assert) {
bun.assert(false); // Should not happen
} else {
log("uv_loop_close failed after cleanup: {s}", .{@tagName(final_err)});
}
}
} else {
// Non-BUSY error, log it
log("uv_loop_close failed: {s}", .{@tagName(err)});
}
}🤖 Prompt for AI Agents
In src/deps/libuv.zig around lines 665-679, the shutdown path only uses
debugAssert after the second uv_loop_close call, which means release builds
ignore a potential final close failure; update the logic to (1) handle the
initial uv_loop_close error when it's not BUSY by logging the error, (2) after
walking and running the loop, re-call uv_loop_close and if that still errors
then either assert when assertions are enabled or log the final error in release
mode (use the existing bun assert/log facilities and include the error tagName
for clarity), and (3) ensure threadlocal_loop is only cleared after
handling/logging the final close result.
This was referenced Dec 19, 2025
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
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.
What does this PR do?
We need to call
uv_loop_closein order to remove the threadlocal loop from a list in libuv so it won't be used later. This explains the crash reports because they haveworkers_terminatedin features.Fixes #24804
Closes BUN-3NV
Closes ENG-21523
How did you verify your code works?
Manually. I'm not sure how to write a test yet other than manually clicking sleep