Run the Cross-Process Lock Test on Every Platform

The test that checks the guard yields to a lock held elsewhere forked a
child to hold it, so it was left out on Windows. The OS lock belongs to
the handle on Windows and to the open file description elsewhere, so a
second handle in the same process is refused like another instance
would be. The test now holds the lock that way and runs everywhere.

.github/workflows/ofl-ota-cronjob.yml

@@ -146,7 +146,7 @@ jobs:
                 "repos/${{ github.repository }}/actions/workflows/post_merge_profiles.yml/runs" \
                 -f branch="$branch" -f event=workflow_dispatch -f per_page=100)"
               run_id="$(jq -r --arg marker "[OFL cron $dispatch_id]" \
-                '[.workflow_runs[] | select((.display_title // "") | contains($marker))]
+                '[.workflow_runs[] | select((.display_title // "") | contains($marker))] \
                 | sort_by(.created_at) | last | .id // empty' <<< "$runs_json")"
               [ -n "$run_id" ] && break
               sleep 5

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"
@@ -743,10 +744,13 @@ 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());
+
     // 1) Read the complete config file into a boost::property_tree.
     namespace pt = boost::property_tree;
     pt::ptree tree;
@@ -996,7 +1000,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;
 
@@ -1126,43 +1129,18 @@ 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;
-
-#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());
+
     // 1) Read the complete config file into a boost::property_tree.
     namespace pt = boost::property_tree;
     pt::ptree tree;
@@ -1300,7 +1278,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)
@@ -1336,39 +1313,39 @@ 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);
-    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
+    // Not flushed to the device: the idle handler saves on the GUI thread after
+    // any change, and the rename already gives a complete old or new file.
+    if (const std::error_code ec = write_file_atomically(path, body)) {
+        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 = {};
+#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))
+        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);
@@ -1857,6 +1834,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
@@ -1893,7 +1875,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"
@@ -122,14 +123,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; }
@@ -339,6 +344,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; }
@@ -449,8 +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`
+	// 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 for the next ten seconds, so the
+	// idle handler does not repeat a hopeless write on every event.
+	std::chrono::steady_clock::time_point						m_retry_save_at{};
 	// 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

@@ -306,6 +306,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

@@ -1538,11 +1538,9 @@ 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;
 }
 

src/libslic3r/InstanceLock.cpp

@@ -0,0 +1,165 @@
+#include "InstanceLock.hpp"
+
+#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_CREAT | O_CLOEXEC, 0644))
+    {
+        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;
+        // A signal (a child exiting, for one) interrupts the call like any other; the caller polls again.
+        if (errno == EWOULDBLOCK || errno == EINTR)
+            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. The
+// file is kept rather than deleted on release because the lock state lives in
+// the kernel on the open file, and deleting it would let a third instance
+// lock a fresh file while the second still holds the old one.
+struct InstanceLock::Slot
+{
+    std::recursive_mutex mutex;
+    // Non-null exactly while this process holds the file lock.
+    std::unique_ptr<NativeFileLock> file_lock;
+    int  depth{0};
+    // Until this point, after a guard could not open, lock or wait out the
+    // file, guards do not touch it.
+    std::chrono::steady_clock::time_point cooldown_until{};
+};
+
+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;
+}
+
+// Starts the cool-down. Called with the slot mutex held.
+void InstanceLock::defer(Slot &slot, const std::string &reason)
+{
+    slot.cooldown_until = std::chrono::steady_clock::now() + cooldown;
+    BOOST_LOG_TRIVIAL(warning) << reason << "; proceeding without the lock for the next " << cooldown.count() << " ms";
+}
+
+// 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 {
+#ifdef _WIN32
+        // The lock opens an existing file; created once, on the first miss.
+        const std::wstring wide_path = boost::nowide::widen(lock_file_path);
+        try {
+            slot.file_lock = std::make_unique<NativeFileLock>(wide_path.c_str());
+        } catch (const std::exception &) {
+            boost::nowide::ofstream(lock_file_path, std::ios::app).close();
+            slot.file_lock = std::make_unique<NativeFileLock>(wide_path.c_str());
+        }
+#else
+        slot.file_lock = std::make_unique<NativeFileLock>(lock_file_path.c_str());
+#endif
+        return true;
+    } catch (const std::exception &e) {
+        defer(slot, "Cannot open lock file " + lock_file_path + ": " + e.what() + " (check its owner and permissions)");
+        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->cooldown_until && open_lock_file(*m_slot, lock_file_path)) {
+        const auto deadline = now + timeout;
+        bool       taken    = false;
+        for (;;) {
+            try {
+                if ((taken = m_slot->file_lock->try_lock()))
+                    break;
+            } catch (const std::exception &e) {
+                defer(*m_slot, "Cannot lock " + lock_file_path + ": " + e.what());
+                break;
+            }
+            if (std::chrono::steady_clock::now() >= deadline) {
+                defer(*m_slot, "Another instance has held " + lock_file_path + " for over " + std::to_string(timeout.count()) + " ms");
+                break;
+            }
+            std::this_thread::sleep_for(std::chrono::milliseconds(5));
+        }
+        if (! taken)
+            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_lock != nullptr;
+}
+
+InstanceLock::~InstanceLock()
+{
+    if (m_slot == nullptr)
+        return;
+    if (-- m_slot->depth == 0 && m_slot->file_lock) {
+        try {
+            m_slot->file_lock->unlock();
+        } catch (const std::exception &e) {
+            BOOST_LOG_TRIVIAL(warning) << "Cannot unlock instance lock: " << e.what();
+        }
+        m_slot->file_lock.reset();
+    }
+}
+
+} // namespace Slic3r

src/libslic3r/InstanceLock.hpp

@@ -0,0 +1,60 @@
+#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`. The lock file is
+// created on first use and kept; the OS releases the lock when its holder
+// exits, so a crashed instance never leaves a stale lock behind.
+//
+// Best effort: when the lock file cannot be opened or locked, or another
+// instance still holds it after `timeout`, the guard keeps only the in-process
+// mutex, locked() reports false and the write proceeds, since a hung instance
+// must never block another one from saving. For `cooldown` afterwards guards
+// leave the file alone. The wait for the in-process mutex is bounded only by
+// the longest critical section, so a guard covers a few file operations and
+// nothing slower.
+//
+// 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 guard
+// must not be added around save_user_presets(), which takes the collection
+// mutex through delete_preset().
+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};
+    // Long enough that a holder stuck in a debugger does not cost a stall per
+    // save; 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);
+    static void  defer(Slot &slot, const std::string &reason);
+
+    Slot *m_slot{nullptr};
+    std::unique_lock<std::recursive_mutex> m_slot_guard;
+    bool  m_locked{false};
+};
+
+} // namespace Slic3r

src/libslic3r/PNGReadWrite.cpp

