Wait for service dependencies to be running

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,29 +1293,31 @@ 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' } },
 					),
 				],
 				isTarget: true,
 			});
 
 			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(