[ACTIONS] skip superflous pull request synchronized event (#2314)

Skip a HookEventPullRequestSync event if it has the same CommitSHA as an existing HookEventPullRequest event in the ActionRun table. A HookEventPullRequestSync event must only create an ActionRun if the CommitSHA is different from what it was when the PR was open.

This guards against a race that can happen when the following is done in parallel:

* A commit C is pushed to a repo on branch B
* A pull request with head on branch B

it is then possible that the pull request is created first, successfully. The commit that was just pushed is not known yet but the PR only references the repository and the B branch so it is fine.

A HookEventPullRequest event is sent to the notification queue but not processed immediately.

The commit C is pushed and processed successfully. Since the PR already exists and has a head that matches the branch, the head of the PR is updated with the commit C and a HookEventPullRequestSync event is sent to the notification queue.

The HookEventPullRequest event is processed and since the head of the PR was updated to be commit C, an ActionRun with CommitSHA C is created.

The HookEventPullRequestSync event is then processed and also has a CommitSHA equal to C.

Refs: https://codeberg.org/forgejo/forgejo/issues/2009
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/2314
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>
(cherry picked from commit 7b4dba3aa0)

Conflicts:
	services/actions/notifier_helper.go
	tests/integration/actions_trigger_test.go
	trivial context conficts
	services/actions/main_test.go is different in v1.21

services/actions/main_test.go

@@ -0,0 +1,20 @@
+// Copyright 2024 The Forgejo Authors
+// SPDX-License-Identifier: MIT
+
+package actions
+
+import (
+	"path/filepath"
+	"testing"
+
+	"code.gitea.io/gitea/models/unittest"
+
+	_ "code.gitea.io/gitea/models/actions"
+	_ "code.gitea.io/gitea/models/activities"
+)
+
+func TestMain(m *testing.M) {
+	unittest.MainTest(m, &unittest.TestOptions{
+		GiteaRootPath: filepath.Join("..", ".."),
+	})
+}

services/actions/notifier_helper.go

@@ -10,6 +10,7 @@ import (
 	"strings"
 
 	actions_model "code.gitea.io/gitea/models/actions"
+	"code.gitea.io/gitea/models/db"
 	issues_model "code.gitea.io/gitea/models/issues"
 	packages_model "code.gitea.io/gitea/models/packages"
 	access_model "code.gitea.io/gitea/models/perm/access"
@@ -144,6 +145,11 @@ func notify(ctx context.Context, input *notifyInput) error {
 		return fmt.Errorf("gitRepo.GetCommit: %w", err)
 	}
 
+	if SkipPullRequestEvent(ctx, input.Event, input.Repo.ID, commit.ID.String()) {
+		log.Trace("repo %s with commit %s skip event %v", input.Repo.RepoPath(), commit.ID, input.Event)
+		return nil
+	}
+
 	var detectedWorkflows []*actions_module.DetectedWorkflow
 	actionsConfig := input.Repo.MustGetUnit(ctx, unit_model.TypeActions).ActionsConfig()
 	workflows, schedules, err := actions_module.DetectWorkflows(gitRepo, commit, input.Event, input.Payload)
@@ -195,6 +201,24 @@ func notify(ctx context.Context, input *notifyInput) error {
 	return handleWorkflows(ctx, detectedWorkflows, commit, input, ref)
 }
 
+func SkipPullRequestEvent(ctx context.Context, event webhook_module.HookEventType, repoID int64, commitSHA string) bool {
+	if event != webhook_module.HookEventPullRequestSync {
+		return false
+	}
+
+	run := actions_model.ActionRun{
+		Event:     webhook_module.HookEventPullRequest,
+		RepoID:    repoID,
+		CommitSHA: commitSHA,
+	}
+	exist, err := db.GetEngine(ctx).Exist(&run)
+	if err != nil {
+		log.Error("Exist ActionRun %v: %v", run, err)
+		return false
+	}
+	return exist
+}
+
 func handleWorkflows(
 	ctx context.Context,
 	detectedWorkflows []*actions_module.DetectedWorkflow,

services/actions/notifier_helper_test.go

@@ -0,0 +1,50 @@
+// Copyright 2024 The Forgejo Authors
+// SPDX-License-Identifier: MIT
+
+package actions
+
+import (
+	"testing"
+
+	actions_model "code.gitea.io/gitea/models/actions"
+	"code.gitea.io/gitea/models/db"
+	"code.gitea.io/gitea/models/unittest"
+	webhook_module "code.gitea.io/gitea/modules/webhook"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_SkipPullRequestEvent(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+
+	repoID := int64(1)
+	commitSHA := "1234"
+
+	// event is not webhook_module.HookEventPullRequestSync, never skip
+	assert.False(t, SkipPullRequestEvent(db.DefaultContext, webhook_module.HookEventPullRequest, repoID, commitSHA))
+
+	// event is webhook_module.HookEventPullRequestSync but there is nothing in the ActionRun table, do not skip
+	assert.False(t, SkipPullRequestEvent(db.DefaultContext, webhook_module.HookEventPullRequestSync, repoID, commitSHA))
+
+	// there is a webhook_module.HookEventPullRequest event but the SHA is different, do not skip
+	index := int64(1)
+	run := &actions_model.ActionRun{
+		Index:     index,
+		Event:     webhook_module.HookEventPullRequest,
+		RepoID:    repoID,
+		CommitSHA: "othersha",
+	}
+	unittest.AssertSuccessfulInsert(t, run)
+	assert.False(t, SkipPullRequestEvent(db.DefaultContext, webhook_module.HookEventPullRequestSync, repoID, commitSHA))
+
+	// there already is a webhook_module.HookEventPullRequest with the same SHA, skip
+	index++
+	run = &actions_model.ActionRun{
+		Index:     index,
+		Event:     webhook_module.HookEventPullRequest,
+		RepoID:    repoID,
+		CommitSHA: commitSHA,
+	}
+	unittest.AssertSuccessfulInsert(t, run)
+	assert.True(t, SkipPullRequestEvent(db.DefaultContext, webhook_module.HookEventPullRequestSync, repoID, commitSHA))
+}

tests/integration/actions_trigger_test.go

@@ -18,6 +18,8 @@ import (
 	user_model "code.gitea.io/gitea/models/user"
 	actions_module "code.gitea.io/gitea/modules/actions"
 	"code.gitea.io/gitea/modules/git"
+	webhook_module "code.gitea.io/gitea/modules/webhook"
+	actions_service "code.gitea.io/gitea/services/actions"
 	pull_service "code.gitea.io/gitea/services/pull"
 	repo_service "code.gitea.io/gitea/services/repository"
 	files_service "code.gitea.io/gitea/services/repository/files"
@@ -135,6 +137,8 @@ func TestPullRequestTargetEvent(t *testing.T) {
 		}
 		err = pull_service.NewPullRequest(git.DefaultContext, baseRepo, pullIssue, nil, nil, pullRequest, nil)
 		assert.NoError(t, err)
+		// if a PR "synchronized" event races the "opened" event by having the same SHA, it must be skipped. See https://codeberg.org/forgejo/forgejo/issues/2009.
+		assert.True(t, actions_service.SkipPullRequestEvent(git.DefaultContext, webhook_module.HookEventPullRequestSync, baseRepo.ID, addFileToForkedResp.Commit.SHA))
 
 		// load and compare ActionRun
 		assert.Equal(t, 1, unittest.GetCount(t, &actions_model.ActionRun{RepoID: baseRepo.ID}))