Skip to content

fix: identity concurrency creation err#4075

Merged
chronark merged 7 commits intomainfrom
eng-2091-duplicate-identity-index-conflict-throws-a-500
Oct 7, 2025
Merged

fix: identity concurrency creation err#4075
chronark merged 7 commits intomainfrom
eng-2091-duplicate-identity-index-conflict-throws-a-500

Conversation

@Flo4604
Copy link
Member

@Flo4604 Flo4604 commented Oct 6, 2025

What does this PR do?

Fixes #4024

This fixes the case when we can't find a identity in our current transaction, but it got created concurrently via another api call and when we insert we get a duplicate key error.

Instead we now just search for the identity again in our normal Read only connection instead of throwing a 500 errror.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (refactoring code, technical debt, workflow improvements)
  • Enhancement (small improvements)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How should this be tested?

Tests, and trying to create this error by concurrently creating the same identity

Checklist

Required

  • Filled out the "How to test" section in this PR
  • Read Contributing Guide
  • Self-reviewed my own code
  • Commented on my code in hard-to-understand areas
  • Ran pnpm build
  • Ran pnpm fmt
  • Checked for warnings, there are none
  • Removed all console.logs
  • Merged the latest changes from main onto my branch with git pull origin main
  • My changes don't cause any responsiveness issues

Appreciated

  • If a UI change was made: Added a screen recording or screenshots to this PR
  • Updated the Unkey Docs if changes were necessary

@linear
Copy link

linear bot commented Oct 6, 2025

@changeset-bot
Copy link

changeset-bot bot commented Oct 6, 2025

⚠️ No Changeset found

Latest commit: 90d96fa

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Oct 6, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
dashboard Ignored Ignored Preview Oct 7, 2025 9:47am
engineering Ignored Ignored Preview Oct 7, 2025 9:47am

@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2025

Thank you for following the naming conventions for pull request titles! 🙏

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 6, 2025

📝 Walkthrough

Walkthrough

Adds a concurrency test and implements retryable transactional flows in key create/update handlers to handle identity-creation races and MySQL deadlocks; also introduces a DB deadlock detector utility. Identity lookup/creation, ratelimit/permission/role handling, audit logging, and error wrapping are moved into retry-aware transactions.

Changes

Cohort / File(s) Summary
Concurrent key creation test
go/apps/api/routes/v2_keys_create_key/200_test.go
Adds TestCreateKeyConcurrentWithSameExternalId which runs 5 concurrent CreateKey requests with the same externalId, asserts 200 responses with key IDs, verifies all keys reference the same IdentityID, and ensures only one Identity exists for that workspace/externalId.
CreateKey handler (retry + identity race handling)
go/apps/api/routes/v2_keys_create_key/handler.go
Wraps create-key workflow in a retry loop (up to 3 attempts) to handle deadlocks and duplicate-identity races; performs identity lookup/create inside retryable transaction; moves name/meta/expires/credits/refill, encryption, permissions, ratelimits, roles, and audit log writes into the transactional path with refined error wrapping.
UpdateKey handler (retry + identity ensure + bulk updates)
go/apps/api/routes/v2_keys_update_key/handler.go
Adds retryable transaction for updates to handle identity-creation races and deadlocks; ensures/clears IdentityID based on externalId; applies conditional updates for fields, reworks ratelimit/permission/role bulk operations, enhances auditing, and refines error handling.
DB deadlock detector utility
go/pkg/db/handle_err_deadlock.go
Adds IsDeadlockError(err error) bool which unwraps errors to *mysql.MySQLError and returns true when error number is 1213 (MySQL deadlock).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Client
  participant API as API (CreateKey)
  participant TX as DB (Write Tx)
  participant RO as DB (Read)

  Note right of API: retry loop up to 3 attempts on retryable errors
  Client->>API: POST /v2/keys (externalId, workspace, params)
  API->>TX: Begin Tx -> try INSERT Identity
  alt insert succeeds
    TX-->>API: identityID
  else duplicate or deadlock
    API->>RO: FindIdentity(workspace, externalId)
    RO-->>API: existing identityID (or not found)
    alt found
      API->>TX: proceed using existing identityID
    else not found
      API->>TX: retry or propagate retryable error
    end
  end
  API->>TX: INSERT Key, encryption, permissions, ratelimits, roles, audit logs
  TX-->>API: Commit
  API-->>Client: 200 { keyId, identityId }
