Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid panic caused by broken payload when creating commit status #23216

Merged
merged 4 commits into from
Mar 4, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion routers/api/actions/runner/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ func (s *Service) UpdateTask(
}

if err := actions_service.CreateCommitStatus(ctx, task.Job); err != nil {
log.Error("Update commit status failed: %v", err)
log.Error("Update commit status for job %v failed: %v", task.Job.ID, err)
// go on
}

Expand Down
22 changes: 15 additions & 7 deletions routers/web/repo/actions/view.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"code.gitea.io/gitea/models/unit"
"code.gitea.io/gitea/modules/actions"
context_module "code.gitea.io/gitea/modules/context"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/timeutil"
"code.gitea.io/gitea/modules/util"
"code.gitea.io/gitea/modules/web"
Expand Down Expand Up @@ -212,15 +213,18 @@ func Rerun(ctx *context_module.Context) {
job.Stopped = 0

if err := db.WithTx(ctx, func(ctx context.Context) error {
if _, err := actions_model.UpdateRunJob(ctx, job, builder.Eq{"status": status}, "task_id", "status", "started", "stopped"); err != nil {
return err
}
return actions_service.CreateCommitStatus(ctx, job)
_, err := actions_model.UpdateRunJob(ctx, job, builder.Eq{"status": status}, "task_id", "status", "started", "stopped")
return err
}); err != nil {
ctx.Error(http.StatusInternalServerError, err.Error())
return
}

if err := actions_service.CreateCommitStatus(ctx, job); err != nil {
techknowlogick marked this conversation as resolved.
Show resolved Hide resolved
log.Error("Update commit status for job %v failed: %v", job.ID, err)
// go on
}

ctx.JSON(http.StatusOK, struct{}{})
}

Expand Down Expand Up @@ -253,16 +257,20 @@ func Cancel(ctx *context_module.Context) {
if err := actions_model.StopTask(ctx, job.TaskID, actions_model.StatusCancelled); err != nil {
return err
}
if err := actions_service.CreateCommitStatus(ctx, job); err != nil {
return err
}
}
return nil
}); err != nil {
ctx.Error(http.StatusInternalServerError, err.Error())
return
}

for _, job := range jobs {
if err := actions_service.CreateCommitStatus(ctx, job); err != nil {
log.Error("Update commit status for job %v failed: %v", job.ID, err)
// go on
}
}

ctx.JSON(http.StatusOK, struct{}{})
}

Expand Down
22 changes: 17 additions & 5 deletions services/actions/clear_tasks.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ func stopTasks(ctx context.Context, opts actions_model.FindTaskOptions) error {
return fmt.Errorf("find tasks: %w", err)
}

jobs := make([]*actions_model.ActionRunJob, 0, len(tasks))
for _, task := range tasks {
if err := db.WithTx(ctx, func(ctx context.Context) error {
if err := actions_model.StopTask(ctx, task.ID, actions_model.StatusFailure); err != nil {
Expand All @@ -51,7 +52,8 @@ func stopTasks(ctx context.Context, opts actions_model.FindTaskOptions) error {
if err := task.LoadJob(ctx); err != nil {
return err
}
return CreateCommitStatus(ctx, task.Job)
jobs = append(jobs, task.Job)
return nil
}); err != nil {
log.Warn("Cannot stop task %v: %v", task.ID, err)
// go on
Expand All @@ -61,6 +63,14 @@ func stopTasks(ctx context.Context, opts actions_model.FindTaskOptions) error {
remove()
}
}

for _, job := range jobs {
if err := CreateCommitStatus(ctx, job); err != nil {
log.Error("Update commit status for job %v failed: %v", job.ID, err)
// go on
}
}

return nil
}

Expand All @@ -80,14 +90,16 @@ func CancelAbandonedJobs(ctx context.Context) error {
job.Status = actions_model.StatusCancelled
job.Stopped = now
if err := db.WithTx(ctx, func(ctx context.Context) error {
if _, err := actions_model.UpdateRunJob(ctx, job, nil, "status", "stopped"); err != nil {
return err
}
return CreateCommitStatus(ctx, job)
_, err := actions_model.UpdateRunJob(ctx, job, nil, "status", "stopped")
return err
}); err != nil {
log.Warn("cancel abandoned job %v: %v", job.ID, err)
// go on
}
if err := CreateCommitStatus(ctx, job); err != nil {
log.Error("Update commit status for job %v failed: %v", job.ID, err)
// go on
}
}

return nil
Expand Down
10 changes: 10 additions & 0 deletions services/actions/commit_status.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,16 @@ func CreateCommitStatus(ctx context.Context, job *actions_model.ActionRunJob) er
return fmt.Errorf("GetPushEventPayload: %w", err)
}

// Since the payload comes from json data, we should check if it's broken, or it will cause panic
switch {
case payload.Repo == nil:
return fmt.Errorf("miss Repo in event payload")
case payload.Pusher == nil:
return fmt.Errorf("miss Pusher in event payload")
case payload.HeadCommit == nil:
return fmt.Errorf("miss HeadCommit in event payload")
wolfogre marked this conversation as resolved.
Show resolved Hide resolved
}

creator, err := user_model.GetUserByID(ctx, payload.Pusher.ID)
if err != nil {
return fmt.Errorf("GetUserByID: %w", err)
Expand Down
3 changes: 2 additions & 1 deletion services/actions/notifier_helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,8 @@ func notify(ctx context.Context, input *notifyInput) error {
} else {
for _, job := range jobs {
if err := CreateCommitStatus(ctx, job); err != nil {
log.Error("CreateCommitStatus: %v", err)
log.Error("Update commit status for job %v failed: %v", job.ID, err)
// go on
}
}
}
Expand Down