From 1f1bfb85f95c57a6f800c03cca87dfc00687fb77 Mon Sep 17 00:00:00 2001 From: aulorbe Date: Wed, 18 Dec 2024 14:35:45 -0800 Subject: [PATCH 01/11] Refactor retries, add them to more ops, refactor retry tests --- src/control/configureIndex.ts | 11 +- src/data/index.ts | 2 +- src/data/vectors/update.ts | 10 +- src/errors/http.ts | 2 + .../control/configureIndex.test.ts | 277 +++++++++--------- .../data/vectors/upsertAndUpdate.test.ts | 188 +++++++----- src/pinecone.ts | 2 +- src/utils/retries.ts | 56 +++- 8 files changed, 318 insertions(+), 230 deletions(-) diff --git a/src/control/configureIndex.ts b/src/control/configureIndex.ts index eaf94279..4aa64719 100644 --- a/src/control/configureIndex.ts +++ b/src/control/configureIndex.ts @@ -6,6 +6,7 @@ import { import { PineconeArgumentError } from '../errors'; import type { IndexName } from './types'; import { ValidateProperties } from '../utils/validateProperties'; +import { RetryOnServerFailure } from '../utils'; // Properties for validation to ensure no unknown/invalid properties are passed, no req'd properties are missing type ConfigureIndexRequestType = keyof ConfigureIndexRequest; @@ -49,11 +50,17 @@ export const configureIndex = (api: ManageIndexesApi) => { return async ( indexName: IndexName, - options: ConfigureIndexRequest + options: ConfigureIndexRequest, + maxRetries?: number ): Promise => { validator(indexName, options); - return await api.configureIndex({ + const retryWrapper = new RetryOnServerFailure( + api.configureIndex.bind(api), + maxRetries + ); + + return await retryWrapper.execute({ indexName, configureIndexRequest: options, }); diff --git a/src/data/index.ts b/src/data/index.ts index 5f09465f..80c0c32e 100644 --- a/src/data/index.ts +++ b/src/data/index.ts @@ -539,7 +539,7 @@ export class Index { * @returns A promise that resolves when the update is completed. */ async update(options: UpdateOptions) { - return await this._updateCommand.run(options); + return await this._updateCommand.run(options, this.config.maxRetries); } /** diff --git a/src/data/vectors/update.ts b/src/data/vectors/update.ts index 7a586c68..aa9f1186 100644 --- a/src/data/vectors/update.ts +++ b/src/data/vectors/update.ts @@ -7,6 +7,7 @@ import type { } from './types'; import { PineconeArgumentError } from '../../errors'; import { ValidateProperties } from '../../utils/validateProperties'; +import { RetryOnServerFailure } from '../../utils'; /** * This type is very similar to { @link PineconeRecord }, but differs because the @@ -62,7 +63,7 @@ export class UpdateCommand { } }; - async run(options: UpdateOptions): Promise { + async run(options: UpdateOptions, maxRetries?: number): Promise { this.validator(options); const requestOptions = { @@ -73,7 +74,12 @@ export class UpdateCommand { }; const api = await this.apiProvider.provide(); - await api.updateVector({ + const retryWrapper = new RetryOnServerFailure( + api.updateVector.bind(api), + maxRetries + ); + + await retryWrapper.execute({ updateRequest: { ...requestOptions, namespace: this.namespace }, }); return; diff --git a/src/errors/http.ts b/src/errors/http.ts index f229b3c0..0573b758 100644 --- a/src/errors/http.ts +++ b/src/errors/http.ts @@ -198,6 +198,8 @@ export const mapHttpStatusError = (failedRequestInfo: FailedRequestInfo) => { return new PineconeInternalServerError(failedRequestInfo); case 501: return new PineconeNotImplementedError(failedRequestInfo); + case 503: + return new PineconeUnavailableError(failedRequestInfo); default: throw new PineconeUnmappedHttpError(failedRequestInfo); } diff --git a/src/integration/control/configureIndex.test.ts b/src/integration/control/configureIndex.test.ts index 1f203b9f..fe8fc4c5 100644 --- a/src/integration/control/configureIndex.test.ts +++ b/src/integration/control/configureIndex.test.ts @@ -10,6 +10,7 @@ import { sleep, waitUntilReady, } from '../test-helpers'; +import { RetryOnServerFailure } from '../../utils'; let podIndexName: string, serverlessIndexName: string, pinecone: Pinecone; @@ -58,126 +59,110 @@ describe('configure index', () => { await retryDeletes(pinecone, serverlessIndexName); }); - describe('pod index', () => { - test('scale replicas up', async () => { - const description = await pinecone.describeIndex(podIndexName); - expect(description.spec.pod?.replicas).toEqual(1); - - await pinecone.configureIndex(podIndexName, { - spec: { pod: { replicas: 2 } }, - }); - const description2 = await pinecone.describeIndex(podIndexName); - expect(description2.spec.pod?.replicas).toEqual(2); - }); - - test('scale podType up', async () => { - // Verify starting state of podType is same as originally created - const description = await pinecone.describeIndex(podIndexName); - expect(description.spec.pod?.podType).toEqual('p1.x1'); - - // Scale up podType to x2 - let state = true; - let retryCount = 0; - const maxRetries = 10; - while (state && retryCount < maxRetries) { - try { - await pinecone.configureIndex(podIndexName, { - spec: { pod: { podType: 'p1.x2' } }, - }); - state = false; - } catch (e) { - if (e instanceof PineconeInternalServerError) { - retryCount++; - await sleep(2000); - } else { - console.log('Unexpected error:', e); - throw e; - } - } - } - await waitUntilReady(podIndexName); - const description2 = await pinecone.describeIndex(podIndexName); - expect(description2.spec.pod?.podType).toEqual('p1.x2'); - }); - - test('Remove index tag from pod index', async () => { - const description = await pinecone.describeIndex(podIndexName); - expect(description.tags).toEqual({ - project: 'pinecone-integration-tests', - }); - - await pinecone.configureIndex(podIndexName, { - tags: { project: '' }, - }); - const description2 = await pinecone.describeIndex(podIndexName); - expect(description2.tags).toBeUndefined(); - }); - }); - - describe('serverless index', () => { - test('enable and disable deletionProtection', async () => { - await pinecone.configureIndex(serverlessIndexName, { - deletionProtection: 'enabled', - }); - await waitUntilReady(serverlessIndexName); - - // verify we cannot delete the index - await pinecone.deleteIndex(serverlessIndexName).catch((e) => { - const err = e as PineconeBadRequestError; - expect(err.name).toEqual('PineconeBadRequestError'); - expect(err.message).toContain( - 'Deletion protection is enabled for this index' - ); - }); - - // disable so we can clean the index up - await pinecone.configureIndex(serverlessIndexName, { - deletionProtection: 'disabled', - }); - }); - - test('Remove index tag from serverless index', async () => { - const description = await pinecone.describeIndex(serverlessIndexName); - expect(description.tags).toEqual({ - project: 'pinecone-integration-tests', - }); - - await pinecone.configureIndex(serverlessIndexName, { - tags: { project: '' }, - }); - const description2 = await pinecone.describeIndex(serverlessIndexName); - expect(description2.tags).toBeUndefined(); - }); - }); + // describe('pod index', () => { + // test('scale replicas up', async () => { + // const description = await pinecone.describeIndex(podIndexName); + // expect(description.spec.pod?.replicas).toEqual(1); + // + // await pinecone.configureIndex(podIndexName, { + // spec: { pod: { replicas: 2 } }, + // }); + // const description2 = await pinecone.describeIndex(podIndexName); + // expect(description2.spec.pod?.replicas).toEqual(2); + // }); + // + // test('scale podType up', async () => { + // // Verify starting state of podType is same as originally created + // const description = await pinecone.describeIndex(podIndexName); + // expect(description.spec.pod?.podType).toEqual('p1.x1'); + // + // await pinecone.configureIndex(podIndexName, { + // spec: { pod: { podType: 'p1.x2' } }, + // }); + // + // await waitUntilReady(podIndexName); + // const description2 = await pinecone.describeIndex(podIndexName); + // expect(description2.spec.pod?.podType).toEqual('p1.x2'); + // }); + // + // test('Remove index tag from pod index', async () => { + // const description = await pinecone.describeIndex(podIndexName); + // expect(description.tags).toEqual({ + // project: 'pinecone-integration-tests', + // }); + // + // await pinecone.configureIndex(podIndexName, { + // tags: { project: '' }, + // }); + // const description2 = await pinecone.describeIndex(podIndexName); + // expect(description2.tags).toBeUndefined(); + // }); + // }); + + // describe('serverless index', () => { + // test('enable and disable deletionProtection', async () => { + // await pinecone.configureIndex(serverlessIndexName, { + // deletionProtection: 'enabled', + // }); + // await waitUntilReady(serverlessIndexName); + // + // // verify we cannot delete the index + // await pinecone.deleteIndex(serverlessIndexName).catch((e) => { + // const err = e as PineconeBadRequestError; + // expect(err.name).toEqual('PineconeBadRequestError'); + // expect(err.message).toContain( + // 'Deletion protection is enabled for this index' + // ); + // }); + // + // // disable so we can clean the index up + // await pinecone.configureIndex(serverlessIndexName, { + // deletionProtection: 'disabled', + // }); + // }); + // + // test('Remove index tag from serverless index', async () => { + // const description = await pinecone.describeIndex(serverlessIndexName); + // expect(description.tags).toEqual({ + // project: 'pinecone-integration-tests', + // }); + // + // await pinecone.configureIndex(serverlessIndexName, { + // tags: { project: '' }, + // }); + // const description2 = await pinecone.describeIndex(serverlessIndexName); + // expect(description2.tags).toBeUndefined(); + // }); + // }); describe('error cases', () => { - test('cannot configure index with invalid index name', async () => { - try { - await pinecone.configureIndex('non-existent-index', { - spec: { pod: { replicas: 2 } }, - }); - } catch (e) { - const err = e as BasePineconeError; - expect(err.name).toEqual('PineconeNotFoundError'); - } - }); - - test('cannot configure index when exceeding quota', async () => { - try { - await pinecone.configureIndex(podIndexName, { - spec: { pod: { replicas: 20 } }, - }); - } catch (e) { - const err = e as BasePineconeError; - expect(err.name).toEqual('PineconeBadRequestError'); - expect(err.message).toContain( - `You've reached the max pods allowed in project` - ); - expect(err.message).toContain( - 'To increase this limit, adjust your project settings in the console' - ); - } - }); + // test('cannot configure index with invalid index name', async () => { + // try { + // await pinecone.configureIndex('non-existent-index', { + // spec: { pod: { replicas: 2 } }, + // }); + // } catch (e) { + // const err = e as BasePineconeError; + // expect(err.name).toEqual("PineconeMaxRetriesExceededError"); // TODO: This should be more helpful error + // } + // }); + + // test('cannot configure index when exceeding quota', async () => { + // try { + // await pinecone.configureIndex(podIndexName, { + // spec: { pod: { replicas: 20 } }, + // }); + // } catch (e) { + // const err = e as BasePineconeError; + // expect(err.name).toEqual('PineconeBadRequestError'); + // expect(err.message).toContain( + // `You've reached the max pods allowed in project` + // ); + // expect(err.message).toContain( + // 'To increase this limit, adjust your project settings in the console' + // ); + // } + // }); test('cannot change base pod type', async () => { try { @@ -192,33 +177,33 @@ describe('configure index', () => { } }); - test('cannot set deletionProtection value other than enabled / disabled', async () => { - try { - await pinecone.configureIndex(serverlessIndexName, { - // @ts-expect-error - deletionProtection: 'bogus', - }); - } catch (e) { - const err = e as BasePineconeError; - expect(err.name).toEqual('PineconeBadRequestError'); - expect(err.message).toContain( - 'Invalid deletion_protection, value should be either enabled or disabled' - ); - } - }); - - test('cannot configure pod spec for serverless', async () => { - try { - await pinecone.configureIndex(serverlessIndexName, { - spec: { pod: { replicas: 2 } }, - }); - } catch (e) { - const err = e as BasePineconeError; - expect(err.name).toEqual('PineconeBadRequestError'); - expect(err.message).toContain( - 'Configuring replicas and pod type is not supported for serverless' - ); - } - }); + // test('cannot set deletionProtection value other than enabled / disabled', async () => { + // try { + // await pinecone.configureIndex(serverlessIndexName, { + // // @ts-expect-error + // deletionProtection: 'bogus', + // }); + // } catch (e) { + // const err = e as BasePineconeError; + // expect(err.name).toEqual('PineconeBadRequestError'); + // expect(err.message).toContain( + // 'Invalid deletion_protection, value should be either enabled or disabled' + // ); + // } + // }); + + // test('cannot configure pod spec for serverless', async () => { + // try { + // await pinecone.configureIndex(serverlessIndexName, { + // spec: { pod: { replicas: 2 } }, + // }); + // } catch (e) { + // const err = e as BasePineconeError; + // expect(err.name).toEqual('PineconeBadRequestError'); + // expect(err.message).toContain( + // 'Configuring replicas and pod type is not supported for serverless' + // ); + // } + // }); }); }); diff --git a/src/integration/data/vectors/upsertAndUpdate.test.ts b/src/integration/data/vectors/upsertAndUpdate.test.ts index 7ed52683..48c81d6d 100644 --- a/src/integration/data/vectors/upsertAndUpdate.test.ts +++ b/src/integration/data/vectors/upsertAndUpdate.test.ts @@ -9,7 +9,7 @@ import { waitUntilReady, } from '../../test-helpers'; import { RetryOnServerFailure } from '../../../utils'; -import { PineconeMaxRetriesExceededError } from '../../../errors'; +import { PineconeMaxRetriesExceededError, PineconeUnavailableError } from '../../../errors'; import http from 'http'; import { parse } from 'url'; @@ -46,46 +46,46 @@ afterAll(async () => { }); // todo: add sparse values update -describe('upsert and update to serverless index', () => { - test('verify upsert and update', async () => { - const recordToUpsert = generateRecords({ - dimension: 2, - quantity: 1, - withSparseValues: false, - withMetadata: true, - }); - - // Upsert record - await serverlessIndex.upsert(recordToUpsert); - - // Build new values - const newValues = [0.5, 0.4]; - const newMetadata = { flavor: 'chocolate' }; - - const updateSpy = jest - .spyOn(serverlessIndex, 'update') - .mockResolvedValue(undefined); - - // Update values w/new values - await serverlessIndex.update({ - id: '0', - values: newValues, - metadata: newMetadata, - }); - - expect(updateSpy).toHaveBeenCalledWith({ - id: '0', - values: newValues, - metadata: newMetadata, - }); - - // Clean up spy after the test - updateSpy.mockRestore(); - }); -}); +// describe('upsert and update to serverless index', () => { +// test('verify upsert and update', async () => { +// const recordToUpsert = generateRecords({ +// dimension: 2, +// quantity: 1, +// withSparseValues: false, +// withMetadata: true, +// }); +// +// // Upsert record +// await serverlessIndex.upsert(recordToUpsert); +// +// // Build new values +// const newValues = [0.5, 0.4]; +// const newMetadata = { flavor: 'chocolate' }; +// +// const updateSpy = jest +// .spyOn(serverlessIndex, 'update') +// .mockResolvedValue(undefined); +// +// // Update values w/new values +// await serverlessIndex.update({ +// id: '0', +// values: newValues, +// metadata: newMetadata, +// }); +// +// expect(updateSpy).toHaveBeenCalledWith({ +// id: '0', +// values: newValues, +// metadata: newMetadata, +// }); +// +// // Clean up spy after the test +// updateSpy.mockRestore(); +// }); +// }); // Retry logic tests -describe('Testing retry logic on Upsert operation, as run on a mock, in-memory http server', () => { +describe('Testing retry logic via a mock, in-memory http server', () => { const recordsToUpsert = generateRecords({ dimension: 2, quantity: 1, @@ -96,13 +96,14 @@ describe('Testing retry logic on Upsert operation, as run on a mock, in-memory h let server: http.Server; // Note: server cannot be something like an express server due to conflicts w/edge runtime let mockServerlessIndex: Index; let callCount: number; + let op: string; // Helper function to start the server with a specific response pattern const startMockServer = (shouldSucceedOnSecondCall: boolean) => { // Create http server server = http.createServer((req, res) => { const { pathname } = parse(req.url || '', true); - if (req.method === 'POST' && pathname === '/vectors/upsert') { + if (req.method === 'POST' && pathname === `/vectors/${op}`) { callCount++; if (shouldSucceedOnSecondCall && callCount === 1) { res.writeHead(503, { 'Content-Type': 'application/json' }); @@ -137,6 +138,7 @@ describe('Testing retry logic on Upsert operation, as run on a mock, in-memory h }); test('Upsert operation should retry 1x if server responds 1x with error and 1x with success', async () => { + op = 'upsert'; pinecone = new Pinecone({ apiKey: process.env['PINECONE_API_KEY'] || '', maxRetries: 2, @@ -153,42 +155,82 @@ describe('Testing retry logic on Upsert operation, as run on a mock, in-memory h startMockServer(true); // Call Upsert operation - await mockServerlessIndex.upsert(recordsToUpsert); + // const toThrow = async() => {await mockServerlessIndex.upsert(recordsToUpsert)}; + await mockServerlessIndex.upsert(recordsToUpsert) + // await expect(toThrow).rejects.toThrowError(PineconeUnavailableError); + + // const toThrow = async () => { + // await deleteOneFn(''); + // }; + // await expect(toThrow).rejects.toThrowError(PineconeArgumentError); + // await expect(toThrow).rejects.toThrowError( + // 'You must pass a non-empty string for `options` in order to delete a record.' + // ); + // }); // 2 total tries: 1 initial call, 1 retry - expect(retrySpy).toHaveBeenCalledTimes(1); - expect(delaySpy).toHaveBeenCalledTimes(1); + expect(retrySpy).toHaveBeenCalledTimes(1); // passes + expect(delaySpy).toHaveBeenCalledTimes(1); // fails expect(callCount).toBe(2); }); - test('Max retries exceeded w/o resolve', async () => { - pinecone = new Pinecone({ - apiKey: process.env['PINECONE_API_KEY'] || '', - maxRetries: 3, - }); - - mockServerlessIndex = pinecone - .Index(serverlessIndexName, 'http://localhost:4000') - .namespace(globalNamespaceOne); - - const retrySpy = jest.spyOn(RetryOnServerFailure.prototype, 'execute'); - const delaySpy = jest.spyOn(RetryOnServerFailure.prototype, 'delay'); - - // Start server with persistent 503 errors on every call - startMockServer(false); - - // Catch expected error from Upsert operation - const errorResult = async () => { - await mockServerlessIndex.upsert(recordsToUpsert); - }; - - await expect(errorResult).rejects.toThrowError( - PineconeMaxRetriesExceededError - ); - - // Out of 3 total tries, 2 are retries (i.e. delays), and 1 is the initial call: - expect(retrySpy).toHaveBeenCalledTimes(1); - expect(delaySpy).toHaveBeenCalledTimes(2); - expect(callCount).toBe(3); - }); + // test('Update operation should retry 1x if server responds 1x with error and 1x with success', async () => { + // op = 'update'; + // + // pinecone = new Pinecone({ + // apiKey: process.env['PINECONE_API_KEY'] || '', + // maxRetries: 2, + // }); + // + // mockServerlessIndex = pinecone.Index(serverlessIndexName, 'http://localhost:4000') + // + // const retrySpy = jest.spyOn(RetryOnServerFailure.prototype, 'execute'); + // const delaySpy = jest.spyOn(RetryOnServerFailure.prototype, 'delay'); + // + // // Start server with a successful response on the second call + // startMockServer(true); + // + // const recordIdToUpdate = recordsToUpsert[0].id; + // const newMetadata = { flavor: 'chocolate' }; + // + // // Call Update operation + // await mockServerlessIndex.update({id: recordIdToUpdate, metadata: newMetadata}); + // + // // 2 total tries: 1 initial call, 1 retry + // expect(retrySpy).toHaveBeenCalledTimes(1); + // expect(delaySpy).toHaveBeenCalledTimes(1); + // expect(callCount).toBe(2); + // }); + + // test('Max retries exceeded w/o resolve', async () => { + // op = 'upsert'; + // pinecone = new Pinecone({ + // apiKey: process.env['PINECONE_API_KEY'] || '', + // maxRetries: 3, + // }); + // + // mockServerlessIndex = pinecone + // .Index(serverlessIndexName, 'http://localhost:4000') + // .namespace(globalNamespaceOne); + // + // const retrySpy = jest.spyOn(RetryOnServerFailure.prototype, 'execute'); + // const delaySpy = jest.spyOn(RetryOnServerFailure.prototype, 'delay'); + // + // // Start server with persistent 503 errors on every call + // startMockServer(false); + // + // // Catch expected error from Upsert operation + // const errorResult = async () => { + // await mockServerlessIndex.upsert(recordsToUpsert); + // }; + // + // await expect(errorResult).rejects.toThrowError( + // PineconeMaxRetriesExceededError + // ); + // + // // Out of 3 total tries, 2 are retries (i.e. delays), and 1 is the initial call: + // expect(retrySpy).toHaveBeenCalledTimes(1); + // expect(delaySpy).toHaveBeenCalledTimes(2); + // expect(callCount).toBe(3); + // }); }); diff --git a/src/pinecone.ts b/src/pinecone.ts index ee270e63..5fde1965 100644 --- a/src/pinecone.ts +++ b/src/pinecone.ts @@ -472,7 +472,7 @@ export class Pinecone { * @returns A promise that resolves to {@link IndexModel} when the request to configure the index is completed. */ configureIndex(indexName: IndexName, options: ConfigureIndexRequest) { - return this._configureIndex(indexName, options); + return this._configureIndex(indexName, options, this.config.maxRetries); } /** diff --git a/src/utils/retries.ts b/src/utils/retries.ts index 1a4d3b4b..44a2b2cb 100644 --- a/src/utils/retries.ts +++ b/src/utils/retries.ts @@ -1,4 +1,4 @@ -import { PineconeMaxRetriesExceededError } from '../errors'; +import { BasePineconeError, mapHttpStatusError, PineconeMaxRetriesExceededError } from '../errors'; /* Retry asynchronous operations. * @@ -24,24 +24,64 @@ export class RetryOnServerFailure { } async execute(...args: A): Promise { + console.log("Retrying!") + console.log("maxRetries =" ,this.maxRetries) + if (this.maxRetries < 1) { + return this.asyncFn(...args); + } + for (let attempt = 0; attempt < this.maxRetries; attempt++) { + console.log("Attempt = ", attempt) try { const response = await this.asyncFn(...args); + + // Return immediately if the response is not a retryable error if (!this.isRetryError(response)) { - return response; // Return if there's no retryable error + console.log("Response is not retryable") + return response; } - throw response; // Throw if response is retryable, and catch below + + // Retryable response, throw it to trigger retry logic + console.log("Response is retryable, retrying...") + throw response; + } catch (error) { + console.log("Caught retryable error") + + const mappedError = this.mapErrorIfNeeded(error); + + + // If the error is not retryable, throw it immediately + if (this.shouldStopRetrying(mappedError)) { + console.log("Should stop retrying retryable error") + throw mappedError; + } + + // On the last retry, throw a MaxRetriesExceededError if (attempt === this.maxRetries - 1) { + console.log("Max retries exceeded") throw new PineconeMaxRetriesExceededError(this.maxRetries); } - await this.delay(attempt + 1); // Increment before passing to `delay` + + // Wait before retrying + console.log("Waiting before retrying") + await this.delay(attempt + 1); } } - // Fallback throw in case no value is successfully returned in order to comply w/return type Promise + + console.log("Fell through to max retries exceeded") + // This fallback is unnecessary, but included for type safety throw new PineconeMaxRetriesExceededError(this.maxRetries); } + private mapErrorIfNeeded(error: any): any { + if (error?.status) { + return mapHttpStatusError(error); + } + return error; // Return original error if no mapping is needed + } + + isRetryError(response): boolean { if (response) { if ( @@ -59,7 +99,13 @@ export class RetryOnServerFailure { return false; } + // todo: write unit test for this + private shouldStopRetrying(error: any): boolean { + return (error.status && error.status < 500) || (error.name !== 'PineconeUnavailableError' && error.name !== 'PineconeInternalServerError'); + } + async delay(attempt: number): Promise { + console.log("Delaying!") const delayTime = this.calculateRetryDelay(attempt); return new Promise((resolve) => setTimeout(resolve, delayTime)); } From 8df8e7d1c3b3abd2bef718725fdc61bc46eabfaa Mon Sep 17 00:00:00 2001 From: aulorbe Date: Wed, 18 Dec 2024 17:07:46 -0800 Subject: [PATCH 02/11] Clean up --- .../control/configureIndex.test.ts | 274 +++++++++--------- .../data/vectors/upsertAndUpdate.test.ts | 138 +++++---- src/utils/__tests__/retries.test.ts | 38 +++ src/utils/retries.ts | 53 ++-- 4 files changed, 259 insertions(+), 244 deletions(-) diff --git a/src/integration/control/configureIndex.test.ts b/src/integration/control/configureIndex.test.ts index fe8fc4c5..846acb9d 100644 --- a/src/integration/control/configureIndex.test.ts +++ b/src/integration/control/configureIndex.test.ts @@ -1,16 +1,6 @@ -import { - BasePineconeError, - PineconeBadRequestError, - PineconeInternalServerError, -} from '../../errors'; +import { BasePineconeError, PineconeBadRequestError } from '../../errors'; import { Pinecone } from '../../index'; -import { - randomIndexName, - retryDeletes, - sleep, - waitUntilReady, -} from '../test-helpers'; -import { RetryOnServerFailure } from '../../utils'; +import { randomIndexName, retryDeletes, waitUntilReady } from '../test-helpers'; let podIndexName: string, serverlessIndexName: string, pinecone: Pinecone; @@ -59,110 +49,110 @@ describe('configure index', () => { await retryDeletes(pinecone, serverlessIndexName); }); - // describe('pod index', () => { - // test('scale replicas up', async () => { - // const description = await pinecone.describeIndex(podIndexName); - // expect(description.spec.pod?.replicas).toEqual(1); - // - // await pinecone.configureIndex(podIndexName, { - // spec: { pod: { replicas: 2 } }, - // }); - // const description2 = await pinecone.describeIndex(podIndexName); - // expect(description2.spec.pod?.replicas).toEqual(2); - // }); - // - // test('scale podType up', async () => { - // // Verify starting state of podType is same as originally created - // const description = await pinecone.describeIndex(podIndexName); - // expect(description.spec.pod?.podType).toEqual('p1.x1'); - // - // await pinecone.configureIndex(podIndexName, { - // spec: { pod: { podType: 'p1.x2' } }, - // }); - // - // await waitUntilReady(podIndexName); - // const description2 = await pinecone.describeIndex(podIndexName); - // expect(description2.spec.pod?.podType).toEqual('p1.x2'); - // }); - // - // test('Remove index tag from pod index', async () => { - // const description = await pinecone.describeIndex(podIndexName); - // expect(description.tags).toEqual({ - // project: 'pinecone-integration-tests', - // }); - // - // await pinecone.configureIndex(podIndexName, { - // tags: { project: '' }, - // }); - // const description2 = await pinecone.describeIndex(podIndexName); - // expect(description2.tags).toBeUndefined(); - // }); - // }); - - // describe('serverless index', () => { - // test('enable and disable deletionProtection', async () => { - // await pinecone.configureIndex(serverlessIndexName, { - // deletionProtection: 'enabled', - // }); - // await waitUntilReady(serverlessIndexName); - // - // // verify we cannot delete the index - // await pinecone.deleteIndex(serverlessIndexName).catch((e) => { - // const err = e as PineconeBadRequestError; - // expect(err.name).toEqual('PineconeBadRequestError'); - // expect(err.message).toContain( - // 'Deletion protection is enabled for this index' - // ); - // }); - // - // // disable so we can clean the index up - // await pinecone.configureIndex(serverlessIndexName, { - // deletionProtection: 'disabled', - // }); - // }); - // - // test('Remove index tag from serverless index', async () => { - // const description = await pinecone.describeIndex(serverlessIndexName); - // expect(description.tags).toEqual({ - // project: 'pinecone-integration-tests', - // }); - // - // await pinecone.configureIndex(serverlessIndexName, { - // tags: { project: '' }, - // }); - // const description2 = await pinecone.describeIndex(serverlessIndexName); - // expect(description2.tags).toBeUndefined(); - // }); - // }); + describe('pod index', () => { + test('scale replicas up', async () => { + const description = await pinecone.describeIndex(podIndexName); + expect(description.spec.pod?.replicas).toEqual(1); + + await pinecone.configureIndex(podIndexName, { + spec: { pod: { replicas: 2 } }, + }); + const description2 = await pinecone.describeIndex(podIndexName); + expect(description2.spec.pod?.replicas).toEqual(2); + }); + + test('scale podType up', async () => { + // Verify starting state of podType is same as originally created + const description = await pinecone.describeIndex(podIndexName); + expect(description.spec.pod?.podType).toEqual('p1.x1'); + + await pinecone.configureIndex(podIndexName, { + spec: { pod: { podType: 'p1.x2' } }, + }); + + await waitUntilReady(podIndexName); + const description2 = await pinecone.describeIndex(podIndexName); + expect(description2.spec.pod?.podType).toEqual('p1.x2'); + }); + + test('Remove index tag from pod index', async () => { + const description = await pinecone.describeIndex(podIndexName); + expect(description.tags).toEqual({ + project: 'pinecone-integration-tests', + }); + + await pinecone.configureIndex(podIndexName, { + tags: { project: '' }, + }); + const description2 = await pinecone.describeIndex(podIndexName); + expect(description2.tags).toBeUndefined(); + }); + }); + + describe('serverless index', () => { + test('enable and disable deletionProtection', async () => { + await pinecone.configureIndex(serverlessIndexName, { + deletionProtection: 'enabled', + }); + await waitUntilReady(serverlessIndexName); + + // verify we cannot delete the index + await pinecone.deleteIndex(serverlessIndexName).catch((e) => { + const err = e as PineconeBadRequestError; + expect(err.name).toEqual('PineconeBadRequestError'); + expect(err.message).toContain( + 'Deletion protection is enabled for this index' + ); + }); + + // disable so we can clean the index up + await pinecone.configureIndex(serverlessIndexName, { + deletionProtection: 'disabled', + }); + }); + + test('Remove index tag from serverless index', async () => { + const description = await pinecone.describeIndex(serverlessIndexName); + expect(description.tags).toEqual({ + project: 'pinecone-integration-tests', + }); + + await pinecone.configureIndex(serverlessIndexName, { + tags: { project: '' }, + }); + const description2 = await pinecone.describeIndex(serverlessIndexName); + expect(description2.tags).toBeUndefined(); + }); + }); describe('error cases', () => { - // test('cannot configure index with invalid index name', async () => { - // try { - // await pinecone.configureIndex('non-existent-index', { - // spec: { pod: { replicas: 2 } }, - // }); - // } catch (e) { - // const err = e as BasePineconeError; - // expect(err.name).toEqual("PineconeMaxRetriesExceededError"); // TODO: This should be more helpful error - // } - // }); - - // test('cannot configure index when exceeding quota', async () => { - // try { - // await pinecone.configureIndex(podIndexName, { - // spec: { pod: { replicas: 20 } }, - // }); - // } catch (e) { - // const err = e as BasePineconeError; - // expect(err.name).toEqual('PineconeBadRequestError'); - // expect(err.message).toContain( - // `You've reached the max pods allowed in project` - // ); - // expect(err.message).toContain( - // 'To increase this limit, adjust your project settings in the console' - // ); - // } - // }); + test('cannot configure index with invalid index name', async () => { + try { + await pinecone.configureIndex('non-existent-index', { + spec: { pod: { replicas: 2 } }, + }); + } catch (e) { + const err = e as BasePineconeError; + expect(err.name).toEqual('PineconeNotFoundError'); + } + }); + + test('cannot configure index when exceeding quota', async () => { + try { + await pinecone.configureIndex(podIndexName, { + spec: { pod: { replicas: 20 } }, + }); + } catch (e) { + const err = e as BasePineconeError; + expect(err.name).toEqual('PineconeBadRequestError'); + expect(err.message).toContain( + `You've reached the max pods allowed in project` + ); + expect(err.message).toContain( + 'To increase this limit, adjust your project settings in the console' + ); + } + }); test('cannot change base pod type', async () => { try { @@ -177,33 +167,33 @@ describe('configure index', () => { } }); - // test('cannot set deletionProtection value other than enabled / disabled', async () => { - // try { - // await pinecone.configureIndex(serverlessIndexName, { - // // @ts-expect-error - // deletionProtection: 'bogus', - // }); - // } catch (e) { - // const err = e as BasePineconeError; - // expect(err.name).toEqual('PineconeBadRequestError'); - // expect(err.message).toContain( - // 'Invalid deletion_protection, value should be either enabled or disabled' - // ); - // } - // }); - - // test('cannot configure pod spec for serverless', async () => { - // try { - // await pinecone.configureIndex(serverlessIndexName, { - // spec: { pod: { replicas: 2 } }, - // }); - // } catch (e) { - // const err = e as BasePineconeError; - // expect(err.name).toEqual('PineconeBadRequestError'); - // expect(err.message).toContain( - // 'Configuring replicas and pod type is not supported for serverless' - // ); - // } - // }); + test('cannot set deletionProtection value other than enabled / disabled', async () => { + try { + await pinecone.configureIndex(serverlessIndexName, { + // @ts-expect-error + deletionProtection: 'bogus', + }); + } catch (e) { + const err = e as BasePineconeError; + expect(err.name).toEqual('PineconeBadRequestError'); + expect(err.message).toContain( + 'Invalid deletion_protection, value should be either enabled or disabled' + ); + } + }); + + test('cannot configure pod spec for serverless', async () => { + try { + await pinecone.configureIndex(serverlessIndexName, { + spec: { pod: { replicas: 2 } }, + }); + } catch (e) { + const err = e as BasePineconeError; + expect(err.name).toEqual('PineconeBadRequestError'); + expect(err.message).toContain( + 'Configuring replicas and pod type is not supported for serverless' + ); + } + }); }); }); diff --git a/src/integration/data/vectors/upsertAndUpdate.test.ts b/src/integration/data/vectors/upsertAndUpdate.test.ts index 48c81d6d..38799ced 100644 --- a/src/integration/data/vectors/upsertAndUpdate.test.ts +++ b/src/integration/data/vectors/upsertAndUpdate.test.ts @@ -9,7 +9,7 @@ import { waitUntilReady, } from '../../test-helpers'; import { RetryOnServerFailure } from '../../../utils'; -import { PineconeMaxRetriesExceededError, PineconeUnavailableError } from '../../../errors'; +import { PineconeMaxRetriesExceededError } from '../../../errors'; import http from 'http'; import { parse } from 'url'; @@ -155,18 +155,7 @@ describe('Testing retry logic via a mock, in-memory http server', () => { startMockServer(true); // Call Upsert operation - // const toThrow = async() => {await mockServerlessIndex.upsert(recordsToUpsert)}; - await mockServerlessIndex.upsert(recordsToUpsert) - // await expect(toThrow).rejects.toThrowError(PineconeUnavailableError); - - // const toThrow = async () => { - // await deleteOneFn(''); - // }; - // await expect(toThrow).rejects.toThrowError(PineconeArgumentError); - // await expect(toThrow).rejects.toThrowError( - // 'You must pass a non-empty string for `options` in order to delete a record.' - // ); - // }); + await mockServerlessIndex.upsert(recordsToUpsert); // 2 total tries: 1 initial call, 1 retry expect(retrySpy).toHaveBeenCalledTimes(1); // passes @@ -174,63 +163,68 @@ describe('Testing retry logic via a mock, in-memory http server', () => { expect(callCount).toBe(2); }); - // test('Update operation should retry 1x if server responds 1x with error and 1x with success', async () => { - // op = 'update'; - // - // pinecone = new Pinecone({ - // apiKey: process.env['PINECONE_API_KEY'] || '', - // maxRetries: 2, - // }); - // - // mockServerlessIndex = pinecone.Index(serverlessIndexName, 'http://localhost:4000') - // - // const retrySpy = jest.spyOn(RetryOnServerFailure.prototype, 'execute'); - // const delaySpy = jest.spyOn(RetryOnServerFailure.prototype, 'delay'); - // - // // Start server with a successful response on the second call - // startMockServer(true); - // - // const recordIdToUpdate = recordsToUpsert[0].id; - // const newMetadata = { flavor: 'chocolate' }; - // - // // Call Update operation - // await mockServerlessIndex.update({id: recordIdToUpdate, metadata: newMetadata}); - // - // // 2 total tries: 1 initial call, 1 retry - // expect(retrySpy).toHaveBeenCalledTimes(1); - // expect(delaySpy).toHaveBeenCalledTimes(1); - // expect(callCount).toBe(2); - // }); - - // test('Max retries exceeded w/o resolve', async () => { - // op = 'upsert'; - // pinecone = new Pinecone({ - // apiKey: process.env['PINECONE_API_KEY'] || '', - // maxRetries: 3, - // }); - // - // mockServerlessIndex = pinecone - // .Index(serverlessIndexName, 'http://localhost:4000') - // .namespace(globalNamespaceOne); - // - // const retrySpy = jest.spyOn(RetryOnServerFailure.prototype, 'execute'); - // const delaySpy = jest.spyOn(RetryOnServerFailure.prototype, 'delay'); - // - // // Start server with persistent 503 errors on every call - // startMockServer(false); - // - // // Catch expected error from Upsert operation - // const errorResult = async () => { - // await mockServerlessIndex.upsert(recordsToUpsert); - // }; - // - // await expect(errorResult).rejects.toThrowError( - // PineconeMaxRetriesExceededError - // ); - // - // // Out of 3 total tries, 2 are retries (i.e. delays), and 1 is the initial call: - // expect(retrySpy).toHaveBeenCalledTimes(1); - // expect(delaySpy).toHaveBeenCalledTimes(2); - // expect(callCount).toBe(3); - // }); + test('Update operation should retry 1x if server responds 1x with error and 1x with success', async () => { + op = 'update'; + + pinecone = new Pinecone({ + apiKey: process.env['PINECONE_API_KEY'] || '', + maxRetries: 2, + }); + + mockServerlessIndex = pinecone + .Index(serverlessIndexName, 'http://localhost:4000') + .namespace(globalNamespaceOne); + + const retrySpy = jest.spyOn(RetryOnServerFailure.prototype, 'execute'); + const delaySpy = jest.spyOn(RetryOnServerFailure.prototype, 'delay'); + + // Start server with a successful response on the second call + startMockServer(true); + + const recordIdToUpdate = recordsToUpsert[0].id; + const newMetadata = { flavor: 'chocolate' }; + + // Call Update operation + await mockServerlessIndex.update({ + id: recordIdToUpdate, + metadata: newMetadata, + }); + + // 2 total tries: 1 initial call, 1 retry + expect(retrySpy).toHaveBeenCalledTimes(1); + expect(delaySpy).toHaveBeenCalledTimes(1); + expect(callCount).toBe(2); + }); + + test('Max retries exceeded w/o resolve', async () => { + op = 'upsert'; + pinecone = new Pinecone({ + apiKey: process.env['PINECONE_API_KEY'] || '', + maxRetries: 3, + }); + + mockServerlessIndex = pinecone + .Index(serverlessIndexName, 'http://localhost:4000') + .namespace(globalNamespaceOne); + + const retrySpy = jest.spyOn(RetryOnServerFailure.prototype, 'execute'); + const delaySpy = jest.spyOn(RetryOnServerFailure.prototype, 'delay'); + + // Start server with persistent 503 errors on every call + startMockServer(false); + + // Catch expected error from Upsert operation + const errorResult = async () => { + await mockServerlessIndex.upsert(recordsToUpsert); + }; + + await expect(errorResult).rejects.toThrowError( + PineconeMaxRetriesExceededError + ); + + // Out of 3 total tries, 2 are retries (i.e. delays), and 1 is the initial call: + expect(retrySpy).toHaveBeenCalledTimes(1); + expect(delaySpy).toHaveBeenCalledTimes(2); + expect(callCount).toBe(3); + }); }); diff --git a/src/utils/__tests__/retries.test.ts b/src/utils/__tests__/retries.test.ts index cd0f1d21..e34f176f 100644 --- a/src/utils/__tests__/retries.test.ts +++ b/src/utils/__tests__/retries.test.ts @@ -112,3 +112,41 @@ describe('isRetryError', () => { expect(result).toBe(true); }); }); + +describe('shouldStopRetrying', () => { + test('Should return true for non-retryable status code', () => { + const retryWrapper = new RetryOnServerFailure(() => Promise.resolve({})); + const result = retryWrapper['shouldStopRetrying']({ status: 400 }); + expect(result).toBe(true); + }); + + test('Should return true for non-retryable error name', () => { + const retryWrapper = new RetryOnServerFailure(() => Promise.resolve({})); + const result = retryWrapper['shouldStopRetrying']({ + name: 'SomeOtherError', + }); + expect(result).toBe(true); + }); + + test('Should return false for retryable error name PineconeUnavailableError', () => { + const retryWrapper = new RetryOnServerFailure(() => Promise.resolve({})); + const result = retryWrapper['shouldStopRetrying']({ + name: 'PineconeUnavailableError', + }); + expect(result).toBe(false); + }); + + test('Should return false for retryable error name PineconeInternalServerError', () => { + const retryWrapper = new RetryOnServerFailure(() => Promise.resolve({})); + const result = retryWrapper['shouldStopRetrying']({ + name: 'PineconeInternalServerError', + }); + expect(result).toBe(false); + }); + + test('Should return false for retryable status code', () => { + const retryWrapper = new RetryOnServerFailure(() => Promise.resolve({})); + const result = retryWrapper['shouldStopRetrying']({ status: 500 }); + expect(result).toBe(false); + }); +}); diff --git a/src/utils/retries.ts b/src/utils/retries.ts index 44a2b2cb..704a18b8 100644 --- a/src/utils/retries.ts +++ b/src/utils/retries.ts @@ -1,4 +1,4 @@ -import { BasePineconeError, mapHttpStatusError, PineconeMaxRetriesExceededError } from '../errors'; +import { mapHttpStatusError, PineconeMaxRetriesExceededError } from '../errors'; /* Retry asynchronous operations. * @@ -24,64 +24,42 @@ export class RetryOnServerFailure { } async execute(...args: A): Promise { - console.log("Retrying!") - console.log("maxRetries =" ,this.maxRetries) if (this.maxRetries < 1) { return this.asyncFn(...args); } for (let attempt = 0; attempt < this.maxRetries; attempt++) { - console.log("Attempt = ", attempt) try { const response = await this.asyncFn(...args); // Return immediately if the response is not a retryable error if (!this.isRetryError(response)) { - console.log("Response is not retryable") return response; } - // Retryable response, throw it to trigger retry logic - console.log("Response is retryable, retrying...") - throw response; - + throw response; // Will catch this in next line } catch (error) { - console.log("Caught retryable error") - const mappedError = this.mapErrorIfNeeded(error); - // If the error is not retryable, throw it immediately if (this.shouldStopRetrying(mappedError)) { - console.log("Should stop retrying retryable error") throw mappedError; } // On the last retry, throw a MaxRetriesExceededError if (attempt === this.maxRetries - 1) { - console.log("Max retries exceeded") throw new PineconeMaxRetriesExceededError(this.maxRetries); } // Wait before retrying - console.log("Waiting before retrying") await this.delay(attempt + 1); } } - console.log("Fell through to max retries exceeded") // This fallback is unnecessary, but included for type safety throw new PineconeMaxRetriesExceededError(this.maxRetries); } - private mapErrorIfNeeded(error: any): any { - if (error?.status) { - return mapHttpStatusError(error); - } - return error; // Return original error if no mapping is needed - } - - isRetryError(response): boolean { if (response) { if ( @@ -99,13 +77,7 @@ export class RetryOnServerFailure { return false; } - // todo: write unit test for this - private shouldStopRetrying(error: any): boolean { - return (error.status && error.status < 500) || (error.name !== 'PineconeUnavailableError' && error.name !== 'PineconeInternalServerError'); - } - async delay(attempt: number): Promise { - console.log("Delaying!") const delayTime = this.calculateRetryDelay(attempt); return new Promise((resolve) => setTimeout(resolve, delayTime)); } @@ -134,4 +106,25 @@ export class RetryOnServerFailure { // Ensure delay is not negative or greater than maxDelay return Math.min(maxDelay, Math.max(0, delay)); }; + + private mapErrorIfNeeded(error: any): any { + if (error?.status) { + return mapHttpStatusError(error); + } + return error; // Return original error if no mapping is needed + } + + // todo: write unit test for this + private shouldStopRetrying(error: any): boolean { + if (error.status) { + return error.status < 500; + } + if (error.name) { + return ( + error.name !== 'PineconeUnavailableError' && + error.name !== 'PineconeInternalServerError' + ); + } + return true; + } } From bb9a6de8f2862ce34bb9a0184799c0862cb92942 Mon Sep 17 00:00:00 2001 From: aulorbe Date: Wed, 18 Dec 2024 17:09:13 -0800 Subject: [PATCH 03/11] Add new ops to README --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index d5a30704..5d516c18 100644 --- a/README.md +++ b/README.md @@ -69,7 +69,7 @@ const pc = new Pinecone(); If you prefer to pass configuration in code, the constructor accepts a config object containing the `apiKey` value. This is the object in which you would pass properties like `maxRetries` (defaults to `3`) for retryable operations -(e.g. `upsert`). +(`upsert`, `update`, and `configureIndex`). ```typescript import { Pinecone } from '@pinecone-database/pinecone'; From 61b34d76ea7895483880b4063825f2b4c16fce9b Mon Sep 17 00:00:00 2001 From: aulorbe Date: Wed, 18 Dec 2024 17:14:08 -0800 Subject: [PATCH 04/11] Uncomment tests, remove todos --- .../data/vectors/upsertAndUpdate.test.ts | 74 +++++++++---------- src/utils/retries.ts | 1 - 2 files changed, 37 insertions(+), 38 deletions(-) diff --git a/src/integration/data/vectors/upsertAndUpdate.test.ts b/src/integration/data/vectors/upsertAndUpdate.test.ts index 38799ced..cbf3c927 100644 --- a/src/integration/data/vectors/upsertAndUpdate.test.ts +++ b/src/integration/data/vectors/upsertAndUpdate.test.ts @@ -46,43 +46,43 @@ afterAll(async () => { }); // todo: add sparse values update -// describe('upsert and update to serverless index', () => { -// test('verify upsert and update', async () => { -// const recordToUpsert = generateRecords({ -// dimension: 2, -// quantity: 1, -// withSparseValues: false, -// withMetadata: true, -// }); -// -// // Upsert record -// await serverlessIndex.upsert(recordToUpsert); -// -// // Build new values -// const newValues = [0.5, 0.4]; -// const newMetadata = { flavor: 'chocolate' }; -// -// const updateSpy = jest -// .spyOn(serverlessIndex, 'update') -// .mockResolvedValue(undefined); -// -// // Update values w/new values -// await serverlessIndex.update({ -// id: '0', -// values: newValues, -// metadata: newMetadata, -// }); -// -// expect(updateSpy).toHaveBeenCalledWith({ -// id: '0', -// values: newValues, -// metadata: newMetadata, -// }); -// -// // Clean up spy after the test -// updateSpy.mockRestore(); -// }); -// }); +describe('upsert and update to serverless index', () => { + test('verify upsert and update', async () => { + const recordToUpsert = generateRecords({ + dimension: 2, + quantity: 1, + withSparseValues: false, + withMetadata: true, + }); + + // Upsert record + await serverlessIndex.upsert(recordToUpsert); + + // Build new values + const newValues = [0.5, 0.4]; + const newMetadata = { flavor: 'chocolate' }; + + const updateSpy = jest + .spyOn(serverlessIndex, 'update') + .mockResolvedValue(undefined); + + // Update values w/new values + await serverlessIndex.update({ + id: '0', + values: newValues, + metadata: newMetadata, + }); + + expect(updateSpy).toHaveBeenCalledWith({ + id: '0', + values: newValues, + metadata: newMetadata, + }); + + // Clean up spy after the test + updateSpy.mockRestore(); + }); +}); // Retry logic tests describe('Testing retry logic via a mock, in-memory http server', () => { diff --git a/src/utils/retries.ts b/src/utils/retries.ts index 704a18b8..34b7d9fa 100644 --- a/src/utils/retries.ts +++ b/src/utils/retries.ts @@ -114,7 +114,6 @@ export class RetryOnServerFailure { return error; // Return original error if no mapping is needed } - // todo: write unit test for this private shouldStopRetrying(error: any): boolean { if (error.status) { return error.status < 500; From 3536e72c8f33d6a33fde6a3238f3b26f3c6b0d54 Mon Sep 17 00:00:00 2001 From: aulorbe Date: Wed, 18 Dec 2024 17:18:20 -0800 Subject: [PATCH 05/11] Idk why format process indented this; fixing so workflow runs --- .github/workflows/testing.yml | 368 +++++++++++++++++----------------- 1 file changed, 184 insertions(+), 184 deletions(-) diff --git a/.github/workflows/testing.yml b/.github/workflows/testing.yml index ae41e755..9bb47dd1 100644 --- a/.github/workflows/testing.yml +++ b/.github/workflows/testing.yml @@ -26,190 +26,190 @@ jobs: SERVERLESS_INDEX_NAME=$(npx ts-node ./src/integration/setup.ts | grep "SERVERLESS_INDEX_NAME=" | cut -d'=' -f2) echo "SERVERLESS_INDEX_NAME=$SERVERLESS_INDEX_NAME" >> $GITHUB_OUTPUT - unit-tests: - needs: setup - name: Run unit tests - runs-on: ubuntu-latest - steps: - - name: Checkout - uses: actions/checkout@v4 - - - name: Setup - uses: ./.github/actions/setup - - - name: Run tests - env: - CI: true - run: npm run test:unit -- --coverage - - integration-tests: - needs: setup - name: Run integration tests - runs-on: ubuntu-latest - outputs: - serverlessIndexName: ${{ steps.runTests1.outputs.SERVERLESS_INDEX_NAME }} - strategy: - fail-fast: false - max-parallel: 2 - matrix: - pinecone_env: - - prod - # - staging - node_version: [18.x, 20.x] - config: - [ - { runner: 'npm', jest_env: 'node' }, - { runner: 'npm', jest_env: 'edge' }, - { runner: 'bun', jest_env: 'node', bun_version: '1.0.0' }, - { runner: 'bun', jest_env: 'node', bun_version: '1.0.36' }, - { runner: 'bun', jest_env: 'node', bun_version: '1.1.11' }, - ] - - steps: - - name: Checkout - uses: actions/checkout@v4 - - - name: Setup - uses: ./.github/actions/setup - with: - node_version: ${{ matrix.node_version }} - - - name: Setup bun - if: matrix.config.bun_version - uses: oven-sh/setup-bun@v1 - with: - bun-version: ${{ matrix.config.bun_version }} - - - name: Run integration tests (Prod) - id: runTests1 - if: matrix.pinecone_env == 'prod' - env: - CI: true - PINECONE_API_KEY: ${{ secrets.PINECONE_API_KEY }} - SERVERLESS_INDEX_NAME: ${{ needs.setup.outputs.serverlessIndexName}} - run: | - ${{ matrix.config.runner }} run test:integration:${{ matrix.config.jest_env }} - echo "SERVERLESS_INDEX_NAME=${{ needs.setup.outputs.serverlessIndexName}}" >> $GITHUB_OUTPUT - - - name: Run integration tests (Staging) - if: matrix.pinecone_env == 'staging' - env: - CI: true - PINECONE_API_KEY: ${{ secrets.PINECONE_API_KEY }} - PINECONE_CONTROLLER_HOST: 'https://api-staging.pinecone.io' - run: ${{ matrix.config.runner }} run test:integration:${{ matrix.config.jest_env }} - - teardown: - name: Teardown - needs: integration-tests # Ensure teardown only runs after test jobs - runs-on: ubuntu-latest - if: always() # Run teardown even if the tests fail - steps: - - name: Checkout code - uses: actions/checkout@v4 - - - name: Install dependencies - run: npm ci - - - name: Run teardown script - env: - PINECONE_API_KEY: ${{ secrets.PINECONE_API_KEY }} - SERVERLESS_INDEX_NAME: ${{ needs.integration-tests.outputs.serverlessIndexName}} - run: | - npx ts-node ./src/integration/teardown.ts - - typescript-compilation-tests: - name: TS compile - runs-on: ubuntu-latest - strategy: - fail-fast: false - matrix: - tsVersion: - [ - '~4.1.0', - '~4.2.0', - '~4.3.0', - '~4.4.0', - '~4.5.0', - '~4.6.0', - '~4.7.0', - '~4.8.0', - '~4.9.0', - '~5.0.0', - '~5.1.0', - '~5.2.0', - 'latest', - ] - steps: - - name: Checkout - uses: actions/checkout@v4 - - name: Setup Node - uses: actions/setup-node@v4 - with: - node-version: 18.x - registry-url: 'https://registry.npmjs.org' - - name: Install npm packages - run: | - npm install --ignore-scripts - shell: bash - - name: Build TypeScript for Pinecone - run: npm run build - shell: bash - - name: install, compile, and run sample app - shell: bash - env: - PINECONE_API_KEY: ${{ secrets.PINECONE_API_KEY }} - run: | - set -x - cd .. - cp -r pinecone-ts-client/ts-compilation-test ts-compilation-test - cd ts-compilation-test - npm install typescript@${{ matrix.tsVersion }} --no-cache - npm install '../pinecone-ts-client' --no-cache - cat package.json - cat package-lock.json - npm run tsc-version - npm run build - npm run start - - example-app-semantic-search: - name: 'Example app: semantic search' - runs-on: ubuntu-latest - steps: - - name: Checkout pinecone-ts-client - uses: actions/checkout@v4 - with: - path: pinecone-ts-client - - name: Checkout semantic-search-example - uses: actions/checkout@v4 - with: - repository: pinecone-io/semantic-search-example - ref: spruce - path: semantic-search-example - - name: Install and build client - shell: bash - run: | - cd pinecone-ts-client - npm install --ignore-scripts - npm run build - - name: Install and build sample app - shell: bash - run: | - cd semantic-search-example - npm install --no-cache - npm install '../pinecone-ts-client' - cat package.json - - name: Run example tests with dev version of client - env: - CI: true - PINECONE_API_KEY: ${{ secrets.PINECONE_API_KEY }} - PINECONE_INDEX: 'semantic-search' - PINECONE_CLOUD: 'aws' - PINECONE_REGION: 'us-west-2' - shell: bash - run: | - cd semantic-search-example - npm run test + unit-tests: + needs: setup + name: Run unit tests + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Setup + uses: ./.github/actions/setup + + - name: Run tests + env: + CI: true + run: npm run test:unit -- --coverage + + integration-tests: + needs: setup + name: Run integration tests + runs-on: ubuntu-latest + outputs: + serverlessIndexName: ${{ steps.runTests1.outputs.SERVERLESS_INDEX_NAME }} + strategy: + fail-fast: false + max-parallel: 2 + matrix: + pinecone_env: + - prod + # - staging + node_version: [18.x, 20.x] + config: + [ + { runner: 'npm', jest_env: 'node' }, + { runner: 'npm', jest_env: 'edge' }, + { runner: 'bun', jest_env: 'node', bun_version: '1.0.0' }, + { runner: 'bun', jest_env: 'node', bun_version: '1.0.36' }, + { runner: 'bun', jest_env: 'node', bun_version: '1.1.11' }, + ] + + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Setup + uses: ./.github/actions/setup + with: + node_version: ${{ matrix.node_version }} + + - name: Setup bun + if: matrix.config.bun_version + uses: oven-sh/setup-bun@v1 + with: + bun-version: ${{ matrix.config.bun_version }} + + - name: Run integration tests (Prod) + id: runTests1 + if: matrix.pinecone_env == 'prod' + env: + CI: true + PINECONE_API_KEY: ${{ secrets.PINECONE_API_KEY }} + SERVERLESS_INDEX_NAME: ${{ needs.setup.outputs.serverlessIndexName}} + run: | + ${{ matrix.config.runner }} run test:integration:${{ matrix.config.jest_env }} + echo "SERVERLESS_INDEX_NAME=${{ needs.setup.outputs.serverlessIndexName}}" >> $GITHUB_OUTPUT + + - name: Run integration tests (Staging) + if: matrix.pinecone_env == 'staging' + env: + CI: true + PINECONE_API_KEY: ${{ secrets.PINECONE_API_KEY }} + PINECONE_CONTROLLER_HOST: 'https://api-staging.pinecone.io' + run: ${{ matrix.config.runner }} run test:integration:${{ matrix.config.jest_env }} + + teardown: + name: Teardown + needs: integration-tests # Ensure teardown only runs after test jobs + runs-on: ubuntu-latest + if: always() # Run teardown even if the tests fail + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Install dependencies + run: npm ci + + - name: Run teardown script + env: + PINECONE_API_KEY: ${{ secrets.PINECONE_API_KEY }} + SERVERLESS_INDEX_NAME: ${{ needs.integration-tests.outputs.serverlessIndexName}} + run: | + npx ts-node ./src/integration/teardown.ts + + typescript-compilation-tests: + name: TS compile + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + tsVersion: + [ + '~4.1.0', + '~4.2.0', + '~4.3.0', + '~4.4.0', + '~4.5.0', + '~4.6.0', + '~4.7.0', + '~4.8.0', + '~4.9.0', + '~5.0.0', + '~5.1.0', + '~5.2.0', + 'latest', + ] + steps: + - name: Checkout + uses: actions/checkout@v4 + - name: Setup Node + uses: actions/setup-node@v4 + with: + node-version: 18.x + registry-url: 'https://registry.npmjs.org' + - name: Install npm packages + run: | + npm install --ignore-scripts + shell: bash + - name: Build TypeScript for Pinecone + run: npm run build + shell: bash + - name: install, compile, and run sample app + shell: bash + env: + PINECONE_API_KEY: ${{ secrets.PINECONE_API_KEY }} + run: | + set -x + cd .. + cp -r pinecone-ts-client/ts-compilation-test ts-compilation-test + cd ts-compilation-test + npm install typescript@${{ matrix.tsVersion }} --no-cache + npm install '../pinecone-ts-client' --no-cache + cat package.json + cat package-lock.json + npm run tsc-version + npm run build + npm run start + + example-app-semantic-search: + name: 'Example app: semantic search' + runs-on: ubuntu-latest + steps: + - name: Checkout pinecone-ts-client + uses: actions/checkout@v4 + with: + path: pinecone-ts-client + - name: Checkout semantic-search-example + uses: actions/checkout@v4 + with: + repository: pinecone-io/semantic-search-example + ref: spruce + path: semantic-search-example + - name: Install and build client + shell: bash + run: | + cd pinecone-ts-client + npm install --ignore-scripts + npm run build + - name: Install and build sample app + shell: bash + run: | + cd semantic-search-example + npm install --no-cache + npm install '../pinecone-ts-client' + cat package.json + - name: Run example tests with dev version of client + env: + CI: true + PINECONE_API_KEY: ${{ secrets.PINECONE_API_KEY }} + PINECONE_INDEX: 'semantic-search' + PINECONE_CLOUD: 'aws' + PINECONE_REGION: 'us-west-2' + shell: bash + run: | + cd semantic-search-example + npm run test external-app: name: external-app From 6e357c1fe385196bbffac0c630a648603035f30a Mon Sep 17 00:00:00 2001 From: aulorbe Date: Wed, 18 Dec 2024 17:33:00 -0800 Subject: [PATCH 06/11] Add server.unref to deal with diff versions of Node and http connections --- .../data/vectors/upsertAndUpdate.test.ts | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/integration/data/vectors/upsertAndUpdate.test.ts b/src/integration/data/vectors/upsertAndUpdate.test.ts index cbf3c927..eb413a30 100644 --- a/src/integration/data/vectors/upsertAndUpdate.test.ts +++ b/src/integration/data/vectors/upsertAndUpdate.test.ts @@ -125,6 +125,12 @@ describe('Testing retry logic via a mock, in-memory http server', () => { } }); server.listen(4000); // Host server on local port 4000 + server.unref(); // Allow the server to close even if there are open connections + + // Error handler + server.on('error', (error) => { + console.error('Server error:', error); + }); }; beforeEach(() => { @@ -133,8 +139,14 @@ describe('Testing retry logic via a mock, in-memory http server', () => { afterEach(async () => { // Close server and reset mocks - await new Promise((resolve) => server.close(() => resolve())); - jest.clearAllMocks(); + // await new Promise((resolve) => server.close(() => resolve())); + return new Promise((resolve) => { + server.close(() => { + jest.clearAllMocks(); + server.unref(); + resolve(); + }); + }); }); test('Upsert operation should retry 1x if server responds 1x with error and 1x with success', async () => { From 6c5ba34cc78517e29eb447df600b883e3bbf8bc5 Mon Sep 17 00:00:00 2001 From: aulorbe Date: Wed, 18 Dec 2024 17:38:19 -0800 Subject: [PATCH 07/11] Revert "Add server.unref to deal with diff versions of Node and http connections" This reverts commit 6e357c1fe385196bbffac0c630a648603035f30a. --- .../data/vectors/upsertAndUpdate.test.ts | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/src/integration/data/vectors/upsertAndUpdate.test.ts b/src/integration/data/vectors/upsertAndUpdate.test.ts index eb413a30..cbf3c927 100644 --- a/src/integration/data/vectors/upsertAndUpdate.test.ts +++ b/src/integration/data/vectors/upsertAndUpdate.test.ts @@ -125,12 +125,6 @@ describe('Testing retry logic via a mock, in-memory http server', () => { } }); server.listen(4000); // Host server on local port 4000 - server.unref(); // Allow the server to close even if there are open connections - - // Error handler - server.on('error', (error) => { - console.error('Server error:', error); - }); }; beforeEach(() => { @@ -139,14 +133,8 @@ describe('Testing retry logic via a mock, in-memory http server', () => { afterEach(async () => { // Close server and reset mocks - // await new Promise((resolve) => server.close(() => resolve())); - return new Promise((resolve) => { - server.close(() => { - jest.clearAllMocks(); - server.unref(); - resolve(); - }); - }); + await new Promise((resolve) => server.close(() => resolve())); + jest.clearAllMocks(); }); test('Upsert operation should retry 1x if server responds 1x with error and 1x with success', async () => { From 133b3494a9c061e091a752f0beb22424041a3ca0 Mon Sep 17 00:00:00 2001 From: aulorbe Date: Wed, 18 Dec 2024 17:46:34 -0800 Subject: [PATCH 08/11] Try keepAlive=False since it changed default from False to True in Node20 --- src/integration/data/vectors/upsertAndUpdate.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/integration/data/vectors/upsertAndUpdate.test.ts b/src/integration/data/vectors/upsertAndUpdate.test.ts index cbf3c927..9996bf38 100644 --- a/src/integration/data/vectors/upsertAndUpdate.test.ts +++ b/src/integration/data/vectors/upsertAndUpdate.test.ts @@ -101,7 +101,7 @@ describe('Testing retry logic via a mock, in-memory http server', () => { // Helper function to start the server with a specific response pattern const startMockServer = (shouldSucceedOnSecondCall: boolean) => { // Create http server - server = http.createServer((req, res) => { + server = http.createServer({ keepAlive: false }, (req, res) => { const { pathname } = parse(req.url || '', true); if (req.method === 'POST' && pathname === `/vectors/${op}`) { callCount++; From 28c1c64ebeb0eabc416c4b4fe1e77da37a663438 Mon Sep 17 00:00:00 2001 From: aulorbe Date: Mon, 23 Dec 2024 10:46:38 -0800 Subject: [PATCH 09/11] Add timeout to bypass node20+ errors --- .../data/vectors/upsertAndUpdate.test.ts | 78 ++++++++++--------- 1 file changed, 41 insertions(+), 37 deletions(-) diff --git a/src/integration/data/vectors/upsertAndUpdate.test.ts b/src/integration/data/vectors/upsertAndUpdate.test.ts index 9996bf38..d0c9deaf 100644 --- a/src/integration/data/vectors/upsertAndUpdate.test.ts +++ b/src/integration/data/vectors/upsertAndUpdate.test.ts @@ -6,6 +6,7 @@ import { generateRecords, globalNamespaceOne, randomIndexName, + sleep, waitUntilReady, } from '../../test-helpers'; import { RetryOnServerFailure } from '../../../utils'; @@ -46,43 +47,43 @@ afterAll(async () => { }); // todo: add sparse values update -describe('upsert and update to serverless index', () => { - test('verify upsert and update', async () => { - const recordToUpsert = generateRecords({ - dimension: 2, - quantity: 1, - withSparseValues: false, - withMetadata: true, - }); - - // Upsert record - await serverlessIndex.upsert(recordToUpsert); - - // Build new values - const newValues = [0.5, 0.4]; - const newMetadata = { flavor: 'chocolate' }; - - const updateSpy = jest - .spyOn(serverlessIndex, 'update') - .mockResolvedValue(undefined); - - // Update values w/new values - await serverlessIndex.update({ - id: '0', - values: newValues, - metadata: newMetadata, - }); - - expect(updateSpy).toHaveBeenCalledWith({ - id: '0', - values: newValues, - metadata: newMetadata, - }); - - // Clean up spy after the test - updateSpy.mockRestore(); - }); -}); +// describe('upsert and update to serverless index', () => { +// test('verify upsert and update', async () => { +// const recordToUpsert = generateRecords({ +// dimension: 2, +// quantity: 1, +// withSparseValues: false, +// withMetadata: true, +// }); +// +// // Upsert record +// await serverlessIndex.upsert(recordToUpsert); +// +// // Build new values +// const newValues = [0.5, 0.4]; +// const newMetadata = { flavor: 'chocolate' }; +// +// const updateSpy = jest +// .spyOn(serverlessIndex, 'update') +// .mockResolvedValue(undefined); +// +// // Update values w/new values +// await serverlessIndex.update({ +// id: '0', +// values: newValues, +// metadata: newMetadata, +// }); +// +// expect(updateSpy).toHaveBeenCalledWith({ +// id: '0', +// values: newValues, +// metadata: newMetadata, +// }); +// +// // Clean up spy after the test +// updateSpy.mockRestore(); +// }); +// }); // Retry logic tests describe('Testing retry logic via a mock, in-memory http server', () => { @@ -198,6 +199,9 @@ describe('Testing retry logic via a mock, in-memory http server', () => { test('Max retries exceeded w/o resolve', async () => { op = 'upsert'; + + await sleep(500); // In Node20+, tcp connection closures and resources cleanup takes longer + pinecone = new Pinecone({ apiKey: process.env['PINECONE_API_KEY'] || '', maxRetries: 3, From a04bdd0493290989a7ea1dfd7296428361fb17ac Mon Sep 17 00:00:00 2001 From: aulorbe Date: Mon, 23 Dec 2024 11:12:23 -0800 Subject: [PATCH 10/11] Cleanup --- .../data/vectors/upsertAndUpdate.test.ts | 76 +++++++++---------- 1 file changed, 38 insertions(+), 38 deletions(-) diff --git a/src/integration/data/vectors/upsertAndUpdate.test.ts b/src/integration/data/vectors/upsertAndUpdate.test.ts index d0c9deaf..98dbc5d3 100644 --- a/src/integration/data/vectors/upsertAndUpdate.test.ts +++ b/src/integration/data/vectors/upsertAndUpdate.test.ts @@ -47,43 +47,43 @@ afterAll(async () => { }); // todo: add sparse values update -// describe('upsert and update to serverless index', () => { -// test('verify upsert and update', async () => { -// const recordToUpsert = generateRecords({ -// dimension: 2, -// quantity: 1, -// withSparseValues: false, -// withMetadata: true, -// }); -// -// // Upsert record -// await serverlessIndex.upsert(recordToUpsert); -// -// // Build new values -// const newValues = [0.5, 0.4]; -// const newMetadata = { flavor: 'chocolate' }; -// -// const updateSpy = jest -// .spyOn(serverlessIndex, 'update') -// .mockResolvedValue(undefined); -// -// // Update values w/new values -// await serverlessIndex.update({ -// id: '0', -// values: newValues, -// metadata: newMetadata, -// }); -// -// expect(updateSpy).toHaveBeenCalledWith({ -// id: '0', -// values: newValues, -// metadata: newMetadata, -// }); -// -// // Clean up spy after the test -// updateSpy.mockRestore(); -// }); -// }); +describe('upsert and update to serverless index', () => { + test('verify upsert and update', async () => { + const recordToUpsert = generateRecords({ + dimension: 2, + quantity: 1, + withSparseValues: false, + withMetadata: true, + }); + + // Upsert record + await serverlessIndex.upsert(recordToUpsert); + + // Build new values + const newValues = [0.5, 0.4]; + const newMetadata = { flavor: 'chocolate' }; + + const updateSpy = jest + .spyOn(serverlessIndex, 'update') + .mockResolvedValue(undefined); + + // Update values w/new values + await serverlessIndex.update({ + id: '0', + values: newValues, + metadata: newMetadata, + }); + + expect(updateSpy).toHaveBeenCalledWith({ + id: '0', + values: newValues, + metadata: newMetadata, + }); + + // Clean up spy after the test + updateSpy.mockRestore(); + }); +}); // Retry logic tests describe('Testing retry logic via a mock, in-memory http server', () => { @@ -200,7 +200,7 @@ describe('Testing retry logic via a mock, in-memory http server', () => { test('Max retries exceeded w/o resolve', async () => { op = 'upsert'; - await sleep(500); // In Node20+, tcp connection closures and resources cleanup takes longer + await sleep(500); // In Node20+, tcp connections changed: https://github.com/pinecone-io/pinecone-ts-client/pull/318#issuecomment-2560180936 pinecone = new Pinecone({ apiKey: process.env['PINECONE_API_KEY'] || '', From 591b62474e8b92dec51e41792b424db975ed657f Mon Sep 17 00:00:00 2001 From: aulorbe Date: Mon, 23 Dec 2024 11:32:48 -0800 Subject: [PATCH 11/11] Remove `false` --- src/integration/data/vectors/upsertAndUpdate.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/integration/data/vectors/upsertAndUpdate.test.ts b/src/integration/data/vectors/upsertAndUpdate.test.ts index 98dbc5d3..03deb1fa 100644 --- a/src/integration/data/vectors/upsertAndUpdate.test.ts +++ b/src/integration/data/vectors/upsertAndUpdate.test.ts @@ -102,7 +102,7 @@ describe('Testing retry logic via a mock, in-memory http server', () => { // Helper function to start the server with a specific response pattern const startMockServer = (shouldSucceedOnSecondCall: boolean) => { // Create http server - server = http.createServer({ keepAlive: false }, (req, res) => { + server = http.createServer((req, res) => { const { pathname } = parse(req.url || '', true); if (req.method === 'POST' && pathname === `/vectors/${op}`) { callCount++;