From 2dca27f78b4f731ec3be97a054e9e45640c16b9e Mon Sep 17 00:00:00 2001 From: Oliver Date: Thu, 27 Aug 2026 15:01:20 +1000 Subject: [PATCH] Fix race condition for take_stock method (#12718) --- src/backend/InvenTree/stock/models.py | 13 +++- src/backend/InvenTree/stock/tests.py | 101 ++++++++++++++++++++++++++ 2 files changed, 110 insertions(+), 4 deletions(-) diff --git a/src/backend/InvenTree/stock/models.py b/src/backend/InvenTree/stock/models.py index 47182b0687..c27ea7be74 100644 --- a/src/backend/InvenTree/stock/models.py +++ b/src/backend/InvenTree/stock/models.py @@ -3401,10 +3401,6 @@ class StockItem( except InvalidOperation: return False - # Cannot remove more than the available quantity - # (also ensures the recorded history matches the actual removal) - quantity = min(quantity, self.quantity) - if quantity <= 0: return False @@ -3412,6 +3408,15 @@ class StockItem( if not self.lock_quantity(): return False + # Cannot remove more than the available quantity + # (also ensures the recorded history matches the actual removal) + # This must happen *after* the lock above, so it is checked against + # the current database value rather than a potentially stale copy + quantity = min(quantity, self.quantity) + + if quantity <= 0: + return False + deltas = {} status = self._resolve_status_kwarg(kwargs) diff --git a/src/backend/InvenTree/stock/tests.py b/src/backend/InvenTree/stock/tests.py index fe8d3926bb..e312ae1f17 100644 --- a/src/backend/InvenTree/stock/tests.py +++ b/src/backend/InvenTree/stock/tests.py @@ -2353,3 +2353,104 @@ class StockItemSerialAPIConcurrencyTest(TransactionTestCase): self.assertEqual( StockItem.objects.filter(part=self.part, serial='SN-API-RACE').count(), 1 ) + + +@skipUnlessDBFeature('has_select_for_update') +class StockItemTakeStockConcurrencyTest(TransactionTestCase): + """Genuine cross-transaction regression test for StockItem.take_stock(). + + Uses two real threads (each with its own database connection) to reproduce + a reported race: take_stock() capped the requested removal quantity + against self.quantity *before* calling lock_quantity() (which locks the + row and refreshes self.quantity from the database). Two concurrent + full-quantity removal requests against the same StockItem, each starting + from its own (initially correct, but potentially stale by the time the + lock is acquired) in-memory copy, could both cap to the same amount - the + first to acquire the lock removes it all, and the second, upon acquiring + the lock, would find nothing left, but would still record a phantom + removal in StockItemTracking and report success. + + take_stock() now performs the cap *after* lock_quantity() has refreshed + self.quantity, so the loser of the race is correctly capped to zero and + rejects the request instead of recording a phantom removal. + """ + + fixtures = ['users'] + + def setUp(self): + """Create a single StockItem with just enough quantity for one removal.""" + super().setUp() + + self.user = get_user_model().objects.get(pk=1) + + self.part = Part.objects.create( + name='Take stock concurrency part', + description='Part for take_stock concurrency test', + ) + + self.item = StockItem.objects.create( + part=self.part, quantity=10, delete_on_deplete=False + ) + + def test_concurrent_take_stock_does_not_phantom_remove(self): + """Two concurrent full-quantity removals must not both report success.""" + start_barrier = threading.Barrier(2, timeout=5) + errors = [] + results = [] + results_lock = threading.Lock() + + # Wrap StockItem.lock_quantity() so both threads reach the (real, + # database-level) row lock at the same time - one wins the lock and + # proceeds, the other blocks until the winner's transaction completes. + original_lock_quantity = StockItem.lock_quantity + + def synced_lock_quantity(self_item): + start_barrier.wait(timeout=5) + return original_lock_quantity(self_item) + + def take_stock(): + try: + # Each thread works from its own in-memory copy, fetched + # before either has removed anything - mirroring a real + # request handler that loads the item, then races another. + item = StockItem.objects.get(pk=self.item.pk) + result = item.take_stock(10, self.user) + with results_lock: + results.append(result) + except Exception as exc: # pragma: no cover - surfaced via errors list + with results_lock: + errors.append(exc) + finally: + connection.close() + + thread_a = threading.Thread(target=take_stock) + thread_b = threading.Thread(target=take_stock) + + with mock.patch.object(StockItem, 'lock_quantity', synced_lock_quantity): + thread_a.start() + thread_b.start() + + thread_a.join(timeout=5) + thread_b.join(timeout=5) + + self.assertFalse(thread_a.is_alive()) + self.assertFalse(thread_b.is_alive()) + self.assertEqual(errors, []) + + # Exactly one removal must have succeeded; the other must have been + # rejected (nothing left to remove) rather than reporting a phantom + # success + self.assertEqual(sorted(results), [False, True]) + + self.item.refresh_from_db() + self.assertEqual(self.item.quantity, 0) + + # No phantom removal may appear in the tracking history: the total + # recorded 'removed' amount must not exceed the quantity that + # actually existed + total_removed = sum( + entry.deltas.get('removed', 0) + for entry in self.item.tracking_info.all() + if entry.deltas + ) + self.assertEqual(total_removed, 10)