From 129e413152f0b600e187f0d7310ded523357e13e Mon Sep 17 00:00:00 2001 From: askalf <263217947+askalf@users.noreply.github.com> Date: Wed, 23 Sep 2026 03:12:55 +0000 Subject: [PATCH 1/2] fix(mail): grade outbound delivery on the whole queue, not the top three reasons parseMailQueue capped its reason list at MAX_DEFERRALS before returning, and checkMailDelivery graded that capped list. A relay refusal ranked below the three most common reasons never reached gradeDelivery, so a relayed box whose SASL credentials, TLS or egress to the smarthost were broken graded warn instead of fail. Return every distinct reason from the parse, grade that, and apply the cap in topDeferrals when building the response. The response still carries at most three rows. --- .../src/modules/mail/mail-delivery.service.ts | 13 ++- .../mail/mail-delivery.service.test.ts | 101 +++++++++++++++++- 2 files changed, 111 insertions(+), 3 deletions(-) diff --git a/apps/api/src/modules/mail/mail-delivery.service.ts b/apps/api/src/modules/mail/mail-delivery.service.ts index dea13dea7..aaa160536 100644 --- a/apps/api/src/modules/mail/mail-delivery.service.ts +++ b/apps/api/src/modules/mail/mail-delivery.service.ts @@ -147,7 +147,12 @@ export async function checkMailDelivery(exec: CommandExecutor): Promise b[1] - a[1]) - .slice(0, MAX_DEFERRALS) .map(([reason, count]) => ({ kind: classifyReason(reason), count, reason })); return { queued, sampled, deferrals }; } +/** The deferral rows we report. `gradeDelivery` reads the uncapped list. */ +export function topDeferrals(deferrals: readonly MailDeferral[]): MailDeferral[] { + return deferrals.slice(0, MAX_DEFERRALS); +} + /** * The verdict. * diff --git a/apps/api/test/modules/mail/mail-delivery.service.test.ts b/apps/api/test/modules/mail/mail-delivery.service.test.ts index f27baa466..909b8dad0 100644 --- a/apps/api/test/modules/mail/mail-delivery.service.test.ts +++ b/apps/api/test/modules/mail/mail-delivery.service.test.ts @@ -5,6 +5,7 @@ import { describePath, gradeDelivery, parseMailQueue, + topDeferrals, type MailQueueReading, } from "../../../src/modules/mail/mail-delivery.service"; import { mailQueueProbeCommand } from "@repo/platform/engine/modules/mail/mail-engine"; @@ -51,6 +52,14 @@ const AUTH_REFUSAL = "host email-smtp.us-east-1.amazonaws.com[203.0.113.9] said: 535 Authentication Credentials Invalid (in reply to AUTH LOGIN command)"; const GREYLIST_REFUSAL = "host mx.receiver.example[198.51.100.4] said: 450 4.2.0 Greylisted, try again later (in reply to end of DATA command)"; +const RELAY_TLS_REFUSAL = + "host email-smtp.us-east-1.amazonaws.com[203.0.113.9]: certificate verification failed"; +const BUSY_RECEIVER = "host mx.b.example said: 451 4.3.0 Temporary local problem"; +const FULL_MAILBOX = "host mx.c.example said: 452 4.2.2 Mailbox full"; +const RELAY_NETWORK_REFUSAL = + "connect to email-smtp.us-east-1.amazonaws.com[203.0.113.9]:587: Connection timed out"; +const OTHER_HOST_NETWORK_REFUSAL = + "connect to mx.someone-else.example[198.51.100.9]:25: Connection refused"; const RELAY: OutboundRelay = { enabled: true, @@ -147,7 +156,8 @@ describe("parseMailQueue", () => { ); expect(parsed?.queued).toBe(6); - expect(parsed?.deferrals).toHaveLength(3); + expect(parsed?.deferrals).toHaveLength(6); + expect(topDeferrals(parsed!.deferrals)).toEqual(parsed!.deferrals.slice(0, 3)); }); /** @@ -386,6 +396,95 @@ describe("checkMailDelivery", () => { expect(health.deferrals[0]?.kind).toBe("auth"); }); + const crowdedQueue = (last: string) => + queueOutput([ + { reason: GREYLIST_REFUSAL }, + { reason: GREYLIST_REFUSAL }, + { reason: GREYLIST_REFUSAL }, + { reason: BUSY_RECEIVER }, + { reason: BUSY_RECEIVER }, + { reason: FULL_MAILBOX }, + { reason: FULL_MAILBOX }, + { reason: last }, + ]); + + const crowdedRows = [GREYLIST_REFUSAL, BUSY_RECEIVER, FULL_MAILBOX]; + const tryLater = (i: number) => `host mx${i}.example said: 451 try later`; + + it.each([ + ["an auth refusal ranked fourth", RELAY, crowdedQueue(AUTH_REFUSAL), "fail", crowdedRows], + [ + "a TLS failure at the smarthost ranked fourth", + RELAY, + crowdedQueue(RELAY_TLS_REFUSAL), + "fail", + crowdedRows, + ], + [ + "a connection failure at the smarthost ranked fourth", + RELAY, + crowdedQueue(RELAY_NETWORK_REFUSAL), + "fail", + crowdedRows, + ], + [ + "an auth refusal ranked thirteenth", + RELAY, + queueOutput([ + ...Array.from({ length: 24 }, (_, i) => ({ reason: tryLater(i % 12) })), + { reason: AUTH_REFUSAL }, + ]), + "fail", + [tryLater(0), tryLater(1), tryLater(2)], + ], + [ + "an auth refusal queued after three one-off deferrals", + RELAY, + queueOutput( + [GREYLIST_REFUSAL, BUSY_RECEIVER, FULL_MAILBOX, AUTH_REFUSAL].map((reason) => ({ reason })), + ), + "fail", + crowdedRows, + ], + [ + "an auth refusal on the last shown row", + RELAY, + queueOutput( + [GREYLIST_REFUSAL, GREYLIST_REFUSAL, BUSY_RECEIVER, AUTH_REFUSAL].map((reason) => ({ + reason, + })), + ), + "fail", + [GREYLIST_REFUSAL, BUSY_RECEIVER, AUTH_REFUSAL], + ], + [ + "a network failure at another host ranked fourth", + RELAY, + crowdedQueue(OTHER_HOST_NETWORK_REFUSAL), + "warn", + crowdedRows, + ], + [ + "a TLS failure ranked fourth with no relay host", + { ...RELAY, host: "" }, + crowdedQueue(RELAY_TLS_REFUSAL), + "warn", + crowdedRows, + ], + [ + "an auth refusal ranked fourth on a direct box", + undefined, + crowdedQueue(AUTH_REFUSAL), + "warn", + crowdedRows, + ], + ])("grades %s", async (_label, relay, queue, status, shown) => { + const health = await checkMailDelivery(box({ relay, queue })); + + expect(health.status).toBe(status); + expect(health.deferrals.map((d) => d.reason)).toEqual(shown); + }); + it("probes the queue through the engine, once", async () => { const exec = box({}); From e610b22fc654d84bcdb36a511a29e71317a42481 Mon Sep 17 00:00:00 2001 From: askalf <263217947+askalf@users.noreply.github.com> Date: Thu, 24 Sep 2026 05:44:20 +0000 Subject: [PATCH 2/2] test(mail): keep only the grading rows the uncapped verdict changes The last-shown-row, other-host, empty-relay-host and direct-box rows grade the same with the cap in the parse, so they pin nothing about this change. Read the parse result without a non-null assertion, like the rest of the file. --- .../mail/mail-delivery.service.test.ts | 39 ++----------------- 1 file changed, 3 insertions(+), 36 deletions(-) diff --git a/apps/api/test/modules/mail/mail-delivery.service.test.ts b/apps/api/test/modules/mail/mail-delivery.service.test.ts index 909b8dad0..fedbaec26 100644 --- a/apps/api/test/modules/mail/mail-delivery.service.test.ts +++ b/apps/api/test/modules/mail/mail-delivery.service.test.ts @@ -58,8 +58,6 @@ const BUSY_RECEIVER = "host mx.b.example said: 451 4.3.0 Temporary local problem const FULL_MAILBOX = "host mx.c.example said: 452 4.2.2 Mailbox full"; const RELAY_NETWORK_REFUSAL = "connect to email-smtp.us-east-1.amazonaws.com[203.0.113.9]:587: Connection timed out"; -const OTHER_HOST_NETWORK_REFUSAL = - "connect to mx.someone-else.example[198.51.100.9]:25: Connection refused"; const RELAY: OutboundRelay = { enabled: true, @@ -156,8 +154,9 @@ describe("parseMailQueue", () => { ); expect(parsed?.queued).toBe(6); - expect(parsed?.deferrals).toHaveLength(6); - expect(topDeferrals(parsed!.deferrals)).toEqual(parsed!.deferrals.slice(0, 3)); + const deferrals = parsed?.deferrals ?? []; + expect(deferrals).toHaveLength(6); + expect(topDeferrals(deferrals)).toEqual(deferrals.slice(0, 3)); }); /** @@ -446,38 +445,6 @@ describe("checkMailDelivery", () => { "fail", crowdedRows, ], - [ - "an auth refusal on the last shown row", - RELAY, - queueOutput( - [GREYLIST_REFUSAL, GREYLIST_REFUSAL, BUSY_RECEIVER, AUTH_REFUSAL].map((reason) => ({ - reason, - })), - ), - "fail", - [GREYLIST_REFUSAL, BUSY_RECEIVER, AUTH_REFUSAL], - ], - [ - "a network failure at another host ranked fourth", - RELAY, - crowdedQueue(OTHER_HOST_NETWORK_REFUSAL), - "warn", - crowdedRows, - ], - [ - "a TLS failure ranked fourth with no relay host", - { ...RELAY, host: "" }, - crowdedQueue(RELAY_TLS_REFUSAL), - "warn", - crowdedRows, - ], - [ - "an auth refusal ranked fourth on a direct box", - undefined, - crowdedQueue(AUTH_REFUSAL), - "warn", - crowdedRows, - ], ])("grades %s", async (_label, relay, queue, status, shown) => { const health = await checkMailDelivery(box({ relay, queue }));