Skip to content

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

Merged
PierreBrisorgueil merged 2 commits into
masterfrom
fix/3966-propagate-mail-audit-failures
Jul 17, 2026
Merged

fix(mailer,audit): propagate send and write failures so caller context logging fires#3973
PierreBrisorgueil merged 2 commits into
masterfrom
fix/3966-propagate-mail-audit-failures

Conversation

@PierreBrisorgueil

Copy link
Copy Markdown
Contributor

Summary

  • What changed: 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. Both helpers now propagate the underlying rejection; 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. 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. On top of the propagation, resendVerification (auth) and the invitations resend endpoint now return a stable generic client message on a mail-transport failure instead of the raw provider error string — the real error is still logged server-side with context.
  • Why: the "mail/audit failure never breaks the main flow" invariant was living inside the helpers via a silent swallow-and-return-null, which made every caller's existing .catch() dead code. Propagating moves that invariant to the call sites, which already carry (or, for the two resend endpoints, now carry) the right catch/logging. Propagating also surfaced a pre-existing leak: errors.getMessage(err) on those two endpoints relayed the raw SMTP/Resend provider error straight to the client (flagged by the pre-push critical-review gate) — fixed by giving those two catches a generic client message while still logging the real error server-side.
  • Related issues: Closes 🔧 Propagate mail and audit write failures to callers #3966

Scope

  • Module(s) impacted: lib/helpers/mailer, modules/audit (service + middleware), modules/auth/controllers, modules/invitations/controllers
  • Cross-module impact: yes — every caller of mails.sendMail() and AuditService.log() now sees a rejecting promise instead of null. Audited: auth signup/reset, billing.email.js, invitations service (fire-and-forget create() path), organizations.membership.service.js (×4 sites), users email-change — all already carry their own context .catch(), so behavior is unaffected beyond that catch now actually running. resendVerification and invitations.resend were the two awaited (non-fire-and-forget) call sites without a local .catch() — hardened directly in this PR to log server-side and respond generically instead of relaying the raw error.
  • Risk level: low — the "never break the main flow" invariant is preserved at every existing fire-and-forget call site; the two awaited call sites get new explicit handling instead of falling through to the old swallow-and-null path.

Validation

  • npm run lint
  • npm test (unit — 2189/2189 green, including new coverage for both hardened catches)
  • Manual checks done (N/A — no user-facing behavior change beyond error propagation/messaging)

Tests assert: a provider/repo rejection reaches the caller's context .catch() (mailer + audit unit tests), the main flow never crashes on a mail/audit failure, and — new in this PR — the client response for a mail-transport failure on resendVerification/invitations.resend does not contain the raw provider error string while the server-side context log still fires.

Guardrails check

  • No secrets or credentials introduced (.env*, secrets/**, keys, tokens)
  • No risky rename/move of core stack paths
  • Changes remain merge-friendly for downstream projects
  • Tests added or updated when behavior changed

Notes for reviewers

  • Security considerations: closes a low-severity info-leak — resendVerification and invitations.resend previously relayed the raw SMTP/Resend provider error string to the client via errors.getMessage(err). Both now log the real error server-side with context (userId / invitationId) and respond with a stable generic message ("Failed to send the email, please try again."). 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.
  • Mergeability considerations: resendVerification/invitations.resend now return 422 instead of the old misleading 400/200 on some previously-swallowed failure paths — intentional, more-correct status, not a regression.
  • Follow-up tasks (optional): none

https://claude.ai/code/session_01WfNC8bt1TgL4AsiYgCEGup

…gging 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
… 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
@PierreBrisorgueil PierreBrisorgueil added the Fix A bug fix label Jul 17, 2026
@PierreBrisorgueil PierreBrisorgueil self-assigned this Jul 17, 2026
@PierreBrisorgueil PierreBrisorgueil added the Fix A bug fix label Jul 17, 2026
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@PierreBrisorgueil, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 29 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 71a66cc9-85ec-4d2d-96c7-f21fda6d1c0a

📥 Commits

Reviewing files that changed from the base of the PR and between b9f4951 and 782df4e.

📒 Files selected for processing (12)
  • ERRORS.md
  • lib/helpers/mailer/index.js
  • lib/helpers/mailer/tests/mailer.unit.tests.js
  • modules/audit/middlewares/audit.middleware.js
  • modules/audit/services/audit.service.js
  • modules/audit/tests/audit.middleware.unit.tests.js
  • modules/audit/tests/audit.service.unit.tests.js
  • modules/auth/controllers/auth.controller.js
  • modules/auth/tests/auth.silent.catch.unit.tests.js
  • modules/invitations/controllers/invitations.controller.js
  • modules/invitations/tests/invitations.controller.unit.tests.js
  • modules/invitations/tests/invitations.service.unit.tests.js
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/3966-propagate-mail-audit-failures

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@PierreBrisorgueil
PierreBrisorgueil marked this pull request as ready for review July 17, 2026 00:39
@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.13%. Comparing base (ac772a4) to head (782df4e).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3973      +/-   ##
==========================================
+ Coverage   93.06%   93.13%   +0.07%     
==========================================
  Files         170      170              
  Lines        5595     5596       +1     
  Branches     1802     1803       +1     
==========================================
+ Hits         5207     5212       +5     
+ Misses        313      309       -4     
  Partials       75       75              
Flag Coverage Δ
integration 61.25% <50.00%> (-0.02%) ⬇️
unit 75.01% <100.00%> (+0.16%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac772a4...782df4e. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@PierreBrisorgueil

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.


Your plan includes PR reviews subject to rate limits. More reviews will be available in 29 minutes.

@PierreBrisorgueil
PierreBrisorgueil merged commit 82b5cf0 into master Jul 17, 2026
8 checks passed
@PierreBrisorgueil
PierreBrisorgueil deleted the fix/3966-propagate-mail-audit-failures branch July 17, 2026 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Fix A bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🔧 Propagate mail and audit write failures to callers

1 participant