test(model): guard lockingSpec advisory-lock tests on adapters without standalone support#2670
Conversation
…t standalone support Add `$supportsAdvisoryLocks()` to the database adapter base + overrides on PostgreSQL/MySQL/SQLite (true) and MicrosoftSQLServer (false, awaiting a transaction wrapper). lockingSpec consults the new capability via a beforeEach `skip(...)` instead of either silently `return`-ing on CockroachDB or hard-erroring on H2/SQLServer/Oracle. Compat-matrix can now distinguish "lock implementation broken" from "lock not applicable to this DB." The BoxLang `Expected [false] to be true` failure on the release-on-exception case is intentionally left out of scope — that's a real bug on a DB that does support advisory locks, separate carve-out per the triage. Fixes #2665 Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Wheels Bot — No doc updatesReviewed this PR's diff and found no docs that need updating (purely internal test-fix: the new |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This is a clean, well-scoped fix that replaces the ad-hoc CockroachDB return guards in lockingSpec with a proper capability-accessor pattern ($supportsAdvisoryLocks()) across all database adapters. The mechanics are correct, the test structure is sound, and cross-engine compatibility is fine. One minor nit about a redundant override, but nothing blocking.
Correctness
The change does exactly what the PR description says. The flow is:
Base.cfcprovides a default$supportsAdvisoryLocks() → false(safe default — unknown adapters do not claim advisory-lock support).- PostgreSQL, MySQL, and SQLite override to
true. - SQL Server overrides to
false(same as the base, but with an explanatory comment). - Oracle, H2, and CockroachDB inherit
falsewithout an explicit override — correct, since all three throwWheels.AdvisoryLockNotSupportedfrom their lock methods (verified invendor/wheels/databaseAdapters/{Oracle,H2,CockroachDB}/). lockingSpecguards thewithAdvisoryLockdescribe block with abeforeEachskip, and the oldvar _isCockroachDB+if (_isCockroachDB) return;pattern is fully removed.
All code paths are covered. The new capability-accessor describe block runs unconditionally and verifies that the method exists and returns a boolean on every adapter — a sensible smoke test.
Conventions
Minor nit — redundant SQL Server override
vendor/wheels/databaseAdapters/MicrosoftSQLServer/MicrosoftSQLServerModel.cfc (added lines):
public boolean function $supportsAdvisoryLocks() {
return false;
}This returns exactly what Base.cfc already returns. Oracle, H2, and CockroachDB (also non-supporting) correctly inherit false without an explicit override. Keeping an explicit false only for SQL Server creates a "why is this one different?" reader puzzle.
The comment explaining the implicit-transaction wrapper requirement is genuinely useful — but it could live in Base.cfc's method comment (perhaps under a ## Known non-supporting adapters list) rather than in a redundant override. Alternatively, a follow-up issue tracking the SQL Server implicit-transaction wrapper work is the right home for that detail.
This is a nit only — the code is not wrong, just inconsistent.
Cross-engine
No issues. Specifically checked:
-
publicaccess with$prefix — all four adapter overrides and the base method usepublic boolean function $supportsAdvisoryLocks(). Per CLAUDE.md "Private mixin functions not integrated": mixin CFCs must usepublic+$prefix. Database adapter CFCs are not mixin targets, butpublicis still correct here since the test spec calls the method directly. -
Closure accessing
ginbeforeEach—g = application.wois an unscoped assignment insiderun(), which places it in the CFC'svariablesscope (notlocal). Closures close over the declaring component'svariablesscope on Lucee, Adobe CF, and BoxLang — this is the same reason the existingit()closures can callg.model(...). No regression. -
skip()insidebeforeEachclosure —skip()is inherited fromBaseSpecand resolves the same wayexpect()does in existingit()closures. The call is unqualified and resolves through the component's scope, consistent with every other WheelsTest BDD spec in the repo. -
No bracket-notation function calls, no
clientscope, noapplicationscope members — none of the Adobe CF / BoxLang gotchas apply.
Tests
The new describe("Tests that withAdvisoryLock") beforeEach correctly guards all four it() blocks. The new describe("$supportsAdvisoryLocks adapter capability") has one unconditional test that fires on every adapter and checks the return type. The PR description reports 811 pass, 0 fail, 0 error, 11 skipped on Lucee 7 + SQLite, which is the expected outcome. Compat-matrix should now show "Skipped" (not error) on H2, SQL Server, Oracle, and CockroachDB runs. Good.
Docs
- CHANGELOG
[Unreleased] → Changedentry present. - PR body notes that
.ai/andCLAUDE.mddoc updates are handled by thebot-update-docs.ymlfollow-up.
Commits
Single commit: test(model): guard lockingSpec advisory-lock tests on adapters without standalone support
Type test is in the allowlist. Scope model is appropriate. Subject is 75 characters, not ALL-CAPS. Passes commitlint. DCO sign-off flagged as verified in the PR checklist.
Wheels Bot — Reviewer B (round 1)A's review is accurate and well-reasoned on the main substance. One false positive in the SQL Server nit reasoning, one minor missed item on the BoxLang tracking issue, and one very minor CHANGELOG section placement question. None are blocking. SycophancyNone detected. A filed a COMMENTED review (not APPROVED), raised a concrete nit, and verified cross-engine concerns rather than rubber-stamping. False positives
Missed issues
Verdict alignmentA's COMMENTED verdict is consistent with the finding of one non-blocking nit. I would reach the same verdict. ConvergenceAligned on outcome: no changes required before merge, but requesting a BoxLang tracking issue linked from the PR body (or confirmation that #2649 is the designated home) is a reasonable ask before the PR moves out of draft. The SQL Server nit reasoning is slightly imprecise but the actionable conclusion is correct and does not need to be revisited. Joint recommendation: converged-approve pending confirmation of the BoxLang follow-up ticket. |
Two of the three sub-clusters from #2663 — the third (lockingSpec guard, #2665) already landed via #2670 on develop, so this rebase drops the manual hard-coded skip list in favor of the merged $supportsAdvisoryLocks() capability. 1. Lucee 6 datasource registration: switch dbdriver from "Oracle" to "Other" in tools/docker/lucee6/CFConfig.json so the JDBC URL is used as-is rather than re-templated by Lucee's Oracle extension. Matches the working Adobe 2023/2025 and BoxLang configs. 2. Add oracle to SOFT_FAIL_DBS in compat-matrix.yml so the remaining Oracle constraint-cleanup issues (ORA-02264 / ORA-01003) log as warnings instead of breaking each engine job while a follow-up resolves the migratorSpec / bulkOperationsSpec teardown drift. Constraint cleanup (sub-cluster 3 in the issue) is left for a separate PR — shares shape with #2664's MySQL/H2/SQL Server FK collision work. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two of the three sub-clusters from #2663 — the third (lockingSpec guard, #2665) already landed via #2670 on develop, so this rebase drops the manual hard-coded skip list in favor of the merged $supportsAdvisoryLocks() capability. 1. Lucee 6 datasource registration: switch dbdriver from "Oracle" to "Other" in tools/docker/lucee6/CFConfig.json so the JDBC URL is used as-is rather than re-templated by Lucee's Oracle extension. Matches the working Adobe 2023/2025 and BoxLang configs. 2. Add oracle to SOFT_FAIL_DBS in compat-matrix.yml so the remaining Oracle constraint-cleanup issues (ORA-02264 / ORA-01003) log as warnings instead of breaking each engine job while a follow-up resolves the migratorSpec / bulkOperationsSpec teardown drift. Constraint cleanup (sub-cluster 3 in the issue) is left for a separate PR — shares shape with #2664's MySQL/H2/SQL Server FK collision work. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com>
* ci: unblock Oracle in compat matrix and fix Lucee 6 datasource (#2663) Two of the three sub-clusters from #2663 — the third (lockingSpec guard, #2665) already landed via #2670 on develop, so this rebase drops the manual hard-coded skip list in favor of the merged $supportsAdvisoryLocks() capability. 1. Lucee 6 datasource registration: switch dbdriver from "Oracle" to "Other" in tools/docker/lucee6/CFConfig.json so the JDBC URL is used as-is rather than re-templated by Lucee's Oracle extension. Matches the working Adobe 2023/2025 and BoxLang configs. 2. Add oracle to SOFT_FAIL_DBS in compat-matrix.yml so the remaining Oracle constraint-cleanup issues (ORA-02264 / ORA-01003) log as warnings instead of breaking each engine job while a follow-up resolves the migratorSpec / bulkOperationsSpec teardown drift. Constraint cleanup (sub-cluster 3 in the issue) is left for a separate PR — shares shape with #2664's MySQL/H2/SQL Server FK collision work. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com> * ci(compat-matrix): hoist SOFT_FAIL_DBS out of per-db loop Match the pattern already used in the summary step (line 470) where SOFT_FAIL_DBS is set once before the loop. The previous placement reassigned the same constant on every database iteration. Same value, cleaner scope. Per PR #2683 review feedback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com> * docs(claude): describe SOFT_FAIL_DBS locations by step, not line number Line numbers shift when assignments move (as in the previous commit which hoisted SOFT_FAIL_DBS out of the per-db loop). Replace the `~421, ~469` references with prose tied to the workflow step names so the docs stop rotting on every adjacent edit. Per PR #2683 review. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com> * docs(claude): use exact step name "Run test suites for all databases" Match the workflow step name verbatim per PR #2683 reviewer nit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com> --------- Signed-off-by: Peter Amiri <peter@alurium.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… to avoid FK collision Closes #2663 (sub-cluster 3) and resolves #2664. The migrator spec's migrateTo describe block was leaking the wheels_migrator_versions / c_o_r_e_migrator_versions tables (plus their fk_wheels_level FK constraint) between tests. When the "uses specified versions table name" test then swapped migratorTableName to c_o_r_e_migrator_versions, the bootstrap path in $getVersionsPreviouslyMigrated() added the same fk_wheels_level constraint to a different table — MySQL, H2, SQL Server, and Oracle scope FK names per-schema rather than per-table, so the second CREATE failed with ORA-02264 ("name already used by an existing constraint") / "Duplicate foreign key constraint name". Mirror the cleanup pattern already used by the F15 Phase 1 describe block: in beforeEach, drop both table-name families inside try/catch so the bootstrap recreates them cleanly per test. wheels_levels can stay — it is referenced by the FK, not the source of it, and the bootstrap is happy to reuse it. This completes the three-cluster fix for #2663: - sub-cluster 1 (Lucee 6 Oracle JDBC URL) landed in #2683. - sub-cluster 2 (DBMS_LOCK / lockingSpec guard) landed via #2670. - sub-cluster 3 (constraint cleanup) — this PR. Removing "oracle" from SOFT_FAIL_DBS in compat-matrix.yml is left for a follow-up once a green Oracle matrix run is observed. Signed-off-by: Claude <noreply@anthropic.com>
…ops (#2692) * fix(ci): unbreak Oracle compat-matrix — Lucee 6 dbdriver + cascade drops Three independent Oracle failures tracked in #2663: 1. Lucee 6 datasource registration: `dbdriver: "Other"` left `{host}/{port}/{database}` placeholders unresolved, producing "java.sql.SQLException: Invalid Oracle URL specified" on every spec. Aligned with Lucee 5/7 by setting `dbdriver: "Oracle"` (the Oracle .lex extension is already installed in both Lucee images). 2. lockingSpec DBMS_LOCK errors were already fixed by #2670 (advisory-lock capability flag defaults to false on Base, Oracle inherits, suite skips cleanly). No further work needed on that cluster. 3. Migrator/bulkOperations ORA-02264 (constraint name reuse): Oracle's `dropTable` emitted `DROP TABLE IF EXISTS name` with no cascade clause, so incoming FK constraints survived the drop and the next CREATE collided on the FK name. Added `CASCADE CONSTRAINTS` to the adapter SQL and matched the same shape (with `PURGE` to bypass the recycle bin) in the core tests' populate.cfm so the table-clear loop is robust across matrix re-runs. `oracle` stays in SOFT_FAIL_DBS until a green run confirms the fix; removal will follow in a separate cleanup PR once CI shows clean Oracle output on all engines. Fixes #2663 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: harden compat-matrix against transient external download failures The PR-trigger of the compat-matrix on 2026-05-15 02:54 UTC hit a wave of GitHub Releases 503/504s during engine builds — four of five engines failed before tests ran. Two independent fixes: 1. Drop `commandbox-cfformat` from CI box.json devDependencies. The module's post-install pulls `cftokens` binaries from a GitHub release; when that release returns 503, `chmod` fails and `box install` aborts the build. cfformat is a code-formatting dev tool with no runtime role in the test container — its absence does not affect any spec. Affected: lucee6, lucee7, adobe2023, adobe2025. `commandbox-cfconfig` (Adobe) and `commandbox-dotenv` (all) stay because they apply server config at boot. 2. Retry `docker compose build` up to 3× with a 30s backoff in the "Start CF engine" step. The build is idempotent; the retry only wraps the network-dependent layers (Adoptium JDK/JRE downloads for Adobe/BoxLang, .lex extension downloads for Lucee). Once any attempt succeeds, `docker compose up -d` runs once. This sits on top of the Oracle fixes already in this PR (#2663). With both improvements in place, a re-run on this branch should produce test results for all five engines. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: pin all compat-matrix versions to last-known-good Stop chasing tail every time an upstream dependency releases a new version and breaks the chain. Everything the matrix touches is now pinned to a specific version that we've observed working. Three layers of pins: 1. Service containers (compose.yml): - mysql:9 → mysql:9.7.0 - postgres:18 → postgres:18.4 - cockroachdb/cockroach:latest-v24.3 → :v24.3.32 (both occurrences) - gvenzl/oracle-free:23-slim → :23.26.1-slim - mcr.microsoft.com/mssql/server:2025-latest → :2025-CU4-ubuntu-22.04 2. CommandBox base images (Dockerfiles) — pinned by sha256 digest since ortussolutions/commandbox has no semver tags: - commandbox:latest → @sha256:1f180edf... (lucee6/7, adobe2023, boxlang) - commandbox:jdk21 → @sha256:d65126ca... (adobe2025) These are the digests of the images that built Lucee 7's successful run in matrix run 25897604531. 3. CommandBox modules (box.json): - commandbox-dotenv: "*" → "2.4.1" - commandbox-cfconfig: "*" → "2.2.1" - BoxLang's bx-*: removed all ^ carets, pinning the +build suffix exactly (e.g. "^1.27.0+35" → "1.27.0+35") Legacy engines (lucee5, adobe2018, adobe2021) are NOT in the active CI matrix per CLAUDE.md and are deliberately left alone. Bumping any pin is now a deliberate, reviewable change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ockingSpec skips (#2746) * fix(model): override $supportsAdvisoryLocks=false on CockroachDB so lockingSpec skips CockroachDBModel extends PostgreSQLModel, which reports $supportsAdvisoryLocks()=true since pg_advisory_lock is native there. CockroachDB intentionally omits the primitive — its adapter throws "CockroachDB does not support advisory locks." from $acquireAdvisoryLock — but it never overrode the capability flag, so the four lockingSpec withAdvisoryLock tests bypassed the beforeEach skip-guard added in #2670 and reported as errors on every engine (lucee6, lucee7, boxlang). PR #2670's CHANGELOG entry already named CockroachDB in the "skips on" list, but the override itself was missed. This adds the one-method override, mirroring how MSSQL/SQLite/etc. each carry their own $supportsAdvisoryLocks override over Base.cfc's default-false. Fixes #2743 Signed-off-by: Peter Amiri <peter@alurium.com> * docs(model): trim redundant forUpdate hint from CockroachDB $supportsAdvisoryLocks docblock Reviewer A nit on #2746: the final sentence duplicates guidance already on the adjacent $acquireAdvisoryLock / $releaseAdvisoryLock docblocks. Keep the comment focused on the WHY (inheritance/capability-flag subtlety). Signed-off-by: Peter Amiri <peter@alurium.com> --------- Signed-off-by: Peter Amiri <peter@alurium.com>
) `lockingSpec :: "releases lock even when callback throws an exception"` failed on BoxLang × MySQL / Postgres / SQLite — the missing leg of #2665 that #2670 intentionally deferred. The test tracked exception propagation through `local.exceptionThrown = true` inside the `catch` block; on BoxLang, writes to the `local` scope inside a catch don't survive past the block (the catch body runs under a nested `local` that gets discarded on exit), so the post-catch `expect(local.exceptionThrown)` read the un-touched outer value and failed with "Expected [false] to be true". Lucee 6 / 7 and Adobe CF 2023 / 2025 share the enclosing function's `local` across the catch, so the assignment stuck on every other engine and the gap was BoxLang-only. Switched to the same struct-field pattern TenantResolverSpec already uses for the equivalent assertion: var state = {exceptionThrown = false}; try { ... } catch (TestException e) { state.exceptionThrown = true; } expect(state.exceptionThrown).toBeTrue(); Struct field assignment targets a heap object and survives the scope transition on every engine. `vendor/wheels/model/locking.cfc` is unchanged — the lock-release contract was already correct via the existing `try { callback() } finally { release }`; the lock-release bug the issue title called out turned out not to exist. Documented the BoxLang catch-scope quirk in `.ai/wheels/cross-engine-compatibility.md` so future spec authors don't re-hit it. Both the `var struct.field` form and the `var bareName` form are called out as portable; the `local.X = ...` form is called out as the trap. Fixes #2744 Signed-off-by: Peter Amiri <peter@alurium.com>
) * fix(test): work around BoxLang catch-scope discard in lockingSpec (#2744) `lockingSpec :: "releases lock even when callback throws an exception"` failed on BoxLang × MySQL / Postgres / SQLite — the missing leg of #2665 that #2670 intentionally deferred. The test tracked exception propagation through `local.exceptionThrown = true` inside the `catch` block; on BoxLang, writes to the `local` scope inside a catch don't survive past the block (the catch body runs under a nested `local` that gets discarded on exit), so the post-catch `expect(local.exceptionThrown)` read the un-touched outer value and failed with "Expected [false] to be true". Lucee 6 / 7 and Adobe CF 2023 / 2025 share the enclosing function's `local` across the catch, so the assignment stuck on every other engine and the gap was BoxLang-only. Switched to the same struct-field pattern TenantResolverSpec already uses for the equivalent assertion: var state = {exceptionThrown = false}; try { ... } catch (TestException e) { state.exceptionThrown = true; } expect(state.exceptionThrown).toBeTrue(); Struct field assignment targets a heap object and survives the scope transition on every engine. `vendor/wheels/model/locking.cfc` is unchanged — the lock-release contract was already correct via the existing `try { callback() } finally { release }`; the lock-release bug the issue title called out turned out not to exist. Documented the BoxLang catch-scope quirk in `.ai/wheels/cross-engine-compatibility.md` so future spec authors don't re-hit it. Both the `var struct.field` form and the `var bareName` form are called out as portable; the `local.X = ...` form is called out as the trap. Fixes #2744 Signed-off-by: Peter Amiri <peter@alurium.com> * docs(claude): address Reviewer A nits on #2744 (CLAUDE.md invariant + doc + spec) - Collapse the 8-line in-spec comment to one line. The `.ai/` doc carries the full WHY; the spec only needs the pointer. Matches CLAUDE.md's "one short line max" rule for inline comments. - Switch struct literal in `lockingSpec.cfc` from colon (`:`) to equals (`=`) syntax to match the prior-art pattern in `TenantResolverSpec` (`var result = {threw = false}`) and the WRONG/RIGHT examples in the cross-engine doc. Both are valid CFML; consistency wins. - Add a "Why the bare-`var` form survives" paragraph to the `.ai/wheels/cross-engine-compatibility.md` BoxLang catch-scope section so future readers don't ask why `var caught; caught = true` survives when `local.caught = true` doesn't. Also recommends preferring the struct-field form since the bare-var mechanism may not be a stable contract across BoxLang releases. - Add invariant #10 to CLAUDE.md's "Cross-Engine Invariants" checklist. CLAUDE.md is the canonical drive-by checklist; spec authors who stop there need this rule visible at the surface level. One-liner pattern matches the existing 9 invariants. Signed-off-by: Peter Amiri <peter@alurium.com> --------- Signed-off-by: Peter Amiri <peter@alurium.com>
Summary
Adds a
$supportsAdvisoryLocks()capability accessor to the database adapter base + overrides on PostgreSQL/MySQL/SQLite (true) and MicrosoftSQLServer (false, pending an implicit-transaction wrapper).lockingSpecconsults the new capability via abeforeEach { skip(...) }so advisory-lock tests are now reported as Skipped (with a reason) on H2, SQL Server, Oracle, and CockroachDB instead of erroring. The CockroachDB silentreturnhack is removed in favor of the same skip path, so compat-matrix can distinguish "lock implementation broken" from "lock not applicable to this DB."The BoxLang
Expected [false] to be truefailure on therelease-on-exceptioncase is intentionally left out of scope per the triage — that's a real bug on a DB that does support advisory locks (mysql/postgres/sqlite) and warrants its own ticket.Related Issue
Fixes #2665
Type of Change
Feature Completeness Checklist
Signed-off-by:(usegit commit -s); see CONTRIBUTING.mdwithAdvisoryLockdescribe now skips on unsupported adaptersbot-update-docs.ymlfollow-upbot-update-docs.ymlfollow-upbot-update-docs.ymlfollow-up[Unreleased] → Changedwheels.tests.specs.modelreports 811 pass, 0 fail, 0 error, 11 skipped (all 11 lockingSpec specs pass on SQLite; pre-existing skips on raceconditionSpec / useIndex are unrelated)Test Plan
/wheels/core/tests?db=sqlite&format=json&directory=wheels.tests.specs.modelafter the CI setup-wheels-test-env action — confirms 0 errors and alllockingSpecspecs passing