fix(shell): prevent double-close of fd when using &> redirect with builtins

[dylan-conway]

Summary

• Fix double-close of file descriptor when using &> redirect with shell builtin commands
• Add dupeRef() helper for cleaner reference counting semantics
• Add tests for &> and &>> redirects with builtins

Test plan

• Added tests in test/js/bun/shell/file-io.test.ts that reproduce the bug
• All file-io tests pass

The Bug

When using &> to redirect both stdout and stderr to the same file with a shell builtin command (e.g., pwd &> file.txt), the code was creating two separate IOWriter instances that shared the same file descriptor. When both IOWriters were destroyed, they both tried to close the same fd, causing an EBADF (bad file descriptor) error.

import { $ } from "bun";
await $`pwd &> output.txt`; // Would crash with EBADF

The Fix

1. Share a single IOWriter between stdout and stderr when both are redirected to the same file, with proper reference counting
2. Rename refSelf to dupeRef for clarity across IOReader, IOWriter, CowFd, and add it to Blob for consistency
3. Fix the Body.Value blob case to also properly reference count when the same blob is assigned to multiple outputs

🤖 Generated with Claude Code

[robobun]

^{Updated 3:58 PM PT - Dec 17th, 2025}

@dylan-conway, your commit 2eebcbf is building: #33467

[coderabbitai]

Walkthrough

This pull request refactors reference counting semantics across the shell subsystem by renaming refSelf() methods to dupeRef() in multiple IO-related types. It introduces a new Blob.dupeRef() method, updates all usages to reference the new method names, and adds test coverage for stdout/stderr redirection to the same file using the &> operator.

Changes

Cohort / File(s)	Summary
Method signature refactoring src/shell/IOReader.zig, src/shell/IOWriter.zig, src/shell/interpreter.zig	Renamed public methods from refSelf() to dupeRef() across IOReader, IOWriter, and CowFd types. Method implementations remain unchanged, only symbol names updated.
Reference duplication in shell execution src/shell/Builtin.zig	Introduced new Blob.dupeRef() method and refactored redirect handling to use dupeRef() for stdin/stdout/stderr assignments. Updated redirect logic with early returns and dedicated redirect_writer objects for proper reference lifetime management.
Reference duplication consumers src/shell/IO.zig, src/shell/subproc.zig	Updated to use dupeRef() instead of refSelf() when assigning writer references to subprocess IO structures and capture writers.
Test coverage test/js/bun/shell/file-io.test.ts	Added test block for &> and &>> redirect operators to validate correct handling of shared file descriptors when redirecting stdout and stderr to the same file.

Suggested reviewers

• zackradisic
• pfgithub

Pre-merge checks

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main fix: preventing double-close of file descriptors when using &> redirect with builtin commands, which is the core bug this PR addresses.
Description check	✅ Passed	The description covers both required template sections: it explains what the PR does (fixes double-close bug and adds dupeRef helper) and how it was verified (added tests that reproduce the bug, all tests pass).

---

📜 Recent 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.

📥 Commits

Reviewing files that changed from the base of the PR and between ffd2240 and 2eebcbf.

📒 Files selected for processing (7)

• src/shell/Builtin.zig (4 hunks)
• src/shell/IO.zig (1 hunks)
• src/shell/IOReader.zig (1 hunks)
• src/shell/IOWriter.zig (1 hunks)
• src/shell/interpreter.zig (1 hunks)
• src/shell/subproc.zig (1 hunks)
• test/js/bun/shell/file-io.test.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (7)

src/**/*.{cpp,zig}

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

src/**/*.{cpp,zig}: Use bun bd or bun run build:debug to build debug versions for C++ and Zig source files; creates debug build at ./build/debug/bun-debug
Run tests using bun bd test <test-file> with the debug build; never use bun test directly as it will not include your changes
Execute files using bun bd <file> <...args>; never use bun <file> directly as it will not include your changes
Enable debug logs for specific scopes using BUN_DEBUG_$(SCOPE)=1 environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required

Files:

• src/shell/IOWriter.zig
• src/shell/interpreter.zig
• src/shell/subproc.zig
• src/shell/IOReader.zig
• src/shell/Builtin.zig
• src/shell/IO.zig

src/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

Use bun.Output.scoped(.${SCOPE}, .hidden) for creating debug logs in Zig code

Implement core functionality in Zig, typically in its own directory in src/

src/**/*.zig: Private fields in Zig are fully supported using the # prefix: struct { #foo: u32 };
Use decl literals in Zig for declaration initialization: const decl: Decl = .{ .binding = 0, .value = 0 };
Prefer @import at the bottom of the file (auto formatter will move them automatically)

Files:

