Retry home and commitfest views on serialization failures (#125)

When two requests to the `home` or `commitfest` views race during a
commitfest rollover, the lazy commitfest refresh makes one of them fail
with "could not serialize access due to concurrent update", because
these views run at REPEATABLE READ. That resulted in a 500 for the user.
Now these views are retried once in a fresh transaction on serialization
failures and deadlocks. At that point the other request has already
committed the refresh, so there's nothing left to conflict with.

---------

Co-authored-by: Jelterminator FennemAI-Neo <fennemai@jeltef.nl>

Author: JelteF

pgcommitfest/commitfest/tests/test_retry_on_serialization_failure.py

@@ -0,0 +1,42 @@
+from django.db import OperationalError
+
+import pytest
+
+from pgcommitfest.commitfest.views import retry_on_serialization_failure
+
+
+def _pg_error(pgcode):
+    # Django re-raises psycopg2 errors with ``from``, so the SQLSTATE is only
+    # reachable through ``__cause__.pgcode``, which is what the decorator reads.
+    cause = Exception("underlying psycopg2 error")
+    cause.pgcode = pgcode
+    error = OperationalError("could not serialize access due to concurrent update")
+    error.__cause__ = cause
+    return error
+
+
+@pytest.mark.parametrize(
+    "pgcode,failures,expected_calls",
+    [
+        ("40001", 1, 2),  # serialization failure, recovers
+        ("40P01", 1, 2),  # deadlock, recovers
+        ("40001", 99, 2),  # persistent failure, gives up after one retry
+        ("57014", 99, 1),  # query_canceled is not transient, no retry
+    ],
+)
+def test_retry_on_serialization_failure(pgcode, failures, expected_calls):
+    calls = []
+
+    @retry_on_serialization_failure
+    def view():
+        calls.append(1)
+        if len(calls) <= failures:
+            raise _pg_error(pgcode)
+        return "ok"
+
+    if failures >= expected_calls:
+        with pytest.raises(OperationalError):
+            view()
+    else:
+        assert view() == "ok"
+    assert len(calls) == expected_calls

pgcommitfest/commitfest/views.py

@@ -2,7 +2,7 @@
 from django.contrib import messages
 from django.contrib.auth.decorators import login_required
 from django.contrib.auth.models import User
-from django.db import connection, transaction
+from django.db import OperationalError, connection, transaction
 from django.db.models import Count, Q
 from django.http import (
     Http404,
@@ -15,6 +15,7 @@
 from django.views.decorators.csrf import csrf_exempt
 
 import collections
+import functools
 import hmac
 import json
 import urllib
@@ -48,6 +49,47 @@
     UserInputError,
 )
 
+# PostgreSQL SQLSTATEs for transient conflicts that are safe to retry once the
+# aborted transaction has been rolled back: serialization_failure and
+# deadlock_detected.
+RETRYABLE_PGCODES = {"40001", "40P01"}
+
+
+def retry_on_serialization_failure(view_func):
+    """Re-run a view when its transaction hits a serialization failure.
+
+    A few views (home, commitfest) run the whole request at REPEATABLE READ so
+    that all of their queries observe a single consistent snapshot. Rendering
+    also triggers the lazy, roughly-once-a-month commitfest refresh in
+    CommitFest.relevant_commitfests(), which locks and updates rows with
+    SELECT ... FOR UPDATE. When two such requests race during a rollover, the
+    one that loses the race cannot re-read the newer row under REPEATABLE READ,
+    so PostgreSQL aborts it with "could not serialize access due to concurrent
+    update" (SQLSTATE 40001) instead of blocking and continuing.
+
+    Once that happens the transaction is dead, so the only way to recover is to
+    roll it back and run the view again from scratch. That is why this decorator
+    has to sit *outside* transaction.atomic: each attempt then runs in a brand
+    new transaction with a fresh snapshot. By the time we retry, the request
+    that won the race has already committed the refresh, so the cheap
+    up-to-date check short-circuits and the retry neither writes nor conflicts.
+    """
+
+    @functools.wraps(view_func)
+    def wrapper(*args, **kwargs):
+        try:
+            return view_func(*args, **kwargs)
+        except OperationalError as e:
+            if getattr(e.__cause__, "pgcode", None) not in RETRYABLE_PGCODES:
+                raise
+        # A single retry is enough: the refresh only writes once per rollover,
+        # so after the winning request commits there is nothing left to
+        # conflict with. If the retry fails anyway, something else is going on
+        # and we'd rather surface the error than keep hammering the database.
+        return view_func(*args, **kwargs)
+
+    return wrapper
+
 
 def get_tags_data():
     """Generate JSON data for enhanced selectize dropdown with tag colors and descriptions."""
@@ -64,6 +106,7 @@ def get_tags_data():
     )
 
 
+@retry_on_serialization_failure
 @transaction.atomic
 def home(request):
     curs = connection.cursor()
@@ -619,6 +662,7 @@ def patchlist(request, cf, personalized=False):
     )
 
 
+@retry_on_serialization_failure
 @transaction.atomic
 def commitfest(request, cfid):
     curs = connection.cursor()