bun object: don't report a propagating sql module failure as uncaught - #37198
bun object: don't report a propagating sql module failure as uncaught#37198robobun wants to merge 1 commit into
Conversation
The Bun.sql and Bun.SQL lazy getters had a debug-only block that passed a
failed BunSql module evaluation to reportUncaughtExceptionAtEventLoop with
the exception still pending on the VM. The handler then entered
process->get("_fatalException") (or the process.emit path when that
property was not yet reified) with the exception set, which trips the
exception-scope assertions in debug builds: JSObjectInlines.h(137) when
the property exists, or a Structure check in the emit path otherwise.
Found by fuzzing (a fuzzed script clobbered globalThis.Object, making the
module evaluation throw).
The report was also misleading: the exception is not uncaught, the next
line propagates it and the property access throws it to the caller.
Remove the blocks so the failure propagates cleanly.
WalkthroughThe SQL constructors no longer perform debug-only exception reporting. A subprocess regression test verifies failed ChangesSQL lazy getter failure handling
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/bun-object/lazy-getter-module-failure.test.ts`:
- Around line 26-30: Update the inline Bun.SQL failure test to capture the
thrown error type in the catch block and include it in the logged output, then
update the expected output accordingly. Ensure the test explicitly fails or
reports when Bun.SQL returns normally rather than throwing.
- Around line 34-38: Update the process result handling around proc.stdout and
proc.exited to also read proc.stderr.text() concurrently. Capture the stderr
result and assert that it is empty alongside the existing stdout and exitCode
assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: decd5ee4-0688-4a67-ab44-bc60bbf33b7d
📒 Files selected for processing (2)
src/jsc/bindings/BunObject.cpptest/js/bun/bun-object/lazy-getter-module-failure.test.ts
💤 Files with no reviewable changes (1)
- src/jsc/bindings/BunObject.cpp
| try { | ||
| Bun.SQL; | ||
| } catch (e) {} | ||
| console.log("caught " + caught); | ||
| `, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
target=$(git ls-files | rg '(^|/)lazy-getter-module-failure\.test\.ts$' | head -n 1)
printf '%s\n' "TARGET=$target"
wc -l "$target"
cat -n "$target"
printf '\n--- related references ---\n'
rg -n -C 4 'Bun\.SQL|lazy-getter-module-failure|caught TypeError|Bun\.sql' test src packages 2>/dev/null | head -n 240Repository: oven-sh/bun
Length of output: 18427
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate source files ---'
git ls-files | rg '(^|/)(bun-object|lazy|getters?|sql).*\.(ts|zig|cpp|h|hpp|js)$' | head -n 160
printf '\n--- lazy getter and module failure references ---\n'
rg -n -C 5 'lazy.*getter|getter.*lazy|module.*fail|evaluation error|Bun\.sql|Bun\.SQL' src test --glob '*.{ts,tsx,js,jsx,zig,cpp,c,h,hpp}' | head -n 320
printf '\n--- relevant test conventions ---\n'
rg -n -C 5 'Bun\.spawn\(|stdout\.trim\(\)|e\.constructor\.name|caught = e' test/js test/regression --glob '*.test.{ts,tsx,js,jsx}' 2>/dev/null | head -n 260Repository: oven-sh/bun
Length of output: 48091
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- SQL API generation input ---'
file=$(git ls-files | rg '(^|/)sql\.classes\.ts$' | head -n 1)
printf '%s\n' "FILE=$file"
wc -l "$file"
cat -n "$file" | sed -n '1,240p'
printf '\n--- Bun SQL exports and property registration ---\n'
rg -n -C 6 'sql\.classes|SQL|defaultSQLObject|lazyDefaultSQL|putDirect.*SQL|define.*SQL|property.*sql' src/runtime src/js src/jsc --glob '*.{ts,zig,cpp,c,h,hpp}' | head -n 360
printf '\n--- standalone output-invariant probe ---\n'
node - <<'JS'
function current({ sqlThrows, SQLThrows }) {
let caught = "";
try {
if (sqlThrows) throw new TypeError("sql failure");
} catch (e) {
caught = e.constructor.name;
}
try {
if (SQLThrows) throw new TypeError("SQL failure");
} catch (e) {}
return "caught " + caught;
}
function proposed({ sqlThrows, SQLThrows }) {
let caught = "";
let sqlCaught = "";
try {
if (sqlThrows) throw new TypeError("sql failure");
} catch (e) {
caught = e.constructor.name;
}
try {
if (SQLThrows) throw new TypeError("SQL failure");
} catch (e) {
sqlCaught = e.constructor.name;
}
return "caught " + caught + " SQL " + sqlCaught;
}
for (const SQLThrows of [false, true]) {
const input = { sqlThrows: true, SQLThrows };
console.log(JSON.stringify({ input, current: current(input), proposed: proposed(input) }));
}
JSRepository: oven-sh/bun
Length of output: 24117
Assert the Bun.SQL failure.
The empty catch does not prove that Bun.SQL throws. Capture its error type, include it in the output, and update the expected output. Otherwise, this test can pass when Bun.SQL returns normally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/js/bun/bun-object/lazy-getter-module-failure.test.ts` around lines 26 -
30, Update the inline Bun.SQL failure test to capture the thrown error type in
the catch block and include it in the logged output, then update the expected
output accordingly. Ensure the test explicitly fails or reports when Bun.SQL
returns normally rather than throwing.
Source: Coding guidelines
| stderr: "pipe", | ||
| }); | ||
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); | ||
| expect(stdout.trim()).toBe("caught TypeError"); | ||
| expect(exitCode).toBe(0); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file=$(git ls-files 'test/js/bun/bun-object/lazy-getter-module-failure.test.ts' | head -n 1)
printf '%s\n' "FILE=$file"
if [ -n "$file" ]; then
wc -l "$file"
cat -n "$file"
fi
printf '%s\n' '--- relevant subprocess patterns ---'
rg -n -C 4 'lazy-getter-module-failure|stderr:\s*"pipe"|Promise\.all\(\[.*proc\.stdout|proc\.stderr\.text|proc\.exited' test/js/bun/bun-object -g '*.test.ts' || trueRepository: oven-sh/bun
Length of output: 4074
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- review guidance files ---'
git ls-files | rg '(^|/)REVIEW\.md$|situational|landing' || true
printf '%s\n' '--- subprocess assertions in neighboring tests ---'
rg -n -C 5 'stderr:\s*"pipe"|proc\.stderr\.text\(\)|proc\.exited' test/js/bun/bun-object -g '*.test.ts' | head -n 240
printf '%s\n' '--- bunEnv definition and usage ---'
rg -n -C 8 'export\s+(const|let)\s+bunEnv|bunEnv\s*=' test packages scripts -g '*.ts' -g '*.js' 2>/dev/null | head -n 160 || true
printf '%s\n' '--- structural verifier for the target subprocess ---'
python3 - <<'PY'
from pathlib import Path
import re
path = Path("test/js/bun/bun-object/lazy-getter-module-failure.test.ts").resolve()
text = path.read_text()
spawn = re.search(r'Bun\.spawn\(\{(?P<body>.*?)\n\s*\}\)', text, re.S)
assert spawn, "target Bun.spawn call not found"
body = spawn.group("body")
assert 'stdout: "pipe"' in body
assert 'stderr: "pipe"' in body
awaits = re.search(r'const \[(?P<vars>[^\]]+)\]\s*=\s*await Promise\.all\(\[(?P<items>[^\]]+)\]\)', text)
assert awaits, "subprocess Promise.all not found"
vars_ = [v.strip() for v in awaits.group("vars").split(",")]
items = [i.strip() for i in awaits.group("items").split(",")]
print("absolute_test_path:", path)
print("configured_streams:", [x for x in ("stdout", "stderr") if f'{x}: "pipe"' in body])
print("awaited_values:", vars_)
print("awaited_expressions:", items)
print("stderr_is_consumed:", any("proc.stderr" in item for item in items))
print("stderr_is_asserted:", bool(re.search(r'expect\([^)]*stderr|expect\(\{[^}]*stderr', text)))
PYRepository: oven-sh/bun
Length of output: 10585
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- neighboring combined assertion ---'
sed -n '220,250p' test/js/bun/bun-object/deep-equals.test.ts
printf '%s\n' '--- bunEnv quiet settings ---'
sed -n '60,82p' test/harness.ts
printf '%s\n' '--- standalone unread-pipe probe ---'
python3 - <<'PY'
import errno
import fcntl
import os
read_fd, write_fd = os.pipe()
try:
flags = fcntl.fcntl(write_fd, fcntl.F_GETFL)
fcntl.fcntl(write_fd, fcntl.F_SETFL, flags | os.O_NONBLOCK)
chunk = b"x" * 4096
total = 0
while True:
try:
total += os.write(write_fd, chunk)
except BlockingIOError as exc:
assert exc.errno == errno.EAGAIN
break
print("unread_pipe_write_bytes_before_block:", total)
print("unread_pipe_can_block_writer:", total > 0)
finally:
os.close(write_fd)
os.close(read_fd)
PYRepository: oven-sh/bun
Length of output: 2342
Drain and assert the piped stderr.
Read proc.stderr.text() concurrently with stdout and proc.exited. Assert that stderr is empty with the expected stdout and exit code.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/js/bun/bun-object/lazy-getter-module-failure.test.ts` around lines 34 -
38, Update the process result handling around proc.stdout and proc.exited to
also read proc.stderr.text() concurrently. Capture the stderr result and assert
that it is empty alongside the existing stdout and exitCode assertions.
Sources: Coding guidelines, Learnings
|
Closing as a duplicate: #37160 already removes these same two debug-report blocks as part of a broader fix for stale exceptions from failed lazy property initialization, and #37175 covers the inspect-side variants. For that PR's benefit, the reduced deterministic repro from the fuzzer investigation is three lines on a debug build: process._fatalException; globalThis.Object = undefined; Bun.sql. With _fatalException reified it aborts at the JSObjectInlines.h(137) exception-scope assert inside Bun__handleUncaughtException's process get, and without it at a Structure assert in the process.emit path, so a regression test covering both variants fails before the fix and passes after (see test/js/bun/bun-object/lazy-getter-module-failure.test.ts on this branch, free to lift). |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — removes two debug-only reportUncaughtExceptionAtEventLoop calls that fired with the exception still pending (every other caller clears first), letting the module-evaluation failure propagate via the existing RETURN_IF_EXCEPTION. The stderr-drain nit on the test is worth fixing but not blocking.
Extended reasoning...
Overview
Two three-line #if BUN_DEBUG blocks are removed from defaultBunSQLObject and constructBunSQLObject in src/jsc/bindings/BunObject.cpp. These blocks called reportUncaughtExceptionAtEventLoop while the exception was still pending on the VM, then immediately hit RETURN_IF_EXCEPTION to propagate that same exception — so the exception was never actually uncaught, and reporting it while still pending violates JSC's exception-scope invariant (the !scope.exception() || ... || !hasProperty assertion in JSObject::get). A new subprocess test in test/js/bun/bun-object/ reproduces both crash variants.
Security risks
None. This is debug-build-only code being deleted; release builds compiled it out already. No auth, crypto, permissions, or untrusted-input parsing is touched.
Level of scrutiny
Low. The removed code was semantically wrong (reporting an exception as uncaught while simultaneously propagating it) and unique to these two sites — I grepped every other reportUncaughtExceptionAtEventLoop call site in src/jsc/bindings/ and confirmed they all tryClearException() first, matching the PR description's claim. What remains after removal is the standard requireId → RETURN_IF_EXCEPTION → RELEASE_AND_RETURN shape used by every other lazy property callback in this file.
Other factors
The one finding is a test-quality nit: stderr: "pipe" is configured but never drained, so on regression the JSC assertion text would be discarded. Deadlock risk is negligible (abort output is well under the 64KB pipe buffer); the impact is purely diagnosability of a future failure. It does not affect the correctness of the fix or the test's pass/fail behavior on the fixed vs. unfixed build, so it does not block approval. The test correctly covers both the reified and non-reified _fatalException variants and both getters (Bun.sql and Bun.SQL).
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); | ||
| expect(stdout.trim()).toBe("caught TypeError"); |
There was a problem hiding this comment.
🟡 The test sets stderr: "pipe" but never drains it — only proc.stdout.text() and proc.exited are awaited. On a regressed debug build the assertion-abort message goes to stderr and would be silently discarded, leaving only an opaque expected "caught TypeError", got "" failure. Add proc.stderr.text() to the Promise.all and assert on a combined { stdout, stderr, exitCode } object (or drop stderr: "pipe" to inherit).
Extended reasoning...
What the bug is
The subprocess test configures stderr: "pipe" at line 34 but the drain at line 36 only reads stdout:
stderr: "pipe",
});
const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]);
expect(stdout.trim()).toBe("caught TypeError");
expect(exitCode).toBe(0);REVIEW.md's subprocess-test rule is explicit: "Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]) — an unread pipe fills the ~64KB OS buffer and deadlocks the child. assert a combined { stdout, stderr, exitCode } object." CLAUDE.md's canonical test template shows the same three-way drain.
Code path that triggers it
The test spawns bun -e with a script that clobbers globalThis.Object and then touches Bun.sql / Bun.SQL. On the fixed build the child writes ~20 bytes to stdout and nothing to stderr, so the unread pipe never fills and the test passes. But this test exists specifically to guard a debug-build assertion abort — and on the unfixed build (or any future regression), the child aborts with:
ASSERTION FAILED: !scope.exception() || vm.hasPendingTerminationException() || !hasProperty
JavaScriptCore/JSObjectInlines.h(137) : JSValue JSC::JSObject::get(...)
That message, plus the ASan/backtrace dump, goes to stderr. Because stderr is piped but never read, that output is discarded when the process is reaped.
Why existing code doesn't prevent it
Nothing in the test consumes proc.stderr. The await using proc disposer will close the pipe on exit, but by then the diagnostic bytes are gone — they were never collected into a JS string, so they can't appear in the test-failure output.
Impact
- Deadlock risk: negligible. Even the abort path writes well under the ~64KB OS pipe buffer, so the child won't block on a full stderr pipe in practice.
- Diagnosability: real. When this test regresses, CI will report only
expected "caught TypeError", got ""andexitCode !== 0, with no indication of which JSC assertion tripped or in which of the tworeifyFatalExceptionvariants. The whole point of this test is to catch a specific assertion; throwing away the assertion text defeats that.
Step-by-step proof
- Revert the
BunObject.cppchange (re-add the#if BUN_DEBUGreport block). - Run
bun bd test test/js/bun/bun-object/lazy-getter-module-failure.test.ts. - The child aborts;
proc.exitedresolves to a nonzero code / signal;proc.stdout.text()resolves to"". - The test fails on
expect("").toBe("caught TypeError"). - The
ASSERTION FAILED: !scope.exception() ...line and backtrace were written to the child's stderr pipe, which was never read — the failure output shows nothing about the actual crash.
Fix
const [stdout, stderr, exitCode] = await Promise.all([
proc.stdout.text(),
proc.stderr.text(),
proc.exited,
]);
expect({ stdout: stdout.trim(), stderr, exitCode }).toEqual({
stdout: "caught TypeError",
stderr: "",
exitCode: 0,
});Alternatively, drop stderr: "pipe" so stderr inherits to the test runner's stderr — then a regression's abort message surfaces directly in CI logs. Either satisfies the REVIEW.md rule; the combined-object assertion is the repo convention.
1 participant
What
Fuzzilli has been hitting this flaky assertion for a while (17 reports of the same fingerprint):
A report finally arrived with enough stderr context to reconstruct the donor-process state, and it reduces to a deterministic three liner:
The chain: the
Bun.sql/Bun.SQLlazy getters carry a debug-only block that, when the BunSql internal module fails to evaluate, passes the failure toreportUncaughtExceptionAtEventLoopwith the exception still pending on the VM. That handler entersBun__handleUncaughtException, which doesprocess->get(globalObject, "_fatalException"); entering that get with a pending exception and an existing property is exactly the asserted condition. When_fatalExceptionhas not been reified yet, the handler instead falls into theprocess.emitpath with the pending exception and trips a Structure assertion, so both crash signatures share this root cause.The report is also semantically wrong: the exception is not uncaught.
RETURN_IF_EXCEPTIONon the next line propagates it, and the property access throws it to the caller (every other caller of the reporter clears the exception first, for exampleconstructStdioWriteStream).Fix
Remove the two
#if BUN_DEBUGreport blocks. The module failure now propagates cleanly:Bun.sqlthrows the evaluation error as a catchable exception and the process stays healthy.Test
test/js/bun/bun-object/lazy-getter-module-failure.test.tsruns the reduced repro in a subprocess for both variants (with and without_fatalExceptionreified). Both abort the unfixed debug build with the respective assertions and pass with the fix; release builds were unaffected (asserts compiled out), which is why the fuzzer only saw this on the ASAN lane.Related: the fingerprint also has slower-burn instances in event-loop completion paths with swallowed settle errors; those are tracked separately (#37004 for the DNS drains, #34829 for valkey). This PR fixes the instance that made the fuzzer reports reproducible.