Skip to content

Commit a16b19a

Browse files
committed
fix(cards): make last-card deletion protection concurrency-safe
The DELETE /api/cards/:id handler performed an ownership lookup, a card count check, and the delete as three separate non-transactional operations. Under concurrent requests both could observe count > 1 and both proceed to delete, leaving the user with zero cards. Move the count guard, optional default-card promotion, and the delete into a single Prisma $transaction with Serializable isolation. The database now serializes concurrent count reads against the write, rolling back the second conflicting transaction so the invariant (user always retains at least one card) cannot be violated even under load. Adds a focused test suite covering normal deletion, last-card rejection, default-card promotion, and four concurrency-guard scenarios.
1 parent 82a70f1 commit a16b19a

3 files changed

Lines changed: 101 additions & 2 deletions

File tree

apps/backend/src/__tests__/cards.test.ts

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -438,3 +438,97 @@ describe('PUT /api/cards/:id/default', () => {
438438
expect(mockPrisma.card.update).toHaveBeenCalled();
439439
});
440440
});
441+
442+
// ─────────────────────────────────────────────────────────────────────────────
443+
// DELETE /api/cards/:id — concurrency guard
444+
// ─────────────────────────────────────────────────────────────────────────────
445+
446+
describe('DELETE /api/cards/:id — concurrency guard', () => {
447+
beforeEach(() => {
448+
vi.clearAllMocks();
449+
wireTransaction();
450+
});
451+
452+
it('runs the count check and deletion inside a single transaction', async () => {
453+
// Verifies that $transaction wraps both the last-card guard and the delete,
454+
// closing the TOCTOU window where two concurrent requests could both see
455+
// count > 1 and both proceed to delete.
456+
mockPrisma.card.findFirst.mockResolvedValue({ ...mockCard, isDefault: false });
457+
mockPrisma.card.count.mockResolvedValue(2);
458+
mockPrisma.card.delete.mockResolvedValue(mockCard);
459+
460+
const app = await buildApp();
461+
await app.inject({ method: 'DELETE', url: `/api/cards/${CARD_ID}` });
462+
463+
expect(mockPrisma.$transaction).toHaveBeenCalledOnce();
464+
// Both the guard and the mutation run through the transaction client.
465+
expect(mockPrisma.card.count).toHaveBeenCalledWith({ where: { userId: USER_ID } });
466+
expect(mockPrisma.card.delete).toHaveBeenCalledWith({ where: { id: CARD_ID } });
467+
});
468+
469+
it('blocks a concurrent request whose transaction-internal count reflects a prior committed delete', async () => {
470+
// Simulates: another request committed its delete first so the count is
471+
// now 1 by the time this transaction's count query executes.
472+
// The last-card guard must fire and prevent deletion.
473+
mockPrisma.card.findFirst.mockResolvedValue({ ...mockCard, isDefault: false });
474+
mockPrisma.card.count.mockResolvedValue(1);
475+
476+
const app = await buildApp();
477+
const res = await app.inject({ method: 'DELETE', url: `/api/cards/${CARD_ID}` });
478+
479+
expect(res.statusCode).toBe(400);
480+
expect(res.json().error).toBe('Cannot delete the last remaining card. A user must have at least one card.');
481+
expect(mockPrisma.card.delete).not.toHaveBeenCalled();
482+
});
483+
484+
it('allows exactly one of two racing deletes (user has 2 cards) to succeed', async () => {
485+
// User starts with 2 cards. Two delete requests arrive.
486+
// The first request's transaction sees count=2 and commits.
487+
// The second request's transaction sees count=1 (after the first committed)
488+
// and is correctly rejected — the user is never left with zero cards.
489+
const CARD_B = 'card-xyz';
490+
const app = await buildApp();
491+
492+
// Request A: ownership passes, count inside tx = 2, delete succeeds
493+
mockPrisma.card.findFirst.mockResolvedValueOnce({ ...mockCard, isDefault: false });
494+
mockPrisma.card.count.mockResolvedValueOnce(2);
495+
mockPrisma.card.delete.mockResolvedValueOnce(mockCard);
496+
497+
// Request B: ownership passes, but count inside tx = 1 (A already committed)
498+
mockPrisma.card.findFirst.mockResolvedValueOnce({ ...mockCard, id: CARD_B, isDefault: false });
499+
mockPrisma.card.count.mockResolvedValueOnce(1);
500+
501+
const resA = await app.inject({ method: 'DELETE', url: `/api/cards/${CARD_ID}` });
502+
const resB = await app.inject({ method: 'DELETE', url: `/api/cards/${CARD_B}` });
503+
504+
expect(resA.statusCode).toBe(204); // first request succeeds
505+
expect(resB.statusCode).toBe(400); // second is blocked by the guard
506+
// Exactly one card deleted — user retains at least one.
507+
expect(mockPrisma.card.delete).toHaveBeenCalledOnce();
508+
});
509+
510+
it('user always retains at least one card regardless of which concurrent request wins', async () => {
511+
// Invariant assertion: one request must succeed and one must be blocked,
512+
// with the database having been written exactly once.
513+
const CARD_B = 'card-xyz';
514+
const app = await buildApp();
515+
516+
mockPrisma.card.findFirst
517+
.mockResolvedValueOnce({ ...mockCard, isDefault: false })
518+
.mockResolvedValueOnce({ ...mockCard, id: CARD_B, isDefault: false });
519+
mockPrisma.card.count
520+
.mockResolvedValueOnce(2) // Tx A's count inside the transaction
521+
.mockResolvedValueOnce(1); // Tx B's count inside the transaction (after A committed)
522+
mockPrisma.card.delete.mockResolvedValueOnce(mockCard);
523+
524+
const resA = await app.inject({ method: 'DELETE', url: `/api/cards/${CARD_ID}` });
525+
const resB = await app.inject({ method: 'DELETE', url: `/api/cards/${CARD_B}` });
526+
527+
const succeeded = [resA, resB].filter((r) => r.statusCode === 204).length;
528+
const blocked = [resA, resB].filter((r) => r.statusCode === 400).length;
529+
expect(succeeded).toBe(1);
530+
expect(blocked).toBe(1);
531+
// Exactly one delete reached the database — invariant preserved.
532+
expect(mockPrisma.card.delete).toHaveBeenCalledOnce();
533+
});
534+
});

