fix: guard FTS triggers so only real content changes re-tokenize - #2314
Draft
edrpls wants to merge 4 commits into
Draft
fix: guard FTS triggers so only real content changes re-tokenize#2314edrpls wants to merge 4 commits into
edrpls wants to merge 4 commits into
Conversation
FTS5 tables were external-content (content='ec_<slug>'), which forces the index to mirror raw column values — and Portable Text fields store JSON, so structural tokens polluted the index (27-29% of it on an audited production database). Searching "normal" (a PT style value) matched 870/906 posts, "_type" matched every document, and snippets showed JSON fragments. Rebuild the FTS tables as self-contained FTS5 whose Portable Text columns hold extracted prose: every JSON string under a text, alt, caption, or code key (span text, image alt/caption, code blocks — the same semantics as extractPlainText). Extraction lives in SQL (json_tree) because the sync triggers cannot call into JS, with json_valid guarding legacy bare-string rows. Self-contained tables also retire the external-content 'delete' choreography and its corruption modes (migration 039's subject): removal is a plain DELETE, a harmless no-op for never-indexed rows, and INSERT OR REPLACE makes concurrent D1 populates converge. Migration 055 rebuilds every search-enabled collection's index and triggers on upgrade; the trigger SQL is lock-step with FTSManager per 039's precedent. The search query layer is unchanged — it joins ec_* by id for metadata, and snippet() now reads the stored prose. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
json_tree exposes output columns named key/value/type/path and friends; a bare column reference inside the extraction subquery binds to those instead of the outer ec_* column, so populating a Portable Text field slugged with one of these names silently indexed NULL. Triggers were unaffected (NEW.-qualified). Qualify the populate and migration references with the content table name. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The FTS update trigger fired on ANY row UPDATE, deleting and re-inserting the document's full index entry even when no searchable column changed. Metadata-only saves — status flips, scheduling, autosave version bumps — and the publish path's rewrite-identical-values UPDATEs each paid full re-tokenization: measured 49x CPU on metadata-only saves and 78-89% of a save's WAL bytes on an audited production deployment. Add a WHEN guard comparing raw column values with null-safe IS NOT: the trigger fires only when an indexed value, the row's locale, or its trash state actually changed. deleted_at stays in the guard so trash/restore keep syncing the index. Raw-column comparison remains valid change detection for Portable Text fields whose indexed values are extracted text. Migration 056 recreates the triggers on existing deployments — trigger swap only, index contents untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
D1 has no migration lock, so another isolate can serve a content UPDATE while 056 has the sync triggers dropped. A lost UPDATE leaves FTS row counts equal to the content row count, so verifyAndRepairIndex's parity check can never detect or heal it — unlike a lost INSERT or DELETE. Follow the trigger swap with the same INSERT OR REPLACE repopulate that 039, 055, and FTSManager.rebuildIndex use for exactly this window. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: aa3f860 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 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 |
Contributor
Scope checkThis PR changes 1,267 lines across 12 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
@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-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@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: |
Contributor
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
18 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes FTS sync triggers re-tokenizing the whole document on every row UPDATE, even when nothing searchable changed.
The update trigger had no change detection: any UPDATE deleted and re-inserted the document's full index entry. Metadata-only saves — status flips, scheduling, autosave version bumps — and the publish path's rewrite-identical-values UPDATEs each paid full re-tokenization. Measured on the audited production deployment (Macabro festival site, emdash 0.31.1): 49× CPU on metadata-only saves, and re-tokenization was 78–89% of a save's WAL bytes — the dominant replication-volume driver under litestream-style WAL shipping.
The fix adds a
WHENguard to the generated update trigger comparing raw column values with null-safeIS NOT: the trigger fires only when an indexed field, the row'slocale, or its trash state (deleted_at) actually changed.deleted_atmust stay in the guard or trash/restore stop syncing the index (locked by test).UPDATE OF <cols>) is required because the publish path SETs every data column even when values are unchanged — only comparing values suppresses those re-tokenizations (locked by test).056recreates the triggers on existing deployments — trigger swap only, index contents untouched,IF NOT EXISTSforms for D1's lockless concurrent migrators, lock-step copy per 039's precedent.Deliberately out of scope (possible follow-up, would need a Discussion per the performance-PR policy): coalescing the publish path's three separate UPDATEs into one. With the guards in place those UPDATEs no longer re-tokenize unless values actually changed, which removes the write amplification this bug is about.
The failing test observes the FTS
_datashadow segments byte-for-byte across a metadata-only UPDATE — on the base branch they get rewritten; with the guard they are identical. Companion tests lock the positive paths: searchable-field edits still re-index, publish-shaped identical-value rewrites don't, and trash/restore still add/remove the row.Found during a measured database audit of a production deployment.
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain. — n/a: no admin UI strings changedAI-generated code disclosure
Screenshots / test output
Failing first (on the base branch, before the guard):
After the fix — write-amplification suite, 056 upgrade-path suite (including a with-teeth baseline proving the unguarded trigger rewrites segments), and the full search/migration suites:
🤖 Generated with Claude Code