• src/shell/IOWriter.zig
• src/shell/interpreter.zig
• src/shell/subproc.zig
• src/shell/IOReader.zig
• src/shell/Builtin.zig
• src/shell/IO.zig

**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)

**/*.zig: Expose generated bindings in Zig structs using pub const js = JSC.Codegen.JS<ClassName> with trait conversion methods: toJS, fromJS, and fromJSDirect
Use consistent parameter name globalObject instead of ctx in Zig constructor and method implementations
Use bun.JSError!JSValue return type for Zig methods and constructors to enable proper error handling and exception propagation
Implement resource cleanup using deinit() method that releases resources, followed by finalize() called by the GC that invokes deinit() and frees the pointer
Use JSC.markBinding(@src()) in finalize methods for debugging purposes before calling deinit()
For methods returning cached properties in Zig, declare external C++ functions using extern fn and callconv(JSC.conv) calling convention
Implement getter functions with naming pattern get<PropertyName> in Zig that accept this and globalObject parameters and return JSC.JSValue
Access JavaScript CallFrame arguments using callFrame.argument(i), check argument count with callFrame.argumentCount(), and get this with callFrame.thisValue()
For reference-counted objects, use .deref() in finalize instead of destroy() to release references to other JS objects

In Zig code, be careful with allocators and use defer for cleanup

Files:

• src/shell/IOWriter.zig
• src/shell/interpreter.zig
• src/shell/subproc.zig
• src/shell/IOReader.zig
• src/shell/Builtin.zig
• src/shell/IO.zig

test/**/*.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

test/**/*.{js,ts,jsx,tsx}: Write tests as JavaScript and TypeScript files using Jest-style APIs (test, describe, expect) and import from bun:test
Use test.each and data-driven tests to reduce boilerplate when testing multiple similar cases

Files:

• test/js/bun/shell/file-io.test.ts

test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}

📄 CodeRabbit inference engine (test/CLAUDE.md)

test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Use bun:test with files that end in *.test.{ts,js,jsx,tsx,mjs,cjs}
Do not write flaky tests. Never wait for time to pass in tests; always wait for the condition to be met instead of using an arbitrary amount of time
Never use hardcoded port numbers in tests. Always use port: 0 to get a random port
Prefer concurrent tests over sequential tests using test.concurrent or describe.concurrent when multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
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
Use -e flag for single-file tests when spawning Bun processes
Use tempDir() from harness to create temporary directories with files for multi-file tests instead of creating files manually
Prefer async/await over callbacks in tests
When callbacks must be used and it's just a single callback, use Promise.withResolvers to create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
Use Buffer.alloc(count, fill).toString() instead of 'A'.repeat(count) to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Use describe blocks for grouping related tests
Always use await using or using to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Always check exit codes and test error scenarios in error tests
Use describe.each() for parameterized tests
Use toMatchSnapshot() for snapshot testing
Use beforeAll(), afterEach(), beforeEach() for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup in afterEach()

Files:

• test/js/bun/shell/file-io.test.ts

**/*.test.ts?(x)

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.test.ts?(x): Never use bun test directly - always use bun bd test to run tests with debug build changes
For single-file tests, prefer -e flag over tempDir
For multi-file tests, prefer tempDir and Bun.spawn over single-file tests
Use normalizeBunSnapshot to normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
Use tempDir from harness to create temporary directories - do not use tmpdirSync or fs.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 use setTimeout in tests; instead await the condition to be met
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
Test files must end with .test.ts or .test.tsx
Avoid shell commands like find or grep in tests - use Bun's Glob and built-in tools instead

Files:

• test/js/bun/shell/file-io.test.ts

test/**/*.test.ts?(x)

📄 CodeRabbit inference engine (CLAUDE.md)

Always use port: 0 in tests - do not hardcode ports or use custom random port number functions

Files:

• test/js/bun/shell/file-io.test.ts

🧠 Learnings (15)

📓 Common learnings

Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.

📚 Learning: 2025-11-11T22:55:08.547Z

Learnt from: pfgithub
Repo: oven-sh/bun PR: 24571
File: src/css/values/url.zig:97-116
Timestamp: 2025-11-11T22:55:08.547Z
Learning: In Zig 0.15, the standard library module `std.io` was renamed to `std.Io` (with capital I). Code using `std.Io.Writer.Allocating` and similar types is correct for Zig 0.15+.

Applied to files:

• src/shell/IOWriter.zig
• src/shell/IOReader.zig
• src/shell/IO.zig

📚 Learning: 2025-11-24T18:35:39.205Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-11-24T18:35:39.205Z
Learning: Applies to **/js_*.zig : Implement proper memory management with reference counting using `ref()`/`deref()` in JavaScript bindings

