From 431a6e49a0325c1fce214a825c83f44c8d80672a Mon Sep 17 00:00:00 2001 From: Oliver Date: Mon, 13 Jul 2026 18:53:18 +1000 Subject: [PATCH] fix concurrent update issues (#12380) - Ensure database values are used for delta updates - Regression tests --- src/backend/InvenTree/build/models.py | 15 +++-- src/backend/InvenTree/build/serializers.py | 6 +- src/backend/InvenTree/build/test_build.py | 64 +++++++++++++++++++ src/backend/InvenTree/order/api.py | 29 ++++++--- src/backend/InvenTree/order/models.py | 33 +++++++--- .../InvenTree/order/test_sales_order.py | 23 +++++++ 6 files changed, 144 insertions(+), 26 deletions(-) diff --git a/src/backend/InvenTree/build/models.py b/src/backend/InvenTree/build/models.py index 3ae7969e73..4e7a3efe7b 100644 --- a/src/backend/InvenTree/build/models.py +++ b/src/backend/InvenTree/build/models.py @@ -1237,9 +1237,11 @@ class Build( trigger_event(BuildEvents.OUTPUT_COMPLETED, id=output.pk, build_id=self.pk) # Increase the completed quantity for this build - self.completed += output.quantity - - self.save() + # Increment at the database level to prevent lost updates + # (multiple outputs may be completed concurrently) + self.completed = F('completed') + output.quantity + self.save(update_fields=['completed']) + self.refresh_from_db(fields=['completed']) @transaction.atomic def auto_allocate_stock( @@ -2074,8 +2076,11 @@ class BuildItem(InvenTree.models.InvenTreeMetadataModel): ) # Increase the "consumed" count for the associated BuildLine - self.build_line.consumed += quantity - self.build_line.save() + # Increment at the database level to prevent lost updates + # (multiple allocations against the same BuildLine may complete concurrently) + self.build_line.consumed = F('consumed') + quantity + self.build_line.save(update_fields=['consumed']) + self.build_line.refresh_from_db(fields=['consumed']) # Decrease the allocated quantity self.quantity = max(0, self.quantity - quantity) diff --git a/src/backend/InvenTree/build/serializers.py b/src/backend/InvenTree/build/serializers.py index d542671e00..004a47bef5 100644 --- a/src/backend/InvenTree/build/serializers.py +++ b/src/backend/InvenTree/build/serializers.py @@ -1006,7 +1006,11 @@ class BuildAllocationSerializer(serializers.Serializer): } try: - if build_item := BuildItem.objects.filter(**params).first(): + # Lock the row, so concurrent allocations cannot both read + # the same starting quantity (lost update) + if build_item := ( + BuildItem.objects.select_for_update().filter(**params).first() + ): # Find an existing BuildItem for this stock item # If it exists, increase the quantity build_item.quantity += quantity diff --git a/src/backend/InvenTree/build/test_build.py b/src/backend/InvenTree/build/test_build.py index 329808f367..79813954c6 100644 --- a/src/backend/InvenTree/build/test_build.py +++ b/src/backend/InvenTree/build/test_build.py @@ -647,6 +647,70 @@ class BuildTest(BuildTestBase): for output in outputs: self.assertFalse(output.is_building) + def test_complete_output_stale_build_instance(self): + """The 'completed' count is incremented atomically at the database level. + + Simulates two concurrent processes completing different outputs of the + same build, each holding its own (stale) copy of the Build instance. + """ + self.stock_1_1.quantity = 1000 + self.stock_1_1.save() + + self.stock_2_1.quantity = 30 + self.stock_2_1.save() + + self.build.issue_build() + + # Allocate non-tracked parts + self.allocate_stock( + None, + { + self.stock_1_1: self.stock_1_1.quantity, + self.stock_1_2: 10, + self.stock_2_1: 30, + }, + ) + + # Allocate tracked parts against each output + self.allocate_stock(self.output_1, {self.stock_3_1: 6}) + self.allocate_stock(self.output_2, {self.stock_3_1: 14}) + + # Two independent in-memory copies of the same build + build_a = Build.objects.get(pk=self.build.pk) + build_b = Build.objects.get(pk=self.build.pk) + + build_a.complete_build_output(self.output_1, None) + build_b.complete_build_output(self.output_2, None) + + # Both completions must be counted + self.build.refresh_from_db() + self.assertEqual(self.build.completed, 10) + + def test_complete_allocation_stale_build_line(self): + """The 'consumed' count is incremented atomically at the database level. + + Simulates two concurrent workers completing different allocations + against the same BuildLine, each holding its own (stale) copy of the line. + """ + self.build.issue_build() + + self.allocate_stock(None, {self.stock_1_1: 3, self.stock_1_2: 5}) + + alloc_a, alloc_b = BuildItem.objects.filter(build_line=self.line_1).order_by( + 'pk' + ) + + # Cache a separate copy of the BuildLine on each allocation + self.assertEqual(alloc_a.build_line.consumed, 0) + self.assertEqual(alloc_b.build_line.consumed, 0) + + alloc_a.complete_allocation(user=self.user) + alloc_b.complete_allocation(user=self.user) + + # Both consumed quantities must be counted + self.line_1.refresh_from_db() + self.assertEqual(self.line_1.consumed, 8) + def test_complete_with_required_tests(self): """Test the prevention completion when a required test is missing feature.""" # with required tests incompleted the save should fail diff --git a/src/backend/InvenTree/order/api.py b/src/backend/InvenTree/order/api.py index 40882dd38d..ab7c65d07e 100644 --- a/src/backend/InvenTree/order/api.py +++ b/src/backend/InvenTree/order/api.py @@ -6,6 +6,7 @@ from typing import cast from django.conf import settings from django.contrib.auth import authenticate, login from django.contrib.auth.models import User +from django.db import transaction from django.db.models import F, Q from django.http.response import JsonResponse from django.urls import include, path, re_path @@ -684,18 +685,26 @@ class PurchaseOrderLineItemList( # possibly merge duplicate items line_item = None if data.get('merge_items', True): - other_line = models.PurchaseOrderLineItem.objects.filter( - part=data.get('part'), - order=data.get('order'), - target_date=data.get('target_date'), - destination=data.get('destination'), - ).first() + with transaction.atomic(): + # Lock the matching row, so concurrent line creations cannot + # both read the same starting quantity (lost update) + other_line = ( + models.PurchaseOrderLineItem.objects + .select_for_update() + .filter( + part=data.get('part'), + order=data.get('order'), + target_date=data.get('target_date'), + destination=data.get('destination'), + ) + .first() + ) - if other_line is not None: - other_line.quantity += Decimal(data.get('quantity', 0)) - other_line.save() + if other_line is not None: + other_line.quantity += Decimal(data.get('quantity', 0)) + other_line.save() - line_item = other_line + line_item = other_line # otherwise create a new line item if line_item is None: diff --git a/src/backend/InvenTree/order/models.py b/src/backend/InvenTree/order/models.py index 50a0d235b9..4c609b6b33 100644 --- a/src/backend/InvenTree/order/models.py +++ b/src/backend/InvenTree/order/models.py @@ -805,7 +805,9 @@ class PurchaseOrder(TotalPriceMixin, Order): if group: # Check if there is already a matching line item (for this PurchaseOrder) - matches = self.lines.filter(part=supplier_part) + # Lock the matching row, so concurrent additions cannot both read + # the same starting quantity (lost update) + matches = self.lines.select_for_update().filter(part=supplier_part) if matches.count() > 0: line = matches.first() @@ -1064,9 +1066,14 @@ class PurchaseOrder(TotalPriceMixin, Order): # Cache the custom status options for the StockItem model custom_stock_status_values = stock.models.StockItem.STATUS_CLASS.custom_values() - line_items = PurchaseOrderLineItem.objects.filter( - pk__in=line_items_ids - ).prefetch_related('part', 'part__part', 'order') + # Lock the line item rows, so that concurrent receipts against the same + # lines cannot both read the same 'received' value (lost update) + line_items = ( + PurchaseOrderLineItem.objects + .select_for_update() + .filter(pk__in=line_items_ids) + .prefetch_related('part', 'part__part', 'order') + ) # Map order line items to their corresponding stock items line_item_map = {line.pk: line for line in line_items} @@ -1197,8 +1204,10 @@ class PurchaseOrder(TotalPriceMixin, Order): stock_data['is_building'] = False # Increase the 'completed' quantity for the build order - build_order.completed += stock_quantity - build_order.save() + # Increment at the database level to prevent lost updates + build_order.completed = F('completed') + stock_quantity + build_order.save(update_fields=['completed']) + build_order.refresh_from_db(fields=['completed']) elif build_order.status == BuildStatus.CANCELLED: # A 'cancelled' build order is ignored pass @@ -2896,8 +2905,10 @@ class SalesOrderAllocation(models.Model): ) # Update the 'shipped' quantity - self.line.shipped += self.quantity - self.line.save() + # Increment at the database level to prevent lost updates + self.line.shipped = F('shipped') + self.quantity + self.line.save(update_fields=['shipped']) + self.line.refresh_from_db(fields=['shipped']) # Update our own reference to the StockItem # (It may have changed if the stock was split) @@ -4006,8 +4017,10 @@ class TransferOrderAllocation(models.Model): # Update the transferred qty # Note: use the quantity which was *actually* transferred - self.line.transferred += transfer_quantity - self.line.save() + # Increment at the database level to prevent lost updates + self.line.transferred = F('transferred') + transfer_quantity + self.line.save(update_fields=['transferred']) + self.line.refresh_from_db(fields=['transferred']) def _touch_order_updated_at(instance): diff --git a/src/backend/InvenTree/order/test_sales_order.py b/src/backend/InvenTree/order/test_sales_order.py index 7837c58098..c293eb57be 100644 --- a/src/backend/InvenTree/order/test_sales_order.py +++ b/src/backend/InvenTree/order/test_sales_order.py @@ -220,6 +220,29 @@ class SalesOrderTest(InvenTreeTestCase): self.assertTrue(self.line.is_fully_allocated()) self.assertEqual(self.line.allocated_quantity(), 50) + def test_complete_allocation_stale_line_instance(self): + """The 'shipped' count is incremented atomically at the database level. + + Simulates two concurrent workers completing different allocations + against the same order line, each holding its own (stale) copy of the line. + """ + self.allocate_stock(True) + + alloc_a, alloc_b = SalesOrderAllocation.objects.filter(line=self.line).order_by( + 'pk' + ) + + # Cache a separate copy of the line on each allocation + self.assertEqual(alloc_a.line.shipped, 0) + self.assertEqual(alloc_b.line.shipped, 0) + + alloc_a.complete_allocation(None) + alloc_b.complete_allocation(None) + + # Both shipped quantities must be counted + self.line.refresh_from_db() + self.assertEqual(self.line.shipped, 50) + def test_allocate_variant(self): """Allocate a variant of the designated item.""" SalesOrderAllocation.objects.create(