diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 73c0e451..3e4eac6a 100644 --- 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/fs.ts b/src/fs.ts index 45abbcb2..f31d605e 100644 --- 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 => +// 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/retry-busy.ts b/src/retry-busy.ts index 40ceb31d..71c5affb 100644 --- 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 = (fn: (path: string) => Promise) => { +export const retryBusy = ( + fn: (path: string) => Promise, + extraCodes?: Set, +) => { const method = async ( path: string, opt: RimrafAsyncOptions, @@ -24,9 +27,24 @@ export const retryBusy = (fn: (path: string) => Promise) => { 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 retry busy', { + error: er, + total, + mbo, + retries, + backoff, + }) + } + /* c8 ignore stop */ if (total < mbo) { await setTimeout(backoff) return method(path, opt, backoff, total) @@ -45,7 +63,10 @@ export const retryBusy = (fn: (path: string) => Promise) => { } // just retries, no async so no backoff -export const retryBusySync = (fn: (path: string) => T) => { +export const retryBusySync = ( + fn: (path: string) => T, + extraCodes?: Set, +) => { const method = (path: string, opt: RimrafOptions) => { const max = opt.maxRetries || MAXRETRIES let retries = 0 @@ -56,7 +77,7 @@ export const retryBusySync = (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 index f6a3ef1b..9e9550bc 100644 --- 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, ent: Dirent | Stats, ): Promise => { 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,13 @@ const rimrafMoveRemoveDir = async ( if (errorCode(entries) === 'ENOENT') { return true } + if (errorCode(entries) === 'EPERM') { + // TODO: what to do here?? + console.trace('EPERM reading dir uring move remove', { + error: entries, + }) + throw entries + } if (errorCode(entries) !== 'ENOTDIR') { throw entries } @@ -74,14 +94,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 +118,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 ( +export const rimrafMoveRemoveSync = ( path: string, - tmp: string, - rm: (p: string) => Promise, + { 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, 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 +157,13 @@ const rimrafMoveRemoveDirSync = ( if (errorCode(entries) === 'ENOENT') { return true } + if (errorCode(entries) === 'EPERM') { + // TODO: what to do here?? + console.trace('EPERM reading dir uring move remove', { + error: entries, + }) + throw entries + } if (errorCode(entries) !== 'ENOTDIR') { throw entries } @@ -157,14 +172,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 +190,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) -} diff --git a/src/rimraf-windows.ts b/src/rimraf-windows.ts index 567e7143..fd25a991 100644 --- a/src/rimraf-windows.ts +++ b/src/rimraf-windows.ts @@ -37,7 +37,7 @@ const rimrafWindowsDirMoveRemoveFallback = async ( await rimrafWindowsDirRetry(path, opt) return true } catch (er) { - if (errorCode(er) === 'ENOTEMPTY') { + if (errorCode(er) === 'ENOTEMPTY' || errorCode(er) === 'EPERM') { return rimrafMoveRemove(path, opt) } throw er @@ -55,7 +55,7 @@ const rimrafWindowsDirMoveRemoveFallbackSync = ( rimrafWindowsDirRetrySync(path, opt) return true } catch (er) { - if (errorCode(er) === 'ENOTEMPTY') { + if (errorCode(er) === 'ENOTEMPTY' || errorCode(er) === 'EPERM') { return rimrafMoveRemoveSync(path, opt) } throw er @@ -101,6 +101,14 @@ const rimrafWindowsDir = async ( if (errorCode(entries) === 'ENOENT') { return true } + if (errorCode(entries) === 'EPERM') { + // TODO: what to do here?? + console.trace('EPERM reading dir during rimrafWindowsDir', { + error: entries, + }) + await ignoreENOENT(rimrafWindowsDirMoveRemoveFallback(path, opt)) + return true + } if (errorCode(entries) !== 'ENOTDIR') { throw entries } @@ -155,6 +163,16 @@ const rimrafWindowsDirSync = ( if (errorCode(entries) === 'ENOENT') { return true } + if (errorCode(entries) === 'EPERM') { + // TODO: what to do here?? + console.trace('EPERM reading dir during rimrafWindowsDirSync', { + error: entries, + }) + ignoreENOENTSync(() => + rimrafWindowsDirMoveRemoveFallbackSync(path, opt), + ) + return true + } if (errorCode(entries) !== 'ENOTDIR') { throw entries } diff --git a/test/integration/eperm.ts b/test/integration/eperm.ts new file mode 100644 index 00000000..44b81cd9 --- /dev/null +++ b/test/integration/eperm.ts @@ -0,0 +1,172 @@ +import t, { Test } from 'tap' +import { mkdirSync, readdirSync, writeFileSync } from 'fs' +import { sep, join } from 'path' +import { globSync } from 'glob' +import { windows, windowsSync } from '../../src/index.js' +import { randomBytes } from 'crypto' +import assert from 'assert' + +const arrSame = (arr1: string[], arr2: string[]) => { + const s = (a: string[]) => [...a].sort().join(',') + return s(arr1) === s(arr2) +} + +const setup = (t: Test) => { + const [iterations, depth, fileCount, fileKb] = + process.env.CI ? [20_000, 15, 7, 100] : [200, 8, 3, 10] + + const dir = t.testdir() + + const letters = (length: number) => + Array.from({ length }).map((_, i) => (10 + i).toString(36)) + const files = letters(fileCount).map(f => `file_${f}`) + const dirs = join(...letters(depth)) + .split(sep) + .reduce((acc, d) => acc.concat(join(acc.at(-1) ?? '', d)), []) + const entries = dirs + .flatMap(d => [d, ...files.map(f => join(d, f))]) + .map(d => join(dir, d)) + + let iteration = 0 + return function* () { + while (iteration !== iterations) { + // use custom error to throw instead of using tap assertions to cut down + // on output when running many iterations + class RunError extends Error { + constructor(message: string, c?: Record) { + super(message, { + cause: { + testName: t.name, + iteration, + ...c, + }, + }) + } + } + + const actual = readdirSync(dir) + assert( + !actual.length, + new RunError(`dir is not empty`, { + found: actual, + }), + ) + + mkdirSync(join(dir, dirs.at(-1)!), { recursive: true }) + for (const d of dirs) { + for (const f of files) { + writeFileSync(join(dir, d, f), randomBytes(1024 * fileKb)) + } + } + + // randomize results from glob so that when running Promise.all(rimraf) + // on the result it will potentially delete parent directories before + // child directories and their files. This seems to make EPERM errors + // more likely on Windows. + const matches = globSync('**/*', { cwd: dir }) + .sort(() => 0.5 - Math.random()) + .map(m => join(dir, m)) + + assert( + arrSame(matches, entries), + new RunError(`glob result does not match expected`, { + found: matches, + wanted: entries, + }), + ) + + iteration += 1 + + yield [matches, RunError] as const + } + + return { + contents: readdirSync(dir), + iteration, + iterations, + } + } +} + +// Copied from sindresorhus/del since it was reported in +// https://github.com/isaacs/rimraf/pull/314 that this test would throw EPERM +// errors consistently in Windows CI environments. +// https://github.com/sindresorhus/del/blob/chore/update-deps/test.js#L116 +t.test('windows does not throw EPERM', t => { + if (process.env.RIMRAF_TEST_SKIP_EPERM_INTEGRATION) { + return t.end() + } + + t.test('sync', t => { + let i + const r = setup(t)() + while ((i = r.next())) { + if (i.done) { + i = i.value + break + } + + const [matches, RunError] = i.value + const result = matches + .map(path => { + try { + return { + path, + deleted: windowsSync(path), + } + } catch (error) { + throw new RunError('rimraf error', { error, path }) + } + }) + .filter(({ deleted }) => deleted !== true) + assert( + !result.length, + new RunError(`some entries were not deleted`, { + found: result, + }), + ) + } + + t.strictSame(i.contents, []) + t.equal(i.iteration, i.iterations, `ran all ${i.iteration} iterations`) + t.end() + }) + + t.test('async', async t => { + let i + const r = setup(t)() + while ((i = r.next())) { + if (i.done) { + i = i.value + break + } + + const [matches, RunError] = i.value + const result = ( + await Promise.all( + matches.map(async path => { + try { + return { + path, + deleted: await windows(path), + } + } catch (error) { + throw new RunError('rimraf error', { error, path }) + } + }), + ) + ).filter(({ deleted }) => deleted !== true) + assert( + !result.length, + new RunError(`some entries were not deleted`, { + found: result, + }), + ) + } + + t.strictSame(i.contents, []) + t.equal(i.iteration, i.iterations, `ran all ${i.iteration} iterations`) + }) + + t.end() +}) diff --git a/test/retry-busy.ts b/test/retry-busy.ts index eaee7af5..8e1965fd 100644 --- a/test/retry-busy.ts +++ b/test/retry-busy.ts @@ -7,7 +7,7 @@ import { retryBusySync, } from '../src/retry-busy.js' -import t from 'tap' +import t, { Test } from 'tap' t.matchSnapshot( { @@ -36,11 +36,15 @@ t.test('basic working operation when no errors happen', async t => { await rB(arg, opt).then(() => t.equal(calls, 2)) }) -t.test('retry when known error code thrown', t => { - t.plan(codes.size) +t.test('retry when known error code thrown', async t => { + t.plan(codes.size + 1) - for (const code of codes) { - t.test(code, async t => { + const testCode = ( + t: Test, + code: string, + extraCodes: Set | undefined, + ) => + t.test(`${code} extraCodes:${!!extraCodes}`, async t => { let thrown = false let calls = 0 const arg = 'path' @@ -60,14 +64,19 @@ t.test('retry when known error code thrown', t => { } } const asyncMethod = async (a: string, b?: unknown) => method(a, b) - const rBS = retryBusySync(method) + const rBS = retryBusySync(method, extraCodes) rBS(arg, opt) t.equal(calls, 2) calls = 0 - const rB = retryBusy(asyncMethod) + const rB = retryBusy(asyncMethod, extraCodes) await rB(arg, opt).then(() => t.equal(calls, 2)) }) + + for (const code of codes) { + await testCode(t, code, undefined) } + + await testCode(t, 'ESOMETHINGELSE', new Set(['ESOMETHINGELSE'])) }) t.test('retry and eventually give up', t => {