Open the Lock File Per Guard and Keep Directory Removal Out of the Lock

The lock file stayed open for the life of the process, which pinned the
data dir on Windows so the preset tests leaked a directory each, and
asked for a check that the file behind the path was still the one
opened. The outermost guard opens the file and closes it when it goes,
so whatever is at the path is what gets locked and nothing stays open
between saves; the identity check and its interval are gone with it.

Removing a bundle or a user folder held the lock for the whole tree, and
the wait for the in-process mutex is bounded only by the longest
critical section, so a save on the GUI thread could wait for a tree of
files to go on a slow share. The tree is renamed aside under the lock in
one step and removed afterwards. A bundle import extracts into a
per-process folder, so two instances importing at once do not clear
each other's extraction. A lock file that cannot be opened or locked
backs off for longer with each failure, like a timed-out wait does; the
preset scan's three removal sites share one helper; a restore that fails
after a refused rename names where the previous content went; and the
warnings about a set-aside file and an in-place fallback are logged once
per file.

src/libslic3r/AppConfig.cpp

@@ -5,6 +5,7 @@
 //BBS
 #include "Preset.hpp"
 #include "Exception.hpp"
+#include "InstanceLock.hpp"
 #include "LocalesUtils.hpp"
 #include "Thread.hpp"
 #include "format.hpp"
@@ -730,10 +731,17 @@ static bool verify_config_file_checksum(boost::nowide::ifstream &ifs)
 
 
 #ifdef USE_JSON_CONFIG
-std::string AppConfig::load()
+std::string AppConfig::load(bool read_only)
 {
     json j;
 
+    // Keep another instance from replacing or restoring the file mid-read.
+    InstanceLock instance_lock(read_only ? std::string() : lock_path());
+    if (instance_lock.locked()) {
+        const boost::filesystem::path conf(config_path());
+        remove_stale_temp_files(conf.parent_path(), conf.filename().string());
+    }
+
     // 1) Read the complete config file into a boost::property_tree.
     namespace pt = boost::property_tree;
     pt::ptree tree;
@@ -983,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();
-    std::string path_pid = (boost::format("%1%.%2%") % path % get_current_pid()).str();
 
     json j;
 
@@ -1113,43 +1120,22 @@ void AppConfig::save()
 
         j["local_machines"][local_machine.first] = m_json;
     }
-    boost::nowide::ofstream c;
+    const std::string config_str = j.dump(1, '\t');
-    c.open(path_pid, std::ios::out | std::ios::trunc);
+    if (write_config_file(path, config_str + "\n", config_str))
-    c << j.dump(1, '\t') << std::endl;
+        m_dirty = false;
-
-#ifdef WIN32
-    // WIN32 specific: The final "rename_file()" call 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,
-    // we save the config file into a backup first before moving it to the final destination.
-    c << appconfig_md5_hash_line(j.dump(1, '\t'));
-#endif
-
-    c.close();
-    if (c.fail()) {
-      BOOST_LOG_TRIVIAL(error) << "Failed to write new configuration to " << path_pid << "; aborting attempt to overwrite original configuration";
-      return;
-    }
-
-#ifdef WIN32
-    // Make a backup of the configuration file before copying it to the final destination.
-    std::string error_message;
-    std::string backup_path = (boost::format("%1%.bak") % path).str();
-    // Copy configuration file with PID suffix into the configuration file with "bak" suffix.
-    if (copy_file(path_pid, backup_path, error_message, false) != SUCCESS)
-        BOOST_LOG_TRIVIAL(error) << "Copying from " << path_pid << " to " << backup_path << " failed. Failed to create a backup configuration.";
-#endif
-
-    // Rename the config atomically.
-    // On Windows, the rename is likely NOT atomic, thus it may fail if PrusaSlicer crashes on another thread in the meanwhile.
-    // To cope with that, we already made a backup of the config on Windows.
-    rename_file(path_pid, path);
-    m_dirty = false;
 }
 
 #else
 
-std::string AppConfig::load()
+std::string AppConfig::load(bool read_only)
 {
+    // Keep another instance from replacing or restoring the file mid-read.
+    InstanceLock instance_lock(read_only ? std::string() : lock_path());
+    if (instance_lock.locked()) {
+        const boost::filesystem::path conf(config_path());
+        remove_stale_temp_files(conf.parent_path(), conf.filename().string());
+    }
+
     // 1) Read the complete config file into a boost::property_tree.
     namespace pt = boost::property_tree;
     pt::ptree tree;
@@ -1287,7 +1273,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();
-    std::string path_pid = (boost::format("%1%.%2%") % path % get_current_pid()).str();
 
     std::stringstream config_ss;
     if (m_mode == EAppMode::Editor)
@@ -1323,39 +1308,41 @@ void AppConfig::save()
     // One empty line before the MD5 sum.
     config_ss << std::endl;
 
-    std::string config_str = config_ss.str();
+    const std::string config_str = config_ss.str();
-    boost::nowide::ofstream c;
+    if (write_config_file(path, config_str, config_str))
-    c.open(path_pid, std::ios::out | std::ios::trunc);
+        m_dirty = false;
-    c << config_str;
-#ifdef WIN32
-    // WIN32 specific: The final "rename_file()" call 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,
-    // we save the config file into a backup first before moving it to the final destination.
-    c << appconfig_md5_hash_line(config_str);
-#endif
-    c.close();
-    if (c.fail()) {
-      BOOST_LOG_TRIVIAL(error) << "Failed to write new configuration to " << path_pid << "; aborting attempt to overwrite original configuration";
-      return;
-    }
-
-#ifdef WIN32
-    // Make a backup of the configuration file before copying it to the final destination.
-    std::string error_message;
-    std::string backup_path = (boost::format("%1%.bak") % path).str();
-    // Copy configuration file with PID suffix into the configuration file with "bak" suffix.
-    if (copy_file(path_pid, backup_path, error_message, false) != SUCCESS)
-        BOOST_LOG_TRIVIAL(error) << "Copying from " << path_pid << " to " << backup_path << " failed. Failed to create a backup configuration.";
-#endif
-
-    // Rename the config atomically.
-    // On Windows, the rename is likely NOT atomic, thus it may fail if PrusaSlicer crashes on another thread in the meanwhile.
-    // To cope with that, we already made a backup of the config on Windows.
-    rename_file(path_pid, path);
-    m_dirty = false;
 }
 #endif
 
+bool 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());
+#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,
+    // 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
+    return true;
+}
+
 bool AppConfig::get_variant(const std::string &vendor, const std::string &model, const std::string &variant) const
 {
     const auto it_v = m_vendors.find(vendor);
@@ -1844,6 +1831,11 @@ void AppConfig::reset_selections()
     }
 }
 
+std::string AppConfig::lock_path()
+{
+    return Slic3r::data_dir().empty() ? std::string() : config_path() + ".lock";
+}
+
 std::string AppConfig::config_path()
 {
 #ifdef USE_JSON_CONFIG
@@ -1880,7 +1872,7 @@ bool AppConfig::exists()
 
 std::string AppConfig::load_if_exists()
 {
-    return boost::filesystem::exists(loading_path()) ? load() : std::string();
+    return boost::filesystem::exists(loading_path()) ? load(/*read_only=*/true) : std::string();
 }
 
 }; // namespace Slic3r

src/libslic3r/AppConfig.hpp

@@ -2,6 +2,7 @@
 #define slic3r_AppConfig_hpp_
 
 #include <set>
+#include <chrono>
 #include <map>
 #include <string>
 #include "nlohmann/json.hpp"
@@ -121,14 +122,18 @@ public:
 
 	// Load the slic3r.ini from a user profile directory (or a datadir, if configured).
 	// Return an error string, or an empty string on success.
-	std::string         load();
+	std::string         load(bool read_only = false);
 	// Treat a missing config as default state; otherwise load it normally.
+	// The CLI's load: it never saves, so it takes no lock and creates no lock file.
 	std::string         load_if_exists();
 	// Store the slic3r.ini into a user profile directory (or a datadir, if configured).
 	void 			   	save();
 
 	// Does this config need to be saved?
 	bool 				dirty() const { return m_dirty; }
+	// False for ten seconds after a failed write, so the idle handler does not
+	// repeat a hopeless attempt on every event; an explicit save() always tries.
+	bool 				save_due() const { return std::chrono::steady_clock::now() >= m_retry_save_at; }
 
 
 	void				set_dirty() { m_dirty = true; }
@@ -338,6 +343,8 @@ public:
 
 	// Get the default config path from Slic3r::data_dir().
 	std::string			config_path();
+	// Lock file guarding config_path() against other running instances; empty without a data dir.
+	std::string			lock_path();
 
 	// Returns true if the user's data directory comes from before Slic3r 1.40.0 (no updating)
 	bool 				legacy_datadir() const { return m_legacy_datadir; }
@@ -448,8 +455,17 @@ 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`
+	// 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, 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/CMakeLists.txt

@@ -304,6 +304,8 @@ set(lisbslic3r_sources
     Geometry/VoronoiUtils.cpp
     Geometry/VoronoiUtils.hpp
     Geometry/VoronoiVisualUtils.hpp
+    InstanceLock.cpp
+    InstanceLock.hpp
     Int128.hpp
     KDTreeIndirect.hpp
     Layer.cpp

src/libslic3r/Config.cpp

@@ -1522,12 +1522,10 @@ void ConfigBase::save_to_json(const std::string &file, const std::string &name,
     // Serialize first: if that throws (invalid UTF-8), the existing file stays untouched.
     std::ostringstream ss;
     this->save_to_json(ss, name, from, version);
-    boost::nowide::ofstream c;
+    if (const std::error_code ec = write_file_atomically(file, ss.str()))
-    c.open(file, std::ios::out | std::ios::trunc);
+        BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << boost::format(": failed to save config to %1%: %2%") % file % ec.message();
-    c << ss.str();
+    else
-    c.close();
+        BOOST_LOG_TRIVIAL(info) << __FUNCTION__ << ":" <<__LINE__ << boost::format(", saved config to %1%\n")%file;
-
-    BOOST_LOG_TRIVIAL(info) << __FUNCTION__ << ":" <<__LINE__ << boost::format(", saved config to %1%\n")%file;
 }
 
 void ConfigBase::save_to_json(std::ostream &os, const std::string &name, const std::string &from, const std::string &version, bool replace_invalid_utf8) const

src/libslic3r/InstanceLock.cpp

@@ -0,0 +1,186 @@
+#include "InstanceLock.hpp"
+
+#include <algorithm>
+#include <map>
+#include <memory>
+#include <system_error>
+#include <thread>
+
+#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
+
+namespace Slic3r {
+
+#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. The lock
+// file is opened by the outermost guard and closed when it goes, so the file
+// is never held open between guards: whatever is at the path is what gets
+// locked, and a data dir can be removed once nothing is saving into it.
+struct InstanceLock::Slot
+{
+    std::recursive_mutex mutex;
+    std::unique_ptr<NativeFileLock> file_lock;
+    int  depth{0};
+    bool file_locked{false};
+    // After a timed-out wait, guards skip waiting until this point; each wait
+    // that times out in a row doubles the next cool-down, up to a few minutes.
+    std::chrono::steady_clock::time_point skip_waiting_until{};
+    int consecutive_timeouts{0};
+    // After a failed open or a failing lock call, guards skip the file lock
+    // entirely until this point, again doubling for failures in a row.
+    std::chrono::steady_clock::time_point retry_at{};
+    int consecutive_failures{0};
+};
+
+InstanceLock::Slot &InstanceLock::slot_for(const std::string &lock_file_path)
+{
+    // Never freed: a save during static destruction still needs its 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::unique_ptr<Slot> &slot = (*registry)[lock_file_path];
+    if (! slot)
+        slot = std::make_unique<Slot>();
+    return *slot;
+}
+
+static std::chrono::milliseconds backoff_for(int failures_in_a_row, std::chrono::milliseconds base)
+{
+    return std::min(base * (1 << std::min(failures_in_a_row, 5)), std::chrono::milliseconds(std::chrono::minutes(5)));
+}
+
+// Creates the lock file if needed and opens it, or starts the cool-down.
+// Called with the slot mutex held.
+bool InstanceLock::open_lock_file(Slot &slot, const std::string &lock_file_path)
+{
+    try {
+        // 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<NativeFileLock>(boost::nowide::widen(lock_file_path).c_str());
+#else
+        slot.file_lock = std::make_unique<NativeFileLock>(lock_file_path.c_str());
+#endif
+        return true;
+    } catch (const std::exception &e) {
+        const auto backoff = backoff_for(slot.consecutive_failures ++, cooldown);
+        slot.retry_at      = std::chrono::steady_clock::now() + backoff;
+        BOOST_LOG_TRIVIAL(warning) << "Cannot open lock file " << lock_file_path << ": " << e.what()
+                                   << " (check its owner and permissions); other instances are not excluded from writing for the next "
+                                   << backoff.count() << " ms";
+        return false;
+    }
+}
+
+InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milliseconds timeout)
+{
+    if (lock_file_path.empty())
+        return;
+    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();
+    if (m_slot->depth == 0 && now >= m_slot->retry_at && open_lock_file(*m_slot, lock_file_path)) {
+        const bool wait     = now >= m_slot->skip_waiting_until;
+        const auto deadline = now + timeout;
+        for (;;) {
+            try {
+                if (m_slot->file_lock->try_lock()) {
+                    m_slot->file_locked          = true;
+                    m_slot->consecutive_timeouts = 0;
+                    m_slot->consecutive_failures = 0;
+                    m_slot->skip_waiting_until   = {};
+                    break;
+                }
+            } catch (const std::exception &e) {
+                const auto backoff = backoff_for(m_slot->consecutive_failures ++, cooldown);
+                m_slot->retry_at   = std::chrono::steady_clock::now() + backoff;
+                BOOST_LOG_TRIVIAL(warning) << "Cannot lock " << lock_file_path << ": " << e.what()
+                                           << "; proceeding without the lock for the next " << backoff.count() << " ms";
+                break;
+            }
+            if (! wait)
+                break;
+            if (std::chrono::steady_clock::now() >= deadline) {
+                const auto backoff = backoff_for(m_slot->consecutive_timeouts ++, cooldown);
+                m_slot->skip_waiting_until = deadline + backoff;
+                BOOST_LOG_TRIVIAL(warning) << "Another instance has held " << lock_file_path << " for over "
+                                           << timeout.count() << " ms; proceeding without the lock for the next "
+                                           << backoff.count() << " ms";
+                break;
+            }
+            std::this_thread::sleep_for(std::chrono::milliseconds(5));
+        }
+        if (! m_slot->file_locked)
+            m_slot->file_lock.reset();
+    }
+    // Counted last, so a throw above leaves the slot exactly as it was found.
+    ++ m_slot->depth;
+    m_locked = m_slot->file_locked;
+}
+
+InstanceLock::~InstanceLock()
+{
+    if (m_slot == nullptr)
+        return;
+    if (-- m_slot->depth == 0 && m_slot->file_lock) {
+        if (m_slot->file_locked) {
+            try {
+                m_slot->file_lock->unlock();
+            } catch (const std::exception &e) {
+                BOOST_LOG_TRIVIAL(warning) << "Cannot unlock instance lock: " << e.what();
+            }
+            m_slot->file_locked = false;
+        }
+        m_slot->file_lock.reset();
+    }
+}
+
+} // namespace Slic3r

src/libslic3r/InstanceLock.hpp

@@ -0,0 +1,72 @@
+#pragma once
+
+#include <chrono>
+#include <mutex>
+#include <string>
+
+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` (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). The file is created on
+// first use and kept, since the lock state lives in the kernel on the open
+// file and deleting the file on release would let a third instance lock a
+// fresh file while the second still holds the old one; it is opened by the
+// outermost guard and closed when that guard goes, so whatever is at the path
+// is what gets locked and nothing stays open between saves. The OS releases
+// the lock when its holder exits, so a crashed instance never leaves a stale
+// lock behind.
+//
+// The lock is best effort: when the lock file cannot be created, or another
+// instance still holds it after `timeout` (a second by default), the guard keeps only the in-process
+// mutex and locked() reports false. Writes then proceed unprotected rather than
+// letting one hung instance block every other one from saving. After such a
+// timeout the same lock file is not waited on again for `cooldown`, doubling
+// for every further timeout in a row up to a few minutes, so a batch of saves
+// pays the wait once rather than once per file and a holder that never lets go
+// does not cost a stall every cool-down for good; a lock file that could
+// not be opened, or a lock call that fails outright (a share without a lock
+// service), is likewise retried only after `cooldown`, doubling the same way.
+// A wait for the in-process mutex is bounded only by the longest critical
+// section, which is why every guard covers a few file operations and nothing
+// slower, such as removing a directory tree.
+//
+// Lock order: the preset collection mutex may be held when a guard is taken
+// (set_sync_info_and_save() calls save_info() under it), never the reverse.
+// That is why the guards sit at the leaf readers and writers, and why a
+// batch-level guard around save_user_presets(), which takes the collection
+// mutex through delete_preset(), must not be added.
+class InstanceLock
+{
+public:
+    // Long against a critical section of milliseconds, short against the GUI
+    // thread, which is where most guards are taken.
+    static constexpr std::chrono::milliseconds default_timeout{1000};
+    // 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};
+
+    // An empty path makes the guard a no-op.
+    explicit InstanceLock(const std::string &lock_file_path, std::chrono::milliseconds timeout = default_timeout);
+    ~InstanceLock();
+
+    InstanceLock(const InstanceLock &) = delete;
+    InstanceLock &operator=(const InstanceLock &) = delete;
+
+    // True while this process holds the cross-process file lock.
+    bool locked() const { return m_locked; }
+
+private:
+    struct Slot;
+    static Slot &slot_for(const std::string &lock_file_path);
+    static bool  open_lock_file(Slot &slot, const std::string &lock_file_path);
+
+    Slot *m_slot{nullptr};
+    std::unique_lock<std::recursive_mutex> m_slot_guard;
+    bool  m_locked{false};
+};
+
+} // namespace Slic3r

