fix(cli): catch all possible errors thrown from proper-filelock (#5347)

packages/utils/src/locking.js

@@ -35,16 +35,33 @@ export async function getLockPath(config) {
 export async function lock({ id, dir, root, options }) {
   const lockPath = await getLockPath({ id, dir, root })
 
-  const locked = await properlock.check(lockPath)
-  if (locked) {
-    consola.fatal(`A lock with id '${id}' already exists on ${dir}`)
+  try {
+    const locked = await properlock.check(lockPath)
+    if (locked) {
+      consola.fatal(`A lock with id '${id}' already exists on ${dir}`)
+    }
+  } catch (e) {
+    consola.debug(`Check for an existing lock with id '${id}' on ${dir} failed`, e)
   }
 
-  options = getLockOptions(options)
-  const release = await properlock.lock(lockPath, options)
+  let lockWasCompromised = false
+  let release
+
+  try {
+    options = getLockOptions(options)
+
+    const onCompromised = options.onCompromised
+    options.onCompromised = (err) => {
+      onCompromised(err)
+      lockWasCompromised = true
+    }
+
+    release = await properlock.lock(lockPath, options)
+  } catch (e) {}
 
   if (!release) {
     consola.warn(`Unable to get a lock with id '${id}' on ${dir} (but will continue)`)
+    return false
   }
 
   if (!lockPaths.size) {
@@ -59,8 +76,27 @@ export async function lock({ id, dir, root, options }) {
   lockPaths.add(lockPath)
 
   return async function lockRelease() {
-    await release()
-    await fs.remove(lockPath)
-    lockPaths.delete(lockPath)
+    try {
+      await fs.remove(lockPath)
+      lockPaths.delete(lockPath)
+
+      // release as last so the lockPath is still removed
+      // when it fails on a compromised lock
+      await release()
+    } catch (e) {
+      if (!lockWasCompromised || !e.message.includes('already released')) {
+        consola.debug(e)
+        return
+      }
+
+      // proper-lockfile doesnt remove lockDir when lock is compromised
+      // removing it here could cause the 'other' process to throw an error
+      // as well, but in our case its much more likely the lock was
+      // compromised due to mtime update timeouts
+      const lockDir = `${lockPath}.lock`
+      if (await fs.exists(lockDir)) {
+        await fs.remove(lockDir)
+      }
+    }
   }
 }

packages/utils/test/locking.test.js

@@ -61,7 +61,7 @@ describe('util: locking', () => {
   })
 
   test('lock creates a lock and returns a release fn', async () => {
-    properlock.lock.mockImplementationOnce(() => true)
+    properlock.lock.mockReturnValue(true)
 
     const fn = await lock(lockConfig)
 
@@ -75,7 +75,7 @@ describe('util: locking', () => {
   })
 
   test('lock throws error when lock already exists', async () => {
-    properlock.check.mockImplementationOnce(() => true)
+    properlock.check.mockReturnValue(true)
 
     await lock(lockConfig)
     expect(properlock.check).toHaveBeenCalledTimes(1)
@@ -84,7 +84,19 @@ describe('util: locking', () => {
   })
 
   test('lock logs warning when it couldnt get a lock', async () => {
-    properlock.lock.mockImplementationOnce(() => false)
+    properlock.lock.mockReturnValue(false)
+
+    const fn = await lock(lockConfig)
+    expect(fn).toBe(false)
+    expect(properlock.lock).toHaveBeenCalledTimes(1)
+    expect(consola.warn).toHaveBeenCalledTimes(1)
+    expect(consola.warn).toHaveBeenCalledWith(`Unable to get a lock with id '${lockConfig.id}' on ${lockConfig.dir} (but will continue)`)
+  })
+
+  test('lock logs warning when proper.lock threw error', async () => {
+    properlock.lock.mockImplementation(() => {
+      throw new Error('test error')
+    })
 
     await lock(lockConfig)
     expect(properlock.lock).toHaveBeenCalledTimes(1)
@@ -94,7 +106,7 @@ describe('util: locking', () => {
 
   test('lock returns a release method for unlocking both lockfile as lockPath', async () => {
     const release = jest.fn()
-    properlock.lock.mockImplementationOnce(() => release)
+    properlock.lock.mockImplementation(() => release)
 
     const fn = await lock(lockConfig)
     await fn()
@@ -105,7 +117,7 @@ describe('util: locking', () => {
 
   test('lock release also cleansup onExit set', async () => {
     const release = jest.fn()
-    properlock.lock.mockImplementationOnce(() => release)
+    properlock.lock.mockImplementation(() => release)
 
     const fn = await lock(lockConfig)
     expect(lockPaths.size).toBe(1)
@@ -114,8 +126,58 @@ describe('util: locking', () => {
     expect(lockPaths.size).toBe(0)
   })
 
+  test('lock release only logs error when error thrown', async () => {
+    const release = jest.fn(() => {
+      throw new Error('test error')
+    })
+    properlock.lock.mockImplementation(() => release)
+
+    const fn = await lock(lockConfig)
+    await expect(fn()).resolves.not.toThrow()
+
+    expect(consola.debug).toHaveBeenCalledTimes(1)
+  })
+
+  test('lock check only logs error when error thrown', async () => {
+    const testError = new Error('check error')
+    properlock.lock.mockImplementation(() => () => {})
+    properlock.check.mockImplementation(() => {
+      throw testError
+    })
+
+    const fn = await lock(lockConfig)
+    expect(fn).toEqual(expect.any(Function))
+
+    expect(consola.debug).toHaveBeenCalledTimes(1)
+    expect(consola.debug).toHaveBeenCalledWith(`Check for an existing lock with id '${lockConfig.id}' on ${lockConfig.dir} failed`, testError)
+  })
+
+  test('lock release doesnt log error when error thrown because lock compromised', async () => {
+    fs.exists.mockReturnValue(true)
+    const testError = new Error('Lock is already released')
+    const release = jest.fn(() => {
+      throw testError
+    })
+
+    properlock.lock.mockImplementation((path, options) => {
+      options.onCompromised()
+      return release
+    })
+
+    const fn = await lock({
+      ...lockConfig,
+      options: {
+        // overwrite default compromised which calls consola.warn
+        onCompromised() {}
+      }
+    })
+
+    await expect(fn()).resolves.not.toThrow()
+    expect(consola.warn).not.toHaveBeenCalled()
+  })
+
   test('lock sets exit listener once to remove lockPaths', async () => {
-    properlock.lock.mockImplementationOnce(() => true)
+    properlock.lock.mockReturnValue(true)
 
     await lock(lockConfig)
     await lock(lockConfig)
@@ -124,8 +186,10 @@ describe('util: locking', () => {
   })
 
   test('exit listener removes all lockPaths when called', async () => {
+    properlock.lock.mockReturnValue(true)
+
     let callback
-    onExit.mockImplementationOnce(cb => (callback = cb))
+    onExit.mockImplementation(cb => (callback = cb))
 
     const lockConfig2 = Object.assign({}, lockConfig, { id: 'id2' })
 
@@ -145,7 +209,7 @@ describe('util: locking', () => {
   })
 
   test('lock uses setLockOptions to set defaults', async () => {
-    const spy = properlock.lock.mockImplementationOnce(() => true)
+    const spy = properlock.lock.mockReturnValue(true)
 
     await lock(lockConfig)