Address review comments

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

Co-authored-by: Klaim <Klaim@users.noreply.github.com>

Author: jjerphan

libmamba/include/mamba/core/shard_index_loader.hpp

@@ -7,7 +7,9 @@
 #ifndef MAMBA_CORE_SHARD_INDEX_LOADER_HPP
 #define MAMBA_CORE_SHARD_INDEX_LOADER_HPP
 
+#include <cstdint>
 #include <optional>
+#include <string_view>
 
 #include "mamba/core/error_handling.hpp"
 #include "mamba/core/shard_types.hpp"
@@ -21,6 +23,14 @@
 
 namespace mamba
 {
+    /**
+     * Compute shard index cache age in seconds from the file modification time.
+     *
+     * On failure (e.g. file missing or inaccessible), logs at debug level and returns nullopt.
+     */
+    std::optional<std::int64_t>
+    shard_index_cache_age_seconds(const fs::u8path& cache_path, std::string_view subdir_name);
+
     /**
      * Fetch and parse shard index from repodata_shards.msgpack.zst.
      *

libmamba/src/core/shard_index_loader.cpp

@@ -5,6 +5,7 @@
 // The full license is in the file LICENSE, distributed with this software.
 
 #include <chrono>
+#include <cstdint>
 #include <fstream>
 #include <iostream>
 #include <map>
@@ -286,6 +287,27 @@ namespace
 
 namespace mamba
 {
+    std::optional<std::int64_t>
+    shard_index_cache_age_seconds(const fs::u8path& cache_path, std::string_view subdir_name)
+    {
+        try
+        {
+            const fs::file_time_type last_write = fs::last_write_time(cache_path);
+            const fs::file_time_type now = fs::file_time_type::clock::now();
+            const std::int64_t age_sec = std::chrono::duration_cast<std::chrono::seconds>(
+                                             now - last_write
+            )
+                                             .count();
+            return age_sec;
+        }
+        catch (const std::exception& e)
+        {
+            LOG_DEBUG << "Could not check shard index cache age for " << subdir_name << ": "
+                      << e.what();
+            return std::nullopt;
+        }
+    }
+
     auto ShardIndexLoader::shard_index_cache_path(const SubdirIndexLoader& subdir) -> fs::u8path
     {
         // Use similar naming as repodata.json cache but with .msgpack.zst extension
@@ -510,18 +532,15 @@ namespace mamba
         fs::u8path cache_path = shard_index_cache_path(subdir);
         if (fs::exists(cache_path) && shards_ttl > 0)
         {
-            try
+            if (auto age_sec = shard_index_cache_age_seconds(cache_path, subdir.name()))
             {
-                auto last_write = fs::last_write_time(cache_path);
-                auto now = fs::file_time_type::clock::now();
-                auto age = std::chrono::duration_cast<std::chrono::seconds>(now - last_write).count();
-                if (age >= 0 && static_cast<std::size_t>(age) <= shards_ttl)
+                if (*age_sec >= 0 && static_cast<std::size_t>(*age_sec) <= shards_ttl)
                 {
                     auto cached_index = parse_shard_index(cache_path);
                     if (cached_index.has_value())
                     {
                         LOG_DEBUG << "Using cached shard index for " << subdir.name()
-                                  << " (within TTL, " << age << "s old)";
+                                  << " (within TTL, " << *age_sec << "s old)";
                         if (Console::can_report_status())
                         {
                             std::string label = "Using Cached Shard Index for " + subdir.name();
@@ -535,10 +554,6 @@ namespace mamba
                     }
                 }
             }
-            catch (const std::exception& e)
-            {
-                LOG_DEBUG << "Could not check cache age for " << subdir.name() << ": " << e.what();
-            }
         }
 
         // Refresh shards availability (HEAD check) if metadata is stale for TTL, then re-check.

libmamba/src/core/subdir_index.cpp

@@ -527,22 +527,15 @@ namespace mamba
         {
             return;
         }
-        try
+        if (auto age_sec = shard_index_cache_age_seconds(cache_path, name()))
         {
-            auto last_write = fs::last_write_time(cache_path);
-            auto now = fs::file_time_type::clock::now();
-            auto age = std::chrono::duration_cast<std::chrono::seconds>(now - last_write).count();
-            if (age >= 0 && static_cast<std::size_t>(age) <= params.repodata_shards_ttl)
+            if (*age_sec >= 0 && static_cast<std::size_t>(*age_sec) <= params.repodata_shards_ttl)
             {
-                LOG_DEBUG << "Shard index cache valid for " << name() << " (" << age << "s old), "
-                          << "skipping full repodata download";
+                LOG_DEBUG << "Shard index cache valid for " << name() << " (" << *age_sec
+                          << "s old), skipping full repodata download";
                 set_shards_availability(true);
             }
         }
-        catch (const std::exception& e)
-        {
-            LOG_DEBUG << "Could not check shard index cache age for " << name() << ": " << e.what();
-        }
     }
 
     void SubdirIndexLoader::clear_valid_cache_files()
@@ -876,16 +869,13 @@ namespace mamba
                 fs::u8path cache_path = ShardIndexLoader::shard_index_cache_path(*this);
                 if (fs::exists(cache_path))
                 {
-                    try
+                    if (auto age_sec = shard_index_cache_age_seconds(cache_path, name()))
                     {
-                        auto last_write = fs::last_write_time(cache_path);
-                        auto now = fs::file_time_type::clock::now();
-                        auto age = std::chrono::duration_cast<std::chrono::seconds>(now - last_write)
-                                       .count();
-                        if (age >= 0 && static_cast<std::size_t>(age) <= params.repodata_shards_ttl)
+                        if (*age_sec >= 0
+                            && static_cast<std::size_t>(*age_sec) <= params.repodata_shards_ttl)
                         {
                             LOG_DEBUG << "Skipping shards HEAD check for " << name()
-                                      << " (cached index within TTL, " << age << "s old)";
+                                      << " (cached index within TTL, " << *age_sec << "s old)";
                             // Mark shards as available so load_subdir_with_shards is used
                             set_shards_availability(true);
                         }
@@ -896,10 +886,8 @@ namespace mamba
                             );
                         }
                     }
-                    catch (const std::exception& e)
+                    else
                     {
-                        LOG_DEBUG << "Could not check shard index cache age for " << name() << ": "
-                                  << e.what();
                         request.push_back(
                             ShardIndexLoader::build_shards_availability_check_request(*this)
                         );

libmamba/tests/src/core/test_sharded_repodata_integration.cpp

@@ -967,10 +967,7 @@ TEST_CASE(
     "[mamba::core][sharded][.integration][!mayfail]"
 )
 {
-    auto& ctx = mambatests::context();
-    const bool original_offline = ctx.offline;
-    on_scope_exit restore_offline{ [&] { ctx.offline = original_offline; } };
-
+    Context ctx;
     ctx.channels = { "https://prefix.dev/conda-forge" };
     ctx.repodata_use_shards = true;