From 508bb7f2aed62479b03335b3591bd890260cfe37 Mon Sep 17 00:00:00 2001 From: limiting-factor Date: Sat, 9 May 2026 04:46:56 +0200 Subject: [PATCH] fix: in actions_service cancelJobsForRun is bugous use killRun instead (#12366) The cancelJobsForRun function is redundant with the killRun function and has bugs: - It does not use a transaction and may fail in a non-recoverable way - It does not update the commit status of the run - It does not set NeedRemoval to false if needed Remove the cancelJobsForRun function and use killRun instead (fixing forgejo/forgejo#12386). Both calls are covered by existing tests: - TestCancelPreviousJobs - TestCancelPreviousWithConcurrencyGroup A new integration test TestActionsPullRequestTrustPushCancel is added to verify that the NeedApproval field is set to false whenever a run is cancelled (fixing forgejo/forgejo#12350). Closes forgejo/forgejo#12350 Closes forgejo/forgejo#12386 --- Reverting the change fails the test at https://codeberg.org/limiting-factor/forgejo/src/commit/b6178e5634c230006a3c2b791ec8e06f09367e2d/tests/integration/actions_trust_test.go#L520-L533 with: ``` TAGS='sqlite sqlite_unlock_notify' make 'test-sqlite#TestActionsPullRequestTrustPushCancel' ... actions_trust_test.go:523: Error Trace: /home/limiting-factor/forgejo/tests/integration/actions_trust_test.go:523 /home/limiting-factor/forgejo/tests/integration/git_helper_for_declarative_test.go:98 /home/limiting-factor/forgejo/tests/integration/actions_trust_test.go:476 Error: Should be false Test: TestActionsPullRequestTrustPushCancel ``` ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. All work and communication must conform to Forgejo's [AI Agreement](https://codeberg.org/forgejo/governance/src/branch/main/AIAgreement.md). 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 for Go changes (can be removed for JavaScript changes) - I added test coverage for Go changes... - [ ] in their respective `*_test.go` for unit tests. - [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I ran... - [x] `make pr-go` before pushing ### 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 - [x] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change. - [ ] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change. ## Release notes - User Interface bug fixes - [PR](https://codeberg.org/forgejo/forgejo/pulls/12366): When the author of a pull request is [denied the right to run Actions](https://forgejo.org/docs/next/user/actions/security-pull-request/) by clicking on the "Deny" button on the pull request trust management panel, the workflow runs created for all commits pushed to the pull request are cancelled. Before that, runs that were automatically cancelled because a newer commit was pushed to the pull request [were stuck in a state waiting for approval](https://codeberg.org/forgejo/forgejo/issues/12350). Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12366 Reviewed-by: Andreas Ahlenstorf Reviewed-by: Mathieu Fenniak --- models/actions/run.go | 4 +- release-notes/12366.md | 1 + services/actions/schedule_tasks.go | 63 +++---------------------- tests/integration/actions_trust_test.go | 62 ++++++++++++++++++++++++ 4 files changed, 71 insertions(+), 59 deletions(-) create mode 100644 release-notes/12366.md diff --git a/models/actions/run.go b/models/actions/run.go index 862125224f..692dab841b 100644 --- a/models/actions/run.go +++ b/models/actions/run.go @@ -347,7 +347,7 @@ func UpdateRunApprovalByID(ctx context.Context, id int64, approval ApprovalType, func GetRunsNotDoneByRepoIDAndPullRequestPosterID(ctx context.Context, repoID, pullRequestPosterID int64) ([]*ActionRun, error) { var runs []*ActionRun // performance relies on indexes on repo_id and status - if err := db.GetEngine(ctx).Where("repo_id=? AND pull_request_poster_id=?", repoID, pullRequestPosterID).And(builder.In("status", []Status{StatusUnknown, StatusWaiting, StatusRunning, StatusBlocked})).Find(&runs); err != nil { + if err := db.GetEngine(ctx).Where("repo_id=? AND pull_request_poster_id=?", repoID, pullRequestPosterID).And(builder.In("status", PendingStatuses())).Find(&runs); err != nil { return nil, err } return runs, nil @@ -356,7 +356,7 @@ func GetRunsNotDoneByRepoIDAndPullRequestPosterID(ctx context.Context, repoID, p func GetRunsNotDoneByRepoIDAndPullRequestID(ctx context.Context, repoID, pullRequestID int64) ([]*ActionRun, error) { var runs []*ActionRun // performance relies on indexes on repo_id and status - if err := db.GetEngine(ctx).Where("repo_id=? AND pull_request_id=?", repoID, pullRequestID).And(builder.In("status", []Status{StatusUnknown, StatusWaiting, StatusRunning, StatusBlocked})).Find(&runs); err != nil { + if err := db.GetEngine(ctx).Where("repo_id=? AND pull_request_id=?", repoID, pullRequestID).And(builder.In("status", PendingStatuses())).Find(&runs); err != nil { return nil, err } return runs, nil diff --git a/release-notes/12366.md b/release-notes/12366.md new file mode 100644 index 0000000000..12cc3c478d --- /dev/null +++ b/release-notes/12366.md @@ -0,0 +1 @@ +When the author of a pull request is [denied the right to run Actions](https://forgejo.org/docs/next/user/actions/security-pull-request/) by clicking on the "Deny" button on the pull request trust management panel, the workflow runs created for all commits pushed to the pull request are cancelled. Before that, runs that were automatically cancelled because a newer commit was pushed to the pull request [were stuck in a state waiting for approval](https://codeberg.org/forgejo/forgejo/issues/12350). diff --git a/services/actions/schedule_tasks.go b/services/actions/schedule_tasks.go index fab9ae8df5..a1e9874984 100644 --- a/services/actions/schedule_tasks.go +++ b/services/actions/schedule_tasks.go @@ -23,7 +23,6 @@ import ( "code.forgejo.org/forgejo/runner/v12/act/jobparser" act_model "code.forgejo.org/forgejo/runner/v12/act/model" "github.com/gdgvda/cron" - "xorm.io/builder" ) // StartScheduleTasks start the task @@ -220,7 +219,7 @@ func CancelPreviousJobs(ctx context.Context, repoID int64, ref, workflowID strin // Iterate over each found run and cancel its associated jobs. errorSlice := []error{} for _, run := range runs { - err := cancelJobsForRun(ctx, run.ID) + err := killRun(ctx, run, actions_model.StatusCancelled) errorSlice = append(errorSlice, err) } err = errors.Join(errorSlice...) @@ -236,21 +235,21 @@ func CancelPreviousWithConcurrencyGroup(ctx context.Context, repoID int64, concu // Find all runs in the concurrency group which have at least one job that is still pending; we can't use the run's // status for this because runs are set to failed if a single job is marked as failed, even if other jobs are still // running. - runIDs := make([]int64, 0, 10) + runs := make([]*actions_model.ActionRun, 0, 10) if err := db.GetEngine(ctx).Table("action_run"). Join("INNER", "action_run_job", "action_run_job.run_id = action_run.id"). Where("action_run.repo_id = ? AND action_run.concurrency_group = ?", repoID, strings.ToLower(concurrencyGroup)). In("action_run_job.status", actions_model.PendingStatuses()). Distinct("action_run.id"). - Select("action_run.id"). - Find(&runIDs); err != nil { + Select("action_run.id,action_run.need_approval"). + Find(&runs); err != nil { return err } // Iterate over each found run and cancel its associated jobs. errorSlice := []error{} - for _, runID := range runIDs { - err := cancelJobsForRun(ctx, runID) + for _, run := range runs { + err := killRun(ctx, run, actions_model.StatusCancelled) errorSlice = append(errorSlice, err) } err := errors.Join(errorSlice...) @@ -261,56 +260,6 @@ func CancelPreviousWithConcurrencyGroup(ctx context.Context, repoID int64, concu return nil } -func cancelJobsForRun(ctx context.Context, runID int64) error { - // Find all jobs associated with the current run. - jobs, err := db.Find[actions_model.ActionRunJob](ctx, actions_model.FindRunJobOptions{ - RunID: runID, - }) - if err != nil { - return err - } - - // Iterate over each job and attempt to cancel it. - errorSlice := []error{} - for _, job := range jobs { - // Skip jobs that are already in a terminal state (completed, cancelled, etc.). - status := job.Status - if status.IsDone() { - continue - } - - // If the job has no associated task (probably an error), set its status to 'Cancelled' and stop it. - if job.TaskID == 0 { - job.Status = actions_model.StatusCancelled - job.Stopped = timeutil.TimeStampNow() - - // Update the job's status and stopped time in the database. - n, err := UpdateRunJob(ctx, job, builder.Eq{"task_id": 0}, "status", "stopped") - if err != nil { - errorSlice = append(errorSlice, err) - continue - } - - // If the update affected 0 rows, it means the job has changed in the meantime, so we need to try again. - if n == 0 { - errorSlice = append(errorSlice, errors.New("job has changed, try again")) - continue - } - - // Continue with the next job. - continue - } - - // If the job has an associated task, try to stop the task, effectively cancelling the job. - if err := StopTask(ctx, job.TaskID, actions_model.StatusCancelled); err != nil { - errorSlice = append(errorSlice, err) - continue - } - } - - return errors.Join(errorSlice...) -} - func CleanRepoScheduleTasks(ctx context.Context, repo *repo_model.Repository, cancelPreviousJobs bool) error { // If actions disabled when there is schedule task, this will remove the outdated schedule tasks // There is no other place we can do this because the app.ini will be changed manually diff --git a/tests/integration/actions_trust_test.go b/tests/integration/actions_trust_test.go index c150bd6c2e..c1449aeda2 100644 --- a/tests/integration/actions_trust_test.go +++ b/tests/integration/actions_trust_test.go @@ -471,3 +471,65 @@ func TestActionsPullRequestTrustCancelOnClose(t *testing.T) { } }) } + +func TestActionsPullRequestTrustPushCancel(t *testing.T) { + onApplicationRun(t, func(t *testing.T, u *url.URL) { + ownerUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + ownerSession := loginUser(t, ownerUser.Name) + + regularUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5}) + + baseRepo, f := actionsTrustTestCreateBaseRepo(t, ownerUser) + defer f() + + forkedRepo, pullRequest, addFileToForkedResp := actionsTrustTestCreatePullRequestFromForkedRepo(t, ownerUser, baseRepo, regularUser) + pullRequestLink := pullRequest.Issue.Link() + + // there is one commit in the pull request and it is blocked from running actions pending approval + { + actionRun := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, CommitSHA: addFileToForkedResp.Commit.SHA}) + assert.True(t, actionRun.NeedApproval) + assert.Equal(t, actions_model.StatusWaiting, actionRun.Status) + actionRunJob := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{RunID: actionRun.ID, RepoID: baseRepo.ID}) + assert.Equal(t, actions_model.StatusBlocked, actionRunJob.Status) + } + + // another commit is pushed to the head branch of the pull request + otherFileInForkResp := actionsTrustTestRepoModify(t, regularUser, baseRepo, forkedRepo, "add_file_one.txt") + + // there are two commits + // - the oldest one switched from Blocked to Cancelled and no longer needs approval + // - the newest one is Blocked and pending approval + { + actionRun := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, CommitSHA: addFileToForkedResp.Commit.SHA}) + assert.False(t, actionRun.NeedApproval) + assert.Equal(t, actions_model.StatusCancelled, actionRun.Status) + actionRunJob := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{RunID: actionRun.ID, RepoID: baseRepo.ID}) + assert.Equal(t, actions_model.StatusCancelled, actionRunJob.Status) + + otherActionRun := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, CommitSHA: otherFileInForkResp.Commit.SHA}) + assert.True(t, otherActionRun.NeedApproval) + assert.Equal(t, actions_model.StatusWaiting, otherActionRun.Status) + otherActionRunJob := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{RunID: otherActionRun.ID, RepoID: baseRepo.ID}) + assert.Equal(t, actions_model.StatusBlocked, otherActionRunJob.Status) + } + + actionsTrustTestAssertTrustPanel(t, ownerSession, pullRequestLink) + actionsTrustTestClickTrustPanel(t, ownerSession, pullRequestLink, string(actions_service.UserTrustDenied)) + + // there are two commits, both are Cancelled and not pending approval + { + actionRun := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, CommitSHA: addFileToForkedResp.Commit.SHA}) + assert.False(t, actionRun.NeedApproval) + assert.Equal(t, actions_model.StatusCancelled, actionRun.Status) + actionRunJob := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{RunID: actionRun.ID, RepoID: baseRepo.ID}) + assert.Equal(t, actions_model.StatusCancelled, actionRunJob.Status) + + otherActionRun := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, CommitSHA: otherFileInForkResp.Commit.SHA}) + assert.False(t, otherActionRun.NeedApproval) + assert.Equal(t, actions_model.StatusCancelled, otherActionRun.Status) + otherActionRunJob := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{RunID: otherActionRun.ID, RepoID: baseRepo.ID}) + assert.Equal(t, actions_model.StatusCancelled, otherActionRunJob.Status) + } + }) +}