diff --git a/packages/plugins/paperclip-plugin-alertmanager/README.md b/packages/plugins/paperclip-plugin-alertmanager/README.md index 7189e2877bd8..6eee391af03b 100644 --- a/packages/plugins/paperclip-plugin-alertmanager/README.md +++ b/packages/plugins/paperclip-plugin-alertmanager/README.md @@ -658,8 +658,47 @@ the alert, runs the cover cascade itself (`recordSourceResolvedAndCloseCovers`). That is idempotent by construction and only cancels a cover whose every member has resolved, so a storm-batched sibling that is still firing keeps the cover open. The "chain exhausted" -comment sits behind the swap, so no announcement is posted for an alert that -has already cleared. +comment sits behind the swap, so a **refused** swap posts no announcement for +an alert that has already cleared. A swap that **succeeds** in that same window +still can: the resolve is mid-delivery and has not stored `resolvedAt` yet, so +the rung reads the alert as firing and posts "while alert remains firing" on +the source issue. The cover is still closed by the resolve's post-commit +cascade below, so the announcement is the only residue. + +That compensation only fires when the swap is **refused**, which left one more +interleaving open (BLO-33497). The webhook's cover cascade ran *before* it +stored `resolvedAt`, so a resolve could cascade while the cover did not yet +exist — nothing to mark — and then store `resolvedAt` only *after* the sweep's +swap had already succeeded. The swap succeeding means no compensation runs, and +no later resolve will ever cascade into that cover again: an open +`[user-cover]` with an unresolved member, for an alert that has cleared, +permanently. + +The obvious repair — move the cascade behind the state write — is wrong, and +the existing tests say so. `ctx.state.set` is the delivery's **commit point**, +and every side effect is deliberately sequenced ahead of it so that a failure +leaves `resolvedAt` unwritten and the retry redoes the lot. Moving the cascade +past it swaps a concurrency orphan for a failure orphan: a cascade that throws +would leave a record asserting the alert is over with its cover uncleaned. + +So `handleResolved` cascades **twice**, and the two calls answer different +failures: + +- **ahead of the commit point** — makes cover cleanup a precondition of + recording the resolution. A throwing cascade aborts the delivery with nothing + recorded. +- **behind the commit point** — catches a cover that did not exist yet when the + first call ran. A swap that *succeeds* means the sweep read, created its cover + and claimed all before the commit, so by the time the second call runs the + cover is there to be closed. + +Between them there is no window: the sweep compensates the refused-swap half, +and the post-commit cascade covers the succeeded-swap half. The second call is +close to free — `recordSourceResolvedAndCloseCovers` early-returns when the +alert never joined a cover (the common case), re-marking is +`COALESCE(resolved_at, now())`, and the close is a single-UPDATE claim only one +caller can win. If it throws, the delivery still fails and the retry's +pre-commit cascade closes the cover, which by then exists. ### Bearer rotation in a Kubernetes deployment diff --git a/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/escalation.test.ts b/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/escalation.test.ts index 66112b3d0a09..c873b229baf9 100644 --- a/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/escalation.test.ts +++ b/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/escalation.test.ts @@ -857,6 +857,111 @@ describe("BLO-20650 concurrent webhook + sweep on one alert-state record", () => expect(coverRow.cancelled_at).not.toBeNull(); }); + /** + * BLO-33497 — the compensation above is reached only when the swap is + * REFUSED, so it does not cover the interleaving where the swap SUCCEEDS. + * The webhook cascaded only *before* storing `resolvedAt`, which allowed: + * the resolve's cascade runs while the cover still does not exist (no + * membership to mark), the sweep then creates the cover and wins its swap + * against a record the webhook has not written yet, and only afterwards does + * the webhook store `resolvedAt`. Nothing compensates, and no later resolve + * can ever cascade into that cover again — it is orphaned open with an + * unresolved member for an alert that has already cleared. + * + * The fix is a SECOND cascade in `handleResolved`, behind the state write. + * Simply moving the existing one is wrong: `ctx.state.set` is the commit + * point, and `worker.test.ts`'s "fails the delivery without marking resolved + * when cover cleanup fails" pins the cascade ahead of it so a failure leaves + * `resolvedAt` unwritten. Two calls answer the two failures — the first makes + * cleanup a precondition of committing, the second sees a cover that did not + * exist when the first ran, since a swap that succeeds means the cover was + * created before the commit. + */ + it("closes the cover when the resolve cascades before it exists and stores state after the swap", async () => { + const exhausted: AlertStateRecord = { ...unresolved(), escalationAttempt: 1 }; + const covers = buildFakeAlertmanagerStore(); + + // Hold the webhook's authoritative (un-guarded) state write open until the + // sweep has finished, and report when it is reached. That *is* the + // interleaving: everything the webhook does before storing `resolvedAt` + // happens first, and the store itself happens last. Gating on the write + // rather than on a tick count keeps it exact in both orderings. + let reachedStateWrite = false; + let releaseStateWrite!: () => void; + const gate = new Promise((resolve) => { releaseStateWrite = resolve; }); + const base = buildFakeStateStore(exhausted); + const store = { + ...base, + set: vi.fn(async (ref: unknown, value: AlertStateRecord, options?: { ifMatch?: unknown }) => { + if (!options || !("ifMatch" in options)) { + reachedStateWrite = true; + await gate; + } + return base.set(ref, value, options); + }), + } as unknown as ReturnType; + + const { ctx, mocks } = sweepContext(exhausted, null, covers); + mocks.state = store as never; + + // The resolve shares the sweep's covers/members tables, so its cascade is + // real rather than modelled — that is the whole point of this test. Every + // other table keeps the permissive single-delivery answers `resolveContext` + // already uses, so the only difference from the sibling test above is the + // cascade's visibility of the membership. + const resolveDb = { + namespace: "ns", + execute: async (sql: string, params: unknown[] = []) => + sql.includes("cover") ? covers.db.execute(sql, params) : { rowCount: 0 }, + query: async (sql: string, params: unknown[] = []) => + sql.includes("cover") ? covers.db.query(sql, params) : [], + }; + + let webhook!: Promise; + mocks.access.members.list = vi.fn(async () => { + // Start the resolve inside `createCover`, at the same await the + // compensated test uses — after the membership guard, before the cover + // issue exists. + webhook = handleResolved( + { ...(resolveContext(store) as unknown as Record), db: resolveDb } as unknown as PluginContext, + config(), + resolvedAlert, + ); + // Let it run right up to its state write. Bounded, so a change to the + // webhook's shape fails this test rather than hanging it. + for (let i = 0; i < 1000 && !reachedStateWrite; i++) await Promise.resolve(); + return [{ principalType: "user", principalId: "board-1", status: "active", membershipRole: "owner" }]; + }); + + // The sweep catches and logs a per-issue failure (runAlertEscalationSweep), + // so an assertion inside the members.list mock would be swallowed there. + // Assert the ordering out here instead, after the webhook is released and + // awaited: a webhook rejection surfaces first, then the ordering guard. + await runAlertEscalationSweep(ctx, config(), new Date("2026-07-11T01:00:00Z")); + releaseStateWrite(); + await webhook; + expect(reachedStateWrite).toBe(true); + + // The swap SUCCEEDED here — this is deliberately not the compensated + // branch, which is what makes it a distinct case from the test above. + expect(store.read().escalationComplete).toBe(true); + expect(store.read().resolvedAt).toBe("2026-07-11T02:00:00Z"); + // The two lines above hold on the refused-swap branch too, since the + // webhook's own write sets both. Only the claimed path posts the + // chain-exhausted comment, so this pins the test to the branch it names. + expect(mocks.issues.createComment).toHaveBeenCalledWith( + "issue-1", expect.stringContaining("Agent chain exhausted"), "company-1", + ); + // No cover may be left open with an unresolved member for a cleared alert. + // Both assertions fail with the cascade moved back ahead of the state + // write: membership stays open, which blocks the closing claim, so + // `reconcileStuckCovers` cannot clean it up either. + const [coverRow] = [...covers.covers.values()]; + expect(coverRow).toBeDefined(); + expect(covers.openMemberCount(coverRow.cover_issue_id)).toBe(0); + expect(coverRow.cancelled_at).not.toBeNull(); + }); + it("refuses a stale sweep write against a record any other writer touched", async () => { // Same guard, non-resolve mutation: any concurrent rewrite must void the // sweep's read. Otherwise this would be a special case for one field diff --git a/packages/plugins/paperclip-plugin-alertmanager/src/escalation.ts b/packages/plugins/paperclip-plugin-alertmanager/src/escalation.ts index 26aa9fea3d4d..d75f14bf0516 100644 --- a/packages/plugins/paperclip-plugin-alertmanager/src/escalation.ts +++ b/packages/plugins/paperclip-plugin-alertmanager/src/escalation.ts @@ -460,10 +460,19 @@ async function advanceIssueLadder( const claimed = await casAlertState(ctx, ref, state, { ...state, escalationAttempt: MAX_ATTEMPTS, escalationComplete: true, nextEscalationAt: null }); if (!claimed) { // A webhook won the record while the cover was being created. If it was a - // resolve, its own cascade ran before the cover existed and so could not - // see it — leaving an open board-assigned cover for an alert that has - // already cleared. Re-running the cascade here against the cover we just - // created is the compensating close. + // resolve, its own cascade may have run before the cover existed and so + // could not see it — which would leave an open board-assigned cover for + // an alert that has already cleared. Re-running the cascade here against + // the cover we just created is the compensating close. + // + // BLO-33497: this branch is reached only when the swap is REFUSED, which + // is exactly the half `handleResolved` cannot see for itself. It pairs + // with the second `recordSourceResolvedAndCloseCovers` there, behind its + // commit point: a swap that SUCCEEDS means the webhook had not yet stored + // `resolvedAt` when we claimed, so its post-commit cascade still lies + // ahead of it and will find the cover we just created. Keep the two in + // step — drop that call and this compensation stops being sufficient on + // its own. // // Safe to run unconditionally on a resolved winner: the cascade is // idempotent (`COALESCE(resolved_at, now())` plus the single-UPDATE diff --git a/packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts b/packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts index 05050a8fbe27..e5c6f0a83667 100644 --- a/packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts +++ b/packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts @@ -2537,6 +2537,14 @@ export async function handleResolved( // exhausted because the alert kept firing, not because the underlying // issue's status policy says so, so a resolved alert means its membership // in the shared cover is done either way. + // + // Position is load-bearing: this sits AHEAD of the `ctx.state.set` below, + // which is this delivery's commit point. Cover cleanup is therefore a + // precondition of recording the resolution — a throwing cascade aborts the + // delivery with `resolvedAt` still unwritten, so the retry re-runs every + // side effect rather than stranding an uncleaned cover behind a record that + // already claims the alert is over. Do not move it past the commit point; + // the second call below exists precisely so this one does not have to. await recordSourceResolvedAndCloseCovers( ctx, existing.paperclipCompanyId, @@ -2589,6 +2597,42 @@ export async function handleResolved( }; await ctx.state.set(stateRef, updated); + // BLO-33497: cascade a SECOND time, behind the commit point. This is not a + // duplicate of the call above — the two cover different failures, and + // neither subsumes the other: + // + // - the call AHEAD of the commit point makes cover cleanup a precondition + // of recording the resolution, so a cascade failure leaves nothing + // recorded and the retry redoes everything; + // - this one catches a cover that did not exist yet when that call ran. + // + // The escalation sweep's chain-exhausted rung creates its cover BEFORE its + // compare-and-swap (see `escalation.ts`; claiming first would leave a + // failed `createCover` permanently uncovered), so the two paths interleave. + // The sweep compensates when its swap is REFUSED, which is the half where + // this delivery had already stored `resolvedAt`. The other half had nothing + // watching it: the cascade above ran while the cover did not yet exist — + // no membership to mark — and `resolvedAt` landed only after the sweep's + // swap, so the swap SUCCEEDED and the sweep's compensation never ran. That + // stranded an open [user-cover] with an unresolved member for a cleared + // alert, which no later resolve can ever cascade into again. + // + // A swap that succeeds means the sweep read, created its cover and claimed + // all before the write above — so by the time we get here the cover exists + // and this call sees it. Together the two halves leave no window. + // + // Cheap and idempotent: `recordSourceResolvedAndCloseCovers` early-returns + // on `rowCount === 0` (the common case — most alerts never join a cover), + // re-marking is `COALESCE(resolved_at, now())`, and the close is a + // single-UPDATE claim only one caller can win. Failing here still fails the + // delivery, and the retry's pre-commit cascade closes the cover, which by + // then exists. + await recordSourceResolvedAndCloseCovers( + ctx, + existing.paperclipCompanyId, + aggregateResolution.issueId, + ); + await ctx.events.emit( "alertmanager.alert.resolved", existing.paperclipCompanyId,