Skip to content

Prevent duplicate actions email #35215

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
+461 −19
Draft

Prevent duplicate actions email #35215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
21e7eef
fix
NorthRealm Aug 5, 2025
0ec1bb3
add trace
NorthRealm Aug 5, 2025
3957352
add trace
NorthRealm Aug 5, 2025
cdb1e80
assume
NorthRealm Aug 5, 2025
7384a65
add trace
NorthRealm Aug 5, 2025
1d9c378
update
NorthRealm Aug 5, 2025
c3a7f15
Revert "assume"
NorthRealm Aug 5, 2025
bc3a467
remove duplicate workflow run trigger + add test for cancel
ChristopherHX Aug 6, 2025
cf547bb
update
NorthRealm Aug 6, 2025
8901e59
Merge branch 'main' into patch-actions-email-2
NorthRealm Aug 8, 2025
a9a826f
Merge branch 'main' into patch-actions-email-2
NorthRealm Aug 8, 2025
e212011
remove timeout
ChristopherHX Aug 8, 2025
0506162
update
NorthRealm Aug 9, 2025
6bcdd7e
fix more workflow_run completion events
ChristopherHX Aug 10, 2025
3a44248
modernize
ChristopherHX Aug 10, 2025
3d8b2f5
Merge branch 'main' into patch-actions-email-2
NorthRealm Aug 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions routers/web/repo/actions/view.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,12 @@ func Rerun(ctx *context_module.Context) {
ctx.ServerError("UpdateRun", err)
return
}

if err := run.LoadAttributes(ctx); err != nil {
ctx.ServerError("run.LoadAttributes", err)
return
}
notify_service.WorkflowRunStatusUpdate(ctx, run.Repo, run.TriggerUser, run)
}

job, jobs := getRunJobs(ctx, runIndex, jobIndex)
Expand Down Expand Up @@ -485,7 +491,6 @@ func rerunJob(ctx *context_module.Context, job *actions_model.ActionRunJob, shou
}

actions_service.CreateCommitStatus(ctx, job)
actions_service.NotifyWorkflowRunStatusUpdateWithReload(ctx, job)
notify_service.WorkflowJobStatusUpdate(ctx, job.Run.Repo, job.Run.TriggerUser, job, nil)

return nil
Expand Down Expand Up @@ -560,9 +565,8 @@ func Cancel(ctx *context_module.Context) {
if len(updatedjobs) > 0 {
job := updatedjobs[0]
actions_service.NotifyWorkflowRunStatusUpdateWithReload(ctx, job)
notify_service.WorkflowRunStatusUpdate(ctx, job.Run.Repo, job.Run.TriggerUser, job.Run)
}
ctx.JSON(http.StatusOK, struct{}{})
ctx.JSONOK()
}

func Approve(ctx *context_module.Context) {
Expand Down Expand Up @@ -606,15 +610,14 @@ func Approve(ctx *context_module.Context) {
if len(updatedjobs) > 0 {
job := updatedjobs[0]
actions_service.NotifyWorkflowRunStatusUpdateWithReload(ctx, job)
notify_service.WorkflowRunStatusUpdate(ctx, job.Run.Repo, job.Run.TriggerUser, job.Run)
}

for _, job := range updatedjobs {
_ = job.LoadAttributes(ctx)
notify_service.WorkflowJobStatusUpdate(ctx, job.Run.Repo, job.Run.TriggerUser, job, nil)
}

ctx.JSON(http.StatusOK, struct{}{})
ctx.JSONOK()
}

func Delete(ctx *context_module.Context) {
Expand Down
26 changes: 21 additions & 5 deletions services/actions/clear_tasks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,8 @@ func notifyWorkflowJobStatusUpdate(ctx context.Context, jobs []*actions_model.Ac
_ = job.LoadAttributes(ctx)
notify_service.WorkflowJobStatusUpdate(ctx, job.Run.Repo, job.Run.TriggerUser, job, nil)
}
if len(jobs) > 0 {
job := jobs[0]
notify_service.WorkflowRunStatusUpdate(ctx, job.Run.Repo, job.Run.TriggerUser, job.Run)
}
job := jobs[0]
notify_service.WorkflowRunStatusUpdate(ctx, job.Run.Repo, job.Run.TriggerUser, job.Run)
}
}

