COLDBOX-1420 Keep WireBox mappings registered when first metadata processing fails

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.

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

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;
 				}
 			} );

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();
+					} );
+				} );
+			} );
+		} );
 	}
 
 }