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.
This commit is contained in:
kolaente
2026-07-26 23:09:45 +02:00
parent e005d6f242
commit fbf08ef196
2 changed files with 24 additions and 1 deletions
+16 -1
View File
@@ -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)
}
+8
View File
@@ -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