From 1ffe46d2a4ed99bbf605997a5e2b368a61b6a6cb Mon Sep 17 00:00:00 2001 From: Giteabot Date: Wed, 14 Oct 2026 15:47:59 -0700 Subject: [PATCH] fix(actions): fix schedule cleanup and default branch switch (#39740) (#39786) Backport #39740 by @Zettat123 Cancelling running schedule runs has not worked since https://github.com/go-gitea/gitea/pull/35767: callers pass a branch name like `main`, while schedule runs store the full ref like `refs/heads/main`. Disabling Actions, archiving the repository or changing the default branch left them running. Fixing the ref alone would also cancel them on every push to the default branch, which GitHub does not do, and whenever the Actions setting is saved, so those paths now only replace the schedules. Changing or renaming the default branch deleted the schedules without registering the new branch's ones (https://github.com/go-gitea/gitea/pull/28691), so they stopped until the next push, and the API path kept the old ones. Both now switch the schedules to the new default branch, since scheduled workflows only run on the default branch. Only the push case was verified on GitHub, as its scheduled workflows have rarely been triggering lately. Co-authored-by: Zettat123 --- models/actions/schedule.go | 3 +- routers/api/v1/repo/repo.go | 6 ++ services/actions/clear_tasks.go | 18 ++++ services/actions/notifier_helper.go | 5 +- services/repository/branch.go | 37 ++------ services/repository/setting.go | 25 +++-- tests/integration/actions_schedule_test.go | 105 ++++++++++++++++----- 7 files changed, 135 insertions(+), 64 deletions(-) diff --git a/models/actions/schedule.go b/models/actions/schedule.go index 79679203489..9047155c859 100644 --- a/models/actions/schedule.go +++ b/models/actions/schedule.go @@ -11,6 +11,7 @@ import ( "gitea.dev/models/db" repo_model "gitea.dev/models/repo" user_model "gitea.dev/models/user" + "gitea.dev/modules/git" "gitea.dev/modules/timeutil" "gitea.dev/modules/util" webhook_module "gitea.dev/modules/webhook" @@ -110,7 +111,7 @@ func CleanRepoScheduleTasks(ctx context.Context, repo *repo_model.Repository) ([ jobs, err := CancelPreviousJobs( ctx, repo.ID, - repo.DefaultBranch, + git.RefNameFromBranch(repo.DefaultBranch).String(), "", webhook_module.HookEventSchedule, ) diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go index 871b3582088..73f93a9c3cc 100644 --- a/routers/api/v1/repo/repo.go +++ b/routers/api/v1/repo/repo.go @@ -717,7 +717,9 @@ func updateBasicProperties(ctx *context.APIContext, opts api.EditRepoOption) err // Default branch only updated if changed and exist or the repository is empty updateRepoLicense := false + oldDefaultBranch := "" if opts.DefaultBranch != nil && repo.DefaultBranch != *opts.DefaultBranch && (repo.IsEmpty || git.IsBranchExist(ctx, ctx.Repo.Repository, *opts.DefaultBranch)) { + oldDefaultBranch = repo.DefaultBranch repo.DefaultBranch = *opts.DefaultBranch if !repo.IsEmpty { if err := git.SetDefaultBranch(ctx, repo, repo.DefaultBranch); err != nil { @@ -733,6 +735,10 @@ func updateBasicProperties(ctx *context.APIContext, opts api.EditRepoOption) err return err } + if oldDefaultBranch != "" { + actions_service.SwitchSchedulesToDefaultBranch(ctx, repo, oldDefaultBranch) + } + if visibilityChanged { audit.Record(ctx, audit_model.RepositoryVisibility, repo, "visibility", repo.IsPrivate) } diff --git a/services/actions/clear_tasks.go b/services/actions/clear_tasks.go index a2fe4bc1b76..a1389a54957 100644 --- a/services/actions/clear_tasks.go +++ b/services/actions/clear_tasks.go @@ -12,7 +12,9 @@ import ( actions_model "gitea.dev/models/actions" "gitea.dev/models/db" repo_model "gitea.dev/models/repo" + unit_model "gitea.dev/models/unit" "gitea.dev/modules/actions" + "gitea.dev/modules/git" "gitea.dev/modules/log" "gitea.dev/modules/setting" "gitea.dev/modules/timeutil" @@ -62,6 +64,22 @@ func CleanRepoScheduleTasks(ctx context.Context, repo *repo_model.Repository) er return err } +// SwitchSchedulesToDefaultBranch cancels the running schedule runs of the old default branch +// and replaces the schedules with the ones on the current default branch +func SwitchSchedulesToDefaultBranch(ctx context.Context, repo *repo_model.Repository, oldDefaultBranch string) { + if err := actions_model.DeleteScheduleTaskByRepo(ctx, repo.ID); err != nil { + log.Error("DeleteScheduleTaskByRepo: %v", err) + } + if err := CancelPreviousJobs(ctx, repo.ID, git.RefNameFromBranch(oldDefaultBranch).String(), "", webhook_module.HookEventSchedule); err != nil { + log.Error("CancelPreviousJobs: %v", err) + } + if repo.UnitEnabled(ctx, unit_model.TypeActions) { + if err := DetectAndHandleSchedules(ctx, repo); err != nil { + log.Error("DetectAndHandleSchedules: %v", err) + } + } +} + func shouldBlockJobByConcurrency(ctx context.Context, job *actions_model.ActionRunJob) (bool, error) { if job.RawConcurrency != "" && !job.IsConcurrencyEvaluated { // when the job depends on other jobs, we cannot evaluate its concurrency, so it should be blocked and will be evaluated again when its dependencies are done diff --git a/services/actions/notifier_helper.go b/services/actions/notifier_helper.go index 6b8ac63814e..7bfee941963 100644 --- a/services/actions/notifier_helper.go +++ b/services/actions/notifier_helper.go @@ -577,8 +577,9 @@ func handleSchedules( log.Error("CountSchedules: %v", err) return err } else if count > 0 { - if err := CleanRepoScheduleTasks(ctx, input.Repo); err != nil { - log.Error("CleanRepoScheduleTasks: %v", err) + // replacing the schedules leaves their in-progress runs alone, as GitHub does + if err := actions_model.DeleteScheduleTaskByRepo(ctx, input.Repo.ID); err != nil { + log.Error("DeleteScheduleTaskByRepo: %v", err) } } diff --git a/services/repository/branch.go b/services/repository/branch.go index 0598e7c83a4..73988a093ca 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -9,7 +9,6 @@ import ( "fmt" "strings" - actions_model "gitea.dev/models/actions" "gitea.dev/models/db" git_model "gitea.dev/models/git" issues_model "gitea.dev/models/issues" @@ -28,7 +27,6 @@ import ( "gitea.dev/modules/reqctx" "gitea.dev/modules/timeutil" "gitea.dev/modules/util" - webhook_module "gitea.dev/modules/webhook" actions_service "gitea.dev/services/actions" notify_service "gitea.dev/services/notify" release_service "gitea.dev/services/release" @@ -458,21 +456,6 @@ func RenameBranch(ctx context.Context, repo *repo_model.Repository, doer *user_m } if isDefault { - // if default branch changed, we need to delete all schedules and cron jobs - if err := actions_model.DeleteScheduleTaskByRepo(ctx, repo.ID); err != nil { - log.Error("DeleteCronTaskByRepo: %v", err) - } - // cancel running cron jobs of this repository and delete old schedules - if err := actions_service.CancelPreviousJobs( - ctx, - repo.ID, - from, - "", - webhook_module.HookEventSchedule, - ); err != nil { - log.Error("CancelPreviousJobs: %v", err) - } - err2 = git.SetDefaultBranch(ctx, repo, to) if err2 != nil { return err2 @@ -484,6 +467,10 @@ func RenameBranch(ctx context.Context, repo *repo_model.Repository, doer *user_m return "", err } + if isDefault { + actions_service.SwitchSchedulesToDefaultBranch(ctx, repo, from) + } + notify_service.DeleteRef(ctx, doer, repo, git.RefNameFromBranch(from)) notify_service.CreateRef(ctx, doer, repo, git.RefNameFromBranch(to), fromBranch.CommitID) @@ -709,25 +696,13 @@ func SetRepoDefaultBranch(ctx context.Context, repo *repo_model.Repository, newB return err } - if err := actions_model.DeleteScheduleTaskByRepo(ctx, repo.ID); err != nil { - log.Error("DeleteCronTaskByRepo: %v", err) - } - // cancel running cron jobs of this repository and delete old schedules - if err := actions_service.CancelPreviousJobs( - ctx, - repo.ID, - oldDefaultBranchName, - "", - webhook_module.HookEventSchedule, - ); err != nil { - log.Error("CancelPreviousJobs: %v", err) - } - return git.SetDefaultBranch(ctx, repo, newBranchName) }); err != nil { return err } + actions_service.SwitchSchedulesToDefaultBranch(ctx, repo, oldDefaultBranchName) + if !repo.IsEmpty { if err := AddRepoToLicenseUpdaterQueue(&LicenseUpdaterOptions{ RepoID: repo.ID, diff --git a/services/repository/setting.go b/services/repository/setting.go index 5182c9b20ec..da828c77a42 100644 --- a/services/repository/setting.go +++ b/services/repository/setting.go @@ -7,6 +7,7 @@ import ( "context" "slices" + actions_model "gitea.dev/models/actions" "gitea.dev/models/db" repo_model "gitea.dev/models/repo" "gitea.dev/models/unit" @@ -16,20 +17,19 @@ import ( // UpdateRepositoryUnits updates a repository's units func UpdateRepositoryUnits(ctx context.Context, repo *repo_model.Repository, units []repo_model.RepoUnit, deleteUnitTypes []unit.Type) (err error) { - return db.WithTx(ctx, func(ctx context.Context) error { + disableActions := slices.Contains(deleteUnitTypes, unit.TypeActions) + + err = db.WithTx(ctx, func(ctx context.Context) error { // Delete existing settings of units before adding again for _, u := range units { deleteUnitTypes = append(deleteUnitTypes, u.Type) } - if slices.Contains(deleteUnitTypes, unit.TypeActions) { - if err := actions_service.CleanRepoScheduleTasks(ctx, repo); err != nil { - log.Error("CleanRepoScheduleTasks: %v", err) - } - } - for _, u := range units { if u.Type == unit.TypeActions { + if err := actions_model.DeleteScheduleTaskByRepo(ctx, repo.ID); err != nil { + log.Error("DeleteScheduleTaskByRepo: %v", err) + } if err := actions_service.DetectAndHandleSchedules(ctx, repo); err != nil { log.Error("DetectAndHandleSchedules: %v", err) } @@ -49,4 +49,15 @@ func UpdateRepositoryUnits(ctx context.Context, repo *repo_model.Repository, uni return nil }) + if err != nil { + return err + } + + // only disabling stops in-progress schedule runs + if disableActions { + if err := actions_service.CleanRepoScheduleTasks(ctx, repo); err != nil { + log.Error("CleanRepoScheduleTasks: %v", err) + } + } + return nil } diff --git a/tests/integration/actions_schedule_test.go b/tests/integration/actions_schedule_test.go index 230a8a41175..c6aa79cf34a 100644 --- a/tests/integration/actions_schedule_test.go +++ b/tests/integration/actions_schedule_test.go @@ -4,6 +4,7 @@ package integration import ( + "net/http" "net/url" "strconv" "strings" @@ -17,13 +18,16 @@ import ( unit_model "gitea.dev/models/unit" "gitea.dev/models/unittest" user_model "gitea.dev/models/user" + "gitea.dev/modules/git" "gitea.dev/modules/migration" api "gitea.dev/modules/structs" + actions_service "gitea.dev/services/actions" mirror_service "gitea.dev/services/mirror" repo_service "gitea.dev/services/repository" files_service "gitea.dev/services/repository/files" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestScheduleUpdate(t *testing.T) { @@ -32,15 +36,17 @@ func TestScheduleUpdate(t *testing.T) { t.Run("DisableAndEnableActionsUnit", testScheduleUpdateDisableAndEnableActionsUnit) t.Run("ArchiveAndUnarchive", testScheduleUpdateArchiveAndUnarchive) t.Run("MirrorSync", testScheduleUpdateMirrorSync) + t.Run("ChangeDefaultBranch", testScheduleUpdateChangeDefaultBranch) + t.Run("RenameDefaultBranch", testScheduleUpdateRenameDefaultBranch) } func testScheduleUpdatePush(t *testing.T) { - doTestScheduleUpdate(t, func(t *testing.T, u *url.URL, testContext APITestContext, user *user_model.User, repo *repo_model.Repository) (commitID, expectedSpec string) { + doTestScheduleUpdate(t, func(t *testing.T, u *url.URL, testContext APITestContext, user *user_model.User, repo *repo_model.Repository) (expectedSpec string) { + run := createScheduleRun(t, repo) newCron := "30 5 * * 1,3" pushScheduleChange(t, u, repo, newCron) - branch, err := git_model.GetBranchExisting(t.Context(), repo.ID, repo.DefaultBranch) - assert.NoError(t, err) - return branch.CommitID, newCron + assertRunStatus(t, run, actions_model.StatusWaiting) + return newCron }) } @@ -68,7 +74,7 @@ jobs: for _, mergeStyle := range mergeStyles { t.Run(string(mergeStyle), func(t *testing.T) { - doTestScheduleUpdate(t, func(t *testing.T, u *url.URL, testContext APITestContext, user *user_model.User, repo *repo_model.Repository) (commitID, expectedSpec string) { + doTestScheduleUpdate(t, func(t *testing.T, u *url.URL, testContext APITestContext, user *user_model.User, repo *repo_model.Repository) (expectedSpec string) { // update workflow file _, err := files_service.ChangeRepoFiles(t.Context(), repo, user, &files_service.ChangeRepoFilesOptions{ NewBranch: newBranchName, @@ -91,15 +97,13 @@ jobs: testPullMerge(t, testContext.Session, repo.OwnerName, repo.Name, strconv.FormatInt(apiPull.Index, 10), MergeOptions{ Style: mergeStyle, }) - - pull := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: apiPull.ID}) - return pull.MergedCommitID, "@every 2m" + return "@every 2m" }) }) } t.Run(string(repo_model.MergeStyleManuallyMerged), func(t *testing.T) { - doTestScheduleUpdate(t, func(t *testing.T, u *url.URL, testContext APITestContext, user *user_model.User, repo *repo_model.Repository) (commitID, expectedSpec string) { + doTestScheduleUpdate(t, func(t *testing.T, u *url.URL, testContext APITestContext, user *user_model.User, repo *repo_model.Repository) (expectedSpec string) { // enable manual-merge doAPIEditRepository(testContext, &api.EditRepoOption{ HasPullRequests: new(true), @@ -137,13 +141,13 @@ jobs: pull := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: apiPull.ID}) assert.Equal(t, issues_model.PullRequestStatusManuallyMerged, pull.Status) - return pull.MergedCommitID, "@every 2m" + return "@every 2m" }) }) } func testScheduleUpdateMirrorSync(t *testing.T) { - doTestScheduleUpdate(t, func(t *testing.T, u *url.URL, testContext APITestContext, user *user_model.User, repo *repo_model.Repository) (commitID, expectedSpec string) { + doTestScheduleUpdate(t, func(t *testing.T, u *url.URL, testContext APITestContext, user *user_model.User, repo *repo_model.Repository) (expectedSpec string) { // create mirror repo opts := migration.MigrateOptions{ RepoName: "actions-schedule-mirror", @@ -193,41 +197,79 @@ func testScheduleUpdateMirrorSync(t *testing.T) { scheduleSpec = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionScheduleSpec{RepoID: mirrorRepo.ID, ScheduleID: actionSchedule.ID}) assert.Equal(t, newCron, scheduleSpec.Spec) - return repoDefaultBranch.CommitID, newCron + return newCron }) } func testScheduleUpdateArchiveAndUnarchive(t *testing.T) { - doTestScheduleUpdate(t, func(t *testing.T, u *url.URL, testContext APITestContext, user *user_model.User, repo *repo_model.Repository) (commitID, expectedSpec string) { + doTestScheduleUpdate(t, func(t *testing.T, u *url.URL, testContext APITestContext, user *user_model.User, repo *repo_model.Repository) (expectedSpec string) { + run := createScheduleRun(t, repo) doAPIEditRepository(testContext, &api.EditRepoOption{ Archived: new(true), })(t) assert.Zero(t, unittest.GetCount(t, &actions_model.ActionSchedule{RepoID: repo.ID})) + assertRunStatus(t, run, actions_model.StatusCancelled) doAPIEditRepository(testContext, &api.EditRepoOption{ Archived: new(false), })(t) - branch, err := git_model.GetBranchExisting(t.Context(), repo.ID, repo.DefaultBranch) - assert.NoError(t, err) - return branch.CommitID, "@every 1m" + return "@every 1m" }) } func testScheduleUpdateDisableAndEnableActionsUnit(t *testing.T) { - doTestScheduleUpdate(t, func(t *testing.T, u *url.URL, testContext APITestContext, user *user_model.User, repo *repo_model.Repository) (commitID, expectedSpec string) { + doTestScheduleUpdate(t, func(t *testing.T, u *url.URL, testContext APITestContext, user *user_model.User, repo *repo_model.Repository) (expectedSpec string) { + run := createScheduleRun(t, repo) + doAPIEditRepository(testContext, &api.EditRepoOption{ + HasActions: new(true), + })(t) + assertRunStatus(t, run, actions_model.StatusWaiting) + doAPIEditRepository(testContext, &api.EditRepoOption{ HasActions: new(false), })(t) assert.Zero(t, unittest.GetCount(t, &actions_model.ActionSchedule{RepoID: repo.ID})) + assertRunStatus(t, run, actions_model.StatusCancelled) doAPIEditRepository(testContext, &api.EditRepoOption{ HasActions: new(true), })(t) - branch, err := git_model.GetBranchExisting(t.Context(), repo.ID, repo.DefaultBranch) - assert.NoError(t, err) - return branch.CommitID, "@every 1m" + return "@every 1m" }) } -type scheduleUpdateTrigger func(t *testing.T, u *url.URL, testContext APITestContext, user *user_model.User, repo *repo_model.Repository) (commitID, expectedSpec string) +func testScheduleUpdateChangeDefaultBranch(t *testing.T) { + doTestScheduleUpdate(t, func(t *testing.T, u *url.URL, testContext APITestContext, user *user_model.User, repo *repo_model.Repository) (expectedSpec string) { + oldBranch, newBranch := repo.DefaultBranch, "release" + testAPICreateBranch(t, testContext.Session, repo.OwnerName, repo.Name, oldBranch, newBranch, http.StatusCreated) + + run := createScheduleRun(t, repo) + req := NewRequestWithValues(t, "POST", "/"+repo.FullName()+"/settings/branches", map[string]string{ + "action": "default_branch", + "branch": newBranch, + }) + testContext.Session.MakeRequest(t, req, http.StatusSeeOther) + assertRunStatus(t, run, actions_model.StatusCancelled) + unittest.AssertExistsAndLoadBean(t, &actions_model.ActionSchedule{RepoID: repo.ID, Ref: git.RefNameFromBranch(newBranch).String()}) + + run = createScheduleRun(t, repo) + doAPIEditRepository(testContext, &api.EditRepoOption{ + DefaultBranch: new(oldBranch), + })(t) + assertRunStatus(t, run, actions_model.StatusCancelled) + return "@every 1m" + }) +} + +func testScheduleUpdateRenameDefaultBranch(t *testing.T) { + doTestScheduleUpdate(t, func(t *testing.T, u *url.URL, testContext APITestContext, user *user_model.User, repo *repo_model.Repository) (expectedSpec string) { + newName := repo.DefaultBranch + "-renamed" + run := createScheduleRun(t, repo) + testAPIRenameBranch(t, user.Name, repo.OwnerName, repo.Name, repo.DefaultBranch, newName, http.StatusNoContent) + assertRunStatus(t, run, actions_model.StatusCancelled) + return "@every 1m" + }) +} + +type scheduleUpdateTrigger func(t *testing.T, u *url.URL, testContext APITestContext, user *user_model.User, repo *repo_model.Repository) (expectedSpec string) func doTestScheduleUpdate(t *testing.T, updateTrigger scheduleUpdateTrigger) { onGiteaRun(t, func(t *testing.T, u *url.URL) { @@ -260,14 +302,31 @@ jobs: scheduleSpec := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionScheduleSpec{RepoID: repo.ID, ScheduleID: actionSchedule.ID}) assert.Equal(t, "@every 1m", scheduleSpec.Spec) - commitID, expectedSpec := updateTrigger(t, u, httpContext, user2, repo) + expectedSpec := updateTrigger(t, u, httpContext, user2, repo) - actionSchedule = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionSchedule{RepoID: repo.ID, CommitSHA: commitID}) + defaultBranch := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: repo.ID}).DefaultBranch + branch, err := git_model.GetBranchExisting(t.Context(), repo.ID, defaultBranch) + require.NoError(t, err) + actionSchedule = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionSchedule{RepoID: repo.ID, Ref: git.RefNameFromBranch(defaultBranch).String(), CommitSHA: branch.CommitID}) scheduleSpec = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionScheduleSpec{RepoID: repo.ID, ScheduleID: actionSchedule.ID}) assert.Equal(t, expectedSpec, scheduleSpec.Spec) }) } +func createScheduleRun(t *testing.T, repo *repo_model.Repository) *actions_model.ActionRun { + spec := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionScheduleSpec{RepoID: repo.ID}) + spec.Repo = repo + spec.Schedule = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionSchedule{ID: spec.ScheduleID}) + require.NoError(t, actions_service.CreateScheduleTaskBySpec(t.Context(), spec)) + run := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: repo.ID, ScheduleID: spec.ScheduleID}) + assert.Equal(t, actions_model.StatusWaiting, run.Status) + return run +} + +func assertRunStatus(t *testing.T, run *actions_model.ActionRun, status actions_model.Status) { + assert.Equal(t, status, unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: run.ID}).Status) +} + func pushScheduleChange(t *testing.T, u *url.URL, repo *repo_model.Repository, newCron string) { workflowTreePath := ".gitea/workflows/actions-schedule.yml" workflowContent := `name: actions-schedule