fix(mailer,audit): propagate send and write failures so caller context logging fires (#3973)

* fix(mailer,audit): propagate send/write failures so caller context logging fires

lib/helpers/mailer/index.js#sendMail and modules/audit/services/audit.service.js#log
each wrapped their failing call in a local try/catch, logged one generic line, and
resolved null instead of rejecting. Every caller's own context-rich .catch()
(action/userId/orgId/targetType) never ran on a real outage — dead code hidden
behind a swallowed rejection.

Let both helpers propagate the underlying rejection. The "mail/audit failure never
breaks the main flow" invariant now lives at the call sites (their existing
.catch()), not centrally in the helper:
- auth signup/reset, billing.email.js, invitations.service.js, and the 4
  organizations.membership.service.js sites already had context-logging .catch()
  handlers that were previously unreachable — now they fire.
- invitations.service.js#resend already documented "a transport failure surfaces
  as an error" but the swallow silently defeated that; propagation fixes it to
  match its own doc comment.
- audit.middleware.js's .catch() is enriched with action/userId/organizationId/
  targetType/targetId so a write failure now logs full request context, not just
  message+stack.

Closes #3966

Claude-Session: https://claude.ai/code/session_01WfNC8bt1TgL4AsiYgCEGup

* fix(mailer): return a generic client message on resend mail-transport failures

resendVerification (auth.controller.js) and the invitations resend endpoint
now surface propagated mail-transport failures via errors.getMessage(err),
which relayed the raw SMTP/Resend provider error string to the client on
these two endpoints — a low-severity infra-detail leak flagged by the
pre-push critical-review gate on #3966.

Both catches now log the real error server-side with context (userId /
invitationId) and respond with a stable generic message. invitations.resend
still threads deliberate AppError statuses (409 conflict, 404 not found, 422
mailer-not-configured) through unchanged — only the untagged transport
rejection (no `.status`) gets the generic treatment. resendVerification's
catch is resend-email-scoped end-to-end, so the whole catch goes generic.

Claude-Session: https://claude.ai/code/session_01WfNC8bt1TgL4AsiYgCEGup

Author: PierreBrisorgueil

ERRORS.md

@@ -33,3 +33,4 @@ Use this file as a compact memory of recurring AI mistakes.
 - [2026-07-16] security: `users.repository.js findByIdAndUpdatePopulated()` does `.populate()` with no `.select()`, so `organizations.controller.js switchOrganization` serialized the raw doc (password hash + OAuth tokens + reset/verification tokens) straight to the client; `users.account.controller.js me()` separately forwarded `providerData` (OAuth tokens) verbatim -> any endpoint returning a Mongoose user doc must go through `UserService.removeSensitive()` (whitelist, `modules/users/utils/sanitizeUser.js`) at the response boundary, never serialize `req.user`/a populated doc directly; a local-signup fixture's `providerData` defaults to `{}` so a naive falsy check won't catch this — seed a fake OAuth token in tests to prove the leak is actually closed; see pierreb-devkit/Node#3963
 - [2026-07-16] billing/stripe: the #3742 priceId-map fix (`resolvePlan`/`buildPriceIdToPlanMap`) was applied only to the webhook handler; `billing.admin.service.js` `resolveStripePlan` (admin force-sync, a DB WRITE path) and `billing.reconcile.service.js` `resolveStripePlan` (LOG-ONLY divergence check) kept their own copies reading only `metadata.planId` -> a paid org's Stripe subscription (which never carries `metadata.planId`) resolved to `'free'` on admin sync (silently downgrading a paying org) and produced a false `planMismatch` alert on every reconcile run; fix = one shared resolver (`modules/billing/lib/billing.planResolver.js`) used by all three call sites, and a null-unresolved sentinel (never guess `'free'`) so the admin write path ABORTS instead of downgrading and the reconcile log path skips the comparison instead of alerting; see pierreb-devkit/Node#3964
 - [2026-07-16] architecture: `users.service.js remove()`'s sole-owner cascade called `OrganizationsRepository.remove()` directly instead of `organizations.crud.service.js#remove()`, silently skipping `runOrganizationRemovedHandlers` (the `onOrganizationRemoved` seam `modules/tasks/tasks.init.js` registers for org-scoped task cleanup) -> deleting an org must always route through the owning module's SERVICE method, never straight to its Repository, even for a "bare" cascade delete from another module — a direct-repository shortcut silently bypasses any registered removal hook; a stale cross-test fixture in `tasks.integration.tests.js` (an orphaned-but-still-existing task reused across unrelated test cases) was inadvertently relying on this bug and needed a properly isolated fixture once the cascade actually cleaned up; see pierreb-devkit/Node#3965
+- [2026-07-17] error handling: shared helpers (`lib/helpers/mailer/index.js#sendMail`, `modules/audit/services/audit.service.js#log`) wrapped the failing call in their own try/catch, logged a single generic line, and resolved `null` -> every caller's own context-rich `.catch()` (action/userId/orgId) became dead code on a real outage, since the promise never rejected; a central helper must let the underlying call's rejection propagate and let each CALL SITE own the "never break the main flow" `.catch()` with its own context — never swallow centrally just because most callers currently attach a catch; see pierreb-devkit/Node#3966

lib/helpers/mailer/index.js

@@ -2,7 +2,6 @@ import path from 'path';
 import handlebars from 'handlebars';
 
 import config from '../../../config/index.js';
-import logger from '../../services/logger.js';
 import files from '../files.js';
 import NodemailerProvider from './provider.nodemailer.js';
 import ResendProvider from './provider.resend.js';
@@ -73,7 +72,12 @@ const validateAttachments = (attachments) => {
  * @param {Array} [mail.attachments] - Optional attachments array
  * @param {string} mail.attachments[].filename - Attachment filename
  * @param {string|Buffer} mail.attachments[].content - Attachment content (string or Buffer)
- * @returns {Promise<Object|null>} The send result or null if not configured
+ * @returns {Promise<Object|null>} The send result, or null if not configured
+ * @throws {Error} If the provider's send() call fails — propagated so each
+ *   caller's own `.catch()` can log with its flow-specific context
+ *   (action, userId, orgId, ...). Callers that must never let a mail failure
+ *   break their main flow are responsible for attaching that `.catch()`
+ *   themselves; this helper does not swallow errors centrally.
  */
 const sendMail = async (mail) => {
   if (!isConfigured()) return null;
@@ -83,20 +87,15 @@ const sendMail = async (mail) => {
   const file = await files.readFile(path.resolve(`./config/templates/${sanitizedTemplate}.html`));
   const template = handlebars.compile(file);
   const html = template(mail.params);
-  try {
-    const result = await getProvider().send({
-      from: config.mailer.from,
-      to: mail.to,
-      subject: mail.subject,
-      html,
-      attachments: mail.attachments,
-    });
-    if (!Array.isArray(result?.accepted)) return { ...result, accepted: [mail.to], rejected: [] };
-    return result;
-  } catch (err) {
-    logger.error('Mail send error', err);
-    return null;
-  }
+  const result = await getProvider().send({
+    from: config.mailer.from,
+    to: mail.to,
+    subject: mail.subject,
+    html,
+    attachments: mail.attachments,
+  });
+  if (!Array.isArray(result?.accepted)) return { ...result, accepted: [mail.to], rejected: [] };
+  return result;
 };
 
 export default { sendMail, isConfigured };

lib/helpers/mailer/tests/mailer.unit.tests.js

@@ -29,15 +29,6 @@ jest.unstable_mockModule('../provider.nodemailer.js', () => ({
   default: jest.fn().mockImplementation(() => ({ send: jest.fn() })),
 }));
 
-jest.unstable_mockModule('../../../services/logger.js', () => ({
-  default: {
-    error: jest.fn(),
-    warn: jest.fn(),
-    info: jest.fn(),
-    debug: jest.fn(),
-  },
-}));
-
 const { default: mailer } = await import('../index.js');
 
 describe('mailer index with resend provider unit tests:', () => {
@@ -102,17 +93,35 @@ describe('mailer index with resend provider unit tests:', () => {
     );
   });
 
-  test('should return null on send error', async () => {
+  test('should propagate (reject) when the provider send fails, instead of swallowing to null', async () => {
     mockSend.mockRejectedValue(new Error('API failure'));
 
-    const result = await mailer.sendMail({
-      to: 'user@example.com',
-      subject: 'Test',
-      template: 'welcome',
-      params: { name: 'Bob' },
-    });
+    await expect(
+      mailer.sendMail({
+        to: 'user@example.com',
+        subject: 'Test',
+        template: 'welcome',
+        params: { name: 'Bob' },
+      }),
+    ).rejects.toThrow('API failure');
+  });
+
+  test('should let a provider rejection reach a caller-attached .catch() with its own context', async () => {
+    mockSend.mockRejectedValue(new Error('API failure'));
+    const callerLogger = { warn: jest.fn() };
+
+    // Mirrors the call-site pattern used across the codebase: fire-and-forget
+    // sendMail() with a local .catch() that logs flow-specific context.
+    await mailer
+      .sendMail({
+        to: 'user@example.com',
+        subject: 'Test',
+        template: 'welcome',
+        params: { name: 'Bob' },
+      })
+      .catch((err) => callerLogger.warn('caller: mail failed', { message: err?.message, userId: 'u1' }));
 
-    expect(result).toBeNull();
+    expect(callerLogger.warn).toHaveBeenCalledWith('caller: mail failed', { message: 'API failure', userId: 'u1' });
   });
 
   test('should throw when attachment is missing content', async () => {

modules/audit/middlewares/audit.middleware.js

@@ -118,15 +118,26 @@ const createAuditMiddleware = (options = {}) => {
       const targetType = deriveTargetType(routePath, req.baseUrl);
       const targetId = deriveTargetId(req.params);
 
+      const userId = req.user?._id || req.user?.id;
+      const organizationId = req.organization?._id || req.organization?.id;
+
       AuditService.log({
         action,
-        userId: req.user?._id || req.user?.id,
-        organizationId: req.organization?._id || req.organization?.id,
+        userId,
+        organizationId,
         ip: config.audit?.captureIp !== false ? (req.ip || req.connection?.remoteAddress || '') : undefined,
         userAgent: config.audit?.captureUserAgent !== false ? (req.headers?.['user-agent'] || '') : undefined,
         targetType,
         targetId,
-      }).catch((err) => logger.error('audit.middleware: audit log write failed', { message: err?.message, stack: err?.stack }));
+      }).catch((err) => logger.error('audit.middleware: audit log write failed', {
+        message: err?.message,
+        stack: err?.stack,
+        action,
+        userId,
+        organizationId,
+        targetType,
+        targetId,
+      }));
     });
 
     return next();

modules/audit/services/audit.service.js

@@ -2,7 +2,6 @@
  * Module dependencies
  */
 import config from '../../../config/index.js';
-import logger from '../../../lib/services/logger.js';
 import AuditRepository from '../repositories/audit.repository.js';
 
 /**
@@ -17,7 +16,12 @@ import AuditRepository from '../repositories/audit.repository.js';
  * @param {string} [params.targetType] - Type of the target entity
  * @param {string} [params.targetId] - ID of the target entity
  * @param {Object} [params.metadata] - Additional metadata
- * @returns {Promise<Object|null>} The created audit log entry or null if disabled
+ * @returns {Promise<Object|null>} The created audit log entry, or null if disabled
+ * @throws {Error} If `AuditRepository.create` fails — propagated so the caller's
+ *   own `.catch()` can log with its request context (action, userId, orgId,
+ *   targetType, ...). Audit must never break the main flow — that guarantee is
+ *   enforced by the caller's `.catch()` (see `audit.middleware.js`), not by
+ *   swallowing the write failure here.
  */
 const log = async ({ action, userId, organizationId, ip, userAgent, targetType, targetId, metadata } = {}) => {
   if (!config.audit?.enabled) return null;
@@ -35,13 +39,7 @@ const log = async ({ action, userId, organizationId, ip, userAgent, targetType,
   if (ip !== undefined) entry.ip = ip || '';
   if (userAgent !== undefined) entry.userAgent = userAgent || '';
 
-  try {
-    return await AuditRepository.create(entry);
-  } catch (err) {
-    // Audit must never break the main flow
-    logger.error('AuditLog write failed:', { message: err.message });
-    return null;
-  }
+  return AuditRepository.create(entry);
 };
 
 /**

modules/audit/tests/audit.middleware.unit.tests.js

@@ -257,7 +257,7 @@ describe('Audit middleware unit tests:', () => {
     expect(mockLog).not.toHaveBeenCalled();
   });
 
-  test('should not throw when AuditService.log rejects and should log the error', async () => {
+  test('should not throw when AuditService.log rejects and should log the error with request context', async () => {
     const dbError = new Error('DB down');
     mockLog = jest.fn().mockRejectedValue(dbError);
     const middleware = createAuditMiddleware();
@@ -266,13 +266,21 @@ describe('Audit middleware unit tests:', () => {
     const next = jest.fn();
 
     middleware(req, res, next);
-    // Should not throw
+    // Should not throw — a rejected AuditService.log() must never break the request flow
     expect(() => res.emit('finish')).not.toThrow();
     // Allow the .catch() handler to run
     await new Promise((r) => setTimeout(r, 10));
     expect(mockLoggerError).toHaveBeenCalledWith(
       'audit.middleware: audit log write failed',
-      { message: dbError.message, stack: dbError.stack },
+      {
+        message: dbError.message,
+        stack: dbError.stack,
+        action: 'auth.signin',
+        userId: '507f1f77bcf86cd799439011',
+        organizationId: '507f1f77bcf86cd799439012',
+        targetType: 'User',
+        targetId: '',
+      },
     );
   });
 });

modules/audit/tests/audit.service.unit.tests.js

@@ -22,15 +22,6 @@ jest.unstable_mockModule('../../../config/index.js', () => ({
   default: mockConfig,
 }));
 
-// Mock logger to avoid winston config dependency in unit tests
-jest.unstable_mockModule('../../../lib/services/logger.js', () => ({
-  default: {
-    error: jest.fn(),
-    warn: jest.fn(),
-    info: jest.fn(),
-  },
-}));
-
 /**
  * Unit tests
  */
@@ -90,11 +81,28 @@ describe('AuditService unit tests:', () => {
     expect(arg.userAgent).toBeFalsy();
   });
 
-  test('should not throw when repository create fails', async () => {
+  test('should propagate (reject) when repository create fails, instead of swallowing to null', async () => {
     mockCreate = jest.fn().mockRejectedValue(new Error('DB down'));
 
-    const result = await AuditService.log({ action: 'test.fail' });
-    expect(result).toBeNull();
+    await expect(AuditService.log({ action: 'test.fail' })).rejects.toThrow('DB down');
+  });
+
+  test('should let a repository rejection reach a caller-attached .catch() with its own context', async () => {
+    mockCreate = jest.fn().mockRejectedValue(new Error('DB down'));
+    const callerLogger = { error: jest.fn() };
+
+    // Mirrors audit.middleware.js's pattern: fire-and-forget log() with a
+    // local .catch() that logs request context (action/userId/orgId).
+    await AuditService.log({ action: 'test.fail', userId: 'u1', organizationId: 'o1' }).catch((err) =>
+      callerLogger.error('audit.middleware: audit log write failed', { message: err?.message, action: 'test.fail', userId: 'u1', orgId: 'o1' }),
+    );
+
+    expect(callerLogger.error).toHaveBeenCalledWith('audit.middleware: audit log write failed', {
+      message: 'DB down',
+      action: 'test.fail',
+      userId: 'u1',
+      orgId: 'o1',
+    });
   });
 
   test('should list audit logs with filters', async () => {

modules/auth/controllers/auth.controller.js

@@ -850,7 +850,17 @@ const resendVerification = async (req, res) => {
     if (!acceptedCount) return responses.error(res, 400, 'Bad Request', 'Failure sending email')();
     return responses.success(res, 'Verification email sent')({ status: true });
   } catch (err) {
-    responses.error(res, 422, 'Unprocessable Entity', errors.getMessage(err))(err);
+    // #3966 hardening: sendVerificationEmail (mailer.sendMail) now propagates a
+    // transport failure instead of swallowing it — the raw SMTP/provider error
+    // string must not leak to the client. Log the real error server-side with
+    // context (this catch is resend-email-scoped: an unexpected failure here is
+    // effectively always the mail send); respond with a stable generic message.
+    logger.error('[auth.resendVerification] failed', {
+      userId: req.user?.id,
+      message: err?.message,
+      stack: err?.stack,
+    });
+    responses.error(res, 422, 'Unprocessable Entity', 'Failed to send the email, please try again.')(err);
   }
 };