Service installs: Fix creating two tasks when moving a device

src/features/ci-cd/hooks/service-installs.ts

@@ -287,18 +287,25 @@ hooks.addPureHook('PATCH', 'resin', 'device', {
 		const affectedIds = await sbvrUtils.getAffectedIds(args);
 		if (affectedIds.length !== 0) {
 			await deleteServiceInstallsForCurrentApp(args.api, newAppId, affectedIds);
-			await createAppServiceInstalls(args.api, newAppId, affectedIds, args.tx);
 		}
 	},
-});
-
-hooks.addPureHook('PATCH', 'resin', 'device', {
 	POSTRUN: async ({ api, request, tx }) => {
 		const affectedIds = request.affectedIds!;
-		if (
-			request.values.is_pinned_on__release !== undefined &&
-			affectedIds.length !== 0
-		) {
+		if (affectedIds.length === 0) {
+			return;
+		}
+		const newAppId = request.values.belongs_to__application;
+		if (newAppId != null) {
+			// We could also have an optimization for the case that `values.is_pinned_on__release != null`
+			// to make the part that finds the target release faster, but chose to keep this simpler since:
+			// a) We expect that in the majority of device move requests users will not be also be pinning
+			//    the device at the same time.
+			// b) Only the sync approach would benefit from it, since creating the service installs via tasks
+			//    (which is going to be the default), only accepts the deviceIds as a parameter,
+			await createAppServiceInstalls(api, newAppId, affectedIds, tx);
+			return;
+		}
+		if (request.values.is_pinned_on__release !== undefined) {
 			// If the device was preloaded, and then pinned, service_installs do not exist
 			// for this device+release combination. We need to create these
 			if (request.values.is_pinned_on__release != null) {

test/25_service-installs.ts

@@ -8,6 +8,7 @@ import * as fixtures from './test-lib/fixtures.js';
 import { assertExists, expectToEventually } from './test-lib/common.js';
 import * as config from '../src/lib/config.js';
 import { supertest } from './test-lib/supertest.js';
+import { expectNewTasks, resetLatestTaskIds } from './test-lib/api-helpers.js';
 
 export default () => {
 	versions.test((version, pineTest) => {
@@ -45,6 +46,8 @@ export default () => {
 
 					config.TEST_MOCK_ONLY.ASYNC_TASK_CREATE_SERVICE_INSTALLS_ENABLED =
 						isServiceInstallEnabled;
+
+					await resetLatestTaskIds('create_service_installs');
 				});
 
 				after(async () => {
@@ -74,6 +77,19 @@ export default () => {
 							ctx.app1Service1.id,
 						);
 					});
+					await expectNewTasks(
+						'create_service_installs',
+						isServiceInstallEnabled
+							? [
+									{
+										is_executed_with__parameter_set: {
+											devices: [ctx.device.id],
+										},
+										status: 'succeeded',
+									},
+								]
+							: [],
+					);
 				});
 
 				it('for pinning an application to a release', async () => {
@@ -104,6 +120,19 @@ export default () => {
 							ctx.app1Service2.id,
 						);
 					});
+					await expectNewTasks(
+						'create_service_installs',
+						isServiceInstallEnabled
+							? [
+									{
+										is_executed_with__parameter_set: {
+											devices: [ctx.device.id],
+										},
+										status: 'succeeded',
+									},
+								]
+							: [],
+					);
 				});
 
 				it('when a device is pinned on a different release', async () => {
@@ -133,6 +162,19 @@ export default () => {
 							ctx.app1Service3.id,
 						);
 					});
+					await expectNewTasks(
+						'create_service_installs',
+						isServiceInstallEnabled
+							? [
+									{
+										is_executed_with__parameter_set: {
+											devices: [ctx.device.id],
+										},
+										status: 'succeeded',
+									},
+								]
+							: [],
+					);
 				});
 
 				it('when device is moved to different application', async () => {
@@ -170,6 +212,27 @@ export default () => {
 							ctx.app2Service1.id,
 						);
 					});
+					await expectNewTasks(
+						'create_service_installs',
+						isServiceInstallEnabled
+							? [
+									// the first one is from unpinning
+									{
+										is_executed_with__parameter_set: {
+											devices: [ctx.device.id],
+										},
+										status: 'succeeded',
+									},
+									// the second one is from moving application
+									{
+										is_executed_with__parameter_set: {
+											devices: [ctx.device.id],
+										},
+										status: 'succeeded',
+									},
+								]
+							: [],
+					);
 				});
 
 				it('should be able to use service_install to create a device_service_environment_variable', async () => {

test/test-lib/api-helpers.ts

@@ -7,7 +7,9 @@ import type { UserObjectParam } from '../test-lib/supertest.js';
 import { supertest } from '../test-lib/supertest.js';
 import type { TokenUserPayload } from '../../src/index.js';
 import type { RequiredField } from '@balena/pinejs/out/sbvr-api/common-types.js';
-import { assertExists } from './common.js';
+import { assertExists, expectToEventually } from './common.js';
+import { sbvrUtils, permissions } from '@balena/pinejs';
+import type { tasks } from '@balena/pinejs';
 
 const version = 'resin';
 
@@ -208,7 +210,7 @@ export const thatIsDateStringAfter = (
 
 const validJwtProps = ['id', 'jwt_secret', 'authTime', 'iat', 'exp'].sort();
 
-export function expectJwt(tokenOrJwt: string | AnyObject) {
+export function expectJwt(tokenOrJwt: string | object) {
 	const decoded = (
 		typeof tokenOrJwt === 'string'
 			? jsonwebtoken.decode(tokenOrJwt)
@@ -232,3 +234,86 @@ export function expectJwt(tokenOrJwt: string | AnyObject) {
 
 	return decoded;
 }
+
+export type TaskExpectation = Pick<
+	tasks.Task['Read'],
+	'is_executed_with__parameter_set' | 'status'
+>;
+
+const expectTasks = async (
+	handler: string,
+	expectedTasks: TaskExpectation[],
+	lastId = '0',
+) => {
+	const tasks = await sbvrUtils.api.tasks.get({
+		resource: 'task',
+		passthrough: { req: permissions.rootRead },
+		options: {
+			$select: ['id', 'is_executed_with__parameter_set', 'status'],
+			$filter: {
+				id: { $gt: lastId },
+				is_executed_by__handler: handler,
+			},
+			$orderby: { id: 'asc' },
+		},
+	});
+	const actual = tasks.map((t) =>
+		_.pick(t, ['is_executed_with__parameter_set', 'status']),
+	);
+
+	expect(actual).to.deep.equal(expectedTasks);
+	return tasks;
+};
+
+const latestTaskIdByHandler: Dictionary<string | undefined> = {};
+
+export const expectNewTasks = async (
+	handler: string,
+	expectedUsage: TaskExpectation[],
+) => {
+	const tasks = await expectTasks(
+		handler,
+		expectedUsage,
+		latestTaskIdByHandler[handler],
+	);
+	const lastTaskId = tasks.at(-1)?.id;
+	if (lastTaskId != null) {
+		latestTaskIdByHandler[handler] = lastTaskId;
+	}
+};
+
+export const resetLatestTaskIds = async (handler: string) => {
+	await expectToEventually(async () => {
+		const runningTasks = await sbvrUtils.api.tasks.get({
+			resource: 'task',
+			passthrough: { req: permissions.rootRead },
+			options: {
+				$top: 1,
+				$select: 'id',
+				$filter: {
+					status: 'queued',
+					is_executed_by__handler: handler,
+				},
+				$orderby: { id: 'desc' },
+			},
+		});
+		expect(runningTasks).to.have.lengthOf(0);
+	});
+	const [latestTask] = await sbvrUtils.api.tasks.get({
+		resource: 'task',
+		passthrough: { req: permissions.rootRead },
+		options: {
+			$top: 1,
+			$select: 'id',
+			$filter: {
+				status: 'succeeded',
+				is_executed_by__handler: handler,
+			},
+			$orderby: { id: 'desc' },
+		},
+	});
+	if (latestTask == null) {
+		return;
+	}
+	latestTaskIdByHandler[handler] = latestTask?.id;
+};