process: handle exception in nextTick lazy property callback#30441
Closed
robobun wants to merge 2 commits into
Closed
process: handle exception in nextTick lazy property callback#30441robobun wants to merge 2 commits into
robobun wants to merge 2 commits into
Conversation
constructNextTickFn is a LazyPropertyCallback that runs JS via profiledCall. If that throws (e.g. stack overflow near the limit), the callback returned the Exception* it got back from executeCall and left the exception pending. reifyStaticProperty then stored that Exception as the property value, and JSObject::get asserted because getPropertySlot reported the property as found while an exception was set. Match constructStdin/Stdout/Stderr: catch the exception, report it via reportUncaughtExceptionAtEventLoop, and return jsUndefined() so the property is reified to a sane value.
Collaborator
Author
|
Updated 4:12 PM PT - May 9th, 2026
❌ @autofix-ci[bot], your commit 2e56e1d has 3 failures in
🧪 To try this PR locally: bunx bun-pr 30441That installs a local version of the PR into your bun-30441 --bun |
Contributor
WalkthroughThis change adds exception-safety in Process::constructNextTickFn to clear and report exceptions during initializer construction or profiledCall and return undefined on error. It also adds a regression test ensuring process.nextTick is not an Exception object after a stack overflow and the child exits normally. ChangesProcess.nextTick Exception Safety
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/node/process/process-nexttick-stack-overflow.test.ts`:
- Around line 28-33: The test currently only asserts proc.signalCode is null
which can still allow a non-zero exit; after the stdout expectations (using
stdout.trim()) add an explicit assertion that exitCode is 0 (the resolved
exitCode variable from Promise.all) to ensure the subprocess succeeded; keep
stdout checks before the exit code check so failures show helpful output, and
reference the variables stdout, stderr, exitCode and proc.signalCode when making
the change.
🪄 Autofix (Beta)
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: 4a3d794c-60d7-4249-aeba-8b8d16fb9095
📒 Files selected for processing (2)
src/jsc/bindings/BunProcess.cpptest/js/node/process/process-nexttick-stack-overflow.test.ts
Comment on lines
+28
to
+33
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stdout.trim()).not.toBe("typeof=object"); | ||
| expect(["typeof=function", "typeof=undefined"]).toContain(stdout.trim()); | ||
| expect(proc.signalCode).toBeNull(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Assert exitCode to avoid false-positive passes.
signalCode === null does not guarantee success; the subprocess can still exit non-zero and this test would pass. Add an explicit exitCode assertion (after stdout checks).
Suggested patch
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stdout.trim()).not.toBe("typeof=object");
expect(["typeof=function", "typeof=undefined"]).toContain(stdout.trim());
+ expect(stderr).toBe("");
expect(proc.signalCode).toBeNull();
+ expect(exitCode).toBe(0);
});As per coding guidelines: "When spawning processes in tests, check stdout expectations before exit code expectations to provide more useful error messages on test failure".
📝 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 [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | |
| expect(stdout.trim()).not.toBe("typeof=object"); | |
| expect(["typeof=function", "typeof=undefined"]).toContain(stdout.trim()); | |
| expect(proc.signalCode).toBeNull(); | |
| }); | |
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | |
| expect(stdout.trim()).not.toBe("typeof=object"); | |
| expect(["typeof=function", "typeof=undefined"]).toContain(stdout.trim()); | |
| expect(stderr).toBe(""); | |
| expect(proc.signalCode).toBeNull(); | |
| expect(exitCode).toBe(0); | |
| }); |
🤖 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/node/process/process-nexttick-stack-overflow.test.ts` around lines 28
- 33, The test currently only asserts proc.signalCode is null which can still
allow a non-zero exit; after the stdout expectations (using stdout.trim()) add
an explicit assertion that exitCode is 0 (the resolved exitCode variable from
Promise.all) to ensure the subprocess succeeded; keep stdout checks before the
exit code check so failures show helpful output, and reference the variables
stdout, stderr, exitCode and proc.signalCode when making the change.
Collaborator
Author
There was a problem hiding this comment.
The subprocess exits 1 with the fix because the stack overflow is reported via reportUncaughtExceptionAtEventLoop (same as constructStdin/Stdout/Stderr). Asserting exitCode === 0 would make the test fail on the fixed build. The signalCode check covers the actual regression (SIGABRT from the assertion).
Contributor
There was a problem hiding this comment.
@robobun, you're right — my suggestion was incorrect. Since reportUncaughtExceptionAtEventLoop reports the stack overflow and exits with code 1 (same as constructStdin/Stdout/Stderr), asserting exitCode === 0 would incorrectly fail on the fixed build. The signalCode === null check is the appropriate assertion here: the regression being guarded against is a SIGABRT from the JSObject::get assertion failure, not a clean non-zero exit. Apologies for the noise.
ʕ •ᴥ•ʔ 🐇
✏️ Learnings added
Learnt from: robobun
Repo: oven-sh/bun PR: 30441
File: test/js/node/process/process-nexttick-stack-overflow.test.ts:28-33
Timestamp: 2026-05-09T22:04:49.296Z
Learning: In oven-sh/bun, when a LazyPropertyCallback (e.g., `constructNextTickFn` in `src/jsc/bindings/BunProcess.cpp`) handles an exception via `reportUncaughtExceptionAtEventLoop` (mirroring `constructStdin`/`constructStdout`/`constructStderr`), the spawned subprocess exits with code 1—not 0. In regression tests for this pattern (e.g., `test/js/node/process/process-nexttick-stack-overflow.test.ts`), do NOT assert `expect(exitCode).toBe(0)`. The correct assertion is `expect(proc.signalCode).toBeNull()`, which guards against the actual regression (SIGABRT from the JSC `JSObject::get` assertion) while allowing the expected non-zero exit.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: robobun
Repo: oven-sh/bun PR: 27056
File: test/bundler/standalone.test.ts:281-324
Timestamp: 2026-02-16T04:26:25.185Z
Learning: In Bun test files that exercise Bun.build(), assertions for configuration-validation errors thrown synchronously by JSBundler.fromJS() (via globalThis.throwInvalidArguments()) should use toThrow, e.g., expect(() => Bun.build({...})).toThrow()). Do not use rejects.toThrow() since rejections occur only for asynchronous build errors.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 27385
File: test/js/bun/http/tls-keepalive.test.ts:115-140
Timestamp: 2026-02-24T21:02:00.725Z
Learning: In Bun's test suites, avoid adding tests for trivial environment/fixture script validation (e.g., checking if env vars exist) within test fixtures. Focus test coverage on actual behavior being tested (e.g., TLS keepalive, memory leaks) rather than auxiliary fixture validation. If a test file is primarily for fixtures, skip or limit tests that validate simple JS behavior like if (!env) throw; prioritize meaningful end-to-end or unit behavior instead.
Learnt from: LawoodDev
Repo: oven-sh/bun PR: 27855
File: test/cli/run/concurrency-filter.test.ts:32-32
Timestamp: 2026-03-06T16:21:42.189Z
Learning: In Bun's test runner, describe.concurrent is supported (since Bun v1.2.23). Use describe.concurrent/test.concurrent for concurrent tests. Be aware of limitations: expect.assertions() and expect.hasAssertions() are not supported; toMatchSnapshot() is not supported (toMatchInlineSnapshot() is); and beforeAll/afterAll hooks are not executed concurrently. The broader guideline to prefer concurrent tests over sequential tests using test.concurrent or describe.concurrent remains valid and should be applied to test files such as test/cli/run/concurrency-filter.test.ts and similar test files.
Learnt from: LawoodDev
Repo: oven-sh/bun PR: 27855
File: test/cli/run/concurrency-filter.test.ts:32-32
Timestamp: 2026-03-06T16:22:55.570Z
Learning: In test/cli/run/concurrency-filter.test.ts and similar test files, timing-sensitive tests that assert on wall-clock elapsed time to verify concurrency behavior (e.g., expect(elapsed).toBeGreaterThan(800)) must remain in a sequential describe block rather than describe.concurrent. Running such tests concurrently can cause CPU contention and skew timing assertions, leading to flaky results. The guideline to prefer describe.concurrent does NOT apply for timing-based correctness verification.
Learnt from: robobun
Repo: oven-sh/bun PR: 28214
File: test/regression/issue/18115.test.ts:1-158
Timestamp: 2026-03-18T15:19:38.407Z
Learning: In Bun test files, when a resource like tempDir is a DisposableString implementing both Symbol.dispose (sync) and Symbol.asyncDispose, prefer plain using over await using. Do not recommend converting to await using for tempDir in Bun test files. This keeps tests idiomatic and avoids unnecessary async disposal. If a resource only supports asyncDispose, use await using.
Learnt from: robobun
Repo: oven-sh/bun PR: 28308
File: test/js/node/net/blocklist-gc.test.ts:4-12
Timestamp: 2026-03-20T09:29:51.349Z
Learning: In oven-sh/bun regression/unit test files under `test/js/node/**` (e.g., `**/*.test.ts`), avoid adding inline comments inside the test body that restate the bug context or reference the issue/PR number being tested. Rely on PR descriptions/commit messages for that context and keep the test code focused on setup, action, and assertions.
Learnt from: robobun
Repo: oven-sh/bun PR: 28425
File: test/regression/issue/28422.test.ts:65-79
Timestamp: 2026-03-22T10:12:05.719Z
Learning: In oven-sh/bun test files matching test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}, follow CLAUDE.md by asserting the command exit code LAST—after all other assertions such as stdout/stderr checks and filesystem validation. Do not assert exitCode earlier than those checks. Also, avoid asserting stdout for commands like bun install whose output can vary between runs.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 28863
File: scripts/build/deps/webkit.ts:149-161
Timestamp: 2026-04-04T19:43:49.607Z
Learning: When reviewing Node/TypeScript code that uses `node:path.join()`, do not treat a later path segment that starts with `/` as a Windows/absolute-path override bug. `path.join()` concatenates segments and normalizes; it only resets the root when using `path.resolve()` (e.g., when it encounters an absolute-looking segment). Therefore, patterns like `join(base, "/relPath")` or `join(homedir(), env.slice(1))` where `env.slice(1)` becomes `"/WebKit"` are expected to produce `base/relPath` (cross-platform). Only flag cases where `path.resolve()` (or other root-resetting logic) is used in a way that could unintentionally ignore the base path.
Learnt from: robobun
Repo: oven-sh/bun PR: 28923
File: test/regression/issue/28921.test.ts:0-0
Timestamp: 2026-04-06T19:19:08.790Z
Learning: In oven-sh/bun tests, prefer `tempDir` (from the `harness` module) over `tempDirWithFiles` when using the `using` statement for automatic cleanup. `tempDirWithFiles(...)` returns a plain `string`, so `using tempDirWithFiles(...)` is effectively a no-op and will not trigger disposal/cleanup. `tempDir` returns a `DisposableString` that implements `Symbol.dispose`, so it will correctly trigger cleanup on scope exit.
Learnt from: robobun
Repo: oven-sh/bun PR: 29050
File: test/regression/issue/29042.test.ts:60-94
Timestamp: 2026-04-08T21:22:00.840Z
Learning: In this repo’s Bun environment, `Bun.RedisClient` does not implement `Symbol.dispose` or `Symbol.asyncDispose`, so you cannot rely on `using` / `await using` for automatic cleanup. When creating a `Bun.RedisClient` in tests, close it explicitly with `try/finally`, calling `client.close()` in the `finally` block.
Learnt from: robobun
Repo: oven-sh/bun PR: 29322
File: test/js/web/workers/worker-terminate-after-exit.test.ts:38-43
Timestamp: 2026-04-15T01:57:52.469Z
Learning: In oven-sh/bun test files (matching `test/**/*.test.ts`), when you spawn a subprocess in a bun:test and you assert on its exit code, follow the CLAUDE.md house style: write `if (exitCode !== 0) { expect(stderr).toBe(""); }` immediately before `expect(exitCode).toBe(0)`. This is intentional so that, on failure, bun:test surfaces the full `stderr` content in the diff output. Do not replace this with a custom/second assertion that formats stderr into the exit-code expectation (e.g., `expect(exitCode, \\`stderr: ${stderr}\\`).toBe(0)` or any single-assertion equivalent).
Learnt from: robobun
Repo: oven-sh/bun PR: 29389
File: test/js/bun/util/v8-heap-snapshot-large-strings.test.ts:4-152
Timestamp: 2026-04-17T02:55:14.338Z
Learning: In oven-sh/bun, do not enforce the `test/regression/issue/${issueNumber}.test.ts` placement rule based solely on PR descriptions that include a speculative GitHub issue link like “might fix `#NNNNN`” without a confirmed regression (e.g., no verifying stack trace/reproduction). If the issue is not confirmed per CLAUDE.md (“confirmed numbered issue” only), the test should be placed next to the closest related existing test file for the affected feature/module (e.g., alongside `test/js/bun/util/v8-heap-snapshot.test.ts`) and should not be flagged as a guideline violation. Likewise, tests that validate a broader behavioral invariant (e.g., V8-matching 1024-char string truncation in heap snapshots) are not purely issue regressions and should live with the feature’s existing test suite rather than under `test/regression/issue/`.
Learnt from: robobun
Repo: oven-sh/bun PR: 29426
File: test/js/node/tls/node-tls-root-certs-concurrent-init.test.ts:80-82
Timestamp: 2026-04-18T00:50:38.905Z
Learning: In oven-sh/bun Jest/Bun test files under `test/js/` that spawn subprocesses using `bunEnv` from the `harness` module, it’s safe and intentional to assert `expect(stderr).toBe("")` unconditionally. `bunEnv` sets `BUN_DEBUG_QUIET_LOGS=1`, which suppresses ASAN/debug-build stderr noise, so an unexpected stderr value should fail the test and show useful diagnostics. Do not gate `expect(stderr).toBe("")` behind `if (exitCode !== 0)` for these `bunEnv`-based subprocess tests—follow the established pattern used in similar tests (e.g., `test/js/node/tls/test-use-system-ca.test.ts`).
Learnt from: robobun
Repo: oven-sh/bun PR: 29441
File: test/js/web/broadcastchannel/broadcast-channel-worker-gc.test.ts:10-14
Timestamp: 2026-04-18T10:36:45.033Z
Learning: In oven-sh/bun test files that spawn subprocesses using `bunEnv`, suppress the known ASAN startup noise in the subprocess stderr before asserting it is empty. Use the repo’s established convention: split stderr into lines and filter with `.filter(line => !line.startsWith("WARNING: ASAN interferes"))`, then assert the remaining stderr lines are empty. Do not switch to an alternative like `str.replace(...)`; the filter-based approach is the repo convention. This is safe because `ZigGlobalObject.cpp` emits that warning via `std::call_once`, so at most one matching line appears per process.
Learnt from: robobun
Repo: oven-sh/bun PR: 29564
File: test/regression/issue/29513.test.ts:51-51
Timestamp: 2026-04-22T02:58:30.645Z
Learning: In oven-sh/bun TypeScript test files, it is acceptable to use `Bun.sleep(0)` specifically as a macrotask barrier to deterministically drain the pending microtask queue before asserting. Do NOT flag `Bun.sleep(0)` as a timing-wait violation. The “do not use setTimeout/Bun.sleep in tests” guideline is intended to prevent load-sensitive wall-clock delays (e.g., `Bun.sleep(100)` or other timing windows). Use `Bun.sleep(0)` only when you need to observe a fully settled Promise/microtask chain (e.g., after deferred resolution and multiple internal `.then()` hops) where a single `await Promise.resolve()` would not advance far enough; `Bun.sleep(0)` resumes in a later macrotask after pending microtasks complete, without relying on elapsed time.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 29581
File: src/bun.js/modules/NodeModuleModule.cpp:663-681
Timestamp: 2026-04-22T20:47:10.896Z
Learning: In oven-sh/bun code reviews, do not recommend adding standalone regression tests that depend on setting `BUN_JSC_validateExceptionChecks=1` to exercise JSC throw-scope/exception-scope validator paths (e.g., PropertyCallback/reify interactions like `reifyAllStaticProperties`). Per `CLAUDE.md`, tests are expected to pass with `USE_SYSTEM_BUN=1`, and `BUN_JSC_validateExceptionChecks` is a no-op on release/system Bun builds. Instead, treat this class of validator coverage issue as covered by: (1) the x64-asan CI shard that enables the validator automatically, and (2) the `test/no-validate-exceptions.txt` opt-out list for tests that hit pre-existing throw-scope assertion failures unrelated to the change under review. If helpful, add an in-source comment pointing to the specific existing exerciser (e.g., the relevant `tsgo/bun-types` test) to document the intent without relying on the env var.
Learnt from: robobun
Repo: oven-sh/bun PR: 29820
File: test/js/node/process/process-execve.test.ts:47-52
Timestamp: 2026-04-28T11:35:58.257Z
Learning: In oven-sh/bun test files under `test/**/*.test.ts`, when a test uses the `tempDir` fixture and spawns a subprocess via `await using proc = Bun.spawn(...)` (i.e., the embedded script runs as a spawned subprocess), do not recommend adding a fixture-level or embedded-script `setTimeout` watchdog to prevent hangs. The `await using` scope exit should terminate the subprocess automatically, and Bun test per-test timeouts already bound execution time. Also, avoid embedded `setTimeout` watchdog patterns that violate Bun’s “no setTimeout in tests” guideline. If the worker/subprocess exits silently without posting, rely on the test’s stdout/exitCode assertions plus Bun’s outer timeout rather than a watchdog, even when the embedded fixture script uses `worker_threads` or other async constructs.
Learnt from: robobun
Repo: oven-sh/bun PR: 29874
File: test/js/web/websocket/websocket-proxy-tunnel-upgrade-leak.test.ts:15-16
Timestamp: 2026-04-28T21:34:23.491Z
Learning: In oven-sh/bun, when a test is intentionally validating native refcount leak detection using Bun debug-only instrumentation (e.g., `BUN_DEBUG_alloc=1` and `[alloc] new(...)/destroy(...)` log lines produced only by debug builds when `Environment.enable_logs` is set), use `test.skipIf(!isDebug)` as the correct/intentional guard. Do not flag this `test.skipIf(!isDebug)` as a guideline violation for this class of tests. The debug-only `[alloc] ...` lines are absent in release and ASAN builds, and there is no equivalent observable system-Bun hook to assert a leak when only debug-build instrumentation exists (so the `USE_SYSTEM_BUN=1` rule in `CLAUDE.md` does not apply in this situation).
Learnt from: robobun
Repo: oven-sh/bun PR: 30073
File: test/js/node/fs/cp-symlink-target.test.ts:76-96
Timestamp: 2026-05-02T08:03:34.343Z
Learning: In oven-sh/bun, when adding a new test for an existing feature area (e.g., `fs.cp`) that must also be runnable under Node’s test runner (e.g., `node --test`, potentially with flags like `--experimental-strip-types`), it is intentional to place the test in a separate `*.test.ts` file (e.g., `test/js/node/fs/cp-symlink-target.test.ts`) rather than merging it into an existing sibling file (e.g., `test/js/node/fs/cp.test.ts`). Do not flag these as violations of an "add tests to existing files" guideline when the existing file depends on Bun-only modules (e.g., imports from `bun:test` or `harness`) that Node cannot load. As a check, allow standalone dual-runtime Node-compatible tests that only use `node:*` built-ins plus `node:test` and `node:assert`.
Learnt from: robobun
Repo: oven-sh/bun PR: 30073
File: test/js/node/fs/cp-symlink-target.test.ts:21-23
Timestamp: 2026-05-02T08:03:32.663Z
Learning: In oven-sh/bun, treat files intended to run under both `bun test` and Node’s test runner (dual-runtime) as exempt from the Bun `harness` guideline that bans `fs.mkdtempSync`. Specifically: if a test file imports `node:test` (not `bun:test`) and uses `node:assert`, treat it as dual-runtime and do not flag `fs.mkdtempSync` as a guideline violation. These dual-runtime tests must avoid Bun-only imports like `harness` to preserve Node.js compatibility. Prefer the portable temp-dir base as: `fs.mkdtempSync(path.join(fs.realpathSync(os.tmpdir()), "prefix-"))`.
Learnt from: robobun
Repo: oven-sh/bun PR: 30073
File: test/js/node/fs/cp-symlink-target.test.ts:21-23
Timestamp: 2026-05-02T08:03:39.284Z
Learning: For cross-runtime portable tests in this repo (files under test/js/node that import from `node:test` and `node:assert` and are meant to run under both `bun test` and `node --experimental-strip-types --test`):
- Do not import Bun-only modules such as `harness`.
- For temporary directory creation, use the portable pattern `fs.mkdtempSync(path.join(fs.realpathSync(os.tmpdir()), "prefix-"))`.
- When reviewing these files, do not treat `fs.mkdtempSync`/`os.tmpdir()` as a guideline violation, since it’s the intended portable approach for these dual-runtime tests.
Learnt from: robobun
Repo: oven-sh/bun PR: 30120
File: test/js/node/zlib/zlib-dictionary-detach.test.ts:126-134
Timestamp: 2026-05-02T16:51:38.954Z
Learning: In oven-sh/bun TypeScript test files under test/js/node that spawn subprocesses with ASAN enabled via `bunEnv`/`bunEnv.ASAN_OPTIONS`, suppress the ASAN startup banner by adding `allow_user_segv_handler=1` directly to the subprocess environment’s `ASAN_OPTIONS` before spawning (e.g., append `allow_user_segv_handler=1` to any existing `bunEnv.ASAN_OPTIONS`). This is preferred to post-filtering or split/filter/joining stderr output because ASAN’s Options.cpp gates emission based on these options.
Learnt from: robobun
Repo: oven-sh/bun PR: 30138
File: test/js/node/buffer-write-detach.test.ts:21-32
Timestamp: 2026-05-02T23:55:54.485Z
Learning: In `test/js/node/**/*.test.ts`, don’t treat `BENIGN_STDERR` filters that suppress/whitelist the specific benign ASAN stderr line (e.g., `/^WARNING: ASAN interferes with JSC signal handlers;[^\n]*\n$/`) as a problem or as something that must be replaced with explicit `ASAN_OPTIONS` manipulation in subprocess env. In this repo’s setup, `bunEnv` from `test/harness.ts` already sets the required `ASAN_OPTIONS` under ASAN builds for subprocesses spawned with `bunEnv`, so the banner suppression is already handled at the source. Keeping/copying the `BENIGN_STDERR` regex as a cross-file defensive consistency measure is legitimate.
Learnt from: robobun
Repo: oven-sh/bun PR: 30153
File: test/bundler/plugin-sync-exception-fallback.test.ts:75-91
Timestamp: 2026-05-03T01:53:50.441Z
Learning: In this repo’s Bun test files that use `Bun.spawn`, don’t “parse/assert stdout before checking `exitCode`” when the expected failure mode is a crash (e.g., SIGSEGV or UBSan abort) that may produce empty stdout. Parsing/validating empty stdout first can mask the more useful signal/stderr. Instead, assert the spawned-process result by including `stdout` in the object passed to `toMatchObject` alongside `exitCode`, `signalCode`, and `stderr`, so stdout/stderr/signal all appear together in the failure diff (same pattern as `test/bundler/plugin-error-nested-throw.test.ts`).
Learnt from: robobun
Repo: oven-sh/bun PR: 30166
File: test/js/node/fs/fs.test.ts:1227-1229
Timestamp: 2026-05-03T02:52:14.737Z
Learning: In oven-sh/bun TypeScript test files that spawn subprocesses and explicitly override `ASAN_OPTIONS` via `bunEnv.ASAN_OPTIONS` (e.g., appending `symbolize=0`, `abort_on_error=1`, `allow_user_segv_handler=1`), do not require an assertion like `expect(stderr).toBe("")`.
Reason: `bunEnv.ASAN_OPTIONS` is only populated when the outer test runner is `bun-asan`. On `bun-debug` builds where the binary is ASAN-instrumented but `bunEnv.ASAN_OPTIONS` is absent, explicitly setting `ASAN_OPTIONS` can still cause an ASAN diagnostic line (e.g., “WARNING: ASAN interferes with JSC signal handlers”) to be written to stderr.
Instead, assert the outcome using a combined expectation on `stdout` and `exitCode` (e.g., `toEqual({ stdout: <expected>, exitCode: 0 })`) and treat stderr as non-empty only as a diagnostic; this still cleanly distinguishes an ASAN abort (exit code 134, empty/unsuitable stdout) from a passing run (exit code 0 with expected stdout).
Learnt from: robobun
Repo: oven-sh/bun PR: 30179
File: test/js/node/process/process-setgroups.test.ts:1-30
Timestamp: 2026-05-03T04:17:24.602Z
Learning: In oven-sh/bun, when reviewing new Bun-specific per-feature test files under `test/js/node/process/` (e.g., `process-setgroups.test.ts`), do not mark them as a “should merge into existing file” violation if there is no prior Bun-specific test file in that directory covering the same feature. If the only related coverage in the broader Node test tree is an upstream Node.js copy (e.g., a test that requires `../common`), treat it as the wrong home for Bun-specific or fuzzer regression coverage—creating a new feature-specific test in `test/js/node/process/` is intentional.
Learnt from: robobun
Repo: oven-sh/bun PR: 30245
File: test/regression/issue/19650.test.ts:9-30
Timestamp: 2026-05-04T20:27:55.527Z
Learning: In oven-sh/bun test files, prefer using flat `test.concurrent.each([...])` when you want every parameterized test case to run fully concurrently across the entire parameter matrix. By contrast, `describe.each([...])` executes its describe blocks sequentially; while tests inside each describe block may be `test.concurrent`, concurrency is limited to within that block rather than across the whole matrix.
Learnt from: robobun
Repo: oven-sh/bun PR: 30118
File: test/js/node/zlib/zlib-writestate-detached.test.ts:78-90
Timestamp: 2026-05-04T20:37:57.348Z
Learning: In this Bun repository, do not flag code in Bun subprocess fixtures/tests where `console.log(...)` (or similar synchronous stdout/stderr writes) is immediately followed by `process.exit(n)` as a potential output-loss problem. Bun’s `process.exit()` flushes stdout and stderr synchronously before exiting (per the implementation in `src/runtime/node/process/exit.zig`), so `console.log` + `process.exit` is considered a safe, established Bun convention.
Learnt from: robobun
Repo: oven-sh/bun PR: 30268
File: test/js/bun/net/named-pipe-listen-error.test.ts:137-137
Timestamp: 2026-05-05T02:16:13.255Z
Learning: When reviewing JavaScript/TypeScript regex literals, treat `\b` as an escaped backslash followed by `b` (i.e., it matches a literal backslash and then `b`), not the regex word-boundary metacharacter. The word-boundary metacharacter is an unescaped `\b` in the source code (i.e., `\b` in the pattern string/literal syntax), which has word-boundary semantics.
So: do not flag `\b` inside a regex as a word-boundary issue by default. Only flag `\b` when the intent is to match a literal backslash+`b` and word-boundary semantics would be incorrect. Example: `/^\\\.\\pipe\\/` (as written) matches the Windows named-pipe prefix `\\.\pipe\`.
Learnt from: robobun
Repo: oven-sh/bun PR: 30284
File: test/cli/test/path-ignore-patterns.test.ts:467-495
Timestamp: 2026-05-05T15:02:03.877Z
Learning: In oven-sh/bun test files under `test/**/*.test.ts`, when verifying that a test was NOT executed (for example, it was filtered out by `pathIgnorePatterns`), assert the absence of the test name string (e.g., `expect(stderr).not.toContain("explicit test")`) rather than asserting that the filename is absent. Bun may echo the filename in its `"The following filters did not match any test files:"` error output even when no tests ran, so filename-based assertions can be misleading.
Learnt from: robobun
Repo: oven-sh/bun PR: 30306
File: test/js/web/fetch/blob-write.test.ts:88-96
Timestamp: 2026-05-06T01:36:05.893Z
Learning: TempDir must be invoked with two arguments in test harness code: basename: string and filesOrAbsolutePathToCopyFolderFrom: DirectoryTree | string. Calls like tempDir("foo") should be flagged as invalid. tempDirWithFiles("name", {}) is a permitted pattern in existing tests (e.g., test/js/web/fetch/blob-write.test.ts line 55) when the result is assigned with const (not using) and consistent with the file's conventions. Apply this rule to test files across the repository (oven-sh/bun), and do not flag compliant const-based patterns that follow the established usage.
Learnt from: robobun
Repo: oven-sh/bun PR: 30350
File: test/cli/test/bun-test.test.ts:1319-1324
Timestamp: 2026-05-07T06:52:44.159Z
Learning: In oven-sh/bun TypeScript test files under `test/**/*.test.ts`, when the test constructs the snapshot input by intentionally `.filter()`-ing raw stderr to only the reporter-generated status/output lines (e.g., lines matching `/^\((pass|fail|skip|todo)\)/`, `^ ...` explanation lines, and `AssertionError:` lines), do not require `normalizeBunSnapshot` for that snapshot. In this design, the `.filter()` is what stabilizes the snapshot across `Execution.Result` variants; adding `normalizeBunSnapshot` would unnecessarily retain extra output (stack traces, repeated failures block, summaries), making snapshots ~3x larger and more fragile. Accept the local convention of small ad-hoc `.replace()` regex normalization for volatile timing fragments (e.g., stripping `[{d}ms]` and `after {d}ms` timeout text) where applied consistently within the same test suite.
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/js/node/process/process-nexttick-stack-overflow.test.ts (1)
28-32:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert clean subprocess completion, not just
signalCode.Line 32 only shows the child was not killed by a signal. A normal non-zero exit would still pass here, and without a
stderrassertion the test can miss diagnostics from the failure path.Suggested patch
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); expect(stdout.trim()).not.toBe("typeof=object"); expect(["typeof=function", "typeof=undefined"]).toContain(stdout.trim()); + expect(stderr).toBe(""); expect(proc.signalCode).toBeNull(); + expect(exitCode).toBe(0);Based on learnings: in
test/js/subprocess tests that usebunEnvfromharness,expect(stderr).toBe("")is intentional, and exit-code assertions should come after stdout checks.🤖 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/node/process/process-nexttick-stack-overflow.test.ts` around lines 28 - 32, The test currently only asserts proc.signalCode is null which misses non-zero exit codes and stderr output; after the stdout assertions, also assert that stderr is empty (expect(stderr).toBe("")) and that exitCode is 0 (or expect(exitCode).toBe(0)) using the already awaited exitCode variable, ensuring the subprocess completed cleanly; keep the stderr assertion before the exitCode assertion as in other test/js subprocess tests that use bunEnv/harness.
🤖 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.
Duplicate comments:
In `@test/js/node/process/process-nexttick-stack-overflow.test.ts`:
- Around line 28-32: The test currently only asserts proc.signalCode is null
which misses non-zero exit codes and stderr output; after the stdout assertions,
also assert that stderr is empty (expect(stderr).toBe("")) and that exitCode is
0 (or expect(exitCode).toBe(0)) using the already awaited exitCode variable,
ensuring the subprocess completed cleanly; keep the stderr assertion before the
exitCode assertion as in other test/js subprocess tests that use bunEnv/harness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d588b3d9-9e27-4943-8368-286c8b6950b4
📒 Files selected for processing (1)
test/js/node/process/process-nexttick-stack-overflow.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
LGTM — mirrors the existing constructStdin exception-handling pattern exactly.
Extended reasoning...
Overview
This PR adds exception handling to Process::constructNextTickFn in src/jsc/bindings/BunProcess.cpp (7 lines: a DECLARE_TOP_EXCEPTION_SCOPE plus a check-clear-report-return block after profiledCall), and adds a 33-line regression test that spawns a subprocess, exhausts the stack, triggers lazy process.nextTick initialization, and asserts the property is not reified as an internal Exception cell and the process does not crash with a signal.
Security risks
None. This is defensive exception handling in a lazy property callback. No user-controlled input flows into new code paths; the change only ensures a pending JSC exception is cleared and reported rather than leaked into the property reification machinery.
Level of scrutiny
Low. The added block is byte-for-byte the same pattern as constructStdin (BunProcess.cpp:2660–2674) — DECLARE_TOP_EXCEPTION_SCOPE, scope.exception() check after profiledCall, tryClearException(), reportUncaughtExceptionAtEventLoop, return jsUndefined(). This is a fuzzer-found crash fix following an established in-file convention, not a new design.
Other factors
- CodeRabbit's suggestion to assert
exitCode === 0was correctly rebutted by the author (the reported uncaught exception causes exit 1 by design) and retracted. - The CI failures in the build comment are all
build-zig/build-cppagent failures across every platform simultaneously (including "Failed to create agent"), which is characteristic of infra flake rather than this 7-line C++ change — a C++-only diff would not break every Zig build target. - A bot flagged potential overlap with #30285; that's a merge-coordination note, not a correctness concern for this diff.
- No CODEOWNERS cover the modified paths.
Collaborator
Author
|
CI failures in build #53072 are all pre-existing and unrelated to this change:
The new |
Collaborator
Author
|
Closing in favor of #30612, which covers the same crash more broadly:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fuzzilli found a crash at
JSObjectInlines.h:102:Repro:
What happens
process.nextTickis aPropertyCallbackin the static hash table. Its callback,constructNextTickFn, runs JS viaprofiledCallto set up the tick queue. When the stack is nearly exhausted this throwsRangeError: Maximum call stack size exceeded, andexecuteCallreturns theException*as aJSValue.The callback then returned that value with the exception still pending.
reifyStaticPropertystored it withputDirect,getPropertySlotreportedhasProperty=true, andJSObject::getasserted because an exception was set for a found property. In release builds the assertion is compiled out, butprocess.nextTickends up reified as an internalExceptioncell (typeof process.nextTick === "object"), which later trips a different assertion inJSCell::toStringSlowCasewhen JS tries to call it.new Worker(...)reaches this path viaprocess->emitOnNextTick→queueNextTick→process->get("nextTick").Fix
Match the existing pattern in
constructStdin/constructStdout/constructStderr: catch the exception fromprofiledCall, report it viareportUncaughtExceptionAtEventLoop, and returnjsUndefined()so the property reifies to a sane value and no exception leaks out of theLazyPropertyCallback.