Fix accounting race conditions and run the addclass concurrency test

esp/.test_durations

@@ -1,4 +1,8 @@
 {
+    "esp/accounting/tests/test_concurrency.py::AccountingConcurrencyTest::test_admission_charged_once": 1.36,
+    "esp/accounting/tests/test_concurrency.py::AccountingConcurrencyTest::test_double_submitted_preference_kept_once": 2.37,
+    "esp/accounting/tests/test_concurrency.py::AccountingConcurrencyTest::test_financial_aid_granted_once": 2.31,
+    "esp/accounting/tests/test_concurrency.py::AccountingConcurrencyTest::test_line_item_type_created_once": 1.76,
     "esp/accounting/tests/test_models.py::AccountTest::test_balance_empty": 0.05504703000042355,
     "esp/accounting/tests/test_models.py::AccountTest::test_balance_with_incoming_transfer": 0.030873531000452203,
     "esp/accounting/tests/test_models.py::AccountTest::test_balance_with_outgoing_transfer": 0.03027481900062412,
@@ -977,7 +981,7 @@
     "esp/program/modules/tests/test_accountingmodule.py::AccountingModuleTest::test_invalid_user_lookup": 0.05951309200099786,
     "esp/program/modules/tests/test_accountingmodule.py::AccountingModuleTest::test_user_lookup_via_post_data": 0.019189870999980485,
     "esp/program/modules/tests/test_accountingmodule.py::AccountingModuleTest::test_user_lookup_via_url_target_user": 0.017656103998888284,
-    "esp/program/modules/tests/test_addclass_concurrency.py::AddClassConcurrencyRegressionTest::test_concurrent_addclass_last_seat": 1.229655146999903,
+    "esp/program/modules/tests/test_addclass_concurrency.py::AddClassConcurrencyRegressionTest::test_concurrent_addclass_last_seat": 2.26,
     "esp/program/modules/tests/test_admin_search.py::AdminSearchEntriesTest::test_entries_have_required_fields": 1.0664856469993538,
     "esp/program/modules/tests/test_admin_search.py::AdminSearchEntriesTest::test_excluded_views_not_in_results": 0.9991949259992907,
     "esp/program/modules/tests/test_admin_search.py::AdminSearchEntriesTest::test_humanize_view_name": 0.7200085709991981,

esp/esp/accounting/controllers.py

@@ -38,6 +38,7 @@
 
 from esp.accounting.models import Transfer, Account, FinancialAidGrant, LineItemType, LineItemOptions
 from esp.accounting import ReconciliationError, DuplicatePaymentError
+from esp.db.locking import get_or_create_locked, lock_row
 from esp.program.models import FinancialAidRequest, Program, SplashInfo
 from esp.users.models import ESPUser
 from esp.utils.query_utils import nest_Q
@@ -127,7 +128,7 @@ def setup_accounts(self):
         #   For now, just create a single account for the program.  In the
         #   future we may want finer grained accounting per program.
         program = self.program
-        (account, created) = Account.objects.get_or_create(name=slugify(program.name), description='Main account', program_id=program.id)
+        (account, created) = get_or_create_locked(Account, program, name=slugify(program.name), description='Main account', program_id=program.id)
         return account
 
     def setup_lineitemtypes(self, base_cost, optional_items=None, select_items=None):
@@ -142,50 +143,51 @@ def setup_lineitemtypes(self, base_cost, optional_items=None, select_items=None)
         if select_items is None:
             select_items = ()
 
-        (lit_required, created) = LineItemType.objects.get_or_create(
+        (lit_required, created) = LineItemType.get_or_create_for_program(
+            program,
             text='Program admission',
             amount_dec=Decimal('%.2f' % base_cost),
             required=True,
             max_quantity=1,
-            program=program,
             for_payments=False,
             for_finaid=True
         )
         result.append(lit_required)
 
-        (lit_payments, created) = LineItemType.objects.get_or_create(
+        (lit_payments, created) = LineItemType.get_or_create_for_program(
+            program,
             text='Student payment',
-            program=program,
             for_payments=True
         )
         result.append(lit_payments)
 
-        (lit_finaid, created) = LineItemType.objects.get_or_create(
-            text='Financial aid grant',
-            program=program
+        (lit_finaid, created) = LineItemType.get_or_create_for_program(
+            program,
+            text='Financial aid grant'
         )
         result.append(lit_finaid)
 
         for item in optional_items:
-            (lit_optional, created) = LineItemType.objects.get_or_create(
+            (lit_optional, created) = LineItemType.get_or_create_for_program(
+                program,
                 text=item[0],
                 amount_dec=Decimal('%.2f' % item[1]),
                 required=False,
                 max_quantity=item[2],
-                program=program,
                 for_payments=False
             )
             result.append(lit_optional)
 
         for item in select_items:
-            (lit_select, created) = LineItemType.objects.get_or_create(
+            (lit_select, created) = LineItemType.get_or_create_for_program(
+                program,
                 text=item[0],
                 required=False,
                 max_quantity=1,
-                program=program,
             )
             for option in item[1]:
-                (lio, created) = LineItemOptions.objects.get_or_create(
+                (lio, created) = get_or_create_locked(
+                LineItemOptions, program,
                 lineitem_type=lit_select,
                 description=option[0],
                 amount_dec=option[1]
@@ -226,8 +228,8 @@ def default_admission_lineitemtype(self):
 
     def default_refund_lineitemtype(self):
         """Get or create a line item type for refunds."""
-        lineitem, created = LineItemType.objects.get_or_create(
-            program=self.program,
+        lineitem, created = LineItemType.get_or_create_for_program(
+            self.program,
             text='Student refund',
             defaults={'for_payments': False}
         )
@@ -237,7 +239,7 @@ def donation_lineitemtype(self):
         for module_name in ['CreditCardModule_Stripe', 'DonationModule']:
             other_module = self.program.getModule(module_name)
             if other_module and other_module.get_setting('offer_donation', default=True):
-                return LineItemType.objects.get_or_create(program=self.program, text=other_module.get_setting('donation_text'))[0]
+                return LineItemType.get_or_create_for_program(self.program, text=other_module.get_setting('donation_text'))[0]
         return None
 
     def get_lineitemtypes_Q(self, include_donations=True, required_only=False, optional_only=False, payment_only=False, lineitemtype_id=None):
@@ -372,8 +374,16 @@ def ensure_required_transfers(self):
         source_account = self.default_source_account()
         required_line_items = self.get_lineitemtypes(required_only=True)
 
-        existing_transfers_by_li = {t.line_item_id: t for t in Transfer.objects.filter(
-            user=self.user, line_item__in=required_line_items)}
+        def transfers_by_line_item():
+            return {t.line_item_id: t for t in Transfer.objects.filter(
+                user=self.user, line_item__in=required_line_items)}
+
+        existing_transfers_by_li = transfers_by_line_item()
+        if any(item.id not in existing_transfers_by_li for item in required_line_items):
+            # Some are missing. Lock this user and re-check, so that concurrent
+            # requests create each missing transfer only once.
+            lock_row(self.user)
+            existing_transfers_by_li = transfers_by_line_item()
 
         for item in required_line_items:
             transfer = existing_transfers_by_li.get(item.id)
@@ -409,6 +419,10 @@ def apply_preferences(self, optional_items):
             The last 2 items, cost and option ID, are non-required and should
             be set to None if unused.   """
 
+        #   Serialize with concurrent submissions by this user; otherwise each
+        #   one's delete misses the other's new transfers and both are kept.
+        lock_row(self.user)
+
         result = []
         line_items = self.get_lineitemtypes(include_donations=False).exclude(text__in=self.admission_items)
 
@@ -424,6 +438,8 @@ def apply_preferences(self, optional_items):
     @transaction.atomic
     def set_preference(self, lineitem_name, quantity, amount=None, option_id=None):
         #   Sets a single preference, after removing any exactly matching transfers.
+        #   The lock serializes concurrent calls for this user, as in apply_preferences().
+        lock_row(self.user)
         line_item = self.get_lineitemtypes().get(text=lineitem_name)
 
         kwargs = {'line_item': line_item}
@@ -567,6 +583,10 @@ def record_payment_from_identifier(identifier, amount_paid, transaction_id='',
 
     @transaction.atomic
     def set_finaid_params(self, dollar_amount, discount_percent):
+        #   Serialize with concurrent calls for this user so that only one
+        #   request and grant get created.
+        lock_row(self.user)
+
         #   Get the user's financial aid request or create one if it doesn't exist
         requests = FinancialAidRequest.objects.filter(user=self.user, program=self.program)
         if requests.exists():
@@ -575,7 +595,7 @@ def set_finaid_params(self, dollar_amount, discount_percent):
             request = FinancialAidRequest.objects.create(user=self.user, program=self.program, extra_explaination='Request automatically generated by accounting system')
 
         #   Create or modify a corresponding grant with the discount set to 100%
-        (grant, created) = FinancialAidGrant.objects.get_or_create(request=request)
+        (grant, created) = get_or_create_locked(FinancialAidGrant, self.user, request=request)
         if dollar_amount:
             grant.amount_max_dec = Decimal('%.2f' % dollar_amount)
         grant.percent = discount_percent

esp/esp/accounting/models.py

@@ -39,8 +39,9 @@
 from esp.users.models import ESPUser
 from esp.program.models import Program, FinancialAidRequest
 from esp.db.fields import AjaxForeignKey
+from esp.db.locking import get_or_create_locked, lock_row
 
-from django.db import models
+from django.db import models, transaction
 from django.db.models import Sum
 
 from decimal import Decimal
@@ -60,6 +61,16 @@ class LineItemType(models.Model):
         help_text='For line items with options, whether students may select one option (single) or multiple options (multiple).',
     )
 
+    @classmethod
+    def get_or_create_for_program(cls, program, defaults=None, **kwargs):
+        """
+        Like LineItemType.objects.get_or_create(program=program, **kwargs),
+        but safe when concurrent requests race to create the same row; see
+        get_or_create_locked(). Its newest-duplicate choice matches the
+        default_*_lineitemtype() lookups.
+        """
+        return get_or_create_locked(cls, program, defaults=defaults, program=program, **kwargs)
+
     @property
     def amount(self):
         if self.amount_dec is None:
@@ -172,15 +183,23 @@ def finalize(self):
             return
 
         from esp.accounting.controllers import IndividualAccountingController
-        iac = IndividualAccountingController(self.program, self.user)
-        source_account = iac.default_finaid_account()
-        dest_account = iac.default_source_account()
-        line_item_type = iac.default_finaid_lineitemtype()
-
-        (transfer, created) = Transfer.objects.get_or_create(source=source_account, destination=dest_account, user=self.user, line_item=line_item_type, amount_dec=iac.amount_finaid())
-        self.finalized = True
-        self.save()
-        return transfer
+        with transaction.atomic():
+            # Serialize concurrent finalize() calls for this user, then re-check
+            # the flag, which may have been set since this instance was loaded.
+            lock_row(self.user)
+            if self.pk and FinancialAidGrant.objects.filter(pk=self.pk, finalized=True).exists():
+                self.finalized = True
+                return
+
+            iac = IndividualAccountingController(self.program, self.user)
+            source_account = iac.default_finaid_account()
+            dest_account = iac.default_source_account()
+            line_item_type = iac.default_finaid_lineitemtype()
+
+            (transfer, created) = get_or_create_locked(Transfer, self.user, source=source_account, destination=dest_account, user=self.user, line_item=line_item_type, amount_dec=iac.amount_finaid())
+            self.finalized = True
+            self.save()
+            return transfer
 
     def __str__(self):
         if self.percent and self.amount_max_dec:

esp/esp/accounting/tests/test_concurrency.py

@@ -0,0 +1,116 @@
+"""
+Concurrency tests for lazily created accounting rows.
+
+Each test races two threads through the same code path, with save() slowed
+down to widen the window between the existence check and the insert, so that
+without serialization both threads would create a row. They use
+TransactionTestCase so each thread commits on its own database connection, as
+concurrent requests do in production.
+"""
+import threading
+import time
+import unittest
+from unittest import mock
+
+from django.db import close_old_connections, connection
+from django.test import TransactionTestCase
+
+from esp.accounting.controllers import (
+    GlobalAccountingController,
+    IndividualAccountingController,
+    ProgramAccountingController,
+)
+from esp.accounting.models import FinancialAidGrant, LineItemType, Transfer
+from esp.program.models import FinancialAidRequest, Program
+from esp.users.models import ESPUser
+
+
+def slowed_save(model):
+    """Patch model.save() to pause before writing."""
+    original_save = model.save
+
+    def slow_save(instance, *args, **kwargs):
+        time.sleep(0.5)
+        return original_save(instance, *args, **kwargs)
+
+    return mock.patch.object(model, 'save', slow_save)
+
+
+class AccountingConcurrencyTest(TransactionTestCase):
+
+    def setUp(self):
+        # SQLite ignores row locks; this regression targets backends used in production.
+        if not connection.features.has_select_for_update:
+            raise unittest.SkipTest(
+                'Database backend must support SELECT ... FOR UPDATE (not available on SQLite).'
+            )
+        super().setUp()
+        self.program = Program.objects.create(grade_min=7, grade_max=12)
+        GlobalAccountingController().setup_accounts()
+        pac = ProgramAccountingController(self.program)
+        pac.setup_accounts()
+        pac.setup_lineitemtypes(50.0, [('T-shirt', 15.0, 1)])
+        self.student = ESPUser.objects.create(username='student')
+
+    def race(self, target):
+        """Run target() in two threads started together; return any exceptions."""
+        barrier = threading.Barrier(2, timeout=60)
+        errors = []
+        lock = threading.Lock()
+
+        def attempt():
+            close_old_connections()
+            try:
+                barrier.wait()
+                target()
+            except Exception as e:
+                with lock:
+                    errors.append(e)
+            finally:
+                close_old_connections()
+
+        threads = [threading.Thread(target=attempt) for _ in range(2)]
+        for t in threads:
+            t.start()
+        for t in threads:
+            t.join()
+        return errors
+
+    def test_line_item_type_created_once(self):
+        with slowed_save(LineItemType):
+            errors = self.race(lambda: LineItemType.get_or_create_for_program(self.program, text='Donation'))
+        self.assertEqual(errors, [])
+        self.assertEqual(LineItemType.objects.filter(program=self.program, text='Donation').count(), 1)
+
+    def test_admission_charged_once(self):
+        def view_amount_due():
+            IndividualAccountingController(self.program, self.student).ensure_required_transfers()
+
+        with slowed_save(Transfer):
+            errors = self.race(view_amount_due)
+        self.assertEqual(errors, [])
+        self.assertEqual(Transfer.objects.filter(
+            user=self.student, line_item__text='Program admission').count(), 1)
+
+    def test_double_submitted_preference_kept_once(self):
+        def choose_tshirt():
+            IndividualAccountingController(self.program, self.student).set_preference('T-shirt', 1)
+
+        with slowed_save(Transfer):
+            errors = self.race(choose_tshirt)
+        self.assertEqual(errors, [])
+        self.assertEqual(Transfer.objects.filter(user=self.student, line_item__text='T-shirt').count(), 1)
+
+    def test_financial_aid_granted_once(self):
+        request = FinancialAidRequest.objects.create(user=self.student, program=self.program)
+        grant = FinancialAidGrant.objects.create(request=request, percent=100)
+
+        def finalize():
+            FinancialAidGrant.objects.get(pk=grant.pk).finalize()
+
+        with slowed_save(Transfer):
+            errors = self.race(finalize)
+        self.assertEqual(errors, [])
+        self.assertEqual(Transfer.objects.filter(
+            user=self.student, line_item__text='Financial aid grant').count(), 1)
+        self.assertTrue(FinancialAidGrant.objects.get(pk=grant.pk).finalized)