Fall Back When a Replace Is Refused and Sweep Only Old Temporaries

Some mounts refuse to replace an existing file in one rename, and the
old remove-then-rename worked there where the atomic path now fails, so
rename_file() takes the two-step route when a one-step replace is
refused and write_file_atomically() writes in place whenever the rename
fails, not only for the Windows sharing case.

An instance that gave up waiting for the lock writes unlocked by design,
so a peer holding the lock could sweep its in-flight temporary and make
its rename fail; only temporaries older than ten minutes are removed now,
and the sweep uses the error-code overloads so an entry vanishing between
listing and stat cannot throw out of startup. The registry mutex is
leaked like the map it guards, so a save during static destruction does
not lock a destroyed mutex.

The physical printer loader is never called, so its guard is gone, while
the two delete paths that do run now hold the lock. The bundle metadata
loader is lock-free again, since the zip import reads it from a scratch
folder; the guard sits at the two scans that read the user's bundles.
Preset::save() builds what it writes before taking the lock, the cache
writer reserves its payload, and the retry test tolerates a slow runner.

src/libslic3r/InstanceLock.cpp

@@ -34,12 +34,12 @@ struct InstanceLock::Slot
 
 InstanceLock::Slot &InstanceLock::slot_for(const std::string &lock_file_path)
 {
-    static std::mutex registry_mutex;
     // Never freed: the slots keep the lock files open for as long as anything
     // in the process may still save, including during static destruction.
-    static auto *registry = new std::map<std::string, std::unique_ptr<Slot>>();
+    static auto *registry_mutex = new std::mutex();
+    static auto *registry       = new std::map<std::string, std::unique_ptr<Slot>>();
 
-    std::lock_guard<std::mutex> guard(registry_mutex);
+    std::lock_guard<std::mutex> guard(*registry_mutex);
     std::unique_ptr<Slot> &slot = (*registry)[lock_file_path];
     if (! slot)
         slot = std::make_unique<Slot>();

src/libslic3r/Preset.cpp

@@ -713,13 +713,16 @@ void Preset::save(DynamicPrintConfig* parent_config)
     else
         from_str = std::string("Default");
 
-    InstanceLock instance_lock(user_presets_lock_path());
-    boost::filesystem::create_directories(fs::path(this->file).parent_path());
     const std::string bare_name = get_preset_bare_name(this->name);
 
+    // What gets written: the diff against the parent, the config plus its
+    // filament id, or the config as is. Built before the lock is taken so the
+    // exclusive window covers only the file writes.
+    DynamicPrintConfig        temp_config;
+    const DynamicPrintConfig *to_save = &this->config;
+
     //BBS: only save difference if it has parent
     if (parent_config) {
-        DynamicPrintConfig temp_config;
         std::vector<std::string> dirty_options = config.diff(*parent_config);
 
         std::string extruder_id_name, extruder_variant_name;
@@ -755,14 +758,16 @@ void Preset::save(DynamicPrintConfig* parent_config)
                     opt_dst->set(opt_src);
             }
         }
-        temp_config.save_to_json(this->file, bare_name, from_str, this->version.to_string());
+        to_save = &temp_config;
     } else if (!filament_id.empty() && inherits().empty()) {
-        DynamicPrintConfig temp_config = config;
+        temp_config = config;
         temp_config.set_key_value(BBL_JSON_KEY_FILAMENT_ID, new ConfigOptionString(filament_id));
-        temp_config.save_to_json(this->file, bare_name, from_str, this->version.to_string());
-    } else {
-        this->config.save_to_json(this->file, bare_name, from_str, this->version.to_string());
+        to_save = &temp_config;
     }
+
+    InstanceLock instance_lock(user_presets_lock_path());
+    boost::filesystem::create_directories(fs::path(this->file).parent_path());
+    to_save->save_to_json(this->file, bare_name, from_str, this->version.to_string());
     BOOST_LOG_TRIVIAL(info) << __FUNCTION__ << " save config for: " << this->name << " and filament_id: " << filament_id << " and base_id: " << this->base_id;
 
     // Bundle presets are synced via bundle_id and don't need individual .info files.
@@ -4345,7 +4350,6 @@ void PhysicalPrinterCollection::load_printers(
     std::string errors_cummulative;
     // Store the loaded printers into a new vector, otherwise the binary search for already existing presets would be broken.
     std::deque<PhysicalPrinter> printers_loaded;
-    InstanceLock instance_lock(user_presets_lock_path());
     //BBS: change to json format
     for (auto& dir_entry : boost::filesystem::directory_iterator(dir))
     {
@@ -4553,7 +4557,10 @@ bool PhysicalPrinterCollection::delete_printer(const std::string& name)
 
     const PhysicalPrinter& printer = *it;
     // Erase the preset file.
-    boost::nowide::remove(printer.file.c_str());
+    {
+        InstanceLock instance_lock(user_presets_lock_path());
+        boost::nowide::remove(printer.file.c_str());
+    }
     m_printers.erase(it);
     return true;
 }
@@ -4565,7 +4572,10 @@ bool PhysicalPrinterCollection::delete_selected_printer()
     const PhysicalPrinter& printer = this->get_selected_printer();
 
     // Erase the preset file.
-    boost::nowide::remove(printer.file.c_str());
+    {
+        InstanceLock instance_lock(user_presets_lock_path());
+        boost::nowide::remove(printer.file.c_str());
+    }
     // Remove the preset from the list.
     m_printers.erase(m_printers.begin() + m_idx_selected);
     // unselect all printers

src/libslic3r/PresetBundle.cpp

@@ -1232,6 +1232,8 @@ PresetsConfigSubstitutions PresetBundle::load_user_presets(std::string user, For
     fs::path local_dir(folder / PRESET_LOCAL_DIR);
     if (fs::exists(local_dir)) {
         dir_user_presets_local = local_dir;
+        // Held across the metadata reads; a read-only load (the CLI) takes no lock.
+        InstanceLock instance_lock(read_only ? std::string() : user_presets_lock_path());
         for (auto& entry : fs::directory_iterator(local_dir)) {
             if (!fs::is_directory(entry.path())) continue;
 
@@ -1241,7 +1243,7 @@ PresetsConfigSubstitutions PresetBundle::load_user_presets(std::string user, For
             if (!fs::exists(metadata_file)) continue;
 
             BundleMetadata metadata;
-            if (!metadata.load_from_json(metadata_file.string(), read_only)) continue;
+            if (!metadata.load_from_json(metadata_file.string())) continue;
             metadata.print_presets.clear();
             metadata.filament_presets.clear();
             metadata.printer_presets.clear();
@@ -1267,6 +1269,7 @@ PresetsConfigSubstitutions PresetBundle::load_user_presets(std::string user, For
     // Load bundle metadata from _subscribed directory
     fs::path subscribed_dir(folder / PRESET_SUBSCRIBED_DIR);
     if (fs::exists(subscribed_dir)) {
+        InstanceLock instance_lock(read_only ? std::string() : user_presets_lock_path());
         for (auto& entry : fs::directory_iterator(subscribed_dir)) {
             if (!fs::is_directory(entry.path())) continue;
 
@@ -1276,7 +1279,7 @@ PresetsConfigSubstitutions PresetBundle::load_user_presets(std::string user, For
             if (!fs::exists(metadata_file)) continue;
 
             BundleMetadata metadata;
-            if (!metadata.load_from_json(metadata_file.string(), read_only)) continue;
+            if (!metadata.load_from_json(metadata_file.string())) continue;
             metadata.print_presets.clear();
             metadata.filament_presets.clear();
             metadata.printer_presets.clear();
@@ -7867,9 +7870,8 @@ bool PresetBundle::check_duplicate_filament_subtypes() const
 }
 
 // Orca: BundleMetadata method implementations
-bool BundleMetadata::load_from_json(const std::string& path, bool read_only)
+bool BundleMetadata::load_from_json(const std::string& path)
 {
-    InstanceLock instance_lock(read_only ? std::string() : user_presets_lock_path());
     try {
         boost::nowide::ifstream ifs(path);
         if (!ifs.good())

src/libslic3r/PresetBundle.hpp

@@ -129,8 +129,7 @@ struct BundleMetadata
     bool                            not_found{false};
     bool                            unauthorized{false};
 
-    // A read-only load (the CLI) takes no instance lock.
-    bool load_from_json(const std::string& path, bool read_only = false);
+    bool load_from_json(const std::string& path);
     bool save_to_json(const std::string& path) const;
 };
 

src/libslic3r/PresetCacheFormat.cpp

@@ -410,7 +410,9 @@ bool write_cache_blob(const std::string& path, const std::string& blob)
         fhdr.version   = CACHE_VERSION;
         fhdr.data_size = static_cast<uint64_t>(blob.size());
         fhdr.crc32     = crc.checksum();
-        std::string payload(reinterpret_cast<const char*>(&fhdr), sizeof(fhdr));
+        std::string payload;
+        payload.reserve(sizeof(fhdr) + blob.size());
+        payload.append(reinterpret_cast<const char*>(&fhdr), sizeof(fhdr));
         payload += blob;
         if (const std::error_code ec = write_file_atomically(path, payload, /*binary=*/true)) {
             BOOST_LOG_TRIVIAL(warning) << "VendorCacheFile: write failed (" << path << "): " << ec.message();

src/libslic3r/Utils.hpp

@@ -231,8 +231,9 @@ extern std::error_code rename_file(const std::string &from, const std::string &t
 // or pipe) is written in place, since replacing it would change what it is.
 extern std::error_code write_file_atomically(const std::string &path, const std::string &content, bool binary = false);
 // Remove the `<name>.<pid>.tmp` files a crashed write_file_atomically() left in
-// `dir`, restricted to names starting with `name_prefix` when given. Call it only
-// while holding the lock that guards writes into `dir`. Returns how many were removed.
+// `dir` at least ten minutes ago, restricted to names starting with `name_prefix`
+// when given. Call it only while holding the lock that guards writes into `dir`.
+// Returns how many were removed.
 extern size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::string &name_prefix = std::string());
 
 enum CopyFileResult {

src/libslic3r/utils.cpp

@@ -707,7 +707,15 @@ std::error_code rename_file(const std::string &from, const std::string &to)
 #else
 	// rename(2) replaces an existing target atomically; removing it first would
 	// leave a window in which the file does not exist at all.
-	return std::make_error_code(static_cast<std::errc>(boost::nowide::rename(from.c_str(), to.c_str()) == 0 ? 0 : errno));
+	if (boost::nowide::rename(from.c_str(), to.c_str()) == 0)
+		return {};
+	const int err = errno;
+	// Some mounts (sshfs without its rename workaround, for one) refuse to
+	// replace an existing target in one step; take the old two-step route there.
+	if ((err == EPERM || err == EEXIST || err == ENOTEMPTY) && boost::nowide::remove(to.c_str()) == 0 &&
+	    boost::nowide::rename(from.c_str(), to.c_str()) == 0)
+		return {};
+	return std::make_error_code(static_cast<std::errc>(err));
 #endif
 }
 
@@ -741,18 +749,16 @@ std::error_code write_file_atomically(const std::string &path, const std::string
 	if (target_exists)
 		boost::filesystem::permissions(tmp_path, target.permissions(), bec);
 	std::error_code ec = rename_file(tmp_path, path);
-	if (ec)
+	if (ec) {
 		boost::nowide::remove(tmp_path.c_str());
-#ifdef _WIN32
-	// Windows refuses to replace a file another process holds open without
-	// FILE_SHARE_DELETE, which is how the C runtime opens files for reading.
-	// Losing the save is worse than a reader seeing a partial file, so write
-	// in place the way this used to work before the atomic path existed.
-	if (ec == std::errc::permission_denied) {
-		BOOST_LOG_TRIVIAL(warning) << "Cannot replace " << path << " while another process holds it open; writing in place";
+		// Windows refuses to replace a file another process holds open without
+		// FILE_SHARE_DELETE, which is how the C runtime opens files for reading,
+		// and some mounts cannot replace a file in one step at all. Losing the
+		// save is worse than a reader seeing a partial file, so write in place
+		// the way this used to work before the atomic path existed.
+		BOOST_LOG_TRIVIAL(warning) << "Cannot replace " << path << " (" << ec.message() << "); writing in place";
 		ec = write_whole_file(path, content, binary);
 	}
-#endif
 	return ec;
 }
 
@@ -771,11 +777,20 @@ size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::st
 			return false;
 		return std::all_of(name.begin() + dot + 1, name.begin() + digits_end, [](char c) { return c >= '0' && c <= '9'; });
 	};
+	// An instance that gave up waiting for the lock writes unlocked by design,
+	// so a temporary this young may still be in flight; a crash leftover is old.
+	constexpr std::time_t stale_age = 10 * 60;
+	const std::time_t     now       = std::time(nullptr);
 	size_t removed = 0;
 	boost::system::error_code ec;
 	for (boost::filesystem::directory_iterator it(dir, ec), end; ! ec && it != end; it.increment(ec)) {
-		if (! boost::filesystem::is_regular_file(it->symlink_status()) || ! is_temp_name(it->path().filename().string()))
+		if (! boost::filesystem::is_regular_file(it->symlink_status(ec)) || ! is_temp_name(it->path().filename().string()))
 			continue;
+		const std::time_t written = boost::filesystem::last_write_time(it->path(), ec);
+		if (ec || now - written < stale_age) {
+			ec.clear();
+			continue;
+		}
 		if (boost::filesystem::remove(it->path(), ec)) {
 			BOOST_LOG_TRIVIAL(info) << "Removed stale temporary file " << it->path();
 			++ removed;

tests/libslic3r/test_instance_lock.cpp

@@ -67,9 +67,10 @@ TEST_CASE("InstanceLock retries a lock file it could not open once the cool-down
     ScopedTemporaryDir dir;
     const std::string  path = (dir.path() / "later" / "shared.lock").string();
     const auto         saved_cooldown = InstanceLock::cooldown;
-    InstanceLock::cooldown = 50ms;
+    InstanceLock::cooldown = 300ms;
 
     bool before_dir, during_cooldown, after_cooldown;
+    const auto started = std::chrono::steady_clock::now();
     {
         InstanceLock lock(path, 100ms);
         before_dir = lock.locked();
@@ -79,7 +80,8 @@ TEST_CASE("InstanceLock retries a lock file it could not open once the cool-down
         InstanceLock lock(path, 100ms);
         during_cooldown = lock.locked();
     }
-    std::this_thread::sleep_for(80ms);
+    const bool second_guard_inside_cooldown = std::chrono::steady_clock::now() - started < InstanceLock::cooldown;
+    std::this_thread::sleep_for(400ms);
     {
         InstanceLock lock(path, 100ms);
         after_cooldown = lock.locked();
@@ -87,7 +89,10 @@ TEST_CASE("InstanceLock retries a lock file it could not open once the cool-down
     InstanceLock::cooldown = saved_cooldown;
 
     REQUIRE_FALSE(before_dir);
-    REQUIRE_FALSE(during_cooldown);
+    // A loaded runner may take longer than the cool-down to get here; then the
+    // second guard legitimately retried, so only assert when the timing held.
+    if (second_guard_inside_cooldown)
+        REQUIRE_FALSE(during_cooldown);
     REQUIRE(after_cooldown);
 }
 

tests/libslic3r/test_utils.cpp

@@ -8,6 +8,7 @@
 
 #include <algorithm>
 #include <cctype>
+#include <ctime>
 #include <fstream>
 #include <string>
 #include <system_error>
@@ -121,10 +122,15 @@ TEST_CASE("write_file_atomically writes through a symlink and keeps the target's
 }
 #endif
 
-TEST_CASE("remove_stale_temp_files removes only <name>.<pid>.tmp files", "[utils]") {
+TEST_CASE("remove_stale_temp_files removes only old <name>.<pid>.tmp files", "[utils]") {
     ScopedTemporaryDir dir;
-    for (const char *name : { "a.json.123.tmp", "b.info.4.tmp", "c.json", "d.tmp", "e.json.x.tmp", "f.json..tmp" })
+    for (const char *name : { "a.json.123.tmp", "b.info.4.tmp", "c.json", "d.tmp", "e.json.x.tmp", "f.json..tmp" }) {
         REQUIRE_FALSE(write_file_atomically((dir.path() / name).string(), "x"));
+        // An hour old: long past the age below which a temporary may still be in flight.
+        boost::filesystem::last_write_time(dir.path() / name, std::time(nullptr) - 3600);
+    }
+    // Just written: possibly another instance's in-flight save, so it stays.
+    REQUIRE_FALSE(write_file_atomically((dir.path() / "g.json.7.tmp").string(), "x"));
 
     SECTION("with a name prefix only matching names go") {
         REQUIRE(remove_stale_temp_files(dir.path(), "a.json") == 1);
@@ -138,7 +144,8 @@ TEST_CASE("remove_stale_temp_files removes only <name>.<pid>.tmp files", "[utils
             (void) entry;
             ++entries;
         }
-        REQUIRE(entries == 4);
+        REQUIRE(entries == 5);
+        REQUIRE(boost::filesystem::exists(dir.path() / "g.json.7.tmp"));
     }
 }