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

4 files changed, 41 insertions, 21 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
diff --git a/src-tauri/src/state.rs b/src-tauri/src/state.rs
index 9cd3698..8f3d52b 100644
--- a/src-tauri/src/state.rs
+++ b/src-tauri/src/state.rs
@@ -1,4 +1,3 @@
-use std::path::PathBuf;
 use std::sync::{Arc, Mutex};
 use serde::{Deserialize, Serialize};
 use crate::storage::AppData;
@@ -24,11 +23,9 @@ pub struct TimerState {
 /// Wrapper held in Tauri managed state — contains the Arc so commands can clone it.
 pub struct TimerStateWrapper(pub Arc<Mutex<TimerState>>);
 
-/// Wrapper for AppData — contains the Arc and the data directory path.
+/// Wrapper for AppData — contains the Arc.
 pub struct AppDataWrapper {
     pub data: Arc<Mutex<AppData>>,
-    #[allow(dead_code)]
-    pub data_dir: PathBuf,
 }
 
 impl TimerState {
diff --git a/src-tauri/src/storage.rs b/src-tauri/src/storage.rs
index 28b1f86..8e2f148 100644
--- a/src-tauri/src/storage.rs
+++ b/src-tauri/src/storage.rs
@@ -57,10 +57,15 @@ pub fn load(app_data_dir: &PathBuf) -> AppData {
     if !path.exists() {
         return AppData::default();
     }
-    match fs::read_to_string(&path) {
+    let mut data = match fs::read_to_string(&path) {
         Ok(contents) => serde_json::from_str(&contents).unwrap_or_default(),
         Err(_) => AppData::default(),
+    };
+    // Guard against a corrupt/zero value that would cause division by zero
+    if data.settings.sessions_before_long_break == 0 {
+        data.settings.sessions_before_long_break = Settings::default().sessions_before_long_break;
     }
+    data
 }
 
 pub fn save(app_data_dir: &PathBuf, data: &AppData) -> Result<(), String> {
@@ -69,7 +74,10 @@ pub fn save(app_data_dir: &PathBuf, data: &AppData) -> Result<(), String> {
     let path = data_path(app_data_dir);
     let contents = serde_json::to_string_pretty(data)
         .map_err(|e| format!("Failed to serialize data: {}", e))?;
-    fs::write(&path, contents)
-        .map_err(|e| format!("Failed to write data file: {}", e))?;
+    let tmp_path = app_data_dir.join("data.json.tmp");
+    fs::write(&tmp_path, contents)
+        .map_err(|e| format!("Failed to write temp data file: {}", e))?;
+    fs::rename(&tmp_path, &path)
+        .map_err(|e| format!("Failed to finalize data file: {}", e))?;
     Ok(())
 }
diff --git a/src-tauri/src/timer.rs b/src-tauri/src/timer.rs
index 7e26ad8..44e5999 100644
--- a/src-tauri/src/timer.rs
+++ b/src-tauri/src/timer.rs
@@ -7,6 +7,10 @@ use tauri::{AppHandle, Emitter};
 use crate::state::{TimerPhase, TimerState};
 use crate::storage::{self, AppData};
 
+// 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.
+
 // ── Event payloads ──────────────────────────────────────────────────────────
 
 #[derive(Clone, Serialize)]