Changes from code review

Author: zwoop

doc/developer-guide/cripts/cripts-misc.en.rst

@@ -418,7 +418,7 @@ Debug logging uses the same format string syntax as ``fmt::format()`` in ``libfm
 Cache Groups
 ============
 
-As a way to manage assosication between cache entries, Cripts provides an infrastructure
+As a way to manage association between cache entries, Cripts provides an infrastructure
 for cache groups. A cache group is a set of cache entries that are logically
 associated with each other via custom identifiers.
 

include/cripts/CacheGroup.hpp

@@ -49,6 +49,14 @@ class Group
     uint64_t            hash;      // Hash value of the group ID, needed when writing to disk
   };
 
+  // Header structure for on-disk map files (after VERSION field)
+  struct _MapHeader {
+    time_t created_ts;
+    time_t last_write_ts;
+    time_t last_sync_ts;
+    size_t count;
+  };
+
   using _MapType = std::unordered_map<uint64_t, _Entry>;
 
   struct _MapSlot {
@@ -65,9 +73,11 @@ class Group
                                       (static_cast<uint64_t>('P') << 24) | (static_cast<uint64_t>('S') << 16) |
                                       (static_cast<uint64_t>('0') << 8) | 0x00; // Change this on version bump
 
+  static constexpr std::chrono::seconds DEFAULT_MAX_AGE{63072000}; // 2 Years, max cache lifetime in ATS as well
+
   Group(const std::string &name, const std::string &base_dir, size_t max_entries = 1024, size_t num_maps = 3)
   {
-    Initialize(name, base_dir, num_maps, max_entries, std::chrono::seconds{63072000});
+    Initialize(name, base_dir, num_maps, max_entries, DEFAULT_MAX_AGE);
   }
 
   // Not used at the moment.
@@ -79,7 +89,7 @@ class Group
   self_type &operator=(const self_type &) = delete;
 
   void Initialize(const std::string &name, const std::string &base_dir, size_t num_maps = 3, size_t max_entries = 1024,
-                  std::chrono::seconds max_age = std::chrono::seconds{63072000});
+                  std::chrono::seconds max_age = DEFAULT_MAX_AGE);
 
   void
   SetMaxEntries(size_t max_entries)
@@ -127,7 +137,7 @@ class Group
   std::string               _name        = "CacheGroup";
   size_t                    _num_maps    = 3;
   size_t                    _max_entries = 1024;
-  std::chrono::seconds      _max_age     = std::chrono::seconds(63072000);
+  std::chrono::seconds      _max_age     = DEFAULT_MAX_AGE;
   std::atomic<size_t>       _map_index   = 0;
   cripts::Time::Point       _last_sync   = cripts::Time::Point{};
 
@@ -166,8 +176,19 @@ class Group
       if (std::filesystem::exists(_base_dir)) {
         _base_dir += "/cache_groups";
         if (!std::filesystem::exists(_base_dir)) {
-          std::filesystem::create_directories(_base_dir);
-          std::filesystem::permissions(_base_dir, std::filesystem::perms::group_write, std::filesystem::perm_options::add);
+          std::error_code ec;
+
+          std::filesystem::create_directories(_base_dir, ec);
+          if (ec) {
+            TSError("cripts::Cache::Group::Manager: Failed to create directory `%s': %s", _base_dir.c_str(), ec.message().c_str());
+          } else {
+            std::filesystem::permissions(_base_dir, std::filesystem::perms::group_write, std::filesystem::perm_options::add, ec);
+
+            if (ec) {
+              TSWarning("cripts::Cache::Group::Manager: Failed to set permissions on `%s': %s", _base_dir.c_str(),
+                        ec.message().c_str());
+            }
+          }
         }
       }
       _scheduleCont(); // Kick it off

include/cripts/Matcher.hpp

@@ -23,7 +23,6 @@
 #include <algorithm>
 #include <vector>
 #include <tuple>
-#include <algorithm>
 
 #include "cripts/Headers.hpp"
 #include "cripts/Lulu.hpp"

src/cripts/CacheGroup.cc

@@ -17,7 +17,6 @@
 */
 
 #include <algorithm>
-#include <iostream>
 #include <filesystem>
 
 #include "ts/ts.h"
@@ -47,14 +46,15 @@ _cripts_cache_group_sync(TSCont cont, TSEvent /* event */, void * /* edata */)
   size_t       processed      = 0;
   auto         now            = cripts::Time::Clock::now();
 
-  for (auto it = groups.begin(); it != groups.end() && processed < max_to_process; ++it) {
+  for (auto it = groups.begin(); it != groups.end() && processed < max_to_process;) {
     if (auto group = it->second.lock()) {
       if (group->LastSync() + std::chrono::seconds{_SYNC_GROUP_EVERY} < now) {
         group->WriteToDisk();
         ++processed;
       }
+      ++it;
     } else {
-      // The group has been deleted, remove it from the map ??
+      // The group has been deleted, remove it from the map
       it = groups.erase(it);
     }
   }
@@ -81,8 +81,17 @@ Cache::Group::Initialize(const std::string &name, const std::string &base_dir, s
   _log_path = _base_dir + "/" + "txn.log";
 
   if (!std::filesystem::exists(_base_dir)) {
-    std::filesystem::create_directories(_base_dir);
-    std::filesystem::permissions(_base_dir, std::filesystem::perms::group_write, std::filesystem::perm_options::add);
+    std::error_code ec;
+
+    std::filesystem::create_directories(_base_dir, ec);
+    if (ec) {
+      TSWarning("cripts::Cache::Group: Failed to create directory `%s': %s", _base_dir.c_str(), ec.message().c_str());
+    } else {
+      std::filesystem::permissions(_base_dir, std::filesystem::perms::group_write, std::filesystem::perm_options::add, ec);
+      if (ec) {
+        TSWarning("cripts::Cache::Group: Failed to set permissions on `%s': %s", _base_dir.c_str(), ec.message().c_str());
+      }
+    }
   }
 
   for (size_t ix = 0; ix < _num_maps; ++ix) {
@@ -207,12 +216,9 @@ Cache::Group::LoadFromDisk()
   std::ifstream    log(_log_path, std::ios::binary);
 
   for (size_t slot_ix = 0; slot_ix < _slots.size(); ++slot_ix) {
-    auto         &slot          = _slots[slot_ix];
-    uint64_t      version_id    = 0;
-    size_t        count         = 0;
-    time_t        created_ts    = 0;
-    time_t        last_write_ts = 0;
-    time_t        last_sync_ts  = 0;
+    auto         &slot       = _slots[slot_ix];
+    uint64_t      version_id = 0;
+    _MapHeader    header{};
     std::ifstream file(slot.path, std::ios::binary);
 
     if (!file) {
@@ -226,19 +232,24 @@ Cache::Group::LoadFromDisk()
       continue;
     }
 
-    file.read(reinterpret_cast<char *>(&created_ts), sizeof(created_ts));
-    file.read(reinterpret_cast<char *>(&last_write_ts), sizeof(last_write_ts));
-    file.read(reinterpret_cast<char *>(&last_sync_ts), sizeof(last_sync_ts));
-    file.read(reinterpret_cast<char *>(&count), sizeof(count));
+    file.read(reinterpret_cast<char *>(&header), sizeof(header));
+    if (!file) {
+      TSWarning("cripts::Cache::Group: Failed to read header from map file: %s. Skipping this map.", slot.path.c_str());
+      continue;
+    }
 
-    slot.created    = cripts::Time::Clock::from_time_t(created_ts);
-    slot.last_write = cripts::Time::Clock::from_time_t(last_write_ts);
-    slot.last_sync  = cripts::Time::Clock::from_time_t(last_sync_ts);
+    slot.created    = cripts::Time::Clock::from_time_t(header.created_ts);
+    slot.last_write = cripts::Time::Clock::from_time_t(header.last_write_ts);
+    slot.last_sync  = cripts::Time::Clock::from_time_t(header.last_sync_ts);
 
-    for (size_t i = 0; i < count; ++i) {
+    for (size_t i = 0; i < header.count; ++i) {
       Cache::Group::_Entry entry;
 
       file.read(reinterpret_cast<char *>(&entry), sizeof(entry));
+      if (!file) {
+        TSWarning("cripts::Cache::Group: Failed to read entry %zu from map file: %s. Stopping entry load.", i, slot.path.c_str());
+        break;
+      }
       if (!std::ranges::any_of(_slots, [&](const auto &slot) { return slot.map->find(entry.hash) != slot.map->end(); })) {
         slot.map->insert_or_assign(entry.hash, entry);
       }
@@ -286,7 +297,7 @@ Cache::Group::WriteToDisk()
 
 //
 // Here comes the private member methods, these must never be called without
-// already hodling an exclusive lock on the mutex.
+// already holding an exclusive lock on the mutex.
 //
 
 void
@@ -309,48 +320,59 @@ Cache::Group::syncMap(size_t index)
 {
   constexpr size_t                   BUFFER_SIZE = 64 * 1024;
   std::array<std::byte, BUFFER_SIZE> buffer;
-  size_t                             buf_pos  = 0;
-  const auto                        &slot     = _slots[index];
-  const std::string                  tmp_path = slot.path + ".tmp";
+  size_t                             buf_pos      = 0;
+  bool                               write_failed = false;
+  const auto                        &slot         = _slots[index];
+  const std::string                  tmp_path     = slot.path + ".tmp";
   std::ofstream                      o_file(tmp_path, std::ios::binary | std::ios::trunc);
 
   if (!o_file) {
-    std::cerr << "Failed to open temp file for sync: " << tmp_path << "\n";
+    TSWarning("cripts::Cache::Group: Failed to open temp file for sync: %s.", tmp_path.c_str());
     return;
   }
 
   // Helper lambda to append data to the write buffer
   auto _AppendToBuffer = [&](const void *data, size_t size) {
+    if (write_failed) {
+      return;
+    }
     if (buf_pos + size > buffer.size()) {
       o_file.write(reinterpret_cast<const char *>(buffer.data()), buf_pos);
+      if (!o_file) {
+        write_failed = true;
+        return;
+      }
       buf_pos = 0;
     }
     std::memcpy(buffer.data() + buf_pos, static_cast<const std::byte *>(data), size);
     buf_pos += size;
   };
 
-  time_t created_ts    = cripts::Time::Clock::to_time_t(slot.created);
-  time_t last_write_ts = cripts::Time::Clock::to_time_t(slot.last_write);
-  time_t last_sync_ts  = cripts::Time::Clock::to_time_t(slot.last_sync);
-  size_t count         = slot.map->size();
+  _MapHeader header{.created_ts    = cripts::Time::Clock::to_time_t(slot.created),
+                    .last_write_ts = cripts::Time::Clock::to_time_t(slot.last_write),
+                    .last_sync_ts  = cripts::Time::Clock::to_time_t(slot.last_sync),
+                    .count         = slot.map->size()};
 
   _AppendToBuffer(&VERSION, sizeof(VERSION));
-  _AppendToBuffer(&created_ts, sizeof(created_ts));
-  _AppendToBuffer(&last_write_ts, sizeof(last_write_ts));
-  _AppendToBuffer(&last_sync_ts, sizeof(last_sync_ts));
-  _AppendToBuffer(&count, sizeof(count));
+  _AppendToBuffer(&header, sizeof(header));
 
   // Write entries
   for (const auto &[_, entry] : *slot.map) {
     _AppendToBuffer(&entry, sizeof(entry));
   }
 
-  if (buf_pos > 0) {
+  if (buf_pos > 0 && !write_failed) {
     o_file.write(reinterpret_cast<const char *>(buffer.data()), buf_pos);
   }
   o_file.flush();
   o_file.close();
 
+  if (write_failed || !o_file) {
+    TSWarning("cripts::Cache::Group: Failed to write to temp file `%s'.", tmp_path.c_str());
+    std::filesystem::remove(tmp_path);
+    return;
+  }
+
   if (std::rename(tmp_path.c_str(), slot.path.c_str()) != 0) {
     TSWarning("cripts::Cache::Group: Failed to rename temp file `%s' to `%s'.", tmp_path.c_str(), slot.path.c_str());
     std::filesystem::remove(tmp_path);