Better error for failed emails

client/www/app/docs/self-hosting/page.md

@@ -47,6 +47,14 @@ INSTANT_TEAM_EMAIL_SENDER_EMAIL=teams@example.com
 Restart the backend and try logging in to the dashboard. If all goes right, you
 should get an email delivered!
 
+{% callout type="warning" %}
+
+New Postmark accounts start in a pending-approval state and can only send to
+addresses on your own domain. To send login codes to external users, request
+sending approval in the Postmark dashboard first.
+
+{% /callout %}
+
 ### Configure email with SendGrid
 
 You can use [SendGrid](https://sendgrid.com/) instead of Postmark. Create an API

server/src/instant/postmark.clj

@@ -28,6 +28,56 @@
   (= 504
      (-> e ex-data :body :ErrorCode)))
 
+;; A Postmark account that hasn't finished the approval process yet.
+;; 412 = account pending approval (can only send to same-domain recipients),
+;; 413 = account not approved for sending at all.
+;; C.F https://postmarkapp.com/developer/api/overview#error-codes
+(defn account-not-approved? [e]
+  (contains? #{412 413}
+             (-> e ex-data :body :ErrorCode)))
+
+;; Sender-signature problems. We deliberately re-throw these raw (rather than
+;; translating them) so callers like magic-code-auth can catch them and retry
+;; with the default sender. See `instant.runtime.magic-code-auth/invalid-sender?`.
+(def unconfirmed-sender-error-code 400)
+(def sender-not-found-error-code 401)
+
+(defn sender-signature-problem? [e]
+  (contains? #{unconfirmed-sender-error-code sender-not-found-error-code}
+             (-> e ex-data :body :ErrorCode)))
+
+(defn throw-send-error!
+  "Translates a failed Postmark send into a typed instant-exception with a
+   human-readable message, so the client gets a real error instead of a generic
+   500. Sender-signature errors are re-thrown raw so callers can fall back to
+   the default sender. The provider's own error code/message are recorded to the
+   trace for debugging, not surfaced to the client."
+  [e to]
+  (if (sender-signature-problem? e)
+    (throw e)
+    (do
+      (tracer/add-data! {:attributes {:postmark-error-code (-> e ex-data :body :ErrorCode)
+                                      :postmark-error-message (-> e ex-data :body :Message)}})
+      (cond
+        (inactive-recipient? e)
+        (ex/throw-email-send-failed!
+         (format "We couldn't deliver the email to %s. The address has been marked as inactive and can't receive mail." to)
+         {:recipient to
+          :recipient-problem? true}
+         e)
+
+        (account-not-approved? e)
+        (ex/throw-email-send-failed!
+         "The Postmark account isn't approved for sending yet."
+         {:recipient to}
+         e)
+
+        :else
+        (ex/throw-email-send-failed!
+         "We weren't able to send the email."
+         {:recipient to}
+         e)))))
+
 ;; --------
 ;; API
 
@@ -63,12 +113,7 @@
                       "Content-Type" "application/json"}
             :body (->json body)})
           (catch Exception e
-            (if (inactive-recipient? e)
-              (ex/throw-validation-err!
-               :email
-               to
-               [{:message "This email address has been marked inactive."}])
-              (throw e))))))))
+            (throw-send-error! e to)))))))
 
 (comment
   (send! {:from "verify@dash-pm.instantdb.com"

server/src/instant/runtime/magic_code_auth.clj

@@ -75,14 +75,16 @@
                                          :source "bucket4j"}})
       (ex/throw-record-email-rate-limited!))))
 
