fix: undo locks its history entries and marks them only once (WR-11)

Two concurrent undos of one entry both passed the already-undone check and
both restored. Undo and UndoBatch now read the entries FOR UPDATE, and the
final mark is conditional on undone_at IS NULL, refusing with
already_undone when another undo got there first.

classes/undo.go

@@ -15,6 +15,7 @@ import (
 	"git.golem15.com/golem15/summercms/modules/bouncer"
 	"git.golem15.com/golem15/summercms/modules/pact"
 	"gorm.io/gorm"
+	"gorm.io/gorm/clause"
 )
 
 // The undo refusals (D-13, T-32-27).
@@ -68,8 +69,11 @@ func Undoable(r models.Revision) bool {
 // later change touched the same values (undo_stale, naming who and when),
 // when the restore would create a redirect loop (undo_loop) or when the
 // record it belongs to is gone (undo_gone).
+//
+// The entry is locked (FOR UPDATE) until tx ends, so a second undo of the
+// same entry waits and is then refused as already undone (WR-11).
 func (s *Service) Undo(ctx context.Context, tx *gorm.DB, id uint) ([]uint, error) {
-	rev, err := LoadRevision(tx, id)
+	rev, err := LoadRevision(tx.Clauses(clause.Locking{Strength: "UPDATE"}), id)
 	if err != nil {
 		return nil, err
 	}
@@ -87,7 +91,7 @@ func (s *Service) Undo(ctx context.Context, tx *gorm.DB, id uint) ([]uint, error
 // has nothing left to undo.
 func (s *Service) UndoBatch(ctx context.Context, tx *gorm.DB, batch string) ([]uint, error) {
 	var revs []models.Revision
-	if err := tx.Where("batch_id = ? AND undone_at IS NULL", batch).Order("id DESC").Find(&revs).Error; err != nil {
+	if err := tx.Clauses(clause.Locking{Strength: "UPDATE"}).Where("batch_id = ? AND undone_at IS NULL", batch).Order("id DESC").Find(&revs).Error; err != nil {
 		return nil, err
 	}
 	if len(revs) == 0 {
@@ -339,7 +343,14 @@ func (s *Service) undoOne(ctx context.Context, tx *gorm.DB, rev models.Revision)
 	} else if p, ok := mcpPrincipal(ctx); ok && p.ID != 0 {
 		updates["undone_by_id"] = p.ID
 	}
-	return tx.Model(&models.Revision{}).Where("id = ?", rev.ID).Updates(updates).Error
+	marked := tx.Model(&models.Revision{}).Where("id = ? AND undone_at IS NULL", rev.ID).Updates(updates)
+	if marked.Error != nil {
+		return marked.Error
+	}
+	if marked.RowsAffected == 0 {
+		return &UndoRefusal{Code: UndoDone, Key: "golem15.seo::lang.history.undo_done"}
+	}
+	return nil
 }
 
 // kindLabels are the phrase keys of the history subjects in the undo_gone

redirects_test.go

@@ -817,3 +817,60 @@ func TestAddressChangeFromEvent(t *testing.T) {
 		t.Fatalf("redirects after the admin save = %+v, want both old addresses on /pages/delta", rows)
 	}
 }
+
+// TestConcurrentUndo checks that undo is idempotent under concurrent
+// requests (WR-11): while one transaction holds the undo of a deleted
+// redirect uncommitted, a second undo of the same entry waits, then is
+// refused as already undone; the redirect is re-created once.
+func TestConcurrentUndo(t *testing.T) {
+	env := bootEnv(t)
+	svc := env.seoPlugin(t).Service()
+	r := models.NewRedirect()
+	r.Source, r.Target = "/undo-twice", "/x"
+	if err := env.db.Transaction(func(tx *gorm.DB) error {
+		if _, err := svc.SaveRedirect(t.Context(), tx, &r); err != nil {
+			return err
+		}
+		return svc.DeleteRedirect(t.Context(), tx, r)
+	}); err != nil {
+		t.Fatal(err)
+	}
+	var revID uint
+	if err := env.db.Raw(`SELECT id FROM golem15_seo_revisions WHERE subject_kind = 'redirect' ORDER BY id DESC LIMIT 1`).Scan(&revID).Error; err != nil {
+		t.Fatal(err)
+	}
+
+	first := env.db.Begin()
+	if _, err := svc.Undo(t.Context(), first, revID); err != nil {
+		first.Rollback()
+		t.Fatal(err)
+	}
+	done := make(chan error, 1)
+	go func() {
+		done <- env.db.Transaction(func(tx *gorm.DB) error {
+			_, err := svc.Undo(context.Background(), tx, revID)
+			return err
+		})
+	}()
+	select {
+	case err := <-done:
+		first.Rollback()
+		t.Fatalf("second undo finished while the first was uncommitted: %v", err)
+	case <-time.After(300 * time.Millisecond):
+	}
+	if err := first.Commit().Error; err != nil {
+		t.Fatal(err)
+	}
+	var refusal *classes.UndoRefusal
+	if err := <-done; !errors.As(err, &refusal) || refusal.Code != classes.UndoDone {
+		t.Fatalf("second undo: err = %v, want already_undone", err)
+	}
+	var n int64
+	if err := env.db.Raw(`SELECT COUNT(*) FROM golem15_seo_redirects WHERE source = '/undo-twice'`).Scan(&n).Error; err != nil || n != 1 {
+		t.Fatalf("redirects of /undo-twice = %d (err %v), want 1", n, err)
+	}
+	var undos int64
+	if err := env.db.Raw(`SELECT COUNT(*) FROM golem15_seo_revisions WHERE via = 'undo'`).Scan(&undos).Error; err != nil || undos != 1 {
+		t.Fatalf("undo history rows = %d (err %v), want 1", undos, err)
+	}
+}