fix(cards): prevent concurrent default card race condition (#344)#385
fix(cards): prevent concurrent default card race condition (#344)#385udaycodespace wants to merge 4 commits into
Conversation
| function wireTransaction() { | ||
| mockPrisma.$transaction.mockImplementation( | ||
| async (callback: (tx: typeof mockPrisma) => Promise<unknown>) => callback(mockPrisma), | ||
| async (callback: (tx: typeof mockPrisma) => Promise<unknown>, options?: any) => callback(mockPrisma), |
There was a problem hiding this comment.
Can we not use any breaks ts safety.
There was a problem hiding this comment.
Fixed in the latest commit.
Removed the any usage from the transaction mock and switched to type-safe typings throughout the affected test code.
|
|
||
| return { id: card.id, title: card.title, isDefault: card.isDefault, links: card.cardLinks.map((cl: any) => cl.platformLink) } | ||
| } catch (error: any) { | ||
| if (error.code === 'P2034' && attempt < MAX_RETRIES) continue; |
There was a problem hiding this comment.
We have handleDB error util function can you use that here?
There was a problem hiding this comment.
I checked this one while updating the review comments.
handleDbError currently requires Fastify request/reply objects and is already used in the route handlers. Since cardService.ts is a service-layer module, I kept error propagation there and retained handleDbError usage in the route layer to preserve the existing separation of concerns.
I did add logging for unexpected failures before rethrowing and removed the TypeScript-unsafe error handling in the service.
| return { id: card.id, title: card.title, isDefault: card.isDefault, links: card.cardLinks.map((cl: any) => cl.platformLink) } | ||
| } catch (error: any) { | ||
| if (error.code === 'P2034' && attempt < MAX_RETRIES) continue; | ||
| throw error; |
There was a problem hiding this comment.
Can you add app.log.error here
There was a problem hiding this comment.
Addressed in the latest update.
Added app.log.error(error) before rethrowing unexpected failures while preserving the existing retry flow for P2034 serialization conflicts.
| isolationLevel: 'Serializable' as Prisma.TransactionIsolationLevel | ||
| }) | ||
|
|
||
| return { id: card.id, title: card.title, isDefault: card.isDefault, links: card.cardLinks.map((cl: any) => cl.platformLink) } |
There was a problem hiding this comment.
Return typed response please
There was a problem hiding this comment.
Addressed in the latest update.
Replaced the previous any-based response mapping with a typed response structure and updated the card/link mapping to operate on typed data instead of untyped objects.
|
Could please also add unit and lint tests terminal proof in the PR description. |
Addressed all review comments in the latest commit:
Please re-review when convenient. Thanks! |
|
Hi @Harxhit, all requested review changes have been addressed in the latest commit. |
Validation screenshots attached. Successfully validated:
Result:
I also attempted repository-wide lint and typecheck validation locally. The current branch reports ESLint configuration issues in packages/shared and no local TypeScript executable was available through pnpm exec tsc. Please let me know if there is a preferred validation command or if I should first sync/rebase against the latest upstream main before re-running those checks. |
I cannot see the screen shot ? Could you please check again? |
|
@udaycodespace Lint proofs too please |
I've attached the lint and typecheck outputs as requested.
The backend regression tests for this change pass successfully (23/23). For lint, the current branch reports an ESLint configuration issue in For typecheck, the current branch reports TypeScript errors, including some in files outside the scope of this PR. Please let me know if you would like me to first sync/rebase against the latest upstream main before investigating those further. |
|
CI Results — ❌ Some checks failed🖥️ Backend (❌ failure)
📱 Mobile (⏭️ skipped)
🌐 Web (⏭️ skipped)
🕐 Last updated: |




Summary
Fixes a race condition during concurrent first-card creation where multiple cards could be assigned
isDefault: truefor the same user.The first-card initialization flow now executes inside a Prisma Serializable transaction with bounded retry handling for serialization conflicts, ensuring deterministic behavior under concurrent requests.
Closes #344
Type of Change
What Changed
Concurrency Fix
apps/backend/src/services/cardService.tsto wrap first-card creation in a Prisma Serializable transaction.P2034serialization conflicts to safely recover from concurrent transaction failures.apps/backend/src/__tests__/cards.test.tscovering Serializable transaction usage and retry behavior.Review Feedback Updates (commit
bc1cc06)anyusage from the affected service and test code.app.log.error(...)before rethrowing unexpected failures.How to Test
P2034retry handling pass successfully.Checklist
console.logor debug statements left in the code.Validation Proof
Unit Tests
Commands:
pnpm prisma generate pnpm --filter backend run test src/__tests__/cards.test.tsResult:
Diff Summary
Result:
Additional Context
The previous implementation relied on a standalone
card.count()check before card creation, which introduced a TOCTOU race condition under concurrent requests.Using Serializable transaction isolation prevents concurrent transactions from successfully creating multiple default cards for the same user, while bounded retry handling allows serialization conflicts to be resolved transparently without impacting user experience.
Note
Commit
bc1cc06was added after the initial PR submission to address maintainer review feedback related to TypeScript safety, typed responses, logging, and test improvements. The underlying concurrency-fix approach (Serializable transaction + P2034 retry handling) remains unchanged.Validation proof attached below.