Merge pull request #2234 from balena-os/update-lock-during-state-apply

Update lock during state apply

docker-compose.test.yml

@@ -20,6 +20,7 @@ services:
       - mock-systemd
     volumes:
       - dbus:/shared/dbus
+      - tmp:/mnt/root/tmp/balena-supervisor/services
       - ./test/data/root:/mnt/root
       - ./test/data/root/mnt/boot:/mnt/boot
       - ./test/lib/wait-for-it.sh:/wait-for-it.sh
@@ -71,6 +72,7 @@ services:
     stop_grace_period: 3s
     volumes:
       - dbus:/shared/dbus
+      - tmp:/mnt/root/tmp/balena-supervisor/services
     # Set required supervisor configuration variables here
     environment:
       DOCKER_HOST: tcp://docker:2375
@@ -97,8 +99,12 @@ services:
       - /mnt/data
 
 volumes:
+  # Use tmpfs to avoid files remaining between runs
   dbus:
     driver_opts:
-      # Use tmpfs to avoid files remaining between runs
+      type: tmpfs
+      device: tmpfs
+  tmp:
+    driver_opts:
       type: tmpfs
       device: tmpfs

entry.sh

@@ -80,10 +80,6 @@ modprobe ip6_tables || true
 export BASE_LOCK_DIR="/tmp/balena-supervisor/services"
 export LOCKFILE_UID=65534
 
