[bpamiri]

Fixes the advisory-lock release-on-abort behaviour and pins it with a permanent core regression spec.

Closes #4219.

The bug

withAdvisoryLock(transaction=true) takes a session-scoped database lock on MySQL (GET_LOCK) and SQL Server (sp_getapplock @LockOwner='Session'). When the callback ends the request from inside the lock with a real abort, that lock was not released on BoxLang — it stayed held on the pooled connection until the connection closed, so a later caller that borrowed the same pooled session could be blocked.

Root cause: in $advisoryLockTransactionBody, the release ran in the same try/finally that also carries the transaction's commit / rollback (transaction action="…"). On BoxLang, when an abort leaves a cftransaction, a finally that contains a transaction action= statement is skipped (Lucee and Adobe run it either way) — so the release never ran. The default (non-transaction) path was unaffected because its finally carries no transaction-action.

PostgreSQL cannot hit this: transaction = true there uses pg_advisory_xact_lock, which is transaction-scoped and auto-released by the database at transaction end regardless of how the request exits.

The fix

Move the lock release into an inner try/finally that wraps only the callback and carries no transaction-action statement. That inner finally runs on every engine when the callback aborts, releasing the lock. The outer try/finally keeps the commit/rollback handling exactly as before — this PR changes only where the lock release runs. The normal path still commits before releasing, so the next holder reads committed state. One change, all engines — no per-engine lock-owner switch.

Regression spec

advisoryLockAbortReleaseSpec drives request-ending actions over HTTP via $testClient for the default (session-scoped) and transaction = true paths, each ending via abort and via redirectTo. Fixtures: AdvisoryLockProbe.cfc + the /_advisorylock routes (core-test-only).

