From 8e98e7996828ae95f7a30df9fc779f4946b637fc Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sun, 7 Jun 2026 23:58:14 -0400 Subject: [PATCH 1/2] fix(alembic): lock_timeout on migrations, drop the advisory lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverses the advisory-lock approach (7309d1d) — it treated a replica race that wasn't the cause and added a new indefinite-hang mode (a sibling/stale migrator holding the xact lock). Real cause of the 0040 hang (operator-diagnosed 2026-06-07): web has always been a single replica. The migration's ALTER series_page queued behind a concurrent tag-merge that held a series_page lock for minutes — _do_merge repoints series_page then runs _create_protective_aliases, an unindexed full scan of image_record (JSON column, ~59k rows). Migrations ran with no lock_timeout, so the DDL hung indefinitely and silently. Fix: SET lock_timeout (default 30s, env-overridable) on the migration connection before alembic's transaction. A blocked DDL now fails fast with 'canceling statement due to lock timeout'; the entrypoint exits non-zero so the deploy retries / surfaces loudly instead of wedging. General protection for every future migration. (The slow _create_protective_aliases scan — the actual lock holder — is the separate perf fix still under discussion.) Co-Authored-By: Claude Opus 4.8 (1M context) --- alembic/env.py | 43 +++++++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/alembic/env.py b/alembic/env.py index b657465..f9d8164 100644 --- a/alembic/env.py +++ b/alembic/env.py @@ -1,5 +1,7 @@ """Alembic environment — reads DATABASE_URL from app config.""" +import os +import re from logging.config import fileConfig from sqlalchemy import engine_from_config, pool, text @@ -8,14 +10,18 @@ from alembic import context from backend.app.config import get_config from backend.app.models import Base -# Arbitrary fixed 64-bit key for the session/transaction advisory lock that -# serializes concurrent `alembic upgrade head` runs. Every `web` replica runs -# migrations in its entrypoint, so under `docker stack deploy` two replicas can -# boot at once and race the same DDL — duplicate CREATE TABLE, then a crashed -# replica (operator-flagged 2026-06-07: 0040 raced; one backend died with -# AdminShutdown). The first replica to reach the lock migrates; the rest block, -# then find the version table already at head and apply nothing. -_MIGRATION_LOCK_KEY = 0xFCA1E35C +# Fail a blocked migration FAST instead of hanging forever. Migrations run +# against the live DB while workers hold locks; 0040's `ALTER series_page` queued +# behind a tag-merge that held a series_page lock for minutes (the merge runs an +# unindexed full scan over image_record while repointing series_page) and hung +# with no timeout — silent, indefinite (operator-flagged 2026-06-07). With a +# lock_timeout a blocked DDL errors ("canceling statement due to lock timeout") +# and the entrypoint's `alembic upgrade head` exits non-zero, so the deploy +# retries / surfaces loudly rather than wedging. Override via env when a known +# slow-lock window is expected. +_MIGRATION_LOCK_TIMEOUT = os.environ.get("MIGRATION_LOCK_TIMEOUT", "30s") +if not re.fullmatch(r"\d+\s*(ms|s|min)?", _MIGRATION_LOCK_TIMEOUT.strip()): + _MIGRATION_LOCK_TIMEOUT = "30s" # ignore a malformed override config = context.config @@ -47,24 +53,21 @@ def run_migrations_online() -> None: poolclass=pool.NullPool, ) with connectable.connect() as connection: + # Session-level lock_timeout for every DDL statement in this run. Set + # (and commit) before alembic opens its own transaction so the GUC + # persists on this connection regardless of how alembic structures its + # transactions. Value is from our own env, so f-string interpolation is + # safe (and it's been pattern-validated above); SET takes no bind params. + connection.execute( + text(f"SET lock_timeout = '{_MIGRATION_LOCK_TIMEOUT}'") + ) + connection.commit() context.configure( connection=connection, target_metadata=target_metadata, compare_type=True, ) with context.begin_transaction(): - # Serialize concurrent migrators (see _MIGRATION_LOCK_KEY). A - # transaction-scoped advisory lock: the first replica to get here - # holds it for the whole upgrade and is auto-released when this - # transaction ends. A sibling replica blocks on this line, and only - # once the leader commits does it proceed to read the version table - # — now at head — so it runs zero migrations instead of re-applying - # the same DDL. The lock is acquired BEFORE run_migrations() reads - # the current revision, which is what makes the no-op correct. - connection.execute( - text("SELECT pg_advisory_xact_lock(:k)"), - {"k": _MIGRATION_LOCK_KEY}, - ) context.run_migrations() From e90e6b2c3434af77f35706cae65f7578baf8d093 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 8 Jun 2026 00:15:40 -0400 Subject: [PATCH 2/2] perf(tags): protective-alias uses tag kind, drops the image_record full scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _create_protective_aliases scanned every image_record's tagger_predictions JSON (unindexed full scan, ~59k rows) to find the categories a merged-away tag's name was predicted under. That scan ran inside the merge transaction AFTER it had locked series_page — on a large library it held that lock for minutes and is what blocked migration 0040 (and starved the standardization task into its 40-min timeout). The scan was redundant: the tagger's tag_to_category map is one-to-one (a name has exactly one category) and a tag's kind is set from that category when created, so kind already IS the tagger's category for the name. The scan only ever rediscovered the kind. Build the single protective alias from src_kind directly — no scan, no lock-holding slow step in the merge. Rewrote test_alias_per_observed_prediction_category (which encoded the can't-actually-happen one-name-two-categories case) → test_protective_alias_uses_tag_kind. Co-Authored-By: Claude Opus 4.8 (1M context) --- backend/app/services/tag_service.py | 63 +++++++++++------------------ tests/test_tag_merge.py | 13 +++--- 2 files changed, 29 insertions(+), 47 deletions(-) diff --git a/backend/app/services/tag_service.py b/backend/app/services/tag_service.py index 4616812..11192a4 100644 --- a/backend/app/services/tag_service.py +++ b/backend/app/services/tag_service.py @@ -562,50 +562,33 @@ class TagService: async def _create_protective_aliases( self, src_name: str, src_kind: TagKind, tgt: int ) -> bool: - """One alias per category the tagger has actually emitted for - src_name (so future predictions resolve to target); fall back to - the tag's kind when the tagger never predicted this exact name. - Idempotent — never clobbers a pre-existing operator alias.""" + """Alias (src_name, category) -> tgt so future tagger predictions of + src_name resolve to the merge survivor. Idempotent — never clobbers a + pre-existing operator alias. + + The category is the tag's kind. The tagger's tag_to_category map is + one-to-one (a name has exactly ONE category), and a tag's kind is set + from that category when it's created — so kind already IS the tagger's + category for this name. This used to SELECT DISTINCT category by scanning + every image_record's tagger_predictions JSON (an unindexed full scan that, + on a large library, held a series_page lock for minutes inside the merge + and blocked migration 0040 — operator-flagged 2026-06-07). That scan only + ever rediscovered the kind, so it's gone.""" from ..models.tag_alias import TagAlias - rows = ( - await self.session.execute( - text( - "SELECT DISTINCT " - " (tagger_predictions::jsonb -> :n ->> 'category') " - " AS cat " - "FROM image_record " - "WHERE tagger_predictions IS NOT NULL " - " AND (tagger_predictions::jsonb) ? :n" - ), - {"n": src_name}, + category = src_kind.value if hasattr(src_kind, "value") else str(src_kind) + res = await self.session.execute( + pg_insert(TagAlias) + .values( + alias_string=src_name, + alias_category=category, + canonical_tag_id=tgt, ) - ).all() - categories = {r.cat for r in rows if r.cat} - if not categories: - kind_val = ( - src_kind.value - if hasattr(src_kind, "value") - else str(src_kind) + .on_conflict_do_nothing( + index_elements=["alias_string", "alias_category"] ) - categories = {kind_val} - - created = False - for cat in categories: - res = await self.session.execute( - pg_insert(TagAlias) - .values( - alias_string=src_name, - alias_category=cat, - canonical_tag_id=tgt, - ) - .on_conflict_do_nothing( - index_elements=["alias_string", "alias_category"] - ) - ) - if res.rowcount: - created = True - return created + ) + return bool(res.rowcount) # --------------------------------------------------------------------------- diff --git a/tests/test_tag_merge.py b/tests/test_tag_merge.py index ed772c3..69d6b8a 100644 --- a/tests/test_tag_merge.py +++ b/tests/test_tag_merge.py @@ -320,7 +320,12 @@ async def test_no_alias_when_purely_manual(db): @pytest.mark.asyncio -async def test_alias_per_observed_prediction_category(db): +async def test_protective_alias_uses_tag_kind(db): + # The protective alias category is the tag's KIND — the tagger maps each name + # to exactly one category and a tag's kind is set from it, so kind already IS + # the tagger's category. The merge no longer scans image_record's predictions + # to rediscover it. Even with a (contrived) differing prediction category + # present, the merge writes a single (name, kind) alias. from backend.app.models import ImageRecord from backend.app.models.tag_alias import TagAlias @@ -332,11 +337,6 @@ async def test_alias_per_observed_prediction_category(db): await svc.add_to_image(img, a.id, source="ml_auto") r1 = await db.get(ImageRecord, img) r1.tagger_predictions = { - "predname": {"category": "general", "confidence": 0.9} - } - i2 = await _img(db) - r2 = await db.get(ImageRecord, i2) - r2.tagger_predictions = { "predname": {"category": "copyright", "confidence": 0.8} } await db.flush() @@ -353,7 +353,6 @@ async def test_alias_per_observed_prediction_category(db): ).all() assert {(r.alias_string, r.alias_category) for r in rows} == { ("predname", "general"), - ("predname", "copyright"), } assert all(r.canonical_tag_id == b.id for r in rows)