@@ -54,43 +54,9 @@ static void png_read_callback(png_struct *png_ptr,
     // Retrieve our input buffer through the png_ptr
     auto reader = static_cast<IStream *>(png_get_io_ptr(png_ptr));
 
-    // libpng expects a short read to be reported through png_error(); returning quietly would leave
+    if (!reader || !reader->is_ok()) return;
-    // it decoding whatever happened to be in outBytes.
-    if (!reader || !reader->is_ok() ||
-        reader->read(static_cast<std::uint8_t *>(outBytes), byteCountToRead) != byteCountToRead)
-        png_error(png_ptr, "PNG data is truncated");
-}
 
-// libpng reports a corrupt or truncated image by longjmp()ing back to the jump buffer set with
+    reader->read(static_cast<std::uint8_t *>(outBytes), byteCountToRead);
-// setjmp(). The frame it lands in must own nothing that needs destroying: with exceptions enabled
-// MSVC unwinds the stack as part of longjmp, and returning from a frame unwound that way crashes -
-// which is what a truncated texture did on Windows while working everywhere else. So the calls that
-// can fail live in these two helpers, which hold nothing but pointers, and every C++ object the
-// decoders need stays in their own frames.
-static bool png_read_header_guarded(png_struct *png, png_info *info, IStream *in_buf, int sig_bytes)
-{
-    if (setjmp(png_jmpbuf(png)))
-        return false;
-
-    png_set_read_fn(png, static_cast<void *>(in_buf), png_read_callback);
-    // Tell that we have already read the first bytes to check the signature
-    png_set_sig_bytes(png, sig_bytes);
-    png_read_info(png, info);
-    return true;
-}
-
-// `bottom_up` fills the buffer last row first, which is the order the colour decoder hands back.
-static bool png_read_rows_guarded(png_struct *png, png_info *info, png_bytep dst, size_t rows, size_t rowbytes,
-                                  bool bottom_up, bool read_end)
-{
-    if (setjmp(png_jmpbuf(png)))
-        return false;
-
-    for (size_t i = 0; i < rows; ++i)
-        png_read_row(png, dst + (bottom_up ? rows - 1 - i : i) * rowbytes, nullptr);
-    if (read_end)
-        png_read_end(png, info);
-    return true;
 }
 
 bool decode_png(IStream &in_buf, ImageGreyscale &out_img)
@@ -111,8 +77,12 @@ bool decode_png(IStream &in_buf, ImageGreyscale &out_img)
     dsc.info = png_create_info_struct(dsc.png);
     if(!dsc.info) return false;
 
-    if (!png_read_header_guarded(dsc.png, dsc.info, &in_buf, PNG_SIG_BYTES))
+    png_set_read_fn(dsc.png, static_cast<void *>(&in_buf), png_read_callback);
-        return false;
+
+    // Tell that we have already read the first bytes to check the signature
+    png_set_sig_bytes(dsc.png, PNG_SIG_BYTES);
+
+    png_read_info(dsc.png, dsc.info);
 
     out_img.cols = png_get_image_width(dsc.png, dsc.info);
     out_img.rows = png_get_image_height(dsc.png, dsc.info);
@@ -124,8 +94,11 @@ bool decode_png(IStream &in_buf, ImageGreyscale &out_img)
 
     out_img.buf.resize(out_img.rows * out_img.cols);
 
-    return png_read_rows_guarded(dsc.png, dsc.info, static_cast<png_bytep>(out_img.buf.data()), out_img.rows,
+    auto readbuf = static_cast<png_bytep>(out_img.buf.data());
-                                 out_img.cols, /* bottom_up */ false, /* read_end */ false);
+    for (size_t r = 0; r < out_img.rows; ++r)
+        png_read_row(dsc.png, readbuf + r * out_img.cols, nullptr);
+
+    return true;
 }
 
 bool decode_colored_png(IStream &in_buf, ImageColorscale &out_img)
@@ -155,10 +128,12 @@ bool decode_colored_png(IStream &in_buf, ImageColorscale &out_img)
         return false;
     }
 
-    if (!png_read_header_guarded(dsc.png, dsc.info, &in_buf, PNG_SIG_BYTES)) {
+    png_set_read_fn(dsc.png, static_cast<void *>(&in_buf), png_read_callback);
-        BOOST_LOG_TRIVIAL(error) << "decode_colored_png: corrupt or truncated PNG data";
+
-        return false;
+    // Tell that we have already read the first bytes to check the signature
-    }
+    png_set_sig_bytes(dsc.png, PNG_SIG_BYTES);
+
+    png_read_info(dsc.png, dsc.info);
 
     out_img.cols = png_get_image_width(dsc.png, dsc.info);
     out_img.rows = png_get_image_height(dsc.png, dsc.info);
@@ -187,12 +162,13 @@ bool decode_colored_png(IStream &in_buf, ImageColorscale &out_img)
     int interlace_type = png_get_interlace_type(dsc.png, dsc.info);
     BOOST_LOG_TRIVIAL(info) << boost::format("filter_type %1%, compression_type %2%, interlace_type %3%, rowbytes %4%")%filter_type %compression_type %interlace_type %rowbytes;
 
-    if (!png_read_rows_guarded(dsc.png, dsc.info, static_cast<png_bytep>(out_img.buf.data()), out_img.rows, rowbytes,
+    auto readbuf = static_cast<png_bytep>(out_img.buf.data());
-                               /* bottom_up */ true, /* read_end */ true)) {
+    for (size_t r = out_img.rows; r > 0; r--)
-        BOOST_LOG_TRIVIAL(error) << "decode_colored_png: corrupt or truncated PNG data";
+    {
-        return false;
+        png_read_row(dsc.png, readbuf + (r - 1) * rowbytes, nullptr);
     }
 
+    png_read_end(dsc.png, dsc.info);
     png_destroy_read_struct(&dsc.png, &dsc.info, NULL);
 
     return true;

src/libslic3r/Preset.cpp

@@ -51,6 +51,9 @@
 #include "libslic3r.h"
 #include "LifecycleEvents.hpp"
 #include "Utils.hpp"
+#include "InstanceLock.hpp"
+
+#include <sstream>
 #include "Time.hpp"
 #include "PlaceholderParser.hpp"
 #include "libslic3r/GCode/Thumbnails.hpp"
@@ -107,6 +110,32 @@ 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.
+// The scan passes read_only when it could not take the lock, since the file may
+// then be another instance's fresh write that it merely raced.
+static void remove_preset_files(const std::string &preset_file, bool read_only)
+{
+    if (read_only)
+        return;
+    InstanceLock instance_lock(user_presets_lock_path());
+    // A symlink whose target is missing stays, since the target may come back; one
+    // that cannot even be followed is removed like any other unreadable file.
+    auto remove = [](const fs::path &file) {
+        boost::system::error_code ec;
+        if (fs::status(file, ec).type() != fs::file_not_found)
+            fs::remove(file, ec);
+    };
+    fs::path file_path(preset_file);
+    remove(file_path);
+    file_path.replace_extension(".info");
+    remove(file_path);
+}
+
 std::string get_preset_bare_name(const std::string &canonical_name)
 {
     const auto pos = canonical_name.find_last_of('/');
@@ -649,18 +678,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)
@@ -669,6 +700,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);
@@ -686,11 +718,11 @@ void Preset::remove_files(bool cloud_already_deleted)
 }
 
 //BBS: add logic for only difference save
