Conversation
|
Warning Review limit reached
Next review available in: 34 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 (7)
Comment |
|
Updated 10:24 PM PT - Jul 6th, 2026
❌ @robobun, your commit e3ebbd6 has some failures in 🧪 To try this PR locally: bunx bun-pr 33495That installs a local version of the PR into your bun-33495 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Confirming the two bot findings, since they point at different things: #5121 is an exact match and is now in the description. It is the #31831 overlaps on exactly that third, and nothing else. Concretely, it makes the same two edits:
It does not touch
The overlap is ~20 lines across 2 files. I'm happy to drop those hunks and rebase on #31831 if it lands first, or leave them so this stands alone if it doesn't. Whichever is less work for you. |
|
Thanks, these were good. Three of the four were real bugs, and I checked each against node v26.3.0 rather than reasoning from the source, which changed the answer twice. Fixed in 8cc9215 (and a4728f4). 1. Signal handler added mid-drain (
|
…of, and rawListeners `process` is backed by the native WebCore::EventEmitter rather than node:events, and that implementation diverged from Node in three ways: - The 'removeListener' meta-event was never emitted. Libraries that mirror the listener table via 'newListener'/'removeListener' saw every add and no removal, so they leaked handlers. Emit it from EventEmitter::removeListener so every removal path is covered, including the auto-removal of a once() listener when it fires. removeAllListeners(type) now drains LIFO like Node, and the no-arg form drains per event type, which also fixes onDidChangeListener never running and leaving OS signal handlers installed. - `process instanceof EventEmitter` was false. node:events now splices EventEmitter.prototype underneath the native prototype. That prototype no longer carries IsImmutablePrototypeExoticObject, which was copied from EventTarget (where WebIDL mandates it); Node's EventEmitter.prototype is an ordinary object. - rawListeners() was aliased to listeners(). It now returns a wrapper for once() listeners carrying the documented `.listener` back-pointer, cached per registration so its identity is stable. removeListener() accepts such a wrapper and resolves it back to the registration it was made for.
…ed mid-drain Two gaps found in review: - rawListeners() hands out a wrapper, but emit() invoked the stored function directly, so the wrapper's one-shot guard never tripped and a wrapper held across an emit would invoke the listener a second time. Fire the wrapper when one exists, as Node does. A wrapper that never fired still invokes its listener even once the registration is gone, matching Node, which gates only on whether the wrapper itself ran. - A 'removeListener' handler can register listeners for event types the no-arg removeAllListeners() drain already snapshotted past. Those are wiped silently, as in Node, but the native side was never told, so an OS signal handler installed by that late registration outlived its listener.
…nt emit
Removing a once() listener now emits 'removeListener', so the auto-removal of
one once('removeListener') handler re-entrantly invokes the next one while the
outer emit still holds it in its snapshot, firing it twice.
Node's stored once() wrapper carries a `fired` flag for exactly this, so give
the registration the same bit. It has to key off the listener having already
run rather than off it having been removed: an emit in flight still invokes a
once() listener that an earlier handler unregistered, which is what Node does.
8cc9215 to
39fb0f5
Compare
Build 69072 passed 277 jobs with 0 failures; 9 jobs expired waiting for agents (6 of them on the darwin queue).
Status: ready for review, CI is red on the darwin lane onlyThe diff is green. Every lane that actually executed the test suite passed it: linux x64, linux aarch64, linux x64-baseline, alpine musl, windows x64-baseline, windows aarch64. The one red job never ran a test. It died downloading the build artifact: CI history
The darwin queue has been starved or timing out on artifact downloads across all three. I've used my one retrigger and won't push again, so this needs a maintainer to either re-run the darwin lane or merge past it. Two things still worth a maintainer's call
|
… tests macOS x64 terminates the child with SIGSYS where Linux delivers SIGUSR2, for the same default-terminate outcome once the handler is uninstalled. Assert the portable shape (killed by a signal, empty stdout) and pin SIGUSR2 only on Linux. The empty-stdout check is still the discriminator: an unfixed build leaves the handler installed, so the child reaches the 'survived' print.
|
Correction to my status note above: build 69142 had two darwin reds, not one, and they were different in kind.
The macOS failure was narrow and not a fix regression: the child was still terminated (empty stdout, never reached the Fixed in e3ebbd6 by asserting the portable shape (killed by a signal, empty stdout) and pinning The SIGSYS-vs-SIGUSR2 difference is in pre-existing signal-delivery code that this PR only newly reaches via |
Status: diff is green, darwin-aarch64 lane is blocked on infraBuild 69466 is final: 282 passed, 2 failed, 2 expired. Every platform that actually executed the suite passed it, including darwin 14 x64, which is the lane that caught my over-strict SIGSYS assertion on build 69142. e3ebbd6 fixed that and it now passes on CI. Everything still red is darwin-aarch64, and none of it ran a test:
This is a persistent darwin-aarch64 artifact-store/queue problem across four consecutive builds (69072 agent capacity, 69142 artifact timeout, 69466 both), independent of this diff. I've used my one retrigger and won't push again; this needs a maintainer to re-run the darwin-aarch64 lane or merge past it. The two open decisions from earlier are unchanged: the |
|
Status against current main (165dc9f), now that #31831 has landed and closed #5121. The Already on main: Still failing on main:
The repro from the description on Node v26.3.0 prints meta-events So the meta-event and |
|
Checked this PR against main after #31831 merged. Method: debug build of main at 731aa92, then this PR's Landed on main (the passing tests):
Still open on main (the failing tests):
The branch conflicts with main in Per-test result on main |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-06, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
Fixes #5121
Note
The
instanceofpart of this PR (droppingIsImmutablePrototypeExoticObjectand thenode:eventssplice, ~20 lines across 2 files) is the same change as a hunk in #31831. The'removeListener'meta-event andrawListeners()fixes are independent of it. Happy to drop the overlapping hunks and rebase if #31831 lands first.Repro
A plain
new EventEmitter()gets all three right, so this is specific toprocess.Cause
processis backed by the nativeWebCore::EventEmitter(src/jsc/bindings/webcore/EventEmitter.cpp) rather thannode:events, and that implementation drifted from Node in three places:JSEventEmitter::addListeneremits'newListener', but nothing ever emitted'removeListener'.JSEventEmitterPrototype's[[Prototype]]isObject.prototype, soEventEmitter.prototypeis not inprocess's chain.rawListenerswas wired straight to thelistenershost function (there was aTODOon the line), soonce()listeners came back unwrapped.Two further divergences fell out of the same code while fixing the first one:
EventEmitter::removeAllListeners()(no args) cleared the map without callingonDidChangeListener, soprocess.removeAllListeners()left OS signal handlers installed and the IPC channel ref'd.'removeListener'when aonce()listener fires (itsonceWrapperremoves itself); Bun removes the registration ininnerInvokeEventListenerswithout notifying.'newListener'/'removeListener'is the documented way to mirror the listener table, so signal-handler managers and graceful-shutdown libraries that install a real handler only while a user listener exists saw every add and no removal, and leaked handlers.Fix
'removeListener'is emitted fromEventEmitter::removeListener, the one place every removal funnels through, sooff(),removeListener(),removeAllListeners(), and the auto-removal of aonce()listener all report.removeAllListeners(type)drains LIFO like Node; the no-arg form drains one event type at a time (handling'removeListener'itself last), which also getsonDidChangeListenerrunning again and fixes the leaked signal handler. Both overloads protectthisbecause the meta-event runs user JS in their frame.instanceof:node:eventssplicesEventEmitter.prototypein underneath the native prototype. Doing it there rather than atprocesscreation keepsnode:eventsoff the startup path for programs that never load it, and you cannot observeprocess instanceof EventEmitterwithout loading it anyway.JSEventEmitterPrototypeno longer carriesIsImmutablePrototypeExoticObject, which was copied fromJSEventTargetwhere WebIDL mandates it (EventTarget.idl); Node'sEventEmitter.prototypeis an ordinary object.rawListenersgets its own host function that materializes a once-wrapper exposing the documented.listenerback-pointer, built by the newEventEmitterOnce.tsbuiltin. Registrations storeonce()listeners unwrapped, so the wrapper is created on demand and cached in aJSC::Weakon the registration to keep its identity stable. It is weak deliberately: once nothing holds the wrapper its identity is unobservable.removeListener()resolves such a wrapper back to the registration it was made for, and calling the wrapper removes that registration and invokes the original, as Node's does.listeners()keeps returning unwrapped functions, and'newListener'keeps receiving the original foronce()— both now have regression coverage.Verification
test/js/node/events/event-emitter.test.tsgains aprocessblock. 7 of the 9 new tests fail onmainand pass here; the other 2 are the regression guards above. Anything mutating process-wide listener state runs in a child so it cannot disturb the test runner.The signal-handler leak is asserted directly: a child does
process.on("SIGUSR2", …); process.removeAllListeners(); process.kill(process.pid, "SIGUSR2"). With the handler still installed it printssurvived; with it uninstalled the default action terminates the child, which is what Node does.Suites run against the debug (ASAN) build
test/js/node/events/event-emitter.test.ts— 76 passtest/js/node/test/parallel/test-event*— 39 pass, 1 pre-existing failure (test-events-add-abort-listener.mjs, also fails onmain)test/js/node/test/parallel/test-{event,process,signal}*— 100 pass, 3 pre-existing/timeouttest/js/bun/spawn/spawn.ipc.test.ts,spawn-ipc-gc.test.ts,test/js/web/workers/worker.test.ts— 35 passtest/js/node/process/{process,process-on,process-signal-listener-count,process-memory-pressure}— passAn ASAN stress pass (2000
once()/rawListeners()/emit()/removeAllListeners()cycles with forced GC, plusremoveListenerhandlers that remove and re-register listeners re-entrantly) is clean. Verified thatprocess instanceof EventEmitter, the back-pointer, and the meta-event all hold inside worker threads, which get their own VM and prototype.Bun.inspect(process)andfor…in processare unchanged, and the latter matches Node.