test(bigtable): fix flaky backup system tests by danieljbruce · Pull Request #9717 · googleapis/google-cloud-node

danieljbruce · 2026-10-23T18:45:26Z

Description

This change fixes flaky backup system tests in the Bigtable package.

Impact

It prevents table collisions and backup metadata write quota failures during CI.

Changes

Replace hardcoded 'old-table' and 'new-table' identifiers in should restore a copied backup with generateId('table'), wait for table replication before creating the source backup, and delete both created tables and backups in finally so Mocha test retries (--retries 3) do not fail with 6 ALREADY_EXISTS: Table already exists.
Delete the temporary secondary cluster (destinationClusterId) in should create backup of a table and copy it on another cluster of the same instance (and move secondary instance deletion into finally blocks) so extra clusters do not leak into subsequent tests on the shared instance.
Add a 60-second cooldown delay at the start of the before hook in backups with gapic (since Mocha runs before hooks prior to beforeEach hooks and does not retry before hooks) and retry backup metadata write operations on 8 RESOURCE_EXHAUSTED quota errors.

Testing

Compiled handwritten/bigtable and verified existing unit tests and strict linter checks pass.
Verified backups and backups with gapic system test setup and teardown paths in handwritten/bigtable/system-test/bigtable.ts.

Alternatives

Not merging this change leaves should restore a copied backup and backups with gapic vulnerable to ALREADY_EXISTS table collisions on Mocha retries and RESOURCE_EXHAUSTED backup metadata write quota failures in CI.
Relying only on beforeEach delays without a cooldown in backups with gapic's before hook or unique table IDs does not protect the before hook or retry attempts.

gemini-code-assist

Code Review

This pull request introduces a withBackupQuotaRetry helper function to handle RESOURCE_EXHAUSTED quota errors during Bigtable backup operations in system tests, wrapping various backup creation, copy, and deletion calls. It also refactors test setup and teardown to improve resource cleanup and dynamic ID generation. The review feedback suggests removing an unconditional 60-second sleep in a before hook to speed up successful test runs, utilizing non-nullable local constants (createdBackup and copiedBackup) to avoid non-null assertions and potential TypeScript strict null check issues, and using an inline structural type assertion for error code checks to prevent potential import issues.

gemini-code-assist · 2026-10-23T18:47:42Z

