Skip to content

fix(db): make insert_documentation an upsert (closes #48) - #52

Closed
nelsonmfinda wants to merge 1 commit into
deandevz:mainfrom
nelsonmfinda:fix-seed-upsert
Closed

nelsonmfinda wants to merge 1 commit into
deandevz:mainfrom
nelsonmfinda:fix-seed-upsert

Conversation

@nelsonmfinda

Copy link
Copy Markdown
Contributor

Summary

Fix #48: re-seeding the same corpus name no longer raises sqlite3.IntegrityError. db.insert_documentation now upserts the documentations row via INSERT ... ON CONFLICT(name) DO UPDATE ... RETURNING id, then clears and rewrites that doc's sections (cascading query_cache, FTS5 'delete' protocol on sections_fts). Embedding writes are deferred until after the SQLite
commit, so a rollback never leaves embeddings.npy ahead of the DB.

While in there: _get_connection now sets PRAGMA foreign_keys = ON on every connection. Without this, ON DELETE CASCADE was a silent no-op everywhere except init_db, leaving orphan
rows after deletes. The upsert depends on the cascade; every other delete path benefits too.

Related issue

Closes #48.

Type of change

  • Bug fix (non-breaking change that fixes an issue)

How it was tested

pytest -q
742 passed, 1 skipped

8 tests in TestInsertDocumentationUpsert:

  • The original repro: re-seed the same name without raising
  • Sections fully replaced (old gone, new present)
  • FTS5 index pruned (old tokens not searchable, new tokens are)
  • query_cache cascade-pruned (stale section_id removed)
  • Fresh insert unchanged (no regression)
  • created_at preserved across re-seed; doc_id stable; updated_at advances
  • Rollback safety: a malformed second insert leaves original sections + FTS index intact
  • Fixture isolation regression guard: with a stub embedding model attached, the test writes to tmp_path and never touches the repo's real data/embeddings.npy

I haven't run a live king-scrape <url> --name X --yes twice end-to-end in this PR, but the unit tests cover the pre-fix repro precisely. Happy to do a live run as part of review if you
want.

Checklist

  • My code follows the project style (English-only code, comments, and identifiers)
  • I ran the test suite locally and it passes (pytest)
  • I added or updated tests where it made sense
  • I updated the documentation in docs/ and/or README.md if behavior changed (no public-surface change; CHANGELOG entry added)
  • I read the Contributing guide and the Code of Conduct
  • My commits follow the project commit message style
  • I confirmed there are no secrets or credentials in the diff

Additional notes

A few choices worth flagging:

  • Atomic UPSERT instead of SELECT-then-DELETE-then-INSERT. First draft did the latter; senior review surfaced a TOCTOU race under concurrent king-scrape runs against the same name.
    ON CONFLICT(name) DO UPDATE is one statement, gets the writer lock, and removes the race. Same form preserves created_at and keeps doc_id stable, which is what callers caching the ID would expect.
  • Embeddings deferred to post-commit. _generate_and_save_embedding writes to embeddings.npy and section_mapping.json, which are not part of the SQLite transaction. If a rollback fired with the previous shape, the numpy file would race ahead of the DB. Now we stage (section_id, content) tuples during the transaction and flush after conn.commit(). CHANGELOG describes the boundary explicitly: atomic at the SQLite level, embeddings deferred.
  • PRAGMA foreign_keys = ON in _get_connection. Pre-existing latent bug separate from [Bug] seed_one re-seed fails on UNIQUE constraint #48, surfaces here because the upsert depends on cascade. Worth a one-time kctx doctor-style check on existing dbs to verify they have no orphan rows from before this fix; out of scope for this PR.
  • Embedding leak on re-seed (storage). Old section IDs leave behind unused rows in embeddings.npy. Not a correctness bug because cascade-deleted IDs are never queried, but the file grows on every re-seed. Worth a follow-up issue (prune on upsert, or rebuild via a kctx reindex command); not a blocker for the bug fix.
  • temp_db fixture extended to patch EMBEDDINGS_PATH + SECTION_MAPPING_PATH and reset _embeddings / _section_id_to_idx. Without this, any future test that loads the embedding model would silently write to the real repo's data/. Latent today, fixed now.

@deandevz

Copy link
Copy Markdown
Owner

Verified locally: 114 tests pass, upsert behaves correctly (same doc_id on re seed, created_at preserved, updated_at advances, FK CASCADE actually fires with the per connection PRAGMA foreign_keys = ON, FTS5 'delete' protocol runs before sections delete, embeddings flush after commit). No blockers.

One non blocking thing worth knowing about the embeddings file bloat on repeated re seeds is in #55, alongside consolidated feedback for your other three PRs. Suggested a unified PR over there if you want to avoid the cli.py conflicts between #49 / #51 / #54.

@nelsonmfinda nelsonmfinda mentioned this pull request May 14, 2026
10 tasks
@deandevz

Copy link
Copy Markdown
Owner

Superseded by #56, merged as ae1f941. The upsert + FK
CASCADE + post-commit embeddings flush all landed verbatim
in the unified stack. Thanks for the original work,
closing as superseded.

@deandevz deandevz closed this May 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] seed_one re-seed fails on UNIQUE constraint

2 participants