smartconnpool: avoid close deadlock with refresh reopen by arthurschreiber · Pull Request #20157 · vitessio/vitess

arthurschreiber · 2026-05-21T11:33:36Z

Description

Fixes a close-time deadlock in smartconnpool where CloseWithContext could wait for background workers while still holding capacityMu. A refresh worker already inside reopen() could then block on that same mutex, leaving close stuck.

This also keeps closed/open state separate from capacity changes in the affected paths. SetCapacity can still update desired capacity, but Get, getNew, put(nil), and returned idle connections now check whether the pool is closed before opening or retaining connections. The stress test avoids using SetCapacity(0) as a synthetic close signal so it exercises real CloseWithContext behavior.

Related Issue(s)

Related to #20122.

Backport justification: this fixes a rare smartconnpool shutdown deadlock when CloseWithContext races with refresh-driven reopen(). The race can stall pool shutdown in production, and the fix is contained to connection-pool close/open state handling.

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

Local test runs:

go test -count=1 -run '^(TestTxPoolBeginWithPoolConnectionError_Errno2006_Transient)$' -timeout 180s ./go/vt/vttablet/tabletserver
go test -count=1 ./go/pools/smartconnpool
go test -count=1 -race -run '^(TestCloseWithContextAfterSetCapacityZeroClosesPool|TestStressCloseDuringTraffic)$' -timeout 240s ./go/pools/smartconnpool
go test -count=1 -race -run '^(TestTxPoolBeginWithPoolConnectionError_Errno2006_Transient)$' -timeout 180s ./go/vt/vttablet/tabletserver

Deployment Notes

No deployment steps required.

AI Disclosure

This PR was written with AI assistance from Codex, with direction and review from Arthur.

Signed-off-by: Arthur Schreiber <arthur@planetscale.com>

vitess-bot · 2026-05-21T11:34:12Z

Copilot

Pull request overview

Fixes a close-time deadlock in smartconnpool by ensuring CloseWithContext does not wait for worker goroutines while holding capacityMu, and by preventing refresh reopens / capacity changes from reviving a pool after it has been closed.

Changes:

Update CloseWithContext to release capacityMu before waiting on pool.workers.
Make reopen() exit early when the pool is closed, and make SetCapacity() return ErrConnPoolClosed after close.
Add regression + stress tests that exercise close-during-traffic with concurrent refresh reopen and capacity updates.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
go/pools/smartconnpool/pool.go	Adjusts close/reopen/capacity logic to avoid deadlock and prevent post-close resurrection.
go/pools/smartconnpool/pool_test.go	Adds focused regressions for close behavior after `SetCapacity(0)` and for `SetCapacity` after close.
go/pools/smartconnpool/stress_test.go	Adds a multi-cycle stress test that closes the pool under concurrent traffic, refresh reopen, and capacity changes.

codecov · 2026-05-21T11:39:05Z

Codecov Report

❌ Patch coverage is 66.66667% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.74%. Comparing base (70c7a72) to head (c9149b6).
⚠️ Report is 270 commits behind head on main.

Files with missing lines	Patch %	Lines
go/pools/smartconnpool/pool.go	66.66%	7 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (70c7a72) and HEAD (c9149b6). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (70c7a72) HEAD (c9149b6)

1 0

Additional details and impacted files

@@             Coverage Diff             @@
##             main   #20157       +/-   ##
===========================================
- Coverage   69.67%   54.74%   -14.93%     
===========================================
  Files        1614        8     -1606     
  Lines      216793      937   -215856     
===========================================
- Hits       151044      513   -150531     
+ Misses      65749      424    -65325

