Skip to content
This repository was archived by the owner on Mar 4, 2026. It is now read-only.

fix: pool.ts: assign unique IDs to clients and include those IDs in log messages#2416

Merged
dconeybe merged 8 commits intomainfrom
dconeybe/ClientIdsLoggedForB437011084
Sep 16, 2025
Merged

fix: pool.ts: assign unique IDs to clients and include those IDs in log messages#2416
dconeybe merged 8 commits intomainfrom
dconeybe/ClientIdsLoggedForB437011084

Conversation

@dconeybe
Copy link
Contributor

@dconeybe dconeybe commented Sep 11, 2025 •
edited
Loading

Here are some example logs showing the "client IDs" (which all begin with "cli") that are now included in the logs and will help troubleshoot issues with client management:

tag [ClientPool.release]: Garbage collecting client [cliAbHPt] (3 active clients: {cliAbHPt, cliDQeFV, cliIUz9K}, 0 failed clients: {})
tag [ClientPool.release]: Garbage collecting client [cliDQeFV] (2 active clients: {cliDQeFV, cliIUz9K}, 0 failed clients: {})
tag [ClientPool.release]: Garbage collecting client [cliIUz9K] (1 active clients: {cliIUz9K}, 0 failed clients: {})
tag [ClientPool.release]: Garbage collected client [cliAbHPt] (0 active clients: {}, 0 failed clients: {})
tag [ClientPool.release]: Garbage collected client [cliDQeFV] (0 active clients: {}, 0 failed clients: {})
tag [ClientPool.release]: Garbage collected client [cliIUz9K] (0 active clients: {}, 0 failed clients: {})
tag [ClientPool.acquire]: Creating a new client [cliV4B49] (requiresGrpc: true)
tag [ClientPool.release]: Garbage collecting client [cliV4B49] (1 active clients: {cliV4B49}, 1 failed clients: {cliV4B49})
tag [ClientPool.release]: Garbage collected client [cliV4B49] (0 active clients: {}, 0 failed clients: {})
tag [ClientPool.acquire]: Creating a new client [clixvnaw] (requiresGrpc: true)
tag [ClientPool.acquire]: Creating a new client [cliAAl91] (requiresGrpc: false)
tag [ClientPool.acquire]: Re-using existing client [cliAAl91] with 1 remaining operations

The motivation for this PR is to help investigate b/437011084, where ClientPool.acquire() apparently is returning previously-garbage-collected clients, resulting in "GoogleError: The client has already been closed". A follow-up PR, #2420, adds even more logging towards this investigation.

BEGIN_COMMIT_OVERRIDE
fix: Improve debug logging for the internal client pool. Added client IDs to debug log statements for client management.
END_COMMIT_OVERRIDE

@dconeybe
pool.ts: assign unique IDs to clients and include those IDs in log me…
57318ed 
…ssages.

The immediate use case is to assist with debugging b/437011084, a
suspected race condition in the acquire/release logic of ClientPool.
@dconeybe dconeybe self-assigned this Sep 11, 2025
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: firestore Issues related to the googleapis/nodejs-firestore API. labels Sep 11, 2025
@dconeybe dconeybe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 12, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 12, 2025
@dconeybe dconeybe marked this pull request as ready for review September 15, 2025 17:29
@dconeybe dconeybe requested review from a team as code owners September 15, 2025 17:29
@dconeybe dconeybe requested a review from ehsannas September 15, 2025 17:29
ehsannas
ehsannas previously approved these changes Sep 15, 2025
View reviewed changes
@dconeybe dconeybe merged commit 99918f1 into main Sep 16, 2025
24 of 25 checks passed
@dconeybe dconeybe deleted the dconeybe/ClientIdsLoggedForB437011084 branch September 16, 2025 02:17
@MarkDuckworth MarkDuckworth changed the title change: pool.ts: assign unique IDs to clients and include those IDs in log messages fix: pool.ts: assign unique IDs to clients and include those IDs in log messages Sep 16, 2025
@MarkDuckworth MarkDuckworth added the release-please:force-run To run release-please label Sep 16, 2025
@release-please release-please bot removed the release-please:force-run To run release-please label Sep 16, 2025
@MarkDuckworth MarkDuckworth added the release-please:force-run To run release-please label Sep 16, 2025
@release-please release-please bot removed the release-please:force-run To run release-please label Sep 16, 2025
@MarkDuckworth MarkDuckworth added the release-please:force-run To run release-please label Sep 16, 2025
@release-please release-please bot removed the release-please:force-run To run release-please label Sep 16, 2025
@dconeybe dconeybe mentioned this pull request Sep 19, 2025
Merged
@dconeybe dconeybe mentioned this pull request Sep 25, 2025
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@ehsannas ehsannas ehsannas approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@dconeybe dconeybe

Labels

api: firestore Issues related to the googleapis/nodejs-firestore API. size: m Pull request size is medium.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dconeybe @ehsannas @MarkDuckworth @yoshi-kokoro