internal/docker/client.go

@@ -537,12 +537,13 @@ func (c *Client) RemoveImage(ctx context.Context, imageID ImageID) error {
 	return nil
 }
 
-// ListImageIDs returns the IDs of all images tagged in the given repository,
-// such as "upaas-myapp".
-func (c *Client) ListImageIDs(
+// ListImageTags returns the tags in the given repository, such as
+// "upaas-myapp:12" in "upaas-myapp", each with the ID of its image.
+// Tags the same image has in other repositories are left out.
+func (c *Client) ListImageTags(
 	ctx context.Context,
 	repository string,
-) ([]ImageID, error) {
+) (map[string]ImageID, error) {
 	if c.docker == nil {
 		return nil, ErrNotConnected
 	}
@@ -554,12 +555,35 @@ func (c *Client) ListImageIDs(
 		return nil, fmt.Errorf("failed to list images: %w", err)
 	}
 
-	ids := make([]ImageID, 0, len(images))
+	tags := make(map[string]ImageID)
+
 	for _, img := range images {
-		ids = append(ids, ImageID(img.ID))
+		for _, tag := range img.RepoTags {
+			if strings.HasPrefix(tag, repository+":") {
+				tags[tag] = ImageID(img.ID)
+			}
+		}
 	}
 
-	return ids, nil
+	return tags, nil
+}
+
+// RemoveImageTag removes a tag such as "upaas-myapp:12", without force.
+// Docker then deletes the image, and the untagged images it was built on,
+// only if no other tag and no container still uses it.
+func (c *Client) RemoveImageTag(ctx context.Context, tag string) error {
+	if c.docker == nil {
+		return ErrNotConnected
+	}
+
+	_, err := c.docker.ImageRemove(ctx, tag, image.RemoveOptions{
+		PruneChildren: true,
+	})
+	if err != nil && !client.IsErrNotFound(err) {
+		return fmt.Errorf("failed to remove image tag %s: %w", tag, err)
+	}
+
+	return nil
 }
 
 func (c *Client) performBuild(

internal/service/deploy/deploy.go

@@ -9,8 +9,10 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
+	"maps"
 	"os"
 	"path/filepath"
+	"slices"
 	"strings"
 	"sync"
 	"time"
@@ -524,18 +526,11 @@ func (svc *Service) runBuildAndDeploy(
 		return err
 	}
 
-	// Save current image as previous before updating to new one
-	if app.ImageID.Valid && app.ImageID.String != "" {
-		app.PreviousImageID = app.ImageID
-	}
-
-	err = svc.updateAppRunning(bgCtx, app, imageID)
+	err = svc.recordDeployedImage(bgCtx, app, deployment, imageID)
 	if err != nil {
 		return err
 	}
 
-	svc.removeUnusedImages(bgCtx, app, deployment)
-
 	// Use context.WithoutCancel to ensure health check completes even if
 	// the parent context is cancelled (e.g., HTTP request ends).
 	go svc.checkHealthAfterDelay(bgCtx, app, deployment)
@@ -716,57 +711,68 @@ func (svc *Service) checkCancelled(
 	return ErrDeployCancelled
 }
 
-// removeUnusedImages removes the app's images (tagged upaas-<app>:<deployment>
-// by buildImage) except the one the running container uses and the one
-// Rollback would start. Removing an image also removes the untagged images
-// it was built on, unless another image still needs them.
+// recordDeployedImage runs once the new container has started: it makes
+// imageID the app's current image, keeps the replaced one as the previous
+// image for Rollback, and then removes the app's other images.
+func (svc *Service) recordDeployedImage(
+	ctx context.Context,
+	app *models.App,
+	deployment *models.Deployment,
+	imageID docker.ImageID,
+) error {
+	// Save current image as previous before updating to new one
+	if app.ImageID.Valid && app.ImageID.String != "" {
+		app.PreviousImageID = app.ImageID
+	}
+
+	err := svc.updateAppRunning(ctx, app, imageID)
+	if err != nil {
+		return err
+	}
+
+	svc.removeUnusedImages(ctx, app, deployment)
+
+	return nil
+}
+
+// removeUnusedImages removes the app's tags (upaas-<app>:<deployment>, set by
+// buildImage) except those of the image the running container uses and the
+// one Rollback would start. Docker deletes an image only once no other tag,
+// such as another app's, and no container still uses it.
 func (svc *Service) removeUnusedImages(
 	ctx context.Context,
 	app *models.App,
 	deployment *models.Deployment,
 ) {
-	images, err := svc.docker.ListImageIDs(ctx, "upaas-"+app.Name)
+	tags, err := svc.docker.ListImageTags(ctx, "upaas-"+app.Name)
 	if err != nil {
 		svc.log.Error("failed to list app images", "error", err, "app", app.Name)
 
 		return
 	}
 
-	for _, imageID := range unusedImages(images, app) {
-		removeErr := svc.docker.RemoveImage(ctx, imageID)
+	for _, tag := range slices.Sorted(maps.Keys(tags)) {
+		imageID := tags[tag].String()
+		if imageID == app.ImageID.String || imageID == app.PreviousImageID.String {
+			continue
+		}
+
+		removeErr := svc.docker.RemoveImageTag(ctx, tag)
 		if removeErr != nil {
 			svc.log.Error("failed to remove old image",
-				"error", removeErr, "app", app.Name, "image", imageID)
+				"error", removeErr, "app", app.Name, "tag", tag)
 			_ = deployment.AppendLog(
 				ctx,
-				"WARNING: failed to remove old image "+
-					imageID.String()+": "+removeErr.Error(),
+				"WARNING: failed to remove old image "+tag+": "+removeErr.Error(),
 			)
 
 			continue
 		}
 
-		_ = deployment.AppendLog(ctx, "Removed old image: "+imageID.String())
+		_ = deployment.AppendLog(ctx, "Removed old image: "+tag)
 	}
 }
 
-// unusedImages returns the images that are neither the app's current image
-// nor its previous image, which Rollback uses.
-func unusedImages(images []docker.ImageID, app *models.App) []docker.ImageID {
-	var unused []docker.ImageID
-
-	for _, imageID := range images {
-		if imageID.String() == app.ImageID.String ||
-			imageID.String() == app.PreviousImageID.String {
-			continue
-		}
-
-		unused = append(unused, imageID)
-	}
-
-	return unused
-}
-
 // cleanupCancelledDeploy removes orphan resources left by a cancelled deployment.
 func (svc *Service) cleanupCancelledDeploy(
 	ctx context.Context,

internal/service/deploy/deploy_images_test.go

@@ -1,48 +1,106 @@
 package deploy_test
 
 import (
+	"context"
 	"database/sql"
+	"log/slog"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"strings"
+	"sync"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/fx/fxtest"
 
+	"sneak.berlin/go/upaas/internal/config"
+	"sneak.berlin/go/upaas/internal/database"
 	"sneak.berlin/go/upaas/internal/docker"
+	"sneak.berlin/go/upaas/internal/logger"
 	"sneak.berlin/go/upaas/internal/models"
 	"sneak.berlin/go/upaas/internal/service/deploy"
 )
 
-const currentImage = "sha256:current"
-
-func TestUnusedImages_KeepsCurrentAndRollbackImage(t *testing.T) {
+// TestRecordDeployedImageRemovesOldImages runs the step after a deploy
+// against a fake Docker API. Image one is also tagged for another app,
+// image two was the previous image, three the current one, four is new.
+func TestRecordDeployedImageRemovesOldImages(t *testing.T) {
 	t.Parallel()
 
-	app := &models.App{
-		ImageID:         sql.NullString{String: currentImage, Valid: true},
-		PreviousImageID: sql.NullString{String: "sha256:previous", Valid: true},
-	}
-	images := []docker.ImageID{
-		"sha256:oldest",
-		"sha256:previous",
-		"sha256:older",
-		currentImage,
-	}
-
-	assert.Equal(t,
-		[]docker.ImageID{"sha256:oldest", "sha256:older"},
-		deploy.UnusedImages(images, app),
+	var (
+		mu      sync.Mutex
+		removed []string
+		forced  bool
 	)
-}
 
-func TestUnusedImages_FirstDeployHasNoRollbackImage(t *testing.T) {
-	t.Parallel()
+	srv := httptest.NewServer(http.HandlerFunc(
+		func(w http.ResponseWriter, r *http.Request) {
+			w.Header().Set("Content-Type", "application/json")
 
-	app := &models.App{
-		ImageID: sql.NullString{String: currentImage, Valid: true},
+			switch {
+			case r.Method == http.MethodDelete:
+				_, name, _ := strings.Cut(r.URL.Path, "/images/")
+
+				mu.Lock()
+
+				removed = append(removed, name)
+				forced = forced || r.URL.Query().Get("force") != ""
+				mu.Unlock()
+
+				_, _ = w.Write([]byte(`[]`))
+			case strings.HasSuffix(r.URL.Path, "/images/json"):
+				_, _ = w.Write([]byte(`[
+					{"Id":"sha256:one","RepoTags":["upaas-myapp:1","upaas-otherapp:7"]},
+					{"Id":"sha256:two","RepoTags":["upaas-myapp:2"]},
+					{"Id":"sha256:three","RepoTags":["upaas-myapp:3"]},
+					{"Id":"sha256:four","RepoTags":["upaas-myapp:4"]}
+				]`))
+			default:
+				_, _ = w.Write([]byte(`{}`))
 			}
-	images := []docker.ImageID{currentImage, "sha256:failed"}
+		},
+	))
+	t.Cleanup(srv.Close)
 
-	assert.Equal(t,
-		[]docker.ImageID{"sha256:failed"},
-		deploy.UnusedImages(images, app),
-	)
+	log := slog.New(slog.NewTextHandler(os.Stderr, nil))
+	lifecycle := fxtest.NewLifecycle(t)
+
+	dockerClient, err := docker.New(lifecycle, docker.Params{
+		Logger: logger.NewForTest(log),
+		Config: &config.Config{DockerHost: "tcp://" + srv.Listener.Addr().String()},
+	})
+	require.NoError(t, err)
+
+	lifecycle.RequireStart()
+	t.Cleanup(lifecycle.RequireStop)
+
+	db := database.NewTestDatabase(t)
+	ctx := context.Background()
+
+	app := models.NewApp(db)
+	app.ID = "myapp-id"
+	app.Name = "myapp"
+	app.ImageID = sql.NullString{String: "sha256:three", Valid: true}
+	app.PreviousImageID = sql.NullString{String: "sha256:two", Valid: true}
+	require.NoError(t, app.Save(ctx))
+
+	deployment := models.NewDeployment(db)
+	deployment.AppID = app.ID
+	require.NoError(t, deployment.Save(ctx))
+
+	svc := deploy.NewTestServiceWithConfig(log, &config.Config{}, dockerClient)
+
+	err = svc.RecordDeployedImage(ctx, app, deployment, "sha256:four")
+	require.NoError(t, err)
+
+	assert.Equal(t, "sha256:four", app.ImageID.String)
+	assert.Equal(t, "sha256:three", app.PreviousImageID.String)
+
+	mu.Lock()
+	defer mu.Unlock()
+
+	assert.Equal(t, []string{"upaas-myapp:1", "upaas-myapp:2"}, removed)
+	assert.False(t, forced, "old image tags must be removed without force")
 }

internal/service/deploy/export_test.go

@@ -90,9 +90,14 @@ func (svc *Service) GetBuildDirExported(appName string) string {
 	return svc.GetBuildDir(appName)
 }
 
-// UnusedImages exposes unusedImages for testing.
-func UnusedImages(images []docker.ImageID, app *models.App) []docker.ImageID {
-	return unusedImages(images, app)
+// RecordDeployedImage exposes recordDeployedImage for testing.
+func (svc *Service) RecordDeployedImage(
+	ctx context.Context,
+	app *models.App,
+	deployment *models.Deployment,
+	imageID docker.ImageID,
+) error {
+	return svc.recordDeployedImage(ctx, app, deployment, imageID)
 }
 
 // BuildContainerOptionsExported exposes buildContainerOptions for testing.