Give Each Atomic Write Its Own Temporary and Route the Config Through It

Two threads writing one target without a lock shared the same temporary
name, so one could truncate it under the other; the name now carries a
per-call counter. The helper takes its content as chunks, which lets the
vendor cache hand over its header and body without copying the blob, and
AppConfig::save() goes through it too, including the Windows backup copy,
so the config's temporaries get the same handling as everyone else's.

Copying the target's permissions onto the temporary set a read-only bit
that stopped the rename itself on Windows; they are applied after the
rename there. The two-step rename fallback triggers on whatever error a
mount reports when the target is still there, since FUSE, gvfs and MTP
refuse a replacing rename with errors other than the three it handled.

Every successful write sweeps stale temporaries of its own target, so
leftovers beside the bundle metadata, physical printers, plugin config
and profile cache are covered, and the older OrcaSlicer.conf.<pid> form
with them. The bundle metadata guard is taken per file, never while the
bundle registry's writer lock is held, so the two are not taken in both
orders. The lock guard's open-and-lock steps are one block, and the
read-only choice lives in user_presets_lock_path() instead of at each
call site.

The vendor cache test that blocked the old fixed temporary name with a
directory provokes the failure through permissions instead.

src/libslic3r/AppConfig.cpp

@@ -737,10 +737,6 @@ std::string AppConfig::load()
 
     // Keep another instance from replacing or restoring the file mid-read.
     InstanceLock instance_lock(lock_path());
-    if (instance_lock.locked()) {
-        const boost::filesystem::path conf(config_path());
-        remove_stale_temp_files(conf.parent_path(), conf.filename().string());
-    }
 
     // 1) Read the complete config file into a boost::property_tree.
     namespace pt = boost::property_tree;
@@ -992,7 +988,6 @@ void AppConfig::save()
     // to avoid race conditions with multiple instances of Slic3r
     const auto path = config_path();
     InstanceLock instance_lock(lock_path());
-    std::string path_pid = (boost::format("%1%.%2%.tmp") % path % get_current_pid()).str();
 
     json j;
 
@@ -1122,37 +1117,25 @@ void AppConfig::save()
 
         j["local_machines"][local_machine.first] = m_json;
     }
-    boost::nowide::ofstream c;
-    c.open(path_pid, std::ios::out | std::ios::trunc);
-    c << j.dump(1, '\t') << std::endl;
-
+    std::string config_str = j.dump(1, '\t');
 #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
+    // 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.
-    c << appconfig_md5_hash_line(j.dump(1, '\t'));
+    // load() verifies the checksum over the text up to the closing brace, so it is taken before the newline.
+    const std::string md5_line = appconfig_md5_hash_line(config_str);
+    config_str += "\n";
+    config_str += md5_line;
+    const std::string backup_path = (boost::format("%1%.bak") % path).str();
+    if (const std::error_code ec = write_file_atomically(backup_path, config_str))
+        BOOST_LOG_TRIVIAL(error) << "Failed to write the backup configuration " << backup_path << ": " << ec.message();
+#else
+    config_str += "\n";
 #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;
+    if (const std::error_code ec = write_file_atomically(path, config_str)) {
+        BOOST_LOG_TRIVIAL(error) << "Failed to write the configuration " << path << ": " << ec.message();
+        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.
-    if (const std::error_code ec = rename_file(path_pid, path))
-        BOOST_LOG_TRIVIAL(error) << "Failed to replace " << path << " with the new configuration: " << ec.message();
     m_dirty = false;
 }
 
