Skip to content

fix(gax): suppress cancel destroy error in StreamArrayParser and add Bun certification matrix - #9652

Draft
danieljbruce wants to merge 2 commits into
googleapis:mainfrom
danieljbruce:test/bun-full-matrix-certification
Draft

danieljbruce wants to merge 2 commits into
googleapis:mainfrom
danieljbruce:test/bun-full-matrix-certification

Conversation

@danieljbruce

Copy link
Copy Markdown
Contributor

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes b/574441098
Fixes b/574456004

Summary

This PR adds the automated Bun Full Matrix Sign-off & Certification Test Suite (core/packages/gax/test/unit/bunCertificationMatrix.ts) and fixes a stream teardown bug in StreamArrayParser uncovered during real-HTTP server-streaming cancellation testing (b/574456004):

  1. StreamArrayParser._destroy Cancellation Fix (core/packages/gax/src/streamArrayParser.ts):
    • When streamArrayParser.cancel() is invoked on an active HTTP/1.1 REST fallback server stream backed by a real node-fetch response body, cancelController.abort() causes node-fetch to destroy response.body with AbortError. stream.pipeline forwards that error to streamArrayParser.destroy(err), which previously emitted a spurious AbortError event on streamArrayParser despite cancelRequested being true.
    • Overriding _destroy(err, callback) to pass null when this.cancelRequested is true suppresses the spurious AbortError while preserving error propagation for non-cancelled stream failures and timeouts.
  2. Bun Full Matrix Certification Suite (core/packages/gax/test/unit/bunCertificationMatrix.ts):
    • Bun Packager Requirements: Verifies bun install reliability and warm-cache installation speed, lockfile (bun.lock) determinism across clean installs and --frozen-lockfile CI environments, postinstall/prepare lifecycle hook ordering and trustedDependencies, and monorepo workspaces (workspace:*) resolution.
    • Transport Parity (gRPC over HTTP/2 & HTTP/1.1 REST Fallback): Verifies request/response headers, trailing metadata and grpc-status-details-bin / ErrorInfo decoding, canonical status code preservation, deadlines (DEADLINE_EXCEEDED), and unary/streaming cancellations (CANCELLED).
    • Streaming Integrity & Backpressure: Verifies 1,000-message high-throughput streaming across arbitrary chunk boundaries with .pause()/.resume() flow control.
    • Resource & Concurrency Stability: Verifies high-concurrency LROperation polling and regular read workloads with bounded heap delta, zero leaked file descriptors/sockets, low p95 latency, and zero unhandledRejection events.
    • Target Environment Portability: Verifies SDK execution across developer workstations, minimal OCI container environments, Cloud Run (K_SERVICE / PORT), and Firebase (FIREBASE_CONFIG / GCLOUD_PROJECT).

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements a _destroy method in StreamArrayParser to handle cancellation requests and introduces a comprehensive Bun certification test suite. The reviewer feedback highlights several improvement opportunities in the new test file, including explicitly importing the protobufjs library to resolve missing type references, replacing a flaky hardcoded setTimeout with a robust listener on the stream's 'close' event, and wrapping heap-growth assertions in checks that verify globalThis.gc is available to prevent failures in environments where garbage collection is not exposed.

Comment on lines +26 to +27
import * as grpc from '@grpc/grpc-js';
import {PassThroughClient} from 'google-auth-library';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The test file references protobuf.Service and protobuf.Message but does not import the protobufjs library. To prevent compilation errors or reliance on ambient globals, please explicitly import protobufjs.

Suggested change
import * as grpc from '@grpc/grpc-js';
import {PassThroughClient} from 'google-auth-library';
import * as grpc from '@grpc/grpc-js';
import * as protobuf from 'protobufjs';
import {PassThroughClient} from 'google-auth-library';

});
parser.on('error', (err: Error) => streamErrors.push(err));

await new Promise<void>(resolve => setTimeout(resolve, 120));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using a hardcoded setTimeout of 120ms to wait for stream teardown can lead to flaky test failures in slow or resource-constrained CI environments. Since parser.cancel() internally calls this.destroy(), you can robustly wait for the stream's 'close' event instead.

Suggested change
await new Promise<void>(resolve => setTimeout(resolve, 120));
await new Promise<void>(resolve => parser.on('close', resolve));

