Conversation
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughInternal async_hooks tracking is extended to separately count hooks that supply an ChangesInit-hook tracking and AsyncResource validation
Related PRs: None identified. Suggested labels: Suggested reviewers: None identified. Poem 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 4:14 PM PT - Jul 6th, 2026
❌ @robobun, your commit b9060c3 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33435That installs a local version of the PR into your bun-33435 --bun |
|
Status: ready for review. The red CI lanes are unrelated to this diff. Reproduced with the script in the description: stock bun prints Verification, against both a debug (ASAN) and a release build of this branch:
The diff is three files and contains no Rust or C++: CI.
Everything else red on this branch was retried green and is listed under CI's own For reference, recent |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
LGTM — clean Node compat fix replacing a one-way latch with a properly balanced init-hook counter.
Extended reasoning...
Overview
This PR fixes new AsyncResource("") to only throw ERR_ASYNC_TYPE while a hook with an init callback is currently enabled, matching Node's async_hook_fields[kInit] > 0 gate. It replaces a module-level hasEnabledCreateHook latch (set once, never cleared, triggered by any hook regardless of callbacks) with an activeInitHooks counter in internal/async_hooks, maintained via the existing markHookEnabled/markHookDisabled calls which now take a hasInit boolean. Three files touched: ~15 lines of source change plus one new subprocess test.
Security risks
None. This only affects when an argument-validation error is thrown in a stubbed compat API; no auth, crypto, filesystem, or network paths involved.
Level of scrutiny
Low. This is a narrow behavioral correction in a Node compat shim (createHook is already largely a no-op stub in Bun). The counter is guarded against underflow (activeInitHooks > 0 before decrement), and the per-hook kHookEnabled symbol already ensures enable/disable are idempotent, so double-enable and double-disable cannot desync the count. The type.length === 0 && ... short-circuit means the require() only fires on the empty-type edge case, so the common path is unchanged. Grep confirms no other callers of markHookEnabled/markHookDisabled exist, so the signature change is safe, and hasEnabledCreateHook has no remaining references.
Other factors
The new test is thorough — 12 named scenarios in a child process (hook counts are process-global) covering empty hooks, non-init hooks, init hooks, disable/re-enable, double-enable, double-disable, and disable-without-enable, with expected values taken from Node v26.3.0. The PR description notes test-async-hooks-asyncresource-constructor.js and the test-stream-finished-* consumers of enabledHooksExist() still pass. No prior human reviews or outstanding comments on the timeline.
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/node/async_hooks/async_hooks.node.test.ts`:
- Line 95: The AsyncResource subprocess test should be made concurrent to match
the project guideline for isolated process-spawning tests. Update the test
definition for the `new AsyncResource('') only throws while an init hook is
enabled` case to use `test.concurrent` so it can run alongside sibling tests
without shared state issues. Keep the existing test logic unchanged and ensure
the surrounding `async_hooks.node.test` cases remain compatible with concurrent
execution.
- Around line 152-158: The async_hooks test is discarding diagnostic output from
the spawned fixture, so failures can hide the real error. Update the
`Bun.spawn`/`proc.stdout.text()`/`proc.stderr.text()` flow to retain `stderr`
instead of ignoring it, and use that captured output in the assertion or failure
message when `stdout` is empty or JSON parsing fails. Keep the existing
`proc.exited` handling, but make the test surface the `stderr` contents from the
spawned process for better failure diagnostics.
🪄 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: 26795349-6248-4303-ac67-787159dd0b68
📒 Files selected for processing (3)
src/js/internal/async_hooks.tssrc/js/node/async_hooks.tstest/js/node/async_hooks/async_hooks.node.test.ts
8e0737f to
504bf99
Compare
There was a problem hiding this comment.
LGTM — small, well-tested Node compat fix that replaces a permanent latch with a properly ref-counted initHooksExist() check.
Extended reasoning...
Overview
This PR fixes new AsyncResource("") to only throw ERR_ASYNC_TYPE while a hook with an init callback is currently enabled, matching Node.js's initHooksExist() semantics. Previously a module-level boolean latch (hasEnabledCreateHook) was set permanently on the first createHook().enable() regardless of which callbacks were supplied, and never cleared on disable(). The fix replaces the latch with an activeInitHooks counter in internal/async_hooks maintained alongside the existing activeHooks counter, and gates the type check on initHooksExist().
Three files touched: src/js/internal/async_hooks.ts (adds counter + initHooksExist()), src/js/node/async_hooks.ts (removes latch, passes init !== undefined to mark helpers, consults initHooksExist()), and a comprehensive subprocess test in test/js/node/async_hooks/async_hooks.node.test.ts.
Security risks
None. Pure JS in the async_hooks stub layer; no auth, crypto, filesystem, or network paths involved. The change narrows when an error is thrown to match Node — it does not disable any validation that Node performs.
Level of scrutiny
Low-to-moderate. This is a ~15-line source change in a Node compat shim (Bun's createHook is largely a stub). The counter logic is straightforward and mirrors the existing activeHooks pattern one line above. The kHookEnabled per-instance guard (pre-existing) already prevents double-counting on repeated enable/disable, and the new counter is additionally guarded against underflow. I verified there are no other callers of markHookEnabled/markHookDisabled that would break from the signature change.
Other factors
- The test is thorough: 12 labeled cases covering no-hook, empty-hook, non-init-hook, init-hook enabled/disabled, multiple init hooks, double-enable, double-disable, and disable-without-enable — all verified against Node v26.3.0 output. It correctly runs in a subprocess since hook counts are process-global.
- The condition was reordered to
type.length === 0 && initHooksExist()so therequire()only fires on the rare empty-type path, consistent with the built-in JS lazy-require guideline. - The one CodeRabbit stderr-diagnostics nit was addressed; the remaining
test.concurrentsuggestion is a trivial style preference and not blocking. - PR description confirms
test-async-hooks-asyncresource-constructor.jsand the rest oftest/js/node/async_hooks/still pass.
…init hooks
hasEnabledCreateHook was a write-once latch. Any createHook(...).enable() set
it, including a hook with no callbacks at all, and disable() never cleared it,
so new AsyncResource("") threw ERR_ASYNC_TYPE for the rest of the process.
Node gates the check on initHooksExist(), the count of currently-enabled hooks
that supply an init callback. Track that count in internal/async_hooks next to
the existing enabled-hook count.
504bf99 to
c66541d
Compare
There was a problem hiding this comment.
LGTM — small, well-tested Node compat fix.
Extended reasoning...
Overview
This PR fixes a Node.js compatibility bug in node:async_hooks: new AsyncResource("") was throwing ERR_ASYNC_TYPE for the rest of the process after any hook was enabled, whereas Node only throws while a hook with an init callback is currently enabled. The fix replaces a module-level boolean latch with an activeInitHooks counter in src/js/internal/async_hooks.ts (mirroring Node's async_hook_fields[kInit]), and AsyncResource now consults initHooksExist(). Three files touched: the internal counter module, the node:async_hooks implementation, and a new subprocess test.
Security risks
None. This is a validation gate on the type argument to AsyncResource. The change makes the check less strict (matching Node), so there's no new rejection surface, and no auth/crypto/permissions/untrusted-input handling involved.
Level of scrutiny
Low-to-moderate. The change is small (~20 lines of runtime code), self-contained, and strictly additive to the internal module's export surface. The only two callers of markHookEnabled/markHookDisabled are both updated. The existing kHookEnabled per-hook guard already ensures the counter can't double-count on repeated enable()/disable(), and the underflow guard (activeInitHooks > 0) is defensive. The condition was reordered to check type.length === 0 before the require() call, which is a harmless micro-optimization.
Other factors
- The new test is thorough: 12 ordered cases in an isolated subprocess (hook counts are process-global), each verified against Node v26.3.0 output, including double-enable, double-disable, and disable-without-enable to pin the counter against underflow.
- The test follows repo conventions:
bunEnv/bunExe, concurrent pipe drain viaPromise.all, exit code asserted last, stderr surfaced in the diff only on crash (avoiding the "never assert stderr is empty" pitfall). - Both CodeRabbit comments are resolved — one addressed with a better fix than suggested, one correctly declined with reasoning that CodeRabbit accepted.
- The PR description confirms the vendored
test-async-hooks-asyncresource-constructor.js(which registers aninithook to trigger validation) still passes, as do thetest-stream-finished-*tests that readenabledHooksExist()from the same module. - No outstanding human reviewer comments.
There was a problem hiding this comment.
LGTM — small, well-tested Node compat fix that replaces a one-way latch with a proper init-hook counter.
Extended reasoning...
Overview
This PR fixes new AsyncResource('') to throw ERR_ASYNC_TYPE only while a hook with an init callback is currently enabled, matching Node's initHooksExist() gate. It touches three files: src/js/internal/async_hooks.ts (adds activeInitHooks counter + initHooksExist(), extends markHookEnabled/markHookDisabled to accept a hasInit flag), src/js/node/async_hooks.ts (replaces the module-level hasEnabledCreateHook latch with initHooksExist(), passes init !== undefined to the mark functions), and adds a 12-case subprocess test verified against Node v26.3.0.
Security risks
None. This is pure JS-level Node-compat logic that gates when an argument-validation error is thrown. The change is strictly more permissive (throws in fewer cases) and matches upstream Node semantics exactly.
Level of scrutiny
Low-to-medium. The diff is ~20 source lines following the existing activeHooks counter pattern in the same file, with underflow guards. I verified markHookEnabled/markHookDisabled have no other callers, and enabledHooksExist() (used by vendored test-stream-finished-* tests) is unchanged. Double-enable/double-disable cannot desync the counter because the existing per-hook kHookEnabled flag already guards those transitions — and the test explicitly pins all four counter edge cases (double-enable, double-disable, disable-without-enable, one-of-two-remaining).
Other factors
The bug hunting system found no issues. Both CodeRabbit inline comments are resolved (one addressed with a sensible stderr-in-diff pattern, one correctly declined). The test follows all harness conventions: subprocess isolation for process-global state, bunEnv/bunExe, concurrent pipe draining via Promise.all, exitCode asserted last, await using for the proc. No CODEOWNERS entries cover these paths. The type.length === 0 check is ordered before the require() call, keeping the lazy import inside the rare branch per the built-in-module perf guideline.
Repro
Once any
createHook(...).enable()has run,new AsyncResource("")throwsERR_ASYNC_TYPEfor the rest of the process, even when the hook had no callbacks and even after every hook is disabled again. Loading a dependency that enables a no-op hook (APM agents,async-listenershims, some test frameworks) permanently changes whatnew AsyncResource("")does, and the failure shows up far from its cause.Cause
src/js/node/async_hooks.tsgated the check on a module-level latch:It was set by every
enable()regardless of which callbacks the hook supplied, anddisable()never cleared it.Node gates the same check on
initHooksExist(), i.e.async_hook_fields[kInit] > 0: the number of currently enabled hooks that supply aninitcallback.enable()anddisable()increment and decrement it, and re-enabling a hook that is already active does not count it twice.Fix
Replace the latch with an
activeInitHookscounter ininternal/async_hooks, maintained alongside the existingactiveHookscount and only moved when the hook supplies aninitcallback.AsyncResourcenow consultsinitHooksExist().Verification
Added to
test/js/node/async_hooks/async_hooks.node.test.ts. Hook counts are process-global, so the whole lifecycle runs in one child process. Expected values are what node v26.3.0 prints for the same script (ERR=ERR_ASYNC_TYPE):The last four cases pin the counter against double-enable, double-disable and disable-without-enable, so it cannot underflow past a hook that is still enabled.
test/js/node/test/parallel/test-async-hooks-asyncresource-constructor.js(which registers aninithook precisely so the type gets validated) still passes, as do thetest-stream-finished-*tests that readenabledHooksExist()from the same module, and the rest oftest/js/node/async_hooks/(112 pass, 0 fail).