fix(core): remove taxonomy assignments when content is permanently deleted - #3091
Conversation
Term assignments are keyed by the entry's translation group. Permanently deleting an entry removes its row, SEO data, comments and revisions, but not the assignments, so once the group's last row is gone nothing owns them. Against the current handler, permanently deleting a post that has no translations leaves its assignment behind, and so does deleting both translations of a translated post. A third case pins that a translation in the trash keeps the assignments, since restoring it brings its terms back.
…leted Term assignments belong to the translation group rather than to a single locale row, so the permanent-delete handler now removes them only when no row of the group is left. A translation in the trash counts as left: it can be restored, and its terms with it. The removal takes the group directly instead of an entry id, because the entry-id variant resolves the group through a row that no longer exists at that point.
🦋 Changeset detectedLatest commit: f435c31 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-test
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-moderation
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approach judgment: This is the right change. Permanent delete should remove translation-group–owned taxonomy assignments once the last content row of the group is gone, and keeping them while a trashed row remains matches the restore workflow. The implementation follows existing repository conventions, adds a focused regression test for the three cases (single translation, surviving translation, and trashed translation), and uses safe SQL (sql.ref for the identifier, parameterized value).
What I checked: diff, changed handler and repository files, existing permanent-delete.test.ts and content-taxonomies.test.ts, the transaction utility, AGENTS.md conventions, and the changeset.
Headline conclusion: The code is correct in the success path and the tests cover it. The main concern is that the cleanup is split across multiple statements and is not atomic on D1 (the production runtime), and the read-then-write pattern is also vulnerable to a TOCTOU race under the default READ COMMITTED isolation used by SQLite and PostgreSQL. I would address that before merging, or at least document the limitation, because the bug fix can still leave orphan assignments in concurrent or failure scenarios. I also note that no migration or backfill is provided for assignments already orphaned before this fix.
Changeset: purge-entry-term-assignments.md is a clear, user-facing patch entry that states the fixed behavior and when assignments are preserved. It meets the changeset README standard.
| // Term assignments are keyed by translation_group, so they belong to the | ||
| // group rather than to this row. They go only once no row of the group is | ||
| // left, trashed ones included, since a trashed row can still be restored. | ||
| if (item?.translationGroup) { | ||
| const groupSurvives = await trxRepo.hasTranslationsIncludingTrashed( | ||
| collection, | ||
| item.translationGroup, | ||
| ); | ||
| if (!groupSurvives) { | ||
| await new TaxonomyRepository(trx).clearEntryGroupTerms( |
There was a problem hiding this comment.
[suggestion] The cleanup block uses a read-then-write pattern: it deletes the content row, probes whether any group rows remain, and then deletes taxonomy assignments. This is wrapped in withTransaction, but packages/core/src/database/transaction.ts documents that D1 does not support transactions—the wrapper falls back to running the callback directly against the database, so multi-statement atomicity is lost on the production runtime. If the handler fails or the worker is evicted after the content row is deleted but before the assignment cleanup runs, the assignments stay orphaned, which partly undermines the fix.
Under the default READ COMMITTED isolation used by SQLite and PostgreSQL, the same pattern is also vulnerable to a TOCTOU race: two concurrent permanent deletes of the last two rows in a group can each see the other's row during the probe and both skip cleanup.
Consider making the assignment cleanup a single self-checking statement so the delete itself is atomic, e.g. by adding to TaxonomyRepository:
async clearEntryGroupTermsIfNoRowsRemain(
collection: string,
entryGroup: string,
): Promise<number> {
validateIdentifier(collection, "collection type");
const tableName = `ec_${collection}`;
const result = await this.db
.deleteFrom("content_taxonomies")
.where("collection", "=", collection)
.where("entry_id", "=", entryGroup)
.where(({ not, exists }) =>
not(
exists(
this.db
.selectFrom(tableName as keyof Database)
.select("id")
.where("translation_group", "=", entryGroup),
),
),
)
.executeTakeFirst();
const removed = Number(result.numDeletedRows ?? 0);
if (removed > 0) invalidateTaxonomyObjectCache();
return removed;
}Then replace the probe-and-delete block with a single call to that method after permanentDelete. This does not close the crash-between-statements gap on D1 (only a future deferred cleanup can), but it removes the read-then-write race and makes the cleanup statement atomic. The same limitation also applies to the existing SEO/comment/revision cleanup in this handler, so fixing it comprehensively may belong in a follow-up.
There was a problem hiding this comment.
I'm keeping the probe. I ran both cleanups as plain SQL against PostgreSQL with two connections: each deleted one of a group's last two translations, then ran its cleanup before either committed. Neither saw the other's uncommitted delete, so with the probe and DELETE … WHERE NOT EXISTS alike, the assignment stayed. Run in sequence, the second cleanup removes it in either case. SQLite lets one connection write at a time (a second connection's delete got database is locked), and D1 is single-writer, as withTransaction notes, so on both, the check after the second delete finds no row left. The D1 gap between statements is the lost multi-statement atomicity withTransaction calls a known D1 limitation, and the SEO, comment and revision cleanup here share it, so it's a question for the whole handler, not this PR. The description now states both limits, and that assignments orphaned before this change stay.
github-actions
Bot
added
review/awaiting-author
2 participants
What does this PR do?
Removes an entry's taxonomy assignments when its last translation is permanently deleted.
Assignments belong to the translation group rather than to a row, with no foreign key, and permanent delete never removed them, so they stayed in the database after the entry was gone. The handler now reads the row's group before the delete and removes the group's assignments once no row of it is left. A translation in the trash keeps them, since restoring it brings its terms back.
The check is the same row lookup and
hasTranslationsIncludingTrashedhelper that #1928 and #2799 add to this handler for reference edges. On D1 it runs after the delete without a transaction, like the SEO, comment and revision cleanup. On PostgreSQL, two overlapping permanent deletes of a group's last two translations each still see the other's row, so both keep the assignments, and a singleDELETE … WHERE NOT EXISTSkeeps them the same way. SQLite and D1 run one write at a time, so there the check after the second delete finds no row left. Assignments that earlier deletes left behind stay.Part of #1683, the third gap in the audit.
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change) (the fullemdashsuite)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain. (n/a: no admin UI change)AI-generated code disclosure
Screenshots / test output
Not applicable for screenshots. Against the unmodified handler, the two cases in
permanent-delete-terms.test.tsthat delete a group's last row fail, and the trash case passes:With the change, all three pass on SQLite and PostgreSQL, and the full
emdashsuite passes on SQLite: