fix: clean up application images when an upload fails

api/application.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"mime/multipart"
 	"net/http"
 	"os"
 	"path/filepath"
@@ -447,20 +448,25 @@ func (a *ApplicationAPI) UploadApplicationImage(ctx *gin.Context) {
 				return generateImageName() + ext
 			})
 
-			err = ctx.SaveUploadedFile(file, a.ImageDir+name)
+			err = saveImage(file, a.ImageDir+name)
 			if err != nil {
 				ctx.AbortWithError(500, err)
 				return
 			}
 
-			if app.Image != "" {
-				os.Remove(a.ImageDir + app.Image)
-			}
-
+			oldImage := app.Image
 			app.Image = name
-			if success := successOrAbort(ctx, 500, a.DB.UpdateApplication(app)); !success {
+			if err := a.DB.UpdateApplication(app); err != nil {
+				// The application still references the old image, so keep
+				// it and drop the new one that nothing references.
+				os.Remove(a.ImageDir + name)
+				ctx.AbortWithError(500, err)
 				return
 			}
+
+			if oldImage != "" {
+				os.Remove(a.ImageDir + oldImage)
+			}
 			ctx.JSON(200, withResolvedImage(app))
 		} else {
 			ctx.AbortWithError(404, fmt.Errorf("app with id %d doesn't exists", id))
@@ -542,6 +548,34 @@ func withResolvedImage(app *model.Application) *model.Application {
 	return app
 }
 
+// saveImage writes the uploaded file to dst. If it can't be written
+// completely, the partially written file is removed.
+func saveImage(file *multipart.FileHeader, dst string) error {
+	src, err := file.Open()
+	if err != nil {
+		return err
+	}
+	defer src.Close()
+	return writeImage(dst, src)
+}
+
+func writeImage(dst string, src io.Reader) (err error) {
+	out, err := os.Create(dst)
+	if err != nil {
+		return err
+	}
+	defer func() {
+		if closeErr := out.Close(); err == nil {
+			err = closeErr
+		}
+		if err != nil {
+			os.Remove(dst)
+		}
+	}()
+	_, err = io.Copy(out, src)
+	return err
+}
+
 func exist(path string) bool {
 	if _, err := os.Stat(path); os.IsNotExist(err) {
 		return false

api/application_test.go

@@ -483,6 +483,43 @@ func (s *ApplicationSuite) Test_UploadAppImage_WithImageFile_DeleteExstingImage(
 	assert.Len(s.T(), listing, 1)
 }
 
+func (s *ApplicationSuite) Test_UploadAppImage_UpdateFails_KeepsExistingImage() {
+	existingImageName := "existing.png"
+	s.db.User(5)
+	s.db.CreateApplication(&model.Application{UserID: 5, ID: 1, Image: existingImageName})
+	fakeImage(s.T(), s.imageDir.Path(existingImageName))
+	s.a.DB = &failingUpdateAppDB{ApplicationDatabase: s.db}
+
+	cType, buffer, err := upload(map[string]*os.File{"file": mustOpen("../test/assets/image.png")})
+	require.NoError(s.T(), err)
+	s.ctx.Request = httptest.NewRequest("POST", "/irrelevant", &buffer)
+	s.ctx.Request.Header.Set("Content-Type", cType)
+	test.WithUser(s.ctx, 5)
+	s.ctx.Params = gin.Params{{Key: "id", Value: "1"}}
+
+	s.a.UploadApplicationImage(s.ctx)
+
+	assert.Equal(s.T(), 500, s.recorder.Code)
+	app, err := s.db.GetApplicationByID(1)
+	require.NoError(s.T(), err)
+	assert.Equal(s.T(), existingImageName, app.Image)
+	listing, err := os.ReadDir(s.imageDir.Path())
+	require.NoError(s.T(), err)
+	require.Len(s.T(), listing, 1)
+	assert.Equal(s.T(), existingImageName, listing[0].Name())
+}
+
+func (s *ApplicationSuite) Test_WriteImage_RemovesPartialFileOnError() {
+	dst := s.imageDir.Path("partial.png")
+	src := io.MultiReader(strings.NewReader("partial image data"), errReader{errors.New("connection reset")})
+
+	err := writeImage(dst, src)
+
+	assert.EqualError(s.T(), err, "connection reset")
+	_, err = os.Stat(dst)
+	assert.True(s.T(), os.IsNotExist(err), "the partially written file must be removed")
+}
+
 func (s *ApplicationSuite) Test_UploadAppImage_WithTextFile_expectBadRequest() {
 	s.db.User(5).App(1)
 
@@ -762,3 +799,16 @@ func fakeImage(t *testing.T, path string) {
 	err = os.WriteFile(path, data, 0o644)
 	assert.Nil(t, err)
 }
+
+// failingUpdateAppDB fails every UpdateApplication call.
+type failingUpdateAppDB struct {
+	ApplicationDatabase
+}
+
+func (d *failingUpdateAppDB) UpdateApplication(*model.Application) error {
+	return errors.New("update failed")
+}
+
+type errReader struct{ err error }
+
+func (r errReader) Read([]byte) (int, error) { return 0, r.err }