Re-apply the non-BISAC nonfiction reset a release later (PP-5129)

[dbernstein]

Description

One startup task that re-applies the non-BISAC nonfiction reset a release after the original repair, plus the first test coverage of the real startup_tasks/ directory. Two files, 76 lines.

def run(services: Services, session: Session, log: logging.Logger) -> Signature | None:
    return reset_non_bisac_nonfiction_subjects.s()

No migration and no schema change anywhere in this workstream: the reset is a Celery task, reset_non_bisac_nonfiction_subjects, added in #3726 and dispatched by both startup tasks. Hence no DB migration label.

The task key is 2026_10_02_reapply_non_bisac_nonfiction_reset — dated to the merge, not to when the file was written. Keys come from the filename and discovery runs them in sorted order, so the original 2026_09_14_ prefix sorted this ahead of the 2026_09_14_reclassify_... repair it re-applies. Only reachable on a database where both are pending, where there is nothing to repair anyway, but the listing read backwards. Nothing has recorded the old key, so the rename cost nothing.

Motivation and Context

#3726 shipped in v48.0.0, so the release N repair — startup task 2026_09_14_reclassify_non_bisac_nonfiction_subjects, which resets the affected subjects and chains the re-score behind it — has gone out and this is now unblocked.

Repairing a subject means resetting checked=False so it gets re-scored. Anything reaching Subject.assign_to_genre before the re-score lands consumes that reset, and code running the superseded rules re-stamps checked=True with the same wrong value. Nothing errors, nothing retries, and no later run revisits the subject: the repair silently did nothing, having paid for a reindex to do it.

Chaining the re-score shrinks that window to seconds, but does not close it. Deployment ordering below is from the hosting-playbook repo (helpers/migrate.yml); the container layout is from docker/ here.

path	exposed?	why
Celery workers	no	the playbook stops the scripts container before the migrate step and restarts it from the new image afterwards. Every worker and beat runs there — docker/runit-scripts/ holds beat, worker-default, worker-high, worker-apply — so no old worker is alive when the reset is dispatched.
Web containers	yes	only pre-pulled before the migrate step; the rolling recycle happens after it. docker/runit-web/ runs nginx and uwsgi, and the admin work editor reaches classification directly: WorkController.edit and .edit_classifications both call calculate_presentation with a classify=True policy, which runs assign_genres and so Subject.assign_to_genre.
ECS / Fargate	yes	the playbook branches on selectattr('fargate', 'true') and does no docker work for those. Not governed by that file at all.

Running the reset once more a release later closes both, because by then no old code is live anywhere. It needs no deployment-topology argument to hold — which is the point, given that two of those three rows depend on infrastructure detail that can change without anyone touching this repo.

The task is idempotent and self-selecting: it recomputes which subjects the classifier no longer agrees with rather than replaying a stored list. If the release N repair took, this finds nothing and the run is a no-op.

Re-scoring is deliberately left to the nightly classify_unchecked_subjects. The release N task chains it immediately because at that moment the reset is racing old code; here there is none to lose it to, so dispatching would only move a reindex into deploy time for nothing.

This is the second half of a shape the repo has used before. For the sibling null-audience defect, startup task 2026_06_17 re-ran a release later what 2026_05_12 had dispatched; startup_tasks/2026_06_17_repair_remaining_null_audience_works.py is the direct model for this file, down to being a one-line .s() dispatcher.

Cleanup

The docstring carries a TODO naming the full removal surface for PP-5129, to be done once this has run on all deployments: this task, the release N startup task, reset_non_bisac_nonfiction_subjects with its script and bin/ wrapper, BISACClassifier.contradicts_stored_fiction (whose only non-test caller is that task), and the second of the two tests added here.

How Has This Been Tested?

tests/manager/scripts/test_startup.py — 20 passed under tox -e py312-docker, including two new tests. mypy clean; all pre-commit hooks pass.

Nothing previously imported the shipped startup_tasks/ directory. Every test in that file points discover_startup_tasks at tmp_path, and coverage source (pyproject.toml:149) does not include the directory — so a task that failed to import would have shipped green. Verified by appending a syntax error to this task and re-running: the suite stayed green. This PR closes that:

• test_discover_shipped_startup_tasks — discovery over the real STARTUP_TASKS_DIR. Discovery logs and skips a module it cannot import, so asserting only that the results are callable would still pass on a broken task; the test compares the discovered keys against the files on disk. Covers all six tasks, and any added later.
• test_reapply_non_bisac_nonfiction_reset — this task loads from the real directory and returns a signature for reset_non_bisac_nonfiction_subjects, and does not chain the re-score.

Both were confirmed to fail against a deliberately broken copy of the task before being kept.

