Hold the POSIX Lock With flock and Leave a Moved-Aside File to the User

An fcntl lock belongs to the process and goes with the first close of
any other descriptor to the lock file, so a backup or an export walking
the data dir could drop the guard's lock without a trace. On POSIX the
guard holds a flock on its own open file instead, which nothing else in
the process can release; Windows keeps LockFileEx.

The Windows write probe opened the target for writing and took a
sharing violation for a refusal, so a file another process merely held
open was not saved at all; only a real denial refuses now, since the
rename that follows moves an open destination aside. A file moved aside
by a refused rename may be the last copy of a file or a file since
deleted on purpose, so the sweep logs it and leaves it to the user
rather than removing or restoring it. The sweep throttles itself per
directory, so a load followed by a save reads each directory once, and
the identity check on the lock file runs at most once a second, so a
scan of hundreds of presets pays for it once. A config that stays
unwritable backs off for longer with each failure in a row, and its
backup copy is written after the config, never before. The Windows
identity helper is shared with the rename that already computed it, and
the header comment that had lost its indentation and its neighbour's
description is whole again.

src/libslic3r/AppConfig.cpp

@@ -1323,17 +1323,23 @@ bool AppConfig::write_config_file(const std::string &path, std::string body, con
     // provided by Windows (sic!). Therefore we save a MD5 checksum to be able to verify file corruption. In addition,
     // we save the config file into a backup first before moving it to the final destination.
     body += appconfig_md5_hash_line(checksum_source);
+#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)) {
+        BOOST_LOG_TRIVIAL(error) << "Failed to write the configuration " << path << ": " << ec.message()
+                                 << "; trying again in " << m_retry_save_after.count() << " s";
+        m_retry_save_at    = std::chrono::steady_clock::now() + m_retry_save_after;
+        m_retry_save_after = std::min(m_retry_save_after * 2, std::chrono::seconds(300));
+        return false;
+    }
+    m_retry_save_at    = {};
+    m_retry_save_after = std::chrono::seconds(10);
+#ifdef WIN32
+    // Written after the config, so the backup never holds a state that was not confirmed written.
     const std::string backup_path = (boost::format("%1%.bak") % path).str();
     if (const std::error_code ec = write_file_atomically(backup_path, body, false, /*replace_read_only=*/true))
         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)) {
-        BOOST_LOG_TRIVIAL(error) << "Failed to write the configuration " << path << ": " << ec.message() << "; trying again in 10 s";
-        m_retry_save_at = std::chrono::steady_clock::now() + std::chrono::seconds(10);
-        return false;
-    }
-    m_retry_save_at = {};
     return true;
 }
 

src/libslic3r/AppConfig.hpp

@@ -456,13 +456,16 @@ private:
 	// Preset for each machine
 	MachineSettingMap											m_printer_settings;
 	// Writes the assembled config text, and on Windows its checksum and a backup copy; false when the
-// config itself could not be written, in which case the caller stays dirty and retries. `checksum_source`
+	// config itself could not be written, in which case the caller stays dirty and retries. `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);
 
+	// Has any value been modified since the config.ini has been last saved or loaded?
 	bool														m_dirty;
-	// After a failed write, save_due() is false until this point.
+	// After a failed write, save_due() is false until this point, which moves out
+	// ten seconds, then twenty, up to five minutes, for every failure in a row.
 	std::chrono::steady_clock::time_point						m_retry_save_at{};
+	std::chrono::seconds										m_retry_save_after{10};
 	// Original version found in the ini file before it was overwritten
 	Semver                                                      m_orig_version;
 	// Whether the existing version is before system profiles & configuration updating

src/libslic3r/InstanceLock.cpp

@@ -5,27 +5,63 @@
 #include <memory>
 #include <thread>
 
-#include <boost/interprocess/sync/file_lock.hpp>
-#include <boost/log/trivial.hpp>
 #include <boost/filesystem.hpp>
+#include <boost/log/trivial.hpp>
 #include <boost/nowide/fstream.hpp>
 #ifdef _WIN32
