Resolve the new-incident insert conflict instead of dropping the payload
CI / chart (push) Successful in 1s
CI / security (push) Successful in 20s
CI / test (push) Has been cancelled

openIncident's INSERT had no ON CONFLICT clause, relying entirely on
incidentForGroup's earlier SELECT to avoid a duplicate. On more than
one replica, two webhook deliveries for the very first occurrence of
a brand-new groupKey can both pass that SELECT before either INSERTs;
the loser then hit incidents_open_group_key_idx's unique violation,
which rolled back its whole transaction — including that payload's
alert upserts, done earlier in the same transaction. ingest's error is
only logged and receiveWebhook answers 200 regardless, so nothing
retried it: the loser's alerts silently never existed.

Add ON CONFLICT (team_id, group_key) WHERE resolved_at IS NULL DO
NOTHING to the INSERT, matching the partial unique index. Postgres
only resolves that conflict after the winning transaction commits (or
rolls back), so by the time RETURNING comes back empty,
existingOpenIncident's follow-up SELECT is guaranteed to see the
winner's row. The loser attaches to it instead of failing outright,
and the rest of its payload commits normally. Covers both callers,
since the dead man's switch sweeper shares this same function.

New test (package api_test, fires N webhook deliveries for one
groupKey from a synchronized start with distinct fingerprints, so
they aren't accidentally serialized by upsertAlerts' own per-
fingerprint lock) confirmed meaningful: with the ON CONFLICT clause
reverted, it fails 10/10 on a missing alert fingerprint; restored, 0/10.
Note while building it: "exactly one incident" alone cannot
distinguish fixed from broken, since the DB's own unique index already
guarantees that either way — the real signal is the loser's payload
surviving.

Chart comment updated: all three of the chart's original reasons for
Recreate are now addressed in code, though replicas stays at 1 and the
strategy stays Recreate pending a deliberate decision to raise it.

Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
Niklas Ye
2026-10-03 11:58:39 +02:00
parent 0050738ca0
commit 9e5b085d8b
3 changed files with 155 additions and 6 deletions
@@ -10,11 +10,14 @@ spec:
selector:
matchLabels:
{{- include "terdut-server.selectorLabels" . | nindent 6 }}
# Recreate, not RollingUpdate, even though the PVC that forced it is gone: the
# sweeper, notifier and migration runner now take a Postgres advisory lock
# each, so two replicas overlapping during a rollout no longer both page for
# the same incident or race applying a migration, but new-incident creation
# on the first webhook for a brand-new groupKey is still unguarded.
# Recreate, not RollingUpdate, even though the PVC that forced it is gone:
# the sweeper, notifier and migration runner take a Postgres advisory lock
# each, and new-incident creation on the first webhook for a brand-new
# groupKey resolves its own insert conflict — so two replicas overlapping
# during a rollout no longer double-page, race a migration, or drop a
# webhook payload. Nothing left here actually requires Recreate anymore;
# it stays the default pending a deliberate decision to raise replicas
# above 1 and move to RollingUpdate.
strategy:
type: Recreate
template: