From 4dee2e6f9e169af3d62f93624d92a2116059f0ac Mon Sep 17 00:00:00 2001 From: Austin DeNoble Date: Fri, 31 May 2024 19:54:10 -0400 Subject: [PATCH] Fix `IndexHostSingleton` holding onto one instance of `ManageIndexesAPI` (#224) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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: https://github.com/pinecone-io/pinecone-ts-client/issues/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-/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 --- src/data/__tests__/indexHostSingleton.test.ts | 50 ++++++++++++++++++- src/data/indexHostSingleton.ts | 27 ++++------ src/integration/data/delete.test.ts | 12 ++--- src/integration/errorHandling.test.ts | 4 +- 4 files changed, 68 insertions(+), 25 deletions(-) diff --git a/src/data/__tests__/indexHostSingleton.test.ts b/src/data/__tests__/indexHostSingleton.test.ts index 6ecdee97..d7381581 100644 --- a/src/data/__tests__/indexHostSingleton.test.ts +++ b/src/data/__tests__/indexHostSingleton.test.ts @@ -1,13 +1,14 @@ 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), }; }); @@ -15,6 +16,7 @@ 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 index fcd9e769..5bcddc86 100644 --- 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 | null = null; const _describeIndex = async ( config: PineconeConfiguration, indexName: IndexName ): Promise => { - 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 index 30590029..df5cf3ff 100644 --- 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 index e661344f..72f165c8 100644 --- 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.' ); } });