Never Mistake a Numbered Backup for a Temporary and Check Real Write Access

The sweep's prefix form counted any <name>.<digits> file as a leftover of
the old config writer, so a user's OrcaSlicer.conf.1 backup went at the
next start. Only the <name>.<pid>.<n>.tmp form is a temporary now, and
the name is tested before the entry is stat'ed.

The read-only check went by mode bits, which misses a deny-write ACL on
Windows and refuses a root-owned file root can write; it asks the OS
with access() instead. The two-step rename fallback runs only for the
errors a refused replace produces, never for an I/O error that would
only lose the target, and says so if the second step fails after the
old file is gone.

The orphaned-.info scan on the sync thread reads each .info under the
lock, so a remove_files() in another instance is seen whole or not at
all, and queues the cloud delete after the lock is released, keeping the
queue mutex outside it. AppConfig::save() assembles its text before it
takes the lock, like Preset::save(). The two bundle loops share one
metadata loader, and the two-thread write test counts leftover
temporaries rather than every directory entry, since a scanner on
Windows may hold the old file under another name for a moment.

src/libslic3r/AppConfig.cpp

@@ -991,7 +991,6 @@ void AppConfig::save()
     // The config is first written to a file with a PID suffix and then moved
     // to avoid race conditions with multiple instances of Slic3r
     const auto path = config_path();
-    InstanceLock instance_lock(lock_path());
 
     json j;
 
@@ -1122,6 +1121,8 @@ void AppConfig::save()
         j["local_machines"][local_machine.first] = m_json;
     }
     std::string config_str = j.dump(1, '\t');
+    // Everything above is assembly; only the writes need the other instances kept out.
+    InstanceLock instance_lock(lock_path());
 #ifdef WIN32
     // WIN32 specific: the final replace is not safe in case of an application crash, there is no atomic "rename file" API
     // provided by Windows (sic!). Therefore we save a MD5 checksum to be able to verify file corruption. In addition,
@@ -1291,7 +1292,6 @@ void AppConfig::save()
     // The config is first written to a file with a PID suffix and then moved
     // to avoid race conditions with multiple instances of Slic3r
     const auto path = config_path();
-    InstanceLock instance_lock(lock_path());
 
     std::stringstream config_ss;
     if (m_mode == EAppMode::Editor)
@@ -1328,6 +1328,8 @@ void AppConfig::save()
     config_ss << std::endl;
 
     std::string config_str = config_ss.str();
+    // Everything above is assembly; only the writes need the other instances kept out.
+    InstanceLock instance_lock(lock_path());
 #ifdef WIN32
     // WIN32 specific: the final replace is not safe in case of an application crash, there is no atomic "rename file" API
     // provided by Windows (sic!). Therefore we save a MD5 checksum to be able to verify file corruption. In addition,

src/libslic3r/PresetBundle.cpp

@@ -1228,6 +1228,15 @@ PresetsConfigSubstitutions PresetBundle::load_user_presets(std::string user, For
 
     const auto user_load_t0 = std::chrono::steady_clock::now();
 
+    // Reads one bundle's metadata under the lock, per file, so the lock is never
+    // held when bundles.WriteLock() is taken afterwards.
+    auto load_bundle_metadata = [read_only](const fs::path &bundle_dir, const fs::path &metadata_file, BundleMetadata &metadata) {
+        InstanceLock instance_lock(user_presets_lock_path(read_only));
+        if (instance_lock.locked())
+            remove_stale_temp_files(bundle_dir);
+        return metadata.load_from_json(metadata_file.string());
+    };
+
     // Load bundle metadata from _local directory first
     fs::path local_dir(folder / PRESET_LOCAL_DIR);
     if (fs::exists(local_dir)) {
@@ -1241,13 +1250,7 @@ PresetsConfigSubstitutions PresetBundle::load_user_presets(std::string user, For
             if (!fs::exists(metadata_file)) continue;
 
             BundleMetadata metadata;
-            {
-                // Per file, so the lock is never held when bundles.WriteLock() is taken below.
-                InstanceLock instance_lock(user_presets_lock_path(read_only));
-                if (instance_lock.locked())
-                    remove_stale_temp_files(entry.path());
-                if (!metadata.load_from_json(metadata_file.string())) continue;
-            }
+            if (!load_bundle_metadata(entry.path(), metadata_file, metadata)) continue;
             metadata.print_presets.clear();
             metadata.filament_presets.clear();
             metadata.printer_presets.clear();
@@ -1282,13 +1285,7 @@ PresetsConfigSubstitutions PresetBundle::load_user_presets(std::string user, For
             if (!fs::exists(metadata_file)) continue;
 
             BundleMetadata metadata;
-            {
-                // Per file, so the lock is never held when bundles.WriteLock() is taken below.
-                InstanceLock instance_lock(user_presets_lock_path(read_only));
-                if (instance_lock.locked())
-                    remove_stale_temp_files(entry.path());
-                if (!metadata.load_from_json(metadata_file.string())) continue;
-            }
+            if (!load_bundle_metadata(entry.path(), metadata_file, metadata)) continue;
             metadata.print_presets.clear();
             metadata.filament_presets.clear();
             metadata.printer_presets.clear();

src/libslic3r/Utils.hpp

@@ -235,10 +235,8 @@ extern std::error_code write_file_atomically(const std::string &path, std::initi
 inline std::error_code write_file_atomically(const std::string &path, const std::string &content, bool binary = false)
 	{ return write_file_atomically(path, { std::string_view(content) }, binary); }
 // Remove the `<name>.<pid>.<n>.tmp` files a crashed write_file_atomically() left in
-// `dir` at least ten minutes ago. With `name_prefix`, only names starting with it
-// go, and so do `<name_prefix>.<pid>` leftovers of the older AppConfig writer, so
-// the prefix form is for directories the application owns, never a user's export
-// folder. Returns how many were removed.
+// `dir` at least ten minutes ago, only names starting with `name_prefix` when it is
+// given. Meant for directories the application owns. 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

@@ -10,6 +10,7 @@
 #include <filesystem>
 #include <sstream>
 #include <cerrno>
+#include <cstring>
 #include <iomanip>
 #include <algorithm>
 #include <cmath>
@@ -711,16 +712,30 @@ std::error_code rename_file(const std::string &from, const std::string &to)
 		return {};
 	const int err = errno;
 	// Some mounts (sshfs, gvfs, MTP and a few SMB setups) refuse to replace an
-	// existing target in one step, with whichever error they see fit; take the
-	// old two-step route whenever the target is still there to be replaced.
+	// existing target in one step and report it as one of these; take the old
+	// two-step route there, but not for an I/O error that would only lose the
+	// target for nothing.
+	const bool replace_refused = err == EPERM || err == EACCES || err == EEXIST || err == ENOTEMPTY || err == EBUSY || err == ENOTSUP || err == EOPNOTSUPP;
 	boost::system::error_code ec;
-	if (err != ENOENT && err != EXDEV && boost::filesystem::exists(to, ec) && boost::nowide::remove(to.c_str()) == 0 &&
-	    boost::nowide::rename(from.c_str(), to.c_str()) == 0)
-		return {};
+	if (replace_refused && boost::filesystem::exists(to, ec) && boost::nowide::remove(to.c_str()) == 0) {
+		if (boost::nowide::rename(from.c_str(), to.c_str()) == 0)
+			return {};
+		BOOST_LOG_TRIVIAL(error) << "Replacing " << to << " failed after the old file was removed: " << std::strerror(errno);
+	}
 	return std::make_error_code(static_cast<std::errc>(err));
 #endif
 }
 
+// Whether this process may open `path` for writing.
+static bool is_writable(const std::string &path)
+{
+#ifdef _WIN32
+	return ::_waccess(boost::nowide::widen(path).c_str(), 2) == 0;
+#else
+	return ::access(path.c_str(), W_OK) == 0;
+#endif
+}
+
 static std::error_code write_whole_file(const std::string &path, std::initializer_list<std::string_view> chunks, bool binary)
 {
 	errno = 0;
@@ -740,9 +755,9 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_
 	const bool target_exists = ! bec && boost::filesystem::exists(target);
 	if (target_exists && ! boost::filesystem::is_regular_file(target))
 		return write_whole_file(path, chunks, binary);
-	// Replacing needs only a writable directory, so a file the user made
-	// read-only has to be honoured here, as the in-place write used to.
-	if (target_exists && (target.permissions() & (boost::filesystem::owner_write | boost::filesystem::group_write | boost::filesystem::others_write)) == boost::filesystem::no_perms)
+	// Replacing needs only a writable directory, so a file this process may
+	// not write has to be refused here, as the in-place write used to be.
+	if (target_exists && ! is_writable(path))
 		return std::make_error_code(std::errc::permission_denied);
 
 	// Unique per process and per call, so two threads writing one target
@@ -781,22 +796,17 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_
 
 size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::string &name_prefix)
 {
-	auto all_digits = [](std::string::const_iterator begin, std::string::const_iterator end) {
-		return begin != end && std::all_of(begin, end, [](char c) { return c >= '0' && c <= '9'; });
-	};
-	// <name_prefix>...<digits>.tmp, or the older <name_prefix>.<digits> when a prefix is given.
-	auto is_temp_name = [&](const std::string &name) {
+	// <name_prefix>...<digits>.tmp
+	auto is_temp_name = [&name_prefix](const std::string &name) {
 		if (name.compare(0, name_prefix.size(), name_prefix) != 0)
 			return false;
-		if (! name_prefix.empty() && name.size() > name_prefix.size() + 1 && name[name_prefix.size()] == '.' &&
-		    all_digits(name.begin() + name_prefix.size() + 1, name.end()))
-			return true;
 		static const std::string suffix = ".tmp";
 		if (name.size() <= suffix.size() || name.compare(name.size() - suffix.size(), suffix.size(), suffix) != 0)
 			return false;
 		const size_t digits_end = name.size() - suffix.size();
 		const size_t dot        = name.rfind('.', digits_end - 1);
-		return dot != std::string::npos && dot != 0 && all_digits(name.begin() + dot + 1, name.begin() + digits_end);
+		return dot != std::string::npos && dot != 0 && dot + 1 != digits_end &&
+		       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.
@@ -805,7 +815,8 @@ size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::st
 	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(ec)) || ! is_temp_name(it->path().filename().string()))
+		// The name test first: it is free, the stat is not.
+		if (! is_temp_name(it->path().filename().string()) || ! boost::filesystem::is_regular_file(it->symlink_status(ec)))
 			continue;
 		const std::time_t written = boost::filesystem::last_write_time(it->path(), ec);
 		if (ec || now - written < stale_age) {

src/slic3r/GUI/GUI_App.cpp

@@ -8956,15 +8956,19 @@ void GUI_App::scan_orphaned_info_files()
             fs::path preset_file = info_file;
             preset_file.replace_extension(".json");
 
-            // If .json doesn't exist, .info is orphaned
-            if (!fs::exists(preset_file)) {
-                // Extract setting_id from .info file
-                std::string setting_id = extract_setting_id_from_info(info_file.string());
-                if (!setting_id.empty()) {
-                    // Add to need_delete_presets
-                    delete_preset_from_cloud(setting_id, info_file.string());
-                    BOOST_LOG_TRIVIAL(info) << "Found orphaned .info file on startup: " << info_file.string();
-                }
+            // If .json doesn't exist, .info is orphaned. Read under the lock, so a
+            // remove_files() in another instance is seen whole or not at all; the
+            // delete queue's own mutex is taken after the lock is released.
+            std::string setting_id;
+            {
+                InstanceLock instance_lock(user_presets_lock_path());
+                if (!fs::exists(preset_file))
+                    setting_id = extract_setting_id_from_info(info_file.string());
+            }
+            if (!setting_id.empty()) {
+                // Add to need_delete_presets
+                delete_preset_from_cloud(setting_id, info_file.string());
+                BOOST_LOG_TRIVIAL(info) << "Found orphaned .info file on startup: " << info_file.string();
             }
         }
         if (ec)

tests/libslic3r/test_utils.cpp

@@ -155,12 +155,13 @@ TEST_CASE("write_file_atomically survives two threads writing one target", "[Uti
     load_string_file(target, content);
     const bool whole = content == a || content == b;
     REQUIRE(whole);
-    size_t entries = 0;
-    for (auto &entry : boost::filesystem::directory_iterator(dir.path())) {
-        (void) entry;
-        ++entries;
-    }
-    REQUIRE(entries == 1);
+    // No temporary may be left; a scanner on Windows may briefly hold the old
+    // file under another name, so only the temporaries are counted.
+    size_t temporaries = 0;
+    for (auto &entry : boost::filesystem::directory_iterator(dir.path()))
+        if (entry.path().extension() == ".tmp")
+            ++temporaries;
+    REQUIRE(temporaries == 0);
 }
 
 TEST_CASE("remove_stale_temp_files removes only old <name>.<pid>.tmp files", "[Utils]") {
@@ -173,11 +174,11 @@ TEST_CASE("remove_stale_temp_files removes only old <name>.<pid>.tmp files", "[U
     // 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, including the older <name>.<pid> form") {
-        REQUIRE(remove_stale_temp_files(dir.path(), "a.json") == 3);
+    SECTION("with a name prefix only matching names go, and a numbered backup is not a temporary") {
+        REQUIRE(remove_stale_temp_files(dir.path(), "a.json") == 2);
         REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "a.json.123.tmp"));
-        REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "a.json.99"));
         REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "a.json.12.3.tmp"));
+        REQUIRE(boost::filesystem::exists(dir.path() / "a.json.99"));
         REQUIRE(boost::filesystem::exists(dir.path() / "b.info.4.tmp"));
     }
     SECTION("without a prefix every stale temporary goes and nothing else") {