Run schema upgrades in one transaction and roll back stray failures

[laffer1]

Lands #191 on main.

Why this PR exists

#191 was opened with fix-delete-txn-rollback as its base branch rather than main. GitHub merged it into that branch as 583015d and marked it merged. When #190 was then squash-merged into main, only #190's own commit was carried over, so #191's changes never reached main. Main's mport_upgrade_master_schema still runs upgrade steps with no error checking and no rollback.

This is a clean cherry-pick of 583015d onto current main, with no conflicts and no other changes.

What #191 does

• mport_upgrade_master_schema runs every step and the user_version bump inside one BEGIN IMMEDIATE transaction, stops at the first failing step, and rolls back. Step 12to13 drops its own nested transaction.
• verify rolls back when its COMMIT fails; merge rolls back on any failure inside its per-bundle transaction.
• Rollbacks in verify and the orphan purge go through sqlite3_exec so a failing ROLLBACK cannot overwrite the error being reported.
• New test fakes a version 11 registry, makes step 12to13 fail, and checks that both the version and the rows step 11to12 added are rolled back, then that a retry on the same connection succeeds.

Verification on this branch

• Full tree builds under -Werror.
• kyua test: 92/92 passed, including the new upgrade rollback test.

Once this merges, fix-delete-txn-rollback can be deleted.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VqVYKWFCX58Cb5NiGHHjzo

Summary by Sourcery

Make schema upgrades and related database operations roll back cleanly on failure.

Bug Fixes:

• Make schema upgrades atomic so any failed upgrade step restores the original schema version and data, while leaving the database connection ready for retry.
• Preserve original database errors when rollback operations are needed during verification, orphan cleanup, and bundle merging.

Enhancements:

• Run all master schema migration steps and the database version update within a single transaction, with failure checking at each step.

Tests:

• Add coverage verifying that a failed schema upgrade rolls back earlier changes and that a subsequent retry succeeds.

[sourcery-ai]

Sorry @laffer1, you've used your own review budget of 250,000 diff characters for the last 7 days.

You can request another review in 2 days and 2 hours by commenting @sourcery-ai review. Upgrade to get a review now.

[github-actions]

Claude finished @laffer1's task in 1m 55s —— View job

---

Code Review: Run schema upgrades in one transaction and roll back stray failures

Tasks

