feat: checksum FFI binding + benchmark (cachekit-core#13 Phase 2) - #212
Conversation
0.3.0 ships the standalone checksum/verify_checksum primitive (cachekit-core#13) that the PyO3 bindings mirror in this PR. Verified live on crates.io via cargo add --dry-run before pinning. Co-authored-by: multica-agent <github@multica.ai>
Free functions registered unconditionally in the pymodule (usable with the checksum feature alone — must not vanish when encryption is off). verify_checksum rejects non-8-byte expected with ValueError instead of panicking or returning a wrong verdict. Docstrings carry the non-cryptographic warning (corruption detection, not tamper-resistance). Tests byte-verify the FFI against the protocol KAT vectors pinned in cachekit-core src/checksum.rs AND against the pure-Python xxhash package, proving three independent implementations agree on the wire bytes. Also corrects the module __description__ strings that falsely claimed Blake3 checksums (the checksum has been xxHash3-64 since 0.1.0; docs drift tracked in #168). Co-authored-by: multica-agent <github@multica.ai>
…-core#13) Pinned contract: 64B/1KB/64KB/1MB, >=30 rounds, per-size groups. Deliverable artifact, not a gate — results recorded in the module docstring. Verdict: performance-neutral everywhere (worst gap ~65ns/call at 1KB compute where py-xxhash's C mid-size path beats xxhash-rust; FFI wins small-payload verify 2.5x). Serializer migration to the FFI should be decided on dependency hygiene, not speed. Co-authored-by: multica-agent <github@multica.ai>
…hecksum API CI: fail if rust/Cargo.toml carries a path-dep for cachekit-core (a leftover path would make maturin build public wheels from the local workspace — release-please rewrites versions, not path->version), and run clippy --locked so a stale Cargo.lock fails instead of silently re-resolving. Docs: document checksum/verify_checksum with an executable example (markdown-docs) and the non-cryptographic warning. Corrects this page's false Blake3 claims (xxHash3-64 since core 0.1.0); the repo-wide docs sweep remains #168. Co-authored-by: multica-agent <github@multica.ai>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThe PR upgrades ChangesChecksum FFI and integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PythonCaller
participant checksum_py
participant cachekit_core
PythonCaller->>checksum_py: checksum(data)
checksum_py->>cachekit_core: compute xxHash3-64
cachekit_core-->>checksum_py: 8-byte digest
checksum_py-->>PythonCaller: bytes digest
PythonCaller->>checksum_py: verify_checksum(data, expected)
checksum_py->>cachekit_core: verify checksum
cachekit_core-->>checksum_py: boolean result
checksum_py-->>PythonCaller: verification result
Possibly related issues
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/features/rust-serialization.md`:
- Line 41: Update the pipeline diagram in rust-serialization.md to replace
“Blake3 integrity hash” with “xxHash3-64” while preserving the existing
corruption-detection description and diagram formatting, so it matches the
checksum name in the comparison table.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 2c1d1a9c-2352-4352-9359-5ea81a9434e5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.github/workflows/ci.ymldocs/features/rust-serialization.mdrust/Cargo.tomlrust/src/lib.rsrust/src/python_bindings.rstests/benchmarks/benchmark_checksum_ffi.pytests/unit/test_checksum_ffi.py
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Apply the expert-panel review findings on the checksum FFI (LAB-132). - checksum/verify_checksum took &[u8], which in PyO3 0.29 accepts bytes only. The Arrow serializer they target hashes a memoryview (write arrow_serializer.py:245, verify body = mv[8:] :283), so the deferred serializer migration would TypeError in prod while the bytes-only test stayed green. Switch both to PyBuffer<u8> (bytes/bytearray/memoryview/Arrow buffers); wire bytes are unchanged. - CI pin guard grepped only the inline dep form. Assert the positive invariant on Cargo.lock instead: cachekit-core must resolve to the crates.io registry. A path or [patch.crates-io] redirect has no source line, so one check covers every Cargo.toml form and the workspace root. - Extend the xxhash byte-compat test to the 200 B mid-size and >64 KB accumulator-merge paths (previously unchecked above ~10 KB), and add memoryview/bytearray coverage including the Arrow mv[8:] verify shape. - docs: fix the leftover "Blake3" in the pipeline diagram; note the standalone checksum API ships in v0.12.0. Co-authored-by: multica-agent <github@multica.ai>
a260705 to
d699dd8
Compare
Closes the cachekit-py half of cachekit-io/cachekit-core#13 (Phase 2 of the checksum-only API plan; Phase 1 shipped in cachekit-core 0.3.0 via cachekit-io/cachekit-core#50).
What
cachekit-core0.2.0 → 0.3.0 — verified live on crates.io (cargo add --dry-run) before flipping the pin;Cargo.lockupdated,cargo build --lockedresolves from the registry.checksum/verify_checksumvia PyO3 — free functions registered unconditionally in the_rust_serializerpymodule (they must not vanish when the encryption feature is off).verify_checksumrejects a non-8-byte expected value withValueErrorinstead of panicking or returning a wrong verdict. Docstrings carry the non-cryptographic warning.src/checksum.rs(checksum(b"cachekit-kat"),checksum(b"")) and equalsxxhash.xxh3_64_digest— three independent implementations agreeing on the exact wire bytes. No wire format change (the primitive produces the same bytes already embedded in every StorageEnvelope).path =dep for cachekit-core (would let maturin build public wheels from the local workspace);clippy --lockedso a stale lockfile fails loudly.Verification
pytest tests/unit→ 1620 passed;pytest tests/critical→ 255 passed (suites run separately, as CI does)ruff format --check ./ruff check ./basedpyright --level error→ cleancargo fmt --check/cargo clippy --locked -- -D warnings→ cleanpytest --markdown-docs docs/features/rust-serialization.md→ the new doc example executesOut of scope (per plan)
Migrating the Arrow/orjson envelope internals from the
xxhashpackage to the FFI, and dropping theblake3dependency (still used byhash_utils.pyfor cache keys — a separate, security-relevant concern).Summary by CodeRabbit
--lockedclippy checks.