Lib/read options key cache#3189
Conversation
📝 WalkthroughWalkthroughThe archive library and CLI now pass shared ChangesReadOptions and key-cache migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a KeyCache mechanism to ReadOptions, allowing derived encryption keys to be cached and reused across entries, which significantly optimizes key derivation for encrypted archives. The changes propagate this cache through the CLI and library components. The review feedback points out a breaking change regarding the removal of equality and ordering traits from ReadOptions and suggests optimizing the cache implementation using a Vec with a FIFO eviction policy instead of a HashMap to improve performance and cache management.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
github-actions
Bot
added
cli
Replace password-only solid entry APIs with ReadOptions-based variants so normal and solid reads share the same configuration and derived-key cache. Update CLI callers, examples, fuzz coverage, and tests for the unified API.
c01f56f to
6af0638
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/src/command/extract.rs (1)
1295-1308: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRemove the explicit
'alifetime from theread_optionsparameter to resolve a compilation error.The parameter
read_options: &'a ReadOptionsties the lifetime of the options reference to the'alifetime ofOutputOption<'a>. Becauseread_optionsis instantiated as a local stack variable in the calling function (run_extract_archive_readerandrun_extract_archive), it cannot satisfy the outer'alifetime boundary, resulting in a "borrowed value does not live long enough" compiler error.
cli/src/command/extract.rs#L1295-L1308: changeread_options: &'a ReadOptionsto an elidedread_options: &ReadOptions.cli/src/command/extract.rs#L1363-L1379: changeread_options: &'a ReadOptionsto an elidedread_options: &ReadOptions.🤖 Prompt for 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. In `@cli/src/command/extract.rs` around lines 1295 - 1308, Remove the explicit 'a lifetime from the read_options parameter in extract_file_entry and the corresponding function at cli/src/command/extract.rs lines 1295-1308 and 1363-1379, changing both to an elided reference lifetime (&ReadOptions) so local ReadOptions values can be passed without requiring the OutputOption lifetime.
🧹 Nitpick comments (4)
lib/examples/async_io.rs (1)
41-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare
ReadOptionsto leverage the derived-key cache.Creating a new
ReadOptionsinstance for every entry defeats its internal caching mechanism, which is designed to prevent re-deriving keys for every encrypted entry. Since this is an example, it's best to demonstrate the recommended pattern by instantiatingReadOptionsonce and sharing it across the extraction loop.♻️ Proposed refactor
async fn extract(path: String) -> io::Result<()> { let file = tokio::fs::File::open(path).await?.compat(); let mut archive = Archive::read_header_async(file).await?; + let read_options = ReadOptions::builder().build(); while let Some(entry) = archive.read_entry_async().await? { match entry { ReadEntry::Solid(solid_entry) => { - for entry in solid_entry.entries(ReadOptions::builder().build())? { + for entry in solid_entry.entries(&read_options)? { let entry = entry?; let mut file = io::Cursor::new(Vec::new()); - let mut reader = entry.reader(ReadOptions::builder().build())?.compat(); + let mut reader = entry.reader(&read_options)?.compat(); tokio::io::copy(&mut reader, &mut file).await?; } } ReadEntry::Normal(entry) => { let mut file = io::Cursor::new(Vec::new()); - let mut reader = entry.reader(ReadOptions::builder().build())?.compat(); + let mut reader = entry.reader(&read_options)?.compat(); tokio::io::copy(&mut reader, &mut file).await?; } }🤖 Prompt for 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. In `@lib/examples/async_io.rs` at line 41, Instantiate a single ReadOptions value before the extraction loop and pass a shared reference to it when calling solid_entry.entries. Update the loop around solid_entry.entries so all entries reuse the same ReadOptions instance and its derived-key cache.cli/src/command/verify.rs (2)
89-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the redundant
passwordvariable.Since
ReadOptionsimplements theReadOptiontrait, you can query the password directly viaread_options.password(). You can completely eliminate thepasswordvariable and argument fromverify_solidandverify_entryto simplify the code.♻️ Proposed refactor
ReadEntry::Solid(solid) => { solid_blocks += 1; - if solid.encryption() != Encryption::No && password.is_none() { + if solid.encryption() != Encryption::No && read_options.password().is_none() { println!("<solid block #{solid_blocks}>: skipped (encrypted)"); report.skipped += 1; return Ok(()); } if let Err(err) = - verify_solid(&solid, password, &read_options, fast, &mut report) + verify_solid(&solid, &read_options, fast, &mut report) { println!("<solid block #{solid_blocks}>: FAILED ({err})"); report.failed += 1; report.encrypted_failure |= solid.encryption() != Encryption::No; } } ReadEntry::Normal(entry) => { - verify_entry(&entry, password, &read_options, fast, &mut report) + verify_entry(&entry, &read_options, fast, &mut report) }🤖 Prompt for 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. In `@cli/src/command/verify.rs` around lines 89 - 98, Remove the redundant password variable from the verification flow and stop passing it to verify_solid and verify_entry. Within those functions, retrieve the password through the existing read_options.password() method, preserving the current verification behavior.
126-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the redundant
passwordparameter from function signatures.As noted above,
passwordis already held withinread_options. Removing it simplifies the interface.♻️ Proposed refactor
fn verify_solid( solid: &SolidEntry, - password: Option<&[u8]>, read_options: &ReadOptions, fast: bool, report: &mut VerifyReport, ) -> io::Result<()> { for entry in solid.entries(read_options)? { - verify_entry(&entry?, password, read_options, fast, report); + verify_entry(&entry?, read_options, fast, report); } Ok(()) } fn verify_entry( entry: &NormalEntry, - password: Option<&[u8]>, read_options: &ReadOptions, fast: bool, report: &mut VerifyReport, ) {🤖 Prompt for 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. In `@cli/src/command/verify.rs` around lines 126 - 139, Remove the redundant password parameter from the verification function signatures and update all corresponding call sites, including the call from the loop over solid.entries in the surrounding verification function. Use read_options as the sole source of the password while preserving the existing verify_entry behavior.lib/src/entry/read.rs (1)
22-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
and_thento avoid edition-specific syntax.The
if let ... && let ...syntax (let_chains) relies on the 2024 edition. You can achieve the exact same behavior in a fully backward-compatible and idiomatic way usingand_then.♻️ Proposed refactor
fn resolve_key(phsf: &str, password: &[u8], key_cache: Option<&KeyCache>) -> io::Result<Output> { - if let Some(cache) = key_cache - && let Some(key) = cache.get(phsf) - { + if let Some(key) = key_cache.and_then(|c| c.get(phsf)) { return Ok(key); } let password_hash = derive_password_hash(phsf, password)?;🤖 Prompt for 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. In `@lib/src/entry/read.rs` around lines 22 - 36, Update resolve_key to replace the edition-specific chained if-let condition with an and_then-based lookup that preserves the existing cache-hit early return and fallback password-hash derivation behavior.
🤖 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 `@lib/src/entry.rs`:
- Around line 478-488: Update the return type of entries to remove O from the
use captures bound, changing use<'a, T, O> to capture only the lifetimes and
types required by the returned iterator. Keep options consumed synchronously for
decrypt_reader initialization without tying the iterator’s lifetime to the
options type.
In `@lib/tests/extract_solid_compatibility.rs`:
- Around line 53-54: Update the assert_entry helper to accept &ReadOptions
instead of a raw password, clone those options when constructing the entry
reader, and change every assert_entry call in extract_all—including the
extraction loop—to pass the shared &read_options so key-cache reuse is
exercised.
---
Outside diff comments:
In `@cli/src/command/extract.rs`:
- Around line 1295-1308: Remove the explicit 'a lifetime from the read_options
parameter in extract_file_entry and the corresponding function at
cli/src/command/extract.rs lines 1295-1308 and 1363-1379, changing both to an
elided reference lifetime (&ReadOptions) so local ReadOptions values can be
passed without requiring the OutputOption lifetime.
---
Nitpick comments:
In `@cli/src/command/verify.rs`:
- Around line 89-98: Remove the redundant password variable from the
verification flow and stop passing it to verify_solid and verify_entry. Within
those functions, retrieve the password through the existing
read_options.password() method, preserving the current verification behavior.
- Around line 126-139: Remove the redundant password parameter from the
verification function signatures and update all corresponding call sites,
including the call from the loop over solid.entries in the surrounding
verification function. Use read_options as the sole source of the password while
preserving the existing verify_entry behavior.
In `@lib/examples/async_io.rs`:
- Line 41: Instantiate a single ReadOptions value before the extraction loop and
pass a shared reference to it when calling solid_entry.entries. Update the loop
around solid_entry.entries so all entries reuse the same ReadOptions instance
and its derived-key cache.
In `@lib/src/entry/read.rs`:
- Around line 22-36: Update resolve_key to replace the edition-specific chained
if-let condition with an and_then-based lookup that preserves the existing
cache-hit early return and fallback password-hash derivation behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ddeb9ee-e11f-424a-89fa-7037280ca6fc
📒 Files selected for processing (24)
cli/src/command/acl.rscli/src/command/core.rscli/src/command/core/archive_source.rscli/src/command/diff.rscli/src/command/extract.rscli/src/command/list.rscli/src/command/sort.rscli/src/command/update.rscli/src/command/verify.rscli/src/command/xattr.rscli/tests/cli/stdio/option_ignore_zeros.rscli/tests/cli/stdio/option_mac_metadata.rscli/tests/cli/utils/archive.rsfuzz/fuzz_targets/split_archive.rslib/examples/async_io.rslib/src/archive.rslib/src/archive/read.rslib/src/archive/read/slice.rslib/src/archive/write.rslib/src/entry.rslib/src/entry/builder.rslib/src/entry/options.rslib/src/entry/read.rslib/tests/extract_solid_compatibility.rs
| pub fn entries<'a, O: ReadOption>( | ||
| &'a self, | ||
| options: O, | ||
| ) -> io::Result<impl Iterator<Item = io::Result<NormalEntry>> + use<'a, T, O>> { | ||
| let reader = decrypt_reader( | ||
| self.encoded_reader(), | ||
| self.header.encryption, | ||
| self.header.cipher_mode, | ||
| self.phsf.as_deref(), | ||
| password, | ||
| options.password(), | ||
| options.key_cache(), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove O from the use<...> captures bound.
The options argument is consumed synchronously to initialize the DecryptReader, and the returned iterator does not capture or store it. Including O in the use<'a, T, O> bound incorrectly ties the iterator's lifetime to O's lifetime. If a caller passes a reference (e.g., &ReadOptions), this artificial capture bound will prevent them from mutating or moving the options while the iterator lives, leading to unnecessary borrow-checker friction.
♻️ Proposed fix
pub fn entries<'a, O: ReadOption>(
&'a self,
options: O,
- ) -> io::Result<impl Iterator<Item = io::Result<NormalEntry>> + use<'a, T, O>> {
+ ) -> io::Result<impl Iterator<Item = io::Result<NormalEntry>> + use<'a, T>> {
let reader = decrypt_reader(
self.encoded_reader(),
self.header.encryption,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn entries<'a, O: ReadOption>( | |
| &'a self, | |
| options: O, | |
| ) -> io::Result<impl Iterator<Item = io::Result<NormalEntry>> + use<'a, T, O>> { | |
| let reader = decrypt_reader( | |
| self.encoded_reader(), | |
| self.header.encryption, | |
| self.header.cipher_mode, | |
| self.phsf.as_deref(), | |
| password, | |
| options.password(), | |
| options.key_cache(), | |
| pub fn entries<'a, O: ReadOption>( | |
| &'a self, | |
| options: O, | |
| ) -> io::Result<impl Iterator<Item = io::Result<NormalEntry>> + use<'a, T>> { | |
| let reader = decrypt_reader( | |
| self.encoded_reader(), | |
| self.header.encryption, | |
| self.header.cipher_mode, | |
| self.phsf.as_deref(), | |
| options.password(), | |
| options.key_cache(), |
🧰 Tools
🪛 GitHub Actions: semver-checks / 0_Semver checks.txt
[error] 478-478: cargo semver checks failed: method_requires_different_generic_type_params. Failed: libpna::SolidEntry::entries now takes 1 generic type parameter instead of 0.
🤖 Prompt for 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.
In `@lib/src/entry.rs` around lines 478 - 488, Update the return type of entries
to remove O from the use captures bound, changing use<'a, T, O> to capture only
the lifetimes and types required by the returned iterator. Keep options consumed
synchronously for decrypt_reader initialization without tying the iterator’s
lifetime to the options type.
| let read_options = ReadOptions::with_password(password); | ||
| for entry in archive_reader.entries_with_options(&read_options) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Share ReadOptions to exercise the key cache.
Currently, assert_entry is called with the raw password, causing it to internally instantiate a new ReadOptions (and therefore an empty key cache) for every single entry. This means the Key Derivation Function (KDF) is executed repeatedly, and the cache reuse logic—a primary objective of this PR—is never actually tested.
Update the unchanged assert_entry helper to accept options: &ReadOptions, use options.clone() when creating the reader, and pass the shared &read_options to it from this extraction loop.
🛠️ Proposed changes
For the unchanged assert_entry helper at the top of the file:
-fn assert_entry(item: NormalEntry, password: Option<&[u8]>) {
+fn assert_entry(item: NormalEntry, options: &ReadOptions) {
let path = item.header().path().as_str();
let mut dist = Vec::new();
- let mut reader = item.reader(ReadOptions::with_password(password)).unwrap();
+ let mut reader = item.reader(options.clone()).unwrap();And update the assert_entry call sites throughout extract_all:
let read_options = ReadOptions::with_password(password);
for entry in archive_reader.entries_with_options(&read_options) {
let item = entry.unwrap();
if item.header().data_kind() == DataKind::Directory {
continue;
}
n += 1;
- assert_entry(item, password);
+ assert_entry(item, &read_options);
}
// ...
for entry in archive_reader.entries() {
let item = entry.unwrap();
match item {
ReadEntry::Solid(item) => {
for item in item.entries(&read_options).unwrap() {
let item = item.unwrap();
if item.header().data_kind() == DataKind::Directory {
continue;
}
n += 1;
- assert_entry(item, password);
+ assert_entry(item, &read_options);
}
}
ReadEntry::Normal(item) => {
if item.header().data_kind() == DataKind::Directory {
continue;
}
n += 1;
- assert_entry(item, password);
+ assert_entry(item, &read_options);
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let read_options = ReadOptions::with_password(password); | |
| for entry in archive_reader.entries_with_options(&read_options) { | |
| fn assert_entry(item: NormalEntry, options: &ReadOptions) { | |
| let path = item.header().path().as_str(); | |
| let mut dist = Vec::new(); | |
| let mut reader = item.reader(options.clone()).unwrap(); | |
| assert_entry(item, &read_options); | |
| assert_entry(item, &read_options); | |
| assert_entry(item, &read_options); |
🤖 Prompt for 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.
In `@lib/tests/extract_solid_compatibility.rs` around lines 53 - 54, Update the
assert_entry helper to accept &ReadOptions instead of a raw password, clone
those options when constructing the entry reader, and change every assert_entry
call in extract_all—including the extraction loop—to pass the shared
&read_options so key-cache reuse is exercised.
1 participant
Summary by CodeRabbit