Adds Cache Groups concepts to Cripts

[zwoop]

This is a second version, since half of the original patch was merged in a separate PR.

[Copilot]

Pull request overview

This PR introduces Cache Groups functionality to Cripts, providing infrastructure for managing associations between cache entries using custom identifiers. This implementation follows an emerging RFC draft for cache group invalidation patterns in HTTP caching.

Key changes include:

• New Cache::Group class with disk persistence, rotating hash maps, and configurable aging policies
• Thread-safe operations using shared_mutex with automatic periodic syncing to disk
• Example implementation and comprehensive documentation for using Cache Groups in Cripts

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
src/cripts/CacheGroup.cc	Core implementation with hash-based storage, disk I/O, transaction logging, and Manager singleton for lifecycle management
include/cripts/CacheGroup.hpp	Public API defining the Group class with Insert/Lookup methods and nested Manager class
src/cripts/CMakeLists.txt	Adds CacheGroup.hpp to the list of public headers
include/cripts/Matcher.hpp	Includes algorithm header (duplicate include)
example/cripts/cache_groups.cc	Working example demonstrating Cache Groups for cache invalidation workflows
doc/developer-guide/cripts/cripts-misc.en.rst	Documentation explaining Cache Groups concept and usage patterns

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[bryancall]

Thanks for adding Cache Groups to Cripts! This is a useful feature for implementing cache group invalidation patterns. I have a few observations:

Critical Bug: Iterator increment after erase()

In _cripts_cache_group_sync (line ~50-60), there's an iterator invalidation bug:

for (auto it = groups.begin(); it != groups.end() && processed < max_to_process; ++it) {
    if (auto group = it->second.lock()) {
      // ...
    } else {
      it = groups.erase(it);  // Returns next iterator, then loop does ++it, skipping an element
    }
}

When erase() returns the next valid iterator and then the loop increments again, an element gets skipped. Consider:

for (auto it = groups.begin(); it != groups.end() && processed < max_to_process; ) {
    if (auto group = it->second.lock()) {
      // ...
      ++it;
    } else {
      it = groups.erase(it);  // Don't increment here
    }
}

Error Handling

1. Missing file read error checking in LoadFromDisk(): The file.read() calls (lines ~229-232 and ~241) don't check if reads succeeded. If the file is corrupted or truncated, uninitialized data gets used.

2. clearLog() called unconditionally in WriteToDisk(): If syncMap() fails, the transaction log is still cleared, which could lead to data loss. Consider only clearing the log after all syncs succeed.

3. Inconsistent error reporting: Line 318 uses std::cerr while the rest of the code uses TSWarning. Should be consistent.

4. Missing filesystem error handling in Initialize() (lines ~85-86): create_directories and permissions can throw or fail silently. The clearLog() method (line 363) shows the correct pattern using error_code overloads.

Documentation & Style

1. Spelling: "hodling" → "holding" (line 289), "assosication" → "association" (docs line 421)
2. Duplicate #include <algorithm> in Matcher.hpp
3. The magic number 63072000 (2 years in seconds) appears multiple times - consider a named constant with documentation explaining the choice

Testing

This is a significant feature with complex persistence logic (disk serialization, transaction log replay, crash recovery). Would be good to have automated test coverage for these code paths.

Minor API Note

The Factory() returning void* requiring manual delete in do_delete_instance() is a bit awkward, but I understand this may be constrained by the Cripts plugin interface.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 15 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The _map_index member is declared as std::atomic but is accessed without atomic operations in multiple places. In the Insert method, it's read and written non-atomically (lines 126, 144), and in the Lookup method, it's read without atomic load (line 176). Since _mutex protects these operations, using std::atomic is unnecessary and misleading. Either use proper atomic operations or change it to a regular size_t since it's already protected by the mutex.

Suggested change

	std::atomic<size_t> _map_index = 0;
	size_t _map_index = 0;

[Copilot]

The comparison operator should use '!=' instead of '>' for comparing with time_point zero to check validity. Time points don't have a natural ordering relative to "zero" in the same way integers do, and using '>' is semantically unclear. Consider using '!=' to check if the date is valid/not-zero.

Suggested change

	if (date > 0) {
	if (date != 0) {

[Copilot]

The same comparison pattern appears here. Using '>' to compare a time_t value with 0 works but using '!=' would be more semantically clear to indicate checking for validity rather than ordering.

Suggested change

	if (date > 0) {
	auto cache_groups = cached.response["Cache-Groups"];
	if (!cache_groups.empty()) {
	borrow cg = *static_cast<std::shared_ptr<cripts::Cache::Group> *>(ptr);
	if (date != 0) {
	auto cache_groups = cached.response["Cache-Groups"];
	if (!cache_groups.empty()) {
	borrow cg = *static_cast<std::shared_ptr<std::shared_ptr<cripts::Cache::Group> *>(ptr);

[Copilot]

The hash collision detection at line 129 only checks prefix and length but doesn't verify the actual key content. While the prefix check provides some collision resistance, two different keys could have the same hash, same length, and same 4-byte prefix, leading to incorrect cache group behavior. Consider either storing the full key or adding a more robust collision detection mechanism.

[Copilot]

The same hash collision issue exists in the Lookup method. The check at line 189 verifies hash, timestamp, length, and prefix, but doesn't confirm the actual key matches. This could result in false positive lookups when hash collisions occur with keys that have matching length and prefix.

[Copilot]

The comment contains a typo: "contination" should be "continuation".

[Copilot]

Memory allocated by 'new std::shared_ptr' in the Factory method could leak if an exception is thrown before the caller takes ownership. Consider using std::make_unique or returning the shared_ptr by value instead of raw pointer to ensure exception safety.

[Copilot]

The continuation is scheduled with TSContScheduleEveryOnPool but there's no check whether scheduling succeeded. If _action is nullptr after this call, the continuation won't run and cache groups won't be synced to disk, potentially leading to data loss. Add error checking and logging if scheduling fails.

[Copilot]

Missing documentation for the Cache Groups feature. The code lacks explanation of key concepts like what cache groups are used for, how the rotation mechanism works, what the parameters mean (num_maps, max_entries, max_age), and how the disk persistence works. Consider adding comprehensive documentation including class-level and method-level comments explaining the design and usage patterns.

[Copilot]

If std::rename fails, the temporary file is removed but the original file may be in an inconsistent state. The error handling should ensure that either the update succeeds completely or the original file remains intact. Consider implementing a more robust error recovery strategy, such as keeping a backup of the original file before attempting the rename.