Let an Explicit Config Save Bypass the Back-Off and Lock the Scan's Failure Cleanup

The ten-second back-off after a failed config write sat inside save()
itself, so the save on exit could return without writing and lose the
session's changes; it gates only the idle-time save now, through
save_due(), and an explicit save always tries.

The preset scan's failure handlers removed a broken preset and its .info
after the per-file guard had ended, so another instance's fresh copy of
that file could be deleted from under it; they take the guard too. A
lock that is acquired ends the cool-down a timeout started, rather than
letting every guard for the rest of it proceed unlocked whenever the
lock is momentarily held. The lock file's identity is checked on every
acquisition, one stat, since a handle to a replaced file locks nothing
anyone else can see. Write access on Windows is probed with an open for
writing, which sees ACLs where _waccess sees only the read-only
attribute, and a temporary name too long for the filesystem falls back
to an in-place write for a new file as well as an existing one. The lock
tests restore the knobs they change however they end.

src/libslic3r/AppConfig.cpp

@@ -987,8 +987,6 @@ 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
@@ -1271,8 +1269,6 @@ 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

src/libslic3r/AppConfig.hpp

@@ -131,6 +131,9 @@ public:
 
 	// 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; }
@@ -458,8 +461,7 @@ 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.
+	// After a failed write, save_due() is false until this point.
 	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;

src/libslic3r/InstanceLock.cpp

@@ -28,8 +28,6 @@ struct InstanceLock::Slot
     std::unique_ptr<boost::interprocess::file_lock> file_lock;
     // What the lock file was when it was opened; a different answer means the
     // path was unlinked or replaced and the open handle locks a dead file.
-    // Checked at most every few seconds, since a preset scan takes one guard
-    // per file.
     std::string identity;
     std::chrono::steady_clock::time_point identity_checked_at{};
     int  depth{0};