@@ -1162,10 +1145,6 @@ std::string AppConfig::load()
 {
     // Keep another instance from replacing or restoring the file mid-read.
     InstanceLock instance_lock(lock_path());
-    if (instance_lock.locked()) {
-        const boost::filesystem::path conf(config_path());
-        remove_stale_temp_files(conf.parent_path(), conf.filename().string());
-    }
 
     // 1) Read the complete config file into a boost::property_tree.
     namespace pt = boost::property_tree;
@@ -1305,7 +1284,6 @@ void AppConfig::save()
     // to avoid race conditions with multiple instances of Slic3r
     const auto path = config_path();
     InstanceLock instance_lock(lock_path());
-    std::string path_pid = (boost::format("%1%.%2%.tmp") % path % get_current_pid()).str();
 
     std::stringstream config_ss;
     if (m_mode == EAppMode::Editor)
@@ -1342,35 +1320,19 @@ void AppConfig::save()
     config_ss << std::endl;
 
     std::string config_str = config_ss.str();
-    boost::nowide::ofstream c;
-    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
+    // 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.
-    c << appconfig_md5_hash_line(config_str);
+    config_str += appconfig_md5_hash_line(config_str);
+    const std::string backup_path = (boost::format("%1%.bak") % path).str();
+    if (const std::error_code ec = write_file_atomically(backup_path, config_str))
+        BOOST_LOG_TRIVIAL(error) << "Failed to write the backup configuration " << backup_path << ": " << ec.message();
 #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;
+    if (const std::error_code ec = write_file_atomically(path, config_str)) {
+        BOOST_LOG_TRIVIAL(error) << "Failed to write the configuration " << path << ": " << ec.message();
+        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.
-    if (const std::error_code ec = rename_file(path_pid, path))
-        BOOST_LOG_TRIVIAL(error) << "Failed to replace " << path << " with the new configuration: " << ec.message();
     m_dirty = false;
 }
 #endif

src/libslic3r/InstanceLock.cpp

@@ -72,11 +72,8 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli
     m_slot = &slot_for(lock_file_path);
     m_slot->mutex.lock();
     const auto now = std::chrono::steady_clock::now();
-    if (m_slot->depth++ == 0 && now >= m_slot->retry_at) {
-        if (! m_slot->file_lock && ! open_lock_file(m_slot->file_lock, lock_file_path))
-            m_slot->retry_at = now + cooldown;
-    }
-    if (m_slot->depth == 1 && m_slot->file_lock && now >= m_slot->retry_at) {
+    if (m_slot->depth++ == 0 && now >= m_slot->retry_at &&
+        (m_slot->file_lock || open_lock_file(m_slot->file_lock, lock_file_path))) {
         const bool wait     = now >= m_slot->skip_waiting_until;
         const auto deadline = now + timeout;
         for (;;) {
@@ -102,6 +99,8 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli
             }
             std::this_thread::sleep_for(std::chrono::milliseconds(5));
         }
+    } else if (m_slot->depth == 1 && ! m_slot->file_lock && now >= m_slot->retry_at) {
+        m_slot->retry_at = now + cooldown;
     }
     m_locked = m_slot->file_locked;
 }

src/libslic3r/Preset.cpp

@@ -107,9 +107,9 @@ std::string get_preset_canonical_name(const std::string &preset_bare_name, const
     }
 }
 
-std::string user_presets_lock_path()
+std::string user_presets_lock_path(bool read_only)
 {
-    return data_dir().empty() ? std::string() : (fs::path(data_dir()) / (PRESET_USER_DIR ".lock")).string();
+    return read_only || data_dir().empty() ? std::string() : (fs::path(data_dir()) / (PRESET_USER_DIR ".lock")).string();
 }
 
 std::string get_preset_bare_name(const std::string &canonical_name)
