Don't use globalThis.takeException when rejecting Promise values#24986
Merged
Conversation
Collaborator
|
Updated 1:51 AM PT - Nov 23rd, 2025
❌ @Jarred-Sumner, your commit cea3ff6 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 24986That installs a local version of the PR into your bun-24986 --bun |
Contributor
WalkthroughPublic error-handling types and flows were unified: bundler plugin end-callback APIs now accept Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (8)📓 Common learnings📚 Learning: 2025-09-05T19:49:26.188ZApplied to files:
📚 Learning: 2025-11-03T20:40:59.655ZApplied to files:
📚 Learning: 2025-10-18T20:50:47.750ZApplied to files:
📚 Learning: 2025-09-05T19:49:26.188ZApplied to files:
📚 Learning: 2025-11-14T16:07:01.064ZApplied to files:
📚 Learning: 2025-11-03T20:43:06.996ZApplied to files:
📚 Learning: 2025-09-19T19:55:22.427ZApplied to files:
🔇 Additional comments (1)
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 (3)
src/bun.js/api/JSBundler.zig(2 hunks)src/bun.js/bindings/JSPromise.zig(1 hunks)src/bundler/bundle_v2.zig(3 hunks)
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
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.
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.
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: src/valkey/js_valkey_functions.zig:853-854
Timestamp: 2025-09-19T19:55:22.427Z
Learning: In Bun's JSPromise API, `resolvedPromiseValue(globalThis: *JSGlobalObject, value: JSValue) JSValue` returns a JSValue directly (not a JSPromise that needs `.toJS()` called on it). This is different from `resolvedPromise()` which returns a *JSPromise that would need `.toJS()`.
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: test/js/valkey/valkey.test.ts:561-564
Timestamp: 2025-09-20T00:58:38.042Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods (like ctx.redis.set(), ctx.redis.unsubscribe(), etc.) - Bun's Redis client implementation differs from Node.js and can throw synchronously even for async methods. The maintainer has explicitly requested to stop looking at this error pattern.
📚 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/bun.js/api/JSBundler.zigsrc/bun.js/bindings/JSPromise.zigsrc/bundler/bundle_v2.zig
📚 Learning: 2025-11-14T16:07:01.064Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 24719
File: docs/bundler/executables.mdx:527-560
Timestamp: 2025-11-14T16:07:01.064Z
Learning: In the Bun repository, certain bundler features like compile with code splitting (--compile --splitting) are CLI-only and not supported in the Bun.build() JavaScript API. Tests for CLI-only features use backend: "cli" flag (e.g., test/bundler/bundler_compile_splitting.test.ts). The CompileBuildConfig interface correctly restricts these with splitting?: never;. When documenting CLI-only bundler features, add a note clarifying they're not available via the programmatic API.
Applied to files:
src/bun.js/api/JSBundler.zigsrc/bundler/bundle_v2.zig
📚 Learning: 2025-09-05T19:49:26.188Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun’s Zig code, `.js_undefined` is a valid and preferred JSValue literal for “undefined” (e.g., resolving JSPromise). Do not refactor usages to `jsc.JSValue.jsUndefined()`, especially in src/valkey/js_valkey_functions.zig unsubscribe().
Applied to files:
src/bun.js/api/JSBundler.zigsrc/bun.js/bindings/JSPromise.zigsrc/bundler/bundle_v2.zig
📚 Learning: 2025-10-18T20:50:47.750Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:366-373
Timestamp: 2025-10-18T20:50:47.750Z
Learning: In Bun's Zig codebase (src/bun.js/bindings/JSValue.zig), the JSValue enum uses `.null` (not `.js_null`) for JavaScript's null value. Only `js_undefined` has the `js_` prefix to avoid collision with Zig's built-in `undefined` keyword. The correct enum fields are: `js_undefined`, `null`, `true`, `false`, and `zero`.
Applied to files:
src/bun.js/api/JSBundler.zigsrc/bun.js/bindings/JSPromise.zig
📚 Learning: 2025-09-05T19:49:26.188Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun's Zig codebase, `.js_undefined` is a valid way to represent JavaScript's undefined value when working with JSPromise.resolve() and similar JavaScript interop functions. This is the correct pattern to use rather than `jsc.JSValue.jsUndefined()`.
Applied to files:
src/bun.js/api/JSBundler.zigsrc/bun.js/bindings/JSPromise.zig
📚 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/api/JSBundler.zigsrc/bun.js/bindings/JSPromise.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/api/JSBundler.zig
📚 Learning: 2025-09-19T19:55:22.427Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: src/valkey/js_valkey_functions.zig:853-854
Timestamp: 2025-09-19T19:55:22.427Z
Learning: In Bun's JSPromise API, `resolvedPromiseValue(globalThis: *JSGlobalObject, value: JSValue) JSValue` returns a JSValue directly (not a JSPromise that needs `.toJS()` called on it). This is different from `resolvedPromise()` which returns a *JSPromise that would need `.toJS()`.
Applied to files:
src/bun.js/api/JSBundler.zigsrc/bun.js/bindings/JSPromise.zigsrc/bundler/bundle_v2.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/bun.js/bindings/JSPromise.zig
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.
Applied to files:
src/bun.js/bindings/JSPromise.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/bun.js/bindings/JSPromise.zig
📚 Learning: 2025-09-05T19:48:44.481Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/valkey.zig:0-0
Timestamp: 2025-09-05T19:48:44.481Z
Learning: In ValkeyCommand.zig, the Promise type is a wrapper that encapsulates valkey-specific behavior. The actual underlying promise is accessed via the `.promise` field, so the correct pattern is `promise_ptr.promise.resolve(...)` not `promise_ptr.resolve(...)`.
Applied to files:
src/bun.js/bindings/JSPromise.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/bindings/JSPromise.zig
Comment on lines
1182
to
1188
| this, | ||
| build_promise.asValue(globalThis), | ||
| build_result, | ||
| rejection, | ||
| rejection catch |err| globalThis.takeException(err), | ||
| ); | ||
|
|
||
| try scope.returnIfException(); |
Contributor
There was a problem hiding this comment.
Don't reintroduce takeException OOM path in plugin rejection
We changed JSPromise.reject specifically to avoid globalThis.takeException() because it blows up on error.OutOfMemory, yet this catch block still calls it. If the rejection arrives as error.OutOfMemory, we'll land here, call takeException, and hit the same OOM panic this PR set out to eliminate. Please mirror the new pattern from JSPromise.reject: synthesize the OOM value, propagate error.JSTerminated, and otherwise fall back to tryTakeException().
- const value = JSBundlerPlugin__runOnEndCallbacks(
- this,
- build_promise.asValue(globalThis),
- build_result,
- rejection catch |err| globalThis.takeException(err),
- );
+ const rejection_value = rejection catch |err| switch (err) {
+ error.OutOfMemory => globalThis.createOutOfMemoryError(),
+ error.JSTerminated => return err,
+ else => err: {
+ const exception = globalThis.tryTakeException() orelse {
+ @panic("A JavaScript exception was thrown, but it was cleared before it could be read.");
+ };
+ break :err exception.toError() orelse exception;
+ },
+ };
+
+ const value = JSBundlerPlugin__runOnEndCallbacks(
+ this,
+ build_promise.asValue(globalThis),
+ build_result,
+ rejection_value,
+ );🤖 Prompt for AI Agents
In src/bun.js/api/JSBundler.zig around lines 1182 to 1188, the plugin rejection
catch currently calls globalThis.takeException(), reintroducing the OOM panic;
change this to follow the JSPromise.reject pattern: do not call takeException(),
instead detect error.OutOfMemory and synthesize an OOM JS Error value to use as
the rejection value, map error.JSTerminated to propagate that state, and for all
other errors use globalThis.tryTakeException() (or equivalent non-panicking
extraction) so we avoid the OOM panic while preserving existing error
propagation semantics.
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
2 participants
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 can't use globalThis.takeException() because it throws out of memory error when we instead need to take the exception.
How did you verify your code works?