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 <zettat123@gmail.com>

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

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

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

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 {
+		// replacing the schedules leaves their in-progress runs alone, as GitHub does
-			log.Error("CleanRepoScheduleTasks: %v", err)
+		if err := actions_model.DeleteScheduleTaskByRepo(ctx, input.Repo.ID); err != nil {
+			log.Error("DeleteScheduleTaskByRepo: %v", err)
 		}
 	}
 

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,

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
 }

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)
+		assertRunStatus(t, run, actions_model.StatusWaiting)
-		assert.NoError(t, err)
+		return newCron
-		return branch.CommitID, 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,
 				})
-
+				return "@every 2m"
-				pull := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: apiPull.ID})
-				return pull.MergedCommitID, "@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)
+		return "@every 1m"
-		assert.NoError(t, err)
-		return branch.CommitID, "@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)
+		return "@every 1m"
-		assert.NoError(t, err)
-		return branch.CommitID, "@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