Skip to content

Changed batched newsletter events to use exact transactional transitions - #30668

Draft
jonatansberg wants to merge 5 commits into
jonatan-ber-3932-linear-event-result-mergingfrom
jonatan-ber-3933-transactional-email-events
Draft

Changed batched newsletter events to use exact transactional transitions#30668
jonatansberg wants to merge 5 commits into
jonatan-ber-3932-linear-event-result-mergingfrom
jonatan-ber-3933-transactional-email-events

Conversation

@jonatansberg

Copy link
Copy Markdown
Member

Batched newsletter ingestion currently writes delivery, open and failure timestamps separately. To replace repeated analytics recounts with counters, we first need to know exactly which recipients changed, and keep those changes inside a transaction that future counter updates can share.

This PR groups pending events by email and returns the exact recipient/member sets whose timestamps changed from NULL. Duplicate events return zero transitions. Delivery, open and failure remain independent facts, and the earliest timestamp within a pending batch wins.

for each email, in ID order
  begin transaction
    lock eligible recipients in primary-key order
    apply guarded delivery / open / failure updates
    collect the exact transitioned recipient and member sets
  commit

advance the page cursor after every email has committed

The locking read matters because an affected-row count cannot tell us which members changed. It uses FORCE INDEX (PRIMARY) and ORDER BY id; sorting an IN list alone would not establish lock order. The MySQL test checks the actual query plan and races competing flushes alongside batch-creation inserts. Deadlocks retry the whole transaction, up to three attempts. Other errors propagate so the page can be replayed.

Existing recounts remain responsible for counters. We also retain replayed members in the recount queue: a previous run may have committed recipient facts and failed before updating statistics. Skipping duplicate-driven member recounts is deferred until member counters commit atomically with recipient transitions. Sequential processing and automation/gift storage keep their existing behavior.

Validation:

  • 102 email-service MySQL integration tests and 180 analytics unit tests passed; 27 storage unit tests passed.
  • Regression coverage includes duplicate replay, out-of-order timestamps, independent event types, transaction rollback/retry, contention, multiple emails, partial progress and a lost commit acknowledgement.
  • Core and test TypeScript checks, focused ESLint and commit hooks passed. Full pnpm check stopped at existing formatting issues in 452 unrelated files; no changed file appears in that list.
  • Five independent review passes covered each slice and the combined PR 1–2 stack. The recovery finding led to retaining replay recounts; re-review found no remaining high-confidence issues.

A local synthetic comparison used 5,000 members, 500 historical emails and 2,009,528 recipient rows, with 300-event pages and unchanged recounts. Median handler reads over three measured cycles were:

Duplicate opens Previous This PR
0% 4,000,418 4,005,418
50% 4,000,418 4,002,918
100% 4,000,418 4,000,418

Recounts still dominate; this step establishes correctness for the later counter change. The harness uses innodb_flush_log_at_trx_commit=0 and no binary log. Local timing varied, so these results make no production write-latency or throughput claim.

Stacked on #30667. Future counter updates belong inside the recipient transaction.

ref https://linear.app/ghost/issue/BER-3933/make-batched-email-event-writes-transactional-with-exact-per-email

@coderabbitai

coderabbitai Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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

@nx-cloud

nx-cloud Bot commented Sep 10, 2026

Copy link
Copy Markdown

🤖 Nx Cloud AI Fix

Ensure the fix-ci command is configured to always run in your CI pipeline to get automatic fixes in future runs. For more information, please see https://nx.dev/ci/features/self-healing-ci


View your CI Pipeline Execution ↗ for commit b3e2c0d

Command Status Duration Result
nx run ghost:test:ci:integration ✅ Succeeded 4m 42s View ↗
nx run ghost:test:integration ✅ Succeeded 2m 25s View ↗
nx run ghost:test:ci:e2e ✅ Succeeded 4m 13s View ↗
nx run ghost:test:legacy ✅ Succeeded 3m 17s View ↗
nx run ghost:test:e2e ✅ Succeeded 1m 59s View ↗
nx run-many -t test:unit -p ghost ✅ Succeeded 34s View ↗
nx run ghost-monorepo:lint:boundaries ✅ Succeeded <1s View ↗
nx run-many -t lint -p ghost,ghost-monorepo ✅ Succeeded 17s View ↗
Additional runs (4) ✅ Succeeded ... View ↗

💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗


☁️ Nx Cloud last updated this comment at 2026-09-11 09:23:45 UTC

@codecov

codecov Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.10390% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.75%. Comparing base (505a1a7) to head (b3e2c0d).

Files with missing lines Patch % Lines
...es/email-service/newsletter-email-event-storage.js 95.38% 3 Missing ⚠️
Additional details and impacted files
@@                               Coverage Diff                                @@
##           jonatan-ber-3932-linear-event-result-merging   #30668      +/-   ##
================================================================================
+ Coverage                                         67.71%   67.75%   +0.03%     
================================================================================
  Files                                              1676     1670       -6     
  Lines                                             60534    60349     -185     
  Branches                                          10466    10436      -30     
================================================================================
- Hits                                              40993    40891     -102     
+ Misses                                            17221    17151      -70     
+ Partials                                           2320     2307      -13     
Flag Coverage Δ
e2e-tests 70.52% <96.10%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ref https://linear.app/ghost/issue/BER-3933/make-batched-email-event-writes-transactional-with-exact-per-email

Lock eligible recipients before applying opens so duplicates return no transitions and later counters can use the exact committed member set.
ref https://linear.app/ghost/issue/BER-3933/make-batched-email-event-writes-transactional-with-exact-per-email

Use the locked eligible recipient sets for independent delivery, open and failure changes. Retry rolled-back deadlocks as whole transactions, preserving exact transition results under competing flushes and batch creation.
ref https://linear.app/ghost/issue/BER-3933/make-batched-email-event-writes-transactional-with-exact-per-email

Keep replayed members eligible for recount until counters share the recipient transaction, preserving recovery after a crash or lost commit acknowledgement.
ref https://linear.app/ghost/issue/BER-3933/make-batched-email-event-writes-transactional-with-exact-per-email

Review found that a page which failed after some emails had committed
kept every pending entry, including the committed ones, and that a
failure before the flush (for example while saving a permanent failure)
left the queued page behind entirely. The storage is shared by every
fetch job, so the next job would flush those entries and report another
page's transitions as its own, and a persistently failing email re-ran
a locking read for every accumulated email on each attempt. The batch
processor now owns the page lifecycle: pending updates are discarded
whether or not the flush ran or succeeded. The caller already keeps its
cursor behind a failed page and replays it, and the guarded updates make
that replay a no-op for rows that did commit.

Lock waits are as likely as deadlocks while batch creation inserts
recipients for the same email, so ER_LOCK_WAIT_TIMEOUT is retried too,
with jitter. The guarded update now asserts it matched the locked set,
so a database without row locks fails loudly instead of reporting a
transition that did not happen. The three batched handlers share one
queueing helper, MySQL detection reuses DatabaseInfo, and the JSDoc
return type now references the instance method rather than the class.
The MySQL plan checks also assert a primary-key range access path for
a multi-type page.
@jonatansberg
jonatansberg force-pushed the jonatan-ber-3933-transactional-email-events branch from d0807aa to b9e963b Compare September 11, 2026 06:41
ref https://linear.app/ghost/issue/BER-3933/make-batched-email-event-writes-transactional-with-exact-per-email

New test files follow the repository's TypeScript direction, which the
CodeRabbit "New files are TypeScript" check enforces for this PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant