Prevent duplicate actions email #35215

NorthRealm · 2025-08-05T02:40:56Z

Trying to prevent duplicate action emails by adding an extra check on job status.

Producing the issue:

Spin up a local instance with notify email enabled and mailer setup. Trace log.
Create a repository.
Add a workflow with many jobs.
Run the workflow (no need to register runner) and then cancel it manually.
Observe trace log and mailbox.

NorthRealm · 2025-08-05T02:45:58Z

So I was trying to answer @inxcts question before I came across such issue. After applying the patch, I no longer see duplicate email (that's with unfinished job status while run is concluded as done). However, redoing the steps to reproduce and I still observe one duplicate attempt to send email in trace log, which means there are twice that can pass the applied check right after manually cancelling run. I notice MailActionsTrigger is triggered multiple times.
cc @ChristopherHX, could you please elaborate on how's the event triggered? Is there anywhere that sends the same event twice?

ChristopherHX · 2025-08-05T10:25:43Z

The event source is scattered. The "show call hierarchy" golang ide feature, of the event function should show every possible source of the event.

Maybe we should restructure this to trigger only completed events from the caller that is able to atomically update the database struct to success/failure/etc...

Add a workflow with many jobs

How many did you test? My unit tests have small number of jobs and could be the cause of being undetected.

Sounds like I need to audit the event source with bigger workflows, workflow_run action triggger and webhook would also trigger twice if this is true.

I thought about adding temporary traces that dumps strack traces then look at the stack traces of the doubled event

NorthRealm · 2025-08-05T10:30:02Z

@ChristopherHX Previously I had only tested the feature with like 1, 2 or 3 dummy jobs. 🤦‍♂️

on:
  push:
  workflow_dispatch:

jobs:
  test: 
    runs-on: ubuntu-latest
    steps:
      - run: exit 0

  test2: 
    needs: [test]
    runs-on: ubuntu-latest
    steps:
      - run: exit 0

  test3: 
    needs: [test, test2]
    runs-on: ubuntu-latest
    steps:
      - run: exit 0
  
  test4: 
    needs: [test, test2, test3]
    runs-on: ubuntu-latest
    steps:
      - run: exit 0
  
  test5: 
    needs: [test, test2, test4]
    runs-on: ubuntu-latest
    steps:
      - run: exit 0
  
  test6:
    strategy:
      matrix:
        os: [ubuntu-20.04, ubuntu-22.04, ubuntu-24.04] 
    needs: [test, test2, test3]
    runs-on: ${{ matrix.os }}
    steps:
      - run: exit 0
  
  test7: 
    needs: test6
    runs-on: ubuntu-latest
    steps:
      - run: exit 0
  
  test8: 
    runs-on: ubuntu-latest
    steps:
      - run: exit 0
  
  test9: 
    strategy:
      matrix:
        os: [ubuntu-20.04, ubuntu-22.04, ubuntu-24.04, ubuntu-25.04, windows-2022, windows-2025, macos-13, macos-14, macos-15]
    runs-on: ${{ matrix.os }}
    steps:
      - run: exit 0

  test10: 
    runs-on: ubuntu-latest
    steps:
      - run: exit 0

Here's what I'm currently using to triage.

NorthRealm · 2025-08-05T10:38:05Z

@ChristopherHX In routers/web/repo/actions/view.go, in Approve and Cancel, why use both NotifyWorkflowRunStatusUpdateWithReload and WorkflowRunStatusUpdate?

ChristopherHX · 2025-08-05T10:53:40Z

Thanks for taking a look :)

@ChristopherHX In routers/web/repo/actions/view.go, in Approve and Cancel, why use both NotifyWorkflowRunStatusUpdateWithReload and WorkflowRunStatusUpdate?

Could be very likely a bug that found it's way into my PR during changes by reviewers, I might missed writing a test for cancel/approve flows so this was not detected in CI

The cases where WorkflowJobStatus update follows sounds more correct

NorthRealm · 2025-08-05T11:04:00Z

@ChristopherHX I applied cdb1e80 yet still see the same one duplicate attempt. I could not figure out cause at this moment so feel free to propose changes or directly push to the branch.

ChristopherHX · 2025-08-05T11:38:54Z

Yes I can take a look at this as well.

Notice:

approve workflow run notice was expected to be triggered before running the jobs, for cancelling the order cancel jobs then notify workflow run

This reverts commit cdb1e80.

* missing test for approve

ChristopherHX · 2025-08-06T13:35:28Z

In routers/web/repo/actions/view.go, in Approve and Cancel, why use both NotifyWorkflowRunStatusUpdateWithReload and WorkflowRunStatusUpdate?

I have added a test and this has been a clear event duplication that is reproducible in tests until removing the dupped WorkflowRunStatusUpdate.

The approve flow is harder to test in my option and has been skipped by me for now.

Now I wonder what other duplication scenario did you see? Is this the same workflow, my test has no runners. Should I add runners?

NorthRealm · 2025-08-06T13:49:24Z

Quick manual test. I can't reproduce it now. I am not entirely sure about it, as I do not understand the difference. Yesterday in cdb1e80 I applied the same patch removing the unneeded WorkflowRunStatusUpdate calls but still observed the issue. While after your bc3a467 the issue seems to be eliminated.

NorthRealm · 2025-08-06T13:55:15Z

@ChristopherHX My manual test has no actual runner. I simply just observe event triggering sending emails. You might want it anyway.

ChristopherHX

Thanks for finding two workflow_run delivery problems

