Clear the Read-Only Attribute Before Replacing a Config and Back Off After a Failed Save

Nothing replaces a read-only file on Windows, neither a rename over it
nor an in-place write, so the opt-in that keeps a read-only config
saveable clears the attribute first; without that the test for it could
not pass there. A failed config write still leaves the flag dirty, as
before this change, but the next attempt waits ten seconds, so the idle
handler does not repeat a hopeless write on every event.

A file moved aside by a refused rename can be the only copy left if the
process dies in between, so the sweep puts such a file back when its
original is missing rather than removing it, and the directory-only
errors that could only come from a directory target no longer trigger
the move-aside at all. The sweep after a write runs only under the data
dir and at most once an hour per directory, so a batch of saves does not
read the directory once per file and an export into a user's folder
never reads that folder. A lock holder that never lets go doubles the
cool-down for each timeout in a row, up to five minutes, instead of
costing a stall every ten seconds for good.

src/libslic3r/AppConfig.cpp

@@ -987,6 +987,8 @@ void AppConfig::save()
         BOOST_LOG_TRIVIAL(fatal) << "Calling AppConfig::save() from a worker thread!";
         throw CriticalException("Calling AppConfig::save() from a worker thread!");
     }
+    if (std::chrono::steady_clock::now() < m_retry_save_at)
+        return;
 
     // The config is first written to a file with a PID suffix and then moved
     // to avoid race conditions with multiple instances of Slic3r
