Stop using the coveragerecords table (PP-4653)

[dbernstein]

Description

Release-1 half of retiring the coveragerecords table, split out of #3521.

#3520 retired the CoverageProvider machinery and left the CoverageRecord model "dormant" for a release. It wasn't dormant: Identifier, DataSource and Collection still mapped a coverage_records relationship, and SQLAlchemy loads a relationship whenever its parent is deleted — to cascade the delete (mapper.cascade_iterator) or to null the child's foreign key (dependency.presort_deletes). So every session.delete() on one of those parents still SELECTed from coveragerecords.

This PR actually stops using the table:

• Removes the coverage_records relationships from Identifier, DataSource and Collection, and the identifier / data_source / collection relationships that pointed back at them from CoverageRecord.
• Reduces CoverageRecord to a bare table definition — columns, __table_args__ and the trailing module-level Index. lookup, add_for, bulk_add, assert_coverage_operation, human_readable and __repr__ are gone, along with the five *_OPERATION constants that only fed them, and BaseCoverageRecord.not_covered with the ALL_STATUSES / PREVIOUSLY_ATTEMPTED / DEFAULT_COUNT_AS_COVERED lists that only fed it. Nothing called any of them, and they had become a hazard rather than merely dead: with the parent relationships gone, a row written by add_for/bulk_add could no longer be cleaned up when its parent is deleted, so it would make that delete fail on a foreign key. status_enum and the four status constants stay — they are the column type for both dormant models.
• Empties coveragerecords in a migration. Its foreign keys carry no ON DELETE clause, so with the relationships gone SQLAlchemy no longer cleans up children by hand and a surviving row would make deleting its parent fail with a foreign-key violation. The rows are dead data, so TRUNCATE is cheaper than teaching a table we drop next release to cascade. It runs under SET LOCAL lock_timeout = '5s': N-1 servers still read the table, so TRUNCATE's ACCESS EXCLUSIVE lock can queue behind an in-flight parent delete and block every later reader — failing fast and retrying beats stalling instance startup.
• Removes the CoverageRecord tests and the db.coverage_record fixture, so the next release's backwards-compatibility gate doesn't run them against a schema where the table is gone.

The CoverageRecord and EquivalencyCoverageRecord models stay for one more release. A fresh database's schema is built with create_all from the models rather than by replaying migrations (InstanceInitializationScript.initialize_database_schema), so deleting the classes now would drop the table out from under N-1 servers on new installs. The models and the tables go together in the stacked follow-up, #3521.

equivalentscoveragerecords needs no change here — it has no ORM relationships pointing at it and its one foreign key already declares ON DELETE CASCADE.

Motivation and Context

JIRA (PP-4653)

This is the missing middle step in the CoverageProvider retirement. The original plan was #3520 (stop using) → #3521 (drop), but #3520 did not stop the ORM-level reads, so the drop in #3521 would have broken N-1 webservers on any collection or identifier delete during a rolling deploy.

