feat: make decoders cache refreshable by adding a new setting option #36
Conversation
Summary of ChangesHello @ashuralyk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the application's caching strategy by introducing a configurable expiration time for decoder caches. This allows the system to automatically refresh cached Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 valuable feature for making decoder caches refreshable via a new configuration setting. The implementation is well-structured, adding the necessary configuration, updating the data types, and ensuring cache directories are created at startup. My review focuses on a logic bug and a potential for improvement in the new cache expiration function, along with a suggestion for improving configuration consistency.
| fn file_older_than_minutes(file_path: &PathBuf, minutes: u64) -> bool { | ||
| match std::fs::metadata(file_path) { | ||
| Ok(metadata) => { | ||
| let Ok(mut duration) = SystemTime::now().duration_since(SystemTime::UNIX_EPOCH) else { | ||
| return true; | ||
| }; | ||
| if let Ok(Ok(checkpoint)) = metadata | ||
| .modified() | ||
| .map(|time| time.duration_since(SystemTime::UNIX_EPOCH)) | ||
| { | ||
| duration = duration.saturating_sub(checkpoint); | ||
| } else if let Ok(Ok(checkpoint)) = metadata | ||
| .created() | ||
| .map(|time| time.duration_since(SystemTime::UNIX_EPOCH)) | ||
| { | ||
| duration = duration.saturating_sub(checkpoint); | ||
| } | ||
| duration.as_secs() / 60 >= minutes | ||
| } | ||
| Err(_) => true, | ||
| } | ||
| } |
There was a problem hiding this comment.
The implementation of file_older_than_minutes is a bit complex and contains a subtle bug. If a file's modification timestamp is in the future compared to the system clock (which can happen due to clock drift, NTP updates, or manual changes), the current logic will calculate a zero duration, incorrectly concluding the file is not old and doesn't need to be refreshed. The safe approach in such cases is to refresh the cache.
I suggest refactoring this function for clarity and correctness using SystemTime::elapsed(), which simplifies duration calculation and handles time-related errors gracefully.
This revised version is more readable and robustly handles edge cases like file timestamps from the future.
fn file_older_than_minutes(file_path: &PathBuf, minutes: u64) -> bool {
if minutes == 0 {
return true;
}
let metadata = match std::fs::metadata(file_path) {
Ok(m) => m,
Err(_) => return true, // File doesn't exist or we can't access it, so it's "old".
};
let file_time = match metadata.modified().or_else(|_| metadata.created()) {
Ok(time) => time,
Err(_) => return true, // Can't get a timestamp, assume it's old.
};
match file_time.elapsed() {
Ok(elapsed) => elapsed.as_secs() >= minutes * 60,
Err(_) => true, // System clock is earlier than file time, assume it's old to be safe.
}
}|
|
||
| # directory that stores decoders on hard-disk, including on-chain and off-chain binary files | ||
| decoders_cache_directory = "cache/decoders" | ||
| decoders_cache_directory = "cache/decoders/testnet" |
There was a problem hiding this comment.
It's a good practice to namespace the cache directory for testnet to avoid conflicts. For consistency and to prevent any potential confusion when switching between environments, you might consider also namespacing the mainnet cache directory in settings.mainnet.toml (e.g., to cache/decoders/mainnet).
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a cache expiration mechanism for decoders, configured via a new decoders_cache_expiration_minutes setting. The implementation adds a helper function to check file age and updates the decoder fetching logic to use it. Additionally, it ensures cache directories are created at startup. My review focuses on improving configuration consistency, error handling, and application robustness. I've suggested making the mainnet cache directory path specific to avoid conflicts, improving error logging in the new file age check function, and handling startup errors more gracefully instead of panicking.
|
|
||
| # directory that stores decoders on hard-disk, including on-chain and off-chain binary files | ||
| decoders_cache_directory = "cache/decoders" | ||
| decoders_cache_directory = "cache/decoders/testnet" |
There was a problem hiding this comment.
Adding a network-specific path for the testnet cache is a good improvement to prevent conflicts. To be consistent and avoid potential issues, consider also updating settings.mainnet.toml to use a mainnet-specific path, for example cache/decoders/mainnet, instead of the generic cache/decoders.
| let metadata = match std::fs::metadata(file_path) { | ||
| Ok(m) => m, | ||
| Err(_) => return true, // File doesn't exist or we can't access it, so it's "old". | ||
| }; | ||
|
|
||
| let file_time = match metadata.modified().or_else(|_| metadata.created()) { | ||
| Ok(time) => time, | ||
| Err(_) => return true, // Can't get a timestamp, assume it's old. | ||
| }; | ||
|
|
||
| match file_time.elapsed() { | ||
| Ok(elapsed) => elapsed.as_secs() >= minutes * 60, | ||
| Err(_) => true, // System clock is earlier than file time, assume it's old to be safe. | ||
| } |
There was a problem hiding this comment.
The error handling in this function consistently returns true while ignoring the underlying error. This is a safe fallback, but it makes debugging potential filesystem issues (like permissions) very difficult because the errors are completely silent. It would be beneficial to log these errors before returning true to provide insight into why a file couldn't be accessed.
| fs::create_dir_all(&settings.decoders_cache_directory) | ||
| .expect("failed to create decoders cache directory"); | ||
| fs::create_dir_all(&settings.dobs_cache_directory) | ||
| .expect("failed to create DOBs cache directory"); |
There was a problem hiding this comment.
Using .expect() will cause the application to panic if it fails to create the directories. While this is a fatal startup error, it's generally better to handle the Result explicitly, log a helpful error message, and exit the process cleanly. This avoids a panic and provides more context to the user running the application.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a cache expiration mechanism for decoders, configured via a new decoders_cache_expiration_minutes setting. The implementation is sound, adding a helper function to check file age and applying it correctly to TypeId and TypeScript decoders. The changes also include a good robustness improvement by ensuring cache directories exist at startup. My review includes a couple of suggestions to improve code idiom and enhance debuggability.
| fn file_older_than_minutes(file_path: &PathBuf, minutes: u64) -> bool { | ||
| if minutes == 0 { | ||
| return true; | ||
| } | ||
| let metadata = match std::fs::metadata(file_path) { | ||
| Ok(m) => m, | ||
| Err(_) => return true, // File doesn't exist or we can't access it, so it's "old". | ||
| }; | ||
|
|
||
| let file_time = match metadata.modified().or_else(|_| metadata.created()) { | ||
| Ok(time) => time, | ||
| Err(_) => return true, // Can't get a timestamp, assume it's old. | ||
| }; | ||
|
|
||
| match file_time.elapsed() { | ||
| Ok(elapsed) => elapsed.as_secs() >= minutes * 60, | ||
| Err(_) => true, // System clock is earlier than file time, assume it's old to be safe. | ||
| } | ||
| } |
There was a problem hiding this comment.
This function is logically correct. However, it can be made more concise and idiomatic by chaining Result and Option combinators. This approach reduces nesting and can be more readable for developers familiar with functional-style error handling in Rust. The unwrap_or(true) pattern elegantly handles all error cases (file not found, timestamp unavailable, clock skew) by defaulting to the safe behavior of treating the cache as expired.
fn file_older_than_minutes(file_path: &PathBuf, minutes: u64) -> bool {
if minutes == 0 {
return true;
}
let threshold = std::time::Duration::from_secs(minutes * 60);
// Treat the file as "old" (return true) if metadata can't be read,
// timestamps are unavailable, or if the system clock is behind the file time.
std::fs::metadata(file_path)
.and_then(|m| m.modified().or_else(|_| m.created()))
.map(|time| {
time.elapsed()
.map(|elapsed| elapsed >= threshold)
.unwrap_or(true) // System clock is earlier than file time, assume old.
})
.unwrap_or(true) // File doesn't exist or timestamps unavailable, assume old.
}| fs::create_dir_all(&settings.decoders_cache_directory) | ||
| .expect("failed to create decoders cache directory"); | ||
| fs::create_dir_all(&settings.dobs_cache_directory) | ||
| .expect("failed to create DOBs cache directory"); |
There was a problem hiding this comment.
Using .expect() is suitable for fatal startup errors. However, the panic messages can be more helpful for debugging. By including the problematic path and the underlying I/O error, you can more quickly diagnose issues related to file permissions or incorrect path configurations.
| fs::create_dir_all(&settings.decoders_cache_directory) | |
| .expect("failed to create decoders cache directory"); | |
| fs::create_dir_all(&settings.dobs_cache_directory) | |
| .expect("failed to create DOBs cache directory"); | |
| fs::create_dir_all(&settings.decoders_cache_directory) | |
| .unwrap_or_else(|e| panic!("failed to create decoders cache directory at {:?}: {}", &settings.decoders_cache_directory, e)); | |
| fs::create_dir_all(&settings.dobs_cache_directory) | |
| .unwrap_or_else(|e| panic!("failed to create DOBs cache directory at {:?}: {}", &settings.dobs_cache_directory, e)); |
1 participant
Description
Just added a new setting option for enabling a feature of making decoder caches expire-able in minutes unit