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/<pull request number>.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 <aahlenst@noreply.codeberg.org>
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net>

Author: mfenniak

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{

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

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

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