From af3f1955f73b1df05253faee0b563ed0a90409df Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sat, 6 Nov 2021 23:46:03 +0000 Subject: [PATCH 1/4] Correctly handle failed migrations There is a bug in handling failed migrations whereby the migration task gets decoupled from the migration repository. This leads to a failure of the task to get deleted with the repository and also leads to the migration failed page resulting in a ISE. This PR removes the zeroing out of the task id from the migration but also makes the migration handler tolerate missing tasks much nicer. Fix #17571 Signed-off-by: Andrew Thornton --- modules/task/migrate.go | 5 +++-- modules/task/task.go | 6 +++--- options/locale/locale_en-US.ini | 1 + routers/web/repo/view.go | 7 +++++++ routers/web/user/task.go | 7 +++++++ templates/repo/migrate/migrating.tmpl | 6 +++++- 6 files changed, 26 insertions(+), 6 deletions(-) diff --git a/modules/task/migrate.go b/modules/task/migrate.go index 52f4bb91cf722..aca32a73c0415 100644 --- a/modules/task/migrate.go +++ b/modules/task/migrate.go @@ -58,11 +58,12 @@ func runMigrateTask(t *models.Task) (err error) { t.EndTime = timeutil.TimeStampNow() t.Status = structs.TaskStatusFailed t.Message = err.Error() - t.RepoID = 0 - if err := t.UpdateCols("status", "errors", "repo_id", "end_time"); err != nil { + if err := t.UpdateCols("status", "errors", "end_time"); err != nil { log.Error("Task UpdateCols failed: %v", err) } + _ = t.LoadRepo() + if t.Repo != nil { if errDelete := models.DeleteRepository(t.Doer, t.OwnerID, t.Repo.ID); errDelete != nil { log.Error("DeleteRepository: %v", errDelete) diff --git a/modules/task/task.go b/modules/task/task.go index 4e782869f93f5..51377df78c931 100644 --- a/modules/task/task.go +++ b/modules/task/task.go @@ -90,7 +90,7 @@ func CreateMigrateTask(doer, u *models.User, opts base.MigrateOptions) (*models. return nil, err } - var task = models.Task{ + var task = &models.Task{ DoerID: doer.ID, OwnerID: u.ID, Type: structs.TaskTypeMigrateRepo, @@ -98,7 +98,7 @@ func CreateMigrateTask(doer, u *models.User, opts base.MigrateOptions) (*models. PayloadContent: string(bs), } - if err := models.CreateTask(&task); err != nil { + if err := models.CreateTask(task); err != nil { return nil, err } @@ -126,5 +126,5 @@ func CreateMigrateTask(doer, u *models.User, opts base.MigrateOptions) (*models. return nil, err } - return &task, nil + return task, nil } diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 4a9e3c3894d7e..0c83175e408c5 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -905,6 +905,7 @@ migrate.migrate = Migrate From %s migrate.migrating = Migrating from %s ... migrate.migrating_failed = Migrating from %s failed. migrate.migrating_failed.error = Error: %s +migrate.migrating_failed_no_addr = Migration failed. migrate.github.description = Migrate data from github.com or other Github instances. migrate.git.description = Migrate a repository only from any Git service. migrate.gitlab.description = Migrate data from gitlab.com or other GitLab instances. diff --git a/routers/web/repo/view.go b/routers/web/repo/view.go index 90be631c73499..18d868eff33fc 100644 --- a/routers/web/repo/view.go +++ b/routers/web/repo/view.go @@ -582,6 +582,13 @@ func checkHomeCodeViewable(ctx *context.Context) { if ctx.Repo.Repository.IsBeingCreated() { task, err := models.GetMigratingTask(ctx.Repo.Repository.ID) if err != nil { + if models.IsErrTaskDoesNotExist(err) { + ctx.Data["Repo"] = ctx.Repo + ctx.Data["CloneAddr"] = "" + ctx.Data["Failed"] = true + ctx.HTML(http.StatusOK, tplMigrating) + return + } ctx.ServerError("models.GetMigratingTask", err) return } diff --git a/routers/web/user/task.go b/routers/web/user/task.go index c71d435233933..4dbd1b8537bf4 100644 --- a/routers/web/user/task.go +++ b/routers/web/user/task.go @@ -6,6 +6,7 @@ package user import ( "net/http" + "strconv" "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/context" @@ -16,6 +17,12 @@ import ( func TaskStatus(ctx *context.Context) { task, opts, err := models.GetMigratingTaskByID(ctx.ParamsInt64("task"), ctx.User.ID) if err != nil { + if models.IsErrTaskDoesNotExist(err) { + ctx.JSON(http.StatusNotFound, map[string]interface{}{ + "error": "task `" + strconv.FormatInt(ctx.ParamsInt64("task"), 10) + "` does not exist", + }) + return + } ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ "err": err, }) diff --git a/templates/repo/migrate/migrating.tmpl b/templates/repo/migrate/migrating.tmpl index cc12243205c15..6df7f0a65d687 100644 --- a/templates/repo/migrate/migrating.tmpl +++ b/templates/repo/migrate/migrating.tmpl @@ -25,7 +25,11 @@

