eperm stuff

eperm stuff

make sure other test still run

guard against eperm during windows readdir

retry more

idk

coverage

log less

less test assertions for CI output

add stack traces

add stack traces

retry all FS during moveRemove

fix auto import

remove stack traces

normal ci

Author: lukekarrys

.github/workflows/ci.yml

@@ -36,7 +36,7 @@ jobs:
         run: npm install
 
       - name: Run Tests
-        run: npm test -- -t0 -c
+        run: npm test
         env:
           RIMRAF_TEST_START_CHAR: a
           RIMRAF_TEST_END_CHAR: f

package.json

@@ -93,6 +93,7 @@
   ],
   "module": "./dist/esm/index.js",
   "tap": {
-    "coverage-map": "map.js"
+    "coverage-map": "map.js",
+    "timeout": 0
   }
 }

src/fs.ts

@@ -1,22 +1,14 @@
 // promisify ourselves, because older nodes don't have fs.promises
 
 import fs, { Dirent } from 'fs'
-import { readdirSync as rdSync } from 'fs'
 
 // sync ones just take the sync version from node
-export {
-  chmodSync,
-  mkdirSync,
-  renameSync,
-  rmdirSync,
-  rmSync,
-  statSync,
-  lstatSync,
-  unlinkSync,
-} from 'fs'
+export { chmodSync, renameSync, rmdirSync, rmSync, unlinkSync } from 'fs'
 
-export const readdirSync = (path: fs.PathLike): Dirent[] =>
-  rdSync(path, { withFileTypes: true })
+export const statSync = (path: fs.PathLike) => fs.statSync(path)
+export const lstatSync = (path: fs.PathLike) => fs.lstatSync(path)
+export const readdirSync = (path: fs.PathLike) =>
+  fs.readdirSync(path, { withFileTypes: true })
 
 // unrolled for better inlining, this seems to get better performance
 // than something like:
@@ -26,19 +18,8 @@ export const readdirSync = (path: fs.PathLike): Dirent[] =>
 const chmod = (path: fs.PathLike, mode: fs.Mode): Promise<void> =>
   new Promise((res, rej) => fs.chmod(path, mode, er => (er ? rej(er) : res())))
 
