Skip to content

fix(store): give each db its own blob dir so GC cannot delete a neighbour's content - #351

Merged
getappz merged 1 commit into
masterfrom
fix/385-blob-namespace
Jul 27, 2026
Merged

fix(store): give each db its own blob dir so GC cannot delete a neighbour's content#351
getappz merged 1 commit into
masterfrom
fix/385-blob-namespace

Conversation

@getappz

@getappz getappz commented Jul 27, 2026

Copy link
Copy Markdown
Owner

Closes agentflare item #385 (follow-up from #339 / PR #348).

The bug

~/.agentflare/store.db and ~/.agentflare/flare-docs.db live in the same directory, and blob_disk_path derived the blob directory from the db file's parent — so both resolved to the same ~/.agentflare/blobs/ while keeping separate store_blobs ref-count tables. Neither store can see the other's references.

Two conditions cause content loss:

  1. the same bytes stored in both (content-addressed by blake3, so identical bytes collide by design), and
  2. one store's ref_count hitting zero while the other's does not.

Condition 2 is what PR #348 changed: blob deletion used to happen only on explicit deletes, but Store::gc now runs on every flare-docs fetch and reclaims blobs routinely. Failure is silent — the row in the other store survives and blob_get returns None, no error anywhere.

The fix

Deliberately not the "one-line namespace the blobs dir" fix noted on #339 — rows store hashes, not paths, so changing the derivation alone orphans every blob already on disk.

  • write to <root>/blobs-<db file stem>/<xx>/<hash>
  • read the namespaced path first, fall back to the legacy shared <root>/blobs/
  • delete only ever unlinks from the namespaced directory

Worst case this leaks a legacy file nothing references any more; it can never remove content another store still depends on. No flag day, no migration that can half-complete. open_memory still uses store_blob_chunks and is unaffected.

Tests

Three added in blobs.rs:

  • two stores in one directory holding the same bytes, one reclaims → the other still reads back. Verified this fails on the old derivation (assertion left == right failed: the neighbouring store still references these bytes) and passes with the fix.
  • a blob seeded in the legacy shared dir still reads back.
  • blob_unref never unlinks from the legacy shared dir.

Verification

cargo fmt --all --check, cargo clippy -p agentflare-store -p flare-docs --all-targets --all-features -- -D warnings, and cargo test --workspace all clean.

Workspace-wide clippy also reports 9 pre-existing items_after_statements errors in agentflare-agent-registry; confirmed present on master with these changes stashed, so unrelated to this PR.

Not included

A one-off sweep to reclaim legacy blobs that neither store's store_blobs table references any more. Worth a follow-up item if the leak turns out to matter.

Summary by CodeRabbit

  • Bug Fixes

    • Blob files are now isolated per store, preventing one store from deleting blobs still needed by another.
    • Existing blobs in the legacy shared storage location remain readable.
    • Legacy shared blobs are protected from unintended cleanup.
  • Tests

    • Added coverage for shared-directory stores, blob reclamation, and legacy blob compatibility.

…our's content

store.db and flare-docs.db share ~/.agentflare/ and so derived the same
blobs/ directory while keeping separate store_blobs ref-count tables.
Identical bytes in both stores collide by content address, and neither
side can see the other's references, so a reclaim in one silently
stranded the other's content. PR #348 made this routine by running GC on
every docs fetch.

Writes now go to <root>/blobs-<db stem>/, reads fall back to the legacy
shared <root>/blobs/, and deletes only ever unlink from the store's own
directory. Worst case leaks a legacy file nothing references; it can
never remove content another store still needs. No migration, so blobs
already on disk keep reading back.
@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 435032b7-eb2d-4a43-bb21-241b08611074

📥 Commits

Reviewing files that changed from the base of the PR and between f5b2275 and cee18ee.

📒 Files selected for processing (3)
  • crates/agentflare-store/src/blobs.rs
  • crates/agentflare-store/src/lib.rs
  • crates/flare-docs/src/store.rs

📝 Walkthrough

Walkthrough

Blob disk storage is now namespaced per store, with legacy shared-directory fallback reads. Reclamation only removes files from the owning store directory, and tests cover gzip storage, cross-store references, legacy blobs, and updated path expectations.

Changes

Blob storage isolation

