Using savepoints within transaction.atomic() can result in the entire transaction being incorrectly and silently rolled back

[Chris Lamb]

Using savepoints within transaction.atomic() can result in the entire transaction being incorrectly and silently rolled back. Here is a slightly contrived example:

from django.db import transaction, DatabaseError

with transaction.atomic():
    user = User.objects.all()[0]
    user.last_login = datetime.datetime.utcnow() # any change to demonstrate problem
    user.save()

    sid = transaction.savepoint()
    try:
        # Will always fail
        User.objects.create(pk=user.pk)
        transaction.savepoint_commit(sid)
    except DatabaseError:
        transaction.savepoint_rollback(sid)

# outside of atomic() context - user.last_login change has not been committed!

What happens is the .create() call fails and marks the outermost atomic block as requiring a rollback, so even though we call savepoint_rollback (which does exactly right thing), once we leave the outer transaction.atomic(), the entire transaction is rolled back. In the example above, this means that the change to last_login is not persisted in the database. Rather insiduously, it is done entirely silently. :)

(The outer transaction.atomic() seems a little contrived but note that it is equivalent to the wrapper added by ATOMIC_REQUESTS.)

I'm not entirely sure what the solution is; the transaction.atomic(savepoint=False) call within ModelBase.save_base simply does not (and cannot) know that some other code will manually handle the savepoint rollback and thus no choice but to mark the entire transaction as borked. The only way it could know is if .create and .save took yet another kwarg, but this seems bizarre and would not be at all obvious.

Maybe manual savepoints should be become a private API after all (contradicting the current note in the documentation). Alternatively, it could be clarified that "manual" savepoint management they should not be used in conjunction with atomic() blocks (and thus ATOMIC_REQUESTS). The behaviour above is certainly not obvious at all.

(Hm, update_or_create uses savepoints manually, I hope that isn't breaking installations?)

[Anssi Kääriäinen]

Ill mark this as release blocker, seems like some solution is going to be needed... I haven't tested anything, but I feel confident enough in the report to just mark this accepted, too.

I wonder if savepoint_commit() and savepoint_rollback() could just mark the transaction block as "correct" again. Wouldn't this solve the problem?

[Aymeric Augustin]

One solution is to remove the savepoint=False option. But it was included for two good reasons:

• it improves performance,
• it avoids breaking all assertNumQueries with extra SAVEPOINT / RELEASE SAVEPOINT queries.

Another solution is to discourage using the low level API. Technically, the inner block in the example above is strictly equivalent to:

with transaction.atomic():
    User.objects.create(pk=user.pk)

I'm in favor of the second solution because I still have to see a use case that isn't covered by transaction.atomic(). There aren't many patterns for using savepoints in an application.

[Anssi Kääriäinen]

This is the idea I was considering: ​https://github.com/akaariai/django/compare/manual_savepoint_atomic - seems to work OK.

[Chris Lamb]

Replying to aaugustin:

Technically, the inner block in the example above is strictly equivalent to:

with transaction.atomic():
    User.objects.create(pk=user.pk)

Yes, although one still needs to catch the DatabaseError to be literally identical. :)

I'm in favor of the second solution because I still have to see a use case that isn't covered by transaction.atomic(). There aren't many patterns for using savepoints in an application.

Just for clarity: I assume here you mean there aren't many patterns for using savepoints *that are not also covered by using transaction.atomic*.

I wonder if savepoint_commit() and savepoint_rollback() could just mark the transaction block as "correct" again. Wouldn't this solve the problem?

No. That's too blunt as there is no way to know we are rolling back to a savepoint that results in a clean block. I'm not explaining that very well, so here's an untested example:

with transaction.atomic():
    s1 = transaction.savepoint()
    try:
        User.objects.create(pk=user.pk)
    except DatabaseError:
        # block now correctly marked as requiring rollback

        s2 = transaction.savepoint()
        # [..]
        transaction.savepoint_rollback(s2)

        # Oops. Assuming your solution, the block would now be incorrectly
        # marked as *not* requiring rollback even though it is required
        # due to the User.objects.create failure. We would only be able to
        # mark the block as clean if we rolled back to s1, but we have no
        # way of knowing that.

[Chris Lamb]

[Reverting to "bug"; seemed like an accidental change considering the severity is "release blocker".]

