Skip to content

fix: evaluate EVENT_CACHE_SUFFIX per request instead of freezing it at first use - #680

Merged
lmajano merged 1 commit into
developmentfrom
claude/event-cache-suffix-fix
Aug 16, 2026
Merged

fix: evaluate EVENT_CACHE_SUFFIX per request instead of freezing it at first use#680
lmajano merged 1 commit into
developmentfrom
claude/event-cache-suffix-fix

Conversation

@lmajano

@lmajano lmajano commented Aug 16, 2026

Copy link
Copy Markdown
Member

Description

A dynamic EVENT_CACHE_SUFFIX closure was evaluated once, the first time an event's caching metadata was memoized, and the resulting value was frozen into the memoized dictionary entry for the lifetime of the app. Every later request for that event — regardless of locale, session, slug, or whatever the closure actually reads — reused that first request's value, so different requests could silently share a cache key and serve each other's cached content.

This is the same underlying bug reported in #675 (thanks @homestar9 for finding and reporting it — COLDBOX-1411). That PR is against a fork I can't push to, and reviewing its approach surfaced two correctness gaps in the fix itself, so this PR is an independent implementation that starts from the same diagnosis but closes both gaps from the start. Details on what those gaps were and why below.

The fix

Store the closure itself in the memoized entry (never evaluate it during the once-per-app metadata build), and evaluate it on every read via a new resolveCacheSuffix() helper, on a shallow copy of the entry so the memoized original keeps the closure. This runs on both the request-start cache-lookup path (RequestService.eventCachingTest()HandlerService.getEventMetadataEntry()) and the cache-write path (HandlerService.getHandler()getEventCachingMetadata()), which must produce the same cache key from the same closure — otherwise a cached response is built under one key and looked up under another, and never served.

Two things make that guarantee easy to accidentally break:

  • The two paths must see the same request context. getEventMetadataEntry() and getEventCachingMetadata() now take requestContext as an explicit parameter, threaded from callers that already have it (RequestService.eventCachingTest()'s own arguments.context, HandlerService.getHandler()'s own oRequestContext), instead of resolveCacheSuffix() reaching for requestService.getContext() on its own — which reads request scope and can auto-create a context if one isn't already there, an unnecessary hazard when the real one was one call away the whole time.

  • The two paths must see an event handler bean with the same action metadata loaded, since a suffix closure may read eventHandlerBean.getActionMetadata(...). getHandlerBean() never loads metadata itself — only getHandler() does, by constructing a handler instance and reflecting it. When handlerCaching is on, the bean getHandlerBean() returns on the lookup path happens to be the same instance getHandler() already populated on an earlier request, so this is invisible — but with handlerCaching off, getHandlerBean() hands back a fresh, un-reflected bean on every call, so the lookup path's closure would silently see empty metadata while the write path's closure sees the real thing: two different suffixes, two different keys, cached responses never served. Factored the metadata-loading block out of getHandler() into ensureHandlerMetadata() and call it from both paths — it only constructs a handler instance when the bean doesn't already have one to reuse, so this doesn't add a second construction to the already-cached common case.

Testing

New test-harness/handlers/eventcachingSuffix.cfc fixture (kept separate from eventcaching.cfc so its handler-global suffix doesn't change the cache keys of every other spec using that handler) exercises, in EventCachingSpec.cfc: per-request re-evaluation producing distinct keys, lookup/build key parity, that same parity specifically under handlerCaching=false, the static-suffix fast path, and that the memoized dictionary keeps the closure rather than a frozen value.

The integration suite couldn't be executed in the local sandbox used for this work (BaseIntegrationTest needs a real servlet CGI scope unavailable there — confirmed pre-existing/unrelated to this change by running the file unmodified and seeing the identical failure). Full local regression suite (the bundles that can run in this sandbox): 200 passed, 4 failed/23 errors — unchanged against the established baseline, i.e. no regressions from this change.

Jira Issues

COLDBOX-1411

Type of change

  • Bug Fix
  • Improvement
  • New Feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist

  • My code follows the style guidelines of this project cfformat
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Generated by Claude Code

…t first use

A dynamic EVENT_CACHE_SUFFIX closure was evaluated once, the first time an
event's caching metadata was memoized, and the resulting value was frozen
into the memoized dictionary entry for the lifetime of the app. Every
later request for that event - regardless of locale, session, slug, or
whatever the closure actually reads - reused that first request's value,
so different requests could silently share a cache key and serve each
other's cached content.

Fix: store the closure itself in the memoized entry (never evaluate it
during the once-per-app metadata build), and evaluate it on every read via
a new resolveCacheSuffix() helper, on a shallow copy of the entry so the
memoized original keeps the closure. This is exercised on both the
request-start cache-lookup path (RequestService.eventCachingTest() ->
HandlerService.getEventMetadataEntry()) and the cache-write path
(HandlerService.getHandler() -> getEventCachingMetadata()), which must
produce the same cache key from the same closure or a cached response is
built under one key and looked up under another - never served.

Two things that guarantee is easy to accidentally break, both addressed
here:

- The two paths must see the same request context. getEventMetadataEntry()
  and getEventCachingMetadata() now take requestContext as an explicit
  parameter, threaded from callers that already have it
  (RequestService.eventCachingTest()'s own arguments.context,
  HandlerService.getHandler()'s own oRequestContext), instead of a
  resolveCacheSuffix() implementation reaching for
  requestService.getContext() on its own - which reads request scope and
  can auto-create a context if one isn't already there, an unnecessary
  hazard when the real one was one call away the whole time.

- The two paths must see an event handler bean with the same action
  metadata loaded, since a suffix closure may read
  eventHandlerBean.getActionMetadata(...). getHandlerBean() never loads
  metadata itself - only getHandler() does, by constructing a handler
  instance and reflecting it. When handlerCaching is on, the bean
  getHandlerBean() returns on the lookup path happens to be the same
  instance getHandler() already populated on an earlier request, so this
  is invisible - but with handlerCaching off, getHandlerBean() hands back
  a fresh, un-reflected bean on every call, so the lookup path's closure
  would silently see empty metadata while the write path's closure sees
  the real thing: two different suffixes, two different keys, cached
  responses never served. Factored the metadata-loading block out of
  getHandler() into ensureHandlerMetadata() and call it from both paths -
  it only constructs a handler instance when the bean doesn't already
  have one to reuse, so this doesn't add a second construction to the
  already-cached common case.

New test-harness/handlers/eventcachingSuffix.cfc fixture (separate from
eventcaching.cfc so its handler-global suffix doesn't change the cache
keys of every other spec using that handler) exercises: per-request
re-evaluation producing distinct keys, lookup/build key parity, that same
parity under handlerCaching=false specifically, the static-suffix
fast path, and that the dictionary keeps the closure rather than a frozen
value.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016kCmPkBvNZZhU6iuDcG4NR
@github-actions

github-actions Bot commented Aug 16, 2026

Copy link
Copy Markdown

Test Results

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ❌ ±0 

Results for commit 5271b37. ± Comparison against base commit 1def0d3.

♻️ This comment has been updated with latest results.

@lmajano
lmajano merged commit ea63aac into development Aug 16, 2026
28 checks passed
@lmajano
lmajano deleted the claude/event-cache-suffix-fix branch August 16, 2026 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants