Skip to content

feat(janitor): reap expired pending deletes nightly - #882

Merged
ajslater merged 1 commit into
developfrom
feat/pending-deletes-reaper
Sep 21, 2026
Merged

ajslater merged 1 commit into
developfrom
feat/pending-deletes-reaper

Conversation

@ajslater

Copy link
Copy Markdown
Owner

Implements P1e of tasks/followups-implementation-plan.md §4.8 — the last of the pending-delete mechanism.

Stacked on #881#880#879#878. Retarget as each merges.

Until this lands nothing ever really deletes a stamped row. That is a safe state, but it leaks rows indefinitely.

The window is a constant, not a setting

_PENDING_DELETE_WINDOW: Final = timedelta(hours=24)

A safety delay before an irreversible action, not a tuning knob. The precedent is nearly exact: the telemeter already has a 24-hour grace window whose purpose is "give the admin time to opt out before anything irreversible happens", it is a bare constant (_ONE_DAY), and it is documented to admins in prose while still not being configurable. The nearest neighbour in the delete path is also a commented constant pair (_MASS_DELETE_FLOOR / _MASS_DELETE_FRACTION).

An AdminFlag would cost roughly ten files (enum, title, seed default, data migration, settings/db.py helper, serializer validation, two JSON description files, a v-else-if arm, a Stats field, plus make build-choices) and would be the only configurable knob in the whole delete-safety story.

The effective window is 24h plus the time to the next nightly run, which is exactly what "24 hours or more" meant. Said so in the docstring.

Priority: index 17

Verified in place: UpdateCollectionsTaskJanitorReapPendingDeletesTaskJanitorCleanFKsTask.

  • FK cleanup must follow it. While a stamped comic still exists its Publisher/Imprint/Series/Volume are not orphaned; only after the reap do they become orphans. A reaper at index 24+ leaves emptied groups visible until the next night. cleanup_fks runs a convergence loop, so it cleans the whole chain in the same pass.
  • Search-index sync must follow it, or a night of dead FTS rows survives.
  • Not earlier than 17. Indices 11–16 are tag-writing and import tasks; a bulk DELETE under the write lock ahead of a queued import would stall a user-initiated import behind maintenance.

Two tests pin the ordering relative to JanitorCleanFKsTask, SearchIndexSyncTask and ImportTask.

The folder guard, and the trap in it

A folder delete cascades to its comics through Comic.parent_folder and to sub-folders through Folder.parent_folder, so reaping a stamped ancestor would take live descendants — and their bookmarks — with it.

The obvious implementation is wrong, and the test caught it:

# WRONG — a folder with NO comics is NULL-extended on the LEFT OUTER
# JOIN, so this reads its absent comic as a live one and the folder
# can never be reaped.
expired.exclude(comic__missing_since__isnull=True)

Replaced with explicit ~Exists(live_comics) / ~Exists(live_subfolders) subqueries, which have no NULL-extension semantics to get wrong. Comics are reaped first, then folders, because reaping the comics is what can make a folder eligible.

Twelve registrations, not three

CLAUDE.md names three. The actual list is twelve, and omitting #8 is the silent-failure modeget_task_priority raises ValueError: tuple.index(x): x not in tuple and the job simply never runs.

Task class · status class · JANITOR_STATII · migration · _JANITOR_STATII · _NIGHTLY_TASKS · _JANITOR_METHOD_MAP · _SCRIBE_TASK_PRIORITY · _JANITOR_NIGHTLY_STATUSES · the Jobs-tab entry · _TASK_MAP · make build-choices.

test_the_job_is_fully_registered asserts all of them.

One corrected premise: the plan warned that max_length=max_choices_len(StatusChoices) is computed from the labels, so a long title would change the column width. It is computed from the choice keys (choice[0]), which are all three characters. max_length=3 is unchanged; migration 0056 only alters choices.

"Reap now" needed no new endpoint

Registrations #10 and #11 make it POST-able through the existing POST /admin/tasks/run + librarianTask(...) store action, exactly as Poll and Force Update work from the library rows — which gets the Jobs-tab entry, the sidebar progress row and the abort plumbing for free. The job carries a confirm string, since it permanently deletes read progress.

Publishing

