From 504566a40da8788aa7fa645faed7d1611b012fea Mon Sep 17 00:00:00 2001 From: Rajat Saxena Date: Thu, 29 Oct 2026 16:24:25 +0530 Subject: [PATCH 1/3] fix(api): explain why an ESP can't be deleted and never drop its history (#12) Delete now answers 409 with a reason code (esp_is_default, esp_has_delivery_history, esp_retire_instead, delivery_source_in_use) instead of silently deleting a team's live default or reporting every blocker as delivery_source_in_use. Activated drafts can be retired, MCP gains retire_esp, and blocked/organization deletes go to PostHog. --- apps/api/src/mcp/policy.ts | 1 + apps/api/src/mcp/server.test.ts | 4 +- apps/api/src/mcp/tools/esp.ts | 47 ++++-- apps/api/src/mcp/tools/tools.test.ts | 32 +++- apps/api/src/observability/instance.ts | 3 + apps/api/src/organization/routes.ts | 26 +++- apps/api/src/settings/esp/delete-blockers.ts | 31 ++++ apps/api/src/settings/esp/queries.test.ts | 149 ++++++++++++++++++- apps/api/src/settings/esp/queries.ts | 76 +++++++--- apps/api/src/settings/esp/routes.test.ts | 32 +++- apps/api/src/settings/esp/routes.ts | 15 +- packages/api-contract/src/contract.ts | 4 +- packages/api-contract/src/mcp-parity.ts | 4 +- 13 files changed, 364 insertions(+), 60 deletions(-) create mode 100644 apps/api/src/settings/esp/delete-blockers.ts diff --git a/apps/api/src/mcp/policy.ts b/apps/api/src/mcp/policy.ts index b99cca26..f4882ce4 100644 --- a/apps/api/src/mcp/policy.ts +++ b/apps/api/src/mcp/policy.ts @@ -131,6 +131,7 @@ const toolPolicies = { delete_esp: MCP_SCOPES.espWrite, test_esp: MCP_SCOPES.espWrite, activate_esp: MCP_SCOPES.espWrite, + retire_esp: MCP_SCOPES.espWrite, list_teams: MCP_SCOPES.teamsRead, get_plan_usage: MCP_SCOPES.teamsRead, diff --git a/apps/api/src/mcp/server.test.ts b/apps/api/src/mcp/server.test.ts index 13c853cf..931f2b7b 100644 --- a/apps/api/src/mcp/server.test.ts +++ b/apps/api/src/mcp/server.test.ts @@ -67,11 +67,11 @@ describe("MCP server", () => { expect(client.getProtocolEra()).toBe("modern"); const result = await client.listTools(); - expect(result.tools).toHaveLength(108); + expect(result.tools).toHaveLength(109); expect(result.tools.map((tool) => tool.name).sort()).toEqual( Object.keys(listMcpToolPolicies()).sort(), ); - expect(new Set(result.tools.map((tool) => tool.name)).size).toBe(108); + expect(new Set(result.tools.map((tool) => tool.name)).size).toBe(109); expect(result.ttlMs).toBe(300_000); expect(result.cacheScope).toBe("private"); for (const tool of result.tools) { diff --git a/apps/api/src/mcp/tools/esp.ts b/apps/api/src/mcp/tools/esp.ts index 0bb7a311..18faec4c 100644 --- a/apps/api/src/mcp/tools/esp.ts +++ b/apps/api/src/mcp/tools/esp.ts @@ -6,6 +6,7 @@ import { import { z } from "zod"; import { invalidateEspTransport, invalidateTeamTransport } from "../../mail/transport"; import { warmupState } from "../../mail/warmup"; +import { espDeleteBlockerMessage } from "../../settings/esp/delete-blockers"; import { checkAndRememberDomainAuthentication, createEspConfig, @@ -145,7 +146,7 @@ export function registerEspTools(server: McpToolRegistrar): void { "delete_esp_config", { description: - "Removes the team's default ESP configuration. If another user ESP exists, it's promoted to default; otherwise campaign/transactional sends fail until a new ESP is configured.", + "Legacy: removes the team's first ESP configuration. A live team default can't be removed (make another source the default, or retire it with retire_esp, first), and an ESP with delivery history or queued work is never deleted.", outputSchema: successMessageSchema, annotations: { readOnlyHint: false, @@ -159,11 +160,7 @@ export function registerEspTools(server: McpToolRegistrar): void { try { await deleteEspConfig(teamId); } catch (err: any) { - return errorResult( - err.message === "esp_in_use" - ? "ESP is in use by an active sequence or a queued transactional email and cannot be removed." - : err.message, - ); + return errorResult(espDeleteBlockerMessage(err.message) ?? err.message); } invalidateTeamTransport(teamId); return jsonResult({ message: "ESP configuration removed." }); @@ -358,7 +355,7 @@ export function registerEspTools(server: McpToolRegistrar): void { "delete_esp", { description: - "Removes an ESP that has no active or queued delivery work. Draft, paused, and completed sequences are detached first.", + "Deletes an ESP that has never carried delivery history. Blocked when it is the team's live default, is used by a sending sequence or broadcast or by queued email, or has transactional email, delivery feedback or grants (retire it with retire_esp instead). Draft, paused and completed sequences are detached first; sent-mail history is kept.", inputSchema: { espId: z.string().min(1) }, outputSchema: successMessageSchema, annotations: { @@ -375,11 +372,7 @@ export function registerEspTools(server: McpToolRegistrar): void { try { await deleteEspConfig(teamId, args.espId); } catch (err: any) { - return errorResult( - err.message === "delivery_source_in_use" - ? "ESP is in use and cannot be removed." - : err.message, - ); + return errorResult(espDeleteBlockerMessage(err.message) ?? err.message); } invalidateEspTransport(teamId, config.id); return jsonResult({ message: "ESP configuration removed." }); @@ -416,6 +409,36 @@ export function registerEspTools(server: McpToolRegistrar): void { }, ); + server.registerTool( + "retire_esp", + { + description: + "Retires a team-owned ESP (active, suspended or draining) so it no longer sends. This is how an ESP with delivery history is archived; its history stays intact. A retired ESP can be deleted with delete_esp only if it has no history.", + inputSchema: { espId: z.string().min(1) }, + outputSchema: espConfigSchema, + annotations: { + readOnlyHint: false, + destructiveHint: true, + openWorldHint: false, + }, + }, + async (args: any, extra: any) => { + const teamId = getTeamId(extra); + if (!teamId) return AUTH_ERROR; + const config = await getEspConfigByEspId(teamId, args.espId); + if (!config) return NOT_FOUND; + try { + const retired = await transitionEspConfig(config, "retire", { cancel: true }); + invalidateEspTransport(teamId, config.id); + return jsonResult(toPublicShape(retired)); + } catch { + return errorResult( + "Only an active, suspended or draining ESP can be retired. Delete a draft ESP instead.", + ); + } + }, + ); + server.registerTool( "test_esp", { diff --git a/apps/api/src/mcp/tools/tools.test.ts b/apps/api/src/mcp/tools/tools.test.ts index 0da7c488..0da94feb 100644 --- a/apps/api/src/mcp/tools/tools.test.ts +++ b/apps/api/src/mcp/tools/tools.test.ts @@ -724,7 +724,37 @@ describe("MCP ESP tools", () => { const result = await tools.get("delete_esp")!.handler({ espId: "esp_2" }, auth); expect(result.isError).toBe(true); - expect(result.content[0].text).toContain("in use"); + expect(result.content[0].text).toContain("queued email"); + }); + + it("explains a blocked delete of the team's default ESP", async () => { + const tools = makeToolRegistry(registerEspTools); + mocks.getEspConfigByEspId.mockResolvedValue({ id: "internal-2", espId: "esp_2" }); + mocks.deleteEspConfig.mockRejectedValue(new Error("esp_is_default")); + + const result = await tools.get("delete_esp")!.handler({ espId: "esp_2" }, auth); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("default delivery source"); + expect(mocks.invalidateEspTransport).not.toHaveBeenCalled(); + }); + + it("retires a team ESP so it stops sending", async () => { + const tools = makeToolRegistry(registerEspTools); + const config = { id: "internal-2", espId: "esp_2", name: "Old", status: "active" }; + mocks.getEspConfigByEspId.mockResolvedValue(config); + mocks.transitionEspConfig.mockResolvedValue({ ...config, status: "retired" }); + + const result = await tools.get("retire_esp")!.handler({ espId: "esp_2" }, auth); + + expect(mocks.transitionEspConfig).toHaveBeenCalledWith(config, "retire", { + cancel: true, + }); + expect(mocks.invalidateEspTransport).toHaveBeenCalledWith("team-1", "internal-2"); + expect(result.structuredContent).toMatchObject({ + espId: "esp_2", + status: "retired", + }); }); it("tests a specific ESP by espId", async () => { diff --git a/apps/api/src/observability/instance.ts b/apps/api/src/observability/instance.ts index 2c193576..a332cde1 100644 --- a/apps/api/src/observability/instance.ts +++ b/apps/api/src/observability/instance.ts @@ -79,6 +79,8 @@ const PROPERTY_ALLOWLIST = new Set([ "media_deleted_count", // Bulk contact deletion (contacts/bulk-delete.ts): counts only. "suppressed_count", + // ESP deletion (settings/esp, organization ESPs): public esp_ id. + "esp_id", ]); /** Allowlisted keys that hold opaque record ids. They skip the package's @@ -96,6 +98,7 @@ const IDENTIFIER_KEYS = new Set([ "organization_id", "form_id", "txe_id", + "esp_id", ]); /** diff --git a/apps/api/src/organization/routes.ts b/apps/api/src/organization/routes.ts index 6b65f3be..e757acec 100644 --- a/apps/api/src/organization/routes.ts +++ b/apps/api/src/organization/routes.ts @@ -40,6 +40,8 @@ import { toPublicFeedbackConnection, } from "../delivery-feedback/feedback-connection-shape"; import { warmupState } from "../mail/warmup"; +import { captureEvent } from "../observability"; +import { isEspDeleteBlocker } from "../settings/esp/delete-blockers"; import { canActivateForOrganization, requiresFeedbackForOrganization, @@ -1233,13 +1235,27 @@ const impl = s.router(contract.organizations, { "organization_esp.deleted", { metadata: { espId: config.espId } }, ); + captureEvent({ + event: "organization_esp_deleted", + source: "organizations.esps.delete", + properties: { + esp_id: config.espId, + organization_id: authorization.organization.id, + }, + }); return { status: 204, body: undefined }; } catch (error: any) { - if (error.message === "delivery_source_in_use") { - return { - status: 409, - body: { error: "delivery_source_in_use" }, - }; + if (isEspDeleteBlocker(error.message)) { + captureEvent({ + event: "esp_config_delete_blocked", + source: "organizations.esps.delete", + properties: { + esp_id: config.espId, + organization_id: authorization.organization.id, + reason: error.message, + }, + }); + return { status: 409, body: { error: error.message } }; } throw error; } diff --git a/apps/api/src/settings/esp/delete-blockers.ts b/apps/api/src/settings/esp/delete-blockers.ts new file mode 100644 index 00000000..18baa298 --- /dev/null +++ b/apps/api/src/settings/esp/delete-blockers.ts @@ -0,0 +1,31 @@ +/** + * Why an ESP can't be physically deleted. Each code tells the caller what to + * do instead, so the REST API, MCP and dashboard explain it the same way. + */ +export const espDeleteBlockerMessages = { + // The team's live default delivery source. + esp_is_default: + "This ESP is the team's default delivery source. Make another source the default, or retire this ESP, before deleting it.", + // An organization ESP that has been activated is retired, never deleted, + // so grants and reports keep their source. + esp_retire_instead: + "A shared ESP that has been activated can't be deleted. Retire it instead; a retired ESP can't send, and its reports stay intact.", + // Grants (usage history), a delivery-feedback connection (delivery events) + // or transactional email point at it, and none of that may be lost. + esp_has_delivery_history: + "This ESP has delivery history (transactional email, delivery feedback, or team assignments), so it can't be deleted. Retire it instead; a retired ESP can't send, and its history stays intact.", + // A sending sequence or broadcast, or queued email, still needs it. + delivery_source_in_use: + "This ESP is used by a sending sequence or broadcast, or by queued email. Pause or finish that work, or wait for the queue to drain, then try again.", +} as const; + +export type EspDeleteBlocker = keyof typeof espDeleteBlockerMessages; + +export function isEspDeleteBlocker(code: unknown): code is EspDeleteBlocker { + return typeof code === "string" && Object.hasOwn(espDeleteBlockerMessages, code); +} + +/** The explanation for a delete blocker, or `undefined` for any other error. */ +export function espDeleteBlockerMessage(code: unknown): string | undefined { + return isEspDeleteBlocker(code) ? espDeleteBlockerMessages[code] : undefined; +} diff --git a/apps/api/src/settings/esp/queries.test.ts b/apps/api/src/settings/esp/queries.test.ts index d27d915b..7b2af58d 100644 --- a/apps/api/src/settings/esp/queries.test.ts +++ b/apps/api/src/settings/esp/queries.test.ts @@ -7,19 +7,26 @@ mock.module("../../db/client", async () => { }); const { db } = await import("../../db/client"); -const { espConfigs, outboundMessages, sequences, teamDeliverySettings } = await import( - "../../db/schema" -); +const { + espConfigs, + outboundMessages, + sequences, + teamDeliverySettings, + transactionalEmails, +} = await import("../../db/schema"); const { seedTeamAndContact, truncateAll } = await import("../../test/db"); type TestDb = import("../../test/db").TestDb; const { seedSequence } = await import("../../test/fixtures"); const { createEspConfig, + createOrganizationEspConfig, deleteEspConfig, + deleteOrganizationEspConfig, getDecryptedEspCredentials, getEspConfigByEspId, listEspConfigs, recordEspTestResult, + transitionEspConfig, upsertEspConfig, checkAndRememberDomainAuthentication, } = await import("./queries"); @@ -103,26 +110,152 @@ describe("ESP config queries", () => { await expect(getEspConfigByEspId(team.id, draft.espId)).resolves.toBeNull(); }); - it("deletes an unused active default and clears its delivery selection", async () => { + it("refuses to delete the team's live default", async () => { const { team } = await seedTeamAndContact(tdb); const [active] = await listEspConfigs(team.id); - await expect(deleteEspConfig(team.id, active.espId)).resolves.toBe(true); - await expect(getEspConfigByEspId(team.id, active.espId)).resolves.toBeNull(); + await expect(deleteEspConfig(team.id, active.espId)).rejects.toThrow( + "esp_is_default", + ); + await expect(getEspConfigByEspId(team.id, active.espId)).resolves.not.toBeNull(); + }); + + it("deletes a retired default and moves the default to another active ESP", async () => { + const { team } = await seedTeamAndContact(tdb); + const [retired] = await listEspConfigs(team.id); + await tdb + .update(espConfigs) + .set({ status: "retired", retiredAt: new Date() }) + .where(eq(espConfigs.id, retired.id)); + const backup = await createEspConfig(team.id, { + name: "Backup", + provider: "smtp", + host: "backup.example.com", + port: 587, + secure: false, + }); + await tdb + .update(espConfigs) + .set({ status: "active" }) + .where(eq(espConfigs.id, backup.id)); + + await expect(deleteEspConfig(team.id, retired.espId)).resolves.toBe(true); const [delivery] = await tdb .select() .from(teamDeliverySettings) .where(eq(teamDeliverySettings.teamId, team.id)); expect(delivery).toMatchObject({ - defaultSource: null, - defaultTeamEspConfigId: null, + defaultSource: "team", + defaultTeamEspConfigId: backup.id, + }); + }); + + it("keeps an ESP with transactional history and asks for retirement", async () => { + const { team } = await seedTeamAndContact(tdb); + const esp = await createEspConfig(team.id, { + name: "Transactional", + provider: "smtp", + host: "tx.example.com", + port: 587, + secure: false, + }); + await tdb.insert(transactionalEmails).values({ + teamId: team.id, + deliverySourceType: "team", + outboxId: esp.id, + toEmail: "recipient@example.com", + subject: "Receipt", + status: "sent", }); + + await expect(deleteEspConfig(team.id, esp.espId)).rejects.toThrow( + "esp_has_delivery_history", + ); + await expect(getEspConfigByEspId(team.id, esp.espId)).resolves.not.toBeNull(); + }); + + it("blocks a delete while transactional email is queued on the ESP", async () => { + const { team } = await seedTeamAndContact(tdb); + const esp = await createEspConfig(team.id, { + name: "Queued", + provider: "smtp", + host: "queued.example.com", + port: 587, + secure: false, + }); + await tdb.insert(transactionalEmails).values({ + teamId: team.id, + deliverySourceType: "team", + outboxId: esp.id, + toEmail: "recipient@example.com", + subject: "Receipt", + status: "queued", + }); + + await expect(deleteEspConfig(team.id, esp.espId)).rejects.toThrow( + "delivery_source_in_use", + ); + }); + + it("deletes a never-activated organization draft but not an activated one", async () => { + const { organization } = await seedTeamAndContact(tdb); + const draft = await createOrganizationEspConfig(organization.id, { + name: "Draft", + provider: "smtp", + host: "org-draft.example.com", + port: 587, + secure: false, + }); + const retired = await createOrganizationEspConfig(organization.id, { + name: "Retired", + provider: "smtp", + host: "org-retired.example.com", + port: 587, + secure: false, + }); + await tdb + .update(espConfigs) + .set({ status: "retired", activatedAt: new Date(), retiredAt: new Date() }) + .where(eq(espConfigs.id, retired.id)); + + await expect( + deleteOrganizationEspConfig(organization.id, draft.espId), + ).resolves.toBe(true); + await expect( + deleteOrganizationEspConfig(organization.id, retired.espId), + ).rejects.toThrow("esp_retire_instead"); + }); + + it("retires a once-active draft but not a never-activated one", async () => { + const { team } = await seedTeamAndContact(tdb); + const fresh = await createEspConfig(team.id, { + name: "Fresh", + provider: "smtp", + host: "fresh.example.com", + port: 587, + secure: false, + }); + await expect( + transitionEspConfig(fresh, "retire", { cancel: true }), + ).rejects.toThrow("invalid_lifecycle_transition"); + const [edited] = await tdb + .update(espConfigs) + .set({ activatedAt: new Date() }) + .where(eq(espConfigs.id, fresh.id)) + .returning(); + await expect( + transitionEspConfig(edited, "retire", { cancel: true }), + ).resolves.toMatchObject({ status: "retired" }); }); it("deletes an ESP pinned only to draft, paused, or completed sequences", async () => { const { team } = await seedTeamAndContact(tdb); const [active] = await listEspConfigs(team.id); + await tdb + .update(teamDeliverySettings) + .set({ defaultSource: null, defaultTeamEspConfigId: null }) + .where(eq(teamDeliverySettings.teamId, team.id)); const draft = await seedSequence(tdb, { teamId: team.id, status: "draft", diff --git a/apps/api/src/settings/esp/queries.ts b/apps/api/src/settings/esp/queries.ts index 7132e327..0f4d7e61 100644 --- a/apps/api/src/settings/esp/queries.ts +++ b/apps/api/src/settings/esp/queries.ts @@ -352,27 +352,30 @@ export async function upsertEspConfig( } async function deleteOwnedEspConfig(config: EspConfig): Promise { - // Organization ESPs retain their stricter, never-activated delete rule. - // A team can remove an otherwise-unused active ESP from Settings; its - // delivery default is cleared (or moved to another active team ESP) - // atomically before the foreign-key-protected row is removed. + // Organization ESPs keep their stricter rule: only a never-activated + // draft is deleted. Anything that has been live is retired instead. if ( config.ownerScope === "organization" && (config.status !== "draft" || config.activatedAt) ) { - throw new Error("delivery_source_in_use"); + throw new Error("esp_retire_instead"); } return db.transaction(async (tx) => { - const [grant] = await tx - .select({ id: espConfigTeamGrants.id }) - .from(espConfigTeamGrants) - .where(eq(espConfigTeamGrants.espConfigId, config.id)) - .limit(1); - const [feedback] = await tx - .select({ id: espFeedbackConnections.id }) - .from(espFeedbackConnections) - .where(eq(espFeedbackConnections.espConfigId, config.id)) - .limit(1); + // A live team default can't vanish from under the team's sends. A + // retired one no longer sends, so deleting it just clears the pointer. + if (config.ownerScope === "team" && config.teamId && config.status !== "retired") { + const [defaultFor] = await tx + .select({ teamId: teamDeliverySettings.teamId }) + .from(teamDeliverySettings) + .where( + and( + eq(teamDeliverySettings.teamId, config.teamId), + eq(teamDeliverySettings.defaultTeamEspConfigId, config.id), + ), + ) + .limit(1); + if (defaultFor) throw new Error("esp_is_default"); + } const [activeSequence] = await tx .select({ id: sequences.id }) .from(sequences) @@ -383,10 +386,15 @@ async function deleteOwnedEspConfig(config: EspConfig): Promise { ), ) .limit(1); - const [transactional] = await tx + const [queuedTransactional] = await tx .select({ id: transactionalEmails.id }) .from(transactionalEmails) - .where(eq(transactionalEmails.outboxId, config.id)) + .where( + and( + eq(transactionalEmails.outboxId, config.id), + eq(transactionalEmails.status, "queued"), + ), + ) .limit(1); const [queuedOutbound] = await tx .select({ id: outboundMessages.id }) @@ -398,9 +406,30 @@ async function deleteOwnedEspConfig(config: EspConfig): Promise { ), ) .limit(1); - if (grant || feedback || activeSequence || transactional || queuedOutbound) { + if (activeSequence || queuedTransactional || queuedOutbound) { throw new Error("delivery_source_in_use"); } + // Grants carry usage history, feedback connections own delivery events + // (which cascade), and transactional email must keep its source. None + // of these may be lost, so such an ESP is retired, not deleted. + const [grant] = await tx + .select({ id: espConfigTeamGrants.id }) + .from(espConfigTeamGrants) + .where(eq(espConfigTeamGrants.espConfigId, config.id)) + .limit(1); + const [feedback] = await tx + .select({ id: espFeedbackConnections.id }) + .from(espFeedbackConnections) + .where(eq(espFeedbackConnections.espConfigId, config.id)) + .limit(1); + const [transactional] = await tx + .select({ id: transactionalEmails.id }) + .from(transactionalEmails) + .where(eq(transactionalEmails.outboxId, config.id)) + .limit(1); + if (grant || feedback || transactional) { + throw new Error("esp_has_delivery_history"); + } // Draft and paused sequences resolve a fresh source when started or // resumed, and completed sequences no longer send. Remove their old @@ -615,10 +644,13 @@ export async function transitionEspConfig( if (config.status !== "suspended") throw new Error("invalid_lifecycle_transition"); values.status = "active"; } else { - if ( - !["active", "suspended", "draining"].includes(config.status) || - config.status === "retired" - ) { + // An ESP that was once live and went back to draft (a credential + // change does that) may still carry history, so it can be retired too. + // A never-activated draft is deleted instead. + const retirable = + ["active", "suspended", "draining"].includes(config.status) || + (config.status === "draft" && config.activatedAt !== null); + if (!retirable) { throw new Error("invalid_lifecycle_transition"); } if (options?.cancel) { diff --git a/apps/api/src/settings/esp/routes.test.ts b/apps/api/src/settings/esp/routes.test.ts index d09219d3..47285b98 100644 --- a/apps/api/src/settings/esp/routes.test.ts +++ b/apps/api/src/settings/esp/routes.test.ts @@ -35,7 +35,8 @@ mock.module("../../mail/transport", () => ({ mock.module("./test", () => ({ testEspConfig: vi.fn().mockResolvedValue({ success: true }), })); -mock.module("../../observability", () => ({ captureEvent: vi.fn() })); +const captureEvent = vi.fn(); +mock.module("../../observability", () => ({ captureEvent })); const { db } = await import("../../db/client"); const { espConfigs, outboundMessages } = await import("../../db/schema"); @@ -147,6 +148,9 @@ describe("ESP settings routes", () => { }); expect(response.status).toBe(409); + expect((await response.json()) as unknown).toEqual({ + error: "delivery_source_in_use", + }); expect(transportMocks.invalidateEsp).not.toHaveBeenCalled(); const [stillPresent] = await tdb .select() @@ -155,6 +159,32 @@ describe("ESP settings routes", () => { expect(stillPresent).toBeTruthy(); }); + it("explains why the team's live default can't be deleted (#12)", async () => { + const owner = await seedTeamAndContact(tdb); + const [seeded] = await tdb + .select() + .from(espConfigs) + .where(eq(espConfigs.teamId, owner.team.id)); + requestContext.teamId = owner.team.id; + requestContext.userId = owner.account.id; + + const response = await requestApp(app(), `/settings/esps/${seeded.espId}`, { + method: "DELETE", + }); + + expect(response.status).toBe(409); + expect((await response.json()) as unknown).toEqual({ error: "esp_is_default" }); + expect(captureEvent).toHaveBeenCalledWith( + expect.objectContaining({ + event: "esp_config_delete_blocked", + properties: { esp_id: seeded.espId, reason: "esp_is_default" }, + }), + ); + await expect( + getEspConfigByEspId(owner.team.id, seeded.espId), + ).resolves.not.toBeNull(); + }); + it("deletes an ESP used only by a completed broadcast", async () => { const owner = await seedTeamAndContact(tdb); const config = await createEspConfig(owner.team.id, { diff --git a/apps/api/src/settings/esp/routes.ts b/apps/api/src/settings/esp/routes.ts index b32c6c17..6dbcc1ad 100644 --- a/apps/api/src/settings/esp/routes.ts +++ b/apps/api/src/settings/esp/routes.ts @@ -9,7 +9,7 @@ import { invalidateEspTransport, invalidateTeamTransport } from "../../mail/tran import { effectiveDailySendLimit, warmupState } from "../../mail/warmup"; import { captureEvent } from "../../observability"; import { getTeamMembership } from "../../team/queries"; - +import { isEspDeleteBlocker } from "./delete-blockers"; import { checkAndRememberDomainAuthentication, createEspConfig, @@ -169,11 +169,14 @@ const collectionImpl = s.router(contract.settings.esps, { try { await deleteEspConfig(teamId, params.espId); } catch (err: any) { - if (err.message === "delivery_source_in_use") { - return { - status: 409, - body: { error: "delivery_source_in_use" }, - }; + if (isEspDeleteBlocker(err.message)) { + captureEvent({ + event: "esp_config_delete_blocked", + source: "settings.esps.remove", + teamId, + properties: { esp_id: config.espId, reason: err.message }, + }); + return { status: 409, body: { error: err.message } }; } throw err; } diff --git a/packages/api-contract/src/contract.ts b/packages/api-contract/src/contract.ts index 18cb3902..56eea6ea 100644 --- a/packages/api-contract/src/contract.ts +++ b/packages/api-contract/src/contract.ts @@ -907,7 +907,7 @@ const espCollectionContract = c.router( }, summary: "Remove a user-managed ESP configuration", description: - "Removes an ESP with no active or queued dependencies. Draft, paused, and completed sequences are detached first.", + "Deletes an ESP that has never carried delivery history; delivery history is never deleted. Draft, paused, and completed sequences are detached first. 409 explains a blocked delete: `esp_is_default` (the team's live default; pick another default or retire it first), `delivery_source_in_use` (a sending sequence or broadcast, or queued email, needs it), or `esp_has_delivery_history` (transactional email, delivery feedback, or grants point at it; retire it instead).", }, domainAuthentication: { method: "GET", @@ -1634,6 +1634,8 @@ const organizationsContract = c.router( 409: errorSchema, }, summary: "Delete an eligible never-activated organization ESP", + description: + "Only a never-activated draft with no history is deleted. 409 explains a blocked delete: `esp_retire_instead` (it has been activated; retire it instead), `esp_has_delivery_history`, or `delivery_source_in_use`.", }, getEspFeedback: { method: "GET", diff --git a/packages/api-contract/src/mcp-parity.ts b/packages/api-contract/src/mcp-parity.ts index 19c2e30b..a545be84 100644 --- a/packages/api-contract/src/mcp-parity.ts +++ b/packages/api-contract/src/mcp-parity.ts @@ -80,7 +80,7 @@ const ORGANIZATION_KEYS = const PROVISIONING = "Team provisioning authenticates with organization API keys; MCP is team-scoped and does not accept organization keys."; const ESP_LIFECYCLE = - "ESP suspend, resume and retire are operational lifecycle controls kept REST/dashboard-only; MCP exposes create, activate, test and delete."; + "ESP suspend and resume are operational lifecycle controls kept REST/dashboard-only; MCP exposes create, activate, test, retire and delete."; const DELIVERY_SELECTION = "Team delivery-source selection (shared organization delivery vs user ESPs) is REST/dashboard-only; MCP manages user ESP configurations directly."; const DEFAULT_ESP_ALIAS = @@ -202,7 +202,7 @@ export const mcpParityManifest: readonly McpParityManifestEntry[] = [ required("settings.esps.activate", "activate_esp", "write"), restOnly("settings.esps.suspend", ESP_LIFECYCLE), restOnly("settings.esps.resume", ESP_LIFECYCLE), - restOnly("settings.esps.retire", ESP_LIFECYCLE), + required("settings.esps.retire", "retire_esp", "destructive"), mcpOnly("get_esp_config", DEFAULT_ESP_ALIAS), mcpOnly("update_esp_config", DEFAULT_ESP_ALIAS), mcpOnly("delete_esp_config", DEFAULT_ESP_ALIAS), From 04462caaabadac85a853af492711f3d780e87797 Mon Sep 17 00:00:00 2001 From: Rajat Saxena Date: Thu, 29 Oct 2026 16:24:26 +0530 Subject: [PATCH 2/3] fix(web): make ESP delete work from settings and the organization page (#12) Settings keeps the confirm dialog open with the reason a delete is blocked and adds Retire. The organization page offered Retire for drafts and Delete for retired mailboxes, both of which the API always refused; it now offers Delete for never-activated drafts and Retire otherwise. --- .../(dashboard)/organizations/page.test.tsx | 88 +++++++++++++- .../app/(dashboard)/organizations/page.tsx | 108 ++++++++++-------- apps/web/app/(dashboard)/settings/page.tsx | 69 ++++++++++- apps/web/lib/api.ts | 7 ++ apps/web/lib/delivery-source.test.ts | 12 +- apps/web/lib/delivery-source.ts | 8 +- 6 files changed, 236 insertions(+), 56 deletions(-) diff --git a/apps/web/app/(dashboard)/organizations/page.test.tsx b/apps/web/app/(dashboard)/organizations/page.test.tsx index 4e36d860..c6c4a1d5 100644 --- a/apps/web/app/(dashboard)/organizations/page.test.tsx +++ b/apps/web/app/(dashboard)/organizations/page.test.tsx @@ -30,6 +30,7 @@ const mocks = { createOrganization: mock(), abandonPendingOrganization: mock(), deleteOrganization: mock(), + deleteOrganizationEsp: mock(), clearOrganizationIdCookie: mock(), clearTeamIdCookie: mock(), navigateTo: mock(), @@ -98,7 +99,7 @@ mock.module("@/lib/api", () => ({ archiveOrganizationTeam: mock(), createOrganizationEsp: mock(), updateOrganizationEsp: mock(), - deleteOrganizationEsp: mock(), + deleteOrganizationEsp: mocks.deleteOrganizationEsp, testOrganizationEsp: mock(), activateOrganizationEsp: mock(), suspendOrganizationEsp: mock(), @@ -896,3 +897,88 @@ describe("organization danger zone", () => { ); }); }); + +describe("shared mailbox delete (#12)", () => { + const esp = (overrides: Record) => ({ + espId: "esp_x", + name: "Mailbox", + provider: "smtp", + host: "smtp.example.com", + port: 587, + secure: false, + username: null, + hasPassword: false, + fromName: null, + fromEmail: "a@example.com", + lastTestedAt: null, + lastTestStatus: null, + lastTestError: null, + status: "draft", + secretVersion: 1, + activatedAt: null, + drainUntil: null, + retiredAt: null, + updatedAt: "2026-08-01T00:00:00.000Z", + ...overrides, + }); + + async function openMenu(name: string) { + const trigger = await screen.findByRole("button", { + name: `Mailbox actions for ${name}`, + }); + fireEvent.pointerDown(trigger, { button: 0, ctrlKey: false, pointerType: "mouse" }); + return screen.findByRole("menu"); + } + + beforeEach(() => { + mocks.listOrganizationEsps.mockResolvedValue({ + items: [ + esp({ espId: "esp_draft", name: "Draft box" }), + esp({ + espId: "esp_live", + name: "Live box", + status: "active", + activatedAt: "2026-08-01T00:00:00.000Z", + }), + esp({ + espId: "esp_old", + name: "Old box", + status: "retired", + activatedAt: "2026-08-01T00:00:00.000Z", + retiredAt: "2026-08-02T00:00:00.000Z", + }), + ], + }); + }); + + it("deletes a never-activated draft and retires anything that has been live", async () => { + mocks.deleteOrganizationEsp.mockResolvedValue(undefined); + renderPage(); + await openTab("Delivery"); + + const draftMenu = await openMenu("Draft box"); + expect(within(draftMenu).queryByRole("menuitem", { name: /Retire/ })).toBeNull(); + fireEvent.click( + within(draftMenu).getByRole("menuitem", { name: /Delete shared ESP/ }), + ); + const dialog = await screen.findByRole("alertdialog"); + await act(async () => { + fireEvent.click( + within(dialog).getByRole("button", { name: "Delete shared ESP" }), + ); + }); + await waitFor(() => + expect(mocks.deleteOrganizationEsp).toHaveBeenCalledWith("org_1", "esp_draft"), + ); + + const liveMenu = await openMenu("Live box"); + expect( + within(liveMenu).getByRole("menuitem", { name: /Retire shared ESP/ }), + ).toBeTruthy(); + expect(within(liveMenu).queryByRole("menuitem", { name: /Delete/ })).toBeNull(); + + expect( + screen.queryByRole("button", { name: "Mailbox actions for Old box" }), + ).toBeNull(); + }); +}); diff --git a/apps/web/app/(dashboard)/organizations/page.tsx b/apps/web/app/(dashboard)/organizations/page.tsx index b657238b..4ee1fe02 100644 --- a/apps/web/app/(dashboard)/organizations/page.tsx +++ b/apps/web/app/(dashboard)/organizations/page.tsx @@ -220,7 +220,11 @@ const ORGANIZATION_ERROR_MESSAGES: Record = { delivery_policy_not_found: "Delivery settings are not available for this organization.", delivery_source_in_use: - "This mailbox is still assigned to a team. Remove its assignments and try again.", + "This mailbox is still in use by sending campaigns or queued email. Let that work finish, or retire the mailbox to cancel it, and try again.", + esp_has_delivery_history: + "This mailbox has delivery history, so it stays retired to keep reports intact. A retired mailbox can't send.", + esp_retire_instead: + "A mailbox that has been activated can't be deleted. Retire it instead; it stops sending and its reports stay intact.", domain_exists: "That sending domain has already been added.", domain_invalid: "Enter a valid domain, such as example.com.", domain_not_found: "That sending domain could not be found.", @@ -2015,54 +2019,59 @@ function SharedEspsSection({ )} - - - - - - - - {esp.status !== "retired" && esp.status !== "draining" && ( - void transition(esp)} - > - - {transitioningId === esp.espId - ? "Working…" - : esp.status === "active" - ? "Suspend" - : esp.status === "suspended" - ? "Resume" - : "Activate"} - - )} - - {esp.status !== "retired" ? ( - setRetiringEsp(esp)} + {esp.status !== "retired" && ( + + + - - Retire shared ESP - - ) : ( - setDeletingEsp(esp)} - > - - Delete shared ESP - - )} - - + + + + + {esp.status !== "draining" && ( + void transition(esp)} + > + + {transitioningId === esp.espId + ? "Working…" + : esp.status === "active" + ? "Suspend" + : esp.status === "suspended" + ? "Resume" + : "Activate"} + + )} + {/* A never-activated draft is deleted; anything that + has been live is retired and kept for its history + (#12). A retired mailbox has nothing left to do. */} + + {esp.status === "draft" && !esp.activatedAt ? ( + setDeletingEsp(esp)} + > + + Delete shared ESP + + ) : ( + setRetiringEsp(esp)} + > + + Retire shared ESP + + )} + + + )} @@ -2107,7 +2116,8 @@ function SharedEspsSection({ Delete {deletingEsp?.name}? - This permanently removes the retired shared ESP. It cannot be restored. + This permanently removes the draft shared ESP. It has never been + activated, so no team or report depends on it. It cannot be restored. diff --git a/apps/web/app/(dashboard)/settings/page.tsx b/apps/web/app/(dashboard)/settings/page.tsx index 9d3e91cd..7a7b4f8c 100644 --- a/apps/web/app/(dashboard)/settings/page.tsx +++ b/apps/web/app/(dashboard)/settings/page.tsx @@ -1,6 +1,7 @@ "use client"; import { + Archive, CheckCircle2, Copy, Mail, @@ -26,6 +27,16 @@ import { ScrollablePage } from "@/components/dashboard/scrollable-page"; import { SendErrorMessage } from "@/components/dashboard/send-error-message"; import { TrackingDomainSettings } from "@/components/dashboard/tracking-domain-settings"; import { WebArchiveSettings } from "@/components/dashboard/web-archive-settings"; +import { + AlertDialog, + AlertDialogAction, + AlertDialogCancel, + AlertDialogContent, + AlertDialogDescription, + AlertDialogFooter, + AlertDialogHeader, + AlertDialogTitle, +} from "@/components/ui/alert-dialog"; import { Badge } from "@/components/ui/badge"; import { Card, @@ -62,6 +73,7 @@ import { getTeamDeliverySettings, listEsps, listTeamKeys, + retireEsp, setTeamEspAsDefault, testEsp, updateEsp, @@ -99,6 +111,7 @@ export default function SettingsPage() { const [espFormOpen, setEspFormOpen] = useState(false); const [editingEsp, setEditingEsp] = useState(null); const [espPendingDelete, setEspPendingDelete] = useState(null); + const [espPendingRetire, setEspPendingRetire] = useState(null); const [feedbackEsp, setFeedbackEsp] = useState(null); const [domainAuthEsp, setDomainAuthEsp] = useState(null); const [testingEspId, setTestingEspId] = useState(null); @@ -199,16 +212,24 @@ export default function SettingsPage() { } } + // A blocked delete throws, so the confirmation dialog stays open and says + // why (#12) instead of closing with the reason far up the page. async function handleDelete(esp: EspConfig) { + setError(null); + await deleteEsp(esp.espId); + await load(); + } + + async function handleRetire(esp: EspConfig) { setError(null); try { - await deleteEsp(esp.espId); + await retireEsp(esp.espId); await load(); } catch (err) { setError( err instanceof ApiError ? presentDeliverySourceError(err.message) - : "Failed to remove ESP", + : "Failed to retire ESP", ); } } @@ -294,11 +315,44 @@ export default function SettingsPage() { title="Remove email service provider?" description={`This will permanently remove "${espPendingDelete?.name ?? "this email service provider"}". Sending through it will no longer be available.`} confirmLabel="Remove ESP" + errorMessage={(failure) => + failure instanceof ApiError + ? presentDeliverySourceError(failure.message) + : "Failed to remove ESP. Please try again." + } onConfirm={async () => { if (!espPendingDelete) return; await handleDelete(espPendingDelete); }} /> + { + if (!open) setEspPendingRetire(null); + }} + > + + + Retire {espPendingRetire?.name}? + + It stops sending now and can't be reactivated. Its delivery history and + reports stay intact. Campaigns pinned to it need another delivery + source. + + + + Keep ESP + { + if (espPendingRetire) void handleRetire(espPendingRetire); + }} + > + Retire ESP + + + + { @@ -683,6 +737,17 @@ export default function SettingsPage() { > + {esp.status !== "retired" && + (esp.status !== "draft" || esp.activatedAt) && ( + setEspPendingRetire(esp)} + > + + + )} (client.settings.esps.activate({ params: { espId } })); } +/** Retires a team ESP now: it stops sending and keeps its delivery history. */ +export function retireEsp(espId: string) { + return unwrap( + client.settings.esps.retire({ params: { espId }, body: { transition: "cancel" } }), + ); +} + // ---- Bounce/complaint delivery feedback ------------------------------------ /** Providers with a reviewed webhook adapter — see diff --git a/apps/web/lib/delivery-source.test.ts b/apps/web/lib/delivery-source.test.ts index 521a980e..81abbcd6 100644 --- a/apps/web/lib/delivery-source.test.ts +++ b/apps/web/lib/delivery-source.test.ts @@ -8,10 +8,16 @@ import { } from "./delivery-source"; describe("presentDeliverySourceError", () => { - it("explains why an ESP cannot be removed", () => { - expect(presentDeliverySourceError("delivery_source_in_use")).toBe( - "This ESP can't be removed while it is used by an active sequence, queued email, or delivery integration.", + it("explains each reason an ESP cannot be removed (#12)", () => { + expect(presentDeliverySourceError("delivery_source_in_use")).toContain( + "sending sequence or broadcast", ); + expect(presentDeliverySourceError("esp_is_default")).toContain( + "Make another ESP the default", + ); + for (const code of ["esp_has_delivery_history", "esp_retire_instead"]) { + expect(presentDeliverySourceError(code)).toContain("Retire it instead"); + } }); it("explains paused and suspended sending instead of showing the code", () => { diff --git a/apps/web/lib/delivery-source.ts b/apps/web/lib/delivery-source.ts index 6817643f..56657f55 100644 --- a/apps/web/lib/delivery-source.ts +++ b/apps/web/lib/delivery-source.ts @@ -30,7 +30,13 @@ export function presentDeliverySourceError(error: string): string { case "organization_quota_exhausted": return "The organization's shared mailbox has reached its sending quota for now. Ask an organization admin to raise it, or wait for it to reset."; case "delivery_source_in_use": - return "This ESP can't be removed while it is used by an active sequence, queued email, or delivery integration."; + return "This ESP is used by a sending sequence or broadcast, or by queued email. Pause or finish that work, or wait for the queue to empty, then try again."; + case "esp_is_default": + return "This ESP is the team's default delivery source. Make another ESP the default, or retire this one, before removing it."; + case "esp_has_delivery_history": + return "This ESP has delivery history (transactional email, delivery feedback or shared-mailbox assignments), so it can't be removed. Retire it instead: it stops sending and its history stays intact."; + case "esp_retire_instead": + return "This ESP has been activated, so it can't be deleted. Retire it instead: it stops sending and its reports stay intact."; default: return error; } From 069b69ea75a4e90e6498558d5c5d2f9b07fa5a5a Mon Sep 17 00:00:00 2001 From: Rajat Saxena Date: Thu, 29 Oct 2026 16:24:27 +0530 Subject: [PATCH 3/3] docs(api): document ESP delete blockers and retire (#12) --- apps/api/README.md | 18 ++++++++++++++++++ apps/api/docs/organizations.md | 8 +++++++- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/apps/api/README.md b/apps/api/README.md index 616d9bbe..3da9661e 100644 --- a/apps/api/README.md +++ b/apps/api/README.md @@ -248,6 +248,24 @@ AES-256-GCM in `src/utils/secret-crypto.ts` and are never returned to clients. `/settings/esp` (singular) remains as a backward-compatible alias over the team's default user ESP. +Deleting an ESP never deletes delivery history. `DELETE /settings/esps/:espId` +(and MCP `delete_esp`) answers `409` with a reason code when the ESP can't go: + +- `esp_is_default`: it is the team's live default delivery source. Make another + source the default, or retire it (`POST /settings/esps/:espId/retire`, MCP + `retire_esp`) first. +- `delivery_source_in_use`: a sending sequence or broadcast, or queued email, + still needs it. +- `esp_has_delivery_history`: transactional email, a delivery-feedback + connection, or organization grants point at it. Retire it instead; a retired + ESP can't send and its history stays intact. + +Otherwise the ESP is deleted: draft, paused, and completed sequences are +detached from it, sent campaign messages keep their provider snapshot, and a +retired default is replaced by another active team ESP (or cleared). +Organization ESPs follow the same rules, and in addition only a never-activated +draft is deleted (`esp_retire_instead` otherwise). + Every sequence and transactional email persists an internal `deliveryRoute` (`custom` or `platform`) alongside the resolved user ESP's internal id (`outboxId`). `custom` is the only route currently reachable — it always diff --git a/apps/api/docs/organizations.md b/apps/api/docs/organizations.md index 5f82375a..15dc744e 100644 --- a/apps/api/docs/organizations.md +++ b/apps/api/docs/organizations.md @@ -845,7 +845,10 @@ Changing only the display name does not invalidate connection verification. `POST .../retire` performs retirement. `DELETE` performs physical deletion and is allowed only for a never-activated draft with no grant, feedback connection, -pinned work, outbound message, or audit reference. Historical FKs use +pinned work, outbound message, or audit reference. A blocked delete answers +`409` with the reason: `esp_retire_instead` (it has been activated; retire it), +`esp_has_delivery_history` (grants, feedback, or transactional email point at +it), or `delivery_source_in_use` (sending or queued work needs it). Historical FKs use `RESTRICT` or soft-retained references; they never cascade-delete delivery or feedback history. Operator-only organization/team purge workflows retire and then explicitly delete/anonymize dependants in retention order. @@ -2213,6 +2216,9 @@ content, or unredacted webhook credentials. | 404 | `esp_not_found` | Missing/foreign ESP in the relevant owner scope | | 409 | `provisioning_conflict` | External ID exists with conflicting immutable fields | | 409 | `delivery_source_in_use` | Unsafe source disable/delete | +| 409 | `esp_retire_instead` | Delete of an activated organization ESP; retire it instead | +| 409 | `esp_has_delivery_history` | Delete of an ESP with grants, feedback, or transactional mail | +| 409 | `esp_is_default` | Delete of a team's live default ESP | | 409 | `invalid_lifecycle_transition` | ESP/grant transition is not permitted | | 409 | `last_organization_owner` | Operation would leave no owner | | 422 | `delivery_source_required` | No unambiguous default |