fix(rm/polyfill): handle ENOENT in rmdir more appropriately

an ENOENT from rmdir should follow the same rules as every
other error handler, with one major exception. in windows,
if the path passed to rmdir is not a directory an ENOENT is
thrown instead of the ENOTDIR present in other platforms.
to determine if we should ignore the ENOENT or throw our own
ENOTDIR, we use an lstat to see if the path still exists

Author: nlf

lib/rm/polyfill.js

@@ -9,7 +9,8 @@
 // - remove all callback related code
 // - drop sync support
 // - change assertions back to non-internal methods (see options.js)
-const { EISDIR } = require('os').constants.errno
+// - throws ENOTDIR when rmdir gets an ENOENT for a path that exists in Windows
+const errnos = require('os').constants.errno
 const { join } = require('path')
 const fs = require('../fs.js')
 
@@ -48,21 +49,37 @@ class ERR_FS_EISDIR extends Error {
       message: 'is a directory',
       path,
       syscall: 'rm',
-      errno: EISDIR,
+      errno: errnos.EISDIR,
     }
     this.name = 'SystemError'
     this.code = 'ERR_FS_EISDIR'
-    this.errno = EISDIR
-    this.message = `Path is a directory: rm returned EISDIR (is a directory) ${path}`
+    this.errno = errnos.EISDIR
     this.syscall = 'rm'
     this.path = path
+    this.message = `Path is a directory: ${this.syscall} returned ${this.info.code} (is a directory) ${path}`
   }
 
   toString () {
     return `${this.name} [${this.code}]: ${this.message}`
   }
 }
 
+class ENOTDIR extends Error {
+  constructor (path) {
+    super()
+    this.name = 'Error'
+    this.code = 'ENOTDIR'
+    this.errno = errnos.ENOTDIR
+    this.syscall = 'rmdir'
+    this.path = path
+    this.message = `not a directory, ${this.syscall} '${this.path}'`
+  }
+
+  toString () {
+    return `${this.name}: ${this.code}: ${this.message}`
+  }
+}
+
 // force is passed separately here because we respect it for the first entry
 // into rimraf only, any further calls that are spawned as a result (i.e. to
 // delete content within the target) will ignore ENOENT errors
