diff --git a/src/lib/lockfile.ts b/src/lib/lockfile.ts index 149ce925..5e3e1c61 100644 --- a/src/lib/lockfile.ts +++ b/src/lib/lockfile.ts @@ -109,8 +109,8 @@ export async function lock(path: string, uid: number = os.userInfo().uid) { export async function unlock(path: string): Promise { // Removing the lockfile releases the lock await fs.unlink(path).catch((e) => { - // if the error is EPERM, the file is a directory - if (e.code === 'EPERM') { + // if the error is EPERM|EISDIR, the file is a directory + if (e.code === 'EPERM' || e.code === 'EISDIR') { return fs.rmdir(path).catch(() => { // if the directory is not empty or something else // happens, ignore @@ -127,7 +127,7 @@ export function unlockSync(path: string) { try { return unlinkSync(path); } catch (e) { - if (e.code === 'EPERM') { + if (e.code === 'EPERM' || e.code === 'EISDIR') { return rmdirSync(path); } throw e; diff --git a/test/integration/lib/lockfile.spec.ts b/test/integration/lib/lockfile.spec.ts index 96d29467..72e89145 100644 --- a/test/integration/lib/lockfile.spec.ts +++ b/test/integration/lib/lockfile.spec.ts @@ -1,196 +1,200 @@ import { expect } from 'chai'; -import { dirname, basename } from 'path'; -import { stub, SinonStub } from 'sinon'; -import { promises as fs } from 'fs'; -import mock = require('mock-fs'); - +import { promises as fs, mkdirSync } from 'fs'; +import { testfs, TestFs } from 'mocha-pod'; +import * as os from 'os'; +import * as path from 'path'; +import { stub } from 'sinon'; import * as lockfile from '~/lib/lockfile'; import * as fsUtils from '~/lib/fs-utils'; -const BASE_LOCK_DIR = '/tmp/balena-supervisor/services'; -const LOCKFILE_UID = 65534; + +const NOBODY_UID = 65534; describe('lib/lockfile', () => { - const lockPath = `${BASE_LOCK_DIR}/1234567/one/updates.lock`; - const lockPath2 = `${BASE_LOCK_DIR}/7654321/two/updates.lock`; + const lockdir = '/tmp/lockdir'; - const mockDir = (opts: { createLock: boolean } = { createLock: false }) => { - mock({ - [BASE_LOCK_DIR]: { - '1234567': { - one: opts.createLock - ? { 'updates.lock': mock.file({ uid: LOCKFILE_UID }) } - : {}, - }, - '7654321': { - two: opts.createLock - ? { 'updates.lock': mock.directory({ uid: LOCKFILE_UID }) } - : {}, + let testFs: TestFs.Enabled; + + beforeEach(async () => { + testFs = await testfs( + { + [lockdir]: { + 'other.lock': testfs.file({ uid: NOBODY_UID }), }, }, - }); - }; - - const checkLockDirFiles = async ( - path: string, - opts: { shouldExist: boolean } = { shouldExist: true }, - ) => { - const files = await fs.readdir(dirname(path)); - if (opts.shouldExist) { - expect(files).to.include(basename(path)); - } else { - expect(files).to.have.length(0); - } - }; - - let execStub: SinonStub; - - beforeEach(() => { - // @ts-ignore - execStub = stub(fsUtils, 'exec').callsFake(async (command, opts) => { - // Sanity check for the command call - expect(command.trim().startsWith('lockfile')).to.be.true; - - // Remove any `lockfile` command options to leave just the command and the target filepath - const [, targetPath] = command - .replace(/-v|-nnn|-r\s+\d+|-l\s+\d+|-s\s+\d+|-!|-ml|-mu/g, '') - .split(/\s+/); - - // Emulate the lockfile binary exec call - await fsUtils.touch(targetPath); - await fs.chown(targetPath, opts!.uid!, 0); - }); - - mock({ [BASE_LOCK_DIR]: {} }); + { cleanup: [path.join(lockdir, '**.lock')] }, + ).enable(); }); afterEach(async () => { - execStub.restore(); + await testFs.restore(); + }); - // Even though mock-fs is restored, this is needed to delete any in-memory storage of locks - for (const lock of lockfile.getLocksTaken()) { - await lockfile.unlock(lock); - } + it('should create a lockfile as the current user by default', async () => { + const lock = path.join(lockdir, 'updates.lock'); - mock.restore(); + // Take the lock passing a uid + await expect(lockfile.lock(lock)).to.not.be.rejected; + + // The file should exist + await expect(fs.access(lock)).to.not.be.rejected; + + // Verify lockfile UID + expect((await fs.stat(lock)).uid).to.equal(os.userInfo().uid); }); it('should create a lockfile as the `nobody` user at target path', async () => { - mockDir(); + const lock = path.join(lockdir, 'updates.lock'); - await lockfile.lock(lockPath, LOCKFILE_UID); + // Take the lock passing a uid + await expect(lockfile.lock(lock, NOBODY_UID)).to.not.be.rejected; - // Verify lockfile exists - await checkLockDirFiles(lockPath, { shouldExist: true }); + // The file should exist + await expect(fs.access(lock)).to.not.be.rejected; // Verify lockfile UID - expect((await fs.stat(lockPath)).uid).to.equal(LOCKFILE_UID); + expect((await fs.stat(lock)).uid).to.equal(NOBODY_UID); + }); + + it('should not be able to take the lock if it already exists', async () => { + const lock = path.join(lockdir, 'updates.lock'); + + // Take the lock passing a uid + await expect(lockfile.lock(lock, NOBODY_UID)).to.not.be.rejected; + + // The file should exist + await expect(fs.access(lock)).to.not.be.rejected; + + // Trying to take the lock again should fail + await expect(lockfile.lock(lock, NOBODY_UID)).to.be.rejected; }); it('should create a lockfile with the provided `uid` if specified', async () => { - mockDir(); + const lock = path.join(lockdir, 'updates.lock'); - await lockfile.lock(lockPath, 2); + // Take the lock passing a uid + await expect(lockfile.lock(lock, 2)).to.not.be.rejected; - // Verify lockfile exists - await checkLockDirFiles(lockPath, { shouldExist: true }); + // The file should exist + await expect(fs.access(lock)).to.not.be.rejected; // Verify lockfile UID - expect((await fs.stat(lockPath)).uid).to.equal(2); + expect((await fs.stat(lock)).uid).to.equal(2); }); it('should not create a lockfile if `lock` throws', async () => { - mockDir(); - - // Override default exec stub declaration, as it normally emulates a lockfile call with - // no errors, but we want it to throw an error just for this unit test - execStub.restore(); - - const childProcessError = new lockfile.LockfileExistsError( - '/tmp/test/path', + // Stub the call to exec. + // WARNING: This is relying on internal knowledge of the function + // which is generally not a good testing practice, but I'm not sure + // how to do it otherwise + const execStub = stub(fsUtils, 'exec').throws( + new Error('Something bad happened'), ); - execStub = stub(fsUtils, 'exec').throws(childProcessError); - try { - await lockfile.lock(lockPath, LOCKFILE_UID); - expect.fail('lockfile.lock should throw an error'); - } catch (err) { - expect(err).to.exist; - } + const lock = path.join(lockdir, 'updates.lock'); - // Verify lockfile does not exist - await checkLockDirFiles(lockPath, { shouldExist: false }); + // Take the lock passing a uid + await expect(lockfile.lock(lock, NOBODY_UID)).to.be.rejected; + + // The file should not have been created + await expect(fs.access(lock)).to.be.rejected; + + // Restore the stub + execStub.restore(); }); it('should asynchronously unlock a lockfile', async () => { - mockDir({ createLock: true }); + const lock = path.join(lockdir, 'updates.lock'); - // Verify lockfile exists - await checkLockDirFiles(lockPath, { shouldExist: true }); + // Take the lock passing a uid + await expect(lockfile.lock(lock)).to.not.be.rejected; - await lockfile.unlock(lockPath); - await lockfile.unlock(lockPath2); + // The file should exist + await expect(fs.access(lock)).to.not.be.rejected; - // Verify lockfile removal - await checkLockDirFiles(lockPath, { shouldExist: false }); - await checkLockDirFiles(lockPath2, { shouldExist: false }); + // Unlock should never throw + await expect(lockfile.unlock(lock)).to.not.be.rejected; + + // The file should no longer exist + await expect(fs.access(lock)).to.be.rejected; + }); + + it('should asynchronously unlock a lock directory', async () => { + const lock = path.join(lockdir, 'updates.lock'); + + // Crete a lock directory + await fs.mkdir(lock, { recursive: true }); + + // The directory should exist + await expect(fs.access(lock)).to.not.be.rejected; + + // Unlock should never throw + await expect(lockfile.unlock(lock)).to.not.be.rejected; + + // The file should no longer exist + await expect(fs.access(lock)).to.be.rejected; }); it('should not error on async unlock if lockfile does not exist', async () => { - mockDir({ createLock: false }); + const lock = path.join(lockdir, 'updates.lock'); - // Verify lockfile does not exist - await checkLockDirFiles(lockPath, { shouldExist: false }); + // The file should not exist before + await expect(fs.access(lock)).to.be.rejected; - try { - await lockfile.unlock(lockPath); - } catch (err) { - expect.fail((err as Error)?.message ?? err); - } + // Unlock should never throw + await expect(lockfile.unlock(lock)).to.not.be.rejected; + + // The file should still not exist + await expect(fs.access(lock)).to.be.rejected; }); it('should synchronously unlock a lockfile', () => { - mockDir({ createLock: true }); + const lock = path.join(lockdir, 'other.lock'); - lockfile.unlockSync(lockPath); - lockfile.unlockSync(lockPath2); + lockfile.unlockSync(lock); // Verify lockfile does not exist - return Promise.all([ - checkLockDirFiles(lockPath, { shouldExist: false }).catch((err) => { - expect.fail((err as Error)?.message ?? err); - }), - checkLockDirFiles(lockPath2, { shouldExist: false }), - ]); + return expect(fs.access(lock)).to.be.rejected; + }); + + it('should synchronously unlock a lockfile dir', () => { + const lock = path.join(lockdir, 'update.lock'); + + mkdirSync(lock, { recursive: true }); + + lockfile.unlockSync(lock); + + // Verify lockfile does not exist + return expect(fs.access(lock)).to.be.rejected; }); it('should try to clean up existing locks on process exit', async () => { - mockDir({ createLock: false }); - - // Create lockfiles for multiple appId / uuids - await lockfile.lock(lockPath, LOCKFILE_UID); - await lockfile.lock(lockPath2, LOCKFILE_UID); + // Create lockfiles + const lockOne = path.join(lockdir, 'updates.lock'); + const lockTwo = path.join(lockdir, 'two.lock'); + await expect(lockfile.lock(lockOne)).to.not.be.rejected; + await expect(lockfile.lock(lockTwo, NOBODY_UID)).to.not.be.rejected; // @ts-ignore process.emit('exit'); // Verify lockfile removal regardless of appId / appUuid - await checkLockDirFiles(lockPath, { shouldExist: false }); - await checkLockDirFiles(lockPath2, { shouldExist: false }); + await expect(fs.access(lockOne)).to.be.rejected; + await expect(fs.access(lockTwo)).to.be.rejected; }); - it('should list locks taken according to a filter function', async () => { - mockDir({ createLock: false }); - - // Create lockfiles for multiple appId / uuids - await lockfile.lock(lockPath, LOCKFILE_UID); - await lockfile.lock(lockPath2, LOCKFILE_UID); + it('allows to list locks taken according to a filter function', async () => { + // Create multiple lockfiles + const lockOne = path.join(lockdir, 'updates.lock'); + const lockTwo = path.join(lockdir, 'two.lock'); + await expect(lockfile.lock(lockOne)).to.not.be.rejected; + await expect(lockfile.lock(lockTwo, NOBODY_UID)).to.not.be.rejected; expect( - lockfile.getLocksTaken((path) => path.includes('1234567')), - ).to.have.members([lockPath]); + lockfile.getLocksTaken((filepath) => filepath.includes('lockdir')), + ).to.have.members([lockOne, lockTwo]); expect( - lockfile.getLocksTaken((path) => path.includes('7654321')), - ).to.have.members([lockPath2]); - expect(lockfile.getLocksTaken()).to.have.members([lockPath, lockPath2]); + lockfile.getLocksTaken((filepath) => filepath.includes('two')), + ).to.have.members([lockTwo]); + expect(lockfile.getLocksTaken()).to.have.members([lockOne, lockTwo]); }); });