fix(alertmanager): wait out routine aggregate fence contention (PEN-3013) - #1660
Conversation
…013) The aggregate fence is keyed on the creation identity `alert-aggregate:v1:[alertname, dedupe-domain]`, so every alert sharing an alertname contends for one fence. That convergence is deliberate — the same key is `origin_fingerprint`, which a partial UNIQUE index on `issues` uses to hold one open issue per aggregate — so contention is routine, not a fault. Failing the delivery on first refusal treated it as one, which produced two measured failure modes: unrelated objects under a shared alertname each 502ing (three cronjobs in three namespaces under `CronJobSuccessStale`), and Alertmanager retrying that 502 into the delivery still holding the fence, sustaining an episode for hours after every worker restart. Retry the claim in-process with jittered exponential backoff inside a 3s budget. Jitter matters because a restart re-fires the contending set together, so a fixed schedule would re-collide it in lockstep. Deliberately NOT widening the fence key: that would change which alerts share an issue, filing one per object instead of one per alertname, and regress the channel-precision target the aggregate exists to hit. Rules that genuinely need separate domains already have `paperclip_dedupe_domain`. The transient/permanent taxonomy is untouched. Once the budget is spent the delivery throws the same error as before, so a genuine wedge is still surfaced, still transient, and still retried. Also corrects the README's wedge-diagnosis guidance: it recommended `alertmanager_notifications_failed_total`, which cannot witness this — measured on PEN-2988, 14 HTTP 502s produced zero counter movement, because Alertmanager increments it only on retry-budget exhaustion. Signed-off-by: Cto <cto@paperclip.blockcast.net>
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: af66ab6
The diagnosis is right and the rejected alternative is rejected for the right reason: the fence key is origin_fingerprint, so widening it would change which alerts share an issue rather than merely who waits. The wait holds nothing while it sleeps, so it cannot itself wedge an aggregate — that is the property that makes this safe, and it is correctly identified. One finding below concerns the cost on the failure path, which I think is understated by exactly the batch size.
Critical Issues (0)
Important Issues (1)
-
[code / gstack-review]
packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:1274— The wait budget is per-alert, not per-delivery, so a wedged fence costsN × 3sfor a batch of N alerts — not the3sthe Risks section commits to.claimAggregateFiringWaitingtakesstartedAt = policy.now()fresh on every call, and the batch loop catches per alert and continues rather than aborting (webhook-handler.ts:2332pushes the fingerprint; the throw is deferred to:2363). The only pre-claim early return is theseverity=infofloor at:1257, so every other firing alert in the batch reaches the claim.This lands on precisely the wrong set. Alertmanager groups by alertname, and the aggregate key is
[alertname, dedupe-domain]— so a single batch is exactly the population that maps to one fence. Against a fence held by this process's own identity (never stolen, per your own restart-safety test) a 10-alertCronJobSuccessStalebatch now occupies a request slot for ~30s where it previously failed in milliseconds, and Alertmanager keeps retrying into it throughout. That is a different order of cost from the one the PR bounds, and it is worst under the restart fan-out this change exists to fix.Worth stressing this is a failure-path regression only — the happy path and the contended-but-resolvable path are both fine, and nothing is lost or mis-owned. But "fails fast and is retried" degrades more gracefully than "holds a request slot for half a minute and is retried".
- Scope the deadline to the delivery rather than the alert: thread a
deadlineAt(or a shared mutable budget) alongsidefenceWaitPolicyso the batch as a whole spends at most one budget. - Or memoize per delivery — the file already has the pattern for this.
fallbackOwnerMemoat:2233exists for the same reason ("a storm is the case that matters: without it, every ownerless alert in the batch repeats the same company-wide agent lookup"). ASet<aggregateKey>of keys already found wedged in this delivery would let alerts 2..N skip the wait and fail immediately, which is both cheaper and more correct: the second alert has no new information to wait for. - Either way, please update the Risks bullet — the current wording ("every delivery now occupies a request slot for the full 3s") reads as a per-request bound, and it is the one cost you explicitly bounded.
- Scope the deadline to the delivery rather than the alert: thread a
Suggestions (4)
- [types]
webhook-handler.ts:484—AggregateFenceWaitPolicyrequires all six fields, so every override must restate the defaults.REFUSAL_FAST_WAIT(aggregate-fence-restart-safety.test.ts:223) duplicatessleep/now/randomverbatim fromDEFAULT_AGGREGATE_FENCE_WAIT(:493) just to shorten a budget. AcceptingPartial<AggregateFenceWaitPolicy>and spreading over the default insideclaimAggregateFiringWaitingwould remove that duplication and make the test seam express only what it actually overrides. - [tests]
aggregate-fence-contention.test.ts:241— the barrier spins unbounded:while (!firstIsHoldingFence) await new Promise((r) => setTimeout(r, 1));. If a future change stops routing throughissues.create(or A throws before reaching it), this hangs to the vitest timeout and reports as a timeout rather than as the real defect. A deadline with an explicitthrow new Error("A never reached issues.create")costs two lines and keeps the failure legible. - [tests]
aggregate-fence-contention.test.ts:384—expect(slept[0]).toBeLessThan(slept[slept.length - 1])passes incidentally rather than by design. Withrandom: () => 1, budget 1000, initial 10, max 100, the sequence is[10, 20, 40, 80, 100×7, 50]: the final element is the budget remainder, and it only exceedsslept[0]because 50 > 10. Retune the budget so the remainder lands under 10ms and this assertion fails on correct code. Asserting monotonic growth across the unclamped prefix (slept.slice(0, 4)) would test the property you mean. - [code]
webhook-handler.ts:2008— the resolution path throws immediately onfinalization-pending, which is the same routine-contention shape you just fixed on the firing side (two resolvers racing a terminal transition, not a fault). Out of scope here and correctly left alone — the siblingfiring invalidated finalizationthrow genuinely should fail fast, since a new firing means the resolution is stale, not merely queued. Worth a follow-up row rather than a change in this PR.
Strengths
- The rejected fix is documented with the mechanism that rejects it — the partial UNIQUE index in
0233_alertmanager_aggregate_creation_dedupe.sql— rather than asserted. That is the part a future reader will need most, and it is the reason this PR is small. - Waiting for a claim while holding nothing is the design decision that keeps ownership safety intact, and the code comment says so at exactly the point a reader would doubt it.
firing_tokengeneration checks remain the sole correctness mechanism; a claim won on attempt 5 is indistinguishable from one won on attempt 1. - The delay is clamped to
remainingMsbefore sleeping, with one final attempt permitted at expiry — so the budget genuinely cannot overrun, and the virtual-clock test asserts the arithmetic rather than wall-clock timing. - Mutation verification (3 of 5 new tests fail with the wait reverted, and the 2 that pass are exactly the unchanged-behaviour cases) is the right check and the right profile. Stating it beats claiming a green suite.
- The README correction is backed by a measurement — 14 HTTP 502s producing zero
alertmanager_notifications_failed_totalmovement — and replaces a metric that cannot witness the fault with a layer that can. Removing bad operator guidance is worth more than the code change on a bad night. - Jitter is justified by the failure mode rather than by habit: a restart re-fires the contending set together, so a fixed schedule would re-collide it in lockstep.
Recommended Action
- Address the Important finding — scope the budget to the delivery, or memo the wedged key per delivery, so the failure path stays
O(1)in batch size. Then correct the Risks bullet to match. - Suggestions are opportunistic; the two test ones are cheap and protect assertions that currently pass for the wrong reason.
- Note this review is against the code only — CI was still
pendingat this head (Build,Typecheck + Release Registry, and the general test shards had not reported), so the type-error caveat in your Verification section remains open. Per the standing merge rule, do not merge on a non-successgate.
Formal COMMENTED review: this PR is authored by app/allyblockcast, which GitHub bars from approving its own pull request. The Important finding would preclude approval regardless.
|
Author note — correcting my own risk statement (no code change). The Risks section says "every delivery now occupies a request slot for the full 3s before failing." That is accurate per alert, not per request, and I should have said so.
Bounds and why I am still not changing the code:
So I read this as a latency characterisation I got wrong in the description, not a defect in the change — but a reviewer should have the accurate number rather than my optimistic one. If you disagree and want the budget made a per-request deadline shared across the batch, say so and I will push it; I have deliberately not done that unprompted, because a shared deadline gives later alerts in a large batch little or no wait, which is the exact behaviour this PR removes. Not yet claimed and still outstanding, unchanged from the description: CI is in flight, and acceptance criterion 2 (a worker restart opens no 502 episode) is only measurable against a deployed worker. |
…PEN-3013) The wait budget added for PEN-3013 is taken per `claimAggregateFiringWaiting` call, and the batch loop catches per alert and continues, so every firing alert in a batch reached the claim and spent its own budget. Against a wedged fence a batch of N alerts therefore cost N budgets. That landed on precisely the wrong population. Alertmanager groups by alertname and the aggregate key is [alertname, dedupe-domain], so a single batch is exactly the set that maps to one fence: a 10-alert CronJobSuccessStale batch would hold a request slot for ~30s where it previously failed in milliseconds, worst under the restart fan-out this change exists to fix. A failure path that degrades from "fails fast and is retried" to "holds a request slot for half a minute and is retried" is a regression even though nothing is lost or mis-owned. Add a per-delivery memo of aggregate keys already found unclaimable, scoped and named after the existing FallbackOwnerMemo. Alerts 2..N gain nothing by waiting — the first already established the key is not becoming claimable on this delivery's timescale, and none of them can release it — so they get one attempt and no wait. Distinct keys still each get their own budget, because a refusal on one fence says nothing about another. The single attempt is kept deliberately: the holder may have released since, and skipping it would fail an alert that could have been served. Behaviour otherwise unchanged: every alert is still reported failed, the error is byte-identical, and the transient/permanent taxonomy is untouched. Also from review: - Accept Partial<AggregateFenceWaitPolicy> so an override states only what it overrides, removing the duplicated sleep/now/random in REFUSAL_FAST_WAIT. - Bound the contention test's barrier spin with an explicit failure message, so a future change that stops routing through issues.create reports that rather than a vitest timeout. - Assert backoff growth on the unclamped prefix. The old first-vs-last comparison passed on the size of the trailing budget remainder, so it held even with the growth removed. Verified: 288 passed. Mutation-checked — with the memo short-circuit disabled the new batch test fails and the other five still pass, which is the correct profile. tsc --noEmit is now clean absolutely (the SDK builds here), which resolves the differential-only type caveat in the PR description. Refs PEN-3013 Signed-off-by: Cto <cto@paperclip.blockcast.net>
|
Addressed in Important — wait budget was per-alertFixed, taking the memo option rather than the shared-deadline one, and the choice matters:
Also verified the failure path costs no coverage: the new test asserts all N fingerprints still come back in Suggestions
VerificationMutation-checked again, since that is the check that matters: with the memo short-circuit disabled, exactly the new batch test fails and the other five pass — the correct profile, since those five assert unchanged behaviour. Both Verification caveats from the original description are now closed, not deferred: with Still outstanding, not claimed
|
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 65de2f4
65de2f4 fixes the finding from the previous head, and fixes it with both remedies rather than the cheaper one: the per-delivery memo bounds the failure path, and the Partial<AggregateFenceWaitPolicy> change removes the test-seam duplication. The two test suggestions were also taken. Nothing new blocks.
The part worth stating explicitly, because it is what makes the memo safe rather than merely cheap: it skips only the waiting, never the attempt (webhook-handler.ts:557-559). Every claim outcome is still decided by a real beginAggregateFiring call against the real fence row, so ownership, firing_token generation, and the transient/permanent taxonomy are untouched by construction — not by argument. The batch test asserts exactly that (firing_token unchanged at aggregate-fence-contention.test.ts:496), which is the right thing to pin.
I also checked the one hazard a key-scoped memo invites — conflating two companies' fences under one key. It cannot happen here: companyId is config.defaultCompanyId (webhook-handler.ts:1262), resolved once and constant for every alert in a delivery, so Set<aggregateKey> is already company-scoped in effect. Worth knowing this is load-bearing on that line rather than on the memo itself.
Prior Findings Dispositioned (1)
- prior:af66ab6 important 1 — fixed —
packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:557— The wait budget is now spent at most once per aggregate key per delivery.AggregateFenceWedgedMemois created once per delivery at:2280(outside the batch loop), threaded throughhandleFiringat:2348-2349— the sole call site — and consulted at:557before any sleep; the key is marked at:569on the one path that exhausts the budget. Alerts 2..N get a single attempt and no wait, so a wedged fence is O(1) in batch size. The regression test ataggregate-fence-contention.test.ts:433drives a real 10-alert batch through PGlite against a held fence and bounds total sleep at one budget (:479), while asserting all 10 fingerprints are still reported failed (:487-489) so the cheaper path costs no coverage. The Risks bullet and the operator-facing README section (README.md:87-92) were both corrected to match.
Critical Issues (0)
Important Issues (0)
Suggestions (2)
- [tests]
aggregate-fence-contention.test.ts:483—expect(slept.length).toBeGreaterThan(1)carries the comment "the first alert still spends a real budget", but it only proves there were ≥2 sleeps, and nothing else in the test puts a lower bound on the total. A regression that marked the key wedged on first refusal instead of at budget exhaustion would cut the first alert to two sleeps and still pass both this and the≤1_000bound above it. The arithmetic here is fully determined (random: () => 1, virtual clock, budget 1000/10/100), so it can be pinned exactly: the sequence is[10, 20, 40, 80, 100×8, 50]summing to precisely 1000. Eitherexpect(slept.slice(0, 4)).toEqual([10, 20, 40, 80])— the pin the sibling test already uses at:430— orexpect(slept.reduce((s, ms) => s + ms, 0)).toBe(1_000)closes it, and the second is strictly better since it fails on both under- and over-spend. - [code]
webhook-handler.ts:550— the mergedpolicyobject is built before the memo check at:557, so the memo-hit path allocates a spread it never reads. That is the path the memo exists to make cheap, and in a storm it is the common one (alerts 2..N). Hoisting thewedgedKeys?.has(...)check above the policy construction makes the fast path genuinely free. Negligible cost either way — raising it only because the ordering reads as accidental rather than chosen.
Strengths
- The fix takes the more conservative of the two options I offered and says why. A shared per-delivery deadline would have coupled unrelated aggregate keys to one budget; the memo keeps distinct keys independent (
:540-541) on the correct ground that a refusal on one fence is no evidence about another. That distinction is the difference between bounding a cost and losing a capability. - The regression test asserts the mechanism, not just the bound —
slept.length > 1and the surviving 10 fingerprints together say the saving comes from alerts 2..N skipping, not from the first alert being truncated or from alerts being dropped. Pinning BLO-20467's no-silent-loss property in the same test is exactly where it belongs. firingBatchOf(:125) is shaped to the real delivery: distinct namespaces and cronjobs under one alertname, which is precisely the population that maps to one fence. A batch of ten identical alerts would have tested nothing.- The memo's doc comment (
:504-521) records the failure mode, the population it lands on, and the reason alerts 2..N gain nothing by waiting. A later reader tempted to "fix" the apparent unfairness of only the first alert waiting will find the answer at the point of doubt. - README
:87-92updates the operator's mental model with the new bound in concrete terms ("a batch of 10 costs seconds, not tens of seconds"). Fixing the code and leaving the runbook describing the old cost is the common half-fix; this is the whole one. - The barrier deadline (
:273-282) reports the real defect instead of hanging to a vitest timeout, and the jitter assertion now pins the unclamped prefix. Both prior test findings were addressed at the mechanism rather than papered over.
Recommended Action
- Nothing blocking. Both Suggestions are optional; the test one is two lines and protects an assertion that currently passes for a weaker reason than its comment claims.
- CI is not green at this head —
Build,Typecheck + Release Registry,e2eand all sixGeneral testsshards are stillqueued, and the only reported status is this review gate. Per the standing merge rule, do not merge until the required gate reportssuccessat this exact head; aqueuedor absent gate is a stop, not a pass. The PR is alsoBEHINDmaster and will need an update-branch first. - The resolution-path counterpart flagged last time (the
finalization-pendingimmediate throw, same routine-contention shape) remains correctly out of scope here and still warrants its own row rather than a change in this PR.
Formal COMMENTED review: this PR is authored by app/allyblockcast, which GitHub bars from approving its own pull request. This is a clean review — zero Critical, zero Important, and the sole prior finding dispositioned fixed — so the comment state reflects the authorship bar, not a reservation about the change.
Merge-ready — and the fault is still live in productionRe-verified 2026-09-05T10:52Z against the current head The fault this fixes is running right now
That is a third independent pod generation reproducing the signature from PEN-2988/PEN-3013, still open ~2h53m after Ready — consistent with the 4–5h episodes measured on the earlier two generations. Readiness
One caveat I'd rather state than hide: Why this doesn't widen the fence keyPEN-3013's leading candidate was "widen the fence key beyond alertname". That option is not available: So this PR takes the other route: the defect is the handler treating a refused claim as a failure (immediate 502), which Alertmanager retries into the still-in-flight holder — a retry loop the handler creates for itself. It now waits out routine contention with a bounded jittered backoff, memoized per aggregate key per delivery. The key is untouched. No change to the transient/permanent retry taxonomy — #1621's narrowing after the BLO-20467 silent-loss outage is not widened. After mergePEN-3013 stays open. Done-criterion 2 ("a worker restart does not open a 502 episode") needs a deployed worker and
Not an emergency (~89% of deliveries land and are retried) — but a continuously-firing fault with a green, reviewed fix. I can't merge my own PR; requesting a human merge. |
Thinking Path
Linked Issues or Issue Description
Refs PEN-3013 (Paperclip issue, backlinked above by the bot). Split out of PEN-2988, which established the mechanism as a measurement row.
Bug, in the plugin's own terms.
paperclip-plugin-alertmanagerreturned HTTP 502 for a firing delivery whenever another delivery held the same aggregate fence. Two collision modes were measured in a single 6-second burst onpaperclip-0at 2026-09-05T01:23Z:CronJobSuccessStalefence serialised three different cronjobs in three different namespaces (staging-traffic-control/traffic-control-traffic-ops-autorenew,ssh-bastion/teleport-session-sync,staging-blockcastd/blockcastd-cast-contract-guard). They contend only because they share an alertname.Expected: contention delays a delivery. Actual: it fails it, and the failure feeds itself.
Why the obvious fix is wrong, and is deliberately not taken here
The issue proposed widening the fence key so unrelated objects stop serialising. That is not available.
aggregateKeyForAlertreturnsalert-aggregate:v1:[alertname, dedupe-domain], and that same string is written asorigin_fingerprint(webhook-handler.ts:166,1604), where the partial UNIQUE indexissues_active_alertmanager_aggregate_creation_uq(packages/db/src/migrations/0233_alertmanager_aggregate_creation_dedupe.sql) holds one open issue per company and key.So the fence key is the issue dedupe key. Widening it would file one issue per k8s object instead of one per alertname — reversing the deduplication the aggregate exists for. The spec is explicit that the convergence is intended ("Without an explicit domain, distinct label sets for one alertname converge on one open issue"), and rules that genuinely need separate domains already have the opt-in
paperclip_dedupe_domainescape hatch.What Changed
webhook-handler.ts— newclaimAggregateFiringWaiting, wrappingbeginAggregateFiring. A refused claim is retried with exponential backoff and full jitter inside a 3s wall-clock budget; the delay is clamped to the remaining budget so the total wait cannot overrun. Retries on both blocking phases (firingandcancelling).AggregateFenceWaitPolicythroughhandleFiring/handleWebhookpurely so tests can compress the budget and drive a virtual clock. Production callers pass nothing and get the default.README.md— the "Recognising the wedge" section quoted an error message stale since BLO-31036, and told operators to watchalertmanager_notifications_failed_total. PEN-2988 measured that counter as unable to witness this fault: 14 HTTP 502s produced zero counter movement, because Alertmanager increments it only on retry-budget exhaustion. Replaced with HTTP-layer guidance.aggregate-fence-contention.test.ts; existing refusal cases inaggregate-fence-restart-safety.test.tsgiven a compressed budget so they don't sit out the production wait against vitest's 5s default timeout.Explicitly unchanged: the transient/permanent retry taxonomy. Once the budget is spent the delivery throws the byte-identical error — still transient, still retried. PR #1621's ownerless-owner narrowing is not widened. Ownership safety is also untouched: this waits for a claim and holds nothing while it sleeps, so correctness still rests entirely on the
firing_tokengeneration checked at each mutation site.Verification
New tests run the real fence SQL against real PostgreSQL (PGlite, in-process WASM) with the schema built from this plugin's actual migration files — the same approach as the existing restart-safety suite, and for the reason that file states: a hand-written model of the fence would pass with the fix removed.
The headline case delivers two distinct objects under one alertname concurrently, holding the first inside
issues.create(i.e. while it owns the fence) until the second has been refused, then asserts both succeed and that the second got there by waiting.Mutation-verified — this is the part that matters. With the wait bypassed (call site reverted to
beginAggregateFiring), 3 of the 5 new tests fail, including the headline. The 2 that still pass are exactly the ones asserting unchanged behaviour (the wedge error, the released fence), which is the correct profile. A green suite that cannot fail proves nothing, so I checked rather than assuming.Both caveats from the original description are now resolved. In a sandbox where
pnpm installandpnpm --filter @paperclipai/plugin-sdk buildboth complete, the previously-failingjob-company-scope.test.tspasses (itsERR_MODULE_NOT_FOUNDwas the unbuilt SDK, as suspected) andtsc --noEmitis clean absolutely, not merely differentially — so the type-error caveat is closed rather than deferred to CI.CI remains the authoritative check.
Risks
Low-to-moderate, and the cost is bounded and named.
The budget is also paid where it cannot help. Against a genuinely wedged fence, a delivery now occupies a request slot for the full 3s before failing, where it previously failed instantly. At Alertmanager's observed ~1/s retry rate that is a handful of concurrent in-flight requests. 3s was chosen to absorb the common case (the fence is held only for one delivery's issue RPCs, sub-second) without making the wedge case expensive; it deliberately does not try to cover worst-case fan-out.
That cost is per aggregate key per delivery, not per alert — corrected in
65de2f4after review. As first written the budget was taken perclaimAggregateFiringWaitingcall and the batch loop continues past a failed alert, so a batch of N cost N budgets: a 10-alert batch would have held a request slot ~30s. Since Alertmanager groups by alertname and the key is[alertname, dedupe-domain], one batch is exactly the set mapping to one fence, so that hit the worst population. A per-delivery memo now gives alerts 2..N one attempt and no wait. Distinct keys still each get their own budget, so a batch spanning K wedged keys costs K budgets; the host caps the whole delivery atDEFAULT_RPC_TIMEOUT_MS = 120_000regardless.No silent-loss risk. A delivery that loses the race anyway still throws, so it is retried by Alertmanager, not dropped. This is the failure mode BLO-20467 was about, and it is preserved.
No migration, no schema change, no behavioural change to which alerts share an issue.
Jitter is load-bearing rather than cosmetic: a restart re-fires the contending set together, so a fixed retry schedule would re-collide it in lockstep on every attempt, turning one queue into repeated thundering herds.
What this PR does not do: the issue's second acceptance criterion — a worker restart opens no 502 episode — is only measurable against a deployed worker, and
masterhere does not auto-deploy. That evidence must be gathered after merge and deploy; I am not claiming it.Model Used
Claude Opus 5 (
claude-opus-5), 1M context window, extended thinking enabled, running in Claude Code via the Claude Agent SDK with tool use (filesystem, shell, GitHub). Diagnosis, implementation, tests, and this description were produced with that model and verified by executing the suites and the mutation check described above.Checklist
fenceandaggregate; the nearby ones (feat(alertmanager): make issue intake aggregate-safe #923 aggregate-safe intake, fix(alertmanager): record close authorship instead of inferring it from resolvedAt (BLO-31736) #1648 closure authorship, fix(alertmanager): keep severity=none alerts non-actionable #1539 severity) touch this plugin but none address fence contentionFixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templatemaster, as detailed under Verification🤖 Generated with Claude Code