-void Preset::save(DynamicPrintConfig* parent_config)
+bool Preset::save(DynamicPrintConfig* parent_config)
 {
     //BBS: add project embedded preset logic
     if (this->is_project_embedded)
-        return;
+        return true;
     //BBS: change to json format
     //this->config.save(this->file);
     std::string from_str;
@@ -705,12 +737,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;
@@ -746,13 +782,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());
+
+    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())) {
+        // No .info either: one without its preset reads as a cloud deletion request.
+        BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << ": failed to save " << this->file << ": " << ec.message();
+        return false;
     }
     BOOST_LOG_TRIVIAL(info) << __FUNCTION__ << " save config for: " << this->name << " and filament_id: " << filament_id << " and base_id: " << this->base_id;
 
@@ -762,6 +807,7 @@ void Preset::save(DynamicPrintConfig* parent_config)
         idx_file.replace_extension(".info");
         this->save_info(idx_file.string());
     }
+    return true;
 }
 
 void Preset::reload(Preset const &parent)
@@ -773,6 +819,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));
@@ -1671,6 +1718,22 @@ std::string PresetCollection::canonical_preset_name(const std::string &name, con
     return get_preset_canonical_name(parsed.bare, origin);
 }
 
+PresetCollection::PresetFilesOnDisk PresetCollection::PresetFilesOnDisk::read(const boost::filesystem::path &file)
+{
+    auto read_if_present = [](const fs::path &path) -> std::optional<std::string> {
+        std::string bytes;
+        try {
+            load_string_file(path, bytes);
+        } catch (const std::exception &) {
+            return std::nullopt;
+        }
+        return bytes;
+    };
+    fs::path info_path(file);
+    info_path.replace_extension(".info");
+    return { read_if_present(file), read_if_present(info_path) };
+}
+
 PresetCollection::UserPresetLoad PresetCollection::resolve_user_preset(
     const boost::filesystem::path &file, const std::string &canonical_name,
     const PresetOrigin &load_origin, ForwardCompatibilitySubstitutionRule substitution_rule,
@@ -1682,6 +1745,9 @@ PresetCollection::UserPresetLoad PresetCollection::resolve_user_preset(
     Preset &preset = out.preset;
     preset.bundle_id = load_origin.bundle_id;
     preset.file = file.string();
+    // Before either file is parsed, so a save that lands during the parse still shows
+    // up as a difference when commit compares.
+    out.on_disk = PresetFilesOnDisk::read(file);
     // Load the preset file, apply preset values on top of defaults.
     try {
         fs::path idx_path(preset.file);
@@ -1794,14 +1860,8 @@ void PresetCollection::commit_user_preset(UserPresetLoad &&loaded, std::deque<Pr
         ++m_errors;
         BOOST_LOG_TRIVIAL(error) << error;
     }
-    if (loaded.discard_file && !read_only) {
+    if (loaded.discard_file)
-        fs::path file_path(loaded.preset.file);
+        remove_preset_files(loaded.preset.file, read_only);
-        if (fs::exists(file_path))
-            fs::remove(file_path);
-        file_path.replace_extension(".info");
-        if (fs::exists(file_path))
-            fs::remove(file_path);
-    }
     if (!loaded.install)
         return;
 
@@ -1809,9 +1869,12 @@ void PresetCollection::commit_user_preset(UserPresetLoad &&loaded, std::deque<Pr
         if (loaded.save_compatible_printers) {
             // A filesystem error from the rewrite is counted, and the preset still loads.
             try {
-                if (!read_only)
+                if (read_only)
-                    preset.save(nullptr);
+                    BOOST_LOG_TRIVIAL(info) << __FUNCTION__ << " added compatible_printers for preset: " << preset.name << " (not written back)";
+                else if (preset.save(nullptr))
                     BOOST_LOG_TRIVIAL(info) << __FUNCTION__ << " added compatible_printers for preset: " << preset.name;
+                else
+                    ++m_errors; // save() logged why
             } catch (const std::runtime_error &err) {
                 ++m_errors;
                 BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << " can not write compatible_printers back to " << preset.file << ": " << err.what();
@@ -1897,11 +1960,32 @@ void PresetCollection::load_presets(
             return this->resolve_user_preset(files[i].path, files[i].canonical_name, resolved_origin, substitution_rule,
                                              extruder_id_name, extruder_variant_name, key_set1, key_set2);
         },
-        [&](size_t, UserPresetLoad &&loaded) {
+        [&](size_t i, UserPresetLoad &&loaded) {
+            // Resolve read the files without the lock, so another instance may have saved
+            // over them since. A file that changed is resolved again under the lock before
+            // commit removes or rewrites it, which also keeps its .json and .info from two
+            // different saves apart. Without the lock nothing is checked, so commit leaves
+            // the files alone.
+            const std::string lock_path = user_presets_lock_path(read_only);
+            InstanceLock      instance_lock(lock_path);
+            if (instance_lock.locked()) {
+                const PresetFilesOnDisk   on_disk = PresetFilesOnDisk::read(files[i].path);
+                boost::system::error_code ec;
+                // The link itself, and a stat error counts as present, so a file that is
+                // there but cannot be read is still counted and removed by commit.
+                if (! on_disk.json && fs::symlink_status(files[i].path, ec).type() == fs::file_not_found)
+                    return; // removed by another instance since it was read
+                if (! (on_disk == loaded.on_disk)) {
+                    CNumericLocalesSetter locales_setter;
+                    loaded = this->resolve_user_preset(files[i].path, files[i].canonical_name, resolved_origin, substitution_rule,
+                                                       extruder_id_name, extruder_variant_name, key_set1, key_set2);
+                }
+            }
+            const bool leave_files = read_only || (! lock_path.empty() && ! instance_lock.locked());
             // Committing can remove an unreadable preset's file, and a filesystem error
             // there is reported without stopping the rest of the directory.
             try {
-                this->commit_user_preset(std::move(loaded), presets_loaded, substitutions, preset_loaded_fn, read_only);
+                this->commit_user_preset(std::move(loaded), presets_loaded, substitutions, preset_loaded_fn, leave_files);
             } catch (const std::runtime_error &err) {
                 errors_cummulative += err.what();
                 errors_cummulative += "\n";
@@ -2211,7 +2295,10 @@ void PresetCollection::set_sync_info_and_save(std::string name, std::string sett
             preset->setting_id = setting_id;
             if (update_time > 0)
                 preset->updated_time = update_time;
-            preset->sync_info == "update" ? preset->save(nullptr) : preset->save_info();
+            if (preset->sync_info == "update")
+                preset->save(nullptr);
+            else
+                preset->save_info();
             break;
         }
     }
@@ -4293,8 +4380,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;
@@ -4426,6 +4520,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.
@@ -4618,7 +4713,10 @@ bool PhysicalPrinterCollection::delete_printer(const std::string& name)
 
     const PhysicalPrinter& printer = *it;
     // Erase the preset file.
+    {
+        InstanceLock instance_lock(user_presets_lock_path());
         boost::nowide::remove(printer.file.c_str());
+    }
     m_printers.erase(it);
     return true;
 }
@@ -4630,7 +4728,10 @@ bool PhysicalPrinterCollection::delete_selected_printer()
     const PhysicalPrinter& printer = this->get_selected_printer();
 
     // Erase the preset file.
+    {
+        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

@@ -8,6 +8,7 @@
 #include <unordered_set>
 #include <functional>
 #include <mutex>
+#include <optional>
 #include <boost/algorithm/string/predicate.hpp>
 #include <boost/filesystem/path.hpp>
 #include <boost/property_tree/ptree_fwd.hpp>
@@ -330,7 +331,8 @@ public:
 
     //BBS: add logic for only difference save
     //if parent_config is null, save all keys, otherwise, only save difference
-    void                save(DynamicPrintConfig* parent_config);
+    // Returns false when the preset file could not be written.
+    bool                save(DynamicPrintConfig* parent_config);
     void                reload(Preset const & parent);
 
     // Return a label of this preset, consisting of a name and a "(modified)" suffix, if this preset is dirty.
@@ -488,6 +490,12 @@ 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);
+
 // 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());
 
@@ -897,6 +905,16 @@ protected:
     void            set_custom_preset_alias(Preset &preset);
 
 private:
+    // A preset file and its .info as read from disk, std::nullopt for one that is missing.
+    struct PresetFilesOnDisk
+    {
+        std::optional<std::string> json;
+        std::optional<std::string> info;
+
+        static PresetFilesOnDisk read(const boost::filesystem::path &file);
+        bool operator==(const PresetFilesOnDisk &rhs) const { return json == rhs.json && info == rhs.info; }
+    };
+
     // One preset file read and flattened against the presets already in this
     // collection, before anything the collection shares has been touched.
     struct UserPresetLoad
@@ -914,6 +932,9 @@ private:
         bool        discard_file { false };
         // The .info file read beside the preset, which commit logs.
         std::string info_file;
+        // Both files as they were before the preset was read from them. Resolve runs
+        // without the instance lock, so commit compares this with the disk under it.
+        PresetFilesOnDisk on_disk;
         // Counted and logged by commit, in the order the directory listed the files.
         std::vector<std::string>   errors;
         PresetsConfigSubstitutions substitutions;
@@ -1102,7 +1123,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

@@ -1,3 +1,4 @@
+#include <atomic>
 #include <cassert>
 #include <chrono>
 #include <ctime>
@@ -13,6 +14,7 @@
 #include "libslic3r.h"
 #include "I18N.hpp"
 #include "Utils.hpp"
+#include "InstanceLock.hpp"
 #include "LocalesUtils.hpp"
 #include "Model.hpp"
 #include "TriangleSelector.hpp"
@@ -1233,6 +1235,13 @@ 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 &metadata_file, BundleMetadata &metadata) {
+        InstanceLock instance_lock(user_presets_lock_path(read_only));
+        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)) {
@@ -1246,7 +1255,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(metadata_file, metadata)) continue;
             metadata.print_presets.clear();
             metadata.filament_presets.clear();
             metadata.printer_presets.clear();
@@ -1281,7 +1290,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(metadata_file, metadata)) continue;
             metadata.print_presets.clear();
             metadata.filament_presets.clear();
             metadata.printer_presets.clear();
@@ -1615,10 +1624,12 @@ 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");
+            // Under cache/, per process and per import, so two instances importing
+            // at once do not clear each other's extraction and no preset scan reads it.
+            static std::atomic<unsigned> import_counter{0};
+            fs::path temp_folder(fs::path(data_dir()) / "cache" / ("import." + std::to_string(get_current_pid()) + "." + std::to_string(import_counter++)));
             std::string user_default_temp_dir = temp_folder.make_preferred().string();
-            if (fs::exists(temp_folder)) fs::remove_all(temp_folder);
+            fs::create_directories(temp_folder, ec);
-            fs::create_directory(temp_folder, ec);
             if (ec) BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << " create directory failed: " << ec.message();
 
             file = boost::filesystem::path(file).make_preferred().string();
@@ -1630,6 +1641,9 @@ PresetsConfigSubstitutions PresetBundle::import_presets(std::vector<std::string>
             status        = mz_zip_reader_init_cfile(&zip_archive, zipFile, 0, MZ_ZIP_FLAG_CASE_SENSITIVE | MZ_ZIP_FLAG_IGNORE_PATH);
             if (MZ_FALSE == status) {
                 BOOST_LOG_TRIVIAL(info) << __FUNCTION__ << " Failed to initialize reader ZIP archive";
+                if (zipFile != nullptr)
+                    std::fclose(zipFile);
+                fs::remove_all(temp_folder, ec);
                 return substitutions;
             } else {
                 BOOST_LOG_TRIVIAL(info) << __FUNCTION__ << " Success to initialize reader ZIP archive";
@@ -2237,10 +2251,9 @@ 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);
+    boost::system::error_code ec;
-    if (fs::exists(folder)) {
+    InstanceLock instance_lock(user_presets_lock_path());
-        fs::remove_all(folder);
+    fs::remove_all(fs::path(dir_user_presets), ec);
-    }
 }
 
 void PresetBundle::update_system_preset_setting_ids(std::map<std::string, std::map<std::string, std::string>>& system_presets)
@@ -8187,9 +8200,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());
-        {
-            boost::nowide::ofstream ofs(tmp_path, std::ios::binary | std::ios::trunc);
-            if (!ofs.is_open()) {
-                BOOST_LOG_TRIVIAL(warning) << "VendorCacheFile: cannot open for writing: " << tmp_path;
-                return false;
-            }
         CacheFileHeader fhdr;
         fhdr.magic     = CACHE_MAGIC;
         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));
+        const std::string_view header(reinterpret_cast<const char*>(&fhdr), sizeof(fhdr));
-            ofs.write(blob.data(), static_cast<std::streamsize>(blob.size()));
+        if (const std::error_code ec = write_file_atomically(path, { header, std::string_view(blob) }, /*binary=*/true)) {
-            ofs.close();   // flush; close() raises failbit on error
+            BOOST_LOG_TRIVIAL(warning) << "VendorCacheFile: write failed (" << path << "): " << ec.message();
-            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,21 @@ 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. Text mode unless `binary`, so Windows writes CRLF
+// as the streams this replaces did. A target that is not a regular file (a
+// device or pipe) is written in place, since replacing it would change what it
+// is, and so is an existing target beside which no temporary can be created or
+// whose replace the filesystem refuses; a symlink is followed and the file it
+// names is replaced. On Windows a reader holding the
+// target open without sharing its deletion, which the C runtime does not, makes
+// the replace fall back to the in-place write too, so an unlocked reader there
+// can still see a partial file.
+extern std::error_code write_file_atomically(const std::string &path, std::initializer_list<std::string_view> chunks, bool binary = false);
+inline std::error_code write_file_atomically(const std::string &path, const std::string &content, bool binary = false)
+	{ return write_file_atomically(path, { std::string_view(content) }, binary); }
 
 enum CopyFileResult {
 	SUCCESS = 0,

src/libslic3r/utils.cpp

@@ -9,6 +9,8 @@
 #include <stdio.h>
 #include <filesystem>
 #include <sstream>
+#include <cerrno>
+#include <mutex>
 #include <iomanip>
 #include <algorithm>
 #include <cmath>
@@ -703,13 +705,99 @@ 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, each with the error it sees fit; every error
+	// is worth the remove-then-rename this always did, except the ones no retry
+	// can help: nothing at the source, a different device, or a directory where
+	// a file was expected and the reverse.
+	const bool worth_retrying = err != ENOENT && err != EXDEV && err != ENOTDIR && err != EISDIR;
+	if (worth_retrying && boost::nowide::remove(to.c_str()) == 0 && boost::nowide::rename(from.c_str(), to.c_str()) == 0)
+		return {};
+	return std::make_error_code(static_cast<std::errc>(err));
 #endif
 }
 
+static std::error_code write_whole_file(const std::string &path, std::initializer_list<std::string_view> chunks, bool binary)
+{
+	errno = 0;
+	FILE *file = boost::nowide::fopen(path.c_str(), binary ? "wb" : "w");
+	if (file == nullptr)
+		return std::make_error_code(errno != 0 ? static_cast<std::errc>(errno) : std::errc::io_error);
+	bool ok = true;
+	for (const std::string_view chunk : chunks)
+		ok = ok && std::fwrite(chunk.data(), 1, chunk.size(), file) == chunk.size();
+	ok = ok && std::fflush(file) == 0;
+	const int err = ok ? 0 : errno;
+	ok = std::fclose(file) == 0 && ok;
+	if (ok)
+		return {};
+	return std::make_error_code(err != 0 ? static_cast<std::errc>(err) : std::errc::io_error);
+}
+
+// The in-place fallback truncates the target, so two threads of this process
+// on the same file must not both be in it. One mutex for all such writes: they
+// are the rare case. Never freed, like the InstanceLock registry, so a save
+// during static destruction still finds it.
+static std::error_code write_in_place(const std::string &path, std::initializer_list<std::string_view> chunks, bool binary)
+{
+	static auto *mutex = new std::mutex();
+	std::lock_guard<std::mutex> guard(*mutex);
+	return write_whole_file(path, chunks, binary);
+}
+
+std::error_code write_file_atomically(const std::string &path, std::initializer_list<std::string_view> chunks, bool binary)
+{
+	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_symlink(target)) {
+		// A config or preset kept in a dotfiles repository: the link stays,
+		// the file it points to is replaced like any other.
+		const boost::filesystem::path resolved = boost::filesystem::canonical(path, bec);
+		if (! bec && boost::filesystem::is_regular_file(resolved, bec))
+			return write_file_atomically(resolved.string(), chunks, binary);
+	}
+	if (target_exists && ! boost::filesystem::is_regular_file(target))
+		return write_in_place(path, chunks, binary);
+
+	// 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 (const std::error_code ec = write_whole_file(tmp_path, chunks, binary)) {
+		boost::nowide::remove(tmp_path.c_str());
+		if (! target_exists)
+			return ec;
+		// A directory that lets this process write its files but not create
+		// one: losing the save is worse than a reader seeing a partial file.
+		BOOST_LOG_TRIVIAL(warning) << "Cannot create a temporary beside " << path << " (" << ec.message() << "); writing in place";
+		return write_in_place(path, chunks, binary);
+	}
+#ifndef _WIN32
+	// Not on Windows, where 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.
+		BOOST_LOG_TRIVIAL(warning) << "Cannot replace " << path << " (" << ec.message() << "); writing in place";
+		return write_in_place(path, chunks, binary);
+	}
+	return {};
+}
+
 #ifdef __linux__
 // Copied from boost::filesystem.
 // Called by copy_file_linux() in case linux sendfile() API is not supported.

