From 1b30ae7424a9ce7555b51094950b8da52b6e9aee Mon Sep 17 00:00:00 2001 From: Hanif Koh Date: Thu, 24 Sep 2026 22:44:09 +0800 Subject: [PATCH] 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 | 17 ++++----- src/libslic3r/AppConfig.hpp | 5 +-- src/libslic3r/InstanceLock.cpp | 3 +- src/libslic3r/InstanceLock.hpp | 6 ++-- src/libslic3r/Utils.hpp | 11 ++++-- src/libslic3r/utils.cpp | 49 ++++++++++++++------------ tests/libslic3r/test_instance_lock.cpp | 5 +-- tests/libslic3r/test_utils.cpp | 10 +++--- 8 files changed, 61 insertions(+), 45 deletions(-) diff --git a/src/libslic3r/AppConfig.cpp b/src/libslic3r/AppConfig.cpp index f1a711f40c..2012a72aee 100644 --- a/src/libslic3r/AppConfig.cpp +++ b/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 diff --git a/src/libslic3r/AppConfig.hpp b/src/libslic3r/AppConfig.hpp index abfad5fc7a..2bb274b02a 100644 --- a/src/libslic3r/AppConfig.hpp +++ b/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 diff --git a/src/libslic3r/InstanceLock.cpp b/src/libslic3r/InstanceLock.cpp index da787f8caf..22480349f0 100644 --- a/src/libslic3r/InstanceLock.cpp +++ b/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; } } diff --git a/src/libslic3r/InstanceLock.hpp b/src/libslic3r/InstanceLock.hpp index fa20153e8b..4de8d5915a 100644 --- a/src/libslic3r/InstanceLock.hpp +++ b/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}; diff --git a/src/libslic3r/Utils.hpp b/src/libslic3r/Utils.hpp index 18414fc1bb..4689f49cc2 100644 --- a/src/libslic3r/Utils.hpp +++ b/src/libslic3r/Utils.hpp @@ -8,6 +8,8 @@ #include #include #include +#include +#include #include #include @@ -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 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 `...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 `...tmp` files a crashed write_file_atomically() and +// the `..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. diff --git a/src/libslic3r/utils.cpp b/src/libslic3r/utils.cpp index 2b1135dee0..ed4f61a92e 100644 --- a/src/libslic3r/utils.cpp +++ b/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) { - // .....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) { + // .....tmp, exactly the shape write_file_atomically() + // makes, or ....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. diff --git a/tests/libslic3r/test_instance_lock.cpp b/tests/libslic3r/test_instance_lock.cpp index b5dd4818a1..36ca30670a 100644 --- a/tests/libslic3r/test_instance_lock.cpp +++ b/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()); diff --git a/tests/libslic3r/test_utils.cpp b/tests/libslic3r/test_utils.cpp index e75c34d28b..77b2bffe2b 100644 --- a/tests/libslic3r/test_utils.cpp +++ b/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 ...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 ...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")); }