fix: ensure transactions rollback on failure#767
Conversation
| self._id = transaction_response.transaction | ||
|
|
||
| async def _rollback(self) -> None: | ||
| async def _rollback(self, source_exc=None) -> None: |
There was a problem hiding this comment.
It seems unfortunate that we have to add an additional parameter to this function. I am guessing the underscore prefix means it is private? If so, I guess it's not that big of a deal, but if this argument is only being used for that one case in the call function then could we catch errors there caused by await transaction._rollback(source_exc=exc) and do raise exc from source_exc there instead?
There was a problem hiding this comment.
I'm not worried about the new parameter because 1) this is a private function, and 2) it's an optional paramater, so existing behaviour would remain consistent
But that's a good point that after this refactor, the rollback only happens in one place. In this case, we can rely on built-in exception chaining instead of raise from, since the exception occurs while handling a different exception. I simplified the code to remove the source_exc
Previously, non-retryable exceptions raised during a transaction would be raised immediately, without causing a rollback. This PR adds an extra try...except block to make sure rollback is always called
It also uses exception chaining to resolve actionable error issues for the repo
Fixes: #766, #765