feat(dispatch): add start delay

[siavashs]

This change adds a new cmd flag --dispatch.start-delay which corresponds to the --rules.alert.resend-delay flag in Prometheus.
This flag controls the minimum amount of time that Prometheus waits before resending an alert to Alertmanager.

By adding this value to the start time of Alertmanager, we delay the aggregation groups' first flush, until we are confident all alerts are resent by Prometheus instances.

This should help avoid race conditions in inhibitions after a (re)start.

Other improvements:

• remove hasFlushed flag from aggrGroup
• remove mutex locking from aggrGroup

[Spaceman1701]

Would you mind splitting this into two PRs? One that adds the --alerts.resend-delay and one that adds the wait_on_startup config to the route?

I'm asking because I think the --alerts.resend-delay is something we should definitely merge, but I'm a little concerned about wait_on_startup.

From the description in the PR, it seems like these are both aimed at solving the same problem - the inhibitor and the dispatcher race on alertmanager restart because alertmanager has to wait for prometheus to resend alerts. resend-delay seems to address this directly, while wait_on_startup seems more like a hack - there's no guarantee that group_wait is the right duration to wait after a restart. Additionally, group_wait is intended to express user's logic, not handle the protocol between alertmanager and prometheus. I wouldn't want to give users competing concerns around what value to use group_wait.

Is there any other use case you envision for wait_on_startup that I might be missing?

[Spaceman1701]

Somewhat unrelated to this change, but I noticed it when reviewing the new code - I think there's a very minor logic bug here - if an alert's StartsAt is in the past, but not at least ag.opts.GroupWait in the past, I think we should check if the next flush is before or after it would be scheduled purely from the new alert. If it's after, we should reset the timer to that duration. I don't thin we're keeping track of the next flush time outside of the timer, so that'd need to change too 🤔

E.g.

wantedFlush := time.Since(alert.StartsAt.Add(ag.opts.GroupWait))
if wantedFlush < time.Duration(0) {
    wantedFlush = time.Duration(0)
}
actualFlush := ag.durationToNextFlush()
if wantedFlush < actualFlush {
  timer.Reset(wantedFlush)
}

I don't think we should change the behavior in this PR though. Perhaps as a follow up.

[siavashs]

Good catch, we can add it here or as a follow up.

[juliusv]

there's no guarantee that group_wait is the right duration to wait after a restart.

That's what I was thinking as well: some people may even have a group_wait: 1d for low-prio grouped alerts. Then you would never get any alerts if you restarted Alertmanager once a day, right?

[siavashs]

I'm dropping the WaitOnStartup as we never used it internally and based on the comments it can be tricky if user uses a long group_wait value.

[siavashs]

We are now failing this test which is vague and I remember debugging before but not documenting:

alertmanager/test/with_api_v2/acceptance/send_test.go

Lines 422 to 466 in aa879e1

	func TestReload(t *testing.T) {
	t.Parallel()
	// This integration test ensures that the first alert isn't notified twice
	// and repeat_interval applies after the AlertManager process has been
	// reloaded.
	conf := `
	route:
	receiver: "default"
	group_by: []
	group_wait: 1s
	group_interval: 6s
	repeat_interval: 10m
	receivers:
	- name: "default"
	webhook_configs:
	- url: 'http://%s'
	`
	at := NewAcceptanceTest(t, &AcceptanceOpts{
	Tolerance: 150 * time.Millisecond,
	})
	co := at.Collector("webhook")
	wh := NewWebhook(t, co)
	amc := at.AlertmanagerCluster(fmt.Sprintf(conf, wh.Address()), 1)
	amc.Push(At(1), Alert("alertname", "test1"))
	at.Do(At(3), amc.Reload)
	amc.Push(At(4), Alert("alertname", "test2"))
	co.Want(Between(2, 2.5), Alert("alertname", "test1").Active(1))
	// Timers are reset on reload regardless, so we count the 6 second group
	// interval from 3 onwards.
	co.Want(Between(9, 9.5),
	Alert("alertname", "test1").Active(1),
	Alert("alertname", "test2").Active(4),
	)
	at.Run()
	t.Log(co.Check())
	}

[ultrotter]

We are now failing this test which is vague and I remember debugging before but not documenting:

alertmanager/test/with_api_v2/acceptance/send_test.go

Lines 422 to 466 in aa879e1

	func TestReload(t *testing.T) {
	t.Parallel()
	// This integration test ensures that the first alert isn't notified twice
	// and repeat_interval applies after the AlertManager process has been
	// reloaded.
	conf := `
	route:
	receiver: "default"
	group_by: []
	group_wait: 1s
	group_interval: 6s
	repeat_interval: 10m
	receivers:
	- name: "default"
	webhook_configs:
	- url: 'http://%s'
	`
	at := NewAcceptanceTest(t, &AcceptanceOpts{
	Tolerance: 150 * time.Millisecond,
	})
	co := at.Collector("webhook")
	wh := NewWebhook(t, co)
	amc := at.AlertmanagerCluster(fmt.Sprintf(conf, wh.Address()), 1)
	amc.Push(At(1), Alert("alertname", "test1"))
	at.Do(At(3), amc.Reload)
	amc.Push(At(4), Alert("alertname", "test2"))
	co.Want(Between(2, 2.5), Alert("alertname", "test1").Active(1))
	// Timers are reset on reload regardless, so we count the 6 second group
	// interval from 3 onwards.
	co.Want(Between(9, 9.5),
	Alert("alertname", "test1").Active(1),
	Alert("alertname", "test2").Active(4),
	)
	at.Run()
	t.Log(co.Check())
	}