Splitting at the relationship rather than at the model is what makes the sequence work: release 1 (this PR) removes the reads but keeps the table in fresh schemas; release 2 (#3521) removes the model and the table together.

How Has This Been Tested?

• mypy clean; pre-commit clean.
• tests/migration/, tests/manager/sqlalchemy/ and tests/manager/data_layer/ pass under tox -e py312-docker (664 tests), including a new migration test covering the truncate and confirming the table itself survives this release.
• Backwards compatibility verified locally, which is the point of the PR:

  ./docker/ci/test_backwards_compatibility.sh 7a3274ea15ba50c1fb61b83d19db7e34335eded4   # the #3520 release
  → 3346 passed — "The previous release works against the current schema 🎉"
  

  For contrast, the same gate run against the old single-PR Drop coveragerecords and equivalentscoveragerecords tables (PP-4653) #3521 failed with 30 tests erroring on relation "coveragerecords" does not exist, 24 of them purely from the relationships being walked on parent deletes.
• The stacked Drop coveragerecords and equivalentscoveragerecords tables (PP-4653) #3521 passes the same gate against this branch's content (3391 passed). It needs a rebase onto the current head of this branch before that result applies to the pushed commits.

Checklist

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

🤖 Generated with Claude Code

[claude]

Claude finished @dbernstein's task in 2m 57s —— View job

---

Summary

The PR does what it says. It removes the coverage_records relationships that made SQLAlchemy read coveragerecords whenever an Identifier, DataSource or Collection was deleted, and it strips CoverageRecord down to a bare table definition. The migration truncates the table under a 5s lock_timeout and then resets the timeout, so later revisions in the same upgrade don't inherit it. I checked three things:

• Removed helpers: nothing in src at the base commit calls lookup, add_for, bulk_add or .coverage_records.
• Fresh databases: create_all still creates the table. coverage.py is still imported through collection.py and sqlalchemy/session.py, so the model stays registered.
• Compatibility gate: tests/migration is excluded from docker/ci/test_backwards_compatibility.sh (--ignore=tests/migration).

The one real issue is in the new lock-timeout test: it passes on this branch but will fail once the stacked #3521 is rebased onto it.

Details

Minor: tests/migration/test_20260916_58ebd34c5092_empty_coveragerecords.py:70-98

test_upgrade_does_not_leak_lock_timeout runs upgrade() directly against alembic_engine, whose schema is built by Base.metadata.create_all (SessionManager.initialize_schema). It only passes because the CoverageRecord model still exists. #3521 deletes that model and hasn't been rebased onto this commit yet (its head still has the old docstring and no reset). Once it is rebased, this test will fail on the TRUNCATE with relation "coveragerecords" does not exist. test_empties_coveragerecords avoids this because it migrates down first, and #3521's drop migration recreates the table in its downgrade(). Doing the same here keeps the test working across the stack:

def test_upgrade_does_not_leak_lock_timeout(
    alembic_runner: MigrationContext, alembic_engine: Engine
) -> None:
    alembic_runner.migrate_down_to(REVISION)
    revision = _load_revision()
    ...

circulation/tests/migration/test_20260916_58ebd34c5092_empty_coveragerecords.py

Lines 70 to 98 in 664289f

	def test_upgrade_does_not_leak_lock_timeout(alembic_engine: Engine) -> None:
	"""upgrade() leaves lock_timeout exactly as it found it.
	The TRUNCATE runs under a short ``lock_timeout``, set with ``SET LOCAL``,
	which lasts to the end of the *transaction* rather than the end of this
	revision. ``alembic/env.py`` runs every pending revision inside a single
	``context.begin_transaction()``, so without an explicit reset the timeout
	would silently apply to every later revision in the same upgrade.
	Driving the migration through ``alembic_runner`` could not catch that: it
	commits between revisions, which discards the setting regardless of what
	the migration did. So run ``upgrade()`` against a transaction we hold open
	ourselves, which is the situation env.py actually creates.
	"""
	revision = _load_revision()
	with alembic_engine.begin() as connection:
	before = connection.execute(text("SHOW lock_timeout")).scalar_one()
	# setattr, not plain assignment: the module is typed as ModuleType, so
	# mypy does not know about the ``op`` its revision file imports.
	setattr(
	revision, "op", Operations(RuntimeMigrationContext.configure(connection))
	)
	revision.upgrade()
	after = connection.execute(text("SHOW lock_timeout")).scalar_one()
	assert after == before

Nit: tests/migration/test_20260916_58ebd34c5092_empty_coveragerecords.py:89-94

Alembic already has an API for this. Operations.context(migration_context) puts a real Operations behind the global alembic.op proxy, which is what the revision module already imports. Using it removes the setattr workaround and its mypy comment, and the "private copy, so rebinding its op…" explanation in _load_revision's docstring is no longer needed:

with Operations.context(RuntimeMigrationContext.configure(connection)):
    revision.upgrade()

circulation/tests/migration/test_20260916_58ebd34c5092_empty_coveragerecords.py

Lines 89 to 94 in 664289f

	# setattr, not plain assignment: the module is typed as ModuleType, so
	# mypy does not know about the ``op`` its revision file imports.
	setattr(
	revision, "op", Operations(RuntimeMigrationContext.configure(connection))
	)
	revision.upgrade()

[greptile-apps]

Confidence Score: 5/5

[Critical risk] Database migration empties and prepares to drop the coveragerecords table.

The PR appears safe to merge; no outstanding blocking failure was identified.

Summary

This PR removes ORM relationships and unused helpers that still caused access to the dormant coveragerecords table, while retaining its model for fresh-database compatibility.

• Empties the table before parent deletions stop cleaning up its rows.
• Adds migration tests confirming that the table remains and the temporary lock timeout does not leak into later revisions.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Existing database] --> B[Migration truncates coveragerecords]
  B --> C[Parent ORM relationships removed]
  D[Fresh database] --> E[Retained model creates table]
  C --> F[Table remains for this release]
  E --> F
  F --> G[Stacked follow-up drops table]

