diff --git a/backend/tests/test_telemetry.py b/backend/tests/test_telemetry.py index c7e4ddc..502e4c6 100644 --- a/backend/tests/test_telemetry.py +++ b/backend/tests/test_telemetry.py @@ -144,7 +144,7 @@ def test_telemetry_api_routes(client): token = login_r.json()["access_token"] headers = {"Authorization": f"Bearer {token}"} - # 2. Ingest telemetry batch + now = datetime.now(timezone.utc) payload = { "events": [ { @@ -164,8 +164,8 @@ def test_telemetry_api_routes(client): "client_type": "desktop-macos", "client_version": "1.3.2", "device_name": "TestMac", - "started_at": "2026-09-16T21:00:00Z", - "completed_at": "2026-09-16T21:01:00Z", + "started_at": (now - timedelta(minutes=5)).strftime("%Y-%m-%dT%H:%M:%SZ"), + "completed_at": (now - timedelta(minutes=4)).strftime("%Y-%m-%dT%H:%M:%SZ"), "duration_ms": 60000, "status": "failed", "files_scanned": 100, diff --git a/client-tauri/src-tauri/src/sync.rs b/client-tauri/src-tauri/src/sync.rs index d2d3b2d..0e7f616 100644 --- a/client-tauri/src-tauri/src/sync.rs +++ b/client-tauri/src-tauri/src/sync.rs @@ -355,81 +355,6 @@ pub fn run_sync(client: &mut ApiClient, local_root: &Path) -> Result<(u64, u64, let to_del_local_set: HashSet = to_del_local.iter().cloned().collect(); let to_del_remote_set: HashSet = to_del_remote.iter().cloned().collect(); - let total_work = to_del_remote.len() + to_del_local.len() - + current_remote.difference(¤t_local).filter(|p| !is_ignored(p)).count() - + current_local.difference(¤t_remote).filter(|p| !is_ignored(p)).count(); - let total_work = total_work as u64; - let mut done = 0u64; - - for path in &to_del_remote { - set_progress("delete_server", done, total_work); - if let Err(e) = client.delete_file(path) { - let err_code = classify_error(&e); - enqueue_diagnostic_event(crate::api::DiagnosticEventPayload { - trace_id: Some(trace_id.clone()), - created_at: Some(chrono::Utc::now().to_rfc3339()), - client_type: config::get_client_type().to_string(), - device_name: config::get_device_name(), - level: "ERROR".to_string(), - category: "sync.delete".to_string(), - error_code: err_code.to_string(), - message: format!("Delete server {}: {}", path, e), - context_json: Some(serde_json::json!({ "path": path }).to_string()), - }); - return Err(format!("Delete server {}: {}", path, e)); - } - state.file_hashes.remove(path); - done += 1; - } - let mut failed_local_deletions: HashSet = HashSet::new(); - for path in &to_del_local { - set_progress("delete_local", done, total_work); - let full = match safe_local_path(local_root, path) { - Some(p) => p, - None => { - log::warn!("delete_local: skipping unsafe path: {}", path); - done += 1; - continue; - } - }; - if full.exists() && full.is_file() { - if let Err(e) = std::fs::remove_file(&full) { - log::warn!("Delete local {}: {}, keeping in synced state to avoid resurrection", path, e); - failed_local_deletions.insert(path.clone()); - let err_code = classify_error(&e.to_string()); - enqueue_diagnostic_event(crate::api::DiagnosticEventPayload { - trace_id: Some(trace_id.clone()), - created_at: Some(chrono::Utc::now().to_rfc3339()), - client_type: config::get_client_type().to_string(), - device_name: config::get_device_name(), - level: "ERROR".to_string(), - category: "sync.delete_local".to_string(), - error_code: err_code.to_string(), - message: format!("Delete local {}: {}", path, e), - context_json: Some(serde_json::json!({ "path": path }).to_string()), - }); - } else { - state.file_hashes.remove(path); - let mut parent = full.parent(); - while let Some(p) = parent { - if p != local_root && p.read_dir().map(|mut d| d.next().is_none()).unwrap_or(false) { - let _ = std::fs::remove_dir(p); - parent = p.parent(); - } else { - break; - } - } - } - } else { - state.file_hashes.remove(path); - } - done += 1; - } - - let remaining_local: HashSet = current_local.difference(&to_del_local_set).cloned().collect(); - let remaining_remote: HashSet = current_remote.difference(&to_del_remote_set).cloned().collect(); - - let mut to_download: Vec = current_remote .difference(¤t_local) .filter(|p| !is_ignored(p)) @@ -501,6 +426,85 @@ pub fn run_sync(client: &mut ApiClient, local_root: &Path) -> Result<(u64, u64, .map(|(path, _)| path.clone()) .collect(); + let total_work = (to_del_remote.len() + to_del_local.len() + to_download.len() + to_upload.len()) as u64; + let mut done = 0u64; + + log::info!( + "Sync plan: {} to_download, {} to_upload, {} delete_server, {} delete_local", + to_download.len(), + to_upload.len(), + to_del_remote.len(), + to_del_local.len() + ); + + for path in &to_del_remote { + set_progress("delete_server", done, total_work); + if let Err(e) = client.delete_file(path) { + let err_code = classify_error(&e); + enqueue_diagnostic_event(crate::api::DiagnosticEventPayload { + trace_id: Some(trace_id.clone()), + created_at: Some(chrono::Utc::now().to_rfc3339()), + client_type: config::get_client_type().to_string(), + device_name: config::get_device_name(), + level: "ERROR".to_string(), + category: "sync.delete".to_string(), + error_code: err_code.to_string(), + message: format!("Delete server {}: {}", path, e), + context_json: Some(serde_json::json!({ "path": path }).to_string()), + }); + return Err(format!("Delete server {}: {}", path, e)); + } + state.file_hashes.remove(path); + done += 1; + } + let mut failed_local_deletions: HashSet = HashSet::new(); + for path in &to_del_local { + set_progress("delete_local", done, total_work); + let full = match safe_local_path(local_root, path) { + Some(p) => p, + None => { + log::warn!("delete_local: skipping unsafe path: {}", path); + done += 1; + continue; + } + }; + if full.exists() && full.is_file() { + if let Err(e) = std::fs::remove_file(&full) { + log::warn!("Delete local {}: {}, keeping in synced state to avoid resurrection", path, e); + failed_local_deletions.insert(path.clone()); + let err_code = classify_error(&e.to_string()); + enqueue_diagnostic_event(crate::api::DiagnosticEventPayload { + trace_id: Some(trace_id.clone()), + created_at: Some(chrono::Utc::now().to_rfc3339()), + client_type: config::get_client_type().to_string(), + device_name: config::get_device_name(), + level: "ERROR".to_string(), + category: "sync.delete_local".to_string(), + error_code: err_code.to_string(), + message: format!("Delete local {}: {}", path, e), + context_json: Some(serde_json::json!({ "path": path }).to_string()), + }); + } else { + state.file_hashes.remove(path); + let mut parent = full.parent(); + while let Some(p) = parent { + if p != local_root && p.read_dir().map(|mut d| d.next().is_none()).unwrap_or(false) { + let _ = std::fs::remove_dir(p); + parent = p.parent(); + } else { + break; + } + } + } + } else { + state.file_hashes.remove(path); + } + done += 1; + } + + let remaining_local: HashSet = current_local.difference(&to_del_local_set).cloned().collect(); + let remaining_remote: HashSet = current_remote.difference(&to_del_remote_set).cloned().collect(); + let to_download_set: HashSet = to_download.iter().cloned().collect(); let to_upload_set: HashSet = to_upload.iter().cloned().collect(); let mut base_synced: HashSet = remaining_local @@ -512,14 +516,6 @@ pub fn run_sync(client: &mut ApiClient, local_root: &Path) -> Result<(u64, u64, base_synced.insert(path.clone()); } - log::info!( - "Sync plan: {} to_download, {} to_upload, {} delete_server, {} delete_local", - to_download.len(), - to_upload.len(), - to_del_remote.len(), - to_del_local.len() - ); - let mut bytes_downloaded = 0u64; let mut completed_downloads: HashSet = HashSet::new(); let mut skipped_downloads: HashSet = HashSet::new(); @@ -881,6 +877,68 @@ mod tests { assert!(!new_synced.contains("failed_upload.txt")); } + #[test] + fn test_total_work_includes_modified_files_not_just_set_differences() { + // Scenario: A file exists both locally and remotely, but was modified locally. + let current_local: HashSet = ["existing.txt".to_string()].into_iter().collect(); + let current_remote: HashSet = ["existing.txt".to_string()].into_iter().collect(); + let to_del_remote: Vec = Vec::new(); + let to_del_local: Vec = Vec::new(); + + // Old calculation: + let old_total_work = to_del_remote.len() + to_del_local.len() + + current_remote.difference(¤t_local).filter(|p| !is_ignored(p)).count() + + current_local.difference(¤t_remote).filter(|p| !is_ignored(p)).count(); + assert_eq!( + old_total_work, 0, + "Old calculation missed modified files because their path exists in both local and remote sets" + ); + + // In reality, to_upload plans the modified file: + let to_download: Vec = Vec::new(); + let to_upload: Vec = vec!["existing.txt".to_string()]; + + // New calculation: + let new_total_work = (to_del_remote.len() + to_del_local.len() + to_download.len() + to_upload.len()) as u64; + assert_eq!(new_total_work, 1, "New calculation must include all planned transfers in total_work"); + } + + #[test] + fn test_total_work_accurate_in_mixed_scenario() { + // 1 server deletion, 1 local deletion, 1 new download, 1 modified download, 1 new upload, 1 modified upload + let to_del_remote = vec!["deleted_locally.txt".to_string()]; + let to_del_local = vec!["deleted_server.txt".to_string()]; + + let current_local: HashSet = [ + "modified_remote.txt".to_string(), + "new_local.txt".to_string(), + "modified_local.txt".to_string(), + ] + .into_iter() + .collect(); + + let current_remote: HashSet = [ + "new_remote.txt".to_string(), + "modified_remote.txt".to_string(), + "modified_local.txt".to_string(), + ] + .into_iter() + .collect(); + + let to_download = vec!["new_remote.txt".to_string(), "modified_remote.txt".to_string()]; + let to_upload = vec!["new_local.txt".to_string(), "modified_local.txt".to_string()]; + + let old_total_work = to_del_remote.len() + to_del_local.len() + + current_remote.difference(¤t_local).filter(|p| !is_ignored(p)).count() + + current_local.difference(¤t_remote).filter(|p| !is_ignored(p)).count(); + // Old calculation only saw 1 + 1 + 1 (new_remote) + 1 (new_local) = 4 + assert_eq!(old_total_work, 4); + + // New calculation sees all 6 operations: + let new_total_work = (to_del_remote.len() + to_del_local.len() + to_download.len() + to_upload.len()) as u64; + assert_eq!(new_total_work, 6); + } + #[test] fn test_error_classification() { assert_eq!(classify_error("524 Gateway Timeout"), "CLOUDFLARE_524"); diff --git a/openspec/changes/fix-sync-progress-total-work-calculation/.openspec.yaml b/openspec/changes/fix-sync-progress-total-work-calculation/.openspec.yaml new file mode 100644 index 0000000..9076a66 --- /dev/null +++ b/openspec/changes/fix-sync-progress-total-work-calculation/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-10-20 diff --git a/openspec/changes/fix-sync-progress-total-work-calculation/design.md b/openspec/changes/fix-sync-progress-total-work-calculation/design.md new file mode 100644 index 0000000..ffde43c --- /dev/null +++ b/openspec/changes/fix-sync-progress-total-work-calculation/design.md @@ -0,0 +1,36 @@ +## Context + +See `proposal.md` for motivation. In `client-tauri/src-tauri/src/sync.rs`, the synchronization engine coordinates four distinct execution phases: +1. `delete_server` +2. `delete_local` +3. `download` +4. `upload` + +Previously, `total_work` was estimated before `to_download` and `to_upload` were built using only `current_remote \ current_local` and `current_local \ current_remote`. Because modified files already exist in both sets, they were excluded from the work estimate, causing progress undercounting or overflow during transfers. + +## Goals / Non-Goals + +**Goals:** +- Construct the transfer candidate lists (`to_download` and `to_upload`) prior to executing file operations so the complete sync plan is known. +- Set `total_work = (to_del_remote.len() + to_del_local.len() + to_download.len() + to_upload.len()) as u64`. +- Log the complete sync plan upfront before executing deletions. +- Keep progress reporting strictly bounded (`done <= total_work`) and monotonic. +- Maintain existing 3-way hash reconciliation, clock skew resilience, and deletion failure handling. + +**Non-Goals:** +- Modifying UI components in `client-tauri/src/Settings.tsx` (which already render `current / total` and calculate percentage). +- Changing sync conflict resolution rules or hash algorithms. + +## Decisions + +### Decision 1: Compute full sync plan upfront before executing deletions +- **Rationale**: Evaluating `to_download` and `to_upload` relies only on read operations (`safe_local_path`, file hashes, and remote item listings). Deletion targets are already partitioned into `to_del_remote_set` and `to_del_local_set`, which both candidate lists filter out. Constructing them upfront provides an exact total work item count and allows logging the plan before any mutations occur. +- **Alternatives considered**: + - Dynamically incrementing `total_work` when encountering modified files: causes the progress bar to regress or jump unexpectedly, providing a confusing user experience. + +### Decision 2: Maintain delayed base_synced resolution for failed local deletions +- **Rationale**: If a local file scheduled for deletion cannot be deleted (e.g. file lock or permissions), it is added to `failed_local_deletions` and preserved in `base_synced` to prevent resurrection. We maintain this flow by calculating `base_synced` after local deletions complete. + +## Risks / Trade-offs + +- [Risk] File deleted or modified by another application between planning and transfer execution → Mitigation: Transfer loops already handle missing files gracefully (`safe_local_path`, `full.exists()`, and 404 response handling), recording skipped transfers and warning summaries without crashing. diff --git a/openspec/changes/fix-sync-progress-total-work-calculation/proposal.md b/openspec/changes/fix-sync-progress-total-work-calculation/proposal.md new file mode 100644 index 0000000..14aa7a8 --- /dev/null +++ b/openspec/changes/fix-sync-progress-total-work-calculation/proposal.md @@ -0,0 +1,26 @@ +## Why + +During desktop client synchronization, `total_work` was pre-calculated using path set differences (`current_remote \ current_local` and `current_local \ current_remote`), which only accounts for newly added files and completely ignores modified existing files that need to be downloaded or uploaded. Consequently, whenever existing files are modified, `total_work` is undercounted (or evaluates to 0 when only existing files change), resulting in suppressed progress bars, division errors, and `done` exceeding `total_work` (reporting over 100% progress like "upload 3 / 1"). + +## What Changes + +- Calculate the complete sync transfer sets (`to_download` and `to_upload`) prior to executing sync operations. +- Accurately determine `total_work` as `(to_del_remote.len() + to_del_local.len() + to_download.len() + to_upload.len()) as u64`. +- Log the complete sync plan upfront before any destructive or mutating operations begin. +- Ensure monotonic, bounded progress reporting (`done <= total_work`) during all sync phases. +- Add unit tests verifying `total_work` accuracy when modified existing files are present. + +## Capabilities + +### New Capabilities + + + +### Modified Capabilities + +- `desktop-client`: Adds a requirement for accurate synchronization progress accounting so that transfers of modified files are included in total progress and progress counts never exceed the total planned items. + +## Impact + +- `client-tauri/src-tauri/src/sync.rs`: Sync planning order and progress calculations. +- Desktop UI (`Settings.tsx`): Receives accurate `current` and `total` progress events across deletions, downloads, and uploads. diff --git a/openspec/changes/fix-sync-progress-total-work-calculation/specs/desktop-client/spec.md b/openspec/changes/fix-sync-progress-total-work-calculation/specs/desktop-client/spec.md new file mode 100644 index 0000000..327d535 --- /dev/null +++ b/openspec/changes/fix-sync-progress-total-work-calculation/specs/desktop-client/spec.md @@ -0,0 +1,16 @@ +## ADDED Requirements + +### Requirement: Comprehensive Sync Progress Accounting +The desktop client synchronization engine SHALL calculate total planned work by accounting for all deletions, downloads (both newly added and modified remote files), and uploads (both newly added and modified local files) before executing file operations, and SHALL report progress where completed count is strictly bounded by total work. + +#### Scenario: Synchronization with modified local files +- **WHEN** existing files are modified locally while no new files are created +- **THEN** total planned work equals the count of modified files to upload and progress reflects actual file upload progress rather than reporting zero total work + +#### Scenario: Synchronization with modified remote files +- **WHEN** existing files are modified on the server +- **THEN** total planned work includes all modified files to download and progress counts do not exceed total work + +#### Scenario: Mixed additions, modifications, and deletions +- **WHEN** a sync run contains a combination of deletions, new additions, and file modifications +- **THEN** total planned work matches the sum of server deletions, local deletions, planned downloads, and planned uploads, and emitted progress remains bounded between 0 and total work diff --git a/openspec/changes/fix-sync-progress-total-work-calculation/tasks.md b/openspec/changes/fix-sync-progress-total-work-calculation/tasks.md new file mode 100644 index 0000000..e6f47d8 --- /dev/null +++ b/openspec/changes/fix-sync-progress-total-work-calculation/tasks.md @@ -0,0 +1,11 @@ +## 1. Upfront Sync Plan & Total Work Calculation + +- [x] 1.1 Move `to_download` and `to_upload` planning ahead of deletion execution in `client-tauri/src-tauri/src/sync.rs` and verify compilation +- [x] 1.2 Calculate `total_work` as `(to_del_remote.len() + to_del_local.len() + to_download.len() + to_upload.len()) as u64` and log the sync plan before deletions begin +- [x] 1.3 Maintain `base_synced` resolution after local deletions to correctly retain any `failed_local_deletions` + +## 2. Regression Testing & Verification + +- [x] 2.1 Add unit test in `sync.rs` verifying that modified files are included in total work and progress does not overflow +- [x] 2.2 Run `cargo test` in `client-tauri/src-tauri` and verify all tests pass +- [x] 2.3 Run full project QA suite via `./scripts/run-qa.sh`