diff --git a/src/backend/InvenTree/build/models.py b/src/backend/InvenTree/build/models.py index ede56d0f61..8f3680bb02 100644 --- a/src/backend/InvenTree/build/models.py +++ b/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 diff --git a/src/backend/InvenTree/build/tasks.py b/src/backend/InvenTree/build/tasks.py index ad90443493..f1b5e425b4 100644 --- a/src/backend/InvenTree/build/tasks.py +++ b/src/backend/InvenTree/build/tasks.py @@ -266,16 +266,36 @@ 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 remove_allocated_stock: - complete_build_allocations(build_id, user_id) - else: - build.allocated_stock.all().delete() + if build.status == BuildStatus.CANCELLED.value: + logger.warning( + 'Build order <%s> is already cancelled - skipping cancellation task', + build.pk, + ) + return - if remove_incomplete_outputs: - build.build_outputs.filter(is_building=True).delete() + user = User.objects.filter(pk=user_id).first() if user_id else None + + if remove_allocated_stock: + complete_build_allocations(build_id, user_id) + else: + build.allocated_stock.all().delete() + + 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( diff --git a/src/backend/InvenTree/build/test_build.py b/src/backend/InvenTree/build/test_build.py index 2fd09cfd54..7075db8e3f 100644 --- a/src/backend/InvenTree/build/test_build.py +++ b/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) diff --git a/src/backend/InvenTree/generic/states/__init__.py b/src/backend/InvenTree/generic/states/__init__.py index b33592139a..9a827a040b 100644 --- a/src/backend/InvenTree/generic/states/__init__.py +++ b/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', diff --git a/src/backend/InvenTree/generic/states/transition.py b/src/backend/InvenTree/generic/states/transition.py index e33eba079f..2543ef46cf 100644 --- a/src/backend/InvenTree/generic/states/transition.py +++ b/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. diff --git a/src/backend/InvenTree/order/test_sales_order.py b/src/backend/InvenTree/order/test_sales_order.py index 2c53b3a92b..f1420a241b 100644 --- a/src/backend/InvenTree/order/test_sales_order.py +++ b/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)