[danieljbruce]

Description

Stabilizes flaky Bigtable tests and reverts exporter interval check from 1ea27c9.

Impact

Eliminates flaky Bigtable unit and system test failures without modifying production logic.

Testing

• Verified GIT_DIFF_ARG="upstream/main...HEAD -- :!packages" node ./bin/linter.mjs --strict passes with zero errors.
• Verified handwritten/bigtable unit tests pass across 5 runs on both Node.js and Bun (>= 95% confidence) as well as ci/run_conditional_tests.sh --strict (875 passing).

[danieljbruce]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a retry mechanism (withBackupQuotaRetry) and robust resource cleanup in system tests to handle backup quota limitations, alongside adjustments to metric interval validation in client-side metrics tests. Feedback on these changes highlights three key areas: first, reverting the interval check in the production exporter leaves it vulnerable to export failures, so the strictly-greater-than adjustment should be implemented there rather than only in tests; second, the retry helper should validate that the caught error is a truthy object to prevent potential TypeError crashes; and third, manually constructing the table instance in the restore test may be masking a production bug in backup.restore() where the returned table is not correctly associated with the target instance.

[gemini-code-assist]

Reverting the interval check entirely from the production exporter leaves production environments vulnerable to metric export failures if startTime >= endTime occurs (e.g., due to clock drift, container virtualization, or high-resolution timer limitations). Since Cloud Monitoring requires startTime < endTime for cumulative/delta metrics, the previous fix of setting them equal was indeed insufficient, but the correct solution is to apply the strictly-greater-than adjustment (adding 1ms/1,000,000 nanos) in the production exporter itself, rather than only in the system tests.

  let endSec = dataPoint.endTime[0];
  let endNanos = dataPoint.endTime[1];
  const startSec = dataPoint.startTime[0];
  const startNanos = dataPoint.startTime[1];

  if (startSec > endSec || (startSec === endSec && startNanos >= endNanos)) {
    const adjustedNanos = startNanos + 1000000;
    if (adjustedNanos >= 1000000000) {
      endSec = startSec + 1;
      endNanos = adjustedNanos - 1000000000;
    } else {
      endSec = startSec;
      endNanos = adjustedNanos;
    }
  }

  return {
    endTime: {
      seconds: endSec,
      nanos: endNanos,
    },
    startTime: {
      seconds: startSec,
      nanos: startNanos,
    },
  };

[danieljbruce]

I don't think I agree with this. https://github.com/googleapis/google-cloud-node/pull/9717/changes#r4401545583 offers this solution, but failed exporter calls are silent so it doesn't bombard the user with errors and we don't want hidden code somewhere artificially manipulating the data.

[gemini-code-assist]

If the caught error err is null or not an object, casting it directly and accessing serviceErr.code or serviceErr.message will throw a TypeError, crashing the test runner. To ensure robust error handling and prevent TypeErrors, validate that err is truthy and is an object upfront before performing property checks.

Suggested change

	const serviceErr = err as Partial<ServiceError>;
	const isQuotaError =
	serviceErr.code === 8 ||
	/RESOURCE_EXHAUSTED|Quota exceeded/i.test(serviceErr.message ?? '');
	if (!err || typeof err !== 'object') {
	return;
	}
	const serviceErr = err as Partial<ServiceError>;
	const isQuotaError =
	serviceErr.code === 8 ||
	/RESOURCE_EXHAUSTED|Quota exceeded/i.test(serviceErr.message ?? '');

References

1. Always validate and reject missing or falsy arguments upfront before performing property checks, to prevent TypeErrors (such as when 'typeof null' evaluates to 'object').

[gemini-code-assist]

If table.exists() was failing or checking the wrong instance, it suggests that backup.restore() might have a bug where the returned Table instance is associated with the source instance instead of the target DIFF_INSTANCE passed in the options. Working around this in the test by manually constructing the table via DIFF_INSTANCE.table(...) hides this potential production bug. Please verify if backup.restore() correctly associates the returned Table with the target instance when options.instance is provided, and if not, fix it in the production implementation of backup.restore().

[gemini-code-assist]

Code Review

This pull request simplifies metric interval retrieval, introduces a retry mechanism (withBackupQuotaRetry) to handle quota exhaustion errors during backup operations in system tests, and ensures proper cleanup of test resources. It also adds metric interval validation to client-side metrics tests and updates unit tests to use sandboxed timers. Review feedback suggests adding a .js extension to a new import for ESM compatibility, adding a safety guard to prevent potential TypeErrors when checking service errors, and removing a redundant 60-second delay in the test setup to reduce overall test execution time.

[gemini-code-assist]

For consistency with other imports in this file (such as ../src/mutation.js) and to ensure compatibility with ESM resolution in Node.js, please add the .js extension to the import path.

Suggested change

	import '../src/table';
	import '../src/table.js';

[gemini-code-assist]

To prevent potential runtime TypeErrors during error handling in tests, add a safety guard to ensure serviceErr is not null or undefined before accessing its properties.

        const serviceErr = err as Partial<ServiceError> | null | undefined;
        const isQuotaError =
          serviceErr &&
          (serviceErr.code === 8 ||
            /RESOURCE_EXHAUSTED|Quota exceeded/i.test(serviceErr.message ?? ''));

References

