fix: [node-core-library] Retry transient Windows EPERM/EBUSY in FileSystem.deleteFolder(Async)#5836
Conversation
…ystem.deleteFolder(Async)
… FileSystem.ensureEmptyFolderAsync
bartvandenende-wm
requested review from
apostolisms,
bmiddha,
dmichon-msft,
iclanton,
jxanthony and
octogonz
as code owners
|
@iclanton any chance this could get a review? |
|
(FWIW: Although we're at the same company, I did independently encounter and come up with basically the same solution as Bart did, just on a different repo. Without the fix, a very basic one-line TS "hello world" with just TS+heft as the main dependencies will hit the EPERM issue maybe about a quarter of the time, simply from |
| "changes": [ | ||
| { | ||
| "packageName": "@rushstack/node-core-library", | ||
| "comment": "Make FileSystem.deleteFolder, FileSystem.deleteFolderAsync, FileSystem.ensureEmptyFolder, and FileSystem.ensureEmptyFolderAsync resilient to transient Windows EPERM/EBUSY/ENOTEMPTY errors by using native fs.rm with retries.", |
There was a problem hiding this comment.
| "comment": "Make FileSystem.deleteFolder, FileSystem.deleteFolderAsync, FileSystem.ensureEmptyFolder, and FileSystem.ensureEmptyFolderAsync resilient to transient Windows EPERM/EBUSY/ENOTEMPTY errors by using native fs.rm with retries.", | |
| "comment": "Make `FileSystem.deleteFolder`, `FileSystem.deleteFolderAsync`, `FileSystem.ensureEmptyFolder`, and `FileSystem.ensureEmptyFolderAsync` resilient to transient Windows `EPERM`/`EBUSY`/`ENOTEMPTY` errors by using native `fs.rm` with retries.", |
| public static deleteFolder(folderPath: string): void { | ||
| FileSystem._wrapException(() => { | ||
| fsx.removeSync(folderPath); | ||
| fs.rmSync(folderPath, { recursive: true, force: true, maxRetries: 3, retryDelay: 100 }); |
There was a problem hiding this comment.
Stick this options object in a shared constant?
| try { | ||
| items = fsx.readdirSync(folderPath); | ||
| } catch { | ||
| fsx.ensureDirSync(folderPath); |
There was a problem hiding this comment.
Specifically check for a not-exist error using FileSystem.isNotExistError, and otherwise re-throw
| fsx.emptyDirSync(folderPath); | ||
| let items: string[]; | ||
| try { | ||
| items = fsx.readdirSync(folderPath); |
There was a problem hiding this comment.
Use FileSystem.readFolderItems and handle subfolders.
| return; | ||
| } | ||
| for (const item of items) { | ||
| fs.rmSync(nodeJsPath.join(folderPath, item), { |
There was a problem hiding this comment.
Forward shashes work fine on Windows.
| fs.rmSync(nodeJsPath.join(folderPath, item), { | |
| fs.rmSync(`${folderPath}/${item}`, { |
| await fsx.ensureDir(folderPath); | ||
| return; | ||
| } | ||
| await Promise.all( |
There was a problem hiding this comment.
Limit concurrency here using Async.forEachAsync. It may be worthwhile to recursive enumerate files and sort them by depth, and delete from deepest-in.
| /** | ||
| * An async version of {@link FileSystem.ensureEmptyFolder}. | ||
| */ | ||
| public static async ensureEmptyFolderAsync(folderPath: string): Promise<void> { |
There was a problem hiding this comment.
Same feedback as the sync version applies here.
3 participants
Summary
Restores retry-on-transient-error behavior to
FileSystem.deleteFolderandFileSystem.deleteFolderAsyncon Windows, fixingEPERM: operation not permitted, rmdirfailures duringrush rebuild/ HeftcleanFileson self-hosted Windows runners.Fixes #5373.
Details
Root cause
In v5.11.0 (PR #5088)
node-core-librarybumped itsfs-extradependency from~7.0.1to~11.3.0.fs-extra.remove(Sync)was backed byrimraf@2, which retriesEBUSY/EPERM/ENOTEMPTYon Windows by default (maxBusyTries: 3).fs-extra@10onward,remove(Sync)is a thin wrapper aroundfs.rm(path, { recursive: true, force: true })with no retry options.The retry behavior was silently lost, which made consumers susceptible to the documented Windows transient where another process briefly holds a handle to a child of the directory being removed.
Change
Replace the two affected methods with native
fs.rmSync/fsPromises.rmusing{ recursive: true, force: true, maxRetries: 3, retryDelay: 100 }.fs-extra@11'sremove(Sync)is already a thin wrapper over the same native call.Note
I also added
ensureEmptyFolder/ensureEmptyFolderAsyncfor consistency as they exhibit the same regression.How it was tested
manual testing