fix(dispatch): Global cache cull, date parsing, CF version gate, existence caches - #2933
Conversation
…tence caches
Addresses six review findings in vendor/wheels/Global.cfc (wave 2,
package global-core):
- dispatch-core:2 — collapse the three duplicate "Adobe ColdFusion"
branches in $checkMinimumVersion (only the first ever ran, enforcing
EOL CF 11.0.18) into a single branch enforcing the documented CF 2018
minimum, and document the Lucee per-major-release numeral-key floor.
- dispatch-core:3 — $addToCache no longer deletes keys from the struct
it is iterating: snapshot keys per category, compute $cacheCount()
once, and cull expired items across all categories (the maximum is
global) so a write to a small category can actually free room instead
of silently dropping the new item.
- dispatch-core:4 — funnel all slash-date disambiguation through a new
$parseSlashDate() helper (d1>12 → DD/MM, d2>12 → MM/DD, ambiguous →
engine adapter). The inline BoxLang block previously treated D1/D2/YYYY
as DD/MM unconditionally, so "06/25/2024 10:30 AM" crashed with
CreateDateTime(2024,25,6,...). Also fixes the catch-fallback's dead
\\d regexes (literal backslash-d never matched a digit) and routes
model/miscellaneous.cfc's BoxLang slash-date coercion through the same
helper instead of exception-driven recovery.
- dispatch-core:13 — $objectFileName memoizes file existence in structs
keyed by path instead of comma lists scanned with O(n) ListFindNoCase
per materialized object (which also lost entries to unlocked
concurrent ListAppend).
- dispatch-core:16 — $get() probes the tenant config override with a
cheap StructKeyExists chain instead of string-parsing
IsDefined("request.wheels.tenant.config") on every settings read.
- routing:18 — $lockedLoadRoutes clears the staticRoutes index alongside
routes/namedRoutePositions so a route reload cannot serve stale
first-write-wins entries; delete the dead variables.staticRoutes in
Mapper.cfc.
New specs (verified red against pre-fix code, green post-fix on
Lucee 7 + SQLite): addToCacheSpec, convertToStringSpec, loadRoutesSpec,
objectFileNameSpec; internalSpec's Adobe version expectations updated
for the CF 2018 floor.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Peter Amiri <peter@alurium.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: Six tightly-scoped fixes for the global-core package — cache cull snapshot, $parseSlashDate disambiguation, Adobe 2018 version gate, struct-backed file-existence memo, $get() StructKeyExists chain, and staticRoutes clear on reload. All primary fixes are correct and well-tested. Two minor follow-up items worth tracking: the ISO regex in the catch-fallback branch carries the same \\d bug the PR fixes in the slash branch, and the DC16 $get() change has no corresponding spec. Overall verdict: comment (no correctness, cross-engine, or security blockers).
Correctness
ISO fallback regex still carries the \\d bug
vendor/wheels/Global.cfc (catch-fallback, branch 1):
if (ReFind("(?i)^(\\d{4})-(\\d{2})-(\\d{2})(?:[ T](\\d{1,2}):(\\d{2})(?::(\\d{2}))?)?$", local.s2)) {CFML does not treat \ as a string-escape character, so "\\d" is the two-character string \d — which as a Java regex is \d (digit metacharacter) — wait, actually "\\d" is two source backslashes, making the string \\d, which the Java regex engine reads as \\d = literal-backslash + digit. Concretely, this regex never matches an ISO date like 2024-06-25; it would need \dddd-\dd-\dd as input.
The adjacent slash-date regex was fixed in this PR ("^\\d{1,2}/..." → "^\d{1,2}/..."), and the PR body explicitly notes this ISO branch as a deferred non-blocking follow-up:
"the ISO branch of
$convertToString's catch-fallback still carries the same dead\d-as-literal regex bug"
Severity: low — ParseDateTime handles ISO strings correctly on all supported engines, so the catch branch never fires for ISO inputs. Not a blocker, but it should be tracked.
Suggested fix (for the follow-up PR):
// Replace \\d with \d throughout the ISO branch regex and backreference expressions
if (ReFind("(?i)^(\d{4})-(\d{2})-(\d{2})(?:[ T](\d{1,2}):(\d{2})(?::(\d{2}))?)?$", local.s2)) {
local.parts = ReReplace(local.s2, "^(\d{4})-(\d{2})-(\d{2}).*$", "\1-\2-\3", "all");
local.timePart = ReReplace(local.s2, ".*[ T](\d{1,2}:\d{2}(?::\d{2})?).*$", "\1", "all");Tests
DC16 ($get()) fix has no spec
The IsDefined("request.wheels.tenant.config") → StructKeyExists chain change (Global.cfc:636-650) is untested. For a perf-only fix this is understandable, but a simple spec asserting that tenant config overrides are applied (and that the code path doesn't throw when request.wheels is absent) would provide a safety net against future regressions.
loadRoutesSpec.cfc afterAll leaks a spurious staticRoutes key
vendor/wheels/tests/specs/global/loadRoutesSpec.cfc lines 37-40:
function afterAll() {
application.wheels.routes = _originalRoutes
application.wheels.staticRoutes = _originalStaticRoutes // ← restores {} when key didn't exist before
application.wheels.namedRoutePositions = _originalNamedRoutePositions
}_originalStaticRoutes is set to {} when staticRoutes wasn't present before the spec ran (beforeAll line 7: StructKeyExists(application.wheels, "staticRoutes") ? StructCopy(...) : {}). After the spec, application.wheels.staticRoutes = {} leaves an extra empty struct key that wasn't there originally. Dispatch.cfc guards it correctly (StructKeyExists(application.wheels, "staticRoutes") → enters block, finds no matching key → falls through), so there is no functional impact. Still worth fixing for spec isolation hygiene:
function afterAll() {
application.wheels.routes = _originalRoutes
application.wheels.namedRoutePositions = _originalNamedRoutePositions
if (_hadStaticRoutes) {
application.wheels.staticRoutes = _originalStaticRoutes
} else {
StructDelete(application.wheels, "staticRoutes")
}
}where beforeAll records _hadStaticRoutes = StructKeyExists(application.wheels, "staticRoutes").
Docs
No CHANGELOG entry
The PR body states: "Entry deliberately omitted; consolidated at campaign end." This deviates from the repo convention (CLAUDE.md: [Unreleased] entry expected). Acknowledged and deferred — noting for the release author to pick up.
Everything else looks good
- Cache cull (
$addToCache): snapshot-before-delete withStructKeyArray, global cross-category culling,$cacheCount()called once,local.currentCountdecremented by actual deletes — all correct.cacheLastCulledAtis updated even when no items were culled (pre-existing behaviour, not a regression). $parseSlashDate(): disambiguation logic (d1 > 12→ DD/MM,d2 > 12→ MM/DD, ambiguous → engine adapter) is correct and consistent with the three call sites it replaces. Declaredpublicwith$prefix — mixin-safe per invariant 7.$objectFileNamestruct memo:StructKeyExists/ struct-assignment correctly replacesListFindNoCase/ListAppend. TheListSetAtmutation oflocal.fullObjectPathis correctly dropped; the proper-case name is now stored as the struct VALUE, retrieved via a case-insensitive key lookup (default struct semantics, all engines).StructKeyExists(local, "file")guard in the final return block remains correct.- Adobe 2018 version gate: three dead duplicate branches correctly collapsed;
minimumBuild = ""with theLen(local.minimumBuild)guard skips the build check — any 2018.x passes. The per-majorlocal.5 = {…}Lucee floor is unaffected. $get()StructKeyExists chain: short-circuit evaluation means none of the inner calls execute whenrequestdoesn't carry a"wheels"key, so no risk of "undefined variable" errors during early request lifecycle.$lockedLoadRoutesstaticRoutes clear:StructKeyExistsguard prevents a throw if the key was never written;StructClearcorrectly empties the index so stale entries can't survive a route reload.Dispatch.cfcreads fromapplication.wheelswhich equalsapplication[$appKey()]at runtime (afteronapplicationstartrenames$wheels→wheels). ✓- Regex fix:
"^\\d{1,2}/\\d{1,2}/\\d{4}"→"^\d{1,2}/\d{1,2}/\d{4}"and"\\d{1,2}:\\d{2}"→"\d{1,2}:\d{2}"— correct; the double-backslash forms produced Java regex\\d(literal-backslash + digit), which never matched real date/time strings. - Specs:
addToCacheSpec,convertToStringSpec,loadRoutesSpec,objectFileNameSpec,internalSpec— all follow BDDextends="wheels.WheelsTest"style, no inline closures as constructor named args, nolocal.X = ...inside catch,StructKeyExists(local, …)guards used appropriately. - Commit:
fix(dispatch): Global cache cull, date parsing, CF version gate, existence caches— valid conventional-commit header (72 chars, scope unrestricted).
|
Wheels Bot — Reviewer B (round 1) A's review is solid. No sycophancy, no false positives, findings verified. A correctly left the verdict at 'comment' (no blocker) and I agree. The three non-blocking items are all real, none prevents merge. Sycophancy: None detected. A's verdict is 'comment,' not approve, and every 'looks good' section cites specific implementation details rather than hand-waving. False positives: None detected.
Missed issues: None that rise above what A flagged. Rescan covered: cross-engine invariants (no inline closures as constructor named args; the new slash-date helper correctly declared public with dollar prefix; the catch-block assignment in miscellaneous.cfc mutates a pre-existing struct field so BoxLang invariant 11 does not apply); commit header length (82 chars, within limit); the checkMinimumVersion collapse (minimumBuild empty-string with Len guard means any 2018.x passes); onapplicationstart.cfc struct initialization matches the new StructKeyExists usage. No additional issues found. Verdict alignment: A's 'comment' verdict is consistent with the findings: every item is non-blocking and either already acknowledged in the PR body or framed as a hygiene suggestion. 'Comment' is the right state. Convergence: Aligned. A and I both read the PR as merge-ready with three tracked non-blockers: (1) the ISO catch-fallback regex deferred to a follow-up PR, (2) a missing spec for the settings-getter StructKeyExists chain, and (3) the loadRoutesSpec afterAll hygiene. No correctness, cross-engine, or security blocker found. Joint recommendation: merge as-is; three items go into a follow-up. |
Resolves the one conflict (vendor/wheels/events/onapplicationstart.cfc) by composing both sides: keep #2930's helperFileCache/layoutFileCache struct caches from develop (consumed by Controller.cfc and controller/layouts.cfc) and keep this PR's struct-typed existingObjectFiles/nonExistingObjectFiles existence caches (consumed by Global.cfc $objectFileName). The obsolete existing/nonExisting helper- and layout-file list initializers are dropped; no consumers remain repo-wide. Auto-merged files verified semantically: Mapper.cfc (dead instance-level staticRoutes removal vs #2924 comment update — disjoint), model/miscellaneous.cfc ($parseSlashDate call vs #2931 $stampTimestampProperty helper — disjoint), Global.cfc (PR hunks at ~633-3007 vs #2912/#2943 hunks at ~3063-3181 — disjoint; both sides present in the merge). Verified locally on Lucee 7 + SQLite (dir-only docker mount): global 133/0/0, dispatch 104/0/0, mapper dir 89/0/0, mapperSpec 64/0/0, mapperModernSpec 27/0/0, routingSpec 5/0/0, events 36/0/0, controller 456/0/0 (1 skip), model 869/0/0 (11 skip). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: Re-review at head b7a2b6b0d — the only change since the previous review (84ad292) is a merge of origin/develop into the branch to bring it current; the PR code itself is unchanged. All six primary fixes are correct and well-tested. Three carry-forward findings from the prior pass remain; no new concerns are introduced by the merge. Verdict: comment.
Correctness
ISO fallback regex (carry-forward, deferred)
vendor/wheels/Global.cfc — the catch-fallback ISO branch still uses "\\d{4}"-style patterns (double-backslash → Java regex \\d → literal backslash + d, not a digit). This was explicitly called out in the PR body as a deferred non-blocking follow-up. ParseDateTime handles ISO strings correctly on all supported engines so the catch branch never fires for real ISO input; the risk is latent but not blocking.
DC16 ($get()) change lacks a dedicated spec (carry-forward)
vendor/wheels/Global.cfc:633-650 — the IsDefined → StructKeyExists chain has no corresponding spec. The change is straightforward and low-risk, but a small test asserting the tenant-config override path (and that it does not throw when request.wheels is absent) would prevent future regressions. Still not a blocker.
Tests
loadRoutesSpec.cfc afterAll leaks a spurious staticRoutes key (carry-forward)
vendor/wheels/tests/specs/global/loadRoutesSpec.cfc:37-40:
function afterAll() {
application.wheels.routes = _originalRoutes
application.wheels.staticRoutes = _originalStaticRoutes // restores {} when key was absent before
application.wheels.namedRoutePositions = _originalNamedRoutePositions
}_originalStaticRoutes is {} when staticRoutes was not present before the spec ran (beforeAll line 5: StructKeyExists(...) ? StructCopy(...) : {}). After the spec, application.wheels.staticRoutes = {} leaves an extra empty struct key that was not there originally. Dispatch.cfc guards with StructKeyExists so there is no functional impact, but spec teardown should mirror setup fidelity.
Suggested fix:
function beforeAll() {
_hadStaticRoutes = StructKeyExists(application.wheels, "staticRoutes")
_originalStaticRoutes = _hadStaticRoutes ? StructCopy(application.wheels.staticRoutes) : {}
// ... rest of beforeAll
}
function afterAll() {
application.wheels.routes = _originalRoutes
application.wheels.namedRoutePositions = _originalNamedRoutePositions
if (_hadStaticRoutes) {
application.wheels.staticRoutes = _originalStaticRoutes
} else {
StructDelete(application.wheels, "staticRoutes")
}
}Docs
No CHANGELOG entry (carry-forward, intentional)
The PR body states "Entry deliberately omitted; consolidated at campaign end." This deviates from the repo convention (CLAUDE.md: [Unreleased] entry expected). Flagging for the release author to pick up at campaign close — no action required on the PR author.
Everything else looks good
A pass over the merge-updated diff confirms no regressions:
$addToCachecull:$cacheCount()called once intolocal.currentCount;StructKeyArraysnapshots prevent concurrent-modification;local.currentCount -= local.deletedItemstracks the adjusted total; the final admission guard is correct for all cull-condition branches (cull fired, cull skipped — cache not full, cull skipped — interval not elapsed).$parseSlashDatehelper: declaredpublicwith$prefix (mixin-safe per invariant 7); disambiguation logic (d1>12 → DD/MM, d2>12 → MM/DD, ambiguous →$engineAdapter().parseAmbiguousSlashDate) is correct. The call site inmodel/miscellaneous.cfc:313retains itstry/catchfallback; the AM/PM block inGlobal.cfcvalidates the format withReFindbefore calling the helper.- Regex fix:
"^\\d{1,2}/\\d{1,2}/\\d{4}"→"^\d{1,2}/\d{1,2}/\d{4}"and the time pattern — correct. In CFML,"\\d"is three characters\\dwhich the Java regex engine reads as literal-backslash + digit, never matching a real date string;"\d"is two characters\d= digit metaclass. $objectFileNamestruct memo:application.wheels.existingObjectFiles[path] = properCaseName(value) /nonExistingObjectFiles[path] = falsecorrectly replaces O(n)ListFindNoCase+ListAppend.onapplicationstart.cfcinit changed from""to{}to match. Case-insensitive struct lookup preserves the oldListFindNoCasesemantics on all supported engines.- Adobe version gate: three duplicate
Adobe ColdFusionbranches collapsed to a single 2018 floor;minimumBuild = ""with theLen()guard skips the build check — any 2018.x passes. $lockedLoadRoutesstatic-routes clear:StructKeyExistsguard prevents a throw if the key was never written;StructClearcorrectly empties the index on reload. Deadvariables.staticRoutesinMapper.cfccorrectly removed (Mapper wrote only the application-scoped index; Dispatch reads it fromapplication[appKey]).$get()StructKeyExists chain: short-circuit evaluation means inner expressions never execute whenrequesthas no"wheels"key — no early-lifecycle undefined-variable risk.- Specs:
addToCacheSpec,convertToStringSpec,loadRoutesSpec,objectFileNameSpec,internalSpec— all extendwheels.WheelsTestBDD style, no inline closures as constructor named args (invariant 5), nolocal.Xassignments inside catch bodies (invariant 11),StructKeyExists(local, ...)guards used appropriately. - Commit:
fix(dispatch): Global cache cull, date parsing, CF version gate, existence caches— 72-char header, valid conventional-commit type and scope.
Wheels Bot — Reviewer B (round 1)A's review is solid. The three carry-forward findings are accurately sourced, the technical walkthrough checks out against every line I re-read in the diff, and the "comment" verdict is calibrated correctly — non-blocking items should not gate a merge. One minor factual error in A's review, one small omission in the teardown analysis, neither changes the recommendation. SycophancyNone detected. A uses verdict "comment" (not approve), backs every finding with file and line references, and explicitly does not clear the carry-forward items despite them being non-blocking. The "Everything else looks good" section is preceded by a line-by-line walkthrough that demonstrates the claim. False positives
All other claims I verified directly against the diff:
Missed issues
No other missed issues. I re-scanned for cross-engine compat (invariants 1-14), tests that do not exercise the change, and security concerns. Nothing A skipped. Verdict alignmentA's "comment" verdict is consistent with the findings — three acknowledged non-blocking carry-forwards, all primary fixes verified correct, no new concerns introduced by the merge commit. "Approve" would have been sycophantic here; "request-changes" would be overcorrecting on deferred items the PR author already acknowledged. ConvergenceAligned. A's review is accurate, the verdict is appropriate, and no code changes are needed for this SHA. The |
…ntegration-and-refle Resolves the CHANGELOG.md conflict by integrating the PR's entry as a new "### Performance" subsection inside develop's populated "## [Unreleased]" section (discarding the PR hunk's malformed single-hash "# [Unreleased]" header). vendor/wheels/Global.cfc auto-merged cleanly: develop's sibling changes since the merge base (#2933 cache culls + $objectFileName struct memoization, #2939 $deprecated registry, #2912/#2943 provider isolation + mixin-collision helpers, staticRoutes clear in $lockedLoadRoutes) touch regions orthogonal to this PR's $cachedModelLookup/$cachedControllerLookup helpers and the model()/controller() fast paths; both sides' semantics verified to survive — the fast path reads the same application.wheels.models/controllers structs that develop's unchanged $cachedModelClassExists/$createModelClass write. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com>
Summary
Fixes six findings from the internal framework review in the global-core package: the duplicated Adobe CF version gate, the
$addToCachecull that mutated the struct it was iterating, the contradictory slash-date parsers in$convertToString(the inline BoxLang block crashed on unambiguous US dates like06/25/2024 10:30 AM), the comma-list file-existence memo in$objectFileName, the per-readIsDefined("request.wheels.tenant.config")string parse in$get(), and thestaticRoutesindex that$lockedLoadRoutesnever cleared.Reviewer-noted non-blocking follow-ups deferred (out of scope here): the ISO branch of
$convertToString's catch-fallback still carries the same dead\d-as-literal regex bug fixed in the adjacent slash branch, and$addToCachestill lacks the try/catch parity$getFromCachehas (the in-loopStructKeyExistsguard mitigates the practical race).Findings addressed
vendor/wheels/Global.cfc:3045(collapsed branch inside$checkMinimumVersionat:2996; enforces 2018.0.0,Len()guards the emptyminimumBuild; Lucee numeral-key floor documented)$addToCachecull deletes keys from the struct it iterates and mixes per-category culling with the global item count @vendor/wheels/Global.cfc:819($cacheCount()computed once, categories/keys snapshotted viaStructKeyArraybefore deletion, expired items culled globally with aCeilingcap consistent with the global trigger/insert checks)$convertToStringcontains three contradictory slash-date parsers; the inline BoxLang one breaks unambiguous US dates @vendor/wheels/Global.cfc:2637(new$parseSlashDate()helper: d1>12 → DD/MM, d2>12 → MM/DD, ambiguous → engine adapter); all three call sites funneled through it, includingvendor/wheels/model/miscellaneous.cfc:313; the catch-fallback's dead"\\d"regexes (matched a literal backslash, never a digit) fixed in the slash branch$objectFileNamememoizes file-existence in comma lists scanned with O(n)ListFindNoCaseon every model-object materialization @vendor/wheels/Global.cfc:1023(struct memo keyed by path; init atvendor/wheels/events/onapplicationstart.cfc:108,111; default structs are case-insensitive on every engine, preserving the oldListFindNoCasesemantics)$get()evaluatesIsDefined("request.wheels.tenant.config")on every settings read @vendor/wheels/Global.cfc:643-650(cheapStructKeyExistschain;request.wheels.tenantis only ever assigned a struct byswitchTenant, so the chain cannot throw)staticRoutesindex not cleared by$lockedLoadRoutes— stale first-write-wins entries survive a route reload @vendor/wheels/Global.cfc:1443-1444(StructClearalongsideroutes/namedRoutePositions); deadvariables.staticRoutesdeleted fromvendor/wheels/Mapper.cfc(formerly:43, zero readers — Mapper writes only the application-scoped index, Dispatch reads are guarded)Findings verified already-fixed
None — all six package findings still reproduced against
origin/developat implementation time. Independently spot-checked during review for DC2 (the three duplicate branches at develop'sGlobal.cfc:3011/3016/3021) and R18 ($lockedLoadRoutesmissing the clear).Source
Internal multi-agent framework review 2026-06-09, wave 2, package global-core.
Tests
New BDD specs under
vendor/wheels/tests/specs/global/, each verified red against pre-fix code (4 failures / 3 errors total) and green post-fix:addToCacheSpec.cfc— cull math (pct=50, 10 expired → 6 remaining incl. the new item), global culling across categories, snapshot-before-deleteconvertToStringSpec.cfc—$parseSlashDatedisambiguation incl. the previously crashing06/25/2024 10:30 AMcase (pre-fix: 3 errors on the missing helper)loadRoutesSpec.cfc—staticRoutescleared on reload (pre-fix: stale sentinel survived)objectFileNameSpec.cfc— struct-shaped existence memo across cache-on/off × exists/missing paths (pre-fix:IsStructfailure)internalSpec.cfc— Adobe version expectations updated for the CF 2018 floorLocal verification: single-bundle Docker run on Lucee 7 + SQLite (worktree-safe recipe). The full engine × DB matrix runs in CI, which is the real gate.
Cross-engine notes
$parseSlashDatehelper ispublicwith$prefix — mixin-safe per invariant 7 (private mixin functions are not integrated on Lucee/Adobe).ListFindNoCase.convertToStringerrors came from the missing helper, not the BoxLang crash itself).Changelog
Entry deliberately omitted; consolidated at campaign end.
🤖 Generated with Claude Code