1. Always validate and reject missing or falsy arguments upfront before performing property checks, to prevent TypeErrors (such as when 'typeof null' evaluates to 'object').

[gemini-code-assist]

Since withBackupQuotaRetry is already implemented to dynamically handle quota errors with a 60-second cooldown, this unconditional 60-second delay in the before block (combined with the existing 60-second delay in beforeEach) is redundant. It unnecessarily adds 2-3 minutes of idle wait time to the test suite. Consider removing this unconditional delay and relying on the dynamic retry helper.

[danieljbruce]

Note: The changes here are actually just reverting changes that were applied in 1ea27c9#diff-39c2f3c691ce7c9e2f54ab8c9020dff4aef632d80a11aa13fe19074c889aaad5R51-R60. We do not want the client side metrics exporter artificially warping collected client side metrics data which is what this commit did.

If we can change the tests instead then that would offer a much better solution.

[danieljbruce]

And this PR attempts to change the tests instead to solve the Flakey test problem.

[danieljbruce]

Summary of the errors observed in bigtable-system-tests that this PR cleans up:

1. Backup metadata write quota exhaustion (8 RESOURCE_EXHAUSTED):

   • Observed error: Error: 8 RESOURCE_EXHAUSTED: Quota exceeded for quota metric 'Backup metadata write requests' and limit 'Backup metadata write requests per minute' of service 'bigtableadmin.googleapis.com' in Bigtable > backups, Bigtable > Copy Backup, and the "before all" hook in "backups with gapic" in handwritten/bigtable/system-test/bigtable.ts. Because Mocha does not retry before hooks even when --retries 3 is configured, hitting the per-minute quota in the backups with gapic before hook immediately after the Copy Backup suite caused the entire backups with gapic suite to fail.
   • Cleanup in this PR: Added withBackupQuotaRetry() to retry backup creation and copy calls after a 60-second cooldown when 8 RESOURCE_EXHAUSTED / Quota exceeded is returned, along with a 60-second cooldown at the start of the backups with gapic before hook.

2. Table/backup collisions (6 ALREADY_EXISTS) and leaked resources across Mocha retries (--retries 3):

   • Observed error: Error: 6 ALREADY_EXISTS: Table ... already exists (and backup ID collisions) when Mocha retried flaky tests in Bigtable > backups, Bigtable > Copy Backup (including should restore a copied backup, which used hardcoded 'old-table' and 'new-table' table names), and Bigtable > backups with gapic. In addition, secondary clusters (destinationClusterId), secondary instances, backups, and restored tables were not cleaned up in finally blocks, and should restore a copied backup created a backup immediately after table.insert(...) without waiting for replication (await table.waitForReplication()) across clusters.
   • Cleanup in this PR: Scoped generateId(...) table and backup IDs per test execution (it(...)) instead of sharing them across retries at the describe(...) level, waited for replication before backing up in should restore a copied backup, guarded DIFF_INSTANCE.create(...) with DIFF_INSTANCE.exists(), and wrapped created backups, restored tables, temporary clusters, and temporary instances in try ... finally cleanup blocks.

3. Restored table existence check on the wrong instance in should restore a backup to a different instance:

   • Observed error: AssertionError [ERR_ASSERTION]: Expected values to be strictly equal: false !== true at const [tExists] = await table.exists() in Bigtable > backups > should restore a backup to a different instance. Backup#restoreTo restores the table to DIFF_INSTANCE, but constructs the returned Table instance from this.cluster.instance (the source INSTANCE). Previously, table.exists() only passed by accident because restoreTableIdFromCluster was shared with the preceding should restore a backup (cluster) test, which had left a table with the same ID on INSTANCE.
   • Cleanup in this PR: Updated the test to check await DIFF_INSTANCE.table(restoreTableIdFromCluster).exists().

4. Client-side metrics export errors (3 INVALID_ARGUMENT: The start time must be before the end time, 4 DEADLINE_EXCEEDED, and done() called multiple times):

   • Observed errors: In handwritten/bigtable/system-test/client-side-metrics.ts and handwritten/bigtable/system-test/client-side-metrics-all-methods.ts (Bigtable/ClientSideMetricsToGCM and Bigtable/ClientSideMetricsToGCMTimeout):
     • 3 INVALID_ARGUMENT: One or more TimeSeries could not be written: ... The start time must be before the end time when OpenTelemetry recorded a cumulative data point with startTime >= endTime (due to clock skew between Date.now() and hrTime()). Commit 1ea27c9 had attempted to work around this in production src/client-side-metrics/exporter.ts by clamping endTime = startTime, which both artificially modified metric timestamps in production exporter code and still failed Cloud Monitoring's strict startTime < endTime requirement for cumulative metrics.
     • 4 DEADLINE_EXCEEDED and Mocha done() called multiple times errors because each test's TestExporter continued running super.export(...) on every subsequent 60-second PeriodicExportingMetricReader tick after the test had already completed (accumulating 190+ concurrent background exports across the test run), and the error catch block invoked both done(result) and done(error).
   • Cleanup in this PR: Reverted the 1ea27c9 timestamp clamping from production src/client-side-metrics/exporter.ts, normalized startTime >= endTime intervals only inside the system test TestExporter (ensureValidMetricIntervals), guarded TestExporter.export() so each test exporter exports at most once, and removed the duplicate done(error) call.