@@ -1723,9 +1723,7 @@ void PresetCollection::load_presets(
     Preset::get_extruder_names_and_keysets(m_type, extruder_id_name, extruder_variant_name, &key_set1, &key_set2);
 
     // Held across the scan so no instance replaces or removes a file mid-read.
-    // A read-only scan (the CLI) never rewrites or deletes, and many CLI jobs
-    // may share one data dir, so it does not queue behind the others.
-    InstanceLock instance_lock(read_only ? std::string() : user_presets_lock_path());
+    InstanceLock instance_lock(user_presets_lock_path(read_only));
     if (instance_lock.locked())
         remove_stale_temp_files(dir);
     //BBS: change to json format

src/libslic3r/Preset.hpp

@@ -485,8 +485,10 @@ std::string get_preset_canonical_name(const std::string &preset_bare_name, const
 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.
-std::string user_presets_lock_path();
+// 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());

src/libslic3r/PresetBundle.cpp

@@ -1232,8 +1232,6 @@ PresetsConfigSubstitutions PresetBundle::load_user_presets(std::string user, For
     fs::path local_dir(folder / PRESET_LOCAL_DIR);
     if (fs::exists(local_dir)) {
         dir_user_presets_local = local_dir;
-        // Held across the metadata reads; a read-only load (the CLI) takes no lock.
-        InstanceLock instance_lock(read_only ? std::string() : user_presets_lock_path());
         for (auto& entry : fs::directory_iterator(local_dir)) {
             if (!fs::is_directory(entry.path())) continue;
 
@@ -1243,7 +1241,11 @@ 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;
+            {
+                // Per file, so the lock is never held when bundles.WriteLock() is taken below.
+                InstanceLock instance_lock(user_presets_lock_path(read_only));
+                if (!metadata.load_from_json(metadata_file.string())) continue;
+            }
             metadata.print_presets.clear();
             metadata.filament_presets.clear();
             metadata.printer_presets.clear();
@@ -1269,7 +1271,6 @@ PresetsConfigSubstitutions PresetBundle::load_user_presets(std::string user, For
     // Load bundle metadata from _subscribed directory
     fs::path subscribed_dir(folder / PRESET_SUBSCRIBED_DIR);
     if (fs::exists(subscribed_dir)) {
-        InstanceLock instance_lock(read_only ? std::string() : user_presets_lock_path());
         for (auto& entry : fs::directory_iterator(subscribed_dir)) {
             if (!fs::is_directory(entry.path())) continue;
 
@@ -1279,7 +1280,11 @@ 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;
+            {
+                // Per file, so the lock is never held when bundles.WriteLock() is taken below.
+                InstanceLock instance_lock(user_presets_lock_path(read_only));
+                if (!metadata.load_from_json(metadata_file.string())) continue;
+            }
             metadata.print_presets.clear();
             metadata.filament_presets.clear();
             metadata.printer_presets.clear();

src/libslic3r/PresetCacheFormat.cpp

@@ -410,11 +410,8 @@ bool write_cache_blob(const std::string& path, const std::string& blob)
         fhdr.version   = CACHE_VERSION;
         fhdr.data_size = static_cast<uint64_t>(blob.size());
         fhdr.crc32     = crc.checksum();
-        std::string payload;
-        payload.reserve(sizeof(fhdr) + blob.size());
-        payload.append(reinterpret_cast<const char*>(&fhdr), sizeof(fhdr));
-        payload += blob;
-        if (const std::error_code ec = write_file_atomically(path, payload, /*binary=*/true)) {
+        const std::string_view header(reinterpret_cast<const char*>(&fhdr), sizeof(fhdr));
+        if (const std::error_code ec = write_file_atomically(path, { header, std::string_view(blob) }, /*binary=*/true)) {
             BOOST_LOG_TRIVIAL(warning) << "VendorCacheFile: write failed (" << path << "): " << ec.message();
             return false;
         }

src/libslic3r/Utils.hpp

@@ -224,15 +224,19 @@ 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 `content` 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 file is removed on failure and an existing target
-// keeps its permissions. A target that is not a regular file (a symlink, device
-// or pipe) is written in place, since replacing it would change what it is.
-extern std::error_code write_file_atomically(const std::string &path, const std::string &content, bool binary = false);
-// Remove the `<name>.<pid>.tmp` files a crashed write_file_atomically() left in
-// `dir` at least ten minutes ago, restricted to names starting with `name_prefix`
-// when given. Call it only while holding the lock that guards writes into `dir`.
+// Write `chunks`, in order, to `path` through a temporary file beside it that is
+// then renamed over the target, so a concurrent reader sees the old or the new
+// file, never a partial one. The temporary is removed on failure and an existing
+// target keeps its permissions. A target that is not a regular file (a symlink,
+// device or pipe) is written in place, since replacing it would change what it
+// is, and so is a target whose replace the filesystem refuses. A successful
+// write also removes stale temporaries of the same target.
+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); }
+// Remove the `<name>.<pid>.<n>.tmp` files a crashed write_file_atomically() left in
+// `dir` at least ten minutes ago. With `name_prefix`, only names starting with it
+// go, and so do `<name_prefix>.<pid>` leftovers of the older AppConfig writer.
 // Returns how many were removed.
 extern size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::string &name_prefix = std::string());
 

src/libslic3r/utils.cpp

@@ -710,44 +710,53 @@ std::error_code rename_file(const std::string &from, const std::string &to)
 	if (boost::nowide::rename(from.c_str(), to.c_str()) == 0)
 		return {};
 	const int err = errno;
-	// Some mounts (sshfs without its rename workaround, for one) refuse to
-	// replace an existing target in one step; take the old two-step route there.
-	if ((err == EPERM || err == EEXIST || err == ENOTEMPTY) && boost::nowide::remove(to.c_str()) == 0 &&
+	// Some mounts (sshfs, gvfs, MTP and a few SMB setups) refuse to replace an
+	// existing target in one step, with whichever error they see fit; take the
+	// old two-step route whenever the target is still there to be replaced.
+	boost::system::error_code ec;
+	if (err != ENOENT && err != EXDEV && boost::filesystem::exists(to, ec) && 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, const std::string &content, bool binary)
+static std::error_code write_whole_file(const std::string &path, std::initializer_list<std::string_view> chunks, bool binary)
 {
 	errno = 0;
 	boost::nowide::ofstream out(path, std::ios::out | std::ios::trunc | (binary ? std::ios::binary : std::ios::openmode{}));
-	out << content;
+	for (const std::string_view chunk : chunks)
+		out.write(chunk.data(), static_cast<std::streamsize>(chunk.size()));
 	out.close();
 	if (! out.fail())
 		return {};
 	return std::make_error_code(errno != 0 ? static_cast<std::errc>(errno) : std::errc::io_error);
 }
 
-std::error_code write_file_atomically(const std::string &path, const std::string &content, bool 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_regular_file(target))
-		return write_whole_file(path, content, binary);
+		return write_whole_file(path, chunks, binary);
 
-	const std::string tmp_path = path + "." + std::to_string(get_current_pid()) + ".tmp";
-	if (std::error_code ec = write_whole_file(tmp_path, content, 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 (std::error_code ec = write_whole_file(tmp_path, chunks, binary)) {
 		boost::nowide::remove(tmp_path.c_str());
 		// A directory that allows writing the file but not creating one beside it.
 		if (target_exists && ec == std::errc::permission_denied)
-			return write_whole_file(path, content, binary);
+			return write_whole_file(path, chunks, binary);
 		return ec;
 	}
+#ifndef _WIN32
+	// On Windows a read-only bit on the temporary would stop the rename itself.
 	if (target_exists)
 		boost::filesystem::permissions(tmp_path, target.permissions(), bec);
+#endif
 	std::error_code ec = rename_file(tmp_path, path);
 	if (ec) {
 		boost::nowide::remove(tmp_path.c_str());
@@ -757,25 +766,35 @@ std::error_code write_file_atomically(const std::string &path, const std::string
 		// 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";
-		ec = write_whole_file(path, content, binary);
+		return write_whole_file(path, chunks, binary);
 	}
-	return ec;
+#ifdef _WIN32
+	if (target_exists)
+		boost::filesystem::permissions(path, target.permissions(), bec);
+#endif
+	const boost::filesystem::path target_path(path);
+	remove_stale_temp_files(target_path.parent_path(), target_path.filename().string());
+	return {};
 }
 
 size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::string &name_prefix)
 {
-	// <anything>.<digits>.tmp
-	auto is_temp_name = [&name_prefix](const std::string &name) {
+	auto all_digits = [](std::string::const_iterator begin, std::string::const_iterator end) {
+		return begin != end && std::all_of(begin, end, [](char c) { return c >= '0' && c <= '9'; });
+	};
+	// <name_prefix>...<digits>.tmp, or the older <name_prefix>.<digits> when a prefix is given.
+	auto is_temp_name = [&](const std::string &name) {
 		if (name.compare(0, name_prefix.size(), name_prefix) != 0)
 			return false;
+		if (! name_prefix.empty() && name.size() > name_prefix.size() + 1 && name[name_prefix.size()] == '.' &&
+		    all_digits(name.begin() + name_prefix.size() + 1, name.end()))
+			return true;
 		static const std::string suffix = ".tmp";
 		if (name.size() <= suffix.size() || name.compare(name.size() - suffix.size(), suffix.size(), suffix) != 0)
 			return false;
 		const size_t digits_end = name.size() - suffix.size();
 		const size_t dot        = name.rfind('.', digits_end - 1);
-		if (dot == std::string::npos || dot == 0 || dot + 1 == digits_end)
-			return false;
-		return std::all_of(name.begin() + dot + 1, name.begin() + digits_end, [](char c) { return c >= '0' && c <= '9'; });
+		return dot != std::string::npos && dot != 0 && all_digits(name.begin() + dot + 1, name.begin() + digits_end);
 	};
 	// An instance that gave up waiting for the lock writes unlocked by design,
 	// so a temporary this young may still be in flight; a crash leftover is old.

tests/libslic3r/test_utils.cpp

@@ -11,6 +11,7 @@
 #include <ctime>
 #include <fstream>
 #include <string>
+#include <thread>
 #include <system_error>
 
 #ifndef _WIN32
@@ -122,9 +123,34 @@ TEST_CASE("write_file_atomically writes through a symlink and keeps the target's
 }
 #endif
 
+TEST_CASE("write_file_atomically survives two threads writing one target", "[utils]") {
+    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);
+    size_t entries = 0;
+    for (auto &entry : boost::filesystem::directory_iterator(dir.path())) {
+        (void) entry;
+        ++entries;
+    }
+    REQUIRE(entries == 1);
+}
+
 TEST_CASE("remove_stale_temp_files removes only old <name>.<pid>.tmp files", "[utils]") {
     ScopedTemporaryDir dir;
-    for (const char *name : { "a.json.123.tmp", "b.info.4.tmp", "c.json", "d.tmp", "e.json.x.tmp", "f.json..tmp" }) {
+    for (const char *name : { "a.json.123.tmp", "b.info.4.tmp", "c.json", "d.tmp", "e.json.x.tmp", "f.json..tmp", "a.json.99", "a.json.12.3.tmp" }) {
         REQUIRE_FALSE(write_file_atomically((dir.path() / name).string(), "x"));
         // An hour old: long past the age below which a temporary may still be in flight.
         boost::filesystem::last_write_time(dir.path() / name, std::time(nullptr) - 3600);
@@ -132,19 +158,22 @@ TEST_CASE("remove_stale_temp_files removes only old <name>.<pid>.tmp files", "[u
     // Just written: possibly another instance's in-flight save, so it stays.
     REQUIRE_FALSE(write_file_atomically((dir.path() / "g.json.7.tmp").string(), "x"));
 
-    SECTION("with a name prefix only matching names go") {
-        REQUIRE(remove_stale_temp_files(dir.path(), "a.json") == 1);
+    SECTION("with a name prefix only matching names go, including the older <name>.<pid> form") {
+        REQUIRE(remove_stale_temp_files(dir.path(), "a.json") == 3);
         REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "a.json.123.tmp"));
+        REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "a.json.99"));
+        REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "a.json.12.3.tmp"));
         REQUIRE(boost::filesystem::exists(dir.path() / "b.info.4.tmp"));
     }
     SECTION("without a prefix every stale temporary goes and nothing else") {
-        REQUIRE(remove_stale_temp_files(dir.path()) == 2);
+        REQUIRE(remove_stale_temp_files(dir.path()) == 3);
         size_t entries = 0;
         for (auto &entry : boost::filesystem::directory_iterator(dir.path())) {
             (void) entry;
             ++entries;
         }
-        REQUIRE(entries == 5);
+        REQUIRE(entries == 6);
+        REQUIRE(boost::filesystem::exists(dir.path() / "a.json.99"));
         REQUIRE(boost::filesystem::exists(dir.path() / "g.json.7.tmp"));
     }
 }

tests/libslic3r/test_vendor_cache.cpp

@@ -1,5 +1,9 @@
 #include <catch2/catch_all.hpp>
 
+#ifndef _WIN32
+#include <unistd.h>
+#endif
+
 #include <boost/filesystem.hpp>
 #include <boost/crc.hpp>
 #include <cereal/archives/binary.hpp>
@@ -1356,23 +1360,31 @@ 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)));
 }
 
+#ifndef _WIN32
+// Permissions are what makes the write fail here, which Windows does not
+// express through chmod and root ignores; the helper's own tests cover the rest.
 TEST_CASE("a failed write leaves the previous cache in place", "[VendorCache]")
 {
+    if (::geteuid() == 0)
+        SKIP("permissions do not apply to root");
     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);
+    // No temp file can be created beside the cache and the cache itself cannot
+    // be opened for writing: the write cannot complete, and must not have
+    // destroyed what was already there to find that out.
+    fs::permissions(cache, fs::owner_read | fs::group_read | fs::others_read);
+    fs::permissions(tmp.path, fs::owner_read | fs::owner_exe | fs::group_read | fs::group_exe | fs::others_read | fs::others_exe);
 
     REQUIRE_FALSE(save_one_vendor(cache, one_vendor("Durable"), "Durable", "2.0.0"));
-    CHECK(slurp(cache) == before);
 
-    fs::remove_all(blocker);
+    fs::permissions(tmp.path, fs::owner_all | fs::group_read | fs::group_exe | fs::others_read | fs::others_exe);
+    fs::permissions(cache, fs::owner_read | fs::owner_write | fs::group_read | fs::others_read);
+    CHECK(slurp(cache) == before);
 }
+#endif
 
 TEST_CASE("a cache written by another build's option ordering still loads", "[VendorCache]")
 {