🐛 Fixed members inheriting stale suppression state on resignup#27408
🐛 Fixed members inheriting stale suppression state on resignup#27408jonatansberg merged 3 commits intomainfrom
Conversation
WalkthroughThe PR refactors Mailgun suppression handling to validate bounce codes before attempting database insertion, narrows the try/catch to the suppression add call, and moves dispatch of the 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
ref ONC-1640 When a member whose email was previously suppressed was deleted and a new member signed up with the same address, the new member was created with email_disabled=false despite the suppression record still existing. The send filter then included them in every newsletter, Mailgun rejected each send with a 607, and the handler silently swallowed the ER_DUP_ENTRY from trying to re-insert the existing suppression record — so email_disabled was never corrected. - member-bread-service.add() now checks the suppression list and sets email_disabled, mirroring edit() - mailgun-email-suppression-list handleEvent now dispatches EmailSuppressedEvent even on ER_DUP_ENTRY, self-healing drifted members on their next bounce/complaint
jonatansberg
force-pushed
the
fix/onc-1640-suppression-check-on-member-add
branch
from
45642dd to
abfbb2e
Compare
The ONC-1640 fix only checked for MySQL's ER_DUP_ENTRY when catching duplicate suppression inserts, so on SQLite the SQLITE_CONSTRAINT error was treated as an unknown failure and the EmailSuppressedEvent dispatch was skipped. This broke the regression test on the sqlite3 CI job.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ghost/core/test/unit/server/services/members/members-api/services/members-bread-service.test.js (1)
186-196: Optional: hoist the duplicatedcreateMockMemberModel.SonarCloud flagged the inner
createMockMemberModelas duplicating the one in theadddescribe. The two helpers differ slightly (this one is simpler and uses a stubbedrelatedreturningfind), so a straight hoist isn't free, but a small parameterized factory at module scope would dedupe both. Strictly optional — leaving as-is is fine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/members/members-api/services/members-bread-service.test.js` around lines 186 - 196, Hoist and consolidate the duplicated createMockMemberModel by moving it to module scope as a small parameterized factory that can produce the two variants used in the tests (the simpler one with related().find and the more complex one used in the "add" describe); update calls in both describe blocks to call this factory with a flag or options object (e.g., createMockMemberModel({ withRelatedFind: true }) or similar) so the helper is reused while preserving each test's differences; ensure the exported/local symbol remains named createMockMemberModel to minimize changes to tests that reference it.ghost/core/test/integration/services/mailgun-email-suppression-list.test.js (1)
212-213: Optional: also reset the member'semail_disabledin cleanup.The cleanup removes the
Suppressionrow but leavesmembers.email_disabled=truefor recipient[5]. Today that's safe because recipient[5] is only used by this test, but it's a footgun if anyone later adds a test that reuses this recipient and asserts an unsuppressed starting state. Cheap to harden.🧹 Defensive cleanup
// Clean up so subsequent tests using this email aren't affected await models.Suppression.destroy({destroyBy: {email: recipient}}); + await models.Member.edit({email_disabled: false}, {id: memberId});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/integration/services/mailgun-email-suppression-list.test.js` around lines 212 - 213, After removing the suppression row with models.Suppression.destroy({destroyBy: {email: recipient}}), also reset the corresponding member's email_disabled flag back to false to avoid side effects; locate the cleanup block that uses the recipient variable and add an update to the Member model (e.g., find the member by email and set email_disabled: false or run an update where email: recipient) so tests start with an unsuppressed member state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ghost/core/core/server/services/email-suppression-list/mailgun-email-suppression-list.js`:
- Around line 135-143: Tighten the SQLite constraint check in the catch block
that currently tests err.code === 'SQLITE_CONSTRAINT' so it only treats
true-duplicate unique-constraint failures as equivalent to MySQL's ER_DUP_ENTRY;
update the logic around the existing error handling (the catch handling that
currently calls logging.error or logs the "Re-dispatching EmailSuppressedEvent"
via logging.info) to accept either err.code === 'ER_DUP_ENTRY' OR (err.code ===
'SQLITE_CONSTRAINT' AND the err.message indicates a UNIQUE/duplicate constraint
failure), otherwise log the error and return; mirror the existing
uniqueness-message pattern used in webhooks-service.js and keep
EmailSuppressedEvent re-dispatch behavior only for confirmed unique-violation
cases.
---
Nitpick comments:
In `@ghost/core/test/integration/services/mailgun-email-suppression-list.test.js`:
- Around line 212-213: After removing the suppression row with
models.Suppression.destroy({destroyBy: {email: recipient}}), also reset the
corresponding member's email_disabled flag back to false to avoid side effects;
locate the cleanup block that uses the recipient variable and add an update to
the Member model (e.g., find the member by email and set email_disabled: false
or run an update where email: recipient) so tests start with an unsuppressed
member state.
In
`@ghost/core/test/unit/server/services/members/members-api/services/members-bread-service.test.js`:
- Around line 186-196: Hoist and consolidate the duplicated
createMockMemberModel by moving it to module scope as a small parameterized
factory that can produce the two variants used in the tests (the simpler one
with related().find and the more complex one used in the "add" describe); update
calls in both describe blocks to call this factory with a flag or options object
(e.g., createMockMemberModel({ withRelatedFind: true }) or similar) so the
helper is reused while preserving each test's differences; ensure the
exported/local symbol remains named createMockMemberModel to minimize changes to
tests that reference it.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3124ad7a-b007-4301-893d-da040df34b6a
📒 Files selected for processing (4)
ghost/core/core/server/services/email-suppression-list/mailgun-email-suppression-list.jsghost/core/core/server/services/members/members-api/services/member-bread-service.jsghost/core/test/integration/services/mailgun-email-suppression-list.test.jsghost/core/test/unit/server/services/members/members-api/services/members-bread-service.test.js
| } catch (err) { | ||
| if (err.code !== 'ER_DUP_ENTRY') { | ||
| if (err.code !== 'ER_DUP_ENTRY' && err.code !== 'SQLITE_CONSTRAINT') { | ||
| logging.error(err); | ||
| return; | ||
| } | ||
| // Suppression already exists — still dispatch so any drifted | ||
| // member state (e.g. email_disabled=false) gets corrected. | ||
| logging.info(`Re-dispatching EmailSuppressedEvent for existing suppression (${reason}): ${event.email}`); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In TryGhost/Ghost, what error code does the bookshelf/knex layer surface for a UNIQUE conflict on the suppressions table when running on sqlite3 (vs. mysql)? Is the primary code SQLITE_CONSTRAINTor an extended code likeSQLITE_CONSTRAINT_UNIQUE?
💡 Result:
In TryGhost/Ghost using the bookshelf/knex layer on sqlite3, a UNIQUE constraint conflict on the suppressions table surfaces the SQLite error code SQLITE_CONSTRAINT (numeric value 19), not a more specific extended code like SQLITE_CONSTRAINT_UNIQUE. Knex passes through the raw SQLite error, where the error.code (or errno) is 'SQLITE_CONSTRAINT' with underlying numeric code 19 for any constraint violation, including UNIQUE. The error message typically includes "UNIQUE constraint failed: suppressions.email" (assuming email is the unique field, per Ghost's suppressions table schema with UNIQUE constraints added via migrations). This contrasts with MySQL, which uses ER_DUP_ENTRY (1062) for unique violations.
Citations:
- 1: https://sqlite.org/conflict.html
- 2: https://stackoverflow.com/questions/35415469/sqlite-constraint-primarykey-sqlite3-result-code-1555-unique-constraint-failed
- 3: https://stackoverflow.com/questions/52684205/where-can-i-find-documentation-for-the-types-of-knex-errors/52753070
- 4: https://stackoverflow.com/questions/77034457/inserting-a-list-of-entries-into-sqlite-using-knex-on-expressjs-with-unique-fiel
- 5: How to catch constraint errors? knex/knex#272
- 6: https://stackoverflow.com/questions/75667713/in-sqlite3-how-do-i-troubleshoot-a-error-unique-constraint-failed-from-an-i
🏁 Script executed:
find . -type f -name "mailgun-email-suppression-list.js" | head -5Repository: TryGhost/Ghost
Length of output: 149
🏁 Script executed:
git ls-files | grep -i "mailgun.*suppression"Repository: TryGhost/Ghost
Length of output: 223
🏁 Script executed:
cat -n ghost/core/core/server/services/email-suppression-list/mailgun-email-suppression-list.js | sed -n '125,155p'Repository: TryGhost/Ghost
Length of output: 1503
🏁 Script executed:
cat -n ghost/core/core/server/services/email-suppression-list/mailgun-email-suppression-list.js | sed -n '1,50p'Repository: TryGhost/Ghost
Length of output: 2005
🏁 Script executed:
# Check for Suppression model or schema definition
find ghost/core -type f \( -name "*suppression*" -o -name "*migration*" \) | grep -E "\.(js|ts)$" | head -10Repository: TryGhost/Ghost
Length of output: 867
🏁 Script executed:
cat -n ghost/core/core/server/models/suppression.jsRepository: TryGhost/Ghost
Length of output: 337
🏁 Script executed:
cat -n ghost/core/core/server/data/migrations/versions/5.27/2023-01-04-04-13-add-suppressions-table.jsRepository: TryGhost/Ghost
Length of output: 564
🏁 Script executed:
cat -n ghost/core/test/integration/services/mailgun-email-suppression-list.test.js | head -100Repository: TryGhost/Ghost
Length of output: 5254
🏁 Script executed:
cat -n ghost/core/test/integration/services/mailgun-email-suppression-list.test.js | grep -A 30 "duplicate\|exists\|ER_DUP_ENTRY\|SQLITE_CONSTRAINT"Repository: TryGhost/Ghost
Length of output: 1920
🏁 Script executed:
# Check if there are any other places in the codebase that handle SQLITE_CONSTRAINT or ER_DUP_ENTRY
rg "ER_DUP_ENTRY|SQLITE_CONSTRAINT" --type jsRepository: TryGhost/Ghost
Length of output: 5630
SQLITE_CONSTRAINT is broader than uniqueness — tighten to also match message for uniqueness failures.
In node-sqlite3, err.code === 'SQLITE_CONSTRAINT' is the generic code for every constraint violation (UNIQUE, NOT NULL, CHECK, FOREIGN KEY); specificity lives only in err.message. MySQL's ER_DUP_ENTRY is duplicate-specific, creating an asymmetry. A NOT NULL or FOREIGN KEY violation on suppressions will match the bare SQLITE_CONSTRAINT check and silently dispatch EmailSuppressedEvent even though no row was inserted, emitting spurious events and masking real errors.
Also require the message to indicate a UNIQUE failure (precedent for this pattern exists in webhooks-service.js):
🛡️ Suggested fix
} catch (err) {
- if (err.code !== 'ER_DUP_ENTRY' && err.code !== 'SQLITE_CONSTRAINT') {
+ const isMysqlDuplicate = err.code === 'ER_DUP_ENTRY';
+ const isSqliteUniqueViolation = err.code === 'SQLITE_CONSTRAINT'
+ && /unique constraint failed/i.test(err.message || '');
+ if (!isMysqlDuplicate && !isSqliteUniqueViolation) {
logging.error(err);
return;
}📝 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.
| } catch (err) { | |
| if (err.code !== 'ER_DUP_ENTRY') { | |
| if (err.code !== 'ER_DUP_ENTRY' && err.code !== 'SQLITE_CONSTRAINT') { | |
| logging.error(err); | |
| return; | |
| } | |
| // Suppression already exists — still dispatch so any drifted | |
| // member state (e.g. email_disabled=false) gets corrected. | |
| logging.info(`Re-dispatching EmailSuppressedEvent for existing suppression (${reason}): ${event.email}`); | |
| } | |
| } catch (err) { | |
| const isMysqlDuplicate = err.code === 'ER_DUP_ENTRY'; | |
| const isSqliteUniqueViolation = err.code === 'SQLITE_CONSTRAINT' | |
| && /unique constraint failed/i.test(err.message || ''); | |
| if (!isMysqlDuplicate && !isSqliteUniqueViolation) { | |
| logging.error(err); | |
| return; | |
| } | |
| // Suppression already exists — still dispatch so any drifted | |
| // member state (e.g. email_disabled=false) gets corrected. | |
| logging.info(`Re-dispatching EmailSuppressedEvent for existing suppression (${reason}): ${event.email}`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ghost/core/core/server/services/email-suppression-list/mailgun-email-suppression-list.js`
around lines 135 - 143, Tighten the SQLite constraint check in the catch block
that currently tests err.code === 'SQLITE_CONSTRAINT' so it only treats
true-duplicate unique-constraint failures as equivalent to MySQL's ER_DUP_ENTRY;
update the logic around the existing error handling (the catch handling that
currently calls logging.error or logs the "Re-dispatching EmailSuppressedEvent"
via logging.info) to accept either err.code === 'ER_DUP_ENTRY' OR (err.code ===
'SQLITE_CONSTRAINT' AND the err.message indicates a UNIQUE/duplicate constraint
failure), otherwise log the error and return; mirror the existing
uniqueness-message pattern used in webhooks-service.js and keep
EmailSuppressedEvent re-dispatch behavior only for confirmed unique-violation
cases.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 077fa458f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }, event.timestamp)); | ||
| } catch (err) { | ||
| if (err.code !== 'ER_DUP_ENTRY') { | ||
| if (err.code !== 'ER_DUP_ENTRY' && err.code !== 'SQLITE_CONSTRAINT') { |
There was a problem hiding this comment.
The new catch branch treats any SQLITE_CONSTRAINT as a duplicate and still dispatches EmailSuppressedEvent. In SQLite environments this code covers non-unique failures too (for example foreign-key/not-null constraint errors), so we can mark a member email_disabled=true even when the suppression row was never written. Please gate the re-dispatch to true duplicate-email conflicts only (e.g. by checking the error message/details for UNIQUE on suppressions.email) and keep other constraint failures on the error path.
Useful? React with 👍 / 👎.
| if (data.email) { | ||
| const isSuppressed = (await this.emailSuppressionList.getSuppressionData(data.email))?.suppressed; | ||
| data.email_disabled = !!isSuppressed; | ||
| } |
There was a problem hiding this comment.
We might need to check if email_disabled is already set on data so that it is possible to create new members with email_disabled = false even if there are suppression rules
The test used email_recipients[5] which has failed_at set, but the fixture was loaded with 'members:emails' which filters out recipients with non-null failed_at. This meant the recipient didn't exist in the DB, so the event processor couldn't find it and never dispatched the EmailBouncedEvent. Changed to 'members:emails:failed' to include all recipients.
jonatansberg
force-pushed
the
fix/onc-1640-suppression-check-on-member-add
branch
from
077fa45 to
760945b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/test/integration/services/mailgun-email-suppression-list.test.js (1)
189-214: Optional: wrap the test body in try/finally so the seeded Suppression always cleans up.If any assertion fails between
models.Suppression.add(...)(Line 189) andmodels.Suppression.destroy(...)(Line 213), the seeded suppression row is left behind. In a non-fresh DB (re-runs, watch mode, local dev) this could mask the very regression this test guards against by making the “pre-existing record” state implicit on later runs.♻️ Suggested cleanup hardening
- await models.Suppression.add({ - email: recipient, - email_id: emailBatch.email_id, - reason: 'bounce', - created_at: new Date(1999, 0, 1) - }); - - const memberBefore = await models.Member.findOne({id: memberId}, {require: true}); - assert.equal(memberBefore.get('email_disabled'), false, 'Test setup: the member should start with email_disabled=false'); - - events = [createPermanentFailedEvent({ - errorCode: 605, - providerId, - timestamp, - recipient - })]; - - await emailAnalytics.fetchLatestOpenedEvents(); - await DomainEvents.allSettled(); - - const memberAfter = await models.Member.findOne({id: memberId}, {require: true}); - assert.equal(memberAfter.get('email_disabled'), true, 'Member should be disabled even when the Suppression record already existed'); - - // Clean up so subsequent tests using this email aren't affected - await models.Suppression.destroy({destroyBy: {email: recipient}}); + await models.Suppression.add({ + email: recipient, + email_id: emailBatch.email_id, + reason: 'bounce', + created_at: new Date(1999, 0, 1) + }); + + try { + const memberBefore = await models.Member.findOne({id: memberId}, {require: true}); + assert.equal(memberBefore.get('email_disabled'), false, 'Test setup: the member should start with email_disabled=false'); + + events = [createPermanentFailedEvent({ + errorCode: 605, + providerId, + timestamp, + recipient + })]; + + await emailAnalytics.fetchLatestOpenedEvents(); + await DomainEvents.allSettled(); + + const memberAfter = await models.Member.findOne({id: memberId}, {require: true}); + assert.equal(memberAfter.get('email_disabled'), true, 'Member should be disabled even when the Suppression record already existed'); + } finally { + // Clean up so subsequent tests/runs using this email aren't affected + await models.Suppression.destroy({destroyBy: {email: recipient}}); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/integration/services/mailgun-email-suppression-list.test.js` around lines 189 - 214, Wrap the portion of the test that runs after seeding the suppression in a try/finally so the seeded row is always removed: after calling models.Suppression.add(...) start a try block that contains the test logic (the events setup, calls to emailAnalytics.fetchLatestOpenedEvents(), DomainEvents.allSettled(), and the assertions that check memberBefore/memberAfter), and move the cleanup await models.Suppression.destroy({destroyBy: {email: recipient}}) into a finally block so it always runs (ensure the destroy call is awaited in finally and rethrow any caught error if you capture it).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/test/integration/services/mailgun-email-suppression-list.test.js`:
- Around line 189-214: Wrap the portion of the test that runs after seeding the
suppression in a try/finally so the seeded row is always removed: after calling
models.Suppression.add(...) start a try block that contains the test logic (the
events setup, calls to emailAnalytics.fetchLatestOpenedEvents(),
DomainEvents.allSettled(), and the assertions that check
memberBefore/memberAfter), and move the cleanup await
models.Suppression.destroy({destroyBy: {email: recipient}}) into a finally block
so it always runs (ensure the destroy call is awaited in finally and rethrow any
caught error if you capture it).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4acab991-28bc-41af-b245-f4d25d53656d
📒 Files selected for processing (3)
ghost/core/core/server/services/members/members-api/services/member-bread-service.jsghost/core/test/integration/services/mailgun-email-suppression-list.test.jsghost/core/test/unit/server/services/members/members-api/services/members-bread-service.test.js
1 participant
Summary
When a suppressed member was deleted and someone later signed up with the same email, the new member was created with
email_disabled=falseeven though the suppression record still existed. The send filter then included them in every newsletter.Mailgun rejected each send with a 607, triggering the suppression handler. But the handler silently swallowed the
ER_DUP_ENTRYerror from trying to re-insert the existing suppression record, never reaching the dispatch that would have setemail_disabled=true. The cycle repeated indefinitely.On Tangle, 224 members were in this state.
Changes
member-bread-service.add()now checks the suppression list before creating a member and setsemail_disabledaccordingly, mirroring whatedit()already doesmailgun-email-suppression-listhandleEventrestructured soEmailSuppressedEventdispatches even whenSuppression.add()throwsER_DUP_ENTRY— this self-heals any members who drifted into the broken state, correcting them on their next bounce/complaint. Non-ER_DUP_ENTRYerrors still skip the dispatch.Why the self-heal matters
No backfill migration needed — the 224 affected members on Tangle (and any equivalents on other Pro sites) will self-correct on the next newsletter send: Mailgun generates the 607, the handler now dispatches
EmailSuppressedEventdespite the duplicate suppression record, andemail_disabledgets flipped totrue.Test plan
add()suppression check (both suppressed and clean cases)edit()suppression check (including the case where email isn't being changed)email_disabled=false, verifies the member is disabled via the model (not the API, which would mask the bug)ref ONC-1640