Updater: cap hangs at 30s, surface real error causes

vdavid · vdavid · commit e5be14677f0b · 2026-05-09T00:13:21.000+02:00
- Add `connect_timeout(10s)` + overall `timeout(30s)` to the manifest fetch in `check_for_update`. Was unbounded — observed to hang ~2.5 min on a TCP handshake that never completed against the redirect target, looking like a hung app and tripping the auto error reporter.
- Add `connect_timeout(10s)` + `read_timeout(30s)` to the tarball fetch in `download_update`. No overall `timeout` — legitimate slow downloads on a hotel/mobile network still work; mid-download stalls now fail in 30s instead of minutes.
- New `describe_error_chain` walks `reqwest::Error::source()` so logs and the Settings UI show the real cause (`tcp connect error: deadline has elapsed`, `dns error: failed to lookup ...`) instead of just `error sending request for url (...)`.
- Split the `checkForUpdates` catch into a check phase (warn — transient, expected) and a download/install phase (error — real failure, auto-reports). Helper `finishCheckWithFailure(err, phase)` keeps duplication minimal. The previous single-catch silently downgraded install/signature failures to warn too — actual bugs we'd want auto-reported.
- Two synthetic unit tests for the chain walker (always run); two `#[ignore]`'d network tests with the captured chain content for ad-hoc verification.
diff --git a/apps/desktop/src-tauri/src/updater/CLAUDE.md b/apps/desktop/src-tauri/src/updater/CLAUDE.md
@@ -38,6 +38,18 @@ signatures use base64(minisign-text-format) encoding, matching Tauri's internal
 `do shell script ... with administrator privileges` shows the native macOS auth dialog. `rsync` is used because it
 expresses the full sync (copy + delete stale) in a single shell command.
 
+**Decision**: Bounded `connect_timeout` (10 s) and overall `timeout` (30 s) on the manifest fetch.
+**Why**: `reqwest::get` uses a default client with no overall timeout. A stuck TCP handshake to the redirect target
+(`getcmdr.com/latest.json`) was observed to hang for 2.5 min before reporting `error sending request for url …`,
+which made transient network blips look like a hung app and tripped the auto error reporter. The bounds keep the
+periodic check honest. Download/install paths are intentionally NOT timed out — they run with user attention and
+can legitimately take a while.
+
+**Decision**: Walk `reqwest::Error::source()` for log-friendly messages (`describe_error_chain`).
+**Why**: `reqwest::Error`'s `Display` only prints the outermost layer (`error sending request for url …`), so logs
+hid the real cause (DNS lookup, TCP connect timeout, TLS handshake). Walking the source chain surfaces the
+underlying error class without pulling in `anyhow`.
+
 **Decision**: Atomic rename (write to temp file, then `rename()`) instead of in-place `fs::copy`.
 **Why**: `fs::copy` overwrites the destination in-place, keeping the same inode. macOS's kernel code signing cache
 keys on inode — it validates the new binary's pages against the old binary's cached code directory, causing
diff --git a/apps/desktop/src-tauri/src/updater/mod.rs b/apps/desktop/src-tauri/src/updater/mod.rs
@@ -16,8 +16,37 @@ mod signature;
 use manifest::UpdateInfo;
 use std::path::PathBuf;
 use std::sync::Mutex;
+use std::time::Duration;
 use tauri::State;
 
