fix: skip failing contacts in campaign batch instead of halting#411
fix: skip failing contacts in campaign batch instead of halting#411tejassinghbhati wants to merge 1 commit into
Conversation
|
@tejassinghbhati is attempting to deploy a commit to the kmkoushik's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughIn 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/web/src/server/service/campaign-service.ts`:
- Around line 1037-1060: The catch in campaign-service’s contact processing path
is too broad and currently swallows systemic failures while the batch still
advances lastCursor. In processContactEmail handling inside the contact loop,
keep skip-and-continue only for explicitly contact-scoped, non-retriable errors,
and rethrow all other exceptions so the surrounding batch can retry safely
instead of permanently skipping contacts. Use the existing logger.error block
for the non-retriable path, and update the logic around the campaign/contact
processing flow so lastCursor only advances when the batch is safe to commit.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8b85a09-bf70-447a-b4fc-2e552e2229ea
📒 Files selected for processing (1)
apps/web/src/server/service/campaign-service.ts
| try { | ||
| await processContactEmail({ | ||
| contact, | ||
| campaign, | ||
| allowedVariables, | ||
| emailConfig: { | ||
| from: campaign.from, | ||
| subject: campaign.subject, | ||
| replyTo: Array.isArray(campaign.replyTo) ? campaign.replyTo : [], | ||
| cc: Array.isArray(campaign.cc) ? campaign.cc : [], | ||
| bcc: Array.isArray(campaign.bcc) ? campaign.bcc : [], | ||
| teamId: campaign.teamId, | ||
| campaignId: campaign.id, | ||
| previewText: campaign.previewText ?? undefined, | ||
| domainId: domain.id, | ||
| region: domain.region, | ||
| }, | ||
| }); | ||
| } catch (err) { | ||
| logger.error( | ||
| { err, contactId: contact.id, campaignId }, | ||
| "Failed to process contact; skipping to next", | ||
| ); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not swallow systemic failures while still advancing lastCursor
Line 1037 and Line 1055 currently catch all exceptions and continue, so transient infra failures (DB/Redis/queue) can skip contacts permanently once the cursor advances at Line 1065. Skip-and-continue should be limited to explicitly contact-scoped, non-retriable errors; rethrow the rest so the batch can retry safely.
Suggested direction
+ const isSkippableContactError = (err: unknown) =>
+ err instanceof TemplateVariableError ||
+ err instanceof MissingContactDataError;
+
for (const contact of contacts) {
if (existingSet.has(contact.id)) continue;
try {
await processContactEmail({
// ...
});
} catch (err) {
logger.error(
{ err, contactId: contact.id, campaignId },
"Failed to process contact; skipping to next",
);
+ if (!isSkippableContactError(err)) throw err;
}
}📝 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.
| try { | |
| await processContactEmail({ | |
| contact, | |
| campaign, | |
| allowedVariables, | |
| emailConfig: { | |
| from: campaign.from, | |
| subject: campaign.subject, | |
| replyTo: Array.isArray(campaign.replyTo) ? campaign.replyTo : [], | |
| cc: Array.isArray(campaign.cc) ? campaign.cc : [], | |
| bcc: Array.isArray(campaign.bcc) ? campaign.bcc : [], | |
| teamId: campaign.teamId, | |
| campaignId: campaign.id, | |
| previewText: campaign.previewText ?? undefined, | |
| domainId: domain.id, | |
| region: domain.region, | |
| }, | |
| }); | |
| } catch (err) { | |
| logger.error( | |
| { err, contactId: contact.id, campaignId }, | |
| "Failed to process contact; skipping to next", | |
| ); | |
| } | |
| const isSkippableContactError = (err: unknown) => | |
| err instanceof TemplateVariableError || | |
| err instanceof MissingContactDataError; | |
| try { | |
| await processContactEmail({ | |
| contact, | |
| campaign, | |
| allowedVariables, | |
| emailConfig: { | |
| from: campaign.from, | |
| subject: campaign.subject, | |
| replyTo: Array.isArray(campaign.replyTo) ? campaign.replyTo : [], | |
| cc: Array.isArray(campaign.cc) ? campaign.cc : [], | |
| bcc: Array.isArray(campaign.bcc) ? campaign.bcc : [], | |
| teamId: campaign.teamId, | |
| campaignId: campaign.id, | |
| previewText: campaign.previewText ?? undefined, | |
| domainId: domain.id, | |
| region: domain.region, | |
| }, | |
| }); | |
| } catch (err) { | |
| logger.error( | |
| { err, contactId: contact.id, campaignId }, | |
| "Failed to process contact; skipping to next", | |
| ); | |
| if (!isSkippableContactError(err)) throw err; | |
| } |
🤖 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 `@apps/web/src/server/service/campaign-service.ts` around lines 1037 - 1060,
The catch in campaign-service’s contact processing path is too broad and
currently swallows systemic failures while the batch still advances lastCursor.
In processContactEmail handling inside the contact loop, keep skip-and-continue
only for explicitly contact-scoped, non-retriable errors, and rethrow all other
exceptions so the surrounding batch can retry safely instead of permanently
skipping contacts. Use the existing logger.error block for the non-retriable
path, and update the logic around the campaign/contact processing flow so
lastCursor only advances when the batch is safe to commit.
|
ty, took the commit and worked on top of it |
Closes #408
Problem
The campaign batch worker had no per-contact error handling. If
processContactEmailthrew for any single contact (bad template variables, missing region queue, etc.), the entireforloop exited immediately — before the cursor update ran. The campaign would stayRUNNINGwithlastCursorfrozen, and the scheduler would re-enqueue the same batch every ~1.5s, hitting the same failing contact in an infinite loop.Fix
Wrapped
processContactEmailin atry/catchinside the loop. A failing contact is now logged and skipped; the loop continues to the remaining contacts and the cursor advances normally at the end.What changes
apps/web/src/server/service/campaign-service.ts— added try/catch aroundprocessContactEmailcall in the batch worker loopSummary by cubic
Prevent campaign batches from halting on a single failing contact by skipping errors and continuing, so campaigns progress and the scheduler stops re-enqueuing the same batch.
processContactEmailin a try/catch inside the per-contact loop.contactIdandcampaignId, then skip to the next contact.RUNNINGstate and infinite re-enqueue.Written for commit 306a9dd. Summary will update on new commits.
Summary by CodeRabbit