Refactor lockfile module

Updated interfaces for clarity

Change-type: patch

src/lib/lockfile.ts

@@ -1,5 +1,4 @@
 import { promises as fs } from 'fs';
-import type { Stats, Dirent } from 'fs';
 import os from 'os';
 import { dirname } from 'path';
 
@@ -9,38 +8,68 @@ import { isENOENT, isEISDIR, isEPERM } from './errors';
 // Equivalent to `drwxrwxrwt`
 const STICKY_WRITE_PERMISSIONS = 0o1777;
 
+interface LockInfo {
+	/**
+	 * The lock file path
+	 */
+	path: string;
+	/**
+	 * The linux user id (uid) of the
+	 * lock
+	 */
+	owner: number;
+}
+
+interface FindAllArgs {
+	root: string;
+	filter: (lock: LockInfo) => boolean;
+	recursive: boolean;
+}
+
 // Returns all current locks taken under a directory (default: /tmp)
 // Optionally accepts filter function for only getting locks that match a condition.
 // A file is counted as a lock by default if it ends with `.lock`.
-export const getLocksTaken = async (
-	rootDir: string = '/tmp',
-	lockFilter: (path: string, stat: Stats) => boolean = (p) =>
-		p.endsWith('.lock'),
-): Promise<string[]> => {
-	const locksTaken: string[] = [];
-	let filesOrDirs: Dirent[] = [];
-	try {
-		filesOrDirs = await fs.readdir(rootDir, { withFileTypes: true });
-	} catch (err) {
-		// If lockfile directory doesn't exist, no locks are taken
-		if (isENOENT(err)) {
-			return locksTaken;
+export async function findAll({
+	root = '/tmp',
+	filter = (l) => l.path.endsWith('.lock'),
+	recursive = true,
+}: Partial<FindAllArgs>): Promise<string[]> {
+	// Queue of directories to search
+	const queue: string[] = [root];
+	const locks: string[] = [];
+
+	while (queue.length > 0) {
+		root = queue.shift()!;
+		try {
+			const contents = await fs.readdir(root, { withFileTypes: true });
+
+			for (const file of contents) {
+				const path = `${root}/${file.name}`;
+				const stats = await fs.lstat(path);
+
+				// A lock is taken if it's a file or directory within root dir that passes filter fn.
+				// We also don't want to follow symlinks since we don't want to follow the lock to
+				// the target path if it's a symlink and only care that it exists or not.
+				if (filter({ path, owner: stats.uid })) {
+					locks.push(path);
+				} else if (file.isDirectory() && recursive) {
+					// Otherwise, if non-lock directory, seek locks recursively within directory
+					queue.push(path);
+				}
+			}
+		} catch (err) {
+			// if file of directory does not exist continue the search
+			// the file, could have been deleted after starting the call
+			// to findAll
+			if (isENOENT(err)) {
+				continue;
+			}
+			throw err;
 		}
 	}
-	for (const fileOrDir of filesOrDirs) {
-		const lockPath = `${rootDir}/${fileOrDir.name}`;
-		// A lock is taken if it's a file or directory within rootDir that passes filter fn.
-		// We also don't want to follow symlinks since we don't want to follow the lock to
-		// the target path if it's a symlink and only care that it exists or not.
-		if (lockFilter(lockPath, await fs.lstat(lockPath))) {
-			locksTaken.push(lockPath);
-			// Otherwise, if non-lock directory, seek locks recursively within directory
-		} else if (fileOrDir.isDirectory()) {
-			locksTaken.push(...(await getLocksTaken(lockPath, lockFilter)));
-		}
-	}
-	return locksTaken;
-};
+
+	return locks;
+}
 
 interface ChildProcessError {
 	code: number;

src/lib/update-lock.ts

@@ -1,6 +1,5 @@
 import { promises as fs } from 'fs';
 import path from 'path';
-import type { Stats } from 'fs';
 import { isRight } from 'fp-ts/lib/Either';
 
 import {
@@ -212,11 +211,10 @@ export class LocksTakenMap extends Map<number, Set<string>> {
 export async function getLocksTaken(
 	rootDir: string = pathOnRoot(BASE_LOCK_DIR),
 ): Promise<string[]> {
-	return await lockfile.getLocksTaken(
-		rootDir,
-		(p: string, s: Stats) =>
-			p.endsWith('updates.lock') && s.uid === LOCKFILE_UID,
-	);
+	return await lockfile.findAll({
+		root: rootDir,
+		filter: (l) => l.path.endsWith('updates.lock') && l.owner === LOCKFILE_UID,
+	});
 }
 
 /**

test/integration/lib/lockfile.spec.ts

@@ -165,7 +165,7 @@ describe('lib/lockfile', () => {
 		await Promise.all(locks.map((lock) => lockfile.lock(lock)));
 
 		// Assert all locks are listed as taken
-		expect(await lockfile.getLocksTaken(lockdir)).to.have.members(
+		expect(await lockfile.findAll({ root: lockdir })).to.have.members(
 			locks.concat([`${lockdir}/other.lock`]),
 		);
 
@@ -194,30 +194,23 @@ describe('lib/lockfile', () => {
 		// Assert appropriate locks are listed as taken...
 		// - with a specific UID
 		expect(
-			await lockfile.getLocksTaken(
-				lockdir,
-				(p, stats) => p.endsWith('.lock') && stats.uid === NOBODY_UID,
-			),
+			await lockfile.findAll({
+				root: lockdir,
+				filter: (lock) =>
+					lock.path.endsWith('.lock') && lock.owner === NOBODY_UID,
+			}),
 		).to.have.members([
 			`${lockdir}/updates.lock`,
 			`${lockdir}/1/resin-updates.lock`,
 			`${lockdir}/other.lock`,
 		]);
-		// - as a directory
-		expect(
-			await lockfile.getLocksTaken(
-				lockdir,
-				(p, stats) => p.endsWith('.lock') && stats.isDirectory(),
-			),
-		).to.have.members([
-			`${lockdir}/1/updates.lock`,
-			`${lockdir}/1/resin-updates.lock`,
-		]);
+
 		// - under a different root dir from default
 		expect(
-			await lockfile.getLocksTaken(`${lockdir}/services`, (p) =>
-				p.endsWith('.lock'),
-			),
+			await lockfile.findAll({
+				root: `${lockdir}/services`,
+				filter: (lock) => lock.path.endsWith('.lock'),
+			}),
 		).to.have.members([
 			`${lockdir}/services/main/updates.lock`,
 			`${lockdir}/services/aux/resin-updates.lock`,
@@ -233,9 +226,10 @@ describe('lib/lockfile', () => {
 		// Create symlink lock
 		await fs.symlink('/nonexistent', `${lockdir}/updates.lock`);
 
-		expect(
-			await lockfile.getLocksTaken(lockdir, (_p, s) => s.isSymbolicLink()),
-		).to.have.members([`${lockdir}/updates.lock`]);
+		expect(await lockfile.findAll({ root: lockdir })).to.have.members([
+			`${lockdir}/other.lock`,
+			`${lockdir}/updates.lock`,
+		]);
 
 		// Cleanup symlink lock
 		await fs.rm(`${lockdir}/updates.lock`);

test/integration/lib/update-lock.spec.ts

@@ -634,9 +634,9 @@ describe('lib/update-lock', () => {
 
 				// Take lock for second service of two services
 				await lockfile.lock(`${lockdir}/1/${svcs[1]}/updates.lock`);
-				expect(await lockfile.getLocksTaken(lockdir)).to.deep.include.members([
-					`${lockdir}/1/${svcs[1]}/updates.lock`,
-				]);
+				expect(
+					await lockfile.findAll({ root: lockdir }),
+				).to.deep.include.members([`${lockdir}/1/${svcs[1]}/updates.lock`]);
 
 				// Watch for added files, as Supervisor-taken locks should be added
 				// then removed within updateLock.takeLock
@@ -656,16 +656,16 @@ describe('lib/update-lock', () => {
 
 				// ..but upon error, Supervisor-taken locks should have been cleaned up
 				expect(
-					await lockfile.getLocksTaken(lockdir),
+					await lockfile.findAll({ root: lockdir }),
 				).to.not.deep.include.members([
 					`${lockdir}/1/${svcs[0]}/updates.lock`,
 					`${lockdir}/1/${svcs[0]}/resin-updates.lock`,
 				]);
 
 				// User lock should be left behind
-				expect(await lockfile.getLocksTaken(lockdir)).to.deep.include.members([
-					`${lockdir}/1/${svcs[1]}/updates.lock`,
-				]);
+				expect(
+					await lockfile.findAll({ root: lockdir }),
+				).to.deep.include.members([`${lockdir}/1/${svcs[1]}/updates.lock`]);
 
 				// Clean up watcher
 				await watcher.close();