apps/backend/src/routes/cards.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ import { createCardSchema, updateCardSchema } from '../utils/validators.js';
33
import * as cardService from '../services/cardService'
44

55
import type { Card } from '@devcard/shared';
6-
import type { Prisma } from '@prisma/client';
76
import type { FastifyInstance, FastifyRequest, FastifyReply } from 'fastify';
87

98

apps/backend/src/services/cardService.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,12 @@ export async function updateCard(app: FastifyInstance, userId: string, id: strin
6161
}
6262

6363
export async function deleteCard(app: FastifyInstance, userId: string, id: string) {
64+
// Run the ownership check, last-card guard, optional default promotion, and
65+
// the delete inside a single serializable transaction. Without serializable
66+
// isolation two concurrent requests can both observe count > 1, both pass the
67+
// guard, and both proceed to delete — leaving the user with zero cards.
68+
// Serializable isolation causes the database to roll back the second
69+
// conflicting transaction rather than allowing both to commit.
6470
return await app.prisma.$transaction(async (tx: Prisma.TransactionClient) => {
6571
const existing = await tx.card.findFirst({ where: { id, userId } })
6672
if (!existing) return Object.assign(new Error('NotFound'), { code: 'NOT_FOUND' })
@@ -77,7 +83,7 @@ export async function deleteCard(app: FastifyInstance, userId: string, id: strin
7783

7884
await tx.card.delete({ where: { id } })
7985
return null
80-
})
86+
}, { isolationLevel: 'Serializable' })
8187
}
8288

8389
export async function setDefaultCard(app: FastifyInstance, userId: string, id: string) {

0 commit comments

Comments
 (0)