From 1243cf161bf095a0fcfa675c41564dd381da0e6a Mon Sep 17 00:00:00 2001 From: oratis Date: Sun, 9 Aug 2026 23:04:18 +0800 Subject: [PATCH 1/2] fix(email): dry-run send no longer strands approved pipeline jobs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With no mail provider configured, the email.send worker took a dev fallback that returned early — before the pipeline_jobs sync, the thread row and the event. So the contact reached status='sent' while pipeline_jobs.stage stayed at 'send' forever: the Pipeline page showed the job stuck mid-flight, "Sent / Monitoring" never incremented, and the thread view was empty. That is every provider-less deployment — which is what CI is, and what a fresh self-host is before Resend is wired up. The branch now skips only the provider call. Everything downstream runs with a synthetic { success: true, messageId: null, provider: 'dry-run' } result, so the flow completes exactly as it would with a provider, minus the send: stage advances to 'monitor', the outbound email_replies row exists, and the 'sent' event carries dryRun: true so nobody mistakes it for a real delivery. Provider ids stay NULL because nothing was sent. The integration test and the Playwright spec both pinned the old behavior as a deliberate canary — the spec's header said in as many words that it should be updated if the dry-run branch ever learned to sync the pipeline row. Both now assert the corrected flow. The stage-gate assertion also stopped hardcoding "send" in the rejection message: the worker races it, so which stage the second approve reports is timing, not behavior. Verified in a browser against a provider-less server: approving moved Awaiting Review 1 → 0 and Sent / Monitoring 0 → 1, the stage track advanced to its final node, and the row reads "Sent". DB after: stage='monitor', email_sent_at set, 1 thread row, 1 event with dryRun: true. Co-Authored-By: Claude Fable 5 --- e2e/tests/pipeline-approve.spec.js | 35 ++++++------ .../pipeline-send-integration.test.js | 34 ++++++++--- server/email-jobs.js | 57 ++++++++++--------- 3 files changed, 73 insertions(+), 53 deletions(-) diff --git a/e2e/tests/pipeline-approve.spec.js b/e2e/tests/pipeline-approve.spec.js index dee4131..2642215 100644 --- a/e2e/tests/pipeline-approve.spec.js +++ b/e2e/tests/pipeline-approve.spec.js @@ -12,16 +12,16 @@ * → enqueues email.send on the in-process queue * the email.send worker (server/email-jobs.js) finds no mail provider * (RESEND_API_KEY / SMTP_* / Gmail OAuth all blank, no mailbox_accounts row) - * and takes its documented dev fallback at email-jobs.js:154-160: + * and takes its dev fallback: it skips the provider call and nothing else. * → contacts.status: 'pending' → 'sent', sent_at set, send_attempts = 1 - * → returns { dryRun: true } *before* the provider-success block, so - * pipeline_jobs.stage stays 'send' (never reaches 'monitor'), - * email_sent_at / smtp_message_id stay NULL, and no email_replies or - * email_events rows are written. + * → pipeline_jobs.stage: 'send' → 'monitor', email_sent_at set + * → an outbound email_replies row and a 'sent' email_event (labelled + * dryRun) are written; provider ids stay NULL because nothing was sent. * - * That last point is a real quirk of the no-provider path, so it is asserted - * explicitly rather than papered over — if the dry-run branch ever learns to - * sync the pipeline row, this spec fails loudly and should be updated. + * This spec previously asserted the opposite and said it should be updated if + * the dry-run branch ever learned to sync the pipeline row. It has: the early + * return used to strand every approved job at 'send' on any provider-less + * deployment — which is what CI is, and what a fresh self-host is. */ const { test, expect } = require('@playwright/test'); @@ -107,14 +107,14 @@ test('approving a review-stage job moves it out of review and marks the contact expect(job.email_approved).toBe(1); expect(job.email_to).toBe(fixture.emailTo); expect(job.error).toBeNull(); - // Canary for the dry-run early-return described in the file header: with no - // mail provider the worker never reaches the pipeline_jobs sync, so the job - // stops at 'send' instead of advancing to 'monitor'. - expect(job.stage).toBe('send'); - expect(job.email_sent_at).toBeNull(); + // The no-provider path runs the same bookkeeping as a real send, minus the + // send: the job advances out of 'send', and the thread row and event exist + // so the UI isn't blank. Provider ids stay NULL because nothing was sent. + expect(job.stage).toBe('monitor'); + expect(job.email_sent_at).toBeTruthy(); expect(job.smtp_message_id).toBeNull(); - expect(countRows('email_replies', 'contact_id = ?', [fixture.contactId])).toBe(0); - expect(countRows('email_events', 'contact_id = ?', [fixture.contactId])).toBe(0); + expect(countRows('email_replies', 'contact_id = ?', [fixture.contactId])).toBe(1); + expect(countRows('email_events', 'contact_id = ?', [fixture.contactId])).toBe(1); }); test('approve is stage-gated: the action disappears and a second approve is rejected', async ({ page }) => { @@ -137,5 +137,8 @@ test('approve is stage-gated: the action disappears and a second approve is reje const second = await approveOnce(); expect(second.status()).toBe(400); - expect((await second.json()).error).toBe('Job is in stage "send", not "review"'); + // Match the gate, not the stage the job happens to have moved on to — the + // worker races this assertion ('send' while queued, 'monitor' once the + // handler finishes), and which side wins is timing, not behavior. + expect((await second.json()).error).toMatch(/^Job is in stage "(send|monitor)", not "review"$/); }); diff --git a/server/__tests__/pipeline-send-integration.test.js b/server/__tests__/pipeline-send-integration.test.js index 5f09fd1..0a77cf7 100644 --- a/server/__tests__/pipeline-send-integration.test.js +++ b/server/__tests__/pipeline-send-integration.test.js @@ -336,10 +336,13 @@ test('approve → send: contact reaches sent and the pipeline job advances to mo assert.equal(queue.getStats().failed, 0); }); -test('no mail provider configured: the dry-run branch marks the contact sent but leaves the job at send', async () => { - // Pins the behavior the Playwright suite observes in CI, where no - // RESEND_API_KEY exists: email-jobs.js:154-160 returns { dryRun: true } - // *before* the pipeline_jobs sync, so the job never reaches 'monitor'. +test('no mail provider configured: dry-run completes the whole flow without calling out', async () => { + // This test used to pin the opposite: the dry-run branch returned early, + // *before* the pipeline_jobs sync, so a provider-less deployment (which is + // what CI is, and what a fresh self-host is) left every approved job + // stranded at stage='send' — the contact said "sent" while the Pipeline + // page showed it stuck mid-flight forever. The branch now skips only the + // provider call; all bookkeeping below it runs. const f = await driveToReview('dryrun'); const mailAgent = makeMailAgent({ configured: false }); const queue = makeQueue(mailAgent); @@ -350,16 +353,29 @@ test('no mail provider configured: the dry-run branch marks the contact sent but const contact = await getContact(contactId); assert.equal(contact.status, 'sent'); assert.ok(contact.sent_at); - assert.equal(contact.provider_message_id, null); + assert.equal(contact.provider_message_id, null, 'nothing was sent, so there is no provider id'); const job = await getJob(f.pipelineJobId); - assert.equal(job.stage, 'send'); - assert.equal(job.email_sent_at, null); + assert.equal(job.stage, 'monitor', 'the job must not be stranded at send'); + assert.ok(job.email_sent_at, 'pipeline row records when the flow completed'); assert.equal(job.smtp_message_id, null); assert.equal(mailAgent.sent.length, 0, 'no provider call may happen in dry-run mode'); - const events = await query('SELECT event_type FROM email_events WHERE contact_id = ?', [contactId]); - assert.equal(events.rows.length, 0); + + // The thread row and the event exist so the UI isn't blank, and the event + // is labelled so nobody mistakes a dry run for a real delivery. + const thread = await query( + "SELECT direction, to_email FROM email_replies WHERE contact_id = ?", [contactId] + ); + assert.equal(thread.rows.length, 1); + assert.equal(thread.rows[0].direction, 'outbound'); + + const events = await query( + 'SELECT event_type, payload FROM email_events WHERE contact_id = ?', [contactId] + ); + assert.equal(events.rows.length, 1); + assert.equal(events.rows[0].event_type, 'sent'); + assert.equal(JSON.parse(events.rows[0].payload || '{}').dryRun, true); }); test('terminal provider failure: contact fails and the pipeline job bounces back to review', async () => { diff --git a/server/email-jobs.js b/server/email-jobs.js index 52d2d0f..578a8b7 100644 --- a/server/email-jobs.js +++ b/server/email-jobs.js @@ -150,38 +150,39 @@ function register({ jobQueue, query, queryOne, exec, mailAgent }) { const mailbox = await resolveMailbox(contact); - // Dev fallback: if no provider config, mark sent (keeps existing behavior). - if (!mailAgent.isConfigured() && !mailbox) { - await exec( - `UPDATE contacts SET status='sent', sent_at=CURRENT_TIMESTAMP, send_error=NULL, last_send_attempt_at=CURRENT_TIMESTAMP, send_attempts=COALESCE(send_attempts,0)+1 WHERE id=?`, - [contactId] - ); - return { dryRun: true }; - } + // Dev fallback: with no provider configured we don't call out to anyone, + // but everything downstream still runs. This used to `return` early, which + // skipped the pipeline_jobs sync below — so on every provider-less + // deployment an approved job reached contacts.status='sent' while + // pipeline_jobs.stage stayed at 'send' forever, and the Pipeline page + // showed it stuck mid-flight with no thread row and no event. + const dryRun = !mailAgent.isConfigured() && !mailbox; await exec( `UPDATE contacts SET send_attempts = COALESCE(send_attempts, 0) + 1, last_send_attempt_at = CURRENT_TIMESTAMP WHERE id = ?`, [contactId] ); - const result = await mailAgent.sendEmail({ - to: emailTo, - subject, - body, - mailboxAccount: mailbox, - // Persist refreshed Gmail tokens so the next send uses the new access_token - onCredsRefreshed: async (fresh) => { - if (!mailbox?.id) return; - try { - await exec( - 'UPDATE mailbox_accounts SET credentials_encrypted = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?', - [secrets.encrypt(fresh), mailbox.id] - ); - } catch (e) { - log.warn('[email-jobs] failed to persist refreshed creds:', e.message); - } - }, - }); + const result = dryRun + ? { success: true, messageId: null, provider: 'dry-run' } + : await mailAgent.sendEmail({ + to: emailTo, + subject, + body, + mailboxAccount: mailbox, + // Persist refreshed Gmail tokens so the next send uses the new access_token + onCredsRefreshed: async (fresh) => { + if (!mailbox?.id) return; + try { + await exec( + 'UPDATE mailbox_accounts SET credentials_encrypted = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?', + [secrets.encrypt(fresh), mailbox.id] + ); + } catch (e) { + log.warn('[email-jobs] failed to persist refreshed creds:', e.message); + } + }, + }); if (!result.success) { // Transient vs terminal distinction: network-ish errors -> throw to retry. @@ -255,10 +256,10 @@ function register({ jobQueue, query, queryOne, exec, mailAgent }) { contactId, providerMessageId: result.messageId, eventType: 'sent', - payload: { provider: result.provider, to: emailTo }, + payload: { provider: result.provider, to: emailTo, dryRun: dryRun || undefined }, }); - return { success: true, messageId: result.messageId }; + return { success: true, messageId: result.messageId, dryRun: dryRun || undefined }; }); // ---- email.batch_send ---- From 0a710923efd7b1cae0c72b1b6b2667095951c5a6 Mon Sep 17 00:00:00 2001 From: oratis Date: Sun, 9 Aug 2026 23:13:33 +0800 Subject: [PATCH 2/2] fix(email): warn when the dry-run path marks a send that never happened MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on this PR. The fix is right, but it removes the only accidental signal that a deployment has no mail provider: jobs used to strand at pipeline stage 'send', which at least looked wrong. Now the flow reports complete success — contact 'sent', stage 'monitor', a thread row, a 'sent' event — and the dry-run branch itself logs nothing at all. That matters beyond dev because nothing fails fast on a missing provider: server/email.js isConfigured() is a plain boolean check and no boot guard consults it. A production revision that lost RESEND_API_KEY would report every outreach as delivered, with `provider: 'dry-run'` buried in the email_events payload as the only tell. Adds a log.warn naming the three things it looked for and the contact it is about to mark sent without sending. No behavior change — the synthetic result, the bookkeeping and the dryRun labelling are untouched. Verified: node --test server/__tests__/{pipeline-send-integration,email-jobs}.test.js → 18/18. Full suite 678/678 on this branch merged onto main (serialized; the 4 SQLITE_BUSY failures on a default parallel run are the known shared influencex.db flake and reproduce on main alone). Co-Authored-By: Claude Opus 5 --- server/email-jobs.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/server/email-jobs.js b/server/email-jobs.js index 578a8b7..e413b58 100644 --- a/server/email-jobs.js +++ b/server/email-jobs.js @@ -157,6 +157,16 @@ function register({ jobQueue, query, queryOne, exec, mailAgent }) { // pipeline_jobs.stage stayed at 'send' forever, and the Pipeline page // showed it stuck mid-flight with no thread row and no event. const dryRun = !mailAgent.isConfigured() && !mailbox; + if (dryRun) { + // Everything below records a fully successful send, so this line is the + // only operator-facing signal that nothing actually went out. Nothing + // fails fast at boot without a mail provider (email.js isConfigured() is + // just a boolean), so this path is reachable in production — where it + // means outreach is silently going nowhere. Warn, don't swallow. + log.warn( + `[email-jobs] dry-run: no mail provider configured (RESEND_API_KEY / SMTP_* / mailbox account all absent) — contact ${contactId} will be marked sent without sending` + ); + } await exec( `UPDATE contacts SET send_attempts = COALESCE(send_attempts, 0) + 1, last_send_attempt_at = CURRENT_TIMESTAMP WHERE id = ?`,