diff --git a/pgcommitfest/commitfest/tests/test_retry_on_serialization_failure.py b/pgcommitfest/commitfest/tests/test_retry_on_serialization_failure.py new file mode 100644 index 00000000..bc07a6cc --- /dev/null +++ b/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 diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index 34553217..7e86d7d5 100644 --- a/pgcommitfest/commitfest/views.py +++ b/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() @@ -607,6 +650,7 @@ def patchlist(request, cf, personalized=False): ) +@retry_on_serialization_failure @transaction.atomic def commitfest(request, cfid): curs = connection.cursor()