Recognize more common connection errors (#2858)

After doing a pass on Sentry issues I've added a bunch more patterns to
db_connection_error.ex.

As we see more errors recurring in Sentry, we should add them to
patterns in DbConnectionError so as not to treat them as unknown
anymore.

---------

Co-authored-by: msfstef <msfstef@gmail.com>

Author: alco

.changeset/many-rocks-provide.md

@@ -0,0 +1,5 @@
+---
+"@core/sync-service": patch
+---
+
+Recognize additional common connection errors and handle appropriately.

packages/sync-service/lib/electric/connection/manager.ex

@@ -600,13 +600,13 @@ defmodule Electric.Connection.Manager do
     # Try repairing the connection opts and try connecting again. If we're already using noSSL
     # and IPv4, the error will be propagated to a `shutdown_or_reconnect()` function call
     # further down below.
-    error = strip_exit_signal_stacktrace(reason)
     state = nillify_pid(state, pid)
+
     {step, _} = state.current_step
     conn_opts = connection_opts_for_step(step, state)
 
     repaired_conn_opts =
-      case error do
+      case reason do
         %Postgrex.Error{message: "ssl not available"} ->
           maybe_fallback_to_no_ssl(conn_opts)
 
@@ -625,7 +625,7 @@ defmodule Electric.Connection.Manager do
       state = update_connection_opts(step, repaired_conn_opts, state)
       {:noreply, state, {:continue, step}}
     else
-      shutdown_or_reconnect(error, state)
+      shutdown_or_reconnect(reason, state)
     end
   end
 
@@ -638,15 +638,14 @@ defmodule Electric.Connection.Manager do
         %State{replication_client_pid: pid, current_phase: :running} = state
       ) do
     state = nillify_pid(state, pid)
-    error = strip_exit_signal_stacktrace(reason)
 
     state = %{
       state
       | current_phase: :restarting_replication_client,
         current_step: {:start_replication_client, nil}
     }
 
-    shutdown_or_reconnect(error, state)
+    shutdown_or_reconnect(reason, state)
   end
 
   # The most likely reason for any database connection to get closed after we've already opened a
@@ -972,7 +971,10 @@ defmodule Electric.Connection.Manager do
   end
 
   defp shutdown_or_reconnect(error, state) do
-    error = DbConnectionError.from_error(error)
+    error =
+      error
+      |> strip_exit_signal_stacktrace()
+      |> DbConnectionError.from_error()
 
     with false <- drop_slot_and_restart(error, state),
          false <- stop_if_fatal_error(error, state) do
@@ -1287,13 +1289,29 @@ defmodule Electric.Connection.Manager do
     :erlang.start_timer(@connection_status_check_interval, self(), {:check_status, type})
   end
 
-  # It's possible that the exit signal received from the replication client process includes a
-  # stacktrace. I haven't found the rule that would describe when the stacktrace is to be
-  # expected or not. This implementation is based on empirical evidence.
-  defp strip_exit_signal_stacktrace(signal) do
-    case signal do
-      {reason, stacktrace} when is_list(stacktrace) -> reason
-      reason -> reason
+  # It's possible that the exit signal received from an exiting process includes a
+  # stacktrace. This happens when the process crashes due an exception getting raised as
+  # opposed to exiting with a custom reason in an orderly fashion.
+  defp strip_exit_signal_stacktrace(error) do
+    case error do
+      {reason, stacktrace} when is_list(stacktrace) ->
+        if stacktrace?(stacktrace) do
+          reason
+        else
+          error
+        end
+
+      _ ->
+        error
+    end
+  end
+
+  defp stacktrace?(val) do
+    try do
+      _ = Exception.format_stacktrace(val)
+      true
+    rescue
+      _ -> false
     end
   end
 

packages/sync-service/lib/electric/db_connection_error.ex

@@ -5,12 +5,35 @@ defmodule Electric.DbConnectionError do
 
   alias Electric.DbConnectionError
 
