Skip to content

Migration runner skips a version already recorded under a different name, dropping the DDL silently #389

Description

@beardthelion

What happens

run_pending_migrations decides whether a migration has already run by looking only at its version number:

let already: bool = sqlx::query(
    "SELECT EXISTS(SELECT 1 FROM schema_migrations WHERE version = $1) AS applied",
)
.bind(m.version)
...
if already {
    continue;
}

crates/gitlawb-node/src/db/mod.rs:381-393 on main (bfc44f9). The name column is written when a migration is applied and never read back for comparison. So when two branches independently claim the same version number, whichever merges second has its DDL skipped in full, migrate() returns Ok, and the startup log reports a healthy schema. The node then 500s on every statement naming the object that migration was supposed to create.

This is live right now: three open PRs claim version 28 (#386, #384, #327), and #333 and #347 both claim 30 and 31.

Reproduction

Against origin/main at bfc44f9, with a real Postgres. Roll a fresh database back to a pre-v26 schema, record version 26 under a name from another branch, and migrate:

sqlx::query("ALTER TABLE pin_repair_sweep DROP COLUMN discovery_cursor_created_at").execute(&db.pool).await.unwrap();
sqlx::query("ALTER TABLE pin_repair_sweep DROP COLUMN discovery_cursor_id").execute(&db.pool).await.unwrap();
sqlx::query("DELETE FROM schema_migrations WHERE version = 26").execute(&db.pool).await.unwrap();
sqlx::query("INSERT INTO schema_migrations (version, name, applied_at) VALUES ($1, $2, $3)")
    .bind(26i32).bind("some_other_branch_v26").bind("2026-08-31T00:00:00Z")
    .execute(&db.pool).await.unwrap();

db.migrate().await.expect("migrate() must not error");

migrate() returns Ok, and discovery_cursor_created_at does not exist:

REPRO: migrate() returned Ok but v26's column is absent; the DDL was skipped
because version 26 was already recorded under the name some_other_branch_v26

Control, same setup but with no row at version 26 at all: migrate() reapplies the DDL and the column comes back, so the skip is caused by the recorded version and not by a failed ALTER.

Suggested fix

Read the recorded name and refuse to start rather than skipping silently. I ran this against the suite:

let recorded: Option<String> =
    sqlx::query_scalar("SELECT name FROM schema_migrations WHERE version = $1")
        .bind(m.version)
        .fetch_optional(&self.pool)
        .await?;

if let Some(recorded_name) = recorded {
    if recorded_name != m.name {
        anyhow::bail!(
            "migration v{} is recorded as \"{}\" but this build carries \"{}\": \
             two branches claimed the same version and one body was never applied. \
             Renumber the newer migration rather than starting on this schema.",
            m.version, recorded_name, m.name
        );
    }
    continue;
}

With that in place the reproduction above aborts with the mismatch named instead of returning Ok, and the rest of the suite is unaffected: 13/13 migration tests and 96/96 db:: tests pass, including the legacy-install backfill and the existing upgrade-path tests. It is a startup abort, so an operator who somehow has a genuinely diverged database gets a loud failure instead of a node serving 500s, which is the tradeoff I would want here.

Severity

P2. It takes two branches claiming a version to trigger, so it is not reachable by an untrusted caller, but the failure mode is a production node running on a schema it believes is complete, with nothing in the log to say otherwise. The version-number collision it depends on is a normal consequence of parallel branches and is already present across five open PRs.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    crate:nodegitlawb-node — the serving node and REST APIkind:bugDefect fix — wrong or unsafe behaviorsev:highMajor break or real security/trust risk, no easy workaroundsubsystem:storageBlob/object store, Arweave, IPFS, archives

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions