From 89467c9dd07d017f05797e6a23d9fcc937a7e823 Mon Sep 17 00:00:00 2001 From: silverwind Date: Tue, 21 Jul 2026 11:03:15 +0000 Subject: [PATCH] fix: stop racing the daemon when removing containers (#1093) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-on: https://gitea.com/gitea/runner/pulls/1093 Reviewed-by: bircni Co-authored-by: silverwind --- act/container/docker_run.go | 7 +++++- act/container/docker_run_test.go | 39 ++++++++++++++++++++++++++++++++ act/runner/action.go | 2 +- act/runner/action_test.go | 39 ++++++++++++++++++++++++++++++++ act/runner/step_docker.go | 2 +- act/runner/step_docker_test.go | 38 +++++++++++++++++++++++++++++++ 6 files changed, 124 insertions(+), 3 deletions(-) diff --git a/act/container/docker_run.go b/act/container/docker_run.go index 87323cfd..8365562b 100644 --- a/act/container/docker_run.go +++ b/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)) } diff --git a/act/container/docker_run_test.go b/act/container/docker_run_test.go index dc51278e..57c180fa 100644 --- a/act/container/docker_run_test.go +++ b/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) { diff --git a/act/runner/action.go b/act/runner/action.go index ea93c571..4e736a09 100644 --- a/act/runner/action.go +++ b/act/runner/action.go @@ -405,7 +405,7 @@ func execAsDocker(ctx context.Context, step actionStep, actionName, actionDir, b stepContainer.Create(rc.Config.ContainerCapAdd, rc.Config.ContainerCapDrop), stepContainer.Start(true), ).Finally( - stepContainer.Remove().IfBool(!rc.Config.ReuseContainers), + stepContainer.Remove().IfBool(!rc.Config.ReuseContainers && !rc.Config.AutoRemove), ).Finally(stepContainer.Close())(ctx) } diff --git a/act/runner/action_test.go b/act/runner/action_test.go index 9122e025..a3b686fe 100644 --- a/act/runner/action_test.go +++ b/act/runner/action_test.go @@ -20,6 +20,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" ) type closerMock struct { @@ -150,6 +151,44 @@ runs: } } +// With AutoRemove the daemon reaps the container on exit, so act must not remove it afterwards. +func TestExecAsDockerAutoRemove(t *testing.T) { + orig := ContainerNewContainer + defer func() { ContainerNewContainer = orig }() + + for _, tc := range []struct { + autoRemove bool + removes int + }{ + {false, 2}, // stale + post-run + {true, 1}, // post-run skipped + } { + cm := &containerMock{} + ContainerNewContainer = func(*container.NewContainerInput) container.ExecutionsEnvironment { return cm } + + step := &stepActionRemote{ + Step: &model.Step{ID: "1", Uses: "org/action@v1"}, + RunContext: &RunContext{ + Config: &Config{AutoRemove: tc.autoRemove}, + Run: &model.Run{JobID: "1", Workflow: &model.Workflow{Jobs: map[string]*model.Job{"1": {}}}}, + JobContainer: cm, + }, + action: &model.Action{Runs: model.ActionRuns{Using: "docker", Image: "docker://node:14"}}, + } + + removes := 0 + cm.On("Pull", false).Return(func(context.Context) error { return nil }) + cm.On("Remove").Return(func(context.Context) error { removes++; return nil }) + cm.On("Create", []string(nil), []string(nil)).Return(func(context.Context) error { return nil }) + cm.On("Start", true).Return(func(context.Context) error { return nil }) + cm.On("Close").Return(func(context.Context) error { return nil }) + + require.NoError(t, execAsDocker(context.Background(), step, "action", t.TempDir(), t.TempDir(), false)) + cm.AssertExpectations(t) + assert.Equal(t, tc.removes, removes) + } +} + func TestActionRunner(t *testing.T) { table := []struct { name string diff --git a/act/runner/step_docker.go b/act/runner/step_docker.go index 9d2a85a1..ecdf26f9 100644 --- a/act/runner/step_docker.go +++ b/act/runner/step_docker.go @@ -85,7 +85,7 @@ func (sd *stepDocker) runUsesContainer() common.Executor { stepContainer.Create(rc.Config.ContainerCapAdd, rc.Config.ContainerCapDrop), stepContainer.Start(true), ).Finally( - stepContainer.Remove().IfBool(!rc.Config.ReuseContainers), + stepContainer.Remove().IfBool(!rc.Config.ReuseContainers && !rc.Config.AutoRemove), ).Finally(stepContainer.Close())(ctx) } } diff --git a/act/runner/step_docker_test.go b/act/runner/step_docker_test.go index da508d1f..2aac21f3 100644 --- a/act/runner/step_docker_test.go +++ b/act/runner/step_docker_test.go @@ -16,6 +16,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" ) func TestStepDockerMain(t *testing.T) { @@ -118,6 +119,43 @@ func TestStepDockerMain(t *testing.T) { cm.AssertExpectations(t) } +// With AutoRemove the daemon reaps the container on exit, so act must not remove it afterwards. +func TestStepDockerAutoRemove(t *testing.T) { + orig := ContainerNewContainer + defer func() { ContainerNewContainer = orig }() + + for _, tc := range []struct { + autoRemove bool + removes int + }{ + {false, 2}, // stale + post-run + {true, 1}, // post-run skipped + } { + cm := &containerMock{} + ContainerNewContainer = func(*container.NewContainerInput) container.ExecutionsEnvironment { return cm } + + sd := &stepDocker{ + RunContext: &RunContext{ + Config: &Config{AutoRemove: tc.autoRemove}, + Run: &model.Run{JobID: "1", Workflow: &model.Workflow{Jobs: map[string]*model.Job{"1": {}}}}, + JobContainer: cm, + }, + Step: &model.Step{ID: "1", Uses: "docker://node:14"}, + } + + removes := 0 + cm.On("Pull", false).Return(func(context.Context) error { return nil }) + cm.On("Remove").Return(func(context.Context) error { removes++; return nil }) + cm.On("Create", []string(nil), []string(nil)).Return(func(context.Context) error { return nil }) + cm.On("Start", true).Return(func(context.Context) error { return nil }) + cm.On("Close").Return(func(context.Context) error { return nil }) + + require.NoError(t, sd.runUsesContainer()(context.Background())) + cm.AssertExpectations(t) + assert.Equal(t, tc.removes, removes) + } +} + func TestStepDockerNewStepContainerAllocatePTY(t *testing.T) { for _, tc := range []struct { name string