Flag	Coverage Δ
partial	`54.74% <66.66%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: Arthur Schreiber <arthur@planetscale.com>

Mirror the cleanup applied to the other shutdown stress tests: fold the two watchdog-failure cleanup sequences into a small abort closure with a two-mode signature (pre-close vs. mid-close), rename WatchdogDelay to Watchdog so the constant matches the sibling helpers, inline the unused worker := i alias, and add a doc comment to TestStressCloseDuringTraffic describing the scenario and the invariants it checks. No behavior change. Signed-off-by: Arthur Schreiber <arthur@planetscale.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

arthurschreiber · 2026-05-21T15:32:59Z

@@ -310,6 +310,7 @@ func (pool *ConnPool[C]) CloseWithContext(ctx context.Context) error {
 	closeChan := *pool.close.Swap(nil)
 	close(closeChan)

+	pool.capacityMu.Unlock()
 	pool.workers.Wait()


Good catch — fixed in c9149b6. setCapacity's early return now also requires pool.active <= newcap, so a CloseWithContext following a timed-out SetCapacity(0) re-enters the drain loop rather than short-circuiting. Added a regression test (TestCloseWithContextDrainsAfterTimedOutSetCapacityZero) that holds a conn, drives SetCapacity(0) with a cancelled context to force the timeout, then asserts CloseWithContext returns an error rather than silently completing.

…y(0) setCapacity short-circuits when oldcap == newcap. That's fine for a genuine no-op SetCapacity, but it also fires after a prior SetCapacity(0) timed out with borrowed conns still out: capacity is swapped to 0 before the drain loop, so the loop's timeout leaves the pool at capacity=0 with active>0. A subsequent CloseWithContext then calls setCapacity(0) again, hits the early return without re-entering the drain loop, and returns nil despite borrowed conns still being out — in violation of CloseWithContext's documented contract. Tighten the early return to require pool.active <= newcap as well, so a follow-up call resumes draining when the pool is over the target. Borrowed conns released after CloseWithContext returns still get closed cleanly via tryReturnConn's closed-pool branch; this just fixes the caller-visible return value. Add a regression test that holds a conn, runs SetCapacity(0) with a cancelled context to force the timeout, then asserts CloseWithContext also returns an error rather than silently completing. Signed-off-by: Arthur Schreiber <arthur@planetscale.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

mattlord

LGTM! Thanks, @arthurschreiber !

…l-close-lock-order Signed-off-by: Arthur Schreiber <arthur@planetscale.com> # Conflicts: # go/pools/smartconnpool/pool.go

The cherry-pick of 4061458 left conflict markers in pool.go. The upstream PR added an early-return check to tryReturnConn, but the function signature differs between branches: main has `(conn *Pooled[C], updateIdleTime bool)` while release-24.0 has `(conn *Pooled[C])`. Keep release-24.0's signature and apply the new close-state check on top. Signed-off-by: Arthur Schreiber <arthur@planetscale.com>

The cherry-pick of 4061458 left conflict markers in pool.go. The upstream PR added an early-return check to tryReturnConn, but the function signature differs between branches: main has `(conn *Pooled[C], updateIdleTime bool)` while release-23.0 has `(conn *Pooled[C])`. Keep release-23.0's signature and apply the new close-state check on top. Signed-off-by: Arthur Schreiber <arthur@planetscale.com>

#20157) (#20181) Signed-off-by: Arthur Schreiber <arthur@planetscale.com> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com> Co-authored-by: Arthur Schreiber <arthur@planetscale.com>

#20157) (#20180) Signed-off-by: Arthur Schreiber <arthur@planetscale.com> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com> Co-authored-by: Arthur Schreiber <arthur@planetscale.com>

smartconnpool: fix close lock ordering

5dcc7f8

Signed-off-by: Arthur Schreiber <arthur@planetscale.com>

Copilot AI review requested due to automatic review settings May 21, 2026 11:33

arthurschreiber requested a review from harshit-gangal as a code owner May 21, 2026 11:33

github-actions Bot added this to the v25.0.0 milestone May 21, 2026

Copilot AI reviewed May 21, 2026

View reviewed changes

arthurschreiber mentioned this pull request May 21, 2026

smartconnpool: reject SetCapacity on a closed pool + shutdown stress tests #20158

Draft

5 tasks

smartconnpool: preserve closed state during capacity changes

4faf081

Signed-off-by: Arthur Schreiber <arthur@planetscale.com>

Copilot AI review requested due to automatic review settings May 21, 2026 14:40

Copilot started reviewing on behalf of arthurschreiber May 21, 2026 14:40 View session

Copilot AI reviewed May 21, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings May 21, 2026 15:06

Copilot started reviewing on behalf of arthurschreiber May 21, 2026 15:07 View session

Copilot AI reviewed May 21, 2026

View reviewed changes

mattlord approved these changes May 22, 2026

View reviewed changes

Merge remote-tracking branch 'upstream/main' into arthur/smartconnpoo…

009aad6

…l-close-lock-order Signed-off-by: Arthur Schreiber <arthur@planetscale.com> # Conflicts: # go/pools/smartconnpool/pool.go

timvaillancourt approved these changes May 26, 2026

View reviewed changes

arthurschreiber merged commit 4061458 into main May 26, 2026
107 of 108 checks passed

arthurschreiber deleted the arthur/smartconnpool-close-lock-order branch May 26, 2026 13:38

This was referenced May 26, 2026

[release-23.0] smartconnpool: avoid close deadlock with refresh reopen (#20157) #20180

Merged

[release-24.0] smartconnpool: avoid close deadlock with refresh reopen (#20157) #20181

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

smartconnpool: avoid close deadlock with refresh reopen#20157

smartconnpool: avoid close deadlock with refresh reopen#20157
arthurschreiber merged 5 commits into
mainfrom
arthur/smartconnpool-close-lock-order

arthurschreiber commented May 21, 2026 •

edited

Loading

Uh oh!

vitess-bot Bot commented May 21, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

codecov Bot commented May 21, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

arthurschreiber May 21, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

mattlord left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

arthurschreiber commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue(s)

Checklist

Deployment Notes

AI Disclosure

Uh oh!

vitess-bot Bot commented May 21, 2026

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

codecov Bot commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

arthurschreiber May 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mattlord left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

arthurschreiber commented May 21, 2026 •

edited

Loading

codecov Bot commented May 21, 2026 •

edited

Loading