DO NOT MERGE: testing update lock bug

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

src/compose/app.ts

@@ -25,6 +25,7 @@ import { checkTruthy } from '../lib/validation';
 import type { ServiceComposeConfig, DeviceMetadata } from './types/service';
 import { pathExistsOnRoot } from '../lib/host-utils';
 import { isSupervisor } from '../lib/supervisor-metadata';
+import { isLockInProgress } from '../lib/update-lock';
 import type { LocksTakenMap } from '../lib/update-lock';
 
 export interface AppConstructOpts {
@@ -169,14 +170,21 @@ export class App {
 
 		// Generate lock steps from appsToLock
 		for (const [appId, services] of Object.entries(appsToLock)) {
+			const numAppId = parseInt(appId, 10);
 			if (services.size > 0) {
-				steps.push(
-					generateStep('takeLock', {
-						appId: parseInt(appId, 10),
-						services: Array.from(services),
-						force: state.force,
-					}),
-				);
+				// Keep state funnel alive while locks are in process of
+				// being taken, as it's not an instant operation
+				if (isLockInProgress(numAppId)) {
+					steps.push(generateStep('noop', {}));
+				} else {
+					steps.push(
+						generateStep('takeLock', {
+							appId: numAppId,
+							services: Array.from(services),
+							force: state.force,
+						}),
+					);
+				}
 			}
 		}
 
@@ -219,6 +227,7 @@ export class App {
 				);
 			}
 		}
+		console.log({ steps });
 		return steps;
 	}
 
