Repository navigation
Conversation
doc_delete and flare-docs' stale-item reconciliation soft-deleted rows without ever calling blob_unref, so every deleted blob-backed document orphaned its file under <root>/blobs permanently. For file-backed stores that is where the bytes actually live -- store_blob_chunks is only used by in-memory stores -- which made it the dominant growth path behind issue #339, ahead of the database file itself. Both sites now release the ref after the row is soft-deleted, matching the ordering the asset MCP tool's delete path already documents, and outside the connection guard since blob_unref takes the same non-reentrant mutex. A re-delete of an already soft-deleted row no longer unrefs a second time. Tests are file-backed on purpose: an in-memory store keeps blob bytes in store_blob_chunks, so an in-memory test would pass while the real on-disk path kept leaking.
doc_delete releases the document's blob reference itself, so the asset tool's explicit unref dropped two references for a single delete. The existing asset_shared_storage_delete_safety test caught it: deleting one of two assets sharing identical bytes reclaimed the file out from under the surviving asset.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBlob lifecycle handling now releases superseded, deleted, and reconciled document blobs after durable database changes. History snapshots and shared references preserve needed files, while MCP asset deletion delegates cleanup to ChangesBlob reference lifecycle cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MCPAsset
participant Store_doc_delete
participant SQLite
participant BlobStore
MCPAsset->>Store_doc_delete: validate and delete asset document
Store_doc_delete->>SQLite: soft-delete document
Store_doc_delete->>BlobStore: unref blob once
BlobStore-->>MCPAsset: reclaim file if final reference
sequenceDiagram
participant DocsStore
participant SQLite
participant BlobStore
DocsStore->>SQLite: find and soft-delete stale documents
DocsStore->>SQLite: commit reconciliation changes
DocsStore->>BlobStore: unref stale blob hashes
BlobStore-->>DocsStore: remove unreferenced files
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: 2
🤖 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/documents.rs`:
- Around line 415-434: Update doc_upsert_with_opts to detect when an existing
row’s blob_hash is replaced and unref the previous blob after the row update
succeeds. Preserve reference counts when the hash is unchanged, and reuse the
existing blob_unref mechanism used by the delete path.
In `@crates/flare-docs/src/store.rs`:
- Around line 188-196: Update the stale-blob cleanup loop in the reconciliation
flow around stale_blobs and blob_unref so one unref failure does not abort
processing. Attempt blob_unref for every stale hash, retain the first error (or
equivalent aggregate error), and propagate it only after all hashes have been
processed.
🪄 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: 9ec019c4-cdb0-42bb-ae1d-ebd16322707d
📒 Files selected for processing (4)
crates/agentflare-store/src/blobs.rscrates/agentflare-store/src/documents.rscrates/flare-docs/src/store.rssrc/mcp_server/asset.rs
…rror Addresses both CodeRabbit findings on #343. A blob replaced by a new one on upsert was never released, the third variant of the same leak this PR is about. It is only orphaned when nothing else kept it, though: a history-tracking upsert has just snapshotted the old hash into store_doc_history, which is what doc_history and diff read the previous version's bytes back through. Reclaiming unconditionally emptied artifact history -- two agentflare-artifacts tests caught it. The release is now gated on whether a snapshot row was written, bound once so the two decisions cannot drift. The flare-docs stale-blob loop used ? on the first unref, stranding the rest of the batch as orphans. It now attempts every hash and reports the first failure afterwards; the rows are already committed, so there is nothing to roll back and nothing gained by stopping early.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/agentflare-store/src/documents.rs (2)
449-455: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not resurrect a released blob reference without reacquiring it.
After this delete path releases the last reference,
doc_upsert_with_optscan reactivate the soft-deleted row withblob_hash: None. It preserves the old hash while clearingdeleted_at, leaving a live document pointing to reclaimed bytes. Track prior deletion state in the upsert query and reject this restore or clear the stale hash unless the caller supplies a newly acquired blob reference. Add a file-backed delete-then-upsert regression test.🤖 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/documents.rs` around lines 449 - 455, Update doc_upsert_with_opts to track whether the existing row was soft-deleted; when restoring a deleted row with blob_hash: None, do not preserve its released hash—reject the restore or clear the stale hash, while retaining normal updates with a newly acquired blob reference. Add a file-backed regression test covering delete followed by upsert without a blob hash.
413-441: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winMake the live-to-deleted transition atomic before releasing the blob.
Line 429 unconditionally updates the row after a non-transactional
already_deletedread. TwoStoreinstances on the same file can both observefalseand both callblob_unref; the second release can reclaim a blob still referenced by another live document. Use aBEGIN IMMEDIATEtransaction for the read/update/FTS delete, and release only when a conditionalWHERE deleted_at IS NULLupdate affects one row.Suggested direction
- let conn = self.conn(); + let conn = self.conn(); + let tx = rusqlite::Transaction::new_unchecked( + &conn, + rusqlite::TransactionBehavior::Immediate, + )?; ... - conn.execute( - "UPDATE store_documents SET deleted_at = ?1 WHERE id = ?2", + let transitioned = tx.execute( + "UPDATE store_documents + SET deleted_at = ?1 + WHERE id = ?2 AND deleted_at IS NULL", params![now, id], )?; ... - (true, if already_deleted { None } else { blob_hash }) + tx.commit()?; + (true, if transitioned == 1 { blob_hash } else { None })🤖 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/documents.rs` around lines 413 - 441, Make the delete flow atomic by wrapping the document read, conditional soft-delete update, and FTS-row deletion in a BEGIN IMMEDIATE transaction. In the relevant delete method, update store_documents with WHERE id = ? and deleted_at IS NULL, use the affected-row count to determine whether this call performed the live-to-deleted transition, and only return the blob hash for that transition before committing; preserve the no-document result and avoid blob_unref for already-deleted rows.
🤖 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.
Outside diff comments:
In `@crates/agentflare-store/src/documents.rs`:
- Around line 449-455: Update doc_upsert_with_opts to track whether the existing
row was soft-deleted; when restoring a deleted row with blob_hash: None, do not
preserve its released hash—reject the restore or clear the stale hash, while
retaining normal updates with a newly acquired blob reference. Add a file-backed
regression test covering delete followed by upsert without a blob hash.
- Around line 413-441: Make the delete flow atomic by wrapping the document
read, conditional soft-delete update, and FTS-row deletion in a BEGIN IMMEDIATE
transaction. In the relevant delete method, update store_documents with WHERE id
= ? and deleted_at IS NULL, use the affected-row count to determine whether this
call performed the live-to-deleted transition, and only return the blob hash for
that transition before committing; preserve the no-document result and avoid
blob_unref for already-deleted rows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 98b3984e-1458-4d41-9730-5cb1a52a39ac
📒 Files selected for processing (3)
crates/agentflare-store/src/blobs.rscrates/agentflare-store/src/documents.rscrates/flare-docs/src/store.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/flare-docs/src/store.rs
This was referenced Jul 26, 2026
getappz
added a commit
that referenced
this pull request
Jul 26, 2026
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.
getappz
added a commit
that referenced
this pull request
Jul 26, 2026
* fix(store): dedup documents before creating the unique index `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. * fix(store): release history and blob refs when dedup discards a row 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Step 1 of #339. Fixes the leak that issue turned out to be mostly about; the size cap, LRU and VACUUM work is deliberately not here.
What was wrong
Store::blob_unrefis a working ref-counted GC — at zero refs it deletes the chunk rows and the on-disk file. Nothing called it on the document delete paths:doc_delete(crates/agentflare-store/src/documents.rs) setdeleted_atand cleared the FTS row, but never unreffedblob_hash.crates/flare-docs/src/store.rs, added in flare-docs: refetching a crate never removes stale per-item docs that disappeared upstream #334/fix: flare-docs stale-item reconciliation + doc history-skip check #337) did the same.So every deleted blob-backed document orphaned its file under
<root>/blobspermanently. That is where the bytes actually live for file-backed stores —store_blob_chunksis only used by in-memory ones — which makes it the dominant growth path behind #339, ahead of the database file itself. Measured on a live install:flare-docs.db1.8 MB against~/.agentflare/blobsat 5.7 MB across 365 files.What changed
Both delete paths now release the reference after the row is soft-deleted, matching the ordering the asset MCP tool's delete path already documented: the row is the source of truth, so a failure leaks a file rather than stranding a live document with reclaimed content. Both do it outside the connection guard, since
blob_unreftakes the same non-reentrant mutex and opens its own transaction.Re-deleting an already soft-deleted row no longer unrefs a second time — only the delete that actually transitions live to deleted owns a reference.
The second commit removes the explicit
blob_unrefinsrc/mcp_server/asset.rs. Withdoc_deleteowning the reference, that path was dropping two refs per delete. This was caught by the existingasset_shared_storage_delete_safetytest, not by inspection: deleting one of two assets sharing identical bytes reclaimed the file out from under the survivor.Tests
Three new tests, all file-backed on purpose — an in-memory store keeps blob bytes in
store_blob_chunks, so an in-memory version would pass while the real on-disk path kept leaking:Each was confirmed to fail before the fix. The flare-docs one passed on its first run, so the unref loop was temporarily stubbed out to verify the test actually fails without it, then restored.
cargo test --workspace: 824 passed, 0 failed.cargo clippy -p agentflare-store -p flare-docs --all-targets --all-features -- -D warnings: clean.Known ceiling, not addressed here
A caller that resurrects a soft-deleted row via
doc_upsert_with_optswithout supplying a freshblob_hashwould point at reclaimed bytes. Every blob-backed caller today (assets, artifacts, flare-docs rustdoc) always passes one on upsert, so this is unreachable now; there is a comment at the unref site saying so.Unrelated pre-existing failure
cargo clippy --workspace --all-targets --all-features -- -D warningsfails on 19unsafe_codeand 2items_after_statementserrors inagent-registry,src/paths.rs,src/agents.rsand others — all files this branch does not touch, so master fails identically under local stable 1.95.0. The repo pins no toolchain and CI resolvesstablethrough a cached action, so CI is probably on an older stable that lacks these lints.Summary by CodeRabbit