praefect sql-migrate discards PlanMigration error and exits 0 when planning fails

Summary

praefect sql-migrate reports success and exits 0 even when it fails to plan (and therefore apply) any migrations. In Omnibus, gitlab-ctl reconfigure trusts the exit code, prints bash[migrate praefect database] ran successfully, and continues. Praefect then crash-loops on startup against a database missing tables required by the new version.

Observed during a 19.1.3 → 19.2.2 Linux package upgrade. Internal context: gitlab-com/request-for-help#5357 (confidential). The same symptom class was seen previously in gitlab-com/request-for-help#923 (column "verified_at" does not exist (SQLSTATE 42703) until praefect sql-migrate was run manually).

Root cause

In internal/cli/praefect/subcmd_sql_migrate.go#L71 the error from PlanMigration is discarded:

// Find all migrations that are currently down.
planMigrations, _, _ := migrationSet.PlanMigration(db, "postgres", planSource, migrate.Up, 0)

subCmd := progname + " " + cmd.Name
if len(planMigrations) == 0 {
    fmt.Fprintf(cmd.Writer, "%s: all migrations are up\n", subCmd)
    return nil
}

When PlanMigration fails (DB connectivity/auth error, failure to create or read the migrations table, unknown-migration error when ignore-unknown=false, etc.) it returns a nil plan and an error. The nil plan is indistinguishable from "nothing to do", so the command prints all migrations are up and returns nil, resulting in exit code 0.

Steps to reproduce

  1. Configure Praefect with a database that PlanMigration cannot read (e.g. wrong credentials, unreachable host, or a DB where the schema_migrations table cannot be created).
  2. Run praefect -config /var/opt/gitlab/praefect/config.toml sql-migrate.
  3. Observe praefect sql-migrate: all migrations are up and $? == 0.

Expected behaviour

sql-migrate returns a non-zero exit code and a clear error whenever planning fails, so gitlab-ctl reconfigure fails loudly rather than reporting success.

Proposed fix

planMigrations, _, err := migrationSet.PlanMigration(db, "postgres", planSource, migrate.Up, 0)
if err != nil {
    return fmt.Errorf("planning migrations: %w", err)
}

Plus a test covering the failure path.

Additional asks

  • Consider a backport to 19.2.x.
  • Audit other Praefect subcommands for the same discarded-error pattern (e.g. sql-migrate-status, sql-migrate-down).