Skip to content

Fix stale exception when a lazy property initializer throws during property enumeration #37158

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 1 commit into from
Closed

Fix stale exception when a lazy property initializer throws during property enumeration #37158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 4 additions & 3 deletions src/jsc/bindings/bindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5378,10 +5378,11 @@ static void JSC__JSValue__forEachPropertyImpl(JSC::EncodedJSValue JSValue0, JSC:
}

JSC::PropertySlot slot(object, PropertySlot::InternalMethodType::Get);
if (!object->getPropertySlot(globalObject, property, slot))
continue;
// Ignore exceptions from "Get" proxy traps.
bool hasProperty = object->getPropertySlot(globalObject, property, slot);
// Ignore exceptions from "Get" proxy traps and throwing lazy property callbacks.
CLEAR_IF_EXCEPTION(scope);
if (!hasProperty)
continue;

if ((slot.attributes() & PropertyAttribute::DontEnum) != 0) {
if (property == propertyNames->underscoreProto
Expand Down
27 changes: 27 additions & 0 deletions test/js/bun/util/inspect.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -803,3 +803,30 @@
}"
`);
});

it("skips lazy properties whose initializer throws without leaving a stale exception", async () => {
// With the global Symbol overwritten, reifying Bun.$ (and Bun.sql) throws.
// The exception must be cleared before moving to the next property:
// enumeration should keep the remaining properties, and a direct access
// should throw a catchable error.
const code = `
globalThis.Symbol = 0;
const out = Bun.inspect(Bun);
for (const name of ["Archive", "CryptoHasher", "serve", "spawn"]) {
if (!out.includes(name)) throw new Error("missing " + name);
}
let caught = 0;
try { Bun.$; } catch { caught++; }
try { Bun.sql; } catch { caught++; }
console.log("OK", caught);
`;
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", code],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]);
expect(stdout.trim()).toBe("OK 2");

Check warning on line 830 in test/js/bun/util/inspect.test.js

View check run for this annotation

Claude / Claude Code Review

Subprocess test pipes stderr but never drains it 

The test sets `stderr: "pipe"` but never drains it — only `proc.stdout.text()` and `proc.exited` are awaited. Match the neighboring "huge sparse array" test and REVIEW.md's subprocess rule: `const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited])`, then assert the combined `{ stdout: stdout.trim(), stderr, exitCode }` object. Otherwise a regression that reintroduces the debug abort will show only `received ""` with the stderr backtrace silently
Comment on lines +828 to +830

@claude claude Bot Aug 7, 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.

🟡 The test sets stderr: "pipe" but never drains it — only proc.stdout.text() and proc.exited are awaited. Match the neighboring "huge sparse array" test and REVIEW.md's subprocess rule: const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]), then assert the combined { stdout: stdout.trim(), stderr, exitCode } object. Otherwise a regression that reintroduces the debug abort will show only received "" with the stderr backtrace silently discarded.

Extended reasoning...

What the bug is

The new test at test/js/bun/util/inspect.test.js:823-830 spawns a subprocess with stderr: "pipe" but only awaits proc.stdout.text() and proc.exited:

await using proc = Bun.spawn({
  cmd: [bunExe(), "-e", code],
  env: bunEnv,
  stdout: "pipe",
  stderr: "pipe",
});
const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]);
expect(stdout.trim()).toBe("OK 2");
expect(exitCode).toBe(0);

The stderr pipe is opened but never read. REVIEW.md rejects this pattern explicitly under Tests reviewers reject → Subprocess tests: drain pipes concurrently: "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."

Why local convention also demands it

The immediately neighboring subprocess test in the same file — "Bun.inspect huge sparse array summarizes holes without iterating them" — follows the correct pattern exactly:

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ stdout, stderr, exitCode }).toEqual({ stdout: "...", stderr: "", exitCode: 0 });

REVIEW.md separately requires "Match the exact file's local conventions", so this test diverges from both the repo-wide rule and the pattern established a few lines above it.

Step-by-step: what goes wrong on a regression

The whole point of this test is to catch a regression of the debug-build abort described in the PR (ASSERTION FAILED: Unexpected exception observed). Walk through what happens if that abort comes back:

  1. Child runs globalThis.Symbol = 0; Bun.inspect(Bun); — the lazy Bun.$ callback throws, the stale exception leaks, and the debug build aborts with a JSC assertion.
  2. The assertion message and backtrace are written to stderr.
  3. The child exits before printing "OK 2" to stdout.
  4. The parent reads stdout → "", and exitCode → non-zero (or a signal).
  5. The test failure prints only: expect(stdout.trim()).toBe("OK 2") — Expected "OK 2", Received "".

The actual diagnostic — the assertion text and backtrace on stderr — is silently discarded because nothing ever read the pipe. Whoever debugs the CI failure has no idea why the child died. With the combined-object assertion, the failure output would instead show stderr: "ASSERTION FAILED: Unexpected exception observed... <backtrace>" and exitCode: <signal>, which is exactly the signal you want.

Deadlock direction

Bun.inspect(Bun) reifies every lazy property on the Bun object; with Symbol clobbered several of these ($, sql, postgres, SQL) throw during reification. In the current passing path stderr stays empty, so the ~64KB pipe-buffer deadlock does not occur today. But the pattern is what REVIEW.md rejects regardless — if any of those reification paths (or a future warning) becomes noisy on stderr, the child could block writing to a full pipe while the parent only drains stdout.

Fix

One-line change to match the neighboring test:

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ stdout: stdout.trim(), stderr, exitCode }).toEqual({ stdout: "OK 2", stderr: "", exitCode: 0 });

Severity

Nit. The test passes as written and nothing concretely breaks today — the deadlock risk is theoretical for this particular script, and the diagnostic loss only bites on a future regression. But it violates an explicit repo review rule, diverges from the immediate neighboring convention, and the fix is trivial.

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