Skip to content

Fix FTS5 lifecycle handling#465

Open
Pouf5 wants to merge 11 commits intoemdash-cms:mainfrom
Pouf5:fix-fts5
Open

Fix FTS5 lifecycle handling#465
Pouf5 wants to merge 11 commits intoemdash-cms:mainfrom
Pouf5:fix-fts5

Conversation

@Pouf5
Copy link
Copy Markdown
Contributor

@Pouf5 Pouf5 commented Apr 11, 2026

What does this PR do?

It fixes a bug where creating a searchable collection and a searchable field does not spin up FTS5 tables as expected. Now the lifecycle is wired correctly.

Type of change

  • Bug fix
  • Feature (requires approved Discussion)
  • Refactor (no behavior change)
  • Documentation
  • Performance improvement
  • Tests
  • Chore (dependencies, CI, tooling)

Checklist

  • I have read CONTRIBUTING.md
  • pnpm typecheck passes
  • pnpm --silent lint:json | jq '.diagnostics | length' returns 0
  • pnpm test passes (or targeted tests for my change)
  • pnpm format has been run
  • I have added/updated tests for my changes (if applicable)
  • I have added a changeset (if this PR changes a published package)
  • New features link to an approved Discussion: https://github.com/emdash-cms/emdash/discussions/...

AI-generated code disclosure

  • This PR includes AI-generated code

Screenshots / test output

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 11, 2026

🦋 Changeset detected

Latest commit: 63fa42a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
emdash Patch
@emdash-cms/cloudflare Patch
@emdash-cms/admin Patch
@emdash-cms/auth Patch
@emdash-cms/blocks Patch
@emdash-cms/gutenberg-to-portable-text Patch
@emdash-cms/x402 Patch
create-emdash Patch
@emdash-cms/plugin-embeds Patch

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

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Apr 11, 2026

Open in StackBlitz

@emdash-cms/admin

npm i https://pkg.pr.new/@emdash-cms/admin@465

@emdash-cms/auth

npm i https://pkg.pr.new/@emdash-cms/auth@465

@emdash-cms/blocks

npm i https://pkg.pr.new/@emdash-cms/blocks@465

@emdash-cms/cloudflare

npm i https://pkg.pr.new/@emdash-cms/cloudflare@465

emdash

npm i https://pkg.pr.new/emdash@465

create-emdash

npm i https://pkg.pr.new/create-emdash@465

@emdash-cms/gutenberg-to-portable-text

npm i https://pkg.pr.new/@emdash-cms/gutenberg-to-portable-text@465

@emdash-cms/x402

npm i https://pkg.pr.new/@emdash-cms/x402@465

@emdash-cms/plugin-ai-moderation

npm i https://pkg.pr.new/@emdash-cms/plugin-ai-moderation@465

@emdash-cms/plugin-atproto

npm i https://pkg.pr.new/@emdash-cms/plugin-atproto@465

@emdash-cms/plugin-audit-log

npm i https://pkg.pr.new/@emdash-cms/plugin-audit-log@465

@emdash-cms/plugin-color

npm i https://pkg.pr.new/@emdash-cms/plugin-color@465

@emdash-cms/plugin-embeds

npm i https://pkg.pr.new/@emdash-cms/plugin-embeds@465

@emdash-cms/plugin-forms

npm i https://pkg.pr.new/@emdash-cms/plugin-forms@465

@emdash-cms/plugin-webhook-notifier

npm i https://pkg.pr.new/@emdash-cms/plugin-webhook-notifier@465

commit: 63fa42a

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes the SchemaRegistry ↔ FTS5 lifecycle wiring so that FTS tables/triggers are created/rebuilt/disabled automatically when collections toggle "search" support and when searchable fields are added/removed/changed.

Changes:

  • Add SchemaRegistry.syncSearchState() and invoke it when collection "search" support is toggled and when searchable fields are created/updated/deleted.
  • Reorder searchable-field deletion to remove FTS trigger dependencies before dropping the content-table column.
  • Add unit tests covering FTS table creation, rebuild, and disable scenarios.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