The task being dispatched is covered in #3726 by test_reset_non_bisac_nonfiction_subjects (7 offender shapes), test_reset_non_bisac_nonfiction_subjects_leaves_everything_else_checked (6 contrast cases) and test_reset_non_bisac_nonfiction_subjects_is_idempotent — the last of which is what makes re-running it here safe.

Checklist

• I have updated the documentation accordingly.
• All new and existing tests passed.

🤖 Generated with Claude Code

[claude]

Claude finished @dbernstein's task in 1m 30s —— View job

---

Summary

The code is correct and I found nothing blocking. run() matches the 2026_06_17 precedent and returns a bare reset_non_bisac_nonfiction_subjects.s(). I checked the claims the change relies on:

• A second run is a no-op. The reset only looks at subjects with checked=True AND fiction=False, then filters them through contradicts_stored_fiction, so it finds nothing once the first repair has worked.
• Re-scoring is covered. classify_unchecked_subjects is in the beat schedule at 22:30, so leaving the re-score to the nightly run is safe.
• Fresh installs are handled. They stamp the task MARKED without running it.
• Ordering is right. The 2026_10_02 key sorts after 2026_09_14.

The new test_discover_shipped_startup_tasks is a good addition. Until now, nothing imported the real startup_tasks/ directory, and discovery logs and skips a module that fails to import, so a broken task file would have passed CI. The test asserts signature.task == reset_non_bisac_nonfiction_subjects.name, so it would also fail if someone added a chain, because a chain's .task is celery.chain. That matches what its docstring promises.

I couldn't run pytest here because the sandbox wouldn't allow it. CI's Test workflow was still running when I posted this.

Details

Minor: tests/manager/scripts/test_startup.py:99-133

The PR description no longer matches the diff. It says "That is the whole diff — one file, 37 lines", "Startup tasks have no per-task tests in this repo; all six rely on that discovery suite", and "18 passed". This push adds two tests to test_startup.py, including one aimed at this task. Please update the description and the "How Has This Been Tested?" section to mention both new tests and the new pass count, so reviewers aren't working from the earlier version.

circulation/tests/manager/scripts/test_startup.py

Lines 99 to 133 in bef4f14

	def test_discover_shipped_startup_tasks(self) -> None:
	"""Every task we actually ship imports and exposes a usable run().
	The other tests here point discovery at `tmp_path`, so nothing else
	imports `startup_tasks/` -- a task that fails to load would ship green.
	Discovery logs and skips a module it cannot import, so compare against
	the files on disk rather than just checking what came back.
	"""
	expected = {
	path.stem
	for path in STARTUP_TASKS_DIR.glob("*.py")
	if not path.stem.startswith("_")
	}
	result = discover_startup_tasks(STARTUP_TASKS_DIR)
	assert expected, f"No startup tasks found in {STARTUP_TASKS_DIR}."
	assert set(result) == expected
	assert all(callable(run) for run in result.values())
	def test_reapply_non_bisac_nonfiction_reset(self) -> None:
	"""The re-apply task queues the reset, and only the reset.
	Unlike the release N task it deliberately does not chain the re-score.
	TODO: Remove with the rest of the repair (PP-5129).
	"""
	run = discover_startup_tasks(STARTUP_TASKS_DIR)[
	"2026_10_02_reapply_non_bisac_nonfiction_reset"
	]
	signature = run(MagicMock(), MagicMock(), logging.getLogger())
	assert isinstance(signature, Signature)
	assert signature.task == reset_non_bisac_nonfiction_subjects.name

[greptile-apps]

Confidence Score: 5/5

[Medium risk] Adds a database repair task to run at startup.

The PR appears safe to merge; the later-release reset has a distinct startup identity and its dispatch contract is directly covered.

Summary

Adds a later-release startup task that re-dispatches the idempotent non-BISAC nonfiction reset after superseded application code is no longer expected to be running.

• Returns only the existing reset_non_bisac_nonfiction_subjects Celery signature, leaving rescoring to the nightly process.
• Adds coverage that imports every shipped startup task and verifies exact discovery parity.
• Adds a task-specific assertion that the new entry point dispatches the intended reset task without chaining a rescore.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Startup[Startup task runner] --> Discover[Discover 2026_10_02 task]
  Discover --> Reset[Queue reset_non_bisac_nonfiction_subjects]
  Reset --> Subjects[Mark affected subjects unchecked]
  Subjects --> Nightly[Nightly classify_unchecked_subjects]
  Nightly --> Rescore[Reclassify and reindex subjects]

Loading

_{Reviews (10) · Last reviewed commit: "Re-date the task key and cover the real ..."}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.75%. Comparing base (c360582) to head (bef4f14).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3737   +/-   ##
=======================================
  Coverage   93.75%   93.75%           
=======================================
  Files         510      510           
  Lines       46477    46477           
  Branches     6312     6312           
=======================================
  Hits        43574    43574           
  Misses       1875     1875           
  Partials     1028     1028           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.