Add uncovered_release_search_markers table (migration 0156) - #2158
Merged
Conversation
jakebromberg
force-pushed
the
uncovered-release-list-schema
branch
from
August 15, 2026 00:55
4fa1d9a to
94c5636
Compare
Schema constraint shape reportProbed: This PR adds:
one or more probes failed; see above. The check status is non-blocking. |
jakebromberg
added a commit
that referenced
this pull request
Aug 16, 2026
…d record who deleted (BS#2112) Second review round on DELETE /library/:id. Eight findings, all of which end in either a 500 on a routine release, destroyed play history, or an audit trail nobody can read. The endpoint 500'd on any binned release. The transitive play-count guard's predicate is `rotation_id IN (...) AND album_id IS DISTINCT FROM $1`, and the only index touching `flowsheet.rotation_id` — `flowsheet_rotation_no_match_idx`, migration 0132 — is partial on `metadata_status = 'enriched_no_match'`. That predicate isn't implied, so the planner cannot use it and falls back to a sequential scan of the ~2.6M-row / ~1.7 GB heap, past the 5 s DB_STATEMENT_TIMEOUT_MS, while this transaction holds FOR UPDATE on the library row and every one of its rotation rows. The ON DELETE SET NULL action on `flowsheet.rotation_id` does its own unindexed lookup per cascaded rotation row on top of that. Migration 0148 adds the general partial index `ON flowsheet (rotation_id) WHERE rotation_id IS NOT NULL`, following this table's established pattern: declared in schema.ts, shipped with IF NOT EXISTS and NOT CONCURRENTLY (Drizzle wraps each migration in a transaction), with the out-of-band CREATE INDEX CONCURRENTLY recipe an operator runs on prod first so the apply is a no-op. The narrower sibling stays — it is an order of magnitude smaller and serves a cron query where that selectivity earns its keep. The denylist had a TOCTOU inside library-etl. `loadDeleteDenylist` snapshots the table once at the top of a transaction that then runs for the length of the import, and `db.transaction` is READ COMMITTED — so a delete committing mid-run is invisible to that Set while every later statement sees a fresh snapshot. `findExistingRelease` then finds no row and the INSERT branch resurrects the release under a new `library.id` stripped of its cascaded dependents. The resulting state was terminal and self-concealing: every later run consults the denylist, finds the id, and continues, so the resurrected row was never updated or removed while the log reported "skipped as deleted". There are now three checks. The run-start Set stays as a free pre-filter; `isDeniedAtWriteTime` re-reads per release at the point of write, ahead of `findExistingRelease` rather than merely ahead of the INSERT, because that call's canonical-tuple match can back-stamp a deleted release's legacy id onto a different row; and `reconcileDenylistedInserts` sweeps after the loop, deleting only rows this same uncommitted transaction inserted (nothing has seen them, no dependent can have accrued) and reporting — never repairing — anything older, with a non-zero exit so the state stops hiding. The sweep also runs on idle passes, which are the common case. The documented un-delete recipe could not work. Both `jobs/library-etl/README.md` and the schema.ts docstring told an operator to clear the denylist row and let the next run re-import. `buildReleaseQuery` filters `WHERE lr.TIME_LAST_MODIFIED > <last run>` and a Backend-side delete never touches tubafrenzy, so the upstream timestamp is older than every subsequent watermark and the release is never re-selected. The recipe now clears the denylist row and then either takes an upstream edit in /wxycdb (preferred, re-selects exactly that release) or drops the `cronjob_runs` watermark to force one full re-sync — verified against `getLastRunTimestamp` returning null and `buildReleaseQuery` emitting no delta predicate — plus a query to confirm the restore landed, since the failure mode is silent. The same mistake ran the other way through every "returns within 30 minutes" claim: the resurrection is triggered by an upstream edit or a full re-sync, not by the clock, so the exposure is open-ended rather than imminent. Corrected in the schema docstring, both READMEs, the service and controller docstrings, and the integration spec's header. Nothing recorded who deleted. `catalog:write` is held by two roles, so a denylist row naming only the release and the timestamp leaves incident response unable to separate a legitimate deletion from an abusive one. Migration 0149 adds three nullable attribution columns; the controller threads `req.auth`, falling back to the JWT `sub` claim, and a thin token (AUTH_BYPASS, no `id`) costs the audit trail rather than the delete. A structured log line and a Sentry breadcrumb carry the same fact into the request log, where a responder looking at a time window rather than a release id will meet it. Plays linked only by `flowsheet.legacy_release_id` counted as zero. The tubafrenzy webhook writes that column and `jobs/legacy-linkage-resolve` turns it into an `album_id` later; in that window both existing counts read zero, and deleting is worse than blanking — the denylist guarantees no future library row carries that legacy id, so the resolver can never link them. The refusal now counts a disjoint third arm and the 409 body breaks it out. It cannot be locked: with no FK there is no RI check to conflict with, so a webhook INSERT landing inside the guard is invisible. That residual is one statement wide, one-sided, and documented in both the docstring and app.yaml rather than papered over. The resolver's own UPDATE is covered — setting `album_id` fires the FK check, which takes FOR KEY SHARE on the row this transaction holds FOR UPDATE. The lock-order inversion is bounded rather than reasoned about. This transaction takes library-then-rotation; a flowsheet INSERT carrying both columns takes the same two locks via its RI checks in constraint-OID order, which today agrees (album_id's FK predates rotation_id's, migration 0097) but is an accident, not an invariant — migration 0147 in this same PR drops and re-adds a constraint, which is exactly the operation that reorders them. Rather than depend on OIDs, the transaction sets `lock_timeout` to 750 ms, deliberately below the default 1 s `deadlock_timeout`, so it gives up before the deadlock detector runs and this side is always the one that yields: the librarian gets a retryable 503, never a DJ an aborted play insert or a 35 s hang. `40P01` maps the same way for a non-default `deadlock_timeout`. 503, not 409 — 409 on this endpoint means refused on the merits, and this refusal says nothing about the release. `library_identity_history.library_id` is left dangling deliberately, and now says so. It is the other FK-less reference to `library.id`, and a supersedure audit log has to outlive the row it describes or cascading destroys exactly the record an auditor came for. The table docstring states what a reader should expect to find (a LEFT JOIN that misses means hard-deleted, corroborated by the denylist row), that an orphan scan must exclude it, and that `jobs/library-call-number-dedup` repoints rather than orphans it on a merge — so a delete is the only source of dangling ids. Pinned by an integration test so a later orphan cleanup has to argue with it. `jobs/library-call-number-dedup`'s README said `artist_library_crossreference.library_id` was `no action` and that nothing since had altered it — migration 0147 alters exactly that. The reference-site table, the counts, and that sentence are corrected. The direction of the trade is written down in merge.ts: under `no action` an incomplete repoint of that table failed loudly (the survivor's DELETE raised an FK violation and the slot rolled back); under `cascade` the same bug fails silently, so the delete-ordering argument is now a correctness property rather than a defensive habit. `uncovered_release_search_markers` is deliberately absent from the dependent enumeration — that table arrives with #2158, which is not merged. Follow-up once it lands.
7 tasks
jakebromberg
added a commit
that referenced
this pull request
Aug 17, 2026
…d record who deleted (BS#2112) Second review round on DELETE /library/:id. Eight findings, all of which end in either a 500 on a routine release, destroyed play history, or an audit trail nobody can read. The endpoint 500'd on any binned release. The transitive play-count guard's predicate is `rotation_id IN (...) AND album_id IS DISTINCT FROM $1`, and the only index touching `flowsheet.rotation_id` — `flowsheet_rotation_no_match_idx`, migration 0132 — is partial on `metadata_status = 'enriched_no_match'`. That predicate isn't implied, so the planner cannot use it and falls back to a sequential scan of the ~2.6M-row / ~1.7 GB heap, past the 5 s DB_STATEMENT_TIMEOUT_MS, while this transaction holds FOR UPDATE on the library row and every one of its rotation rows. The ON DELETE SET NULL action on `flowsheet.rotation_id` does its own unindexed lookup per cascaded rotation row on top of that. Migration 0148 adds the general partial index `ON flowsheet (rotation_id) WHERE rotation_id IS NOT NULL`, following this table's established pattern: declared in schema.ts, shipped with IF NOT EXISTS and NOT CONCURRENTLY (Drizzle wraps each migration in a transaction), with the out-of-band CREATE INDEX CONCURRENTLY recipe an operator runs on prod first so the apply is a no-op. The narrower sibling stays — it is an order of magnitude smaller and serves a cron query where that selectivity earns its keep. The denylist had a TOCTOU inside library-etl. `loadDeleteDenylist` snapshots the table once at the top of a transaction that then runs for the length of the import, and `db.transaction` is READ COMMITTED — so a delete committing mid-run is invisible to that Set while every later statement sees a fresh snapshot. `findExistingRelease` then finds no row and the INSERT branch resurrects the release under a new `library.id` stripped of its cascaded dependents. The resulting state was terminal and self-concealing: every later run consults the denylist, finds the id, and continues, so the resurrected row was never updated or removed while the log reported "skipped as deleted". There are now three checks. The run-start Set stays as a free pre-filter; `isDeniedAtWriteTime` re-reads per release at the point of write, ahead of `findExistingRelease` rather than merely ahead of the INSERT, because that call's canonical-tuple match can back-stamp a deleted release's legacy id onto a different row; and `reconcileDenylistedInserts` sweeps after the loop, deleting only rows this same uncommitted transaction inserted (nothing has seen them, no dependent can have accrued) and reporting — never repairing — anything older, with a non-zero exit so the state stops hiding. The sweep also runs on idle passes, which are the common case. The documented un-delete recipe could not work. Both `jobs/library-etl/README.md` and the schema.ts docstring told an operator to clear the denylist row and let the next run re-import. `buildReleaseQuery` filters `WHERE lr.TIME_LAST_MODIFIED > <last run>` and a Backend-side delete never touches tubafrenzy, so the upstream timestamp is older than every subsequent watermark and the release is never re-selected. The recipe now clears the denylist row and then either takes an upstream edit in /wxycdb (preferred, re-selects exactly that release) or drops the `cronjob_runs` watermark to force one full re-sync — verified against `getLastRunTimestamp` returning null and `buildReleaseQuery` emitting no delta predicate — plus a query to confirm the restore landed, since the failure mode is silent. The same mistake ran the other way through every "returns within 30 minutes" claim: the resurrection is triggered by an upstream edit or a full re-sync, not by the clock, so the exposure is open-ended rather than imminent. Corrected in the schema docstring, both READMEs, the service and controller docstrings, and the integration spec's header. Nothing recorded who deleted. `catalog:write` is held by two roles, so a denylist row naming only the release and the timestamp leaves incident response unable to separate a legitimate deletion from an abusive one. Migration 0149 adds three nullable attribution columns; the controller threads `req.auth`, falling back to the JWT `sub` claim, and a thin token (AUTH_BYPASS, no `id`) costs the audit trail rather than the delete. A structured log line and a Sentry breadcrumb carry the same fact into the request log, where a responder looking at a time window rather than a release id will meet it. Plays linked only by `flowsheet.legacy_release_id` counted as zero. The tubafrenzy webhook writes that column and `jobs/legacy-linkage-resolve` turns it into an `album_id` later; in that window both existing counts read zero, and deleting is worse than blanking — the denylist guarantees no future library row carries that legacy id, so the resolver can never link them. The refusal now counts a disjoint third arm and the 409 body breaks it out. It cannot be locked: with no FK there is no RI check to conflict with, so a webhook INSERT landing inside the guard is invisible. That residual is one statement wide, one-sided, and documented in both the docstring and app.yaml rather than papered over. The resolver's own UPDATE is covered — setting `album_id` fires the FK check, which takes FOR KEY SHARE on the row this transaction holds FOR UPDATE. The lock-order inversion is bounded rather than reasoned about. This transaction takes library-then-rotation; a flowsheet INSERT carrying both columns takes the same two locks via its RI checks in constraint-OID order, which today agrees (album_id's FK predates rotation_id's, migration 0097) but is an accident, not an invariant — migration 0147 in this same PR drops and re-adds a constraint, which is exactly the operation that reorders them. Rather than depend on OIDs, the transaction sets `lock_timeout` to 750 ms, deliberately below the default 1 s `deadlock_timeout`, so it gives up before the deadlock detector runs and this side is always the one that yields: the librarian gets a retryable 503, never a DJ an aborted play insert or a 35 s hang. `40P01` maps the same way for a non-default `deadlock_timeout`. 503, not 409 — 409 on this endpoint means refused on the merits, and this refusal says nothing about the release. `library_identity_history.library_id` is left dangling deliberately, and now says so. It is the other FK-less reference to `library.id`, and a supersedure audit log has to outlive the row it describes or cascading destroys exactly the record an auditor came for. The table docstring states what a reader should expect to find (a LEFT JOIN that misses means hard-deleted, corroborated by the denylist row), that an orphan scan must exclude it, and that `jobs/library-call-number-dedup` repoints rather than orphans it on a merge — so a delete is the only source of dangling ids. Pinned by an integration test so a later orphan cleanup has to argue with it. `jobs/library-call-number-dedup`'s README said `artist_library_crossreference.library_id` was `no action` and that nothing since had altered it — migration 0147 alters exactly that. The reference-site table, the counts, and that sentence are corrected. The direction of the trade is written down in merge.ts: under `no action` an incomplete repoint of that table failed loudly (the survivor's DELETE raised an FK violation and the slot rolled back); under `cascade` the same bug fails silently, so the delete-ordering argument is now a correctness property rather than a defensive habit. `uncovered_release_search_markers` is deliberately absent from the dependent enumeration — that table arrives with #2158, which is not merged. Follow-up once it lands.
jakebromberg
force-pushed
the
uncovered-release-list-schema
branch
from
August 23, 2026 04:46
94c5636 to
6ba4c79
Compare
jakebromberg
changed the title
Member
Author
|
Rebased onto current The migration was regenerated rather than hand-renamed, and the resulting DDL is byte-identical to what One conflict was semantic, not textual. CI green. Title updated to match the new number. Note: this stack currently sits on a red |
jakebromberg
force-pushed
the
uncovered-release-list-schema
branch
from
August 23, 2026 17:23
6ba4c79 to
9b45d4b
Compare
The "searched, found nothing" marker for the uncovered-release-list search handoff (BS#1877, ADR 0013). One row per `library.id` that has ever been included in a published `uncovered-releases.jsonl` snapshot — written when a release is handed off to WXYC/research-data for search, not after any confirmation the search happened or found something. Chosen over a `source_key` convention on `album_critic_reviews` because every column that table's UPSERT natural key would need to carry — `source`, `source_url`, `snippet` — is NOT NULL and semantically "this IS a review." A dedicated table keeps "has a review" and "already handed off for search" as two independently-anti-joined predicates without inventing sentinel review rows in the table `GET /proxy/metadata/album` reads. `ON DELETE CASCADE` off `library` mirrors `album_critic_reviews`, which makes the new table the 14th FK site into `library.id` — so it is registered in `jobs/library-call-number-dedup`'s FK_TARGETS (`uniqueKey: ['album_id']`) and in that job's enforced-fk-actions assertion, which introspects `information_schema` for every inbound FK. Without the registration a merge would let the marker cascade away instead of repointing it, and the integration spec fails on the unregistered site. Schema only — no reader or writer. The job that consumes this table lands separately; the schema.ts doc comment forward-references it. Renumbered 0146 -> 0156 on rebase: main landed its own 0146 (library-delete-denylist) while this sat open, so both branches had claimed the number. The migration was regenerated rather than hand-renamed, and the resulting DDL is byte-identical to what 0146 carried; only the number and a note in the file header changed. The rebase also resolved a real conflict in `jobs/library-call-number-dedup/README.md`, not just a textual one. This branch's copy listed `artist_library_crossreference` as `no action`; main's lists it as `cascade`, because migration 0147 (BS#2112) repaired that constraint in the interim. Main is correct, so the merged table keeps `cascade` there and adds `uncovered_release_search_markers` alongside it — 14 sites, seven cascading, up from 13 and six. Refs #1877.
….id reference sites The README's reference-site inventory is what the merge's delete-ordering argument rests on, and it was one site short. `library_delete_denylist.library_id` arrived with migration 0146 — the same 0146 this branch had to renumber around, which is why the collision was noticed and the new row was not. It belongs in the no-FK row: keyed on `legacy_release_id`, its `library_id` is explicitly informational, recording the id the row carried at delete time. So a merge that deletes the losing `library` row leaves it alone by design, and that is correct — the denylist's one consumer, `jobs/library-etl`'s import loop, reads only `legacy_release_id`. Nothing to repoint; the entry exists so the inventory is complete rather than merely correct. No test change. `enforced-fk-actions` reads `information_schema.referential_constraints`, so a column with no FK cannot appear there in either direction — including the catalog-to-code pass that exists to catch omissions. That is precisely why the no-FK sites are tracked only in this table, and why this one could drift silently.
jakebromberg
force-pushed
the
uncovered-release-list-schema
branch
from
August 23, 2026 17:27
9b45d4b to
48b09d4
Compare
Member
Author
|
Rebased onto current
That matters because Also in this push: the README's Verified locally: Merge note: GitHub routes stacked PRs away from the sync merge endpoint, and |
This was referenced Aug 23, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds the
uncovered_release_search_markerstable (migration 0146) — the "searched, found nothing" marker for the uncovered-release search handoff (ADR 0013, #1877).Schema only. No reader, no writer, no job. The job that consumes this table lands in the next PR of the stack;
schema.ts's doc comment forward-references it.One row per
library.idthat has ever been included in a publisheduncovered-releases.jsonlsnapshot — written the moment a release is handed off toWXYC/research-datafor search, not after any confirmation that a search happened or found something. Semantics are deliberately publish-once, never retried.Why a dedicated table
The ADR's other named option was a
source_keyconvention onalbum_critic_reviews. Every column that table's UPSERT natural key would need to carry —source,source_url,snippet— isNOT NULLand semantically "this IS a review." Recording "we looked, found nothing" there means inventing sentinel values for columns whose whole contract is a real review's attribution, in the exact tableGET /proxy/metadata/albumreads.A separate table keeps "has a review" and "already handed off for search" as two independently-anti-joined predicates, never conflated.
handoff_count/last_handed_off_atare audit-only — they should stay at1/== first_handed_off_atunder normal operation, and exist so a re-inclusion bug is visible in the data rather than silent.The FK registration is the non-obvious half
ON DELETE CASCADEofflibrarymirrorsalbum_critic_reviews— which makes this the 14th FK site intolibrary.id, so it is registered injobs/library-call-number-dedup'sFK_TARGETS(uniqueKey: ['album_id']), in that job's README reference-site table, and in theenforced-fk-actionsintegration assertion, which introspectsinformation_schemafor every inbound FK rather than trustingschema.ts.Without the registration, a call-number merge would let the marker cascade away instead of repointing it to the survivor — and the integration spec fails on the unregistered site.
Why the diff looks big
9 files / ~6,616 insertions, but 6,509 of those are
shared/database/src/migrations/meta/0146_snapshot.json— drizzle-kit's auto-generated full-schema snapshot that ships with every migration in this repo (see 0125–0145 for precedent). The hand-written surface is 107 lines.Stack
This is the base of a three-PR chain, split out of the original #1879 (3,137 reviewable lines) so the generated snapshot doesn't sit on top of reviewable code:
Closes #1877.Closes #2157.Merge in order; the stack is rebase-merged.
Test plan
npm run lint:migrations— clean (journalwhen= previous + 1, frozen hash present).npm run typechecknpm run lint— 0 errorsnpm run format:checknpm run test:unitnpm run ci:testmock— includestests/integration/library-call-number-dedup-merge.spec.js, whoseenforced-fk-actionscase asserts the new site'sCASCADEagainst the live catalog.Refs #1877.