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

Migration 52d1bbdd4671 repairs subjects that were stored as nonfiction
because an unresolvable BISAC code fell through the ruleset catch-all.
It only sets checked=false; the re-scoring happens later, in
classify_unchecked_subjects.

That leaves a window. If old code reaches those subjects first -- a
still-running scripts server, or a host that redeploys itself via
watchtower or ECS/Fargate auto_restart -- it re-scores them under the
superseded rules and re-stamps checked=true. The reset is consumed
rather than lost, so nothing errors, nothing retries, and no later run
revisits them. The repair quietly did nothing, having paid for a full
reindex. Raised in review on #3726; it is not preventable from inside
the migration, so make it recoverable instead.

Adds reset_non_bisac_nonfiction_subjects, a Celery task that re-applies
the reset, with ResetNonBisacNonfictionSubjectsScript and
bin/work_reset_non_bisac_nonfiction_subjects to queue it -- the same
three layers as reclassify_null_audience_works, which repairs the
sibling audience defect.

The task resets only. Re-scoring stays with classify_unchecked_subjects,
which picks these subjects up on its next nightly run and can be
triggered immediately through bin/work_classify_unchecked_subjects. One
task, one job, and no large reindex fired the moment someone runs the
repair.

The selection lives on BISACClassifier as contradicts_stored_fiction, so
the migration and the task cannot disagree about which rows are
affected. Both already import the classifier, so this adds no coupling
that was not there, and it puts the definition with the code that owns
the answer. The migration is updated to use it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: dbernstein

alembic/versions/20260902_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py

@@ -63,14 +63,7 @@ def upgrade() -> None:
     # value would survive this repair.
     stale_ids = []
     for row in candidates:
-        if not row.identifier and not row.name:
-            # Nothing to classify. Subject.lookup will not create such a row,
-            # but the columns are nullable, so don't assume.
-            continue
-        identifier, name = BISACClassifier.scrub_identifier_and_name(
-            row.identifier, row.name
-        )
-        if BISACClassifier.is_fiction(identifier, name) is not False:
+        if BISACClassifier.contradicts_stored_fiction(row.identifier, row.name, False):
             stale_ids.append(row.id)
             log.info(
                 f"Reset checked=False for subject id={row.id} "

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,55 @@ 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:
+    """Re-apply the reset that repairs subjects stored as nonfiction in error.
+
+    Migration 52d1bbdd4671 marks these subjects unchecked so that
+    classify_unchecked_subjects re-scores them. That reset can be consumed
+    before it takes effect: if old code reaches the subjects first -- a
+    still-running scripts server, or a host that redeploys itself -- it
+    re-scores them under the superseded rules and re-stamps checked=True.
+    Nothing errors, and nothing revisits them afterwards, so the repair
+    quietly did nothing. This task exists to run the reset again.
+
+    It resets only. The re-scoring stays with classify_unchecked_subjects,
+    which picks these subjects up on its next nightly run; trigger
+    bin/work_classify_unchecked_subjects to have it happen sooner.
+
+    Idempotent: a second run finds nothing to do.
+    """
+    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

@@ -689,6 +689,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 in migration 52d1bbdd4671 can be applied again, in
+    case its reset was consumed by old code before the new classifier was live.
+
+    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.
 

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 re-applies the reset for subjects stored as nonfiction in error.
+
+    Re-runnable stand-in for migration 52d1bbdd4671, for when that migration's
+    reset was consumed by old code before the new classifier was live.
+    """
+    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

@@ -549,6 +549,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