src/slic3r/GUI/GLCanvas3D.cpp

@@ -6546,11 +6546,8 @@ void GLCanvas3D::render_thumbnail_internal(ThumbnailData& thumbnail_data, const
     //    glsafe(::glClearColor(1.0f, 1.0f, 1.0f, 1.0f));
     BOOST_LOG_TRIVIAL(info) << boost::format("render_thumbnail: finished");
 
-    // Puts the canvas viewport back in place of the thumbnail one set above. The CLI renders
+    // Puts the canvas viewport back in place of the thumbnail one set above.
-    // thumbnails with no application and no plater, so there is no canvas viewport to restore.
+    wxGetApp().plater()->get_camera().apply_viewport();
-    if (wxTheApp != nullptr)
-        if (Plater *plater = wxGetApp().plater(); plater != nullptr)
-            plater->get_camera().apply_viewport();
 }
 
 void GLCanvas3D::render_thumbnail_framebuffer(ThumbnailData& thumbnail_data, unsigned int w, unsigned int h, const ThumbnailsParams& thumbnail_params,

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"
@@ -3570,7 +3571,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();
 
     });
@@ -7664,8 +7665,11 @@ 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;
+                                InstanceLock instance_lock(user_presets_lock_path());
                                 boost::filesystem::remove_all(bundle_folder, ec);
