From 8582c38529d2dbe7408f8792cc837579c4e6bc41 Mon Sep 17 00:00:00 2001
From: Arnold Cartagena <cartagena.arnold@gmail.com>
Date: Mon, 2 Mar 2026 07:44:02 +0000
Subject: [PATCH] fix: prevent race condition in approval decisions

CRITICAL: Use atomic UPDATE with status='pending' check in WHERE clause
to prevent concurrent requests from both deciding the same approval.

Before: Two concurrent requests could both read status='pending' and
both modify the approval, causing duplicate notifications and
confused audit trails.

After: Only one request wins the race; the other gets None (already
decided). Uses RETURNING clause to get the updated approval.

Adds test_concurrent_decisions_race_condition to verify the fix.
---
 .../services/approval_service.py              | 32 +++++++----
 tests/test_approvals.py                       | 54 +++++++++++++++++++
 2 files changed, 76 insertions(+), 10 deletions(-)

diff --git a/src/edictum_server/services/approval_service.py b/src/edictum_server/services/approval_service.py
index b124f8a..f41ae00 100644
--- a/src/edictum_server/services/approval_service.py
+++ b/src/edictum_server/services/approval_service.py
@@ -102,17 +102,29 @@ async def submit_decision(
 ) -> Approval | None:
     """Submit a human decision on a pending approval.
 
-    Returns None if not found or already decided.
+    Uses atomic UPDATE with status='pending' in WHERE clause to prevent
+    race conditions where concurrent requests could both decide the same approval.
+
+    Returns None if not found or already decided (including if another
+    concurrent request decided it first).
     """
-    approval = await get_approval(db, tenant_id, approval_id)
-    if approval is None or approval.status != "pending":
-        return None
-
-    approval.status = "approved" if approved else "denied"
-    approval.decided_by = decided_by
-    approval.decided_at = datetime.now(UTC)
-    approval.decision_reason = reason
-    approval.decided_via = decided_via
+    result = await db.execute(
+        update(Approval)
+        .where(
+            Approval.id == approval_id,
+            Approval.tenant_id == tenant_id,
+            Approval.status == "pending",  # Atomic check - prevents race condition
+        )
+        .values(
+            status="approved" if approved else "denied",
+            decided_by=decided_by,
+            decided_at=datetime.now(UTC),
+            decision_reason=reason,
+            decided_via=decided_via or "console",
+        )
+        .returning(Approval)
+    )
+    approval = result.scalar_one_or_none()
     await db.flush()
     return approval
 
diff --git a/tests/test_approvals.py b/tests/test_approvals.py
index 509eaa1..79f2318 100644
--- a/tests/test_approvals.py
+++ b/tests/test_approvals.py
@@ -141,3 +141,57 @@ async def test_expire_approvals(
     resp = await client.get(f"/api/v1/approvals/{created['id']}")
     assert resp.status_code == 200
     assert resp.json()["status"] == "timeout"
+
+
+async def test_concurrent_decisions_race_condition(
+    client: AsyncClient,
+    db_session: AsyncSession,
+) -> None:
+    """Test that concurrent decisions on the same approval only succeed once.
+
+    This tests the atomic UPDATE in submit_decision() that prevents race conditions
+    where two concurrent requests could both decide the same approval.
+    """
+    import asyncio
+
+    from edictum_server.services.approval_service import submit_decision
+
+    created = await _create_approval(client)
+    approval_id = uuid.UUID(created["id"])
+
+    # Get tenant_id from the approval record
+    approval = await db_session.get(Approval, approval_id)
+    assert approval is not None
+    tenant_id = approval.tenant_id
+
+    # Simulate two concurrent decision attempts on the same approval
+    async def decide(approved: bool, decided_by: str):
+        return await submit_decision(
+            db_session,
+            tenant_id=tenant_id,
+            approval_id=approval_id,
+            approved=approved,
+            decided_by=decided_by,
+        )
+
+    # Run both decisions concurrently
+    results = await asyncio.gather(
+        decide(True, "admin1"),
+        decide(False, "admin2"),
+        return_exceptions=True,
+    )
+
+    # Count how many succeeded (returned non-None Approval)
+    successful = [r for r in results if r is not None and not isinstance(r, Exception)]
+
+    # Only ONE should succeed - the atomic UPDATE should prevent both from winning
+    assert (
+        len(successful) == 1
+    ), f"Expected exactly 1 successful decision, got {len(successful)}"
+
+    # Verify the approval has the winner's decision
+    await db_session.commit()
+    approval = await db_session.get(Approval, approval_id)
+    assert approval is not None
+    assert approval.status in ("approved", "denied")
+    assert approval.decided_by in ("admin1", "admin2")