Fix embedart: retry pHash compare without colorspaces on empty IM 7 output

beets/dbcore/db.py

@@ -586,10 +586,19 @@ def store(self, fields: Iterable[str] | None = None) -> None:
         if fields is None:
             fields = self._fields
 
+        # Separate the requested fields into fixed (real columns in the main
+        # table) and flexible (persisted via ``_flex_table``). Callers such as
+        # ``beet update`` may pass a flex attribute through ``fields=`` -- for
+        # example, when a plugin registers it via both ``item_types`` and
+        # ``add_media_field``. Treating a flex attribute as a column here used
+        # to build ``UPDATE <table> SET <flex>=?`` and crash with
+        # ``sqlite3.OperationalError: no such column: <flex>`` (see #5580).
+        column_fields = [f for f in fields if f in self._fields]
+
         # Build assignments for query.
         assignments = []
         subvars: list[SQLiteType] = []
-        for key in fields:
+        for key in column_fields:
             if key != "id" and key in self._dirty:
                 self._dirty.remove(key)
                 assignments.append(f"{key}=?")

beets/util/artresizer.py

@@ -11,6 +11,7 @@
 import subprocess
 from abc import ABC, abstractmethod
 from contextlib import suppress
+from dataclasses import dataclass
 from enum import Enum
 from itertools import chain
 from typing import TYPE_CHECKING, Any, ClassVar
@@ -25,7 +26,7 @@
 )
 
 if TYPE_CHECKING:
-    from collections.abc import Mapping
+    from collections.abc import Mapping, Sequence
 
 PROXY_URL = "https://images.weserv.nl/"
 
@@ -48,6 +49,21 @@ class LocalBackendNotAvailableError(Exception):
     pass
 
 
+@dataclass
+class CompareResult:
+    """Outcome of a single PHASH compare pipeline run.
+
+    `score` is None when no parseable PHASH score was produced. The
+    `empty_output` flag distinguishes the #6348 case (ImageMagick ≥7.1.1-44
+    emits empty stdout+stderr with the `phash:colorspaces` define) from a
+    genuine parse error or subprocess failure — only the empty-output
+    case warrants a retry without the define.
+    """
+
+    score: float | None
+    empty_output: bool = False
+
+
 # Singleton pattern that the typechecker understands:
 # https://peps.python.org/pep-0484/#support-for-singleton-types-in-unions
 class NotAvailable(Enum):
@@ -373,6 +389,15 @@ def compare(
             "gray",
             "MIFF:-",
         ]
