Merge pull request #3837 from egeoztass/fix/email-error-propagation

fix: propagate SMTP transport errors from EmailService.sendEmail

server/src/api/controllers/settingsController.ts

@@ -93,24 +93,30 @@ class SettingsController implements ISettingsController {
 		if (!html) {
 			throw new AppError({ message: "Failed to build email template.", status: 500 });
 		}
-		const messageId = await this.emailService.sendEmail(to, subject, html, {
-			systemEmailHost,
-			systemEmailPort,
-			systemEmailUser,
-			systemEmailAddress,
-			systemEmailDisplayName,
-			systemEmailPassword,
-			systemEmailConnectionHost,
-			systemEmailSecure,
-			systemEmailPool,
-			systemEmailIgnoreTLS,
-			systemEmailRequireTLS,
-			systemEmailRejectUnauthorized,
-			systemEmailTLSServername,
-		});
-
-		if (!messageId) {
-			throw new AppError({ message: "Failed to send test email.", status: 500 });
+		let messageId: string;
+		try {
+			messageId = await this.emailService.sendEmail(to, subject, html, {
+				systemEmailHost,
+				systemEmailPort,
+				systemEmailUser,
+				systemEmailAddress,
+				systemEmailDisplayName,
+				systemEmailPassword,
+				systemEmailConnectionHost,
+				systemEmailSecure,
+				systemEmailPool,
+				systemEmailIgnoreTLS,
+				systemEmailRequireTLS,
+				systemEmailRejectUnauthorized,
+				systemEmailTLSServername,
+			});
+		} catch (error: unknown) {
+			// Surface the underlying SMTP failure: diagnosing the settings is the whole
+			// point of the test email endpoint.
+			throw new AppError({
+				message: error instanceof Error ? `Failed to send test email. ${error.message}` : "Failed to send test email.",
+				status: 500,
+			});
 		}
 
 		return res.status(200).json({

server/src/domain/invites/invite.service.ts

@@ -96,13 +96,15 @@ export class InviteService implements IInviteService {
 			});
 		}
 
-		const result = await this.emailService.sendEmail(invite.email, "Welcome to Uptime Monitor", html);
-		if (!result) {
+		try {
+			await this.emailService.sendEmail(invite.email, "Welcome to Uptime Monitor", html);
+		} catch (error: unknown) {
 			throw new AppError({
 				message: "Failed to send invite e-mail... Please verify your settings.",
 				service: SERVICE_NAME,
 				method: "sendInviteEmail",
 				status: 500,
+				details: { cause: error instanceof Error ? error.message : "Unknown error" },
 			});
 		}
 	};

server/src/domain/notifications/providers/email.ts

@@ -35,16 +35,18 @@ export class EmailProvider extends NotificationProvider {
 			return false;
 		}
 
-		const messageId = await this.emailService.sendEmail(notification.address, subject, html);
-		if (!messageId) {
+		try {
+			await this.emailService.sendEmail(notification.address, subject, html);
+			return true;
+		} catch (error: unknown) {
 			this.logger.warn({
 				message: "Email test alert failed",
 				service: SERVICE_NAME,
 				method: "sendTestAlert",
+				stack: error instanceof Error ? error.stack : undefined,
 			});
 			return false;
 		}
-		return true;
 	}
 
 	async sendMessage(notification: Notification, message: NotificationMessage): Promise<boolean> {
@@ -64,16 +66,20 @@ export class EmailProvider extends NotificationProvider {
 			return false;
 		}
 
-		const messageId = await this.emailService.sendEmail(notification.address, subject, html);
-		if (!messageId) {
+		// Providers must resolve to a boolean: NotificationsService fans these out through
+		// Promise.all, so a rejection here would abort the other channels' alerts too.
+		try {
+			await this.emailService.sendEmail(notification.address, subject, html);
+			return true;
+		} catch (error: unknown) {
 			this.logger.warn({
 				message: "Email notification failed",
 				service: SERVICE_NAME,
 				method: "sendMessage",
+				stack: error instanceof Error ? error.stack : undefined,
 			});
 			return false;
 		}
-		return true;
 	}
 
 	private buildSubject(message: NotificationMessage): string {

server/src/domain/users/user.service.ts

@@ -30,7 +30,7 @@ export interface IUserService {
 		currentUserEmail: string
 	): Promise<User>;
 	checkSuperadminExists(): Promise<boolean>;
-	requestRecovery(email: string): Promise<string | false | undefined>;
+	requestRecovery(email: string): Promise<string>;
 	validateRecovery(recoveryToken: string): Promise<void>;
 	resetPassword(password: string, recoveryToken: string): Promise<{ user: User; token: string }>;
 	deleteUser(params: { userId: string; teamId: string; roles: UserRole[] }): Promise<void>;

server/src/service/emailService.ts

@@ -2,6 +2,7 @@ import { fileURLToPath } from "url";
 import { EmailTransportConfig } from "@/domain/app-settings/app-settings.type.js";
 import { ISettingsService } from "@/domain/app-settings/app-settings.service.js";
 import { ILogger } from "@/utils/logger.js";
+import { AppError } from "@/utils/AppError.js";
 import fs from "node:fs";
 import path from "node:path";
 import nodemailer from "nodemailer";
@@ -20,7 +21,7 @@ type TemplateCompiler = (template: string) => (context: Record<string, unknown>)
 export interface IEmailService {
 	init(): void;
 	buildEmail(template: string, context: Record<string, unknown>): Promise<string | undefined>;
-	sendEmail(to: string, subject: string, html: string, transportConfig?: EmailTransportConfig): Promise<string | false | undefined>;
+	sendEmail(to: string, subject: string, html: string, transportConfig?: EmailTransportConfig): Promise<string>;
 }
 
 export class EmailService implements IEmailService {
@@ -151,7 +152,12 @@ export class EmailService implements IEmailService {
 				method: "verifyTransporter",
 				stack: error instanceof Error ? error.stack : undefined,
 			});
-			return false;
+			throw new AppError({
+				message: "Email transporter verification failed",
+				service: SERVICE_NAME,
+				method: "sendEmail",
+				details: { cause: error instanceof Error ? error.message : "Unknown error" },
+			});
 		}
 
 		const trimmedDisplayName = systemEmailDisplayName?.trim();
@@ -172,6 +178,12 @@ export class EmailService implements IEmailService {
 				method: "sendEmail",
 				stack: error instanceof Error ? error.stack : undefined,
 			});
+			throw new AppError({
+				message: "Failed to send email",
+				service: SERVICE_NAME,
+				method: "sendEmail",
+				details: { cause: error instanceof Error ? error.message : "Unknown error" },
+			});
 		}
 	};
 }

