[Sqlalchemy-tickets] Issue #4151: Rollback of nested and subtransactions incomplete, causing DetachedInstanceError (zzzeek/sqlalchemy)

[Torsten L. <iss...@bi...>]

New issue 4151: Rollback of nested and subtransactions incomplete, causing DetachedInstanceError
https://bitbucket.org/zzzeek/sqlalchemy/issues/4151/rollback-of-nested-and-subtransactions

Torsten Landschoff:

We hit a curious bug (so it seems to me at least) in SQLAlchemy which causes some erratic problems in our application. Basically, a rollback after a completed subtransaction (same for nested transactions) seems incomplete.

I had a hard time to reproduce this in a small example but funny enough in our application it depends on the garbage collector. Guess there is a cycle somewhere but I was unable to find it.

The core of the problem is exposed by this code:

```
#!python
with session.begin(subtransactions=True):
    new_videos = Directory(name="New Videos")
    new_videos.entries = videos.entries
    documents.entries[0] = new_videos
    session.flush()  # this is required to trigger the problem

    video = File(name="falcon_landing.mp4", content="Space Ship Landing")
    new_videos.entries[0] = video
    # new_videos.entries.append(video)  # instead of replacing works. wtf?

print("Pre rollback session content: {0}".format(list(session)))
session.rollback()
```

The example uses an association proxy to represent the entries of a directory (like in a filesystem, but like in git a folder can be referenced by multiple parent folders).

By assigning `new_videos.entries = videos.entries`, these association objects are copied. The code then replaces the first entry via `new_videos.entries[0] = video`. This causes an update to the underlying association object.

After the rollback, the association object should get expunged, because the cause for its creation was undone, but in fact it stays alive in the session. So far nothing bad happens, but the next action using the session can trigger an attribute refresh which fails, interestingly, because the DirectoryEntry instance is allegedly not bound to the session:

```
#!python
$ python3 incomplete_rollback.py
Initial session content: [<Directory: Documents (1)>, <Directory: Videos (2)>]
Pre rollback session content: [<Directory: Videos (2)>, <Directory: New Videos (5)>, <DirectoryEntry @ 7fd19e5b3eb8>, <File: falcon_landing.mp4 (6)>, <DirectoryEntry @ 7fd19e5b37b8>, <Directory: Documents (1)>, <File: dancing.mp4 (3)>, <DirectoryEntry @ 7fd19e5b3668>, <DirectoryEntry @ 7fd19e5b3b00>]
After rollback session content: [<Directory: Videos (2)>, <Directory: Documents (1)>, <DirectoryEntry @ 7fd19e5b37b8>]
Traceback (most recent call last):
  File "incomplete_rollback.py", line 100, in <module>
    session.commit()
[...]
sqlalchemy.orm.exc.DetachedInstanceError: Instance <DirectoryEntry at 0x7fd19e5b37b8> is not bound to a Session; attribute refresh operation cannot proceed (Background on this error at: http://sqlalche.me/e/bhk3)
```

I tested this with pysqlite2 (in python 2.7) as well as with sqlite3 (from stock python3.5), with SQLAlchemy 1.1.13 as well as 1.2.0. Older versions behave differently in that they report a primary key conflict.

```
#!python
$ pip install sqlalchemy==1.0.19
[...]
Installing collected packages: sqlalchemy
  Found existing installation: SQLAlchemy 1.2.0
    Uninstalling SQLAlchemy-1.2.0:
      Successfully uninstalled SQLAlchemy-1.2.0
Successfully installed sqlalchemy-1.0.19
$ python incomplete_rollback.py
Initial session content: [<Directory: Documents (1)>, <Directory: Videos (2)>]
Pre rollback session content: [<File: falcon_landing.mp4 (6)>, <DirectoryEntry @ 7f0643176d90>, <Directory: Documents (1)>, <Directory: New Videos (5)>, <File: dancing.mp4 (3)>, <DirectoryEntry @ 7f06431bab50>, <DirectoryEntry @ 7f06431891d0>, <DirectoryEntry @ 7f0643189150>, <Directory: Videos (2)>]
After rollback session content: [<Directory: Documents (1)>, <DirectoryEntry @ 7f06431bab50>, <Directory: Videos (2)>]
Traceback (most recent call last):
  File "incomplete_rollback.py", line 100, in <module>
    session.commit()
  File ".../python2.7/site-packages/sqlalchemy/orm/session.py", line 896, in commit
    self.transaction.commit()
  [...]
  File ".../python2.7/site-packages/sqlalchemy/orm/persistence.py", line 305, in _organize_states_for_save
    state_str(existing)))
sqlalchemy.orm.exc.FlushError: New instance <DirectoryEntry at 0x7f0643189b10> with identity key (<class '__main__.DirectoryEntry'>, (5, 3)) conflicts with persistent instance <DirectoryEntry at 0x7f06431bab50>
```

The following patch seems to fix this problem, I did not check for adverse effects though because I have no idea what I am doing here ;-)

```
#!diff
diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py
index bd0bf91..a5ac519 100644
--- a/lib/sqlalchemy/orm/session.py
+++ b/lib/sqlalchemy/orm/session.py
@@ -346,7 +346,8 @@ class SessionTransaction(object):
         for s, (oldkey, newkey) in self._key_switches.items():
             self.session.identity_map.safe_discard(s)
             s.key = oldkey
-            self.session.identity_map.replace(s)
+            if s.key in self.session.identity_map:
+                self.session.identity_map.replace(s)

         for s in set(self._deleted).union(self.session._deleted):
             self.session._update_impl(s, revert_deletion=True)
```

Basically, it seems like SQLAlchemy noticed that the state was dropped (due to the rollback) and removed it from the
identity map. However, because the state also had changes, it was reinstated with the original key after first
correctly removing it.

I hope you can interpret my ramblings ;-)