From 2f680d041c7fcb931126b8e33bd32c1183a4c9c1 Mon Sep 17 00:00:00 2001
From: kolaente <k@knt.li>
Date: Wed, 25 Feb 2026 10:38:27 +0100
Subject: [PATCH] fix: address review comments on session lifecycle

- user_export.go: Remove defer s.Close() from checkExportRequest since
  it returns the session to callers. Callers now own the session
  lifecycle with their own defer s.Close(). Close session on all error
  paths within checkExportRequest.

- user_delete.go: Close the read session immediately after Find() before
  the per-user deletion loop, avoiding a long-lived transaction holding
  locks unnecessarily.

- user/delete.go: Remove double s.Close() in notifyUsersScheduledForDeletion
  by closing immediately after Find() instead of using both defer and
  explicit close.

- caldav_token.go: Return nil token on Commit() error to prevent callers
  from using an unpersisted token.
---
 pkg/models/user_delete.go        | 2 +-
 pkg/routes/api/v1/user_export.go | 9 ++++++---
 pkg/user/caldav_token.go         | 8 ++++++--
 pkg/user/delete.go               | 5 +----
 4 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/pkg/models/user_delete.go b/pkg/models/user_delete.go
index 65a663394..8ccd137e0 100644
--- a/pkg/models/user_delete.go
+++ b/pkg/models/user_delete.go
@@ -42,10 +42,10 @@ func RegisterUserDeletionCron() {
 
 func deleteUsers() {
 	s := db.NewSession()
-	defer s.Close()
 	users := []*user.User{}
 	err := s.Where(builder.Lt{"deletion_scheduled_at": time.Now()}).
 		Find(&users)
+	s.Close()
 	if err != nil {
 		log.Errorf("Could not get users scheduled for deletion: %s", err)
 		return
diff --git a/pkg/routes/api/v1/user_export.go b/pkg/routes/api/v1/user_export.go
index 2def8d6b7..43efadfe7 100644
--- a/pkg/routes/api/v1/user_export.go
+++ b/pkg/routes/api/v1/user_export.go
@@ -32,11 +32,10 @@ import (
 
 func checkExportRequest(c *echo.Context) (s *xorm.Session, u *user.User, err error) {
 	s = db.NewSession()
-	defer s.Close()
 
 	u, err = user.GetCurrentUserFromDB(s, c)
 	if err != nil {
-		_ = s.Rollback()
+		s.Close()
 		return nil, nil, err
 	}
 
@@ -47,17 +46,19 @@ func checkExportRequest(c *echo.Context) (s *xorm.Session, u *user.User, err err
 
 	var pass UserPasswordConfirmation
 	if err := c.Bind(&pass); err != nil {
+		s.Close()
 		return nil, nil, echo.NewHTTPError(http.StatusBadRequest, "No password provided.").Wrap(err)
 	}
 
 	err = c.Validate(pass)
 	if err != nil {
+		s.Close()
 		return nil, nil, echo.NewHTTPError(http.StatusBadRequest, err.Error()).Wrap(err)
 	}
 
 	err = user.CheckUserPassword(u, pass.Password)
 	if err != nil {
-		_ = s.Rollback()
+		s.Close()
 		return nil, nil, err
 	}
 
@@ -80,6 +81,7 @@ func RequestUserDataExport(c *echo.Context) error {
 	if err != nil {
 		return err
 	}
+	defer s.Close()
 
 	err = events.Dispatch(&models.UserDataExportRequestedEvent{
 		User: u,
@@ -115,6 +117,7 @@ func DownloadUserDataExport(c *echo.Context) error {
 	if err != nil {
 		return err
 	}
+	defer s.Close()
 
 	err = s.Commit()
 	if err != nil {
diff --git a/pkg/user/caldav_token.go b/pkg/user/caldav_token.go
index 096fe3add..438136525 100644
--- a/pkg/user/caldav_token.go
+++ b/pkg/user/caldav_token.go
@@ -25,10 +25,14 @@ func GenerateNewCaldavToken(u *User) (token *Token, err error) {
 	token, err = generateHashedToken(s, u, TokenCaldavAuth)
 	if err != nil {
 		_ = s.Rollback()
-		return
+		return nil, err
 	}
 
-	return token, s.Commit()
+	if err = s.Commit(); err != nil {
+		return nil, err
+	}
+
+	return token, nil
 }
 
 func GetCaldavTokens(u *User) (tokens []*Token, err error) {
diff --git a/pkg/user/delete.go b/pkg/user/delete.go
index 154c9f741..a242f95fe 100644
--- a/pkg/user/delete.go
+++ b/pkg/user/delete.go
@@ -38,10 +38,10 @@ func RegisterDeletionNotificationCron() {
 
 func notifyUsersScheduledForDeletion() {
 	s := db.NewSession()
-	defer s.Close()
 	users := []*User{}
 	err := s.Where(builder.NotNull{"deletion_scheduled_at"}).
 		Find(&users)
+	s.Close()
 	if err != nil {
 		log.Errorf("Could not get users scheduled for deletion: %s", err)
 		return
@@ -51,9 +51,6 @@ func notifyUsersScheduledForDeletion() {
 		return
 	}
 
-	// Close the read-only session before processing each user in its own transaction.
-	s.Close()
-
 	log.Debugf("Found %d users scheduled for deletion to notify", len(users))
 
 	for _, user := range users {