Comment on lines +1284 to +1287
assert.ok(
heapDeltaMb < 40,
`Expected bounded LRO heap growth (<40MB), got ${heapDeltaMb.toFixed(2)}MB`,
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Checking heap growth without a guaranteed garbage collection run can lead to flaky test failures in CI environments where globalThis.gc is not exposed (i.e., when Node.js is not run with the --expose-gc flag). To make this test robust, please wrap the heap delta assertion in a check that ensures globalThis.gc is available.

        if (typeof globalThis.gc === 'function') {
          assert.ok(
            heapDeltaMb < 40,
            'Expected bounded LRO heap growth (<40MB), got ' + heapDeltaMb.toFixed(2) + 'MB',
          );
        }

Comment on lines +1432 to +1435
assert.ok(
heapDeltaMb < 40,
`Expected bounded read heap growth (<40MB), got ${heapDeltaMb.toFixed(2)}MB`,
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to the LRO polling test, checking heap growth without a guaranteed garbage collection run can lead to flaky test failures in CI environments where globalThis.gc is not exposed. Please wrap this heap delta assertion in a check that ensures globalThis.gc is available.

        if (typeof globalThis.gc === 'function') {
          assert.ok(
            heapDeltaMb < 40,
            'Expected bounded read heap growth (<40MB), got ' + heapDeltaMb.toFixed(2) + 'MB',
          );
        }

@danieljbruce

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a comprehensive Bun certification and compatibility test suite (bunCertificationMatrix.ts) to verify package installation, transport parity (gRPC and REST fallback), streaming integrity, and resource stability under Bun. It also implements a _destroy method in StreamArrayParser to properly handle stream cancellation without emitting unexpected errors, along with corresponding unit tests. The review feedback suggests dynamically detecting the Bun lockfile name (bun.lock vs. bun.lockb) to ensure compatibility with Bun versions prior to 1.2.0.

Comment on lines +191 to +196
function requireBun(ctx: Context): string {
if (!bunBin) {
ctx.skip();
}
return bunBin!;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In Bun versions prior to 1.2.0, the default lockfile format is binary and named bun.lockb instead of bun.lock. To ensure the certification matrix is compatible across all Bun versions (both pre-1.2 and 1.2+), we should dynamically detect and use the correct lockfile name.

    function requireBun(ctx: Context): string {
      if (!bunBin) {
        ctx.skip();
      }
      return bunBin!;
    }

    function getLockfilename(dir: string): string {
      return fs.existsSync(path.join(dir, 'bun.lock')) ? 'bun.lock' : 'bun.lockb';
    }

{cwd: coldDir, encoding: 'utf8', timeout: 60000},
);
assert.strictEqual(coldRes.status, 0, coldRes.stderr);
assert.ok(fs.existsSync(path.join(coldDir, 'bun.lock')));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Use the dynamic lockfile name helper to support both bun.lock and bun.lockb depending on the Bun version.

Suggested change
assert.ok(fs.existsSync(path.join(coldDir, 'bun.lock')));
assert.ok(fs.existsSync(path.join(coldDir, getLockfilename(coldDir))));

Comment on lines +286 to +294
const lockHashA = crypto
.createHash('sha256')
.update(fs.readFileSync(path.join(cleanA, 'bun.lock')))
.digest('hex');
const lockHashB = crypto
.createHash('sha256')
.update(fs.readFileSync(path.join(cleanB, 'bun.lock')))
.digest('hex');
assert.strictEqual(lockHashA, lockHashB);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Use the dynamic lockfile name helper to support both bun.lock and bun.lockb depending on the Bun version.

Suggested change
const lockHashA = crypto
.createHash('sha256')
.update(fs.readFileSync(path.join(cleanA, 'bun.lock')))
.digest('hex');
const lockHashB = crypto
.createHash('sha256')
.update(fs.readFileSync(path.join(cleanB, 'bun.lock')))
.digest('hex');
assert.strictEqual(lockHashA, lockHashB);
const lockFilenameA = getLockfilename(cleanA);
const lockHashA = crypto
.createHash('sha256')
.update(fs.readFileSync(path.join(cleanA, lockFilenameA)))
.digest('hex');
const lockFilenameB = getLockfilename(cleanB);
const lockHashB = crypto
.createHash('sha256')
.update(fs.readFileSync(path.join(cleanB, lockFilenameB)))
.digest('hex');
assert.strictEqual(lockHashA, lockHashB);

Comment on lines +313 to +317
const lockHashAfter = crypto
.createHash('sha256')
.update(fs.readFileSync(path.join(cleanA, 'bun.lock')))
.digest('hex');
assert.strictEqual(lockHashAfter, lockHashA);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Use the dynamic lockfile name helper to support both bun.lock and bun.lockb depending on the Bun version.

      const lockFilenameAfter = getLockfilename(cleanA);
      const lockHashAfter = crypto
        .createHash('sha256')
        .update(fs.readFileSync(path.join(cleanA, lockFilenameAfter)))
        .digest('hex');
      assert.strictEqual(lockHashAfter, lockHashA);

Comment on lines +347 to +354
fs.copyFileSync(
path.join(ciEnv1, 'package.json'),
path.join(ciEnv2, 'package.json'),
);
fs.copyFileSync(
path.join(ciEnv1, 'bun.lock'),
path.join(ciEnv2, 'bun.lock'),
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Use the dynamic lockfile name helper to support both bun.lock and bun.lockb depending on the Bun version.

      // Copy package.json + lockfile to second isolated CI runner directory
      const lockFilename = getLockfilename(ciEnv1);
      fs.copyFileSync(
        path.join(ciEnv1, 'package.json'),
        path.join(ciEnv2, 'package.json'),
      );
      fs.copyFileSync(
        path.join(ciEnv1, lockFilename),
        path.join(ciEnv2, lockFilename),
      );

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant