Keep the Config Dirty After a Failed Write and Sweep a Write's Own Leftovers

Before this change a config whose temporary could not be written stayed
dirty, so the idle handler tried again; the last round cleared the flag
regardless, which lost a pending change to a transient failure. The
writer reports whether the config itself was written and the flag clears
only then, as it always did.

The Windows rename retry duplicated what WindowsSupport::rename already
does, retrying and moving an open destination aside, and multiplied its
error logging; it is gone. The file a refused rename moves aside is
named so the sweep can clear it after a crash in between, and every
successful atomic write sweeps leftovers of earlier writes to the same
target, so the caches and the smaller state files are covered too; the
name shapes are strict enough that nothing of the user's matches.

The lock wait defaults to a second, long against a critical section of
milliseconds and short against the GUI thread; a lock file that cannot
be opened says to check its owner and permissions; the test timing
bounds tolerate a loaded runner; and the helper's header includes what
its declarations use.

src/libslic3r/AppConfig.cpp

@@ -1121,10 +1121,8 @@ void AppConfig::save()
         j["local_machines"][local_machine.first] = m_json;
     }
     const std::string config_str = j.dump(1, '\t');
-    write_config_file(path, config_str + "\n", config_str);
-    // Cleared even after a failed write, as before: the failure is logged, and
-    // the next change tries again rather than every idle tick.
-    m_dirty = false;
+    if (write_config_file(path, config_str + "\n", config_str))
+        m_dirty = false;
 }
 
 #else
@@ -1311,12 +1309,12 @@ void AppConfig::save()
     config_ss << std::endl;
 
     const std::string config_str = config_ss.str();
-    write_config_file(path, config_str, config_str);
-    m_dirty = false;
+    if (write_config_file(path, config_str, config_str))
+        m_dirty = false;
 }
 #endif
 
-void AppConfig::write_config_file(const std::string &path, std::string body, const std::string &checksum_source)
+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());
@@ -1330,8 +1328,11 @@ void AppConfig::write_config_file(const std::string &path, std::string body, con
         BOOST_LOG_TRIVIAL(error) << "Failed to write the backup configuration " << backup_path << ": " << ec.message();
 #endif
     // The config was always replaced, never written in place, so a read-only one is replaced still.
-    if (const std::error_code ec = write_file_atomically(path, body, false, /*replace_read_only=*/true))
+    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();
+        return false;
+    }
+    return true;
 }
 
 bool AppConfig::get_variant(const std::string &vendor, const std::string &model, const std::string &variant) const

src/libslic3r/AppConfig.hpp

@@ -451,9 +451,10 @@ private:
 
 	// Preset for each machine
 	MachineSettingMap											m_printer_settings;
-	// Writes the assembled config text, and on Windows its checksum and a backup copy. `checksum_source`
+	// 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.
-	void												write_config_file(const std::string &path, std::string body, const std::string &checksum_source);
+	bool												write_config_file(const std::string &path, std::string body, const std::string &checksum_source);
 
 	bool														m_dirty;
 	// Original version found in the ini file before it was overwritten

src/libslic3r/InstanceLock.cpp

@@ -77,7 +77,8 @@ bool InstanceLock::open_lock_file(Slot &slot, const std::string &lock_file_path)
     } catch (const std::exception &e) {
         slot.retry_at = std::chrono::steady_clock::now() + cooldown;
         BOOST_LOG_TRIVIAL(warning) << "Cannot open lock file " << lock_file_path << ": " << e.what()
-                                   << "; other instances are not excluded from writing for the next " << cooldown.count() << " ms";
+                                   << " (check its owner and permissions); other instances are not excluded from writing for the next "
+                                   << cooldown.count() << " ms";
         return false;
     }
 }

src/libslic3r/InstanceLock.hpp

