diff --git a/aw-sync/src/lib.rs b/aw-sync/src/lib.rs index d165dceb..3a85988a 100644 --- a/aw-sync/src/lib.rs +++ b/aw-sync/src/lib.rs @@ -12,6 +12,7 @@ pub use report::{ mod sync; pub use sync::create_datastore; +pub use sync::sanitize_hostname; pub use sync::sync_datastores; pub use sync::sync_run; pub use sync::SyncSpec; diff --git a/aw-sync/src/sync.rs b/aw-sync/src/sync.rs index c128b167..7b48e0b6 100644 --- a/aw-sync/src/sync.rs +++ b/aw-sync/src/sync.rs @@ -337,6 +337,42 @@ fn utf8_db_path(path: &Path) -> Result<&str, String> { .ok_or_else(|| format!("Sync database path is not valid UTF-8: {}", path.display())) } +/// Sanitize a device hostname so it is a legal bucket hostname and a stable +/// sync-ID suffix. +/// +/// Must stay byte-identical to aw-android's `sanitizeDeviceHostname` +/// (`mobile/src/main/java/net/activitywatch/android/DeviceHostname.kt`): +/// trim, lowercase, replace `[^a-z0-9_-]+` with `_`, trim `_`. Empty result +/// becomes `"unknown"`. +/// +/// Divergence here forks destination buckets the day Android migrates its +/// hostname column (ActivityWatch/aw-android#272) onto a different +/// `-synced-from-` ID (ActivityWatch/activitywatch#1373). +pub fn sanitize_hostname(raw: &str) -> String { + let value = raw.trim(); + if value.is_empty() { + return "unknown".to_string(); + } + let lower = value.to_lowercase(); + let mut out = String::with_capacity(lower.len()); + let mut in_run = false; + for c in lower.chars() { + if c.is_ascii_lowercase() || c.is_ascii_digit() || c == '_' || c == '-' { + out.push(c); + in_run = false; + } else if !in_run { + out.push('_'); + in_run = true; + } + } + let trimmed = out.trim_matches('_'); + if trimmed.is_empty() { + "unknown".to_string() + } else { + trimmed.to_string() + } +} + /// Returns the sync-destination bucket for a given bucket, creates it if it doesn't exist. /// /// Returns an error rather than panicking on a datastore failure or on bucket @@ -376,32 +412,86 @@ fn get_or_create_sync_bucket( ) }; + // Look up the unsanitized ID first. Any device that was synced before + // aw-android added hostname sanitization (ActivityWatch/aw-android#272) may + // have left a local bucket whose ID and hostname contain whitespace (e.g. + // `…-synced-from-POCO F8 Ultra`) or a case-only fork (`…-synced-from-PIXEL8`). + // Keep using that ID to avoid a full re-import (ActivityWatch/activitywatch#1373). match ds_to.get_bucket(new_id.as_str()) { - Ok(bucket) => Ok(bucket), - Err(DatastoreError::NoSuchBucket(_)) => { - let mut bucket_new = bucket_from.clone(); - bucket_new.id = new_id.clone(); - // Only stamp $aw.sync.origin on pull/import. The derived origin already handles - // the legacy case: hostname is used when the source bucket has no metadata field. - if let Some(origin) = sync_origin { - bucket_new - .data - .insert("$aw.sync.origin".to_string(), serde_json::json!(origin)); - } else { - // Push path: strip any stale $aw.sync.origin that bucket_from may carry - // (e.g. if it was previously imported by a pull). Staging copies must - // never look like synced-from-remote buckets. - bucket_new.data.remove("$aw.sync.origin"); - } - ds_to - .create_bucket(&bucket_new) - .map_err(|e| format!("Failed to create bucket '{new_id}': {e:?}"))?; - ds_to - .get_bucket(new_id.as_str()) - .map_err(|e| format!("Failed to read back bucket '{new_id}': {e:?}")) + Ok(bucket) => return Ok(bucket), + Err(DatastoreError::NoSuchBucket(_)) => {} + Err(e) => return Err(format!("Failed to get bucket '{new_id}': {e:?}")), + } + + // Always sanitize. DeviceHostname.kt lowercases and replaces punctuation, + // not just whitespace — `PIXEL8` vs `pixel8` is the same fork as + // `POCO F8 Ultra` vs `poco_f8_ultra`, just without spaces. Lookup order + // stays *raw ID → sanitized ID*; create under the sanitized ID whenever it + // differs from raw so Android's hostname-column migration lands on an + // existing bucket regardless of which character class differed. + // + // Desktop peers with dots (`erb-m2.localdomain`) will create + // `…-synced-from-erb-m2_localdomain` for *new* imports; existing ones are + // found via the raw lookup. That is a display wart the `(device_id, id)` + // identity work in ActivityWatch/activitywatch#302 removes — not a reason + // to keep the fork open. + let sanitized_hostname = sanitize_hostname(&bucket_from.hostname); + let sanitized_id = if let Some(ref origin) = sync_origin { + let orig_bucketid = bucket_from + .id + .split("-synced-from-") + .next() + .unwrap_or(bucket_from.id.as_str()); + format!("{orig_bucketid}-synced-from-{}", sanitize_hostname(origin)) + } else { + // Push path: keep the original bucket ID; only the hostname field + // needs to be a legal create_bucket value. + new_id.clone() + }; + // Android maps empty/punctuation-only names to the "unknown" sentinel. + // Creating `-synced-from-unknown` on pull would mix every such remote + // into one destination — the same provenance hole the + // `hostname == "unknown"` guard in `sync_datastores` exists to close. + // Refuse; the per-bucket warn+continue then skips this bucket. + if !is_push + && (sanitized_hostname == "unknown" || sanitized_id.ends_with("-synced-from-unknown")) + { + return Err(format!( + "Bucket '{}' hostname sanitizes to the unknown sentinel; \ + refusing to sync it without provenance", + bucket_from.id + )); + } + if sanitized_id != new_id { + match ds_to.get_bucket(sanitized_id.as_str()) { + Ok(bucket) => return Ok(bucket), + Err(DatastoreError::NoSuchBucket(_)) => {} + Err(e) => return Err(format!("Failed to get bucket '{sanitized_id}': {e:?}")), } - Err(e) => Err(format!("Failed to get bucket '{new_id}': {e:?}")), } + let (final_id, final_hostname) = (sanitized_id, sanitized_hostname); + + let mut bucket_new = bucket_from.clone(); + bucket_new.id = final_id.clone(); + bucket_new.hostname = final_hostname; + // Only stamp $aw.sync.origin on pull/import. The derived origin already handles + // the legacy case: hostname is used when the source bucket has no metadata field. + if let Some(origin) = sync_origin { + bucket_new + .data + .insert("$aw.sync.origin".to_string(), serde_json::json!(origin)); + } else { + // Push path: strip any stale $aw.sync.origin that bucket_from may carry + // (e.g. if it was previously imported by a pull). Staging copies must + // never look like synced-from-remote buckets. + bucket_new.data.remove("$aw.sync.origin"); + } + ds_to + .create_bucket(&bucket_new) + .map_err(|e| format!("Failed to create bucket '{final_id}': {e:?}"))?; + ds_to + .get_bucket(final_id.as_str()) + .map_err(|e| format!("Failed to read back bucket '{final_id}': {e:?}")) } /// Number of events fetched per page in the chunked-fetch loop in `sync_one`. @@ -528,10 +618,43 @@ pub fn sync_datastores( // Sync buckets in order of most recently updated buckets_from.sort_by_key(|b| b.metadata.end); + // Partial failure is non-fatal (one bad bucket must not skip the rest). + // Total failure must still be Err: otherwise a destination that is down + // reports success, which is the #682 silence reintroduced via #688's skip. let mut buckets = Vec::with_capacity(buckets_from.len()); + let mut attempted = 0usize; + let mut succeeded = 0usize; + let mut last_err: Option = None; for bucket_from in buckets_from { - let bucket_to = get_or_create_sync_bucket(&bucket_from, ds_to, is_push)?; - buckets.push(sync_one(ds_from, ds_to, bucket_from, bucket_to, sync_spec)?); + attempted += 1; + let bucket_id = bucket_from.id.clone(); + let bucket_to = match get_or_create_sync_bucket(&bucket_from, ds_to, is_push) { + Ok(b) => b, + Err(e) => { + warn!(" ! Skipping bucket '{}': {}", bucket_id, e); + last_err = Some(e); + continue; + } + }; + match sync_one(ds_from, ds_to, bucket_from, bucket_to, sync_spec) { + Ok(synced) => { + succeeded += 1; + buckets.push(synced); + } + Err(e) => { + warn!( + " ! Skipping sync for bucket '{bucket_id}': {e}. \ + Destination may already contain a partial write; next pass resumes from dest newest" + ); + last_err = Some(e); + } + } + } + if attempted > 0 && succeeded == 0 { + return Err(format!( + "all {attempted} buckets failed; last error: {}", + last_err.as_deref().unwrap_or("unknown") + )); } Ok(buckets) @@ -921,3 +1044,42 @@ mod pull_only_staging_tests { let _ = fs::remove_dir_all(&dir); } } + +#[cfg(test)] +mod hostname_sanitize_tests { + use super::sanitize_hostname; + + #[test] + fn poco_f8_ultra_matches_android() { + // The contract Erik asked for on ActivityWatch/aw-server-rust#697: + // whitespace-only replace would produce "POCO_F8_Ultra" and fork the + // day aw-android#272 migrates the phone's hostname column. + assert_eq!(sanitize_hostname("POCO F8 Ultra"), "poco_f8_ultra"); + // Case-only fork: no whitespace, but Android still lowercases. + assert_eq!(sanitize_hostname("PIXEL8"), "pixel8"); + // Dotted desktop hostname: punctuation becomes `_`. + assert_eq!( + sanitize_hostname("erb-m2.localdomain"), + "erb-m2_localdomain" + ); + } + + #[test] + fn android_device_hostname_contract() { + // Byte-identical to aw-android DeviceHostnameTest.kt. + assert_eq!(sanitize_hostname("Pixel 8"), "pixel_8"); + assert_eq!(sanitize_hostname("My-Phone_1"), "my-phone_1"); + assert_eq!(sanitize_hostname(" Pixel 8 "), "pixel_8"); + assert_eq!(sanitize_hostname(""), "unknown"); + assert_eq!(sanitize_hostname(" "), "unknown"); + assert_eq!(sanitize_hostname("***"), "unknown"); + // Whitespace + punctuation only: the get_or_create pull-refuse path. + assert_eq!(sanitize_hostname(" * "), "unknown"); + assert_eq!(sanitize_hostname(" !!! "), "unknown"); + assert_eq!(sanitize_hostname("PIXEL8"), "pixel8"); + assert_eq!( + sanitize_hostname("erb-m2.localdomain"), + "erb-m2_localdomain" + ); + } +} diff --git a/aw-sync/src/sync_wrapper.rs b/aw-sync/src/sync_wrapper.rs index b14d2542..21955d75 100644 --- a/aw-sync/src/sync_wrapper.rs +++ b/aw-sync/src/sync_wrapper.rs @@ -41,24 +41,54 @@ pub fn pull_all(client: &AwClient) -> Result> { .map(|d| d.path.display().to_string()) .collect::>() ); + // Partial failure is non-fatal (one bad peer must not skip the rest). + // Total failure must still be Err so CLI/JNI/supervisor can tell a + // destination-down pass from a successful one. + let mut attempted = 0usize; + let mut succeeded = 0usize; + let mut last_err: Option = None; for remote in selection.selected { + attempted += 1; + // Per-peer isolation: a peer that fails to open must not abort the + // pass and skip every peer after it. Bucket-level errors are already + // non-fatal in `sync_datastores`; peer-level open failures need the + // same warn+continue so a later skip-on-mismatch (#693) has somewhere + // to go instead of becoming "abort pass" (#688). match pull_db(client, &remote.hostname, &remote.path) { - Ok(one) => report.merge(one), + Ok(one) => { + succeeded += 1; + report.merge(one); + } Err(e) => { + warn!( + "Skipping peer '{}' ({:?}): {e}", + remote.hostname, remote.path + ); + warn!( + "Skipping peer '{}' ({:?}): {e}", + remote.hostname, remote.path + ); report.peers.push(PeerReport::failed( remote.device_id, remote.hostname, remote.path, e.to_string(), )); - report.finish(); - // Persist the aggregate (earlier peers + this failure), not - // only the phase-local report from the failing `sync_run`. - crate::report::persist_last_report_warn(&report); - return Err(e); + last_err = Some(e.to_string()); } } } + if attempted > 0 && succeeded == 0 { + report.finish(); + // Persist the aggregate (the failures) so a total abort is visible + // afterwards, then fail the pass. + crate::report::persist_last_report_warn(&report); + return Err(format!( + "all {attempted} peers failed; last error: {}", + last_err.as_deref().unwrap_or("unknown") + ) + .into()); + } report.finish(); Ok(report) } diff --git a/aw-sync/tests/sync.rs b/aw-sync/tests/sync.rs index add2937b..d4e02669 100644 --- a/aw-sync/tests/sync.rs +++ b/aw-sync/tests/sync.rs @@ -96,14 +96,18 @@ mod sync_tests { .collect() } - /// A datastore failure must be *returned*, never panicked. + /// A datastore failure must not panic. /// /// On Android the sync step runs inside a JNI `extern "C"` frame, where an /// unwinding panic aborts the whole app process rather than surfacing as an /// exception — the SIGABRT in ActivityWatch/aw-android#220. `sync_datastores` /// used to `unwrap()` every datastore call, so any failure here was fatal. + /// + /// Since the per-bucket non-fatal change (#692), a broken *sibling* does + /// not abort the whole pass. A pass where every bucket fails is still Err + /// so callers can tell it from success. No panic is the key invariant. #[test] - fn test_unusable_datastore_returns_error_instead_of_panicking() { + fn test_unusable_datastore_does_not_panic() { let state = init_teststate(); create_bucket(&state.ds_src, 0); @@ -114,6 +118,10 @@ mod sync_tests { )) .expect("path is valid UTF-8"); + // Previously this panicked (unwrap on datastore failure). Per-bucket + // skip makes a *partial* failure non-fatal, but every bucket failing + // (destination down) must still be Err so callers can tell it from + // success. The key property: it must not panic. let result = aw_sync::sync_datastores( &state.ds_src, &ds_broken, @@ -121,17 +129,360 @@ mod sync_tests { Some("device-0"), &SyncSpec::default(), ); + let err = + result.expect_err("total bucket failure must return Err, not Ok(()); must not panic"); + assert!( + err.contains("all 1 buckets failed"), + "error should report total failure, got: {err}" + ); + } + + /// Pulling a peer whose bucket hostname contains whitespace must not fail with + /// a 400: aw-server-rust rejects new buckets with whitespace hostnames (#658). + /// `get_or_create_sync_bucket` must sanitize the hostname (and derived ID) + /// before creating, while still re-using any legacy unsanitized bucket that + /// was imported before the sanitization was added. + #[test] + fn test_whitespace_hostname_pull_creates_sanitized_bucket() { + let state = init_teststate(); + + // Source bucket whose hostname contains a space, as produced by Android + // devices that were named before aw-android added hostname sanitization + // (ActivityWatch/aw-android#272). + let bucket: Bucket = serde_json::from_value(serde_json::json!({ + "id": "aw-watcher-android", + "type": "currentwindow", + "hostname": "POCO F8 Ultra", + "client": "aw-android" + })) + .unwrap(); + state.ds_src.create_bucket(&bucket).unwrap(); + + // In-memory datastore does not enforce the server-side whitespace check, + // so the sync completes without 400. + aw_sync::sync_datastores( + &state.ds_src, + &state.ds_dest, + false, // pull + None, + &SyncSpec::default(), + ) + .unwrap(); + + let dest_buckets = state.ds_dest.get_buckets().unwrap(); + + // Must match aw-android's sanitizeDeviceHostname, not a whitespace-only + // replace ("POCO_F8_Ultra" would fork when Android migrates). + let sanitized_id = "aw-watcher-android-synced-from-poco_f8_ultra"; + assert!( + dest_buckets.contains_key(sanitized_id), + "expected sanitized bucket id '{sanitized_id}', got: {:?}", + dest_buckets.keys().collect::>() + ); + // No whitespace bucket must have been created. + let whitespace_id = "aw-watcher-android-synced-from-POCO F8 Ultra"; + assert!( + !dest_buckets.contains_key(whitespace_id), + "whitespace bucket id '{whitespace_id}' must not be created" + ); + assert!( + !dest_buckets.contains_key("aw-watcher-android-synced-from-POCO_F8_Ultra"), + "whitespace-only replace must not be used; got: {:?}", + dest_buckets.keys().collect::>() + ); + + // The hostname field on the destination bucket must also be sanitized. + let dest_bucket = dest_buckets.get(sanitized_id).unwrap(); + assert_eq!( + dest_bucket.hostname, "poco_f8_ultra", + "destination hostname must match Android's sanitizer" + ); + // $aw.sync.origin keeps the raw hostname so the pre-migration phone + // identity is still recoverable. + assert_eq!( + dest_bucket + .data + .get("$aw.sync.origin") + .and_then(|v| v.as_str()), + Some("POCO F8 Ultra") + ); + } + + /// If a legacy unsanitized bucket already exists in the destination (imported + /// before the sanitization was added), re-use it instead of creating a new + /// sanitized one. Creating a sanitized copy forks the destination and causes + /// a full re-import (ActivityWatch/activitywatch#1373). + #[test] + fn test_whitespace_hostname_pull_reuses_legacy_unsanitized_bucket() { + let state = init_teststate(); + + let bucket: Bucket = serde_json::from_value(serde_json::json!({ + "id": "aw-watcher-android", + "type": "currentwindow", + "hostname": "POCO F8 Ultra", + "client": "aw-android" + })) + .unwrap(); + state.ds_src.create_bucket(&bucket).unwrap(); + + // Simulate a pre-existing legacy destination bucket with unsanitized ID. + let legacy_id = "aw-watcher-android-synced-from-POCO F8 Ultra"; + let legacy_bucket: Bucket = serde_json::from_value(serde_json::json!({ + "id": legacy_id, + "type": "currentwindow", + "hostname": "POCO F8 Ultra", + "client": "aw-android" + })) + .unwrap(); + state.ds_dest.create_bucket(&legacy_bucket).unwrap(); + + aw_sync::sync_datastores( + &state.ds_src, + &state.ds_dest, + false, // pull + None, + &SyncSpec::default(), + ) + .unwrap(); + + let dest_buckets = state.ds_dest.get_buckets().unwrap(); + + // The legacy bucket must be present and re-used — not replaced. + assert!( + dest_buckets.contains_key(legacy_id), + "legacy unsanitized bucket must be preserved" + ); + // No new sanitized duplicate must have been created. + let sanitized_id = "aw-watcher-android-synced-from-poco_f8_ultra"; assert!( - result.is_err(), - "an unusable destination datastore must return Err, got {result:?}" + !dest_buckets.contains_key(sanitized_id), + "a sanitized fork must not be created when legacy bucket exists, got: {:?}", + dest_buckets.keys().collect::>() ); } - /// Bucket metadata of an unexpected shape must also be an error, not a panic: + /// Case-only hostnames (`PIXEL8`) have no whitespace, so a whitespace-only + /// guard would leave the destination as `…-synced-from-PIXEL8`. Android's + /// later hostname migration produces `pixel8` and forks the history. + #[test] + fn test_case_only_hostname_pull_creates_sanitized_bucket() { + let state = init_teststate(); + + let bucket: Bucket = serde_json::from_value(serde_json::json!({ + "id": "aw-watcher-android", + "type": "currentwindow", + "hostname": "PIXEL8", + "client": "aw-android" + })) + .unwrap(); + state.ds_src.create_bucket(&bucket).unwrap(); + + aw_sync::sync_datastores( + &state.ds_src, + &state.ds_dest, + false, + None, + &SyncSpec::default(), + ) + .unwrap(); + + let dest_buckets = state.ds_dest.get_buckets().unwrap(); + let sanitized_id = "aw-watcher-android-synced-from-pixel8"; + assert!( + dest_buckets.contains_key(sanitized_id), + "expected sanitized bucket id '{sanitized_id}', got: {:?}", + dest_buckets.keys().collect::>() + ); + assert!( + !dest_buckets.contains_key("aw-watcher-android-synced-from-PIXEL8"), + "case-only fork must not be created, got: {:?}", + dest_buckets.keys().collect::>() + ); + assert_eq!(dest_buckets.get(sanitized_id).unwrap().hostname, "pixel8"); + } + + /// Dotted desktop hostnames (`erb-m2.localdomain`) sanitize punctuation to + /// `_` for *new* imports. Existing raw IDs are still found via the raw lookup. + #[test] + fn test_dotted_hostname_pull_creates_sanitized_bucket() { + let state = init_teststate(); + + let bucket: Bucket = serde_json::from_value(serde_json::json!({ + "id": "aw-watcher-window", + "type": "currentwindow", + "hostname": "erb-m2.localdomain", + "client": "aw-watcher-window" + })) + .unwrap(); + state.ds_src.create_bucket(&bucket).unwrap(); + + aw_sync::sync_datastores( + &state.ds_src, + &state.ds_dest, + false, + None, + &SyncSpec::default(), + ) + .unwrap(); + + let dest_buckets = state.ds_dest.get_buckets().unwrap(); + let sanitized_id = "aw-watcher-window-synced-from-erb-m2_localdomain"; + assert!( + dest_buckets.contains_key(sanitized_id), + "expected sanitized bucket id '{sanitized_id}', got: {:?}", + dest_buckets.keys().collect::>() + ); + assert!( + !dest_buckets.contains_key("aw-watcher-window-synced-from-erb-m2.localdomain"), + "dotted raw id must not be created for new imports, got: {:?}", + dest_buckets.keys().collect::>() + ); + } + + /// If a legacy case-only bucket already exists, re-use it rather than + /// creating `…-synced-from-pixel8` beside `…-synced-from-PIXEL8`. + #[test] + fn test_case_only_hostname_pull_reuses_legacy_bucket() { + let state = init_teststate(); + + let bucket: Bucket = serde_json::from_value(serde_json::json!({ + "id": "aw-watcher-android", + "type": "currentwindow", + "hostname": "PIXEL8", + "client": "aw-android" + })) + .unwrap(); + state.ds_src.create_bucket(&bucket).unwrap(); + + let legacy_id = "aw-watcher-android-synced-from-PIXEL8"; + let legacy_bucket: Bucket = serde_json::from_value(serde_json::json!({ + "id": legacy_id, + "type": "currentwindow", + "hostname": "PIXEL8", + "client": "aw-android" + })) + .unwrap(); + state.ds_dest.create_bucket(&legacy_bucket).unwrap(); + + aw_sync::sync_datastores( + &state.ds_src, + &state.ds_dest, + false, + None, + &SyncSpec::default(), + ) + .unwrap(); + + let dest_buckets = state.ds_dest.get_buckets().unwrap(); + assert!( + dest_buckets.contains_key(legacy_id), + "legacy case-only bucket must be preserved" + ); + assert!( + !dest_buckets.contains_key("aw-watcher-android-synced-from-pixel8"), + "a sanitized fork must not be created when legacy bucket exists, got: {:?}", + dest_buckets.keys().collect::>() + ); + } + + /// If `$aw.sync.origin` is already clean while `bucket.hostname` still has + /// whitespace, the sanitizer must still run: otherwise `create_bucket` 400s + /// on the hostname field even though the derived ID is legal. + #[test] + fn test_whitespace_hostname_sanitizes_even_when_id_is_clean() { + let state = init_teststate(); + + let bucket: Bucket = serde_json::from_value(serde_json::json!({ + "id": "aw-watcher-android", + "type": "currentwindow", + "hostname": "POCO F8 Ultra", + "client": "aw-android", + "data": {"$aw.sync.origin": "poco_f8_ultra"} + })) + .unwrap(); + state.ds_src.create_bucket(&bucket).unwrap(); + + aw_sync::sync_datastores( + &state.ds_src, + &state.ds_dest, + false, // pull + None, + &SyncSpec::default(), + ) + .unwrap(); + + let dest_buckets = state.ds_dest.get_buckets().unwrap(); + let sanitized_id = "aw-watcher-android-synced-from-poco_f8_ultra"; + let dest_bucket = dest_buckets.get(sanitized_id).unwrap_or_else(|| { + panic!( + "expected sanitized bucket id '{sanitized_id}', got: {:?}", + dest_buckets.keys().collect::>() + ) + }); + assert_eq!(dest_bucket.hostname, "poco_f8_ultra"); + } + + /// A hostname that contains whitespace but sanitizes to the "unknown" + /// sentinel (e.g. `" * "`) must not create `-synced-from-unknown` on pull — + /// that ID is shared by every such remote and would mix events. The bucket + /// is skipped; a healthy sibling still syncs (per-bucket non-fatal). + #[test] + fn test_whitespace_hostname_that_sanitizes_to_unknown_is_skipped_on_pull() { + let state = init_teststate(); + + let junk: Bucket = serde_json::from_value(serde_json::json!({ + "id": "bucket-junk", + "type": "test", + "hostname": " * ", + "client": "test" + })) + .unwrap(); + state.ds_src.create_bucket(&junk).unwrap(); + + let healthy: Bucket = serde_json::from_value(serde_json::json!({ + "id": "bucket-healthy", + "type": "test", + "hostname": "device-0", + "client": "test" + })) + .unwrap(); + state.ds_src.create_bucket(&healthy).unwrap(); + + let result = aw_sync::sync_datastores( + &state.ds_src, + &state.ds_dest, + false, // pull + None, + &SyncSpec::default(), + ); + assert!( + result.is_ok(), + "junk hostname must skip that bucket, not abort the pass; got {result:?}" + ); + + let dest_buckets = state.ds_dest.get_buckets().unwrap(); + assert!( + !dest_buckets + .keys() + .any(|k| k.contains("bucket-junk") || k.ends_with("-synced-from-unknown")), + "must not create -synced-from-unknown, got: {:?}", + dest_buckets.keys().collect::>() + ); + assert!( + dest_buckets.contains_key("bucket-healthy-synced-from-device-0"), + "healthy sibling must still sync, got: {:?}", + dest_buckets.keys().collect::>() + ); + } + + /// Bucket metadata of an unexpected shape must not panic: /// `$aw.sync.origin` is read from data written by another host, so it is not /// under this host's control. + /// + /// With the per-bucket non-fatal change (#692), a malformed bucket is now + /// skipped (warn + continue) rather than aborting the whole sync pass. #[test] - fn test_non_string_sync_origin_returns_error_instead_of_panicking() { + fn test_non_string_sync_origin_does_not_panic() { let state = init_teststate(); let bucket: Bucket = serde_json::from_str( r#"{ @@ -145,6 +496,9 @@ mod sync_tests { .unwrap(); state.ds_src.create_bucket(&bucket).unwrap(); + // Previously this panicked. A single malformed bucket is a total + // failure of the pass, so it must return Err (not Ok after skip). + // No panic is still the key invariant. let result = aw_sync::sync_datastores( &state.ds_src, &state.ds_dest, @@ -152,10 +506,17 @@ mod sync_tests { None, &SyncSpec::default(), ); - let err = result.expect_err("a non-string $aw.sync.origin must return Err"); + let err = result.expect_err("total failure of a one-bucket pass must be Err"); + assert!( + err.contains("all 1 buckets failed"), + "error should report total failure, got: {err}" + ); + // The malformed bucket must have been skipped, not imported. + let dest_buckets = state.ds_dest.get_buckets().unwrap(); assert!( - err.contains("$aw.sync.origin"), - "error should name the offending field, got: {err}" + dest_buckets.is_empty(), + "skipped bucket must not appear in destination, got: {:?}", + dest_buckets.keys().collect::>() ); } diff --git a/aw-sync/tests/sync_roundtrip.rs b/aw-sync/tests/sync_roundtrip.rs index e9477bf7..1e35aab0 100644 --- a/aw-sync/tests/sync_roundtrip.rs +++ b/aw-sync/tests/sync_roundtrip.rs @@ -87,10 +87,12 @@ fn round_trip() -> (Datastore, Datastore) { sync_datastores(&a_local, &a_export, true, Some("device-A"), &spec).unwrap(); // 2. HOSTB pulls HOSTA's export. This copy is correct and expected. + // Destination IDs use the sanitized origin (`hosta`), matching + // DeviceHostname.kt — not the raw `HOSTA` hostname. sync_datastores(&a_export, &b_local, false, None, &spec).unwrap(); assert!( - bucket_ids(&b_local).contains(&"aw-watcher-window_HOSTA-synced-from-HOSTA".to_string()), - "precondition: HOSTB should hold HOSTA's data as a synced-from-HOSTA bucket, got {:?}", + bucket_ids(&b_local).contains(&"aw-watcher-window_HOSTA-synced-from-hosta".to_string()), + "precondition: HOSTB should hold HOSTA's data as a synced-from-hosta bucket, got {:?}", bucket_ids(&b_local) );