From bf175dde6db559ac4e4ae374d69eafdd4471c87f Mon Sep 17 00:00:00 2001 From: kolaente Date: Fri, 19 Jun 2026 10:09:00 +0200 Subject: [PATCH] fix(kanban): upsert race condition in kanban task bucket sync (#2938) --- pkg/models/kanban_task_bucket.go | 38 +++++++----------- pkg/models/kanban_task_bucket_test.go | 57 +++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 23 deletions(-) diff --git a/pkg/models/kanban_task_bucket.go b/pkg/models/kanban_task_bucket.go index ae6757b30..cd58e6928 100644 --- a/pkg/models/kanban_task_bucket.go +++ b/pkg/models/kanban_task_bucket.go @@ -22,7 +22,9 @@ import ( "code.vikunja.io/api/pkg/db" "code.vikunja.io/api/pkg/events" "code.vikunja.io/api/pkg/web" + "xorm.io/xorm" + "xorm.io/xorm/schemas" ) // TaskBucket represents the relation between a task and a kanban bucket. @@ -58,27 +60,19 @@ func (b *TaskBucket) CanUpdate(s *xorm.Session, a web.Auth) (bool, error) { } func (b *TaskBucket) upsert(s *xorm.Session) (err error) { - count, err := s.Where("task_id = ? AND project_view_id = ?", b.TaskID, b.ProjectViewID). - Cols("bucket_id"). - Update(b) - if err != nil { - return - } - - if count == 0 { - _, err = s.Insert(b) - if err != nil { - // Check if this is a unique constraint violation for the task_buckets table - if db.IsUniqueConstraintError(err, "UQE_task_buckets_task_project_view") { - return ErrTaskAlreadyExistsInBucket{ - TaskID: b.TaskID, - ProjectViewID: b.ProjectViewID, - } - } - return - } + // A native upsert moves the task in one atomic statement, without + // depending on the affected-row count (MySQL/MariaDB report 0 affected + // rows for an unchanged value). + onConflict := "ON CONFLICT (task_id, project_view_id) DO UPDATE SET bucket_id = excluded.bucket_id" + if db.Type() == schemas.MYSQL { + onConflict = "ON DUPLICATE KEY UPDATE bucket_id = VALUES(bucket_id)" } + // Raw SQL bypasses xorm's bean-based table-name handling, so qualify the + // table ourselves to honor a configured postgres schema (database.schema). + table := s.Engine().TableName(b, true) + query := "INSERT INTO " + table + " (task_id, project_view_id, bucket_id) VALUES (?, ?, ?) " + onConflict + _, err = s.Exec(query, b.TaskID, b.ProjectViewID, b.BucketID) return } @@ -151,10 +145,8 @@ func updateTaskBucket(s *xorm.Session, a web.Auth, b *TaskBucket) (err error) { if err != nil { return err } - // If the task is already in the default bucket, skip the - // upsert — MySQL's UPDATE returns 0 affected rows when - // the value is unchanged, which would make upsert fall - // through to INSERT and hit the unique constraint. + // The task is already in the default bucket, so there is + // nothing to move and no count to bump. if b.BucketID == oldTaskBucket.BucketID { updateBucket = false } diff --git a/pkg/models/kanban_task_bucket_test.go b/pkg/models/kanban_task_bucket_test.go index 7bde2ade4..bf5c42ac3 100644 --- a/pkg/models/kanban_task_bucket_test.go +++ b/pkg/models/kanban_task_bucket_test.go @@ -226,6 +226,63 @@ func TestTaskBucket_Update(t *testing.T) { }) }) + t.Run("done task already in another view's done bucket", func(t *testing.T) { + // Regression test: marking a task done syncs it into the done bucket + // of every kanban view in the project. When the task already sits in + // such a view's done bucket the sync is a no-op update, but on + // MySQL/MariaDB an UPDATE that doesn't change the value reports 0 + // affected rows. The upsert then mistook that for "row missing" and + // inserted, hitting the unique index with ErrTaskAlreadyExistsInBucket. + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + // A second manual kanban view on project 1. Creating it auto-generates + // the To-Do/Doing/Done buckets and sets its done bucket. + secondView := &ProjectView{ + Title: "Second Kanban", + ProjectID: 1, + ViewKind: ProjectViewKindKanban, + BucketConfigurationMode: BucketConfigurationModeManual, + } + err := secondView.Create(s, u) + require.NoError(t, err) + require.NotZero(t, secondView.DoneBucketID) + + // Pre-place task 1 in the second view's done bucket without going + // through the done-sync, so the task itself is still open and view 4 + // still has it in its default bucket. + _, err = s.Where("task_id = ? AND project_view_id = ?", 1, secondView.ID). + Cols("bucket_id"). + Update(&TaskBucket{BucketID: secondView.DoneBucketID}) + require.NoError(t, err) + + // Moving task 1 into view 4's done bucket marks it done and triggers + // the cross-view sync into the second view's done bucket, where it + // already lives. This must succeed rather than error. + tb := &TaskBucket{ + TaskID: 1, + BucketID: 3, // done bucket on view 4 + ProjectViewID: 4, + ProjectID: 1, + } + err = tb.Update(s, u) + require.NoError(t, err) + err = s.Commit() + require.NoError(t, err) + assert.True(t, tb.Task.Done) + + db.AssertExists(t, "task_buckets", map[string]interface{}{ + "task_id": 1, + "bucket_id": 3, + }, false) + db.AssertExists(t, "task_buckets", map[string]interface{}{ + "task_id": 1, + "project_view_id": secondView.ID, + "bucket_id": secondView.DoneBucketID, + }, false) + }) + t.Run("saved filter: first task into empty limited bucket is allowed", func(t *testing.T) { // Regression test for #2672: on a saved-filter kanban view the bucket // limit was checked against the total number of tasks matching the