From 83cfe2b1a772b0e9bbcb73aa8df6956736014855 Mon Sep 17 00:00:00 2001 From: Peter Amiri Date: Sun, 24 May 2026 08:55:19 -0700 Subject: [PATCH 1/4] fix(model): bypass nested cftransaction when migrator owns the outer (#2789) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Model.create() / .update() / .deleteAll() inside a migration's up() or down() silently rolled back the row on MSSQL. The Migrator wraps every up() / down() invocation in its own outer cftransaction, and Wheels Model's default transaction="commit" opened a nested cftransaction whose JDBC nested-transaction semantics differ per adapter — most acutely on MSSQL where the inner commit didn't release the row and the eventual outer commit dropped it. Migrator.cfc now sets request.$wheelsTransactionWrapper = true around each up() / down() / migrateIndividual() invocation. Model::invokeWith- Transaction() reads that signal and treats the call as "alreadyopen", skipping the nested cftransaction entirely. The flag is request-scope and explicitly cleared in both the success and error paths of every migrator code path so it never leaks past the migration's outer transaction. Engine-agnostic — works the same way on MSSQL, MySQL, PostgreSQL, SQLite, Oracle, and H2. Specs: - vendor/wheels/tests/specs/model/OuterTransactionSignalSpec.cfc exercises invokeWithTransaction's response to the flag for create / save / deleteAll, plus a control test proving rollback still works when the flag is unset. - vendor/wheels/tests/specs/migrator/MigratorOuterTransactionSpec.cfc runs an end-to-end migration whose up() calls model().create() and asserts the row persists, plus assertions that the flag is set during up() and cleared after both up() and down(). Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: Peter Amiri --- CHANGELOG.md | 1 + vendor/wheels/Migrator.cfc | 36 ++++++ vendor/wheels/model/transactions.cfc | 17 ++- .../001_seed_tag_via_model_create.cfc | 26 +++++ .../migrator/MigratorOuterTransactionSpec.cfc | 83 ++++++++++++++ .../model/OuterTransactionSignalSpec.cfc | 103 ++++++++++++++++++ 6 files changed, 265 insertions(+), 1 deletion(-) create mode 100644 vendor/wheels/tests/_assets/migrator/migrations_2789/001_seed_tag_via_model_create.cfc create mode 100644 vendor/wheels/tests/specs/migrator/MigratorOuterTransactionSpec.cfc create mode 100644 vendor/wheels/tests/specs/model/OuterTransactionSignalSpec.cfc diff --git a/CHANGELOG.md b/CHANGELOG.md index cfec6b3ff2..618f640db9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,7 @@ All historical references to "CFWheels" in this changelog have been preserved fo ### Fixed +- `model().create()` / `.update()` / `.deleteAll()` inside a migration's `up()` or `down()` no longer silently roll back. The Migrator wraps every `up()` / `down()` invocation in its own outer `cftransaction`, and Wheels Model's default `transaction="commit"` opened a nested `cftransaction` whose JDBC nested-transaction semantics differ per adapter — most acutely on MSSQL, where the inner commit didn't release the row and the eventual outer commit dropped it, leaving the user with orphan FK rows referencing IDs that never persisted. `Migrator.cfc` now sets `request.$wheelsTransactionWrapper = true` around each `up()` / `down()` / `migrateIndividual()` invocation and `Model::invokeWithTransaction()` reads that signal, treating the call as `alreadyopen` and skipping the nested `cftransaction` entirely. The flag is request-scope and cleared in both the success and error paths of every migrator code path, so it never leaks past the migration's outer transaction (#2789) - `wheels migrate latest` no longer takes a misleading "down" branch and silently no-ops when `wheels_migrator_versions` records a version whose migration file isn't in the current checkout (shared dev DB / peer migration not yet pulled). `Migrator.migrateTo()` now diffs the tracking table against `app/migrator/migrations/` via the new `$getOrphanVersions()` helper and branches on orphan-at-top before the existing direction check — applying pending local migrations with a clear warning when all DB versions above target are orphans, emitting "Nothing to do" naming target vs current when none are pending, and warning + letting the existing down loop continue in mixed cases (orphan rows skip naturally because the loop iterates files). `wheels migrate info` also now shows orphan rows with `[?] ********** NO FILE **********` (Rails-style) and a footer explaining the cause (#2780) - `tools/test-local.sh` silently aborted with `EXIT=1` (no `/tmp/wheels-test-server.log` written, no diagnostic printed) on every install since the `lucli` → `wheels` rebrand window closed — i.e. anyone whose `~/.lucli/express/` directory never existed. Line 81 ran `LUCEE_LIB=$(find ~/.wheels/express ~/.lucli/express -path "*/lib/ext" -type d 2>/dev/null | head -1)` under `set -euo pipefail`; `find` exits non-zero whenever any path argument doesn't exist (stderr suppressed via `2>/dev/null`, but the exit status survives), `pipefail` propagated it through `head -1`, and the assignment tripped `set -e`. The cleanup trap then fired with no server to clean up, leaving the user staring at "Starting Wheels CLI server on port 8080…" with no further output. Dropped the now-dead `~/.lucli/express` fallback (the rename landed in 3.0 and recent CLI releases extract Lucee Express to `~/.wheels/express/` only) and added `|| true` for defense in depth so a missing directory (e.g. a truly fresh install before `wheels start` has ever run) leaves `LUCEE_LIB` empty and the downstream `[ -n "$LUCEE_LIB" ]` guard skips the JDBC pre-install cleanly (#2796) - Routes registered inside `.namespace("foo")` (or equivalent `.scope()` / `.package()`) with a redundant namespace prefix in the controller path — e.g. `to="foo/dashboard##index"` instead of `to="dashboard##index"` — previously silently produced a `foo.foo/dashboard` lookup that downstream flattened to a `Foodashboard`-style class name with an opaque `Wheels.ViewNotFound` error. The Mapper now rejects this at route-registration time with `Wheels.MapperArgumentInvalid`, naming the namespace and the offending value and pointing at the correct shorter form, so users can find the bad route definition instead of chasing the symptom (#2791) diff --git a/vendor/wheels/Migrator.cfc b/vendor/wheels/Migrator.cfc index fe7eb8c4b7..c339f55704 100644 --- a/vendor/wheels/Migrator.cfc +++ b/vendor/wheels/Migrator.cfc @@ -122,14 +122,29 @@ component output="false" extends="wheels.Global"{ if (application[local.appKey].writeMigratorSQLFiles) { $writeMigrationFile(request.$wheelsMigrationSQLFile, ""); } + // Issue #2789: signal to Model.invokeWithTransaction + // that this migration's outer cftransaction owns + // commit/rollback. Without this, model().update() + // / .deleteAll() inside down() open a nested + // cftransaction whose adapter-specific commit + // semantics silently roll the row back on MSSQL + // (and risk similar surprises on other engines). + request.$wheelsTransactionWrapper = true; local.migration.cfc.down(); local.rv = local.rv & request.$wheelsMigrationOutput; $removeVersionAsMigrated(local.migration.version); } catch (any e) { local.rv = local.rv & "Error migrating to #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"); break; } + // Clear the signal once the outer transaction is + // about to commit. Done on the success path before + // the commit so any work performed by code that + // runs AFTER this block (none today, but defensive) + // sees normal model-transaction semantics. + StructDelete(request, "$wheelsTransactionWrapper"); transaction action="commit"; } } @@ -164,14 +179,30 @@ component output="false" extends="wheels.Global"{ if (application[local.appKey].writeMigratorSQLFiles) { $writeMigrationFile(request.$wheelsMigrationSQLFile, ""); } + // Issue #2789: signal to Model.invokeWithTransaction + // that this migration's outer cftransaction owns + // commit/rollback. Without this, model().create() + // / .update() / .deleteAll() inside up() open a + // nested cftransaction whose adapter-specific + // commit semantics silently roll the row back + // on MSSQL (and risk similar surprises on other + // engines). + request.$wheelsTransactionWrapper = true; local.migration.cfc.up(); local.rv = local.rv & request.$wheelsMigrationOutput; $setVersionAsMigrated(local.migration.version, local.migration.name); } catch (any e) { local.rv = local.rv & "Error migrating to #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"); break; } + // Clear the signal once the outer transaction is + // about to commit. Subsequent migrations re-set the + // flag on their next iteration; code that runs + // outside the migrator scope sees normal + // model-transaction semantics. + StructDelete(request, "$wheelsTransactionWrapper"); transaction action="commit"; } } else if (local.migration.version > arguments.version) { @@ -225,13 +256,18 @@ component output="false" extends="wheels.Global"{ if (application[local.appKey].writeMigratorSQLFiles) { $writeMigrationFile(request.$wheelsMigrationSQLFile, ""); } + // Issue #2789: signal to Model.invokeWithTransaction that this + // migration's outer cftransaction owns commit/rollback. + request.$wheelsTransactionWrapper = true; local.migration.cfc.up(); local.rv = local.rv & request.$wheelsMigrationOutput; $setVersionAsMigrated(local.migration.version, local.migration.name); } catch (any e) { 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"); } + StructDelete(request, "$wheelsTransactionWrapper"); transaction action="commit"; } return local.rv; diff --git a/vendor/wheels/model/transactions.cfc b/vendor/wheels/model/transactions.cfc index 67b1ed00b5..ec420fa68e 100644 --- a/vendor/wheels/model/transactions.cfc +++ b/vendor/wheels/model/transactions.cfc @@ -36,8 +36,23 @@ component { request.wheels.transactions[local.connectionArgs] = false; } + // Issue #2789: when an outer-transaction owner signals that it has + // already opened a transaction wrapping this call (the migrator does + // this around every up()/down()), skip the model-level cftransaction + // entirely. Nesting a model `transaction="commit"` inside the + // migrator's transaction triggers JDBC nested-transaction semantics + // that differ per adapter — most acutely on MSSQL, where the nested + // commit doesn't release the row and the eventual outer commit + // silently drops it. The signal is request-scope so it costs nothing + // to check and never persists across requests. + local.outerTransactionActive = ( + StructKeyExists(request, "$wheelsTransactionWrapper") + && IsBoolean(request.$wheelsTransactionWrapper) + && request.$wheelsTransactionWrapper + ); + // If a transaction is already marked as open, change the mode to "alreadyopen", otherwise open one. - if (request.wheels.transactions[local.connectionArgs]) { + if (local.outerTransactionActive || request.wheels.transactions[local.connectionArgs]) { arguments.transaction = "alreadyopen"; local.closeTransaction = false; } else { diff --git a/vendor/wheels/tests/_assets/migrator/migrations_2789/001_seed_tag_via_model_create.cfc b/vendor/wheels/tests/_assets/migrator/migrations_2789/001_seed_tag_via_model_create.cfc new file mode 100644 index 0000000000..fdeff6b62b --- /dev/null +++ b/vendor/wheels/tests/_assets/migrator/migrations_2789/001_seed_tag_via_model_create.cfc @@ -0,0 +1,26 @@ +component extends="wheels.migrator.Migration" hint="issue ##2789 repro — seed a tag via Model.create() inside up()" { + + function up() { + // Issue #2789 repro: calling model().create() inside up() must persist + // the row when the migrator's outer transaction commits. Without the + // fix, Wheels Model's default transaction='commit' opens a nested + // cftransaction whose savepoint/commit semantics collide with the + // migrator's outer transaction (MSSQL silently rolls the row back). + model("Tag").create(name = "issue2789_via_model_create"); + + // Also capture the flag's value during up() so the spec can assert + // the migrator actually set it. We stash on application scope (request + // is per-call) so the spec can read it after migrateTo returns. + application.$issue2789FlagDuringUp = ( + StructKeyExists(request, "$wheelsTransactionWrapper") + && request.$wheelsTransactionWrapper + ); + } + + function down() { + // removeRecord uses raw queryExecute (no nested transaction), safe for + // every adapter regardless of whether the fix is in place. + removeRecord(table = "c_o_r_e_tags", where = "name = 'issue2789_via_model_create'"); + } + +} diff --git a/vendor/wheels/tests/specs/migrator/MigratorOuterTransactionSpec.cfc b/vendor/wheels/tests/specs/migrator/MigratorOuterTransactionSpec.cfc new file mode 100644 index 0000000000..c22476a16e --- /dev/null +++ b/vendor/wheels/tests/specs/migrator/MigratorOuterTransactionSpec.cfc @@ -0,0 +1,83 @@ +component extends="wheels.WheelsTest" { + + include "helperFunctions.cfm"; + + function beforeAll() { + migration = CreateObject("component", "wheels.migrator.Migration").init(); + // Separate migrations directory so this spec's synthetic 001_ file + // doesn't collide with the reconciliation spec's 001/002/003 fixtures. + migrator = CreateObject("component", "wheels.Migrator").init( + migratePath = "/wheels/tests/_assets/migrator/migrations_2789/", + sqlPath = "/wheels/tests/_assets/migrator/sql_2789/" + ); + } + + function run() { + + var _isCockroachDB = CreateObject("component", "wheels.migrator.Migration").init().adapter.adapterName() == "CockroachDB"; + + describe("Migrator sets $wheelsTransactionWrapper while up()/down() runs (issue ##2789)", () => { + + beforeEach(() => { + deleteMigratorVersions(2); + $cleanSqlDirectory(); + try { + queryExecute( + "DELETE FROM c_o_r_e_tags WHERE name = 'issue2789_via_model_create'", + {}, + { datasource = application.wheels.dataSourceName } + ); + } catch (any e) {} + StructDelete(application, "$issue2789FlagDuringUp"); + StructDelete(request, "$wheelsTransactionWrapper"); + }); + + afterEach(() => { + deleteMigratorVersions(2); + $cleanSqlDirectory(); + try { + queryExecute( + "DELETE FROM c_o_r_e_tags WHERE name = 'issue2789_via_model_create'", + {}, + { datasource = application.wheels.dataSourceName } + ); + } catch (any e) {} + StructDelete(application, "$issue2789FlagDuringUp"); + StructDelete(request, "$wheelsTransactionWrapper"); + }); + + it("model.create() inside up() persists after the outer transaction commits", () => { + if (_isCockroachDB) return; + migrator.migrateTo("001"); + var found = queryExecute( + "SELECT id FROM c_o_r_e_tags WHERE name = 'issue2789_via_model_create'", + {}, + { datasource = application.wheels.dataSourceName } + ); + expect(found.recordCount).toBe(1); + }); + + it("sets request.$wheelsTransactionWrapper for the duration of up()", () => { + if (_isCockroachDB) return; + migrator.migrateTo("001"); + expect(application.$issue2789FlagDuringUp).toBeTrue(); + }); + + it("clears request.$wheelsTransactionWrapper after up() returns", () => { + if (_isCockroachDB) return; + migrator.migrateTo("001"); + expect(StructKeyExists(request, "$wheelsTransactionWrapper")).toBeFalse(); + }); + + it("clears request.$wheelsTransactionWrapper after down() returns", () => { + if (_isCockroachDB) return; + migrator.migrateTo("001"); + migrator.migrateTo("0"); + expect(StructKeyExists(request, "$wheelsTransactionWrapper")).toBeFalse(); + }); + + }); + + } + +} diff --git a/vendor/wheels/tests/specs/model/OuterTransactionSignalSpec.cfc b/vendor/wheels/tests/specs/model/OuterTransactionSignalSpec.cfc new file mode 100644 index 0000000000..409d95b4f8 --- /dev/null +++ b/vendor/wheels/tests/specs/model/OuterTransactionSignalSpec.cfc @@ -0,0 +1,103 @@ +component extends="wheels.WheelsTest" { + + function run() { + + g = application.wo; + + describe("model invokeWithTransaction respects outer-transaction signal (issue ##2789)", () => { + + beforeEach(() => { + // Defensive cleanup in case a prior failing run left the flag set. + if (StructKeyExists(request, "$wheelsTransactionWrapper")) { + StructDelete(request, "$wheelsTransactionWrapper"); + } + queryExecute( + "DELETE FROM c_o_r_e_tags WHERE name IN ('outerSig_control', 'outerSig_signal', 'outerSig_signal_create', 'outerSig_signal_update', 'outerSig_signal_delete')", + {}, + { datasource = application.wheels.dataSourceName } + ); + }); + + afterEach(() => { + if (StructKeyExists(request, "$wheelsTransactionWrapper")) { + StructDelete(request, "$wheelsTransactionWrapper"); + } + queryExecute( + "DELETE FROM c_o_r_e_tags WHERE name IN ('outerSig_control', 'outerSig_signal', 'outerSig_signal_create', 'outerSig_signal_update', 'outerSig_signal_delete')", + {}, + { datasource = application.wheels.dataSourceName } + ); + }); + + it("control: model.create(transaction='rollback') without the flag rolls the row back", () => { + // Establishes the baseline that the rollback path actually works + // in this environment, so the inverse assertion below is meaningful. + var beforeCount = g.model("tag").count(); + g.model("tag").create(name = "outerSig_control", transaction = "rollback"); + var afterCount = g.model("tag").count(); + expect(afterCount).toBe(beforeCount); + }); + + it("treats invokeWithTransaction as 'alreadyopen' when request.$wheelsTransactionWrapper is set", () => { + // With the outer-transaction signal set, the model must NOT open + // its own cftransaction — even when the caller explicitly passes + // transaction='rollback'. The save is treated as alreadyopen, so + // no cftransaction is begun and no rollback is issued; the INSERT + // runs in the ambient (auto-commit) connection state and persists. + // This is the surrogate observable for the original MSSQL bug + // (#2789): inside a migrator's outer cftransaction, the model's + // nested cftransaction silently rolled the row back. Bypassing the + // model's wrapper eliminates the nested-transaction entirely. + var beforeCount = g.model("tag").count(); + request.$wheelsTransactionWrapper = true; + try { + g.model("tag").create(name = "outerSig_signal", transaction = "rollback"); + } finally { + StructDelete(request, "$wheelsTransactionWrapper"); + } + var afterCount = g.model("tag").count(); + expect(afterCount).toBe(beforeCount + 1); + }); + + it("also bypasses the wrapper for update via save()", () => { + // Same semantics for the update path: save() with + // transaction='rollback' would normally roll the UPDATE back, but + // with the signal set, the wrapper is bypassed. + var tag = g.model("tag").create(name = "outerSig_signal_update"); + request.$wheelsTransactionWrapper = true; + try { + tag.name = "outerSig_signal_update_modified"; + tag.save(transaction = "rollback"); + } finally { + StructDelete(request, "$wheelsTransactionWrapper"); + } + var refetched = g.model("tag").findByKey(tag.id); + expect(refetched.name).toBe("outerSig_signal_update_modified"); + // Cleanup explicit because the name no longer matches the afterEach pattern. + queryExecute( + "DELETE FROM c_o_r_e_tags WHERE id = :id", + { id = { value = tag.id, cfsqltype = "cf_sql_integer" } }, + { datasource = application.wheels.dataSourceName } + ); + }); + + it("also bypasses the wrapper for deleteAll", () => { + // Same semantics for deleteAll: transaction='rollback' would + // normally roll the DELETE back, but with the signal set the + // wrapper is bypassed, so the DELETE persists. + g.model("tag").create(name = "outerSig_signal_delete"); + request.$wheelsTransactionWrapper = true; + try { + g.model("tag").deleteAll(where = "name = 'outerSig_signal_delete'", transaction = "rollback"); + } finally { + StructDelete(request, "$wheelsTransactionWrapper"); + } + var remaining = g.model("tag").count(where = "name = 'outerSig_signal_delete'"); + expect(remaining).toBe(0); + }); + + }); + + } + +} From 93db8bdebc047f44a98d7608829fe6d404d839e2 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Sun, 24 May 2026 16:09:44 +0000 Subject: [PATCH 2/4] fix(model): address Reviewer A/B consensus findings (round 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - vendor/wheels/tests/specs/migrator/MigratorOuterTransactionSpec.cfc: Replace `var _isCockroachDB` (local scope, invisible to it() closures on Adobe CF — CLAUDE.md anti-pattern #10) with `var ctx = {isCockroachDB: ...}` and update the four closure guards to `if (ctx.isCockroachDB) return;`. - vendor/wheels/tests/_assets/migrator/migrations_2789/001_seed_tag_via_model_create.cfc: Switch `application.$issue2789FlagDuringUp` to `request.$issue2789FlagDuringUp`. The migration runs in the same HTTP request as the spec's migrateTo call, so request scope is sufficient and avoids global mutable state. Spec's beforeEach/afterEach + assertion updated accordingly. - vendor/wheels/Migrator.cfc, vendor/wheels/model/transactions.cfc: Trim the three multi-line comment blocks (up-path, down-path, migrateIndividual) and the 9-line block in invokeWithTransaction to one-liners per CLAUDE.md "one short line max" convention. Issue number preserved. Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> --- vendor/wheels/Migrator.cfc | 30 ++----------------- vendor/wheels/model/transactions.cfc | 10 +------ .../001_seed_tag_via_model_create.cfc | 12 ++------ .../migrator/MigratorOuterTransactionSpec.cfc | 16 +++++----- 4 files changed, 15 insertions(+), 53 deletions(-) diff --git a/vendor/wheels/Migrator.cfc b/vendor/wheels/Migrator.cfc index c339f55704..e69e936fd6 100644 --- a/vendor/wheels/Migrator.cfc +++ b/vendor/wheels/Migrator.cfc @@ -122,13 +122,7 @@ component output="false" extends="wheels.Global"{ if (application[local.appKey].writeMigratorSQLFiles) { $writeMigrationFile(request.$wheelsMigrationSQLFile, ""); } - // Issue #2789: signal to Model.invokeWithTransaction - // that this migration's outer cftransaction owns - // commit/rollback. Without this, model().update() - // / .deleteAll() inside down() open a nested - // cftransaction whose adapter-specific commit - // semantics silently roll the row back on MSSQL - // (and risk similar surprises on other engines). + // Issue #2789: skip nested cftransaction when migrator's outer one owns commit/rollback. request.$wheelsTransactionWrapper = true; local.migration.cfc.down(); local.rv = local.rv & request.$wheelsMigrationOutput; @@ -139,11 +133,6 @@ component output="false" extends="wheels.Global"{ StructDelete(request, "$wheelsTransactionWrapper"); break; } - // Clear the signal once the outer transaction is - // about to commit. Done on the success path before - // the commit so any work performed by code that - // runs AFTER this block (none today, but defensive) - // sees normal model-transaction semantics. StructDelete(request, "$wheelsTransactionWrapper"); transaction action="commit"; } @@ -179,14 +168,7 @@ component output="false" extends="wheels.Global"{ if (application[local.appKey].writeMigratorSQLFiles) { $writeMigrationFile(request.$wheelsMigrationSQLFile, ""); } - // Issue #2789: signal to Model.invokeWithTransaction - // that this migration's outer cftransaction owns - // commit/rollback. Without this, model().create() - // / .update() / .deleteAll() inside up() open a - // nested cftransaction whose adapter-specific - // commit semantics silently roll the row back - // on MSSQL (and risk similar surprises on other - // engines). + // Issue #2789: skip nested cftransaction when migrator's outer one owns commit/rollback. request.$wheelsTransactionWrapper = true; local.migration.cfc.up(); local.rv = local.rv & request.$wheelsMigrationOutput; @@ -197,11 +179,6 @@ component output="false" extends="wheels.Global"{ StructDelete(request, "$wheelsTransactionWrapper"); break; } - // Clear the signal once the outer transaction is - // about to commit. Subsequent migrations re-set the - // flag on their next iteration; code that runs - // outside the migrator scope sees normal - // model-transaction semantics. StructDelete(request, "$wheelsTransactionWrapper"); transaction action="commit"; } @@ -256,8 +233,7 @@ component output="false" extends="wheels.Global"{ if (application[local.appKey].writeMigratorSQLFiles) { $writeMigrationFile(request.$wheelsMigrationSQLFile, ""); } - // Issue #2789: signal to Model.invokeWithTransaction that this - // migration's outer cftransaction owns commit/rollback. + // Issue #2789: skip nested cftransaction when migrator's outer one owns commit/rollback. request.$wheelsTransactionWrapper = true; local.migration.cfc.up(); local.rv = local.rv & request.$wheelsMigrationOutput; diff --git a/vendor/wheels/model/transactions.cfc b/vendor/wheels/model/transactions.cfc index ec420fa68e..84add8f5ad 100644 --- a/vendor/wheels/model/transactions.cfc +++ b/vendor/wheels/model/transactions.cfc @@ -36,15 +36,7 @@ component { request.wheels.transactions[local.connectionArgs] = false; } - // Issue #2789: when an outer-transaction owner signals that it has - // already opened a transaction wrapping this call (the migrator does - // this around every up()/down()), skip the model-level cftransaction - // entirely. Nesting a model `transaction="commit"` inside the - // migrator's transaction triggers JDBC nested-transaction semantics - // that differ per adapter — most acutely on MSSQL, where the nested - // commit doesn't release the row and the eventual outer commit - // silently drops it. The signal is request-scope so it costs nothing - // to check and never persists across requests. + // Issue #2789: skip model-level cftransaction when an outer owner (e.g. migrator) wraps this call. local.outerTransactionActive = ( StructKeyExists(request, "$wheelsTransactionWrapper") && IsBoolean(request.$wheelsTransactionWrapper) diff --git a/vendor/wheels/tests/_assets/migrator/migrations_2789/001_seed_tag_via_model_create.cfc b/vendor/wheels/tests/_assets/migrator/migrations_2789/001_seed_tag_via_model_create.cfc index fdeff6b62b..1e8dafb4f0 100644 --- a/vendor/wheels/tests/_assets/migrator/migrations_2789/001_seed_tag_via_model_create.cfc +++ b/vendor/wheels/tests/_assets/migrator/migrations_2789/001_seed_tag_via_model_create.cfc @@ -1,17 +1,11 @@ component extends="wheels.migrator.Migration" hint="issue ##2789 repro — seed a tag via Model.create() inside up()" { function up() { - // Issue #2789 repro: calling model().create() inside up() must persist - // the row when the migrator's outer transaction commits. Without the - // fix, Wheels Model's default transaction='commit' opens a nested - // cftransaction whose savepoint/commit semantics collide with the - // migrator's outer transaction (MSSQL silently rolls the row back). + // Issue #2789 repro: model().create() inside up() must persist after the outer transaction commits. model("Tag").create(name = "issue2789_via_model_create"); - // Also capture the flag's value during up() so the spec can assert - // the migrator actually set it. We stash on application scope (request - // is per-call) so the spec can read it after migrateTo returns. - application.$issue2789FlagDuringUp = ( + // Capture the flag's value during up() so the spec can assert the migrator set it. + request.$issue2789FlagDuringUp = ( StructKeyExists(request, "$wheelsTransactionWrapper") && request.$wheelsTransactionWrapper ); diff --git a/vendor/wheels/tests/specs/migrator/MigratorOuterTransactionSpec.cfc b/vendor/wheels/tests/specs/migrator/MigratorOuterTransactionSpec.cfc index c22476a16e..37a7324c57 100644 --- a/vendor/wheels/tests/specs/migrator/MigratorOuterTransactionSpec.cfc +++ b/vendor/wheels/tests/specs/migrator/MigratorOuterTransactionSpec.cfc @@ -14,7 +14,7 @@ component extends="wheels.WheelsTest" { function run() { - var _isCockroachDB = CreateObject("component", "wheels.migrator.Migration").init().adapter.adapterName() == "CockroachDB"; + var ctx = {isCockroachDB: CreateObject("component", "wheels.migrator.Migration").init().adapter.adapterName() == "CockroachDB"}; describe("Migrator sets $wheelsTransactionWrapper while up()/down() runs (issue ##2789)", () => { @@ -28,7 +28,7 @@ component extends="wheels.WheelsTest" { { datasource = application.wheels.dataSourceName } ); } catch (any e) {} - StructDelete(application, "$issue2789FlagDuringUp"); + StructDelete(request, "$issue2789FlagDuringUp"); StructDelete(request, "$wheelsTransactionWrapper"); }); @@ -42,12 +42,12 @@ component extends="wheels.WheelsTest" { { datasource = application.wheels.dataSourceName } ); } catch (any e) {} - StructDelete(application, "$issue2789FlagDuringUp"); + StructDelete(request, "$issue2789FlagDuringUp"); StructDelete(request, "$wheelsTransactionWrapper"); }); it("model.create() inside up() persists after the outer transaction commits", () => { - if (_isCockroachDB) return; + if (ctx.isCockroachDB) return; migrator.migrateTo("001"); var found = queryExecute( "SELECT id FROM c_o_r_e_tags WHERE name = 'issue2789_via_model_create'", @@ -58,19 +58,19 @@ component extends="wheels.WheelsTest" { }); it("sets request.$wheelsTransactionWrapper for the duration of up()", () => { - if (_isCockroachDB) return; + if (ctx.isCockroachDB) return; migrator.migrateTo("001"); - expect(application.$issue2789FlagDuringUp).toBeTrue(); + expect(request.$issue2789FlagDuringUp).toBeTrue(); }); it("clears request.$wheelsTransactionWrapper after up() returns", () => { - if (_isCockroachDB) return; + if (ctx.isCockroachDB) return; migrator.migrateTo("001"); expect(StructKeyExists(request, "$wheelsTransactionWrapper")).toBeFalse(); }); it("clears request.$wheelsTransactionWrapper after down() returns", () => { - if (_isCockroachDB) return; + if (ctx.isCockroachDB) return; migrator.migrateTo("001"); migrator.migrateTo("0"); expect(StructKeyExists(request, "$wheelsTransactionWrapper")).toBeFalse(); From 5c6d81bd25629c410e9f6c9251e45e722371c1ed Mon Sep 17 00:00:00 2001 From: Peter Amiri Date: Tue, 26 May 2026 05:05:52 -0700 Subject: [PATCH 3/4] fix(test): address Reviewer A/B consensus findings (round 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Trim the multi-line comment blocks Reviewer B's round-1 review caught in the test spec and migration fixture. CLAUDE.md: 'Never write multi-line comment blocks — one short line max.' Reviewer A flagged the identical pattern in production code during round 1 and the address-review trimmed Migrator.cfc / transactions.cfc, but the same violation persisted in the new test files. Reviewer B flagged that inconsistency in round 1; this addresses it. - vendor/wheels/tests/specs/model/OuterTransactionSignalSpec.cfc: trimmed the 9-line block in 'treats invokeWithTransaction as alreadyopen', the 2-line block in 'control:', the 3-line block in the save() case, and the 3-line block in the deleteAll case to one-liners each, anchored on issue #2789. - vendor/wheels/tests/_assets/migrator/migrations_2789/001_seed_tag_via_model_create.cfc: trimmed the 2-line block in down() to one line. Skipped (per consensus scope): - migrateIndividual() commit-after-rollback — Reviewer A and B both labelled pre-existing; recommended a follow-up issue. - variables.migration nit in MigratorOuterTransactionSpec.cfc — both reviewers labelled this a nit, not a blocker. Local: bash tools/test-local.sh model → 843 passed; migrator → 238 passed. Pure comment-trim with zero behavior delta. Signed-off-by: Peter Amiri --- .../001_seed_tag_via_model_create.cfc | 3 +-- .../model/OuterTransactionSignalSpec.cfc | 23 ++++--------------- 2 files changed, 6 insertions(+), 20 deletions(-) diff --git a/vendor/wheels/tests/_assets/migrator/migrations_2789/001_seed_tag_via_model_create.cfc b/vendor/wheels/tests/_assets/migrator/migrations_2789/001_seed_tag_via_model_create.cfc index 1e8dafb4f0..97934a1cc8 100644 --- a/vendor/wheels/tests/_assets/migrator/migrations_2789/001_seed_tag_via_model_create.cfc +++ b/vendor/wheels/tests/_assets/migrator/migrations_2789/001_seed_tag_via_model_create.cfc @@ -12,8 +12,7 @@ component extends="wheels.migrator.Migration" hint="issue ##2789 repro — seed } function down() { - // removeRecord uses raw queryExecute (no nested transaction), safe for - // every adapter regardless of whether the fix is in place. + // Issue #2789: raw queryExecute avoids nested transaction; safe across all adapters. removeRecord(table = "c_o_r_e_tags", where = "name = 'issue2789_via_model_create'"); } diff --git a/vendor/wheels/tests/specs/model/OuterTransactionSignalSpec.cfc b/vendor/wheels/tests/specs/model/OuterTransactionSignalSpec.cfc index 409d95b4f8..9577ab0dca 100644 --- a/vendor/wheels/tests/specs/model/OuterTransactionSignalSpec.cfc +++ b/vendor/wheels/tests/specs/model/OuterTransactionSignalSpec.cfc @@ -30,8 +30,7 @@ component extends="wheels.WheelsTest" { }); it("control: model.create(transaction='rollback') without the flag rolls the row back", () => { - // Establishes the baseline that the rollback path actually works - // in this environment, so the inverse assertion below is meaningful. + // Baseline: proves the rollback path actually fires in this environment. var beforeCount = g.model("tag").count(); g.model("tag").create(name = "outerSig_control", transaction = "rollback"); var afterCount = g.model("tag").count(); @@ -39,15 +38,7 @@ component extends="wheels.WheelsTest" { }); it("treats invokeWithTransaction as 'alreadyopen' when request.$wheelsTransactionWrapper is set", () => { - // With the outer-transaction signal set, the model must NOT open - // its own cftransaction — even when the caller explicitly passes - // transaction='rollback'. The save is treated as alreadyopen, so - // no cftransaction is begun and no rollback is issued; the INSERT - // runs in the ambient (auto-commit) connection state and persists. - // This is the surrogate observable for the original MSSQL bug - // (#2789): inside a migrator's outer cftransaction, the model's - // nested cftransaction silently rolled the row back. Bypassing the - // model's wrapper eliminates the nested-transaction entirely. + // Issue #2789: signal set → model bypasses its own wrapper; INSERT persists despite transaction='rollback'. var beforeCount = g.model("tag").count(); request.$wheelsTransactionWrapper = true; try { @@ -60,9 +51,7 @@ component extends="wheels.WheelsTest" { }); it("also bypasses the wrapper for update via save()", () => { - // Same semantics for the update path: save() with - // transaction='rollback' would normally roll the UPDATE back, but - // with the signal set, the wrapper is bypassed. + // Issue #2789: same bypass applies to save()'s UPDATE path. var tag = g.model("tag").create(name = "outerSig_signal_update"); request.$wheelsTransactionWrapper = true; try { @@ -73,7 +62,7 @@ component extends="wheels.WheelsTest" { } var refetched = g.model("tag").findByKey(tag.id); expect(refetched.name).toBe("outerSig_signal_update_modified"); - // Cleanup explicit because the name no longer matches the afterEach pattern. + // Cleanup: row name no longer matches the afterEach pattern. queryExecute( "DELETE FROM c_o_r_e_tags WHERE id = :id", { id = { value = tag.id, cfsqltype = "cf_sql_integer" } }, @@ -82,9 +71,7 @@ component extends="wheels.WheelsTest" { }); it("also bypasses the wrapper for deleteAll", () => { - // Same semantics for deleteAll: transaction='rollback' would - // normally roll the DELETE back, but with the signal set the - // wrapper is bypassed, so the DELETE persists. + // Issue #2789: same bypass applies to deleteAll's DELETE path. g.model("tag").create(name = "outerSig_signal_delete"); request.$wheelsTransactionWrapper = true; try { From a5c2c7f9946c06b4459523d94c653b10ab6fbd8c Mon Sep 17 00:00:00 2001 From: Peter Amiri Date: Tue, 26 May 2026 05:22:03 -0700 Subject: [PATCH 4/4] docs(test): explain why variables.migration is initialized in beforeAll MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewer A/B agreed nit from round-1 review: variables.migration was bare-assigned in beforeAll() without explanation. The variable isn't referenced by any of this spec's describe/it blocks directly; it exists because helperFunctions.cfm engine-detection helpers (isDbCompatible, isDbCompatibleFor_SQLServer, etc.) call migration.adapter.adapterName() under the hood. Spare the next reader from wondering why the assignment is there. Also tightens the adjacent migrator-init comment to one line per CLAUDE.md's 'one short line max' rule. Follow-up tracked separately for the pre-existing migrateIndividual() commit-after-rollback both reviewers flagged out-of-scope: #2811. Local: bash tools/test-local.sh migrator → 238 passed. Signed-off-by: Peter Amiri --- .../tests/specs/migrator/MigratorOuterTransactionSpec.cfc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/vendor/wheels/tests/specs/migrator/MigratorOuterTransactionSpec.cfc b/vendor/wheels/tests/specs/migrator/MigratorOuterTransactionSpec.cfc index 37a7324c57..44fa170ce4 100644 --- a/vendor/wheels/tests/specs/migrator/MigratorOuterTransactionSpec.cfc +++ b/vendor/wheels/tests/specs/migrator/MigratorOuterTransactionSpec.cfc @@ -3,9 +3,9 @@ component extends="wheels.WheelsTest" { include "helperFunctions.cfm"; function beforeAll() { + // Required by helperFunctions.cfm engine-detection helpers (e.g. isDbCompatible's adapter probe). migration = CreateObject("component", "wheels.migrator.Migration").init(); - // Separate migrations directory so this spec's synthetic 001_ file - // doesn't collide with the reconciliation spec's 001/002/003 fixtures. + // Separate migrations dir avoids collision with reconciliation spec's 001/002/003 fixtures. migrator = CreateObject("component", "wheels.Migrator").init( migratePath = "/wheels/tests/_assets/migrator/migrations_2789/", sqlPath = "/wheels/tests/_assets/migrator/sql_2789/"