fix(spanner): avoid double decrement in session counting

[egonelbre]

No description provided.

[gemini-code-assist]

Summary of Changes

Hello @egonelbre, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a subtle bug in the Spanner client's session management where the count of in-use sessions could become negative due to an erroneous double decrement. The change ensures the session counter is accurately maintained, preventing potential issues with session pool health and resource management. New comprehensive tests have been added to validate the fix and prevent future regressions.

Highlights

• Fix double decrement in session counting: The recycle method in spanner/session.go was modified to prevent a double decrement of the numInUse counter. Previously, if a session was destroyed due to expiration and the pool was full, s.destroy would decrement the counter, and then s.pool.decNumInUseLocked would decrement it again. The fix introduces an early return after s.destroy in this specific scenario.
• Added new test cases for session recycling: Two new test cases, TestSessionRecycleAfterPoolClose_NoDoubleDecrement and TestSessionRecycle_AlreadyInvalidSession, were added to spanner/session_test.go. These tests specifically target scenarios where a session might be recycled when its pool is invalid or when the session itself has already been destroyed, ensuring the numInUse counter remains accurate and does not go negative.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request addresses a potential race condition in session management that could lead to a double decrement of the in-use session counter. The fix in spanner/session.go is straightforward and correct, preventing the second decrement by returning early. The addition of two targeted tests in spanner/session_test.go is excellent, as they cover the specific scenarios where this bug could manifest. The changes are well-contained and effectively resolve the issue. I have one minor suggestion to improve consistency in the new test code.

[rahul2393]

@egonelbre Thanks for opening this PR, we are planning to remove all session pool related code in next release since by default client lib uses multiplexed sessions, are you still using regular session in your application? if yes, can you please tell us any specific reason for using it?

[egonelbre]

@rahul2393 sure.

So we've delayed upgrading spanner library for some time because they ended up stalling out our CI pipeline and were unsure why. But, we don't have a nice small reproducer. Yesterday and today I finally had time to bisect the issue to 0491ba6 commit. After updating to latest spanner library and "GOOGLE_CLOUD_SPANNER_MULTIPLEXED_SESSIONS=false" the CI pipeline does seem to work again.

The high level scenario is following, the CI pipeline sets up 7 spanner emulators (to avoid emulator internal locks), each tests sets up and tears down a new database/tables. Tests run in a highly parallel fashion (I would guess about 8000 tests, each setting up 2-3 databases per test and 2-6 connection pools per test). After some time the spanner emulator stops accepting connections. This suggests that something isn't being properly cleaned up. I'm guessing that either connections or sessions are left open, which eventually exhaust the spanner emulator. Or that some goroutines are not being shut down. Or that some retry mechanism is causing problems. But, it could also be a spanner emulator bug with regards to multiplexed sessions that break it down.

I'm still figuring out what the next best step is in debugging multiplexed sessions. Probably will either try to add some monitoring against spanner emulators. Any suggestions for debugging would be very welcome.

This exact fix came about when I ran with that specific environment flag disabled, and had one test failure with the specific message. Directed gemini to diagnose the issue for me. And, if the single sessions are being removed I'm fine closing the PR.

[egonelbre]

It looks like one of the issues is #13396; still testing whether it resolves the full problem.

EDIT: seems that it's not sufficient, but it does avoid goroutine buildup.

[egonelbre]

@rahul2393 as far as I can able to debug. It seems it's partially related to #10378 issue; and partially that the spanner emulator falters around having ~10000 sessions open (GoogleCloudPlatform/cloud-spanner-emulator#159). I'm not yet sure why multiplexed connections make it worse.

[rahul2393]

@olavloite / @sakthivelmanii need help in one more approval to merge this one, please help