fix: rollback mail thread changes on patch creation failure

pgcommitfest/commitfest/tests/test_newpatch_transaction.py

@@ -0,0 +1,68 @@
+from django.db import connection
+
+from datetime import datetime, timezone
+from unittest.mock import patch
+
+import pytest
+
+from pgcommitfest.commitfest.models import MailThread, Patch
+
+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
+    # 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("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": 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.ajax.parse_and_add_attachments",
+            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": MSGID,
+                "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 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:
+        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()
+        )

pgcommitfest/commitfest/views.py

@@ -988,8 +988,11 @@ def patchform(request, patchid):
     )
 
 
+class ThreadAttachFailed(Exception):
+    pass
+
+
 @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,33 +1001,39 @@ 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
+            # 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:
-                doAttachThread(
-                    cf, patch, form.cleaned_data["threadmsgid"], request.user
-                )
+                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()
+                    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.",)
-                )
-            # 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()
+            except ThreadAttachFailed as e:
+                form._errors["threadmsgid"] = form.error_class((str(e),))
     else:
         form = NewPatchForm(request=request)