Add update lock release functionality to state funnel

releaseLock is a step that will be inferred if there are services
in target state, and if some of those services have locks taken by
the Supervisor.

The releaseLock composition step calls the method of the same name
in the updateLock module, which takes the exclusive process lock before
disposing all Supervisor lockfiles in the target appId.

This is half of the update lock incorporation into the state funnel, as
we also need to introduce a takeLock step which triggers during crucial
stages of device state transition.

Signed-off-by: Christina Ying Wang <christina@balena.io>

src/compose/app.ts

@@ -25,6 +25,7 @@ import { checkTruthy } from '../lib/validation';
 import type { ServiceComposeConfig, DeviceMetadata } from './types/service';
 import { pathExistsOnRoot } from '../lib/host-utils';
 import { isSupervisor } from '../lib/supervisor-metadata';
+import type { LocksTakenMap } from '../lib/update-lock';
 
 export interface AppConstructOpts {
 	appId: number;
@@ -43,6 +44,7 @@ export interface UpdateState {
 	availableImages: Image[];
 	containerIds: Dictionary<string>;
 	downloading: string[];
+	locksTaken: LocksTakenMap;
 }
 
 interface ChangingPair<T> {
@@ -166,17 +168,29 @@ export class App {
 			),
 		);
 
-		if (
-			steps.length === 0 &&
-			target.commit != null &&
-			this.commit !== target.commit
-		) {
-			steps.push(
-				generateStep('updateCommit', {
-					target: target.commit,
-					appId: this.appId,
-				}),
-			);
+		if (steps.length === 0) {
+			// Update commit in db if different
+			if (target.commit != null && this.commit !== target.commit) {
+				steps.push(
+					generateStep('updateCommit', {
+						target: target.commit,
+						appId: this.appId,
+					}),
+				);
+			} else if (
+				target.services.length > 0 &&
+				target.services.some(({ appId, serviceName }) =>
+					state.locksTaken.isLocked(appId, serviceName),
+				)
+			) {
+				// Release locks for current services before settling state.
+				// Current services should be the same as target services at this point.
+				steps.push(
+					generateStep('releaseLock', {
+						appId: target.appId,
+					}),
+				);
+			}
 		}
 		return steps;
 	}

src/compose/application-manager.ts

@@ -17,7 +17,7 @@ import {
 	ContractViolationError,
 	InternalInconsistencyError,
 } from '../lib/errors';
-import { lock } from '../lib/update-lock';
+import { getServicesLockedByAppId, lock } from '../lib/update-lock';
 import { checkTruthy } from '../lib/validation';
 
 import App from './app';
@@ -149,6 +149,7 @@ export async function getRequiredSteps(
 		downloading,
 		availableImages,
 		containerIdsByAppId,
+		locksTaken: getServicesLockedByAppId(),
 	});
 }
 
@@ -165,6 +166,7 @@ export async function inferNextSteps(
 		containerIdsByAppId = {} as {
 			[appId: number]: UpdateState['containerIds'];
 		},
+		locksTaken = getServicesLockedByAppId(),
 	} = {},
 ) {
 	const currentAppIds = Object.keys(currentApps).map((i) => parseInt(i, 10));
@@ -216,6 +218,7 @@ export async function inferNextSteps(
 							availableImages,
 							containerIds: containerIdsByAppId[id],
 							downloading,
+							locksTaken,
 						},
 						targetApps[id],
 					),
@@ -229,6 +232,7 @@ export async function inferNextSteps(
 						keepVolumes,
 						downloading,
 						containerIds: containerIdsByAppId[id],
+						locksTaken,
 					}),
 				);
 			}
@@ -252,6 +256,7 @@ export async function inferNextSteps(
 							availableImages,
 							containerIds: containerIdsByAppId[id] ?? {},
 							downloading,
+							locksTaken,
 						},
 						targetApps[id],
 					),

src/compose/composition-steps.ts

@@ -11,6 +11,7 @@ import * as volumeManager from './volume-manager';
 import type Volume from './volume';
 import * as commitStore from './commit';
 import { checkTruthy } from '../lib/validation';
+import * as updateLock from '../lib/update-lock';
 import type { DeviceLegacyReport } from '../types/state';
 
 interface BaseCompositionStepArgs {
@@ -94,6 +95,12 @@ interface CompositionStepArgs {
 	};
 	ensureSupervisorNetwork: object;
 	noop: object;
+	takeLock: {
+		appId: number;
+	};
+	releaseLock: {
+		appId: number;
+	};
 }
 
 export type CompositionStepAction = keyof CompositionStepArgs;
@@ -276,6 +283,12 @@ export function getExecutors(app: {
 		noop: async () => {
 			/* async noop */
 		},
+		takeLock: async () => {
+			// TODO
+		},
+		releaseLock: async (step) => {
+			await updateLock.releaseLock(step.appId);
+		},
 	};
 
 	return executors;

src/lib/update-lock.ts

@@ -55,7 +55,11 @@ export function abortIfHUPInProgress({
 	});
 }
 
