fix(core): flaky durable provider, remove instance on error #13953

.gitignore

@@ -50,3 +50,4 @@ build/config\.gypi
 
 .npmrc
 pnpm-lock.yaml
+/.history

integration/scopes/e2e/durable-providers.spec.ts

@@ -27,10 +27,14 @@ describe('Durable providers', () => {
       tenantId: number,
       end: (err?: any) => void,
       endpoint = '/durable',
+      opts: {
+        forceError: boolean;
+      } = { forceError: false },
     ) =>
       request(server)
         .get(endpoint)
         .set({ ['x-tenant-id']: tenantId })
+        .set({ ['x-force-error']: opts.forceError ? 'true' : 'false' })
         .end((err, res) => {
           if (err) return end(err);
           end(res);
@@ -84,6 +88,23 @@ describe('Durable providers', () => {
       );
       expect(result.body).deep.equal({ tenantId: '3' });
     });
+
+    it(`should not cache durable providers that throw errors`, async () => {
+      let result: request.Response;
+
+      result = await new Promise<request.Response>(resolve =>
+        performHttpCall(10, resolve, '/durable/echo', { forceError: true }),
+      );
+
+      expect(result.statusCode).equal(412);
+
+      // The second request should be successful
+      result = await new Promise<request.Response>(resolve =>
+        performHttpCall(10, resolve, '/durable/echo'),
+      );
+
+      expect(result.body).deep.equal({ tenantId: '10' });
+    });
   });
 
   after(async () => {

integration/scopes/src/durable/durable-context-id.strategy.ts

@@ -6,6 +6,8 @@ const tenants = new Map<string, ContextId>();
 export class DurableContextIdStrategy implements ContextIdStrategy {
   attach(contextId: ContextId, request: Request) {
     const tenantId = request.headers['x-tenant-id'] as string;
+    const forceError = request.headers['x-force-error'] === 'true';
+
     let tenantSubTreeId: ContextId;
 
     if (tenants.has(tenantId)) {
@@ -14,10 +16,18 @@ export class DurableContextIdStrategy implements ContextIdStrategy {
       tenantSubTreeId = { id: +tenantId } as ContextId;
       tenants.set(tenantId, tenantSubTreeId);
     }
+
+    const payload: {
+      tenantId: string;
+      forceError?: boolean;
+    } = { tenantId };
+    if (forceError) {
+      payload.forceError = true;
+    }
     return {
       resolve: (info: HostComponentInfo) =>
         info.isTreeDurable ? tenantSubTreeId : contextId,
-      payload: { tenantId },
+      payload,
     };
   }
 }

integration/scopes/src/durable/durable.service.ts

@@ -1,11 +1,23 @@
-import { Inject, Injectable, Scope } from '@nestjs/common';
+import {
+  Inject,
+  Injectable,
+  PreconditionFailedException,
+  Scope,
+} from '@nestjs/common';
 import { REQUEST } from '@nestjs/core';
 
 @Injectable({ scope: Scope.REQUEST, durable: true })
 export class DurableService {
   public instanceCounter = 0;
 
-  constructor(@Inject(REQUEST) public readonly requestPayload: unknown) {}
+  constructor(
+    @Inject(REQUEST)
+    public readonly requestPayload: { tenantId: string; forceError: boolean },
+  ) {
+    if (requestPayload.forceError) {
+      throw new PreconditionFailedException('Forced error');
+    }
+  }
 
   greeting() {
     ++this.instanceCounter;

packages/core/injector/injector.ts

@@ -170,6 +170,11 @@ export class Injector {
         inquirer,
       );
     } catch (err) {
+      wrapper.removeInstanceByContextId(
+        this.getContextId(contextId, wrapper),
+        inquirerId,
+      );
+
       settlementSignal.error(err);
       throw err;
     }

packages/core/injector/instance-wrapper.ts

@@ -168,6 +168,21 @@ export class InstanceWrapper<T = any> {
     collection.set(contextId, value);
   }
 
+  public removeInstanceByContextId(contextId: ContextId, inquirerId?: string) {
+    if (this.scope === Scope.TRANSIENT && inquirerId) {
+      return this.removeInstanceByInquirerId(contextId, inquirerId);
+    }
+    this.values.delete(contextId);
+  }
+
+  public removeInstanceByInquirerId(contextId: ContextId, inquirerId: string) {
+    const collection = this.transientMap.get(inquirerId);
+    if (!collection) {
+      return;
+    }
+    collection.delete(contextId);
+  }
+
   public addCtorMetadata(index: number, wrapper: InstanceWrapper) {
     if (!this[INSTANCE_METADATA_SYMBOL].dependencies) {
       this[INSTANCE_METADATA_SYMBOL].dependencies = [];

packages/core/test/injector/instance-wrapper.spec.ts

@@ -1,6 +1,7 @@
 import { Scope } from '@nestjs/common';
 import { expect } from 'chai';
 import * as sinon from 'sinon';
+import { createContextId } from '../../helpers';
 import { STATIC_CONTEXT } from '../../injector/constants';
 import { InstanceWrapper } from '../../injector/instance-wrapper';
 
@@ -737,6 +738,53 @@ describe('InstanceWrapper', () => {
     });
   });
 
+  describe('removeInstanceByContextId', () => {
+    describe('without inquirer', () => {
+      it('should remove instance for given context', () => {
+        const wrapper = new InstanceWrapper({
+          scope: Scope.TRANSIENT,
+        });
+
+        const contextId = createContextId();
+        wrapper.setInstanceByContextId(contextId, { instance: {} });
+
+        const existingContext = wrapper.getInstanceByContextId(contextId);
+        expect(existingContext.instance).to.be.not.undefined;
+        wrapper.removeInstanceByContextId(contextId);
+
+        const removedContext = wrapper.getInstanceByContextId(contextId);
+        expect(removedContext.instance).to.be.undefined;
+      });
+    });
+
+    describe('when transient and inquirer has been passed', () => {
+      it('should remove instance for given context', () => {
+        const wrapper = new InstanceWrapper({
+          scope: Scope.TRANSIENT,
+        });
+
+        wrapper.setInstanceByContextId(
+          STATIC_CONTEXT,
+          { instance: {} },
+          'inquirerId',
+        );
+
+        const existingContext = wrapper.getInstanceByContextId(
+          STATIC_CONTEXT,
+          'inquirerId',
+        );
+        expect(existingContext.instance).to.be.not.undefined;
+        wrapper.removeInstanceByContextId(STATIC_CONTEXT, 'inquirerId');
+
+        const removedContext = wrapper.getInstanceByContextId(
+          STATIC_CONTEXT,
+          'inquirerId',
+        );
+        expect(removedContext.instance).to.be.undefined;
+      });
+    });
+  });
+
   describe('isInRequestScope', () => {
     describe('when tree and context are not static and is not transient', () => {
       it('should return true', () => {