Fix race conditions in the memory alerts store

[damnever]

The main branch will easily fail the newly added test case:

=== RUN   TestAlertsConcurrently
    mem_test.go:565:
                Error Trace:    /prometheus-io/alertmanager/provider/mem/mem_test.go:565
                Error:          Not equal:
                                expected: 0
                                actual  : -171
                Test:           TestAlertsConcurrently

There are multiple race conditions in the provider/mem:

1. Any Put or GC operation that occurs concurrently with this code block will introduce a race condition

   alertmanager/provider/mem/mem.go

   Lines 204 to 228 in c920b60

   	existing := false
   	// Check that there's an alert existing within the store before
   	// trying to merge.
   	if old, err := a.alerts.Get(fp); err == nil {
   	existing = true
   	// Merge alerts if there is an overlap in activity range.
   	if (alert.EndsAt.After(old.StartsAt) && alert.EndsAt.Before(old.EndsAt)) ||
   	(alert.StartsAt.After(old.StartsAt) && alert.StartsAt.Before(old.EndsAt)) {
   	alert = old.Merge(alert)
   	}
   	}
   	if err := a.callback.PreStore(alert, existing); err != nil {
   	level.Error(a.logger).Log("msg", "pre-store callback returned error on set alert", "err", err)
   	continue
   	}
   	if err := a.alerts.Set(alert); err != nil {
   	level.Error(a.logger).Log("msg", "error on set alert", "err", err)
   	continue
   	}
   	a.callback.PostStore(alert, existing)

2. A race condition between Put() and Subscribe() can cause some newly added Alerts to be missed.

@simonpasquier @w0rm @gotjosh please take a look

[emanlodovice]

SetGCCallback and Run method on Alerts store are now unused. Should those be deleted?

[rajagopalanand]

I think Run is called by Inhibitor but SetGCCallback is unused in AM but since it's a public method, deleting it might break other things. Could Inhibitor be also changed to use the new GC and Run can be removed?

[damnever]

@simonpasquier @w0rm @gotjosh please take a look

[beorn7]

@simonpasquier @gotjosh is this on your radar?

[damnever]

@grobinson-grafana would you also mind taking a look at this?

[grobinson-grafana]

A race condition between Put() and Subscribe() can cause some newly added Alerts to be missed.

Apologies for asking lots of questions, but I would like to understand this case. I can see there is a race condition between the Get and Set operations in Put because 1.) two or more goroutines can call Put at the same time and 2.) the calls to Get and Set can be interleaved with a gc operation on the store, causing the alert to be deleted and the added back. I agree this needs to be fixed.

What I would still like to understand is how would the alert be missed? The mutex is acquired in Subscribe and before the alert is sent to the listeners, so it should not happen that a new listener misses an alert? Would it be possible to show how this happens with a test?

[grobinson-grafana]

I also opened a draft PR #3840 that builds on this fix. It removes gc and the callback from store.go.

[damnever]

A race condition between Put() and Subscribe() can cause some newly added Alerts to be missed.

Apologies for asking lots of questions, but I would like to understand this case. I can see there is a race condition between the Get and Set operations in Put because 1.) two or more goroutines can call Put at the same time and 2.) the calls to Get and Set can be interleaved with a gc operation on the store, causing the alert to be deleted and the added back. I agree this needs to be fixed.

What I would still like to understand is how would the alert be missed? The mutex is acquired in Subscribe and before the alert is sent to the listeners, so it should not happen that a new listener misses an alert? Would it be possible to show how this happens with a test?

This all comes together, not as an independent case. The reason is that we can delete alerts solely in store.Alerts without the lock from mem.Alerts.mtx. Imagine if the gc happens right after the Put(before the fix), then the listener might miss some alerts due to the race condition.

[grobinson-grafana]

A race condition between Put() and Subscribe() can cause some newly added Alerts to be missed.

Apologies for asking lots of questions, but I would like to understand this case. I can see there is a race condition between the Get and Set operations in Put because 1.) two or more goroutines can call Put at the same time and 2.) the calls to Get and Set can be interleaved with a gc operation on the store, causing the alert to be deleted and the added back. I agree this needs to be fixed.
What I would still like to understand is how would the alert be missed? The mutex is acquired in Subscribe and before the alert is sent to the listeners, so it should not happen that a new listener misses an alert? Would it be possible to show how this happens with a test?

This all comes together, not as an independent case. The reason is that we can delete alerts solely in store.Alerts without the lock from mem.Alerts.mtx. Imagine if the gc happens right after the Put(before the fix), then the listener might miss some alerts due to the race condition.

The reason is that we can delete alerts solely in store.Alerts without the lock from mem.Alerts.mtx.

I agree!

Imagine if the gc happens right after the Put(before the fix), then the listener might miss some alerts due to the race condition.

In this case the listener will receive two events: an event for the Put and a second event for the GC? It can receive them out of order, but I don't think events can be lost?

The reason I think that is the alerts that are sent to a.listeners in Put comes from the argument alerts and not from the alerts in the mem store:

func (a *Alerts) Put(alerts ...*types.Alert) error {
	for _, alert := range alerts {
		...
		a.mtx.Lock()
		for _, l := range a.listeners {
			select {
			case l.alerts <- alert:
			case <-l.done:
			}
		}
		a.mtx.Unlock()
	}

That means even when there is a gc between Set and range a.listeners, the gc operation cannot stop these alerts from being sent to the listeners. The listeners might receive the events out of order, but I still don't see how an event can be missed?

[damnever]

okay, they cannot be stopped, but this is a race.

[damnever]

@grobinson-grafana I have rebased with the main branch and changed some locks to use defer. Please take another look. If there are no major change requirements, I believe we should merge this first and make further improvements as needed.

[grobinson-grafana]

LGTM. Just one comment!

[grobinson-grafana]

LGTM!

[grobinson-grafana]

@gotjosh I want to remove the callback in a future PR, so I'm not too worried about both returning resolved and passing it to the callback.

[gotjosh]

LGTM

[gotjosh]

Suggested change

	// listener is not closed yet, hence proceed.
	// Listener is not closed yet, hence proceed.

You can address it in the next PR.

[gotjosh]

Thank you very much for your contribution!