test: better sync coverage

src-tauri/src/lib.rs

@@ -157,12 +157,12 @@ use settings_manager::{
 };
 
 use sync::{
-  cancel_profile_sync, check_has_e2e_password, delete_e2e_password, enable_sync_for_all_entities,
-  get_unsynced_entity_counts, is_group_in_use_by_synced_profile, is_proxy_in_use_by_synced_profile,
-  is_vpn_in_use_by_synced_profile, request_profile_sync, rollover_encryption_for_all_entities,
-  set_e2e_password, set_extension_group_sync_enabled, set_extension_sync_enabled,
-  set_group_sync_enabled, set_profile_sync_mode, set_proxy_sync_enabled, set_vpn_sync_enabled,
-  verify_e2e_password,
+  cancel_profile_sync, check_has_e2e_password, check_sync_server_connection, delete_e2e_password,
+  enable_sync_for_all_entities, get_unsynced_entity_counts, is_group_in_use_by_synced_profile,
+  is_proxy_in_use_by_synced_profile, is_vpn_in_use_by_synced_profile, request_profile_sync,
+  rollover_encryption_for_all_entities, set_e2e_password, set_extension_group_sync_enabled,
+  set_extension_sync_enabled, set_group_sync_enabled, set_profile_sync_mode,
+  set_proxy_sync_enabled, set_vpn_sync_enabled, verify_e2e_password,
 };
 
 use tag_manager::get_all_tags;
@@ -2799,6 +2799,7 @@ pub fn run_with_builder(
       validate_vless_uri,
       get_sync_settings,
       save_sync_settings,
+      check_sync_server_connection,
       set_profile_sync_mode,
       cancel_profile_sync,
       request_profile_sync,

src-tauri/src/sync/client.rs

@@ -234,10 +234,15 @@ impl SyncClient {
       }
     }
 
+    // The storage host here comes from the presigned URL, so on a self-hosted
+    // server it is whatever the server signed against — frequently an address
+    // only the server can resolve. `reqwest`'s own Display collapses that to
+    // "error sending request", which is why this failure used to be
+    // undiagnosable; report the innermost cause instead.
     let response = req
       .send()
       .await