-  def from_error(
-        %DBConnection.ConnectionError{message: "tcp recv: connection timed out - :etimedout"} =
-          error
-      ) do
+  def from_error(%DBConnection.ConnectionError{message: message} = error)
+      when message in [
+             "tcp recv (idle): closed",
+             "ssl recv (idle): closed",
+             "tcp recv: closed",
+             "ssl recv: closed",
+             "tcp async recv: closed",
+             "ssl async recv: closed",
+             "tcp async_recv: closed",
+             "ssl async_recv: closed"
+           ] do
+    %DbConnectionError{
+      message: "connection closed while connecting to the database",
+      type: :connection_closed,
+      original_error: error,
+      retry_may_fix?: true
+    }
+  end
+
+  def from_error(%DBConnection.ConnectionError{message: message} = error)
+      when message in [
+             "tcp recv: connection timed out - :etimedout",
+             "tcp recv (idle): timeout",
+             "ssl recv (idle): timeout",
+             "tcp recv: timeout",
+             "ssl recv: timeout"
+           ] do
     %DbConnectionError{
-      message: "connection timed out while trying to connect to the database",
+      message: "connection timed out",
       type: :connection_timeout,
       original_error: error,
       retry_may_fix?: true
@@ -19,13 +42,11 @@ defmodule Electric.DbConnectionError do
 
   def from_error(%DBConnection.ConnectionError{message: message} = error)
       when message in [
-             "tcp recv (idle): closed",
-             "ssl recv (idle): closed",
-             "tcp recv: closed",
-             "ssl recv: closed"
+             "tcp send: closed",
+             "ssl send: closed"
            ] do
     %DbConnectionError{
-      message: "connection closed while connecting to the database",
+      message: "connection closed while talking to the database",
       type: :connection_closed,
       original_error: error,
       retry_may_fix?: true
@@ -152,7 +173,8 @@ defmodule Electric.DbConnectionError do
   end
 
   def from_error(%Postgrex.Error{postgres: %{code: :internal_error, pg_code: "XX000"}} = error) do
-    maybe_database_does_not_exist(error) || unknown_error(error)
+    maybe_database_does_not_exist(error) || maybe_compute_quota_exceeded(error) ||
+      unknown_error(error)
   end
 
   def from_error(
@@ -212,6 +234,21 @@ defmodule Electric.DbConnectionError do
     end
   end
 
+  defp maybe_compute_quota_exceeded(error) do
+    case error.postgres.message do
+      "Your account or project has exceeded the compute time quota" <> _ ->
+        %DbConnectionError{
+          message: error.postgres.message,
+          type: :compute_quota_exceeded,
+          original_error: error,
+          retry_may_fix?: false
+        }
+
+      _ ->
+        nil
+    end
+  end
+
   defp maybe_nxdomain_error(error) do
     ~r/\((?<domain>[^:]+).*\): non-existing domain - :nxdomain/
     |> Regex.named_captures(error.message)

packages/sync-service/lib/electric/postgres/replication_client.ex

@@ -221,6 +221,11 @@ defmodule Electric.Postgres.ReplicationClient do
     {:disconnect, :shutdown}
   end
 
+  # Some other exit reason we're not expecting: disconnect and shut down.
+  def handle_info({:EXIT, _pid, reason}, _state) do
+    {:disconnect, reason}
+  end
+
   # The implementation of Postgrex.ReplicationConnection doesn't give us a convenient way to
   # check whether the START_REPLICATION_SLOT statement succeeded before switching the
   # connection into streaming mode. Returning {:query, "START_REPLICATION_SLOT ...", state}

packages/sync-service/test/electric/db_connection_error_test.exs

@@ -71,33 +71,47 @@ defmodule Electric.DbConnectionErrorTest do
     end
 
     test "with a tcp timeout error" do
-      error = %DBConnection.ConnectionError{
-        message: "tcp recv: connection timed out - :etimedout",
-        severity: :error,
-        reason: :error
-      }
+      for message <- [
+            "tcp recv: connection timed out - :etimedout",
+            "ssl recv (idle): timeout",
+            "tcp recv: timeout"
+          ] do
+        error = %DBConnection.ConnectionError{
+          message: message,
+          severity: :error,
+          reason: :error
+        }
 
-      assert %DbConnectionError{
-               message: "connection timed out while trying to connect to the database",
-               type: :connection_timeout,
-               original_error: error,
-               retry_may_fix?: true
-             } == DbConnectionError.from_error(error)
+        assert %DbConnectionError{
+                 message: "connection timed out",
+                 type: :connection_timeout,
+                 original_error: error,
+                 retry_may_fix?: true
+               } == DbConnectionError.from_error(error)
+      end
     end
 
     test "with tcp closed error" do
-      error = %DBConnection.ConnectionError{
-        message: "tcp recv (idle): closed",
-        severity: :error,
-        reason: :error
-      }
+      for message <- [
+            "tcp recv (idle): closed",
+            "ssl recv (idle): closed",
+            "tcp recv: closed",
+            "ssl recv: closed",
+            "ssl async_recv: closed"
+          ] do
+        error = %DBConnection.ConnectionError{
+          message: message,
+          severity: :error,
+          reason: :error
+        }
 
-      assert %DbConnectionError{
-               message: "connection closed while connecting to the database",
-               type: :connection_closed,
-               original_error: error,
-               retry_may_fix?: true
-             } == DbConnectionError.from_error(error)
+        assert %DbConnectionError{
+                 message: "connection closed while connecting to the database",
+                 type: :connection_closed,
+                 original_error: error,
+                 retry_may_fix?: true
+               } == DbConnectionError.from_error(error)
+      end
     end
 
     test "with a connection refused error" do
@@ -114,5 +128,29 @@ defmodule Electric.DbConnectionErrorTest do
                retry_may_fix?: false
              } == DbConnectionError.from_error(error)
     end
+
+    test "with compute quota exceeded error" do
+      error = %Postgrex.Error{
+        message: nil,
+        postgres: %{
+          code: :internal_error,
+          message:
+            "Your account or project has exceeded the compute time quota. Upgrade your plan to increase limits.",
+          unknown: "FATAL",
+          severity: "FATAL",
+          pg_code: "XX000"
+        },
+        connection_id: nil,
+        query: nil
+      }
+
+      assert %DbConnectionError{
+               message:
+                 "Your account or project has exceeded the compute time quota. Upgrade your plan to increase limits.",
+               type: :compute_quota_exceeded,
+               original_error: error,
+               retry_may_fix?: false
+             } == DbConnectionError.from_error(error)
+    end
   end
 end