process: keep the onEachMicrotaskTick nextTick hook armed across preloads

[robobun]

Problem

A --preload script (or anything that runs before the entry and touches process.nextTick) inverts the relative order of process.nextTick vs microtasks scheduled at the top level of the entry module. Since #31216 every node:worker_threads worker implicitly preloads node:worker_threads, which pulls in node:stream, whose internal/streams/destroy reads process.nextTick at top level, so a CJS worker entry always hits this.

process.nextTick(() => console.log("nextTick"));
queueMicrotask(() => console.log("microtask"));

entry	bun before	bun after
no preload	nextTick, microtask	nextTick, microtask
--preload that touches process.nextTick	microtask, nextTick	nextTick, microtask
worker_threads CJS entry	microtask, nextTick	nextTick, microtask

The #34115 symptom (Writable.toWeb(w).close() resolving instead of rejecting with ABORT_ERR when a preload required node:stream) is one instance.

Cause

checkIfNextTickWasCalledDuringMicrotask is installed as vm.setOnEachMicrotaskTick at VM init as a one-shot: on the first microtask where m_nextTickQueue exists it calls resetOnEachMicrotaskTick(), which nulls the hook. A preload that reads process.nextTick (the property is lazy and creates m_nextTickQueue on first access) consumes the one-shot during preload's microtask checkpoint, so the entry module runs with no hook and its top-level nextTick drains only at the next task boundary.

Fix

Keep the hook armed instead of nulling it on first fire. A re-entrancy guard (m_isDrainingNextTickQueue) in the hook and in jsFunctionDrainMicrotaskQueue (the only caller of vm.drainMicrotasks() from inside processTicksAndRejections) prevents recursion; GlobalObject::drainMicrotasks stays unguarded so drain() can still re-enter when a tick callback spins wait_for_promise (zlib/HTMLRewriter/expect().resolves), and its call is gated on !isEmpty() to match the hook. processTicksAndRejections now clears internalField(0) after its do...while so isEmpty() reflects the drained state and the hook short-circuits.

With the hook persistently armed, the mustResetContext branch in JSNextTickQueue::drain (which cleared asyncContextData[0]) became unreachable and is removed: any nextTick queued during its vm.drainMicrotasks() is drained by a nested hook firing. cleanupAsyncHooksData / resetOnEachMicrotaskTick reduce to re-arming the hook (the only other user is AsyncLocalStorage.enterWith), and the asyncHooksNeedsCleanup field they branched on is now write-only and removed.

The per-microtask cost is one bool check plus isEmpty() (two field loads) and only applies once m_nextTickQueue exists, which is the same predicate the old bootstrap lambda already paid on every microtask before the one-shot fired.

This change also makes process.nextTick queued from inside a microtask run between that microtask and its siblings consistently, regardless of whether the queue was accessed earlier. Before, the ordering depended on whether the one-shot had already fired. Node runs it after the whole microtask FIFO; aligning that is a separate concern (#33088 / #33366) and would require moving the drain point, not just keeping the hook armed.

Verification

test/regression/issue/34115.test.ts: four preload variants (reads/calls process.nextTick, requires node:stream/node:zlib), two-preload case, worker_threads CJS entry via file and eval: true, the original Writable.toWeb() repro, plus a wait_for_promise-inside-nextTick case for the ALS frame. 6/9 fail on released bun, all 9 pass with this change.

Fixes #34115

---

[review] gate passed · iteration 18 · 7 files touched

fails on main (without fix)

ASAN without fix: 8 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/regression/issue/34115.test.ts"
bun test v1.4.0 (1ef0083d3)

test/regression/issue/34115.test.ts:
33 |       stdout: "pipe",
34 |       stderr: "pipe",
35 |     });
36 |     const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
37 |     expect(stderr).toBe("");
38 |     expect(stdout).toBe(preloadOutput + "nextTick\nmicrotask\npromise\n");
                        ^
error: expect(received).toBe(expected)

- "nextTick
- microtask
+ "microtask
  promise
+ nextTick
  "

- Expected  - 2
+ Received  + 2

      at <anonymous> (/workspace/bun/test/regression/issue/34115.test.ts:38:20)
