Prevent unintended image removal when calling purge endpoints to remove volumes

Using safeStateClone within doPurge to applyIntermediateTarget after
successful volume purge has led to various type deficiencies being revealed
in common.js. Add several inline types in common.js to satisfy
the type checker (credit: Page <page@balena.io>). Delete common.d.ts
since it's not required and might mistakenly mask true I/O types of
functions in common.js.

Closes: #1611
Change-type: patch
Signed-off-by: Christina Wang <christina@balena.io>

src/compose/application-manager.ts

@@ -78,6 +78,13 @@ export let containerStarted: { [containerId: string]: boolean } = {};
 export let fetchesInProgress = 0;
 export let timeSpentFetching = 0;
 
+// In the case of intermediate target apply, toggle to true to avoid unintended image deletion
+let isApplyingIntermediate = false;
+
+export function setIsApplyingIntermediate(value: boolean = false) {
+	isApplyingIntermediate = value;
+}
+
 export function resetTimeSpentFetching(value: number = 0) {
 	timeSpentFetching = value;
 }
@@ -206,12 +213,15 @@ export async function getRequiredSteps(
 			steps.push({ action: 'ensureSupervisorNetwork' });
 		}
 	} else {
-		if (!localMode && downloading.length === 0) {
+		if (!localMode && downloading.length === 0 && !isApplyingIntermediate) {
+			// Avoid cleaning up dangling images while purging
 			if (cleanupNeeded) {
 				steps.push({ action: 'cleanup' });
 			}
 
-			// Detect any images which must be saved/removed
+			// Detect any images which must be saved/removed, except when purging,
+			// as we only want to remove containers, remove volumes, create volumes
+			// anew, and start containers without images being removed.
 			steps = steps.concat(
 				saveAndRemoveImages(
 					currentApps,
@@ -325,6 +335,7 @@ export async function getRequiredSteps(
 			steps,
 		),
 	);
+
 	return steps;
 }
 
@@ -717,7 +728,9 @@ function saveAndRemoveImages(
 		(image) => {
 			const notUsedForDelta = !_.includes(deltaSources, image.name);
 			const notUsedByProxyvisor = !_.some(proxyvisorImages, (proxyvisorImage) =>
-				imageManager.isSameImage(image, { name: proxyvisorImage }),
+				imageManager.isSameImage(image, {
+					name: proxyvisorImage,
+				}),
 			);
 			return notUsedForDelta && notUsedByProxyvisor;
 		},

src/device-api/common.d.ts

@@ -1,29 +0,0 @@
-import { Service } from '../compose/service';
-import { InstancedDeviceState } from '../types/state';
-
-export interface ServiceAction {
-	action: string;
-	serviceId: number;
-	current: Service;
-	target: Service;
-	options: any;
-}
-
-declare function doRestart(appId: number, force: boolean): Promise<void>;
-
-declare function doPurge(appId: number, force: boolean): Promise<void>;
-
-declare function serviceAction(
-	action: string,
-	serviceId: number,
-	current: Service,
-	target?: Service,
-	options?: any,
-): ServiceAction;
-
-declare function safeStateClone(
-	targetState: InstancedDeviceState,
-): // Use an any here, because it's not an InstancedDeviceState,
-// and it's also not the exact same type as the API serves from
-// state endpoint (more details in the function)
-Dictionary<any>;

src/device-api/common.js

@@ -57,13 +57,29 @@ export async function doPurge(appId, force) {
 				throw new Error(appNotFoundMessage);
 			}
 
-			const app = allApps[appId];
+			const clonedState = safeStateClone(currentState);
+			/**
+			 * With multi-container, Docker adds an invalid network alias equal to the current containerId
+			 * to that service's network configs when starting a service. Thus when reapplying intermediateState
+			 * after purging, use a cloned state instance which automatically filters out invalid network aliases.
+			 *
+			 * This will prevent error logs like the following:
+			 * https://gist.github.com/cywang117/84f9cd4e6a9641dbed530c94e1172f1d#file-logs-sh-L58
+			 *
+			 * When networks do not match because of their aliases, services are killed and recreated
+			 * an additional time which is unnecessary. Filtering prevents this additional restart BUT
+			 * it is a stopgap measure until we can keep containerId network aliases from being stored
+			 * in state's service config objects (TODO)
+			 *
+			 * See https://github.com/balena-os/balena-supervisor/blob/master/src/device-api/common.js#L160-L180
+			 * for a more in-depth explanation of why aliases need to be filtered out.
+			 */
 
-			const currentServices = app.services;
-			const currentVolumes = app.volumes;
+			// After cloning, set services & volumes as empty to be applied as intermediateTargetState
+			allApps[appId].services = [];
+			allApps[appId].volumes = {};
 
-			app.services = [];
-			app.volumes = {};
+			applicationManager.setIsApplyingIntermediate(true);
 
 			return deviceState
 				.pausingApply(() =>
@@ -79,20 +95,23 @@ export async function doPurge(appId, force) {
 							);
 						})
 						.then(() => {
-							app.services = currentServices;
-							app.volumes = currentVolumes;
-							return deviceState.applyIntermediateTarget(currentState, {
+							return deviceState.applyIntermediateTarget(clonedState, {
 								skipLock: true,
 							});
 						}),
 				)
-				.finally(() => deviceState.triggerApplyTarget());
+				.finally(() => {
+					applicationManager.setIsApplyingIntermediate(false);
+					deviceState.triggerApplyTarget();
+				});
 		}),
 	)
 		.then(() =>
 			logger.logSystemMessage('Purged data', { appId }, 'Purge data success'),
 		)
 		.catch((err) => {
+			applicationManager.setIsApplyingIntermediate(false);
+
 			logger.logSystemMessage(
 				`Error purging data: ${err}`,
 				{ appId, error: err },
@@ -109,6 +128,11 @@ export function serviceAction(action, serviceId, current, target, options) {
 	return { action, serviceId, current, target, options };
 }
 
+/**
+ * This doesn't truly return an InstancedDeviceState, but it's close enough to mostly work where it's used
+ *
+ * @returns { import('../types/state').InstancedDeviceState }
+ */
 export function safeStateClone(targetState) {
 	// We avoid using cloneDeep here, as the class
 	// instances can cause a maximum call stack exceeded
@@ -122,6 +146,7 @@ export function safeStateClone(targetState) {
 	// thing to do would be to represent the input with
 	// io-ts and make sure the below conforms to it
 
+	/** @type { any } */
 	const cloned = {
 		local: {
 			config: {},

test/36-compose-app.spec.ts

@@ -1079,6 +1079,7 @@ describe('compose/app', () => {
 		const steps = current.nextStepsForAppUpdate(defaultContext, target);
 		withSteps(steps).expectStep('kill').to.have.length(2);
 	});
+
 	it('should not create a service when a network it depends on is not ready', async () => {
 		const current = createApp([], [defaultNetwork], [], false);
 		const target = createApp(
@@ -1092,4 +1093,93 @@ describe('compose/app', () => {
 		withSteps(steps).expectStep('createNetwork');
 		withSteps(steps).rejectStep('start');
 	}); // no create service, is create network
+
+	describe('Intermediate state apply support', () => {
+		beforeEach(() => {
+			appMock.mockSupervisorNetwork(true);
+			appMock.mockManagers([], [], []);
+			appMock.mockImages([], false, []);
+
+			applicationManager.setIsApplyingIntermediate(true);
+		});
+		afterEach(() => {
+			appMock.unmockAll();
+			applicationManager.setIsApplyingIntermediate(false);
+		});
+
+		it('should generate the correct step sequence for a volume purge request', async () => {
+			const service = await createService({
+				volumes: ['db-volume'],
+				image: 'test-image',
+			});
+			const volume = Volume.fromComposeObject('db-volume', service.appId, {});
+			const contextWithImages = {
+				...defaultContext,
+				...{
+					availableImages: [
+						{
+							appId: service.appId,
+							dependent: 0,
+							imageId: service.imageId,
+							releaseId: service.releaseId,
+							serviceId: service.serviceId,
+							name: 'test-image',
+							serviceName: service.serviceName,
+						} as Image,
+					],
+				},
+			};
+			// Temporarily set target services & volumes to empty, as in doPurge
+			let intermediateTarget = createApp([], [defaultNetwork], [], true);
+
+			// Generate current with one service & one volume
+			const current = createApp([service], [defaultNetwork], [volume], false);
+
+			// Step 1: kill
+			let steps = current.nextStepsForAppUpdate(
+				contextWithImages,
+				intermediateTarget,
+			);
+			withSteps(steps)
+				.expectStep('kill')
+				.forCurrent(service.serviceName as ServicePredicate);
+			expect(steps).to.have.length(1);
+
+			// Step 2: noop (service is stopping)
+			service.status = 'Stopping';
+			steps = current.nextStepsForAppUpdate(
+				contextWithImages,
+				intermediateTarget,
+			);
+			expectStep('noop', steps);
+			expect(steps).to.have.length(1);
+
+			// No steps, simulate container removal & explicit volume removal as in doPurge
+			current.services = [];
+			steps = current.nextStepsForAppUpdate(
+				contextWithImages,
+				intermediateTarget,
+			);
+			expect(steps).to.have.length(0);
+			current.volumes = {};
+
+			// Step 3: start & createVolume
+			service.status = 'Running';
+			intermediateTarget = createApp(
+				[service],
+				[defaultNetwork],
+				[volume],
+				true,
+			);
+			steps = current.nextStepsForAppUpdate(
+				contextWithImages,
+				intermediateTarget,
+			);
+			expect(steps).to.have.length(2);
+			withSteps(steps)
+				.expectStep('start')
+				.forTarget(service.serviceName as ServicePredicate);
+			expectStep('createVolume', steps);
+		});
+	});
 });