fix: handle concurrent OTP signup email unique race

internal/api/errors.go

@@ -2,15 +2,19 @@ package api
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 	"os"
 	"runtime/debug"
 	"time"
 
-	"github.com/pkg/errors"
+	"github.com/jackc/pgconn"
+	"github.com/jackc/pgerrcode"
+	pkgerr "github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"github.com/supabase/auth/internal/api/apierrors"
+	"github.com/supabase/auth/internal/models"
 	"github.com/supabase/auth/internal/observability"
 	"github.com/supabase/auth/internal/utilities"
 )
@@ -22,9 +26,29 @@ const (
 )
 
 var (
-	UserExistsError error = errors.New("user already exists")
+	UserExistsError error = pkgerr.New("user already exists")
 )
 
+// isUniqueConstraintError reports whether err is (or wraps) a Postgres unique
+// violation, including HTTPError wrappers from signupNewUser.
+func isUniqueConstraintError(err error) bool {
+	if err == nil {
+		return false
+	}
+	if models.IsUniqueConstraintViolatedError(err) {
+		return true
+	}
+	var httpErr *apierrors.HTTPError
+	if errors.As(err, &httpErr) && httpErr.InternalError != nil {
+		err = httpErr.InternalError
+	}
+	var pgErr *pgconn.PgError
+	if errors.As(err, &pgErr) {
+		return pgErr.Code == pgerrcode.UniqueViolation
+	}
+	return false
+}
+
 const InvalidChannelError = "Invalid channel, supported values are 'sms' or 'whatsapp'. 'whatsapp' is only supported if Twilio or Twilio Verify is used as the provider."
 
 var oauthErrorMap = map[int]string{

internal/api/otp_test.go

@@ -310,3 +310,37 @@ func (ts *OtpTestSuite) TestSubsequentOtp() {
 	require.Empty(ts.T(), user.RecoverySentAt)
 	require.Empty(ts.T(), user.EmailConfirmedAt)
 }
+
+// TestConcurrentOtpSignupSameEmail ensures a race on users_email_partial_key
+// does not surface as a raw 500 (issue #2675).
+func (ts *OtpTestSuite) TestConcurrentOtpSignupSameEmail() {
+	ts.Config.SMTP.MaxFrequency = 0
+	email := "concurrent-otp-race@example.com"
+
+	const n = 8
+	codes := make(chan int, n)
+	for i := 0; i < n; i++ {
+		go func() {
+			var buffer bytes.Buffer
+			require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{
+				"email":       email,
+				"create_user": true,
+			}))
+			req := httptest.NewRequest(http.MethodPost, "/otp", &buffer)
+			req.Header.Set("Content-Type", "application/json")
+			w := httptest.NewRecorder()
+			ts.API.handler.ServeHTTP(w, req)
+			codes <- w.Code
+		}()
+	}
+
+	for i := 0; i < n; i++ {
+		code := <-codes
+		require.NotEqual(ts.T(), http.StatusInternalServerError, code, "losing concurrent OTP must not return 500")
+		require.Equal(ts.T(), http.StatusOK, code)
+	}
+
+	user, err := models.FindUserByEmailAndAudience(ts.API.db, email, ts.Config.JWT.Aud)
+	require.NoError(ts.T(), err)
+	require.NotNil(ts.T(), user)
+}

internal/api/signup.go

@@ -188,94 +188,114 @@ func (a *API) Signup(w http.ResponseWriter, r *http.Request) error {
 		}
 	}
 
