From fbf08ef196af8727252ddc4e173e3996fb82dd76 Mon Sep 17 00:00:00 2001 From: kolaente Date: Sun, 26 Jul 2026 23:09:45 +0200 Subject: [PATCH] fix: lock the view before placing a batch at the top of it Two concurrent batches read the same lowest position and computed byte-identical slots, which is the arbitrary ordering this branch set out to remove. Take the same lock the single-task path takes, walk projects and views in a stable id order so two batches cannot deadlock, and run the conflict repair after inserting. Address pr-swarm finding: unserialized position read in the batch path. --- pkg/models/bulk_task_create.go | 17 ++++++++++++++++- pkg/models/task_position.go | 8 ++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/pkg/models/bulk_task_create.go b/pkg/models/bulk_task_create.go index de809385a..e48cf5917 100644 --- a/pkg/models/bulk_task_create.go +++ b/pkg/models/bulk_task_create.go @@ -17,6 +17,9 @@ package models import ( + "cmp" + "slices" + "code.vikunja.io/api/pkg/web" "xorm.io/xorm" @@ -70,11 +73,18 @@ func (bt *BulkTaskCreate) Create(s *xorm.Session, a web.Auth) (err error) { viewsByProject := map[int64][]*ProjectView{} lowestByView := map[int64]float64{} + // makeRoomAtTopOfView locks each view it touches, so walk projects and views in a + // stable id order: two batches sharing a project would otherwise be free to grab the + // same locks in opposite orders and deadlock. + slices.Sort(projectIDs) for _, projectID := range projectIDs { views, err := getViewsForProject(s, projectID) if err != nil { return err } + slices.SortFunc(views, func(a, b *ProjectView) int { + return cmp.Compare(a.ID, b.ID) + }) viewsByProject[projectID] = views var needSlot int @@ -114,5 +124,10 @@ func (bt *BulkTaskCreate) Create(s *xorm.Session, a web.Auth) (err error) { } } - return bulkInsertTaskPositions(s, positions, false) + err = bulkInsertTaskPositions(s, positions, false) + if err != nil { + return err + } + + return resolvePositionConflictsAfterInsert(s, positions) } diff --git a/pkg/models/task_position.go b/pkg/models/task_position.go index e10e68e57..4752aec0a 100644 --- a/pkg/models/task_position.go +++ b/pkg/models/task_position.go @@ -462,6 +462,14 @@ func calculateNewPositionForTask(s *xorm.Session, a web.Auth, t *Task, view *Pro // This has to run before the new tasks are inserted: a recalculation orders by position, // which they do not have yet, so they would land in an arbitrary order. func makeRoomAtTopOfView(s *xorm.Session, view *ProjectView, count int, a web.Auth) (lowest float64, err error) { + // Without the lock two concurrent batches read the same lowest position and compute + // byte-identical slots for their tasks. Callers must take the locks in a stable view + // order to avoid deadlocking against each other. + err = lockPositionsForViewUpdate(s, view.ID) + if err != nil { + return 0, err + } + lowest, has, err := getLowestPositionInView(s, view.ID) if err != nil || !has || count == 0 { return 0, err