Refactor update-lock.lock to accept an array of applications to lock

Change-type: patch
Signed-off-by: 20k-ultra <3946250+20k-ultra@users.noreply.github.com>

src/compose/composition-steps.ts

@@ -120,7 +120,7 @@ type Executors<T extends CompositionStepAction> = {
 type LockingFn = (
 	// TODO: Once the entire codebase is typescript, change
 	// this to number
-	app: number | null,
+	app: number | number[] | null,
 	args: BaseCompositionStepArgs,
 	fn: () => Promise<unknown>,
 ) => Promise<unknown>;

src/lib/update-lock.ts

@@ -71,11 +71,20 @@ export const readLock: LockFn = Bluebird.promisify(locker.async.readLock, {
 });
 
 // Unlock all lockfiles, optionally of an appId | appUuid, then release resources.
-async function dispose(appIdentifier: string | number): Promise<void> {
+async function dispose(
+	appIdentifier: string | number,
+	release: () => void,
+): Promise<void> {
 	const locks = lockfile.getLocksTaken((p: string) =>
 		p.includes(`${BASE_LOCK_DIR}/${appIdentifier}`),
 	);
-	await Promise.all(locks.map((l) => lockfile.unlock(l)));
+	try {
+		// Try to unlock all locks taken
+		await Promise.all(locks.map((l) => lockfile.unlock(l)));
+	} finally {
+		// Release final resource
+		release();
+	}
 }
 
 /**
@@ -88,15 +97,18 @@ async function dispose(appIdentifier: string | number): Promise<void> {
  * without an option to skip.
  */
 export async function lock<T extends unknown>(
-	appId: number,
+	appId: number | number[],
 	{ force = false, skipLock = false }: { force: boolean; skipLock?: boolean },
 	fn: () => Resolvable<T>,
 ): Promise<T> {
-	if (skipLock || appId == null) {
+	const appIdsToLock = Array.isArray(appId) ? appId : [appId];
-		return Promise.resolve(fn());
+	if (skipLock || !appId || !appIdsToLock.length) {
+		return fn();
 	}
 
-	const lockDir = getPathOnHost(lockPath(appId));
+	// Sort appIds so they are always locked in the same sequence
+	const sortedIds = appIdsToLock.sort();
+
 	let lockOverride: boolean;
 	try {
 		lockOverride = await config.get('lockOverride');
@@ -106,58 +118,43 @@ export async function lock<T extends unknown>(
 		);
 	}
 
-	let release;
+	const releases = new Map<number, () => void>();
 	try {
-		// Acquire write lock for appId
+		for (const id of sortedIds) {
-		release = await writeLock(appId);
+			const lockDir = getPathOnHost(lockPath(id));
-		// Get list of service folders in lock directory
+			// Acquire write lock for appId
-		let serviceFolders: string[] = [];
+			releases.set(id, await writeLock(id));
-		try {
+			// Get list of service folders in lock directory
-			serviceFolders = await fs.readdir(lockDir);
+			const serviceFolders = await fs.readdir(lockDir).catch((e) => {
-		} catch (err) {
+				if (ENOENT(e)) {
-			if (ENOENT(err)) {
+					return [];
-				// Default to empty list of folders
+				}
-				serviceFolders = [];
+				throw e;
-			} else {
+			});
-				throw err;
+			// Attempt to create a lock for each service
+			for (const service of serviceFolders) {
+				await lockService(id, service, force || lockOverride);
 			}
 		}
-
-		// Attempt to create a lock for each service
-		await Promise.all(
-			serviceFolders.map((service) =>
-				lockService(appId, service, { force, lockOverride }),
-			),
-		);
-
 		// Resolve the function passed
-		return Promise.resolve(fn());
+		return fn();
 	} finally {
-		// Cleanup locks
+		for (const [id, release] of releases.entries()) {
-		if (release) {
+			// Try to dispose all the locks
-			release();
+			await dispose(id, release);
 		}
-		await dispose(appId);
 	}
 }
 
-type LockOptions = {
-	force?: boolean;
-	lockOverride?: boolean;
-};
-
 async function lockService(
 	appId: number,
 	service: string,
-	opts: LockOptions = {
+	force: boolean = false,
-		force: false,
-		lockOverride: false,
-	},
 ): Promise<void> {
 	const serviceLockFiles = lockFilesOnHost(appId, service);
 	for await (const file of serviceLockFiles) {
 		try {
-			if (opts.force || opts.lockOverride) {
+			if (force) {
 				await lockfile.unlock(file);
 			}
 			await lockfile.lock(file, LOCKFILE_UID);

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

@@ -242,6 +242,45 @@ describe('lib/update-lock', () => {
 			);
 		});
 
+		it('locks all applications before resolving input function', async () => {
+			const appIds = [111, 222, 333];
+			// Set up fake filesystem for lockfiles
+			mockFs({
+				[path.join(
+					constants.rootMountPoint,
+					updateLock.lockPath(111),
+					serviceName,
+				)]: {},
+				[path.join(
+					constants.rootMountPoint,
+					updateLock.lockPath(222),
+					serviceName,
+				)]: {},
+				[path.join(
+					constants.rootMountPoint,
+					updateLock.lockPath(333),
+					serviceName,
+				)]: {},
+			});
+
+			await expect(
+				updateLock.lock(appIds, { force: false }, async () => {
+					// At this point the locks should be taken and not removed
+					// until this function has been resolved
+					// Both `updates.lock` and `resin-updates.lock` should have been taken
+					expect(lockSpy.args).to.have.length(6);
+					// Make sure that no locks have been removed also
+					expect(unlockSpy).to.not.be.called;
+					return Promise.resolve();
+				}),
+			).to.eventually.be.fulfilled;
+
+			// Everything that was locked should have been unlocked after function resolves
+			expect(lockSpy.args.map(([lock]) => [lock])).to.deep.equal(
+				unlockSpy.args,
+			);
+		});
+
 		it('resolves input function without locking when appId is null', async () => {
 			mockLockDir({ createLockfile: true });