From 49bba7f830ba3f82bad8e2dc57d54495772e9be6 Mon Sep 17 00:00:00 2001 From: kolaente Date: Tue, 24 Feb 2026 12:35:13 +0100 Subject: [PATCH] fix: eliminate nested database sessions to prevent table locks Refactor functions that created their own sessions when called from within existing transactions, which caused "database table is locked" errors in SQLite's shared-cache mode. Changes: - Add files.CreateWithSession() to reuse caller's session - Refactor DeleteBackgroundFileIfExists() to accept session parameter - Add variadic session parameter to notifications.Notify() and Notifiable.ShouldNotify() interface - Update all Notify callers (~17 sites) to pass their session through - Use files.CreateWithSession in SaveBackgroundFile and NewAttachment - Fix test code to commit sessions before assertions --- pkg/files/files.go | 12 ++++++++ pkg/models/export.go | 2 +- pkg/models/listeners.go | 10 +++--- pkg/models/project.go | 13 ++------ pkg/models/project_test.go | 10 ++++-- pkg/models/task_attachment.go | 4 +-- pkg/models/task_overdue_reminder.go | 2 +- pkg/models/task_reminder.go | 2 +- pkg/models/user_delete.go | 2 +- pkg/modules/auth/openid/openid_test.go | 2 ++ pkg/modules/background/handler/background.go | 4 +-- pkg/notifications/notification.go | 32 ++++++++++++++------ pkg/notifications/notification_test.go | 9 ++++-- pkg/notifications/notify_disabled_test.go | 10 +++--- pkg/user/delete.go | 4 +-- pkg/user/totp.go | 4 +-- pkg/user/update_email.go | 2 +- pkg/user/user.go | 11 +++++-- pkg/user/user_create.go | 2 +- pkg/user/user_password_reset.go | 5 +-- pkg/user/user_test.go | 3 ++ 21 files changed, 92 insertions(+), 53 deletions(-) diff --git a/pkg/files/files.go b/pkg/files/files.go index 5426c9ebf..62cab552f 100644 --- a/pkg/files/files.go +++ b/pkg/files/files.go @@ -97,6 +97,18 @@ func Create(f io.ReadSeeker, realname string, realsize uint64, a web.Auth) (file return CreateWithMime(f, realname, realsize, a, mime.String()) } +// CreateWithSession creates a new file using an existing session to avoid nested transactions +func CreateWithSession(s *xorm.Session, f io.ReadSeeker, realname string, realsize uint64, a web.Auth) (file *File, err error) { + mime, err := mimetype.DetectReader(f) + if err != nil { + return nil, fmt.Errorf("failed to detect mime type: %w", err) + } + if _, err := f.Seek(0, io.SeekStart); err != nil { + return nil, fmt.Errorf("failed to seek after mime detection: %w", err) + } + return CreateWithMimeAndSession(s, f, realname, realsize, a, mime.String(), true) +} + // CreateWithMime creates a new file from an FileHeader and sets its mime type func CreateWithMime(f io.ReadSeeker, realname string, realsize uint64, a web.Auth, mime string) (file *File, err error) { s := db.NewSession() diff --git a/pkg/models/export.go b/pkg/models/export.go index 2189057be..5c953e134 100644 --- a/pkg/models/export.go +++ b/pkg/models/export.go @@ -115,7 +115,7 @@ func ExportUserData(s *xorm.Session, u *user.User) (err error) { // Send a notification return notifications.Notify(u, &DataExportReadyNotification{ User: u, - }) + }, s) } func getRawTasksForExport(s *xorm.Session, projectIDs []int64, a web.Auth) (tasks []*Task, err error) { diff --git a/pkg/models/listeners.go b/pkg/models/listeners.go index 41e9ca8d2..7aea8b152 100644 --- a/pkg/models/listeners.go +++ b/pkg/models/listeners.go @@ -162,7 +162,7 @@ func notifyMentionedUsers(sess *xorm.Session, task *Task, text string, n notific continue } - err = notifications.Notify(u, n) + err = notifications.Notify(u, n, sess) if err != nil { return users, err } @@ -226,7 +226,7 @@ func (s *SendTaskCommentNotification) Handle(msg *message.Message) (err error) { Task: event.Task, Comment: event.Comment, } - err = notifications.Notify(subscriber.User, n) + err = notifications.Notify(subscriber.User, n, sess) if err != nil { return } @@ -315,7 +315,7 @@ func (s *SendTaskAssignedNotification) Handle(msg *message.Message) (err error) Assignee: event.Assignee, Target: subscriber.User, } - err = notifications.Notify(subscriber.User, n) + err = notifications.Notify(subscriber.User, n, sess) if err != nil { return } @@ -368,7 +368,7 @@ func (s *SendTaskDeletedNotification) Handle(msg *message.Message) (err error) { Doer: event.Doer, Task: event.Task, } - err = notifications.Notify(subscriber.User, n) + err = notifications.Notify(subscriber.User, n, sess) if err != nil { return } @@ -826,7 +826,7 @@ func (s *SendProjectCreatedNotification) Handle(msg *message.Message) (err error Doer: event.Doer, Project: event.Project, } - err = notifications.Notify(subscriber.User, n) + err = notifications.Notify(subscriber.User, n, sess) if err != nil { return } diff --git a/pkg/models/project.go b/pkg/models/project.go index f96f39b54..7b5f17b6c 100644 --- a/pkg/models/project.go +++ b/pkg/models/project.go @@ -1191,7 +1191,7 @@ func (p *Project) Delete(s *xorm.Session, a web.Auth) (err error) { return } - err = fullProject.DeleteBackgroundFileIfExists() + err = fullProject.DeleteBackgroundFileIfExists(s) if err != nil { return } @@ -1278,25 +1278,18 @@ func (p *Project) Delete(s *xorm.Session, a web.Auth) (err error) { // DeleteBackgroundFileIfExists deletes the list's background file from the db and the filesystem, // if one exists -func (p *Project) DeleteBackgroundFileIfExists() (err error) { +func (p *Project) DeleteBackgroundFileIfExists(s *xorm.Session) (err error) { if p.BackgroundFileID == 0 { return } - s := db.NewSession() - defer s.Close() - file := files.File{ID: p.BackgroundFileID} err = file.Delete(s) if err != nil && files.IsErrFileDoesNotExist(err) { return nil } - if err != nil { - _ = s.Rollback() - return err - } - return s.Commit() + return err } // SetProjectBackground sets a background file as project background in the db diff --git a/pkg/models/project_test.go b/pkg/models/project_test.go index 0f88bfce2..761954cdc 100644 --- a/pkg/models/project_test.go +++ b/pkg/models/project_test.go @@ -403,6 +403,7 @@ func TestProject_DeleteBackgroundFileIfExists(t *testing.T) { db.LoadAndAssertFixtures(t) files.InitTestFileFixtures(t) s := db.NewSession() + defer s.Close() file := &files.File{ID: 1} project := Project{ ID: 1, @@ -410,13 +411,14 @@ func TestProject_DeleteBackgroundFileIfExists(t *testing.T) { } err := SetProjectBackground(s, project.ID, file, "") require.NoError(t, err) - err = project.DeleteBackgroundFileIfExists() + err = project.DeleteBackgroundFileIfExists(s) require.NoError(t, err) }) t.Run("project with invalid background", func(t *testing.T) { db.LoadAndAssertFixtures(t) files.InitTestFileFixtures(t) s := db.NewSession() + defer s.Close() file := &files.File{ID: 9999} project := Project{ ID: 1, @@ -424,14 +426,16 @@ func TestProject_DeleteBackgroundFileIfExists(t *testing.T) { } err := SetProjectBackground(s, project.ID, file, "") require.NoError(t, err) - err = project.DeleteBackgroundFileIfExists() + err = project.DeleteBackgroundFileIfExists(s) require.NoError(t, err) }) t.Run("project without background", func(t *testing.T) { db.LoadAndAssertFixtures(t) files.InitTestFileFixtures(t) + s := db.NewSession() + defer s.Close() project := Project{ID: 1} - err := project.DeleteBackgroundFileIfExists() + err := project.DeleteBackgroundFileIfExists(s) require.NoError(t, err) }) } diff --git a/pkg/models/task_attachment.go b/pkg/models/task_attachment.go index 64e3a4129..c63182492 100644 --- a/pkg/models/task_attachment.go +++ b/pkg/models/task_attachment.go @@ -60,8 +60,8 @@ func (*TaskAttachment) TableName() string { // NewAttachment creates a new task attachment func (ta *TaskAttachment) NewAttachment(s *xorm.Session, f io.ReadSeeker, realname string, realsize uint64, a web.Auth) error { - // Store the file - file, err := files.Create(f, realname, realsize, a) + // Store the file using the existing session to avoid nested transactions + file, err := files.CreateWithSession(s, f, realname, realsize, a) if err != nil { if files.IsErrFileIsTooLarge(err) { return ErrTaskAttachmentIsTooLarge{Size: realsize} diff --git a/pkg/models/task_overdue_reminder.go b/pkg/models/task_overdue_reminder.go index 31b669e1f..96399cb30 100644 --- a/pkg/models/task_overdue_reminder.go +++ b/pkg/models/task_overdue_reminder.go @@ -178,7 +178,7 @@ func RegisterOverdueReminderCron() { } } - err = notifications.Notify(ut.user, n) + err = notifications.Notify(ut.user, n, s) if err != nil { log.Errorf("[Undone Overdue Tasks Reminder] Could not notify user %d: %s", ut.user.ID, err) return diff --git a/pkg/models/task_reminder.go b/pkg/models/task_reminder.go index 8e5c37b41..def29d04f 100644 --- a/pkg/models/task_reminder.go +++ b/pkg/models/task_reminder.go @@ -383,7 +383,7 @@ func RegisterReminderCron() { } for _, n := range reminders { - err = notifications.Notify(n.User, n) + err = notifications.Notify(n.User, n, s) if err != nil { log.Errorf("[Task Reminder Cron] Could not notify user %d: %s", n.User.ID, err) return diff --git a/pkg/models/user_delete.go b/pkg/models/user_delete.go index 5beffc813..65a663394 100644 --- a/pkg/models/user_delete.go +++ b/pkg/models/user_delete.go @@ -176,7 +176,7 @@ func DeleteUser(s *xorm.Session, u *user.User) (err error) { return notifications.Notify(u, &user.AccountDeletedNotification{ User: u, - }) + }, s) } func ensureProjectAdminUser(s *xorm.Session, l *Project) (hadUsers bool, err error) { diff --git a/pkg/modules/auth/openid/openid_test.go b/pkg/modules/auth/openid/openid_test.go index e32f84465..fca80d426 100644 --- a/pkg/modules/auth/openid/openid_test.go +++ b/pkg/modules/auth/openid/openid_test.go @@ -218,6 +218,8 @@ func TestGetOrCreateUser(t *testing.T) { teamData := getTeamDataFromToken(cl.VikunjaGroups, nil) err := models.SyncExternalTeamsForUser(s, u, teamData, "https://some.issuer", "OIDC") require.NoError(t, err) + err = s.Commit() + require.NoError(t, err) db.AssertMissing(t, "team_members", map[string]interface{}{ "team_id": 14, diff --git a/pkg/modules/background/handler/background.go b/pkg/modules/background/handler/background.go index 9ecd39c93..2f405c8e5 100644 --- a/pkg/modules/background/handler/background.go +++ b/pkg/modules/background/handler/background.go @@ -282,7 +282,7 @@ func SaveBackgroundFile(s *xorm.Session, auth web.Auth, project *models.Project, return err } - f, err := files.Create(bytes.NewReader(buf.Bytes()), filename, filesize, auth) + f, err := files.CreateWithSession(s, bytes.NewReader(buf.Bytes()), filename, filesize, auth) if err != nil { return err } @@ -411,7 +411,7 @@ func RemoveProjectBackground(c *echo.Context) error { return err } - err = project.DeleteBackgroundFileIfExists() + err = project.DeleteBackgroundFileIfExists(s) if err != nil { _ = s.Rollback() return err diff --git a/pkg/notifications/notification.go b/pkg/notifications/notification.go index 83677364d..7f0e1faef 100644 --- a/pkg/notifications/notification.go +++ b/pkg/notifications/notification.go @@ -21,6 +21,8 @@ import ( "code.vikunja.io/api/pkg/db" "code.vikunja.io/api/pkg/log" + + "xorm.io/xorm" ) // Notification is a notification which can be sent via mail or db. @@ -50,20 +52,21 @@ type Notifiable interface { // RouteForDB should return the id of the notifiable entity to save it in the database. RouteForDB() int64 // ShouldNotify provides a last-minute way to cancel a notification. It will be called immediately before - // sending a notification. - ShouldNotify() (should bool, err error) + // sending a notification. An optional session can be passed to reuse an existing transaction. + ShouldNotify(sessions ...*xorm.Session) (should bool, err error) // Lang provides the language which should be used for translations in the mail. Lang() string } -// Notify notifies a notifiable of a notification -func Notify(notifiable Notifiable, notification Notification) (err error) { +// Notify notifies a notifiable of a notification. +// An optional xorm session can be passed to reuse an existing transaction for the DB notification. +func Notify(notifiable Notifiable, notification Notification, sessions ...*xorm.Session) (err error) { if isUnderTest { sentTestNotifications = append(sentTestNotifications, notification) return nil } - should, err := notifiable.ShouldNotify() + should, err := notifiable.ShouldNotify(sessions...) if err != nil || !should { log.Debugf("Not notifying user %d because they are disabled", notifiable.RouteForDB()) return err @@ -74,7 +77,12 @@ func Notify(notifiable Notifiable, notification Notification) (err error) { return } - return notifyDB(notifiable, notification) + var s *xorm.Session + if len(sessions) > 0 && sessions[0] != nil { + s = sessions[0] + } + + return notifyDB(notifiable, notification, s) } func notifyMail(notifiable Notifiable, notification Notification) error { @@ -96,7 +104,7 @@ func notifyMail(notifiable Notifiable, notification Notification) error { return SendMail(mail, notifiable.Lang()) } -func notifyDB(notifiable Notifiable, notification Notification) (err error) { +func notifyDB(notifiable Notifiable, notification Notification, existingSession *xorm.Session) (err error) { dbContent := notification.ToDB() if dbContent == nil { @@ -108,8 +116,6 @@ func notifyDB(notifiable Notifiable, notification Notification) (err error) { return err } - s := db.NewSession() - defer s.Close() dbNotification := &DatabaseNotification{ NotifiableID: notifiable.RouteForDB(), Notification: content, @@ -120,6 +126,14 @@ func notifyDB(notifiable Notifiable, notification Notification) (err error) { dbNotification.SubjectID = subject.SubjectID() } + if existingSession != nil { + _, err = existingSession.Insert(dbNotification) + return err + } + + s := db.NewSession() + defer s.Close() + _, err = s.Insert(dbNotification) if err != nil { _ = s.Rollback() diff --git a/pkg/notifications/notification_test.go b/pkg/notifications/notification_test.go index 397fdbe54..f15eb350b 100644 --- a/pkg/notifications/notification_test.go +++ b/pkg/notifications/notification_test.go @@ -22,6 +22,7 @@ import ( "code.vikunja.io/api/pkg/db" "github.com/stretchr/testify/require" + "xorm.io/xorm" "xorm.io/xorm/schemas" ) @@ -65,7 +66,7 @@ func (t *testNotifiable) RouteForDB() int64 { return 42 } -func (t *testNotifiable) ShouldNotify() (should bool, err error) { +func (t *testNotifiable) ShouldNotify(_ ...*xorm.Session) (should bool, err error) { return t.ShouldSendNotification, nil } @@ -77,9 +78,10 @@ func TestNotify(t *testing.T) { t.Run("normal", func(t *testing.T) { s := db.NewSession() - defer s.Close() _, err := s.Exec("delete from notifications") require.NoError(t, err) + require.NoError(t, s.Commit()) + s.Close() tn := &testNotification{ Test: "somethingsomething", @@ -112,9 +114,10 @@ func TestNotify(t *testing.T) { t.Run("disabled notifiable", func(t *testing.T) { s := db.NewSession() - defer s.Close() _, err := s.Exec("delete from notifications") require.NoError(t, err) + require.NoError(t, s.Commit()) + s.Close() tn := &testNotification{ Test: "somethingsomething", diff --git a/pkg/notifications/notify_disabled_test.go b/pkg/notifications/notify_disabled_test.go index 55b950c6a..363046c46 100644 --- a/pkg/notifications/notify_disabled_test.go +++ b/pkg/notifications/notify_disabled_test.go @@ -22,14 +22,16 @@ import ( "code.vikunja.io/api/pkg/config" "code.vikunja.io/api/pkg/i18n" + + "xorm.io/xorm" ) type disabledMailNotifiable struct{} -func (d *disabledMailNotifiable) RouteForMail() (string, error) { return "test@example.com", nil } -func (d *disabledMailNotifiable) RouteForDB() int64 { return 1 } -func (d *disabledMailNotifiable) ShouldNotify() (bool, error) { return true, nil } -func (d *disabledMailNotifiable) Lang() string { return "en" } +func (d *disabledMailNotifiable) RouteForMail() (string, error) { return "test@example.com", nil } +func (d *disabledMailNotifiable) RouteForDB() int64 { return 1 } +func (d *disabledMailNotifiable) ShouldNotify(_ ...*xorm.Session) (bool, error) { return true, nil } +func (d *disabledMailNotifiable) Lang() string { return "en" } type disabledMailNotification struct{} diff --git a/pkg/user/delete.go b/pkg/user/delete.go index c08e094a7..e985a5869 100644 --- a/pkg/user/delete.go +++ b/pkg/user/delete.go @@ -70,7 +70,7 @@ func notifyUsersScheduledForDeletion() { err = notifications.Notify(user, &AccountDeletionNotification{ User: user, NotificationNumber: number, - }) + }, s) if err != nil { log.Errorf("Could not notify user %d of their deletion: %s", user.ID, err) continue @@ -100,7 +100,7 @@ func RequestDeletion(s *xorm.Session, user *User) (err error) { return notifications.Notify(user, &AccountDeletionConfirmNotification{ User: user, ConfirmToken: token.Token, - }) + }, s) } // ConfirmDeletion ConformDeletion checks a token and schedules the user for deletion diff --git a/pkg/user/totp.go b/pkg/user/totp.go index 0a2241831..2f9314860 100644 --- a/pkg/user/totp.go +++ b/pkg/user/totp.go @@ -172,7 +172,7 @@ func HandleFailedTOTPAuth(s *xorm.Session, user *User) { attempts := a.(int64) if attempts == 3 { - err = notifications.Notify(user, &InvalidTOTPNotification{User: user}) + err = notifications.Notify(user, &InvalidTOTPNotification{User: user}, s) if err != nil { log.Errorf("Could not send failed TOTP notification to user %d: %s", user.ID, err) return @@ -191,7 +191,7 @@ func HandleFailedTOTPAuth(s *xorm.Session, user *User) { } err = notifications.Notify(user, &PasswordAccountLockedAfterInvalidTOTPNotification{ User: user, - }) + }, s) if err != nil { log.Errorf("Could send password information mail to user %d after 10 failed TOTP attempts: %s", user.ID, err) return diff --git a/pkg/user/update_email.go b/pkg/user/update_email.go index b0ec0bec4..73e104682 100644 --- a/pkg/user/update_email.go +++ b/pkg/user/update_email.go @@ -84,6 +84,6 @@ func UpdateEmail(s *xorm.Session, update *EmailUpdate) (err error) { ConfirmToken: token.Token, } - err = notifications.Notify(update.User, n) + err = notifications.Notify(update.User, n, s) return } diff --git a/pkg/user/user.go b/pkg/user/user.go index a8abdc633..dc28c85e9 100644 --- a/pkg/user/user.go +++ b/pkg/user/user.go @@ -140,9 +140,14 @@ func (u *User) RouteForDB() int64 { return u.ID } -func (u *User) ShouldNotify() (bool, error) { - s := db.NewSession() - defer s.Close() +func (u *User) ShouldNotify(sessions ...*xorm.Session) (bool, error) { + var s *xorm.Session + if len(sessions) > 0 && sessions[0] != nil { + s = sessions[0] + } else { + s = db.NewSession() + defer s.Close() + } user, err := getUser(s, &User{ID: u.ID}, true) if err != nil { return false, err diff --git a/pkg/user/user_create.go b/pkg/user/user_create.go index 79c790b9e..85f03614b 100644 --- a/pkg/user/user_create.go +++ b/pkg/user/user_create.go @@ -120,7 +120,7 @@ func CreateUser(s *xorm.Session, user *User) (newUser *User, err error) { ConfirmToken: token.Token, } - err = notifications.Notify(user, n) + err = notifications.Notify(user, n, s) return newUserOut, err } diff --git a/pkg/user/user_password_reset.go b/pkg/user/user_password_reset.go index 9c2e1c684..4450b0e93 100644 --- a/pkg/user/user_password_reset.go +++ b/pkg/user/user_password_reset.go @@ -89,10 +89,11 @@ func ResetPassword(s *xorm.Session, reset *PasswordReset) (userID int64, err err User: user, } - err = notifications.Notify(user, n) + err = notifications.Notify(user, n, s) return } + // PasswordTokenRequest defines the request format for password reset resqest type PasswordTokenRequest struct { Email string `json:"email" valid:"email,length(0|250)" maxLength:"250"` @@ -130,6 +131,6 @@ func RequestUserPasswordResetToken(s *xorm.Session, user *User) (err error) { Token: token, } - err = notifications.Notify(user, n) + err = notifications.Notify(user, n, s) return } diff --git a/pkg/user/user_test.go b/pkg/user/user_test.go index 2af7b9273..b13c63396 100644 --- a/pkg/user/user_test.go +++ b/pkg/user/user_test.go @@ -650,6 +650,9 @@ func TestConfirmDeletion(t *testing.T) { err := ConfirmDeletion(s, user, token) require.NoError(t, err) + err = s.Commit() + require.NoError(t, err) + db.AssertMissing(t, "user_tokens", map[string]interface{}{ "token": token, "kind": TokenAccountDeletion,