Skip to content

Commit a100ac3

Browse files
lunnylunny
authored
Rework create/fork/adopt/generate repository to make sure resources will be cleanup once failed (#31035)
Fix #28144 To make the resources will be cleanup once failed. All repository operations now follow a consistent pattern: - 1. Create a database record for the repository with the status being_migrated. - 2. Register a deferred cleanup function to delete the repository and its related data if the operation fails. - 3. Perform the actual Git and database operations step by step. - 4. Upon successful completion, update the repository’s status to ready. The adopt operation is a special case — if it fails, the repository on disk should not be deleted.
1 parent 90b509a commit a100ac3

File tree

14 files changed

+520
-348
lines changed

14 files changed

+520
-348
lines changed

‎models/git/branch.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,11 @@ func GetDeletedBranchByID(ctx context.Context, repoID, branchID int64) (*Branch,
235235
return &branch, nil
236236
}
237237

238+
func DeleteRepoBranches(ctx context.Context, repoID int64) error {
239+
_, err := db.GetEngine(ctx).Where("repo_id=?", repoID).Delete(new(Branch))
240+
return err
241+
}
242+
238243
func DeleteBranches(ctx context.Context, repoID, doerID int64, branchIDs []int64) error {
239244
return db.WithTx(ctx, func(ctx context.Context) error {
240245
branches := make([]*Branch, 0, len(branchIDs))

‎models/repo/release.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -558,3 +558,8 @@ func FindTagsByCommitIDs(ctx context.Context, repoID int64, commitIDs ...string)
558558
}
559559
return res, nil
560560
}
561+
562+
func DeleteRepoReleases(ctx context.Context, repoID int64) error {
563+
_, err := db.GetEngine(ctx).Where("repo_id = ?", repoID).Delete(new(Release))
564+
return err
565+
}

‎modules/repository/init.go

Lines changed: 0 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,7 @@ import (
1111
"strings"
1212

1313
issues_model "code.gitea.io/gitea/models/issues"
14-
repo_model "code.gitea.io/gitea/models/repo"
15-
"code.gitea.io/gitea/modules/git"
16-
"code.gitea.io/gitea/modules/gitrepo"
1714
"code.gitea.io/gitea/modules/label"
18-
"code.gitea.io/gitea/modules/log"
1915
"code.gitea.io/gitea/modules/options"
2016
"code.gitea.io/gitea/modules/setting"
2117
"code.gitea.io/gitea/modules/util"
@@ -121,29 +117,6 @@ func LoadRepoConfig() error {
121117
return nil
122118
}
123119

124-
func CheckInitRepository(ctx context.Context, repo *repo_model.Repository) (err error) {
125-
// Somehow the directory could exist.
126-
isExist, err := gitrepo.IsRepositoryExist(ctx, repo)
127-
if err != nil {
128-
log.Error("Unable to check if %s exists. Error: %v", repo.FullName(), err)
129-
return err
130-
}
131-
if isExist {
132-
return repo_model.ErrRepoFilesAlreadyExist{
133-
Uname: repo.OwnerName,
134-
Name: repo.Name,
135-
}
136-
}
137-
138-
// Init git bare new repository.
139-
if err = git.InitRepository(ctx, repo.RepoPath(), true, repo.ObjectFormatName); err != nil {
140-
return fmt.Errorf("git.InitRepository: %w", err)
141-
} else if err = gitrepo.CreateDelegateHooks(ctx, repo); err != nil {
142-
return fmt.Errorf("createDelegateHooks: %w", err)
143-
}
144-
return nil
145-
}
146-
147120
// InitializeLabels adds a label set to a repository using a template
148121
func InitializeLabels(ctx context.Context, id int64, labelTemplate string, isOrg bool) error {
149122
list, err := LoadTemplateLabelsByDisplayName(labelTemplate)

‎services/repository/adopt.go

Lines changed: 45 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import (
1616
repo_model "code.gitea.io/gitea/models/repo"
1717
user_model "code.gitea.io/gitea/models/user"
1818
"code.gitea.io/gitea/modules/container"
19-
"code.gitea.io/gitea/modules/git"
2019
"code.gitea.io/gitea/modules/gitrepo"
2120
"code.gitea.io/gitea/modules/log"
2221
"code.gitea.io/gitea/modules/optional"
@@ -28,6 +27,18 @@ import (
2827
"github.com/gobwas/glob"
2928
)
3029

30+
func deleteFailedAdoptRepository(repoID int64) error {
31+
return db.WithTx(db.DefaultContext, func(ctx context.Context) error {
32+
if err := deleteDBRepository(ctx, repoID); err != nil {
33+
return fmt.Errorf("deleteDBRepository: %w", err)
34+
}
35+
if err := git_model.DeleteRepoBranches(ctx, repoID); err != nil {
36+
return fmt.Errorf("deleteRepoBranches: %w", err)
37+
}
38+
return repo_model.DeleteRepoReleases(ctx, repoID)
39+
})
40+
}
41+
3142
// AdoptRepository adopts pre-existing repository files for the user/organization.
3243
func AdoptRepository(ctx context.Context, doer, u *user_model.User, opts CreateRepoOptions) (*repo_model.Repository, error) {
3344
if !doer.IsAdmin && !u.CanCreateRepo() {
@@ -48,58 +59,51 @@ func AdoptRepository(ctx context.Context, doer, u *user_model.User, opts CreateR
4859
IsPrivate: opts.IsPrivate,
4960
IsFsckEnabled: !opts.IsMirror,
5061
CloseIssuesViaCommitInAnyBranch: setting.Repository.DefaultCloseIssuesViaCommitsInAnyBranch,
51-
Status: opts.Status,
62+
Status: repo_model.RepositoryBeingMigrated,
5263
IsEmpty: !opts.AutoInit,
5364
}
5465

55-
if err := db.WithTx(ctx, func(ctx context.Context) error {
56-
isExist, err := gitrepo.IsRepositoryExist(ctx, repo)
66+
// 1 - create the repository database operations first
67+
err := db.WithTx(ctx, func(ctx context.Context) error {
68+
return createRepositoryInDB(ctx, doer, u, repo, false)
69+
})
70+
if err != nil {
71+
return nil, err
72+
}
73+
74+
// last - clean up if something goes wrong
75+
// WARNING: Don't override all later err with local variables
76+
defer func() {
5777
if err != nil {
58-
log.Error("Unable to check if %s exists. Error: %v", repo.FullName(), err)
59-
return err
60-
}
61-
if !isExist {
62-
return repo_model.ErrRepoNotExist{
63-
OwnerName: u.Name,
64-
Name: repo.Name,
78+
// we can not use the ctx because it maybe canceled or timeout
79+
if errDel := deleteFailedAdoptRepository(repo.ID); errDel != nil {
80+
log.Error("Failed to delete repository %s that could not be adopted: %v", repo.FullName(), errDel)
6581
}
6682
}
83+
}()
6784

68-
if err := CreateRepositoryByExample(ctx, doer, u, repo, true, false); err != nil {
69-
return err
70-
}
71-
72-
// Re-fetch the repository from database before updating it (else it would
73-
// override changes that were done earlier with sql)
74-
if repo, err = repo_model.GetRepositoryByID(ctx, repo.ID); err != nil {
75-
return fmt.Errorf("getRepositoryByID: %w", err)
76-
}
77-
return nil
78-
}); err != nil {
79-
return nil, err
85+
// Re-fetch the repository from database before updating it (else it would
86+
// override changes that were done earlier with sql)
87+
if repo, err = repo_model.GetRepositoryByID(ctx, repo.ID); err != nil {
88+
return nil, fmt.Errorf("getRepositoryByID: %w", err)
8089
}
8190

82-
if err := func() error {
83-
if err := adoptRepository(ctx, repo, opts.DefaultBranch); err != nil {
84-
return fmt.Errorf("adoptRepository: %w", err)
85-
}
91+
// 2 - adopt the repository from disk
92+
if err = adoptRepository(ctx, repo, opts.DefaultBranch); err != nil {
93+
return nil, fmt.Errorf("adoptRepository: %w", err)
94+
}
8695

87-
if err := repo_module.CheckDaemonExportOK(ctx, repo); err != nil {
88-
return fmt.Errorf("checkDaemonExportOK: %w", err)
89-
}
96+
// 3 - Update the git repository
97+
if err = updateGitRepoAfterCreate(ctx, repo); err != nil {
98+
return nil, fmt.Errorf("updateGitRepoAfterCreate: %w", err)
99+
}
90100

91-
if stdout, _, err := git.NewCommand("update-server-info").
92-
RunStdString(ctx, &git.RunOpts{Dir: repo.RepoPath()}); err != nil {
93-
log.Error("CreateRepository(git update-server-info) in %v: Stdout: %s\nError: %v", repo, stdout, err)
94-
return fmt.Errorf("CreateRepository(git update-server-info): %w", err)
95-
}
96-
return nil
97-
}(); err != nil {
98-
if errDel := DeleteRepository(ctx, doer, repo, false /* no notify */); errDel != nil {
99-
log.Error("Failed to delete repository %s that could not be adopted: %v", repo.FullName(), errDel)
100-
}
101-
return nil, err
101+
// 4 - update repository status
102+
repo.Status = repo_model.RepositoryReady
103+
if err = repo_model.UpdateRepositoryCols(ctx, repo, "status"); err != nil {
104+
return nil, fmt.Errorf("UpdateRepositoryCols: %w", err)
102105
}
106+
103107
notify_service.AdoptRepository(ctx, doer, u, repo)
104108

105109
return repo, nil

‎services/repository/adopt_test.go

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"code.gitea.io/gitea/models/unittest"
1515
user_model "code.gitea.io/gitea/models/user"
1616
"code.gitea.io/gitea/modules/setting"
17+
"code.gitea.io/gitea/modules/util"
1718

1819
"github.com/stretchr/testify/assert"
1920
)
@@ -89,10 +90,36 @@ func TestListUnadoptedRepositories_ListOptions(t *testing.T) {
8990

9091
func TestAdoptRepository(t *testing.T) {
9192
assert.NoError(t, unittest.PrepareTestDatabase())
92-
assert.NoError(t, unittest.SyncDirs(filepath.Join(setting.RepoRootPath, "user2", "repo1.git"), filepath.Join(setting.RepoRootPath, "user2", "test-adopt.git")))
93+
9394
user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
94-
_, err := AdoptRepository(db.DefaultContext, user2, user2, CreateRepoOptions{Name: "test-adopt"})
95+
96+
// a successful adopt
97+
destDir := filepath.Join(setting.RepoRootPath, user2.Name, "test-adopt.git")
98+
assert.NoError(t, unittest.SyncDirs(filepath.Join(setting.RepoRootPath, user2.Name, "repo1.git"), destDir))
99+
100+
adoptedRepo, err := AdoptRepository(db.DefaultContext, user2, user2, CreateRepoOptions{Name: "test-adopt"})
95101
assert.NoError(t, err)
96102
repoTestAdopt := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{Name: "test-adopt"})
97103
assert.Equal(t, "sha1", repoTestAdopt.ObjectFormatName)
104+
105+
// just delete the adopted repo's db records
106+
err = deleteFailedAdoptRepository(adoptedRepo.ID)
107+
assert.NoError(t, err)
108+
109+
unittest.AssertNotExistsBean(t, &repo_model.Repository{OwnerName: user2.Name, Name: "test-adopt"})
110+
111+
// a failed adopt because some mock data
112+
// remove the hooks directory and create a file so that we cannot create the hooks successfully
113+
_ = os.RemoveAll(filepath.Join(destDir, "hooks", "update.d"))
114+
assert.NoError(t, os.WriteFile(filepath.Join(destDir, "hooks", "update.d"), []byte("tests"), os.ModePerm))
115+
116+
adoptedRepo, err = AdoptRepository(db.DefaultContext, user2, user2, CreateRepoOptions{Name: "test-adopt"})
117+
assert.Error(t, err)
118+
assert.Nil(t, adoptedRepo)
119+
120+
unittest.AssertNotExistsBean(t, &repo_model.Repository{OwnerName: user2.Name, Name: "test-adopt"})
121+
122+
exist, err := util.IsExist(repo_model.RepoPath(user2.Name, "test-adopt"))
123+
assert.NoError(t, err)
124+
assert.True(t, exist) // the repository should be still in the disk
98125
}

0 commit comments

Comments
 (0)