Move a Refused Target Aside Rather Than Removing It and Wait Only for a Reader

The two-step rename fallback removed the target before its second try,
and rename(2) reports the same errors for a source it cannot move, so a
refusal about the source cost the caller its existing file. The target
is moved aside and put back if the second step fails too.

The wait for a reader holding the target open was layered on the write
helper and ran on every platform and for every refusal, so a read-only
target on Windows cost half a second before failing anyway, while G-code
exports through rename_file() got no wait at all. It lives in
rename_file() now, on Windows only, and only for a target this process
could write. A failed config write clears the dirty flag as it always
did, so the idle handler does not repeat it on every tick.

The lock file is created readable and writable by every user, since
another user sharing the data dir has to open it read-write; the check
that the file behind the path is still the one opened runs at most every
few seconds rather than once per preset during a scan; the physical
printer loader reads under the lock; the stat headers join the existing
platform include block; and the utility tests keep their file's tag.

src/libslic3r/AppConfig.cpp

@@ -1121,8 +1121,10 @@ void AppConfig::save()
         j["local_machines"][local_machine.first] = m_json;
     }
     const std::string config_str = j.dump(1, '\t');
-    if (write_config_file(path, config_str + "\n", config_str))
-        m_dirty = false;
+    write_config_file(path, config_str + "\n", config_str);
+    // Cleared even after a failed write, as before: the failure is logged, and
+    // the next change tries again rather than every idle tick.
+    m_dirty = false;
 }
 
 #else
@@ -1309,12 +1311,12 @@ void AppConfig::save()
     config_ss << std::endl;
 
     const std::string config_str = config_ss.str();
-    if (write_config_file(path, config_str, config_str))
-        m_dirty = false;
+    write_config_file(path, config_str, config_str);
+    m_dirty = false;
 }
 #endif
 
