Hardening build outputs (#12376)

- ensure outputs cannot be double completed

src/backend/InvenTree/build/models.py

@@ -1041,6 +1041,15 @@ class Build(
         if not output:
             raise ValidationError(_('No build output specified'))
 
+        # Re-check the state of the output itself:
+        # It may have changed since the scrap request was validated
+        # (e.g. a duplicated background task, or a concurrent request)
+        if not output.is_building:
+            raise ValidationError(_('Build output has already been completed'))
+
+        if output.build != self:
+            raise ValidationError(_('Build output does not match Build Order'))
+
         # If quantity is not specified, assume the entire output quantity
         if quantity is None:
             quantity = output.quantity
@@ -1111,6 +1120,15 @@ class Build(
         Raises:
             ValidationError: If the build output cannot be completed, with an appropriate message
         """
+        # Re-check the state of the output itself:
+        # It may have changed since the completion request was validated
+        # (e.g. a duplicated background task, or a concurrent request)
+        if not output.is_building:
+            raise ValidationError(_('Build output has already been completed'))
+
+        if output.build != self:
+            raise ValidationError(_('Build output does not match Build Order'))
+
         prevent_incomplete = get_global_setting(
             'PREVENT_BUILD_COMPLETION_HAVING_INCOMPLETED_TESTS'
         )

src/backend/InvenTree/build/tasks.py

@@ -115,9 +115,21 @@ def delete_build_outputs(build_id: int, output_ids: list, **kwargs):
 
     with transaction.atomic():
         for output_id in output_ids:
-            output = StockItem.objects.filter(pk=output_id).first()
-            if output:
-                build.delete_output(output)
+            # Lock the output row, and re-check that it is still "in production" -
+            # it may have been processed already (e.g. by a duplicated task)
+            output = StockItem.objects.select_for_update().filter(pk=output_id).first()
+
+            if not output:
+                continue
+
+            if not output.is_building:
+                logger.warning(
+                    'Build output <%s> is no longer in production - skipping deletion',
+                    output.pk,
+                )
+                continue
+
+            build.delete_output(output)
 
 
 @tracer.start_as_current_span('scrap_build_outputs')
@@ -149,16 +161,33 @@ def scrap_build_outputs(
 
     with transaction.atomic():
         for item in outputs:
-            output = StockItem.objects.filter(pk=item['output_id']).first()
-            if output:
-                build.scrap_build_output(
-                    output,
-                    item.get('quantity'),
-                    location,
-                    user=user,
-                    notes=notes,
-                    discard_allocations=discard_allocations,
+            # Lock the output row, and re-check that it is still "in production" -
+            # it may have been processed already (e.g. by a duplicated task)
+            output = (
+                StockItem.objects
+                .select_for_update()
+                .filter(pk=item['output_id'])
+                .first()
+            )
+
+            if not output:
+                continue
+
+            if not output.is_building:
+                logger.warning(
+                    'Build output <%s> is no longer in production - skipping scrap',
+                    output.pk,
                 )
+                continue
+
+            build.scrap_build_output(
+                output,
+                item.get('quantity'),
+                location,
+                user=user,
+                notes=notes,
+                discard_allocations=discard_allocations,
+            )
 
 
 @tracer.start_as_current_span('complete_build_outputs')
@@ -194,17 +223,34 @@ def complete_build_outputs(
 
     with transaction.atomic():
         for item in outputs:
-            output = StockItem.objects.filter(pk=item['output_id']).first()
-            if output:
-                build.complete_build_output(
-                    output,
-                    user,
-                    quantity=item.get('quantity'),
-                    location=location,
-                    status=status,
-                    notes=notes,
-                    required_tests=required_tests,
+            # Lock the output row, and re-check that it is still "in production" -
+            # it may have been processed already (e.g. by a duplicated task)
+            output = (
+                StockItem.objects
+                .select_for_update()
+                .filter(pk=item['output_id'])
+                .first()
+            )
+
+            if not output:
+                continue
+
+            if not output.is_building:
+                logger.warning(
+                    'Build output <%s> is no longer in production - skipping completion',
+                    output.pk,
                 )
+                continue
+
+            build.complete_build_output(
+                output,
+                user,
+                quantity=item.get('quantity'),
+                location=location,
+                status=status,
+                notes=notes,
+                required_tests=required_tests,
+            )
 
 
 @tracer.start_as_current_span('cancel_build')

src/backend/InvenTree/build/test_build.py

@@ -26,7 +26,12 @@ from InvenTree.unit_test import (
 )
 from order.models import PurchaseOrder, PurchaseOrderLineItem
 from part.models import BomItem, BomItemSubstitute, Part, PartTestTemplate
-from stock.models import StockItem, StockItemTestResult, StockLocation
+from stock.models import (
+    StockItem,
+    StockItemTestResult,
+    StockItemTracking,
+    StockLocation,
+)
 from stock.status_codes import StockStatus
 from users.models import Owner
 
@@ -1262,6 +1267,72 @@ class BuildTaskTests(BuildTestBase):
         self.build.complete_build_output(self.output_1, None)
         self.build.complete_build_output(self.output_2, None)
 
+    # -----------------------------------------------------------------------
+    # complete_build_outputs / scrap_build_outputs tasks
+    # -----------------------------------------------------------------------
+
+    def test_complete_outputs_task_is_idempotent(self):
+        """Duplicate execution of the output completion task must not double-count.
+
+        Regression test: the task never re-checked 'is_building', so a duplicated
+        (or redelivered) task run completed the same outputs twice - inflating the
+        'completed' count for the build order and duplicating stock history.
+        """
+        self.build.issue_build()
+
+        outputs = [{'output_id': self.output_1.pk}, {'output_id': self.output_2.pk}]
+
+        build.tasks.complete_build_outputs(
+            self.build.pk,
+            outputs,
+            self.location.pk,
+            StockStatus.OK.value,
+            user_id=self.user.pk,
+        )
+
+        self.build.refresh_from_db()
+        self.output_1.refresh_from_db()
+
+        self.assertEqual(self.build.completed, 10)
+        self.assertFalse(self.output_1.is_building)
+
+        n_tracking = StockItemTracking.objects.count()
+
+        # Run the task again (simulating a duplicated / redelivered task)
+        build.tasks.complete_build_outputs(
+            self.build.pk,
+            outputs,
+            self.location.pk,
+            StockStatus.OK.value,
+            user_id=self.user.pk,
+        )
+
+        # The 'completed' count has not been double-counted,
+        # and no additional stock history has been generated
+        self.build.refresh_from_db()
+        self.assertEqual(self.build.completed, 10)
+        self.assertEqual(StockItemTracking.objects.count(), n_tracking)
+
+    def test_complete_output_twice_rejected(self):
+        """Completing or scrapping an already-completed output must be rejected.
+
+        Regression test: neither complete_build_output() nor scrap_build_output()
+        re-checked the 'is_building' state of the output.
+        """
+        self.build.issue_build()
+
+        self.build.complete_build_output(self.output_1, None)
+
+        with self.assertRaises(ValidationError):
+            self.build.complete_build_output(self.output_1, None)
+
+        with self.assertRaises(ValidationError):
+            self.build.scrap_build_output(self.output_1, None, self.location)
+
+        # An output belonging to a *different* build order is also rejected
+        with self.assertRaises(ValidationError):
+            self.build.complete_build_output(self.stockitem_wo_required_test, None)
+
     # -----------------------------------------------------------------------
     # cancel_build task
     # -----------------------------------------------------------------------