-// Unlock all lockfiles of an appId | appUuid, then release resources.
+/**
+ * Unlock all lockfiles of an appId | appUuid, then release resources.
+ * Meant for use in update-lock module only as as it assumes that a
+ * write lock has been acquired.
+ */
 async function dispose(
 	appIdentifier: string | number,
 	release: () => void,
@@ -72,6 +76,15 @@ async function dispose(
 	}
 }
 
+/**
+ * Composition step used by Supervisor compose module.
+ * Release all locks for an appId | appUuid.
+ */
+export async function releaseLock(appId: number) {
+	const release = await takeGlobalLockRW(appId);
+	await dispose(appId, release);
+}
+
 /**
  * Given a lockfile path `p`, return a tuple [appId, serviceName] of that path.
  * Paths are assumed to end in the format /:appId/:serviceName/(resin-)updates.lock.

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

@@ -3,12 +3,15 @@ import * as path from 'path';
 import { promises as fs } from 'fs';
 import { testfs } from 'mocha-pod';
 import type { TestFs } from 'mocha-pod';
+import { setTimeout } from 'timers/promises';
 
 import * as updateLock from '~/lib/update-lock';
 import { UpdatesLockedError } from '~/lib/errors';
 import * as config from '~/src/config';
 import * as lockfile from '~/lib/lockfile';
 import { pathOnRoot, pathOnState } from '~/lib/host-utils';
+import { mkdirp } from '~/lib/fs-utils';
+import { takeGlobalLockRW } from '~/lib/process-lock';
 
 describe('lib/update-lock', () => {
 	describe('abortIfHUPInProgress', () => {
@@ -353,4 +356,112 @@ describe('lib/update-lock', () => {
 			await Promise.all(invalidPaths.map((p) => lockfile.unlock(p)));
 		});
 	});
+
+	describe('composition step actions', () => {
+		const lockdir = pathOnRoot(updateLock.BASE_LOCK_DIR);
+		const serviceLockPaths = {
+			1: [
+				`${lockdir}/1/server/updates.lock`,
+				`${lockdir}/1/server/resin-updates.lock`,
+				`${lockdir}/1/client/updates.lock`,
+				`${lockdir}/1/client/resin-updates.lock`,
+			],
+			2: [
+				`${lockdir}/2/main/updates.lock`,
+				`${lockdir}/2/main/resin-updates.lock`,
+			],
+		};
+
+		describe('takeLock', () => {
+			// TODO
+		});
+
+		describe('releaseLock', () => {
+			let testFs: TestFs.Enabled;
+
+			beforeEach(async () => {
+				testFs = await testfs(
+					{},
+					{ cleanup: [path.join(lockdir, '*', '*', '**.lock')] },
+				).enable();
+				// TODO: Update mocha-pod to work with creating empty directories
+				await mkdirp(`${lockdir}/1/server`);
+				await mkdirp(`${lockdir}/1/client`);
+				await mkdirp(`${lockdir}/2/main`);
+			});
+
+			afterEach(async () => {
+				await testFs.restore();
+				await fs.rm(`${lockdir}/1`, { recursive: true });
+				await fs.rm(`${lockdir}/2`, { recursive: true });
+			});
+
+			it('releases locks for an appId', async () => {
+				// Lock services for appId 1
+				for (const lockPath of serviceLockPaths[1]) {
+					await lockfile.lock(lockPath);
+				}
+				// Sanity check that locks are taken & tracked by Supervisor
+				expect(lockfile.getLocksTaken()).to.deep.include.members(
+					serviceLockPaths[1],
+				);
+				// Release locks for appId 1
+				await updateLock.releaseLock(1);
+				// Locks should have been released
+				expect(lockfile.getLocksTaken()).to.have.length(0);
+				// Double check that the lockfiles are removed
+				expect(await fs.readdir(`${lockdir}/1/server`)).to.have.length(0);
+				expect(await fs.readdir(`${lockdir}/1/client`)).to.have.length(0);
+			});
+
+			it('does not error if there are no locks to release', async () => {
+				expect(lockfile.getLocksTaken()).to.have.length(0);
+				// Should not error
+				await updateLock.releaseLock(1);
+				expect(lockfile.getLocksTaken()).to.have.length(0);
+			});
+
+			it('ignores locks outside of appId scope', async () => {
+				const lockPath = `${lockdir}/2/main/updates.lock`;
+				// Lock services outside of appId scope
+				await lockfile.lock(lockPath);
+				// Sanity check that locks are taken & tracked by Supervisor
+				expect(lockfile.getLocksTaken()).to.deep.include.members([lockPath]);
+				// Release locks for appId 1
+				await updateLock.releaseLock(1);
+				// Locks for appId 2 should not have been released
+				expect(lockfile.getLocksTaken()).to.deep.include.members([lockPath]);
+				// Double check that the lockfile is still there
+				expect(await fs.readdir(`${lockdir}/2/main`)).to.have.length(1);
+				// Clean up the lockfile
+				await lockfile.unlock(lockPath);
+			});
+
+			it('waits to release locks until resource write lock is taken', async () => {
+				// Lock services for appId 1
+				for (const lockPath of serviceLockPaths[1]) {
+					await lockfile.lock(lockPath);
+				}
+				// Sanity check that locks are taken & tracked by Supervisor
+				expect(lockfile.getLocksTaken()).to.deep.include.members(
+					serviceLockPaths[1],
+				);
+				// Take the write lock for appId 1
+				const release = await takeGlobalLockRW(1);
+				// Queue releaseLock, won't resolve until the write lock is released
+				const releaseLockPromise = updateLock.releaseLock(1);
+				// Locks should have not been released even after waiting
+				await setTimeout(500);
+				expect(lockfile.getLocksTaken()).to.deep.include.members(
+					serviceLockPaths[1],
+				);
+				// Release the write lock
+				release();
+				// Release locks for appId 1 should resolve
+				await releaseLockPromise;
+				// Locks should have been released
+				expect(lockfile.getLocksTaken()).to.have.length(0);
+			});
+		});
+	});
 });

test/unit/compose/app.spec.ts

@@ -2,6 +2,7 @@ import { expect } from 'chai';
 import type { Image } from '~/src/compose/images';
 import Network from '~/src/compose/network';
 import Volume from '~/src/compose/volume';
+import { LocksTakenMap } from '~/lib/update-lock';
 
 import {
 	createService,
@@ -17,6 +18,7 @@ const defaultContext = {
 	availableImages: [] as Image[],
 	containerIds: {},
 	downloading: [] as string[],
+	locksTaken: new LocksTakenMap(),
 };
 
 describe('compose/app', () => {
@@ -1671,4 +1673,93 @@ describe('compose/app', () => {
 			expectNoStep('kill', steps);
 		});
 	});
+
+	describe('update lock state behavior', () => {
+		it('should infer a releaseLock step if there are locks to be released before settling target state', async () => {
+			const services = [
+				await createService({ serviceName: 'server' }),
+				await createService({ serviceName: 'client' }),
+			];
+			const current = createApp({
+				services,
+				networks: [DEFAULT_NETWORK],
+			});
+			const target = createApp({
+				services,
+				networks: [DEFAULT_NETWORK],
+				isTarget: true,
+			});
+
+			const steps = current.nextStepsForAppUpdate(
+				{
+					...defaultContext,
+					locksTaken: new LocksTakenMap([
+						{ appId: 1, services: ['server', 'client'] },
+					]),
+				},
+				target,
+			);
+			const [releaseLockStep] = expectSteps('releaseLock', steps, 1);
+			expect(releaseLockStep).to.have.property('appId').that.equals(1);
+
+			// Even if not all the locks are taken, releaseLock should be inferred
+			const steps2 = current.nextStepsForAppUpdate(
+				{
+					...defaultContext,
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['server'] }]),
+				},
+				target,
+			);
+			const [releaseLockStep2] = expectSteps('releaseLock', steps2, 1);
+			expect(releaseLockStep2).to.have.property('appId').that.equals(1);
+		});
+
+		it('should not infer a releaseLock step if there are no locks to be released', async () => {
+			const services = [
+				await createService({ serviceName: 'server' }),
+				await createService({ serviceName: 'client' }),
+			];
+			const current = createApp({
+				services,
+				networks: [DEFAULT_NETWORK],
+			});
+			const target = createApp({
+				services,
+				networks: [DEFAULT_NETWORK],
+				isTarget: true,
+			});
+
+			const steps = current.nextStepsForAppUpdate(defaultContext, target);
+			expect(steps).to.have.length(0);
+		});
+
+		it('should infer a releaseLock step for the current appId only', async () => {
+			const services = [
+				await createService({ serviceName: 'server' }),
+				await createService({ serviceName: 'client' }),
+			];
+			const current = createApp({
+				services,
+				networks: [DEFAULT_NETWORK],
+			});
+			const target = createApp({
+				services,
+				networks: [DEFAULT_NETWORK],
+				isTarget: true,
+			});
+
+			const steps = current.nextStepsForAppUpdate(
+				{
+					...defaultContext,
+					locksTaken: new LocksTakenMap([
+						{ appId: 1, services: ['server', 'client'] },
+						{ appId: 2, services: ['main'] },
+					]),
+				},
+				target,
+			);
+			const [releaseLockStep] = expectSteps('releaseLock', steps, 1);
+			expect(releaseLockStep).to.have.property('appId').that.equals(1);
+		});
+	});
 });