Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThe PR refactors identity database query patterns by splitting a generic Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as Delete Handler
participant DB as Database
participant Tx as Transaction
Client->>Handler: DELETE /identities/{id}
activate Handler
Handler->>DB: FindIdentityWithRatelimits
activate DB
DB-->>Handler: identity + ratelimits
deactivate DB
Handler->>Tx: BEGIN
activate Tx
Tx->>Tx: Soft DELETE identity
alt Duplicate Key Error
Tx->>DB: FindIdentityByID (current)
DB-->>Tx: check if already soft-deleted
alt Already Deleted
Tx-->>Handler: ✓ Idempotent (no audit)
else Not Yet Deleted
Tx->>DB: DeleteOldIdentityByExternalID
DB-->>Tx: cleanup old soft-deleted
Tx->>Tx: Retry soft DELETE
alt Duplicate Key Again
Tx-->>Handler: ✓ Idempotent (no audit)
else Success
Tx->>DB: INSERT audit logs
end
end
else Success
Tx->>DB: INSERT audit logs
end
Tx->>Tx: COMMIT
deactivate Tx
Handler-->>Client: 200 OK
deactivate Handler
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes This PR involves significant refactoring across handler logic, generated database code, and SQL definitions. The complexity stems from: (1) new duplicate-key error handling with idempotency checks and concurrent cleanup logic in the delete handler, (2) transaction-scoped result management in the update handler, (3) JSON parsing of ratelimits across multiple handlers, (4) changes to the core database query layer affecting multiple call sites, and (5) heterogeneous logic patterns requiring separate reasoning for each affected handler (get, delete, update, create/update key). While the generated code changes are repetitive, the handler modifications introduce non-trivial control flow and state management logic. Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)go/apps/api/routes/v2_identities_delete_identity/200_test.go (5)
⏰ 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)
🔇 Additional comments (7)
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 |
Flo4604
force-pushed
the
eng-2120-fix-timing-issue-when-deleting-same-identity-in-parallel
branch
from
0fa82d9 to
89711e4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go/apps/api/routes/v2_identities_delete_identity/handler.go (1)
182-210: Delete associated ratelimits or update audit logs
The handler logs each ratelimit as “Deleted” but only calls SoftDeleteIdentity (which updates identities) and never deletes ratelimit rows (no ON DELETE CASCADE). This leaves orphaned ratelimits and misleading logs. Either invoke DeleteManyRatelimitsByIDs/DeleteManyRatelimitsByIdentityID in the same transaction before inserting audit logs, or change the audit event to avoid implying ratelimit deletion.
🧹 Nitpick comments (8)
go/apps/api/routes/v2_keys_update_key/three_state_test.go (1)
274-277: Consider explicitly setting theDeletedfield for clarity.The
Deletedfield inFindIdentityByIDParamsis currently omitted, relying on the zero value (false). For consistency with other test files (e.g.,v2_keys_create_key/200_test.goline 286) and improved readability, consider explicitly settingDeleted: false.Apply this diff to be explicit:
identity, err := db.Query.FindIdentityByID(ctx, h.DB.RO(), db.FindIdentityByIDParams{ IdentityID: key.IdentityID.String, WorkspaceID: h.Resources().UserWorkspace.ID, + Deleted: false, })Also applies to: 321-324
go/apps/api/routes/v2_keys_update_key/200_test.go (1)
197-200: Consider explicitly setting theDeletedfield for clarity.Similar to other test files, the
Deletedfield is omitted, relying on the zero value. For consistency withv2_keys_create_key/200_test.goand improved readability, consider explicitly settingDeleted: false.Apply this diff:
identity, err := db.Query.FindIdentityByID(ctx, h.DB.RO(), db.FindIdentityByIDParams{ IdentityID: key.IdentityID.String, WorkspaceID: h.Resources().UserWorkspace.ID, + Deleted: false, })go/pkg/db/queries/identity_find_with_ratelimits.sql (1)
23-45: UNION ALL + LIMIT 1 is nondeterministic; prefer ID match deterministically.If a user-chosen external_id happens to equal some identity ID, both branches can match; without ORDER BY the returned row is undefined. Prefer the ID match deterministically (e.g., wrap UNION ALL in a subquery with a priority column and ORDER BY priority DESC LIMIT 1), or enforce that external IDs cannot collide with ID format.
go/apps/api/routes/v2_identities_delete_identity/200_test.go (1)
69-69: Prefer db.IsNotFound(err) over comparing sql.ErrNoRows.More resilient across drivers and future DB changes; improves readability.
Example change:
- require.Equal(t, sql.ErrNoRows, err) + require.True(t, db.IsNotFound(err))Also applies to: 123-123, 158-158, 239-239, 247-247, 321-321
go/apps/api/routes/v2_identities_update_identity/handler.go (1)
138-143: Don’t silently ignore malformed ratelimits JSONAt least log unmarshal errors at debug to aid diagnostics; otherwise responses may miss existing rate limits without any trace.
- if ratelimitBytes, ok := identityRow.Ratelimits.([]byte); ok && ratelimitBytes != nil { - _ = json.Unmarshal(ratelimitBytes, &existingRatelimits) // Ignore error, default to empty array - } + if ratelimitBytes, ok := identityRow.Ratelimits.([]byte); ok && ratelimitBytes != nil { + if err := json.Unmarshal(ratelimitBytes, &existingRatelimits); err != nil { + h.Logger.Debug(ctx, "failed to parse identity ratelimits JSON", "identity_id", identityRow.ID, "err", err.Error()) + } + }go/apps/api/routes/v2_identities_delete_identity/handler.go (1)
95-100: Log ratelimits JSON parse failuresRecord at debug to avoid silently dropping ratelimit delete logs.
- if ratelimitBytes, ok := identity.Ratelimits.([]byte); ok && ratelimitBytes != nil { - _ = json.Unmarshal(ratelimitBytes, &ratelimits) // Ignore error, default to empty array - } + if ratelimitBytes, ok := identity.Ratelimits.([]byte); ok && ratelimitBytes != nil { + if err := json.Unmarshal(ratelimitBytes, &ratelimits); err != nil { + h.Logger.Debug(ctx, "failed to parse ratelimits JSON for delete", "identity_id", identity.ID, "err", err.Error()) + } + }go/pkg/db/identity_find_with_ratelimits.sql_generated.go (1)
71-76: Prefer concrete type for Ratelimits to avoid type assertionsMap the JSON column to []byte or json.RawMessage via sqlc type overrides to drop interface{} and the unsafe type assertion in handlers.
go/pkg/db/identity_find.sql_generated.go (1)
12-18: Split readers by key are clear and efficientGood move removing the OR path. Ensure suitable indexes back these queries for consistent performance.
Consider composite indexes:
- (workspace_id, id, deleted)
- (workspace_id, external_id, deleted)
Also applies to: 20-27, 49-56, 57-63, 70-73
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
go/apps/api/routes/v2_identities_create_identity/200_test.go(10 hunks)go/apps/api/routes/v2_identities_delete_identity/200_test.go(4 hunks)go/apps/api/routes/v2_identities_delete_identity/handler.go(4 hunks)go/apps/api/routes/v2_identities_get_identity/handler.go(1 hunks)go/apps/api/routes/v2_identities_list_identities/200_test.go(1 hunks)go/apps/api/routes/v2_identities_update_identity/handler.go(8 hunks)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/200_test.go(1 hunks)go/apps/api/routes/v2_keys_update_key/handler.go(1 hunks)go/apps/api/routes/v2_keys_update_key/three_state_test.go(2 hunks)go/pkg/db/identity_delete_old_by_external_id.sql_generated.go(1 hunks)go/pkg/db/identity_find.sql_generated.go(1 hunks)go/pkg/db/identity_find_with_ratelimits.sql_generated.go(1 hunks)go/pkg/db/querier_generated.go(2 hunks)go/pkg/db/queries/identity_delete_old_by_external_id.sql(1 hunks)go/pkg/db/queries/identity_find.sql(1 hunks)go/pkg/db/queries/identity_find_with_ratelimits.sql(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (13)
go/apps/api/routes/v2_identities_list_identities/200_test.go (1)
go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityByExternalIDParams(20-24)
go/apps/api/routes/v2_identities_get_identity/handler.go (3)
go/pkg/db/identity_find_with_ratelimits.sql_generated.go (1)
FindIdentityWithRatelimitsParams(60-64)go/apps/api/openapi/gen.go (1)
Identity(161-173)go/pkg/db/custom_types.go (1)
RatelimitInfo(23-31)
go/apps/api/routes/v2_keys_update_key/three_state_test.go (1)
go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityByIDParams(57-61)
go/apps/api/routes/v2_keys_update_key/handler.go (1)
go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityByExternalIDParams(20-24)
go/apps/api/routes/v2_keys_create_key/200_test.go (2)
go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityByExternalIDParams(20-24)go/pkg/testutil/seed/seed.go (1)
Resources(23-28)
go/apps/api/routes/v2_keys_update_key/200_test.go (1)
go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityByIDParams(57-61)
go/apps/api/routes/v2_identities_create_identity/200_test.go (2)
go/pkg/db/identity_find.sql_generated.go (2)
FindIdentityByIDParams(57-61)FindIdentityByExternalIDParams(20-24)go/pkg/testutil/seed/seed.go (1)
Resources(23-28)
go/apps/api/routes/v2_identities_update_identity/handler.go (7)
go/pkg/db/queries.go (2)
Query(29-29)BulkQuery(31-31)go/pkg/db/identity_find_with_ratelimits.sql_generated.go (2)
FindIdentityWithRatelimitsParams(60-64)FindIdentityWithRatelimitsRow(66-76)go/apps/api/openapi/gen.go (4)
Identity(161-173)RatelimitResponse(420-435)Meta(279-282)RatelimitRequest(381-417)go/pkg/db/custom_types.go (1)
RatelimitInfo(23-31)go/pkg/db/tx.go (1)
TxWithResult(113-147)go/pkg/db/identity_insert_ratelimit.sql_generated.go (1)
InsertIdentityRatelimitParams(40-49)go/pkg/uid/uid.go (1)
RatelimitPrefix(29-29)
go/apps/api/routes/v2_keys_create_key/handler.go (2)
go/pkg/db/queries.go (1)
Query(29-29)go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityByExternalIDParams(20-24)
go/apps/api/routes/v2_identities_delete_identity/handler.go (6)
go/pkg/db/identity_find_with_ratelimits.sql_generated.go (1)
FindIdentityWithRatelimitsParams(60-64)go/pkg/db/models_generated.go (1)
Identity(629-638)go/pkg/db/custom_types.go (1)
RatelimitInfo(23-31)go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityByIDParams(57-61)go/pkg/db/identity_delete_old_by_external_id.sql_generated.go (1)
DeleteOldIdentityByExternalIDParams(22-26)go/pkg/db/handle_err_duplicate_key.go (1)
IsDuplicateKeyError(7-13)
go/pkg/db/querier_generated.go (5)
go/pkg/partition/db/database.go (1)
DBTX(10-10)go/pkg/db/identity_delete_old_by_external_id.sql_generated.go (1)
DeleteOldIdentityByExternalIDParams(22-26)go/pkg/db/identity_find.sql_generated.go (2)
FindIdentityByExternalIDParams(20-24)FindIdentityByIDParams(57-61)go/pkg/db/models_generated.go (1)
Identity(629-638)go/pkg/db/identity_find_with_ratelimits.sql_generated.go (2)
FindIdentityWithRatelimitsParams(60-64)FindIdentityWithRatelimitsRow(66-76)
go/pkg/db/identity_find.sql_generated.go (1)
go/apps/api/openapi/gen.go (2)
Identity(161-173)Meta(279-282)
go/apps/api/routes/v2_identities_delete_identity/200_test.go (4)
go/pkg/testutil/seed/seed.go (3)
CreateIdentityRequest(351-356)CreateRatelimitRequest(307-315)New(39-46)go/pkg/db/identity_find.sql_generated.go (2)
FindIdentityByExternalIDParams(20-24)FindIdentityByIDParams(57-61)go/pkg/testutil/http.go (1)
CallRoute(271-305)go/pkg/array/fill.go (1)
Fill(23-33)
⏰ 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 Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
🔇 Additional comments (15)
go/pkg/db/queries/identity_find.sql (1)
1-13: LGTM! Improved query performance.The split from a single OR-based query to two separate queries (by ID and by external ID) should improve performance by avoiding full table scans and enabling better index usage.
go/pkg/db/identity_delete_old_by_external_id.sql_generated.go (1)
1-40: LGTM! Addresses the race condition.This generated method correctly implements the cleanup of old soft-deleted identities by external ID while excluding the current identity. The LEFT JOIN ensures associated ratelimits are also removed, preventing orphaned data.
go/apps/api/routes/v2_keys_update_key/handler.go (1)
154-158: LGTM! Correct use of the new identity lookup.The change from
FindIdentitytoFindIdentityByExternalIDis appropriate here since we're looking up by the external ID from the request. The error handling and fallback logic remain intact.go/apps/api/routes/v2_keys_create_key/200_test.go (1)
283-287: LGTM! Correctly verifies identity deduplication.The change to
FindIdentityByExternalIDcorrectly verifies that only one identity was created despite concurrent key creation requests with the same external ID. The explicitDeleted: falseparameter enhances readability.go/pkg/db/querier_generated.go (1)
93-102: LGTM! Generated interface correctly reflects the new queries.The generated interface methods properly expose:
DeleteOldIdentityByExternalIDfor cleanup of old soft-deleted identitiesFindIdentityByExternalIDandFindIdentityByIDfor explicit lookup pathsFindIdentityWithRatelimitsusing UNION ALL for flexible lookups with rate limit dataAlso applies to: 268-330
go/pkg/db/queries/identity_delete_old_by_external_id.sql (1)
1-8: LGTM! Correctly handles cleanup of old identities.This query properly cleans up old soft-deleted identities by external ID while:
- Excluding the current identity (
i.id != current_identity_id)- Only targeting soft-deleted records (
i.deleted = true)- Removing associated ratelimits via LEFT JOIN
This addresses the parallel deletion race condition mentioned in the PR objectives.
go/apps/api/routes/v2_identities_list_identities/200_test.go (1)
358-358: LGTM — test aligned with FindIdentityByExternalID params.Correct workspace scoping and deleted=false filter.
go/apps/api/routes/v2_keys_create_key/handler.go (1)
213-216: LGTM — externalId-based lookup is correct and retry-suitable.Params and placement inside the transaction look good. Retry handling for duplicate/deadlock is appropriate.
go/apps/api/routes/v2_identities_update_identity/handler.go (3)
115-121: Good switch to a single fast read path + clean not-found handlingUNION-based reader avoids the OR full-scan and simplifies the flow. Looks good.
Also applies to: 128-136
396-406: Response construction via in-tx assembled ratelimitsAvoids extra SELECT. Clean and efficient.
262-275: Upsert semantics and created_at handling are correct –InsertIdentityRatelimitusesON DUPLICATE KEY UPDATEand does not overwritecreated_aton updates.go/apps/api/routes/v2_identities_delete_identity/handler.go (2)
74-79: Lookup + not-found handling LGTMFast path query and explicit not-found handling are clear.
Also applies to: 86-94
101-121: Robust idempotency for concurrent deletesDuplicate-key handling + old-deleted cleanup + retry covers the race. Good work.
Please confirm DeleteOldIdentityByExternalID only targets already-deleted rows and cascades related rows (if any).
Also applies to: 122-149
go/apps/api/routes/v2_identities_create_identity/200_test.go (1)
52-56: Tests correctly migrated to FindIdentityByID/ByExternalIDLookup helpers and param shapes updated cleanly; assertions remain equivalent.
Also applies to: 75-79, 111-115, 133-137, 174-178, 233-237, 309-313, 350-354, 380-384, 420-424
go/pkg/db/identity_find_with_ratelimits.sql_generated.go (1)
13-31: JSON_OBJECT already emits real JSON booleans forrl.auto_apply = 1, so Go’sboolunmarshal will succeed; no SQL change neededLikely an incorrect or invalid review comment.
ogzhanolguncu
left a comment
ogzhanolguncu
left a comment
There was a problem hiding this comment.
Other than the file name and organization, everything works well. I tested it manually and ran the unit tests.
|
Thank you for following the naming conventions for pull request titles! 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
go/pkg/db/queries/identity_find_by_id.sql (1)
2-6: *Use explicit column list instead of SELECT .Reduces fragility when schema changes and keeps generated code stable.
-SELECT * +SELECT id, external_id, workspace_id, environment, meta, deleted, created_at, updated_at FROM identities WHERE workspace_id = sqlc.arg(workspace_id) AND id = sqlc.arg(identity_id) AND deleted = sqlc.arg(deleted);Indexing note: The query efficiently uses the PRIMARY KEY on
id. The existing UNIQUE constraint(workspace_id, external_id, deleted)covers the external_id lookup pattern used elsewhere in the codebase. The current index strategy is adequate for this find-by-id query.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
go/pkg/db/identity_find_by_external_id.sql_generated.go(1 hunks)go/pkg/db/identity_find_by_id.sql_generated.go(1 hunks)go/pkg/db/queries/identity_find_by_external_id.sql(1 hunks)go/pkg/db/queries/identity_find_by_id.sql(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
go/pkg/db/identity_find_by_external_id.sql_generated.go (1)
go/pkg/db/models_generated.go (2)
Identity(629-638)Environment(618-627)
go/pkg/db/identity_find_by_id.sql_generated.go (1)
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). (2)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
🔇 Additional comments (3)
go/pkg/db/identity_find_by_external_id.sql_generated.go (1)
12-47: LGTM (generated).Column order matches scan targets; method signature fits existing DBTX patterns. No manual changes.
Ensure upstream callers pass contexts with timeouts/deadlines for DB calls on request paths.
go/pkg/db/identity_find_by_id.sql_generated.go (1)
12-46: LGTM (generated).Param mapping and scan order are correct; aligns with identity_find_by_id.sql.
go/pkg/db/queries/identity_find_by_external_id.sql (1)
2-6: Correct constraint design; keep SELECT * recommendation; collation relies on database default.The unique constraint
(workspace_id, external_id, deleted)is correctly designed for soft-delete patterns—it allows multiple rows per (workspace_id, external_id) pair with different deleted states. Since the query filtersdeleted = arg(deleted), single-row semantics are guaranteed. No changes needed to the constraint.The
external_idcolumn uses default MySQL collation (case-insensitive typically); no explicit collation is set. If case-sensitive matching is required by the API, this should be verified with stakeholders, but no current misconfiguration is evident.The SELECT * recommendation remains valid to avoid schema surprises:
-SELECT * +SELECT id, external_id, workspace_id, environment, meta, deleted, created_at, updated_at FROM identities WHERE workspace_id = sqlc.arg(workspace_id) AND external_id = sqlc.arg(external_id) AND deleted = sqlc.arg(deleted);
…-identity-in-parallel
…-identity-in-parallel
Graphite Automations"Notify author when CI fails" took an action on this PR • (10/22/25)1 teammate was notified to this PR based on Andreas Thomas's automation. "Post a GIF when PR approved" took an action on this PR • (10/23/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
imeyer
left a comment
imeyer
left a comment
There was a problem hiding this comment.
a nit and a random q, but LGTM otherwise!
|
|
Flo4604
deleted the
eng-2120-fix-timing-issue-when-deleting-same-identity-in-parallel
branch
3 participants
What does this PR do?
Fixes #4102 aka when you'd delete a identity we would sometimes 500 because we'd confuse which identity to hard delete and what to soft delete.
Improves the performance of finding an identity using a union query to either find id or external id as the OR was leading to a full table scan.
Splits up FindIdentity to FindIdentityByID and FindIdentityByExternalID for tests.
Selects ratelimits with the identity so we can skip the second query.
Type of change
How should this be tested?
Create identities via identities Api's, update them delete them in parallel and test around.
What didn't work was
Create Identity via API
Delete identity
Create Identity via API
Delete again
Create Identity via API
Try to delete -> we would 500 here.
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated