From 3e41b6ea40b9204a0b839ddb08cf0c6416faee5b Mon Sep 17 00:00:00 2001
From: Philip Jenvey <pjenvey@underboss.org>
Date: Mon, 1 May 2023 11:55:32 -0700
Subject: [PATCH] fix: don't eat WebpushSocket poll_complete errors

instead check specifically for the harmless connection closed error and don't
log it

also remove the stacktrace from ApcError::to_string, include it in the sentry
event exception instead

Issue SYNC-3444
---
 autopush-common/src/errors.rs | 12 +-----------
 autopush/src/client.rs        |  3 +++
 autopush/src/server/mod.rs    | 19 ++++++++-----------
 3 files changed, 12 insertions(+), 22 deletions(-)

diff --git a/autopush-common/src/errors.rs b/autopush-common/src/errors.rs
index 8aa13d399..4eef2bbd2 100644
--- a/autopush-common/src/errors.rs
+++ b/autopush-common/src/errors.rs
@@ -31,19 +31,9 @@ pub struct ApcError {
     pub backtrace: Box<Backtrace>,
 }
 
-// Print out the error and backtrace, including source errors
 impl Display for ApcError {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(f, "Error: {}\nBacktrace: \n{:?}", self.kind, self.backtrace)?;
-
-        // Go down the chain of errors
-        let mut error: &dyn std::error::Error = &self.kind;
-        while let Some(source) = error.source() {
-            write!(f, "\n\nCaused by: {source}")?;
-            error = source;
-        }
-
-        Ok(())
+        self.kind.fmt(f)
     }
 }
 
diff --git a/autopush/src/client.rs b/autopush/src/client.rs
index edd7846e2..265b9d170 100644
--- a/autopush/src/client.rs
+++ b/autopush/src/client.rs
@@ -638,6 +638,9 @@ where
         let error = if let Some(ref err) = error {
             let ua_info = ua_info.clone();
             let mut event = sentry::event_from_error(err);
+            event.exception.last_mut().unwrap().stacktrace =
+                sentry::integrations::backtrace::backtrace_to_stacktrace(&err.backtrace);
+
             event.user = Some(sentry::User {
                 id: Some(webpush.uaid.as_simple().to_string()),
                 ..Default::default()
diff --git a/autopush/src/server/mod.rs b/autopush/src/server/mod.rs
index dfcfca6e4..c2e165de7 100644
--- a/autopush/src/server/mod.rs
+++ b/autopush/src/server/mod.rs
@@ -406,12 +406,14 @@ impl Server {
                     handle.spawn(client.then(move |res| {
                         srv.open_connections.set(srv.open_connections.get() - 1);
                         if let Err(e) = res {
-                            let mut errstr: &str = &e.to_string();
-                            if errstr.contains("Connection closed normally") {
-                                // we don't need the stack for a Success Error.
-                                errstr = errstr.split('\n').collect::<Vec<&str>>()[0];
+                            // ConnectionClosed denotes the connection closed
+                            // normally, so no need to log it
+                            if !matches!(
+                                e.kind,
+                                ApcErrorKind::Ws(tungstenite::error::Error::ConnectionClosed)
+                            ) {
+                                debug!("🤫 {}: {}", addr, e.to_string());
                             }
-                            debug!("🤫 {}: {}", addr, errstr);
                         }
                         Ok(())
                     }));
@@ -909,14 +911,9 @@ where
     }
 
     /// Handle the poll completion.
-    /// Note, that this eats the errors of poll_complete(), because one of the known states
-    /// is "Error: Connection closed normally" which can raise a false error condition.
     fn poll_complete(&mut self) -> Poll<(), ApcError> {
         try_ready!(self.send_ws_ping());
-        if self.inner.poll_complete().is_err() {
-            warn!("Error encountered with poll_complete");
-        }
-        Ok(Async::Ready(()))
+        Ok(self.inner.poll_complete()?)
     }
 
     fn close(&mut self) -> Poll<(), ApcError> {