(fail) process.nextTick ordering is preserved with --preload > preload that reads process.nextTick [472.60ms]
33 |       stdout: "pipe",
34 |       stderr: "pipe",
35 |     });
36 |     const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
37 |     expect(stderr).toBe("");
38 |     expect(stdout).toBe(preloadOutput + "nextTick\nmicrotask\npromise\n"
... (truncated)

release without fix: all passed
bun test v1.4.0-canary.1 (cc50790ce)

test/regression/issue/34115.test.ts:
(pass) process.nextTick ordering is preserved with --preload > preload that reads process.nextTick [13.93ms]
(pass) process.nextTick ordering is preserved with --preload > preload that calls process.nextTick [9.98ms]
(pass) process.nextTick ordering is preserved with --preload > preserved with two preload scripts [12.02ms]
(pass) process.nextTick ordering is preserved with --preload > preload that requires node:stream [17.29ms]
(pass) process.nextTick ordering is preserved with --preload > preload that requires node:zlib [17.57ms]
(pass) AsyncLocalStorage frame survives a nextTick callback that spins wait_for_promise [14.09ms]
(pass) Writable.toWeb() close rejects with ABORT_ERR when preload requires node:stream [17.73ms]
(pass) process.nextTick ordering at the top level of a worker_threads CJS entry > matches the main thread (file entry) [49.18ms]
(pass) process.nextTick ordering at the top level of a worker_threads CJS entry > matches the main thread (eval: true) [49.94ms]

 9 pass
 0 fail
 27 expect() calls
Ran 9 tests across 1 file. [231.00ms]
__F:0:S:0

passes on PR (with fix)

ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/regression/issue/34115.test.ts"
bun test v1.4.0 (1ef0083d3)

test/regression/issue/34115.test.ts:
(pass) process.nextTick ordering is preserved with --preload > preload that reads process.nextTick [467.25ms]
(pass) process.nextTick ordering is preserved with --preload > preload that calls process.nextTick [442.17ms]
(pass) process.nextTick ordering is preserved with --preload > preserved with two preload scripts [459.11ms]
(pass) process.nextTick ordering is preserved with --preload > preload that requires node:stream [788.12ms]
(pass) process.nextTick ordering is preserved with --preload > preload that requires node:zlib [922.76ms]
(pass) Writable.toWeb() close rejects with ABORT_ERR when preload requires node:stream [979.77ms]
(pass) AsyncLocalStorage frame survives a nextTick callback that spins wait_for_promise [840.47ms]
(pass) process.nextTick ordering at the top level of a worker_threads CJS entry > matches the main thread (file entry) [2557.64ms]
(pass) process.nextTick ordering at the top level of a worker_threads CJS en
... (truncated)

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 719ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/123] gen cpp.rs (cppbind)
[2/123] gen BunProcess.lut.h
Generating /workspace/bun/build/release/codegen/BunProcess.lut.h from /workspace/bun/src/jsc/bindings/BunProcess.cpp
[3/123] gen generated_host_exports.rs
generated_host_exports.rs: 94 exports (host=3, lazy=10, generic=81, rust=0); 239 extern-C blocks audited
[4/123] gen JS modules (bundle-modules)
Preprocess modules (9402ms)
Bundle modules (69ms)
Postprocesss modules (258ms)
Bundle Functions (811ms)
Generate Code (35ms)

[10.60s] Bundled "src/js" for production
  2570 kb
  193 internal modules
  13 native modules
  90 internal functions across 19 files
[4/122] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu)

  nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19)