@@ -158,9 +175,26 @@ const rmdir = async (path, options, originalErr, isTop) => {
   if (!options.recursive && isTop) {
     throw originalErr || new ERR_FS_EISDIR(path)
   }
+  const force = isTop ? options.force : true
 
   return fs.rmdir(path)
     .catch(async (err) => {
+      // in Windows, calling rmdir on a file path will fail with ENOENT rather
+      // than ENOTDIR. to determine if that's what happened, we have to do
+      // another lstat on the path. if the path isn't actually gone, we throw
+      // away the ENOENT and replace it with our own ENOTDIR
+      if (isWindows && err.code === 'ENOENT') {
+        const stillExists = await fs.lstat(path).then(() => true, () => false)
+        if (stillExists) {
+          err = new ENOTDIR(path)
+        }
+      }
+
+      // not there, not a problem
+      if (err.code === 'ENOENT' && force) {
+        return
+      }
+
       // we may not have originalErr if lstat tells us our target is a
       // directory but that changes before we actually remove it, so
       // only throw it here if it's set

test/rm/polyfill.js

@@ -18,6 +18,7 @@ class ErrorCode extends Error {
 const EISDIR = new ErrorCode('EISDIR')
 const EMFILE = new ErrorCode('EMFILE')
 const ENOENT = new ErrorCode('ENOENT')
+const ENOTDIR = new ErrorCode('ENOTDIR')
 const EPERM = new ErrorCode('EPERM')
 const EUNKNOWN = new ErrorCode('EUNKNOWN') // fake error code for else coverage
 
@@ -86,6 +87,23 @@ t.test('can delete a directory', async (t) => {
   t.equal(await fs.exists(target), false, 'target no longer exists')
 })
 
+t.test('resolves when rmdir gets ENOENT with force', async (t) => {
+  const dir = t.testdir()
+  // doesn't actually exist
+  const target = join(dir, 'directory')
+
+  const lstat = realFs.lstat
+  realFs.lstat = (path, cb) => {
+    realFs.lstat = lstat
+    setImmediate(cb, null, { isDirectory: () => true })
+  }
+  t.teardown(() => {
+    realFs.lstat = lstat
+  })
+
+  await t.resolves(rm(target, { recursive: true, force: true }))
+})
+
 t.test('rejects with EISDIR when deleting a directory without recursive', async (t) => {
   const dir = t.testdir({
     directory: {},
@@ -198,6 +216,43 @@ t.test('rejects with unknown error removing top directory', async (t) => {
   })
 })
 
+t.test('posix', async (t) => {
+  let posixRm
+  t.before(() => {
+    t.context.platform = Object.getOwnPropertyDescriptor(process, 'platform')
+    Object.defineProperty(process, 'platform', {
+      ...t.context.platform,
+      value: 'linux',
+    })
+    posixRm = t.mock('../../lib/rm/polyfill.js')
+  })
+
+  t.teardown(() => {
+    Object.defineProperty(process, 'platform', t.context.platform)
+  })
+
+  t.test('EPERM in unlink calls rmdir', async (t) => {
+    const dir = t.testdir({
+      file: 'a file',
+    })
+    const target = join(dir, 'file')
+
+    const rmdir = realFs.rmdir
+    const unlink = realFs.unlink
+    // need to mock rmdir too so we can force an ENOTDIR
+    realFs.rmdir = (path, cb) => setImmediate(cb, ENOTDIR)
+    realFs.unlink = (path, cb) => setImmediate(cb, EPERM)
+    t.teardown(() => {
+      realFs.rmdir = rmdir
+      realFs.unlink = unlink
+    })
+
+    await t.rejects(posixRm(target, { recursive: true }), {
+      code: 'EPERM',
+    }, 'got the EPERM')
+  })
+})
+
 t.test('windows', async (t) => {
   // t.mock instead of require so we flush the cache first
   let winRm
@@ -454,4 +509,84 @@ t.test('windows', async (t) => {
     t.not(await fs.exists(join(target, 'file')), 'file no longer exists')
     t.not(await fs.exists(target), 'target no longer exists')
   })
+
+  t.test('ENOENT in rmdir resolves when file is really gone with force', async (t) => {
+    const dir = t.testdir({
+      directory: {},
+    })
+    const target = join(dir, 'directory')
+
+    let rmdirCalled = false
+    const lstat = realFs.lstat
+    const rmdir = realFs.rmdir
+    realFs.rmdir = (path, cb) => {
+      rmdirCalled = true
+      realFs.lstat = (path, cb) => setImmediate(cb, ENOENT)
+      setImmediate(cb, ENOENT)
+    }
+    t.teardown(() => {
+      realFs.lstat = lstat
+      realFs.rmdir = rmdir
+    })
+
+    await winRm(target, { recursive: true, force: true })
+    t.ok(rmdirCalled, 'rmdir() was called')
+  })
+
+  t.test('ENOENT in rmdir rejects with ENOENT when file is really gone without force', async (t) => {
+    const dir = t.testdir({
+      directory: {},
+    })
+    const target = join(dir, 'directory')
+
+    const lstat = realFs.lstat
+    const rmdir = realFs.rmdir
+    realFs.rmdir = (path, cb) => {
+      // need to hijack this here so only the lstat after rmdir gets the ENOENT
+      realFs.lstat = (path, cb) => setImmediate(cb, ENOENT)
+      setImmediate(cb, ENOENT)
+    }
+    t.teardown(() => {
+      realFs.lstat = lstat
+      realFs.rmdir = rmdir
+    })
+
+    await t.rejects(winRm(target, { recursive: true }), {
+      code: 'ENOENT',
+    }, 'got the ENOENT')
+  })
+
+  t.test('ENOENT in rmdir rejects with ENOTDIR when target still exists', async (t) => {
+    const dir = t.testdir({
+      directory: {},
+    })
+    const target = join(dir, 'directory')
+
+    const rmdir = realFs.rmdir
+    realFs.rmdir = (path, cb) => setImmediate(cb, ENOENT)
+    t.teardown(() => {
+      realFs.rmdir = rmdir
+    })
+
+    await t.rejects(winRm(target, { recursive: true }), {
+      code: 'ENOTDIR',
+    }, 'got the ENOTDIR')
+  })
+
+  t.test('ENOENT in rmdir rejects with ENOTDIR when target still exists and force is set', async (t) => {
+    const dir = t.testdir({
+      directory: {},
+    })
+    const target = join(dir, 'directory')
+
+    const rmdir = realFs.rmdir
+    realFs.rmdir = (path, cb) => setImmediate(cb, ENOENT)
+    t.teardown(() => {
+      realFs.rmdir = rmdir
+    })
+
+    await t.rejects(winRm(target, { recursive: true, force: true }), {
+      code: 'ENOTDIR',
+    }, 'got the ENOTDIR')
+  })
 })