The job clears its own caches, re-stamps the reaped comics' collections through TimestampUpdater, and enqueues LIBRARY_CHANGED_TASK itself — the janitor has no importer finish() to do it, and cleanup_fks notifying nothing is a gap, not a precedent. Collections are gathered before the delete, since afterwards there is nothing to read them off.

It also enqueues CoverRemoveTask: #880 deliberately left covers in place so a revived row would find its own, so the reaper owns removing them.

Scheduling

Untouched. The nightly trigger stays the cron thread; commit 11b3210b4 fixed "nightly janitor running dozens of times each midnight" and nothing here adds a second wake-up.

Tests

Eight cases in TestReaper: a fresh stamp is not reaped, an expired one is, a live comic is never touched, a folder with a live comic is spared (with its comic), a folder whose comics all expired is reaped, the announcement, the twelve registrations, and the ordering.

tests/test_scribe_priority.py still passes. make build-choices run. make fix && make lint && make ty clean; makemigrations --check and django-check clean. Full make test green: 1294 pytest (+8), 579 vitest.

No new NEWS line — #881's covers the user-visible behaviour.

Known limitation, worth stating

poll_force cannot recover a stamped library — it sets stat[8] = 0.0 on every DB row and skips the stale-stat refresh, so every path reads as modified, never as revived. That matters because Force Poll is the button an admin will press. The admin surface (P1f) should say so, or the force path should run the unstamp pass.

🤖 Generated with Claude Code

@ajslater
ajslater force-pushed the feat/pending-deletes-reaper branch from 778478d to 16c7f78 Compare September 21, 2026 08:52
@ajslater
ajslater added this pull request to stack #891 September 21, 2026 16:20
@ajslater
ajslater force-pushed the feat/pending-deletes-reaper branch from 16c7f78 to cd1544c Compare September 21, 2026 16:20
@ajslater
ajslater force-pushed the feat/pending-deletes-reaper branch from cd1544c to accfa4b Compare September 21, 2026 16:21
@ajslater
ajslater force-pushed the feat/pending-deletes-reaper branch from accfa4b to 3ec022c Compare September 21, 2026 16:22
Base automatically changed from feat/pending-deletes-revive to develop September 21, 2026 16:22
Last of the pending-delete mechanism. Until now nothing ever really
deleted a stamped row, which was safe but leaked rows indefinitely.

The window is a bare module constant, not a setting. It is a safety
delay before an irreversible action rather than a tuning knob, and the
nearest precedent is exact: the telemeter's 24-hour opt-out grace is a
bare constant that is documented to admins in prose while still not
being configurable. An AdminFlag would cost roughly ten files and would
be the only configurable knob in the whole delete-safety story. The
effective window is 24h plus the time to the next nightly run, which is
what "24 hours or more" meant.

Priority index 17, the first slot in the contiguous cleanup band. The
FK cleanup must follow it -- while a stamped comic still exists its
Publisher/Imprint/Series/Volume are not orphaned, so a reaper later in
the tuple leaves emptied groups visible until the next night -- and so
must the search sync, which drops FTS rows whose comic no longer
exists. Not earlier, because 11-16 are tag-writing and import tasks and
a bulk DELETE under the write lock should not stall a user-initiated
import behind maintenance.

The folder guard uses Exists subqueries rather than a join predicate on
purpose: on a LEFT OUTER JOIN a folder with no comics at all is
NULL-extended, so `exclude(comic__missing_since__isnull=True)` reads
its absent comic as a live one and the folder can never be reaped.
Caught by the test for exactly that case.

Covers are removed here, because the stamp deliberately left them in
place so a revived row would find its own. And the job publishes its
own cache clear, collection re-stamp and library.changed broadcast --
the janitor has no importer finish() to do it, and cleanup_fks
notifying nothing is a gap rather than a precedent.

Twelve registrations, not the three CLAUDE.md names. Omitting the
priority tuple is the silent-failure mode: get_task_priority raises
ValueError and the job never runs. A test asserts all of them.

One corrected premise: LibrarianStatus.status_type's max_length is
derived from the choice *keys*, which are all three characters, so the
column width does not move.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ajslater
ajslater force-pushed the feat/pending-deletes-reaper branch from 3ec022c to 312d103 Compare September 21, 2026 16:22
@ajslater
ajslater merged commit 83ab1c0 into develop Sep 21, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant