Fixed member deletion errors from concurrent API requests

[kevinansfield]

Summary

• When multiple DELETE /members/{id} requests target the same member concurrently, a race condition causes Bookshelf to throw "No Rows Deleted" errors that bubble up as 500s to Sentry
• The findOne check passes for both requests (member exists), but by the time the second request's destroy() runs, the member is already gone
• Fixed by passing require: false to Bookshelf's destroy() so it treats a missing row as a no-op — the member is already deleted, which is the desired outcome

refs https://linear.app/ghost/issue/ONC-1516

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 848b7354-4c5a-4f73-b99e-423f2115bcc0

📥 Commits

Reviewing files that changed from the base of the PR and between 4ed40d9 and c009de6.

📒 Files selected for processing (2)

• ghost/core/core/server/services/members/members-api/repositories/member-repository.js
• ghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js

🚧 Files skipped from review as they are similar to previous changes (2)

• ghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js
• ghost/core/core/server/services/members/members-api/repositories/member-repository.js

---

Walkthrough

The MemberRepository.destroy implementation was changed to merge require: false into the options passed to the underlying destroy call, with an explanatory comment added, so concurrent deletions do not raise a "No Rows Deleted" error. The public method signature remains async destroy(data, options). Unit tests were added to verify that destroy is called with require: false when a member exists and that no error is thrown or destroy call made when the member is absent.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title accurately summarizes the main change: fixing member deletion errors caused by concurrent API requests, which directly matches the changeset that adds require: false to prevent race condition errors.
Description check	✅ Passed	The PR description is directly related to the changeset, providing clear context about the concurrent deletion race condition and explaining the fix applied to prevent 'No Rows Deleted' errors.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch worktree-ONC-1516-member-deletion-errors

---

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 and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

ghost/core/core/server/services/members/members-api/repositories/member-repository.js (1)

815-816: Message-based error matching has limited alternatives in Bookshelf 1.2.0.

The "No Rows Deleted" check at line 815 is brittle across ORM/version changes, as suggested. However, Bookshelf 1.2.0 does not expose a stable error code or custom error class for this specific case (unlike database constraint errors which use err.code). This pattern is also used consistently elsewhere in the codebase (e.g., donation-checkout-session.test.js) and is well-tested. Consider upgrading to a newer Bookshelf version or adding a comment documenting this limitation if an alternative discriminator becomes available.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@ghost/core/core/server/services/members/members-api/repositories/member-repository.js`
around lines 815 - 816, The string-equality check if (err.message === 'No Rows
Deleted') is brittle because Bookshelf 1.2.0 exposes no stable error code/class
for this case; update the member-repository.js where this check appears to add a
clear comment above the check stating that Bookshelf 1.2.0 only exposes the
message (hence the message-based match), referencing the fact this pattern is
used elsewhere (e.g., donation-checkout-session.test.js), and note that
upgrading Bookshelf or replacing this check with a proper discriminator should
be done when a newer Bookshelf version/error API is available; leave the
existing conditional otherwise to preserve current behavior and tests.

🤖 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/core/server/services/members/members-api/repositories/member-repository.js`:
- Around line 815-816: The string-equality check if (err.message === 'No Rows
Deleted') is brittle because Bookshelf 1.2.0 exposes no stable error code/class
for this case; update the member-repository.js where this check appears to add a
clear comment above the check stating that Bookshelf 1.2.0 only exposes the
message (hence the message-based match), referencing the fact this pattern is
used elsewhere (e.g., donation-checkout-session.test.js), and note that
upgrading Bookshelf or replacing this check with a proper discriminator should
be done when a newer Bookshelf version/error API is available; leave the
existing conditional otherwise to preserve current behavior and tests.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 747ceac and 4ed40d9.

📒 Files selected for processing (2)

• ghost/core/core/server/services/members/members-api/repositories/member-repository.js
• ghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js

[ErisDS]

🤖 Velo CI Failure Analysis

Classification: 🟠 SOFT FAIL

• Workflow: CI
• Failed Step: Run Playwright tests locally
• Run: View failed run
  What failed: Playwright browser tests failed with exit code 1
  Why: The final error in the logs indicates that the Playwright browser tests failed with exit code 1, which is a soft failure caused by a code issue, likely a test assertion failure or a problem with the application code under test.
  Action:
  The author should investigate the Playwright test failures and fix any issues in the application code or test suite to resolve the CI failure.

[allouis]

This can be simplified a little, but it's not blocking

[allouis]

You can pass {...options, require: false} here and avoid the whole try/catch and error inspection