From 9218ad8e4b3f0554bcc97e496687632bf5ff6f2d Mon Sep 17 00:00:00 2001 From: AndresL230 <190146319+AndresL230@users.noreply.github.com> Date: Sat, 1 Aug 2026 00:06:51 -0700 Subject: [PATCH] fix(db): raise maintenance_work_mem in the migration runner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applying the reconciled backlog to staging died seven files in: ProgramLimitExceeded: memory required is 35 MB, maintenance_work_mem is 32 MB 0039_rag_vector_store builds an ivfflat index over VECTOR(768). Supabase defaults maintenance_work_mem to 32 MB, so the build cannot complete — and because apply_migration wraps each file plus its ledger INSERT in one transaction and run() has no per-file recovery, it took the remaining four migrations down with it. The ledger stopped at 44 of 49 with no partial state, which is the one good thing about that failure mode. This is not staging-specific. Any environment on the default hits it the first time 0039 runs, prod included, and prod has not been migrated yet. Set per session, not per environment. `ALTER DATABASE ... SET` only reaches backends started after it, and a pooled connection is frequently already established — observed directly against Supavisor, where a fresh backend picked up the new value while a reused one still reported 32 MB, making the change look like it had silently failed. A session-level SET always lands on the connection actually running the DDL. 128 MB is transient per-operation memory during an index build, not a reservation, and leaves headroom over 0039's ~35 MB without being reckless on a small instance. MIGRATE_MAINTENANCE_WORK_MEM overrides it. The SET is a literal rather than a bound parameter because SET does not accept one; the value is operator config, never request input, and a bad value fails loudly at the start instead of mid-migration. Verified by using it: staging's remaining 5 migrations applied cleanly, and the drift report now reports 49 on disk / 49 recorded / 0 pending / 0 orphans, exit 0. That also closed #316 (avatars bucket now public=true) and #265 (assignments_source_check now admits 'gradescope'). 1557 passed, 38 skipped; ruff clean. Co-Authored-By: Claude Opus 5 --- backend/db/migrate.py | 26 ++++++++ backend/tests/test_migrate_work_mem.py | 89 ++++++++++++++++++++++++++ 2 files changed, 115 insertions(+) create mode 100644 backend/tests/test_migrate_work_mem.py diff --git a/backend/db/migrate.py b/backend/db/migrate.py index 5a9a1ad2..a7a1d0b3 100644 --- a/backend/db/migrate.py +++ b/backend/db/migrate.py @@ -37,6 +37,26 @@ MIGRATIONS_DIR = Path(__file__).parent / "migrations" +# Index builds size their working set from maintenance_work_mem, and the server +# default is not enough for every migration in this repo. 0039_rag_vector_store +# builds an ivfflat index over VECTOR(768) and needs ~35 MB; Supabase defaults to +# 32 MB, so it dies with +# ProgramLimitExceeded: memory required is 35 MB, maintenance_work_mem is 32 MB +# and — because apply_migration runs each file plus its ledger INSERT in one +# transaction, and run() has no per-file recovery — takes every migration queued +# behind it down too. Staging hit exactly this with 5 files still pending. +# +# Set it PER SESSION rather than per environment. `ALTER DATABASE ... SET` only +# reaches backends started after it, and a pooled connection is often already +# established, so the change appears to do nothing (observed against Supavisor: +# a fresh backend saw the new value while a reused one still reported 32 MB). A +# session-level SET always lands on the connection actually running the DDL. +# +# Transient — this is per-operation memory during an index build, not a +# reservation. Override for a memory-tight instance: +# MIGRATE_MAINTENANCE_WORK_MEM=64MB python -m db.migrate +MAINTENANCE_WORK_MEM = os.environ.get("MIGRATE_MAINTENANCE_WORK_MEM", "128MB") + # Two accepted filename shapes, both sortable and both fixed-width: # NNNN_ legacy sequential prefix (frozen — see below) @@ -115,6 +135,12 @@ def run( baseline: bool = False, ) -> list[str]: """Apply (or baseline-record) all pending migrations. Returns filenames handled.""" + with conn.cursor() as cur: + # Quoted as a literal, not a bound parameter: SET does not accept one. + # The value is operator-supplied config, never request input, and a bad + # value fails loudly here rather than mid-migration. + cur.execute(f"SET maintenance_work_mem = '{MAINTENANCE_WORK_MEM}'") + conn.commit() ensure_tracking_table(conn) applied = applied_filenames(conn) pending = pending_migrations(discover_migrations(migrations_dir), applied) diff --git a/backend/tests/test_migrate_work_mem.py b/backend/tests/test_migrate_work_mem.py new file mode 100644 index 00000000..dee62a06 --- /dev/null +++ b/backend/tests/test_migrate_work_mem.py @@ -0,0 +1,89 @@ +"""run() must raise maintenance_work_mem before applying anything (#510 fallout). + +0039_rag_vector_store builds an ivfflat index over VECTOR(768) and needs ~35 MB. +Supabase defaults maintenance_work_mem to 32 MB, so the migration dies with +"memory required is 35 MB, maintenance_work_mem is 32 MB" — and because +apply_migration wraps each file plus its ledger INSERT in one transaction with +no per-file recovery, everything queued behind it dies too. Staging hit this +with 5 migrations still pending. + +The SET has to happen on the connection that runs the DDL: `ALTER DATABASE` +only reaches backends started after it, and a pooled connection is often +already established. +""" +from pathlib import Path + +import pytest + +from db import migrate + + +class _FakeCursor: + def __init__(self, log: list[str]): + self.log = log + + def execute(self, sql, params=None): + self.log.append(sql if isinstance(sql, str) else str(sql)) + + def fetchall(self): + return [] + + def __enter__(self): + return self + + def __exit__(self, *exc): + return False + + +class _FakeConn: + """Records executed SQL; no pending migrations so run() short-circuits.""" + + def __init__(self): + self.log: list[str] = [] + self.commits = 0 + + def cursor(self): + return _FakeCursor(self.log) + + def commit(self): + self.commits += 1 + + +@pytest.fixture +def empty_dir(tmp_path) -> Path: + return tmp_path + + +def test_run_sets_maintenance_work_mem_before_touching_the_ledger(empty_dir): + """Order matters: an index build inside the very first migration must + already see the raised value.""" + conn = _FakeConn() + + migrate.run(conn, migrations_dir=empty_dir) + + set_stmts = [s for s in conn.log if s.startswith("SET maintenance_work_mem")] + assert set_stmts, f"run() never raised maintenance_work_mem; executed: {conn.log}" + + first_set = conn.log.index(set_stmts[0]) + first_ddl = next( + (i for i, s in enumerate(conn.log) if "schema_migrations" in s), len(conn.log) + ) + assert first_set < first_ddl, "the SET must precede any migration work" + + +def test_the_value_is_configurable_for_a_memory_tight_instance(empty_dir, monkeypatch): + """A small instance must be able to dial this down without editing code.""" + monkeypatch.setattr(migrate, "MAINTENANCE_WORK_MEM", "64MB") + conn = _FakeConn() + + migrate.run(conn, migrations_dir=empty_dir) + + assert any("'64MB'" in s for s in conn.log), conn.log + + +def test_the_default_clears_the_known_requirement(empty_dir): + """0039 needs ~35 MB. The default must exceed that with headroom, or this + fix does not actually fix the migration that motivated it.""" + value = migrate.MAINTENANCE_WORK_MEM + assert value.upper().endswith("MB"), f"expected an MB value, got {value!r}" + assert int(value[:-2]) >= 64, f"{value} leaves no headroom over 0039's ~35 MB"