From 25e1dcf205cd14feafdd9b4cf6b7a66f253ba6d2 Mon Sep 17 00:00:00 2001
From: Solstice <solstice@local>
Date: Tue, 9 Jun 2026 00:17:02 -0700
Subject: fix: backend code review — atomic writes, lock ordering, validation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 src-tauri/src/lib.rs | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

(limited to 'src-tauri/src/lib.rs')

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
-- 
cgit v1.3-2-g0d8e