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