@@ -16,7 +16,7 @@ namespace Slic3r {
 // exits, so a crashed instance never leaves a stale lock behind.
 //
 // The lock is best effort: when the lock file cannot be created, or another
-// instance still holds it after `timeout`, the guard keeps only the in-process
+// 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
@@ -32,7 +32,9 @@ namespace Slic3r {
 class InstanceLock
 {
 public:
-    static constexpr std::chrono::milliseconds default_timeout{2000};
+    // Long against a critical section of milliseconds, short against the GUI
+    // thread, which is where most guards are taken.
+    static constexpr std::chrono::milliseconds default_timeout{1000};
     // How long a lock file is left alone after a timed-out wait, a failed open
     // or a failing lock call. Mutable so tests can shorten it.
     static inline std::chrono::milliseconds cooldown{10000};

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>
@@ -233,12 +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.
 extern std::error_code write_file_atomically(const std::string &path, std::initializer_list<std::string_view> chunks, bool binary = false, bool replace_read_only = false);
 inline std::error_code write_file_atomically(const std::string &path, const std::string &content, bool binary = false, bool replace_read_only = false)
 	{ return write_file_atomically(path, { std::string_view(content) }, binary, replace_read_only); }
-// Remove the `<name>.<pid>.<n>.tmp` files a crashed write_file_atomically() left in
-// `dir` at least an hour ago, only names starting with `name_prefix` when it is
-// given. Meant for directories the application owns. Returns how many were removed.
+// 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
+// many were removed.
 extern size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::string &name_prefix = std::string());
 // Names the file object behind `path` (device and inode, or volume and file index),
 // for noticing that a path was unlinked and recreated. Empty when it cannot be read.

src/libslic3r/utils.cpp

@@ -706,16 +706,8 @@ namespace WindowsSupport
 std::error_code rename_file(const std::string &from, const std::string &to)
 {
 #ifdef _WIN32
-	std::error_code ec = WindowsSupport::rename(from, to);
-	// Windows refuses to replace a file another process holds open without
-	// FILE_SHARE_DELETE, which is how the C runtime opens files for reading; a
-	// reader is done in milliseconds, so wait it out. A target that is not
-	// writable at all is refused for good and not worth waiting on.
-	for (int attempt = 0; attempt < 20 && ec == std::errc::permission_denied && ::_waccess(boost::nowide::widen(to).c_str(), 2) == 0; ++ attempt) {
-		std::this_thread::sleep_for(std::chrono::milliseconds(25));
-		ec = WindowsSupport::rename(from, to);
-	}
-	return ec;
+	// Retries and moves an open destination aside itself.
+	return WindowsSupport::rename(from, to);
 #else
 	// rename(2) replaces an existing target atomically; removing it first would
 	// leave a window in which the file does not exist at all.
@@ -728,6 +720,7 @@ std::error_code rename_file(const std::string &from, const std::string &to)
 	// 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;
 	if (replace_refused) {
+		// Named so remove_stale_temp_files() can clear it after a crash in between.
 		const std::string aside = to + "." + std::to_string(get_current_pid()) + ".old";
 		if (boost::nowide::rename(to.c_str(), aside.c_str()) == 0) {
 			if (boost::nowide::rename(from.c_str(), to.c_str()) == 0) {
@@ -799,13 +792,17 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_
 #endif
 	if (const std::error_code ec = rename_file(tmp_path, path)) {
 		boost::nowide::remove(tmp_path.c_str());
-		// A reader on Windows that outlasted rename_file()'s wait, 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.
+		// 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_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());
 	return {};
 }
 
@@ -832,15 +829,11 @@ std::string file_identity(const std::string &path)
 
 size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::string &name_prefix)
 {
-	// <name_prefix>...<digits>.<digits>.tmp, exactly the shape write_file_atomically() makes.
-	auto is_temp_name = [&name_prefix](const std::string &name) {
-		if (name.compare(0, name_prefix.size(), name_prefix) != 0)
-			return false;
-		static const std::string suffix = ".tmp";
-		if (name.size() <= suffix.size() || name.compare(name.size() - suffix.size(), suffix.size(), suffix) != 0)
-			return false;
-		size_t end = name.size() - suffix.size();
-		for (int segment = 0; segment < 2; ++ segment) {
+	// <name_prefix>...<pid>.<n>.tmp, exactly the shape write_file_atomically()
+	// makes, or <name_prefix>...<pid>.old, the shape rename_file() moves a
+	// target aside under.
+	auto digit_segments_before = [](const std::string &name, size_t end, int count) {
+		for (int segment = 0; segment < count; ++ segment) {
 			const size_t dot = name.rfind('.', end - 1);
 			if (dot == std::string::npos || dot == 0 || dot + 1 == end ||
 			    ! std::all_of(name.begin() + dot + 1, name.begin() + end, [](char c) { return c >= '0' && c <= '9'; }))
@@ -849,6 +842,16 @@ size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::st
 		}
 		return true;
 	};
+	auto is_temp_name = [&](const std::string &name) {
+		if (name.compare(0, name_prefix.size(), name_prefix) != 0)
+			return false;
+		static const std::string tmp_suffix = ".tmp", old_suffix = ".old";
+		if (name.size() > tmp_suffix.size() && name.compare(name.size() - tmp_suffix.size(), tmp_suffix.size(), tmp_suffix) == 0)
+			return digit_segments_before(name, name.size() - tmp_suffix.size(), 2);
+		if (name.size() > old_suffix.size() && name.compare(name.size() - old_suffix.size(), old_suffix.size(), old_suffix) == 0)
+			return digit_segments_before(name, name.size() - old_suffix.size(), 1);
+		return false;
+	};
 	// An instance that gave up waiting for the lock writes unlocked by design,
 	// so a temporary this young may still be in flight, and hosts sharing a
 	// data dir may disagree on the time by minutes; a crash leftover is old.

tests/libslic3r/test_instance_lock.cpp

@@ -32,7 +32,8 @@ TEST_CASE("InstanceLock creates its lock file and holds it for the guard's scope
     const auto   started = std::chrono::steady_clock::now();
     InstanceLock again(path, 5000ms);
     REQUIRE(again.locked());
-    REQUIRE(std::chrono::steady_clock::now() - started < 1000ms);
+    // 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]")
@@ -193,7 +194,7 @@ TEST_CASE("InstanceLock yields to another process and reports it", "[InstanceLoc
 
     REQUIRE_FALSE(locked_while_child_holds);
     REQUIRE_FALSE(locked_during_cooldown);
-    REQUIRE(cooldown_wait < 1000ms);
+    REQUIRE(cooldown_wait < 4000ms);
     // A guard inside the cool-down still takes the lock when it is free.
     InstanceLock lock(path);
     REQUIRE(lock.locked());

tests/libslic3r/test_utils.cpp

@@ -189,7 +189,7 @@ TEST_CASE("write_file_atomically survives two threads writing one target", "[uti
 
 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" }) {
+    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" }) {
         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);
@@ -198,20 +198,22 @@ TEST_CASE("remove_stale_temp_files removes only old <name>.<pid>.<n>.tmp files",
     REQUIRE_FALSE(write_file_atomically((dir.path() / "g.json.7.2.tmp").string(), "x"));
 
     SECTION("with a name prefix only matching names go; a numbered backup or a one-segment name is not a temporary") {
-        REQUIRE(remove_stale_temp_files(dir.path(), "a.json") == 1);
+        REQUIRE(remove_stale_temp_files(dir.path(), "a.json") == 2);
         REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "a.json.123.7.tmp"));
+        REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "a.json.123.old"));
         REQUIRE(boost::filesystem::exists(dir.path() / "a.json.99"));
         REQUIRE(boost::filesystem::exists(dir.path() / "a.json.12.tmp"));
         REQUIRE(boost::filesystem::exists(dir.path() / "b.info.4.0.tmp"));
     }
     SECTION("without a prefix every stale temporary goes and nothing else") {
-        REQUIRE(remove_stale_temp_files(dir.path()) == 2);
+        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 == 7);
+        REQUIRE(entries == 8);
+        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"));
     }