Loading

_{Reviews (6) · Last reviewed commit: "Address review: fix lock_timeout rationa..."}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.74%. Comparing base (54de833) to head (664289f).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3744      +/-   ##
==========================================
+ Coverage   93.71%   93.74%   +0.02%     
==========================================
  Files         510      510              
  Lines       46515    46409     -106     
  Branches     6314     6299      -15     
==========================================
- Hits        43593    43507      -86     
+ Misses       1888     1874      -14     
+ Partials     1034     1028       -6     

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

Both points addressed in 8f46bc290, and the branch is rebased onto current main.

1. TRUNCATE lock rationale

Correct, and the comment was worse than imprecise — it contradicted the premise of the PR. It claimed the ACCESS EXCLUSIVE lock was uncontended "because no running code (current or N-1) reads or writes this table", when N-1 reading the table on every parent delete is the entire reason this PR exists.

Rewritten to say what's actually true, including the queueing behaviour (a waiting ACCESS EXCLUSIVE request blocks every later reader, so the stall isn't limited to the in-flight delete), and the guard added as suggested:

def upgrade() -> None:
    op.execute("SET LOCAL lock_timeout = '5s'")
    op.execute("TRUNCATE TABLE coveragerecords")

Confirmed SET LOCAL behaves inside alembic's transaction — the migration test still passes.

2. Dead methods on CoverageRecord

Agreed, including the footgun argument: with the parent relationships gone, a row written by add_for/bulk_add could no longer be cleaned up on parent delete and would just make that delete fail on a foreign key. CoverageRecord is now a bare table definition — columns, __table_args__, and the trailing module-level Index.

Extended slightly past the lines you flagged, after checking each was unreferenced outside coverage.py:

• the five *_OPERATION constants, whose only consumer was assert_coverage_operation
• BaseCoverageRecord.not_covered, plus ALL_STATUSES / PREVIOUSLY_ATTEMPTED / DEFAULT_COUNT_AS_COVERED, which only fed it — and whose test this PR had already deleted, so it was dead and uncovered for the same reason

status_enum and the four status constants stay: they're the column type for both dormant models. autoflake then dropped the imports left unused.

Verification

Re-ran the backwards-compatibility gate on both halves of the stack after these changes:

	result
#3744 vs the #3520 release (7a3274e)	3346 passed ✅
#3521 vs #3744	3391 passed ✅

Plus tests/migration/ + tests/manager/sqlalchemy/ + tests/manager/data_layer/ at 664 (this PR) and 665 (#3521), mypy and pre-commit clean on both. #3521 was rebased onto this branch as well, so the stack still applies cleanly.

Thanks — the second one in particular was a real hazard, not just dead weight.

[dbernstein]

Good catch, and the leak is real — fixed in 15eab5d24.

I confirmed the premise before changing anything: alembic/env.py calls context.configure(...) without transaction_per_migration, then wraps context.run_migrations() in a single context.begin_transaction(), so every pending revision in an alembic upgrade shares one transaction. And I checked the actual scoping against Postgres 16 rather than trusting the docs, since I'd already got this statement wrong once:

BEGIN; SET LOCAL lock_timeout = '5s';
       SELECT current_setting('lock_timeout');   -- 5s   ← leaks to later revisions
BEGIN; SET LOCAL lock_timeout = '5s';
       SET LOCAL lock_timeout = DEFAULT;
       SELECT current_setting('lock_timeout');   -- 0    ← back to default

Applied exactly as suggested:

op.execute("SET LOCAL lock_timeout = '5s'")
op.execute("TRUNCATE TABLE coveragerecords")
# Scope the timeout to the statement above; see the module docstring.
op.execute("SET LOCAL lock_timeout = DEFAULT")

The module docstring now spells out why the reset is there — that it's transaction-scoped rather than revision-scoped, and which two cases would otherwise inherit it (an instance upgrading across this release and the stacked #3521 drop in one pass, or any revision added before this ships) — so it doesn't look redundant to whoever reads it next.

tests/migration/ still passes (20 tests), which also exercises the new statement against real Postgres.

[tdilauro]

The code here looks good. 🎉

There is an assertion about the migration retrying that doesn’t seem correct and what I think is a missing test.

There are a few comments about some of the comments. In some cases they seem excessively long and more detailed that needed or make references to code that has been removed in the PR and, thus, will not be helpful to future readers.

[tdilauro]

the migration fails fast and is retried rather than blocking instance startup.

What retries the migration, if it fails? It seems like there would be no instance startup in that case. It would just fail. In that case we’d need to redeploy it.

[dbernstein]

You are right, and it was worse than imprecise. Nothing retries it — and it does not just get logged past either: initialize_database catches CommandError, but a lock_timeout abort surfaces as OperationalError, which is not caught, so instance init fails and the deploy has to be re-run exactly as you describe.

Reworded to claim only what the timeout actually buys — a bounded, loud failure instead of a TRUNCATE that queues ahead of every reader of the table and stalls them indefinitely:

With it, the statement gives up after 5s and the migration fails -- nothing retries it, so the deploy has to be re-run, which is the better failure: loud and bounded rather than a spreading stall.

[tdilauro]

I don’t think we should reference stuff that went away. This would be a helpful comment on the PR itself, but just adds noise to the codebase.

[dbernstein]

Agreed — that is PR commentary, not something a future reader of this file needs. Cut to a single line:

class BaseCoverageRecord:
    """Holds the ``coverage_status`` enum shared by the two dormant coverage models."""

[tdilauro]

Minor - This probably doesn’t need four paragraphs. But it’s okay if it will all go away in the final PR of this series.

[dbernstein]

Trimmed rather than left for the final PR — same reasoning as your other comment, and it is cheap now. Four paragraphs down to one, keeping only why the class still exists and why it is columns-only, dropping the history of what was removed.

[tdilauro]

I don’t see a test that verifies that the lock timeout gets reset. If it’s not reset, it could affect later migrations.

[dbernstein]

Added, and it is a real one rather than a passing assertion — thank you for pushing on this.

The catch is that a test driven through alembic_runner cannot fail: it commits between revisions, so the setting is discarded regardless of what the migration does. The new test runs upgrade() against a transaction it holds open itself, which is the situation env.py actually creates, then asserts lock_timeout is unchanged.

I checked it fails when the reset is removed, rather than assuming:

>       assert after == before
E       AssertionError: assert 5s == 0

[dbernstein]

Thanks @tdilauro : I will make the suggested changes before merging.