From a395d11ec71d2a1277bb2cca3edc70cba572dc9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Brandst=C3=A4tter?= Date: Tue, 3 Nov 2026 09:32:39 +0100 Subject: [PATCH] fix(client): record sync summary and flush telemetry on delete failure --- client-tauri/src-tauri/src/config.rs | 3 + client-tauri/src-tauri/src/credentials.rs | 5 +- client-tauri/src-tauri/src/sync.rs | 95 ++++++++++++++++++- .../.openspec.yaml | 2 + .../design.md | 54 +++++++++++ .../proposal.md | 27 ++++++ .../specs/desktop-client/spec.md | 8 ++ .../tasks.md | 14 +++ 8 files changed, 203 insertions(+), 5 deletions(-) create mode 100644 openspec/changes/fix-sync-delete-failure-telemetry-summary/.openspec.yaml create mode 100644 openspec/changes/fix-sync-delete-failure-telemetry-summary/design.md create mode 100644 openspec/changes/fix-sync-delete-failure-telemetry-summary/proposal.md create mode 100644 openspec/changes/fix-sync-delete-failure-telemetry-summary/specs/desktop-client/spec.md create mode 100644 openspec/changes/fix-sync-delete-failure-telemetry-summary/tasks.md diff --git a/client-tauri/src-tauri/src/config.rs b/client-tauri/src-tauri/src/config.rs index 4302d59..5664ee0 100644 --- a/client-tauri/src-tauri/src/config.rs +++ b/client-tauri/src-tauri/src/config.rs @@ -311,6 +311,9 @@ pub fn get_device_name() -> String { .unwrap_or_else(|_| "Desktop-Client".to_string()) } +#[cfg(test)] +pub static ENV_MUTEX: std::sync::Mutex<()> = std::sync::Mutex::new(()); + #[cfg(test)] mod tests { use super::*; diff --git a/client-tauri/src-tauri/src/credentials.rs b/client-tauri/src-tauri/src/credentials.rs index 715bbc7..d411b5d 100644 --- a/client-tauri/src-tauri/src/credentials.rs +++ b/client-tauri/src-tauri/src/credentials.rs @@ -183,11 +183,10 @@ pub fn clear_stored() { #[cfg(test)] mod tests { use super::*; - static TEST_MUTEX: std::sync::Mutex<()> = std::sync::Mutex::new(()); #[test] fn test_store_and_get() { - let _lock = TEST_MUTEX.lock().unwrap(); + let _lock = crate::config::ENV_MUTEX.lock().unwrap(); let temp_dir = std::env::temp_dir().join(format!("brandybox_test_{}", uuid::Uuid::new_v4())); std::env::set_var("BRANDYBOX_CONFIG_DIR", &temp_dir); clear_stored(); @@ -216,7 +215,7 @@ mod tests { #[test] fn test_get_stored_expired_token() { - let _lock = TEST_MUTEX.lock().unwrap(); + let _lock = crate::config::ENV_MUTEX.lock().unwrap(); let temp_dir = std::env::temp_dir().join(format!("brandybox_test_{}", uuid::Uuid::new_v4())); std::env::set_var("BRANDYBOX_CONFIG_DIR", &temp_dir); clear_stored(); diff --git a/client-tauri/src-tauri/src/sync.rs b/client-tauri/src-tauri/src/sync.rs index d2d3b2d..8dcc802 100644 --- a/client-tauri/src-tauri/src/sync.rs +++ b/client-tauri/src-tauri/src/sync.rs @@ -365,6 +365,7 @@ pub fn run_sync(client: &mut ApiClient, local_root: &Path) -> Result<(u64, u64, set_progress("delete_server", done, total_work); if let Err(e) = client.delete_file(path) { let err_code = classify_error(&e); + let err_msg = format!("Delete server {}: {}", path, e); enqueue_diagnostic_event(crate::api::DiagnosticEventPayload { trace_id: Some(trace_id.clone()), created_at: Some(chrono::Utc::now().to_rfc3339()), @@ -373,10 +374,27 @@ pub fn run_sync(client: &mut ApiClient, local_root: &Path) -> Result<(u64, u64, level: "ERROR".to_string(), category: "sync.delete".to_string(), error_code: err_code.to_string(), - message: format!("Delete server {}: {}", path, e), + message: err_msg.clone(), context_json: Some(serde_json::json!({ "path": path }).to_string()), }); - return Err(format!("Delete server {}: {}", path, e)); + enqueue_sync_summary(crate::api::SyncSummaryPayload { + trace_id: trace_id.clone(), + client_type: config::get_client_type().to_string(), + client_version: env!("CARGO_PKG_VERSION").to_string(), + device_name: config::get_device_name(), + started_at: started_at.clone(), + completed_at: Some(chrono::Utc::now().to_rfc3339()), + duration_ms: start_instant.elapsed().as_millis() as i64, + status: "failed".to_string(), + files_scanned: (local_list.len() + remote_list.len()) as i64, + files_uploaded: 0, + files_downloaded: 0, + failure_count: 1, + bytes_transferred: 0, + error_summary_json: Some(serde_json::json!({ "error": err_msg }).to_string()), + }); + let _ = flush_telemetry_queue(client); + return Err(err_msg); } state.file_hashes.remove(path); done += 1; @@ -1108,5 +1126,78 @@ mod tests { state.file_hashes.remove("deleted.txt"); assert_eq!(state.file_hashes.len(), 0); } + + #[test] + fn test_sync_delete_failure_records_summary_and_flushes_telemetry() { + let _lock = crate::config::ENV_MUTEX.lock().unwrap(); + use std::io::{Read, Write}; + use std::net::TcpListener; + use std::sync::atomic::{AtomicBool, Ordering}; + use std::sync::Arc; + + let temp_dir = std::env::temp_dir().join(format!("test_sync_del_tel_{}", uuid::Uuid::new_v4())); + let _ = std::fs::create_dir_all(&temp_dir); + let sync_dir = temp_dir.join("sync_folder"); + let _ = std::fs::create_dir_all(&sync_dir); + let config_dir = temp_dir.join("config"); + let _ = std::fs::create_dir_all(&config_dir); + + std::env::set_var("BRANDYBOX_CONFIG_DIR", config_dir.to_str().unwrap()); + + let mut initial_state = SyncStateFile::default(); + initial_state.paths.push("to_remove.txt".to_string()); + save_sync_state(&initial_state); + + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let port = listener.local_addr().unwrap().port(); + let base_url = format!("http://127.0.0.1:{}", port); + + let telemetry_flushed = Arc::new(AtomicBool::new(false)); + let telemetry_flushed_clone = telemetry_flushed.clone(); + + let server_thread = std::thread::spawn(move || { + for stream in listener.incoming() { + if let Ok(mut s) = stream { + let mut buf = [0u8; 2048]; + let n = s.read(&mut buf).unwrap_or(0); + let req = String::from_utf8_lossy(&buf[..n]); + if req.starts_with("GET /api/files/list") { + let body = r#"[{"path":"to_remove.txt","mtime":100.0,"size":10,"hash":"abc"}]"#; + let resp = format!( + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{}", + body.len(), + body + ); + let _ = s.write_all(resp.as_bytes()); + } else if req.starts_with("DELETE /api/files/delete") { + let resp = "HTTP/1.1 403 Forbidden\r\nContent-Length: 0\r\nConnection: close\r\n\r\n"; + let _ = s.write_all(resp.as_bytes()); + } else if req.starts_with("POST /api/telemetry/events") { + telemetry_flushed_clone.store(true, Ordering::SeqCst); + // Return 500 so queue stays saved on disk for inspection + let resp = "HTTP/1.1 500 Internal Server Error\r\nContent-Length: 0\r\nConnection: close\r\n\r\n"; + let _ = s.write_all(resp.as_bytes()); + break; + } + } + } + }); + + let mut client = ApiClient::new(base_url); + client.set_access_token(Some("dummy_token".to_string())); + + let res = run_sync(&mut client, &sync_dir); + assert!(res.is_err(), "run_sync should return Err on delete failure"); + + let queue = load_telemetry_queue(); + assert_eq!(queue.summaries.len(), 1, "Expected 1 SyncSummaryPayload enqueued on delete failure"); + assert_eq!(queue.summaries[0].status, "failed"); + assert_eq!(queue.summaries[0].failure_count, 1); + assert!(telemetry_flushed.load(Ordering::SeqCst), "Expected flush_telemetry_queue to be called"); + + std::env::remove_var("BRANDYBOX_CONFIG_DIR"); + let _ = server_thread.join(); + let _ = std::fs::remove_dir_all(&temp_dir); + } } diff --git a/openspec/changes/fix-sync-delete-failure-telemetry-summary/.openspec.yaml b/openspec/changes/fix-sync-delete-failure-telemetry-summary/.openspec.yaml new file mode 100644 index 0000000..ac359ee --- /dev/null +++ b/openspec/changes/fix-sync-delete-failure-telemetry-summary/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-11-03 diff --git a/openspec/changes/fix-sync-delete-failure-telemetry-summary/design.md b/openspec/changes/fix-sync-delete-failure-telemetry-summary/design.md new file mode 100644 index 0000000..129b001 --- /dev/null +++ b/openspec/changes/fix-sync-delete-failure-telemetry-summary/design.md @@ -0,0 +1,54 @@ +## Context + +In Brandy Box desktop client, `run_sync` in `client-tauri/src-tauri/src/sync.rs` orchestrates the two-way sync loop between the local sync directory and the backend server. +Telemetry tracking is split into two models: +1. `DiagnosticEvent`: Granular diagnostic events detailing specific errors (e.g. `sync.list`, `sync.delete`, `sync.download`, `sync.upload`). +2. `SyncSummary`: Per-sync-cycle overview documenting `status` (`"ok"`, `"warning"`, `"failed"`), duration, file counts scanned/transferred, failure count, and error summary. + +During `run_sync`: +- If `client.list_files()` fails (lines 283–316), both a `DiagnosticEvent` and a `SyncSummary` with `status: "failed"` are enqueued, and `flush_telemetry_queue(client)` is invoked before returning `Err(e)`. +- If sync finishes without early error (lines 728–744), a `SyncSummary` with `status: "ok"` or `"warning"` is enqueued and `flush_telemetry_queue(client)` is invoked. +- However, if `client.delete_file(path)` fails during remote deletion (lines 364–380), only a `DiagnosticEvent` was queued. `enqueue_sync_summary` and `flush_telemetry_queue` were omitted before returning `Err`. + +## Goals / Non-Goals + +**Goals:** +- Ensure that any early exit from `run_sync` due to remote deletion failure records a `SyncSummaryPayload` with `status: "failed"` and `failure_count: 1`. +- Ensure `flush_telemetry_queue(client)` is invoked prior to returning `Err` so telemetry is dispatched immediately or saved to the offline buffer. +- Add regression testing to verify that delete failure generates the expected summary and triggers telemetry queue flushing. + +**Non-Goals:** +- Changing retry semantics of `ApiClient::delete_file`. +- Modifying how callers in `lib.rs` handle the returned `Err`. + +## Decisions + +### Decision 1: Populate `SyncSummaryPayload` on `delete_file` Failure +When `client.delete_file(path)` returns `Err(e)`: +- Create error message: `let err_msg = format!("Delete server {}: {}", path, e);` +- Enqueue diagnostic event (category `"sync.delete"`, error code from `classify_error(&e)`). +- Enqueue sync summary: + - `trace_id`: `trace_id.clone()` + - `client_type`: `config::get_client_type().to_string()` + - `client_version`: `env!("CARGO_PKG_VERSION").to_string()` + - `device_name`: `config::get_device_name()` + - `started_at`: `started_at.clone()` + - `completed_at`: `Some(chrono::Utc::now().to_rfc3339())` + - `duration_ms`: `start_instant.elapsed().as_millis() as i64` + - `status`: `"failed".to_string()` + - `files_scanned`: `(local_list.len() + remote_list.len()) as i64` + - `files_uploaded`: 0 + - `files_downloaded`: 0 + - `failure_count`: 1 + - `bytes_transferred`: 0 + - `error_summary_json`: `Some(serde_json::json!({ "error": err_msg }).to_string())` +- Call `let _ = flush_telemetry_queue(client);` +- Return `Err(err_msg)` + +*Rationale*: +Mirrors the pattern established in `list_files` error handling and ensures symmetry across failure exit points. + +## Risks / Trade-offs + +- **[Risk]** If backend is unreachable, `flush_telemetry_queue` will fail. + - **Mitigation**: `flush_telemetry_queue` already handles network failure gracefully by retaining items in the offline queue file on disk (`config::get_telemetry_queue_path()`) and logging a warning. The next sync attempt flushes them. diff --git a/openspec/changes/fix-sync-delete-failure-telemetry-summary/proposal.md b/openspec/changes/fix-sync-delete-failure-telemetry-summary/proposal.md new file mode 100644 index 0000000..d36764c --- /dev/null +++ b/openspec/changes/fix-sync-delete-failure-telemetry-summary/proposal.md @@ -0,0 +1,27 @@ +## Why + +In `client-tauri/src-tauri/src/sync.rs`, the desktop sync engine synchronizes file deletions by identifying files removed locally and deleting them from the remote server via `client.delete_file(path)`. If `client.delete_file(path)` returns an error (e.g. HTTP 403, 500, network disconnect), the loop records a `DiagnosticEventPayload` and immediately terminates `run_sync` by returning `Err(format!("Delete server {}: {}", path, e))`. + +However, unlike the `list_files` error handler (which properly enqueues a `SyncSummaryPayload` with status `"failed"` and flushes the telemetry queue via `flush_telemetry_queue`), the `delete_file` error path: +1. Omits calling `enqueue_sync_summary`, meaning no `SyncSummary` entry is ever created or persisted for this failed synchronization run. +2. Omits calling `flush_telemetry_queue(client)`, leaving the queued `DiagnosticEvent` unflushed in the offline queue file. +3. Leaves server administrators and diagnostics tools (`brandybox doctor`) completely blind to the failed sync run in aggregate metrics and summary tables. + +## What Changes + +- In `client-tauri/src-tauri/src/sync.rs`, when `client.delete_file(path)` fails: + - Enqueue a `SyncSummaryPayload` with `status: "failed"`, `failure_count: 1`, elapsed `duration_ms`, `files_scanned: (local_list.len() + remote_list.len()) as i64`, and an `error_summary_json` detailing the delete failure. + - Call `flush_telemetry_queue(client)` before returning the error so that both the diagnostic event and the sync summary are sent to the backend immediately (or retained in the offline buffer if offline). +- Add regression unit test in `client-tauri/src-tauri/src/sync.rs` asserting that when server file deletion fails during synchronization, a `"failed"` `SyncSummaryPayload` is queued and telemetry flushing is triggered. + +## Capabilities + +### Modified Capabilities +- `desktop-client`: Require the desktop client synchronization engine to generate a failed `SyncSummaryPayload` and flush telemetry when a remote file deletion fails during a synchronization cycle. + +## Impact + +- **Affected code**: `client-tauri/src-tauri/src/sync.rs` +- **APIs**: No changes to API schemas; existing `/api/telemetry/events` payload is properly populated. +- **Dependencies**: No new dependencies. +- **Behavior**: Synchronization runs that fail during remote file deletion now reliably emit `SyncSummary` records with status `"failed"`, allowing server telemetry and `brandybox doctor` to reflect sync failures accurately. diff --git a/openspec/changes/fix-sync-delete-failure-telemetry-summary/specs/desktop-client/spec.md b/openspec/changes/fix-sync-delete-failure-telemetry-summary/specs/desktop-client/spec.md new file mode 100644 index 0000000..78837eb --- /dev/null +++ b/openspec/changes/fix-sync-delete-failure-telemetry-summary/specs/desktop-client/spec.md @@ -0,0 +1,8 @@ +## ADDED Requirements + +### Requirement: Sync Summary Emission on Remote Deletion Failure +When the desktop client synchronization engine encounters an error while deleting a remote file on the server, it SHALL record a `SyncSummary` event with status `"failed"`, document the failure count, error summary, and scan metrics, and attempt to flush the telemetry queue before returning the error. + +#### Scenario: Server file deletion returns an error +- **WHEN** a local file was deleted and scheduled for remote deletion via `client.delete_file(path)`, but the server request fails with an error (e.g. HTTP 403, 500, or network disconnect) +- **THEN** the sync engine records a `DiagnosticEvent` with category `"sync.delete"`, records a `SyncSummary` with `status: "failed"` and `failure_count: 1`, flushes the telemetry queue, and returns the error to the caller. diff --git a/openspec/changes/fix-sync-delete-failure-telemetry-summary/tasks.md b/openspec/changes/fix-sync-delete-failure-telemetry-summary/tasks.md new file mode 100644 index 0000000..010de16 --- /dev/null +++ b/openspec/changes/fix-sync-delete-failure-telemetry-summary/tasks.md @@ -0,0 +1,14 @@ +## 1. Unit Testing and Reproduction + +- [x] 1.1 Add unit test in `client-tauri/src-tauri/src/sync.rs` asserting that when server file deletion fails during `run_sync`, a `SyncSummary` with `status: "failed"` is enqueued and telemetry flush is executed. +- [x] 1.2 Verify the reproduction test fails or demonstrates missing summary before the fix. + +## 2. Desktop Client Implementation + +- [x] 2.1 Update `run_sync` in `client-tauri/src-tauri/src/sync.rs` on `client.delete_file` error to call `enqueue_sync_summary` with `status: "failed"` and invoke `flush_telemetry_queue(client)`. +- [x] 2.2 Verify that all tests pass. + +## 3. Verification and Quality Assurance + +- [x] 3.1 Run `cargo test` in `client-tauri/src-tauri` and verify all tests pass. +- [x] 3.2 Run repo-wide QA checks and confirm no regressions.