Fix `IndexHostSingleton` holding onto one instance of `ManageIndexesAPI` #224

austin-denoble · 2024-05-31T22:16:32Z

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:

We were missing a bunch of awaits in front of assertWithRetries calls, so I cleaned that up as well.

Type of Change

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)

$ 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

$ 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

… for index hosts

austin-denoble · 2024-05-31T22:18:53Z

src/data/indexHostSingleton.ts

-      indexOperationsApi = indexOperationsBuilder(config);
-    }
-
+    const indexOperationsApi = indexOperationsBuilder(config);


I really don't know what I was doing here, seems really silly to me now lol.

austin-denoble · 2024-05-31T22:19:12Z

src/data/indexHostSingleton.ts

@@ -34,19 +29,17 @@ export const IndexHostSingleton = (function () {
    }
  };

-  const key = (config, indexName) => `${config.apiKey}-${indexName}`;
+  const _key = (config: PineconeConfiguration, indexName: string) =>


Minor tweak, underscore just to make it more clear it's internal to the singleton.

austin-denoble · 2024-05-31T22:19:54Z

src/data/indexHostSingleton.ts

+  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);


This is primarily a stylistic change, I wanted the context of this to be more clear. I think this reads a bit better overall but if anyone has any other feedback let me know.

How does this call to singleton._set work, since singleton doesn't exist yet?

Ok, I see now after talking this over with you. By the time the function that refers to singleton runs, singleton has already been initialized. Fancy!

…e errorHandling test which now fails with the appropriate error response

austin-denoble · 2024-05-31T22:51:08Z

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');


We get the correct error here now, which is great.

austin-denoble added 2 commits May 31, 2024 14:07

make sure to recreate the ManageIndexesApi inside the singleton cache…

210f35c

… for index hosts

add unit test, fix scoping of this in singleton

a33bcd4

austin-denoble requested review from jhamon and aulorbe May 31, 2024 22:16

austin-denoble commented May 31, 2024

View reviewed changes

await assertWithRetries properly to prevent jest runner errors, updat…

2dd7d08

…e errorHandling test which now fails with the appropriate error response

austin-denoble commented May 31, 2024

View reviewed changes

austin-denoble mentioned this pull request May 31, 2024

[Bug] PineconeNotFoundError when there are 2 connections to pinecone #220

Closed

2 tasks

ssmith-pc approved these changes May 31, 2024

View reviewed changes

austin-denoble merged commit 4dee2e6 into main May 31, 2024
25 checks passed

austin-denoble deleted the adenoble/fix-index-host-singleton-indexes-api branch May 31, 2024 23:54

WesleyR10 mentioned this pull request Aug 21, 2024

[Snyk] Upgrade @pinecone-database/pinecone from 2.2.1 to 2.2.2 WesleyR10/AI-COMPANION-WR#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `IndexHostSingleton` holding onto one instance of `ManageIndexesAPI` #224

Fix `IndexHostSingleton` holding onto one instance of `ManageIndexesAPI` #224

austin-denoble commented May 31, 2024 •

edited

Loading

austin-denoble May 31, 2024

austin-denoble May 31, 2024 •

edited

Loading

austin-denoble May 31, 2024

ssmith-pc May 31, 2024

ssmith-pc May 31, 2024 •

edited

Loading

austin-denoble May 31, 2024

Fix IndexHostSingleton holding onto one instance of ManageIndexesAPI #224

Fix IndexHostSingleton holding onto one instance of ManageIndexesAPI #224

Conversation

austin-denoble commented May 31, 2024 • edited Loading

Problem

Solution

Type of Change

Test Plan

austin-denoble May 31, 2024

Choose a reason for hiding this comment

austin-denoble May 31, 2024 • edited Loading

Choose a reason for hiding this comment

austin-denoble May 31, 2024

Choose a reason for hiding this comment

ssmith-pc May 31, 2024

Choose a reason for hiding this comment

ssmith-pc May 31, 2024 • edited Loading

Choose a reason for hiding this comment

austin-denoble May 31, 2024

Choose a reason for hiding this comment

Fix `IndexHostSingleton` holding onto one instance of `ManageIndexesAPI` #224

Fix `IndexHostSingleton` holding onto one instance of `ManageIndexesAPI` #224

austin-denoble commented May 31, 2024 •

edited

Loading

austin-denoble May 31, 2024 •

edited

Loading

ssmith-pc May 31, 2024 •

edited

Loading