Fixed FOR UPDATE lock leaking to relation fetches during post edit

[cmraible]

Summary

• Overrides _handleEager on Ghost's base Bookshelf model to strip the lock option before it reaches eager-loaded relation queries
• Previously, a single post edit with lock: 'forUpdate' acquired FOR UPDATE locks on 9 tables (posts, tags, users, products, post_revisions, posts_meta, emails, roles, roles_users) via Bookshelf's eager loading propagation
• Now only the primary post row gets the FOR UPDATE lock; relations are loaded without row-level locking

Context

A customer was experiencing database deadlocks at the PUT /ghost/api/admin/posts/:id endpoint. The root cause: crud.js sets options.lock = 'forUpdate' for edit operations, and Bookshelf propagates this lock to all eager-loaded relation queries via _handleEager → EagerRelation.fetch() → Sync constructor.

Example deadlocking query from production:

select distinct `tags`.*, `posts_tags`.`post_id` as `_pivot_post_id`,
  `posts_tags`.`tag_id` as `_pivot_tag_id`,
  `posts_tags`.`sort_order` as `_pivot_sort_order`
from `tags` inner join `posts_tags` on `posts_tags`.`tag_id` = `tags`.`id`
where `posts_tags`.`post_id` in ('69b8927caa3f5600011b6197')
order by `sort_order` ASC for update

The FOR UPDATE on relation fetches is unnecessary:

• Same-post concurrency: Already serialized by the post row's FOR UPDATE lock
• Different-post concurrency: Relation writes target different pivot table rows (different post_id), so no conflict
• The relation rows themselves (tags, users) are never modified during a post edit

Test plan

• Unit tests: crud.js (10 passing), post.js (40 passing)
• E2E tests: posts.test.js (1510 passing)
• E2E tests: posts-legacy + posts-bulk (1510 passing)
• E2E tests: e2e-webhooks/posts.test.js (passing)
• Monitor Innodb_row_lock_waits, Innodb_row_lock_time, and Innodb_deadlocks after deploy

[coderabbitai]

Walkthrough

The edit method in the CRUD operations file was modified to create a separate fetchOptions object from the provided options when transacting is enabled. This fetchOptions object is used for the model.fetch call with the forUpdate lock applied, rather than mutating the original options object. Corresponding test assertions were updated to verify that the fetch method receives a shallow copy of options with an undefined lock property, and that the save method is called with the original filteredOptions object reference.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: fixing a FOR UPDATE lock that was leaking into relation fetches during post edits, which aligns with the primary objective of the changeset.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check	✅ Passed	The pull request description clearly explains the issue (FOR UPDATE lock leaking to relation fetches), the solution (scoping lock to initial fetch), and provides concrete evidence of the problem with SQL examples and test results.

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

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch cmraible/fix-edit-forupdate-leak

📝 Coding Plan

• Generate coding plan for human review comments

---

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/test/unit/server/models/base/crud.test.js (1)

162-170: Strengthen this test to actually verify the options-copy contract.

Right now this can still pass even if fetch receives the same object as filteredOptions. Add explicit reference inequality (and method boundary) checks so regressions are caught.

✅ Suggested assertion upgrade

                 // fetch receives a shallow copy of options (without lock or method)
                 const fetchOptions = fetchStub.args[0][0];
+                assert.notEqual(fetchOptions, filteredOptions);
                 assert.equal(fetchOptions.id, filteredOptions.id);
                 assert.equal(fetchOptions.lock, undefined);
+                assert.equal(fetchOptions.method, undefined);

                 const filteredData = filterDataSpy.returnValues[0];
                 assert.equal(saveStub.args[0][0], filteredData);
                 assert.equal(saveStub.args[0][1].method, 'update');
                 assert.equal(saveStub.args[0][1], filteredOptions);

🤖 Prompt for AI Agents

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

In `@ghost/core/test/unit/server/models/base/crud.test.js` around lines 162 - 170,
The test must assert that fetch received a shallow copy of filteredOptions
rather than the same object: add an assertion that fetchOptions is not the same
reference as filteredOptions (e.g., assert.notStrictEqual(fetchOptions,
filteredOptions)) and verify method/boundary isolation by asserting
fetchOptions.method is undefined while saveStub.args[0][1].method === 'update';
optionally mutate fetchOptions (e.g., set a temp prop) and assert
filteredOptions unchanged to prove they are distinct. Ensure you reference
fetchStub, filteredOptions, fetchOptions, saveStub, and filterDataSpy when
adding these assertions.

🤖 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/unit/server/models/base/crud.test.js`:
- Around line 162-170: The test must assert that fetch received a shallow copy
of filteredOptions rather than the same object: add an assertion that
fetchOptions is not the same reference as filteredOptions (e.g.,
assert.notStrictEqual(fetchOptions, filteredOptions)) and verify method/boundary
isolation by asserting fetchOptions.method is undefined while
saveStub.args[0][1].method === 'update'; optionally mutate fetchOptions (e.g.,
set a temp prop) and assert filteredOptions unchanged to prove they are
distinct. Ensure you reference fetchStub, filteredOptions, fetchOptions,
saveStub, and filterDataSpy when adding these assertions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ad91e6f3-21da-49de-84ab-ef91740a2179

📥 Commits

Reviewing files that changed from the base of the PR and between 7ff7c00 and 04df7e0.

📒 Files selected for processing (2)

• ghost/core/core/server/models/base/plugins/crud.js
• ghost/core/test/unit/server/models/base/crud.test.js