-      .map_err(|e| SyncError::NetworkError(e.to_string()))?;
+      .map_err(|e| SyncError::NetworkError(super::preflight::transport_reason(&e)))?;
 
     if !response.status().is_success() {
       let status = response.status();
@@ -256,7 +261,7 @@ impl SyncClient {
       .get(presigned_url)
       .send()
       .await
-      .map_err(|e| SyncError::NetworkError(e.to_string()))?;
+      .map_err(|e| SyncError::NetworkError(super::preflight::transport_reason(&e)))?;
 
     if !response.status().is_success() {
       return Err(SyncError::NetworkError(format!(

src-tauri/src/sync/engine.rs

@@ -134,12 +134,41 @@ fn critical_failure_message(action: &str, failures: &[(String, String)]) -> Stri
 
   match failures.first() {
     Some((_, cause)) => format!(
-      "Critical files failed to {action}: {files}. Cause: {cause}. Sync aborted to prevent data loss."
+      "Critical files failed to {action}: {files}. Cause: {cause}.{hint} Sync aborted to prevent data loss.",
+      hint = storage_endpoint_hint(cause)
     ),
     None => format!("Critical files failed to {action}: {files}. Sync aborted to prevent data loss."),
   }
 }
 
+/// The one fix worth naming when every transfer dies at connect.
+///
+/// Transfers go straight to the storage host named in the presigned URL, not
+/// through the sync server, so a self-hosted server that signs URLs against an
+/// address only it can resolve fails every file here while its own `/health`
+/// and `/readyz` stay green. The cause string already carries the host; without
+/// this line it still reads as an unexplained network fault, and the setting
+/// that fixes it lives on the server, where the user is not looking.
+fn storage_endpoint_hint(cause: &str) -> String {
+  let lowered = cause.to_ascii_lowercase();
+  let is_transport_failure = [
+    "connection failed",
+    "timed out",
+    "dns",
+    "error sending request",
+  ]
+  .iter()
+  .any(|marker| lowered.contains(marker));
+
+  if is_transport_failure {
+    " The storage host in the presigned URL could not be reached from this device. \
+     On a self-hosted server, set S3_PUBLIC_ENDPOINT to an address this device can reach."
+      .to_string()
+  } else {
+    String::new()
+  }
+}
+
 /// Validate that a manifest-supplied relative file path is safe to join onto a
 /// profile directory before writing/deleting. The manifest is remote-controlled
 /// (a self-hosted or compromised sync server, a MITM on a plaintext Regular-mode
@@ -4326,6 +4355,43 @@ mod tests {
     assert!(message.contains("failed to download"));
   }
 
+  #[test]
+  fn test_critical_failure_message_names_the_storage_endpoint_fix() {
+    // A self-hosted server that signs presigned URLs against a container-only
+    // host fails every transfer at connect while the server itself looks
+    // healthy. Naming the file and the socket error is not enough to find the
+    // setting that fixes it.
+    let failures = vec![(
+      "Default/Cookies".to_string(),
+      "Failed to upload Default/Cookies after 3 retries: connection failed: \
+       failed to lookup address information for minio"
+        .to_string(),
+    )];
+
+    let message = critical_failure_message("upload", &failures);
+    assert!(message.contains("S3_PUBLIC_ENDPOINT"), "{message}");
+    assert!(message.contains("could not be reached from this device"));
+    assert!(message.contains("Sync aborted to prevent data loss."));
+  }
+
+  #[test]
+  fn test_critical_failure_message_omits_the_hint_for_non_transport_causes() {
+    // A rejected signature or a full disk is not a routing problem, and
+    // pointing those users at S3_PUBLIC_ENDPOINT sends them the wrong way.
+    for cause in [
+      "Upload failed with status 403: SignatureDoesNotMatch",
+      "Upload failed with status 507: quota exceeded",
+      "No space left on device",
+    ] {
+      let failures = vec![("Default/Cookies".to_string(), cause.to_string())];
+      let message = critical_failure_message("upload", &failures);
+      assert!(
+        !message.contains("S3_PUBLIC_ENDPOINT"),
+        "hint must not fire for: {cause}"
+      );
+    }
+  }
+
   #[test]
   fn test_is_safe_manifest_path() {
     // Legitimate profile-relative paths are accepted.

src-tauri/src/sync/mod.rs

@@ -2,6 +2,7 @@ mod client;
 pub mod encryption;
 mod engine;
 pub mod manifest;
+pub mod preflight;
 pub mod scheduler;
 pub mod subscription;
 pub mod types;
@@ -25,6 +26,7 @@ pub use manifest::{
   compute_diff, compute_diff_with_bias, generate_manifest, DiffBias, HashCache, ManifestDiff,
   SyncManifest,
 };
+pub use preflight::{check_sync_server, check_sync_server_connection, SyncServerCheck};
 pub use scheduler::{get_global_scheduler, set_global_scheduler, SyncScheduler};
 pub use subscription::{SubscriptionManager, SyncWorkItem};
 pub use types::{SyncError, SyncResult};

src-tauri/src/sync/preflight.rs

@@ -0,0 +1,263 @@
+//! Pre-flight check for a sync server, run from the network stack that
+//! actually performs transfers.
+//!
+//! A self-hosted server almost always reaches its storage over an address only
+//! it can resolve: the documented compose file points `S3_ENDPOINT` at
+//! `http://minio:9000`, a Docker service name that exists on the compose
+//! network and nowhere else. Presigned URLs are signed against the host they
+//! name, so every URL handed to this device names a host it cannot open. The
+//! server is healthy, `/health` and `/readyz` are green, and every single file
+//! transfer fails at connect.
+//!
+//! Checking the server alone is what let that configuration look correct. This
+//! module also opens the storage host the server says it hands out, from here,
+//! with the same client the uploader uses, so the break is named at the moment
+//! the user configures sync instead of after the first sync fails.
+
+use serde::{Deserialize, Serialize};
+use std::time::Duration;
+
+/// Both probes are liveness questions, not transfers, so they must fail fast
+/// rather than sit on a connect that is never going to answer.
+const PROBE_TIMEOUT: Duration = Duration::from_secs(8);
+
+/// What a pre-flight found. Every field is reported rather than collapsed into
+/// one boolean: "the server answers but its storage is unreachable from here"
+/// is a different problem with a different fix than "the server is down", and
+/// the UI has to be able to say which one happened.
+#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, Default)]
+pub struct SyncServerCheck {
+  /// The sync server itself answered.
+  pub server_reachable: bool,
+  /// The server reports it can reach its own storage. `None` when the server
+  /// is too old to serve `/readyz`, which is a working server, not a broken
+  /// one.
+  pub storage_ready: Option<bool>,
+  /// The host the server signs into presigned URLs, when it discloses one.
+  /// Withheld by cloud deployments on purpose.
+  pub storage_endpoint: Option<String>,
+  /// Whether that host answered *this device*. `None` when there was nothing
+  /// to probe.
+  pub storage_reachable: Option<bool>,
+  /// Why the storage probe failed, for the log and the error surface.
+  pub storage_error: Option<String>,
+}
+
+impl SyncServerCheck {
+  /// Whether sync can actually move bytes. A green server with an unreachable
+  /// storage host is the exact state this check exists to stop reporting as
+  /// success.
+  pub fn is_usable(&self) -> bool {
+    self.server_reachable
+      && self.storage_ready != Some(false)
+      && self.storage_reachable != Some(false)
+  }
+}
+
+/// The `/readyz` body. Every field is optional: older servers answer `/health`
+/// only, and cloud deployments withhold `storageEndpoint`.
+#[derive(Debug, Deserialize)]
+struct ReadyzBody {
+  #[serde(default)]
+  s3: Option<bool>,
+  #[serde(default, rename = "storageEndpoint")]
+  storage_endpoint: Option<String>,
+}
+
+fn probe_client() -> reqwest::Client {
+  // Matches how `SyncClient` builds its client, so a TLS trust or proxy
+  // condition that would fail an upload fails the probe the same way. A probe
+  // that is more permissive than the uploader would report a working setup for
+  // a configuration that cannot transfer.
+  reqwest::Client::builder()
+    .timeout(PROBE_TIMEOUT)
+    .build()
+    .unwrap_or_default()
+}
+
+/// Ask the sync server about itself, then verify the storage host it names.
+pub async fn check_sync_server(server_url: &str) -> SyncServerCheck {
+  let base = server_url.trim().trim_end_matches('/');
+  if base.is_empty() {
+    return SyncServerCheck::default();
+  }
+
+  let client = probe_client();
+  let mut check = SyncServerCheck::default();
+
+  let readyz = match client.get(format!("{base}/readyz")).send().await {
+    Ok(response) => response,
+    Err(e) => {
+      log::warn!("Sync pre-flight: {base}/readyz did not answer: {e}");
+      return check;
+    }
+  };
+
+  if readyz.status() == reqwest::StatusCode::NOT_FOUND {
+    // Predates /readyz. It is still a working server, so fall back rather than
+    // failing a healthy setup, and leave the storage fields unknown.
+    check.server_reachable = matches!(
+      client.get(format!("{base}/health")).send().await,
+      Ok(health) if health.status().is_success()
+    );
+    return check;
+  }
+
+  // A 503 from /readyz is the server telling us its storage is down. That is a
+  // reachable server with a real diagnosis in the body, so read it rather than
+  // discarding it as a failed request.
+  check.server_reachable = readyz.status().is_success() || readyz.status().as_u16() == 503;
+  if !check.server_reachable {
+    return check;
+  }
+
+  let body = readyz.json::<ReadyzBody>().await.ok();
+  check.storage_ready = body.as_ref().and_then(|b| b.s3);
+  check.storage_endpoint = body.and_then(|b| b.storage_endpoint);
+
+  if let Some(endpoint) = check.storage_endpoint.clone() {
+    match probe_storage_endpoint(&client, &endpoint).await {
+      Ok(()) => check.storage_reachable = Some(true),
+      Err(e) => {
+        log::warn!("Sync pre-flight: storage endpoint {endpoint} is unreachable from here: {e}");
+        check.storage_reachable = Some(false);
+        check.storage_error = Some(e);
+      }
+    }
+  }
+
+  check
+}
+
+/// Open the storage host and report only whether it answered.
+///
+/// ANY HTTP status counts as reachable, including 403 and 404. An unsigned GET
+/// of a bucket root is supposed to be refused; being refused proves DNS, TCP
+/// and TLS all worked, which is the entire question. Only a transport error
+/// means the presigned URLs cannot be opened from this device.
+async fn probe_storage_endpoint(client: &reqwest::Client, endpoint: &str) -> Result<(), String> {
+  match client.get(endpoint).send().await {
+    Ok(_) => Ok(()),
+    Err(e) => Err(transport_reason(&e)),
+  }
+}
+
+/// A short reason for a failed request.
+///
+/// `reqwest::Error`'s own `Display` is one line about the request and hides the
+/// cause chain, so a DNS failure reads as "error sending request" — the exact
+/// uninformative text that made this class of failure undiagnosable in the
+/// first place. Walk to the innermost source instead.
+///
+/// Shared with the transfer path so a failed upload and a failed probe describe
+/// the same network condition in the same words.
+pub(crate) fn transport_reason(error: &reqwest::Error) -> String {
+  let kind = if error.is_timeout() {
+    "timed out"
+  } else if error.is_connect() {
+    "connection failed"
+  } else {
+    "request failed"
+  };
+
+  let mut source: Option<&(dyn std::error::Error + 'static)> = std::error::Error::source(error);
+  let mut innermost: Option<String> = None;
+  while let Some(cause) = source {
+    innermost = Some(cause.to_string());
+    source = cause.source();
+  }
+
+  match innermost {
+    Some(detail) => format!("{kind}: {detail}"),
+    None => kind.to_string(),
+  }
+}
+
+/// Pre-flight a sync server before saving it, and before trusting it to sync.
+#[tauri::command]
+pub async fn check_sync_server_connection(server_url: String) -> Result<SyncServerCheck, String> {
+  Ok(check_sync_server(&server_url).await)
+}
+
+#[cfg(test)]
+mod tests {
+  use super::*;
+
+  #[test]
+  fn unreachable_storage_is_not_usable() {
+    // The shape that used to report as healthy: server up, server's own
+    // storage fine, and the host it hands to clients resolving nowhere but the
+    // compose network.
+    let check = SyncServerCheck {
+      server_reachable: true,
+      storage_ready: Some(true),
+      storage_endpoint: Some("http://minio:9000".to_string()),
+      storage_reachable: Some(false),
+      storage_error: Some("connection failed: dns error".to_string()),
+    };
+    assert!(!check.is_usable());
+  }
+
+  #[test]
+  fn reachable_storage_is_usable() {
+    let check = SyncServerCheck {
+      server_reachable: true,
+      storage_ready: Some(true),
+      storage_endpoint: Some("http://localhost:9101".to_string()),
+      storage_reachable: Some(true),
+      storage_error: None,
+    };
+    assert!(check.is_usable());
+  }
+
+  #[test]
+  fn server_without_readyz_is_usable() {
+    // A server old enough to predate /readyz discloses nothing about storage.
+    // Unknown must not read as broken, or every older self-hosted server would
+    // start reporting a failure it does not have.
+    let check = SyncServerCheck {
+      server_reachable: true,
+      storage_ready: None,
+      storage_endpoint: None,
+      storage_reachable: None,
+      storage_error: None,
+    };
+    assert!(check.is_usable());
+  }
+
+  #[test]
+  fn server_reporting_its_own_storage_down_is_not_usable() {
+    let check = SyncServerCheck {
+      server_reachable: true,
+      storage_ready: Some(false),
+      ..Default::default()
+    };
+    assert!(!check.is_usable());
+  }
+
+  #[test]
+  fn unreachable_server_is_not_usable() {
+    assert!(!SyncServerCheck::default().is_usable());
+  }
+
+  #[tokio::test]
+  async fn empty_url_reports_unreachable_without_a_request() {
+    assert_eq!(check_sync_server("   ").await, SyncServerCheck::default());
+  }
+
+  #[tokio::test]
+  async fn unresolvable_storage_host_is_reported_with_a_cause() {
+    // Exercises the real probe against a host that cannot resolve, which is
+    // what a container-only endpoint looks like from the desktop.
+    let client = probe_client();
+    let error = probe_storage_endpoint(&client, "http://minio.invalid:9000")
+      .await
+      .expect_err("an unresolvable host must not report as reachable");
+    assert!(
+      error.contains("failed") || error.contains("timed out"),
+      "unexpected reason: {error}"
+    );
+    // The bare reqwest Display is what this exists to avoid.
+    assert_ne!(error, "error sending request");
+  }
+}