Alerting: Fix duplicated silences in remote primary mode bug (#91902)

* Alerting: Fix duplicated silences in remote primary mode bug

* test that a new silence id returned by calling CreateSilence() on the internal Alertmanager is ignored

pkg/services/ngalert/remote/forked_alertmanager_test.go

@@ -3,6 +3,7 @@ package remote
 import (
 	"context"
 	"errors"
+	"fmt"
 	"testing"
 	"time"
 
@@ -474,6 +475,32 @@ func TestForkedAlertmanager_ModeRemotePrimary(t *testing.T) {
 		id, err = forked.CreateSilence(ctx, testSilence)
 		require.NoError(tt, err)
 		require.Equal(tt, expID, id)
+
+		// If the silence ID changes, the internal Alertmanager should attempt to expire the old silence.
+		newID := "new"
+		internal, remote, forked = genTestAlertmanagers(tt, modeRemotePrimary)
+		remote.EXPECT().CreateSilence(mock.Anything, mock.Anything).Return(newID, nil).Once()
+		internal.EXPECT().DeleteSilence(mock.Anything, mock.Anything).Return(nil).Once()
+		// If internal.CreateSilence() returns a new id, it should be ignored.
+		internal.EXPECT().CreateSilence(mock.Anything, mock.Anything).Return("random-id", nil).Once()
+		id, err = forked.CreateSilence(ctx, testSilence)
+		require.NoError(tt, err)
+		require.Equal(tt, newID, testSilence.ID)
+		require.Equal(tt, newID, id)
+
+		// Restore original ID.
+		testSilence.ID = expID
+
+		// An error attempting to delete a silence in the internal Alertmanager not be returned.
+		internal, remote, forked = genTestAlertmanagers(tt, modeRemotePrimary)
+		remote.EXPECT().CreateSilence(mock.Anything, mock.Anything).Return(newID, nil).Once()
+		internal.EXPECT().DeleteSilence(mock.Anything, mock.Anything).Return(fmt.Errorf("test error")).Once()
+		// If internal.CreateSilence() returns a new id, it should be ignored.
+		internal.EXPECT().CreateSilence(mock.Anything, mock.Anything).Return("random-id", nil).Once()
+		id, err = forked.CreateSilence(ctx, testSilence)
+		require.NoError(tt, err)
+		require.Equal(tt, newID, testSilence.ID)
+		require.Equal(tt, newID, id)
 	})
 
 	t.Run("DeleteSilence", func(tt *testing.T) {

pkg/services/ngalert/remote/remote_primary_forked_alertmanager.go

@@ -2,6 +2,7 @@ package remote
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	alertingNotify "github.com/grafana/alerting/notify"
@@ -72,16 +73,30 @@ func (fam *RemotePrimaryForkedAlertmanager) GetStatus(ctx context.Context) (apim
 }
 
 func (fam *RemotePrimaryForkedAlertmanager) CreateSilence(ctx context.Context, silence *apimodels.PostableSilence) (string, error) {
-	uid, err := fam.remote.CreateSilence(ctx, silence)
+	originalID := silence.ID
+	id, err := fam.remote.CreateSilence(ctx, silence)
 	if err != nil {
 		return "", err
 	}
 
-	silence.ID = uid
+	if originalID != "" && originalID != id {
+		// ID has changed, expire the old silence before creating a new one.
+		if err := fam.internal.DeleteSilence(ctx, originalID); err != nil {
+			if errors.Is(err, alertingNotify.ErrSilenceNotFound) {
+				// This can happen if the silence was created in the remote AM without using the Grafana UI
+				// in remote primary mode, or if the silence failed to be replicated in the internal AM.
+				fam.log.Warn("Failed to delete silence in the internal Alertmanager", "err", err, "id", originalID)
+			} else {
+				fam.log.Error("Failed to delete silence in the internal Alertmanager", "err", err, "id", originalID)
+			}
+		}
+	}
+
+	silence.ID = id
 	if _, err := fam.internal.CreateSilence(ctx, silence); err != nil {
 		fam.log.Error("Error creating silence in the internal Alertmanager", "err", err, "silence", silence)
 	}
-	return uid, nil
+	return id, nil
 }
 
 func (fam *RemotePrimaryForkedAlertmanager) DeleteSilence(ctx context.Context, id string) error {