[Anssi Kääriäinen]

I see. But isn't this a problem in current Django code, too:

with transaction.atomic():
    try:
        User.objects.create(pk=user.pk) # exception marks connection needing rollback
    except Exception:
        with transaction.atomic():
             # do something that raises exception
             # exception marks needs_rollback = False
    # Whoops, the outer block is marked clean!

[Anssi Kääriäinen]

I was wrong in the above example, the second transaction.atomic() doesn't create any savepoints as the whole transaction needs to be rolled back anyways. Thus the needs_rollback flag isn't cleared.

I updated the ​https://github.com/akaariai/django/compare/manual_savepoint_atomic patch. Now you can't create savepoints in failed blocks, so s2 in the example of comment:4 would fail. This seems good enough, there really isn't any point in creating a savepoint which is going to be rolled back anyways.

As for do we need the ability to use manual savepoints? If possible, yes. Some things become nasty to code if your only option is using exceptions. Say, you call a method, and if that method returns false you will need to rollback the current savepoint. Options:

def myf():
    sp = savepoint()
    if somemethod():
        savepoint_commit()
    else:
        savepoint_rollback()

def myf():
    try:
        with atomic():
            if somemethod():
                return
            else:
                raise FakeException  # This will now rollback the savepoint.
    except FakeException:
        return

Another situation where atomic() isn't easy is if you have different paths for enter and exit. Then naturally any with statement can't be used. An example is TransactionMiddleware.

[Aymeric Augustin]

Anssi, I read your code, I read your arguments, and I understand what you're trying to achieve, but I don't think that's the right way.

My work on transactions sets up a strict top-down control flow. The high-level API (atomic) drives the low-level APIs (commit/rollback, savepoint_commit/rollback). The low-level API never hooks back up inside the high-level API. (Well, right now commit/rollback still reset the dirty flag, but that's deprecated and going away.) Your proposal introduces a reversed control flow, making it harder to reason about the behavior, and I'm not comfortable with that.

My instinct initially told me not to include the savepoint=False option. I eventually added it because you convinced me it was important and I couldn't find any way to break it. However, this ticket shows that I wasn't creative enough. Every additional complexity creates a risk of unforeseen interactions, and I'm becoming paranoid, because any transaction-related bug is a data loss bug.

---

In addition to the two proposals I made in comment 2, here's a third one: add an advanced API to mark the innermost atomic block as needing or not needing rollback.

This solution /probably/ works as expected, since the only purpose of needs_rollback is to declare that an inner atomic block without a savepoint exited with an exception and the innermost atomic block that has a savepoint should exit with a rollback. By resetting needs_rollback, you disable this behavior, which is the root cause of the bug described here. As a matter of fact, that's already what I'm doing in TestCase._fixture_teardown, except I'm forcing a rollback instead of preventing one.

So we have two solutions: document needs_rollback as a public API, or introduce a getter and a setter around needs_rollback. I'm in favor of the second solution since it allows the API to live in django.db.transaction. Proof of concept patch coming shortly.

[Aymeric Augustin]

I've written the patch and I'm now 100% sure it's the way to go. I'm committing it.

[Aymeric Augustin <aymeric.augustin@…>]

In c1284c3d3c6131a9d0ded9601ae0feb9a2e81a65:

Fixed #20571 -- Added an API to control connection.needs_rollback.

This is useful:

• to force a rollback on the exit of an atomic block without having to raise and catch an exception;
• to prevent a rollback after handling an exception manually.

[Anssi Kääriäinen]

I think Django should prevent running *anything else* than rollback or savepoint rollback once inside a transaction that is marked as needs_rollback. This is what PostgreSQL does, and with good reason - letting users continue a transaction that will be rolled back anyways will lead to errors. Addition of needs_rollback API lets users continue after errors when needed, but do so only explicitly.

BTW I think I proposed adding @in_transaction decorator, not atomic(savepoint=False). This would have been no-op if a transaction was going on, otherwise it would have created a transaction. This is subtly different from @atomic(savepoint=False) which marks the outer block for needs_rollback on errors. @in_transaction is what is needed by model.save() for example (and yes, using savepoints unconditionally in that case is too expensive).

In any case, this ticket is already solved so time to move on.