Wait for service dependencies to be running

This fixes a regression where dependencies would only be started in
order and would start the dependent service if its dependency had been
started at some point in the past, regardless of the running condition.

This makes the behavior more consistent with docker compose where the
[dependency needs to be
running or healthy](69a83d1303/pkg/compose/convergence.go (L441)) for the service to be started.

Change-type: patch

src/compose/app.ts

@@ -813,24 +813,28 @@ class AppImpl implements App {
 					serviceName: target.serviceName,
 				}),
 			];
-		} else if (
-			target != null &&
-			this.dependenciesMetForServiceStart(
-				target,
-				targetApp,
-				availableImages,
-				networkPairs,
-				volumePairs,
-				servicePairs,
-			)
-		) {
-			if (!servicesLocked) {
-				this.services
-					.concat(targetApp.services)
-					.forEach((svc) => appsToLock[target.appId].add(svc.serviceName));
-				return [];
+		} else if (target != null) {
+			if (
+				this.dependenciesMetForServiceStart(
+					target,
+					targetApp,
+					availableImages,
+					networkPairs,
+					volumePairs,
+					servicePairs,
+				)
+			) {
+				if (!servicesLocked) {
+					this.services
+						.concat(targetApp.services)
+						.forEach((svc) => appsToLock[target.appId].add(svc.serviceName));
+					return [];
+				}
+				return [generateStep('start', { target })];
+			} else {
+				// Wait for dependencies to be started
+				return [generateStep('noop', {})];
 			}
-			return [generateStep('start', { target })];
 		} else {
 			return [];
 		}
@@ -881,7 +885,7 @@ class AppImpl implements App {
 		// different to a dependency which is in the servicePairs below, as these
 		// 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) => {
+		const depInstallingButNotRunning = _.some(this.services, (svc) => {
 			if (target.dependsOn?.includes(svc.serviceName)) {
 				if (!svc.config.running) {
 					return true;

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

@@ -821,9 +821,9 @@ describe('compose/application-manager', () => {
 					containerIdsByAppId,
 				},
 			);
-			expectSteps('noop', steps2, 1);
+
 			// No other steps
-			expect(steps2).to.have.length(1);
+			expect(steps2.every((s) => s.action === 'noop'));
 
 			/**
 			 * Only start target services after both images downloaded
@@ -932,7 +932,7 @@ describe('compose/application-manager', () => {
 		);
 
 		// Only noop steps should be seen at this point
-		expect(steps.filter((s) => s.action !== 'noop')).to.have.lengthOf(0);
+		expect(steps.every((s) => s.action === 'noop'));
 	});
 
 	it('infers to kill several services as long as there is no unmet dependency', async () => {
@@ -1099,7 +1099,7 @@ describe('compose/application-manager', () => {
 			.that.deep.includes({ serviceName: 'dep' });
 
 		// No more steps until the first container has been started
-		expect(nextSteps).to.have.lengthOf(0);
+		expect(nextSteps.every((s) => s.action === 'noop'));
 	});
 
 	it('infers to start a service once its dependency has been met', async () => {

test/unit/compose/app.spec.ts

@@ -348,7 +348,6 @@ describe('compose/app', () => {
 					target,
 				);
 
-			expect(recreateVolumeSteps).to.have.length(1);
 			expectSteps('createVolume', recreateVolumeSteps);
 
 			// Step 5: takeLock
@@ -1294,22 +1293,23 @@ describe('compose/app', () => {
 				.to.deep.include({ serviceName: 'main' });
 		});
 
-		it('should not try to start a container which has exited and has restart policy of no', async () => {
+		it('should not try to start a container which has exited', async () => {
 			// Container is a "run once" type of service so it has exitted.
 			const current = createApp({
 				services: [
 					await createService(
-						{ composition: { restart: 'no' }, running: false },
+						{ composition: { restart: 'yes' }, running: false },
 						{ state: { containerId: 'run_once' } },
 					),
 				],
+				networks: [DEFAULT_NETWORK],
 			});
 
 			// Now test that another start step is not added on this service
 			const target = createApp({
 				services: [
 					await createService(
-						{ composition: { restart: 'no' }, running: false },
+						{ composition: { restart: 'always' }, running: false },
 						{ state: { containerId: 'run_once' } },
 					),
 				],
@@ -1317,6 +1317,7 @@ describe('compose/app', () => {
 			});
 
 			const steps = current.nextStepsForAppUpdate(defaultContext, target);
+			expect(steps.length).to.equal(0);
 			expectNoStep('start', steps);
 		});
 
@@ -1472,6 +1473,83 @@ describe('compose/app', () => {
 				.that.deep.includes({ serviceName: 'main' });
 		});
 
+		it('should not start a container when it depends on a service that is not running', async () => {
+			const current = createApp({
+				services: [
+					await createService(
+						{
+							running: false,
+							appId: 1,
+							serviceName: 'dep',
+						},
+						{
+							state: {
+								containerId: 'dep-id',
+							},
+						},
+					),
+				],
+				networks: [DEFAULT_NETWORK],
+			});
+			const target = createApp({
+				services: [
+					await createService({
+						appId: 1,
+						serviceName: 'main',
+						composition: {
+							depends_on: ['dep'],
+						},
+					}),
+					await createService({
+						appId: 1,
+						serviceName: 'dep',
+					}),
+				],
+				networks: [DEFAULT_NETWORK],
+				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 },
+				lock: mockLock,
+			};
+
+			// Only one start step and it should be that of the 'dep' service
+			const stepsToIntermediate = current.nextStepsForAppUpdate(
+				contextWithImages,
+				target,
+			);
+			expectNoStep('start', stepsToIntermediate);
+			expectSteps('noop', stepsToIntermediate);
+
+			// we now make our current state have the 'dep' service as started...
+			const intermediate = createApp({
+				services: [
+					await createService(
+						{ appId: 1, serviceName: 'dep' },
+						{ state: { containerId: 'dep-id' } },
+					),
+				],
+				networks: [DEFAULT_NETWORK],
+			});
+
+			// we should now see a start for the 'main' service...
+			const stepsToTarget = intermediate.nextStepsForAppUpdate(
+				{ ...contextWithImages, ...{ containerIds: { dep: 'dep-id' } } },
+				target,
+			);
+			const [startMainStep] = expectSteps('start', stepsToTarget);
+			expect(startMainStep)
+				.to.have.property('target')
+				.that.deep.includes({ serviceName: 'main' });
+		});
+
 		it('should not create a start step when all that changes is a running state', async () => {
 			const contextWithImages = {
 				...defaultContext,
@@ -1993,7 +2071,7 @@ describe('compose/app', () => {
 				target,
 			);
 			expectNoStep('start', steps);
-			expectSteps('noop', steps, 1);
+			expectSteps('noop', steps, 1, Infinity);
 
 			// Take lock before starting once downloads complete
 			const steps2 = current.nextStepsForAppUpdate(