fix(cli): keep shared dotrain meta until the last uri drops it - #256
Open
thedavidmeister wants to merge 1 commit into
Open
fix(cli): keep shared dotrain meta until the last uri drops it#256thedavidmeister wants to merge 1 commit into
thedavidmeister wants to merge 1 commit into
Conversation
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>
|
Warning Review limit reachedNext included review available in 21 minutes. View limit detailsLimit 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. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
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. Comment |
This was referenced Aug 25, 2026
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.
Closes #172
The defect
dotrain_cachemaps uri -> hash and the hash is content-determined (keccak of the cbor encodedDotrainV1item), so two uris holding identical text necessarily share onecacheentry. 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 removingold_hash.The result was a dangling uri:
get_dotrain_hashstill returned the hash,get_dotrain_metareturnedNone.The fix
One private
remove_unreferenced_metaguard, called from both sites oncedotrain_cacheno 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
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 revertingremove_unreferenced_metato the base bodyself.cache.remove(hash);and re-running: both reportassertion failed: store.get_dotrain_meta("b.rain").is_some(), 14 passed / 2 failed; restoring the guard returns 16 passed / 0 failed).crates/cli/src/meta/mod.rsremove_unreferenced_meta, each run over the wholemeta::tests::test_storeset (16 cases).self.cache.remove(hash);→ 14 passed / 2 failed, killed bytest_store_dotrain_shared_meta_survives_deleteandtest_store_set_dotrain_keeps_shared_old_meta(assertion failed: store.get_dotrain_meta("b.rain").is_some()in each). The pre-existingtest_store_delete_dotrain_keep_metaandtest_store_set_dotrain_branchessurvive it — that is the gap this PR closes.test_store_delete_dotrain_keep_metaandtest_store_set_dotrain_branchesAND 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.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), andget_dotrain_meta"get the corresponding meta bytes of the given dotrain uri if it exists". Expected values are the hashesset_dotrainreturns for each uri, compared for equality rather than recomputed from the code under test.delete_dotrain'skeep_meta = falsebranch andset_dotrain'skeep_old = falsedifferent-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 existingtest_store_delete_dotrain_keep_metaandtest_store_set_dotrain_branchesare unchanged and still pass.cargo fmt --all --check,cargo clippy -p rain-metadata --lib --all-features -- -D warningsand the 16meta::tests::test_storecases are green locally. The full suite was not run locally by instruction (shared machine).🤖 Generated with Claude Code