🚪 fix: Let Concurrent First OpenID Logins Continue as the New User - #16778
Conversation
codegraph-librechat
Bot
added
🗺️ Backend Infra
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: add4cc20e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| user = await createOpenIDUser({ | ||
| lookup, | ||
| create: () => createUser(newUser, balanceConfig, true, true), | ||
| }); |
There was a problem hiding this comment.
When concurrent callbacks for the same subject carry different profile claims—for example, after the IdP email or display name changes—the losing callback receives the winner here but remains inside the if (!user) branch, so it skips the existing-user normalization at lines 698–711. The subsequent updateUser therefore persists and returns the winner's stale email, username, name, idOnTheSource, and verification state instead of applying the current callback's claims; route recovered users through the same normalization used for an initially found account.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 22c4ad6. The claims-to-account mapping now lives in applyOpenIDProfile (packages/api/src/auth/openid.ts). The strategy applies it to an account found by the first lookup, and createOpenIDUser applies it to the account it recovers after a duplicate key, so the losing callback's email, verification state, username, name and idOnTheSource are what updateUser persists. createOpenIDUser also builds the inserted user from the same profile, so the create path and the refresh path read one mapping.
Tests: openid.spec.ts › "refreshes the recovered user with the losing request's claims" (real MongoDB: the winner is stored with one email and name, the loser's callback carries another, and the returned user has the loser's values). openidStrategy.spec.js › "writes this callback's claims over the ones the other request stored" (updateUser receives the current token's email, emailVerified, username and name). With recovery returning the raw recovered user instead, both tests fail.
The recovered account keeps the config the winner was admitted under (base config, which every new user gets per resolveAppConfigForUser) rather than re-resolving tenant overrides; both requests carried the same identity and passed the same base checks.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22c4ad6280
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function isDuplicateKeyError(error: unknown): boolean { | ||
| return typeof error === 'object' && error !== null && 'code' in error && error.code === 11000; |
There was a problem hiding this comment.
The new API-layer helper identifies MongoDB conflicts by inspecting the storage-specific 11000 error code, although its create dependency otherwise presents a generic callback contract. Any repository wrapper or alternate storage implementation that normalizes uniqueness errors will silently bypass recovery and reintroduce failed concurrent first logins. Classify the insert conflict in packages/data-schemas and expose a typed, storage-neutral result or error to this service.
AGENTS.md reference: AGENTS.md:L73-L75
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e3f3e71. The conflict is now classified in packages/data-schemas: createUserIfAbsent returns { ok: true, value } or { ok: false, error: { code: 'user_exists' } } (CreateUserIfAbsentResult), and only the insert that creates the user credits the start balance (creditStartBalance, shared with createUser). createOpenIDUser takes a create that returns that result and no longer inspects storage errors. The OpenID strategy creates through createUserIfAbsent. A conflict the repeated lookup cannot account for now throws a plain error rather than rethrowing the database error.
Tests: user.methods.spec.ts › createUserIfAbsent (real MongoDB): one of two concurrent inserts reports user_exists and the balance is credited once; an account holding the same OpenID identity under another email reports user_exists; a validation failure still throws. With the classification removed, the two user_exists tests fail. The createOpenIDUser (real MongoDB) and strategy race tests now run on the typed result.
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a62f59a092
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| create: (user: CreateUserRequest) => Promise<CreateUserIfAbsentResult>; | ||
| lookup: OpenIDUserLookup; | ||
| profile: OpenIDProfile; | ||
| }): Promise<IUser> { |
There was a problem hiding this comment.
Change this newly exported service signature to use a plain user DTO: IUser extends Mongoose Document, and both CreateUserIfAbsentResult and the return type expose it through @librechat/api. This widens the existing storage leak and forces any alternate repository implementation or TypeScript caller to emulate Mongoose document members even though the function only needs plain account fields.
AGENTS.md reference: AGENTS.md:L83-L88
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 18f5c30. UserRecord (packages/data-schemas/src/types/user.ts) is the stored user as plain data, the user's own fields plus _id, without Document members. It follows the same plain/Document split as IChatProject/IChatProjectDocument. CreateUserIfAbsentResult returns it (user.toObject() as UserRecord), createOpenIDUser returns Promise<UserRecord>, and applyOpenIDProfile is bounded by UserRecord rather than IUser. IUser itself and the existing contracts that already return it (findOpenIDUser, findUser) are unchanged, per AGENTS.md's "stop widening it rather than rewriting what exists". npx tsc --noEmit is clean for the touched files in packages/api and packages/data-schemas.
|
|
||
| await Balance.findOneAndUpdate( | ||
| { _id: userId }, | ||
| { ...update, $setOnInsert: { user: userId, tokenCredits: balanceConfig.startBalance } }, |
There was a problem hiding this comment.
When concurrent first-login callbacks run on separate workers and the insert winner pauses before this upsert, the recovered callback can complete authentication and let /openid/callback's setBalanceConfig create the balance first. That middleware resolves configuration from the post-role-sync user, whereas this path received the base configuration used to admit a new user; because tokenCredits is now insert-only, role-specific configurations with different start balances make the initial credit depend on timing instead of retaining the creator's configured value as before. Make the creation configuration authoritative or reconcile the competing initialization paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 18f5c30. creditStartBalance now writes tokenCredits with $set (refill settings with it) and only user with $setOnInsert. When login balance sync initializes the record first from the role-resolved config, the creating request's configured start balance overwrites it, which is the same outcome as the no-race path, where sync finds the record already credited and leaves it alone. Because it is a $set rather than an $inc, it is never added on top.
Test: user.methods.spec.ts › "keeps the creator's start balance over one login balance sync initialized first" (real MongoDB: a balance record with 300 exists for the new user's id; createUserIfAbsent with startBalance: 500 leaves exactly one record with 500). With tokenCredits back on $setOnInsert, the test fails (300). The concurrent-insert test still credits once.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18f5c30e76
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| create: (user: CreateUserRequest) => Promise<CreateUserIfAbsentResult>; | ||
| lookup: OpenIDUserLookup; | ||
| profile: OpenIDProfile; | ||
| }): Promise<UserRecord> { |
There was a problem hiding this comment.
The follow-up still leaves this newly exported packages/api contract coupled to Mongoose: CreateUserRequest extends Partial<IUser> (and therefore Document), while the returned UserRecord hard-codes mongoose.Types.ObjectId. Consequently, an alternate repository implementation or consumer of @librechat/api must retain Mongoose types for both sides of this callback; define a plain request/response DTO with a storage-neutral identifier instead. The fresh evidence beyond the resolved comment is that the new aliases remove document methods from the result but still retain Types.ObjectId and reuse the document-derived request type.
AGENTS.md reference: AGENTS.md:L83-L87
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0e86a25. UserRecord now types its id as _id: { toString(): string } (the user's own fields plus an id any storage can satisfy), not Types.ObjectId. NewUserData (Partial of those plain fields plus email) replaces the Document-derived CreateUserRequest on both sides of the callback: createUserIfAbsent(data: NewUserData, ...) and createOpenIDUser({ create: (user: NewUserData) => ... }). Neither contract now references Document or Types.ObjectId; tsc --noEmit is clean for the touched files in both workspaces.
| await Balance.findOneAndUpdate( | ||
| { _id: userId }, | ||
| { $set: set, $setOnInsert: { user: userId } }, | ||
| { upsert: true, new: true }, |
There was a problem hiding this comment.
When the winning first-login request pauses after User.create, the recovered callback can finish authentication, let setBalanceConfig initialize the balance, and return a session that begins spending credits before the winner resumes. This unconditional $set then restores startBalance and erases that intervening debit, granting the spent credits again. This is a fresh consequence of the $set fix discussed in the existing balance thread; use an initialization compare-and-set/marker or otherwise complete balance initialization before the account becomes usable so only competing initialization is replaced, not later account activity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0e86a25 by removing the window rather than arbitrating it. createUserIfAbsent validates the new user, initializes its start balance under the user's (freshly generated) id, and only then inserts the user. The account is never visible without its balance, so login balance sync for a recovered login always finds the record and never initializes it, and no later activity can be overwritten. creditStartBalance is insert-only again ($setOnInsert) and never replaces an existing balance. An insert that fails (user_exists or any other error) removes the balance it initialized for its never-used id; a validation failure writes nothing.
Tests (user.methods.spec.ts, real MongoDB): the balance write is invoked before the user save; after a lost race only the winner's balance remains; concurrent inserts leave one balance of the start amount; a validation failure leaves no balance. Moving the credit back after the insert fails the ordering test; removing the cleanup fails the race and orphan tests.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e86a25be4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch (error) { | ||
| await discardStartBalance(user._id, balanceConfig); |
There was a problem hiding this comment.
When user.save() commits the insert but rejects because the connection drops before the acknowledgement reaches the caller, this catch treats the rejection as proof that no user exists and deletes that real user's balance. A concurrent callback may already have found the committed account and started using its credits, so this cleanup can erase the starting balance or intervening debits; restrict deletion to a confirmed failed insert, or verify the generated user ID before removing its balance.
AGENTS.md reference: AGENTS.md:L44-L47
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cf924bf. createUserIfAbsent now removes the balance it initialized only when a unique index rejected the insert (code === 11000), which proves no user was written. Any other save failure rethrows and keeps the balance, since an unacknowledged insert may still have committed; at worst that leaves an unused balance under an id that never became a user.
Test (user.methods.spec.ts): a save that rejects with a non-duplicate error leaves the start balance in place. With cleanup on every failure, the test fails.
| if (created.ok) return created.value; | ||
|
|
||
| const strategyName = lookup.strategyName ?? 'openid'; | ||
| const resolution = await findOpenIDUser(lookup); |
There was a problem hiding this comment.
During a rolling deployment, an old worker can insert the user and pause before its previous $inc-based balance write; a new worker then loses the user insert, recovers the now-visible account here, and lets setBalanceConfig create its starting balance before the old worker resumes and increments that balance again. This grants the account the start balance twice, or adds the full start balance after intervening spending. Fresh evidence beyond the resolved same-version balance thread is that the pre-insert initialization only protects users created by new workers, while old workers still publish the user before crediting it; stage a backward-compatible initialization change or prevent recovery from exposing an old-created account until its balance is initialized.
AGENTS.md reference: AGENTS.md:L44-L47
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cf924bf, by one invariant rather than another ordering tweak: a recovered login continues only once the account's creator has finished provisioning it. With a start balance configured (enabled and startBalance, the condition data-schemas credits under), createOpenIDUser requires the recovered account's balance to exist (findBalanceByUser, passed in by the strategy). If it doesn't, the login fails with AUTH_FAILED, as it did before recovery existed. Creators on this release always pass immediately, because createUserIfAbsent writes the balance before the user. A previous-release creator still between its insert and its $inc makes the recovered login fail instead of letting balance sync initialize the record, so the old $inc lands on an absent record and credits once.
Tests: openid.spec.ts (real MongoDB): an account inserted without a balance, as an old-release winner would leave it mid-flight, fails the recovered login with AUTH_FAILED and leaves no balance behind; with startBalance: 0 the same account recovers. openidStrategy.spec.js: with a start balance configured and no balance found, the strategy checks the winner's id and fails with AUTH_FAILED without updating the user. With the balance check removed, the openid.spec.ts test fails.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf924bfb4c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const user = new User(userData); | ||
| await user.validate(); | ||
| await creditStartBalance(user._id, balanceConfig); |
There was a problem hiding this comment.
If creditStartBalance commits its upsert but rejects because the database connection drops before acknowledgment, this await exits before user.save(), leaving a funded balance whose generated user ID will never exist. Every retry generates another ID, so these orphan balances are neither reused nor removed and can accumulate during transient database failures; reconcile or clean up the provisional balance when this step fails.
AGENTS.md reference: AGENTS.md:L44-L46
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5e7786a. A balance write that rejects now removes the provisional balance, best effort, before rethrowing. The user cannot exist yet, because its insert has not run, so deleting the balance under that never-used id is safe. If the cleanup itself fails (the connection is still down), it logs and the original error propagates.
Test (user.methods.spec.ts): the balance write commits and then rejects; createUserIfAbsent rethrows and leaves no balance and no user. Without the cleanup, one balance remains and the test fails.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e7786a37b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| export type OpenIDUserLookup = { | ||
| openidId: string; | ||
| findUser: UserMethods['findUser']; |
There was a problem hiding this comment.
Replace UserMethods['findUser'] in this newly exported API type with a plain lookup contract. That method's signature accepts Mongoose FilterQuery<IUser> and returns the Document-derived IUser, so every consumer of OpenIDUserLookup must implement a storage-specific Mongoose contract despite this service needing only ordinary identity fields and a plain user record. This widens the database abstraction leak the repository explicitly requires new packages/api signatures to avoid.
AGENTS.md reference: AGENTS.md:L83-L87
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e7054e5 by not exporting a new type. OpenIDUserLookup is gone, and findOpenIDUser keeps the inline parameter type it had before this PR (findUser: UserMethods['findUser'], unchanged from dev). createOpenIDUser takes Parameters<typeof findOpenIDUser>[0], so it passes through exactly the contract that already exists rather than widening it.
Replacing that contract with a plain equality lookup is not a drop-in change here: findOpenIDUser builds Mongo operator conditions ({ openidIssuer: { $exists: false } } and the other legacy-issuer filters in getIssuerBoundConditions) that a field-equality lookup cannot express. Moving that query behind a data-schemas method would rewrite the existing lookup, and AGENTS.md asks to stop widening that leak rather than rewrite what exists, which is out of scope for this bug fix. The new contracts this PR adds (createUserIfAbsent, UserRecord, NewUserData) are storage-neutral.
| user = await createOpenIDUser({ | ||
| lookup, | ||
| profile, | ||
| balanceConfig: getBalanceConfig(appConfig), | ||
| createUserIfAbsent, |
There was a problem hiding this comment.
When the initial lookup misses but a tenant-scoped OpenID account is provisioned by another flow before this insert, appConfig was fixed to the base config and its allowedDomains check already ran before the recovered user was known. If role sync leaves the recovered account's role unchanged, the later post-sync check is skipped, so this callback can authenticate an email that an ordinary login to the same account would reject under its tenant/role configuration; resolve and enforce the recovered user's config before continuing.
AGENTS.md reference: AGENTS.md:L186-L189
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e7054e5. A recovered account is now admitted exactly as if the first lookup had found it. If it has a tenantId, createOpenIDUser resolves its config through resolveAppConfigForUser (the same call the strategy makes for a found tenant user) and enforces that config's allowedDomains, throwing 'Email domain not allowed', which createOpenIDCallback already maps to done(null, false, { message }). It returns { user, appConfig }, and the strategy continues under the recovered account's config for the rest of the login (role sync, avatar strategy).
Tests (openid.spec.ts, real MongoDB inside a tenant context, so the insert collides on the tenant-scoped indexes): a tenant account whose config allows only other.example fails the recovered login with 'Email domain not allowed', after resolving the config with that tenantId; a tenant config that admits the email continues, and the returned appConfig is the tenant's. With recovery keeping the base config, both tests fail.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7054e5fa9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| const userId = resolution.user._id.toString(); | ||
| if (hasStartBalance(balanceConfig) && !(await findBalanceByUser(userId))) { |
There was a problem hiding this comment.
When tenant provisioning races this callback, balanceConfig still comes from the pre-recovery base config, even though the winner discovered below may have a different tenant/role balance policy. If the base enables a start balance but the tenant disables it, the recovered login is incorrectly rejected for lacking a balance; conversely, if only the tenant enables it, this skips the mixed-version barrier and lets downstream balance initialization race an older creator's credit write. Resolve accountConfig first and test the balance configuration that actually applies to the recovered account.
AGENTS.md reference: AGENTS.md:L44-L47
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 144acc7. createOpenIDUser now resolves the recovered account's config first (tenant config via resolveAppConfigForUser for a tenant account), enforces its email-domain policy, and only then applies the provisioning barrier, using the start balance that config sets: the config login balance sync initializes from. It takes getBalanceConfig as a dependency: the strategy passes the @librechat/api export, and importing it from app/config would close a cycle through utils/identity.
Tests (openid.spec.ts, real MongoDB in a tenant context): with a base start balance of 500 and a tenant config that sets none, a recovered tenant account without a balance continues; with no base start balance and a tenant start balance of 500, the same account fails with AUTH_FAILED. Checking the base config's policy instead fails both tests.
There was a problem hiding this comment.
Correction to my earlier reply: the change in 144acc7 was wrong, and 336434f reverts it. The barrier guards against a previous-release creator's $inc landing on a balance that login balance sync already initialized. That creator credits the base config's start balance only, so:
- "tenant enables, base does not": there is no old
$incto race, because tenant balances are initialized insert-only by balance sync. - "base enables, tenant disables": a creator on this release writes the base start balance before the user exists, so the recovered login finds it.
Keying the barrier on the tenant config instead failed every concurrent first login in tenants that set a start balance the base config does not (the next review round's finding). The barrier now reads the config new users are created under; see the reply on that thread for the tests.
|
@codex review |
Two requests for the same not-yet-created OpenID identity both missed
findOpenIDUser and both called createUser; the unique email and OpenID
indexes rejected the second insert, and that login failed with E11000.
createUserIfAbsent (data-schemas) inserts a first-login user and reports
a lost race as { ok: false, error: { code: 'user_exists' } }. It
initializes the start balance under the new user's id before inserting
the user, so the account is never visible without it, and removes that
balance when the write fails or a unique index rejects the insert.
createUser shares the insert-only credit (creditStartBalance).
createOpenIDUser (packages/api) creates through it and, on user_exists,
repeats findOpenIDUser with its provider and issuer checks and continues
as the account that won, admitted and refreshed exactly as if the first
lookup had found it: the account's own config (tenant config for a
tenant account) and its email-domain policy apply, and applyOpenIDProfile
writes this callback's claims. It continues only once the winner
finished provisioning the account: when that config sets a start
balance, the balance must exist, which a previous-release creator
credits only after its insert. UserRecord and NewUserData are the plain,
storage-neutral contracts.
Related to #16766
144acc7 to
cc5ff47
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc5ff47aa5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| idOnTheSource: profile.idOnTheSource, | ||
| openidIssuer: profile.openidIssuer, | ||
| }, | ||
| getBalanceConfig(appConfig) ?? undefined, |
There was a problem hiding this comment.
When concurrent first logins create a tenant-scoped account and the base config has no start balance while the tenant config does, this call provisions the winner using the base appConfig, so the user becomes visible without a balance. The losing request then resolves the tenant config and fails the new barrier at line 418 before the winner reaches setBalanceConfig; thus the one-off first-login failure remains timing-dependent for this supported configuration. Resolve the new account's tenant configuration before provisioning, or otherwise let the barrier distinguish a same-version creator whose tenant balance initialization is still pending.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 336434f, by reverting the barrier to the config new users are created under rather than adding a tenant-aware creation path. The barrier exists for one hazard: a previous-release creator adding the start balance with $inc after login balance sync initialized the record. Previous-release creators only credit the base config's start balance (getBalanceConfig(baseConfig) for every new user), so the barrier now applies only when appConfig, the config the account is created under, sets a start balance. A start balance that only the tenant config sets is initialized by login balance sync with an insert-only write (upsertBalanceFields with insertOnly), which nothing adds on top of, so it no longer holds the recovered login back.
Tests (openid.spec.ts, real MongoDB in a tenant context): no base start balance and a tenant start balance of 500, with the recovered account lacking a balance, continues (your scenario). A base start balance of 500 and a tenant config without one, with no balance, fails with AUTH_FAILED. Keying the barrier on the tenant config again fails both tests.
The provisioning barrier read the recovered account's tenant config. With a tenant start balance the base config does not set, every concurrent first login in that tenant failed again, because the winner creates the user under the base config and login balance sync only initializes the tenant balance afterwards. The barrier guards against a previous-release creator's $inc landing on a balance login balance sync already initialized, and that creator only credits the base config's start balance. createOpenIDUser now holds the recovered login only when the config new users are created under sets a start balance; a tenant-only start balance is initialized insert-only by login balance sync and cannot be added twice.
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Pull Request
Summary
When two requests complete an OpenID login for the same user who has no LibreChat account yet, both run
findOpenIDUser, both miss, and both callcreateUser. The uniqueemail_1_tenantId_1index (or theopenidId/openidIssuer/tenantIdindex) rejects the second insert, so that login fails withE11000 duplicate key error. The account itself is created correctly and the next attempt succeeds, so the user sees a one-off failed first login. Two browser tabs finishing the callback together trigger it, and so does any deployment that authenticates parallel requests throughprocessOpenIDAuth, where the SPA's first parallel requests hit it every time.processOpenIDAuthnow creates first-login users throughcreateOpenIDUserinpackages/api, which inserts through a new data-schemas method,createUserIfAbsent. That method reports a lost insert race as a typeduser_existsresult instead of a database error. Onuser_exists, the request repeats the same lookup and continues as the user the other request created, admitted and refreshed exactly as if its first lookup had found that account: a tenant account resolves its tenant config and that config's email-domain policy applies, and the account carries this callback's claims (email, verification state, username, name). That lookup applies the usual provider,suband issuer checks. If it rejects the match (for example, a different provider registered the email in the meantime), the login fails with the normalAUTH_FAILEDresponse instead of a raw database error. If the lookup finds nothing that explains the conflict, the login fails with an error, as an unexpected duplicate did before. The start balance is still credited once, with the creating request's value:createUserIfAbsentinitializes it under the new user's id before inserting the user, so the account is never visible without it, the recovered request's login balance sync always finds it, and an insert a unique index rejects removes the balance it initialized. A recovered login continues only once the account's creator finished provisioning it: when the config new users are created under sets a start balance, that balance must exist, otherwise that login fails as it did before this change (only possible while a creator on the previous release sits between its insert and its credit during a rolling deploy).Related to #16766
How it works
Storage knowledge stays in
packages/data-schemas:createUserIfAbsentreturns{ ok: true, value } | { ok: false, error: { code: 'user_exists' } }, andcreateUserandcreateUserIfAbsentshare the start-balance credit throughcreditStartBalance. The new contracts are storage-neutral: they takeNewUserDataand returnUserRecord(the user's own fields plus_id: { toString(): string }), not theIUserdocument type or theDocument-derivedCreateUserRequest. The strategy passes the samelookupobject to both lookups, so the retry cannot drift from the first one. The claims-to-account mapping moved intoapplyOpenIDProfile, which runs for both a found account and a recovered one:createOpenIDUserreturns{ user, appConfig }: the config this request resolved for a user it created, or the recovered account's own config (tenant config viaresolveAppConfigForUserfor a tenant account), so the rest of the login runs under the same config an ordinary login to that account would.findOpenIDUserkeeps its existing parameter contract;createOpenIDUsertakes the same shape.Type of change
Testing
The bug reproduces against a real MongoDB with the real
createUserand the real user indexes. TwofindOpenIDUsercalls for a new identity both returnnull, then two concurrentcreateUsercalls reject one insert withE11000 duplicate key error collection: test.users index: email_1_tenantId_1, which is the error in the issue.Tested environments/configuration:
UserandBalancemodels and indexes syncedAutomated tests:
packages/data-schemas/src/methods/user.methods.spec.ts, newcreateUserIfAbsentblock against a real MongoDB. One of two concurrent inserts reportsuser_exists, the collection holds one user, and the start balance is credited once. An account holding the same OpenID identity under another email reportsuser_exists. The balance write happens before the user save; after a lost race only the winner's balance remains; a non-duplicate save failure keeps the balance (the insert may have committed); a balance write that commits but reports a failure leaves no balance and no user; a validation failure writes no balance. A validation failure still throws.packages/api/src/auth/openid.spec.ts, newcreateOpenIDUserblock against a real MongoDB. Concurrent first logins resolve to one user, and the collection holds exactly one document. When the losing request's claims differ, the recovered user carries the loser's email, name and verification state. WithstartBalance: 500, concurrent first logins leave one balance of 500. If a local-provider user takes the email between lookup and insert, the login is rejected withAUTH_FAILED. An account found without its start balance (as a previous-release winner leaves it mid-provisioning) fails the recovered login withAUTH_FAILED, and recovers when no start balance is configured. Inside a tenant context, a recovered tenant account whose config disallows the email fails withEmail domain not allowed, and one whose config admits it continues under the tenant's config; the balance barrier follows the start balance new users are created with, not the tenant's (only the tenant sets one: continues, since login balance sync initializes it insert-only; the base sets one and it is missing: fails). A conflict the lookup cannot explain throws. A validation failure throws without a second lookup.api/strategies/openidStrategy.spec.js, newconcurrent first loginblock. The losing request logs in as the winner's_idandupdateUsertargets that user. When the winner stored older claims,updateUserreceives the current token's email,emailVerified, username and name. A concurrent other-provider account, or a recovered account whose start balance does not exist yet, maps todone(null, false, { message: AUTH_FAILED }).openid.spec.tsand the strategy race tests fail. With recovery returning the raw recovered user, both claim-refresh tests fail. WithcreateUserIfAbsentrethrowing the duplicate instead of reportinguser_exists, bothuser_existstests fail. With the credit moved back after the insert, the ordering test fails; without the cleanup, the race and orphan-balance tests fail; with cleanup on every save failure, the non-duplicate failure test fails; without cleanup on a failed balance write, that test fails; without the provisioning check, the missing-balance recovery test fails; with recovery keeping the base config, both tenant-policy tests fail.packages/apispecs that import the user methods, includingopenid.spec.ts(2343 passed, 4 skipped);openidStrategy.spec.js,openIdJwtStrategy.spec.js,AuthController.spec.js,OpenIDSessionRefresh.spec.js,OboTokenService.spec.js,GraphApiService.spec.js(437 passed).Screenshots / recordings
No user-facing change.
Risk / compatibility
Low. Recovery only runs when the first-login insert reports
user_exists, so the normal path still does one lookup and one insert.createUserkeeps its signature and order; its balance credit moved into a shared helper and is now insert-only, which only differs from the old$incwhen a balance record for the brand-new user id already exists (the old code added to it). No schema, config or wire changes.Mixed versions: during a rolling deploy, a previous-release worker still inserts the user before crediting it. A recovered login on this release that finds such an account before its balance exists fails, which is the pre-change behavior, rather than letting login balance sync initialize the record ahead of the old
$inc.SAML (
samlStrategy.js), LDAP (ldapStrategy.js) and social logins (createSocialUserinapi/strategies/process.js) have the same lookup-then-insert shape and are not changed here; #16779, stacked on this branch, covers them.Checklist