-(def postmark-unconfirmed-sender-body-error-code 400)
-
-(def postmark-not-found-sender-body-error-code 401)
-
 (defn invalid-sender? [e]
-  (let [code (-> e ex-data :body :ErrorCode)]
-    (or (= code postmark-unconfirmed-sender-body-error-code)
-        (= code postmark-not-found-sender-body-error-code))))
+  (postmark/sender-signature-problem? e))
+
+(defn suppress-send-failure-for-test-user?
+  "For test users we don't care whether the email actually gets delivered, so a
+   validation/send failure should be swallowed instead of thrown."
+  [e req]
+  (and (contains? #{::ex/validation-failed ::ex/email-send-failed}
+                  (-> e ex-data ::ex/type))
+       (some? (app-model/get-test-user req))))
 
 (defn default-body [{:keys [app_title code expiration]}]
   (postmark/standard-body
@@ -219,11 +221,16 @@
                               (invalid-sender? e)
                               (do
                                 (tracer/record-info! {:name "magic-code/unconfirmed-or-unknown-sender" :attributes {:email sender-email :app-id app-id}})
-                                (email-router/send-structured! (magic-code-email email (assoc email-params :sender-email default-sender-email))))
+                                (try
+                                  (email-router/send-structured! (magic-code-email email (assoc email-params :sender-email default-sender-email)))
+                                  (catch clojure.lang.ExceptionInfo fallback-e
+                                    ;; Don't throw if it's a test user, even if the fallback send fails
+                                    (if (suppress-send-failure-for-test-user? fallback-e req)
+                                      false
+                                      (throw fallback-e)))))
 
                               ;; Don't throw if it's a test user, even if we can't send email to it
-                              (and (= ::ex/validation-failed (-> e ex-data ::ex/type))
-                                   (not (nil? (app-model/get-test-user req))))
+                              (suppress-send-failure-for-test-user? e req)
                               false
 
                               :else

server/src/instant/sendgrid.clj

@@ -1,11 +1,35 @@
 (ns instant.sendgrid
   (:require
    [clj-http.client :as clj-http]
-   [clojure.data.json :as json]
    [instant.config :as config]
+
+   [instant.util.exception :as ex]
+   [instant.util.json :refer [->json <-json]]
    [instant.util.tracer :as tracer]
    [instant.postmark :as postmark]))
 
+(defn error-detail
+  "Best-effort extraction of SendGrid's first error message. Unlike Postmark, the
+   SendGrid request isn't sent with `:as :json`, so the error body is a raw JSON
+   string like {\"errors\":[{\"message\":\"...\"}]}."
+  [e]
+  (try
+    (-> e ex-data :body (<-json true) :errors first :message)
+    (catch Exception _ nil)))
+
+(defn throw-send-error!
+  "Translates a failed SendGrid send into a typed instant-exception with a
+   human-readable message, so the client gets a real error instead of a generic
+   500. The provider's status/error text are recorded to the trace for
+   debugging, not surfaced to the client."
+  [e to]
+  (tracer/add-data! {:attributes {:sendgrid-status (-> e ex-data :status)
+                                  :sendgrid-error (error-detail e)}})
+  (ex/throw-email-send-failed!
+   "We weren't able to send the email."
+   {:recipient (-> to first :email)}
+   e))
+
 (defn send! [{:keys [from to cc bcc subject html reply-to]}]
   (let [personalization (cond-> {:to to}
                           cc (assoc :cc cc)
@@ -28,11 +52,14 @@
       (tracer/with-span!
         {:name "sendgrid/send"
          :attributes {:body body}}
-        (clj-http/post
-         "https://api.sendgrid.com/v3/mail/send"
-         {:headers {"Authorization" (str "Bearer " (config/sendgrid-token))
-                    "Content-Type" "application/json"}
-          :body (json/write-str body)})))))
+        (try
+          (clj-http/post
+           "https://api.sendgrid.com/v3/mail/send"
+           {:headers {"Authorization" (str "Bearer " (config/sendgrid-token))
+                      "Content-Type" "application/json"}
+            :body (->json body)})
+          (catch Exception e
+            (throw-send-error! e to)))))))
 
 (comment
   (send! {:from {:email "verify@auth-sg.instantdb.com"}

server/src/instant/util/exception.clj

@@ -38,6 +38,7 @@
                 ::param-malformed
 
                 ::validation-failed
+                ::email-send-failed
                 ::operation-timed-out
                 ::rate-limited
                 ::parameter-limit-exceeded
@@ -357,6 +358,24 @@
   (when (seq errors)
     (throw-validation-err! input-type input errors)))
 
+;; -----
+;; Email
+
+(defn throw-email-send-failed!
+  "Raised when an email provider (Postmark/SendGrid) rejects or fails a send.
+   `::email-send-failed` is intentionally not a bad-request type, so it renders
+   as a 500 with a clear, typed message instead of the generic \"Something went
+   wrong\". The hint is returned to the client, so keep it free of provider
+   internals (record those on the trace instead); `cause` should be the original
+   provider exception when available."
+  ([message] (throw-email-send-failed! message nil nil))
+  ([message hint] (throw-email-send-failed! message hint nil))
+  ([message hint cause]
+   (throw+ {::type ::email-send-failed
+            ::message message
+            ::hint hint}
+           cause)))
+
 ;; ------
 ;; Params
 

server/test/instant/runtime/routes_test.clj

@@ -7,6 +7,7 @@
    [instant.db.model.attr :as attr-model]
    [instant.db.model.triple :as triples]
    [instant.db.permissioned-transaction :as permissioned-tx]
+   [instant.email-router :as email-router]
    [instant.fixtures :refer [random-email with-empty-app]]
    [instant.flags :as flags]
    [instant.isn :as isn]
@@ -29,6 +30,7 @@
    [instant.system-catalog :as system-catalog]
    [instant.util.coll :as coll]
    [instant.util.crypt :as crypt-util]
+   [instant.util.exception :as ex]
    [instant.util.json :refer [->json <-json]]
    [instant.util.test :as test-util]
    [instant.util.tracer :as tracer])
@@ -226,6 +228,28 @@
             (is (= custom-email
                    (get-in @letter [:from :email])))))))))
 
+(deftest magic-code-invalid-sender-fallback-failure-suppressed-for-test-user
+  (with-empty-app
+    (fn [{app-id :id}]
+      (let [email "test@example.com"
+            calls (atom 0)
+            ;; First send fails with an invalid-sender error (triggers the
+            ;; default-sender fallback); the fallback send then also fails.
+            fake-send (fn [_req]
+                        (if (= 1 (swap! calls inc))
+                          (throw (ex-info "invalid sender" {:body {:ErrorCode 400}}))
+                          (ex/throw-email-send-failed! "fallback boom" {:recipient email})))]
+        (app-model/create-test-user! {:app-id app-id
+                                      :email email
+                                      :code "424242"})
+        (testing "a test user's failing fallback send is swallowed, not thrown"
+          (with-redefs [magic-code-auth/check-send-rate-limit! (constantly nil)
+                        email-router/send-structured! fake-send]
+            (let [res (magic-code-auth/send! {:app-id app-id :email email})]
+              ;; fallback was attempted, and the failure didn't propagate
+              (is (= 2 @calls))
+              (is (false? (:sent-email res))))))))))
+
 (defn update-created-at [app-id code created-at]
   (sql/execute!
    (aurora/conn-pool :write)