Don't let unrecognized BISAC codes classify fiction as nonfiction (PP-4849)

bin/work_reset_non_bisac_nonfiction_subjects

@@ -0,0 +1,11 @@
+#!/usr/bin/env python
+"""Queue the reset_non_bisac_nonfiction_subjects Celery task.
+
+Convenience wrapper that manually dispatches the repair for BISAC subjects
+stored as nonfiction in error (the `reset_non_bisac_nonfiction_subjects` Celery
+task) for a worker to process.
+"""
+
+from palace.manager.scripts.work import ResetNonBisacNonfictionSubjectsScript
+
+ResetNonBisacNonfictionSubjectsScript().run()

src/palace/manager/celery/tasks/work.py

@@ -4,6 +4,7 @@
 from sqlalchemy.orm import Session
 
 from palace.manager.celery.task import Task
+from palace.manager.core.classifier.bisac import BISACClassifier
 from palace.manager.data_layer.policy.presentation import PresentationCalculationPolicy
 from palace.manager.service.celery.celery import QueueNames
 from palace.manager.sqlalchemy.model.classification import Classification, Subject
@@ -39,6 +40,54 @@ def reclassify_null_audience_works(task: Task) -> None:
             session.commit()
 
 
+@shared_task(queue=QueueNames.default, bind=True)
+def reset_non_bisac_nonfiction_subjects(task: Task) -> None:
+    """Mark BISAC subjects unchecked when their stored fiction status went stale.
+
+    A code that cannot be resolved to a canonical BISAC heading used to be
+    read as nonfiction by the ruleset catch-all, so those subjects carry a
+    fabricated fiction=False. Subjects are only re-examined when checked is
+    false, so repairing them means resetting that flag.
+
+    This resets only. Re-scoring is classify_unchecked_subjects' job: the
+    startup task that runs this at deploy chains the two together, and the
+    nightly run picks up anything left over.
+
+    Idempotent and self-selecting -- it recomputes which subjects the
+    classifier no longer agrees with, so a second run finds nothing to do.
+    That is what lets a later release re-apply it safely.
+    """
+    with task.session() as session:
+        candidates = (
+            session.query(Subject.id, Subject.identifier, Subject.name)
+            .filter(
+                Subject.type == Subject.BISAC,
+                Subject.checked == True,  # noqa: E712
+                Subject.fiction == False,  # noqa: E712
+            )
+            .all()
+        )
+
+        stale_ids = [
+            row.id
+            for row in candidates
+            if BISACClassifier.contradicts_stored_fiction(
+                row.identifier, row.name, False
+            )
+        ]
+
+        if stale_ids:
+            session.query(Subject).filter(Subject.id.in_(stale_ids)).update(
+                {Subject.checked: False}, synchronize_session=False
+            )
+            session.commit()
+
+        task.log.info(
+            f"Reset checked=False for {len(stale_ids)} of {len(candidates)} "
+            f"BISAC subjects stored as nonfiction."
+        )
+
+
 @shared_task(queue=QueueNames.default, bind=True)
 def classify_unchecked_subjects(task: Task) -> None:
     """Reclassify all Works whose current classifications appear to

src/palace/manager/core/classifier/bisac.py

@@ -1,5 +1,6 @@
 import csv
 import re
+from collections.abc import Callable, Sequence
 
 from frozendict import frozendict
 
@@ -654,27 +655,130 @@ class BISACClassifier(Classifier):
         m(classifier.Life_Strategies, nonfiction, social_topics),
     ]
 
+    # The top-level headings a canonical BISAC name can begin with ("Fiction",
+    # "Juvenile Nonfiction", "Antiques & Collectibles", ...).
+    TOP_LEVEL_HEADINGS: frozenset[str] = frozenset(
+        Lowercased(name.split("/")[0].strip()) for name in NAMES.values()
+    ) | frozenset(
+        # Former top-level spellings of renamed categories, which distributors
+        # still send and bisac.csv no longer lists. They belong here so that a
+        # name beginning with one still reaches the catch-all rules: without
+        # the entry, a deprecated spelling carried by a code that does not
+        # resolve abstains instead of being read as nonfiction/Adult. That is
+        # the test for whether a new entry belongs -- not whether the rulesets
+        # have an Interchangeable for it, which is a separate mechanism in
+        # GENRE, and GENRE does not consult this set.
+        Lowercased(name)
+        for name in (
+            "Mind & Spirit",
+            "Psychology & Psychiatry",
+            "Technology",
+            "Foreign Language Study",
+            "Literary Criticism & Collections",
+        )
+    )
+
+    @classmethod
+    def _has_canonical_heading(cls, name: list[str]) -> bool:
+        """Does `name` begin with a real BISAC top-level heading?
+
+        This is the premise the FICTION and AUDIENCE catch-all rules rest on,
+        so it is what has to be checked before running them. It holds for a
+        name that came from `NAMES`, and for a heading a distributor supplied
+        in the identifier field -- an OPDS `<category term="FICTION / Horror">`
+        with no `label` arrives that way, since the OPDS1 extractor maps `term`
+        to the identifier and `label` to the name. A bare top-level heading
+        such as "Juvenile" is equally a heading.
+
+        It does not hold for the fragment left over when a code cannot be
+        resolved ("Historical", "English literature"), which is the case these
+        rulesets must not be applied to.
+        """
+        return bool(name) and name[0] in cls.TOP_LEVEL_HEADINGS
+
+    @classmethod
+    def contradicts_stored_fiction(
+        cls,
+        identifier: str | None,
+        name: str | None,
+        stored_fiction: bool | None,
+    ) -> bool:
+        """Does this classifier disagree with a subject's stored fiction status?
+
+        Subjects are only re-examined when `checked` is false, so a value
+        scored under superseded rules persists indefinitely. Repairs that
+        reset `checked` need to identify those rows, and they need to agree
+        with each other about which rows they are. Expressing the question
+        here keeps that definition in one place: a subject is stale when the
+        classifier, run now, does not return what is stored.
+
+        :param identifier: The subject's identifier, as stored.
+        :param name: The subject's name, as stored.
+        :param stored_fiction: The subject's current `fiction` value.
+        :return: True when the classifier no longer agrees with `stored_fiction`.
+        """
+        if not identifier and not name:
+            # Nothing to classify. Subject.lookup will not create such a row,
+            # but both columns are nullable, so do not assume.
+            return False
+        scrubbed_identifier, scrubbed_name = cls.scrub_identifier_and_name(
+            identifier, name
+        )
+        return cls.is_fiction(scrubbed_identifier, scrubbed_name) is not stored_fiction
+
+    @classmethod
+    def _apply_rulesets[RulesetResult](
+        cls,
+        identifier: str | None,
+        name: list[str],
+        rulesets: Sequence[MatchingRule],
+        keyword_fallback: Callable[[str | None, str], RulesetResult | None],
+    ) -> RulesetResult | None:
+        """Match `name` against `rulesets`, falling back to keyword matching.
+
+        Both the FICTION and AUDIENCE rulesets end in a catch-all that reasons
+        from the top-level BISAC heading -- "not filed under Fiction, therefore
+        nonfiction", "no juvenile heading, therefore Adult". That inference
+        holds for a canonical BISAC name and for nothing else, so an
+        unrecognized code skips the rulesets entirely and is left to the keyword
+        classifier, which abstains when the distributor's name carries no
+        signal.
+
+        Only those two callers share this. `genre` must not skip its rulesets:
+        GENRE has no catch-all but does have rules that match a bare fragment,
+        so a subject named "Historical" is still a Historical Fiction signal
+        even though "Historical" is not a top-level heading. `target_age` is
+        left out for scope rather than correctness -- its rules key off a
+        juvenile first token, which only a real heading produces, so routing it
+        through here would be safe.
+        """
+        # A subject with no identifier had no code that could fail to resolve,
+        # so there is nothing here to protect it from -- and some distributors
+        # classify entirely this way. Bibliotheca sends every genre as a bare
+        # name ("Action & Adventure", "Magic") with no code at all; gating
+        # those on the heading would leave its titles with no fiction evidence
+        # whatsoever. Whether a bare sub-heading should carry the rulesets'
+        # top-level inference is a real question, but a separate one.
+        if not identifier or cls._has_canonical_heading(name):
+            for ruleset in rulesets:
+                result = ruleset.match(*name)
+                if result is cls.stop:
+                    return None
+                if result is not None:
+                    return result
+        return keyword_fallback(identifier, "/".join(name))
+
     @classmethod
     def is_fiction(cls, identifier, name):
-        for ruleset in cls.FICTION:
-            fiction = ruleset.match(*name)
-            if fiction is cls.stop:
-                return None
-            if fiction is not None:
-                return fiction
-        keyword = "/".join(name)
-        return KeywordBasedClassifier.is_fiction(identifier, keyword)
+        return cls._apply_rulesets(
+            identifier, name, cls.FICTION, KeywordBasedClassifier.is_fiction
+        )
 
     @classmethod
     def audience(cls, identifier, name):
-        for ruleset in cls.AUDIENCE:
-            audience = ruleset.match(*name)
-            if audience is cls.stop:
-                return None
-            if audience is not None:
-                return audience
-        keyword = "/".join(name)
-        return KeywordBasedClassifier.audience(identifier, keyword)
+        return cls._apply_rulesets(
+            identifier, name, cls.AUDIENCE, KeywordBasedClassifier.audience
+        )
 
     @classmethod
     def target_age(cls, identifier, name):
@@ -725,10 +829,14 @@ def scrub_identifier(cls, identifier):
         identifier = identifier.removeprefix("FB")
         # Some distributors (e.g. Palace Marketplace) append an "N" suffix to
         # standard BISAC codes (e.g. "FBJUV000000N" becomes "JUV000000N" after
-        # FB-stripping). Official BISAC codes always end with digits, so a
-        # trailing "N" is always a non-standard extension; strip it so the code
-        # resolves to its canonical entry.
-        identifier = identifier.removesuffix("N")
+        # FB-stripping). Strip it only when doing so produces a code we know,
+        # because the identifier field does not always hold a code: a heading
+        # can arrive there too, and stripping unconditionally would turn
+        # "FICTION" into "FICTIO" (likewise RELIGION, EDUCATION, DESIGN,
+        # TRANSPORTATION) and stop it being recognized as a heading.
+        stripped = identifier.removesuffix("N")
+        if stripped in cls.NAMES or stripped in cls.NON_STANDARD_CODE_ALIASES:
+            identifier = stripped
         # Remap any remaining non-standard codes to their canonical equivalents.
         identifier = cls.NON_STANDARD_CODE_ALIASES.get(identifier, identifier)
         if identifier in cls.NAMES:

src/palace/manager/scripts/work.py

@@ -11,6 +11,7 @@
 from palace.manager.celery.tasks.work import (
     classify_unchecked_subjects,
     reclassify_null_audience_works,
+    reset_non_bisac_nonfiction_subjects,
 )
 from palace.manager.data_layer.policy.presentation import (
     PresentationCalculationPolicy,
@@ -252,6 +253,25 @@ class WorkOPDSScript(WorkPresentationScript):
     )
 
 
+class ResetNonBisacNonfictionSubjectsScript(Script):
+    """Manually dispatch the ``reset_non_bisac_nonfiction_subjects`` Celery task.
+
+    The work itself happens in the Celery task; this script just queues it. It
+    exists so the repair can be applied again on demand, in case its reset was
+    consumed by old code before the new classifier was live everywhere.
+
+    TODO: Remove this script when the ``reset_non_bisac_nonfiction_subjects``
+    Celery task is removed.
+    """
+
+    def do_run(self, *args: Any, **kwargs: Any) -> None:
+        reset_non_bisac_nonfiction_subjects.delay()
+        self.log.info(
+            'The "reset_non_bisac_nonfiction_subjects" task has been queued for '
+            "execution. See the celery logs for details about task execution."
+        )
+
+
 class ReclassifyNullAudienceWorksScript(Script):
     """Manually dispatch the ``reclassify_null_audience_works`` Celery task.
 

