fix(alertmanager): cascade covers behind the commit point too, closing the orphaned [user-cover] race (BLO-33497)

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
 

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<void>((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<typeof buildFakeStateStore>;
+
+    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<unknown>;
+    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<string, unknown>), 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

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

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,