+                            }
 
                             preset_bundle->bundles.WriteLock();
                             preset_bundle->bundles.m_bundles.erase(bundle.id);
@@ -8952,6 +8956,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;
     }
@@ -9014,17 +9019,21 @@ 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;
+            {
+                InstanceLock instance_lock(user_presets_lock_path());
+                if (!fs::exists(preset_file))
+                    setting_id = extract_setting_id_from_info(info_file.string());
+            }
             if (!setting_id.empty()) {
                 // Add to need_delete_presets
                 delete_preset_from_cloud(setting_id, info_file.string());
                 BOOST_LOG_TRIVIAL(info) << "Found orphaned .info file on startup: " << info_file.string();
             }
         }
-        }
         if (ec)
             BOOST_LOG_TRIVIAL(warning) << "scan_orphaned_info_files: failed to scan " << type_dir.string() << ": " << ec.message();
     }

src/slic3r/GUI/WebGuideDialog.cpp

@@ -1478,9 +1478,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;
@@ -1489,18 +1487,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;
-            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

@@ -32,7 +32,6 @@ add_executable(${_TEST_NAME}_tests
     test_geometry.cpp
     test_multimaterial_segmentation.cpp
     test_placeholder_parser.cpp
-    test_png_read_write.cpp
     test_polygon.cpp
     test_mutable_polygon.cpp
     test_mutable_priority_queue.cpp
@@ -57,6 +56,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,228 @@
+#include <catch2/catch_all.hpp>
+
+#include <atomic>
+#include <chrono>
+#include <thread>
+
+#include <boost/filesystem.hpp>
+
+#include "libslic3r/InstanceLock.hpp"
+#include "test_utils.hpp"
+
+#ifdef _WIN32
+#include <boost/interprocess/sync/file_lock.hpp>
+#include <boost/nowide/convert.hpp>
+#include <boost/nowide/fstream.hpp>
+#else
+#include <fcntl.h>
+#include <sys/file.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);
+}
+
+// Holds the OS lock on a file through a handle of its own, as another instance would. The lock
+// belongs to the handle on Windows and to the open file description elsewhere, so the guard's
+// handle is refused while this one holds it.
+class OtherHolder
+{
+public:
+    explicit OtherHolder(const std::string &path)
+    {
+#ifdef _WIN32
+        boost::nowide::ofstream(path, std::ios::app).close();
+        m_lock = boost::interprocess::file_lock(boost::nowide::widen(path).c_str());
+        m_held = m_lock.try_lock();
+#else
+        m_fd   = ::open(path.c_str(), O_RDWR | O_CREAT, 0644);
+        m_held = m_fd >= 0 && ::flock(m_fd, LOCK_EX | LOCK_NB) == 0;
+#endif
+    }
+    ~OtherHolder() { release(); }
+    OtherHolder(const OtherHolder &) = delete;
+    OtherHolder &operator=(const OtherHolder &) = delete;
+
+    bool held() const { return m_held; }
+    void release()
+    {
+#ifdef _WIN32
+        if (m_held)
+            m_lock.unlock();
+        m_lock = boost::interprocess::file_lock();
+#else
+        if (m_fd >= 0)
+            ::close(m_fd);
+        m_fd = -1;
+#endif
+        m_held = false;
+    }
+
+private:
+#ifdef _WIN32
+    boost::interprocess::file_lock m_lock;
+#else
+    int m_fd{-1};
+#endif
+    bool m_held{false};
+};
+
+TEST_CASE("InstanceLock yields to a lock held through another handle and reports it", "[InstanceLock]")
+{
+    ScopedTemporaryFile lock_file(".lock");
+    const std::string   path = lock_file.string();
+    ScopedStaticValue   cooldown(InstanceLock::cooldown, 300ms);
+
+    OtherHolder other(path);
+    REQUIRE(other.held());
+
+    bool locked_while_other_holds;
+    {
+        InstanceLock lock(path, 100ms);
+        locked_while_other_holds = lock.locked();
+    }
+    // The timed-out wait starts a cool-down: the next guard does not touch the file.
+    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;
+    other.release();
+
+    REQUIRE_FALSE(locked_while_other_holds);
+    REQUIRE_FALSE(locked_during_cooldown);
+    REQUIRE(cooldown_wait < 4000ms);
+    // Once the cool-down passes, the lock the other holder released is taken again.
+    std::this_thread::sleep_for(400ms);
+    InstanceLock lock(path);
+    REQUIRE(lock.locked());
+}

