fix: hanging resolve bug (#212)

* fix: resolve hanging bug WIP * test: add extra test case * fixup! test: add extra test case * test: add test case for client environment --------- Co-authored-by: Andreas Karlsson <[email protected]>
spotify · Jan 10, 2025 · 7b22bc0 · 7b22bc0
1 parent 988707e
commit 7b22bc0
Show file tree

Hide file tree

Showing 3 changed files with 69 additions and 32 deletions.
diff --git a/packages/sdk/src/FlagResolution.ts b/packages/sdk/src/FlagResolution.ts
@@ -114,7 +114,7 @@ export class ReadyFlagResolution implements FlagResolution {
 
 ReadyFlagResolution.prototype.state = 'READY';
 
-class FailedFlagResolution implements FlagResolution {
+export class FailedFlagResolution implements FlagResolution {
   declare state: 'ERROR';
   constructor(readonly context: Value.Struct, readonly code: FlagEvaluation.ErrorCode, readonly message: string) {}
 

diff --git a/packages/sdk/src/FlagResolverClient.test.ts b/packages/sdk/src/FlagResolverClient.test.ts
@@ -10,7 +10,7 @@ import { setMaxListeners } from 'node:events';
 import { SdkId } from './generated/confidence/flags/resolver/v1/types';
 import { abortableSleep, FetchBuilder } from './fetch-util';
 import { ApplyFlagsRequest, ResolveFlagsRequest } from './generated/confidence/flags/resolver/v1/api';
-import { FlagResolution } from './FlagResolution';
+import { FailedFlagResolution, FlagResolution } from './FlagResolution';
 import { Telemetry } from './Telemetry';
 import { LibraryTraces_Library, LibraryTraces_TraceId, Platform } from './generated/confidence/telemetry/v1/telemetry';
 const RESOLVE_ENDPOINT = 'https://resolver.confidence.dev/v1/flags:resolve';
@@ -45,28 +45,52 @@ const fetchImplementation = async (request: Request): Promise<Response> => {
   }
 };
 
-resolveHandlerMock.mockImplementation(createFlagResolutionResponse);
+beforeEach(() => {
+  resolveHandlerMock.mockImplementation(createFlagResolutionResponse);
+});
+
+afterEach(() => {
+  resolveHandlerMock.mockClear();
+  applyHandlerMock.mockClear();
+});
 
 describe('Client environment Evaluation', () => {
   // const flagResolutionResponseJson = JSON.stringify(createFlagResolutionResponse());
-  const instanceUnderTest = new FetchingFlagResolverClient({
-    fetchImplementation,
-    clientSecret: 'secret',
-    applyTimeout: 10,
-    sdk: {
-      id: SdkId.SDK_ID_JS_CONFIDENCE,
-      version: 'test',
-    },
-    environment: 'client',
-    resolveTimeout: 10,
-    telemetry: new Telemetry({ disabled: true, logger: { warn: jest.fn() }, environment: 'client' }),
+  let instanceUnderTest: FetchingFlagResolverClient;
+
+  beforeEach(() => {
+    instanceUnderTest = new FetchingFlagResolverClient({
+      fetchImplementation,
+      clientSecret: 'secret',
+      applyTimeout: 10,
+      sdk: {
+        id: SdkId.SDK_ID_JS_CONFIDENCE,
+        version: 'test',
+      },
+      environment: 'client',
+      resolveTimeout: 10,
+      telemetry: new Telemetry({ disabled: true, logger: { warn: jest.fn() }, environment: 'client' }),
+    });
+  });
+
+  it('should resolve a FailedFlagResolution on fetch errors', async () => {
+    resolveHandlerMock.mockRejectedValue(new Error('Test error'));
+    const flagResolution = await instanceUnderTest.resolve({}, []);
+    expect(flagResolution).toBeInstanceOf(FailedFlagResolution);
+    // Expect this error to log as a Resolve timeout in the client environment
+    // This is due the request logic that's only used in the client environment
+    expect(flagResolution.evaluate('testflag', {})).toEqual({
+      errorCode: 'TIMEOUT',
+      errorMessage: 'Resolve timeout',
+      reason: 'ERROR',
+      value: {},
+    });
   });
 
   describe('apply', () => {
     it('should send an apply event', async () => {
       const flagResolution = await instanceUnderTest.resolve({}, []);
       flagResolution.evaluate('testflag.bool', false);
-
       const [applyRequest] = await nextMockArgs(applyHandlerMock);
       expect(applyRequest).toMatchObject({
         clientSecret: 'secret',
@@ -81,23 +105,23 @@ describe('Client environment Evaluation', () => {
         ],
       });
     });
-  });
 
-  it('should apply when a flag has no segment match', async () => {
-    const flagResolution = await instanceUnderTest.resolve({}, []);
-    flagResolution.evaluate('no-seg-flag.enabled', false);
-    const [applyRequest] = await nextMockArgs(applyHandlerMock);
-    expect(applyRequest).toMatchObject({
-      clientSecret: 'secret',
-      resolveToken: dummyResolveToken,
-      sendTime: expect.any(Date),
-      sdk: { id: 13, version: 'test' },
-      flags: [
-        {
-          applyTime: expect.any(Date),
-          flag: 'flags/no-seg-flag',
-        },
-      ],
+    it('should apply when a flag has no segment match', async () => {
+      const flagResolution = await instanceUnderTest.resolve({}, []);
+      flagResolution.evaluate('no-seg-flag.enabled', false);
+      const [applyRequest] = await nextMockArgs(applyHandlerMock);
+      expect(applyRequest).toMatchObject({
+        clientSecret: 'secret',
+        resolveToken: dummyResolveToken,
+        sendTime: expect.any(Date),
+        sdk: { id: 13, version: 'test' },
+        flags: [
+          {
+            applyTime: expect.any(Date),
+            flag: 'flags/no-seg-flag',
+          },
+        ],
+      });
     });
   });
 });
@@ -270,6 +294,18 @@ describe('Backend environment Evaluation', () => {
       });
     });
   });
+
+  it('should resolve a FailedFlagResolution on fetch errors', async () => {
+    resolveHandlerMock.mockRejectedValue(new Error('Test error'));
+    const flagResolution = await instanceUnderTest.resolve({}, ['testflag']);
+    expect(flagResolution).toBeInstanceOf(FailedFlagResolution);
+    expect(flagResolution.evaluate('testflag', {})).toEqual({
+      errorCode: 'GENERAL',
+      errorMessage: '500: Test error',
+      reason: 'ERROR',
+      value: {},
+    });
+  });
 });
 
 describe('intercept', () => {
@@ -451,6 +487,7 @@ describe('CachingFlagResolverClient', () => {
     expect(pendingResolution.signal.aborted).toBe(true);
   });
 });
+
 function nextCall(mock: jest.Mock): Promise<void> {
   return new Promise(resolve => {
     const impl = mock.getMockImplementation();

diff --git a/packages/sdk/src/FlagResolverClient.ts b/packages/sdk/src/FlagResolverClient.ts
@@ -158,7 +158,7 @@ export class FetchingFlagResolverClient implements FlagResolverClient {
           if (error instanceof ResolveError) {
             return FlagResolution.failed(context, error.code, error.message);
           }
-          throw error;
+          return FlagResolution.failed(context, 'GENERAL', error.message);
         })
         .finally(() => {
           this.markLatency(Date.now() - start);