src/libslic3r/Preset.cpp

@@ -48,6 +48,9 @@
 
 #include "libslic3r.h"
 #include "Utils.hpp"
+#include "InstanceLock.hpp"
+
+#include <sstream>
 #include "Time.hpp"
 #include "PlaceholderParser.hpp"
 #include "libslic3r/GCode/Thumbnails.hpp"
@@ -104,6 +107,43 @@ std::string get_preset_canonical_name(const std::string &preset_bare_name, const
     }
 }
 
+std::string user_presets_lock_path(bool read_only)
+{
+    return read_only || data_dir().empty() ? std::string() : (fs::path(data_dir()) / (PRESET_USER_DIR ".lock")).string();
+}
+
+// Removes a preset file the scan could not load, and its .info, under the lock.
+static void remove_preset_files(const std::string &preset_file, bool read_only)
+{
+    if (read_only)
+        return;
+    InstanceLock instance_lock(user_presets_lock_path());
+    boost::system::error_code ec;
+    fs::path file_path(preset_file);
+    fs::remove(file_path, ec);
+    file_path.replace_extension(".info");
+    fs::remove(file_path, ec);
+}
+
+void remove_directory_tree_locked(const boost::filesystem::path &dir)
+{
+    boost::system::error_code ec;
+    fs::path doomed = dir;
+    doomed += "." + std::to_string(get_current_pid()) + ".removing";
+    {
+        InstanceLock instance_lock(user_presets_lock_path());
+        if (! fs::exists(dir, ec))
+            return;
+        fs::rename(dir, doomed, ec);
+        if (ec) {
+            // Cannot be set aside: the slow way, still under the lock.
+            fs::remove_all(dir, ec);
+            return;
+        }
+    }
+    fs::remove_all(doomed, ec);
+}
+
 std::string get_preset_bare_name(const std::string &canonical_name)
 {
     const auto pos = canonical_name.find_last_of('/');
@@ -646,18 +686,20 @@ void Preset::save_info(std::string file)
         file = idx_file.string();
     }
 
-    boost::nowide::ofstream c;
-    c.open(file, std::ios::out | std::ios::trunc);
     std::string sync_info_to_save;
     //BBS: hold is used for stop requesting to server this time
     if (this->sync_info.compare("hold") != 0)
         sync_info_to_save = this->sync_info;
+    std::ostringstream c;
     c << "sync_info" << " = " << sync_info_to_save << std::endl;
     c << "user_id" << " = " << this->user_id << std::endl;
     c << "setting_id" << " = " << this->setting_id << std::endl;
     c << "base_id" << " = " << this->base_id << std::endl;
     c << "updated_time" << " = " << std::to_string(this->updated_time) << std::endl;
-    c.close();
+
+    InstanceLock instance_lock(user_presets_lock_path());
+    if (const std::error_code ec = write_file_atomically(file, c.str()))
+        BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << ": failed to save " << file << ": " << ec.message();
 }
 
 void Preset::remove_files(bool cloud_already_deleted)
@@ -666,6 +708,7 @@ void Preset::remove_files(bool cloud_already_deleted)
     if (this->is_project_embedded) {
         return;
     }
+    InstanceLock instance_lock(user_presets_lock_path());
     // Erase the preset file.
     boost::nowide::remove(this->file.c_str());
     fs::path idx_path(this->file);
@@ -702,12 +745,16 @@ void Preset::save(DynamicPrintConfig* parent_config)
     else
         from_str = std::string("Default");
 
-    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;
@@ -743,15 +790,22 @@ 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());
+        to_save = &temp_config;
-    } else {
-        this->config.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;
+
+    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());
+    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()) {
@@ -770,6 +824,7 @@ void Preset::reload(Preset const &parent)
     std::string                        reason;
     ForwardCompatibilitySubstitutionRule substitution_rule    = ForwardCompatibilitySubstitutionRule::Disable;
     try {
+        InstanceLock instance_lock(user_presets_lock_path());
         ConfigSubstitutions                config_substitutions = config.load_from_json(file, substitution_rule, key_values, reason);
         this->config = parent.config;
         this->config.apply(std::move(config));
@@ -1704,6 +1759,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);
 
+    {
+        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))
     {
@@ -1725,30 +1785,31 @@ 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()) {
+                            remove_preset_files(preset.file, read_only);
+                            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);
@@ -1832,23 +1893,13 @@ void PresetCollection::load_presets(
                 } catch (const std::ifstream::failure &err) {
                     ++m_errors;
                     BOOST_LOG_TRIVIAL(error) << boost::format("The user-config cannot be loaded: %1%. Reason: %2%")%preset.file %err.what();
-                    fs::path file_path(preset.file);
+                    remove_preset_files(preset.file, read_only);
-                    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);
                     //throw Slic3r::RuntimeError(std::string("The selected preset cannot be loaded: ") + preset.file + "\n\tReason: " + err.what());
                 } catch (const std::runtime_error &err) {
                     ++m_errors;
                     BOOST_LOG_TRIVIAL(error) << boost::format("Failed loading the user-config file: %1%. Reason: %2%")%preset.file %err.what();
                     //throw Slic3r::RuntimeError(std::string("Failed loading the preset file: ") + preset.file + "\n\tReason: " + err.what());
-                    fs::path file_path(preset.file);
+                    remove_preset_files(preset.file, read_only);
-                    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);
                 }
 
                 if (preset_loaded_fn != nullptr)
