From ee0ede66a3f8af752dc153985d89a5fafb5cb549 Mon Sep 17 00:00:00 2001 From: Peter Amiri Date: Mon, 5 Oct 2026 01:01:48 -0700 Subject: [PATCH 1/4] test(model): pin advisory-lock release when the callback ends the request (4219) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A withAdvisoryLock() callback that ends the request from inside the lock — via a real abort or a real redirectTo() (cflocation) — must still release the database lock. The release runs in a finally, which executes on those exit paths (only a catch is skipped by abort), so there is nothing to leak; this makes that a permanent, measured guarantee instead of a caveat. Adds a core regression spec that drives request-ending actions over HTTP via $testClient and then, from its own separate connection, asserts the named lock is free — for the default (session-scoped) and transaction = true paths, each ending via abort and via redirectTo. A release regressed out of the finally would leave the lock on the aborting request's pooled connection and red this spec. Skips on databases without advisory-lock support; afterEach best-effort clears any probe lock so a failure cannot hand a held lock to a later spec. Fixtures: vendor/wheels/tests/_assets/controllers/AdvisoryLockProbe.cfc + the /_advisorylock routes. Verified locally on Lucee 7 with MySQL and SQL Server (4/0/0 each); the compat matrix proves every engine x MySQL/PostgreSQL/SQL Server. Closes #4219 Co-Authored-By: Claude Opus 4.8 Signed-off-by: Peter Amiri --- .../_assets/controllers/AdvisoryLockProbe.cfc | 41 +++++++ vendor/wheels/tests/routes.cfm | 10 ++ .../model/advisoryLockAbortReleaseSpec.cfc | 105 ++++++++++++++++++ 3 files changed, 156 insertions(+) create mode 100644 vendor/wheels/tests/_assets/controllers/AdvisoryLockProbe.cfc create mode 100644 vendor/wheels/tests/specs/model/advisoryLockAbortReleaseSpec.cfc diff --git a/vendor/wheels/tests/_assets/controllers/AdvisoryLockProbe.cfc b/vendor/wheels/tests/_assets/controllers/AdvisoryLockProbe.cfc new file mode 100644 index 0000000000..cc1f82af1f --- /dev/null +++ b/vendor/wheels/tests/_assets/controllers/AdvisoryLockProbe.cfc @@ -0,0 +1,41 @@ +/** + * Test-asset controller for the #4219 release-on-abort regression spec + * (advisoryLockAbortReleaseSpec). Each action takes a withAdvisoryLock() lock + * and then ends the request from INSIDE the callback — with a real `abort` or a + * real `redirectTo()` (which ends in a cflocation) — for both the default + * (session-scoped) and the transaction = true path. + * + * The lock's release runs in a `finally`, which executes on abort / cflocation + * (measured on Lucee, Adobe and BoxLang), so the lock must be free once the + * request ends. The spec drives these actions over HTTP via $testClient and + * then, from its own (separate) connection, asserts the named lock is no longer + * held anywhere — a release skipped on abort would leave it held and red the spec. + * + * Lock names are fixed per action so the spec can check and clean up each one. + */ +component extends="Controller" { + + function abortDefault() { + model("author").withAdvisoryLock(name = "probe_abort_default", callback = function() { + abort; + }); + } + + function redirectDefault() { + model("author").withAdvisoryLock(name = "probe_redirect_default", callback = function() { + redirectTo(url = "/"); + }); + } + + function abortTransaction() { + model("author").withAdvisoryLock(name = "probe_abort_tx", transaction = true, callback = function() { + abort; + }); + } + + function redirectTransaction() { + model("author").withAdvisoryLock(name = "probe_redirect_tx", transaction = true, callback = function() { + redirectTo(url = "/"); + }); + } +} diff --git a/vendor/wheels/tests/routes.cfm b/vendor/wheels/tests/routes.cfm index 41f88cbb87..9d06f7d3ed 100644 --- a/vendor/wheels/tests/routes.cfm +++ b/vendor/wheels/tests/routes.cfm @@ -17,6 +17,16 @@ mapper() .post(name="browserTestLogout", pattern="/logout", to="BrowserTestSessions##destroy") .get(name="browserTestLoginAs", pattern="/login-as", to="BrowserTestLogin##create") .end() + // Advisory-lock release-on-abort fixtures (#4219) — a lock taken by + // withAdvisoryLock() whose callback ends the request via abort/redirectTo + // must still be released (the release runs in a finally). Driven by + // wheels.tests.specs.model.advisoryLockAbortReleaseSpec. Must precede .wildcard(). + .scope(path="/_advisorylock") + .get(name="advisoryLockAbortDefault", pattern="/abort-default", to="AdvisoryLockProbe##abortDefault") + .get(name="advisoryLockRedirectDefault", pattern="/redirect-default", to="AdvisoryLockProbe##redirectDefault") + .get(name="advisoryLockAbortTx", pattern="/abort-tx", to="AdvisoryLockProbe##abortTransaction") + .get(name="advisoryLockRedirectTx", pattern="/redirect-tx", to="AdvisoryLockProbe##redirectTransaction") + .end() .wildcard() .get(name="wheelstestbox", pattern="wheels/core/tests", to="wheels##public##tests") .get(name="sampleLinkToTest", pattern="sample/linktotest", to="sample##linktotest") diff --git a/vendor/wheels/tests/specs/model/advisoryLockAbortReleaseSpec.cfc b/vendor/wheels/tests/specs/model/advisoryLockAbortReleaseSpec.cfc new file mode 100644 index 0000000000..36dc6cbb68 --- /dev/null +++ b/vendor/wheels/tests/specs/model/advisoryLockAbortReleaseSpec.cfc @@ -0,0 +1,105 @@ +/** + * #4219: a withAdvisoryLock() callback that ends the request from inside the lock — with a real + * `abort` or a real `redirectTo()` (which ends in a cflocation) — must still release the database + * lock. The release runs in a `finally`, which executes on those exit paths (measured on Lucee, + * Adobe and BoxLang; only a `catch` is skipped by abort). This spec pins that: it drives + * request-ending actions over HTTP via $testClient and then, from its own separate connection, + * asserts the named lock is no longer held anywhere. A release that regressed out of the `finally` + * would leave the lock on the aborting request's pooled connection and red this spec. + * + * Covered: the default (session-scoped) path and the transaction = true path, each ending via abort + * and via redirectTo. Advisory locks only apply to PostgreSQL, MySQL and SQL Server, so other + * databases skip. The lock-held check uses the adapter's own $isAdvisoryLockHeld (the same + * canonical free-check the release verification uses), on a fresh adapter/connection. + * + * Fixtures: vendor/wheels/tests/_assets/controllers/AdvisoryLockProbe.cfc + the /_advisorylock + * routes in vendor/wheels/tests/routes.cfm. + */ +component extends="wheels.WheelsTest" { + + // A separate adapter instance on the test datasource, so the held-check runs on its own + // connection rather than the model's (mirrors advisoryLockTransactionalSpec.freshAdapter()). + function freshAdapter() { + var classData = variables.g.model("author").$classData(); + var folder = Left(variables.adapterName, Len(variables.adapterName) - 5); + return CreateObject("component", "wheels.databaseAdapters.#folder#.#variables.adapterName#").$init( + dataSource = classData.dataSource, + username = classData.username, + password = classData.password + ); + } + + // Best-effort: if a scenario failed and left a probe lock held, acquiring+releasing it through the + // public API clears it when it is free (a no-op) and times out harmlessly when a leaked session + // still holds it on a pooled connection (only that session could release it). Either way a failure + // cannot silently hand a held lock to a later spec. Each name is cleaned independently. + function releaseProbeLocks() { + var noop = function() { + return ""; + }; + for (var probe in ListToArray(variables.probeNames)) { + try { + variables.g.model("author").withAdvisoryLock(name = probe, timeout = 1, callback = noop); + } catch (any e) { + // lock still held by a leaked session, or the DB does not support it — nothing to do. + } + } + } + + function run() { + // Set at registration time (not beforeAll): the describe callback below reads these while + // the suite is being built, before any lifecycle method runs. + variables.g = application.wo; + variables.adapterName = variables.g.get("adapterName"); + // Databases whose adapter takes real advisory locks (mirrors advisoryLockExclusionSpec / + // advisoryLockTransactionalSpec). Everything else has no lock to leak, so it skips. + variables.applies = ListFindNoCase("PostgreSQLModel,MySQLModel,MicrosoftSQLServerModel", variables.adapterName) > 0; + variables.probeNames = "probe_abort_default,probe_redirect_default,probe_abort_tx,probe_redirect_tx"; + + describe("withAdvisoryLock releases the lock when the callback ends the request (4219)", () => { + + if (!variables.applies) { + it("skips on databases without advisory-lock support", () => { + skip("Advisory locks apply to PostgreSQL, MySQL and SQL Server only (adapter: " & variables.adapterName & ")."); + }); + return; + } + + afterEach(() => { + releaseProbeLocks(); + }); + + it("default path: frees the lock after the callback aborts", () => { + var tc = $testClient(); + tc.get("/_advisorylock/abort-default"); + expect(freshAdapter().$isAdvisoryLockHeld(name = "probe_abort_default")).toBeFalse( + "a default-path lock stayed held after the callback aborted — the release did not run on abort" + ); + }); + + it("default path: frees the lock after the callback redirects", () => { + var tc = $testClient(); + tc.get("/_advisorylock/redirect-default"); + expect(freshAdapter().$isAdvisoryLockHeld(name = "probe_redirect_default")).toBeFalse( + "a default-path lock stayed held after the callback redirected — the release did not run on cflocation" + ); + }); + + it("transaction = true: frees the lock after the callback aborts", () => { + var tc = $testClient(); + tc.get("/_advisorylock/abort-tx"); + expect(freshAdapter().$isAdvisoryLockHeld(name = "probe_abort_tx")).toBeFalse( + "a transaction-path lock stayed held after the callback aborted — the release did not run on abort" + ); + }); + + it("transaction = true: frees the lock after the callback redirects", () => { + var tc = $testClient(); + tc.get("/_advisorylock/redirect-tx"); + expect(freshAdapter().$isAdvisoryLockHeld(name = "probe_redirect_tx")).toBeFalse( + "a transaction-path lock stayed held after the callback redirected — the release did not run on cflocation" + ); + }); + }); + } +} From 9c4ddf10d0a1ff02efa0e9f50f0541e39efd1164 Mon Sep 17 00:00:00 2001 From: Peter Amiri Date: Mon, 5 Oct 2026 01:15:31 -0700 Subject: [PATCH 2/4] test(model): add a positive control to the advisory-lock release spec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review: the spec's 'not held afterwards' assertions passed vacuously if the probe request never reached withAdvisoryLock (a 404, the wrong app or datasource, an early throw) — tc.get()'s result was ignored. Each probe callback now records, from a SECOND connection while inside the lock and before ending the request, the result of a server-wide $isAdvisoryLockHeld into server["wheelsAdvisoryProbe_"]. The spec asserts that marker is present and true (the lock was genuinely held at the moment of the abort / redirect), asserts the response (200 for abort, a 3xx redirect for redirectTo), and only then asserts the lock is free. If the request never took the lock the marker is absent and the test fails instead of passing silently. Verified it can fail: moving the release out of the withAdvisoryLock finally (into a catch, release after the block) reds the abort scenarios on MySQL and SQL Server (3 pass / 1 fail: 'a default-path lock stayed held after the callback aborted'), confirming the spec guards the finally. redirectTo stays green under that break because Wheels defers the cflocation to after the action, so the callback returns normally and the release runs — redirectTo is inherently safe; abort is the path the finally protects. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Peter Amiri --- .../_assets/controllers/AdvisoryLockProbe.cfc | 60 ++++++++++++------- .../model/advisoryLockAbortReleaseSpec.cfc | 36 +++++++++++ 2 files changed, 75 insertions(+), 21 deletions(-) diff --git a/vendor/wheels/tests/_assets/controllers/AdvisoryLockProbe.cfc b/vendor/wheels/tests/_assets/controllers/AdvisoryLockProbe.cfc index cc1f82af1f..6b92be2acb 100644 --- a/vendor/wheels/tests/_assets/controllers/AdvisoryLockProbe.cfc +++ b/vendor/wheels/tests/_assets/controllers/AdvisoryLockProbe.cfc @@ -1,41 +1,59 @@ /** * Test-asset controller for the #4219 release-on-abort regression spec - * (advisoryLockAbortReleaseSpec). Each action takes a withAdvisoryLock() lock - * and then ends the request from INSIDE the callback — with a real `abort` or a - * real `redirectTo()` (which ends in a cflocation) — for both the default - * (session-scoped) and the transaction = true path. + * (advisoryLockAbortReleaseSpec). Each action takes a withAdvisoryLock() lock and then ends the + * request from INSIDE the callback — with a real `abort` or a real `redirectTo()` (which ends in a + * cflocation) — for both the default (session-scoped) and the transaction = true path. * - * The lock's release runs in a `finally`, which executes on abort / cflocation - * (measured on Lucee, Adobe and BoxLang), so the lock must be free once the - * request ends. The spec drives these actions over HTTP via $testClient and - * then, from its own (separate) connection, asserts the named lock is no longer - * held anywhere — a release skipped on abort would leave it held and red the spec. + * Positive control: while inside the lock, and before ending the request, each callback records a + * held-check taken from a SECOND connection into server["wheelsAdvisoryProbe_"]. The spec + * reads that marker and asserts it is true, so a scenario cannot pass vacuously if the request never + * reached withAdvisoryLock (a 404, a load failure, the wrong datasource, an early throw) — in those + * cases the marker is simply absent. $isAdvisoryLockHeld is a server-wide query, so the separate + * connection proves the lock was genuinely held at the moment of the abort / redirect. * - * Lock names are fixed per action so the spec can check and clean up each one. + * The lock's release runs in a `finally`, which executes on abort / cflocation, so the lock must be + * free once the request ends — which the spec then asserts. */ component extends="Controller" { function abortDefault() { - model("author").withAdvisoryLock(name = "probe_abort_default", callback = function() { - abort; - }); + $probe(name = "probe_abort_default", tx = false, ending = "abort"); } function redirectDefault() { - model("author").withAdvisoryLock(name = "probe_redirect_default", callback = function() { - redirectTo(url = "/"); - }); + $probe(name = "probe_redirect_default", tx = false, ending = "redirect"); } function abortTransaction() { - model("author").withAdvisoryLock(name = "probe_abort_tx", transaction = true, callback = function() { - abort; - }); + $probe(name = "probe_abort_tx", tx = true, ending = "abort"); } function redirectTransaction() { - model("author").withAdvisoryLock(name = "probe_redirect_tx", transaction = true, callback = function() { - redirectTo(url = "/"); + $probe(name = "probe_redirect_tx", tx = true, ending = "redirect"); + } + + private function $probe(required string name, required boolean tx, required string ending) { + var probeName = arguments.name; + var probeEnding = arguments.ending; + // A second-connection adapter on the model's own datasource, built BEFORE the lock so the + // callback closure only references captured locals (invariant 3). $isAdvisoryLockHeld() is a + // server-wide query, so from this separate connection a true result proves the lock really is + // held during the callback. + var classData = model("author").$classData(); + var folder = Left(get("adapterName"), Len(get("adapterName")) - 5); + var checker = CreateObject("component", "wheels.databaseAdapters." & folder & "." & get("adapterName")).$init( + dataSource = classData.dataSource, + username = classData.username, + password = classData.password + ); + + model("author").withAdvisoryLock(name = probeName, transaction = arguments.tx, callback = function() { + server["wheelsAdvisoryProbe_" & probeName] = checker.$isAdvisoryLockHeld(name = probeName); + if (probeEnding == "redirect") { + redirectTo(url = "/"); + } else { + abort; + } }); } } diff --git a/vendor/wheels/tests/specs/model/advisoryLockAbortReleaseSpec.cfc b/vendor/wheels/tests/specs/model/advisoryLockAbortReleaseSpec.cfc index 36dc6cbb68..57bf98b725 100644 --- a/vendor/wheels/tests/specs/model/advisoryLockAbortReleaseSpec.cfc +++ b/vendor/wheels/tests/specs/model/advisoryLockAbortReleaseSpec.cfc @@ -66,12 +66,24 @@ component extends="wheels.WheelsTest" { } afterEach(() => { + for (var probe in ListToArray(variables.probeNames)) { + StructDelete(server, "wheelsAdvisoryProbe_" & probe); + } releaseProbeLocks(); }); it("default path: frees the lock after the callback aborts", () => { var tc = $testClient(); + StructDelete(server, "wheelsAdvisoryProbe_probe_abort_default"); tc.get("/_advisorylock/abort-default"); + expect(tc.statusCode()).toBe(200, "the abort probe request did not complete with 200"); + // positive control: the callback ran inside the lock, and a second connection saw it held + expect(StructKeyExists(server, "wheelsAdvisoryProbe_probe_abort_default")).toBeTrue( + "the probe callback never ran — the request did not reach withAdvisoryLock" + ); + expect(server["wheelsAdvisoryProbe_probe_abort_default"]).toBeTrue( + "the lock was not observably held during the callback — nothing to prove released" + ); expect(freshAdapter().$isAdvisoryLockHeld(name = "probe_abort_default")).toBeFalse( "a default-path lock stayed held after the callback aborted — the release did not run on abort" ); @@ -79,7 +91,15 @@ component extends="wheels.WheelsTest" { it("default path: frees the lock after the callback redirects", () => { var tc = $testClient(); + StructDelete(server, "wheelsAdvisoryProbe_probe_redirect_default"); tc.get("/_advisorylock/redirect-default"); + tc.assertRedirect(); + expect(StructKeyExists(server, "wheelsAdvisoryProbe_probe_redirect_default")).toBeTrue( + "the probe callback never ran — the request did not reach withAdvisoryLock" + ); + expect(server["wheelsAdvisoryProbe_probe_redirect_default"]).toBeTrue( + "the lock was not observably held during the callback — nothing to prove released" + ); expect(freshAdapter().$isAdvisoryLockHeld(name = "probe_redirect_default")).toBeFalse( "a default-path lock stayed held after the callback redirected — the release did not run on cflocation" ); @@ -87,7 +107,15 @@ component extends="wheels.WheelsTest" { it("transaction = true: frees the lock after the callback aborts", () => { var tc = $testClient(); + StructDelete(server, "wheelsAdvisoryProbe_probe_abort_tx"); tc.get("/_advisorylock/abort-tx"); + expect(tc.statusCode()).toBe(200, "the abort probe request did not complete with 200"); + expect(StructKeyExists(server, "wheelsAdvisoryProbe_probe_abort_tx")).toBeTrue( + "the probe callback never ran — the request did not reach withAdvisoryLock" + ); + expect(server["wheelsAdvisoryProbe_probe_abort_tx"]).toBeTrue( + "the lock was not observably held during the callback — nothing to prove released" + ); expect(freshAdapter().$isAdvisoryLockHeld(name = "probe_abort_tx")).toBeFalse( "a transaction-path lock stayed held after the callback aborted — the release did not run on abort" ); @@ -95,7 +123,15 @@ component extends="wheels.WheelsTest" { it("transaction = true: frees the lock after the callback redirects", () => { var tc = $testClient(); + StructDelete(server, "wheelsAdvisoryProbe_probe_redirect_tx"); tc.get("/_advisorylock/redirect-tx"); + tc.assertRedirect(); + expect(StructKeyExists(server, "wheelsAdvisoryProbe_probe_redirect_tx")).toBeTrue( + "the probe callback never ran — the request did not reach withAdvisoryLock" + ); + expect(server["wheelsAdvisoryProbe_probe_redirect_tx"]).toBeTrue( + "the lock was not observably held during the callback — nothing to prove released" + ); expect(freshAdapter().$isAdvisoryLockHeld(name = "probe_redirect_tx")).toBeFalse( "a transaction-path lock stayed held after the callback redirected — the release did not run on cflocation" ); From 8206096fce8eb27a379f9ffb27fbcaa123feafe8 Mon Sep 17 00:00:00 2001 From: Peter Amiri Date: Mon, 5 Oct 2026 03:18:33 -0700 Subject: [PATCH 3/4] fix(model): release withAdvisoryLock(transaction=true) lock when the callback aborts The release lived in the same try/finally that commits or rolls back the transaction. When an abort leaves the transaction, some engines skip that finally, so a session-scoped lock (MySQL, SQL Server) stayed held on the pooled connection until the connection closed. Move the release into an inner finally that carries no transaction-action statement, so it runs on every engine when the callback aborts; the normal path still commits before releasing so the next holder reads committed state. The regression spec now skips the transaction=true cases on PostgreSQL, whose pg_advisory_xact_lock is transaction-scoped and auto-released by the database, so there is no session-held lock to leak. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Peter Amiri --- .../4219-advisory-lock-abort-release.fixed.md | 1 + vendor/wheels/model/locking.cfm | 80 ++++++++++++------- .../model/advisoryLockAbortReleaseSpec.cfc | 14 ++++ 3 files changed, 67 insertions(+), 28 deletions(-) create mode 100644 changelog.d/4219-advisory-lock-abort-release.fixed.md diff --git a/changelog.d/4219-advisory-lock-abort-release.fixed.md b/changelog.d/4219-advisory-lock-abort-release.fixed.md new file mode 100644 index 0000000000..00d2118403 --- /dev/null +++ b/changelog.d/4219-advisory-lock-abort-release.fixed.md @@ -0,0 +1 @@ +- `withAdvisoryLock(transaction=true)` now releases its session-scoped database lock (MySQL, SQL Server) when the callback ends the request with `abort` or `redirectTo()`. The release previously ran in the same step that commits or rolls back the transaction, which some engines skip when an `abort` leaves the transaction, so the lock could stay held on the pooled connection until that connection closed; it now runs in its own step that executes on every engine, while the normal path still commits before releasing so the next holder reads committed state (#4219) diff --git a/vendor/wheels/model/locking.cfm b/vendor/wheels/model/locking.cfm index ad3a660695..60118eb412 100644 --- a/vendor/wheels/model/locking.cfm +++ b/vendor/wheels/model/locking.cfm @@ -237,37 +237,61 @@ public boolean function $advisoryLockTransactionBody(required string name, required numeric timeout, required any callback, required struct state) { local.adapter = variables.wheels.class.adapter; local.committed = {flag = false}; - var release = {error = ""}; + var release = {error = "", done = false}; var threw = {flag = false}; + var cb = {done = false}; local.adapter.$acquireAdvisoryLockTransactional(name = arguments.name, timeout = arguments.timeout); try { - local.cbResult = arguments.callback(); - if (StructKeyExists(local, "cbResult")) { - arguments.state.hasResult = true; - arguments.state.result = local.cbResult; + try { + local.cbResult = arguments.callback(); + if (StructKeyExists(local, "cbResult")) { + arguments.state.hasResult = true; + arguments.state.result = local.cbResult; + } + cb.done = true; + } finally { + // This inner finally holds the lock RELEASE and contains NO transaction-action statement, + // so BoxLang runs it even when the callback ends the request with `abort`: a `transaction + // action="…"` inside a finally is skipped by BoxLang when an abort leaves the surrounding + // cftransaction (#4219 — Lucee/Adobe run it either way). On an abnormal exit (abort or a + // throw -> cb.done stays false) the lock is released HERE; on the normal path cb.done is + // true and the commit-then-release below runs instead, preserving commit-before-release so + // the next holder reads committed state. Unscoped struct writes persist past the catch on + // BoxLang (invariant 11). + if (!cb.done) { + try { + $releaseTransactionalAdvisoryLock(adapter = local.adapter, name = arguments.name); + } catch (any releaseErr) { + WriteLog(type = "error", file = "wheels", text = "withAdvisoryLock(transaction=true): release on the abnormal-exit path also failed: " & releaseErr.message); + } + release.done = true; + } } - // Commit on the pinned connection BEFORE the release, so the next holder reads committed - // state. The block stays open on the same connection for the release that follows; - // invokeWithTransaction's own block close then has nothing left to commit. `committed` flips - // only AFTER a clean commit, so a failing commit takes the failure path (rollback). + // Normal path only (abort/throw skipped past this via the inner finally). Commit on the pinned + // connection BEFORE the release, so the next holder reads committed state. `committed` flips + // only AFTER a clean commit, so a failing commit takes the catch/finally path below. transaction action="commit"; local.committed.flag = true; + try { + $releaseTransactionalAdvisoryLock(adapter = local.adapter, name = arguments.name); + } catch (any releaseErr2) { + release.error = releaseErr2; + } + release.done = true; } catch (any e) { - // Mark a caught throw (callback or commit) so the finally does NOT roll back here: on a - // throw, invokeWithTransaction's own catch rolls the transaction back after this method - // returns, so a rollback in the finally would be a redundant second one. Only the ABORT - // path needs the finally to roll back, because an abort is never caught and the wrapper's - // catch never runs (see the finally). Unscoped struct write so it persists out of the catch - // on BoxLang (invariant 11). The exception still propagates to invokeWithTransaction. + // A caught throw (callback or commit). On a throw, invokeWithTransaction's own catch rolls the + // transaction back after this method returns, so the finally does NOT roll back. Unscoped + // struct write persists past the catch on BoxLang (invariant 11). The exception still + // propagates to invokeWithTransaction. threw.flag = true; rethrow; } finally { // Roll back ONLY on the ABORT path — not committed, and not a caught throw. An aborting // callback is never caught, so invokeWithTransaction's catch never runs and the engine would - // otherwise decide the open transaction's fate at block exit (a commit-at-exit would reopen - // the pre-commit window after the lock is freed). The rollback is in its own try/catch: a - // rollback that itself throws (a dead connection the server has already rolled back) must not - // skip the release below. + // otherwise decide the open transaction's fate at block exit. This rollback is a + // transaction-action, so BoxLang skips it when the abort leaves the cftransaction (the writes + // then commit there — a separate BoxLang data issue); the LOCK release above is in a finally + // with no transaction-action, so it still runs on every engine. Lucee/Adobe run this normally. if (!local.committed.flag && !threw.flag) { try { transaction action="rollback"; @@ -275,16 +299,16 @@ WriteLog(type = "error", file = "wheels", text = "withAdvisoryLock(transaction=true): rollback on the abort path failed: " & rollbackErr.message); } } - // Always attempt the release, whatever the rollback did. - try { - $releaseTransactionalAdvisoryLock(adapter = local.adapter, name = arguments.name); - } catch (any releaseErr) { - // Unscoped struct write so it persists out of the catch on BoxLang (invariant 11). - if (local.committed.flag) { - release.error = releaseErr; - } else { - WriteLog(type = "error", file = "wheels", text = "withAdvisoryLock(transaction=true): release after a failed callback/commit also failed: " & releaseErr.message); + // Safety net for a commit failure: a throw AFTER the inner finally leaves cb.done true (so the + // inner finally did not release) and committed false (so the commit threw). A throw runs this + // finally on every engine, so release here if it has not happened yet. + if (!release.done) { + try { + $releaseTransactionalAdvisoryLock(adapter = local.adapter, name = arguments.name); + } catch (any releaseErr3) { + WriteLog(type = "error", file = "wheels", text = "withAdvisoryLock(transaction=true): release after a failed commit also failed: " & releaseErr3.message); } + release.done = true; } } // Reached only on the committed path (a callback / commit failure propagates past here). Hand a diff --git a/vendor/wheels/tests/specs/model/advisoryLockAbortReleaseSpec.cfc b/vendor/wheels/tests/specs/model/advisoryLockAbortReleaseSpec.cfc index 57bf98b725..49159c7a69 100644 --- a/vendor/wheels/tests/specs/model/advisoryLockAbortReleaseSpec.cfc +++ b/vendor/wheels/tests/specs/model/advisoryLockAbortReleaseSpec.cfc @@ -12,6 +12,12 @@ * databases skip. The lock-held check uses the adapter's own $isAdvisoryLockHeld (the same * canonical free-check the release verification uses), on a fresh adapter/connection. * + * The two transaction = true cases run only on adapters whose transactional advisory lock is + * SESSION-scoped ($transactionalAdvisoryLockIsSessionScoped() == true: MySQL, SQL Server). On + * PostgreSQL the transactional lock is pg_advisory_xact_lock, which the database auto-releases when + * the transaction ends regardless of how the request exits — so there is no session-held lock to + * leak and the assertion would be vacuous. Those cases skip on PostgreSQL with an explanatory note. + * * Fixtures: vendor/wheels/tests/_assets/controllers/AdvisoryLockProbe.cfc + the /_advisorylock * routes in vendor/wheels/tests/routes.cfm. */ @@ -106,6 +112,10 @@ component extends="wheels.WheelsTest" { }); it("transaction = true: frees the lock after the callback aborts", () => { + if (!freshAdapter().$transactionalAdvisoryLockIsSessionScoped()) { + skip("transaction=true uses a transaction-scoped advisory lock on " & variables.adapterName & " (PostgreSQL's pg_advisory_xact_lock), auto-released by the database at transaction end — there is no session-held lock to leak when the callback aborts."); + return; + } var tc = $testClient(); StructDelete(server, "wheelsAdvisoryProbe_probe_abort_tx"); tc.get("/_advisorylock/abort-tx"); @@ -122,6 +132,10 @@ component extends="wheels.WheelsTest" { }); it("transaction = true: frees the lock after the callback redirects", () => { + if (!freshAdapter().$transactionalAdvisoryLockIsSessionScoped()) { + skip("transaction=true uses a transaction-scoped advisory lock on " & variables.adapterName & " (PostgreSQL's pg_advisory_xact_lock), auto-released by the database at transaction end — there is no session-held lock to leak when the callback redirects."); + return; + } var tc = $testClient(); StructDelete(server, "wheelsAdvisoryProbe_probe_redirect_tx"); tc.get("/_advisorylock/redirect-tx"); From 9fe0383ebf586fdc4768c0825aeff0af358412a6 Mon Sep 17 00:00:00 2001 From: Peter Amiri Date: Mon, 5 Oct 2026 03:39:37 -0700 Subject: [PATCH 4/4] test(model): run the transaction=true abort case on PostgreSQL as a hang detector MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review on #4330. On PostgreSQL the transaction=true advisory lock is transaction-scoped and auto-released, so it cannot leak — but a request that never returns keeps it held. Un-skip the abort case there and bound it with a short $testClient timeout, so a stuck request fails that case rather than timing out the whole engine leg. The redirect case still skips on PostgreSQL. Add an optional timeout (seconds, default 30) to $testClient().get() for that bound; the default keeps existing behaviour. Reword the locking.cfm comment to state only that the release must sit in a finally with no transaction-action statement, which BoxLang skips on abort. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Peter Amiri --- .../4219-testclient-get-timeout.added.md | 1 + vendor/wheels/model/locking.cfm | 6 ++--- .../model/advisoryLockAbortReleaseSpec.cfc | 26 +++++++++++-------- vendor/wheels/wheelstest/TestClient.cfc | 12 ++++++--- 4 files changed, 27 insertions(+), 18 deletions(-) create mode 100644 changelog.d/4219-testclient-get-timeout.added.md diff --git a/changelog.d/4219-testclient-get-timeout.added.md b/changelog.d/4219-testclient-get-timeout.added.md new file mode 100644 index 0000000000..ed492820c1 --- /dev/null +++ b/changelog.d/4219-testclient-get-timeout.added.md @@ -0,0 +1 @@ +- `$testClient().get()` accepts an optional `timeout` (seconds, default 30) that bounds the underlying HTTP request, so a request that may hang fails the asserting test case rather than timing out the whole test run (#4219) diff --git a/vendor/wheels/model/locking.cfm b/vendor/wheels/model/locking.cfm index 60118eb412..034401a259 100644 --- a/vendor/wheels/model/locking.cfm +++ b/vendor/wheels/model/locking.cfm @@ -289,9 +289,9 @@ // Roll back ONLY on the ABORT path — not committed, and not a caught throw. An aborting // callback is never caught, so invokeWithTransaction's catch never runs and the engine would // otherwise decide the open transaction's fate at block exit. This rollback is a - // transaction-action, so BoxLang skips it when the abort leaves the cftransaction (the writes - // then commit there — a separate BoxLang data issue); the LOCK release above is in a finally - // with no transaction-action, so it still runs on every engine. Lucee/Adobe run this normally. + // transaction-action, and BoxLang skips a finally that contains one when an abort leaves the + // cftransaction; the LOCK release above sits in a separate finally with no transaction-action, + // so it still runs on every engine on abort. Lucee/Adobe run this finally normally. if (!local.committed.flag && !threw.flag) { try { transaction action="rollback"; diff --git a/vendor/wheels/tests/specs/model/advisoryLockAbortReleaseSpec.cfc b/vendor/wheels/tests/specs/model/advisoryLockAbortReleaseSpec.cfc index 49159c7a69..7a8c157b85 100644 --- a/vendor/wheels/tests/specs/model/advisoryLockAbortReleaseSpec.cfc +++ b/vendor/wheels/tests/specs/model/advisoryLockAbortReleaseSpec.cfc @@ -12,11 +12,14 @@ * databases skip. The lock-held check uses the adapter's own $isAdvisoryLockHeld (the same * canonical free-check the release verification uses), on a fresh adapter/connection. * - * The two transaction = true cases run only on adapters whose transactional advisory lock is - * SESSION-scoped ($transactionalAdvisoryLockIsSessionScoped() == true: MySQL, SQL Server). On - * PostgreSQL the transactional lock is pg_advisory_xact_lock, which the database auto-releases when - * the transaction ends regardless of how the request exits — so there is no session-held lock to - * leak and the assertion would be vacuous. Those cases skip on PostgreSQL with an explanatory note. + * The transaction = true abort case runs on every advisory-lock database, but means something + * different per lock scope. On SESSION-scoped engines ($transactionalAdvisoryLockIsSessionScoped() + * == true: MySQL, SQL Server) it proves the session lock is released on abort. On PostgreSQL the + * transactional lock is pg_advisory_xact_lock, which the database auto-releases when the transaction + * ends — so it can't leak, but a request that never ends keeps it held; there the case becomes a + * hang detector, driven with a bounded $testClient timeout so a stuck request fails THIS case rather + * than timing out the whole engine leg. The transaction = true redirect case skips on PostgreSQL + * (redirect/cflocation completes the transaction normally, so there is nothing new to prove there). * * Fixtures: vendor/wheels/tests/_assets/controllers/AdvisoryLockProbe.cfc + the /_advisorylock * routes in vendor/wheels/tests/routes.cfm. @@ -112,14 +115,15 @@ component extends="wheels.WheelsTest" { }); it("transaction = true: frees the lock after the callback aborts", () => { - if (!freshAdapter().$transactionalAdvisoryLockIsSessionScoped()) { - skip("transaction=true uses a transaction-scoped advisory lock on " & variables.adapterName & " (PostgreSQL's pg_advisory_xact_lock), auto-released by the database at transaction end — there is no session-held lock to leak when the callback aborts."); - return; - } var tc = $testClient(); StructDelete(server, "wheelsAdvisoryProbe_probe_abort_tx"); - tc.get("/_advisorylock/abort-tx"); - expect(tc.statusCode()).toBe(200, "the abort probe request did not complete with 200"); + // Bounded timeout: on a transaction-scoped-lock engine (PostgreSQL) the lock is held for + // as long as the transaction is open, so a request that never returns keeps the lock held. + // A short timeout makes such a hang fail THIS assertion (statusCode != 200) instead of + // timing out the whole engine leg. Session-lock engines complete in milliseconds, so the + // bound never bites them. + tc.get(path = "/_advisorylock/abort-tx", timeout = 20); + expect(tc.statusCode()).toBe(200, "the abort probe request did not return 200 within the 20s bound (a transaction-scoped-lock engine that hangs here would keep the lock held)"); expect(StructKeyExists(server, "wheelsAdvisoryProbe_probe_abort_tx")).toBeTrue( "the probe callback never ran — the request did not reach withAdvisoryLock" ); diff --git a/vendor/wheels/wheelstest/TestClient.cfc b/vendor/wheels/wheelstest/TestClient.cfc index 92213af5d8..fc2777338c 100644 --- a/vendor/wheels/wheelstest/TestClient.cfc +++ b/vendor/wheels/wheelstest/TestClient.cfc @@ -287,13 +287,16 @@ component { * @path URL path (appended to baseUrl) * @params Query string parameters as a struct * @headers Additional headers for this request + * @timeout cfhttp timeout in seconds (default 30). Lower it to bound a request that may hang so + * the caller's assertion fails rather than the whole test leg timing out. */ public TestClient function get( required string path, struct params = {}, - struct headers = {} + struct headers = {}, + numeric timeout = 30 ) { - $makeRequest(method = "GET", path = arguments.path, params = arguments.params, headers = arguments.headers); + $makeRequest(method = "GET", path = arguments.path, params = arguments.params, headers = arguments.headers, timeout = arguments.timeout); return this; } @@ -733,7 +736,8 @@ component { required string path, struct params = {}, struct body = {}, - struct headers = {} + struct headers = {}, + numeric timeout = 30 ) { $requireLeadingSlash(arguments.path); @@ -755,7 +759,7 @@ component { var result = {}; - cfhttp(url = fullUrl, method = arguments.method, timeout = "30", result = "result", redirect = false) { + cfhttp(url = fullUrl, method = arguments.method, timeout = arguments.timeout, result = "result", redirect = false) { // Add merged headers for (var hName in mergedHeaders) { cfhttpparam(type = "header", name = hName, value = mergedHeaders[hName]);