Fix #2363: [Bug] skills upsert (INSERT OR REPLACE) skips FTS cleanup trigger under recursiv - #2368
Memtensor-AI wants to merge 2 commits into
Conversation
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 MemTensor#2363
Memtensor-AI
added
ai:generated
🤖 Open Code ReviewTarget: PR #2368 ✅ OpenCodeReview: Review complete: 0 finding(s) across 1 selected item(s). Generated by cloud-assistant via Open Code Review. |
🔧 Open Code Review requested Agent fixOpen Code Review found 1 issue(s). I have resumed the development Agent to fix them.
The Agent will push a new commit to this PR branch. OCR will recheck after the commit is pushed. |
`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 MemTensor#2363). Addresses OCR review of PR MemTensor#2368.
✅ Automated Test Results: PASSEDAll tests passed (75/75 executed, 5 skipped). memos_github_open_source/smoke: 1/1, memos_local_plugin/unit: 3/3, memos_python_core/changed-repo-python: 71 passed, 5 skipped. Duration: 14s [advisory, non-gating] AI-generated tests on branch test/auto-gen-261a199963ed3881-20260915052250: 303/305 passed, 2 failed — these do NOT affect the PR verdict; review the branch manually. Branch: |
2 participants
Description
Fixes #2363. Root cause is exactly as reported:
apps/memos-local-plugin/core/storage/repos/skills.tsupserts viaINSERT OR REPLACE, and per SQLite's documented REPLACE semantics the internal DELETE only firesAFTER DELETEtriggers whenPRAGMA recursive_triggersis ON. SQLite's default is OFF, and several hosts (SQLCipher and some ORM stacks) leave it OFF, so theskills_fts_adcleanup trigger is skipped on every upsert-on-conflict whileskills_fts_aistill runs — leavingskills_ftswith duplicate/orphan rows relative toskillsand pollutingsearchByTextrankings. The same footgun applies uniformly to everyonConflict: "replace"upsert paired with an FTS mirror (traces/policies/world_model).Chose option 2 from the issue: set
PRAGMA recursive_triggers = ONat connection open inapps/memos-local-plugin/core/storage/connection.ts(both read-write and read-only branches, for symmetry). One line, zero API surface change, no migration, covers all four FTS-backed tables at once, and future-proofs any additionalAFTER DELETEtrigger we may add. Rejected option 1 (rewriting each upsert as explicit DELETE+INSERT) because it duplicates the fix across four repos, changes the atomicity contract callers rely on, and doesn't help the next FTS-mirrored table someone adds. Foreign-key cascades are unaffected — those are FK actions, not triggers, and always run.Regression coverage lives in
apps/memos-local-plugin/tests/unit/storage/skills-fts-upsert.test.ts(3 cases: base row-count invariantcount(skills_fts) == count(skills)after upsert-on-conflict, stale-token search behavior through the fullsearchByTextpath, and a directPRAGMA recursive_triggersassertion so a future contributor silently dropping the pragma is caught immediately). All three cases fail on the pre-fix code and pass on the fix.Verified: plugin unit suite 180/180 files, 1553/1553 tests pass (1 skipped, pre-existing); integration suite 3/3 files, 5/5 tests pass;
npm run lint(tsc --noEmit) is clean. opsp artifacts (proposal / spec / design / verification-report / task) are inopenspec/changes/2026-09-14-2363-.../and synced+pushed tospecs/2026-09-14-2363-.../. Working branch pushed to origin at commit20c13665.Related Issue (Required): Fixes #2363
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Not run; documentation-only change.
Checklist
@wustzdy please review this PR.
Reviewer Checklist