Fix crash in `Bun.file().writer()` with invalid path/fd options #30277

claude · 2026-05-05T10:42:36Z

🟡 Nit: the try on this line can also early-return (e.g. { get highWaterMark() { throw 1 } }), in which case sink from line 2954 is leaked — there's no errdefer sink.deref(). Since this PR is already adding sink.deref() for the .err branch right below, an errdefer sink.deref(); after line 2954 would cover this path (and the existing manual sink.deref() calls could then be dropped). Pre-existing, but it's the same hardening this PR is doing.

Extended reasoning...

What the bug is

FileSink.init (FileSink.zig:553) heap-allocates the sink via bun.new with a ref count and increments live_count; it must be deref()'d to be freed. At Blob.zig:2954 the sink is created, but there is no errdefer sink.deref() afterward. The very next failure point that uses try is line 2977:

stream_start = try jsc.WebCore.streams.Start.fromJSWithTag(globalThis, arguments[0], .FileSink);

fromJSWithTag (streams.zig:74) returns bun.JSError!Start and internally does try value.fastGet(globalThis, .highWaterMark), try value.fastGet(globalThis, .path), try value.getTruthy(globalThis, "fd"), and try path.toSlice(...). Any of these propagate error.JSError if a JS getter throws or toSlice fails. When that happens, try returns from getWriter without ever calling sink.deref(), leaking the FileSink.

Step-by-step proof

Bun.file('/tmp/x').writer({ get highWaterMark() { throw 1 } })

Blob.zig:2954 — sink = FileSink.init(...) heap-allocates a FileSink, live_count += 1.

Blob.zig:2976 — arguments[0].isObject() is true.

Blob.zig:2977 — fromJSWithTag calls value.fastGet(globalThis, .highWaterMark), which invokes the throwing getter → returns error.JSError.

try propagates error.JSError out of getWriter. No errdefer runs for sink. The only defer is input_path.deinit() (line 2968).

sink is never deref'd → leaked.

Why existing code doesn't prevent it

The PR adds sink.deref() for the .err union variant returned by fromJSWithTag (line 2980), and there's an existing sink.deref() in the sink.start error branch (line 2995). But neither covers the Zig-error (error.JSError) path on line 2977, which bypasses the switch entirely via try.

Impact

Small heap leak per call, only triggerable with adversarial throwing getters on the options object. Not a correctness or crash issue. However, this PR is specifically hardening invalid-options handling in this exact block and explicitly adds sink.deref() one line below — so the omission on the adjacent try path is an incomplete fix of the same logical change.

Fix

Add errdefer sink.deref(); immediately after line 2954. This covers the try fromJSWithTag path and any future try-paths in this block. Optionally, the manual sink.deref() calls in the .err branches (lines 2980, 2995) could then become plain returns relying on the errdefer, though leaving them as-is is also fine since those branches don't return a Zig error.

coderabbitai · 2026-05-05T10:37:03Z

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Use tempDir for the new test instead of tmpdirSync.

Line 211 adds another tmpdirSync() call in a .test.ts file. Please use tempDir here so cleanup is deterministic and consistent with test conventions.

Proposed change

it.skipIf(isWindows)("writer() throws on invalid options instead of crashing", () => { - const file = Bun.file(path.join(tmpdirSync(), "test.txt")); + using dir = tempDir("filesink-invalid-options"); + const file = Bun.file(path.join(dir, "test.txt")); expect(() => file.writer({ path: 123 })).toThrow(expect.objectContaining({ code: "EINVAL" })); expect(() => file.writer({ fd: "not a number" })).toThrow(expect.objectContaining({ code: "EBADF" })); expect(() => file.writer({ fd: -9999.5 })).toThrow(expect.objectContaining({ code: "EBADF" })); });

-import { fileDescriptorLeakChecker, isPosix, isWindows, tmpdirSync } from "harness"; +import { fileDescriptorLeakChecker, isPosix, isWindows, tempDir, tmpdirSync } from "harness";

As per coding guidelines: test/**/*.test.ts: “Use tempDir from harness to create temporary directories; do not use tmpdirSync or fs.mkdtempSync.”

