fix(firestore): eager evict idle REST clients on gRPC transition

[dlarocque]

When ClientPool transitions from REST to gRPC (for example, when unary REST ops are followed by a grpc listen/onsnapshot stream), idle REST clients stay in activeClients indefinitely. Because acquire() rejects REST clients once pool.grpcEnabled is true, these idle REST clients were never acquired or released again, making release-time GC ineffective for them.

So, when gRPC clients completed ops, shouldGarbageCollectClient() summed idle capacity across all clients, including the zombie REST client (100 REST + 100 gRPC = 200 > 100 max). This resulted in every gRPC client being immediately destroyed upon release, causing ~1:1 connection churn and google auth token refreshes on all subsequent RPCs.

Changes:

1. Added a transitionToGrpc() helper that eagerly evicts idle REST clients (activeRequestCount === 0) from activeClients when transitioning to gRPC.
2. Filtering idle capacity calculations in shouldGarbageCollectClient() so idle count calculations only include clients matching pool.grpcEnabled.
3. Added unit tests asserting gRPC client reuse after transition.

Notes:

• REST clients with active requests during transition will complete and are garbage collected on release(). They are not eagerly evicted.

Fixes #8294

[gemini-code-assist]

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

[dlarocque]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a transition mechanism to gRPC in ClientPool, allowing eager garbage collection of idle REST clients when switching. It also updates the idle capacity calculation to filter by the current gRPC status and adds corresponding unit tests. The review feedback highlights two key issues: a potential resource leak where active REST clients completing after the transition may not be garbage collected, and a risk of unhandled promise rejections when invoking the client destructor without error handling.

[gemini-code-assist]

Active REST Clients Leak After Transition

There is a potential connection/resource leak for REST clients that are active during the transition to gRPC.

The PR description notes:

REST clients with active requests during transition will complete and are garbage collected on release(). They are not eagerly evicted.

However, under the new logic in shouldGarbageCollectClient, when an active REST client completes and release() is called, shouldGarbageCollectClient is invoked. Inside this method, idleCapacityCount only sums the idle capacity of clients matching this.grpcEnabled (which is now true for gRPC).

Since the REST client's metadata.grpcEnabled is false, its idle capacity is not counted. If the total idle capacity of the gRPC clients does not exceed maxIdleCapacityCount, shouldGarbageCollectClient will return false for the REST client.

As a result, the REST client will not be garbage collected and will remain in activeClients indefinitely as an idle zombie, because it can never be acquired again.

Suggested Fix

To prevent this leak, we should immediately return true (garbage collect) if the client being evaluated does not match the pool's current grpcEnabled status.

You can add this check at the very beginning of shouldGarbageCollectClient (which is outside the current diff hunk):

private shouldGarbageCollectClient(client: T): boolean {
  const clientMetadata = this.activeClients.get(client);
  if (clientMetadata && clientMetadata.grpcEnabled !== this.grpcEnabled) {
    return true;
  }
  // ... rest of the existing logic
}

[dlarocque]

The idleCapacityCount check only happens after we check

if (this.grpcEnabled !== clientMetadata.grpcEnabled)

which is true since this.grpcEnabled is true, and clientMetadata.grpcEnabled would be false- in which case we immediately return PoolIsTransitioningFromGrpc(), which has shouldGarbageCollectClient: true.

I added a test to verify that the rest connection is garbage collected.

[dlarocque]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a mechanism to transition the client pool to gRPC and eagerly garbage-collect idle REST clients, along with comprehensive unit tests to verify the transition and cleanup behavior. The review feedback identifies a critical issue where calling the optional clientDestructor directly can cause a runtime TypeError if it is undefined, or crash the pool transition if it throws a synchronous error. A code suggestion is provided to safely check and invoke the destructor.

[gemini-code-assist]

The clientDestructor parameter is optional in the ClientPool constructor and can be undefined (as demonstrated in some of the unit tests where only 3 arguments are passed). Calling this.clientDestructor(client) directly without checking if it is defined will throw a TypeError at runtime and crash the pool transition.

Additionally, wrapping the call in Promise.resolve(this.clientDestructor(client)) does not catch synchronous exceptions thrown by the destructor function itself, which would also propagate and crash the caller.

To fix both issues, we should check if this.clientDestructor is defined, and safely invoke it within a .then() block of a resolved promise chain to ensure any synchronous errors are safely caught and logged.

        if (this.clientDestructor) {
          const destructor = this.clientDestructor;
          void Promise.resolve()
            .then(() => destructor(client))
            .catch(err => {
              logger(
                'ClientPool[' + this.instanceId + '].transitionToGrpc',
                requestTag,
                'Failed to destroy client: %s',
                err,
              );
            });
        }