feat: add retry logic and circuit breaker for email service (#326) - #765
Conversation
…ar0036singh#326) When the email provider fails, events were simply not notified with no retry, no alerting and no trace of the lost messages. - utils/emailResilience.ts: EmailCircuitBreaker (opens after 5 consecutive failures, half-open after cooldown), retryWithExponentialBackoff (1s, 2s, 4s, 8s, 16s), enqueueDeadLetter (Firestore email_dead_letter_queue) and alertAdmins (admin_alerts collection + error log) - utils/emailSender.ts: sendEmailWithRetry wraps sendEmail with backoff + breaker; final failures are logged with recipient/eventId/reason context and queued to the DLQ; breaker trips alert admins - sendBulkEmails.ts: per-recipient EmailJS sends now go through the same retry/breaker/DLQ path (timeout is scoped per attempt so backoff cannot be starved by a single AbortController) - retryDeadLetterEmail.ts: new onCall (admin/club only) that re-sends a queued DLQ entry via Resend and tracks retryCount/status - 10 new tests (backoff, breaker states, DLQ writes, open-breaker short-circuit, onCall auth/roles/re-send); tsc clean; 21 tests pass in the affected suites
|
📝 WalkthroughWalkthroughThe pull request adds email retries, circuit-breaker protection, dead-letter persistence, administrator alerts, and a callable function for manual dead-letter retries. It also updates bulk email delivery and adds Jest coverage. ChangesEmail resilience
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant sendBulkEmails
participant sendEmailWithRetry
participant sendEmail
participant Firestore
sendBulkEmails->>sendEmailWithRetry: send email with retry context
sendEmailWithRetry->>sendEmail: attempt provider delivery
sendEmail-->>sendEmailWithRetry: return success or provider error
sendEmailWithRetry->>Firestore: queue exhausted failure
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
cloud-functions/src/index.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. cloud-functions/src/retryDeadLetterEmail.test.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. cloud-functions/src/retryDeadLetterEmail.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (8)
cloud-functions/src/retryDeadLetterEmail.test.ts (2)
27-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
emailResiliencemock will drift from the real module.The mock exports only
emailCircuitBreaker.isOpenandDEFAULT_RETRY_DELAYS_MS.cloud-functions/src/utils/emailResilience.tsalso exportsEmailCircuitBreaker,BreakerState,retryWithExponentialBackoff,enqueueDeadLetter, andalertAdmins. The tests pass today only becausecloud-functions/src/retryDeadLetterEmail.tsimportsemailCircuitBreakeralone, and because./utils/emailSenderis also mocked.If the handler starts to use another export, the failure appears as an unclear
undefined is not a function. Spread the real module in the mock factory.♻️ Proposed fix
jest.mock('./utils/emailResilience', () => ({ + ...jest.requireActual('./utils/emailResilience'), emailCircuitBreaker: { isOpen: jest.fn(() => false), }, - DEFAULT_RETRY_DELAYS_MS: [1000, 2000, 4000, 8000, 16000], }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud-functions/src/retryDeadLetterEmail.test.ts` around lines 27 - 32, Update the jest.mock factory for emailResilience to spread the real module’s exports, while overriding only emailCircuitBreaker as needed by the test. Preserve DEFAULT_RETRY_DELAYS_MS and ensure EmailCircuitBreaker, BreakerState, retryWithExponentialBackoff, enqueueDeadLetter, and alertAdmins remain available from the mock.
40-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the failure paths and call
testEnv.cleanup().Four tests cover authentication, authorization, validation, and the success path. The paths that issue
#326depends on are untested:
- The circuit breaker open path at
cloud-functions/src/retryDeadLetterEmail.tsLines 44-49.- The non-
resendprovider rejection at Lines 51-56.- The missing entry rejection at Lines 35-37.
- The failure update that writes
status: 'queued'andlastErrorat Lines 81-86.The suite also never calls
testEnv.cleanup(), so thefirebase-functions-testenvironment stays initialized after the run.♻️ Proposed additions
describe('retryDeadLetterEmail', () => { beforeEach(() => { jest.clearAllMocks(); }); + + afterAll(() => { + testEnv.cleanup(); + });Add a case for the failure update:
+ it('marks the entry queued and records the error when the resend fails', async () => { + const admin = require('firebase-admin'); + const docRef = admin.firestore().collection('email_dead_letter_queue').doc('dlq-2'); + docRef.get.mockResolvedValue({ + exists: true, + data: () => ({ to: 'user@example.com', provider: 'resend', retryCount: 0 }), + }); + docRef.update.mockResolvedValue({}); + (sendEmailWithRetry as unknown as jest.Mock).mockResolvedValue({ + success: false, + error: 'RESEND_500', + }); + + const result = await wrapped( + { entryId: 'dlq-2' }, + { auth: { uid: 'a1', token: { admin: true } } } as any, + ); + + expect(result).toEqual({ success: false, entryId: 'dlq-2', error: 'RESEND_500' }); + expect(docRef.update).toHaveBeenCalledWith( + expect.objectContaining({ status: 'queued', retryCount: 1, lastError: 'RESEND_500' }), + ); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud-functions/src/retryDeadLetterEmail.test.ts` around lines 40 - 96, Extend the retryDeadLetterEmail tests with cases covering the open circuit breaker, non-resend provider rejection, missing Firestore entry, and send failure update that writes status 'queued' with lastError; assert each expected rejection or update behavior using the existing mocks. Add testEnv.cleanup() teardown so the firebase-functions-test environment is released after the suite.cloud-functions/src/utils/sendEmailWithRetry.test.ts (2)
36-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe manual state reset needs a
reset()method on the breaker.Line 41 assigns
'CLOSED' as neverto bypass theBreakerStateenum. Lines 40-42 reach into three private-by-convention fields. If the breaker gains a field, every test that resets it becomes stale and starts leaking state between tests.Add a
reset()method toEmailCircuitBreakerincloud-functions/src/utils/emailResilience.ts. Operators also need that method to clear a tripped breaker.♻️ Proposed API addition and test simplification
In
cloud-functions/src/utils/emailResilience.ts:recordSuccess() { this.consecutiveFailures = 0; this.state = BreakerState.CLOSED; this.openedAt = null; } + + /** Clears all breaker state. Use for operator recovery and in tests. */ + reset() { + this.recordSuccess(); + }In this test file:
- emailCircuitBreaker.consecutiveFailures = 0; - emailCircuitBreaker.state = 'CLOSED' as never; - emailCircuitBreaker.openedAt = null; + emailCircuitBreaker.reset();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud-functions/src/utils/sendEmailWithRetry.test.ts` around lines 36 - 44, Replace the test’s direct assignments to emailCircuitBreaker.consecutiveFailures, state, and openedAt with a public reset() call. Add EmailCircuitBreaker.reset() in emailResilience.ts to restore all breaker state to its initial closed condition, and use the same method as the supported operator-facing way to clear a tripped breaker.
70-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test does not assert the dead-letter write that it names.
The test title states that the email is queued to the dead-letter queue. The assertions check only the result, the call count, and the failure counter. The Firestore mock at Lines 16-22 creates a new
addmock on everycollection()call, so no assertion can reach it.Hoist the mocks and assert the write.
♻️ Proposed fix to make the dead-letter write assertable
+const mockDlqAdd = jest.fn().mockResolvedValue({ id: 'dlq-1' }); +const mockCollection = jest.fn(() => ({ add: mockDlqAdd })); jest.mock('firebase-admin', () => ({ - firestore: jest.fn(() => ({ - collection: jest.fn(() => ({ - add: jest.fn().mockResolvedValue({ id: 'dlq-1' }), - })), - })), + firestore: jest.fn(() => ({ collection: mockCollection })), }));expect(mockResendSend).toHaveBeenCalledTimes(6); expect(emailCircuitBreaker.consecutiveFailures).toBeGreaterThan(0); + expect(mockCollection).toHaveBeenCalledWith('email_dead_letter_queue'); + expect(mockDlqAdd).toHaveBeenCalledWith( + expect.objectContaining({ + to: 'user@example.com', + provider: 'resend', + eventId: 'event-1', + status: 'queued', + }), + );
jest.mockis hoisted aboveconstdeclarations, so the mock factory must reference the variables lazily or use themock-prefixed naming that Jest allows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud-functions/src/utils/sendEmailWithRetry.test.ts` around lines 70 - 81, Hoist the Firestore collection and add mocks used by the test setup so repeated collection() calls share the same add mock, while keeping Jest mock-factory references lazy or mock-prefixed. In the “queues the email to the dead-letter queue after exhausting retries” test, assert that the shared add mock was called with the expected dead-letter email payload.cloud-functions/src/sendBulkEmails.ts (2)
192-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
elsebranch is unreachable.
retryWithExponentialBackoffreturns only when the callback returns. The callback at Lines 177-183 throws whenres.okis false. Thereforeresponse.okis always true at Line 192, and Lines 195-197 never run.Remove the condition.
♻️ Proposed simplification
- if (response.ok) { - emailCircuitBreaker.recordSuccess(); - successCount++; - } else { - failureCount++; - } + void response; + emailCircuitBreaker.recordSuccess(); + successCount++;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud-functions/src/sendBulkEmails.ts` around lines 192 - 197, Remove the response.ok conditional in the bulk email processing flow around retryWithExponentialBackoff, since unsuccessful responses are already thrown by the callback. Treat each returned response as successful by recording success and incrementing successCount, and remove the unreachable failureCount branch.
199-216: 🔒 Security & Privacy | 🔵 TrivialRecipient email addresses are written to the error log.
Issue
#326requires failure logging with the email address, so the field is intentional. Confirm that the log sink applies a retention limit and access control that match your privacy policy.cloud-functions/src/utils/emailResilience.tsLine 125 logs the same field.Line 210 also calls
admin.firestore()inside the per-participant handler. Thedbvalue from Line 83 is already in scope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud-functions/src/sendBulkEmails.ts` around lines 199 - 216, Keep the intentional recipient email logging in the bulk failure record, but verify its log sink enforces the required privacy-policy retention and access controls, including the matching logging in emailResilience. In the emailCircuitBreaker alert path, replace the per-participant admin.firestore() call with the existing in-scope db value from the bulk-send setup.Source: Linters/SAST tools
cloud-functions/src/utils/emailSender.ts (1)
158-167: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueThe
attemptsvalue in the dead-letter entry does not reflect the real attempt count.No caller passes
retryContext.attempts, so Line 165 always records1. The send actually made six attempts.cloud-functions/src/sendBulkEmails.tsLine 224 hardcodes6for the same field. Operators reading the queue cannot compare the two providers.Return the attempt count from
retryWithExponentialBackoffthroughonRetry, or count the attempts locally.Line 150 and Line 167 also call
admin.firestore()twice. Reuse thedbvalue.♻️ Proposed fix
+ let attemptCount = 1; const finalResult = await retryWithExponentialBackoff( async () => { const result = await sendEmail(options); if (!result.success) { throw new Error(result.error ?? 'Unknown email send failure'); } return result; }, - { label: `email to ${options.to}` }, + { + label: `email to ${options.to}`, + onRetry: attempt => { + attemptCount = attempt + 1; + }, + }, ).catch((error: unknown) => {emailCircuitBreaker.recordFailure(); + const db = admin.firestore(); if (emailCircuitBreaker.isOpen()) { - const db = admin.firestore(); await alertAdmins(db, {reason: finalResult.error ?? 'Unknown email send failure', - attempts: retryContext.attempts ?? 1, + attempts: retryContext.attempts ?? attemptCount, }; - await enqueueDeadLetter(admin.firestore(), deadLetter); + await enqueueDeadLetter(db, deadLetter);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud-functions/src/utils/emailSender.ts` around lines 158 - 167, Update the dead-letter construction in the email send flow to record the actual number of attempts, rather than defaulting to 1: propagate the count from retryWithExponentialBackoff through onRetry or track it locally, matching the six-attempt behavior used by sendBulkEmails. Also reuse the existing db value when calling enqueueDeadLetter instead of invoking admin.firestore() a second time.cloud-functions/src/utils/emailResilience.test.ts (1)
8-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEnable fake timers in
retryWithExponentialBackofftests.
EmailCircuitBreaker.afterEachrestores real timers before this block runs. Add block-localbeforeEachandafterEachhooks becauserunOnlyPendingTimersAsync()andrunAllTimersAsync()require fake timers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud-functions/src/utils/emailResilience.test.ts` around lines 8 - 22, Add block-local beforeEach and afterEach hooks to the retryWithExponentialBackoff test block, enabling Jest fake timers before each test and restoring real timers afterward. Ensure flushTimers and runOnlyPendingTimersAsync/runAllTimersAsync execute while fake timers are active, without changing the EmailCircuitBreaker hooks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cloud-functions/src/retryDeadLetterEmail.test.ts`:
- Around line 1-96: Format the retryDeadLetterEmail test file with Prettier,
especially the long statements around the mocked Firestore update and wrapped
function invocation. Run the configured Prettier command so all formatting,
including the lines exceeding print width, matches repository conventions.
In `@cloud-functions/src/retryDeadLetterEmail.ts`:
- Around line 1-94: Run Prettier on the retryDeadLetterEmail function file using
the project’s specified command, and retain the resulting formatting changes
without altering the function’s behavior.
- Around line 16-22: Add ownership tracking to DeadLetterEntry and populate the
owner identity in both dead-letter entry producers. In the retryDeadLetterEmail
handler, preserve unrestricted access for admins but require non-admin club
callers to match the entry owner before loading or resending it; reject
mismatches with permission-denied.
- Around line 58-86: Protect the retry flow around the entry read and
sendEmailWithRetry call by claiming the entry in a conditional Firestore
transaction before sending. Only transition an eligible entry to an in-progress
state and increment its retry count within the transaction; abort when another
invocation has already claimed it, and use the claimed count for subsequent
delivered or queued updates in the retryDeadLetterEmail handler.
In `@cloud-functions/src/sendBulkEmails.ts`:
- Around line 161-190: Remove the outer AbortController and setTimeout
declarations near the bulk email retry flow, leaving the controller and timeout
created and cleared within the retryWithExponentialBackoff callback unchanged.
Ensure no references to the removed outer variables remain.
- Line 1: Bound retries in sendBulkEmails and emailSender by passing explicit
short delaysMs values and deadlines that preserve time for the
audit-log/dead-letter writes; do not use the 31-second default inside the
callable request. Prefer writing the dead-letter entry on the first failure and
moving retry processing to a durable Cloud Tasks or Pub/Sub worker so requests
return promptly and retries survive instance restarts.
- Around line 218-226: Update the bulk dead-letter creation in the failed-email
path to use the provider value accepted by retryDeadLetterEmail and persist the
template parameters required for resending. Extend retryDeadLetterEmail’s
provider branching to handle these emailjs entries by calling the EmailJS REST
API, while preserving the existing resend behavior for resend entries.
In `@cloud-functions/src/utils/emailResilience.ts`:
- Around line 43-55: Update the breaker admission logic centered on isOpen() so
it evaluates the cooldown and transitions an OPEN breaker to HALF_OPEN before
allowing one trial; ensure callers such as emailSender and retryDeadLetterEmail
use this behavior without requiring separate tryReset() calls. Also update
failure handling so a failed HALF_OPEN trial reopens the breaker, while
successful trials close it through recordSuccess().
- Around line 98-106: Add an optional templateData field to the DeadLetterEntry
interface, then populate it when constructing dead-letter entries in the
emailSender producer from options.templateData and in the sendBulkEmails
producer from template_params. Preserve the existing retryDeadLetterEmail
consumption path so retries receive the original template values.
In `@cloud-functions/src/utils/emailSender.ts`:
- Around line 129-141: Update retryWithExponentialBackoff in emailResilience.ts
to support an optional RetryOptions.isRetryable predicate and stop retrying when
it returns false for the caught error. In emailSender.ts, classify deterministic
sendEmail failures such as template rendering errors and missing RESEND_API_KEY
as non-retryable, while preserving retries for transient failures and the
existing final failure result.
---
Nitpick comments:
In `@cloud-functions/src/retryDeadLetterEmail.test.ts`:
- Around line 27-32: Update the jest.mock factory for emailResilience to spread
the real module’s exports, while overriding only emailCircuitBreaker as needed
by the test. Preserve DEFAULT_RETRY_DELAYS_MS and ensure EmailCircuitBreaker,
BreakerState, retryWithExponentialBackoff, enqueueDeadLetter, and alertAdmins
remain available from the mock.
- Around line 40-96: Extend the retryDeadLetterEmail tests with cases covering
the open circuit breaker, non-resend provider rejection, missing Firestore
entry, and send failure update that writes status 'queued' with lastError;
assert each expected rejection or update behavior using the existing mocks. Add
testEnv.cleanup() teardown so the firebase-functions-test environment is
released after the suite.
In `@cloud-functions/src/sendBulkEmails.ts`:
- Around line 192-197: Remove the response.ok conditional in the bulk email
processing flow around retryWithExponentialBackoff, since unsuccessful responses
are already thrown by the callback. Treat each returned response as successful
by recording success and incrementing successCount, and remove the unreachable
failureCount branch.
- Around line 199-216: Keep the intentional recipient email logging in the bulk
failure record, but verify its log sink enforces the required privacy-policy
retention and access controls, including the matching logging in
emailResilience. In the emailCircuitBreaker alert path, replace the
per-participant admin.firestore() call with the existing in-scope db value from
the bulk-send setup.
In `@cloud-functions/src/utils/emailResilience.test.ts`:
- Around line 8-22: Add block-local beforeEach and afterEach hooks to the
retryWithExponentialBackoff test block, enabling Jest fake timers before each
test and restoring real timers afterward. Ensure flushTimers and
runOnlyPendingTimersAsync/runAllTimersAsync execute while fake timers are
active, without changing the EmailCircuitBreaker hooks.
In `@cloud-functions/src/utils/emailSender.ts`:
- Around line 158-167: Update the dead-letter construction in the email send
flow to record the actual number of attempts, rather than defaulting to 1:
propagate the count from retryWithExponentialBackoff through onRetry or track it
locally, matching the six-attempt behavior used by sendBulkEmails. Also reuse
the existing db value when calling enqueueDeadLetter instead of invoking
admin.firestore() a second time.
In `@cloud-functions/src/utils/sendEmailWithRetry.test.ts`:
- Around line 36-44: Replace the test’s direct assignments to
emailCircuitBreaker.consecutiveFailures, state, and openedAt with a public
reset() call. Add EmailCircuitBreaker.reset() in emailResilience.ts to restore
all breaker state to its initial closed condition, and use the same method as
the supported operator-facing way to clear a tripped breaker.
- Around line 70-81: Hoist the Firestore collection and add mocks used by the
test setup so repeated collection() calls share the same add mock, while keeping
Jest mock-factory references lazy or mock-prefixed. In the “queues the email to
the dead-letter queue after exhausting retries” test, assert that the shared add
mock was called with the expected dead-letter email payload.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2be504b6-3dcf-47bc-a77b-2a9c21e47207
📒 Files selected for processing (8)
cloud-functions/src/index.tscloud-functions/src/retryDeadLetterEmail.test.tscloud-functions/src/retryDeadLetterEmail.tscloud-functions/src/sendBulkEmails.tscloud-functions/src/utils/emailResilience.test.tscloud-functions/src/utils/emailResilience.tscloud-functions/src/utils/emailSender.tscloud-functions/src/utils/sendEmailWithRetry.test.ts
| const functionsTest = require('firebase-functions-test'); | ||
|
|
||
| jest.mock('firebase-admin', () => { | ||
| const getMock = jest.fn(); | ||
| const updateMock = jest.fn(); | ||
| const collectionMock = jest.fn(() => ({ | ||
| doc: jest.fn(() => ({ | ||
| get: getMock, | ||
| update: updateMock, | ||
| })), | ||
| })); | ||
| return { | ||
| apps: [], | ||
| initializeApp: jest.fn(), | ||
| firestore: jest.fn(() => ({ collection: collectionMock })), | ||
| }; | ||
| }); | ||
|
|
||
| jest.mock('./utils/emailSender', () => { | ||
| const sendEmailWithRetry = jest.fn(); | ||
| return { | ||
| sendEmail: jest.fn(async () => ({ success: true })), | ||
| sendEmailWithRetry, | ||
| }; | ||
| }); | ||
|
|
||
| jest.mock('./utils/emailResilience', () => ({ | ||
| emailCircuitBreaker: { | ||
| isOpen: jest.fn(() => false), | ||
| }, | ||
| DEFAULT_RETRY_DELAYS_MS: [1000, 2000, 4000, 8000, 16000], | ||
| })); | ||
|
|
||
| import { retryDeadLetterEmail } from './retryDeadLetterEmail'; | ||
| import { sendEmailWithRetry } from './utils/emailSender'; | ||
|
|
||
| const testEnv = functionsTest(); | ||
| const wrapped = testEnv.wrap(retryDeadLetterEmail as any); | ||
|
|
||
| describe('retryDeadLetterEmail', () => { | ||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('rejects unauthenticated calls', async () => { | ||
| await expect(wrapped({ entryId: 'x' }, { auth: null } as any)).rejects.toThrow( | ||
| 'You must be signed in.', | ||
| ); | ||
| }); | ||
|
|
||
| it('rejects students', async () => { | ||
| await expect( | ||
| wrapped({ entryId: 'x' }, { auth: { uid: 's1', token: {} } } as any), | ||
| ).rejects.toThrow('Only admins or clubs can retry dead-letter emails.'); | ||
| }); | ||
|
|
||
| it('rejects a missing entryId', async () => { | ||
| await expect( | ||
| wrapped({}, { auth: { uid: 'a1', token: { admin: true } } } as any), | ||
| ).rejects.toThrow(/entryId/); | ||
| }); | ||
|
|
||
| it('re-sends a queued entry and marks it delivered', async () => { | ||
| const admin = require('firebase-admin'); | ||
| admin.firestore().collection('email_dead_letter_queue') // init chain | ||
| .doc('dlq-1') | ||
| .get | ||
| .mockResolvedValue({ | ||
| exists: true, | ||
| data: () => ({ | ||
| to: 'user@example.com', | ||
| subject: 'Hello', | ||
| templateId: 'universal_email_template', | ||
| templateData: {}, | ||
| provider: 'resend', | ||
| retryCount: 1, | ||
| status: 'queued', | ||
| }), | ||
| }); | ||
| admin.firestore().collection('email_dead_letter_queue').doc('dlq-1').update.mockResolvedValue({}); | ||
| (sendEmailWithRetry as unknown as jest.Mock).mockResolvedValue({ | ||
| success: true, | ||
| messageId: 'msg-retry', | ||
| }); | ||
|
|
||
| const result = await wrapped( | ||
| { entryId: 'dlq-1' }, | ||
| { auth: { uid: 'a1', token: { admin: true } } } as any, | ||
| ); | ||
|
|
||
| expect(result).toEqual({ success: true, entryId: 'dlq-1', retryCount: 2 }); | ||
| expect(sendEmailWithRetry).toHaveBeenCalledWith( | ||
| expect.objectContaining({ to: 'user@example.com' }), | ||
| ); | ||
| }); | ||
| }); No newline at end of file |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the Prettier formatting failure.
The CI jobs CI / Lint & Test and PR Validation / validate-functions fail on this file. Line 65-67 and Line 80 exceed the configured print width. Run npx prettier --write cloud-functions/src/retryDeadLetterEmail.test.ts.
🧰 Tools
🪛 GitHub Actions: CI / 0_Lint & Test.txt
[error] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
🪛 GitHub Actions: CI / Lint & Test
[error] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
🪛 GitHub Actions: PR Validation / 1_validate-functions.txt
[error] 1-1: Prettier formatting check failed. Run 'npx prettier --write .' to fix code style issues.
🪛 GitHub Actions: PR Validation / validate-functions
[error] 1-1: Prettier formatting check failed. Run 'npx prettier --write .' to fix code style issues.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloud-functions/src/retryDeadLetterEmail.test.ts` around lines 1 - 96, Format
the retryDeadLetterEmail test file with Prettier, especially the long statements
around the mocked Firestore update and wrapped function invocation. Run the
configured Prettier command so all formatting, including the lines exceeding
print width, matches repository conventions.
Source: Pipeline failures
| import * as admin from 'firebase-admin'; | ||
| import * as functions from 'firebase-functions'; | ||
| import { logger } from './logger'; | ||
| import { sendEmailWithRetry } from './utils/emailSender'; | ||
| import { emailCircuitBreaker } from './utils/emailResilience'; | ||
|
|
||
| /** | ||
| * Manual retry of a failed email from the dead-letter queue (#326). | ||
| * Callable by admins/clubs; re-sends the queued email and updates the | ||
| * queue entry's status/retryCount. | ||
| */ | ||
| export const retryDeadLetterEmail = functions.https.onCall(async (data, context) => { | ||
| if (!context.auth) { | ||
| throw new functions.https.HttpsError('unauthenticated', 'You must be signed in.'); | ||
| } | ||
| const token = context.auth.token; | ||
| if (!token.admin && !token.club) { | ||
| throw new functions.https.HttpsError( | ||
| 'permission-denied', | ||
| 'Only admins or clubs can retry dead-letter emails.', | ||
| ); | ||
| } | ||
|
|
||
| const { entryId } = data ?? {}; | ||
| if (!entryId || typeof entryId !== 'string') { | ||
| throw new functions.https.HttpsError( | ||
| 'invalid-argument', | ||
| 'entryId is required.', | ||
| ); | ||
| } | ||
|
|
||
| const db = admin.firestore(); | ||
| const entryRef = db.collection('email_dead_letter_queue').doc(entryId); | ||
| const snap = await entryRef.get(); | ||
| if (!snap.exists) { | ||
| throw new functions.https.HttpsError('not-found', 'Dead-letter entry not found.'); | ||
| } | ||
|
|
||
| const entry = snap.data() ?? {}; | ||
| if (entry.status === 'delivered') { | ||
| return { success: true, alreadyDelivered: true, entryId }; | ||
| } | ||
|
|
||
| if (emailCircuitBreaker.isOpen()) { | ||
| throw new functions.https.HttpsError( | ||
| 'unavailable', | ||
| 'Email circuit breaker is open; try again later.', | ||
| ); | ||
| } | ||
|
|
||
| if (entry.provider !== 'resend') { | ||
| throw new functions.https.HttpsError( | ||
| 'failed-precondition', | ||
| `Manual retry is only supported for resend entries (got ${entry.provider}).`, | ||
| ); | ||
| } | ||
|
|
||
| const result = await sendEmailWithRetry({ | ||
| to: entry.to, | ||
| subject: entry.subject ?? 'Retried: Previous email failed to send', | ||
| templateName: entry.templateId ?? 'universal_email_template', | ||
| templateData: entry.templateData ?? {}, | ||
| }); | ||
|
|
||
| const retryCount = (entry.retryCount ?? 0) + 1; | ||
| if (result.success) { | ||
| await entryRef.update({ | ||
| status: 'delivered', | ||
| retryCount, | ||
| deliveredAt: new Date().toISOString(), | ||
| lastAttemptAt: new Date().toISOString(), | ||
| }); | ||
| logger.info({ | ||
| message: 'dead-letter email delivered on retry', | ||
| entryId, | ||
| to: entry.to, | ||
| }); | ||
| return { success: true, entryId, retryCount }; | ||
| } | ||
|
|
||
| await entryRef.update({ | ||
| status: 'queued', | ||
| retryCount, | ||
| lastAttemptAt: new Date().toISOString(), | ||
| lastError: result.error ?? 'Unknown error', | ||
| }); | ||
| logger.error({ | ||
| message: 'dead-letter email retry failed', | ||
| entryId, | ||
| to: entry.to, | ||
| error: result.error, | ||
| }); | ||
| return { success: false, entryId, error: result.error }; | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the Prettier formatting failure.
The CI jobs CI / Lint & Test and PR Validation / validate-functions fail on this file. Run npx prettier --write cloud-functions/src/retryDeadLetterEmail.ts.
🧰 Tools
🪛 GitHub Actions: CI / 0_Lint & Test.txt
[error] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
🪛 GitHub Actions: CI / Lint & Test
[error] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
🪛 GitHub Actions: PR Validation / 1_validate-functions.txt
[error] 1-1: Prettier formatting check failed. Run 'npx prettier --write .' to fix code style issues.
🪛 GitHub Actions: PR Validation / validate-functions
[error] 1-1: Prettier formatting check failed. Run 'npx prettier --write .' to fix code style issues.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloud-functions/src/retryDeadLetterEmail.ts` around lines 1 - 94, Run
Prettier on the retryDeadLetterEmail function file using the project’s specified
command, and retain the resulting formatting changes without altering the
function’s behavior.
Source: Pipeline failures
| const token = context.auth.token; | ||
| if (!token.admin && !token.club) { | ||
| throw new functions.https.HttpsError( | ||
| 'permission-denied', | ||
| 'Only admins or clubs can retry dead-letter emails.', | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Any user with the club claim can resend any dead-letter entry.
The handler checks only that the caller holds the admin or club claim. It does not check that the caller owns the entry. The entry is then loaded by caller-supplied entryId at Line 33.
A club user can enumerate entryId values and resend emails addressed to arbitrary recipients, including recipients of another organizer's event. The endpoint becomes a send primitive that bypasses the rate limit enforced in cloud-functions/src/sendBulkEmails.ts Lines 89-112.
DeadLetterEntry in cloud-functions/src/utils/emailResilience.ts Lines 98-106 carries no owner field, so the check cannot be added without a contract change.
Restrict non-admin callers to entries that they created.
🔒️ Proposed ownership check
Add an owner field to DeadLetterEntry and populate it in both producers:
export interface DeadLetterEntry {
to: string;
provider: 'resend' | 'emailjs';
+ /** UID of the principal that triggered the original send. */
+ ownerUid?: string;
subject?: string;Then enforce it here:
const entry = snap.data() ?? {};
+ if (!token.admin && entry.ownerUid !== context.auth.uid) {
+ throw new functions.https.HttpsError(
+ 'permission-denied',
+ 'You can only retry your own dead-letter entries.',
+ );
+ }
if (entry.status === 'delivered') {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloud-functions/src/retryDeadLetterEmail.ts` around lines 16 - 22, Add
ownership tracking to DeadLetterEntry and populate the owner identity in both
dead-letter entry producers. In the retryDeadLetterEmail handler, preserve
unrestricted access for admins but require non-admin club callers to match the
entry owner before loading or resending it; reject mismatches with
permission-denied.
| const result = await sendEmailWithRetry({ | ||
| to: entry.to, | ||
| subject: entry.subject ?? 'Retried: Previous email failed to send', | ||
| templateName: entry.templateId ?? 'universal_email_template', | ||
| templateData: entry.templateData ?? {}, | ||
| }); | ||
|
|
||
| const retryCount = (entry.retryCount ?? 0) + 1; | ||
| if (result.success) { | ||
| await entryRef.update({ | ||
| status: 'delivered', | ||
| retryCount, | ||
| deliveredAt: new Date().toISOString(), | ||
| lastAttemptAt: new Date().toISOString(), | ||
| }); | ||
| logger.info({ | ||
| message: 'dead-letter email delivered on retry', | ||
| entryId, | ||
| to: entry.to, | ||
| }); | ||
| return { success: true, entryId, retryCount }; | ||
| } | ||
|
|
||
| await entryRef.update({ | ||
| status: 'queued', | ||
| retryCount, | ||
| lastAttemptAt: new Date().toISOString(), | ||
| lastError: result.error ?? 'Unknown error', | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Concurrent calls send duplicate emails and lose the retry count.
Lines 34-39 read the entry. Line 58 sends the email. Line 65 computes retryCount from the value that was read. Line 67 or Line 81 writes it back. Nothing prevents a second call from starting between the read and the write.
Two concurrent calls with the same entryId both read retryCount: 1, both send the email, and both write retryCount: 2. The recipient receives the email twice and the counter records one retry. The callable endpoint is reachable from any client, so a double click produces this result.
Claim the entry with a conditional transaction before sending.
🐛 Proposed fix using a claim transaction
+ // Claim the entry so that concurrent calls cannot send it twice.
+ const retryCount = await db.runTransaction(async transaction => {
+ const fresh = await transaction.get(entryRef);
+ const freshData = fresh.data() ?? {};
+ if (freshData.status === 'delivered' || freshData.status === 'retrying') {
+ throw new functions.https.HttpsError(
+ 'aborted',
+ 'This entry is already delivered or a retry is in progress.',
+ );
+ }
+ const next = (freshData.retryCount ?? 0) + 1;
+ transaction.update(entryRef, {
+ status: 'retrying',
+ retryCount: next,
+ lastAttemptAt: new Date().toISOString(),
+ });
+ return next;
+ });
+
const result = await sendEmailWithRetry({
to: entry.to,
subject: entry.subject ?? 'Retried: Previous email failed to send',
templateName: entry.templateId ?? 'universal_email_template',
templateData: entry.templateData ?? {},
});
- const retryCount = (entry.retryCount ?? 0) + 1;
if (result.success) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const result = await sendEmailWithRetry({ | |
| to: entry.to, | |
| subject: entry.subject ?? 'Retried: Previous email failed to send', | |
| templateName: entry.templateId ?? 'universal_email_template', | |
| templateData: entry.templateData ?? {}, | |
| }); | |
| const retryCount = (entry.retryCount ?? 0) + 1; | |
| if (result.success) { | |
| await entryRef.update({ | |
| status: 'delivered', | |
| retryCount, | |
| deliveredAt: new Date().toISOString(), | |
| lastAttemptAt: new Date().toISOString(), | |
| }); | |
| logger.info({ | |
| message: 'dead-letter email delivered on retry', | |
| entryId, | |
| to: entry.to, | |
| }); | |
| return { success: true, entryId, retryCount }; | |
| } | |
| await entryRef.update({ | |
| status: 'queued', | |
| retryCount, | |
| lastAttemptAt: new Date().toISOString(), | |
| lastError: result.error ?? 'Unknown error', | |
| }); | |
| // Claim the entry so that concurrent calls cannot send it twice. | |
| const retryCount = await db.runTransaction(async transaction => { | |
| const fresh = await transaction.get(entryRef); | |
| const freshData = fresh.data() ?? {}; | |
| if (freshData.status === 'delivered' || freshData.status === 'retrying') { | |
| throw new functions.https.HttpsError( | |
| 'aborted', | |
| 'This entry is already delivered or a retry is in progress.', | |
| ); | |
| } | |
| const next = (freshData.retryCount ?? 0) + 1; | |
| transaction.update(entryRef, { | |
| status: 'retrying', | |
| retryCount: next, | |
| lastAttemptAt: new Date().toISOString(), | |
| }); | |
| return next; | |
| }); | |
| const result = await sendEmailWithRetry({ | |
| to: entry.to, | |
| subject: entry.subject ?? 'Retried: Previous email failed to send', | |
| templateName: entry.templateId ?? 'universal_email_template', | |
| templateData: entry.templateData ?? {}, | |
| }); | |
| if (result.success) { | |
| await entryRef.update({ | |
| status: 'delivered', | |
| retryCount, | |
| deliveredAt: new Date().toISOString(), | |
| lastAttemptAt: new Date().toISOString(), | |
| }); | |
| logger.info({ | |
| message: 'dead-letter email delivered on retry', | |
| entryId, | |
| to: entry.to, | |
| }); | |
| return { success: true, entryId, retryCount }; | |
| } | |
| await entryRef.update({ | |
| status: 'queued', | |
| retryCount, | |
| lastAttemptAt: new Date().toISOString(), | |
| lastError: result.error ?? 'Unknown error', | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloud-functions/src/retryDeadLetterEmail.ts` around lines 58 - 86, Protect
the retry flow around the entry read and sendEmailWithRetry call by claiming the
entry in a conditional Firestore transaction before sending. Only transition an
eligible entry to an in-progress state and increment its retry count within the
transaction; abort when another invocation has already claimed it, and use the
claimed count for subsequent delivered or queued updates in the
retryDeadLetterEmail handler.
| @@ -1,6 +1,14 @@ | |||
| import * as admin from 'firebase-admin'; | |||
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Inline retry backoff of 31 seconds runs on the HTTPS callable request thread. DEFAULT_RETRY_DELAYS_MS in cloud-functions/src/utils/emailResilience.ts Line 9 totals 31 seconds across five retries, and retryWithExponentialBackoff sleeps with setTimeout inside the handler. Both call sites therefore hold the caller and the function instance open for the full backoff. The default timeout for a Firebase callable function is 60 seconds, so a persistent provider outage produces a client timeout instead of the intended graceful degradation, and the dead-letter write at the end of the flow never runs.
cloud-functions/src/sendBulkEmails.ts#L161-190: bound the retry budget for the bulk path. The retry runs insidePromise.allSettledover batches of 25, and batches run sequentially, so one failing batch alone can consume the whole callable timeout. Pass a shorterdelaysMsand a deadline that leaves time for the audit-log update at Line 236.cloud-functions/src/utils/emailSender.ts#L129-141: pass an explicitdelaysMsand a deadline instead of relying on the 31-second default, or move the retry to a Cloud Tasks or Pub/Sub worker and return immediately after the first failure.
The durable fix is to retry asynchronously: write the dead-letter entry on the first failure and let a background worker drain the queue. That also makes the retry survive an instance restart, which the current in-process backoff does not.
🧰 Tools
🪛 GitHub Actions: CI / 0_Lint & Test.txt
[error] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
🪛 GitHub Actions: CI / Lint & Test
[error] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
🪛 GitHub Actions: PR Validation / 1_validate-functions.txt
[error] 1-1: Prettier formatting check failed. Run 'npx prettier --write .' to fix code style issues.
🪛 GitHub Actions: PR Validation / validate-functions
[error] 1-1: Prettier formatting check failed. Run 'npx prettier --write .' to fix code style issues.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloud-functions/src/sendBulkEmails.ts` at line 1, Bound retries in
sendBulkEmails and emailSender by passing explicit short delaysMs values and
deadlines that preserve time for the audit-log/dead-letter writes; do not use
the 31-second default inside the callable request. Prefer writing the
dead-letter entry on the first failure and moving retry processing to a durable
Cloud Tasks or Pub/Sub worker so requests return promptly and retries survive
instance restarts.
| const response = await retryWithExponentialBackoff( | ||
| async () => { | ||
| const controller = new AbortController(); | ||
| const timeout = setTimeout(() => controller.abort(), 10000); | ||
| try { | ||
| const res = await fetch( | ||
| 'https://api.emailjs.com/api/v1.0/email/send', | ||
| { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: JSON.stringify(payload), | ||
| signal: controller.signal, | ||
| }, | ||
| ); | ||
| if (!res.ok) { | ||
| throw new Error( | ||
| `EmailJS rejected send: ${res.status} ${await res | ||
| .text() | ||
| .catch(() => '')}`, | ||
| ); | ||
| } | ||
| return res; | ||
| } finally { | ||
| clearTimeout(timeout); | ||
| } | ||
| }, | ||
| { label: `bulk email to ${p.email}` }, | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The outer AbortController and its 10-second timer are now orphaned.
Lines 143-144 still create a controller and a timeout. The fetch call that consumed them was replaced by the retry block. Line 163-164 creates a new controller and timeout inside the retry callback and clears the new timer at Line 186.
The outer timeout is never cleared. Each participant now leaks a pending 10-second timer, and the outer controller is unused. Pending timers keep the Node.js event loop active and can delay the function instance from settling.
Delete the outer AbortController and setTimeout.
🐛 Proposed fix to remove the orphaned timer
- const controller = new AbortController();
- const timeout = setTimeout(() => controller.abort(), 10000);
-
const payload = {
service_id: EMAILJS_SERVICE_ID,Run the following script to confirm that no other statement uses the outer controller or timeout:
#!/bin/bash
# Description: Show every use of controller and timeout in sendBulkEmails.ts.
set -euo pipefail
rg -nP --type=ts '\b(controller|timeout)\b' cloud-functions/src/sendBulkEmails.ts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloud-functions/src/sendBulkEmails.ts` around lines 161 - 190, Remove the
outer AbortController and setTimeout declarations near the bulk email retry
flow, leaving the controller and timeout created and cleared within the
retryWithExponentialBackoff callback unchanged. Ensure no references to the
removed outer variables remain.
| const deadLetter: DeadLetterEntry = { | ||
| to: p.email, | ||
| provider: 'emailjs', | ||
| subject, | ||
| templateId, | ||
| reason, | ||
| attempts: 6, | ||
| }; | ||
| await enqueueDeadLetter(admin.firestore(), deadLetter); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Dead-letter entries from this path cannot be retried manually.
This code writes entries with provider: 'emailjs'. The manual retry endpoint rejects them. cloud-functions/src/retryDeadLetterEmail.ts Lines 51-56 throw failed-precondition when entry.provider !== 'resend'.
Issue #326 requires a dead-letter queue so that failed emails can be retried manually. Bulk emails are the highest-volume path, and they produce entries that no operator can act on. The entries also omit template_params, so the payload needed for a resend is lost.
Persist the template parameters and support the emailjs provider in the retry endpoint, or document that bulk entries are diagnostic only.
🐛 Proposed change to preserve the resend payload
const deadLetter: DeadLetterEntry = {
to: p.email,
provider: 'emailjs',
subject,
templateId,
+ templateData: payload.template_params,
reason,
attempts: 6,
};cloud-functions/src/retryDeadLetterEmail.ts must then branch on entry.provider and call the EmailJS REST API for emailjs entries.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloud-functions/src/sendBulkEmails.ts` around lines 218 - 226, Update the
bulk dead-letter creation in the failed-email path to use the provider value
accepted by retryDeadLetterEmail and persist the template parameters required
for resending. Extend retryDeadLetterEmail’s provider branching to handle these
emailjs entries by calling the EmailJS REST API, while preserving the existing
resend behavior for resend entries.
| /** Allows a single trial request after the cooldown has elapsed. */ | ||
| tryReset(): boolean { | ||
| if (this.state !== BreakerState.OPEN || this.openedAt === null) return false; | ||
| if (Date.now() - this.openedAt >= this.cooldownMs) { | ||
| this.state = BreakerState.HALF_OPEN; | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| isOpen(): boolean { | ||
| return this.state === BreakerState.OPEN; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
The breaker latches open permanently; tryReset is never called.
isOpen() only reads state. It does not consider cooldownMs. The only transition out of OPEN is recordSuccess(), and recordSuccess() runs only after a successful send. Consumers skip sends while isOpen() returns true, so no send can ever succeed again.
Both consumers check isOpen() and no consumer calls tryReset():
cloud-functions/src/utils/emailSender.tsLine 120 returns early.cloud-functions/src/retryDeadLetterEmail.tsLine 44 throwsunavailable.
After five consecutive failures, all email delivery stops for the lifetime of the function instance, including the manual dead-letter retry path that operators need for recovery. The cooldown and HALF_OPEN state are unreachable.
Make the cooldown check part of the admission decision.
🐛 Proposed fix to allow half-open trials
/** Allows a single trial request after the cooldown has elapsed. */
tryReset(): boolean {
if (this.state !== BreakerState.OPEN || this.openedAt === null) return false;
if (Date.now() - this.openedAt >= this.cooldownMs) {
this.state = BreakerState.HALF_OPEN;
return true;
}
return false;
}
+ /**
+ * Returns true when the request must be rejected. Promotes an expired
+ * OPEN breaker to HALF_OPEN so that one trial request is admitted.
+ */
isOpen(): boolean {
- return this.state === BreakerState.OPEN;
+ if (this.state === BreakerState.OPEN) {
+ return !this.tryReset();
+ }
+ return false;
}A trial failure in HALF_OPEN must also re-open the breaker:
recordFailure() {
this.consecutiveFailures += 1;
- if (this.consecutiveFailures >= this.failureThreshold) {
+ if (
+ this.state === BreakerState.HALF_OPEN ||
+ this.consecutiveFailures >= this.failureThreshold
+ ) {
this.state = BreakerState.OPEN;
this.openedAt = Date.now();
}
}Run the following script to confirm that no caller invokes tryReset:
#!/bin/bash
# Description: Find all callers of the breaker API to confirm tryReset is unused.
set -euo pipefail
echo "--- tryReset call sites ---"
rg -nP --type=ts -C3 '\btryReset\s*\(' || echo "no tryReset call sites"
echo "--- isOpen call sites ---"
rg -nP --type=ts -C3 '\bisOpen\s*\('
echo "--- recordSuccess call sites ---"
rg -nP --type=ts -C3 '\brecordSuccess\s*\('🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloud-functions/src/utils/emailResilience.ts` around lines 43 - 55, Update
the breaker admission logic centered on isOpen() so it evaluates the cooldown
and transitions an OPEN breaker to HALF_OPEN before allowing one trial; ensure
callers such as emailSender and retryDeadLetterEmail use this behavior without
requiring separate tryReset() calls. Also update failure handling so a failed
HALF_OPEN trial reopens the breaker, while successful trials close it through
recordSuccess().
| export interface DeadLetterEntry { | ||
| to: string; | ||
| provider: 'resend' | 'emailjs'; | ||
| subject?: string; | ||
| templateId?: string; | ||
| eventId?: string; | ||
| reason: string; | ||
| attempts: number; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
DeadLetterEntry omits templateData, so manual retries render empty templates.
enqueueDeadLetter persists only the fields of DeadLetterEntry. No producer stores templateData:
cloud-functions/src/utils/emailSender.tsLines 158-166 build the entry withoutoptions.templateData.cloud-functions/src/sendBulkEmails.tsLines 218-225 build the entry withouttemplate_params.
The consumer at cloud-functions/src/retryDeadLetterEmail.ts Line 62 reads entry.templateData ?? {}. That field is always undefined. The retried email renders templateName with an empty data object, so all placeholders resolve to empty values. The recipient receives a blank email that the system reports as delivered.
Add the field to the contract and populate it in both producers.
🐛 Proposed contract change
export interface DeadLetterEntry {
to: string;
provider: 'resend' | 'emailjs';
subject?: string;
templateId?: string;
+ /** Placeholder values required to re-render the template on manual retry. */
+ templateData?: Record<string, unknown>;
eventId?: string;
reason: string;
attempts: number;
}In cloud-functions/src/utils/emailSender.ts:
const deadLetter: DeadLetterEntry = {
to: options.to,
provider: 'resend',
subject: options.subject,
templateId: options.templateName,
+ templateData: options.templateData,
eventId: retryContext.eventId,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloud-functions/src/utils/emailResilience.ts` around lines 98 - 106, Add an
optional templateData field to the DeadLetterEntry interface, then populate it
when constructing dead-letter entries in the emailSender producer from
options.templateData and in the sendBulkEmails producer from template_params.
Preserve the existing retryDeadLetterEmail consumption path so retries receive
the original template values.
| const finalResult = await retryWithExponentialBackoff( | ||
| async () => { | ||
| const result = await sendEmail(options); | ||
| if (!result.success) { | ||
| throw new Error(result.error ?? 'Unknown email send failure'); | ||
| } | ||
| return result; | ||
| }, | ||
| { label: `email to ${options.to}` }, | ||
| ).catch((error: unknown) => { | ||
| const reason = error instanceof Error ? error.message : String(error); | ||
| return { success: false, error: reason }; | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Permanent failures are retried six times over 31 seconds.
sendEmail returns success: false for deterministic errors as well as transient ones:
- Line 65-68 returns a template render error.
- Line 83-86 returns
RESEND_API_KEYis not configured.
The wrapper at Lines 130-136 converts every failure into a thrown error, so retryWithExponentialBackoff retries all of them. A missing API key now blocks the caller for 31 seconds, trips the circuit breaker after five calls, and writes a dead-letter entry that manual retry cannot fix.
Fail fast on non-retryable errors.
🐛 Proposed fix to skip retries for deterministic failures
+class NonRetryableEmailError extends Error {}
+
+const NON_RETRYABLE_PATTERNS = [/are not configured/i, /template/i];
+
export async function sendEmailWithRetry( const finalResult = await retryWithExponentialBackoff(
async () => {
const result = await sendEmail(options);
if (!result.success) {
- throw new Error(result.error ?? 'Unknown email send failure');
+ const reason = result.error ?? 'Unknown email send failure';
+ if (NON_RETRYABLE_PATTERNS.some(pattern => pattern.test(reason))) {
+ throw new NonRetryableEmailError(reason);
+ }
+ throw new Error(reason);
}
return result;
},
{ label: `email to ${options.to}` },
).catch((error: unknown) => {retryWithExponentialBackoff must also stop on that error type. Add an isRetryable predicate to RetryOptions in cloud-functions/src/utils/emailResilience.ts and break the loop when the predicate returns false.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloud-functions/src/utils/emailSender.ts` around lines 129 - 141, Update
retryWithExponentialBackoff in emailResilience.ts to support an optional
RetryOptions.isRetryable predicate and stop retrying when it returns false for
the caught error. In emailSender.ts, classify deterministic sendEmail failures
such as template rendering errors and missing RESEND_API_KEY as non-retryable,
while preserving retries for transient failures and the existing final failure
result.
1 participant
Closes #326
Problem
When the email provider fails, events were simply not notified — no retry, no alerting, and no trace of the lost messages (only a ).
Changes
utils/emailResilience.ts(new):EmailCircuitBreaker— opens after 5 consecutive failures, half-open trial after cooldown, closes on successretryWithExponentialBackoff— 1s, 2s, 4s, 8s, 16s (max) with per-attempt loggingenqueueDeadLetter— persists failed sends to theemail_dead_letter_queueFirestore collection with full contextalertAdmins— records circuit-breaker trips toadmin_alerts+ error logutils/emailSender.ts: newsendEmailWithRetrywraps the Resend path with backoff + breaker; final failures are logged with recipient/eventId/reason and queued to the DLQ.sendBulkEmails.ts: per-recipient EmailJS sends now go through the same retry/breaker/DLQ path (timeout scoped per attempt so backoff isn't starved by a single AbortController).retryDeadLetterEmail.ts(new, exported): onCall function (admin/club only) that manually re-sends a queued DLQ entry and tracks retryCount/status.Verification
tsc --noEmitclean.auth/index.test.ts+clubReputation.integration.test.tsare pre-existing (verified identical on pristine main).Summary by CodeRabbit
New Features
Bug Fixes
Tests