fix concurrent update issues (#12380)
- Ensure database values are used for delta updates - Regression tests
This commit is contained in:
parent
dd05611ea4
commit
431a6e49a0
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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):
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
Loading…
Reference in New Issue