Repository navigation
fix: rollback mail thread changes on patch creation failure - #122
Merged
JelteF merged 5 commits intoOct 18, 2026
Merged
Conversation
JelteF
reviewed
Oct 18, 2026
| doAttachThread( | ||
| cf, patch, form.cleaned_data["threadmsgid"], request.user | ||
| ) | ||
| with transaction.atomic(): |
Collaborator
There was a problem hiding this comment.
This whole function already has transaction.atomic. So I'm unsure why this is needed.
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.
JelteF
approved these changes
Oct 18, 2026
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When attaching the thread failed during patch creation, the patch was deleted again, but the
MailThreadcreated bydoAttachThreadwas left behind. And if the failure was a database error, re-rendering the form failed because the transaction was already aborted. Now patch creation and attaching the thread happen in a single transaction that is rolled back as a whole when attaching the thread fails.