From cd35473212985d3f6b61b606d739510c60a8a7dc Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Tue, 19 Aug 2025 01:43:43 +0200 Subject: [PATCH] [v12.0/forgejo] a corrupted Forgejo Actions scheduled workflow is disabled (#8944) **Backport: https://codeberg.org/forgejo/forgejo/pulls/8942** The following errors are specific to the scheduled workflow being handled. They do not imply the remaining scheduled workflows cannot be scheduled successfully. - Failure to create a scheduled task which is most likely caused by an invalid YAML file. - Failure to parse the scheduling specs which can be caused by a number of formating errors. Instead of returning on error, the corrupted workflow is disabled. Also display more informative error messages so that the failed workflow can be identified from the logged error. (cherry picked from commit ab3cf7ddcf91907071d94f45d9704ad87deff3e4) ``` Conflicts: services/actions/schedule_tasks.go trivial context conflict ``` Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8944 Reviewed-by: Otto Co-authored-by: Earl Warren Co-committed-by: Earl Warren --- models/repo/repo_unit.go | 5 +- .../action_schedule.yml | 41 ++++++++++++++ .../action_schedule_spec.yml | 15 ++++++ services/actions/schedule_tasks.go | 30 ++++++++--- services/actions/schedule_tasks_test.go | 54 +++++++++++++++++++ 5 files changed, 136 insertions(+), 9 deletions(-) create mode 100644 services/actions/TestServiceActions_startTask/action_schedule.yml create mode 100644 services/actions/TestServiceActions_startTask/action_schedule_spec.yml diff --git a/models/repo/repo_unit.go b/models/repo/repo_unit.go index e50f79e945..aa6f2fa0ae 100644 --- a/models/repo/repo_unit.go +++ b/models/repo/repo_unit.go @@ -336,5 +336,8 @@ func getUnitsByRepoID(ctx context.Context, repoID int64) (units []*RepoUnit, err // UpdateRepoUnit updates the provided repo unit func UpdateRepoUnit(ctx context.Context, unit *RepoUnit) error { _, err := db.GetEngine(ctx).ID(unit.ID).Update(unit) - return err + if err != nil { + return fmt.Errorf("UpdateRepoUnit: %v", err) + } + return nil } diff --git a/services/actions/TestServiceActions_startTask/action_schedule.yml b/services/actions/TestServiceActions_startTask/action_schedule.yml new file mode 100644 index 0000000000..d0e7234475 --- /dev/null +++ b/services/actions/TestServiceActions_startTask/action_schedule.yml @@ -0,0 +1,41 @@ +# A corrupted cron spec with a valid schedule workflow +- + id: 1 + title: schedule_title1 + specs: + - '* * * * *' + repo_id: 4 + owner_id: 2 + workflow_id: 'workflow1.yml' + trigger_user_id: 2 + ref: main + commit_sha: shashasha + event: "schedule" + event_payload: "fakepayload" + content: | + jobs: + job2: + runs-on: ubuntu-latest + steps: + - run: true + +# A valid cron spec with a corrupted schedule workflow +- + id: 2 + title: schedule_title2 + specs: + - '* * * * *' + repo_id: 4 + owner_id: 2 + workflow_id: 'workflow2.yml' + trigger_user_id: 2 + ref: main + commit_sha: shashasha + event: "schedule" + event_payload: "fakepayload" + content: | + jobs: + job2: { invalid yaml + runs-on: ubuntu-latest + steps: + - run: true diff --git a/services/actions/TestServiceActions_startTask/action_schedule_spec.yml b/services/actions/TestServiceActions_startTask/action_schedule_spec.yml new file mode 100644 index 0000000000..7bcc78f010 --- /dev/null +++ b/services/actions/TestServiceActions_startTask/action_schedule_spec.yml @@ -0,0 +1,15 @@ +# A corrupted cron spec with a valid schedule workflow +- + id: 1 + repo_id: 4 + schedule_id: 1 + next: 1 + spec: 'corrupted * *' + +# A valid cron spec with a corrupted schedule workflow +- + id: 2 + repo_id: 4 + schedule_id: 2 + next: 1 + spec: '* * * * *' diff --git a/services/actions/schedule_tasks.go b/services/actions/schedule_tasks.go index b422057d6f..ec38f5eb8a 100644 --- a/services/actions/schedule_tasks.go +++ b/services/actions/schedule_tasks.go @@ -20,6 +20,7 @@ import ( "github.com/nektos/act/pkg/jobparser" act_model "github.com/nektos/act/pkg/model" + "github.com/robfig/cron/v3" "xorm.io/builder" ) @@ -83,20 +84,33 @@ func startTasks(ctx context.Context) error { } return fmt.Errorf("GetUnit: %w", err) } - if cfg.ActionsConfig().IsWorkflowDisabled(row.Schedule.WorkflowID) { + actionConfig := cfg.ActionsConfig() + if actionConfig.IsWorkflowDisabled(row.Schedule.WorkflowID) { continue } - if err := CreateScheduleTask(ctx, row.Schedule); err != nil { - log.Error("CreateScheduleTask: %v", err) - return err + createAndSchedule := func(row *actions_model.ActionScheduleSpec) (cron.Schedule, error) { + if err := CreateScheduleTask(ctx, row.Schedule); err != nil { + return nil, fmt.Errorf("CreateScheduleTask: %v", err) + } + + // Parse the spec + schedule, err := row.Parse() + if err != nil { + return nil, fmt.Errorf("Parse(Spec=%v): %v", row.Spec, err) + } + return schedule, nil } - // Parse the spec - schedule, err := row.Parse() + schedule, err := createAndSchedule(row) if err != nil { - log.Error("Parse: %v", err) - return err + log.Error("RepoID=%v WorkflowID=%v: %v", row.Schedule.RepoID, row.Schedule.WorkflowID, err) + actionConfig.DisableWorkflow(row.Schedule.WorkflowID) + if err := repo_model.UpdateRepoUnit(ctx, cfg); err != nil { + log.Error("RepoID=%v WorkflowID=%v: CreateScheduleTask: %v", row.Schedule.RepoID, row.Schedule.WorkflowID, err) + return err + } + continue } // Update the spec's next run time and previous run time diff --git a/services/actions/schedule_tasks_test.go b/services/actions/schedule_tasks_test.go index 7073985252..31ed5ec813 100644 --- a/services/actions/schedule_tasks_test.go +++ b/services/actions/schedule_tasks_test.go @@ -7,7 +7,9 @@ import ( "testing" actions_model "forgejo.org/models/actions" + "forgejo.org/models/db" repo_model "forgejo.org/models/repo" + "forgejo.org/models/unit" "forgejo.org/models/unittest" webhook_module "forgejo.org/modules/webhook" @@ -15,6 +17,58 @@ import ( "github.com/stretchr/testify/require" ) +func TestServiceActions_startTask(t *testing.T) { + defer unittest.OverrideFixtures("services/actions/TestServiceActions_startTask")() + require.NoError(t, unittest.PrepareTestDatabase()) + + // Load fixtures that are corrupted and create one valid scheduled workflow + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4}) + + workflowID := "some.yml" + schedules := []*actions_model.ActionSchedule{ + { + Title: "scheduletitle1", + RepoID: repo.ID, + OwnerID: repo.OwnerID, + WorkflowID: workflowID, + TriggerUserID: repo.OwnerID, + Ref: "branch", + CommitSHA: "fakeSHA", + Event: webhook_module.HookEventSchedule, + EventPayload: "fakepayload", + Specs: []string{"* * * * *"}, + Content: []byte( + ` +jobs: + job2: + runs-on: ubuntu-latest + steps: + - run: true +`), + }, + } + + require.Equal(t, 2, unittest.GetCount(t, actions_model.ActionScheduleSpec{})) + require.NoError(t, actions_model.CreateScheduleTask(t.Context(), schedules)) + require.Equal(t, 3, unittest.GetCount(t, actions_model.ActionScheduleSpec{})) + _, err := db.GetEngine(db.DefaultContext).Exec("UPDATE `action_schedule_spec` SET next = 1") + require.NoError(t, err) + + // After running startTasks an ActionRun row is created for the valid scheduled workflow + require.Empty(t, unittest.GetCount(t, actions_model.ActionRun{WorkflowID: workflowID})) + require.NoError(t, startTasks(t.Context())) + require.NotEmpty(t, unittest.GetCount(t, actions_model.ActionRun{WorkflowID: workflowID})) + + // The invalid workflows loaded from the fixtures are disabled + repo = unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4}) + actionUnit, err := repo.GetUnit(t.Context(), unit.TypeActions) + require.NoError(t, err) + actionConfig := actionUnit.ActionsConfig() + assert.True(t, actionConfig.IsWorkflowDisabled("workflow2.yml")) + assert.True(t, actionConfig.IsWorkflowDisabled("workflow1.yml")) + assert.False(t, actionConfig.IsWorkflowDisabled("some.yml")) +} + func TestCreateScheduleTask(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2, OwnerID: 2})