fix: replace @eggjs/yauzl with upstream yauzl 3.4.0, update yazl to 3.3.1 - #145
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review limit reached
Next review available in: 27 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe ZIP uncompression code now uses the ChangesZIP yauzl migration
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #145 +/- ##
==========================================
+ Coverage 97.15% 97.18% +0.02%
==========================================
Files 19 19
Lines 1197 1207 +10
Branches 309 310 +1
==========================================
+ Hits 1163 1173 +10
Misses 34 34 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses Node 26 CI hangs in ZIP extraction by replacing the @eggjs/yauzl fork (and its problematic transitive fd-slicer2 dependency) with upstream yauzl@3.4.0, and adjusting the affected test expectation to be robust to an upstream error-message capitalization change.
Changes:
- Replace
@eggjs/yauzlwith upstreamyauzl@^3.4.0in dependencies. - Update ZIP uncompress implementation to require
yauzlinstead of the fork. - Make the “end of central directory record signature not found” assertion case-insensitive to match upstream message capitalization.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
package.json |
Swaps @eggjs/yauzl for upstream yauzl@^3.4.0 to avoid Node 26 stream hang via transitive deps. |
lib/zip/uncompress_stream.js |
Updates the yauzl import to use the upstream package. |
test/zip/uncompress_stream.test.js |
Makes the ZIP-format error assertion case-insensitive to accommodate upstream message formatting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@package.json`:
- Line 42: Regenerate pnpm-lock.yaml from the updated yauzl dependency
declaration so the lockfile resolves yauzl ^3.4.0 instead of the stale
`@eggjs/yauzl` 2.11.0 entry, while preserving the dependency required by
lib/zip/uncompress_stream.js.
🪄 Autofix
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: 2549f4c0-a10d-4063-8d61-2c032b88573b
📒 Files selected for processing (3)
lib/zip/uncompress_stream.jspackage.jsontest/zip/uncompress_stream.test.js
89baf57 to
ae7e238
Compare
fengmk2
changed the title
The fork depends on fd-slicer2, whose ReadStream loses data when piped on Node.js 26: any zip entry over 64 KiB delivers roughly the first chunk and then stalls with no end, no error, and no close. That is why zip.uncompress() hangs until the test timeout on Node 26 while passing on 18 through 24. Upstream yauzl 3.4.0 dropped fd-slicer entirely, its only dependency now being pend, and does not have the bug. Verified identical behaviour on the contain-absolute-path.zip fixture that motivated the fork in the first place: 31 entries, Buffer fileNames under decodeStrings:false, externalFileAttributes intact, and the leading "/" entry still read. Suite is 171 passing on both Node 24 and Node 26. The only visible difference is that yauzl 3.x capitalises the "end of central directory record signature not found" message, so that assertion is now case-insensitive. Reported upstream at node-modules/yauzl#3.
yazl 3 turns "add entries after calling end()" from a tolerated no-op into a thrown error. compressing hits it because _onEntryFinish() finalizes as soon as the queue is momentarily empty, and for zip the finish callback runs synchronously, so a caller adding entries back to back closed the archive after the first one. yazl 2 accepted the later entries anyway and produced a correct archive, which is why this never surfaced. Finalize on the next tick instead, and skip it if an entry arrived in the meantime. Verified the produced archive still contains every entry.
2078c1a to
35ab10c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
lib/tar/stream.js:138
_onEntryFinish()schedules a newsetImmediate()every time the queue becomes empty. WithZipStream,_onEntryFinish()is called synchronously for eachaddEntry(), so multiple back-to-back entries will enqueue multiple immediates and can call_finalize()more than once (e.g., multiplezipfile.end()calls), which can throw or cause inconsistent stream termination. Coalesce the finalization scheduling so only one pending finalize runs per idle period.
setImmediate(() => {
if (this._processing || this._waitingEntries.length > 0) return;
this._finalize();
});
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
lib/tar/stream.js:138
- The new
setImmediate-based finalize defers closing correctly, but_onEntryFinish()will schedule a newsetImmediateeach time the queue becomes empty. With multiple back-to-back entries, this can result in_finalize()being called multiple times (one per scheduled callback), which may throw or behave unexpectedly (e.g.,yazl.ZipFile#end()/tar-stream#finalize()are not guaranteed to be idempotent). Consider de-duplicating the scheduled finalize and guarding against double-finalization.
setImmediate(() => {
if (this._processing || this._waitingEntries.length > 0) return;
this._finalize();
});
The directory placeholder emitted 'end' from a setImmediate, so the event fired whether or not the consumer had finished with the entry. A listener that creates the directory asynchronously would therefore be handed the next entry, a file inside that directory, before the directory existed, and the write failed with ENOENT. That is the intermittent "ENOENT ... /xxx/bar.txt" seen in test/zip/uncompress_stream.test.js on loaded CI runners. Reproduces every time by delaying the mkdir in the entry handler. Push EOF instead, so 'end' arrives only once the consumer reads or resumes the entry and the ordering no longer depends on timing.
[skip ci] ## <small>2.1.3 (2026-08-05)</small> * fix: replace @eggjs/yauzl with upstream yauzl 3.4.0, update yazl to 3.3.1 (#145) ([572a0ba](572a0ba)), closes [#145](#145) [#140](#140) * chore(deps): update dependency @types/node to v24 (#123) ([3499eb2](3499eb2)), closes [#123](#123) * chore(deps): update dependency iconv-lite to ^0.7.0 (#122) ([e7ba2a5](e7ba2a5)), closes [#122](#122)
|
🎉 This PR is included in version 2.1.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
lib/tar/stream.js:138
_onEntryFinish()schedules asetImmediate()finalization every time the queue is empty. When entries are added back-to-back in the same tick (especially forZipStream, where_onEntryFinish()is synchronous), this can queue multiple finalization callbacks and call_finalize()more than once. Debounce the scheduling so at most one finalize attempt is pending at a time.
setImmediate(() => {
if (this._processing || this._waitingEntries.length > 0) return;
this._finalize();
});
Fixes the Node 26 CI failure on master, and moves both zip dependencies to their upstream, maintained versions.
yauzl: the Node 26 fix
@eggjs/yauzldepends onfd-slicer2, whoseReadStreamloses data when piped on Node 26. Any zip entry over the 64 KiBhighWaterMarkdelivers roughly the first chunk and then stalls, with noend, noerror, noclose. That is whyzip.uncompress()hangs until the 60s timeout on Node 26 while passing on 18 through 24.Not our code: released 2.1.1 reproduces it identically. Reported upstream at node-modules/yauzl#3.
yauzl@3.4.0droppedfd-slicerentirely (only dependency is nowpend) and does not have the bug.The fork was adopted for
decodeStrings: falseso absolute paths survivevalidateFileName. I checked that still holds against thecontain-absolute-path.zipfixture rather than assuming:fileNameis BufferexternalFileAttributespresent/entryOnly visible difference: yauzl 3 capitalises the "End of central directory record signature not found" message, so that assertion is now case-insensitive.
yazl 3 and the early-finalize bug it exposed
yazl 3 turns "add entries after calling
end()" from a tolerated no-op into a thrown error, and compressing trips it immediately._onEntryFinish()finalizes as soon as the entry queue is momentarily empty. For zip the finish callback runs synchronously, so a caller doing:closed the archive after the first entry, and the second threw. Tar avoids it only because its
fs.statmakes the callback async, which lets the later entries queue first.Worth being precise about the old behaviour: yazl 2 did not drop those entries. I checked, and the produced archive contained all of them. So this was latent, not a live data-loss bug.
Fix is to finalize on the next tick and skip it if an entry arrived meanwhile. Verified the produced archive still contains every entry.
Residual limitation, unchanged in spirit from before: entries added after a longer async gap still finalize early. That is the existing drain heuristic, and giving the stream an explicit "done adding" call would be an API change worth doing separately.
Result
171 passing on both Node 24 and Node 26, lint and
tscclean. On Node 26 the zip suite finishes in ~495ms where it previously hung for 60s. The symlink cases from #140 were re-checked through the new zip path and still block.Drops
fd-slicer2andbuffer-crc32from the tree. Supersedes #132.