Repository navigation
fix(content-store): confine hash paths to the store, write blobs atomically, repair damaged ones - #163
Conversation
…ically, repair damaged ones Three defects in reversible-core's ContentStore, each reproduced by a test that failed before this change (see the proof-statement audit). 1. A hash string could name a path outside the store. `content_path` split the text after two characters and joined it onto the root. `Path::join` replaces the root when the right side is absolute, so a ContentHash such as "sha256:ab/<absolute path>" resolved to that path, and `delete` (and `obliterate`, which calls `content_path`) acted on it. ContentHash has a public field and derives Deserialize, so its text is untrusted. Whether stored metadata can supply such a hash to those callers was not shown. `content_path` now fails closed: anything other than "sha256:" plus 64 lowercase hex digits maps to a fixed path under the root that no blob occupies. The layout for well-formed hashes is unchanged (pinned by a test). 2. A crash left a blob truncated at its final path for good. `store` wrote in place, and its deduplication check was `path.exists()`, so every later store of the same content was a no-op and every retrieve failed. `store` now writes a temporary file in the blob's directory, flushes it, and renames it into place, so the final path only ever holds a complete blob. 3. A blob already damaged (by an older crash or by bit rot) was never repaired. Deduplication now reads the existing blob back and trusts it only if its hash matches; otherwise it is replaced. `count()` ignores ".tmp-" files, which a crash can orphan. They are not collected here. Cost, measured on one VM in release mode, 200 distinct 4 KiB blobs: about 0.5-0.6 ms per blob (plain) and 0.8-1.3 ms (gzip), up from 0.07 ms and 0.14-0.20 ms, almost all of it the fsync. Storing content that is already stored now also reads the existing blob back: about +20 ms at 16 MiB and +90-130 ms at 64 MiB. TOPOLOGY.adoc and READINESS.adoc: 119 -> 123 tests (four added), as dashboard-check requires. Checked: cargo fmt --check; cargo clippy --workspace --all-targets -D warnings; cargo test --workspace --locked (all pass, 123 tests); dashboard-check reports no divergence other than the Idris2/Agda proof rows, whose binaries are not installed in this environment. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013CuBToxr5h9catptGiTsFX
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 45 minutes. View limit details
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
CI on 286f933: the 20 check runs that exist are green or skipped, but five workflows failed with zero jobs, so they produced no check run: Not caused by this PR. The same five fail the same way on What changed is outside the repo. On 10-22 the same Ruled out (by checking live remotes):
Not known: the failure text. The API gives no reason for a zero-job run, and the run page is not reachable from my environment. Opening one of the runs above will show the banner and settle whether this is another GitHub-side validation change. I have not changed anything in response. Squabble ( Generated by Claude Code |
|
Add Carrot credits or activate Agent usage billing to use Autofix |
main gained #163 (content-store fixes, +4 tests), which edits the same test-count lines as this branch. The merged total is 123 + 9 = 132, checked against `cargo test --workspace --locked -- --list`. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kjdf4Mf4Z41bNQh2TrsEp9
The remote merge of main (42cacae) kept this branch's 128 tests, but main's #163 adds 4, so the declared total is 132 (checked against `cargo test --workspace --locked -- --list`). dashboard-check would have failed on the stale 128. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kjdf4Mf4Z41bNQh2TrsEp9
Summary
First step out of a proof-statement audit of
PROOF-NEEDS.adoc. These are bugs inreversible-core'sContentStorethat make the "content store never returns wrong content" obligation false as stated. They are fixed before anything is proved about it.Each defect was reproduced by a test that failed before this change.
1. A hash string could name a path outside the store
content_pathsplit the hash text after two characters and joined it onto the store root.Path::joinreplaces the root when the right-hand side is absolute, so a hash such assha256:ab/<absolute path>resolved to that path.deleteacts on that path, and so doesobliterate, which callscontent_pathand then overwrites and removes the file.ContentHashhas a public field and derivesDeserialize, so its text is untrusted.Not shown: that stored metadata can actually deliver such a hash to those callers. I only showed that the store API accepted it. Please treat this as a possible security issue when deciding how to talk about it.
Fix:
content_pathfails closed. Anything other thansha256:plus 64 lowercase hex digits maps to a fixed path under the root that no blob occupies. The layout for well-formed hashes is unchanged, pinned bywell_formed_hash_keeps_the_existing_layout(which passes on the old code too).2. A crash could leave a blob truncated for good
storewrote in place, and its deduplication check waspath.exists(). After a crash, every laterstoreof that content was a no-op and everyretrievefailed.storenow writes a temporary file in the blob's directory, flushes it, and renames it into place, so the final path only ever holds a complete blob.3. A damaged blob was never repaired
Deduplication now reads the existing blob back and trusts it only if its hash matches; otherwise it is replaced. This also heals blobs damaged by an older crash or by bit rot.
count()ignores.tmp-files, which a crash can orphan. Nothing collects them yet.Cost (your call whether it is acceptable)
Measured on one VM, release mode, 200 distinct 4 KiB blobs:
Almost all of that is the
fsync, so it depends on the disk and will be worse on spinning or network storage. Storing content that is already stored now also reads the existing blob back: about +20 ms at 16 MiB and +90–130 ms at 64 MiB. Thefsyncis what makes the write durable across power loss. If you would rather trade that for speed, the atomic rename alone still covers a process crash.Verification
content_store.rs:malformed_hash_never_leaves_the_store_root(11 malformed hashes, plain and gzip),well_formed_hash_keeps_the_existing_layout,store_repairs_a_blob_truncated_by_a_crash,store_leaves_no_temp_files_and_count_ignores_strays. Run before the fix: 3 failed, 1 passed (the layout pin).cargo fmt --all -- --check,cargo clippy --workspace --all-targets -- -D warningsandcargo test --workspace --lockedall pass (123 tests).TOPOLOGY.adocandREADINESS.adoc: 119 → 123 tests, asdashboard-checkrequires.dashboard-checkreports no divergence except the Idris2 and Agda proof rows. I could not run those here because the binaries are not installed in my sandbox; CI has them.Not in this PR
ContentHashon deserialization.content_pathis now safe regardless, but rejecting bad metadata earlier would be cleaner. Follow-up.ObliterationProofwording, and the transaction-log atomic write (already tracked as T-5 in proofs: transaction atomicity in the Agda kernel (T-1..T-3), and the gaps it exposes (T-4..T-7) #161). Each is its own change.PROOF-NEEDS.adocobligations. The audit that motivated this is a separate document.Generated by Claude Code