Fix shell interpreter double-free on GC finalization

src/shell/interpreter.zig

@@ -280,6 +280,17 @@ pub const Interpreter = struct {
     exit_code: ?ExitCode = 0,
     this_jsvalue: JSValue = .zero,
 
+    /// 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,
+
     __alloc_scope: if (bun.Environment.enableAllocScopes) bun.AllocationScope else void,
     estimated_size_for_gc: usize = 0,
 
@@ -1225,15 +1236,34 @@ pub const Interpreter = struct {
         this.keep_alive.disable();
         this.root_shell.deinitImpl(false, false);
         this.this_jsvalue = .zero;
+        // Mark that we've cleaned up runtime resources, so the finalizer knows
+        // it only needs to clean up buffered IO, args, and the interpreter itself
+        this.cleanup_state = .runtime_cleaned;
     }
 
     fn deinitFromFinalizer(this: *ThisInterpreter) void {
+        log("Interpreter(0x{x}) deinitFromFinalizer (cleanup_state={s})", .{ @intFromPtr(this), @tagName(this.cleanup_state) });
+
+        switch (this.cleanup_state) {
+            .needs_full_cleanup => {
+                // The interpreter never finished normally, so we need to clean up everything
+                this.root_io.deref();
+                this.root_shell.deinitImpl(false, false);
+            },
+            .runtime_cleaned => {
+                // deinitAfterJSRun already cleaned up IO and shell env, nothing more to do here
+            },
+        }
+
+        // Clean up the buffered IO that was not freed by deinitImpl
+        // (deinitImpl is called with free_buffered_io=false)
         if (this.root_shell._buffered_stderr == .owned) {
             this.root_shell._buffered_stderr.owned.deinit(bun.default_allocator);
         }
         if (this.root_shell._buffered_stdout == .owned) {
             this.root_shell._buffered_stdout.owned.deinit(bun.default_allocator);
         }
+
         this.this_jsvalue = .zero;
         this.args.deinit();
         this.allocator.destroy(this);

test/regression/issue-shell-double-free.test.ts

@@ -0,0 +1,76 @@
+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.
+
+test("shell interpreter should handle GC during concurrent execution", async () => {
+  // This test mimics the opencode usage pattern where shell commands
+  // are used to resolve paths during permission checks
+  const promises = [];
+
+  for (let i = 0; i < 50; i++) {
+    // Similar to the opencode pattern: $`realpath ${arg}`.quiet().nothrow().text()
+    promises.push($`echo "/tmp/test${i}"`.quiet().nothrow().text());
+  }
+
+  const results = await Promise.all(promises);
+  expect(results.length).toBe(50);
+
+  // Multiple GC passes to trigger finalizers
+  // The bug would manifest as a segfault during finalization
+  for (let i = 0; i < 5; i++) {
+    Bun.gc(true);
+    await Bun.sleep(1);
+  }
+});
+
+test("shell interpreter sequential with frequent GC", async () => {
+  // Sequential execution with GC after each command
+  // increases pressure on the finalizer
+  for (let i = 0; i < 100; i++) {
+    const result = await $`echo "test ${i}"`.quiet().nothrow().text();
+    expect(result.trim()).toBe(`test ${i}`);
+
+    // Force GC frequently to trigger finalizers while
+    // other interpreters might still be finishing
+    if (i % 5 === 0) {
+      Bun.gc(true);
+    }
+  }
+
+  // Final GC pass
+  Bun.gc(true);
+});
+
+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);
+  }
+});