diff --git a/dashboard/backend/database_postgres.py b/dashboard/backend/database_postgres.py index 34116599..45a99a6b 100644 --- a/dashboard/backend/database_postgres.py +++ b/dashboard/backend/database_postgres.py @@ -226,27 +226,6 @@ def _init_schema(self) -> None: """ ) - cur.execute( - "CREATE INDEX IF NOT EXISTS idx_agent_runs_session " - "ON agent_runs(session_id)" - ) - cur.execute( - "CREATE INDEX IF NOT EXISTS idx_agent_runs_session_mode " - "ON agent_runs(session_id, mode)" - ) - cur.execute( - "CREATE INDEX IF NOT EXISTS idx_run_timestamp " - "ON equity_timeseries(run_id, timestamp)" - ) - cur.execute( - "CREATE INDEX IF NOT EXISTS idx_trades_run " - "ON trades(run_id, timestamp)" - ) - cur.execute( - "CREATE INDEX IF NOT EXISTS idx_decisions_run " - "ON backtest_decisions(run_id, step_index)" - ) - # ADDING A COLUMN LATER? It must go in an `ALTER TABLE ... ADD COLUMN IF # NOT EXISTS` below, *not* only in the CREATE above. CREATE TABLE IF NOT # EXISTS silently no-ops once the table exists, so an existing deployment @@ -426,6 +405,38 @@ def _init_schema(self) -> None: "market_rule_closing_gate_effective BOOLEAN" ) + # Indexes sit below every ADD COLUMN on purpose. agent_runs.session_id + # arrives by ALTER on a table that predates it, and an index created + # above that ALTER raises UndefinedColumn there before the column + # exists -- the #432 Render boot crash, in the credits twin. + # test_store_twin_parity.py pins this order for every twin -- + # by *source* position, which is a proxy for execution order and + # holds here because this method runs its DDL top to bottom. It + # is not a universal property (users_postgres.py executes a + # hoisted constant after later inline ALTERs), so keep these + # CREATE INDEX calls physically below the ALTERs rather than + # relying on the guard to notice if they move. + cur.execute( + "CREATE INDEX IF NOT EXISTS idx_agent_runs_session " + "ON agent_runs(session_id)" + ) + cur.execute( + "CREATE INDEX IF NOT EXISTS idx_agent_runs_session_mode " + "ON agent_runs(session_id, mode)" + ) + cur.execute( + "CREATE INDEX IF NOT EXISTS idx_run_timestamp " + "ON equity_timeseries(run_id, timestamp)" + ) + cur.execute( + "CREATE INDEX IF NOT EXISTS idx_trades_run " + "ON trades(run_id, timestamp)" + ) + cur.execute( + "CREATE INDEX IF NOT EXISTS idx_decisions_run " + "ON backtest_decisions(run_id, step_index)" + ) + # Postgres counterpart of SQLite's # _ensure_equity_timeseries_uniqueness/_apply_equity_timeseries_uniqueness: # the natural key that makes a rerun replace rather than duplicate, and diff --git a/dashboard/backend/domain/credits/repository_postgres.py b/dashboard/backend/domain/credits/repository_postgres.py index 71cc274f..6cc1ebc6 100644 --- a/dashboard/backend/domain/credits/repository_postgres.py +++ b/dashboard/backend/domain/credits/repository_postgres.py @@ -331,6 +331,18 @@ def _evidence_identity_json(evidence_json: str) -> str: """ +# ADDING A COLUMN LATER? It must go in an `ALTER TABLE ... ADD COLUMN IF NOT +# EXISTS` here, *not* only in CREDITS_POSTGRES_DDL above: CREATE TABLE IF NOT +# EXISTS silently no-ops once the table exists, so a deployed table would never +# gain the column and every query naming it would raise UndefinedColumn. Any +# index over that column must be created here too, *below* its ADD COLUMN -- +# never in the base DDL, which runs first. #432 killed the Render boot exactly +# that way, and credits_store is built at import, so the crash took the whole +# app down rather than one surface. Nothing catches either mistake first: +# SQLite is the default test tier and CI's Postgres container is empty on +# every run, so only the CREATE path is ever exercised there. +# test_store_twin_parity.py pins both rules statically; see +# domain/agents/repository_postgres.py for the worked example. CREDITS_POSTGRES_GRANT_MIGRATION_DDL = """ ALTER TABLE credit_ledger_entries ADD COLUMN IF NOT EXISTS bucket TEXT; @@ -366,8 +378,6 @@ def _evidence_identity_json(evidence_json: str) -> str: ALTER TABLE credit_llm_reservations ADD CONSTRAINT credit_llm_reservations_attempt_index_check CHECK (attempt_index >= 0); -CREATE INDEX IF NOT EXISTS idx_credit_llm_reservations_run_status -ON credit_llm_reservations(run_id, status, call_index, attempt_index); ALTER TABLE credit_accounts ADD COLUMN IF NOT EXISTS restriction_reason TEXT; UPDATE credit_llm_reservations @@ -422,11 +432,75 @@ def _evidence_identity_json(evidence_json: str) -> str: END LOOP; END $$; -ALTER TABLE credit_llm_reservations -DROP CONSTRAINT IF EXISTS credit_llm_reservations_logical_attempt_key; -ALTER TABLE credit_llm_reservations -ADD CONSTRAINT credit_llm_reservations_logical_attempt_key -UNIQUE (user_id, run_id, call_index, attempt_index); +DO $$ +BEGIN + -- Converging, for the same reason as the index repair below: this DDL + -- runs on every boot and ADD CONSTRAINT ... UNIQUE builds a full index + -- under ACCESS EXCLUSIVE, so skip both statements once the constraint is + -- already the four-column one. Column identity comes from conkey rather + -- than a pg_get_constraintdef text match -- the same idiom as the legacy + -- sweep above, and it cannot drift with Postgres's deparsing. Any + -- mismatch simply falls back to drop+add, i.e. the previous behaviour. + IF NOT EXISTS ( + SELECT 1 + FROM pg_constraint AS con + WHERE con.conrelid = 'credit_llm_reservations'::regclass + AND con.conname = 'credit_llm_reservations_logical_attempt_key' + AND con.contype = 'u' + AND ( + SELECT array_agg(att.attname ORDER BY key.ord) + FROM unnest(con.conkey) WITH ORDINALITY AS key(attnum, ord) + JOIN pg_attribute AS att + ON att.attrelid = con.conrelid + AND att.attnum = key.attnum + ) = ARRAY['user_id', 'run_id', 'call_index', 'attempt_index']::name[] + ) THEN + ALTER TABLE credit_llm_reservations + DROP CONSTRAINT IF EXISTS credit_llm_reservations_logical_attempt_key; + ALTER TABLE credit_llm_reservations + ADD CONSTRAINT credit_llm_reservations_logical_attempt_key + UNIQUE (user_id, run_id, call_index, attempt_index); + END IF; +END +$$; +DO $$ +BEGIN + -- Prod carried this index name over (run_id, status, call_index) before + -- #432 added attempt_index. CREATE INDEX IF NOT EXISTS matches by name + -- alone and would keep that stale definition forever, so drop it -- but + -- only while it is stale: this DDL runs on every boot (credits_store is + -- built at import), and an unconditional DROP+CREATE would rebuild the + -- index under ACCESS EXCLUSIVE on every deploy instead of converging. + -- + -- Scope, so the next reader does not over-read this: converging is a + -- property of this repair and of the UNIQUE above, NOT of the migration + -- as a whole. Every CHECK and the FOREIGN KEY on credit_llm_reservations + -- and credit_ledger_entries is still dropped and re-added unconditionally, + -- each costing a validating full-table scan per boot. That is deliberate: + -- recognising an existing CHECK means comparing pg_get_constraintdef text, + -- which drifts with Postgres's deparsing, and a mismatch there converges + -- to nothing while adding a way to skip a constraint that ought to be + -- rewritten. Both tables sit behind a disabled billing flag today; revisit + -- if either grows. + IF EXISTS ( + SELECT 1 + FROM pg_index AS idx + JOIN pg_class AS rel ON rel.oid = idx.indexrelid + WHERE idx.indrelid = 'credit_llm_reservations'::regclass + AND rel.relname = 'idx_credit_llm_reservations_run_status' + AND split_part(pg_get_indexdef(idx.indexrelid), ' USING btree ', 2) + <> '(run_id, status, call_index, attempt_index)' + ) THEN + -- IF EXISTS, not bare: the predicate above is evaluated before the + -- lock is taken, so two concurrent boots can both reach this line and + -- the loser would raise "index does not exist", aborting _init_schema + -- -- fatal, because credits_store is built at import. + DROP INDEX IF EXISTS idx_credit_llm_reservations_run_status; + END IF; +END +$$; +CREATE INDEX IF NOT EXISTS idx_credit_llm_reservations_run_status +ON credit_llm_reservations(run_id, status, call_index, attempt_index); ALTER TABLE credit_grant_pool_ledger_entries ADD COLUMN IF NOT EXISTS pool_name_snapshot TEXT; diff --git a/dashboard/backend/tests/domain/credits/test_repository_postgres.py b/dashboard/backend/tests/domain/credits/test_repository_postgres.py index d4045e31..48b36b85 100644 --- a/dashboard/backend/tests/domain/credits/test_repository_postgres.py +++ b/dashboard/backend/tests/domain/credits/test_repository_postgres.py @@ -3,6 +3,7 @@ from __future__ import annotations import os +import re import uuid from concurrent.futures import ThreadPoolExecutor from contextlib import contextmanager @@ -88,6 +89,81 @@ def test_postgres_schema_tracks_provider_attempt_identity(): ) +def test_postgres_boot_ddl_repairs_the_stale_run_status_index_conditionally(): + """CREATE INDEX IF NOT EXISTS matches by *name*, so prod's pre-#432 + three-column index satisfies it forever. The migration must drop that + definition -- but only that one: an unconditional DROP+CREATE runs under + ACCESS EXCLUSIVE on every boot and never converges to a no-op. + + What makes the drop conditional is its *position* -- inside the DO block's + staleness guard -- not the presence or absence of any keyword on the DROP + itself. Do not re-add an assertion banning ``IF EXISTS`` here: that spelling + is a concurrency requirement, because two boots can both pass the predicate + and the loser must no-op rather than abort _init_schema. + """ + migration = pg_module.CREDITS_POSTGRES_GRANT_MIGRATION_DDL + add_column = "ADD COLUMN IF NOT EXISTS attempt_index INTEGER NOT NULL DEFAULT 0" + stale_check = "<> '(run_id, status, call_index, attempt_index)'" + create_index = ( + "CREATE INDEX IF NOT EXISTS idx_credit_llm_reservations_run_status\n" + "ON credit_llm_reservations(run_id, status, call_index, attempt_index);" + ) + + for statement in (add_column, stale_check, create_index): + assert statement in migration, statement + + drops = [ + match.start() + for match in re.finditer( + r"DROP\s+INDEX\s+(?:IF\s+EXISTS\s+)?" + r"idx_credit_llm_reservations_run_status", + migration, + ) + ] + assert len(drops) == 1, f"expected exactly one drop of the index, got {drops}" + + guard_at = migration.index(stale_check) + end_if_at = migration.index("END IF;", guard_at) + assert guard_at < drops[0] < end_if_at, ( + "the DROP INDEX must sit inside the staleness guard; an unconditional " + "one rebuilds the index under ACCESS EXCLUSIVE on every boot" + ) + assert migration.index(add_column) < guard_at + assert end_if_at < migration.index(create_index) + + +def test_postgres_boot_ddl_rebuilds_the_logical_attempt_key_conditionally(): + """The UNIQUE that backs (user_id, run_id, call_index, attempt_index) is an + index build under ACCESS EXCLUSIVE, so it gets the same treatment as the + stale-index repair: skipped once conkey already names those four columns. + + Column identity must come from ``conkey``, not a pg_get_constraintdef text + match -- the deparsed text drifts across Postgres versions, and a guard that + silently stops matching converges to nothing while still looking correct. + """ + migration = pg_module.CREDITS_POSTGRES_GRANT_MIGRATION_DDL + conkey_check = ( + "ARRAY['user_id', 'run_id', 'call_index', 'attempt_index']::name[]" + ) + assert conkey_check in migration + + adds = [ + match.start() + for match in re.finditer( + r"ADD\s+CONSTRAINT\s+credit_llm_reservations_logical_attempt_key", + migration, + ) + ] + assert len(adds) == 1, f"expected exactly one ADD CONSTRAINT, got {adds}" + + guard_at = migration.index(conkey_check) + end_if_at = migration.index("END IF;", guard_at) + assert guard_at < adds[0] < end_if_at, ( + "ADD CONSTRAINT ... UNIQUE must sit inside the conkey guard; " + "unconditionally it rebuilds a full index on every boot" + ) + + def test_postgres_provider_attempt_index_is_created_after_its_column(): base_ddl = pg_module.CREDITS_POSTGRES_DDL migration_ddl = pg_module.CREDITS_POSTGRES_GRANT_MIGRATION_DDL @@ -170,6 +246,54 @@ def _schema_url(database_url: str, schema: str) -> str: return urlunsplit((parts.scheme, parts.netloc, parts.path, urlencode(query), "")) +@contextmanager +def _isolated_schema(prefix: str): + """A throwaway schema on TEST_POSTGRES_URL, dropped (CASCADE) on exit. + + Yields a URL whose search_path is pinned to the schema, so every store and + test connection built from it sees only this test's tables. + """ + base_url = require_local_postgres_url(TEST_POSTGRES_URL) + schema = f"{prefix}_{uuid.uuid4().hex}" + with psycopg.connect(base_url) as conn: + conn.execute(sql.SQL("CREATE SCHEMA {}").format(sql.Identifier(schema))) + try: + yield _schema_url(base_url, schema) + finally: + db_pool._reset_for_tests() + with psycopg.connect(base_url) as conn: + conn.execute( + sql.SQL("DROP SCHEMA IF EXISTS {} CASCADE").format( + sql.Identifier(schema) + ) + ) + + +def _create_users(conn, rows, *, created_at: str) -> None: + """The users table every credits fixture needs; rows are (id, email, name, role).""" + conn.execute( + """ + CREATE TABLE users ( + id INTEGER PRIMARY KEY, + email TEXT NOT NULL UNIQUE, + display_name TEXT NOT NULL, + password_hash TEXT NOT NULL, + role TEXT NOT NULL, + created_at TEXT NOT NULL + ) + """ + ) + with conn.cursor() as cur: + cur.executemany( + """ + INSERT INTO users ( + id, email, display_name, password_hash, role, created_at + ) VALUES (%s, %s, %s, 'unused', %s, %s) + """, + [(*row, created_at) for row in rows], + ) + + LEGACY_CREDITS_POSTGRES_DDL = """ CREATE TABLE credit_accounts ( user_id INTEGER PRIMARY KEY REFERENCES users(id) ON DELETE CASCADE, @@ -261,88 +385,68 @@ def _schema_url(database_url: str, schema: str) -> str: """ +# Prod reservation table as of PR #431 — exists, but has neither attempt_index +# nor the four-column unique key. CREATE TABLE IF NOT EXISTS no-ops against it. +PRE_FAILOVER_RESERVATION_DDL = """ +CREATE TABLE credit_llm_reservations ( + reservation_id TEXT PRIMARY KEY CHECK (length(trim(reservation_id)) > 0), + user_id INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE, + run_id TEXT NOT NULL CHECK (length(trim(run_id)) > 0), + call_index INTEGER NOT NULL CHECK (call_index >= 0), + reserved_micro BIGINT NOT NULL CHECK (reserved_micro > 0), + reserved_grant_micro BIGINT NOT NULL CHECK (reserved_grant_micro >= 0), + reserved_purchased_micro BIGINT NOT NULL CHECK (reserved_purchased_micro >= 0), + settled_micro BIGINT NOT NULL DEFAULT 0 CHECK (settled_micro >= 0), + actual_micro BIGINT NOT NULL DEFAULT 0 CHECK (actual_micro >= 0), + outstanding_micro BIGINT NOT NULL DEFAULT 0 CHECK (outstanding_micro >= 0), + outstanding_recovered_micro BIGINT NOT NULL DEFAULT 0 + CHECK (outstanding_recovered_micro >= 0), + status TEXT NOT NULL DEFAULT 'open' + CHECK (status IN ('open', 'settled', 'released')), + operation_key TEXT NOT NULL UNIQUE CHECK (length(trim(operation_key)) > 0), + request_digest TEXT NOT NULL CHECK (length(trim(request_digest)) > 0), + evidence_json TEXT, + failure_reason TEXT, + created_at TEXT NOT NULL, + updated_at TEXT NOT NULL, + CHECK (reserved_micro = reserved_grant_micro + reserved_purchased_micro), + UNIQUE (user_id, run_id, call_index) +); + +CREATE INDEX idx_credit_llm_reservations_run_status +ON credit_llm_reservations(run_id, status, call_index); +""" + + @pytest.fixture def pg_credits_store(): - base_url = require_local_postgres_url(TEST_POSTGRES_URL) - schema = f"credits_{uuid.uuid4().hex}" - with psycopg.connect(base_url) as conn: - conn.execute(sql.SQL("CREATE SCHEMA {}").format(sql.Identifier(schema))) - - scoped_url = _schema_url(base_url, schema) - try: + with _isolated_schema("credits") as scoped_url: with psycopg.connect(scoped_url) as conn: - conn.execute( - """ - CREATE TABLE users ( - id INTEGER PRIMARY KEY, - email TEXT NOT NULL UNIQUE, - display_name TEXT NOT NULL, - password_hash TEXT NOT NULL, - role TEXT NOT NULL, - created_at TEXT NOT NULL - ) - """ + _create_users( + conn, + [ + (1, "buyer@example.com", "Buyer", "user"), + (2, "admin@example.com", "Admin", "admin"), + (3, "other@example.com", "Other", "user"), + ], + created_at="2026-08-13T00:00:00+00:00", ) - with conn.cursor() as cur: - cur.executemany( - """ - INSERT INTO users ( - id, email, display_name, password_hash, role, created_at - ) - VALUES (%s, %s, %s, 'unused', %s, '2026-08-13T00:00:00+00:00') - """, - [ - (1, "buyer@example.com", "Buyer", "user"), - (2, "admin@example.com", "Admin", "admin"), - (3, "other@example.com", "Other", "user"), - ], - ) yield pg_module.PostgresCreditsStore(scoped_url) - finally: - db_pool._reset_for_tests() - with psycopg.connect(base_url) as conn: - conn.execute( - sql.SQL("DROP SCHEMA IF EXISTS {} CASCADE").format( - sql.Identifier(schema) - ) - ) @pytest.fixture def pg_legacy_credits_url(): - base_url = require_local_postgres_url(TEST_POSTGRES_URL) - schema = f"credits_legacy_{uuid.uuid4().hex}" - with psycopg.connect(base_url) as conn: - conn.execute(sql.SQL("CREATE SCHEMA {}").format(sql.Identifier(schema))) - - scoped_url = _schema_url(base_url, schema) - try: + with _isolated_schema("credits_legacy") as scoped_url: with psycopg.connect(scoped_url) as conn: - conn.execute( - """ - CREATE TABLE users ( - id INTEGER PRIMARY KEY, - email TEXT NOT NULL UNIQUE, - display_name TEXT NOT NULL, - password_hash TEXT NOT NULL, - role TEXT NOT NULL, - created_at TEXT NOT NULL - ) - """ + _create_users( + conn, + [ + (1, "legacy@example.com", "Legacy", "user"), + (2, "admin@example.com", "Admin", "admin"), + ], + created_at="2026-08-01T00:00:00+00:00", ) - with conn.cursor() as cur: - cur.executemany( - """ - INSERT INTO users ( - id, email, display_name, password_hash, role, created_at - ) VALUES (%s, %s, %s, 'unused', %s, '2026-08-01T00:00:00+00:00') - """, - [ - (1, "legacy@example.com", "Legacy", "user"), - (2, "admin@example.com", "Admin", "admin"), - ], - ) conn.execute(LEGACY_CREDITS_POSTGRES_DDL) conn.execute( """ @@ -437,15 +541,36 @@ def pg_legacy_credits_url(): ) yield scoped_url - finally: - db_pool._reset_for_tests() - with psycopg.connect(base_url) as conn: + + +@pytest.fixture +def pg_pre_failover_reservations_url(): + with _isolated_schema("credits_pre_failover") as scoped_url: + with psycopg.connect(scoped_url) as conn: + _create_users( + conn, + [(1, "legacy@example.com", "Legacy", "user")], + created_at="2026-08-31T00:00:00+00:00", + ) + conn.execute(PRE_FAILOVER_RESERVATION_DDL) conn.execute( - sql.SQL("DROP SCHEMA IF EXISTS {} CASCADE").format( - sql.Identifier(schema) + """ + INSERT INTO credit_llm_reservations ( + reservation_id, user_id, run_id, call_index, reserved_micro, + reserved_grant_micro, reserved_purchased_micro, settled_micro, + actual_micro, outstanding_micro, outstanding_recovered_micro, + status, operation_key, request_digest, created_at, updated_at + ) VALUES ( + 'pre-failover-reservation', 1, 'pre-failover-run', 0, + 1000000, 1000000, 0, 0, 0, 0, 0, 'open', + 'pre-failover-operation', repeat('p', 64), + '2026-08-31T00:00:00+00:00', '2026-08-31T00:00:00+00:00' ) + """ ) + yield scoped_url + def _pending_order( store, @@ -1074,6 +1199,105 @@ def test_postgres_migration_preserves_legacy_stripe_ledger(pg_legacy_credits_url assert reopened_pool_entries["count"] == 0 +def _run_status_index(conn) -> dict: + """OID + definition of the reservations run/status index on the search path.""" + return conn.execute( + """ + SELECT 'idx_credit_llm_reservations_run_status'::regclass::oid AS oid, + pg_get_indexdef( + 'idx_credit_llm_reservations_run_status'::regclass + ) AS indexdef + """ + ).fetchone() + + +def _logical_attempt_constraint(conn) -> dict: + """OID + backing-index OID of the reservations logical-attempt UNIQUE. + + conindid is the load-bearing half: a drop+re-add keeps the constraint's + *name* but builds a brand new index, so only the OIDs can tell a converged + boot from one that rebuilt under ACCESS EXCLUSIVE. + """ + return conn.execute( + """ + SELECT con.oid AS oid, + con.conindid AS conindid, + pg_get_constraintdef(con.oid) AS condef + FROM pg_constraint AS con + WHERE con.conrelid = 'credit_llm_reservations'::regclass + AND con.conname = 'credit_llm_reservations_logical_attempt_key' + """ + ).fetchone() + + +@pg_only +def test_postgres_boot_migrates_pre_failover_reservation_table( + pg_pre_failover_reservations_url, +): + """Reproduce the #432 Render crash: existing reservations, no attempt_index, + and the three-column run/status index prod has carried since c0bcd863. + """ + pg_module.PostgresCreditsStore(pg_pre_failover_reservations_url) + + with psycopg.connect( + pg_pre_failover_reservations_url, row_factory=dict_row + ) as conn: + row = conn.execute( + """ + SELECT attempt_index, provider_id + FROM credit_llm_reservations + WHERE reservation_id = 'pre-failover-reservation' + """ + ).fetchone() + index = _run_status_index(conn) + unique_key = conn.execute( + """ + SELECT pg_get_constraintdef(oid) AS definition + FROM pg_constraint + WHERE conrelid = 'credit_llm_reservations'::regclass + AND conname = 'credit_llm_reservations_logical_attempt_key' + """ + ).fetchone() + + assert row == {"attempt_index": 0, "provider_id": None} + assert index["indexdef"].endswith( + "(run_id, status, call_index, attempt_index)" + ), index["indexdef"] + assert unique_key == { + "definition": "UNIQUE (user_id, run_id, call_index, attempt_index)" + } + + +@pg_only +def test_postgres_boot_leaves_a_repaired_run_status_index_alone( + pg_pre_failover_reservations_url, +): + """The stale-index repair must converge. credits_store is built at import, + so this migration runs on every deploy; a DROP+CREATE that fires + unconditionally rebuilds the index under ACCESS EXCLUSIVE each time. + """ + pg_module.PostgresCreditsStore(pg_pre_failover_reservations_url) + with psycopg.connect( + pg_pre_failover_reservations_url, row_factory=dict_row + ) as conn: + repaired = _run_status_index(conn) + repaired_constraint = _logical_attempt_constraint(conn) + + db_pool._reset_for_tests() + pg_module.PostgresCreditsStore(pg_pre_failover_reservations_url) + with psycopg.connect( + pg_pre_failover_reservations_url, row_factory=dict_row + ) as conn: + rebooted = _run_status_index(conn) + rebooted_constraint = _logical_attempt_constraint(conn) + + assert rebooted == repaired + # Same for the UNIQUE beside it: identical OIDs mean the second boot did + # not drop and rebuild the index behind the constraint. + assert repaired_constraint is not None + assert rebooted_constraint == repaired_constraint + + @pg_only def test_postgres_migration_adds_attempt_index_before_dependent_index( pg_legacy_credits_url, diff --git a/dashboard/backend/tests/test_store_twin_parity.py b/dashboard/backend/tests/test_store_twin_parity.py index 9048ca9d..179b8980 100644 --- a/dashboard/backend/tests/test_store_twin_parity.py +++ b/dashboard/backend/tests/test_store_twin_parity.py @@ -17,6 +17,11 @@ must repeat every lazy migration the SQLite store performs (declaring a column in ``CREATE`` alone reaches a fresh database but never a deployed one, which is the failure mode the twin's own header comment warns about). +* **Index order.** The same no-op bites a ``CREATE INDEX`` that names a + column the twin only adds by ``ALTER TABLE`` further down: on a deployed + table the index runs first and raises ``UndefinedColumn`` at import (#432 + killed the Render boot this way). Each index must sit below every ADD + COLUMN it depends on. #227 hit the first axis: it added ``live_trading_enabled`` to ``AgentStore.update_agent`` only, and every agent Configure PATCH on prod @@ -261,15 +266,45 @@ def test_postgres_twin_signatures_match_sqlite( _EXPR = "__EXPR__" # stands in for an f-string interpolation +# A bare or double-quoted identifier, optionally schema-qualified. Matching only +# the bare form (as this did until the #433 review) is not a narrower guard, it +# is a *silent* one: `ON public.t(a)` and `ON "t"(a)` simply do not match, so a +# twin written that way would sail past the ordering check below with zero +# indexes parsed and nothing to show for it. _CREATE_INDEX_KEYWORD / +# test_ddl_parser_sees_every_create_index exist to make that failure loud. +_SQL_IDENT = r'(?:"[^"]+"|[A-Za-z_][A-Za-z0-9_]*)' +_SQL_QUALIFIED = rf"(?:{_SQL_IDENT}\s*\.\s*)*({_SQL_IDENT})" +_CREATE_INDEX_KEYWORD = re.compile(r"CREATE\s+(?:UNIQUE\s+)?INDEX\b", re.IGNORECASE) + _CREATE_TABLE = re.compile( - r"CREATE\s+TABLE\s+(?:IF\s+NOT\s+EXISTS\s+)?([A-Za-z_][A-Za-z0-9_]*)\s*\(", + rf"CREATE\s+TABLE\s+(?:IF\s+NOT\s+EXISTS\s+)?{_SQL_QUALIFIED}\s*\(", re.IGNORECASE, ) _ADD_COLUMN = re.compile( - r"ALTER\s+TABLE\s+([A-Za-z_][A-Za-z0-9_]*)\s+ADD\s+COLUMN\s+" - r"(?:IF\s+NOT\s+EXISTS\s+)?([A-Za-z_][A-Za-z0-9_]*)", + rf"ALTER\s+TABLE\s+(?:IF\s+EXISTS\s+)?(?:ONLY\s+)?{_SQL_QUALIFIED}" + rf"\s+ADD\s+COLUMN\s+(?:IF\s+NOT\s+EXISTS\s+)?{_SQL_QUALIFIED}", + re.IGNORECASE, +) +_CREATE_INDEX = re.compile( + rf"CREATE\s+(?:UNIQUE\s+)?INDEX\s+(?:CONCURRENTLY\s+)?" + rf"(?:IF\s+NOT\s+EXISTS\s+)?{_SQL_QUALIFIED}\s+ON\s+(?:ONLY\s+)?" + rf"{_SQL_QUALIFIED}\s*(?:USING\s+[A-Za-z_]+\s*)?\(", re.IGNORECASE, ) + + +def _name(raw: str) -> str: + """Fold one captured identifier to its comparison key. + + Postgres treats a quoted identifier as case-sensitive and an unquoted one + as folded to lower case, so ``"T"`` and ``T`` really are different tables. + This guard collapses them anyway: over-matching makes it flag an ordering + it should not (loud, and fixable), while under-matching makes it miss the + #432 boot crash (silent, and shipped). + """ + return raw.strip().strip('"').lower() +_SQL_STRING = re.compile(r"'(?:[^']|'')*'") +_IDENTIFIER = re.compile(r"[A-Za-z_][A-Za-z0-9_]*") # Tables a Postgres twin deliberately never creates, keyed by twin class name. # The default is that both twins declare the same tables -- a divergence is # normally the #227 bug -- so every entry needs its reason recorded here, and @@ -299,13 +334,48 @@ def test_postgres_twin_signatures_match_sqlite( } +def _blank_sql_comments(literal: str) -> str: + """``literal`` with every ``--`` comment replaced by spaces, same length. + + Two reasons this is not cosmetic. A commented-out ``CREATE INDEX`` would + otherwise be read as real DDL, and prose inside a comment ("CREATE INDEX + IF NOT EXISTS matches by name ...", which the credits twin really does + say) would be counted as a statement the parser failed to understand. + Length is preserved so every offset the callers compare stays valid. + """ + out = list(literal) + i, n = 0, len(literal) + while i < n: + if literal[i] == "'": + i = _skip_quoted(literal, i) + continue + if literal.startswith("--", i): + while i < n and literal[i] != "\n": + out[i] = " " + i += 1 + continue + i += 1 + return "".join(out) + + +def _ddl_literals(source: str) -> list[str]: + """``_string_literals`` with SQL comments blanked -- the SQL readers' view.""" + return [_blank_sql_comments(literal) for literal in _string_literals(source)] + + def _string_literals(source: str) -> list[str]: - """Every string literal in a module, with f-strings reassembled. + """Every string literal in a module, in source order, f-strings reassembled. Adjacent plain literals are folded by the parser, so a statement split across source lines arrives as one string. f-strings become one JoinedStr whose interpolations collapse to a placeholder -- enough to read the column name, which is never interpolated. + + Source order is what the index-ordering guard reads as a *proxy for* + execution order -- see that test's docstring for where the two come apart. + ``ast.walk`` is breadth-first, so a statement nested one level deeper + (inside an ``if``, say) would otherwise sort after a shallower statement + that follows it in the file. """ tree = ast.parse(source) @@ -314,18 +384,22 @@ def _string_literals(source: str) -> list[str]: if isinstance(node, ast.JoinedStr): nested.update(id(inner) for inner in ast.walk(node) if inner is not node) - literals = [] + positioned: list[tuple[int, int, str]] = [] for node in ast.walk(tree): if isinstance(node, ast.JoinedStr): - literals.append( - "".join( - ( - part.value - if isinstance(part, ast.Constant) - and isinstance(part.value, str) - else _EXPR - ) - for part in node.values + positioned.append( + ( + node.lineno, + node.col_offset, + "".join( + ( + part.value + if isinstance(part, ast.Constant) + and isinstance(part.value, str) + else _EXPR + ) + for part in node.values + ), ) ) elif ( @@ -333,8 +407,49 @@ def _string_literals(source: str) -> list[str]: and isinstance(node.value, str) and id(node) not in nested ): - literals.append(node.value) - return literals + positioned.append((node.lineno, node.col_offset, node.value)) + positioned.sort(key=lambda item: item[:2]) + return [value for _, _, value in positioned] + + +class _IndexReference(NamedTuple): + name: str + table: str + #: every identifier the index names -- key columns, INCLUDE, the partial + #: WHERE predicate -- because a missing column anywhere in it is fatal + columns: frozenset[str] + + +def _index_references(literal: str) -> list[tuple[int, _IndexReference]]: + """``(offset, reference)`` for every ``CREATE INDEX`` in one literal. + + Identifiers are collected from the whole statement after ``ON `` + up to its ``;`` (or the literal's end), minus string literals, so a + partial index's predicate counts too. Keywords (DESC, WHERE, TRUE ...) + come along; the guard only ever intersects this set with a table's + migrated columns, so they cost nothing. + """ + references = [] + for match in _CREATE_INDEX.finditer(literal): + open_paren = match.end() - 1 + body = _balanced_body(literal, open_paren) + if body is None: + continue + close_paren = open_paren + len(body) + 1 + terminator = literal.find(";", close_paren) + predicate = literal[close_paren + 1 : None if terminator == -1 else terminator] + clause = _SQL_STRING.sub(" ", f"{body} {predicate}") + references.append( + ( + match.start(), + _IndexReference( + _name(match.group(1)), + _name(match.group(2)), + frozenset(tok.lower() for tok in _IDENTIFIER.findall(clause)), + ), + ) + ) + return references def _skip_quoted(text: str, i: int) -> int: @@ -435,16 +550,16 @@ class _Schema(NamedTuple): def _parse_ddl(source: str) -> _Schema: declared: dict[str, set[str]] = {} migrated: dict[str, set[str]] = {} - for literal in _string_literals(source): + for literal in _ddl_literals(source): for match in _CREATE_TABLE.finditer(literal): body = _balanced_body(literal, match.end() - 1) if body is None: continue - declared.setdefault(match.group(1).lower(), set()).update( + declared.setdefault(_name(match.group(1)), set()).update( _column_names(body) ) for match in _ADD_COLUMN.finditer(literal): - table, column = match.group(1).lower(), match.group(2).lower() + table, column = _name(match.group(1)), _name(match.group(2)) declared.setdefault(table, set()).add(column) migrated.setdefault(table, set()).add(column) return _Schema(declared, migrated) @@ -509,6 +624,189 @@ def _init_schema(self): assert schema.migrated == {"widgets": {"retired", "mode", "note"}} +def test_string_literals_come_back_in_source_order(): + """The ordering guard below reads position as execution order.""" + source = ( + "def f(x):\n" + " if x:\n" + " a = 'nested first'\n" + " b = 'shallow second'\n" + ) + + assert _string_literals(source) == ["nested first", "shallow second"] + + +def test_ddl_parser_extracts_index_references(): + """Guards the ordering check below from passing vacuously.""" + source = ''' +cur.execute( + "CREATE INDEX IF NOT EXISTS idx_widgets_owner " + "ON widgets(owner_id, updated_at DESC)" +) +cur.execute( + """ + CREATE UNIQUE INDEX IF NOT EXISTS uq_widgets_default + ON widgets (owner_id) WHERE is_default = TRUE AND label <> 'retired'; + CREATE INDEX idx_widgets_note ON widgets USING btree (lower(note)); + """ +) +''' + references = [ + reference + for literal in _ddl_literals(source) + for _, reference in _index_references(literal) + ] + + assert [reference.name for reference in references] == [ + "idx_widgets_owner", + "uq_widgets_default", + "idx_widgets_note", + ] + assert {reference.table for reference in references} == {"widgets"} + assert {"owner_id", "updated_at"} <= references[0].columns + # The partial-index predicate counts: a column missing there is just as + # fatal. The quoted value must not, or a column literally named "retired" + # would be reported as referenced. + assert {"owner_id", "is_default", "label"} <= references[1].columns + assert "retired" not in references[1].columns + assert "note" in references[2].columns + + +def test_ddl_parser_reads_qualified_and_quoted_identifiers(): + """Schema-qualified and quoted DDL must parse, not silently vanish. + + Before the #433 review the identifier patterns accepted a bare name only, + so ``ON public.t(a)`` and ``ON "t"(a)`` matched nothing at all -- the + ordering guard would have reported zero indexes for such a twin and passed. + """ + source = ''' +cur.execute( + """ + ALTER TABLE public.widgets ADD COLUMN IF NOT EXISTS "owner_id" INTEGER; + CREATE INDEX IF NOT EXISTS idx_q ON public.widgets(owner_id); + CREATE UNIQUE INDEX uq_q ON "widgets"("owner_id", label); + """ +) +''' + literal = _ddl_literals(source)[0] + + references = [reference for _, reference in _index_references(literal)] + assert [reference.name for reference in references] == ["idx_q", "uq_q"] + assert {reference.table for reference in references} == {"widgets"} + assert "owner_id" in references[0].columns + assert {"owner_id", "label"} <= references[1].columns + + added = [ + (_name(match.group(1)), _name(match.group(2))) + for match in _ADD_COLUMN.finditer(literal) + ] + assert added == [("widgets", "owner_id")] + + +def test_ddl_parser_ignores_sql_comments(): + """A ``--`` comment is prose, not DDL -- in both directions. + + The credits twin's migration really does contain the sentence "CREATE + INDEX IF NOT EXISTS matches by name" inside a comment, so a parser that + reads comments both invents an index and, via the coverage guard below, + accuses itself of failing to parse one. + """ + source = ''' +cur.execute( + """ + -- CREATE INDEX IF NOT EXISTS idx_commented ON widgets(owner_id); + CREATE INDEX IF NOT EXISTS idx_real ON widgets(owner_id); -- trailing note + """ +) +''' + literal = _ddl_literals(source)[0] + assert [name for _, (name, *_rest) in _index_references(literal)] == ["idx_real"] + assert len(_CREATE_INDEX_KEYWORD.findall(literal)) == 1 + + +@pytest.mark.parametrize( + "sqlite_mod,sqlite_cls,postgres_mod,postgres_cls", _TWINS, ids=_TWIN_IDS +) +def test_ddl_parser_sees_every_create_index( + sqlite_mod, sqlite_cls, postgres_mod, postgres_cls +): + """Every CREATE INDEX in a twin's DDL must actually parse. + + This is the anti-vacuity guard for the ordering test below. That test can + only report an index it managed to read, so an unparsed spelling does not + fail it -- it empties it. Counting the keyword and the parsed statements + separately is the only way a regex blind spot shows up as a failure rather + than as a green run over an unchecked twin. + """ + literals = _ddl_literals(_module_source_path(postgres_mod).read_text("utf-8")) + + keyword_hits = sum(len(_CREATE_INDEX_KEYWORD.findall(lit)) for lit in literals) + parsed = sum(len(_index_references(lit)) for lit in literals) + + assert parsed == keyword_hits, ( + f"{postgres_cls}: {keyword_hits} CREATE INDEX statement(s) in the DDL " + f"but only {parsed} parsed. _CREATE_INDEX has a blind spot, and the " + f"ordering guard silently skips whatever it cannot read." + ) + + +@pytest.mark.parametrize( + "sqlite_mod,sqlite_cls,postgres_mod,postgres_cls", _TWINS, ids=_TWIN_IDS +) +def test_postgres_twin_indexes_a_migrated_column_only_after_adding_it( + sqlite_mod, sqlite_cls, postgres_mod, postgres_cls +): + """The #432 Render boot crash, as a rule rather than a credits-only guard. + + An ``ALTER TABLE t ADD COLUMN IF NOT EXISTS c`` exists because some + deployed table predates ``c``. On that deployment ``CREATE TABLE IF NOT + EXISTS`` no-ops, so a ``CREATE INDEX`` naming ``c`` that runs *before* + the ALTER raises UndefinedColumn at import -- fatal for a store built at + module scope, and invisible to CI, whose Postgres is empty on every run + and therefore only ever exercises the CREATE path. + + Scope: this reads *source* position, which is a proxy for execution order, + not the thing itself. It holds for a twin whose DDL literals run where they + are written, which is every twin today for the columns that matter. It + already does not hold in general: ``users_postgres.py`` defines + ``AUTH_SESSIONS_DDL`` near the top of the module but executes it *after* + the inline ``ALTER TABLE users ADD COLUMN`` statements further down. + Nothing crosses tables there, so the proxy costs nothing today -- but a + future ``CREATE INDEX`` inside a hoisted constant, over a column added by + an ALTER executed earlier, would be reported as too-early when it is fine, + and the mirror case would pass while being the #432 bug. Read a failure + here as "check the execution order", not as proof of one. + """ + source = _module_source_path(postgres_mod).read_text(encoding="utf-8") + literals = _ddl_literals(source) + + first_added: dict[tuple[str, str], tuple[int, int]] = {} + for position, literal in enumerate(literals): + for match in _ADD_COLUMN.finditer(literal): + key = (_name(match.group(1)), _name(match.group(2))) + first_added.setdefault(key, (position, match.start())) + + too_early = [] + for position, literal in enumerate(literals): + for offset, reference in _index_references(literal): + for column in sorted(reference.columns): + added_at = first_added.get((reference.table, column)) + if added_at is not None and added_at > (position, offset): + too_early.append( + f" {reference.name} indexes {reference.table}.{column} " + f"before ALTER TABLE {reference.table} ADD COLUMN IF NOT " + f"EXISTS {column}" + ) + + assert not too_early, ( + f"{postgres_cls} creates an index on a column its own migration adds " + f"later. On a deployment whose table predates that column the CREATE " + f"TABLE no-ops and the CREATE INDEX raises UndefinedColumn at import " + f"(the #432 Render boot crash). Move the CREATE INDEX below the ADD " + f"COLUMN:\n" + "\n".join(too_early) + ) + + @pytest.mark.parametrize( "sqlite_mod,sqlite_cls,postgres_mod,postgres_cls", _TWINS, ids=_TWIN_IDS ) diff --git a/docs/superpowers/specs/2026-09-01-postgres-attempt-index-migration-order-design.md b/docs/superpowers/specs/2026-09-01-postgres-attempt-index-migration-order-design.md index dbc3ce58..eac6ce15 100644 --- a/docs/superpowers/specs/2026-09-01-postgres-attempt-index-migration-order-design.md +++ b/docs/superpowers/specs/2026-09-01-postgres-attempt-index-migration-order-design.md @@ -61,3 +61,43 @@ The migration continues to run in the existing PostgreSQL transaction. Any later ## Deployment After the hotfix PR merges, manually trigger a Render deployment because the service has `autoDeploy` disabled. Verify the deployed commit reaches `live`, confirm the startup migration no longer raises `UndefinedColumn`, then run one platform-model smoke test that can exercise OpenRouter-to-CommonStack failover. + +## Amendment (PR #433, 2026-09-02): the index may already exist with the wrong columns + +The design above assumes a legacy table has *no* `idx_credit_llm_reservations_run_status`. Production had one: commit `c0bcd863` (2026-08-24) created it over `(run_id, status, call_index)`, and `CREATE INDEX IF NOT EXISTS` matches by name alone, so the bare statement in the migration DDL no-ops against that table and the four-column definition never lands. The migration now drops the index only when `pg_get_indexdef` reports a column list other than `(run_id, status, call_index, attempt_index)`, then recreates it. The drop is conditional on purpose: this DDL runs at import on every deploy, and an unconditional DROP+CREATE would rebuild the index under ACCESS EXCLUSIVE each time instead of converging. + +Pinned by `test_postgres_boot_migrates_pre_failover_reservation_table` (starts from the exact pre-#432 table, stale index included) and `test_postgres_boot_leaves_a_repaired_run_status_index_alone` (a second boot keeps the same index object). `test_store_twin_parity.py` now also checks that every Postgres twin creates an index only below the `ADD COLUMN` of any column it names, which is the general form of this defect. + +## Amendment (PR #433 review follow-up, 2026-09-04): what converges, and what does not + +Two corrections to the amendment above. + +**The conditional drop needs `IF EXISTS` anyway.** The `IF EXISTS (...)` predicate that +decides whether to drop is evaluated before the `DROP INDEX` takes its lock, so two +processes booting against the same database can both pass it. The loser then raises +`index "..." does not exist`, which aborts `_init_schema`; because `credits_store` is +built at import, that takes down the whole app rather than one surface. Conditionality +comes from the drop's *position inside the guard*, not from the absence of `IF EXISTS`, +and the source guard no longer asserts otherwise. + +**Converging was true of the index and false of everything beside it.** The migration +drops and re-adds thirteen constraints unconditionally on every boot. One of them — +`credit_llm_reservations_logical_attempt_key` — is a `UNIQUE`, i.e. exactly the full +index build under ACCESS EXCLUSIVE that the index repair was written to avoid, on the +same table. It is now guarded too: skipped when `pg_constraint.conkey` already names +`(user_id, run_id, call_index, attempt_index)`. Column identity is read from `conkey` +rather than matched against `pg_get_constraintdef` text, which is a rendering and drifts +between Postgres versions; a mismatch falls back to the previous drop+add. + +The remaining twelve — the `CHECK`s on `credit_llm_reservations` and +`credit_ledger_entries`, and the `actor_user_id` foreign key — are **deliberately left +unconditional**, each costing a validating full-table scan per boot. Recognising an +existing `CHECK` has no `conkey` equivalent; it means comparing deparsed SQL text, and a +guard that silently stops matching converges to nothing while still looking correct. +Both tables sit behind a disabled billing flag today. Revisit if either grows — the cost +is real, it is just not yet worth buying with a fragile predicate. + +`test_postgres_boot_leaves_a_repaired_run_status_index_alone` now pins the constraint's +`oid` and `conindid` across a second boot as well. `conindid` is the load-bearing half: +a drop and re-add keeps the constraint's *name*, so only the OID of the backing index +distinguishes a converged boot from a rebuilt one.