From e32fefa0783ca7abd172c7b6d58f1dc4f9ecd84d Mon Sep 17 00:00:00 2001 From: Guewen Baconnier Date: Thu, 28 Jan 2021 16:11:52 +0100 Subject: [PATCH] Fix concurrency race condition on picking state When the 2 last moves of a stock.picking are assigned at the same time by 2 jobs (different products both available), none of the transaction will see that it is the last move to be assigned. As a result, the picking will stay in state "confirmed" even if all its moves are assigned. Lock the stock.picking records when we call auto_assign. If we have many pickings touched for the same product, the lock can be quite large, so we may have to find a better option. I could not write a test to exercise this, because we can't have 2 transactions being aware of it, even if we create the picking in demo data as tests can be run during install. --- .../models/product_product.py | 29 +++++++++- stock_move_auto_assign/tests/common.py | 56 +++++++++++++++++++ .../tests/test_auto_assign.py | 53 +----------------- 3 files changed, 87 insertions(+), 51 deletions(-) create mode 100644 stock_move_auto_assign/tests/common.py diff --git a/stock_move_auto_assign/models/product_product.py b/stock_move_auto_assign/models/product_product.py index 5310950a5..2a2f880f1 100644 --- a/stock_move_auto_assign/models/product_product.py +++ b/stock_move_auto_assign/models/product_product.py @@ -1,10 +1,17 @@ # Copyright 2020 Camptocamp SA # License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html). +import logging + +import psycopg2 + from odoo import models +from odoo.addons.queue_job.exception import RetryableJobError from odoo.addons.queue_job.job import job +_logger = logging.getLogger(__name__) + class ProductProduct(models.Model): _inherit = "product.product" @@ -25,9 +32,10 @@ class ProductProduct(models.Model): # out anyway. ] + # TODO remove decorator @job(default_channel="root.stock_auto_assign") def moves_auto_assign(self, locations): - """Try to reserve moves based on product and locations + """Job trying to reserve moves based on product and locations When a product has been added to a location, it searches all* the moves with a source equal or above this location and try @@ -38,4 +46,23 @@ class ProductProduct(models.Model): """ self.ensure_one() moves = self.env["stock.move"].search(self._moves_auto_assign_domain(locations)) + pickings = moves.picking_id + if not pickings: + return + try: + self.env.cr.execute( + "SELECT id FROM stock_picking WHERE id IN %s FOR UPDATE NOWAIT", + (tuple(pickings.ids),), + ) + except psycopg2.OperationalError as err: + if err.pgcode == "55P03": # could not obtain the lock + _logger.debug( + "Another job is already auto-assigning moves and acquired a" + " lock on one or some of stock.picking%s, retry later.", + tuple(pickings.ids), + ) + raise RetryableJobError( + "Could not obtain lock on transfers, will retry.", ignore_retry=True + ) + raise moves._action_assign() diff --git a/stock_move_auto_assign/tests/common.py b/stock_move_auto_assign/tests/common.py new file mode 100644 index 000000000..777fab414 --- /dev/null +++ b/stock_move_auto_assign/tests/common.py @@ -0,0 +1,56 @@ +# Copyright 2020-2021 Camptocamp SA +# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html). + +from odoo.tests import SavepointCase + + +class StockMoveAutoAssignCase(SavepointCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.env = cls.env(context=dict(cls.env.context, tracking_disable=True)) + + cls.wh = cls.env.ref("stock.warehouse0") + cls.out_type = cls.wh.out_type_id + cls.in_type = cls.wh.in_type_id + cls.int_type = cls.wh.int_type_id + + cls.customer_loc = cls.env.ref("stock.stock_location_customers") + cls.supplier_loc = cls.env.ref("stock.stock_location_suppliers") + cls.shelf1_loc = cls.env.ref("stock.stock_location_components") + cls.shelf2_loc = cls.env.ref("stock.stock_location_14") + + cls.product = cls.env["product.product"].create( + {"name": "Product", "type": "product"} + ) + + def _create_move( + self, + product, + picking_type, + qty=1.0, + state="confirmed", + procure_method="make_to_stock", + move_dest=None, + ): + source = picking_type.default_location_src_id or self.supplier_loc + dest = picking_type.default_location_dest_id or self.customer_loc + move_vals = { + "name": product.name, + "product_id": product.id, + "product_uom_qty": qty, + "product_uom": product.uom_id.id, + "picking_type_id": picking_type.id, + "location_id": source.id, + "location_dest_id": dest.id, + "state": state, + "procure_method": procure_method, + } + if move_dest: + move_vals["move_dest_ids"] = [(4, move_dest.id, False)] + return self.env["stock.move"].create(move_vals) + + def _update_qty_in_location(self, location, product, quantity): + location.env["stock.quant"]._update_available_quantity( + product, location, quantity + ) diff --git a/stock_move_auto_assign/tests/test_auto_assign.py b/stock_move_auto_assign/tests/test_auto_assign.py index 2d389ec74..7c2bc7d66 100644 --- a/stock_move_auto_assign/tests/test_auto_assign.py +++ b/stock_move_auto_assign/tests/test_auto_assign.py @@ -1,65 +1,18 @@ # Copyright 2020 Camptocamp SA # License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html). -from odoo.tests import SavepointCase - from odoo.addons.queue_job.job import identity_exact from odoo.addons.queue_job.tests.common import mock_with_delay +from .common import StockMoveAutoAssignCase -class TestStockMoveAutoAssign(SavepointCase): - @classmethod - def setUpClass(cls): - super().setUpClass() - cls.env = cls.env(context=dict(cls.env.context, tracking_disable=True)) - - cls.wh = cls.env.ref("stock.warehouse0") - cls.out_type = cls.wh.out_type_id - cls.in_type = cls.wh.in_type_id - cls.int_type = cls.wh.int_type_id - - cls.customer_loc = cls.env.ref("stock.stock_location_customers") - cls.supplier_loc = cls.env.ref("stock.stock_location_suppliers") - cls.shelf1_loc = cls.env.ref("stock.stock_location_components") - cls.shelf2_loc = cls.env.ref("stock.stock_location_14") - - cls.product = cls.env["product.product"].create( - {"name": "Product", "type": "product"} - ) - - def _create_move( - self, - product, - picking_type, - qty=1.0, - state="confirmed", - procure_method="make_to_stock", - move_dest=None, - ): - source = picking_type.default_location_src_id or self.supplier_loc - dest = picking_type.default_location_dest_id or self.customer_loc - move_vals = { - "name": product.name, - "product_id": product.id, - "product_uom_qty": qty, - "product_uom": product.uom_id.id, - "picking_type_id": picking_type.id, - "location_id": source.id, - "location_dest_id": dest.id, - "state": state, - "procure_method": procure_method, - } - if move_dest: - move_vals["move_dest_ids"] = [(4, move_dest.id, False)] - return self.env["stock.move"].create(move_vals) - - def _update_qty_in_location(self, location, product, quantity): - self.env["stock.quant"]._update_available_quantity(product, location, quantity) +class TestStockMoveAutoAssign(StockMoveAutoAssignCase): def test_job_assign_confirmed_move(self): """Test job method, assign moves matching product and location""" move1 = self._create_move(self.product, self.out_type) move2 = self._create_move(self.product, self.out_type) + (move1 | move2)._assign_picking() # put stock in Stock/Shelf 1, the move has a source location in Stock self._update_qty_in_location(self.shelf1_loc, self.product, 100) self.product.moves_auto_assign(self.shelf1_loc)