�[1m�[92m   Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core)
�[1m�[92m   Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno)
�[1m�[92m   Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr)
�[1m�[92m   Com
... (truncated)

diff hotspot

src/js/builtins/ProcessObjectInternals.ts |   1 +
 src/jsc/bindings/BunProcess.cpp           |   2 +
 src/jsc/bindings/JSNextTickQueue.cpp      |   6 --
 src/jsc/bindings/NodeAsyncHooks.cpp       |   4 +-
 src/jsc/bindings/ZigGlobalObject.cpp      |  45 +++------
 src/jsc/bindings/ZigGlobalObject.h        |   2 +-
 test/regression/issue/34115.test.ts       | 162 ++++++++++++++++++++++++++++++
 7 files changed, 181 insertions(+), 41 deletions(-)

gate history · 16 passed · 3 rejected · iteration 18

evidence per changed file

file                                       reads  edits  tests
src/js/builtins/ProcessObjectInternals.ts      2      1      0
src/jsc/bindings/BunProcess.cpp                6      2      0
src/jsc/bindings/JSNextTickQueue.cpp          10     14      0
src/jsc/bindings/NodeAsyncHooks.cpp            3      1      0
src/jsc/bindings/ZigGlobalObject.cpp           9     19      0
src/jsc/bindings/ZigGlobalObject.h             4      3      0
test/regression/issue/34115.test.ts            8     10      0

[robobun]

^{Updated 1:45 PM PT - Jul 29th, 2026}

✅ @robobun, your commit 1ef0083d32cd0c8f5eccf90a87e5c5c1f5d0df4c passed in Build #85387! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 34121

That installs a local version of the PR into your bun-34121 executable, so you can run:

bun-34121 --bun

[coderabbitai]

Walkthrough

Changes

The change adds guarded nextTick draining, centralizes microtask hook handling, clears queue bookkeeping after microtask drains, and adds regression coverage for preload, worker, stream, and async-hook scenarios.

NextTick microtask integration

Layer / File(s)	Summary
Guard nextTick queue draining src/jsc/bindings/ZigGlobalObject.h, src/jsc/bindings/BunProcess.cpp, src/jsc/bindings/ZigGlobalObject.cpp, src/js/builtins/ProcessObjectInternals.ts	Adds draining-state tracking, prevents recursive queue drains during microtask processing, and clears nextTick queue bookkeeping after microtasks drain.
Refine microtask hook dispatch src/jsc/bindings/ZigGlobalObject.cpp, src/jsc/bindings/NodeAsyncHooks.cpp	Routes hook setup and async cleanup through the shared nextTick checker and reset method.
Regression coverage test/regression/issue/34115.test.ts	Validates ordering across preload and worker cases, stream abort behavior, and AsyncLocalStorage state across nested microtask drains.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly states the main change: keeping the onEachMicrotaskTick nextTick hook armed across preloads.
Description check	✅ Passed	The description covers the problem, cause, fix, and verification, matching the template content even though the headings differ.

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

I didn't find bugs, but this changes core microtask/nextTick scheduling semantics and warrants a human look — the hook is now permanent (per-microtask cost + an acknowledged ordering change for nextTick-inside-microtask), and it directly conflicts with the design in #33088.

Checked: the SetForScope re-entrancy guard covers the drainMicrotasks() call inside processTicksAndRejections; isEmpty() now reflects reality after drain via the new $putInternalField(nextTickQueue, 0, 0); cleanupAsyncHooksData / resetOnEachMicrotaskTick simplifications preserve the async_hooks cleanup path. The test's expect(stderr).toBe("") was flagged and ruled out — subprocess uses bunEnv so debug noise is suppressed.

Extended reasoning...

Overview

The PR keeps the onEachMicrotaskTick hook armed permanently instead of one-shot, so --preload scripts that touch process.nextTick no longer consume the hook before the entry module runs. Touches ZigGlobalObject.cpp/.h (hook lifecycle, new m_isDrainingNextTickQueue flag), JSNextTickQueue.cpp (re-entrancy guard via SetForScope), ProcessObjectInternals.ts (reset internal field 0 after drain), plus a regression test.

Security risks

None identified. This is scheduling/ordering logic; no untrusted-input parsing, auth, or crypto surface.

Level of scrutiny

High. onEachMicrotaskTick fires between every microtask for the life of the process once m_nextTickQueue exists — this is one of the hottest paths in the runtime. The change also carries an intentional user-visible ordering change outside the preload case (nextTick scheduled from inside a microtask now always preempts sibling microtasks, where previously it depended on whether the one-shot had fired). The author explicitly calls out that #33088 takes a different design for the same hook and the two conflict; a maintainer should decide which approach (or a merged one) lands.

Other factors

The fix itself reads correct: the re-entrancy guard is set before processTicksAndRejections is called and SetForScope restores it on all exits including exception paths; the hook's fast-path check (m_isDrainingNextTickQueue || !queue || isEmpty()) is cheap. Test coverage is good (five preload shapes + the original Writable.toWeb repro, verified failing on main). But the combination of hot-path perf implications, acknowledged behavior change, and open design conflict with #33088 makes this a maintainer call rather than a mechanical fix.

[robobun]

Agreed on all three points; they're called out in the PR body. The per-microtask cost is the guard bool + isEmpty() (two field loads) and only applies once m_nextTickQueue exists, which is the same predicate the old bootstrap lambda already paid on every microtask before the one-shot fired. The nextTick-inside-microtask ordering was already non-Node-compliant and access-order-dependent; this makes it access-order-independent. Happy to rebase on top of #33088's module-boundary detection if that lands first; the change needed there is replacing its nextTickQueueHandoffDone one-shot with this re-entrancy guard so preloads don't consume the handoff.

[claude]

I didn't find any bugs, but this reworks core microtask/nextTick interleaving in ZigGlobalObject.cpp and carries an acknowledged behavior change plus a design overlap with #33088, so it warrants a maintainer's sign-off.

Checked: the SetForScope guard covers both re-entry paths (the hook itself and jsFunctionDrainMicrotaskQueue), and drain() remains re-enterable from GlobalObject::drainMicrotasks for the wait_for_promise case noted in the comment. Verified isEmpty() reads internalField(0), which the new reset in processTicksAndRejections now keeps accurate. The simplified cleanupAsyncHooksData/resetOnEachMicrotaskTick still route to cleanupAsyncHooksData when asyncHooksNeedsCleanup is set. expect(stderr).toBe("") in the test was flagged and ruled out — bunEnv sets BUN_DEBUG_QUIET_LOGS.

Extended reasoning...

Overview

This PR changes the onEachMicrotaskTick hook that interleaves process.nextTick with microtasks from a one-shot design (self-nulls after first fire) to a persistent one guarded by a re-entrancy flag. Touched: ZigGlobalObject.cpp (checkIfNextTickWasCalledDuringMicrotask, cleanupAsyncHooksData, resetOnEachMicrotaskTick, the bootstrap lambda in Zig__GlobalObject__create), BunProcess.cpp (jsFunctionDrainMicrotaskQueue gains the same guard), ZigGlobalObject.h (new m_isDrainingNextTickQueue bool), ProcessObjectInternals.ts (processTicksAndRejections resets internal field 0 so isEmpty() is accurate post-drain), plus a regression test.

Security risks

None. This is event-loop scheduling; no untrusted input parsing, auth, crypto, or permission surface is involved.

Level of scrutiny

High. onEachMicrotaskTick fires between every microtask for the process lifetime once m_nextTickQueue exists, and the ordering it governs is observable by essentially all user code. The PR itself calls out (a) a per-microtask cost that is now permanent rather than one-shot, (b) a behavior change for process.nextTick scheduled inside a microtask that makes Bun consistently non-Node-compliant in that case rather than access-order-dependent, and (c) a direct design conflict with open PR #33088 that reworks the same hook differently. Those are exactly the kind of trade-offs a maintainer should sign off on rather than a bot.

Other factors

The mechanics look correct to me: SetForScope on m_isDrainingNextTickQueue brackets both the hook's own drain() call and the drainMicrotasks() inside processTicksAndRejections, while JSNextTickQueue::drain itself is left un-guarded so the wait_for_promise re-entry path from GlobalObject::drainMicrotasks still works. The internalField(0) reset makes isEmpty() accurate so the persistent hook short-circuits cheaply when the queue is drained. The async_hooks cleanup path still installs cleanupAsyncHooksData when asyncHooksNeedsCleanup is set, and that in turn re-installs the nextTick hook. Tests cover the reported preload variants plus the original Writable.toWeb repro, and the gate evidence shows fail-on-main / pass-on-PR under both ASAN and release. Deferring purely because this is core event-loop machinery with an explicit design decision to make vs #33088.

[claude]

I didn't find any bugs, but this changes hot-path microtask/nextTick scheduling semantics for every program (the hook is now permanent, and nextTick-from-microtask ordering changes as noted), and it conflicts with #33088's competing design — worth a maintainer sign-off on which approach to take.

Checked: re-entrancy guard placement (hook + jsFunctionDrainMicrotaskQueue, not drain() itself) is consistent with the wait_for_promise path noted in the PR body; isEmpty() now correctly reflects drained state via the new internalField(0) reset; cleanupAsyncHooksData/resetOnEachMicrotaskTick no longer null the hook and their only external caller (NodeAsyncHooks.cpp) is unaffected.

Extended reasoning...

Overview

The PR changes onEachMicrotaskTick from a one-shot hook (nulled on first fire after m_nextTickQueue exists) to a permanent hook, so --preload scripts that touch process.nextTick no longer consume it before the entry module runs. It adds m_isDrainingNextTickQueue on ZigGlobalObject with SetForScope guards at two entry points (the hook itself and jsFunctionDrainMicrotaskQueue), resets internalField(0) at the end of processTicksAndRejections so isEmpty() is accurate post-drain, simplifies cleanupAsyncHooksData/resetOnEachMicrotaskTick, and replaces the bootstrap lambda with the named function. New regression tests cover several preload shapes plus the original Writable.toWeb() repro.

Security risks

None identified — no auth, crypto, permissions, or untrusted-input parsing surface is touched.

Level of scrutiny

High. This is core event-loop scheduling that runs on every microtask for the lifetime of the process once m_nextTickQueue exists. The re-entrancy guard placement is subtle (guarding drain() itself deadlocked zlib.test.js and the proxy-stress suites, per the PR body), and the PR explicitly acknowledges a semantic change outside the preload case: process.nextTick scheduled from inside a microtask now always preempts that microtask's siblings, whereas before it depended on access order. That is a runtime-wide behavior change even if no existing test observes it.

Other factors

The PR openly conflicts with #33088, which takes a different design (one-shot with an explicit handoff bit) for a related ordering issue. The author notes whichever lands second must absorb the other. That's a design decision — persistent hook + re-entrancy guard vs. one-shot + module-boundary detection — that a maintainer should weigh in on rather than an auto-approver. The test coverage and verification (nextTick, async_hooks, zlib, streams, proxy-stress, timers, events suites) look thorough, and the bug hunting system found nothing; my concern is scope and design coordination, not correctness of the code as written.

[robobun]

CI state: the fix itself is green. Build 72754 had 283/286 passing with one Windows GC-timing flake; the re-roll (72831) has 278/286 with the same flake plus two more GC-timing/process-reaping tests on specific Windows and darwin lanes. None touch the nextTick/microtask hook this PR changes:

• test-http-client-leaky-with-double-response.js (Windows 2019 x64 only, 1 of 3 Windows lanes): a FinalizationRegistry + setInterval(global.gc) GC-timing test. Passes 10/10 on Linux with this branch.
• napi.test.ts napi_wrap > has the right lifetime (Windows x64/x64-baseline): gcUntil loop that gave up after 100 GC attempts. Was [flaky] in 72754; known flaky on Windows.
• no-orphans.test.ts fast-exit intermediate (darwin 14 x64 only): perl subprocess-reaping test. Passes in 286ms on Linux.
• binary-size: informational annotation; this branch is actually ~900KB smaller on darwin.

The zlib and proxy-stress hangs from build 72730 (caused by my initial guard placement inside drain()) are fixed in both 72754 and 72831. Ready for review.

[coderabbitai]

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/regression/issue/34115.test.ts`:
- Around line 2-8: Remove the explanatory multi-line comments above the
regression test, leaving only the existing issue-reference URL comment on line
1. Do not alter the test logic or add replacement commentary.

🪄 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: b6bc6c3f-961b-4769-9379-b2555ab40421

📥 Commits

Reviewing files that changed from the base of the PR and between e532ad9 and a8418e6.

📒 Files selected for processing (5)

• src/js/builtins/ProcessObjectInternals.ts
• src/jsc/bindings/BunProcess.cpp
• src/jsc/bindings/ZigGlobalObject.cpp
• src/jsc/bindings/ZigGlobalObject.h
• test/regression/issue/34115.test.ts

[claude]

All three prior findings are addressed (the mustResetContext/ALS clear now guards on m_isDrainingNextTickQueue, tests are test.concurrent, and resetOnEachMicrotaskTick is reduced to the single live path). No new issues found this pass, but I'm deferring to a human on the design: this makes the per-microtask nextTick hook permanent, changes ordering for nextTick-inside-microtask outside the preload case (acknowledged in the description), and directly conflicts with #33088's approach to the same hook.

What was reviewed: re-checked the SetForScope guard placement across the three entry points (checkIfNextTickWasCalledDuringMicrotask, jsFunctionDrainMicrotaskQueue, GlobalObject::drainMicrotasks) — the unguarded drainMicrotasks path is intentional per the wait_for_promise re-entrancy note; verified cleanupAsyncHooksData still restores checkIfNextTickWasCalledDuringMicrotask so async_hooks doesn't permanently displace the hook; confirmed the new internalField(0) reset can no longer reach the ALS clear while an outer drain owns the context.

Extended reasoning...

Overview

Changes the onEachMicrotaskTick hook from one-shot to permanent so that --preload scripts (including the implicit node:worker_threads preload) touching process.nextTick don't consume the hook before the entry module runs. Touches ZigGlobalObject.{cpp,h} (hook + re-entrancy flag + resetOnEachMicrotaskTick/cleanupAsyncHooksData simplification), BunProcess.cpp (jsFunctionDrainMicrotaskQueue guard), JSNextTickQueue.cpp (skip mustResetContext clear under an outer drain), ProcessObjectInternals.ts (reset internalField(0) after processTicksAndRejections), plus a new regression test.

Security risks

None identified. This is scheduling/ordering logic; no untrusted input parsing, no auth/crypto, no new allocations or lifetime changes.

Level of scrutiny

High. This is core event-loop machinery that runs on every microtask once process.nextTick has been touched, and the re-entrancy surface is subtle: the author already hit one deadlock (guard inside drain() broke wait_for_promise spins, build 72730), and my earlier pass found a second latent issue (the internalField(0) reset re-activating a previously-dead mustResetContext branch that could drop an ALS frame mid-callback). Both are fixed, but the number of interacting states (hook ↔ m_isDrainingNextTickQueue ↔ processTicksAndRejections ↔ wait_for_promise ↔ async_hooks cleanup) warrants a maintainer's eyes.

Other factors

• The PR explicitly changes behaviour outside the bug being fixed: nextTick queued from inside a microtask now always preempts sibling microtasks, whereas before it depended on whether the one-shot had fired. The description says this makes an already-non-Node-compliant behaviour access-order-independent, and that no existing test is affected — but it's a semantic change a human should sign off on.
• ESM worker entries flip from accidentally-Node-matching to matching Bun's main-thread ESM ordering (which diverges from Node). Also acknowledged as pre-existing divergence, but worth a human confirming that trade-off.
• Direct conflict with open #33088, which reworks the same hook with a different design (one-shot + nextTickQueueHandoffDone). Whichever lands second needs to absorb the other; a maintainer should decide sequencing.
• Test coverage is solid (preload variants, worker file/eval entries, the original Writable.toWeb repro) and the mechgate evidence shows all cases fail on main. CI history shows the substantive failures were fixed; remaining flakes are unrelated GC-timing tests.

[robobun]

Build 84545 (ae40523, after addressing all review feedback): 190/196 passed, 0 test failures. The 6 Expired are darwin test lanes that never ran (CI agent availability). Every item in the failure annotations is [flaky] and passed on retry or alone. Ready for review.

[robobun]

The diff at 571184d builds and passes 21/21 locally. The internal gate check is showing BUILD FAILED (no junit output) for both the with-fix and without-fix ASAN legs (i.e., the gate environment can't build origin/main either), and its release-without-fix leg is reusing a cached binary from f0db312 instead of rebuilding with src/ stashed. That's gate infrastructure, not this diff; I've re-pushed once and it persisted.

The known 5 architectural regressions from the c2b8eb1 sync-CJS rewrite (http2 nextTick-during-constructor, macro-import deadlock, ShadowRealm wrong-global needing a WebKit PR, heapUsed=0, type-export error text) remain the design-level blockers documented earlier.

[robobun]

Reverted to the persistent-hook approach (was at ae40523 / build 84545: 0 test failures). The sync-CJS-load rewrite in c2b8eb1 caused 11 failing test files across five architectural regressions (http2 onConnect-during-constructor, ShadowRealm wrong-realm requiring a WebKit fix, heapUsed == 0, type-export error text, broadcast-channel worker crash) that did not converge over ~8 iterations; that approach is tracked separately with the WebKit piece in oven-sh/WebKit#371 + #36321.

Rebased onto current main (5de9985). 8/8 regression tests pass; all 11 previously-failing tests pass locally. PR title and body updated to match.

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 src/jsc/bindings/JSNextTickQueue.cpp:94-98 — Nit: 856ede9 gated the first drain() at ZigGlobalObject.cpp:3221 on !nextTickQueue->isEmpty() (matching :3243), so drain() now only enters with isEmpty()==true from the sole ungated caller Process__dispatchOnBeforeExit (BunProcess.cpp:841) — which runs at event-loop idle, never under processTicksAndRejections. That makes !zigGlobal->m_processTicksAndRejectionsOnStack at line 94 always-true when reached, and the SetForScope at line 98 sets a flag no reachable path reads. Either drop the guard machinery this PR added (the field at ZigGlobalObject.h:435, the SetForScope, #include <wtf/SetForScope.h> / "ZigGlobalObject.h", the zigGlobal local, and the comment) — the previous review's option (b) — or gate BunProcess.cpp:841 on !isEmpty() too and delete the whole if (isEmpty()) block from drain().

  Extended reasoning...

  What

  Commit 856ede9 addressed the previous review by gating the first nextTickQueue->drain() call at ZigGlobalObject.cpp:3221 on !nextTickQueue->isEmpty(), matching the second call at :3243. With both GlobalObject::drainMicrotasks callers now gated, the if (isEmpty()) branch in JSNextTickQueue::drain (lines 84–89) — and thus the whole mustResetContext / m_processTicksAndRejectionsOnStack machinery — is provably dead. The m_processTicksAndRejectionsOnStack field (ZigGlobalObject.h:435), the SetForScope onStack(...) at line 98, #include <wtf/SetForScope.h>, #include "ZigGlobalObject.h", and the zigGlobal local were all added by this PR (for the earlier framing of the ALS-clear review) and orphaned by this PR's own 856ede9.

  Callers of JSNextTickQueue::drain

  Grep shows exactly three live callers:

  1. ZigGlobalObject.cpp:3222 — gated on nextTickQueue && !nextTickQueue->isEmpty() (856ede9).
  2. ZigGlobalObject.cpp:3244 — gated on nextTickQueue && !nextTickQueue->isEmpty() (added earlier in this PR).
  3. BunProcess.cpp:841 (Process__dispatchOnBeforeExit) — ungated.

  (BakeGlobalObject.cpp has a commented-out call.)

  Why the flag check at line 94 is always-true when reached

  mustResetContext is set only inside the if (isEmpty()) branch at line 84, so mustResetContext == true at line 94 requires drain() to have been entered with internalField(0) == 0. Callers 1 and 2 are gated on !isEmpty(), so they never enter with isEmpty()==true. That leaves caller 3.

  Process__dispatchOnBeforeExit is invoked only from ExitHandler::dispatch_on_before_exit in VirtualMachine.rs, reached exclusively from top-level Rust event-loop drivers (run_command.rs, web_worker.rs, repl_command.rs, test_command.rs) at event-loop idle — never synchronously from inside processTicksAndRejections. So whenever mustResetContext == true, m_processTicksAndRejectionsOnStack is false, and !zigGlobal->m_processTicksAndRejectionsOnStack is always true.

  Why the SetForScope write at line 98 is never observed

  The flag is only read at line 94, and only when mustResetContext == true, i.e. only when drain() was entered with isEmpty()==true. During the SetForScope's lifetime (bracketing the JSC::call(drainFn, ...) → processTicksAndRejections), internalField(0) stays 1 — the new $putInternalField(nextTickQueue, 0, 0) at ProcessObjectInternals.ts:390 is the last statement before processTicksAndRejections returns, after which the SetForScope immediately destructs. So any nested drain() reached via wait_for_promise → tick → GlobalObject::drainMicrotasks from inside a nextTick callback sees !isEmpty(): callers 1/2's gates pass, drain() enters with isEmpty()==false, mustResetContext stays false, and line 94 short-circuits on the left operand without reading the flag. Caller 3 cannot re-enter here (it's not on the stack). Grep confirms no other reader of m_processTicksAndRejectionsOnStack.

  Step-by-step proof

  Take the re-entrancy scenario the guard was added for — a nextTick callback that spins wait_for_promise:

  1. Event loop → GlobalObject::drainMicrotasks → line 3221: m_nextTickQueue exists, !isEmpty() (a tick is queued) → drain().
  2. drain(): isEmpty()==false → skip lines 84–89, mustResetContext = false. Line 94 short-circuits (left operand false); flag is not read.
  3. Line 98: SetForScope sets flag = true. JSC::call runs processTicksAndRejections.
  4. Inside a tick callback, user code triggers wait_for_promise → tick() → GlobalObject::drainMicrotasks → line 3221: internalField(0) is still 1 (pTAR hasn't reached line 390) → !isEmpty() → nested drain().
  5. Nested drain(): isEmpty()==false → mustResetContext = false → line 94 short-circuits; flag is not read. Runs pTAR again, returns.
  6. Outer pTAR finishes its do-while, executes $putInternalField(nextTickQueue, 0, 0), returns. SetForScope destructs, flag = false.

  And the beforeExit path:

  1. Event-loop idle → on_before_exit → Process__dispatchOnBeforeExit → BunProcess.cpp:841 drain() (ungated). No processTicksAndRejections frame is on the stack.
  2. drain(): isEmpty()==true → vm.drainMicrotasks() → suppose a microtask calls process.nextTick → internalField(0) = 1, mustResetContext = true.
  3. Line 94: mustResetContext && !flag → true && !false → true. The flag check contributes nothing.

  No reachable path reads the flag as true.

  Impact & fix

  No incorrect runtime behavior — this is dead defensive machinery. Per REVIEW.md "Delete dead code in the same PR that makes it dead": either

  • (a) drop the guard entirely — remove m_processTicksAndRejectionsOnStack from ZigGlobalObject.h:435, revert line 94 to if (mustResetContext), drop the SetForScope at line 98, the zigGlobal local at line 81, the two #includes at lines 13/17, and the comment at line 93 (the previous review's option "drop the clear entirely, since with both call sites gated it becomes provably dead again"); or
  • (b) gate BunProcess.cpp:841 on !nextTickQueue->isEmpty() too and delete the whole if (isEmpty()) { … mustResetContext = true; } block plus the mustResetContext clear from drain(), leaving it a straight if (!isEmpty()) { call(drainFn); }.

  Option (b) is the larger simplification since it makes drain()'s contract "caller checked !isEmpty()" uniform across all three sites.

[robobun]

Re the JSNextTickQueue dead-guard finding: the review targeted 856ede9 (since reverted), but the same reasoning applies to Phase 1's m_isDrainingNextTickQueue check at the same site. mustResetContext requires isEmpty()==true on entry; the flag is only true inside processTicksAndRejections, during which internalField(0)==1, so the two are mutually exclusive. Dropped in 23ee272 (JSNextTickQueue.cpp is now unchanged vs main).

[coderabbitai]

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/regression/issue/34115.test.ts`:
- Around line 125-127: Remove the explanatory three-line comment about the
nextTick callback, GlobalObject::drainMicrotasks, and asyncContextData from the
regression test, while preserving the existing issue URL comment.

🪄 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: 4d6f254e-177b-4705-89bb-51373b77c3c7

📥 Commits

Reviewing files that changed from the base of the PR and between 23ee272 and ee301a4.

📒 Files selected for processing (4)

• src/jsc/bindings/NodeAsyncHooks.cpp
• src/jsc/bindings/ZigGlobalObject.cpp
• src/jsc/bindings/ZigGlobalObject.h
• test/regression/issue/34115.test.ts

💤 Files with no reviewable changes (1)

• src/jsc/bindings/ZigGlobalObject.h

[claude]

No issues found in this pass, but this reworks the per-microtask nextTick hook (now persistently armed with a re-entrancy guard) and removes the asyncContextData[0] clear from JSNextTickQueue::drain — core event-loop machinery whose invariants took several iterations to get right here. Worth a maintainer look at the final shape.

What was reviewed:

• Re-entrancy: hook + jsFunctionDrainMicrotaskQueue under SetForScope, GlobalObject::drainMicrotasks intentionally unguarded but gated on !isEmpty(); traced the wait_for_promise-inside-nextTick path against the ALS test.
• mustResetContext deletion: confirmed all three drain() callers either gate on !isEmpty() or reach it with the hook armed, so the clear is unreachable.
• asyncHooksNeedsCleanup removal: only remaining read was a debug ASSERT satisfied by construction; cleanupAsyncHooksData/resetOnEachMicrotaskTick reduce to re-arming the hook.
• Tests: 9 subprocess cases (preload variants, worker CJS/eval, Writable.toWeb repro, ALS+HTMLRewriter spin) drain pipes concurrently and assert exact output.

Extended reasoning...

Overview

The PR fixes #34115: a --preload script that touches process.nextTick consumes the one-shot onEachMicrotaskTick hook during preload's microtask checkpoint, so the entry module's top-level nextTick drains after its microtasks instead of before. The fix keeps the hook persistently armed instead of nulling it on first fire, adds an m_isDrainingNextTickQueue re-entrancy guard, resets internalField(0) after processTicksAndRejections's do-while so isEmpty() reflects the drained state, and gates GlobalObject::drainMicrotasks's drain() call on !isEmpty(). With the hook persistent, mustResetContext and its asyncContextData[0] clear in JSNextTickQueue::drain become provably unreachable and are deleted, as is the write-only asyncHooksNeedsCleanup field.

Files touched: ProcessObjectInternals.ts (builtin JS), BunProcess.cpp, JSNextTickQueue.cpp, NodeAsyncHooks.cpp, ZigGlobalObject.cpp/.h, plus a 9-test regression file.

Security risks

None identified. No parsing of untrusted input, no auth/crypto, no allocation sizing. The change is control-flow / state-machine only.

Level of scrutiny

High. This is the per-microtask hook and the nextTick/microtask interleaving machinery — one of the most re-entrancy-sensitive paths in the runtime. The PR's own history bears that out: an earlier sync-CJS-load approach was fully reverted after causing 5 architectural regressions across 11 test files, and the current approach went through several rounds catching real bugs (nested pTAR resetting field(0) under an outer SetForScope breaking the claimed mutual-exclusion invariant; the ALS frame being cleared via mustResetContext on a wait_for_promise spin). The final diff is compact and every prior finding is resolved, but the invariants ("hook is never null", "which call sites are guarded vs gated on !isEmpty()", "the asyncContextData clear is safe to delete") are subtle enough that a maintainer familiar with the wait_for_promise / ALS interaction should confirm the final shape.

Other factors

• The PR description notes a residual, now-consistent divergence from Node: process.nextTick queued from inside a microtask runs between that microtask and its siblings, whereas Node runs it after the whole FIFO. This PR makes that ordering deterministic (previously it depended on whether the one-shot had fired) but does not align it — flagged as a separate concern (#33088/#33366). That's a design call worth a human ack.
• The hook now runs on every microtask once m_nextTickQueue exists (bool + two field loads); the PR argues this matches what the old bootstrap lambda already paid pre-fire, which reads correct, but the steady-state cost is new.
• All inline findings from prior runs are addressed and resolved; no bugs were surfaced this pass. Test coverage is solid (preload matrix, two-preload, worker file/eval, the original Writable.toWeb repro, and the ALS+wait_for_promise guard).