fix: answer 404 when a delete matched no row

CHANGELOG.md

@@ -43,6 +43,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   now goes through the same validation as a `POST`.
 - A `DELETE` of a missing software answered `404` but still recorded a
   delete event and notified the webhooks.
+- Two concurrent `DELETE` of the same software or publisher both
+  answered `204` and recorded two delete events.
 - The event recorded by an analysis `PATCH` had no actor.
 - `?from` and `?to` refused RFC 3339 values with a zone offset.
 - `?from` and `?to` were ignored on the per entity log lists and on the

internal/handlers/bundles.go

@@ -130,19 +130,13 @@ func (b *Bundle) PatchBundle(ctx *fiber.Ctx) error {
 func (b *Bundle) DeleteBundle(ctx *fiber.Ctx) error {
 	const errMsg = "can't delete Bundle"
 
-	bundle, err := findOne[models.Bundle](b.db, ctx.Params("id"), findOptions{
-		title: errMsg,
-		name:  bundleEntityName,
-	})
-	if err != nil {
-		return err
-	}
+	bundle := models.Bundle{ID: ctx.Params("id")}
 
 	// Select("Software") also removes the bundles_software join rows.
 	if err := models.Transaction(writeDB(ctx, b.db), func(tran *gorm.DB) error {
-		return tran.Select(softwareAssociation).Delete(bundle).Error
+		return deleteResult(tran.Select(softwareAssociation).Delete(&bundle))
 	}); err != nil {
-		return common.InternalServerError(errMsg)
+		return deleteError(err, errMsg, bundleEntityName)
 	}
 
 	return ctx.SendStatus(fiber.StatusNoContent)

internal/handlers/catalogs.go

@@ -262,9 +262,9 @@ func (c *Catalog) DeleteCatalog(ctx *fiber.Ctx) error {
 			return err
 		}
 
-		return tran.Delete(&catalog).Error
+		return deleteResult(tran.Delete(&catalog))
 	}); err != nil {
-		return common.InternalServerError(errMsg)
+		return deleteError(err, errMsg, "Catalog")
 	}
 
 	if conflictErr != nil {

internal/handlers/crud.go

@@ -280,6 +280,32 @@ func deleteEntityLogs(tran *gorm.DB, entity models.Model) error {
 		Delete(&models.Log{}).Error
 }
 
+// deleteResult reports gorm.ErrRecordNotFound when the delete matched no
+// row. gorm runs the AfterDelete hook either way, so a concurrent DELETE
+// of an id already gone would otherwise answer 204 and record a second
+// delete event.
+func deleteResult(result *gorm.DB) error {
+	if result.Error != nil {
+		return result.Error
+	}
+
+	if result.RowsAffected == 0 {
+		return gorm.ErrRecordNotFound
+	}
+
+	return nil
+}
+
+// deleteError maps the error of a delete to Problem JSON: 404 when the row
+// was already gone, 500 otherwise.
+func deleteError(err error, title, name string) error {
+	if errors.Is(err, gorm.ErrRecordNotFound) {
+		return common.Error(fiber.StatusNotFound, title, name+" was not found")
+	}
+
+	return common.InternalServerError(title)
+}
+
 // updateColumns writes the listed columns of entity and only those. A plain
 // struct update skips a nil pointer, so a merge patch nulling an optional
 // field would answer 200 and leave the old value in the row. Naming the

internal/handlers/publishers.go

@@ -68,10 +68,14 @@ func (p *Publisher) PatchPublisher(ctx *fiber.Ctx) error {
 	return updatePublisher(ctx, writeDB(ctx, p.db), *found)
 }
 
-// DeletePublisher deletes the publisher with the given ID.
+// DeletePublisher deletes the publisher with the given ID. The lookup
+// stays because the delete matches the UUID alone, while the id in the
+// path can be an alternative id.
 func (p *Publisher) DeletePublisher(ctx *fiber.Ctx) error {
+	const errMsg = "can't delete Publisher"
+
 	found, err := findOne[models.Publisher](p.db, ctx.Params("id"), findOptions{
-		title:           "can't delete Publisher",
+		title:           errMsg,
 		name:            publisherEntityName,
 		byAlternativeID: true,
 	})
@@ -86,9 +90,9 @@ func (p *Publisher) DeletePublisher(ctx *fiber.Ctx) error {
 			return err
 		}
 
-		return tran.Select(codeHostingAssociation).Delete(&publisher).Error
+		return deleteResult(tran.Select(codeHostingAssociation).Delete(&publisher))
 	}); err != nil {
-		return common.InternalServerError("can't delete Publisher")
+		return deleteError(err, errMsg, publisherEntityName)
 	}
 
 	return ctx.SendStatus(fiber.StatusNoContent)