🤖 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/util/filesink.test.ts` around lines 210 - 215, Replace the tmpdirSync() call in the test block that verifies file.writer() error handling with the harness-provided tempDir() so the temporary directory is created and cleaned deterministically; locate the test named "writer() throws on invalid options instead of crashing" (uses isWindows and calls Bun.file(...).writer(...)) and change the path construction from path.join(tmpdirSync(), "test.txt") to use tempDir() (ensuring tempDir is available in the test scope per other tests) so it follows the test suite convention.

This test matches the existing convention in this file — every other test here uses tmpdirSync() (lines 47, 58, 199). More importantly, this test never creates any files: .writer() throws on the invalid options before any I/O happens, so there's nothing to clean up. (Also, the proposed diff is missing tempDir's required second argument.)

@robobun, you're right on all three points — the file-level convention is tmpdirSync, no files are ever created so there's nothing to clean up, and my proposed diff was indeed missing the second argument to tempDir. I'll withdraw the suggestion. Thanks for the clear explanation.

✏️ Learnings added

Learnt from: robobun Repo: oven-sh/bun PR: 30277 File: test/js/bun/util/filesink.test.ts:210-215 Timestamp: 2026-05-05T10:39:19.660Z Learning: In `test/js/bun/util/filesink.test.ts`, the established convention is to use `tmpdirSync()` (from `harness`) for temporary directories, not `tempDir`. Do not flag `tmpdirSync()` usage in this file as a guideline violation. Additionally, when a test calls `.writer()` solely to assert that it throws on invalid options (before any I/O occurs), no temporary directory cleanup is needed, making `tempDir` unnecessary.

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: 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: 29441 File: test/js/web/broadcastchannel/broadcast-channel-worker-gc.test.ts:10-14 Timestamp: 2026-04-18T10:36:45.033Z Learning: In oven-sh/bun test files that spawn subprocesses using `bunEnv`, suppress the known ASAN startup noise in the subprocess stderr before asserting it is empty. Use the repo’s established convention: split stderr into lines and filter with `.filter(line => !line.startsWith("WARNING: ASAN interferes"))`, then assert the remaining stderr lines are empty. Do not switch to an alternative like `str.replace(...)`; the filter-based approach is the repo convention. This is safe because `ZigGlobalObject.cpp` emits that warning via `std::call_once`, so at most one matching line appears per process.

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: 30230 File: test/js/bun/util/bun-file-fd-read.test.ts:109-117 Timestamp: 2026-05-04T06:28:25.345Z Learning: When reviewing oven-sh/bun JS tests that intentionally exercise Bun’s internal IO-thread behavior (e.g., the `io.zig` epoll registration path used by `ReadFile`/`WriteFile`), it’s acceptable to use `await Bun.sleep(N)` (like 100ms) as a timing window *only* when there is no JS-observable side effect that can be awaited directly (i.e., the relevant internal event has no JS-side condition to wait on). The test should still be considered correct if its semantics are validated across race outcomes (e.g., EPOLLERR/alternate recv path still preserves fail-before/pass-after behavior). Do not flag these `Bun.sleep(N)` calls as flaky/fragile when the author has empirically verified the behavior (multiple iterations) and also confirmed it reliably with `USE_SYSTEM_BUN=1` that the intended target path is exercised and the test fails correctly without the fix.

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\`.

-Original file line number
+Diff line change
@@ Expand Up / @@ -2974,8 +2974,20 @@ @@
         };
         if (arguments.len > 0 and arguments.ptr[0].isObject()) {
             stream_start = try jsc.WebCore.streams.Start.fromJSWithTag(globalThis, arguments[0], .FileSink);
-            stream_start.FileSink.input_path = input_path;
+            switch (stream_start) {
+                .err => |err| {
+                    sink.deref();
+                    return globalThis.throwValue(try err.toJS(globalThis));
+                },
+                .FileSink => |*file| {
+                    file.input_path.deinit();
+                    file.input_path = input_path;
+                },
+                else => {
+                    stream_start = .{ .FileSink = .{ .input_path = input_path } };
+                },
+            }
         }
         switch (sink.start(stream_start)) {
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -207,6 +207,13 @@ it("write result is not cumulative", async () => { @@
       await util.promisify(fs.close)(fd);
     });
+    it.skipIf(isWindows)("writer() throws on invalid options instead of crashing", () => {
+      const file = Bun.file(path.join(tmpdirSync(), "test.txt"));
+      expect(() => file.writer({ path: 123 })).toThrow(expect.objectContaining({ code: "EINVAL" }));
+      expect(() => file.writer({ fd: "not a number" })).toThrow(expect.objectContaining({ code: "EBADF" }));
+      expect(() => file.writer({ fd: -9999.5 })).toThrow(expect.objectContaining({ code: "EBADF" }));
+    });
     if (isWindows) {
       it("ENOENT, Windows", () => {
         expect(() => Bun.file("A:\\this-does-not-exist.txt").writer()).toThrow(
@@ Expand Down @@

Navigation Menu

Uh oh!

Fix crash in `Bun.file().writer()` with invalid path/fd options #30277

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Claude / Claude Code Review

claude Bot May 5, 2026

Uh oh!

coderabbitai Bot May 5, 2026 •

edited

Loading

Uh oh!

robobun May 5, 2026

Uh oh!

coderabbitai Bot May 5, 2026

Uh oh!

Uh oh!

Uh oh!

Fix crash in Bun.file().writer() with invalid path/fd options #30277

Uh oh!

Fix crash in Bun.file().writer() with invalid path/fd options #30277

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Claude / Claude Code Review

claude Bot May 5, 2026

Choose a reason for hiding this comment

What the bug is

Step-by-step proof

Why existing code doesn't prevent it

Impact

Fix

Uh oh!

coderabbitai Bot May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robobun May 5, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot May 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Fix crash in `Bun.file().writer()` with invalid path/fd options #30277

Fix crash in `Bun.file().writer()` with invalid path/fd options #30277

coderabbitai Bot May 5, 2026 •

edited

Loading