Applied to files:

• src/shell/IOWriter.zig
• src/shell/Builtin.zig

📚 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/js/bun/shell/file-io.test.ts

📚 Learning: 2025-10-26T01:32:04.844Z

Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.

Applied to files:

• test/js/bun/shell/file-io.test.ts
• src/shell/Builtin.zig

📚 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/shell/file-io.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/shell/file-io.test.ts

📚 Learning: 2025-11-24T18:35:08.612Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/**/*.test.ts : Use `dev.write()`, `dev.patch()`, and `dev.delete()` to mutate the filesystem instead of `node:fs` APIs, as dev server functions are hooked to wait for hot-reload and notify clients

Applied to files:

• test/js/bun/shell/file-io.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) : Avoid shell commands like `find` or `grep` in tests - use Bun's Glob and built-in tools instead

Applied to files:

• test/js/bun/shell/file-io.test.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Always check exit codes and test error scenarios in error tests

Applied to files:

• test/js/bun/shell/file-io.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 single-file tests, prefer `-e` flag over `tempDir`

Applied to files:

• test/js/bun/shell/file-io.test.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/js/node/test/{parallel,sequential}/*.js : For test/js/node/test/{parallel,sequential}/*.js files without a .test extension, use `bun bd <file>` instead of `bun bd test <file>` since these expect exit code 0 and don't use bun's test runner

Applied to files:

• test/js/bun/shell/file-io.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/shell/file-io.test.ts

📚 Learning: 2025-11-24T18:35:50.422Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Applies to test/cli/**/*.{js,ts,jsx,tsx} : When testing Bun as a CLI, use the `spawn` API from `bun` with the `bunExe()` and `bunEnv` from `harness` to execute Bun commands and validate exit codes, stdout, and stderr

Applied to files:

• test/js/bun/shell/file-io.test.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `-e` flag for single-file tests when spawning Bun processes

Applied to files:

• test/js/bun/shell/file-io.test.ts

🔇 Additional comments (11)

src/shell/interpreter.zig (1)

180-183: LGTM! Consistent API rename.

The rename from refSelf to dupeRef clarifies the reference-counting semantics and aligns with the project-wide refactoring. The implementation correctly increments the reference count and returns the same pointer.

src/shell/IOWriter.zig (1)

73-76: LGTM! API rename aligns with project standards.

The dupeRef method name better conveys the intent of duplicating a reference with proper ref-counting. Implementation is correct.

src/shell/IOReader.zig (1)

33-36: LGTM! Consistent with the dupeRef pattern.

The rename maintains consistency across IOReader, IOWriter, and CowFd. The reference-counting logic is preserved.

src/shell/IO.zig (1)

173-173: LGTM! Correctly uses the renamed API.

The call to dupeRef() properly duplicates the writer reference for subprocess stdio handling.

src/shell/subproc.zig (1)

1124-1124: LGTM! Correct usage of dupeRef for capture writer.

The change properly duplicates the capture writer reference using the new API.

test/js/bun/shell/file-io.test.ts (1)

144-157: LGTM! Test coverage validates the fix.

The new test block appropriately covers the bug scenario where &> redirects both stdout and stderr to the same file with builtin commands. The inline comments provide valuable context about the double-close issue that was fixed.

src/shell/Builtin.zig (5)

267-270: LGTM! New dupeRef method for Blob.

The dupeRef helper follows the same pattern as other reference-counted types in the shell subsystem, maintaining consistency.

---

348-348: LGTM! Properly uses dupeRef for stdin initialization.

The change from direct assignment to dupeRef() ensures proper reference counting for the stdin file descriptor.

---

352-357: LGTM! Reference counting for stdout and stderr initialization.

Both stdout and stderr now correctly use dupeRef() to manage the writer reference lifecycle.

---

490-509: Excellent fix for the double-close bug!

This is the core fix for the issue described in the PR. The key improvements:

1. Early return guard (lines 490-492) avoids unnecessary work when no redirects exist
2. Single shared writer (lines 494-498) creates one IOWriter instance instead of two
3. Proper reference counting via dupeRef() (lines 503, 508) allows stdout and stderr to share the same writer
4. Guaranteed cleanup with defer redirect_writer.deref() (line 499) ensures the initial reference is released

This prevents the EBADF error that occurred when two IOWriter instances both attempted to close the same file descriptor.

---

543-565: LGTM! Consistent blob reference handling.

The blob redirection path follows the same reference-counting pattern:

• Early return when no redirects (lines 543-545)
• Create blob once and defer cleanup (line 551)
• Share via dupeRef() for stdin/stdout/stderr (lines 555, 560, 565)

This maintains consistency with the file descriptor redirection handling and prevents similar double-free issues.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}