• Gather context (PR body, diff, changed files)
• Understand request (cherry-pick of Run schema upgrades in one transaction and roll back stray failures #191 onto main; review for correctness/safety)
• Review libmport/db.c (schema upgrade transaction)
• Review libmport/install_primative.c
• Review libmport/merge_primative.c
• Review libmport/verify.c
• Review tests/mport_install_test.c
• Post final review

Summary

This is a clean, well-scoped cherry-pick that does what the PR body claims. I traced every changed function against its callers and the shared error-handling macros (RUN_SQL, RETURN_CURRENT_ERROR, mport_db_do) rather than just re-reading the diff in isolation.

libmport/db.c

• mport_upgrade_master_schema now wraps run_master_schema_upgrades() + mport_set_database_version() + COMMIT in one BEGIN IMMEDIATE transaction, rolling back via sqlite3_exec(db, "ROLLBACK", ...) directly (not mport_db_do) so a failing rollback can't clobber mport_err_code() — matches the stated intent.
• The UPGRADE_STEP macro correctly propagates each step's error via RETURN_CURRENT_ERROR inside run_master_schema_upgrades, which the caller then checks.
• The nested BEGIN/COMMIT previously inside 12to13 is correctly removed (replaced with a comment) now that the whole upgrade runs in one outer transaction — no double-transaction bug.
• Removing the old case 14: break; is safe: databaseVersion == MPORT_MASTER_VERSION (14) already short-circuits at the top of the function, so that case was dead past that guard.
• Single call site is instance.c:151 (mport_connect/init path), with no outer transaction already open, so no nesting hazard.

libmport/install_primative.c (purge_orphaned_rows) and libmport/merge_primative.c (build_stub_db)

• Both convert ad-hoc inline rollback-then-RETURN_CURRENT_ERROR blocks into a shared goto rollback label that does the same sqlite3_exec direct rollback. In merge_primative.c the label sits inside the per-file for loop with a continue; guarding the fallthrough on success — the label and jump are within the same loop iteration/scope, so this is valid and doesn't skip the sqlite3_finalize(stmt) calls (all call sites goto rollback only after stmt has already been finalized or before it was allocated).
• Both preserve parameterized queries (%Q) for table names/pkg names — no SQL injection concern introduced.

libmport/verify.c (mport_recompute_checksums)

• Rollback-on-error path switched from mport_db_do(mport->db, "ROLLBACK") to a direct sqlite3_exec call for the same clobber-avoidance reason; a new rollback branch was added for the previously-unhandled COMMIT failure case. Both stmt/update_stmt are finalized before either rollback path, no leak.

tests/mport_install_test.c

• New failed_schema_upgrade_rolls_back test forges a v11 registry, pre-creates temp_settings so step 12to13 fails, and verifies: (1) user_version stays at 11, (2) the rows step 11to12 added are rolled back, (3) a retry on the same connection succeeds after removing the blocker. This directly exercises the new transactional behavior and the "connection stays usable after rollback" guarantee. Uses existing mport_db_count helper correctly.

No correctness, memory-safety, SQL-injection, or error-propagation issues found. The pattern (rollback via raw sqlite3_exec to preserve the original error) is applied consistently across all four files.

Not verified in this session: I was unable to execute build/test commands (make, kyua test) — all Bash tool calls in this environment required approval that wasn't available. The PR body reports a clean -Werror build and 92/92 kyua test passing on this branch; that should be confirmed by CI rather than taken solely on my static review.

[sourcery-ai]

Reviewer's Guide

Schema upgrades now execute all migration steps and the version bump atomically, with fail-fast rollback that preserves the original error; related transaction failure paths were hardened, and a regression test verifies partial-upgrade recovery and retryability.

Sequence diagram for atomic schema upgrade and rollback

sequenceDiagram
    participant Caller
    participant Upgrade as mport_upgrade_master_schema
    participant Runner as run_master_schema_upgrades
    participant SQLite

    Caller->>Upgrade: mport_upgrade_master_schema(db, databaseVersion)
    Upgrade->>SQLite: mport_db_do(BEGIN IMMEDIATE TRANSACTION)
    Upgrade->>Runner: run_master_schema_upgrades(db, databaseVersion)
    Runner->>SQLite: mport_upgrade_master_schema_11to12(db)
    Runner->>SQLite: mport_upgrade_master_schema_12to13(db)
    alt upgrade step fails
        Runner-->>Upgrade: RETURN_CURRENT_ERROR
        Upgrade->>SQLite: sqlite3_exec(ROLLBACK)
        Upgrade-->>Caller: error, schema and version unchanged
    else all steps succeed
        Runner-->>Upgrade: MPORT_OK
        Upgrade->>SQLite: mport_set_database_version(db)
        Upgrade->>SQLite: mport_db_do(COMMIT TRANSACTION)
        Upgrade-->>Caller: MPORT_OK
    end

Loading

Sequence diagram for retrying a failed schema upgrade

sequenceDiagram
    participant Test
    participant Upgrade as mport_upgrade_master_schema
    participant SQLite

    Test->>Upgrade: upgrade version 11 connection
    Upgrade->>SQLite: BEGIN IMMEDIATE TRANSACTION
    Upgrade->>SQLite: mport_upgrade_master_schema_11to12(db)
    Upgrade->>SQLite: mport_upgrade_master_schema_12to13(db)
    SQLite-->>Upgrade: failure
    Upgrade->>SQLite: sqlite3_exec(ROLLBACK)
    Upgrade-->>Test: error
    Test->>SQLite: verify version and rows restored
    Test->>Upgrade: retry on same connection
    Upgrade->>SQLite: BEGIN IMMEDIATE TRANSACTION
    Upgrade->>SQLite: run remaining upgrade steps
    Upgrade->>SQLite: mport_set_database_version(db)
    Upgrade->>SQLite: COMMIT TRANSACTION
    Upgrade-->>Test: MPORT_OK

Loading

File-Level Changes

Change	Details	Files
Wrap master schema upgrades in a single transaction with fail-fast error handling and rollback.	Route each applicable upgrade step through a checked wrapper. Apply the database version bump only after all steps succeed. Rollback on step, version-update, or commit failure while preserving the original error. Remove the nested transaction from the 12-to-13 migration.	libmport/db.c
Make transactional cleanup preserve failures in orphan purging, bundle merging, and checksum verification.	Redirect transaction failures to rollback paths. Execute rollback directly through SQLite so rollback errors cannot replace the original error.	libmport/install_primative.c libmport/merge_primative.c libmport/verify.c
Add regression coverage for partial schema-upgrade failure and connection reuse.	Simulate a version-11 registry and force the 12-to-13 step to fail after an earlier step mutates data. Verify the schema version and earlier data changes are restored. Retry the upgrade on the same connection and verify successful completion.	tests/mport_install_test.c

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.