Skip to content

fix(core): add missing catch handler for forward-ref provider resolution#16133

Merged
kamilmysliwiec merged 1 commit intonestjs:masterfrom
coti-z:fix/injector-unhandled-promise-rejection
Dec 29, 2025
Merged

fix(core): add missing catch handler for forward-ref provider resolution#16133
kamilmysliwiec merged 1 commit intonestjs:masterfrom
coti-z:fix/injector-unhandled-promise-rejection

Conversation

@coti-z
Copy link
Contributor

@coti-z coti-z commented Dec 28, 2025

PR Checklist

PR Type

  • Bugfix

What is the current behavior?

Why is this a problem?

  1. No .catch() handler → The error becomes an unhandled promise rejection
  2. No settlementSignal.error() called → Other providers waiting on donePromise never receive the error
  3. Waiting providers hang forever or receive undefined instead of a proper error

When a forwardRef provider fails to load in a non-static context (REQUEST/TRANSIENT scope), the error is silently swallowed. This happens because the promise chain in resolveComponentHost uses fire-and-forget pattern without a .catch() handler, causing unhandled promise rejections.

What is the new behavior?

Added a .catch() handler that propagates errors through settlementSignal.error(), matching the existing error handling pattern used elsewhere in the injector.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

  • This fix follows the same error propagation pattern already used in loadInstance (lines 196-203).
  • Added testcode

When forwardRef provider fails to load in a non-static context
(REQUEST/TRANSIENT scope, the error was silently swallowed because the
promise was fire-and-forget without catch handler

This fix add catch handler that propagate the error through
ettlementSignal, ensuring proper error handling and preventing unhandled
promise rejection
@coveralls
Copy link

Pull Request Test Coverage Report for Build 2097797b-97a8-412e-a64d-a1f81ae9b453

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 89.759%

Totals Coverage Status
Change from base Build 17dabc9f-0c10-4dd0-bc58-5458b80cef25: 0.05%
Covered Lines: 7441
Relevant Lines: 8290

💛 - Coveralls

@kamilmysliwiec
Copy link
Member

Out of curiosity: could you provide a minimum reproduction repository (that shows when the unhandled error occurs)?

@coti-z
Copy link
Contributor Author

coti-z commented Dec 28, 2025

@kamilmysliwiec

Sure, I'll prepare a reproduction repository and mention you once it's ready.

@coti-z
Copy link
Contributor Author

coti-z commented Dec 28, 2025

@kamilmysliwiec Here's the reproduction repository:

https://github.com/coti-z/unhandledRejection

Steps:

  1. clone repository
  2. npm install
  3. npm run start:dev
  4. curl http://localhost:3000/

Expected: BService throws → error propagates → request fails with 500
Actual: BService throws → AService receives undefined → request returns ok + UnhandledPromiseRejectionWarning

Let me know if you need anything else!

@kamilmysliwiec
Copy link
Member

thanks @coti-z

@coti-z
Copy link
Contributor Author

coti-z commented Dec 28, 2025

Glad I could help!

@kamilmysliwiec kamilmysliwiec merged commit ad88fa8 into nestjs:master Dec 29, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants