From b0b6bd36581035c8eecbd70d43d2131344b1f9f3 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Mon, 18 Aug 2025 22:45:10 +0200 Subject: [PATCH] fix: a corrupted Forgejo Actions scheduled workflow is disabled (#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. ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests - I added test coverage for Go changes... - [x] in their respective `*_test.go` for unit tests. - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] I do not want this change to show in the release notes. - [x] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/.md` to be be used for the release notes instead of the title. ## Release notes - Bug fixes - [PR](https://codeberg.org/forgejo/forgejo/pulls/8942): a corrupted Forgejo Actions scheduled workflow is disabled Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8942 Reviewed-by: Michael Kriese 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 ab8b330ba2..1e90ad4b3f 100644 --- a/services/actions/schedule_tasks.go +++ b/services/actions/schedule_tasks.go @@ -20,6 +20,7 @@ import ( "code.forgejo.org/forgejo/runner/v9/act/jobparser" act_model "code.forgejo.org/forgejo/runner/v9/act/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})