smartconnpool: do not hand connections to expired waiters - #238
Conversation
A returned connection could be handed to a waiter whose acquisition context had already expired. The waiter had no way to refuse it, so the connection was consumed by a request that could no longer use it. Semantic backport of vitessio#20308, adapted to the v21 semaphore-based waitlist: - tryReturnConnSlow evicts expired waiters during the return scan and continues past interior expired waiters to find a live one - maybeStarvingCount no longer counts expired waiters as starving - go/list gains O(1) RemoveIfPresent so an evicted waiter that is concurrently timing out cannot double-remove - WaitCount/WaitTime remain successful-acquisition-only: waitForConn can now return (nil, nil), so recordWait is guarded on conn == nil No waiter cap, no timeout changes, no unrelated fixes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1f56db40-28ec-4335-97a1-b60deee78b07
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 1
新建议题s introduced by this change (2)
| Severity | Finding |
|---|---|
 |
go/pools/smartconnpool/post_deadline_handoff_test.go — This polling loop has no timeout, so a failure to enqueue leaves the package test hanging until the… |
 |
go/list/list.go — This wording implies that concurrent removals are safe, but e.list and the link fields are… |
What changed in this PR
Backports expired-waiter eviction to v21’s semaphore-based smart connection pool.
Changes:
- Evicts expired waiters before connection handoff.
- Preserves success-only wait metrics.
- Adds O(1) waitlist removal and regression tests.
| File | Description |
|---|---|
go/pools/smartconnpool/waitlist.go |
Evicts expired waiters safely. |
go/pools/smartconnpool/pool.go |
Handles nil eviction results. |
go/list/list.go |
Adds conditional O(1) removal. |
go/pools/smartconnpool/post_deadline_handoff_test.go |
Tests deadline and interior-waiter behavior. |
go/pools/smartconnpool/expired_waiter_test.go |
Tests eviction and race safety. |
Suppressed comments (1)
go/pools/smartconnpool/post_deadline_handoff_test.go:203
- This second enqueue wait is also unbounded; if the expired waiter exits or never queues, the test hangs rather than reporting the failed precondition. Bound the poll with
require.Eventually.
for p.wait.waiting() < 2 {
time.Sleep(200 * time.Microsecond)
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| for p.wait.waiting() < 1 { | ||
| time.Sleep(200 * time.Microsecond) | ||
| } |
| // RemoveIfPresent removes e from l if e is currently an element of l, and | ||
| // reports whether it was removed. Unlike Remove, it does not panic when e does | ||
| // not belong to l, so callers that may race with another goroutine removing e | ||
| // can use the return value to decide who owns the element. |
Note on red CI + independent linux/amd64 verificationThe red checks on this PR are pre-existing repo-wide CI breakage, not caused by this change. Nearly every failing job dies in the The Proof this is unrelated to this PR: #236 is a Dependabot npm/yarn bump containing zero Go changes, and it has 150 failing Independent verification on linux/amd64Because CI cannot run the suite, I verified in a clean GitHub Codespace ( Control — unpatched base Note This PR's head Same results were obtained on darwin/arm64 (go1.26.4), so the behaviour is not platform-specific. |
Load-level evidence: 5.5x fewer post-deadline acquisitions, at zero throughput costThe deterministic tests in this PR prove the invariant. This adds a load-level A/B against the exact deployed base ( Harness. Open-loop staged overload driving the real Production TX-pool waiters are FIFO-ordered, so the run uses heterogeneous deadlines (25% of requests use 150ms): a short-deadline waiter enqueued early expires while longer-deadline waiters behind it are still live, which is exactly the interior-expired-waiter state this PR addresses. Metric. Successful acquisitions whose acquisition context had already expired (
5.5x reduction, n=3, no overlap. Worst-case excess past the deadline also drops: base reached 49.0ms / 34.1ms / 17.4ms across runs, this PR stayed at 11.8ms or below. Throughput is unchanged. Goodput per stage was identical on both builds: So the extra list scan and the eviction path cost nothing measurable at 4x overload with ~6,400 concurrent waiters. The residue is the goroutine-scheduling floor, not a remaining defect. On this branch What this does not showThe pre-fix build also kept making progress in this harness: it plateaued at ~2,200 tx/s, held InUse at 19.9/20, and recovered fully. This load test therefore does not demonstrate that the defect causes pool collapse, and it is not offered as incident-causality evidence. It shows the defect is real and reachable under ordinary overload, that this PR removes ~82% of its occurrences, and that it does so without a throughput or latency cost. Suites
|
ExpiredHandoffs was an indicator, not a count. It fired when an expired waiter was at index 0 or matched the returned connection's Setting, which is wrong in both directions: - undercount: an expired waiter behind a live one with age > maxAge was the actual pre-fix target, but the counter stayed at 0. - overcount: an expired waiter at the front incremented the counter even when a live Setting-match further down would have won under pristine v21. Reconstruct the pre-fix target in full instead, so the counter increments exactly iff the waiter that pristine v21 would have selected was expired. The pre-fix rule is "first waiter with age > maxAge || setting == connSetting, else the front element", so the predicate is evaluated on every element, in the original order, before any unlinking. This runs inside the existing scan under the existing lock: no second pass and no wider critical section. ctx.Err() is now read once per element and shared by the eviction and the accounting. No change to #238 behaviour: eviction and target selection are untouched. Tests: deterministic cases for both counterexamples plus an exhaustive check over every waitlist shape up to length 4 (333,450 cases) against an independent reference implementation of the pristine rule. All three fail on the previous heuristic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1f56db40-28ec-4335-97a1-b60deee78b07
release-21.0-github builds with go 1.23.10 (go.mod), but the backport of vitessio#19004 brought in test code written against the release-22.0 toolchain: - t.Context() (testing.T.Context, Go 1.24) - wg.Go(...) (sync.WaitGroup.Go, Go 1.25) - b.Loop() (testing.B.Loop, Go 1.24) This makes go/pools/smartconnpool fail to typecheck, which fails 'Static Code Checks Etc', every 'Unit Test' matrix job and 'Code Coverage' on this branch, and hides the results of the new expired-waiter tests added by this PR. Rewrite the four call sites with their Go 1.23 equivalents. No test behaviour changes: the contexts already carry their own timeout and cancel, and b.ResetTimer() was already being called before the loop. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CI status: what this PR broke vs. what was already brokenShort version: the one failure actually attributable to this PR is fixed (be4736c). Everything still red is pre-existing on Fixed here:
|
| Symbol | Requires |
|---|---|
t.Context() |
Go 1.24 |
wg.Go(...) |
Go 1.25 |
b.Loop() |
Go 1.24 |
go/pools/smartconnpool/pool_test.go:1297:42: t.Context undefined
go/pools/smartconnpool/pool_test.go:1331:6: wg.Go undefined
go/pools/smartconnpool/pool_test.go:1449:8: b.Loop undefined
FAIL vitess.io/vitess/go/pools/smartconnpool [build failed]
That failed Static Code Checks Etc, every Unit Test job and Code Coverage — and meant the new expired-waiter tests in this PR were never actually executed. #216 was merged with these same checks red, so this arrived with the base, not with this PR.
be4736c rewrites the four call sites with their Go 1.23 equivalents. No behaviour change: the contexts already carry their own timeout + cancel, and b.ResetTimer() was already called before the loop.
Verified locally against the real go1.23.10 toolchain:
go vet ./go/pools/smartconnpool/... ./go/list/... # clean
go test ./go/pools/smartconnpool/... ./go/list/... # ok
go test -race ./go/pools/smartconnpool/... ./go/list/... # ok
golangci-lint v1.60.2 run ... # exit 0
Result on CI:
Static Code Checks Etc— passok vitess.io/vitess/go/pools/smartconnpool 23.7s(was[build failed])ok vitess.io/vitess/go/list
Still red, all pre-existing on the base branch
I verified each of these by running them at base commit 865f789e15 with none of this PR's changes, and independently in #240, which touches only CI workflow YAML and contains zero Go changes — yet reproduces them all.
1. libtinfo5 404 — ~57 e2e jobs + Unit Test (mysql57) / (evalengine_mysql57)
dpkg-deb: error: 'libtinfo5_6.3-2ubuntu0.1_amd64.deb' is not a Debian format archive
Ubuntu superseded 6.3-2ubuntu0.1 with 6.3-2ubuntu0.2 and dropped the old file. The pinned URL now returns a 1807-byte HTML 404; curl -L -O (no --fail) writes it to disk and exits 0, then dpkg chokes on the HTML.
Fixed in #240. With that change these jobs install libtinfo5:amd64 (6.3-2ubuntu0.2) and run to completion — #240 went from 69 failures to 2. This PR will need base merged in once #240 lands.
2. planbuilder (10 cases) + semantics/TestCopySemanticInfoIntoColName
All introduced by 126279d2ea ("Fix query planning for complex queries with impossible conditions", from #204), which changed operators/subquery_builder.go and semantics/semantic_table.go and added 308 lines of select_cases.json expectations.
These are not stale-whitespace nits and should not be blindly regenerated. Regenerating testdata/expected/select_cases.json would silently bake in a planning regression:
- "OperatorType": "VindexLookup" (expected)
+ "OperatorType": "Route" (actual)
i.e. a lookup-vindex plan degrading to a plain Route. Other diffs drop a "Table" key and a "Type" key. This needs whoever owns that backport — out of scope for a connection-pool change, and untouched by it (semantics doesn't even import go/list or go/pools/smartconnpool).
3. Flaky / independently broken
Cluster (vtbackup)—TestTabletInitialBackup,TestTabletBackupOnly; also failing on Change connection pool idle expiration logic (#19004) #216 and on ci: bump libtinfo5 to 6.3-2ubuntu0.2 to unbreak dependency install #240go/mysql TestStaticConfigHUPgo/cmd/vttablet/cli— nil deref intopo.(*Lock).lockfrom a background goroutine
None of these touch the connection pool.
Summary
| Check | Cause | Status |
|---|---|---|
Static Code Checks Etc |
Go 1.23 incompat from #216 | fixed here |
smartconnpool build |
Go 1.23 incompat from #216 | fixed here |
~57 e2e + mysql57 unit tests |
libtinfo5 404 |
fixed in #240 |
planbuilder / semantics |
incomplete backport in #204 | pre-existing, needs owner |
vtbackup, TestStaticConfigHUP, vttablet/cli |
flaky / pre-existing | pre-existing |
Superseded by #241 — retargeted to
|
Problem
smartconnpoolcan hand a returned connection to a waiter whose connection-acquisition context has already expired. The waiter has no way to refuse it, so a scarce transaction slot is consumed by a request that can no longer use it.Production evidence
865f789e15165bfd506e75e6e4292e9874924780(this branch's base).helpers/vttablet-upomits--queryserver-config-txpool-timeout, sodefaultConfig.TxPool.Timeout = time.Secondapplies).InUse899.67 -> 10.75,ResourceExhausted0 -> 127,834,Activepinned at 900.WaitCount/WaitTimeare recorded on successful acquisitions only, so a recorded wait above the deadline means a waiter was served after its deadline had passed.Correctness invariant
Fix
Semantic backport of vitessio#20308, adapted to the v21 semaphore-based implementation.
tryReturnConnSlowchecks each waiter's context before handing over the connection and evicts expired ones.RemoveIfPresent—go/listgains an idempotent removal so a waiter being evicted by the returner while concurrently timing out itself cannot be double-removed.WaitCount/WaitTimesuccess-only semantics — the fix makeswaitForConnable to return(nil, nil), which previously could not happen.recordWaitis therefore guarded onconn == nilso the timeout path never pollutes the success-only metrics. Without this guard the fix would silently change the meaning of the very metric used as evidence above.Not included
--queryserver-config-txpool-waiter-capdoes not exist in v21)Validation
Base
865f789e15, Go 1.26.4.TestPostDeadlineHandoffon unpatched v21err=<nil> ctxErr=context deadline exceeded, WaitCount=1 WaitTime=202ms(connection handed to an expired waiter)TestPostDeadlineHandoffafter fixRESOURCE_EXHAUSTED,WaitCount=0 WaitTime=0sTestInteriorExpiredWaiterNotServedon unpatched v21live served=1 expired served=1TestInteriorExpiredWaiterNotServedafter fixlive served=1 expired served=0TestExpiredWaitersDoNotReceiveConnectionsTestExpiredWaiterEvictionIsRaceSafego/pools/smartconnpool,go/listok 24.552s/ok 0.493sok 46.723s/ok 1.684sHealthy-path handoff is unchanged: the added work is a context check per waiter examined, only on the return path, and the existing pool benchmarks/suite show no material change.
Incident causality
The production signature is strongly consistent with this defect and the behavior is reproducible on the deployed commit, but waiter-level telemetry was unavailable to prove incident causality conclusively.
Specifically not claimed: that root cause is confirmed, that vitessio#20308 caused 5229, or that every >1s acquisition was an expired handoff. A recorded wait above the deadline could also arise from scheduling delay after a pre-deadline assignment; source inspection alone cannot exclude that. The follow-up observability PR adds the counters that would settle it.
Upstream
vitessio#20308 — fixed upstream in June, present in v23 and v24 (verified in
v24.0.2source,go/pools/smartconnpool/waitlist.go).Strategic note
release-21.0is EOL. This backport is the immediate repair. The longer-term recommendation is migration to supported v24, which already contains this fix; that upgrade is tracked separately and is gated on gh-ost Online DDL strategy migration (v24 removed thegh-oststrategy).