+      // Sleep here for just over a minute before the "before all" hook creates
+      // backups so that requests from the preceding backup tests do not trigger
+      // per-minute 'Backup metadata write requests' quota exhaustion.
+      await new Promise(r => setTimeout(r, 60001));
+      const [backupOpSSD] = await withBackupQuotaRetry(() =>


Unconditional 60-second sleep in the before hook adds a significant delay to the test suite on every run, even when there is no quota exhaustion. Since both createBackup calls inside this hook are already wrapped in withBackupQuotaRetry, any quota exhaustion will be handled dynamically and retried with a delay only when necessary. Removing this unconditional sleep will speed up successful test runs by a full minute.

const [backupOpSSD] = await withBackupQuotaRetry(() =>

gemini-code-assist · 2026-10-23T18:47:42Z

+          const [copiedBackup, copyOperation] = await withBackupQuotaRetry(() =>
+            backup!.copy(config),
+          );


Using createdBackup directly instead of backup! is safer and avoids the need for a non-null assertion operator (!), as createdBackup is a non-nullable constant in this scope.

Suggested change

const [copiedBackup, copyOperation] = await withBackupQuotaRetry(() =>

backup!.copy(config),

);

const [copiedBackup, copyOperation] = await withBackupQuotaRetry(() =>

createdBackup.copy(config),

);

gemini-code-assist · 2026-10-23T18:47:42Z

          // Restore a table from the copied backup
-          const [newTable, restoreOperation] =
-            await newBackup.restore('new-table');
+          const [restoredTable, restoreOperation] =
+            await newBackup.restore(restoredTableId);


Using copiedBackup directly instead of newBackup avoids potential TypeScript compilation errors under strictNullChecks, as newBackup is declared as Backup | undefined and is not asserted or narrowed here.

Suggested change

// Restore a table from the copied backup

const [newTable, restoreOperation] =

await newBackup.restore('new-table');

const [restoredTable, restoreOperation] =

await newBackup.restore(restoredTableId);

// Restore a table from the copied backup

const [restoredTable, restoreOperation] =

await copiedBackup.restore(restoredTableId);

gemini-code-assist · 2026-10-23T18:47:42Z

+      const isQuotaError =
+        (err as ServiceError)?.code === 8 ||
+        (err as Error)?.message?.includes('RESOURCE_EXHAUSTED');


To avoid potential TypeScript compilation errors if ServiceError is not imported in this file, you can use an inline structural type assertion (err as {code?: number}) instead of (err as ServiceError).

Suggested change

const isQuotaError =

(err as ServiceError)?.code === 8 ||

(err as Error)?.message?.includes('RESOURCE_EXHAUSTED');

const isQuotaError =

(err as {code?: number})?.code === 8 ||

(err as Error)?.message?.includes('RESOURCE_EXHAUSTED');

… style guidelines

…ments (round 1)

…ments (round 2)

danieljbruce · 2026-10-23T19:19:34Z

/gemini review

gemini-code-assist

Code Review

This pull request enhances the reliability of Bigtable system tests by wrapping backup operations in a new withBackupQuotaRetry helper function, which retries operations upon encountering RESOURCE_EXHAUSTED quota errors. Additionally, it improves test isolation and resource cleanup by scoping variables locally and utilizing try...finally blocks to delete created backups, tables, and instances. The review feedback recommends replacing the ServiceError type cast with a structural type cast to prevent potential TypeScript compilation errors if ServiceError is not imported.

gemini-code-assist · 2026-10-23T19:22:25Z

+      const isQuotaError =
+        (err as ServiceError)?.code === 8 ||
+        (err as Error)?.message?.includes('RESOURCE_EXHAUSTED');


Using ServiceError without an explicit import from @grpc/grpc-js or google-gax can lead to TypeScript compilation errors if the type is not already in scope. To ensure robust compilation and avoid unnecessary imports in the test file, consider using a structural type cast like (err as {code?: number}) or (err as any).

Suggested change

const isQuotaError =

(err as ServiceError)?.code === 8 ||

(err as Error)?.message?.includes('RESOURCE_EXHAUSTED');

const isQuotaError =

(err as {code?: number})?.code === 8 ||

(err as Error)?.message?.includes('RESOURCE_EXHAUSTED');

…n backup system test

danieljbruce · 2026-10-26T18:14:34Z

[Generated] Fixed the CI issues on this PR in commit c05842418b:

bigtable-system-tests (long-door-651) (Bigtable > backups > should restore a backup to a different instance):
- Cause: All 272 other system tests passed, including all new backup copy and GAPIC backup tests, and only should restore a backup to a different instance failed at assert.strictEqual(tExists, true) (false !== true). Backup#restoreTo (handwritten/bigtable/src/backup.ts) restores the table into config.instance (DIFF_INSTANCE), but constructs the returned Table object via this.cluster.instance.table(config.tableId) (bound to the source INSTANCE). Previously, restoreTableIdFromCluster was shared at the describe('backups') scope with the preceding test (should restore a backup (cluster)), which had just created a table with that same ID in INSTANCE, so table.exists() passed by accident. Once should restore a backup (cluster) and should restore a backup to a different instance generated distinct per-test restoreTableIdFromCluster IDs, table.exists() checked INSTANCE instead of DIFF_INSTANCE and returned false.
- Fix: Updated should restore a backup to a different instance in handwritten/bigtable/system-test/bigtable.ts to check await DIFF_INSTANCE.table(restoreTableIdFromCluster).exists(), and updated withBackupQuotaRetry to use (err as {code?: number})?.code === 8 per review feedback.
presubmit-bun (Bun (1.4.2)):
- Cause: Bun (1.4.2, shard 0) passed (conclusion: "success"), but the Bun (1.4.2) shard-status job queried github.rest.actions.listJobsForWorkflowRun ~3 seconds after shard 0 completed, before shard 0's conclusion: "success" was reflected in the GitHub API response. Pushing c05842418b re-triggers presubmit-bun alongside bigtable-system-tests.

…-side metrics export interval

danieljbruce · 2026-10-26T20:37:31Z

[Generated] Addressed the remaining CI failures on this PR (04d269228a):

presubmit-bun (Bun (1.4.2, shard 0)) unit test crash:
- Cause: handwritten/bigtable/src/utils/mutateInternal.ts imported PartialFailureError (a runtime value) from ../table instead of ../tabular-api-surface (where PartialFailureError is defined). When Mocha loaded build/test/table.js before src/tabular-api-surface.js was cached (test/table.js -> src/row.js -> src/utils/getRowsInternal.js -> src/utils/createReadStreamInternal.js -> src/tabular-api-surface.js -> src/utils/mutateInternal.js -> src/table.js), src/table.js evaluated class Table extends TabularApiSurface before src/tabular-api-surface.js finished exporting TabularApiSurface, throwing TypeError: The superclass is not a constructor (under Bun) / TypeError: Class extends value undefined is not a constructor or null (under Node.js).
- Fix: Imported PartialFailureError and MutateCallback directly from ../tabular-api-surface in src/utils/mutateInternal.ts to break the circular runtime require("../table").
bigtable-system-tests (long-door-651) (Bigtable/ClientSideMetricsAllMethods) failures:
- Cause:
  - All backup system tests in system-test/bigtable.ts passed, but Bigtable/ClientSideMetricsAllMethods failed in Bigtable/ClientSideMetricsToGCMTimeout with INVALID_ARGUMENT: The start time must be before the end time and DEADLINE_EXCEEDED / done() called multiple times.
  - In src/client-side-metrics/exporter.ts, when startTime >= endTime due to clock skew between Date.now() and hrTime(), getInterval() clamped endTime equal to startTime, which Cloud Monitoring rejects for non-gauge (CUMULATIVE) metrics (start_time must be strictly before end_time).
  - In system-test/client-side-metrics-all-methods.ts and system-test/client-side-metrics.ts, TestExporter instances in ClientSideMetricsToGCM and ClientSideMetricsToGCMTimeout continued calling super.export() on every 60s PeriodicExportingMetricReader tick even after their initial export and test completion, accumulating 190+ concurrent background exporters and invoking done() on already-finished Mocha tests.
- Fix:
  - Updated getInterval() in src/client-side-metrics/exporter.ts to advance endTime to startTime + 1ms (1_000_000 ns) whenever startTime >= endTime.
  - Guarded TestExporter.export() in system-test/client-side-metrics-all-methods.ts and system-test/client-side-metrics.ts so each TestExporter exports at most once and short-circuits subsequent periodic reader ticks with resultCallback({code: 0}).

danieljbruce · 2026-10-26T20:50:38Z

+    if (endNanos >= 1_000_000_000) {
+      endSec += 1;
+      endNanos -= 1_000_000_000;
+    }


Modifying the timestamps for our metrics does not seem like something we should do.

…andbox

test(bigtable): fix flaky backup system tests

08362cf

product-auto-label Bot added the api: bigtable Issues related to the Bigtable API. label Oct 23, 2026

gemini-code-assist Bot reviewed Oct 23, 2026

View reviewed changes

danieljbruce added 3 commits October 23, 2026 18:49

[Style Maintenance] align changes with CONTRIBUTING.md and gts coding…

d57cbd1

… style guidelines

[Independent review follow-ups] address independent Gemini review com…

c6ddbd2

…ments (round 1)

[Independent review follow-ups] address independent Gemini review com…

1cf29f8

…ments (round 2)

gemini-code-assist Bot reviewed Oct 23, 2026

View reviewed changes

[Address CI errors] check restored table existence on DIFF_INSTANCE i…

c058424

…n backup system test

[Address CI errors] fix circular require in mutateInternal and client…

04d2692

…-side metrics export interval

danieljbruce commented Oct 26, 2026

View reviewed changes

[Address CI errors] restore waitForReplication fake timer spies via s…

fd56037

…andbox

Navigation Menu

Repository navigation

test(bigtable): fix flaky backup system tests - #9717

test(bigtable): fix flaky backup system tests#9717
danieljbruce wants to merge 7 commits into
googleapis:mainfrom
danieljbruce:fix/bigtable-flaky-backup-system-tests

danieljbruce commented Oct 23, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot Oct 23, 2026

Uh oh!

gemini-code-assist Bot Oct 23, 2026

Uh oh!

gemini-code-assist Bot Oct 23, 2026

Uh oh!

gemini-code-assist Bot Oct 23, 2026

Uh oh!

danieljbruce commented Oct 23, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot Oct 23, 2026

Uh oh!

danieljbruce commented Oct 26, 2026

Uh oh!

danieljbruce commented Oct 26, 2026

Uh oh!

danieljbruce Oct 26, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

danieljbruce commented Oct 23, 2026

Description

Impact

Changes

Testing

Alternatives

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

danieljbruce commented Oct 23, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

danieljbruce commented Oct 26, 2026

Uh oh!

danieljbruce commented Oct 26, 2026

Uh oh!

danieljbruce Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant