Skip to content

Replace bincode with rkyv 0.8.15#117

Merged
genedna merged 9 commits intoweb3infra-foundation:mainfrom
genedna:main
Mar 13, 2026
Merged

Replace bincode with rkyv 0.8.15#117
genedna merged 9 commits intoweb3infra-foundation:mainfrom
genedna:main

Conversation

@genedna
Copy link
Member

@genedna genedna commented Mar 13, 2026

No description provided.

Signed-off-by: Quanyi Ma <eli@patch.sh>
Copilot AI review requested due to automatic review settings March 13, 2026 14:11
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bincode with rkyv across the workspace (Cargo + Buck third-party crates).
  • Update pack cache object file persistence to use rkyv encode/decode, and add rkyv derives to types used in cached structures.
  • Regenerate/update buckal.snap and 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.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

genedna added 2 commits March 13, 2026 22:22
Signed-off-by: Quanyi Ma <eli@patch.sh>
Signed-off-by: Quanyi Ma <eli@patch.sh>
Copilot AI review requested due to automatic review settings March 13, 2026 14:30
Signed-off-by: Quanyi Ma <eli@patch.sh>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bincode usage in the pack cache on-disk serialization with rkyv and introduce a versioned cache layout directory (rkyv-v1).
  • Add rkyv derives 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 (+ transitive unty/virtue) and add rkyv (+ 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>
Copy link
Member Author

@genedna genedna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) and CacheObjectOnDiskRef<'a> (borrowed, for serialization). This avoids cloning the data_decompressed: Vec<u8> during writes via Cow::Borrowed — good zero-copy discipline.
  • The FileLoadStore trait is cleanly refactored: the blanket impl now targets rkyv's trait bounds instead of serde's, and the manual impl for CacheObject correctly bridges through the on-disk proxy types.
  • CACHE_LAYOUT_VERSION in 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:

  1. Unbounded retry loop in write_bytes_atomically (cache_object.rs):
    The loop increments attempt via saturating_add(1) but has no upper bound. If a pathological scenario causes temp files to accumulate (e.g., a zombie process holding .temp files), 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",
            ));
        }
    }
  2. Residual serde::{Serialize, Deserialize} on CacheObject: The struct still derives serde::Serialize and serde::Deserialize, but CacheObject now has a manual FileLoadStore impl that uses rkyv through the proxy types. If serde on CacheObject is 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.

  3. ArcWrapperBounds trait alias change: The bound was updated from Serialize + Deserialize to FileLoadStore, which is correct and aligns with the new architecture. Good.


2. Security

Strengths:

  • bytecheck feature is enabled — this is critical. rkyv without validation can cause undefined behavior from malformed input. The use of rkyv::from_bytes::<T, RkyvError>() which goes through HighValidator + CheckBytes ensures archived data is validated before access.
  • The CheckBytes bound on the FileLoadStore blanket 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. The bytecheck validation provides adequate defense-in-depth here.

3. Performance

Strengths:

  • rkyv's zero-copy deserialization means f_load only 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.
  • CacheObjectOnDiskRef serializes by borrowing data_decompressed as Cow::Borrowed(&[u8]), avoiding a full copy during f_save. This is the right pattern for rkyv.
  • The CACHE_LAYOUT_VERSION subdirectory ensures no fallback parsing of old format — clean cutover, no performance penalty from format detection.

Minor observations:

  1. No fsync before rename: write_bytes_atomically does file.write_all(&data)drop(file)fs::rename() without calling file.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.

  2. rkyv::to_bytes allocates an AlignedVec on every f_save call. This is inherent to rkyv's serialization model and is comparable to what bincode was doing (allocating a Vec<u8>). For the write path this is acceptable since the bottleneck is disk I/O.


4. Testing

Strengths:

  • test_cache_object_serialize is updated to use the new rkyv proxy types (CacheObjectOnDiskRef / CacheObjectOnDisk), and now also asserts b.mem_recorder.is_none() — verifying the non-serializable field is correctly excluded. Good.
  • New test test_write_bytes_atomically_retries_when_temp_exists covers the stale-temp-file retry path.

Suggestions:

  1. Missing test: roundtrip through FileLoadStore trait (file I/O path): The existing test_cache_object_serialize tests in-memory serialization, but there's no test that exercises CacheObject::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();
    }
  2. Test uses relative paths: test_write_bytes_atomically_retries_when_temp_exists uses .cache_temp/... which depends on the working directory and could leave artifacts. Consider using std::env::temp_dir() or the existing test pattern for consistency.

  3. Missing edge case: No test for write_bytes_atomically when 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_VERSION constant 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>
Copilot AI review requested due to automatic review settings March 13, 2026 15:05
Copy link
Member Author

@genedna genedna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Unbounded retry loop — Fixed. MAX_TEMP_ATTEMPTS = 64 constant added, with a clear error message including path.display() for debuggability.

  2. Dead serde derives on CacheObject — Fixed. CacheObject now derives only #[derive(Debug)], removing the unused serde::Serialize and serde::Deserialize. Clean.

  3. CACHE_LAYOUT_VERSION doc comment — Added. Three-line doc comment explains the purpose clearly.

  4. File I/O roundtrip test — Added. test_cache_object_file_roundtrip exercises f_save()f_load() through actual files and verifies mem_recorder is None after deserialization.

  5. Early-return edge case test — Added. test_write_bytes_atomically_returns_when_target_exists confirms idempotent behavior when the target already exists.

  6. 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.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bincode usage in the pack cache with rkyv, 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/virtue and adding rkyv + 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.

@genedna
Copy link
Member Author

genedna commented Mar 13, 2026

@codex review

Signed-off-by: Quanyi Ma <eli@patch.sh>
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +187 to +189
let data = rkyv::to_bytes::<RkyvError>(&CacheObjectOnDiskRef::from(self))
.map_err(io::Error::other)?;
write_bytes_atomically(path, &data)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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>
Copilot AI review requested due to automatic review settings March 13, 2026 15:45
@genedna
Copy link
Member Author

genedna commented Mar 13, 2026

@codex review

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bincode with rkyv in Rust dependencies and Buck targets; removed now-unused bincode/unty/virtue third-party definitions.
  • Updated pack cache object persistence to use rkyv, including a cache layout version segment to avoid deserializing old cache formats.
  • Added rkyv derives to core object/hash types that were previously using bincode’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> {
Comment on lines +666 to +675
#[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");
}
@chatgpt-codex-connector
Copy link

Codex Review: Didn't find any major issues. Swish!

ℹ️ 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>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 with rkyv and 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 added rkyv (+ 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.

@genedna genedna merged commit 53f1c55 into web3infra-foundation:main Mar 13, 2026
12 checks passed
genedna added a commit to genedna/git-internal that referenced this pull request Mar 15, 2026
* 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>
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.

2 participants