{{if and .Failed .Permission.IsAdmin}} From 01c2d5c5f55399b7e8c1254aaa0ed34236093ff9 Mon Sep 17 00:00:00 2001 From: zeripath Date: Sun, 7 Nov 2021 00:11:16 +0000 Subject: [PATCH 2/4] Actually let's keep zero out the repo id --- modules/task/migrate.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/modules/task/migrate.go b/modules/task/migrate.go index aca32a73c0415..602af259cf167 100644 --- a/modules/task/migrate.go +++ b/modules/task/migrate.go @@ -58,12 +58,14 @@ func runMigrateTask(t *models.Task) (err error) { t.EndTime = timeutil.TimeStampNow() t.Status = structs.TaskStatusFailed t.Message = err.Error() - if err := t.UpdateCols("status", "errors", "end_time"); err != nil { - log.Error("Task UpdateCols failed: %v", err) - } _ = t.LoadRepo() + t.RepoID = 0 + if err := t.UpdateCols("status", "errors", "repo_id", "end_time"); err != nil { + log.Error("Task UpdateCols failed: %v", err) + } + if t.Repo != nil { if errDelete := models.DeleteRepository(t.Doer, t.OwnerID, t.Repo.ID); errDelete != nil { log.Error("DeleteRepository: %v", errDelete) From de6ae4ae7e47aa3d732906d9cfaf40e968f553e6 Mon Sep 17 00:00:00 2001 From: zeripath Date: Wed, 10 Nov 2021 10:54:46 +0000 Subject: [PATCH 3/4] Update modules/task/migrate.go Co-authored-by: Gusted --- modules/task/migrate.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/task/migrate.go b/modules/task/migrate.go index 602af259cf167..0c340d0b43da4 100644 --- a/modules/task/migrate.go +++ b/modules/task/migrate.go @@ -58,7 +58,7 @@ func runMigrateTask(t *models.Task) (err error) { t.EndTime = timeutil.TimeStampNow() t.Status = structs.TaskStatusFailed t.Message = err.Error() - + // Ensure that the repo loaded before we zero out the repo ID from the task - thus ensuring that we can delete it _ = t.LoadRepo() t.RepoID = 0 From af8db66e340f21287651058c527c8417e4227bea Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 10 Nov 2021 11:11:32 +0000 Subject: [PATCH 4/4] formatting Signed-off-by: Andrew Thornton --- modules/task/migrate.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/task/migrate.go b/modules/task/migrate.go index 0c340d0b43da4..715e76b4ade08 100644 --- a/modules/task/migrate.go +++ b/modules/task/migrate.go @@ -58,7 +58,7 @@ func runMigrateTask(t *models.Task) (err error) { t.EndTime = timeutil.TimeStampNow() t.Status = structs.TaskStatusFailed t.Message = err.Error() - // Ensure that the repo loaded before we zero out the repo ID from the task - thus ensuring that we can delete it + // Ensure that the repo loaded before we zero out the repo ID from the task - thus ensuring that we can delete it _ = t.LoadRepo() t.RepoID = 0