Loading
sequenceDiagram
  autonumber
  actor Client
  participant API as API (UpdateKey)
  participant TX as DB (Write Tx)
  participant RO as DB (Read)

  Note right of API: retry loop up to 3 attempts for identity/deadlock races
  Client->>API: PATCH /v2/keys/{id} (may include externalId, updates)
  API->>TX: Begin Tx -> conditional INSERT Identity if externalId provided
  alt insert succeeds
    TX-->>API: identityID
  else duplicate or deadlock
    API->>RO: FindIdentity(workspace, externalId)
    RO-->>API: existing identityID
    API->>TX: proceed (or retry)
  end
  API->>TX: apply updates, adjust ratelimits, permissions, roles, audit logs
  TX-->>API: Commit
  API-->>Client: 200 { updated key payload }
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • imeyer
  • chronark
  • perkinsjr

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Out of Scope Changes Check ⚠️ Warning The changes introduce broad enhancements around permissions, rate limits, roles, credits, metadata handling, and audit logging that are not mentioned in the linked issue and exceed the scope of fixing the duplicate identity index conflict. Please extract the unrelated permissions, rate-limit, and audit-logging enhancements into separate pull requests and limit this PR to the duplicate-key handling and retry logic.
✅ Passed checks (4 passed)
Check name Status Explanation
Title Check ✅ Passed The title concisely describes the primary fix addressing the concurrency error during identity creation and aligns with the changeset’s purpose.
Linked Issues Check ✅ Passed The implementation wraps the identity creation in a retry loop that catches duplicate key errors and re-queries the identity, and the new concurrency test verifies this behavior, directly satisfying issue #4024.
Description Check ✅ Passed The pull request description follows the repository template by including a clear summary with issue reference, specifying the change type, providing testing instructions, and completing the required checklist sections.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch eng-2091-duplicate-identity-index-conflict-throws-a-500

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
go/apps/api/routes/v2_keys_update_key/handler.go (1)

186-190: Avoid replica lag on post-duplicate lookup; prefer primary (RW) or add a short retry.

Using RO here escapes the transactional snapshot, but if RO points to a replica, replication lag can still make this lookup miss and return 500. Use the primary or add a bounded retry.

Suggested minimal change:

