fix(recommendations): replace TOCTOU count+update with atomic RPC claim

src/app/actions/recommendations.test.ts

@@ -4,6 +4,7 @@ const mocks = vi.hoisted(() => {
   return {
     mockGetUser: vi.fn(),
     mockServiceFrom: vi.fn(),
+    mockServiceRpc: vi.fn(),
     mockCacheGet: vi.fn(),
     mockCacheSet: vi.fn(),
     mockCacheDel: vi.fn(),
@@ -22,6 +23,7 @@ vi.mock('@/lib/supabase/server', () => ({
 vi.mock('@/lib/supabase/service', () => ({
   getServiceSupabase: vi.fn(() => ({
     from: mocks.mockServiceFrom,
+    rpc: mocks.mockServiceRpc,
   })),
 }));
 
@@ -109,6 +111,7 @@ describe('Recommendations Server Actions', () => {
     mocks.mockGetUser.mockResolvedValue({ data: { user: { id: 'test-user-id' } } });
     mocks.mockRateLimit.mockResolvedValue({ ok: true });
     mocks.mockServiceFrom.mockImplementation(() => createMockChain(null, null));
+    mocks.mockServiceRpc.mockResolvedValue({ data: [{ id: 1 }], error: null });
   });
 
   describe('getRecommendations', () => {
@@ -190,40 +193,44 @@ describe('Recommendations Server Actions', () => {
   });
 
   describe('claimRecommendation', () => {
-    it('updates status to claimed and sets claimed_at, invalidating cache', async () => {
-      mocks.mockServiceFrom
-        .mockReturnValueOnce(createMockChain({ count: 0 })) // count claims
-        .mockReturnValueOnce(createMockChain(null, { data: { id: 1 }, error: null })) // update
-        .mockReturnValueOnce(createMockChain({})); // insert activity_log
+    it('claims the rec atomically via RPC and invalidates cache', async () => {
+      // RPC returns a row with the claimed rec ID.
+      mocks.mockServiceRpc.mockResolvedValueOnce({ data: [{ id: 1 }], error: null });
+      // insert activity_log
+      mocks.mockServiceFrom.mockReturnValueOnce(createMockChain({}));
 
       const result = await claimRecommendation(1);
 
       expect(result).toEqual({ ok: true, data: { id: 1 } });
+      expect(mocks.mockServiceRpc).toHaveBeenCalledWith('claim_recommendation_atomic', {
+        p_rec_id: 1,
+        p_user_id: 'test-user-id',
+      });
       expect(mocks.mockCacheDel).toHaveBeenCalledWith('recs:test-user-id');
     });
 
-    it('returns already_claimed error if status is not open', async () => {
-      mocks.mockServiceFrom
-        .mockReturnValueOnce(createMockChain({ count: 0 })) // count claims
-        .mockReturnValueOnce(createMockChain(null, { data: null, error: null })); // update returns null row
+    it('returns already_claimed when RPC returns zero rows and count is below limit', async () => {
+      // RPC returns empty rows (rec is not open).
+      mocks.mockServiceRpc.mockResolvedValueOnce({ data: [], error: null });
+      // Fallback count query: 1 claimed rec (below limit of 3).
+      mocks.mockServiceFrom.mockReturnValueOnce(createMockChain({ count: 1 }));
 
       const result = await claimRecommendation(1);
 
       expect(result.ok).toBe(false);
-      if (!result.ok) {
-        expect(result.error.code).toBe('already_claimed');
-      }
+      if (!result.ok) expect(result.error.code).toBe('already_claimed');
     });
 
-    it('returns claim_limit error if user has 3 or more claims', async () => {
-      mocks.mockServiceFrom.mockReturnValueOnce(createMockChain({ count: 3 })); // count claims
+    it('returns claim_limit when RPC returns zero rows and count is at limit', async () => {
+      // RPC returns empty rows (3-claim cap reached).
+      mocks.mockServiceRpc.mockResolvedValueOnce({ data: [], error: null });
+      // Fallback count query: 3 claimed recs (at limit).
+      mocks.mockServiceFrom.mockReturnValueOnce(createMockChain({ count: 3 }));
 
       const result = await claimRecommendation(1);
 
       expect(result.ok).toBe(false);
-      if (!result.ok) {
-        expect(result.error.code).toBe('claim_limit');
-      }
+      if (!result.ok) expect(result.error.code).toBe('claim_limit');
     });
 
     it('returns not_configured error if service role missing', async () => {
@@ -235,10 +242,11 @@ describe('Recommendations Server Actions', () => {
       if (!result.ok) expect(result.error.code).toBe('not_configured');
     });
 
-    it('returns persist_failed error if update fails', async () => {
-      mocks.mockServiceFrom
-        .mockReturnValueOnce(createMockChain({ count: 0 })) // count claims
-        .mockReturnValueOnce(createMockChain(null, { data: null, error: new Error('DB Error') }));
+    it('returns persist_failed when RPC returns an error', async () => {
+      mocks.mockServiceRpc.mockResolvedValueOnce({
+        data: null,
+        error: { message: 'function not found' },
+      });
 
       const result = await claimRecommendation(1);
       expect(result.ok).toBe(false);

src/app/actions/recommendations.ts

@@ -112,33 +112,39 @@ export async function claimRecommendation(recId: number): Promise<Result<{ id: n
   });
   if (!rateRes.ok) return err('rate_limited', 'slow down', true);
 
-  // Enforce 3-claim limit: count only non-expired claimed recs.  Expired claims
-  // are released by the hourly recsExpire cron, but we also filter here so users
-  // are never locked out between cron runs.
-  const now = new Date().toISOString();
-  const { count: claimedCount } = await service
-    .from('recommendations')
-    .select('id', { count: 'exact', head: true })
-    .eq('user_id', user.id)
-    .eq('status', 'claimed')
-    .gte('expires_at', now);
-  if ((claimedCount ?? 0) >= 3) {
-    return err('claim_limit', 'you already have 3 active claims - merge or close them first');
-  }
+  // Atomic claim via a PostgreSQL function.
+  //
+  // The previous implementation read the active-claim count and then wrote the
+  // status update as two separate round-trips. Concurrent requests from the
+  // same user could both pass the count check before either write committed,
+  // allowing more than 3 simultaneous claims (TOCTOU).
+  //
+  // claim_recommendation_atomic runs the count check and the UPDATE in a
+  // single statement so they execute atomically. If the user already holds 3
+  // active claims or the rec is not open, the function returns zero rows and
+  // the action returns an appropriate error — no race window exists between
+  // the check and the write.
+  const { data: rpcRows, error: rpcErr } = await service.rpc('claim_recommendation_atomic', {
+    p_rec_id: recId,
+    p_user_id: user.id,
+  });
 
-  // Atomic claim: UPDATE ... WHERE status='open' AND user_id=auth.uid()
-  // Zero rows affected = already claimed or doesn't exist.
-  const { data, error: updateErr } = await service
-    .from('recommendations')
-    .update({ status: 'claimed', claimed_at: new Date().toISOString() })
-    .eq('id', recId)
-    .eq('user_id', user.id)
-    .eq('status', 'open')
-    .select('id')
-    .maybeSingle();
+  if (rpcErr) return err('persist_failed', rpcErr.message);
 
-  if (updateErr) return err('persist_failed', updateErr.message);
-  if (!data) return err('already_claimed', 'this rec is no longer open');
+  const claimedId = (rpcRows as Array<{ id: number }>)?.[0]?.id;
+  if (claimedId === undefined) {
+    // Zero rows: either at the 3-claim limit or the rec is no longer open.
+    // Distinguish by checking whether the rec exists and is still open.
+    const { count } = await service
+      .from('recommendations')
+      .select('id', { count: 'exact', head: true })
+      .eq('user_id', user.id)
+      .eq('status', 'claimed');
+    if ((count ?? 0) >= 3) {
+      return err('claim_limit', 'you already have 3 active claims - merge or close them first');
+    }
+    return err('already_claimed', 'this rec is no longer open');
+  }
 
   // Invalidate cache so next dashboard load is fresh.
   await cacheDel(`recs:${user.id}`);
@@ -150,7 +156,7 @@ export async function claimRecommendation(recId: number): Promise<Result<{ id: n
     detail: { recId } as never,
   });
 
-  return ok({ id: data.id });
+  return ok({ id: claimedId });
 }
 
 const PR_URL_RE = /^https:\/\/github\.com\/[^/]+\/[^/]+\/pull\/\d+$/;

supabase/migrations/0012_claim_recommendation_atomic.sql

@@ -0,0 +1,46 @@
+-- Migration: atomic claim_recommendation function
+--
+-- Replaces the two-step COUNT + UPDATE in claimRecommendation with a single
+-- atomic statement. The previous implementation read the active-claim count
+-- and then wrote the status update as two independent round-trips, creating
+-- a TOCTOU window: concurrent requests from the same user could both observe
+-- a count below 3 and both succeed, allowing more than 3 simultaneous claims.
+--
+-- The function below runs the count check and the status flip in one UPDATE
+-- statement inside a single implicit transaction. PostgreSQL row-level locking
+-- on the target row (via FOR UPDATE on the SELECT inside UPDATE ... WHERE)
+-- ensures concurrent calls serialise correctly.
+--
+-- Returns the ID of the claimed recommendation if the claim succeeded, or an
+-- empty result set if the limit was already reached or the rec is not open.
+
+create or replace function claim_recommendation_atomic(
+  p_rec_id  bigint,
+  p_user_id uuid
+)
+returns table(id bigint)
+language plpgsql
+security definer
+as $$
+begin
+  return query
+  update recommendations r
+  set
+    status     = 'claimed',
+    claimed_at = now()
+  where
+    r.id      = p_rec_id
+    and r.user_id = p_user_id
+    and r.status  = 'open'
+    and (
+      select count(*)
+      from   recommendations r2
+      where  r2.user_id = p_user_id
+        and  r2.status  = 'claimed'
+    ) < 3
+  returning r.id;
+end;
+$$;
+
+-- Revoke public access; the function is called only through the service role.
+revoke all on function claim_recommendation_atomic(bigint, uuid) from public;