🐛 Fixed 500 error when deleting posts with threaded comments - #29682
Conversation
ref https://linear.app/ghost/issue/ONC-1925 The `comments.in_reply_to_id` foreign key was created with `ON DELETE SET NULL`, while `comments.parent_id` uses `ON DELETE CASCADE`. When a post is deleted, InnoDB cascades into `comments` row by row and the `SET NULL` action issues an `UPDATE` on reply rows whose parent comment is delete-marked in the same cascade. That `UPDATE` re-validates the row's other foreign keys and fails with `ER_NO_REFERENCED_ROW_2` (`errno 1452`), making it impossible to delete any post that has a reply-to-reply comment Switching the action to `CASCADE` makes the post delete cascade deletes-only, which cannot fail this way. Nothing depends on the old `SET NULL` behaviour: it only fires when a comment row is hard-deleted, and the only hard delete in the product is the post delete cascade itself, where every comment is removed regardless. Individual comment deletion is always a soft delete (the `Comment` model overrides `destroy` to set `status='deleted'`), and the UI / serializers handle removed reply targets via that status, not via a nulled foreign key
WalkthroughPost deletion now clears Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This comment was marked as outdated.
This comment was marked as outdated.
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 2m 31s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 3m 26s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 3m 14s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 2m 47s | View ↗ |
nx run-many -t test:unit -p ghost |
✅ Succeeded | 32s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 22s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | <1s | View ↗ |
nx run-many -t lint -p ghost,ghost-monorepo |
✅ Succeeded | 21s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 3s | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-08-03 09:26:46 UTC
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #29682 +/- ##
==========================================
- Coverage 75.44% 75.39% -0.06%
==========================================
Files 1606 1609 +3
Lines 141107 141663 +556
Branches 17455 17486 +31
==========================================
+ Hits 106465 106806 +341
- Misses 33598 33815 +217
+ Partials 1044 1042 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| member_id: {type: 'string', maxlength: 24, nullable: true, unique: false, references: 'members.id', setNullDelete: true}, | ||
| parent_id: {type: 'string', maxlength: 24, nullable: true, unique: false, references: 'comments.id', cascadeDelete: true}, | ||
| in_reply_to_id: {type: 'string', maxlength: 24, nullable: true, unique: false, references: 'comments.id', setNullDelete: true}, | ||
| in_reply_to_id: {type: 'string', maxlength: 24, nullable: true, unique: false, references: 'comments.id', cascadeDelete: true}, |
There was a problem hiding this comment.
MySQL 8 apparently doesn't support cascading more than 15-levels deep:
Cascading operations may not be nested more than 15 levels deep.
https://dev.mysql.com/doc/mysql-reslimits-excerpt/8.0/en/ansi-diff-foreign-keys.html
That would mean that deletion on a post with a 15-reply chain is going to fail. I've had Claude test that assumption locally and it was able to get a ERROR 3008: Foreign key cascade delete/update exceeds max depth of 15 error on a post with a 15-reply chain
There was a problem hiding this comment.
So given that neither SET NULL or CASCADE are proving sufficient here, I think we might need to manually nullify in_reply_to_id in in the transaction that deletes the post?
Sample code to illustrate:
destroy: function destroy(unfilteredOptions) {
let options = this.filterOptions(unfilteredOptions, 'destroy', {extraAllowedProperties: ['id']});
const destroyPost = async () => {
await ghostBookshelf.knex('comments')
.where('post_id', options.id)
.update('in_reply_to_id', null)
.transacting(options.transacting);
return ghostBookshelf.Model.destroy.call(this, options);
};
if (!options.transacting) {
return ghostBookshelf.transaction((transacting) => {
options.transacting = transacting;
return destroyPost();
});
}
return destroyPost();
},
(then do the same for the bulk destroy)
There was a problem hiding this comment.
Nice one @sagzy
We discussed this on Slack and have gone ahead and updated the implementation using this approach 👍
…tion ref https://linear.app/ghost/issue/ONC-1925 No foreign key delete rule works for `comments.in_reply_to_id`: `SET NULL` fails mid-cascade (`errno 1452`) for any post with a reply-to-reply comment, and `CASCADE` hits MySQL's 15-level cascade depth limit (`errno 3008`) on long back-and-forth reply chains, which form a linked list through `in_reply_to_id`. The previous approach of migrating the foreign key to `CASCADE` is therefore reverted - the fix is instead to clear `in_reply_to_id` for the post's comments inside the delete transaction (single and bulk destroy), so the existing `post_id` cascade can delete the comments regardless of thread shape
| await ghostBookshelf.knex('comments') | ||
| .where('post_id', options.id) | ||
| .update('in_reply_to_id', null) | ||
| .transacting(options.transacting); |
There was a problem hiding this comment.
Are we sure that there is always a transaction here?
Else we may want to do sth like
if (!options.transacting) {
// create transaction
// then call destroyPost()
}
There was a problem hiding this comment.
destroyPost is only called after options.transacting is set the existing if (!options.transacting) block just below (cut off in the diff view) creates the transaction and assigns it before invoking destroyPost. Callers that pass their own transacting (e.g. deleteAllContent) take the direct path
ref https://linear.app/ghost/issue/ONC-1925
The
comments.in_reply_to_idforeign key was created withON DELETE SET NULL, whilecomments.parent_idusesON DELETE CASCADE. When a post is deleted, InnoDB cascades intocommentsrow by row and theSET NULLaction issues anUPDATEon reply rows whose parent comment is delete-marked in the same cascade. ThatUPDATEre-validates the row's other foreign keys and fails withER_NO_REFERENCED_ROW_2(errno 1452), making it impossible to delete any post that has a reply-to-reply commentNo foreign key delete rule can fix this. Switching
in_reply_to_idtoCASCADE(this PR's original approach) removes the failingUPDATE, butin_reply_to_idforms a linked list through back-and-forth conversations - each reply referencing the previous one - and InnoDB hard-limits nested cascades to 15 levels, so deleting a post with a 15+ message reply chain fails witherrno 3008insteadThe fix is therefore applied in the application instead of the schema: clear
in_reply_to_idfor the post's comments inside the delete transaction - inPost.destroy(single delete, also used by delete-all-content) and inPostsService#bulkDestroy(bulk delete from the posts list). With the reply chains cleared, the existingcomments.post_id ON DELETE CASCADEdeletes the comments as designed, regardless of thread shape. The foreign key is left unchanged and no migration is neededBoth admin API delete paths are covered by regression tests using a reply-to-reply thread plus a 20-deep reply chain, verified red/green against MySQL (
1452without the fix; and the chain guards the depth limit ifCASCADEis ever reintroduced)