Rework create/fork/adopt/generate repository to make sure resources will be cleanup once failed

models/git/branch.go

@@ -223,6 +223,11 @@ func GetDeletedBranchByID(ctx context.Context, repoID, branchID int64) (*Branch,
 	return &branch, nil
 }
 
+func DeleteRepoBranches(ctx context.Context, repoID int64) error {
+	_, err := db.GetEngine(ctx).Where("repo_id=?", repoID).Delete(new(Branch))
+	return err
+}
+
 func DeleteBranches(ctx context.Context, repoID, doerID int64, branchIDs []int64) error {
 	return db.WithTx(ctx, func(ctx context.Context) error {
 		branches := make([]*Branch, 0, len(branchIDs))

models/repo/release.go

@@ -558,3 +558,8 @@ func FindTagsByCommitIDs(ctx context.Context, repoID int64, commitIDs ...string)
 	}
 	return res, nil
 }
+
+func DeleteRepoReleases(ctx context.Context, repoID int64) error {
+	_, err := db.GetEngine(ctx).Where("repo_id = ?", repoID).Delete(new(Release))
+	return err
+}

services/repository/adopt.go

@@ -28,6 +28,18 @@ import (
 	"github.com/gobwas/glob"
 )
 
+func deleteFailedAdoptRepository(ctx context.Context, repoID int64) error {
+	return db.WithTx(ctx, func(ctx context.Context) error {
+		if err := deleteDBRepository(ctx, repoID); err != nil {
+			return fmt.Errorf("deleteDBRepository: %w", err)
+		}
+		if err := git_model.DeleteRepoBranches(ctx, repoID); err != nil {
+			return fmt.Errorf("deleteRepoBranches: %w", err)
+		}
+		return repo_model.DeleteRepoReleases(ctx, repoID)
+	})
+}
+
 // AdoptRepository adopts pre-existing repository files for the user/organization.
 func AdoptRepository(ctx context.Context, doer, u *user_model.User, opts CreateRepoOptions) (*repo_model.Repository, error) {
 	if !doer.IsAdmin && !u.CanCreateRepo() {
@@ -48,58 +60,56 @@ func AdoptRepository(ctx context.Context, doer, u *user_model.User, opts CreateR
 		IsPrivate:                       opts.IsPrivate,
 		IsFsckEnabled:                   !opts.IsMirror,
 		CloseIssuesViaCommitInAnyBranch: setting.Repository.DefaultCloseIssuesViaCommitsInAnyBranch,
-		Status:                          opts.Status,
+		Status:                          repo_model.RepositoryBeingMigrated,
 		IsEmpty:                         !opts.AutoInit,
 	}
 
-	if err := db.WithTx(ctx, func(ctx context.Context) error {
-		isExist, err := gitrepo.IsRepositoryExist(ctx, repo)
+	// 1 - create the repository database operations first
+	err := db.WithTx(ctx, func(ctx context.Context) error {
+		return CreateRepositoryInDB(ctx, doer, u, repo, true, false)
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	// last - clean up if something goes wrong
+	// WARNING: Don't override all later err with local variables
+	defer func() {
 		if err != nil {
-			log.Error("Unable to check if %s exists. Error: %v", repo.FullName(), err)
-			return err
-		}
-		if !isExist {
-			return repo_model.ErrRepoNotExist{
-				OwnerName: u.Name,
-				Name:      repo.Name,
+			if errDel := deleteFailedAdoptRepository(ctx, repo.ID); errDel != nil {
+				log.Error("Failed to delete repository %s that could not be adopted: %v", repo.FullName(), errDel)
 			}
 		}
+	}()
 
-		if err := CreateRepositoryByExample(ctx, doer, u, repo, true, false); err != nil {
-			return err
-		}
+	// Re-fetch the repository from database before updating it (else it would
+	// override changes that were done earlier with sql)
+	if repo, err = repo_model.GetRepositoryByID(ctx, repo.ID); err != nil {
+		return nil, fmt.Errorf("getRepositoryByID: %w", err)
+	}
 
-		// Re-fetch the repository from database before updating it (else it would
-		// override changes that were done earlier with sql)
-		if repo, err = repo_model.GetRepositoryByID(ctx, repo.ID); err != nil {
-			return fmt.Errorf("getRepositoryByID: %w", err)
-		}
-		return nil
-	}); err != nil {
-		return nil, err
+	// 3 - adopt the repository from disk
+	if err = adoptRepository(ctx, repo, opts.DefaultBranch); err != nil {
+		return nil, fmt.Errorf("adoptRepository: %w", err)
 	}
 
-	if err := func() error {
-		if err := adoptRepository(ctx, repo, opts.DefaultBranch); err != nil {
-			return fmt.Errorf("adoptRepository: %w", err)
-		}
+	if err = repo_module.CheckDaemonExportOK(ctx, repo); err != nil {
+		return nil, fmt.Errorf("checkDaemonExportOK: %w", err)
+	}
 
-		if err := repo_module.CheckDaemonExportOK(ctx, repo); err != nil {
-			return fmt.Errorf("checkDaemonExportOK: %w", err)
-		}
+	var stdout string
+	if stdout, _, err = git.NewCommand("update-server-info").
+		RunStdString(ctx, &git.RunOpts{Dir: repo.RepoPath()}); err != nil {
+		log.Error("CreateRepository(git update-server-info) in %v: Stdout: %s\nError: %v", repo, stdout, err)
+		return nil, fmt.Errorf("CreateRepository(git update-server-info): %w", err)
+	}
 
-		if stdout, _, err := git.NewCommand("update-server-info").
-			RunStdString(ctx, &git.RunOpts{Dir: repo.RepoPath()}); err != nil {
-			log.Error("CreateRepository(git update-server-info) in %v: Stdout: %s\nError: %v", repo, stdout, err)
-			return fmt.Errorf("CreateRepository(git update-server-info): %w", err)
-		}
-		return nil
-	}(); err != nil {
-		if errDel := DeleteRepository(ctx, doer, repo, false /* no notify */); errDel != nil {
-			log.Error("Failed to delete repository %s that could not be adopted: %v", repo.FullName(), errDel)
-		}
-		return nil, err
+	// 4 - update repository status
+	repo.Status = repo_model.RepositoryReady
+	if err = repo_model.UpdateRepositoryCols(ctx, repo, "status"); err != nil {
+		return nil, fmt.Errorf("UpdateRepositoryCols: %w", err)
 	}
+
 	notify_service.AdoptRepository(ctx, doer, u, repo)
 
 	return repo, nil

services/repository/adopt_test.go

@@ -4,16 +4,19 @@
 package repository
 
 import (
+	"context"
 	"os"
 	"path"
 	"path/filepath"
 	"testing"
+	"time"
 
 	"code.gitea.io/gitea/models/db"
 	repo_model "code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/models/unittest"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -89,10 +92,31 @@ func TestListUnadoptedRepositories_ListOptions(t *testing.T) {
 
 func TestAdoptRepository(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
+
+	// a successful adopt
 	assert.NoError(t, unittest.SyncDirs(filepath.Join(setting.RepoRootPath, "user2", "repo1.git"), filepath.Join(setting.RepoRootPath, "user2", "test-adopt.git")))
 	user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
-	_, err := AdoptRepository(db.DefaultContext, user2, user2, CreateRepoOptions{Name: "test-adopt"})
+	adoptedRepo, err := AdoptRepository(db.DefaultContext, user2, user2, CreateRepoOptions{Name: "test-adopt"})
 	assert.NoError(t, err)
 	repoTestAdopt := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{Name: "test-adopt"})
 	assert.Equal(t, "sha1", repoTestAdopt.ObjectFormatName)
+
+	// just delete the adopted repo's db records
+	err = deleteFailedAdoptRepository(db.DefaultContext, adoptedRepo.ID)
+	assert.NoError(t, err)
+
+	unittest.AssertNotExistsBean(t, &repo_model.Repository{OwnerName: "user2", Name: "test-adopt"})
+
+	// a failed adopt
+	ctx, cancel := context.WithTimeout(db.DefaultContext, 1*time.Microsecond)
+	defer cancel()
+	adoptedRepo, err = AdoptRepository(ctx, user2, user2, CreateRepoOptions{Name: "test-adopt"})
+	assert.Error(t, err)
+	assert.Nil(t, adoptedRepo)
+
+	unittest.AssertNotExistsBean(t, &repo_model.Repository{OwnerName: "user2", Name: "test-adopt"})
+
+	exist, err := util.IsExist(repo_model.RepoPath("user2", "test-adopt"))
+	assert.NoError(t, err)
+	assert.True(t, exist) // the repository should be still in the disk
 }

services/repository/create.go

@@ -17,6 +17,7 @@ import (
 	"code.gitea.io/gitea/models/perm"
 	access_model "code.gitea.io/gitea/models/perm/access"
 	repo_model "code.gitea.io/gitea/models/repo"
+	system_model "code.gitea.io/gitea/models/system"
 	"code.gitea.io/gitea/models/unit"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/models/webhook"
@@ -244,100 +245,105 @@ func CreateRepositoryDirectly(ctx context.Context, doer, u *user_model.User, opt
 		ObjectFormatName:                opts.ObjectFormatName,
 	}
 
-	var rollbackRepo *repo_model.Repository
+	needsUpdateStatus := opts.Status != repo_model.RepositoryReady
 
-	if err := db.WithTx(ctx, func(ctx context.Context) error {
-		if err := CreateRepositoryByExample(ctx, doer, u, repo, false, false); err != nil {
-			return err
-		}
-
-		// No need for init mirror.
-		if opts.IsMirror {
-			return nil
-		}
+	// 1 - create the repository database operations first
+	err := db.WithTx(ctx, func(ctx context.Context) error {
+		return CreateRepositoryInDB(ctx, doer, u, repo, false, false)
+	})
+	if err != nil {
+		return nil, err
+	}
 
-		isExist, err := gitrepo.IsRepositoryExist(ctx, repo)
+	// last - clean up if something goes wrong
+	// WARNING: Don't override all later err with local variables
+	defer func() {
 		if err != nil {
-			log.Error("Unable to check if %s exists. Error: %v", repo.FullName(), err)
-			return err
-		}
-		if isExist {
-			// repo already exists - We have two or three options.
-			// 1. We fail stating that the directory exists
-			// 2. We create the db repository to go with this data and adopt the git repo
-			// 3. We delete it and start afresh
-			//
-			// Previously Gitea would just delete and start afresh - this was naughty.
-			// So we will now fail and delegate to other functionality to adopt or delete
-			log.Error("Files already exist in %s and we are not going to adopt or delete.", repo.FullName())
-			return repo_model.ErrRepoFilesAlreadyExist{
-				Uname: u.Name,
-				Name:  repo.Name,
+			if errDelete := DeleteRepositoryDirectly(ctx, doer, repo.ID); errDelete != nil {
+				log.Error("Rollback deleteRepository: %v", errDelete)
+				// add system notice
+				if err := system_model.CreateRepositoryNotice("DeleteRepositoryDirectly failed when create repository: %v", errDelete); err != nil {
+					log.Error("CreateRepositoryNotice: %v", err)
+				}
 			}
 		}
+	}()
 
-		if err = initRepository(ctx, doer, repo, opts); err != nil {
-			if err2 := gitrepo.DeleteRepository(ctx, repo); err2 != nil {
-				log.Error("initRepository: %v", err)
-				return fmt.Errorf(
-					"delete repo directory %s/%s failed(2): %v", u.Name, repo.Name, err2)
-			}
-			return fmt.Errorf("initRepository: %w", err)
-		}
+	// No need for init mirror.
+	if opts.IsMirror {
+		return repo, nil
+	}
 
-		// Initialize Issue Labels if selected
-		if len(opts.IssueLabels) > 0 {
-			if err = repo_module.InitializeLabels(ctx, repo.ID, opts.IssueLabels, false); err != nil {
-				rollbackRepo = repo
-				rollbackRepo.OwnerID = u.ID
-				return fmt.Errorf("InitializeLabels: %w", err)
-			}
+	var isExist bool
+	isExist, err = gitrepo.IsRepositoryExist(ctx, repo)
+	if err != nil {
+		log.Error("Unable to check if %s exists. Error: %v", repo.FullName(), err)
+		return nil, err
+	}
+	if isExist {
+		// repo already exists in disk - We have two or three options.
+		// 1. We fail stating that the directory exists
+		// 2. We create the db repository to go with this data and adopt the git repo
+		// 3. We delete it and start afresh
+		//
+		// Previously Gitea would just delete and start afresh - this was naughty.
+		// So we will now fail and delegate to other functionality to adopt or delete
+		log.Error("Files already exist in %s and we are not going to adopt or delete.", repo.FullName())
+		return nil, repo_model.ErrRepoFilesAlreadyExist{
+			Uname: u.Name,
+			Name:  repo.Name,
 		}
+	}
 
-		if err := repo_module.CheckDaemonExportOK(ctx, repo); err != nil {
-			return fmt.Errorf("checkDaemonExportOK: %w", err)
+	if err = initRepository(ctx, doer, repo, opts); err != nil {
+		return nil, fmt.Errorf("initRepository: %w", err)
+	}
+
+	// Initialize Issue Labels if selected
+	if len(opts.IssueLabels) > 0 {
+		if err = repo_module.InitializeLabels(ctx, repo.ID, opts.IssueLabels, false); err != nil {
+			return nil, fmt.Errorf("InitializeLabels: %w", err)
 		}
+	}
 
-		if stdout, _, err := git.NewCommand("update-server-info").
-			RunStdString(ctx, &git.RunOpts{Dir: repo.RepoPath()}); err != nil {
-			log.Error("CreateRepository(git update-server-info) in %v: Stdout: %s\nError: %v", repo, stdout, err)
-			rollbackRepo = repo
-			rollbackRepo.OwnerID = u.ID
-			return fmt.Errorf("CreateRepository(git update-server-info): %w", err)
+	if err = repo_module.CheckDaemonExportOK(ctx, repo); err != nil {
+		return nil, fmt.Errorf("checkDaemonExportOK: %w", err)
+	}
+
+	var stdout string
+	if stdout, _, err = git.NewCommand("update-server-info").
+		RunStdString(ctx, &git.RunOpts{Dir: repo.RepoPath()}); err != nil {
+		log.Error("CreateRepository(git update-server-info) in %v: Stdout: %s\nError: %v", repo, stdout, err)
+		return nil, fmt.Errorf("CreateRepository(git update-server-info): %w", err)
+	}
+
+	if needsUpdateStatus {
+		repo.Status = repo_model.RepositoryReady
+		if err = repo_model.UpdateRepositoryCols(ctx, repo, "status"); err != nil {
+			return nil, fmt.Errorf("UpdateRepositoryCols: %w", err)
 		}
+	}
 
-		// update licenses
-		var licenses []string
-		if len(opts.License) > 0 {
-			licenses = append(licenses, opts.License)
+	// update licenses
+	var licenses []string
+	if len(opts.License) > 0 {
+		licenses = append(licenses, opts.License)
 
-			stdout, _, err := git.NewCommand("rev-parse", "HEAD").RunStdString(ctx, &git.RunOpts{Dir: repo.RepoPath()})
-			if err != nil {
-				log.Error("CreateRepository(git rev-parse HEAD) in %v: Stdout: %s\nError: %v", repo, stdout, err)
-				rollbackRepo = repo
-				rollbackRepo.OwnerID = u.ID
-				return fmt.Errorf("CreateRepository(git rev-parse HEAD): %w", err)
-			}
-			if err := repo_model.UpdateRepoLicenses(ctx, repo, stdout, licenses); err != nil {
-				return err
-			}
+		stdout, _, err = git.NewCommand("rev-parse", "HEAD").RunStdString(ctx, &git.RunOpts{Dir: repo.RepoPath()})
+		if err != nil {
+			log.Error("CreateRepository(git rev-parse HEAD) in %v: Stdout: %s\nError: %v", repo, stdout, err)
+			return nil, fmt.Errorf("CreateRepository(git rev-parse HEAD): %w", err)
 		}
-		return nil
-	}); err != nil {
-		if rollbackRepo != nil {
-			if errDelete := DeleteRepositoryDirectly(ctx, doer, rollbackRepo.ID); errDelete != nil {
-				log.Error("Rollback deleteRepository: %v", errDelete)
-			}
+		if err = repo_model.UpdateRepoLicenses(ctx, repo, stdout, licenses); err != nil {
+			return nil, err
 		}
-
-		return nil, err
 	}
 
 	return repo, nil
 }
 
-// CreateRepositoryByExample creates a repository for the user/organization.
-func CreateRepositoryByExample(ctx context.Context, doer, u *user_model.User, repo *repo_model.Repository, overwriteOrAdopt, isFork bool) (err error) {
+// CreateRepositoryInDB creates a repository for the user/organization.
+func CreateRepositoryInDB(ctx context.Context, doer, u *user_model.User, repo *repo_model.Repository, overwriteOrAdopt, isFork bool) (err error) {
 	if err = repo_model.IsUsableRepoName(repo.Name); err != nil {
 		return err
 	}
@@ -357,7 +363,17 @@ func CreateRepositoryByExample(ctx context.Context, doer, u *user_model.User, re
 		log.Error("Unable to check if %s exists. Error: %v", repo.FullName(), err)
 		return err
 	}
-	if !overwriteOrAdopt && isExist {
+	if overwriteOrAdopt != isExist {
+		if overwriteOrAdopt {
+			// repo should exist but doesn't - We have two or three options.
+			log.Error("Files do not exist in %s and we are not going to adopt or delete.", repo.FullName())
+			return repo_model.ErrRepoNotExist{
+				OwnerName: u.Name,
+				Name:      repo.Name,
+			}
+		}
+
+		// repo already exists - We have two or three options.
 		log.Error("Files already exist in %s and we are not going to adopt or delete.", repo.FullName())
 		return repo_model.ErrRepoFilesAlreadyExist{
 			Uname: u.Name,