Repository navigation
Conversation
Drop a leftover search_index_rebuilt before CREATE so an interrupted d2e3f4a5b6c7 upgrade can retry instead of failing with table already exists. Regression covers the empty leftover-table case from basicmachines-co#1853. Signed-off-by: MbappeWU <MbappeWU@users.noreply.github.com>
|
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
MbappeWU
changed the title
|
I have read the CLA Document and I hereby sign the CLA |
|
@CLAassistant check |
|
Thanks, @MbappeWU, for reporting the interrupted migration failure and contributing the recovery fix and regression test! We cherry-picked your commit into #1865, preserving your authorship and sign-off, and added real SQLite integration coverage for upgrade and downgrade retries with empty and populated scratch tables. Closing this PR in favor of #1865, which is approved and passing CI. Thank you for the contribution! |
…1865) ## Why Two reported SQLite failures block reindexing or startup: deleting a large chunk manifest exceeds SQLite's expression-depth limit (#1850), and an interrupted script-ngram migration leaves a scratch table that prevents the next startup (#1853). Both reproduce on current main with the contributor's regression tests. Fixes #1850. Fixes #1853. ## What Changed - Cherry-pick @MbappeWU's fixes from #1857 and #1859, retaining authorship and sign-offs. - Batch pending-vector deletion predicates and row-id deletes in groups of 200. - Drop a leftover `search_index_rebuilt` scratch table before rebuilding the intact source index. - Add real SQLite integration tests covering a 1,201-record delete, protected generations/project partitions, repeated deletion, and upgrade/downgrade retries with empty or populated scratch tables. ## Implementation Details Deletion retains the existing project, key, source-hash and pending-status guards in one transaction. Batching limits expression depth and bound parameters without changing the concurrency protocol. The migration rebuilds from `search_index`; the scratch table has no authoritative state. Integration tests commit the interrupted state before opening a new transaction and verify preserved full-text matches afterward. They use Alembic's real Operations context and sqlite-vec without application mocks. Contributor workflow-evidence notes are omitted from the final diff. ## Testing - Before fixes: the two new contributor regressions fail on main with `Expression tree is too large` and `table search_index_rebuilt already exists`. - `uv run pytest tests/repository/test_sqlite_vector_search_repository.py tests/test_script_ngram_search_migration.py test-int/test_sqlite_index_recovery.py -q -o addopts= --tb=short`: 46 passed. - `just fast-check`: lint, format and full typecheck passed. - `just doctor`: sandboxed write, index and search passed. - `git diff --check`: passed. ## Risks / Follow-ups Changes are SQLite-specific; PostgreSQL production paths are unchanged. The recovery assumes the original FTS table remains intact, matching the reported interrupted upgrade. #1861's watcher lifecycle changes are reviewed separately. --------- Signed-off-by: MbappeWU <MbappeWU@users.noreply.github.com> Signed-off-by: phernandez <paul@basicmachines.co> Co-authored-by: MbappeWU <MbappeWU@users.noreply.github.com>
|
Thanks @phernandez — happy this made it into #1865. Appreciate the cherry-pick and keeping the authorship. |
Fixes #1853.
Problem
Migration
d2e3f4a5b6c7createssearch_index_rebuiltwithout clearing a leftover table. An interrupted upgrade can leave an empty FTS5search_index_rebuiltwhilealembic_versionstays behind, so every later start fails withtable search_index_rebuilt already exists.Change
Drop
search_index_rebuiltwithDROP TABLE IF EXISTSbefore the CREATE inrebuild_sqlite_search_index.Tests
New case seeds a leftover empty
search_index_rebuiltbeside an intactsearch_indexand asserts the upgrade completes withscript_ngramsand preserved rows.