Skip to content

fix(stream): node v26 read() chunk-at-a-time, no spurious EOF readable, pipe dedup#5096

Merged
proggeramlug merged 1 commit into
mainfrom
fix/stream-read-pipe-dup-diffs
Jun 13, 2026
Merged

fix(stream): node v26 read() chunk-at-a-time, no spurious EOF readable, pipe dedup#5096
proggeramlug merged 1 commit into
mainfrom
fix/stream-read-pipe-dup-diffs

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes the largest real-gap cluster in test-parity/node-suite/stream by matching node v26's internal/streams/readable byte-for-byte. Baseline (pristine origin/main, node v26.3.0): 758 pass / 10 real fail (after excluding NODE-FAIL tests where node itself throws). This fixes 6 of the 10 via three distinct root causes; the remaining 4 are a single separate codegen gap (see below).

Root causes & fixes

1. read() with no size returns the head chunk, not the whole buffer.
node v26 howMuchToRead(NaN) has a fast path: without a string decoder it always returns state.buffer[bufferIndex].length (one buffer at a time), even when paused. Only a setEncoding (decoded) paused stream concatenates the whole buffer into one string. Perry drained the entire buffer whenever paused.
→ fixes readable/readable-mode-only, readable/read-in-readable-listener, readable/unshift-after-read, readable/iter-yields-buffer-no-encoding

# readable-mode-only, before → after
- drained: aabb
+ drained: aa|bb

2. objectMode read() of the last item no longer emits a final readable.
node never emits readable just to deliver a null at EOF — after the last item of an ended stream it goes straight to end (endReadable). The extra emit made a fixed-count consumer observe a spurious read()===null pair.
→ fixes readable/object-mode-read-returns-object

  first: {"a":1}
  second: {"b":2}
- first: null
- second: null

3. pipe() destinations now consume-on-emit.
A flowing destination (piped-into PassThrough/Duplex) must drop each chunk from its own readable buffer when it emits data live; otherwise the chunk lingers and the destination's drain microtask re-emits it, duplicating every piped chunk. pipeline() already marks both ends via mark_live_pipe_consume_on_emit; pipe() now marks the destination too.
→ fixes pipe/repipe-after-unpipe

- joined: after-repipeafter-repipe
+ joined: after-repipe

Also updates two perry-runtime unit tests (node_stream_tests_extra.rs) that asserted the pre-v26 drain-whole behavior; verified against node v26, read() after unshift returns the unshifted head chunk and leaves the remainder.

Verification (node v26.3.0, macOS)

  • The 6 target tests pass.
  • Focused 232-test regression over the read/pipe/objectMode/flowing/transform/duplex/consumers surface: 226 pass, 6 node-fail, 0 compile-fail, 0 diffs — no new regressions.
  • Cross-module: pipe fan-out (one→two sinks) and chained pipes match node exactly.
  • cargo test -p perry-runtime --lib: green (the Date.prototype global-state test passes in isolation — a pre-existing flake unrelated to this change).

A full single-machine harness sweep was not feasible during this run because the shared host was at ~6× CPU load from concurrent builds; the full stream/http/etc. sweep runs in CI.

Not addressed — separate codegen gap

subclass/extends-{readable,writable,duplex,transform} still throw TypeError: Class extends value is not a constructor. User-defined class X extends Readable requires native-stream subclassing support in codegen (synthesized super() initialising native stream state on this, routing push()/_read to the user object) — a much larger, independent change, not a stream-runtime bug.

Summary by CodeRabbit

  • Bug Fixes

    • Improved stream reading behavior for better Node.js v26 compatibility
    • Fixed duplicate chunk re-emission when piping to stream destinations
    • Refined EOF signaling to prevent extra read observations
  • Tests

    • Updated tests to verify Node v26 stream reading behavior

…e, pipe dedup

Closes the largest real-gap cluster in test-parity/node-suite/stream by
matching node v26's internal/streams/readable byte-for-byte. Three distinct
root causes (6 previously-failing node-suite tests):

1. read() with no size returns the HEAD chunk, not the whole buffer.
   node v26 howMuchToRead(NaN) takes a fast path: WITHOUT a string decoder it
   always returns state.buffer[bufferIndex].length (one buffer at a time), even
   when paused. Only a setEncoding (decoded) paused stream concatenates the
   whole buffer into a single string. Perry drained the entire buffer whenever
   paused, so multi-chunk paused read() over-concatenated.
   Fixes: readable-mode-only, read-in-readable-listener, unshift-after-read,
   iter-yields-buffer-no-encoding.

