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.

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 7 commits into
base: main
Choose a base branch
from

Conversation

NorthRealm
Copy link
Contributor

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.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 5, 2025
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Aug 5, 2025
@NorthRealm
Copy link
Contributor Author

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
Copy link
Contributor

ChristopherHX commented Aug 5, 2025

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@ChristopherHX
Copy link
Contributor

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
Copy link
Contributor Author

NorthRealm commented Aug 5, 2025

@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
Copy link
Contributor

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants