fix(plugin-catalog): preserve Unicode paths in failure diagnostics

CHANGELOG.md

@@ -101,6 +101,20 @@ SceneEntities and PlotMarkers:
 - `PlotMarkers`: the decoder drops a marker whose `kind` it does not know instead of drawing it as
   a time region; wire numbers unchanged.
 
+Plugin catalog and source cache, errors that threw instead of being returned:
+
+- On Windows, a plugin path that the active code page cannot represent no longer makes
+  `inspectPluginDso` or `scanPluginDsos` throw. They formatted their own error text with
+  `path.string()`, and the scan checked each file's suffix the same way, so one such file anywhere
+  under the plugin folder made the whole scan throw. Paths in these messages are now UTF-8 on
+  Windows whatever the code page; a consumer that decoded them as the local code page must decode
+  UTF-8.
+- `scanPluginDsos` reports a subdirectory it cannot read and goes on with the rest. It threw
+  (`recursive_directory_iterator cannot pop`) on the first unreadable directory, on every platform.
+- The source request cache has the same Windows fix for its lock and flush errors. "Lock is held
+  elsewhere", an ordinary outcome during cleanup and eviction, threw under a cache root the code
+  page cannot represent.
+
 Removed:
 
 - **Breaking (source):** the `pj.colormap.v1` service: `PJ_colormap_registry_vtable_t`,

pj_base/src/source/file_lock.cpp

@@ -6,6 +6,8 @@
 #include <system_error>
 #include <utility>
 
+#include "source/path_utf8.hpp"
+
 #if defined(_WIN32)
 // windows.h min/max macros break std::max / numeric_limits<>::max.
 #ifndef NOMINMAX
@@ -46,7 +48,7 @@ std::optional<std::intptr_t> tryAcquireHandle(
       FILE_ATTRIBUTE_NORMAL, nullptr);
   if (handle == INVALID_HANDLE_VALUE) {
     if (error != nullptr) {
-      *error = "could not open lock file " + path.string() + " (error " + std::to_string(::GetLastError()) + ")";
+      *error = "could not open lock file " + pathUtf8(path) + " (error " + std::to_string(::GetLastError()) + ")";
     }
     return std::nullopt;
   }
@@ -58,7 +60,7 @@ std::optional<std::intptr_t> tryAcquireHandle(
       *contended = true;  // FAIL_IMMEDIATELY: the region is held elsewhere
     }
     if (error != nullptr) {
-      *error = "lock is held elsewhere: " + path.string();
+      *error = "lock is held elsewhere: " + pathUtf8(path);
     }
     return std::nullopt;
   }
@@ -67,7 +69,7 @@ std::optional<std::intptr_t> tryAcquireHandle(
   const int fd = ::open(path.c_str(), O_RDWR | O_CREAT | O_CLOEXEC, 0600);
   if (fd < 0) {
     if (error != nullptr) {
-      *error = "could not open lock file " + path.string() + ": " +
+      *error = "could not open lock file " + pathUtf8(path) + ": " +
                std::error_code(errno, std::generic_category()).message();
     }
     return std::nullopt;
@@ -80,8 +82,8 @@ std::optional<std::intptr_t> tryAcquireHandle(
       *contended = held_elsewhere;
     }
     if (error != nullptr) {
-      *error = held_elsewhere ? "lock is held elsewhere: " + path.string()
-                              : "flock failed on " + path.string() + ": " +
+      *error = held_elsewhere ? "lock is held elsewhere: " + pathUtf8(path)
+                              : "flock failed on " + pathUtf8(path) + ": " +
                                     std::error_code(flock_errno, std::generic_category()).message();
     }
     return std::nullopt;

pj_base/src/source/fs_durability.cpp

@@ -6,6 +6,8 @@
 #include <cerrno>
 #include <system_error>
 
+#include "source/path_utf8.hpp"
+
 #if defined(_WIN32)
 // windows.h min/max macros break std::max / numeric_limits<>::max.
 #ifndef NOMINMAX
@@ -44,21 +46,21 @@ bool syncFile(const fs::path& file, std::string* error) {
       file.c_str(), GENERIC_WRITE, FILE_SHARE_READ, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr);
   if (handle == INVALID_HANDLE_VALUE) {
     if (error != nullptr) {
-      *error = "could not reopen for flush: " + file.string();
+      *error = "could not reopen for flush: " + pathUtf8(file);
     }
     return false;
   }
   const bool ok = ::FlushFileBuffers(handle) != 0;
   ::CloseHandle(handle);
   if (!ok && error != nullptr) {
-    *error = "FlushFileBuffers failed: " + file.string();
+    *error = "FlushFileBuffers failed: " + pathUtf8(file);
   }
   return ok;
 #else
   const int fd = ::open(file.c_str(), O_RDONLY | O_CLOEXEC);
   if (fd < 0) {
     if (error != nullptr) {
-      *error = "could not reopen for fsync: " + file.string() + ": " +
+      *error = "could not reopen for fsync: " + pathUtf8(file) + ": " +
                std::error_code(errno, std::generic_category()).message();
     }
     return false;
@@ -67,7 +69,7 @@ bool syncFile(const fs::path& file, std::string* error) {
   const int fsync_errno = errno;
   ::close(fd);
   if (!ok && error != nullptr) {
-    *error = "fsync failed: " + file.string() + ": " + std::error_code(fsync_errno, std::generic_category()).message();
+    *error = "fsync failed: " + pathUtf8(file) + ": " + std::error_code(fsync_errno, std::generic_category()).message();
   }
   return ok;
 #endif

pj_base/src/source/path_utf8.hpp

@@ -0,0 +1,27 @@
+#pragma once
+// Copyright 2026 Davide Faconti
+// SPDX-License-Identifier: Apache-2.0
+
+#include <filesystem>
+#include <string>
+
+namespace PJ {
+namespace sdk {
+namespace source {
+namespace detail {
+
+/// The path as UTF-8, for error text and name checks.
+///
+/// Use this instead of `path.string()`: on Windows that narrows through the
+/// active code page and throws `std::system_error` for a name the code page
+/// cannot represent, so a function meant to return an error throws instead.
+/// On POSIX the result is the same bytes `string()` returns.
+[[nodiscard]] inline std::string pathUtf8(const std::filesystem::path& path) {
+  const std::u8string utf8 = path.u8string();
+  return {utf8.begin(), utf8.end()};
+}
+
+}  // namespace detail
+}  // namespace source
+}  // namespace sdk
+}  // namespace PJ

pj_base/src/source/request_cache.cpp

@@ -11,6 +11,7 @@
 #include "pj_base/sdk/testing/request_cache_probe.hpp"
 #include "source/file_lock.hpp"
 #include "source/fs_durability.hpp"
+#include "source/path_utf8.hpp"
 
 #if defined(_WIN32)
 // windows.h min/max macros break std::max / numeric_limits<>::max.
@@ -85,10 +86,9 @@ class CacheLayout {
   /// Managed names only: "<digest><suffix>" and "<digest><suffix>.partial.<pid>";
   /// sidecars, foreign files and malformed digests are kOther.
   [[nodiscard]] Classified classify(const fs::path& file) const {
-    // u8string(): a foreign Unicode name classifies as kOther instead of
+    // pathUtf8(): a foreign Unicode name classifies as kOther instead of
     // throwing through .string() on a non-ACP Windows code page.
-    const std::u8string u8name = file.filename().u8string();
-    const std::string name(u8name.begin(), u8name.end());
+    const std::string name = detail::pathUtf8(file.filename());
     const std::size_t hex = spec_.identity.hexChars();
     const auto digestOk = [&name, hex] {
       return name.size() >= hex && std::all_of(
@@ -238,7 +238,7 @@ Expected<RequestArtifactCache::Hit, CacheError> RequestArtifactCache::WriteTrans
     impl.lock.reset();
     return unexpected(CacheError{std::move(reason)});
   };
-  const std::string what = "cache commit rejected " + impl.partial.filename().string() + ": ";
+  const std::string what = "cache commit rejected " + detail::pathUtf8(impl.partial.filename()) + ": ";
   if (!impl.validator) {
     return fail(what + "no artifact validator configured");
   }

pj_base/tests/descriptor_import_request_cache_test.cpp

@@ -434,6 +434,23 @@ TEST_F(RequestCacheTest, UnicodeCacheRootRoundTripsAndCleans) {
   EXPECT_FALSE(fs::exists(ucache.pathFor(identityFor(kHexA))));
 }
 
+TEST_F(RequestCacheTest, UnicodeCacheRootReportsContentionInsteadOfThrowing) {
+  // "Lock is held elsewhere" is an ordinary outcome, and its message names the
+  // lock file. On Windows that name went through path.string(), which throws
+  // for a root the active code page cannot represent.
+  const fs::path uroot = root() / fs::path(u8"пж-缓存-ü");
+  fs::create_directories(uroot);
+  RequestArtifactCache ucache(CacheSpec{uroot, kSuffix, IdentityScheme{kPrefix, 32}}, acceptAll());
+  auto first = ucache.beginWrite(identityFor(kHexA));
+  ASSERT_TRUE(first) << first.error().message;
+  auto second = ucache.beginWrite(identityFor(kHexA));
+  ASSERT_FALSE(second);
+  EXPECT_TRUE(second.error().retryable);
+  const std::u8string u8root = uroot.filename().u8string();
+  EXPECT_NE(second.error().message.find(std::string(u8root.begin(), u8root.end())), std::string::npos)
+      << second.error().message;
+}
+
 TEST_F(RequestCacheTest, OutOfRangeDigestWidthStillRoundTrips) {
   // The spec's scheme normalizes the width exactly like identity minting, so
   // a provider that misconfigures digest_hex_chars still gets a coherent

pj_plugins/src/plugin_catalog.cpp

@@ -43,8 +43,10 @@ struct ManifestCandidate {
   std::string manifest_json;
 };
 
+// Compared as paths, so no file name is ever narrowed: a name that cannot be
+// converted must not make the scan throw.
 bool hasDsoSuffix(const std::filesystem::path& path) {
-  return path.extension().string() == kDsoSuffix;
+  return path.extension() == std::filesystem::path(kDsoSuffix);
 }
 
 bool nativeSymbolPresent(void* handle, const char* symbol) noexcept {
@@ -402,23 +404,25 @@ std::string_view toString(PluginFamily family) noexcept {
 
 Expected<PluginDescriptor> inspectPluginDso(const std::filesystem::path& dso_path) {
   if (!hasDsoSuffix(dso_path)) {
-    return unexpected(fmt::format("not a platform plugin DSO: {}", dso_path.string()));
+    return unexpected(fmt::format("not a platform plugin DSO: {}", detail::pathForLegacyAccessor(dso_path)));
   }
 
   detail::LibraryPathIdentity recorded_path;
   auto raw_handle = detail::loadLibraryHandle(dso_path, &recorded_path);
   if (!raw_handle) {
-    return unexpected(fmt::format("{}: {}", dso_path.string(), raw_handle.error()));
+    return unexpected(fmt::format("{}: {}", detail::pathForLegacyAccessor(dso_path), raw_handle.error()));
   }
   return inspectPluginDso(detail::adoptLibraryHandle(*raw_handle), recorded_path.load_path);
 }
 
 Expected<PluginDescriptor> inspectPluginDso(
     const std::shared_ptr<void>& handle, const std::filesystem::path& dso_path) {
   if (!hasDsoSuffix(dso_path)) {
-    return unexpected(fmt::format("not a platform plugin DSO: {}", dso_path.string()));
+    return unexpected(fmt::format("not a platform plugin DSO: {}", detail::pathForLegacyAccessor(dso_path)));
   }
-  auto with_path = [&](const std::string& error) { return fmt::format("{}: {}", dso_path.string(), error); };
+  auto with_path = [&](const std::string& error) {
+    return fmt::format("{}: {}", detail::pathForLegacyAccessor(dso_path), error);
+  };
   if (handle == nullptr) {
     return unexpected(with_path("library not loaded"));
   }
@@ -487,10 +491,10 @@ Expected<std::vector<PluginFamily>> exportedPluginFamilies(
 Expected<PluginScanResult> scanPluginDsos(const std::filesystem::path& directory) {
   std::error_code ec;
   if (!std::filesystem::exists(directory, ec)) {
-    return unexpected(fmt::format("plugin directory not found: {}", directory.string()));
+    return unexpected(fmt::format("plugin directory not found: {}", detail::pathForLegacyAccessor(directory)));
   }
   if (!std::filesystem::is_directory(directory, ec)) {
-    return unexpected(fmt::format("plugin path is not a directory: {}", directory.string()));
+    return unexpected(fmt::format("plugin path is not a directory: {}", detail::pathForLegacyAccessor(directory)));
   }
 
   PluginScanResult result;
@@ -518,12 +522,25 @@ Expected<PluginScanResult> scanPluginDsos(const std::filesystem::path& directory
       result.diagnostics.push_back({entry.path(), fmt::format("stat failed: {}", entry_ec.message())});
     }
 
+    // increment() descends into a directory entry, and when that fails the
+    // iterator is left at the end: the scan is over. So a directory that
+    // cannot be opened is found here, reported, and not descended into.
+    std::error_code type_ec;
+    if (it.recursion_pending() && entry.is_directory(type_ec) && !entry.is_symlink(type_ec)) {
+      std::error_code open_ec;
+      const std::filesystem::directory_iterator probe(entry.path(), open_ec);
+      if (open_ec) {
+        result.diagnostics.push_back({entry.path(), fmt::format("directory iteration failed: {}", open_ec.message())});
+        it.disable_recursion_pending();
+      }
+    }
+
     std::error_code inc_ec;
     it.increment(inc_ec);
     if (inc_ec) {
+      // The iterator is at the end now; pop() on it would throw.
       result.diagnostics.push_back({entry.path(), fmt::format("directory iteration failed: {}", inc_ec.message())});
-      // Skip the unreadable subtree but continue with the rest of the scan.
-      it.pop();
+      break;
     }
   }
 

pj_plugins/tests/plugin_catalog_test.cpp

@@ -12,6 +12,7 @@
 #include <optional>
 #include <string>
 #include <string_view>
+#include <system_error>
 #include <utility>
 
 #include "detail/library_loader.hpp"
@@ -25,6 +26,8 @@
 #define WIN32_LEAN_AND_MEAN
 #endif
 #include <windows.h>
+#else
+#include <unistd.h>
 #endif
 
 namespace PJ {
@@ -68,6 +71,58 @@ class PluginCatalogTest : public ::testing::Test {
   std::filesystem::path dir_;
 };
 
+// Unicode errors must be UTF-8 on every Windows ACP, not only UTF-8 systems.
+TEST_F(PluginCatalogTest, UnicodeFailureDiagnosticsPreservePath) {
+  const auto directory = dir_ / std::filesystem::path(u8"\u8cc7\u6599-\U0001f680");
+  const auto utf8 = directory.u8string();
+  const std::string expected(utf8.begin(), utf8.end());
+  auto missing = scanPluginDsos(directory);
+  ASSERT_FALSE(missing.has_value());
+  EXPECT_NE(missing.error().find(expected), std::string::npos);
+  // Some filesystems cannot store this name (no supplementary-plane characters,
+  // or a non-Unicode encoding). That is not a catalog failure: skip the cases
+  // that need the directory, but still fail on any other I/O error.
+  std::error_code created;
+  std::filesystem::create_directories(directory, created);
+  if (created == std::errc::illegal_byte_sequence || created == std::errc::invalid_argument) {
+    GTEST_SKIP() << "filesystem cannot store the Unicode test name: " << created.message();
+  }
+  ASSERT_FALSE(created) << created.message();
+  const auto broken = directory / pluginFileName("broken");
+  {
+    std::ofstream output(broken);
+    ASSERT_TRUE(output.is_open());
+    output << "not a shared library";
+    ASSERT_TRUE(output.good());
+  }
+  auto invalid = inspectPluginDso(broken);
+  ASSERT_FALSE(invalid.has_value());
+  EXPECT_NE(invalid.error().find(expected), std::string::npos);
+  auto unopened = inspectPluginDso(std::shared_ptr<void>{}, broken);
+  ASSERT_FALSE(unopened.has_value());
+  EXPECT_NE(unopened.error().find(expected), std::string::npos);
+  // A suffix made of a supplementary-plane character is refused as "not a
+  // plugin", by both overloads, before anything is loaded.
+  const auto not_a_plugin = directory / std::filesystem::path(u8"file.\U0001f680");
+  auto unsupported = inspectPluginDso(not_a_plugin);
+  ASSERT_FALSE(unsupported.has_value());
+  EXPECT_NE(unsupported.error().find("not a platform plugin DSO"), std::string::npos) << unsupported.error();
+  EXPECT_NE(unsupported.error().find(expected), std::string::npos);
+  auto unsupported_handle = inspectPluginDso(std::shared_ptr<void>{}, not_a_plugin);
+  ASSERT_FALSE(unsupported_handle.has_value());
+  EXPECT_NE(unsupported_handle.error().find("not a platform plugin DSO"), std::string::npos)
+      << unsupported_handle.error();
+  EXPECT_NE(unsupported_handle.error().find(expected), std::string::npos);
+  auto scan = scanPluginDsos(directory);
+  ASSERT_TRUE(scan.has_value()) << scan.error();
+  EXPECT_TRUE(scan->plugins.empty());
+  ASSERT_EQ(scan->diagnostics.size(), 1u);
+  EXPECT_NE(scan->diagnostics.front().message.find(expected), std::string::npos);
+  auto not_directory = scanPluginDsos(broken);
+  ASSERT_FALSE(not_directory.has_value());
+  EXPECT_NE(not_directory.error().find(expected), std::string::npos);
+}
+
 TEST_F(PluginCatalogTest, MissingDirectoryReturnsError) {
   auto result = scanPluginDsos("/nonexistent/path/xyz");
   EXPECT_FALSE(result.has_value());
@@ -184,6 +239,42 @@ TEST_F(PluginCatalogTest, ForwardedEntryPointIsRejected) {
 }
 #endif
 
+#if !defined(_WIN32)
+// A subdirectory the scan cannot read (an extension folder owned by another
+// user) is reported and skipped. It must not end the scan, and it must not
+// throw: the scan's contract is to return diagnostics.
+TEST_F(PluginCatalogTest, ScanReportsAnUnreadableSubdirectoryAndContinues) {
+  if (::geteuid() == 0) {
+    GTEST_SKIP() << "root can read every directory";
+  }
+  const std::filesystem::path locked = dir_ / "a_locked";
+  const std::filesystem::path later = dir_ / "z_later";
+  std::filesystem::create_directories(locked);
+  std::filesystem::create_directories(later);
+  std::ofstream(later / pluginFileName("broken")) << "not a shared library";
+  std::filesystem::permissions(locked, std::filesystem::perms::none);
+  struct Restore {
+    std::filesystem::path path;
+    ~Restore() {
+      std::error_code ec;  // so TearDown can remove the tree whatever the test did
+      std::filesystem::permissions(path, std::filesystem::perms::owner_all, ec);
+    }
+  } restore{locked};
+
+  auto scan = scanPluginDsos(dir_);
+  ASSERT_TRUE(scan.has_value()) << scan.error();
+  EXPECT_TRUE(scan->plugins.empty());
+  bool reported_locked = false;
+  bool reached_later = false;
+  for (const auto& diagnostic : scan->diagnostics) {
+    reported_locked = reported_locked || diagnostic.path == locked;
+    reached_later = reached_later || diagnostic.path == later / pluginFileName("broken");
+  }
+  EXPECT_TRUE(reported_locked) << "the unreadable directory is named in a diagnostic";
+  EXPECT_TRUE(reached_later) << "the scan went on to the entries after it";
+}
+#endif
+
 TEST_F(PluginCatalogTest, UnicodeExtensionPathLoadsOnWindows) {
   const std::filesystem::path unicode_dir = dir_ / std::filesystem::path(u8"插件-π");
   std::filesystem::create_directories(unicode_dir);