2. objectMode read() of the last buffered item no longer emits a final
   'readable'. node never emits 'readable' just to hand the consumer a null at
   EOF — once read() returns the last item from an ended stream it transitions
   straight to 'end' (endReadable). The extra emit made a fixed-count consumer
   observe a spurious read()===null pair.
   Fixes: object-mode-read-returns-object.

3. pipe() destinations now consume-on-emit. A flowing destination (piped-into
   PassThrough/Duplex) must drop each chunk from its own readable buffer when it
   emits 'data' live; otherwise the chunk lingers and the destination's drain
   microtask re-emits it, duplicating every piped chunk. pipeline() already
   marks both ends via mark_live_pipe_consume_on_emit; pipe() now marks the
   destination too.
   Fixes: pipe/repipe-after-unpipe.

Also updates two perry-runtime unit tests in node_stream_tests_extra.rs that
asserted the pre-v26 drain-whole behavior (read() after unshift returning the
concatenated buffer). Verified against node v26: read() returns the unshifted
head chunk and leaves the remainder, so the tests now assert chunk-at-a-time.

Verification (node v26.3.0, macOS): the 6 target tests pass; a focused
232-test regression over the read/pipe/objectMode/flowing/transform/duplex/
consumers surface is clean (226 pass, 6 node-fail, 0 diffs, 0 new regressions);
pipe fan-out and chained-pipe cross-checks match node. cargo test -p
perry-runtime --lib green (the Date.prototype global-state test passes in
isolation, a pre-existing flake unrelated to this change).

Not addressed (separate codegen gap): subclass/extends-{readable,writable,
duplex,transform} still throw "Class extends value is not a constructor" —
user-defined `class X extends Readable` requires native-stream subclassing
support in codegen (synthesized super() initialising native stream state on
`this`, routing push()/_read to the user object), which is a much larger,
independent change.
@coderabbitai

coderabbitai Bot commented Jun 13, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e61d6d35-ced3-423b-9ccb-a7d1f0fdcbe2

📥 Commits

Reviewing files that changed from the base of the PR and between 12ae086 and 1b4dd55.

📒 Files selected for processing (3)
  • crates/perry-runtime/src/node_stream_readable_read.rs
  • crates/perry-runtime/src/node_stream_readwrite.rs
  • crates/perry-runtime/src/node_stream_tests_extra.rs

📝 Walkthrough

Walkthrough

This PR refines Perry's readable stream implementation to align with Node v26 semantics. Buffered chunk draining in paused mode now requires both non-flowing state and active encoding. End-of-stream handling removes duplicate readable events. Pipe destination streams are explicitly marked for live consumption. Tests validate chunk-at-a-time read behavior.

Changes

Stream Read and Pipe Behavior

Layer / File(s) Summary
Readable stream buffer drain and EOF behavior
crates/perry-runtime/src/node_stream_readable_read.rs, crates/perry-runtime/src/node_stream_tests_extra.rs
Buffer drain condition now requires both non-flowing state and readable encoding tag; EOF handling removes duplicate queue_readable_event call, leaving only schedule_readable_end. Two tests updated to verify chunk-at-a-time read() returns head chunk first after unshift, consistent with Node v26.
Pipe destination consumption marking
crates/perry-runtime/src/node_stream_readwrite.rs
pipe_stream_to_destination adds mark_live_pipe_consume_on_emit(dest) before pipe listener installation to ensure flowing piped destinations consume readable buffers on data emission, preventing duplicate re-emission.

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐇 A stream flows true, chunk by chunk we read,
No drain unless decoder plants the seed,
At EOF's edge, no double-event spree,
Piped paths consume what they should see! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main changes: chunk-at-a-time read() behavior, EOF readable event fix, and pipe deduplication.
Description check ✅ Passed The description is comprehensive and follows the template structure with Summary, Changes, Related issue, Test plan, and Checklist sections clearly addressed.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/stream-read-pipe-dup-diffs

Comment @coderabbitai help to get the list of available commands and usage tips.

@proggeramlug proggeramlug merged commit 84d59a7 into main Jun 13, 2026
14 checks passed
@proggeramlug proggeramlug deleted the fix/stream-read-pipe-dup-diffs branch June 13, 2026 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant