From 4d67dd122cb023951adf29d808df148be3e69e5d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 06:05:49 +0000 Subject: [PATCH 1/3] COLDBOX-1454 #resolve interceptor buffer pool races between a request and its async announce thread InterceptorService.getLazyBuffer()'s per-request buffer pool only skipped pooling for the announce() call that itself spawns an async/ asyncAll thread. It missed that code running *inside* that spawned thread can make its own synchronous announce() calls - most commonly WireBox's afterInstanceAutowire announcement, fired by every getInstance() call - which still pool against the same request-scoped array as the original request thread. `request` scope, and thus the pool array, is shared between a request and any cfthread spawned from it. CFML/BoxLang arrays aren't thread-safe, so two real concurrent threads (the request thread and an async announce()'s thread) popping/releasing the same array can corrupt it, throwing "can not pop Element from array, array is empty" mid-request. Also check controller.getUtil().inThread() (the same check InterceptorState.process() already uses for its own async dispatch decision) so a synchronous announce() executing inside any cfthread always gets a fresh, unpooled buffer - never touching the array the request thread is using. Added regression coverage for both the pooled (reused) and never-pooled (inside a thread / not asked to pool) paths. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KiK6DMek9iJMuYk2PzcjPj --- system/web/services/InterceptorService.cfc | 10 +++- .../web/services/InterceptorserviceTest.cfc | 59 +++++++++++++++++++ 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/system/web/services/InterceptorService.cfc b/system/web/services/InterceptorService.cfc index c24a98692..6401d3c3d 100644 --- a/system/web/services/InterceptorService.cfc +++ b/system/web/services/InterceptorService.cfc @@ -218,7 +218,15 @@ component extends="coldbox.system.web.services.BaseService" accessors="true" { var event = controller.getRequestService().getContext() // Threaded paths hand the buffer to a background thread that can outlive this call, so // it can never be safely pooled/reused - only the synchronous (default) path pools it. - var pooled = !arguments.async && !arguments.asyncAll + // Also never pool while already executing inside a cfthread (e.g. a synchronous announce() + // triggered from code running inside an async/asyncAll thread, such as getInstance()'s + // afterInstanceAutowire announcement): `request` scope - and thus the pool array - is + // shared between the request thread and any cfthread spawned from it, and CFML/BoxLang + // arrays aren't thread-safe, so two real concurrent threads popping/releasing the same + // array can corrupt it ("can not pop Element from array, array is empty" - COLDBOX-1454). + // InterceptorState.process() uses the same controller.getUtil().inThread() check for its + // own async dispatch decision. + var pooled = !arguments.async && !arguments.asyncAll && !variables.controller.getUtil().inThread() var buffer = getLazyBuffer( pooled ) try { diff --git a/tests/specs/web/services/InterceptorserviceTest.cfc b/tests/specs/web/services/InterceptorserviceTest.cfc index 374b5bf0d..157f8d6fc 100755 --- a/tests/specs/web/services/InterceptorserviceTest.cfc +++ b/tests/specs/web/services/InterceptorserviceTest.cfc @@ -19,6 +19,7 @@ // Mock model Dependencies mockController.$( "getRequestService", mockRequestService ); + mockController.$( "getUtil", new coldbox.system.core.util.Util() ); mockController.setLogBox( mockLogBox ); mockController.setWireBox( mockWireBox ); @@ -177,6 +178,64 @@ expect( local.output ).toBe( "buffered output" ) } + /** + * Unambiguous reference-identity check: CFML's own object equality can compare by value/ + * string representation, which makes two freshly-created, still-empty InterceptorBuffer + * instances look "equal" even though they're genuinely different objects. The JVM's identity + * hash sidesteps that entirely. + */ + private function objectId( required obj ){ + return createObject( "java", "java.lang.System" ).identityHashCode( arguments.obj ) + } + + function testAnnouncePoolsAndReusesTheBufferWhenNotInsideAThread(){ + var buffers = [] + iService.listen( function( event, data, buffer ){ + buffers.append( arguments.buffer ) + }, "onPoolTest" ) + + iService.announce( "onPoolTest" ) + iService.announce( "onPoolTest" ) + + // Sequential, non-threaded announce() calls reuse the one pooled buffer + expect( objectId( buffers[ 1 ] ) ).toBe( objectId( buffers[ 2 ] ) ) + } + + function testAnnounceNeverPoolsTheBufferWhileInsideAThread(){ + // COLDBOX-1454: `request` scope - and thus the buffer pool array - is shared between + // the request thread and any cfthread spawned from it. A synchronous announce() running + // on a spawned thread (e.g. WireBox's afterInstanceAutowire, triggered by getInstance() + // from code running inside an async/asyncAll announce()'s thread) must never touch the + // same pool the request thread is using, or two real concurrent threads can pop/release + // the same array at once and corrupt it. + mockController.$( "getUtil", mockBox.createStub().$( "inThread", true ) ) + + var buffers = [] + iService.listen( function( event, data, buffer ){ + buffers.append( arguments.buffer ) + }, "onPoolTest" ) + + iService.announce( "onPoolTest" ) + iService.announce( "onPoolTest" ) + + expect( objectId( buffers[ 1 ] ) ).notToBe( objectId( buffers[ 2 ] ) ) + } + + function testGetLazyBufferPoolsAndReusesWhenAskedTo(){ + var buffer1 = iService.getLazyBuffer( true ) + iService.releaseLazyBuffer( buffer1 ) + var buffer2 = iService.getLazyBuffer( true ) + + expect( objectId( buffer1 ) ).toBe( objectId( buffer2 ) ) + } + + function testGetLazyBufferNeverReusesWhenNotAskedTo(){ + var buffer1 = iService.getLazyBuffer( false ) + var buffer2 = iService.getLazyBuffer( false ) + + expect( objectId( buffer1 ) ).notToBe( objectId( buffer2 ) ) + } + function testInterceptionPoints(){ // test registration again assertTrue( arrayLen( iService.getInterceptionPoints() ) gt 0 ); From 336288f8916d24c26396024823b69f936b9787ef Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 06:11:51 +0000 Subject: [PATCH 2/3] COLDBOX-1454 #comment account for the pool outliving its request An async announce()'s thread can itself become unbound from the request that spawned it - a fire-and-forget async announce, or one nobody joined, can keep running after the request has already ended. By the time that orphaned thread's own work touches the buffer pool, `request` scope may no longer be the one the pool was built against, or may not be usable at all. The pool is a performance nicety, never load-bearing for correctness, so getLazyBuffer()/releaseLazyBuffer() now treat any failure reading or writing it the same as "no pool available": fall back to a fresh, unpooled buffer (get) or silently drop it (release), rather than letting that propagate out of announce(). Added regression tests for both directions, and hardened the test file's setup() to clear any leftover pool state between tests, since request scope - and thus the pool - persists across every test in a TestBox run. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KiK6DMek9iJMuYk2PzcjPj --- system/web/services/InterceptorService.cfc | 49 +++++++++++++------ .../web/services/InterceptorserviceTest.cfc | 30 ++++++++++++ 2 files changed, 65 insertions(+), 14 deletions(-) diff --git a/system/web/services/InterceptorService.cfc b/system/web/services/InterceptorService.cfc index 6401d3c3d..28318c7af 100644 --- a/system/web/services/InterceptorService.cfc +++ b/system/web/services/InterceptorService.cfc @@ -267,23 +267,37 @@ component extends="coldbox.system.web.services.BaseService" accessors="true" { * instead of allocating a new component every time. A reentrant announce() (e.g. an * interceptor that itself triggers `announce( "onException", ... )` while executing) finds * the pool empty and gets its own instance, so it can never clobber the in-flight buffer of - * the call still running further up the stack. The async/asyncAll paths always get a fresh, - * unpooled instance since their buffer is handed to a background thread that can outlive - * this call. + * the call still running further up the stack. The async/asyncAll paths, and any announce() + * running inside a cfthread, always get a fresh, unpooled instance since their buffer can be + * used by a background thread that can outlive this call. + * + * The pool lives in `request` scope, but an async announce()'s thread can itself outlive the + * request that spawned it (a fire-and-forget async announce, or one nobody joined) - by the + * time that orphaned thread's own work runs, `request` scope may no longer be the one this + * pool was built against, or may not be usable at all. The pool is a performance nicety only, + * never load-bearing for correctness, so any failure reading it here is treated the same as + * "no pool available" - fall back to a fresh, unpooled buffer rather than letting the + * announce() itself fail. * * @pooled Whether to check out from (and later return to) the per-request pool * * @return { get(), clear(), append(), length(), getString() } */ function getLazyBuffer( boolean pooled = true ){ - if ( - arguments.pooled && - structKeyExists( request, "cbox_interceptorBufferPool" ) && - request.cbox_interceptorBufferPool.len() - ) { - var buffer = request.cbox_interceptorBufferPool.pop() - buffer.clear() - return buffer + if ( arguments.pooled ) { + try { + if ( + structKeyExists( request, "cbox_interceptorBufferPool" ) && + request.cbox_interceptorBufferPool.len() + ) { + var buffer = request.cbox_interceptorBufferPool.pop() + buffer.clear() + return buffer + } + } catch ( any e ) { + // request scope is gone or unusable (e.g. this thread has outlived the request + // that spawned it) - the pool goes with it, fall through to an unpooled buffer. + } } return new coldbox.system.web.context.InterceptorBuffer() } @@ -291,13 +305,20 @@ component extends="coldbox.system.web.services.BaseService" accessors="true" { /** * Return a buffer checked out via getLazyBuffer( true ) back to the per-request pool. * + * Silently drops the buffer if `request` scope is gone or unusable (see getLazyBuffer()) - + * there is no pool left to release it into, and that's fine, it just won't be reused. + * * @buffer The buffer instance to release */ function releaseLazyBuffer( required buffer ){ - if ( !structKeyExists( request, "cbox_interceptorBufferPool" ) ) { - request.cbox_interceptorBufferPool = [] + try { + if ( !structKeyExists( request, "cbox_interceptorBufferPool" ) ) { + request.cbox_interceptorBufferPool = [] + } + request.cbox_interceptorBufferPool.append( arguments.buffer ) + } catch ( any e ) { + // Nothing to release into - safe to drop. } - request.cbox_interceptorBufferPool.append( arguments.buffer ) } /** diff --git a/tests/specs/web/services/InterceptorserviceTest.cfc b/tests/specs/web/services/InterceptorserviceTest.cfc index 157f8d6fc..300d82ed7 100755 --- a/tests/specs/web/services/InterceptorserviceTest.cfc +++ b/tests/specs/web/services/InterceptorserviceTest.cfc @@ -3,6 +3,12 @@ function setup(){ super.setup(); + // `request` scope persists across every test in this suite (they all run inside one + // physical HTTP request to the test runner), so a leftover interceptor buffer pool - or + // one a test deliberately broke to exercise the COLDBOX-1454 fallback - must not leak + // into the next test. + structDelete( request, "cbox_interceptorBufferPool" ); + // Create Mock Objects variables.mockbox = getMockBox(); variables.mockController = mockBox.createMock( "coldbox.system.testing.mock.web.MockController" ); @@ -236,6 +242,30 @@ expect( objectId( buffer1 ) ).notToBe( objectId( buffer2 ) ) } + function testGetLazyBufferFallsBackToAnUnpooledBufferWhenThePoolIsUnusable(){ + // COLDBOX-1454: an async announce()'s thread can outlive the request that spawned it (a + // fire-and-forget async announce, or one nobody joined). By the time that orphaned + // thread's own work runs, `request` scope - and the pool living in it - may no longer be + // usable. The pool is a performance nicety only, so this must degrade to a fresh, + // unpooled buffer instead of throwing. A struct in place of the expected array simulates + // that unusable state: structs have no pop() method, so reading it throws. + request.cbox_interceptorBufferPool = { "not" : "an array" } + + var buffer = iService.getLazyBuffer( true ) + + expect( isObject( buffer ) ).toBeTrue() + expect( buffer.hasContent() ).toBeFalse() + } + + function testReleaseLazyBufferSilentlyDropsWhenThePoolIsUnusable(){ + // Same COLDBOX-1454 scenario as above, from the release side: a plain string in place of + // the expected array has no append() method, so writing back to it throws - and that + // must not propagate out of announce()'s finally block. + request.cbox_interceptorBufferPool = "not an array" + + iService.releaseLazyBuffer( new coldbox.system.web.context.InterceptorBuffer() ) + } + function testInterceptionPoints(){ // test registration again assertTrue( arrayLen( iService.getInterceptionPoints() ) gt 0 ); From b357598401cde8ad39d155749c3a4937d17e7d75 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 06:20:20 +0000 Subject: [PATCH 3/3] COLDBOX-1454 #comment fix CI failure: use a real setter for getUtil() in tests CI showed 16 errors on lucee@5/6, all in InterceptorserviceTest.cfc: mockController.$( "getUtil", ... ) made MockBox generate a stub method whose inferred return type was "string", then fail to cast the real Util component (or, in one test, a TestBox Stub) to it. Switched to mockController.setUtil( ... ), a real accessor-generated setter rather than a MockBox $() stub - the same pattern setLogBox()/ setWireBox()/setCacheBox() already use for their own properties in this same setup(), and which doesn't go through MockBox's return-type inference at all. Lucee 5/6 aren't available in this sandbox (no cached artifact, and ForgeBox is unreachable here) to reproduce directly, but this removes the exact code path CI's stack trace pointed at, in favor of a pattern already proven safe on every engine in this same file. Re-verified 27/27 passing on Lucee 7 across repeated runs. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KiK6DMek9iJMuYk2PzcjPj --- tests/specs/web/services/InterceptorserviceTest.cfc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/specs/web/services/InterceptorserviceTest.cfc b/tests/specs/web/services/InterceptorserviceTest.cfc index 300d82ed7..5b77e23b8 100755 --- a/tests/specs/web/services/InterceptorserviceTest.cfc +++ b/tests/specs/web/services/InterceptorserviceTest.cfc @@ -25,11 +25,15 @@ // Mock model Dependencies mockController.$( "getRequestService", mockRequestService ); - mockController.$( "getUtil", new coldbox.system.core.util.Util() ); mockController.setLogBox( mockLogBox ); mockController.setWireBox( mockWireBox ); mockController.setCacheBox( mockCacheBox ); + // A real setter, not a $() stub: MockBox's $() return-type inference for getUtil() + // misfires on Lucee 5/6 (it infers "string" and then fails to cast the real Util + // component to it), the same way setLogBox()/setWireBox()/setCacheBox() above sidestep + // it for their own properties. + mockController.setUtil( new coldbox.system.core.util.Util() ); mockRequestService.$( "getFlashScope", mockFlash ); mockLogBox.$( "getLogger", mockLogger ); @@ -214,7 +218,7 @@ // from code running inside an async/asyncAll announce()'s thread) must never touch the // same pool the request thread is using, or two real concurrent threads can pop/release // the same array at once and corrupt it. - mockController.$( "getUtil", mockBox.createStub().$( "inThread", true ) ) + mockController.setUtil( mockBox.createStub().$( "inThread", true ) ) var buffers = [] iService.listen( function( event, data, buffer ){