mirror of
https://github.com/frappe/erpnext.git
synced 2026-08-13 14:41:53 +00:00
fix: classify repost recovery by exception type, not traceback string
repost() decided whether a failed Repost Item Valuation was recoverable (re-queue as "In Progress") or permanently "Failed" by string-matching the traceback for "timeout" or MariaDB's "Deadlock found". On Postgres a deadlock surfaces as "deadlock detected" / "could not serialize access" and matches neither, so a retriable deadlock was marked Failed and never re-queued -- the scheduler only re-picks Queued/In Progress entries. Classify by isinstance(e, RecoverableErrors) instead, the same tuple already used to gate the error email. This covers deadlocks and lock/query timeouts on both engines (frappe raises QueryDeadlockError / QueryTimeoutError uniformly) and the advisory-lock repost gate's own QueryTimeoutError, which previously recovered only because its class name incidentally contains "timeout".
This commit is contained in:
@@ -425,10 +425,10 @@ def repost(doc):
|
||||
if isinstance(message, dict):
|
||||
message = message.get("message")
|
||||
|
||||
status = "Failed"
|
||||
# If failed because of timeout, set status to In Progress
|
||||
if traceback and ("timeout" in traceback.lower() or "Deadlock found" in traceback):
|
||||
status = "In Progress"
|
||||
# Recoverable errors (deadlock, lock/query timeout, job timeout) re-queue as In Progress.
|
||||
# Classify by type: the old traceback string-match only knew MariaDB's "Deadlock found" and
|
||||
# missed Postgres deadlocks ("deadlock detected"), failing them permanently.
|
||||
status = "In Progress" if isinstance(e, RecoverableErrors) else "Failed"
|
||||
|
||||
if traceback:
|
||||
message += "<br><br>" + "<b>Traceback:</b> <br>" + traceback
|
||||
@@ -447,7 +447,8 @@ def repost(doc):
|
||||
"Email Account", {"default_outgoing": 1, "enable_outgoing": 1}, "name"
|
||||
)
|
||||
|
||||
if outgoing_email_account and not isinstance(e, RecoverableErrors):
|
||||
# status == "Failed" already implies e is not recoverable, so no need to re-check here.
|
||||
if outgoing_email_account:
|
||||
notify_error_to_stock_managers(doc, message)
|
||||
doc.set_status("Failed")
|
||||
finally:
|
||||
|
||||
@@ -220,6 +220,39 @@ class TestRepostItemValuation(ERPNextTestSuite, StockTestMixin):
|
||||
sorted(frappe.parse_json(frappe.as_json(set([("a", "b"), ("c", "d")])))),
|
||||
)
|
||||
|
||||
def test_recoverable_error_requeues_instead_of_failing(self):
|
||||
# A recoverable DB error (e.g. Postgres deadlock -> QueryDeadlockError) must re-queue the
|
||||
# repost as "In Progress"; a non-recoverable error still fails. Regression: the old check
|
||||
# string-matched MariaDB's "Deadlock found" and missed Postgres deadlocks ("deadlock detected").
|
||||
from unittest.mock import patch
|
||||
|
||||
from frappe.exceptions import QueryDeadlockError
|
||||
|
||||
from erpnext.stock.doctype.repost_item_valuation import repost_item_valuation as riv
|
||||
|
||||
orig_max_writes = frappe.db.MAX_WRITES_PER_TRANSACTION
|
||||
self.addCleanup(setattr, frappe.db, "MAX_WRITES_PER_TRANSACTION", orig_max_writes)
|
||||
|
||||
def status_after(error):
|
||||
doc = frappe.new_doc("Repost Item Valuation")
|
||||
doc.name = "test-recoverable-riv"
|
||||
doc.set_status = doc.log_error = doc.db_set = MagicMock()
|
||||
captured = {}
|
||||
with (
|
||||
patch.object(frappe, "in_test", False),
|
||||
patch.object(frappe.db, "exists", return_value=True),
|
||||
patch.object(frappe.db, "commit"),
|
||||
patch.object(frappe.db, "rollback"),
|
||||
patch.object(frappe.db, "set_value", side_effect=lambda *a, **k: captured.update(a[2])),
|
||||
patch.object(riv, "repost_sl_entries", side_effect=error),
|
||||
patch.object(frappe, "get_cached_value", return_value=None),
|
||||
):
|
||||
riv.repost(doc)
|
||||
return captured.get("status")
|
||||
|
||||
self.assertEqual(status_after(QueryDeadlockError("deadlock detected")), "In Progress")
|
||||
self.assertEqual(status_after(ValueError("boom")), "Failed")
|
||||
|
||||
def test_gl_repost_progress(self):
|
||||
from erpnext.accounts import utils
|
||||
|
||||
|
||||
Reference in New Issue
Block a user