fix: pool.ts: add more detailed logging for client garbage collection (#2420)

dconeybe · web-flow · commit 1bbca46ff2a6 · 2025-09-22T14:37:37.000-04:00
diff --git a/dev/src/pool.ts b/dev/src/pool.ts
@@ -176,8 +176,10 @@ export class ClientPool<T extends object> {
    * @internal
    */
   private async release(requestTag: string, client: T): Promise<void> {
+    const clientId = this.clientIdByClient.get(client);
     const metadata = this.activeClients.get(client);
     assert(metadata && metadata.activeRequestCount > 0, 'No active requests');
+
     this.activeClients.set(client, {
       grpcEnabled: metadata.grpcEnabled,
       activeRequestCount: metadata.activeRequestCount - 1,
@@ -186,26 +188,38 @@ export class ClientPool<T extends object> {
       this.terminateDeferred.resolve();
     }
 
-    if (this.shouldGarbageCollectClient(client)) {
-      const clientId = this.clientIdByClient.get(client);
-      logger(
-        'ClientPool.release',
-        requestTag,
-        'Garbage collecting client [%s] (%s)',
-        clientId,
-        this.lazyLogStringForAllClientIds
-      );
-      this.activeClients.delete(client);
-      this.failedClients.delete(client);
-      await this.clientDestructor(client);
-      logger(
-        'ClientPool.release',
-        requestTag,
-        'Garbage collected client [%s] (%s)',
-        clientId,
-        this.lazyLogStringForAllClientIds
-      );
+    const gcDetermination = this.shouldGarbageCollectClient(client);
+    logger(
+      'ClientPool.release',
+      requestTag,
+      'Releasing client [%s] (gc=%s)',
+      clientId,
+      gcDetermination
+    );
+
+    if (!gcDetermination.shouldGarbageCollectClient) {
+      return;
     }
+
+    logger(
+      'ClientPool.release',
+      requestTag,
+      'Garbage collecting client [%s] (%s)',
+      clientId,
+      this.lazyLogStringForAllClientIds
+    );
+
+    this.activeClients.delete(client);
+    this.failedClients.delete(client);
+    await this.clientDestructor(client);
+
+    logger(
+      'ClientPool.release',
+      requestTag,
+      'Garbage collected client [%s] (%s)',
+      clientId,
+      this.lazyLogStringForAllClientIds
+    );
   }
 
   /**
@@ -214,23 +228,36 @@ export class ClientPool<T extends object> {
    * @private
    * @internal
    */
-  private shouldGarbageCollectClient(client: T): boolean {
+  private shouldGarbageCollectClient(
+    client: T
+  ): ShouldGarbageCollectClientResult {
     const clientMetadata = this.activeClients.get(client)!;
 
     if (clientMetadata.activeRequestCount !== 0) {
       // Don't garbage collect clients that have active requests.
-      return false;
+      return new ClientHasActiveRequests({
+        shouldGarbageCollectClient: false,
+        clientActiveRequestCount: clientMetadata.activeRequestCount,
+      });
     }
 
     if (this.grpcEnabled !== clientMetadata.grpcEnabled) {
       // We are transitioning to GRPC. Garbage collect REST clients.
-      return true;
+      return new PoolIsTransitioningToGrpc({
+        shouldGarbageCollectClient: true,
+        clientActiveRequestCount: clientMetadata.activeRequestCount,
+        poolGrpcEnabled: this.grpcEnabled,
+        clientGrpcEnabled: clientMetadata.grpcEnabled,
+      });
     }
 
     // Idle clients that have received RST_STREAM errors are always garbage
     // collected.
     if (this.failedClients.has(client)) {
-      return true;
+      return new ClientIsFailed({
+        shouldGarbageCollectClient: true,
+        clientActiveRequestCount: clientMetadata.activeRequestCount,
+      });
     }
 
     // Otherwise, only garbage collect if we have too much idle capacity (e.g.
@@ -240,9 +267,17 @@ export class ClientPool<T extends object> {
       idleCapacityCount +=
         this.concurrentOperationLimit - metadata.activeRequestCount;
     }
-    return (
-      idleCapacityCount > this.maxIdleClients * this.concurrentOperationLimit
-    );
+
+    const maxIdleCapacityCount =
+      this.maxIdleClients * this.concurrentOperationLimit;
+    return new IdleCapacity({
+      shouldGarbageCollectClient: idleCapacityCount > maxIdleCapacityCount,
+      clientActiveRequestCount: clientMetadata.activeRequestCount,
+      idleCapacityCount: idleCapacityCount,
+      maxIdleCapacityCount: maxIdleCapacityCount,
+      maxIdleClients: this.maxIdleClients,
+      concurrentOperationLimit: this.concurrentOperationLimit,
+    });
   }
 
   /**
@@ -386,3 +421,113 @@ class LazyLogStringForAllClientIds<T extends object> {
       .join(', ');
   }
 }
+
+/**
+ * Minimum data to be included in the objects returned from
+ * ClientPool.shouldGarbageCollectClient().
+ */
+abstract class BaseShouldGarbageCollectClientResult {
+  abstract readonly name: string;
+  abstract readonly shouldGarbageCollectClient: boolean;
+  abstract readonly clientActiveRequestCount: number;
+
+  /**
+   * Return a terse, one-line string representation. This makes it easy to
+   * grep through log output to find the logged values.
+   */
+  toString(): string {
+    const propertyStrings: string[] = [];
+    for (const propertyName of Object.getOwnPropertyNames(this)) {
+      const propertyValue = this[propertyName as keyof typeof this];
+      propertyStrings.push(`${propertyName}=${propertyValue}`);
+    }
+    return '{' + propertyStrings.join(', ') + '}';
+  }
+}
+
+class ClientHasActiveRequests extends BaseShouldGarbageCollectClientResult {
+  override readonly name = 'ClientHasActiveRequests' as const;
+  override readonly shouldGarbageCollectClient: false;
+  override readonly clientActiveRequestCount: number;
+
+  constructor(args: {
+    shouldGarbageCollectClient: false;
+    clientActiveRequestCount: number;
+  }) {
+    super();
+    this.shouldGarbageCollectClient = args.shouldGarbageCollectClient;
+    this.clientActiveRequestCount = args.clientActiveRequestCount;
+  }
+}
+
+class PoolIsTransitioningToGrpc extends BaseShouldGarbageCollectClientResult {
+  override readonly name = 'PoolIsTransitioningToGrpc' as const;
+  override readonly shouldGarbageCollectClient: true;
+  override readonly clientActiveRequestCount: 0;
+  readonly poolGrpcEnabled: boolean;
+  readonly clientGrpcEnabled: boolean;
+
+  constructor(args: {
+    shouldGarbageCollectClient: true;
+    clientActiveRequestCount: 0;
+    poolGrpcEnabled: boolean;
+    clientGrpcEnabled: boolean;
+  }) {
+    super();
+    this.shouldGarbageCollectClient = args.shouldGarbageCollectClient;
+    this.clientActiveRequestCount = args.clientActiveRequestCount;
+    this.poolGrpcEnabled = args.poolGrpcEnabled;
+    this.clientGrpcEnabled = args.clientGrpcEnabled;
+  }
+}
+
+class ClientIsFailed extends BaseShouldGarbageCollectClientResult {
+  override readonly name = 'ClientIsFailed' as const;
+  override readonly shouldGarbageCollectClient: true;
+  override readonly clientActiveRequestCount: 0;
+
+  constructor(args: {
+    shouldGarbageCollectClient: true;
+    clientActiveRequestCount: 0;
+  }) {
+    super();
+    this.shouldGarbageCollectClient = args.shouldGarbageCollectClient;
+    this.clientActiveRequestCount = args.clientActiveRequestCount;
+  }
+}
+
+class IdleCapacity extends BaseShouldGarbageCollectClientResult {
+  override readonly name = 'IdleCapacity' as const;
+  override readonly shouldGarbageCollectClient: boolean;
+  override readonly clientActiveRequestCount: 0;
+  readonly idleCapacityCount: number;
+  readonly maxIdleCapacityCount: number;
+  readonly maxIdleClients: number;
+  readonly concurrentOperationLimit: number;
+
+  constructor(args: {
+    shouldGarbageCollectClient: boolean;
+    clientActiveRequestCount: 0;
+    idleCapacityCount: number;
+    maxIdleCapacityCount: number;
+    maxIdleClients: number;
+    concurrentOperationLimit: number;
+  }) {
+    super();
+    this.shouldGarbageCollectClient = args.shouldGarbageCollectClient;
+    this.clientActiveRequestCount = args.clientActiveRequestCount;
+    this.idleCapacityCount = args.idleCapacityCount;
+    this.maxIdleCapacityCount = args.maxIdleCapacityCount;
+    this.maxIdleClients = args.maxIdleClients;
+    this.concurrentOperationLimit = args.concurrentOperationLimit;
+  }
+}
+
+/**
+ * The set of return types from ClientPool.shouldGarbageCollectClient().
+ */
+type ShouldGarbageCollectClientResult =
+  | ClientHasActiveRequests
+  | PoolIsTransitioningToGrpc
+  | ClientIsFailed
+  | IdleCapacity;
