From c69c6dbffd13f9f9a64e32b42e662f15e8eb72b1 Mon Sep 17 00:00:00 2001 From: Peter Amiri Date: Mon, 5 Oct 2026 05:54:44 -0700 Subject: [PATCH 1/2] refactor(model): drop the abort rollback from the transactional advisory-lock body $advisoryLockTransactionBody() rolled the transaction back in its outer finally when the callback neither committed nor threw, i.e. when the request ended with abort. Since #4335, invokeWithTransaction() itself rolls back on abort ($invokeTransactionMethod()), and this method only runs through invokeWithTransaction(), so that rollback is redundant. Removing it also drops the two flags only it read and the outer catch that only set one of them, which leaves the outer try catch-free (cross-engine invariant 22). The inner catch-free release on abort or throw and the release after a failed commit are unchanged. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Peter Amiri --- vendor/wheels/model/locking.cfm | 40 +++++++++------------------------ 1 file changed, 11 insertions(+), 29 deletions(-) diff --git a/vendor/wheels/model/locking.cfm b/vendor/wheels/model/locking.cfm index 72ba74712d..ac7b21c237 100644 --- a/vendor/wheels/model/locking.cfm +++ b/vendor/wheels/model/locking.cfm @@ -225,7 +225,9 @@ * `false` is a legitimate callback result, not a failure — a genuine failure (callback or commit) * throws, and invokeWithTransaction then rolls back and fires afterRollback. The callback's real * result is written into the shared `state` struct in the try body only (nothing written in a - * catch — BoxLang invariant 11). `committed` flips only after a clean commit. + * catch — BoxLang invariant 11). A request that ends with `abort` inside the callback is rolled + * back by invokeWithTransaction() itself ($invokeTransactionMethod(), #4335); this method only + * makes sure the lock is released on that path. * * A release failure must never reach invokeWithTransaction's rollback handler (which would fire * afterRollback on already-committed data and suppress afterCommit). So the release runs in its own @@ -236,9 +238,7 @@ */ 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 = "", done = false}; - var threw = {flag = false}; var cb = {done = false}; local.adapter.$acquireAdvisoryLockTransactional(name = arguments.name, timeout = arguments.timeout); try { @@ -269,40 +269,22 @@ } } // 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. + // connection BEFORE the release, so the next holder reads committed state. A failing commit + // throws, and the finally below still releases the lock. 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) { - // 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. This outer try has a - // catch clause, so BoxLang skips this finally on abort (cross-engine invariant 22); there the - // rollback comes from $invokeTransactionMethod(), whose catch-free finally runs this method - // through invokeWithTransaction(), and the LOCK release above sits in a separate catch-free - // try/finally. 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); - } - } - // 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. + // Safety net for a commit failure: a throw AFTER the inner finally leaves cb.done true, so the + // inner finally did not release. Release here if it has not happened yet. This try is + // catch-free too (cross-engine invariant 22), so this finally runs on every engine, including + // on abort, where the inner finally has already released and this is a no-op. A failed + // callback or commit is rolled back by invokeWithTransaction() after the exception leaves this + // method; an abort is rolled back by its $invokeTransactionMethod() (#4335). if (!release.done) { try { $releaseTransactionalAdvisoryLock(adapter = local.adapter, name = arguments.name); From c102d095ac4ce09e6cfd49ec4dfff6a16902a12d Mon Sep 17 00:00:00 2001 From: Peter Amiri Date: Mon, 5 Oct 2026 05:56:22 -0700 Subject: [PATCH 2/2] test(core): drop the locking.cfm exception from the try/catch/finally guard $advisoryLockTransactionBody() no longer has a try with both a catch and a finally, so the guard needs no exceptions. With develop's locking.cfm and an empty allowlist the guard fails at the old outer try; with this branch it passes. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Peter Amiri --- .../tests/specs/security/TryCatchFinallyGuardSpec.cfc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/vendor/wheels/tests/specs/security/TryCatchFinallyGuardSpec.cfc b/vendor/wheels/tests/specs/security/TryCatchFinallyGuardSpec.cfc index 86a349b372..1553dd708e 100644 --- a/vendor/wheels/tests/specs/security/TryCatchFinallyGuardSpec.cfc +++ b/vendor/wheels/tests/specs/security/TryCatchFinallyGuardSpec.cfc @@ -42,10 +42,8 @@ component extends="wheels.WheelsTest" { var selfName = "TryCatchFinallyGuardSpec.cfc"; var root = ExpandPath("/wheels"); - // Reviewed, intentional exceptions: path-suffix match => reason. - var allowList = { - "/model/locking.cfm": "$advisoryLockTransactionBody outer try: the lock release already runs in an inner catch-free finally (invariant 22, PR ##4330); the outer catch/rollback is tracked separately." - }; + // Reviewed, intentional exceptions: path-suffix match => reason. None at present. + var allowList = {}; // BoxLang's DirectoryList() returns a fixed-size array, so copy before appending (invariant 20). var files = [];