Skip to content

fix(sql): don't crash in debug builds when bun:sql module load throws #34831

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 2 commits into from
Closed

fix(sql): don't crash in debug builds when bun:sql module load throws #34831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3c7be57
fix(sql): don't crash in debug builds when bun:sql module fails to load
robobun Jul 20, 2026
b9f302c
[autofix.ci] apply automated fixes
autofix-ci[bot] Jul 20, 2026
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
6 changes: 0 additions & 6 deletions src/jsc/bindings/BunObject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,9 +319,6 @@ static JSValue defaultBunSQLObject(VM& vm, JSObject* bunObject)
auto scope = DECLARE_THROW_SCOPE(vm);
auto* globalObject = defaultGlobalObject(bunObject->globalObject());
JSValue sqlValue = globalObject->internalModuleRegistry()->requireId(globalObject, vm, InternalModuleRegistry::BunSql);
#if BUN_DEBUG
if (scope.exception()) globalObject->reportUncaughtExceptionAtEventLoop(globalObject, scope.exception());
#endif
RETURN_IF_EXCEPTION(scope, {});
RELEASE_AND_RETURN(scope, sqlValue.getObject()->get(globalObject, vm.propertyNames->defaultKeyword));
}
Expand All @@ -331,9 +328,6 @@ static JSValue constructBunSQLObject(VM& vm, JSObject* bunObject)
auto scope = DECLARE_THROW_SCOPE(vm);
auto* globalObject = defaultGlobalObject(bunObject->globalObject());
JSValue sqlValue = globalObject->internalModuleRegistry()->requireId(globalObject, vm, InternalModuleRegistry::BunSql);
#if BUN_DEBUG
if (scope.exception()) globalObject->reportUncaughtExceptionAtEventLoop(globalObject, scope.exception());
#endif
RETURN_IF_EXCEPTION(scope, {});
auto clientData = WebCore::clientData(vm);
RELEASE_AND_RETURN(scope, sqlValue.getObject()->get(globalObject, clientData->builtinNames().SQLPublicName()));
Expand Down
31 changes: 31 additions & 0 deletions test/js/sql/sql-lazy-load-throw.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { expect, test } from "bun:test";
import { bunEnv, bunExe } from "harness";

test("Bun.SQL lazy getter propagates module load errors without crashing", async () => {
// The bun:sql internal module reads global `Symbol` at top level. If user
// code has clobbered the global before the first access, evaluating the
// module throws. That exception must propagate back to the caller as a
// normal JS error rather than crashing the process.
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`
globalThis.Symbol = "i";
let caught = 0;
try { Bun.sql; } catch { caught++; }
try { Bun.SQL; } catch { caught++; }
console.log("caught=" + caught);
`,
],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stdout.trim()).toBe("caught=2");
expect(stderr).toBe("");

@claude claude Bot Jul 20, 2026

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.

🟡 Per REVIEW.md, avoid asserting stderr is exactly empty — ASAN/debug builds can emit benign diagnostics that make this flake in CI. The stdout === "caught=2" and exitCode === 0 assertions already prove the process didn't crash and both exceptions were caught; consider dropping this line or asserting a combined { stdout, exitCode } snapshot instead.

Extended reasoning...

What the issue is

Line 29 asserts expect(stderr).toBe(""). REVIEW.md's Tests reviewers reject section explicitly calls this out:

Never assert stderr is exactly empty (ASAN/debug builds emit benign warnings); assert a combined { stdout, stderr, exitCode } object.

Why it can flake

The test spawns bunExe() with bunEnv. bunEnv sets BUN_DEBUG_QUIET_LOGS=1, which suppresses Bun's own scoped debug loggers, but it does not suppress ASAN/UBSAN runtime diagnostics or other libc/loader warnings that can appear on stderr in the debug+ASAN CI lanes. When such a benign line appears, stderr becomes non-empty and this assertion fails even though the behavior under test — the catchable TypeError propagating out of the Bun.sql/Bun.SQL lazy getters — is working correctly.

Step-by-step

  1. CI runs the debug+ASAN lane.
  2. The spawned child prints caught=2\n to stdout and exits 0 — the fix is working.
  3. ASAN (or another runtime component) writes a benign one-line warning to stderr.
  4. expect(stdout.trim()).toBe("caught=2") passes.
  5. expect(stderr).toBe("") fails → spurious red CI.

Why the existing assertions already suffice

The purpose of this test is to prove the process doesn't crash and that the module-load exception propagates as a catchable JS error. Both facts are fully established by:

  • stdout.trim() === "caught=2" → both try { Bun.sql } and try { Bun.SQL } threw and were caught.
  • exitCode === 0 → no crash, no abort, no uncaught exception.

The stderr === "" assertion adds no additional signal — it only introduces flake risk.

Suggested fix

Either drop line 29 entirely, or use the repo-conventional combined-object snapshot:

expect({ stdout: stdout.trim(), exitCode }).toEqual({ stdout: "caught=2", exitCode: 0 });

(If you want stderr visible in failure output for debugging, include it in the object but don't assert it's exactly empty.)

Severity

Marking as nit: this is a test-hygiene / potential-flake concern, not a correctness bug in the fix itself. The pattern also appears widely elsewhere in the test suite, so it's not uniquely blocking here — just worth aligning with the stated guideline while the file is new.

expect(exitCode).toBe(0);
});
Loading