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.