Skip to content

fix(stream): flush piped transforms before end - #11037

Closed
proggeramlug wants to merge 2 commits into
mainfrom
fix/10450-transform-pipe-flush
Closed

proggeramlug wants to merge 2 commits into
mainfrom
fix/10450-transform-pipe-flush

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • finalize zero-length pipe destinations in the source end turn so transforms run their flush hook before a queued readable drain can emit end
  • route delayed pipe completion through the same transform flush path after buffered writes finish
  • keep readable draining from ending a transform while an asynchronous flush callback is pending
  • add runtime and compiled TypeScript regressions for direct, piped, and asynchronous piped flush output

Fixes #10450

Testing

  • cargo test --release -p perry-runtime piped_transform_flush_callback_pushes_tail_before_finish -- --nocapture
  • compiled test_gap_10450_transform_pipe_flush.ts with the coherent release compiler/runtime archive set and compared its output byte-for-byte with Node 26.5.1
  • cargo fmt --all -- --check
  • git diff --check
  • scripts/check_file_size.sh
  • python3 scripts/raw_handle_debt.py --check

Summary by CodeRabbit

  • Bug Fixes
    • Fixed piped Transform streams so flush output is emitted before the stream’s end event.
    • Ensured asynchronous flush callbacks complete before finishing the destination stream.
    • Preserved the correct order of input data and flush-generated output in direct and piped scenarios.
  • Tests
    • Added coverage for synchronous and asynchronous transform flushing, including piped streams.

@coderabbitai

coderabbitai Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 9d69be6e-d6c8-4255-91a2-47db46ef2b37

📥 Commits

Reviewing files that changed from the base of the PR and between f5cfbff and 37946c9.

📒 Files selected for processing (5)
  • changelog.d/transform-pipe-flush.md
  • crates/perry-runtime/src/node_stream.rs
  • crates/perry-runtime/src/node_stream_readwrite.rs
  • crates/perry-runtime/src/node_stream_tests.rs
  • test-files/test_gap_10450_transform_pipe_flush.ts

Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

Piped destinations now run transform flush completion before ordinary stream completion. Readable end emission waits while flush is pending. Tests cover direct, piped, and asynchronous flush callbacks.

Changes

Transform pipe flush completion

Layer / File(s) Summary
Pipe destination transform completion
crates/perry-runtime/src/node_stream.rs, crates/perry-runtime/src/node_stream_readwrite.rs
Pipe destination finishing now checks stream state, runs finish_transform_stream before finish_stream, and performs the empty-writable-length path synchronously. Readable end emission is suppressed while transform finishing is pending.
Transform flush regression coverage
crates/perry-runtime/src/node_stream_tests.rs, test-files/test_gap_10450_transform_pipe_flush.ts, changelog.d/transform-pipe-flush.md
Tests verify flush output ordering, single flush invocation, stream state, direct and piped usage, and asynchronous flush callbacks. The changelog records the corrected behavior.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant PassThrough
  participant Transform
  participant finish_pipe_destination
  participant finish_transform_stream
  PassThrough->>finish_pipe_destination: Request destination finish
  finish_pipe_destination->>finish_transform_stream: Complete transform flush
  finish_transform_stream->>Transform: Push flush output
  finish_transform_stream-->>finish_pipe_destination: Report completion
  finish_pipe_destination->>Transform: Complete stream
  Transform-->>PassThrough: Emit flush output before end
Loading

Merge Risk: ⚪ Minimal · up to 37946

The pipe flush ordering change is covered for synchronous and asynchronous cases, with no remaining merge-blocking risk identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 4 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary change: flushing piped transforms before the stream emits end.
Description check ✅ Passed The description explains the fix, lists the main changes, references issue #10450, and provides concrete validation commands and comparisons. It omits the exact Changes, Test plan, and Checklist headi…
Linked Issues check ✅ Passed The changes satisfy #10450. Pipe destination completion now calls finish_transform_stream(dest, None) before plain finish_stream, so _flush and flush output use the same completion path as dir…
Out of Scope Changes check ✅ Passed The changed runtime code, runtime regression, TypeScript regression, and changelog entry directly support #10450. No unrelated behavior or files are shown.
Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 4 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

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

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Landed on main in merge train 257 (#11039, v0.5.1640), main 990b3eeada.

Carried at head 37946c9e66. CI on the train head passed every job except the known public-baseline lint step: all 6 gap shards, cargo-test, e2e-scoped, gc-stress, check, warnings and security-audit green.

This train was split by blast radius after an earlier 35-PR assembly hit five gap regressions: it carries only PRs touching no lowering path. Trains rebase-merge, so commits get new SHAs and GitHub cannot mark this merged. Closed as landed.

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.

Transform flush output is dropped when the Transform is a pipe() destination (_flush / options.flush)

1 participant