feat(asset-store): migrate to agentflare-store documents+blobs - #282
Merged
Conversation
…ts+blobs (#185) Replace agentflare-backend::asset's storage with agentflare-store's documents+blobs (content-addressed blob store with dedup/unref), and route handoff/asset MCP tool handlers through it.
…ed forever - Store::doc_get() didn't filter deleted_at, so asset get/delete on an already-deleted id returned 200 instead of not-found (regression from agentflare_backend::asset::get, worse now that delete also physically purges the blob once ref_count hits 0 -- soft-deleted docs pointed at already-gone content). - with_store()'s one-time legacy backfill only fires when backend_db is unlocked at call time; attach always nests with_store inside with_backend_db so it never got a chance there. Trigger it once, unlocked, at the top of attach so first-run migration isn't dependent on call order. - replaced a lock().unwrap() in the backfill path with a graceful skip on a poisoned mutex, matching the rest of with_store/with_backend_db.
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR moves MCP asset persistence from backend asset rows and filesystem paths to store documents and blobs. It adds document metadata and size schema support, migrates legacy assets, updates handoff attachments, and adds deletion lifecycle coverage. ChangesStore-backed asset migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant AgentflareMcp
participant StoreDocuments
participant BlobStore
MCPClient->>AgentflareMcp: attach, get, list, or delete asset
AgentflareMcp->>StoreDocuments: upsert or retrieve asset document
StoreDocuments->>BlobStore: store, retrieve, or unref blob
BlobStore-->>StoreDocuments: blob content or hash
StoreDocuments-->>AgentflareMcp: document result
AgentflareMcp-->>MCPClient: asset JSON response
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
🧹 Nitpick comments (1)
src/mcp_server/asset.rs (1)
179-213: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftBoth asset
listand handoff version-counting do a full-workspacedoc_listscan. Root cause: theStoreAPI only exposes a whole-workspacedoc_list, so every entity-scoped read filters the entire workspace's documents client-side.
src/mcp_server/asset.rs#L179-L213: filters all workspace docs bydoc_type == "asset"and an optional path prefix on everyasset(list)call.src/mcp_server/handoff.rs#L116-L155: fetches all workspace docs just to count siblings underitem_attachment/{item.id}for the display "asset_version".Consider adding a prefix-scoped query (e.g.
doc_list_by_prefix) toagentflare-store's document API so both call sites can avoid scanning documents outside their entity/prefix.🤖 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 `@src/mcp_server/asset.rs` around lines 179 - 213, Add a prefix-scoped document query to the agentflare-store API, such as doc_list_by_prefix, and implement it in the Store layer. Update asset list in src/mcp_server/asset.rs lines 179-213 to query only the resolved prefix when one is provided, while preserving workspace-wide filtering for an empty prefix; update handoff version-counting in src/mcp_server/handoff.rs lines 116-155 to use the item_attachment/{item.id} prefix instead of doc_list. Keep the existing asset-type filtering and result behavior unchanged.
🤖 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 `@src/asset_store.rs`:
- Around line 41-90: Update backfill_legacy_assets to handle read_file,
blob_store, and per-asset doc_upsert_with_opts failures within the loop without
returning early; skip or log the failing asset, continue processing remaining
assets, and still write _asset_backfill_done after the batch completes so
successful assets are not replayed.
In `@src/mcp_server.rs`:
- Around line 627-655: Update with_store’s one-time backfill flow to explicitly
handle poisoned backend_db and store mutexes instead of silently skipping them,
while avoiding deadlock for the nested backend case. Capture
backfill_legacy_assets failures, log or propagate them according to the existing
error-handling conventions, and ensure the backfill completion state is recorded
or otherwise prevents retrying already-processed assets after partial failure,
avoiding duplicate history versions.
In `@src/mcp_server/asset.rs`:
- Around line 66-72: Ensure the initial backfill trigger in the attach flow
opens or initializes backend_db before invoking self.with_store(|_| ())?. Update
the surrounding attach logic, using the existing backend_db
initialization/access mechanism, so the trigger observes a populated connection
on the first attach call while backend_db remains unlocked.
- Around line 214-231: In the "delete" branch of the asset operation, update the
store closure to call doc_delete before blob_unref. Preserve the existing
blob_hash lookup and error mappings, and only release the blob reference after
the document deletion succeeds.
---
Nitpick comments:
In `@src/mcp_server/asset.rs`:
- Around line 179-213: Add a prefix-scoped document query to the
agentflare-store API, such as doc_list_by_prefix, and implement it in the Store
layer. Update asset list in src/mcp_server/asset.rs lines 179-213 to query only
the resolved prefix when one is provided, while preserving workspace-wide
filtering for an empty prefix; update handoff version-counting in
src/mcp_server/handoff.rs lines 116-155 to use the item_attachment/{item.id}
prefix instead of doc_list. Keep the existing asset-type filtering and result
behavior unchanged.
🪄 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: 24a37168-0474-4f36-ad67-260bdd58cbbb
📒 Files selected for processing (10)
crates/agentflare-backend/src/asset.rscrates/agentflare-store/src/documents.rscrates/agentflare-store/src/migrations.rssrc/asset_store.rssrc/main.rssrc/mcp_server.rssrc/mcp_server/asset.rssrc/mcp_server/handoff.rssrc/mcp_server/tests/artifact_tests.rssrc/mcp_server/tests/asset_tests.rs
Comment on lines
+627
to
+655
| /// Lock the agentflare-store, lazily opening it on first use. | ||
| /// After opening, runs the one-time asset backfill (best-effort, | ||
| /// skipped if backend_db is already locked by this thread). | ||
| fn with_store<T>( | ||
| &self, | ||
| f: impl FnOnce(&agentflare_store::Store) -> T, | ||
| ) -> Result<T, ErrorData> { | ||
| self.ensure_store()?; | ||
|
|
||
| // One-time backfill: try_lock to avoid deadlock when called from | ||
| // within with_backend_db (attach handler nests store inside backend). | ||
| // If backend isn't open yet, skip — with_backend_db will handle it. | ||
| if let Ok(bg) = self.backend_db.try_lock() { | ||
| if let Some(ref conn) = *bg { | ||
| let base_path = crate::paths::home().join(".agentflare"); | ||
| if let Ok(s) = self.store.lock() { | ||
| if let Some(ref store) = *s { | ||
| let _ = crate::asset_store::backfill_legacy_assets(store, conn, &base_path); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let guard = self | ||
| .store | ||
| .lock() | ||
| .map_err(|e| ErrorData::internal_error(e.to_string(), None))?; | ||
| Ok(f(guard.as_ref().expect("ensure_store just initialized it"))) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
python3 - <<'PY'
import os, subprocess, textwrap, json, sys
def sh(cmd):
print(f"\n$ {cmd}")
r = subprocess.run(cmd, shell=True, text=True, capture_output=True)
print(r.stdout)
if r.stderr:
print(r.stderr, file=sys.stderr)
print("exit:", r.returncode)
return r.returncode, r.stdout, r.stderr
# Locate relevant symbols and read focused slices.
commands = [
r"rg -n \"fn with_store|try_lock\(|backfill_legacy_assets|_asset_backfill_done|Poison|poison\" src/mcp_server.rs src/asset_store.rs",
r"sed -n '600,690p' src/mcp_server.rs",
r"sed -n '1,260p' src/asset_store.rs",
]
for c in commands:
sh(c)
PYRepository: getappz/agentflare
Length of output: 7704
🏁 Script executed:
python3 - <<'PY'
import subprocess, sys
def sh(cmd):
print(f"\n$ {cmd}")
r = subprocess.run(cmd, shell=True, text=True, capture_output=True)
print(r.stdout)
if r.stderr:
print(r.stderr, file=sys.stderr)
print("exit:", r.returncode)
for cmd in [
r"rg -n \"backend_db|with_backend_db|clear_poison|Poisoned|TryLockError\" src/mcp_server.rs src",
r"sed -n '520,610p' src/mcp_server.rs",
]:
sh(cmd)
PYRepository: getappz/agentflare
Length of output: 3792
🏁 Script executed:
python3 - <<'PY'
import subprocess, sys
def sh(cmd):
print(f"\n$ {cmd}")
r = subprocess.run(cmd, shell=True, text=True, capture_output=True)
print(r.stdout)
if r.stderr:
print(r.stderr, file=sys.stderr)
print("exit:", r.returncode)
for cmd in [
r"rg -n \"doc_upsert_with_opts|struct DocUpsertOpts|fn doc_upsert|version\" .",
r"rg -n \"_asset_backfill_done\" .",
]:
sh(cmd)
PYRepository: getappz/agentflare
Length of output: 586
🏁 Script executed:
python3 - <<'PY'
import subprocess, sys
def sh(cmd):
print(f"\n$ {cmd}")
r = subprocess.run(cmd, shell=True, text=True, capture_output=True)
print(r.stdout)
if r.stderr:
print(r.stderr, file=sys.stderr)
print("exit:", r.returncode)
for cmd in [
r"rg -n 'doc_upsert_with_opts|fn doc_upsert|struct DocUpsertOpts' src",
r"rg -n '_asset_backfill_done' src",
]:
sh(cmd)
PYRepository: getappz/agentflare
Length of output: 690
🏁 Script executed:
python3 - <<'PY'
import subprocess, sys
def sh(cmd):
print(f"\n$ {cmd}")
r = subprocess.run(cmd, shell=True, text=True, capture_output=True)
print(r.stdout)
if r.stderr:
print(r.stderr, file=sys.stderr)
print("exit:", r.returncode)
for cmd in [
r"git ls-files | rg '(^|/)(store|agentflare_store|src)/.*\.(rs|toml)$'",
r"rg -n 'doc_upsert_with_opts|fn doc_upsert|upsert_with_opts|history|version' .",
]:
sh(cmd)
PYRepository: getappz/agentflare
Length of output: 50375
🏁 Script executed:
python3 - <<'PY'
import subprocess, sys
def sh(cmd):
print(f"\n$ {cmd}")
r = subprocess.run(cmd, shell=True, text=True, capture_output=True)
print(r.stdout)
if r.stderr:
print(r.stderr, file=sys.stderr)
print("exit:", r.returncode)
for cmd in [
r"sed -n '100,190p' crates/agentflare-store/src/documents.rs",
r"sed -n '730,810p' crates/agentflare-store/src/documents.rs",
]:
sh(cmd)
PYRepository: getappz/agentflare
Length of output: 6247
Handle poisoned mutexes and backfill failures src/mcp_server.rs:627-655
try_lock()dropsPoisoned, so a poisonedbackend_dbwill keep skipping this backfill.backfill_legacy_assets(...)errors are ignored, and_asset_backfill_doneis only written on success; a partial failure will rerun the whole import on the next call. Sincedoc_upsert_with_opts()increments existing docs, that can add duplicate history versions for already-migrated assets.
🤖 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 `@src/mcp_server.rs` around lines 627 - 655, Update with_store’s one-time
backfill flow to explicitly handle poisoned backend_db and store mutexes instead
of silently skipping them, while avoiding deadlock for the nested backend case.
Capture backfill_legacy_assets failures, log or propagate them according to the
existing error-handling conventions, and ensure the backfill completion state is
recorded or otherwise prevents retrying already-processed assets after partial
failure, avoiding duplicate history versions.
…-features clippy errors Agentflare-Agent: claude-code_2-1-215_agent Agentflare-Branch: item-185-asset-store-migration
- backfill_legacy_assets: don't abort the whole batch on one bad legacy asset; skip and log it, migrate the rest, and always write the completion marker. Previously a single unreadable asset meant the marker never got written, so every later with_store() call replayed the entire batch, re-upserting already-migrated docs and bumping their version/history each time. - attach: open backend_db (and release it) before triggering the backfill check, so a totally fresh instance's very first attach call can actually backfill, not just the second one onward. - delete: soft-delete the document before unref'ing its blob, so a failure between the two steps can't leave the last reference's content purged while the asset still reads as live. Agentflare-Agent: claude-code_2-1-215_agent Agentflare-Branch: item-185-asset-store-migration
Owner
Author
|
Addressed all 4 CodeRabbit findings in 4a19ec2:
Added a regression test ( |
Agentflare-Agent: claude-code_2-1-215_agent Agentflare-Branch: item-185-asset-store-migration
3 tasks
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
agentflare_backend::assettable + raw filesystem storage withagentflare_store::Store(content-addressed blobs + versioned documents), for both theassetandhandoffMCP tools.Store::doc_get()now excludes soft-deleted rows (a deleted asset was stillget/delete-able, worse once its blob was purged); the legacy backfill no longer silently skips forever for attach-only workflows; a lock().unwrap() in the backfill path now degrades gracefully instead of panicking.Test plan
cargo test --workspace --all-features: 703 passed / 7 failed (7 failures are pre-existing, orchestrator-sandboxedgit worktreetests, unrelated to this change)cargo clippy -p agentflare-store -p agentflare --all-targets: no new warningsdocuments::tests::soft_delete_and_list(extended),asset_get_and_delete_after_delete_return_not_foundcargo fmt: this branch has pre-existing formatting drift (12 files) from the original commit, not addressed here — recommend a follow-up fmt-only pass before mergeCloses #179.
Summary by CodeRabbit
metadataandsize, including schema support and preserved version history.getand repeateddelete.get/deletecoverage; updated handoff filename assertions; removed the older content-dedup test.