Skip to content

temp fix to race condition #70

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jamesholcombe merged 2 commits into from
Aug 7, 2025
Merged

temp fix to race condition #70

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eb77d99
temp fix to race condition
jamesholcombe Aug 7, 2025
5a90251
push user simplification
jamesholcombe Aug 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions packages/app/src/db/crud/user.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,14 @@ export async function readUserById(id: string): Promise<User | undefined> {
export async function createUser(userInsert: UserInsert): Promise<User> {
// this is done in transaction to avoid race condition when creating user, for conflicts on authId
return await db.transaction(async (trx) => {
// First try to find the user by authId
// Also check by email to handle edge cases
Copy link

Copilot AI Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment 'Also check by email to handle edge cases' is vague. It should explain what specific edge cases this addresses and why checking both email and authId is necessary.

Suggested change
// Also check by email to handle edge cases
// Check for an existing user with both the same email and authId.
// This handles edge cases where:
// - A user may have registered with the same email address using different authentication providers (resulting in different authIds).
// - There may be attempts to create duplicate accounts with the same email but different authIds, or vice versa.
// By checking both fields, we ensure that we do not create duplicate user records for the same logical user across different auth providers.

Copilot uses AI. Check for mistakes.
const [existingUser] = await trx
.select()
.from(user)
.where(
and(
eq(user.authId, userInsert.authId),
eq(user.email, userInsert.email),
eq(user.authId, userInsert.authId),
),
)
.execute();
Expand All @@ -59,13 +59,13 @@ export async function createUser(userInsert: UserInsert): Promise<User> {
const [insertedUser] = await trx
.insert(user)
.values(userInsert)
.returning()
.onConflictDoUpdate({
target: [user.email],
target: [user.authId],
set: {
authId: userInsert.authId,
email: userInsert.email,
Copy link

Copilot AI Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the conflict target from email to authId could have unintended consequences. If the same email is used with different authIds, this will now allow duplicate emails instead of updating the existing user's authId. This seems inconsistent with the original logic.

Copilot uses AI. Check for mistakes.
},
})
.returning()
.execute();

return insertedUser;
Copy link

Copilot AI Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of onConflictDoUpdate creates a potential race condition. Between checking for existing users and inserting a new one, another concurrent transaction could create the same user, causing the insert to fail with a constraint violation.

Copilot uses AI. Check for mistakes.
Expand Down
53 changes: 44 additions & 9 deletions packages/app/src/lib/auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,15 +77,50 @@ export const getServerUser = async (): Promise<User> => {
throw new Error(" New user has no email address");
}

const createdUser = await createUser({
email,
authId,
firstName: clerkUser.firstName || "",
lastName: clerkUser.lastName || "",
picture: clerkUser.imageUrl,
privacyPolicyAcceptedAt: new Date(),
termsOfUseAcceptedAt: new Date(),
});
// Retry mechanism for race conditions when creating new users
let createdUser: User | undefined;
let retryCount = 0;
const maxRetries = 3;

while (retryCount < maxRetries) {
try {
createdUser = await createUser({
email,
authId,
firstName: clerkUser.firstName || "",
lastName: clerkUser.lastName || "",
picture: clerkUser.imageUrl,
privacyPolicyAcceptedAt: new Date(),
termsOfUseAcceptedAt: new Date(),
});
break; // Success, exit the retry loop
} catch (error: any) {
Copy link

Copilot AI Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using 'any' type for error handling defeats type safety. Consider using 'unknown' or a more specific error type to maintain type safety while handling errors.

Suggested change
} catch (error: any) {
} catch (error: unknown) {

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using 'any' type for error handling defeats TypeScript's type safety. Consider using 'unknown' or a more specific error type to maintain type safety.

Suggested change
} catch (error: any) {
} catch (error: unknown) {

Copilot uses AI. Check for mistakes.
retryCount++;

// If it's a duplicate key error and we haven't exceeded max retries, try again
if (retryCount < maxRetries) {
Copy link

Copilot AI Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The retry logic doesn't differentiate between error types. The comment mentions 'duplicate key error' but the code retries for any error. This could lead to unnecessary retries for non-recoverable errors like network issues or permission errors.

Suggested change
if (retryCount < maxRetries) {
// Only retry on duplicate key error
const isDuplicateKeyError =
// PostgreSQL unique violation
(error && error.code === '23505') ||
// MongoDB duplicate key
(error && error.code === 11000) ||
// MySQL duplicate entry
(error && error.errno === 1062) ||
// Fallback: error message contains 'duplicate' or 'unique'
(typeof error?.message === 'string' &&
/duplicate|unique/i.test(error.message));
retryCount++;
// If it's a duplicate key error and we haven't exceeded max retries, try again
if (isDuplicateKeyError && retryCount < maxRetries) {

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The retry logic doesn't check for specific error types. This could cause unnecessary retries for errors that aren't related to race conditions (e.g., network errors, validation errors). Consider checking for specific database constraint violation errors before retrying.

Copilot uses AI. Check for mistakes.
// Wait a bit before retrying (exponential backoff)
await new Promise((resolve) =>
setTimeout(resolve, Math.pow(2, retryCount) * 100),
);

// Check if user was created by another request in the meantime
const existingUser = await readUserByAuthId(authId);
if (existingUser) {
createdUser = existingUser;
break;
}
continue;
}

// If it's not a duplicate key error or we've exceeded retries, rethrow
throw error;
}
}

if (!createdUser) {
throw new Error("Failed to create user after multiple attempts");
}

// Identify newly created user in PostHog
await identifyUserServer(createdUser, {
Expand Down