@@ -111,6 +109,7 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli
                 if (m_slot->file_lock->try_lock()) {
                     m_slot->file_locked          = true;
                     m_slot->consecutive_timeouts = 0;
+                    m_slot->skip_waiting_until   = {};
                     break;
                 }
             } catch (const std::exception &e) {

src/libslic3r/InstanceLock.hpp

@@ -41,8 +41,10 @@ public:
     // or a failing lock call. Mutable so tests can shorten it.
     static inline std::chrono::milliseconds cooldown{10000};
     // How often a guard re-checks that the lock file behind the path is still
-    // the one it opened. Mutable so tests can shorten it.
-    static inline std::chrono::milliseconds identity_check_interval{5000};
+    // the one it opened: on every acquisition by default, one stat, since a
+    // handle to a replaced file would lock nothing anyone else can see.
+    // Mutable so tests can change it.
+    static inline std::chrono::milliseconds identity_check_interval{0};
 
     // An empty path makes the guard a no-op.
     explicit InstanceLock(const std::string &lock_file_path, std::chrono::milliseconds timeout = default_timeout);

src/libslic3r/Preset.cpp

@@ -1866,6 +1866,7 @@ void PresetCollection::load_presets(
                 } catch (const std::ifstream::failure &err) {
                     ++m_errors;
                     BOOST_LOG_TRIVIAL(error) << boost::format("The user-config cannot be loaded: %1%. Reason: %2%")%preset.file %err.what();
+                    InstanceLock instance_lock(user_presets_lock_path(read_only));
                     fs::path file_path(preset.file);
                     if (!read_only && fs::exists(file_path))
                         fs::remove(file_path);
@@ -1877,6 +1878,7 @@ void PresetCollection::load_presets(
                     ++m_errors;
                     BOOST_LOG_TRIVIAL(error) << boost::format("Failed loading the user-config file: %1%. Reason: %2%")%preset.file %err.what();
                     //throw Slic3r::RuntimeError(std::string("Failed loading the preset file: ") + preset.file + "\n\tReason: " + err.what());
+                    InstanceLock instance_lock(user_presets_lock_path(read_only));
                     fs::path file_path(preset.file);
                     if (!read_only && fs::exists(file_path))
                         fs::remove(file_path);

src/libslic3r/utils.cpp

@@ -740,7 +740,12 @@ std::error_code rename_file(const std::string &from, const std::string &to)
 static bool is_writable(const std::string &path)
 {
 #ifdef _WIN32
-	return ::_waccess(boost::nowide::widen(path).c_str(), 2) == 0;
+	// _waccess() sees only the read-only attribute; an open for writing sees ACLs too.
+	HANDLE handle = ::CreateFileW(boost::nowide::widen(path).c_str(), GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr);
+	if (handle == INVALID_HANDLE_VALUE)
+		return false;
+	::CloseHandle(handle);
+	return true;
 #else
 	return ::access(path.c_str(), W_OK) == 0;
 #endif
@@ -784,9 +789,9 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_
 		boost::nowide::remove(tmp_path.c_str());
 		// A directory that allows writing the file but not creating one beside
 		// it, or a name the longer temporary pushes past the path limit.
-		const bool only_the_temporary_failed = ec == std::errc::permission_denied || ec == std::errc::no_such_file_or_directory ||
+		const bool only_the_temporary_failed = (target_exists && (ec == std::errc::permission_denied || ec == std::errc::no_such_file_or_directory)) ||
 		                                       ec == std::errc::filename_too_long;
-		if (target_exists && only_the_temporary_failed) {
+		if (only_the_temporary_failed) {
 			BOOST_LOG_TRIVIAL(warning) << "Cannot create a temporary beside " << path << " (" << ec.message() << "); writing in place";
 			return write_whole_file(path, chunks, binary);
 		}

src/slic3r/GUI/GUI_App.cpp

@@ -3532,7 +3532,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();
 
     });

tests/libslic3r/test_instance_lock.cpp

@@ -18,6 +18,15 @@
 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");
@@ -67,8 +76,7 @@ TEST_CASE("InstanceLock retries a lock file it could not open once the cool-down
 {
     ScopedTemporaryDir dir;
     const std::string  path = (dir.path() / "later" / "shared.lock").string();
-    const auto         saved_cooldown = InstanceLock::cooldown;
-    InstanceLock::cooldown = 300ms;
+    ScopedStaticValue  cooldown(InstanceLock::cooldown, 300ms);
 
     bool before_dir, during_cooldown, after_cooldown;
     const auto started = std::chrono::steady_clock::now();
@@ -87,7 +95,6 @@ TEST_CASE("InstanceLock retries a lock file it could not open once the cool-down
         InstanceLock lock(path, 100ms);
         after_cooldown = lock.locked();
     }
-    InstanceLock::cooldown = saved_cooldown;
 
     REQUIRE_FALSE(before_dir);
     // A loaded runner may take longer than the cool-down to get here; then the
@@ -101,8 +108,7 @@ TEST_CASE("InstanceLock reopens a lock file that was replaced on disk", "[Instan
 {
     ScopedTemporaryFile lock_file(".lock");
     const std::string   path = lock_file.string();
-    const auto          saved_interval = InstanceLock::identity_check_interval;
-    InstanceLock::identity_check_interval = 0ms;
+    ScopedStaticValue   interval(InstanceLock::identity_check_interval, 0ms);
     {
         InstanceLock lock(path);
         REQUIRE(lock.locked());
@@ -110,7 +116,6 @@ TEST_CASE("InstanceLock reopens a lock file that was replaced on disk", "[Instan
 
     boost::filesystem::remove(path);
     InstanceLock lock(path);
-    InstanceLock::identity_check_interval = saved_interval;
     REQUIRE(lock.locked());
     // Only a reopen recreates the file; a guard still holding the unlinked one
     // would leave the path missing. (The inode number itself may be reused once