kodev-org
/
vikunja
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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
This commit is contained in:
kolaente 2026-02-24 12:35:13 +01:00
parent a6e6f252db
commit 49bba7f830
21 changed files with 92 additions and 53 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
pkg/files/files.go
View File

@ -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()

2
pkg/models/export.go
View File

@ -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) {

10
pkg/models/listeners.go
View File

@ -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
}

13
pkg/models/project.go
View File

@ -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

10
pkg/models/project_test.go
View File

@ -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)
})
}

4
pkg/models/task_attachment.go
View File

@ -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}

2
pkg/models/task_overdue_reminder.go
View File

@ -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

2
pkg/models/task_reminder.go
View File

@ -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

2
pkg/models/user_delete.go
View File

@ -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) {

2
pkg/modules/auth/openid/openid_test.go
View File

@ -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,

4
pkg/modules/background/handler/background.go
View File

@ -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

32
pkg/notifications/notification.go
View File

@ -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()

9
pkg/notifications/notification_test.go
View File

@ -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",

10
pkg/notifications/notify_disabled_test.go
View File

@ -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{}

4
pkg/user/delete.go
View File

@ -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

4
pkg/user/totp.go
View File

@ -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

2
pkg/user/update_email.go
View File

@ -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
}

11
pkg/user/user.go
View File

@ -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

2
pkg/user/user_create.go
View File

@ -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
}

5
pkg/user/user_password_reset.go
View File

@ -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
}

3
pkg/user/user_test.go
View File

@ -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,