@@ -4201,8 +4252,15 @@ void PhysicalPrinter::update_preset_names_in_config()
     }
 }
 
+void PhysicalPrinter::save(DynamicPrintConfig* /* parent_config */)
+{
+    InstanceLock instance_lock(user_presets_lock_path());
+    this->config.save_to_json(this->file, std::string("Physical_Printer"), std::string("User"), std::string(SLIC3R_VERSION));
+}
+
 void PhysicalPrinter::save(const std::string& file_name_from, const std::string& file_name_to)
 {
+    InstanceLock instance_lock(user_presets_lock_path());
     // rename the file
     boost::nowide::rename(file_name_from.data(), file_name_to.data());
     this->file = file_name_to;
@@ -4334,6 +4392,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.
@@ -4526,7 +4585,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;
 }
@@ -4538,7 +4600,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/Preset.hpp

@@ -484,6 +484,18 @@ std::string get_preset_canonical_name(const std::string &preset_bare_name, const
 // Tail segment of a canonical name — what's written to the bundle's .json filename and JSON "name" field.
 std::string get_preset_bare_name(const std::string &canonical_name);
 
+// Lock file guarding every user preset file under data_dir() against other
+// running instances and the preset sync thread. Empty without a data dir, and
+// for a read-only load (the CLI), which never rewrites or deletes and may run
+// many jobs on one data dir.
+std::string user_presets_lock_path(bool read_only = false);
+
+// Removes a directory tree under the user preset lock without holding the lock
+// for the removal itself: the tree is renamed aside under the lock, in one
+// step, and deleted afterwards, so a save waiting on the lock waits
+// milliseconds rather than for a tree of files to go.
+void remove_directory_tree_locked(const boost::filesystem::path &dir);
+
 // Resolve an origin from a directory path when the caller passes Kind::Auto.
 PresetOrigin detect_origin_from_path(const boost::filesystem::path &path, const PresetOrigin &explicit_origin = PresetOrigin());
 
@@ -1053,7 +1065,7 @@ public:
 
     //BBS: change to json format
     //void                save() { this->config.save(this->file); }
-    void                save(DynamicPrintConfig* parent_config) { this->config.save_to_json(this->file, std::string("Physical_Printer"), std::string("User"), std::string(SLIC3R_VERSION)); }
+    void                save(DynamicPrintConfig* parent_config);
     void                save(const std::string& file_name_from, const std::string& file_name_to);
 
     void                update_from_preset(const Preset& preset);

src/libslic3r/PresetBundle.cpp

@@ -12,6 +12,7 @@
 #include "libslic3r.h"
 #include "I18N.hpp"
 #include "Utils.hpp"
+#include "InstanceLock.hpp"
 #include "LocalesUtils.hpp"
 #include "Model.hpp"
 #include "TriangleSelector.hpp"
@@ -1227,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)) {
@@ -1240,7 +1250,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())) continue;
+            if (!load_bundle_metadata(entry.path(), metadata_file, metadata)) continue;
             metadata.print_presets.clear();
             metadata.filament_presets.clear();
             metadata.printer_presets.clear();
@@ -1275,7 +1285,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())) continue;
+            if (!load_bundle_metadata(entry.path(), metadata_file, metadata)) continue;
             metadata.print_presets.clear();
             metadata.filament_presets.clear();
             metadata.printer_presets.clear();
@@ -1609,7 +1619,8 @@ PresetsConfigSubstitutions PresetBundle::import_presets(std::vector<std::string>
             if (ec) BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << " create directory failed: " << ec.message();
             //create temp folder
             //std::string user_default_temp_dir = data_dir() + "/" + PRESET_USER_DIR + "/" + DEFAULT_USER_FOLDER_NAME + "/" + "temp";
-            fs::path temp_folder(configs_folder / "temp");
+            // Per process, so two instances importing at once do not clear each other's extraction.
+            fs::path temp_folder(configs_folder / ("temp_" + std::to_string(get_current_pid())));
             std::string user_default_temp_dir = temp_folder.make_preferred().string();
             if (fs::exists(temp_folder)) fs::remove_all(temp_folder);
             fs::create_directory(temp_folder, ec);
@@ -2231,10 +2242,7 @@ void PresetBundle::remove_user_presets_directory(const std::string preset_folder
         return;
     }
     BOOST_LOG_TRIVIAL(debug) << __FUNCTION__ << boost::format(" enter, delete directory : %1%") % dir_user_presets;
-    fs::path folder(dir_user_presets);
+    remove_directory_tree_locked(fs::path(dir_user_presets));
-    if (fs::exists(folder)) {
-        fs::remove_all(folder);
-    }
 }
 
 void PresetBundle::update_system_preset_setting_ids(std::map<std::string, std::map<std::string, std::string>>& system_presets)
@@ -7932,9 +7940,13 @@ bool BundleMetadata::save_to_json(const std::string& path) const
         j["filament_presets"] = strip_prefix(this->filament_presets);                                                                                                                                                                                                                       
         j["printer_presets"] = strip_prefix(this->printer_presets);
 
