fix(model): release withAdvisoryLock(transaction=true) lock when the callback aborts

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)

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)

vendor/wheels/model/locking.cfm

@@ -237,54 +237,78 @@
 	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, 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";
 				} catch (any rollbackErr) {
 					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

vendor/wheels/tests/_assets/controllers/AdvisoryLockProbe.cfc

@@ -0,0 +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.
+ *
+ * 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_<name>"]. 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.
+ *
+ * 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() {
+		$probe(name = "probe_abort_default", tx = false, ending = "abort");
+	}
+
+	function redirectDefault() {
+		$probe(name = "probe_redirect_default", tx = false, ending = "redirect");
+	}
+
+	function abortTransaction() {
+		$probe(name = "probe_abort_tx", tx = true, ending = "abort");
+	}
+
+	function redirectTransaction() {
+		$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;
+			}
+		});
+	}
+}

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")

vendor/wheels/tests/specs/model/advisoryLockAbortReleaseSpec.cfc

@@ -0,0 +1,159 @@
+/**
+ * #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.
+ *
+ * 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.
+ */
+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(() => {
+				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"
+				);
+			});
+
+			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"
+				);
+			});
+
+			it("transaction = true: frees the lock after the callback aborts", () => {
+				var tc = $testClient();
+				StructDelete(server, "wheelsAdvisoryProbe_probe_abort_tx");
+				// 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"
+				);
+				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"
+				);
+			});
+
+			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");
+				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"
+				);
+			});
+		});
+	}
+}