fix: stop racing the daemon when removing containers (#1093)

Containers set `HostConfig.AutoRemove` but act also removes them explicitly, so the two removers race and the loser logs a 409 `removal of container X is already in progress` — seen at the end of nearly every `uses: docker://` step.

The explicit remove is redundant for `docker://` steps and docker actions (`Start(true)` already awaited exit), so it's skipped. Job and service containers keep both removers — their `sleep` entrypoint needs `AutoRemove` as a fallback reaper — so there the race is inherent and `remove()` now treats `NotFound` and `Conflict` as success.

---------

Co-authored-by: bircni <bircni@icloud.com>
Reviewed-on: https://gitea.com/gitea/runner/pulls/1093
Reviewed-by: bircni <bircni@icloud.com>
Co-authored-by: silverwind <me@silverwind.io>

act/container/docker_run.go

@@ -376,6 +376,11 @@ func (cr *containerReference) find() common.Executor {
 	}
 }
 
+// isContainerGone reports whether a failed remove still left the container gone (NotFound or Conflict).
+func isContainerGone(err error) bool {
+	return cerrdefs.IsNotFound(err) || cerrdefs.IsConflict(err)
+}
+
 func (cr *containerReference) remove() common.Executor {
 	return func(ctx context.Context) error {
 		if cr.id == "" {
@@ -387,7 +392,7 @@ func (cr *containerReference) remove() common.Executor {
 			RemoveVolumes: true,
 			Force:         true,
 		})
-		if err != nil {
+		if err != nil && !isContainerGone(err) {
 			logger.Error(fmt.Errorf("failed to remove container: %w", err))
 		}
 

act/container/docker_run_test.go

@@ -116,6 +116,11 @@ func (m *mockDockerClient) ContainerList(ctx context.Context, opts mobyclient.Co
 	return args.Get(0).(mobyclient.ContainerListResult), args.Error(1)
 }
 
+func (m *mockDockerClient) ContainerRemove(ctx context.Context, id string, opts mobyclient.ContainerRemoveOptions) (mobyclient.ContainerRemoveResult, error) {
+	args := m.Called(ctx, id, opts)
+	return args.Get(0).(mobyclient.ContainerRemoveResult), args.Error(1)
+}
+
 type endlessReader struct {
 	io.Reader
 }
@@ -381,6 +386,40 @@ func TestDockerCopyTarStreamErrorInMkdir(t *testing.T) {
 	client.AssertExpectations(t)
 }
 
+// A remove that raced the daemon's AutoRemove teardown is not a failure and must not
+// be logged as one.
+func TestRemoveIgnoresAutoRemoveRace(t *testing.T) {
+	removeOpts := mobyclient.ContainerRemoveOptions{RemoveVolumes: true, Force: true}
+	for _, tc := range []struct {
+		name     string
+		err      error
+		wantLogs bool
+	}{
+		{name: "removal in progress", err: cerrdefs.ErrConflict.WithMessage("removal of container abc is already in progress")},
+		{name: "already removed", err: cerrdefs.ErrNotFound.WithMessage("No such container: abc")},
+		{name: "removed cleanly", err: nil},
+		{name: "real failure", err: errors.New("driver failed to remove root filesystem"), wantLogs: true},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			logger, hook := test.NewNullLogger()
+			ctx := common.WithLogger(context.Background(), logger)
+			client := &mockDockerClient{}
+			client.On("ContainerRemove", ctx, "abc", removeOpts).Return(mobyclient.ContainerRemoveResult{}, tc.err)
+			cr := &containerReference{id: "abc", cli: client}
+
+			require.NoError(t, cr.remove()(ctx))
+			assert.Empty(t, cr.id)
+
+			if tc.wantLogs {
+				assert.Len(t, hook.AllEntries(), 1)
+			} else {
+				assert.Empty(t, hook.AllEntries())
+			}
+			client.AssertExpectations(t)
+		})
+	}
+}
+
 // find() must drop a stale cached id so later Copy/Exec don't hit the
 // daemon with a torn-down container.
 func TestFindRevalidatesStaleID(t *testing.T) {