[RFC/Prototype] Additive webhook ingestion path for email analytics/suppression (#29828)

[wakqasahmed]

What is this?

A prototype attached to the design discussion in #29828, which itself exists to unblock #29553. Opened as a draft / RFC, not a request to merge as-is — the goal is to give the analytics/suppression refactor a concrete shape to react to, per @9larsons' note on #29553 that "the core team will likely take this over or adapt contributions."

What it does

Adds a webhook-based ingestion path for email analytics/suppression, additive to the existing Mailgun poll loop (email-analytics-service-wrapper.js is untouched):

• EmailProviderBase (proposed standalone in Added email provider adapter wiring #29553) gains two optional methods: verifyWebhookRequest(req) and parseWebhookEvents(req). Both have real (non-throwing) default implementations, so an adapter that only implements send() is unaffected; the controller detects "not overridden" by comparing the instance method against the prototype method, not by return value.
• EmailAnalyticsWebhookController calls those two methods on the active adapter, then forwards each normalized event (delivered / opened / permanent_failed / temporary_failed / unsubscribed / complained) to the same EmailEventProcessor the poll loop already writes through — same suppression list, same domain events, no duplicate logic. Recipients for a whole batch are resolved in one batchGetRecipients() call up front rather than one DB round-trip per event; events within a batch are processed with a bounded concurrency of 5 (not serially, and not unbounded) to stay within the knex pool's real capacity; and flushBatchedUpdates() runs at the end of each batch so this path doesn't silently buffer forever under emailAnalytics:batchProcessing. A batch over 500 events is rejected with 413 rather than accepted and only partially processed.
• Wired as POST /webhooks/email-analytics on the members app, mirroring the existing POST /webhooks/stripe route's raw-body handling — accepts any content-type (not just application/json) since SNS posts text/plain — behind a dedicated per-IP rate limiter (shared.middleware.brute.emailAnalyticsWebhookLimiter, lazily constructed per-request like every other limiter in this codebase) ahead of the adapter's own signature check.
• MailgunEmailSuppressionList's bounce gate now checks an explicit isWebhookSourced flag (set only by this controller) before trusting a permanent-failure event without a Mailgun-shaped 605/607 code. Mailgun's own gate logic — including its own quirk of not suppressing a permanent bounce whose error is non-integer/absent — is completely unchanged; only webhook-sourced events get the bypass.
• adapter-manager's base-classes.ts registers an 'email' base class, so this composes with Added email provider adapter wiring #29553 instead of duplicating it — that PR's getEmailProvider()/adapter-manager wiring is included here as a foundation, since Added email provider adapter wiring #29553 hasn't merged yet and this needs it to run.

What it deliberately doesn't do

• Doesn't touch the Mailgun poll loop, the analytics scheduler, or any batching/aggregation logic — and doesn't yet select an ingestion mode, so a site with both Mailgun credentials and a webhook adapter configured would run both paths at once against the same shared processor. Worth an explicit decision before this goes further than a prototype.
• Doesn't define a specific wire format for any real provider (SES, Postmark, etc.) — that's the adapter's job, by design, so Ghost core doesn't need to know about SNS signatures or Postmark's payload shape.
• Doesn't add a shared-secret/auth config convention — verifyWebhookRequest is deliberately left to the adapter for now.
• Doesn't add replay/idempotency protection. Retries are now whole-batch, at-least-once: a 500 response causes the provider to redeliver the entire batch, including events that already succeeded. delivered/opened tolerate that for free (whereNull guards); complained/unsubscribed do not — a replay re-inserts a complaint record attempt, re-runs a member update, and re-calls the provider's own unsubscribe API. This needs a real answer (an event-id dedup window) before it's more than a prototype, not just a rate limit.

Review history

Two cold-start review rounds so far, each from an agent with no prior context — PR diff and linked issue only.

Round 1 (16 comments) surfaced a reproduced hang bug plus the batch-flush/content-type/retry/suppression-gate/concurrency issues described above in their first-draft form.

Round 2 (12 comments) independently re-verified round 1's fixes against the actual code rather than assuming they held, and found four of them had moved the defect rather than closed it — all four are fixed as of the current commit:

• Unbounded Promise.all over the whole batch defeated the exact pool-exhaustion protection the concurrency fix was supposed to add (each event's 70ms waitForEvent() throttle only works if something bounds concurrent DB work) — now capped at 5 concurrent.
• The batch-flush fix introduced a real race: calling the shared processor's flushBatchedUpdates() from concurrent HTTP requests could silently discard events added while a previous flush was in flight, because it .clear()'d the whole pending map instead of only what it had snapshotted. Reproduced locally, fixed by deleting only the snapshotted keys (a regression test now pins this in newsletter-email-event-storage.test.js).
• The suppression-gate fix changed stock Mailgun behaviour — it keyed off error shape rather than event provenance, and Mailgun genuinely produces a non-integer/absent error code for some real permanent bounces. Fixed with an explicit isWebhookSourced flag threaded through EmailEventProcessor.handlePermanentFailed → EmailBouncedEvent, so only webhook-sourced events bypass the 605/607 check; Mailgun's own gate is back to being untouched.
• The documented "adapter doesn't support webhooks → 501" path was dead code — because the base class defines both methods with real defaults, typeof adapter.verifyWebhookRequest === 'function' was always true for any subclass, so a send-only adapter got 401 instead of 501. Reproduced with a real EmailProviderBase subclass (invisible in the existing tests, which all used plain object doubles). Fixed by comparing the instance method against the prototype method.

Also addressed from round 2: the rate limiter's values were either too weak to matter (10–100ms penalty) or too tight for a real SNS burst (200 req/1000s), and it was the only limiter in the codebase constructed eagerly at route-registration instead of lazily per-request — moved into shared/middleware/brute.js with retuned defaults; EmailProviderBase now stubs all three requiredFns (it previously couldn't satisfy its own contract); email-service-wrapper.js's config.get('adapters:email') check was inconsistent with the controller's own adapters:email:active fix for the identical bug — now consistent.

Deferred, both rounds agree it's out of scope for this PR: the untyped JS base class vs. the typed @tryghost/adapter-base-* siblings (packaging question), and the whole-batch retry/idempotency question above (a contract decision for maintainers, not an implementation detail).

Testing

Ran for real against this repo's own toolchain (scoped pnpm install --filter="ghost...", not simulated):

• npx vitest run across the seven changed/added unit test files — 111/111 passing, including new coverage for a real EmailProviderBase subclass (501 vs 401), invalid timestamps, the per-request event cap, bounded concurrency, and a regression test for the flush race.
• The full email-analytics/email-service/email-suppression-list/adapters/adapter-manager/web unit suites — 1092/1094 passing; the two failures (email-renderer.test.js's hardcoded-date assertion, upload.test.js's SVG-sanitization test under load) both reproduce identically on main and in isolation respectively, unrelated to this change.
• npx eslint on every changed file — zero errors.
• npx tsc --noEmit — only the pre-existing @tryghost/adapter-base-* missing-type-decl errors, confirmed present on main too.

Open questions for maintainers

• Where should verifyWebhookRequest auth actually live — fully adapter-owned (as here), or should Ghost core provide a shared-secret helper adapters call into?
• Is EmailProviderBase the right place for these two methods, or should webhook ingestion be a separate, more explicitly optional interface?
• This lives under the members app for parity with the Stripe webhook, so the public URL is /members/webhooks/email-analytics — email analytics isn't really a members concern. Worth a maintainer's explicit call rather than inheritance-by-proximity.
• Should Ghost select an ingestion mode per adapter (poll vs. webhook) rather than allowing both to run concurrently against the same processor?
• What should the idempotency/replay story be for complained/unsubscribed webhooks specifically, given retries are whole-batch and at-least-once?
• Is a per-IP rate limiter even the right primitive for a machine-to-machine webhook, versus a body-size/concurrency cap alone?

Happy to iterate on any of this, rework it entirely, or step back if the core team wants to take it from here.

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: e58994e1-19ac-4d4d-bd06-aab798d9f4ae

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[wakqasahmed]

Cold-start review of the prototype against the goals in #29828 and the constraints in #29553 / #28247. Verified against the branch locally (npx vitest run on the three new/changed test files: 12 passing; npx eslint clean on all changed files). No merge authority claimed here — COMMENT only.

Verdict: the transport half is basically right, the ingestion-semantics half is not yet. The route/adapter delegation is genuinely additive (poll loop and scheduler untouched, 501 when no adapter, provider specifics stay behind verifyWebhookRequest/parseWebhookEvents), and converging on the existing EmailEventProcessor is the right call. But goal 2 of the issue — "feed suppression events into the same suppression list Mailgun polling currently populates" — is not actually achieved by this diff, and there are two confirmed crash/data-loss defects in the ingestion path. Details inline; summarised by severity:

High

1. parseWebhookEvents() returning a non-array (very easy for an SNS-shaped payload) throws inside handle(), the response is never ended and the rejection escapes into express 4 — request hangs, unhandled rejection. Reproduced locally.
2. Suppression still won't fill for a non-Mailgun adapter: MailgunEmailSuppressionList#init only inserts a suppressions row for EmailBouncedEvent when Number.isInteger(event.error.code) && (code === 607 || code === 605) — Mailgun-specific codes. An SES/Postmark adapter emitting 550/"Permanent" dispatches the domain event and writes nothing.
3. Under emailAnalytics:batchProcessing, handleDelivered/handleOpened/handlePermanentFailed only buffer into NewsletterEmailEventStorage#pendingUpdates; flushBatchedUpdates() is called only from the Mailgun batch processor. With a webhook-only adapter the poll loop never runs, so those events are held in memory and never written.
4. providerId is resolved via email_batches.provider_id (a per-batch id Ghost stored at send time). Webhook providers report a per-recipient message id, so getRecipient() returns undefined and every event is dropped — silently, with no log or metric.

Medium
5. Always 200, even when every event failed → provider never retries, transient DB errors become permanent data loss (contrast the Stripe controller, which 500s).
6. Serial await per event plus the deliberate 70 ms waitForEvent() sleep in handlePermanentFailed/handleComplained means a few hundred bounces exceeds a typical webhook timeout → provider retries → duplicate work.
7. bodyParser.raw({type: 'application/json'}) won't match SNS's text/plain; charset=UTF-8 — req.body ends up {} and the stated first consumer (SES via SNS) 401s on every event.
8. webhookController is export let, undefined until init(); the middleware ignores next and doesn't .catch(next).
9. All getAdapter('email') failures — including genuine misconfiguration and adapter constructor throws — are swallowed into a bare 501 with no logging.
10. Endpoint is unauthenticated and unthrottled, with no replay/idempotency handling.
11. Nothing switches ingestion mode: with an email adapter configured the Mailgun poll jobs are still scheduled.

Low / nits
12. requiredFns lists two methods the base class doesn't stub — EmailProviderBase would fail adapter-manager's own requiredFns validation.
13. Send-path change in email-service-wrapper.js is #29553's scope, and config.get('adapters:email') truthiness diverges from adapter-manager's active-based resolution.
14. Test gaps (non-array return, async verify rejection, parse throwing, empty batch, unknown type, route wiring) and a couple of convention nits.

Also worth doing before this leaves prototype status: issue item 4 (documenting the event contract in the adapter interface docs, not just JSDoc) isn't addressed.

[wakqasahmed]

High — confirmed defect: non-array return hangs the request.

events ?? [] only guards null/undefined. If an adapter returns any non-iterable (an object — e.g. it forwards a raw SNS envelope like {Records: [...]} or {Message: ...} by mistake), for...of throws after the try/catch blocks, so:

• res is never ended → the client socket hangs until timeout,
• the async handle() returns a rejected promise which express 4 ignores (express@4.22.2 here) → unhandledRejection.

Reproduced locally against this branch:

adapter.parseWebhookEvents = () => ({Records: []})
→ REJECTED: (events ?? []) is not iterable   res: {"code":null,"ended":false}

Suggest if (!Array.isArray(events)) { logging.warn(...); res.writeHead(400); return res.end(); }, and wrap the whole body of handle() so nothing can escape without ending the response.

[wakqasahmed]

High — this contract does not actually populate the suppression list, which is goal 2 of #29828.

Suppression rows are written by MailgunEmailSuppressionList#init() (core/server/services/email-suppression-list/mailgun-email-suppression-list.js:117-150), which subscribes to EmailBouncedEvent and gates insertion on:

if (!Number.isInteger(event.error?.code)) { return; }
if (event.error.code !== 607 && event.error.code !== 605) { return; }

607/605 are Mailgun codes. This typedef declares code: number|string, so an SES/Postmark adapter emitting SMTP 550, or a string like "Permanent", dispatches EmailBouncedEvent and then silently writes no suppression row — the exact "suppression list never fills, Ghost keeps emailing dead addresses" failure the issue was filed to fix. Routing through EmailEventProcessor is necessary but not sufficient; the suppression subscriber needs a provider-neutral "is this permanent" signal (or the adapter must be able to state it directly).

Secondary mismatch on the same typedef: EmailEventProcessor documents error as {code: number, message: string, enhancedCode: string|number} and NewsletterEmailEventStorage#saveFailure reads event.error.enhancedCode for the enhanced_code column. enhancedCode is absent here, so it will always land null for webhook-sourced failures. Also note saveFailure early-returns with a logging.warn when error is null — and the controller passes error: event.error ?? null — so a permanent_failed event without error info sets failed_at but records no failure row.

[wakqasahmed]

High — sharing this processor instance loses events under emailAnalytics:batchProcessing.

newsletterEmailEventProcessor wraps a NewsletterEmailEventStorage whose handleDelivered/handleOpened/handlePermanentFailed check config.get('emailAnalytics:batchProcessing') and, when it's on, only accumulate into the private #pendingUpdates maps (newsletter-email-event-storage.js:38-107). Those are drained solely by flushBatchedUpdates(), whose only caller is the Mailgun batch processor (newsletter-email-analytics-batch-processor.js:61).

So with batch processing enabled:

• webhook-ingested delivered/opened/failed events are buffered in memory and never written by this path;
• for a webhook-only adapter the poll loop never runs, so they are never written at all (and are lost on restart);
• if both paths do run, two writers mutate the same unsynchronised #pendingUpdates maps and the flush ordering is arbitrary.

The webhook path either needs to flush after a batch, or to construct its own processor/storage pinned to sequential mode. Worth an explicit test with emailAnalytics:batchProcessing true.

[wakqasahmed]

High — providerId almost certainly won't resolve, and the drop is invisible.

EmailEventProcessor.getRecipient() → getEmailId(providerId) looks the id up in email_batches.provider_id — the batch id Ghost recorded from the send call. Webhook providers report a per-recipient message id (SES/SNS mail.messageId, Postmark MessageID), which will not match any email_batches row. Result: getRecipient() returns undefined, handle*() no-ops, and the controller discards the return value and still answers 200. Every event silently disappears with no log line, no metric, and nothing for an operator to correlate.

Two things needed:

1. The event contract has to carry something resolvable — either emailId/batch id (EmailIdentification already supports emailId, which short-circuits the email_batches lookup entirely) or a documented requirement that the adapter's send() returns per-recipient ids Ghost persists.
2. Log/count unresolved events (the handle* methods return the recipient or undefined — check it) so a broken mapping is diagnosable instead of looking like a healthy 200.

Separately: the EmailIdentification typedef declares providerId as required string while EmailAnalyticsEvent.providerId is optional — if both emailId and providerId are absent getRecipient() bails immediately, so an adapter can silently no-op the whole batch.

[wakqasahmed]

Medium — unconditional 200 turns transient failures into permanent data loss.

processEvent() swallows every error, so a DB outage or a knex pool exhaustion mid-batch still yields 200. Providers treat 2xx as "delivered, never send again": those analytics/suppression events are gone for good, and unlike the poll loop there is no cursor to re-read them from.

The Stripe controller in this repo takes the opposite position — a handler failure produces a non-2xx so Stripe retries. Suggest tracking failures per batch and returning 500 when any event failed (or when the failure is retryable), accepting that the provider will redeliver — which in turn makes the idempotency question below unavoidable rather than optional.

[wakqasahmed]

Medium (design) — "additive" is satisfied structurally, but nothing selects an ingestion mode.

The poll loop is genuinely untouched, which matches the #28247 framing. The flip side is that configuring an email adapter doesn't stop the Mailgun analytics jobs from being scheduled (boot.js:451, backgroundJobs:emailAnalytics) — a site running SES gets recurring Mailgun poll jobs that can only error or no-op, on top of the webhook path. And if a site has both Mailgun credentials and a webhook adapter, two independent writers feed the same processor with no coordination, no shared cursor and no dedup.

Issue items 1 and 3 together imply the ingestion source should be selectable (adapter supports webhooks → skip scheduling the poll jobs for that traffic). Even for a prototype it's worth stating explicitly which combinations are supported, because "both at once" is the configuration most likely to be hit accidentally.

Small note on the comment above: "registered before init() runs" isn't accurate in the normal boot — initExpressApps() builds the app but rootApp.use(ghostApp) happens after initServices(), so the route isn't reachable pre-init. The mutable binding still deserves the guard (see the members/app.js comment), just not for the stated reason.

[wakqasahmed]

Low — the base class can't satisfy its own requiredFns contract.

requiredFns names getMaximumRecipients and getTargetDeliveryWindow, but only send() gets a throwing stub. AdapterManager.getAdapter() iterates requiredFns and throws IncorrectUsageError for any that isn't a function, so EmailProviderBase itself would fail that check — inconsistent with send(), which is stubbed precisely so subclasses inherit a clear failure. Either stub all three or drop the two that aren't defined.

Also 'use strict'; on line 1 isn't used by the surrounding CJS modules in core/server — harmless, but it's a diff-visible divergence in a PR arguing for convention-matching.

[wakqasahmed]

Low — scope, and a config-key mismatch.

Two things:

1. This is the send-side seam, which is Added email provider adapter wiring #29553's diff, not Add webhook-based email analytics/suppression ingestion path to unblock provider adapters (blocking #29553) #29828's. Add webhook-based email analytics/suppression ingestion path to unblock provider adapters (blocking #29553) #29828 is explicitly scoped to analytics/suppression ingestion so it can be reviewed independently of the blocked PR; folding the send seam back in re-couples them. The commit message acknowledges the overlap — but a maintainer reviewing "webhook ingestion" now has to also re-review the sending path.
2. config.get('adapters:email') is a truthiness check on the whole block, whereas AdapterManager resolves via adapters.email.active (resolveAdapterOptions). A config that defines adapters.email without active (or with an unloadable class) makes this branch call getAdapter('email'), which throws IncorrectUsageError from inside emailService.init() in boot.js's Promise.all — i.e. a partially-specified adapter config takes the whole site down at boot instead of falling back to Mailgun. Checking adapters:email:active would match the resolver's own rules.

(Note adapterManager.init() at boot.js:99 only validates configured types, so registering email in base-classes.ts is correctly a no-op for stock installs — that part is fine.)

[wakqasahmed]

Low — coverage gaps; the suite passes but doesn't cover the paths that actually break.

Verified locally: all 12 tests pass and eslint is clean. Uncovered:

• parseWebhookEvents() returning a non-array — the confirmed hang above.
• parseWebhookEvents() throwing → 400 (the branch exists, nothing exercises it).
• verifyWebhookRequest() rejecting vs returning false — the tests only cover the sync false return, so the try/await 401 branch and the sync-throw-vs-rejected-promise distinction are untested (they do behave the same here, but nothing pins that).
• Empty events array, and events being null/undefined (the ?? [] guard).
• Unknown event.type → logging.warn + still 200.
• An event the processor can't resolve (handler returns undefined recipient) — currently indistinguishable from success, which is finding 4.
• emailAnalytics:batchProcessing enabled — finding 3.

Also nothing covers the wiring at all: no test asserts POST /members/webhooks/email-analytics exists, returns 501 on a stock (Mailgun-only) install, or survives being hit before emailAnalytics.init(). That route is the entire security boundary this PR adds; an e2e test asserting the default-install 501 would be the single highest-value test to add.

Nit in email-provider-base.test.js: it deep-equals requiredFns but never asserts the non-writability that the Object.defineProperty(..., {writable: false}) is there to provide.

[wakqasahmed]

Nit — convention divergence. Every other entry in this map is a typed @tryghost/adapter-base-* package; this one is an untyped JS file inside core/server/adapters/email/, requiring a new @ts-expect-error in a file that otherwise has none of its own. It also means getAdapter('email') returns a type derived from an any, so the RegistryOf<BaseClasses> typing this file exists to provide is lost for exactly the new adapter type.

Writing the base class in TS (as redirects/route-settings/sso adapters here already are) would keep getAdapter('email') precisely typed and remove the suppression. Placement is otherwise defensible — core/server/adapters/storage/LocalStorageBase.ts sets the precedent for a non-adapter helper living in an adapter directory.

[wakqasahmed]

Second-pass cold-start review of the current diff (f44a6c1 on top of 9e49299), read independently of the previous round and verified against the branch rather than the fix commit message. Everything below was checked locally: npx vitest run on the four touched test files (28/28 green), eslint on all changed sources (clean), tsc --noEmit (no new errors attributable to this diff), plus throwaway repro tests for the three findings marked "Reproduced on this branch".

How the previous round's fixes held up

Genuinely fixed: the non-array hang (400 + warn, tested), the missing .catch(next) and the undefined controller guard in web/members/app.js, the logging.error(err) on adapter-load failure, the 501-vs-500 split via adapters:email:active, the SNS content-type (type: () => true), the missing-body 400, the unresolved-recipient log line, enhancedCode on the error typedef, and the emailId correlation escape hatch — that last one is the right answer to the providerId/email_batches mismatch, and sending-service.js does hand emailId to send(), so it is implementable.

Fixed in form but not in substance, or newly broken:

• Serialisation → unbounded Promise.all. Removes the timeout, but also removes the waitForEvent() pool throttle it was there to provide, and opens one forUpdate transaction per failure event against a pool of ~10.
• flushBatchedUpdates() on the shared processor. Fixes "batched updates are never written" and introduces "batched updates from a concurrent request are written by nobody" — clear() discards anything accumulated during the flush's awaits. Reproduced.
• Suppression gate. Now suppresses on any bounce whose error.code isn't a Mailgun integer — including error: null, which mailgun-client.js:322 produces for real Mailgun events. That is a behaviour change to the stock poll path, which #29828 item 3 rules out. The gate needs to key off provenance/permanence, not error shape.
• Retry-on-failure. Implemented; the idempotency prerequisite it creates was not.
• Boundary validation. Boolean(event.timestamp) still lets Invalid Date through to moment.utc().format(). Reproduced.
• Rate limiter. New code, not previously reviewed: 10–100 ms penalty is no real defence, while 200 requests/1000 s per IP is well inside a legitimate SNS burst. Also the only route in the codebase that instantiates a brute limiter at registration time instead of via shared/middleware/brute.js.
• Also new and not previously reported: the documented "adapter doesn't support webhooks → 501" path is dead, because EmailProviderBase defines both webhook methods, so such an adapter gets 401. Reproduced.

Unaddressed nits from last round: requiredFns still unstubbed on the base class, email-service-wrapper.js still uses config.get('adapters:email') (now inconsistent with the controller's own fix), base-classes.ts still needs @ts-expect-error.

Still an RFC comment, not a merge gate — the overall direction (adapter owns transport/verification, Ghost owns the normalized contract and the single write path) continues to look right to me. The recurring theme in the list above is that a shared, unsynchronised EmailEventProcessor driven from an unbounded HTTP path is doing work that wants a bounded queue behind the request; that one structural choice accounts for findings 1, 2, 6 and most of 11.

[wakqasahmed]

High — the serialisation fix traded a timeout for unbounded DB concurrency; it defeats the very protection the old code had.

The previous round flagged "serialised processing will time out on real batches" and suggested either enqueue-and-200 or the batched path (batchGetRecipients + recipientCache). This commit took the batched path — good — but also replaced the sequential loop with an unbounded Promise.all over every event in the payload. Those are separate changes and the second one is a regression:

• EmailEventProcessor.handlePermanentFailed()/handleComplained() end with await waitForEvent() — a 70 ms sleep whose stated purpose is "Avoids knex connection pool to run dry" (email-event-processor.js:154,197). Running them all concurrently removes that throttle entirely: the sleeps now overlap, so they cost nothing and protect nothing.
• Each permanent_failed/temporary_failed goes through NewsletterEmailEventStorage#saveFailure, which opens EmailRecipientFailure.transaction() and does a forUpdate: true read (newsletter-email-event-storage.js:132-141). A 500-event SNS batch therefore opens ~500 concurrent transactions, each holding row locks, against a knex pool whose default max is 10. Expected outcome is acquireConnectionTimeout / lock-wait timeouts under load, not a faster response — and because the whole batch then 500s (see the hadFailure comment below), the provider redelivers and does it again.
• batchGetRecipients is passed the unbounded event list too, and builds one orWhere per event (email-event-processor.js:334-341). 5 MB of events (the new body-parser limit) is a very large generated WHERE clause.

Concurrency here needs a bound (p-map/chunked for loop with a small limit) at minimum, and a cap on events per request. The prior review's first suggestion — verify, enqueue, 200, process on the existing job infrastructure — still looks like the correct shape for a webhook handler and would sidestep all three points.

[wakqasahmed]

High — new bug introduced by this commit: calling the shared processor's flushBatchedUpdates() from a concurrent HTTP path silently drops events.

NewsletterEmailEventStorage#flushBatchedUpdates() snapshots each pending map, awaits the DB writes, and then calls .clear() on all three maps (newsletter-email-event-storage.js:275-303). Anything accumulated during those awaits is wiped without ever being written. That was latent-but-unreachable while the only caller was the serialised Mailgun batch processor. This line makes it reachable from an unbounded number of concurrent HTTP requests sharing one newsletterEmailEventProcessor instance (index.ts:69), and concurrent with the poll loop's own flush.

Reproduced on this branch (throwaway vitest against the real storage, fake knex, emailAnalytics:batchProcessing on):

await storage.handleDelivered({emailRecipientId: 'A', timestamp: new Date()});
const flushing = storage.flushBatchedUpdates();   // request A: raw() in flight
await new Promise(r => setImmediate(r));
await storage.handleDelivered({emailRecipientId: 'B', timestamp: new Date()});  // request B
resolveRaw(1); await flushing;
await storage.flushBatchedUpdates();              // second flush writes nothing
// written === [['A']]   -> B silently discarded by clear()

So the fix for "batched updates are never written" now creates "batched updates from a concurrent request are written by nobody". Options: give the webhook path its own EmailEventProcessor/NewsletterEmailEventStorage (which also removes the cross-writer coupling the prior review raised), serialise flushes behind a promise chain, or make flushBatchedUpdates delete only the keys it snapshotted instead of clear()ing.

Also note the flush is unconditional — with emailAnalytics:batchProcessing off it is a pure no-op, and there is still no test with the flag on, which was the specific test the last round asked for.

[wakqasahmed]

High — this changes stock Mailgun behaviour, which issue #29828 item 3 explicitly rules out ("leave the Mailgun poll path untouched").

Before: a bounce with a non-integer/absent error.code was skipped. After: it falls through and writes a suppression row. The gate now keys off the shape of the error, not the source of the event — and nothing on EmailBouncedEvent carries provenance, so there is no way for this code to tell "SES adapter" from "Mailgun".

Mailgun regularly produces exactly that shape. mailgun-client.js:322 sets

error: event['delivery-status'] && (typeof (event['delivery-status'].message || event['delivery-status'].description) === 'string') ? {...} : null

so any Mailgun failed/permanent event whose delivery-status has no string message/description arrives with error: null → previously not suppressed → now suppressed. That is a behaviour change for every existing Mailgun install, shipped inside a PR whose premise is that it is additive. The new test trusts a non-Mailgun adapter permanent bounce with no error object asserts precisely this, but the event it dispatches is indistinguishable from a Mailgun one.

The prior review's actual ask was a provider-neutral permanence signal. That means adding something to the contract (e.g. error.permanent: true, or a suppress flag on the normalized event) that the adapter sets and Mailgun's normalizer does not, and keeping the 605/607 gate as the Mailgun-only fallback. As written the defect isn't fixed, it's inverted: instead of suppressing too little for adapters, it now suppresses too much for Mailgun.

[wakqasahmed]

High — the "adapter doesn't support webhooks" path documented here does not exist; such an adapter gets 401.

This docblock says returning false signals no webhook support and "Ghost responds 501 and does not call parseWebhookEvents". It doesn't. The controller's 501 branch is typeof adapter.verifyWebhookRequest !== 'function' || typeof adapter.parseWebhookEvents !== 'function' (email-analytics-webhook-controller.js:55) — and because the base class defines both methods, every adapter that extends EmailProviderBase passes that check. Control reaches if (!verified) and returns 401.

Reproduced on this branch:

class SES extends EmailProviderBase {}          // send-only adapter, no webhook support
await controller.handle({body: Buffer.from('x')}, res);
// status for send-only adapter: 401     (expected 501)

So the 501 branch only fires for an adapter that does not extend the base class — i.e. exactly the object the unit test at email-analytics-webhook-controller.test.js:67 constructs ({send: async () => {}}), which is why the suite is green while the real path is wrong. Practical effect: an operator running a send-only adapter sees repeated 401 Unauthorized in their provider's webhook dashboard and reasonably concludes their signing secret is wrong.

Fix is either a supportsWebhooks flag / verifyWebhookRequest === EmailProviderBase.prototype.verifyWebhookRequest check in the controller, or don't define these on the base at all and keep the typeof probe honest. Whichever, the test needs to use an actual EmailProviderBase subclass.

[wakqasahmed]

Medium — the boundary validation added for the last round only checks that timestamp is truthy, so the exact defect flagged (a bad timestamp reaching the write path) still lands in the DB.

Boolean(event.timestamp) accepts any non-empty string. processEvent then does new Date(event.timestamp) → Invalid Date, and NewsletterEmailEventStorage does moment.utc(Invalid Date).format('YYYY-MM-DD HH:mm:ss') → the literal string "Invalid date", written into email_recipients.delivered_at / opened_at / failed_at. saveFailure writes the Invalid Date object straight into failed_at and compares it with existing.get('failed_at') > event.timestamp, which is always false for NaN — so the out-of-order guard is disabled too.

Reproduced on this branch:

parseWebhookEvents: () => [{type: 'delivered', email: 'a@b.c', providerId: 'p', timestamp: 'yesterday'}]
// status: 200   forwarded timestamp: Invalid Date   isNaN: true

Number.isNaN(new Date(event.timestamp).getTime()) is a one-line addition. Two other gaps in the same predicate while you're here: event.type is not validated against the known set (an unknown type is only caught later, after it has already been counted as valid and included in the batchGetRecipients query), and neither emailId nor providerId is required — an event with neither is guaranteed to no-op in getRecipient() (email-event-processor.js:209) yet still occupies a slot in the batch lookup.

[wakqasahmed]

Medium — these limiter values neither protect Ghost nor stay out of a real provider's way.

Copied from webmentions_block, but the threat models are different.

• minWait: 10 / maxWait: 100 are milliseconds. Once an attacker exhausts the free retries, the enforced gap is 10–100 ms, i.e. ~10–100 req/s sustained, forever. The stated purpose ("bounds the per-IP request rate ahead of an adapter's own signature verification") isn't achieved — an attacker can still drive unbounded signature verifications, and post-Promise.all each accepted request is now much more expensive than it was.
• freeRetries: 200 / lifetime: 1000 is, on the other side, easy for a legitimate sender to hit. SNS delivers from a small pool of AWS source IPs, and a large newsletter's delivery/open notifications will exceed 200 requests from one IP inside ~16 minutes. The limiter then returns TooManyRequestsError; SNS treats non-2xx as failure, retries with backoff, and eventually drops the notification — the "analytics silently go dark" outcome Add webhook-based email analytics/suppression ingestion path to unblock provider adapters (blocking #29553) #29828 exists to prevent, reintroduced by the mitigation.

Per-IP brute-force counting is a poor fit for a machine-to-machine webhook in general. If the goal is to bound pre-verification work, a body-size cap plus a concurrency cap gets you most of it; if a limiter stays, the values need to be justified against a real provider's burst profile and the failure mode (429 → provider drops events) documented.

Also worth checking whether defaults.json additions need a corresponding update anywhere else — this key is only read via spam.email_analytics_webhook in spam-prevention.js, so a typo in either place fails open silently to {} (ExpressBrute defaults: freeRetries: 2, minWait: 500ms), which would 429 a real provider almost immediately. A test pinning the resolved options would be cheap insurance.

[wakqasahmed]

Medium — not addressed, and now internally inconsistent with the fix that was applied.

The controller was changed this round to check config.get('adapters:email:active') (email-analytics-webhook-controller.js:41) with a comment explaining exactly why the truthiness-of-the-whole-block check is wrong. This line still does config.get('adapters:email'). Two code paths in the same PR now disagree about what "an email adapter is configured" means.

The consequence flagged last time is unchanged: a config that defines adapters.email without active (or with a class that fails to load) makes this branch call adapterManager.getAdapter('email'), which throws IncorrectUsageError out of emailService.init() inside boot.js's Promise.all — a partially-specified adapter config takes the whole site down at boot instead of falling back to Mailgun. Worse now: the webhook endpoint would answer 501 (correctly reporting "not configured") on a site that can't boot at all.

config.get('adapters:email:active') here matches resolveAdapterOptions and makes the two checks agree.

The scope point from last round also stands — this is the send-side seam from #29553, in a PR scoped to analytics ingestion — but that's a maintainer call, not a defect.

[wakqasahmed]

Low — unchanged from last round: the base class still cannot satisfy its own requiredFns.

requiredFns names send, getMaximumRecipients and getTargetDeliveryWindow; only send() has a throwing stub. AdapterManager.getAdapter() iterates requiredFns and throws IncorrectUsageError for any entry that isn't a function (adapter-manager.ts:143), so EmailProviderBase itself fails its own contract. The commit added an assert.throws for the non-writability of the property (good — that was the nit) but left the underlying inconsistency.

Related, and newly relevant: send() now has no @param documentation at all, yet the emailId typedef above tells adapter authors to "echo back" Ghost's email id "via provider-side message tags/metadata set at send time". They can — sending-service.js:138-145 does pass emailId in the send() payload — but nothing in this base class says so. Since emailId is now the recommended correlation mechanism (and the only one that reliably works for per-recipient-id providers like SES/Postmark), the send() payload shape, and specifically the presence of emailId in it, belongs in this file. #29828 item 4 asks for exactly that contract to be documented.

[wakqasahmed]

Low/Medium — the two open design questions from last round are answered only in a comment.

1. Ingestion-mode selection. Configuring a webhook adapter still doesn't stop the Mailgun analytics jobs being scheduled (boot.js, backgroundJobs:emailAnalytics). A site on SES gets recurring poll jobs that can only no-op or error, plus the webhook path. The comment above now says the two "converge here on the same EmailEventProcessor" — which is true, and is also the problem: sharing one processor is what makes the flushBatchedUpdates() race reachable (see the controller comment). At minimum the PR should state which combinations are supported.
2. Lazy binding. The revised comment correctly retracts the "registered before init() runs" claim, and the 503 guard in web/members/app.js is the right belt-and-braces. But the guard is untested, so it will silently rot.

Constructing a dedicated EmailEventProcessor + NewsletterEmailEventStorage for the webhook path here would resolve (1)'s coupling concern and the flush race in one move, at the cost of one extra object per boot. Worth considering before this shape gets baked into the adapter contract.

[wakqasahmed]

Low — the suite grew from 12 to 16 tests and is green, but the four defects reported in this round are all in the gaps.

Verified locally: npx vitest run passes 28/28 across the four touched test files, eslint clean on all changed sources, and tsc --noEmit shows no new errors attributable to this diff (the base-classes.ts TS2307s are pre-existing, from unbuilt workspace packages).

Still uncovered, in rough priority order:

• emailAnalytics:batchProcessing enabled. Explicitly requested last round; the fix is flushBatchedUpdates(), and there is still no test that exercises it with the flag on. A test with two overlapping handle() calls would have caught the clear() race.
• A real EmailProviderBase subclass. Every adapter double in this file is a plain object literal, which is why the 501-vs-401 bug is invisible here. One test using class X extends EmailProviderBase {} pins the send-only-adapter contract.
• Invalid timestamps. 'yesterday', NaN, {} — the "coerces a string timestamp" test only covers the happy ISO string.
• Concurrency. Nothing asserts a bound on how many handlers run at once; a test that counts in-flight calls would make the Promise.all decision explicit rather than incidental.
• Route wiring. Still zero coverage of POST /members/webhooks/email-analytics itself — the 503 guard, the rate limiter, the type: () => true body parser, and the default-install 501. As noted last round, an e2e asserting the stock-install 501 is the single highest-value test this PR could add, since that route is the entire new security boundary.