fix(spp_hide_menus_base): make the menu-hiding pass best-effort on database errors (#526)

spp_hide_menus_base/README.rst

@@ -103,6 +103,23 @@ Dependencies
 Changelog
 =========
 
+19.0.2.1.1
+~~~~~~~~~~
+
+- fix: make the menu-hiding pass run by ``_register_hook`` (and
+  ``next()``) best-effort on database errors (#526). ``hide_menus()``
+  now runs in its own savepoint and logs and skips any
+  ``psycopg2.Error`` instead of letting it escape ``_register_hook`` and
+  abort the registry load — on a poisoned cursor that meant every
+  restart failed until the database was repaired by hand, the shape of
+  the 2026-07-30 preprod incident (OpenSPP/odoo-job-worker#22), which
+  #383 first attributed to the ``spp_base_common`` menu-icon hook. The
+  caller's own pending writes are flushed before the guard and stay the
+  caller's error; a missing menu xmlid is skipped through
+  ``raise_if_not_found=False``. Only ``psycopg2.Error`` is caught, and
+  the pass stays all-or-nothing: a Python exception here is a bug in
+  this module and still fails loudly.
+
 19.0.2.1.0
 ~~~~~~~~~~
 

spp_hide_menus_base/__manifest__.py

@@ -5,7 +5,7 @@
 {
     "name": "OpenSPP Hide Non-OpenSPP Menus: Base",
     "category": "OpenSPP",
-    "version": "19.0.2.1.0",
+    "version": "19.0.2.1.1",
     "summary": "Administrators can manage the visibility of OpenSPP navigation menus, streamlining the user interface for specific user groups. The module modifies ir.ui.menu records to control menu visibility, providing a foundation for other modules to selectively hide non-essential navigation items.",
     "sequence": 1,
     "author": "OpenSPP.org",

spp_hide_menus_base/models/ir_module_module.py

@@ -1,5 +1,7 @@
 import logging
 
+import psycopg2
+
 from odoo import models
 
 _logger = logging.getLogger(__name__)
@@ -58,41 +60,74 @@ class IrModuleModule(models.Model):
     }
 
     def hide_menus(self):
+        """Hide the root menus of the stock apps listed in MENU_APP.
+
+        Best-effort: this runs from ``_register_hook`` at the end of every
+        registry load, where a database error that escapes aborts the load
+        and takes the instance (or a job worker) down with it. A menu left
+        visible is recoverable, so the whole pass runs in its own savepoint
+        and is logged and skipped on any ``psycopg2.Error``.
+
+        The caller's pending ORM writes are flushed first so that only the
+        hiding pass itself is covered by the guard; a failure in the caller's
+        own writes stays the caller's error. Retryable errors (serialization
+        failures, deadlocks) are deliberately swallowed too: the registry
+        load has no retry loop, and on the ``next()`` path a retry would
+        rebuild the registry for a menu write.
+
+        Two deliberate limits. Only ``psycopg2.Error`` is caught: a Python
+        exception raised here is a bug in this module and must fail loudly
+        in tests rather than silently leave menus visible in production. And
+        the pass is all-or-nothing, as in ``spp_base_common``: database
+        errors are transaction-wide in practice, so a savepoint per catalog
+        entry would add cost without adding recoverable cases.
+        """
+        self.env.cr.flush()
+        try:
+            with self.env.cr.savepoint():
+                self._hide_catalog_menus()
+        except psycopg2.Error:
+            _logger.warning(
+                "Skipping the OpenSPP menu hiding pass because the database reported an error; "
+                "the menus keep their current visibility and this hook will not be the cause of a failed registry load",
+                exc_info=True,
+            )
+
+    def _hide_catalog_menus(self):
         for module in self.search([]):
             menu_info = self.MENU_APP.get(module.name)
-            if menu_info:
-                try:
-                    menu = self.env.ref(menu_info["menu_xml_id"])
-                except ValueError:
-                    _logger.debug("Menu XML ID not found: %s", menu_info["menu_xml_id"])
-                    menu = False
+            if not menu_info:
+                continue
+            menu = self.env.ref(menu_info["menu_xml_id"], raise_if_not_found=False)
+            if not menu:
+                _logger.debug("Menu XML ID not found: %s", menu_info["menu_xml_id"])
+                continue
 
-                if menu:
-                    hidden_menus = self.env["spp.hide.menu"].search([("menu_id", "=", menu.id)])
-                    if not hidden_menus:
-                        hidden_menu = self.env["spp.hide.menu"].create(
-                            {
-                                "menu_id": menu.id,
-                                "xml_id": menu_info["menu_xml_id"],
-                            }
-                        )
-                        hidden_menu.hide_menu()
-                        continue
+            hidden_menus = self.env["spp.hide.menu"].search([("menu_id", "=", menu.id)])
+            if not hidden_menus:
+                hidden_menu = self.env["spp.hide.menu"].create(
+                    {
+                        "menu_id": menu.id,
+                        "xml_id": menu_info["menu_xml_id"],
+                    }
+                )
+                hidden_menu.hide_menu()
+                continue
 
-                    # Read state off ONE row, never off the search result. This
-                    # method runs from _register_hook, so an Expected singleton
-                    # here aborts the whole registry load and every request 500s
-                    # until the extra row is deleted by hand. UNIQUE(menu_id)
-                    # normally rules that out, but a database that already held
-                    # duplicates when the constraint landed keeps them: the
-                    # registry logs the failed constraint and carries on.
-                    hidden_menu = hidden_menus._primary()
-                    if hidden_menu.state == "show":
-                        hidden_menu.hide_menu()
-                    elif hidden_menu.state == "hide":
-                        # Module upgrade may have reset group_ids via XML
-                        # (noupdate="0"). Re-apply hiding if stale.
-                        hidden_menu._reapply_hide()
+            # Read state off ONE row, never off the search result. This
+            # method runs from _register_hook, so an Expected singleton
+            # here aborts the whole registry load and every request 500s
+            # until the extra row is deleted by hand. UNIQUE(menu_id)
+            # normally rules that out, but a database that already held
+            # duplicates when the constraint landed keeps them: the
+            # registry logs the failed constraint and carries on.
+            hidden_menu = hidden_menus._primary()
+            if hidden_menu.state == "show":
+                hidden_menu.hide_menu()
+            elif hidden_menu.state == "hide":
+                # Module upgrade may have reset group_ids via XML
+                # (noupdate="0"). Re-apply hiding if stale.
+                hidden_menu._reapply_hide()
 
     def next(self):
         # Call your menu hiding logic first

spp_hide_menus_base/readme/HISTORY.md

@@ -1,3 +1,18 @@
+### 19.0.2.1.1
+
+- fix: make the menu-hiding pass run by ``_register_hook`` (and ``next()``)
+  best-effort on database errors (#526). ``hide_menus()`` now runs in its own
+  savepoint and logs and skips any ``psycopg2.Error`` instead of letting it
+  escape ``_register_hook`` and abort the registry load — on a poisoned
+  cursor that meant every restart failed until the database was repaired by
+  hand, the shape of the 2026-07-30 preprod incident
+  (OpenSPP/odoo-job-worker#22), which #383 first attributed to the
+  ``spp_base_common`` menu-icon hook. The caller's own pending writes are
+  flushed before the guard and stay the caller's error; a missing menu xmlid
+  is skipped through ``raise_if_not_found=False``. Only ``psycopg2.Error`` is
+  caught, and the pass stays all-or-nothing: a Python exception here is a
+  bug in this module and still fails loudly.
+
 ### 19.0.2.1.0
 
 - Enforce ``UNIQUE(menu_id)`` on ``spp.hide.menu``: a second configuration row

spp_hide_menus_base/static/description/index.html

@@ -474,6 +474,24 @@ <h2><a class="toc-backref" href="#toc-entry-1">Changelog</a></h2>
 </div>
 </div>
 <div class="section" id="section-1">
+<h1>19.0.2.1.1</h1>
+<ul class="simple">
+<li>fix: make the menu-hiding pass run by <tt class="docutils literal">_register_hook</tt> (and
+<tt class="docutils literal">next()</tt>) best-effort on database errors (#526). <tt class="docutils literal">hide_menus()</tt>
+now runs in its own savepoint and logs and skips any
+<tt class="docutils literal">psycopg2.Error</tt> instead of letting it escape <tt class="docutils literal">_register_hook</tt> and
+abort the registry load — on a poisoned cursor that meant every
+restart failed until the database was repaired by hand, the shape of
+the 2026-07-30 preprod incident (OpenSPP/odoo-job-worker#22), which
+#383 first attributed to the <tt class="docutils literal">spp_base_common</tt> menu-icon hook. The
+caller’s own pending writes are flushed before the guard and stay the
+caller’s error; a missing menu xmlid is skipped through
+<tt class="docutils literal">raise_if_not_found=False</tt>. Only <tt class="docutils literal">psycopg2.Error</tt> is caught, and
+the pass stays all-or-nothing: a Python exception here is a bug in
+this module and still fails loudly.</li>
+</ul>
+</div>
+<div class="section" id="section-2">
 <h1>19.0.2.1.0</h1>
 <ul class="simple">
 <li>Enforce <tt class="docutils literal">UNIQUE(menu_id)</tt> on <tt class="docutils literal">spp.hide.menu</tt>: a second
@@ -495,7 +513,7 @@ <h1>19.0.2.1.0</h1>
 Target the existing record or drop the seed.</li>
 </ul>
 </div>
-<div class="section" id="section-2">
+<div class="section" id="section-3">
 <h1>19.0.2.0.1</h1>
 <ul class="simple">
 <li>Keep hidden menus hidden after a module upgrade resets their
@@ -505,7 +523,7 @@ <h1>19.0.2.0.1</h1>
 <tt class="docutils literal">next()</tt>.</li>
 </ul>
 </div>
-<div class="section" id="section-3">
+<div class="section" id="section-4">
 <h1>19.0.2.0.0</h1>
 <ul class="simple">
 <li>Initial migration to OpenSPP2</li>

spp_hide_menus_base/tests/__init__.py

@@ -1,3 +1,4 @@
 # Part of OpenSPP. See LICENSE file for full copyright and licensing details.
 from . import test_hide_menu
 from . import test_migration_dedup_hide_menu
+from . import test_register_hook_guard

spp_hide_menus_base/tests/test_register_hook_guard.py

@@ -0,0 +1,175 @@
+# Part of OpenSPP. See LICENSE file for full copyright and licensing details.
+"""Regression tests for the database-error guard around ``hide_menus()``.
+
+``hide_menus()`` runs from ``ir.module.module._register_hook`` at the end of
+every registry load (startup, install, upgrade, worker reload). A
+``psycopg2.Error`` escaping it aborts the registry load, so on a poisoned
+cursor every restart fails until the database is repaired by hand. Hiding a
+menu is best-effort there: a menu left visible is recoverable, an aborted
+registry load is an outage.
+"""
+
+import contextlib
+import functools
+from unittest.mock import patch
+
+import psycopg2
+import psycopg2.errors
+import psycopg2.extensions
+
+from odoo.tests import TransactionCase, tagged
+from odoo.tools import mute_logger
+
+HOOK_LOGGER = "odoo.addons.spp_hide_menus_base.models.ir_module_module"
+MISSING_MENU_XMLID = "spp_hide_menus_base.test_526_menu_that_does_not_exist"
+
+
+def _failing_lookup_for(menu_xml_id, original_lookup):
+    """An ``ir.model.data._xmlid_lookup`` stand-in that aborts the transaction
+    for one xmlid and behaves normally for every other one."""
+
+    @functools.wraps(original_lookup)
+    def _xmlid_lookup(model, xmlid):
+        if xmlid == menu_xml_id:
+            model.env.cr.execute("SELECT 1 FROM spp_table_that_does_not_exist")
+        return original_lookup(model, xmlid)
+
+    return _xmlid_lookup
+
+
+@tagged("post_install", "-at_install")
+class TestHideMenusDatabaseErrorGuard(TransactionCase):
+    def setUp(self):
+        super().setUp()
+        self.IrModule = self.env["ir.module.module"]
+        self.HideMenu = self.env["spp.hide.menu"]
+        self.hide_group = self.HideMenu._hide_group()
+        self.assertTrue(self.hide_group, "precondition: the hide group must exist")
+
+    def _catalog(self, **entries):
+        """Replace MENU_APP with ``{module_name: menu_xml_id}`` for the test."""
+        menu_app = {module: {"menu_xml_id": xml_id} for module, xml_id in entries.items()}
+        return patch.object(type(self.IrModule), "MENU_APP", menu_app)
+
+    def _menu_without_hide_row(self):
+        """A menu no spp.hide.menu row points at yet."""
+        taken = self.HideMenu.search([]).menu_id.ids
+        menu = self.env["ir.ui.menu"].search([("id", "not in", taken)], limit=1)
+        self.assertTrue(menu, "no unconfigured ir.ui.menu left to test against")
+        return menu
+
+    def _menu_with_external_id(self, name):
+        """A menu with no hide configuration yet, reachable through a fresh xmlid."""
+        menu = self._menu_without_hide_row()
+        self.env["ir.model.data"].create(
+            {"module": "spp_hide_menus_base", "name": name, "model": "ir.ui.menu", "res_id": menu.id}
+        )
+        return menu, f"spp_hide_menus_base.{name}"
+
+    def _first_and_last_module(self):
+        """Two module names at opposite ends of the order hide_menus() walks,
+        so a failure planted on the last one is reached after the first one
+        has already done its work."""
+        modules = self.IrModule.search([])
+        self.assertGreater(len(modules), 1)
+        return modules[0].name, modules[-1].name
+
+    def _poison_cursor(self):
+        # Odoo's assertRaises runs its body in a savepoint and rolls it back,
+        # which would un-poison the cursor; suppress keeps the transaction aborted.
+        with mute_logger("odoo.sql_db"), contextlib.suppress(psycopg2.Error):
+            self.env.cr.execute("SELECT 1 FROM spp_table_that_does_not_exist")
+        self.assertEqual(
+            self.env.cr.connection.get_transaction_status(),
+            psycopg2.extensions.TRANSACTION_STATUS_INERROR,
+            "precondition: the transaction must be aborted before the hook runs",
+        )
+
+    def test_01_missing_menu_xmlid_is_skipped(self):
+        """A catalog entry whose menu does not exist neither raises nor warns."""
+        first, _last = self._first_and_last_module()
+        before = self.HideMenu.search([])
+
+        with self._catalog(**{first: MISSING_MENU_XMLID}), self.assertNoLogs(HOOK_LOGGER, level="WARNING"):
+            self.IrModule._register_hook()
+
+        self.assertEqual(self.HideMenu.search([]), before)
+
+    def test_02_database_error_inside_hook_is_logged_and_skipped(self):
+        """A SQL failure while hiding is logged, the registry load continues,
+        and the half-done pass is rolled back as a whole."""
+        first, last = self._first_and_last_module()
+        menu, menu_xml_id = self._menu_with_external_id("test_526_real_menu")
+        groups_before = menu.group_ids
+        self.assertNotIn(self.hide_group, groups_before, "precondition: the menu starts visible")
+        IrModelData = type(self.env["ir.model.data"])
+        failing_lookup = _failing_lookup_for(MISSING_MENU_XMLID, IrModelData._xmlid_lookup)
+
+        # Control: the same catalog entry does hide the menu when nothing fails,
+        # so the "rolled back" assertions below cannot pass by never having run.
+        with self._catalog(**{first: menu_xml_id}):
+            self.IrModule._register_hook()
+        row = self.HideMenu.search([("menu_id", "=", menu.id)])
+        self.assertTrue(row, "control: the catalog entry must create a hide row")
+        self.assertIn(self.hide_group, menu.group_ids, "control: the catalog entry must hide the menu")
+        row.show_menu()
+        row.unlink()
+        self.assertEqual(menu.group_ids, groups_before, "control undone: the menu is visible again")
+
+        with (
+            self._catalog(**{first: menu_xml_id, last: MISSING_MENU_XMLID}),
+            patch.object(IrModelData, "_xmlid_lookup", failing_lookup),
+            mute_logger("odoo.sql_db"),
+            self.assertLogs(HOOK_LOGGER, level="WARNING") as captured,
+        ):
+            self.IrModule._register_hook()
+
+        self.assertTrue(
+            any("menu hiding pass" in message for message in captured.output),
+            f"expected a skipped-hiding warning, got {captured.output}",
+        )
+        # The transaction is still usable: the failure was contained in a savepoint.
+        self.env.cr.execute("SELECT 1")
+        self.assertEqual(self.env.cr.fetchone(), (1,))
+        # ...and the pass is atomic: the menu hidden before the failure is visible again
+        # and its configuration row is gone.
+        self.assertEqual(menu.group_ids, groups_before)
+        self.assertFalse(self.HideMenu.search([("menu_id", "=", menu.id)]))
+
+    def test_03_hook_survives_already_aborted_transaction(self):
+        """The hook must not raise when the cursor is already poisoned before it starts."""
+        self.env.cr.execute("SAVEPOINT test_526_poisoned_cursor")
+        try:
+            self._poison_cursor()
+            # Driven through _register_hook() as the loader does; this relies on
+            # super()._register_hook() issuing no SQL of its own.
+            with mute_logger("odoo.sql_db"), self.assertLogs(HOOK_LOGGER, level="WARNING"):
+                self.IrModule._register_hook()
+        finally:
+            self.env.cr.execute("ROLLBACK TO SAVEPOINT test_526_poisoned_cursor")
+            self.env.cr.execute("RELEASE SAVEPOINT test_526_poisoned_cursor")
+
+    def test_04_callers_pending_writes_are_not_swallowed(self):
+        """A failure flushing the caller's own pending writes is the caller's error.
+
+        The guard covers only the hiding pass. Pending ORM writes queued before
+        it are flushed ahead of the guard, so their failure surfaces where it
+        belongs instead of being logged as a menu-hiding problem.
+        """
+        menu = self._menu_without_hide_row()
+        row = self.HideMenu.create({"menu_id": menu.id, "xml_id": "test.pending"})
+        self.env.cr.execute("SAVEPOINT test_526_pending_write")
+        try:
+            row.write({"xml_id": "test.pending_changed"})  # queued, not flushed yet
+            self._poison_cursor()
+
+            raised = None
+            with mute_logger("odoo.sql_db"), self.assertNoLogs(HOOK_LOGGER, level="WARNING"):
+                try:
+                    self.IrModule.hide_menus()
+                except psycopg2.Error as exc:
+                    raised = exc
+            self.assertIsInstance(raised, psycopg2.errors.InFailedSqlTransaction)
+        finally:
+            self.env.cr.execute("ROLLBACK TO SAVEPOINT test_526_pending_write")
+            self.env.cr.execute("RELEASE SAVEPOINT test_526_pending_write")