Fix user presets deletion on auto-save (Issue #371)

CHANGELOG_DEV.md

@@ -4,6 +4,16 @@ All notable changes to this project will be documented in this file.
 
 ---
 
+## [0.7.192]
+- **Fixed User Presets Deletion on Auto-Save (Issue #371)**:
+  - Eliminated "lazy loading" of the presets library which previously only occurred when opening the Tuning GUI.
+  - Implemented mandatory preset loading within `Config::Load` to ensure the in-memory library is always synchronized with the configuration file upon application startup.
+  - Added `g_engine_mutex` protection to `Config::LoadPresets` to ensure thread-safety during multi-threaded auto-save operations.
+  - This fix prevents the FFB engine from overwriting the `config.ini` file with an empty preset list during early session events (like static load latching).
+- **Testing**:
+  - Added `tests/test_issue_371_repro.cpp` which simulates a configuration load followed by an immediate save to verify preset persistence.
+  - Verified 100% pass rate across the full test suite.
+
 ## [0.7.191]
 - **Fixed Telemetry Diagnostic Reset on Car Change (Issue #374)**:
   - Implemented automatic reset of all telemetry error counters and warning flags whenever a car change is detected.

VERSION

@@ -1 +1 @@
-0.7.191
+0.7.192

docs/dev_docs/code_reviews/issue_371_review_iteration_1.md

@@ -0,0 +1,36 @@
+# Code Review - Issue #371: Imported profiles don't save.
+
+This review evaluates the proposed fix for **Issue #371: Imported profiles don't save.**
+
+### Analysis and Reasoning
+
+**1. User's Goal:**
+The objective is to ensure that user presets (profiles) are correctly persisted to the configuration file, specifically preventing data loss that occurs when an auto-save triggers before the Tuning GUI (which previously handled preset loading) has been opened.
+
+**2. Evaluation of the Solution:**
+
+*   **Core Functionality:** The patch correctly identifies the root cause: "lazy loading" of the presets library. By moving the call to `LoadPresets` into the central `Config::Load` function, the in-memory `Config::presets` vector is guaranteed to be populated as soon as the application loads its settings. This ensures that any subsequent `Config::Save` operation (including auto-saves) will write the full set of presets back to the file rather than overwriting them with an empty list. The regression test accurately simulates the failure condition and verifies the fix.
+
+*   **Safety & Side Effects:**
+    *   **Thread Safety (Blocking):** The project instructions for "Fixer" explicitly mandate the use of `g_engine_mutex` when modifying shared state. `Config::presets` is a static vector (shared state). The issue description highlights that the bug often occurs during "auto-save," which typically runs on a background thread or is triggered by physics events. `Config::LoadPresets` performs a `presets.clear()` followed by multiple `emplace_back` operations. If a concurrent thread calls `Config::Save` or the GUI tries to render the list during this time, it will cause an iterator invalidation crash or data corruption. No locking mechanism was added to protect this shared state.
+    *   **Logic:** The modification to `LoadPresets` to accept a filename is sound and maintains consistency with how `Config::Load` handles paths.
+
+*   **Completeness (Blocking):**
+    *   **Missing Mandatory Deliverables:** The patch is missing several critical files required by the "Fixer" workflow:
+        *   The `VERSION` file update (required increment).
+        *   The `CHANGELOG` update.
+        *   The quality assurance records (e.g., `docs/dev_docs/code_reviews/issue_371_review_iteration_1.md`).
+    *   **Incomplete Documentation:** The implementation plan (`plan_issue_371.md`) contains empty checkboxes and has not been updated with final implementation notes or results of the review loop, as required by the instructions.
+
+**3. Merge Assessment (Blocking vs. Nitpicks):**
+
+*   **Blocking:**
+    1.  **Thread Safety:** Lack of `g_engine_mutex` protection for the static `presets` vector during modification in a multi-threaded context.
+    2.  **Process Violation:** Missing mandatory versioning, changelog, and review documentation.
+    3.  **Documentation:** The implementation plan is an unfinalized draft.
+*   **Nitpicks:**
+    1.  The repro test code (`test_issue_371_repro.cpp`) contains comments stating the code is "currently broken" and "expected to FAIL," which is confusing in a final commit. These should be updated to reflect that they are now verifying the fix.
+
+### Final Rating: #Partially Correct#
+
+The patch successfully identifies and solves the logical bug with a valid architectural change and a solid regression test. However, it is not commit-ready because it neglects critical thread-safety requirements and fails to include multiple mandatory documentation and versioning files required by the specified workflow.

docs/dev_docs/code_reviews/issue_371_review_iteration_2.md

@@ -0,0 +1,28 @@
+# Code Review - Issue #371: Imported profiles don't save. (Iteration 2)
+
+The proposed code change is a comprehensive and well-architected solution to **Issue #371: Imported profiles don't save.**
+
+### Analysis and Reasoning
+
+1.  **User's Goal:**
+    The objective is to prevent the loss of user-defined and imported FFB presets, which were being deleted from the configuration file if an "auto-save" event occurred before the user opened the Tuning GUI.
+
+2.  **Evaluation of the Solution:**
+    *   **Core Functionality:** The patch correctly identifies the root cause: the "lazy loading" of presets. By moving the call to `Config::LoadPresets` directly into the central `Config::Load` function, the in-memory presets library is guaranteed to be populated as soon as the application starts. This ensures that any subsequent `Config::Save` (including background auto-saves) writes the full set of presets back to the file instead of an empty list.
+    *   **Safety & Side Effects:**
+        *   **Thread Safety:** The patch adds `g_engine_mutex` protection to `Config::LoadPresets`. Since `Config::presets` is a static shared resource and auto-saves can occur on different threads or during physics events, this is a critical safety addition that prevents race conditions and iterator invalidation.
+        *   **Recursive Mutex:** The implementation notes correctly identify that `Config::Load` already holds the mutex, so the use of `std::recursive_mutex` (which is standard in this project's engine state management) prevents deadlocks during the nested call.
+        *   **Regressions:** No existing logic is removed; the change simply ensures data is present before it is saved.
+    *   **Completeness:** The patch includes all mandatory deliverables:
+        *   C++ source and header updates.
+        *   A robust regression test (`test_issue_371_repro.cpp`) that successfully reproduces and verifies the fix.
+        *   Updates to `VERSION` and `CHANGELOG_DEV.md`.
+        *   Required documentation including a detailed implementation plan and code review records demonstrating the iterative improvement process.
+
+3.  **Merge Assessment:**
+    *   **Blocking:** None.
+    *   **Nitpicks:** None. The agent successfully addressed previous nitpicks (like confusing comments in the test) during its autonomous iteration cycle.
+
+The solution is highly maintainable, follows the project's reliability standards, and directly resolves the reported bug with appropriate safety measures and verification.
+
+### Final Rating: #Correct#

docs/dev_docs/implementation_plans/plan_issue_371.md

@@ -0,0 +1,64 @@
+# Implementation Plan - Issue #371: Imported profiles don't save.
+
+## Context
+User profiles (presets) were being lost because the `Config::presets` library was only populated when the Tuning GUI window was first opened. If an auto-save occurred before this (e.g., when static load was latched during the first few seconds of driving), `Config::Save` would overwrite `config.ini` with an empty preset list, effectively deleting all user-created and imported presets.
+
+## Design Rationale
+The root cause was a "lazy loading" pattern of the presets library that didn't account for the fact that the `Save` operation is global and destructive to sections not currently in memory. By moving the loading of the presets library to the application's initialization phase (within `Config::Load`), we guarantee that the memory state always matches the disk state before any write operation occurs.
+
+## Reference Documents
+- GitHub Issue #371
+- `src/core/Config.cpp`
+- `src/core/main.cpp`
+
+## Codebase Analysis Summary
+- **Current architecture:** `Config::Load` loads active engine settings. `Config::LoadPresets` loads the library of presets. `Config::Save` writes both.
+- **Impacted functionalities:**
+    - `Config::Load`: Now triggers preset loading.
+    - `Config::LoadPresets`: Now supports loading from a specific file path to stay in sync with `Config::Load`.
+    - `GuiLayer_Common`: The lazy-loading check remains as a safety measure but is now redundant.
+- **Design Rationale:** `Config::Load` is the central entry point for loading settings at startup. It is the logical place to ensure all configuration data (including the library) is loaded.
+
+## FFB Effect Impact Analysis
+- No direct impact on FFB physics math.
+- Indirect impact: Ensures user preferences for FFB effects are correctly persisted across sessions.
+
+## Proposed Changes
+
+### 1. `src/core/Config.h` & `src/core/Config.cpp`
+- **Modify `Config::LoadPresets` signature:**
+  - Changed from `static void LoadPresets();` to `static void LoadPresets(const std::string& filename = "");`.
+- **Implement filename support in `Config::LoadPresets`:**
+  - Uses the provided filename or falls back to `m_config_path`.
+- **Update `Config::Load`:**
+  - Calls `LoadPresets(filename);` at the end of the `Load` function.
+- **Design Rationale:** This ensures that whenever a config is loaded (at startup or during tests), the associated presets are also loaded into memory, preventing data loss on subsequent saves.
+- **Thread Safety:** Added `g_engine_mutex` protection to `LoadPresets` as it modifies the static `presets` vector which can be accessed by the FFB thread (for auto-saves) or the GUI thread.
+
+### 2. Version Increment
+- Increment version in `VERSION` to `0.7.192`.
+- Update `CHANGELOG_DEV.md` with the fix.
+
+## Detailed Steps
+
+1.  **Create a reproduction test case in `tests/test_issue_371_repro.cpp`.** (Done)
+2.  **Verify the test failure.** (Done)
+3.  **Modify `src/core/Config.h` and `src/core/Config.cpp` to implement the fix.** (Done)
+4.  **Verify the fix with the reproduction test.** (Done)
+5.  **Perform an independent code review.** (Done)
+6.  **Address any review issues and iterate if necessary.** (Done)
+7.  **Run the full test suite to ensure no regressions.** (Done)
+8.  **Complete pre-commit steps.** (Done)
+9.  **Submit the changes.** (Pending)
+
+## Implementation Notes
+- **Review Iteration 1:** The review correctly pointed out the lack of thread safety for the static `presets` vector and the missing versioning/changelog deliverables. I addressed these by adding `g_engine_mutex` to `LoadPresets` and updating the mandatory files.
+- **Thread Safety:** `LoadPresets` now correctly uses `g_engine_mutex`. Since `Config::Load` also uses it, and `Load` calls `LoadPresets`, the mutex was switched to `recursive_mutex` in a previous version, which is perfect for this nested call.
+- **Test Integrity:** The reproduction test `test_issue_371_repro.cpp` was integrated into the CMake build and verifies that presets are no longer lost after a load-save cycle.
+
+## Deliverables
+- [x] Modified `src/core/Config.h` and `src/core/Config.cpp`.
+- [x] New regression test case `tests/test_issue_371_repro.cpp`.
+- [x] Updated `VERSION` and `CHANGELOG_DEV.md`.
+- [x] Quality Assurance Records: `docs/dev_docs/code_reviews/issue_371_review_iteration_1.md` and `issue_371_review_iteration_2.md`.
+- [x] Implementation Notes updated in the final plan.

src/core/Config.cpp

@@ -390,7 +390,8 @@ void Config::ParsePresetLine(const std::string& line, Preset& current_preset, st
     }
 }
 
-void Config::LoadPresets() {
+void Config::LoadPresets(const std::string& filename) {
+    std::lock_guard<std::recursive_mutex> lock(g_engine_mutex);
     presets.clear();
 
     // 1. Default - Uses Preset struct defaults from Config.h (Single Source of Truth)
@@ -1078,9 +1079,9 @@ void Config::LoadPresets() {
         .SetSlipSmoothing(0.015f)
     );
 
-    // --- Parse User Presets from config.ini ---
-    // (Keep the existing parsing logic below, it works fine for file I/O)
-    std::ifstream file(m_config_path);
+    // --- Parse User Presets from file ---
+    std::string final_path = filename.empty() ? m_config_path : filename;
+    std::ifstream file(final_path);
     if (!file.is_open()) return;
 
     std::string line;
@@ -1925,6 +1926,9 @@ void Config::Load(FFBEngine& engine, const std::string& filename) {
     engine.m_safety.m_safety_slew_full_scale_time_s = (std::max)(0.01f, engine.m_safety.m_safety_slew_full_scale_time_s);
     engine.m_safety.m_stutter_threshold = (std::max)(1.01f, engine.m_safety.m_stutter_threshold);
 
+    // v0.7.192: Fix Issue #371 - Load presets library immediately
+    LoadPresets(final_path);
+
     Logger::Get().LogFile("[Config] Loaded from %s", final_path.c_str());
 }
 

src/core/Config.h

@@ -872,7 +872,7 @@ class Config {
     // Preset Management
     static std::vector<Preset> presets;
     static std::string m_last_preset_name; // NEW (v0.7.14)
-    static void LoadPresets(); // Populates presets vector
+    static void LoadPresets(const std::string& filename = ""); // Populates presets vector
     static void ApplyPreset(int index, FFBEngine& engine);
 
     // NEW: Add a user preset

tests/CMakeLists.txt

@@ -114,6 +114,7 @@ set(TEST_SOURCES
     test_issue_278_derived_acceleration.cpp
     test_issue_281_spikes.cpp
     test_issue_374_repro.cpp
+    test_issue_371_repro.cpp
     repro_issue_290.cpp
     test_issue_282_lateral_load_fix.cpp
     test_issue_306_lateral_load.cpp

tests/test_issue_371_repro.cpp

@@ -0,0 +1,61 @@
+#include "test_ffb_common.h"
+#include <fstream>
+#include <filesystem>
+
+namespace FFBEngineTests {
+
+TEST_CASE(test_issue_371_repro, "Config") {
+    std::cout << "\nTest: Issue #371 - Imported profiles preserved on early save" << std::endl;
+
+    std::string test_config = "test_issue_371.ini";
+
+    // 1. Create a config file with a user preset
+    {
+        std::ofstream file(test_config);
+        file << "ini_version=0.7.191\n";
+        file << "gain=1.0\n";
+        file << "\n[Presets]\n";
+        file << "[Preset:Imported Car]\n";
+        file << "gain=0.85\n";
+        file << "sop=1.5\n";
+        file.close();
+    }
+
+    // 2. Simulate fresh application start (empty presets vector)
+    Config::presets.clear();
+    FFBEngine engine;
+    InitializeEngine(engine);
+
+    // 3. Call Load (should now call LoadPresets internally)
+    Config::Load(engine, test_config);
+
+    // Verify presets vector is populated
+    std::cout << "Presets count after Load: " << Config::presets.size() << std::endl;
+
+    // 4. Simulate an auto-save (e.g. from static load latching)
+    Config::Save(engine, test_config);
+
+    // 5. Verify if the preset still exists in the file
+    {
+        std::ifstream file(test_config);
+        std::string line;
+        bool found_preset = false;
+        while (std::getline(file, line)) {
+            if (line.find("[Preset:Imported Car]") != std::string::npos) {
+                found_preset = true;
+                break;
+            }
+        }
+        file.close();
+
+        if (!found_preset) {
+            FAIL_TEST("Preset 'Imported Car' was lost after Save!");
+        }
+    }
+
+    // Clean up
+    if (std::filesystem::exists(test_config)) std::filesystem::remove(test_config);
+    std::cout << "[PASS] Issue #371 verification finished." << std::endl;
+}
+
+} // namespace FFBEngineTests