@@ -1269,6 +1271,8 @@ void AppConfig::save()
 {
     if (! is_main_thread_active())
         throw CriticalException("Calling AppConfig::save() from a worker thread!");
+    if (std::chrono::steady_clock::now() < m_retry_save_at)
+        return;
 
     // The config is first written to a file with a PID suffix and then moved
     // to avoid race conditions with multiple instances of Slic3r
@@ -1329,9 +1333,11 @@ bool AppConfig::write_config_file(const std::string &path, std::string body, con
 #endif
     // The config was always replaced, never written in place, so a read-only one is replaced still.
     if (const std::error_code ec = write_file_atomically(path, body, false, /*replace_read_only=*/true)) {
-        BOOST_LOG_TRIVIAL(error) << "Failed to write the configuration " << path << ": " << ec.message();
+        BOOST_LOG_TRIVIAL(error) << "Failed to write the configuration " << path << ": " << ec.message() << "; trying again in 10 s";
+        m_retry_save_at = std::chrono::steady_clock::now() + std::chrono::seconds(10);
         return false;
     }
+    m_retry_save_at = {};
     return true;
 }
 

src/libslic3r/AppConfig.hpp

@@ -2,6 +2,7 @@
 #define slic3r_AppConfig_hpp_
 
 #include <set>
+#include <chrono>
 #include <map>
 #include <string>
 #include "nlohmann/json.hpp"
@@ -457,6 +458,9 @@ private:
 	bool												write_config_file(const std::string &path, std::string body, const std::string &checksum_source);
 
 	bool														m_dirty;
+	// After a failed write, save() does nothing until this point, so the idle
+	// handler does not repeat the attempt 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/InstanceLock.cpp

@@ -1,5 +1,6 @@
 #include "InstanceLock.hpp"
 
+#include <algorithm>
 #include <map>
 #include <memory>
 #include <thread>
@@ -33,8 +34,10 @@ struct InstanceLock::Slot
     std::chrono::steady_clock::time_point identity_checked_at{};
     int  depth{0};
     bool file_locked{false};
-    // After a timed-out wait, guards skip waiting until this point.
+    // After a timed-out wait, guards skip waiting until this point; each wait
+    // that times out in a row doubles the next cool-down, up to a few minutes.
     std::chrono::steady_clock::time_point skip_waiting_until{};
+    int consecutive_timeouts{0};
     // After a failed open or a failing lock call, guards skip the file lock
     // entirely until this point.
     std::chrono::steady_clock::time_point retry_at{};
@@ -106,7 +109,8 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli
         for (;;) {
             try {
                 if (m_slot->file_lock->try_lock()) {
-                    m_slot->file_locked = true;
+                    m_slot->file_locked          = true;
+                    m_slot->consecutive_timeouts = 0;
                     break;
                 }
             } catch (const std::exception &e) {
@@ -118,10 +122,12 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli
             if (! wait)
                 break;
             if (std::chrono::steady_clock::now() >= deadline) {
-                m_slot->skip_waiting_until = deadline + cooldown;
+                const auto backoff = std::min(cooldown * (1 << std::min(m_slot->consecutive_timeouts, 5)), std::chrono::milliseconds(std::chrono::minutes(5)));
+                ++ m_slot->consecutive_timeouts;
+                m_slot->skip_waiting_until = deadline + backoff;
                 BOOST_LOG_TRIVIAL(warning) << "Another instance has held " << lock_file_path << " for over "
                                            << timeout.count() << " ms; proceeding without the lock for the next "
-                                           << cooldown.count() << " ms";
+                                           << backoff.count() << " ms";
                 break;
             }
             std::this_thread::sleep_for(std::chrono::milliseconds(5));

src/libslic3r/InstanceLock.hpp

@@ -19,8 +19,10 @@ namespace Slic3r {
 // instance still holds it after `timeout` (a second by default), the guard keeps only the in-process
 // mutex and locked() reports false. Writes then proceed unprotected rather than
 // letting one hung instance block every other one from saving. After such a
-// timeout the same lock file is not waited on again for `cooldown`, so a batch
-// of saves pays the wait once rather than once per file; a lock file that could
+// timeout the same lock file is not waited on again for `cooldown`, doubling
+// for every further timeout in a row up to a few minutes, so a batch of saves
+// pays the wait once rather than once per file and a holder that never lets go
+// does not cost a stall every cool-down for good; a lock file that could
 // not be opened, or a lock call that fails outright (a share without a lock
 // service), is likewise retried only after `cooldown`.
 //

src/libslic3r/Utils.hpp

@@ -235,14 +235,15 @@ extern std::error_code rename_file(const std::string &from, const std::string &t
 // target that is not a regular file (a symlink, device or pipe) is written in
 // place, since replacing it would change what it is, and so is a target whose
 // replace the filesystem refuses or beside which no temporary can be created.
-// A successful write also removes stale leftovers of earlier writes to the
-// same target.
+// A successful write into a directory under data_dir() also removes stale
+// leftovers there, at most once an hour per directory.
 extern std::error_code write_file_atomically(const std::string &path, std::initializer_list<std::string_view> chunks, bool binary = false, bool replace_read_only = false);
 inline std::error_code write_file_atomically(const std::string &path, const std::string &content, bool binary = false, bool replace_read_only = false)
 	{ return write_file_atomically(path, { std::string_view(content) }, binary, replace_read_only); }
 // Remove the `<name>.<pid>.<n>.tmp` files a crashed write_file_atomically() and
 // the `<name>.<pid>.old` files a crashed rename_file() left in `dir` at least an
-// hour ago, only names starting with `name_prefix` when it is given. Returns how
+// hour ago, only names starting with `name_prefix` when it is given; an `.old`
+// whose `<name>` is missing is the last copy and is put back instead. Returns how
 // many were removed.
 extern size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::string &name_prefix = std::string());
 // Names the file object behind `path` (device and inode, or volume and file index),

src/libslic3r/utils.cpp

@@ -11,6 +11,8 @@
 #include <sstream>
 #include <cerrno>
 #include <chrono>
+#include <map>
+#include <mutex>
 #include <thread>
 #include <cstring>
 #include <iomanip>
@@ -718,7 +720,7 @@ std::error_code rename_file(const std::string &from, const std::string &to)
 	// existing target in one step and report it as one of these; move the
 	// target aside and try again there, and put it back if that fails too, so
 	// a refusal that was really about the source never costs the target.
-	const bool replace_refused = err == EPERM || err == EACCES || err == EEXIST || err == ENOTEMPTY || err == EBUSY || err == ENOTSUP || err == EOPNOTSUPP;
+	const bool replace_refused = err == EPERM || err == EACCES || err == EBUSY || err == ENOTSUP || err == EOPNOTSUPP;
 	if (replace_refused) {
 		// Named so remove_stale_temp_files() can clear it after a crash in between.
 		const std::string aside = to + "." + std::to_string(get_current_pid()) + ".old";
@@ -765,8 +767,14 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_
 		return write_whole_file(path, chunks, binary);
 	// Replacing needs only a writable directory, so a file this process may
 	// not write has to be refused here, as the in-place write used to be.
-	if (target_exists && ! replace_read_only && ! is_writable(path))
-		return std::make_error_code(std::errc::permission_denied);
+	if (target_exists && ! is_writable(path)) {
+		if (! replace_read_only)
+			return std::make_error_code(std::errc::permission_denied);
+#ifdef _WIN32
+		// Nothing replaces a read-only file on Windows, so the attribute goes first.
+		boost::filesystem::permissions(path, boost::filesystem::add_perms | boost::filesystem::owner_write, bec);
+#endif
+	}
 
 	// Unique per process and per call, so two threads writing one target
 	// without a lock never share a temporary.
@@ -799,10 +807,26 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_
 		BOOST_LOG_TRIVIAL(warning) << "Cannot replace " << path << " (" << ec.message() << "); writing in place";
 		return write_whole_file(path, chunks, binary);
 	}
-	// Crash leftovers of earlier writes to this same file, wherever it lives;
-	// the name shapes are specific enough that nothing of the user's matches.
-	const boost::filesystem::path target_path(path);
-	remove_stale_temp_files(target_path.parent_path(), target_path.filename().string());
+	// Crash leftovers of earlier writes into this directory, for the files the
+	// application owns; once an hour per directory, so a batch of saves does
+	// not read the directory once per file.
+	const boost::filesystem::path dir = boost::filesystem::path(path).parent_path();
+	if (! data_dir().empty() && boost::algorithm::starts_with(dir.generic_string(), boost::filesystem::path(data_dir()).generic_string())) {
+		static std::mutex swept_mutex;
+		static std::map<std::string, std::chrono::steady_clock::time_point> swept_at;
+		const auto now = std::chrono::steady_clock::now();
+		bool sweep = false;
+		{
+			std::lock_guard<std::mutex> guard(swept_mutex);
+			auto &last = swept_at[dir.string()];
+			if (last == std::chrono::steady_clock::time_point{} || now - last >= std::chrono::hours(1)) {
+				last  = now;
+				sweep = true;
+			}
+		}
+		if (sweep)
+			remove_stale_temp_files(dir);
+	}
 	return {};
 }
 
@@ -869,6 +893,17 @@ size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::st
 		const std::time_t written = boost::filesystem::last_write_time(it->path(), entry_ec);
 		if (entry_ec || now - written < stale_age)
 			continue;
+		const std::string name = it->path().filename().string();
+		if (name.size() > 4 && name.compare(name.size() - 4, 4, ".old") == 0) {
+			// A target rename_file() moved aside and never put back or removed:
+			// the last copy of the file if the target is missing, so it goes back.
+			const boost::filesystem::path original = it->path().parent_path() / name.substr(0, name.rfind('.', name.size() - 5));
+			if (! boost::filesystem::exists(original, entry_ec)) {
+				boost::filesystem::rename(it->path(), original, entry_ec);
+				BOOST_LOG_TRIVIAL(warning) << "Restored " << original << " from " << it->path() << (entry_ec ? ": failed" : "");
+				continue;
+			}
+		}
 		if (boost::filesystem::remove(it->path(), entry_ec)) {
 			BOOST_LOG_TRIVIAL(info) << "Removed stale temporary file " << it->path();
 			++ removed;

tests/libslic3r/test_utils.cpp

@@ -187,9 +187,23 @@ TEST_CASE("write_file_atomically survives two threads writing one target", "[uti
     REQUIRE(temporaries == 0);
 }
 
+TEST_CASE("remove_stale_temp_files puts back a moved-aside file whose original is missing", "[utils]") {
+    ScopedTemporaryDir dir;
+    REQUIRE_FALSE(write_file_atomically((dir.path() / "lost.json.4242.old").string(), "last copy"));
+    boost::filesystem::last_write_time(dir.path() / "lost.json.4242.old", std::time(nullptr) - 7200);
+
+    REQUIRE(remove_stale_temp_files(dir.path()) == 0);
+    REQUIRE(boost::filesystem::exists(dir.path() / "lost.json"));
+    REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "lost.json.4242.old"));
+    std::string content;
+    load_string_file(dir.path() / "lost.json", content);
+    REQUIRE(content == "last copy");
+}
+
 TEST_CASE("remove_stale_temp_files removes only old <name>.<pid>.<n>.tmp files", "[utils]") {
     ScopedTemporaryDir dir;
-    for (const char *name : { "a.json.123.7.tmp", "b.info.4.0.tmp", "c.json", "d.tmp", "e.json.x.1.tmp", "f.json..tmp", "a.json.99", "a.json.12.tmp", "a.json.123.old", "h.json.old" }) {
+    // a.json exists, so its .old is a leftover; a moved-aside file without its original is a different case.
+    for (const char *name : { "a.json", "a.json.123.7.tmp", "b.info.4.0.tmp", "c.json", "d.tmp", "e.json.x.1.tmp", "f.json..tmp", "a.json.99", "a.json.12.tmp", "a.json.123.old", "h.json.old" }) {
         REQUIRE_FALSE(write_file_atomically((dir.path() / name).string(), "x"));
         // An hour old: long past the age below which a temporary may still be in flight.
         boost::filesystem::last_write_time(dir.path() / name, std::time(nullptr) - 3600);
@@ -212,7 +226,8 @@ TEST_CASE("remove_stale_temp_files removes only old <name>.<pid>.<n>.tmp files",
             (void) entry;
             ++entries;
         }
-        REQUIRE(entries == 8);
+        REQUIRE(entries == 9);
+        REQUIRE(boost::filesystem::exists(dir.path() / "a.json"));
         REQUIRE(boost::filesystem::exists(dir.path() / "h.json.old"));
         REQUIRE(boost::filesystem::exists(dir.path() / "a.json.99"));
         REQUIRE(boost::filesystem::exists(dir.path() / "g.json.7.2.tmp"));