-# Cleanup leftover Supervisor-created lockfiles from any previous processes.
-# Supervisor-created lockfiles have a UID of 65534.
-find "${ROOT_MOUNTPOINT}${BASE_LOCK_DIR}" -type f -user "${LOCKFILE_UID}" -name "*updates.lock" -delete || true
-
 if [ "${LIVEPUSH}" = "1" ]; then
 	exec npx nodemon --watch src --watch typings --ignore tests -e js,ts,json \
 		--exec node -r ts-node/register/transpile-only src/app.ts

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,8 @@ export interface UpdateState {
 	availableImages: Image[];
 	containerIds: Dictionary<string>;
 	downloading: string[];
+	locksTaken: LocksTakenMap;
+	force: boolean;
 }
 
 interface ChangingPair<T> {
@@ -50,6 +53,10 @@ interface ChangingPair<T> {
 	target?: T;
 }
 
+export interface AppsToLockMap {
+	[appId: number]: Set<string>;
+}
+
 export class App {
 	public appId: number;
 	public appUuid?: string;
@@ -109,8 +116,15 @@ export class App {
 		// Check to see if we need to polyfill in some "new" data for legacy services
 		this.migrateLegacy(target);
 
-		// Check for changes in the volumes. We don't remove any volumes until we remove an
+		let steps: CompositionStep[] = [];
-		// entire app
+
+		// Any services which have died get a remove step
+		for (const service of this.services) {
+			if (service.status === 'Dead') {
+				steps.push(generateStep('remove', { current: service }));
+			}
+		}
+
 		const volumeChanges = this.compareComponents(
 			this.volumes,
 			target.volumes,
@@ -122,55 +136,65 @@ export class App {
 			true,
 		);
 
-		let steps: CompositionStep[] = [];
+		const { removePairs, installPairs, updatePairs, dependentServices } =
-
+			this.compareServices(
-		// Any services which have died get a remove step
-		for (const service of this.services) {
-			if (service.status === 'Dead') {
-				steps.push(generateStep('remove', { current: service }));
-			}
-		}
-
-		const { removePairs, installPairs, updatePairs } = this.compareServices(
 				this.services,
 				target.services,
 				state.containerIds,
+				networkChanges,
+				volumeChanges,
 			);
 
 		// For every service which needs to be updated, update via update strategy.
 		const servicePairs = removePairs.concat(updatePairs, installPairs);
-		steps = steps.concat(
+		// generateStepsForService will populate appsToLock with services that
-			servicePairs
+		// need to be locked, including services that need to be removed due to
-				.map((pair) =>
+		// network or volume changes.
+		const appsToLock: AppsToLockMap = {
+			// this.appId should always equal target.appId.
+			[target.appId]: new Set<string>(),
+		};
+		const serviceSteps = servicePairs
+			.flatMap((pair) =>
 				this.generateStepsForService(pair, {
 					...state,
 					servicePairs,
 					targetApp: target,
 					networkPairs: networkChanges,
 					volumePairs: volumeChanges,
+					appsToLock,
 				}),
 			)
-				.filter((step) => step != null) as CompositionStep[],
+			.filter((step) => step != null);
+
+		// Generate lock steps from appsToLock
+		for (const [appId, services] of Object.entries(appsToLock)) {
+			if (services.size > 0) {
+				steps.push(
+					generateStep('takeLock', {
+						appId: parseInt(appId, 10),
+						services: Array.from(services),
+						force: state.force,
+					}),
 				);
+			}
+		}
+
+		// Attach service steps
+		steps = steps.concat(serviceSteps);
 
 		// Generate volume steps
 		steps = steps.concat(
-			this.generateStepsForComponent(volumeChanges, servicePairs, (v, svc) =>
+			this.generateStepsForComponent(volumeChanges, dependentServices),
-				svc.hasVolume(v.name),
-			),
 		);
 		// Generate network steps
 		steps = steps.concat(
-			this.generateStepsForComponent(networkChanges, servicePairs, (n, svc) =>
+			this.generateStepsForComponent(networkChanges, dependentServices),
-				svc.hasNetwork(n.name),
-			),
 		);
 
-		if (
+		if (steps.length === 0) {
-			steps.length === 0 &&
+			// Update commit in db if different
-			target.commit != null &&
+			if (target.commit != null && this.commit !== target.commit) {
-			this.commit !== target.commit
-		) {
 				steps.push(
 					generateStep('updateCommit', {
 						target: target.commit,
@@ -178,6 +202,23 @@ export class App {
 					}),
 				);
 			}
+			// Current & target should be the same appId, but one of either current
+			// or target may not have any services, so we need to check both
+			const allServices = this.services.concat(target.services);
+			if (
+				allServices.length > 0 &&
+				allServices.some((s) =>
+					state.locksTaken.isLocked(s.appId, s.serviceName),
+				)
+			) {
+				// Release locks for all services before settling state
+				steps.push(
+					generateStep('releaseLock', {
+						appId: target.appId,
+					}),
+				);
+			}
+		}
 		return steps;
 	}
 
@@ -185,6 +226,21 @@ export class App {
 		state: Omit<UpdateState, 'availableImages'> & { keepVolumes: boolean },
 	): CompositionStep[] {
 		if (Object.keys(this.services).length > 0) {
+			// Take all locks before killing
+			if (
+				this.services.some(
+					(svc) => !state.locksTaken.isLocked(svc.appId, svc.serviceName),
+				)
+			) {
+				return [
+					generateStep('takeLock', {
+						appId: this.appId,
+						services: this.services.map((svc) => svc.serviceName),
+						force: state.force,
+					}),
+				];
+			}
+
 			return Object.values(this.services).map((service) =>
 				generateStep('kill', { current: service }),
 			);
@@ -289,14 +345,24 @@ export class App {
 		return outputs;
 	}
 
+	private getDependentServices<T extends Volume | Network>(
+		component: T,
+		dependencyFn: (component: T, service: Service) => boolean,
+	) {
+		return this.services.filter((s) => dependencyFn(component, s));
+	}
+
 	private compareServices(
 		current: Service[],
 		target: Service[],
-		containerIds: Dictionary<string>,
+		containerIds: UpdateState['containerIds'],
+		networkChanges: Array<ChangingPair<Network>>,
+		volumeChanges: Array<ChangingPair<Volume>>,
 	): {
 		installPairs: Array<ChangingPair<Service>>;
 		removePairs: Array<ChangingPair<Service>>;
 		updatePairs: Array<ChangingPair<Service>>;
+		dependentServices: Service[];
 	} {
 		const currentByServiceName = _.keyBy(current, 'serviceName');
 		const targetByServiceName = _.keyBy(target, 'serviceName');
@@ -304,8 +370,26 @@ export class App {
 		const currentServiceNames = Object.keys(currentByServiceName);
 		const targetServiceNames = Object.keys(targetByServiceName);
 
+		// For volume|network removal or config changes, we require dependent
+		// services be killed first.
+		const dependentServices: Service[] = [];
+		for (const { current: c } of networkChanges) {
+			if (c != null) {
+				dependentServices.push(
+					...this.getDependentServices(c, (n, svc) => svc.hasNetwork(n.name)),
+				);
+			}
+		}
+		for (const { current: c } of volumeChanges) {
+			if (c != null) {
+				dependentServices.push(
+					...this.getDependentServices(c, (v, svc) => svc.hasVolume(v.name)),
+				);
+			}
+		}
 		const toBeRemoved = _(currentServiceNames)
 			.difference(targetServiceNames)
+			.union(dependentServices.map((s) => s.serviceName))
 			.map((id) => ({ current: currentByServiceName[id] }))
 			.value();
 
@@ -413,6 +497,15 @@ export class App {
 			);
 		};
 
+		/**
+		 * Checks if a service is destined for removal due to a network or volume change
+		 */
+		const shouldBeRemoved = (serviceCurrent: Service) => {
+			return toBeRemoved.some(
+				(pair) => pair.current.serviceName === serviceCurrent.serviceName,
+			);
+		};
+
 		/**
 		 * Filter all the services which should be updated due to run state change, or config mismatch.
 		 */
@@ -423,16 +516,18 @@ export class App {
 			}))
 			.filter(
 				({ current: c, target: t }) =>
-					!isEqualExceptForRunningState(c, t) ||
+					!shouldBeRemoved(c) &&
+					(!isEqualExceptForRunningState(c, t) ||
 						shouldBeStarted(c, t) ||
 						shouldBeStopped(c, t) ||
-					shouldWaitForStop(c),
+						shouldWaitForStop(c)),
 			);
 
 		return {
 			installPairs: toBeInstalled,
 			removePairs: toBeRemoved,
 			updatePairs: toBeUpdated,
+			dependentServices,
 		};
 	}
 
@@ -444,8 +539,7 @@ export class App {
 	// it should be changed.
 	private generateStepsForComponent<T extends Volume | Network>(
 		components: Array<ChangingPair<T>>,
-		changingServices: Array<ChangingPair<Service>>,
+		dependentServices: Service[],
-		dependencyFn: (component: T, service: Service) => boolean,
 	): CompositionStep[] {
 		if (components.length === 0) {
 			return [];
@@ -453,36 +547,42 @@ export class App {
 
 		let steps: CompositionStep[] = [];
 
+		const componentIsVolume =
+			(components[0].current ?? components[0].target) instanceof Volume;
+
 		const actions: {
 			create: CompositionStepAction;
 			remove: CompositionStepAction;
-		} =
+		} = componentIsVolume
-			(components[0].current ?? components[0].target) instanceof Volume
 			? { create: 'createVolume', remove: 'removeVolume' }
 			: { create: 'createNetwork', remove: 'removeNetwork' };
 
 		for (const { current, target } of components) {
 			// If a current exists, we're either removing it or updating the configuration. In
-			// both cases, we must remove the component first, so we output those steps first.
+			// both cases, we must remove the component before creating it to avoid
+			// Engine conflicts. So we always emit a remove step first.
 			// If we do remove the component, we first need to remove any services which depend
-			// on the component
+			// on the component. The service removal steps are generated in this.generateStepsForService
+			// after their removal is calculated in this.compareServices.
 			if (current != null) {
-				// Find any services which are currently running which need to be killed when we
+				// If there are any dependent services which have the volume or network,
-				// recreate this component
+				// we cannot proceed to component removal.
-				const dependencies = _.filter(this.services, (s) =>
+				const dependentServicesOfComponent = dependentServices.filter((s) => {
-					dependencyFn(current, s),
+					if (componentIsVolume) {
-				);
+						return this.serviceHasNetworkOrVolume(
-				if (dependencies.length > 0) {
+							s,
-					// We emit kill steps for these services, and wait to destroy the component in
+							[],
-					// the next state application loop
+							[{ current: current as Volume, target: target as Volume }],
-					// FIXME: We should add to the changingServices array, as we could emit several
-					// kill steps for a service
-					steps = steps.concat(
-						dependencies.flatMap((svc) =>
-							this.generateKillStep(svc, changingServices),
-						),
 						);
 					} else {
+						return this.serviceHasNetworkOrVolume(
+							s,
+							[{ current: current as Network, target: target as Network }],
+							[],
+						);
+					}
+				});
+				if (dependentServicesOfComponent.length === 0) {
 					steps = steps.concat([generateStep(actions.remove, { current })]);
 				}
 			} else if (target != null) {
@@ -496,24 +596,25 @@ export class App {
 	private generateStepsForService(
 		{ current, target }: ChangingPair<Service>,
 		context: {
-			availableImages: Image[];
-			downloading: string[];
 			targetApp: App;
-			containerIds: Dictionary<string>;
 			networkPairs: Array<ChangingPair<Network>>;
 			volumePairs: Array<ChangingPair<Volume>>;
 			servicePairs: Array<ChangingPair<Service>>;
-		},
+			appsToLock: AppsToLockMap;
-	): Nullable<CompositionStep> {
+		} & UpdateState,
+	): CompositionStep[] {
+		const servicesLocked = this.services
+			.concat(context.targetApp.services)
+			.every((svc) => context.locksTaken.isLocked(svc.appId, svc.serviceName));
 		if (current?.status === 'Stopping') {
 			// There's a kill step happening already, emit a noop to ensure
 			// we stay alive while this happens
-			return generateStep('noop', {});
+			return [generateStep('noop', {})];
 		}
 		if (current?.status === 'Dead') {
 			// A remove step will already have been generated, so we let the state
 			// application loop revisit this service, once the state has settled
-			return;
+			return [];
 		}
 
 		const needsDownload =
@@ -530,7 +631,7 @@ export class App {
 		) {
 			// The image needs to be downloaded, and it's currently downloading.
 			// We simply keep the application loop alive
-			return generateStep('noop', {});
+			return [generateStep('noop', {})];
 		}
 
 		if (current == null) {
@@ -539,6 +640,8 @@ export class App {
 				target!,
 				context.targetApp,
 				needsDownload,
+				servicesLocked,
+				context.appsToLock,
 				context.availableImages,
 				context.networkPairs,
 				context.volumePairs,
@@ -547,6 +650,8 @@ export class App {
 		} else {
 			// This service is in both current & target so requires an update,
 			// or it's a service that's not in target so requires removal
+
+			// Skip updateMetadata for services with networks or volumes
 			const needsSpecialKill = this.serviceHasNetworkOrVolume(
 				current,
 				context.networkPairs,
@@ -557,8 +662,14 @@ export class App {
 				target != null &&
 				current.isEqualConfig(target, context.containerIds)
 			) {
-				// we're only starting/stopping a service
+				// Update service metadata or start/stop a service
-				return this.generateContainerStep(current, target);
+				return this.generateContainerStep(
+					current,
+					target,
+					context.appsToLock,
+					context.targetApp.services,
+					servicesLocked,
+				);
 			}
 
 			let strategy: string;
@@ -590,29 +701,13 @@ export class App {
 				dependenciesMetForStart,
 				dependenciesMetForKill,
 				needsSpecialKill,
+				servicesLocked,
+				services: this.services.concat(context.targetApp.services),
+				appsToLock: context.appsToLock,
 			});
 		}
 	}
 
-	// We return an array from this function so the caller can just concatenate the arrays
-	// without worrying if the step is skipped or not
-	private generateKillStep(
-		service: Service,
-		changingServices: Array<ChangingPair<Service>>,
-	): CompositionStep[] {
-		if (
-			service.status !== 'Stopping' &&
-			!_.some(
-				changingServices,
-				({ current }) => current?.serviceName === service.serviceName,
-			)
-		) {
-			return [generateStep('kill', { current: service })];
-		} else {
-			return [];
-		}
-	}
-
 	private serviceHasNetworkOrVolume(
 		svc: Service,
 		networkPairs: Array<ChangingPair<Network>>,
@@ -647,16 +742,39 @@ export class App {
 		);
 	}
 
-	private generateContainerStep(current: Service, target: Service) {
+	private generateContainerStep(
-		// if the services release doesn't match, then rename the container...
+		current: Service,
+		target: Service,
+		appsToLock: AppsToLockMap,
+		targetServices: Service[],
+		servicesLocked: boolean,
+	): CompositionStep[] {
+		// Update container metadata if service release has changed
 		if (current.commit !== target.commit) {
-			return generateStep('updateMetadata', { current, target });
+			if (servicesLocked) {
-		} else if (target.config.running !== current.config.running) {
+				return [generateStep('updateMetadata', { current, target })];
-			if (target.config.running) {
-				return generateStep('start', { target });
 			} else {
-				return generateStep('stop', { current });
+				// Otherwise, take lock for all services first
+				this.services.concat(targetServices).forEach((s) => {
+					appsToLock[target.appId].add(s.serviceName);
+				});
+				return [];
 			}
+		} else if (target.config.running !== current.config.running) {
+			// Take lock for all services before starting/stopping container
+			if (!servicesLocked) {
+				this.services.concat(targetServices).forEach((s) => {
+					appsToLock[target.appId].add(s.serviceName);
+				});
+				return [];
+			}
+			if (target.config.running) {
+				return [generateStep('start', { target })];
+			} else {
+				return [generateStep('stop', { current })];
+			}
+		} else {
+			return [];
 		}
 	}
 
@@ -664,21 +782,26 @@ export class App {
 		target: Service,
 		targetApp: App,
 		needsDownload: boolean,
-		availableImages: Image[],
+		servicesLocked: boolean,
+		appsToLock: AppsToLockMap,
+		availableImages: UpdateState['availableImages'],
 		networkPairs: Array<ChangingPair<Network>>,
 		volumePairs: Array<ChangingPair<Volume>>,
 		servicePairs: Array<ChangingPair<Service>>,
-	): CompositionStep | undefined {
+	): CompositionStep[] {
 		if (
 			needsDownload &&
 			this.dependenciesMetForServiceFetch(target, servicePairs)
 		) {
 			// We know the service name exists as it always does for targets
-			return generateStep('fetch', {
+			return [
+				generateStep('fetch', {
 					image: imageManager.imageFromService(target),
-				serviceName: target.serviceName!,
+					serviceName: target.serviceName,
-			});
+				}),
+			];
 		} else if (
+			target != null &&
 			this.dependenciesMetForServiceStart(
 				target,
 				targetApp,
@@ -688,7 +811,15 @@ export class App {
 				servicePairs,
 			)
 		) {
-			return generateStep('start', { target });
+			if (!servicesLocked) {
+				this.services
+					.concat(targetApp.services)
+					.forEach((svc) => appsToLock[target.appId].add(svc.serviceName));
+				return [];
+			}
+			return [generateStep('start', { target })];
+		} else {
+			return [];
 		}
 	}
 
@@ -728,7 +859,7 @@ export class App {
 	private dependenciesMetForServiceStart(
 		target: Service,
 		targetApp: App,
-		availableImages: Image[],
+		availableImages: UpdateState['availableImages'],
 		networkPairs: Array<ChangingPair<Network>>,
 		volumePairs: Array<ChangingPair<Volume>>,
 		servicePairs: Array<ChangingPair<Service>>,
@@ -738,7 +869,7 @@ export class App {
 		// are services which are changing). We could have a dependency which is
 		// starting up, but is not yet running.
 		const depInstallingButNotRunning = _.some(targetApp.services, (svc) => {
-			if (target.dependsOn?.includes(svc.serviceName!)) {
+			if (target.dependsOn?.includes(svc.serviceName)) {
 				if (!svc.config.running) {
 					return true;
 				}
@@ -785,14 +916,14 @@ export class App {
 	// block the killing too much, potentially causing a deadlock)
 	private dependenciesMetForServiceKill(
 		targetApp: App,
-		availableImages: Image[],
+		availableImages: UpdateState['availableImages'],
 	) {
 		return this.targetImagesReady(targetApp.services, availableImages);
 	}
 
 	private targetImagesReady(
 		targetServices: Service[],
-		availableImages: Image[],
+		availableImages: UpdateState['availableImages'],
 	) {
 		return targetServices.every((service) =>
 			availableImages.some(

src/compose/application-manager.ts

@@ -17,10 +17,11 @@ import {
 	ContractViolationError,
 	InternalInconsistencyError,
 } from '../lib/errors';
-import { lock } from '../lib/update-lock';
+import { getServicesLockedByAppId, LocksTakenMap } from '../lib/update-lock';
 import { checkTruthy } from '../lib/validation';
 
 import App from './app';
+import type { UpdateState } from './app';
 import * as volumeManager from './volume-manager';
 import * as networkManager from './network-manager';
 import * as serviceManager from './service-manager';
@@ -63,7 +64,6 @@ export function resetTimeSpentFetching(value: number = 0) {
 }
 
 const actionExecutors = getExecutors({
-	lockFn: lock,
 	callbacks: {
 		fetchStart: () => {
 			fetchesInProgress += 1;
@@ -119,6 +119,7 @@ export async function getRequiredSteps(
 	targetApps: InstancedAppState,
 	keepImages?: boolean,
 	keepVolumes?: boolean,
+	force: boolean = false,
 ): Promise<CompositionStep[]> {
 	// get some required data
 	const [downloading, availableImages, { localMode, delta }] =
@@ -145,9 +146,11 @@ export async function getRequiredSteps(
 		// Volumes are not removed when stopping an app when going to local mode
 		keepVolumes,
 		delta,
+		force,
 		downloading,
 		availableImages,
 		containerIdsByAppId,
+		locksTaken: await getServicesLockedByAppId(),
 	});
 }
 
@@ -159,9 +162,13 @@ export async function inferNextSteps(
 		keepImages = false,
 		keepVolumes = false,
 		delta = true,
-		downloading = [] as string[],
+		force = false,
-		availableImages = [] as Image[],
+		downloading = [] as UpdateState['downloading'],
-		containerIdsByAppId = {} as { [appId: number]: Dictionary<string> },
+		availableImages = [] as UpdateState['availableImages'],
+		containerIdsByAppId = {} as {
+			[appId: number]: UpdateState['containerIds'];
+		},
+		locksTaken = new LocksTakenMap(),
 	} = {},
 ) {
 	const currentAppIds = Object.keys(currentApps).map((i) => parseInt(i, 10));
@@ -213,6 +220,8 @@ export async function inferNextSteps(
 							availableImages,
 							containerIds: containerIdsByAppId[id],
 							downloading,
+							locksTaken,
+							force,
 						},
 						targetApps[id],
 					),
@@ -226,6 +235,8 @@ export async function inferNextSteps(
 						keepVolumes,
 						downloading,
 						containerIds: containerIdsByAppId[id],
+						locksTaken,
+						force,
 					}),
 				);
 			}
@@ -249,6 +260,8 @@ export async function inferNextSteps(
 							availableImages,
 							containerIds: containerIdsByAppId[id] ?? {},
 							downloading,
+							locksTaken,
+							force,
 						},
 						targetApps[id],
 					),
@@ -293,17 +306,6 @@ export async function inferNextSteps(
 	return steps;
 }
 
-export async function stopAll({ force = false, skipLock = false } = {}) {
-	const services = await serviceManager.getAll();
-	await Promise.all(
-		services.map(async (s) => {
-			return lock(s.appId, { force, skipLock }, async () => {
-				await serviceManager.kill(s, { removeContainer: false, wait: true });
-			});
-		}),
-	);
-}
-
 // The following two function may look pretty odd, but after the move to uuids,
 // there's a chance that the current running apps don't have a uuid set. We
 // still need to be able to work on these and perform various state changes. To
@@ -493,7 +495,7 @@ function killServicesUsingApi(current: InstancedAppState): CompositionStep[] {
 // intermediate targets to perform changes
 export async function executeStep(
 	step: CompositionStep,
-	{ force = false, skipLock = false } = {},
+	{ force = false } = {},
 ): Promise<void> {
 	if (!validActions.includes(step.action)) {
 		return Promise.reject(
@@ -507,7 +509,6 @@ export async function executeStep(
 	await actionExecutors[step.action]({
 		...step,
 		force,
-		skipLock,
 	} as any);
 }
 
@@ -651,7 +652,7 @@ export function bestDeltaSource(
 function saveAndRemoveImages(
 	current: InstancedAppState,
 	target: InstancedAppState,
-	availableImages: imageManager.Image[],
+	availableImages: UpdateState['availableImages'],
 	skipRemoval: boolean,
 ): CompositionStep[] {
 	type ImageWithoutID = Omit<imageManager.Image, 'dockerImageId' | 'id'>;
@@ -670,9 +671,8 @@ function saveAndRemoveImages(
 			.filter((img) => img[1] != null)
 			.value();
 
-	const availableWithoutIds: ImageWithoutID[] = _.map(
+	const availableWithoutIds: ImageWithoutID[] = availableImages.map((image) =>
-		availableImages,
+		_.omit(image, ['dockerImageId', 'id']),
-		(image) => _.omit(image, ['dockerImageId', 'id']),
 	);
 
 	const currentImages = _.flatMap(current, (app) =>
@@ -692,18 +692,16 @@ function saveAndRemoveImages(
 	const targetServices = Object.values(target).flatMap((app) => app.services);
 	const targetImages = targetServices.map(imageManager.imageFromService);
 
-	const availableAndUnused = _.filter(
+	const availableAndUnused = availableWithoutIds.filter(
-		availableWithoutIds,
 		(image) =>
-			!_.some(currentImages.concat(targetImages), (imageInUse) => {
+			!currentImages.concat(targetImages).some((imageInUse) => {
 				return _.isEqual(image, _.omit(imageInUse, ['dockerImageId', 'id']));
 			}),
 	);
 
-	const imagesToDownload = _.filter(
+	const imagesToDownload = targetImages.filter(
-		targetImages,
 		(targetImage) =>
-			!_.some(availableImages, (available) =>
+			!availableImages.some((available) =>
 				imageManager.isSameImage(available, targetImage),
 			),
 	);
@@ -713,10 +711,9 @@ function saveAndRemoveImages(
 	);
 
 	// Images that are available but we don't have them in the DB with the exact metadata:
-	const imagesToSave: imageManager.Image[] = _.filter(
+	const imagesToSave: imageManager.Image[] = targetImages.filter(
-		targetImages,
 		(targetImage) => {
-			const isActuallyAvailable = _.some(availableImages, (availableImage) => {
+			const isActuallyAvailable = availableImages.some((availableImage) => {
 				// There is an image with same image name or digest
 				// on the database
 				if (imageManager.isSameImage(availableImage, targetImage)) {
@@ -734,7 +731,7 @@ function saveAndRemoveImages(
 			});
 
 			// There is no image in the database with the same metadata
-			const isNotSaved = !_.some(availableWithoutIds, (img) =>
+			const isNotSaved = !availableWithoutIds.some((img) =>
 				_.isEqual(img, targetImage),
 			);
 

src/compose/composition-steps.ts

@@ -1,64 +1,43 @@
-import _ from 'lodash';
-
 import * as config from '../config';
-
 import type { Image } from './images';
 import * as images from './images';
 import type Network from './network';
 import type Service from './service';
 import * as serviceManager from './service-manager';
-import type Volume from './volume';
-
-import { checkTruthy } from '../lib/validation';
 import * as networkManager from './network-manager';
 import * as volumeManager from './volume-manager';
-import type { DeviceLegacyReport } from '../types/state';
+import type Volume from './volume';
 import * as commitStore from './commit';
+import * as updateLock from '../lib/update-lock';
+import type { DeviceLegacyReport } from '../types/state';
 
-interface BaseCompositionStepArgs {
-	force?: boolean;
-	skipLock?: boolean;
-}
-
-// FIXME: Most of the steps take the
-// BaseCompositionStepArgs, but some also take an options
-// structure which includes some of the same fields. It
-// would be nice to remove the need for this
 interface CompositionStepArgs {
 	stop: {
 		current: Service;
 		options?: {
-			skipLock?: boolean;
 			wait?: boolean;
 		};
-	} & BaseCompositionStepArgs;
+	};
 	kill: {
 		current: Service;
 		options?: {
-			skipLock?: boolean;
 			wait?: boolean;
 		};
-	} & BaseCompositionStepArgs;
+	};
 	remove: {
 		current: Service;
-	} & BaseCompositionStepArgs;
+	};
 	updateMetadata: {
 		current: Service;
 		target: Service;
-		options?: {
-			skipLock?: boolean;
 	};
-	} & BaseCompositionStepArgs;
 	restart: {
 		current: Service;
 		target: Service;
-		options?: {
-			skipLock?: boolean;
 	};
-	} & BaseCompositionStepArgs;
 	start: {
 		target: Service;
-	} & BaseCompositionStepArgs;
+	};
 	updateCommit: {
 		target: string;
 		appId: number;
@@ -67,10 +46,9 @@ interface CompositionStepArgs {
 		current: Service;
 		target: Service;
 		options?: {
-			skipLock?: boolean;
 			timeout?: number;
 		};
-	} & BaseCompositionStepArgs;
+	};
 	fetch: {
 		image: Image;
 		serviceName: string;
@@ -96,6 +74,14 @@ interface CompositionStepArgs {
 	};
 	ensureSupervisorNetwork: object;
 	noop: object;
+	takeLock: {
+		appId: number;
+		services: string[];
+		force: boolean;
+	};
+	releaseLock: {
+		appId: number;
+	};
 }
 
 export type CompositionStepAction = keyof CompositionStepArgs;
@@ -117,13 +103,6 @@ export function generateStep<T extends CompositionStepAction>(
 type Executors<T extends CompositionStepAction> = {
 	[key in T]: (step: CompositionStepT<key>) => Promise<unknown>;
 };
-type LockingFn = (
-	// TODO: Once the entire codebase is typescript, change
-	// this to number
-	app: number | number[] | null,
-	args: BaseCompositionStepArgs,
-	fn: () => Promise<unknown>,
-) => Promise<unknown>;
 
 interface CompositionCallbacks {
 	// TODO: Once the entire codebase is typescript, change
@@ -135,90 +114,48 @@ interface CompositionCallbacks {
 	bestDeltaSource: (image: Image, available: Image[]) => string | null;
 }
 
-export function getExecutors(app: {
+export function getExecutors(app: { callbacks: CompositionCallbacks }) {
-	lockFn: LockingFn;
-	callbacks: CompositionCallbacks;
-}) {
 	const executors: Executors<CompositionStepAction> = {
-		stop: (step) => {
+		stop: async (step) => {
-			return app.lockFn(
+			// Should always be preceded by a takeLock step,
-				step.current.appId,
+			// so the call is executed assuming that the lock is taken.
-				{
-					force: step.force,
-					skipLock: step.skipLock || _.get(step, ['options', 'skipLock']),
-				},
-				async () => {
-					const wait = _.get(step, ['options', 'wait'], false);
 			await serviceManager.kill(step.current, {
 				removeContainer: false,
-						wait,
+				wait: step.options?.wait || false,
 			});
 		},
-			);
+		kill: async (step) => {
-		},
+			// Should always be preceded by a takeLock step,
-		kill: (step) => {
+			// so the call is executed assuming that the lock is taken.
-			return app.lockFn(
-				step.current.appId,
-				{
-					force: step.force,
-					skipLock: step.skipLock || _.get(step, ['options', 'skipLock']),
-				},
-				async () => {
 			await serviceManager.kill(step.current);
 		},
-			);
-		},
 		remove: async (step) => {
 			// Only called for dead containers, so no need to
 			// take locks
 			await serviceManager.remove(step.current);
 		},
-		updateMetadata: (step) => {
+		updateMetadata: async (step) => {
-			const skipLock =
+			// Should always be preceded by a takeLock step,
-				step.skipLock ||
+			// so the call is executed assuming that the lock is taken.
-				checkTruthy(step.current.config.labels['io.balena.legacy-container']);
-			return app.lockFn(
-				step.current.appId,
-				{
-					force: step.force,
-					skipLock: skipLock || _.get(step, ['options', 'skipLock']),
-				},
-				async () => {
 			await serviceManager.updateMetadata(step.current, step.target);
 		},
-			);
+		restart: async (step) => {
-		},
+			// Should always be preceded by a takeLock step,
-		restart: (step) => {
+			// so the call is executed assuming that the lock is taken.
-			return app.lockFn(
-				step.current.appId,
-				{
-					force: step.force,
-					skipLock: step.skipLock || _.get(step, ['options', 'skipLock']),
-				},
-				async () => {
 			await serviceManager.kill(step.current, { wait: true });
 			await serviceManager.start(step.target);
 		},
-			);
-		},
 		start: async (step) => {
 			await serviceManager.start(step.target);
 		},
 		updateCommit: async (step) => {
 			await commitStore.upsertCommitForApp(step.appId, step.target);
 		},
-		handover: (step) => {
+		handover: async (step) => {
-			return app.lockFn(
+			// Should always be preceded by a takeLock step,
-				step.current.appId,
+			// so the call is executed assuming that the lock is taken.
-				{
-					force: step.force,
-					skipLock: step.skipLock || _.get(step, ['options', 'skipLock']),
-				},
-				async () => {
 			await serviceManager.handover(step.current, step.target);
 		},
-			);
-		},
 		fetch: async (step) => {
 			const startTime = process.hrtime();
 			app.callbacks.fetchStart();
@@ -278,6 +215,12 @@ export function getExecutors(app: {
 		noop: async () => {
 			/* async noop */
 		},
+		takeLock: async (step) => {
+			await updateLock.takeLock(step.appId, step.services, step.force);
+		},
+		releaseLock: async (step) => {
+			await updateLock.releaseLock(step.appId);
+		},
 	};
 
 	return executors;

src/compose/update-strategies.ts

@@ -2,6 +2,7 @@ import * as imageManager from './images';
 import type Service from './service';
 import type { CompositionStep } from './composition-steps';
 import { generateStep } from './composition-steps';
+import type { AppsToLockMap } from './app';
 import { InternalInconsistencyError } from '../lib/errors';
 import { checkString } from '../lib/validation';
 
@@ -12,43 +13,82 @@ export interface StrategyContext {
 	dependenciesMetForStart: boolean;
 	dependenciesMetForKill: boolean;
 	needsSpecialKill: boolean;
+	services: Service[];
+	servicesLocked: boolean;
+	appsToLock: AppsToLockMap;
+}
+
+function generateLockThenKillStep(
+	current: Service,
+	currentServices: Service[],
+	servicesLocked: boolean,
+	appsToLock: AppsToLockMap,
+): CompositionStep[] {
+	if (!servicesLocked) {
+		currentServices.forEach((svc) =>
+			appsToLock[svc.appId].add(svc.serviceName),
+		);
+		return [];
+	}
+	return [generateStep('kill', { current })];
 }
 
 export function getStepsFromStrategy(
 	strategy: string,
 	context: StrategyContext,
-): CompositionStep {
+): CompositionStep[] {
 	switch (strategy) {
 		case 'download-then-kill':
 			if (context.needsDownload && context.target) {
-				return generateStep('fetch', {
+				return [
+					generateStep('fetch', {
 						image: imageManager.imageFromService(context.target),
 						serviceName: context.target.serviceName,
-				});
+					}),
+				];
 			} else if (context.dependenciesMetForKill) {
 				// We only kill when dependencies are already met, so that we minimize downtime
-				return generateStep('kill', { current: context.current });
+				return generateLockThenKillStep(
+					context.current,
+					context.services,
+					context.servicesLocked,
+					context.appsToLock,
+				);
 			} else {
-				return generateStep('noop', {});
+				return [generateStep('noop', {})];
 			}
 		case 'kill-then-download':
 		case 'delete-then-download':
-			return generateStep('kill', { current: context.current });
+			return generateLockThenKillStep(
+				context.current,
+				context.services,
+				context.servicesLocked,
+				context.appsToLock,
+			);
 		case 'hand-over':
 			if (context.needsDownload && context.target) {
-				return generateStep('fetch', {
+				return [
+					generateStep('fetch', {
 						image: imageManager.imageFromService(context.target),
 						serviceName: context.target.serviceName,
-				});
+					}),
+				];
 			} else if (context.needsSpecialKill && context.dependenciesMetForKill) {
-				return generateStep('kill', { current: context.current });
+				return generateLockThenKillStep(
+					context.current,
+					context.services,
+					context.servicesLocked,
+					context.appsToLock,
+				);
 			} else if (context.dependenciesMetForStart && context.target) {
-				return generateStep('handover', {
+				return [
+					generateStep('handover', {
 						current: context.current,
 						target: context.target,
-				});
+					}),
+				];
 			} else {
-				return generateStep('noop', {});
+				return [generateStep('noop', {})];
 			}
 		default:
 			throw new InternalInconsistencyError(

src/config/configJson.ts

@@ -4,7 +4,7 @@ import _ from 'lodash';
 import * as constants from '../lib/constants';
 import * as hostUtils from '../lib/host-utils';
 import * as osRelease from '../lib/os-release';
-import { readLock, writeLock } from '../lib/update-lock';
+import { takeGlobalLockRO, takeGlobalLockRW } from '../lib/process-lock';
 import type * as Schema from './schema';
 
 export default class ConfigJsonConfigBackend {
@@ -25,9 +25,9 @@ export default class ConfigJsonConfigBackend {
 		this.schema = schema;
 
 		this.writeLockConfigJson = () =>
-			writeLock('config.json').disposer((release) => release());
+			takeGlobalLockRW('config.json').disposer((release) => release());
 		this.readLockConfigJson = () =>
-			readLock('config.json').disposer((release) => release());
+			takeGlobalLockRO('config.json').disposer((release) => release());
 	}
 
 	public async set<T extends Schema.SchemaKey>(keyVals: {

src/config/index.ts

@@ -56,7 +56,7 @@ export async function get<T extends SchemaTypeKey>(
 ): Promise<SchemaReturn<T>> {
 	const $db = trx || db.models;
 
-	if (Object.prototype.hasOwnProperty.call(Schema.schema, key)) {
+	if (Object.hasOwn(Schema.schema, key)) {
 		const schemaKey = key as Schema.SchemaKey;
 
 		return getSchema(schemaKey, $db).then((value) => {
@@ -82,7 +82,7 @@ export async function get<T extends SchemaTypeKey>(
 			// the type system happy
 			return checkValueDecode(decoded, key, value) && decoded.right;
 		});
-	} else if (Object.prototype.hasOwnProperty.call(FnSchema.fnSchema, key)) {
+	} else if (Object.hasOwn(FnSchema.fnSchema, key)) {
 		const fnKey = key as FnSchema.FnSchemaKey;
 		// Cast the promise as something that produces an unknown, and this means that
 		// we can validate the output of the function as well, ensuring that the type matches
@@ -269,7 +269,7 @@ function validateConfigMap<T extends SchemaTypeKey>(
 	// throw if any value fails verification
 	return _.mapValues(configMap, (value, key) => {
 		if (
-			!Object.prototype.hasOwnProperty.call(Schema.schema, key) ||
+			!Object.hasOwn(Schema.schema, key) ||
 			!Schema.schema[key as Schema.SchemaKey].mutable
 		) {
 			throw new Error(

src/device-api/actions.ts

@@ -11,11 +11,11 @@ import * as applicationManager from '../compose/application-manager';
 import type { CompositionStepAction } from '../compose/composition-steps';
 import { generateStep } from '../compose/composition-steps';
 import * as commitStore from '../compose/commit';
+import type Service from '../compose/service';
 import { getApp } from '../device-state/db-format';
 import * as TargetState from '../device-state/target-state';
 import log from '../lib/supervisor-console';
 import blink = require('../lib/blink');
-import { lock } from '../lib/update-lock';
 import * as constants from '../lib/constants';
 import {
 	InternalInconsistencyError,
@@ -88,32 +88,29 @@ export const regenerateKey = async (oldKey: string) => {
 export const doRestart = async (appId: number, force: boolean = false) => {
 	await deviceState.initialized();
 
-	return await lock(appId, { force }, async () => {
 	const currentState = await deviceState.getCurrentState();
 	if (currentState.local.apps?.[appId] == null) {
 		throw new InternalInconsistencyError(
 			`Application with ID ${appId} is not in the current state`,
 		);
 	}
+
 	const app = currentState.local.apps[appId];
 	const services = app.services;
-		app.services = [];
 
-		return deviceState
+	try {
-			.applyIntermediateTarget(currentState, {
+		// Set target so that services get deleted
-				skipLock: true,
+		app.services = [];
-			})
+		await deviceState.applyIntermediateTarget(currentState, { force });
-			.then(() => {
+		// Restore services
 		app.services = services;
 		return deviceState.applyIntermediateTarget(currentState, {
-					skipLock: true,
 			keepVolumes: false,
+			force,
 		});
-			})
+	} finally {
-			.finally(() => {
 		deviceState.triggerApplyTarget();
-			});
+	}
-	});
 };
 
 /**
@@ -131,47 +128,37 @@ export const doPurge = async (appId: number, force: boolean = false) => {
 		'Purge data',
 	);
 
-	return await lock(appId, { force }, async () => {
 	const currentState = await deviceState.getCurrentState();
 	if (currentState.local.apps?.[appId] == null) {
 		throw new InternalInconsistencyError(
 			`Application with ID ${appId} is not in the current state`,
 		);
 	}
-
+	// Save & delete the app from the current state
 	const app = currentState.local.apps[appId];
-
-		// Delete the app from the current state
 	delete currentState.local.apps[appId];
 
-		return deviceState
+	try {
-			.applyIntermediateTarget(currentState, {
+		// Purposely tell the apply function to delete volumes so
-				skipLock: true,
+		// they can get deleted even in local mode
-				// Purposely tell the apply function to delete volumes so they can get
+		await deviceState.applyIntermediateTarget(currentState, {
-				// deleted even in local mode
 			keepVolumes: false,
-			})
+			force,
-			.then(() => {
+		});
+		// Restore user app after purge
 		currentState.local.apps[appId] = app;
-				return deviceState.applyIntermediateTarget(currentState, {
+		await deviceState.applyIntermediateTarget(currentState);
-					skipLock: true,
+		logger.logSystemMessage('Purged data', { appId }, 'Purge data success');
-				});
+	} catch (err: any) {
-			})
-			.finally(() => {
-				deviceState.triggerApplyTarget();
-			});
-	})
-		.then(() =>
-			logger.logSystemMessage('Purged data', { appId }, 'Purge data success'),
-		)
-		.catch((err) => {
 		logger.logSystemMessage(
 			`Error purging data: ${err}`,
-				{ appId, error: err },
+			{ appId, error: err?.message ?? err },
 			'Purge data error',
 		);
 		throw err;
-		});
+	} finally {
+		deviceState.triggerApplyTarget();
+	}
 };
 
 type ClientError = BadRequestError | NotFoundError;
@@ -224,6 +211,57 @@ export const executeDeviceAction = async (
 	});
 };
 
+/**
+ * Used internally by executeServiceAction to handle locks
+ * around execution of a service action.
+ */
+const executeDeviceActionWithLock = async ({
+	action,
+	appId,
+	currentService,
+	targetService,
+	force = false,
+}: {
+	action: CompositionStepAction;
+	appId: number;
+	currentService?: Service;
+	targetService?: Service;
+	force: boolean;
+}) => {
+	try {
+		if (currentService) {
+			// Take lock for current service to be modified / stopped
+			await executeDeviceAction(
+				generateStep('takeLock', {
+					appId,
+					services: [currentService.serviceName],
+					force,
+				}),
+				// FIXME: deviceState.executeStepAction only accepts force as a separate arg
+				// instead of reading force from the step object, so we have to pass it twice
+				force,
+			);
+		}
+
+		// Execute action on service
+		await executeDeviceAction(
+			generateStep(action, {
+				current: currentService,
+				target: targetService,
+				wait: true,
+			}),
+			force,
+		);
+	} finally {
+		// Release lock regardless of action success to prevent leftover lockfile
+		await executeDeviceAction(
+			generateStep('releaseLock', {
+				appId,
+			}),
+		);
+	}
+};
+
 /**
  * Executes a composition step action on a service.
  * isLegacy indicates that the action is being called from a legacy (v1) endpoint,
@@ -279,15 +317,23 @@ export const executeServiceAction = async ({
 		throw new NotFoundError(messages.targetServiceNotFound);
 	}
 
+	// A single service start action doesn't require locks
+	if (action === 'start') {
 		// Execute action on service
-	return await executeDeviceAction(
+		await executeDeviceAction(
 			generateStep(action, {
-			current: currentService,
 				target: targetService,
-			wait: true,
 			}),
-		force,
 		);
+	} else {
+		await executeDeviceActionWithLock({
+			action,
+			appId,
+			currentService,
+			targetService,
+			force,
+		});
+	}
 };
 
 /**

src/device-state.ts

@@ -23,6 +23,7 @@ import {
 	UpdatesLockedError,
 } from './lib/errors';
 import * as updateLock from './lib/update-lock';
+import { takeGlobalLockRO, takeGlobalLockRW } from './lib/process-lock';
 import * as dbFormat from './device-state/db-format';
 import { getGlobalApiKey } from './device-api';
 import * as sysInfo from './lib/system-info';
@@ -101,8 +102,6 @@ type DeviceStateStep<T extends PossibleStepTargets> =
 	| deviceConfig.ConfigStep;
 
 let currentVolatile: DeviceReport = {};
-const writeLock = updateLock.writeLock;
-const readLock = updateLock.readLock;
 let maxPollTime: number;
 let intermediateTarget: InstancedDeviceState | null = null;
 let applyBlocker: Nullable<Promise<void>>;
@@ -295,11 +294,11 @@ function emitAsync<T extends keyof DeviceStateEvents>(
 }
 
 const readLockTarget = () =>
-	readLock('target').disposer((release) => release());
+	takeGlobalLockRO('target').disposer((release) => release());
 const writeLockTarget = () =>
-	writeLock('target').disposer((release) => release());
+	takeGlobalLockRW('target').disposer((release) => release());
 const inferStepsLock = () =>
-	writeLock('inferSteps').disposer((release) => release());
+	takeGlobalLockRW('inferSteps').disposer((release) => release());
 function usingReadLockTarget<T extends () => any, U extends ReturnType<T>>(
 	fn: T,
 ): Bluebird<UnwrappedPromise<U>> {
@@ -575,11 +574,7 @@ export async function shutdown({
 // should happen via intermediate targets
 export async function executeStepAction(
 	step: DeviceStateStep<PossibleStepTargets>,
-	{
+	{ force, initial }: { force?: boolean; initial?: boolean },
-		force,
-		initial,
-		skipLock,
-	}: { force?: boolean; initial?: boolean; skipLock?: boolean },
 ) {
 	if (deviceConfig.isValidAction(step.action)) {
 		await deviceConfig.executeStepAction(step as deviceConfig.ConfigStep, {
@@ -588,7 +583,6 @@ export async function executeStepAction(
 	} else if (applicationManager.validActions.includes(step.action)) {
 		return applicationManager.executeStep(step as any, {
 			force,
-			skipLock,
 		});
 	} else {
 		switch (step.action) {
@@ -614,11 +608,9 @@ export async function applyStep(
 	{
 		force,
 		initial,
-		skipLock,
 	}: {
 		force?: boolean;
 		initial?: boolean;
-		skipLock?: boolean;
 	},
 ) {
 	if (shuttingDown) {
@@ -628,7 +620,6 @@ export async function applyStep(
 		await executeStepAction(step, {
 			force,
 			initial,
-			skipLock,
 		});
 		emitAsync('step-completed', null, step);
 	} catch (e: any) {
@@ -686,7 +677,6 @@ export const applyTarget = async ({
 	force = false,
 	initial = false,
 	intermediate = false,
-	skipLock = false,
 	nextDelay = 200,
 	retryCount = 0,
 	keepVolumes = undefined as boolean | undefined,
@@ -725,6 +715,7 @@ export const applyTarget = async ({
 				// the value
 				intermediate || undefined,
 				keepVolumes,
+				force,
 			);
 
 			if (_.isEmpty(appSteps)) {
@@ -770,16 +761,13 @@ export const applyTarget = async ({
 		}
 
 		try {
-			await Promise.all(
+			await Promise.all(steps.map((s) => applyStep(s, { force, initial })));
-				steps.map((s) => applyStep(s, { force, initial, skipLock })),
-			);
 
 			await setTimeout(nextDelay);
 			await applyTarget({
 				force,
 				initial,
 				intermediate,
-				skipLock,
 				nextDelay,
 				retryCount,
 				keepVolumes,
@@ -803,7 +791,7 @@ export const applyTarget = async ({
 
 function pausingApply(fn: () => any) {
 	const lock = () => {
-		return writeLock('pause').disposer((release) => release());
+		return takeGlobalLockRW('pause').disposer((release) => release());
 	};
 	// TODO: This function is a bit of a mess
 	const pause = () => {
@@ -885,11 +873,7 @@ export function triggerApplyTarget({
 
 export async function applyIntermediateTarget(
 	intermediate: InstancedDeviceState,
-	{
+	{ force = false, keepVolumes = undefined as boolean | undefined } = {},
-		force = false,
-		skipLock = false,
-		keepVolumes = undefined as boolean | undefined,
-	} = {},
 ) {
 	return pausingApply(async () => {
 		// TODO: Make sure we don't accidentally overwrite this
@@ -898,7 +882,6 @@ export async function applyIntermediateTarget(
 		return applyTarget({
 			intermediate: true,
 			force,
-			skipLock,
 			keepVolumes,
 		}).then(() => {
 			intermediateTarget = null;

src/device-state/preload.ts

@@ -12,8 +12,8 @@ import * as imageManager from '../compose/images';
 
 import {
 	AppsJsonParseError,
-	EISDIR,
+	isEISDIR,
-	ENOENT,
+	isENOENT,
 	InternalInconsistencyError,
 } from '../lib/errors';
 import log from '../lib/supervisor-console';
@@ -163,7 +163,7 @@ export async function loadTargetFromFile(appsPath: string): Promise<boolean> {
 		// It can be an empty path because if the file does not exist
 		// on host, the docker daemon creates an empty directory when
 		// the bind mount is added
-		if (ENOENT(e) || EISDIR(e)) {
+		if (isENOENT(e) || isEISDIR(e)) {
 			log.debug('No apps.json file present, skipping preload');
 		} else {
 			log.debug(e.message);

src/device-state/target-state.ts

@@ -8,7 +8,7 @@ import type { TargetState } from '../types/state';
 import { InternalInconsistencyError } from '../lib/errors';
 import { getGotInstance } from '../lib/request';
 import * as config from '../config';
-import { writeLock } from '../lib/update-lock';
+import { takeGlobalLockRW } from '../lib/process-lock';
 import * as constants from '../lib/constants';
 import log from '../lib/supervisor-console';
 
@@ -26,7 +26,7 @@ export const emitter: StrictEventEmitter<EventEmitter, TargetStateEvents> =
 	new EventEmitter();
 
 const lockGetTarget = () =>
-	writeLock('getTarget').disposer((release) => release());
+	takeGlobalLockRW('getTarget').disposer((release) => release());
 
 type CachedResponse = {
 	etag?: string | string[];

src/event-tracker.ts

@@ -5,6 +5,8 @@ export type EventTrackProperties = Dictionary<any>;
 
 const mixpanelMask = [
 	'appId',
+	'force',
+	'services',
 	'delay',
 	'error',
 	'interval',

src/host-config.ts

@@ -6,7 +6,7 @@ import path from 'path';
 import * as config from './config';
 import * as applicationManager from './compose/application-manager';
 import * as dbus from './lib/dbus';
-import { ENOENT } from './lib/errors';
+import { isENOENT } from './lib/errors';
 import { mkdirp, unlinkAll } from './lib/fs-utils';
 import {
 	writeToBoot,
@@ -66,8 +66,8 @@ async function readProxy(): Promise<ProxyConfig | undefined> {
 	let redsocksConf: string;
 	try {
 		redsocksConf = await readFromBoot(redsocksConfPath, 'utf-8');
-	} catch (e: any) {
+	} catch (e: unknown) {
-		if (!ENOENT(e)) {
+		if (!isENOENT(e)) {
 			throw e;
 		}
 		return;
@@ -97,8 +97,8 @@ async function readProxy(): Promise<ProxyConfig | undefined> {
 		if (noProxy.length) {
 			conf.noProxy = noProxy;
 		}
-	} catch (e: any) {
+	} catch (e: unknown) {
-		if (!ENOENT(e)) {
+		if (!isENOENT(e)) {
 			throw e;
 		}
 	}

src/lib/errors.ts

@@ -40,23 +40,27 @@ export class BadRequestError extends StatusError {
 export const isBadRequestError = (e: unknown): e is BadRequestError =>
 	isStatusError(e) && e.statusCode === 400;
 
+export class DeviceNotFoundError extends TypedError {}
+
 interface CodedSysError extends Error {
 	code?: string;
 }
 
-export class DeviceNotFoundError extends TypedError {}
+const isCodedSysError = (e: unknown): e is CodedSysError =>
+	// See https://mdn.io/hasOwn
+	e != null && e instanceof Error && Object.hasOwn(e, 'code');
 
-export function ENOENT(err: CodedSysError): boolean {
+export const isENOENT = (e: unknown): e is CodedSysError =>
-	return err.code === 'ENOENT';
+	isCodedSysError(e) && e.code === 'ENOENT';
-}
 
-export function EEXIST(err: CodedSysError): boolean {
+export const isEEXIST = (e: unknown): e is CodedSysError =>
-	return err.code === 'EEXIST';
+	isCodedSysError(e) && e.code === 'EEXIST';
-}
 
-export function EISDIR(err: CodedSysError): boolean {
+export const isEISDIR = (e: unknown): e is CodedSysError =>
-	return err.code === 'EISDIR';
+	isCodedSysError(e) && e.code === 'EISDIR';
-}
+
+export const isEPERM = (e: unknown): e is CodedSysError =>
+	isCodedSysError(e) && e.code === 'EPERM';
 
 export function UnitNotLoadedError(err: string[]): boolean {
 	return endsWith(err[0], 'not loaded.');

src/lib/fs-utils.ts

@@ -3,6 +3,7 @@ import path from 'path';
 import { exec as execSync } from 'child_process';
 import { promisify } from 'util';
 import { uptime } from 'os';
+import { isENOENT } from './errors';
 
 export const exec = promisify(execSync);
 
@@ -76,7 +77,7 @@ export const touch = (file: string, time = new Date()) =>
 	fs.utimes(file, time, time).catch((e) =>
 		// only create the file if it doesn't exist,
 		// if some other error happens is probably better to not touch it
-		e.code === 'ENOENT'
+		isENOENT(e)
 			? fs
 					.open(file, 'w')
 					.then((fd) => fd.close())

src/lib/lockfile.ts

@@ -1,34 +1,44 @@
-import { promises as fs, unlinkSync, rmdirSync } from 'fs';
+import { promises as fs } from 'fs';
+import type { Stats, Dirent } from 'fs';
 import os from 'os';
 import { dirname } from 'path';
 
 import { exec } from './fs-utils';
+import { isENOENT, isEISDIR, isEPERM } from './errors';
 
 // Equivalent to `drwxrwxrwt`
 const STICKY_WRITE_PERMISSIONS = 0o1777;
 
-/**
+// Returns all current locks taken under a directory (default: /tmp)
- * Internal lockfile manager to track files in memory
- */
-// Track locksTaken, so that the proper locks can be cleaned up on process exit
-const locksTaken: { [lockName: string]: boolean } = {};
-
-// Returns all current locks taken, as they've been stored in-memory.
 // Optionally accepts filter function for only getting locks that match a condition.
-export const getLocksTaken = (
+// A file is counted as a lock by default if it ends with `.lock`.
-	lockFilter: (path: string) => boolean = () => true,
+export const getLocksTaken = async (
-): string[] => Object.keys(locksTaken).filter(lockFilter);
+	rootDir: string = '/tmp',
-
+	lockFilter: (path: string, stat: Stats) => boolean = (p) =>
-// Try to clean up any existing locks when the process exits
+		p.endsWith('.lock'),
-process.on('exit', () => {
+): Promise<string[]> => {
-	for (const lockName of getLocksTaken()) {
+	const locksTaken: string[] = [];
+	let filesOrDirs: Dirent[] = [];
 	try {
-			unlockSync(lockName);
+		filesOrDirs = await fs.readdir(rootDir, { withFileTypes: true });
-		} catch (e) {
+	} catch (err) {
-			// Ignore unlocking errors
+		// If lockfile directory doesn't exist, no locks are taken
+		if (isENOENT(err)) {
+			return locksTaken;
 		}
 	}
-});
+	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
+		if (lockFilter(lockPath, await fs.stat(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;
+};
 
 interface ChildProcessError {
 	code: number;
@@ -77,8 +87,6 @@ export async function lock(path: string, uid: number = os.userInfo().uid) {
 	try {
 		// Lock the file using binary
 		await exec(`lockfile -r 0 ${path}`, { uid });
-		// Store a lock in memory as taken
-		locksTaken[path] = true;
 	} catch (error) {
 		// Code 73 refers to EX_CANTCREAT (73) in sysexits.h, or:
 		// A (user specified) output file cannot be created.
@@ -110,7 +118,7 @@ export async function unlock(path: string): Promise<void> {
 	// Removing the lockfile releases the lock
 	await fs.unlink(path).catch((e) => {
 		// if the error is EPERM|EISDIR, the file is a directory
-		if (e.code === 'EPERM' || e.code === 'EISDIR') {
+		if (isEPERM(e) || isEISDIR(e)) {
 			return fs.rmdir(path).catch(() => {
 				// if the directory is not empty or something else
 				// happens, ignore
@@ -119,17 +127,4 @@ export async function unlock(path: string): Promise<void> {
 		// If the file does not exist or some other error
 		// happens, then ignore the error
 	});
-	// Remove lockfile's in-memory tracking of a file
-	delete locksTaken[path];
-}
-
-export function unlockSync(path: string) {
-	try {
-		return unlinkSync(path);
-	} catch (e: any) {
-		if (e.code === 'EPERM' || e.code === 'EISDIR') {
-			return rmdirSync(path);
-		}
-		throw e;
-	}
 }

src/lib/log-types.ts

@@ -168,3 +168,13 @@ export const removeNetworkError: LogType = {
 	eventName: 'Network removal error',
 	humanName: 'Error removing network',
 };
+
+export const takeLock: LogType = {
+	eventName: 'Take update locks',
+	humanName: 'Taking update locks',
+};
+
+export const releaseLock: LogType = {
+	eventName: 'Release update locks',
+	humanName: 'Releasing update locks',
+};

src/lib/process-lock.ts

@@ -0,0 +1,35 @@
+/**
+ * This module contains the functionality for locking & unlocking resources
+ * within the Supervisor Node process, useful for methods that need to acquire
+ * exclusive access to a resource across multiple ticks in the event loop, async
+ * functions for example.
+ *
+ * It is different from lockfile and update-lock modules, which handle
+ * inter-container communication via lockfiles.
+ *
+ * TODO:
+ * - Use a maintained solution such as async-lock
+ * - Move to native Promises
+ */
+
+import Bluebird from 'bluebird';
+import Lock from 'rwlock';
+import type { Release } from 'rwlock';
+
+type LockFn = (key: string | number) => Bluebird<Release>;
+
+const locker = new Lock();
+
+export const takeGlobalLockRW: LockFn = Bluebird.promisify(
+	locker.async.writeLock,
+	{
+		context: locker,
+	},
+);
+
+export const takeGlobalLockRO: LockFn = Bluebird.promisify(
+	locker.async.readLock,
+	{
+		context: locker,
+	},
+);

src/lib/update-lock.ts

@@ -1,18 +1,21 @@
-import Bluebird from 'bluebird';
 import { promises as fs } from 'fs';
 import path from 'path';
-import Lock from 'rwlock';
+import type { Stats } from 'fs';
 import { isRight } from 'fp-ts/lib/Either';
 
 import {
-	ENOENT,
+	isENOENT,
 	UpdatesLockedError,
 	InternalInconsistencyError,
 } from './errors';
 import { pathOnRoot, pathExistsOnState } from './host-utils';
+import { mkdirp } from './fs-utils';
 import * as config from '../config';
 import * as lockfile from './lockfile';
-import { NumericIdentifier } from '../types';
+import { NumericIdentifier, StringIdentifier, DockerName } from '../types';
+import { takeGlobalLockRW } from './process-lock';
+import * as logger from '../logger';
+import * as logTypes from './log-types';
 
 const decodedUid = NumericIdentifier.decode(process.env.LOCKFILE_UID);
 export const LOCKFILE_UID = isRight(decodedUid) ? decodedUid.right : 65534;
@@ -56,24 +59,19 @@ export function abortIfHUPInProgress({
 	});
 }
 
-type LockFn = (key: string | number) => Bluebird<() => void>;
+/**
-const locker = new Lock();
+ * Unlock all lockfiles of an appId | appUuid, then release resources.
-export const writeLock: LockFn = Bluebird.promisify(locker.async.writeLock, {
+ * Meant for use in update-lock module only as as it assumes that a
-	context: locker,
+ * write lock has been acquired.
-});
+ */
-export const readLock: LockFn = Bluebird.promisify(locker.async.readLock, {
-	context: locker,
-});
-
-// Unlock all lockfiles, optionally of an appId | appUuid, then release resources.
 async function dispose(
 	appIdentifier: string | number,
 	release: () => void,
 ): Promise<void> {
-	const locks = lockfile.getLocksTaken((p: string) =>
-		p.includes(`${BASE_LOCK_DIR}/${appIdentifier}`),
-	);
 	try {
+		const locks = await getLocksTaken(
+			pathOnRoot(`${BASE_LOCK_DIR}/${appIdentifier}`),
+		);
 		// Try to unlock all locks taken
 		await Promise.all(locks.map((l) => lockfile.unlock(l)));
 	} finally {
@@ -82,22 +80,196 @@ async function dispose(
 	}
 }
 
+/**
+ * Composition step used by Supervisor compose module.
+ * Take all locks for an appId | appUuid, creating directories if they don't exist.
+ */
+export async function takeLock(
+	appId: number,
+	services: string[],
+	force: boolean = false,
+) {
+	logger.logSystemEvent(logTypes.takeLock, {
+		appId,
+		services,
+		force,
+	});
+
+	const release = await takeGlobalLockRW(appId);
+	try {
+		const actuallyLocked: string[] = [];
+		const locksTaken = await getServicesLockedByAppId();
+		// Filter out services that already have Supervisor-taken locks.
+		// This needs to be done after taking the appId write lock to avoid
+		// race conditions with locking.
+		const servicesWithoutLock = services.filter(
+			(svc) => !locksTaken.isLocked(appId, svc),
+		);
+		for (const service of servicesWithoutLock) {
+			await mkdirp(pathOnRoot(lockPath(appId, service)));
+			await lockService(appId, service, force);
+			actuallyLocked.push(service);
+		}
+		return actuallyLocked;
+	} catch (err) {
+		// If something errors while taking the lock, we should remove any
+		// lockfiles that may have been created so that all services return
+		// to unlocked status.
+		await dispose(appId, release);
+		// Re-throw error to be handled in caller
+		throw err;
+	} finally {
+		// If not already released from catch, released the RW process lock.
+		// If already released, this will not error.
+		release();
+	}
+}
+
+/**
+ * Composition step used by Supervisor compose module.
+ * Release all locks for an appId | appUuid.
+ */
+export async function releaseLock(appId: number) {
+	logger.logSystemEvent(logTypes.releaseLock, { appId });
+
+	const release = await takeGlobalLockRW(appId);
+	await dispose(appId, release);
+}
+
+/**
+ * Given a lockfile path `p`, return an array [appId, serviceName, filename] of that path.
+ * Paths are assumed to end in the format /:appId/:serviceName/(resin-)updates.lock.
+ */
+function getIdentifiersFromPath(p: string) {
+	const parts = p.split('/');
+	const filename = parts.pop();
+	if (filename?.match(/updates\.lock/) === null) {
+		return [];
+	}
+	const serviceName = parts.pop();
+	const appId = parts.pop();
+	return [appId, serviceName, filename];
+}
+
+type LockedEntity = { appId: number; services: string[] };
+
+/**
+ * A map of locked services by appId.
+ * Exported for tests only; getServicesLockedByAppId is the public generator interface.
+ */
+export class LocksTakenMap extends Map<number, Set<string>> {
+	constructor(lockedEntities: LockedEntity[] = []) {
+		// Construct a Map<number, Set<string>> from user-friendly input args
+		super(
+			lockedEntities.map(({ appId, services }) => [appId, new Set(services)]),
+		);
+	}
+
+	// Add one or more locked services to an appId
+	public add(appId: number, services: string | string[]): void {
+		if (typeof services === 'string') {
+			services = [services];
+		}
+		if (this.has(appId)) {
+			const lockedSvcs = this.get(appId)!;
+			services.forEach((s) => lockedSvcs.add(s));
+		} else {
+			this.set(appId, new Set(services));
+		}
+	}
+
+	/**
+	 * @private Use this.getServices instead as there is no need to return
+	 * a mutable reference to the internal Set data structure.
+	 */
+	public get(appId: number): Set<string> | undefined {
+		return super.get(appId);
+	}
+
+	// Return an array copy of locked services under an appId
+	public getServices(appId: number): string[] {
+		return this.has(appId) ? Array.from(this.get(appId)!) : [];
+	}
+
+	// Return whether a service is locked under an appId
+	public isLocked(appId: number, service: string): boolean {
+		return this.has(appId) && this.get(appId)!.has(service);
+	}
+}
+
+// A wrapper function for lockfile.getLocksTaken that filters for Supervisor-taken locks.
+// Exported for tests only; getServicesLockedByAppId is the intended public interface.
+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 a list of services that are locked by the Supervisor under each appId.
+ * Both `resin-updates.lock` and `updates.lock` should be present per
+ * [appId, serviceName] pair for a service to be considered locked.
+ */
+export async function getServicesLockedByAppId(): Promise<LocksTakenMap> {
+	const locksTaken = await getLocksTaken();
+	// Group locksTaken paths by appId & serviceName.
+	// filesTakenByAppId is of type Map<appId, Map<serviceName, Set<filename>>>
+	// and represents files taken under every [appId, serviceName] pair.
+	const filesTakenByAppId = new Map<number, Map<string, Set<string>>>();
+	for (const lockTakenPath of locksTaken) {
+		const [appId, serviceName, filename] =
+			getIdentifiersFromPath(lockTakenPath);
+		if (
+			!StringIdentifier.is(appId) ||
+			!DockerName.is(serviceName) ||
+			!filename?.match(/updates\.lock/)
+		) {
+			continue;
+		}
+		const numAppId = +appId;
+		if (!filesTakenByAppId.has(numAppId)) {
+			filesTakenByAppId.set(numAppId, new Map());
+		}
+		const servicesTaken = filesTakenByAppId.get(numAppId)!;
+		if (!servicesTaken.has(serviceName)) {
+			servicesTaken.set(serviceName, new Set());
+		}
+		servicesTaken.get(serviceName)!.add(filename);
+	}
+
+	// Construct a LocksTakenMap from filesTakenByAppId, which represents
+	// services locked by the Supervisor.
+	const servicesByAppId = new LocksTakenMap();
+	for (const [appId, servicesTaken] of filesTakenByAppId) {
+		for (const [serviceName, filenames] of servicesTaken) {
+			if (
+				filenames.has('resin-updates.lock') &&
+				filenames.has('updates.lock')
+			) {
+				servicesByAppId.add(appId, serviceName);
+			}
+		}
+	}
+	return servicesByAppId;
+}
+
 /**
  * Try to take the locks for an application. If force is set, it will remove
  * all existing lockfiles before performing the operation
  *
  * TODO: convert to native Promises and async/await. May require native implementation of Bluebird's dispose / using
- *
- * TODO: Remove skipLock as it's not a good interface. If lock is called it should try to take the lock
- * without an option to skip.
  */
 export async function lock<T>(
 	appId: number | number[],
-	{ force = false, skipLock = false }: { force: boolean; skipLock?: boolean },
+	{ force = false }: { force: boolean },
 	fn: () => Resolvable<T>,
 ): Promise<T> {
 	const appIdsToLock = Array.isArray(appId) ? appId : [appId];
-	if (skipLock || !appId || !appIdsToLock.length) {
+	if (!appId || !appIdsToLock.length) {
 		return fn();
 	}
 
@@ -118,10 +290,10 @@ export async function lock<T>(
 		for (const id of sortedIds) {
 			const lockDir = pathOnRoot(lockPath(id));
 			// Acquire write lock for appId
-			releases.set(id, await writeLock(id));
+			releases.set(id, await takeGlobalLockRW(id));
 			// Get list of service folders in lock directory
 			const serviceFolders = await fs.readdir(lockDir).catch((e) => {
-				if (ENOENT(e)) {
+				if (isENOENT(e)) {
 					return [];
 				}
 				throw e;

src/logger.ts

@@ -5,7 +5,7 @@ import * as config from './config';
 import * as db from './db';
 import * as eventTracker from './event-tracker';
 import type { LogType } from './lib/log-types';
-import { writeLock } from './lib/update-lock';
+import { takeGlobalLockRW } from './lib/process-lock';
 import type { LogBackend, LogMessage } from './logging';
 import { BalenaLogBackend, LocalLogBackend } from './logging';
 import type { MonitorHook } from './logging/monitor';
@@ -129,7 +129,7 @@ export function logSystemMessage(
 }
 
 export function lock(containerId: string): Bluebird.Disposer<() => void> {
-	return writeLock(containerId).disposer((release) => {
+	return takeGlobalLockRW(containerId).disposer((release) => {
 		release();
 	});
 }

src/network.ts

@@ -5,7 +5,7 @@ import os from 'os';
 import url from 'url';
 
 import * as constants from './lib/constants';
-import { EEXIST } from './lib/errors';
+import { isEEXIST } from './lib/errors';
 import { checkFalsey } from './lib/validation';
 
 import blink = require('./lib/blink');
@@ -71,7 +71,7 @@ export const startConnectivityCheck = _.once(
 		try {
 			await fs.mkdir(constants.vpnStatusPath);
 		} catch (err: any) {
-			if (EEXIST(err)) {
+			if (isEEXIST(err)) {
 				log.debug('VPN status path exists.');
 			} else {
 				throw err;

test/integration/compose/application-manager.spec.ts

@@ -8,6 +8,7 @@ import Network from '~/src/compose/network';
 import * as networkManager from '~/src/compose/network-manager';
 import Volume from '~/src/compose/volume';
 import * as config from '~/src/config';
+import { LocksTakenMap } from '~/lib/update-lock';
 import { createDockerImage } from '~/test-lib/docker-helper';
 import {
 	createService,
@@ -111,6 +112,8 @@ describe('compose/application-manager', () => {
 				downloading,
 				availableImages,
 				containerIdsByAppId,
+				// Mock lock taken to avoid takeLock step
+				locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
 			},
 		);
 
@@ -221,6 +224,8 @@ describe('compose/application-manager', () => {
 				downloading,
 				availableImages,
 				containerIdsByAppId,
+				// Mock lock taken to avoid takeLock step
+				locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
 			},
 		);
 
@@ -271,6 +276,8 @@ describe('compose/application-manager', () => {
 				downloading,
 				availableImages,
 				containerIdsByAppId,
+				// Mock lock taken to avoid takeLock step
+				locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
 			},
 		);
 
@@ -402,6 +409,8 @@ describe('compose/application-manager', () => {
 					downloading: c1.downloading,
 					availableImages: c1.availableImages,
 					containerIdsByAppId: c1.containerIdsByAppId,
+					// Mock lock taken for `main` service which just needs metadata updated
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
 				},
 			);
 			// There should be two noop steps, one for target service which is still downloading,
@@ -448,6 +457,10 @@ describe('compose/application-manager', () => {
 					downloading,
 					availableImages,
 					containerIdsByAppId,
+					// Mock locks taken for all services in either current or target state
+					locksTaken: new LocksTakenMap([
+						{ appId: 1, services: ['old', 'main', 'new'] },
+					]),
 				},
 			);
 			// Service `old` is safe to kill after download for `new` has completed
@@ -493,6 +506,11 @@ describe('compose/application-manager', () => {
 					// to avoid removeImage steps
 					availableImages: [],
 					containerIdsByAppId: c1.containerIdsByAppId,
+					// Mock locks for service to be updated via updateMetadata
+					// or kill to avoid takeLock step
+					locksTaken: new LocksTakenMap([
+						{ appId: 1, services: ['old', 'main', 'new'] },
+					]),
 				},
 			);
 			// Service `new` should be fetched
@@ -565,6 +583,10 @@ describe('compose/application-manager', () => {
 						}),
 					],
 					containerIdsByAppId: c1.containerIdsByAppId,
+					// Mock lock taken for all services in target state
+					locksTaken: new LocksTakenMap([
+						{ appId: 1, services: ['old', 'main', 'new'] },
+					]),
 				},
 			);
 			// Service `new` should be started
@@ -605,6 +627,11 @@ describe('compose/application-manager', () => {
 					// to avoid removeImage steps
 					availableImages: [],
 					containerIdsByAppId: c1.containerIdsByAppId,
+					// Mock locks for service to be updated via updateMetadata
+					// or kill to avoid takeLock step
+					locksTaken: new LocksTakenMap([
+						{ appId: 1, services: ['old', 'main', 'new'] },
+					]),
 				},
 			);
 			// Service `new` should be fetched
@@ -677,6 +704,10 @@ describe('compose/application-manager', () => {
 						}),
 					],
 					containerIdsByAppId: c1.containerIdsByAppId,
+					// Mock lock taken for all services in target state
+					locksTaken: new LocksTakenMap([
+						{ appId: 1, services: ['main', 'new'] },
+					]),
 				},
 			);
 			// Service `new` should be started
@@ -773,6 +804,10 @@ describe('compose/application-manager', () => {
 						}),
 					],
 					containerIdsByAppId,
+					// Mock locks taken for all services in target state
+					locksTaken: new LocksTakenMap([
+						{ appId: 1, services: ['one', 'two'] },
+					]),
 				},
 			);
 			expectSteps('start', steps3, 2);
@@ -841,6 +876,8 @@ describe('compose/application-manager', () => {
 				downloading,
 				availableImages,
 				containerIdsByAppId,
+				// Mock lock taken to avoid takeLock step
+				locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
 			},
 		);
 
@@ -921,6 +958,10 @@ describe('compose/application-manager', () => {
 				downloading,
 				availableImages,
 				containerIdsByAppId,
+				// Mock locks taken to avoid takeLock step
+				locksTaken: new LocksTakenMap([
+					{ appId: 1, services: ['main', 'dep'] },
+				]),
 			},
 		);
 
@@ -986,10 +1027,14 @@ describe('compose/application-manager', () => {
 				downloading,
 				availableImages,
 				containerIdsByAppId,
+				// Mock locks taken to avoid takeLock step
+				locksTaken: new LocksTakenMap([
+					{ appId: 1, services: ['main', 'dep'] },
+				]),
 			},
 		);
 
-		// A start step should happen for the depended service first
+		// A start step should happen for the dependant service first
 		expect(startStep).to.have.property('action').that.equals('start');
 		expect(startStep)
 			.to.have.property('target')
@@ -1054,6 +1099,10 @@ describe('compose/application-manager', () => {
 				downloading,
 				availableImages,
 				containerIdsByAppId,
+				// Mock locks taken to avoid takeLock step
+				locksTaken: new LocksTakenMap([
+					{ appId: 1, services: ['main', 'dep'] },
+				]),
 			},
 		);
 
@@ -1096,6 +1145,11 @@ describe('compose/application-manager', () => {
 				downloading,
 				availableImages,
 				containerIdsByAppId,
+				// Mock lock already taken for the new and leftover services
+				locksTaken: new LocksTakenMap([
+					{ appId: 5, services: ['old-service'] },
+					{ appId: 1, services: ['main'] },
+				]),
 			},
 		);
 
@@ -1601,6 +1655,11 @@ describe('compose/application-manager', () => {
 				downloading,
 				availableImages,
 				containerIdsByAppId,
+				// Mock locks taken to avoid takeLock step
+				locksTaken: new LocksTakenMap([
+					{ appId: 1, services: ['main'] },
+					{ appId: 2, services: ['main'] },
+				]),
 			},
 		);
 
@@ -1623,6 +1682,699 @@ describe('compose/application-manager', () => {
 		).to.have.lengthOf(1);
 	});
 
+	describe('taking and releasing locks', () => {
+		it('should take locks for all services in current state when they should be killed', async () => {
+			const targetApps = createApps(
+				{
+					services: [],
+					networks: [DEFAULT_NETWORK],
+				},
+				true,
+			);
+			const { currentApps, availableImages, downloading, containerIdsByAppId } =
+				createCurrentState({
+					services: [
+						await createService({ serviceName: 'one' }),
+						await createService({ serviceName: 'two' }),
+					],
+					networks: [DEFAULT_NETWORK],
+					images: [],
+				});
+
+			// takeLock
+			const steps = await applicationManager.inferNextSteps(
+				currentApps,
+				targetApps,
+				{
+					downloading,
+					availableImages,
+					containerIdsByAppId,
+				},
+			);
+			const [takeLockStep] = expectSteps('takeLock', steps, 1, 1);
+			expect(takeLockStep)
+				.to.have.property('services')
+				.that.deep.includes.members(['one', 'two']);
+
+			// kill
+			const steps2 = await applicationManager.inferNextSteps(
+				currentApps,
+				targetApps,
+				{
+					downloading,
+					availableImages,
+					containerIdsByAppId,
+					// Mock locks taken
+					locksTaken: new LocksTakenMap([
+						{ appId: 1, services: ['one', 'two'] },
+					]),
+				},
+			);
+			expectSteps('kill', steps2, 2);
+		});
+
+		it('should take locks for all services in current state when they should be stopped', async () => {
+			const targetApps = createApps(
+				{
+					services: [
+						await createService({
+							serviceName: 'one',
+							running: false,
+						}),
+						await createService({
+							serviceName: 'two',
+							running: false,
+						}),
+					],
+					networks: [DEFAULT_NETWORK],
+				},
+				true,
+			);
+			const { currentApps, availableImages, downloading, containerIdsByAppId } =
+				createCurrentState({
+					services: [
+						await createService({ serviceName: 'one' }),
+						await createService({ serviceName: 'two' }),
+					],
+					networks: [DEFAULT_NETWORK],
+					images: [],
+				});
+
+			// takeLock
+			const steps = await applicationManager.inferNextSteps(
+				currentApps,
+				targetApps,
+				{
+					downloading,
+					availableImages,
+					containerIdsByAppId,
+				},
+			);
+			const [takeLockStep] = expectSteps('takeLock', steps, 1, 1);
+			expect(takeLockStep)
+				.to.have.property('services')
+				.that.deep.includes.members(['one', 'two']);
+
+			// stop
+			const steps2 = await applicationManager.inferNextSteps(
+				currentApps,
+				targetApps,
+				{
+					downloading,
+					availableImages,
+					containerIdsByAppId,
+					// Mock locks taken
+					locksTaken: new LocksTakenMap([
+						{ appId: 1, services: ['one', 'two'] },
+					]),
+				},
+			);
+			expectSteps('stop', steps2, 2);
+		});
+
+		it('should take locks for all services in target state before they should be started', async () => {
+			const targetApps = createApps(
+				{
+					services: [
+						await createService({ serviceName: 'one', image: 'one-image' }),
+						await createService({ serviceName: 'two', image: 'two-image' }),
+					],
+					networks: [DEFAULT_NETWORK],
+				},
+				true,
+			);
+			const { currentApps, availableImages, downloading, containerIdsByAppId } =
+				createCurrentState({
+					services: [],
+					networks: [DEFAULT_NETWORK],
+					images: [
+						createImage({
+							serviceName: 'one',
+							name: 'one-image',
+						}),
+						createImage({
+							serviceName: 'two',
+							name: 'two-image',
+						}),
+					],
+				});
+
+			// takeLock
+			const steps = await applicationManager.inferNextSteps(
+				currentApps,
+				targetApps,
+				{
+					downloading,
+					availableImages,
+					containerIdsByAppId,
+				},
+			);
+			const [takeLockStep] = expectSteps('takeLock', steps, 1, 1);
+			expect(takeLockStep)
+				.to.have.property('services')
+				.that.deep.includes.members(['one', 'two']);
+
+			// start
+			const steps2 = await applicationManager.inferNextSteps(
+				currentApps,
+				targetApps,
+				{
+					downloading,
+					availableImages,
+					containerIdsByAppId,
+					// Mock locks taken
+					locksTaken: new LocksTakenMap([
+						{ appId: 1, services: ['one', 'two'] },
+					]),
+				},
+			);
+			expectSteps('start', steps2, 2);
+		});
+
+		it('should take locks for all services in current and target states when services should be stopped, started, or killed', async () => {
+			const targetApps = createApps(
+				{
+					services: [
+						await createService({
+							serviceName: 'to-stop',
+							image: 'image-to-stop',
+							running: false,
+						}),
+						await createService({
+							serviceName: 'to-start',
+							image: 'image-to-start',
+						}),
+						await createService({
+							serviceName: 'unchanged',
+							image: 'image-unchanged',
+						}),
+					],
+					networks: [DEFAULT_NETWORK],
+				},
+				true,
+			);
+			const { currentApps, availableImages, downloading, containerIdsByAppId } =
+				createCurrentState({
+					services: [
+						await createService({
+							serviceName: 'to-stop',
+							image: 'image-to-stop',
+						}),
+						await createService({
+							serviceName: 'to-kill',
+							image: 'image-to-kill',
+						}),
+						await createService({
+							serviceName: 'unchanged',
+							image: 'image-unchanged',
+						}),
+					],
+					networks: [DEFAULT_NETWORK],
+					images: [
+						createImage({ serviceName: 'to-start', name: 'image-to-start' }),
+						createImage({ serviceName: 'to-stop', name: 'image-to-stop' }),
+						createImage({ serviceName: 'unchanged', name: 'image-unchanged' }),
+					],
+				});
+
+			const steps = await applicationManager.inferNextSteps(
+				currentApps,
+				targetApps,
+				{
+					downloading,
+					availableImages,
+					containerIdsByAppId,
+				},
+			);
+			// No matter the number of services, we should see a single takeLock for all services
+			// regardless if they're only in current or only in target
+			const [takeLockStep] = expectSteps('takeLock', steps, 1, 1);
+			expect(takeLockStep)
+				.to.have.property('services')
+				.that.deep.includes.members([
+					'to-stop',
+					'to-kill',
+					'to-start',
+					'unchanged',
+				]);
+		});
+
+		it('should download images before taking locks & killing when update strategy is download-then-kill or handover', async () => {
+			const targetApps = createApps(
+				{
+					services: [
+						await createService({
+							serviceName: 'one',
+							image: 'one',
+							labels: {
+								'io.balena.update.strategy': 'download-then-kill',
+								'io.updated': 'true',
+							},
+						}),
+						await createService({
+							serviceName: 'two',
+							image: 'two',
+							labels: {
+								'io.balena.update.strategy': 'handover',
+								'io.updated': 'true',
+							},
+						}),
+					],
+					networks: [DEFAULT_NETWORK],
+				},
+				true,
+			);
+			const { currentApps, availableImages, downloading, containerIdsByAppId } =
+				createCurrentState({
+					services: [
+						await createService({
+							serviceName: 'one',
+							image: 'one',
+							labels: { 'io.balena.update.strategy': 'download-then-kill' },
+						}),
+						await createService({
+							serviceName: 'two',
+							image: 'two',
+							labels: { 'io.balena.update.strategy': 'handover' },
+						}),
+					],
+					networks: [DEFAULT_NETWORK],
+				});
+
+			// fetch
+			const steps = await applicationManager.inferNextSteps(
+				currentApps,
+				targetApps,
+				{
+					downloading: [],
+					availableImages: [],
+					containerIdsByAppId,
+				},
+			);
+			expectSteps('fetch', steps, 2);
+
+			// noop while downloading
+			const steps2 = await applicationManager.inferNextSteps(
+				currentApps,
+				targetApps,
+				{
+					downloading: ['one', 'two'],
+					availableImages: [],
+					containerIdsByAppId,
+				},
+			);
+			expectSteps('noop', steps2, 2);
+
+			// takeLock after download complete
+			const steps3 = await applicationManager.inferNextSteps(
+				currentApps,
+				targetApps,
+				{
+					downloading,
+					availableImages,
+					containerIdsByAppId,
+				},
+			);
+			const [takeLockStep] = expectSteps('takeLock', steps3, 1);
+			expect(takeLockStep)
+				.to.have.property('services')
+				.that.deep.includes.members(['one', 'two']);
+
+			// kill
+			const steps4 = await applicationManager.inferNextSteps(
+				currentApps,
+				targetApps,
+				{
+					downloading,
+					availableImages,
+					containerIdsByAppId,
+					// Mock locks taken
+					locksTaken: new LocksTakenMap([
+						{ appId: 1, services: ['one', 'two'] },
+					]),
+				},
+			);
+			expectSteps('kill', steps4, 2);
+		});
+
+		it('should take locks & kill before downloading images when update strategy is kill|delete-then-download', async () => {
+			const targetApps = createApps(
+				{
+					services: [
+						await createService({
+							serviceName: 'one',
+							labels: {
+								'io.balena.update.strategy': 'kill-then-download',
+								'io.updated': 'true',
+							},
+						}),
+						await createService({
+							serviceName: 'two',
+							labels: {
+								'io.balena.update.strategy': 'delete-then-download',
+								'io.updated': 'true',
+							},
+						}),
+					],
+					networks: [DEFAULT_NETWORK],
+				},
+				true,
+			);
+			const { currentApps, availableImages, containerIdsByAppId } =
+				createCurrentState({
+					services: [
+						await createService({
+							serviceName: 'one',
+							labels: { 'io.balena.update.strategy': 'kill-then-download' },
+						}),
+						await createService({
+							serviceName: 'two',
+							labels: { 'io.balena.update.strategy': 'delete-then-download' },
+						}),
+					],
+					networks: [DEFAULT_NETWORK],
+				});
+
+			// takeLock
+			const steps = await applicationManager.inferNextSteps(
+				currentApps,
+				targetApps,
+				{
+					// Images haven't finished downloading,
+					// but kill steps should still be inferred
+					downloading: ['one', 'two'],
+					availableImages,
+					containerIdsByAppId,
+				},
+			);
+			const [takeLockStep] = expectSteps('takeLock', steps, 1, 1);
+			expect(takeLockStep)
+				.to.have.property('services')
+				.that.deep.includes.members(['one', 'two']);
+
+			// kill
+			const steps2 = await applicationManager.inferNextSteps(
+				currentApps,
+				targetApps,
+				{
+					downloading: ['one', 'two'],
+					availableImages,
+					containerIdsByAppId,
+					// Mock locks taken
+					locksTaken: new LocksTakenMap([
+						{ appId: 1, services: ['one', 'two'] },
+					]),
+				},
+			);
+			expectSteps('kill', steps2, 2);
+		});
+
+		it('should infer takeLock & kill steps for dependent services before their network should be removed', async () => {
+			const targetApps = createApps(
+				{
+					services: [
+						await createService({ serviceName: 'main', appUuid: 'deadbeef' }),
+					],
+					networks: [DEFAULT_NETWORK],
+				},
+				true,
+			);
+			const { currentApps, availableImages, downloading, containerIdsByAppId } =
+				createCurrentState({
+					services: [
+						await createService({
+							serviceName: 'main',
+							appUuid: 'deadbeef',
+							composition: { networks: { test: {} } },
+						}),
+					],
+					networks: [
+						DEFAULT_NETWORK,
+						Network.fromComposeObject('test', 1, 'deadbeef', {
+							driver: 'bridge',
+						}),
+					],
+				});
+
+			// takeLock
+			const steps = await applicationManager.inferNextSteps(
+				currentApps,
+				targetApps,
+				{
+					downloading,
+					availableImages,
+					containerIdsByAppId,
+				},
+			);
+			const [takeLockStep] = expectSteps('takeLock', steps, 1, 1);
+			expect(takeLockStep)
+				.to.have.property('services')
+				.that.deep.includes.members(['main']);
+
+			// kill
+			const steps2 = await applicationManager.inferNextSteps(
+				currentApps,
+				targetApps,
+				{
+					downloading,
+					availableImages,
+					containerIdsByAppId,
+					// Mock locks taken
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
+				},
+			);
+			expectSteps('kill', steps2);
+
+			// removeNetwork
+			const intermediateCurrent = createCurrentState({
+				services: [],
+				networks: [
+					DEFAULT_NETWORK,
+					Network.fromComposeObject('test', 1, 'deadbeef', {
+						driver: 'bridge',
+					}),
+				],
+			});
+			const steps3 = await applicationManager.inferNextSteps(
+				intermediateCurrent.currentApps,
+				targetApps,
+				{
+					downloading: intermediateCurrent.downloading,
+					availableImages: intermediateCurrent.availableImages,
+					containerIdsByAppId: intermediateCurrent.containerIdsByAppId,
+					// Mock locks taken
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
+				},
+			);
+			expectSteps('removeNetwork', steps3);
+		});
+
+		it('should infer takeLock & kill steps for dependent services before their network should have config changed', async () => {
+			const targetApps = createApps(
+				{
+					services: [
+						await createService({
+							serviceName: 'main',
+							appUuid: 'deadbeef',
+							composition: { networks: { test: {} } },
+						}),
+					],
+					networks: [
+						DEFAULT_NETWORK,
+						Network.fromComposeObject('test', 1, 'deadbeef', {
+							driver: 'local',
+						}),
+					],
+				},
+				true,
+			);
+			const { currentApps, availableImages, downloading, containerIdsByAppId } =
+				createCurrentState({
+					services: [
+						await createService({
+							serviceName: 'main',
+							appUuid: 'deadbeef',
+							composition: { networks: { test: {} } },
+						}),
+					],
+					networks: [
+						DEFAULT_NETWORK,
+						Network.fromComposeObject('test', 1, 'deadbeef', {
+							driver: 'bridge',
+						}),
+					],
+				});
+
+			// takeLock
+			const steps = await applicationManager.inferNextSteps(
+				currentApps,
+				targetApps,
+				{
+					downloading,
+					availableImages,
+					containerIdsByAppId,
+				},
+			);
+			const [takeLockStep] = expectSteps('takeLock', steps, 1, 1);
+			expect(takeLockStep)
+				.to.have.property('services')
+				.that.deep.includes.members(['main']);
+
+			// kill
+			const steps2 = await applicationManager.inferNextSteps(
+				currentApps,
+				targetApps,
+				{
+					downloading,
+					availableImages,
+					containerIdsByAppId,
+					// Mock locks taken
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
+				},
+			);
+			expectSteps('kill', steps2, 1);
+
+			// removeNetwork
+			const intermediateCurrent = createCurrentState({
+				services: [],
+				networks: [
+					DEFAULT_NETWORK,
+					Network.fromComposeObject('test', 1, 'deadbeef', {
+						driver: 'bridge',
+					}),
+				],
+			});
+			const steps3 = await applicationManager.inferNextSteps(
+				intermediateCurrent.currentApps,
+				targetApps,
+				{
+					downloading: intermediateCurrent.downloading,
+					availableImages: intermediateCurrent.availableImages,
+					containerIdsByAppId: intermediateCurrent.containerIdsByAppId,
+					// Mock locks taken
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
+				},
+			);
+			expectSteps('removeNetwork', steps3);
+		});
+
+		it('should infer takeLock & kill steps for dependent services before their volume should have config changed', async () => {
+			const targetApps = createApps(
+				{
+					services: [
+						await createService({
+							serviceName: 'main',
+							appUuid: 'deadbeef',
+							composition: { volumes: ['test:/test'] },
+						}),
+					],
+					networks: [DEFAULT_NETWORK],
+					volumes: [
+						Volume.fromComposeObject('test', 1, 'deadbeef', {
+							labels: { 'io.updated': 'true' },
+						}),
+					],
+				},
+				true,
+			);
+			const { currentApps, availableImages, downloading, containerIdsByAppId } =
+				createCurrentState({
+					services: [
+						await createService({
+							serviceName: 'main',
+							appUuid: 'deadbeef',
+							composition: { volumes: ['test:/test'] },
+						}),
+					],
+					networks: [DEFAULT_NETWORK],
+					volumes: [Volume.fromComposeObject('test', 1, 'deadbeef')],
+				});
+
+			// takeLock
+			const steps = await applicationManager.inferNextSteps(
+				currentApps,
+				targetApps,
+				{
+					downloading,
+					availableImages,
+					containerIdsByAppId,
+				},
+			);
+			const [takeLockStep] = expectSteps('takeLock', steps, 1, 1);
+			expect(takeLockStep)
+				.to.have.property('services')
+				.that.deep.includes.members(['main']);
+
+			// kill
+			const steps2 = await applicationManager.inferNextSteps(
+				currentApps,
+				targetApps,
+				{
+					downloading,
+					availableImages,
+					containerIdsByAppId,
+					// Mock locks taken
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
+				},
+			);
+			expectSteps('kill', steps2, 1);
+
+			// removeVolume
+			const intermediateCurrent = createCurrentState({
+				services: [],
+				networks: [DEFAULT_NETWORK],
+				volumes: [Volume.fromComposeObject('test', 1, 'deadbeef')],
+			});
+			const steps3 = await applicationManager.inferNextSteps(
+				intermediateCurrent.currentApps,
+				targetApps,
+				{
+					downloading: intermediateCurrent.downloading,
+					availableImages: intermediateCurrent.availableImages,
+					containerIdsByAppId: intermediateCurrent.containerIdsByAppId,
+					// Mock locks taken
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
+				},
+			);
+			expectSteps('removeVolume', steps3);
+		});
+
+		it('should release locks before settling state', async () => {
+			const targetApps = createApps(
+				{
+					services: [
+						await createService({ serviceName: 'one' }),
+						await createService({ serviceName: 'two' }),
+					],
+					networks: [DEFAULT_NETWORK],
+				},
+				true,
+			);
+			const { currentApps, availableImages, downloading, containerIdsByAppId } =
+				createCurrentState({
+					services: [
+						await createService({ serviceName: 'one' }),
+						await createService({ serviceName: 'two' }),
+					],
+					networks: [DEFAULT_NETWORK],
+				});
+			const steps = await applicationManager.inferNextSteps(
+				currentApps,
+				targetApps,
+				{
+					downloading,
+					availableImages,
+					containerIdsByAppId,
+					locksTaken: new LocksTakenMap([
+						{ appId: 1, services: ['one', 'two'] },
+					]),
+				},
+			);
+			const [releaseLockStep] = expectSteps('releaseLock', steps, 1, 1);
+			expect(releaseLockStep).to.have.property('appId').that.equals(1);
+		});
+	});
+
 	describe("getting application's current state", () => {
 		let getImagesState: sinon.SinonStub;
 		let getServicesState: sinon.SinonStub;
@@ -2025,6 +2777,10 @@ describe('compose/application-manager', () => {
 					downloading,
 					availableImages,
 					containerIdsByAppId,
+					// Mock locks taken for all services in target state
+					locksTaken: new LocksTakenMap([
+						{ appId: 1, services: ['one', 'two', 'three', 'four'] },
+					]),
 				});
 
 			[startStep1, startStep2, startStep3, startStep4].forEach((step) => {

test/integration/device-api/actions.spec.ts

@@ -4,6 +4,7 @@ import { stub } from 'sinon';
 import Docker from 'dockerode';
 import request from 'supertest';
 import { setTimeout } from 'timers/promises';
+import { testfs } from 'mocha-pod';
 
 import * as deviceState from '~/src/device-state';
 import * as config from '~/src/config';
@@ -11,10 +12,12 @@ import * as hostConfig from '~/src/host-config';
 import * as deviceApi from '~/src/device-api';
 import * as actions from '~/src/device-api/actions';
 import * as TargetState from '~/src/device-state/target-state';
+import * as updateLock from '~/lib/update-lock';
+import { pathOnRoot } from '~/lib/host-utils';
+import { exec } from '~/lib/fs-utils';
+import * as lockfile from '~/lib/lockfile';
 import { cleanupDocker } from '~/test-lib/docker-helper';
 
-import { exec } from '~/src/lib/fs-utils';
-
 export async function dbusSend(
 	dest: string,
 	path: string,
@@ -79,6 +82,7 @@ describe('manages application lifecycle', () => {
 	const BALENA_SUPERVISOR_ADDRESS =
 		process.env.BALENA_SUPERVISOR_ADDRESS || 'http://balena-supervisor:48484';
 	const APP_ID = 1;
+	const lockdir = pathOnRoot(updateLock.BASE_LOCK_DIR);
 	const docker = new Docker();
 
 	const getSupervisorTarget = async () =>
@@ -218,6 +222,11 @@ describe('manages application lifecycle', () => {
 			ctns.every(({ State }) => !startedAt.includes(State.StartedAt));
 	};
 
+	const mockFs = testfs(
+		{ [`${lockdir}/${APP_ID}`]: {} },
+		{ cleanup: [`${lockdir}/${APP_ID}/**/*.lock`] },
+	);
+
 	before(async () => {
 		// Images are ignored in local mode so we need to pull the base image
 		await docker.pull(BASE_IMAGE);
@@ -251,10 +260,16 @@ describe('manages application lifecycle', () => {
 		});
 
 		beforeEach(async () => {
+			await mockFs.enable();
+
 			// Create a single-container application in local mode
 			await setSupervisorTarget(targetState);
 		});
 
+		afterEach(async () => {
+			await mockFs.restore();
+		});
+
 		// Make sure the app is running and correct before testing more assertions
 		it('should setup a single container app (sanity check)', async () => {
 			containers = await waitForSetup(targetState);
@@ -292,6 +307,69 @@ describe('manages application lifecycle', () => {
 			);
 		});
 
+		it('should not restart an application when user locks are present', async () => {
+			containers = await waitForSetup(targetState);
+
+			// Create a lock
+			await lockfile.lock(
+				`${lockdir}/${APP_ID}/${serviceNames[0]}/updates.lock`,
+			);
+
+			await request(BALENA_SUPERVISOR_ADDRESS)
+				.post(`/v1/restart`)
+				.set('Content-Type', 'application/json')
+				.send(JSON.stringify({ appId: APP_ID }))
+				.expect(423);
+
+			// Containers should not have been restarted
+			const containersAfterRestart = await waitForSetup(targetState);
+			expect(
+				containersAfterRestart.map((ctn) => ctn.State.StartedAt),
+			).to.deep.include.members(containers.map((ctn) => ctn.State.StartedAt));
+
+			// Remove the lock
+			await lockfile.unlock(
+				`${lockdir}/${APP_ID}/${serviceNames[0]}/updates.lock`,
+			);
+		});
+
+		it('should restart an application when user locks are present if force is specified', async () => {
+			containers = await waitForSetup(targetState);
+			const isRestartSuccessful = startTimesChanged(
+				containers.map((ctn) => ctn.State.StartedAt),
+			);
+
+			// Create a lock
+			await lockfile.lock(
+				`${lockdir}/${APP_ID}/${serviceNames[0]}/updates.lock`,
+			);
+
+			await request(BALENA_SUPERVISOR_ADDRESS)
+				.post(`/v1/restart`)
+				.set('Content-Type', 'application/json')
+				.send(JSON.stringify({ appId: APP_ID, force: true }));
+
+			const restartedContainers = await waitForSetup(
+				targetState,
+				isRestartSuccessful,
+			);
+
+			// Technically the wait function above should already verify that the two
+			// containers have been restarted, but verify explcitly with an assertion
+			expect(isRestartSuccessful(restartedContainers)).to.be.true;
+
+			// Containers should have different Ids since they're recreated
+			expect(restartedContainers.map(({ Id }) => Id)).to.not.have.members(
+				containers.map((ctn) => ctn.Id),
+			);
+
+			// Wait briefly for state to settle which includes releasing locks
+			await setTimeout(1000);
+
+			// User lock should be overridden
+			expect(await updateLock.getLocksTaken()).to.deep.equal([]);
+		});
+
 		it('should restart service by removing and recreating corresponding container', async () => {
 			containers = await waitForSetup(targetState);
 			const isRestartSuccessful = startTimesChanged(
@@ -321,6 +399,73 @@ describe('manages application lifecycle', () => {
 			);
 		});
 
+		// Since restart-service follows the same code paths as start|stop-service,
+		// these lock test cases should be sufficient to cover all three service actions.
+		it('should not restart service when user locks are present', async () => {
+			containers = await waitForSetup(targetState);
+
+			// Create a lock
+			await lockfile.lock(
+				`${lockdir}/${APP_ID}/${serviceNames[0]}/updates.lock`,
+			);
+
+			await request(BALENA_SUPERVISOR_ADDRESS)
+				.post('/v2/applications/1/restart-service')
+				.set('Content-Type', 'application/json')
+				.send(JSON.stringify({ serviceName: serviceNames[0] }))
+				.expect(423);
+
+			// Containers should not have been restarted
+			const containersAfterRestart = await waitForSetup(targetState);
+			expect(
+				containersAfterRestart.map((ctn) => ctn.State.StartedAt),
+			).to.deep.include.members(containers.map((ctn) => ctn.State.StartedAt));
+
+			// Remove the lock
+			await lockfile.unlock(
+				`${lockdir}/${APP_ID}/${serviceNames[0]}/updates.lock`,
+			);
+		});
+
+		it('should restart service when user locks are present if force is specified', async () => {
+			containers = await waitForSetup(targetState);
+			const isRestartSuccessful = startTimesChanged(
+				containers
+					.filter((ctn) => ctn.Name.includes(serviceNames[0]))
+					.map((ctn) => ctn.State.StartedAt),
+			);
+
+			// Create a lock
+			await lockfile.lock(
+				`${lockdir}/${APP_ID}/${serviceNames[0]}/updates.lock`,
+			);
+
+			await request(BALENA_SUPERVISOR_ADDRESS)
+				.post('/v2/applications/1/restart-service')
+				.set('Content-Type', 'application/json')
+				.send(JSON.stringify({ serviceName: serviceNames[0], force: true }));
+
+			const restartedContainers = await waitForSetup(
+				targetState,
+				isRestartSuccessful,
+			);
+
+			// Technically the wait function above should already verify that the two
+			// containers have been restarted, but verify explcitly with an assertion
+			expect(isRestartSuccessful(restartedContainers)).to.be.true;
+
+			// Containers should have different Ids since they're recreated
+			expect(restartedContainers.map(({ Id }) => Id)).to.not.have.members(
+				containers.map((ctn) => ctn.Id),
+			);
+
+			// Wait briefly for state to settle which includes releasing locks
+			await setTimeout(1000);
+
+			// User lock should be overridden
+			expect(await updateLock.getLocksTaken()).to.deep.equal([]);
+		});
+
 		it('should stop a running service', async () => {
 			containers = await waitForSetup(targetState);
 
@@ -520,10 +665,15 @@ describe('manages application lifecycle', () => {
 		});
 
 		beforeEach(async () => {
+			await mockFs.enable();
 			// Create a multi-container application in local mode
 			await setSupervisorTarget(targetState);
 		});
 
+		afterEach(async () => {
+			await mockFs.restore();
+		});
+
 		// Make sure the app is running and correct before testing more assertions
 		it('should setup a multi-container app (sanity check)', async () => {
 			containers = await waitForSetup(targetState);
@@ -560,6 +710,69 @@ describe('manages application lifecycle', () => {
 			);
 		});
 
+		it('should not restart an application when user locks are present', async () => {
+			containers = await waitForSetup(targetState);
+
+			// Create a lock
+			await lockfile.lock(
+				`${lockdir}/${APP_ID}/${serviceNames[0]}/updates.lock`,
+			);
+
+			await request(BALENA_SUPERVISOR_ADDRESS)
+				.post(`/v1/restart`)
+				.set('Content-Type', 'application/json')
+				.send(JSON.stringify({ appId: APP_ID }))
+				.expect(423);
+
+			// Containers should not have been restarted
+			const containersAfterRestart = await waitForSetup(targetState);
+			expect(
+				containersAfterRestart.map((ctn) => ctn.State.StartedAt),
+			).to.deep.include.members(containers.map((ctn) => ctn.State.StartedAt));
+
+			// Remove the lock
+			await lockfile.unlock(
+				`${lockdir}/${APP_ID}/${serviceNames[0]}/updates.lock`,
+			);
+		});
+
+		it('should restart an application when user locks are present if force is specified', async () => {
+			containers = await waitForSetup(targetState);
+			const isRestartSuccessful = startTimesChanged(
+				containers.map((ctn) => ctn.State.StartedAt),
+			);
+
+			// Create a lock
+			await lockfile.lock(
+				`${lockdir}/${APP_ID}/${serviceNames[0]}/updates.lock`,
+			);
+
+			await request(BALENA_SUPERVISOR_ADDRESS)
+				.post(`/v1/restart`)
+				.set('Content-Type', 'application/json')
+				.send(JSON.stringify({ appId: APP_ID, force: true }));
+
+			const restartedContainers = await waitForSetup(
+				targetState,
+				isRestartSuccessful,
+			);
+
+			// Technically the wait function above should already verify that the two
+			// containers have been restarted, but verify explcitly with an assertion
+			expect(isRestartSuccessful(restartedContainers)).to.be.true;
+
+			// Containers should have different Ids since they're recreated
+			expect(restartedContainers.map(({ Id }) => Id)).to.not.have.members(
+				containers.map((ctn) => ctn.Id),
+			);
+
+			// Wait briefly for state to settle which includes releasing locks
+			await setTimeout(500);
+
+			// User lock should be overridden
+			expect(await updateLock.getLocksTaken()).to.deep.equal([]);
+		});
+
 		it('should restart service by removing and recreating corresponding container', async () => {
 			containers = await waitForSetup(targetState);
 			const serviceName = serviceNames[0];
@@ -601,6 +814,73 @@ describe('manages application lifecycle', () => {
 			expect(sharedIds.length).to.equal(1);
 		});
 
+		// Since restart-service follows the same code paths as start|stop-service,
+		// these lock test cases should be sufficient to cover all three service actions.
+		it('should not restart service when user locks are present', async () => {
+			containers = await waitForSetup(targetState);
+
+			// Create a lock
+			await lockfile.lock(
+				`${lockdir}/${APP_ID}/${serviceNames[0]}/updates.lock`,
+			);
+
+			await request(BALENA_SUPERVISOR_ADDRESS)
+				.post('/v2/applications/1/restart-service')
+				.set('Content-Type', 'application/json')
+				.send(JSON.stringify({ serviceName: serviceNames[0] }))
+				.expect(423);
+
+			// Containers should not have been restarted
+			const containersAfterRestart = await waitForSetup(targetState);
+			expect(
+				containersAfterRestart.map((ctn) => ctn.State.StartedAt),
+			).to.deep.include.members(containers.map((ctn) => ctn.State.StartedAt));
+
+			// Remove the lock
+			await lockfile.unlock(
+				`${lockdir}/${APP_ID}/${serviceNames[0]}/updates.lock`,
+			);
+		});
+
+		it('should restart service when user locks are present if force is specified', async () => {
+			containers = await waitForSetup(targetState);
+			const isRestartSuccessful = startTimesChanged(
+				containers
+					.filter((ctn) => ctn.Name.includes(serviceNames[0]))
+					.map((ctn) => ctn.State.StartedAt),
+			);
+
+			// Create a lock
+			await lockfile.lock(
+				`${lockdir}/${APP_ID}/${serviceNames[0]}/updates.lock`,
+			);
+
+			await request(BALENA_SUPERVISOR_ADDRESS)
+				.post('/v2/applications/1/restart-service')
+				.set('Content-Type', 'application/json')
+				.send(JSON.stringify({ serviceName: serviceNames[0], force: true }));
+
+			const restartedContainers = await waitForSetup(
+				targetState,
+				isRestartSuccessful,
+			);
+
+			// Technically the wait function above should already verify that the two
+			// containers have been restarted, but verify explcitly with an assertion
+			expect(isRestartSuccessful(restartedContainers)).to.be.true;
+
+			// Containers should have different Ids since they're recreated
+			expect(restartedContainers.map(({ Id }) => Id)).to.not.have.members(
+				containers.map((ctn) => ctn.Id),
+			);
+
+			// Wait briefly for state to settle which includes releasing locks
+			await setTimeout(500);
+
+			// User lock should be overridden
+			expect(await updateLock.getLocksTaken()).to.deep.equal([]);
+		});
+
 		it('should stop a running service', async () => {
 			containers = await waitForSetup(targetState);
 

test/integration/lib/lockfile.spec.ts

@@ -1,5 +1,5 @@
 import { expect } from 'chai';
-import { promises as fs, mkdirSync } from 'fs';
+import { promises as fs } from 'fs';
 import type { TestFs } from 'mocha-pod';
 import { testfs } from 'mocha-pod';
 import * as os from 'os';
@@ -148,54 +148,82 @@ describe('lib/lockfile', () => {
 		await expect(fs.access(lock)).to.be.rejected;
 	});
 
-	it('should synchronously unlock a lockfile', () => {
+	it('should get locks taken with default args', async () => {
-		const lock = path.join(lockdir, 'other.lock');
+		// Set up lock dirs
+		await fs.mkdir(`${lockdir}/1/main`, { recursive: true });
+		await fs.mkdir(`${lockdir}/2/aux`, { recursive: true });
 
-		lockfile.unlockSync(lock);
+		// Take some locks
+		const locks = [
+			`${lockdir}/updates.lock`,
+			`${lockdir}/two.lock`,
+			`${lockdir}/1/main/updates.lock`,
+			`${lockdir}/1/main/resin-updates.lock`,
+			`${lockdir}/2/aux/updates.lock`,
+			`${lockdir}/2/aux/resin-updates.lock`,
+		];
+		await Promise.all(locks.map((lock) => lockfile.lock(lock)));
 
-		// Verify lockfile does not exist
+		// Assert all locks are listed as taken
-		return expect(fs.access(lock)).to.be.rejected;
+		expect(await lockfile.getLocksTaken(lockdir)).to.have.members(
+			locks.concat([`${lockdir}/other.lock`]),
+		);
+
+		// Clean up locks
+		await fs.rm(`${lockdir}`, { recursive: true });
 	});
 
-	it('should synchronously unlock a lockfile dir', () => {
+	it('should get locks taken with a custom filter', async () => {
-		const lock = path.join(lockdir, 'update.lock');
+		// Set up lock dirs
+		await fs.mkdir(`${lockdir}/1`, { recursive: true });
+		await fs.mkdir(`${lockdir}/services/main`, { recursive: true });
+		await fs.mkdir(`${lockdir}/services/aux`, { recursive: true });
 
-		mkdirSync(lock, { recursive: true });
+		// Take some locks...
-
+		// - with a specific UID
-		lockfile.unlockSync(lock);
+		await lockfile.lock(`${lockdir}/updates.lock`, NOBODY_UID);
-
+		// - as a directory
-		// Verify lockfile does not exist
+		await fs.mkdir(`${lockdir}/1/updates.lock`);
-		return expect(fs.access(lock)).to.be.rejected;
+		// - as a directory with a specific UID
-	});
+		await fs.mkdir(`${lockdir}/1/resin-updates.lock`);
-
+		await fs.chown(`${lockdir}/1/resin-updates.lock`, NOBODY_UID, NOBODY_UID);
-	it('should try to clean up existing locks on process exit', async () => {
+		// - under a different root dir from default
-		// Create lockfiles
+		await lockfile.lock(`${lockdir}/services/main/updates.lock`);
-		const lockOne = path.join(lockdir, 'updates.lock');
+		await lockfile.lock(`${lockdir}/services/aux/resin-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-expect-error simulate process exit event
-		process.emit('exit');
-
-		// Verify lockfile removal regardless of appId / appUuid
-		await expect(fs.access(lockOne)).to.be.rejected;
-		await expect(fs.access(lockTwo)).to.be.rejected;
-	});
-
-	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;
 
+		// Assert appropriate locks are listed as taken...
+		// - with a specific UID
 		expect(
-			lockfile.getLocksTaken((filepath) => filepath.includes('lockdir')),
+			await lockfile.getLocksTaken(
-		).to.have.members([lockOne, lockTwo]);
+				lockdir,
+				(p, stats) => p.endsWith('.lock') && stats.uid === NOBODY_UID,
+			),
+		).to.have.members([
+			`${lockdir}/updates.lock`,
+			`${lockdir}/1/resin-updates.lock`,
+			`${lockdir}/other.lock`,
+		]);
+		// - as a directory
 		expect(
-			lockfile.getLocksTaken((filepath) => filepath.includes('two')),
+			await lockfile.getLocksTaken(
-		).to.have.members([lockTwo]);
+				lockdir,
-		expect(lockfile.getLocksTaken()).to.have.members([lockOne, lockTwo]);
+				(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'),
+			),
+		).to.have.members([
+			`${lockdir}/services/main/updates.lock`,
+			`${lockdir}/services/aux/resin-updates.lock`,
+		]);
+
+		// Clean up locks
+		await fs.rm(`${lockdir}`, { recursive: true });
 	});
 });

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

@@ -2,12 +2,17 @@ import { expect } from 'chai';
 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 { watch } from 'chokidar';
 
 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', () => {
@@ -66,13 +71,16 @@ describe('lib/update-lock', () => {
 		const takeLocks = () =>
 			Promise.all(
 				supportedLockfiles.map((lf) =>
-					lockfile.lock(path.join(lockdir(testAppId, testServiceName), lf)),
+					lockfile.lock(
+						path.join(lockdir(testAppId, testServiceName), lf),
+						updateLock.LOCKFILE_UID,
+					),
 				),
 			);
 
 		const releaseLocks = async () => {
 			await Promise.all(
-				lockfile.getLocksTaken().map((lock) => lockfile.unlock(lock)),
+				(await updateLock.getLocksTaken()).map((lock) => lockfile.unlock(lock)),
 			);
 
 			// Remove any other lockfiles created for the testAppId
@@ -146,8 +154,8 @@ describe('lib/update-lock', () => {
 				)
 				.catch((err) => expect(err).to.be.instanceOf(UpdatesLockedError));
 
-			// Since the lock-taking failed, there should be no locks to dispose of
+			// Since the lock-taking with `nobody` uid failed, there should be no locks to dispose of
-			expect(lockfile.getLocksTaken()).to.have.length(0);
+			expect(await updateLock.getLocksTaken()).to.have.length(0);
 
 			// Restore the locks that were taken at the beginning of the test
 			await releaseLocks();
@@ -281,4 +289,456 @@ describe('lib/update-lock', () => {
 			);
 		});
 	});
+
+	describe('getLocksTaken', () => {
+		const lockdir = pathOnRoot(updateLock.BASE_LOCK_DIR);
+		before(async () => {
+			await testfs({
+				[lockdir]: {},
+			}).enable();
+			// TODO: enable mocha-pod to work with empty directories
+			await fs.mkdir(`${lockdir}/123/main`, { recursive: true });
+			await fs.mkdir(`${lockdir}/123/aux`, { recursive: true });
+			await fs.mkdir(`${lockdir}/123/invalid`, { recursive: true });
+		});
+		after(async () => {
+			await fs.rm(`${lockdir}/123`, { recursive: true });
+			await testfs.restore();
+		});
+
+		it('resolves with all locks taken with the Supervisor lockfile UID', async () => {
+			// Set up valid lockfiles including some directories
+			await Promise.all(
+				['resin-updates.lock', 'updates.lock'].map((lf) => {
+					const p = `${lockdir}/123/main/${lf}`;
+					return fs
+						.mkdir(p)
+						.then(() =>
+							fs.chown(p, updateLock.LOCKFILE_UID, updateLock.LOCKFILE_UID),
+						);
+				}),
+			);
+			await Promise.all([
+				lockfile.lock(
+					`${lockdir}/123/aux/updates.lock`,
+					updateLock.LOCKFILE_UID,
+				),
+				lockfile.lock(
+					`${lockdir}/123/aux/resin-updates.lock`,
+					updateLock.LOCKFILE_UID,
+				),
+			]);
+
+			// Set up invalid lockfiles with root UID
+			await Promise.all(
+				['resin-updates.lock', 'updates.lock'].map((lf) =>
+					lockfile.lock(`${lockdir}/123/invalid/${lf}`),
+				),
+			);
+
+			const locksTaken = await updateLock.getLocksTaken();
+			expect(locksTaken).to.have.length(4);
+			expect(locksTaken).to.deep.include.members([
+				`${lockdir}/123/aux/resin-updates.lock`,
+				`${lockdir}/123/aux/updates.lock`,
+				`${lockdir}/123/main/resin-updates.lock`,
+				`${lockdir}/123/main/updates.lock`,
+			]);
+			expect(locksTaken).to.not.deep.include.members([
+				`${lockdir}/123/invalid/resin-updates.lock`,
+				`${lockdir}/123/invalid/updates.lock`,
+			]);
+		});
+	});
+
+	describe('getServicesLockedByAppId', () => {
+		const lockdir = pathOnRoot(updateLock.BASE_LOCK_DIR);
+		const validDirs = [
+			`${lockdir}/123/one`,
+			`${lockdir}/123/two`,
+			`${lockdir}/123/three`,
+			`${lockdir}/456/server`,
+			`${lockdir}/456/client`,
+			`${lockdir}/789/main`,
+		];
+		const validPaths = ['resin-updates.lock', 'updates.lock']
+			.map((lf) => validDirs.map((d) => path.join(d, lf)))
+			.flat();
+		const invalidPaths = [
+			// No appId
+			`${lockdir}/456/updates.lock`,
+			// No service
+			`${lockdir}/server/updates.lock`,
+			// No appId or service
+			`${lockdir}/test/updates.lock`,
+			// One of (resin-)updates.lock is missing
+			`${lockdir}/123/one/resin-updates.lock`,
+			`${lockdir}/123/two/updates.lock`,
+		];
+		let tFs: TestFs.Enabled;
+		beforeEach(async () => {
+			tFs = await testfs({
+				[lockdir]: {},
+			}).enable();
+			// TODO: mocha-pod should support empty directories
+			await Promise.all(
+				validPaths
+					.concat(invalidPaths)
+					.map((p) => fs.mkdir(path.dirname(p), { recursive: true })),
+			);
+		});
+		afterEach(async () => {
+			await Promise.all(
+				validPaths
+					.concat(invalidPaths)
+					.map((p) => fs.rm(path.dirname(p), { recursive: true })),
+			);
+			await tFs.restore();
+		});
+
+		it('should return locks taken by appId', async () => {
+			// Set up lockfiles
+			await Promise.all(
+				validPaths.map((p) => lockfile.lock(p, updateLock.LOCKFILE_UID)),
+			);
+
+			const locksTakenMap = await updateLock.getServicesLockedByAppId();
+			expect([...locksTakenMap.keys()]).to.deep.include.members([
+				123, 456, 789,
+			]);
+			// Should register as locked if only `updates.lock` is present
+			expect(locksTakenMap.getServices(123)).to.deep.include.members([
+				'one',
+				'two',
+				'three',
+			]);
+			expect(locksTakenMap.getServices(456)).to.deep.include.members([
+				'server',
+				'client',
+			]);
+			// Should register as locked if only `resin-updates.lock` is present
+			expect(locksTakenMap.getServices(789)).to.deep.include.members(['main']);
+
+			// Cleanup lockfiles
+			await Promise.all(validPaths.map((p) => lockfile.unlock(p)));
+		});
+
+		it('should ignore invalid lockfile locations', async () => {
+			// Set up lockfiles
+			await Promise.all(
+				invalidPaths.map((p) => lockfile.lock(p, updateLock.LOCKFILE_UID)),
+			);
+			// Take another lock with an invalid UID but with everything else
+			// (appId, service, both lockfiles present) correct
+			await Promise.all(
+				['resin-updates.lock', 'updates.lock'].map((lf) =>
+					lockfile.lock(path.join(`${lockdir}/789/main`, lf)),
+				),
+			);
+			expect((await updateLock.getServicesLockedByAppId()).size).to.equal(0);
+
+			// Cleanup lockfiles
+			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', () => {
+			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(path.join(lockdir, '1', 'server'));
+				await mkdirp(path.join(lockdir, '1', 'client'));
+				await mkdirp(path.join(lockdir, '2', 'main'));
+			});
+
+			afterEach(async () => {
+				await testFs.restore();
+				await fs.rm(path.join(lockdir, '1'), { recursive: true });
+				await fs.rm(path.join(lockdir, '2'), { recursive: true });
+			});
+
+			it('takes locks for a list of services for an appId', async () => {
+				// Take locks for appId 1
+				await updateLock.takeLock(1, ['server', 'client']);
+				// Locks should have been taken
+				expect(await updateLock.getLocksTaken()).to.deep.include.members(
+					serviceLockPaths[1],
+				);
+				expect(await updateLock.getLocksTaken()).to.have.length(4);
+				expect(
+					await fs.readdir(path.join(lockdir, '1', 'server')),
+				).to.include.members(['updates.lock', 'resin-updates.lock']);
+				expect(
+					await fs.readdir(path.join(lockdir, '1', 'client')),
+				).to.include.members(['updates.lock', 'resin-updates.lock']);
+				// Take locks for appId 2
+				await updateLock.takeLock(2, ['main']);
+				// Locks should have been taken for appid 1 & 2
+				expect(await updateLock.getLocksTaken()).to.deep.include.members([
+					...serviceLockPaths[1],
+					...serviceLockPaths[2],
+				]);
+				expect(await updateLock.getLocksTaken()).to.have.length(6);
+				expect(
+					await fs.readdir(path.join(lockdir, '2', 'main')),
+				).to.have.length(2);
+				// Clean up the lockfiles
+				for (const lockPath of serviceLockPaths[1].concat(
+					serviceLockPaths[2],
+				)) {
+					await lockfile.unlock(lockPath);
+				}
+			});
+
+			it('creates lock directory recursively if it does not exist', async () => {
+				// Take locks for app with nonexistent service directories
+				await updateLock.takeLock(3, ['api']);
+				// Locks should have been taken
+				expect(await updateLock.getLocksTaken()).to.deep.include(
+					path.join(lockdir, '3', 'api', 'updates.lock'),
+					path.join(lockdir, '3', 'api', 'resin-updates.lock'),
+				);
+				// Directories should have been created
+				expect(await fs.readdir(path.join(lockdir))).to.deep.include.members([
+					'3',
+				]);
+				expect(
+					await fs.readdir(path.join(lockdir, '3')),
+				).to.deep.include.members(['api']);
+				// Clean up the lockfiles & created directories
+				await lockfile.unlock(path.join(lockdir, '3', 'api', 'updates.lock'));
+				await lockfile.unlock(
+					path.join(lockdir, '3', 'api', 'resin-updates.lock'),
+				);
+				await fs.rm(path.join(lockdir, '3'), { recursive: true });
+			});
+
+			it('should not take lock for services where Supervisor-taken lock already exists', async () => {
+				// Take locks for one service of appId 1
+				await lockfile.lock(serviceLockPaths[1][0], updateLock.LOCKFILE_UID);
+				await lockfile.lock(serviceLockPaths[1][1], updateLock.LOCKFILE_UID);
+				// Sanity check that locks are taken & tracked by Supervisor
+				expect(await updateLock.getLocksTaken()).to.deep.include(
+					serviceLockPaths[1][0],
+					serviceLockPaths[1][1],
+				);
+				expect(await updateLock.getLocksTaken()).to.have.length(2);
+				// Take locks using takeLock, should only lock service which doesn't
+				// already have locks
+				await expect(
+					updateLock.takeLock(1, ['server', 'client']),
+				).to.eventually.deep.include.members(['client']);
+				// Check that locks are taken
+				expect(await updateLock.getLocksTaken()).to.deep.include.members(
+					serviceLockPaths[1],
+				);
+				// Clean up lockfiles
+				for (const lockPath of serviceLockPaths[1]) {
+					await lockfile.unlock(lockPath);
+				}
+			});
+
+			it('should error if service has a non-Supervisor-taken lock', async () => {
+				// Simulate a user service taking the lock for services with appId 1
+				for (const lockPath of serviceLockPaths[1]) {
+					await fs.writeFile(lockPath, '');
+				}
+				// Take locks using takeLock, should error
+				await expect(
+					updateLock.takeLock(1, ['server', 'client']),
+				).to.eventually.be.rejectedWith(UpdatesLockedError);
+				// No Supervisor locks should have been taken
+				expect(await updateLock.getLocksTaken()).to.have.length(0);
+				// Clean up user-created lockfiles
+				for (const lockPath of serviceLockPaths[1]) {
+					await fs.rm(lockPath);
+				}
+				// Take locks using takeLock, should not error
+				await expect(
+					updateLock.takeLock(1, ['server', 'client']),
+				).to.eventually.not.be.rejectedWith(UpdatesLockedError);
+				// Check that locks are taken
+				expect(await updateLock.getLocksTaken()).to.deep.include.members(
+					serviceLockPaths[1],
+				);
+				expect(await updateLock.getLocksTaken()).to.have.length(4);
+				// Clean up lockfiles
+				for (const lockPath of serviceLockPaths[1]) {
+					await lockfile.unlock(lockPath);
+				}
+			});
+
+			it('waits to take locks until resource write lock is taken', async () => {
+				// Take the write lock for appId 1
+				const release = await takeGlobalLockRW(1);
+				// Queue takeLock, won't resolve until the write lock is released
+				const takeLockPromise = updateLock.takeLock(1, ['server', 'client']);
+				// Locks should have not been taken even after waiting
+				await setTimeout(500);
+				expect(await updateLock.getLocksTaken()).to.have.length(0);
+				// Release the write lock
+				release();
+				// Locks should be taken
+				await takeLockPromise;
+				// Locks should have been taken
+				expect(await updateLock.getLocksTaken()).to.deep.include.members(
+					serviceLockPaths[1],
+				);
+			});
+
+			it('should release locks when takeLock step errors to return services to unlocked state', async () => {
+				const svcs = ['server', 'client'];
+
+				// 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`,
+				]);
+
+				// Watch for added files, as Supervisor-taken locks should be added
+				// then removed within updateLock.takeLock
+				const addedFiles: string[] = [];
+				const watcher = watch(lockdir).on('add', (p) => addedFiles.push(p));
+
+				// updateLock.takeLock should error
+				await expect(updateLock.takeLock(1, svcs, false)).to.be.rejectedWith(
+					UpdatesLockedError,
+				);
+
+				// Service without user lock should have been locked by Supervisor..
+				expect(addedFiles).to.deep.include.members([
+					`${lockdir}/1/${svcs[0]}/updates.lock`,
+					`${lockdir}/1/${svcs[0]}/resin-updates.lock`,
+				]);
+
+				// ..but upon error, Supervisor-taken locks should have been cleaned up
+				expect(
+					await lockfile.getLocksTaken(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`,
+				]);
+
+				// Clean up watcher
+				await watcher.close();
+			});
+		});
+
+		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, updateLock.LOCKFILE_UID);
+				}
+				// Sanity check that locks are taken & tracked by Supervisor
+				expect(await updateLock.getLocksTaken()).to.deep.include.members(
+					serviceLockPaths[1],
+				);
+				// Release locks for appId 1
+				await updateLock.releaseLock(1);
+				// Locks should have been released
+				expect(await updateLock.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(await updateLock.getLocksTaken()).to.have.length(0);
+				// Should not error
+				await updateLock.releaseLock(1);
+				expect(await updateLock.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, updateLock.LOCKFILE_UID);
+				// Sanity check that locks are taken & tracked by Supervisor
+				expect(await updateLock.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(await updateLock.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, updateLock.LOCKFILE_UID);
+				}
+				// Sanity check that locks are taken & tracked by Supervisor
+				expect(await updateLock.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(await updateLock.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(await updateLock.getLocksTaken()).to.have.length(0);
+			});
+		});
+	});
 });

test/lib/mocked-dockerode.ts

@@ -74,7 +74,7 @@ export function registerOverride<
 }
 
 export function restoreOverride<T extends DockerodeFunction>(name: T) {
-	if (Object.prototype.hasOwnProperty.call(overrides, name)) {
+	if (Object.hasOwn(overrides, name)) {
 		delete overrides[name];
 	}
 }

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,
@@ -14,9 +15,11 @@ import {
 
 const defaultContext = {
 	keepVolumes: false,
+	force: false,
 	availableImages: [] as Image[],
 	containerIds: {},
 	downloading: [] as string[],
+	locksTaken: new LocksTakenMap(),
 };
 
 describe('compose/app', () => {
@@ -149,6 +152,7 @@ describe('compose/app', () => {
 					}),
 				],
 				volumes: [Volume.fromComposeObject('test-volume', 1, 'deadbeef')],
+				networks: [DEFAULT_NETWORK],
 			});
 			const target = createApp({
 				services: [
@@ -162,16 +166,42 @@ describe('compose/app', () => {
 						labels: { test: 'test' },
 					}),
 				],
+				networks: [DEFAULT_NETWORK],
 				isTarget: true,
 			});
 
-			// Calculate steps
+			const availableImages = [createImage({ serviceName: 'test' })];
-			const steps = current.nextStepsForAppUpdate(defaultContext, target);
+			// Take lock first
+			const steps = current.nextStepsForAppUpdate(
+				{
+					...defaultContext,
+					availableImages,
+				},
+				target,
+			);
+			const [lockStep] = expectSteps('takeLock', steps);
+			expect(lockStep)
+				.to.have.property('services')
+				.that.deep.includes.members(['test']);
 
-			const [killStep] = expectSteps('kill', steps);
+			// Then kill
+			const steps2 = current.nextStepsForAppUpdate(
+				{
+					...defaultContext,
+					availableImages,
+					// Mock lock already taken
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['test'] }]),
+				},
+				target,
+			);
+			const [killStep] = expectSteps('kill', steps2);
 			expect(killStep)
 				.to.have.property('current')
 				.that.deep.includes({ serviceName: 'test' });
+
+			// No remove volume steps until dependencies are killed
+			expectNoStep('removeVolume', steps);
+			expectNoStep('removeVolume', steps2);
 		});
 
 		it('should correctly infer to remove an app volumes when the app is being removed', () => {
@@ -245,17 +275,32 @@ describe('compose/app', () => {
 				volumes: [volume],
 			});
 
-			// Step 1: kill
+			// Step 1: takeLock
-			const steps = current.nextStepsForAppUpdate(
+			const lockStep = current.nextStepsForAppUpdate(
 				contextWithImages,
 				intermediateTarget,
 			);
-			expectSteps('kill', steps);
+			expectSteps('takeLock', lockStep, 1, 1);
 
-			// Step 2: noop (service is stopping)
+			// Step 2: kill
+			const killSteps = current.nextStepsForAppUpdate(
+				{
+					...contextWithImages,
+					// Mock locks already taken
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
+				},
+				intermediateTarget,
+			);
+			expectSteps('kill', killSteps);
+
+			// Step 3: noop (service is stopping)
 			service.status = 'Stopping';
 			const secondStageSteps = current.nextStepsForAppUpdate(
-				contextWithImages,
+				{
+					...contextWithImages,
+					// Mock locks already taken
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
+				},
 				intermediateTarget,
 			);
 			expectSteps('noop', secondStageSteps);
@@ -281,7 +326,7 @@ describe('compose/app', () => {
 				volumes: [],
 			});
 
-			// Step 3: createVolume
+			// Step 4: createVolume
 			service.status = 'Running';
 			const target = createApp({
 				services: [service],
@@ -298,18 +343,29 @@ describe('compose/app', () => {
 			expect(recreateVolumeSteps).to.have.length(1);
 			expectSteps('createVolume', recreateVolumeSteps);
 
-			// Final step: start service
+			// Step 5: takeLock
 			const currentWithVolumeRecreated = createApp({
 				services: [],
 				networks: [DEFAULT_NETWORK],
 				volumes: [volume],
 			});
-
+			const lockStepAfterRecreate =
-			const createServiceSteps =
 				currentWithVolumeRecreated.nextStepsForAppUpdate(
 					contextWithImages,
 					target,
 				);
+			expectSteps('takeLock', lockStepAfterRecreate, 1, 1);
+
+			// Final step: start service
+			const createServiceSteps =
+				currentWithVolumeRecreated.nextStepsForAppUpdate(
+					{
+						...contextWithImages,
+						// Mock locks already taken
+						locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
+					},
+					target,
+				);
 			expectSteps('start', createServiceSteps);
 		});
 	});
@@ -436,6 +492,7 @@ describe('compose/app', () => {
 			});
 			const target = createApp({
 				networks: [
+					DEFAULT_NETWORK,
 					Network.fromComposeObject('test-network', 1, 'deadbeef', {}),
 				],
 				services: [
@@ -449,13 +506,61 @@ describe('compose/app', () => {
 				isTarget: true,
 			});
 
-			const steps = current.nextStepsForAppUpdate(defaultContext, target);
+			const availableImages = [createImage({ appUuid: 'deadbeef' })];
+			// Take lock first
+			const steps = current.nextStepsForAppUpdate(
+				{
+					...defaultContext,
+					availableImages,
+				},
+				target,
+			);
+			const [lockStep] = expectSteps('takeLock', steps);
+			expect(lockStep)
+				.to.have.property('services')
+				.that.deep.includes.members(['test']);
 
-			const [removeNetworkStep] = expectSteps('kill', steps);
+			// Then kill
-
+			const steps2 = current.nextStepsForAppUpdate(
-			expect(removeNetworkStep).to.have.property('current').that.deep.includes({
+				{
+					...defaultContext,
+					availableImages,
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['test'] }]),
+				},
+				target,
+			);
+			const [killStep] = expectSteps('kill', steps2);
+			expect(killStep).to.have.property('current').that.deep.includes({
 				serviceName: 'test',
 			});
+
+			// removeNetwork should not be generated until after the kill
+			expectNoStep('removeNetwork', steps);
+			expectNoStep('removeNetwork', steps2);
+
+			// Then remove duplicate networks
+			const current2 = createApp({
+				appUuid: 'deadbeef',
+				networks: [
+					DEFAULT_NETWORK,
+					Network.fromComposeObject('test-network', 1, 'deadbeef', {}),
+					Network.fromComposeObject('test-network', 1, 'deadbeef', {}),
+				],
+				services: [],
+			});
+
+			const steps3 = current2.nextStepsForAppUpdate(
+				{
+					...defaultContext,
+					availableImages,
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['test'] }]),
+				},
+				target,
+			);
+			const [removeNetworkStep] = expectSteps('removeNetwork', steps3);
+			expect(removeNetworkStep).to.have.property('current').that.deep.includes({
+				name: 'test-network',
+			});
 		});
 
 		it('should correctly infer more than one network removal step', () => {
@@ -558,15 +663,33 @@ describe('compose/app', () => {
 			});
 
 			const availableImages = [createImage({ appUuid: 'deadbeef' })];
-
+			// Take lock first
 			const steps = current.nextStepsForAppUpdate(
 				{ ...defaultContext, availableImages },
 				target,
 			);
-			const [killStep] = expectSteps('kill', steps);
+			const [lockStep] = expectSteps('takeLock', steps);
+			expect(lockStep)
+				.to.have.property('services')
+				.that.deep.includes.members(['test']);
+
+			// Then kill
+			const steps2 = current.nextStepsForAppUpdate(
+				{
+					...defaultContext,
+					availableImages,
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['test'] }]),
+				},
+				target,
+			);
+			const [killStep] = expectSteps('kill', steps2);
 			expect(killStep)
 				.to.have.property('current')
 				.that.deep.includes({ serviceName: 'test' });
+
+			// Network should not be removed until after dependency kills
+			expectNoStep('removeNetwork', steps);
+			expectNoStep('removeNetwork', steps2);
 		});
 
 		it('should kill dependencies of networks before changing config', async () => {
@@ -574,7 +697,7 @@ describe('compose/app', () => {
 				services: [
 					await createService({
 						serviceName: 'test',
-						composition: { networks: ['test-network'] },
+						composition: { networks: { 'test-network': {} } },
 					}),
 				],
 				networks: [Network.fromComposeObject('test-network', 1, 'appuuid', {})],
@@ -593,16 +716,37 @@ describe('compose/app', () => {
 				],
 				isTarget: true,
 			});
+			const availableImages = [createImage({ appId: 1, serviceName: 'test' })];
+			// Take lock first
+			const steps = current.nextStepsForAppUpdate(
+				{
+					...defaultContext,
+					availableImages,
+				},
+				target,
+			);
+			const [lockStep] = expectSteps('takeLock', steps);
+			expect(lockStep)
+				.to.have.property('services')
+				.that.deep.includes.members(['test']);
 
-			const steps = current.nextStepsForAppUpdate(defaultContext, target);
+			// Then kill
-			const [killStep] = expectSteps('kill', steps);
+			const steps2 = current.nextStepsForAppUpdate(
-
+				{
+					...defaultContext,
+					availableImages,
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['test'] }]),
+				},
+				target,
+			);
+			const [killStep] = expectSteps('kill', steps2);
 			expect(killStep)
 				.to.have.property('current')
 				.that.deep.includes({ serviceName: 'test' });
 
-			// We shouldn't try to remove the network until we have gotten rid of the dependencies
+			// Network should not be removed until after dependency kills
 			expectNoStep('removeNetwork', steps);
+			expectNoStep('removeNetwork', steps2);
 		});
 
 		it('should always kill dependencies of networks before removing', async () => {
@@ -649,19 +793,35 @@ describe('compose/app', () => {
 				createImage({ appId: 1, serviceName: 'one', name: 'alpine' }),
 				createImage({ appId: 1, serviceName: 'two', name: 'alpine' }),
 			];
-
+			// Take lock first
 			const steps = current.nextStepsForAppUpdate(
 				{ ...defaultContext, availableImages },
 				target,
 			);
-			const [killStep] = expectSteps('kill', steps);
+			const [lockStep] = expectSteps('takeLock', steps);
+			expect(lockStep)
+				.to.have.property('services')
+				.that.deep.includes.members(['one']);
 
+			// Then kill
+			const steps2 = current.nextStepsForAppUpdate(
+				{
+					...defaultContext,
+					availableImages,
+					locksTaken: new LocksTakenMap([
+						{ appId: 1, services: ['one', 'two'] },
+					]),
+				},
+				target,
+			);
+			const [killStep] = expectSteps('kill', steps2);
 			expect(killStep)
 				.to.have.property('current')
 				.that.deep.includes({ serviceName: 'one' });
 
 			// We shouldn't try to remove the network until we have gotten rid of the dependencies
 			expectNoStep('removeNetwork', steps);
+			expectNoStep('removeNetwork', steps2);
 		});
 
 		it('should kill dependencies of networks before updating between releases', async () => {
@@ -712,20 +872,35 @@ describe('compose/app', () => {
 				createImage({ appId: 1, serviceName: 'one', name: 'alpine' }),
 				createImage({ appId: 1, serviceName: 'two', name: 'alpine' }),
 			];
-
+			// Take lock first
 			const steps = current.nextStepsForAppUpdate(
 				{ ...defaultContext, availableImages },
 				target,
 			);
-			expectSteps('kill', steps, 2);
+			const [lockStep] = expectSteps('takeLock', steps);
+			expect(lockStep)
+				.to.have.property('services')
+				.that.deep.includes.members(['one', 'two']);
 
-			expect(steps.map((s) => (s as any).current.serviceName)).to.have.members([
+			// Then kill
-				'one',
+			const steps2 = current.nextStepsForAppUpdate(
-				'two',
+				{
-			]);
+					...defaultContext,
+					availableImages,
+					locksTaken: new LocksTakenMap([
+						{ appId: 1, services: ['one', 'two'] },
+					]),
+				},
+				target,
+			);
+			expectSteps('kill', steps2, 2);
+			expect(steps2.map((s) => (s as any).current.serviceName)).to.have.members(
+				['one', 'two'],
+			);
 
 			// We shouldn't try to remove the network until we have gotten rid of the dependencies
 			expectNoStep('removeNetwork', steps);
+			expectNoStep('removeNetwork', steps2);
 		});
 
 		it('should create the default network if it does not exist', () => {
@@ -841,6 +1016,7 @@ describe('compose/app', () => {
 				isTarget: true,
 			});
 
+			// Take lock first
 			const steps = current.nextStepsForAppUpdate(
 				{
 					...defaultContext,
@@ -850,7 +1026,24 @@ describe('compose/app', () => {
 				},
 				target,
 			);
-			const [killStep] = expectSteps('kill', steps);
+			const [lockStep] = expectSteps('takeLock', steps);
+			expect(lockStep)
+				.to.have.property('services')
+				.that.deep.includes.members(['main', 'aux']);
+
+			// Then kill
+			const steps2 = current.nextStepsForAppUpdate(
+				{
+					...defaultContext,
+					availableImages: [createImage({ serviceName: 'main' })],
+					// Mock locks already taken
+					locksTaken: new LocksTakenMap([
+						{ appId: 1, services: ['main', 'aux'] },
+					]),
+				},
+				target,
+			);
+			const [killStep] = expectSteps('kill', steps2);
 			expect(killStep)
 				.to.have.property('current')
 				.to.deep.include({ serviceName: 'aux' });
@@ -949,26 +1142,48 @@ describe('compose/app', () => {
 			expectNoStep('fetch', steps);
 		});
 
-		it('should emit an updateMetadata step when a service has not changed but the release has', async () => {
+		it('should emit a takeLock followed by an updateMetadata step when a service has not changed but the release has', async () => {
 			const current = createApp({
 				services: [
-					await createService({ serviceName: 'main', commit: 'old-release' }),
+					await createService({
+						serviceName: 'main',
+						appId: 1,
+						commit: 'old-release',
+					}),
 				],
+				networks: [DEFAULT_NETWORK],
 			});
 			const target = createApp({
 				services: [
-					await createService({ serviceName: 'main', commit: 'new-release' }),
+					await createService({
+						serviceName: 'main',
+						appId: 1,
+						commit: 'new-release',
+					}),
 				],
+				networks: [DEFAULT_NETWORK],
 				isTarget: true,
 			});
 
+			// Take lock before updating metadata
 			const steps = current.nextStepsForAppUpdate(defaultContext, target);
-			const [updateMetadataStep] = expectSteps('updateMetadata', steps);
+			const [takeLockStep] = expectSteps('takeLock', steps);
+			expect(takeLockStep)
+				.to.have.property('services')
+				.that.deep.equals(['main']);
 
+			// Infer updateMetadata after locks are taken
+			const steps2 = current.nextStepsForAppUpdate(
+				{
+					...defaultContext,
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
+				},
+				target,
+			);
+			const [updateMetadataStep] = expectSteps('updateMetadata', steps2);
 			expect(updateMetadataStep)
 				.to.have.property('current')
 				.to.deep.include({ serviceName: 'main', commit: 'old-release' });
-
 			expect(updateMetadataStep)
 				.to.have.property('target')
 				.to.deep.include({ serviceName: 'main', commit: 'new-release' });
@@ -985,8 +1200,22 @@ describe('compose/app', () => {
 				isTarget: true,
 			});
 
+			// Take lock first
 			const steps = current.nextStepsForAppUpdate(defaultContext, target);
-			const [stopStep] = expectSteps('stop', steps);
+			const [lockStep] = expectSteps('takeLock', steps);
+			expect(lockStep)
+				.to.have.property('services')
+				.that.deep.includes.members(['main']);
+
+			// Then stop
+			const steps2 = current.nextStepsForAppUpdate(
+				{
+					...defaultContext,
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
+				},
+				target,
+			);
+			const [stopStep] = expectSteps('stop', steps2);
 			expect(stopStep)
 				.to.have.property('current')
 				.to.deep.include({ serviceName: 'main' });
@@ -1045,9 +1274,19 @@ describe('compose/app', () => {
 				isTarget: true,
 			});
 
-			// should see a 'stop'
+			// Take lock first
+			const steps = current.nextStepsForAppUpdate(contextWithImages, target);
+			const [lockStep] = expectSteps('takeLock', steps);
+			expect(lockStep)
+				.to.have.property('services')
+				.that.deep.includes.members(['main']);
+
+			// Then kill
 			const stepsToIntermediate = current.nextStepsForAppUpdate(
-				contextWithImages,
+				{
+					...contextWithImages,
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
+				},
 				target,
 			);
 			const [killStep] = expectSteps('kill', stepsToIntermediate);
@@ -1055,19 +1294,21 @@ describe('compose/app', () => {
 				.to.have.property('current')
 				.that.deep.includes({ serviceName: 'main' });
 
-			// assume the intermediate step has already removed the app
+			// Assume the intermediate step has already removed the app
 			const intermediate = createApp({
 				services: [],
 				// Default network was already created
 				networks: [DEFAULT_NETWORK],
 			});
 
-			// now should see a 'start'
+			// Then start
 			const stepsToTarget = intermediate.nextStepsForAppUpdate(
-				contextWithImages,
+				{
+					...contextWithImages,
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
+				},
 				target,
 			);
-
 			const [startStep] = expectSteps('start', stepsToTarget);
 			expect(startStep)
 				.to.have.property('target')
@@ -1079,12 +1320,6 @@ describe('compose/app', () => {
 		});
 
 		it('should not start a container when it depends on a service which is being installed', async () => {
-			const availableImages = [
-				createImage({ appId: 1, serviceName: 'main', name: 'main-image' }),
-				createImage({ appId: 1, serviceName: 'dep', name: 'dep-image' }),
-			];
-			const contextWithImages = { ...defaultContext, ...{ availableImages } };
-
 			const current = createApp({
 				services: [
 					await createService(
@@ -1121,12 +1356,24 @@ describe('compose/app', () => {
 				isTarget: true,
 			});
 
+			const availableImages = [
+				createImage({ appId: 1, serviceName: 'main', name: 'main-image' }),
+				createImage({ appId: 1, serviceName: 'dep', name: 'dep-image' }),
+			];
+			// As service is already being installed, lock for target should have been taken
+			const contextWithImages = {
+				...defaultContext,
+				...{ availableImages },
+				locksTaken: new LocksTakenMap([
+					{ appId: 1, services: ['main', 'dep'] },
+				]),
+			};
+
+			// Only one start step and it should be that of the 'dep' service
 			const stepsToIntermediate = current.nextStepsForAppUpdate(
 				contextWithImages,
 				target,
 			);
-
-			// Only one start step and it should be that of the 'dep' service
 			const [startStep] = expectSteps('start', stepsToIntermediate);
 			expect(startStep)
 				.to.have.property('target')
@@ -1148,7 +1395,6 @@ describe('compose/app', () => {
 				{ ...contextWithImages, ...{ containerIds: { dep: 'dep-id' } } },
 				target,
 			);
-
 			const [startMainStep] = expectSteps('start', stepsToTarget);
 			expect(startMainStep)
 				.to.have.property('target')
@@ -1220,11 +1466,28 @@ describe('compose/app', () => {
 				isTarget: true,
 			});
 
-			const stepsToIntermediate = current.nextStepsForAppUpdate(
+			// Take lock first
+			const stepsToIntermediateBeforeLock = current.nextStepsForAppUpdate(
 				contextWithImages,
 				target,
 			);
+			const [takeLockStep] = expectSteps(
+				'takeLock',
+				stepsToIntermediateBeforeLock,
+			);
+			expect(takeLockStep)
+				.to.have.property('services')
+				.that.deep.includes.members(['main']);
 
+			// Then kill
+			const stepsToIntermediate = current.nextStepsForAppUpdate(
+				{
+					...contextWithImages,
+					// Mock locks taken before kill
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
+				},
+				target,
+			);
 			const [killStep] = expectSteps('kill', stepsToIntermediate);
 			expect(killStep)
 				.to.have.property('current')
@@ -1237,7 +1500,12 @@ describe('compose/app', () => {
 			});
 
 			const stepsToTarget = intermediate.nextStepsForAppUpdate(
-				contextWithImages,
+				{
+					...contextWithImages,
+					// Mock locks still taken after kill (releaseLock not
+					// yet inferred as state is not yet settled)
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
+				},
 				target,
 			);
 
@@ -1349,23 +1617,42 @@ describe('compose/app', () => {
 				isTarget: true,
 			});
 
-			const stepsFirstTry = current.nextStepsForAppUpdate(
+			// Take lock first
+			const stepsBeforeLock = current.nextStepsForAppUpdate(
 				contextWithImages,
 				target,
 			);
+			const [takeLockStep] = expectSteps('takeLock', stepsBeforeLock);
+			expect(takeLockStep)
+				.to.have.property('services')
+				.that.deep.includes.members(['main']);
+
+			// Then kill
+			const stepsFirstTry = current.nextStepsForAppUpdate(
+				{
+					...contextWithImages,
+					// Mock locks taken from previous step
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
+				},
+				target,
+			);
 
 			const [killStep] = expectSteps('kill', stepsFirstTry);
 			expect(killStep)
 				.to.have.property('current')
 				.that.deep.includes({ serviceName: 'main' });
 
-			// if at first you don't succeed
+			// As long as a kill step has not succeeded (current state hasn't
+			// changed), a kill step should be generated.
 			const stepsSecondTry = current.nextStepsForAppUpdate(
-				contextWithImages,
+				{
+					...contextWithImages,
+					// Mock locks taken from previous step
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
+				},
 				target,
 			);
 
-			// Since current state has not changed, another kill step needs to be generated
 			const [newKillStep] = expectSteps('kill', stepsSecondTry);
 			expect(newKillStep)
 				.to.have.property('current')
@@ -1393,8 +1680,22 @@ describe('compose/app', () => {
 				isTarget: true,
 			});
 
+			// Take lock first
 			const steps = current.nextStepsForAppUpdate(defaultContext, target);
-			const [killStep] = expectSteps('kill', steps);
+			const [lockStep] = expectSteps('takeLock', steps);
+			expect(lockStep)
+				.to.have.property('services')
+				.that.deep.includes.members(['test']);
+
+			// Then kill
+			const steps2 = current.nextStepsForAppUpdate(
+				{
+					...defaultContext,
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['test'] }]),
+				},
+				target,
+			);
+			const [killStep] = expectSteps('kill', steps2);
 			expect(killStep)
 				.to.have.property('current')
 				.that.deep.includes({ serviceName: 'test' });
@@ -1445,6 +1746,7 @@ describe('compose/app', () => {
 						commit: 'old-release',
 					}),
 				],
+				networks: [DEFAULT_NETWORK],
 			});
 			const target = createApp({
 				services: [
@@ -1454,19 +1756,35 @@ describe('compose/app', () => {
 						commit: 'new-release',
 					}),
 				],
+				networks: [DEFAULT_NETWORK],
 				isTarget: true,
 			});
 
-			const steps = current.nextStepsForAppUpdate(
+			const contextWithImages = {
-				{
 				...defaultContext,
 				// With default download-then-kill strategy, target images
 				// should all be available before a kill step is inferred
 				availableImages: [createImage({ serviceName: 'three' })],
+			};
+			// Take lock first
+			const steps = current.nextStepsForAppUpdate(contextWithImages, target);
+			const [lockStep] = expectSteps('takeLock', steps);
+			expect(lockStep)
+				.to.have.property('services')
+				.that.deep.includes.members(['one', 'two', 'three']);
+
+			// Then kill
+			const steps2 = current.nextStepsForAppUpdate(
+				{
+					...contextWithImages,
+					// Mock locks already taken
+					locksTaken: new LocksTakenMap([
+						{ appId: 1, services: ['one', 'two', 'three'] },
+					]),
 				},
 				target,
 			);
-			expectSteps('kill', steps, 2);
+			expectSteps('kill', steps2, 2);
 		});
 
 		it('should not infer a kill step with the default strategy before all target images have been downloaded', async () => {
@@ -1571,21 +1889,6 @@ describe('compose/app', () => {
 		});
 
 		it('should infer a start step only when target images have been downloaded', async () => {
-			const contextWithImages = {
-				...defaultContext,
-				...{
-					downloading: [], // One of the images is being downloaded
-					availableImages: [
-						createImage({ appId: 1, name: 'main-image', serviceName: 'main' }),
-						createImage({
-							appId: 1,
-							name: 'other-image',
-							serviceName: 'other',
-						}),
-					],
-				},
-			};
-
 			const current = createApp({
 				services: [],
 				networks: [DEFAULT_NETWORK],
@@ -1609,9 +1912,59 @@ describe('compose/app', () => {
 				isTarget: true,
 			});
 
-			// No kill steps should be generated
+			// No start steps should be generated as long as any target image is downloading
-			const steps = current.nextStepsForAppUpdate(contextWithImages, target);
+			const steps = current.nextStepsForAppUpdate(
-			expectSteps('start', steps, 2);
+				{
+					...defaultContext,
+					downloading: ['other-image'],
+					availableImages: [
+						createImage({ appId: 1, name: 'main-image', serviceName: 'main' }),
+					],
+				},
+				target,
+			);
+			expectNoStep('start', steps);
+			expectSteps('noop', steps, 1);
+
+			// Take lock before starting once downloads complete
+			const steps2 = current.nextStepsForAppUpdate(
+				{
+					...defaultContext,
+					availableImages: [
+						createImage({ appId: 1, name: 'main-image', serviceName: 'main' }),
+						createImage({
+							appId: 1,
+							name: 'other-image',
+							serviceName: 'other',
+						}),
+					],
+				},
+				target,
+			);
+			const [lockStep] = expectSteps('takeLock', steps2);
+			expect(lockStep)
+				.to.have.property('services')
+				.that.deep.includes.members(['main', 'other']);
+
+			// Then start
+			const steps3 = current.nextStepsForAppUpdate(
+				{
+					...defaultContext,
+					availableImages: [
+						createImage({ appId: 1, name: 'main-image', serviceName: 'main' }),
+						createImage({
+							appId: 1,
+							name: 'other-image',
+							serviceName: 'other',
+						}),
+					],
+					locksTaken: new LocksTakenMap([
+						{ appId: 1, services: ['main', 'other'] },
+					]),
+				},
+				target,
+			);
+			expectSteps('start', steps3, 2);
 		});
 	});
 
@@ -1671,4 +2024,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);
+		});
+	});
 });

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

@@ -14,4 +14,45 @@ describe('lib/update-lock: unit tests', () => {
 			);
 		});
 	});
+
+	describe('LocksTakenMap', () => {
+		it('should be an instance of Map<number, Set<string>>', () => {
+			const map = new updateLock.LocksTakenMap();
+			expect(map).to.be.an.instanceof(Map);
+		});
+
+		it('should add services while ignoring duplicates', () => {
+			const map = new updateLock.LocksTakenMap();
+			map.add(123, 'main');
+			expect(map.getServices(123)).to.deep.include.members(['main']);
+
+			map.add(123, 'main');
+			expect(map.getServices(123)).to.deep.include.members(['main']);
+
+			map.add(123, ['main', 'aux']);
+			expect(map.getServices(123)).to.deep.include.members(['main', 'aux']);
+		});
+
+		it('should track any number of appIds', () => {
+			const map = new updateLock.LocksTakenMap();
+			map.add(123, 'main');
+			map.add(456, ['aux', 'dep']);
+			expect(map.getServices(123)).to.deep.include.members(['main']);
+			expect(map.getServices(456)).to.deep.include.members(['aux', 'dep']);
+			expect(map.size).to.equal(2);
+		});
+
+		it('should return empty array for non-existent appIds', () => {
+			const map = new updateLock.LocksTakenMap();
+			expect(map.getServices(123)).to.deep.equal([]);
+		});
+
+		it('should return whether a service is locked under an appId', () => {
+			const map = new updateLock.LocksTakenMap();
+			map.add(123, 'main');
+			expect(map.isLocked(123, 'main')).to.be.true;
+			expect(map.isLocked(123, 'aux')).to.be.false;
+			expect(map.isLocked(456, 'main')).to.be.false;
+		});
+	});
 });