diff --git a/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/owner-resolver.test.ts b/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/owner-resolver.test.ts index c36bb89cdcc6..5481e7cdb040 100644 --- a/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/owner-resolver.test.ts +++ b/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/owner-resolver.test.ts @@ -383,7 +383,7 @@ describe("resolveFallbackAgentId", () => { "company-1", "alert fallback", ), - ).resolves.toBe("agent-fallback"); + ).resolves.toEqual({ agentId: "agent-fallback" }); expect(agents.list).toHaveBeenCalledWith({ companyId: "company-1" }); }); @@ -397,12 +397,14 @@ describe("resolveFallbackAgentId", () => { const logger = { warn: vi.fn() }; const ctx = { agents, logger } as unknown as Parameters[0]; + // Both are config facts a retry cannot change, so both must be `permanent`: + // two same-named invokable agents stay ambiguous, and a wrong name stays wrong. await expect( resolveFallbackAgentId(ctx, "company-1", "Alert Fallback"), - ).resolves.toBeUndefined(); + ).resolves.toEqual({ refusal: "permanent" }); await expect( resolveFallbackAgentId(ctx, "company-1", "Missing Agent"), - ).resolves.toBeUndefined(); + ).resolves.toEqual({ refusal: "permanent" }); expect(logger.warn).toHaveBeenCalledTimes(2); }); }); @@ -518,45 +520,66 @@ describe("resolveFallbackAgentId", () => { ]); await expect( resolveFallbackAgentId(ctx, "company-1", "Ops Triage"), - ).resolves.toBe("agent-1"); + ).resolves.toEqual({ agentId: "agent-1" }); }); it("matches case-insensitively after trimming", async () => { const { ctx } = mkAgentCtx([{ id: "agent-1", name: " Ops Triage " }]); await expect( resolveFallbackAgentId(ctx, "company-1", "ops triage"), - ).resolves.toBe("agent-1"); + ).resolves.toEqual({ agentId: "agent-1" }); }); - it("returns undefined without querying when no name is configured", async () => { + it("refuses permanently without querying when no name is configured", async () => { const { ctx, list } = mkAgentCtx([{ id: "agent-1", name: "Ops Triage" }]); await expect( resolveFallbackAgentId(ctx, "company-1", undefined), - ).resolves.toBeUndefined(); + ).resolves.toEqual({ refusal: "permanent" }); await expect( resolveFallbackAgentId(ctx, "company-1", " "), - ).resolves.toBeUndefined(); + ).resolves.toEqual({ refusal: "permanent" }); expect(list).not.toHaveBeenCalled(); }); - it("returns undefined and warns when the name matches nothing", async () => { + it("refuses permanently and warns when the name matches nothing", async () => { const { ctx, logger } = mkAgentCtx([{ id: "agent-1", name: "Ops Triage" }]); await expect( resolveFallbackAgentId(ctx, "company-1", "Nobody"), - ).resolves.toBeUndefined(); + ).resolves.toEqual({ refusal: "permanent" }); expect(logger.warn).toHaveBeenCalledWith( expect.stringContaining("resolved to 0 invokable agents"), ); }); - it("returns undefined and warns when the name is ambiguous", async () => { + // An empty roster is the shape a degraded-but-non-throwing `agents.list` + // takes, and it is the one input the unmatched-name branch cannot tell from + // a genuine typo. A throwing list already retries (the caller's memo evicts + // and a plain Error propagates); this pins that the silent-failure variant of + // the *same* host fault gets the same survivable treatment rather than a + // permanent 200 drop. + // + // Paired deliberately with the wrong-name case directly above: same call, + // same "no match" outcome, opposite refusal class. Asserting only the empty + // case would not catch a change that made *both* transient and reinstated + // the retry burst on genuinely wrong config. + it("refuses transiently when the roster itself comes back empty", async () => { + const { ctx, logger } = mkAgentCtx([]); + await expect( + resolveFallbackAgentId(ctx, "company-1", "Ops Triage"), + ).resolves.toEqual({ refusal: "transient" }); + expect(logger.warn).toHaveBeenCalledWith( + expect.stringContaining("roster came back empty"), + ); + }); + + it("refuses permanently and warns when the name is ambiguous", async () => { const { ctx, logger } = mkAgentCtx([ { id: "agent-1", name: "Ops Triage" }, { id: "agent-2", name: "ops triage" }, ]); await expect( resolveFallbackAgentId(ctx, "company-1", "Ops Triage"), - ).resolves.toBeUndefined(); + ).resolves.toEqual({ refusal: "permanent" }); expect(logger.warn).toHaveBeenCalledWith( expect.stringContaining("resolved to 2 invokable agents"), ); @@ -576,20 +599,54 @@ describe("resolveFallbackAgentId", () => { // it or the ownerless-issue metric reads clean while the harm continues. // ------------------------------------------------------------------------- - it.each(["paused", "pending_approval", "terminated"])( - "refuses a sole name match that is %s", - async (status) => { - const { ctx, logger } = mkAgentCtx([ - { id: "agent-1", name: "Ops Triage", status }, - ]); - await expect( - resolveFallbackAgentId(ctx, "company-1", "Ops Triage"), - ).resolves.toBeUndefined(); - expect(logger.warn).toHaveBeenCalledWith( - expect.stringContaining("none are invokable"), - ); - }, - ); + // Each status carries its own refusal class, so these are a table of + // (status, class) pairs rather than one shared `it.each` over statuses: a + // paused owner becomes invokable on its own and must keep Alertmanager's + // retry window, while a terminated one needs a roster edit and must not. + // Bundling them would assert only "refuses" and lose the distinction. + // + // ⚠️ The `terminated` row pins the map entry in isolation, not the + // production route. It injects a terminated agent into the mocked list, but + // the real `agents.list` filters those out (see the comment above), so in + // production a terminated owner is absent from the roster entirely and is + // refused by the unmatched-name branch instead — same `permanent` outcome, + // different code path. Keep the row as a guard on the map; do not read it as + // evidence that the classification ladder handles `terminated` live. The + // empty-roster case below covers the branch that does the real deciding. + it.each([ + ["paused", "transient"], + ["pending_approval", "transient"], + ["terminated", "permanent"], + ])("refuses a sole name match that is %s as %s", async (status, refusal) => { + const { ctx, logger } = mkAgentCtx([ + { id: "agent-1", name: "Ops Triage", status }, + ]); + await expect( + resolveFallbackAgentId(ctx, "company-1", "Ops Triage"), + ).resolves.toEqual({ refusal }); + expect(logger.warn).toHaveBeenCalledWith( + expect.stringContaining("none are invokable"), + ); + }); + + it("refuses a status it does not recognise as transient", async () => { + // The one `REFUSAL_CLASS_BY_INVOKABILITY_REASON` entry with no other + // coverage, and the one that encodes the asymmetry policy stated at + // `owner-resolver.ts:226-229`: an unrecognised status means this resolver + // cannot claim the condition is unfixable, so it takes the survivable + // branch. Pinned so a later tidy-up does not read it as an arbitrary + // default and flip it — misclassifying transient-as-permanent drops an + // alert, which is the single outcome this whole path exists to prevent. + const { ctx, logger } = mkAgentCtx([ + { id: "agent-1", name: "Ops Triage", status: "hibernating" }, + ]); + await expect( + resolveFallbackAgentId(ctx, "company-1", "Ops Triage"), + ).resolves.toEqual({ refusal: "transient" }); + expect(logger.warn).toHaveBeenCalledWith( + expect.stringContaining("agent-1=unknown_status"), + ); + }); it("names the blocking reason rather than reporting an unmatched name", async () => { // "paused" and "name is wrong" need different fixes, so an operator must @@ -611,7 +668,7 @@ describe("resolveFallbackAgentId", () => { ]); await expect( resolveFallbackAgentId(ctx, "company-1", "Ops Triage"), - ).resolves.toBe("agent-1"); + ).resolves.toEqual({ agentId: "agent-1" }); }, ); @@ -624,7 +681,7 @@ describe("resolveFallbackAgentId", () => { ]); await expect( resolveFallbackAgentId(ctx, "company-1", "Ops Triage"), - ).resolves.toBe("agent-live"); + ).resolves.toEqual({ agentId: "agent-live" }); }); it("still refuses when two invokable agents share the name", async () => { @@ -634,7 +691,7 @@ describe("resolveFallbackAgentId", () => { ]); await expect( resolveFallbackAgentId(ctx, "company-1", "Ops Triage"), - ).resolves.toBeUndefined(); + ).resolves.toEqual({ refusal: "permanent" }); expect(logger.warn).toHaveBeenCalledWith( expect.stringContaining("resolved to 2 invokable agents"), ); @@ -652,9 +709,11 @@ describe("resolveFallbackAgentId", () => { reportsTo: "agent-gone", } as { id: string; name: string; status: string }, ]); + // Transient, not permanent: the chain becomes valid again when the manager + // upstream is restored, with nobody editing this plugin's config. await expect( resolveFallbackAgentId(ctx, "company-1", "Ops Triage"), - ).resolves.toBeUndefined(); + ).resolves.toEqual({ refusal: "transient" }); expect(logger.warn).toHaveBeenCalledWith( expect.stringContaining("none are invokable"), ); diff --git a/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts b/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts index 92394e57d7e4..efb359b6f191 100644 --- a/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts +++ b/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts @@ -669,16 +669,304 @@ describe("handleWebhook — firing first time", () => { it("fails closed when the fallback agent configuration is missing", async () => { const { ctx, mocks } = mkCtx(); const config = baseConfig({ ownerMap: {}, fallbackAgentName: undefined }); + // The fail-closed guarantee (BLO-26613) is unchanged and still asserted + // below: no ownerless issue, no state row. PEN-2581 changed only how the + // drop is *reported* — an unresolvable owner is a config/roster fact no + // retry can fix, so the delivery acknowledges it instead of failing. + await expect( + handleWebhook(ctx, config, true, baseInput()), + ).resolves.toBeUndefined(); + expect(mocks.issues.create).not.toHaveBeenCalled(); + expect(mocks.state.set).not.toHaveBeenCalled(); + expect(mocks.metrics.write).toHaveBeenCalledWith( + "alertmanager.owner.fallback_failed", + 1, + { + alertname: "CiliumPolicyDropsHigh", + severity: "critical", + refusal: "permanent", + }, + ); + expect(mocks.metrics.write).toHaveBeenCalledWith( + "alertmanager.alert.permanent_error", + 1, + { alertname: "CiliumPolicyDropsHigh", severity: "critical" }, + ); + }); + + it("drops one ownerless alert without aborting the loop or failing the delivery", async () => { + // What this pins, stated precisely, because the obvious reading is wrong: + // the per-alert catch in `handleWebhook` ALREADY kept the rest of the batch + // processing before PEN-2581. The catch is inside the loop and + // `AlertDeliveryIncompleteError` is thrown only after it completes, so a + // sibling alert's issue was created on the first attempt even when the + // delivery reported 502. Verified by running it, not assumed, and + // reproducible as written: neutralise the carve-out (`const permanent = + // false` at the per-alert catch in `webhook-handler.ts`), then comment out + // BOTH the outcome assertion below and the `permanent_error` assertion at + // the end — on the pre-change source that alert reports through + // `alertmanager.alert.error`. The `issues.create` and `alert:` state + // assertions still pass. Both have to be neutralised first because Vitest + // aborts a test at its first failing assertion, so a run that trips the + // outcome assertion never reaches the ones that carry the point. + // + // What PEN-2581 actually changed is the delivery's reported *outcome*: the + // ownerless fingerprint is no longer accumulated, so Alertmanager is no + // longer told to retry a batch 15-17× for a fault no retry can fix, and the + // resulting failure storm no longer masks genuinely-transient failures that + // retrying would have fixed. + // + // The production incident did dark-tier every alert, but for an + // incident-specific reason rather than a structural one: with + // `fallbackAgentName` unset, *every* unmapped alert in the batch took this + // same throw, so there were no healthy siblings left to survive. That case + // is real and reachable — it is just not what "one ownerless alert" does to + // a mixed batch, which is what this test covers. + const { ctx, mocks } = mkCtx(); + const config = baseConfig({ fallbackAgentName: undefined }); + // team=platform resolves through ownerMap; team=storage is unmapped, and + // with no fallbackAgentName it is permanently ownerless. + mocks.users.findByEmail.mockResolvedValue({ + id: "user-42", + email: "alice@example.com", + name: "Alice", + }); + const owned = baseAlert({ + labels: { + alertname: "CiliumPolicyDropsHigh", + severity: "critical", + team: "platform", + node: "pve-3", + }, + fingerprint: "aaaa1111", + }); + const ownerless = baseAlert({ + labels: { + alertname: "CephOsdNearFull", + severity: "critical", + team: "storage", + node: "pve-4", + }, + fingerprint: "bbbb2222", + }); + + await expect( + handleWebhook( + ctx, + config, + true, + baseInput({ + parsedBody: baseEnvelope({ alerts: [ownerless, owned] }), + }), + ), + ).resolves.toBeUndefined(); + + // The healthy alert still became tracked work — and it is ordered SECOND in + // the payload, behind the ownerless one, so this pins that the permanent + // drop does not abort the remainder of the loop. + expect(mocks.issues.create).toHaveBeenCalledTimes(1); + expect(mocks.issues.create.mock.calls[0][0].title).toBe( + "[critical] CiliumPolicyDropsHigh · platform", + ); + // The other half of the BLO-26613 fail-closed guarantee, asserted here and + // not only in the single-alert tests: the ownerless alert must leave no + // state row behind, and a multi-alert batch is where that is easiest to + // regress. + // + // Filtered to `alert:` keys rather than counting `state.set` calls + // outright: the healthy alert's owner lookup also memoises + // `owner-by-email:…` on the instance scope, so a raw count would be 2 and + // would couple this fail-closed assertion to an unrelated cache. Keying on + // the fingerprint says the thing we actually mean — one alert row, and it + // belongs to the alert that got an issue. + const alertStateWrites = mocks.state.set.mock.calls.filter((call) => + String(call[0].stateKey).startsWith("alert:"), + ); + expect(alertStateWrites).toHaveLength(1); + expect(alertStateWrites[0][0].stateKey).toBe(`alert:${owned.fingerprint}`); + expect(mocks.metrics.write).toHaveBeenCalledWith( + "alertmanager.alert.permanent_error", + 1, + { alertname: "CephOsdNearFull", severity: "critical" }, + ); + }); + + // The permanent drop above is driven by *unset config*, which returns before + // `agents.list` is ever called. This one is driven by roster contents: the + // name is configured and correct-looking, and the refusal is decided by what + // the list came back with. That is the branch every roster-derived permanent + // refusal actually takes in production — `agents.list` filters terminated + // agents out, so a terminated fallback owner never reaches the eligibility + // ladder and lands here as an unmatched name instead. + it("permanently drops when the configured name is absent from the roster", async () => { + const { ctx, mocks } = mkCtx(); + const config = baseConfig({ ownerMap: {} }); + // A non-empty roster that simply does not contain the configured name — + // indistinguishable from a typo, and correctly permanent. + mocks.agents.list.mockResolvedValue([ + { id: "agent-other", name: "Someone Else", status: "idle" }, + ]); + + await expect( + handleWebhook(ctx, config, true, baseInput()), + ).resolves.toBeUndefined(); + + expect(mocks.issues.create).not.toHaveBeenCalled(); + expect(mocks.state.set).not.toHaveBeenCalled(); + expect(mocks.metrics.write).toHaveBeenCalledWith( + "alertmanager.owner.fallback_failed", + 1, + { + alertname: "CiliumPolicyDropsHigh", + severity: "critical", + refusal: "permanent", + }, + ); + }); + + // The guard that separates a degraded host from a wrong name. Same "no + // match" outcome as the test directly above, opposite refusal class, and the + // only difference in the input is that the roster is empty rather than merely + // lacking the name. An `agents.list` that fails by *returning* `[]` instead + // of throwing would otherwise be dropped at 200 and never retried, while the + // throwing variant of the identical fault keeps its retry window. + it("keeps the retry window when the roster comes back empty", async () => { + const { ctx, mocks } = mkCtx(); + const config = baseConfig({ ownerMap: {} }); + mocks.agents.list.mockResolvedValue([]); + await expect( handleWebhook(ctx, config, true, baseInput()), ).rejects.toBeInstanceOf(AlertDeliveryIncompleteError); + + // Fail-closed is intact either way — the class change is about the + // reporting channel, never about creating an ownerless issue. expect(mocks.issues.create).not.toHaveBeenCalled(); expect(mocks.state.set).not.toHaveBeenCalled(); expect(mocks.metrics.write).toHaveBeenCalledWith( "alertmanager.owner.fallback_failed", 1, + { + alertname: "CiliumPolicyDropsHigh", + severity: "critical", + refusal: "transient", + }, + ); + expect(mocks.metrics.write).not.toHaveBeenCalledWith( + "alertmanager.alert.permanent_error", + 1, + expect.anything(), + ); + }); + + it("keeps the retry window when the fallback agent is only paused", async () => { + // The counterpart to the permanent drop above, and the case that makes the + // refusal *class* load-bearing rather than cosmetic. A paused fallback owner + // becomes invokable again with nobody editing config, so Alertmanager's + // 15-17 retries are the only thing that lets the alert land within minutes + // of the unpause instead of waiting out a whole `repeat_interval`. Dropping + // it at 200 here would be a time-to-detect regression for a critical alert. + const { ctx, mocks } = mkCtx(); + const config = baseConfig({ ownerMap: {} }); + mocks.agents.list.mockResolvedValue([ + { id: "agent-fallback", name: "Alert Fallback", status: "paused" }, + ]); + + await expect( + handleWebhook(ctx, config, true, baseInput()), + ).rejects.toBeInstanceOf(AlertDeliveryIncompleteError); + + // Fail-closed is still intact — a paused owner is no more assignable than a + // terminated one; only the reporting channel differs. + expect(mocks.issues.create).not.toHaveBeenCalled(); + expect(mocks.state.set).not.toHaveBeenCalled(); + // Reported as transient, NOT permanent. + expect(mocks.metrics.write).toHaveBeenCalledWith( + "alertmanager.alert.error", + 1, { alertname: "CiliumPolicyDropsHigh", severity: "critical" }, ); + expect(mocks.metrics.write).not.toHaveBeenCalledWith( + "alertmanager.alert.permanent_error", + 1, + expect.anything(), + ); + // The `refusal` label's other value, pinned here because the permanent test + // above pins only `"permanent"`. Splitting drop-from-retry within this one + // series is the label's entire purpose, so a regression that hardcoded + // `"permanent"` at the write site would otherwise pass the whole suite — + // the alert-level metric split asserted just above would still be correct. + expect(mocks.metrics.write).toHaveBeenCalledWith( + "alertmanager.owner.fallback_failed", + 1, + { + alertname: "CiliumPolicyDropsHigh", + severity: "critical", + refusal: "transient", + }, + ); + // The operator-facing warning names the blocking reason, so the next + // occurrence is diagnosable from the log alone (PEN-2581). + expect(mocks.logger.warn).toHaveBeenCalledWith( + expect.stringContaining("agent-fallback=paused"), + ); + }); + + it("still acknowledges a permanent drop when the metrics write fails", async () => { + // The permanent drop's own invariant must not depend on telemetry being up. + // If the `fallback_failed` write threw, handleFiring would surface a + // *metrics* error instead of PermanentAlertError, the per-alert catch would + // treat it as transient and push the fingerprint, and the delivery would + // 502 — reinstating the doomed retry burst this path exists to remove, and + // taking the rest of the batch down with it. + const { ctx, mocks } = mkCtx(); + const config = baseConfig({ ownerMap: {}, fallbackAgentName: undefined }); + mocks.metrics.write.mockImplementation(async (name: string) => { + if (name === "alertmanager.owner.fallback_failed") { + throw new Error("metrics backend unavailable"); + } + }); + + await expect( + handleWebhook(ctx, config, true, baseInput()), + ).resolves.toBeUndefined(); + + expect(mocks.issues.create).not.toHaveBeenCalled(); + expect(mocks.state.set).not.toHaveBeenCalled(); + // The permanent classification survived the telemetry outage. + expect(mocks.metrics.write).toHaveBeenCalledWith( + "alertmanager.alert.permanent_error", + 1, + { alertname: "CiliumPolicyDropsHigh", severity: "critical" }, + ); + // And the swallowed metrics failure is still audible in the log. + expect(mocks.logger.error).toHaveBeenCalledWith( + expect.stringContaining("failed to record fallback owner metric"), + ); + }); + + it("still fails the delivery for a transient per-alert fault", async () => { + // Control for the two tests above: the permanent carve-out must not have + // widened into "swallow every per-alert failure". A transient fault still + // owes Alertmanager a retry, so it still fails the delivery and still + // reports through the transient metric (BLO-20467's silent-loss guard). + const { ctx, mocks } = mkCtx(); + const config = baseConfig(); + mocks.issues.create.mockRejectedValueOnce(new Error("issue RPC timed out")); + + await expect( + handleWebhook(ctx, config, true, baseInput()), + ).rejects.toBeInstanceOf(AlertDeliveryIncompleteError); + expect(mocks.metrics.write).toHaveBeenCalledWith( + "alertmanager.alert.error", + 1, + { alertname: "CiliumPolicyDropsHigh", severity: "critical" }, + ); + expect(mocks.metrics.write).not.toHaveBeenCalledWith( + "alertmanager.alert.permanent_error", + 1, + expect.anything(), + ); }); it("joins an active aggregate winner before requiring fallback ownership", async () => { diff --git a/packages/plugins/paperclip-plugin-alertmanager/src/owner-resolver.ts b/packages/plugins/paperclip-plugin-alertmanager/src/owner-resolver.ts index 1a0b2c4f6ff4..714985d6067c 100644 --- a/packages/plugins/paperclip-plugin-alertmanager/src/owner-resolver.ts +++ b/packages/plugins/paperclip-plugin-alertmanager/src/owner-resolver.ts @@ -7,6 +7,7 @@ import type { PluginContext } from "@paperclipai/plugin-sdk"; import { getAgentWorkEligibility } from "@paperclipai/shared"; +import type { AgentEligibilityLifecycleReason } from "@paperclipai/shared"; import { ASSIGNEE_OVERRIDE_ANNOTATION, ASSIGNEE_OVERRIDE_LABEL, @@ -176,12 +177,12 @@ export async function resolveAssigneeUserId( * can review in a config diff. That makes the lookup ambiguous in principle, * so anything other than exactly one match is refused: zero matches means the * name is wrong, and more than one means the caller cannot know which agent - * the operator meant. Both return `undefined`, and the caller fails closed - * rather than filing an ownerless issue. + * the operator meant. Both refuse, and the caller fails closed rather than + * filing an ownerless issue. * - * Returning `undefined` for blank/absent config is deliberate: an instance - * with no `fallbackAgentName` at all is a misconfiguration for this plugin, - * and the caller — not this resolver — decides how loudly to fail. + * Refusing for blank/absent config is deliberate: an instance with no + * `fallbackAgentName` at all is a misconfiguration for this plugin, and the + * caller — not this resolver — decides how loudly to fail. * * A name match is not enough: the agent must also be *invokable*. The host's * `agents.list` filters only `terminated` (`server/src/services/agents.ts`), so @@ -195,18 +196,107 @@ export async function resolveAssigneeUserId( * in step with what `invoke` actually enforces (including an invalid reporting * chain, which blocks invoke just as surely as a paused status). The whole * company snapshot is already in hand, which is exactly the input it needs. + * + * A refusal is additionally classified `permanent` or `transient`, because the + * caller reports the two through different channels: a permanent refusal is + * dropped with a 200 (retrying cannot help), while a transient one keeps + * Alertmanager's retry window, which is the only thing that lets an alert land + * within minutes of a pause lifting. See `REFUSAL_CLASS_BY_INVOKABILITY_REASON`. + */ +export type FallbackOwnerRefusal = "permanent" | "transient"; + +/** + * Either a resolved fallback owner, or a refusal with its class. Exactly one + * field is ever set — the caller checks `agentId` first and only consults + * `refusal` on the miss. + */ +export type FallbackOwnerResolution = + | { agentId: string; refusal?: undefined } + | { agentId?: undefined; refusal: FallbackOwnerRefusal }; + +/** + * Which non-invokable reasons clear on their own, and which need a human to + * change config or the roster. + * + * `paused` and `pending_approval` are process state: an unpause or a board + * approval flips them with nobody editing config. `invalid_org_chain` is + * mixed — `getAgentOrgChainHealth` returns it for `missing_manager`, which is + * genuinely self-clearing, but also for `cycle` and `terminated_ancestor`, + * which need a human roster edit exactly as `terminated` does. It is + * classified `transient` under the survivable-direction rule below, *not* + * because it always self-clears. `terminated` is the one reason here that is + * both unambiguous and unfixable by waiting. + * + * Note that `terminated` is unreachable from this resolver in production, and + * the entry is a guard on the map rather than a live classification: the host + * calls `agents.list` with no options (`plugin-host-services.ts`), which + * filters `ne(status, "terminated")`, so a terminated fallback owner is never + * in the list to be evaluated. It resolves as zero name matches instead, and + * is refused permanently by the unmatched-name branch below. + * + * `unknown_status` is deliberately transient: it means this resolver does not + * recognise the status, so it cannot claim the condition is unfixable. + * Misclassifying transient-as-permanent drops an alert; permanent-as-transient + * only costs a retry burst, so the unknown case takes the survivable error. + * + * Declared as an exhaustive `Record` rather than a `Set` of the permanent ones + * so that adding a reason to `AgentEligibilityLifecycleReason` fails to compile + * here instead of silently defaulting a new condition into either class. + */ +const REFUSAL_CLASS_BY_INVOKABILITY_REASON: Record< + AgentEligibilityLifecycleReason, + FallbackOwnerRefusal +> = { + terminated: "permanent", + paused: "transient", + pending_approval: "transient", + invalid_org_chain: "transient", + unknown_status: "transient", + // Never reached: an `eligible` agent is invokable and so never refused. + // Present only to keep the record exhaustive. Mapped to `transient` rather + // than `permanent` so that the unreachable case, if a future refactor ever + // does reach it, fails in the same survivable direction as `unknown_status`: + // a needless retry burst, not a dropped alert. + eligible: "transient", +}; + +/** + * Resolve the configured `fallbackAgentName` to exactly one invokable agent id, + * or explain why it could not. */ export async function resolveFallbackAgentId( ctx: Pick, companyId: string, fallbackAgentName: string | undefined, -): Promise { +): Promise { const target = fallbackAgentName?.trim().toLowerCase(); - if (!target) return undefined; + // No name configured at all: nothing resolves until someone edits config. + if (!target) return { refusal: "permanent" }; // One unwindowed company-wide snapshot rather than a paged scan: the host's // list is unordered, so paging could drift a match across page boundaries // and turn a stable config into an intermittent ownerless-issue bug. const agents = await ctx.agents.list({ companyId }); + // A host fault that *throws* is already handled correctly downstream: the + // caller's memo evicts and a plain `Error` keeps Alertmanager's retry window. + // This guard covers the same class of degradation arriving by a quieter + // route — a lagging read replica, a company-scoping regression, a partial + // read — where the list resolves to `[]` instead. Without it that lands as + // zero name matches below and drops the alert permanently at 200, which is + // strictly worse than the throwing case for an identical underlying fault. + // + // A company with a configured Alertmanager plugin and zero non-terminated + // agents is not a legitimate steady state, so treating it as a host fault + // costs nothing real. Deliberately narrow: zero matches against a *non-empty* + // roster stays permanent, because nothing distinguishes it from the wrong + // name it usually is. Only the empty roster is separable, and it takes the + // survivable direction under the same asymmetry stated above for + // `unknown_status` — a retry burst, not a dropped alert. + if (agents.length === 0) { + ctx.logger.warn( + `Fallback agent "${fallbackAgentName}" could not be resolved: the company roster came back empty; refusing ownerless issue creation (transient)`, + ); + return { refusal: "transient" }; + } const nameMatches = agents.filter( (agent) => agent.name.trim().toLowerCase() === target, ); @@ -227,17 +317,41 @@ export async function resolveFallbackAgentId( `${entry.agent.id}=${entry.eligibility.invokabilityReason}`, ) .join(", "); + // Any one self-clearing candidate makes the whole refusal transient: a + // paused duplicate alongside a terminated one still becomes resolvable + // the moment the pause lifts, with no config edit. + // + // Tested as `!== "permanent"` rather than `=== "transient"` so that the + // *runtime* default matches the documented policy above: a reason missing + // from the map indexes to `undefined`, and only this direction lands that + // on the survivable branch. The exhaustive `Record` makes a gap + // unreachable within the monorepo, but the plugin resolves + // `getAgentWorkEligibility` from `@paperclipai/shared` at runtime, so a + // built plugin running against a newer host could see a reason its own + // copy of the map never had. That skew must not silently start dropping + // alerts. + const refusal: FallbackOwnerRefusal = evaluated.some( + (entry) => + REFUSAL_CLASS_BY_INVOKABILITY_REASON[ + entry.eligibility.invokabilityReason + ] !== "permanent", + ) + ? "transient" + : "permanent"; ctx.logger.warn( - `Fallback agent "${fallbackAgentName}" matched ${evaluated.length} agent(s) but none are invokable (${reasons}); refusing ownerless issue creation`, + `Fallback agent "${fallbackAgentName}" matched ${evaluated.length} agent(s) but none are invokable (${reasons}); refusing ownerless issue creation (${refusal})`, ); - return undefined; + return { refusal }; } ctx.logger.warn( `Fallback agent "${fallbackAgentName}" resolved to ${invokable.length} invokable agents; refusing ownerless issue creation`, ); - return undefined; + // Zero name matches (wrong name) and two or more invokable matches + // (genuinely ambiguous) are both config facts: neither changes on its own. + return { refusal: "permanent" }; } - return invokable[0]?.agent.id; + const agentId = invokable[0]?.agent.id; + return agentId ? { agentId } : { refusal: "permanent" }; } function normalizeEmail(email: string): string { diff --git a/packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts b/packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts index 68cd26712588..1c91bf2671d0 100644 --- a/packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts +++ b/packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts @@ -26,6 +26,7 @@ import { } from "./issue-mapping.js"; import { resolveIssueRoute } from "./issue-route-resolver.js"; import { resolveAssigneeUserId, resolveFallbackAgentId } from "./owner-resolver.js"; +import type { FallbackOwnerResolution } from "./owner-resolver.js"; import { aggregateKeyForAlert } from "./aggregate-key.js"; import { escalationDeadlineMs, recordSourceResolvedAndCloseCovers } from "./escalation.js"; import { @@ -65,6 +66,36 @@ export class AlertDeliveryIncompleteError extends Error { } } +/** + * Raised by a per-alert path whose failure no retry can fix — a configuration + * or roster fact rather than process state. + * + * The per-alert catch treats these as *handled*: the alert is dropped, the + * failure is recorded (log + metric), and the fingerprint is deliberately NOT + * added to `failedFingerprints`, so the delivery still answers 200. + * + * This is the same "log + 200" treatment the malformed-payload and + * permanent-policy drops already get. It exists because the taxonomy the + * per-alert catch was written against — "these failures are issue-RPC, + * state-store, event, and metric errors, which are transient" — stopped being + * true once `handleFiring` began throwing on unresolvable fallback ownership + * (PEN-2581). Reporting a permanent fault through the transient channel makes + * Alertmanager retry it 15-17× and drop the delivery anyway, and the resulting + * `alertmanager_notifications_failed_total` storm masks concurrent *transient* + * failures that retrying would genuinely have fixed. + * + * Only reachable from the firing path (owner resolution is never run on + * resolve), so a dropped alert that is still firing returns on Alertmanager's + * next `repeat_interval` — this trades a doomed retry burst for a later + * re-delivery, not for silent permanent loss. + */ +export class PermanentAlertError extends Error { + constructor(message: string) { + super(message); + this.name = "PermanentAlertError"; + } +} + const AGGREGATE_CREATION_CLAIMS_TABLE = "alertmanager_aggregate_creation_claims"; const AGGREGATE_MEMBERS_TABLE = "alertmanager_aggregate_members"; const AGGREGATE_LIFECYCLE_FENCES_TABLE = "alertmanager_aggregate_lifecycle_fences"; @@ -1065,14 +1096,14 @@ function suppressionExpiryLabel( * delivery (rather than the module) is what keeps it correct: a config edit or * an agent being paused takes effect on the very next delivery. */ -export type FallbackOwnerMemo = Map>; +export type FallbackOwnerMemo = Map>; function resolveFallbackAgentIdMemoized( ctx: Pick, companyId: string, fallbackAgentName: string | undefined, memo: FallbackOwnerMemo | undefined, -): Promise { +): Promise { if (!memo) return resolveFallbackAgentId(ctx, companyId, fallbackAgentName); // JSON-encoded pair rather than a naive `a + sep + b`: agent names are // operator-supplied config, so any single-character separator could be @@ -1085,11 +1116,12 @@ function resolveFallbackAgentIdMemoized( companyId, fallbackAgentName, ).catch((err: unknown) => { - // Evict on failure. A refusal (bad name / paused / ambiguous) resolves to - // `undefined` and IS cached — it is a config fact, stable for the delivery. - // A *throw* is a transient host fault, and caching it would let one failed - // `agents.list` poison every remaining alert in the batch, converting a - // blip that previously cost one alert into a whole-delivery failure. + // Evict on failure. A refusal (bad name / paused / ambiguous) resolves to a + // `refusal` value and IS cached — the underlying condition is stable for the + // delivery, whether or not it is permanent beyond it. A *throw* is a + // transient host fault, and caching it would let one failed `agents.list` + // poison every remaining alert in the batch, converting a blip that + // previously cost one alert into a whole-delivery failure. memo.delete(key); throw err; }); @@ -1522,7 +1554,7 @@ export async function handleFiring( ? `agent:${resolution.agentId}` : resolution.email ?? "(none)"; } - const fallbackAssigneeAgentId = + const fallbackResolution = retainedIssue || createAssigneeAgentId || createAssigneeUserId ? undefined : await resolveFallbackAgentIdMemoized( @@ -1531,18 +1563,48 @@ export async function handleFiring( config.fallbackAgentName, fallbackOwnerMemo, ); + const fallbackAssigneeAgentId = fallbackResolution?.agentId; const finalAssigneeAgentId = createAssigneeAgentId ?? fallbackAssigneeAgentId; if (!retainedIssue && !finalAssigneeAgentId && !createAssigneeUserId) { + // Only `terminated` / wrong-name / genuinely-ambiguous is unfixable by + // retrying. A `paused` or `pending_approval` fallback owner becomes + // invokable without anyone editing config, and Alertmanager's retry window + // is the only thing that lets the alert land within minutes of that rather + // than waiting out a whole `repeat_interval`. Absent a classification we + // take the transient branch: a needless retry burst is survivable, a + // wrongly-dropped alert is not. + const isPermanent = fallbackResolution?.refusal === "permanent"; ctx.logger.warn( - `Cannot create issue for ${alertname}: fallbackAgentName is missing, invalid, or ambiguous`, - ); - await ctx.metrics.write("alertmanager.owner.fallback_failed", 1, { - alertname, - severity, - }); - throw new Error( - `Fallback owner resolution failed for ${alertname}; refusing ownerless issue creation`, + `Cannot create issue for ${alertname}: fallbackAgentName is missing, invalid, or ambiguous (${ + isPermanent ? "permanent" : "transient" + })`, ); + try { + // `refusal` splits the two outcomes this metric otherwise conflates: a + // permanent refusal is dropped at 200 and will not be retried, a + // transient one keeps Alertmanager's retry window. Without the label an + // operator has to join this series against + // `alertmanager.alert.permanent_error` to tell "gone until someone edits + // config" from "retrying, may still land". Two values, so no meaningful + // cardinality cost. + await ctx.metrics.write("alertmanager.owner.fallback_failed", 1, { + alertname, + severity, + refusal: isPermanent ? "permanent" : "transient", + }); + } catch (metricErr) { + // Best-effort, matching the severity-floor and opt-out drops above. On the + // permanent branch this is load-bearing: letting a metrics outage throw + // would surface a *metrics* error instead of `PermanentAlertError`, the + // per-alert catch would push the fingerprint, and the delivery would 502 + // — reinstating exactly the doomed retry burst this path removes, and + // taking the rest of the batch down with it. + ctx.logger.error( + `paperclip-plugin-alertmanager: failed to record fallback owner metric for ${alert.fingerprint}: ${String(metricErr)}`, + ); + } + const message = `Fallback owner resolution failed for ${alertname}; refusing ownerless issue creation`; + throw isPermanent ? new PermanentAlertError(message) : new Error(message); } const routeProjectId = nonEmptyString(issueRoute?.projectId); const routeGoalId = nonEmptyString(issueRoute?.goalId); @@ -2227,17 +2289,44 @@ export async function handleWebhook( // so Alertmanager stopped retrying and the alert was destroyed with no // durable issue or state row — the same silent-loss class as the outage // this plugin already suffered (BLO-20467). + // + // `PermanentAlertError` is the one documented exception to that taxonomy: + // a config/roster fault no retry can fix, so it takes the same "log + 200" + // route as the malformed payload above instead of the transient-retry + // route. Retrying it burns Alertmanager's 15-17 attempts, drops the + // delivery anyway, and storms the failure metric that transient faults + // need to stay legible. See the class doc (PEN-2581). + const permanent = err instanceof PermanentAlertError; ctx.logger.error( - `paperclip-plugin-alertmanager: error processing alert ${alert.fingerprint}: ${String(err)}`, + permanent + ? `paperclip-plugin-alertmanager: permanently dropping alert ${alert.fingerprint}: ${String(err)} — no retry can resolve this, so the delivery is not failed` + : `paperclip-plugin-alertmanager: error processing alert ${alert.fingerprint}: ${String(err)}`, ); - failedFingerprints.push(alert.fingerprint); + if (!permanent) { + failedFingerprints.push(alert.fingerprint); + } try { - await ctx.metrics.write("alertmanager.alert.error", 1, { - alertname: alert.labels.alertname ?? "unknown", - }); + // `severity` is carried on both branches for the same reason the + // `refusal` label exists on `alertmanager.owner.fallback_failed`: + // without it, "did we drop a critical?" needs a join against another + // series. It matters most on the permanent branch — that drop returns + // 200, so it is by design invisible in Alertmanager's own failure + // metrics and this series is the entire detection surface for it. + await ctx.metrics.write( + permanent + ? "alertmanager.alert.permanent_error" + : "alertmanager.alert.error", + 1, + { + alertname: alert.labels.alertname ?? "unknown", + severity: alert.labels.severity ?? "unknown", + }, + ); } catch (metricErr) { // Telemetry is best-effort; a metrics outage must not be the thing that - // aborts the remaining alerts. The delivery already counts as failed. + // aborts the remaining alerts. The delivery's outcome is already + // decided either way — failed for a transient fault, 200 for a + // permanent one. ctx.logger.error( `paperclip-plugin-alertmanager: failed to record alert error metric for ${alert.fingerprint}: ${String(metricErr)}`, );