-        boost::nowide::ofstream ofs(path);
+        const std::string content = j.dump(4);
-        ofs << j.dump(4);
+        InstanceLock instance_lock(user_presets_lock_path());
-        return ofs.good();
+        if (const std::error_code ec = write_file_atomically(path, content)) {
+            BOOST_LOG_TRIVIAL(error) << "Failed to save bundle metadata to " << path << ": " << ec.message();
+            return false;
+        }
+        return true;
     } catch (const std::exception& e) {
         BOOST_LOG_TRIVIAL(error) << "Failed to save bundle metadata to " << path << ": " << e.what();
         return false;

src/libslic3r/PresetCacheFormat.cpp

@@ -400,46 +400,24 @@ bool write_cache_blob(const std::string& path, const std::string& blob)
 {
     boost::crc_32_type crc;
     crc.process_bytes(blob.data(), blob.size());
-    // Written beside the target and moved into place, as AppConfig::save does:
+    // Written beside the target and moved into place: a cache is truncated and
-    // a cache is truncated and rewritten in full, so a write that dies partway
+    // rewritten in full, so a write that dies partway would otherwise leave a
-    // would otherwise leave a header claiming more body than the file holds.
+    // header claiming more body than the file holds.
-    // The PID suffix also keeps two instances writing the same vendor from
-    // interleaving.
-    const std::string tmp_path = path + "." + std::to_string(get_current_pid()) + ".tmp";
     try {
         boost::filesystem::create_directories(boost::filesystem::path(path).parent_path());
-        {
+        CacheFileHeader fhdr;
-            boost::nowide::ofstream ofs(tmp_path, std::ios::binary | std::ios::trunc);
+        fhdr.magic     = CACHE_MAGIC;
-            if (!ofs.is_open()) {
+        fhdr.version   = CACHE_VERSION;
-                BOOST_LOG_TRIVIAL(warning) << "VendorCacheFile: cannot open for writing: " << tmp_path;
+        fhdr.data_size = static_cast<uint64_t>(blob.size());
-                return false;
+        fhdr.crc32     = crc.checksum();
-            }
+        const std::string_view header(reinterpret_cast<const char*>(&fhdr), sizeof(fhdr));
-            CacheFileHeader fhdr;
+        if (const std::error_code ec = write_file_atomically(path, { header, std::string_view(blob) }, /*binary=*/true)) {
-            fhdr.magic     = CACHE_MAGIC;
+            BOOST_LOG_TRIVIAL(warning) << "VendorCacheFile: write failed (" << path << "): " << ec.message();
-            fhdr.version   = CACHE_VERSION;
-            fhdr.data_size = static_cast<uint64_t>(blob.size());
-            fhdr.crc32     = crc.checksum();
-            ofs.write(reinterpret_cast<const char*>(&fhdr), sizeof(fhdr));
-            ofs.write(blob.data(), static_cast<std::streamsize>(blob.size()));
-            ofs.close();   // flush; close() raises failbit on error
-            if (! ofs.good()) {
-                BOOST_LOG_TRIVIAL(warning) << "VendorCacheFile: write failed (" << tmp_path << ")";
-                boost::system::error_code ec;
-                boost::filesystem::remove(tmp_path, ec);
-                return false;
-            }
-        }
-        if (const std::error_code ec = rename_file(tmp_path, path)) {
-            BOOST_LOG_TRIVIAL(warning) << "VendorCacheFile: could not move " << tmp_path << " into place: " << ec.message();
-            boost::system::error_code rm;
-            boost::filesystem::remove(tmp_path, rm);
             return false;
         }
         return true;
     } catch (const std::exception& e) {
         BOOST_LOG_TRIVIAL(warning) << "VendorCacheFile: write failed (" << path << "): " << e.what();
-        boost::system::error_code ec;
-        boost::filesystem::remove(tmp_path, ec);
         return false;
     }
 }

src/libslic3r/Utils.hpp

@@ -8,6 +8,8 @@
 #include <functional>
 #include <type_traits>
 #include <system_error>
+#include <initializer_list>
+#include <string_view>
 #include <regex>
 
 #include <boost/system/error_code.hpp>
@@ -224,6 +226,26 @@ extern std::vector<std::string> split_string(const std::string &str, char delimi
 // On Windows, the file explorer (or anti-virus or whatever else) often locks the file
 // for a short while, so the file may not be movable. Retry while we see recoverable errors.
 extern std::error_code rename_file(const std::string &from, const std::string &to);
+// Write `chunks`, in order, to `path` through a temporary file beside it that is
+// then renamed over the target, so a concurrent reader sees the old or the new
+// file, never a partial one. The temporary is removed on failure and an existing
+// target keeps its permissions. A target this process may not write is refused,
+// as an in-place write would be, unless `replace_read_only` is set for a file
+// that was always replaced rather than written, such as the app config. A
+// 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 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() 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());
 
 enum CopyFileResult {
 	SUCCESS = 0,

src/libslic3r/utils.cpp

@@ -9,6 +9,13 @@
 #include <stdio.h>
 #include <filesystem>
 #include <sstream>
+#include <cerrno>
+#include <chrono>
+#include <map>
+#include <set>
+#include <mutex>
+#include <thread>
+#include <cstring>
 #include <iomanip>
 #include <algorithm>
 #include <cmath>
@@ -34,6 +41,7 @@
 #else
 	#include <unistd.h>
 	#include <sys/types.h>
+	#include <sys/stat.h>
 	#include <sys/param.h>
     #include <sys/resource.h>
 	#ifdef BSD
@@ -44,7 +52,6 @@
 		#include <libproc.h>
 	#endif
 	#ifdef __linux__
-		#include <sys/stat.h>
 		#include <fcntl.h>
 		#include <sys/sendfile.h>
 		#include <dirent.h>
@@ -452,6 +459,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.
+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>
@@ -673,7 +686,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;
 					}
@@ -702,13 +715,209 @@ namespace WindowsSupport
 std::error_code rename_file(const std::string &from, const std::string &to)
 {
 #ifdef _WIN32
+	// Retries and moves an open destination aside itself.
 	return WindowsSupport::rename(from, to);
 #else
-	boost::nowide::remove(to.c_str());
+	// rename(2) replaces an existing target atomically; removing it first would
-	return std::make_error_code(static_cast<std::errc>(boost::nowide::rename(from.c_str(), to.c_str())));
+	// leave a window in which the file does not exist at all.
+	if (boost::nowide::rename(from.c_str(), to.c_str()) == 0)
+		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; 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 == EBUSY || err == ENOTSUP || err == EOPNOTSUPP;
+	if (replace_refused) {
+		// Named so remove_stale_temp_files() can clear it after a crash in between.
+		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 {};
+			}
+			if (boost::nowide::rename(aside.c_str(), to.c_str()) != 0)
+				BOOST_LOG_TRIVIAL(error) << "Could not put " << to << " back after a failed replace; its previous content is at " << aside;
+		}
+	}
+	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
+	// _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) {
+		const DWORD err = ::GetLastError();
+		return err == ERROR_SHARING_VIOLATION || err == ERROR_LOCK_VIOLATION;
+	}
+	::CloseHandle(handle);
+	return true;
+#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;
+	boost::nowide::ofstream out(path, std::ios::out | std::ios::trunc | (binary ? std::ios::binary : std::ios::openmode{}));
+	for (const std::string_view chunk : chunks)
+		out.write(chunk.data(), static_cast<std::streamsize>(chunk.size()));
+	out.close();
+	if (! out.fail())
+		return {};
+	return std::make_error_code(errno != 0 ? static_cast<std::errc>(errno) : std::errc::io_error);
+}
+
+std::error_code write_file_atomically(const std::string &path, std::initializer_list<std::string_view> chunks, bool binary, bool replace_read_only)
+{
+	boost::system::error_code bec;
+	const boost::filesystem::file_status target = boost::filesystem::symlink_status(path, bec);
+	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 this process may
+	// not write has to be refused here, as the in-place write used to be.
+	if (target_exists && ! is_writable(path)) {
+		if (! replace_read_only)
+			return std::make_error_code(std::errc::permission_denied);
+#ifdef _WIN32
+		// Nothing replaces a read-only file on Windows, so the attribute goes first.
+		boost::filesystem::permissions(path, boost::filesystem::add_perms | boost::filesystem::owner_write, bec);
+#endif
+	}
+
+	// Unique per process and per call, so two threads writing one target
+	// without a lock never share a temporary.
+	static std::atomic<unsigned> counter{0};
+	const std::string tmp_path = path + "." + std::to_string(get_current_pid()) + "." + std::to_string(counter++) + ".tmp";
+	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, or a name the longer temporary pushes past the path limit.
+		const bool only_the_temporary_failed = (target_exists && (ec == std::errc::permission_denied || ec == std::errc::no_such_file_or_directory)) ||
+		                                       ec == std::errc::filename_too_long;
+		if (only_the_temporary_failed) {
+			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
+	// 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
+	if (const std::error_code ec = rename_file(tmp_path, path)) {
+		boost::nowide::remove(tmp_path.c_str());
+		// A reader on Windows holding the target open without FILE_SHARE_DELETE,
+		// 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.
+		// Once per file at warning level, so a mount that never replaces does
+		// not fill the log, and the loss of the atomic path is still on record.
+		static std::mutex warned_mutex;
+		static std::set<std::string> warned;
+		bool first;
+		{
+			std::lock_guard<std::mutex> guard(warned_mutex);
+			first = warned.insert(path).second;
+		}
+		if (first)
+			BOOST_LOG_TRIVIAL(warning) << "Cannot replace " << path << " (" << ec.message() << "); writing in place, here and for later saves of this file";
+		else
+			BOOST_LOG_TRIVIAL(info) << "Cannot replace " << path << " (" << ec.message() << "); writing in place";
+		return write_whole_file(path, chunks, binary);
+	}
+	// Crash leftovers of earlier writes into this directory, for the files the
+	// 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()))
+		remove_stale_temp_files(dir);
+	return {};
+}
+
+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.
+	auto digit_segments_before = [](const std::string &name, size_t end, int count) {
+		for (int segment = 0; segment < count; ++ 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;
+	};
+	auto is_temp_name = [&](const std::string &name) {
+		if (name.compare(0, name_prefix.size(), name_prefix) != 0)
+			return false;
+		static const std::string tmp_suffix = ".tmp", old_suffix = ".old";
+		if (name.size() > tmp_suffix.size() && name.compare(name.size() - tmp_suffix.size(), tmp_suffix.size(), tmp_suffix) == 0)
+			return digit_segments_before(name, name.size() - tmp_suffix.size(), 2);
+		if (name.size() > old_suffix.size() && name.compare(name.size() - old_suffix.size(), old_suffix.size(), old_suffix) == 0)
+			return digit_segments_before(name, name.size() - old_suffix.size(), 1);
+		return false;
+	};
+	// An instance that gave up waiting for the lock writes unlocked by design,
+	// 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;
+	for (boost::filesystem::directory_iterator it(dir, ec), end; ! ec && it != end; it.increment(ec)) {
+		// The name test first: it is free, the stat is not.
+		if (! is_temp_name(it->path().filename().string()))
+			continue;
+		boost::system::error_code entry_ec;
+		if (! boost::filesystem::is_regular_file(it->symlink_status(entry_ec)))
+			continue;
+		const std::time_t written = boost::filesystem::last_write_time(it->path(), entry_ec);
+		if (entry_ec || now - written < stale_age)
+			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.
+			// It may be the last copy of a file, or a file since deleted on
+			// purpose; neither removing nor restoring it is safe unasked.
+			static std::mutex warned_mutex;
+			static std::set<std::string> warned;
+			std::lock_guard<std::mutex> guard(warned_mutex);
+			if (warned.insert(it->path().string()).second)
+				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();
+			++ removed;
+		}
+	}
+	return removed;
+}
+
 #ifdef __linux__
 // Copied from boost::filesystem.
 // Called by copy_file_linux() in case linux sendfile() API is not supported.

src/slic3r/GUI/GUI_App.cpp

@@ -85,6 +85,7 @@
 #include "libslic3r/Model.hpp"
 #include "libslic3r/I18N.hpp"
 #include "libslic3r/PresetBundle.hpp"
+#include "libslic3r/InstanceLock.hpp"
 #include "libslic3r/Thread.hpp"
 #include "libslic3r/miniz_extension.hpp"
 #include "libslic3r/Utils.hpp"
@@ -3531,7 +3532,7 @@ bool GUI_App::on_init_inner()
             update_publish_status();
         }
 
-        if (m_post_initialized && app_config->dirty())
+        if (m_post_initialized && app_config->dirty() && app_config->save_due())
             app_config->save();
 
     });
@@ -7623,8 +7624,7 @@ void GUI_App::start_sync_user_preset(bool with_progress_dlg)
 
                             // Delete the bundle folder and bundle
                             fs::path bundle_folder = fs::path(bundle.path.c_str()).parent_path();
-                            boost::system::error_code ec;
+                            remove_directory_tree_locked(bundle_folder);
-                            boost::filesystem::remove_all(bundle_folder, ec);
 
                             preset_bundle->bundles.WriteLock();
                             preset_bundle->bundles.m_bundles.erase(bundle.id);
@@ -8889,6 +8889,7 @@ void GUI_App::preset_deleted_from_cloud(std::string setting_id)
 
     // Delete the .info file after cloud deletion is confirmed
     if (!preset_file_path.empty() && fs::exists(fs::path(preset_file_path))) {
+        InstanceLock instance_lock(user_presets_lock_path());
         boost::nowide::remove(preset_file_path.c_str());
         BOOST_LOG_TRIVIAL(info) << "Deleted .info file after cloud confirmation: " << preset_file_path;
     }
