Lock the Preset Scan Per File and Reopen a Lock File Replaced on Disk

Holding the lock across the whole preset scan meant a reload on a
background thread, which the login path runs, blocked a save on the GUI
thread for the scan's duration through the in-process mutex, which has
no timeout. Each file is locked on its own now, which keeps a file whole
under a reader without keeping the saver waiting.

A guard kept its handle to the lock file for good, so a lock file that
someone deleted or recreated left this instance locking a file no other
instance could see. The guard compares what the path names against what
it opened and reopens when they differ. Preset::save() serialises before
it takes the lock, so the exclusive window is the two file writes.

On Windows an unlocked reader, which the CLI and a timed-out instance
are by design, made the rename fail at once and the write go in place
under that reader; the rename is retried for half a second first, since
a reader is done in milliseconds, and the fallback when no temporary
can be created is logged like the other one. The sweep matches only the
exact <name>.<pid>.<n>.tmp shape and waits an hour, since hosts sharing a
data dir may disagree on the time. Real write access is checked with
access(), the read-only tests skip as root, and the dead permissions
block after the rename is gone.

src/libslic3r/InstanceLock.cpp

@@ -11,6 +11,8 @@
 #include <boost/nowide/convert.hpp>
 #endif
 
+#include "Utils.hpp"
+
 namespace Slic3r {
 
 // One slot per lock file, shared by every guard in the process. A POSIX fcntl
@@ -22,6 +24,9 @@ struct InstanceLock::Slot
     std::recursive_mutex mutex;
     // Absent until the lock file has been created and opened.
     std::unique_ptr<boost::interprocess::file_lock> file_lock;
+    // What the lock file was when it was opened; a different answer means the
+    // path was unlinked or replaced and the open handle locks a dead file.
+    std::string identity;
     int  depth{0};
     bool file_locked{false};
     // After a timed-out wait, guards skip waiting until this point.
@@ -57,6 +62,7 @@ bool InstanceLock::open_lock_file(Slot &slot, const std::string &lock_file_path)
 #else
         slot.file_lock = std::make_unique<boost::interprocess::file_lock>(lock_file_path.c_str());
 #endif
+        slot.identity = file_identity(lock_file_path);
         return true;
     } catch (const std::exception &e) {
         slot.retry_at = std::chrono::steady_clock::now() + cooldown;
@@ -73,6 +79,12 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli
     m_slot       = &slot_for(lock_file_path);
     m_slot_guard = std::unique_lock<std::recursive_mutex>(m_slot->mutex);
     const auto now = std::chrono::steady_clock::now();
+    // A handle to a lock file that was deleted or recreated since it was opened
+    // would lock a file no other instance can see.
+    if (m_slot->depth == 0 && m_slot->file_lock && file_identity(lock_file_path) != m_slot->identity) {
+        BOOST_LOG_TRIVIAL(info) << "Lock file " << lock_file_path << " was replaced; reopening it";
+        m_slot->file_lock.reset();
+    }
     if (m_slot->depth == 0 && now >= m_slot->retry_at &&
         (m_slot->file_lock || open_lock_file(*m_slot, lock_file_path))) {
         const bool wait     = now >= m_slot->skip_waiting_until;

src/libslic3r/Preset.cpp

@@ -765,10 +765,15 @@ void Preset::save(DynamicPrintConfig* parent_config)
         to_save = &temp_config;
     }
 
+    std::ostringstream json;
+    to_save->save_to_json(json, bare_name, from_str, this->version.to_string());
+
     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;
+    if (const std::error_code ec = write_file_atomically(this->file, json.str()))
+        BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << ": failed to save " << this->file << ": " << ec.message();
+    else
+        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.
     if (! this->is_from_bundle()) {
@@ -1722,10 +1727,11 @@ void PresetCollection::load_presets(
     std::set<std::string> *key_set1 = nullptr, *key_set2 = nullptr;
     Preset::get_extruder_names_and_keysets(m_type, extruder_id_name, extruder_variant_name, &key_set1, &key_set2);
 
-    // Held across the scan so no instance replaces or removes a file mid-read.
-    InstanceLock instance_lock(user_presets_lock_path(read_only));
-    if (instance_lock.locked())
-        remove_stale_temp_files(dir);
+    {
+        InstanceLock instance_lock(user_presets_lock_path(read_only));
+        if (instance_lock.locked())
+            remove_stale_temp_files(dir);
+    }
     //BBS: change to json format
     for (auto &dir_entry : boost::filesystem::directory_iterator(dir))
     {
@@ -1747,30 +1753,36 @@ void PresetCollection::load_presets(
                 preset.file = dir_entry.path().string();
                 // Load the preset file, apply preset values on top of defaults.
                 try {
-                    fs::path idx_path(preset.file);
-                    idx_path.replace_extension(".info");
-                    if (fs::exists(idx_path)) {
-                        preset.load_info(idx_path.string());
-                    }
                     DynamicPrintConfig config;
-                    //BBS: change to json format
-                    //ConfigSubstitutions config_substitutions = config.load_from_ini(preset.file, substitution_rule);
                     std::map<std::string, std::string> key_values;
                     std::string reason;
-                    ConfigSubstitutions config_substitutions = config.load_from_json(preset.file, substitution_rule, key_values, reason);
+                    ConfigSubstitutions config_substitutions;
+                    {
+                        // Per file, so no instance replaces or removes it mid-read, and a
+                        // save on another thread never waits for the whole scan.
+                        InstanceLock instance_lock(user_presets_lock_path(read_only));
+                        fs::path idx_path(preset.file);
+                        idx_path.replace_extension(".info");
+                        if (fs::exists(idx_path)) {
+                            preset.load_info(idx_path.string());
+                        }
+                        //BBS: change to json format
+                        //ConfigSubstitutions config_substitutions = config.load_from_ini(preset.file, substitution_rule);
+                        config_substitutions = config.load_from_json(preset.file, substitution_rule, key_values, reason);
+                        if (!reason.empty()) {
+                            fs::path file_path(preset.file);
+                            if (!read_only && fs::exists(file_path))
+                                fs::remove(file_path);
+                            file_path.replace_extension(".info");
+                            if (!read_only && fs::exists(file_path))
+                                fs::remove(file_path);
+                            BOOST_LOG_TRIVIAL(error) << boost::format("parse config %1% failed")%preset.file;
+                            ++m_errors;
+                            continue;
+                        }
+                    }
                     if (! config_substitutions.empty())
                         substitutions.push_back({ preset.name, m_type, PresetConfigSubstitutions::Source::UserFile, preset.file, std::move(config_substitutions) });
-                    if (!reason.empty()) {
-                        fs::path file_path(preset.file);
-                        if (!read_only && fs::exists(file_path))
-                            fs::remove(file_path);
-                        file_path.replace_extension(".info");
-                        if (!read_only && fs::exists(file_path))
-                            fs::remove(file_path);
-                        BOOST_LOG_TRIVIAL(error) << boost::format("parse config %1% failed")%preset.file;
-                        ++m_errors;
-                        continue;
-                    }
 
                     std::string version_str = key_values[BBL_JSON_KEY_VERSION];
                     boost::optional<Semver> version = Semver::parse(version_str);

src/libslic3r/Utils.hpp

@@ -235,9 +235,12 @@ 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, only names starting with `name_prefix` when it is
+// `dir` at least an hour 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());
+// Names the file object behind `path` (device and inode, or volume and file index),
+// for noticing that a path was unlinked and recreated. Empty when it cannot be read.
+extern std::string file_identity(const std::string &path);
 
 enum CopyFileResult {
 	SUCCESS = 0,

src/libslic3r/utils.cpp

@@ -10,6 +10,8 @@
 #include <filesystem>
 #include <sstream>
 #include <cerrno>
+#include <chrono>
+#include <thread>
 #include <cstring>
 #include <iomanip>
 #include <algorithm>
@@ -767,50 +769,84 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_
 	if (std::error_code ec = write_whole_file(tmp_path, chunks, binary)) {
 		boost::nowide::remove(tmp_path.c_str());
 		// A directory that allows writing the file but not creating one beside it.
-		if (target_exists && ec == std::errc::permission_denied)
+		if (target_exists && ec == std::errc::permission_denied) {
+			BOOST_LOG_TRIVIAL(warning) << "Cannot create a temporary beside " << path << " (" << ec.message() << "); writing in place";
 			return write_whole_file(path, chunks, binary);
+		}
 		return ec;
 	}
 #ifndef _WIN32
-	// On Windows a read-only bit on the temporary would stop the rename itself.
+	// Only here: on Windows the target was refused above unless writable, and
+	// a read-only bit on the temporary would stop the rename itself.
 	if (target_exists)
 		boost::filesystem::permissions(tmp_path, target.permissions(), bec);
 #endif
-	std::error_code ec = rename_file(tmp_path, path);
+	// Windows refuses to replace a file another process holds open without
+	// FILE_SHARE_DELETE, which is how the C runtime opens files for reading; a
+	// reader is done in milliseconds, so wait it out before giving up on the
+	// atomic path.
+	std::error_code ec;
+	for (int attempt = 0; attempt < 20; ++ attempt) {
+		ec = rename_file(tmp_path, path);
+		if (ec != std::errc::permission_denied)
+			break;
+		std::this_thread::sleep_for(std::chrono::milliseconds(25));
+	}
 	if (ec) {
 		boost::nowide::remove(tmp_path.c_str());
-		// 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.
+		// Still refused, or a mount that 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";
 		return write_whole_file(path, chunks, binary);
 	}
-#ifdef _WIN32
-	if (target_exists)
-		boost::filesystem::permissions(path, target.permissions(), bec);
-#endif
 	return {};
 }
 
+std::string file_identity(const std::string &path)
+{
+#ifdef _WIN32
+	const std::wstring wide = boost::nowide::widen(path);
+	HANDLE handle = ::CreateFileW(wide.c_str(), 0, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, nullptr);
+	if (handle == INVALID_HANDLE_VALUE)
+		return {};
+	BY_HANDLE_FILE_INFORMATION info;
+	const bool ok = ::GetFileInformationByHandle(handle, &info) != 0;
+	::CloseHandle(handle);
+	if (! ok)
+		return {};
+	return std::to_string(info.dwVolumeSerialNumber) + ":" + std::to_string((static_cast<uint64_t>(info.nFileIndexHigh) << 32) | info.nFileIndexLow);
+#else
+	struct stat st;
+	if (::stat(path.c_str(), &st) != 0)
+		return {};
+	return std::to_string(static_cast<unsigned long long>(st.st_dev)) + ":" + std::to_string(static_cast<unsigned long long>(st.st_ino));
+#endif
+}
+
 size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::string &name_prefix)
 {
-	// <name_prefix>...<digits>.tmp
+	// <name_prefix>...<digits>.<digits>.tmp, exactly the shape write_file_atomically() makes.
 	auto is_temp_name = [&name_prefix](const std::string &name) {
 		if (name.compare(0, name_prefix.size(), name_prefix) != 0)
 			return false;
 		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 && dot + 1 != digits_end &&
-		       std::all_of(name.begin() + dot + 1, name.begin() + digits_end, [](char c) { return c >= '0' && c <= '9'; });
+		size_t end = name.size() - suffix.size();
+		for (int segment = 0; segment < 2; ++ segment) {
+			const size_t dot = name.rfind('.', end - 1);
+			if (dot == std::string::npos || dot == 0 || dot + 1 == end ||
+			    ! std::all_of(name.begin() + dot + 1, name.begin() + end, [](char c) { return c >= '0' && c <= '9'; }))
+				return false;
+			end = dot;
+		}
+		return true;
 	};
 	// 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;
+	// so a temporary this young may still be in flight, and hosts sharing a
+	// data dir may disagree on the time by minutes; a crash leftover is old.
+	constexpr std::time_t stale_age = 60 * 60;
 	const std::time_t     now       = std::time(nullptr);
 	size_t removed = 0;
 	boost::system::error_code ec;

tests/libslic3r/test_instance_lock.cpp

@@ -96,6 +96,24 @@ TEST_CASE("InstanceLock retries a lock file it could not open once the cool-down
     REQUIRE(after_cooldown);
 }
 
+TEST_CASE("InstanceLock reopens a lock file that was replaced on disk", "[InstanceLock]")
+{
+    ScopedTemporaryFile lock_file(".lock");
+    const std::string   path = lock_file.string();
+    {
+        InstanceLock lock(path);
+        REQUIRE(lock.locked());
+    }
+
+    boost::filesystem::remove(path);
+    InstanceLock lock(path);
+    REQUIRE(lock.locked());
+    // Only a reopen recreates the file; a guard still holding the unlinked one
+    // would leave the path missing. (The inode number itself may be reused once
+    // the old handle is closed, so it is no proof either way.)
+    REQUIRE(boost::filesystem::exists(path));
+}
+
 TEST_CASE("InstanceLock serialises the threads of one process", "[InstanceLock]")
 {
     ScopedTemporaryFile lock_file(".lock");

tests/libslic3r/test_utils.cpp

@@ -93,6 +93,10 @@ TEST_CASE("write_file_atomically reports a missing directory and writes nothing"
 }
 
 TEST_CASE("write_file_atomically refuses a read-only target and leaves it untouched", "[Utils]") {
+#ifndef _WIN32
+    if (::geteuid() == 0)
+        SKIP("a read-only file does not stop root");
+#endif
     ScopedTemporaryDir dir;
     const boost::filesystem::path target = dir.path() / "pinned.json";
     REQUIRE_FALSE(write_file_atomically(target.string(), "pinned"));
@@ -164,33 +168,33 @@ TEST_CASE("write_file_atomically survives two threads writing one target", "[Uti
     REQUIRE(temporaries == 0);
 }
 
-TEST_CASE("remove_stale_temp_files removes only old <name>.<pid>.tmp files", "[Utils]") {
+TEST_CASE("remove_stale_temp_files removes only old <name>.<pid>.<n>.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", "a.json.99", "a.json.12.3.tmp" }) {
+    for (const char *name : { "a.json.123.7.tmp", "b.info.4.0.tmp", "c.json", "d.tmp", "e.json.x.1.tmp", "f.json..tmp", "a.json.99", "a.json.12.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"));
+    REQUIRE_FALSE(write_file_atomically((dir.path() / "g.json.7.2.tmp").string(), "x"));
 
-    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.12.3.tmp"));
+    SECTION("with a name prefix only matching names go; a numbered backup or a one-segment name is not a temporary") {
+        REQUIRE(remove_stale_temp_files(dir.path(), "a.json") == 1);
+        REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "a.json.123.7.tmp"));
         REQUIRE(boost::filesystem::exists(dir.path() / "a.json.99"));
-        REQUIRE(boost::filesystem::exists(dir.path() / "b.info.4.tmp"));
+        REQUIRE(boost::filesystem::exists(dir.path() / "a.json.12.tmp"));
+        REQUIRE(boost::filesystem::exists(dir.path() / "b.info.4.0.tmp"));
     }
     SECTION("without a prefix every stale temporary goes and nothing else") {
-        REQUIRE(remove_stale_temp_files(dir.path()) == 3);
+        REQUIRE(remove_stale_temp_files(dir.path()) == 2);
         size_t entries = 0;
         for (auto &entry : boost::filesystem::directory_iterator(dir.path())) {
             (void) entry;
             ++entries;
         }
-        REQUIRE(entries == 6);
+        REQUIRE(entries == 7);
         REQUIRE(boost::filesystem::exists(dir.path() / "a.json.99"));
-        REQUIRE(boost::filesystem::exists(dir.path() / "g.json.7.tmp"));
+        REQUIRE(boost::filesystem::exists(dir.path() / "g.json.7.2.tmp"));
     }
 }
 

tests/libslic3r/test_vendor_cache.cpp

@@ -1,5 +1,9 @@
 #include <catch2/catch_all.hpp>
 
+#ifndef _WIN32
+#include <unistd.h>
+#endif
+
 #include <boost/filesystem.hpp>
 #include <boost/crc.hpp>
 #include <cereal/archives/binary.hpp>
@@ -1358,6 +1362,10 @@ TEST_CASE("a header claiming more body than the file holds is rejected", "[Vendo
 
 TEST_CASE("a failed write leaves the previous cache in place", "[VendorCache]")
 {
+#ifndef _WIN32
+    if (::geteuid() == 0)
+        SKIP("a read-only file does not stop root");
+#endif
     TempDir tmp;
     const std::string cache = (tmp.path / "Durable.opc").string();
     REQUIRE(save_one_vendor(cache, one_vendor("Durable"), "Durable", "1.0.0"));