From 369927892fde63d07b199d4e80fe05ae780573d5 Mon Sep 17 00:00:00 2001 From: Joshen Lim Date: Mon, 12 Oct 2026 11:28:12 +0800 Subject: [PATCH] Harden profile creation logic from dashboard (#51483) ## Context When signing up a new account on the dashboard, the dashboard's the one making the request to create a new profile for the user (ideally not, but the proper fix on the BE is currently blocked). We do so by making an initial request to GET `/profile`, and if it returns a 404, it's implied that the account doesn't exist and we fire `createProfile` Because of this, a race condition could exist (due to replica lag for example): - GET `/profile` returns 404, treat user as new - POST `/profile` returns 200 - Profile query is still in "error" state (`isError` is `true), and post sign-in tries to redirect to the new org page which triggers the `useEfect` [here](https://github.com/supabase/supabase/blob/master/apps/studio/lib/profile.tsx#L82-L96) - Which prompts another POST `/profile`, and it returns a 409 because the account's already created - This then keeps going in a loop which keeps calling `createProfile` and we didn't have a way to handle this, leading to users in a stuck loading state until the browser's refreshed ## Changes involved - Opt to invalidate the profile, permissions, and organizations if the user runs into a 409 - This should refresh the profile and we'll no longer be stuck in a create loop since `isError` should no longer be `true` then - Tighten the dependency array for where `createProfile` is triggered - Add exponential backoff up to 3 times for profile creation for transient failures (5xx, network, 429) - Never retry profile creation for 409, since there's no point doing so --- .../data/profile/profile-create-mutation.ts | 11 ++ apps/studio/lib/profile.test.tsx | 142 ++++++++++++++++++ apps/studio/lib/profile.tsx | 44 +++--- 3 files changed, 177 insertions(+), 20 deletions(-) create mode 100644 apps/studio/lib/profile.test.tsx diff --git a/apps/studio/data/profile/profile-create-mutation.ts b/apps/studio/data/profile/profile-create-mutation.ts index 25917ca50b3..f8317053092 100644 --- a/apps/studio/data/profile/profile-create-mutation.ts +++ b/apps/studio/data/profile/profile-create-mutation.ts @@ -28,6 +28,17 @@ export const useProfileCreateMutation = ({ return useMutation({ mutationFn: () => createProfile(), + // Retry transient failures (5xx, network, 429), but never 4xx - a 409 means the profile + // already exists, so retrying the POST can't help + retry: (failureCount, error) => { + const isClientError = error.code !== undefined && error.code >= 400 && error.code < 500 + if (isClientError && error.code !== 429) return false + return failureCount < 3 + }, + retryDelay: (failureCount, error) => { + if (error.code === 429 && error.retryAfter) return error.retryAfter * 1000 + return Math.min(1000 * 2 ** failureCount, 8000) + }, async onSuccess(data, variables, context) { await Promise.all([ queryClient.invalidateQueries({ queryKey: profileKeys.profile() }), diff --git a/apps/studio/lib/profile.test.tsx b/apps/studio/lib/profile.test.tsx new file mode 100644 index 00000000000..0caa2501c94 --- /dev/null +++ b/apps/studio/lib/profile.test.tsx @@ -0,0 +1,142 @@ +import { screen, waitFor } from '@testing-library/react' +import { HttpResponse } from 'msw' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +import { ProfileProvider, useProfile } from './profile' +import type { ProfileResponse } from '@/data/profile/profile-create-mutation' +import { customRender } from '@/tests/lib/custom-render' +import { addAPIMock, type APIErrorBody } from '@/tests/lib/msw' +import { createMockProfile } from '@/tests/lib/profile-helpers' + +vi.mock('common', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + useParams: () => ({ ref: 'default' }), + useUser: () => ({ id: 'user-1', email: 'test@example.com' }), + useIsLoggedIn: () => true, + } +}) + +vi.mock('@sentry/nextjs', () => ({ captureMessage: vi.fn(), captureException: vi.fn() })) +vi.mock('@/lib/telemetry/track', () => ({ useTrack: () => vi.fn() })) + +const profileResponse = (username?: string) => + HttpResponse.json(createMockProfile({ username })) +const errorResponse = (message: string, status: number) => + HttpResponse.json({ message }, { status }) + +const ProfileStatus = () => { + const { profile, isSuccess } = useProfile() + return
{isSuccess ? `profile:${profile?.username}` : 'no-profile'}
+} + +const renderProvider = () => + customRender( + + + + ) + +// Serves each GET response in order, repeating the last one +const mockProfileGet = (responses: Array<'not-found' | 'found'>) => { + const getSpy = vi.fn() + addAPIMock({ + method: 'get', + path: '/platform/profile', + response: () => { + const next = responses[Math.min(getSpy.mock.calls.length, responses.length - 1)] + getSpy() + return next === 'found' + ? profileResponse('new-user') + : errorResponse("User's profile not found", 404) + }, + }) + return getSpy +} + +describe('ProfileProvider profile creation', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('creates the profile once when it does not exist', async () => { + const getSpy = mockProfileGet(['not-found', 'found']) + const postSpy = vi.fn() + addAPIMock({ + method: 'post', + path: '/platform/profile', + response: () => { + postSpy() + return profileResponse() + }, + }) + + renderProvider() + + await waitFor(() => expect(screen.getByText('profile:new-user')).toBeInTheDocument()) + expect(postSpy).toHaveBeenCalledTimes(1) + expect(getSpy).toHaveBeenCalledTimes(2) + }) + + it('refetches the profile when creation returns 409', async () => { + mockProfileGet(['not-found', 'found']) + const postSpy = vi.fn() + addAPIMock({ + method: 'post', + path: '/platform/profile', + response: () => { + postSpy() + return errorResponse('Profile already exists', 409) + }, + }) + + renderProvider() + + await waitFor(() => expect(screen.getByText('profile:new-user')).toBeInTheDocument()) + expect(postSpy).toHaveBeenCalledTimes(1) + }) + + it('does not loop POSTing when the profile is still not found after a 409', async () => { + const getSpy = mockProfileGet(['not-found']) + const postSpy = vi.fn() + addAPIMock({ + method: 'post', + path: '/platform/profile', + response: () => { + postSpy() + return errorResponse('Profile already exists', 409) + }, + }) + + renderProvider() + + // Initial GET, then the refetch triggered by the 409 + await waitFor(() => expect(getSpy).toHaveBeenCalledTimes(2)) + await new Promise((resolve) => setTimeout(resolve, 300)) + expect(postSpy).toHaveBeenCalledTimes(1) + expect(getSpy).toHaveBeenCalledTimes(2) + }) + + it('retries creation on a server error', async () => { + mockProfileGet(['not-found', 'found']) + const postSpy = vi.fn() + addAPIMock({ + method: 'post', + path: '/platform/profile', + response: () => { + postSpy() + return postSpy.mock.calls.length === 1 + ? errorResponse('Internal error', 500) + : profileResponse() + }, + }) + + renderProvider() + + await waitFor(() => expect(screen.getByText('profile:new-user')).toBeInTheDocument(), { + timeout: 5000, + }) + expect(postSpy).toHaveBeenCalledTimes(2) + }) +}) diff --git a/apps/studio/lib/profile.tsx b/apps/studio/lib/profile.tsx index b4f14943b4b..9097ddbcd61 100644 --- a/apps/studio/lib/profile.tsx +++ b/apps/studio/lib/profile.tsx @@ -1,4 +1,5 @@ import * as Sentry from '@sentry/nextjs' +import { useQueryClient } from '@tanstack/react-query' import { useIsLoggedIn, useUser } from 'common' import { useRouter } from 'next/router' import { createContext, PropsWithChildren, useContext, useEffect, useMemo } from 'react' @@ -6,7 +7,10 @@ import { toast } from 'sonner' import { useSignOut } from './auth' import { getGitHubProfileImgUrl } from './github' +import { organizationKeys } from '@/data/organizations/keys' +import { permissionKeys } from '@/data/permissions/keys' import { usePermissionsQuery } from '@/data/permissions/permissions-query' +import { profileKeys } from '@/data/profile/keys' import { useProfileCreateMutation } from '@/data/profile/profile-create-mutation' import { useProfileIdentitiesQuery } from '@/data/profile/profile-identities-query' import { useProfileQuery } from '@/data/profile/profile-query' @@ -32,6 +36,7 @@ export const ProfileContext = createContext({ export const ProfileProvider = ({ children }: PropsWithChildren<{}>) => { const user = useUser() + const queryClient = useQueryClient() const isLoggedIn = useIsLoggedIn() const router = useRouter() const signOut = useSignOut() @@ -53,14 +58,13 @@ export const ProfileProvider = ({ children }: PropsWithChildren<{}>) => { }, onError: (error) => { if (error.code === 409) { - // [Joshen] There's currently an assumption that createProfile is getting triggered - // multiple times unnecessarily, although the tracing the code i can't see why this might - // be happening unless GET profile is somehow returning `User's profile not found` incorrectly - // Adding a Sentry capture + toast in hopes to catch this while developing on local / staging + // The profile already exists (e.g. GET hit a lagging replica, or it was created + // elsewhere), so the earlier "not found" is stale. Refetch rather than leaving the + // user stuck on the cached error. Sentry capture is kept to monitor how often this occurs. Sentry.captureMessage('Profile already exists: ' + error.message) - if (process.env.NEXT_PUBLIC_ENVIRONMENT !== 'prod') { - toast.error('[DEV] createProfile called despite profile already exists: ' + error.message) - } + queryClient.invalidateQueries({ queryKey: profileKeys.profile() }) + queryClient.invalidateQueries({ queryKey: organizationKeys.list() }) + queryClient.invalidateQueries({ queryKey: permissionKeys.list() }) } else { Sentry.captureMessage('Failed to create users profile: ' + error.message) toast.error('Failed to create your profile. Please refresh to try again.') @@ -79,21 +83,21 @@ export const ProfileProvider = ({ children }: PropsWithChildren<{}>) => { enabled: isLoggedIn, }) + const isProfileNotFound = isError && error?.message === "User's profile not found" useEffect(() => { - if (!isError) return - // if the user does not yet exist, create a profile for them - if (error?.message === "User's profile not found") { - createProfile() - } + if (!isProfileNotFound || !user?.id) return + createProfile() + }, [isProfileNotFound, user?.id, createProfile]) - // [Alaister] If the user has a bad auth token, auth-js won't know about it - // and will think the user is authenticated. Since fetching the profile happens - // on every page load, we can check for a 401 here and sign the user out if - // they have a bad token. - if (error?.code === 401) { - signOut().then(() => router.push('/sign-in')) - } - }, [error, signOut, router, createProfile, isError]) + // [Alaister] If the user has a bad auth token, auth-js won't know about it + // and will think the user is authenticated. Since fetching the profile happens + // on every page load, we can check for a 401 here and sign the user out if + // they have a bad token. + const isUnauthorized = isError && error?.code === 401 + useEffect(() => { + if (!isUnauthorized) return + signOut().then(() => router.push('/sign-in')) + }, [isUnauthorized, signOut, router]) const { isInitialLoading: isLoadingPermissions } = usePermissionsQuery({ enabled: isLoggedIn })