From a2d38cbc5f489e09cb68bfd40babac296af1b67f Mon Sep 17 00:00:00 2001 From: vidaks <39301796+vidaks@users.noreply.github.com> Date: Tue, 16 Jun 2026 15:21:25 +0200 Subject: [PATCH] fix: close DB connections, honor path env vars, gate CI on tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A design/architecture/implementation review surfaced a set of small but real findings; this addresses them together. - Store leaked SQLite connections: _conn() used the bare sqlite3 context manager, which commits but never closes the handle — an fd leak and a ResourceWarning storm in the long-running maintain process. _conn is now a contextmanager that commits AND closes. - COLOPHON_DB and COLOPHON_REPORTS were documented (README, .env.example) but never read; the changelog DB and reports always used repo-relative paths. Both now override their defaults (unset => unchanged behaviour). The five duplicated report-directory blocks in the CLI collapsed into one helper. - CI compiled and linted but never ran the 69-test network-free suite, despite the docs stating it gates CI. Added the unittest step across Python 3.9-3.12. - Grimmory.token() let a raw urllib error (a bad X-Edda-Proxy-Auth secret or an unreachable server) escape past the CLI's GrimmoryError/PreconditionError handler as a traceback; it is now a GrimmoryError naming the endpoint. - Corrected the dedup docs: the README/CHANGELOG described an automated "collapse duplicate records" feature that was never built. The audit only identifies duplicate groups and suggests a keeper. - Removed a dead, misleading MAX_PER_RUN constant from heal.py (the per-run cap lives in backfill.py; heal_book enforces none). Co-authored-by: Claude Opus 4.8 --- .github/workflows/ci.yml | 2 ++ CHANGELOG.md | 25 +++++++++++++++++++++++-- README.md | 11 ++++++----- colophon/cli.py | 29 ++++++++++++++--------------- colophon/grimmory.py | 8 +++++++- colophon/heal.py | 2 -- colophon/store.py | 16 ++++++++++++++-- 7 files changed, 66 insertions(+), 27 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9733646..5b6afc3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,3 +26,5 @@ jobs: run: | python -m pip install --quiet ruff ruff check --output-format=github . + - name: Tests (stdlib unittest, network-free) + run: python -m unittest discover -s tests -v diff --git a/CHANGELOG.md b/CHANGELOG.md index b63549d..54a2ca6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,27 @@ All notable changes to this project are documented here. The format is based on `resolver`; no writes, no grimmory DB at runtime. Exits 0/3/4 (match/mismatch/unverifiable). `epub.inspect` now also returns `opf_author` (dc:creator). +### Changed +- **CI now runs the test suite.** The workflow compiled and linted but never ran the + 69-test (network-free) suite, despite the docs stating it gates CI; added the + `unittest discover` step across Python 3.9–3.12. +- **`COLOPHON_DB` and `COLOPHON_REPORTS` are now honored.** Both were documented in the + README and `.env.example` but ignored — the changelog DB and reports always landed at + repo-relative paths. They now override those defaults (unset ⇒ unchanged behavior). The + five duplicated report-directory blocks in the CLI collapsed into one helper. + +### Fixed +- **Store no longer leaks SQLite connections.** `_conn()` used the bare `sqlite3` + context manager, which commits but never closes — an fd leak and a `ResourceWarning` + storm in the long-running `maintain` process. It now commits *and* closes. +- **Token-mint failures surface cleanly.** A bad `X-Edda-Proxy-Auth` secret or an + unreachable server raised a raw `urllib` error past the CLI's handler; it is now a + `GrimmoryError` with the endpoint and cause. +- **Corrected the dedup docs.** The README and CHANGELOG described "collapses duplicate + records / moves the file onto the keeper" — a feature that was never built. The audit + *identifies* duplicate groups and suggests a keeper; no automated dedup command exists + (Colophon never deletes or merges records). + ## [0.2.0] — 2026-06-04 ### Added @@ -70,8 +91,8 @@ Initial public release. only above a confidence threshold. - **Series audit** — compare series numbers against the provider's authoritative position and heal genuine mismatches (read-only by default). -- **Dedup** — collapse duplicate records via attach-to-keeper (loser's file preserved as an - alternative format; empty record removed). +- **Dedup (audit-only)** — the audit groups duplicate records and suggests a keeper; no + automated collapse (Colophon never deletes or merges records). - **Oversight** — weekly changelog review (oscillation + error-rate verdict) that emails only when flagged. - Precondition gate (files never touched), SQLite changelog with `revert`, dry-run default, diff --git a/README.md b/README.md index a5e9a70..114d5e4 100644 --- a/README.md +++ b/README.md @@ -41,8 +41,9 @@ lock pins the edition, so a healed book *stays* healed (set-once, no oscillation authoritative series, and heals genuine mismatches: a wrong/missing number, an ungrouped book (no series name), or a variant series name where the title still matches the provider. A true mis-seed (name *and* title disagree) is left for the resolver. Read-only by default; runs nightly. -- **Dedup** — collapses duplicate records, moving the loser's file onto the keeper as an - *alternative format* (nothing deleted from disk) and removing the empty record. +- **Dedup (audit-only)** — the `audit` report groups duplicate records and names a + suggested keeper (largest file; newest on a tie). Collapsing them is left to you — + Colophon never deletes or merges records on its own. - **Oversight** — a weekly changelog review that flags drift (a book healed more than once = convergence failing; sustained error rate) and emails you *only when flagged*. @@ -127,9 +128,9 @@ the safety net (there is no human approval gate): - **Bounded blast radius** — per-run limits + a circuit-breaker that stops on repeated errors. ⚠️ **Two things to know:** -1. **Dedup is not `revert`-able.** It moves a file and deletes a record; the loser's bytes - survive as an alternative format on the keeper, but there is no changelog undo. Re-import - if you need the separate record back. +1. **Not every write is `revert`-able.** Metadata *heals* are — that is what `revert` + replays. `enrich` is a missing-only refresh that fills empty fields on unidentified + books; it is additive and not changelog-tracked, so `revert` does not cover it. 2. **Data leaves your machine.** `resolve` and `series-audit` send book titles/authors to Hardcover and (for `resolve`) Anthropic. Don't run it on data you can't share with them. diff --git a/colophon/cli.py b/colophon/cli.py index 7b40e83..6f65d2b 100644 --- a/colophon/cli.py +++ b/colophon/cli.py @@ -27,6 +27,15 @@ def _fmt(snap, keys=("title", "series_name", "series_number", "isbn_13", return " | ".join(f"{k}={snap.get(k)}" for k in keys) +def _reports_dir(): + """Directory reports are written to (created if missing). COLOPHON_REPORTS + overrides the repo-relative default.""" + d = os.environ.get("COLOPHON_REPORTS") or os.path.abspath( + os.path.join(os.path.dirname(__file__), os.pardir, "reports")) + os.makedirs(d, exist_ok=True) + return d + + def cmd_precheck(args, g, store): ok, details = g.preconditions() print("preconditions (book files must never be touched):") @@ -146,9 +155,7 @@ def cmd_enrich(args, g, store): def cmd_audit(args, g, store): res = run_audit(limit=args.limit) report = render_report(res) - reports_dir = os.path.abspath(os.path.join(os.path.dirname(__file__), os.pardir, "reports")) - os.makedirs(reports_dir, exist_ok=True) - path = os.path.join(reports_dir, f"audit-{time.strftime('%Y%m%dT%H%M%S')}.md") + path = os.path.join(_reports_dir(), f"audit-{time.strftime('%Y%m%dT%H%M%S')}.md") with open(path, "w") as f: f.write(report) print(report) @@ -172,9 +179,7 @@ def cmd_resolve(args, g, store): print(e) return 2 report = render(res) - reports_dir = os.path.abspath(os.path.join(os.path.dirname(__file__), os.pardir, "reports")) - os.makedirs(reports_dir, exist_ok=True) - path = os.path.join(reports_dir, f"resolve-{time.strftime('%Y%m%dT%H%M%S')}.md") + path = os.path.join(_reports_dir(), f"resolve-{time.strftime('%Y%m%dT%H%M%S')}.md") with open(path, "w") as f: f.write(report) print(report) @@ -190,9 +195,7 @@ def cmd_series_audit(args, g, store): print(e) return 2 report = render(res) - reports_dir = os.path.abspath(os.path.join(os.path.dirname(__file__), os.pardir, "reports")) - os.makedirs(reports_dir, exist_ok=True) - path = os.path.join(reports_dir, f"series-{time.strftime('%Y%m%dT%H%M%S')}.md") + path = os.path.join(_reports_dir(), f"series-{time.strftime('%Y%m%dT%H%M%S')}.md") with open(path, "w") as f: f.write(report) print(report) @@ -204,9 +207,7 @@ def cmd_oversight(args, g, store): from . import oversight res = oversight.review(store, days=args.days) report = oversight.render(res) - reports_dir = os.path.abspath(os.path.join(os.path.dirname(__file__), os.pardir, "reports")) - os.makedirs(reports_dir, exist_ok=True) - path = os.path.join(reports_dir, f"oversight-{time.strftime('%Y%m%dT%H%M%S')}.md") + path = os.path.join(_reports_dir(), f"oversight-{time.strftime('%Y%m%dT%H%M%S')}.md") with open(path, "w") as f: f.write(report) print(report) @@ -228,9 +229,7 @@ def cmd_maintain(args, g, store): res = M.run_maintain(g, store, limit=args.limit, min_conf=args.min_conf, apply=args.apply, force=args.force) body = M.render_summary(res) - reports_dir = os.path.abspath(os.path.join(os.path.dirname(__file__), os.pardir, "reports")) - os.makedirs(reports_dir, exist_ok=True) - path = os.path.join(reports_dir, f"maintain-{time.strftime('%Y%m%dT%H%M%S')}.md") + path = os.path.join(_reports_dir(), f"maintain-{time.strftime('%Y%m%dT%H%M%S')}.md") with open(path, "w") as f: f.write(body) print(body) diff --git a/colophon/grimmory.py b/colophon/grimmory.py index c8890d3..0ac3c2a 100644 --- a/colophon/grimmory.py +++ b/colophon/grimmory.py @@ -53,7 +53,13 @@ def token(self): f"{self.base}/auth/remote", headers=headers, ) - self._token = json.load(urllib.request.urlopen(req, timeout=30)).get("accessToken") + try: + with urllib.request.urlopen(req, timeout=30) as r: + self._token = json.load(r).get("accessToken") + except (urllib.error.HTTPError, urllib.error.URLError) as e: + # A bad proxy-auth secret or unreachable server surfaces here; make it a + # clean GrimmoryError (the CLI handler catches that, not raw urllib errors). + raise GrimmoryError(f"token mint failed at {self.base}/auth/remote: {e}") if not self._token: raise GrimmoryError("failed to mint admin token (Remote-User auth)") return self._token diff --git a/colophon/heal.py b/colophon/heal.py index a6dfa81..b2cf045 100644 --- a/colophon/heal.py +++ b/colophon/heal.py @@ -7,8 +7,6 @@ from .grimmory import snapshot, signature, wait_for_change from .store import Store -MAX_PER_RUN = 50 # rate-limit: refuse to change more than this in one run - class PreconditionError(Exception): pass diff --git a/colophon/store.py b/colophon/store.py index 0dc7253..c1eb698 100644 --- a/colophon/store.py +++ b/colophon/store.py @@ -8,8 +8,12 @@ import os import sqlite3 import time +from contextlib import contextmanager -DB_PATH = os.path.join(os.path.dirname(__file__), os.pardir, "colophon.db") +# Repo-relative by default; COLOPHON_DB overrides it (e.g. a persistent path for a +# deployed service). Unset keeps the historical /colophon.db location. +DB_PATH = os.environ.get("COLOPHON_DB") or os.path.join( + os.path.dirname(__file__), os.pardir, "colophon.db") class Store: @@ -87,10 +91,18 @@ def bump_epoch(self): "ON CONFLICT(key) DO UPDATE SET value=excluded.value", (str(e),)) return e + @contextmanager def _conn(self): + """A connection that commits on clean exit and ALWAYS closes — the bare + sqlite3 context manager commits but leaks the handle (fd leak + a noisy + ResourceWarning in the long-running maintain process).""" c = sqlite3.connect(self.path) c.row_factory = sqlite3.Row - return c + try: + yield c + c.commit() + finally: + c.close() @staticmethod def new_run_id():