Correctly rollback in ForkRepository (#17034) (#17045)

Backport #17034

The rollback functionality in
services/repository/repository.go:ForkRepository is incorrect and could
lead to a deadlock as it uses DeleteRepository to delete the rolled-back
repository - a function which creates its own transaction.

This PR adjusts the rollback function to only use RemoveAll as any
database changes will be automatically rolled-back. It also handles
panics and adjusts the Close within WithTx to ensure that if there is a
panic the session will always be closed.

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

diff --git a/models/context.go b/models/context.go
index 1221ab7dede9d9900cce889bd98affcd5add7b11..6481a8209cc21fc9ba245e1368039b2e154ba3d6 100644 (file)
--- a/models/context.go
+++ b/models/context.go
@@ -45,19 +45,16 @@ func WithContext(f func(ctx DBContext) error) error {
 // WithTx represents executing database operations on a transaction
 func WithTx(f func(ctx DBContext) error) error {
        sess := x.NewSession()
+       defer sess.Close()
        if err := sess.Begin(); err != nil {
-               sess.Close()
                return err
        }
 
        if err := f(DBContext{sess}); err != nil {
-               sess.Close()
                return err
        }
 
-       err := sess.Commit()
-       sess.Close()
-       return err
+       return sess.Commit()
 }
 
 // Iterate iterates the databases and doing something

diff --git a/modules/repository/fork.go b/modules/repository/fork.go
index 232ad4226a3419c5bb307f099f9f5f0fbbc7578b..6025faca33dbcaf1de7fa965ec1215d9a668a8ba 100644 (file)
--- a/modules/repository/fork.go
+++ b/modules/repository/fork.go
@@ -13,6 +13,7 @@ import (
        "code.gitea.io/gitea/modules/git"
        "code.gitea.io/gitea/modules/log"
        "code.gitea.io/gitea/modules/structs"
+       "code.gitea.io/gitea/modules/util"
 )
 
 // ForkRepository forks a repository
@@ -45,38 +46,60 @@ func ForkRepository(doer, owner *models.User, oldRepo *models.Repository, name,
 
        oldRepoPath := oldRepo.RepoPath()
 
+       needsRollback := false
+       rollbackFn := func() {
+               if !needsRollback {
+                       return
+               }
+
+               repoPath := models.RepoPath(owner.Name, repo.Name)
+
+               if exists, _ := util.IsExist(repoPath); !exists {
+                       return
+               }
+
+               // As the transaction will be failed and hence database changes will be destroyed we only need
+               // to delete the related repository on the filesystem
+               if errDelete := util.RemoveAll(repoPath); errDelete != nil {
+                       log.Error("Failed to remove fork repo")
+               }
+       }
+
+       needsRollbackInPanic := true
+       defer func() {
+               panicErr := recover()
+               if panicErr == nil {
+                       return
+               }
+
+               if needsRollbackInPanic {
+                       rollbackFn()
+               }
+               panic(panicErr)
+       }()
+
        err = models.WithTx(func(ctx models.DBContext) error {
                if err = models.CreateRepository(ctx, doer, owner, repo, false); err != nil {
                        return err
                }
 
-               rollbackRemoveFn := func() {
-                       if repo.ID == 0 {
-                               return
-                       }
-                       if errDelete := models.DeleteRepository(doer, owner.ID, repo.ID); errDelete != nil {
-                               log.Error("Rollback deleteRepository: %v", errDelete)
-                       }
-               }
-
                if err = models.IncrementRepoForkNum(ctx, oldRepo.ID); err != nil {
-                       rollbackRemoveFn()
                        return err
                }
 
                // copy lfs files failure should not be ignored
-               if err := models.CopyLFS(ctx, repo, oldRepo); err != nil {
-                       rollbackRemoveFn()
+               if err = models.CopyLFS(ctx, repo, oldRepo); err != nil {
                        return err
                }
 
+               needsRollback = true
+
                repoPath := models.RepoPath(owner.Name, repo.Name)
                if stdout, err := git.NewCommand(
                        "clone", "--bare", oldRepoPath, repoPath).
                        SetDescription(fmt.Sprintf("ForkRepository(git clone): %s to %s", oldRepo.FullName(), repo.FullName())).
                        RunInDirTimeout(10*time.Minute, ""); err != nil {
                        log.Error("Fork Repository (git clone) Failed for %v (from %v):\nStdout: %s\nError: %v", repo, oldRepo, stdout, err)
-                       rollbackRemoveFn()
                        return fmt.Errorf("git clone: %v", err)
                }
 
@@ -84,23 +107,23 @@ func ForkRepository(doer, owner *models.User, oldRepo *models.Repository, name,
                        SetDescription(fmt.Sprintf("ForkRepository(git update-server-info): %s", repo.FullName())).
                        RunInDir(repoPath); err != nil {
                        log.Error("Fork Repository (git update-server-info) failed for %v:\nStdout: %s\nError: %v", repo, stdout, err)
-                       rollbackRemoveFn()
                        return fmt.Errorf("git update-server-info: %v", err)
                }
 
                if err = createDelegateHooks(repoPath); err != nil {
-                       rollbackRemoveFn()
                        return fmt.Errorf("createDelegateHooks: %v", err)
                }
                return nil
        })
+       needsRollbackInPanic = false
        if err != nil {
+               rollbackFn()
                return nil, err
        }
 
        // even if below operations failed, it could be ignored. And they will be retried
        ctx := models.DefaultDBContext()
-       if err = repo.UpdateSize(ctx); err != nil {
+       if err := repo.UpdateSize(ctx); err != nil {
                log.Error("Failed to update size for repository: %v", err)
        }
        if err := models.CopyLanguageStat(oldRepo, repo); err != nil {