From e80735f73f9e81224e1a0e6d11767e08620880e1 Mon Sep 17 00:00:00 2001 From: feywind <57276408+feywind@users.noreply.github.com> Date: Tue, 28 Jul 2026 15:29:32 -0400 Subject: [PATCH 1/6] tests: clean up some resource leaks for system tests --- .../bigtable/system-test/app-profile.ts | 3 +- handwritten/bigtable/system-test/bigtable.ts | 61 +++---------------- .../client-side-metrics-all-methods.ts | 4 +- .../system-test/client-side-metrics.ts | 4 +- handwritten/bigtable/system-test/cluster.ts | 8 ++- handwritten/bigtable/system-test/common.ts | 51 ++++++++++++++++ .../read-modify-write-row-interceptors.ts | 4 +- handwritten/bigtable/system-test/tpc-tests.ts | 17 +++--- 8 files changed, 85 insertions(+), 67 deletions(-) diff --git a/handwritten/bigtable/system-test/app-profile.ts b/handwritten/bigtable/system-test/app-profile.ts index 6c3286497197..4b8477e8fbaf 100644 --- a/handwritten/bigtable/system-test/app-profile.ts +++ b/handwritten/bigtable/system-test/app-profile.ts @@ -13,7 +13,7 @@ // limitations under the License. import {describe, it, before, after} from 'mocha'; -import {generateId} from './common'; +import {generateId, reapInstances} from './common'; import {AppProfileOptions, Bigtable, Instance} from '../src'; import {AppProfile} from '../src'; import assert = require('assert'); @@ -38,6 +38,7 @@ describe('📦 App Profile', () => { } before(async () => { + await reapInstances(bigtable); // Creates an instance with clusters const instanceClusters = [ 'us-east1-c', diff --git a/handwritten/bigtable/system-test/bigtable.ts b/handwritten/bigtable/system-test/bigtable.ts index e83489adbc7f..35672a5915a0 100644 --- a/handwritten/bigtable/system-test/bigtable.ts +++ b/handwritten/bigtable/system-test/bigtable.ts @@ -35,7 +35,7 @@ import {Family} from '../src/family.js'; import {Row} from '../src/row.js'; import {Table} from '../src/table.js'; import {RawFilter} from '../src/filter'; -import {generateId, PREFIX} from './common'; +import {generateId, reapBackups, reapInstances} from './common'; import {BigtableTableAdminClient} from '../src/v2'; import {ServiceError} from 'google-gax'; import {BigtableDate, QueryResultRow} from '../src/execute-query/values'; @@ -56,49 +56,8 @@ describe('Bigtable', () => { const CLUSTER_ID = generateId('cluster'); const CLUSTER_ID_HDD = generateId('cluster'); - async function reapBackups(instance: Instance) { - try { - const [backups] = await instance.getBackups(); - for (const backup of backups) { - try { - await backup.delete({timeout: 50 * 1000}); - } catch (e) { - console.log(`Error deleting backup: ${backup.id}: ${e}`); - } - } - } catch (e) { - console.error(`Error listing backups from ${instance.name}: ${e}`); - } - } - - async function reapInstances() { - const [instances] = await bigtable.getInstances(); - const testInstances = instances - .filter(i => i.id.match(PREFIX)) - .filter(i => { - const timeCreated = i.metadata!.labels!.time_created as {} as Date; - // Only delete stale resources. - const oneHourAgo = new Date(Date.now() - 3600000); - return !timeCreated || timeCreated <= oneHourAgo; - }); - // need to delete backups first due to instance deletion precondition - const deleteBackupPromises = testInstances.map(instance => - reapBackups(instance), - ); - for (const backupPromise of deleteBackupPromises) { - await backupPromise; - } - for (const instance of testInstances) { - try { - await instance.delete(); - } catch (e) { - console.log(`Error deleting instance: ${instance.id}`); - } - } - } - before(async () => { - await reapInstances(); + await reapInstances(bigtable); const [, operation] = await INSTANCE.create({ clusters: [ { @@ -140,22 +99,20 @@ describe('Bigtable', () => { }); after(async () => { - const q = []; const instances = [INSTANCE, DIFF_INSTANCE, CMEK_INSTANCE, INSTANCE_HDD]; // need to delete backups first due to instance deletion precondition await Promise.all(instances.map(instance => reapBackups(instance))); await Promise.all( - instances.map(instance => { - q.push(async () => { - try { - await instance.delete(); - } catch (e) { - console.log(`Error deleting instance: ${instance.id}`); - } - }); + instances.map(async instance => { + try { + await instance.delete(); + } catch (e) { + console.log(`Error deleting instance: ${instance.id}`); + } }), ); + await reapInstances(bigtable); }); describe('instances', () => { diff --git a/handwritten/bigtable/system-test/client-side-metrics-all-methods.ts b/handwritten/bigtable/system-test/client-side-metrics-all-methods.ts index b01043833338..3747418ad011 100644 --- a/handwritten/bigtable/system-test/client-side-metrics-all-methods.ts +++ b/handwritten/bigtable/system-test/client-side-metrics-all-methods.ts @@ -38,7 +38,7 @@ import {ClientOptions} from 'google-gax'; import {ClientSideMetricsConfigManager} from '../src/client-side-metrics/metrics-config-manager'; import {MetricServiceClient} from '@google-cloud/monitoring'; import {MethodName} from '../src/client-side-metrics/client-side-metrics-attributes'; -import {generateId} from './common'; +import {generateId, reapInstances} from './common'; const SECOND_PROJECT_ID = 'cfdb-sdk-node-tests'; const instanceId1 = generateId('instance'); @@ -357,6 +357,7 @@ describe('Bigtable/ClientSideMetricsAllMethods', () => { let defaultProjectId: string; before(async () => { + await reapInstances(new Bigtable()); /* For both the default project and the secondary project we need to create instances with some data in them so that the tests can collect all the @@ -413,6 +414,7 @@ describe('Bigtable/ClientSideMetricsAllMethods', () => { console.warn('The instance has been deleted already'); } } + await reapInstances(new Bigtable()); }); describe('Bigtable/ClientSideMetricsToGCM', () => { diff --git a/handwritten/bigtable/system-test/client-side-metrics.ts b/handwritten/bigtable/system-test/client-side-metrics.ts index f9baa3ef8596..1be4d894db4f 100644 --- a/handwritten/bigtable/system-test/client-side-metrics.ts +++ b/handwritten/bigtable/system-test/client-side-metrics.ts @@ -38,7 +38,7 @@ import {PassThrough} from 'stream'; import {generateChunksFromRequest} from '../test-common/utils/readRowsImpl'; import {TabularApiSurface} from '../src/tabular-api-surface'; import {MetricServiceClient} from '@google-cloud/monitoring'; -import {generateId} from './common'; +import {generateId, reapInstances} from './common'; const SECOND_PROJECT_ID = 'cfdb-sdk-node-tests'; const instanceId1 = generateId('instance'); @@ -334,6 +334,7 @@ describe('Bigtable/ClientSideMetrics', () => { let defaultProjectId: string; before(async () => { + await reapInstances(new Bigtable()); /* For both the default project and the secondary project we need to create instances with some data in them so that the tests can collect all the @@ -390,6 +391,7 @@ describe('Bigtable/ClientSideMetrics', () => { console.warn('The instance has been deleted already'); } } + await reapInstances(new Bigtable()); }); describe('Bigtable/ClientSideMetricsToGCM', () => { diff --git a/handwritten/bigtable/system-test/cluster.ts b/handwritten/bigtable/system-test/cluster.ts index 8e1c9e9f4a64..03f7c2f4a648 100644 --- a/handwritten/bigtable/system-test/cluster.ts +++ b/handwritten/bigtable/system-test/cluster.ts @@ -12,8 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. -import {afterEach, beforeEach, describe, it} from 'mocha'; -import {generateId} from './common'; +import {afterEach, before, beforeEach, describe, it} from 'mocha'; +import {generateId, reapInstances} from './common'; import {Bigtable, ClusterInfo, Instance, Cluster} from '../src'; import assert = require('assert'); import {ClusterUtils} from '../src/utils/cluster'; @@ -31,6 +31,10 @@ describe('Cluster', () => { const bigtable = new Bigtable(); let instance: Instance; + before(async () => { + await reapInstances(bigtable); + }); + async function checkMetadata( cluster: Cluster, compareValues: SetClusterMetadataOptions, diff --git a/handwritten/bigtable/system-test/common.ts b/handwritten/bigtable/system-test/common.ts index 06560422f17e..1736366eacfe 100644 --- a/handwritten/bigtable/system-test/common.ts +++ b/handwritten/bigtable/system-test/common.ts @@ -16,6 +16,8 @@ import * as uuid from 'uuid'; import {Cluster} from '../src/cluster'; import * as inst from '../src/instance'; +import {Bigtable, Instance} from '../src'; + export const PREFIX = 'gt-'; export function generateId(resourceType: string) { @@ -23,6 +25,55 @@ export function generateId(resourceType: string) { return `${PREFIX}${resourceType}-${newUuid}-${Date.now()}`; } +export async function reapBackups(instance: Instance) { + try { + const [backups] = await instance.getBackups(); + for (const backup of backups) { + try { + await backup.delete({timeout: 50 * 1000}); + } catch (e) { + console.log(`Error deleting backup: ${backup.id}: ${e}`); + } + } + } catch (e) { + console.error(`Error listing backups from ${instance.name}: ${e}`); + } +} + +export async function reapInstances( + bigtable: Bigtable, + maxAgeMs = 15 * 60 * 1000, +) { + try { + const [instances] = await bigtable.getInstances(); + const testInstances = instances + .filter(i => i.id.match(PREFIX)) + .filter(i => { + const timeCreated = i.metadata!.labels!.time_created as {} as Date; + // Only delete stale resources. + const staleThreshold = new Date(Date.now() - maxAgeMs); + return !timeCreated || timeCreated <= staleThreshold; + }); + + // need to delete backups first due to instance deletion precondition + const deleteBackupPromises = testInstances.map(instance => + reapBackups(instance), + ); + for (const backupPromise of deleteBackupPromises) { + await backupPromise; + } + for (const instance of testInstances) { + try { + await instance.delete(); + } catch (e) { + console.log(`Error deleting instance: ${instance.id}: ${e}`); + } + } + } catch (e) { + console.error(`Error reaping instances: ${e}`); + } +} + export class FakeCluster extends Cluster { calledWith_: Array<{}>; constructor(...args: [inst.Instance, string]) { diff --git a/handwritten/bigtable/system-test/read-modify-write-row-interceptors.ts b/handwritten/bigtable/system-test/read-modify-write-row-interceptors.ts index 420b7146ed89..fe3e0d7234f5 100644 --- a/handwritten/bigtable/system-test/read-modify-write-row-interceptors.ts +++ b/handwritten/bigtable/system-test/read-modify-write-row-interceptors.ts @@ -29,8 +29,9 @@ import { import * as assert from 'assert'; import {status as GrpcStatus} from '@grpc/grpc-js'; import {createMetricsUnaryInterceptorProvider} from '../src/client-side-metrics/metric-interceptor'; +import {generateId, reapInstances} from './common'; -const INSTANCE_ID = 'isolated-rmw-instance'; +const INSTANCE_ID = generateId('isolated-rmw-inst'); const TABLE_ID = 'isolated-rmw-table'; const ZONE = 'us-central2-a'; const CLUSTER = 'fake-cluster'; @@ -152,6 +153,7 @@ describe('Bigtable/ReadModifyWriteRowInterceptorMetrics', () => { before(async () => { bigtable = new Bigtable(); + await reapInstances(bigtable); await getProjectIdFromClient(bigtable); await createInstance(bigtable, INSTANCE_ID, CLUSTER, ZONE); await createTable(bigtable, INSTANCE_ID, TABLE_ID, COLUMN_FAMILIES); diff --git a/handwritten/bigtable/system-test/tpc-tests.ts b/handwritten/bigtable/system-test/tpc-tests.ts index 68c97be5d3a2..c7154ee040ec 100644 --- a/handwritten/bigtable/system-test/tpc-tests.ts +++ b/handwritten/bigtable/system-test/tpc-tests.ts @@ -14,6 +14,7 @@ import {after, describe, it} from 'mocha'; import {Bigtable} from '../src'; +import {generateId} from './common'; // INSTRUCTIONS FOR RUNNING TEST: // 1. Change describe.skip to describe.only below. @@ -70,19 +71,17 @@ describe.skip('Universe domain tests', () => { } } - const instanceId = 'emulator-test-instance'; + const instanceId = generateId('emu-test-inst'); const tableId = 'my-table'; const columnFamilyId = 'cf1'; after(async () => { - // TODO: Solve the issue where tests fail because instances don't get created on time. - // Notes: Creating instances can take time and if they are not ready in - // time then tests can fail. This shouldn't happen because if the create - // instance long running operation completes then the instance should be - // ready and shouldn't produce the `Error: 5 NOT_FOUND` error. - // Uncomment the code below when the task above is addressed: - // const instance = bigtable.instance(instanceId); - // await instance.delete({}); + try { + const instance = new Bigtable().instance(instanceId); + await instance.delete({}); + } catch (e) { + console.log(`Error deleting instance ${instanceId}: ${e}`); + } }); it('should set the universe with a client option', done => { From 55449ee876e950ab083f1b746cca404ebb19ddd9 Mon Sep 17 00:00:00 2001 From: feywind <57276408+feywind@users.noreply.github.com> Date: Wed, 29 Jul 2026 13:29:29 -0400 Subject: [PATCH 2/6] tests: lower some resource usage in system tests, and catch more leaks --- handwritten/bigtable/system-test/bigtable.ts | 20 +++--- handwritten/bigtable/system-test/common.ts | 7 +- handwritten/bigtable/system-test/read-rows.ts | 65 ++++++++++--------- 3 files changed, 51 insertions(+), 41 deletions(-) diff --git a/handwritten/bigtable/system-test/bigtable.ts b/handwritten/bigtable/system-test/bigtable.ts index 35672a5915a0..d92d599598be 100644 --- a/handwritten/bigtable/system-test/bigtable.ts +++ b/handwritten/bigtable/system-test/bigtable.ts @@ -63,7 +63,7 @@ describe('Bigtable', () => { { id: CLUSTER_ID, location: 'us-central2-c', - nodes: 3, + nodes: 1, storage: 'ssd', }, ], @@ -76,7 +76,7 @@ describe('Bigtable', () => { { id: CLUSTER_ID_HDD, location: 'us-central2-c', - nodes: 3, + nodes: 1, storage: 'hdd', }, ], @@ -172,7 +172,7 @@ describe('Bigtable', () => { { id: clusteId, location: 'us-central2-c', - nodes: 3, + nodes: 1, }, ], labels: { @@ -225,7 +225,7 @@ describe('Bigtable', () => { { id: CMEK_CLUSTER.id, location: 'us-central2-a', - nodes: 3, + nodes: 1, key: kmsKeyName, }, ], @@ -257,7 +257,7 @@ describe('Bigtable', () => { // eslint-disable-next-line @typescript-eslint/no-unused-vars const [_, operation] = await cluster.create({ location: 'us-central2-b', - nodes: 3, + nodes: 1, key: kmsKeyName, }); await operation.promise(); @@ -273,7 +273,7 @@ describe('Bigtable', () => { // eslint-disable-next-line @typescript-eslint/no-unused-vars const [_, operation] = await cluster.create({ location: 'us-central2-b', - nodes: 3, + nodes: 1, }); await operation.promise(); throw new Error('Cluster creation should not have succeeded'); @@ -389,7 +389,7 @@ describe('Bigtable', () => { it('should update a cluster', async () => { const metadata = { - nodes: 4, + nodes: 2, }; const [operation] = await CLUSTER.setMetadata(metadata); await operation.promise(); @@ -1673,7 +1673,7 @@ describe('Bigtable', () => { clusters: [ { id: destinationClusterId, - nodes: 3, + nodes: 1, location: 'us-central2-d', storage: 'ssd', }, @@ -1717,7 +1717,7 @@ describe('Bigtable', () => { destinationClusterId, ).create({ location: 'us-central2-b', - nodes: 3, + nodes: 1, }); await operation.promise(); } @@ -1762,7 +1762,7 @@ describe('Bigtable', () => { clusters: [ { id: destinationClusterId, - nodes: 3, + nodes: 1, location: 'us-central2-d', storage: 'ssd', }, diff --git a/handwritten/bigtable/system-test/common.ts b/handwritten/bigtable/system-test/common.ts index 1736366eacfe..64c0e0e753f2 100644 --- a/handwritten/bigtable/system-test/common.ts +++ b/handwritten/bigtable/system-test/common.ts @@ -47,7 +47,12 @@ export async function reapInstances( try { const [instances] = await bigtable.getInstances(); const testInstances = instances - .filter(i => i.id.match(PREFIX)) + .filter( + i => + i.id.match(PREFIX) || + i.id.startsWith('instance-for-views') || + i.id.startsWith('instance-'), + ) .filter(i => { const timeCreated = i.metadata!.labels!.time_created as {} as Date; // Only delete stale resources. diff --git a/handwritten/bigtable/system-test/read-rows.ts b/handwritten/bigtable/system-test/read-rows.ts index 8d26fe77ce22..73c3309e3c9e 100644 --- a/handwritten/bigtable/system-test/read-rows.ts +++ b/handwritten/bigtable/system-test/read-rows.ts @@ -114,39 +114,44 @@ describe('Bigtable/Table', () => { it('should fail when invoking readRows with closed client', async () => { const instance = bigtable.instance(INSTANCE_NAME); const table = instance.table('fake-table'); - const [, operation] = await instance.create({ - clusters: { - id: 'fake-cluster3', - location: 'us-west1-c', - nodes: 1, - }, - }); - await operation.promise(); - const gaxOptions: CallOptions = { - retry: { - retryCodes: [grpc.status.DEADLINE_EXCEEDED, grpc.status.NOT_FOUND], - }, - maxRetries: 10, - }; - await table.create({ - gaxOptions, - }); - await table.getRows(); // This is done to initialize the data client - await bigtable.close(); try { - await table.getRows(); - assert.fail( - 'An error should have been thrown because the client is closed', - ); - } catch (err: any) { - assert.strictEqual(err.message, 'The client has already been closed.'); + const [, operation] = await instance.create({ + clusters: { + id: 'fake-cluster3', + location: 'us-west1-c', + nodes: 1, + }, + }); + await operation.promise(); + const gaxOptions: CallOptions = { + retry: { + retryCodes: [grpc.status.DEADLINE_EXCEEDED, grpc.status.NOT_FOUND], + }, + maxRetries: 10, + }; + await table.create({ + gaxOptions, + }); + await table.getRows(); // This is done to initialize the data client + await bigtable.close(); + try { + await table.getRows(); + assert.fail( + 'An error should have been thrown because the client is closed', + ); + } catch (err: any) { + assert.strictEqual(err.message, 'The client has already been closed.'); + } + } finally { + try { + const bigtableSecondClient = new Bigtable(); + const cleanInstance = bigtableSecondClient.instance(INSTANCE_NAME); + await cleanInstance.delete({}); + } catch (e) { + // ignore error if already deleted + } } }); - after(async () => { - const bigtableSecondClient = new Bigtable(); - const instance = bigtableSecondClient.instance(INSTANCE_NAME); - await instance.delete({}); - }); }); describe('createReadStream', () => { From 13e1574f87cff57a19c0e5c13fa23514dbeae34c Mon Sep 17 00:00:00 2001 From: feywind <57276408+feywind@users.noreply.github.com> Date: Wed, 29 Jul 2026 15:13:40 -0400 Subject: [PATCH 3/6] tests: more resource leak cleanup --- .../client-side-metrics-all-methods.ts | 3 +- .../client-side-metrics-setup-table.ts | 41 +++++++++++-------- .../system-test/client-side-metrics.ts | 3 +- 3 files changed, 27 insertions(+), 20 deletions(-) diff --git a/handwritten/bigtable/system-test/client-side-metrics-all-methods.ts b/handwritten/bigtable/system-test/client-side-metrics-all-methods.ts index 3747418ad011..19f2b5998a45 100644 --- a/handwritten/bigtable/system-test/client-side-metrics-all-methods.ts +++ b/handwritten/bigtable/system-test/client-side-metrics-all-methods.ts @@ -358,6 +358,7 @@ describe('Bigtable/ClientSideMetricsAllMethods', () => { before(async () => { await reapInstances(new Bigtable()); + await reapInstances(new Bigtable({projectId: SECOND_PROJECT_ID})); /* For both the default project and the secondary project we need to create instances with some data in them so that the tests can collect all the @@ -413,8 +414,8 @@ describe('Bigtable/ClientSideMetricsAllMethods', () => { } catch (e) { console.warn('The instance has been deleted already'); } + await reapInstances(bigtable); } - await reapInstances(new Bigtable()); }); describe('Bigtable/ClientSideMetricsToGCM', () => { diff --git a/handwritten/bigtable/system-test/client-side-metrics-setup-table.ts b/handwritten/bigtable/system-test/client-side-metrics-setup-table.ts index 4bac03e49a97..53d9ae1a9f23 100644 --- a/handwritten/bigtable/system-test/client-side-metrics-setup-table.ts +++ b/handwritten/bigtable/system-test/client-side-metrics-setup-table.ts @@ -20,24 +20,29 @@ export async function setupBigtable( tableIds: string[], ) { const instance = bigtable.instance(instanceId); - const [instanceInfo] = await instance.exists(); - while (!instanceInfo) { - const [, operation] = await instance.create({ - clusters: { - id: 'fake-cluster3', - location: 'us-west1-c', - nodes: 1, - }, - }); - await operation.promise(); - /** - * For whatever reason, even after waiting for an operation.promise() - * call to complete, the instance still doesn't seem to be ready yet so - * we do another check to ensure the instance is ready. - */ - const [instanceInfoAgain] = await instance.exists(); - if (instanceInfoAgain) { - break; + let [instanceExists] = await instance.exists(); + if (!instanceExists) { + try { + const [, operation] = await instance.create({ + clusters: { + id: 'fake-cluster3', + location: 'us-west1-c', + nodes: 1, + }, + labels: { + time_created: Date.now(), + }, + }); + await operation.promise(); + } catch (e) { + // Instance creation might already be in progress or completed + } + for (let attempt = 0; attempt < 5; attempt++) { + [instanceExists] = await instance.exists(); + if (instanceExists) { + break; + } + await new Promise(resolve => setTimeout(resolve, 2000)); } } const tables = tableIds.map(tableId => instance.table(tableId)); diff --git a/handwritten/bigtable/system-test/client-side-metrics.ts b/handwritten/bigtable/system-test/client-side-metrics.ts index 1be4d894db4f..bb3a2b06fc47 100644 --- a/handwritten/bigtable/system-test/client-side-metrics.ts +++ b/handwritten/bigtable/system-test/client-side-metrics.ts @@ -335,6 +335,7 @@ describe('Bigtable/ClientSideMetrics', () => { before(async () => { await reapInstances(new Bigtable()); + await reapInstances(new Bigtable({projectId: SECOND_PROJECT_ID})); /* For both the default project and the secondary project we need to create instances with some data in them so that the tests can collect all the @@ -390,8 +391,8 @@ describe('Bigtable/ClientSideMetrics', () => { } catch (e) { console.warn('The instance has been deleted already'); } + await reapInstances(bigtable); } - await reapInstances(new Bigtable()); }); describe('Bigtable/ClientSideMetricsToGCM', () => { From 028fc61fef10d6c9736b57179b48fefd39ceeb86 Mon Sep 17 00:00:00 2001 From: feywind <57276408+feywind@users.noreply.github.com> Date: Thu, 30 Jul 2026 16:50:15 -0400 Subject: [PATCH 4/6] chore: update for Gemini CRs --- .../bigtable/system-test/app-profile.ts | 2 +- handwritten/bigtable/system-test/bigtable.ts | 8 +- .../client-side-metrics-all-methods.ts | 88 +++++++++++-------- .../client-side-metrics-setup-table.ts | 2 +- .../system-test/client-side-metrics.ts | 88 +++++++++++-------- handwritten/bigtable/system-test/cluster.ts | 2 +- handwritten/bigtable/system-test/common.ts | 12 ++- .../read-modify-write-row-interceptors.ts | 2 +- handwritten/bigtable/system-test/read-rows.ts | 4 +- handwritten/bigtable/system-test/tpc-tests.ts | 5 +- 10 files changed, 128 insertions(+), 85 deletions(-) diff --git a/handwritten/bigtable/system-test/app-profile.ts b/handwritten/bigtable/system-test/app-profile.ts index 4b8477e8fbaf..15cf9cfc5b34 100644 --- a/handwritten/bigtable/system-test/app-profile.ts +++ b/handwritten/bigtable/system-test/app-profile.ts @@ -61,7 +61,7 @@ describe('📦 App Profile', () => { }; }), labels: { - time_created: Date.now(), + time_created: String(Date.now()), }, }); await operation.promise(); diff --git a/handwritten/bigtable/system-test/bigtable.ts b/handwritten/bigtable/system-test/bigtable.ts index d92d599598be..0a2c6898d9db 100644 --- a/handwritten/bigtable/system-test/bigtable.ts +++ b/handwritten/bigtable/system-test/bigtable.ts @@ -68,7 +68,7 @@ describe('Bigtable', () => { }, ], labels: { - time_created: Date.now(), + time_created: String(Date.now()), }, }); const [, operationHDD] = await INSTANCE_HDD.create({ @@ -81,7 +81,7 @@ describe('Bigtable', () => { }, ], labels: { - time_created: Date.now(), + time_created: String(Date.now()), }, }); @@ -176,7 +176,7 @@ describe('Bigtable', () => { }, ], labels: { - time_created: Date.now(), + time_created: String(Date.now()), }, }); await operation.promise(); @@ -230,7 +230,7 @@ describe('Bigtable', () => { }, ], labels: { - time_created: Date.now(), + time_created: String(Date.now()), }, }); await operation.promise(); diff --git a/handwritten/bigtable/system-test/client-side-metrics-all-methods.ts b/handwritten/bigtable/system-test/client-side-metrics-all-methods.ts index 19f2b5998a45..1794b99e30a6 100644 --- a/handwritten/bigtable/system-test/client-side-metrics-all-methods.ts +++ b/handwritten/bigtable/system-test/client-side-metrics-all-methods.ts @@ -357,8 +357,14 @@ describe('Bigtable/ClientSideMetricsAllMethods', () => { let defaultProjectId: string; before(async () => { - await reapInstances(new Bigtable()); - await reapInstances(new Bigtable({projectId: SECOND_PROJECT_ID})); + const reapClient1 = new Bigtable(); + const reapClient2 = new Bigtable({projectId: SECOND_PROJECT_ID}); + try { + await reapInstances(reapClient1); + await reapInstances(reapClient2); + } finally { + await Promise.all([reapClient1.close(), reapClient2.close()]); + } /* For both the default project and the secondary project we need to create instances with some data in them so that the tests can collect all the @@ -371,50 +377,60 @@ describe('Bigtable/ClientSideMetricsAllMethods', () => { metrics actually get written for that other project instead of the default project. */ - for (const bigtable of [ + const setupClients = [ new Bigtable(), new Bigtable({projectId: SECOND_PROJECT_ID}), - ]) { - for (const instanceId of [instanceId1, instanceId2]) { - await setupBigtableWithInsert(bigtable, columnFamilyId, instanceId, [ - tableId1, - tableId2, - ]); - } - defaultProjectId = await new Promise((resolve, reject) => { - bigtable.getProjectId_((err: Error | null, projectId?: string) => { - if (err) { - reject(err); - } else { - resolve(projectId as string); - } + ]; + try { + for (const bigtable of setupClients) { + for (const instanceId of [instanceId1, instanceId2]) { + await setupBigtableWithInsert(bigtable, columnFamilyId, instanceId, [ + tableId1, + tableId2, + ]); + } + defaultProjectId = await new Promise((resolve, reject) => { + bigtable.getProjectId_((err: Error | null, projectId?: string) => { + if (err) { + reject(err); + } else { + resolve(projectId as string); + } + }); }); - }); + } + } finally { + await Promise.all(setupClients.map(c => c.close())); } }); after(async () => { - for (const bigtable of [ + const cleanupClients = [ new Bigtable(), new Bigtable({projectId: SECOND_PROJECT_ID}), - ]) { - try { - // If the instance has been deleted already by another source, we don't - // want this after hook to block the continuous integration pipeline. - const instance = bigtable.instance(instanceId1); - await instance.delete({}); - } catch (e) { - console.warn('The instance has been deleted already'); - } - try { - // If the instance has been deleted already by another source, we don't - // want this after hook to block the continuous integration pipeline. - const instance = bigtable.instance(instanceId2); - await instance.delete({}); - } catch (e) { - console.warn('The instance has been deleted already'); + ]; + try { + for (const bigtable of cleanupClients) { + try { + // If the instance has been deleted already by another source, we don't + // want this after hook to block the continuous integration pipeline. + const instance = bigtable.instance(instanceId1); + await instance.delete({}); + } catch (e) { + console.warn('The instance has been deleted already'); + } + try { + // If the instance has been deleted already by another source, we don't + // want this after hook to block the continuous integration pipeline. + const instance = bigtable.instance(instanceId2); + await instance.delete({}); + } catch (e) { + console.warn('The instance has been deleted already'); + } + await reapInstances(bigtable); } - await reapInstances(bigtable); + } finally { + await Promise.all(cleanupClients.map(c => c.close())); } }); diff --git a/handwritten/bigtable/system-test/client-side-metrics-setup-table.ts b/handwritten/bigtable/system-test/client-side-metrics-setup-table.ts index 53d9ae1a9f23..7ac6a0cf7c3d 100644 --- a/handwritten/bigtable/system-test/client-side-metrics-setup-table.ts +++ b/handwritten/bigtable/system-test/client-side-metrics-setup-table.ts @@ -30,7 +30,7 @@ export async function setupBigtable( nodes: 1, }, labels: { - time_created: Date.now(), + time_created: String(Date.now()), }, }); await operation.promise(); diff --git a/handwritten/bigtable/system-test/client-side-metrics.ts b/handwritten/bigtable/system-test/client-side-metrics.ts index bb3a2b06fc47..a51ef9d5a237 100644 --- a/handwritten/bigtable/system-test/client-side-metrics.ts +++ b/handwritten/bigtable/system-test/client-side-metrics.ts @@ -334,8 +334,14 @@ describe('Bigtable/ClientSideMetrics', () => { let defaultProjectId: string; before(async () => { - await reapInstances(new Bigtable()); - await reapInstances(new Bigtable({projectId: SECOND_PROJECT_ID})); + const reapClient1 = new Bigtable(); + const reapClient2 = new Bigtable({projectId: SECOND_PROJECT_ID}); + try { + await reapInstances(reapClient1); + await reapInstances(reapClient2); + } finally { + await Promise.all([reapClient1.close(), reapClient2.close()]); + } /* For both the default project and the secondary project we need to create instances with some data in them so that the tests can collect all the @@ -348,50 +354,60 @@ describe('Bigtable/ClientSideMetrics', () => { metrics actually get written for that other project instead of the default project. */ - for (const bigtable of [ + const setupClients = [ new Bigtable(), new Bigtable({projectId: SECOND_PROJECT_ID}), - ]) { - for (const instanceId of [instanceId1, instanceId2]) { - await setupBigtableWithInsert(bigtable, columnFamilyId, instanceId, [ - tableId1, - tableId2, - ]); - } - defaultProjectId = await new Promise((resolve, reject) => { - bigtable.getProjectId_((err: Error | null, projectId?: string) => { - if (err) { - reject(err); - } else { - resolve(projectId as string); - } + ]; + try { + for (const bigtable of setupClients) { + for (const instanceId of [instanceId1, instanceId2]) { + await setupBigtableWithInsert(bigtable, columnFamilyId, instanceId, [ + tableId1, + tableId2, + ]); + } + defaultProjectId = await new Promise((resolve, reject) => { + bigtable.getProjectId_((err: Error | null, projectId?: string) => { + if (err) { + reject(err); + } else { + resolve(projectId as string); + } + }); }); - }); + } + } finally { + await Promise.all(setupClients.map(c => c.close())); } }); after(async () => { - for (const bigtable of [ + const cleanupClients = [ new Bigtable(), new Bigtable({projectId: SECOND_PROJECT_ID}), - ]) { - try { - // If the instance has been deleted already by another source, we don't - // want this after hook to block the continuous integration pipeline. - const instance = bigtable.instance(instanceId1); - await instance.delete({}); - } catch (e) { - console.warn('The instance has been deleted already'); - } - try { - // If the instance has been deleted already by another source, we don't - // want this after hook to block the continuous integration pipeline. - const instance = bigtable.instance(instanceId2); - await instance.delete({}); - } catch (e) { - console.warn('The instance has been deleted already'); + ]; + try { + for (const bigtable of cleanupClients) { + try { + // If the instance has been deleted already by another source, we don't + // want this after hook to block the continuous integration pipeline. + const instance = bigtable.instance(instanceId1); + await instance.delete({}); + } catch (e) { + console.warn('The instance has been deleted already'); + } + try { + // If the instance has been deleted already by another source, we don't + // want this after hook to block the continuous integration pipeline. + const instance = bigtable.instance(instanceId2); + await instance.delete({}); + } catch (e) { + console.warn('The instance has been deleted already'); + } + await reapInstances(bigtable); } - await reapInstances(bigtable); + } finally { + await Promise.all(cleanupClients.map(c => c.close())); } }); diff --git a/handwritten/bigtable/system-test/cluster.ts b/handwritten/bigtable/system-test/cluster.ts index 03f7c2f4a648..634accc27174 100644 --- a/handwritten/bigtable/system-test/cluster.ts +++ b/handwritten/bigtable/system-test/cluster.ts @@ -71,7 +71,7 @@ describe('Cluster', () => { const [, operation] = await instance.create({ clusters, labels: { - time_created: Date.now(), + time_created: String(Date.now()), }, }); await operation.promise(); diff --git a/handwritten/bigtable/system-test/common.ts b/handwritten/bigtable/system-test/common.ts index 64c0e0e753f2..6ff6568f76b0 100644 --- a/handwritten/bigtable/system-test/common.ts +++ b/handwritten/bigtable/system-test/common.ts @@ -54,10 +54,16 @@ export async function reapInstances( i.id.startsWith('instance-'), ) .filter(i => { - const timeCreated = i.metadata!.labels!.time_created as {} as Date; - // Only delete stale resources. + const timeCreatedRaw = i.metadata?.labels?.time_created; + if (!timeCreatedRaw) { + return true; + } + const timeCreatedNum = Number(timeCreatedRaw); + const timeCreated = new Date( + isNaN(timeCreatedNum) ? timeCreatedRaw : timeCreatedNum, + ); const staleThreshold = new Date(Date.now() - maxAgeMs); - return !timeCreated || timeCreated <= staleThreshold; + return timeCreated <= staleThreshold; }); // need to delete backups first due to instance deletion precondition diff --git a/handwritten/bigtable/system-test/read-modify-write-row-interceptors.ts b/handwritten/bigtable/system-test/read-modify-write-row-interceptors.ts index fe3e0d7234f5..015b14e4346a 100644 --- a/handwritten/bigtable/system-test/read-modify-write-row-interceptors.ts +++ b/handwritten/bigtable/system-test/read-modify-write-row-interceptors.ts @@ -72,7 +72,7 @@ async function createInstance( }, ], labels: { - time_created: Date.now(), + time_created: String(Date.now()), }, }); await operation.promise(); diff --git a/handwritten/bigtable/system-test/read-rows.ts b/handwritten/bigtable/system-test/read-rows.ts index 73c3309e3c9e..3b9840755de4 100644 --- a/handwritten/bigtable/system-test/read-rows.ts +++ b/handwritten/bigtable/system-test/read-rows.ts @@ -143,12 +143,14 @@ describe('Bigtable/Table', () => { assert.strictEqual(err.message, 'The client has already been closed.'); } } finally { + const bigtableSecondClient = new Bigtable(); try { - const bigtableSecondClient = new Bigtable(); const cleanInstance = bigtableSecondClient.instance(INSTANCE_NAME); await cleanInstance.delete({}); } catch (e) { // ignore error if already deleted + } finally { + await bigtableSecondClient.close(); } } }); diff --git a/handwritten/bigtable/system-test/tpc-tests.ts b/handwritten/bigtable/system-test/tpc-tests.ts index c7154ee040ec..4b3e63af44a3 100644 --- a/handwritten/bigtable/system-test/tpc-tests.ts +++ b/handwritten/bigtable/system-test/tpc-tests.ts @@ -76,11 +76,14 @@ describe.skip('Universe domain tests', () => { const columnFamilyId = 'cf1'; after(async () => { + const cleanClient = new Bigtable(); try { - const instance = new Bigtable().instance(instanceId); + const instance = cleanClient.instance(instanceId); await instance.delete({}); } catch (e) { console.log(`Error deleting instance ${instanceId}: ${e}`); + } finally { + await cleanClient.close(); } }); From dc9a54febb59ea517dde16aa45b385cdcdf7be3e Mon Sep 17 00:00:00 2001 From: feywind <57276408+feywind@users.noreply.github.com> Date: Thu, 30 Jul 2026 17:03:01 -0400 Subject: [PATCH 5/6] tests: shorten instance name to fix a failure (max 36 chars) --- .../bigtable/system-test/read-modify-write-row-interceptors.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/handwritten/bigtable/system-test/read-modify-write-row-interceptors.ts b/handwritten/bigtable/system-test/read-modify-write-row-interceptors.ts index 015b14e4346a..7f3e93e541f7 100644 --- a/handwritten/bigtable/system-test/read-modify-write-row-interceptors.ts +++ b/handwritten/bigtable/system-test/read-modify-write-row-interceptors.ts @@ -31,7 +31,7 @@ import {status as GrpcStatus} from '@grpc/grpc-js'; import {createMetricsUnaryInterceptorProvider} from '../src/client-side-metrics/metric-interceptor'; import {generateId, reapInstances} from './common'; -const INSTANCE_ID = generateId('isolated-rmw-inst'); +const INSTANCE_ID = generateId('rmw-inst'); const TABLE_ID = 'isolated-rmw-table'; const ZONE = 'us-central2-a'; const CLUSTER = 'fake-cluster'; From 872261a82f783c94a79f8e852240270613c388d2 Mon Sep 17 00:00:00 2001 From: feywind <57276408+feywind@users.noreply.github.com> Date: Thu, 30 Jul 2026 17:03:16 -0400 Subject: [PATCH 6/6] tests: add client-side metrics retries --- .../client-side-metrics-all-methods.ts | 51 ++++++++++++------- .../client-side-metrics-setup-table.ts | 5 ++ .../system-test/client-side-metrics.ts | 40 +++++++++------ 3 files changed, 61 insertions(+), 35 deletions(-) diff --git a/handwritten/bigtable/system-test/client-side-metrics-all-methods.ts b/handwritten/bigtable/system-test/client-side-metrics-all-methods.ts index 1794b99e30a6..d0d8d9ebe49b 100644 --- a/handwritten/bigtable/system-test/client-side-metrics-all-methods.ts +++ b/handwritten/bigtable/system-test/client-side-metrics-all-methods.ts @@ -324,7 +324,7 @@ const rules = [ * Cloud Monitoring. */ async function checkForPublishedMetrics(projectId: string) { - const monitoringClient = new MetricServiceClient(); // Correct instantiation + const monitoringClient = new MetricServiceClient({projectId}); // Correct instantiation const now = Math.floor(Date.now() / 1000); const filters = [ 'metric.type="bigtable.googleapis.com/client/attempt_latencies"', @@ -334,22 +334,32 @@ async function checkForPublishedMetrics(projectId: string) { 'metric.type="bigtable.googleapis.com/client/first_response_latencies"', ]; for (let i = 0; i < filters.length; i++) { + // A race condition with the metrics submission can cause this to check too + // early. Check up to 5 times with 2 seconds between checks. const filter = filters[i]; - const [series] = await monitoringClient.listTimeSeries({ - name: `projects/${projectId}`, - interval: { - endTime: { - seconds: now, - nanos: 0, + let seriesLength = 0; + for (let attempt = 0; attempt < 5; attempt++) { + const [series] = await monitoringClient.listTimeSeries({ + name: `projects/${projectId}`, + interval: { + endTime: { + seconds: now, + nanos: 0, + }, + startTime: { + seconds: now - 1000 * 60 * 60 * 24, + nanos: 0, + }, }, - startTime: { - seconds: now - 1000 * 60 * 60 * 24, - nanos: 0, - }, - }, - filter, - }); - assert(series.length > 0); + filter, + }); + if (series.length > 0) { + seriesLength = series.length; + break; + } + await new Promise(r => setTimeout(r, 2000)); + } + assert(seriesLength > 0); } } @@ -482,12 +492,15 @@ describe('Bigtable/ClientSideMetricsAllMethods', () => { // result from calling export was successful. assert.strictEqual(result.code, 0); resultCallback({code: 0}); - void checkForPublishedMetrics(projectId).then(() => { - done(); - }); + void checkForPublishedMetrics(projectId) + .then(() => { + done(); + }) + .catch(err => { + done(err); + }); } catch (error) { // The code here isn't 0 so we report the original error to the mocha test runner. - done(result); done(error); } } else { diff --git a/handwritten/bigtable/system-test/client-side-metrics-setup-table.ts b/handwritten/bigtable/system-test/client-side-metrics-setup-table.ts index 7ac6a0cf7c3d..ec0b42c6b294 100644 --- a/handwritten/bigtable/system-test/client-side-metrics-setup-table.ts +++ b/handwritten/bigtable/system-test/client-side-metrics-setup-table.ts @@ -44,6 +44,11 @@ export async function setupBigtable( } await new Promise(resolve => setTimeout(resolve, 2000)); } + if (!instanceExists) { + throw new Error( + `Failed to setup Bigtable: Instance ${instanceId} does not exist or creation failed.`, + ); + } } const tables = tableIds.map(tableId => instance.table(tableId)); for (const currentTable of tables) { diff --git a/handwritten/bigtable/system-test/client-side-metrics.ts b/handwritten/bigtable/system-test/client-side-metrics.ts index a51ef9d5a237..7a029b37378e 100644 --- a/handwritten/bigtable/system-test/client-side-metrics.ts +++ b/handwritten/bigtable/system-test/client-side-metrics.ts @@ -312,21 +312,31 @@ async function checkForPublishedMetrics(projectId: string) { ]; for (let i = 0; i < filters.length; i++) { const filter = filters[i]; - const [series] = await monitoringClient.listTimeSeries({ - name: `projects/${projectId}`, - interval: { - endTime: { - seconds: now, - nanos: 0, - }, - startTime: { - seconds: now - 1000 * 60 * 60 * 24, - nanos: 0, + // A race condition with the metrics submission can cause this to check too + // early. Check up to 5 times with 2 seconds between checks. + let seriesLength = 0; + for (let attempt = 0; attempt < 5; attempt++) { + const [series] = await monitoringClient.listTimeSeries({ + name: `projects/${projectId}`, + interval: { + endTime: { + seconds: now, + nanos: 0, + }, + startTime: { + seconds: now - 1000 * 60 * 60 * 24, + nanos: 0, + }, }, - }, - filter, - }); - assert(series.length > 0); + filter, + }); + if (series.length > 0) { + seriesLength = series.length; + break; + } + await new Promise(r => setTimeout(r, 2000)); + } + assert(seriesLength > 0); } } @@ -464,12 +474,10 @@ describe('Bigtable/ClientSideMetrics', () => { done(); }) .catch(err => { - done(new Error('Metrics have not been published')); done(err); }); } catch (error) { // The code here isn't 0 so we report the original error to the mocha test runner. - done(result); done(error); } } else {