tests/libslic3r/test_png_read_write.cpp

@@ -1,83 +0,0 @@
-#include <catch2/catch_all.hpp>
-
-#include <cstdint>
-#include <fstream>
-#include <iterator>
-#include <vector>
-
-#include <boost/filesystem.hpp>
-
-#include "libslic3r/PNGReadWrite.hpp"
-
-using namespace Slic3r;
-
-// libpng reports a corrupt or truncated file by longjmp()ing out of the decoder, so the decoders have
-// to come back with false rather than crash or hand back a half filled image.
-namespace {
-
-// A real PNG, produced by the writer next door, so the bytes are a file libpng accepts.
-std::vector<uint8_t> encoded_png(size_t w, size_t h)
-{
-    std::vector<uint8_t> pixels(w * h);
-    for (size_t i = 0; i < pixels.size(); ++ i)
-        pixels[i] = uint8_t((i * 7) % 256);
-
-    const boost::filesystem::path path = boost::filesystem::temp_directory_path() /
-                                         boost::filesystem::unique_path("png_rw_%%%%%%%%.png");
-    REQUIRE(png::write_gray_to_file(path.string(), w, h, pixels));
-    std::vector<uint8_t> bytes;
-    {
-        std::ifstream ifs(path.string(), std::ios::binary);
-        bytes.assign(std::istreambuf_iterator<char>(ifs), std::istreambuf_iterator<char>());
-    }
-    boost::system::error_code ec;
-    boost::filesystem::remove(path, ec);
-    REQUIRE(bytes.size() > 64);
-    return bytes;
-}
-
-png::ReadBuf buf_of(const std::vector<uint8_t> &bytes, size_t size)
-{
-    return png::ReadBuf{ bytes.data(), size };
-}
-
-} // namespace
-
-TEST_CASE("A whole PNG decodes", "[PNG]") {
-    const std::vector<uint8_t> bytes = encoded_png(24, 16);
-
-    png::ImageGreyscale grey;
-    REQUIRE(png::decode_png(buf_of(bytes, bytes.size()), grey));
-    CHECK(grey.cols == 24);
-    CHECK(grey.rows == 16);
-    CHECK(grey.buf.size() == 24 * 16);
-}
-
-TEST_CASE("A truncated PNG is refused instead of crashing", "[PNG]") {
-    const std::vector<uint8_t> bytes = encoded_png(64, 64);
-
-    // Cut past the signature: inside the header, and inside the pixel data. Not in the trailing
-    // chunks - decode_png() does not read those, so a file missing only its IEND still decodes, and
-    // that is the pre-existing contract rather than anything this change touches.
-    const size_t size = GENERATE_COPY(size_t(16), size_t(40), bytes.size() / 2, bytes.size() * 3 / 4);
-    REQUIRE(size < bytes.size());
-
-    png::ImageGreyscale grey;
-    CHECK_FALSE(png::decode_png(buf_of(bytes, size), grey));
-
-    png::ImageColorscale colour;
-    CHECK_FALSE(png::decode_colored_png(buf_of(bytes, size), colour));
-}
-
-TEST_CASE("A PNG whose body is garbage is refused", "[PNG]") {
-    std::vector<uint8_t> bytes = encoded_png(32, 32);
-    // Keep the signature, scribble over everything after it.
-    for (size_t i = 8; i < bytes.size(); ++ i)
-        bytes[i] = uint8_t(0xA5);
-
-    png::ImageGreyscale grey;
-    CHECK_FALSE(png::decode_png(buf_of(bytes, bytes.size()), grey));
-
-    png::ImageColorscale colour;
-    CHECK_FALSE(png::decode_colored_png(buf_of(bytes, bytes.size()), colour));
-}

tests/libslic3r/test_preset_bundle_loading.cpp

@@ -5,6 +5,7 @@
 #include <fstream>
 
 #include "libslic3r/PresetBundle.hpp"
