Skip to content

fix(cli): keep shared dotrain meta until the last uri drops it - #256

Open
thedavidmeister wants to merge 1 commit into
mainfrom
2026-08-25-issue-172
Open

fix(cli): keep shared dotrain meta until the last uri drops it#256
thedavidmeister wants to merge 1 commit into
mainfrom
2026-08-25-issue-172

Conversation

@thedavidmeister

Copy link
Copy Markdown
Contributor

Closes #172

The defect

dotrain_cache maps uri -> hash and the hash is content-determined (keccak of the cbor encoded DotrainV1 item), so two uris holding identical text necessarily share one cache entry. Both removal paths dropped that entry without checking whether another uri still pointed at it:

  • delete_dotrain(uri, keep_meta = false) — documented as deleting a record, one uri.
  • set_dotrain(text, uri, keep_old = false) — the different-hash branch removing old_hash.

The result was a dangling uri: get_dotrain_hash still returned the hash, get_dotrain_meta returned None.

The fix

One private remove_unreferenced_meta guard, called from both sites once dotrain_cache no longer maps the departing uri to the hash. The entry is removed only when no remaining uri maps to it, so the last uri off a hash still takes its meta with it and the single-uri behaviour both flags already had is unchanged.

QA

  • Discriminating tests: meta::tests::test_store_dotrain_shared_meta_survives_delete, meta::tests::test_store_set_dotrain_keeps_shared_old_meta — each fails on base (verified by reverting remove_unreferenced_meta to the base body self.cache.remove(hash); and re-running: both report assertion failed: store.get_dotrain_meta("b.rain").is_some(), 14 passed / 2 failed; restoring the guard returns 16 passed / 0 failed).
  • Mutations applied: two, both on crates/cli/src/meta/mod.rs remove_unreferenced_meta, each run over the whole meta::tests::test_store set (16 cases).
    1. Guard dropped, back to the base unconditional self.cache.remove(hash); → 14 passed / 2 failed, killed by test_store_dotrain_shared_meta_survives_delete and test_store_set_dotrain_keeps_shared_old_meta (assertion failed: store.get_dotrain_meta("b.rain").is_some() in each). The pre-existing test_store_delete_dotrain_keep_meta and test_store_set_dotrain_branches survive it — that is the gap this PR closes.
    2. Body made a no-op, never remove → 12 passed / 4 failed, killed by the pre-existing test_store_delete_dotrain_keep_meta and test_store_set_dotrain_branches AND by the second half of each new test (assertion failed: store.get_meta(&hash).is_none() / store.get_meta(&shared).is_none()), so the fix cannot degenerate into never freeing the cache.
  • Oracle: the doc contract quoted in the issue, not the implementation — Store "just maps the hash of the content to the given uri" (many-to-one, so identical text under two uris is one cache entry), delete_dotrain "deletes a dotrain record given a uri" (scoped to the one record), and get_dotrain_meta "get the corresponding meta bytes of the given dotrain uri if it exists". Expected values are the hashes set_dotrain returns for each uri, compared for equality rather than recomputed from the code under test.
  • Category check: issue asks for the dangling-uri state on two units — delete_dotrain's keep_meta = false branch and set_dotrain's keep_old = false different-hash branch; both are covered, one test each. The issue's triage framing left the contract open between reference-checking and documenting the flags as unsafe; this takes the reference-checking option, which is the one that keeps the three doc statements above true, and updates both doc comments to state the resulting contract.

Tests

Two new cases in crates/cli/src/meta/mod.rs; the existing test_store_delete_dotrain_keep_meta and test_store_set_dotrain_branches are unchanged and still pass.

cargo fmt --all --check, cargo clippy -p rain-metadata --lib --all-features -- -D warnings and the 16 meta::tests::test_store cases are green locally. The full suite was not run locally by instruction (shared machine).

🤖 Generated with Claude Code

dotrain_cache is many-to-one, so delete_dotrain(keep_meta = false) and
set_dotrain(keep_old = false) were removing cache entries that other uris
still mapped to, leaving those uris resolvable by hash but not by meta.

Closes #172

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 25, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 21 minutes.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 41e68d9a-0c19-4295-9b14-31d95e055d0f

📥 Commits

Reviewing files that changed from the base of the PR and between 45ca96c and 44617c6.

📒 Files selected for processing (1)
  • crates/cli/src/meta/mod.rs

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

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.

delete_dotrain / set_dotrain(keep_old=false) remove shared meta bytes still referenced by other dotrain uris

1 participant