+#include <boost/interprocess/sync/file_lock.hpp>
 #include <boost/nowide/convert.hpp>
+#else
+#include <cerrno>
+#include <fcntl.h>
+#include <sys/file.h>
+#include <unistd.h>
 #endif
+#include <system_error>
 
 #include "Utils.hpp"
 
 namespace Slic3r {
 
-// One slot per lock file, shared by every guard in the process. A POSIX fcntl
-// lock is owned by the process, not the thread or descriptor, so a single
-// file_lock per path behind a mutex is what makes the guard re-entrant and
-// safe to use from the preset sync thread and the GUI thread at once.
+#ifdef _WIN32
+// LockFileEx, held by this handle alone.
+using NativeFileLock = boost::interprocess::file_lock;
+#else
+// flock(2) rather than an fcntl lock: it belongs to this open file description,
+// so any other code in the process that opens and closes the lock file, as a
+// backup or an export walking the data dir might, cannot drop it. An fcntl
+// lock would go with the first such close.
+class NativeFileLock
+{
+public:
+    explicit NativeFileLock(const char *path) : m_fd(::open(path, O_RDWR | O_CLOEXEC))
+    {
+        if (m_fd < 0)
+            throw std::system_error(errno, std::generic_category(), path);
+    }
+    ~NativeFileLock() { ::close(m_fd); }
+    bool try_lock()
+    {
+        if (::flock(m_fd, LOCK_EX | LOCK_NB) == 0)
+            return true;
+        if (errno == EWOULDBLOCK)
+            return false;
+        throw std::system_error(errno, std::generic_category(), "flock");
+    }
+    void unlock() { ::flock(m_fd, LOCK_UN); }
+private:
+    int m_fd;
+};
+#endif
+
+// One slot per lock file, shared by every guard in the process: one lock
+// object per path behind a mutex is what makes the guard re-entrant and safe
+// to use from the preset sync thread and the GUI thread at once.
 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;
+    std::unique_ptr<NativeFileLock> 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;
@@ -60,17 +96,17 @@ 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, read-write, and every user
-        // sharing the data dir has to be able to open it.
+        // The lock opens an existing file 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());
+        slot.file_lock = std::make_unique<NativeFileLock>(boost::nowide::widen(lock_file_path).c_str());
 #else
-        slot.file_lock = std::make_unique<boost::interprocess::file_lock>(lock_file_path.c_str());
+        slot.file_lock = std::make_unique<NativeFileLock>(lock_file_path.c_str());
 #endif
         slot.identity            = file_identity(lock_file_path);
         slot.identity_checked_at = std::chrono::steady_clock::now();

src/libslic3r/InstanceLock.hpp

@@ -9,7 +9,9 @@ namespace Slic3r {
 // Scoped write lock on a file shared by every running instance of the
 // application, such as the app config or the user preset directory. Threads
 // of this process are serialised through a recursive mutex, other processes
-// through an advisory OS file lock on `lock_file_path`, created on first use
+// through an advisory OS file lock on `lock_file_path` (flock on POSIX, tied to
+// the guard's own open file so nothing else in the process can drop it by
+// opening and closing the file; LockFileEx on Windows), created on first use
 // and kept: the lock state lives in the kernel on the open file, so deleting
 // the file on release would let a third instance lock a fresh file while the
 // second still holds the old one. The OS releases the lock when its holder
@@ -41,10 +43,10 @@ public:
     // 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: on every acquisition by default, one stat, since a
-    // handle to a replaced file would lock nothing anyone else can see.
-    // Mutable so tests can change it.
-    static inline std::chrono::milliseconds identity_check_interval{0};
+    // the one it opened, since a handle to a replaced file would lock nothing
+    // anyone else can see: a stat once a second bounds that window while a scan
+    // of hundreds of presets pays for it once. Mutable so tests can change it.
+    static inline std::chrono::milliseconds identity_check_interval{1000};
 
     // 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/Utils.hpp

@@ -235,16 +235,16 @@ extern std::error_code rename_file(const std::string &from, const std::string &t
 // target that is not a regular file (a symlink, device or pipe) is written in
 // place, since replacing it would change what it is, and so is a target whose
 // replace the filesystem refuses or beside which no temporary can be created.
-// A successful write into a directory under data_dir() also removes stale
-// leftovers there, at most once an hour per directory.
+// A successful write into a directory under data_dir() also sweeps stale
+// leftovers there.
 extern std::error_code write_file_atomically(const std::string &path, std::initializer_list<std::string_view> chunks, bool binary = false, bool replace_read_only = false);
 inline std::error_code write_file_atomically(const std::string &path, const std::string &content, bool binary = false, bool replace_read_only = false)
 	{ return write_file_atomically(path, { std::string_view(content) }, binary, replace_read_only); }
-// Remove the `<name>.<pid>.<n>.tmp` files a crashed write_file_atomically() and
-// the `<name>.<pid>.old` files a crashed rename_file() left in `dir` at least an
-// hour ago, only names starting with `name_prefix` when it is given; an `.old`
-// whose `<name>` is missing is the last copy and is put back instead. Returns how
-// many were removed.
+// Remove the `<name>.<pid>.<n>.tmp` files a crashed write_file_atomically() left
+// in `dir` at least an hour ago, only names starting with `name_prefix` when it is
+// given, and log the `<name>.<pid>.old` files a crashed rename_file() left, which
+// may be a last copy or a since-deleted file and so stay for the user. Runs at
+// most once an hour per directory and prefix. 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.

src/libslic3r/utils.cpp

@@ -458,6 +458,12 @@ boost::filesystem::path get_log_file_name()
 
 #ifdef _WIN32
 // The following helpers are borrowed from the LLVM project https://github.com/llvm
+// Names a file object by volume and file index, the same identity file_identity() reports.
+static std::string file_identity_of(const BY_HANDLE_FILE_INFORMATION &info)
+{
+	return std::to_string(info.dwVolumeSerialNumber) + ":" + std::to_string((static_cast<uint64_t>(info.nFileIndexHigh) << 32) | info.nFileIndexLow);
+}
+
 namespace WindowsSupport
 {
 	template <typename HandleTraits>
@@ -679,7 +685,7 @@ namespace WindowsSupport
 						BY_HANDLE_FILE_INFORMATION FI2;
 						if (! ::GetFileInformationByHandle(to_handle2, &FI2))
 							return map_windows_error(GetLastError());
-						if (FI.nFileIndexHigh != FI2.nFileIndexHigh || FI.nFileIndexLow != FI2.nFileIndexLow || FI.dwVolumeSerialNumber != FI2.dwVolumeSerialNumber)
+						if (file_identity_of(FI) != file_identity_of(FI2))
 							break;
 						continue;
 					}
@@ -740,10 +746,14 @@ std::error_code rename_file(const std::string &from, const std::string &to)
 static bool is_writable(const std::string &path)
 {
 #ifdef _WIN32
-	// _waccess() sees only the read-only attribute; an open for writing sees ACLs too.
+	// _waccess() sees only the read-only attribute; an open for writing sees
+	// ACLs too. Another process merely holding the file open is not a refusal:
+	// the rename that follows moves an open destination aside.
 	HANDLE handle = ::CreateFileW(boost::nowide::widen(path).c_str(), GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr);
-	if (handle == INVALID_HANDLE_VALUE)
-		return false;
+	if (handle == INVALID_HANDLE_VALUE) {
+		const DWORD err = ::GetLastError();
+		return err == ERROR_SHARING_VIOLATION || err == ERROR_LOCK_VIOLATION;
+	}
 	::CloseHandle(handle);
 	return true;
 #else
@@ -813,25 +823,10 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_
 		return write_whole_file(path, chunks, binary);
 	}
 	// Crash leftovers of earlier writes into this directory, for the files the
-	// application owns; once an hour per directory, so a batch of saves does
-	// not read the directory once per file.
+	// application owns; the sweep throttles itself per directory.
 	const boost::filesystem::path dir = boost::filesystem::path(path).parent_path();
-	if (! data_dir().empty() && boost::algorithm::starts_with(dir.generic_string(), boost::filesystem::path(data_dir()).generic_string())) {
-		static std::mutex swept_mutex;
-		static std::map<std::string, std::chrono::steady_clock::time_point> swept_at;
-		const auto now = std::chrono::steady_clock::now();
-		bool sweep = false;
-		{
-			std::lock_guard<std::mutex> guard(swept_mutex);
-			auto &last = swept_at[dir.string()];
-			if (last == std::chrono::steady_clock::time_point{} || now - last >= std::chrono::hours(1)) {
-				last  = now;
-				sweep = true;
-			}
-		}
-		if (sweep)
-			remove_stale_temp_files(dir);
-	}
+	if (! data_dir().empty() && boost::algorithm::starts_with(dir.generic_string(), boost::filesystem::path(data_dir()).generic_string()))
+		remove_stale_temp_files(dir);
 	return {};
 }
 
@@ -847,7 +842,7 @@ std::string file_identity(const std::string &path)
 	::CloseHandle(handle);
 	if (! ok)
 		return {};
-	return std::to_string(info.dwVolumeSerialNumber) + ":" + std::to_string((static_cast<uint64_t>(info.nFileIndexHigh) << 32) | info.nFileIndexLow);
+	return file_identity_of(info);
 #else
 	struct stat st;
 	if (::stat(path.c_str(), &st) != 0)
@@ -858,6 +853,18 @@ std::string file_identity(const std::string &path)
 
 size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::string &name_prefix)
 {
+	// Once an hour per directory: a batch of saves or a load followed by a
+	// save must not read the same directory over and over.
+	{
+		static std::mutex swept_mutex;
+		static std::map<std::string, std::chrono::steady_clock::time_point> swept_at;
+		const auto now = std::chrono::steady_clock::now();
+		std::lock_guard<std::mutex> guard(swept_mutex);
+		auto &last = swept_at[dir.string() + "|" + name_prefix];
+		if (last != std::chrono::steady_clock::time_point{} && now - last < std::chrono::hours(1))
+			return 0;
+		last = now;
+	}
 	// <name_prefix>...<pid>.<n>.tmp, exactly the shape write_file_atomically()
 	// makes, or <name_prefix>...<pid>.old, the shape rename_file() moves a
 	// target aside under.
@@ -900,14 +907,11 @@ size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::st
 			continue;
 		const std::string name = it->path().filename().string();
 		if (name.size() > 4 && name.compare(name.size() - 4, 4, ".old") == 0) {
-			// A target rename_file() moved aside and never put back or removed:
-			// the last copy of the file if the target is missing, so it goes back.
-			const boost::filesystem::path original = it->path().parent_path() / name.substr(0, name.rfind('.', name.size() - 5));
-			if (! boost::filesystem::exists(original, entry_ec)) {
-				boost::filesystem::rename(it->path(), original, entry_ec);
-				BOOST_LOG_TRIVIAL(warning) << "Restored " << original << " from " << it->path() << (entry_ec ? ": failed" : "");
-				continue;
-			}
+			// A target rename_file() moved aside and never put back or removed.
+			// It may be the last copy of a file, or a file since deleted on
+			// purpose; neither removing nor restoring it is safe unasked.
+			BOOST_LOG_TRIVIAL(warning) << it->path() << " was set aside by an interrupted save; restore or delete it by hand";
+			continue;
 		}
 		if (boost::filesystem::remove(it->path(), entry_ec)) {
 			BOOST_LOG_TRIVIAL(info) << "Removed stale temporary file " << it->path();

tests/libslic3r/test_instance_lock.cpp

@@ -11,6 +11,7 @@
 
 #ifndef _WIN32
 #include <fcntl.h>
+#include <sys/file.h>
 #include <sys/wait.h>
 #include <unistd.h>
 #endif
@@ -149,9 +150,9 @@ TEST_CASE("InstanceLock serialises the threads of one process", "[InstanceLock]"
 }
 
 #ifndef _WIN32
-// The cross-process side of the lock is a POSIX fcntl write lock, which a
-// child process takes here directly; the same primitive backs the guard on
-// Windows through LockFileEx, but spawning a child there is not worth a test.
+// The cross-process side of the lock is a POSIX flock, which a child process
+// takes here directly; LockFileEx backs the guard on Windows, but spawning a
+// child there is not worth a test.
 TEST_CASE("InstanceLock yields to another process and reports it", "[InstanceLock]")
 {
     ScopedTemporaryFile lock_file(".lock");
@@ -164,11 +165,8 @@ TEST_CASE("InstanceLock yields to another process and reports it", "[InstanceLoc
     const pid_t child = ::fork();
     REQUIRE(child >= 0);
     if (child == 0) {
-        int fd = ::open(path.c_str(), O_RDWR | O_CREAT, 0644);
-        struct flock lock{};
-        lock.l_type   = F_WRLCK;
-        lock.l_whence = SEEK_SET;
-        char byte = ::fcntl(fd, F_SETLK, &lock) == 0 ? '1' : '0';
+        int  fd   = ::open(path.c_str(), O_RDWR | O_CREAT, 0644);
+        char byte = ::flock(fd, LOCK_EX | LOCK_NB) == 0 ? '1' : '0';
         if (::write(child_holds[1], &byte, 1) != 1 || ::read(child_may_exit[0], &byte, 1) != 1)
             ::_exit(1);
         ::_exit(0);

tests/libslic3r/test_utils.cpp

@@ -187,17 +187,14 @@ TEST_CASE("write_file_atomically survives two threads writing one target", "[uti
     REQUIRE(temporaries == 0);
 }
 
-TEST_CASE("remove_stale_temp_files puts back a moved-aside file whose original is missing", "[utils]") {
+TEST_CASE("remove_stale_temp_files leaves a moved-aside file to the user", "[utils]") {
     ScopedTemporaryDir dir;
-    REQUIRE_FALSE(write_file_atomically((dir.path() / "lost.json.4242.old").string(), "last copy"));
+    REQUIRE_FALSE(write_file_atomically((dir.path() / "lost.json.4242.old").string(), "maybe the last copy"));
     boost::filesystem::last_write_time(dir.path() / "lost.json.4242.old", std::time(nullptr) - 7200);
 
     REQUIRE(remove_stale_temp_files(dir.path()) == 0);
-    REQUIRE(boost::filesystem::exists(dir.path() / "lost.json"));
-    REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "lost.json.4242.old"));
-    std::string content;
-    load_string_file(dir.path() / "lost.json", content);
-    REQUIRE(content == "last copy");
+    REQUIRE(boost::filesystem::exists(dir.path() / "lost.json.4242.old"));
+    REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "lost.json"));
 }
 
 TEST_CASE("remove_stale_temp_files removes only old <name>.<pid>.<n>.tmp files", "[utils]") {
@@ -211,22 +208,23 @@ TEST_CASE("remove_stale_temp_files removes only old <name>.<pid>.<n>.tmp files",
     // Just written: possibly another instance's in-flight save, so it stays.
     REQUIRE_FALSE(write_file_atomically((dir.path() / "g.json.7.2.tmp").string(), "x"));
 
-    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") == 2);
+    SECTION("with a name prefix only matching names go; a numbered backup, a one-segment name or an .old is not removed") {
+        REQUIRE(remove_stale_temp_files(dir.path(), "a.json") == 1);
         REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "a.json.123.7.tmp"));
-        REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "a.json.123.old"));
+        REQUIRE(boost::filesystem::exists(dir.path() / "a.json.123.old"));
         REQUIRE(boost::filesystem::exists(dir.path() / "a.json.99"));
         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 == 9);
+        REQUIRE(entries == 10);
+        REQUIRE(boost::filesystem::exists(dir.path() / "a.json.123.old"));
         REQUIRE(boost::filesystem::exists(dir.path() / "a.json"));
         REQUIRE(boost::filesystem::exists(dir.path() / "h.json.old"));
         REQUIRE(boost::filesystem::exists(dir.path() / "a.json.99"));