-bool AppConfig::write_config_file(const std::string &path, std::string body, const std::string &checksum_source)
+void AppConfig::write_config_file(const std::string &path, std::string body, const std::string &checksum_source)
 {
     // Everything before this is assembly; only the writes need the other instances kept out.
     InstanceLock instance_lock(lock_path());
@@ -1328,11 +1330,8 @@ bool AppConfig::write_config_file(const std::string &path, std::string body, con
         BOOST_LOG_TRIVIAL(error) << "Failed to write the backup configuration " << backup_path << ": " << ec.message();
 #endif
     // The config was always replaced, never written in place, so a read-only one is replaced still.
-    if (const std::error_code ec = write_file_atomically(path, body, false, /*replace_read_only=*/true)) {
+    if (const std::error_code ec = write_file_atomically(path, body, false, /*replace_read_only=*/true))
         BOOST_LOG_TRIVIAL(error) << "Failed to write the configuration " << path << ": " << ec.message();
-        return false;
-    }
-    return true;
 }
 
 bool AppConfig::get_variant(const std::string &vendor, const std::string &model, const std::string &variant) const

src/libslic3r/AppConfig.hpp

@@ -453,7 +453,7 @@ private:
 	MachineSettingMap											m_printer_settings;
 	// Writes the assembled config text, and on Windows its checksum and a backup copy. `checksum_source`
 	// is the text load() will verify, which for the JSON config ends before the trailing newline.
-	bool												write_config_file(const std::string &path, std::string body, const std::string &checksum_source);
+	void												write_config_file(const std::string &path, std::string body, const std::string &checksum_source);
 
 	bool														m_dirty;
 	// Original version found in the ini file before it was overwritten

src/libslic3r/InstanceLock.cpp

@@ -6,6 +6,7 @@
 
 #include <boost/interprocess/sync/file_lock.hpp>
 #include <boost/log/trivial.hpp>
+#include <boost/filesystem.hpp>
 #include <boost/nowide/fstream.hpp>
 #ifdef _WIN32
 #include <boost/nowide/convert.hpp>
@@ -26,7 +27,10 @@ struct InstanceLock::Slot
     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.
+    // Checked at most every few seconds, since a preset scan takes one guard
+    // per file.
     std::string identity;
+    std::chrono::steady_clock::time_point identity_checked_at{};
     int  depth{0};
     bool file_locked{false};
     // After a timed-out wait, guards skip waiting until this point.
@@ -55,14 +59,20 @@ InstanceLock::Slot &InstanceLock::slot_for(const std::string &lock_file_path)
 bool InstanceLock::open_lock_file(Slot &slot, const std::string &lock_file_path)
 {
     try {
-        // file_lock only opens existing files.
+        // file_lock only opens existing files, read-write, and every user
+        // sharing the data dir has to be able to open it.
         boost::nowide::ofstream(lock_file_path, std::ios::app).close();
+        boost::system::error_code ec;
+        boost::filesystem::permissions(lock_file_path, boost::filesystem::owner_read | boost::filesystem::owner_write |
+                                                       boost::filesystem::group_read | boost::filesystem::group_write |
+                                                       boost::filesystem::others_read | boost::filesystem::others_write, ec);
 #ifdef _WIN32
         slot.file_lock = std::make_unique<boost::interprocess::file_lock>(boost::nowide::widen(lock_file_path).c_str());
 #else
         slot.file_lock = std::make_unique<boost::interprocess::file_lock>(lock_file_path.c_str());
 #endif
-        slot.identity = file_identity(lock_file_path);
+        slot.identity            = file_identity(lock_file_path);
+        slot.identity_checked_at = std::chrono::steady_clock::now();
         return true;
     } catch (const std::exception &e) {
         slot.retry_at = std::chrono::steady_clock::now() + cooldown;
@@ -81,9 +91,12 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli
     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 && m_slot->file_lock && now - m_slot->identity_checked_at >= identity_check_interval) {
+        m_slot->identity_checked_at = now;
+        if (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))) {

src/libslic3r/InstanceLock.hpp

@@ -36,6 +36,9 @@ public:
     // How long a lock file is left alone after a timed-out wait, a failed open
     // or a failing lock call. Mutable so tests can shorten it.
     static inline std::chrono::milliseconds cooldown{10000};
+    // How often a guard re-checks that the lock file behind the path is still
+    // the one it opened. Mutable so tests can shorten it.
+    static inline std::chrono::milliseconds identity_check_interval{5000};
 
     // An empty path makes the guard a no-op.
     explicit InstanceLock(const std::string &lock_file_path, std::chrono::milliseconds timeout = default_timeout);

src/libslic3r/Preset.cpp

@@ -4375,6 +4375,7 @@ void PhysicalPrinterCollection::load_printers(
                 continue;
             }
             try {
+                InstanceLock instance_lock(user_presets_lock_path());
                 PhysicalPrinter printer(name, this->default_config());
                 printer.file = dir_entry.path().string();
                 // Load the preset file, apply preset values on top of defaults.

src/libslic3r/utils.cpp

@@ -14,12 +14,6 @@
 #include <thread>
 #include <cstring>
 #include <iomanip>
-#ifdef _WIN32
-#include <io.h>
-#else
-#include <unistd.h>
-#include <sys/stat.h>
-#endif
 #include <algorithm>
 #include <cmath>
 
@@ -44,6 +38,7 @@
 #else
 	#include <unistd.h>
 	#include <sys/types.h>
+	#include <sys/stat.h>
 	#include <sys/param.h>
     #include <sys/resource.h>
 	#ifdef BSD
@@ -54,7 +49,6 @@
 		#include <libproc.h>
 	#endif
 	#ifdef __linux__
-		#include <sys/stat.h>
 		#include <fcntl.h>
 		#include <sys/sendfile.h>
 		#include <dirent.h>
@@ -712,7 +706,16 @@ namespace WindowsSupport
 std::error_code rename_file(const std::string &from, const std::string &to)
 {
 #ifdef _WIN32
-	return WindowsSupport::rename(from, to);
+	std::error_code ec = WindowsSupport::rename(from, to);
+	// 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. A target that is not
+	// writable at all is refused for good and not worth waiting on.
+	for (int attempt = 0; attempt < 20 && ec == std::errc::permission_denied && ::_waccess(boost::nowide::widen(to).c_str(), 2) == 0; ++ attempt) {
+		std::this_thread::sleep_for(std::chrono::milliseconds(25));
+		ec = WindowsSupport::rename(from, to);
+	}
+	return ec;
 #else
 	// rename(2) replaces an existing target atomically; removing it first would
 	// leave a window in which the file does not exist at all.
@@ -720,14 +723,19 @@ 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 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.
+	// existing target in one step and report it as one of these; move the
+	// target aside and try again there, and put it back if that fails too, so
+	// a refusal that was really about the source never costs the target.
 	const bool replace_refused = err == EPERM || err == EACCES || err == EEXIST || err == ENOTEMPTY || err == EBUSY || err == ENOTSUP || err == EOPNOTSUPP;
-	if (replace_refused && 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);
+	if (replace_refused) {
+		const std::string aside = to + "." + std::to_string(get_current_pid()) + ".old";
+		if (boost::nowide::rename(to.c_str(), aside.c_str()) == 0) {
+			if (boost::nowide::rename(from.c_str(), to.c_str()) == 0) {
+				boost::nowide::remove(aside.c_str());
+				return {};
+			}
+			boost::nowide::rename(aside.c_str(), to.c_str());
+		}
 	}
 	return std::make_error_code(static_cast<std::errc>(err));
 #endif
@@ -789,22 +797,12 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_
 	if (target_exists)
 		boost::filesystem::permissions(tmp_path, target.permissions(), bec);
 #endif
-	std::error_code ec = rename_file(tmp_path, path);
-#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; a
-	// reader is done in milliseconds, so wait it out before giving up on the
-	// atomic path. Elsewhere a refused replace does not clear by waiting.
-	for (int attempt = 0; attempt < 20 && ec == std::errc::permission_denied; ++ attempt) {
-		std::this_thread::sleep_for(std::chrono::milliseconds(25));
-		ec = rename_file(tmp_path, path);
-	}
-#endif
-	if (ec) {
+	if (const std::error_code ec = rename_file(tmp_path, path)) {
 		boost::nowide::remove(tmp_path.c_str());
-		// 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.
+		// A reader on Windows that outlasted rename_file()'s wait, or a mount
+		// that cannot replace a file 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);
 	}

tests/libslic3r/test_instance_lock.cpp

@@ -100,6 +100,8 @@ TEST_CASE("InstanceLock reopens a lock file that was replaced on disk", "[Instan
 {
     ScopedTemporaryFile lock_file(".lock");
     const std::string   path = lock_file.string();
+    const auto          saved_interval = InstanceLock::identity_check_interval;
+    InstanceLock::identity_check_interval = 0ms;
     {
         InstanceLock lock(path);
         REQUIRE(lock.locked());
@@ -107,6 +109,7 @@ TEST_CASE("InstanceLock reopens a lock file that was replaced on disk", "[Instan
 
     boost::filesystem::remove(path);
     InstanceLock lock(path);
+    InstanceLock::identity_check_interval = saved_interval;
     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

tests/libslic3r/test_utils.cpp

@@ -20,7 +20,7 @@
 
 using namespace Slic3r;
 
-TEST_CASE("per_user_temp_dir composes a per-user temp root", "[Utils]") {
+TEST_CASE("per_user_temp_dir composes a per-user temp root", "[utils]") {
     const std::string base = "/tmp";
 
     SECTION("an empty id returns base unchanged") {
@@ -34,7 +34,7 @@ TEST_CASE("per_user_temp_dir composes a per-user temp root", "[Utils]") {
     }
 }
 
-TEST_CASE("per_user_temp_id follows the platform contract", "[Utils]") {
+TEST_CASE("per_user_temp_id follows the platform contract", "[utils]") {
     const std::string id = per_user_temp_id();
 
     SECTION("stable across calls") {
@@ -54,7 +54,7 @@ TEST_CASE("per_user_temp_id follows the platform contract", "[Utils]") {
 
 // The end-to-end contract callers depend on: the temp root is left alone on
 // Windows and isolated per user on Linux/macOS.
-TEST_CASE("per-user temp root is unchanged on Windows, isolated elsewhere", "[Utils]") {
+TEST_CASE("per-user temp root is unchanged on Windows, isolated elsewhere", "[utils]") {
     const std::string base = "/tmp";
     const std::string root = per_user_temp_dir(base, per_user_temp_id());
 #ifdef _WIN32
@@ -65,7 +65,7 @@ TEST_CASE("per-user temp root is unchanged on Windows, isolated elsewhere", "[Ut
 #endif
 }
 
-TEST_CASE("write_file_atomically replaces the target and leaves no temporary file", "[Utils]") {
+TEST_CASE("write_file_atomically replaces the target and leaves no temporary file", "[utils]") {
     ScopedTemporaryDir dir;
     const boost::filesystem::path target = dir.path() / "preset.json";
 
@@ -83,7 +83,7 @@ TEST_CASE("write_file_atomically replaces the target and leaves no temporary fil
     REQUIRE(entries == 1);
 }
 
-TEST_CASE("write_file_atomically reports a missing directory and writes nothing", "[Utils]") {
+TEST_CASE("write_file_atomically reports a missing directory and writes nothing", "[utils]") {
     ScopedTemporaryDir dir;
     const boost::filesystem::path target = dir.path() / "missing" / "preset.json";
 
@@ -92,7 +92,7 @@ TEST_CASE("write_file_atomically reports a missing directory and writes nothing"
     REQUIRE_FALSE(boost::filesystem::exists(target));
 }
 
-TEST_CASE("write_file_atomically refuses a read-only target and leaves it untouched", "[Utils]") {
+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");
@@ -111,7 +111,7 @@ TEST_CASE("write_file_atomically refuses a read-only target and leaves it untouc
     REQUIRE(content == "pinned");
 }
 
-TEST_CASE("write_file_atomically replaces a read-only target when asked to", "[Utils]") {
+TEST_CASE("write_file_atomically replaces a read-only target when asked to", "[utils]") {
 #ifndef _WIN32
     if (::geteuid() == 0)
         SKIP("a read-only file does not stop root");
@@ -130,7 +130,7 @@ TEST_CASE("write_file_atomically replaces a read-only target when asked to", "[U
     REQUIRE(content == "replaced");
 }
 
-TEST_CASE("write_file_atomically keeps bytes intact in binary mode", "[Utils]") {
+TEST_CASE("write_file_atomically keeps bytes intact in binary mode", "[utils]") {
     ScopedTemporaryDir dir;
     const boost::filesystem::path target = dir.path() / "blob.bin";
     const std::string bytes("a\r\nb\0c", 6);
@@ -140,7 +140,7 @@ TEST_CASE("write_file_atomically keeps bytes intact in binary mode", "[Utils]")
 }
 
 #ifndef _WIN32
-TEST_CASE("write_file_atomically writes through a symlink and keeps the target's permissions", "[Utils]") {
+TEST_CASE("write_file_atomically writes through a symlink and keeps the target's permissions", "[utils]") {
     ScopedTemporaryDir dir;
     const boost::filesystem::path real = dir.path() / "real.json";
     const boost::filesystem::path link = dir.path() / "link.json";
@@ -161,7 +161,7 @@ TEST_CASE("write_file_atomically writes through a symlink and keeps the target's
 }
 #endif
 
-TEST_CASE("write_file_atomically survives two threads writing one target", "[Utils]") {
+TEST_CASE("write_file_atomically survives two threads writing one target", "[utils]") {
     ScopedTemporaryDir dir;
     const boost::filesystem::path target = dir.path() / "shared.json";
     const std::string a(20000, 'a'), b(20000, 'b');
@@ -187,7 +187,7 @@ 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>.<n>.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.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"));
@@ -217,7 +217,7 @@ TEST_CASE("remove_stale_temp_files removes only old <name>.<pid>.<n>.tmp files",
     }
 }
 
-TEST_CASE("copy_file reports the OS error when the destination cannot be written", "[Utils]") {
+TEST_CASE("copy_file reports the OS error when the destination cannot be written", "[utils]") {
     ScopedTemporaryFile source(".txt");
     {
         std::ofstream ofs(source.string(), std::ios::binary);
@@ -246,7 +246,7 @@ TEST_CASE("copy_file reports the OS error when the destination cannot be written
 #endif // _WIN32
 }
 
-TEST_CASE("A resolved input path still names the same file after the working directory changes", "[Utils]") {
+TEST_CASE("A resolved input path still names the same file after the working directory changes", "[utils]") {
     ScopedTemporaryFile model(".3mf");
     { std::ofstream out(model.string()); out << "3mf"; }
     const std::string name = model.path().filename().string();
@@ -263,7 +263,7 @@ TEST_CASE("A resolved input path still names the same file after the working dir
     REQUIRE_FALSE(boost::filesystem::exists(name));
 }
 
-TEST_CASE("resolve_cli_input_path completes a relative path against the working directory", "[Utils]") {
+TEST_CASE("resolve_cli_input_path completes a relative path against the working directory", "[utils]") {
     ScopedWorkingDirectory cwd(boost::filesystem::temp_directory_path());
     // Read back rather than reusing temp_directory_path(): changing to it resolves any symlink.
     const boost::filesystem::path here = boost::filesystem::current_path();
@@ -279,7 +279,7 @@ TEST_CASE("resolve_cli_input_path completes a relative path against the working
     }
 }
 
-TEST_CASE("resolve_cli_input_path leaves inputs that must not be completed unchanged", "[Utils]") {
+TEST_CASE("resolve_cli_input_path leaves inputs that must not be completed unchanged", "[utils]") {
     SECTION("an absolute path") {
         const boost::filesystem::path absolute = (boost::filesystem::temp_directory_path() / "model.3mf").make_preferred();
         REQUIRE(resolve_cli_input_path(absolute.string()) == absolute.string());