From ce96379aef6e92cff2e9982031d5248ef8b01947 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sun, 11 Feb 2024 13:06:54 +0000 Subject: [PATCH] [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 Co-committed-by: Earl Warren (cherry picked from commit 7b4dba3aa07b2a54c922010eab63f61078123ef2) 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 | 20 +++++++++ services/actions/notifier_helper.go | 24 +++++++++++ services/actions/notifier_helper_test.go | 50 +++++++++++++++++++++++ tests/integration/actions_trigger_test.go | 4 ++ 4 files changed, 98 insertions(+) create mode 100644 services/actions/main_test.go create mode 100644 services/actions/notifier_helper_test.go diff --git a/services/actions/main_test.go b/services/actions/main_test.go new file mode 100644 index 0000000000..73598dab1b --- /dev/null +++ b/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("..", ".."), + }) +} diff --git a/services/actions/notifier_helper.go b/services/actions/notifier_helper.go index f917602034..26c86275e9 100644 --- a/services/actions/notifier_helper.go +++ b/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, diff --git a/services/actions/notifier_helper_test.go b/services/actions/notifier_helper_test.go new file mode 100644 index 0000000000..3c23414b8e --- /dev/null +++ b/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)) +} diff --git a/tests/integration/actions_trigger_test.go b/tests/integration/actions_trigger_test.go index 82c1967b52..7a3576bf4d 100644 --- a/tests/integration/actions_trigger_test.go +++ b/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}))