internal/handlers/software.go

@@ -86,28 +86,20 @@ func (p *Software) PatchSoftware(ctx *fiber.Ctx) error {
 	return updateSoftware(ctx, writeDB(ctx, p.db), software)
 }
 
-// DeleteSoftware deletes the software with the given ID. The row is
-// looked up first: gorm runs the AfterDelete hook even when the delete
-// matched nothing, and the hook would record an event for a software
-// that never existed.
+// DeleteSoftware deletes the software with the given ID.
 func (p *Software) DeleteSoftware(ctx *fiber.Ctx) error {
 	const errMsg = "can't delete Software"
 
-	found, err := findOne[models.Software](p.db, ctx.Params("id"), findOptions{title: errMsg, name: softwareEntityName})
-	if err != nil {
-		return err
-	}
-
-	software := *found
+	software := models.Software{ID: ctx.Params("id")}
 
 	if err := models.Transaction(writeDB(ctx, p.db), func(tran *gorm.DB) error {
 		if err := deleteEntityLogs(tran, software); err != nil {
 			return err
 		}
 
-		return tran.Select("Aliases", "Bundles").Delete(&software).Error
+		return deleteResult(tran.Select("Aliases", "Bundles").Delete(&software))
 	}); err != nil {
-		return common.InternalServerError(errMsg)
+		return deleteError(err, errMsg, softwareEntityName)
 	}
 
 	return ctx.SendStatus(fiber.StatusNoContent)

internal/handlers/webhooks.go

@@ -138,21 +138,16 @@ func (p *Webhook[T]) PostSingleResourceWebhook(ctx *fiber.Ctx) error {
 	return ctx.JSON(&webhook)
 }
 
-// DeleteWebhook deletes the webhook with the given ID. The row is looked
-// up first, so that the AfterDelete hook records no event for a missing
-// id.
+// DeleteWebhook deletes the webhook with the given ID.
 func (p *Webhook[T]) DeleteWebhook(ctx *fiber.Ctx) error {
 	const errMsg = "can't delete Webhook"
 
-	webhook, err := findOne[models.Webhook](p.db, ctx.Params("id"), findOptions{title: errMsg, name: "Webhook"})
-	if err != nil {
-		return err
-	}
+	webhook := models.Webhook{ID: ctx.Params("id")}
 
 	if err := models.Transaction(writeDB(ctx, p.db), func(tran *gorm.DB) error {
-		return tran.Delete(webhook).Error
+		return deleteResult(tran.Delete(&webhook))
 	}); err != nil {
-		return common.InternalServerError(errMsg)
+		return deleteError(err, errMsg, "Webhook")
 	}
 
 	return ctx.SendStatus(fiber.StatusNoContent)

main_test.go

@@ -322,6 +322,33 @@ func dbNull(t *testing.T, table, column, whereCol, whereVal string) bool {
 	return val == nil
 }
 
+// addEntityLogs attaches count logs to the entity. Soft deleting them
+// makes the delete of the entity itself slow enough that a concurrent
+// DELETE of the same id still finds the row.
+func addEntityLogs(t *testing.T, entityType, entityID string, count int) {
+	t.Helper()
+
+	query := fmt.Sprintf(
+		"INSERT INTO logs (id, message, entity_id, entity_type, created_at, updated_at) "+
+			"VALUES (%s, %s, %s, %s, %s, %s)",
+		placeholder(1),
+		placeholder(2),
+		placeholder(3),
+		placeholder(4),
+		placeholder(5),
+		placeholder(6),
+	)
+
+	now := time.Now().UTC()
+
+	for i := range count {
+		id := fmt.Sprintf("33333333-3333-3333-3333-%012d", i)
+
+		_, err := db.Exec(query, id, "concurrent delete padding", entityID, entityType, now, now)
+		require.NoError(t, err)
+	}
+}
+
 // decodeJSON unmarshals a response body into a map, so a test can assert on
 // the keys it carries.
 func decodeJSON(t *testing.T, body []byte) map[string]any {

publishers_test.go

@@ -2,6 +2,7 @@ package main
 
 import (
 	"fmt"
+	"slices"
 	"strings"
 	"testing"
 	"time"
@@ -1220,6 +1221,54 @@ func TestPublishersDeleteDBChecks(t *testing.T) {
 		assert.False(t, dbNull(t, "logs", "deleted_at", "id", logID))
 		assert.Equal(t, 1, dbCount(t, "events", "entity_id", italiaPublisherID))
 	})
+
+	t.Run("concurrent DELETE of one publisher answers 404 once", func(t *testing.T) {
+		loadFixtures(t)
+
+		const (
+			publisherID = "0947ebca-ae8f-46e1-9cfe-2caccc87108b"
+			paddingLogs = 300
+		)
+
+		addEntityLogs(t, "publishers", publisherID, paddingLogs)
+
+		start := make(chan struct{})
+		statuses := make(chan int, 2)
+
+		for range 2 {
+			go func() {
+				req, err := newTestRequest("DELETE", "/v1/publishers/"+publisherID, nil)
+				if err != nil {
+					statuses <- 0
+
+					return
+				}
+
+				req.Header = map[string][]string{"Authorization": {goodToken}}
+
+				<-start
+
+				res, err := app.Test(req, -1)
+				if err != nil {
+					statuses <- 0
+
+					return
+				}
+
+				_ = res.Body.Close()
+
+				statuses <- res.StatusCode
+			}()
+		}
+
+		close(start)
+
+		statusCodes := []int{<-statuses, <-statuses}
+		slices.Sort(statusCodes)
+
+		assert.Equal(t, []int{204, 404}, statusCodes)
+		assert.Equal(t, 1, dbCount(t, "events", "entity_id", publisherID))
+	})
 }
 
 func addPublishersForPaginationCapTest(t *testing.T) {

software_test.go

@@ -3,6 +3,7 @@ package main
 import (
 	"encoding/json"
 	"fmt"
+	"slices"
 	"strings"
 	"sync"
 	"testing"
@@ -2159,6 +2160,54 @@ func TestSoftwareDeleteDBChecks(t *testing.T) {
 
 		assert.Equal(t, 0, dbCount(t, "events", "entity_id", softwareID))
 	})
+
+	t.Run("concurrent DELETE of one software answers 404 once", func(t *testing.T) {
+		loadFixtures(t)
+
+		const (
+			softwareID  = "11e101c4-f989-4cc4-a665-63f9f34e83f6"
+			paddingLogs = 300
+		)
+
+		addEntityLogs(t, "software", softwareID, paddingLogs)
+
+		start := make(chan struct{})
+		statuses := make(chan int, 2)
+
+		for range 2 {
+			go func() {
+				req, err := newTestRequest("DELETE", "/v1/software/"+softwareID, nil)
+				if err != nil {
+					statuses <- 0
+
+					return
+				}
+
+				req.Header = map[string][]string{"Authorization": {goodToken}}
+
+				<-start
+
+				res, err := app.Test(req, -1)
+				if err != nil {
+					statuses <- 0
+
+					return
+				}
+
+				_ = res.Body.Close()
+
+				statuses <- res.StatusCode
+			}()
+		}
+
+		close(start)
+
+		statusCodes := []int{<-statuses, <-statuses}
+		slices.Sort(statusCodes)
+
+		assert.Equal(t, []int{204, 404}, statusCodes)
+		assert.Equal(t, 1, dbCount(t, "events", "entity_id", softwareID))
+	})
 }
 
 func addSoftwareForPaginationCapTest(t *testing.T) {