-	err = db.Transaction(func(tx *storage.Connection) error {
-		var terr error
-		if user != nil {
-			if (params.Provider == "email" && user.IsConfirmed()) || (params.Provider == "phone" && user.IsPhoneConfirmed()) {
-				return UserExistsError
-			}
-			// do not update the user because we can't be sure of their claimed identity
-		} else {
-			user, terr = a.signupNewUser(tx, signupUser)
-			if terr != nil {
-				return terr
-			}
-		}
-		identity, terr := models.FindIdentityByIdAndProvider(tx, user.ID.String(), params.Provider)
-		if terr != nil {
-			if !models.IsNotFoundError(terr) {
-				return terr
-			}
-			identityData := structs.Map(provider.Claims{
-				Subject: user.ID.String(),
-				Email:   user.GetEmail(),
-			})
-			for k, v := range params.Data {
-				if _, ok := identityData[k]; !ok {
-					identityData[k] = v
+	for attempt := 0; attempt < 2; attempt++ {
+		err = db.Transaction(func(tx *storage.Connection) error {
+			var terr error
+			if user != nil {
+				if (params.Provider == "email" && user.IsConfirmed()) || (params.Provider == "phone" && user.IsPhoneConfirmed()) {
+					return UserExistsError
 				}
-			}
-			identity, terr = a.createNewIdentity(tx, user, params.Provider, identityData)
-			if terr != nil {
-				return terr
-			}
-			if terr := user.RemoveUnconfirmedIdentities(tx, identity); terr != nil {
-				return terr
-			}
-		}
-		user.Identities = []models.Identity{*identity}
-
-		if params.Provider == "email" && !user.IsConfirmed() {
-			if config.Mailer.Autoconfirm {
-				if terr = models.NewAuditLogEntry(config.AuditLog, r, tx, user, models.UserSignedUpAction, "", map[string]interface{}{
-					"provider": params.Provider,
-				}); terr != nil {
+				// do not update the user because we can't be sure of their claimed identity
+			} else {
+				user, terr = a.signupNewUser(tx, signupUser)
+				if terr != nil {
 					return terr
 				}
-				if terr = user.Confirm(tx); terr != nil {
-					return apierrors.NewInternalServerError("Database error updating user").WithInternalError(terr)
-				}
-			} else {
-				if terr = models.NewAuditLogEntry(config.AuditLog, r, tx, user, models.UserConfirmationRequestedAction, "", map[string]interface{}{
-					"provider": params.Provider,
-				}); terr != nil {
+			}
+			identity, terr := models.FindIdentityByIdAndProvider(tx, user.ID.String(), params.Provider)
+			if terr != nil {
+				if !models.IsNotFoundError(terr) {
 					return terr
 				}
-				if isPKCEFlow(flowType) {
-					_, terr := generateFlowState(tx, params.Provider, models.EmailSignup, params.CodeChallengeMethod, params.CodeChallenge, &user.ID)
-					if terr != nil {
-						return terr
+				identityData := structs.Map(provider.Claims{
+					Subject: user.ID.String(),
+					Email:   user.GetEmail(),
+				})
+				for k, v := range params.Data {
+					if _, ok := identityData[k]; !ok {
+						identityData[k] = v
 					}
 				}
-				if terr = a.sendConfirmation(r, tx, user, flowType); terr != nil {
+				identity, terr = a.createNewIdentity(tx, user, params.Provider, identityData)
+				if terr != nil {
 					return terr
 				}
-			}
-		} else if params.Provider == "phone" && !user.IsPhoneConfirmed() {
-			if config.Sms.Autoconfirm {
-				if terr = models.NewAuditLogEntry(config.AuditLog, r, tx, user, models.UserSignedUpAction, "", map[string]interface{}{
-					"provider": params.Provider,
-					"channel":  params.Channel,
-				}); terr != nil {
+				if terr := user.RemoveUnconfirmedIdentities(tx, identity); terr != nil {
 					return terr
 				}
-				if terr = user.ConfirmPhone(tx); terr != nil {
-					return apierrors.NewInternalServerError("Database error updating user").WithInternalError(terr)
-				}
-			} else {
-				if terr = models.NewAuditLogEntry(config.AuditLog, r, tx, user, models.UserConfirmationRequestedAction, "", map[string]interface{}{
-					"provider": params.Provider,
-				}); terr != nil {
-					return terr
+			}
+			user.Identities = []models.Identity{*identity}
+
+			if params.Provider == "email" && !user.IsConfirmed() {
+				if config.Mailer.Autoconfirm {
+					if terr = models.NewAuditLogEntry(config.AuditLog, r, tx, user, models.UserSignedUpAction, "", map[string]interface{}{
+						"provider": params.Provider,
+					}); terr != nil {
+						return terr
+					}
+					if terr = user.Confirm(tx); terr != nil {
+						return apierrors.NewInternalServerError("Database error updating user").WithInternalError(terr)
+					}
+				} else {
+					if terr = models.NewAuditLogEntry(config.AuditLog, r, tx, user, models.UserConfirmationRequestedAction, "", map[string]interface{}{
+						"provider": params.Provider,
+					}); terr != nil {
+						return terr
+					}
+					if isPKCEFlow(flowType) {
+						_, terr := generateFlowState(tx, params.Provider, models.EmailSignup, params.CodeChallengeMethod, params.CodeChallenge, &user.ID)
+						if terr != nil {
+							return terr
+						}
+					}
+					if terr = a.sendConfirmation(r, tx, user, flowType); terr != nil {
+						return terr
+					}
 				}
-				if _, terr := a.sendPhoneConfirmation(r, tx, user, params.Phone, phoneConfirmationOtp, params.Channel); terr != nil {
-					return terr
+			} else if params.Provider == "phone" && !user.IsPhoneConfirmed() {
+				if config.Sms.Autoconfirm {
+					if terr = models.NewAuditLogEntry(config.AuditLog, r, tx, user, models.UserSignedUpAction, "", map[string]interface{}{
+						"provider": params.Provider,
+						"channel":  params.Channel,
+					}); terr != nil {
+						return terr
+					}
+					if terr = user.ConfirmPhone(tx); terr != nil {
+						return apierrors.NewInternalServerError("Database error updating user").WithInternalError(terr)
+					}
+				} else {
+					if terr = models.NewAuditLogEntry(config.AuditLog, r, tx, user, models.UserConfirmationRequestedAction, "", map[string]interface{}{
+						"provider": params.Provider,
+					}); terr != nil {
+						return terr
+					}
+					if _, terr := a.sendPhoneConfirmation(r, tx, user, params.Phone, phoneConfirmationOtp, params.Channel); terr != nil {
+						return terr
+					}
 				}
 			}
-		}
 
-		return nil
-	})
+			return nil
+		})
+
+		if err == nil || errors.Is(err, UserExistsError) || !isUniqueConstraintError(err) {
+			break
+		}
+		// Concurrent signup for the same email/phone won the insert. Reload the
+		// user and retry once on the existing-user path (same as a later client retry).
+		var findErr error
+		switch params.Provider {
+		case "email":
+			user, findErr = models.FindUserByEmailAndAudience(db, params.Email, params.Aud)
+		case "phone":
+			user, findErr = models.FindUserByPhoneAndAudience(db, params.Phone, params.Aud)
+		default:
+			return err
+		}
+		if findErr != nil {
+			return apierrors.NewInternalServerError("Database error finding user").WithInternalError(findErr)
+		}
+	}
 
 	if err != nil {
 		if errors.Is(err, UserExistsError) {
@@ -383,6 +403,9 @@ func (a *API) signupNewUser(conn *storage.Connection, user *models.User) (*model
 	err := conn.Transaction(func(tx *storage.Connection) error {
 		var terr error
 		if terr = tx.Create(user); terr != nil {
+			if isUniqueConstraintError(terr) {
+				return models.UserEmailUniqueConflictError{}
+			}
 			return apierrors.NewInternalServerError("Database error saving new user").WithInternalError(terr)
 		}
 		if terr = user.SetRole(tx, config.JWT.DefaultGroupName); terr != nil {

internal/api/signup_test.go

@@ -169,3 +169,16 @@ func (ts *SignupTestSuite) TestSignupRequestBodyTooLarge() {
 	require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&data))
 	require.Equal(ts.T(), "request_entity_too_large", data["error_code"])
 }
+
+func (ts *SignupTestSuite) TestSignupNewUserUniqueEmailConflict() {
+	u1, err := models.NewUser("", "dup@example.com", "password123", ts.Config.JWT.Aud, nil)
+	require.NoError(ts.T(), err)
+	_, err = ts.API.signupNewUser(ts.API.db, u1)
+	require.NoError(ts.T(), err)
+
+	u2, err := models.NewUser("", "dup@example.com", "password123", ts.Config.JWT.Aud, nil)
+	require.NoError(ts.T(), err)
+	_, err = ts.API.signupNewUser(ts.API.db, u2)
+	require.True(ts.T(), models.IsUniqueConstraintViolatedError(err), "got: %v", err)
+	require.True(ts.T(), isUniqueConstraintError(err))
+}