+#include "libslic3r/ParallelResolve.hpp"
 #include "libslic3r/AppConfig.hpp"
 #include "libslic3r/Model.hpp"
 #include "libslic3r/TriangleMesh.hpp"
@@ -17,6 +18,10 @@
 #include <iostream>
 #include <initializer_list>
 
+#ifndef _WIN32
+#include <unistd.h> // geteuid
+#endif
+
 using namespace Slic3r;
 
 namespace {
@@ -5684,3 +5689,214 @@ TEST_CASE("A project saved with pressure advance per filament applies it to ever
     check_double_vector(pla.opt<ConfigOptionFloatsNullable>("filament_flow_ratio")->values, { 0.95, 0.96 });
     check_double_vector(petg.opt<ConfigOptionFloatsNullable>("filament_flow_ratio")->values, { 0.97 });
 }
+
+TEST_CASE("A user preset saved over by another instance while its directory loads is read again under the lock", "[Preset][Bundle][InstanceLock]")
+{
+    ScopedTemporaryDir   temp_dir;
+    ScopedDataDir        data_dir_scope(temp_dir.path());
+    RenameTestCollection coll;
+
+    Preset &parent = add_inmemory_preset(coll, "Parent Process");
+    parent.config.option<ConfigOptionFloat>("layer_height", true)->value = 0.24;
+    parent.is_system = true;
+
+    // Fewer than one batch, so every file is read before the first one is committed.
+    constexpr int  children = 8;
+    static_assert(children <= int(resolve_batch_size));
+    const fs::path dir      = temp_dir.path() / PRESET_PRINT_NAME;
+    for (int i = 0; i < children; ++ i)
+        write_minimal_child(dir / ("Child " + std::to_string(i) + ".json"), "Child " + std::to_string(i), "Parent Process");
+
+    // The first commit stands in for another instance saving every other preset after
+    // they were read and before they are committed.
+    std::string first;
+    auto save_the_others = [&](Preset &preset) {
+        if (! first.empty())
+            return;
+        first = preset.name;
+        for (int i = 0; i < children; ++ i) {
+            const std::string name = "Child " + std::to_string(i);
+            if (name != first)
+                std::ofstream((dir / (name + ".json")).string())
+                    << R"({"type":"process","name":")" << name
+                    << R"(","from":"User","version":"1.0.0","inherits":"Parent Process","layer_height":"0.3"})";
+        }
+    };
+    PresetsConfigSubstitutions substitutions;
+    coll.load_presets(temp_dir.path().string(), PRESET_PRINT_NAME, substitutions,
+                      ForwardCompatibilitySubstitutionRule::Disable, save_the_others);
+
+    REQUIRE_FALSE(first.empty());
+    CHECK(coll.error_count() == 0);
+    for (int i = 0; i < children; ++ i) {
+        const std::string name  = "Child " + std::to_string(i);
+        const Preset     *child = coll.find_preset(name);
+        REQUIRE(child != nullptr);
+        CHECK_THAT(child->config.opt_float("layer_height"), Catch::Matchers::WithinAbs(name == first ? 0.24 : 0.3, 1e-9));
+    }
+}
+
+TEST_CASE("A user preset removed by another instance while its directory loads is not installed", "[Preset][Bundle][InstanceLock]")
+{
+    ScopedTemporaryDir   temp_dir;
+    ScopedDataDir        data_dir_scope(temp_dir.path());
+    RenameTestCollection coll;
+
+    Preset &parent = add_inmemory_preset(coll, "Parent Process");
+    parent.is_system = true;
+
+    // Fewer than one batch, so every file is read before the first one is committed.
+    constexpr int  children = 8;
+    static_assert(children <= int(resolve_batch_size));
+    const fs::path dir      = temp_dir.path() / PRESET_PRINT_NAME;
+    for (int i = 0; i < children; ++ i)
+        write_minimal_child(dir / ("Child " + std::to_string(i) + ".json"), "Child " + std::to_string(i), "Parent Process");
+
+    std::string first;
+    auto remove_the_others = [&](Preset &preset) {
+        if (! first.empty())
+            return;
+        first = preset.name;
+        for (int i = 0; i < children; ++ i)
+            if (const std::string name = "Child " + std::to_string(i); name != first)
+                fs::remove(dir / (name + ".json"));
+    };
+    PresetsConfigSubstitutions substitutions;
+    coll.load_presets(temp_dir.path().string(), PRESET_PRINT_NAME, substitutions,
+                      ForwardCompatibilitySubstitutionRule::Disable, remove_the_others);
+
+    REQUIRE_FALSE(first.empty());
+    CHECK(coll.error_count() == 0);
+    CHECK(coll.size() == 3); // the default preset, the parent and the first child
+    CHECK(coll.find_preset(first) != nullptr);
+}
+
+TEST_CASE("Without the instance lock a user preset loads but its file is not written back", "[Preset][Bundle][InstanceLock]")
+{
+    ScopedTemporaryDir temp_dir;
+    ScopedDataDir      data_dir_scope(temp_dir.path());
+    // A directory where the lock file belongs, so the lock cannot be taken.
+    fs::create_directories(fs::path(user_presets_lock_path()));
+    PresetBundle       bundle;
+    const fs::path     file = temp_dir.path() / PRESET_FILAMENT_NAME / "My PLA @Test Printer.json";
+    write_preset_with_inherits(bundle.filaments.default_preset().config, file, "My PLA @Test Printer", std::string());
+    const std::string before = read_file(file);
+
+    PresetsConfigSubstitutions substitutions;
+    bundle.filaments.load_presets(temp_dir.path().string(), PRESET_FILAMENT_NAME, substitutions,
+                                  ForwardCompatibilitySubstitutionRule::EnableSilent);
+
+    const Preset *preset = bundle.filaments.find_preset("My PLA @Test Printer");
+    REQUIRE(preset != nullptr);
+    CHECK(preset->config.option<ConfigOptionStrings>("compatible_printers")->values == std::vector<std::string>{ "Test Printer" });
+    CHECK(read_file(file) == before);
+}
+
+#ifndef _WIN32
+// File permissions stop reading only on POSIX.
+TEST_CASE("A user preset that cannot be read under the instance lock is counted and removed", "[Preset][Bundle][InstanceLock]")
+{
+    if (::geteuid() == 0)
+        SKIP("file permissions do not stop root");
+    ScopedTemporaryDir   temp_dir;
+    ScopedDataDir        data_dir_scope(temp_dir.path());
+    RenameTestCollection coll;
+
+    Preset &parent = add_inmemory_preset(coll, "Parent Process");
+    parent.is_system = true;
+    const fs::path file = temp_dir.path() / PRESET_PRINT_NAME / "Unreadable.json";
+    write_minimal_child(file, "Unreadable", "Parent Process");
+    fs::permissions(file, fs::no_perms);
+
+    PresetsConfigSubstitutions substitutions;
+    coll.load_presets(temp_dir.path().string(), PRESET_PRINT_NAME, substitutions,
+                      ForwardCompatibilitySubstitutionRule::Disable);
+
+    CHECK(coll.error_count() == 1);
+    CHECK(coll.find_preset("Unreadable") == nullptr);
+    CHECK_FALSE(fs::exists(file));
+}
+#endif
+
+#ifndef _WIN32
+// Creating a symlink needs no privilege only on POSIX.
+TEST_CASE("A user preset that is a symlink to itself is counted and removed while the rest still load", "[Preset][Bundle][InstanceLock]")
+{
+    ScopedTemporaryDir   temp_dir;
+    ScopedDataDir        data_dir_scope(temp_dir.path());
+    RenameTestCollection coll;
+
+    Preset &parent = add_inmemory_preset(coll, "Parent Process");
+    parent.is_system = true;
+    const fs::path dir = temp_dir.path() / PRESET_PRINT_NAME;
+    write_minimal_child(dir / "Good A.json", "Good A", "Parent Process");
+    write_minimal_child(dir / "Good B.json", "Good B", "Parent Process");
+    const fs::path loop = dir / "Loop.json";
+    fs::create_symlink(loop.filename(), loop);
+
+    PresetsConfigSubstitutions substitutions;
+    REQUIRE_NOTHROW(coll.load_presets(temp_dir.path().string(), PRESET_PRINT_NAME, substitutions,
+                                      ForwardCompatibilitySubstitutionRule::Disable));
+
+    CHECK(coll.error_count() == 1);
+    CHECK(coll.find_preset("Good A") != nullptr);
+    CHECK(coll.find_preset("Good B") != nullptr);
+    CHECK(coll.find_preset("Loop") == nullptr);
+    boost::system::error_code ec;
+    CHECK(fs::symlink_status(loop, ec).type() == fs::file_not_found);
+}
+#endif
+
+#ifndef _WIN32
+// Creating a symlink needs no privilege only on POSIX.
+TEST_CASE("A user preset symlinked to a missing target is counted but the link is kept", "[Preset][Bundle][InstanceLock]")
+{
+    ScopedTemporaryDir   temp_dir;
+    ScopedDataDir        data_dir_scope(temp_dir.path());
+    RenameTestCollection coll;
+
+    const fs::path dir  = temp_dir.path() / PRESET_PRINT_NAME;
+    const fs::path link = dir / "Linked.json";
+    fs::create_directories(dir);
+    fs::create_symlink(temp_dir.path() / "unmounted" / "Linked.json", link);
+
+    PresetsConfigSubstitutions substitutions;
+    coll.load_presets(temp_dir.path().string(), PRESET_PRINT_NAME, substitutions,
+                      ForwardCompatibilitySubstitutionRule::Disable);
+
+    CHECK(coll.error_count() == 1);
+    CHECK(coll.find_preset("Linked") == nullptr);
+    boost::system::error_code ec;
+    CHECK(fs::symlink_status(link, ec).type() == fs::symlink_file);
+}
+#endif
+
+#ifndef _WIN32
+// The read-only bit on a directory stops file creation only on POSIX.
+TEST_CASE("A user filament whose derived compatible printer cannot be written back still loads with the failure counted", "[Preset][Bundle]")
+{
+    if (::geteuid() == 0)
+        SKIP("a read-only directory does not stop root");
+    ScopedTemporaryDir temp_dir;
+    PresetBundle       bundle;
+    const fs::path     dir  = temp_dir.path() / PRESET_FILAMENT_NAME;
+    const fs::path     file = dir / "My PLA @Test Printer.json";
+    write_preset_with_inherits(bundle.filaments.default_preset().config, file, "My PLA @Test Printer", std::string());
+    const std::string before = read_file(file);
+
+    fs::permissions(file, fs::owner_read);
+    fs::permissions(dir, fs::owner_read | fs::owner_exe);
+    PresetsConfigSubstitutions substitutions;
+    bundle.filaments.load_presets(temp_dir.path().string(), PRESET_FILAMENT_NAME, substitutions,
+                                  ForwardCompatibilitySubstitutionRule::EnableSilent);
+    // Restored before any assertion, so a failure never leaves an unremovable directory behind.
+    fs::permissions(dir, fs::owner_all);
+    fs::permissions(file, fs::owner_read | fs::owner_write);
+
+    const Preset *preset = bundle.filaments.find_preset("My PLA @Test Printer");
+    REQUIRE(preset != nullptr);
+    CHECK(preset->config.option<ConfigOptionStrings>("compatible_printers")->values == std::vector<std::string>{ "Test Printer" });
+    CHECK(bundle.filaments.error_count() == 1);
+    CHECK(read_file(file) == before);
+}
+#endif

