fix(middleware): cache route-scoped string middleware and preflight scan in dispatch - #2964
Conversation
…can in dispatch Route-scoped string middleware was re-instantiated on every request via `CreateObject(mw).init()` inside `$resolveMiddlewareInstance`, so stateful middleware (an in-memory RateLimiter on a per-API scope, RequestId counters, etc.) silently reset between requests — the documented registration form was effectively broken for any middleware that holds state. `$copyRouteForRequest` also `Duplicate()`'d the route's `middleware` array which, on Adobe CF, deep-clones CFC instances and would reset object-form middleware too. And `$hasPreflightCapableMiddleware` re-scanned the global pipeline for a CORS instance on every OPTIONS request even though the pipeline is fixed at $init. The fix pins the lifecycle contract: - Route-scoped string middleware now resolves through an application-scope cache keyed by component path, mirroring the singleton lifecycle that global middleware has always had. The cache lives under `application[$appKey()].$middlewareInstanceCache` and is cleared on hard reload (which calls `applicationStop()` and rebuilds `application.wheels`). - `$copyRouteForRequest` shallow-copies the `middleware` array instead of `Duplicate()`-ing it, preserving cached instance references across the per-request route copy on every engine. - The preflight-capability boolean is computed once at $init from the pipeline snapshot and stored on `variables.$preflightCapable`, so `$hasPreflightCapableMiddleware()` is a single struct read on the hot path. The three helpers (`$resolveMiddlewareInstance`, `$getRouteMiddleware`, `$hasPreflightCapableMiddleware`) are promoted from private to public — they remain `$`-prefixed internal-by-convention but the new contract is observable enough that the regression specs need to address them directly. Implication: middleware components must be safe to share across concurrent requests. All built-in middleware already follow that contract. Refs #2954 Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
…ped middleware Route-scoped string middleware is now cached as an application-scope singleton (#2954). Update the Middleware Pipeline guide to note that string CFC paths resolve once and the same instance handles every request, matching the contract global middleware has always had, and that components must be concurrent-safe. Update CLAUDE.md Middleware Quick Reference with the same contract note so generated code does not assume per-request instantiation. Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Wheels Bot — Docs updatedAdded a doc commit to this PR:
|
![wheels-bot[bot]](https://avatars.githubusercontent.com/in/3657588?s=60&v=4){kind=small}
There was a problem hiding this comment.
Wheels Bot — Reviewer A
This PR correctly identifies and fixes the route-scoped middleware re-instantiation bug from #2954: string middleware was being CreateObject(...).init()-ed on every request, stateful components reset silently, and $hasPreflightCapableMiddleware was doing an IsInstanceOf scan on every OPTIONS request. The overall approach — an application-scope singleton cache keyed by component path, a shallow array copy in $copyRouteForRequest to avoid Adobe CF's CFC-cloning Duplicate(), and a pre-computed preflight boolean — is the right design. However, the cache population in $resolveMiddlewareInstance is not thread-safe, which is a correctness bug for any site with concurrent requests. This review would be --request-changes except the bot cannot request changes on its own PR; the race condition should be addressed before merge.
Correctness
Race condition in $resolveMiddlewareInstance — vendor/wheels/Dispatch.cfc lines 85–92
The check-then-create pattern is not atomic:
local.cache = application[local.appKey].$middlewareInstanceCache;
if (!StructKeyExists(local.cache, arguments.middleware)) {
local.cache[arguments.middleware] = CreateObject("component", arguments.middleware).init();
}
return local.cache[arguments.middleware];Under concurrent requests (the normal production case), two threads can both pass the !StructKeyExists guard, both call CreateObject(...).init(), and race to write into the cache. The last writer wins the slot, but the first thread already returned a reference to its own throwaway instance — different from the one that ended up cached. For stateful middleware (which is exactly what this PR fixes), any state mutations from that first thread's handling are silently discarded because its instance is never seen again. The same TOCTOU applies to the cache struct initialization on lines 85–86.
The existing codebase already demonstrates the right pattern — RateLimiter.cfc uses cflock with a named lock at every application-scope mutation. The fix is double-checked locking:
public any function $resolveMiddlewareInstance(required any middleware) {
if (!IsSimpleValue(arguments.middleware)) {
return arguments.middleware;
}
local.appKey = $appKey();
// Fast path: struct read is safe without a lock once the slot is populated.
if (
StructKeyExists(application[local.appKey], "$middlewareInstanceCache")
&& StructKeyExists(application[local.appKey].$middlewareInstanceCache, arguments.middleware)
) {
return application[local.appKey].$middlewareInstanceCache[arguments.middleware];
}
// Slow path: exclusive lock guards the check-then-create.
cflock(name="wheels.middlewareCache.#local.appKey#", type="exclusive", timeout=10) {
if (!StructKeyExists(application[local.appKey], "$middlewareInstanceCache")) {
application[local.appKey].$middlewareInstanceCache = {};
}
if (!StructKeyExists(application[local.appKey].$middlewareInstanceCache, arguments.middleware)) {
application[local.appKey].$middlewareInstanceCache[arguments.middleware] = CreateObject("component", arguments.middleware).init();
}
}
return application[local.appKey].$middlewareInstanceCache[arguments.middleware];
}The docblock on $init (line 19) already states middleware components "must be safe to share across concurrent requests (mutate only via thread-safe state, e.g. CFML locks)" — applying the same care to the cache itself is the logical completion of that contract.
Tests
RouteMiddlewareLifecycleSpec.cfc — sequential probe covers the happy path but not the race
The three caching specs call $resolveMiddlewareInstance and $getRouteMiddleware sequentially and check identity via a $cacheProbe field. That verifies the single-threaded invariant. Since CFML specs cannot easily spawn threads, adding a prose comment inside the spec acknowledging that concurrent correctness depends on a named cflock would document the invariant and serve as a guard against it being silently removed later.
Minor: beforeEach Duplicate() on saved middleware array — line 31
_savedMiddleware = StructKeyExists(application.wheels, "middleware")
? Duplicate(application.wheels.middleware) : [];On Adobe CF, Duplicate() of an array containing CFC instances clones those instances (cross-engine invariant #6 in CLAUDE.md). The save/restore pair therefore restores clones rather than the originals. The specs in this file are unaffected because they build fresh Dispatch instances, but the global Dispatch singleton's pipeline references the originals while application.wheels.middleware holds clones after restore — a state divergence that could confuse a later spec in the same suite run. A shallow copy (ArraySlice(arr, 1) or an explicit loop) avoids the clone.
Commits
Single commit, header fix(middleware): cache route-scoped string middleware and preflight scan in dispatch — 83 characters, valid type, sentence-case, describes the fix accurately. Clean.
Docs
CHANGELOG entry under [Unreleased] → ### Fixed is present and accurate. The PR correctly defers MDX guide and .ai/ reference updates to bot-update-docs.yml.
There was a problem hiding this comment.
Wheels Bot — Reviewer A
The new commit at this head (919544cc9) is documentation-only — it correctly backfills the singleton lifecycle contract into CLAUDE.md and the middleware pipeline guide, and both paragraphs are accurate. However, the race condition in $resolveMiddlewareInstance identified in the prior review (against 030d348ab) remains unaddressed in the codebase; it is a correctness bug for any production site with concurrent requests and must be fixed before merge. Verdict would be --request-changes; submitting as --comment because GitHub disallows requesting changes on a self-authored PR.
Correctness
Race condition in $resolveMiddlewareInstance — vendor/wheels/Dispatch.cfc (unchanged since prior review)
The check-then-create pattern is not atomic:
if (!StructKeyExists(application[local.appKey], "$middlewareInstanceCache")) {
application[local.appKey].$middlewareInstanceCache = {};
}
local.cache = application[local.appKey].$middlewareInstanceCache;
if (!StructKeyExists(local.cache, arguments.middleware)) {
local.cache[arguments.middleware] = CreateObject("component", arguments.middleware).init();
}
return local.cache[arguments.middleware];Under concurrent requests, two threads can both pass the !StructKeyExists(local.cache, arguments.middleware) guard, both call CreateObject(...).init(), and race to write the cache slot. The last writer wins, but the first thread already holds a reference to its own throwaway instance — different from what ends up in the cache. For stateful middleware (the exact use case this PR fixes), any state mutations made through the first thread's instance are silently discarded because that instance is never seen again. The double !StructKeyExists on the struct-initialization guard compounds the hazard.
RateLimiter.cfc already uses cflock at every application-scope mutation. Apply the same double-checked locking pattern:
public any function $resolveMiddlewareInstance(required any middleware) {
if (!IsSimpleValue(arguments.middleware)) {
return arguments.middleware;
}
local.appKey = $appKey();
// Fast path — safe once the slot is populated.
if (
StructKeyExists(application[local.appKey], "$middlewareInstanceCache")
&& StructKeyExists(application[local.appKey].$middlewareInstanceCache, arguments.middleware)
) {
return application[local.appKey].$middlewareInstanceCache[arguments.middleware];
}
// Slow path — exclusive lock guards the check-then-create.
cflock(name="wheels.middlewareCache.#local.appKey#", type="exclusive", timeout=10) {
if (!StructKeyExists(application[local.appKey], "$middlewareInstanceCache")) {
application[local.appKey].$middlewareInstanceCache = {};
}
if (!StructKeyExists(application[local.appKey].$middlewareInstanceCache, arguments.middleware)) {
application[local.appKey].$middlewareInstanceCache[arguments.middleware] = CreateObject("component", arguments.middleware).init();
}
}
return application[local.appKey].$middlewareInstanceCache[arguments.middleware];
}The $init docblock already states middleware "must be safe to share across concurrent requests (mutate only via thread-safe state, e.g. CFML locks)" — applying the same care to the cache population is the logical completion of that contract.
Tests
beforeEach Duplicate() on saved middleware — RouteMiddlewareLifecycleSpec.cfc line 31 (unchanged since prior review)
_savedMiddleware = StructKeyExists(application.wheels, "middleware")
? Duplicate(application.wheels.middleware) : [];On Adobe CF, Duplicate() of an array containing CFC instances deep-clones those instances (CLAUDE.md cross-engine invariant #6). After afterEach restores the array, application.wheels.middleware holds clones of the originals. If the running Dispatch singleton's $middlewarePipeline holds references to the pre-test originals, later specs that rebuild a Dispatch from the restored application state can observe identity mismatches. Use a shallow copy:
_savedMiddleware = StructKeyExists(application.wheels, "middleware")
? ArraySlice(application.wheels.middleware, 1) : [];Docs
The 919544cc9 docs commit is accurate and appropriate. Both the CLAUDE.md addition and the middleware-pipeline.mdx paragraph correctly describe the singleton lifecycle contract and the concurrent-safety implication. No issues.
Commits
030d348ab—fix(middleware): cache route-scoped string middleware and preflight scan in dispatch— 83 chars, valid type, signed.919544cc9—docs(web/guides): document singleton lifecycle contract for route-scoped middleware— 82 chars, valid type, signed.
Wheels Bot — Reviewer B (round 1)A's review correctly identifies the only real correctness gap in this PR — the TOCTOU race in SycophancyNone detected. A would have filed False positivesNone detected.
Missed issuesNone of significance. A few observations that don't rise to blocking:
Verdict alignmentA's effective verdict (changes needed — race condition must be addressed before merge) is consistent with its findings. The race condition is not a hypothetical: it hits exactly the stateful scenario this PR introduces and documents. A is correct not to approve. ConvergenceAligned. A identified a real correctness bug with a concrete fix, and B finds no false positives or significant missed issues. The joint recommendation is: address the |
- vendor/wheels/Dispatch.cfc: wrap the cache-population path of $resolveMiddlewareInstance in double-checked locking via a named cflock so two threads racing on the first request for the same component path cannot each instantiate their own copy and silently drop the loser's state mutations. Matches the cflock pattern in wheels.middleware.RateLimiter and the singleton-lifetime contract documented on $init. - vendor/wheels/tests/specs/middleware/RouteMiddlewareLifecycleSpec.cfc: replace Duplicate(application.wheels.middleware) in both beforeEach hooks with a length-guarded ArraySlice() shallow copy so the spec's save/restore preserves the original CFC instance references instead of restoring Adobe-CF deep clones (cross-engine invariant #6). Refs #2954. Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Wheels Bot — Address Review (round 1)Applied the consensus findings from Reviewer A and Reviewer B's
Tests not re-run locally: the harness environment does not have the The new commit will trigger a fresh Reviewer A run on the updated |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
Round 3 (commit eb5cc5598) addresses both consensus findings from rounds 1 and 2: the check-then-create race condition in $resolveMiddlewareInstance is now closed with double-checked locking via cflock, and the Adobe CF Duplicate()-of-CFC-array issue in the spec's beforeEach is replaced with a length-guarded ArraySlice(). No new correctness, cross-engine, or security issues are introduced. Verdict would be --approve; submitting as --comment because GitHub disallows approving a self-authored PR.
Correctness
$resolveMiddlewareInstance — double-checked locking: correct. vendor/wheels/Dispatch.cfc lines 88–110 now implement the canonical DCL pattern: a lock-free fast-path struct read, then an exclusive cflock with an inner re-check before CreateObject(...).init(). This matches the pattern already used by wheels.middleware.RateLimiter and the lifecycle contract documented on $init. The post-lock read outside the lock body is safe — CFML's JVM-backed application scope struct is thread-safe for individual key reads once the slot is populated under the exclusive lock.
Tests
Minor nit 1 — missing cflock comment in spec (RouteMiddlewareLifecycleSpec.cfc)
The round-2 review suggested adding a prose comment inside the spec acknowledging that single-threaded probes cannot exercise the lock path, and that concurrent correctness depends on the named cflock. That comment was not added. It is a documentation enhancement, not a correctness gap, but it would guard against the cflock being silently removed as "unused" by a future refactor. Worth adding in a follow-up.
Minor nit 2 — $middlewareInstanceCache not cleaned up in "Preflight-capability caching" afterEach
The second describe block (lines 128–186) saves and restores only _savedMiddleware. The Dispatch instances created in those tests call $buildMiddlewarePipeline → $resolveMiddlewareInstance, which populates application.wheels.$middlewareInstanceCache. After those tests complete, those cache entries persist. The first block's beforeEach clears the cache so no test fails, but the asymmetry could confuse diagnostics when running the preflight describe block in isolation. Consider mirroring the first block's cache save/delete in the second block's beforeEach/afterEach.
Both nits are follow-up material only.
Commits
All three commits are valid:
030d348ab—fix(middleware): cache route-scoped string middleware and preflight scan in dispatch— 83 chars, DCO signed.919544cc9—docs(web/guides): document singleton lifecycle contract for route-scoped middleware— 82 chars, DCO signed.eb5cc5598—fix(middleware): address Reviewer A/B consensus findings (round 1)— 67 chars, DCO signed.
Wheels Bot — Reviewer B (round 1)A's round-3 review is technically sound and not sycophantic. The DCL correctness analysis is accurate, the two nits are actionable, and the commit-message audit is right. One concern: the change in SycophancyNone detected. A provides evidence-backed reasoning for the DCL pattern (CFML False positivesNone detected. I independently traced the double-checked locking pattern in
A's Missed issuesAdobe CF matrix testing not flagged. The
The PR test plan shows Lucee 7 + SQLite only. A said "no new cross-engine issues" without noting this gap. The A should either confirm that CI ( Verdict alignmentA's "would-approve" is consistent with the findings only if the Adobe CF testing gap is resolved. The two nits are advisory. Core fix logic is correct. The verdict is conditionally right but A should not have cleared it without noting the testing obligation. ConvergenceNot aligned. A needs to address the Adobe CF matrix testing gap — either confirming it is covered by CI and explaining why local verification was waived, or flagging it as a required step before merge. Once A addresses this I see no remaining blocker. |
There was a problem hiding this comment.
Wheels Bot — Reviewer A response (round 1)
B's sole critique is well-founded: A's round-3 review asserted "no new cross-engine issues" on a change that directly targets Adobe CF's Duplicate()-clones-CFCs behavior without explicitly addressing the testing gate that CLAUDE.md requires for exactly that category. A concedes the omission. The practical pre-merge gate for already-committed code is compat-matrix.yml CI, which runs Adobe CF 2023 and 2025; that should have been stated rather than assumed. The implementation fix itself is correct and portable — ArrayAppend is a uniform reference-append across all CFML engines, not an Adobe-only API. With the gate now stated explicitly, A's verdict remains approve.
Conceded points
-
Adobe CF matrix testing gap not flagged. The
$copyRouteForRequestchange is cross-engine-risky under CLAUDE.md's criteria: it exists specifically to work around Adobe CF'sDuplicate()-deep-cloning-CFCs-inside-arrays behavior (invariant #6). CLAUDE.md line 30 requirestools/test-matrix.sh adobe2023 mysqlANDtools/test-matrix.sh lucee7 mysqlfor such changes, and line 53 says "Verify Adobe CF fixes locally before pushing — don't iterate via CI." A's round-3 review said "no new cross-engine issues" without noting either requirement. B is correct: that assertion was incomplete.The appropriate explicit statement, which A omitted: CLAUDE.md's "don't iterate via CI" governs the development loop, not the merge decision for code that is already committed. For a PR under review, the pre-merge gate is
compat-matrix.yml, which covers Adobe CF 2023 and 2025. Merge is conditional on that run passing clean for the Adobe CF engines. If it is green, the requirement is satisfied; if it surfaces a failure on$copyRouteForRequestor theArraySlicespec harness, that is a blocker.
Defended points
- DCL correctness (round-3 finding). B independently verified the double-checked locking pattern in round 1 and found it correct. A maintains that finding.
- Minor nits (spec comment, preflight
afterEachcache cleanup). Not in dispute and remain advisory-only follow-up items.
Updated verdict
Approve, conditional on compat-matrix.yml Adobe CF 2023/2025 CI run passing. The only gap B identified was A's failure to state that gate explicitly — not a defect in the implementation. No other blockers remain.
1 participant
Summary
Route-scoped string middleware was re-instantiated on every request via
CreateObject(mw).init()inside$resolveMiddlewareInstance, so stateful middleware (an in-memoryRateLimiteron a per-API scope,RequestIdcounters, etc.) silently reset between requests — the documented registration form was effectively broken for any middleware that holds state. This PR pins a singleton lifecycle contract for route-scoped middleware so it matches the contract global middleware has always had: components are resolved once, cached in application scope keyed by component path, and reused across requests.$copyRouteForRequestno longerDuplicate()s the route'smiddlewarearray (Adobe CF clones CFCs inside arrays, which would silently reset the cached instances), and$hasPreflightCapableMiddlewarereturns a boolean precomputed at$initinstead of re-scanning the global pipeline on every OPTIONS request.Fixes #2954
Related Issue
Closes #2954
Type of Change
Feature Completeness Checklist
Signed-off-by:matchinggit config user.name/emailvendor/wheels/tests/specs/middleware/RouteMiddlewareLifecycleSpec.cfcpins:Test Plan
Local Lucee 7 + SQLite (via the existing server on port 60007, since the bundled `tools/test-local.sh` has a Mac-specific `sed -i ''` that doesn't apply on this Linux runner):
```
curl 'http://localhost:60007/wheels/core/tests?db=sqlite&format=json&directory=wheels.tests.specs.middleware' | jq '{p:.totalPass,f:.totalFail,e:.totalError}'
→ { "p": 183, "f": 0, "e": 0 }
curl 'http://localhost:60007/wheels/core/tests?db=sqlite&format=json&directory=wheels.tests.specs.dispatch' | jq '{p:.totalPass,f:.totalFail,e:.totalError}'
→ { "p": 106, "f": 0, "e": 0 }
```
The new `RouteMiddlewareLifecycleSpec` errored 5/6 cases before the fix (private methods unreachable + state lost between resolves) and passes 6/6 after.
Cross-engine reviewer note: the fix avoids
Duplicate()on the route's middleware array so Adobe CF (which deep-clones CFCs inside arrays) doesn't silently reset the cached instances. Stateful middleware components are now under a documented singleton-across-requests contract — all built-in middleware (RateLimiter,Cors,SecurityHeaders,RequestId,TenantResolver,AuthMiddleware,BrowserTestFixtureGuard) already satisfy this. Worth a second look on the application-scope cache key (application[\$appKey()].\$middlewareInstanceCache) to confirm the reload path clears it as expected on every engine.Screenshots / Output
n/a