+// Per-call timeouts for the manifest fetch. The default `reqwest::get` client has no
+// overall timeout — a stuck TCP handshake against the redirect target can hang for
+// minutes before the OS gives up. These bounds keep a flaky network from looking like
+// a hung app and stop the auto-error-reporter from firing on long hangs.
+const MANIFEST_CONNECT_TIMEOUT: Duration = Duration::from_secs(10);
+const MANIFEST_REQUEST_TIMEOUT: Duration = Duration::from_secs(30);
+
+// Per-call timeouts for the tarball download. No overall `timeout` here — a 60+ MB
+// download on a slow connection can legitimately take minutes. `read_timeout` bounds
+// "no bytes received in N seconds" instead, which catches mid-download stalls without
+// punishing slow-but-working networks.
+const DOWNLOAD_CONNECT_TIMEOUT: Duration = Duration::from_secs(10);
+const DOWNLOAD_READ_TIMEOUT: Duration = Duration::from_secs(30);
+
+/// Renders an error and its full `source()` chain. `reqwest::Error`'s `Display` only
+/// prints the outermost layer (`error sending request for url …`), which hides the
+/// real cause (DNS lookup, TCP connect timeout, TLS handshake, etc.).
+fn describe_error_chain(err: &(dyn std::error::Error + 'static)) -> String {
+    let mut out = err.to_string();
+    let mut src = err.source();
+    while let Some(cause) = src {
+        out.push_str(": ");
+        out.push_str(&cause.to_string());
+        src = cause.source();
+    }
+    out
+}
+
 /// Shared state between `download_update` and `install_update`.
 /// Holds the path to the downloaded (and verified) tarball.
 pub struct UpdateState {
@@ -52,14 +81,22 @@ pub async fn check_for_update() -> Result<Option<UpdateInfo>, String> {
     let arch = manifest::platform_key().strip_prefix("darwin-").unwrap_or("unknown");
     let url = format!("https://api.getcmdr.com/update-check/{current_version}?arch={arch}");
 
-    let response = reqwest::get(&url)
+    let client = reqwest::Client::builder()
+        .connect_timeout(MANIFEST_CONNECT_TIMEOUT)
+        .timeout(MANIFEST_REQUEST_TIMEOUT)
+        .build()
+        .map_err(|e| format!("Couldn't build update HTTP client: {}", describe_error_chain(&e)))?;
+
+    let response = client
+        .get(&url)
+        .send()
         .await
-        .map_err(|e| format!("Couldn't fetch update manifest: {e}"))?;
+        .map_err(|e| format!("Couldn't fetch update manifest: {}", describe_error_chain(&e)))?;
 
     let manifest: manifest::UpdateManifest = response
         .json()
         .await
-        .map_err(|e| format!("Couldn't parse update manifest: {e}"))?;
+        .map_err(|e| format!("Couldn't parse update manifest: {}", describe_error_chain(&e)))?;
 
     Ok(manifest::check_manifest(&manifest, current_version))
 }
@@ -71,14 +108,22 @@ pub async fn check_for_update() -> Result<Option<UpdateInfo>, String> {
 pub async fn download_update(url: String, signature: String, state: State<'_, UpdateState>) -> Result<(), String> {
     log::info!("Downloading update from {url}");
 
-    let response = reqwest::get(&url)
+    let client = reqwest::Client::builder()
+        .connect_timeout(DOWNLOAD_CONNECT_TIMEOUT)
+        .read_timeout(DOWNLOAD_READ_TIMEOUT)
+        .build()
+        .map_err(|e| format!("Couldn't build update HTTP client: {}", describe_error_chain(&e)))?;
+
+    let response = client
+        .get(&url)
+        .send()
         .await
-        .map_err(|e| format!("Couldn't download update: {e}"))?;
+        .map_err(|e| format!("Couldn't download update: {}", describe_error_chain(&e)))?;
 
     let bytes = response
         .bytes()
         .await
-        .map_err(|e| format!("Couldn't read update response: {e}"))?;
+        .map_err(|e| format!("Couldn't read update response: {}", describe_error_chain(&e)))?;
 
     log::info!("Downloaded {} bytes, verifying signature", bytes.len());
     signature::verify(&bytes, &signature)?;
@@ -120,3 +165,109 @@ pub async fn install_update(state: State<'_, UpdateState>) -> Result<(), String>
         .await
         .map_err(|e| format!("Install task panicked: {e}"))?
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::error::Error;
+    use std::fmt;
+
+    #[derive(Debug)]
+    struct ChainErr {
+        msg: &'static str,
+        source: Option<Box<dyn Error + 'static>>,
+    }
+
+    impl fmt::Display for ChainErr {
+        fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+            f.write_str(self.msg)
+        }
+    }
+
+    impl Error for ChainErr {
+        fn source(&self) -> Option<&(dyn Error + 'static)> {
+            self.source.as_deref()
+        }
+    }
+
+    #[test]
+    fn describe_error_chain_renders_only_outer_when_no_source() {
+        let err = ChainErr {
+            msg: "outer",
+            source: None,
+        };
+        assert_eq!(describe_error_chain(&err), "outer");
+    }
+
+    #[test]
+    fn describe_error_chain_walks_full_source_chain() {
+        let inner = ChainErr {
+            msg: "io broken pipe",
+            source: None,
+        };
+        let middle = ChainErr {
+            msg: "hyper transport",
+            source: Some(Box::new(inner)),
+        };
+        let outer = ChainErr {
+            msg: "reqwest send",
+            source: Some(Box::new(middle)),
+        };
+        assert_eq!(
+            describe_error_chain(&outer),
+            "reqwest send: hyper transport: io broken pipe"
+        );
+    }
+
+    /// Sanity-check against an actual `reqwest::Error` for a name that can never resolve
+    /// (`.invalid` TLD per RFC 6761). `#[ignore]`'d because it depends on the local resolver
+    /// — run with
+    /// `cargo nextest run -p cmdr describe_error_chain --run-ignored=ignored-only --no-capture`
+    /// to see what reqwest 0.13's source() chain actually surfaces. The `eprintln!` is
+    /// allowed locally because the whole point of these tests is to render the chain into
+    /// stderr for human inspection — they're verification harnesses, not production code.
+    #[tokio::test]
+    #[ignore = "network-dependent; run manually to verify reqwest chain content"]
+    #[allow(clippy::print_stderr, reason = "verification harness — see fn doc")]
+    async fn describe_error_chain_unwraps_reqwest_dns_failure() {
+        let err = reqwest::Client::builder()
+            .connect_timeout(Duration::from_secs(2))
+            .timeout(Duration::from_secs(5))
+            .build()
+            .unwrap()
+            .get("http://nonexistent-host-for-cmdr-tests.invalid/")
+            .send()
+            .await
+            .expect_err("request to .invalid should fail");
+        let msg = describe_error_chain(&err);
+        eprintln!("DNS-failure chain: {msg}");
+        assert!(msg.len() > 60, "chain too short, source() likely empty: {msg}");
+    }
+
+    /// Sanity-check against an actual connect timeout (RFC 5737 unreachable address).
+    /// `#[ignore]` for the same reason as the DNS test.
+    #[tokio::test]
+    #[ignore = "network-dependent; run manually to verify reqwest chain content"]
+    #[allow(clippy::print_stderr, reason = "verification harness — see fn doc")]
+    async fn describe_error_chain_unwraps_reqwest_connect_timeout() {
+        let err = reqwest::Client::builder()
+            .connect_timeout(Duration::from_millis(500))
+            .build()
+            .unwrap()
+            .get("http://10.255.255.1/")
+            .send()
+            .await
+            .expect_err("connect to 10.255.255.1 should time out");
+        let msg = describe_error_chain(&err);
+        eprintln!("connect-timeout chain: {msg}");
+        // reqwest 0.13 wording, captured from a one-shot run on macOS:
+        //   error sending request for url (http://10.255.255.1/): client error (Connect): tcp connect error: deadline has elapsed
+        // Match on the "tcp connect" cause rather than a "timeout" keyword — reqwest words it
+        // as "deadline has elapsed", not "timeout".
+        let lower = msg.to_lowercase();
+        assert!(
+            lower.contains("tcp connect") || lower.contains("deadline") || lower.contains("timed out"),
+            "expected connect/deadline-shaped cause in chain: {msg}"
+        );
+    }
+}
diff --git a/apps/desktop/src/lib/updates/CLAUDE.md b/apps/desktop/src/lib/updates/CLAUDE.md
@@ -125,6 +125,9 @@ interval is acceptable.
 - The update manifest endpoint is hardcoded in the Rust backend (`https://getcmdr.com/latest.json`), not in TypeScript.
 - The `check_for_update` command returns `None` when `CI` env var is set — no network calls in CI.
 - No retry or backoff on error — the next interval fires a fresh attempt.
+- The catch in `checkForUpdates()` logs at `warn`, not `error`, so transient network failures during the periodic
+  background check don't trip the auto error reporter (Flow B). The Settings UI still surfaces the message via
+  `updateState.error`. See `apps/desktop/src-tauri/src/error_reporter/CLAUDE.md` § convention.
 - Default interval: 60 minutes. Configurable in settings from 5 minutes to 24 hours.
 - Unit tests in `updater.test.ts` cover the gating logic via the pure `shouldShowUpdateToast` predicate plus the
   `notifyOnboardingComplete` and `setFdaPromptShowing` triggers. The download-and-install path is still untested — it
diff --git a/apps/desktop/src/lib/updates/updater.svelte.ts b/apps/desktop/src/lib/updates/updater.svelte.ts
@@ -85,59 +85,117 @@ export async function checkForUpdates(): Promise<void> {
   updateState.status = 'checking'
   updateState.error = null
 
+  log.debug('Checking for updates (current: v{version})...', { version: currentVersion })
+
+  // Platform branches diverge significantly: macOS runs three custom commands (split download +
+  // install phases, preserves TCC), non-macOS uses the Tauri plugin's fused `downloadAndInstall`.
+  // The two-phase error handling (warn on check, error on download/install) lives inside each.
+  if (isMacOS) {
+    await runMacUpdateFlow(currentVersion)
+  } else {
+    await runPluginUpdateFlow(currentVersion)
+  }
+}
+
+/**
+ * macOS path: custom updater that preserves TCC/Full Disk Access permissions by syncing files
+ * into the existing `.app` bundle. Three Tauri commands; download and install are distinct
+ * phases so the UI can show separate `downloading` and `installing` states.
+ */
+async function runMacUpdateFlow(currentVersion: string): Promise<void> {
+  let update: UpdateInfo | null
   try {
-    log.debug('Checking for updates (current: v{version})...', { version: currentVersion })
-
-    if (isMacOS) {
-      // macOS: custom updater preserves TCC/Full Disk Access permissions.
-      // Platform asymmetry: the macOS path runs `download_update` and `install_update` as two
-      // distinct invokes, so we can split the UI into a `downloading` phase and an `installing`
-      // phase. The non-macOS path uses one fused `downloadAndInstall()` and stays in `downloading`.
-      const update = await invoke<UpdateInfo | null>('check_for_update')
-
-      if (update !== null) {
-        log.info('Update available: v{current} -> v{next}', { current: currentVersion, next: update.version })
-        updateState.nextVersion = update.version
-        updateState.status = 'downloading'
-        await invoke('download_update', { url: update.url, signature: update.signature })
-        updateState.status = 'installing'
-        await invoke('install_update')
-        log.info('v{version} installed, restart to apply', { version: update.version })
-        updateState.status = 'ready'
-        updateState.update = update
-        showUpdateToast()
-      } else {
-        log.debug('v{version} is up to date', { version: currentVersion })
-        updateState.status = 'idle'
-        updateState.nextVersion = null
-      }
-    } else {
-      // Non-macOS: delegate to the Tauri updater plugin. `downloadAndInstall()` is a single fused
-      // call, so we can't expose a separate `installing` phase here — we stay in `downloading`
-      // throughout. The Settings/menu UIs accept this asymmetry.
-      const { check } = await import('@tauri-apps/plugin-updater')
-      const update = await check()
-
-      if (update) {
-        log.info('Update available: v{current} -> v{next}', { current: currentVersion, next: update.version })
-        updateState.nextVersion = update.version
-        updateState.status = 'downloading'
-        await update.downloadAndInstall()
-        log.info('v{version} installed, restart to apply', { version: update.version })
-        updateState.status = 'ready'
-        updateState.update = { version: update.version, url: '', signature: '' }
-        showUpdateToast()
-      } else {
-        log.debug('v{version} is up to date', { version: currentVersion })
-        updateState.status = 'idle'
-        updateState.nextVersion = null
-      }
-    }
+    update = await invoke<UpdateInfo | null>('check_for_update')
   } catch (error) {
-    updateState.status = 'idle'
-    updateState.nextVersion = null
-    updateState.error = error instanceof Error ? error.message : String(error)
-    log.error('Check failed: {error}', { error: updateState.error })
+    finishCheckWithFailure(error, 'check')
+    return
+  }
+
+  if (update === null) {
+    finishCheckWithNoUpdate(currentVersion)
+    return
+  }
+
+  log.info('Update available: v{current} -> v{next}', { current: currentVersion, next: update.version })
+  updateState.nextVersion = update.version
+  updateState.status = 'downloading'
+
+  try {
+    await invoke('download_update', { url: update.url, signature: update.signature })
+    updateState.status = 'installing'
+    await invoke('install_update')
+  } catch (error) {
+    finishCheckWithFailure(error, 'download-install')
+    return
+  }
+
+  log.info('v{version} installed, restart to apply', { version: update.version })
+  updateState.status = 'ready'
+  updateState.update = update
+  showUpdateToast()
+}
+
+/**
+ * Non-macOS path: Tauri updater plugin. `downloadAndInstall()` is fused so we stay in
+ * `downloading` throughout the second phase (no separate `installing` state).
+ */
+async function runPluginUpdateFlow(currentVersion: string): Promise<void> {
+  let update: Awaited<ReturnType<typeof import('@tauri-apps/plugin-updater').check>>
+  try {
+    const { check } = await import('@tauri-apps/plugin-updater')
+    update = await check()
+  } catch (error) {
+    finishCheckWithFailure(error, 'check')
+    return
+  }
+
+  if (!update) {
+    finishCheckWithNoUpdate(currentVersion)
+    return
+  }
+
+  log.info('Update available: v{current} -> v{next}', { current: currentVersion, next: update.version })
+  updateState.nextVersion = update.version
+  updateState.status = 'downloading'
+
+  try {
+    await update.downloadAndInstall()
+  } catch (error) {
+    finishCheckWithFailure(error, 'download-install')
+    return
+  }
+
+  log.info('v{version} installed, restart to apply', { version: update.version })
+  updateState.status = 'ready'
+  updateState.update = { version: update.version, url: '', signature: '' }
+  showUpdateToast()
+}
+
+function finishCheckWithNoUpdate(currentVersion: string): void {
+  log.debug('v{version} is up to date', { version: currentVersion })
+  updateState.status = 'idle'
+  updateState.nextVersion = null
+}
+
+/**
+ * Reset state and log the failure at the right level for the phase.
+ *
+ * - `'check'` failures (network, DNS, bad manifest) are transient and expected on the periodic
+ *   background tick — log at warn so they don't trip the auto error reporter on a momentary blip.
+ * - `'download-install'` failures (signature mismatch, FS errors, partial writes) reach a code
+ *   path the user already opted into — log at error so they DO trip auto-report. The Settings
+ *   UI surfaces both via `updateState.error` regardless of log level.
+ *
+ * See `apps/desktop/src-tauri/src/error_reporter/CLAUDE.md` § convention.
+ */
+function finishCheckWithFailure(error: unknown, phase: 'check' | 'download-install'): void {
+  updateState.status = 'idle'
+  updateState.nextVersion = null
+  updateState.error = error instanceof Error ? error.message : String(error)
+  if (phase === 'check') {
+    log.warn('Check failed: {error}', { error: updateState.error })
+  } else {
+    log.error('Download/install failed: {error}', { error: updateState.error })
   }
 }
 
