From 63b10ac8042213434f1631209ec8acce20bacd4e Mon Sep 17 00:00:00 2001 From: Vijay Raghav Varada Date: Thu, 6 Aug 2026 15:41:40 +0530 Subject: [PATCH] fix(deploy): the lock_timeout prelude was invalid SQL and blocked every deploy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #368 shipped `SET lock_timeout = 5s;` — unquoted. Postgres answers "trailing junk after numeric literal", and under ON_ERROR_STOP=1 that failed the FIRST migration in the ladder. So a guard whose entire purpose was to stop a stalled session from blocking deploys blocked every deploy itself. Caught on its first real run; no DDL executed, because the prelude is the first statement in the session and the ladder stops there. Two things were wrong, and the second is the one worth keeping. 1. Quote the value. '5s' and '5000' are both accepted by Postgres; a bare 5s is a syntax error. Verified against the live server, not inferred. 2. A safety feature must not be able to brick the thing it protects. The prelude is now PROBED once, before the ladder runs. If the server rejects it — a typo'd MIGRATION_LOCK_TIMEOUT, a server that spells it differently — the runner says so loudly and applies migrations WITHOUT it. Degrading to the previous behaviour is survivable; an undeployable box is not. With the probe in place, the original unquoted bug would have produced a warning and a successful deploy instead of an outage in the deploy path. Why the tests missed it, and what changed so they cannot again: - `test_migrations_set_a_lock_timeout` asserted the string was PRESENT, not that it was valid SQL. Presence is not validity. `test_the_lock_timeout_value_is_ QUOTED` now parses the emitted statement and requires a quoted value. It scans only non-comment lines — the runner quotes the broken form in its own explanation of this bug, and a test that cannot tell an example from an instruction would fail on the documentation that prevents the next occurrence. - The shell harness's fake psql drained stdin without looking at it, so any SQL passed. It now mimics Postgres's actual rule and rejects a bare unit. Re-run with the original bug reintroduced, it reproduces "trailing junk" exactly — and the deploy still succeeds, via the new fallback. - `test_a_bad_prelude_degrades_instead_of_bricking_the_deploy` pins the fallback path itself, so a future edit cannot quietly make the guard fail closed again. Verify: uv run pytest tests/unit/test_stalled_session_cannot_freeze_the_db.py -q -> 12 passed uv run ruff check . --select F821,F601,F602,F502,F7,B006 -> All checks passed! bash -n on the runner -> OK Red-first confirmed: with the unquoted form reintroduced, test_the_lock_timeout_value_is_QUOTED fails and the fixed form passes. Behavioural, against a SQL-validating fake psql: valid prelude, clean apply -> exit 0, 1 attempt lock busy 2x then succeeds -> exit 0, 3 attempts lock busy past the retry budget -> exit 1, exactly 3 attempts MIGRATION_LOCK_TIMEOUT=nonsense -> exit 0, warns, applies without the timeout (the new fallback) No migration. No .env change. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/apply_migrations.sh | 41 ++++++++++++++-- ...st_stalled_session_cannot_freeze_the_db.py | 47 +++++++++++++++++++ 2 files changed, 84 insertions(+), 4 deletions(-) diff --git a/scripts/apply_migrations.sh b/scripts/apply_migrations.sh index f6080514..7b58aebe 100644 --- a/scripts/apply_migrations.sh +++ b/scripts/apply_migrations.sh @@ -95,6 +95,37 @@ else echo " !! $BACKUP_SCRIPT not found — applying migrations WITHOUT a backup." >&2 fi +# --- Prove the prelude PARSES before betting the whole ladder on it ---------- +# +# Learned the hard way on the very first deploy that carried it: the prelude +# shipped as `SET lock_timeout = 5s;` — unquoted — and Postgres answers +# "trailing junk after numeric literal". Under ON_ERROR_STOP=1 that failed the +# FIRST migration, so a guard whose entire purpose was to stop a stalled session +# blocking deploys blocked every deploy itself. The tests missed it because they +# asserted the string was PRESENT, not that it was valid SQL, and the harness's +# fake psql drained stdin without parsing it. +# +# Two lessons, both encoded here: +# 1. Quote the value. '5s' and '5000' are both accepted; a bare 5s is not. +# 2. A safety feature must not be able to brick the thing it protects. If the +# prelude does not parse — a typo'd MIGRATION_LOCK_TIMEOUT, a server that +# spells it differently — say so LOUDLY and fall back to running without +# it. Degrading to the previous behaviour is survivable; making the box +# undeployable is not. +LOCK_PRELUDE="SET lock_timeout = '$MIGRATION_LOCK_TIMEOUT';" +if ! printf '%s\n' "$LOCK_PRELUDE" \ + | docker exec -i "$PG_CONTAINER" \ + psql -v ON_ERROR_STOP=1 -U "$PG_USER" -d "$PG_DB" -q \ + >/dev/null 2>/tmp/lock_probe_err; then + echo " !! lock_timeout prelude REJECTED by this server:" >&2 + sed 's/^/ /' /tmp/lock_probe_err >&2 + echo " !! MIGRATION_LOCK_TIMEOUT='$MIGRATION_LOCK_TIMEOUT' is not a value it" >&2 + echo " accepts. Applying migrations WITHOUT a lock timeout — a stale" >&2 + echo " reader can once again freeze a table for every later query." >&2 + echo " Fix the value; do not leave this in place." >&2 + LOCK_PRELUDE="" +fi + say "Applying migrations to db '$PG_DB' as '$PG_USER' (container: $PG_CONTAINER)" # Apply 02+ in numeric order. Only NUMBERED migration files (NN_*.sql) are @@ -119,10 +150,12 @@ for f in $(ls "$MIGRATIONS_DIR"/[0-9][0-9]*_*.sql | sort -V); do printf " - %s ... " "$base" attempt=1 while :; do - # `SET lock_timeout` is prepended to the stream rather than passed as a psql - # flag so it lands in the SAME session as the migration, ahead of any BEGIN - # the file opens. Session-level, so one SET covers every statement in it. - if { printf 'SET lock_timeout = %s;\n' "$MIGRATION_LOCK_TIMEOUT"; cat "$f"; } \ + # The prelude is prepended to the stream rather than passed as a psql flag so + # it lands in the SAME session as the migration, ahead of any BEGIN the file + # opens. Session-level, so one SET covers every statement in it. Empty when + # the probe above rejected it, in which case the migration runs exactly as it + # did before this guard existed. + if { [ -n "$LOCK_PRELUDE" ] && printf '%s\n' "$LOCK_PRELUDE"; cat "$f"; } \ | docker exec -i "$PG_CONTAINER" \ psql -v ON_ERROR_STOP=1 -U "$PG_USER" -d "$PG_DB" -q \ >/dev/null 2>/tmp/migrate_err; then diff --git a/tests/unit/test_stalled_session_cannot_freeze_the_db.py b/tests/unit/test_stalled_session_cannot_freeze_the_db.py index 2b4264c4..1dc9343c 100644 --- a/tests/unit/test_stalled_session_cannot_freeze_the_db.py +++ b/tests/unit/test_stalled_session_cannot_freeze_the_db.py @@ -227,6 +227,53 @@ def test_migrations_set_a_lock_timeout(): assert "MIGRATION_LOCK_TIMEOUT" in src +def test_the_lock_timeout_value_is_QUOTED(): + """The bug this file previously shipped, now pinned. + + `SET lock_timeout = 5s;` is not valid SQL — Postgres answers "trailing junk + after numeric literal". Under ON_ERROR_STOP=1 that failed the FIRST + migration, so the guard meant to stop a stalled session blocking deploys + blocked every deploy itself. `'5s'` and `'5000'` are both accepted. + + The old assertion (`"SET lock_timeout" in src`) passed on the broken code, + which is exactly why presence is not a substitute for validity. + """ + # Comment lines are excluded deliberately: the runner QUOTES the broken form + # in its own explanation of this bug, and a test that cannot tell an example + # from an instruction would fail on the very documentation that prevents the + # next occurrence. + code = "\n".join( + line for line in _migration_runner().splitlines() + if not line.lstrip().startswith("#") + ) + stmts = re.findall(r"SET lock_timeout\s*=\s*[^;\n]+", code) + assert stmts, "no lock_timeout statement found in the runner's actual code" + for stmt in stmts: + value = stmt.split("=", 1)[1].strip() + assert value.startswith("'") and value.rstrip(";").endswith("'"), ( + f"unquoted lock_timeout value in {stmt!r} — Postgres rejects a bare " + f"unit like 5s, and ON_ERROR_STOP=1 turns that into a failed deploy" + ) + + +def test_a_bad_prelude_degrades_instead_of_bricking_the_deploy(): + """A safety feature must not be able to brick the thing it protects. + + If the prelude will not parse, the runner has to warn and apply migrations + WITHOUT it. Failing closed here means an un-deployable box, which is + strictly worse than the stall the guard exists to prevent. + """ + src = _migration_runner() + assert "LOCK_PRELUDE" in src, "the prelude must be a variable that can be emptied" + assert re.search(r'LOCK_PRELUDE=""', src), ( + "no fallback path — a prelude Postgres rejects would fail every migration" + ) + assert re.search(r'\[ -n "\$LOCK_PRELUDE" \]', src), ( + "the emit site must skip an emptied prelude rather than print nothing " + "meaningful into the SQL stream" + ) + + def test_the_unbounded_psql_invocation_is_gone(): """Pins the SHAPE, not just the presence of a setting.