tests/libslic3r/test_utils.cpp

@@ -10,6 +10,8 @@
 #include <cctype>
 #include <fstream>
 #include <string>
+#include <thread>
+#include <system_error>
 
 #ifndef _WIN32
 #include <unistd.h>     // getuid
@@ -62,6 +64,113 @@ 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));
+}
+
+#ifndef _WIN32
+// The read-only bit on a directory stops file creation only on POSIX.
+TEST_CASE("write_file_atomically writes in place when no temporary can be created beside an existing target", "[utils]") {
+    if (::geteuid() == 0)
+        SKIP("a read-only directory does not stop root");
+    ScopedTemporaryDir dir;
+    const boost::filesystem::path target = dir.path() / "preset.json";
+    REQUIRE_FALSE(write_file_atomically(target.string(), "first"));
+    boost::filesystem::permissions(dir.path(), boost::filesystem::owner_read | boost::filesystem::owner_exe);
+
+    const std::error_code replaced = write_file_atomically(target.string(), "second");
+    const std::error_code created  = write_file_atomically((dir.path() / "new.json").string(), "x");
+    // Restored before any assertion, so a failure never leaves an unremovable directory behind.
+    boost::filesystem::permissions(dir.path(), boost::filesystem::owner_all);
+
+    REQUIRE_FALSE(replaced);
+    REQUIRE(created == std::errc::permission_denied);
+    std::string content;
+    load_string_file(target, content);
+    REQUIRE(content == "second");
+}
+#endif
+
+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("copy_file reports the OS error when the destination cannot be written", "[utils]") {
     ScopedTemporaryFile source(".txt");
     {

tests/libslic3r/test_vendor_cache.cpp

@@ -1804,24 +1804,6 @@ TEST_CASE("a header claiming more body than the file holds is rejected", "[Vendo
     REQUIRE_FALSE(bundle.load_vendor_cache(cache, "Bounded", Semver(1, 0, 0)));
 }
 
-TEST_CASE("a failed write leaves the previous cache in place", "[VendorCache]")
-{
-    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,
-    // and must not have destroyed what was already there to find that out.
-    const fs::path blocker = fs::path(cache + "." + std::to_string(get_current_pid()) + ".tmp");
-    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);
-}
-
 TEST_CASE("a cache written by another build's option ordering still loads", "[VendorCache]")
 {
     // The regression the fingerprint used to prevent by refusing the file