This is a bug from this website’s own codebase. It never reached production, but only because of an accident of configuration rather than anything we did well — and the shape of it is common enough that it is worth writing down properly. Everything below can be checked against the repository.
The contact form accepted a submission, told the visitor we had received it, and gave them a reference number. Under the wrong conditions, the email carrying that submission was never sent, nothing was logged, and the reference number referred to nothing at all. The visitor had every reason to believe they had contacted us, and would have waited.
The three pieces
No single line was wrong. Three reasonable decisions lined up, and the failure lived in the space between them.
First, the mail transport was written with an unconfigured path, because the site had to run before an email provider existed. That path returned a value describing what had happened rather than throwing.
export async function sendMail(
message: MailMessage,
): Promise<{ delivered: boolean }> {
if (!isMailConfigured()) {
console.warn("[mailer] not configured — skipping send");
return { delivered: false };
}
const response = await fetch(BREVO_ENDPOINT, { /* ... */ });
if (!response.ok) {
return { delivered: false };
}
return { delivered: true };
}Read on its own that is defensible. It is honest about what happened, it does not crash the request, and the return type describes the two outcomes. The problem is entirely in how it was called.
Second, the route dispatched the sends concurrently and did not want one failure to abort the others, so it used the combinator that exists for exactly that.
await Promise.allSettled([
sendMail(buildTeamNotification(fields, reference)),
sendMail(buildClientConfirmation(fields, reference)),
saveInquiry({ ...inquiry, reference }),
]);
const body: InquiryResponse = { ok: true, reference, message: SUCCESS_BODY };
return NextResponse.json(body, { status: 200 });Third — and this is the actual defect — the success response did not depend on any of it. The 200 was going to be returned whatever those three promises did.
What allSettled actually promises
Promise.allSettled waits for every promise to settle and reports how each one settled. That is all it does. Its two outcomes are fulfilled and rejected, and those words mean exactly what they mean everywhere else in the language: a promise is rejected when something throws, and fulfilled when it does not.
A function that returns a value describing failure has not rejected. It has fulfilled, with a value.
const results = await Promise.allSettled([
Promise.resolve({ delivered: false }),
Promise.reject(new Error("network unreachable")),
]);
console.log(results);
[
{ status: "fulfilled", value: { delivered: false } },
{ status: "rejected", reason: Error: network unreachable },
]The first entry is a total failure of the thing the program exists to do, and it is reported as fulfilled. There is nothing wrong with allSettled here; it answered the question it was asked. The question was the wrong one.
It is worth stating the general form, because it has nothing to do with email. A combinator that observes rejection can only see failures that throw. Any failure path that returns instead is invisible to it, and invisible in a way that reads as success. The type system does not help either — nothing obliges a caller to inspect a returned `delivered: false`, and here nothing did.
Why it was latent, and what would have switched it on
The bug did no damage for as long as it existed, and the reason is uncomfortable: a different guard, added for an unrelated reason, was masking it.
The route refuses submissions outright when no mail transport is configured. That is an honest 503 with a real email address in the message, and it exists because telling somebody ‘we have received your request’ when the request has nowhere to go is a lie with a reference number on it.
if (!isMailConfigured()) {
console.error(
"[contact] Rejected a submission: no mail transport configured.",
);
const body: InquiryResponse = {
ok: false,
message: `This form isn't connected to our inbox yet. Please email us directly at ${contact.email}.`,
};
return NextResponse.json(body, { status: 503 });
}So in every environment without credentials — local development, previews, anywhere the keys had not been set — nothing was ever accepted, and therefore nothing was ever lost. The unconfigured branch of sendMail was unreachable from the route.
The day real credentials were configured, that guard would have stopped firing. From that moment every submission would have been accepted, and any send that failed for any reason — a provider outage, a rejected key, a network timeout, a rate limit — would have returned its failure value, been recorded as fulfilled, produced no log line, and been answered with a 200 and a reference number.
This is the property worth taking away. A latent bug is not a mild bug. It is a bug scheduled to activate on the day the system starts mattering, which is usually the same day somebody stops watching it closely.
The fix
Four changes, in the order they matter.
Every failure path in the mail transport now throws. There is no non-throwing failure route left in the file, and the docblock at the top says so, because the next person to add an SMTP fallback needs to know that this is a rule rather than a coincidence.
async function sendViaBrevo(message: MailMessage): Promise<void> {
let response: Response;
try {
response = await fetch(BREVO_ENDPOINT, {
method: "POST",
headers: { "api-key": mailConfig.apiKey /* ... */ },
body: JSON.stringify(payload),
signal: AbortSignal.timeout(BREVO_TIMEOUT_MS),
cache: "no-store",
});
} catch (cause) {
throw new MailError(
`Brevo request failed before a response: ${describe(cause)}`,
);
}
if (!response.ok) {
const body = await response.text().catch(() => "");
throw new MailError(
`Brevo API returned ${response.status}: ${body || response.statusText}`,
{ statusCode: response.status, body },
);
}
}The return type went from `Promise<{ delivered: boolean }>` to `Promise<void>`. That is the substantive change. A void return means there is no failure value a caller can accidentally ignore, because there is no value at all — success is the absence of a throw, which is the one convention every combinator in the language already understands.
Second, the one step that must not fail quietly was pulled out of the batch and awaited on its own. If we cannot tell ourselves that an enquiry arrived, the enquiry is lost, so that step decides the status code.
try {
await sendMail(buildTeamNotification(fields, reference));
} catch (error) {
console.error(`[contact] ${reference} FAILED to notify the team:`, error);
const body: InquiryResponse = {
ok: false,
message: `Something went wrong on our side and your message didn't reach us.`,
};
return NextResponse.json(body, { status: 502 });
}Third, the genuinely best-effort work — the confirmation to the sender, and persistence — still runs through allSettled, but the results are now read and every rejection is logged with the reference attached. If you are not going to inspect the results, allSettled is the wrong tool and a plain loop would be more honest about what you are doing.
const results = await Promise.allSettled([
(async () => sendMail(buildClientConfirmation(fields, reference)))(),
(async () => saveInquiry({ ...inquiry, reference, attachmentKey }))(),
]);
results.forEach((result, index) => {
if (result.status === "rejected") {
const step = ["confirm to client", "persist"][index];
console.error(`[contact] ${reference} failed to ${step}:`, result.reason);
}
});The async thunks around each call are not decoration and they fix a second, smaller version of the same class of bug. `buildClientConfirmation` and the object spread are synchronous, so calling them inline evaluates them outside any promise. A throw from either escaped allSettled entirely and produced a bare 500 — after the enquiry had already reached us, which invites the visitor to submit again. Wrapping each argument in an async function puts the synchronous part inside the promise, where the combinator can see it.
Fourth, the persistence stub got the same treatment. It is an unimplemented integration point, and the previous version returned `{ stored: false }` — the identical mistake, one file over.
export async function saveInquiry(
inquiry: StoredInquiry,
): Promise<{ stored: boolean }> {
if (!isStoreConfigured()) {
return { stored: false };
}
throw new Error(
`[inquiry-store] INQUIRY_STORE_URL is set but saveInquiry() is not ` +
`implemented. Inquiry ${inquiry.reference} was not persisted.`,
);
}
// Not configured still returns quietly — that is a deliberate no-op, not
// a failure. Configured-but-unimplemented throws, loudly, with the
// reference in the message.The rules that came out of it
- An integration stub should throw, not return. A stub that returns a failure value is indistinguishable from success at every call site that does not explicitly check, and nothing makes them check. A throw is the only failure signal that propagates by default.
- Prefer a void return for an operation that either works or does not. It removes the possibility of an ignored result rather than relying on discipline.
- Promise.allSettled only observes what throws, and it never rejects. That combination is exactly what makes it easy to await and then ignore.
- If you do not read the results array, you have written a fire-and-forget with extra steps. Use a loop, or read the results.
- Distinguish ‘this did not run because it is switched off’ from ‘this ran and failed’. The first is a configuration state and can return quietly. The second is an error and must throw.
- Never issue a reference number before the step that makes it true. A confirmation is a promise to the person reading it.
- Ask what a guard is currently hiding. The 503 in this route is correct and it is also the reason this bug survived review — a check that makes a path unreachable also makes it untested.
We publish this because it is checkable, and because ‘what does this team do when they find something like this’ is a more useful question about an engineering supplier than anything we could assert about ourselves. The current state of every file quoted above is in the repository. The rest of what we check →