Fix shell interpreter double-free on GC finalization #25916
Conversation
This fixes a crash that occurred when the GC finalizer tried to free memory
that was already partially freed during normal shell interpreter cleanup.
The issue happened in this sequence:
1. When a shell interpreter finished execution, `deinitAfterJSRun()` was called
which partially deinitialized the interpreter (freed env maps, IO, etc.)
2. Later, when the GC ran and called the finalizer, `deinitFromFinalizer()`
tried to free resources that may have already been freed, or were in an
inconsistent state
The fix adds a `cleanup_state` enum to track which resources have been cleaned
up. When `deinitAfterJSRun()` completes, it sets the state to `runtime_cleaned`.
The finalizer then checks this state and only performs the necessary cleanup:
- If `needs_full_cleanup`: perform full cleanup (interpreter never ran/failed early)
- If `runtime_cleaned`: only cleanup buffered IO, args, and the interpreter itself
This approach makes the ownership and cleanup responsibilities explicit, preventing
the double-free crash that was reported on Windows when using Bun's shell API
(`import { $ } from "bun"`).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
WalkthroughIntroduces cleanup state tracking to the Interpreter struct to prevent double-freeing during garbage collection finalization. Deinitialization is now conditional based on cleanup state. Adds regression tests for concurrent and sequential GC scenarios. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/shell/interpreter.zig:
- Around line 283-292: Rename the internal field cleanup_state on the
interpreter struct to use the private-field prefix (#cleanup_state) and update
all references to it in deinitAfterJSRun and deinitFromFinalizer (and any other
methods that touch cleanup_state) to use this.#cleanup_state; ensure the enum
declaration remains the same but is bound to the new #cleanup_state identifier
and adjust any pattern matches or assignments to reference the renamed field.
In @test/regression/issue-shell-double-free.test.ts:
- Around line 1-16: Rename the test file issue-shell-double-free.test.ts to
issue-23177.test.ts and move it into the regression issue tests directory
(create that directory if it doesn't exist), then update any references or
imports that point to the old filename so CI and test discovery still find the
test.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/shell/interpreter.zigtest/regression/issue-shell-double-free.test.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts?(x): Never usebun testdirectly - always usebun bd testto run tests with debug build changes
For single-file tests, prefer-eflag overtempDir
For multi-file tests, prefertempDirandBun.spawnover single-file tests
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
UsetempDirfromharnessto create temporary directories - do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests; instead await the condition to be met
Verify tests fail withUSE_SYSTEM_BUN=1 bun test <file>and pass withbun bd test <file>- tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with.test.tsor.test.tsx
Avoid shell commands likefindorgrepin tests - use Bun's Glob and built-in tools instead
Files:
test/regression/issue-shell-double-free.test.ts
test/**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
port: 0in tests - do not hardcode ports or use custom random port number functions
Files:
test/regression/issue-shell-double-free.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 bd test <...test file>to run tests with compiled code changes. Do not usebun testas it will not include your changes.
Usebun:testfor files ending in*.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, usebun bd <file>instead ofbun bd test <file>since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.
Files:
test/regression/issue-shell-double-free.test.ts
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, be careful with allocators and use defer for cleanup
Files:
src/shell/interpreter.zig
src/**/*.zig
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.zig: Use the#prefix for private fields in Zig structs, e.g.,struct { #foo: u32 };
Use Decl literals in Zig, e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };
Place@importstatements at the bottom of the file in Zig (auto formatter will handle positioning)
Never use@import()inline inside functions in Zig; always place imports at the bottom of the file or containing struct
Files:
src/shell/interpreter.zig
🧠 Learnings (22)
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to test/regression/issue/*.test.ts : Place regression tests for specific GitHub issues in `test/regression/issue/${issueNumber}.test.ts` with real issue numbers only
Applied to files:
test/regression/issue-shell-double-free.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/regression/issue-shell-double-free.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Organize regression tests for specific issues in `/test/regression/issue/${issueNumber}.test.ts`. Do not place regression tests in the regression directory if there is no associated issue number.
Applied to files:
test/regression/issue-shell-double-free.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Avoid shell commands like `find` or `grep` in tests - use Bun's Glob and built-in tools instead
Applied to files:
test/regression/issue-shell-double-free.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Applied to files:
test/regression/issue-shell-double-free.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : For multi-file tests, prefer `tempDir` and `Bun.spawn` over single-file tests
Applied to files:
test/regression/issue-shell-double-free.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
Applied to files:
test/regression/issue-shell-double-free.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*-fixture.ts : Test files that spawn Bun processes should end in `*-fixture.ts` to identify them as test fixtures rather than tests themselves.
Applied to files:
test/regression/issue-shell-double-free.test.ts
📚 Learning: 2025-10-26T01:32:04.844Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.
Applied to files:
test/regression/issue-shell-double-free.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Never use `bun test` directly - always use `bun bd test` to run tests with debug build changes
Applied to files:
test/regression/issue-shell-double-free.test.ts
📚 Learning: 2025-11-06T00:58:23.965Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.
Applied to files:
test/regression/issue-shell-double-free.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending in `*.test.{ts,js,jsx,tsx,mjs,cjs}`. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use `bun bd <file>` instead of `bun bd test <file>` since they expect exit code 0.
Applied to files:
test/regression/issue-shell-double-free.test.ts
📚 Learning: 2025-11-08T04:06:33.198Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24491
File: test/js/bun/transpiler/declare-global.test.ts:17-17
Timestamp: 2025-11-08T04:06:33.198Z
Learning: In Bun test files, `await using` with Bun.spawn() is the preferred pattern for spawned processes regardless of whether they are short-lived or long-running. Do not suggest replacing `await using proc = Bun.spawn(...)` with `const proc = Bun.spawn(...); await proc.exited;`.
Applied to files:
test/regression/issue-shell-double-free.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Verify tests fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test <file>` - tests are invalid if they pass with USE_SYSTEM_BUN=1
Applied to files:
test/regression/issue-shell-double-free.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun bd test <...test file>` to run tests with compiled code changes. Do not use `bun test` as it will not include your changes.
Applied to files:
test/regression/issue-shell-double-free.test.ts
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Write JS builtins for Bun's Node.js compatibility and APIs, and run `bun bd` after changes
Applied to files:
test/regression/issue-shell-double-free.test.ts
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/regression/issue-shell-double-free.test.ts
📚 Learning: 2025-10-14T04:04:17.132Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 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/interpreter.zig
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.zig : In Zig code, be careful with allocators and use defer for cleanup
Applied to files:
src/shell/interpreter.zig
📚 Learning: 2025-11-03T20:43:06.996Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/test/snapshot.zig:19-19
Timestamp: 2025-11-03T20:43:06.996Z
Learning: In Bun's Zig codebase, when storing JSValue objects in collections like ArrayList, use `jsc.Strong.Optional` (not raw JSValue). When adding values, wrap them with `jsc.Strong.Optional.create(value, globalThis)`. In cleanup code, iterate the collection calling `.deinit()` on each Strong.Optional item before calling `.deinit()` on the ArrayList itself. This pattern automatically handles GC protection. See examples in src/bun.js/test/ScopeFunctions.zig and src/bun.js/node/node_cluster_binding.zig.
Applied to files:
src/shell/interpreter.zig
📚 Learning: 2025-10-14T04:04:47.894Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 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/interpreter.zig
📚 Learning: 2026-01-05T16:32:07.551Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:07.551Z
Learning: In Zig codebases (e.g., Bun), treat std.posix.sigaction as returning void and do not perform runtime error handling for its failure. The Zig standard library views sigaction failures as programmer errors (unreachable) because they only occur with invalid signals like SIGKILL/SIGSTOP. Apply this pattern across Zig files that call sigaction (e.g., crash_handler.zig, main.zig, filter_run.zig, process.zig) and ensure failures are not handled as recoverable errors; prefer reaching an explicit unreachable/compile-time assumption when such failures are detected.
Applied to files:
src/shell/interpreter.zig
🧬 Code graph analysis (1)
test/regression/issue-shell-double-free.test.ts (1)
scripts/utils.mjs (1)
$(143-161)
🔇 Additional comments (4)
src/shell/interpreter.zig (2)
1237-1246: LGTM!The state transition is correctly placed after the cleanup operations complete, ensuring the finalizer accurately knows which resources have already been freed.
1248-1274: LGTM!The conditional cleanup based on
cleanup_statecorrectly prevents double-free:
- When
needs_full_cleanup: the interpreter was never properly finalized, so clean everything- When
runtime_cleaned: skip IO and shell env cleanup sincedeinitAfterJSRunalready freed themThe buffered IO cleanup (lines 1264-1269) correctly runs unconditionally since
deinitImplis called withfree_buffered_io=falsein both paths.test/regression/issue-shell-double-free.test.ts (2)
17-36: LGTM!The test effectively exercises the concurrent execution pattern that triggered the original bug, with multiple GC passes to stress the finalizer paths.
38-54: LGTM!The sequential test with periodic GC effectively stresses the cleanup path while verifying correct output.
| /// Tracks which resources have been cleaned up to avoid double-free. | ||
| /// When the interpreter finishes normally via deinitAfterJSRun, it cleans up | ||
| /// the runtime resources (IO, shell env) and sets this to mark them as freed. | ||
| /// The finalizer then only cleans up what remains (buffered IO, args, interpreter itself). | ||
| cleanup_state: enum { | ||
| /// Nothing has been cleaned up yet | ||
| needs_full_cleanup, | ||
| /// Runtime resources (IO, shell env) have been cleaned up, only buffered IO + args remain | ||
| runtime_cleaned, | ||
| } = .needs_full_cleanup, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using # prefix for private field.
The cleanup_state field is an internal implementation detail used only by cleanup methods (deinitAfterJSRun, deinitFromFinalizer). Per coding guidelines for src/**/*.zig, private fields should use the # prefix.
♻️ Suggested change
- cleanup_state: enum {
+ #cleanup_state: enum {
/// Nothing has been cleaned up yet
needs_full_cleanup,
/// Runtime resources (IO, shell env) have been cleaned up, only buffered IO + args remain
runtime_cleaned,
} = .needs_full_cleanup,Note: This would also require updating references in deinitAfterJSRun and deinitFromFinalizer to use this.#cleanup_state.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/shell/interpreter.zig around lines 283 - 292, Rename the internal field
cleanup_state on the interpreter struct to use the private-field prefix
(#cleanup_state) and update all references to it in deinitAfterJSRun and
deinitFromFinalizer (and any other methods that touch cleanup_state) to use
this.#cleanup_state; ensure the enum declaration remains the same but is bound
to the new #cleanup_state identifier and adjust any pattern matches or
assignments to reference the renamed field.
| import { $ } from "bun"; | ||
| import { expect, test } from "bun:test"; | ||
|
|
||
| // Regression test for shell interpreter double-free issue | ||
| // The bug occurred when the GC finalizer tried to free memory that was | ||
| // already partially freed when the shell finished execution. | ||
| // This was particularly reproducible on Windows. | ||
| // | ||
| // The issue was that deinitAfterJSRun() would partially deinitialize | ||
| // the interpreter, and then deinitFromFinalizer() would try to free | ||
| // resources that were already freed or in an inconsistent state. | ||
| // | ||
| // NOTE: This bug is non-deterministic and may not always crash, even | ||
| // without the fix. The test serves to verify the fix doesn't break | ||
| // normal operation and documents the usage pattern that triggered the bug. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
File should be renamed to follow regression test naming convention.
Per coding guidelines, regression tests for specific GitHub issues should be placed in test/regression/issue/${issueNumber}.test.ts. Since this test is for issue #23177, it should be renamed.
#!/bin/bash
# Verify the issue directory exists and check naming conventions
if [ -d "test/regression/issue" ]; then
echo "test/regression/issue directory exists"
ls -la test/regression/issue/ 2>/dev/null | head -20
else
echo "test/regression/issue directory does not exist"
ls -la test/regression/ 2>/dev/null | head -20
fi🤖 Prompt for AI Agents
In @test/regression/issue-shell-double-free.test.ts around lines 1 - 16, Rename
the test file issue-shell-double-free.test.ts to issue-23177.test.ts and move it
into the regression issue tests directory (create that directory if it doesn't
exist), then update any references or imports that point to the old filename so
CI and test discovery still find the test.
| test("shell interpreter error handling with GC", async () => { | ||
| // Test that error paths also properly clean up | ||
| const promises = []; | ||
|
|
||
| for (let i = 0; i < 20; i++) { | ||
| // Mix of successful and potentially failing commands | ||
| if (i % 3 === 0) { | ||
| promises.push($`echo "success ${i}"`.quiet().nothrow().text()); | ||
| } else { | ||
| promises.push($`echo "test ${i}"`.quiet().text()); | ||
| } | ||
| } | ||
|
|
||
| await Promise.all(promises); | ||
|
|
||
| // GC should not crash even with mixed success/error states | ||
| for (let i = 0; i < 3; i++) { | ||
| Bun.gc(true); | ||
| await Bun.sleep(1); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test doesn't actually exercise error paths as intended.
Both command variants (echo "success ${i}" and echo "test ${i}") will succeed. The .nothrow() modifier only affects how non-zero exit codes are handled, but echo always exits with 0. To test error handling with GC, consider using commands that actually fail:
♻️ Suggested improvement
test("shell interpreter error handling with GC", async () => {
// Test that error paths also properly clean up
const promises = [];
for (let i = 0; i < 20; i++) {
// Mix of successful and potentially failing commands
if (i % 3 === 0) {
promises.push($`echo "success ${i}"`.quiet().nothrow().text());
} else {
- promises.push($`echo "test ${i}"`.quiet().text());
+ // Use a failing command with nothrow to test error cleanup
+ promises.push($`exit 1`.quiet().nothrow().text());
}
}
await Promise.all(promises);
// GC should not crash even with mixed success/error states
for (let i = 0; i < 3; i++) {
Bun.gc(true);
await Bun.sleep(1);
}
});📝 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.
| test("shell interpreter error handling with GC", async () => { | |
| // Test that error paths also properly clean up | |
| const promises = []; | |
| for (let i = 0; i < 20; i++) { | |
| // Mix of successful and potentially failing commands | |
| if (i % 3 === 0) { | |
| promises.push($`echo "success ${i}"`.quiet().nothrow().text()); | |
| } else { | |
| promises.push($`echo "test ${i}"`.quiet().text()); | |
| } | |
| } | |
| await Promise.all(promises); | |
| // GC should not crash even with mixed success/error states | |
| for (let i = 0; i < 3; i++) { | |
| Bun.gc(true); | |
| await Bun.sleep(1); | |
| } | |
| }); | |
| test("shell interpreter error handling with GC", async () => { | |
| // Test that error paths also properly clean up | |
| const promises = []; | |
| for (let i = 0; i < 20; i++) { | |
| // Mix of successful and potentially failing commands | |
| if (i % 3 === 0) { | |
| promises.push($`echo "success ${i}"`.quiet().nothrow().text()); | |
| } else { | |
| // Use a failing command with nothrow to test error cleanup | |
| promises.push($`exit 1`.quiet().nothrow().text()); | |
| } | |
| } | |
| await Promise.all(promises); | |
| // GC should not crash even with mixed success/error states | |
| for (let i = 0; i < 3; i++) { | |
| Bun.gc(true); | |
| await Bun.sleep(1); | |
| } | |
| }); |
|
I think the tests are probably not testing anything, but I think the bugfix is very likely correct - so thank you. I'm going to let CI run and if it looks consistent with the rest of CI, I'll delete the test you added and then merge it. |
2 participants
Summary
This PR fixes a crash that occurs when the GC finalizer tries to free memory that was already partially freed during normal shell interpreter cleanup.
Fixes: #23177
Problem
The issue happens in this sequence:
deinitAfterJSRun()is called which partially deinitializes the interpreter (frees env maps, IO, etc.)deinitFromFinalizer()tries to free resources that may have already been freed, or were in an inconsistent stateThis causes segfaults reported on Windows when using Bun's shell API (
import { $ } from "bun").Solution
Adds a
cleanup_stateenum to track which resources have been cleaned up:needs_full_cleanup: Nothing has been cleaned up yetruntime_cleaned: Runtime resources (IO, shell env) have been cleaned up, only buffered IO + args remainWhen
deinitAfterJSRun()completes, it sets the state toruntime_cleaned. The finalizer then checks this state and only performs the necessary cleanup.Testing
This fix addresses the crash reported in issue #23177 and duplicates (#23266, #24022, #24057, #24182, #24368, #24435, #24903).