fix(server): use fs.promises.rm to delete temp upload directories#40755
Merged
pavelfeldman merged 1 commit intomicrosoft:mainfrom May 9, 2026
Merged
fix(server): use fs.promises.rm to delete temp upload directories#40755pavelfeldman merged 1 commit intomicrosoft:mainfrom
pavelfeldman merged 1 commit intomicrosoft:mainfrom
Conversation
_deleteAllTempDirs uses fs.promises.unlink which cannot remove directories (returns EISDIR on Linux, EPERM on Windows). The error is silently caught, so temp upload directories are never cleaned up. Use fs.promises.rm with recursive and force options instead. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
pavelfeldman
approved these changes
May 9, 2026
pavelfeldman
reviewed
May 9, 2026
|
|
||
| private async _deleteAllTempDirs(): Promise<void> { | ||
| await Promise.all(this._tempDirs.map(async dir => await fs.promises.unlink(dir).catch(e => {}))); | ||
| await Promise.all(this._tempDirs.map(async dir => await fs.promises.rm(dir, { recursive: true, force: true }).catch(e => {}))); |
Member
There was a problem hiding this comment.
Here and in other places dealing with tmp, we want it to be a best effort. If a leaking process holding onto one of the files, we should give up and give it a pass.
Contributor
Author
There was a problem hiding this comment.
Agreed. The current code uses force: true (ignores ENOENT) and .catch(e => {}) (swallows any remaining errors like EBUSY from held files). So it is already best-effort: it cleans up what it can and silently gives up on the rest.
Member
There was a problem hiding this comment.
Ah, there are no retries, I missed that. All good then.
pavelfeldman
approved these changes
May 9, 2026
Contributor
Test results for "MCP"7044 passed, 1068 skipped Merge workflow run. |
Contributor
Test results for "tests 1"2 flaky41704 passed, 850 skipped Merge workflow run. |
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
2 participants
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.
Summary
_deleteAllTempDirsusesfs.promises.unlinkwhich cannot remove directories (returns EISDIR on Linux, EPERM on Windows).catch(e => {}), so temp upload directories and their contents are never cleaned upfs.promises.rmwithrecursiveandforceoptions insteadFailure scenario: Every file upload via
setInputFilescreates a temp directory (upload-<guid>) in the artifacts dir. On context close,_deleteAllTempDirsattemptsunlinkon these directories, which always fails silently. The directories and uploaded files accumulate on disk indefinitely.Origin:
_deleteAllTempDirswas introduced in #12860 (2022-03-18) when temp entries were files. #31165 (2024-06-12) added folder uploads which pushes directories into_tempDirs, but the cleanup method was not updated.Same pattern in this repo:
fs.promises.rmfix for dashboard recording temp dir (open)