Maybe checking the hasFlushed condition would help this test too? After all that should exactly prevent from notifying twice?

[siavashs]

Maybe checking the hasFlushed condition would help this test too? After all that should exactly prevent from notifying twice?

So the problem is not duplicate notification but earlier than expected notification:

        interval [2,2.5]
        ---
        - &{map[] 0001-01-01T00:00:00.000Z <nil> [] 0001-01-01T00:00:01.000Z <nil> <nil> { map[alertname:test1]}}[-9.223372036854776e+09:]
          [ ✓ ]
        interval [9,9.5]
        ---
        - &{map[] 0001-01-01T00:00:00.000Z <nil> [] 0001-01-01T00:00:01.000Z <nil> <nil> { map[alertname:test1]}}[-9.223372036854776e+09:]
        - &{map[] 0001-01-01T00:00:00.000Z <nil> [] 0001-01-01T00:00:04.000Z <nil> <nil> { map[alertname:test2]}}[-9.223372036854776e+09:]
          [ ✗ ]

        received:
        @ 2.00549375
        - &{map[] 0001-01-01T00:00:00.000Z <nil> [] 2025-11-07T15:47:48.705+01:00 <nil> <nil> { map[alertname:test1]}}[1.002707:]
        @ 4.009307375
        - &{map[] 0001-01-01T00:00:00.000Z <nil> [] 2025-11-07T15:47:48.705+01:00 <nil> <nil> { map[alertname:test1]}}[1.002707:]
        - &{map[] 0001-01-01T00:00:00.000Z <nil> [] 2025-11-07T15:47:51.706+01:00 <nil> <nil> { map[alertname:test2]}}[4.003107:]

[siavashs]

hasFlashed is dropped now since Dispatcher controls that with status checks after creating an AG.
This also means that AGs are now lock-free.

[siavashs]

It seems I broke acceptance tests again, I'll check and fix them.

[TheMeier]

Default in prometheus is 1m so should the default in AM also be 1m?

[siavashs]

I initially set this to 1m but it changes the default behaviour and lots of acceptance tests fail. We can avoid breaking the existing tests by setting it to 0 for all tests.
Adjusting timings is not possible since we add +1m to each test.

But I thought same thing could happen to users unexpectedly if they don't pay attention to the changelog and this new cmd flag.

I'm open to setting the default to 1m to sync it with Prometheus defaults.

[TheMeier]

I see, whatever you choose, you will choose wrong :D

[SuperQ]

I think we should set this to 1m to match the Prometheus defaults. But we can do that in a followup PR.

[ultrotter]

LGTM, with a few minor comments! Thanks!

[ultrotter]

I know we use go.uber.org/atomic elsewhere, but does this buy us anything over sync/atomic Bool? https://pkg.go.dev/sync/atomic#pkg-types Should we use this and start a switch?

[siavashs]

There is currently a lint check which enforces this.
I guess we inherit this from Prometheus.
I need to check if this is still required.
cc @SuperQ

[juliusv]

FYI, there's an open effort in Prometheus to switch to the new stdlib atomic types (those only got added in Go 1.19):

• replace uber atomic with stdlib atomic types prometheus#14866
• refactor: replace uber atomic with stdlib atomic types (Issue #14866) prometheus#17358

[SuperQ]

Yea, I'm not sure. This was introduced in Prometheus in 2020. prometheus/prometheus#7647

I guess we'll need to find out from the rest of the Prometheus devs if we still need this.

[bwplotka]

We can remove the check, std lib is what we want as per prometheus/prometheus#14866

[SuperQ]

Looks like we can migrate to to the stdlib now: prometheus/prometheus#14866

[siavashs]

I guess we wait for this to be fix in Prometheus, and Alertmanager will get it as part of the next sync of common ci/build setup.

[ultrotter]

Yeah, I was going to note that the new stdlib fixes the issue and provides that... I think we can target a migration for post 0.30, since it's more of a cleanup than something that is urgently needed?

[Spaceman1701]

Overall, LGTM now. The state to string map is the only thing I'd really prefer that we changed before merging.

[TheMeier]

Should we add this one to the v0.30.0 project? Look pretty far along to me

[siavashs]

Should we add this one to the v0.30.0 project? Look pretty far along to me

I think it can make it for that release, all related comments are resolved.
I'll wait for a final review by @Spaceman1701 and @ultrotter

[Spaceman1701]

LGTM. Thanks for making those changes!