Conversation
index_items now calls upsert_batch_reconciled, which soft-deletes any previously-indexed item under a crate's item/ path prefix that's absent from the fresh fetch, in the same transaction as the batch upsert. Fixes #334.
…all opts fields Previously only content and blob_hash were compared to decide whether to skip the store_doc_history snapshot. A re-upsert with unchanged content but a different title, doc_type, mime, tags, metadata, size, session_id, or source silently skipped history even though the document's persisted state changed. Extend the existing-row read to include title/doc_type/ tags/session_id/source and require every Some(...)-provided opts field to match its stored value for the write to count as a true no-op. Fixes #333.
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe document store now detects changes across provided persisted fields before writing history. Flare-docs indexing now reconciles each crate item batch against stored paths, soft-deleting items absent from subsequent refetches. ChangesDocument history detection
Crate item index reconciliation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant index_items
participant DocsStore
participant SQLite
index_items->>DocsStore: upsert_batch_reconciled(item_prefix, batch)
DocsStore->>SQLite: upsert current crate items
DocsStore->>SQLite: find stored paths under item_prefix
DocsStore->>SQLite: soft-delete paths absent from batch
DocsStore-->>index_items: commit transaction and return count
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
getappz
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/agentflare-store/src/documents.rs (1)
910-929: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the required metadata-only regression test.
These tests cover title-only changes and title/tags no-ops, but not a same-content re-upsert with changed
metadata. Add that case and assert the history row contains the old metadata; also verify identical metadata remains a no-op.🤖 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 910 - 929, Extend the document upsert regression tests near identical_content_and_opts_reupsert_skips_history_row to cover metadata-only changes: re-upsert identical content with changed metadata and assert history contains the previous metadata, then re-upsert with identical metadata and assert no additional history row is created.
🤖 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 213-214: Update the unchanged check in the document re-upsert flow
to treat an omitted opts.blob_hash as unchanged, while still comparing hashes
when opts.blob_hash is Some. Preserve the existing old_content comparison and
avoid creating history rows for no-op updates to blob-backed documents.
In `@crates/flare-docs/src/store.rs`:
- Around line 136-155: Update the reconciliation soft-delete loop in the
batch-writing flow to obtain each matching store_documents rowid and delete that
rowid from the manually synchronized store_docs_fts table within the same
transaction as the deleted_at update. Mirror the explicit FTS DELETE pattern
used by write_batch_items, while preserving the existing fresh_paths filtering
and soft-delete behavior.
- Around line 136-149: Make the stale-path filtering in the refetch logic
case-sensitive: after querying stored paths, require each stored_path to satisfy
stored_path.starts_with(path_prefix) before soft-deleting it. Keep the existing
fresh_paths containment check and deletion behavior unchanged.
---
Nitpick comments:
In `@crates/agentflare-store/src/documents.rs`:
- Around line 910-929: Extend the document upsert regression tests near
identical_content_and_opts_reupsert_skips_history_row to cover metadata-only
changes: re-upsert identical content with changed metadata and assert history
contains the previous metadata, then re-upsert with identical metadata and
assert no additional history row is created.
🪄 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: 1632a001-1afb-4640-a9d9-22bf39098f17
📒 Files selected for processing (3)
crates/agentflare-store/src/documents.rscrates/flare-docs/src/rustdoc.rscrates/flare-docs/src/store.rs
- doc_upsert_with_opts: treat an omitted opts.blob_hash as making no claim about the field (matching every other opts field), instead of forcing unchanged=false whenever the stored row already has a blob_hash set. - upsert_batch_reconciled: soft-deleting a stale item now also deletes its store_docs_fts row (store_docs_fts is manually synced, not content=/ external-content, so a stale entry would otherwise remain directly matchable via a raw FTS query even though doc_search's own deleted_at filter excludes it). - upsert_batch_reconciled: re-check the LIKE-matched path prefix with a case-sensitive starts_with, since SQLite's LIKE is case-insensitive for ASCII by default and a prefix match must be exact. Adds regression tests for all three plus the metadata-only history test CodeRabbit's nitpick asked for.
This was referenced Jul 26, 2026
This was referenced Jul 26, 2026
getappz
added a commit
that referenced
this pull request
Jul 27, 2026
…en sync (#347) * refactor(store): drive every FTS5 index from triggers, not hand-written sync The four fts5 indexes in this repo were standalone tables kept in sync by hand from Rust, across 10 write sites in 4 files. Every base-table write path was a chance to desync -- not hypothetically: #334/#337 was exactly that bug, a soft-delete that forgot its store_docs_fts row and left stale documents matchable. Any new purge path (#339) would have inherited the same rule. Each index is now external-content with AFTER INSERT/DELETE/UPDATE triggers, following src/memory/schema.rs. The FTS write is a property of the base table, so a call site can no longer forget it, and the index no longer stores its own copy of the indexed text. The store_docs_fts triggers are guarded on deleted_at rather than being straight mirrors: soft-deleted documents stay out of the index (what the manual code did), and on an external-content table a 'delete' naming an unindexed row corrupts it -- which an unguarded AFTER UPDATE would do on every resurrecting upsert. AFTER UPDATE is scoped to the indexed columns so the optional-field UPDATEs in doc_upsert_with_opts and the per-load bandit writes in skill-registry don't re-tokenize whole documents. The two registries have no migration framework, so open_db drops a legacy standalone index and refills from the base table in the same open -- waiting for the next rebuild would leave search empty in between. Adds Store::doc_fts_rebuild as the repair path: store_documents has a TEXT primary key, so VACUUM (which #339 proposes) may renumber the implicit rowids this index and doc_search's JOIN are both keyed on. Also fixes three assertions this change would otherwise have defanged: `SELECT count(*)` on an external-content fts5 table is answered from the content table, so it counts base-table rows whether or not they are indexed. Index state has to be asserted through MATCH. * fix(store): make the legacy FTS conversion atomic; apply fmt CodeRabbit's finding on #347: the drop/recreate/backfill in both registries was three separate statements, and the halfway state is not self-correcting. With the index dropped but not yet refilled, the next open finds no such table, reads `legacy` as 0, and recreates an empty index over a populated base table -- search silently missing everything until something forces a full rebuild. One transaction each; SQLite makes DDL transactional. Same treatment for doc_fts_rebuild's clear-then-refill. Also splits optional_field_updates_do_not_disturb_the_index: its assertion held whether or not the trigger was scoped, since an unscoped AFTER UPDATE produces an identical index, just nine times over. Scoping is a property of the DDL, so it is now asserted on the trigger definition. And runs cargo fmt, which the first commit missed.
getappz
added a commit
that referenced
this pull request
Jul 27, 2026
…reclaim (#348) * feat(store): bound the docs cache with retention, eviction, and page reclaim The docs cache had no ceiling of any kind. Soft-deleted rows (#334/#337) were never physically removed, no policy capped total size, and nothing ever handed freed pages back to the filesystem -- a long-lived install refreshing a rotating set of packages grows without bound (#339). agentflare-store gains a maintenance module: Store::gc purges tombstones past a 7-day retention, evicts the least recently updated live documents while the project is over a 256 MB budget, then reclaims. Blob accounting is the part worth reading twice -- a tombstone gave up its own blob reference when it was soft-deleted, so purging must not release it again, while an evicted live document still holds one. History snapshots hold references of their own and are released as their rows go, the same leak the soft-delete path closed earlier in this issue. Size is measured as live content plus referenced blobs, not the .db file: blobs dominate a docs cache (5.7 MB against a 1.8 MB database when measured), and the file does not shrink at the moment rows are deleted, which would make it useless as the loop variable for eviction. db-kit now asks for auto_vacuum=INCREMENTAL and synchronous=NORMAL, and sets foreign_keys=ON explicitly instead of inheriting it from the linked SQLite build. The auto_vacuum pragma goes ahead of journal_mode and that ordering is load-bearing: switching to WAL writes the header, after which the setting is silently ignored -- and it reads back as INCREMENTAL until the first table lands, so a test that checks too early passes either way. Databases created before this stay in NONE mode, so gc runs one full VACUUM to convert them, rebuilding the external-content FTS index behind it, and uses incremental_vacuum from then on. Collection runs on the flare-docs fetch path rather than a schedule -- every route that grows the cache funnels through it, so nothing has to own a timer -- and non-empty runs are journaled to ~/.agentflare/audit/gc.jsonl beside the git shim's log. * fix(store): scope the budget to the project and re-measure between evictions Two findings from review, both about eviction removing more than it should. cache_bytes summed all of store_blobs while the content term was project-scoped, so in a multi-project database another project's bytes decided when this one evicted -- and since evicting this project's documents never brought that total down, it emptied the project and still finished over budget. Only blobs referenced by this project's live documents count now. Eviction also estimated each document's contribution up front and deleted a whole batch against it. A blob two documents share is only reclaimed when the second one goes, so the estimate under-counts what the batch frees, and an estimate that can never reach its target walks the candidate list to the end. Replaced with a loop that re-measures after each eviction and stops the moment the project fits, bounded by the candidate query returning None. Cache maintenance also ran inline on the single-threaded MCP runtime, which is what that function's own doc comment gives as the reason the fetch uses spawn_blocking. A purge can remove thousands of rows, and the first run against a pre-auto_vacuum database rewrites the whole file and rebuilds the search index. It now goes to the blocking pool, detached -- the caller's answer does not depend on it. Not changed: the journal_mode pragma flagged as needing pragma_update_and_check. rusqlite 0.40.1's pragma_update runs the statement through execute_batch, which discards returned rows, so ExecuteReturnedResults cannot arise -- and the line predates this branch. * fix(mcp): await the docs-store lock instead of parking the runtime Detaching GC onto the blocking pool moved the work off the runtime but not the contention: it holds the same std::sync::Mutex every docs handler takes, so a request arriving mid-maintenance blocked the MCP runtime's only thread for as long as a purge or a full VACUUM ran -- stalling every other tool call, not just the docs ones. The store now sits behind a tokio mutex. Handlers await it; the GC task, already off the runtime, takes it with blocking_lock.
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
1 participant
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.
Summary
index_itemsnow reconciles per-item docs against each fetch — items that disappear from a crate's index (renamed/removed/made private) are soft-deleted instead of lingering forever. Fixes flare-docs: refetching a crate never removes stale per-item docs that disappeared upstream #334.doc_upsert_with_opts's history-skip check now compares everySome(...)-provided opts field (title, doc_type, mime, tags, metadata, size, session_id, source), not just content/blob_hash, so metadata-only changes still write a history row. Fixes agentflare-store: doc_upsert_with_opts history-skip check ignores non-content field changes #333.Both were flagged by CodeRabbit on #323 and deferred as follow-ups.
Test plan
cargo test -p flare-docs(27 passed, incl. new refetch-reconciliation regression test)cargo test -p agentflare-store(48 passed, incl. 2 new history-skip tests)cargo clippy -p flare-docs --all-targets -- -D warnings/cargo clippy -p agentflare-store --all-targets -- -D warningscleancargo build --workspace --all-featurescargo test --workspace --all-featuresSummary by CodeRabbit