HS: Prune stale cache files from disk v4.1 #14516
Open
+497
−83
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 11 commits
December 19, 2025 09:40
To have a system-level overview of when was the last time the file was used, update the file modification timestamp to to the current time. This is needed to remove stale cache files of the system. Access time is not used as it may be, on the system level, disabled. Ticket: 7830
Ticket: 7830
Hyperscan MPM can cache the compiled contexts to files. This however grows as rulesets change and leads to bloating the system. This addition prunes the stale cache files based on their modified file timestamp. Part of this work incorporates new model for MPM cache stats to split it out from the cache save function and aggregate cache-related stats in one place (newly added pruning). Ticket: 7830
This is especially relevant for multi-instance simultaneous setups as we might risk read/write races.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14516 +/- ##
==========================================
- Coverage 82.11% 82.10% -0.01%
==========================================
Files 1013 1013
Lines 262297 262495 +198
==========================================
+ Hits 215380 215532 +152
- Misses 46917 46963 +46
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 41 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+355
to
+359
| PatternDatabaseCache *pd_cache_stats = mpm_conf->cache_stats; | ||
| pd_cache_stats->hs_dbs_cache_pruned_cnt = removed; | ||
| pd_cache_stats->hs_dbs_cache_pruned_considered_cnt = considered; | ||
| pd_cache_stats->hs_dbs_cache_pruned_cutoff = cutoff; | ||
| pd_cache_stats->cache_max_age_seconds = mpm_conf->cache_max_age_seconds; |
There was a problem hiding this comment.
In SCHSCachePrune, mpm_conf->cache_stats is dereferenced without checking if it's NULL. If CacheStatsInit is not implemented or returns NULL for some reason but CachePrune is implemented, this will cause a null pointer dereference. Add a null check before accessing cache_stats.
| { | ||
| uint32_t hash[2] = { 0 }; | ||
| hashword2(&pd->pattern_cnt, 1, &hash[0], &hash[1]); | ||
| SCSha256 *hasher = SCSha256New(); |
There was a problem hiding this comment.
SCSha256New() is called without checking if it returns NULL. While Box::new in Rust will panic on allocation failure rather than returning NULL, it's good practice to check the return value when crossing FFI boundaries to make the code more defensive.
|
Information: QA ran without warnings. Pipeline = 28848 |
| } | ||
|
|
||
| if (!SCSha256FinalizeToHex(hasher, hash, hash_len)) { | ||
| hasher = NULL; |
Member
There was a problem hiding this comment.
why is this set to NULL here and not in the success case?
Member
|
For a bigger and more complex PR like this, please don't include doc space cleanup commits. Those can go into their own PR to ease review. |
Member
|
In my MT setup, the old files are removed as expected, but I do get lots of warnings |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow-up of #14512
Link to ticket: https://redmine.openinfosecfoundation.org/issues/7830
Describe changes:
v4.1:
v4.0:
v3.3:
v3:
v2:
v1:
The logic to determine a stale file is currently based on the modification timestamp in the file systems. The accessed time stamp was not used as it may be switched off. Alternatively, we could use a local DB/notekeeping file of the last used files/caches but this approach seemed simpler.
I can also add GitHub CI tests, I thought of some scenarios.