Layer / File(s) Summary
Store blob directory initialization
crates/agentflare-store/src/lib.rs
Store tracks a per-database blob_dir; memory stores initialize it with :memory:.
Namespaced blob IO and reclamation
crates/agentflare-store/src/blobs.rs
Blob writes and reads use the namespaced directory, reads fall back to <root>/blobs, and reclamation deletes only from the store-specific directory.
Blob path and compatibility validation
crates/agentflare-store/src/blobs.rs, crates/flare-docs/src/store.rs
Tests and reconciliation expectations use namespaced paths and cover gzip storage, cross-store reclamation, and legacy shared-directory blobs.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: per-database blob directories to prevent GC from deleting another store’s blobs.
Description check ✅ Passed The description covers the bug, fix, tests, verification, and reviewer notes, though it does not follow the exact template headings.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/385-blob-namespace

Comment @coderabbitai help to get the list of available commands.

@getappz
getappz merged commit 527a4c8 into master Jul 27, 2026
17 checks passed
@getappz
getappz deleted the fix/385-blob-namespace branch July 27, 2026 10:19
getappz added a commit that referenced this pull request Jul 28, 2026
* feat(store): sweep to reclaim orphaned legacy shared blobs

Blob directories are per-database since #351, and deletes deliberately
never touch the legacy shared <root>/blobs -- reads still fall back to
it, and only a check against every database in the directory can prove a
file there is dead. So nothing reclaims it in the normal course of
things.

sweep_legacy_blobs collects the referenced hashes from every .db in the
directory, then removes the legacy files none of them names. It aborts
rather than deleting if a database cannot be read, and refuses outright
when there is no database to consult -- an incomplete reference set
makes live content look like garbage. A .db without a store_blobs table
is skipped as unrelated, checked via sqlite_master so a genuinely broken
database still surfaces.

Exposed as 'agentflare docs sweep-legacy-blobs [--dry-run]'. On this
install the dry run reports 107 of 372 legacy files unreferenced.

* fix(store): make the legacy sweep's no-database refusal actually reachable

The guard counted any .db file and the CLI opened DocsStore before
dispatching, so a home with legacy blobs but no store left gained a fresh
empty database and swept every blob. Consult first, refuse on an empty
store set, propagate read_dir entry errors, and dispatch the sweep before
the store is opened.
getappz added a commit that referenced this pull request Aug 11, 2026
…ge collisions (#456)

* git-shim/worktree residual polish: teardown messaging, branch-create accuracy, stranded-checkout recovery

- worktree deny message now distinguishes provisioning (claim) from
  teardown (check_merge/release/audit --prune) so an agent denied
  mid-teardown gets the exact cleanup action (vent #350)
- is_branch_create() classifies checkout -b/-B and switch -c/-C;
  canonical-checkout deny says 'create a new branch' instead of the
  misleading 'would detach HEAD' (vent #395)
- audit_orphans flags worktrees stranded on the default branch (intact
  gitdir) so worktree audit --prune clears the gh pr merge --delete-
  branch / post-merge-sync collision root cause (vents #351/#394/#423)
- AGENTFLARE_GIT_ALLOW_CANONICAL_MUTATE now also lifts the default-branch
  checkout/switch deny in the canonical checkout, giving stranded
  checkouts a sanctioned way back (vent #386 residual)
- allowlist classify.rs (already 1604L on master) in the LOC gate

Agentflare-Agent: 1
Agentflare-Branch: task/441
Agentflare-Item: 441

* fix(git-shim): close branch-create detection gaps, protect dirty stranded worktrees

CodeRabbit review on PR #456:
- is_branch_create only matched exact -b/-B/-c/-C, missing --orphan
  (checkout+switch), switch's --create/--force-create long forms, and
  attached short-option spellings (-bname). Those forms slipped past
  both would_detach_head and deny_canonical_detach_reason undetected,
  letting an agent create a branch in the canonical checkout via e.g.
  'git switch --orphan x' or 'git checkout -bx' with no deny at all.
- audit_orphans' new on-default-branch detection could hand a dirty,
  uncommitted stranded worktree to gc_orphans for deletion; the only
  other gc_orphans caller (cleanup_item_worktree) already refuses on
  a dirty status check first. Apply the same guard here.

Agentflare-Agent: claude-code_2-1-227_agent
Agentflare-Branch: task/441
Agentflare-Item: 441
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant