Correctly handle failed migrations (#17575)

* 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 <art27@cantab.net>

modules/task/migrate.go

@@ -58,6 +58,9 @@ 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
 		if err := t.UpdateCols("status", "errors", "repo_id", "end_time"); err != nil {
 			log.Error("Task UpdateCols failed: %v", err)

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
 }

options/locale/locale_en-US.ini

@@ -904,6 +904,7 @@ migrate.migrate = Migrate From %s
 migrate.migrating = Migrating from <b>%s</b> ...
 migrate.migrating_failed = Migrating from <b>%s</b> 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.

routers/web/repo/view.go

@@ -583,6 +583,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
 			}

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,
 		})

templates/repo/migrate/migrating.tmpl

@@ -25,7 +25,11 @@
 								<p id="repo_migrating_progress_message"></p>
 							</div>
 							<div id="repo_migrating_failed" hidden>
-								<p>{{.i18n.Tr "repo.migrate.migrating_failed" .CloneAddr | Safe}}</p>
+								{{if .CloneAddr}}
+									<p>{{.i18n.Tr "repo.migrate.migrating_failed" .CloneAddr | Safe}}</p>
+								{{else}}
+									<p>{{.i18n.Tr "repo.migrate.migrating_failed_no_addr" | Safe}}</p>
+								{{end}}
 								<p id="repo_migrating_failed_error"></p>
 							</div>
 							{{if and .Failed .Permission.IsAdmin}}