connpool: Allow time out during shutdown

[vmg]

Description

Since rolling out the new connection pool in Vitess 18, we've seen in production some cases were the connection pool fails to shutdown properly. I've spotted the following issues:

• The new connection pool is very strict when it comes to shutting down: it won't finish the shutdown operation until every single connection has been returned to the pool and cleanly closed. The previous connection pool was not this strict, so this behavior is new and seems to be causing issues.
• The new connection pool returns the wrong Error when trying to fetch a connection from the pool while the pool is already closed or during the process of closing. The old connection pool had a specific ErrConnPoolClosed, whilst the new pool returns a generic ErrTimeout that was causing very confusing messages to appear in the logs.
• The combination of the pool waiting indefinitely to shutdown and the wrong error message being reported caused logs to be spammed with a confusing error message.

I believe the most reliable approach here is to time-out the connection pool shutdown process and log a meaningful error to the logs. Of course, it's also fundamental to figure out the cause for the missing connections which are not being returned to the pool.

I've detected two likely culprits:

• Some connections that have been fetched from the pool are not being properly Recycled (i.e. returned) from the caller. This usually happens when the connection errors out; the most likely scenario is when a connection panics and the Recycle method on the collection was not called inside a defer. A similar leak can happen during normal error handling, if the connection is simply not being Recycled during the error handling path.

• Re-opening or refreshing a connection pool can race with a normal Shutdown, causing the shutdown to never finish. This would be very unlikely but very possible. The fix for this is straightforward by locking the shutdown process.

cc @harshit-gangal @deepthi

Related Issue(s)

• Bug Report: Connection Pool Full error locks up vttablet #15745 (shutdown lockup; confusing error message in logs)
• TODO @aquarapid (detected case where a connection leaks after an error)

Checklist

• "Backport to:" labels have been added if this change should be back-ported to release branches
• If this change is to be back-ported to previous releases, a justification is included in the PR description
• Tests were added or are not required
• Did the new or modified tests pass consistently locally and on CI?
• Documentation was added or is not required

Deployment Notes

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

• Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

• Apply the release notes (needs details) label if users need to know about this change.
• New features should be documented.
• There should be some code comments as to why things are implemented the way they are.
• There should be a comment at the top of each new or modified test to explain what the test does.

New flags

• Is this flag really necessary?
• Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
• vtctl command output order should be stable and awk-able.

[codecov]

Codecov Report

Attention: Patch coverage is 72.72727% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 68.47%. Comparing base (8b60b59) to head (80a6995).
Report is 7 commits behind head on main.

Files	Patch %	Lines
go/vt/vttablet/tabletserver/debugenv.go	0.00%	11 Missing ⚠️
go/pools/smartconnpool/pool.go	91.42%	3 Missing ⚠️
go/vt/vttablet/tabletserver/tabletserver.go	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15979      +/-   ##
==========================================
+ Coverage   68.46%   68.47%   +0.01%     
==========================================
  Files        1562     1562              
  Lines      197059   197100      +41     
==========================================
+ Hits       134919   134972      +53     
+ Misses      62140    62128      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[harshit-gangal]

The new connection pool is very strict when it comes to shutting down: it won't finish the shutdown operation until every single connection has been returned to the pool and cleanly closed. The previous connection pool was not this strict, so this behavior is new and seems to be causing issues.

Just correcting the assumption here. The old pool was equally strict and did not return from close unless the capacity reaches 0 i.e. all the connections are closed.