Merge pull request #33875 from dimagi/gh/user/caching-bug

Clear user cache after successful save

corehq/apps/users/models.py

@@ -1478,7 +1478,6 @@ def save(self, fire_signals=True, update_django_user=True, fail_hard=False, **pa
         # When updating this method, please also ensure that your updates also
         # carry over to bulk_auto_deactivate_commcare_users.
         self.last_modified = datetime.utcnow()
-        self.clear_quickcache_for_user()
         with CriticalSection(['username-check-%s' % self.username], fail_hard=fail_hard, timeout=120):
             # test no username conflict
             by_username = self.get_db().view('users/by_username', key=self.username, reduce=False).first()
@@ -1491,7 +1490,11 @@ def save(self, fire_signals=True, update_django_user=True, fail_hard=False, **pa
 
             if not self.to_be_deleted():
                 self._save_user_data()
-            super(CouchUser, self).save(**params)
+            try:
+                super(CouchUser, self).save(**params)
+            finally:
+                # ensure the cache is cleared even if something goes wrong while saving the user to couch
+                self.clear_quickcache_for_user()
 
         if fire_signals:
             self.fire_signals()

corehq/apps/users/tests/test_models.py

@@ -3,6 +3,8 @@
 
 from django.test import SimpleTestCase, TestCase
 
+from couchdbkit.schema.base import DocumentBase
+
 from corehq.apps.users.models import (
     CommCareUser,
     CouchUser,
@@ -13,6 +15,7 @@
 )
 
 from corehq.apps.domain.models import Domain
+from corehq.apps.domain.shortcuts import create_domain
 
 
 class CouchUserTest(SimpleTestCase):
@@ -312,3 +315,39 @@ def test_diff_includes_missing_permissions_from_right(self):
         left = HqPermissions(view_reports=True)
         right = HqPermissions()
         self.assertEqual(HqPermissions.diff(left, right), ['view_reports'])
+
+
+class CouchUserSaveRaceConditionTests(TestCase):
+
+    def test_couch_user_save_race_condition(self):
+        """
+        WebUser and CommCareUser use the same underlying save method that is being tested here
+        """
+        username = 'race-test-user@test.com'
+        user = WebUser.create(self.domain.name, username, '***', None, None)
+        self.addCleanup(user.delete, None, deleted_by=None)
+
+        rev_before = WebUser.get_by_username(username)._rev
+        super_save = DocumentBase.save
+
+        def race_save(self, *args, **kw):
+            """
+            Simulate a scenario where another process calls get_by_username while the current process is executing
+            user.save(). The call happens after user.save() is called, but prior to the user object actually being
+            saved to Couch (prior to super().save() being called)
+            """
+            WebUser.get_by_username(username)
+            return super_save(self, *args, **kw)
+
+        with patch.object(DocumentBase, "save", race_save):
+            user.save()
+
+        rev_after = WebUser.get_by_username(username)._rev
+        diff = int(rev_after.split('-')[0]) - int(rev_before.split('-')[0])
+        self.assertEqual(diff, 1)
+
+    @classmethod
+    def setUpClass(cls):
+        super().setUpClass()
+        cls.domain = create_domain('race-user-test')
+        cls.addClassCleanup(cls.domain.delete)