Repository navigation
fix(console) Async frame deduplication - #26028
gavinhailey wants to merge 7 commits into
Conversation
WalkthroughThis PR introduces duplicate-frame and internal-builtin-frame filtering for stack traces in Bun. It adds helper functions to detect consecutive duplicate async frames and internal builtin frames, refines frame collection logic during error stack trace assembly, and updates stack trace formatting to apply filtering to the output. Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @test/js/bun/test/stack-format.test.ts:
- Around line 6-7: The test "deep async call stacks should not have duplicates"
mutates Error.stackTraceLimit without restoring it; capture the original value
(e.g., const original = Error.stackTraceLimit) before setting
Error.stackTraceLimit = 50 and restore it at the end of the test (in a finally
block or after hook) so other tests are unaffected; reference
Error.stackTraceLimit and the test name to locate where to save and restore the
value.
📜 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 (5)
src/bun.js/bindings/ErrorStackTrace.cppsrc/bun.js/bindings/ErrorStackTrace.hsrc/bun.js/bindings/FormatStackTraceForJS.cpptest/js/bun/test/stack-format.test.tstest/regression/issue/22339.test.ts
🧰 Additional context used
📓 Path-based instructions (5)
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: Create classes in three parts in C++ when there is a public constructor: Foo (JSDestructibleObject), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Define properties using HashTableValue arrays in C++ JavaScript class bindings
Add iso subspaces for C++ classes with fields in JavaScript class bindings
Cache structures in ZigGlobalObject for JavaScript class bindings
Files:
src/bun.js/bindings/FormatStackTraceForJS.cppsrc/bun.js/bindings/ErrorStackTrace.cpp
**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts?(x): Never usebun testdirectly - always usebun bd testto run tests with debug build changes
For single-file tests, prefer-eflag overtempDir
For multi-file tests, prefertempDirandBun.spawnover single-file tests
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
UsetempDirfromharnessto create temporary directories - do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests; instead await the condition to be met
Verify tests fail withUSE_SYSTEM_BUN=1 bun test <file>and pass withbun bd test <file>- tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with.test.tsor.test.tsx
Avoid shell commands likefindorgrepin tests - use Bun's Glob and built-in tools instead
Files:
test/js/bun/test/stack-format.test.tstest/regression/issue/22339.test.ts
test/**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
port: 0in tests - do not hardcode ports or use custom random port number functions
Files:
test/js/bun/test/stack-format.test.tstest/regression/issue/22339.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun bd test <...test file>to run tests with compiled code changes. Do not usebun testas it will not include your changes.
Usebun:testfor files ending in*.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, usebun bd <file>instead ofbun bd test <file>since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.
Files:
test/js/bun/test/stack-format.test.tstest/regression/issue/22339.test.ts
test/regression/issue/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place regression tests for specific GitHub issues in
test/regression/issue/${issueNumber}.test.tswith real issue numbers only
Files:
test/regression/issue/22339.test.ts
🧠 Learnings (44)
📓 Common learnings
Learnt from: alii
Repo: oven-sh/bun PR: 25360
File: src/bake/hmr-runtime-client.ts:240-249
Timestamp: 2025-12-05T19:28:39.534Z
Learning: In Bun's HMR runtime client (src/bake/hmr-runtime-client.ts), when handling error events where `event.error` is falsy, do not wrap `event.message` in a new Error object. Doing so would create misleading stack frames that show the error originating from the HMR client code rather than the actual error location, which hinders debugging.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced.
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 25111
File: packages/bun-types/wasm.d.ts:103-104
Timestamp: 2025-11-27T15:18:21.607Z
Learning: In the Bun repository, autofixci automatically formats code in the main branch, which can cause formatting diffs to appear in pull requests even when the PR author didn't make those changes. These formatting changes from autofixci are expected and not the PR author's responsibility.
📚 Learning: 2025-11-24T18:37:47.899Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/AGENTS.md:0-0
Timestamp: 2025-11-24T18:37:47.899Z
Learning: Applies to src/bun.js/bindings/v8/**/<UNKNOWN> : <UNKNOWN>
Applied to files:
src/bun.js/bindings/FormatStackTraceForJS.cppsrc/bun.js/bindings/ErrorStackTrace.h
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.cpp : Use JSC::WriteBarrier for heap-allocated references in V8 objects and implement visitChildren() for custom heap objects to support garbage collection
Applied to files:
src/bun.js/bindings/FormatStackTraceForJS.cppsrc/bun.js/bindings/ErrorStackTrace.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.txt : Add symbol names without leading underscore to src/symbols.txt for each new V8 API method
Applied to files:
src/bun.js/bindings/FormatStackTraceForJS.cppsrc/bun.js/bindings/ErrorStackTrace.cppsrc/bun.js/bindings/ErrorStackTrace.h
📚 Learning: 2025-12-05T19:28:39.534Z
Learnt from: alii
Repo: oven-sh/bun PR: 25360
File: src/bake/hmr-runtime-client.ts:240-249
Timestamp: 2025-12-05T19:28:39.534Z
Learning: In Bun's HMR runtime client (src/bake/hmr-runtime-client.ts), when handling error events where `event.error` is falsy, do not wrap `event.message` in a new Error object. Doing so would create misleading stack frames that show the error originating from the HMR client code rather than the actual error location, which hinders debugging.
Applied to files:
src/bun.js/bindings/FormatStackTraceForJS.cppsrc/bun.js/bindings/ErrorStackTrace.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.dyn : Add symbol names with leading underscore and semicolons in braces to src/symbols.dyn for each new V8 API method
Applied to files:
src/bun.js/bindings/FormatStackTraceForJS.cppsrc/bun.js/bindings/ErrorStackTrace.cppsrc/bun.js/bindings/ErrorStackTrace.h
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.cpp : Use isolate->currentHandleScope()->createLocal<T>() to create local V8 handles and ensure all V8 values are created within an active handle scope
Applied to files:
src/bun.js/bindings/FormatStackTraceForJS.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.h : Add BUN_EXPORT visibility attribute to all public V8 API functions to ensure proper symbol export across platforms
Applied to files:
src/bun.js/bindings/FormatStackTraceForJS.cppsrc/bun.js/bindings/ErrorStackTrace.h
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.cpp : Create V8 class implementations with .cpp extension following the pattern V8ClassName.cpp that include the header, v8_compatibility_assertions.h, use ASSERT_V8_TYPE_LAYOUT_MATCHES macro, and implement methods using isolate->currentHandleScope()->createLocal<T>() for handle creation
Applied to files:
src/bun.js/bindings/FormatStackTraceForJS.cpp
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Add iso subspaces for C++ classes with fields in JavaScript class bindings
Applied to files:
src/bun.js/bindings/FormatStackTraceForJS.cppsrc/bun.js/bindings/ErrorStackTrace.cppsrc/bun.js/bindings/ErrorStackTrace.h
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.cpp : Use localToJSValue() to convert V8 handles to JSC values and perform JSC operations within V8 method implementations
Applied to files:
src/bun.js/bindings/FormatStackTraceForJS.cpp
📚 Learning: 2025-11-19T05:15:22.823Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 23764
File: src/bun.js/test/timers/FakeTimers.zig:240-0
Timestamp: 2025-11-19T05:15:22.823Z
Learning: In Bun's Zig codebase, `CallFrame.argumentsAsArray(N)` always returns an array of exactly N elements. If fewer than N arguments were passed to the function, the missing slots are filled with `.undefined` JSValue. This means accessing `argumentsAsArray(N)[i]` where i < N is always safe and will never trap—it will simply return `.undefined` if that argument wasn't provided by the caller.
Applied to files:
src/bun.js/bindings/FormatStackTraceForJS.cpp
📚 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/bindings/FormatStackTraceForJS.cpptest/js/bun/test/stack-format.test.tssrc/bun.js/bindings/ErrorStackTrace.h
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/js/bun/test/stack-format.test.tstest/regression/issue/22339.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun bd test <...test file>` to run tests with compiled code changes. Do not use `bun test` as it will not include your changes.
Applied to files:
test/js/bun/test/stack-format.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending in `*.test.{ts,js,jsx,tsx,mjs,cjs}`. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use `bun bd <file>` instead of `bun bd test <file>` since they expect exit code 0.
Applied to files:
test/js/bun/test/stack-format.test.tstest/regression/issue/22339.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Use `normalizeBunSnapshot` to normalize snapshot output of tests
Applied to files:
test/js/bun/test/stack-format.test.tstest/regression/issue/22339.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Never use `bun test` directly - always use `bun bd test` to run tests with debug build changes
Applied to files:
test/js/bun/test/stack-format.test.ts
📚 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:
test/js/bun/test/stack-format.test.tstest/regression/issue/22339.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*-fixture.ts : Test files that spawn Bun processes should end in `*-fixture.ts` to identify them as test fixtures rather than tests themselves.
Applied to files:
test/js/bun/test/stack-format.test.tstest/regression/issue/22339.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Do not set a timeout on tests. Bun already has timeouts built-in.
Applied to files:
test/js/bun/test/stack-format.test.tstest/regression/issue/22339.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
Applied to files:
test/js/bun/test/stack-format.test.tstest/regression/issue/22339.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : For multi-file tests, prefer `tempDir` and `Bun.spawn` over single-file tests
Applied to files:
test/js/bun/test/stack-format.test.tstest/regression/issue/22339.test.ts
📚 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:
test/js/bun/test/stack-format.test.tstest/regression/issue/22339.test.ts
📚 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:
test/js/bun/test/stack-format.test.tstest/regression/issue/22339.test.ts
📚 Learning: 2025-09-20T00:58:38.042Z
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.
Applied to files:
test/js/bun/test/stack-format.test.ts
📚 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:
test/js/bun/test/stack-format.test.tstest/regression/issue/22339.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Verify tests fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test <file>` - tests are invalid if they pass with USE_SYSTEM_BUN=1
Applied to files:
test/js/bun/test/stack-format.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to test/regression/issue/*.test.ts : Place regression tests for specific GitHub issues in `test/regression/issue/${issueNumber}.test.ts` with real issue numbers only
Applied to files:
test/regression/issue/22339.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Organize regression tests for specific issues in `/test/regression/issue/${issueNumber}.test.ts`. Do not place regression tests in the regression directory if there is no associated issue number.
Applied to files:
test/regression/issue/22339.test.ts
📚 Learning: 2025-09-20T00:57:56.685Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: test/js/valkey/valkey.test.ts:268-276
Timestamp: 2025-09-20T00:57:56.685Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods like ctx.redis.set() - the maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/regression/issue/22339.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Do not write flaky tests - do not use `setTimeout` in tests; instead await the condition to be met
Applied to files:
test/regression/issue/22339.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Applied to files:
test/regression/issue/22339.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8-module/main.cpp : Create test functions in test/v8/v8-module/main.cpp that take FunctionCallbackInfo<Value> parameter, use the test V8 API, print results for comparison with Node.js, and return Undefined
Applied to files:
test/regression/issue/22339.test.ts
📚 Learning: 2025-09-30T22:53:19.887Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 23117
File: src/bun.js/test/snapshot.zig:265-276
Timestamp: 2025-09-30T22:53:19.887Z
Learning: In Bun's snapshot testing (src/bun.js/test/snapshot.zig), multiple inline snapshots at the same line and column (same call position) must have identical values. However, multiple inline snapshots on the same line at different columns are allowed to have different values. The check is position-specific (line+col), not line-wide.
Applied to files:
test/regression/issue/22339.test.ts
📚 Learning: 2025-10-18T05:23:24.403Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/telemetry-server.test.ts:91-100
Timestamp: 2025-10-18T05:23:24.403Z
Learning: In the Bun codebase, telemetry tests (test/js/bun/telemetry-*.test.ts) should focus on telemetry API behavior: configure/disable/isEnabled, callback signatures and invocation, request ID correlation, and error handling. HTTP protocol behaviors like status code normalization (e.g., 200 with empty body → 204) should be tested in HTTP server tests (test/js/bun/http/), not in telemetry tests. Keep separation of concerns: telemetry tests verify the telemetry API contract; HTTP tests verify HTTP semantics.
Applied to files:
test/regression/issue/22339.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/napi/napi.zig : For each new V8 C++ method, add both GCC/Clang and MSVC mangled symbol names to the V8API struct in src/napi/napi.zig using extern fn declarations
Applied to files:
src/bun.js/bindings/ErrorStackTrace.cppsrc/bun.js/bindings/ErrorStackTrace.h
📚 Learning: 2025-12-23T06:50:41.142Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 25429
File: src/bun.js/bindings/helpers.h:422-422
Timestamp: 2025-12-23T06:50:41.142Z
Learning: In Bun's C++ bindings (src/bun.js/bindings/helpers.h and similar files), when returning an empty JSC::Identifier and a VM is accessible, prefer using `vm.propertyNames->emptyIdentifier` over constructing with `JSC::Identifier(JSC::Identifier::EmptyIdentifierFlag::EmptyIdentifier)`. The cached identifier from the VM's property names table is more efficient and consistent with WebKit upgrade patterns.
Applied to files:
src/bun.js/bindings/ErrorStackTrace.cpp
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Cache structures in ZigGlobalObject for JavaScript class bindings
Applied to files:
src/bun.js/bindings/ErrorStackTrace.cppsrc/bun.js/bindings/ErrorStackTrace.h
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use JSC intrinsics (prefixed with `$`) such as `$Array.from()`, `$isCallable()`, and `$newArrayWithSize()` for performance-critical operations
Applied to files:
src/bun.js/bindings/ErrorStackTrace.cpp
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Create classes in three parts in C++ when there is a public constructor: Foo (JSDestructibleObject), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Applied to files:
src/bun.js/bindings/ErrorStackTrace.cpp
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.ts : Builtin functions must include `this` parameter typing in TypeScript to enable direct method binding in C++
Applied to files:
src/bun.js/bindings/ErrorStackTrace.h
📚 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/ErrorStackTrace.h
📚 Learning: 2025-12-23T06:50:31.577Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 25429
File: src/bun.js/bindings/helpers.h:422-422
Timestamp: 2025-12-23T06:50:31.577Z
Learning: In Bun's C++ bindings, when returning an empty JSC::Identifier and a VM is accessible, prefer using vm.propertyNames->emptyIdentifier over constructing with JSC::Identifier(JSC::Identifier::EmptyIdentifierFlag::EmptyIdentifier). The cached identifier from the VM's property names table is more efficient and consistent with WebKit upgrade patterns. Apply this guidance to src/bun.js/bindings/helpers.h and similar header files in the same bindings directory (i.e., any file that constructs an EmptyIdentifier).
Applied to files:
src/bun.js/bindings/ErrorStackTrace.h
🧬 Code graph analysis (3)
src/bun.js/bindings/FormatStackTraceForJS.cpp (2)
src/bun.js/bindings/ZigException.cpp (2)
frame(264-410)frame(264-264)src/bun.js/bindings/ErrorStackTrace.cpp (10)
isImplementationVisibilityPrivate(43-68)isImplementationVisibilityPrivate(43-43)isImplementationVisibilityPrivate(70-94)isImplementationVisibilityPrivate(70-70)isInternalBuiltinFrame(223-231)isInternalBuiltinFrame(223-223)isConsecutiveDuplicateAsyncFrame(125-164)isConsecutiveDuplicateAsyncFrame(125-125)isConsecutiveDuplicateAsyncFrame(166-220)isConsecutiveDuplicateAsyncFrame(166-166)
src/bun.js/bindings/ErrorStackTrace.cpp (1)
src/bun.js/bindings/ZigException.cpp (2)
frame(264-410)frame(264-264)
src/bun.js/bindings/ErrorStackTrace.h (1)
src/bun.js/bindings/ErrorStackTrace.cpp (6)
isConsecutiveDuplicateAsyncFrame(125-164)isConsecutiveDuplicateAsyncFrame(125-125)isConsecutiveDuplicateAsyncFrame(166-220)isConsecutiveDuplicateAsyncFrame(166-166)isInternalBuiltinFrame(223-231)isInternalBuiltinFrame(223-223)
🔇 Additional comments (18)
src/bun.js/bindings/ErrorStackTrace.h (1)
213-214: LGTM!The new function declarations are consistent with their implementations in ErrorStackTrace.cpp and appropriately placed in the
Zignamespace alongside the existingisImplementationVisibilityPrivateoverloads.src/bun.js/bindings/FormatStackTraceForJS.cpp (3)
150-174: LGTM - Efficient index-based filtering.The filtering approach using
filteredIndicesis well-designed:
- Avoids copying frames by storing indices
- Correctly tracks
prevUserFramefor duplicate detection- Applies filters in the right order (private visibility → builtin → duplicate)
One observation: This file appears to have local implementations of
isConsecutiveDuplicateAsyncFrame(per the relevant code snippets at lines 124-219) that are similar to those inErrorStackTrace.cpp. If these are intentionally different overloads (one forStackVisitor, one forStackFrame), this is fine. Otherwise, consider consolidating.
203-203: Correctly updated to use filtered frames.The SyntaxError handling now properly uses the first filtered frame instead of the raw first frame, which aligns with the deduplication behavior.
257-265: LGTM - Safe indexed access.The assertion and loop correctly use the filtered indices. The bounds are safe since
i < framesCountwhereframesCount = filteredIndices.size().test/regression/issue/22339.test.ts (5)
1-16: Well-structured regression test with clear issue reference.The file is correctly placed in
test/regression/issue/with a real GitHub issue number. ThegetCallSites()helper properly captures stack traces usingError.prepareStackTrace.
18-35: Good duplicate detection helper.The
hasConsecutiveDuplicateslogic correctly mirrors the C++ implementation incompareFramesForDuplicates:
- Same function name
- Same file name
- Line numbers within 1 line of each other
37-62: Comprehensive async chain test.Good test coverage that verifies:
- No consecutive duplicates in the captured stack
- Expected function names are present
- Each function appears exactly once (for non-recursive case)
89-110: Important recursion test.This test correctly validates that actual recursive calls are preserved (multiple frames allowed) while consecutive duplicates (JSC artifacts) are filtered. The comment at lines 94-95 and 101-102 clearly explains the distinction.
166-191: Good baseline test for sync functions.Ensures the deduplication logic doesn't inadvertently affect regular synchronous function stacks.
test/js/bun/test/stack-format.test.ts (3)
25-43: Good comprehensive assertions.The test properly validates:
- All 5 levels are present in the stack
- No consecutive duplicate lines
- No
asyncFunctionResumeinternal frames leak through
46-65: Important recursive async test.Correctly validates that actual recursion preserves multiple frames while filtering duplicates.
139-157: Good Promise rejection coverage.Tests the edge case of
Promise.reject()stack traces, ensuring deduplication works correctly for rejected promises.src/bun.js/bindings/ErrorStackTrace.cpp (6)
96-123: Well-documented duplicate detection logic.The comment at lines 117-120 clearly explains the rationale for the 1-line threshold for detecting JSC's duplicate async frames. The logic correctly handles edge cases (empty names, empty URLs).
125-164: Potential inconsistency in function name retrieval.The
StackVisitoroverload retrievescurrentFunctionNameviavisitor->functionName()(line 131), but forprevFrameit only usesjsFunc->name(vm)(line 136).In contrast, the
StackFrameoverload (lines 178-194) triesZig::functionName(vm, codeBlock)first, then falls back tojsFunc->name(vm).This asymmetry could cause missed duplicate detection if the function name is stored differently (e.g., in the code block's ecmaName vs the JSFunction's name property).
Consider aligning the name retrieval logic:
♻️ Suggested alignment
WTF::String prevFunctionName; if (auto* callee = prevFrame->callee()) { if (auto* jsFunction = jsDynamicCast<JSFunction*>(callee)) { - prevFunctionName = jsFunction->name(vm); + if (auto* codeBlock = prevFrame->codeBlock()) { + prevFunctionName = Zig::functionName(vm, codeBlock); + } + if (prevFunctionName.isEmpty()) { + prevFunctionName = jsFunction->name(vm); + } } }
166-220: Consistent name retrieval in StackFrame overload.This overload correctly tries
Zig::functionName(vm, codeBlock)first before falling back tojsFunc->name(vm)for both current and previous frames. This is the correct pattern.
222-231: Clean builtin detection.Simple and effective check using the unlinked code block's
isBuiltinFunction()method.
243-265: Correct filtering infromExisting.The filtering logic properly:
- Skips private visibility frames
- Skips internal builtin frames
- Skips consecutive duplicate async frames
- Tracks
prevUserFramecorrectly for duplicate detection
367-427: Comprehensive frame tracking ingetFramesForCaller.The dual-pointer approach (
prevFrameandprevUserFrame) is well-designed:
prevFrametracks the last added frame for general purposesprevUserFrametracks the last non-builtin frame specifically for duplicate detectionLines 416-418 correctly update
prevUserFrameonly for non-builtin frames.
| test("deep async call stacks should not have duplicates", async () => { | ||
| Error.stackTraceLimit = 50; // Increase to see all frames |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider saving and restoring Error.stackTraceLimit.
Setting Error.stackTraceLimit = 50 without restoring the original value could affect subsequent tests in the same test run.
♻️ Suggested fix
test("deep async call stacks should not have duplicates", async () => {
+ const originalLimit = Error.stackTraceLimit;
Error.stackTraceLimit = 50; // Increase to see all frames
+ try {And at the end of the test:
// No asyncFunctionResume
expect(stack).not.toContain("asyncFunctionResume");
}
+ } finally {
+ Error.stackTraceLimit = originalLimit;
+ }
});🤖 Prompt for AI Agents
In @test/js/bun/test/stack-format.test.ts around lines 6 - 7, The test "deep
async call stacks should not have duplicates" mutates Error.stackTraceLimit
without restoring it; capture the original value (e.g., const original =
Error.stackTraceLimit) before setting Error.stackTraceLimit = 50 and restore it
at the end of the test (in a finally block or after hook) so other tests are
unaffected; reference Error.stackTraceLimit and the test name to locate where to
save and restore the value.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Thank you for this PR.
This should be fixed in WebKit upstream and not in Bun - and it will be fixed in the next upgrade
|
That makes sense, felt a bit patchy implementing it, looks like this WebKit PR fixed it. |
What does this PR do?
This PR fixes duplicate consecutive stack frames in async functions (#22339) by implementing frame deduplication logic in Bun's stack trace generation. When JavaScriptCore generates stack traces for async functions, it creates duplicate frames on adjacent lines (one for the function declaration, another for the await expression). This PR filters these duplicates to match Node.js behavior, while preserving actual recursive async calls.
This behavior also seems to cause issues with Nx in #25695
The fix operates in two key locations:
ErrorStackTrace.cpp- When building JSCStackTrace from frames during error constructionFormatStackTraceForJS.cpp- When formatting stack traces into strings for Error.stackThe deduplication logic compares consecutive frames and removes duplicates when they:
How did you verify your code works?
bun bd test test/regression/issue/22339.test.tsandbun bd test test/js/bun/test/stack-format.test.ts- all tests passUSE_SYSTEM_BUN=1 bun test <file>, ensuring they actually validate the fix