From 5feb11c982c81bd17a3af29a548a0146cde93a3e Mon Sep 17 00:00:00 2001 From: David Levin Date: Mon, 17 Aug 2026 17:02:55 -0700 Subject: [PATCH] COLDBOX-1420 fix: keep WireBox mappings registered when first metadata processing fails WireBox deleted a mapping when its first mapping.process() call failed. The first caller got the real error, but every later caller got Injector.InstanceNotFoundException until the app was reinitialized. A temporary load error (deploy race, file lock, compile timeout) became a lasting outage for explicit binder.map().to() mappings, which nothing re-registers. Now a failed mapping stays registered and unprocessed, so the next lookup retries processing. The first caller still gets the original error. Retrying is safe: Mapping.process() only marks a mapping discovered at the very end, runs inside an exclusive lock, and all the DI add methods skip names that are already registered. Applied in both places that deleted on error: Injector.getInstance() and Binder.processMappings(). Tests: 4 new specs in InjectorLiveTest.cfc. Full wirebox suite passes on Adobe 2023, BoxLang 1.15, and Lucee 5.4. --- .../coldbox-1420-keep-failed-mappings.md | 79 +++++++++++++ system/ioc/Injector.cfc | 13 +-- system/ioc/config/Binder.cfc | 3 +- tests/specs/ioc/InjectorLiveTest.cfc | 108 ++++++++++++++++++ 4 files changed, 191 insertions(+), 12 deletions(-) create mode 100644 docs/plans/coldbox-1420-keep-failed-mappings.md diff --git a/docs/plans/coldbox-1420-keep-failed-mappings.md b/docs/plans/coldbox-1420-keep-failed-mappings.md new file mode 100644 index 000000000..43aa3e151 --- /dev/null +++ b/docs/plans/coldbox-1420-keep-failed-mappings.md @@ -0,0 +1,79 @@ +# COLDBOX-1420: Keep WireBox mappings when first metadata processing fails + +## Context + +A handoff file from another repo (`plans/handoff-coldbox-jira-mapping-deletion.md`) reported this bug: when a WireBox mapping's first `mapping.process()` call fails (for example, the CFC file is briefly missing during a deploy), WireBox deletes the mapping from the binder. The first caller gets the real error. Every later caller gets `Injector.InstanceNotFoundException` until the app is reinitialized. A temporary error becomes a lasting outage. + +The handoff was verified against this codebase. The bug is real, the code sites it names are exact, and the fix it suggests is the right one. + +## Claims table (verification of the handoff) + +| # | Claim | Verdict | Evidence | +|---|-------|---------|----------| +| 1 | `getInstance()` in `system/ioc/Injector.cfc` deletes the mapping in a catch block around `mapping.process()` | **Confirmed** | `system/ioc/Injector.cfc:577-588` (before the fix), inside `getInstance()`. | +| 2 | `Binder.processMappings()` has the same delete-on-error behavior | **Confirmed** | `system/ioc/config/Binder.cfc:1366-1373` (before the fix) — `variables.mappings.delete( key )` in the catch. | +| 3 | After deletion, later lookups throw `Injector.InstanceNotFoundException` | **Confirmed** | `system/ioc/Injector.cfc:561-566` throws exactly that type when `mappingExists()` is false and nothing else finds the name. | +| 4 | Models found by folder scanning may recover; explicit `binder.map().to()` mappings cannot | **Confirmed** | `system/ioc/Injector.cfc:524-571`: a missing mapping goes through `locateInstance()` scan locations and `registerNewInstance()`. An explicit mapping's path is not in a scan location, so nothing re-registers it. | +| 5 | Affects the `development` branch | **Confirmed** | Both delete sites existed on the branch (version 8.2.0 in box.json). | +| 6 | Affects ColdBox 8.1.0+34 on Lucee 6.3.4; repro script fails 10/10 | **Unverifiable** | The repro script was not run here. The code path makes the described result expected. | +| 7 | JIRA tickets COLDBOX-1420 and COLDBOX-1419 exist | **Unverifiable** | No JIRA access from the review environment. | +| 8 | Production failure story (`SQLTokenStorage@rememberMe`) | **Unverifiable** | Belongs to the other repo. Consistent with the confirmed code path. | + +## Why the fix is safe + +- **Why the delete existed:** added April 2018 in commit `1adec53ce` ("New caching and some error handling") with no ticket and no comment beyond "Remove bad mapping". No test asserted this behavior. +- **Retry is safe.** `Mapping.process()` sets `variables.discovered = true` only at the very end, inside an exclusive lock. A failed run leaves `discovered = false`, so a retry runs the whole thing again. The add methods (`addDIConstructorArgument`, `addDIProperty`, `addDISetter`) skip names that are already registered, and the annotation checks are guarded by `if ( !len( ... ) )`, so a retry does not double-register dependencies. The common transient failure (missing or uncompilable file) happens at the metadata fetch, before any state is written. +- **The old delete was already inconsistent.** `process()` registers alias keys in the binder before later steps that can throw. The delete removed only the one requested name, leaving alias keys pointing at the failed mapping. A mapping registered under several names (`map([ "a", "b" ])`) lost only the name that was looked up. +- **Other `process()` call sites already keep the mapping on failure:** `system/ioc/Builder.cfc:802-803`, `system/ioc/Builder.cfc:1033-1035`, `system/ioc/Injector.cfc` (autowire path). None of them delete on error. +- **Behavior change to be aware of:** for a mapping whose path is permanently wrong, later lookups now throw the original metadata/load error on every call instead of `InstanceNotFoundException` after the first. That error is more informative. No code in the framework or its tests relied on the old type for this case. + +## Decision record + +**Chosen: remove the deletion in both places.** In `getInstance()`, the try/catch only deleted and rethrew, so the whole try/catch was removed and `mapping.process()` is called directly. In `processMappings()`, only the `variables.mappings.delete( key )` line was removed; the collect-then-throw flow stays. + +Alternatives rejected: + +1. **Delete only scan-discovered mappings, keep explicit ones** (the handoff's fallback idea). Rejected: there is no flag on `Mapping` that records how it was created, so this needs new state for no benefit. Keeping a scanned mapping is also fine — a retry re-processes the same path, which is exactly what re-discovery would produce. +2. **Do nothing here; work around it in the consuming app.** Rejected: the bug is in ColdBox and affects every consumer. The workaround (eagerly load and re-register in a catch) treats the symptom and must be repeated in every app. + +## Corrections sent back to the source repo + +The handoff was accurate. Only small notes: + +- The code comment was `// Remove bad mapping`, not the paraphrase in the handoff. No behavior difference. +- The delete-on-error dates to April 2018 (commit `1adec53ce`), so it affects far more versions than 8.1.0+34. The fix lands in 8.2.0 (current development version). +- Extra detail: the delete removed only the looked-up name, so aliases registered during the failed processing kept pointing at the dead mapping. The bug was worse than described for multi-name mappings. + +## Changes made + +1. `system/ioc/Injector.cfc` — `getInstance()` now calls `mapping.process()` directly; the delete-and-rethrow catch block is gone. +2. `system/ioc/config/Binder.cfc` — `processMappings()` no longer deletes a mapping whose processing failed; it still throws after the loop. +3. `tests/specs/ioc/InjectorLiveTest.cfc` — new feature block "Mappings survive a failed first processing (COLDBOX-1420)" with four specs: + - Explicit mapping to a missing path: first and second lookups both throw the original error (not `InstanceNotFoundException`) and the mapping stays registered. + - Recovery: mapping whose file is missing on the first lookup builds fine on the second lookup after the file is written. + - `processMappings()` throws but keeps the failed mapping registered. + - A mapping registered under two names keeps both names after a failed lookup. + +## Verification (actual results) + +`box run-script tests:wirebox` (33 bundles, 189 specs) run against three engines on port 8599: + +| Engine | Result | +|--------|--------| +| Adobe ColdFusion 2023.0.23 | 189 pass, 0 fail, 0 error | +| BoxLang 1.15.0 (CFML compat) | 187 pass, 2 skipped (engine-specific skips), 0 fail, 0 error | +| Lucee 5.4.8 | 188 pass, 1 skipped (Lucee-specific skip), 0 fail, 0 error | + +`box cfformat run` on the three touched files produced no changes beyond the fix itself. + +Note: one transient error (`InjectorCreationTest.testProviderMethods`, "Injector not found in scope registration information") appeared in a single early run and never again across five later full runs on three engines. It came from leftover application-scope state after an ad-hoc single-bundle run in the same server boot, not from this change. + +## Rollback + +Revert the fix commit. The change is two localized edits plus additive tests. No config, data, or API surface changes. + +## Out of scope + +- Duplicate entries possible in a mapping's alias array if a retry re-runs alias processing — cosmetic, only reachable with a deterministic failure, not worth extra guards. +- COLDBOX-1419 (scheduler missing module mappings) — separate ticket (see `docs/plans/scheduler-module-mappings.md`). +- The consuming app's rememberMe workaround removal — the other repo's task after this ships. diff --git a/system/ioc/Injector.cfc b/system/ioc/Injector.cfc index 54f8145ca..66d56bb00 100644 --- a/system/ioc/Injector.cfc +++ b/system/ioc/Injector.cfc @@ -575,16 +575,9 @@ component serializable="false" accessors="true" { // Check if the mapping has been discovered yet, and if it hasn't it must be autowired enabled in order to process. if ( NOT mapping.isDiscovered() ) { - try { - // process inspection of instance - mapping.process( binder = variables.binder, injector = this ); - } catch ( any e ) { - // Remove bad mapping - var mappings = variables.binder.getMappings(); - mappings.delete( name ); - // rethrow - throw( object = e ); - } + // process inspection of instance + // If this fails, the mapping stays registered and unprocessed so a later lookup can retry (COLDBOX-1420) + mapping.process( binder = variables.binder, injector = this ); } // Request object from scope now, we now have it from the scope created, initialized and wired diff --git a/system/ioc/config/Binder.cfc b/system/ioc/config/Binder.cfc index 855d4b50c..c60a93375 100644 --- a/system/ioc/config/Binder.cfc +++ b/system/ioc/config/Binder.cfc @@ -1367,8 +1367,7 @@ component accessors="true" { // process the metadata arguments.thisMapping.process( binder = this, injector = variables.injector ); } catch ( any e ) { - // Remove bad mapping - variables.mappings.delete( key ); + // Keep the mapping registered and unprocessed so a later lookup can retry (COLDBOX-1420) mappingError = e; } } ); diff --git a/tests/specs/ioc/InjectorLiveTest.cfc b/tests/specs/ioc/InjectorLiveTest.cfc index 1c44cdc0d..b9aa3ffcb 100644 --- a/tests/specs/ioc/InjectorLiveTest.cfc +++ b/tests/specs/ioc/InjectorLiveTest.cfc @@ -130,6 +130,114 @@ component extends="tests.resources.BaseIntegrationTest" { } ); } ); } ); + + feature( "Mappings survive a failed first processing (COLDBOX-1420)", function(){ + beforeEach( function( currentSpec ){ + variables.injector1420 = new coldbox.system.ioc.Injector(); + variables.ghostPath1420 = expandPath( "/tests/resources/Ghost1420.cfc" ); + } ); + + afterEach( function( currentSpec ){ + if ( fileExists( variables.ghostPath1420 ) ) { + fileDelete( variables.ghostPath1420 ); + } + } ); + + story( "I want explicit mappings to stay registered when their first processing fails", function(){ + given( "an explicit mapping to a path that does not exist", function(){ + then( "the mapping stays registered and a retry throws the original error, not InstanceNotFoundException", function(){ + injector1420 + .getBinder() + .map( "ghost1420@demo" ) + .to( "tests.resources.DoesNotExist1420" ); + + var firstErrorType = "NONE"; + try { + injector1420.getInstance( "ghost1420@demo" ); + } catch ( any e ) { + firstErrorType = e.type; + } + expect( firstErrorType ).notToBe( "NONE", "The first getInstance() should have thrown" ); + expect( firstErrorType ).notToBe( "Injector.InstanceNotFoundException" ); + + // The mapping must still be registered after the failure + expect( injector1420.getBinder().mappingExists( "ghost1420@demo" ) ).toBeTrue(); + + // A second lookup retries processing and throws the original error again + var secondErrorType = "NONE"; + try { + injector1420.getInstance( "ghost1420@demo" ); + } catch ( any e ) { + secondErrorType = e.type; + } + expect( secondErrorType ).notToBe( "NONE", "The second getInstance() should have thrown" ); + expect( secondErrorType ).notToBe( "Injector.InstanceNotFoundException" ); + } ); + } ); + + given( "an explicit mapping whose file is missing on the first lookup but present on the second", function(){ + then( "the second lookup recovers and builds the instance", function(){ + injector1420 + .getBinder() + .map( "ghostFile1420@demo" ) + .to( "tests.resources.Ghost1420" ); + + // First lookup fails because the file does not exist yet + var firstErrorType = "NONE"; + try { + injector1420.getInstance( "ghostFile1420@demo" ); + } catch ( any e ) { + firstErrorType = e.type; + } + expect( firstErrorType ).notToBe( "NONE", "The first getInstance() should have thrown" ); + expect( injector1420.getBinder().mappingExists( "ghostFile1420@demo" ) ).toBeTrue(); + + // Restore the file and retry the same mapping + fileWrite( variables.ghostPath1420, "component {}" ); + var instance = injector1420.getInstance( "ghostFile1420@demo" ); + expect( isObject( instance ) ).toBeTrue(); + } ); + } ); + + given( "a mapping registered under several names to a bad path", function(){ + then( "all names stay registered after a failed lookup", function(){ + injector1420 + .getBinder() + .map( [ "aliasA1420", "aliasB1420" ] ) + .to( "tests.resources.DoesNotExist1420" ); + + try { + injector1420.getInstance( "aliasA1420" ); + } catch ( any e ) { + // Expected: the bad path makes processing fail. This spec only checks the mappings below. + } + + expect( injector1420.getBinder().mappingExists( "aliasA1420" ) ).toBeTrue(); + expect( injector1420.getBinder().mappingExists( "aliasB1420" ) ).toBeTrue(); + } ); + } ); + } ); + + story( "I want processMappings() to keep mappings that fail processing", function(){ + given( "a binder with a mapping to a bad path", function(){ + then( "processMappings() throws but keeps the mapping registered", function(){ + injector1420 + .getBinder() + .map( "bad1420" ) + .to( "tests.resources.DoesNotExist1420" ); + + var errorType = "NONE"; + try { + injector1420.getBinder().processMappings(); + } catch ( any e ) { + errorType = e.type; + } + expect( errorType ).notToBe( "NONE", "processMappings() should have thrown" ); + expect( injector1420.getBinder().mappingExists( "bad1420" ) ).toBeTrue(); + } ); + } ); + } ); + } ); } }