From a44d4a264758071082a1da2d839ba3d46c240c27 Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 23 Sep 2026 09:06:48 +0000 Subject: [PATCH 1/2] Remove the git clone container's anonymous volume with it (closes #215) The pinned alpine/git image declares a volume at /git, so Docker gives every clone container an anonymous volume. The container was removed without its volumes, leaving one volume behind per deploy. The removal now also removes the container's anonymous volumes, on success and on failure. A test runs a clone against a fake Docker API and checks the removal asks for volumes to be removed. Model: opus-5-5 --- TODO.md | 3 ++ internal/docker/client.go | 4 +- internal/docker/validation_test.go | 72 ++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 1 deletion(-) diff --git a/TODO.md b/TODO.md index 72043db..27bd714 100644 --- a/TODO.md +++ b/TODO.md @@ -20,6 +20,9 @@ regress. # Completed Steps +- 2026-09-23: The git clone container is now removed together with its anonymous + volume (the `alpine/git` image declares one at `/git`), so a deploy no longer + leaves a Docker volume behind (#215). - 2026-09-23: Deployment log files are now stored under `logs//` instead of `logs///`, so downloads keep working after the upaas container is recreated; logs written under an old hostname directory are diff --git a/internal/docker/client.go b/internal/docker/client.go index 168f3fa..eb56a73 100644 --- a/internal/docker/client.go +++ b/internal/docker/client.go @@ -656,11 +656,13 @@ func (c *Client) performClone( return nil, err } + // The git image declares a volume, so Docker gives each clone container + // an anonymous volume; remove it with the container. defer func() { _ = c.docker.ContainerRemove( ctx, gitContainerID.String(), - container.RemoveOptions{Force: true}, + container.RemoveOptions{Force: true, RemoveVolumes: true}, ) }() diff --git a/internal/docker/validation_test.go b/internal/docker/validation_test.go index 2d033c8..9a57b82 100644 --- a/internal/docker/validation_test.go +++ b/internal/docker/validation_test.go @@ -2,8 +2,16 @@ package docker //nolint:testpackage // tests unexported regexps and Client struc import ( "errors" + "fmt" "log/slog" + "net/http" + "net/http/httptest" + "net/url" + "path/filepath" + "strings" "testing" + + "github.com/docker/docker/client" ) // mainBranch is the branch name used across validation tests. @@ -149,3 +157,67 @@ func TestCloneRepoRejectsInjection(t *testing.T) { }) } } + +// TestPerformCloneRemovesContainerVolumes runs a clone against a fake Docker +// API and checks that the clone container is removed together with its +// anonymous volumes, whether the clone succeeds or fails. +func TestPerformCloneRemovesContainerVolumes(t *testing.T) { + t.Parallel() + + for _, exitCode := range []int{0, 1} { + t.Run(fmt.Sprintf("exit code %d", exitCode), func(t *testing.T) { + t.Parallel() + + removeQuery := make(chan url.Values, 1) + + srv := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + + switch { + case r.Method == http.MethodDelete: + removeQuery <- r.URL.Query() + case strings.HasSuffix(r.URL.Path, "/containers/create"): + _, _ = w.Write([]byte(`{"Id":"gitcontainer"}`)) + case strings.HasSuffix(r.URL.Path, "/wait"): + _, _ = fmt.Fprintf(w, `{"StatusCode":%d}`, exitCode) + default: + _, _ = w.Write([]byte(`{}`)) + } + }, + )) + t.Cleanup(srv.Close) + + dockerAPI, err := client.NewClientWithOpts( + client.WithHost("tcp://" + srv.Listener.Addr().String()), + ) + if err != nil { + t.Fatal(err) + } + + c := &Client{docker: dockerAPI, log: slog.Default()} + + dir := t.TempDir() + cfg := &cloneConfig{ + repoURL: "git@example.com:repo.git", + branch: mainBranch, + sshPrivateKey: "fake-key", + containerDir: filepath.Join(dir, "repo"), + hostDir: filepath.Join(dir, "repo"), + keyFile: filepath.Join(dir, "deploy_key"), + hostKeyFile: filepath.Join(dir, "deploy_key"), + } + + _, _ = c.performClone(t.Context(), cfg) + + select { + case query := <-removeQuery: + if query.Get("v") != "1" { + t.Errorf("clone container removed without its volumes: %v", query) + } + default: + t.Error("clone container was not removed") + } + }) + } +} -- 2.54.0 From 3e64d6f087be0e4a50e9cc18f78523d13c61c9e1 Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 23 Sep 2026 09:28:11 +0000 Subject: [PATCH 2/2] Remove the clone container even when the deploy is cancelled (closes #215) The deferred removal used the deploy context, so a cancelled deploy (a newer push, or the build timeout) never sent the remove request and left the clone container and its volume behind. It now uses context.WithoutCancel, and the test gains a case that cancels the context while the clone is running. Model: opus-5-5 --- internal/docker/client.go | 5 +++-- internal/docker/validation_test.go | 28 +++++++++++++++++++++++----- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/internal/docker/client.go b/internal/docker/client.go index eb56a73..9e6abc5 100644 --- a/internal/docker/client.go +++ b/internal/docker/client.go @@ -657,10 +657,11 @@ func (c *Client) performClone( } // The git image declares a volume, so Docker gives each clone container - // an anonymous volume; remove it with the container. + // an anonymous volume; remove it with the container. The removal must + // still run when the deploy is cancelled. defer func() { _ = c.docker.ContainerRemove( - ctx, + context.WithoutCancel(ctx), gitContainerID.String(), container.RemoveOptions{Force: true, RemoveVolumes: true}, ) diff --git a/internal/docker/validation_test.go b/internal/docker/validation_test.go index 9a57b82..3ea3378 100644 --- a/internal/docker/validation_test.go +++ b/internal/docker/validation_test.go @@ -1,6 +1,7 @@ package docker //nolint:testpackage // tests unexported regexps and Client struct import ( + "context" "errors" "fmt" "log/slog" @@ -160,14 +161,27 @@ func TestCloneRepoRejectsInjection(t *testing.T) { // TestPerformCloneRemovesContainerVolumes runs a clone against a fake Docker // API and checks that the clone container is removed together with its -// anonymous volumes, whether the clone succeeds or fails. +// anonymous volumes, whether the clone succeeds, fails, or is cancelled. func TestPerformCloneRemovesContainerVolumes(t *testing.T) { t.Parallel() - for _, exitCode := range []int{0, 1} { - t.Run(fmt.Sprintf("exit code %d", exitCode), func(t *testing.T) { + tests := []struct { + name string + exitCode int + cancel bool + }{ + {name: "succeeds", exitCode: 0}, + {name: "fails", exitCode: 1}, + {name: "cancelled", cancel: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { t.Parallel() + ctx, cancel := context.WithCancel(t.Context()) + t.Cleanup(cancel) + removeQuery := make(chan url.Values, 1) srv := httptest.NewServer(http.HandlerFunc( @@ -179,8 +193,12 @@ func TestPerformCloneRemovesContainerVolumes(t *testing.T) { removeQuery <- r.URL.Query() case strings.HasSuffix(r.URL.Path, "/containers/create"): _, _ = w.Write([]byte(`{"Id":"gitcontainer"}`)) + case strings.HasSuffix(r.URL.Path, "/wait") && tt.cancel: + // Cancel the deploy while the clone is running. + cancel() + <-r.Context().Done() case strings.HasSuffix(r.URL.Path, "/wait"): - _, _ = fmt.Fprintf(w, `{"StatusCode":%d}`, exitCode) + _, _ = fmt.Fprintf(w, `{"StatusCode":%d}`, tt.exitCode) default: _, _ = w.Write([]byte(`{}`)) } @@ -208,7 +226,7 @@ func TestPerformCloneRemovesContainerVolumes(t *testing.T) { hostKeyFile: filepath.Join(dir, "deploy_key"), } - _, _ = c.performClone(t.Context(), cfg) + _, _ = c.performClone(ctx, cfg) select { case query := <-removeQuery: -- 2.54.0