Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/bun.js/AsyncModule.zig
Original file line number Diff line number Diff line change
Expand Up @@ -662,12 +662,15 @@ pub const AsyncModule = struct {
jsc_vm.transpiler.linker.log = log;
jsc_vm.transpiler.log = log;
jsc_vm.transpiler.resolver.log = log;
jsc_vm.packageManager().log = log;
// Note: Do not swap the package manager's log here.
// The temporary log uses an allocator that may become invalid during
// transpilation. If the PM's log is swapped and auto-install runs,
// runTasks attempts to log HTTP errors which crashes. PM errors are
// properly propagated via callbacks instead.
defer {
jsc_vm.transpiler.linker.log = old_log;
jsc_vm.transpiler.log = old_log;
jsc_vm.transpiler.resolver.log = old_log;
jsc_vm.packageManager().log = old_log;
}

// We _must_ link because:
Expand Down
13 changes: 7 additions & 6 deletions src/bun.js/ModuleLoader.zig
Original file line number Diff line number Diff line change
Expand Up @@ -209,17 +209,18 @@ pub fn transpileSourceCode(
jsc_vm.transpiler.log = log;
jsc_vm.transpiler.linker.log = log;
jsc_vm.transpiler.resolver.log = log;
if (jsc_vm.transpiler.resolver.package_manager) |pm| {
pm.log = log;
}
// Note: Do not swap the package manager's log here.
// The temporary log created above uses an allocator that may become
// invalid during transpilation. If the PM's log is swapped and
// auto-install runs (e.g., no node_modules), runTasks attempts to
// log HTTP errors via manager.log.addErrorFmt() which crashes.
// PM errors are properly propagated via callbacks (onPackageManifestError,
// onDependencyError) rather than through the log.

defer {
jsc_vm.transpiler.log = old;
jsc_vm.transpiler.linker.log = old;
jsc_vm.transpiler.resolver.log = old;
if (jsc_vm.transpiler.resolver.package_manager) |pm| {
pm.log = old;
}
}

// this should be a cheap lookup because 24 bytes == 8 * 3 so it's read 3 machine words
Expand Down
94 changes: 94 additions & 0 deletions test/cli/run/run-autoinstall.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,3 +84,97 @@ test("--install=fallback to install missing packages", async () => {
expect(stderr?.toString("utf8")).not.toContain("error: Cannot find package 'is-odd'");
expect(stdout?.toString("utf8")).toBe("true false\n");
});

// Regression test: Auto-install should not crash with a segfault when logging errors.
// The crash occurred because the PackageManager's log used an allocator
// that could become invalid during transpilation.
describe("autoinstall should not crash on resolution errors", () => {
test("should handle missing package gracefully without segfault", async () => {
const dir = tmpdirSync();
mkdirSync(dir, { recursive: true });

// Create a project that imports a non-existent package
await Promise.all([
Bun.write(
join(dir, "index.js"),
"import nonExistentPackage from 'this-package-does-not-exist-12345'; console.log(nonExistentPackage);",
),
Bun.write(
join(dir, "package.json"),
JSON.stringify({
name: "test-autoinstall-crash",
type: "module",
}),
),
]);

// Run without node_modules to trigger auto-install code path
const { exitCode, stderr } = Bun.spawnSync({
cmd: [bunExe(), join(dir, "index.js")],
cwd: dir,
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});

const stderrText = stderr?.toString("utf8") ?? "";

// Should NOT crash with segfault (exit codes 128+N indicate death by signal N)
expect(exitCode).not.toBe(134); // SIGABRT (128 + 6)
expect(exitCode).not.toBe(139); // SIGSEGV (128 + 11)

// Should contain a normal error message about the missing package
expect(stderrText).toContain("this-package-does-not-exist-12345");
Comment on lines +120 to +131

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider reordering assertions: check stderr before exit code for better diagnostics.

Per coding guidelines, when spawning processes in tests, expect output (stdout/stderr) before expecting exit code for more useful error messages on test failure. If these assertions fail, seeing the stderr content first provides more context.

♻️ Suggested reordering
     const stderrText = stderr?.toString("utf8") ?? "";
 
+    // Should contain a normal error message about the missing package
+    expect(stderrText).toContain("this-package-does-not-exist-12345");
+
+    // Should NOT contain crash indicators
+    expect(stderrText).not.toContain("Segmentation fault");
+    expect(stderrText).not.toContain("Bun has crashed");
+
     // Should NOT crash with segfault (exit codes 128+N indicate death by signal N)
     expect(exitCode).not.toBe(134); // SIGABRT (128 + 6)
     expect(exitCode).not.toBe(139); // SIGSEGV (128 + 11)
-
-    // Should NOT contain crash indicators
-    expect(stderrText).not.toContain("Segmentation fault");
-    expect(stderrText).not.toContain("Bun has crashed");
-
-    // Should contain a normal error message about the missing package
-    expect(stderrText).toContain("this-package-does-not-exist-12345");
📝 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
const stderrText = stderr?.toString("utf8") ?? "";
// Should NOT crash with segfault (exit codes 128+N indicate death by signal N)
expect(exitCode).not.toBe(134); // SIGABRT (128 + 6)
expect(exitCode).not.toBe(139); // SIGSEGV (128 + 11)
// Should NOT contain crash indicators
expect(stderrText).not.toContain("Segmentation fault");
expect(stderrText).not.toContain("Bun has crashed");
// Should contain a normal error message about the missing package
expect(stderrText).toContain("this-package-does-not-exist-12345");
const stderrText = stderr?.toString("utf8") ?? "";
// Should contain a normal error message about the missing package
expect(stderrText).toContain("this-package-does-not-exist-12345");
// Should NOT contain crash indicators
expect(stderrText).not.toContain("Segmentation fault");
expect(stderrText).not.toContain("Bun has crashed");
// Should NOT crash with segfault (exit codes 128+N indicate death by signal N)
expect(exitCode).not.toBe(134); // SIGABRT (128 + 6)
expect(exitCode).not.toBe(139); // SIGSEGV (128 + 11)
🤖 Prompt for AI Agents
In @test/cli/run/run-autoinstall.test.ts around lines 120 - 131, Reorder the
assertions so the stderr content checks run before the exit code checks: move
the lines that build stderrText and the expect(...).not.toContain /
expect(...).toContain assertions (the checks using stderrText for "Segmentation
fault", "Bun has crashed", and "this-package-does-not-exist-12345") to appear
prior to the expect(exitCode).not.toBe(...) assertions (the SIGABRT/SIGSEGV
checks). Keep the same variables (stderrText, exitCode) and messages unchanged
so failures show stderr diagnostics first.

});
Comment on lines +92 to +128

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

LGTM! Consider reordering assertions for better debugging.

The test correctly validates the segfault regression. Per coding guidelines, checking stderr content before exit code provides more useful error messages on test failure. Consider moving stderr assertions before exit code assertions.

♻️ Suggested reordering
     const stderrText = stderr?.toString("utf8") ?? "";

-    // Should NOT crash with segfault (exit codes 128+N indicate death by signal N)
-    expect(exitCode).not.toBe(134); // SIGABRT (128 + 6)
-    expect(exitCode).not.toBe(139); // SIGSEGV (128 + 11)
-
     // Should NOT contain crash indicators
     expect(stderrText).not.toContain("Segmentation fault");
     expect(stderrText).not.toContain("Bun has crashed");

     // Should contain a normal error message about the missing package
     expect(stderrText).toContain("this-package-does-not-exist-12345");
+
+    // Should NOT crash with segfault (exit codes 128+N indicate death by signal N)
+    expect(exitCode).not.toBe(134); // SIGABRT (128 + 6)
+    expect(exitCode).not.toBe(139); // SIGSEGV (128 + 11)
🤖 Prompt for AI Agents
In @test/cli/run/run-autoinstall.test.ts around lines 92 - 132, Move the
assertions that inspect stderrText before the assertions that check exitCode so
test failures show the captured error output first; specifically, in the "should
handle missing package gracefully without segfault" test reorder the
expectations so the checks using stderrText
(expect(stderrText).not.toContain(...), expect(stderrText).toContain(...)) run
before the exitCode checks (expect(exitCode).not.toBe(134),
expect(exitCode).not.toBe(139)), keeping the same assertions and variable names
(stderrText, exitCode) and preserving the final semantic that the test ensures
no segfault and that the missing-package message is present.

Comment on lines +112 to +128

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider checking stderr before exit code for better failure diagnostics.

Based on coding guidelines, expecting output before exit code provides more useful error messages on test failure. If the process crashes, seeing what stderr contained helps diagnose the issue.

💡 Suggested reorder
     const { exitCode, stderr } = Bun.spawnSync({
       cmd: [bunExe(), join(dir, "index.js")],
       cwd: dir,
       env: bunEnv,
       stdout: "pipe",
       stderr: "pipe",
     });

     const stderrText = stderr?.toString("utf8") ?? "";

-    // Should NOT crash with segfault (exit codes 128+N indicate death by signal N)
-    expect(exitCode).not.toBe(134); // SIGABRT (128 + 6)
-    expect(exitCode).not.toBe(139); // SIGSEGV (128 + 11)
-
     // Should contain a normal error message about the missing package
     expect(stderrText).toContain("this-package-does-not-exist-12345");
+
+    // Should NOT crash with segfault (exit codes 128+N indicate death by signal N)
+    expect(exitCode).not.toBe(134); // SIGABRT (128 + 6)
+    expect(exitCode).not.toBe(139); // SIGSEGV (128 + 11)
🤖 Prompt for AI Agents
In @test/cli/run/run-autoinstall.test.ts around lines 112 - 128, Reorder the
assertions so the test asserts on stderrText before checking exitCode to surface
useful failure output first: move the
expect(stderrText).toContain("this-package-does-not-exist-12345") to precede the
exitCode assertions that check for SIGABRT/SIGSEGV (the variables exitCode and
stderrText and the Bun.spawnSync call are the anchors to change); keep the
stderrText computation and the exitCode assertions intact but placed after the
stderr assertion so test failures show stderr output first for diagnostics.


test("should handle resolution during transpilation without segfault", async () => {
const dir = tmpdirSync();
mkdirSync(dir, { recursive: true });

// Create a TypeScript project that triggers transpilation + resolution
await Promise.all([
Bun.write(
join(dir, "index.ts"),
`
// TypeScript file to trigger transpilation
const x: string = "hello";
import pkg from 'another-nonexistent-pkg-67890';
console.log(x, pkg);
`,
),
Bun.write(
join(dir, "package.json"),
JSON.stringify({
name: "test-autoinstall-ts-crash",
type: "module",
}),
),
Bun.write(
join(dir, "tsconfig.json"),
JSON.stringify({
compilerOptions: {
target: "ESNext",
module: "ESNext",
},
}),
),
]);

const { exitCode, stderr } = Bun.spawnSync({
cmd: [bunExe(), join(dir, "index.ts")],
cwd: dir,
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});

const stderrText = stderr?.toString("utf8") ?? "";

// Should NOT crash with segfault (exit codes 128+N indicate death by signal N)
expect(exitCode).not.toBe(134); // SIGABRT (128 + 6)
expect(exitCode).not.toBe(139); // SIGSEGV (128 + 11)

// Should contain a normal error message
expect(stderrText).toContain("another-nonexistent-pkg-67890");
Comment on lines +175 to +185

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Same suggestion: reorder assertions for better diagnostics on failure.

Consistent with the first test, checking stderr content before exit codes provides more useful context if the test fails.

♻️ Suggested reordering
     const stderrText = stderr?.toString("utf8") ?? "";
 
+    // Should contain a normal error message
+    expect(stderrText).toContain("another-nonexistent-pkg-67890");
+
+    expect(stderrText).not.toContain("Segmentation fault");
+    expect(stderrText).not.toContain("Bun has crashed");
+
     // Should NOT crash with segfault (exit codes 128+N indicate death by signal N)
     expect(exitCode).not.toBe(134); // SIGABRT (128 + 6)
     expect(exitCode).not.toBe(139); // SIGSEGV (128 + 11)
-
-    expect(stderrText).not.toContain("Segmentation fault");
-    expect(stderrText).not.toContain("Bun has crashed");
-
-    // Should contain a normal error message
-    expect(stderrText).toContain("another-nonexistent-pkg-67890");
🤖 Prompt for AI Agents
In @test/cli/run/run-autoinstall.test.ts around lines 175 - 185, Reorder the
assertions so the stderr content checks run before the exit-code checks: first
assert stderrText contains "another-nonexistent-pkg-67890" and does not contain
"Segmentation fault" or "Bun has crashed" (using the stderrText variable), then
assert exitCode is not 134 or 139 (using exitCode); update the block around
stderrText and exitCode in run-autoinstall.test.ts to reflect this new order for
better failure diagnostics.

});
});