Keep the library working when its custom-column definitions can't be read

[new-usemame]

Fixes the red main (see "Why now") and a real production 500. For #1153.

Why

get_cc_columns reads a library's custom-column definitions — the extra fields you can add to a book in Calibre. Five surfaces call it: the book detail page (cps/web.py show_book), the detail API, the books table, and both search screens.

Only one of them (the API) had any error handling, and it caught the wrong thing — AttributeError/KeyError/TypeError, while an actually-unreadable schema raises SQLAlchemy's OperationalError (no such table: custom_columns). So an unreadable definitions table took whole pages down with a 500 over a supplementary field, while the rest of the library read perfectly fine.

Reachable whenever the metadata DB opens but its schema isn't readable: a library mid-write, a moved/renamed library path, custom_columns mid-migration, a partially-restored library.

Root cause

The API's guard looked like it worked because calibre_db.session could be None, and None.query(...) raises AttributeError. So the no-session case was caught and the unreadable-schema case was never exercised. Production never had that cushion — there the session is real, the query runs, and it 500s.

Pre-existing, not introduced by #1149. What #1149 changed is that session became a property that always materialises, removing the accidental AttributeError and turning the latent gap into red tests.

Fix

Degrade inside CalibreDB.get_cc_columns itself — catch SQLAlchemyError/AttributeError, log, return [] — rather than copying a try/except to five callsites. All five callers want "the custom columns, if any" and none can act on a DB error; [] is the shape they already render for a library with no custom columns. _detail_custom_columns keeps its own guard as defense-in-depth.

Correction to this PR's first revision: it fixed only the API endpoint and claimed the web.py/search.py callers were deliberately out of scope because a page that can't read the library "has nothing to degrade to". That was wrong — driving the real routes showed /book/<id> still 500'd while the rest of the book rendered fine. The e2e caught what the unit tests didn't.

Why now

main is red and blocks every PR (the docs-only #1152 is red too). Test Suite was green on 0690b42d, failed on cc00a47b:

FAILED tests/unit/test_587_koreader_progress.py::test_detail_endpoint_surfaces_kosync_progress
FAILED tests/unit/test_587_koreader_progress.py::test_detail_endpoint_null_progress_when_unsynced
  → sqlalchemy.exc.OperationalError: (sqlite3.OperationalError) no such table: custom_columns

auto-revert then fired against main and pushed an auto-revert/cc00a47 branch, failing to open the PR — tracked separately in #1155.

These pass in isolation and fail after any test that leaves a session_factory behind, so --dist=loadfile worker grouping decides whether they go red.

Validation

Live, on cwn-local, against a running server whose custom_columns table was renamed out from under it:

Route	Pre-fix	Post-fix
/book/2 (classic book page)	500	200
/api/v1/books/2 (detail API)	500	200
/table	200	200
/advsearch	200	200
/	200	200

Pre-fix 500 traceback pointed at cps/api/books.py:44 in _detail_custom_columns. Post-fix the book renders complete (title, authors, 27 fields) with custom columns omitted and Custom-column definitions unavailable logged (4 warnings across the run). Every route returns 200 again once the table is restored.

Red/green — each fix verified red while uncommitted:

• test_missing_custom_columns_table_does_not_raise → red without the books.py guard.
• test_get_cc_columns_degrades_for_every_caller → red without the db.py guard (this is the one that pins the four unguarded callers).

Full suite (-m "smoke or unit", single process — stricter than CI's --dist=loadfile): 9F/4189P before → 4F/4197P after. The 4 remaining were all failing pre-fix and are unrelated (test_smoke::test_required_directories_exist, test_802_metadata_change_dispatch, test_ingest_batch_dirty, test_metadata_db_write_coordination).

4 regression tests in tests/unit/test_book_detail_custom_columns_degrade.py. Two are contract pins (red/green above); one asserts the fixture reproduces the real raise so the contract tests can't pass vacuously; one pins that session materialises rather than returning None, which is the precondition that removed the old accidental cushion. The fixture saves/restores the class-level session_factory — the bug class here is cross-test pollution, so the test must not add to it.

Not fixed here

• auto-revert workflow can't open PRs, so the red-main safety net produces nothing actionable #1155 — auto-revert can't open PRs (GitHub Actions is not permitted to create or approve pull requests), so the red-main safety net currently produces only an orphan branch. The unblocking toggle also lets Actions approve PRs, which would weaken the main ruleset, so it needs an explicit decision rather than an autopilot flip.
• Stray auto-revert/cc00a47 branch on origin, left in place rather than deleting a branch this autopilot didn't create.

Tier

needs-review — fork-original. Two files, both a single try/except; no dependency, license, or external-URL change.

[new-usemame]

Cross-family review (Terra) — disposition

Reviewed the diff against the anti-slop bar plus a targeted brief on the sharpest risk: whether swallowing a DB error leaves the greenlet's Session unusable for the rest of the request. Terra found no HIGH and no data-loss path. Four findings, all dispositioned:

Severity	Finding	Disposition
MED	except SQLAlchemyError around an autoflushing query can convert an unrelated pending write's IntegrityError into a swallowed [], leaving the session in partial-rollback for the rest of the request. Confirmed as SQLAlchemy behaviour; not reachable from the five current (read-only) callers.	Fixed, but not by narrowing the catch — the read now runs under no_autoflush (31adc8b). Flushing a caller's pending write was never part of what a definitions lookup needs, so this removes the hazard at the source instead of relying on all five callers staying read-only.
MED	Tests prove the return value but not the property that makes degrading worthwhile: that the request can keep querying afterwards. A poisoned session would still satisfy == [].	Fixed — test_request_can_keep_querying_after_the_failure fails the lookup, then reuses the same session and asserts session.is_active.
LOW	test_get_cc_columns_degrades_for_every_caller could pass vacuously if get_cc_columns became an unconditional return [] — which would hide custom columns from every healthy library.	Fixed — test_healthy_library_still_returns_its_custom_columns pins the positive case. Mutation-verified: injecting return [] at the top of get_cc_columns fails exactly that test and no other.
LOW	AttributeError in the except is broad and could mask a programming regression as "no custom columns".	Kept, comment corrected. It's load-bearing: session is None whenever session_factory is (before init_db, or after an explicit session = None), and None.query() is an AttributeError, not a SQLAlchemyError. Terra was right that my original comment mis-described this as the "reconnect window" — reworded to state the actual condition.

Terra's independent check that the session-poisoning scenario is real is worth recording, since it's the reason the no_autoflush change is in rather than waved off: "failed autoflush → session.is_active == False → next SELECT raises PendingRollbackError."

I'd also measured the non-autoflush case directly before the review landed — against a real 42-book library with custom_columns renamed out from under a live session, subsequent queries on that same session returned 42 and the session stayed active. So the plain schema-missing path never poisoned anything; the autoflush path was the only way in, and it's now closed.

Reviewer thread 019f9e82-919e-7501-9824-58975009e988 retained for the re-review leg.

Final suite

-m "smoke or unit", single process (stricter than CI's --dist=loadfile): 9F/4189P before → 3F/4199P after. All 3 remaining were failing pre-fix and are unrelated: test_smoke::test_required_directories_exist, test_802_metadata_change_dispatch, test_ingest_batch_dirty.