packages/core/src/schema/registry.ts Adds syncSearchState and calls it from collection/field lifecycle operations; reorders searchable-field deletion to avoid trigger/DDL conflicts.
packages/core/tests/unit/schema/registry.test.ts Adds integration-style unit tests validating FTS table lifecycle behavior through SchemaRegistry operations.
.changeset/polite-cameras-dance.md Publishes a patch changeset documenting the FTS lifecycle bug fix.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Pouf5
Copy link
Copy Markdown
Contributor Author

Pouf5 commented Apr 12, 2026

Opus review:

  ---
  BUG: updateCollection FTS sync failure creates unrecoverable state
  File: packages/core/src/schema/registry.ts:236-243
  Category: Data Integrity
  Severity: HIGH

  The collection record is committed to the database before syncSearchState runs. If syncSearchState throws, the caller gets an error, but the supports column is
  already persisted with the new value. On retry, hadSearch === hasSearch evaluates to true (both read from the now-updated DB), so the sync is never re-attempted.

  Trigger:
  1. Collection has supports: ["drafts"], user updates to supports: ["search"]
  2. DB update at line 190 succeeds — collection now has ["search"]
  3. syncSearchState at line 241 fails (disk error, FTS5 extension not loaded, etc.)
  4. Caller gets error, retries the same updateCollection("articles", { supports: ["search"] })
  5. existing is re-read: supports is already ["search"]
  6. hadSearch = true, hasSearch = true → hadSearch !== hasSearch is false → sync skipped
  7. Collection permanently claims search support with no FTS table. No code path repairs this without first toggling search off, then back on.

  Fix: Move the syncSearchState call before the DB update (sync first, persist if sync succeeds), or wrap the update + sync in withTransaction so the DB update rolls
  back on sync failure.

  ---
  BUG: deleteField orphans a column if syncSearchState fails
  File: packages/core/src/schema/registry.ts:504-517
  Category: Data Integrity
  Severity: MEDIUM

  The field record is deleted from _emdash_fields (line 509) before syncSearchState runs (line 513). If syncSearchState throws, the field record is gone but the column
   still exists in the content table and the old FTS triggers still reference it. The system no longer knows the field exists, so no code path will clean up the
  orphaned column.

  Trigger:
  1. Collection has two searchable fields: title, body
  2. User deletes body
  3. Line 509: field record for body deleted from _emdash_fields
  4. Line 513: syncSearchState called — suppose rebuildIndex fails (e.g., createFtsTable DDL error)
  5. _emdash_fields has no body record, but ec_articles still has a body column
  6. FTS triggers still reference the old column set including body
  7. dropColumn at line 517 never executes

  Fix: Wrap lines 509-517 in a try/catch that re-inserts the field record if sync or drop fails, or use withTransaction.

  ---
  BUG: syncSearchState throws on non-SQLite databases via enableSearch
  File: packages/core/src/schema/registry.ts:481-486
  Category: Error Handling
  Severity: MEDIUM

  The old rebuildSearchIndex only called rebuildIndex (which has if (!isSqlite(this.db)) return; — a silent no-op) and disableSearch (also a silent no-op on
  non-SQLite). The new syncSearchState routes through enableSearch in the !ftsActive branch, and enableSearch throws new Error("Full-text search is only available with
   SQLite databases") instead of silently returning.

  Trigger:
  1. Run EmDash on a non-SQLite dialect (or any future dialect)
  2. Create a collection with supports: ["search"]
  3. Add a searchable field
  4. syncSearchState → enableSearch → throws

  The old code would have silently done nothing. The new code crashes the operation.

  Fix: Add if (!isSqlite(this.db)) return; as the first line of syncSearchState, matching the guard pattern used in every other FTS method.

  ---
  BUG: PR checklist has unchecked lint item
  File: PR description
  Category: Process
  Severity: LOW

  The pnpm --silent lint:json | jq '.diagnostics | length' checkbox is unchecked. CONTRIBUTING.md says all CI checks must pass. This is either an oversight in checking
   the box, or lint is actually failing.

  ---
  No other bugs found. The disableSearch weight-preservation fix, the deleteCollection FTS cleanup, and the syncSearchState state machine logic are all correct for the
   happy path.

To my knowledge, D1 doesn't support transactions.

@github-actions github-actions bot added size/XL and removed size/L labels Apr 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants