engine: don't stop container when applied status is running

CHANGELOG.md

@@ -1,5 +1,8 @@
 # Changelog
 
+## 1.20.0 - dev
+* Bug - Fixed a bug where container marked as stopped comes back with a running status [#1446](https://github.com/aws/amazon-ecs-agent/pull/1446)
+
 ## 1.19.0
 * Feature - Private registry can be authenticated through task definition using AWS Secrets Manager [#1427](https://github.com/aws/amazon-ecs-agent/pull/1427)
 

agent/engine/task_manager.go

@@ -973,7 +973,20 @@ func (mtask *managedTask) containerNextState(container *apicontainer.Container)
 		// It's not enough to just check if container is in steady state here
 		// we should really check if >= RUNNING <= STOPPED
 		if !container.IsRunning() {
-			// If it's not currently running we do not need to do anything to make it become stopped.
+			// If the container's AppliedStatus is running, it means the StartContainer
+			// api call has already been scheduled, we should not mark it as stopped
+			// directly, because when the stopped container comes back, we will end up
+			// with either:
+			// 1. The task is not cleaned up, the handleStoppedToRunningContainerTransition
+			// function will handle this case, but only once. If there are some
+			// other stopped containers come back, they will not be stopped by
+			// Agent.
+			// 2. The task has already been cleaned up, in this case any stopped container
+			// will not be stopped by Agent when they come back.
+			if container.GetAppliedStatus() == apicontainerstatus.ContainerRunning {
+				nextState = apicontainerstatus.ContainerStatusNone
+			}
+
 			return &containerTransition{
 				nextState:      nextState,
 				actionRequired: false,

agent/engine/task_manager_test.go

@@ -567,11 +567,36 @@ func TestContainerNextStateWithPullCredentials(t *testing.T) {
 			transition := task.containerNextState(container)
 			assert.Equal(t, tc.expectedContainerStatus, transition.nextState, "Mismatch container status")
 			assert.Equal(t, tc.expectedTransitionReason, transition.reason, "Mismatch transition possible")
-			assert.Equal(t, tc.expectedTransitionActionable, transition.actionRequired, "Mismatch transition actionalbe")
+			assert.Equal(t, tc.expectedTransitionActionable, transition.actionRequired, "Mismatch transition actionable")
 		})
 	}
 }
 
+func TestContainerNextStateWithAvoidingDanglingContainers(t *testing.T) {
+	container := &apicontainer.Container{
+		DesiredStatusUnsafe:       apicontainerstatus.ContainerStopped,
+		KnownStatusUnsafe:         apicontainerstatus.ContainerCreated,
+		AppliedStatus:             apicontainerstatus.ContainerRunning,
+		TransitionDependenciesMap: make(map[apicontainerstatus.ContainerStatus]apicontainer.TransitionDependencySet),
+	}
+
+	task := &managedTask{
+		Task: &apitask.Task{
+			Containers: []*apicontainer.Container{
+				container,
+			},
+			DesiredStatusUnsafe: apitaskstatus.TaskStopped,
+		},
+		engine: &DockerTaskEngine{},
+	}
+	transition := task.containerNextState(container)
+	assert.Equal(t, apicontainerstatus.ContainerStatusNone, transition.nextState,
+		"Expected next state [%s] != Retrieved next state [%s]",
+		apicontainerstatus.ContainerStatusNone.String(), transition.nextState.String())
+	assert.Equal(t, false, transition.actionRequired, "Mismatch transition actionable")
+	assert.Equal(t, nil, transition.reason, "Mismatch transition possible")
+}
+
 func TestStartContainerTransitionsWhenForwardTransitionPossible(t *testing.T) {
 	steadyStates := []apicontainerstatus.ContainerStatus{apicontainerstatus.ContainerRunning, apicontainerstatus.ContainerResourcesProvisioned}
 	for _, steadyState := range steadyStates {