[vropkg] (#839) Fix 'Too many files open' Error on Windows for Package with many Elements#1066
Open
akantchev wants to merge 9 commits intovmware:mainfrom
Open
[vropkg] (#839) Fix 'Too many files open' Error on Windows for Package with many Elements#1066akantchev wants to merge 9 commits intovmware:mainfrom
akantchev wants to merge 9 commits intovmware:mainfrom
Conversation
…nSigned-off-by:akantchev
bcpmihail
reviewed
Mar 18, 2026
added 3 commits
March 23, 2026 16:21
Signed-off-by: akantchev
… a promise\nSigned-off-by:akantchev
Contributor
Author
|
Please add the following labels to this PR: kind/bugarea/vropkglang/typescriptversion/patch |
joroaf
requested changes
Mar 25, 2026
Contributor
joroaf
left a comment
There was a problem hiding this comment.
Required changes (blocking) to address before approval:
-
Fix
finalizeArchive()Promise settling / listener usage- Ensure the Promise settles exactly once (guard against both
finalize()reject + later streamclose, etc.). - Use
once('close' | 'error')instead ofon(...)to avoid accumulating listeners. - Capture/reject on both output stream errors and archiver errors (not just rely on thrown errors elsewhere).
- Ensure the Promise settles exactly once (guard against both
-
Stop
throwing inside stream/event handlers inarchive()- In
packaging.ts, don’tthrowfromoutput.on('error')/instance.on('error'). - Log there, but let
finalizeArchive()be the mechanism that rejects and propagates errors to callers.
- In
-
Replace “sync FS inside Promise” with true async
- In
tree.tsandserialize/util.ts, replacefs.copySync(...)wrapped inPromise.resolve().then(...)withawait fs.copy(...)/fs.copy(...)(orfs.copyFile(...)), so we don’t block and we keep handle usage predictable.
- In
-
Ensure no remaining unsafe direct
.finalize()calls onp.archive()- Repo-wide check: any code doing
p.archive(...).finalize()should be switched top.finalizeArchive(arch)to avoid reintroducing the EMFILE issue.
- Repo-wide check: any code doing
… removed throw within archive() method, added true async operations in file io operations within a promise, updated usage of the finalizeArchive() method\nSigned-off-by:akantchev
Contributor
Author
|
Hi @joroaf, I've addressed the changes you've requested. Thanks for the detailed comments. |
added 3 commits
March 26, 2026 15:20
…ng a release on PR push event\nSigned-Off-By:akantchev
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The
archive()function inpackaging.tswas creating archiver instances without properly managing the output stream lifecycle. When callers usedarchive.finalize(), the Promise returned by finalize() didn't wait for the stream to close, leaving file handles open.Solution:
Added
finalizeArchive()Promise settling and listener usage (packaging.ts)• Added settled flag with settleOnce() guard to prevent multiple Promise settlements
• Changed all on() listeners to once() to prevent listener accumulation
• Added comprehensive error handling for both output stream and archiver errors
• All error paths now provide descriptive messages with output path for debugging
Replaced sync FS with true async operations
• tree.ts: Replaced Promise.resolve().then(() => fs.copySync(...)) with fs.copy(...)
• util.ts: Replaced Promise.resolve().then(() => fs.copySync(...)) with fs.copy(...)
• Both files now use non-blocking async I/O operations
Eliminated all unsafe direct
.finalize()calls• tree.ts: Replaced manual Promise-based archiver code with p.archive() + p.finalizeArchive()
• util.ts: Replaced manual Promise-based archiver code with p.archive() + p.finalizeArchive()
• flat.ts: Already using p.finalizeArchive() correctly (no changes needed)
• All archiver operations now use the centralized, safe finalizeArchive() function
Benefits:
1. Prevents EMFILE errors - Proper async/await ensures file handles are closed before proceeding
2. No unhandled exceptions - Event handlers don't throw, errors propagate through Promises
3. No memory leaks - Using once() prevents listener accumulation
4. No race conditions - Guard against multiple Promise settlements
5. True async I/O - No blocking sync operations disguised as Promises
6. Consistent error handling - All archive operations use the same safe pattern### Checklist
Fixed #XXX -orClosed #XXX -prefix to auto-close the issueTesting
Manual testing is required.
Related issues and PRs
#839