fix(alertmanager): fix ApplyConfig/Stop data race and shutdown-vs-lazy-create window

[sandy2008]

What this PR does

Fixes the data race behind the flaky TestMultitenantAlertmanager_ServeHTTPWithFallbackConfig (issue #7603) — which is a production bug, not a test artifact.

Alertmanager.ApplyConfig (pkg/alertmanager/alertmanager.go) spawned the per-tenant dispatcher and inhibitor Run goroutines fire-and-forget. The vendored alertmanager v0.32.1 dispatch.Dispatcher performs its WaitGroup's first finished.Add(1) inside the Run goroutine (dispatch.go:190), while Dispatcher.Stop() calls finished.Wait() (dispatch.go:449). A Stop() that executes before the spawned goroutine is scheduled violates the sync.WaitGroup contract ("calls with a positive delta that start when the counter is zero must happen before a Wait") — exactly what -race reports. The same window makes Inhibitor.Stop() a silent no-op (the inhibitor goroutine then leaks forever) when it runs before Run() has installed ih.cancel.

Two commits:

1. Startup barrier in ApplyConfig — replace the bare spawns with upstream cmd/alertmanager's own embedder protocol (its reload path, inhibitor first "so we don't accidentally notify for an alert that will be inhibited"): go inhibitor.Run(); inhibitor.WaitForLoading(); go dispatcher.Run(now); dispatcher.WaitForLoading().
2. Shutdown gate in MultitenantAlertmanager — a shuttingDown flag under the existing alertmanagersMtx so the fallback lazy-create path can no longer register a brand-new per-tenant Alertmanager (which would never be stopped) after stopping() has begun.

Why this is reachable

Every per-tenant stop path can interleave with a recent ApplyConfig: process shutdown (stopping()), tenant deactivation (syncConfigs), and config reload (ApplyConfig stops the previous generation's dispatcher/inhibitor, hitting the same unordered Wait-vs-Add pair if the previous ApplyConfig just returned). The CI flake was the lazy-create (fallback) path racing test shutdown; the same shape occurs in production whenever a tenant Alertmanager is created or reconfigured shortly before it is stopped. The Add-inside-Run dispatcher shape ships with the v0.32.1 upgrade (#7462).

The shutdown-vs-lazy-create hole is independent: stopping() stops only the alertmanagers present in the map when it runs, and serveRequest can be reached without ServeHTTP's service-state check (the gRPC HandleRequest path when sharding is enabled) or after passing that check just before the state transition. A request losing that race would upload a config, register a new Alertmanager after shutdown stopped the others, return 200 from a shutting-down process, and leak the new instance's goroutines.

How the fix resolves it

Barrier: WaitForLoading() returns only after the Run goroutine has performed its initial load — program-ordered after the dispatcher's first finished.Add(1) and after the inhibitor installs ih.cancel. ApplyConfig and Stop are serialized by their callers (alertmanagersMtx), so once ApplyConfig returns, every later Stop() is ordered after startup: the WaitGroup misuse and the ignored-Stop inhibitor leak are both gone. The waits live on ApplyConfig's success path only — deliberately not in Stop() (see below).

Gate: stopping() sets shuttingDown under the already-held lock before stopping tenants; the authoritative check sits in setConfig immediately after it takes the same lock and returns an errAlertmanagerShuttingDown sentinel (this also prevents a post-stop reload from restarting goroutines on an already-stopped Alertmanager); alertmanagerFromFallbackConfig makes a cheap best-effort check before uploading the empty config to the alert store; serveRequest maps the sentinel via errors.Is to HTTP 503 — consistent with ServeHTTP's existing 503 when the service is not Running, so a request losing the race gets the same answer it would have gotten a moment later.

Tests

Both regression tests were verified to fail without their fix and pass with it (under -race):

• TestAlertmanagerStopBeforeDispatcherStart (commit 1, alertmanager_test.go): a sequential, sleep-free 30× loop of {New → ApplyConfig → StopAndWait}, with one double-ApplyConfig iteration to cover the reload path. Without the fix it fails on the first -count=1 run with the exact CI signature (race at vendored dispatch.go:190 vs :449); with the fix it passes -count=30. It is sequential by design: callers are serialized in production and the racing actor is the spawned Run goroutine itself, so a concurrent ApplyConfig∥Stop test would exercise an unsupported contract.
• TestMultitenantAlertmanager_NoFallbackCreateAfterShutdown (commit 2, multitenant_test.go): drives a real service shutdown, then exercises the fallback lazy-create path directly. Without the gate it deterministically registers (and leaks) a new tenant Alertmanager and returns 200; with the gate it returns 503 and asserts am.alertmanagers stays empty.
• The originally flaky TestMultitenantAlertmanager_ServeHTTPWithFallbackConfig: green at -race -count=30, both plain and with GOMAXPROCS=2 -cpu 2 (mimicking CI).
• One existing test adapted: TestDispatcherGroupLimits asserts an exact aggregation-group-limit counter value, but the vendored v0.32.1 dispatcher's limit check reads the group counter without synchronization with concurrent group creation, so alerts routed through its concurrent ingestion workers produce a nondeterministic count. This test is already flaky on unmodified master (measured 7/20 failed runs at the base commit on a 12-core host; CI's lower core count shrinks the window), and the barrier increases the exposure (14/20) because the initial slurp is now provably empty under the old alert/config ordering. The test now puts its alerts before ApplyConfig, so the dispatcher routes all of them sequentially from its initial slurp — which the barrier guarantees completes before ApplyConfig returns — making the asserted count exact on every run (30/30 green, poll satisfied immediately).
• Full pkg/alertmanager suite under -race: green.

Why not other approaches

• Per-AM lifecycle mutex (+ stopped flag): empirically falsified — a faithful implementation of that design still races on the first run. alertmanagersMtx already serialized both synchronous bodies in the CI failure; no lock held by the callers can order the unstarted Run goroutine's first WaitGroup.Add. Only a happens-before edge to a post-Add event in the Run goroutine fixes it, which is what WaitForLoading() provides.
• Waiting in Stop() instead of ApplyConfig: a failed reload (e.g. buildIntegrationsMap error after the inhibitor was already swapped) leaves a never-Run inhibitor whose loading counter is never Doned — a Stop-side WaitForLoading() would deadlock stopping() while holding alertmanagersMtx. The ApplyConfig-side barrier is immune by construction: it only waits for goroutines it just spawned.
• Editing the vendored alertmanager (moving Add before the goroutine starts): vendored code must not be modified; the protocol used here is exactly how upstream's own cmd/alertmanager consumes these APIs.
• Bumping alertmanager to v0.32.2: diff inspected — no change to Dispatcher.Run/Stop, would not help.

Scope

Production changes are small and flag-free: the barrier in pkg/alertmanager/alertmanager.go and the ~15-line gate in pkg/alertmanager/multitenant.go (existing mutex, no new locks). Behavior changes: ApplyConfig now returns after the dispatcher/inhibitor finish their initial in-memory load (the same work upstream performs inline on reload), and a fallback request that loses the race with shutdown now gets a 503 instead of silently creating an Alertmanager that leaks. Test changes: two regression tests added, plus the TestDispatcherGroupLimits determinism adaptation described above (test-only; the underlying unsynchronized limit accounting is in the vendored dispatcher and is upstream's to fix).

Known-separate and deliberately not addressed here: the unsynchronized am.dispatcher read in the API GroupFunc closure (pkg/alertmanager/alertmanager.go:291-293) can race a concurrent config reload from API-handler goroutines. That is a distinct race (a pointer-field read, not the WaitGroup internals fixed here) with its own fix shape (upstream publishes the dispatcher through an atomic pointer after WaitForLoading), and is left for a follow-up issue.

Which issue(s) this PR fixes

Fixes #7603

Checklist

• CHANGELOG.md updated — single [BUGFIX] Alertmanager entry (the entry whose name doesn't end in master).
• Documentation updated — N/A; no flags or config changed (make doc not required).
• Tests: two regression tests added, each verified to fail without its fix.
• Commits signed off (DCO).

Test plan

• gofmt -l / goimports -local github.com/cortexproject/cortex -l on changed files — clean
• go vet -tags "netgo slicelabels" ./pkg/alertmanager/... — clean
• go test -tags "netgo slicelabels" -race -count=1 -run 'TestAlertmanagerStopBeforeDispatcherStart$' ./pkg/alertmanager/ — FAILS before commit 1 (data race, dispatch.go:449 Stop vs dispatch.go:190 Run), PASSES after; -count=30 PASS
• go test -tags "netgo slicelabels" -race -count=1 -run 'TestMultitenantAlertmanager_NoFallbackCreateAfterShutdown$' ./pkg/alertmanager/ — FAILS before commit 2 (200 + leaked tenant Alertmanager), PASSES after
• GOMAXPROCS=2 go test -tags "netgo slicelabels" -race -cpu 2 -count=30 -run 'TestMultitenantAlertmanager_ServeHTTPWithFallbackConfig$' ./pkg/alertmanager/ — PASS (and plain -count=30 — PASS)
• go test -tags "netgo slicelabels" -race -count=30 -run 'TestDispatcherGroupLimits$' ./pkg/alertmanager/ — PASS after the determinism adaptation (baseline on unmodified master: 7/20 FAIL)
• go test -tags "netgo slicelabels" -race ./pkg/alertmanager/ — PASS (full package)

This PR was developed with AI assistance (multi-agent design review + implementation); all changes, races, and test results were verified as described in the test plan.

🤖 Generated with Claude Code