From 283a001bb3b27bdaaa5a0c79bfe89a12cb2d3fd9 Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Mon, 2 Feb 2026 23:20:41 +0100 Subject: [PATCH] fix: cancel runs pending approval when a PR is closed (#11134) Fixes #11125. When a PR is closed, cancel any action runs associated with the pull request that are not approved so that they do not remain in the Actions list as a blocked action. ## 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. - [x] 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 - [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. *The decision if the pull request will be shown in the release notes is up to the mergers / release team.* The content of the `release-notes/.md` file will serve as the basis for the release notes. If the file does not exist, the title of the pull request will be used instead. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11134 Reviewed-by: Andreas Ahlenstorf Reviewed-by: Gusted Co-authored-by: Mathieu Fenniak Co-committed-by: Mathieu Fenniak --- services/actions/notifier.go | 9 +++++++ services/actions/trust.go | 19 ++++++++++++++ services/actions/trust_test.go | 14 ++++++++++ tests/integration/actions_trust_test.go | 35 +++++++++++++++++++++++++ 4 files changed, 77 insertions(+) diff --git a/services/actions/notifier.go b/services/actions/notifier.go index 7b291d491b..401a64e8cb 100644 --- a/services/actions/notifier.go +++ b/services/actions/notifier.go @@ -138,6 +138,15 @@ func (n *actionsNotifier) IssueChangeStatus(ctx context.Context, doer *user_mode WithPayload(apiPullRequest). WithPullRequest(issue.PullRequest). Notify(ctx) + + // PR may have left unapproved runs if the author wasn't trusted, and now that it's been closed those should be + // cancelled. Note that this occurs after new events are fired -- even a 'closed' event could generate a pull + // request run that needs to be cancelled. + if isClosed { + if err := cleanupPullRequestUnapprovedRuns(ctx, issue.RepoID, issue.PullRequest.ID); err != nil { + log.Error("cleanupPullRequestUnapprovedRuns: %v", err) + } + } return } apiIssue := &api.IssuePayload{ diff --git a/services/actions/trust.go b/services/actions/trust.go index 55d97a5756..eed411bf1b 100644 --- a/services/actions/trust.go +++ b/services/actions/trust.go @@ -5,6 +5,7 @@ package actions import ( "context" + "errors" "fmt" actions_model "forgejo.org/models/actions" @@ -324,3 +325,21 @@ func pullRequestApprove(ctx context.Context, doerID, repoID, pullRequestID int64 } return nil } + +func cleanupPullRequestUnapprovedRuns(ctx context.Context, repoID, pullRequestID int64) error { + runs, err := actions_model.GetRunsThatNeedApprovalByRepoIDAndPullRequestID(ctx, repoID, pullRequestID) + if err != nil { + return err + } + + errorSlice := []error{} + for _, run := range runs { + if err := CancelRun(ctx, run); err != nil { + errorSlice = append(errorSlice, err) + } + } + if len(errorSlice) > 0 { + return errors.Join(errorSlice...) + } + return nil +} diff --git a/services/actions/trust_test.go b/services/actions/trust_test.go index d5aa08e35f..e43ab946b7 100644 --- a/services/actions/trust_test.go +++ b/services/actions/trust_test.go @@ -227,6 +227,20 @@ jobs: assert.False(t, run.NeedApproval) } }) + + t.Run("cleanupPullRequestUnapprovedRuns", func(t *testing.T) { + pullRequestID := int64(534) + runNotApproved := createPullRequestRun(t, pullRequestID, repoID) + + previousCancelledCount := unittest.GetCount(t, &actions_model.ActionRun{Status: actions_model.StatusCancelled}) + + require.NoError(t, cleanupPullRequestUnapprovedRuns(t.Context(), repoID, pullRequestID)) + + currentCancelledCount := unittest.GetCount(t, &actions_model.ActionRun{Status: actions_model.StatusCancelled}) + assert.Equal(t, previousCancelledCount+1, currentCancelledCount) + run := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: runNotApproved.ID}) + assert.Equal(t, actions_model.StatusCancelled.String(), run.Status.String()) + }) } func TestActionsTrust_GetPullRequestUserIsTrustedWithActions(t *testing.T) { diff --git a/tests/integration/actions_trust_test.go b/tests/integration/actions_trust_test.go index 651700cd26..c150bd6c2e 100644 --- a/tests/integration/actions_trust_test.go +++ b/tests/integration/actions_trust_test.go @@ -12,6 +12,7 @@ import ( "time" actions_model "forgejo.org/models/actions" + auth_model "forgejo.org/models/auth" issues_model "forgejo.org/models/issues" repo_model "forgejo.org/models/repo" unit_model "forgejo.org/models/unit" @@ -436,3 +437,37 @@ func TestActionsPullRequestTrustPanelWIPConflicts(t *testing.T) { }) }) } + +func TestActionsPullRequestTrustCancelOnClose(t *testing.T) { + onApplicationRun(t, func(t *testing.T, u *url.URL) { + ownerUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + regularUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5}) + regularSession := loginUser(t, regularUser.Name) + token := getTokenForLoggedInUser(t, regularSession, auth_model.AccessTokenScopeWriteIssue) + + baseRepo, f := actionsTrustTestCreateBaseRepo(t, ownerUser) + defer f() + + _, pullRequest, addFileToForkedResp := actionsTrustTestCreatePullRequestFromForkedRepo(t, ownerUser, baseRepo, regularUser) + prAPILink := pullRequest.Issue.APIURL(t.Context()) + + { + actionRun := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, CommitSHA: addFileToForkedResp.Commit.SHA}) + assert.True(t, actionRun.NeedApproval) + actionRunJob := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{RunID: actionRun.ID, RepoID: baseRepo.ID}) + assert.Equal(t, actions_model.StatusBlocked, actionRunJob.Status) + } + + req := NewRequestWithJSON(t, "PATCH", prAPILink, &structs.PullRequest{State: "closed"}).AddTokenAuth(token) + MakeRequest(t, req, http.StatusCreated) + + { + 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) + } + }) +}