feat(db): move fetch_url.last_fetched_at to a runtime fetch_state table (SA)

[mousebrains]

What

Dataset-separation SA, second sub-slice (AC #6). Relocates the operational timestamp last_fetched_at off the dataset-owned fetch_url table onto a new runtime-only fetch_state table, so engine runtime (levels fetch) no longer mutates a dataset-owned table.

Why

AC #6: "Engine runtime … never create or mutate dataset-owned metadata rows/columns outside sync-metadata or an explicit schema migration; operational timestamps and caches live in runtime tables." fetch_url is projected from the dataset's sources.yaml; fetch.py wrote last_fetched_at into it every run. (It's the exact churn column #145 had to special-case in the OPTIONAL-vs-EXCLUDED distinction.)

Change

• Migration 0076 — CREATE TABLE fetch_state (fetch_url_id PK→fetch_url ON DELETE CASCADE, last_fetched_at) + ALTER TABLE fetch_url DROP COLUMN last_fetched_at. Schema-only (no DML → passes the >0074 schema-only guard). The CREATE TABLE mirrors create_all()'s separate PK/FK-clause form so the migrated schema and the ORM introspect identically (schema-parity). DROP COLUMN is safe on prod (Debian 12+/13 → SQLite ≥ 3.40; the column is in no index/FK).
• models.py — drop FetchUrl.last_fetched_at; add the FetchState runtime model (not dataset-owned; absent from CONTRACT_CSVS, never exported/synced).
• fetch.py — the writer upserts fetch_state by fetch_url id instead of mutating the fetch_url row.
• layout.py — drop the fetch_url entry from EXCLUDED_COLUMNS (only the reach geom/gradient sidecar columns remain); comment sweep in metadata_csv.py / export_metadata.py / import_metadata.py.
• Docs/snapshots — regenerate live_schema.sql + manifest; document fetch_state in database-schema.md (remove the fetch_url.last_fetched_at row); fix the ORM table count 24→25 / 25→26 live in CLAUDE.md / README.md / database-schema.md; regenerate schema-overview.svg (also clears 0075's unknown_station_policy drift); make regenerate_schema_svg.sh portable (BSD mktemp has no --suffix).

No kayak_data change

fetch_url.last_fetched_at was already EXCLUDED from fetch_url.csv, so removing it leaves the CSV and generate-sources --check byte-identical — no dataset PR, no pin bump. init-db creates fetch_state via create_all; migrate creates it via 0076.

Verification

• Full gate green: ruff check, ruff format --check, mypy (CI scope), pytest -m "not slow" = 1384 passed, wheel-smoke.
• Migration on real prod schema: applied 0076 to a copy of the live ../DB — creates fetch_state, drops last_fetched_at, zero FK violations; init-db (create_all) and migrate (0076) produce identical schemas (schema-parity).
• fetch writes fetch_state: new real-DB test on _process_work_item (upserts a single row; fetch_url has no such column).
• Real dataset unaffected: generate-sources --check byte-identical, validate-dataset OK.
• Two-round adversarial self-review — dry: round 1 no correctness bugs (one harmless theoretical TOCTOU note) + doc-drift (counts + stale SVG) which round-1 fixed; round 2 confirmed all fixes correct and complete, no new findings.

Scope / next

No editor/PHP impact (the column had zero readers). Next SA sub-slices (separate PRs): SA-3 CI writer-boundary guard + convert the maintenance-tool writers; then the reverse-sync teardown.

🤖 Generated with Claude Code

[mousebrains]

Adversarial review findings:

• P3 tests/fixtures/live_schema.sql:118 - The regenerated schema fixture introduces trailing spaces on the is_active, unknown_station_policy, and PRIMARY KEY lines. git diff --check 7c7da6cc2e97171ad890da6809262ea38eed9d81..HEAD fails on this branch. Please strip those spaces so the branch stays clean under the standard whitespace check.

• P3 scripts/regenerate_schema_svg.sh:40 - TMP_DB="$(mktemp)".db creates one temp file, then points SQLite at a second path with .db appended. The trap only removes the .db file, so each run leaks the original empty mktemp file. A portable fix is to keep the base path and trap both files, or create a temp directory and put schema.db inside it.

No functional blockers found in the migration/fetch path after live DB verification.

Verification performed:

• uv run --extra dev pytest -q in the PR worktree: 1361 passed, 3 skipped for missing optional geospatial libraries.
• Focused regression suite passed: tests/test_cli/test_fetch.py, tests/test_cli/test_sync_metadata.py, tests/test_db/test_schema_parity.py, tests/test_schema_doc_sync.py, tests/test_scripts/test_migrations_schema_only.py.
• Copied the fresh live DB from /Users/pat/tpw/DB/kayak.db to /private/tmp/kayak-pr147-live.db; precheck showed fetch_url.last_fetched_at present and fetch_state absent, with 115 fetch_url rows.
• Applied 0076 on that live DB copy. Postcheck showed fetch_url.last_fetched_at removed, fetch_state(fetch_url_id, last_fetched_at) present, fetch_url still at 115 rows, fetch_state at 0 rows, PRAGMA foreign_key_check clean, and migrate --check clean.
• gh pr checks 147: all reported checks passing.

[mousebrains]

Adversarial review — PR #147 (move fetch_url.last_fetched_at → runtime fetch_state, SA-2)

This is the highest-stakes PR in the SA batch — a schema migration that drops a column on the live DB — so I verified it against a copy of the fresh ../DB (630 MB), not just the fixture. I found no correctness bug; every load-bearing claim holds on the real prod schema. The only thing to manage is deploy sequencing (a DB-writing deploy).

The critical "is it safe to drop?" checks — all pass

• Zero readers, verified repo-wide. git grep last_fetched_at over non-migration/non-test/non-doc code returns only the writer (fetch.py, now writing FetchState), the new model, and a comment — no PHP, no build, no query reads it. So dropping the column (and not carrying the old values forward) breaks nothing; the next fetch repopulates fetch_state. The "pure churn, no readers" claim is true.
• DROP COLUMN is supported here: host SQLite is 3.46.1 (DROP COLUMN landed in 3.35). And on the real schema last_fetched_at is in no index and no FK (only PRIMARY KEY(id), UNIQUE(url)), so the plain ALTER … DROP COLUMN is safe — no 12-step rebuild.
• Old values lost is intentional and harmless (schema-only migration; no readers; repopulated next run). Confirmed.

Migration 0076 on the real prod schema

Applied 0076 to a copy of the fresh live DB via migrate: fetch_state created, last_fetched_at gone from fetch_url, PRAGMA foreign_key_check clean.

• Schema parity holds — the load-bearing claim. I compared the migrated schema (0076) against a fresh create_all() schema for both tables: fetch_state MATCH and fetch_url MATCH (byte-identical sqlite_master.sql). So a migrated prod DB and a fresh install are indistinguishable — the separate-PK/FK-clause CREATE form did its job.

Runtime + dataset unaffected

• Writer handles the empty table: session.get(FetchState, id) or FetchState(fetch_url_id=id) — INSERTs on the first post-migration fetch (empty fetch_state), UPDATEs after. Correct for the cold-start case.
• Dataset byte-identical: generate-sources --check rc 0 and validate-dataset OK on the real kayak_data. Removing last_fetched_at from both the FetchUrl model and EXCLUDED_COLUMNS leaves expected_columns(fetch_url) unchanged, so fetch_url.csv doesn't move — no kayak_data PR / pin bump needed, as claimed.
• 23 schema-parity + migrate tests pass (incl. the >0074 schema-only guard, which 0076 satisfies — CREATE + DROP, no DML).

Deploy note (operational, not a defect)

Like 0075, this is a DB-writing deploy with a pull→migrate sequencing constraint: after pull, the new code expects fetch_state to exist (the writer does session.get(FetchState, …)), so a pipeline fetch landing between pull and migrate would crash "no such table: fetch_state." Mitigation is the same as #143 — pull+migrate as one chain while the pipeline is idle, with a pre-migrate backup. (Note the inverse of 0075's case: there the DB lacked a column the model needed; here the model drops a column but expects a new table.) No data migration needed — last_fetched_at is intentionally not carried over.

Verdict

Correct, schema-parity-clean, real-prod-verified. The self-review's "harmless theoretical TOCTOU" on the get-or-create is genuinely harmless (fetch is single-process, one session per URL). No findings beyond the standard schema-deploy sequencing. Ready for a careful (backup → pull+migrate) deploy.

🤖 Generated with Claude Code

[mousebrains]

Both P3 findings addressed in 42ec555.

P3 — trailing whitespace in live_schema.sql. Root cause: SQLAlchemy's CREATE TABLE DDL emits a trailing space after every column comma, so the whole fixture has always carried them (157 lines) — regenerating the fetch_url block just surfaced 3 of them in the diff and tripped git diff --check. Fixed durably: regenerate_schema_snapshot.sh now pipes its output through sed 's/[[:space:]]*$//', and I regenerated the fixture — 0 trailing-whitespace lines now, and future regens stay clean (so the next migration author won't hit this). schema-parity is unaffected (the parser re-introspects; whitespace in the DDL text is irrelevant). git diff --check main is now clean.

P3 — SVG temp-file leak. Switched regenerate_schema_svg.sh to mktemp -d (portable on BSD + GNU) with schema.db inside it and trap 'rm -rf "$TMP_DIR"' — no orphaned temp file. (The .svg itself is unchanged: re-running eralchemy only reorders one edge group non-deterministically, so I kept the committed deterministic output rather than churn it.)

Verified: full gate green (ruff/format/mypy, pytest -m "not slow" = 1384, wheel-smoke), git diff --check main clean.

Thanks for the careful real-prod-DB verification of the migration + the deploy-sequencing note (pull+migrate as one chain while the pipeline is idle, with a pre-migrate backup — same as #143/0075). Still not merging — yours.