@@ -232,13 +241,19 @@ export class App {
 					(svc) => !state.locksTaken.isLocked(svc.appId, svc.serviceName),
 				)
 			) {
-				return [
-					generateStep('takeLock', {
-						appId: this.appId,
-						services: this.services.map((svc) => svc.serviceName),
-						force: state.force,
-					}),
-				];
+				// Keep state funnel alive while locks are in process of
+				// being taken, as it's not an instant operation
+				if (isLockInProgress(this.appId)) {
+					return [generateStep('noop', {})];
+				} else {
+					return [
+						generateStep('takeLock', {
+							appId: this.appId,
+							services: this.services.map((svc) => svc.serviceName),
+							force: state.force,
+						}),
+					];
+				}
 			}
 
 			return Object.values(this.services).map((service) =>

src/compose/composition-steps.ts

@@ -10,6 +10,7 @@ import type Volume from './volume';
 import * as commitStore from './commit';
 import * as updateLock from '../lib/update-lock';
 import type { DeviceLegacyReport } from '../types/state';
+import { promises as fs } from 'fs';
 
 interface CompositionStepArgs {
 	stop: {
@@ -217,6 +218,20 @@ export function getExecutors(app: { callbacks: CompositionCallbacks }) {
 		},
 		takeLock: async (step) => {
 			await updateLock.takeLock(step.appId, step.services, step.force);
+			console.log(
+				'services locked after takeLock action according to getServicesLockedByAppId',
+				JSON.stringify(await updateLock.getServicesLockedByAppId(), null, 2),
+			);
+			const services = await fs.readdir(
+				`/mnt/root/tmp/balena-supervisor/services/${step.appId}`,
+			);
+			for (const service of services) {
+				const servicePath = `/mnt/root/tmp/balena-supervisor/services/${step.appId}/${service}`;
+				const contents = await fs.readdir(servicePath);
+				if (contents.length) {
+					console.log(`Service ${service} has ${contents.join(', ')}`);
+				}
+			}
 		},
 		releaseLock: async (step) => {
 			await updateLock.releaseLock(step.appId);

src/lib/update-lock.ts

@@ -80,6 +80,11 @@ async function dispose(
 	}
 }
 
+const locksInProgress: { [appId: number]: boolean } = {};
+
+export const isLockInProgress = (appId: number): boolean =>
+	!!locksInProgress[appId];
+
 /**
  * Composition step used by Supervisor compose module.
  * Take all locks for an appId | appUuid, creating directories if they don't exist.
@@ -96,7 +101,9 @@ export async function takeLock(
 	});
 
 	const release = await takeGlobalLockRW(appId);
+	locksInProgress[appId] = true;
 	try {
+		const lockOverride = await config.get('lockOverride');
 		const actuallyLocked: string[] = [];
 		const locksTaken = await getServicesLockedByAppId();
 		// Filter out services that already have Supervisor-taken locks.
@@ -107,7 +114,7 @@ export async function takeLock(
 		);
 		for (const service of servicesWithoutLock) {
 			await mkdirp(pathOnRoot(lockPath(appId, service)));
-			await lockService(appId, service, force);
+			await lockService(appId, service, force || lockOverride);
 			actuallyLocked.push(service);
 		}
 		return actuallyLocked;
@@ -116,12 +123,14 @@ export async function takeLock(
 		// lockfiles that may have been created so that all services return
 		// to unlocked status.
 		await dispose(appId, release);
+		locksInProgress[appId] = false;
 		// 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();
+		locksInProgress[appId] = false;
 	}
 }
 
@@ -215,7 +224,10 @@ export async function getLocksTaken(
  * [appId, serviceName] pair for a service to be considered locked.
  */
 export async function getServicesLockedByAppId(): Promise<LocksTakenMap> {
+	console.log('In getServicesLockedByAppId');
 	const locksTaken = await getLocksTaken();
+	console.log({ locksTaken });
+
 	// 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.
@@ -223,11 +235,13 @@ export async function getServicesLockedByAppId(): Promise<LocksTakenMap> {
 	for (const lockTakenPath of locksTaken) {
 		const [appId, serviceName, filename] =
 			getIdentifiersFromPath(lockTakenPath);
+		console.log({ appId, serviceName, filename });
 		if (
 			!StringIdentifier.is(appId) ||
 			!DockerName.is(serviceName) ||
 			!filename?.match(/updates\.lock/)
 		) {
+			console.log(`Skipping ${lockTakenPath}`);
 			continue;
 		}
 		const numAppId = +appId;
@@ -245,7 +259,9 @@ export async function getServicesLockedByAppId(): Promise<LocksTakenMap> {
 	// services locked by the Supervisor.
 	const servicesByAppId = new LocksTakenMap();
 	for (const [appId, servicesTaken] of filesTakenByAppId) {
+		console.log({ appId, servicesTaken });
 		for (const [serviceName, filenames] of servicesTaken) {
+			console.log({ serviceName, filenames });
 			if (
 				filenames.has('resin-updates.lock') &&
 				filenames.has('updates.lock')

src/types/basic.ts

@@ -96,7 +96,7 @@ const CONFIG_VAR_NAME_REGEX = /^[a-zA-Z_][a-zA-Z0-9_:]*$/;
 const shortStringWithRegex = (name: string, regex: RegExp, message: string) =>
 	new t.Type<string, string>(
 		name,
-		(s: unknown): s is string => ShortString.is(s) && VAR_NAME_REGEX.test(s),
+		(s: unknown): s is string => ShortString.is(s) && regex.test(s),
 		(i, c) =>
 			pipe(
 				ShortString.validate(i, c),

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

@@ -15,6 +15,11 @@ import { mkdirp } from '~/lib/fs-utils';
 import { takeGlobalLockRW } from '~/lib/process-lock';
 
 describe('lib/update-lock', () => {
+	beforeEach(async () => {
+		await config.initialized();
+		await config.set({ lockOverride: false });
+	});
+
 	describe('abortIfHUPInProgress', () => {
 		const breadcrumbFiles = [
 			'rollback-health-breadcrumb',
@@ -106,9 +111,6 @@ describe('lib/update-lock', () => {
 			).to.eventually.deep.equal(exists ? supportedLockfiles : []);
 
 		before(async () => {
-			await config.initialized();
-			await config.set({ lockOverride: false });
-
 			// Ensure the directory is available for all tests
 			await fs.mkdir(lockdir(testAppId, testServiceName), {
 				recursive: true,

test/unit/types.spec.ts

@@ -1,7 +1,7 @@
 import { expect } from 'chai';
 import * as t from 'io-ts';
 
-import { withDefault } from '~/src/types';
+import { withDefault, DockerName } from '~/src/types';
 
 describe('types', () => {
 	describe('withDefault', () => {
@@ -39,4 +39,32 @@ describe('types', () => {
 				.that.equals(false);
 		});
 	});
+
+	describe('DockerName', () => {
+		// Should match /^[a-zA-Z0-9][a-zA-Z0-9_.-]*$/
+		it('decodes valid Docker names', () => {
+			const validDockerNames = [
+				'a',
+				'A',
+				'0',
+				'a0',
+				'A0',
+				'a.',
+				'A.',
+				'a-',
+				'A-',
+				'a_',
+				'A_',
+				'a.a',
+				'A.A',
+				'a-a',
+				'A-A',
+				'a_a',
+				'A_A',
+			];
+			for (const name of validDockerNames) {
+				expect(DockerName.is(name)).to.be.true;
+			}
+		});
+	});
 });