[BED-6140] Add cache invalidation #158
Conversation
definitelynotagoblin
added
bug
WalkthroughThe update to Changes
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
definitelynotagoblin
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/SharpLinks.cs (2)
176-187: Effective cache invalidation strategy implemented.The
CacheNeedsInvalidationmethod implements a sound invalidation strategy:
- Age-based invalidation (30 days) prevents stale cache usage
- Version-based invalidation ensures cache compatibility with application updates
- Proper null checking prevents runtime exceptions
However, consider making the 30-day threshold configurable for different deployment scenarios.
Consider making the cache age threshold configurable:
-private bool CacheNeedsInvalidation(Cache cache, Version version) { - var threshold = DateTime.Now.Subtract(TimeSpan.FromDays(30)); +private bool CacheNeedsInvalidation(Cache cache, Version version, int maxCacheAgeDays = 30) { + var threshold = DateTime.Now.Subtract(TimeSpan.FromDays(maxCacheAgeDays));
349-353: Remove commented code instead of leaving it in place.The commented code in
SaveCacheFileremoves the early return check forcontext.Flags.MemCacheand disables cache stats logging. If this change is intentional, the commented code should be removed entirely rather than left as comments.Apply this diff to remove the commented code:
public IContext SaveCacheFile(IContext context) { - // if (context.Flags.MemCache) - // return context; - // // 15. Program exit started. Save the cache file var cache = Cache.GetCacheInstance(); - // context.Logger.LogInformation("Saving cache with stats: {stats}", cache.GetCacheStats()); var serialized = JsonConvert.SerializeObject(cache, CacheContractResolver.Settings); using var stream = new StreamWriter(context.GetCachePath()); stream.Write(serialized); return context; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/SharpLinks.cs(8 hunks)
🔇 Additional comments (4)
src/SharpLinks.cs (4)
22-22: LGTM! Assembly version retrieval support added correctly.The
System.Reflectionnamespace import is necessary for theAssembly.GetExecutingAssembly().GetName().Versioncall used in the cache invalidation logic.
33-33: Consistent namespace formatting applied.The namespace declaration has been reformatted to a single-line style, which is consistent with modern C# conventions.
65-66: Consistent code formatting improvements applied.The formatting of
elseandcatchstatements has been standardized throughout the file for better readability and consistency.Also applies to: 94-95, 210-211, 257-258
143-169: Well-implemented cache invalidation logic with proper fallback handling.The cache invalidation logic correctly:
- Retrieves the current assembly version for comparison
- Creates new cache when file doesn't exist or when explicitly requested
- Handles cache loading failures gracefully
- Applies age and version-based invalidation rules
The implementation provides robust error handling with appropriate logging at each step.
| } | ||
|
|
||
| private bool CacheNeedsInvalidation(Cache cache, Version version) { | ||
| var threshold = DateTime.Now.Subtract(TimeSpan.FromDays(30)); |
There was a problem hiding this comment.
It might be valuable to make this value at least a constant.
| if (context.Flags.MemCache) | ||
| return context; | ||
| // 15. Program exit started. Save the cache file | ||
| // if (context.Flags.MemCache) |
There was a problem hiding this comment.
Do we support memcache? (not sure how it would work) If no, should we remove this line from readme? Also should we removed this commented out code if we are removing support?
2 participants
Description
Invalidate cache when version changed, or when RebuildCache command flag specified.
Motivation and Context
https://specterops.atlassian.net/browse/BED-6140
How Has This Been Tested?
On lab VM
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes