WIP#23151
Closed
dylan-conway wants to merge 2 commits into
Closed
Conversation
Collaborator
|
Updated 9:06 PM PT - Sep 30th, 2025
❌ @dylan-conway, your commit 302b1e2 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 23151That installs a local version of the PR into your bun-23151 --bun |
Contributor
WalkthroughAdds bun.allocators StackFallbackAllocator and stackFallback factory. Refactors numerous call sites from std.heap.stackFallback to bun.allocators.stackFallback. Introduces a runtime feature flag to force heap fallback in CI. Updates HTTPThread to use the new allocator type for RequestBodyBuffer. No other functional logic changes. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/bun.js/bindings/ZigString.zig (1)
72-87: Potential leak when input >512 bytes in toSliceLowercase; free the temp bufferNested stackFallback alloc + toOwnedSlice can spill to the fallback allocator and isn’t freed. Use toSlice with defer deinit to avoid leaks and keep stack fast-path.
Apply this diff:
pub fn toSliceLowercase(this: ZigString, allocator: std.mem.Allocator) Slice { if (this.len == 0) return Slice.empty; - var fallback = bun.allocators.stackFallback(512, allocator); - const fallback_allocator = fallback.get(); - - const uppercase_buffer = this.toOwnedSlice(fallback_allocator) catch unreachable; - const buffer = allocator.alloc(u8, uppercase_buffer.len) catch unreachable; - const out = strings.copyLowercase(uppercase_buffer, buffer); + var fallback = bun.allocators.stackFallback(512, allocator); + const fb_alloc = fallback.get(); + // Get a temporary slice; it will be freed on deinit() even if it overflowed the stack buffer. + var tmp = this.toSlice(fb_alloc); + defer tmp.deinit(); + const buffer = allocator.alloc(u8, tmp.len) catch unreachable; + const out = strings.copyLowercase(tmp.slice(), buffer); return Slice{ .allocator = NullableAllocator.init(allocator), .ptr = out.ptr, .len = @as(u32, @truncate(out.len)), }; }src/bake/DevServer.zig (1)
1590-1628: Bug: returning freed memory from ArrayListUnmanaged when allocation falls back to heap.array.items is returned while defer array.deinit(sfa) still runs. If the allocation came from the fallback allocator, deinit frees the buffer before return. Use toOwnedSlice() for the heap path and only dupe when on the stack buffer.
Apply this diff:
var sfa_state = bun.allocators.stackFallback(65536, dev.allocator()); const sfa = sfa_state.get(); var array = bun.handleOom(std.ArrayListUnmanaged(u8).initCapacity(sfa, 65536)); defer array.deinit(sfa); @@ -// Avoid-recloning if it is was moved to the heap -return if (array.items.ptr == &sfa_state.buffer) - try dev.allocator().dupe(u8, array.items) -else - array.items; +// If allocated on the stack buffer, duplicate into dev allocator. +// Otherwise transfer ownership without freeing here. +if (bun.allocators.isSliceInBuffer(array.items, sfa_state.buffer[0..])) { + return try dev.allocator().dupe(u8, array.items); +} +return try array.toOwnedSlice(sfa);
♻️ Duplicate comments (6)
src/deps/c_ares.zig (6)
940-946: Same allocator pattern here; OK.Repeat of the safe pattern; consider using the shared constant as above.
1023-1028: Same allocator pattern; OK.No issues; prefer shared constant for the stack size.
1097-1102: Same allocator pattern; OK.Looks good; reuse the constant.
1199-1204: Same allocator pattern; OK.All good; consider centralizing 2048.
1292-1296: Same allocator pattern; OK.No concerns; reuse constant.
1375-1380: Same allocator pattern; OK.Pattern remains safe for struct_any_reply; constantize size as above.
🧹 Nitpick comments (10)
src/shell/IOWriter.zig (1)
433-463: Allocator change LGTM; consider sizing hint.Swap to bun.allocators.stackFallback is fine. Optionally, pre-size “seen” by min(this.writers.len(), 64) to reduce early heap fallback in large batches.
src/bun.js/webcore/Blob.zig (1)
3891-3898: Always deinit the ArrayList; avoid accessing allocator internals
Remove the manual end_index check and callstack.deinit()unconditionally—free()is a no-op for static-buffer allocations and properly frees any heap fallback.- defer if (stack_allocator.fixed_buffer_allocator.end_index >= 1024) stack.deinit(); + defer stack.deinit();src/bun.js/bindings/ZigString.zig (1)
62-70: Optional: simplify allocator usage in eqlCaseInsensitive
You can passbun.default_allocatordirectly totoSliceLowercasein both calls to avoid unnecessary nested fallback allocators.src/deps/c_ares.zig (1)
862-867: Allocator migration LGTM; consider centralizing stack size.Using bun.allocators.stackFallback with a scoped Arena is correct and bounded. Minor: extract 2048 into a file-level constant to avoid magic numbers and ease tuning.
Example:
+const TOJS_STACK_BYTES: usize = 2048; … - var stack = bun.allocators.stackFallback(2048, parent_allocator); + var stack = bun.allocators.stackFallback(TOJS_STACK_BYTES, parent_allocator);src/shell/shell.zig (3)
1590-1592: Safe, but bump the inline capacity to reduce immediate reallocs.Current fallback buffer fits exactly 1 SimpleAtom; compound atoms (common) trigger an immediate realloc to self.alloc. Consider starting with 4 to amortize growth without meaningfully increasing stack usage.
- var array_alloc = bun.allocators.stackFallback(@sizeOf(AST.SimpleAtom), self.alloc); - var atoms = try std.ArrayList(AST.SimpleAtom).initCapacity(array_alloc.get(), 1); + const inline_atoms = 4; + var array_alloc = bun.allocators.stackFallback(@sizeOf(AST.SimpleAtom) * inline_atoms, self.alloc); + var atoms = try std.ArrayList(AST.SimpleAtom).initCapacity(array_alloc.get(), inline_atoms);
4319-4321: LGTM; lifetime ordering is correct.jsstrings uses a stack-fallback allocator with capacity 4 and deinit runs before arena.deinit; safe. If you expect more than 4 substitutions in tests, consider a slightly larger inline to avoid parent allocations.
4387-4389: LGTM; consistent with shellLex.Same pattern with bun.ArenaAllocator; safe and consistent.
src/allocators.zig (1)
959-974: Good design on vtable-backed wrapper; small polish suggestions.
- Consider documenting that get() must be called once per instance and is not thread-safe; you enforce single-call only in CI builds. A brief comment helps avoid misuse.
- Optional: expose a helper in bun.allocators like inBuffer(self, ptr) to centralize ownsPtr logic for external call sites that need to check stack vs heap.
Also applies to: 975-989, 991-1009, 1011-1029, 1031-1049
src/bun.js/node/node_fs.zig (2)
1066-1066: Optional: name the stack buffer size for clarity and easy tuning.Consider a local const to avoid magic number 8192 and mirror other uses.
- var stack = bun.allocators.stackFallback(8192, bun.default_allocator); + const entries_stack_bytes: usize = 8192; + var stack = bun.allocators.stackFallback(entries_stack_bytes, bun.default_allocator);
4659-4666: Optional: hoist byte sizes to named consts for consistency across both stacks.This aids future tuning (CI knob) and keeps sizes in one place.
- var iterator_stack = bun.allocators.stackFallback(128, bun.default_allocator); + const iterator_stack_bytes: usize = 128; + var iterator_stack = bun.allocators.stackFallback(iterator_stack_bytes, bun.default_allocator); var stack = std.fifo.LinearFifo([:0]const u8, .{ .Dynamic = {} }).init(iterator_stack.get()); - var basename_stack = bun.allocators.stackFallback(8192 * 2, bun.default_allocator); + const basename_stack_bytes: usize = 8192 * 2; + var basename_stack = bun.allocators.stackFallback(basename_stack_bytes, bun.default_allocator);As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
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 (92)
src/allocators.zig(1 hunks)src/ast/P.zig(1 hunks)src/ast/Parser.zig(1 hunks)src/bake/DevServer.zig(14 hunks)src/bake/DevServer/ErrorReportRequest.zig(1 hunks)src/bake/DevServer/HotReloadEvent.zig(1 hunks)src/bake/DevServer/IncrementalGraph.zig(2 hunks)src/bake/DevServer/SerializedFailure.zig(2 hunks)src/bake/FrameworkRouter.zig(3 hunks)src/bake/production.zig(4 hunks)src/bun.js/ConsoleObject.zig(1 hunks)src/bun.js/api/BunObject.zig(2 hunks)src/bun.js/api/JSTranspiler.zig(1 hunks)src/bun.js/api/bun/process.zig(2 hunks)src/bun.js/api/bun/socket.zig(2 hunks)src/bun.js/api/bun/socket/SocketAddress.zig(1 hunks)src/bun.js/api/server.zig(2 hunks)src/bun.js/api/server/NodeHTTPResponse.zig(1 hunks)src/bun.js/bindings/JSGlobalObject.zig(7 hunks)src/bun.js/bindings/JSValue.zig(2 hunks)src/bun.js/bindings/NodeModuleModule.zig(2 hunks)src/bun.js/bindings/ZigString.zig(2 hunks)src/bun.js/node/node_assert_binding.zig(1 hunks)src/bun.js/node/node_fs.zig(2 hunks)src/bun.js/node/node_os.zig(1 hunks)src/bun.js/node/node_process.zig(3 hunks)src/bun.js/node/node_util_binding.zig(1 hunks)src/bun.js/node/path.zig(10 hunks)src/bun.js/node/util/parse_args.zig(1 hunks)src/bun.js/test/expect.zig(5 hunks)src/bun.js/virtual_machine_exports.zig(1 hunks)src/bun.js/webcore/Blob.zig(3 hunks)src/bun.js/webcore/prompt.zig(3 hunks)src/bundler/HTMLImportManifest.zig(1 hunks)src/bundler/LinkerContext.zig(1 hunks)src/bundler/bundle_v2.zig(1 hunks)src/bundler/linker_context/computeChunks.zig(1 hunks)src/bundler/linker_context/generateCompileResultForHtmlChunk.zig(2 hunks)src/bundler/linker_context/scanImportsAndExports.zig(1 hunks)src/cli/filter_run.zig(1 hunks)src/cli/run_command.zig(1 hunks)src/cli/test_command.zig(4 hunks)src/crash_handler.zig(1 hunks)src/css/css_modules.zig(1 hunks)src/css/css_parser.zig(1 hunks)src/css/properties/font.zig(1 hunks)src/css/values/color_js.zig(1 hunks)src/deps/c_ares.zig(7 hunks)src/deps/uws/socket.zig(3 hunks)src/deps/uws/us_socket_t.zig(1 hunks)src/dns.zig(1 hunks)src/env_loader.zig(1 hunks)src/feature_flags.zig(1 hunks)src/fs.zig(1 hunks)src/glob/GlobWalker.zig(1 hunks)src/http.zig(2 hunks)src/http/HTTPThread.zig(2 hunks)src/ini.zig(2 hunks)src/install/NetworkTask.zig(1 hunks)src/install/PackageInstall.zig(1 hunks)src/install/PackageInstaller.zig(1 hunks)src/install/PackageManager/PackageManagerLifecycle.zig(1 hunks)src/install/PackageManager/PackageManagerResolution.zig(1 hunks)src/install/PackageManager/UpdateRequest.zig(1 hunks)src/install/PackageManager/install_with_manager.zig(1 hunks)src/install/PackageManager/patchPackage.zig(3 hunks)src/install/dependency.zig(1 hunks)src/install/lockfile/Package.zig(1 hunks)src/install/npm.zig(1 hunks)src/install/repository.zig(1 hunks)src/install/yarn.zig(1 hunks)src/interchange/toml.zig(2 hunks)src/io/io.zig(1 hunks)src/js_printer.zig(4 hunks)src/patch.zig(1 hunks)src/resolver/resolver.zig(1 hunks)src/s3/credentials.zig(2 hunks)src/semver/SemverObject.zig(2 hunks)src/shell/IOWriter.zig(1 hunks)src/shell/ParsedShellScript.zig(1 hunks)src/shell/Yield.zig(1 hunks)src/shell/braces.zig(2 hunks)src/shell/shell.zig(3 hunks)src/shell/states/Expansion.zig(1 hunks)src/sourcemap/LineOffsetTable.zig(1 hunks)src/sourcemap/sourcemap.zig(2 hunks)src/sql/mysql/MySQLTypes.zig(1 hunks)src/sql/mysql/js/JSMySQLConnection.zig(1 hunks)src/sql/postgres/DataCell.zig(1 hunks)src/string.zig(1 hunks)src/string/immutable.zig(1 hunks)src/valkey/js_valkey_functions.zig(9 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/bun.js/api/bun/socket/SocketAddress.zigsrc/bun.js/bindings/ZigString.zigsrc/bun.js/api/server/NodeHTTPResponse.zigsrc/bun.js/node/node_assert_binding.zigsrc/bake/DevServer/SerializedFailure.zigsrc/feature_flags.zigsrc/shell/states/Expansion.zigsrc/sql/mysql/js/JSMySQLConnection.zigsrc/cli/run_command.zigsrc/bun.js/bindings/NodeModuleModule.zigsrc/bun.js/node/node_os.zigsrc/install/PackageInstall.zigsrc/install/PackageManager/install_with_manager.zigsrc/install/PackageManager/UpdateRequest.zigsrc/shell/braces.zigsrc/crash_handler.zigsrc/env_loader.zigsrc/shell/Yield.zigsrc/css/css_modules.zigsrc/ini.zigsrc/bundler/linker_context/scanImportsAndExports.zigsrc/css/properties/font.zigsrc/patch.zigsrc/valkey/js_valkey_functions.zigsrc/interchange/toml.zigsrc/bun.js/api/BunObject.zigsrc/semver/SemverObject.zigsrc/bake/FrameworkRouter.zigsrc/bun.js/node/node_util_binding.zigsrc/resolver/resolver.zigsrc/string/immutable.zigsrc/bun.js/bindings/JSValue.zigsrc/http/HTTPThread.zigsrc/bun.js/bindings/JSGlobalObject.zigsrc/bake/DevServer/IncrementalGraph.zigsrc/bundler/HTMLImportManifest.zigsrc/bun.js/test/expect.zigsrc/install/lockfile/Package.zigsrc/deps/c_ares.zigsrc/glob/GlobWalker.zigsrc/s3/credentials.zigsrc/bundler/linker_context/generateCompileResultForHtmlChunk.zigsrc/bun.js/node/util/parse_args.zigsrc/bundler/bundle_v2.zigsrc/bake/production.zigsrc/bun.js/virtual_machine_exports.zigsrc/install/npm.zigsrc/install/dependency.zigsrc/install/NetworkTask.zigsrc/bun.js/api/bun/process.zigsrc/shell/ParsedShellScript.zigsrc/io/io.zigsrc/bun.js/ConsoleObject.zigsrc/bun.js/webcore/prompt.zigsrc/sql/mysql/MySQLTypes.zigsrc/deps/uws/us_socket_t.zigsrc/bake/DevServer/ErrorReportRequest.zigsrc/ast/Parser.zigsrc/dns.zigsrc/shell/shell.zigsrc/shell/IOWriter.zigsrc/cli/filter_run.zigsrc/bun.js/api/bun/socket.zigsrc/bun.js/api/server.zigsrc/cli/test_command.zigsrc/allocators.zigsrc/bun.js/node/node_fs.zigsrc/install/yarn.zigsrc/install/PackageManager/PackageManagerResolution.zigsrc/fs.zigsrc/bun.js/node/path.zigsrc/bundler/linker_context/computeChunks.zigsrc/install/PackageManager/PackageManagerLifecycle.zigsrc/bake/DevServer/HotReloadEvent.zigsrc/deps/uws/socket.zigsrc/install/repository.zigsrc/js_printer.zigsrc/bun.js/api/JSTranspiler.zigsrc/css/values/color_js.zigsrc/install/PackageManager/patchPackage.zigsrc/sourcemap/sourcemap.zigsrc/http.zigsrc/sourcemap/LineOffsetTable.zigsrc/bun.js/webcore/Blob.zigsrc/bun.js/node/node_process.zigsrc/install/PackageInstaller.zigsrc/ast/P.zigsrc/sql/postgres/DataCell.zigsrc/css/css_parser.zigsrc/bundler/LinkerContext.zigsrc/string.zigsrc/bake/DevServer.zig
**/*.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
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/bun.js/api/bun/socket/SocketAddress.zigsrc/bun.js/bindings/ZigString.zigsrc/bun.js/api/server/NodeHTTPResponse.zigsrc/bun.js/node/node_assert_binding.zigsrc/bake/DevServer/SerializedFailure.zigsrc/feature_flags.zigsrc/shell/states/Expansion.zigsrc/sql/mysql/js/JSMySQLConnection.zigsrc/cli/run_command.zigsrc/bun.js/bindings/NodeModuleModule.zigsrc/bun.js/node/node_os.zigsrc/install/PackageInstall.zigsrc/install/PackageManager/install_with_manager.zigsrc/install/PackageManager/UpdateRequest.zigsrc/shell/braces.zigsrc/crash_handler.zigsrc/env_loader.zigsrc/shell/Yield.zigsrc/css/css_modules.zigsrc/ini.zigsrc/bundler/linker_context/scanImportsAndExports.zigsrc/css/properties/font.zigsrc/patch.zigsrc/valkey/js_valkey_functions.zigsrc/interchange/toml.zigsrc/bun.js/api/BunObject.zigsrc/semver/SemverObject.zigsrc/bake/FrameworkRouter.zigsrc/bun.js/node/node_util_binding.zigsrc/resolver/resolver.zigsrc/string/immutable.zigsrc/bun.js/bindings/JSValue.zigsrc/http/HTTPThread.zigsrc/bun.js/bindings/JSGlobalObject.zigsrc/bake/DevServer/IncrementalGraph.zigsrc/bundler/HTMLImportManifest.zigsrc/bun.js/test/expect.zigsrc/install/lockfile/Package.zigsrc/deps/c_ares.zigsrc/glob/GlobWalker.zigsrc/s3/credentials.zigsrc/bundler/linker_context/generateCompileResultForHtmlChunk.zigsrc/bun.js/node/util/parse_args.zigsrc/bundler/bundle_v2.zigsrc/bake/production.zigsrc/bun.js/virtual_machine_exports.zigsrc/install/npm.zigsrc/install/dependency.zigsrc/install/NetworkTask.zigsrc/bun.js/api/bun/process.zigsrc/shell/ParsedShellScript.zigsrc/io/io.zigsrc/bun.js/ConsoleObject.zigsrc/bun.js/webcore/prompt.zigsrc/sql/mysql/MySQLTypes.zigsrc/deps/uws/us_socket_t.zigsrc/bake/DevServer/ErrorReportRequest.zigsrc/ast/Parser.zigsrc/dns.zigsrc/shell/shell.zigsrc/shell/IOWriter.zigsrc/cli/filter_run.zigsrc/bun.js/api/bun/socket.zigsrc/bun.js/api/server.zigsrc/cli/test_command.zigsrc/allocators.zigsrc/bun.js/node/node_fs.zigsrc/install/yarn.zigsrc/install/PackageManager/PackageManagerResolution.zigsrc/fs.zigsrc/bun.js/node/path.zigsrc/bundler/linker_context/computeChunks.zigsrc/install/PackageManager/PackageManagerLifecycle.zigsrc/bake/DevServer/HotReloadEvent.zigsrc/deps/uws/socket.zigsrc/install/repository.zigsrc/js_printer.zigsrc/bun.js/api/JSTranspiler.zigsrc/css/values/color_js.zigsrc/install/PackageManager/patchPackage.zigsrc/sourcemap/sourcemap.zigsrc/http.zigsrc/sourcemap/LineOffsetTable.zigsrc/bun.js/webcore/Blob.zigsrc/bun.js/node/node_process.zigsrc/install/PackageInstaller.zigsrc/ast/P.zigsrc/sql/postgres/DataCell.zigsrc/css/css_parser.zigsrc/bundler/LinkerContext.zigsrc/string.zigsrc/bake/DevServer.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/bun/socket/SocketAddress.zigsrc/bun.js/api/server/NodeHTTPResponse.zigsrc/valkey/js_valkey_functions.zigsrc/bun.js/api/BunObject.zigsrc/bun.js/api/bun/process.zigsrc/bun.js/api/bun/socket.zigsrc/bun.js/api/server.zigsrc/js_printer.zigsrc/bun.js/api/JSTranspiler.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/api/bun/socket/SocketAddress.zigsrc/bun.js/bindings/ZigString.zigsrc/bun.js/api/server/NodeHTTPResponse.zigsrc/bun.js/node/node_assert_binding.zigsrc/bun.js/bindings/NodeModuleModule.zigsrc/bun.js/node/node_os.zigsrc/bun.js/api/BunObject.zigsrc/bun.js/node/node_util_binding.zigsrc/bun.js/bindings/JSValue.zigsrc/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/test/expect.zigsrc/bun.js/node/util/parse_args.zigsrc/bun.js/virtual_machine_exports.zigsrc/bun.js/api/bun/process.zigsrc/bun.js/ConsoleObject.zigsrc/bun.js/webcore/prompt.zigsrc/bun.js/api/bun/socket.zigsrc/bun.js/api/server.zigsrc/bun.js/node/node_fs.zigsrc/bun.js/node/path.zigsrc/bun.js/api/JSTranspiler.zigsrc/bun.js/webcore/Blob.zigsrc/bun.js/node/node_process.zig
src/**/js_*.zig
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
src/**/js_*.zig: Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Always implement proper cleanup in deinit() and finalize() for JS-exposed types
Files:
src/valkey/js_valkey_functions.zigsrc/js_printer.zig
src/bun.js/api/BunObject.zig
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Create a getter function in BunObject.zig (define const Getter with toJSGetter and export it in exportAll)
Files:
src/bun.js/api/BunObject.zig
🧠 Learnings (11)
📚 Learning: 2025-09-02T18:27:23.279Z
Learnt from: taylordotfish
PR: oven-sh/bun#22227
File: src/collections/multi_array_list.zig:24-24
Timestamp: 2025-09-02T18:27:23.279Z
Learning: The `#allocator` syntax in bun's custom Zig implementation is valid and does not require quoting with @"#allocator". Private fields using the `#` prefix work correctly throughout the codebase without special quoting syntax.
Applied to files:
src/bun.js/bindings/ZigString.zigsrc/bun.js/api/BunObject.zigsrc/bun.js/node/node_util_binding.zigsrc/bun.js/test/expect.zigsrc/bun.js/node/util/parse_args.zigsrc/bundler/bundle_v2.zigsrc/install/npm.zigsrc/install/dependency.zigsrc/ast/P.zig
📚 Learning: 2025-09-02T17:09:17.391Z
Learnt from: taylordotfish
PR: oven-sh/bun#22227
File: src/allocators.zig:778-782
Timestamp: 2025-09-02T17:09:17.391Z
Learning: In Zig's bun codebase, the `isDefault` function in `src/allocators.zig` only needs to compare `allocator.vtable == c_allocator.vtable` rather than also checking the `ptr` field, because: (1) the codebase never creates multiple allocators that use `c_allocator.vtable` but have different `ptr`s, and (2) the default allocator vtable ignores the `ptr` field anyway, so any allocators sharing the same vtable would function identically.
Applied to files:
src/bun.js/bindings/ZigString.zig
📚 Learning: 2025-09-03T05:09:24.272Z
Learnt from: Jarred-Sumner
PR: oven-sh/bun#22335
File: src/http.zig:1376-1393
Timestamp: 2025-09-03T05:09:24.272Z
Learning: In bun's HTTP module, `bun.http.default_allocator` is the correct allocator to use for internal HTTP buffers like MutableString initialization for response_message_buffer and compressed_body. This allocator is initialized in HTTPThread.zig as `bun.http.default_arena.allocator()` and is used consistently across the HTTP module, separate from any local `default_allocator` variables.
Applied to files:
src/bun.js/api/server/NodeHTTPResponse.zigsrc/http/HTTPThread.zigsrc/install/dependency.zigsrc/http.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/bun.js/node/node_assert_binding.zigsrc/bun.js/bindings/NodeModuleModule.zigsrc/bun.js/node/node_util_binding.zigsrc/bun.js/bindings/JSValue.zigsrc/bun.js/api/JSTranspiler.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
src/bun.js/bindings/NodeModuleModule.zigsrc/bun.js/bindings/JSValue.zigsrc/bun.js/api/JSTranspiler.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/bun.js/bindings/NodeModuleModule.zigsrc/bun.js/bindings/JSValue.zigsrc/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/api/JSTranspiler.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
Applied to files:
src/semver/SemverObject.zigsrc/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/bun.js/bindings/JSValue.zigsrc/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/api/JSTranspiler.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/bun.js/api/JSTranspiler.zig
📚 Learning: 2025-09-08T04:44:59.101Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.101Z
Learning: Applies to **/*.zig : In Zig, manage memory carefully with allocators and use defer for cleanup
Applied to files:
src/ast/P.zig
⏰ 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 (103)
src/css/properties/font.zig (1)
940-940: LGTM! API migration looks correct.The change from
std.heap.stackFallbacktobun.allocators.stackFallbackis straightforward and maintains the same usage pattern. The memory management is correct with proper cleanup viadefer seen.deinit(alloc)on line 943. The 664-byte stack buffer size is reasonable for the deduplication set.src/fs.zig (1)
155-156: LGTM! Refactoring aligns with PR objectives.The change from
std.heap.stackFallbacktobun.allocators.stackFallbackis straightforward and maintains the same usage pattern. The stack size (512 bytes) and fallback allocator remain unchanged, and the cleanup viadeferon line 158 ensures proper resource management.src/bun.js/bindings/NodeModuleModule.zig (2)
9-9: LGTM! Consistent refactor to bun's allocator pathway.The change from
std.heap.stackFallbacktobun.allocators.stackFallbackis straightforward and maintains the same API. The subsequent usage pattern (.get(), UTF-8 conversion, defer cleanup) remains unchanged.
166-166: LGTM! Consistent refactor matching the pattern infindPath.This change mirrors the refactor on line 9, replacing
std.heap.stackFallbackwithbun.allocators.stackFallback. The usage pattern and error handling remain identical.src/sql/postgres/DataCell.zig (1)
561-561: LGTM! Clean API migration tobun.allocators.stackFallback.The refactor from
std.heap.stackFallbacktobun.allocators.stackFallbackis straightforward and aligns with the PR objective to test stack fallback allocators in CI. The usage pattern remains unchanged, and memory management is correctly handled withdefer numeric_buffer.deinit().src/io/io.zig (1)
198-198: Approve allocator replacement
The functionbun.allocators.stackFallbackis declared at line 931 ofsrc/allocators.zigand correctly imported insrc/io/io.zig, matching the originalstd.heap.stackFallbacksignature.src/shell/ParsedShellScript.zig (1)
117-117: Approve replacement of std.heap.stackFallback with bun.allocators.stackFallback.The new
stackFallbackinsrc/allocators.zighas the same comptime size and fallback allocator signature and returns the identicalStackFallbackAllocatortype—usage in ParsedShellScript.zig remains fully compatible. No changes needed.src/env_loader.zig (1)
527-527: ```diff
Verifybun.allocators.stackFallbackimport and API compatibility in `src/env_loader.zig`The refactor replacing
std.heap.stackFallbackwithbun.allocators.stackFallbackis correct. Ensure that:
src/env_loader.zigimports the root module that re-exportsallocators(e.g.,const bun = @import("bun");).- The signature
pub fn stackFallback(comptime size: usize, fallback_allocator: std.mem.Allocator) StackFallbackAllocator(size)aligns with the previousstd.heap.stackFallbackAPI (it does).No further code changes needed here if imports are present and correct.
</blockquote></details> <details> <summary>src/sql/mysql/MySQLTypes.zig (1)</summary><blockquote> `784-801`: **Allocator swap is correct; lifetime safe.** Using bun.allocators.stackFallback with defered ArrayList deinit is sound; heap fallback covers longer decimals. LGTM. </blockquote></details> <details> <summary>src/bundler/bundle_v2.zig (1)</summary><blockquote> `372-381`: **Good switch and scope management.** Using bun.allocators.stackFallback for temp bitsets with proper deinit on stack_alloc is correct. No concerns. </blockquote></details> <details> <summary>src/deps/uws/us_socket_t.zig (1)</summary><blockquote> `294-306`: **Allocator swap is appropriate for this hot path.** Short-lived stackFallback with deferred node_buffer.deinit is safe; heap fallback handles large payloads. Looks good. </blockquote></details> <details> <summary>src/s3/credentials.zig (1)</summary><blockquote> `786-786`: **LGTM! Clean allocator migration.** The two stack fallback allocators used for AWS Signature V4 query parameter construction are correctly migrated to `bun.allocators.stackFallback`. The usage pattern with `.get()` remains unchanged. Also applies to: 837-837 </blockquote></details> <details> <summary>src/install/PackageInstaller.zig (1)</summary><blockquote> `741-741`: **LGTM! Correct allocator update.** The stack fallback allocator used for constructing the temporary `name_and_version` string in patch dependency handling is correctly migrated to `bun.allocators.stackFallback`. </blockquote></details> <details> <summary>src/bun.js/node/util/parse_args.zig (1)</summary><blockquote> `681-681`: **LGTM! Consistent allocator migration.** The stack fallback allocator for storing option definitions during Node.js `parseArgs` processing is correctly updated to use `bun.allocators.stackFallback`. </blockquote></details> <details> <summary>src/bun.js/node/node_assert_binding.zig (1)</summary><blockquote> `11-11`: **LGTM! Myers diff allocator updated correctly.** The stack fallback allocator used as backing for the arena allocator in the Myers diff implementation is correctly migrated to `bun.allocators.stackFallback`. </blockquote></details> <details> <summary>src/bundler/linker_context/computeChunks.zig (1)</summary><blockquote> `10-10`: **LGTM! Bundler chunk computation allocator migrated correctly.** The stack fallback allocator used for temporary allocations during chunk computation is correctly updated to use `bun.allocators.stackFallback`. This backs the arena allocator for temporary data structures like chunk maps. </blockquote></details> <details> <summary>src/cli/filter_run.zig (1)</summary><blockquote> `585-585`: **LGTM! Consistent with the PR's refactoring objective.** This change correctly replaces `std.heap.stackFallback` with `bun.allocators.stackFallback` while maintaining the same buffer size (256 bytes) and fallback allocator. The usage pattern and surrounding logic remain unchanged. </blockquote></details> <details> <summary>src/sourcemap/sourcemap.zig (2)</summary><blockquote> `1064-1064`: **LGTM! Proper stack fallback allocation for source map parsing.** The change from `std.heap.stackFallback` to `bun.allocators.stackFallback` is correct. The 1024-byte stack allocation is appropriate for temporary arena allocations during source map parsing, with a safe fallback to `bun.default_allocator`. --- `1477-1477`: **LGTM! Appropriate stack allocation for source map joining.** The change correctly migrates to `bun.allocators.stackFallback`. The 16KB stack allocation is reasonable for the StringJoiner's node allocator, as noted in the comment explaining it covers VLQ encodings and join nodes (but not JSON payloads or source code). </blockquote></details> <details> <summary>src/shell/states/Expansion.zig (1)</summary><blockquote> `238-238`: **LGTM! Safe stack allocation for brace expansion.** The migration to `bun.allocators.stackFallback` is correct. The stack allocation size is properly bounded by the comptime assertion (line 236) ensuring it stays within 256 bytes, and the arena allocator is an appropriate fallback for this temporary allocation. </blockquote></details> <details> <summary>src/glob/GlobWalker.zig (1)</summary><blockquote> `646-646`: **LGTM! Correct stack allocation for path optimization.** The change to `bun.allocators.stackFallback` is appropriate for this optimization path. The 256-byte stack allocation is sufficient for duplicating path components during the literal pattern matching optimization, with a proper fallback to the walker's arena allocator. </blockquote></details> <details> <summary>src/install/repository.zig (1)</summary><blockquote> `34-34`: **LGTM! Appropriate stack allocation for config parsing.** The migration to `bun.allocators.stackFallback` is correct. The 4KB stack allocation is suitable for parsing the `.gitconfig` file contents, with proper fallback to `bun.default_allocator`. The allocator usage is properly scoped with deferred cleanup (line 39). </blockquote></details> <details> <summary>src/sourcemap/LineOffsetTable.zig (1)</summary><blockquote> `87-87`: **LGTM! Clean allocator migration.** The switch from `std.heap.stackFallback` to `bun.allocators.stackFallback` is consistent with the PR's objective to centralize allocator management for testing purposes. </blockquote></details> <details> <summary>src/bake/DevServer/IncrementalGraph.zig (2)</summary><blockquote> `881-881`: **LGTM! Consistent allocator migration.** The replacement aligns with the PR's allocator standardization. The stack size and usage pattern remain unchanged. --- `1731-1731`: **LGTM! Allocator migration applied correctly.** The switch to `bun.allocators.stackFallback` maintains the same behavior while enabling centralized allocator control for CI testing. </blockquote></details> <details> <summary>src/bun.js/virtual_machine_exports.zig (1)</summary><blockquote> `170-170`: **LGTM! Systematic allocator migration across VM exports.** All five source provider functions consistently switched to `bun.allocators.stackFallback`. The pattern is uniform and maintains the same buffer sizes and fallback behavior. Also applies to: 177-177, 184-184, 191-191, 198-198 </blockquote></details> <details> <summary>src/feature_flags.zig (1)</summary><blockquote> `7-7`: **LGTM! Feature flag added for CI allocator testing.** The new `BUN_DEBUG_FORCE_HEAP_FALLBACK_ALLOCATORS` flag follows the established naming convention and enables the runtime testing mentioned in the PR objectives. </blockquote></details> <details> <summary>src/bun.js/api/bun/socket/SocketAddress.zig (1)</summary><blockquote> `213-213`: **LGTM! Allocator migration in socket address initialization.** The switch to `bun.allocators.stackFallback` is consistent with the PR's allocator centralization. The 64-byte stack buffer size is appropriate for temporary address string formatting. </blockquote></details> <details> <summary>src/install/lockfile/Package.zig (1)</summary><blockquote> `1644-1648`: **Allocator swap aligns with bun fallback helper.** `bun.allocators.stackFallback` is a drop-in for the previous std helper here, so the temporary allocator behavior stays the same. </blockquote></details> <details> <summary>src/bun.js/api/bun/process.zig (2)</summary><blockquote> `1267-1274`: **Temp allocator swap looks good.** The new `bun.allocators.stackFallback` keeps the short-lived list allocations on the same strategy without affecting cleanup paths. --- `1546-1555`: **Same story for the Windows path.** The stack fallback allocator transition is consistent with the POSIX path and preserves the existing ownership expectations. </blockquote></details> <details> <summary>src/deps/uws/socket.zig (1)</summary><blockquote> `472-474`: **Consistent allocator migration.** All three call sites now use `bun.allocators.stackFallback`, matching the project-wide shift while keeping the short-lived dupes scoped the same way. Also applies to: 579-583, 603-605 </blockquote></details> <details> <summary>src/shell/braces.zig (1)</summary><blockquote> `247-250`: **Parser scratch allocator update LGTM.** The new helper provides the same stack-backed allocator used to build temporary AST nodes, so nothing else needs to change. Also applies to: 278-281 </blockquote></details> <details> <summary>src/bundler/linker_context/generateCompileResultForHtmlChunk.zig (1)</summary><blockquote> `136-143`: **HTML tag builder uses new stack fallback cleanly.** The temporary allocations still free through the returned allocator, so behavior matches the previous std helper. Also applies to: 239-244 </blockquote></details> <details> <summary>src/bun.js/webcore/Blob.zig (2)</summary><blockquote> `574-579`: **Allocator swap looks correct for FormData assembly.** Switch to bun.allocators.stackFallback(1024, arena.allocator()) with get() is appropriate; joiner’s done(allocator) still frees to the provided fallback via arena. LGTM. --- `3578-3587`: **Allocator swap in toJSONWithBytes is fine.** Using bun.allocators.stackFallback(4096, bun.default_allocator) for the transient UTF‑16 buffer preserves semantics; freeing external via allocator.free(external) remains safe. </blockquote></details> <details> <summary>src/bun.js/node/path.zig (10)</summary><blockquote> `437-452`: **basename(): allocator swap is correct.** Using bun.allocators.stackFallback(...).get() for ZigString slices matches prior behavior. LGTM. --- `626-632`: **dirname(): allocator swap is correct.** Temporary slice allocation via stack fallback remains safe; deinit handled. LGTM. --- `822-828`: **extname(): allocator swap is correct.** Behavior preserved; no lifetime issues. LGTM. --- `930-974`: **format(): allocator swap is correct.** Stack fallback for transient “root/dir/base/name/ext” slices is fine; frees are paired. LGTM. --- `1641-1647`: **normalize(): allocator swap is correct.** Transient slice allocation remains scoped; semantics unchanged. LGTM. --- `1961-1967`: **parse(): allocator swap is correct.** Stack fallback usage for path slice is fine; deinit paired. LGTM. --- `2323-2331`: **relative(): allocator swap is correct.** Temporary slices and lifetime handling unchanged. LGTM. --- `2767-2799`: **resolve(): arena-backed fallback is okay; lifetime is scoped to function.** Using arena as fallback for paths mirrors join(); overall deallocation via arena.deinit(). LGTM. --- `2909-2915`: **toNamespacedPath(): allocator swap is correct.** Transient slice allocation preserved; behavior unchanged. LGTM. --- `1232-1245`: **approve code changes: arena-backed fallback safe** allocator.free(paths) is a no-op when using the arena allocator and arena.deinit() releases all memory; no frees of individual toSlice() buffers were found. </blockquote></details> <details> <summary>src/bun.js/test/expect.zig (1)</summary><blockquote> `1144-1144`: **LGTM! Allocator refactor is consistent.** All stack fallback allocator changes follow the same pattern: replacing `std.heap.stackFallback` with `bun.allocators.stackFallback` while preserving the same usage semantics (size, backing allocator, `.get()` call, and subsequent usage). Also applies to: 1697-1697, 1731-1731, 1747-1747, 1827-1827 </blockquote></details> <details> <summary>src/bun.js/node/node_process.zig (1)</summary><blockquote> `47-47`: **LGTM! Allocator refactor is consistent.** The stack fallback allocator changes are correct and follow the established pattern across all three locations in this file. Also applies to: 147-147, 299-299 </blockquote></details> <details> <summary>src/semver/SemverObject.zig (1)</summary><blockquote> `39-39`: **LGTM! Allocator refactor is consistent.** Both changes in the `order` and `satisfies` functions correctly replace `std.heap.stackFallback` with `bun.allocators.stackFallback` while maintaining the same usage pattern. Also applies to: 85-85 </blockquote></details> <details> <summary>src/shell/Yield.zig (1)</summary><blockquote> `94-94`: **LGTM! Allocator refactor is consistent.** The change correctly replaces `std.heap.stackFallback` with `bun.allocators.stackFallback` while maintaining the same usage pattern for the pipeline stack. </blockquote></details> <details> <summary>src/install/dependency.zig (1)</summary><blockquote> `1283-1283`: **LGTM! Allocator refactor is consistent.** The change correctly replaces `std.heap.stackFallback` with `bun.allocators.stackFallback` in the `fromJS` function while preserving the same usage semantics. </blockquote></details> <details> <summary>src/dns.zig (1)</summary><blockquote> `405-405`: **LGTM!** The replacement of `std.heap.stackFallback` with `bun.allocators.stackFallback` is a straightforward refactor. The 512-byte stack allocation is appropriate for IPv6 address formatting. </blockquote></details> <details> <summary>src/install/PackageManager/PackageManagerLifecycle.zig (1)</summary><blockquote> `93-93`: **LGTM!** The allocator replacement is correct. The 1024-byte stack buffer is appropriate for temporary package name and version string formatting. </blockquote></details> <details> <summary>src/install/PackageManager/PackageManagerResolution.zig (1)</summary><blockquote> `89-89`: **LGTM!** The allocator source change is appropriate. Using a 4KB stack buffer with arena fallback is a good pattern for temporary allocations during disk cache resolution. </blockquote></details> <details> <summary>src/bundler/linker_context/scanImportsAndExports.zig (1)</summary><blockquote> `1201-1201`: **LGTM!** The allocator replacement is correct for CSS module composition validation. The 1KB stack buffer with graph allocator fallback is appropriate for temporary visitor allocations. </blockquote></details> <details> <summary>src/bundler/HTMLImportManifest.zig (1)</summary><blockquote> `99-99`: **LGTM!** The allocator source change is appropriate. The 4KB stack buffer with default allocator fallback is suitable for temporary JSON string building in HTML import manifest generation. </blockquote></details> <details> <summary>src/install/PackageManager/UpdateRequest.zig (1)</summary><blockquote> `52-52`: **LGTM: Consistent allocator refactor** The switch from `std.heap.stackFallback` to `bun.allocators.stackFallback` aligns with the PR's objective to standardize stack fallback allocators across the codebase. The allocation size (1024 bytes) and fallback allocator remain unchanged, preserving existing behavior. </blockquote></details> <details> <summary>src/bun.js/api/JSTranspiler.zig (1)</summary><blockquote> `1005-1005`: **LGTM: Consistent allocator refactor** The switch from `std.heap.stackFallback` to `bun.allocators.stackFallback` in `namedExportsToJS` maintains the same allocation size and fallback behavior. This change aligns with the PR's standardization of allocator usage across the codebase. </blockquote></details> <details> <summary>src/css/css_modules.zig (1)</summary><blockquote> `398-398`: **LGTM: Consistent allocator refactor** The switch from `std.heap.stackFallback` to `bun.allocators.stackFallback` in the `hash()` function maintains the same 128-byte stack buffer size and fallback behavior. This change is consistent with the broader standardization effort across the codebase. </blockquote></details> <details> <summary>src/install/NetworkTask.zig (1)</summary><blockquote> `93-93`: **LGTM: Consistent allocator refactor** The switch from `std.heap.stackFallback` to `bun.allocators.stackFallback` in `forManifest` maintains the 512-byte stack buffer for URL encoding operations. This change aligns with the PR's standardization of allocator usage without affecting the manifest fetching logic. </blockquote></details> <details> <summary>src/bun.js/bindings/JSValue.zig (2)</summary><blockquote> `712-712`: **LGTM: Consistent allocator refactor** The switch from `std.heap.stackFallback` to `bun.allocators.stackFallback` in `printString` maintains the same parameterized stack buffer size and fallback behavior. This change aligns with the PR's standardization effort. --- `724-724`: **LGTM: Consistent allocator refactor** The switch from `std.heap.stackFallback` to `bun.allocators.stackFallback` in `printStringPretty` is consistent with the same change in `printString` above. Both functions now use the standardized Bun allocator system. </blockquote></details> <details> <summary>src/sql/mysql/js/JSMySQLConnection.zig (1)</summary><blockquote> `672-672`: **LGTM: Allocator migration to bun.allocators** The change from `std.heap.stackFallback` to `bun.allocators.stackFallback` is consistent with the PR's objective to standardize stack fallback allocator usage across the codebase. </blockquote></details> <details> <summary>src/bake/DevServer/HotReloadEvent.zig (1)</summary><blockquote> `190-190`: **LGTM: Consistent allocator abstraction** Migration to `bun.allocators.stackFallback` is consistent with the codebase-wide refactor. </blockquote></details> <details> <summary>src/string.zig (1)</summary><blockquote> `196-196`: **LGTM: Allocator migration in String.createFormat** The migration to `bun.allocators.stackFallback` maintains the same behavior while standardizing on Bun's allocator abstraction. </blockquote></details> <details> <summary>src/ini.zig (2)</summary><blockquote> `54-54`: **LGTM: Rope stack allocator migration** Consistent migration to `bun.allocators.stackFallback` for the rope parsing allocator. --- `265-265`: **LGTM: Value parsing allocator migration** Consistent migration to `bun.allocators.stackFallback` for value parsing. </blockquote></details> <details> <summary>src/crash_handler.zig (1)</summary><blockquote> `1667-1667`: **LGTM: Stack trace symbolizer allocator migration** The migration to `bun.allocators.stackFallback` is consistent with the PR objective. The larger stack size (16384 bytes) is appropriate for stack trace symbolization. </blockquote></details> <details> <summary>src/css/values/color_js.zig (1)</summary><blockquote> `142-144`: **Allocator swap aligns with new API.** Switching to `bun.allocators.stackFallback` keeps the buffer size and fallback allocator identical, so the temporary arena behaves the same while using Bun's allocator helpers. </blockquote></details> <details> <summary>src/bun.js/ConsoleObject.zig (1)</summary><blockquote> `502-505`: **Allocator swap keeps TablePrinter temp storage consistent.** The new call preserves the stack buffer size and fallback allocator, so the `columns` list still allocates from the intended stack-backed arena via Bun's allocator helpers. </blockquote></details> <details> <summary>src/install/PackageInstall.zig (1)</summary><blockquote> `145-147`: **Allocator factory swap keeps git tag reads unchanged.** Using `bun.allocators.stackFallback` here maintains the 2 KiB scratch buffer semantics while aligning with the centralized allocator helpers. </blockquote></details> <details> <summary>src/string/immutable.zig (1)</summary><blockquote> `2052-2052`: **LGTM - Allocator source updated consistently.** The change from `std.heap.stackFallback` to `bun.allocators.stackFallback` is correct and maintains the same stack size (1024 bytes) and usage pattern. </blockquote></details> <details> <summary>src/bake/production.zig (4)</summary><blockquote> `798-798`: **LGTM - Allocator source updated.** The change from `std.heap.stackFallback` to `bun.allocators.stackFallback` maintains the same stack size (1024 bytes) and behavior. --- `810-810`: **LGTM - Allocator source updated.** The change from `std.heap.stackFallback` to `bun.allocators.stackFallback` maintains the same stack size (`@sizeOf(bun.PathBuffer) * 2`) and behavior. --- `1023-1023`: **LGTM - Allocator source updated.** The change from `std.heap.stackFallback` to `bun.allocators.stackFallback` maintains the same stack size (4096 bytes) and behavior. --- `1036-1036`: **LGTM - Allocator source updated.** The change from `std.heap.stackFallback` to `bun.allocators.stackFallback` maintains the same stack size (4096 bytes) and behavior. </blockquote></details> <details> <summary>src/bun.js/api/server/NodeHTTPResponse.zig (1)</summary><blockquote> `468-468`: **LGTM - Allocator source updated.** The change from `std.heap.stackFallback` to `bun.allocators.stackFallback` maintains the same stack size (256 bytes) and behavior. The use of `bun.default_allocator` is consistent with the surrounding code. </blockquote></details> <details> <summary>src/resolver/resolver.zig (1)</summary><blockquote> `3441-3443`: **Allocator swap LGTM** Using bun.allocators.stackFallback with arena.allocator() here is correct. Lifetime is contained to the function; list.deinit() uses the same alloc. No issues. </blockquote></details> <details> <summary>src/ast/Parser.zig (1)</summary><blockquote> `345-351`: **Assertions valid – no adjustments needed** The `StackFallbackAllocator` struct defines a `fixed_buffer_allocator` field (see src/allocators.zig), and `std.heap.FixedBufferAllocator` implements `ownsPtr`, so the assertions in Parser.zig lines 360–362 remain correct. </blockquote></details> <details> <summary>src/css/css_parser.zig (1)</summary><blockquote> `3967-3973`: **Allocator swap looks good.** Thanks for migrating this helper to `bun.allocators.stackFallback`; sizing the scratch buffer to `@sizeOf(T)` keeps the existing semantics and matches the rest of the refactor. 👍 </blockquote></details> <details> <summary>src/bun.js/node/node_os.zig (1)</summary><blockquote> `58-58`: **LGTM! Clean refactor to use Bun's allocator system.** The migration from `std.heap.stackFallback` to `bun.allocators.stackFallback` maintains the same behavior while standardizing on Bun's allocator infrastructure. The 8KB stack buffer is appropriate for reading `/proc/stat`. </blockquote></details> <details> <summary>src/valkey/js_valkey_functions.zig (1)</summary><blockquote> `107-107`: **LGTM! Consistent allocator standardization across all Redis command functions.** All nine instances consistently migrate to `bun.allocators.stackFallback(512, bun.default_allocator)` for temporary `ArrayList<JSArgument>` allocations. The 512-byte stack buffer is appropriate for typical command argument counts, with proper heap fallback for larger cases. Also applies to: 282-282, 325-325, 389-389, 434-434, 509-509, 996-996, 1040-1040, 1142-1142 </blockquote></details> <details> <summary>src/bun.js/api/bun/socket.zig (1)</summary><blockquote> `932-932`: **LGTM! Appropriate stack buffer sizing for socket write operations.** Both instances migrate to `bun.allocators.stackFallback(16 * 1024, bun.default_allocator)`. The 16KB stack buffer is well-suited for socket write operations, optimizing for common payload sizes while maintaining proper heap fallback for larger data. Also applies to: 1075-1075 </blockquote></details> <details> <summary>src/ast/P.zig (1)</summary><blockquote> `3539-3543`: **Allocator migration looks solid** Switching to `bun.allocators.stackFallback` keeps the existing fallback semantics (`get()` + conditional `deinit`) intact, so this block will still short-circuit the heap path unless we overflow the fixed buffer. Looks good to me. </blockquote></details> <details> <summary>src/shell/shell.zig (1)</summary><blockquote> `862-862`: **No std.heap.stackFallback occurrences remain.** </blockquote></details> <details> <summary>src/http.zig (1)</summary><blockquote> `1262-1295`: **Allocator switch looks good** Swapping over to `bun.allocators.stackFallback` keeps the existing semantics while routing the fallback through the shared bun allocator helpers. No functional change spotted here. </blockquote></details> <details> <summary>src/bun.js/api/BunObject.zig (2)</summary><blockquote> `847-856`: **doResolve fallback allocator update** Using `bun.allocators.stackFallback` here mirrors the prior behaviour while taking advantage of the new allocator factory. Looks solid. --- `1128-1140`: **escapeHTML stack allocator migration** The allocator swap in `Bun__escapeHTML8` preserves the short-string stack path and keeps larger inputs on the fallback allocator. Implementation remains correct. </blockquote></details> <details> <summary>src/install/PackageManager/patchPackage.zig (3)</summary><blockquote> `637-645`: **Stack fallback switch for existing patch hash** The move to `bun.allocators.stackFallback` keeps the temporary formatting scoped and continues to fall back to the manager allocator when needed. No issues spotted. --- `674-683`: **Second existing patch hash block** Same pattern change here; behaviour stays the same with the new helper. All good. --- `926-937`: **pkgInfo stack fallback** Initialising the `IdPair` array list with the new stack fallback allocator preserves the previous ownership model. Looks correct. </blockquote></details> <details> <summary>src/bun.js/bindings/JSGlobalObject.zig (7)</summary><blockquote> `262-276`: **createErrorInstance allocator swap** The helper now relies on `bun.allocators.stackFallback` but otherwise maintains the same flow. Formatting and cloning logic stay intact. Looks good. --- `284-295`: **createTypeErrorInstance allocator swap** Same change pattern here—new allocator helper, unchanged semantics. Approval from my side. --- `299-308`: **createDOMExceptionInstance allocator swap** Stack fallback migration keeps the try/alloc path identical. Good to go. --- `313-322`: **createSyntaxErrorInstance allocator switch** No behaviour change besides the helper. Implementation remains correct. --- `327-336`: **createRangeErrorInstance allocator switch** Migration mirrors the preceding helpers; all semantics preserved. --- `447-453`: **throwError formatter allocator** Formatter still guards OOM and prefixes error names; new helper is a drop-in replacement. Looks fine. --- `947-955`: **createError helper allocator** Formatting path still frees through the allocator after creating the error instance. The new helper behaves like the old one. Approved. </blockquote></details> <details> <summary>src/http/HTTPThread.zig (2)</summary><blockquote> `52-72`: **RequestBodyBuffer stack variant** Updating the union to the new `bun.allocators.StackFallbackAllocator` keeps the buffer semantics aligned with the helper change below. No concerns here. --- `116-118`: **getRequestBodySendBuffer allocator swap** Returning a stack buffer via `bun.allocators.stackFallback` continues to size it off the 32 KB constant and fall back to the heap path as before. Looks correct. </blockquote></details> <details> <summary>src/bake/DevServer.zig (1)</summary><blockquote> `1112-1114`: **Allocator swap LGTM.** Using bun.allocators.stackFallback(...).get() is consistent with the new API and fits the prior patterns (temp allocators with clear lifetimes). No issues spotted at these sites. Also applies to: 1245-1247, 1538-1541, 1840-1842, 1935-1936, 2017-2020, 2081-2084, 2555-2558, 2572-2574, 2979-2981, 3613-3615, 3643-3645, 4218-4220 </blockquote></details> <details> <summary>src/bun.js/node/node_fs.zig (2)</summary><blockquote> `4659-4666`: **Allocator swap in recursive sync walk LGTM.** iterator_stack and basename_stack lifetimes are confined to the function; LinearFifo.deinit() and freeing of duplicated basenames ensure no leaks. Behavior matches the prior std.heap.stackFallback usage. --- `1066-1071`: **Approve bun.allocators.stackFallback usage; API parity confirmed** bun.allocators.stackFallback(size, bun.default_allocator).get() returns std.mem.Allocator and requires no deinit, mirroring std.heap.stackFallback behavior. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+936
to
+939
| .force_heap = if (comptime Environment.ci_assert) | ||
| !bun.getRuntimeFeatureFlag(.BUN_DEBUG_FORCE_HEAP_FALLBACK_ALLOCATORS) | ||
| else {}, | ||
| }; |
Contributor
There was a problem hiding this comment.
force_heap flag is inverted (breaks CI “force heap” behavior).
The PR description says the runtime feature flag forces heap fallback in CI. Current code negates it, so enabling BUN_DEBUG_FORCE_HEAP_FALLBACK_ALLOCATORS disables forcing.
Apply this diff:
- .force_heap = if (comptime Environment.ci_assert)
- !bun.getRuntimeFeatureFlag(.BUN_DEBUG_FORCE_HEAP_FALLBACK_ALLOCATORS)
- else {},
+ .force_heap = if (comptime Environment.ci_assert)
+ bun.getRuntimeFeatureFlag(.BUN_DEBUG_FORCE_HEAP_FALLBACK_ALLOCATORS)
+ else {},📝 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.
Suggested change
| .force_heap = if (comptime Environment.ci_assert) | |
| !bun.getRuntimeFeatureFlag(.BUN_DEBUG_FORCE_HEAP_FALLBACK_ALLOCATORS) | |
| else {}, | |
| }; | |
| .force_heap = if (comptime Environment.ci_assert) | |
| bun.getRuntimeFeatureFlag(.BUN_DEBUG_FORCE_HEAP_FALLBACK_ALLOCATORS) | |
| else {}, | |
| }; |
🤖 Prompt for AI Agents
In src/allocators.zig around lines 936 to 939, the .force_heap assignment
inverts the BUN_DEBUG_FORCE_HEAP_FALLBACK_ALLOCATORS flag (using !) so enabling
the flag disables the CI "force heap" behavior; remove the negation so
.force_heap uses
bun.getRuntimeFeatureFlag(.BUN_DEBUG_FORCE_HEAP_FALLBACK_ALLOCATORS) directly
when Environment.ci_assert is true, leaving the else branch unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
testing stack fallback allocators in CI
How did you verify your code works?