From 20c136651d7d9ae7dffcb363a544ae16e540b832 Mon Sep 17 00:00:00 2001 From: autodev Date: Tue, 15 Sep 2026 05:17:23 +0800 Subject: [PATCH 1/2] fix(storage): set PRAGMA recursive_triggers=ON so upsert cleans FTS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Under `PRAGMA recursive_triggers=0` (SQLite's default, and what several hosts including SQLCipher ship), `INSERT OR REPLACE INTO skills` executes an internal DELETE that skips AFTER DELETE triggers. The `skills_fts_ad` cleanup trigger therefore never fires on upsert-on-conflict, while `skills_fts_ai` still runs — so `skills_fts` accumulates duplicate/orphan rows relative to `skills`, and `repos.skills.searchByText` ranks over the polluted keyword index. The same footgun applies to every FTS-backed table using `onConflict: "replace"` (traces / policies / world_model). Fix at the connection layer via one pragma so all four channels benefit uniformly and any future AFTER DELETE trigger works as intended without every repo author having to remember. Regression tests exercise the DB layer directly: base row-count invariant, stale-token search behavior, and a direct pragma assertion. Refs #2363 --- .../core/storage/connection.ts | 10 ++ .../unit/storage/skills-fts-upsert.test.ts | 152 ++++++++++++++++++ 2 files changed, 162 insertions(+) create mode 100644 apps/memos-local-plugin/tests/unit/storage/skills-fts-upsert.test.ts diff --git a/apps/memos-local-plugin/core/storage/connection.ts b/apps/memos-local-plugin/core/storage/connection.ts index 704434e39..e07b38da9 100644 --- a/apps/memos-local-plugin/core/storage/connection.ts +++ b/apps/memos-local-plugin/core/storage/connection.ts @@ -50,9 +50,19 @@ export function openDb(opts: OpenDbOptions): StorageDb { raw.pragma(`busy_timeout = ${busyTimeoutMs}`); // Better concurrency: stop readers from blocking writers briefly. raw.pragma("wal_autocheckpoint = 1000"); + // FTS-sync correctness (issue #2363): the row-DELETE that SQLite runs + // internally as part of `INSERT OR REPLACE` (used by skills / traces / + // policies / world_model repos) only fires AFTER DELETE triggers when + // `recursive_triggers` is ON. Without this, every upsert-on-conflict + // leaves an orphan row in the paired `*_fts` table because the AFTER + // DELETE cleanup trigger is skipped while the AFTER INSERT trigger still + // runs. SQLite's default is host-dependent (SQLCipher and several ORMs + // set it to 0), so we make it explicit here. + raw.pragma("recursive_triggers = ON"); } else { raw.pragma(`busy_timeout = ${busyTimeoutMs}`); raw.pragma("foreign_keys = ON"); + raw.pragma("recursive_triggers = ON"); } // We deliberately type the cache as `any` — the upstream Statement type is diff --git a/apps/memos-local-plugin/tests/unit/storage/skills-fts-upsert.test.ts b/apps/memos-local-plugin/tests/unit/storage/skills-fts-upsert.test.ts new file mode 100644 index 000000000..94d38c426 --- /dev/null +++ b/apps/memos-local-plugin/tests/unit/storage/skills-fts-upsert.test.ts @@ -0,0 +1,152 @@ +/** + * Regression guard for issue #2363: + * + * `skills.upsert` compiles to `INSERT OR REPLACE INTO skills`. SQLite implements + * REPLACE by internally deleting the conflicting row and then inserting the new + * one. Per SQLite's docs, the row-delete happens WITHOUT firing DELETE triggers + * unless `PRAGMA recursive_triggers` is ON. Because SQLite defaults that pragma + * to OFF (and better-sqlite3 does not override it), the AFTER DELETE trigger + * `skills_fts_ad` never fires on upsert-on-conflict, leaving the old FTS row + * behind while the AFTER INSERT trigger appends a fresh one. `skills_fts` then + * accumulates orphan rows relative to `skills`, and `searchByText` starts to + * rank/paginate over duplicate/stale hits. + * + * These tests exercise the DB layer directly (no core.skill pipeline needed) so + * the failing case is obvious. + */ + +import { describe, expect, it } from "vitest"; + +import { makeTmpDb, type TmpDbHandle } from "../../helpers/tmp-db.js"; + +function vec(arr: number[]): Float32Array { + return new Float32Array(arr); +} + +function baseSkill(handle: TmpDbHandle, opts: { + id: string; + name: string; + invocationGuide: string; +}) { + handle.repos.skills.upsert({ + id: opts.id as never, + name: opts.name, + status: "active", + invocationGuide: opts.invocationGuide, + procedureJson: null, + eta: 0.5, + support: 1, + gain: 0.2, + trialsAttempted: 0, + trialsPassed: 0, + sourcePolicyIds: [], + sourceWorldModelIds: [], + evidenceAnchors: [], + vec: vec([1, 0, 0]), + createdAt: 0 as never, + updatedAt: 0 as never, + version: 1, + }); +} + +function countFts(handle: TmpDbHandle, table: string, idCol: string, id: string): number { + return ( + handle.db + .prepare<{ id: string }, { n: number }>( + `SELECT COUNT(*) AS n FROM ${table} WHERE ${idCol} = @id`, + ) + .get({ id })?.n ?? 0 + ); +} + +describe("storage/skills — upsert keeps skills_fts consistent (regression #2363)", () => { + it("upserting an existing skill does not leave an orphan skills_fts row", () => { + const handle = makeTmpDb(); + try { + const id = "sk_upsert_2363"; + baseSkill(handle, { + id, + name: "original name", + invocationGuide: "originalguideneedle", + }); + + // Sanity: the AFTER INSERT trigger populated the FTS side. + expect(countFts(handle, "skills_fts", "skill_id", id)).toBe(1); + + // Upsert same id with new indexed content. Under recursive_triggers=OFF, + // the internal REPLACE delete skips the AFTER DELETE trigger, so the old + // FTS row is left behind AND the AFTER INSERT trigger appends a new one. + baseSkill(handle, { + id, + name: "revised name", + invocationGuide: "revisedguideneedle", + }); + + // Base row is still a single row (REPLACE semantics on `skills`). + const skillRows = handle.db + .prepare<{ id: string }, { n: number }>( + `SELECT COUNT(*) AS n FROM skills WHERE id = @id`, + ) + .get({ id })?.n; + expect(skillRows).toBe(1); + + // FTS side must mirror the base row: exactly one entry for this skill. + expect(countFts(handle, "skills_fts", "skill_id", id)).toBe(1); + + // Global invariant: skills_fts row count matches skills row count. + const total = handle.db + .prepare( + `SELECT (SELECT COUNT(*) FROM skills) AS skills, + (SELECT COUNT(*) FROM skills_fts) AS fts`, + ) + .get(); + expect(total?.fts).toBe(total?.skills); + } finally { + handle.cleanup(); + } + }); + + it("stale invocation-guide text no longer matches after upsert", () => { + const handle = makeTmpDb(); + try { + const id = "sk_stale_2363"; + baseSkill(handle, { + id, + name: "docker syslib install fix", + invocationGuide: "obsoletetokenalpha kubernetes pod restart", + }); + + // Pre-upsert: the old token should hit. + const preHits = handle.repos.skills.searchByText('"obsoletetokenalpha"', 10); + expect(preHits.map((h) => h.id)).toContain(id); + + baseSkill(handle, { + id, + name: "docker syslib install fix", + invocationGuide: "freshtokenbeta kubernetes pod restart", + }); + + // Post-upsert: the old token must NOT hit any longer — otherwise the + // repos.searchByText ranker will surface stale content. + const staleHits = handle.repos.skills.searchByText('"obsoletetokenalpha"', 10); + expect(staleHits.map((h) => h.id)).not.toContain(id); + + const freshHits = handle.repos.skills.searchByText('"freshtokenbeta"', 10); + expect(freshHits.map((h) => h.id)).toContain(id); + } finally { + handle.cleanup(); + } + }); + + it("connection sets recursive_triggers ON so REPLACE-driven deletes fire triggers", () => { + const handle = makeTmpDb(); + try { + const rt = (handle.db.raw.pragma("recursive_triggers") as Array<{ + recursive_triggers: number; + }>)[0]?.recursive_triggers; + expect(rt).toBe(1); + } finally { + handle.cleanup(); + } + }); +}); From 5603ee8125690d522d3ff43ad5cb631149b82af8 Mon Sep 17 00:00:00 2001 From: autodev Date: Tue, 15 Sep 2026 05:23:46 +0800 Subject: [PATCH 2/2] fix(storage): drop no-op recursive_triggers pragma from read-only branch `recursive_triggers = ON` only affects DELETE/UPDATE triggers fired by implicit REPLACE deletions, none of which can occur on a read-only handle. Setting it in the read-only branch was dead code and misleading about which pragmas the read path actually depends on. Keep the pragma on the writable branch where it fixes the FTS orphan bug (issue #2363). Addresses OCR review of PR #2368. --- apps/memos-local-plugin/core/storage/connection.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/apps/memos-local-plugin/core/storage/connection.ts b/apps/memos-local-plugin/core/storage/connection.ts index e07b38da9..a633b0b33 100644 --- a/apps/memos-local-plugin/core/storage/connection.ts +++ b/apps/memos-local-plugin/core/storage/connection.ts @@ -62,7 +62,6 @@ export function openDb(opts: OpenDbOptions): StorageDb { } else { raw.pragma(`busy_timeout = ${busyTimeoutMs}`); raw.pragma("foreign_keys = ON"); - raw.pragma("recursive_triggers = ON"); } // We deliberately type the cache as `any` — the upstream Statement type is