From d51145d30805901ce02e1971ed447de9e9960dfc Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio Date: Sun, 18 Oct 2026 09:32:16 +0200 Subject: [PATCH 1/3] Retry home and commitfest views on serialization failures The `home` and `commitfest` views run at REPEATABLE READ so all their queries see one snapshot. Rendering them can also trigger the lazy commitfest refresh in `CommitFest.relevant_commitfests()`, which locks rows with `SELECT ... FOR UPDATE`. When two requests race during a commitfest rollover, the one that loses can't lock the row that the other just updated, so PostgreSQL aborts it with "could not serialize access due to concurrent update" and the user gets a 500. These views are now wrapped in `retry_on_serialization_failure`, which sits outside `transaction.atomic` and reruns the view in a fresh transaction when it fails with a serialization failure (40001) or deadlock (40P01). On retry the winning request has already committed the refresh, so the up-to-date check short-circuits and nothing conflicts anymore. Attempts are capped at five so a persistent conflict still surfaces as an error. --- .../test_retry_on_serialization_failure.py | 76 +++++++++++++++++++ pgcommitfest/commitfest/views.py | 49 +++++++++++- 2 files changed, 124 insertions(+), 1 deletion(-) create mode 100644 pgcommitfest/commitfest/tests/test_retry_on_serialization_failure.py 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..9b7d7b0d --- /dev/null +++ b/pgcommitfest/commitfest/tests/test_retry_on_serialization_failure.py @@ -0,0 +1,76 @@ +from django.db import OperationalError + +import pytest + +from pgcommitfest.commitfest.views import retry_on_serialization_failure + + +def _pg_error(pgcode): + """Build an OperationalError that looks like one Django raised from psycopg2. + + Django re-raises the psycopg2 error with ``from`` set, so the SQLSTATE is + reachable through ``__cause__.pgcode`` -- which is exactly what the decorator + inspects. + """ + cause = Exception("underlying psycopg2 error") + cause.pgcode = pgcode + error = OperationalError("could not serialize access due to concurrent update") + error.__cause__ = cause + return error + + +def test_retries_serialization_failure_then_succeeds(): + calls = [] + + @retry_on_serialization_failure + def view(): + calls.append(1) + if len(calls) < 3: + raise _pg_error("40001") + return "ok" + + assert view() == "ok" + # Failed twice with 40001, succeeded on the third attempt. + assert len(calls) == 3 + + +def test_gives_up_after_persistent_serialization_failures(): + calls = [] + + @retry_on_serialization_failure + def view(): + calls.append(1) + raise _pg_error("40001") + + with pytest.raises(OperationalError): + view() + # Initial attempt plus retries, bounded so we never loop forever. + assert len(calls) == 5 + + +def test_deadlock_is_also_retried(): + calls = [] + + @retry_on_serialization_failure + def view(): + calls.append(1) + if len(calls) < 2: + raise _pg_error("40P01") + return "ok" + + assert view() == "ok" + assert len(calls) == 2 + + +def test_other_database_errors_are_not_retried(): + calls = [] + + @retry_on_serialization_failure + def view(): + calls.append(1) + raise _pg_error("23505") # unique_violation, not transient + + with pytest.raises(OperationalError): + view() + # Non-retryable errors propagate immediately without a second attempt. + assert len(calls) == 1 diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index 34553217..f7b4ca36 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,50 @@ 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): + # A handful of attempts is plenty: the refresh only writes once per + # rollover, so after the first one commits there is nothing left to + # conflict with. + last_attempt = 4 + for attempt in range(last_attempt + 1): + try: + return view_func(*args, **kwargs) + except OperationalError as e: + if ( + attempt == last_attempt + or getattr(e.__cause__, "pgcode", None) not in RETRYABLE_PGCODES + ): + raise + + return wrapper + def get_tags_data(): """Generate JSON data for enhanced selectize dropdown with tag colors and descriptions.""" @@ -64,6 +109,7 @@ def get_tags_data(): ) +@retry_on_serialization_failure @transaction.atomic def home(request): curs = connection.cursor() @@ -607,6 +653,7 @@ def patchlist(request, cf, personalized=False): ) +@retry_on_serialization_failure @transaction.atomic def commitfest(request, cfid): curs = connection.cursor() From 9042f902301ba99ed16489c47e777f125dbf35fb Mon Sep 17 00:00:00 2001 From: Jelterminator FennemAI-Neo Date: Sun, 18 Oct 2026 09:59:01 +0200 Subject: [PATCH 2/3] Collapse serialization retry tests into one parametrized test The four retry tests shared the same body and only differed in the SQLSTATE raised, how often it was raised, and the expected number of attempts, so they are now a single parametrized test. The non-retryable case uses 57014 (query_canceled) instead of 23505, because a unique violation surfaces as `IntegrityError` rather than `OperationalError` in Django, which made that case unrealistic. --- .../test_retry_on_serialization_failure.py | 74 +++++-------------- 1 file changed, 20 insertions(+), 54 deletions(-) diff --git a/pgcommitfest/commitfest/tests/test_retry_on_serialization_failure.py b/pgcommitfest/commitfest/tests/test_retry_on_serialization_failure.py index 9b7d7b0d..16afc1f4 100644 --- a/pgcommitfest/commitfest/tests/test_retry_on_serialization_failure.py +++ b/pgcommitfest/commitfest/tests/test_retry_on_serialization_failure.py @@ -6,12 +6,8 @@ def _pg_error(pgcode): - """Build an OperationalError that looks like one Django raised from psycopg2. - - Django re-raises the psycopg2 error with ``from`` set, so the SQLSTATE is - reachable through ``__cause__.pgcode`` -- which is exactly what the decorator - inspects. - """ + # 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") @@ -19,58 +15,28 @@ def _pg_error(pgcode): return error -def test_retries_serialization_failure_then_succeeds(): - calls = [] - - @retry_on_serialization_failure - def view(): - calls.append(1) - if len(calls) < 3: - raise _pg_error("40001") - return "ok" - - assert view() == "ok" - # Failed twice with 40001, succeeded on the third attempt. - assert len(calls) == 3 - - -def test_gives_up_after_persistent_serialization_failures(): - calls = [] - - @retry_on_serialization_failure - def view(): - calls.append(1) - raise _pg_error("40001") - - with pytest.raises(OperationalError): - view() - # Initial attempt plus retries, bounded so we never loop forever. - assert len(calls) == 5 - - -def test_deadlock_is_also_retried(): +@pytest.mark.parametrize( + "pgcode,failures,expected_calls", + [ + ("40001", 2, 3), # serialization failure, recovers + ("40P01", 1, 2), # deadlock, recovers + ("40001", 99, 5), # persistent failure, gives up after 5 attempts + ("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) < 2: - raise _pg_error("40P01") + if len(calls) <= failures: + raise _pg_error(pgcode) return "ok" - assert view() == "ok" - assert len(calls) == 2 - - -def test_other_database_errors_are_not_retried(): - calls = [] - - @retry_on_serialization_failure - def view(): - calls.append(1) - raise _pg_error("23505") # unique_violation, not transient - - with pytest.raises(OperationalError): - view() - # Non-retryable errors propagate immediately without a second attempt. - assert len(calls) == 1 + if failures >= expected_calls: + with pytest.raises(OperationalError): + view() + else: + assert view() == "ok" + assert len(calls) == expected_calls From 08c79d2b5fd98c25e3919e89fc5bf1e463dd030a Mon Sep 17 00:00:00 2001 From: Jelterminator FennemAI-Neo Date: Sun, 18 Oct 2026 10:14:09 +0200 Subject: [PATCH 3/3] Retry serialization failures only once The refresh only writes once per rollover, so once the request that won the race has committed, a retry has nothing left to conflict with. If that single retry still fails, something else is wrong and it's better to surface the error than to keep rerunning the whole view. --- .../test_retry_on_serialization_failure.py | 4 ++-- pgcommitfest/commitfest/views.py | 23 ++++++++----------- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/pgcommitfest/commitfest/tests/test_retry_on_serialization_failure.py b/pgcommitfest/commitfest/tests/test_retry_on_serialization_failure.py index 16afc1f4..bc07a6cc 100644 --- a/pgcommitfest/commitfest/tests/test_retry_on_serialization_failure.py +++ b/pgcommitfest/commitfest/tests/test_retry_on_serialization_failure.py @@ -18,9 +18,9 @@ def _pg_error(pgcode): @pytest.mark.parametrize( "pgcode,failures,expected_calls", [ - ("40001", 2, 3), # serialization failure, recovers + ("40001", 1, 2), # serialization failure, recovers ("40P01", 1, 2), # deadlock, recovers - ("40001", 99, 5), # persistent failure, gives up after 5 attempts + ("40001", 99, 2), # persistent failure, gives up after one retry ("57014", 99, 1), # query_canceled is not transient, no retry ], ) diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index f7b4ca36..7e86d7d5 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -77,19 +77,16 @@ def retry_on_serialization_failure(view_func): @functools.wraps(view_func) def wrapper(*args, **kwargs): - # A handful of attempts is plenty: the refresh only writes once per - # rollover, so after the first one commits there is nothing left to - # conflict with. - last_attempt = 4 - for attempt in range(last_attempt + 1): - try: - return view_func(*args, **kwargs) - except OperationalError as e: - if ( - attempt == last_attempt - or getattr(e.__cause__, "pgcode", None) not in RETRYABLE_PGCODES - ): - raise + 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