fix: return the original promise from an instrumented method

[johnnyhuirilef]

PR Checklist

• The commit messages follow the Angular convention, as commitlint checks them: https://github.com/nestjs/observe/blob/master/.commitlintrc.json
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A (issues are disabled on this repo)

Hi! 👋 With instrumentation on, a provider method that returns a promise with extra members hands the caller a different promise. The members are gone. A got request loses .json(), .text() and .buffer(). execa 10 loses kill() and stdout. A cancelable promise loses cancel(), so the work keeps running.

The cause is in src/instrument/create-instance-decorator.instrument.ts, lines 148 to 150: return result.then(onReturnValue).catch(onError). Each call to then or catch builds a new promise. The caller gets the last one. A Promise subclass also builds two extra instances through Symbol.species.

OutgoingSpanRecorder.endWhenSettled in src/outgoing/outgoing-span.recorder.ts already solves this for outgoing spans. It watches the promise on a side branch and returns the promise untouched. Its comment says the caller's chain must see the same value and the same rejection as without the span.

I ran the repro on 0.3.7:

--- control: no instrument option ---
GET /data:     200 {"a":1}
GET /cancel:   200 {"cancelExists":true,"ending":"rejected: CancelError"}
logged errors: 0
first error:   none

--- with instrument: ObserveInstrument ---
GET /data:     500 {"statusCode":500,"message":"Internal server error"}
GET /cancel:   200 {"cancelExists":false,"ending":"finished"}
logged errors: 1
first error:   this.data.fetchJson(...).json is not a function

RESULT: BUG

Until a release has the fix, createObserveModule({ skipInstrumentation: (instance) => instance instanceof DataService }) keeps such a provider working.

What is the new behavior?

• The wrapper returns the same promise object the method returned. Members and Promise subclasses stay as they are.
• The span ends on a side branch of that promise, when it settles. A rejection ends the span with the error, as before.
• The caller sees the same value and the same rejection object. Nothing is re-thrown on the side branch.
• A throw while closing the span is swallowed on the side branch. It does not reach the caller, and it does not become an unhandled rejection.
• Observing a Promise subclass now builds one derived instance. The old chain built two.
• Detection is still instanceof Promise. Other thenables are not touched.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

No API changes. Two behaviors differ. First, it makes the same trade as endWhenSettled. The side branch marks the original promise as handled, so a caller that drops a rejecting promise no longer raises unhandledRejection. Before, the new chain was the one left unhandled, so the process saw the event. Re-throwing on the side branch brings the event back, but it also raises it for callers that do handle the rejection. Second, a throw while the span closes used to reject the caller and close the span twice. Now the caller keeps the real value or rejection.

Other information

The unit specs call the decorator on a provider method that returns a promise. They check:

• The decorated call returns the same promise (toBe), with its cancel member intact, and the span ends with no error.
• A rejection reaches the caller as the same error object, and the span records that error once.
• The span ends when the promise settles, not when the method returns.
• A throw from the registry while the span closes leaves the caller's value and rejection unchanged, for both outcomes. Vitest fails the run on an unhandled rejection, so these specs also guard that point.
• A Promise subclass builds one derived instance.

The new int spec boots a real Nest app. A provider returns a promise with a json() member, and GET /upstream calls it and returns 200. It returns 500 on master.

The specs for identity and for the subclass fail on master and pass with the fix. I also changed the fix in five ways. I went back to the old chain, returned the side branch, re-threw on the side branch, removed the guard around onReturnValue, and removed the onError call. A spec or an unhandled rejection failed every time.

Three other wrappers also return a new promise: operation-trace.registry.ts (line 873, behind the async createSpan), job-trace-runner.ts (line 330) and ws-observe-agent.service.ts (line 222, with finally). I left them out to keep this PR on one wrapper. I am happy to open a follow-up for them. 🙂