Positive control (so a scenario can't pass vacuously): each callback, while holding the lock and before ending the request, records $isAdvisoryLockHeld() into server["wheelsAdvisoryProbe_<name>"] from a separate adapter. The spec asserts that marker is true (lock genuinely held at the abort), asserts the response (200 for abort, a 3xx redirect for redirectTo), and only then asserts the lock is free against a fresh adapter/connection. A request that never reached withAdvisoryLock (404, wrong datasource, early throw) leaves the marker absent and fails.

On PostgreSQL the transaction = true abort case checks that the aborted request completes (bounded client timeout); the transaction = true redirect case skips there. An earlier matrix leg on Adobe 2025 + PostgreSQL timed out and is being investigated separately.

Verification (local, reds-before / greens-after)

Engine	DB	unfixed (reds-before)	fixed (greens-after)
BoxLang	MySQL	transaction=true abort FAIL — lock stayed held (verified held on the pooled MySQL connection)	4 / 0 / 0
BoxLang	SQL Server	transaction=true abort FAIL — lock stayed held	4 / 0 / 0
Lucee 7	MySQL	finally always ran (no regression to flip)	4 / 0 / 0
Lucee 7	PostgreSQL	—	3 pass / 1 skip (abort-tx runs, redirect-tx skips)
BoxLang	PostgreSQL	—	3 pass / 1 skip
Adobe 2023	MySQL	finally always ran (no regression to flip)	4 / 0 / 0
Adobe 2025	MySQL	finally always ran (no regression to flip)	4 / 0 / 0
Adobe 2025	PostgreSQL	—	3 pass / 1 skip (abort-tx completes within the bound)

The reds-before is a real leak, not a connection-reset artifact: the unfixed run left the lock held on the pooled connection, verifiable directly in the database (and it would have released if the pool reset on abort). The compat matrix covers every engine × MySQL / PostgreSQL / SQL Server.

🤖 Generated with Claude Code

[bpamiri]

rev1-r3 review @ ee0ede6: CHANGES (the spec can pass without the lock ever being taken; and please show it reds)

What I checked and found right:

• The probe routes are core-test only. /_advisorylock/* is in vendor/wheels/tests/routes.cfm, which only the core runner includes (vendor/wheels/tests/runner.cfm L79). The controller is under vendor/wheels/tests/_assets/controllers/. Neither reaches a user app or the wheels new template. The scope sits before .wildcard(), as it must.

• The "lock is free" check can't be fooled by the connection it runs on. $isAdvisoryLockHeld() with no holder is a server-wide query on every supported adapter:

  • PostgreSQL reads pg_locks for the database;
  • MySQL uses IS_USED_LOCK(), which returns any holder's connection id;
  • SQL Server reads sys.dm_tran_locks, falling back to APPLOCK_MODE for its own session plus APPLOCK_TEST for others.

  So even if the fresh adapter's query lands on the very pooled connection a leaked lock sits on, it still reports held. The separate adapter is belt-and-braces, not load-bearing.

• Skips. Exactly the three adapters with real locks run, and everything else (SQLite, H2, Oracle, CockroachDB) gets one explicit skip(). That matches advisoryLockExclusionSpec / advisoryLockTransactionalSpec. PostgreSQL is covered by the matrix's postgres legs.

• afterEach can't mask a leak. It runs after each it()'s assertion, so a leak has already failed that test before cleanup touches it. Each probe has its own lock name, so one scenario's leftover can't affect another's check.

The change I'm asking for: prove each request actually took the lock.

Every assertion is "the lock is not held". That is also exactly what you see if the probe request never reaches withAdvisoryLock(), for example:

• the route doesn't match in the app $testClient addresses (404 or wildcard);
• the controller fails to load;
• the model call throws before acquiring;
• the request goes to a different datasource.

Today all four it()s would pass in each of those cases, and the spec doesn't look at tc.get()'s result at all.

Please add a positive control to each scenario:

1. Assert the response proves the action ran: the expected status for abort (200 with an empty body) and for redirectTo (302 plus the Location, if $testClient doesn't follow redirects).
2. Better, have each probe callback record that it is inside the lock before it ends the request. For example, set server["wheelsAdvisoryProbe_" & name] = true, or write a row the spec can read, then assert it in the spec and clear it in afterEach. Even better, record the result of a held-check ($isAdvisoryLockHeld(name)) taken inside the callback, which should be true. That pins that the lock was really held at the moment of the abort or redirect, so "free afterwards" means something.

Please also show a RED. The PR reports 4/0/0 green on MySQL and SQL Server, but a regression spec that only ever passed doesn't show it can fail. Please run it locally once against a deliberately broken release on at least one database: for example, move the release out of the finally in withAdvisoryLock() (or put it after the callback) in a scratch worktree, and paste the failing output. With the positive control in place, that RED is what proves the spec guards the finally.

Final sign-off after that delta, matrix run 37281337199 (or a fresh run on the new head) and PR CI.

[bpamiri]

rev1-r3 re-review @ 9c4ddf1: code APPROVE. One piece of evidence is still owed before the final: a RED for the transaction = true abort case. That needs no push, just the PR body.

The delta ee0ede66a..9c4ddf10d is test-only (the probe controller and the spec) and adds what I asked for:

• A positive control in all four scenarios. Inside the lock, before ending the request, each callback stores a held-check in server["wheelsAdvisoryProbe_<name>"]. The spec deletes that marker before the request, then asserts it exists and is true. So a request that never reached withAdvisoryLock() (404, load failure, wrong datasource, early throw) now fails on "the probe callback never ran". It can no longer pass as "free". The response is asserted too: statusCode() == 200 for abort, assertRedirect() for redirectTo.
• The server marker is namespaced and cleaned up. Each one is wheelsAdvisoryProbe_ plus one of the four fixed probe names, so it can't collide with another key. It's deleted before each request and again for all four names in afterEach, which runs even when an assertion fails. Nothing outlives the spec.
• The closure reads only captured var locals (probeName, probeEnding, checker), and the checker is built before the lock, which is the right shape. The adapter the Adobe legs exercise is in matrix run 37282611364.

Small wording point, not blocking: on the transaction = true path the checker's query runs inside the lock's cftransaction. Lucee and Adobe then run it on the transaction's own connection, not a "second" one. The check is still right, because $isAdvisoryLockHeld() with no holder is server-wide on all three adapters, and SQL Server's fallback covers the caller's own session through APPLOCK_MODE. Only the "SECOND connection" comments overstate it.

What's still owed: a RED for the transaction path. The RED in the body ("moving the release out of the withAdvisoryLock finally … 3 pass / 1 fail, a default-path lock stayed held") reds only the default abort case. That fits: the mutation touched $runWithAdvisoryLock()'s finally (locking.cfm L110). The transaction = true path releases in a different finally, in $advisoryLockTransactionBody() (L262). It also rolls back there on abort, because the abort skips the wrapper's catch. So the run shown doesn't show that "transaction = true: frees the lock after the callback aborts" can fail.

Please do the same mutation on that finally (L262) in a scratch tree. Paste the result (I'd expect abort-tx to red) and revert. If it stays green, say why: for example, if the abort's rollback or the connection's return to the pool frees the session lock anyway. Then that case documents behaviour rather than guarding the finally, the same as the redirect cases you already explain.

Final sign-off after that note in the PR body, matrix run 37282611364 and PR CI.

[github-actions]

Wheels Test Results

222 036 tests   217 104 ✅  1h 25m 18s ⏱️
 17 216 suites    4 932 💤
     32 files          0 ❌

Results for commit 9fe0383.

♻️ This comment has been updated with latest results.

[bpamiri]

rev1-r3 review @ 8fc602d: CHANGES (the locking.cfm fix is right; the PostgreSQL skip removes the only check on the Adobe 2025 + PostgreSQL hang)

The PR's own change is 8206096fc (locking.cfm, the spec and a changelog fragment). The two develop merges bring in #4329, #4331 and #4332 unchanged.

$advisoryLockTransactionBody(): I traced every exit path.

Exit	Inner finally	catch	Outer finally	Result
Normal	cb.done is true, so no release	—	committed, so no rollback; release.done, so no release	commit, then release (failure captured in release.error and surfaced after the wrapper commits) ✓
Callback throws	releases (failure logged), release.done = true	threw = true, rethrow	no rollback (the wrapper rolls back); release.done, so no release	released once, then rolled back ✓
Commit throws	cb.done is true, so no release	threw = true, rethrow	no rollback; !release.done, so releases	released once (the safety net) ✓
abort	releases, and has no transaction action, so BoxLang runs it too	—	rollback	released on every engine ✓

• No double release: release.done guards both later release points.
• The cross-engine invariants hold. The flags are unscoped var structs written outside any catch (invariant 11). There are no loops in either finally (invariant 12).
• The adapter changes to $advisoryLockConnection() come from fix(model): take MySQL and PostgreSQL advisory locks with the shared connection options #4331, through the merge.
• $transactionalAdvisoryLockIsSessionScoped() isn't new. It's already public and $-prefixed on develop (Base.cfc L1254, overridden in the MySQL and SQL Server adapters), and the spec just calls it.

One behaviour change worth stating: on the abort path, Lucee and Adobe now release before the rollback. Before, they rolled back and then released. That's acceptable, because the next holder can't see uncommitted writes and at worst waits on their row locks until the rollback finishes.

Requested change: don't skip the transaction = true abort case on PostgreSQL.

The reason given is that pg_advisory_xact_lock "auto-releases at transaction end, so the assertion would be vacuous". But the transaction only ends if the request does. The last matrix run (37282611364) showed adobe2025 + postgres hanging: 3 attempts, each about 15 minutes, HTTP 000, no junit file. That's exactly the "aborted request inside withAdvisoryLock(transaction=true) never completes" case. While that request thread is stuck, its transaction is open and the xact lock is held. So on PostgreSQL the "is the lock free afterwards" assertion, together with the status-200 and marker checks, is what catches that hang.

With the skip, this head can't tell us whether the hang is gone. The verification table doesn't include Adobe 2025 + PostgreSQL, and the body doesn't say what the thread dump showed. Please:

1. Run the transaction = true abort case on PostgreSQL again, with a bounded $testClient timeout so a hang fails the case instead of stalling the leg.
2. Report what the Adobe 2025 + PostgreSQL thread dump showed, and whether the case completes on this head. If the new release ordering fixed it, that's worth saying. If Adobe 2025 still doesn't return from an abort inside a PostgreSQL cftransaction, that's a user-facing hang in any app doing the same, and it needs an issue and the evidence in the body. It shouldn't be skipped away.

The redirect-tx case can stay skipped on PostgreSQL, since redirectTo defers its cflocation until after the action, as you measured.

Final sign-off after the PostgreSQL case is restored, matrix run 37296064970 (or a fresh one) with every leg read, including adobe2025 + postgres, and PR CI.

[bpamiri]

rev1-r3 re-review @ 9fe0383: code APPROVE (final sign-off after the matrix run on this head, with every leg read including adobe2025 + postgres, and PR CI)

Delta 8fc602d2f..9fe0383eb (4 files) does what I asked:

• The transaction = true abort case runs on PostgreSQL again. There it acts as a hang detector: while an aborted request's transaction is still open, pg_advisory_xact_lock stays held, so the status-200, marker and lock-free assertions together fail a hang. Only the redirect case still skips on PostgreSQL, with the reason in the docblock.
• The client timeout is bounded. TestClient.get() gains an optional timeout (seconds, default 30, the old hard-coded value), passed through $makeRequest() to cfhttp. Every other verb still gets 30 through $makeRequest()'s default, so existing callers see no change. The new argument comes after the existing ones, so positional calls are unaffected. The abort case uses timeout = 20, so a stuck request fails this case instead of timing out the whole leg. afterEach is bounded too: its probe cleanup uses timeout = 1.
• The body now has the evidence. The verification table has an Adobe 2025 + PostgreSQL row (3 pass / 1 skip, the abort case completing within the bound), plus a note that the earlier leg's timeout is being looked into separately.
• locking.cfm changes in one comment only. There's no code change since my path-by-path review at 8fc602d2f.
• Changelog: there's a new fragment for the get(timeout) argument.

[bpamiri]

rev1-r3 review @ 9fe0383: APPROVE (final; matrix run 37298096881 read leg by leg, PR CI checked)

• Matrix run 37298096881 is on this head, and all 32 legs plus rustcfml succeeded. That includes adobe2025 + postgres, which completed with a full result this time (6936 tests). From the junit artifact for each leg:
  • 6933–6950 tests per leg, with 0 failures and 0 errors on every leg.
  • advisoryLockAbortReleaseSpec:
    • MySQL and SQL Server: 4/4 run and pass on every engine (Lucee 6/7, Adobe 2023/2025, BoxLang), including the two BoxLang legs that failed before this fix.
    • PostgreSQL: 3 run and pass and 1 skips (redirect-tx) on every engine. So the transaction = true abort case runs and completes within its bound everywhere, Adobe 2025 included.
    • Other databases: it skips as designed.
  • advisoryLockTransactionalSpec (20) passes on every leg. That includes boxlang + sqlserver, where "holds the lock on the same session the model queries use" had failed alongside the leak.
• PR CI on this head: 17 pass, 4 skipped, 0 failing or pending.