Retire the CoverageProvider machinery (PP-4468)

[dbernstein]

Description

Retires the legacy CoverageProvider machinery. Overdrive bibliographic coverage now runs as a direct metadata-apply step inside OverdriveAPI.update_licensepool instead of routing through OverdriveBibliographicCoverageProvider.ensure_coverage. The base CoverageProvider classes, the Overdrive/Bibliotheca providers, the coverage-provider runner scripts and their bin/repair entry points, and the now-dead readers (Identifier.missing_coverage_from, the Explain script's coverage block) are deleted.

BibliographicData.apply no longer writes CoverageRecord rows (the create_coverage_record parameter is removed; the Celery apply task and the Bibliotheca updater already passed create_coverage_record=False).

The new _ensure_bibliographic_coverage helper wraps bibliographic.apply(...) in a BasePalaceException guard so bad or incomplete data for a single title is logged and skipped rather than aborting the batch the caller is working through. A failed metadata fetch (Overdrive unreachable, bad credentials) and non-Palace exceptions from apply (a KeyError, a SQLAlchemy error) are deliberately left to propagate: those affect the whole run and should stop it loudly instead of quietly leaving thousands of pools without Works. The docstring spells that split out.

The dead admin refresh_metadata endpoint is removed outright, along with the route, its tests, and the two now-unused METADATA_REFRESH_* problem details. It was unreachable by any client: circulation-admin only renders its "Refresh Metadata" button when the admin OPDS entry carries a http://librarysimplified.org/terms/rel/refresh link, and nothing has emitted that rel since the metadata wrangler was removed (#792); ios-core never touched the admin API. With no provider wired up, any request that did arrive got an unconditional 400.

The CoverageRecord model and coveragerecords table are intentionally left in place (marked dormant) for one release, per the online-migration / N-1 rule. The stacked follow-up #3521 drops the coveragerecords and dormant equivalentscoveragerecords tables.

Note: this PR supersedes #3503; it is opened from a branch on the upstream repository and rebased on the latest main.

Motivation and Context

JIRA (PP-4468)

The CoverageProvider batch-processing pattern is being retired in favor of Celery. Investigation showed nothing live reads coverage rows:

• The only live caller of ensure_coverage (Overdrive's update_licensepool) passes force=True and ignores the result.
• The admin refresh_metadata endpoint is wired with no provider and no client can reach it, so it always returned METADATA_REFRESH_FAILURE.
• Identifier.missing_coverage_from / should_update / the Explain coverage block are only reachable through the already-dead runner scripts.

The Celery bibliographic_apply task and the Bibliotheca updater already skip coverage-record writes, so Overdrive was the last writer.

How Has This Been Tested?

• mypy and the project's pre-commit hooks pass on all changed files.
• Full-suite collection is clean (6136 tests, no dangling imports), and every bin/ script's imports were checked to resolve — bin/ is not imported by CI, which is how the two stale bin/repair coverage entry points survived the first pass.
• Targeted suites pass under tox -e py312-docker (619 tests): Overdrive API including the rewritten update_licensepool bibliographic-coverage path and the tests covering the bibliographic.apply guard (Palace errors swallowed, non-Palace errors propagating), the admin work-editor and route tests, the Bibliotheca package, data-layer apply, admin feeds, scripts, and the affected model tests.

Checklist

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

🤖 Generated with Claude Code

[claude]

Claude finished @dbernstein's task in 6m 3s —— View job

---

Summary

The deletion side of this is careful and well-verified — no dangling imports, bin/ entry points cleaned up, mocks and tests removed in step, and the new _ensure_bibliographic_coverage faithfully reproduces what OverdriveBibliographicCoverageProvider.process_item + handle_success did (same error codes, same _edition/apply/calculate_work/set_presentation_ready sequence), with good tests for both the swallow and the re-raise path. The narrowed except BasePalaceException and the propagating metadata_lookup are both defensible and now documented. My one substantive concern is the dormant-CoverageRecord plan: the model is still wired into Collection with a delete cascade, so this release does not actually stop using the table, which is what the stacked drop in #3521 depends on.

Details

src/palace/manager/sqlalchemy/model/coverage.py:365-371

The docstring's claim that "nothing in the current code reads or writes this table" isn't true while Collection.coverage_records is still mapped with cascade="all". Collection.delete() finishes with _db.delete(self), so SQLAlchemy loads that relationship (SELECT ... FROM coveragerecords WHERE collection_id = ...) and emits DELETEs for the children — which is exactly what TestCollection.test_delete asserts today. Once #3521 drops the table, the collection_delete Celery task running on N-1 (i.e. this release's) workers will fail with UndefinedTable, which is the failure mode CLAUDE.md's "stop using means stopping both reads and writes" rule exists to prevent. Dropping the relationship alone isn't sufficient either: rows still exist and coveragerecords.collection_id has no ON DELETE action, so a release-1 migration needs to hand cleanup to the database (ON DELETE CASCADE on the coveragerecords FKs) before the Collection/Identifier/DataSource relationships come off. Worth confirming with ./docker/ci/test_backwards_compatibility.sh <this commit> checked out against #3521 before that one merges.

circulation/src/palace/manager/sqlalchemy/model/coverage.py

Lines 365 to 371 in 601d261

	Dormant model retained only so the ``coveragerecords`` table stays in the
	schema for one more release. The CoverageProvider machinery that read and
	wrote these records has been retired; nothing in the current code reads or
	writes this table. Per our online-migration convention the table cannot be
	dropped in the same release that stops using it (N-1 app servers still write
	here during a rolling deploy), so this model and the table will be removed in
	a follow-up PR that ships after this release.

Minor: src/palace/manager/integration/license/overdrive/api.py:1340-1343

When calculate_work() returns None the method returns silently, so a pool that ends up with no Work leaves no trace at warning level — the old path logged this as a transient failure ("Work could not be calculated"). This is reachable for the untitled-edition case, where calculate_work only logs at info. Since every other skip in this method logs a warning explaining why the pool has no Work, an else here would keep the diagnostics consistent:

work, _ = license_pool.calculate_work()
if work:
    work.set_presentation_ready()
else:
    self.log.warning(
        "Could not calculate a Work for %s after applying Overdrive bibliographic data.",
        identifier.identifier,
    )

circulation/src/palace/manager/integration/license/overdrive/api.py

Lines 1340 to 1343 in 601d261

	if not license_pool.work or not license_pool.work.presentation_ready:
	work, _ = license_pool.calculate_work()
	if work:
	work.set_presentation_ready()

--- · chore/retire-coverage-provider

[greptile-apps]

Greptile Summary

This PR retires the legacy CoverageProvider batch-processing machinery (~4,500 lines deleted) in favour of Celery, replacing Overdrive's bibliographic coverage path with a new inline _ensure_bibliographic_coverage helper on OverdriveAPI. The dead refresh_metadata admin endpoint and its route are also removed outright; the CoverageRecord model and table are intentionally kept dormant for one release to satisfy the N-1 online-migration rule.

• New _ensure_bibliographic_coverage: fetches Overdrive metadata, extracts BibliographicData, applies it to the edition, and ensures the pool has a presentation-ready Work. BasePalaceException from apply is caught and logged so one bad title cannot abort a batch; network failures and non-Palace exceptions from apply propagate deliberately, as documented in the method's docstring.
• BibliographicData.apply simplified: the create_coverage_record parameter is removed; the method no longer writes CoverageRecord rows. All callers already passed False or are updated.
• Admin cleanup: refresh_metadata endpoint, its route, the two METADATA_REFRESH_* problem details, and all related tests are consistently removed.

Confidence Score: 5/5

• Safe to merge. The change is a well-scoped deletion of verified-dead code plus a small, well-tested replacement for Overdrive bibliographic coverage.
• The deleted code was confirmed dead before removal (no live callers, coverage rows not read by anything), the new _ensure_bibliographic_coverage helper has targeted tests for both the catch and re-raise paths, and the exception-propagation design is explicitly documented. No new logic was added elsewhere in the codebase.
• No files require special attention.

Important Files Changed

Filename	Overview
src/palace/manager/integration/license/overdrive/api.py	Core change: removes OverdriveBibliographicCoverageProvider dependency and introduces _ensure_bibliographic_coverage. New method is well-guarded — handles Overdrive errorCode responses, missing bibliographic data, and BasePalaceException from apply, with the remaining propagation behavior clearly documented. Tests cover both the catch and re-raise paths.
src/palace/manager/data_layer/bibliographic.py	Removes the create_coverage_record parameter and the CoverageRecord.add_for call inside apply. Clean removal — callers already passed False or are updated; the corresponding test is also updated.
src/palace/manager/core/coverage.py	Entire file deleted (1,364 lines). Verified that no remaining production code imports from this module.
src/palace/manager/sqlalchemy/model/coverage.py	CoverageRecord model retained as dormant with a clear docstring explaining the N-1 migration convention. Timestamp model unchanged and still actively used by Monitors.
src/palace/manager/api/admin/controller/work_editor.py	refresh_metadata method fully removed. The route, its tests, and the two METADATA_REFRESH_* problem details are all consistently cleaned up across routes.py and problem_details.py.
tests/manager/integration/license/overdrive/test_api.py	Adds two targeted tests for _ensure_bibliographic_coverage: one confirms BasePalaceException is caught and logged without propagating; the other confirms non-Palace exceptions (e.g. KeyError) re-raise. The existing test_update_licensepool is updated to remove the now-defunct coverage-record assertion.
tests/manager/data_layer/test_bibliographic.py	Removes test_apply_creates_coverage_records. Updates test_apply_does_not_create_coverage_records to call apply without the now-removed create_coverage_record=False kwarg — test body is still correct, but the name is now misleading since there's no longer an alternative behavior to contrast it against.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant OverdriveAPI
    participant Overdrive as Overdrive API
    participant BibliographicData
    participant LicensePool

    Caller->>OverdriveAPI: update_licensepool(book_id)
    OverdriveAPI->>Overdrive: get availability
    Overdrive-->>OverdriveAPI: availability JSON
    OverdriveAPI->>LicensePool: for_foreign_id()
    LicensePool-->>OverdriveAPI: license_pool, is_new

    alt is_new OR no work
        OverdriveAPI->>OverdriveAPI: _ensure_bibliographic_coverage(pool)
        OverdriveAPI->>Overdrive: metadata_lookup(identifier)
        Overdrive-->>OverdriveAPI: metadata JSON

        alt "errorCode = NotFound / InvalidGuid"
            OverdriveAPI-->>OverdriveAPI: log warning, return
        else metadata valid
            OverdriveAPI->>BibliographicData: book_info_to_bibliographic(info)
            BibliographicData-->>OverdriveAPI: bibliographic (or None)

            alt bibliographic is None
                OverdriveAPI-->>OverdriveAPI: log warning, return
            else
                OverdriveAPI->>BibliographicData: apply(db, edition, collection)
                alt BasePalaceException
                    BibliographicData-->>OverdriveAPI: raises
                    OverdriveAPI-->>OverdriveAPI: log warning, return
                else success
                    BibliographicData-->>OverdriveAPI: (edition, changed)
                    OverdriveAPI->>LicensePool: calculate_work()
                    LicensePool-->>OverdriveAPI: work
                    OverdriveAPI->>LicensePool: work.set_presentation_ready()
                end
            end
        end
    end

    OverdriveAPI->>OverdriveAPI: update_licensepool_with_book_info(...)
    OverdriveAPI-->>Caller: (pool, is_new, changed)

Loading

_{Reviews (5): Last reviewed commit: "Say exactly what the bibliographic apply..." | Re-trigger Greptile}

[greptile-apps]

metadata_lookup exceptions propagate outside the guard

The except Exception block covers only bibliographic.apply(). A transient HTTP error or malformed JSON response from metadata_lookup() (lines 1812–1813) will propagate to update_licensepool and abort the entire availability update, preventing update_licensepool_with_book_info from running. This matches the old behaviour — the retired process_batch also did not wrap process_item in a try/except — so this is not a regression, but it is worth calling out explicitly because the guard communicates an intent ("bibliographic failures must not crash availability updates") that isn't fully satisfied if the fetch itself throws. Consider wrapping metadata_lookup in the same guard, or adding a dedicated try/except around just that call with a return on failure.

[dbernstein]

Leaving metadata_lookup outside the guard on purpose, and the docstring now says so instead of implying blanket resilience.

The asymmetry is the point. A failed apply is usually about one title's data. A failed fetch usually isn't: Overdrive is unreachable, or the credentials are wrong, or the collection token is stale — conditions that will hit every identifier in the batch. Callers run this in a loop over batches of identifiers, so swallowing a fetch failure would walk the whole batch logging warnings and finish looking like a successful run, having left every pool without a Work. Letting it propagate fails the task, which is the honest outcome and the one that gets retried.

So the guard's intent isn't "bibliographic failures must not crash availability updates" — it's narrower than that, and the docstring is now explicit about which failures are skipped and which are allowed to stop the run.

[greptile-apps]

That's a well-reasoned split — the asymmetry makes sense. Per-title apply failures are ignorable noise; a fetch failure is a signal about the whole run that should surface loudly rather than be buried in warnings. If the updated docstring now explicitly documents which failures are skipped vs. allowed to propagate, the intent is clear to future readers. No further change needed.

[jonathangreen]

@dbernstein it looks like tests are failing on this one, and greptile has a valid review comment about unconditionally calling self.load_work.

I'll let you resolve those issues before reviewing this one. When its ready you can request a review from @ThePalaceProject/backend again.

[jonathangreen]

Some comments on this for consideration while you are resolving the test issues

[jonathangreen]

Since this does nothing can we deprecate it, or just remove it entirely? What calls this endpoint? I think we need a plan to deal with this endpoint since its no longer functional.

[jonathangreen]

There a 👍🏻 on this, but do we have a ticket or plan for it?

[dbernstein]

No ticket needed — I checked what calls it, and the answer is nothing, so it's removed outright in this PR rather than deprecated.

circulation-admin only renders its "Refresh Metadata" button when the admin OPDS entry carries a http://librarysimplified.org/terms/rel/refresh link (editorAdapter.ts looks for that rel, and BookDetailsEditor gates the button on bookData.refreshLink). Nothing in the manager has emitted that rel since the metadata wrangler came out in #792 — the current admin annotators emit only suppress / unsuppress / edit / search. ios-core has no reference to the rel or the endpoint, and doesn't use the admin API at all.

So the button never appears and the route was unreachable; anything that did reach it got an unconditional 400. Removed the route, the controller method, its tests, and the two orphaned METADATA_REFRESH_* problem details. Also dropped test_feed_includes_refresh_link, which despite the name only asserted the absence of that rel — a leftover from the wrangler removal that no longer has an endpoint behind it.

Tagged the PR incompatible changes since it drops a registered admin route, though in practice no client could reach it.

[codecov]

Codecov Report

❌ Patch coverage is 82.60870% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.69%. Comparing base (a55b97e) to head (601d261).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
...alace/manager/integration/license/overdrive/api.py	81.81%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3520      +/-   ##
==========================================
+ Coverage   93.61%   93.69%   +0.07%     
==========================================
  Files         514      510       -4     
  Lines       47065    46398     -667     
  Branches     6408     6301     -107     
==========================================
- Hits        44060    43471     -589     
+ Misses       1944     1891      -53     
+ Partials     1061     1036      -25     

☔ 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.

[greptile-apps]

Exception guard scope excludes SQLAlchemy errors from apply

The except BasePalaceException block only catches Palace-domain exceptions. BibliographicData.apply performs multiple SQLAlchemy writes (contributors, links, identifiers, equivalencies) that can raise IntegrityError, OperationalError, or DataError — none of which inherit from BasePalaceException. When those slip through, the exception propagates out of _ensure_bibliographic_coverage and into update_licensepool, where there is no further guard, so the outer availability update is aborted for that title.

The old BibliographicCoverageProvider.set_bibliographic wrapped bibliographic.apply in a bare except Exception, which is what the PR description ("broad except Exception") implies was the intent. The two new tests only cover BasePalaceException / KeyError side-effects from a mock, so SQLAlchemy exceptions are untested here.

If the intentional design is to let non-Palace exceptions surface as bugs, the docstring's claim that "a database error … does not propagate out of update_licensepool" should be tightened to match. If the intent truly is to swallow all apply-time errors, the guard should be widened to except Exception.

[dbernstein]

Took the first option: kept the narrow guard and tightened the docs to match.

Widening back to except Exception isn't available — that's what @jonathangreen pushed back on above ("they catch and mask so much, like this would catch and hide a key name typo in the code"), and he's right. So the split is now deliberate and spelled out in the docstring:

• BasePalaceException from apply means bad or incomplete data for this one title. Logged, skipped, batch continues.
• A SQLAlchemy error means a broken session or a genuine data-integrity problem, and a KeyError means a bug. Both propagate, because neither is specific to one title and both should stop the run loudly rather than quietly leaving thousands of pools without Works.

Good catch on the PR description — it still claimed a "broad except Exception" from an earlier revision. Corrected.

[jonathangreen]

Looks like there are some valid AI code review comments that still should be resolved on this one, so I'm going to leave it until you get back from vacation @dbernstein

[jonathangreen]

Looks good