fix(stream): complete Node.js 26.5 stream parity - #7067
Conversation
📝 WalkthroughWalkthroughThis PR updates Web Streams and Node stream semantics, including pipe validation, cancellation, backpressure, async iteration, pipeline errors, uncaught diagnostics, compiler lowering, generator suspension analysis, and parity-test normalization. ChangesStream runtime behavior
Compiler and parity validation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Caller
participant ReadableStream
participant PipeThroughValidation
participant PipeTo
participant TransformStream
Caller->>ReadableStream: pipeThrough(transform, options)
ReadableStream->>PipeThroughValidation: validate locks, handles, and signal
PipeThroughValidation->>PipeTo: start piping to transform writable
PipeTo->>TransformStream: write and schedule transform work
TransformStream-->>Caller: return transform readable
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 8
🧹 Nitpick comments (3)
crates/perry-runtime/src/node_stream_readwrite.rs (1)
1499-1510: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
call_symbol_async_iterator_for_flat_mapfor the genericReadable.frompath.The helper name says
flatMap, butnormalize_readable_from_inputalso uses it directly;Readable.from(asyncIterable)will short-circuit beforeflatten_async_iterable_with_source, so a clearer generic async-iterator probe name avoids implying flatMap-specific behavior.🤖 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/node_stream_readwrite.rs` around lines 1499 - 1510, Rename call_symbol_async_iterator_for_flat_map to a generic async-iterator probe name, then update all references, including normalize_readable_from_input and any flatMap callers. Preserve the existing detection and short-circuit behavior for Readable.from(asyncIterable), while reserving flatten_async_iterable_with_source for inputs that require flattening.crates/perry-transform/src/generator/per_iteration.rs (1)
1073-1096: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood regression test; consider mirroring it for the
for-loop path.
loop_local_used_after_await_stays_hoistedcorrectly locks in theWhilecase. The same bug class (loop-carried binding read afterawait) can also occur throughanalyze_loop'sStmt::Forbranch (Lines 141-157) andloop_contains_suspend'sForarm (Lines 236-248), neither of which has a dedicated test.🤖 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-transform/src/generator/per_iteration.rs` around lines 1073 - 1096, Mirror loop_local_used_after_await_stays_hoisted with a dedicated test using Stmt::For, declaring a loop-local binding, suspending with Await, and reading it afterward. Ensure the test exercises analyze_loop’s Stmt::For path and loop_contains_suspend’s For arm, and asserts the binding remains hoisted by not appearing in collect_per_iteration_ids.crates/perry-stdlib/src/streams/transform.rs (1)
553-575: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider a readable→transform reverse index instead of scanning
TRANSFORM_STREAMS.
transform_initial_backpressure/set_transform_initial_backpressurelinear-scan all transform streams and are hit on everytransform_writeand everytransform_release_writes(i.e. every pipe step). AHashMap<readable_id, transform_id>(mirroringTRANSFORM_PAIRS) would make these O(1) and keep eviction bookkeeping inidalloc::evict_idsexplicit.🤖 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/streams/transform.rs` around lines 553 - 575, Replace the linear scans in transform_initial_backpressure and set_transform_initial_backpressure with a readable_id-to-transform_id reverse index, mirroring TRANSFORM_PAIRS. Maintain this index whenever transform streams are created or removed, and use it to access the corresponding TRANSFORM_STREAMS entry in O(1) while keeping eviction cleanup explicit in idalloc::evict_ids.
🤖 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-codegen/src/lower_call/options/fetch.rs`:
- Around line 969-988: Update the codegen path in the pipeThrough lowering to
retain the promise returned by js_readable_stream_pipe_to, then emit
js_promise_mark_internally_handled for that promise before returning the
readable stream. Preserve the existing validation call and return behavior, or
reuse js_readable_stream_pipe_through if appropriate.
In `@crates/perry-runtime/src/node_stream_constructors/web_adapter.rs`:
- Around line 151-152: Update build_web_read_result to construct the enumerable
object with the value property before done, preserving observable Object.keys()
and JSON serialization order while leaving the existing values unchanged.
In `@crates/perry-runtime/src/node_stream_iter_helpers.rs`:
- Around line 1021-1043: Update ns_take_source_next to check remaining <= 0
before invoking the source iterator’s next operation, including take(0), and
immediately finish/close the source and return the done result. Preserve the
existing promise-resolution handling only for pulls already initiated before the
limit was reached, without performing or discarding an extra source pull.
- Around line 1076-1090: Update ns_iter_take’s retained-source early-return path
to call propagate_stream_state(this, TAG_UNDEFINED, result) after creating the
empty result stream and before returning it, preserving the source hidden error
and abort state. Simplify the source-iterator guard to use .is_some() since the
binding is unused.
In `@crates/perry-runtime/src/node_stream_pipeline.rs`:
- Around line 403-411: Root all retained JS values with RuntimeHandleScope
across allocations and JS-invoking operations, then reload them before
subsequent use. In crates/perry-runtime/src/node_stream_pipeline.rs at lines
403-411, 480-484, 538-550, 1016-1031, and 1098-1122, root the indicated value,
iterator, stage, composite, and source values and preserve reloading after
collection, stream construction, listener allocation or installation, and
priming. In crates/perry-runtime/src/node_stream_tests_extra.rs at lines
923-931, 1044-1061, 1101-1102, 1130-1131, and 1161-1162, root opts, streams,
errors, and derived handles across closure/listener allocation and derive
handles only after allocation as needed; do not rely on Rust locals
conservatively retaining NaN-boxed values or raw handles.
In `@crates/perry-runtime/src/node_stream/async_iterator.rs`:
- Around line 522-557: The source-iterator pull paths reuse unrooted NaN-boxed
locals after invoking user code, allowing GC relocation to invalidate them. In
crates/perry-runtime/src/node_stream/async_iterator.rs lines 522-557, root
iterator and stream with crate::gc::RuntimeHandleScope and reload them before
call_source_iterator_return, destroy_stream, and js_closure_set_capture_f64; in
crates/perry-runtime/src/node_stream_iter_helpers.rs lines 994-1053, root
iterator, source, and source_iterator similarly and reload them before
finish_take_source, take_source_fulfilled, and closure captures.
- Around line 325-327: Update ns_readable_iter_on_data so it resumes the
readable stream after delivering a chunk whenever iterator waiters remain,
rather than always pausing it. Preserve the existing pause behavior when no
pending waiters remain, and align this with ns_readable_iterator_next’s
pending-pull handling.
In `@crates/perry-runtime/src/promise/microtasks.rs`:
- Around line 237-241: Update schedule_uncaught and
diagnostics_channel_drain_uncaught so pending uncaught exceptions remain rooted
across the deferred diagnostics delivery, replacing the plain Vec<f64> storage
with the runtime’s rooted or reloadable value mechanism. Ensure the drained
value is reloaded into a valid JS handle before passing it to
emit_process_event("uncaughtException", …), while preserving the existing
scheduling and drain behavior.
---
Nitpick comments:
In `@crates/perry-runtime/src/node_stream_readwrite.rs`:
- Around line 1499-1510: Rename call_symbol_async_iterator_for_flat_map to a
generic async-iterator probe name, then update all references, including
normalize_readable_from_input and any flatMap callers. Preserve the existing
detection and short-circuit behavior for Readable.from(asyncIterable), while
reserving flatten_async_iterable_with_source for inputs that require flattening.
In `@crates/perry-stdlib/src/streams/transform.rs`:
- Around line 553-575: Replace the linear scans in
transform_initial_backpressure and set_transform_initial_backpressure with a
readable_id-to-transform_id reverse index, mirroring TRANSFORM_PAIRS. Maintain
this index whenever transform streams are created or removed, and use it to
access the corresponding TRANSFORM_STREAMS entry in O(1) while keeping eviction
cleanup explicit in idalloc::evict_ids.
In `@crates/perry-transform/src/generator/per_iteration.rs`:
- Around line 1073-1096: Mirror loop_local_used_after_await_stays_hoisted with a
dedicated test using Stmt::For, declaring a loop-local binding, suspending with
Await, and reading it afterward. Ensure the test exercises analyze_loop’s
Stmt::For path and loop_contains_suspend’s For arm, and asserts the binding
remains hoisted by not appearing in collect_per_iteration_ids.
🪄 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: fffce239-99fc-481b-b24b-ce0e472f6970
📒 Files selected for processing (32)
crates/perry-codegen/src/ext_registry.rscrates/perry-codegen/src/lower_call/builtin.rscrates/perry-codegen/src/lower_call/options/fetch.rscrates/perry-codegen/src/runtime_decls/strings_part2.rscrates/perry-runtime/src/exception.rscrates/perry-runtime/src/fs/validate.rscrates/perry-runtime/src/node_stream.rscrates/perry-runtime/src/node_stream/async_iterator.rscrates/perry-runtime/src/node_stream_constructors/builders.rscrates/perry-runtime/src/node_stream_constructors/pipeline.rscrates/perry-runtime/src/node_stream_constructors/web_adapter.rscrates/perry-runtime/src/node_stream_destroy_state.rscrates/perry-runtime/src/node_stream_dispatch.rscrates/perry-runtime/src/node_stream_iter_helpers.rscrates/perry-runtime/src/node_stream_pipeline.rscrates/perry-runtime/src/node_stream_readwrite.rscrates/perry-runtime/src/node_stream_tests.rscrates/perry-runtime/src/node_stream_tests_extra.rscrates/perry-runtime/src/node_submodules/diagnostics.rscrates/perry-runtime/src/os/os_process_emitter.rscrates/perry-runtime/src/promise/microtasks.rscrates/perry-runtime/src/symbol/iterator.rscrates/perry-stdlib/src/streams.rscrates/perry-stdlib/src/streams/idalloc.rscrates/perry-stdlib/src/streams/pipe.rscrates/perry-stdlib/src/streams/subclass.rscrates/perry-stdlib/src/streams/tests.rscrates/perry-stdlib/src/streams/transform.rscrates/perry-stdlib/src/streams/writable.rscrates/perry-transform/src/generator/per_iteration.rsrun_parity_tests.shtest-parity/node-suite/stream/web/cancel-during-pipeto.ts
| fn build_web_read_result(value: f64, done: bool) -> f64 { | ||
| build_enumerable_object(&[(b"value", value), (b"done", bool_value(done))]) | ||
| build_enumerable_object(&[(b"done", bool_value(done)), (b"value", value)]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore value before done.
This makes observable key order ["done", "value"] for Object.keys() and JSON serialization. Web Streams read() results are created with value before done; keep the original field order. (streams.spec.whatwg.org)
Proposed fix
- build_enumerable_object(&[(b"done", bool_value(done)), (b"value", value)])
+ build_enumerable_object(&[(b"value", value), (b"done", bool_value(done))])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn build_web_read_result(value: f64, done: bool) -> f64 { | |
| build_enumerable_object(&[(b"value", value), (b"done", bool_value(done))]) | |
| build_enumerable_object(&[(b"done", bool_value(done)), (b"value", value)]) | |
| fn build_web_read_result(value: f64, done: bool) -> f64 { | |
| build_enumerable_object(&[(b"value", value), (b"done", bool_value(done))]) |
🤖 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/node_stream_constructors/web_adapter.rs` around
lines 151 - 152, Update build_web_read_result to construct the enumerable object
with the value property before done, preserving observable Object.keys() and
JSON serialization order while leaving the existing values unchanged.
| if remaining <= 0.0 { | ||
| finish_take_source(iterator, None, true); | ||
| return take_source_done_result(); | ||
| } | ||
| finish_take_source(iterator, Some(reason), true); | ||
| return rejected_promise(reason); | ||
| } | ||
| }; | ||
| if remaining <= 0.0 { | ||
| if crate::promise::js_value_is_promise(next) == 0 { | ||
| let close_source = matches!(pipeline_iterator_result(next), Some((false, _))); | ||
| finish_take_source(iterator, None, close_source); | ||
| return take_source_done_result(); | ||
| } | ||
| let promise = crate::value::js_nanbox_get_pointer(next) as *mut crate::promise::Promise; | ||
| let fulfilled = js_closure_alloc(ns_take_limit_fulfilled as *const u8, 1); | ||
| let rejected = js_closure_alloc(ns_take_limit_rejected as *const u8, 1); | ||
| js_closure_set_capture_f64(fulfilled, 0, iterator); | ||
| js_closure_set_capture_f64(rejected, 0, iterator); | ||
| return box_pointer( | ||
| crate::promise::js_promise_then(promise, fulfilled, rejected) as *const u8, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C4 'fn ns_iter_take|TAKE_SOURCE_REMAINING_KEY' crates/perry-runtime/src/node_stream_iter_helpers.rs
rg -rn --iglob '*take*' -l test-parity/node-suite/stream 2>/dev/null || fd -i take test-parity/node-suite/streamRepository: PerryTS/perry
Length of output: 2507
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- node_stream_iter_helpers.rs relevant section ---\n'
sed -n '900,1105p' crates/perry-runtime/src/node_stream_iter_helpers.rs | cat -n -v
printf '\n--- take tests ---\n'
for f in test-parity/node-suite/stream/iter-helpers/take.ts \
test-parity/node-suite/stream/iter-helpers/take-zero.ts \
test-parity/node-suite/stream/iter-helpers/take-then-filter.ts \
test-parity/node-suite/stream/iter-helpers/take-then-foreach.ts \
test-parity/node-suite/stream/readable/from-infinite-take.ts; do
if [ -f "$f" ]; then
echo "### $f"
sed -n '1,220p' "$f"
fi
done
printf '\n--- source pull count assertions ---\n'
rg -n "count|reads|next\\(|return\\(" test-parity/node-suite/stream/iter-helpers/take.ts test-parity/node-suite/stream/iter-helpers/take-zero.ts test-parity/node-suite/stream/iter-helpers/take-then-filter.ts test-parity/node-suite/stream/iter-helpers/take-then-foreach.ts test-parity/node-suite/stream/readable/from-infinite-take.tsRepository: PerryTS/perry
Length of output: 242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- node_stream_iter_helpers.rs relevant section ---'
sed -n '900,1105p' crates/perry-runtime/src/node_stream_iter_helpers.rs | cat -n -v
echo
echo '--- take tests ---'
for f in test-parity/node-suite/stream/iter-helpers/take.ts \
test-parity/node-suite/stream/iter-helpers/take-zero.ts \
test-parity/node-suite/stream/iter-helpers/take-then-filter.ts \
test-parity/node-suite/stream/iter-helpers/take-then-foreach.ts \
test-parity/node-suite/stream/readable/from-infinite-take.ts; do
if [ -f "$f" ]; then
echo "### $f"
sed -n '1,220p' "$f"
fi
done
echo
echo '--- source pull count assertions ---'
rg -n "count|reads|next\\(|return\\(" test-parity/node-suite/stream/iter-helpers/take.ts \
test-parity/node-suite/stream/iter-helpers/take-zero.ts \
test-parity/node-suite/stream/iter-helpers/take-then-filter.ts \
test-parity/node-suite/stream/iter-helpers/take-then-foreach.ts \
test-parity/node-suite/stream/readable/from-infinite-take.ts || trueRepository: PerryTS/perry
Length of output: 11525
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- search for source-read-count assertions in repo ---'
rg -n "count ==|next\\(\\)|extra|read|source|take\\(0\\)|infinite" test-parity crates -g '*.ts' -g '*.rs' || true
echo
echo '--- read relevant StreamNodeIterable source wrappers ---'
rg -n "READABLE_SOURCE_ITERATOR_KEY|readable_source_iterator|async_iterator::call_source_iterator_return|js_native_call_method\\(" crates/perry-runtime/src node_stream_iter_helpers.rs || true
sed -n '1,180p' crates/perry-runtime/src/async_iterator.rs | catsed 1 180 || sed -n '1,180p' crates/perry-runtime/src/async_iterator.rsRepository: PerryTS/perry
Length of output: 50370
🌐 Web query:
Node.js Readable.prototype.take implementation iterator next return behavior take(0)
💡 Result:
In Node.js, the Readable.prototype.take(limit) method is an iterator helper that consumes a specified number of items from a readable stream and then closes the underlying stream [1][2]. When take(0) is called, the method immediately closes the stream without consuming any items from it. Because the take operator is designed to terminate the stream once the specified limit is reached—or immediately if the limit is 0—the internal iterator's next() method, when called subsequently, will return { done: true } [2]. The return behavior of the underlying iterator in this scenario involves triggering the stream's destruction or closure, ensuring that resources are cleaned up once the "take" operation is complete [2]. Technical notes regarding take: - Implementation: The take method is part of Node.js's alignment with the ECMAScript iterator helpers proposal [1][2]. - Stream Closure: A significant aspect of this implementation is that take destroys the source stream by default upon completion [2]. This behavior has been a point of discussion in the Node.js community, with some users seeking options to keep the stream open (e.g., for further consumption after a "take" operation), though standard take semantics mandate closure [3][2]. - Error Handling: Users have occasionally encountered AbortError issues when using take in certain patterns, which often stems from this forced stream closure after the requested number of items has been processed [4].
Citations:
- 1: nodejs/node@9b1a37979c
- 2: stream: Add option to
Readable.takeoperator to not close the stream nodejs/node#46980 - 3: stream: allow stream to stay open after take nodejs/node#47023
- 4: Stream: Error trying to paginate data using Readable.take operator nodejs/node#44026
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- inspect async_iterator source return ---'
rg -n "call_source_iterator_return|fn call_source_iterator_return|source_iterator_return|READABLE_SOURCE_ITERATOR_KEY|READABLE_SOURCE_ITERATOR" crates/perry-runtime/src
echo
echo '--- read async_iterator.rs around return implementations ---'
file=$(rg -l "call_source_iterator_return\(" crates/perry-runtime/src | head -n1)
if [ -n "$file" ]; then
line=$(rg -n "fn call_source_iterator_return|call_source_iterator_return" "$file" | head -n1 | cut -d: -f1)
start=$((line-30)); [ "$start" -lt 1 ] && start=1
end=$((line+120))
sed -n "${start},${end}p" "$file" | cat -n
fi
echo
echo '--- read node_stream_iter_helpers around creation and stream state ---'
sed -n '800,900p' crates/perry-runtime/src/node_stream_iter_helpers.rs | cat -n -v
sed -n '1320,1470p' crates/perry-runtime/src/node_stream_iter_helpers.rs | cat -n -v || trueRepository: PerryTS/perry
Length of output: 15191
Don’t pull the source after reaching the take limit.
ns_take_source_next still calls the source .next() when remaining <= 0, then just discards the response. That can observe one extra pull for shared/side-effecting streams and can also run once for take(0); Node’s iterator helper closes the source after the requested chunks without consuming an extra value.
🤖 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/node_stream_iter_helpers.rs` around lines 1021 -
1043, Update ns_take_source_next to check remaining <= 0 before invoking the
source iterator’s next operation, including take(0), and immediately
finish/close the source and return the done result. Preserve the existing
promise-resolution handling only for pulls already initiated before the limit
was reached, without performing or discarding an extra source pull.
| } else { | ||
| crate::node_submodules::diagnostics::schedule_uncaught(exc); | ||
| ran += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep run --pattern 'fn schedule_uncaught($$$) { $$$ }' --lang rust crates/perry-runtime/src
rg -nP -C6 'fn diagnostics_channel_drain_uncaught' crates/perry-runtime/srcRepository: PerryTS/perry
Length of output: 1361
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== microtasks around the call =="
sed -n '210,255p' crates/perry-runtime/src/promise/microtasks.rs
echo "== diagnostics queue declaration and emit_process_event =="
sed -n '730,790p' crates/perry-runtime/src/node_submodules/diagnostics.rs
rg -n 'fn emit_process_event|fn run_microtasks|pub fn run.*microtasks|DIAG_PENDING_UNCAUGHT' crates/perry-runtime/src -S
echo "== GC/rooting references for NaN-boxed values =="
rg -n 'NaN-box|NaN box|nan_box|unboxed|F64|rooted|root\\(|handle|diagnostics_channel_drain_uncaught|emit_process_event' crates/perry-runtime/src crates/perry -S | head -n 200Repository: PerryTS/perry
Length of output: 5338
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== microtasks loop continuation =="
sed -n '255,360p' crates/perry-runtime/src/promise/microtasks.rs
echo "== emit_process_event implementation =="
sed -n '300,380p' crates/perry-runtime/src/os/os_process_emitter.rs
echo "== run_microtasks callers and diagnostics drain integration =="
rg -n 'run_microtasks|diagnostics_channel_drain_uncaught|try_catch|catch_js|emit_process_event\\(' crates/perry-runtime/src -S
echo "== value handling around f64 exception args and rooting APIs =="
rg -n 'js_value|new_unary|f64|root\\(|Rooted|Handle|JSValue|nan_box|boxed_ptr|unboxed|Boxed|HandleScope|FunctionCallback|Arguments' crates/perry-runtime/src -S | head -n 250
echo "== diagnostic pending declaration =="
sed -n '260,295p' crates/perry-runtime/src/node_submodules/diagnostics.rsRepository: PerryTS/perry
Length of output: 9422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== microtasks continuation and exception handling boundary =="
sed -n '300,520p' crates/perry-runtime/src/promise/microtasks.rs
sed -n '520,620p' crates/perry-runtime/src/promise/microtasks.rs
echo "== emit_process_event implementation =="
sed -n '300,370p' crates/perry-runtime/src/os/os_process_emitter.rs
sed -n '370,440p' crates/perry-runtime/src/os/os_process_emitter.rs
echo "== diagnostics pending declaration =="
sed -n '260,295p' crates/perry-runtime/src/node_submodules/diagnostics.rs
sed -n '828,845p' crates/perry-runtime/src/node_submodules/diagnostics.rs
echo "== run_microtasks/drain integration =="
rg -n 'run_microtasks|diagnostics_channel_drain_uncaught|try_catch|catch_js|emit_process_event\(' crates/perry-runtime/src -S
echo "== rooting and value APIs =="
rg -n 'RuntimeHandleScope|root_raw_mut_ptr|js_value|new_unary|JSValue::|unboxed|nan_box|boxed_ptr|HandleScope|Handle|NaN-box|NaN box' crates/perry-runtime/src crates/perry -S | head -n 300Repository: PerryTS/perry
Length of output: 50369
Root the uncaught exception before storing it for diagnostics drain
schedule_uncaught(exc) stores the exception only as f64 in DIAG_PENDING_UNCAUGHT, then diagnostics_channel_drain_uncaught() runs later and passes it to emit_process_event("uncaughtException", &[err]). This is exactly the “local Rust f64 across allocating JS” case, so the error object may move or be collected before delivery. Root/reload the value in schedule_uncaught()/diagnostics_channel_drain_uncaught() before passing it to the event emitter, instead of keeping the moved error in a plain Vec<f64>.
🤖 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/microtasks.rs` around lines 237 - 241,
Update schedule_uncaught and diagnostics_channel_drain_uncaught so pending
uncaught exceptions remain rooted across the deferred diagnostics delivery,
replacing the plain Vec<f64> storage with the runtime’s rooted or reloadable
value mechanism. Ensure the drained value is reloaded into a valid JS handle
before passing it to emit_process_event("uncaughtException", …), while
preserving the existing scheduling and drain behavior.
Source: Learnings
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/perry-runtime/src/node_stream_pipeline.rs (1)
558-574: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winConfirm the no-op
errorguards can't mask errors past collection.These listeners are not removed until the streams are destroyed, so later
errorevents on those stages can be dropped silently. Since collection completes before failures, propagate or remove these guards at pipeline completion/removal.🤖 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/node_stream_pipeline.rs` around lines 558 - 574, The no-op listeners installed by install_collected_pipeline_error_guards must not suppress errors after pipeline collection completes. Update the pipeline completion or removal path to propagate later stage errors or remove these guards when collection finishes, ensuring the existing guard behavior remains limited to the collection period.crates/perry-runtime/src/node_stream_readwrite.rs (1)
1499-1504: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winVerify
read()andreadableLengthdrain the lazy source.
normalize_readable_from_inputnow returns empty chunks plussource_iteratorforSymbol.asyncIterator, andcollect_pipeline_chunks/node_stream_iterator_nextdrain it, butjs_node_stream_readable_lengthand theread()path still usehidden_buffered_key()/ visible chunks directly, so those can observe0/nullfor async-iterator sources. Applycollect_pipeline_chunksbefore exposing buffered chunks/length, or handleREADABLE_SOURCE_ITERATOR_KEYin those paths.🤖 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/node_stream_readwrite.rs` around lines 1499 - 1504, Update the read() implementation and js_node_stream_readable_length to drain READABLE_SOURCE_ITERATOR_KEY via collect_pipeline_chunks before inspecting hidden_buffered_key() or visible chunks, ensuring async-iterator-backed streams expose available data instead of empty/null results.
🧹 Nitpick comments (2)
crates/perry-runtime/src/array/iterator.rs (1)
991-994: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the async iterator helper.
call_symbol_async_iterator_implis only called bycall_symbol_async_iterator, so the private helper can be inlined into the public API.🤖 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/array/iterator.rs` around lines 991 - 994, Inline the logic of call_symbol_async_iterator_impl into call_symbol_async_iterator and remove the now-unused private helper. Preserve the existing Symbol.asyncIterator resolution and invocation behavior.crates/perry-runtime/src/node_stream/async_iterator.rs (1)
881-883: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the existing
readable_from_chunkshelper.Line 798 in this same module already uses
readable_from_chunks, which is exactlyjs_node_stream_readable_from(box_pointer(chunks as *const u8)).♻️ Proposed tidy-up
- let chunks = crate::array::js_array_alloc(0); - let stream = js_node_stream_readable_from(box_pointer(chunks as *const u8)); + let stream = readable_from_chunks(crate::array::js_array_alloc(0)); let iterator = build_readable_async_iterator(stream, true);🤖 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/node_stream/async_iterator.rs` around lines 881 - 883, Update the setup around build_readable_async_iterator to reuse the existing readable_from_chunks helper instead of manually allocating chunks and calling js_node_stream_readable_from; preserve the current empty-stream behavior and iterator construction.
🤖 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/node_stream_pipeline.rs`:
- Around line 1237-1268: Ensure __perryStreamComposePriming is cleared when
prime_composed_duplex_from_source unwinds through an exception, not only on
normal return. Add a scoped drop guard around the priming call that resets the
flag, using a rooted handle if the composite may move; preserve the existing
pending-error handling and normal-path reset without leaving the flag set.
In `@crates/perry-runtime/src/node_stream_tests_extra.rs`:
- Around line 923-932: Complete RuntimeHandleScope rooting in
crates/perry-runtime/src/node_stream_tests_extra.rs:923-932 by rooting closure
and stream and refreshing them before raw-handle calls; at 1044-1059 root the
error listener and event string before deriving handle; at 1108-1152 apply the
same ordering to stream and ended; and at 1161-1194 root both listeners and
reload each stream before deriving its handle. Do not retain stream or obj raw
addresses across allocations.
---
Outside diff comments:
In `@crates/perry-runtime/src/node_stream_pipeline.rs`:
- Around line 558-574: The no-op listeners installed by
install_collected_pipeline_error_guards must not suppress errors after pipeline
collection completes. Update the pipeline completion or removal path to
propagate later stage errors or remove these guards when collection finishes,
ensuring the existing guard behavior remains limited to the collection period.
In `@crates/perry-runtime/src/node_stream_readwrite.rs`:
- Around line 1499-1504: Update the read() implementation and
js_node_stream_readable_length to drain READABLE_SOURCE_ITERATOR_KEY via
collect_pipeline_chunks before inspecting hidden_buffered_key() or visible
chunks, ensuring async-iterator-backed streams expose available data instead of
empty/null results.
---
Nitpick comments:
In `@crates/perry-runtime/src/array/iterator.rs`:
- Around line 991-994: Inline the logic of call_symbol_async_iterator_impl into
call_symbol_async_iterator and remove the now-unused private helper. Preserve
the existing Symbol.asyncIterator resolution and invocation behavior.
In `@crates/perry-runtime/src/node_stream/async_iterator.rs`:
- Around line 881-883: Update the setup around build_readable_async_iterator to
reuse the existing readable_from_chunks helper instead of manually allocating
chunks and calling js_node_stream_readable_from; preserve the current
empty-stream behavior and iterator construction.
🪄 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: f6bdac5f-6513-4236-9060-a32dde36fdcc
📒 Files selected for processing (13)
crates/perry-codegen/src/lower_call/options/fetch.rscrates/perry-codegen/src/runtime_decls/strings_part2.rscrates/perry-runtime/src/array/iterator.rscrates/perry-runtime/src/array/mod.rscrates/perry-runtime/src/gc/tests/runtime_roots/callback_scanners.rscrates/perry-runtime/src/node_stream/async_iterator.rscrates/perry-runtime/src/node_stream_iter_helpers.rscrates/perry-runtime/src/node_stream_pipeline.rscrates/perry-runtime/src/node_stream_readwrite.rscrates/perry-runtime/src/node_stream_tests_extra.rscrates/perry-runtime/src/node_submodules/mod.rscrates/perry-runtime/src/promise/async_step.rscrates/perry-transform/src/generator/per_iteration.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/perry-transform/src/generator/per_iteration.rs
| set_hidden_value( | ||
| composite.get_nanbox_f64(), | ||
| hidden_key(b"__perryStreamComposePriming"), | ||
| f64::from_bits(TAG_TRUE), | ||
| ); | ||
| prime_composed_duplex_from_source( | ||
| composite.get_nanbox_f64(), | ||
| source.get_nanbox_f64(), | ||
| stages_value.get_nanbox_f64(), | ||
| ); | ||
| set_hidden_value( | ||
| composite.get_nanbox_f64(), | ||
| hidden_key(b"__perryStreamComposePriming"), | ||
| f64::from_bits(TAG_FALSE), | ||
| ); | ||
| if let Some(err) = get_hidden_value( | ||
| composite.get_nanbox_f64(), | ||
| hidden_key(b"__perryStreamComposePendingError"), | ||
| ) { | ||
| set_hidden_value( | ||
| composite.get_nanbox_f64(), | ||
| hidden_key(b"__perryStreamComposePendingError"), | ||
| f64::from_bits(TAG_UNDEFINED), | ||
| ); | ||
| fail_composed_duplex( | ||
| composite.get_nanbox_f64(), | ||
| source.get_nanbox_f64(), | ||
| stages_value.get_nanbox_f64(), | ||
| err, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
__perryStreamComposePriming is leaked if priming unwinds.
prime_composed_duplex_from_source calls prepare_readable_for_iteration, compose_stage_values, and schedule_readable_end outside any catch_pipeline_throw. If any of those throws, the reset at Line 1247 never runs and the composite stays flagged as priming forever — from then on every fail_composed_duplex parks the error in __perryStreamComposePendingError and nothing ever drains it, so the composed duplex silently swallows all future errors instead of destroying.
Clear the flag from a drop guard (or reset it at the top of the pending-error read) so the invariant holds on the abrupt path too.
🛡️ Sketch: reset via a guard
+ // Reset on both the normal and unwinding paths so a throw during
+ // priming cannot leave the composite permanently "priming".
+ struct PrimingGuard(f64);
+ impl Drop for PrimingGuard {
+ fn drop(&mut self) {
+ set_hidden_value(
+ self.0,
+ hidden_key(b"__perryStreamComposePriming"),
+ f64::from_bits(TAG_FALSE),
+ );
+ }
+ }
set_hidden_value(
composite.get_nanbox_f64(),
hidden_key(b"__perryStreamComposePriming"),
f64::from_bits(TAG_TRUE),
);
+ let _priming_guard = PrimingGuard(composite.get_nanbox_f64());
prime_composed_duplex_from_source(
composite.get_nanbox_f64(),
source.get_nanbox_f64(),
stages_value.get_nanbox_f64(),
);
- set_hidden_value(
- composite.get_nanbox_f64(),
- hidden_key(b"__perryStreamComposePriming"),
- f64::from_bits(TAG_FALSE),
- );
+ drop(_priming_guard);Note the guard holds a raw NaN-boxed f64; if the composite can move during priming, capture it as a rooted handle instead and reload in drop.
🤖 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/node_stream_pipeline.rs` around lines 1237 - 1268,
Ensure __perryStreamComposePriming is cleared when
prime_composed_duplex_from_source unwinds through an exception, not only on
normal return. Add a scoped drop guard around the priming call that resets the
flag, using a rooted handle if the composite may move; preserve the existing
pending-error handling and normal-path reset without leaving the flag set.
| let scope = crate::gc::RuntimeHandleScope::new(); | ||
| let opts = scope.root_raw_mut_ptr(crate::object::js_object_alloc(0, 1)); | ||
| let closure = js_closure_alloc(write_capture as *const u8, 0); | ||
| crate::closure::js_register_closure_arity(write_capture as *const u8, 3); | ||
| js_object_set_field_by_name( | ||
| opts.get_raw_mut_ptr(), | ||
| hidden_key(b"write"), | ||
| f64::from_bits(JSValue::pointer(closure as *const u8).bits()), | ||
| ); | ||
| let stream = js_node_stream_writable_new(box_pointer(opts.get_raw_const_ptr())); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Complete RuntimeHandleScope use across these stream tests. Allocated strings can relocate managed values; closures, streams, event values, and derived raw handles must stay rooted and be refreshed before raw-handle calls.
crates/perry-runtime/src/node_stream_tests_extra.rs#L923-L932: rootclosureandstream; do not retainstream/objraw addresses across later allocations.crates/perry-runtime/src/node_stream_tests_extra.rs#L1044-L1059: root the error listener and event string; derivehandleafter them.crates/perry-runtime/src/node_stream_tests_extra.rs#L1108-L1152: apply the same ordering to bothstreamandended.crates/perry-runtime/src/node_stream_tests_extra.rs#L1161-L1194: root both listeners and reload each stream before deriving its handle.
📍 Affects 1 file
crates/perry-runtime/src/node_stream_tests_extra.rs#L923-L932(this comment)crates/perry-runtime/src/node_stream_tests_extra.rs#L1044-L1059crates/perry-runtime/src/node_stream_tests_extra.rs#L1108-L1152crates/perry-runtime/src/node_stream_tests_extra.rs#L1161-L1194
🤖 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/node_stream_tests_extra.rs` around lines 923 - 932,
Complete RuntimeHandleScope rooting in
crates/perry-runtime/src/node_stream_tests_extra.rs:923-932 by rooting closure
and stream and refreshing them before raw-handle calls; at 1044-1059 root the
error listener and event string before deriving handle; at 1108-1152 apply the
same ordering to stream and ended; and at 1161-1194 root both listeners and
reload each stream before deriving its handle. Do not retain stream or obj raw
addresses across allocations.
Source: Learnings
1 participant
Summary
Completes
node:streamcompatibility with Node.js 26.5.0 across classic streams, iterator helpers, pipeline/compose, and Web Streams. The changes address shared runtime causes rather than individual fixtures and bring the complete 801-case parity module to Node-observable behavior.Changes
Readable.from(), pipeline, and compose lifecycle/error semantics with Node.js 26.5.0.prevent*propagation, locks, and microtask ordering.NODE_FAIL.cancel-during-pipetosettle its active pipeline deterministically without weakening its locked-stream assertion.Related issue
Closes #6774
Test plan
cargo build --release -p perry -p perry-runtime -p perry-stdlib -p perry-runtime-static -p perry-stdlib-staticcargo test -p perry-runtime node_stream --lib -- --test-threads=1— 83 passedcargo test -p perry-runtime promise --lib -- --test-threads=1— 71 passed, 1 pre-existing ignoredcargo test -p perry-stdlib streams:: --lib -- --test-threads=1— 17 passedcargo test -p perry-codegen --lib -- --test-threads=1— 282 passedcargo test -p perry-transform --lib -- --test-threads=1— 55 passedshellcheck -S warning run_parity_tests.shcargo test --workspace --exclude perry-ui-ios --exclude perry-ui-tvos --exclude perry-ui-watchos --exclude perry-ui-gtk4 --exclude perry-ui-android --exclude perry-ui-windows— intentionally not run; validation was scoped to affected crates and the task explicitly prohibited workspace-wide testsScreenshots / output
Review scope
This is intentionally a single compatibility PR: the runtime, codegen, stdlib, and parity-runner changes are coupled by the same Node 26.5 oracle and were validated as one 801-fixture gate. The maintainer requested publication after reviewing the full 2,454-line diff summary.
Checklist
feat:/fix:/docs:/chore:prefix convention used in the logSummary by CodeRabbit
pipeThroughsupport with options andAbortSignalvalidation, and more consistent piping behavior.take()behavior with retained sources.Readable.from()input validation and clearer error descriptions for Promise values.@@asyncIteratorresolution behavior.