Make the non-BISAC nonfiction reset re-runnable (PP-5129)

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

@@ -696,6 +696,36 @@ def _has_canonical_heading(cls, name: list[str]) -> bool:
         """
         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,

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.
 

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(),
+    )

tests/manager/celery/tasks/test_work.py

@@ -119,3 +119,64 @@ def test_reclassify_null_audience_works(
         policy = call_obj[1]["policy"]
         assert policy.classify is True
         assert policy.choose_edition is False
+
+
+def test_reset_non_bisac_nonfiction_subjects(
+    db: DatabaseTransactionFixture,
+    celery_fixture: CeleryFixture,
+):
+    """The task resets subjects whose stored nonfiction status went stale.
+
+    Re-runnable, so the repair can be applied again when its reset was consumed
+    by old code before the new classifier was live everywhere.
+    """
+    stale = db.subject(Subject.BISAC, "INFEN000")
+    stale.name = "English literature"
+    stale.fiction = False
+    stale.checked = True
+
+    # A real nonfiction code the classifier still agrees with.
+    agrees = db.subject(Subject.BISAC, "HIS027000")
+    agrees.fiction = False
+    agrees.checked = True
+
+    # Already scored as fiction, so outside the set the task examines.
+    scored_fiction = db.subject(Subject.BISAC, "INFENUSA")
+    scored_fiction.name = "American and Canadian literature"
+    scored_fiction.fiction = True
+    scored_fiction.checked = True
+
+    # Same identifier, but not a BISAC subject.
+    other_type = db.subject(Subject.TAG, "INFEN000")
+    other_type.fiction = False
+    other_type.checked = True
+
+    db.session.commit()
+
+    work_tasks.reset_non_bisac_nonfiction_subjects.delay().wait()
+    db.session.expire_all()
+
+    assert stale.checked is False
+    assert agrees.checked is True
+    assert scored_fiction.checked is True
+    assert other_type.checked is True
+
+
+def test_reset_non_bisac_nonfiction_subjects_is_idempotent(
+    db: DatabaseTransactionFixture,
+    celery_fixture: CeleryFixture,
+):
+    """A second run finds nothing left to do and leaves the reset in place."""
+    subject = db.subject(Subject.BISAC, "INFEN000")
+    subject.name = "English literature"
+    subject.fiction = False
+    subject.checked = True
+    db.session.commit()
+
+    work_tasks.reset_non_bisac_nonfiction_subjects.delay().wait()
+    db.session.expire_all()
+    assert subject.checked is False
+
+    work_tasks.reset_non_bisac_nonfiction_subjects.delay().wait()
+    db.session.expire_all()
+    assert subject.checked is False

tests/manager/core/classifiers/test_bisac.py

@@ -557,6 +557,48 @@ def test_fragments_are_not_top_level_headings(self, fragment: str) -> None:
         """
         assert Lowercased(fragment) not in BISACClassifier.TOP_LEVEL_HEADINGS
 
+    @pytest.mark.parametrize(
+        "identifier,name,stored_fiction,expected",
+        [
+            pytest.param(
+                "INFEN000", "English literature", False, True, id="vendor_code_is_stale"
+            ),
+            pytest.param(
+                "FBZZZ000000", "Historical", False, True, id="unreal_code_is_stale"
+            ),
+            pytest.param(
+                "FBFIC014000",
+                "Historical",
+                False,
+                True,
+                id="real_fiction_code_is_stale",
+            ),
+            pytest.param("HIS027000", None, False, False, id="real_nonfiction_agrees"),
+            pytest.param("FBFIC014000", "Historical", True, False, id="fiction_agrees"),
+            pytest.param(
+                "INFEN000", "English literature", True, False, id="keyword_agrees"
+            ),
+            pytest.param(None, None, False, False, id="nothing_to_classify"),
+        ],
+    )
+    def test_contradicts_stored_fiction(
+        self,
+        identifier: str | None,
+        name: str | None,
+        stored_fiction: bool | None,
+        expected: bool,
+    ) -> None:
+        """The shared definition of a subject whose stored value went stale.
+
+        Subjects are only re-examined when `checked` is false, so the repairs
+        that reset it need one definition of which rows are affected. Both the
+        migration and the re-run task ask this.
+        """
+        assert (
+            BISACClassifier.contradicts_stored_fiction(identifier, name, stored_fiction)
+            is expected
+        )
+
     @pytest.mark.parametrize(
         "identifier,stored_name",
         [

tests/manager/scripts/test_work.py

@@ -10,6 +10,7 @@
 from palace.manager.scripts.work import (
     ReclassifyNullAudienceWorksScript,
     ReclassifyWorksForUncheckedSubjectsScript,
+    ResetNonBisacNonfictionSubjectsScript,
     WorkProcessingScript,
 )
 from palace.manager.sqlalchemy.model.datasource import DataSource
@@ -198,3 +199,13 @@ def test_run(self, db: DatabaseTransactionFixture):
         ) as task:
             ReclassifyNullAudienceWorksScript(db.session).run()
             assert task.delay.call_count == 1
+
+
+class TestResetNonBisacNonfictionSubjectsScript:
+    def test_run(self, db: DatabaseTransactionFixture):
+        """The script queues the reset_non_bisac_nonfiction_subjects Celery task."""
+        with patch(
+            "palace.manager.scripts.work.reset_non_bisac_nonfiction_subjects"
+        ) as task:
+            ResetNonBisacNonfictionSubjectsScript(db.session).run()
+            assert task.delay.call_count == 1