From dffb87080fd252a437e8fe6188c638733874bb7a Mon Sep 17 00:00:00 2001 From: Chris Bartholomew Date: Fri, 27 Mar 2026 11:01:38 -0400 Subject: [PATCH] fix(migrations): bypass PgBouncer for advisory locks via MIGRATION_DATABASE_URL (#726) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(migrations): use HINDSIGHT_API_MIGRATION_DATABASE_URL when set Session-level advisory locks are broken when the database URL goes through PgBouncer in transaction mode: the backend connection is returned to the pool on COMMIT, orphaning the lock, so multiple pods can simultaneously run migrations for the same schema. When HINDSIGHT_API_MIGRATION_DATABASE_URL is set, use it for both the advisory lock connection and the Alembic run. Callers should point this at the direct PostgreSQL endpoint (bypassing the pooler) so the session-level lock is held for the full migration duration. * refactor(migrations): move MIGRATION_DATABASE_URL to standard config Wire HINDSIGHT_API_MIGRATION_DATABASE_URL through HindsightConfig instead of reading os.getenv() directly in migrations.py. Add the field to the dataclass, from_env(), log_config(), all call sites, .env.example, and the configuration docs page. * fix: update test mocks for migration_database_url kwarg and regenerate docs skill --------- Co-authored-by: Nicolò Boschi --- .env.example | 1 + hindsight-api-slim/hindsight_api/admin/cli.py | 2 +- hindsight-api-slim/hindsight_api/config.py | 5 +++++ .../hindsight_api/engine/memory_engine.py | 6 ++---- .../hindsight_api/extensions/context.py | 4 +++- hindsight-api-slim/hindsight_api/migrations.py | 13 +++++++++++-- .../tests/test_admin_backup_restore.py | 6 +++--- hindsight-docs/docs/developer/configuration.md | 1 + .../references/developer/configuration.md | 1 + 9 files changed, 28 insertions(+), 11 deletions(-) diff --git a/.env.example b/.env.example index 191367e0..7be4ee38 100644 --- a/.env.example +++ b/.env.example @@ -44,6 +44,7 @@ HINDSIGHT_API_LOG_LEVEL=info # Database (Optional - uses embedded pg0 by default) # HINDSIGHT_API_DATABASE_URL=postgresql://user:pass@host:5432/db +# HINDSIGHT_API_MIGRATION_DATABASE_URL= # Direct PostgreSQL URL for migrations (bypasses PgBouncer). Falls back to DATABASE_URL. # HINDSIGHT_API_DATABASE_SCHEMA=public # PostgreSQL schema name (default: public) # Vector Extension (Optional - uses pgvector by default) diff --git a/hindsight-api-slim/hindsight_api/admin/cli.py b/hindsight-api-slim/hindsight_api/admin/cli.py index 8b80dca1..7fc37a8c 100644 --- a/hindsight-api-slim/hindsight_api/admin/cli.py +++ b/hindsight-api-slim/hindsight_api/admin/cli.py @@ -249,7 +249,7 @@ async def _run_migration( schemas = list(dict.fromkeys(schemas)) for schema in schemas: - run_migrations(resolved_url, schema=schema) + run_migrations(resolved_url, schema=schema, migration_database_url=config.migration_database_url) if embedding_dimension is not None: for schema in schemas: diff --git a/hindsight-api-slim/hindsight_api/config.py b/hindsight-api-slim/hindsight_api/config.py index 9b371bc3..35b5c003 100644 --- a/hindsight-api-slim/hindsight_api/config.py +++ b/hindsight-api-slim/hindsight_api/config.py @@ -118,6 +118,7 @@ def normalize_config_dict(config: dict[str, Any]) -> dict[str, Any]: # Environment variable names ENV_DATABASE_URL = "HINDSIGHT_API_DATABASE_URL" +ENV_MIGRATION_DATABASE_URL = "HINDSIGHT_API_MIGRATION_DATABASE_URL" ENV_DATABASE_SCHEMA = "HINDSIGHT_API_DATABASE_SCHEMA" ENV_LLM_PROVIDER = "HINDSIGHT_API_LLM_PROVIDER" ENV_LLM_API_KEY = "HINDSIGHT_API_LLM_API_KEY" @@ -616,6 +617,7 @@ class HindsightConfig: # Database database_url: str + migration_database_url: str | None database_schema: str vector_extension: str # "pgvector" or "vchord" text_search_extension: str # "native" or "vchord" @@ -1009,6 +1011,7 @@ class HindsightConfig: config = cls( # Database database_url=os.getenv(ENV_DATABASE_URL, DEFAULT_DATABASE_URL), + migration_database_url=os.getenv(ENV_MIGRATION_DATABASE_URL) or None, database_schema=os.getenv(ENV_DATABASE_SCHEMA, DEFAULT_DATABASE_SCHEMA), vector_extension=os.getenv(ENV_VECTOR_EXTENSION, DEFAULT_VECTOR_EXTENSION).lower(), text_search_extension=os.getenv(ENV_TEXT_SEARCH_EXTENSION, DEFAULT_TEXT_SEARCH_EXTENSION).lower(), @@ -1415,6 +1418,8 @@ class HindsightConfig: def log_config(self) -> None: """Log the current configuration (without sensitive values).""" logger.info(f"Database: {self.database_url} (schema: {self.database_schema})") + if self.migration_database_url: + logger.info(f"Migration database: {self.migration_database_url}") logger.info(f"LLM: provider={self.llm_provider}, model={self.llm_model}") if self.retain_llm_provider or self.retain_llm_model: retain_provider = self.retain_llm_provider or self.llm_provider diff --git a/hindsight-api-slim/hindsight_api/engine/memory_engine.py b/hindsight-api-slim/hindsight_api/engine/memory_engine.py index c1e0aa40..2e4fd013 100644 --- a/hindsight-api-slim/hindsight_api/engine/memory_engine.py +++ b/hindsight-api-slim/hindsight_api/engine/memory_engine.py @@ -1681,18 +1681,16 @@ class MemoryEngine(MemoryEngineInterface): # Migrate all schemas from the tenant extension # The tenant extension is the single source of truth for which schemas exist logger.info("Running database migrations...") + config = get_config() tenants = await self._tenant_extension.list_tenants() if tenants: logger.info(f"Running migrations on {len(tenants)} schema(s)...") for tenant in tenants: schema = tenant.schema if schema: - run_migrations(self.db_url, schema=schema) + run_migrations(self.db_url, schema=schema, migration_database_url=config.migration_database_url) logger.info("Schema migrations completed") - # Get config for vector extension setting - config = get_config() - # Ensure embedding column dimension matches the model's dimension # This is done after migrations and after embeddings.initialize() for tenant in tenants: diff --git a/hindsight-api-slim/hindsight_api/extensions/context.py b/hindsight-api-slim/hindsight_api/extensions/context.py index 88c245c2..1cfbd27d 100644 --- a/hindsight-api-slim/hindsight_api/extensions/context.py +++ b/hindsight-api-slim/hindsight_api/extensions/context.py @@ -120,7 +120,9 @@ class DefaultExtensionContext(ExtensionContext): # CREATE INDEX CONCURRENTLY inside the migration waits for those transactions # forever — a deadlock. config = get_config() - await asyncio.to_thread(run_migrations, db_url, schema=schema) + await asyncio.to_thread( + run_migrations, db_url, schema=schema, migration_database_url=config.migration_database_url + ) # Ensure embedding column dimension matches the model's dimension # This is needed because migrations create columns with default dimension diff --git a/hindsight-api-slim/hindsight_api/migrations.py b/hindsight-api-slim/hindsight_api/migrations.py index 20b9c0bd..b33bc56e 100644 --- a/hindsight-api-slim/hindsight_api/migrations.py +++ b/hindsight-api-slim/hindsight_api/migrations.py @@ -176,6 +176,7 @@ def run_migrations( database_url: str, script_location: str | None = None, schema: str | None = None, + migration_database_url: str | None = None, ) -> None: """ Run database migrations to the latest version using programmatic Alembic configuration. @@ -213,6 +214,14 @@ def run_migrations( script_location="/path/to/copied/_alembic" ) """ + # Prefer a dedicated migration URL that bypasses connection poolers (e.g. + # PgBouncer in transaction mode). Session-level advisory locks don't + # survive a PgBouncer transaction-mode cycle, so the distributed lock is + # ineffective when the app URL goes through a pooler. Configure + # HINDSIGHT_API_MIGRATION_DATABASE_URL to the direct PostgreSQL endpoint + # (e.g. hindsight-pg-rw) to restore correct locking behaviour. + migration_url = migration_database_url or database_url + try: # Determine script location if script_location is None: @@ -249,7 +258,7 @@ def run_migrations( # 2. After acquiring the lock, COMMIT the transaction on the advisory-lock # connection itself before running migrations. pg_advisory_lock is # session-level, so the lock survives the COMMIT. - engine = create_engine(database_url) + engine = create_engine(migration_url) with engine.connect() as conn: logger.debug(f"Acquiring migration advisory lock for schema '{schema_name}' (id={lock_id})...") while True: @@ -394,7 +403,7 @@ def run_migrations( conn.commit() # Run migrations while holding the lock - _run_migrations_internal(database_url, script_location, schema=schema) + _run_migrations_internal(migration_url, script_location, schema=schema) finally: # Explicitly release the lock (also released on connection close) conn.execute(text(f"SELECT pg_advisory_unlock({lock_id})")) diff --git a/hindsight-api-slim/tests/test_admin_backup_restore.py b/hindsight-api-slim/tests/test_admin_backup_restore.py index 2e9cd107..2ffce365 100644 --- a/hindsight-api-slim/tests/test_admin_backup_restore.py +++ b/hindsight-api-slim/tests/test_admin_backup_restore.py @@ -314,7 +314,7 @@ async def test_run_migration_without_schema_discovers_and_deduplicates_schemas(m async def fake_resolve_database_url(db_url: str) -> str: return f"resolved::{db_url}" - def fake_run_migrations(database_url: str, schema: str | None = None) -> None: + def fake_run_migrations(database_url: str, schema: str | None = None, **kwargs) -> None: calls["run_migrations"].append((database_url, schema)) def fake_ensure_vector_extension( @@ -376,7 +376,7 @@ async def test_run_migration_without_schema_runs_optional_post_migration_hooks(m async def fake_resolve_database_url(db_url: str) -> str: return f"resolved::{db_url}" - def fake_run_migrations(database_url: str, schema: str | None = None) -> None: + def fake_run_migrations(database_url: str, schema: str | None = None, **kwargs) -> None: calls["run_migrations"].append((database_url, schema)) def fake_ensure_embedding_dimension( @@ -453,7 +453,7 @@ async def test_run_migration_with_schema_only_runs_requested_schema(monkeypatch) async def fake_resolve_database_url(db_url: str) -> str: return f"resolved::{db_url}" - def fake_run_migrations(database_url: str, schema: str | None = None) -> None: + def fake_run_migrations(database_url: str, schema: str | None = None, **kwargs) -> None: calls["run_migrations"].append((database_url, schema)) def fake_ensure_vector_extension( diff --git a/hindsight-docs/docs/developer/configuration.md b/hindsight-docs/docs/developer/configuration.md index ceba9c18..986beaff 100644 --- a/hindsight-docs/docs/developer/configuration.md +++ b/hindsight-docs/docs/developer/configuration.md @@ -20,6 +20,7 @@ The API service handles all memory operations (retain, recall, reflect). | Variable | Description | Default | |----------|-------------|---------| | `HINDSIGHT_API_DATABASE_URL` | PostgreSQL connection string | `pg0` (embedded) | +| `HINDSIGHT_API_MIGRATION_DATABASE_URL` | Direct PostgreSQL URL for running migrations, bypassing connection poolers (e.g. PgBouncer). When set, advisory locks and Alembic migrations use this URL instead of `DATABASE_URL`. | Falls back to `DATABASE_URL` | | `HINDSIGHT_API_DATABASE_SCHEMA` | PostgreSQL schema name for tables | `public` | | `HINDSIGHT_API_RUN_MIGRATIONS_ON_STARTUP` | Run database migrations on API startup | `true` | diff --git a/skills/hindsight-docs/references/developer/configuration.md b/skills/hindsight-docs/references/developer/configuration.md index de7f7bdc..c0702ebb 100644 --- a/skills/hindsight-docs/references/developer/configuration.md +++ b/skills/hindsight-docs/references/developer/configuration.md @@ -20,6 +20,7 @@ The API service handles all memory operations (retain, recall, reflect). | Variable | Description | Default | |----------|-------------|---------| | `HINDSIGHT_API_DATABASE_URL` | PostgreSQL connection string | `pg0` (embedded) | +| `HINDSIGHT_API_MIGRATION_DATABASE_URL` | Direct PostgreSQL URL for running migrations, bypassing connection poolers (e.g. PgBouncer). When set, advisory locks and Alembic migrations use this URL instead of `DATABASE_URL`. | Falls back to `DATABASE_URL` | | `HINDSIGHT_API_DATABASE_SCHEMA` | PostgreSQL schema name for tables | `public` | | `HINDSIGHT_API_RUN_MIGRATIONS_ON_STARTUP` | Run database migrations on API startup | `true` |