diff --git a/proto/sync.proto b/proto/sync.proto index d26c824..c790ad2 100755 --- a/proto/sync.proto +++ b/proto/sync.proto @@ -554,6 +554,7 @@ message RenameFileRequest { optional string operation_type = 11; // e.g., "RENAME" or "MOVE" ConflictInfo.ResolutionStrategy conflict_resolution = 12; // How to resolve conflicts (default: MANUAL) google.protobuf.Timestamp updated_time = 13; // Client's last known update time for timestamp-based conflict resolution + optional bool overwrite = 14; // If true, tombstone conflicting target before rename/move } // NEW: Rename file response diff --git a/src/handlers/file/batch.rs b/src/handlers/file/batch.rs index 120d0b5..15f6645 100644 --- a/src/handlers/file/batch.rs +++ b/src/handlers/file/batch.rs @@ -179,11 +179,13 @@ async fn process_upload_operation( } } Err(e) => { - error!("Upload operation {} failed: {}", operation_id, e); + let error_msg = e.message(); + error!("Upload operation {} failed: {}", operation_id, error_msg); + let error_code = extract_error_code_from_message(error_msg); create_error_result( operation_id, - &format!("Upload failed: {}", e), - ErrorCode::StorageFailed, + &format!("Upload failed: {}", error_msg), + error_code, ) } } @@ -211,11 +213,13 @@ async fn process_delete_operation( } } Err(e) => { - error!("Delete operation {} failed: {}", operation_id, e); + let error_msg = e.message(); + error!("Delete operation {} failed: {}", operation_id, error_msg); + let error_code = extract_error_code_from_message(error_msg); create_error_result( operation_id, - &format!("Delete failed: {}", e), - ErrorCode::StorageFailed, + &format!("Delete failed: {}", error_msg), + error_code, ) } } @@ -243,11 +247,13 @@ async fn process_rename_operation( } } Err(e) => { - error!("Rename operation {} failed: {}", operation_id, e); + let error_msg = e.message(); + error!("Rename operation {} failed: {}", operation_id, error_msg); + let error_code = extract_error_code_from_message(error_msg); create_error_result( operation_id, - &format!("Rename failed: {}", e), - ErrorCode::StorageFailed, + &format!("Rename failed: {}", error_msg), + error_code, ) } } @@ -266,3 +272,40 @@ fn create_error_result( result: None, } } + +/// Extract appropriate ErrorCode from error message for client-side retry logic classification +fn extract_error_code_from_message(msg: &str) -> ErrorCode { + let msg_lower = msg.to_lowercase(); + + // Permanent errors - client should not retry + if msg_lower.contains("permission denied") || msg_lower.contains("access denied") { + ErrorCode::AuthFailed + } else if msg_lower.contains("already deleted") { + ErrorCode::FileAlreadyDeleted + } else if msg_lower.contains("path conflict") || msg_lower.contains("already exists") { + ErrorCode::PathConflict + } else if msg_lower.contains("quota") || msg_lower.contains("storage limit") { + ErrorCode::QuotaExceeded + } else if msg_lower.contains("invalid") || msg_lower.contains("validation") { + ErrorCode::InvalidRequest + } + // Conflict errors - client should sync and retry + else if msg_lower.contains("revision") || msg_lower.contains("conflict") { + ErrorCode::RevisionConflict + } + // File errors + else if msg_lower.contains("not found") { + ErrorCode::FileNotFound + } + // Schema/DB errors - permanent + else if msg_lower.contains("schema") + || msg_lower.contains("varbinary") + || msg_lower.contains("varchar") + { + ErrorCode::DbSchemaError + } + // Default: transient error - client can retry with backoff + else { + ErrorCode::StorageFailed + } +} diff --git a/src/handlers/file/delete.rs b/src/handlers/file/delete.rs index 560d09b..cb6128a 100644 --- a/src/handlers/file/delete.rs +++ b/src/handlers/file/delete.rs @@ -273,11 +273,20 @@ pub async fn handle_delete_file( })) } Ok(DeleteFileResult::AlreadyDeleted) => { - info!("File already deleted: file_id={}", file_id); - Ok(Response::new(response::file_delete_error_with_code( - "File already deleted", - ErrorCode::FileNotFound, - ))) + // Idempotent success: if the file is already deleted, the client's goal is achieved + info!( + "File already deleted (idempotent success): file_id={}", + file_id + ); + Ok(Response::new(DeleteFileResponse { + success: true, + return_message: "File already deleted".to_string(), + delete_record_id: 0, + new_revision: 0, + conflict: None, + error_code: ErrorCode::Success as i32, + current_revision: 0, + })) } Err(e) => { // Determine error category for logging and client classification diff --git a/src/handlers/file/rename.rs b/src/handlers/file/rename.rs index 5f20762..8d737ed 100644 --- a/src/handlers/file/rename.rs +++ b/src/handlers/file/rename.rs @@ -280,18 +280,18 @@ pub async fn handle_rename_file( } // 6. Broadcast rename notification to other devices - // Fix P0 #3: Include updated file info with Server IDs + // Fix P0 #3: Use SERVER IDs for cross-watcher MOVE let mut broadcast_file_info = file_info.clone(); broadcast_file_info.file_path = req.new_file_path.clone(); broadcast_file_info.revision = new_revision; broadcast_file_info.updated_time.seconds = req.timestamp; broadcast_file_info.updated_time.nanos = 0; - if let Some(gid) = new_group_id { - broadcast_file_info.group_id = gid; - } - if let Some(wid) = new_watcher_id { - broadcast_file_info.watcher_id = wid; + // CRITICAL: For cross-watcher moves, use SERVER IDs (not client IDs) + // so other devices can properly locate the file + if is_cross_watcher_move { + broadcast_file_info.group_id = server_group_id; + broadcast_file_info.watcher_id = server_watcher_id; } if let Err(e) = handler diff --git a/src/server/service.rs b/src/server/service.rs index 97a3a81..1c6dad4 100644 --- a/src/server/service.rs +++ b/src/server/service.rs @@ -1675,7 +1675,11 @@ impl SyncService for SyncServiceImpl { quota_reset_date: summary.quota_reset_date.map(|d| { use prost_types::Timestamp; Timestamp { - seconds: d.and_hms_opt(0, 0, 0).unwrap().and_utc().timestamp(), + seconds: d + .and_hms_opt(0, 0, 0) + .expect("Valid time: midnight") + .and_utc() + .timestamp(), nanos: 0, } }), @@ -1752,7 +1756,11 @@ impl SyncService for SyncServiceImpl { period_start: None, period_end: None, quota_reset_date: summary.quota_reset_date.map(|d| Timestamp { - seconds: d.and_hms_opt(0, 0, 0).unwrap().and_utc().timestamp(), + seconds: d + .and_hms_opt(0, 0, 0) + .expect("Valid time: midnight") + .and_utc() + .timestamp(), nanos: 0, }), is_transfer_exceeded: summary.transfer_percentage >= 100.0, diff --git a/src/services/usage_service.rs b/src/services/usage_service.rs index e65010d..d929406 100644 --- a/src/services/usage_service.rs +++ b/src/services/usage_service.rs @@ -275,13 +275,13 @@ impl UsageService { )); // Update warning timestamp - let _ = self + if let Some(mysql_storage) = self .storage .as_any() .downcast_ref::() - .unwrap() - .update_last_warning(account_hash) - .await; + { + let _ = mysql_storage.update_last_warning(account_hash).await; + } } Ok((true, None, warnings)) diff --git a/src/services/version_service.rs b/src/services/version_service.rs index 1f311fd..4fa2027 100644 --- a/src/services/version_service.rs +++ b/src/services/version_service.rs @@ -219,7 +219,7 @@ impl VersionService for VersionServiceImpl { .map_err(|e| AppError::storage(format!("Failed to get file history: {}", e)))?; let total_versions = files.len() as i32; - let has_more = limit.is_some() && files.len() == limit.unwrap() as usize; + let has_more = limit.map_or(false, |l| files.len() == l as usize); // Convert to FileVersionInfo let versions: Vec = files diff --git a/src/storage/mysql.rs b/src/storage/mysql.rs index 8f143f5..f0279f4 100644 --- a/src/storage/mysql.rs +++ b/src/storage/mysql.rs @@ -1269,19 +1269,19 @@ END"#; account_hash: &str, group_id: i32, watcher_id: i32, - data: Vec, + data: &[u8], ) -> String { let cfg = crate::server::app_state::AppState::get_config(); if let Some(kv) = cfg.server_encode_key.as_ref() { if kv.len() == 32 { let key: &[u8; 32] = kv.as_slice().try_into().expect("len checked"); let aad = format!("{}:{}:{}", account_hash, group_id, watcher_id); - if let Ok(pt) = crate::utils::crypto::aead_decrypt(key, &data, aad.as_bytes()) { + if let Ok(pt) = crate::utils::crypto::aead_decrypt(key, data, aad.as_bytes()) { return String::from_utf8_lossy(&pt).to_string(); } } } - String::from_utf8_lossy(&data).to_string() + String::from_utf8_lossy(data).to_string() } pub async fn migrate_encrypt_paths(&self, batch_size: usize) -> Result { @@ -1332,8 +1332,14 @@ END"#; (maybe_plain_path.as_ref(), maybe_plain_name.as_ref()) { // Already encrypted; recompute indexes from decrypted plaintext - let plain_path = - String::from_utf8_lossy(maybe_plain_path.as_ref().unwrap()).to_string(); + let plain_path = match maybe_plain_path.as_ref() { + Some(bytes) => String::from_utf8_lossy(bytes).to_string(), + None => { + return Err(StorageError::Database( + "Missing plain_path during encryption migration".into(), + )) + } + }; let salt = crate::utils::crypto::derive_salt(key, "meta-index", &account_hash); ( file_path_b, @@ -2000,9 +2006,11 @@ impl Storage for MySqlStorage { )) })?; - // Tombstone row uses next revision, new active uses next+1 to avoid UNIQUE(file_id, revision) conflicts + // Both tombstone and new active row share the same revision (current+1) + // This represents a single logical operation (rename/move) with one revision increment + // Note: No UNIQUE(file_id, revision) constraint exists, so this is safe let tombstone_revision = current_revision + 1; - let new_active_revision = tombstone_revision + 1; + let new_active_revision = current_revision + 1; sqlx::query( r#"INSERT INTO files ( diff --git a/src/storage/mysql_file.rs b/src/storage/mysql_file.rs index 17107c4..5fde76a 100644 --- a/src/storage/mysql_file.rs +++ b/src/storage/mysql_file.rs @@ -344,13 +344,13 @@ impl MySqlFileExt for MySqlStorage { let file_info = FileInfo { file_id, - filename: self.decrypt_text(&account_hash, group_id, watcher_id, filename_b), + filename: self.decrypt_text(&account_hash, group_id, watcher_id, &filename_b), file_hash, device_hash, group_id, watcher_id, is_encrypted: false, - file_path: self.decrypt_text(&account_hash, group_id, watcher_id, file_path_b), + file_path: self.decrypt_text(&account_hash, group_id, watcher_id, &file_path_b), updated_time: timestamp, revision, account_hash, @@ -424,13 +424,13 @@ impl MySqlFileExt for MySqlStorage { let file_info = FileInfo { file_id, - filename: self.decrypt_text(&account_hash, group_id, watcher_id, filename_b), + filename: self.decrypt_text(&account_hash, group_id, watcher_id, &filename_b), file_hash, device_hash, group_id, watcher_id, is_encrypted: false, - file_path: self.decrypt_text(&account_hash, group_id, watcher_id, file_path_b), + file_path: self.decrypt_text(&account_hash, group_id, watcher_id, &file_path_b), updated_time: timestamp, revision, account_hash, @@ -518,13 +518,13 @@ impl MySqlFileExt for MySqlStorage { let file_info = FileInfo { file_id, - filename: self.decrypt_text(&account_hash, group_id, watcher_id, filename_b), + filename: self.decrypt_text(&account_hash, group_id, watcher_id, &filename_b), file_hash, device_hash, group_id, watcher_id, is_encrypted: false, - file_path: self.decrypt_text(&account_hash, group_id, watcher_id, file_path_b), + file_path: self.decrypt_text(&account_hash, group_id, watcher_id, &file_path_b), updated_time: timestamp, revision, account_hash, @@ -593,13 +593,13 @@ impl MySqlFileExt for MySqlStorage { let file_info = FileInfo { file_id, - filename: self.decrypt_text(&account_hash, group_id, watcher_id, filename_b), + filename: self.decrypt_text(&account_hash, group_id, watcher_id, &filename_b), file_hash, device_hash, group_id, watcher_id, is_encrypted: false, - file_path: self.decrypt_text(&account_hash, group_id, watcher_id, file_path_b), + file_path: self.decrypt_text(&account_hash, group_id, watcher_id, &file_path_b), updated_time: timestamp, revision, account_hash, @@ -701,8 +701,8 @@ impl MySqlFileExt for MySqlStorage { let size: u64 = row.try_get("size").unwrap_or(0); let key_id_opt: Option = row.try_get("key_id").ok(); - let file_path = self.decrypt_text(&acc_hash, group_id, watcher_id, file_path_b); - let filename = self.decrypt_text(&acc_hash, group_id, watcher_id, filename_b); + let file_path = self.decrypt_text(&acc_hash, group_id, watcher_id, &file_path_b); + let filename = self.decrypt_text(&acc_hash, group_id, watcher_id, &filename_b); let timestamp = prost_types::Timestamp { seconds: updated_ts.unwrap_or(0), @@ -774,19 +774,20 @@ impl MySqlFileExt for MySqlStorage { )) })?; - if row_opt.is_none() { - debug!( - "File to delete does not exist or does not belong to the user: file_id={}, account_hash={}", - file_id, account_hash - ); - return Err(StorageError::NotFound(format!( - "Cannot find file: {}", - file_id - ))); - } - use sqlx::Row; - let row = row_opt.unwrap(); + let row = match row_opt { + Some(r) => r, + None => { + debug!( + "File to delete does not exist or does not belong to the user: file_id={}, account_hash={}", + file_id, account_hash + ); + return Err(StorageError::NotFound(format!( + "Cannot find file: {}", + file_id + ))); + } + }; let current_revision: i64 = row.try_get("revision").unwrap_or(0); // Validate revision if client provided one @@ -821,9 +822,9 @@ impl MySqlFileExt for MySqlStorage { // Decrypt for logging purposes only let file_path_for_log = - self.decrypt_text(account_hash, group_id, watcher_id, file_path_bytes.clone()); + self.decrypt_text(account_hash, group_id, watcher_id, &file_path_bytes); let filename_for_log = - self.decrypt_text(account_hash, group_id, watcher_id, filename_bytes.clone()); + self.decrypt_text(account_hash, group_id, watcher_id, &filename_bytes); debug!( "Processing file deletion: file_id={}, file_path={}, filename={}, current_revision={}, new_revision={}", file_id, file_path_for_log, filename_for_log, current_revision, new_revision @@ -959,9 +960,9 @@ impl MySqlFileExt for MySqlStorage { // Decrypt for logging let file_path_for_log = - self.decrypt_text(account_hash, group_id, watcher_id, file_path_bytes.clone()); + self.decrypt_text(account_hash, group_id, watcher_id, &file_path_bytes); let filename_for_log = - self.decrypt_text(account_hash, group_id, watcher_id, filename_bytes.clone()); + self.decrypt_text(account_hash, group_id, watcher_id, &filename_bytes); info!( "Restoring file: path={}, filename={}, revision={}", @@ -1022,8 +1023,8 @@ impl MySqlFileExt for MySqlStorage { ); // Return FileInfo for the restored file - let file_path_str = self.decrypt_text(account_hash, group_id, watcher_id, file_path_bytes); - let filename_str = self.decrypt_text(account_hash, group_id, watcher_id, filename_bytes); + let file_path_str = self.decrypt_text(account_hash, group_id, watcher_id, &file_path_bytes); + let filename_str = self.decrypt_text(account_hash, group_id, watcher_id, &filename_bytes); Ok(FileInfo { file_id: restored_file_id, @@ -1102,13 +1103,13 @@ impl MySqlFileExt for MySqlStorage { }; files.push(FileInfo { file_id, - filename: self.decrypt_text(&acc_hash, group_id, watcher_id, filename_b), + filename: self.decrypt_text(&acc_hash, group_id, watcher_id, &filename_b), file_hash, device_hash, group_id, watcher_id, is_encrypted: false, - file_path: self.decrypt_text(&acc_hash, group_id, watcher_id, file_path_b), + file_path: self.decrypt_text(&acc_hash, group_id, watcher_id, &file_path_b), updated_time: timestamp, revision, account_hash: acc_hash, @@ -1185,13 +1186,13 @@ impl MySqlFileExt for MySqlStorage { }; files.push(FileInfo { file_id, - filename: self.decrypt_text(&acc_hash, group_id, watcher_id, filename_b), + filename: self.decrypt_text(&acc_hash, group_id, watcher_id, &filename_b), file_hash, device_hash, group_id, watcher_id, is_encrypted: false, - file_path: self.decrypt_text(&acc_hash, group_id, watcher_id, file_path_b), + file_path: self.decrypt_text(&acc_hash, group_id, watcher_id, &file_path_b), updated_time: timestamp, revision, account_hash: acc_hash, diff --git a/src/storage/mysql_quota.rs b/src/storage/mysql_quota.rs index 518e2d9..a64c844 100644 --- a/src/storage/mysql_quota.rs +++ b/src/storage/mysql_quota.rs @@ -225,9 +225,11 @@ impl MySqlQuotaExt for super::mysql::MySqlStorage { // Calculate next reset date (first day of next month) let now = Utc::now(); let next_month = if now.month() == 12 { - chrono::NaiveDate::from_ymd_opt(now.year() + 1, 1, 1).unwrap() + chrono::NaiveDate::from_ymd_opt(now.year() + 1, 1, 1) + .expect("Valid date: January 1st of next year") } else { - chrono::NaiveDate::from_ymd_opt(now.year(), now.month() + 1, 1).unwrap() + chrono::NaiveDate::from_ymd_opt(now.year(), now.month() + 1, 1) + .expect("Valid date: 1st of next month") }; sqlx::query( @@ -755,9 +757,11 @@ impl MySqlQuotaExt for super::mysql::MySqlStorage { // Update quota reset date let now = Utc::now(); let next_month = if now.month() == 12 { - chrono::NaiveDate::from_ymd_opt(now.year() + 1, 1, 1).unwrap() + chrono::NaiveDate::from_ymd_opt(now.year() + 1, 1, 1) + .expect("Valid date: January 1st of next year") } else { - chrono::NaiveDate::from_ymd_opt(now.year(), now.month() + 1, 1).unwrap() + chrono::NaiveDate::from_ymd_opt(now.year(), now.month() + 1, 1) + .expect("Valid date: 1st of next month") }; sqlx::query(