Fix IndexHostSingleton holding onto one instance of `ManageIndexesA…

…PI` (#224) ## Problem When you instantiate multiple instances of `Pinecone` using different API keys, there's a conflict when the `IndexHostSingleton` attempts to resolve unknown index host URLs through the control plane by creating an instance of `ManageIndexesApi`. Inside `IndexHostSingleton` there's a variable `indexOperationsApi` which is built once using `PineconeConfiguration`, but then we re-use that same API key for subsequent `_describeIndex` calls. This is bad, and leads to issues like this: #220 ## Solution Rebuild the `ManageIndexesApi` object each time we need to call `_describeIndex` to add an index host to the cache, because the `PineconeConfiguration` and API key may have changed. If there are multiple client instances for different keys, they're using the same cache. **Bonus** While running integration tests locally, I noticed we were seeing a lot of warnings about open handles from possibly non-awaited async functions: ![Screenshot 2024-05-31 at 6 39 52 PM](https://github.com/pinecone-io/pinecone-ts-client/assets/119623786/59551d6a-3a60-4903-af74-ee32b6a2a4ad) We were missing a bunch of `awaits` in front of `assertWithRetries` calls, so I cleaned that up as well. ## Type of Change - [X] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update - [ ] Infrastructure change (CI configs, etc) - [ ] Non-code change (docs, etc) - [ ] None of the above: (explain here) ## Test Plan Added a unit test to specifically validate the behavior inside `IndexHostSingleton`. To test you can use a simple script locally to validate instantiating multiple clients with multiple API keys. This is what I was working off of to test: ``` import { Pinecone } from "@pinecone-database/pinecone"; await testTwo(); await testOne(); async function testOne() { const clientOne = new Pinecone({ apiKey: "API_KEY_1", }); const indexOne = clientOne.index("test-index"); console.log("Index 1: ", await indexOne.describeIndexStats()); } async function testTwo() { const clientTwo = new Pinecone({ apiKey: "API_KEY_2", }); const indexTwo = clientTwo.index("test-index-2"); console.log("Index 2: ", await indexTwo.describeIndexStats()); } ``` Running the above on current `main` (v2.2.1) ```bash $ tsc && node dist/index.js Index 2: { namespaces: {}, dimension: 4, indexFullness: 0, totalRecordCount: 0 } /Users/austin/workspace/pinecone-ts-client/dist/errors/http.js:181 return new PineconeAuthorizationError(failedRequestInfo); ^ PineconeAuthorizationError: The API key you provided was rejected while calling https://test-index-<redacted>/describe_index_stats. Please check your configuration values and try again. You can find the configuration values for your project in the Pinecone developer console at https://app.pinecone.io at mapHttpStatusError (/Users/austin/workspace/pinecone-ts-client/dist/errors/http.js:181:20) at /Users/austin/workspace/pinecone-ts-client/dist/errors/handling.js:65:69 at step (/Users/austin/workspace/pinecone-ts-client/dist/errors/handling.js:33:23) at Object.next (/Users/austin/workspace/pinecone-ts-client/dist/errors/handling.js:14:53) at fulfilled (/Users/austin/workspace/pinecone-ts-client/dist/errors/handling.js:5:58) at process.processTicksAndRejections (node:internal/process/task_queues:95:5) { cause: undefined } Node.js v18.17.1 ``` Running the same with the fix implemented ```bash $ tsc && node dist/index.js Index 2: { namespaces: {}, dimension: 4, indexFullness: 0, totalRecordCount: 0 } Index 1: { namespaces: { hello: { recordCount: 4 }, 'another-namespace': { recordCount: 10000 }, '': { recordCount: 20004 }, 'test-namespace': { recordCount: 10000 } }, dimension: 4, indexFullness: 0, totalRecordCount: 40008 } ``` --- - To see the specific tasks where the Asana app for GitHub is being used, see below: - https://app.asana.com/0/0/1207448364074962
pinecone-io · May 31, 2024 · 4dee2e6 · 4dee2e6
1 parent 014e793
commit 4dee2e6
Show file tree

Hide file tree

Showing 4 changed files with 68 additions and 25 deletions.
diff --git a/src/data/__tests__/indexHostSingleton.test.ts b/src/data/__tests__/indexHostSingleton.test.ts
@@ -1,20 +1,22 @@
 import { IndexHostSingleton } from '../indexHostSingleton';
 
 const mockDescribeIndex = jest.fn();
+const mockIndexOperationsBuilder = jest.fn();
 
 jest.mock('../../control', () => {
   const realControl = jest.requireActual('../../control');
-
   return {
     ...realControl,
     describeIndex: () => mockDescribeIndex,
+    indexOperationsBuilder: (config) => mockIndexOperationsBuilder(config),
   };
 });
 
 describe('IndexHostSingleton', () => {
   afterEach(() => {
     IndexHostSingleton._reset();
     mockDescribeIndex.mockReset();
+    mockIndexOperationsBuilder.mockReset();
   });
 
   test('calls describeIndex to resolve host for a specific apiKey and indexName, prepends protocol to host', async () => {
@@ -140,4 +142,50 @@ describe('IndexHostSingleton', () => {
     await IndexHostSingleton.getHostUrl(pineconeConfig, 'test-index');
     expect(mockDescribeIndex).toHaveBeenCalledTimes(1);
   });
+
+  test.only('calling getHostUrl with different apiKey configurations should instantiate new ManageIndexesApi classes', async () => {
+    const pineconeConfig1 = { apiKey: 'test-key-1' };
+    const pineconeConfig2 = { apiKey: 'test-key-2' };
+
+    mockDescribeIndex
+      .mockResolvedValueOnce({
+        name: 'index-1',
+        dimensions: 10,
+        metric: 'cosine',
+        host: 'test-host-1',
+        spec: { pod: { pods: 1, replicas: 1, shards: 1, podType: 'p1.x1' } },
+        status: { ready: true, state: 'Ready' },
+      })
+      .mockResolvedValueOnce({
+        name: 'index-2',
+        dimensions: 10,
+        metric: 'cosine',
+        host: 'test-host-2',
+        spec: { pod: { pods: 1, replicas: 1, shards: 1, podType: 'p1.x1' } },
+        status: { ready: true, state: 'Ready' },
+      });
+    mockIndexOperationsBuilder.mockReturnValue({ test: 'one', test2: 'two' });
+
+    await IndexHostSingleton.getHostUrl(pineconeConfig1, 'index-1');
+    await IndexHostSingleton.getHostUrl(pineconeConfig2, 'index-2');
+
+    expect(mockDescribeIndex).toHaveBeenCalledTimes(2);
+    expect(mockDescribeIndex).toHaveBeenNthCalledWith(1, 'index-1');
+    expect(mockDescribeIndex).toHaveBeenNthCalledWith(2, 'index-2');
+
+    console.log(
+      'mockIndexOperationsBuilder',
+      JSON.stringify(mockIndexOperationsBuilder.mock)
+    );
+
+    expect(mockIndexOperationsBuilder).toHaveBeenCalledTimes(2);
+    expect(mockIndexOperationsBuilder).toHaveBeenNthCalledWith(
+      1,
+      pineconeConfig1
+    );
+    expect(mockIndexOperationsBuilder).toHaveBeenNthCalledWith(
+      2,
+      pineconeConfig2
+    );
+  });
 });
diff --git a/src/data/indexHostSingleton.ts b/src/data/indexHostSingleton.ts
@@ -1,4 +1,3 @@
-import { ManageIndexesApi } from '../pinecone-generated-ts-fetch';
 import type { PineconeConfiguration } from './types';
 import type { IndexName } from '../control';
 import { describeIndex, indexOperationsBuilder } from '../control';
@@ -10,16 +9,12 @@ import { normalizeUrl } from '../utils';
 // and index, so we cache them in a singleton for reuse.
 export const IndexHostSingleton = (function () {
   const hostUrls = {}; // map of apiKey-indexName to hostUrl
-  let indexOperationsApi: InstanceType<typeof ManageIndexesApi> | null = null;
 
   const _describeIndex = async (
     config: PineconeConfiguration,
     indexName: IndexName
   ): Promise<string> => {
-    if (!indexOperationsApi) {
-      indexOperationsApi = indexOperationsBuilder(config);
-    }
-
+    const indexOperationsApi = indexOperationsBuilder(config);
     const describeResponse = await describeIndex(indexOperationsApi)(indexName);
     const host = describeResponse.host;
 
@@ -34,19 +29,17 @@ export const IndexHostSingleton = (function () {
     }
   };
 
-  const key = (config, indexName) => `${config.apiKey}-${indexName}`;
+  const _key = (config: PineconeConfiguration, indexName: string) =>
+    `${config.apiKey}-${indexName}`;
 
-  return {
-    getHostUrl: async function (
-      config: PineconeConfiguration,
-      indexName: IndexName
-    ) {
-      const cacheKey = key(config, indexName);
+  const singleton = {
+    getHostUrl: async (config: PineconeConfiguration, indexName: IndexName) => {
+      const cacheKey = _key(config, indexName);
       if (cacheKey in hostUrls) {
         return hostUrls[cacheKey];
       } else {
         const hostUrl = await _describeIndex(config, indexName);
-        this._set(config, indexName, hostUrl);
+        singleton._set(config, indexName, hostUrl);
 
         if (!hostUrls[cacheKey]) {
           throw new PineconeUnableToResolveHostError(
@@ -74,13 +67,15 @@ export const IndexHostSingleton = (function () {
         return;
       }
 
-      const cacheKey = key(config, indexName);
+      const cacheKey = _key(config, indexName);
       hostUrls[cacheKey] = normalizedHostUrl;
     },
 
     _delete: (config: PineconeConfiguration, indexName: IndexName) => {
-      const cacheKey = key(config, indexName);
+      const cacheKey = _key(config, indexName);
       delete hostUrls[cacheKey];
     },
   };
+
+  return singleton;
 })();
diff --git a/src/integration/data/delete.test.ts b/src/integration/data/delete.test.ts
@@ -69,7 +69,7 @@ describe('delete', () => {
       }
     };
 
-    assertWithRetries(() => ns.fetch(['0']), fetchAssertions);
+    await assertWithRetries(() => ns.fetch(['0']), fetchAssertions);
 
     // Try deleting the record
     await ns.deleteOne('0');
@@ -79,7 +79,7 @@ describe('delete', () => {
       expect(stats.namespaces[namespace]).toBeUndefined();
     };
 
-    assertWithRetries(() => ns.describeIndexStats(), deleteAssertions);
+    await assertWithRetries(() => ns.describeIndexStats(), deleteAssertions);
   });
 
   test('verify deleteMany with ids', async () => {
@@ -113,7 +113,7 @@ describe('delete', () => {
       }
     };
 
-    assertWithRetries(() => ns.fetch(['0']), fetchAssertions);
+    await assertWithRetries(() => ns.fetch(['0']), fetchAssertions);
 
     // Try deleting 2 of 3 records
     await ns.deleteMany(['0', '2']);
@@ -129,7 +129,7 @@ describe('delete', () => {
       }
     };
 
-    assertWithRetries(() => ns.describeIndexStats(), deleteAssertions);
+    await assertWithRetries(() => ns.describeIndexStats(), deleteAssertions);
 
     // Check that record id='1' still exists
     const fetchAssertions2 = (results) => {
@@ -143,7 +143,7 @@ describe('delete', () => {
       }
     };
 
-    assertWithRetries(() => ns.fetch(['1']), fetchAssertions2);
+    await assertWithRetries(() => ns.fetch(['1']), fetchAssertions2);
 
     // deleting non-existent records should not throw
     await ns.deleteMany(['0', '1', '2', '3']);
@@ -159,6 +159,6 @@ describe('delete', () => {
       }
     };
 
-    assertWithRetries(() => ns.describeIndexStats(), deleteAssertions2);
+    await assertWithRetries(() => ns.describeIndexStats(), deleteAssertions2);
   });
 });
diff --git a/src/integration/errorHandling.test.ts b/src/integration/errorHandling.test.ts
@@ -70,9 +70,9 @@ describe('Error handling', () => {
           await p.index('foo-index').query({ topK: 10, id: '1' });
         } catch (e) {
           const err = e as PineconeConnectionError;
-          expect(err.name).toEqual('PineconeAuthorizationError');
+          expect(err.name).toEqual('PineconeConnectionError');
           expect(err.message).toEqual(
-            'The API key you provided was rejected while calling https://api.pinecone.io/indexes/foo-index. Please check your configuration values and try again. You can find the configuration values for your project in the Pinecone developer console at https://app.pinecone.io'
+            'Request failed to reach Pinecone. This can occur for reasons such as network problems that prevent the request from being completed, or a Pinecone API outage. Check your network connection, and visit https://status.pinecone.io/ to see whether any outages are ongoing.'
           );
         }
       });