From 26ef180f466046f9cfab87eea5ea93f5d6da6998 Mon Sep 17 00:00:00 2001 From: zeripath Date: Tue, 14 Sep 2021 17:16:40 +0100 Subject: [PATCH] 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 --- models/context.go | 7 ++--- modules/repository/fork.go | 55 +++++++++++++++++++++++++++----------- 2 files changed, 41 insertions(+), 21 deletions(-) diff --git a/models/context.go b/models/context.go index a074d0683..8c29f74d2 100644 --- 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 892c1494d..4e750dec0 100644 --- 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, opts models.ForkRepoOptions) (_ *m oldRepoPath := opts.BaseRepo.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, 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 { - rollbackRemoveFn() + if err = models.CopyLFS(ctx, repo, opts.BaseRepo); 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", 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 {