src/palace/manager/sqlalchemy/model/work.py

@@ -1368,6 +1368,16 @@ def calculate_quality(self, identifier_ids, default_quality=0):
         if new_quality != self.quality:
             self.quality = new_quality
 
+    @staticmethod
+    def _keep_known_value[T](old_value: T | None, new_value: T | None) -> T | None:
+        """Prefer `new_value`, but never replace a known value with None.
+
+        Handed to the classifier as its default, this leaves an existing
+        determination alone when a recalculation gathers no usable
+        classifications, rather than erasing it.
+        """
+        return old_value if new_value is None else new_value
+
     def assign_genres(
         self,
         identifier_ids,
@@ -1392,8 +1402,16 @@ def assign_genres(
         for classification in classifications:
             classifier.add(classification)
 
+        # Hand the retained values to the classifier as its defaults rather
+        # than restoring them after it has run. classify() derives the genres
+        # from the fiction status and the target age from the audience, so a
+        # value restored afterwards arrives too late to be consulted: the genre
+        # filter would already have run with None -- which disables it, keeping
+        # genres of either fiction status -- and we would then stamp the
+        # retained status back onto a work whose own genres now contradict it.
         (genre_weights, new_fiction, new_audience, target_age) = classifier.classify(
-            default_fiction=default_fiction, default_audience=default_audience
+            default_fiction=self._keep_known_value(old_fiction, default_fiction),
+            default_audience=self._keep_known_value(old_audience, default_audience),
         )
 
         new_target_age = tuple_to_numericrange(target_age)
@@ -1402,12 +1420,7 @@ def assign_genres(
 
         if new_fiction != old_fiction:
             self.fiction = new_fiction
-        # Never let a recalculation erase a known audience. If the classifier
-        # came back with no audience (e.g. it gathered no usable
-        # classifications on this pass), keep whatever we already had rather
-        # than writing NULL over a previously-determined audience.
-        if new_audience is None and old_audience is not None:
-            new_audience = old_audience
+
         if new_audience != old_audience:
             self.audience = new_audience
 

startup_tasks/2026_09_14_reclassify_non_bisac_nonfiction_subjects.py

@@ -0,0 +1,54 @@
+"""Repair BISAC subjects stored as nonfiction because their code did not resolve.
+
+Everything on the Palace Marketplace / Feedbooks category scheme is stored with
+``type='BISAC'``, including codes that are not BISAC at all -- language and
+territory categories such as ``INFEN000`` ("English literature"). Those cannot
+be resolved to a canonical heading, so classification used to infer nonfiction
+from the distributor's name and store ``fiction=False``. The classifier no
+longer does that, which leaves the stored values stale.
+
+Subjects are only re-examined when ``checked`` is false, so this dispatches two
+steps: ``reset_non_bisac_nonfiction_subjects`` marks the stale ones unchecked,
+then ``classify_unchecked_subjects`` re-scores them and recalculates their
+works. The second signature is immutable so the chain does not pass the first
+task's return value into it.
+
+Doing both here matters. The reset on its own is exposed: anything reaching
+``Subject.assign_to_genre`` before the re-score consumes it, and code running
+the superseded rules re-stamps ``checked=True`` with the same wrong value.
+Nothing errors and nothing revisits the subject afterwards, so the repair
+silently did nothing, having paid for a reindex to do it. Chaining the re-score
+closes that gap to seconds rather than waiting for the nightly run.
+
+The timing works out. ``helpers/migrate.yml`` stops the scripts container --
+where every Celery worker and beat run -- before migrating, and starts it again
+from the new image afterwards, so the worker that picks this up is necessarily
+new code.
+
+Web containers are the remaining exposure: the deploy recycles them after the
+migration step, and they can reach ``assign_to_genre`` through a presentation
+recalculation. Fargate deployments are not governed by that playbook at all. A
+second startup task re-applies the reset a release later, once no old code is
+running anywhere.
+
+TODO: Remove this task once it has run on all deployments (PP-5129)."""
+
+from __future__ import annotations
+
+import logging
+
+from celery.canvas import Signature, chain
+from sqlalchemy.orm import Session
+
+from palace.manager.celery.tasks.work import (
+    classify_unchecked_subjects,
+    reset_non_bisac_nonfiction_subjects,
+)
+from palace.manager.service.container import Services
+
+
+def run(services: Services, session: Session, log: logging.Logger) -> Signature | None:
+    return chain(
+        reset_non_bisac_nonfiction_subjects.s(),
+        classify_unchecked_subjects.si(),
+    )