fix(events,fs,globals): node v26 parity — events.on async iterator, fs watch buffering, four globals diffs#5099
Merged
Conversation
… fs watch buffering, and four globals diffs
Root-cause fixes for real byte-parity gaps surfaced by the node-suite
events/globals modules (and one fs/watch buffering gap), validated against
node v26.3.0.
events (events/on, 4/69 → 0):
- The bundled `events.on(emitter, evt)` returned a bare queue array whose
`[Symbol.asyncIterator]` returned the array itself with no `.next`, so
`for await (const args of on(...))` threw "next is not a function" (and
the abort/high-watermark/import variants likewise). Reimplement as a real
Node-style async iterator: `[Symbol.asyncIterator]()` builds a
`{ next, return }` object over shared GC-rooted state — emitted events are
buffered as `[arg]` arrays, `next()` drains the buffer or blocks on a
Promise the listener resolves on the next event, an AbortSignal makes a
buffer-empty `next()` reject, and `return()` detaches the listener.
fs (fs/watch/promises-lazy-start):
- `fsPromises.watch()` took its baseline snapshot lazily at the first
`.next()`, dropping events emitted between `watch()` and the first
iteration. Node registers the FSWatcher eagerly and buffers those events.
Seed the snapshot at creation so the first poll diffs against the
creation-time state.
globals (4/115 → 0):
- Atomics.isLockFree: compared a floored ToInteger, so `isLockFree(4.9)`
was true; Node/V8 compares the raw ToNumber against {1,2,4,8} exactly, so
4.9 is false (and "4" is true).
- escape/unescape: coerced a Symbol argument to a "Symbol(x)" string; ToString
of a Symbol throws TypeError — reject it.
- Array.prototype.slice.call(arr, i, undefined): the generic array-like slice
treated an explicit `undefined` end as 0 (empty) instead of len. Only the
direct `arr.slice` path used the i32::MAX "to-end" sentinel.
- Promise.race/Promise.any over a thenable invoked the thenable's `then`
synchronously. The spec's PromiseResolveThenableJob is a microtask; route
`js_promise_resolved`'s thenable arm through the existing deferred
`promise_resolve_assimilating`. Primitives and native Promises keep their
fast paths, so only real thenables defer (by one microtask the await loop
drains), leaving resolved values identical.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR contains targeted ECMA-262 compliance fixes across array slicing, atomics, legacy builtins, promise resolution, and file watching, plus a comprehensive rewrite of the ChangesTargeted ECMA-262 and Feature Correctness Fixes
events.on() Async Iterator Rewrite
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 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.
Actionable comments posted: 3
🤖 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 `@crates/perry-runtime/src/promise/async_step.rs`:
- Around line 51-61: The async-function tail-return path uses synchronous
js_assimilate_thenable in resolve_trap_next_with_adoption(), causing a timing
mismatch with the new use of super::combinators::promise_resolve_assimilating in
js_promise_resolved/js_async_step_done; update resolve_trap_next_with_adoption()
to route thenables through the same queued assimilation helper (call
promise_resolve_assimilating or an equivalent queued-assimilation wrapper)
instead of calling js_assimilate_thenable(...) so both Promise.resolve(thenable)
and the async tail-return reuse the same scheduled-thenable semantics and
preserve PromiseResolveThenableJob parity.
In `@crates/perry-stdlib/src/events.rs`:
- Around line 1990-2001: The state saved via events_on_state_set currently only
stores EVENTS_ON_HANDLE and EVENTS_ON_LISTENER so events_on_return() cannot undo
the EventTarget/abort-listener cleanup; update the capture/state to include all
teardown metadata and/or a dedicated cleanup closure: when creating the
abort_listener in the shown block, also call events_on_state_set for the signal
pointer (signal_ptr), event_name_ptr (and any target-type indicator or original
listener pointer if different), or create and store a js_closure_alloc "cleanup"
closure that performs the same detach logic the abort handler uses, then store
that cleanup closure pointer in state (e.g. EVENTS_ON_CLEANUP). Finally ensure
events_on_return() reads and invokes that stored cleanup (or uses the additional
metadata) to detach EventTarget listeners and remove the abort_listener just
like the abort path does.
- Around line 1762-1789: In the explicit-return path (the block that calls
events_on_state_set(state, EVENTS_ON_DONE, TAG_TRUE_F64) and then detaches the
listener via EVENTS_ON_HANDLE / EVENTS_ON_LISTENER), also drain/clear the queued
events buffer (EVENTS_ON_BUFFER) so queued items don't surface to later next()
calls: get the buffer with events_on_state_array(state, EVENTS_ON_BUFFER) and
loop while js_array_length(buffer) > 0 calling
perry_runtime::array::js_array_shift_f64(buffer) (discarding the values) to
fully clear it before resolving pending promises; place this drain just after
detaching the listener and before resolving pending items so explicit return
drops queued events.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7de2211c-d165-464d-be09-27dcdeed3792
📒 Files selected for processing (6)
crates/perry-runtime/src/array/generic.rscrates/perry-runtime/src/atomics.rscrates/perry-runtime/src/builtins/globals.rscrates/perry-runtime/src/fs/dir_glob_watch.rscrates/perry-runtime/src/promise/async_step.rscrates/perry-stdlib/src/events.rs
Comment on lines
+51
to
+61
| // codegen-side `Expr::Await` lowering. `promise_resolve_assimilating` | ||
| // implements the spec PromiseResolveThenableJob: a thenable's `.then` is | ||
| // invoked from a SCHEDULED microtask, never synchronously during resolve. | ||
| // This matters for `Promise.race`/`Promise.any` over a thenable, where Node | ||
| // does not call the thenable's `then` until the job runs (so the count of | ||
| // synchronous `then` invocations stays 0). Primitives (fast path above) and | ||
| // native Promises (short-circuit above) never reach here, so the per-await | ||
| // steady state is untouched; only real thenables (drizzle's `QueryPromise`, | ||
| // object literals with `then`) defer by one microtask — which the await | ||
| // loop drains, leaving the resolved value identical. | ||
| super::combinators::promise_resolve_assimilating(promise, value); |
There was a problem hiding this comment.
Keep the async-function tail-return path on the same queued-assimilation semantics.
Line 61 now fixes js_promise_resolved by routing thenables through promise_resolve_assimilating, but resolve_trap_next_with_adoption() later in this file (Lines 350-375) still uses synchronous js_assimilate_thenable(...). That means Promise.resolve(thenable) and the js_async_step_done() reuse fast path for return thenable can now observe different timing, so this only partially fixes the PromiseResolveThenableJob parity issue.
Using the same combinator (or an equivalent queued-assimilation helper) in resolve_trap_next_with_adoption() would keep both adoption paths aligned.
🤖 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-runtime/src/promise/async_step.rs` around lines 51 - 61, The
async-function tail-return path uses synchronous js_assimilate_thenable in
resolve_trap_next_with_adoption(), causing a timing mismatch with the new use of
super::combinators::promise_resolve_assimilating in
js_promise_resolved/js_async_step_done; update resolve_trap_next_with_adoption()
to route thenables through the same queued assimilation helper (call
promise_resolve_assimilating or an equivalent queued-assimilation wrapper)
instead of calling js_assimilate_thenable(...) so both Promise.resolve(thenable)
and the async tail-return reuse the same scheduled-thenable semantics and
preserve PromiseResolveThenableJob parity.
Comment on lines
+1762
to
+1789
| events_on_state_set(state, EVENTS_ON_DONE, TAG_TRUE_F64); | ||
| // Detach the queue listener from the emitter so no further events queue. | ||
| let handle = perry_runtime::array::js_array_get_f64(state, EVENTS_ON_HANDLE); | ||
| let listener = perry_runtime::array::js_array_get_f64(state, EVENTS_ON_LISTENER); | ||
| if handle.to_bits() != TAG_UNDEFINED_F64_BITS | ||
| && listener.to_bits() != TAG_UNDEFINED_F64_BITS | ||
| { | ||
| let handle_id = handle as Handle; | ||
| let listener_ptr = js_nanbox_get_pointer(listener); | ||
| if let Some(emitter) = get_handle_mut::<EventEmitterHandle>(handle_id) { | ||
| remove_listener_by_callback(emitter, listener_ptr); | ||
| } | ||
| } | ||
| // Resolve any blocked `next()` with completion. | ||
| let pending = events_on_state_array(state, EVENTS_ON_PENDING); | ||
| if !pending.is_null() { | ||
| while js_array_length(pending) > 0 { | ||
| let promise = | ||
| js_nanbox_get_pointer(perry_runtime::array::js_array_shift_f64(pending)) | ||
| as *mut Promise; | ||
| if !promise.is_null() { | ||
| js_promise_resolve( | ||
| promise, | ||
| events_iter_result(f64::from_bits(TAG_UNDEFINED_F64_BITS), true), | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Drop queued events on explicit return().
next() drains EVENTS_ON_BUFFER before it checks done, which is correct for abort ordering. But return() never clears that buffer, so anything queued just before close will still come out of a later next() even though the iterator has already been ended. Clear the buffer here, or split explicit-close vs abort state so only abort preserves queued items.
Suggested fix
unsafe {
+ events_on_state_set(
+ state,
+ EVENTS_ON_BUFFER,
+ js_nanbox_pointer(js_array_alloc(0) as i64),
+ );
events_on_state_set(state, EVENTS_ON_DONE, TAG_TRUE_F64);
// Detach the queue listener from the emitter so no further events queue.
let handle = perry_runtime::array::js_array_get_f64(state, EVENTS_ON_HANDLE);🤖 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-stdlib/src/events.rs` around lines 1762 - 1789, In the
explicit-return path (the block that calls events_on_state_set(state,
EVENTS_ON_DONE, TAG_TRUE_F64) and then detaches the listener via
EVENTS_ON_HANDLE / EVENTS_ON_LISTENER), also drain/clear the queued events
buffer (EVENTS_ON_BUFFER) so queued items don't surface to later next() calls:
get the buffer with events_on_state_array(state, EVENTS_ON_BUFFER) and loop
while js_array_length(buffer) > 0 calling
perry_runtime::array::js_array_shift_f64(buffer) (discarding the values) to
fully clear it before resolving pending promises; place this drain just after
detaching the listener and before resolving pending items so explicit return
drops queued events.
Comment on lines
+1990
to
2001
| // Record the emitter handle + listener so `return()` can detach cleanly. | ||
| events_on_state_set(state, EVENTS_ON_HANDLE, handle as f64); | ||
| events_on_state_set(state, EVENTS_ON_LISTENER, js_nanbox_pointer(listener as i64)); | ||
|
|
||
| if let Some(signal) = signal { | ||
| if let Some(signal_ptr) = object_ptr_from_value(signal) { | ||
| let abort_listener = js_closure_alloc(events_on_abort_listener as *const u8, 5); | ||
| js_closure_set_capture_ptr(abort_listener, 0, handle); | ||
| js_closure_set_capture_ptr(abort_listener, 1, listener as i64); | ||
| js_closure_set_capture_ptr(abort_listener, 2, signal_ptr as i64); | ||
| js_closure_set_capture_ptr(abort_listener, 3, abort_promise as i64); | ||
| js_closure_set_capture_ptr(abort_listener, 3, state as i64); | ||
| js_closure_set_capture_ptr(abort_listener, 4, event_name_ptr as i64); |
There was a problem hiding this comment.
Capture full teardown state for return().
The shared state only records handle and the queue listener, so events_on_return() can only detach the EventEmitterHandle case. For EventTarget/stream targets—and for the abort listener registered here—explicit return() leaves callbacks installed, so a closed iterator can keep accumulating future events and stay retained until abort or GC. Store enough metadata to run the same cleanup path from return() that abort uses, or capture a dedicated cleanup closure in the state.
🤖 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-stdlib/src/events.rs` around lines 1990 - 2001, The state saved
via events_on_state_set currently only stores EVENTS_ON_HANDLE and
EVENTS_ON_LISTENER so events_on_return() cannot undo the
EventTarget/abort-listener cleanup; update the capture/state to include all
teardown metadata and/or a dedicated cleanup closure: when creating the
abort_listener in the shown block, also call events_on_state_set for the signal
pointer (signal_ptr), event_name_ptr (and any target-type indicator or original
listener pointer if different), or create and store a js_closure_alloc "cleanup"
closure that performs the same detach logic the abort handler uses, then store
that cleanup closure pointer in state (e.g. EVENTS_ON_CLEANUP). Finally ensure
events_on_return() reads and invokes that stored cleanup (or uses the additional
metadata) to detach EventTarget listeners and remove the abort_listener just
like the abort path does.
…sync-iterator rewrite
proggeramlug
added a commit
that referenced
this pull request
Jun 14, 2026
… post-pull writes are delivered (#5099 regression) (#5117) #5099 seeded the promises-watcher snapshot at watch() creation time AND stopped re-snapshotting at the first .next() pull (in start_promise_watcher), intending to deliver writes emitted between watch() and the first iteration. That broke post-pull delivery. With the baseline frozen at the empty creation-time state, the first poll after the first .next() diffs that empty baseline against the post-early-write directory and resolves the *first* pending pull with the pre-iteration write. The promises-lazy-start test reuses that same pending promise for the next race, so the later write (late.txt) is never delivered to it — and the pre-iteration write is wrongly surfaced. Result: both "pre-pull ignored" and "post-pull delivered" assertions fail against Node. Fix: re-take the baseline snapshot in start_promise_watcher at the first .next() (folding the current directory state into the baseline, so pre-iteration writes are ignored — matching Node, whose async iterator only starts collecting once you iterate), then let promise_watcher_poll_impl keep advancing the baseline after every poll (so a write *after* a pull is detected as a fresh change and delivered). The creation-time snapshot_watch_target call is kept solely as the synchronous path validation / error path (Node throws ENOENT etc. at watch() call time). Validation (node v26): - fs/watch/promises-lazy-start.ts: deterministically prints "pre-pull ignored: true" / "post-pull delivered: true" (3/3). - fs/watch/promises-watch.ts: still PASS. - Isolated repro: a write after a pull begins is delivered to that pull. Co-authored-by: Ralph Küpper <ralph2@skelpo.com>
proggeramlug
added a commit
that referenced
this pull request
Jun 15, 2026
Refresh floors from a clean node-26 full run (2810/2863, 98.1%): - object 23/23, util 86/86, tty 32/32, events 69/69 -> full (#5144/#5106-7/#5099) - stream 770, globals 111, diagnostics_channel 66, fs-promises 77 -> ratcheted up with small flake margins - http floor 19 -> 17: verified 19/19 in isolation but the full-suite harness flakes to 17 under port contention; a real http regression is a far larger drop (link break / behavior break), which still trips the guard. Stops the false-alarm seen on prior runs. Co-authored-by: Ralph Küpper <ralph2@skelpo.com>
proggeramlug
added a commit
that referenced
this pull request
Jun 15, 2026
events.once / events.on with an AbortSignal used to leave a leaked keepalive (pending promise / abort listener) parking the event loop, so the program produced correct output then never exited. The runtime fix landed incidentally with the events.on async-iterator rework (#5099); this adds an explicit guard so it cannot silently regress. The node-suite differential runner does check exit codes, but a hang only surfaces as a timeout there and that runner is not part of the per-PR gate. This stand-alone script (same shape as tests/test_issue_3730_timers_promises_unref_await.sh) compiles both abort fixtures and asserts each exits 0 within a timeout with the expected stdout — a hang trips the timeout (exit 124) and fails. Co-authored-by: Ralph <ralph@skelpo.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Root-cause fixes for the real byte-parity gaps in the node-suite events, globals, and fs modules, validated against node v26.3.0. Triaged with the official
run_parity_tests.shharness (which skips node-nonzero-exit tests) rather than a naive 2>&1 diff.events (
events/on): 4/69 → 0events.on(emitter, evt)returned a bare queue array whose[Symbol.asyncIterator]returned the array itself with no.next, sofor await (const args of on(...))threwnext is not a function.Reimplemented as a real Node-style async iterator:
[Symbol.asyncIterator]()builds a{ next, return }object over shared GC-rooted state — events are buffered as[arg]arrays,next()drains the buffer or blocks on a Promise the listener resolves on the next event, anAbortSignalmakes a buffer-emptynext()reject, andreturn()detaches the listener.Fixes
async-iterator-basic,async-iterator-abort,high-watermark-options,import-callable.globals: 4/115 → 0
Atomics.isLockFreecompared a floored ToInteger, soisLockFree(4.9)wastrue. V8/Node compares the raw ToNumber against{1,2,4,8}exactly →4.9→false,"4"→true.escape/unescapecoerced a Symbol arg to"Symbol(x)".ToStringof a Symbol throwsTypeError— now rejected.Array.prototype.slice.call(arr, i, undefined)— the generic array-like path treated an explicitundefinedend as0(empty) instead oflen. Only the directarr.slicepath used thei32::MAX"to-end" sentinel.Promise.race/Promise.anyover a thenable invoked the thenable'sthensynchronously. The spec's PromiseResolveThenableJob is a microtask;js_promise_resolved's thenable arm now routes through the existing deferredpromise_resolve_assimilating. Primitives and native Promises keep their fast paths, so only real thenables defer (by one microtask the await loop drains) — resolved values are identical.fs (
fs/watch/promises-lazy-start)fsPromises.watch()took its baseline snapshot lazily at the first.next(), dropping events emitted betweenwatch()and the first iteration. Node registers the FSWatcher eagerly and buffers them. Now seeds the snapshot at creation so the first poll diffs against the creation-time state. Perry now deterministically matches node's dominantfalse/false.Documented platform-variant remainder (not a Perry bug)
fs/watch/watchfile-delivery— node's own two-listenerwatchFiledelivery is flaky; 5×noderuns of the first assertion:false, true, false, false, false. Perry is consistently more correct; byte-matching node's flake is neither possible nor desirable. (The official harness already excludes thermdir { recursive: true }andwriteFile([...])cases here because node exits non-zero on them.)Verification (node v26.3.0, this Mac)
builtin-name-length-descriptorsis a node-nonzero-exit skip).fs/watchitems are node-side races documented above.origin/mainbaseline built at the branch's merge-base:buffer134/0,http19/0,util85/1 (the 1 fail is identical on baseline), and all 13 promise/thenable-sensitivestreamtests match (the 2Readable.from(promise)diffs are pre-existing node-crash cases, identical on baseline). The fullstreammodule times out under concurrent load on both branches (pre-existing harness slowness).cargo test --release -p perry-runtime -p perry-stdlib -p perry-codegen: green (0 failed across all binaries).Summary by CodeRabbit
Bug Fixes
New Features
Chores