|
| 1 | +# Automation pipeline: scale review (small/medium workloads) |
| 2 | + |
| 3 | +_Review date: 2026-07-04. Scope: the trigger → enrollment → send pipeline |
| 4 | +(`src/automation/*`, `src/mail/sequence-queue.ts`, `src/mail/sequence-worker.ts`, |
| 5 | +`src/mail/send.ts`)._ |
| 6 | + |
| 7 | +**Verdict:** the architecture (poll-scheduler + durable BullMQ queue + idempotent |
| 8 | +enrollment) is the right shape through medium scale (broadcasts of ~10k+, |
| 9 | +multi-step sequences under load). The findings below were identified before |
| 10 | +pointing real medium-sized workloads at it. Statuses reflect what has been |
| 11 | +fixed vs. deliberately accepted. |
| 12 | + |
| 13 | +## Pipeline recap |
| 14 | + |
| 15 | +1. **Trigger** — event-driven (`fire-event.ts`, called synchronously on tag/contact |
| 16 | + events) or date-based (`process-rules.ts`, 60s polling loop for `DATE_OCCURRED` |
| 17 | + broadcast rules). Both insert rows into `ongoing_sequences` (one row per |
| 18 | + contact-in-a-sequence). |
| 19 | +2. **Scheduler** — `process-ongoing-sequences.ts` polls every 60s for rows whose |
| 20 | + `nextEmailScheduledTime` has passed and enqueues each onto the BullMQ |
| 21 | + `sequence` queue. |
| 22 | +3. **Worker** — `sequence-worker.ts` → `process-ongoing-sequence.ts`: checks quota, |
| 23 | + picks the next published/unsent email, renders (Liquid merge tags + open pixel |
| 24 | + + click-tracked links), sends via `sendMail()`, records an `email_deliveries` |
| 25 | + row, and either schedules the next email or deletes the row (marking a |
| 26 | + broadcast `sent` once every recipient is delivered). |
| 27 | + |
| 28 | +## What was already solid |
| 29 | + |
| 30 | +- **Idempotent enrollment**: unique index on `(sequence_id, contact_id)` plus |
| 31 | + `onConflictDoNothing` means a crash between enrollment and `deleteRule` in |
| 32 | + `process-rules.ts` re-runs harmlessly. |
| 33 | +- **Durable sends**: scheduler/worker decoupling via BullMQ means sends survive |
| 34 | + process restarts. |
| 35 | +- **Retry model**: on send failure the row stays due with `retryCount` |
| 36 | + incremented, so it is retried on a later poll until `sequenceBounceLimit` — |
| 37 | + a reasonable poor-man's backoff. |
| 38 | +- Both polling loops swallow per-item errors, so one bad row cannot stall a tick. |
| 39 | + |
| 40 | +## Findings |
| 41 | + |
| 42 | +### 1. Duplicate enqueues → premature sends in multi-step sequences — **Fixed** |
| 43 | + |
| 44 | +The scheduler re-enqueued **every still-due row on every 60s poll** with no |
| 45 | +`jobId`, so BullMQ did not deduplicate. Whenever the worker backlog exceeded |
| 46 | +60s (any broadcast beyond a few hundred recipients), the queue filled with |
| 47 | +duplicate jobs for the same `ongoingSequenceId`. |
| 48 | + |
| 49 | +- Broadcasts self-healed: the row is deleted after the send, so a duplicate job |
| 50 | + hit the `if (!ongoingSequence) return` guard. |
| 51 | +- Multi-step sequences did **not**: `processOngoingSequence` never re-checked |
| 52 | + that the row was actually due. A duplicate job arriving after the first one |
| 53 | + advanced `nextEmailScheduledTime` found email #1 in `sentEmailIds`, picked |
| 54 | + email #2 as "next", and sent it immediately — skipping its configured delay. |
| 55 | + |
| 56 | +**Fix (both halves applied):** |
| 57 | +- `jobId: ongoingSequence.id` on `sequenceQueue.add()` — BullMQ drops adds whose |
| 58 | + id is already waiting/active/delayed, so a row is never queued twice at once. |
| 59 | +- Dueness guard at the top of `processOngoingSequence` |
| 60 | + (`nextEmailScheduledTime > Date.now()` → return). This is the true safety |
| 61 | + net: it also protects against races if a second worker/instance ever runs. |
| 62 | + |
| 63 | +### 2. Throughput ceiling (~1–3 emails/sec) — **Fixed** |
| 64 | + |
| 65 | +The BullMQ `Worker` used the default concurrency of 1, and each job does |
| 66 | +~5 sequential DB queries, a JSDOM parse, and an SMTP send over a **non-pooled** |
| 67 | +nodemailer transport (a fresh SMTP connection per message). A 10k broadcast |
| 68 | +would take 1–3 hours. |
| 69 | + |
| 70 | +**Fix:** |
| 71 | +- Worker `concurrency: 10`. |
| 72 | +- `pool: true, maxConnections: 5` on the platform default transporter and on |
| 73 | + per-team ESP transporters (`mail/transport.ts` — these are already cached per |
| 74 | + team, so pooling them is safe). |
| 75 | + |
| 76 | +Note: concurrency > 1 makes finding #1's fix a **prerequisite** — without jobId |
| 77 | +dedup + the dueness guard, duplicate jobs for the same row could run |
| 78 | +simultaneously and double-send. |
| 79 | + |
| 80 | +### 3. Unbounded Redis growth — **Fixed** |
| 81 | + |
| 82 | +`sequenceQueue.add()` passed no job options and the Queue had no |
| 83 | +`defaultJobOptions`, so every completed job was kept in Redis forever (BullMQ's |
| 84 | +default). Combined with finding #1, a single 10k broadcast could leave tens of |
| 85 | +thousands of dead job hashes behind. |
| 86 | + |
| 87 | +**Fix:** `defaultJobOptions: { removeOnComplete: true, removeOnFail: 5000 }` on |
| 88 | +the Queue constructor. `removeOnComplete` must be `true` (remove immediately) |
| 89 | +rather than a keep-count: jobs are keyed by ongoing-sequence row id for dedup |
| 90 | +(finding #1), and BullMQ silently ignores an `add` whose jobId still exists in |
| 91 | +the completed set — a lingering completed job would block that row's next email |
| 92 | +tick or retry. Send history lives in logs and `email_deliveries` instead. |
| 93 | + |
| 94 | +### 4. No index on `next_email_scheduled_time` — **Fixed** |
| 95 | + |
| 96 | +`getDueOngoingSequences` full-scanned `ongoing_sequences` every minute. The |
| 97 | +table stays small because completed rows are deleted, but large long-running |
| 98 | +sequence campaigns keep many rows resident. |
| 99 | + |
| 100 | +**Fix:** added a b-tree index on `next_email_scheduled_time` (drizzle migration). |
| 101 | + |
| 102 | +### 5. `countOngoingSequencesForSequence` loaded every row — **Fixed** |
| 103 | + |
| 104 | +It selected all rows for the sequence and returned `rows.length`. After a |
| 105 | +broadcast to N contacts, early cleanup calls pulled thousands of rows just to |
| 106 | +count them. |
| 107 | + |
| 108 | +**Fix:** use SQL `count()`. |
| 109 | + |
| 110 | +## Test coverage |
| 111 | + |
| 112 | +The pipeline is covered by a vitest suite (`pnpm test`) running against an |
| 113 | +in-memory PGlite Postgres with the real drizzle migrations applied (see |
| 114 | +`src/test/db.ts`), so unique indexes, `onConflictDoNothing`, and `jsonb_set` |
| 115 | +behave exactly as in production. Only `sendMail` and the BullMQ queue are |
| 116 | +mocked. Covered: the dueness guard (finding #1's regression), send + delivery |
| 117 | +recording + follow-up scheduling, rendering (merge tags, pixel, click-tracked |
| 118 | +links), broadcast completion, quota skip, missing-contact cleanup, retry and |
| 119 | +bounce-limit handling, enrollment idempotency, due-row selection, jobId-keyed |
| 120 | +enqueueing, and event-triggered enrollment (`fire-event.ts`). |
| 121 | + |
| 122 | +One behavior the tests surfaced: `markBroadcastSent`'s |
| 123 | +`jsonb_set(report, '{broadcast,sentAt}', …)` silently no-ops unless |
| 124 | +`report.broadcast` already exists — which `lockBroadcast` guarantees in the |
| 125 | +real flow (`processRule` locks before any delivery). If broadcasts ever get a |
| 126 | +second enrollment path that skips `lockBroadcast`, `sentAt` would never be |
| 127 | +recorded (status would still flip to `completed`). |
| 128 | + |
| 129 | +## Accepted limitations (documented, not fixed) |
| 130 | + |
| 131 | +- **Single-instance assumption.** The polling loops run inside the API process |
| 132 | + (`startAutomation()` in `index.ts`). Running a second API instance would |
| 133 | + double every enqueue; the jobId dedup and dueness guard make this safe-ish, |
| 134 | + but the design assumes one instance. JSDOM rendering is also CPU work on the |
| 135 | + API's event loop — during a big broadcast, API latency will degrade. When |
| 136 | + "medium" becomes "large", move the scheduler + worker into a separate process. |
| 137 | +- **At-least-once delivery.** A crash between `sendMail()` succeeding and the |
| 138 | + `sentEmailIds` update landing re-sends that email on restart. Standard for |
| 139 | + email pipelines; a transactional outbox is not worth it at this scale. |
| 140 | +- **Quota check is check-then-act.** `hasMailQuotaRemaining` → |
| 141 | + `incrementMailCount` is not atomic, so a team can overshoot its quota by |
| 142 | + roughly the worker concurrency (≤10 emails). Cosmetic. |
| 143 | +- **`getDueOngoingSequences` is unbounded.** All due rows are loaded per poll. |
| 144 | + Fine at this scale since jobId dedup caps queue growth; add a `LIMIT` + |
| 145 | + cursor if tables ever reach 100k+ due rows. |
0 commit comments