fix(spp_attachment_av_scan): never swallow a database error when queueing a scan

spp_attachment_av_scan/README.rst

@@ -134,6 +134,15 @@ External: ``pyclamd`` (Python library for ClamAV integration)
 Changelog
 =========
 
+19.0.2.0.1
+~~~~~~~~~~
+
+- fix: re-raise database errors (``psycopg2.Error``,
+  ``ConcurrencyError``) from the create/write scan-queue hooks instead
+  of swallowing them, so transient serialization failures reach Odoo's
+  transaction retry machinery instead of poisoning the transaction for
+  unrelated downstream code
+
 19.0.2.0.0
 ~~~~~~~~~~
 

spp_attachment_av_scan/__manifest__.py

@@ -1,7 +1,7 @@
 {  # pylint: disable=pointless-statement
     "name": "OpenSPP Attachment Antivirus Scan",
     "category": "OpenSPP",
-    "version": "19.0.2.0.0",
+    "version": "19.0.2.0.1",
     "sequence": 1,
     "author": "OpenSPP.org",
     "website": "https://github.com/OpenSPP/OpenSPP2",

spp_attachment_av_scan/models/ir_attachment.py

@@ -3,11 +3,29 @@
 import json
 import logging
 
+import psycopg2
+
 from odoo import Command, _, api, fields, models
-from odoo.exceptions import AccessError, UserError
+from odoo.exceptions import AccessError, ConcurrencyError, UserError
 
 _logger = logging.getLogger(__name__)
 
+#: Queueing a malware scan is best-effort — a scan that cannot be enqueued must not
+#: block the attachment write. A **database** error is categorically different: it
+#: leaves the transaction unusable, so swallowing one converts a recoverable fault
+#: into an unrelated failure somewhere downstream.
+#:
+#: This is a superset of the classes ``odoo.service.model.retrying`` recovers from
+#: by rolling back and re-running the request (``IntegrityError``, ``OperationalError``,
+#: ``ConcurrencyError`` — see ``odoo/service/model.py``); the non-retryable rest of
+#: ``psycopg2.Error`` then surfaces with the true traceback instead of poisoning
+#: downstream code. ``SerializationFailure``
+#: resolves through that tuple via
+#: ``SerializationFailure -> TransactionRollbackError -> OperationalError``, so a
+#: concurrent-update conflict on an attachment is retried transparently — *unless*
+#: something catches it first.
+_MUST_NOT_SWALLOW = (psycopg2.Error, ConcurrencyError)
+
 QUARANTINE_PROVIDER_PARAM = "spp_attachment_av_scan.quarantine_encryption_provider_id"
 QUARANTINE_RETENTION_DAYS_PARAM = "spp_attachment_av_scan.quarantine_retention_days"
 DEFAULT_QUARANTINE_RETENTION_DAYS = 90
@@ -92,6 +110,11 @@ def create(self, vals_list):
                         priority=20,
                     )._scan_for_malware()
                     _logger.info("Queued malware scan for attachment ID %s", attachment.id)
+                except _MUST_NOT_SWALLOW:
+                    # Never swallow: see ``_MUST_NOT_SWALLOW``. Re-raise so the
+                    # request is rolled back and retried instead of continuing on a
+                    # dead transaction.
+                    raise
                 except Exception as error:
                     _logger.error(
                         "Failed to queue malware scan for attachment ID %s: %s",
@@ -130,6 +153,14 @@ def write(self, vals):
                             "Queued malware scan for updated attachment ID %s",
                             attachment.id,
                         )
