From 244cda58d7717293849afec5b7e9ac0ae505e7df Mon Sep 17 00:00:00 2001 From: Jonas Rocha Date: Sat, 17 Oct 2026 07:53:13 -0300 Subject: [PATCH 1/5] fix: rollback thread changes when patch creation fails --- .../tests/test_newpatch_transaction.py | 92 +++++++++++++++++++ pgcommitfest/commitfest/views.py | 7 +- 2 files changed, 96 insertions(+), 3 deletions(-) create mode 100644 pgcommitfest/commitfest/tests/test_newpatch_transaction.py diff --git a/pgcommitfest/commitfest/tests/test_newpatch_transaction.py b/pgcommitfest/commitfest/tests/test_newpatch_transaction.py new file mode 100644 index 00000000..0980bb01 --- /dev/null +++ b/pgcommitfest/commitfest/tests/test_newpatch_transaction.py @@ -0,0 +1,92 @@ +from datetime import datetime +from unittest.mock import patch + +import pytest + +from pgcommitfest.commitfest.models import MailThread, Patch + +pytestmark = pytest.mark.django_db + + +def test_newpatch_does_not_leave_orphan_thread( + client, alice, open_cf, in_progress_cf, recent_closed_cf +): + client.force_login(alice) + + message = { + "msgid": "test-thread@example.com", + "date": datetime(2025, 1, 10, 12), + "from": "author@example.com", + "subj": "[PATCH] Test", + "atts": [], + } + + with ( + patch( + "pgcommitfest.commitfest.forms._archivesAPI", + return_value=[message], + ), + patch( + "pgcommitfest.commitfest.ajax._archivesAPI", + return_value=[message], + ), + patch( + "pgcommitfest.commitfest.ajax.parse_and_add_attachments", + side_effect=RuntimeError("Simulated failure"), + ) as failure_mock, + ): + response = client.post( + f"/{open_cf.pk}/new/", + { + "name": "Transaction test", + "threadmsgid": "test-thread@example.com", + "no_tags_apply": "on", + "authors": [str(alice.pk)], + }, + ) + + failure_mock.assert_called_once() + assert response.status_code == 200 + assert not Patch.objects.filter(name="Transaction test").exists() + assert not MailThread.objects.filter(messageid="test-thread@example.com").exists() + + +def test_newpatch_creates_patch_and_attaches_thread( + client, alice, open_cf, in_progress_cf, recent_closed_cf +): + client.force_login(alice) + + message = { + "msgid": "success-thread@example.com", + "date": datetime(2025, 1, 10, 12), + "from": "author@example.com", + "subj": "[PATCH] Success", + "atts": [], + } + + with ( + patch( + "pgcommitfest.commitfest.forms._archivesAPI", + return_value=[message], + ), + patch( + "pgcommitfest.commitfest.ajax._archivesAPI", + return_value=[message], + ), + ): + response = client.post( + f"/{open_cf.pk}/new/", + { + "name": "Successful transaction test", + "threadmsgid": "success-thread@example.com", + "no_tags_apply": "on", + "authors": [str(alice.pk)], + }, + ) + + created_patch = Patch.objects.get(name="Successful transaction test") + thread = MailThread.objects.get(messageid="success-thread@example.com") + + assert response.status_code == 302 + assert response["Location"] == f"/patch/{created_patch.pk}/" + assert thread.patches.filter(pk=created_patch.pk).exists() diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index 34553217..1d757d77 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -1008,9 +1008,10 @@ def newpatch(request, cfid): ).save() # Now add the thread try: - doAttachThread( - cf, patch, form.cleaned_data["threadmsgid"], request.user - ) + with transaction.atomic(): + doAttachThread( + cf, patch, form.cleaned_data["threadmsgid"], request.user + ) return HttpResponseRedirect("/patch/%s/" % (patch.id,)) except Http404: # Thread not found! From 7339eebe474fa18f8c55fcea82b7ebe1a62037e2 Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio Date: Sun, 18 Oct 2026 09:38:39 +0200 Subject: [PATCH 2/5] Roll back the whole patch creation when attaching the thread fails Instead of only wrapping `doAttachThread` in a savepoint and then cleaning up the patch by hand with `patch.delete()`, all writes of `newpatch` now happen in a single `transaction.atomic()` block, with the exception handlers outside of it. A failure therefore rolls back the patch, its commitfest entry, its history and the mail thread together, so nothing can be left behind if more writes are added to `doAttachThread` later. Because the rollback happens before the form is re-rendered, this also works when the failure is a database error that aborted the transaction. Previously the follow-up queries would then fail with a `TransactionManagementError` and the user would get a 500. The test now covers that case as well. --- .../tests/test_newpatch_transaction.py | 18 +++++++++++-- pgcommitfest/commitfest/views.py | 26 +++++++++---------- 2 files changed, 29 insertions(+), 15 deletions(-) diff --git a/pgcommitfest/commitfest/tests/test_newpatch_transaction.py b/pgcommitfest/commitfest/tests/test_newpatch_transaction.py index 0980bb01..4b29e070 100644 --- a/pgcommitfest/commitfest/tests/test_newpatch_transaction.py +++ b/pgcommitfest/commitfest/tests/test_newpatch_transaction.py @@ -1,3 +1,5 @@ +from django.db import connection + from datetime import datetime from unittest.mock import patch @@ -8,8 +10,20 @@ pytestmark = pytest.mark.django_db +def _fail_with_database_error(*args, **kwargs): + # A real failing query, not just a raised exception, so that PostgreSQL + # aborts the transaction and any further query in it would fail too. + with connection.cursor() as cursor: + cursor.execute("SELECT 1/0") + + +@pytest.mark.parametrize( + "failure", + [RuntimeError("Simulated failure"), _fail_with_database_error], + ids=["python-error", "database-error"], +) def test_newpatch_does_not_leave_orphan_thread( - client, alice, open_cf, in_progress_cf, recent_closed_cf + client, alice, open_cf, in_progress_cf, recent_closed_cf, failure ): client.force_login(alice) @@ -32,7 +46,7 @@ def test_newpatch_does_not_leave_orphan_thread( ), patch( "pgcommitfest.commitfest.ajax.parse_and_add_attachments", - side_effect=RuntimeError("Simulated failure"), + side_effect=failure, ) as failure_mock, ): response = client.post( diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index 1d757d77..dd882d93 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -989,7 +989,6 @@ def patchform(request, patchid): @login_required -@transaction.atomic def newpatch(request, cfid): cf = get_object_or_404(CommitFest, pk=cfid) if not cf.status == CommitFest.STATUS_OPEN and not request.user.is_staff: @@ -998,17 +997,21 @@ def newpatch(request, cfid): if request.method == "POST": form = NewPatchForm(data=request.POST) if form.is_valid(): - patch = form.save() - poc = PatchOnCommitFest( - patch=patch, commitfest=cf, enterdate=datetime.now() - ) - poc.save() - PatchHistory( - patch=patch, by=request.user, what="Created patch record" - ).save() - # Now add the thread + # The exception handlers are deliberately outside the atomic + # block. That way a failure to attach the thread rolls back the + # patch and everything else created here, and we can still run + # the queries needed to re-render the form with an error, even + # if the failure was a database error that aborted the + # transaction. try: with transaction.atomic(): + patch = form.save() + PatchOnCommitFest( + patch=patch, commitfest=cf, enterdate=datetime.now() + ).save() + PatchHistory( + patch=patch, by=request.user, what="Created patch record" + ).save() doAttachThread( cf, patch, form.cleaned_data["threadmsgid"], request.user ) @@ -1023,9 +1026,6 @@ def newpatch(request, cfid): form._errors["threadmsgid"] = form.error_class( ("An error occurred looking up the thread in the archives.",) ) - # In this case, we have created a patch - delete it. This causes a agp in id's, but it should - # not happen very often. If we successfully attached to it, we will have already returned. - patch.delete() else: form = NewPatchForm(request=request) From e6b13682375557db9f449f68cc7ff695ccdbe2bf Mon Sep 17 00:00:00 2001 From: Jelterminator FennemAI-Neo Date: Sun, 18 Oct 2026 09:59:35 +0200 Subject: [PATCH 3/5] Trim newpatch transaction tests The database-error case already covers everything the plain Python exception case checked, since a real failing query is the stricter scenario that needs the exception handlers outside of the `transaction.atomic()` block. The success test duplicated almost all of the setup, so it is folded into the same parametrized test. --- .../tests/test_newpatch_transaction.py | 93 ++++++------------- 1 file changed, 27 insertions(+), 66 deletions(-) diff --git a/pgcommitfest/commitfest/tests/test_newpatch_transaction.py b/pgcommitfest/commitfest/tests/test_newpatch_transaction.py index 4b29e070..8107315e 100644 --- a/pgcommitfest/commitfest/tests/test_newpatch_transaction.py +++ b/pgcommitfest/commitfest/tests/test_newpatch_transaction.py @@ -1,6 +1,6 @@ from django.db import connection -from datetime import datetime +from datetime import datetime, timezone from unittest.mock import patch import pytest @@ -9,6 +9,8 @@ pytestmark = pytest.mark.django_db +MSGID = "test-thread@example.com" + def _fail_with_database_error(*args, **kwargs): # A real failing query, not just a raised exception, so that PostgreSQL @@ -17,90 +19,49 @@ def _fail_with_database_error(*args, **kwargs): cursor.execute("SELECT 1/0") -@pytest.mark.parametrize( - "failure", - [RuntimeError("Simulated failure"), _fail_with_database_error], - ids=["python-error", "database-error"], -) -def test_newpatch_does_not_leave_orphan_thread( - client, alice, open_cf, in_progress_cf, recent_closed_cf, failure +@pytest.mark.parametrize("fail", [False, True], ids=["success", "database-error"]) +def test_newpatch_thread_attach( + client, alice, open_cf, in_progress_cf, recent_closed_cf, fail ): client.force_login(alice) - message = { - "msgid": "test-thread@example.com", - "date": datetime(2025, 1, 10, 12), + "msgid": MSGID, + "date": datetime(2025, 1, 10, 12, tzinfo=timezone.utc), "from": "author@example.com", "subj": "[PATCH] Test", "atts": [], } with ( - patch( - "pgcommitfest.commitfest.forms._archivesAPI", - return_value=[message], - ), - patch( - "pgcommitfest.commitfest.ajax._archivesAPI", - return_value=[message], - ), + patch("pgcommitfest.commitfest.forms._archivesAPI", return_value=[message]), + patch("pgcommitfest.commitfest.ajax._archivesAPI", return_value=[message]), patch( "pgcommitfest.commitfest.ajax.parse_and_add_attachments", - side_effect=failure, - ) as failure_mock, + side_effect=_fail_with_database_error if fail else None, + ) as attachments_mock, ): response = client.post( f"/{open_cf.pk}/new/", { "name": "Transaction test", - "threadmsgid": "test-thread@example.com", + "threadmsgid": MSGID, "no_tags_apply": "on", "authors": [str(alice.pk)], }, ) - failure_mock.assert_called_once() - assert response.status_code == 200 - assert not Patch.objects.filter(name="Transaction test").exists() - assert not MailThread.objects.filter(messageid="test-thread@example.com").exists() - - -def test_newpatch_creates_patch_and_attaches_thread( - client, alice, open_cf, in_progress_cf, recent_closed_cf -): - client.force_login(alice) - - message = { - "msgid": "success-thread@example.com", - "date": datetime(2025, 1, 10, 12), - "from": "author@example.com", - "subj": "[PATCH] Success", - "atts": [], - } - - with ( - patch( - "pgcommitfest.commitfest.forms._archivesAPI", - return_value=[message], - ), - patch( - "pgcommitfest.commitfest.ajax._archivesAPI", - return_value=[message], - ), - ): - response = client.post( - f"/{open_cf.pk}/new/", - { - "name": "Successful transaction test", - "threadmsgid": "success-thread@example.com", - "no_tags_apply": "on", - "authors": [str(alice.pk)], - }, + attachments_mock.assert_called_once() + if fail: + # The form is re-rendered with an error and nothing is left behind + assert response.status_code == 200 + assert not Patch.objects.filter(name="Transaction test").exists() + assert not MailThread.objects.filter(messageid=MSGID).exists() + else: + created_patch = Patch.objects.get(name="Transaction test") + assert response.status_code == 302 + assert response["Location"] == f"/patch/{created_patch.pk}/" + assert ( + MailThread.objects.get(messageid=MSGID) + .patches.filter(pk=created_patch.pk) + .exists() ) - - created_patch = Patch.objects.get(name="Successful transaction test") - thread = MailThread.objects.get(messageid="success-thread@example.com") - - assert response.status_code == 302 - assert response["Location"] == f"/patch/{created_patch.pk}/" - assert thread.patches.filter(pk=created_patch.pk).exists() From e30082cb0173612099d63b7254ecbc6535e05138 Mon Sep 17 00:00:00 2001 From: Jelterminator FennemAI-Neo Date: Sun, 18 Oct 2026 10:58:58 +0200 Subject: [PATCH 4/5] Only turn thread attach failures into a form error Moving patch creation into the same atomic block as `doAttachThread` also moved `form.save()` and the `PatchOnCommitFest`/`PatchHistory` saves under the broad `except Exception`. A failure in any of those was then shown as "An error occurred looking up the thread in the archives" and never logged, where it used to be a 500 with a traceback. Now only exceptions from `doAttachThread` are caught. They are re-raised as a private `_ThreadAttachFailed`, so they still propagate out of the atomic block and roll everything back, and the form error is set once the transaction is gone. Anything else fails the request like before. --- .../tests/test_newpatch_transaction.py | 1 + pgcommitfest/commitfest/views.py | 46 +++++++++++-------- 2 files changed, 28 insertions(+), 19 deletions(-) diff --git a/pgcommitfest/commitfest/tests/test_newpatch_transaction.py b/pgcommitfest/commitfest/tests/test_newpatch_transaction.py index 8107315e..ca78d14c 100644 --- a/pgcommitfest/commitfest/tests/test_newpatch_transaction.py +++ b/pgcommitfest/commitfest/tests/test_newpatch_transaction.py @@ -54,6 +54,7 @@ def test_newpatch_thread_attach( if fail: # The form is re-rendered with an error and nothing is left behind assert response.status_code == 200 + assert b"An error occurred looking up the thread" in response.content assert not Patch.objects.filter(name="Transaction test").exists() assert not MailThread.objects.filter(messageid=MSGID).exists() else: diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index dd882d93..a01858c1 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -988,6 +988,10 @@ def patchform(request, patchid): ) +class _ThreadAttachFailed(Exception): + pass + + @login_required def newpatch(request, cfid): cf = get_object_or_404(CommitFest, pk=cfid) @@ -997,12 +1001,14 @@ def newpatch(request, cfid): if request.method == "POST": form = NewPatchForm(data=request.POST) if form.is_valid(): - # The exception handlers are deliberately outside the atomic - # block. That way a failure to attach the thread rolls back the - # patch and everything else created here, and we can still run - # the queries needed to re-render the form with an error, even - # if the failure was a database error that aborted the - # transaction. + # A failure to attach the thread is re-raised as + # _ThreadAttachFailed so that it still propagates out of the + # atomic block, which rolls back the patch and everything else + # created here. The form error is only set after leaving the + # block, so the queries needed to re-render the form still work + # even if the failure was a database error that aborted the + # transaction. Failures in the other statements are not caught + # here, so they still result in a logged 500. try: with transaction.atomic(): patch = form.save() @@ -1012,20 +1018,22 @@ def newpatch(request, cfid): PatchHistory( patch=patch, by=request.user, what="Created patch record" ).save() - doAttachThread( - cf, patch, form.cleaned_data["threadmsgid"], request.user - ) + try: + doAttachThread( + cf, patch, form.cleaned_data["threadmsgid"], request.user + ) + except Http404 as e: + # This is a horrible breakage of API layers + raise _ThreadAttachFailed( + "Selected thread did not exist in the archives" + ) from e + except Exception as e: + raise _ThreadAttachFailed( + "An error occurred looking up the thread in the archives." + ) from e return HttpResponseRedirect("/patch/%s/" % (patch.id,)) - except Http404: - # Thread not found! - # This is a horrible breakage of API layers - form._errors["threadmsgid"] = form.error_class( - ("Selected thread did not exist in the archives",) - ) - except Exception: - form._errors["threadmsgid"] = form.error_class( - ("An error occurred looking up the thread in the archives.",) - ) + except _ThreadAttachFailed as e: + form._errors["threadmsgid"] = form.error_class((str(e),)) else: form = NewPatchForm(request=request) From 08716c33a7cae36393d696b3fea4d0bd29937757 Mon Sep 17 00:00:00 2001 From: Jelterminator FennemAI-Neo Date: Sun, 18 Oct 2026 11:00:05 +0200 Subject: [PATCH 5/5] Drop the underscore from ThreadAttachFailed --- pgcommitfest/commitfest/views.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index a01858c1..7e03030d 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -988,7 +988,7 @@ def patchform(request, patchid): ) -class _ThreadAttachFailed(Exception): +class ThreadAttachFailed(Exception): pass @@ -1002,7 +1002,7 @@ def newpatch(request, cfid): form = NewPatchForm(data=request.POST) if form.is_valid(): # A failure to attach the thread is re-raised as - # _ThreadAttachFailed so that it still propagates out of the + # ThreadAttachFailed so that it still propagates out of the # atomic block, which rolls back the patch and everything else # created here. The form error is only set after leaving the # block, so the queries needed to re-render the form still work @@ -1024,15 +1024,15 @@ def newpatch(request, cfid): ) except Http404 as e: # This is a horrible breakage of API layers - raise _ThreadAttachFailed( + raise ThreadAttachFailed( "Selected thread did not exist in the archives" ) from e except Exception as e: - raise _ThreadAttachFailed( + raise ThreadAttachFailed( "An error occurred looking up the thread in the archives." ) from e return HttpResponseRedirect("/patch/%s/" % (patch.id,)) - except _ThreadAttachFailed as e: + except ThreadAttachFailed as e: form._errors["threadmsgid"] = form.error_class((str(e),)) else: form = NewPatchForm(request=request)