fix: disable APT timers during Supabase upgrades

ansible/files/admin_api_scripts/pg_upgrade_scripts/common.sh

@@ -14,27 +14,88 @@ function log {
 	echo "$(date -u '+%Y-%m-%d %H:%M:%S UTC') $*"
 }
 
+TIMERS_TO_DISABLE=(
+	# apt-get update and unattended-upgrades contend for the dpkg lock that
+	# initiate.sh needs, and can modify the system leading to unexpected behavior
+	# at upgrade time.
+	"apt-daily.timer"
+	"apt-daily-upgrade.timer"
+	# DO NOT add the supabase-admin-agent_salt timer to this set!
+)
+
+function unit_exists {
+	systemctl cat "$1" >/dev/null 2>&1
+}
+
+# Runtime-mask (a /run/systemd symlink) rather than disable. Runtime masks
+# should not persist across reboots, so a failure mid-upgrade will be easier to
+# mop up with a reboot.
+function disable_conflicting_timers {
+	local rc=0 timer service
+	for timer in "${TIMERS_TO_DISABLE[@]}"; do
+		unit_exists "$timer" || continue
+		systemctl is-enabled --quiet "$timer" 2>/dev/null || continue
+
+		log "Masking $timer for the duration of the upgrade"
+		systemctl mask --runtime --now "$timer" || rc=1
+
+		service="${timer%.timer}.service"
+		if unit_exists "$service"; then
+			systemctl stop "$service" || rc=1
+		fi
+	done
+	return $rc
+}
+
+function enable_conflicting_timers {
+	local rc=0 timer state
+	for timer in "${TIMERS_TO_DISABLE[@]}"; do
+		state=$(systemctl is-enabled "$timer" 2>/dev/null || true)
+		[ "$state" = "masked-runtime" ] || continue
+
+		log "Unmasking $timer"
+		systemctl unmask --runtime "$timer" || {
+			rc=1
+			continue
+		}
+		systemctl start "$timer" || rc=1
+	done
+	return $rc
+}
+
 # shellcheck disable=SC2120
 # Arguments are passed in other files
 function run_sql {
 	psql -h localhost -U supabase_admin -d postgres "$@"
 }
 
+# Wrap a db name in dbname='...' (escaping \ and ') so characters special to -d
+# parsing (=, spaces, quotes) stay part of a literal name, not a conninfo fragment.
+# psql parses a -d value containing '=' as a full conninfo string, so a customer
+# database named e.g. `host=example.com dbname=postgres` would otherwise redirect
+# this root-launched connection off-box.
+function conninfo_for_db {
+	local d="$1"
+	d="${d//\\/\\\\}"
+	d="${d//\'/\\\'}"
+	printf "dbname='%s'" "$d"
+}
+
 function ship_logs {
 	LOG_FILE=$1
 
 	if [ -z "$REPORTING_ANON_KEY" ]; then
-		echo "No reporting key found. Skipping log upload."
+		log "No reporting key found. Skipping log upload."
 		return 0
 	fi
 
 	if [ ! -f "$LOG_FILE" ]; then
-		echo "No log file found. Skipping log upload."
+		log "No log file found. Skipping log upload."
 		return 0
 	fi
 
 	if [ ! -s "$LOG_FILE" ]; then
-		echo "Log file is empty. Skipping log upload."
+		log "Log file is empty. Skipping log upload."
 		return 0
 	fi
 
@@ -59,12 +120,12 @@ function check_free_space {
 	available_kb=$(df -Pk / | awk 'NR==2 {print $4}')
 
 	if ! [[ $available_kb =~ ^[0-9]+$ ]]; then
-		echo "ERROR: could not determine free space on /; aborting upgrade."
+		log "ERROR: could not determine free space on /; aborting upgrade."
 		return 1
 	fi
 
 	if ((available_kb < required_kb)); then
-		echo "ERROR: only ${available_kb}KB free on / but ${required_kb}KB required; aborting upgrade."
+		log "ERROR: only ${available_kb}KB free on / but ${required_kb}KB required; aborting upgrade."
 		return 1
 	fi
 }
@@ -79,10 +140,10 @@ function retry {
 		wait=$((2 ** (count + 1)))
 		count=$((count + 1))
 		if [ $count -lt "$retries" ]; then
-			echo "Command $* exited with code $exit, retrying..."
+			log "Command $* exited with code $exit, retrying..."
 			sleep $wait
 		else
-			echo "Command $* exited with code $exit, no more retries left."
+			log "Command $* exited with code $exit, no more retries left."
 			return $exit
 		fi
 	done

ansible/files/admin_api_scripts/pg_upgrade_scripts/complete.sh

@@ -19,20 +19,20 @@ function wait_for_data_device {
 	local fstab_src dev=""
 	fstab_src=$(awk '$2 == "/data" {print $1}' /etc/fstab)
 	if [ -z "$fstab_src" ]; then
-		echo "No /data entry in /etc/fstab"
+		log "No /data entry in /etc/fstab"
 		return 1
 	fi
 
-	echo "Waiting for /data device ($fstab_src) to appear"
+	log "Waiting for /data device ($fstab_src) to appear"
 	for _ in $(seq 1 60); do
 		dev=$(findfs "$fstab_src" 2>/dev/null) || dev=""
 		if [ -n "$dev" ] && [ -b "$dev" ]; then
-			echo "/data device ($dev) is available"
+			log "/data device ($dev) is available"
 			return 0
 		fi
 		sleep 1
 	done
-	echo "Timed out waiting for /data device ($fstab_src)"
+	log "Timed out waiting for /data device ($fstab_src)"
 	return 1
 }
 
@@ -42,16 +42,33 @@ function cleanup {
 
 	echo "$UPGRADE_STATUS" >/tmp/pg-upgrade-status
 
+	if [ -z "$IS_CI" ]; then
+		# Warn rather than fail: this runs inside the ERR trap, and aborting here
+		# would skip ship_logs and lose the diagnostics for the actual failure.
+		enable_conflicting_timers || log "WARNING: failed to re-enable one or more timers; check 'systemctl list-timers --all' on this host"
+	fi
+
 	ship_logs "$LOG_FILE" || true
 
 	exit "$EXIT_CODE"
 }
 
+# Callers invoke this as the left operand of `||`, which suppresses set -e for the
+# whole function body — and re-enabling it in a subshell does not restore it (verified
+# on bash 5.3). So failures are tracked explicitly in rc, the same idiom as
+# analyze_partitioned_tables. Without it the function returns its last command's status
+# and a mid-function failure is reported as success.
 function execute_extension_upgrade_patches {
+	local rc=0
 	if [ -f "/var/lib/postgresql/extension/wrappers--0.3.1--0.4.1.sql" ] && [ ! -f "/usr/share/postgresql/15/extension/wrappers--0.3.0--0.4.1.sql" ]; then
-		cp /var/lib/postgresql/extension/wrappers--0.3.1--0.4.1.sql /var/lib/postgresql/extension/wrappers--0.3.0--0.4.1.sql
-		ln -s /var/lib/postgresql/extension/wrappers--0.3.0--0.4.1.sql /usr/share/postgresql/15/extension/wrappers--0.3.0--0.4.1.sql
+		# Only link once the copy landed; a dangling symlink is worse than no symlink
+		if cp /var/lib/postgresql/extension/wrappers--0.3.1--0.4.1.sql /var/lib/postgresql/extension/wrappers--0.3.0--0.4.1.sql; then
+			ln -s /var/lib/postgresql/extension/wrappers--0.3.0--0.4.1.sql /usr/share/postgresql/15/extension/wrappers--0.3.0--0.4.1.sql || rc=1
+		else
+			rc=1
+		fi
 	fi
+	return $rc
 }
 
 function execute_wrappers_patch {
@@ -122,9 +139,14 @@ EOF
 	run_sql -c "$UPDATE_WRAPPERS_SERVER_OPTIONS_QUERY"
 }
 
+# See the note on execute_extension_upgrade_patches for why failures are tracked in rc
+# rather than left to set -e. Each patch is independent, so a failure records rc and the
+# remaining patches still get applied.
 function execute_patches {
+	local rc=0
+
 	# Patch pg_net grants
-	PG_NET_ENABLED=$(run_sql -A -t -c "select count(*) > 0 from pg_extension where extname = 'pg_net';")
+	PG_NET_ENABLED=$(run_sql -A -t -c "select count(*) > 0 from pg_extension where extname = 'pg_net';") || rc=1
 
 	if [ "$PG_NET_ENABLED" = "t" ]; then
 		PG_NET_GRANT_QUERY=$(
@@ -145,11 +167,11 @@ function execute_patches {
 EOF
 		)
 
-		run_sql -c "$PG_NET_GRANT_QUERY"
+		run_sql -c "$PG_NET_GRANT_QUERY" || rc=1
 	fi
 
 	# Patching pg_cron ownership as it resets during upgrade
-	HAS_PG_CRON_OWNED_BY_POSTGRES=$(run_sql -A -t -c "select count(*) > 0 from pg_extension where extname = 'pg_cron' and extowner::regrole::text = 'postgres';")
+	HAS_PG_CRON_OWNED_BY_POSTGRES=$(run_sql -A -t -c "select count(*) > 0 from pg_extension where extname = 'pg_cron' and extowner::regrole::text = 'postgres';") || rc=1
 
 	if [ "$HAS_PG_CRON_OWNED_BY_POSTGRES" = "t" ]; then
 		RECREATE_PG_CRON_QUERY=$(
@@ -168,13 +190,13 @@ EOF
 EOF
 		)
 
-		run_sql -c "$RECREATE_PG_CRON_QUERY"
+		run_sql -c "$RECREATE_PG_CRON_QUERY" || rc=1
 	fi
 
 	# Patching pgmq ownership as it resets during upgrade
-	HAS_PGMQ=$(run_sql -A -t -c "select count(*) > 0 from pg_extension where extname = 'pgmq';")
+	HAS_PGMQ=$(run_sql -A -t -c "select count(*) > 0 from pg_extension where extname = 'pgmq';") || rc=1
 	if [ "$HAS_PGMQ" = "t" ]; then
-		run_sql -c "update pg_extension set extowner = 'postgres'::regrole where extname = 'pgmq';"
+		run_sql -c "update pg_extension set extowner = 'postgres'::regrole where extname = 'pgmq';" || rc=1
 	fi
 
 	# Patch to handle upgrading to pgsodium-less Vault
@@ -218,7 +240,7 @@ EOF
     \$\$;
 EOF
 	)
-	run_sql -c "$REENCRYPT_VAULT_SECRETS_QUERY"
+	run_sql -c "$REENCRYPT_VAULT_SECRETS_QUERY" || rc=1
 
 	GRANT_PREDEFINED_ROLES_TO_POSTGRES_QUERY=$(
 		cat <<EOF
@@ -236,12 +258,14 @@ EOF
     \$\$;
 EOF
 	)
-	run_sql -c "$GRANT_PREDEFINED_ROLES_TO_POSTGRES_QUERY"
+	run_sql -c "$GRANT_PREDEFINED_ROLES_TO_POSTGRES_QUERY" || rc=1
+
+	return $rc
 }
 
 function complete_pg_upgrade {
 	if [ -f /tmp/pg-upgrade-status ]; then
-		echo "Upgrade job already started. Bailing."
+		log "Upgrade job already started. Bailing."
 		exit 0
 	fi
 
@@ -250,6 +274,20 @@ function complete_pg_upgrade {
 	# Set (including from called functions, via dynamic scoping) whenever a fail-soft step failed; ships the log for visibility at the end
 	local warnings=0
 
+	# Fail-soft, unlike initiate.sh's step 0: complete.sh runs no apt/dpkg
+	# commands, and by this point the data volume has already moved — aborting
+	# would take the project down over a timer we can live without stopping.
+	if [ -z "$IS_CI" ]; then
+		log "0. Masking conflicting systemd timers"
+		retry 3 disable_conflicting_timers || {
+			log "WARNING: failed to mask one or more timers"
+			warnings=1
+		}
+		# Stopping an in-flight apt-daily-upgrade.service can leave dpkg
+		# half-configured; repair it the same way initiate.sh step 2 does
+		DEBIAN_FRONTEND=noninteractive dpkg --configure -a --force-confold || true
+	fi
+
 	log "1. Mounting data disk"
 	if [ -z "$IS_CI" ]; then
 		# Let udev finish detecting the vollume before mounting
@@ -261,11 +299,13 @@ function complete_pg_upgrade {
 		# `nofail` in /etc/fstab makes `mount -a` exit with a code of 0 even when the volume is absent
 		# In the offchance of the volume not being mounted or detected, explicitly fail here
 		if ! mountpoint -q /data; then
-			echo "FATAL: /data is not a mountpoint"
-			exit 1
+			log "FATAL: /data is not a mountpoint"
+			# fail as a command, not exit 1: exit skips the ERR trap, so cleanup
+			# would never unmask the timers or flip the status to "failed"
+			false
 		fi
 	else
-		echo "Skipping mount -a -v"
+		log "Skipping mount -a -v"
 	fi
 
 	# copying custom configurations
@@ -337,6 +377,15 @@ function complete_pg_upgrade {
 		warnings=1
 	}
 
+	# The success path never reaches cleanup(), so restore the timers here too. enable_conflicting_timers only touches units still masked by this run, so the cleanup() call is a no-op if this one ran
+	if [ -z "$IS_CI" ]; then
+		log "7. Unmasking conflicting systemd timers"
+		enable_conflicting_timers || {
+			log "WARNING: failed to re-enable one or more timers; check 'systemctl list-timers --all' on this host"
+			warnings=1
+		}
+	fi
+
 	log "Upgrade job completed"
 
 	# Clean runs ship nothing — only warn-but-completed upgrades are reported (hard failures ship via the ERR-trap cleanup)
@@ -393,30 +442,38 @@ function start_vacuum_analyze {
 	if [ "$jobs" -lt 1 ]; then
 		jobs=1
 	fi
-	PGOPTIONS='-c vacuum_cost_delay=0 -c lock_timeout=10s' vacuumdb --all --analyze-in-stages -j "$jobs" -U supabase_admin -h localhost -p 5432
+	# --skip-locked rather than a global lock_timeout: a lock_timeout error aborts the entire staged, all-databases run, and `retry` then restarts from stage 1 — so an attempt dying mid-stage-3 can overwrite the finer stats an earlier attempt already wrote with stage 1's coarse target=1. Skipping just the locked relation leaves every other table's progress intact
+	PGOPTIONS='-c vacuum_cost_delay=0' vacuumdb --all --analyze-in-stages --skip-locked -j "$jobs" -U supabase_admin -h localhost -p 5432
 }
 
 function analyze_partitioned_tables {
-	local rc=0 db stmts dbs
+	local rc=0 db conn oid oids dbs
 	# Capture the list (vs substituting into the for-list) so a failed enumeration can't trip the ERR trap inside the subshell and overwrite the status file
 	dbs=$(run_sql -X -p 5432 -A -t -c "select datname from pg_database where datallowconn") || return 1
 	# Iterate by line, not by word — datnames can contain spaces and glob characters
 	while IFS= read -r db; do
 		if [ -z "$db" ]; then
 			continue
 		fi
-		stmts=$(run_sql -X -p 5432 -d "$db" -A -t -c "select format('ANALYZE %I.%I;', n.nspname, c.relname) from pg_class c join pg_namespace n on n.oid = c.relnamespace where c.relkind = 'p' and not exists (select from pg_statistic s where s.starelid = c.oid)") || {
+		# Never pass a raw datname to -d: psql reads a value containing '=' as a conninfo string, so a database named `host=... dbname=...` would redirect this root-launched connection off-box
+		conn=$(conninfo_for_db "$db")
+		# Enumerate integer OIDs only, never names: line-based iteration would split a relation name containing a newline mid-statement (same pattern as refresh_collation.sh). Parents that already have stats are skipped so retries don't redo finished work
+		oids=$(run_sql -X -p 5432 -d "$conn" -A -t -c "select c.oid from pg_class c where c.relkind = 'p' and not exists (select from pg_statistic s where s.starelid = c.oid)") || {
 			rc=1
 			continue
 		}
-		if [ -n "$stmts" ]; then
-			# The instance is serving traffic by now: fail fast into the retry rather than queue behind a customer lock (autovacuum's ANALYZE cancels itself for us), and cap runaway many-leaf recursion rather than grind against live traffic
-			{
-				echo "set lock_timeout = '10s';"
-				echo "set statement_timeout = '10min';"
-				echo "$stmts"
-			} | run_sql -X -p 5432 -d "$db" -v ON_ERROR_STOP=1 || rc=1
-		fi
+		# One psql call per parent: batching them under ON_ERROR_STOP=1 lets the first failing parent strand every later parent in this database, and the retry then re-fails on that same parent forever
+		while IFS= read -r oid; do
+			case "$oid" in
+			'' | *[!0-9]*) continue ;;
+			esac
+			# The ANALYZE statement is built and run server-side (format %I + \gexec) so object names never round-trip through the shell. A parent dropped since enumeration yields zero rows — \gexec then runs nothing, which is the right outcome. The instance is serving traffic by now: fail fast rather than queue behind a customer lock (autovacuum's ANALYZE cancels itself for us), and cap runaway many-leaf recursion rather than grind against live traffic
+			run_sql -X -p 5432 -d "$conn" -v ON_ERROR_STOP=1 <<-EOSQL || rc=1
+				set lock_timeout = '10s';
+				set statement_timeout = '10min';
+				select format('ANALYZE %I.%I', n.nspname, c.relname) from pg_class c join pg_namespace n on n.oid = c.relnamespace where c.oid = $oid and c.relkind = 'p' \gexec
+			EOSQL
+		done <<<"$oids"
 	done <<<"$dbs"
 	return $rc
 }
@@ -425,14 +482,14 @@ case $# in
 0) ;;
 1)
 	if ! declare -F "$1" >/dev/null; then
-		echo "Error: unknown function $1" >&2
+		log "Error: unknown function $1" >&2
 		exit 1
 	fi
 	$1
 	exit
 	;;
 *)
-	echo "Error: $(basename "$0") takes 0 args or a function to call, got $*" >&2
+	log "Error: $(basename "$0") takes 0 args or a function to call, got $*" >&2
 	exit 1
 	;;
 esac