From e8a69a3222ba3f46f2bed97c6b76e19b943cfdac Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Apr 2026 07:22:01 +0000 Subject: [PATCH 1/4] perf(sync): avoid cloning upload payload `SyncAction::Upload` cloned the file bytes solely so it could later read `data.len()` for the sync-state record. Capture the length up front and move the buffer into `put_file`. --- crates/onyx-core/src/sync.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/onyx-core/src/sync.rs b/crates/onyx-core/src/sync.rs index c97f0c6..6bb626d 100644 --- a/crates/onyx-core/src/sync.rs +++ b/crates/onyx-core/src/sync.rs @@ -724,19 +724,20 @@ async fn execute_action( Err(e) => return Err(e.into()), }; let checksum = compute_checksum(&data); + let len = data.len() as u64; if let Some(parent) = path_parent(path) { client.ensure_dir(parent).await?; } report(&format!(" ^ Uploading {}", path)); - client.put_file(path, data.clone()).await?; + client.put_file(path, data).await?; // Record in sync state using local file metadata 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); + sync_state.record_file(path, &checksum, modified.as_deref(), len); } SyncAction::Conflict { path } => { From 6e161ba819b6c01988127cfdc405ee0064bcc86f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Apr 2026 07:22:27 +0000 Subject: [PATCH 2/4] fix(google_tasks): surface metadata write failures `sync_google_workspace` silently dropped errors from `.listdata.json` and `.onyx-workspace.json` atomic writes via `let _ = ...`, so a sync could report `downloaded: N` while the list/workspace ordering had not been persisted. Push those errors into the `errors` vec returned by `GoogleSyncResult` so callers see the failure. --- crates/onyx-core/src/google_tasks.rs | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/crates/onyx-core/src/google_tasks.rs b/crates/onyx-core/src/google_tasks.rs index e13ecd4..54c9722 100644 --- a/crates/onyx-core/src/google_tasks.rs +++ b/crates/onyx-core/src/google_tasks.rs @@ -358,8 +358,15 @@ pub async fn sync_google_tasks( list_meta.task_order = task_order; list_meta.updated_at = Utc::now(); - if let Ok(meta_content) = serde_json::to_string_pretty(&list_meta) { - let _ = atomic_write(&listdata_path, meta_content.as_bytes()); + match serde_json::to_string_pretty(&list_meta) { + Ok(meta_content) => { + if let Err(e) = atomic_write(&listdata_path, meta_content.as_bytes()) { + errors.push(format!("Failed to write metadata for list '{}': {}", gt_list.title, e)); + } + } + Err(e) => { + errors.push(format!("Failed to serialize metadata for list '{}': {}", gt_list.title, e)); + } } } @@ -374,8 +381,15 @@ pub async fn sync_google_tasks( RootMetadata::default() }; root_meta.list_order = new_list_order; - if let Ok(meta_content) = serde_json::to_string_pretty(&root_meta) { - let _ = atomic_write(&root_meta_path, meta_content.as_bytes()); + match serde_json::to_string_pretty(&root_meta) { + Ok(meta_content) => { + if let Err(e) = atomic_write(&root_meta_path, meta_content.as_bytes()) { + errors.push(format!("Failed to write workspace metadata: {}", e)); + } + } + Err(e) => { + errors.push(format!("Failed to serialize workspace metadata: {}", e)); + } } Ok(GoogleSyncResult { downloaded, errors }) From 12adfdc53271d6841e3d9b21b35bd35c04103355 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Apr 2026 07:23:12 +0000 Subject: [PATCH 3/4] refactor(storage): drop unreachable error in dedup loop The dedup loop wrapped its winner in `Option` and then mapped the `None` case to `Error::InvalidData("Empty dedup entries for task")`. That branch is unreachable: `by_id` is built by pushing every entry of `file_tasks` into the vector for its UUID, so every group has at least one entry, and the `len() > 1` branch keeps the first element after `drain(1..)`. Replace the spurious error with `expect` calls that document the invariant and let the dedup loop yield `Task` directly instead of `Option`. --- crates/onyx-core/src/storage.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/crates/onyx-core/src/storage.rs b/crates/onyx-core/src/storage.rs index 7f8bc8b..b8022dd 100644 --- a/crates/onyx-core/src/storage.rs +++ b/crates/onyx-core/src/storage.rs @@ -454,7 +454,9 @@ impl Storage for FileSystemStorage { let mut tasks = Vec::new(); for (_id, entries) in by_id { - let winner = if entries.len() > 1 { + // `by_id` only inserts non-empty groups, so each `entries` has at + // least one element. + let task = if entries.len() > 1 { // Read mtime once per file so sort_by doesn't hit the filesystem // O(n log n) times and can't produce inconsistent orderings if a // file is touched mid-sort. @@ -479,12 +481,14 @@ impl Storage for FileSystemStorage { eprintln!("Warning: failed to remove stale duplicate task file {:?}: {}", stale_path, e); } } - with_mtime.into_iter().next().map(|(_, t, _)| t) + let (_, t, _) = with_mtime.into_iter().next() + .expect("dedup group is non-empty after drain(1..)"); + t } else { - entries.into_iter().next().map(|(_, t)| t) + let (_, t) = entries.into_iter().next() + .expect("dedup group is non-empty"); + t }; - let task = winner - .ok_or_else(|| Error::InvalidData("Empty dedup entries for task".to_string()))?; tasks.push(task); } From c57ffd3f551b338f238e82486ad06ea027d26e22 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Apr 2026 07:23:34 +0000 Subject: [PATCH 4/4] docs(audit): log 2026-04-27 findings --- Audit.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Audit.md b/Audit.md index 03eba91..8831902 100644 --- a/Audit.md +++ b/Audit.md @@ -1,5 +1,13 @@ # Audit Log +## 2026-04-27 + +Found and fixed 3 issues: + +1. **Perf: needless clone of upload payload** (sync.rs:733) — the `SyncAction::Upload` arm read the file into `data`, computed `compute_checksum(&data)`, then called `client.put_file(path, data.clone())`. The clone existed only because the next statement needed `data.len()` for the sync-state record. Captured `data.len() as u64` into `len` first, moved `data` into `put_file`, and used `len` afterwards — one full byte copy avoided per uploaded file. +2. **Bug: Google Tasks sync silently drops metadata-write failures** (google_tasks.rs:361, 377) — both `.listdata.json` and `.onyx-workspace.json` were written via `if let Ok(meta_content) = serde_json::to_string_pretty(...) { let _ = atomic_write(...); }`, so a serialization or atomic-write error returned `Ok(GoogleSyncResult { downloaded: N, errors: [] })` even though list/workspace ordering was never persisted. Both writes now push their errors into the `errors` vec already returned in `GoogleSyncResult`. +3. **Code quality: unreachable dead-error path in storage dedup** (storage.rs:447) — the dedup loop computed `Option` from each `by_id` group and then `ok_or_else(|| Error::InvalidData("Empty dedup entries for task"))?`. `by_id` is only populated by `entry(uuid).or_default().push(entry)`, so every group has ≥1 element and the `None` branch is unreachable. Replaced the `Option`+`?` with direct `expect` calls (one per branch) that document the non-empty invariant; the loop now yields `Task` directly. + ## 2026-04-25 Found and fixed 3 issues: