fix(server,db): per-user pending oauth_account uniqueness + clean conflict errors#1121
Conversation
…y error The partial unique index `auth_profiles_pending_unique` rejects a second pending oauth_account profile per (org, connector, kind, provider). The UI's "create OAuth account" click omits `slug` and bypassed `handleCreateAuthProfile`'s existing slug-keyed idempotent reuse, so a repeat click leaked `duplicate key value violates unique constraint "auth_profiles_pending_unique"` into a toast. - `createAuthProfile` now catches 23505 on that index, looks up the colliding row, and throws a `PendingAuthConflictError` (extends `ToolUserError`, HTTP 409) carrying the existing row. - `handleCreateAuthProfile` does the same tuple lookup pre-INSERT when no slug was supplied, so the happy path silently reuses the pending row + issues a fresh connect token. The try/catch closes the SELECT/INSERT race window. - New integration test covers no-slug repeat (idempotent), slug-different collision (clean error, no PG text leaked), and direct collision (PendingAuthConflictError with the existing row attached). Does not redesign the index itself; including `created_by` (so two users in the same org can start parallel OAuth flows) is a follow-up.
📝 WalkthroughWalkthroughThis PR introduces per-user pending auth conflict handling for OAuth profile creation. A new database constraint, error model, and detection logic prevent duplicate in-flight OAuth flows for the same user while permitting multiple users to flow in parallel. Changes include database migration, PendingAuthConflictError class, utility lookup logic, tool integration for graceful reuse/error handling, API status code support, and integration tests. ChangesPer-user pending auth conflict handling
Sequence Diagram(s)sequenceDiagram
participant UI
participant manageAuthProfiles
participant findPendingAuthProfile
participant createAuthProfile
participant Database
UI->>manageAuthProfiles: create_auth_profile action
manageAuthProfiles->>findPendingAuthProfile: lookup existing pending for (org, connector, provider, user)
findPendingAuthProfile->>Database: query auth_profiles status=pending_auth
alt existing pending found
findPendingAuthProfile-->>manageAuthProfiles: return existing row
manageAuthProfiles-->>UI: reuse (idempotent)
else no existing pending
findPendingAuthProfile-->>manageAuthProfiles: null
manageAuthProfiles->>createAuthProfile: insert new pending profile
createAuthProfile->>Database: INSERT auth_profiles (normalized provider)
alt conflict detected
Database-->>createAuthProfile: unique constraint error
createAuthProfile->>findPendingAuthProfile: find conflicting row
findPendingAuthProfile-->>createAuthProfile: existing row
createAuthProfile-->>manageAuthProfiles: throw PendingAuthConflictError
manageAuthProfiles-->>UI: { error: "already pending authorization" }
else no conflict
Database-->>createAuthProfile: success
createAuthProfile-->>manageAuthProfiles: new row
manageAuthProfiles-->>UI: new profile
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The old `auth_profiles_pending_unique` was keyed on (org, connector_key, profile_kind, provider) with no caller dimension. For `oauth_account` — which is user-personal — that blocked two members in the same org from running parallel OAuth flows for the same connector: User B's INSERT collided with User A's in-flight pending row. Replace with `auth_profiles_pending_oauth_account_unique` keyed on (org, connector_key, provider, created_by) filtered to status='pending_auth' AND profile_kind='oauth_account'. Other kinds are written status='active' from creation in practice (oauth_app, env via upsertConnectorAuthProfiles; browser_session via worker-api), so the old per-kind bound was unused for them; createAuthProfile's typed 23505 catch stays as the safety net if any future caller pushes a pending row through. findPendingAuthProfile takes createdBy and filters by it for oauth_account. handleCreateAuthProfile passes ctx.userId. New integration test asserts two users in the same org can both reach pending_auth in parallel and that per-user dedup still reuses each user's own row.
|
bug_free 86, simplicity 88, slop 0, bugs 0, 0 blockers Typecheck, unit, and integration logs all passed. Reviewed migration/code paths and ran git diff --check. [env] Extra psql index inspection could not run because DATABASE_URL was not exported/in .env for this shell; no suite failures observed. Full verdict JSON{
"bug_free_confidence": 86,
"bugs": 0,
"slop": 0,
"simplicity": 88,
"blockers": [],
"change_type": "fix",
"behavior_change_risk": "medium",
"tests_adequate": true,
"suggested_fixes": [],
"notes": "Typecheck, unit, and integration logs all passed. Reviewed migration/code paths and ran git diff --check. [env] Extra psql index inspection could not run because DATABASE_URL was not exported/in .env for this shell; no suite failures observed.",
"categories": {
"src": 202,
"tests": 283,
"docs": 0,
"config": 0,
"deps": 0,
"migrations": 35,
"ci": 0,
"generated": 0
}
}Local review gate — branch protection can require the |
buremba
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/server/src/utils/auth-profiles.ts`:
- Around line 395-418: The catch block currently rethrows the raw Postgres error
when isUniqueViolation(..., 'auth_profiles_pending_oauth_account_unique') is
true but findPendingAuthProfile(...) returns null, leaking constraint details;
update the handler in the catch of the INSERT to, after calling
findPendingAuthProfile with params (organizationId, connectorKey, profileKind
'oauth_account', provider normalizedProvider, createdBy), throw a
PendingAuthConflictError(existing) if existing is found, otherwise throw a new,
generic conflict error (e.g., new Error('pending auth profile conflict')) or a
dedicated domain error without exposing DB constraint names so callers do not
receive raw PG details; keep the existing checks for params.profileKind,
params.connectorKey, and normalizedProvider and ensure all branches end with
either PendingAuthConflictError or the sanitized error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c8ebea8c-ec5d-448f-910b-50bb7ae9461c
📒 Files selected for processing (5)
db/migrations/20260528120000_auth_profiles_pending_per_user.sqlpackages/server/src/__tests__/integration/connectors/pending-auth-conflict.test.tspackages/server/src/rest-api.tspackages/server/src/tools/admin/manage_auth_profiles.tspackages/server/src/utils/auth-profiles.ts
| } catch (err) { | ||
| // Partial unique index `auth_profiles_pending_oauth_account_unique` | ||
| // enforces one pending oauth_account row per (org, connector_key, | ||
| // provider, created_by). Translate a raw 23505 into a structured error | ||
| // carrying the existing row so callers can recover (idempotent reuse) | ||
| // or surface a clean message instead of leaking the constraint name. | ||
| if (isUniqueViolation(err, 'auth_profiles_pending_oauth_account_unique')) { | ||
| if ( | ||
| params.profileKind === 'oauth_account' && | ||
| params.connectorKey !== null && | ||
| normalizedProvider !== null | ||
| ) { | ||
| const existing = await findPendingAuthProfile({ | ||
| organizationId: params.organizationId, | ||
| connectorKey: params.connectorKey, | ||
| profileKind: 'oauth_account', | ||
| provider: normalizedProvider, | ||
| createdBy: params.createdBy ?? null, | ||
| }); | ||
| if (existing) throw new PendingAuthConflictError(existing); | ||
| } | ||
| } | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
Edge case: constraint violation with no existing row still leaks raw PG error.
If isUniqueViolation matches but findPendingAuthProfile returns null (the colliding row was deleted between the INSERT failure and the SELECT), the original err is re-thrown at line 417, leaking the constraint name. Wrap with a generic message when existing is null.
Proposed fix
if (isUniqueViolation(err, 'auth_profiles_pending_oauth_account_unique')) {
if (
params.profileKind === 'oauth_account' &&
params.connectorKey !== null &&
normalizedProvider !== null
) {
const existing = await findPendingAuthProfile({
organizationId: params.organizationId,
connectorKey: params.connectorKey,
profileKind: 'oauth_account',
provider: normalizedProvider,
createdBy: params.createdBy ?? null,
});
- if (existing) throw new PendingAuthConflictError(existing);
+ if (existing) {
+ throw new PendingAuthConflictError(existing);
+ }
+ // Row vanished between constraint-check and lookup; surface a generic
+ // conflict so we never leak the raw constraint name to the caller.
+ throw new ToolUserError(
+ 'A pending auth profile for this connector already exists. Please retry.',
+ 409
+ );
}
}
throw err;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/server/src/utils/auth-profiles.ts` around lines 395 - 418, The catch
block currently rethrows the raw Postgres error when isUniqueViolation(...,
'auth_profiles_pending_oauth_account_unique') is true but
findPendingAuthProfile(...) returns null, leaking constraint details; update the
handler in the catch of the INSERT to, after calling findPendingAuthProfile with
params (organizationId, connectorKey, profileKind 'oauth_account', provider
normalizedProvider, createdBy), throw a PendingAuthConflictError(existing) if
existing is found, otherwise throw a new, generic conflict error (e.g., new
Error('pending auth profile conflict')) or a dedicated domain error without
exposing DB constraint names so callers do not receive raw PG details; keep the
existing checks for params.profileKind, params.connectorKey, and
normalizedProvider and ensure all branches end with either
PendingAuthConflictError or the sanitized error.
Live e2e — doneBooted Migration verified liveOld 🔴 RED — pre-fix behavior (raw INSERT bypassing the catch)INSERT INTO auth_profiles (org, slug, connector_key, profile_kind, status, provider, created_by, ...)
VALUES ('org_e2e_test', 'github-account-different', 'github', 'oauth_account', 'pending_auth', 'github', 'user_e2e_a', ...);
ERROR: duplicate key value violates unique constraint "auth_profiles_pending_oauth_account_unique"
DETAIL: Key (organization_id, connector_key, provider, created_by)=(org_e2e_test, github, github, user_e2e_a) already exists.This is exactly the shape of message that leaked into the user's toast on prod. 🟢 GREEN case 1 — no-slug repeat → silent reuse via HTTPServer reuses the existing pending row and mints a fresh connect token so the user can resume the abandoned OAuth flow. No new row created, no error surfaced. 🟢 GREEN case 2 — explicit different slug → friendly conflict response
Row-state assertions after both callsStill exactly the two seeded rows — no orphan duplication. Connect tokens: one fresh token issued by Case 1 against Cross-user parallel flows (proven by integration suite, not re-driven over HTTP)The new per-user index also unblocks the cross-user case (User A's pending OAuth no longer blocks User B's). That path is covered by Prod migration sanityBoth pending oauth_account rows are owned by the same user but for different connectors (github + x), so neither violates the new per-user index. Table is tiny (34 rows total) — DROP+CREATE INDEX is microseconds, no need for The orphan that caused the user's original toast ( |
…t guard (#1138) Stability-audit follow-up. Adds CI guards so two recently-changed schema invariants can't silently regress in a future migration: - migration-invariants.test.ts: asserts the per-user pending oauth_account unique index from #1121 exists (and the old org-wide index is gone), plus a functional contract test — a user's second parallel pending OAuth flow collides while a distinct user's is allowed. Also asserts event_embeddings carries the embedding_model stamp column (#1069/#1080). - embedding-model-literal.test.ts: the legacy-stamp backfill migration hard-codes the model literal; this fails if it ever drifts from DEFAULT_EMBEDDING_MODEL, which would silently re-open the full-corpus recall regression. Exports DEFAULT_EMBEDDING_MODEL for the assertion.
2 participants
Summary
Two related fixes for the GitHub-connector page toast
duplicate key value violates unique constraint "auth_profiles_pending_unique":Root cause: partial-index shape was wrong.
auth_profiles_pending_uniquewas keyed on(org, connector_key, profile_kind, provider) WHERE status='pending_auth'— no caller dimension. Foroauth_account(user-personal), that blocked two members in the same org from running parallel OAuth flows for the same connector. New migration drops it and addsauth_profiles_pending_oauth_account_uniquekeyed on(org, connector_key, provider, created_by) WHERE status='pending_auth' AND profile_kind='oauth_account'. Other kinds (oauth_app,env,interactive,browser_session) are writtenstatus='active'from creation in practice, so the old per-kind bound was unused for them; the typed 23505 catch stays as a safety net.Defensive translation of the remaining collision case.
createAuthProfilecatches 23505 on the new index, looks up the colliding row, and throwsPendingAuthConflictError extends ToolUserError(409)carrying the existing profile — so callers get a structured error instead of a raw PG message bubbling to a toast.handleCreateAuthProfiledoes the same pre-INSERT lookup when no slug is supplied, so the happy-path UI repeat-click silently reuses the user's own pending row + mints a fresh connect token.findPendingAuthProfilenow takescreatedByand filters by it foroauth_account.restToolProxy'shttpStatuscast widened to allow 403/409/422.Test plan
pending-auth-conflict.test.ts(4 cases):auth_profiles.error:containing the existing slug; no "duplicate key" / no constraint name leaked.pending_authin parallel; per-user dedup reuses each user's own row.createAuthProfilecollision →PendingAuthConflictErrorwithhttpStatus=409andexistingpopulated.src/__tests__/integration/connectors/) — 59 tests pass.make review BASE=origin/main— typecheck/unit/integration all green; pi verdictbug_free 86, simplicity 88, slop 0, bugs 0, 0 blockers, tests_adequate=true, behavior_change_risk=medium.lobu runagainst this branch, seed an orphanpending_authrow, drive the connector page in the Chrome extension to confirm the new friendly message / silent reuse. Not yet done — was the gap I called out earlier.lobu-crmorg and clear it once verified unreferenced.Summary by CodeRabbit
New Features
Improvements
Bug Fixes