eperm stuff

eperm stuff make sure other test still run guard against eperm during windows readdir
lukekarrys · Oct 11, 2024 · 784cd09 · 784cd09
1 parent d7b3cd4
commit 784cd09
Show file tree

Hide file tree

Showing 11 changed files with 432 additions and 128 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -3,6 +3,46 @@ name: CI
 on: [push, pull_request]
 
 jobs:
+  eperm:
+    runs-on: windows-latest
+    defaults:
+      run:
+        shell: bash
+
+    steps:
+      - name: Checkout Repository
+        uses: actions/checkout@v4
+
+      - name: Use Nodejs 22.x
+        uses: actions/setup-node@v4
+        with:
+          node-version: 22.x
+
+      - name: Install dependencies
+        run: npm install
+
+      - run: npm test -- -t0 test/integration/eperm.ts --disable-coverage --grep=. --grep=async
+      - run: npm test -- -t0 test/integration/eperm.ts --disable-coverage --grep=. --grep=async
+      - run: npm test -- -t0 test/integration/eperm.ts --disable-coverage --grep=. --grep=async
+      - run: npm test -- -t0 test/integration/eperm.ts --disable-coverage --grep=. --grep=async
+      - run: npm test -- -t0 test/integration/eperm.ts --disable-coverage --grep=. --grep=async
+      - run: npm test -- -t0 test/integration/eperm.ts --disable-coverage --grep=. --grep=async
+      - run: npm test -- -t0 test/integration/eperm.ts --disable-coverage --grep=. --grep=async
+      - run: npm test -- -t0 test/integration/eperm.ts --disable-coverage --grep=. --grep=async
+      - run: npm test -- -t0 test/integration/eperm.ts --disable-coverage --grep=. --grep=async
+      - run: npm test -- -t0 test/integration/eperm.ts --disable-coverage --grep=. --grep=async
+      - run: npm test -- -t0 test/integration/eperm.ts --disable-coverage --grep=. --grep=async
+      - run: npm test -- -t0 test/integration/eperm.ts --disable-coverage --grep=. --grep=async
+      - run: npm test -- -t0 test/integration/eperm.ts --disable-coverage --grep=. --grep=async
+      - run: npm test -- -t0 test/integration/eperm.ts --disable-coverage --grep=. --grep=async
+      - run: npm test -- -t0 test/integration/eperm.ts --disable-coverage --grep=. --grep=async
+      - run: npm test -- -t0 test/integration/eperm.ts --disable-coverage --grep=. --grep=async
+      - run: npm test -- -t0 test/integration/eperm.ts --disable-coverage --grep=. --grep=async
+      - run: npm test -- -t0 test/integration/eperm.ts --disable-coverage --grep=. --grep=async
+      - run: npm test -- -t0 test/integration/eperm.ts --disable-coverage --grep=. --grep=async
+      - run: npm test -- -t0 test/integration/eperm.ts --disable-coverage --grep=. --grep=async
+      - run: npm test -- -t0 test/integration/eperm.ts --disable-coverage --grep=. --grep=async
+
   build:
     strategy:
       matrix:
@@ -41,3 +81,4 @@ jobs:
           RIMRAF_TEST_START_CHAR: a
           RIMRAF_TEST_END_CHAR: f
           RIMRAF_TEST_DEPTH: 5