@@ -8951,15 +8952,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 .json doesn't exist, .info is orphaned. Read under the lock, so a
-            if (!fs::exists(preset_file)) {
+            // remove_files() in another instance is seen whole or not at all; the
-                // Extract setting_id from .info file
+            // delete queue's own mutex is taken after the lock is released.
-                std::string setting_id = extract_setting_id_from_info(info_file.string());
+            std::string setting_id;
-                if (!setting_id.empty()) {
+            {
-                    // Add to need_delete_presets
+                InstanceLock instance_lock(user_presets_lock_path());
-                    delete_preset_from_cloud(setting_id, info_file.string());
+                if (!fs::exists(preset_file))
-                    BOOST_LOG_TRIVIAL(info) << "Found orphaned .info file on startup: " << info_file.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 (ec)

src/slic3r/GUI/WebGuideDialog.cpp

@@ -1481,9 +1481,7 @@ bool GuideFrame::BuildProfileDataFromVendors()
             return false;
 
         // Written through a temp file and moved into place, as the preset caches
-        // are: half a cache must never be readable, and the PID suffix keeps two
+        // are: half a cache must never be readable.
-        // instances from interleaving on one temp file.
-        const std::string tmp_path = cache_file.string() + "." + std::to_string(get_current_pid()) + ".tmp";
         try {
             json out;
             out["format"]  = 1;
@@ -1492,18 +1490,9 @@ bool GuideFrame::BuildProfileDataFromVendors()
             for (const char* key : { "model", "machine", "filament", "process" })
                 profile[key] = m_ProfileJson[key];
             boost::filesystem::create_directories(cache_file.parent_path());
-            {
+            if (const std::error_code ec = write_file_atomically(cache_file.string(), out.dump(-1, ' ', false, json::error_handler_t::ignore), /*binary=*/true))
-                boost::nowide::ofstream ofs(tmp_path, std::ios::binary | std::ios::trunc);
-                ofs << out.dump(-1, ' ', false, json::error_handler_t::ignore);
-                ofs.close();
-                if (! ofs.good())
-                    throw std::runtime_error("write failed");
-            }
-            if (const std::error_code ec = rename_file(tmp_path, cache_file.string()))
                 throw std::runtime_error(ec.message());
         } catch (const std::exception& e) {
-            boost::system::error_code rm;
-            boost::filesystem::remove(tmp_path, rm);
             BOOST_LOG_TRIVIAL(warning) << "GuideFrame: could not write the profile data cache: " << e.what();
         }
         return true;

src/slic3r/Utils/3DPrinterOS.cpp

@@ -2,6 +2,7 @@
 
 #include <algorithm>
 #include <sstream>
+#include <system_error>
 #include <exception>
 #include <boost/format.hpp>
 #include <boost/log/trivial.hpp>
@@ -580,9 +581,10 @@ bool C3DPrinterOS::save_api_session(const std::string &session, const std::strin
     j.put("session", session);
     j.put("email", email);
     try {
-        auto temp_path = m_api_session_file_path + ".tmp";
+        std::ostringstream json;
-        pt::write_json(temp_path, j);
+        pt::write_json(json, j);
-        boost::filesystem::rename(temp_path, m_api_session_file_path);
+        if (const std::error_code ec = write_file_atomically(m_api_session_file_path, json.str()))
+            throw std::system_error(ec);
     } catch (const std::exception &err) {
         BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << ": failed to write json to file. Path = "
                                  << m_api_session_file_path

src/slic3r/Utils/OrcaCloudServiceAgent.cpp

@@ -1475,15 +1475,8 @@ void OrcaCloudServiceAgent::save_sync_state()
     if (sync_state_path.empty())
         return;
 
-    try {
+    if (const std::error_code ec = write_file_atomically(sync_state_path, std::to_string(sync_state.last_sync_timestamp)))
-        std::string tmp_path = sync_state_path + ".tmp";
+        BOOST_LOG_TRIVIAL(warning) << "OrcaCloudServiceAgent: failed to save the sync state: " << ec.message();
-        std::ofstream ofs(tmp_path, std::ios::out | std::ios::trunc);
-        if (ofs.good()) {
-            ofs << std::to_string(sync_state.last_sync_timestamp);
-            ofs.close();
-            boost::filesystem::rename(tmp_path, sync_state_path);
-        }
-    } catch (...) {}
 }
 
 void OrcaCloudServiceAgent::clear_sync_state()
@@ -1572,22 +1565,10 @@ void OrcaCloudServiceAgent::persist_user_secret(const std::string& secret)
             wxFileName::Mkdir(path.GetPath(), wxS_DIR_DEFAULT, wxPATH_MKDIR_FULL);
         }
 
-        const std::string tmp_path = secret_fallback_path + ".tmp";
+        if (const std::error_code ec = write_file_atomically(secret_fallback_path, signed_payload, /*binary=*/true))
-        std::ofstream ofs(tmp_path, std::ios::out | std::ios::trunc | std::ios::binary);
+            BOOST_LOG_TRIVIAL(warning) << "OrcaCloudServiceAgent: cannot write user secret file " << secret_fallback_path << ": " << ec.message();
-        if (ofs.good()) {
+        else
-            ofs << signed_payload;
+            stored = true;
-            ofs.flush();
-            ofs.close();
-
-            if (wxRenameFile(wxString::FromUTF8(tmp_path.c_str()), wxString::FromUTF8(secret_fallback_path.c_str()), true)) {
-                stored = true;
-            } else {
-                wxRemoveFile(wxString::FromUTF8(tmp_path.c_str()));
-                BOOST_LOG_TRIVIAL(warning) << "OrcaCloudServiceAgent: failed to atomically replace user secret file";
-            }
-        } else {
-            BOOST_LOG_TRIVIAL(warning) << "OrcaCloudServiceAgent: cannot open user secret file for write - " << secret_fallback_path;
-        }
     } else {
         // Use wxSecretStore only
         wxSecretStore store = wxSecretStore::GetDefault();

src/slic3r/plugin/PluginConfig.cpp

@@ -249,21 +249,10 @@ bool PluginConfig::save()
         return false;
     }
 
-    // Write to a PID-suffixed file and rename it into place, so a crash mid-write cannot truncate an
+    // Written beside the target and moved into place, so a crash mid-write cannot truncate an
-    // existing config. Same approach as AppConfig::save().
+    // existing config.
-    const std::string path_pid = (boost::format("%1%.%2%") % path % get_current_pid()).str();
+    if (const std::error_code ec = write_file_atomically(path, root.dump(1, '\t') + "\n")) {
-
+        BOOST_LOG_TRIVIAL(error) << "PluginConfig: failed to write " << path << ": " << ec.message() << "; keeping the existing config";
-    boost::nowide::ofstream file;
-    file.open(path_pid, std::ios::out | std::ios::trunc);
-    file << root.dump(1, '\t') << std::endl;
-    file.close();
-    if (file.fail()) {
-        BOOST_LOG_TRIVIAL(error) << "PluginConfig: failed to write " << path_pid << "; keeping the existing config";
-        return false;
-    }
-
-    if (const std::error_code rename_ec = rename_file(path_pid, path)) {
-        BOOST_LOG_TRIVIAL(error) << "PluginConfig: failed to move " << path_pid << " onto " << path << ": " << rename_ec.message();
         return false;
     }
 

tests/libslic3r/CMakeLists.txt

@@ -49,6 +49,7 @@ add_executable(${_TEST_NAME}_tests
     test_ordering_strategies.cpp
     # test_png_io.cpp
     test_indexed_triangle_set.cpp
+    test_instance_lock.cpp
     ../libnest2d/printer_parts.cpp
     )
 

tests/libslic3r/test_instance_lock.cpp

@@ -0,0 +1,202 @@
+#include <catch2/catch_all.hpp>
+
+#include <atomic>
+#include <chrono>
+#include <thread>
+
+#include <boost/filesystem.hpp>
+
+#include "libslic3r/InstanceLock.hpp"
+#include "test_utils.hpp"
+
+#ifndef _WIN32
+#include <fcntl.h>
+#include <sys/file.h>
+#include <sys/wait.h>
+#include <unistd.h>
+#endif
+
+using namespace Slic3r;
+using namespace std::chrono_literals;
+
+// Sets a process-wide knob for one test and restores it however the test ends.
+template<typename T> struct ScopedStaticValue
+{
+    T &ref;
+    T  saved;
+    ScopedStaticValue(T &ref, T value) : ref(ref), saved(ref) { ref = value; }
+    ~ScopedStaticValue() { ref = saved; }
+};
+
+TEST_CASE("InstanceLock creates its lock file and holds it for the guard's scope", "[InstanceLock]")
+{
+    ScopedTemporaryFile lock_file(".lock");
+    const std::string   path = lock_file.string();
+
+    {
+        InstanceLock lock(path);
+        REQUIRE(lock.locked());
+        REQUIRE(boost::filesystem::exists(path));
+    }
+    // Released: a fresh guard gets the lock at once instead of waiting out a timeout.
+    const auto   started = std::chrono::steady_clock::now();
+    InstanceLock again(path, 5000ms);
+    REQUIRE(again.locked());
+    // Well inside the timeout it would otherwise have waited out; loose enough for a loaded runner.
+    REQUIRE(std::chrono::steady_clock::now() - started < 4000ms);
+}
+
+TEST_CASE("InstanceLock nests within one thread", "[InstanceLock]")
+{
+    ScopedTemporaryFile lock_file(".lock");
+    const std::string   path = lock_file.string();
+
+    InstanceLock outer(path);
+    {
+        InstanceLock inner(path, 100ms);
+        REQUIRE(inner.locked());
+    }
+    // The inner guard leaving does not release the outer one.
+    REQUIRE(outer.locked());
+}
+
+TEST_CASE("InstanceLock is a no-op for an empty path and survives an unwritable one", "[InstanceLock]")
+{
+    ScopedTemporaryDir dir;
+
+    InstanceLock none("");
+    REQUIRE_FALSE(none.locked());
+
+    // The directory does not exist, so the lock file cannot be created; the
+    // guard still constructs and the write it guards can go ahead.
+    InstanceLock unwritable((dir.path() / "missing" / "shared.lock").string(), 100ms);
+    REQUIRE_FALSE(unwritable.locked());
+}
+
+TEST_CASE("InstanceLock retries a lock file it could not open once the cool-down passes", "[InstanceLock]")
+{
+    ScopedTemporaryDir dir;
+    const std::string  path = (dir.path() / "later" / "shared.lock").string();
+    ScopedStaticValue  cooldown(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();
+    }
+    boost::filesystem::create_directories(dir.path() / "later");
+    {
+        InstanceLock lock(path, 100ms);
+        during_cooldown = lock.locked();
+    }
+    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();
+    }
+
+    REQUIRE_FALSE(before_dir);
+    // 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);
+}
+
+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());
+    // Each outermost guard opens the file afresh, so the deleted path is back.
+    REQUIRE(boost::filesystem::exists(path));
+}
+
+TEST_CASE("InstanceLock serialises the threads of one process", "[InstanceLock]")
+{
+    ScopedTemporaryFile lock_file(".lock");
+    const std::string   path = lock_file.string();
+
+    std::atomic<bool> holder_ready{false};
+    std::atomic<bool> holder_released{false};
+    std::thread holder([&] {
+        InstanceLock lock(path);
+        holder_ready = true;
+        std::this_thread::sleep_for(150ms);
+        holder_released = true;
+    });
+    while (! holder_ready)
+        std::this_thread::yield();
+
+    bool released_before_acquire = false;
+    {
+        InstanceLock lock(path);
+        released_before_acquire = holder_released;
+    }
+    holder.join();
+    REQUIRE(released_before_acquire);
+}
+
+#ifndef _WIN32
+// 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");
+    const std::string   path = lock_file.string();
+
+    int child_holds[2], child_may_exit[2];
+    REQUIRE(::pipe(child_holds) == 0);
+    REQUIRE(::pipe(child_may_exit) == 0);
+
+    const pid_t child = ::fork();
+    REQUIRE(child >= 0);
+    if (child == 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);
+    }
+
+    char byte = '0';
+    REQUIRE(::read(child_holds[0], &byte, 1) == 1);
+    REQUIRE(byte == '1');
+
+    bool locked_while_child_holds;
+    {
+        InstanceLock lock(path, 100ms);
+        locked_while_child_holds = lock.locked();
+    }
+    // The timed-out wait starts a cool-down: the next guard does not wait again.
+    const auto started = std::chrono::steady_clock::now();
+    bool       locked_during_cooldown;
+    {
+        InstanceLock lock(path, 5000ms);
+        locked_during_cooldown = lock.locked();
+    }
+    const auto cooldown_wait = std::chrono::steady_clock::now() - started;
+    REQUIRE(::write(child_may_exit[1], "x", 1) == 1);
+    int status = 0;
+    REQUIRE(::waitpid(child, &status, 0) == child);
+    for (int fd : {child_holds[0], child_holds[1], child_may_exit[0], child_may_exit[1]})
+        ::close(fd);
+
+    REQUIRE_FALSE(locked_while_child_holds);
+    REQUIRE_FALSE(locked_during_cooldown);
+    REQUIRE(cooldown_wait < 4000ms);
+    // A guard inside the cool-down still takes the lock when it is free.
+    InstanceLock lock(path);
+    REQUIRE(lock.locked());
+}
+#endif

tests/libslic3r/test_utils.cpp

@@ -8,8 +8,11 @@
 
 #include <algorithm>
 #include <cctype>
+#include <ctime>
 #include <fstream>
 #include <string>
+#include <thread>
+#include <system_error>
 
 #ifndef _WIN32
 #include <unistd.h>     // getuid
@@ -62,6 +65,173 @@ 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]") {
+    ScopedTemporaryDir dir;
+    const boost::filesystem::path target = dir.path() / "preset.json";
+
+    REQUIRE_FALSE(write_file_atomically(target.string(), "first"));
+    REQUIRE_FALSE(write_file_atomically(target.string(), "second"));
+
+    std::string content;
+    load_string_file(target, content);
+    REQUIRE(content == "second");
+    size_t entries = 0;
+    for (auto &entry : boost::filesystem::directory_iterator(dir.path())) {
+        (void) entry;
+        ++entries;
+    }
+    REQUIRE(entries == 1);
+}
+
+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";
+
+    const std::error_code ec = write_file_atomically(target.string(), "x");
+    REQUIRE(ec == std::errc::no_such_file_or_directory);
+    REQUIRE_FALSE(boost::filesystem::exists(target));
+}
+
+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"));
+    boost::filesystem::permissions(target, boost::filesystem::owner_read | boost::filesystem::group_read | boost::filesystem::others_read);
+
+    const std::error_code ec = write_file_atomically(target.string(), "replaced");
+    boost::filesystem::permissions(target, boost::filesystem::owner_read | boost::filesystem::owner_write | boost::filesystem::group_read | boost::filesystem::others_read);
+
+    REQUIRE(ec == std::errc::permission_denied);
+    std::string content;
+    load_string_file(target, content);
+    REQUIRE(content == "pinned");
+}
+
+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");
+#endif
+    ScopedTemporaryDir dir;
+    const boost::filesystem::path target = dir.path() / "pinned.json";
+    REQUIRE_FALSE(write_file_atomically(target.string(), "pinned"));
+    boost::filesystem::permissions(target, boost::filesystem::owner_read | boost::filesystem::group_read | boost::filesystem::others_read);
+
+    const std::error_code ec = write_file_atomically(target.string(), "replaced", false, /*replace_read_only=*/true);
+    boost::filesystem::permissions(target, boost::filesystem::owner_read | boost::filesystem::owner_write | boost::filesystem::group_read | boost::filesystem::others_read);
+
+    REQUIRE_FALSE(ec);
+    std::string content;
+    load_string_file(target, content);
+    REQUIRE(content == "replaced");
+}
+
+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);
+
+    REQUIRE_FALSE(write_file_atomically(target.string(), bytes, /*binary=*/true));
+    REQUIRE(boost::filesystem::file_size(target) == bytes.size());
+}
+
+#ifndef _WIN32
+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";
+    REQUIRE_FALSE(write_file_atomically(real.string(), "first"));
+    boost::filesystem::permissions(real, boost::filesystem::owner_read | boost::filesystem::owner_write);
+    boost::filesystem::create_symlink(real, link);
+
+    REQUIRE_FALSE(write_file_atomically(link.string(), "second"));
+
+    REQUIRE(boost::filesystem::is_symlink(boost::filesystem::symlink_status(link)));
+    std::string content;
+    load_string_file(real, content);
+    REQUIRE(content == "second");
+
+    REQUIRE_FALSE(write_file_atomically(real.string(), "third"));
+    const auto perms = boost::filesystem::status(real).permissions() & boost::filesystem::all_all;
+    REQUIRE(perms == (boost::filesystem::owner_read | boost::filesystem::owner_write));
+}
+#endif
+
+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');
+
+    std::thread other([&] {
+        for (int i = 0; i < 50; ++i)
+            write_file_atomically(target.string(), a);
+    });
+    for (int i = 0; i < 50; ++i)
+        write_file_atomically(target.string(), b);
+    other.join();
+
+    std::string content;
+    load_string_file(target, content);
+    const bool whole = content == a || content == b;
+    REQUIRE(whole);
+    // 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 leaves a moved-aside file to the user", "[utils]") {
+    ScopedTemporaryDir dir;
+    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.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]") {
+    ScopedTemporaryDir dir;
+    // a.json exists, so its .old is a leftover; a moved-aside file without its original is a different case.
+    for (const char *name : { "a.json", "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", "a.json.123.old", "h.json.old" }) {
+        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.2.tmp").string(), "x"));
+
+    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(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()) == 2);
+        size_t entries = 0;
+        for (auto &entry : boost::filesystem::directory_iterator(dir.path())) {
+            (void) entry;
+            ++entries;
+        }
+        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"));
+        REQUIRE(boost::filesystem::exists(dir.path() / "g.json.7.2.tmp"));
+    }
+}
+
 TEST_CASE("copy_file reports the OS error when the destination cannot be written", "[utils]") {
     ScopedTemporaryFile source(".txt");
     {

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,20 +1362,23 @@ 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"));
     const std::string before = slurp(cache);
 
-    // A directory where the temp file wants to go: the write cannot complete,
+    // A read-only cache (the read-only attribute on Windows) is refused before
-    // and must not have destroyed what was already there to find that out.
+    // anything is written, so what was there must survive the attempt.
-    const fs::path blocker = fs::path(cache + "." + std::to_string(get_current_pid()) + ".tmp");
+    fs::permissions(cache, fs::owner_read | fs::group_read | fs::others_read);
-    fs::create_directories(blocker);
 
     REQUIRE_FALSE(save_one_vendor(cache, one_vendor("Durable"), "Durable", "2.0.0"));
-    CHECK(slurp(cache) == before);
 
-    fs::remove_all(blocker);
+    fs::permissions(cache, fs::owner_read | fs::owner_write | fs::group_read | fs::others_read);
+    CHECK(slurp(cache) == before);
 }
 
 TEST_CASE("a cache written by another build's option ordering still loads", "[VendorCache]")