From db58858c6879050c647d91224624756d65478799 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Sun, 5 Jul 2026 22:31:29 +0530 Subject: [PATCH] fix(stock): close postgres lock-then-read races in pick list and stock reservation Postgres has no gap locks, so the lock-then-read pattern (plain SELECT ... FOR UPDATE before a grouped read) only serializes on rows that already exist. Two sites had reachable races where the lock set is empty or disjoint: - Pick list: two pick lists against the same SO item submitted concurrently lock only docstatus=1 rows, so with no previously-submitted picks their lock sets are disjoint and both pass validate_picked_qty (over-pick; picked_qty last-writer-wins). Gate on the referenced Sales Order Item / Packed Item rows, which always exist. - Stock reservation: the first concurrent reservations for an (item, warehouse) find no SRE rows to lock, so both pass and reserved qty can exceed actual. Gate on the Bin row, which exists once there is stock. MariaDB is unchanged (its gap locks already serialize both; the gates are postgres-only). Also: ORDER BY on the small-set postgres lock selects for deterministic lock order, and the repost pre-lock in get_future_stock_vouchers selects a constant instead of shipping every matching SLE name to the client. --- erpnext/accounts/utils.py | 6 ++++-- erpnext/stock/doctype/pick_list/pick_list.py | 18 +++++++++++++++--- .../stock_reservation_entry.py | 15 ++++++++++++--- 3 files changed, 31 insertions(+), 8 deletions(-) diff --git a/erpnext/accounts/utils.py b/erpnext/accounts/utils.py index d6cda9cf548..9b50751e95a 100644 --- a/erpnext/accounts/utils.py +++ b/erpnext/accounts/utils.py @@ -13,6 +13,7 @@ from frappe.desk.reportview import build_match_conditions from frappe.model.meta import get_field_precision from frappe.model.naming import determine_consecutive_week_number from frappe.query_builder import AliasedQuery, Case, Criterion, Field, Table +from frappe.query_builder.custom import ConstantColumn from frappe.query_builder.functions import Count, IfNull, Max, Min, Round, Sum from frappe.query_builder.utils import DocType from frappe.utils import ( @@ -1791,9 +1792,10 @@ def get_future_stock_vouchers(posting_date, posting_time, for_warehouses=None, f # transaction can't modify them mid-flight (the original DISTINCT ... FOR UPDATE did this). # MariaDB carries the lock on the grouped query below; postgres rejects FOR UPDATE alongside # GROUP BY, so lock the matching rows in a separate pass first -- the row locks are held until - # the surrounding transaction ends, giving the same protection. + # the surrounding transaction ends, giving the same protection. Select a constant, not the + # name: a deep backdated repost can match millions of rows and only the locks are needed. if frappe.db.db_type == "postgres": - frappe.qb.from_(SLE).select(SLE.name).where(conditions).for_update().run() + frappe.qb.from_(SLE).select(ConstantColumn(1)).where(conditions).for_update().run() # distinct vouchers in chronological order; expressed as GROUP BY + Min() so it's valid on # postgres (SELECT DISTINCT can't ORDER BY non-selected cols, and FOR UPDATE is invalid with both). diff --git a/erpnext/stock/doctype/pick_list/pick_list.py b/erpnext/stock/doctype/pick_list/pick_list.py index 846c020de72..53e515b0f8e 100644 --- a/erpnext/stock/doctype/pick_list/pick_list.py +++ b/erpnext/stock/doctype/pick_list/pick_list.py @@ -952,10 +952,22 @@ def get_picked_items_qty(items, contains_packed_items=False) -> list[dict]: ) # Lock the picked-qty rows so a concurrent pick can't change them mid-transaction. MariaDB carries - # the lock on the grouped query; postgres rejects FOR UPDATE with GROUP BY, so lock the same rows - # in a separate plain SELECT first (held for the transaction). + # the lock on the grouped query (its gap locks also block rows other in-flight picks are about to + # submit); postgres has no gap locks, so first serialize on the referenced SO/packed item rows + # (they always exist), then lock the matching picked rows in a separate plain SELECT. if frappe.db.db_type == "postgres": - frappe.qb.from_(pi_item).select(pi_item.name).where(conditions).for_update().run() + parent = frappe.qb.DocType("Packed Item" if contains_packed_items else "Sales Order Item") + ( + frappe.qb.from_(parent) + .select(parent.name) + .where(parent.name.isin(items)) + .orderby(parent.name) + .for_update() + .run() + ) + frappe.qb.from_(pi_item).select(pi_item.name).where(conditions).orderby( + pi_item.name + ).for_update().run() else: query = query.for_update() diff --git a/erpnext/stock/doctype/stock_reservation_entry/stock_reservation_entry.py b/erpnext/stock/doctype/stock_reservation_entry/stock_reservation_entry.py index ad6e965b186..3dbdc2419c9 100644 --- a/erpnext/stock/doctype/stock_reservation_entry/stock_reservation_entry.py +++ b/erpnext/stock/doctype/stock_reservation_entry/stock_reservation_entry.py @@ -716,10 +716,19 @@ def get_available_qty_to_reserve( conditions &= sre.name != ignore_sre # Lock the rows being aggregated so a concurrent reservation can't change them mid-transaction. - # MariaDB carries the lock on the aggregate query itself; postgres rejects FOR UPDATE with an - # aggregate, so on postgres lock the same rows in a separate plain SELECT first (held for the txn). + # MariaDB carries the lock on the aggregate query itself (its gap locks also serialize two + # FIRST reservations, when no SRE rows exist yet); postgres has no gap locks, so gate on the + # Bin row (exists once there is stock), then lock the matching SREs in a plain SELECT. if frappe.db.db_type == "postgres": - frappe.qb.from_(sre).select(sre.name).where(conditions).for_update().run() + bin_table = frappe.qb.DocType("Bin") + ( + frappe.qb.from_(bin_table) + .select(bin_table.name) + .where((bin_table.item_code == item_code) & (bin_table.warehouse == warehouse)) + .for_update() + .run() + ) + frappe.qb.from_(sre).select(sre.name).where(conditions).orderby(sre.name).for_update().run() query = ( frappe.qb.from_(sre)