ChristopherHX · 2025-08-06T19:15:17Z

services/mailer/mail_workflow_run.go

+	for _, job := range jobs {
+		if !job.Status.IsDone() {
+			log.Trace("composeAndSendActionsWorkflowRunStatusEmail: A job is not done. Will not compose and send actions email.")
+			return
+		}
+	}


this is now no operation, but still useful for diagnostic of other undetected faults, other than adding a workflow_run webhook and looking at the past deliveries.

It seem this has been checked in https://github.com/go-gitea/gitea/blob/main/services/mailer/notify.go#L211?

@lunny Based on how job status is aggregated, that check is not 100% reliable. Before patch I got this erroneous email:

What will happen if a waiting status is considered IsDone?

Also, it’s quite strange that there are three different places checking whether the jobs should be sent.

Intentional. You got better solution?

Before patch I got this erroneous email:

How to reproduce this bug? This should never send a completed workflow run event.

IMO this should be fixed in the workflow_run event itself and the event should be sent if it is completed not if some are completed (except if you spam rerun and cancelation of random jobs to force inconsistency

Other valid events are before starting any job

I switch back to main branch on c4c1a4b and reproduced the bug, by starting a run manually then immediately canceling it. Trace log show there are 2 email attempts.

How does Gitea handle mailer failure? I forgot to turn on mailbox at first on that day and Gitea printed errors in background. Will emails fail to send just go into smoke?

I switch back to main branch on c4c1a4b and reproduced the bug, by starting a run manually then immediately canceling it. Trace log show there are 2 email attempts.

yes 2 email attempts, but that one is fixed here. But I am writing about that my automated test here can not detect the situation that not all jobs are completed if the run completion event has been seen.

by starting a run manually then immediately canceling it.

this is actually what my test added here literally do, but if I add this assert, log.Fatal is never run for me. Even if I run this over and over again. In my point of view there must be some detail other than just cancelling directly after triggering the run without runners.

I placed this code directly in notify.go in WorkflowRunStatusUpdate

if run.Status.IsDone() { jobs, err := actions_model.GetRunJobsByRunID(ctx, run.ID) if err != nil { log.Error("GetRunJobsByRunID: %v", err) return } for _, job := range jobs { if !job.Status.IsDone() { log.Fatal("WorkflowRunStatusUpdate: A job is not done. Will not notify workflow run status update.") return } } }

Do I have to do manual testing to see this? Even if I revert the duplicated event delivery, I only got a duplicated event instead of an event before all jobs are finished.

CancelAbandonedJobs is broken, and may send workflow_run events.
Rerun Multiple jobs is called multiple times, so creates multiple events (should be filtered by email via run is Done)

tests/integration/repo_webhook_test.go

lunny · 2025-08-09T18:35:19Z

services/mailer/notify.go

@@ -208,8 +208,5 @@ func (m *mailNotifier) RepoPendingTransfer(ctx context.Context, doer, newOwner *
 }

 func (m *mailNotifier) WorkflowRunStatusUpdate(ctx context.Context, repo *repo_model.Repository, sender *user_model.User, run *actions_model.ActionRun) {
-	if !run.Status.IsDone() {
-		return
-	}
 	MailActionsTrigger(ctx, sender, repo, run)


The last one from me, then the function MailActionsTrigger is unnecessary now, all the code could be extract into this function.

NorthRealm added 3 commits August 5, 2025 09:54

fix

21e7eef

add trace

0ec1bb3

add trace

3957352

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 5, 2025

github-actions bot added the modifies/go Pull requests that update Go code label Aug 5, 2025

NorthRealm added 2 commits August 5, 2025 18:02

assume

cdb1e80

add trace

7384a65

update

1d9c378

NorthRealm and others added 2 commits August 5, 2025 19:51

Revert "assume"

c3a7f15

This reverts commit cdb1e80.

remove duplicate workflow run trigger + add test for cancel

bc3a467

* missing test for approve

update

cf547bb

Merge branch 'main' into patch-actions-email-2

8901e59

NorthRealm marked this pull request as ready for review August 8, 2025 13:53

Merge branch 'main' into patch-actions-email-2

a9a826f

ChristopherHX approved these changes Aug 8, 2025

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 8, 2025

lunny reviewed Aug 8, 2025

View reviewed changes

tests/integration/repo_webhook_test.go Outdated Show resolved Hide resolved

ChristopherHX and others added 2 commits August 8, 2025 22:49

remove timeout

e212011

update

0506162

lunny reviewed Aug 9, 2025

View reviewed changes

Uh oh!

Prevent duplicate actions email #35215

Are you sure you want to change the base?

Prevent duplicate actions email #35215

Conversation

NorthRealm commented Aug 5, 2025

Uh oh!

NorthRealm commented Aug 5, 2025

Uh oh!

ChristopherHX commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NorthRealm commented Aug 5, 2025

Uh oh!

NorthRealm commented Aug 5, 2025

Uh oh!

ChristopherHX commented Aug 5, 2025

Uh oh!

NorthRealm commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChristopherHX commented Aug 5, 2025

Uh oh!

ChristopherHX commented Aug 6, 2025

Uh oh!

NorthRealm commented Aug 6, 2025

Uh oh!

NorthRealm commented Aug 6, 2025

Uh oh!

ChristopherHX left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristopherHX Aug 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ChristopherHX commented Aug 5, 2025 •

edited

Loading

NorthRealm commented Aug 5, 2025 •

edited

Loading

ChristopherHX Aug 9, 2025 •

edited

Loading