Replace bincode with rkyv 0.8.15#117
Conversation
Signed-off-by: Quanyi Ma <eli@patch.sh>
There was a problem hiding this comment.
Pull request overview
This PR migrates the project’s Rust serialization dependency from bincode to rkyv (0.8.15), updating both the Cargo dependency graph and Buck third-party targets, and switching the pack cache’s on-disk encoding accordingly.
Changes:
- Replace
bincodewithrkyvacross the workspace (Cargo + Buck third-party crates). - Update pack cache object file persistence to use
rkyvencode/decode, and addrkyvderives to types used in cached structures. - Regenerate/update
buckal.snapand dependency lockfiles to reflect the new crate set.
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
Cargo.toml |
Removes bincode, adds rkyv dependency. |
Cargo.lock |
Updates resolved dependencies: removes bincode*, adds rkyv 0.8.15 and related crates. |
BUCK |
Replaces root bincode Buck dep with rkyv. |
buckal.snap |
Updates buckal fingerprints and crate entries to reflect the dependency swap. |
src/internal/pack/cache_object.rs |
Switches disk cache serialization from bincode to rkyv; adds an on-disk wrapper type for CacheObject. |
src/internal/object/types.rs |
Adds rkyv derives to ObjectType for compatibility with new caching format. |
src/internal/object/tree.rs |
Removes bincode Encode/Decode derives; adds rkyv derives for cache-compatibility. |
src/internal/object/signature.rs |
Removes bincode Encode/Decode derives; adds rkyv derives. |
src/internal/object/note.rs |
Removes bincode Encode/Decode derives; adds rkyv derives. |
src/internal/object/commit.rs |
Removes bincode Encode/Decode derives; adds rkyv derives. |
src/hash.rs |
Removes bincode Encode/Decode derives; adds rkyv derives to hash types used in cached structures. |
third-party/rust/crates/rkyv/0.8.15/BUCK |
Adds Buck target for rkyv 0.8.15. |
third-party/rust/crates/rkyv_derive/0.8.15/BUCK |
Adds Buck target for rkyv_derive 0.8.15. |
third-party/rust/crates/bytecheck*/0.8.2/BUCK |
Adds Buck targets for rkyv validation deps (bytecheck, bytecheck_derive). |
third-party/rust/crates/{ptr_meta*,munge*,rancor,rend}/.../BUCK |
Adds Buck targets for additional rkyv 0.8.x dependency chain. |
third-party/rust/crates/{bincode*,virtue,unty}/.../BUCK |
Removes Buck definitions for bincode and its helper crates. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e57c69e78
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Quanyi Ma <eli@patch.sh>
Signed-off-by: Quanyi Ma <eli@patch.sh>
Signed-off-by: Quanyi Ma <eli@patch.sh>
There was a problem hiding this comment.
Pull request overview
This PR migrates the project’s serialization dependency from bincode to rkyv 0.8.15, updating the pack cache’s on-disk persistence and aligning build metadata (Cargo + Buck) with the new dependency graph.
Changes:
- Replace
bincodeusage in the pack cache on-disk serialization withrkyvand introduce a versioned cache layout directory (rkyv-v1). - Add
rkyvderives to core object/hash types needed by the cache’s archived representations. - Update Cargo/Buck dependency manifests and third-party Buck targets to remove
bincode(+ transitiveunty/virtue) and addrkyv(+ bytecheck/rancor/ptr_meta/etc).
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| third-party/rust/crates/virtue/0.0.18/BUCK | Removes Buck target definition for virtue (previously needed by bincode_derive). |
| third-party/rust/crates/unty/0.0.4/BUCK | Removes Buck target definition for unty (previously needed by bincode). |
| third-party/rust/crates/bincode_derive/2.0.1/BUCK | Removes Buck target definition for bincode_derive. |
| third-party/rust/crates/bincode/2.0.1/BUCK | Removes Buck target definition for bincode. |
| third-party/rust/crates/rkyv_derive/0.8.15/BUCK | Adds Buck target for rkyv_derive proc-macro. |
| third-party/rust/crates/rkyv/0.8.15/BUCK | Adds Buck target for rkyv with required features/deps. |
| third-party/rust/crates/rend/0.5.3/BUCK | Adds Buck target for rend (rkyv dependency). |
| third-party/rust/crates/rancor/0.1.1/BUCK | Adds Buck target for rancor (rkyv error/strategy dependency). |
| third-party/rust/crates/ptr_meta_derive/0.3.1/BUCK | Adds Buck target for ptr_meta_derive. |
| third-party/rust/crates/ptr_meta/0.3.1/BUCK | Adds Buck target for ptr_meta. |
| third-party/rust/crates/munge_macro/0.4.7/BUCK | Adds Buck target for munge_macro. |
| third-party/rust/crates/munge/0.4.7/BUCK | Adds Buck target for munge. |
| third-party/rust/crates/bytecheck_derive/0.8.2/BUCK | Adds Buck target for bytecheck_derive. |
| third-party/rust/crates/bytecheck/0.8.2/BUCK | Adds Buck target for bytecheck. |
| src/internal/pack/cache_object.rs | Switches file persistence from bincode to rkyv, adds on-disk structs/refs for CacheObject. |
| src/internal/pack/cache.rs | Versions cache on-disk layout by inserting rkyv-v1 into the temp path. |
| src/internal/object/types.rs | Adds rkyv derives to ObjectType. |
| src/internal/object/tree.rs | Removes bincode derives and adds rkyv derives to tree types. |
| src/internal/object/signature.rs | Removes bincode derives and adds rkyv derives to signature types. |
| src/internal/object/note.rs | Removes bincode derives and adds rkyv derives to Note. |
| src/internal/object/commit.rs | Removes bincode derives and adds rkyv derives to Commit. |
| src/hash.rs | Removes bincode derives and adds rkyv derives to hash types. |
| buckal.snap | Updates buckal fingerprints and crate snapshot entries for the dependency swap. |
| Cargo.toml | Replaces bincode dependency with rkyv = 0.8.15 (+ bytecheck). |
| Cargo.lock | Removes bincode packages and adds rkyv 0.8.15 and its dependency set. |
| BUCK | Updates root Buck deps to remove bincode and add rkyv. |
Signed-off-by: Quanyi Ma <eli@patch.sh>
genedna
left a comment
There was a problem hiding this comment.
Code Review: Replace bincode with rkyv 0.8.15
Overall Assessment
This is a well-motivated dependency migration from bincode to rkyv 0.8.15 for the disk-backed cache serialization path. rkyv's zero-copy deserialization model is a strong fit for this use case — pack cache objects are written frequently on LRU eviction and read back during delta reconstruction, so eliminating deserialization allocations in the hot path is a meaningful performance win. The PR also takes the opportunity to harden the atomic file write logic, which is a welcome improvement.
Overall: Approve with minor suggestions below.
1. Code Quality
Strengths:
- Clean separation of concerns with
CacheObjectOnDisk(owned, for deserialization) andCacheObjectOnDiskRef<'a>(borrowed, for serialization). This avoids cloning thedata_decompressed: Vec<u8>during writes viaCow::Borrowed— good zero-copy discipline. - The
FileLoadStoretrait is cleanly refactored: the blanket impl now targets rkyv's trait bounds instead of serde's, and the manual impl forCacheObjectcorrectly bridges through the on-disk proxy types. CACHE_LAYOUT_VERSIONin the cache path (rkyv-v1) is a simple and effective way to avoid reading stale bincode-format cache files after the migration. Nice forward-thinking.
Suggestions:
-
Unbounded retry loop in
write_bytes_atomically(cache_object.rs):
Theloopincrementsattemptviasaturating_add(1)but has no upper bound. If a pathological scenario causes temp files to accumulate (e.g., a zombie process holding.tempfiles), this will spin indefinitely. Consider adding a max attempt limit:const MAX_TEMP_ATTEMPTS: u32 = 64; // ... Err(err) if err.kind() == io::ErrorKind::AlreadyExists => { attempt = attempt.saturating_add(1); if attempt >= MAX_TEMP_ATTEMPTS { return Err(io::Error::new( io::ErrorKind::AlreadyExists, "exhausted temp file attempts", )); } }
-
Residual
serde::{Serialize, Deserialize}onCacheObject: The struct still derivesserde::Serializeandserde::Deserialize, butCacheObjectnow has a manualFileLoadStoreimpl that uses rkyv through the proxy types. If serde onCacheObjectis no longer needed elsewhere (e.g., not serialized via JSON/other formats), these derives are dead code and could be removed to reduce confusion about which serialization path is active. -
ArcWrapperBoundstrait alias change: The bound was updated fromSerialize + DeserializetoFileLoadStore, which is correct and aligns with the new architecture. Good.
2. Security
Strengths:
bytecheckfeature is enabled — this is critical. rkyv without validation can cause undefined behavior from malformed input. The use ofrkyv::from_bytes::<T, RkyvError>()which goes throughHighValidator+CheckBytesensures archived data is validated before access.- The
CheckBytesbound on theFileLoadStoreblanket impl (T::Archived: for<'a> CheckBytes<HighValidator<'a, RkyvError>>) enforces this at the type level. Well done. create_new(true)on temp file creation prevents symlink attacks (no following existing symlinks).
Notes:
- Cache files are only read from paths the application itself generates (under
tmp_path), so the attack surface for malicious archived data is limited to local filesystem access. Thebytecheckvalidation provides adequate defense-in-depth here.
3. Performance
Strengths:
- rkyv's zero-copy deserialization means
f_loadonly needs to validate and reinterpret the bytes, avoiding the allocation + field-by-field copy that bincode required. For large blob cache objects (multi-MB), this is a significant improvement. CacheObjectOnDiskRefserializes by borrowingdata_decompressedasCow::Borrowed(&[u8]), avoiding a full copy duringf_save. This is the right pattern for rkyv.- The
CACHE_LAYOUT_VERSIONsubdirectory ensures no fallback parsing of old format — clean cutover, no performance penalty from format detection.
Minor observations:
-
No
fsyncbefore rename:write_bytes_atomicallydoesfile.write_all(&data)→drop(file)→fs::rename()without callingfile.sync_data(). For a cache (which can tolerate loss on crash — it'll just be a cache miss), this is acceptable and avoids the fsync latency penalty. But if durability requirements ever change, this should be revisited. -
rkyv::to_bytesallocates anAlignedVecon everyf_savecall. This is inherent to rkyv's serialization model and is comparable to what bincode was doing (allocating aVec<u8>). For the write path this is acceptable since the bottleneck is disk I/O.
4. Testing
Strengths:
test_cache_object_serializeis updated to use the new rkyv proxy types (CacheObjectOnDiskRef/CacheObjectOnDisk), and now also assertsb.mem_recorder.is_none()— verifying the non-serializable field is correctly excluded. Good.- New test
test_write_bytes_atomically_retries_when_temp_existscovers the stale-temp-file retry path.
Suggestions:
-
Missing test: roundtrip through
FileLoadStoretrait (file I/O path): The existingtest_cache_object_serializetests in-memory serialization, but there's no test that exercisesCacheObject::f_save()→CacheObject::f_load()roundtrip through actual files. This would catch issues like path construction bugs or file permission problems. Consider adding:#[test] fn test_cache_object_file_roundtrip() { let dir = PathBuf::from(".cache_temp/test_file_roundtrip"); fs::create_dir_all(&dir).unwrap(); let path = dir.join("obj"); let a = make_obj(1024); a.f_save(&path).unwrap(); let b = CacheObject::f_load(&path).unwrap(); assert_eq!(a.info, b.info); assert_eq!(a.data_decompressed, b.data_decompressed); fs::remove_dir_all(&dir).unwrap(); }
-
Test uses relative paths:
test_write_bytes_atomically_retries_when_temp_existsuses.cache_temp/...which depends on the working directory and could leave artifacts. Consider usingstd::env::temp_dir()or the existing test pattern for consistency. -
Missing edge case: No test for
write_bytes_atomicallywhen the target file already exists at entry (the early-return path on line 1 of the function). Simple to add.
5. Documentation
- The code changes are self-documenting given the mechanical nature of the derive macro swap.
- The
CACHE_LAYOUT_VERSIONconstant is clear in purpose but a brief doc comment explaining why it exists (format incompatibility with bincode) would help future maintainers:/// Cache format version string appended to the disk path so that caches written /// with an incompatible serialization format (e.g. the previous bincode layout) /// are silently ignored rather than causing deserialization errors. const CACHE_LAYOUT_VERSION: &str = "rkyv-v1";
- No
docs/or README changes needed — this is an internal implementation detail.
Summary
| Area | Rating | Notes |
|---|---|---|
| Code Quality | Good | Clean proxy-type pattern; one unbounded loop concern |
| Security | Good | bytecheck validation enabled; create_new prevents symlink issues |
| Performance | Good | Zero-copy deser for reads; borrowed serialization for writes |
| Testing | Adequate | Core roundtrip tested; suggest adding file I/O roundtrip test |
| Documentation | Minor | Add doc comment on CACHE_LAYOUT_VERSION |
Nice work on the migration. The CacheObjectOnDiskRef borrowing pattern is particularly well-crafted.
Signed-off-by: Quanyi Ma <eli@patch.sh>
genedna
left a comment
There was a problem hiding this comment.
Re-Review: Replace bincode with rkyv 0.8.15 (Updated)
All issues from the previous review have been addressed in commit f8f77f9 ("Refine pack cache spill review fixes"). This PR is now in excellent shape.
Changes Since Last Review
The follow-up commit addressed every suggestion:
-
Unbounded retry loop — Fixed.
MAX_TEMP_ATTEMPTS = 64constant added, with a clear error message includingpath.display()for debuggability. -
Dead serde derives on
CacheObject— Fixed.CacheObjectnow derives only#[derive(Debug)], removing the unusedserde::Serializeandserde::Deserialize. Clean. -
CACHE_LAYOUT_VERSIONdoc comment — Added. Three-line doc comment explains the purpose clearly. -
File I/O roundtrip test — Added.
test_cache_object_file_roundtripexercisesf_save()→f_load()through actual files and verifiesmem_recorderisNoneafter deserialization. -
Early-return edge case test — Added.
test_write_bytes_atomically_returns_when_target_existsconfirms idempotent behavior when the target already exists. -
Tests now use
tempfile::tempdir()— Fixed. All new tests use proper temp directories instead of relative.cache_temp/paths. No more test artifacts in the working directory.
Final Assessment (All Areas)
| Area | Rating | Notes |
|---|---|---|
| Code Quality | Excellent | Clean proxy-type pattern; bounded retry loop; no dead derives |
| Security | Excellent | bytecheck validation enforced at type level; create_new(true) prevents symlink attacks |
| Performance | Excellent | Zero-copy deser on reads; Cow::Borrowed avoids cloning on writes |
| Testing | Excellent | In-memory roundtrip, file I/O roundtrip, atomic write retry, idempotent early-return — all covered |
| Documentation | Good | CACHE_LAYOUT_VERSION documented; code is self-documenting |
LGTM. The rkyv migration is well-executed, with proper validation safety, clean type separation for the non-serializable mem_recorder field, and comprehensive test coverage. The CacheObjectOnDiskRef borrowing pattern remains the highlight — zero-copy serialization without sacrificing the in-memory representation.
There was a problem hiding this comment.
Pull request overview
This PR migrates the crate’s internal serialization usage from bincode to rkyv (0.8.15), primarily impacting the pack cache’s on-disk format and the third-party Buck wiring needed to build the new dependency graph.
Changes:
- Replace
bincodeusage in the pack cache withrkyv, including new on-disk representations and additional roundtrip/atomic-write tests. - Version the on-disk cache directory layout (
CACHE_LAYOUT_VERSION = "rkyv-v1") to avoid deserializing incompatible caches. - Update Cargo + Buck dependencies by removing
bincode/unty/virtueand addingrkyv+ its supporting crates.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| third-party/rust/crates/virtue/0.0.18/BUCK | Removes Buck rule for virtue (no longer needed after dropping bincode_derive). |
| third-party/rust/crates/unty/0.0.4/BUCK | Removes Buck rule for unty (no longer needed after dropping bincode). |
| third-party/rust/crates/rkyv_derive/0.8.15/BUCK | Adds Buck rule for rkyv_derive proc-macro crate. |
| third-party/rust/crates/rkyv/0.8.15/BUCK | Adds Buck rule for rkyv with required deps/features. |
| third-party/rust/crates/rend/0.5.3/BUCK | Adds Buck rule for rend needed by rkyv 0.8.x. |
| third-party/rust/crates/rancor/0.1.1/BUCK | Adds Buck rule for rancor needed by rkyv 0.8.x. |
| third-party/rust/crates/ptr_meta_derive/0.3.1/BUCK | Adds Buck rule for ptr_meta_derive needed by ptr_meta 0.3.x. |
| third-party/rust/crates/ptr_meta/0.3.1/BUCK | Adds Buck rule for ptr_meta 0.3.x needed by rkyv 0.8.x. |
| third-party/rust/crates/munge_macro/0.4.7/BUCK | Adds Buck rule for munge_macro needed by munge. |
| third-party/rust/crates/munge/0.4.7/BUCK | Adds Buck rule for munge needed by rkyv 0.8.x. |
| third-party/rust/crates/bytecheck_derive/0.8.2/BUCK | Adds Buck rule for bytecheck_derive required by bytecheck 0.8.x. |
| third-party/rust/crates/bytecheck/0.8.2/BUCK | Adds Buck rule for bytecheck 0.8.x used by rkyv validation. |
| third-party/rust/crates/bincode_derive/2.0.1/BUCK | Removes Buck rule for bincode_derive. |
| third-party/rust/crates/bincode/2.0.1/BUCK | Removes Buck rule for bincode. |
| src/internal/pack/cache_object.rs | Replaces bincode-based file IO with rkyv serialization; adds atomic-write helper + tests; introduces on-disk structs to exclude mem_recorder. |
| src/internal/pack/cache.rs | Versions cache directory layout to avoid incompatible deserialization. |
| src/internal/object/types.rs | Adds rkyv derives to ObjectType. |
| src/internal/object/tree.rs | Removes bincode derives/imports; adds rkyv derives to tree types. |
| src/internal/object/signature.rs | Removes bincode derives/imports; adds rkyv derives to signature types. |
| src/internal/object/note.rs | Removes bincode derives/imports; adds rkyv derives. |
| src/internal/object/commit.rs | Removes bincode derives/imports; adds rkyv derives. |
| src/hash.rs | Removes bincode derives/imports; adds rkyv derives to hash types. |
| buckal.snap | Updates dependency fingerprints to reflect the new graph. |
| Cargo.toml | Swaps bincode dependency for rkyv = 0.8.15. |
| Cargo.lock | Removes bincode/unty/virtue; adds rkyv 0.8.15 and transitive deps. |
| BUCK | Swaps top-level Buck dependency from bincode to rkyv 0.8.15. |
|
@codex review |
Signed-off-by: Quanyi Ma <eli@patch.sh>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50a34c8987
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let data = rkyv::to_bytes::<RkyvError>(&CacheObjectOnDiskRef::from(self)) | ||
| .map_err(io::Error::other)?; | ||
| write_bytes_atomically(path, &data) |
There was a problem hiding this comment.
Skip rkyv encode when spill target already exists
CacheObject::f_save always runs rkyv::to_bytes before checking whether the destination file already exists, because the existence check now happens inside write_bytes_atomically. In this cache, objects loaded back from disk (get_fallback) can be evicted again with the same store_path, so repeated evictions will still re-serialize full payloads even though the write is skipped, causing avoidable CPU and peak-memory spikes for large objects and weakening the intended memory-pressure behavior during churn.
Useful? React with 👍 / 👎.
Signed-off-by: Quanyi Ma <eli@patch.sh>
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR migrates the project’s Rust serialization stack from bincode to rkyv (targeting rkyv 0.8.15), updating both the on-disk pack cache serialization and the third-party Buck crate definitions to match.
Changes:
- Replaced
bincodewithrkyvin Rust dependencies and Buck targets; removed now-unusedbincode/unty/virtuethird-party definitions. - Updated pack cache object persistence to use
rkyv, including a cache layout version segment to avoid deserializing old cache formats. - Added
rkyvderives to core object/hash types that were previously usingbincode’s encode/decode derives.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| third-party/rust/crates/virtue/0.0.18/BUCK | Removed unused third-party Buck target (was only needed for bincode derive stack). |
| third-party/rust/crates/unty/0.0.4/BUCK | Removed unused third-party Buck target (was a bincode dependency). |
| third-party/rust/crates/rkyv_derive/0.8.15/BUCK | Added Buck definition for rkyv_derive proc-macro crate. |
| third-party/rust/crates/rkyv/0.8.15/BUCK | Added Buck definition for rkyv 0.8.15 and its deps. |
| third-party/rust/crates/rend/0.5.3/BUCK | Added Buck definition for rend required by rkyv 0.8.x. |
| third-party/rust/crates/rancor/0.1.1/BUCK | Added Buck definition for rancor required by rkyv 0.8.x. |
| third-party/rust/crates/ptr_meta_derive/0.3.1/BUCK | Added Buck definition for ptr_meta_derive required by new ptr_meta. |
| third-party/rust/crates/ptr_meta/0.3.1/BUCK | Added Buck definition for ptr_meta 0.3.1 required by rkyv 0.8.x. |
| third-party/rust/crates/munge_macro/0.4.7/BUCK | Added Buck definition for munge_macro required by rkyv 0.8.x. |
| third-party/rust/crates/munge/0.4.7/BUCK | Added Buck definition for munge required by rkyv 0.8.x. |
| third-party/rust/crates/bytecheck_derive/0.8.2/BUCK | Added Buck definition for bytecheck_derive used by rkyv validation. |
| third-party/rust/crates/bytecheck/0.8.2/BUCK | Added Buck definition for bytecheck 0.8.2 used by rkyv validation. |
| third-party/rust/crates/bincode_derive/2.0.1/BUCK | Removed third-party Buck target for bincode_derive. |
| third-party/rust/crates/bincode/2.0.1/BUCK | Removed third-party Buck target for bincode. |
| src/internal/pack/cache_object.rs | Switched cache object disk persistence to rkyv; added atomic write helper and new tests. |
| src/internal/pack/cache.rs | Versioned cache directory layout to avoid reading incompatible cache formats. |
| src/internal/object/types.rs | Added rkyv derives to ObjectType. |
| src/internal/object/tree.rs | Replaced bincode encode/decode derives with rkyv derives for tree types. |
| src/internal/object/signature.rs | Replaced bincode encode/decode derives with rkyv derives for signature types. |
| src/internal/object/note.rs | Replaced bincode encode/decode derives with rkyv derives for note types. |
| src/internal/object/commit.rs | Replaced bincode encode/decode derives with rkyv derives for commit type. |
| src/hash.rs | Replaced bincode encode/decode derives with rkyv derives for hash types. |
| buckal.snap | Updated buckal fingerprint/snapshot to reflect dependency graph changes. |
| Cargo.toml | Replaced bincode dependency with rkyv 0.8.15. |
| Cargo.lock | Removed bincode packages; added rkyv 0.8.15 and new transitive deps. |
| BUCK | Swapped root crate dependency from bincode to rkyv. |
| Ok(obj) | ||
| } | ||
|
|
||
| fn f_save(&self, path: &Path) -> Result<(), io::Error> { |
| #[test] | ||
| fn test_write_bytes_atomically_ignores_stale_temp_file() { | ||
| let dir = tempdir().unwrap(); | ||
| let path = dir.path().join("object"); | ||
|
|
||
| fs::write(path.with_extension("temp"), b"stale").unwrap(); | ||
| write_bytes_atomically(&path, b"fresh").unwrap(); | ||
|
|
||
| assert_eq!(fs::read(&path).unwrap(), b"fresh"); | ||
| } |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Signed-off-by: Quanyi Ma <eli@patch.sh>
There was a problem hiding this comment.
Pull request overview
This PR migrates the repository’s internal serialization usage from bincode to rkyv (0.8.15), focusing on the pack cache’s on-disk persistence and updating third-party Buck targets accordingly.
Changes:
- Replaced
bincode-based disk serialization for pack cache objects withrkyvand added atomic file-write logic plus new unit tests. - Introduced a disk cache layout version segment (
rkyv-v1) in the cache path to avoid deserializing incompatible older cache files. - Updated dependency manifests and Buck third-party crate targets: removed
bincode(+ its proc-macro deps) and addedrkyv(+bytecheck,rend,rancor,ptr_meta,munge, etc.).
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| third-party/rust/crates/virtue/0.0.18/BUCK | Removed unused virtue Buck target (bincode proc-macro dependency). |
| third-party/rust/crates/unty/0.0.4/BUCK | Removed unused unty Buck target (bincode dependency). |
| third-party/rust/crates/bincode_derive/2.0.1/BUCK | Removed bincode_derive Buck target. |
| third-party/rust/crates/bincode/2.0.1/BUCK | Removed bincode Buck target. |
| third-party/rust/crates/rkyv_derive/0.8.15/BUCK | Added Buck target for rkyv_derive (proc-macro). |
| third-party/rust/crates/rkyv/0.8.15/BUCK | Added Buck target for rkyv 0.8.15. |
| third-party/rust/crates/rend/0.5.3/BUCK | Added Buck target required by rkyv 0.8.x. |
| third-party/rust/crates/rancor/0.1.1/BUCK | Added Buck target required by rkyv 0.8.x. |
| third-party/rust/crates/ptr_meta_derive/0.3.1/BUCK | Added Buck target required by rkyv 0.8.x. |
| third-party/rust/crates/ptr_meta/0.3.1/BUCK | Added Buck target required by rkyv 0.8.x. |
| third-party/rust/crates/munge_macro/0.4.7/BUCK | Added Buck target required by rkyv 0.8.x. |
| third-party/rust/crates/munge/0.4.7/BUCK | Added Buck target required by rkyv 0.8.x. |
| third-party/rust/crates/bytecheck_derive/0.8.2/BUCK | Added Buck target required for validation support in rkyv. |
| third-party/rust/crates/bytecheck/0.8.2/BUCK | Added Buck target required for validation support in rkyv. |
| src/internal/pack/cache_object.rs | Implemented rkyv-based (de)serialization for cache objects, added atomic persistence, updated bounds, and added tests. |
| src/internal/pack/cache.rs | Added cache layout versioning to disk path construction (tmp_path/rkyv-v1/...). |
| src/internal/object/types.rs | Added rkyv derives to ObjectType to support archiving where used. |
| src/internal/object/tree.rs | Removed bincode derives/imports and added rkyv derives to tree-related types. |
| src/internal/object/signature.rs | Removed bincode derives/imports and added rkyv derives to signature types. |
| src/internal/object/note.rs | Removed bincode derives/imports and added rkyv derives to note type. |
| src/internal/object/commit.rs | Removed bincode derives/imports and added rkyv derives to commit type. |
| src/hash.rs | Removed bincode derives/imports and added rkyv derives to hash types used by the cache metadata. |
| buckal.snap | Updated buckal snapshot/fingerprints to reflect dependency graph changes. |
| Cargo.toml | Swapped dependency from bincode to rkyv (0.8.15). |
| Cargo.lock | Updated lockfile for rkyv and its new dependency tree; removed bincode entries. |
| BUCK | Swapped top-level Buck dependency from bincode to rkyv. |
* Replace bincode with rkyv 0.8.15 Signed-off-by: Quanyi Ma <eli@patch.sh> * Declare rkyv bytecheck feature explicitly Signed-off-by: Quanyi Ma <eli@patch.sh> * Harden pack cache spill format handling Signed-off-by: Quanyi Ma <eli@patch.sh> * Fix rustfmt formatting in cache spill code Signed-off-by: Quanyi Ma <eli@patch.sh> * Harden atomic cache spill writes Signed-off-by: Quanyi Ma <eli@patch.sh> * Refine pack cache spill review fixes Signed-off-by: Quanyi Ma <eli@patch.sh> * Avoid clobbering existing pack cache files Signed-off-by: Quanyi Ma <eli@patch.sh> * Skip redundant pack cache re-encoding Signed-off-by: Quanyi Ma <eli@patch.sh> * Align pack cache spill save and tests Signed-off-by: Quanyi Ma <eli@patch.sh> --------- Signed-off-by: Quanyi Ma <eli@patch.sh>
No description provided.