+                    except _MUST_NOT_SWALLOW:
+                        # Never swallow: see ``_MUST_NOT_SWALLOW``. This is the exact
+                        # site that turned a transient "could not serialize access due
+                        # to concurrent update" on an attachment into an
+                        # InFailedSqlTransaction reported from an unrelated menu-icon
+                        # lookup, on every module upgrade, with the real cause visible
+                        # only as a stray ERROR line in the server log.
+                        raise
                     except Exception as error:
                         _logger.error(
                             "Failed to queue malware scan for updated attachment ID %s: %s",

spp_attachment_av_scan/readme/HISTORY.md

@@ -1,3 +1,10 @@
+### 19.0.2.0.1
+
+- fix: re-raise database errors (`psycopg2.Error`, `ConcurrencyError`) from the
+  create/write scan-queue hooks instead of swallowing them, so transient
+  serialization failures reach Odoo's transaction retry machinery instead of
+  poisoning the transaction for unrelated downstream code
+
 ### 19.0.2.0.0
 
 - Initial migration to OpenSPP2

spp_attachment_av_scan/static/description/index.html

@@ -515,6 +515,16 @@ <h2><a class="toc-backref" href="#toc-entry-1">Changelog</a></h2>
 </div>
 </div>
 <div class="section" id="section-1">
+<h1>19.0.2.0.1</h1>
+<ul class="simple">
+<li>fix: re-raise database errors (<tt class="docutils literal">psycopg2.Error</tt>,
+<tt class="docutils literal">ConcurrencyError</tt>) from the create/write scan-queue hooks instead
+of swallowing them, so transient serialization failures reach Odoo’s
+transaction retry machinery instead of poisoning the transaction for
+unrelated downstream code</li>
+</ul>
+</div>
+<div class="section" id="section-2">
 <h1>19.0.2.0.0</h1>
 <ul class="simple">
 <li>Initial migration to OpenSPP2</li>

spp_attachment_av_scan/tests/__init__.py

@@ -1,2 +1,3 @@
 from . import test_av_scanner_backend
 from . import test_ir_attachment
+from . import test_scan_queue_error_handling

spp_attachment_av_scan/tests/test_scan_queue_error_handling.py

@@ -0,0 +1,131 @@
+"""Queueing a malware scan is best-effort; swallowing a DB error is not.
+
+Incident this pins (dev payroll instance, 2026-07-31): a routine attachment write
+during a module upgrade hit a transient
+
+    ERROR: could not serialize access due to concurrent update
+
+on ``ir_attachment``. The ``create``/``write`` hooks in this module caught it with a
+bare ``except Exception``, logged it, and continued. The transaction was already
+unusable, so the next statement to touch the database — an unrelated
+``env.ref("stock.menu_stock_root")`` inside OpenSPP's menu-icon refresh — failed with
+``InFailedSqlTransaction``, and *that* was the only error the operator ever saw.
+Every module upgrade failed identically, with four different modules blamed in turn.
+
+The swallow also defeated the recovery Odoo already provides:
+``odoo.service.model.retrying`` retries a request on ``IntegrityError`` /
+``OperationalError`` / ``ConcurrencyError``, and ``SerializationFailure`` reaches that
+tuple through ``TransactionRollbackError -> OperationalError``. Left alone, the
+conflict would have been retried transparently. Caught, it became a hard failure
+attributed to the wrong subsystem.
+
+So the contract is two-sided, and both sides are asserted here:
+
+* a **database** error must propagate — it is recoverable, but only if it is allowed
+  to reach the retry machinery;
+* a **non-database** error must still be swallowed and logged — enqueueing a scan is
+  genuinely best-effort and must not block the attachment write.
+"""
+
+import base64
+from unittest.mock import patch
+
+import psycopg2
+
+from odoo.exceptions import ConcurrencyError
+from odoo.service import model as service_model
+from odoo.tests import TransactionCase, tagged
+
+LOGGER = "odoo.addons.spp_attachment_av_scan.models.ir_attachment"
+
+
+def _raise_serialization_failure(*args, **kwargs):
+    """The exact error class from the incident."""
+    raise psycopg2.errors.SerializationFailure("could not serialize access due to concurrent update")
+
+
+def _raise_concurrency_error(*args, **kwargs):
+    """The other member of ``_MUST_NOT_SWALLOW`` — Odoo's own concurrency check."""
+    raise ConcurrencyError("write concurrency check failed")
+
+
+def _raise_value_error(*args, **kwargs):
+    """A non-database failure, e.g. a misconfigured queue channel."""
+    raise ValueError("scan queue is misconfigured")
+
+
+@tagged("post_install", "-at_install")
+class TestScanQueueErrorHandling(TransactionCase):
+    @classmethod
+    def setUpClass(cls):
+        super().setUpClass()
+        cls.Attachment = cls.env["ir.attachment"]
+
+    def _binary_vals(self, name="av-guard-probe.txt", payload=b"probe"):
+        return {
+            "name": name,
+            "datas": base64.b64encode(payload),
+            "mimetype": "text/plain",
+        }
+
+    def test_a_database_error_while_queueing_propagates_on_create(self):
+        """``create`` must not swallow a serialization failure."""
+        with patch.object(type(self.Attachment), "with_delay", _raise_serialization_failure):
+            with self.assertRaises(psycopg2.errors.SerializationFailure):
+                self.Attachment.create(self._binary_vals())
+
+    def test_a_database_error_while_queueing_propagates_on_write(self):
+        """``write`` is the site the incident actually went through."""
+        attachment = self.Attachment.create(self._binary_vals(name="av-guard-write.txt"))
+        with patch.object(type(self.Attachment), "with_delay", _raise_serialization_failure):
+            with self.assertRaises(psycopg2.errors.SerializationFailure):
+                attachment.write({"datas": base64.b64encode(b"changed")})
+
+    def test_a_concurrency_error_while_queueing_propagates_on_create(self):
+        """``ConcurrencyError`` is the second member of ``_MUST_NOT_SWALLOW``."""
+        with patch.object(type(self.Attachment), "with_delay", _raise_concurrency_error):
+            with self.assertRaises(ConcurrencyError):
+                self.Attachment.create(self._binary_vals(name="av-guard-concurrency.txt"))
+
+    def test_a_non_database_error_is_still_swallowed_on_create(self):
+        """Anti-vacuity: the fix must not turn best-effort queueing into a hard gate.
+
+        Re-raising everything would pass both tests above while making any queue
+        misconfiguration block attachment creation across the platform.
+        """
+        with patch.object(type(self.Attachment), "with_delay", _raise_value_error):
+            with self.assertLogs(LOGGER, "ERROR") as logs:
+                attachment = self.Attachment.create(self._binary_vals(name="av-guard-nondb.txt"))
+        self.assertTrue(attachment.exists(), "the attachment must still be created")
+        self.assertIn("Failed to queue malware scan", logs.output[0])
+
+    def test_a_non_database_error_is_still_swallowed_on_write(self):
+        attachment = self.Attachment.create(self._binary_vals(name="av-guard-nondb-write.txt"))
+        with patch.object(type(self.Attachment), "with_delay", _raise_value_error):
+            with self.assertLogs(LOGGER, "ERROR") as logs:
+                attachment.write({"datas": base64.b64encode(b"changed")})
+        self.assertIn("Failed to queue malware scan", logs.output[0])
+
+    def test_the_retry_machinery_can_see_the_error_class_we_re_raise(self):
+        """Guards the *reason* re-raising works, not just that it happens.
+
+        If a future refactor re-raised some wrapped exception instead, the request
+        would no longer be retried and the incident would recur in a new disguise.
+        ``odoo/service/model.py`` retries on ``(IntegrityError, OperationalError,
+        ConcurrencyError)``, then only re-runs the request when the exception is in
+        ``PG_CONCURRENCY_EXCEPTIONS_TO_RETRY``.
+        """
+        self.assertTrue(
+            issubclass(
+                psycopg2.errors.SerializationFailure,
+                (psycopg2.IntegrityError, psycopg2.OperationalError),
+            ),
+            "SerializationFailure must remain catchable by service.model.retrying",
+        )
+        self.assertTrue(
+            issubclass(
+                psycopg2.errors.SerializationFailure,
+                service_model.PG_CONCURRENCY_EXCEPTIONS_TO_RETRY,
+            ),
+            "SerializationFailure must remain in Odoo's retry-eligible set (PG_CONCURRENCY_EXCEPTIONS_TO_RETRY)",
+        )