From 984f1d6a59c1872a663d2b61a3e60453e694036c Mon Sep 17 00:00:00 2001 From: Lucas Holt Date: Tue, 8 Sep 2026 17:48:11 -0400 Subject: [PATCH] Run schema upgrades in one transaction and roll back stray failures This lands PR #191 on main. That PR was opened against the fix-delete-txn-rollback branch and merged there as 583015d, so when #190 was squash-merged to main its content was left behind on the feature branch. This is a clean cherry-pick of that squash commit. Original description: mport_upgrade_master_schema ignored the return value of every upgrade step, ran them outside any transaction, and bumped user_version even when a step had failed. A registry that hit an error part way through (a lock, a step that no longer applies) was left half upgraded with a version number that did not match its schema, and the failure was reported as success. Run every step and the version bump inside one BEGIN IMMEDIATE transaction, stop at the first failing step, and roll back. Step 12to13 loses its own nested transaction, which would now fail to start inside the outer one. Roll back the remaining transactions that could be left open on error: verify when its COMMIT fails, and merge on any failure inside its per-bundle transaction. Switch the rollbacks in verify and the orphan purge to sqlite3_exec so a failing ROLLBACK cannot overwrite the error being reported. Add a test that fakes a version 11 registry, makes step 12to13 fail, and checks the version and the rows step 11to12 added are both rolled back, then that a retry on the same connection succeeds. (cherry picked from commit 583015dd938e921001c3edc60659ee4681c8e08c) AI-Assisted-By: Claude Fable 5.1 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01VqVYKWFCX58Cb5NiGHHjzo Signed-off-by: Lucas Holt --- libmport/db.c | 94 +++++++++++++++++++++++------------- libmport/install_primative.c | 17 ++++--- libmport/merge_primative.c | 24 +++++---- libmport/verify.c | 7 ++- tests/mport_install_test.c | 63 ++++++++++++++++++++++++ 5 files changed, 153 insertions(+), 52 deletions(-) diff --git a/libmport/db.c b/libmport/db.c index 5b82f8d..d770a08 100644 --- a/libmport/db.c +++ b/libmport/db.c @@ -301,65 +301,65 @@ mport_generate_stub_schema(mportInstance *mport, sqlite3 *db) return (MPORT_OK); } -int -mport_upgrade_master_schema(sqlite3 *db, int databaseVersion) -{ - if (databaseVersion == MPORT_MASTER_VERSION) - return MPORT_OK; +/* Run one upgrade step; the caller owns the surrounding transaction. */ +#define UPGRADE_STEP(fn) \ + do { \ + if (fn(db) != MPORT_OK) \ + RETURN_CURRENT_ERROR; \ + } while (0) +static int +run_master_schema_upgrades(sqlite3 *db, int databaseVersion) +{ switch (databaseVersion) { case 0: case 1: - mport_upgrade_master_schema_0to2(db); - mport_upgrade_master_schema_2to3(db); - mport_upgrade_master_schema_4to6(db); - mport_upgrade_master_schema_6to7(db); - mport_upgrade_master_schema_7to8(db); - mport_upgrade_master_schema_8to9(db); - mport_upgrade_master_schema_9to10(db); - mport_upgrade_master_schema_10to11(db); - mport_upgrade_master_schema_11to12(db); - mport_upgrade_master_schema_12to13(db); - mport_upgrade_master_schema_13to14(db); - mport_set_database_version(db); + UPGRADE_STEP(mport_upgrade_master_schema_0to2); + UPGRADE_STEP(mport_upgrade_master_schema_2to3); + UPGRADE_STEP(mport_upgrade_master_schema_4to6); + UPGRADE_STEP(mport_upgrade_master_schema_6to7); + UPGRADE_STEP(mport_upgrade_master_schema_7to8); + UPGRADE_STEP(mport_upgrade_master_schema_8to9); + UPGRADE_STEP(mport_upgrade_master_schema_9to10); + UPGRADE_STEP(mport_upgrade_master_schema_10to11); + UPGRADE_STEP(mport_upgrade_master_schema_11to12); + UPGRADE_STEP(mport_upgrade_master_schema_12to13); + UPGRADE_STEP(mport_upgrade_master_schema_13to14); break; case 2: - mport_upgrade_master_schema_2to3(db); + UPGRADE_STEP(mport_upgrade_master_schema_2to3); /* falls through */ case 3: - mport_upgrade_master_schema_3to4(db); + UPGRADE_STEP(mport_upgrade_master_schema_3to4); /* falls through */ case 4: /* falls through */ case 5: - mport_upgrade_master_schema_4to6(db); + UPGRADE_STEP(mport_upgrade_master_schema_4to6); /* falls through */ case 6: + UPGRADE_STEP(mport_upgrade_master_schema_6to7); /* falls through */ - mport_upgrade_master_schema_6to7(db); case 7: + UPGRADE_STEP(mport_upgrade_master_schema_7to8); /* falls through */ - mport_upgrade_master_schema_7to8(db); case 8: + UPGRADE_STEP(mport_upgrade_master_schema_8to9); /* falls through */ - mport_upgrade_master_schema_8to9(db); case 9: + UPGRADE_STEP(mport_upgrade_master_schema_9to10); /* falls through */ - mport_upgrade_master_schema_9to10(db); case 10: + UPGRADE_STEP(mport_upgrade_master_schema_10to11); /* falls through */ - mport_upgrade_master_schema_10to11(db); case 11: + UPGRADE_STEP(mport_upgrade_master_schema_11to12); /* falls through */ - mport_upgrade_master_schema_11to12(db); case 12: + UPGRADE_STEP(mport_upgrade_master_schema_12to13); /* falls through */ - mport_upgrade_master_schema_12to13(db); case 13: - /* falls through */ - mport_upgrade_master_schema_13to14(db); - mport_set_database_version(db); - case 14: + UPGRADE_STEP(mport_upgrade_master_schema_13to14); break; default: RETURN_ERROR(MPORT_ERR_FATAL, "Invalid master database version"); @@ -368,6 +368,35 @@ mport_upgrade_master_schema(sqlite3 *db, int databaseVersion) return (MPORT_OK); } +/* + * Bring the master database up to MPORT_MASTER_VERSION. + * + * Every step and the version bump run in one transaction. SQLite DDL is + * transactional, so a failure part way (a locked registry, a step that no + * longer applies) leaves the schema and user_version exactly as they were + * instead of a half-upgraded database that the old version number makes + * mport try, and fail, to upgrade again on every start. + */ +int +mport_upgrade_master_schema(sqlite3 *db, int databaseVersion) +{ + if (databaseVersion == MPORT_MASTER_VERSION) + return MPORT_OK; + + if (mport_db_do(db, "BEGIN IMMEDIATE TRANSACTION") != MPORT_OK) + RETURN_CURRENT_ERROR; + + if (run_master_schema_upgrades(db, databaseVersion) != MPORT_OK || + mport_set_database_version(db) != MPORT_OK || + mport_db_do(db, "COMMIT TRANSACTION") != MPORT_OK) { + /* sqlite3_exec directly so the rollback cannot clobber the error */ + (void)sqlite3_exec(db, "ROLLBACK", NULL, NULL, NULL); + RETURN_CURRENT_ERROR; + } + + return (MPORT_OK); +} + static int mport_upgrade_master_schema_0to2(sqlite3 *db) { @@ -482,12 +511,11 @@ mport_upgrade_master_schema_12to13(sqlite3 *db) "CREATE TABLE IF NOT EXISTS conflicts (pkg text NOT NULL, conflict_pkg text NOT NULL, conflict_version text NOT NULL)"); RUN_SQL(db, "CREATE INDEX IF NOT EXISTS conflicts_pkg ON conflicts (pkg, conflict_pkg)"); RUN_SQL(db, "DROP INDEX IF EXISTS settings_name"); - RUN_SQL(db, "BEGIN TRANSACTION;"); + /* mport_upgrade_master_schema() runs the whole upgrade in one transaction */ RUN_SQL(db, "CREATE TABLE temp_settings AS SELECT MIN(rowid) as rowid, name, val FROM settings GROUP BY name;"); RUN_SQL(db, "DELETE FROM settings WHERE rowid NOT IN (SELECT rowid FROM temp_settings);"); RUN_SQL(db, "DROP TABLE temp_settings;"); - RUN_SQL(db, "COMMIT;"); RUN_SQL(db, "CREATE UNIQUE INDEX IF NOT EXISTS settings_name_unique ON settings (name)"); return (MPORT_OK); diff --git a/libmport/install_primative.c b/libmport/install_primative.c index 130b8b1..9ad556a 100644 --- a/libmport/install_primative.c +++ b/libmport/install_primative.c @@ -295,18 +295,19 @@ purge_orphaned_rows(mportInstance *mport, const char *pkg_name) for (i = 0; i < sizeof(tables) / sizeof(tables[0]); i++) { if (mport_db_do(mport->db, "DELETE FROM %s WHERE pkg=%Q", tables[i], pkg_name) != - MPORT_OK) { - (void)mport_db_do(mport->db, "ROLLBACK"); - RETURN_CURRENT_ERROR; - } + MPORT_OK) + goto rollback; } - if (mport_db_do(mport->db, "COMMIT TRANSACTION") != MPORT_OK) { - (void)mport_db_do(mport->db, "ROLLBACK"); - RETURN_CURRENT_ERROR; - } + if (mport_db_do(mport->db, "COMMIT TRANSACTION") != MPORT_OK) + goto rollback; return MPORT_OK; + +rollback: + /* sqlite3_exec directly so the rollback cannot clobber the error */ + (void)sqlite3_exec(mport->db, "ROLLBACK", NULL, NULL, NULL); + RETURN_CURRENT_ERROR; } static int diff --git a/libmport/merge_primative.c b/libmport/merge_primative.c index f116542..4110b2f 100644 --- a/libmport/merge_primative.c +++ b/libmport/merge_primative.c @@ -225,29 +225,29 @@ build_stub_db(mportInstance *mport, sqlite3 **db, const char *tmpdir, const char if (mport_db_do( *db, "CREATE TABLE unsorted AS SELECT * FROM subbundle.packages") != MPORT_OK) - RETURN_CURRENT_ERROR; + goto rollback; } else { if (mport_db_do( *db, "INSERT INTO unsorted SELECT * FROM subbundle.packages") != MPORT_OK) - RETURN_CURRENT_ERROR; + goto rollback; } if (mport_db_do(*db, "INSERT INTO assets SELECT * FROM subbundle.assets") != MPORT_OK) - RETURN_CURRENT_ERROR; + goto rollback; if (mport_db_do(*db, "INSERT INTO conflicts SELECT * FROM subbundle.conflicts") != MPORT_OK) - RETURN_CURRENT_ERROR; + goto rollback; if (mport_db_do(*db, "INSERT INTO depends SELECT * FROM subbundle.depends") != MPORT_OK) - RETURN_CURRENT_ERROR; + goto rollback; /* build our hashtable (pkgname => metadata) up */ if (mport_db_prepare(*db, &stmt, "SELECT pkg FROM subbundle.packages") != MPORT_OK) { sqlite3_finalize(stmt); - RETURN_CURRENT_ERROR; + goto rollback; } while (1) { @@ -257,23 +257,29 @@ build_stub_db(mportInstance *mport, sqlite3 **db, const char *tmpdir, const char name = sqlite3_column_text(stmt, 0); if (insert_into_table(table, name, file) != MPORT_OK) { sqlite3_finalize(stmt); - RETURN_CURRENT_ERROR; + goto rollback; } } else if (ret == SQLITE_DONE) { break; } else { SET_ERROR(MPORT_ERR_FATAL, sqlite3_errmsg(*db)); sqlite3_finalize(stmt); - RETURN_CURRENT_ERROR; + goto rollback; } } sqlite3_finalize(stmt); if (mport_db_do(*db, "COMMIT TRANSACTION") != MPORT_OK) - RETURN_CURRENT_ERROR; + goto rollback; if (mport_db_do(*db, "DETACH subbundle") != MPORT_OK) RETURN_CURRENT_ERROR; + continue; + + rollback: + /* sqlite3_exec directly so the rollback cannot clobber the error */ + (void)sqlite3_exec(*db, "ROLLBACK", NULL, NULL, NULL); + RETURN_CURRENT_ERROR; } /* just have to sort the packages (going from unsorted to packages), no big deal... ;) */ diff --git a/libmport/verify.c b/libmport/verify.c index af3fa0e..b3f1e14 100644 --- a/libmport/verify.c +++ b/libmport/verify.c @@ -281,7 +281,7 @@ mport_recompute_checksums(mportInstance *mport, mportPackageMeta *pack) if (ret != SQLITE_ROW) { /* some error occured */ SET_ERROR(MPORT_ERR_FATAL, sqlite3_errmsg(mport->db)); - mport_db_do(mport->db, "ROLLBACK"); + (void)sqlite3_exec(mport->db, "ROLLBACK", NULL, NULL, NULL); sqlite3_finalize(stmt); sqlite3_finalize(update_stmt); RETURN_CURRENT_ERROR; @@ -376,8 +376,11 @@ mport_recompute_checksums(mportInstance *mport, mportPackageMeta *pack) sqlite3_finalize(stmt); sqlite3_finalize(update_stmt); - if (mport_db_do(mport->db, "COMMIT") != MPORT_OK) + if (mport_db_do(mport->db, "COMMIT") != MPORT_OK) { + /* sqlite3_exec directly so the rollback cannot clobber the error */ + (void)sqlite3_exec(mport->db, "ROLLBACK", NULL, NULL, NULL); RETURN_CURRENT_ERROR; + } return (MPORT_OK); } diff --git a/tests/mport_install_test.c b/tests/mport_install_test.c index cd6fbfe..8a422ed 100644 --- a/tests/mport_install_test.c +++ b/tests/mport_install_test.c @@ -471,6 +471,68 @@ ATF_TC_CLEANUP(failed_delete_rolls_back, tc) cleanup_test_root(); } +/* + * A schema upgrade that fails part way must leave the registry at the version + * it started from with none of the earlier steps applied, and must leave the + * connection usable so a retry can succeed. + */ +ATF_TC_WITH_CLEANUP(failed_schema_upgrade_rolls_back); +ATF_TC_HEAD(failed_schema_upgrade_rolls_back, tc) +{ + atf_tc_set_md_var(tc, "descr", "a failed schema upgrade rolls back every step"); +} +ATF_TC_BODY(failed_schema_upgrade_rolls_back, tc) +{ + mportInstance *mport; + int settings = -1; + + (void)tc; + + mport = create_test_instance(); + ATF_REQUIRE_EQ(MPORT_MASTER_VERSION, mport_get_database_version(mport->db)); + + /* pretend the registry is at schema 11 and lacks the rows step 11to12 adds */ + ATF_REQUIRE_EQ(MPORT_OK, + mport_db_do(mport->db, "DELETE FROM settings WHERE name IN (%Q, %Q)", + MPORT_SETTING_HANDLE_RC_SCRIPTS, MPORT_SETTING_REPO_AUTOUPDATE)); + ATF_REQUIRE_EQ(MPORT_OK, mport_db_do(mport->db, "PRAGMA user_version=11")); + ATF_REQUIRE_EQ(11, mport_get_database_version(mport->db)); + + /* step 12to13 creates this table, so its presence makes that step fail + * after 11to12 has already run */ + ATF_REQUIRE_EQ(MPORT_OK, + mport_db_do(mport->db, "CREATE TABLE temp_settings (rowid int, name text, val text)")); + + ATF_REQUIRE(mport_upgrade_master_schema(mport->db, 11) != MPORT_OK); + + /* version and the rows added by the earlier step are both rolled back */ + ATF_REQUIRE_EQ(11, mport_get_database_version(mport->db)); + ATF_REQUIRE_EQ(MPORT_OK, + mport_db_count(mport->db, &settings, + "SELECT COUNT(*) FROM settings WHERE name IN (%Q, %Q)", + MPORT_SETTING_HANDLE_RC_SCRIPTS, MPORT_SETTING_REPO_AUTOUPDATE)); + ATF_REQUIRE_EQ(0, settings); + + /* no transaction is left open, so a retry goes through */ + ATF_REQUIRE_EQ(MPORT_OK, mport_db_do(mport->db, "DROP TABLE temp_settings")); + ATF_REQUIRE_MSG( + mport_upgrade_master_schema(mport->db, 11) == MPORT_OK, "%s", mport_err_string()); + ATF_REQUIRE_EQ(MPORT_MASTER_VERSION, mport_get_database_version(mport->db)); + ATF_REQUIRE_EQ(MPORT_OK, + mport_db_count(mport->db, &settings, + "SELECT COUNT(*) FROM settings WHERE name IN (%Q, %Q)", + MPORT_SETTING_HANDLE_RC_SCRIPTS, MPORT_SETTING_REPO_AUTOUPDATE)); + ATF_REQUIRE_EQ(2, settings); + + mport_instance_free(mport); +} +ATF_TC_CLEANUP(failed_schema_upgrade_rolls_back, tc) +{ + (void)tc; + + cleanup_test_root(); +} + /* * MidnightBSD does not expose arbitrary descriptors through /dev/fd/N. * Verify package installation can retain the verified descriptor instead of @@ -522,6 +584,7 @@ ATF_TP_ADD_TCS(tp) ATF_TP_ADD_TC(tp, force_reinstall_over_orphaned_rows); ATF_TP_ADD_TC(tp, failed_install_registers_nothing); ATF_TP_ADD_TC(tp, failed_delete_rolls_back); + ATF_TP_ADD_TC(tp, failed_schema_upgrade_rolls_back); return atf_no_error(); }