fix(events): drop orphaned native-async token after synchronous events.once settle#5191
Merged
Conversation
…s.once settle
`events.once(emitter, name)` (and the stream / EventTarget variants) allocate
their Promise through perry-ffi's `JsPromise::new()` → `perry_ffi_promise_new()`,
which registers a native-async completion token (and pins the Promise) so a
worker thread could resolve it later. But `events.once` settles *synchronously*
from `emit(...)` and deliberately bypasses the deferred completion machinery
(it calls `js_promise_resolve`/`js_promise_reject` directly — see the extern
comment in perry-ext-events for why). That bypass never runs
`js_native_async_process_pending`, so the token stays in the registry forever.
`js_native_async_has_active()` then keeps reporting work and the generated event
loop never exits — the process hangs after the awaited event already fired.
Repro (hangs at exit 124, prints the right output first):
import { EventEmitter, once } from "node:events";
const e = new EventEmitter();
once(e, "end").then(() => console.log("done"));
e.emit("end");
Fix: add `js_native_async_drop_promise_token(promise)` to perry-runtime, which
removes the orphaned token from the native-async registry (mirroring the cleanup
`js_native_async_process_pending` performs after a normal completion). Call it
right after every synchronous settle in perry-ext-events' `events.once` paths:
the EventEmitter resolve / error-reject, the abort-signal reject, the stream
resolve / reject, the EventTarget resolve, and the early arg-type / signal
rejects in `js_events_once`.
Closes node-suite stream/events/once-returns-promise and
stream/events/once-static-promise (both were exit-124 hangs; now match node
v26 byte-for-byte). Verified the EventEmitter, stream, EventTarget, abort, and
error-reject once() paths all exit cleanly with no GC use-after-free (the token
drop mirrors the existing post-resolution cleanup lifecycle).
📝 WalkthroughWalkthroughAdds a new runtime function ChangesNative async token cleanup for events.once
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-ext-events/src/module_iterators.rs (1)
105-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd token-drop after synchronous reject in
events_on_abort_listener.
abort_promiseis synchronously rejected on Line 106 but its native-async token is not retired, which can leave active-work bookkeeping stuck for this path too.Suggested fix
if !abort_promise.is_null() { js_promise_reject(abort_promise, js_abort_error_value()); + js_native_async_drop_promise_token(abort_promise); }🤖 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 `@crates/perry-ext-events/src/module_iterators.rs` around lines 105 - 107, In the events_on_abort_listener function, after the synchronous rejection of abort_promise via js_promise_reject call (line 106), add a token-drop operation to retire the native-async token. This ensures that the active-work bookkeeping is properly cleaned up in this synchronous reject path, preventing the token from remaining in an active state.
🤖 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.
Outside diff comments:
In `@crates/perry-ext-events/src/module_iterators.rs`:
- Around line 105-107: In the events_on_abort_listener function, after the
synchronous rejection of abort_promise via js_promise_reject call (line 106),
add a token-drop operation to retire the native-async token. This ensures that
the active-work bookkeeping is properly cleaned up in this synchronous reject
path, preventing the token from remaining in an active state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bfeea816-972b-469c-aa71-65763ec5ec4a
📒 Files selected for processing (4)
crates/perry-ext-events/src/lib.rscrates/perry-ext-events/src/module_iterators.rscrates/perry-runtime/src/promise/mod.rscrates/perry-runtime/src/promise/native_async.rs
Keeps perry-ext-events/src/lib.rs under the 2000-line file-size gate after the once-token-drop additions: the abort / stream-resolve / stream-reject events.once listener trampolines move into module_iterators.rs alongside the existing events_once_event_target_listener (pure code move, no logic change).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-ext-events/src/module_iterators.rs (1)
169-171:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop the native-async token for
events.onabort promises too.
js_events_oncreatesabort_promisewithJsPromise::new()incrates/perry-ext-events/src/lib.rsLines 1732-1733, but this abort listener settles it synchronously withjs_promise_reject()and never retires the native-async token. This is the same orphaned-token keepalive pattern this PR fixes forevents.once.🐛 Proposed fix
if !abort_promise.is_null() { js_promise_reject(abort_promise, js_abort_error_value()); + js_native_async_drop_promise_token(abort_promise); }🤖 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 `@crates/perry-ext-events/src/module_iterators.rs` around lines 169 - 171, The abort_promise in the js_events_on function is settled synchronously with js_promise_reject() but the corresponding native-async token is never retired, creating an orphaned-token keepalive pattern. After calling js_promise_reject(abort_promise, js_abort_error_value()), you must also drop or retire the native-async token associated with the abort_promise to match the fix applied to the events.once case. This ensures the token is properly cleaned up when the abort listener completes.
🤖 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.
Outside diff comments:
In `@crates/perry-ext-events/src/module_iterators.rs`:
- Around line 169-171: The abort_promise in the js_events_on function is settled
synchronously with js_promise_reject() but the corresponding native-async token
is never retired, creating an orphaned-token keepalive pattern. After calling
js_promise_reject(abort_promise, js_abort_error_value()), you must also drop or
retire the native-async token associated with the abort_promise to match the fix
applied to the events.once case. This ensures the token is properly cleaned up
when the abort listener completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 98c8ca1b-df03-4273-b5aa-b51d307afe58
📒 Files selected for processing (2)
crates/perry-ext-events/src/lib.rscrates/perry-ext-events/src/module_iterators.rs
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.
Summary
Root-caused and fixed the one genuine, byte-matchable Perry bug remaining in the
node-suite/streamcluster:events.once(...)makes the process hang after the awaited event already fired (exit 124). Bothstream/events/once-returns-promiseandstream/events/once-static-promisenow match node v26 byte-for-byte.Root cause
events.once(emitter, name)(and the stream /EventTargetvariants inperry-ext-events) allocate their Promise through perry-ffi'sJsPromise::new()→perry_ffi_promise_new(), which registers a native-async completion token (and pins the Promise) so a worker thread could resolve it later.But
events.oncesettles synchronously fromemit(...)and deliberately bypasses the deferred completion machinery — it callsjs_promise_resolve/js_promise_rejectdirectly (see the long extern comment in perry-ext-events for why). That bypass never runsjs_native_async_process_pending, so the token is never removed from the registry.js_native_async_has_active()then keeps reporting work, the generated event loop'sjs_stdlib_has_active_handles()check stays hot, and the process idle-hangs forever — even though the awaited event already fired and the correct output already printed.Minimal repro (prints
done, then hangs at exit 124):Localized by instrumenting the generated event loop's liveness checks (
js_microtasks_pending,js_native_async_has_active, the stdlib active-handle gate) — onlyjs_native_async_has_activestayed attokens=1after settle.Fix
js_native_async_drop_promise_token(promise)that removes the orphaned token from the native-async registry, mirroring the cleanupjs_native_async_process_pendingalready performs after a normal completion (state →COMPLETED,remove_token_from_registry).events.oncepaths — EventEmitter resolve / error-reject, abort-signal reject, stream resolve / reject,EventTargetresolve, and the early arg-type / signal rejects injs_events_once.Not a codegen/dispatch entry and not JS-visible, so no
API_MANIFESTrow needed.Verification
events/once-returns-promise+events/once-static-promisenow MATCH node (exit 0). Verified the EventEmitter, stream,EventTarget, abort, and error-rejectonce()paths all exit cleanly under both the default auto-optimize and--no-auto-optimizelink paths, with no GC use-after-free.-p perry-runtime -p perry-ext-events -p perry-stdlib -p perry-codegen: green. perry-runtime shows 5 failures under parallel test threads (closure::dynamic_props,url::node_compat,object::tests— all unrelated subsystems); they pass single-threaded (1042 passed; 0 failed) — pre-existing parallel-isolation flakes.events.oncepaths). Spot-checked fs/http/net/buffer: the only diffs found (net/server/connection-state-limitshang,http/agent/*crashes) reproduce identically on pristineorigin/mainand don't useevents.once— pre-existing, out of scope.Remaining stream-suite diffs (classified, intentionally not touched)
Out of 801, after this fix the residual diffs are all node-failing-itself or non-byte-matchable:
finished/error-false-option(n=0): not byte-matchable —console.logof anErrorprints node-internal stack frames (node:internal/modules/esm/module_job:439:25) Perry's runtime can't reproduce.web/transform-*cluster (~10, n=13): node v26.3.0 itself deadlocks ("Detected unsettled top-level await", exit 13) on default-HWMTransformStreamwrites-before-reads. Node failing itself.abort/*,compose/sync-generator,readable/from-*,transform/no-options-default,error/missing-write-option,pipeline/*): node correctly throws on invalid input the tests mis-use (e.g.Readable.from(Promise),new Transform()with no_transform, callback-formpipeline(..., {opts}, cb)). Perry is more lenient; matching would require adding strict validation that risks regressing compiled npm packages — skipped per the node_err guidance.Summary by CodeRabbit
events.oncethat could cause the event loop to hang when Promises are settled synchronously.events.onceresolve/reject and abort paths.events.oncelistener trampolines to use shared iterator-based implementations.