Fix: a send timeout was classified as our bug, not the network's

Review caught a real defect, and it reintroduced the exact thing #825 added a
test for.

The timeout borrowed `TelemetryError::Serialize` for want of a better variant.
`classify` treats anything that is not the client's own error as a refusal — sound
for a payload that never left the process, wrong for a timeout, which *did* reach
the network and simply got no verdict. So a slow request filed a false fault and
**spent the one refusal report a run is allowed**, swallowing any genuine refusal
later in the same run.

The per-variant tests passed throughout, because the variant they exercised was
not the one the timeout used. That is the more useful lesson than the bug: the
coverage was per-error-value, and the defect was in which value got chosen.

- A `SendTimeout` variant of its own, since classification follows from the
  variant.
- `classify` is now exhaustive over our own errors as well as the client's, so a
  new one cannot inherit an answer by falling through — which is precisely how
  this happened.
- The existing refusal-report test now runs both kinds of not-reaching-the-API,
  so the property is pinned against the shape that broke it rather than only the
  shape I thought of first.

just lint 0, cargo fmt --check 0, 322 quilt-sync tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: fiskus

quilt-sync/src-tauri/src/error.rs

@@ -50,6 +50,16 @@ pub enum TelemetryError {
 
     #[error("Mixpanel serialization error: {0}")]
     Serialize(String),
+
+    /// A send abandoned before the ingest API answered.
+    ///
+    /// Its own variant rather than borrowing another, because how a failure is
+    /// *classified* follows from its variant: a timeout reached the network and got
+    /// no verdict, which is the user's connection and not our bug. Filed under a
+    /// serialization error it would read as a refusal, raise a false fault, and
+    /// spend the one refusal report a run is allowed.
+    #[error("Mixpanel send timed out after {0}s")]
+    SendTimeout(u64),
 }
 
 #[derive(thiserror::Error, Debug)]

quilt-sync/src-tauri/src/telemetry.rs

@@ -356,20 +356,31 @@ mod tests {
 
     /// And a network failure must not consume the one refusal report — otherwise
     /// the first flaky tunnel masks a real misconfiguration for the whole run.
+    ///
+    /// Both kinds of not-reaching-the-API are exercised, because this exact property
+    /// regressed once through the kind that was missing: a send timeout borrowed the
+    /// serialization error, classified as a refusal, and spent the report.
     #[test]
     fn a_network_failure_does_not_spend_the_refusal_report() {
-        let telemetry = Telemetry::default();
-
-        telemetry.report_delivery_failure_for_test(&crate::Error::from(
-            mixpanel_rs::error::Error::ApiServerError(503),
-        ));
-        telemetry.report_delivery_failure_for_test(&refusal());
+        for network in [
+            crate::Error::from(mixpanel_rs::error::Error::ApiServerError(503)),
+            crate::Error::from(crate::error::TelemetryError::SendTimeout(10)),
+        ] {
+            let telemetry = Telemetry::default();
+
+            telemetry.report_delivery_failure_for_test(&network);
+            assert!(
+                telemetry.reported_faults().is_empty(),
+                "{network} is not a fault to report"
+            );
 
-        assert_eq!(
-            telemetry.reported_faults().len(),
-            1,
-            "the refusal still got through"
-        );
+            telemetry.report_delivery_failure_for_test(&refusal());
+            assert_eq!(
+                telemetry.reported_faults().len(),
+                1,
+                "the refusal still got through after {network}"
+            );
+        }
     }
 
     /// Tests build with `debug_assertions` on, so this pins the branch a

quilt-sync/src-tauri/src/telemetry/mixpanel.rs

@@ -121,12 +121,24 @@ impl DeliveryFailure {
     pub fn classify(err: &crate::Error) -> Self {
         use mixpanel_rs::error::Error as Mp;
 
-        let crate::Error::Telemetry(TelemetryError::Mixpanel(err)) = err else {
-            // Anything that is not the client's own error never reached the
-            // network — a payload we could not serialize is not a bad connection.
+        let crate::Error::Telemetry(err) = err else {
+            // Nothing outside telemetry reaches this, and an unrecognised failure
+            // is safer treated as ours than as the network's.
             return Self::Refused;
         };
 
+        // Exhaustive over our own errors as well as the client's, so a new one
+        // cannot inherit an answer by falling through. That is precisely how a
+        // timeout once read as a refusal.
+        let err = match err {
+            // A timeout *did* reach the network — it simply got no verdict, which
+            // is the user's connection rather than our payload.
+            TelemetryError::SendTimeout(_) => return Self::Unreachable,
+            // A payload we could not even serialize never left the process.
+            TelemetryError::Serialize(_) => return Self::Refused,
+            TelemetryError::Mixpanel(err) => err,
+        };
+
         match err {
             // A verdict came back, and it was no. `ApiClientError` is what the
             // verbose response parses a non-`1` status into.
@@ -306,12 +318,7 @@ async fn send_batch(
 
             tokio::time::timeout(SEND_TIMEOUT, mixpanel.track_batch(events))
                 .await
-                .map_err(|_| {
-                    TelemetryError::Serialize(format!(
-                        "send timed out after {}s",
-                        SEND_TIMEOUT.as_secs()
-                    ))
-                })??;
+                .map_err(|_| TelemetryError::SendTimeout(SEND_TIMEOUT.as_secs()))??;
         }
         Analytics::DryRun => {
             for queued in &batch {
@@ -699,6 +706,25 @@ mod tests {
         assert_eq!(DeliveryFailure::classify(&err), DeliveryFailure::Refused);
     }
 
+    /// A timeout *did* reach the network and got no verdict, so it is the user's
+    /// connection, not our payload.
+    ///
+    /// This regressed once and the existing coverage did not catch it: the timeout
+    /// borrowed the serialization error, which classifies as a refusal, so a slow
+    /// network filed a false fault **and spent the one refusal report a run is
+    /// allowed** — swallowing any genuine refusal later in the same run. The
+    /// per-variant tests passed throughout, because the variant they exercised was
+    /// not the one the timeout used.
+    #[test]
+    fn a_timeout_is_the_network_not_our_payload() {
+        let err = crate::Error::from(TelemetryError::SendTimeout(10));
+        assert_eq!(
+            DeliveryFailure::classify(&err),
+            DeliveryFailure::Unreachable,
+            "a timeout must not be reported as our bug"
+        );
+    }
+
     /// Autosync gets its **own** names rather than folding into the manual
     /// series. Pinned here because that was a deliberate call: adding background
     /// work to `package_published` would silently redefine a series already being