COLDBOX-1420 Keep WireBox mappings registered when first metadata processing fails - #683
Open
homestar9 wants to merge 1 commit into
Open
COLDBOX-1420 Keep WireBox mappings registered when first metadata processing fails#683homestar9 wants to merge 1 commit into
homestar9 wants to merge 1 commit into
Conversation
…a 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.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a WireBox resiliency bug where an explicit mapping could be permanently removed from the Binder if its first mapping.process() attempt failed, causing subsequent lookups to incorrectly throw Injector.InstanceNotFoundException until a reinit. The change keeps failed mappings registered (but undiscovered) so later lookups can retry metadata processing and recover from transient load/compile/deploy issues.
Changes:
- Removed delete-on-error behavior around
mapping.process()inInjector.getInstance(). - Removed delete-on-error behavior in
Binder.processMappings()while preserving the “collect then throw” flow. - Added integration specs to prove mappings remain registered after initial processing failure, support recovery, and preserve multi-name mappings.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
system/ioc/Injector.cfc |
Stops deleting mappings when the first mapping.process() fails during getInstance(), enabling retry on later lookups. |
system/ioc/config/Binder.cfc |
Stops deleting mappings when processMappings() encounters a processing error, keeping the mapping available for future retry. |
tests/specs/ioc/InjectorLiveTest.cfc |
Adds specs covering retry behavior, recovery after file restoration, processMappings() behavior, and multi-name mapping retention. |
docs/plans/coldbox-1420-keep-failed-mappings.md |
Documents the bug, rationale, and verification notes for COLDBOX-1420. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
WireBox deletes a mapping when its first
mapping.process()call fails. The first caller gets the real error, but every later caller getsInjector.InstanceNotFoundExceptionuntil the app is reinitialized. A temporary load error (deploy race, file lock, compile timeout) becomes a lasting outage for explicitbinder.map().to()mappings, because nothing re-registers them. Models found by folder scanning can recover; explicit mappings cannot.This PR removes the delete-on-error behavior in both places that had it:
Injector.getInstance()— the try/catch only deleted the mapping and rethrew, so it is now a directmapping.process()call.Binder.processMappings()— thevariables.mappings.delete( key )line is removed; it still throws after the loop.A failed mapping now 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 the DI add methods (addDIConstructorArgument,addDIProperty,addDISetter) skip names that are already registered, so a retry does not double-register anything.The old behavior was also internally inconsistent: the delete removed only the looked-up name, so aliases registered during the failed processing kept pointing at the dead mapping, and a mapping registered under several names lost only one of them.
The delete dates to 2018 (
1adec53ce) with no ticket and no test covering it. The three othermapping.process()call sites (Builder x2, autowire) already keep the mapping on failure.Jira Issues
Type of change
Checklist
InjectorLiveTest.cfc: mapping kept after a failed lookup with a retry that re-throws the original error instead of InstanceNotFound; recovery once the missing file is restored;processMappings()keeps the failed mapping; multi-name mappings keep all names)