Fix NodeStream.toString registers duplicate error event listener - #7143
Conversation
fubhy
added
bug
🦋 Changeset detectedLatest commit: 411d397 The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
effect-slopcop
Bot
added
the
3.0
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This PR fixes a duplicate error event listener registration in NodeStream.toString by removing the second stream.once("error", ...) call, matching the implementation of toArrayBuffer. It also adds a focused regression test that verifies exactly one error listener is registered.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
fubhy
added
4.0
slop
and removed
3.0
f7594e3 to
a27bb41
Compare
There was a problem hiding this comment.
Caution
The latest commit reverts the source fix from the prior review, so NodeStream.toString again registers two error listeners and the newly added regression test fails. Do not merge as-is.
Reviewed changes
Since the prior Pullfrog review at f7594e3, the branch was rewritten:
- Reverted the
NodeStream.tsfix; the duplicatestream.once("error", ...)registration is back. - Removed the
.changeset/sweet-lizards-sing.mdpatch changeset. - Rewrote the regression test in
NodeStream.test.tsto counterrorlisteners on the evaluated stream instance rather than monkey-patchingReadable.prototype.once.
🚨 PR title and commit message no longer match the change
The PR title and commit a27bb41 say "Fix duplicate NodeStream error listener", but packages/platform-node-shared/src/NodeStream.ts is unchanged from main and still contains two stream.once("error", ...) registrations in toString (src/NodeStream.ts:235-238). The PR body correctly describes the change as test-only, yet the commit message claims a fix. Running the reproduction command from the body confirms the test fails with expected 2 to equal 1.
To land this safely, either restore the implementation fix (remove the duplicate listener and re-add a patch changeset) or align the title/commit with the test-only intent and do not merge until a separate fix is in place.
Technical details
# Reverted fix and failing regression test
## Affected sites
- `packages/platform-node-shared/src/NodeStream.ts:235-238` — `toString` registers `error` twice (pre-existing bug, unchanged by this PR).
- `packages/platform-node-shared/test/NodeStream.test.ts:174-191` — new regression test asserts exactly one `error` listener; it fails because the source still registers two.
- `.changeset/sweet-lizards-sing.md` — removed in the latest commit.
## Required outcome
- The duplicate `stream.once("error", ...)` registration must be removed before this PR can pass CI.
- If this PR is meant to be test-only, the title and commit message must not claim a fix, and the branch should not be merged while CI is red.
## Suggested approach
- Remove the second `stream.once("error", ...)` block in `toString` so the implementation matches `toArrayBuffer`.
- Re-add a patch changeset for `@effect/platform-node-shared`.
- Keep the improved regression test, which is more isolated than the `Readable.prototype.once` version reviewed previously.@v0 or keep the SHA fresh with Dependabot ｜ Fix it ➔ ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior Pullfrog review at a27bb41, the branch now restores the implementation fix and aligns with the PR title:
- Restored the
NodeStream.toStringfix, removing the duplicatestream.once("error", ...)registration sotoStringnow registers a singleerrorlistener, consistent withtoArrayBuffer. - Re-added the
.changeset/sweet-lizards-sing.mdpatch changeset for@effect/platform-node-shared. - Rewrote the regression test to assert
stream.listenerCount("error") === 1on the evaluated stream after a fork + yield, replacing thestream.oncemonkey-patch with a cleaner count-based check.
Verified locally: the fixed source passes the new test (15/15 runs), and reintroducing the duplicate listener makes it fail with expected 2 to equal 1 — so the test is discriminating, not theatre.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
2 participants
Summary
errorlistener fromNodeStream.toString@effect/platform-node-sharedValidation
pnpm test --run packages/platform-node-shared/test/NodeStream.test.tspnpm --filter @effect/platform-node-shared checkoxlintanddprint checkCloses EFF-569