Repository navigation
Fix memory leaks & blocking syscall in Bun Shell - #23636
Conversation
|
Updated 6:12 PM PT - Oct 15th, 2025
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 23636That installs a local version of the PR into your bun-23636 --bun |
WalkthroughAdds memoryCost and estimatedSize metadata to JS API descriptors, exposes an estimatedSize extern in bindings/codegen, and implements memory-accounting (memoryCost/estimatedSize) across Zig shell AST, interpreter, IO, and env structures; updates collection accounting, sys networking logging, and adds tests exercising memory behavior. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (17)
src/bun.js/api/ParsedShellScript.classes.ts(1 hunks)src/bun.js/api/Shell.classes.ts(1 hunks)src/bun.js/bindings/ShellBindings.cpp(2 hunks)src/codegen/generate-classes.ts(1 hunks)src/collections/baby_list.zig(1 hunks)src/shell/EnvMap.zig(1 hunks)src/shell/EnvStr.zig(1 hunks)src/shell/IO.zig(2 hunks)src/shell/IOReader.zig(2 hunks)src/shell/IOWriter.zig(2 hunks)src/shell/ParsedShellScript.zig(2 hunks)src/shell/interpreter.zig(10 hunks)src/shell/shell.zig(14 hunks)src/sys.zig(2 hunks)test/js/bun/shell/bunshell.test.ts(1 hunks)test/js/bun/shell/shell-blocking-pipe.test.ts(1 hunks)test/js/bun/shell/shell-leak-args.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (16)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/shell/EnvMap.zigsrc/shell/IOReader.zigsrc/sys.zigsrc/shell/ParsedShellScript.zigsrc/shell/IOWriter.zigsrc/shell/IO.zigsrc/shell/EnvStr.zigsrc/shell/shell.zigsrc/shell/interpreter.zigsrc/collections/baby_list.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/shell/EnvMap.zigsrc/shell/IOReader.zigsrc/sys.zigsrc/shell/ParsedShellScript.zigsrc/shell/IOWriter.zigsrc/shell/IO.zigsrc/shell/EnvStr.zigsrc/shell/shell.zigsrc/shell/interpreter.zigsrc/collections/baby_list.zig
**/*.classes.ts
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
**/*.classes.ts: Define JavaScript class interfaces in .classes.ts using define({ name, constructor, JSType, finalize, proto: { ... } })
Use cache: true on properties that should use JSC WriteBarrier-backed caching
Files:
src/bun.js/api/Shell.classes.tssrc/bun.js/api/ParsedShellScript.classes.ts
src/**/*.classes.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Define JavaScriptCore class metadata in .classes.ts files to drive code generation
Files:
src/bun.js/api/Shell.classes.tssrc/bun.js/api/ParsedShellScript.classes.ts
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/ShellBindings.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/ShellBindings.cpp
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: When a JS class has a public constructor, implement Foo, FooPrototype, and FooConstructor (JSDestructibleObject, JSNonFinalObject, InternalFunction)
Define class properties using HashTableValue arrays in C++ bindings
Add iso subspaces for classes with C++ fields
Cache structures in ZigGlobalObject for C++-backed classes
Files:
src/bun.js/bindings/ShellBindings.cpp
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/shell/shell-leak-args.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/shell/shell-leak-args.test.ts
test/js/bun/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/shell/shell-leak-args.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/shell/shell-leak-args.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/shell/shell-leak-args.test.ts
test/js/bun/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place Bun-specific API tests under test/js/bun/
Files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/shell/shell-leak-args.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/shell/shell-leak-args.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/shell/shell-leak-args.test.ts
test/js/{bun,node}/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests by module under
/test/js/bun/and/test/js/node/
Files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/shell/shell-leak-args.test.ts
🧠 Learnings (11)
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/shell/shell-leak-args.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Applied to files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/shell/shell-leak-args.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : 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/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-10-12T02:22:34.349Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.349Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/js/bun/shell/shell-blocking-pipe.test.ts
📚 Learning: 2025-10-12T02:22:34.349Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.349Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use utilities like `describe.each`, `toMatchSnapshot`, and lifecycle hooks (`beforeAll`, `beforeEach`, `afterEach`) and track resources for cleanup
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer running tests via bun bd test <file> and use provided harness utilities (bunEnv, bunExe, tempDir)
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-10-12T02:22:34.349Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.349Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Import common utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, platform helpers, GC helpers)
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-10-12T02:22:34.349Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.349Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending with `*.test.{ts,js,jsx,tsx,mjs,cjs}`
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/bake-harness.ts : Provide and maintain shared test utilities: devTest, prodTest, devAndProductionTest, Dev, Client, and helpers in the harness
Applied to files:
test/js/bun/shell/bunshell.test.ts
🧬 Code graph analysis (1)
test/js/bun/shell/shell-leak-args.test.ts (2)
src/node-fallbacks/buffer.js (1)
Buffer(136-145)scripts/utils.mjs (1)
$(143-161)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (22)
src/sys.zig (1)
2108-2108: Verify format specifier for DebugTimer.The logging statements use
({f})to formatdebug_timer, but similar code in thewrite()function at line 1664 uses({})instead. This inconsistency may cause incorrect formatting or runtime errors ifDebugTimerdoesn't support the{f}specifier.Please verify the correct format specifier for
bun.Output.DebugTimer. If{}is correct, apply this diff:- syslog("send({}, {d}) = {s} ({f})", .{ fd, buf.len, err.err.name(), debug_timer }); + syslog("send({}, {d}) = {s} ({})", .{ fd, buf.len, err.err.name(), debug_timer });- syslog("send({}, {d}) = {d} ({f})", .{ fd, buf.len, rc, debug_timer }); + syslog("send({}, {d}) = {d} ({})", .{ fd, buf.len, rc, debug_timer });- syslog("send({}, {d}) = {s} ({f})", .{ fd, buf.len, err.err.name(), debug_timer }); + syslog("send({}, {d}) = {s} ({})", .{ fd, buf.len, err.err.name(), debug_timer });- syslog("send({}, {d}) = {d} ({f})", .{ fd, buf.len, rc, debug_timer }); + syslog("send({}, {d}) = {d} ({})", .{ fd, buf.len, rc, debug_timer });Also applies to: 2112-2112, 2122-2122, 2126-2126
src/collections/baby_list.zig (1)
366-368: LGTM! Correct memory cost calculation.The change from returning just the capacity count to returning the actual byte size (
this.cap * @sizeOf(Type)) is correct. This aligns with the broader memory accounting enhancements in this PR where the GC needs to know the actual memory footprint in bytes, not just the element count.src/bun.js/api/ParsedShellScript.classes.ts (1)
12-13: LGTM! Memory accounting metadata enabled.Adding
memoryCost: trueandestimatedSize: trueto the class definition enables proper memory accounting for ParsedShellScript instances, which is essential for the GC to track memory usage accurately.src/shell/EnvMap.zig (1)
29-38: LGTM! Comprehensive memory cost calculation.The memoryCost implementation correctly accounts for:
- The EnvMap struct overhead
- Array storage for key/value pointers
- Individual string costs via EnvStr.memoryCost()
This provides an accurate total memory footprint for the environment map.
test/js/bun/shell/shell-blocking-pipe.test.ts (1)
9-33: LGTM! Tests verify non-blocking pipe behavior.The tests effectively verify that large writes through pipes don't block the main thread. Both test cases exercise different shell template syntax patterns, and the heap snapshot generation exercises the new memoryCost/estimatedSize fields.
src/shell/IOWriter.zig (2)
78-85: LGTM! Comprehensive memory cost calculation.The memoryCost method correctly aggregates memory costs from all IOWriter components including the buffer, Windows-specific buffer, writers collection, and the writer implementation itself.
168-174: LGTM! Fixes blocking sendto on macOS.The socket flag logic now accounts for macOS-specific behavior where
sendto()withMSG_NOWAITcan block. The fix ensures that on macOS, the socket flag is only set when the socket is explicitly non-blocking.This addresses the blocking syscall issue mentioned in the PR objectives.
src/shell/IOReader.zig (2)
38-43: LGTM! Correct memory cost calculation.The memoryCost method properly aggregates the IOReader struct size, buffer size, and the cost from the readers collection.
234-240: LGTM! Memory cost for child pointers.The implementation correctly returns the size for Cat builtin instances. The TODO comment at line 236 suggests potential future expansion, but the current implementation is complete for the existing ChildPtrRaw union which only contains Cat.
src/shell/IO.zig (4)
12-18: LGTM! IO memory cost calculation.The memoryCost method correctly aggregates memory costs from the IO struct itself and all three stdio streams (stdin, stdout, stderr).
88-93: LGTM! InKind memory cost calculation.The memoryCost implementation correctly returns the reader's memory cost for fd variants and 0 for ignore variants.
104-110: LGTM! OutKind fd memory cost calculation.The memoryCost method properly accounts for both the writer's memory cost and the optional captured buffer's cost.
119-125: LGTM! OutKind memory cost calculation.The memoryCost implementation correctly returns the fd's memory cost for fd variants and 0 for pipe/ignore variants.
src/shell/interpreter.zig (2)
363-373: Bufio.memoryCost looks correctDelegates to underlying ByteList memoryCost for owned/borrowed. LGTM.
1242-1243: Leak fix: ShellArgs deinit in finalizerGood catch. This resolves the ShellArgs lifetime mismatch when the JS wrapper is GC’d.
src/shell/shell.zig (1)
319-326: MemoryCost coverage additions look solid; one consistency nitThe new memoryCost impls broadly traverse children and account container storage. Aside from the CondExpr base-size nit, these look good. If you want full consistency, consider either:
- always include
@sizeOf(T)for node structs, or- exclude base sizes across the board.
Not blocking; current approach is acceptable if “estimated” is the goal.
Also applies to: 331-338, 359-376, 637-645, 671-677, 687-693, 698-705, 714-736, 797-803, 811-824, 910-915, 924-930, 1033-1037, 1055-1062, 1070-1082, 4230-4255, 4290-4297
test/js/bun/shell/bunshell.test.ts (1)
15-16: LGTM: Memory instrumentation added for test suite.The lifecycle hooks log RSS memory usage before and after the test suite runs, providing visibility into memory consumption patterns. This aligns with the PR's memory leak fixes.
src/codegen/generate-classes.ts (1)
1371-1373: LGTM: Improved extern declaration placement.Moving the
estimatedSizeextern declaration from the constructor implementation to the class header generation is cleaner and follows conventional C++ patterns. The extern is only emitted whenobj.estimatedSizeis truthy, which is correct.src/bun.js/api/Shell.classes.ts (1)
14-15: LGTM: Memory cost tracking enabled for ShellInterpreter.Adding
memoryCostandestimatedSizemetadata enables proper GC memory accounting for ShellInterpreter instances. This is consistent with the PR's goal to fix memory leaks.src/bun.js/bindings/ShellBindings.cpp (1)
27-28: LGTM: Memory reporting integrated correctly.The memory reporting is done at the right time (after instance creation, before return) and properly reports the estimated size to the VM heap for GC accounting.
test/js/bun/shell/shell-leak-args.test.ts (2)
28-47: LGTM: Comprehensive memory leak test for shell argv with awaited commands.The test properly measures RSS before and after executing shell commands with large buffers. The 250 MB threshold is reasonable for detecting leaks when running 200 iterations with 1 MB buffers. The warmup period and test structure match the existing pattern.
Based on learnings
49-68: LGTM: Comprehensive memory leak test for non-awaited shell commands.This test covers the non-awaited command execution path, providing complementary coverage to the awaited test. The structure and thresholds are consistent with the awaited test above.
Based on learnings
|
|
||
| #include "ZigGeneratedClasses.h" | ||
|
|
||
| extern "C" SYSV_ABI size_t ShellInterpreter__estimatedSize(void* ptr); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the extern function is implemented.
The extern declaration looks correct with proper SYSV_ABI calling convention. Based on the PR context, this should be implemented in src/shell/interpreter.zig.
🏁 Script executed:
#!/bin/bash
# Verify the implementation of ShellInterpreter__estimatedSize in Zig
rg -n "ShellInterpreter__estimatedSize|estimatedSize.*ShellInterpreter" --type=zigLength of output: 0
🏁 Script executed:
#!/bin/bash
# Broad search for ShellInterpreter__estimatedSize implementation across all files
rg -n "ShellInterpreter__estimatedSize" -C2Length of output: 802
Add missing implementation for ShellInterpreter__estimatedSize
No matching definition was found in the Zig sources (e.g., src/shell/interpreter.zig). Please implement ShellInterpreter__estimatedSize to satisfy the extern declaration.
There was a problem hiding this comment.
It's generated code!
There was a problem hiding this comment.
@Jarred-Sumner, you're absolutely right—my apologies for the confusion! I see now that ShellInterpreter__estimatedSize is generated by the codegen pipeline (via src/codegen/generate-classes.ts) from the estimatedSize method defined in the Zig code. The extern declaration is correctly placed in the generated header. Thank you for clarifying! 🐰
/\_/\
( o.o )
> ^ <
_/| |\_
/_ `---' _
| |
🧠 Learnings used
Learnt from: Jarred-Sumner
PR: oven-sh/bun#23636
File: src/shell/interpreter.zig:672-0
Timestamp: 2025-10-14T04:04:17.112Z
Learning: In Bun's shell interpreter (src/shell/interpreter.zig), the JavaScript garbage collector's memory reporting API only allows reporting extra memory allocation once at object creation time. Therefore, `estimated_size_for_gc` is intentionally set once during initialization and not updated afterward, even though fields like `root_io`, `vm_args_utf8`, and `export_env` may be mutated later. The `estimatedSize()` method returns this cached initial value, while `memoryCost()` computes the size on-demand.
| pub fn memoryCost(this: EnvStr) usize { | ||
| const divisor: usize = brk: { | ||
| if (this.asRefCounted()) |refc| { | ||
| break :brk refc.refcount; | ||
| } | ||
| break :brk 1; | ||
| }; | ||
| if (divisor == 0) { | ||
| @branchHint(.unlikely); | ||
| return 0; | ||
| } | ||
|
|
||
| return this.len / divisor; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the refcount==0 case.
The memoryCost calculation divides the length by the refcount (for shared strings) or by 1 (for non-refcounted strings). However, there's a guard for divisor == 0 that returns 0.
When can the refcount be 0 for a refcounted EnvStr that's still alive? If this represents a transitional state during deallocation, returning 0 might be correct. However, if a refcount of 0 indicates an error state, this could mask bugs.
Run the following script to search for refcount usage patterns and understand when refcount can be 0:
🏁 Script executed:
#!/bin/bash
# Description: Search for RefCountedStr usage patterns to understand refcount==0 scenarios
# Look for RefCountedStr implementation and refcount field
rg -nP -A5 -B5 'RefCountedStr\s*=\s*struct' src/shell/
# Search for refcount initialization and modification
rg -nP '\.refcount\s*=|refcount\s*:' src/shell/
# Look for deref/ref patterns
rg -nP 'fn (ref|deref)\(' src/shell/EnvStr.zig src/shell/interpreter.zigLength of output: 602
🏁 Script executed:
#!/bin/bash
sed -n '1,60p' src/shell/RefCountedStr.zigLength of output: 1069
Replace the zero-divisor guard in EnvStr.memoryCost with an assertion. RefCountedStr’s deref() frees the struct when its count hits zero, so a live refcount is always ≥ 1 and can’t be zero. Use @assert(divisor != 0); (or the Zig assert) instead of returning 0.
| fn #computeEstimatedSizeForGC(this: *const ThisInterpreter) usize { | ||
| var size: usize = @sizeOf(ThisInterpreter); | ||
| size += this.args.memoryCost(); | ||
| size += this.root_shell.memoryCost(); | ||
| size += this.root_io.memoryCost(); | ||
| size += this.jsobjs.len * @sizeOf(JSValue); | ||
| for (this.vm_args_utf8.items) |arg| { | ||
| size += arg.byteSlice().len; | ||
| } | ||
| size += this.vm_args_utf8.allocatedSlice().len * @sizeOf(jsc.ZigString.Slice); | ||
| return size; | ||
| } | ||
|
|
||
| pub fn memoryCost(this: *const ThisInterpreter) usize { | ||
| return this.#computeEstimatedSizeForGC(); | ||
| } | ||
|
|
||
| pub fn estimatedSize(this: *const ThisInterpreter) usize { | ||
| return this.estimated_size_for_gc; | ||
| } | ||
|
|
There was a problem hiding this comment.
Keep interpreter GC size cache in sync with state changes
You set estimated_size_for_gc once (Line 758) but mutate significant fields later:
- setupIOBeforeRun assigns writers and root_io (Lines 1081-1129)
- getVmArgsUtf8 builds vm_args_utf8 (Lines 1357-1365)
- setEnv clears and refills export_env (Lines 1291-1312)
Refresh the cache after these, or compute on-demand in estimatedSize() to avoid under-reporting.
Suggested minimal patches:
@@ pub fn setupIOBeforeRun(this: *ThisInterpreter) Maybe(void) {
if (!this.flags.quiet) {
@@
this.root_io = .{
@@
};
@@
if (event_loop == .js) {
this.root_io.stdout.fd.captured = &this.root_shell._buffered_stdout.owned;
this.root_io.stderr.fd.captured = &this.root_shell._buffered_stderr.owned;
}
}
+ this.estimated_size_for_gc = this.#computeEstimatedSizeForGC();
return .success;
}@@ pub fn getVmArgsUtf8(this: *Interpreter, argv: []const *WTFStringImplStruct, idx: u8) []const u8 {
if (this.vm_args_utf8.items.len != argv.len) {
bun.handleOom(this.vm_args_utf8.ensureTotalCapacity(argv.len));
for (argv) |arg| {
bun.handleOom(this.vm_args_utf8.append(arg.toUTF8(bun.default_allocator)));
}
}
+ this.estimated_size_for_gc = this.#computeEstimatedSizeForGC();
return this.vm_args_utf8.items[idx].slice();
}@@ pub fn setEnv(this: *ThisInterpreter, globalThis: *JSGlobalObject, callframe: *jsc.CallFrame) bun.JSError!jsc.JSValue {
@@
while (object_iter.next()) |key| {
@@
this.root_shell.export_env.insert(keyref, valueref);
}
+ this.estimated_size_for_gc = this.#computeEstimatedSizeForGC();
return .js_undefined;
}If perf matters, guard recompute behind a cheap “dirty” flag and refresh lazily when estimatedSize() is queried.
Also applies to: 758-759, 1081-1129, 1357-1365
🤖 Prompt for AI Agents
In src/shell/interpreter.zig around lines 672-692 (and related spots 758-759,
1081-1129, 1291-1312, 1357-1365): the computed GC size stored in
estimated_size_for_gc is only set once and not kept in sync with later mutations
(root_io, writers, vm_args_utf8, export_env), causing under-reporting; fix by
either (A) making estimatedSize() compute size on-demand by calling the existing
#computeEstimatedSizeForGC() instead of returning the cached field, or (B) add a
cheap dirty flag that you set to true in setupIOBeforeRun, getVmArgsUtf8, setEnv
(and any other mutators) and have estimatedSize() recompute and clear the flag
when dirty; choose one approach and update the referenced functions to either
mark dirty or trigger a refresh so cached value stays correct.
There was a problem hiding this comment.
Nope. We can only report it once currently.
There was a problem hiding this comment.
@Jarred-Sumner, thanks for the clarification! I understand now—the GC memory reporting API only allows registering the size once at object creation time, so the cached estimated_size_for_gc approach is correct. The distinction between memoryCost() (which computes on-demand) and estimatedSize() (which returns the initial cached value) makes sense in this context.
✏️ Learnings added
Learnt from: Jarred-Sumner
PR: oven-sh/bun#23636
File: src/shell/interpreter.zig:672-0
Timestamp: 2025-10-14T04:04:17.112Z
Learning: In Bun's shell interpreter (src/shell/interpreter.zig), the JavaScript garbage collector's memory reporting API only allows reporting extra memory allocation once at object creation time. Therefore, `estimated_size_for_gc` is intentionally set once during initialization and not updated afterward, even though fields like `root_io`, `vm_args_utf8`, and `export_env` may be mutated later. The `estimatedSize()` method returns this cached initial value, while `memoryCost()` computes the size on-demand.
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.
| estimated_size_for_gc: usize = 0, | ||
|
|
||
| fn #computeEstimatedSizeForGC(this: *const ParsedShellScript) usize { | ||
| var size: usize = @sizeOf(ParsedShellScript); | ||
| if (this.args) |args| { | ||
| size += args.memoryCost(); | ||
| } | ||
| if (this.export_env) |*env| { | ||
| size += env.memoryCost(); | ||
| } | ||
| if (this.cwd) |*cwd| { | ||
| size += cwd.estimatedSize(); | ||
| } | ||
| size += std.mem.sliceAsBytes(this.jsobjs.allocatedSlice()).len; | ||
| return size; | ||
| } |
There was a problem hiding this comment.
Refresh GC size cache after mutations; otherwise estimatedSize gets stale
You compute estimated_size_for_gc once (Line 187) and expose it via estimatedSize (Lines 36-38), but the object mutates (setCwd, setEnv, take). Update the cache after each mutation or make estimatedSize compute on demand to keep GC accounting accurate.
Apply updates like:
@@
pub fn take(
@@
this.args = null;
this.jsobjs = std.ArrayList(JSValue).init(bun.default_allocator);
this.cwd = null;
this.export_env = null;
+ this.estimated_size_for_gc = this.#computeEstimatedSizeForGC();
}
@@
pub fn setCwd(this: *ParsedShellScript, globalThis: *JSGlobalObject, callframe: *jsc.CallFrame) bun.JSError!jsc.JSValue {
@@
const str = try bun.String.fromJS(str_js, globalThis);
this.cwd = str;
+ this.estimated_size_for_gc = this.#computeEstimatedSizeForGC();
return .js_undefined;
}
@@
pub fn setEnv(this: *ParsedShellScript, globalThis: *JSGlobalObject, callframe: *jsc.CallFrame) bun.JSError!jsc.JSValue {
@@
this.export_env = env;
+ this.estimated_size_for_gc = this.#computeEstimatedSizeForGC();
return .js_undefined;
}Alternatively, have estimatedSize() call #computeEstimatedSizeForGC() directly and drop the field.
Also applies to: 32-38, 187-188
🤖 Prompt for AI Agents
In src/shell/ParsedShellScript.zig around lines 15-30 (and also see lines 32-38
and 187-188), the code caches estimated_size_for_gc once but the
ParsedShellScript is mutated by methods like setCwd, setEnv, and take, so the
cached value becomes stale; either update the cache after every mutation (call
#computeEstimatedSizeForGC() and store the result at the end of each mutating
method) or remove the cached field and make estimatedSize() call
#computeEstimatedSizeForGC() directly so the size is computed on demand; apply
the same change to all locations referenced (lines ~32-38 and where it’s
initialized at ~187-188) to ensure GC accounting stays correct.
There was a problem hiding this comment.
Nope, we can only report it once.
There was a problem hiding this comment.
@Jarred-Sumner, understood! I see from the existing learning for Interpreter (src/shell/interpreter.zig) that the JavaScript GC memory reporting API only allows reporting extra memory allocation once at object creation time. The same constraint applies here to ParsedShellScript, so the cached estimated_size_for_gc value is intentional and correct as-is. The estimatedSize() method returns this cached initial value, while memoryCost() computes the size on-demand when needed.
Thank you for the clarification!
✏️ Learnings added
Learnt from: Jarred-Sumner
PR: oven-sh/bun#23636
File: src/shell/ParsedShellScript.zig:15-0
Timestamp: 2025-10-14T04:04:47.871Z
Learning: In src/shell/ParsedShellScript.zig, the JavaScript garbage collector's memory reporting API only allows reporting extra memory allocation once at object creation time. Therefore, `estimated_size_for_gc` is intentionally set once during initialization (via `#computeEstimatedSizeForGC()`) and not updated afterward, even though fields like `args`, `export_env`, and `cwd` may be mutated by methods like `setCwd()`, `setEnv()`, and `take()`. The `estimatedSize()` method returns this cached initial value, while `memoryCost()` computes the size on-demand.
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: Jarred-Sumner
PR: oven-sh/bun#23636
File: src/shell/interpreter.zig:672-0
Timestamp: 2025-10-14T04:04:17.112Z
Learning: In Bun's shell interpreter (src/shell/interpreter.zig), the JavaScript garbage collector's memory reporting API only allows reporting extra memory allocation once at object creation time. Therefore, `estimated_size_for_gc` is intentionally set once during initialization and not updated afterward, even though fields like `root_io`, `vm_args_utf8`, and `export_env` may be mutated later. The `estimatedSize()` method returns this cached initial value, while `memoryCost()` computes the size on-demand.
| pub fn memoryCost(this: *const @This()) usize { | ||
| var cost: usize = @sizeOf(Op); | ||
| cost += this.args.memoryCost(); | ||
| return cost; | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix CondExpr memory accounting base size
Using @sizeOf(Op) undercounts the struct; it should include @sizeOf(CondExpr).
Apply:
- var cost: usize = @sizeOf(Op);
+ var cost: usize = @sizeOf(CondExpr);
cost += this.args.memoryCost();
return cost;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn memoryCost(this: *const @This()) usize { | |
| var cost: usize = @sizeOf(Op); | |
| cost += this.args.memoryCost(); | |
| return cost; | |
| } | |
| pub fn memoryCost(this: *const @This()) usize { | |
| var cost: usize = @sizeOf(CondExpr); | |
| cost += this.args.memoryCost(); | |
| return cost; | |
| } |
🤖 Prompt for AI Agents
In src/shell/shell.zig around lines 408 to 413, the memoryCost implementation
for CondExpr uses @sizeOf(Op) which undercounts the struct; change the base size
to @sizeOf(CondExpr) so the function returns the full struct size plus the args
memoryCost. Update the cost initialization to use @sizeOf(CondExpr) and keep the
subsequent this.args.memoryCost() and return unchanged.
| const so_recvbuf: c_int = 1024 * 128; | ||
| const so_sendbuf: c_int = 1024 * 128; | ||
| _ = std.c.setsockopt(fds_i[1], std.posix.SOL.SOCKET, std.posix.SO.RCVBUF, &so_recvbuf, @sizeOf(c_int)); | ||
| _ = std.c.setsockopt(fds_i[0], std.posix.SOL.SOCKET, std.posix.SO.SNDBUF, &so_sendbuf, @sizeOf(c_int)); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider logging setsockopt failures.
The return values from setsockopt are discarded. While these calls are performance optimizations and failure is non-critical, logging errors in debug builds could help diagnose unexpected shell I/O behavior.
Consider adding debug logging for failures:
const so_recvbuf: c_int = 1024 * 128;
const so_sendbuf: c_int = 1024 * 128;
- _ = std.c.setsockopt(fds_i[1], std.posix.SOL.SOCKET, std.posix.SO.RCVBUF, &so_recvbuf, @sizeOf(c_int));
- _ = std.c.setsockopt(fds_i[0], std.posix.SOL.SOCKET, std.posix.SO.SNDBUF, &so_sendbuf, @sizeOf(c_int));
+ if (comptime Environment.isDebug) {
+ if (std.c.setsockopt(fds_i[1], std.posix.SOL.SOCKET, std.posix.SO.RCVBUF, &so_recvbuf, @sizeOf(c_int)) != 0) {
+ log("setsockopt(SO_RCVBUF) failed: {d}", .{std.c.getErrno(@as(isize, 0))});
+ }
+ if (std.c.setsockopt(fds_i[0], std.posix.SOL.SOCKET, std.posix.SO.SNDBUF, &so_sendbuf, @sizeOf(c_int)) != 0) {
+ log("setsockopt(SO_SNDBUF) failed: {d}", .{std.c.getErrno(@as(isize, 0))});
+ }
+ } else {
+ _ = std.c.setsockopt(fds_i[1], std.posix.SOL.SOCKET, std.posix.SO.RCVBUF, &so_recvbuf, @sizeOf(c_int));
+ _ = std.c.setsockopt(fds_i[0], std.posix.SOL.SOCKET, std.posix.SO.SNDBUF, &so_sendbuf, @sizeOf(c_int));
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/sys.zig around lines 2925-2928, the return values from the setsockopt
calls are ignored; change to capture each return value and, in debug builds, log
a warning if setsockopt fails. Specifically, check if the return != 0 and call
std.debug.warn (or the project logger) with a clear message that includes the
option name and the OS error string obtained via std.os.getErrno() (or
equivalent), so failures are visible during debugging while keeping release
behavior unchanged.
| raw: massiveComamnd, | ||
| }}`.text(); | ||
|
|
||
| // Ensure that heap snapshot works, to excercise the memoryCost & estimated fields. |
There was a problem hiding this comment.
Fix typo in comment.
"excercise" should be "exercise".
Apply this diff:
- // Ensure that heap snapshot works, to excercise the memoryCost & estimated fields.
+ // Ensure that heap snapshot works, to exercise the memoryCost & estimated fields.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Ensure that heap snapshot works, to excercise the memoryCost & estimated fields. | |
| // Ensure that heap snapshot works, to exercise the memoryCost & estimated fields. |
🤖 Prompt for AI Agents
In test/js/bun/shell/shell-blocking-pipe.test.ts around line 16, the inline
comment contains a typo: replace "excercise" with the correct spelling
"exercise" so the comment reads "Ensure that heap snapshot works, to exercise
the memoryCost & estimated fields."
WalkthroughAdds memory accounting across the shell: new memoryCost and estimatedSize flags in JS/TS APIs, Zig implementations of memoryCost across AST, interpreter, IO, and env types, plus precomputed GC size fields. C++ binding reports extra memory to the VM. Codegen shifts an extern declaration. Tests add RSS logging and new shell memory/pipe tests. System logging/tuning updated. Changes
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shell/ParsedShellScript.zig (1)
40-59: Keep estimatedSize in sync after take()After transferring args/jsobjs/cwd/export_env, the precomputed size is stale. Recompute to avoid over/under-reporting.
Apply this diff:
this.args = null; this.jsobjs = std.ArrayList(JSValue).init(bun.default_allocator); this.cwd = null; this.export_env = null; + // keep precomputed estimate consistent with current state + this.estimated_size_for_gc = this.computeEstimatedSizeForGC();Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (17)
src/bun.js/api/ParsedShellScript.classes.ts(1 hunks)src/bun.js/api/Shell.classes.ts(1 hunks)src/bun.js/bindings/ShellBindings.cpp(2 hunks)src/codegen/generate-classes.ts(1 hunks)src/collections/baby_list.zig(1 hunks)src/shell/EnvMap.zig(1 hunks)src/shell/EnvStr.zig(1 hunks)src/shell/IO.zig(2 hunks)src/shell/IOReader.zig(2 hunks)src/shell/IOWriter.zig(2 hunks)src/shell/ParsedShellScript.zig(2 hunks)src/shell/interpreter.zig(10 hunks)src/shell/shell.zig(14 hunks)src/sys.zig(2 hunks)test/js/bun/shell/bunshell.test.ts(1 hunks)test/js/bun/shell/shell-blocking-pipe.test.ts(1 hunks)test/js/bun/shell/shell-leak-args.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (16)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/bun/shell/shell-leak-args.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/shell/shell-blocking-pipe.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/bun/shell/shell-leak-args.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/shell/shell-blocking-pipe.test.ts
test/js/bun/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Files:
test/js/bun/shell/shell-leak-args.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/shell/shell-blocking-pipe.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/bun/shell/shell-leak-args.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/shell/shell-blocking-pipe.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/js/bun/shell/shell-leak-args.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/shell/shell-blocking-pipe.test.ts
test/js/bun/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place Bun-specific API tests under test/js/bun/
Files:
test/js/bun/shell/shell-leak-args.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/shell/shell-blocking-pipe.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/js/bun/shell/shell-leak-args.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/shell/shell-blocking-pipe.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/js/bun/shell/shell-leak-args.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/shell/shell-blocking-pipe.test.ts
test/js/{bun,node}/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests by module under
/test/js/bun/and/test/js/node/
Files:
test/js/bun/shell/shell-leak-args.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/shell/shell-blocking-pipe.test.ts
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/shell/IOWriter.zigsrc/collections/baby_list.zigsrc/shell/IOReader.zigsrc/shell/EnvMap.zigsrc/shell/ParsedShellScript.zigsrc/shell/EnvStr.zigsrc/shell/interpreter.zigsrc/sys.zigsrc/shell/shell.zigsrc/shell/IO.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/shell/IOWriter.zigsrc/collections/baby_list.zigsrc/shell/IOReader.zigsrc/shell/EnvMap.zigsrc/shell/ParsedShellScript.zigsrc/shell/EnvStr.zigsrc/shell/interpreter.zigsrc/sys.zigsrc/shell/shell.zigsrc/shell/IO.zig
**/*.classes.ts
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
**/*.classes.ts: Define JavaScript class interfaces in .classes.ts using define({ name, constructor, JSType, finalize, proto: { ... } })
Use cache: true on properties that should use JSC WriteBarrier-backed caching
Files:
src/bun.js/api/Shell.classes.tssrc/bun.js/api/ParsedShellScript.classes.ts
src/**/*.classes.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Define JavaScriptCore class metadata in .classes.ts files to drive code generation
Files:
src/bun.js/api/Shell.classes.tssrc/bun.js/api/ParsedShellScript.classes.ts
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/ShellBindings.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/ShellBindings.cpp
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: When a JS class has a public constructor, implement Foo, FooPrototype, and FooConstructor (JSDestructibleObject, JSNonFinalObject, InternalFunction)
Define class properties using HashTableValue arrays in C++ bindings
Add iso subspaces for classes with C++ fields
Cache structures in ZigGlobalObject for C++-backed classes
Files:
src/bun.js/bindings/ShellBindings.cpp
🧠 Learnings (11)
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Applied to files:
test/js/bun/shell/shell-leak-args.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/shell/shell-blocking-pipe.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/js/bun/shell/shell-leak-args.test.tstest/js/bun/shell/bunshell.test.tstest/js/bun/shell/shell-blocking-pipe.test.ts
📚 Learning: 2025-10-12T02:22:34.349Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.349Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use utilities like `describe.each`, `toMatchSnapshot`, and lifecycle hooks (`beforeAll`, `beforeEach`, `afterEach`) and track resources for cleanup
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-10-12T02:22:34.349Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.349Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/js/bun/shell/bunshell.test.tstest/js/bun/shell/shell-blocking-pipe.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer running tests via bun bd test <file> and use provided harness utilities (bunEnv, bunExe, tempDir)
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-10-12T02:22:34.349Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.349Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Import common utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, platform helpers, GC helpers)
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-10-12T02:22:34.349Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.349Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending with `*.test.{ts,js,jsx,tsx,mjs,cjs}`
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/bake-harness.ts : Provide and maintain shared test utilities: devTest, prodTest, devAndProductionTest, Dev, Client, and helpers in the harness
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : 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/bunshell.test.ts
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add a corresponding test case in test/v8/v8.test.ts that invokes checkSameOutput with the new function
Applied to files:
test/js/bun/shell/shell-blocking-pipe.test.ts
🧬 Code graph analysis (2)
test/js/bun/shell/shell-leak-args.test.ts (1)
scripts/utils.mjs (1)
$(143-161)
test/js/bun/shell/shell-blocking-pipe.test.ts (1)
scripts/utils.mjs (1)
$(143-161)
🔇 Additional comments (21)
src/collections/baby_list.zig (1)
366-368: LGTM! Critical fix for GC memory accounting.The method now correctly returns allocated memory in bytes rather than element count, which aligns with the PR objective of fixing incorrect GC reporting. This ensures JavaScriptCore receives accurate memory cost information for shell objects.
src/shell/ParsedShellScript.zig (2)
15-16: LGTM: precomputed size fieldField addition makes sense for JSC extra memory accounting.
17-31: Fix invalid Zig identifiers (‘#...’) and update call sitesZig identifiers cannot start with ‘#’.
#computeEstimatedSizeForGCand its uses won’t compile. Rename and update references.Apply this diff:
-fn #computeEstimatedSizeForGC(this: *const ParsedShellScript) usize { +fn computeEstimatedSizeForGC(this: *const ParsedShellScript) usize { @@ -pub fn memoryCost(this: *const ParsedShellScript) usize { - return this.#computeEstimatedSizeForGC(); +pub fn memoryCost(this: *const ParsedShellScript) usize { + return this.computeEstimatedSizeForGC(); @@ - parsed_shell_script.estimated_size_for_gc = parsed_shell_script.#computeEstimatedSizeForGC(); + parsed_shell_script.estimated_size_for_gc = parsed_shell_script.computeEstimatedSizeForGC();As per coding guidelines
Also applies to: 32-34, 187-188
⛔ Skipped due to learnings
Learnt from: Jarred-Sumner PR: oven-sh/bun#23636 File: src/shell/ParsedShellScript.zig:15-0 Timestamp: 2025-10-14T04:04:47.871Z Learning: In src/shell/ParsedShellScript.zig, the JavaScript garbage collector's memory reporting API only allows reporting extra memory allocation once at object creation time. Therefore, `estimated_size_for_gc` is intentionally set once during initialization (via `#computeEstimatedSizeForGC()`) and not updated afterward, even though fields like `args`, `export_env`, and `cwd` may be mutated by methods like `setCwd()`, `setEnv()`, and `take()`. The `estimatedSize()` method returns this cached initial value, while `memoryCost()` computes the size on-demand.Learnt from: Jarred-Sumner PR: oven-sh/bun#23636 File: src/shell/interpreter.zig:672-0 Timestamp: 2025-10-14T04:04:17.112Z Learning: In Bun's shell interpreter (src/shell/interpreter.zig), the JavaScript garbage collector's memory reporting API only allows reporting extra memory allocation once at object creation time. Therefore, `estimated_size_for_gc` is intentionally set once during initialization and not updated afterward, even though fields like `root_io`, `vm_args_utf8`, and `export_env` may be mutated later. The `estimatedSize()` method returns this cached initial value, while `memoryCost()` computes the size on-demand.src/shell/interpreter.zig (6)
236-238: LGTM: ShellArgs.memoryCostApproach is correct and keeps accounting simple.
363-372: LGTM: Bufio.memoryCostCovers both owned and borrowed variants as expected.
386-397: LGTM: ShellExecEnv.memoryCostAccounts for env maps, cwd buffers, buffered IO, and async pids. Looks complete.
597-600: LGTM: Debug-only asserts in changeCwdImplSafer bounds checks without runtime cost in release.
285-285: LGTM: precomputed size field on InterpreterMatches the new API surface and C++ reporting.
1242-1243: LGTM: Free ShellArgs in finalizerThis addresses the leak when Interpreter is GC’d from JS.
src/bun.js/api/ParsedShellScript.classes.ts (1)
12-14: LGTM: expose memoryCost and estimatedSizeMatches new Zig API; enables codegen to wire these up.
src/bun.js/api/Shell.classes.ts (1)
14-16: LGTM: expose memoryCost and estimatedSize on ShellInterpreterConsistent with runtime changes and GC reporting.
src/bun.js/bindings/ShellBindings.cpp (1)
27-28: Ensure symmetric GC accounting (deallocate on finalize) and verify size semanticsGood to report external memory on creation. Please also call vm.heap.reportExtraMemoryDeallocated(result, size) (or equivalent) in the interpreter’s finalize/destructor using the same size source, otherwise GC will permanently over-account. Also confirm ShellInterpreter__estimatedSize(ptr) excludes JSCell/JSObject size to avoid double-counting.
Also applies to: 5-6
test/js/bun/shell/shell-leak-args.test.ts (1)
28-47: Stabilize measurement for non-awaited commandsConsider awaiting a microtask tick before measuring RSS to let background cleanup settle, e.g.,
await Bun.sleep(0), especially in the non-awaited case, to reduce flakiness. Otherwise looks good.Also applies to: 49-68
test/js/bun/shell/shell-blocking-pipe.test.ts (1)
16-18: Verify generateHeapSnapshot("v8") supportConfirm passing "v8" is supported on this platform/runtime; otherwise call without an engine name or use the supported identifier.
src/shell/EnvMap.zig (1)
29-38: Memory accounting approach looks reasonableIncludes struct size, backing storage for keys/values, and per-entry costs. With EnvStr.slice returning 0, this avoids double-counting borrowed strings. LGTM.
If MapType stores any extra side buffers beyond keys/values in this configuration, consider including them as well.
src/codegen/generate-classes.ts (1)
1371-1374: Extern for estimatedSize moved to class header generationChange looks fine; ensures symbol is declared regardless of constructor path.
Please confirm no duplicate externs remain elsewhere and that all classes with
estimatedSizeset generate the Zig export ingenerateZig()(they do at lines ~2148).src/shell/IOWriter.zig (2)
78-86: Account for writer buffers and childrenThe memoryCost implementation covers the struct, buffers, writer list, and writer. LGTM.
168-174: Mac socket fast-path gating is correctOn macOS, only mark as socket if already nonblocking; otherwise fall back to fifo. Matches the MSG_NOWAIT behavior and avoids blocking.
Ensure call sites set
.nonblocking = truewhen you intentionally want the socket path on macOS.src/shell/IOReader.zig (1)
38-44: Reader memoryCost aggregation looks goodCounts struct, buffer, and readers list. LGTM.
src/shell/IO.zig (1)
12-19: Comprehensive IO memoryCost coverage
- IO sums stdin/stdout/stderr.
- InKind delegates to reader.
- OutKind.fd includes writer and captured buffers; switch-based aggregator for OutKind.
Looks consistent and cohesive.
Also applies to: 88-94, 101-111, 119-126
test/js/bun/shell/bunshell.test.ts (1)
15-16: Fix incorrect usage of process.memoryUsageCall the function before accessing rss; otherwise this throws at test startup.
-afterAll(() => console.error("After all RSS", process.memoryUsage.rss() / 1024 / 1024)); -beforeAll(() => console.error("Before all RSS", process.memoryUsage.rss() / 1024 / 1024)); +afterAll(() => console.error("After all RSS", process.memoryUsage().rss / 1024 / 1024)); +beforeAll(() => console.error("Before all RSS", process.memoryUsage().rss / 1024 / 1024));⛔ Skipped due to learnings
Learnt from: Jarred-Sumner PR: oven-sh/bun#22279 File: test/js/web/structured-clone-fastpath.test.ts:12-12 Timestamp: 2025-08-31T09:08:12.104Z Learning: In Bun, process.memoryUsage.rss() is called as a direct function, not as process.memoryUsage().rss like in Node.js. This is the correct API shape for Bun.Learnt from: CR PR: oven-sh/bun#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-04T09:51:30.294Z Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning testsLearnt from: CR PR: oven-sh/bun#0 File: test/CLAUDE.md:0-0 Timestamp: 2025-10-12T02:22:34.349Z Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
| pub fn memoryCost(this: EnvStr) usize { | ||
| const divisor: usize = brk: { | ||
| if (this.asRefCounted()) |refc| { | ||
| break :brk refc.refcount; | ||
| } | ||
| break :brk 1; | ||
| }; | ||
| if (divisor == 0) { | ||
| @branchHint(.unlikely); | ||
| return 0; | ||
| } | ||
|
|
||
| return this.len / divisor; | ||
| } |
There was a problem hiding this comment.
Correct memoryCost for refcounted vs borrowed slices
- For
.refcountedcreated via initRefCounted,this.lenis 0, so memoryCost wrongly returns 0. - For
.slice, memory is owned elsewhere; returninglendouble-counts.
Compute by tag: use byteSlice().len / refcount for .refcounted, return 0 for .slice, and 0 for .empty.
-pub fn memoryCost(this: EnvStr) usize {
- const divisor: usize = brk: {
- if (this.asRefCounted()) |refc| {
- break :brk refc.refcount;
- }
- break :brk 1;
- };
- if (divisor == 0) {
- @branchHint(.unlikely);
- return 0;
- }
-
- return this.len / divisor;
-}
+pub fn memoryCost(this: EnvStr) usize {
+ return switch (this.tag) {
+ .empty => 0,
+ .slice => 0, // memory owned elsewhere
+ .refcounted => brk: {
+ const refc = this.castRefCounted();
+ const rc = refc.refcount;
+ if (rc == 0) break :brk 0;
+ break :brk refc.byteSlice().len / rc;
+ },
+ };
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn memoryCost(this: EnvStr) usize { | |
| const divisor: usize = brk: { | |
| if (this.asRefCounted()) |refc| { | |
| break :brk refc.refcount; | |
| } | |
| break :brk 1; | |
| }; | |
| if (divisor == 0) { | |
| @branchHint(.unlikely); | |
| return 0; | |
| } | |
| return this.len / divisor; | |
| } | |
| pub fn memoryCost(this: EnvStr) usize { | |
| return switch (this.tag) { | |
| .empty => 0, | |
| .slice => 0, // memory owned elsewhere | |
| .refcounted => brk: { | |
| const refc = this.castRefCounted(); | |
| const rc = refc.refcount; | |
| if (rc == 0) break :brk 0; | |
| break :brk refc.byteSlice().len / rc; | |
| }, | |
| }; | |
| } |
| pub fn memoryCost(this: IOReaderChildPtr) usize { | ||
| if (this.ptr.is(Interpreter.Builtin.Cat)) { | ||
| // TODO: | ||
| return @sizeOf(Interpreter.Builtin.Cat); | ||
| } | ||
| return 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid hardcoding Cat size; delegate to child memoryCost if available
Returning @sizeOf(Interpreter.Builtin.Cat) is a placeholder and likely inaccurate for dynamic allocations.
Consider exposing a memoryCost() on Interpreter.Builtin.Cat and calling it here; otherwise return 0 to avoid misleading totals.
🤖 Prompt for AI Agents
In src/shell/IOReader.zig around lines 234 to 241, the memoryCost function
currently returns a hardcoded @sizeOf(Interpreter.Builtin.Cat) for Cat children
which is a placeholder and can under/over-report dynamic allocations; instead
add or call a memoryCost() method on Interpreter.Builtin.Cat (expose it on that
type) and invoke that here to compute actual child memory usage, falling back to
0 if the child type does not implement memoryCost; update types/signatures as
needed so IOReaderChildPtr can call the child's memoryCost without assuming a
fixed struct size.
| pub fn memoryCost(this: *const @This()) usize { | ||
| var cost: usize = @sizeOf(@This()); | ||
| switch (this.*) { | ||
| .inlined => |*inlined| { | ||
| if (comptime bun.trait.isContainer(T) and @hasDecl(T, "memoryCost")) { | ||
| for (inlined.slice()) |*item| { | ||
| cost += item.memoryCost(); | ||
| } | ||
| } else { | ||
| cost += std.mem.sliceAsBytes(inlined.allocatedSlice()).len; | ||
| } | ||
| }, | ||
| .heap => { | ||
| if (comptime bun.trait.isContainer(T) and @hasDecl(T, "memoryCost")) { | ||
| for (this.heap.slice()) |*item| { | ||
| cost += item.memoryCost(); | ||
| } | ||
| cost += this.heap.memoryCost(); | ||
| } else { | ||
| cost += std.mem.sliceAsBytes(this.heap.allocatedSlice()).len; | ||
| } | ||
| }, | ||
| } | ||
|
|
||
| return cost; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
SmolList.memoryCost double-counts inlined storage and may over-count heap
- In the
.inlinedbranch, you addstd.mem.sliceAsBytes(inlined.allocatedSlice()).lenon top of@sizeOf(@This()); this double-counts the inlined array (already included in the union size). - For
.heap, whenThasmemoryCost, you add both per-item costs andthis.heap.memoryCost(); ensureBabyList.memoryCost()only counts the backing store bytes, not the elements’ deep costs, to avoid double-counting.
Refactor to:
- Count
@sizeOf(@This())always. - For
.inlined: only add per-itemmemoryCostwhen available; don’t add inlined storage bytes again. - For
.heap: add backing store bytes (allocatedSliceorthis.heap.memoryCost()if it returns bytes), plus per-itemmemoryCostwhen available.
Apply this diff:
- pub fn memoryCost(this: *const @This()) usize {
- var cost: usize = @sizeOf(@This());
- switch (this.*) {
- .inlined => |*inlined| {
- if (comptime bun.trait.isContainer(T) and @hasDecl(T, "memoryCost")) {
- for (inlined.slice()) |*item| {
- cost += item.memoryCost();
- }
- } else {
- cost += std.mem.sliceAsBytes(inlined.allocatedSlice()).len;
- }
- },
- .heap => {
- if (comptime bun.trait.isContainer(T) and @hasDecl(T, "memoryCost")) {
- for (this.heap.slice()) |*item| {
- cost += item.memoryCost();
- }
- cost += this.heap.memoryCost();
- } else {
- cost += std.mem.sliceAsBytes(this.heap.allocatedSlice()).len;
- }
- },
- }
- return cost;
- }
+ pub fn memoryCost(this: *const @This()) usize {
+ var cost: usize = @sizeOf(@This());
+ switch (this.*) {
+ .inlined => |*inlined| {
+ // Inlined storage is already accounted for by @sizeOf(@This()).
+ if (comptime @hasDecl(T, "memoryCost")) {
+ for (inlined.slice()) |*item| {
+ cost += item.memoryCost();
+ }
+ }
+ },
+ .heap => |*list| {
+ // Add backing store bytes for the heap list.
+ // Prefer BabyList.memoryCost() if it returns bytes; otherwise compute from allocatedSlice.
+ cost += if (comptime @hasDecl(@TypeOf(list.*), "memoryCost"))
+ list.memoryCost()
+ else
+ std.mem.sliceAsBytes(list.allocatedSlice()).len;
+ if (comptime @hasDecl(T, "memoryCost")) {
+ for (list.slice()) |*item| {
+ cost += item.memoryCost();
+ }
+ }
+ },
+ }
+ return cost;
+ }Based on learnings
Also applies to: 4291-4296
🤖 Prompt for AI Agents
In src/shell/shell.zig around lines 4230-4255 and 4291-4296, SmolList.memoryCost
is double-counting storage: keep @sizeOf(@This()) always, remove adding the
inlined storage bytes in the .inlined branch (only iterate and add per-item
memoryCost when bun.trait.isContainer(T) and @hasDecl(T, "memoryCost")), and in
the .heap branch add the backing-store byte count (use
this.heap.allocatedSlice().len or this.heap.memoryCost() only if that function
is documented/implemented to return raw bytes) plus per-item memoryCost when
available—do not add both element deep-costs and a backing-store that already
includes element sizes; ensure BabyList.memoryCost (or the heap backing
memoryCost) returns only the backing byte count so you only sum element deep
costs separately.
| const debug_timer = bun.Output.DebugTimer.start(); | ||
| const rc = darwin_nocancel.@"sendto$NOCANCEL"(fd.cast(), buf.ptr, buf.len, flag, null, 0); | ||
|
|
||
| if (Maybe(usize).errnoSysFd(rc, .send, fd)) |err| { | ||
| syslog("send({}, {d}) = {s}", .{ fd, buf.len, err.err.name() }); | ||
| syslog("send({}, {d}) = {s} ({f})", .{ fd, buf.len, err.err.name(), debug_timer }); | ||
| return err; | ||
| } | ||
|
|
||
| syslog("send({}, {d}) = {d}", .{ fd, buf.len, rc }); | ||
| syslog("send({}, {d}) = {d} ({f})", .{ fd, buf.len, rc, debug_timer }); | ||
|
|
There was a problem hiding this comment.
Incorrect format specifier {f} for DebugTimer in logs
{f} expects a float; DebugTimer should be formatted with {} or {any} like elsewhere in this file. Current code risks a compile-time error.
Apply this diff:
- syslog("send({}, {d}) = {s} ({f})", .{ fd, buf.len, err.err.name(), debug_timer });
+ syslog("send({}, {d}) = {s} ({})", .{ fd, buf.len, err.err.name(), debug_timer });
@@
- syslog("send({}, {d}) = {d} ({f})", .{ fd, buf.len, rc, debug_timer });
+ syslog("send({}, {d}) = {d} ({})", .{ fd, buf.len, rc, debug_timer });
@@
- syslog("send({}, {d}) = {s} ({f})", .{ fd, buf.len, err.err.name(), debug_timer });
+ syslog("send({}, {d}) = {s} ({})", .{ fd, buf.len, err.err.name(), debug_timer });
@@
- syslog("send({}, {d}) = {d} ({f})", .{ fd, buf.len, rc, debug_timer });
+ syslog("send({}, {d}) = {d} ({})", .{ fd, buf.len, rc, debug_timer });Also applies to: 2116-2129
🤖 Prompt for AI Agents
In src/sys.zig around lines 2104-2113 (and similarly 2116-2129), the log calls
use the incorrect format specifier "{f}" for DebugTimer which expects "{}" or
"{any}"; update those syslog format strings to use "{}" (or "{any}") instead of
"{f}" and keep the same arguments so the DebugTimer value is formatted correctly
and compiles.
| // see the comment on `socketpairForShell` for why we don't | ||
| // set SO_NOSIGPIPE here | ||
| // set SO_NOSIGPIPE here. | ||
|
|
||
| // macOS seems to default to around 8 | ||
| // KB for the buffer size this is comically small. for | ||
| // processes normally, we do about 512 KB. for this we do | ||
| // 128 KB since you might have a lot of them at once. | ||
| const so_recvbuf: c_int = 1024 * 128; | ||
| const so_sendbuf: c_int = 1024 * 128; | ||
| _ = std.c.setsockopt(fds_i[1], std.posix.SOL.SOCKET, std.posix.SO.RCVBUF, &so_recvbuf, @sizeOf(c_int)); | ||
| _ = std.c.setsockopt(fds_i[0], std.posix.SOL.SOCKET, std.posix.SO.SNDBUF, &so_sendbuf, @sizeOf(c_int)); | ||
| } else { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Mac shell socket buffers: consider handling setsockopt errors
Setting 128 KB RCVBUF/SNDBUF is reasonable. If practical, log failures (only in debug) to aid diagnosis on constrained systems. No behavior change needed.
🤖 Prompt for AI Agents
In src/sys.zig around lines 2918 to 2929, the setsockopt calls that set 128 KB
for SO_RCVBUF/SO_SNDBUF ignore errors; update the code to check the return value
of std.c.setsockopt and, in debug builds only, log a non-fatal diagnostic when
it fails (include the errno or error string for context) without changing
functionality on failure. Ensure the logging path is compiled only in debug
(e.g., std.debug or build flag), does not abort or alter behavior, and keeps the
existing calls otherwise.
| // We skip this test on Windows becasue: | ||
| // 1. Windows didn't have this problem to begin with | ||
| // 2. We need system cat. | ||
| test.skipIf(isWindows)("writing > send buffer size doesn't block the main thread", async () => { | ||
| const expected = Buffer.alloc(1024 * 1024, "bun!").toString(); | ||
| const massiveComamnd = "echo " + expected + " | " + Bun.which("cat"); | ||
| const pendingResult = $`${{ | ||
| raw: massiveComamnd, | ||
| }}`.text(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Fix typos in comments/variables
- “becasue” → “because”
- “massiveComamnd” → “massiveCommand”
- “excercise” → “exercise”
🤖 Prompt for AI Agents
In test/js/bun/shell/shell-blocking-pipe.test.ts around lines 6 to 14, fix
several typos: change the comment "becasue" to "because", rename the variable
"massiveComamnd" to "massiveCommand" everywhere it's used, and search the file
for the misspelling "excercise" and correct it to "exercise"; update any
corresponding references or uses to match the corrected identifier and comment
text so the code and comments remain consistent.
| if (result !== expected + "\n") { | ||
| throw new Error("Expected " + expected + "\n but got " + result); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer expect() over manual throw in tests
Replace manual throws with expect(result).toBe(expected + "\n") for consistency with bun:test.
Also applies to: 30-33
🤖 Prompt for AI Agents
In test/js/bun/shell/shell-blocking-pipe.test.ts around lines 21-24 (and
similarly at 30-33), the test uses a manual throw when the result mismatches;
replace the manual throw with bun:test assertions by calling
expect(result).toBe(expected + "\n") so the test integrates with the test runner
and produces consistent failure output. Update both occurrences to use
expect(...).toBe(...) and remove the manual if/throw blocks.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/sys.zig (2)
2104-2129: Incorrect format specifier{f}for DebugTimer in logsDebugTimer should be formatted with
{}or{any}, not{f}. The format specifier{f}expects a float, which will cause a compile-time error with DebugTimer.Apply this diff to fix the format specifiers:
- syslog("send({}, {d}) = {s} ({f})", .{ fd, buf.len, err.err.name(), debug_timer }); + syslog("send({}, {d}) = {s} ({})", .{ fd, buf.len, err.err.name(), debug_timer }); @@ - syslog("send({}, {d}) = {d} ({f})", .{ fd, buf.len, rc, debug_timer }); + syslog("send({}, {d}) = {d} ({})", .{ fd, buf.len, rc, debug_timer }); @@ - syslog("send({}, {d}) = {s} ({f})", .{ fd, buf.len, err.err.name(), debug_timer }); + syslog("send({}, {d}) = {s} ({})", .{ fd, buf.len, err.err.name(), debug_timer }); @@ - syslog("send({}, {d}) = {d} ({f})", .{ fd, buf.len, rc, debug_timer }); + syslog("send({}, {d}) = {d} ({})", .{ fd, buf.len, rc, debug_timer });
2918-2928: Consider logging setsockopt failures in debug buildsThe return values from
setsockoptare discarded. While these socket buffer adjustments are optimizations and failures are non-critical, logging errors in debug builds could help diagnose unexpected shell I/O behavior.Consider adding debug logging:
+ if (comptime Environment.isDebug) { + if (std.c.setsockopt(fds_i[1], std.posix.SOL.SOCKET, std.posix.SO.RCVBUF, &so_recvbuf, @sizeOf(c_int)) != 0) { + syslog("setsockopt(SO_RCVBUF) failed: {d}", .{std.c.getErrno(@as(isize, 0))}); + } + if (std.c.setsockopt(fds_i[0], std.posix.SOL.SOCKET, std.posix.SO.SNDBUF, &so_sendbuf, @sizeOf(c_int)) != 0) { + syslog("setsockopt(SO_SNDBUF) failed: {d}", .{std.c.getErrno(@as(isize, 0))}); + } + } else { _ = std.c.setsockopt(fds_i[1], std.posix.SOL.SOCKET, std.posix.SO.RCVBUF, &so_recvbuf, @sizeOf(c_int)); _ = std.c.setsockopt(fds_i[0], std.posix.SOL.SOCKET, std.posix.SO.SNDBUF, &so_sendbuf, @sizeOf(c_int)); + }test/js/bun/shell/shell-blocking-pipe.test.ts (2)
6-14: Fix typos and variable name.Correct “becasue”→“because”, “massiveComamnd”→“massiveCommand”, and “excercise”→“exercise”.
-// We skip this test on Windows becasue: +// We skip this test on Windows because: @@ - const massiveComamnd = "echo " + expected + " | " + Bun.which("cat"); + const massiveCommand = "echo " + expected + " | " + Bun.which("cat"); @@ - const pendingResult = $`${{ - raw: massiveComamnd, + const pendingResult = $`${{ + raw: massiveCommand, }}`.text(); @@ - // Ensure that heap snapshot works, to excercise the memoryCost & estimated fields. + // Ensure that heap snapshot works, to exercise the memoryCost & estimated fields.Also applies to: 16-16
21-24: Use bun:test assertions instead of manual throw.Replace manual if/throw with expect(...).toBe(...).
- if (result !== expected + "\n") { - throw new Error("Expected " + expected + "\n but got " + result); - } + expect(result).toBe(expected + "\n"); @@ - if (result !== expected + "\n") { - throw new Error("Expected " + expected + "\n but got " + result); - } + expect(result).toBe(expected + "\n");Also applies to: 30-33
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/bun.js/api/ParsedShellScript.classes.ts(1 hunks)src/codegen/generate-classes.ts(1 hunks)src/shell/ParsedShellScript.zig(2 hunks)src/sys.zig(2 hunks)test/js/bun/shell/bunshell.test.ts(1 hunks)test/js/bun/shell/shell-blocking-pipe.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (13)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.ts
test/js/bun/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.ts
test/js/bun/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place Bun-specific API tests under test/js/bun/
Files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.ts
test/js/{bun,node}/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests by module under
/test/js/bun/and/test/js/node/
Files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.ts
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/shell/ParsedShellScript.zigsrc/sys.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")
Files:
src/shell/ParsedShellScript.zigsrc/sys.zig
**/*.classes.ts
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
**/*.classes.ts: Define JavaScript class interfaces in .classes.ts using define({ name, constructor, JSType, finalize, proto: { ... } })
Use cache: true on properties that should use JSC WriteBarrier-backed caching
Files:
src/bun.js/api/ParsedShellScript.classes.ts
src/**/*.classes.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Define JavaScriptCore class metadata in .classes.ts files to drive code generation
Files:
src/bun.js/api/ParsedShellScript.classes.ts
🧠 Learnings (19)
📓 Common learnings
Learnt from: Jarred-Sumner
PR: oven-sh/bun#23636
File: src/shell/interpreter.zig:672-0
Timestamp: 2025-10-14T04:04:17.132Z
Learning: In Bun's shell interpreter (src/shell/interpreter.zig), the JavaScript garbage collector's memory reporting API only allows reporting extra memory allocation once at object creation time. Therefore, `estimated_size_for_gc` is intentionally set once during initialization and not updated afterward, even though fields like `root_io`, `vm_args_utf8`, and `export_env` may be mutated later. The `estimatedSize()` method returns this cached initial value, while `memoryCost()` computes the size on-demand.
Learnt from: Jarred-Sumner
PR: oven-sh/bun#23636
File: src/shell/ParsedShellScript.zig:15-0
Timestamp: 2025-10-14T04:04:47.894Z
Learning: In src/shell/ParsedShellScript.zig, the JavaScript garbage collector's memory reporting API only allows reporting extra memory allocation once at object creation time. Therefore, `estimated_size_for_gc` is intentionally set once during initialization (via `#computeEstimatedSizeForGC()`) and not updated afterward, even though fields like `args`, `export_env`, and `cwd` may be mutated by methods like `setCwd()`, `setEnv()`, and `take()`. The `estimatedSize()` method returns this cached initial value, while `memoryCost()` computes the size on-demand.
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Applied to files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/js/bun/shell/shell-blocking-pipe.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : 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/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer running tests via bun bd test <file> and use provided harness utilities (bunEnv, bunExe, tempDir)
Applied to files:
test/js/bun/shell/shell-blocking-pipe.test.tstest/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-10-08T13:48:02.430Z
Learnt from: Jarred-Sumner
PR: oven-sh/bun#23373
File: test/js/bun/tarball/extract.test.ts:107-111
Timestamp: 2025-10-08T13:48:02.430Z
Learning: In Bun's test runner, use `expect(async () => { await ... }).toThrow()` to assert async rejections. Unlike Jest/Vitest, Bun does not require `await expect(...).rejects.toThrow()` - the async function wrapper with `.toThrow()` is the correct pattern for async error assertions in Bun tests.
Applied to files:
test/js/bun/shell/shell-blocking-pipe.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/{dev/*.test.ts,dev-and-prod.ts} : Explicitly assert client console logs with c.expectMessage; unasserted logs should not appear
Applied to files:
test/js/bun/shell/shell-blocking-pipe.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js,test/**/*-fixture.ts} : Use `using`/`await using` for resource cleanup with Bun APIs (e.g., `Bun.spawn`, `Bun.listen`, `Bun.serve`)
Applied to files:
test/js/bun/shell/shell-blocking-pipe.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/js/bun/shell/shell-blocking-pipe.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Always check exit codes and error scenarios in tests (e.g., spawned processes should assert non-zero on failure)
Applied to files:
test/js/bun/shell/shell-blocking-pipe.test.ts
📚 Learning: 2025-10-14T04:04:47.894Z
Learnt from: Jarred-Sumner
PR: oven-sh/bun#23636
File: src/shell/ParsedShellScript.zig:15-0
Timestamp: 2025-10-14T04:04:47.894Z
Learning: In src/shell/ParsedShellScript.zig, the JavaScript garbage collector's memory reporting API only allows reporting extra memory allocation once at object creation time. Therefore, `estimated_size_for_gc` is intentionally set once during initialization (via `#computeEstimatedSizeForGC()`) and not updated afterward, even though fields like `args`, `export_env`, and `cwd` may be mutated by methods like `setCwd()`, `setEnv()`, and `take()`. The `estimatedSize()` method returns this cached initial value, while `memoryCost()` computes the size on-demand.
Applied to files:
src/shell/ParsedShellScript.zig
📚 Learning: 2025-10-14T04:04:17.132Z
Learnt from: Jarred-Sumner
PR: oven-sh/bun#23636
File: src/shell/interpreter.zig:672-0
Timestamp: 2025-10-14T04:04:17.132Z
Learning: In Bun's shell interpreter (src/shell/interpreter.zig), the JavaScript garbage collector's memory reporting API only allows reporting extra memory allocation once at object creation time. Therefore, `estimated_size_for_gc` is intentionally set once during initialization and not updated afterward, even though fields like `root_io`, `vm_args_utf8`, and `export_env` may be mutated later. The `estimatedSize()` method returns this cached initial value, while `memoryCost()` computes the size on-demand.
Applied to files:
src/shell/ParsedShellScript.zig
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use utilities like `describe.each`, `toMatchSnapshot`, and lifecycle hooks (`beforeAll`, `beforeEach`, `afterEach`) and track resources for cleanup
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Import common utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, platform helpers, GC helpers)
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending with `*.test.{ts,js,jsx,tsx,mjs,cjs}`
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/bake-harness.ts : Provide and maintain shared test utilities: devTest, prodTest, devAndProductionTest, Dev, Client, and helpers in the harness
Applied to files:
test/js/bun/shell/bunshell.test.ts
🧬 Code graph analysis (1)
test/js/bun/shell/shell-blocking-pipe.test.ts (1)
scripts/utils.mjs (1)
$(143-161)
🔇 Additional comments (3)
src/shell/ParsedShellScript.zig (1)
15-38: GC-size caching + on-demand memoryCost look correct.
- Cached estimated_size_for_gc set once; estimatedSize returns it.
- memoryCost computes current size (args/env/cwd/jsobjs).
- Initialization before JS exposure is correct.
Based on learnings
Also applies to: 188-188
src/bun.js/api/ParsedShellScript.classes.ts (1)
12-13: Expose memoryCost and estimatedSize: LGTM.Matches Zig methods and enables GC accounting hooks.
src/codegen/generate-classes.ts (1)
1371-1374: Approve centralized estimatedSize externThe extern declaration is emitted only once in generate-classes.ts; no duplicates remain.
| afterAll(() => console.error("After all RSS", process.memoryUsage.rss() / 1024 / 1024)); | ||
| beforeAll(() => console.error("Before all RSS", process.memoryUsage.rss() / 1024 / 1024)); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Gate debug RSS logs behind an env flag.
These stderr logs can spam CI. Suggest guarding with if (process.env.DEBUG_RSS) to keep test output clean.
-afterAll(() => console.error("After all RSS", process.memoryUsage.rss() / 1024 / 1024));
-beforeAll(() => console.error("Before all RSS", process.memoryUsage.rss() / 1024 / 1024));
+afterAll(() => { if (process.env.DEBUG_RSS) console.error("After all RSS", process.memoryUsage.rss() / 1024 / 1024); });
+beforeAll(() => { if (process.env.DEBUG_RSS) console.error("Before all RSS", process.memoryUsage.rss() / 1024 / 1024); });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| afterAll(() => console.error("After all RSS", process.memoryUsage.rss() / 1024 / 1024)); | |
| beforeAll(() => console.error("Before all RSS", process.memoryUsage.rss() / 1024 / 1024)); | |
| afterAll(() => { if (process.env.DEBUG_RSS) console.error("After all RSS", process.memoryUsage.rss() / 1024 / 1024); }); | |
| beforeAll(() => { if (process.env.DEBUG_RSS) console.error("Before all RSS", process.memoryUsage.rss() / 1024 / 1024); }); |
🤖 Prompt for AI Agents
In test/js/bun/shell/bunshell.test.ts around lines 15-17, the unconditional
console.error calls printing RSS can clutter CI output; guard these debug logs
with a check like if (process.env.DEBUG_RSS) before calling console.error so
they only run when the DEBUG_RSS environment variable is set, leaving the
existing messages intact.
3 participants
Summary
Fixes two critical bugs in Bun Shell:
Memory leaks & incorrect GC reporting: Shell objects weren't reporting their memory usage to JavaScriptCore's garbage collector, causing memory to accumulate unchecked. Also fixes a leak where
ShellArgswasn't being freed inInterpreter.finalize().Blocking I/O on macOS: Fixes a bug where writing large amounts of data (>1MB) to pipes would block the main thread on macOS. The issue:
sendto()withMSG_NOWAITflag blocks on macOS despite the flag, so we now avoid the socket fast path unless the socket is already non-blocking.Changes
memoryCost()andestimatedSize()implementations across shell AST nodes, interpreter, and I/O structuresvm.heap.reportExtraMemoryAllocated()this.args.deinit()call in interpreter finalizationBabyList.memoryCost()to return bytes, not element countTest plan
shell-leak-args.test.ts- validates memory doesn't leak during parsing/executionshell-blocking-pipe.test.ts- validates large pipe writes don't block the main thread🤖 Generated with Claude Code