fix: backend code review — atomic writes, lock ordering, validation

1 files changed, 25 insertions, 14 deletions

diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs
index 3ca8d99..f596966 100644
--- a/src-tauri/src/lib.rs
+++ b/src-tauri/src/lib.rs
@@ -10,6 +10,10 @@ use uuid::Uuid;
 use state::{AppDataWrapper, TimerPhase, TimerState, TimerStateWrapper};
 use storage::{Settings, Task};
 
+// LOCK ORDERING: When acquiring multiple locks, always take timer_state
+// before app_data to prevent deadlock. Never acquire app_data first
+// while timer_state is not held.
+
 // ── Helper ──────────────────────────────────────────────────────────────────
 
 fn app_data_dir(app: &AppHandle) -> std::path::PathBuf {
@@ -140,6 +144,9 @@ fn update_settings(
     timer: State<'_, TimerStateWrapper>,
     app: AppHandle,
 ) -> Result<(), String> {
+    if settings.sessions_before_long_break == 0 {
+        return Err("sessions_before_long_break must be at least 1".to_string());
+    }
     {
         let mut app_data = data.data.lock().unwrap();
         app_data.settings = settings.clone();
@@ -195,19 +202,24 @@ fn update_task(
     data: State<'_, AppDataWrapper>,
     app: AppHandle,
 ) -> Result<(), String> {
-    let mut app_data = data.data.lock().unwrap();
-    let task = app_data
-        .tasks
-        .iter_mut()
-        .find(|t| t.id == id)
-        .ok_or_else(|| format!("Task {} not found", id))?;
-    if let Some(r) = remaining_sessions {
-        task.remaining_sessions = r;
-    }
-    if let Some(c) = completed {
-        task.completed = c;
-    }
-    storage::save(&app_data_dir(&app), &app_data)?;
+    // Collect mutated data inside block, then save outside so the lock is
+    // not held across the I/O call (consistent with LOCK ORDERING above).
+    let data_snapshot = {
+        let mut app_data = data.data.lock().unwrap();
+        let task = app_data
+            .tasks
+            .iter_mut()
+            .find(|t| t.id == id)
+            .ok_or_else(|| format!("Task {} not found", id))?;
+        if let Some(r) = remaining_sessions {
+            task.remaining_sessions = r;
+        }
+        if let Some(c) = completed {
+            task.completed = c;
+        }
+        app_data.clone()
+    };
+    storage::save(&app_data_dir(&app), &data_snapshot)?;
     Ok(())
 }
 
@@ -266,7 +278,6 @@ pub fn run() {
             app.manage(TimerStateWrapper(Arc::clone(&timer_arc)));
             app.manage(AppDataWrapper {
                 data: Arc::clone(&data_arc),
-                data_dir: data_dir.clone(),
             });
 
             // Spawn background timer thread