fix(mail): bound the transporter's timeouts, and fix a now-stale comment (#260)

adminUploadLinks.ts awaits sendMail on the admin's request path, the first awaited send on a user-facing request in this codebase, but the transporter in mailer.ts set no connectionTimeout, greetingTimeout or socketTimeout. nodemailer's defaults then apply: two minutes to connect, ten minutes on the socket. If the SMTP host is unreachable in a way that drops packets rather than refusing, the link row and its token are already committed by the time sendMail is called, the response hangs for up to two minutes, the browser or reverse proxy gives up first, and the token — shown exactly once and unrecoverable — is never rendered. That is the link being lost in exactly the way this feature's central invariant forbids.

All three timeouts are now set to 5000ms, with a comment explaining why a send on a request path has to fail fast rather than inherit nodemailer's fire-and-forget defaults. Five seconds is generous for a reachable host and short enough that a dead one fails while the admin is still willing to wait, leaving them the "not emailed" warning and a link they can still copy instead of a stuck spinner and a token nobody ever saw.

Also corrects the comment directly above the skipped-blocked return, which said "returning as though it sent" — true before #260, and precisely backwards now that the outcome is reported through MailOutcome rather than swallowed. Reworded to describe what the code actually does today, keeping the explanation of why it skips rather than throws.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
2026-09-03 18:30:49 -05:00
co-authored by Claude Opus 5
parent 5a9022d8d1
commit 0dd4ac36ff
+20 -6
View File
@@ -4,6 +4,16 @@ import nodemailer from 'nodemailer';
// Brevo — has to set host, port and SMTP_SECURE explicitly rather than
// inheriting these, and getting that wrong fails at send time rather than at
// boot. See #64 on validating this at startup instead.
// #260 put the first awaited send on a user-facing request path (the admin
// creating an upload link). nodemailer's defaults are two minutes to connect
// and ten minutes on the socket, which is fine for a fire-and-forget send but
// is not a bound anyone waiting on a response can live with: the link row and
// its token are already committed by the time sendMail is called, the token
// is shown exactly once, and a request that hangs long enough for the browser
// or reverse proxy to give up first loses it for good. Five seconds each is
// long enough for a reachable host and short enough that a dead one fails
// fast, leaving the admin with the "not emailed" warning and a link they can
// still copy, instead of a stuck spinner and a token nobody ever saw.
const transporter = nodemailer.createTransport({
host: process.env.SMTP_HOST || 'smtp.gmail.com',
port: parseInt(process.env.SMTP_PORT || '465', 10),
@@ -11,7 +21,10 @@ const transporter = nodemailer.createTransport({
auth: {
user: process.env.SMTP_USER,
pass: process.env.SMTP_PASSWORD
}
},
connectionTimeout: 5000,
greetingTimeout: 5000,
socketTimeout: 5000
});
interface ParsedAddress {
@@ -105,11 +118,12 @@ export async function sendMail(to: string, subject: string, html: string): Promi
// Guarded here rather than at the four call sites, so every sender is covered
// by construction and a fifth added later cannot bypass it by forgetting.
//
// Skipping rather than throwing, and returning as though it sent: three of
// the callers already swallow send failures into a log, so throwing would
// mostly be caught and logged anyway while risking a 500 on the signup path.
// The flow under test finishes, and the log says why no mail arrived — which
// is the part that was missing when QA was simply muted.
// Skipping rather than throwing, and reporting the skip through MailOutcome
// rather than pretending nothing happened: three of the callers already
// swallow send failures into a log, so throwing would mostly be caught and
// logged anyway while risking a 500 on the signup path. The flow under test
// finishes, and both the log and the returned outcome say why no mail
// arrived — which is the part that was missing when QA was simply muted.
if (!isAllowedRecipient(to, process.env.MAIL_ALLOWLIST)) {
console.warn(`[mail-blocked] ${to} is not on MAIL_ALLOWLIST — skipping "${subject}"`);
return 'skipped-blocked';