+          RIMRAF_TEST_SKIP_EPERM_INTEGRATION: 1
diff --git a/src/error.ts b/src/error.ts
@@ -13,3 +13,5 @@ export const isFsError = (
 
 export const errorCode = (er: unknown) =>
   isRecord(er) && hasString(er, 'code') ? er.code : null
+
+export const errorCause = (er: unknown) => (isRecord(er) ? er.cause : null)
diff --git a/src/fs.ts b/src/fs.ts
@@ -79,3 +79,35 @@ export const promises = {
   lstat,
   unlink,
 }
+
+// import fs, { Dirent } from 'fs'
+// import fsPromises from 'fs/promises'
+
+// export {
+//   chmodSync,
+//   mkdirSync,
+//   renameSync,
+//   rmdirSync,
+//   rmSync,
+//   statSync,
+//   lstatSync,
+//   unlinkSync,
+// } from 'fs'
+
+// export const readdirSync = (path: fs.PathLike): Dirent[] =>
+//   fs.readdirSync(path, { withFileTypes: true })
+
+// const readdir = (path: fs.PathLike): Promise<Dirent[]> =>
+//   fsPromises.readdir(path, { withFileTypes: true })
+
+// export const promises = {
+//   chmod: fsPromises.chmod,
+//   mkdir: fsPromises.mkdir,
+//   readdir,
+//   rename: fsPromises.rename,
+//   rm: fsPromises.rm,
+//   rmdir: fsPromises.rmdir,
+//   stat: fsPromises.stat,
+//   lstat: fsPromises.lstat,
+//   unlink: fsPromises.unlink,
+// }
diff --git a/src/ignore-enoent.ts b/src/ignore-enoent.ts
@@ -1,20 +1,30 @@
 import { errorCode } from './error.js'
 
-export const ignoreENOENT = async <T>(p: Promise<T>, rethrow?: unknown) =>
+const pickError = (er: unknown, er2?: unknown) => {
+  if (!er2) {
+    return er
+  }
+  if (er2 instanceof Error) {
+    er2.cause = er
+  }
+  return er2
+}
+
+export const ignoreENOENT = async <T>(p: Promise<T>, er2?: unknown) =>
   p.catch(er => {
     if (errorCode(er) === 'ENOENT') {
       return
     }
-    throw rethrow ?? er
+    throw pickError(er, er2)
   })
 
-export const ignoreENOENTSync = <T>(fn: () => T, rethrow?: unknown) => {
+export const ignoreENOENTSync = <T>(fn: () => T, er2?: unknown) => {
   try {
     return fn()
   } catch (er) {
     if (errorCode(er) === 'ENOENT') {
       return
     }
-    throw rethrow ?? er
+    throw pickError(er, er2)
   }
 }
diff --git a/src/retry-busy.ts b/src/retry-busy.ts
@@ -9,7 +9,10 @@ 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>) => {
+export const retryBusy = <T>(
+  fn: (path: string) => Promise<T>,
+  extraCodes?: Set<string>,
+) => {
   const method = async (
     path: string,
     opt: RimrafAsyncOptions,
@@ -24,9 +27,27 @@ export const retryBusy = <T>(fn: (path: string) => Promise<T>) => {
       try {
         return await fn(path)
       } catch (er) {
-        if (isFsError(er) && er.path === path && codes.has(er.code)) {
+        if (
+          isFsError(er) &&
+          er.path === path &&
+          (codes.has(er.code) || extraCodes?.has(er.code))
+        ) {
           backoff = Math.ceil(backoff * rate)
           total = backoff + total
+          /* c8 ignore start */
+          if (er.code === 'EPERM') {
+            console.trace(
+              'EPERM',
+              {
+                total,
+                mbo,
+                retries,
+                backoff,
+              },
+              er,
+            )
+          }
+          /* c8 ignore stop */
           if (total < mbo) {
             await setTimeout(backoff)
             return method(path, opt, backoff, total)
@@ -45,7 +66,10 @@ export const retryBusy = <T>(fn: (path: string) => Promise<T>) => {
 }
 
 // just retries, no async so no backoff
-export const retryBusySync = <T>(fn: (path: string) => T) => {
+export const retryBusySync = <T>(
+  fn: (path: string) => T,
+  extraCodes?: Set<string>,
+) => {
   const method = (path: string, opt: RimrafOptions) => {
     const max = opt.maxRetries || MAXRETRIES
     let retries = 0
@@ -56,7 +80,7 @@ export const retryBusySync = <T>(fn: (path: string) => T) => {
         if (
           isFsError(er) &&
           er.path === path &&
-          codes.has(er.code) &&
+          (codes.has(er.code) || extraCodes?.has(er.code)) &&
           retries < max
         ) {
           retries++

diff --git a/src/rimraf-move-remove.ts b/src/rimraf-move-remove.ts
@@ -20,42 +20,55 @@ 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'
+import { retryBusy, retryBusySync } from './retry-busy.js'
 const { lstat, rename, unlink, rmdir } = promises
 
 // crypto.randomBytes is much slower, and Math.random() is enough here
 const uniqueFilename = (path: string) => `.${basename(path)}.${Math.random()}`
 
-const unlinkFixEPERM = fixEPERM(unlink)
-const unlinkFixEPERMSync = fixEPERMSync(unlinkSync)
+const retryCodes = new Set(['EPERM'])
+const unlinkFixEPERM = retryBusy(fixEPERM(unlink), retryCodes)
+const unlinkFixEPERMSync = retryBusySync(fixEPERMSync(unlinkSync), retryCodes)
+const rmdirFixEPERM = retryBusy(fixEPERM(rmdir), retryCodes)
+const rmdirFixEPERMSync = retryBusySync(fixEPERMSync(rmdirSync), retryCodes)
+
+const tmpRename = async (path: string, tmp: string) => {
+  const tmpFile = resolve(tmp, uniqueFilename(path))
+  await rename(path, tmpFile)
+  return tmpFile
+}
+
+const tmpRenameSync = (path: string, tmp: string) => {
+  const tmpFile = resolve(tmp, uniqueFilename(path))
+  renameSync(path, tmpFile)
+  return tmpFile
+}
 
 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).then(stat => rimrafMoveRemoveDir(path, tmp, opt, stat)),
     )) ?? true
   )
 }
 
 const rimrafMoveRemoveDir = async (
   path: string,
-  opt: RimrafAsyncOptions,
+  tmp: string,
+  opt: Omit<RimrafAsyncOptions, 'tmp'>,
   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
   if (!Array.isArray(entries)) {
@@ -66,6 +79,11 @@ const rimrafMoveRemoveDir = async (
       if (errorCode(entries) === 'ENOENT') {
         return true
       }
+      if (errorCode(entries) === 'EPERM') {
+        // TODO: what to do here??
+        console.trace('EPERM', entries)
+        throw entries
+      }
       if (errorCode(entries) !== 'ENOTDIR') {
         throw entries
       }
@@ -74,14 +92,14 @@ const rimrafMoveRemoveDir = async (
     if (opt.filter && !(await opt.filter(path, ent))) {
       return false
     }
-    await ignoreENOENT(tmpUnlink(path, opt.tmp, unlinkFixEPERM))
+    await ignoreENOENT(tmpRename(path, tmp).then(r => unlinkFixEPERM(r, opt)))
     return true
   }
 
   const removedAll = (
     await Promise.all(
       entries.map(ent =>
-        rimrafMoveRemoveDir(resolve(path, ent.name), opt, ent),
+        rimrafMoveRemoveDir(resolve(path, ent.name), tmp, opt, ent),
       ),
     )
   ).every(v => v === true)
@@ -98,47 +116,35 @@ const rimrafMoveRemoveDir = async (
   if (opt.filter && !(await opt.filter(path, ent))) {
     return false
   }
-  await ignoreENOENT(tmpUnlink(path, opt.tmp, rmdir))
+  await ignoreENOENT(tmpRename(path, tmp).then(r => rmdirFixEPERM(r, opt)))
   return true
 }
 
-const tmpUnlink = async <T>(
+export const rimrafMoveRemoveSync = (
   path: string,
-  tmp: string,
-  rm: (p: string) => Promise<T>,
+  { tmp, ...opt }: RimrafSyncOptions,
 ) => {
-  const tmpFile = resolve(tmp, uniqueFilename(path))
-  await rename(path, tmpFile)
-  return await rm(tmpFile)
-}
-
-export const rimrafMoveRemoveSync = (path: string, 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, lstatSync(path)),
+      rimrafMoveRemoveDirSync(path, tmp, opt, lstatSync(path)),
     ) ?? true
   )
 }
 
 const rimrafMoveRemoveDirSync = (
   path: string,
-  opt: RimrafSyncOptions,
+  tmp: string,
+  opt: Omit<RimrafSyncOptions, 'tmp'>,
   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
   if (!Array.isArray(entries)) {
@@ -149,6 +155,11 @@ const rimrafMoveRemoveDirSync = (
       if (errorCode(entries) === 'ENOENT') {
         return true
       }
+      if (errorCode(entries) === 'EPERM') {
+        // TODO: what to do here??
+        console.trace('EPERM', entries)
+        throw entries
+      }
       if (errorCode(entries) !== 'ENOTDIR') {
         throw entries
       }
@@ -157,14 +168,14 @@ const rimrafMoveRemoveDirSync = (
     if (opt.filter && !opt.filter(path, ent)) {
       return false
     }
-    ignoreENOENTSync(() => tmpUnlinkSync(path, tmp, unlinkFixEPERMSync))
+    ignoreENOENTSync(() => unlinkFixEPERMSync(tmpRenameSync(path, tmp), opt))
     return true
   }
 
   let removedAll = true
   for (const ent of entries) {
     const p = resolve(path, ent.name)
-    removedAll = rimrafMoveRemoveDirSync(p, opt, ent) && removedAll
+    removedAll = rimrafMoveRemoveDirSync(p, tmp, opt, ent) && removedAll
   }
   if (!removedAll) {
     return false
@@ -175,16 +186,6 @@ const rimrafMoveRemoveDirSync = (
   if (opt.filter && !opt.filter(path, ent)) {
     return false
   }
-  ignoreENOENTSync(() => tmpUnlinkSync(path, tmp, rmdirSync))
+  ignoreENOENTSync(() => rmdirFixEPERMSync(tmpRenameSync(path, tmp), 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)
-}
Original file line number	Diff line number	Diff line change
Expand Up		@@ -13,3 +13,5 @@ export const isFsError = (

		export const errorCode = (er: unknown) =>
		isRecord(er) && hasString(er, 'code') ? er.code : null

		export const errorCause = (er: unknown) => (isRecord(er) ? er.cause : null)