fix(migrator): skip transaction commit after rollback in migrateIndividual() (#2811) (#2813)

* fix(migrator): skip transaction commit after rollback in migrateIndividual() (#2811)

`migrateIndividual()` issued `transaction action="commit"` unconditionally
after its try/catch — including on the error path, where the catch block
had already issued `transaction action="rollback"`. Unlike `migrateTo()`'s
loops, there is no enclosing `for` to `break` out of, so control always
fell through to the spurious commit.

On Lucee the second action against a closed transaction is a silent no-op.
On Adobe CF 2023/2025 (and potentially BoxLang) the JDBC driver may
throw a "transaction not active" error that masks the real migration
failure, making the underlying problem harder to diagnose.

Fix: return `local.rv` from inside the catch after the rollback. Mirrors
the early-exit pattern in `migrateTo()` (which uses `break` to exit its
for loop and skip the commit); here we use `return` because the
`transaction { }` block is not enclosed in a loop.

Adds a regression spec under an isolated fixture directory
(_assets/migrator-error-path/) so the synthetic always-throw migration
does not pollute the shared 001/002/003 fixtures used by every other
migrator spec.

Diagnosed by both round-1 reviewers on #2810 and explicitly recommended
as a follow-up. See issue #2811 for the full diagnosis and pattern
comparison against migrateTo().

Signed-off-by: Peter Amiri <peter@alurium.com>

* fix(migrator): address Reviewer A/B consensus findings (round 1)

- Strip issue-number prefix from the commit-skip comment in
  Migrator.cfc; preserve the WHY (rollback already closed the
  transaction; mirrors migrateTo()'s break) per CLAUDE.md "Don't
  reference the current task".
- Strip the trailing "Issue #2811." from the synthetic fixture's
  hint attribute in 004_synthetic_failing_migration.cfc.
- Strip the issue prefix from the spec's intro comment and from
  the describe label in MigrateIndividualErrorPathSpec.cfc.
- Remove the duplicate afterEach block in
  MigrateIndividualErrorPathSpec.cfc — beforeEach already
  guarantees a clean state before every it(), so the afterEach
  was a source of confusion rather than additional safety.

The var _isCockroachDB → struct-form consistency nit (Reviewer A
finding 3 / Reviewer B convergence item) was explicitly described
by both reviewers as lowest-priority "can be addressed separately
if desired" and is left for a follow-up to keep this commit
scoped to consensus-only changes.

Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>

---------

Signed-off-by: Peter Amiri <peter@alurium.com>
Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>

Author: bpamiri

vendor/wheels/Migrator.cfc

@@ -242,6 +242,10 @@ component output="false" extends="wheels.Global"{
 				local.rv = local.rv & "Error migrating #local.migration.version#.#Chr(13) & Chr(10)##e.message##Chr(13) & Chr(10)##e.detail##Chr(13) & Chr(10)#";
 				transaction action="rollback";
 				StructDelete(request, "$wheelsTransactionWrapper");
+				// Skip the commit below — rollback already closed the transaction.
+				// Mirrors the `break` in migrateTo()'s catch (no enclosing loop
+				// here, so we return instead).
+				return local.rv;
 			}
 			StructDelete(request, "$wheelsTransactionWrapper");
 			transaction action="commit";

vendor/wheels/tests/_assets/migrator-error-path/migrations/004_synthetic_failing_migration.cfc

@@ -0,0 +1,14 @@
+component extends="wheels.migrator.Migration" hint="Synthetic migration whose up() throws — exercises the migrateIndividual() rollback path." {
+
+	function up() {
+		throw(
+			type    = "Wheels.Test.SyntheticFailure",
+			message = "synthetic failure",
+			detail  = "intentional failure to exercise the rollback path in migrateIndividual()"
+		);
+	}
+
+	function down() {
+	}
+
+}

vendor/wheels/tests/specs/migrator/MigrateIndividualErrorPathSpec.cfc

@@ -0,0 +1,75 @@
+component extends="wheels.WheelsTest" {
+
+	include "helperFunctions.cfm";
+
+	function beforeAll() {
+		migration = CreateObject("component", "wheels.migrator.Migration").init();
+		migrator = CreateObject("component", "wheels.Migrator").init(
+			migratePath = "/wheels/tests/_assets/migrator-error-path/migrations/",
+			sqlPath = "/wheels/tests/_assets/migrator-error-path/sql/"
+		);
+	}
+
+	function run() {
+
+		var _isCockroachDB = CreateObject("component", "wheels.migrator.Migration").init().adapter.adapterName() == "CockroachDB";
+
+		// When migrateIndividual()'s up() throws, the catch block must not fall
+		// through to `transaction action="commit"` after the rollback. On Lucee
+		// the spurious commit is a silent no-op; on Adobe CF 2023/2025 (and
+		// potentially BoxLang) it can throw a JDBC "transaction not active"
+		// error that masks the real migration failure. This spec asserts the
+		// post-fix contract on every engine.
+		describe("migrateIndividual() error path", () => {
+
+			beforeEach(() => {
+				deleteMigratorVersions(2);
+				try {
+					queryExecute(
+						"DELETE FROM #application.wheels.migratorTableName# WHERE version = '004'",
+						{},
+						{ datasource = application.wheels.dataSourceName }
+					);
+				} catch (any e) {}
+				$cleanSqlDirectory();
+			});
+
+			it("returns normally when the migration's up() throws", () => {
+				if (_isCockroachDB) return;
+				var rv = "";
+				var threw = false;
+				var thrownMessage = "";
+				try {
+					rv = migrator.migrateIndividual("004");
+				} catch (any e) {
+					threw = true;
+					thrownMessage = e.message;
+				}
+				expect(threw).toBeFalse(
+					"migrateIndividual() should catch the migration error and return its message, "
+					& "not propagate an exception. Got: " & thrownMessage
+				);
+				expect(rv).toInclude("Error migrating 004");
+				expect(rv).toInclude("synthetic failure");
+			});
+
+			it("does not record a tracking row when the migration throws", () => {
+				if (_isCockroachDB) return;
+				migrator.migrateIndividual("004");
+				var versions = queryExecute(
+					"SELECT version FROM #application.wheels.migratorTableName# WHERE version = '004'",
+					{},
+					{ datasource = application.wheels.dataSourceName }
+				);
+				expect(versions.recordCount).toBe(
+					0,
+					"The migration's up() threw, so the catch should have rolled back "
+					& "and returned early. No tracking row should exist for version 004."
+				);
+			});
+
+		});
+
+	}
+
+}