diff --git a/classes/undo.go b/classes/undo.go index fab4641..716edfc 100644 --- a/classes/undo.go +++ b/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 diff --git a/redirects_test.go b/redirects_test.go index 79a4b9a..a0e138d 100644 --- a/redirects_test.go +++ b/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) + } +}