Correctly rollback in ForkRepository (#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>

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()
+	return sess.Commit()
-	sess.Close()
-	return err
 }
 
 // Iterate iterates the databases and doing something

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, opts models.ForkRepoOptions) (_ *m
 
 	oldRepoPath := opts.BaseRepo.RepoPath()
 
-	err = models.WithTx(func(ctx models.DBContext) error {
+	needsRollback := false
-		if err = models.CreateRepository(ctx, doer, owner, repo, false); err != nil {
+	rollbackFn := func() {
-			return err
+		if !needsRollback {
+			return
+		}
+
+		repoPath := models.RepoPath(owner.Name, repo.Name)
+
+		if exists, _ := util.IsExist(repoPath); !exists {
+			return
 		}
 
-		rollbackRemoveFn := func() {
+		// As the transaction will be failed and hence database changes will be destroyed we only need
-			if repo.ID == 0 {
+		// 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 errDelete := models.DeleteRepository(doer, owner.ID, repo.ID); errDelete != nil {
+
-				log.Error("Rollback deleteRepository: %v", errDelete)
+		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
 		}
 
 		if err = models.IncrementRepoForkNum(ctx, opts.BaseRepo.ID); err != nil {
-			rollbackRemoveFn()
 			return err
 		}
 
 		// copy lfs files failure should not be ignored
-		if err := models.CopyLFS(ctx, repo, opts.BaseRepo); err != nil {
+		if err = models.CopyLFS(ctx, repo, opts.BaseRepo); err != nil {
-			rollbackRemoveFn()
 			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", opts.BaseRepo.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, opts.BaseRepo, stdout, err)
-			rollbackRemoveFn()
 			return fmt.Errorf("git clone: %v", err)
 		}
 
@@ -84,23 +107,23 @@ func ForkRepository(doer, owner *models.User, opts models.ForkRepoOptions) (_ *m
 			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(opts.BaseRepo, repo); err != nil {