[bug] Fix broken build complete transition (#12696)

* Add regression test for build complete failure

* Handle offloading of complete_build

* Allow generic offloading of transitions to the background worker

* Refactor cancel_build method

* additional unit test

src/backend/InvenTree/build/models.py

@@ -42,6 +42,7 @@ from common.models import ProjectCode
 from common.settings import get_global_setting
 from generic.enums import StringEnum
 from generic.states import (
+    DEFERRABLE,
     Deprecations,
     StateTransitionMixin,
     StatusCodeMixin,
@@ -970,7 +971,7 @@ class Build(
     @inventree_transition(
         field=status,
         source=[BuildStatus.PENDING, BuildStatus.PRODUCTION, BuildStatus.ON_HOLD],
-        target=BuildStatus.COMPLETE,
+        target=DEFERRABLE(status, BuildStatus.COMPLETE),
     )
     def complete_build(self, user: User, trim_allocated_stock: bool = False):
         """Transition this Build to COMPLETE status.
@@ -978,6 +979,13 @@ class Build(
         Arguments:
             user: The user who is completing the build
             trim_allocated_stock: If True, trim any allocated stock
+
+        Notes:
+            The actual completion work (consuming allocations, deleting BuildItem
+            records, etc.) is done by build.tasks.complete_build(), which may run
+            inline or genuinely asynchronously on a background worker - see
+            DEFERRABLE for why the target is resolved from offload_task()'s return
+            value rather than being fixed.
         """
         import build.tasks
 
@@ -994,8 +1002,9 @@ class Build(
                 _('Cannot complete build order with incomplete outputs')
             )
 
-        # Offload background task to complete build allocations
-        InvenTree.tasks.offload_task(
+        # Offload background task to complete build allocations. DEFERRABLE
+        # inspects this return value directly, so it must be returned as-is.
+        return InvenTree.tasks.offload_task(
             build.tasks.complete_build,
             self.pk,
             user.pk if user else None,
@@ -1048,21 +1057,28 @@ class Build(
     @inventree_transition(
         field=status,
         source=[BuildStatus.PENDING, BuildStatus.PRODUCTION, BuildStatus.ON_HOLD],
-        target=BuildStatus.CANCELLED,
+        target=DEFERRABLE(status, BuildStatus.CANCELLED),
     )
     def cancel_build(self, user=None, **kwargs):
         """Transition this Build to CANCELLED status.
 
         Offloads expensive cleanup operations (de-allocating stock, removing
         incomplete outputs) to a background task.
+
+        Notes:
+            See DEFERRABLE - build.tasks.cancel_build() performs the actual
+            cleanup and sets completion_date/completed_by/status itself, once
+            that work is genuinely finished, in case it runs asynchronously.
         """
         import build.tasks
 
         remove_allocated_stock = kwargs.get('remove_allocated_stock', False)
         remove_incomplete_outputs = kwargs.get('remove_incomplete_outputs', False)
 
-        # Offload background task to take care of the expensive operations
-        InvenTree.tasks.offload_task(
+        # Offload background task to take care of the expensive operations.
+        # DEFERRABLE inspects this return value directly, so it must be
+        # returned as-is.
+        return InvenTree.tasks.offload_task(
             build.tasks.cancel_build,
             self.pk,
             user.pk if user else None,
@@ -1071,10 +1087,6 @@ class Build(
             group='build',
         )
 
-        # Date of 'completion' is the date the build was cancelled
-        self.completion_date = InvenTree.helpers.current_date()
-        self.completed_by = user
-
     # endregion fsm
 
     @transaction.atomic

src/backend/InvenTree/build/tasks.py

@@ -266,8 +266,22 @@ def cancel_build(
 
     """
     from build.models import Build
+    from build.status_codes import BuildStatus
 
-    build = Build.objects.get(pk=build_id)
+    with transaction.atomic():
+        # Lock the build row: concurrent cancellation tasks (duplicate task
+        # delivery, or repeated cancellation requests) are serialized, and the
+        # status is re-checked below
+        build = Build.objects.select_for_update().get(pk=build_id)
+
+        if build.status == BuildStatus.CANCELLED.value:
+            logger.warning(
+                'Build order <%s> is already cancelled - skipping cancellation task',
+                build.pk,
+            )
+            return
+
+        user = User.objects.filter(pk=user_id).first() if user_id else None
 
         if remove_allocated_stock:
             complete_build_allocations(build_id, user_id)
@@ -277,6 +291,12 @@ def cancel_build(
         if remove_incomplete_outputs:
             build.build_outputs.filter(is_building=True).delete()
 
+        # Mark the build as cancelled
+        build.completion_date = InvenTree.helpers.current_date()
+        build.completed_by = user
+        build.status = BuildStatus.CANCELLED.value
+        build.save()
+
     # Notify users that the order has been canceled
     InvenTree.helpers_model.notify_responsible(
         build,

src/backend/InvenTree/build/test_build.py

@@ -16,6 +16,7 @@ from django.test.utils import override_settings
 from django.urls import reverse
 
 import structlog
+from django_q.models import OrmQ
 
 import build.tasks
 import common.models
@@ -28,6 +29,7 @@ from InvenTree.unit_test import (
     InvenTreeAPITestCase,
     InvenTreeTestCase,
     findOffloadedEvent,
+    findOffloadedTask,
 )
 from order.models import PurchaseOrder, PurchaseOrderLineItem
 from part.models import BomItem, BomItemSubstitute, Part, PartTestTemplate
@@ -2175,3 +2177,169 @@ class BuildAllocateStockConcurrencyTest(TransactionTestCase):
 
         self.assertEqual(total_allocated, 5)
         self.assertLessEqual(total_allocated, self.stock_item.quantity)
+
+
+class BuildCompleteAsyncOffloadTest(BuildTestBase):
+    """Regression test for Build.complete_build() when genuinely offloaded to a background worker.
+
+    Every other test in this module calls complete_build() while InvenTree.tasks.offload_task()
+    takes its synchronous fallback branch, since no django_q worker is ever registered against
+    the test database. That collapses "the completion task was queued" and "the completion task
+    has run" into a single, in-order call - which hides the real-world race where a genuine
+    worker dequeues and runs the task well after complete_build() has already returned (and the
+    @inventree_transition wrapper has already written status=COMPLETE to the database).
+
+    This test forces offload_task() down its genuine asynchronous branch (as if a real django_q
+    cluster were running) without actually running one, so the completion task is left sitting -
+    unexecuted - in the django_q broker table. It then drives that queued task by hand, exactly
+    the way a real worker eventually would, and checks that the build order actually ends up
+    complete: a completion date is recorded, stock is consumed against the build, and the
+    consumed stock is no longer counted as available.
+    """
+
+    def test_complete_build_after_genuine_async_offload(self):
+        """Completing a build via a truly-offloaded task must still consume stock correctly."""
+        user = get_user_model().objects.get(pk=1)
+
+        self.build.issue_build()
+
+        # Fully allocate and complete the trackable line against both outputs
+        for output, qty in [(self.output_1, 6), (self.output_2, 14)]:
+            BuildItem.objects.create(
+                build_line=self.line_3,
+                stock_item=self.stock_3_1,
+                quantity=qty,
+                install_into=output,
+            )
+            self.build.complete_build_output(output, user)
+
+        self.assertEqual(self.build.incomplete_count, 0)
+
+        # Completing the tracked outputs above already consumes stock_3_1 against this
+        # build (synchronously - that path is not under test here). Record that baseline
+        # so the assertions below can isolate the effect of the *untracked* completion,
+        # which is what genuinely goes through the async offload under test.
+        baseline_consumed = StockItem.objects.filter(consumed_by=self.build).count()
+        self.assertGreater(baseline_consumed, 0)
+
+        # Partially allocate untracked stock_1_2 (100 in stock) - less than its full
+        # quantity, so completion must split off exactly the consumed amount
+        BuildItem.objects.create(
+            build_line=self.line_1, stock_item=self.stock_1_2, quantity=40
+        )
+
+        self.assertIsNone(self.build.completion_date)
+        self.assertFalse(StockItem.objects.filter(parent=self.stock_1_2).exists())
+
+        OrmQ.objects.all().delete()
+
+        # Force the *genuine* async branch of offload_task(), as if a real worker cluster
+        # were running - without actually running one
+        with mock.patch('InvenTree.status.is_worker_running', return_value=True):
+            self.build.complete_build(user)
+
+        # The completion task must be queued for the worker, not executed inline
+        task = findOffloadedTask(
+            'build.tasks.complete_build', matching_args=[self.build.pk]
+        )
+        self.assertIsNotNone(task)
+
+        # Nothing further has been consumed yet - the queued task has not actually run
+        self.assertIsNone(Build.objects.get(pk=self.build.pk).completion_date)
+        self.assertEqual(
+            StockItem.objects.filter(consumed_by=self.build).count(), baseline_consumed
+        )
+        self.assertFalse(StockItem.objects.filter(parent=self.stock_1_2).exists())
+        self.assertTrue(
+            BuildItem.objects.filter(
+                build_line=self.line_1, stock_item=self.stock_1_2
+            ).exists()
+        )
+
+        # Now simulate the worker actually picking up and running the queued task
+        build.tasks.complete_build(self.build.pk, user.pk, trim_allocated_stock=False)
+
+        self.build.refresh_from_db()
+
+        # The build must be marked complete, with a completion date recorded
+        self.assertEqual(self.build.status, BuildStatus.COMPLETE)
+        self.assertIsNotNone(self.build.completion_date)
+
+        # Stock must actually have been consumed against this build
+        consumed = StockItem.objects.filter(consumed_by=self.build)
+        self.assertGreater(consumed.count(), 0)
+
+        split_child = StockItem.objects.get(
+            parent=self.stock_1_2, consumed_by=self.build
+        )
+        self.assertEqual(split_child.quantity, 40)
+
+        # The available (unconsumed) stock must be reduced accordingly
+        remaining = StockItem.objects.get(pk=self.stock_1_2.pk)
+        self.assertIsNone(remaining.consumed_by)
+        self.assertEqual(remaining.quantity, 60)
+
+        # No BuildItem allocations should remain
+        self.assertFalse(
+            BuildItem.objects.filter(build_line__build=self.build).exists()
+        )
+
+
+class BuildCancelAsyncOffloadTest(BuildTestBase):
+    """Regression test for Build.cancel_build() when genuinely offloaded to a background worker.
+
+    Mirrors BuildCompleteAsyncOffloadTest: forces InvenTree.tasks.offload_task() down its
+    genuine asynchronous branch (as if a real django_q cluster were running), so
+    build.tasks.cancel_build() is left queued - unexecuted - rather than collapsed into the
+    same call via the synchronous test-mode fallback. Checks that the cleanup (removing
+    allocations, recording who/when it was cancelled) only happens once the queued task is
+    actually driven, and that the build order ends up in a fully consistent cancelled state.
+    """
+
+    def test_cancel_build_after_genuine_async_offload(self):
+        """Cancelling a build via a truly-offloaded task must still clean up correctly."""
+        user = get_user_model().objects.get(pk=1)
+
+        self.build.issue_build()
+
+        # Allocate some untracked stock, to be consumed on cancellation
+        BuildItem.objects.create(
+            build_line=self.line_1, stock_item=self.stock_1_2, quantity=40
+        )
+
+        self.assertIsNone(self.build.completion_date)
+
+        OrmQ.objects.all().delete()
+
+        with mock.patch('InvenTree.status.is_worker_running', return_value=True):
+            self.build.cancel_build(user, remove_allocated_stock=True)
+
+        # The cancellation task must be queued for the worker, not executed inline
+        task = findOffloadedTask(
+            'build.tasks.cancel_build', matching_args=[self.build.pk]
+        )
+        self.assertIsNotNone(task)
+
+        # Nothing has happened yet - the queued task has not actually run
+        stale = Build.objects.get(pk=self.build.pk)
+        self.assertEqual(stale.status, BuildStatus.PRODUCTION)
+        self.assertIsNone(stale.completion_date)
+        self.assertTrue(BuildItem.objects.filter(build_line=self.line_1).exists())
+        self.assertFalse(StockItem.objects.filter(parent=self.stock_1_2).exists())
+
+        # Now simulate the worker actually picking up and running the queued task
+        build.tasks.cancel_build(self.build.pk, user.pk, remove_allocated_stock=True)
+
+        self.build.refresh_from_db()
+
+        # The build must be marked cancelled, with a completion date and user recorded
+        self.assertEqual(self.build.status, BuildStatus.CANCELLED)
+        self.assertIsNotNone(self.build.completion_date)
+        self.assertEqual(self.build.completed_by, user)
+
+        # The allocation must have been consumed (not just silently deleted)
+        self.assertFalse(BuildItem.objects.filter(build_line=self.line_1).exists())
+        split_child = StockItem.objects.get(
+            parent=self.stock_1_2, consumed_by=self.build
+        )
+        self.assertEqual(split_child.quantity, 40)

src/backend/InvenTree/generic/states/__init__.py

@@ -11,9 +11,15 @@ from django_fsm import RETURN_VALUE, can_proceed
 from . import fields
 from .deprecations import Deprecations, deprecated
 from .states import ColorEnum, StatusCode, StatusCodeMixin
-from .transition import StateTransitionMixin, TransitionMethod, inventree_transition
+from .transition import (
+    DEFERRABLE,
+    StateTransitionMixin,
+    TransitionMethod,
+    inventree_transition,
+)
 
 __all__ = [
+    'DEFERRABLE',
     'RETURN_VALUE',  # django_fsm import
     'ColorEnum',
     'Deprecations',

src/backend/InvenTree/generic/states/transition.py

@@ -7,9 +7,10 @@ from functools import wraps
 from django.core.exceptions import ValidationError
 from django.db import transaction
 from django.db.models import Model
+from django.utils.translation import gettext_lazy as _
 
 import structlog
-from django_fsm import TransitionNotAllowed, transition
+from django_fsm import State, TransitionNotAllowed, transition
 
 from plugin.events import trigger_event
 
@@ -97,11 +98,20 @@ def inventree_transition(
                 # Preserve backward-compatible behaviour: an invalid transition
                 # (wrong source state, failed condition, or explicit raise inside
                 # the method body) returns False rather than raising.
-                if getattr(self, field.name) == target:
+                #
+                # `target` may be a State proxy (e.g. DEFERRABLE) rather than a
+                # plain value - such proxies expose their real, fixed target via
+                # a `.target` attribute, so unwrap that for this comparison. A
+                # proxy with no single fixed target (e.g. RETURN_VALUE) falls
+                # back to the generic "invalid transition" message below, since
+                # there is no one value to compare against.
+                resolved_target = getattr(target, 'target', target)
+                if getattr(self, field.name) == resolved_target:
                     target_val = (
-                        target.label
-                        if isinstance(target, Enum) and hasattr(target, 'label')
-                        else target
+                        resolved_target.label
+                        if isinstance(resolved_target, Enum)
+                        and hasattr(resolved_target, 'label')
+                        else resolved_target
                     )
                     raise ValidationError(
                         f'{self._meta.verbose_name} is already {target_val}'
@@ -127,6 +137,68 @@ def inventree_transition(
     return decorator
 
 
+class DEFERRABLE(State):
+    """Target-state proxy for a transition whose real work may be offloaded to a background worker.
+
+    Use this as the ``target=`` of ``@inventree_transition`` for a method whose body
+    calls ``InvenTree.tasks.offload_task(...)`` and returns that call's result
+    directly (do not swallow it). ``offload_task()`` returns one of three things,
+    and the actual next state is resolved accordingly:
+
+    * ``True`` - the task ran synchronously, inline (no worker was available) - the
+      real work is already finished, so the transition completes: the field is set
+      to ``target``.
+    * ``False`` - the task could not even be scheduled - nothing will ever perform
+      this transition now, so a ``ValidationError`` is raised rather than silently
+      pretending the transition happened.
+    * anything else (a task ID) - the work was genuinely queued for a background
+      worker and has not run yet. The field is left as its current (source) value;
+      the background task is responsible for performing the real transition itself
+      once it actually finishes the work.
+
+    Without this, a transition method that merely offloads work would have the
+    field advanced to ``target`` the instant it returns - regardless of whether a
+    worker has run yet - which lets any "is this already done?" guard in the
+    offloaded task itself misfire on its first (and only) real run. See
+    ``Build.complete_build()`` for a worked example.
+
+    Usage::
+
+        @inventree_transition(
+            field=status,
+            source=[...],
+            target=DEFERRABLE(status, MyStatus.COMPLETE),
+        )
+        def complete_thing(self, ...):
+            return InvenTree.tasks.offload_task(my_app.tasks.complete_thing, self.pk, ...)
+    """
+
+    def __init__(self, field, target) -> None:
+        """Store the status field (to read the current value back from) and the real target."""
+        self.field = field
+        self.target = target
+        self.allowed_states = []
+
+    def get_state(self, model, transition, result, args=None, kwargs=None):
+        """Resolve the actual next state from the offload_task() return value."""
+        if result is True:
+            # The offloaded task ran synchronously, inline, on its own copy of
+            # this row, and has already saved its results to the database.
+            # Refresh so those changes are not clobbered by the wrapper's own
+            # save() call, which is about to write this (now-stale) instance.
+            model.refresh_from_db()
+            return self.target
+
+        if result is False:
+            raise ValidationError(
+                _('Failed to offload background task for this transition')
+            )
+
+        # Anything else (a task ID) means the work was genuinely deferred to a
+        # background worker - the transition has not actually happened yet
+        return self.field.get_state(model)
+
+
 class TransitionMethod:
     """Base class for all plugin-defined transition handler classes.
 

src/backend/InvenTree/order/test_sales_order.py

@@ -459,8 +459,27 @@ class SalesOrderTest(InvenTreeAPITestCase):
         # completion must be skipped based on the database state
         self.assertEqual(order_b.status, status.SalesOrderStatus.SHIPPED)
 
-        with self.assertRaises(ValidationError):
-            self.assertFalse(order_b.complete_order(None))
+        with self.assertRaises(ValidationError) as err:
+            order_b.complete_order(None)
+
+        # complete_order() targets RETURN_VALUE(SHIPPED, COMPLETE), a State proxy with
+        # no single fixed value - unlike a plain-target transition (e.g.
+        # Build.cancel_build), inventree_transition cannot resolve a friendly "is
+        # already Complete" message for it, so it falls back to the generic
+        # "invalid transition" message instead. Assert on that message explicitly, so
+        # a regression here (e.g. this falling back to some other error entirely) is
+        # actually caught rather than passing on any ValidationError whatsoever.
+        expected_source = (
+            status.SalesOrderStatus.PENDING,
+            status.SalesOrderStatus.IN_PROGRESS,
+            status.SalesOrderStatus.ON_HOLD,
+            status.SalesOrderStatus.SHIPPED,
+        )
+        self.assertIn(
+            f'Invalid transition on Sales Order.status (source value should be'
+            f' {list(expected_source)}, is {status.SalesOrderStatus.COMPLETE.value})',
+            str(err.exception),
+        )
 
         self.order.refresh_from_db()
         self.assertEqual(self.order.status, status.SalesOrderStatus.COMPLETE)