fix(store): dedup documents before creating the unique index - #346
Conversation
`CREATE UNIQUE INDEX idx_docs_project_path ON store_documents(project_id, path)` was added as a bare migration. Uniqueness on that pair had until then been enforced only by the single-writer application path, so a database written before the index existed can already hold duplicate rows -- and the index creation fails outright against those. That failure is not cosmetic. The migration is rolled back, `user_version` never advances, and the index never comes into existence. `write_batch_items` upserts via `ON CONFLICT(project_id, path) DO UPDATE`, which requires that index, so every per-item batch write fails too: the store is unusable, not merely un-migrated. Dedup first, keeping the most recently written row per (project_id, path), then create the index. Databases that already migrated successfully had no duplicates and are unaffected; databases stuck on the failure re-run the migration and recover, precisely because the failed attempt rolled back. The SQL is a named constant rather than inlined so the dedup behaviour can be tested against a minimal schema, without replaying the full production migration sequence to reach it. Also hoist `write_batch_items`' four statements out of its loop onto `prepare_cached`. Per-item indexing means one crate contributes hundreds of items per batch, each previously re-preparing the same four statements.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds a migration that releases references and removes duplicate documents before enforcing ChangesDocument storage
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/agentflare-store/src/migrations.rs`:
- Around line 13-16: Update the migration’s duplicate-document cleanup to
reconcile or explicitly delete dependent store_doc_history rows before removing
discarded store_documents records, preserving the surviving document’s history
where applicable. Extend the migration test to use the actual store_doc_history
foreign-key relationship and verify no orphaned history remains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f7cd744a-0124-4723-af92-e6974023f2d1
📒 Files selected for processing (2)
crates/agentflare-store/src/migrations.rscrates/flare-docs/src/store.rs
Addresses CodeRabbit's review of the dedup migration. Discarding a duplicate document is not a single DELETE, because two things outlive the row: * `store_doc_history.doc_id` references `store_documents(id)`. This connection never enables `PRAGMA foreign_keys` -- `db_kit::open_file` sets busy_timeout and WAL only -- so the delete does not fail as the review supposed. It silently strands history rows naming a document that no longer exists, and `DELETE FROM store_doc_history` appears nowhere in the codebase, so nothing ever collects them. * `store_blobs.ref_count` is a stored counter, not a live scan over referencing rows (see `blob_unref`). A row dropped without decrementing pins its blob at a positive count permanently -- the same leak class #343 fixed for the ordinary delete path. So: release the references, then the history, then the rows. The decrement counts references removed rather than distinct hashes, because several discarded rows can point at one blob and a per-hash decrement would subtract one where it owes several. Blob files whose count reaches zero are left on disk. Reclaiming them means filesystem I/O from a SQL migration; disk-level GC is issue #339's scope. The test schema now carries the real dependent tables rather than a documents-only stub. Foreign keys stay at SQLite's default (off) on purpose: that is how the production database is opened, so enabling them would test a configuration that never runs.
|
Addressed in Verified the finding before implementing it — it was half right, and the other half was worse than described. Not reproducible as stated: "this delete either fails with foreign keys enabled". Confirmed, and understated: the orphaning is real and permanent — Fix: release references, then history, then rows. The decrement counts references removed rather than distinct hashes — several discarded rows can point at one blob, and a Deliberately out of scope: blob files whose count reaches zero stay on disk. Reclaiming them means filesystem I/O from a SQL migration, and disk-level GC is issue #339. The doc comment states this rather than leaving it implied. Tests — schema now carries the real
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/agentflare-store/src/migrations.rs (1)
31-58: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winGood fix for the orphaned-history issue; consider reducing repeated computation and full-table rewrite.
This correctly resolves the previously flagged critical issue — history is now deleted before documents, in the order the doc comment describes, and test coverage confirms zero orphaned
store_doc_historyrows remain.Two related efficiency concerns in the SQL itself:
- The "discarded rows" subquery (
SELECT MAX(rowid) FROM store_documents GROUP BY project_id, pathcombined withrowid NOT IN (...)) is repeated 4 times across the UPDATE and two DELETEs. Since CTEs are scoped to a single statement, this can't be deduplicated with aWITHclause across the batch, but a temp table computed once would remove the duplication and the risk of the four copies drifting apart in future edits.- The
UPDATE store_blobshas noWHEREclause, so it rewrites every row instore_blobseven when the computed delta is 0 (i.e., even on a store with zero duplicates). For a large blob table this is unnecessary write amplification on every startup migration run.♻️ Proposed refactor using a temp table
pub(crate) const DEDUP_AND_UNIQUE_INDEX_MIGRATION: &str = " - UPDATE store_blobs - SET ref_count = MAX(0, ref_count - - (SELECT COUNT(*) FROM store_documents d - WHERE d.blob_hash = store_blobs.hash - AND d.rowid NOT IN ( - SELECT MAX(rowid) FROM store_documents GROUP BY project_id, path)) - - (SELECT COUNT(*) FROM store_doc_history h - JOIN store_documents d ON d.id = h.doc_id - WHERE h.blob_hash = store_blobs.hash - AND d.rowid NOT IN ( - SELECT MAX(rowid) FROM store_documents GROUP BY project_id, path))); - - DELETE FROM store_doc_history - WHERE doc_id IN ( - SELECT id FROM store_documents - WHERE rowid NOT IN ( - SELECT MAX(rowid) FROM store_documents GROUP BY project_id, path - ) - ); - - DELETE FROM store_documents - WHERE rowid NOT IN ( - SELECT MAX(rowid) FROM store_documents GROUP BY project_id, path - ); + CREATE TEMP TABLE _dedup_discarded AS + SELECT rowid, id, blob_hash FROM store_documents + WHERE rowid NOT IN (SELECT MAX(rowid) FROM store_documents GROUP BY project_id, path); + + UPDATE store_blobs + SET ref_count = MAX(0, ref_count + - (SELECT COUNT(*) FROM _dedup_discarded d WHERE d.blob_hash = store_blobs.hash) + - (SELECT COUNT(*) FROM store_doc_history h + JOIN _dedup_discarded d ON d.id = h.doc_id + WHERE h.blob_hash = store_blobs.hash)) + WHERE hash IN ( + SELECT blob_hash FROM _dedup_discarded WHERE blob_hash IS NOT NULL + UNION + SELECT h.blob_hash FROM store_doc_history h + JOIN _dedup_discarded d ON d.id = h.doc_id WHERE h.blob_hash IS NOT NULL + ); + + DELETE FROM store_doc_history WHERE doc_id IN (SELECT id FROM _dedup_discarded); + + DELETE FROM store_documents WHERE rowid IN (SELECT rowid FROM _dedup_discarded); + + DROP TABLE _dedup_discarded; CREATE UNIQUE INDEX IF NOT EXISTS idx_docs_project_path ON store_documents(project_id, path); ";Separately, since the correlated subqueries filter on
blob_hash/doc_id, please confirm supporting indexes exist onstore_documents.blob_hashandstore_doc_history.doc_idin the production schema — without them this migration does a full scan per blob row on large tables.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/agentflare-store/src/migrations.rs` around lines 31 - 58, Refactor DEDUP_AND_UNIQUE_INDEX_MIGRATION to compute discarded document row IDs once in a temporary table, then reuse it in the blob UPDATE and both document/history DELETE statements. Restrict the store_blobs UPDATE to rows whose reference count actually changes, and verify the production schema defines supporting indexes for store_documents.blob_hash and store_doc_history.doc_id.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/agentflare-store/src/migrations.rs`:
- Around line 31-58: Refactor DEDUP_AND_UNIQUE_INDEX_MIGRATION to compute
discarded document row IDs once in a temporary table, then reuse it in the blob
UPDATE and both document/history DELETE statements. Restrict the store_blobs
UPDATE to rows whose reference count actually changes, and verify the production
schema defines supporting indexes for store_documents.blob_hash and
store_doc_history.doc_id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f7d83745-86d2-4ab9-83fc-b62898bd1b58
📒 Files selected for processing (1)
crates/agentflare-store/src/migrations.rs
|
@coderabbitai resume @coderabbitai review Please re-review
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Target commit and changed files =='
git show --no-ext-diff --format=fuller --stat 03eba4b
git diff-tree --no-commit-id --name-only -r 03eba4b
printf '%s\n' '== Relevant module outlines =='
ast-grep outline crates/agentflare-store/src/migrations.rs --items all || true
fd -t f -e rs . crates | while read -r f; do
if rg -q 'db_kit::open_file|open_file\s*\(|foreign_keys|DEDUP_AND_UNIQUE_INDEX_MIGRATION|store_blobs|store_doc_history' "$f"; then
echo "--- $f"
rg -n -C 4 'db_kit::open_file|(?:\bopen_file\s*\()|foreign_keys|DEDUP_AND_UNIQUE_INDEX_MIGRATION|store_blobs|store_doc_history' "$f"
fi
done
printf '%s\n' '== Migration diff =='
git show --no-ext-diff --format= --unified=100 03eba4b -- crates/agentflare-store/src/migrations.rs
printf '%s\n' '== Store module outline and DB-opening contexts =='
ast-grep outline crates/agentflare-store/src --items all || true
rg -n -C 6 'db_kit::open_file|(?:\bopen_file\s*\()|Connection::open|foreign_keys' crates/agentflare-storeLength of output: 65052 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== db-kit connection helpers =='
ast-grep outline crates/agentflare-db-kit/src/open.rs --items all
cat -n crates/agentflare-db-kit/src/open.rs
printf '%s\n' '== Store constructors =='
sed -n '1,100p' crates/agentflare-store/src/lib.rs
printf '%s\n' '== Production blob reference accounting and document lifecycle =='
rg -n -C 10 'fn blob_ref\b|fn blob_unref\b|blob_ref\s*\(|blob_unref\s*\(|blob_hash|fn doc_delete\b|DELETE FROM store_documents|INSERT INTO store_doc_history' \
crates/agentflare-store/src/blobs.rs crates/agentflare-store/src/documents.rs
printf '%s\n' '== All uses of agentflare-store migrations and Store constructors =='
rg -n -C 4 'agentflare_store::migrations|agentflare_store::Store::(?:open_file|open_memory)|Store::(?:open_file|open_memory)|migrations::migrations\s*\(' \
--glob '*.rs' .
printf '%s\n' '== Foreign-key configuration, including PRAGMA SQL literals =='
rg -n -i -C 3 'pragma_update\s*\([^;]*foreign_keys|PRAGMA\s+foreign_keys|foreign_keys\s*=' \
--glob '*.rs' --glob '*.sql' .
printf '%s\n' '== db-kit Cargo features that could change SQLite defaults =='
fd -t f 'Cargo.toml' crates/agentflare-db-kit -x sh -c 'echo "--- $1"; cat -n "$1"' sh {}Length of output: 91403 @getappz Re-review complete — no remaining finding on the dedup migration.
✅ Action performedReviews resumed. Review finished.
|
1 participant
What
Two fixes to the document store, both recovered from uncommitted work sitting in the
task/343worktree — found while auditing open issues against local branches, unreleated to any of them, and at risk of being lost when the worktree got reclaimed.1. The unique-index migration can brick a store
CREATE UNIQUE INDEX idx_docs_project_path ON store_documents(project_id, path)was added as a bare migration. Uniqueness on that pair had until then been enforced only by the single-writer application path, so a database written before the index existed can already hold duplicate rows — and index creation fails outright against those.That failure is not cosmetic:
user_versionnever advances and the index never exists;write_batch_itemsupserts viaON CONFLICT(project_id, path) DO UPDATE, which requires that index;Per-item indexing (#343) makes this reachable: one crate contributes hundreds of
docsrs/<crate>/latest/item/...paths, so the write volume that could produce a stray duplicate is ~10^3 per package rather than ~1.Fix: dedup first — keep
MAX(rowid)per(project_id, path)— then create the index.Editing the migration's SQL in place is the correct remedy here rather than appending a new one, and specifically because the failed attempt rolled back:
DELETEis a no-op it will never run anyway;user_version, so it re-runs this migration with the dedup step and recovers.Appending a follow-up migration could not fix the stuck case, since migration N+1 is never reached while N keeps failing.
The SQL is a named constant (
DEDUP_AND_UNIQUE_INDEX_MIGRATION) rather than inlined, so the dedup behaviour can be tested against a minimal schema without replaying the full production migration sequence to reach it.2.
write_batch_itemsre-prepared its statements per itemFour statements — select / upsert / FTS delete / FTS insert — were prepared inside the per-item loop. Hoisted onto
prepare_cachedoutside it, in a block so their borrow oftxis released beforecommit(). Same reasoning as above: batches are hundreds of items, not a handful.Verification
cargo test --workspace— 834 bin tests + every crate suite, 0 failurescargo clippy --locked --workspace --all-targets --all-features -- -D warnings -A unsafe_code -A clippy::pedantic(CI's exact invocation) — cleancargo fmt --all— cleanmigrations::tests::dedup_migration_keeps_newest_row_and_enables_the_unique_indexconfirmed green by name: seeds two duplicate rows, asserts the migration SQL applies without error, keeps the newest row, and that the index then actually rejects a fresh duplicate.Summary by CodeRabbit
Bug Fixes
(project, path)consistently after consolidation.Performance
Tests