[v12.0/forgejo] fix: disable Forgejo Actions email notifications on recovery (#8390)

**Backport:** https://codeberg.org/forgejo/forgejo/pulls/8374

- Make the migration to add an index a noop - do not remove it as it would break v12.next & v13.next
- Keep the logic that relies on finding the last run, only always fail to find which is the same as assuming each run is one of a kind

Refs forgejo/forgejo#8373

Co-authored-by: Earl Warren <contact@earl-warren.org>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8390
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Co-authored-by: forgejo-backport-action <forgejo-backport-action@noreply.codeberg.org>
Co-committed-by: forgejo-backport-action <forgejo-backport-action@noreply.codeberg.org>

models/actions/run.go

@@ -284,16 +284,10 @@ func GetLatestRun(ctx context.Context, repoID int64) (*ActionRun, error) {
 	return &run, nil
 }
 
-// GetRunBefore returns the last run that completed a given timestamp (not inclusive).
+func GetRunBefore(ctx context.Context, _ *ActionRun) (*ActionRun, error) {
-func GetRunBefore(ctx context.Context, repoID int64, timestamp timeutil.TimeStamp) (*ActionRun, error) {
+	// TODO return the most recent run related to the run given in argument
-	var run ActionRun
+	// see https://codeberg.org/forgejo/user-research/issues/63 for context
-	has, err := db.GetEngine(ctx).Where("repo_id=? AND stopped IS NOT NULL AND stopped<?", repoID, timestamp).OrderBy("stopped DESC").Limit(1).Get(&run)
+	return nil, nil
-	if err != nil {
-		return nil, err
-	} else if !has {
-		return nil, fmt.Errorf("run before: %w", util.ErrNotExist)
-	}
-	return &run, nil
 }
 
 func GetLatestRunForBranchAndWorkflow(ctx context.Context, repoID int64, branch, workflowFile, event string) (*ActionRun, error) {

models/actions/run_test.go

@@ -5,92 +5,7 @@ package actions
 
 import (
 	"testing"
-	"time"
-
-	"forgejo.org/models/db"
-	"forgejo.org/models/unittest"
-	"forgejo.org/modules/timeutil"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )
 
 func TestGetRunBefore(t *testing.T) {
-	require.NoError(t, unittest.PrepareTestDatabase())
-
-	// this repo is part of the test database requiring loading "repository.yml" in main_test.go
-	var repoID int64 = 1
-
-	workflowID := "test_workflow"
-
-	// third completed run
-	time1, err := time.Parse(time.RFC3339, "2024-07-31T15:47:55+08:00")
-	require.NoError(t, err)
-	timeutil.MockSet(time1)
-	run1 := ActionRun{
-		ID:         1,
-		Index:      1,
-		RepoID:     repoID,
-		Stopped:    timeutil.TimeStampNow(),
-		WorkflowID: workflowID,
-	}
-
-	// fourth completed run
-	time2, err := time.Parse(time.RFC3339, "2024-08-31T15:47:55+08:00")
-	require.NoError(t, err)
-	timeutil.MockSet(time2)
-	run2 := ActionRun{
-		ID:         2,
-		Index:      2,
-		RepoID:     repoID,
-		Stopped:    timeutil.TimeStampNow(),
-		WorkflowID: workflowID,
-	}
-
-	// second completed run
-	time3, err := time.Parse(time.RFC3339, "2024-07-31T15:47:54+08:00")
-	require.NoError(t, err)
-	timeutil.MockSet(time3)
-	run3 := ActionRun{
-		ID:         3,
-		Index:      3,
-		RepoID:     repoID,
-		Stopped:    timeutil.TimeStampNow(),
-		WorkflowID: workflowID,
-	}
-
-	// first completed run
-	time4, err := time.Parse(time.RFC3339, "2024-06-30T15:47:54+08:00")
-	require.NoError(t, err)
-	timeutil.MockSet(time4)
-	run4 := ActionRun{
-		ID:         4,
-		Index:      4,
-		RepoID:     repoID,
-		Stopped:    timeutil.TimeStampNow(),
-		WorkflowID: workflowID,
-	}
-	require.NoError(t, db.Insert(db.DefaultContext, &run1))
-	runBefore, err := GetRunBefore(db.DefaultContext, repoID, run1.Stopped)
-	// there is no run before run1
-	require.Error(t, err)
-	require.Nil(t, runBefore)
-
-	// now there is only run3 before run1
-	require.NoError(t, db.Insert(db.DefaultContext, &run3))
-	runBefore, err = GetRunBefore(db.DefaultContext, repoID, run1.Stopped)
-	require.NoError(t, err)
-	assert.Equal(t, run3.ID, runBefore.ID)
-
-	// there still is only run3 before run1
-	require.NoError(t, db.Insert(db.DefaultContext, &run2))
-	runBefore, err = GetRunBefore(db.DefaultContext, repoID, run1.Stopped)
-	require.NoError(t, err)
-	assert.Equal(t, run3.ID, runBefore.ID)
-
-	// run4 is further away from run1
-	require.NoError(t, db.Insert(db.DefaultContext, &run4))
-	runBefore, err = GetRunBefore(db.DefaultContext, repoID, run1.Stopped)
-	require.NoError(t, err)
-	assert.Equal(t, run3.ID, runBefore.ID)
 }

models/forgejo_migrations/migrate.go

@@ -108,7 +108,7 @@ var migrations = []*Migration{
 	// v33 -> v34
 	NewMigration("Add `notify-email` column to `action_run` table", AddNotifyEmailToActionRun),
 	// v34 -> v35
-	NewMigration("Add index to `stopped` column in `action_run` table", AddIndexToActionRunStopped),
+	NewMigration("Noop because of https://codeberg.org/forgejo/forgejo/issues/8373", NoopAddIndexToActionRunStopped),
 }
 
 // GetCurrentDBVersion returns the current Forgejo database version.

models/forgejo_migrations/v35.go

@@ -4,16 +4,10 @@
 package forgejo_migrations //nolint:revive
 
 import (
-	"forgejo.org/modules/timeutil"
-
 	"xorm.io/xorm"
 )
 
-func AddIndexToActionRunStopped(x *xorm.Engine) error {
+// see https://codeberg.org/forgejo/forgejo/issues/8373
-	type ActionRun struct {
+func NoopAddIndexToActionRunStopped(x *xorm.Engine) error {
-		ID      int64
+	return nil
-		Stopped timeutil.TimeStamp `xorm:"index"`
-	}
-
-	return x.Sync(&ActionRun{})
 }

services/actions/notifier.go

@@ -788,7 +788,7 @@ func (n *actionsNotifier) MigrateRepository(ctx context.Context, doer, u *user_m
 // the ActionRun of the same workflow that finished before priorRun/updatedRun.
 func sendActionRunNowDoneNotificationIfNeeded(ctx context.Context, priorRun, updatedRun *actions_model.ActionRun) error {
 	if !priorRun.Status.IsDone() && updatedRun.Status.IsDone() {
-		lastRun, err := actions_model.GetRunBefore(ctx, updatedRun.RepoID, updatedRun.Stopped)
+		lastRun, err := actions_model.GetRunBefore(ctx, updatedRun)
 		if err != nil && !errors.Is(err, util.ErrNotExist) {
 			return err
 		}

tests/integration/actions_run_now_done_notification_test.go

@@ -49,41 +49,22 @@ func (m *mockNotifier) ActionRunNowDone(ctx context.Context, run *actions_model.
 		assert.Equal(m.t, m.runID, run.ID)
 		assert.Equal(m.t, actions_model.StatusFailure, run.Status)
 		assert.Equal(m.t, actions_model.StatusRunning, priorStatus)
-		assert.Equal(m.t, m.lastRunID, lastRun.ID)
-		assert.Equal(m.t, actions_model.StatusSuccess, lastRun.Status)
 		assert.True(m.t, run.NotifyEmail)
 	case 2:
 		assert.Equal(m.t, m.runID, run.ID)
 		assert.Equal(m.t, actions_model.StatusCancelled, run.Status)
 		assert.Equal(m.t, actions_model.StatusRunning, priorStatus)
-		assert.Equal(m.t, m.lastRunID, lastRun.ID)
-		assert.Equal(m.t, actions_model.StatusFailure, lastRun.Status)
-		assert.True(m.t, run.NotifyEmail)
-	case 3:
-		assert.Equal(m.t, m.runID, run.ID)
-		assert.Equal(m.t, actions_model.StatusSuccess, run.Status)
-		assert.Equal(m.t, actions_model.StatusRunning, priorStatus)
-		assert.Equal(m.t, m.lastRunID, lastRun.ID)
-		assert.Equal(m.t, actions_model.StatusCancelled, lastRun.Status)
-		assert.True(m.t, run.NotifyEmail)
-	case 4:
-		assert.Equal(m.t, m.runID, run.ID)
-		assert.Equal(m.t, actions_model.StatusSuccess, run.Status)
-		assert.Equal(m.t, actions_model.StatusRunning, priorStatus)
-		assert.Equal(m.t, m.lastRunID, lastRun.ID)
-		assert.Equal(m.t, actions_model.StatusSuccess, lastRun.Status)
 		assert.True(m.t, run.NotifyEmail)
 	default:
 		assert.Fail(m.t, "too many notifications")
 	}
-	m.lastRunID = m.runID
 	m.runID++
 	m.testIdx++
 }
 
 // ensure all tests have been run
 func (m *mockNotifier) complete() {
-	assert.Equal(m.t, 5, m.testIdx)
+	assert.Equal(m.t, 3, m.testIdx)
 }
 
 func TestActionNowDoneNotification(t *testing.T) {
@@ -159,24 +140,6 @@ func TestActionNowDoneNotification(t *testing.T) {
 		task = runner.fetchTask(t)
 		require.NoError(t, actions_service.StopTask(db.DefaultContext, task.Id, actions_model.StatusCancelled))
 
-		// we can't differentiate different runs without a delay
-		time.Sleep(time.Millisecond * 2000)
-
-		// 3: successful run after failure
-		_, _, err = workflow.Dispatch(db.DefaultContext, inputGetter, repo, user2)
-		require.NoError(t, err)
-		task = runner.fetchTask(t)
-		runner.succeedAtTask(t, task)
-
-		// we can't differentiate different runs without a delay
-		time.Sleep(time.Millisecond * 2000)
-
-		// 4: successful run after success
-		_, _, err = workflow.Dispatch(db.DefaultContext, inputGetter, repo, user2)
-		require.NoError(t, err)
-		task = runner.fetchTask(t)
-		runner.succeedAtTask(t, task)
-
 		notifier.complete()
 	})
 }