ffi: wrap JSCallback objects passed as cc() function arguments - #31776
Closed
EffortlessSteven wants to merge 1 commit into
Closed
ffi: wrap JSCallback objects passed as cc() function arguments#31776EffortlessSteven wants to merge 1 commit into
EffortlessSteven wants to merge 1 commit into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
WalkthroughThe PR fixes a bug in Bun's FFI ChangesFFI symbol definition lookup fix
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/ffi/cc.test.ts`:
- Around line 399-427: Replace the call to tempDirWithFiles inside the beforeAll
block with tempDir and pass the same directory name and file tree object (the
JS/C sources) as the fileTree argument; locate the beforeAll that assigns dir =
tempDirWithFiles("bun-ffi-cc-jscallback-arg", { ... }) and change it to use dir
= tempDir("bun-ffi-cc-jscallback-arg", { /* same file tree with "call_cb.c" and
"probe.js" */ }), leaving the rest of the setup (JSCallback usage, symbols, and
assertions) unchanged so the harness pre-populates fixtures via tempDir instead
of tempDirWithFiles.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 91b7e9fa-8093-41e0-9d1a-c2fbdc3c60a4
📒 Files selected for processing (2)
src/js/bun/ffi.tstest/js/bun/ffi/cc.test.ts
Comment on lines
+399
to
+427
| beforeAll(() => { | ||
| dir = tempDirWithFiles("bun-ffi-cc-jscallback-arg", { | ||
| "call_cb.c": /* c */ ` | ||
| typedef int (*bun_test_cb)(int); | ||
| int call_cb(void *cb, int value) { return ((bun_test_cb)cb)(value); } | ||
| `, | ||
| "probe.js": /* js */ ` | ||
| import { cc, JSCallback } from "bun:ffi"; | ||
| import source from "./call_cb.c" with { type: "file" }; | ||
|
|
||
| const { symbols } = cc({ | ||
| source, | ||
| symbols: { call_cb: { args: ["function", "int"], returns: "int" } }, | ||
| }); | ||
|
|
||
| const cb = new JSCallback(value => value + 1, { args: ["int"], returns: "int" }); | ||
|
|
||
| if (symbols.call_cb(cb, 41) !== 42) throw new Error("expected the wrapped callback to return 42"); | ||
| cb.close(); | ||
| let threw = false; | ||
| try { | ||
| symbols.call_cb(cb, 41); | ||
| } catch (e) { | ||
| threw = e?.name === "TypeError"; | ||
| } | ||
| if (!threw) throw new Error("expected a TypeError after close (ptr is null)"); | ||
| console.log("ok"); | ||
| `, | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use tempDir(..., fileTree) here instead of tempDirWithFiles.
This adds another tempDirWithFiles site in a multi-file test, but the harness already supports pre-populating fixtures via tempDir(prefix, fileTree). Please switch this setup to tempDir to match the test-harness convention for new coverage in this file.
Based on learnings, tempDir(prefix, fileTree) is available here, and the test guidelines say to use tempDir from harness to create temporary directories.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/js/bun/ffi/cc.test.ts` around lines 399 - 427, Replace the call to
tempDirWithFiles inside the beforeAll block with tempDir and pass the same
directory name and file tree object (the JS/C sources) as the fileTree argument;
locate the beforeAll that assigns dir =
tempDirWithFiles("bun-ffi-cc-jscallback-arg", { ... }) and change it to use dir
= tempDir("bun-ffi-cc-jscallback-arg", { /* same file tree with "call_cb.c" and
"probe.js" */ }), leaving the rest of the setup (JSCallback usage, symbols, and
assertions) unchanged so the harness pre-populates fixtures via tempDir instead
of tempDirWithFiles.
Contributor
Author
There was a problem hiding this comment.
Keeping tempDirWithFiles here. The other four fixture setups in this file (lines 35, 108, 208, 284) use it, and tempDir is a thin wrapper around it (harness.ts:299) whose only addition is using-based disposal. A beforeAll/afterAll setup can't use using (the directory must outlive each it), so tempDir would add no behavior, make this block the file's lone outlier, and use a disposable non-disposably. tempDirWithFiles is the right primitive for this pattern.
Contributor
There was a problem hiding this comment.
That's a correct and well-reasoned call. The Symbol.dispose capability that tempDir adds over tempDirWithFiles is only useful when you can actually use using; in a beforeAll/afterAll lifecycle the directory has to stay alive across the entire describe block, so tempDir would be a disposable used non-disposably — worse than the alternative, not better. tempDirWithFiles is the right primitive here, and staying consistent with the four existing setups in the file is the right call. I'll retract the suggestion.
🐇 withdraws the sticky note
✏️ Learnings added
Learnt from: EffortlessSteven
Repo: oven-sh/bun PR: 31776
File: test/js/bun/ffi/cc.test.ts:399-427
Timestamp: 2026-06-03T19:45:00.193Z
Learning: In oven-sh/bun test files that follow a `beforeAll`/`afterAll` teardown pattern (where `dir` is assigned in `beforeAll` and removed in `afterAll`), prefer `tempDirWithFiles(prefix, fileTree)` over `tempDir(prefix, fileTree)`. `tempDir`'s sole advantage over `tempDirWithFiles` is the `Symbol.dispose` / `using` automatic-disposal support; in a `beforeAll`/`afterAll` lifecycle the directory must outlive individual `it` blocks and `using` cannot be used, so switching to `tempDir` adds no behavior and makes the site a misleading outlier. Do NOT flag `tempDirWithFiles(prefix, fileTree)` in `beforeAll`/`afterAll` blocks as a guideline violation — it is the correct primitive for multi-test fixture directories.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: robobun
Repo: oven-sh/bun PR: 31772
File: src/jsc/bindings/ZigGlobalObject.cpp:636-658
Timestamp: 2026-06-03T17:25:13.735Z
Learning: In `src/jsc/bindings/ZigGlobalObject.cpp` (oven-sh/bun), the shared module-state teardown sequence — taking `WTF::Locker` on `moduleLoader->cellLock()`, calling `moduleLoader->clearAll()`, and then `requireMap()->clear(globalObject)` — has been extracted into `GlobalObject::clearModuleRegistry()`. The three callers are `GlobalObject::reload()`, `Zig__GlobalObject__createForTestIsolation()`, and `Zig__GlobalObject__destructOnExit()`. Each caller manages its own JSC exception scope (`DECLARE_THROW_SCOPE`, `RETURN_IF_EXCEPTION`, `assertNoException`, etc.) independently; `clearModuleRegistry()` itself does not interact with the throw scope.
Learnt from: robobun
Repo: oven-sh/bun PR: 27056
File: test/bundler/standalone.test.ts:281-324
Timestamp: 2026-02-16T04:26:25.185Z
Learning: In Bun test files that exercise Bun.build(), assertions for configuration-validation errors thrown synchronously by JSBundler.fromJS() (via globalThis.throwInvalidArguments()) should use toThrow, e.g., expect(() => Bun.build({...})).toThrow()). Do not use rejects.toThrow() since rejections occur only for asynchronous build errors.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 27385
File: test/js/bun/http/tls-keepalive.test.ts:115-140
Timestamp: 2026-02-24T21:02:00.725Z
Learning: In Bun's test suites, avoid adding tests for trivial environment/fixture script validation (e.g., checking if env vars exist) within test fixtures. Focus test coverage on actual behavior being tested (e.g., TLS keepalive, memory leaks) rather than auxiliary fixture validation. If a test file is primarily for fixtures, skip or limit tests that validate simple JS behavior like if (!env) throw; prioritize meaningful end-to-end or unit behavior instead.
Learnt from: LawoodDev
Repo: oven-sh/bun PR: 27855
File: test/cli/run/concurrency-filter.test.ts:32-32
Timestamp: 2026-03-06T16:21:42.189Z
Learning: In Bun's test runner, describe.concurrent is supported (since Bun v1.2.23). Use describe.concurrent/test.concurrent for concurrent tests. Be aware of limitations: expect.assertions() and expect.hasAssertions() are not supported; toMatchSnapshot() is not supported (toMatchInlineSnapshot() is); and beforeAll/afterAll hooks are not executed concurrently. The broader guideline to prefer concurrent tests over sequential tests using test.concurrent or describe.concurrent remains valid and should be applied to test files such as test/cli/run/concurrency-filter.test.ts and similar test files.
Learnt from: LawoodDev
Repo: oven-sh/bun PR: 27855
File: test/cli/run/concurrency-filter.test.ts:32-32
Timestamp: 2026-03-06T16:22:55.570Z
Learning: In test/cli/run/concurrency-filter.test.ts and similar test files, timing-sensitive tests that assert on wall-clock elapsed time to verify concurrency behavior (e.g., expect(elapsed).toBeGreaterThan(800)) must remain in a sequential describe block rather than describe.concurrent. Running such tests concurrently can cause CPU contention and skew timing assertions, leading to flaky results. The guideline to prefer describe.concurrent does NOT apply for timing-based correctness verification.
Learnt from: robobun
Repo: oven-sh/bun PR: 28214
File: test/regression/issue/18115.test.ts:1-158
Timestamp: 2026-03-18T15:19:38.407Z
Learning: In Bun test files, when a resource like tempDir is a DisposableString implementing both Symbol.dispose (sync) and Symbol.asyncDispose, prefer plain using over await using. Do not recommend converting to await using for tempDir in Bun test files. This keeps tests idiomatic and avoids unnecessary async disposal. If a resource only supports asyncDispose, use await using.
Learnt from: robobun
Repo: oven-sh/bun PR: 28425
File: test/regression/issue/28422.test.ts:65-79
Timestamp: 2026-03-22T10:12:05.719Z
Learning: In oven-sh/bun test files matching test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}, follow CLAUDE.md by asserting the command exit code LAST—after all other assertions such as stdout/stderr checks and filesystem validation. Do not assert exitCode earlier than those checks. Also, avoid asserting stdout for commands like bun install whose output can vary between runs.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 28863
File: scripts/build/deps/webkit.ts:149-161
Timestamp: 2026-04-04T19:43:49.607Z
Learning: When reviewing Node/TypeScript code that uses `node:path.join()`, do not treat a later path segment that starts with `/` as a Windows/absolute-path override bug. `path.join()` concatenates segments and normalizes; it only resets the root when using `path.resolve()` (e.g., when it encounters an absolute-looking segment). Therefore, patterns like `join(base, "/relPath")` or `join(homedir(), env.slice(1))` where `env.slice(1)` becomes `"/WebKit"` are expected to produce `base/relPath` (cross-platform). Only flag cases where `path.resolve()` (or other root-resetting logic) is used in a way that could unintentionally ignore the base path.
Learnt from: robobun
Repo: oven-sh/bun PR: 28923
File: test/regression/issue/28921.test.ts:0-0
Timestamp: 2026-04-06T19:19:08.790Z
Learning: In oven-sh/bun tests, prefer `tempDir` (from the `harness` module) over `tempDirWithFiles` when using the `using` statement for automatic cleanup. `tempDirWithFiles(...)` returns a plain `string`, so `using tempDirWithFiles(...)` is effectively a no-op and will not trigger disposal/cleanup. `tempDir` returns a `DisposableString` that implements `Symbol.dispose`, so it will correctly trigger cleanup on scope exit.
Learnt from: robobun
Repo: oven-sh/bun PR: 29050
File: test/regression/issue/29042.test.ts:60-94
Timestamp: 2026-04-08T21:22:00.840Z
Learning: In this repo’s Bun environment, `Bun.RedisClient` does not implement `Symbol.dispose` or `Symbol.asyncDispose`, so you cannot rely on `using` / `await using` for automatic cleanup. When creating a `Bun.RedisClient` in tests, close it explicitly with `try/finally`, calling `client.close()` in the `finally` block.
Learnt from: robobun
Repo: oven-sh/bun PR: 29322
File: test/js/web/workers/worker-terminate-after-exit.test.ts:38-43
Timestamp: 2026-04-15T01:57:52.469Z
Learning: In oven-sh/bun test files (matching `test/**/*.test.ts`), when you spawn a subprocess in a bun:test and you assert on its exit code, follow the CLAUDE.md house style: write `if (exitCode !== 0) { expect(stderr).toBe(""); }` immediately before `expect(exitCode).toBe(0)`. This is intentional so that, on failure, bun:test surfaces the full `stderr` content in the diff output. Do not replace this with a custom/second assertion that formats stderr into the exit-code expectation (e.g., `expect(exitCode, \\`stderr: ${stderr}\\`).toBe(0)` or any single-assertion equivalent).
Learnt from: robobun
Repo: oven-sh/bun PR: 29389
File: test/js/bun/util/v8-heap-snapshot-large-strings.test.ts:4-152
Timestamp: 2026-04-17T02:55:14.338Z
Learning: In oven-sh/bun, do not enforce the `test/regression/issue/${issueNumber}.test.ts` placement rule based solely on PR descriptions that include a speculative GitHub issue link like “might fix `#NNNNN`” without a confirmed regression (e.g., no verifying stack trace/reproduction). If the issue is not confirmed per CLAUDE.md (“confirmed numbered issue” only), the test should be placed next to the closest related existing test file for the affected feature/module (e.g., alongside `test/js/bun/util/v8-heap-snapshot.test.ts`) and should not be flagged as a guideline violation. Likewise, tests that validate a broader behavioral invariant (e.g., V8-matching 1024-char string truncation in heap snapshots) are not purely issue regressions and should live with the feature’s existing test suite rather than under `test/regression/issue/`.
Learnt from: robobun
Repo: oven-sh/bun PR: 29426
File: test/js/node/tls/node-tls-root-certs-concurrent-init.test.ts:80-82
Timestamp: 2026-04-18T00:50:38.905Z
Learning: In oven-sh/bun Jest/Bun test files under `test/js/` that spawn subprocesses using `bunEnv` from the `harness` module, it’s safe and intentional to assert `expect(stderr).toBe("")` unconditionally. `bunEnv` sets `BUN_DEBUG_QUIET_LOGS=1`, which suppresses ASAN/debug-build stderr noise, so an unexpected stderr value should fail the test and show useful diagnostics. Do not gate `expect(stderr).toBe("")` behind `if (exitCode !== 0)` for these `bunEnv`-based subprocess tests—follow the established pattern used in similar tests (e.g., `test/js/node/tls/test-use-system-ca.test.ts`).
Learnt from: robobun
Repo: oven-sh/bun PR: 29538
File: test/js/bun/resolve/lower-using-bun-target.test.ts:80-82
Timestamp: 2026-04-21T09:47:19.303Z
Learning: In Bun JavaScript/TS tests under `test/js/bun/**` that run runtime subprocesses by spawning `bunExe()` with `bunEnv`, do not add strict `expect(stderr).toBe("")` assertions. In debug ASAN builds, stderr will include `WARNING: ASAN interferes with JSC signal handlers…` on every JS-process launch and it is not suppressed by `bunEnv` / `BUN_DEBUG_QUIET_LOGS=1`. Use the regression guards that are already effective for this area: assert an exact match on `stdout` and `expect(exitCode).toBe(0)`. If you must validate stderr, follow the repo’s filter-based convention: ignore/filter out lines starting with `"WARNING: ASAN interferes"`. If stdout + exitCode provide sufficient coverage, leaving stderr unchecked is acceptable.
Learnt from: robobun
Repo: oven-sh/bun PR: 29538
File: test/js/bun/resolve/lower-using-bun-target.test.ts:133-142
Timestamp: 2026-04-21T09:54:56.748Z
Learning: When testing `bun build` subprocesses in `test/js/bun/**/*.test.ts`, it is acceptable to assert `expect(stderr).toBe("")` (or otherwise expect no stderr noise). `bun build` is compiler-only and does not start a JS VM, so it should not emit the ASAN warning about interfering with JSC signal handlers. Only JS-executing subprocesses (e.g., `bun -e`, running built output like `bun out.js`) are expected to produce that warning, so do not treat empty-stderr assertions as brittle specifically for `bun build` in these tests.
Learnt from: robobun
Repo: oven-sh/bun PR: 29564
File: test/regression/issue/29513.test.ts:51-51
Timestamp: 2026-04-22T02:58:30.645Z
Learning: In oven-sh/bun TypeScript test files, it is acceptable to use `Bun.sleep(0)` specifically as a macrotask barrier to deterministically drain the pending microtask queue before asserting. Do NOT flag `Bun.sleep(0)` as a timing-wait violation. The “do not use setTimeout/Bun.sleep in tests” guideline is intended to prevent load-sensitive wall-clock delays (e.g., `Bun.sleep(100)` or other timing windows). Use `Bun.sleep(0)` only when you need to observe a fully settled Promise/microtask chain (e.g., after deferred resolution and multiple internal `.then()` hops) where a single `await Promise.resolve()` would not advance far enough; `Bun.sleep(0)` resumes in a later macrotask after pending microtasks complete, without relying on elapsed time.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 29581
File: src/bun.js/modules/NodeModuleModule.cpp:663-681
Timestamp: 2026-04-22T20:47:10.896Z
Learning: In oven-sh/bun code reviews, do not recommend adding standalone regression tests that depend on setting `BUN_JSC_validateExceptionChecks=1` to exercise JSC throw-scope/exception-scope validator paths (e.g., PropertyCallback/reify interactions like `reifyAllStaticProperties`). Per `CLAUDE.md`, tests are expected to pass with `USE_SYSTEM_BUN=1`, and `BUN_JSC_validateExceptionChecks` is a no-op on release/system Bun builds. Instead, treat this class of validator coverage issue as covered by: (1) the x64-asan CI shard that enables the validator automatically, and (2) the `test/no-validate-exceptions.txt` opt-out list for tests that hit pre-existing throw-scope assertion failures unrelated to the change under review. If helpful, add an in-source comment pointing to the specific existing exerciser (e.g., the relevant `tsgo/bun-types` test) to document the intent without relying on the env var.
Learnt from: robobun
Repo: oven-sh/bun PR: 29656
File: test/js/bun/s3/s3-path-double-free.test.ts:49-61
Timestamp: 2026-04-23T23:39:21.333Z
Learning: In Bun test files under `test/js/bun/**/*.test.ts`, prefer `test.each()` over `describe.each()` when each parameter value results in a single `test`/`it` assertion. Using `describe.each()` to wrap a single `test` adds unnecessary nesting. Only use `describe.each()` when you need multiple `test`/`it` blocks per parameter value.
Learnt from: robobun
Repo: oven-sh/bun PR: 29820
File: test/js/node/process/process-execve.test.ts:47-52
Timestamp: 2026-04-28T11:35:58.257Z
Learning: In oven-sh/bun test files under `test/**/*.test.ts`, when a test uses the `tempDir` fixture and spawns a subprocess via `await using proc = Bun.spawn(...)` (i.e., the embedded script runs as a spawned subprocess), do not recommend adding a fixture-level or embedded-script `setTimeout` watchdog to prevent hangs. The `await using` scope exit should terminate the subprocess automatically, and Bun test per-test timeouts already bound execution time. Also, avoid embedded `setTimeout` watchdog patterns that violate Bun’s “no setTimeout in tests” guideline. If the worker/subprocess exits silently without posting, rely on the test’s stdout/exitCode assertions plus Bun’s outer timeout rather than a watchdog, even when the embedded fixture script uses `worker_threads` or other async constructs.
Learnt from: robobun
Repo: oven-sh/bun PR: 29874
File: test/js/web/websocket/websocket-proxy-tunnel-upgrade-leak.test.ts:15-16
Timestamp: 2026-04-28T21:34:23.491Z
Learning: In oven-sh/bun, when a test is intentionally validating native refcount leak detection using Bun debug-only instrumentation (e.g., `BUN_DEBUG_alloc=1` and `[alloc] new(...)/destroy(...)` log lines produced only by debug builds when `Environment.enable_logs` is set), use `test.skipIf(!isDebug)` as the correct/intentional guard. Do not flag this `test.skipIf(!isDebug)` as a guideline violation for this class of tests. The debug-only `[alloc] ...` lines are absent in release and ASAN builds, and there is no equivalent observable system-Bun hook to assert a leak when only debug-build instrumentation exists (so the `USE_SYSTEM_BUN=1` rule in `CLAUDE.md` does not apply in this situation).
Learnt from: robobun
Repo: oven-sh/bun PR: 29876
File: test/js/bun/ffi/cc.test.ts:0-0
Timestamp: 2026-04-29T00:09:18.937Z
Learning: In oven-sh/bun tests, when using the `harness` module’s `tempDir`, prefer the overload that accepts an optional second argument: `tempDir(prefix, fileTree)`, where `fileTree` is an object in the same shape as `tempDirWithFiles` (e.g., `{ "file.c": "..." }`). This creates a disposable temp directory pre-populated with files. If the `tempDir` file-tree overload is available, don’t recommend a separate manual `fs.writeFile`/write step for pre-populating files (e.g., when using `using dir = tempDir("prefix", { ... })`).
Learnt from: robobun
Repo: oven-sh/bun PR: 29876
File: test/js/bun/ffi/cc.test.ts:205-231
Timestamp: 2026-04-29T00:24:38.784Z
Learning: In oven-sh/bun’s Bun test files under test/js/bun/, do not treat explicit per-test timeouts as a guideline violation when the test is an RSS-leak regression that spawns a subprocess and performs many iterations (subprocess-heavy leak tests). For these cases, Bun’s default per-test timeout (5s locally) is insufficient—especially under debug+ASAN where these tests may take ~5–14s—so reviewers should expect and accept an explicit, larger per-test timeout (e.g., 60_000). Concretely, tests like the cc() option-string leak test (test/js/bun/ffi/cc.test.ts) and glob-leak tests (e.g., test/js/bun/glob/leak.test.ts) should be reviewed as exceptions: allow explicit timeouts when the intent is to cover RSS-leak/subprocess-heavy regression workloads.
Learnt from: robobun
Repo: oven-sh/bun PR: 29919
File: test/js/bun/util/filesystem_router.test.ts:613-628
Timestamp: 2026-05-02T00:35:55.819Z
Learning: In oven-sh/bun tests under test/js/bun/**, prefer strict stderr assertions like `expect(stderr).toBe("")` for subprocesses spawned with `bunExe()` when you pass a `bunEnv` that already propagates `ASAN_OPTIONS=allow_user_segv_handler=1` from the parent `bun bd` build environment (this suppresses the `WARNING: ASAN interferes with JSC signal handlers` message). On CI ASAN lanes where `isASAN` is true, `bunEnv` sets `isASAN` explicitly as well—so strict stderr expectations are still safe. Only relax/skip strict stderr assertions (e.g., avoid `toBe("")`) when `ASAN_OPTIONS=allow_user_segv_handler=1` is *not* propagated into the subprocess environment.
Learnt from: robobun
Repo: oven-sh/bun PR: 30115
File: test/js/bun/glob/scan.test.ts:877-882
Timestamp: 2026-05-02T17:49:10.214Z
Learning: In oven-sh/bun regression tests for UAFs tied to Bun’s threadpool/event-loop interaction (e.g., WalkTask pending activity), keep the intended repro timing: use `Bun.sleepSync(N)` inside a spawned subprocess to hold the JS event loop without yielding/draining pending tasks, then trigger `Bun.gc(true)` (after the threadpool task has been given time to complete `run()`), and finally drive the result with the corresponding `for await`/iterator consumption to make the UAF observable. Do not replace `Bun.sleepSync(N)` with `await Bun.sleep(0)` or any other event-loop-yielding construct, since it can drain pending concurrent tasks and cause callbacks/`then()` work to run before the GC call, making the bug unobservable. This “sleepSync → gc(true) → for await” sequence is the correct 3-step UAF repro pattern for this bug class.
Learnt from: robobun
Repo: oven-sh/bun PR: 30142
File: test/js/bun/http/bun-serve-html-abort-leak-fixture.ts:28-38
Timestamp: 2026-05-03T01:29:10.031Z
Learning: In oven-sh/bun tests/fixtures that spawn subprocesses with `BUN_DEBUG_alloc` (or `BUN_DEBUG_ALL`) set to a non-zero value (e.g., `"1"`), the `[alloc]` log scope is effectively enabled at runtime for all `bun.new`/`bun.destroy`-allocated types. Because the runtime check in `src/output.zig` forces `really_disable = false` when `BUN_DEBUG_<tagname>` is not `"0"`, such fixtures may emit `[alloc] new(T)` / `[alloc] destroy(T)` lines even when `T` does not declare `log_allocations = true`. In this context, do not flag missing `log_allocations` declarations as a bug in the test fixture or the involved fixture types.
Learnt from: robobun
Repo: oven-sh/bun PR: 30153
File: test/bundler/plugin-sync-exception-fallback.test.ts:75-91
Timestamp: 2026-05-03T01:53:50.441Z
Learning: In this repo’s Bun test files that use `Bun.spawn`, don’t “parse/assert stdout before checking `exitCode`” when the expected failure mode is a crash (e.g., SIGSEGV or UBSan abort) that may produce empty stdout. Parsing/validating empty stdout first can mask the more useful signal/stderr. Instead, assert the spawned-process result by including `stdout` in the object passed to `toMatchObject` alongside `exitCode`, `signalCode`, and `stderr`, so stdout/stderr/signal all appear together in the failure diff (same pattern as `test/bundler/plugin-error-nested-throw.test.ts`).
Learnt from: robobun
Repo: oven-sh/bun PR: 30245
File: test/regression/issue/19650.test.ts:9-30
Timestamp: 2026-05-04T20:27:55.527Z
Learning: In oven-sh/bun test files, prefer using flat `test.concurrent.each([...])` when you want every parameterized test case to run fully concurrently across the entire parameter matrix. By contrast, `describe.each([...])` executes its describe blocks sequentially; while tests inside each describe block may be `test.concurrent`, concurrency is limited to within that block rather than across the whole matrix.
Learnt from: robobun
Repo: oven-sh/bun PR: 30118
File: test/js/node/zlib/zlib-writestate-detached.test.ts:78-90
Timestamp: 2026-05-04T20:37:57.348Z
Learning: In this Bun repository, do not flag code in Bun subprocess fixtures/tests where `console.log(...)` (or similar synchronous stdout/stderr writes) is immediately followed by `process.exit(n)` as a potential output-loss problem. Bun’s `process.exit()` flushes stdout and stderr synchronously before exiting (per the implementation in `src/runtime/node/process/exit.zig`), so `console.log` + `process.exit` is considered a safe, established Bun convention.
Learnt from: robobun
Repo: oven-sh/bun PR: 30268
File: test/js/bun/net/named-pipe-listen-error.test.ts:137-137
Timestamp: 2026-05-05T02:16:13.796Z
Learning: When reviewing JS/TS regex literals in Bun test files under `test/js/bun/`, don’t flag `\\` or `\b` as “bad escaping” if they’re intentionally matching literal backslashes used in Windows named-pipe paths (e.g., `\\.\pipe\name`). In JS regex literals, `\\` represents two literal backslashes, `\.` matches a literal dot, and `\b` (backslash-backslash-b) means a literal backslash followed by `b`, not the `\b` word-boundary escape.
Learnt from: robobun
Repo: oven-sh/bun PR: 30268
File: test/js/bun/net/named-pipe-listen-error.test.ts:137-137
Timestamp: 2026-05-05T02:16:13.255Z
Learning: When reviewing JavaScript/TypeScript regex literals, treat `\b` as an escaped backslash followed by `b` (i.e., it matches a literal backslash and then `b`), not the regex word-boundary metacharacter. The word-boundary metacharacter is an unescaped `\b` in the source code (i.e., `\b` in the pattern string/literal syntax), which has word-boundary semantics.
So: do not flag `\b` inside a regex as a word-boundary issue by default. Only flag `\b` when the intent is to match a literal backslash+`b` and word-boundary semantics would be incorrect. Example: `/^\\\.\\pipe\\/` (as written) matches the Windows named-pipe prefix `\\.\pipe\`.
Learnt from: robobun
Repo: oven-sh/bun PR: 30284
File: test/cli/test/path-ignore-patterns.test.ts:467-495
Timestamp: 2026-05-05T15:02:03.877Z
Learning: In oven-sh/bun test files under `test/**/*.test.ts`, when verifying that a test was NOT executed (for example, it was filtered out by `pathIgnorePatterns`), assert the absence of the test name string (e.g., `expect(stderr).not.toContain("explicit test")`) rather than asserting that the filename is absent. Bun may echo the filename in its `"The following filters did not match any test files:"` error output even when no tests ran, so filename-based assertions can be misleading.
Learnt from: robobun
Repo: oven-sh/bun PR: 30306
File: test/js/web/fetch/blob-write.test.ts:88-96
Timestamp: 2026-05-06T01:36:05.893Z
Learning: TempDir must be invoked with two arguments in test harness code: basename: string and filesOrAbsolutePathToCopyFolderFrom: DirectoryTree | string. Calls like tempDir("foo") should be flagged as invalid. tempDirWithFiles("name", {}) is a permitted pattern in existing tests (e.g., test/js/web/fetch/blob-write.test.ts line 55) when the result is assigned with const (not using) and consistent with the file's conventions. Apply this rule to test files across the repository (oven-sh/bun), and do not flag compliant const-based patterns that follow the established usage.
Learnt from: robobun
Repo: oven-sh/bun PR: 30350
File: test/cli/test/bun-test.test.ts:1319-1324
Timestamp: 2026-05-07T06:52:44.159Z
Learning: In oven-sh/bun TypeScript test files under `test/**/*.test.ts`, when the test constructs the snapshot input by intentionally `.filter()`-ing raw stderr to only the reporter-generated status/output lines (e.g., lines matching `/^\((pass|fail|skip|todo)\)/`, `^ ...` explanation lines, and `AssertionError:` lines), do not require `normalizeBunSnapshot` for that snapshot. In this design, the `.filter()` is what stabilizes the snapshot across `Execution.Result` variants; adding `normalizeBunSnapshot` would unnecessarily retain extra output (stack traces, repeated failures block, summaries), making snapshots ~3x larger and more fragile. Accept the local convention of small ad-hoc `.replace()` regex normalization for volatile timing fragments (e.g., stripping `[{d}ms]` and `after {d}ms` timeout text) where applied consistently within the same test suite.
Learnt from: jgoyvaerts
Repo: oven-sh/bun PR: 30410
File: test/js/bun/http/bun-serve-routes.test.ts:721-745
Timestamp: 2026-05-08T20:24:48.518Z
Learning: For this repo’s Bun/CLI tests under `test/js/bun/**`, follow the rule from `CLAUDE.md`: do not add explicit per-test timeouts (e.g., the 3rd argument to `test()`), including in performance/timing or scaling regression tests. Bun already applies its own timeouts, and adding per-test timeouts will likely interfere with the intended measurement. Only suggest adding explicit timeouts if the target file already uses them and they are explicitly required for correctness. The known exceptions are `test/js/bun/ffi/cc.test.ts` and `test/js/bun/glob/leak.test.ts` (RSS-leak, subprocess-heavy tests where timeouts may be necessary).
Learnt from: robobun
Repo: oven-sh/bun PR: 30414
File: test/js/bun/util/throw-bad-toPrimitive.test.ts:17-17
Timestamp: 2026-05-09T01:26:42.041Z
Learning: In oven-sh/bun test files under test/js/bun/**, enforce `bunExe()` + `-e` only for short inline one-liners (where the subprocess entry point is a single-string expression). If the subprocess entry-point is a fixture file (i.e., the entry point requires module-level `import` declarations and/or references `import.meta.dir`), use the established fixture pattern instead: `[bunExe(), path.join(import.meta.dir, "fixture.ts")]`. Do not flag this fixture pattern as a guideline violation (it matches existing usage across the test suite).
Learnt from: majiayu000
Repo: oven-sh/bun PR: 25687
File: test/bundler/issue-25675.test.ts:1-4
Timestamp: 2026-05-16T17:15:07.036Z
Learning: For Bun bundler tests, if a test file imports or uses `itBundled` / `expectBundled`, it must live under `./test/bundler/` (e.g., `test/bundler/**`). These helpers include a runtime guard that checks the call stack for `test/bundler/` and will throw with “All bundler tests must be placed in ./test/bundler/…”. Do not suggest moving such tests to `test/regression/…`, even for issue-specific/regression cases, because they will fail at runtime.
Learnt from: robobun
Repo: oven-sh/bun PR: 30936
File: test/bundler/transpiler/runtime-transpiler.test.ts:225-225
Timestamp: 2026-05-17T19:03:05.577Z
Learning: This repo (oven-sh/bun) does not enforce Biome lint rules in CI because there is no root Biome config (`biome.json` or `.biome*`). Therefore, during code review do not suggest adding `// biome-ignore` (or similar) suppression comments for Biome rule violations.
Additionally, in test files under `test/bundler/transpiler/`, do not “fix” switch-case code by wrapping intentionally-bare (unwrapped) `const` declarations in `{}` blocks when the test is specifically asserting TDZ/const-inlining behavior across sibling cases (e.g., regression tests like issue `#30932`). Adding a `{}` block can interfere with the const-prefix inliner and the single-use substitution pass, causing the test to miss the intended failure mode.
Learnt from: robobun
Repo: oven-sh/bun PR: 30284
File: test/cli/test/path-ignore-patterns.test.ts:343-375
Timestamp: 2026-05-21T07:56:03.036Z
Learning: In oven-sh/bun test files (Bun test), both `test.each` and `describe.each` are acceptable idioms for parameterized tests. Do not treat `test.each` as a guideline violation in favor of `describe.each`. Use `test.each` when each parameter entry corresponds to a single test body and no nested `test()` blocks are needed; use `describe.each` when you want grouped/structured test suites per parameter set.
Learnt from: robobun
Repo: oven-sh/bun PR: 31201
File: scripts/strip-long-rs-comments.ts:72-74
Timestamp: 2026-05-22T05:32:25.972Z
Learning: In this repo (oven-sh/bun), .gitattributes enforces LF line endings for tracked files, so CR characters from CRLF inputs should not be present. When reviewing TypeScript code that reads text and splits lines (e.g., using `split("\n")`), don’t flag CRLF/"trailing `\r`" concerns as issues, since tracked inputs are expected to contain only `\n` line endings.
Learnt from: robobun
Repo: oven-sh/bun PR: 31270
File: test/js/bun/css/nested-vendor-prefix-duplication.test.ts:113-120
Timestamp: 2026-05-23T14:52:47.580Z
Learning: In Bun/JS test files under `test/js/bun/**`, when a test spawns a subprocess and then reads an output file that the subprocess is supposed to generate, assert the subprocess result (both `exitCode` and `stderr`) together *before* attempting to read the output file. Prefer a combined assertion like `expect({ exitCode, stderr }).toEqual({ exitCode: 0, stderr: "" })` so that failures in `exitCode`/`stderr` surface clearly and don’t get masked by a subsequent “file not found” when the output file was never produced. This is an intentional exception to any general guideline that defers `exitCode` assertions until after filesystem reads.
Learnt from: robobun
Repo: oven-sh/bun PR: 31273
File: test/js/bun/jsonc/jsonc.test.ts:195-195
Timestamp: 2026-05-23T15:10:12.956Z
Learning: In Bun test files under `test/js/bun/**`, avoid adding explicit per-test timeouts except for pathological-input performance regression tests that run a subprocess with a `killSignal: "SIGKILL"` (e.g., tests that validate worst-case/slow inputs under debug+ASAN). For these tests, add an explicit outer test timeout (e.g., `90_000`) that is larger than the subprocess `timeout` option. The subprocess `timeout` is the real hang guard; the outer timeout is only a safety margin to prevent premature failures on slow CI lanes.
Learnt from: robobun
Repo: oven-sh/bun PR: 31514
File: test/js/sql/sqlite-sql.test.ts:5155-5163
Timestamp: 2026-05-28T17:06:44.390Z
Learning: When writing/updating tests that use `bun:sqlite` (oven-sh/bun) to round-trip the Unicode code point `\uFFFE`, account for SQLite’s bind-time behavior: SQLite drops `\uFFFE` during `sqlite3_bind_text16` UTF-16 → UTF-8 conversion, so the stored value becomes zero bytes and reads back as an empty string (`""`). Therefore, tests asserting round-trip behavior of `\uFFFE` should expect `""` (not `"\uFFFE"`). Do not change the expectation or framing to treat `\uFFFE` as preserved or leniently replaced—this is explicitly a SQLite-level drop.
Learnt from: robobun
Repo: oven-sh/bun PR: 31661
File: test/cli/run/env.test.ts:598-598
Timestamp: 2026-06-01T17:43:01.365Z
Learning: In Bun test files, when asserting that a subprocess produced no stderr (e.g., `expect(stderr).toBe("")`), do not add noise-filtering like `.filter(line => !line.startsWith("WARNING: ASAN interferes"))`. After PR `#30412`, Bun subprocesses no longer emit this ASAN startup warning across build variants (debug/ASAN/release), so the plain `toBe("")` assertion is correct for all CI configurations.
Learnt from: robobun
Repo: oven-sh/bun PR: 31661
File: test/cli/run/env.test.ts:598-600
Timestamp: 2026-06-01T17:43:14.469Z
Learning: In Bun test files under `test/**/*.test.ts`, when you spawn a subprocess and expect it to produce **empty stderr**, it’s acceptable to assert stderr unconditionally with `expect(result.stderr.toString('utf8')).toBe('')` before asserting `expect(result.exitCode).toBe(0)`. This avoids checking stderr twice while still showing stderr in the failure diff if stderr is non-empty. Use the conditional pattern (assert stderr only when `result.exitCode !== 0`) when stderr may include known-benign output that is only acceptable under certain failure/special cases (e.g., ASAN startup noise or other stderr exemptions).
Learnt from: robobun
Repo: oven-sh/bun PR: 31694
File: test/js/node/fs/fs-path-length.test.ts:168-170
Timestamp: 2026-06-02T09:34:04.212Z
Learning: In bun:test files, do not flag `expect(async () => await somePromise).toThrow("message")` as incorrect. bun:test’s `.toThrow(...)` supports async functions by inspecting the returned promise; a rejecting async fn with a matching message should pass and a non-matching message should fail. The alternative `await expect(promise).rejects.toThrow(...)` is also valid, but it is not required for bun:test.
Learnt from: EffortlessSteven
Repo: oven-sh/bun PR: 31729
File: test/js/bun/util/arraybuffersink.test.ts:66-123
Timestamp: 2026-06-02T20:41:52.089Z
Learning: For oven-sh/bun tests covering SharedArrayBuffer/resizable-ArrayBuffer snapshot boundary behavior in synchronous “sink” implementations (e.g., `ArrayBufferSink`, and similarly `FileSink` and `ResumableSink`), avoid using concurrency/worker-based mutation after `write()` returns to validate snapshot correctness. Since `ArrayBufferSink.write(chunk)` is fully synchronous (bytes are already copied into the sink buffer before it returns), post-write mutation will pass for both old and new code and does not prove the fix; race-based Worker tests also tend to be timing/Atomics-sensitive and are considered flaky in this repo. Instead, follow the pattern in `test/js/bun/util/arraybuffersink.test.ts`: use guard bytes around the view (e.g., `0xff`) and assert that `sink.end()` output contains only the exact intended view range (no data outside the view), which validates the snapshot boundary without any concurrency.
Learnt from: EffortlessSteven
Repo: oven-sh/bun PR: 31729
File: test/js/bun/s3/s3.test.ts:1805-1870
Timestamp: 2026-06-02T20:42:36.426Z
Learning: For Bun JS tests covering S3/“sink” behavior that copies SharedArrayBuffer/resizable-ArrayBuffer bytes into owned storage before `write()` returns, don’t rely on post-dispatch mutation to prove the UB fix: a post-write mutation will land after the relevant read in both the old (UB) and new (safe snapshot) cases. Instead, write behavior-preserving tests that validate the uploaded view range precisely (e.g., the slice boundaries are exactly correct and no guard/extra bytes leak), demonstrating the snapshot captured the intended slice—without attempting timing-sensitive concurrent Worker mutation races.
cc() callers nest symbol definitions under `options.symbols`, unlike dlopen/linkSymbols which take them at the top level. cc() read argument definitions from `options[key]`, so a "function"/"callback" argument was never passed through FFIBuilder. The unwrapped JSCallback object then reached native code as the callback pointer and the native call jumped to a wild address (SIGSEGV). Read definitions from `options.symbols[key]` so function arguments are wrapped (JSCallback -> pointer). Adds a cc.test.ts regression test.
EffortlessSteven
force-pushed
the
claude/ffi-cc-jscallback-arg
branch
from
June 7, 2026 05:42
e468659 to
2d69164
Compare
Collaborator
|
Closing this since #35246 (bun:ffi: use the engine-native FFI when available) merged and covers the same ground. Thank you @EffortlessSteven for the PR — if there's a piece of this that #35246 didn't pick up, please say so and we'll take another look. (This comment was written by Claude, on behalf of the Bun team.) |
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 this does
Bun.ffi.cc()crashes (SIGSEGV) when a"function"argument is given aJSCallback. Thecc()wrapper loop read arg definitions fromoptions[key]instead ofoptions.symbols[key], so theJSCallbackskippedFFIBuilderand reached native code as a raw callback pointer.Fix:
cc()reads definitions fromoptions.symbols[key], so a function argument is wrapped (JSCallbackto native pointer).dlopenandlinkSymbolstake symbols at the top level and are unchanged.Verification
Real red/green: unpatched crashes in the native call; patched returns the callback's value and rejects after
close().cc.test.tsfunction-argJSCallbackregressionokcc.test.tsReview map
src/js/bun/ffi.ts:cc()reads function-arg defs fromoptions.symbols[key]test/js/bun/ffi/cc.test.ts: subprocess C-trampoline regression, SIGKILL-bounded