From dd05611ea4c436726e5c1e8b825b446f20c04297 Mon Sep 17 00:00:00 2001 From: Oliver Date: Mon, 13 Jul 2026 18:53:07 +1000 Subject: [PATCH] Hardening build outputs (#12376) - ensure outputs cannot be double completed --- src/backend/InvenTree/build/models.py | 18 +++++ src/backend/InvenTree/build/tasks.py | 90 +++++++++++++++++------ src/backend/InvenTree/build/test_build.py | 73 +++++++++++++++++- 3 files changed, 158 insertions(+), 23 deletions(-) diff --git a/src/backend/InvenTree/build/models.py b/src/backend/InvenTree/build/models.py index 1547d35c50..3ae7969e73 100644 --- a/src/backend/InvenTree/build/models.py +++ b/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' ) diff --git a/src/backend/InvenTree/build/tasks.py b/src/backend/InvenTree/build/tasks.py index 0dcac04bb8..13355ac043 100644 --- a/src/backend/InvenTree/build/tasks.py +++ b/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') diff --git a/src/backend/InvenTree/build/test_build.py b/src/backend/InvenTree/build/test_build.py index b1cbfd3d09..329808f367 100644 --- a/src/backend/InvenTree/build/test_build.py +++ b/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 # -----------------------------------------------------------------------