+
+        # The `phash:colorspaces=sRGB,HCLp` define was added in a873a191b
+        # to make PHASH scores deterministic. ImageMagick ≥7.1.1-44
+        # sometimes emits empty stdout+stderr when this define is passed
+        # (see ImageMagick/ImageMagick#5191), which beets interpreted as
+        # a parse error and surfaced to users as "Error while checking
+        # art similarity; skipping" on every track (#6348). We try the
+        # deterministic pipeline first; on empty output we retry without
+        # the define, trading determinism for a usable score.
         compare_cmd = [
             *self.compare_cmd,
             "-define",
@@ -382,6 +407,76 @@ def compare(
             "-",
             "null:",
         ]
+        result = self._run_compare_pipeline(
+            convert_cmd, compare_cmd, im1, im2, is_windows
+        )
+        if result.empty_output:
+            log.debug(
+                "ImageMagick produced no output with phash:colorspaces "
+                "define; retrying without it (see #6348)"
+            )
+            retry_compare_cmd = [
+                *self.compare_cmd,
+                "-metric",
+                "PHASH",
+                "-",
+                "null:",
+            ]
+            result = self._run_compare_pipeline(
+                convert_cmd, retry_compare_cmd, im1, im2, is_windows
+            )
+
+        if result.score is None:
+            return None
+
+        log.debug("ImageMagick compare score: {}", result.score)
+        return result.score <= compare_threshold
+
+    @staticmethod
+    def _parse_compare_output(
+        returncode: int, stdout: bytes, stderr: bytes
+    ) -> float | None:
+        """Parse the output of `compare -metric PHASH` into a score.
+
+        ImageMagick writes the score to stdout when the images are
+        identical (exit 0) and to stderr when they differ (exit 1). Any
+        other exit code, or output that is not parseable as a float
+        after the IM 7.1.1-44 "(diff)" paren extraction, returns None.
+        """
+        if returncode:
+            if returncode != 1:
+                return None
+            out_str = stderr
+        else:
+            out_str = stdout
+
+        # ImageMagick 7.1.1-44 outputs in a different format.
+        if b"(" in out_str and out_str.endswith(b")"):
+            # Extract diff from "... (diff)".
+            out_str = out_str[out_str.index(b"(") + 1 : -1]
+
+        try:
+            return float(out_str)
+        except ValueError:
+            log.debug("IM output is not a number: {0!r}", out_str)
+            return None
+
+    def _run_compare_pipeline(
+        self,
+        convert_cmd: Sequence[bytes | str],
+        compare_cmd: Sequence[bytes | str],
+        im1: bytes,
+        im2: bytes,
+        is_windows: bool,
+    ) -> CompareResult:
+        """Run the convert | compare pipeline once.
+
+        Returns a `CompareResult` whose `score` is None when no parseable
+        PHASH score was produced, and whose `empty_output` flag indicates
+        whether the compare subprocess itself ran cleanly (exit 0 or 1)
+        but produced no usable output — the #6348 case, which warrants a
+        retry without the colorspaces define.
+        """
         log.debug(
             "comparing images with pipeline {} | {}", convert_cmd, compare_cmd
         )
@@ -415,35 +510,32 @@ def compare(
                 convert_proc,
                 convert_stderr,
             )
-            return None
+            return CompareResult(None, empty_output=False)
 
         # Check the compare output.
         stdout, stderr = compare_proc.communicate()
-        if compare_proc.returncode:
-            if compare_proc.returncode != 1:
-                log.debug(
-                    "ImageMagick compare failed: {}, {}",
-                    displayable_path(im2),
-                    displayable_path(im1),
-                )
-                return None
-            out_str = stderr
-        else:
-            out_str = stdout
-
-        # ImageMagick 7.1.1-44 outputs in a different format.
-        if b"(" in out_str and out_str.endswith(b")"):
-            # Extract diff from "... (diff)".
-            out_str = out_str[out_str.index(b"(") + 1 : -1]
-
-        try:
-            phash_diff = float(out_str)
-        except ValueError:
-            log.debug("IM output is not a number: {0!r}", out_str)
-            return None
+        if compare_proc.returncode and compare_proc.returncode != 1:
+            log.debug(
+                "ImageMagick compare failed: {}, {}",
+                displayable_path(im2),
+                displayable_path(im1),
+            )
+            return CompareResult(None, empty_output=False)
 
-        log.debug("ImageMagick compare score: {}", phash_diff)
-        return phash_diff <= compare_threshold
+        score = self._parse_compare_output(
+            compare_proc.returncode, stdout, stderr
+        )
+        # Detect the #6348 case: compare exited 0/1 but emitted no
+        # parseable bytes on the stream we read. We only treat the case
+        # where BOTH streams are empty as retry-worthy — non-empty but
+        # unparseable output is a genuine parse error.
+        empty_output = (
+            score is None
+            and compare_proc.returncode in (0, 1)
+            and not stdout.strip()
+            and not stderr.strip()
+        )
+        return CompareResult(score, empty_output=empty_output)
 
     @property
     def can_write_metadata(self) -> bool:

docs/changelog.rst

@@ -58,6 +58,19 @@ New features
 Bug fixes
 ~~~~~~~~~
 
+- :doc:`/plugins/embedart`: Fixed "Error while checking art similarity;
+  skipping" being logged on every track on ImageMagick ≥7.1.1-44. When the
+  ``phash:colorspaces=sRGB,HCLp`` define (added for score determinism) makes
+  ``magick compare -metric PHASH`` emit empty stdout+stderr, beets now retries
+  the compare without the define, trading determinism for a usable score.
+  :bug:`6348`
+- Fixed a ``sqlite3.OperationalError: no such column`` crash in ``Model.store``
+  when a field passed via the ``fields`` argument was a flexible attribute
+  rather than a fixed column. The fields set is now filtered to fixed columns
+  for the main-table ``UPDATE``; flexible attributes continue to be persisted
+  via the ``_flex_table`` path. This affected plugins that register fields via
+  both ``item_types`` and ``add_media_field`` and were later updated with ``beet
+  update``. :bug:`5580`
 - :doc:`plugins/edit`: Preserve missing album art paths when editing album
   metadata, instead of turning ``artpath: null`` into a path ending in ``None``.
   :bug:`2438`

test/dbcore/test_db.py

@@ -281,6 +281,27 @@ def test_store_and_retrieve_flexattr(self):
         other_model = self.db._get(ModelFixture1, model.id)
         assert other_model.foo == "bar"
 
+    def test_store_with_flex_field_in_fields_argument(self):
+        """A flex attribute passed via ``fields=`` must not crash.
+
+        Regression test for #5580: when a plugin registers a typed flex
+        attribute (via ``item_types``) and a caller such as ``beet update``
+        passes its name through ``fields=`` to ``Model.store()``, the store
+        loop used to build ``UPDATE <table> SET <flex_field>=?`` and crash
+        with ``sqlite3.OperationalError: no such column: <flex_field>``.
+        Flex attributes have no column in the main table; they live in the
+        ``_flex_table`` and must be persisted via the INSERT path.
+        """
+        model = ModelFixture1()
+        model.add(self.db)
+        model.field_one = 42
+        model.some_float_field = 1.5
+        model.store(fields=["field_one", "some_float_field"])
+
+        other_model = self.db._get(ModelFixture1, model.id)
+        assert other_model.field_one == 42
+        assert other_model.some_float_field == 1.5
+
     def test_delete_flexattr(self):
         model = ModelFixture1()
         model["foo"] = "bar"

test/plugins/test_embedart.py

@@ -349,14 +349,32 @@ def _mock_popens(
         compare_stdout=b"",
         compare_stderr=b"",
         convert_status=0,
+        # When the first compare call produces empty/unparseable output,
+        # compare() retries without the phash:colorspaces define. These
+        # args configure that retry call (used by the #6348 regression
+        # tests). If None, no retry is mocked (legacy behavior).
+        retry_status=None,
+        retry_stdout=b"",
+        retry_stderr=b"",
+        retry_convert_status=0,
     ):
         mock_extract.return_value = b"extracted_path"
-        mock_subprocess.Popen.side_effect = [
+        popens = [
             # The `convert` call.
             self._popen(convert_status),
             # The `compare` call.
             self._popen(compare_status, compare_stdout, compare_stderr),
         ]
+        if retry_status is not None:
+            # The retry path runs a second convert + compare (without the
+            # phash:colorspaces define).
+            popens.extend(
+                [
+                    self._popen(retry_convert_status),
+                    self._popen(retry_status, retry_stdout, retry_stderr),
+                ]
+            )
+        mock_subprocess.Popen.side_effect = popens
 
     def test_compare_success_similar(self, mock_extract, mock_subprocess):
         self._mock_popens(mock_extract, mock_subprocess, 0, b"10", b"err")
@@ -388,6 +406,59 @@ def test_compare_parsing_error_and_failure(
         self._mock_popens(mock_extract, mock_subprocess, 1, b"foo", b"bar")
         assert self._similarity(20) is None
 
+    def test_compare_empty_output_triggers_retry(
+        self, mock_extract, mock_subprocess
+    ):
+        """Reproduces #6348: ImageMagick ≥7.1.1-44 sometimes emits empty
+        stdout+stderr when invoked with the `phash:colorspaces=sRGB,HCLp`
+        define. compare() should retry the pipeline without the define
+        rather than returning None silently.
+        """
+        # First call: empty stdout+stderr (the #6348 regression).
+        # Retry: a parseable score on stdout.
+        self._mock_popens(
+            mock_extract,
+            mock_subprocess,
+            compare_status=0,
+            compare_stdout=b"",
+            compare_stderr=b"",
+            retry_status=0,
+            retry_stdout=b"10",
+            retry_stderr=b"",
+        )
+        # Score 10 vs threshold 20 → similar (within threshold).
+        assert self._similarity(20)
+        # And the threshold comparison actually fires in the other direction.
+        self._mock_popens(
+            mock_extract,
+            mock_subprocess,
+            compare_status=0,
+            compare_stdout=b"",
+            compare_stderr=b"",
+            retry_status=0,
+            retry_stdout=b"10",
+            retry_stderr=b"",
+        )
+        assert not self._similarity(5)
+
+    def test_compare_empty_output_retry_also_fails(
+        self, mock_extract, mock_subprocess
+    ):
+        """If both the define-based and the no-define retries produce no
+        parseable output, compare() returns None (same as a parse error).
+        """
+        self._mock_popens(
+            mock_extract,
+            mock_subprocess,
+            compare_status=0,
+            compare_stdout=b"",
+            compare_stderr=b"",
+            retry_status=0,
+            retry_stdout=b"",
+            retry_stderr=b"",
+        )
+        assert self._similarity(20) is None
+
     def test_convert_failure(self, mock_extract, mock_subprocess):
         self._mock_popens(mock_extract, mock_subprocess, convert_status=1)
         assert self._similarity(20) is None