diff --git a/changelog.d/test-runner-swap-lock.fixed.md b/changelog.d/test-runner-swap-lock.fixed.md new file mode 100644 index 0000000000..2d99a95458 --- /dev/null +++ b/changelog.d/test-runner-swap-lock.fixed.md @@ -0,0 +1,11 @@ +- Web test runner (`/wheels/core/tests` and `/wheels/app/tests`): the swap→run→restore window that + temporarily replaces the live `application.wheels` config with test configuration is now serialized + under an exclusive named lock, and the restore runs in a `finally` block. Overlapping test requests + can no longer clobber each other's `application.$$$wheels` backup and leave test config live until + the next `reload=true`, and an erroring suite now restores the original config too. ParallelRunner + partition sub-requests detect the already-applied swap and skip both the swap and the shared lock, + so parallel test mode does not deadlock. Note: this serializes test-vs-test only — a normal request + concurrent with a test run still sees swapped config; true isolation is deferred to a + separate-application-context design (refs [#3025](https://github.com/wheels-dev/wheels/issues/3025)). + Also removes the orphaned legacy RocketUnit runner twin `vendor/wheels/rocketunit_tests/Test.cfc` + (nothing loads it; the active legacy chain via `wheels.Test` is unchanged). diff --git a/vendor/wheels/rocketunit_tests/Test.cfc b/vendor/wheels/rocketunit_tests/Test.cfc deleted file mode 100644 index 213670c91c..0000000000 --- a/vendor/wheels/rocketunit_tests/Test.cfc +++ /dev/null @@ -1,37 +0,0 @@ -component extends="wheels.Test" { - - /* - * Executes once before the test suite runs. - * Populates the test database on reload or if the authors table does not exist. - */ - function beforeAll() { - application.$$$wheels = duplicate(application.wheels); - local.tables = $dbinfo(datasource = application.wheels.dataSourceName, type = "tables"); - local.tableList = ValueList(local.tables.table_name); - local.populate = StructKeyExists(url, "populate") ? url.populate : true; - if (local.populate || !FindNoCase("c_o_r_e_authors", local.tableList)) { - include "populate.cfm"; - } - } - - /* - * Executes before every test case if called from the package via super.superSetup(). - */ - function setup() { - } - - /* - * Executes after every test case if called from the package via super.superTeardown(). - */ - function teardown() { - } - - /* - * Executes once after the test suite runs. - */ - function afterAll() { - application.wheels = application.$$$wheels; - structDelete(application, "$$$wheels"); - } - -} diff --git a/vendor/wheels/tests/runner.cfm b/vendor/wheels/tests/runner.cfm index 8e88cc8a9a..13cccdb103 100644 --- a/vendor/wheels/tests/runner.cfm +++ b/vendor/wheels/tests/runner.cfm @@ -166,143 +166,176 @@ bundlesDiscovered = local.bundlesDiscovered ) - variables.$_setTestboxEnv() - if (!structKeyExists(url, "format") || url.format eq "html") { - result = testBox.run( - reporter = "wheels.wheelstest.system.reports.JSONReporter" - ); - DeJsonResult = DeserializeJSON(result); - - if (DeJsonResult.totalFail > 0 || DeJsonResult.totalError > 0) { - application.wo.$header(statuscode=417); - } else { - application.wo.$header(statuscode=200); - } - } - else if(url.format eq "json"){ - result = testBox.run( - reporter = "wheels.wheelstest.system.reports.JSONReporter" - ); - // `$header()` / `$content()` short-circuit when the servlet response is - // already committed (Adobe CF 2023/2025 commits mid-`testBox.run()` once - // any test output flushes the buffer). The status-code header is the - // signal the CI parser keys on, so best-effort is the right contract — - // a committed response keeps whatever statuscode the engine already - // wrote, and the JSON body still appends below. - application.wo.$content(type="application/json"); - application.wo.$header(name="Access-Control-Allow-Origin", value="*"); - DeJsonResult = DeserializeJSON(result); - if (DeJsonResult.totalFail > 0 || DeJsonResult.totalError > 0) { - if(!structKeyExists(url, "cli") || !url.cli){ - application.wo.$header(statuscode=417); + // ── Concurrency guard (issue #3025) ───────────────────────────────── + // The swap→run→restore window below mutates the LIVE application.wheels + // struct ($_setTestboxEnv backs it up in application.$$$wheels and swaps + // in test config; the finally block swaps it back). Two overlapping test + // requests used to clobber each other's backup, which could restore TEST + // config as the live config until the next reload=true. Serialize the + // whole window under an exclusive named lock (precedent: + // migrator/TenantMigrator.cfc::$runForTenant). + // + // Re-entrancy: ParallelRunner partitions re-enter this template via + // fresh top-level HTTP GETs while the parent request holds the swap and + // the lock. Those sub-requests detect the already-applied swap + // (application.$$$wheels exists) and skip BOTH the swap and the shared + // lock — contending on the parent's lock would deadlock parallel mode. + // A unique per-request suffix turns their lock into a no-op. + local.runnerOwnsSwap = !StructKeyExists(application, "$$$wheels"); + local.runnerLockSuffix = local.runnerOwnsSwap ? "" : "_sub_" & CreateUUID(); + // Timeout must exceed the worst-case full-suite duration on the slowest + // engine; matches the requestTimeout at the top of this template. + lock name="wheelsTestRunner_#application.applicationName##local.runnerLockSuffix#" type="exclusive" timeout="1800" throwontimeout="true" { + try { + if (local.runnerOwnsSwap) { + variables.$_setTestboxEnv(); } - } else { - application.wo.$header(statuscode=200); - } - // Check if 'only' parameter is provided in the URL - if (structKeyExists(url, "only") && url.only eq "failure,error") { - allBundles = DeJsonResult.bundleStats; - if(DeJsonResult.totalFail > 0 || DeJsonResult.totalError > 0){ - - // Filter test results - filteredBundles = []; - - for (bundle in DeJsonResult.bundleStats) { - if (bundle.totalError > 0 || bundle.totalFail > 0) { - filteredSuites = []; - - for (suite in bundle.suiteStats) { - if (suite.totalError > 0 || suite.totalFail > 0) { - filteredSpecs = []; - - for (spec in suite.specStats) { - if (spec.status eq "Error" || spec.status eq "Failed") { - arrayAppend(filteredSpecs, spec); + if (!structKeyExists(url, "format") || url.format eq "html") { + result = testBox.run( + reporter = "wheels.wheelstest.system.reports.JSONReporter" + ); + DeJsonResult = DeserializeJSON(result); + + if (DeJsonResult.totalFail > 0 || DeJsonResult.totalError > 0) { + application.wo.$header(statuscode=417); + } else { + application.wo.$header(statuscode=200); + } + } + else if(url.format eq "json"){ + result = testBox.run( + reporter = "wheels.wheelstest.system.reports.JSONReporter" + ); + // `$header()` / `$content()` short-circuit when the servlet response is + // already committed (Adobe CF 2023/2025 commits mid-`testBox.run()` once + // any test output flushes the buffer). The status-code header is the + // signal the CI parser keys on, so best-effort is the right contract — + // a committed response keeps whatever statuscode the engine already + // wrote, and the JSON body still appends below. + application.wo.$content(type="application/json"); + application.wo.$header(name="Access-Control-Allow-Origin", value="*"); + DeJsonResult = DeserializeJSON(result); + if (DeJsonResult.totalFail > 0 || DeJsonResult.totalError > 0) { + if(!structKeyExists(url, "cli") || !url.cli){ + application.wo.$header(statuscode=417); + } + } else { + application.wo.$header(statuscode=200); + } + // Check if 'only' parameter is provided in the URL + if (structKeyExists(url, "only") && url.only eq "failure,error") { + allBundles = DeJsonResult.bundleStats; + if(DeJsonResult.totalFail > 0 || DeJsonResult.totalError > 0){ + + // Filter test results + filteredBundles = []; + + for (bundle in DeJsonResult.bundleStats) { + if (bundle.totalError > 0 || bundle.totalFail > 0) { + filteredSuites = []; + + for (suite in bundle.suiteStats) { + if (suite.totalError > 0 || suite.totalFail > 0) { + filteredSpecs = []; + + for (spec in suite.specStats) { + if (spec.status eq "Error" || spec.status eq "Failed") { + arrayAppend(filteredSpecs, spec); + } + } + + if (arrayLen(filteredSpecs) > 0) { + suite.specStats = filteredSpecs; + arrayAppend(filteredSuites, suite); + } } } - if (arrayLen(filteredSpecs) > 0) { - suite.specStats = filteredSpecs; - arrayAppend(filteredSuites, suite); + if (arrayLen(filteredSuites) > 0) { + bundle.suiteStats = filteredSuites; + arrayAppend(filteredBundles, bundle); } } } - if (arrayLen(filteredSuites) > 0) { - bundle.suiteStats = filteredSuites; - arrayAppend(filteredBundles, bundle); - } - } - } + DeJsonResult.bundleStats = filteredBundles; + // Update the result with filtered data - DeJsonResult.bundleStats = filteredBundles; - // Update the result with filtered data - - // Build lookup of filtered bundles by name for safe access - filteredBundleMap = {}; - for (fb in filteredBundles) { - filteredBundleMap[fb.name] = fb; - } + // Build lookup of filtered bundles by name for safe access + filteredBundleMap = {}; + for (fb in filteredBundles) { + filteredBundleMap[fb.name] = fb; + } - for(bundle in allBundles){ - writeOutput("Bundle: #bundle.name##Chr(13)##Chr(10)#") - writeOutput("CFML Engine: #DeJsonResult.CFMLEngine# #DeJsonResult.CFMLEngineVersion##Chr(13)##Chr(10)#") - writeOutput("Duration: #bundle.totalDuration#ms#Chr(13)##Chr(10)#") - writeOutput("Labels: #ArrayToList(DeJsonResult.labels, ', ')##Chr(13)##Chr(10)#") - writeOutput("╔═══════════════════════════════════════════════════════════╗#Chr(13)##Chr(10)#║ Suites ║ Specs ║ Passed ║ Failed ║ Errored ║ Skipped ║#Chr(13)##Chr(10)#╠═══════════════════════════════════════════════════════════╣#Chr(13)##Chr(10)#║ #NumberFormat(bundle.totalSuites,'999')# ║ #NumberFormat(bundle.totalSpecs,'999')# ║ #NumberFormat(bundle.totalPass,'999')# ║ #NumberFormat(bundle.totalFail,'999')# ║ #NumberFormat(bundle.totalError,'999')# ║ #NumberFormat(bundle.totalSkipped,'999')# ║#Chr(13)##Chr(10)#╚═══════════════════════════════════════════════════════════╝#Chr(13)##Chr(10)##Chr(13)##Chr(10)#") - if(bundle.totalFail > 0 || bundle.totalError > 0){ - if (structKeyExists(filteredBundleMap, bundle.name)) { - for(suite in filteredBundleMap[bundle.name].suiteStats){ - writeOutput("Suite with Error or Failure: #suite.name##Chr(13)##Chr(10)##Chr(13)##Chr(10)#") - for(spec in suite.specStats){ - writeOutput(" Spec Name: #spec.name##Chr(13)##Chr(10)#") - writeOutput(" Error Message: #spec.failMessage##Chr(13)##Chr(10)#") - writeOutput(" Error Detail: #spec.failDetail##Chr(13)##Chr(10)##Chr(13)##Chr(10)##Chr(13)##Chr(10)#") + for(bundle in allBundles){ + writeOutput("Bundle: #bundle.name##Chr(13)##Chr(10)#") + writeOutput("CFML Engine: #DeJsonResult.CFMLEngine# #DeJsonResult.CFMLEngineVersion##Chr(13)##Chr(10)#") + writeOutput("Duration: #bundle.totalDuration#ms#Chr(13)##Chr(10)#") + writeOutput("Labels: #ArrayToList(DeJsonResult.labels, ', ')##Chr(13)##Chr(10)#") + writeOutput("╔═══════════════════════════════════════════════════════════╗#Chr(13)##Chr(10)#║ Suites ║ Specs ║ Passed ║ Failed ║ Errored ║ Skipped ║#Chr(13)##Chr(10)#╠═══════════════════════════════════════════════════════════╣#Chr(13)##Chr(10)#║ #NumberFormat(bundle.totalSuites,'999')# ║ #NumberFormat(bundle.totalSpecs,'999')# ║ #NumberFormat(bundle.totalPass,'999')# ║ #NumberFormat(bundle.totalFail,'999')# ║ #NumberFormat(bundle.totalError,'999')# ║ #NumberFormat(bundle.totalSkipped,'999')# ║#Chr(13)##Chr(10)#╚═══════════════════════════════════════════════════════════╝#Chr(13)##Chr(10)##Chr(13)##Chr(10)#") + if(bundle.totalFail > 0 || bundle.totalError > 0){ + if (structKeyExists(filteredBundleMap, bundle.name)) { + for(suite in filteredBundleMap[bundle.name].suiteStats){ + writeOutput("Suite with Error or Failure: #suite.name##Chr(13)##Chr(10)##Chr(13)##Chr(10)#") + for(spec in suite.specStats){ + writeOutput(" Spec Name: #spec.name##Chr(13)##Chr(10)#") + writeOutput(" Error Message: #spec.failMessage##Chr(13)##Chr(10)#") + writeOutput(" Error Detail: #spec.failDetail##Chr(13)##Chr(10)##Chr(13)##Chr(10)##Chr(13)##Chr(10)#") + } + } } } + writeOutput("#Chr(13)##Chr(10)##Chr(13)##Chr(10)##Chr(13)##Chr(10)#") } - } - writeOutput("#Chr(13)##Chr(10)##Chr(13)##Chr(10)##Chr(13)##Chr(10)#") - } - }else{ - for(bundle in DeJsonResult.bundleStats){ - writeOutput("Bundle: #bundle.name##Chr(13)##Chr(10)#") - writeOutput("CFML Engine: #DeJsonResult.CFMLEngine# #DeJsonResult.CFMLEngineVersion##Chr(13)##Chr(10)#") - writeOutput("Duration: #bundle.totalDuration#ms#Chr(13)##Chr(10)#") - writeOutput("Labels: #ArrayToList(DeJsonResult.labels, ', ')##Chr(13)##Chr(10)#") - writeOutput("╔═══════════════════════════════════════════════════════════╗#Chr(13)##Chr(10)#║ Suites ║ Specs ║ Passed ║ Failed ║ Errored ║ Skipped ║#Chr(13)##Chr(10)#╠═══════════════════════════════════════════════════════════╣#Chr(13)##Chr(10)#║ #NumberFormat(bundle.totalSuites,'999')# ║ #NumberFormat(bundle.totalSpecs,'999')# ║ #NumberFormat(bundle.totalPass,'999')# ║ #NumberFormat(bundle.totalFail,'999')# ║ #NumberFormat(bundle.totalError,'999')# ║ #NumberFormat(bundle.totalSkipped,'999')# ║#Chr(13)##Chr(10)#╚═══════════════════════════════════════════════════════════╝#Chr(13)##Chr(10)##Chr(13)##Chr(10)##Chr(13)##Chr(10)#") + }else{ + for(bundle in DeJsonResult.bundleStats){ + writeOutput("Bundle: #bundle.name##Chr(13)##Chr(10)#") + writeOutput("CFML Engine: #DeJsonResult.CFMLEngine# #DeJsonResult.CFMLEngineVersion##Chr(13)##Chr(10)#") + writeOutput("Duration: #bundle.totalDuration#ms#Chr(13)##Chr(10)#") + writeOutput("Labels: #ArrayToList(DeJsonResult.labels, ', ')##Chr(13)##Chr(10)#") + writeOutput("╔═══════════════════════════════════════════════════════════╗#Chr(13)##Chr(10)#║ Suites ║ Specs ║ Passed ║ Failed ║ Errored ║ Skipped ║#Chr(13)##Chr(10)#╠═══════════════════════════════════════════════════════════╣#Chr(13)##Chr(10)#║ #NumberFormat(bundle.totalSuites,'999')# ║ #NumberFormat(bundle.totalSpecs,'999')# ║ #NumberFormat(bundle.totalPass,'999')# ║ #NumberFormat(bundle.totalFail,'999')# ║ #NumberFormat(bundle.totalError,'999')# ║ #NumberFormat(bundle.totalSkipped,'999')# ║#Chr(13)##Chr(10)#╚═══════════════════════════════════════════════════════════╝#Chr(13)##Chr(10)##Chr(13)##Chr(10)##Chr(13)##Chr(10)#") + } + } + }else{ + // Thread the resolved-scope facts (and any warnings) into the JSON + // payload so a rejected directory or a 0-bundle discovery is + // detectable instead of masquerading as a green run (issue #3083). + writeOutput(local.scopeResolver.injectScopeMetadata( + resultJson = result, + scope = local.testScope, + bundlesDiscovered = local.bundlesDiscovered, + warnings = local.scopeWarnings + )) } } - }else{ - // Thread the resolved-scope facts (and any warnings) into the JSON - // payload so a rejected directory or a 0-bundle discovery is - // detectable instead of masquerading as a green run (issue #3083). - writeOutput(local.scopeResolver.injectScopeMetadata( - resultJson = result, - scope = local.testScope, - bundlesDiscovered = local.bundlesDiscovered, - warnings = local.scopeWarnings - )) + else if (url.format eq "txt") { + result = testBox.run( + reporter = "wheels.wheelstest.system.reports.TextReporter" + ) + application.wo.$content(type="text/plain"); + writeOutput(result) + } + else if(url.format eq "junit"){ + result = testBox.run( + reporter = "wheels.wheelstest.system.reports.ANTJUnitReporter" + ) + application.wo.$content(type="text/xml"); + writeOutput(result) + } + } finally { + // Reset the original environment. Only the request that created + // the backup restores it — sub-requests never touch the live + // config — and the restore now also runs when the suite errors + // out (previously an exception left test config live until the + // next reload). No loops in this finally block (Lucee 7 + // miscompiles local-scoped loops in finally — invariant 12). + if (local.runnerOwnsSwap && StructKeyExists(application, "$$$wheels")) { + application.wheels = application.$$$wheels; + structDelete(application, "$$$wheels"); + } } } - else if (url.format eq "txt") { - result = testBox.run( - reporter = "wheels.wheelstest.system.reports.TextReporter" - ) - application.wo.$content(type="text/plain"); - writeOutput(result) - } - else if(url.format eq "junit"){ - result = testBox.run( - reporter = "wheels.wheelstest.system.reports.ANTJUnitReporter" - ) - application.wo.$content(type="text/xml"); - writeOutput(result) - } - // reset the original environment - application.wheels = application.$$$wheels - structDelete(application, "$$$wheels") if(!structKeyExists(url, "format") || url.format eq "html"){ // Use our html template type = "Core"; diff --git a/vendor/wheels/tests/specs/internal/TestRunnerSwapLockSpec.cfc b/vendor/wheels/tests/specs/internal/TestRunnerSwapLockSpec.cfc new file mode 100644 index 0000000000..da0000241b --- /dev/null +++ b/vendor/wheels/tests/specs/internal/TestRunnerSwapLockSpec.cfc @@ -0,0 +1,122 @@ +/** + * Guards for issue #3025 (Stage-1 slice): 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 when the suite completes. Two overlapping test requests used + * to clobber each other's backup, which could restore TEST config as the + * live config until the next reload=true (issue #2887 "they fight each + * other"). The fix serializes the swap->run->restore window under an + * exclusive named lock, with a re-entrancy guard so ParallelRunner + * partition sub-requests (fresh top-level HTTP GETs issued while the parent + * request holds the swap) skip BOTH the swap and the shared lock instead of + * deadlocking parallel mode. + * + * Two kinds of coverage: + * + * 1. Structural (precedent: security/BareCfabortGuardSpec.cfc) — runner.cfm + * must contain the exclusive named-lock acquisition and the + * already-swapped detection. The lock itself cannot be observed from + * inside the suite (this spec executes while the lock is held), so the + * source scan is the practical gate. + * + * 2. Behavioral — a completed nested run (same shape as a ParallelRunner + * partition request) must leave the parent request's swap fully intact: + * application.$$$wheels still present and application.wheels still + * pointing at test config. Before the fix the nested run overwrote the + * parent's backup with test config, restored it as "live", and deleted + * the backup key — erroring the parent's own restore. + */ +component extends="wheels.WheelsTest" { + + function run() { + + describe("Web test-runner swap serialization (issue ##3025)", () => { + + it("runner.cfm acquires an exclusive named lock around the config swap window", () => { + var source = FileRead(ExpandPath("/wheels/tests/runner.cfm")); + var fileLines = ListToArray(source, Chr(10), true); + var foundLock = false; + var foundGuard = false; + for (var rawLine in fileLines) { + var trimmed = Trim(Replace(rawLine, Chr(13), "", "all")); + // Skip comment-only lines so a commented-out lock can + // never satisfy this guard (Anti-Pattern 14 spirit). + if (Left(trimmed, 2) == "//" || Left(trimmed, 1) == "*" || Left(trimmed, 2) == "/*") { + continue; + } + // The exclusive named-lock acquisition on the shared + // runner lock name. + if ( + REFindNoCase("(^|[\s;{}])lock\s+[^{]*name\s*=", trimmed) + && FindNoCase("wheelsTestRunner_", trimmed) + && REFindNoCase("type\s*=\s*[""']exclusive[""']", trimmed) + && REFindNoCase("throwontimeout", trimmed) + ) { + foundLock = true; + } + // The re-entrancy detection: sub-requests recognize an + // already-applied swap via the backup key. + if (FindNoCase("StructKeyExists(application, ""$$$wheels"")", trimmed)) { + foundGuard = true; + } + } + expect(foundLock).toBeTrue( + "runner.cfm must wrap the config swap window in an exclusive named lock ('wheelsTestRunner_...', throwOnTimeout) — issue ##3025" + ); + expect(foundGuard).toBeTrue( + "runner.cfm must detect an already-applied swap via StructKeyExists(application, '$$$wheels') so ParallelRunner sub-requests skip the swap and the shared lock" + ); + }); + + it("runner.cfm restores the original config in a finally block", () => { + var source = FileRead(ExpandPath("/wheels/tests/runner.cfm")); + expect(Find("finally", source) > 0).toBeTrue( + "runner.cfm must restore application.wheels from the backup inside a finally block so an erroring suite can no longer leave test config live" + ); + expect(Find("application.wheels = application.$$$wheels", source) > 0).toBeTrue( + "runner.cfm must restore application.wheels from application.$$$wheels" + ); + }); + + it("a completed nested run leaves the parent request's swap intact", () => { + // This spec itself executes inside the swap window, so the + // backup key must be present right now. + expect(StructKeyExists(application, "$$$wheels")).toBeTrue( + "precondition: the suite is running inside the swap window" + ); + + // Same shape as a ParallelRunner partition request: a fresh + // top-level GET back into the runner while this request holds + // the swap. Point directory= at a single bundle file so the + // nested run discovers 0 bundles and completes green in + // milliseconds (the issue-3083 '0-bundle' shape), and pass + // populate=false so pre-fix engines do not repopulate the + // database mid-suite. + var requestParams = { + format = "json", + cli = "true", + populate = "false", + directory = "wheels.tests.specs.internal.parallelRunnerSpec" + }; + if (StructKeyExists(url, "db")) { + requestParams.db = url.db; + } + var tc = $testClient().get(path = "/wheels/core/tests", params = requestParams); + expect(tc.statusCode()).toBe(200, "the nested runner request must complete green"); + + // The nested run must NOT have deleted the parent's backup... + expect(StructKeyExists(application, "$$$wheels")).toBeTrue( + "a completed nested run must not delete the parent's application.$$$wheels backup — only the request that created the swap restores it (issue ##3025)" + ); + // ...and must NOT have restored live config over the + // in-progress parent run (transactionMode='none' is one of + // the swapped-in test settings). + expect(application.wheels.transactionMode).toBe( + "none", + "a completed nested run must not restore the live config while the parent run is still executing" + ); + }); + + }); + } +}