- identity, err = db.Query.FindIdentity(ctx, h.DB.RO(), db.FindIdentityParams{
+ identity, err = db.Query.FindIdentity(ctx, h.DB.RW(), db.FindIdentityParams{

Optionally add a tiny retry (e.g., 3 attempts with 5–20ms backoff) if you must keep RO for load-shedding.

Is h.DB.RO() a replica in production? If yes, please switch to RW here or add a retry to tolerate lag.

go/apps/api/routes/v2_keys_create_key/handler.go (1)

236-259: Post-duplicate lookup should hit primary; consider bounded retry and reuse.

Good fix to bypass tx snapshot. However, using RO may read from a replica and miss the just-created row due to lag.

  • Prefer primary (RW) for the lookup:
- identity, err = db.Query.FindIdentity(ctx, h.DB.RO(), db.FindIdentityParams{
+ identity, err = db.Query.FindIdentity(ctx, h.DB.RW(), db.FindIdentityParams{
  • Optionally add a short bounded retry if you must keep RO.
  • Consider extracting this get-or-create-identity logic into a shared helper to keep create/update paths consistent.

Is RO a replica in production? If so, please switch to RW here or add a retry to prevent rare 500s from replica lag.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9ffae88 and 60ea1e6.

📒 Files selected for processing (3)
  • go/apps/api/routes/v2_keys_create_key/200_test.go (1 hunks)
  • go/apps/api/routes/v2_keys_create_key/handler.go (1 hunks)
  • go/apps/api/routes/v2_keys_update_key/handler.go (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
go/apps/api/routes/v2_keys_create_key/handler.go (5)
go/pkg/db/handle_err_duplicate_key.go (1)
  • IsDuplicateKeyError (7-13)
go/pkg/fault/wrap.go (3)
  • Wrap (25-67)
  • Internal (75-89)
  • Public (97-111)
go/pkg/codes/unkey_application.go (1)
  • App (53-71)
go/pkg/db/identity_find.sql_generated.go (1)
  • FindIdentityParams (20-24)
go/pkg/db/models_generated.go (1)
  • Identity (629-638)
go/apps/api/routes/v2_keys_update_key/handler.go (2)
go/pkg/db/queries.go (1)
  • Query (29-29)
go/pkg/db/identity_find.sql_generated.go (1)
  • FindIdentityParams (20-24)
go/apps/api/routes/v2_keys_create_key/200_test.go (4)
go/pkg/testutil/http.go (3)
  • NewHarness (55-189)
  • TestResponse (241-246)
  • CallRoute (271-305)
go/pkg/testutil/seed/seed.go (2)
  • CreateApiRequest (84-92)
  • Resources (23-28)
go/pkg/db/identity_find.sql_generated.go (1)
  • FindIdentityParams (20-24)
go/pkg/db/models_generated.go (1)
  • Identity (629-638)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Test Go API Local / Test
  • GitHub Check: Test API / API Test Local
  • GitHub Check: Build / Build
  • GitHub Check: Analyze (javascript-typescript)

Copy link
Contributor

@ogzhanolguncu ogzhanolguncu left a comment

Choose a reason for hiding this comment

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

lgtm

@coderabbitai coderabbitai bot requested a review from chronark October 6, 2025 13:56
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

♻️ Duplicate comments (1)
go/apps/api/routes/v2_keys_create_key/200_test.go (1)

235-247: Stop calling testutil.CallRoute from goroutines

CallRoute still uses require.* on the parent *testing.T; invoking it from worker goroutines is not goroutine-safe and will panic if an assertion ever triggers. We already noted this in the previous review—please switch the workers to a non-asserting helper that returns (TestResponse, error) and push any failure through the channel (e.g. add the suggested CallRouteNoAssert in testutil and use it here).

-			res := testutil.CallRoute[handler.Request, handler.Response](h, route, headers, req)
-			if res.Status != 200 {
-				errors <- fmt.Errorf("unexpected status code: %d", res.Status)
-				return
-			}
-			results <- res
+			res, callErr := testutil.CallRouteNoAssert[handler.Request, handler.Response](h, route, headers, req)
+			if callErr != nil {
+				errors <- callErr
+				return
+			}
+			if res.Status != 200 {
+				errors <- fmt.Errorf("unexpected status code: %d", res.Status)
+				return
+			}
+			results <- res
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 60ea1e6 and faffe42.

📒 Files selected for processing (4)
  • go/apps/api/routes/v2_keys_create_key/200_test.go (1 hunks)
  • go/apps/api/routes/v2_keys_create_key/handler.go (1 hunks)
  • go/apps/api/routes/v2_keys_update_key/handler.go (1 hunks)
  • go/pkg/db/handle_err_deadlock.go (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-14T16:25:48.167Z
Learnt from: Flo4604
PR: unkeyed/unkey#3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.

Applied to files:

  • go/apps/api/routes/v2_keys_create_key/200_test.go
🧬 Code graph analysis (3)
go/apps/api/routes/v2_keys_create_key/200_test.go (4)
go/pkg/testutil/http.go (3)
  • NewHarness (55-189)
  • TestResponse (241-246)
  • CallRoute (271-305)
go/pkg/testutil/seed/seed.go (1)
  • CreateApiRequest (84-92)
go/pkg/db/identity_find.sql_generated.go (1)
  • FindIdentityParams (20-24)
go/pkg/db/models_generated.go (1)
  • Identity (629-638)
go/apps/api/routes/v2_keys_update_key/handler.go (15)
go/pkg/db/tx.go (1)
  • Tx (196-201)
go/pkg/db/key_update.sql_generated.go (1)
  • UpdateKeyParams (51-70)
go/pkg/db/queries.go (2)
  • Query (29-29)
  • BulkQuery (31-31)
go/pkg/db/identity_find.sql_generated.go (1)
  • FindIdentityParams (20-24)
go/pkg/db/handle_err_no_rows.go (1)
  • IsNotFound (8-10)
go/pkg/db/identity_insert.sql_generated.go (1)
  • InsertIdentityParams (31-38)
go/pkg/db/handle_err_duplicate_key.go (1)
  • IsDuplicateKeyError (7-13)
go/pkg/db/handle_err_deadlock.go (1)
  • IsDeadlockError (9-16)
go/pkg/db/ratelimit_list_by_key_id.sql_generated.go (1)
  • ListRatelimitsByKeyIDRow (24-30)
go/pkg/db/key_insert_ratelimit.sql_generated.go (1)
  • InsertKeyRatelimitParams (39-49)
go/pkg/db/permission_find_by_slugs.sql_generated.go (1)
  • FindPermissionsBySlugsParams (17-20)
go/pkg/db/permission_insert.sql_generated.go (1)
  • InsertPermissionParams (33-40)
go/pkg/db/key_permission_insert.sql_generated.go (1)
  • InsertKeyPermissionParams (27-33)
go/pkg/db/role_find_by_names.sql_generated.go (2)
  • FindRolesByNamesParams (17-20)
  • FindRolesByNamesRow (22-25)
go/pkg/db/key_role_insert.sql_generated.go (1)
  • InsertKeyRoleParams (27-32)
go/apps/api/routes/v2_keys_create_key/handler.go (16)
go/pkg/db/tx.go (1)
  • Tx (196-201)
go/pkg/db/key_insert.sql_generated.go (1)
  • InsertKeyParams (51-67)
go/pkg/db/identity_find.sql_generated.go (1)
  • FindIdentityParams (20-24)
go/pkg/db/models_generated.go (6)
  • Identity (629-638)
  • Environment (618-627)
  • AuditLog (527-543)
  • Permission (704-712)
  • Key (640-664)
  • Role (772-779)
go/pkg/db/handle_err_no_rows.go (1)
  • IsNotFound (8-10)
go/pkg/db/identity_insert.sql_generated.go (1)
  • InsertIdentityParams (31-38)
go/pkg/db/handle_err_duplicate_key.go (1)
  • IsDuplicateKeyError (7-13)
go/pkg/db/handle_err_deadlock.go (1)
  • IsDeadlockError (9-16)
go/pkg/db/key_encryption_insert.sql_generated.go (1)
  • InsertKeyEncryptionParams (18-24)
go/pkg/db/key_insert_ratelimit.sql_generated.go (1)
  • InsertKeyRatelimitParams (39-49)
go/pkg/db/permission_find_by_slugs.sql_generated.go (1)
  • FindPermissionsBySlugsParams (17-20)
go/pkg/db/permission_insert.sql_generated.go (1)
  • InsertPermissionParams (33-40)
go/pkg/db/key_permission_insert.sql_generated.go (1)
  • InsertKeyPermissionParams (27-33)
go/pkg/auditlog/events.go (3)
  • AuthConnectPermissionKeyEvent (55-55)
  • AuthConnectRoleKeyEvent (53-53)
  • KeyCreateEvent (20-20)
go/pkg/db/role_find_by_names.sql_generated.go (2)
  • FindRolesByNamesParams (17-20)
  • FindRolesByNamesRow (22-25)
go/pkg/db/key_role_insert.sql_generated.go (1)
  • InsertKeyRoleParams (27-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Test Go API Local / Test
  • GitHub Check: Test API / API Test Local
  • GitHub Check: Build / Build
  • GitHub Check: Analyze (javascript-typescript)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between faffe42 and 6032222.

📒 Files selected for processing (2)
  • go/apps/api/routes/v2_keys_create_key/handler.go (1 hunks)
  • go/apps/api/routes/v2_keys_update_key/handler.go (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
go/apps/api/routes/v2_keys_update_key/handler.go (17)
go/pkg/db/tx.go (1)
  • Tx (196-201)
go/pkg/db/key_update.sql_generated.go (1)
  • UpdateKeyParams (51-70)
go/pkg/db/queries.go (2)
  • Query (29-29)
  • BulkQuery (31-31)
go/pkg/db/identity_find.sql_generated.go (1)
  • FindIdentityParams (20-24)
go/pkg/db/handle_err_no_rows.go (1)
  • IsNotFound (8-10)
go/pkg/uid/uid.go (3)
  • IdentityPrefix (28-28)
  • RatelimitPrefix (29-29)
  • PermissionPrefix (27-27)
go/pkg/db/identity_insert.sql_generated.go (1)
  • InsertIdentityParams (31-38)
go/pkg/db/handle_err_duplicate_key.go (1)
  • IsDuplicateKeyError (7-13)
go/pkg/db/handle_err_deadlock.go (1)
  • IsDeadlockError (9-16)
go/pkg/db/ratelimit_list_by_key_id.sql_generated.go (1)
  • ListRatelimitsByKeyIDRow (24-30)
go/pkg/db/key_insert_ratelimit.sql_generated.go (1)
  • InsertKeyRatelimitParams (39-49)
go/pkg/db/permission_find_by_slugs.sql_generated.go (1)
  • FindPermissionsBySlugsParams (17-20)
go/pkg/db/permission_insert.sql_generated.go (1)
  • InsertPermissionParams (33-40)
go/pkg/auditlog/events.go (2)
  • PermissionCreateEvent (46-46)
  • KeyUpdateEvent (22-22)
go/pkg/db/key_permission_insert.sql_generated.go (1)
  • InsertKeyPermissionParams (27-33)
go/pkg/db/role_find_by_names.sql_generated.go (1)
  • FindRolesByNamesRow (22-25)
go/pkg/db/key_role_insert.sql_generated.go (1)
  • InsertKeyRoleParams (27-32)
go/apps/api/routes/v2_keys_create_key/handler.go (17)
go/pkg/db/tx.go (1)
  • Tx (196-201)
go/pkg/db/key_insert.sql_generated.go (1)
  • InsertKeyParams (51-67)
go/pkg/db/queries.go (2)
  • Query (29-29)
  • BulkQuery (31-31)
go/pkg/db/identity_find.sql_generated.go (1)
  • FindIdentityParams (20-24)
go/pkg/db/models_generated.go (6)
  • Identity (629-638)
  • Environment (618-627)
  • AuditLog (527-543)
  • Permission (704-712)
  • Key (640-664)
  • Role (772-779)
go/pkg/db/handle_err_no_rows.go (1)
  • IsNotFound (8-10)
go/pkg/db/identity_insert.sql_generated.go (1)
  • InsertIdentityParams (31-38)
go/pkg/db/handle_err_duplicate_key.go (1)
  • IsDuplicateKeyError (7-13)
go/pkg/db/handle_err_deadlock.go (1)
  • IsDeadlockError (9-16)
go/pkg/db/key_encryption_insert.sql_generated.go (1)
  • InsertKeyEncryptionParams (18-24)
go/pkg/db/key_insert_ratelimit.sql_generated.go (1)
  • InsertKeyRatelimitParams (39-49)
go/pkg/db/permission_find_by_slugs.sql_generated.go (1)
  • FindPermissionsBySlugsParams (17-20)
go/pkg/db/permission_insert.sql_generated.go (1)
  • InsertPermissionParams (33-40)
go/pkg/db/key_permission_insert.sql_generated.go (1)
  • InsertKeyPermissionParams (27-33)
go/pkg/auditlog/events.go (3)
  • AuthConnectPermissionKeyEvent (55-55)
  • AuthConnectRoleKeyEvent (53-53)
  • KeyCreateEvent (20-20)
go/pkg/db/role_find_by_names.sql_generated.go (2)
  • FindRolesByNamesParams (17-20)
  • FindRolesByNamesRow (22-25)
go/pkg/db/key_role_insert.sql_generated.go (1)
  • InsertKeyRoleParams (27-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Test API / API Test Local
  • GitHub Check: Test Go API Local / Test
  • GitHub Check: Build / Build

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
go/apps/api/routes/v2_keys_create_key/handler.go (3)

181-183: Clarify retry comment.

The comment "up to 2 times" is ambiguous—it could mean "2 attempts" or "2 retries." Given the loop for attempt := range 3 yields three iterations (0, 1, 2), this provides an initial attempt plus 2 retries (3 attempts total). Please update the comment to say "up to 2 retries (3 attempts total)" for clarity.

-	// Retry transaction up to 2 times on deadlock or identity creation race
+	// Retry transaction up to 2 retries (3 attempts total) on deadlock or identity creation race

377-417: Consider documenting permission auto-creation behavior.

Permissions are automatically created if they don't exist (lines 377-417), whereas roles must exist beforehand (lines 499-502). This behavioral difference might be intentional based on your domain model, but it could be confusing to API consumers. Consider documenting this distinction in your API documentation or adding a comment explaining the rationale.


592-603: Consider adding exponential backoff between retries.

While not critical for correctness, adding a small delay (e.g., 10ms, 50ms, 100ms) between retry attempts could reduce contention for deadlock scenarios. This would give other transactions time to complete and release locks.

For example, add a brief sleep after determining a retry is needed:

if !isRetryable {
    break
}
// Brief backoff to reduce contention
time.Sleep(time.Duration(attempt+1) * 10 * time.Millisecond)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6032222 and 90d96fa.

📒 Files selected for processing (2)
  • go/apps/api/routes/v2_keys_create_key/handler.go (1 hunks)
  • go/apps/api/routes/v2_keys_update_key/handler.go (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
go/apps/api/routes/v2_keys_create_key/handler.go (12)
go/pkg/db/tx.go (1)
  • Tx (196-201)
go/pkg/db/key_insert.sql_generated.go (1)
  • InsertKeyParams (51-67)
go/pkg/db/identity_find.sql_generated.go (1)
  • FindIdentityParams (20-24)
go/pkg/db/identity_insert.sql_generated.go (1)
  • InsertIdentityParams (31-38)
go/pkg/db/handle_err_duplicate_key.go (1)
  • IsDuplicateKeyError (7-13)
go/pkg/db/handle_err_deadlock.go (1)
  • IsDeadlockError (9-16)
go/pkg/db/key_encryption_insert.sql_generated.go (1)
  • InsertKeyEncryptionParams (18-24)
go/pkg/db/key_insert_ratelimit.sql_generated.go (1)
  • InsertKeyRatelimitParams (39-49)
go/pkg/db/permission_find_by_slugs.sql_generated.go (1)
  • FindPermissionsBySlugsParams (17-20)
go/pkg/db/permission_insert.sql_generated.go (1)
  • InsertPermissionParams (33-40)
go/pkg/db/key_permission_insert.sql_generated.go (1)
  • InsertKeyPermissionParams (27-33)
go/pkg/db/role_find_by_names.sql_generated.go (2)
  • FindRolesByNamesParams (17-20)
  • FindRolesByNamesRow (22-25)
go/apps/api/routes/v2_keys_update_key/handler.go (15)
go/pkg/db/tx.go (1)
  • Tx (196-201)
go/pkg/db/key_update.sql_generated.go (1)
  • UpdateKeyParams (51-70)
go/pkg/db/queries.go (2)
  • Query (29-29)
  • BulkQuery (31-31)
go/pkg/db/identity_find.sql_generated.go (1)
  • FindIdentityParams (20-24)
go/pkg/db/identity_insert.sql_generated.go (1)
  • InsertIdentityParams (31-38)
go/pkg/db/handle_err_duplicate_key.go (1)
  • IsDuplicateKeyError (7-13)
go/pkg/db/handle_err_deadlock.go (1)
  • IsDeadlockError (9-16)
go/pkg/db/ratelimit_list_by_key_id.sql_generated.go (1)
  • ListRatelimitsByKeyIDRow (24-30)
go/pkg/db/key_insert_ratelimit.sql_generated.go (1)
  • InsertKeyRatelimitParams (39-49)
go/pkg/db/permission_find_by_slugs.sql_generated.go (1)
  • FindPermissionsBySlugsParams (17-20)
go/pkg/db/permission_insert.sql_generated.go (1)
  • InsertPermissionParams (33-40)
go/pkg/auditlog/events.go (2)
  • PermissionCreateEvent (46-46)
  • KeyUpdateEvent (22-22)
go/pkg/db/key_permission_insert.sql_generated.go (1)
  • InsertKeyPermissionParams (27-33)
go/pkg/db/role_find_by_names.sql_generated.go (2)
  • FindRolesByNamesParams (17-20)
  • FindRolesByNamesRow (22-25)
go/pkg/db/key_role_insert.sql_generated.go (1)
  • InsertKeyRoleParams (27-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Build / Build
  • GitHub Check: Test API / API Test Local
  • GitHub Check: Test Go API Local / Test
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
go/apps/api/routes/v2_keys_update_key/handler.go (5)

110-116: Retry loop correctly implements 3-attempt strategy.

The retry loop structure properly handles up to 3 attempts for transient failures (deadlocks and identity creation races). The use of for attempt := range 3 yields attempts 0, 1, and 2, which is correct.


146-198: LGTM! Identity race condition handling is correct.

The code properly fixes the race condition described in the PR objectives:

  1. Attempts to find existing identity within the transaction (lines 154-158)
  2. Creates new identity if not found (lines 171-178)
  3. Crucially, returns unwrapped errors for duplicate key and deadlock cases (lines 181-183), allowing the retry loop to handle concurrent identity creation
  4. On retry, the FindIdentity call will succeed because the concurrent request created it

This ensures that concurrent API calls creating the same identity no longer result in 500 errors.


604-615: Retry condition logic is sound.

The differentiated retry strategy is appropriate:

  • Deadlocks: Always retryable (within the 3-attempt loop)
  • Duplicate keys: Retryable only on first 2 attempts (attempt < 2)

This makes sense because duplicate key errors after 2 retries likely indicate a persistent conflict rather than a transient race condition, warranting an immediate failure rather than continued retries.


617-627: Error handling after retry exhaustion is correct.

The final error handling properly distinguishes between retryable errors (wrapped with retry exhaustion context) and other errors (returned as-is). The error messages correctly reference the "update key" operation context.

✓ Past review comment addressed: Error messages updated from "failed to create identity" to "failed to update key after retries".


231-302: Credits handling provides flexible control.

The credit management logic allows independent control of Remaining and Refill fields:

  • Specifying Remaining: null clears remaining requests and refill settings (lines 244-249)
  • Specifying Refill subsequently re-enables refilling (lines 264-298)

This "last specified wins" pattern provides flexibility for scenarios like clearing current credits but setting a new refill schedule. The validation for monthly vs. daily intervals (lines 275-294) properly enforces constraints.

go/apps/api/routes/v2_keys_create_key/handler.go (3)

209-255: LGTM: Identity race condition handling is correct.

The identity lookup-or-create logic properly handles concurrent creation races:

  1. Searches for the identity within the transaction (line 213)
  2. If not found, attempts to create it (lines 230-237)
  3. Duplicate key and deadlock errors are returned unwrapped (lines 240-242), allowing the retry loop to detect and handle them
  4. Other errors are properly wrapped with fault context

This correctly addresses the PR objective of fixing the identity concurrency creation error.


287-294: LGTM: Good validation for monthly refill.

The validation correctly enforces that refillDay must be provided when the refill interval is monthly, preventing invalid configurations.


592-615: LGTM: Retry logic is correct.

The retry logic properly handles both deadlocks and identity creation races:

  1. Breaks on success (lines 593-595)
  2. Retries deadlocks for all attempts
  3. Retries duplicate key errors for attempts 0 and 1 (giving 3 total attempts as intended)
  4. Wraps exhausted retryable errors with appropriate messages (lines 607-612)
  5. Returns non-retryable errors as-is (line 614)

The error messages correctly reference "key" creation (addressing the past review comment).

@chronark chronark added this pull request to the merge queue Oct 7, 2025
@graphite-app
Copy link

graphite-app bot commented Oct 7, 2025

TV gif. We look up at Rowan Atkinson as Mr. Bean wearing medical scrubs. He pulls down a surgical mask, gives a gloved thumbs up, and smiles maniacally. (Added via Giphy)

@graphite-app
Copy link

graphite-app bot commented Oct 7, 2025

Graphite Automations

"Post a GIF when PR approved" took an action on this PR • (10/07/25)

1 gif was posted to this PR based on Andreas Thomas's automation.

Merged via the queue into main with commit 48fc43e Oct 7, 2025
17 checks passed
@chronark chronark deleted the eng-2091-duplicate-identity-index-conflict-throws-a-500 branch October 7, 2025 14:53
@coderabbitai coderabbitai bot mentioned this pull request Oct 16, 2025
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Duplicate identity index conflict throws a 500

3 participants