Expand Down Expand Up @@ -113,6 +111,10 @@ func CancelAbandonedJobs(ctx context.Context) error {
}

now := timeutil.TimeStampNow()

// Collect one job per run to send workflow run status update
updatedRuns := map[int64]*actions_model.ActionRunJob{}

for _, job := range jobs {
job.Status = actions_model.StatusCancelled
job.Stopped = now
Expand All @@ -127,10 +129,24 @@ func CancelAbandonedJobs(ctx context.Context) error {
}
CreateCommitStatus(ctx, job)
if updated {
NotifyWorkflowRunStatusUpdateWithReload(ctx, job)
updatedRuns[job.RunID] = job
notify_service.WorkflowJobStatusUpdate(ctx, job.Run.Repo, job.Run.TriggerUser, job, nil)
}
}

for _, job := range updatedRuns {
c, err := db.Count[actions_model.ActionRunJob](ctx, actions_model.FindRunJobOptions{
RunID: job.RunID,
Statuses: []actions_model.Status{actions_model.StatusWaiting, actions_model.StatusBlocked, actions_model.StatusRunning},
})
if err != nil {
log.Error("Count waiting jobs for run %d: %v", job.RunID, err)
continue
}
if c == 0 {
NotifyWorkflowRunStatusUpdateWithReload(ctx, job)
}
}

return nil
}
21 changes: 15 additions & 6 deletions services/mailer/mail_workflow_run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,18 @@ func generateMessageIDForActionsWorkflowRunStatusEmail(repo *repo_model.Reposito
}

func composeAndSendActionsWorkflowRunStatusEmail(ctx context.Context, repo *repo_model.Repository, run *actions_model.ActionRun, sender *user_model.User, recipients []*user_model.User) {
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.Trace("composeAndSendActionsWorkflowRunStatusEmail: A job is not done. Will not compose and send actions email.")
return
}
}
Comment on lines +42 to +47
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional. You got better solution?

Copy link
Contributor

@ChristopherHX ChristopherHX Aug 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before patch I got this erroneous email:
1

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


subject := "Run"
switch run.Status {
case actions_model.StatusFailure:
Expand All @@ -48,11 +60,6 @@ func composeAndSendActionsWorkflowRunStatusEmail(ctx context.Context, repo *repo
messageID := generateMessageIDForActionsWorkflowRunStatusEmail(repo, run)
metadataHeaders := generateMetadataHeaders(repo)

jobs, err := actions_model.GetRunJobsByRunID(ctx, run.ID)
if err != nil {
log.Error("GetRunJobsByRunID: %v", err)
return
}
sort.SliceStable(jobs, func(i, j int) bool {
si, sj := jobs[i].Status, jobs[j].Status
/*
Expand Down Expand Up @@ -116,6 +123,7 @@ func composeAndSendActionsWorkflowRunStatusEmail(ctx context.Context, repo *repo
}
msgs := make([]*sender_service.Message, 0, len(tos))
for _, rec := range tos {
log.Trace("Sending actions email to %s (UID: %d)", rec.Name, rec.ID)
msg := sender_service.NewMessageFrom(
rec.Email,
displayName,
Expand All @@ -141,7 +149,7 @@ func MailActionsTrigger(ctx context.Context, sender *user_model.User, repo *repo
if setting.MailService == nil {
return
}
if run.Status.IsSkipped() {
if !run.Status.IsDone() || run.Status.IsSkipped() {
return
}

Expand All @@ -160,6 +168,7 @@ func MailActionsTrigger(ctx context.Context, sender *user_model.User, repo *repo
}

if len(recipients) > 0 {
log.Trace("MailActionsTrigger: Initiate email composition")
composeAndSendActionsWorkflowRunStatusEmail(ctx, repo, run, sender, recipients)
}
}
3 changes: 0 additions & 3 deletions services/mailer/notify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
Loading