-const mkdir = (
-  path: fs.PathLike,
-  options?:
-    | fs.Mode
-    | (fs.MakeDirectoryOptions & { recursive?: boolean | null })
-    | null,
-): Promise<string | undefined> =>
+const readdir = async (path: fs.PathLike): Promise<Dirent[]> =>
   new Promise((res, rej) =>
-    fs.mkdir(path, options, (er, made) => (er ? rej(er) : res(made))),
-  )
-
-const readdir = (path: fs.PathLike): Promise<Dirent[]> =>
-  new Promise<Dirent[]>((res, rej) =>
     fs.readdir(path, { withFileTypes: true }, (er, data) =>
       er ? rej(er) : res(data),
     ),
@@ -70,7 +51,6 @@ const unlink = (path: fs.PathLike): Promise<void> =>
 
 export const promises = {
   chmod,
-  mkdir,
   readdir,
   rename,
   rm,

src/retry-busy.ts

@@ -1,30 +1,28 @@
 // note: max backoff is the maximum that any *single* backoff will do
 
 import { setTimeout } from 'timers/promises'
-import { RimrafAsyncOptions, RimrafOptions } from './index.js'
+import { RimrafAsyncOptions, RimrafSyncOptions } from './index.js'
 import { isFsError } from './error.js'
 
 export const MAXBACKOFF = 200
 export const RATE = 1.2
 export const MAXRETRIES = 10
 export const codes = new Set(['EMFILE', 'ENFILE', 'EBUSY'])
 
-export const retryBusy = <T>(fn: (path: string) => Promise<T>) => {
-  const method = async (
-    path: string,
-    opt: RimrafAsyncOptions,
-    backoff = 1,
-    total = 0,
-  ) => {
+export const retryBusy = <T, U extends RimrafAsyncOptions>(
+  fn: (path: string, opt: U) => Promise<T>,
+  retryCodes: Set<string> = codes,
+) => {
+  const method = async (path: string, opt: U, backoff = 1, total = 0) => {
     const mbo = opt.maxBackoff || MAXBACKOFF
     const rate = opt.backoff || RATE
     const max = opt.maxRetries || MAXRETRIES
     let retries = 0
     while (true) {
       try {
-        return await fn(path)
+        return await fn(path, opt)
       } catch (er) {
-        if (isFsError(er) && er.path === path && codes.has(er.code)) {
+        if (isFsError(er) && er.path === path && retryCodes.has(er.code)) {
           backoff = Math.ceil(backoff * rate)
           total = backoff + total
           if (total < mbo) {
@@ -45,18 +43,21 @@ export const retryBusy = <T>(fn: (path: string) => Promise<T>) => {
 }
 
 // just retries, no async so no backoff
-export const retryBusySync = <T>(fn: (path: string) => T) => {
-  const method = (path: string, opt: RimrafOptions) => {
+export const retryBusySync = <T, U extends RimrafSyncOptions>(
+  fn: (path: string, opt: U) => T,
+  retryCodes: Set<string> = codes,
+) => {
+  const method = (path: string, opt: U) => {
     const max = opt.maxRetries || MAXRETRIES
     let retries = 0
     while (true) {
       try {
-        return fn(path)
+        return fn(path, opt)
       } catch (er) {
         if (
           isFsError(er) &&
           er.path === path &&
-          codes.has(er.code) &&
+          retryCodes.has(er.code) &&
           retries < max
         ) {
           retries++

src/rimraf-move-remove.ts

@@ -14,50 +14,109 @@
 import { basename, parse, resolve } from 'path'
 import { defaultTmp, defaultTmpSync } from './default-tmp.js'
 import { ignoreENOENT, ignoreENOENTSync } from './ignore-enoent.js'
-import { lstatSync, promises, renameSync, rmdirSync, unlinkSync } from './fs.js'
+import * as FS from './fs.js'
 import { Dirent, Stats } from 'fs'
 import { RimrafAsyncOptions, RimrafSyncOptions } from './index.js'
 import { readdirOrError, readdirOrErrorSync } from './readdir-or-error.js'
 import { fixEPERM, fixEPERMSync } from './fix-eperm.js'
 import { errorCode } from './error.js'
-const { lstat, rename, unlink, rmdir } = promises
+import { retryBusy, retryBusySync, codes } from './retry-busy.js'
+
+type Tmp<T extends RimrafAsyncOptions | RimrafSyncOptions> = T & {
+  tmp: string
+}
 
 // crypto.randomBytes is much slower, and Math.random() is enough here
-const uniqueFilename = (path: string) => `.${basename(path)}.${Math.random()}`
+const uniqueName = (path: string, tmp: string) =>
+  resolve(tmp, `.${basename(path)}.${Math.random()}`)
+
+// moveRemove is the fallback on Windows and due to flaky EPERM errors
+// if we are actually on Windows, then we add EPERM to the list of
+// error codes that we treat as busy, as well as retrying all fs
+// operations for EPERM only.
+const isWin = process.platform === 'win32'
 
-const unlinkFixEPERM = fixEPERM(unlink)
-const unlinkFixEPERMSync = fixEPERMSync(unlinkSync)
+// all fs functions are only retried for EPERM and only on windows
+const retryFsCodes = isWin ? new Set(['EPERM']) : undefined
+const maybeRetry = <T, U extends RimrafAsyncOptions>(
+  fn: (path: string, opt: U) => Promise<T>,
+) => (retryFsCodes ? retryBusy(fn, retryFsCodes) : fn)
+const maybeRetrySync = <T, U extends RimrafSyncOptions>(
+  fn: (path: string, opt: U) => T,
+) => (retryFsCodes ? retryBusySync(fn, retryFsCodes) : fn)
+const rename = maybeRetry(
+  async (path: string, opt: Tmp<RimrafAsyncOptions>) => {
+    const newPath = uniqueName(path, opt.tmp)
+    await FS.promises.rename(path, newPath)
+    return newPath
+  },
+)
+const renameSync = maybeRetrySync(
+  (path: string, opt: Tmp<RimrafSyncOptions>) => {
+    const newPath = uniqueName(path, opt.tmp)
+    FS.renameSync(path, newPath)
+    return newPath
+  },
+)
+const readdir = maybeRetry(readdirOrError)
+const readdirSync = maybeRetrySync(readdirOrErrorSync)
+const lstat = maybeRetry(FS.promises.lstat)
+const lstatSync = maybeRetrySync(FS.lstatSync)
+
+// unlink and rmdir and always retryable regardless of platform
+// but we add the EPERM error code as a busy signal on Windows only
+const retryCodes = new Set([...codes, ...(retryFsCodes || [])])
+const unlink = retryBusy(fixEPERM(FS.promises.unlink), retryCodes)
+const unlinkSync = retryBusySync(fixEPERMSync(FS.unlinkSync), retryCodes)
+const rmdir = retryBusy(fixEPERM(FS.promises.rmdir), retryCodes)
+const rmdirSync = retryBusySync(fixEPERMSync(FS.rmdirSync), retryCodes)
 
 export const rimrafMoveRemove = async (
   path: string,
-  opt: RimrafAsyncOptions,
+  { tmp, ...opt }: RimrafAsyncOptions,
 ) => {
   opt?.signal?.throwIfAborted()
+
+  tmp ??= await defaultTmp(path)
+  if (path === tmp && parse(path).root !== path) {
+    throw new Error('cannot delete temp directory used for deletion')
+  }
+
   return (
     (await ignoreENOENT(
-      lstat(path).then(stat => rimrafMoveRemoveDir(path, opt, stat)),
+      lstat(path, opt).then(stat =>
+        rimrafMoveRemoveDir(path, { ...opt, tmp }, stat),
+      ),
     )) ?? true
   )
 }
 
+export const rimrafMoveRemoveSync = (
+  path: string,
+  { tmp, ...opt }: RimrafSyncOptions,
+) => {
+  opt?.signal?.throwIfAborted()
+
+  tmp ??= defaultTmpSync(path)
+  if (path === tmp && parse(path).root !== path) {
+    throw new Error('cannot delete temp directory used for deletion')
+  }
+
+  return (
+    ignoreENOENTSync(() =>
+      rimrafMoveRemoveDirSync(path, { ...opt, tmp }, lstatSync(path, opt)),
+    ) ?? true
+  )
+}
+
 const rimrafMoveRemoveDir = async (
   path: string,
-  opt: RimrafAsyncOptions,
+  opt: Tmp<RimrafAsyncOptions>,
   ent: Dirent | Stats,
 ): Promise<boolean> => {
   opt?.signal?.throwIfAborted()
-  if (!opt.tmp) {
-    return rimrafMoveRemoveDir(
-      path,
-      { ...opt, tmp: await defaultTmp(path) },
-      ent,
-    )
-  }
-  if (path === opt.tmp && parse(path).root !== path) {
-    throw new Error('cannot delete temp directory used for deletion')
-  }
 
-  const entries = ent.isDirectory() ? await readdirOrError(path) : null
+  const entries = ent.isDirectory() ? await readdir(path, opt) : null
   if (!Array.isArray(entries)) {
     // this can only happen if lstat/readdir lied, or if the dir was
     // swapped out with a file at just the right moment.
@@ -74,7 +133,7 @@ const rimrafMoveRemoveDir = async (
     if (opt.filter && !(await opt.filter(path, ent))) {
       return false
     }
-    await ignoreENOENT(tmpUnlink(path, opt.tmp, unlinkFixEPERM))
+    await ignoreENOENT(rename(path, opt).then(p => unlink(p, opt)))
     return true
   }
 
@@ -98,49 +157,18 @@ const rimrafMoveRemoveDir = async (
   if (opt.filter && !(await opt.filter(path, ent))) {
     return false
   }
-  await ignoreENOENT(tmpUnlink(path, opt.tmp, rmdir))
+  await ignoreENOENT(rename(path, opt).then(p => rmdir(p, opt)))
   return true
 }
 
-const tmpUnlink = async <T>(
-  path: string,
-  tmp: string,
-  rm: (p: string) => Promise<T>,
-) => {
-  const tmpFile = resolve(tmp, uniqueFilename(path))
-  await rename(path, tmpFile)
-  return await rm(tmpFile)
-}
-
-export const rimrafMoveRemoveSync = (path: string, opt: RimrafSyncOptions) => {
-  opt?.signal?.throwIfAborted()
-  return (
-    ignoreENOENTSync(() =>
-      rimrafMoveRemoveDirSync(path, opt, lstatSync(path)),
-    ) ?? true
-  )
-}
-
 const rimrafMoveRemoveDirSync = (
   path: string,
-  opt: RimrafSyncOptions,
+  opt: Tmp<RimrafSyncOptions>,
   ent: Dirent | Stats,
 ): boolean => {
   opt?.signal?.throwIfAborted()
-  if (!opt.tmp) {
-    return rimrafMoveRemoveDirSync(
-      path,
-      { ...opt, tmp: defaultTmpSync(path) },
-      ent,
-    )
-  }
-  const tmp: string = opt.tmp
 
-  if (path === opt.tmp && parse(path).root !== path) {
-    throw new Error('cannot delete temp directory used for deletion')
-  }
-
-  const entries = ent.isDirectory() ? readdirOrErrorSync(path) : null
+  const entries = ent.isDirectory() ? readdirSync(path, opt) : null
   if (!Array.isArray(entries)) {
     // this can only happen if lstat/readdir lied, or if the dir was
     // swapped out with a file at just the right moment.
@@ -157,7 +185,7 @@ const rimrafMoveRemoveDirSync = (
     if (opt.filter && !opt.filter(path, ent)) {
       return false
     }
-    ignoreENOENTSync(() => tmpUnlinkSync(path, tmp, unlinkFixEPERMSync))
+    ignoreENOENTSync(() => unlinkSync(renameSync(path, opt), opt))
     return true
   }
 
@@ -175,16 +203,6 @@ const rimrafMoveRemoveDirSync = (
   if (opt.filter && !opt.filter(path, ent)) {
     return false
   }
-  ignoreENOENTSync(() => tmpUnlinkSync(path, tmp, rmdirSync))
+  ignoreENOENTSync(() => rmdirSync(renameSync(path, opt), opt))
   return true
 }
-
-const tmpUnlinkSync = (
-  path: string,
-  tmp: string,
-  rmSync: (p: string) => void,
-) => {
-  const tmpFile = resolve(tmp, uniqueFilename(path))
-  renameSync(path, tmpFile)
-  return rmSync(tmpFile)
-}