Skip to content

fix(test): serialize web test-runner config swap under a named lock (#3025) - #3373

Merged
bpamiri merged 1 commit into
developfrom
fix/bot-3025-test-runner-lock
Aug 5, 2026
Merged

fix(test): serialize web test-runner config swap under a named lock (#3025)#3373
bpamiri merged 1 commit into
developfrom
fix/bot-3025-test-runner-lock

Conversation

@bpamiri

@bpamiri bpamiri commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

What / Why

The web test runner (vendor/wheels/tests/runner.cfm) swaps the live application.wheels struct for test configuration — backing it up in application.$$$wheels — and restores it at the end of the run. Two overlapping test requests clobbered each other's backup, which could restore TEST config as the live config until the next reload=true, or error the parent run's restore outright (key [$$$WHEELS] doesn't exist — the #2887 "they fight each other" symptom).

This is the maintainer-endorsed Stage-1 slice of #3025. The issue's titled direction (request-scoped config overlay) was ruled infeasible by the 2026-06-22 adversarial analysis in the thread (blockers B1–B9: adapter baked per model class at init, direct application.wheels.X reads bypassing $get, populate/migrator DDL outside any overlay, browser/parallel tests requiring the global swap).

Approach

  • Exclusive named lock around the swap→run→restore window: wheelsTestRunner_<applicationName>, timeout 1800s (matches the template's requestTimeout; the full suite takes minutes on slow engines), throwOnTimeout=true. Precedent: the swap-under-lock pattern in vendor/wheels/migrator/TenantMigrator.cfc.
  • Re-entrancy guard: ParallelRunner.cfc partitions re-enter runner.cfm via fresh top-level HTTP GETs while the parent request holds the swap. Sub-requests detect the already-applied swap (StructKeyExists(application, "$$$wheels")) and skip both the swap and the shared lock (a unique per-request lock-name suffix), so parallel mode cannot deadlock.
  • Restore moved into a finally block, and only the request that created the backup restores it — an erroring suite no longer leaves test config live. No loops in the finally (Lucee 7 miscompiles local-scoped loops in finally — cross-engine invariant 12); no bare script-context cfabort introduced (invariant 13); the BoxLang $_duplicateWheelsEnv branch stays inside the guarded window.
  • Deleted the orphaned legacy RocketUnit runner twin vendor/wheels/rocketunit_tests/Test.cfc — verified orphaned: nothing loads it; the sole external reference is the scan-exclusion entry in Global.cfc, which is left in place. The active CLI/legacy chain (wheels.Test, $restoreTestRunnerApplicationScope, the EventMethods.cfc hooks) is untouched — it guards a different runner.
  • New spec vendor/wheels/tests/specs/internal/TestRunnerSwapLockSpec.cfc:
    • structural guard (precedent: security/BareCfabortGuardSpec.cfc) that runner.cfm contains the exclusive named-lock acquisition, the re-entrancy detection, and the finally restore;
    • behavioral test: a completed nested run (same shape as a ParallelRunner partition request) must leave the parent's swap intact.
  • Changelog fragment changelog.d/test-runner-swap-lock.fixed.md.

Residual gap — do NOT close #3025 on this PR

This serializes test-vs-test only. A normal application request concurrent with a test run still reads the swapped (test) config for the duration of the run. True isolation is deferred to a separate-application-context design per the issue's 2026-06-22 analysis; the remainder of #3025 should be re-scoped there.

Test evidence

  • TDD fail-first (pre-fix runner.cfm, Lucee 7 + SQLite, wheels.tests.specs.internal): 79 pass / 3 fail / 0 error — all 3 new specs failed, and the run reproduced the exact reported bug: the parent request's restore threw key [$$$WHEELS] doesn't exist at runner.cfm:304 after the nested run clobbered the backup.
  • Full core suite on the branch (Lucee 7 + SQLite, local): 4761 pass / 0 fail / 0 error (27.9s).
  • Adobe CF 2023 + SQLite (Docker, runner.cfm executes on every engine): branch 4745 pass / 3 fail / 13 error vs detached origin/develop 4742 pass / 3 fail / 13 error — failure signatures are byte-identical sets (urlsSpec parser errors, RouteTesterHardeningSpec, JobClassRoundTripSpec, typedColumnDefaultsSpec), i.e. all pre-existing on develop and unrelated to this change; the branch adds exactly the 3 new passing specs. TestRunnerSwapLockSpec: 3 pass / 0 fail / 0 error on Adobe 2023.

Refs #3025

🤖 Generated with Claude Code

…3025)

The web test runner (vendor/wheels/tests/runner.cfm) swaps the LIVE application.wheels
struct for test configuration (backup in application.$$$wheels) and restores it at the
end of the run. Overlapping test requests clobbered each other's backup, which could
restore TEST config as the live config until the next reload=true, or error the parent
run's restore outright (key [$$$WHEELS] doesn't exist).

Stage-1 slice of the issue-3025 analysis (the titled request-scoped overlay was ruled
infeasible in the 2026-06-22 adversarial analysis):

- Wrap the swap->run->restore window in an exclusive named cflock
  ('wheelsTestRunner_<applicationName>', timeout 1800s, throwOnTimeout), following the
  swap-under-lock precedent in migrator/TenantMigrator.cfc.
- Re-entrancy guard: ParallelRunner partition sub-requests re-enter runner.cfm via fresh
  top-level HTTP GETs while the parent holds the swap. They detect the already-applied
  swap (application.$$$wheels exists) and skip BOTH the swap and the shared lock (a
  unique per-request lock-name suffix), so parallel mode cannot deadlock.
- The restore now runs in a finally block, and only the request that created the backup
  restores it — an erroring suite no longer leaves test config live. No loops in the
  finally block (Lucee 7 miscompiles local-scoped loops in finally).
- Delete the orphaned legacy RocketUnit runner twin vendor/wheels/rocketunit_tests/Test.cfc
  (nothing loads it; the Global.cfc scan-exclusion entry stays). The active CLI/legacy
  chain via wheels.Test and $restoreTestRunnerApplicationScope is untouched.

New spec vendor/wheels/tests/specs/internal/TestRunnerSwapLockSpec.cfc: structural guard
for the lock acquisition + re-entrancy detection, and a behavioral nested-run test that
reproduced the backup clobber before the fix.

Residual gap (intentional, refs #3025): this serializes test-vs-test only. A normal
request concurrent with a test run still reads swapped config; true isolation is
deferred to a separate-application-context design.

Refs #3025

Signed-off-by: Peter Amiri <petera@pai.com>

@wheels-bot wheels-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wheels Bot — Reviewer

TL;DR — This PR serializes the web test-runner's live-application.wheels swap→run→restore window under an exclusive named lock, moves the restore into a finally block so only the swap-owner restores it, and deletes an orphaned legacy RocketUnit twin. It is the maintainer-endorsed Stage-1 slice of #3025 and is honest about its residual scope. The change is correct for its stated goal, respects the cross-engine invariants, and ships strong TDD-first coverage. Verdict: comment — one non-blocking observation about the re-entrancy classification, otherwise clean. No blocking findings.

Correctness

The core bug (#2887 key [$$$WHEELS] doesn't exist) is genuinely fixed: only the request that created the backup touches application.$$$wheels (runner.cfm:333if (local.runnerOwnsSwap && StructKeyExists(application, "$$$wheels"))), so overlapping requests can no longer clobber each other's backup, and the finally guarantees restore even when testBox.run() throws. $_setTestboxEnv() sets application.$$$wheels (runner.cfm:36/38), so the runnerOwnsSwap detection at runner.cfm:184 is anchored to the right key. The finally correctly contains no loops (invariant 12), and local.runnerOwnsSwap is set before the lock, so it is always defined when the finally reads it.

Non-blocking observation — re-entrancy classification is decided pre-lock (runner.cfm:184-188):

local.runnerOwnsSwap = !StructKeyExists(application, "$$$wheels");
local.runnerLockSuffix = local.runnerOwnsSwap ? "" : "_sub_" & CreateUUID();
lock name="wheelsTestRunner_#application.applicationName##local.runnerLockSuffix#" ...

runnerOwnsSwap is evaluated before the lock is acquired, and $_setTestboxEnv() (which creates $$$wheels) runs inside it. Two genuinely-independent top-level test runs are therefore serialized only if the second reads line 184 before the first has applied its swap. If a second independent top-level run reads line 184 after the first is mid-suite (so $$$wheels already exists), it is misclassified as a ParallelRunner sub-request, takes a unique _sub_ lock, and runs testBox.run() concurrently with the first against the same DB — the "test-vs-test only" serialization claim has a hole there.

This is not a blocker: it does not reintroduce the fixed backup-clobbering bug (the misclassified request skips both the swap and the restore, so it never touches $$$wheels), it is not a regression (concurrent execution already happened pre-PR with no lock at all), and distinguishing a spawned sub-request from an independent overlap would require threading a run-token the current design intentionally omits. Worth a one-line note in the code comment or the deferred-work tracker so the residual is recorded, not silently assumed away.

Cross-engine

No issues. The try/finally inside lock is standard across engines; the finally body is if + assignments + structDelete with no for loops (invariant 12 satisfied, as the inline comment notes). No bare script-context cfabort is introduced (invariant 13). No obj.map() collisions, no application-scope function members added ($$$wheels is a plain struct), and the lock name is a plain string interpolation. The BoxLang $_duplicateWheelsEnv branch stays inside the guarded window.

Tests

Strong. TestRunnerSwapLockSpec.cfc pairs a structural guard (regex-matched lock acquisition + $$$wheels re-entrancy detection + finally restore, with comment-line skipping in the spirit of Anti-Pattern 14) with a behavioral test that issues a nested top-level GET (the ParallelRunner partition shape) and asserts the parent's $$$wheels backup and transactionMode="none" swap survive. The referenced wheels.tests.specs.internal.parallelRunnerSpec and $testClient() both exist. PR body documents a TDD fail-first run (3/3 new specs failed pre-fix, reproducing the exact runner.cfm:304 restore error). The structural regexes were checked against the shipped lock line and match.

Docs

Changelog fragment changelog.d/test-runner-swap-lock.fixed.md is present and correctly named (<slug>.fixed.md), no direct CHANGELOG.md [Unreleased] edit. It honestly documents the test-vs-test-only scope and the deferred true-isolation work. Deleting the orphaned rocketunit_tests/Test.cfc is well-justified in the body (nothing loads it; the active wheels.Test chain is untouched) and the Global.cfc scan-exclusion entry is correctly left in place.

Commits

Single commit fix(test): serialize web test-runner config swap under a named lock (#3025) — valid type/scope, subject under 100 chars, describes the why. Conforms to commitlint.config.js.

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Wheels Test Results

     31 files    9 884 suites   20m 54s ⏱️
131 873 tests 131 403 ✅ 397 💤 38 ❌ 35 🔥
133 805 runs  133 335 ✅ 397 💤 38 ❌ 35 🔥

For more details on these failures and errors, see this check.

Results for commit 005c8b8.

♻️ This comment has been updated with latest results.

@bpamiri
bpamiri merged commit 9d28432 into develop Aug 5, 2026
16 of 27 checks passed
@bpamiri
bpamiri deleted the fix/bot-3025-test-runner-lock branch August 5, 2026 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test runner: web runner mutates live application.wheels — replace global swap with request-scoped overlay

1 participant