From e33fb9dd0b869e50479c71e6091733b3ac6f07bb Mon Sep 17 00:00:00 2001 From: Tristan Michael Date: Sun, 5 Apr 2026 15:44:49 -0700 Subject: [PATCH] Replace timestamp LWW with checksum-based conflict resolution Rework sync conflict handling to stop using timestamp-based last-write-wins and use a single Conflict action. The conflict handler now downloads the remote file and compares SHA-256 checksums: identical content is treated as a false conflict and skipped; when different, the remote version wins and the local task file is recovered as a duplicate with a new UUID and a "[RECOVERED FROM CONFLICT]" prefix. Duplicates are inserted adjacent to the original in .listdata.json; non-task files still use remote-wins without duplication. Removed local_wins() and its tests, simplified action variants to a single Conflict, and updated conflict-related tests and reporting accordingly. --- crates/onyx-core/src/sync.rs | 237 +++++++++++++++++------------------ 1 file changed, 113 insertions(+), 124 deletions(-) diff --git a/crates/onyx-core/src/sync.rs b/crates/onyx-core/src/sync.rs index 9130455..9b6d8d3 100644 --- a/crates/onyx-core/src/sync.rs +++ b/crates/onyx-core/src/sync.rs @@ -3,7 +3,9 @@ use std::path::Path; use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; use sha2::{Sha256, Digest}; +use uuid::Uuid; use crate::error::{Error, Result}; +use crate::storage::{ListMetadata, TaskFrontmatter}; use crate::webdav::WebDavClient; // --- Sync State --- @@ -32,8 +34,7 @@ pub enum SyncAction { Download { path: String }, DeleteLocal { path: String }, DeleteRemote { path: String }, - ConflictLocalWins { path: String }, - ConflictRemoteWins { path: String }, + Conflict { path: String }, } impl SyncAction { @@ -43,8 +44,7 @@ impl SyncAction { | SyncAction::Download { path } | SyncAction::DeleteLocal { path } | SyncAction::DeleteRemote { path } - | SyncAction::ConflictLocalWins { path } - | SyncAction::ConflictRemoteWins { path } => path, + | SyncAction::Conflict { path } => path, } } } @@ -137,12 +137,7 @@ pub fn compute_sync_actions( (true, false) => actions.push(SyncAction::Upload { path: path.to_string() }), (false, true) => actions.push(SyncAction::Download { path: path.to_string() }), (true, true) => { - // Both modified: last-write-wins based on timestamps - if local_wins(l.modified_at.as_deref(), r.last_modified.as_deref()) { - actions.push(SyncAction::ConflictLocalWins { path: path.to_string() }); - } else { - actions.push(SyncAction::ConflictRemoteWins { path: path.to_string() }); - } + actions.push(SyncAction::Conflict { path: path.to_string() }); } } } @@ -157,13 +152,9 @@ pub fn compute_sync_actions( actions.push(SyncAction::Download { path: path.to_string() }); } - // Both present, no base (both added): last-write-wins - (Some(l), Some(r), None) => { - if local_wins(l.modified_at.as_deref(), r.last_modified.as_deref()) { - actions.push(SyncAction::ConflictLocalWins { path: path.to_string() }); - } else { - actions.push(SyncAction::ConflictRemoteWins { path: path.to_string() }); - } + // Both present, no base (both added): conflict + (Some(_), Some(_), None) => { + actions.push(SyncAction::Conflict { path: path.to_string() }); } // Local present, remote gone, base known: remote was deleted @@ -221,19 +212,6 @@ fn timestamps_equal(a: Option<&str>, b: Option<&str>) -> bool { } } -/// Determine if local wins based on timestamps. True means local wins. -fn local_wins(local_modified: Option<&str>, remote_modified: Option<&str>) -> bool { - // Try parsing both; if we can't parse, local wins by default - let local_ts = local_modified.and_then(parse_timestamp); - let remote_ts = remote_modified.and_then(parse_timestamp); - match (local_ts, remote_ts) { - (Some(l), Some(r)) => l >= r, - (Some(_), None) => true, - (None, Some(_)) => false, - (None, None) => true, // Default to local - } -} - /// Parse a timestamp string (ISO 8601 or HTTP date format). fn parse_timestamp(s: &str) -> Option> { // Try ISO 8601 / RFC 3339 @@ -342,8 +320,7 @@ fn queued_op_to_action(op: &QueuedOperation) -> Option { "download" => Some(SyncAction::Download { path }), "delete_local" => Some(SyncAction::DeleteLocal { path }), "delete_remote" => Some(SyncAction::DeleteRemote { path }), - "conflict_local_wins" => Some(SyncAction::ConflictLocalWins { path }), - "conflict_remote_wins" => Some(SyncAction::ConflictRemoteWins { path }), + "conflict" => Some(SyncAction::Conflict { path }), _ => None, } } @@ -354,8 +331,7 @@ fn action_to_queued_op(action: &SyncAction) -> QueuedOperation { SyncAction::Download { path } => ("download", path), SyncAction::DeleteLocal { path } => ("delete_local", path), SyncAction::DeleteRemote { path } => ("delete_remote", path), - SyncAction::ConflictLocalWins { path } => ("conflict_local_wins", path), - SyncAction::ConflictRemoteWins { path } => ("conflict_remote_wins", path), + SyncAction::Conflict { path } => ("conflict", path), }; QueuedOperation { action_type: action_type.to_string(), @@ -567,11 +543,11 @@ async fn sync_workspace_inner( // Merge with offline queue let all_actions = queue.merge_with_actions(fresh_actions); - // Filter by sync mode + // Filter by sync mode (conflicts always run in any mode since they need both sides) let actions: Vec = all_actions.into_iter().filter(|a| match mode { SyncMode::Full => true, - SyncMode::Push => matches!(a, SyncAction::Upload { .. } | SyncAction::DeleteRemote { .. } | SyncAction::ConflictLocalWins { .. }), - SyncMode::Pull => matches!(a, SyncAction::Download { .. } | SyncAction::DeleteLocal { .. } | SyncAction::ConflictRemoteWins { .. }), + SyncMode::Push => matches!(a, SyncAction::Upload { .. } | SyncAction::DeleteRemote { .. } | SyncAction::Conflict { .. }), + SyncMode::Pull => matches!(a, SyncAction::Download { .. } | SyncAction::DeleteLocal { .. } | SyncAction::Conflict { .. }), }).collect(); // Execute actions, collecting failures for the queue @@ -581,22 +557,17 @@ async fn sync_workspace_inner( match execute_action(&client, workspace_path, action, &mut sync_state, &report).await { Ok(()) => { match action { - SyncAction::Upload { .. } | SyncAction::ConflictLocalWins { .. } => result.uploaded += 1, - SyncAction::Download { .. } | SyncAction::ConflictRemoteWins { .. } => result.downloaded += 1, + SyncAction::Upload { .. } => result.uploaded += 1, + SyncAction::Download { .. } => result.downloaded += 1, SyncAction::DeleteLocal { .. } => result.deleted_local += 1, SyncAction::DeleteRemote { .. } => result.deleted_remote += 1, + SyncAction::Conflict { .. } => result.conflicts += 1, } } Err(e) => { let msg = format!("Failed {}: {}", action.path(), e); report(&format!(" ! {}", msg)); result.errors.push(msg); - if matches!(action, - SyncAction::Upload { .. } | SyncAction::Download { .. } - | SyncAction::ConflictLocalWins { .. } | SyncAction::ConflictRemoteWins { .. } - ) { - result.conflicts += 1; - } failed_actions.push(action.clone()); } } @@ -643,22 +614,80 @@ async fn execute_action( sync_state.record_file(path, &checksum, modified.as_deref(), data.len() as u64); } - SyncAction::ConflictLocalWins { path } => { + SyncAction::Conflict { path } => { let local_path = workspace_path.join(path.replace('/', std::path::MAIN_SEPARATOR_STR)); - let data = std::fs::read(&local_path)?; - let checksum = compute_checksum(&data); + let local_data = std::fs::read(&local_path)?; + let local_checksum = compute_checksum(&local_data); - if let Some(parent) = path_parent(path) { - client.ensure_dir(parent).await?; + let remote_data = client.get_file(path).await?; + let remote_checksum = compute_checksum(&remote_data); + + // If checksums match, it's a false conflict — both sides made the same edit + if local_checksum == remote_checksum { + report(&format!(" = Conflict resolved: identical content for {}", path)); + let modified = std::fs::metadata(&local_path).ok() + .and_then(|m| m.modified().ok()) + .map(|t| { let dt: DateTime = t.into(); dt.to_rfc3339() }); + sync_state.record_file(path, &local_checksum, modified.as_deref(), local_data.len() as u64); + } else { + report(&format!(" ! Conflict: remote wins for {}, recovering local as duplicate", path)); + + // Remote wins: overwrite local with remote content + std::fs::write(&local_path, &remote_data)?; + let modified = std::fs::metadata(&local_path).ok() + .and_then(|m| m.modified().ok()) + .map(|t| { let dt: DateTime = t.into(); dt.to_rfc3339() }); + sync_state.record_file(path, &remote_checksum, modified.as_deref(), remote_data.len() as u64); + + // For .md task files inside a list dir, create a duplicate of the local version + let parts: Vec<&str> = path.split('/').collect(); + if parts.len() == 2 && parts[1].ends_with(".md") && parts[1] != ".listdata.json" { + let local_content = String::from_utf8_lossy(&local_data); + if let Ok((frontmatter, description)) = parse_frontmatter_for_conflict(&local_content) { + let original_id = frontmatter.id; + let new_id = Uuid::new_v4(); + let prefixed_desc = if description.is_empty() { + "[RECOVERED FROM CONFLICT]".to_string() + } else { + format!("[RECOVERED FROM CONFLICT]\n{}", description) + }; + + let new_frontmatter = TaskFrontmatter { + id: new_id, + ..frontmatter + }; + let yaml = serde_yaml::to_string(&new_frontmatter) + .map_err(|e| Error::Sync(e.to_string()))?; + let new_content = format!("---\n{}---\n\n{}", yaml, prefixed_desc); + + // Write the duplicate file using the new UUID as filename + let list_dir = workspace_path.join(parts[0]); + let dup_filename = format!("{}.md", new_id); + let dup_path = list_dir.join(&dup_filename); + std::fs::write(&dup_path, &new_content)?; + + // Insert new task adjacent to original in .listdata.json + let listdata_path = list_dir.join(".listdata.json"); + if listdata_path.exists() { + if let Ok(content) = std::fs::read_to_string(&listdata_path) { + if let Ok(mut metadata) = serde_json::from_str::(&content) { + let insert_pos = metadata.task_order.iter() + .position(|id| *id == original_id) + .map(|p| p + 1) + .unwrap_or(metadata.task_order.len()); + metadata.task_order.insert(insert_pos, new_id); + if let Ok(json) = serde_json::to_string_pretty(&metadata) { + let _ = std::fs::write(&listdata_path, json); + } + } + } + } + + // Don't record the duplicate in sync state — next sync will see it + // as "local added, remote absent" and upload it automatically. + } + } } - - report(&format!(" ^ Conflict: uploading local version of {}", path)); - client.put_file(path, data.clone()).await?; - - let modified = std::fs::metadata(&local_path).ok() - .and_then(|m| m.modified().ok()) - .map(|t| { let dt: DateTime = t.into(); dt.to_rfc3339() }); - sync_state.record_file(path, &checksum, modified.as_deref(), data.len() as u64); } SyncAction::Download { path } => { @@ -679,29 +708,6 @@ async fn execute_action( sync_state.record_file(path, &checksum, modified.as_deref(), data.len() as u64); } - SyncAction::ConflictRemoteWins { path } => { - let local_path = workspace_path.join(path.replace('/', std::path::MAIN_SEPARATOR_STR)); - // Back up local version before overwriting with remote - if local_path.exists() { - let backup_path = local_path.with_extension("conflict-backup"); - let _ = std::fs::copy(&local_path, &backup_path); - report(&format!(" ! Backed up local version to {}", backup_path.display())); - } - report(&format!(" v Conflict: downloading remote version of {}", path)); - let data = client.get_file(path).await?; - let checksum = compute_checksum(&data); - - if let Some(parent) = local_path.parent() { - std::fs::create_dir_all(parent)?; - } - std::fs::write(&local_path, &data)?; - - let modified = std::fs::metadata(&local_path).ok() - .and_then(|m| m.modified().ok()) - .map(|t| { let dt: DateTime = t.into(); dt.to_rfc3339() }); - sync_state.record_file(path, &checksum, modified.as_deref(), data.len() as u64); - } - SyncAction::DeleteLocal { path } => { report(&format!(" x Deleting local {}", path)); let local_path = workspace_path.join(path.replace('/', std::path::MAIN_SEPARATOR_STR)); @@ -720,6 +726,25 @@ async fn execute_action( Ok(()) } +/// Parse frontmatter and description from a markdown task file for conflict recovery. +fn parse_frontmatter_for_conflict(content: &str) -> Result<(TaskFrontmatter, String)> { + let lines: Vec<&str> = content.lines().collect(); + if lines.is_empty() || lines[0] != "---" { + return Err(Error::InvalidData("Missing frontmatter delimiter".to_string())); + } + let end_idx = lines[1..].iter().position(|&line| line == "---") + .ok_or_else(|| Error::InvalidData("Missing closing frontmatter delimiter".to_string()))?; + let frontmatter_str = lines[1..=end_idx].join("\n"); + let frontmatter: TaskFrontmatter = serde_yaml::from_str(&frontmatter_str) + .map_err(|e| Error::Sync(format!("Failed to parse frontmatter: {}", e)))?; + let description = if end_idx + 2 < lines.len() { + lines[end_idx + 2..].join("\n").trim().to_string() + } else { + String::new() + }; + Ok((frontmatter, description)) +} + /// Get the parent path of a sync path (e.g., "My Tasks/file.md" -> "My Tasks"). fn path_parent(path: &str) -> Option<&str> { path.rfind('/').map(|i| &path[..i]) @@ -896,11 +921,9 @@ mod tests { } #[test] - fn test_both_modified_local_newer() { - let mut local = make_local("file.md", "new_local"); - local.modified_at = Some("2026-03-15T12:00:00+00:00".to_string()); + fn test_both_modified_emits_conflict() { + let local = make_local("file.md", "new_local"); let mut remote = make_remote("file.md"); - remote.last_modified = Some("Mon, 01 Mar 2026 00:00:00 GMT".to_string()); remote.size = 200; let mut state = SyncState::default(); @@ -908,23 +931,7 @@ mod tests { let actions = compute_sync_actions(&[local], &[remote], &state); assert_eq!(actions.len(), 1); - assert_eq!(actions[0], SyncAction::ConflictLocalWins { path: "file.md".to_string() }); - } - - #[test] - fn test_both_modified_remote_newer() { - let mut local = make_local("file.md", "new_local"); - local.modified_at = Some("2026-01-01T00:00:00+00:00".to_string()); - let mut remote = make_remote("file.md"); - remote.last_modified = Some("Sun, 15 Mar 2026 12:00:00 GMT".to_string()); - remote.size = 200; - - let mut state = SyncState::default(); - state.files.insert("file.md".to_string(), make_base("old_base")); - - let actions = compute_sync_actions(&[local], &[remote], &state); - assert_eq!(actions.len(), 1); - assert_eq!(actions[0], SyncAction::ConflictRemoteWins { path: "file.md".to_string() }); + assert_eq!(actions[0], SyncAction::Conflict { path: "file.md".to_string() }); } #[test] @@ -954,17 +961,15 @@ mod tests { } #[test] - fn test_both_added_local_newer() { - let mut local = make_local("file.md", "local_content"); - local.modified_at = Some("2026-03-15T12:00:00+00:00".to_string()); - let mut remote = make_remote("file.md"); - remote.last_modified = Some("Mon, 01 Jan 2026 00:00:00 GMT".to_string()); + fn test_both_added_emits_conflict() { + let local = make_local("file.md", "local_content"); + let remote = make_remote("file.md"); let state = SyncState::default(); // No base entry let actions = compute_sync_actions(&[local], &[remote], &state); assert_eq!(actions.len(), 1); - assert_eq!(actions[0], SyncAction::ConflictLocalWins { path: "file.md".to_string() }); + assert_eq!(actions[0], SyncAction::Conflict { path: "file.md".to_string() }); } #[test] @@ -1203,22 +1208,6 @@ mod tests { assert!(result.is_none()); } - #[test] - fn test_local_wins_local_newer() { - assert!(local_wins( - Some("2026-03-15T12:00:00+00:00"), - Some("Mon, 01 Jan 2026 00:00:00 GMT"), - )); - } - - #[test] - fn test_local_wins_remote_newer() { - assert!(!local_wins( - Some("2026-01-01T00:00:00+00:00"), - Some("Sun, 15 Mar 2026 12:00:00 GMT"), - )); - } - // --- path_parent --- #[test]