server/test/unit/providers/notifications/emailProvider.test.ts

@@ -50,9 +50,9 @@ describe("EmailProvider", () => {
 			expect(logger.warn).toHaveBeenCalledWith(expect.objectContaining({ message: "Failed to build test email content" }));
 		});
 
-		it("returns false when sendEmail returns falsy", async () => {
+		it("returns false when sendEmail rejects", async () => {
 			const { provider, emailService, logger } = createProvider();
-			(emailService.sendEmail as jest.Mock).mockResolvedValue(false);
+			(emailService.sendEmail as jest.Mock).mockRejectedValue(new Error("SMTP auth failed"));
 			expect(await provider.sendTestAlert(makeNotification())).toBe(false);
 			expect(logger.warn).toHaveBeenCalledWith(expect.objectContaining({ message: "Email test alert failed" }));
 		});
@@ -82,9 +82,11 @@ describe("EmailProvider", () => {
 			expect(logger.warn).toHaveBeenCalledWith(expect.objectContaining({ message: "Failed to build email content" }));
 		});
 
-		it("returns false when sendEmail returns falsy", async () => {
+		// NotificationsService fans providers out through Promise.all, so a failed email must
+		// resolve to false rather than reject and take the other channels down with it.
+		it("returns false when sendEmail rejects", async () => {
 			const { provider, emailService, logger } = createProvider();
-			(emailService.sendEmail as jest.Mock).mockResolvedValue(undefined);
+			(emailService.sendEmail as jest.Mock).mockRejectedValue(new Error("SMTP auth failed"));
 			expect(await provider.sendMessage(makeNotification() as any, makeMessage())).toBe(false);
 			expect(logger.warn).toHaveBeenCalledWith(expect.objectContaining({ message: "Email notification failed" }));
 		});

server/test/unit/services/emailService.test.ts

@@ -326,15 +326,17 @@ describe("EmailService", () => {
 			expect(nodemailer.createTransport).toHaveBeenCalledWith(expect.objectContaining({ name: "localhost" }));
 		});
 
-		it("returns false when transporter verification fails", async () => {
+		// Callers wrap sendEmail in .catch()/try-catch to react to delivery failures, so a
+		// transport error has to reject rather than resolve to a falsy value.
+		it("throws when transporter verification fails", async () => {
 			const transporter = createMockTransporter();
 			(transporter.verify as jest.Mock).mockRejectedValue(new Error("SMTP auth failed"));
 			const nodemailer = createMockNodemailer(transporter);
 			const { service, logger } = createService({ nodemailer });
 
-			const result = await service.sendEmail("to@example.com", "Subject", "<p>body</p>", makeTransportConfig());
-
-			expect(result).toBe(false);
+			await expect(service.sendEmail("to@example.com", "Subject", "<p>body</p>", makeTransportConfig())).rejects.toThrow(
+				"Email transporter verification failed"
+			);
 			expect(logger.warn).toHaveBeenCalledWith(
 				expect.objectContaining({
 					message: "Email transporter verification failed",
@@ -344,27 +346,38 @@ describe("EmailService", () => {
 			);
 		});
 
+		it("reports the underlying cause when verification fails", async () => {
+			const transporter = createMockTransporter();
+			(transporter.verify as jest.Mock).mockRejectedValue(new Error("SMTP auth failed"));
+			const nodemailer = createMockNodemailer(transporter);
+			const { service } = createService({ nodemailer });
+
+			await expect(service.sendEmail("to@example.com", "Subject", "<p>body</p>", makeTransportConfig())).rejects.toMatchObject({
+				status: 500,
+				details: { cause: "SMTP auth failed" },
+			});
+		});
+
 		it("logs stack as undefined when verification fails with non-Error", async () => {
 			const transporter = createMockTransporter();
 			(transporter.verify as jest.Mock).mockRejectedValue("connection refused");
 			const nodemailer = createMockNodemailer(transporter);
 			const { service, logger } = createService({ nodemailer });
 
-			const result = await service.sendEmail("to@example.com", "Subject", "<p>body</p>", makeTransportConfig());
-
-			expect(result).toBe(false);
+			await expect(service.sendEmail("to@example.com", "Subject", "<p>body</p>", makeTransportConfig())).rejects.toThrow();
 			expect(logger.warn).toHaveBeenCalledWith(expect.objectContaining({ stack: undefined }));
 		});
 
-		it("returns undefined and logs error when sendMail throws", async () => {
+		it("throws and logs error when sendMail throws", async () => {
 			const transporter = createMockTransporter();
 			(transporter.sendMail as jest.Mock).mockRejectedValue(new Error("Recipient rejected"));
 			const nodemailer = createMockNodemailer(transporter);
 			const { service, logger } = createService({ nodemailer });
 
-			const result = await service.sendEmail("to@example.com", "Subject", "<p>body</p>", makeTransportConfig());
-
-			expect(result).toBeUndefined();
+			await expect(service.sendEmail("to@example.com", "Subject", "<p>body</p>", makeTransportConfig())).rejects.toMatchObject({
+				message: "Failed to send email",
+				details: { cause: "Recipient rejected" },
+			});
 			expect(logger.error).toHaveBeenCalledWith(
 				expect.objectContaining({
 					message: "Recipient rejected",
@@ -380,9 +393,9 @@ describe("EmailService", () => {
 			const nodemailer = createMockNodemailer(transporter);
 			const { service, logger } = createService({ nodemailer });
 
-			const result = await service.sendEmail("to@example.com", "Subject", "<p>body</p>", makeTransportConfig());
-
-			expect(result).toBeUndefined();
+			await expect(service.sendEmail("to@example.com", "Subject", "<p>body</p>", makeTransportConfig())).rejects.toMatchObject({
+				details: { cause: "Unknown error" },
+			});
 			expect(logger.error).toHaveBeenCalledWith(
 				expect.objectContaining({
 					message: "Unknown error",

server/test/unit/services/inviteService.test.ts

@@ -127,11 +127,11 @@ describe("InviteService", () => {
 			).rejects.toThrow("Failed to build invite e-mail");
 		});
 
-		it("throws 500 when sendEmail returns falsy", async () => {
+		it("throws 500 when sendEmail rejects", async () => {
 			const { service } = createService({
 				emailService: {
 					buildEmail: jest.fn().mockResolvedValue("<html>ok</html>"),
-					sendEmail: jest.fn().mockResolvedValue(false),
+					sendEmail: jest.fn().mockRejectedValue(new Error("SMTP auth failed")),
 				},
 			});
 

server/test/unit/services/userService.test.ts

@@ -196,6 +196,22 @@ describe("UserService", () => {
 			expect(logger.warn).toHaveBeenCalledWith(expect.objectContaining({ method: "registerUser" }));
 		});
 
+		// The welcome email is fire-and-forget, but its .catch() only runs if sendEmail
+		// actually rejects; swallowing the transport error made that handler dead code.
+		it("logs a warning when the welcome email fails to send", async () => {
+			const { service, logger } = createService({
+				emailService: {
+					buildEmail: jest.fn().mockResolvedValue("<html>Welcome</html>"),
+					sendEmail: jest.fn().mockRejectedValue(new Error("SMTP auth failed")),
+				},
+			});
+
+			const result = await service.registerUser({ password: "pass" }, "invite-token", null);
+
+			expect(result.user).toBeDefined();
+			expect(logger.warn).toHaveBeenCalledWith(expect.objectContaining({ method: "registerUser", message: "SMTP auth failed" }));
+		});
+
 		it("uses default role when invite has no role", async () => {
 			const { service, usersRepository } = createService({
 				invitesRepository: {
@@ -423,6 +439,19 @@ describe("UserService", () => {
 
 			await expect(service.requestRecovery("test@example.com")).rejects.toThrow("Failed to build password reset email HTML");
 		});
+
+		// Previously the send failure was swallowed and the endpoint still reported
+		// "Password recovery email sent successfully" while the user got nothing.
+		it("propagates the failure when the recovery email cannot be sent", async () => {
+			const { service } = createService({
+				emailService: {
+					buildEmail: jest.fn().mockResolvedValue("<html>Reset</html>"),
+					sendEmail: jest.fn().mockRejectedValue(new Error("SMTP auth failed")),
+				},
+			});
+
+			await expect(service.requestRecovery("test@example.com")).rejects.toThrow("SMTP auth failed");
+		});
 	});
 
 	// ── validateRecovery ────────────────────────────────────────────────────