From ca9945bdfb716e1b14b24d3f3069f48cc7aff493 Mon Sep 17 00:00:00 2001
From: 20k-ultra <3946250+20k-ultra@users.noreply.github.com>
Date: Tue, 12 Apr 2022 02:05:56 -0400
Subject: [PATCH] Only start a container once in its lifetime This will ensure
 the restart policy specified is not violated

Change-type: patch
Closes: #1668
Signed-off-by: 20k-ultra <3946250+20k-ultra@users.noreply.github.com>
---
 src/compose/app.ts                           | 16 +++++++---------
 test/src/compose/app.spec.ts                 |  9 +++------
 test/src/compose/application-manager.spec.ts | 10 ++++------
 3 files changed, 14 insertions(+), 21 deletions(-)

diff --git a/src/compose/app.ts b/src/compose/app.ts
index bf1ff313..ab6c6638 100644
--- a/src/compose/app.ts
+++ b/src/compose/app.ts
@@ -336,15 +336,13 @@ export class App {
 				return false;
 			}
 
-			// Check if we previously remember starting it
-			if (
-				applicationManager.containerStarted[serviceCurrent.containerId!] != null
-			) {
-				return false;
-			}
-
-			// If the config otherwise matches, then we should be running
-			return isEqualExceptForRunningState(serviceCurrent, serviceTarget);
+			// Only start a Service if we have never started it before and the service matches target!
+			// This is so the engine can handle the restart policy configured for the container.
+			return (
+				(serviceCurrent.status === 'Installing' ||
+					serviceCurrent.status === 'Installed') &&
+				isEqualExceptForRunningState(serviceCurrent, serviceTarget)
+			);
 		};
 
 		/**
diff --git a/test/src/compose/app.spec.ts b/test/src/compose/app.spec.ts
index b5ade6a9..1817260b 100644
--- a/test/src/compose/app.spec.ts
+++ b/test/src/compose/app.spec.ts
@@ -951,7 +951,7 @@ describe('compose/app', () => {
 			applicationManager.containerStarted = {};
 		});
 
-		it('should create a start step when all that changes is a running state', async () => {
+		it('should not create a start step when all that changes is a running state', async () => {
 			const contextWithImages = {
 				...defaultContext,
 				...{
@@ -972,13 +972,10 @@ describe('compose/app', () => {
 				isTarget: true,
 			});
 
-			// now should see a 'start'
 			const steps = current.nextStepsForAppUpdate(contextWithImages, target);
 
-			const [startStep] = expectSteps('start', steps);
-			expect(startStep)
-				.to.have.property('target')
-				.that.deep.includes({ serviceName: 'main' });
+			// There should be no steps since the engine manages restart policy for stopped containers
+			expect(steps.length).to.equal(0);
 		});
 
 		it('should create a kill step when a service release has to be updated but the strategy is kill-then-download', async () => {
diff --git a/test/src/compose/application-manager.spec.ts b/test/src/compose/application-manager.spec.ts
index 015f7703..2d734b99 100644
--- a/test/src/compose/application-manager.spec.ts
+++ b/test/src/compose/application-manager.spec.ts
@@ -208,7 +208,7 @@ describe('compose/application-manager', () => {
 
 	// TODO: missing tests for getCurrentApps
 
-	it('infers a start step when all that changes is a running state', async () => {
+	it('should not infer a start step when all that changes is a running state', async () => {
 		const targetApps = createApps(
 			{
 				services: [await createService({ running: true, appId: 1 })],
@@ -226,7 +226,7 @@ describe('compose/application-manager', () => {
 			networks: [DEFAULT_NETWORK],
 		});
 
-		const [startStep] = await applicationManager.inferNextSteps(
+		const steps = await applicationManager.inferNextSteps(
 			currentApps,
 			targetApps,
 			{
@@ -236,10 +236,8 @@ describe('compose/application-manager', () => {
 			},
 		);
 
-		expect(startStep).to.have.property('action').that.equals('start');
-		expect(startStep)
-			.to.have.property('target')
-			.that.deep.includes({ serviceName: 'main' });
+		// There should be no steps since the engine manages restart policy for stopped containers
+		expect(steps.length).to.equal(0);
 	});
 
 	it('infers a kill step when a service has to be removed', async () => {