fix: cleanup data before migration retry (#12370)

In the case you hit some API error (Github ratelimit was often a problem) or the instance restarted in the middle of your migration, you would be left with data on the disk and/or database. Upon retrying the migration the migration code would (rightfully) fail because it's trying to migrate stuff that already exists.

This was hit so often on Codeberg it was better to force people to delete and start whole migration process again: 28ee60c91f

Delete the repository data before retrying to solve this.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12370
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
This commit is contained in:
Gusted 2026-05-05 12:41:42 +02:00 committed by Gusted
parent 6f5bef54b0
commit c07ea09050
21 changed files with 191 additions and 71 deletions

View file

@ -14,10 +14,8 @@ import (
asymkey_model "forgejo.org/models/asymkey"
"forgejo.org/models/db"
issues_model "forgejo.org/models/issues"
access_model "forgejo.org/models/perm/access"
repo_model "forgejo.org/models/repo"
"forgejo.org/models/unit"
user_model "forgejo.org/models/user"
"forgejo.org/modules/log"
"forgejo.org/services/stats"
@ -277,7 +275,7 @@ func DoctorUserStarNum(ctx context.Context) (err error) {
}
// DeleteDeployKey delete deploy keys
func DeleteDeployKey(ctx context.Context, doer *user_model.User, id int64) error {
func DeleteDeployKey(ctx context.Context, id, repoID int64) error {
key, err := asymkey_model.GetDeployKeyByID(ctx, id)
if err != nil {
if asymkey_model.IsErrDeployKeyNotExist(err) {
@ -286,21 +284,10 @@ func DeleteDeployKey(ctx context.Context, doer *user_model.User, id int64) error
return fmt.Errorf("GetDeployKeyByID: %w", err)
}
// Check if user has access to delete this key.
if !doer.IsAdmin {
repo, err := repo_model.GetRepositoryByID(ctx, key.RepoID)
if err != nil {
return fmt.Errorf("GetRepositoryByID: %w", err)
}
has, err := access_model.IsUserRepoAdmin(ctx, repo, doer)
if err != nil {
return fmt.Errorf("GetUserRepoPermission: %w", err)
} else if !has {
return asymkey_model.ErrKeyAccessDenied{
UserID: doer.ID,
KeyID: key.ID,
Note: "deploy",
}
if key.RepoID != repoID {
return asymkey_model.ErrKeyAccessDenied{
KeyID: key.ID,
Note: "deploy",
}
}

View file

@ -279,7 +279,7 @@ func DeleteDeploykey(ctx *context.APIContext) {
// "404":
// "$ref": "#/responses/notFound"
if err := asymkey_service.DeleteDeployKey(ctx, ctx.Doer, ctx.ParamsInt64(":id")); err != nil {
if err := asymkey_service.DeleteDeployKey(ctx, ctx.ParamsInt64(":id"), ctx.Repo.Repository.ID); err != nil {
if asymkey_model.IsErrKeyAccessDenied(err) {
ctx.Error(http.StatusForbidden, "", "You do not have access to this key")
} else {

View file

@ -205,7 +205,7 @@ func Migrate(ctx *context.APIContext) {
}
if repo != nil {
if errDelete := repo_service.DeleteRepositoryDirectly(ctx, ctx.Doer, repo.ID); errDelete != nil {
if errDelete := repo_service.DeleteRepositoryDirectly(ctx, repo.ID, repo_service.DeleteRepositoryOpts{}); errDelete != nil {
log.Error("DeleteRepository: %v", errDelete)
}
}

View file

@ -269,7 +269,6 @@ func setMigrationContextData(ctx *context.Context, serviceType structs.GitServic
func MigrateRetryPost(ctx *context.Context) {
ok, err := quota_model.EvaluateForUser(ctx, ctx.Repo.Repository.OwnerID, quota_model.LimitSubjectSizeReposAll)
if err != nil {
log.Error("quota_model.EvaluateForUser: %v", err)
ctx.ServerError("quota_model.EvaluateForUser", err)
return
}
@ -287,7 +286,6 @@ func MigrateRetryPost(ctx *context.Context) {
}
if err := task.RetryMigrateTask(ctx, ctx.Repo.Repository.ID); err != nil {
log.Error("Retry task failed: %v", err)
ctx.ServerError("task.RetryMigrateTask", err)
return
}

View file

@ -99,7 +99,7 @@ func DeployKeysPost(ctx *context.Context) {
// DeleteDeployKey response for deleting a deploy key
func DeleteDeployKey(ctx *context.Context) {
if err := asymkey_service.DeleteDeployKey(ctx, ctx.Doer, ctx.FormInt64("id")); err != nil {
if err := asymkey_service.DeleteDeployKey(ctx, ctx.FormInt64("id"), ctx.Repo.Repository.ID); err != nil {
ctx.Flash.Error("DeleteDeployKey: " + err.Error())
} else {
ctx.Flash.Success(ctx.Tr("repo.settings.deploy_key_deletion_success"))

View file

@ -9,21 +9,13 @@ import (
"forgejo.org/models"
asymkey_model "forgejo.org/models/asymkey"
"forgejo.org/models/db"
user_model "forgejo.org/models/user"
)
// DeleteDeployKey deletes deploy key from its repository authorized_keys file if needed.
func DeleteDeployKey(ctx context.Context, doer *user_model.User, id int64) error {
dbCtx, committer, err := db.TxContext(ctx)
if err != nil {
return err
}
defer committer.Close()
if err := models.DeleteDeployKey(dbCtx, doer, id); err != nil {
return err
}
if err := committer.Commit(); err != nil {
func DeleteDeployKey(ctx context.Context, id, repoID int64) error {
if err := db.WithTx(ctx, func(ctx context.Context) error {
return models.DeleteDeployKey(ctx, id, repoID)
}); err != nil {
return err
}

View file

@ -101,8 +101,8 @@ func registerDeleteMissingRepositories() {
Enabled: false,
RunAtStart: false,
Schedule: "@every 72h",
}, func(ctx context.Context, user *user_model.User, _ Config) error {
return repo_service.DeleteMissingRepositories(ctx, user)
}, func(ctx context.Context, _ *user_model.User, _ Config) error {
return repo_service.DeleteMissingRepositories(ctx)
})
}

View file

@ -7,7 +7,6 @@ import (
"context"
"forgejo.org/models/db"
user_model "forgejo.org/models/user"
"forgejo.org/modules/log"
"forgejo.org/modules/storage"
repo_service "forgejo.org/services/repository"
@ -39,7 +38,6 @@ func deleteOrphanedRepos(ctx context.Context) (int64, error) {
batchSize := db.MaxBatchInsertSize("repository")
e := db.GetEngine(ctx)
var deleted int64
adminUser := &user_model.User{IsAdmin: true}
for {
select {
@ -60,7 +58,7 @@ func deleteOrphanedRepos(ctx context.Context) (int64, error) {
}
for _, id := range ids {
if err := repo_service.DeleteRepositoryDirectly(ctx, adminUser, id, true); err != nil {
if err := repo_service.DeleteRepositoryDirectly(ctx, id, repo_service.DeleteRepositoryOpts{IgnoreOrgTeams: true}); err != nil {
return deleted, err
}
deleted++

View file

@ -12,7 +12,6 @@ import (
"forgejo.org/models/db"
repo_model "forgejo.org/models/repo"
system_model "forgejo.org/models/system"
user_model "forgejo.org/models/user"
"forgejo.org/modules/git"
"forgejo.org/modules/log"
repo_module "forgejo.org/modules/repository"
@ -146,7 +145,7 @@ func gatherMissingRepoRecords(ctx context.Context) (repo_model.RepositoryList, e
}
// DeleteMissingRepositories deletes all repository records that lost Git files.
func DeleteMissingRepositories(ctx context.Context, doer *user_model.User) error {
func DeleteMissingRepositories(ctx context.Context) error {
repos, err := gatherMissingRepoRecords(ctx)
if err != nil {
return err
@ -163,7 +162,7 @@ func DeleteMissingRepositories(ctx context.Context, doer *user_model.User) error
default:
}
log.Trace("Deleting %d/%d...", repo.OwnerID, repo.ID)
if err := DeleteRepositoryDirectly(ctx, doer, repo.ID); err != nil {
if err := DeleteRepositoryDirectly(ctx, repo.ID, DeleteRepositoryOpts{}); err != nil {
log.Error("Failed to DeleteRepository %-v: Error: %v", repo, err)
if err2 := system_model.CreateRepositoryNotice("Failed to DeleteRepository %s [%d]: Error: %v", repo.FullName(), repo.ID, err); err2 != nil {
log.Error("CreateRepositoryNotice: %v", err)

View file

@ -308,7 +308,7 @@ func CreateRepositoryDirectly(ctx context.Context, doer, u *user_model.User, opt
return nil
}); err != nil {
if rollbackRepo != nil {
if errDelete := DeleteRepositoryDirectly(ctx, doer, rollbackRepo.ID); errDelete != nil {
if errDelete := DeleteRepositoryDirectly(ctx, rollbackRepo.ID, DeleteRepositoryOpts{}); errDelete != nil {
log.Error("Rollback deleteRepository: %v", errDelete)
}
}

View file

@ -130,7 +130,7 @@ func TestIncludesAllRepositoriesTeams(t *testing.T) {
}
// Remove repo and check teams repositories.
require.NoError(t, DeleteRepositoryDirectly(db.DefaultContext, user, repoIDs[0]), "DeleteRepository")
require.NoError(t, DeleteRepositoryDirectly(db.DefaultContext, repoIDs[0], DeleteRepositoryOpts{}), "DeleteRepository")
teamRepos[0] = repoIDs[1:]
teamRepos[1] = repoIDs[1:]
teamRepos[3] = repoIDs[1:3]
@ -142,7 +142,7 @@ func TestIncludesAllRepositoriesTeams(t *testing.T) {
// Wipe created items.
for i, rid := range repoIDs {
if i > 0 { // first repo already deleted.
require.NoError(t, DeleteRepositoryDirectly(db.DefaultContext, user, rid), "DeleteRepository %d", i)
require.NoError(t, DeleteRepositoryDirectly(db.DefaultContext, rid, DeleteRepositoryOpts{}), "DeleteRepository %d", i)
}
}
require.NoError(t, organization.DeleteOrganization(db.DefaultContext, org), "DeleteOrganization")

View file

@ -38,9 +38,17 @@ import (
"xorm.io/builder"
)
type DeleteRepositoryOpts struct {
// Don't modify teams if they are attached to this repository.
IgnoreOrgTeams bool
// Keep migration-related beans. Should only be used to cleanup data to
// start another migration.
KeepMigrationBeans bool
}
// DeleteRepository deletes a repository for a user or organization.
// make sure if you call this func to close open sessions (sqlite will otherwise get a deadlock)
func DeleteRepositoryDirectly(ctx context.Context, doer *user_model.User, repoID int64, ignoreOrgTeams ...bool) error {
func DeleteRepositoryDirectly(ctx context.Context, repoID int64, opts DeleteRepositoryOpts) error {
ctx, committer, err := db.TxContext(ctx)
if err != nil {
return err
@ -75,7 +83,7 @@ func DeleteRepositoryDirectly(ctx context.Context, doer *user_model.User, repoID
// In case owner is a organization, we have to change repo specific teams
// if ignoreOrgTeams is not true
var org *user_model.User
if len(ignoreOrgTeams) == 0 || !ignoreOrgTeams[0] {
if !opts.IgnoreOrgTeams {
if org, err = user_model.GetUserByID(ctx, repo.OwnerID); err != nil {
return err
}
@ -88,7 +96,7 @@ func DeleteRepositoryDirectly(ctx context.Context, doer *user_model.User, repoID
}
needRewriteKeysFile := len(deployKeys) > 0
for _, dKey := range deployKeys {
if err := models.DeleteDeployKey(ctx, doer, dKey.ID); err != nil {
if err := models.DeleteDeployKey(ctx, dKey.ID, repoID); err != nil {
return fmt.Errorf("deleteDeployKeys: %w", err)
}
}
@ -173,9 +181,7 @@ func DeleteRepositoryDirectly(ctx context.Context, doer *user_model.User, repoID
&repo_model.Release{RepoID: repoID},
&repo_model.RepoIndexerStatus{RepoID: repoID},
&repo_model.Redirect{RedirectRepoID: repoID},
&repo_model.RepoUnit{RepoID: repoID},
&repo_model.Star{RepoID: repoID},
&admin_model.Task{RepoID: repoID},
&repo_model.Watch{RepoID: repoID},
&webhook.Webhook{RepoID: repoID},
&secret_model.Secret{RepoID: repoID},
@ -195,18 +201,29 @@ func DeleteRepositoryDirectly(ctx context.Context, doer *user_model.User, repoID
return fmt.Errorf("deleteBeans: %w", err)
}
if !opts.KeepMigrationBeans {
if err := db.DeleteBeans(ctx,
&admin_model.Task{RepoID: repoID},
&repo_model.RepoUnit{RepoID: repoID},
); err != nil {
return fmt.Errorf("deleteBeans: %w", err)
}
}
// Delete Pulls and related objects
if err := issues_model.DeletePullsByBaseRepoID(ctx, repoID); err != nil {
return err
}
if cnt, err := sess.ID(repoID).Delete(&repo_model.Repository{}); err != nil {
return err
} else if cnt != 1 {
return repo_model.ErrRepoNotExist{
ID: repoID,
OwnerName: "",
Name: "",
if !opts.KeepMigrationBeans {
if cnt, err := sess.ID(repoID).Delete(&repo_model.Repository{}); err != nil {
return err
} else if cnt != 1 {
return repo_model.ErrRepoNotExist{
ID: repoID,
OwnerName: "",
Name: "",
}
}
}
@ -491,7 +508,7 @@ func DeleteOwnerRepositoriesDirectly(ctx context.Context, owner *user_model.User
break
}
for _, repo := range repos {
if err := DeleteRepositoryDirectly(ctx, owner, repo.ID); err != nil {
if err := DeleteRepositoryDirectly(ctx, repo.ID, DeleteRepositoryOpts{}); err != nil {
return fmt.Errorf("unable to delete repository %s for %s[%d]. Error: %w", repo.Name, owner.Name, owner.ID, err)
}
}

View file

@ -63,7 +63,7 @@ func DeleteRepository(ctx context.Context, doer *user_model.User, repo *repo_mod
notify_service.DeleteRepository(ctx, doer, repo)
}
return DeleteRepositoryDirectly(ctx, doer, repo.ID)
return DeleteRepositoryDirectly(ctx, repo.ID, DeleteRepositoryOpts{})
}
// PushCreateRepo creates a repository when a new repository is pushed to an appropriate namespace

View file

@ -0,0 +1,16 @@
-
id: 1001
repo_id: 20
index: 1
poster_id: 1
original_author_id: 0
name: eepy
content: I am part of a migration.
milestone_id: 0
priority: 0
is_closed: false
is_pull: false
num_comments: 0
created_unix: 1777647620
updated_unix: 1777647620
is_locked: false

View file

@ -0,0 +1,34 @@
-
id: 1001
repo_id: 20
type: 1
config: "{}"
created_unix: 1777646333
-
id: 1002
repo_id: 20
type: 2
config: "{\"EnableTimetracker\":false,\"AllowOnlyContributorsToTrackTime\":false}"
created_unix: 1777646333
-
id: 1003
repo_id: 20
type: 3
config: "{\"IgnoreWhitespaceConflicts\":true,\"AllowMerge\":true,\"AllowRebase\":false,\"AllowRebaseMerge\":true,\"AllowSquash\":false}"
created_unix: 1777646333
-
id: 1004
repo_id: 20
type: 4
config: "{}"
created_unix: 1777646333
-
id: 1005
repo_id: 20
type: 5
config: "{}"
created_unix: 1777646333

View file

@ -0,0 +1,22 @@
-
id: 1001
doer_id: 2
owner_id: 2
repo_id: 1
type: 0
status: 4
start_time: 1777645339
end_time: 1777645339
created: 1777645339
-
id: 1002
doer_id: 2
owner_id: 2
repo_id: 20
type: 0
status: 3
message: 'canceled'
start_time: 1777645339
end_time: 1777645339
created: 1777645339

View file

@ -133,25 +133,31 @@ func CreateMigrateTask(ctx context.Context, doer, u *user_model.User, opts base.
return task, nil
}
// RetryMigrateTask retry a migrate task
// RetryMigrateTask will retry the migration.
// All data, from a previous migration, is deleted before it's retried.
func RetryMigrateTask(ctx context.Context, repoID int64) error {
migratingTask, err := admin_model.GetMigratingTask(ctx, repoID)
if err != nil {
log.Error("GetMigratingTask: %v", err)
return err
return fmt.Errorf("GetMigratingTask: %w", err)
}
if migratingTask.Status == structs.TaskStatusQueued || migratingTask.Status == structs.TaskStatusRunning {
return nil
}
// TODO Need to removing the storage/database garbage brought by the failed task
// The migration is being retried, it could've failed for a variety of cases.
// In most cases however, some data already got uploaded to the disk or
// database. The migration code makes the assumption this is not the case and
// if we do not clean it up, the retry attempt will fail with absolute
// certainty.
if err := repo_service.DeleteRepositoryDirectly(ctx, repoID, repo_service.DeleteRepositoryOpts{IgnoreOrgTeams: true, KeepMigrationBeans: true}); err != nil {
return fmt.Errorf("DeleteRepositoryDirectly: %v", err)
}
// Reset task status and messages
migratingTask.Status = structs.TaskStatusQueued
migratingTask.Message = ""
if err = migratingTask.UpdateCols(ctx, "status", "message"); err != nil {
log.Error("task.UpdateCols failed: %v", err)
return err
return fmt.Errorf("task.UpdateCols: %w", err)
}
return taskQueue.Push(migratingTask)

View file

@ -1,12 +1,21 @@
// Copyright 2025 The Forgejo Authors. All rights reserved.
// SPDX-License-Identifier: GPL-3.0-or-later
package task
import (
"testing"
admin_model "forgejo.org/models/admin"
issues_model "forgejo.org/models/issues"
repo_model "forgejo.org/models/repo"
"forgejo.org/models/unittest"
user_model "forgejo.org/models/user"
"forgejo.org/modules/migration"
"forgejo.org/modules/queue"
"forgejo.org/modules/setting"
"forgejo.org/modules/structs"
"forgejo.org/modules/test"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
@ -50,3 +59,46 @@ func TestCreateMigrateTask(t *testing.T) {
assert.Equal(t, "https://admin:password@example.com", config.CloneAddr)
})
}
func TestRetryMigrateTask(t *testing.T) {
defer unittest.OverrideFixtures("services/task/fixtures/TestRetryMigrateTask/")()
require.NoError(t, unittest.PrepareTestDatabase())
t.Run("Migrate task does not exist", func(t *testing.T) {
err := RetryMigrateTask(t.Context(), 100)
require.ErrorIs(t, err, admin_model.ErrTaskDoesNotExist{RepoID: 100})
})
t.Run("Normal", func(t *testing.T) {
// Override the task queue temporarily.
called := false
testQueue, err := queue.NewWorkerPoolQueueWithContext(t.Context(), "task", setting.QueueSettings{Type: "immediate"}, func(items ...*admin_model.Task) []*admin_model.Task {
if assert.Len(t, items, 1) {
assert.Empty(t, items[0].Message)
assert.Equal(t, structs.TaskStatusQueued, items[0].Status)
assert.EqualValues(t, 1002, items[0].ID)
}
called = true
return nil
}, true)
require.NoError(t, err)
defer test.MockVariableValue(&taskQueue, testQueue)()
// Preconditions.
unittest.AssertExistsIf(t, true, &repo_model.Repository{ID: 20})
unittest.AssertExistsIf(t, true, &admin_model.Task{RepoID: 20, Status: structs.TaskStatusFailed})
unittest.AssertExistsIf(t, true, &issues_model.Issue{RepoID: 20})
unittest.AssertCount(t, &repo_model.RepoUnit{RepoID: 20}, 5)
require.NoError(t, RetryMigrateTask(t.Context(), 20))
// Verify queue was called.
assert.True(t, called)
// Verify some beans were NOT deleted.
unittest.AssertExistsIf(t, true, &repo_model.Repository{ID: 20})
unittest.AssertExistsIf(t, true, &admin_model.Task{RepoID: 20, Status: structs.TaskStatusQueued})
unittest.AssertCount(t, &repo_model.RepoUnit{RepoID: 20}, 5)
// Verify some beans were deleted.
unittest.AssertExistsIf(t, false, &issues_model.Issue{RepoID: 20})
})
}

View file

@ -957,7 +957,7 @@ func TestAPIRepoTransfer(t *testing.T) {
// cleanup
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: apiRepo.ID})
_ = repo_service.DeleteRepositoryDirectly(db.DefaultContext, user, repo.ID)
require.NoError(t, repo_service.DeleteRepositoryDirectly(db.DefaultContext, repo.ID, repo_service.DeleteRepositoryOpts{}))
}
// This test verifies that a repo-specific access token with `write:repository` scope is not a sufficient to transfer a

View file

@ -116,8 +116,7 @@ func TestEphemeralRunnerDeletionOnRepositoryDeletion(t *testing.T) {
task := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionTask{ID: 10054})
assert.Equal(t, actions_model.StatusRunning, task.Status)
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5})
err = repo_service.DeleteRepositoryDirectly(t.Context(), user, task.RepoID, true)
err = repo_service.DeleteRepositoryDirectly(t.Context(), task.RepoID, repo_service.DeleteRepositoryOpts{IgnoreOrgTeams: true})
require.NoError(t, err)
_, err = actions_model.GetRunnerByID(t.Context(), 10000008)

View file

@ -201,7 +201,7 @@ func runTestGitPush(t *testing.T, u *url.URL, objectFormat git.ObjectFormat, git
assert.Equal(t, commitID, branch.CommitID)
}
require.NoError(t, repo_service.DeleteRepositoryDirectly(db.DefaultContext, user, repo.ID))
require.NoError(t, repo_service.DeleteRepositoryDirectly(db.DefaultContext, repo.ID, repo_service.DeleteRepositoryOpts{}))
}
func TestOptionsGitPush(t *testing.T) {
@ -310,6 +310,6 @@ func testOptionsGitPush(t *testing.T, u *url.URL) {
assert.True(t, logFiltered[0])
})
require.NoError(t, repo_service.DeleteRepositoryDirectly(db.DefaultContext, user, repo.ID))
require.NoError(t, repo_service.DeleteRepositoryDirectly(db.DefaultContext, repo.ID, repo_service.DeleteRepositoryOpts{}))
})
}