Skip to content

Retire the CoverageProvider machinery (PP-4468)#3520

Draft
dbernstein wants to merge 6 commits into
mainfrom
chore/retire-coverage-provider
Draft

Retire the CoverageProvider machinery (PP-4468)#3520
dbernstein wants to merge 6 commits into
mainfrom
chore/retire-coverage-provider

Conversation

@dbernstein

@dbernstein dbernstein commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

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 the now-dead readers (Identifier.missing_coverage_from, the Explain script's coverage block, the always-failing admin refresh_metadata provider path) 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 broad except Exception that logs a warning and returns, preserving the resilience of the retired BibliographicCoverageProvider.set_bibliographic so a database error or unexpected data shape during apply does not propagate out of update_licensepool.

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

Note: this PR supersedes #3503; it is opened from a branch on the upstream repository, rebased on the latest main, and addresses the review feedback there (the bibliographic.apply exception guard).

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, so it always returns METADATA_REFRESH_FAILURE without invoking a provider.
  • 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 ruff pass on all changed files.
  • Targeted suites pass under tox -e py312-docker: Overdrive API including the rewritten update_licensepool bibliographic-coverage path and a new test covering the bibliographic.apply error guard, the Bibliotheca circulation updater, data-layer apply, and the affected model tests.

Checklist

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

🤖 Generated with Claude Code

@claude

claude Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

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


Code Review

Summary

The retirement is well-scoped and the inlined _ensure_bibliographic_coverage faithfully reproduces the old OverdriveBibliographicCoverageProvider path (same edition lookup, same ReplacementPolicy.from_license_source(), same set_presentation_ready semantics — the dropped exclude_search=EXCLUDE_SEARCH_INDEX matched the False default). Keeping coveragerecords dormant for one release is the correct N-1 handling. One real problem: two bin/ entry points for the retired providers were left behind and now import deleted modules. Two smaller notes below. (Also worth updating the PR body — it still describes the guard as a "broad except Exception", but the code now catches only BasePalaceException.)

Details

bin/repair/overdrive_bibliographic_coverage:4-9

These two runner scripts were not removed and now import modules/classes this PR deletes, so both fail immediately with ModuleNotFoundError when invoked: bin/repair/overdrive_bibliographic_coverage imports OverdriveBibliographicCoverageProvider (from the deleted overdrive/coverage.py) and RunCoverageProviderScript (from the deleted scripts/coverage_provider.py), and bin/repair/bibliotheca_bibliographic_coverage imports the removed BibliothecaBibliographicCoverageProvider plus RunCollectionCoverageProviderScript. Nothing in CI imports bin/, so this isn't caught by the passing lint/mypy runs. They should be deleted along with the rest of the machinery.

from palace.manager.integration.license.overdrive.coverage import (
OverdriveBibliographicCoverageProvider,
)
from palace.manager.scripts.coverage_provider import RunCoverageProviderScript
RunCoverageProviderScript(OverdriveBibliographicCoverageProvider).run()

Minor: src/palace/manager/integration/license/overdrive/api.py:1839-1849

The guard now catches only BasePalaceException, but the comment still claims "A failure while applying the bibliographic data must not abort the surrounding availability update." A SQLAlchemyError (e.g. an IntegrityError/DataError on a malformed record) raised by bibliographic.apply is not a BasePalaceException, so it will propagate and abort update_licensepool — the opposite of what the comment promises. The narrowing itself is a reasonable response to the earlier review feedback; just tighten the comment so it reflects that only Palace-level errors are swallowed and DB errors are intentionally allowed to surface.

except BasePalaceException as e:
# A failure while applying the bibliographic data must not abort the
# surrounding availability update, so log it and leave the pool's
# presentation edition unchanged.
self.log.warning(
"Error applying Overdrive bibliographic data to edition %s: %s",
edition.id,
e,
exc_info=e,
)
return

Minor: src/palace/manager/api/admin/controller/work_editor.py:448-461

refresh_metadata is now a permanently non-functional endpoint that always returns METADATA_REFRESH_FAILURE (the /works/<type>/<id>/refresh route is still registered). This matches the reviewer's open question on the PR — rather than leaving a dead-but-wired endpoint, decide its fate now: either remove the route + controller method outright, or have it return a clearer "not supported" problem detail. A silent METADATA_REFRESH_FAILURE reads to the admin UI as a transient failure rather than "this feature is gone."

def refresh_metadata(
self, identifier_type: str, identifier: str
) -> Response | ProblemDetail:
"""Refresh the metadata for a book from the content server.
Metadata refresh used to run through the per-source CoverageProvider
machinery, which has been retired. No provider is wired up, so this
endpoint is retained for API compatibility but no longer performs a
refresh; it always reports failure.
"""
library = get_request_library()
self.require_librarian(library)
return METADATA_REFRESH_FAILURE

@dbernstein
dbernstein force-pushed the chore/retire-coverage-provider branch from 8f4574b to 27bf685 Compare June 30, 2026 00:01
@dbernstein
dbernstein requested a review from a team June 30, 2026 00:02
@greptile-apps

greptile-apps Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR retires the CoverageProvider batch-processing machinery (~1 400 lines of core, tests, and runner scripts) in favour of Celery-based processing, and replaces the last live caller — OverdriveBibliographicCoverageProvider.ensure_coverage inside update_licensepool — with a new _ensure_bibliographic_coverage helper. BibliographicData.apply no longer writes CoverageRecord rows; the coveragerecords table is kept dormant for one release per the N-1 rolling-deploy rule.

  • New _ensure_bibliographic_coverage (overdrive/api.py): fetches Overdrive metadata and applies it only for new or work-less license pools; ongoing metadata updates for existing titles continue via the Celery bibliographic_apply task.
  • BibliographicData.apply signature simplified: create_coverage_record parameter removed; the Celery apply task and the Bibliotheca circulation updater already omitted it.
  • refresh_metadata endpoint hardened: drops the dead load_work call and the removed provider parameter; now always returns METADATA_REFRESH_FAILURE after the librarian auth check.
  • CoverageRecord model kept dormant: comment added noting the table will be dropped in a stacked follow-up PR.

Confidence Score: 4/5

Safe to merge with one concern worth confirming: the exception guard in _ensure_bibliographic_coverage is narrower than its docstring implies.

The removal is well-scoped and all live callers were either migrated or verified as dead code before deletion. The except BasePalaceException guard around bibliographic.apply does not catch SQLAlchemy errors, yet the docstring states the intent is to prevent database errors from aborting the outer availability update.

src/palace/manager/integration/license/overdrive/api.py — specifically the exception guard scope in _ensure_bibliographic_coverage.

Important Files Changed

Filename Overview
src/palace/manager/integration/license/overdrive/api.py Adds _ensure_bibliographic_coverage helper that fetches and applies Overdrive metadata for new or work-less license pools; exception guard is narrower than the docstring implies.
src/palace/manager/core/coverage.py Entire 1364-line CoverageProvider machinery deleted; all callers migrated to direct apply or Celery tasks before this removal.
src/palace/manager/data_layer/bibliographic.py Removes create_coverage_record parameter and CoverageRecord.add_for call from BibliographicData.apply; the table is now dormant so no records are written.
src/palace/manager/sqlalchemy/model/coverage.py Adds dormant-model comment to CoverageRecord noting table is kept for N-1 rolling-deploy compatibility and will be dropped in a follow-up.
src/palace/manager/api/admin/controller/work_editor.py Simplifies refresh_metadata to drop provider parameter and dead load_work call; endpoint now always returns METADATA_REFRESH_FAILURE after auth check.
src/palace/manager/integration/license/bibliotheca.py Deletes BibliothecaBibliographicCoverageProvider class; Bibliotheca metadata updates are now handled exclusively by the Celery bibliographic_apply task.
src/palace/manager/sqlalchemy/model/identifier.py Removes Identifier.missing_coverage_from class method (dead code, only reachable from deleted runner scripts).
src/palace/manager/celery/tasks/apply.py Drops the create_coverage_record=False kwarg from the bibliographic_apply Celery task call, matching the removed parameter from BibliographicData.apply.
src/palace/manager/integration/license/overdrive/coverage.py Entire OverdriveBibliographicCoverageProvider class deleted; logic migrated to OverdriveAPI._ensure_bibliographic_coverage.
src/palace/manager/scripts/coverage_provider.py Entire 169-line runner script deleted along with its tests.
tests/manager/integration/license/overdrive/test_api.py Removes coverage-record assertions; adds two new tests verifying BasePalaceException is swallowed and KeyError propagates from _ensure_bibliographic_coverage.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant OD as OverdriveAPI
    participant LP as LicensePool
    participant EBC as _ensure_bibliographic_coverage
    participant ML as metadata_lookup
    participant ORE as OverdriveRepresentationExtractor
    participant BD as BibliographicData.apply
    participant CW as calculate_work

    OD->>LP: for_foreign_id (get or create)
    alt is_new OR not license_pool.work
        OD->>EBC: _ensure_bibliographic_coverage(pool)
        EBC->>ML: metadata_lookup(identifier)
        ML-->>EBC: info dict
        alt errorCode in (NotFound, InvalidGuid)
            EBC-->>OD: return (log warning)
        end
        EBC->>ORE: book_info_to_bibliographic(info)
        ORE-->>EBC: "bibliographic | None"
        alt bibliographic is None
            EBC-->>OD: return (log warning)
        end
        EBC->>BD: bibliographic.apply(db, edition, collection, replace)
        alt raises BasePalaceException
            BD-->>EBC: exception
            EBC-->>OD: return (log warning, pool has no work)
        else success
            BD-->>EBC: (edition, changed)
            EBC->>CW: license_pool.calculate_work()
            CW-->>EBC: (work, _)
            EBC->>CW: work.set_presentation_ready()
        end
    end
    OD->>OD: update_licensepool_with_book_info(availability, ...)
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant OD as OverdriveAPI
    participant LP as LicensePool
    participant EBC as _ensure_bibliographic_coverage
    participant ML as metadata_lookup
    participant ORE as OverdriveRepresentationExtractor
    participant BD as BibliographicData.apply
    participant CW as calculate_work

    OD->>LP: for_foreign_id (get or create)
    alt is_new OR not license_pool.work
        OD->>EBC: _ensure_bibliographic_coverage(pool)
        EBC->>ML: metadata_lookup(identifier)
        ML-->>EBC: info dict
        alt errorCode in (NotFound, InvalidGuid)
            EBC-->>OD: return (log warning)
        end
        EBC->>ORE: book_info_to_bibliographic(info)
        ORE-->>EBC: "bibliographic | None"
        alt bibliographic is None
            EBC-->>OD: return (log warning)
        end
        EBC->>BD: bibliographic.apply(db, edition, collection, replace)
        alt raises BasePalaceException
            BD-->>EBC: exception
            EBC-->>OD: return (log warning, pool has no work)
        else success
            BD-->>EBC: (edition, changed)
            EBC->>CW: license_pool.calculate_work()
            CW-->>EBC: (work, _)
            EBC->>CW: work.set_presentation_ready()
        end
    end
    OD->>OD: update_licensepool_with_book_info(availability, ...)
Loading

Reviews (4): Last reviewed commit: "Set the log level in the apply-error tes..." | Re-trigger Greptile

Comment on lines +1832 to +1851
try:
bibliographic.apply(
self._db,
edition,
self.collection,
replace=ReplacementPolicy.from_license_source(),
)
except Exception as e:
# Mirror the resilience of the former
# ``BibliographicCoverageProvider.set_bibliographic``: a database
# error or unexpected data shape during apply should not crash
# ``update_licensepool``. Log and leave the pool without updated
# coverage, matching the prior "silently continue" behavior.
self.log.warning(
"Error applying Overdrive bibliographic data to edition %s: %s",
edition.id,
e,
exc_info=e,
)
return

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/palace/manager/api/admin/controller/work_editor.py Outdated
@jonathangreen

Copy link
Copy Markdown
Member

@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 jonathangreen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +453 to +456
Metadata refresh used to run through the per-source CoverageProvider
machinery, which has been retired. No provider is wired up, so this
endpoint is retained for API compatibility but no longer performs a
refresh; it always reports failure.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/palace/manager/integration/license/overdrive/api.py Outdated
Comment thread src/palace/manager/integration/license/overdrive/api.py Outdated
Comment thread src/palace/manager/integration/license/overdrive/api.py Outdated
Comment thread tests/manager/integration/license/overdrive/test_api.py Outdated
@dbernstein
dbernstein force-pushed the chore/retire-coverage-provider branch from 27bf685 to ac18fb5 Compare July 1, 2026 18:08
@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 70.83333% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.54%. Comparing base (65b15bf) to head (c7d813d).
⚠️ Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
...alace/manager/integration/license/overdrive/api.py 68.18% 5 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3520      +/-   ##
==========================================
+ Coverage   93.46%   93.54%   +0.07%     
==========================================
  Files         512      509       -3     
  Lines       46611    45964     -647     
  Branches     6352     6245     -107     
==========================================
- Hits        43566    42997     -569     
+ Misses       1969     1915      -54     
+ Partials     1076     1052      -24     

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

@dbernstein
dbernstein requested a review from jonathangreen July 13, 2026 19:53
dbernstein and others added 6 commits July 14, 2026 10:06
Apply Overdrive bibliographic metadata directly instead of routing it through
the CoverageProvider machinery, then delete the now-dead machinery.

Nothing read the coverage rows: the only live caller of ensure_coverage was
OverdriveAPI.update_licensepool (force=True, return value ignored), and the
admin refresh_metadata endpoint was wired with no provider. The Celery apply
task and the Bibliotheca updater already skipped coverage-record writes, so
Overdrive was the last writer.

- Rewrite OverdriveAPI.update_licensepool to fetch + apply metadata and make
  the work presentation-ready via a new _ensure_bibliographic_coverage helper,
  replacing OverdriveBibliographicCoverageProvider.ensure_coverage.
- Stop writing coverage records: drop create_coverage_record from
  BibliographicData.apply and its call sites.
- Delete core/coverage.py, the Overdrive and Bibliotheca coverage providers,
  the coverage-provider runner scripts, Identifier.missing_coverage_from, and
  the Explain script's coverage block.
- Neutralize the now-dead admin refresh_metadata endpoint.
- Leave the CoverageRecord model and coveragerecords table in place (dormant)
  for one release; the drop migration follows in a stacked PR.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Restore the resilience of the retired BibliographicCoverageProvider:
catch and log exceptions from bibliographic.apply in
_ensure_bibliographic_coverage so a database error or unexpected data
shape during apply does not propagate out of update_licensepool.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address review feedback: the _ensure_bibliographic_coverage docstring, its
exception-handling comment, and the matching test comment described the retired
CoverageProvider classes instead of what the code does now. Rewrite them to
explain the current behavior.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address review feedback: a bare `except Exception` around bibliographic.apply
masks programming errors (a mistyped attribute raising AttributeError/KeyError
would be silently swallowed). Catch BasePalaceException instead -- apply raises
PalaceValueError for invalid data -- so genuine bugs propagate while a single
bad title still does not abort the availability update. Update the guard test
to raise PalaceValueError and add a test that an unexpected error propagates.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address Greptile feedback: refresh_metadata always returns
METADATA_REFRESH_FAILURE, so loading the Work only to discard it was a needless
database query on every request. Keep the librarian authorization check and
return the failure directly.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
test_ensure_bibliographic_coverage_apply_error asserted on caplog.text without
setting a capture level, so it relied on the ambient root log level. Under
pytest-randomly, a prior test that raises that level leaves the warning
uncaptured, so caplog.text is empty and the assertion fails. Set the level
explicitly with caplog.set_level(LogLevel.warning), matching the convention used
by other caplog tests.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@dbernstein
dbernstein force-pushed the chore/retire-coverage-provider branch from e5b1dc0 to c7d813d Compare July 14, 2026 17:06
Comment on lines +1832 to +1849
try:
bibliographic.apply(
self._db,
edition,
self.collection,
replace=ReplacementPolicy.from_license_source(),
)
except BasePalaceException as e:
# A failure while applying the bibliographic data must not abort the
# surrounding availability update, so log it and leave the pool's
# presentation edition unchanged.
self.log.warning(
"Error applying Overdrive bibliographic data to edition %s: %s",
edition.id,
e,
exc_info=e,
)
return

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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
dbernstein marked this pull request as draft July 14, 2026 21:31

@jonathangreen jonathangreen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

2 participants