smartconnpool: do not hand connections to expired waiters

[fasterrt]

Problem

smartconnpool can hand a returned connection to a waiter whose connection-acquisition context has already expired. The waiter has no way to refuse it, so the connection is consumed by a request that can no longer use it, and a live waiter is starved.

release-22.0-github is affected. go/pools/smartconnpool/waitlist.go is the same blob (40c924da32) on release-21.0-github, release-22.0-github and upstream release-22.0. Upstream fixed this in v23 and v24 only; there is no upstream v22 backport.

The defect reproduces deterministically on this branch's exact base (3a64a919f9):

--- FAIL: TestPostDeadlineHandoff
    POST-DEADLINE HANDOFF: pool delivered a connection to a waiter whose
    acquisition context expired 101.201708ms earlier.

Two must-preserve invariants

Both ship together. A future forward-port or backport must keep both.

Invariant A — a waiter already expired when examined is not selected

A waiter already expired when examined for selection must not be selected.

tryReturnConnSlow checks e.Value.ctx.Err() on every waiter it examines before it settles on a target, and removes the expired ones instead of leaving them eligible. Deadlines are heterogeneous, so expired waiters sit anywhere in the list, not only at the front.

Serving an already-expired waiter wastes the handoff: that waiter cannot use the connection, so the return makes no progress for anyone and the live waiters keep waiting.

Scope of that claim. This is a selection-time invariant. It does not claim that "no connection can ever be delivered to a context that is expired by the time the waiter wakes." A waiter that is live when selected can still have its context expire before it resumes. That post-selection expiry is inherent to any concurrent handoff, is indistinguishable from a caller cancelling a microsecond after acquisition, and is unchanged by this PR. The defect fixed here is narrower and unambiguous: the pool consulted a waiter it could already see was expired, and handed it the connection anyway.

Invariant B — waiter self-removal is O(1)

A waiter removing itself from the waitlist must not scan the waitlist while holding wl.mu.

This is not cleanup that rode along with Invariant A. It is load-bearing.

	before	after
work removed	up to n pointer-chasing iterations per waiter cancellation	one e.list identity check plus an unlink
serialized resource	wl.mu, the single mutex serializing every acquisition and return in the pool	same mutex, held for O(1)
complexity	O(n) per cancellation, so O(n²) across a timeout storm at depth n	O(1) per cancellation, O(n) across the storm
expected observable effect	mutex hold time on the cancellation path grows with waitlist depth	hold time independent of waitlist depth

The code being replaced walked the list to locate elem before removing it, on both waitForConn exit paths. This is a source-level complexity claim, not a measured production one.

Why both ship

They are independent properties of the same code path. Invariant A changes which waiter is selected but does nothing about the cost every timing-out waiter pays under wl.mu. Invariant B bounds that cost but would still let the pool hand connections to waiters that cannot use them. The upstream change contains both.

An out-of-tree overload harness — not part of this PR, and not production-equivalent — made the dependency concrete: under sufficient offered load the unfixed pool collapsed; A alone was worse than unfixed; B alone restored goodput; A+B restored goodput. Take that as a reason not to drop B, not as evidence about production behaviour.

A future backporter must not conclude that "the ctx check is the real fix and RemoveIfPresent is cleanup."

Reviewer focus

The production change is 3 files, +81 − 29 — of which only 29 added / 20 removed lines are code, the rest being comments. Four things deserve scrutiny; the rest of the diff is tests, comments and CI.

1. The tryReturnConnSlow loop (waitlist.go) — next is captured before Remove unlinks e; the front-most live waiter is retained as the fallback target; the break conditions are unchanged from pristine. TestLiveWaiterSelectionMatchesPristine passes on the unfixed base and here, which is what makes it evidence that live-waiter selection is untouched.
2. Eviction notification happens outside wl.mu, and after the handoff — deliberate, and the one place this PR diverges structurally from upstream v23, which notifies inline under the lock. Rationale for staying outside the lock: under mass expiry that loop is long, and wl.mu serializes the whole pool, so notifying inline would work against Invariant B. Rationale for ordering: the target is the only party with a deadline left to make, so its handoff must not queue behind N semaphore releases — an earlier revision of this PR had that backwards and was caught in adversarial review. Safety: both elements are unlinked under the lock and parked on their semaphores, so no other party can observe or recycle them before we notify.
3. The RemoveIfPresent ownership protocol — the returned boolean is the ownership token. Exactly one of {timing-out waiter, returner} sees true, and exactly one notify happens per waiter. Both production callers hold wl.mu; the helper does no synchronisation of its own.
4. maybeStarvingCount skipping expired waiters — this changes connection-opening decisions, so it is worth a look. It is identical to upstream v23.

Why this is a semantic port and not a cherry-pick

Between v22 and v23, upstream rewrote the waiter notification mechanism, so the upstream patch does not apply here:

	v21 / v22 (this branch)	v23 / v24 (upstream fix)
waiter wakeup	semaphore + helper goroutine + done chan	conn chan *Pooled[C] (buffered, cap 1)
waiter.ctx	already present	added by vitessio#20308
eviction signal	sema.notify(false), conn stays nil	conn <- nil
nil→error mapping	conn == nil guard in pool.go	waitResult() helper
wait field	value	pointer (alignment, because the struct grew)

Cherry-picking 459f4cfe21 conflicts in every hunk of waitlist.go and drags in the wait *waitlist[C] pointer + stack.go alignment commentary, which exist only because upstream's waiter struct grew — ours already has ctx, so it does not grow. Those hunks are deliberately not ported.

What is portable applies exactly: go/list/list.go is byte-identical between v22 and the upstream fix's parent, so RemoveIfPresent is taken verbatim.

Port scope: what is and is not identical between v21 and v22

The waitlist and list implementations relevant to the defect are byte-identical between the validated v21 base and v22, which is why the fix transplants without adaptation:

file	release-21.0-github (865f789e15)	release-22.0-github (3a64a919f9)
go/pools/smartconnpool/waitlist.go	40c924da32	40c924da32	identical
go/list/list.go	2ad837b7c6	2ad837b7c6	identical
go/pools/smartconnpool/pool.go	4eb6c4fa57	cca706bc1c	differs

pool.go is not byte-identical, and the difference is substantive rather than cosmetic: v22 adds idle-count handling — MaxIdleCount, idleCount, setIdleCount(), IdleCount() and closeOnIdleLimitReached.

That interaction was reviewed separately, because this fix touches its reachability. In tryReturnConn:

if pool.wait.tryReturnConn(conn) {
    return true
}
if pool.closeOnIdleLimitReached(conn) {   // v22-only
    return false
}

Before this change, an expired waiter would absorb the connection and wait.tryReturnConn would report true. After it, when every waiter is expired and no live waiter remains, the eviction path falls through and returns false — so closeOnIdleLimitReached becomes reachable in a case where it previously was not.

This is inert under GitHub's deployed/default configuration. defaultConfig.TxPool (go/vt/vttablet/tabletserver/tabletenv/config.go) sets only Size, Timeout and IdleTimeout, so MaxIdleCount takes the zero value, and setIdleCount() maps maxIdleCount == 0 to the pool capacity. The guard idle <= pool.idleCount.Load() therefore always holds on the first iteration and closeOnIdleLimitReached returns false without closing anything. When an operator does set --queryserver-config-txpool-max-idle-count, closing a genuinely idle connection when no live waiter wants it is the intended v22 behaviour, not a regression introduced here.

Fix

• tryReturnConnSlow evicts expired waiters as it examines them, so an interior expired waiter is removed rather than selected (the loop still stops at the existing age > maxAge || setting == connSetting condition)
• the front-most live waiter is the fallback target
• maybeStarvingCount no longer counts expired waiters as starving
• go/list gains O(1) RemoveIfPresent, replacing the O(n) scan a cancelling waiter performed under wl.mu (Invariant B), and giving the waiter and the returner an unambiguous ownership token
• WaitCount/WaitTime are restored to successful-acquisition-only semantics: waitForConn can now return (nil, nil), so recordWait is guarded on conn == nil (see the operator note below)

Operator note: metric semantics after this fix

After this fix, evicted/expired waiters no longer increment WaitCount/WaitTime. This restores those metrics to successful-acquisition semantics: pool.go returns ErrTimeout on the conn == nil guard before reaching pool.recordWait(start).

A decrease in WaitCount/WaitTime after rollout must not automatically be interpreted as reduced contention. Part of any observed drop is the removal of expired-waiter handoffs that were previously being recorded as successful acquisitions. Offered load and contention may be unchanged; the denominator changed. 比较 against ResourceExhausted/timeout rates and pool InUse before concluding anything about contention.

Accepted residual

With a live target, the handoff now happens before the eviction wakeups. In the all-expired case (target == nil) the connection is not stacked until tryReturnConnSlow returns, so the O(N) wakeup loop is charged synchronously to the returning goroutine (Pooled.Recycle → put → tryReturnConn), and a waiter enqueuing in that window waits it out.

Accepted, not fixed: it needs an extreme transient population of linked-but-expired waiters, the delay is proportional to work that must happen anyway, and the 100 ms starvation worker is a backstop. Upstream v23 is strictly worse here — it performs the same N wakeups while still holding wl.mu, blocking the whole pool rather than one returner.

Rollout contract

Deliberately small. This changes waiter selection and WaitCount/WaitTime semantics, and nothing else.

Canary success — all of:

signal	expectation
transaction goodput	flat or better vs. a control tablet
ResourceExhausted / txpool acquisition timeouts	no regression
TxPool liveness	no Active-high / InUse-single-digit divergence
connection lifecycle (Connections, dial rate)	flat — this path neither closes nor dials
CPU, goroutine count, RSS	no material regression
acquisition wait metrics	read only with the changed semantics below

Roll back if any of:

• transaction goodput drops on the canary tablet and not on control
• ResourceExhausted rises on the canary tablet and not on control
• TxPool InUse collapses while Active stays high — i.e. the liveness shape this change is meant to make less likely, appearing after rollout
• connection open/close rate rises materially — this change has no lifecycle path, so that would indicate an unmodelled interaction
• any panic, deadlock or stuck waiter in smartconnpool

Not a success signal: a drop in WaitCount/WaitTime. Part of any drop is definitional, because evicted waiters no longer record a wait. See the operator note above.

#239 adds waiter-level telemetry that would make the InUse/Active and waiter-population signals easier to read. It is not a dependency for this PR; if it lands first, treat it as additional evidence.

Validation

Every test below was run against pristine 3a64a919f9 (this PR's exact merge-base) and against this branch. Six are regression tests that fail on the unfixed base for the intended reason; one is a deliberate control that must pass on both.

Test	unfixed base	this PR	what it catches that nothing else does
TestPostDeadlineHandoff	❌ fail	✅ pass	Get() returning success after its acquisition deadline, and the success-only wait metric therefore exceeding that deadline
TestExpiredWaitersDoNotReceiveConnections	❌ fail	✅ pass	an expired waiter being served when it sits in the prefix of the list
TestInteriorExpiredWaiterNotServed	❌ fail	✅ pass	an expired waiter behind a live one being selected because its Setting matches the returned connection — the case a prefix-only eviction would miss
TestExpiredWaiterEvictionIsRaceSafe	❌ fail	✅ pass	the same invariant under real context.WithTimeout and the real returner/waiter race, rather than a pinned synthetic context
TestEvictionPreservesConnectionLifecycle	❌ fail	✅ pass	eviction silently becoming a lifecycle event: asserts the live waiter gets the same physical connection, with no extra dial, no close, and Active/Capacity unchanged
TestEvictedWaiterErrorIdentity	❌ fail	✅ pass	an evicted waiter observing a different error than a self-timing-out one; asserts the ErrTimeout sentinel itself and RESOURCE_EXHAUSTED
TestLiveWaiterSelectionMatchesPristine	✅ pass (control)	✅ pass	a regression in live-waiter selection. It passes on the unfixed base by design — that is what makes it evidence that FIFO, Setting preference and ageing are untouched
go/list TestRemoveIfPresentOwnership	n/a (helper is new)	✅ pass	the ownership token contract. Proven non-vacuous by mutation: making RemoveIfPresent always return true fails it. Nothing in smartconnpool asserts this directly, and breaking it would double-notify a waiter

Failure on the base is for the right reason, not merely a red result — e.g.:

--- FAIL: TestExpiredWaiterEvictionIsRaceSafe
    expired_waiter_test.go:252: Should be zero, but was 67
    Messages: expired waiter was handed a connection instead of being evicted

Ordering preserved. TestLiveWaiterSelectionMatchesPristine checks 4662 all-live waitlist shapes (lengths 1–4 × settings {nil,s1,s2} × ages {0,9} straddling maxAge=8) and asserts the fix picks the same waiter and leaves the same ages as the pre-fix rule. When no waiter is expired, this change is a behavioural no-op.

Behavioural equivalence with upstream. The four black-box tests in expired_waiter_test.go and post_deadline_handoff_test.go compile and run unmodified against upstream release-23.0 at 459f4cfe21 (which contains vitessio#20308) and all pass. expired_waiter_invariants_test.go is white-box and cannot compile there by construction — it reads waiter.conn, which is a *Pooled[C] field in v22 and a channel in v23. The goal is equivalence on the defect invariant, not source-level similarity.

Scope of the synthetic contexts. expiredCtx and lateExpiryCtx report Err() != nil without firing Done(). That pins the precondition — an expired waiter still linked when a returner examines it — so selection can be asserted deterministically instead of by winning a race. It proves what the pool does if that state is reached; it is not evidence about how often production reaches it. TestExpiredWaiterEvictionIsRaceSafe covers the same invariant with real contexts and the real race.

Suites: go/pools/... and go/list/... pass, under -race (×3) and under -race -cpu=1. go vet and gofmt clean.

Not included

• no waiter cap (the flag does not exist in this lineage; removed upstream in v20 by tablet: remove max-waiters setting vitessio/vitess#15323)
• no txpool timeout tuning
• no Launch admission control
• no unrelated post-v22 fixes

CI changes on this branch (not part of the fix)

The smartconnpool fix is the 6 files above. Every one is byte-identical to the reviewed head c60ea43e05 — git diff c60ea43e05 HEAD -- go/ is empty.

The branch also carries 6 CI-infrastructure commits touching 3 files and no Go code. All of them repair defects that were already failing this repository before this PR; none is conceptually part of the backport. They are here only because #241 cannot produce a valid CI signal without them.

commit	file	what it repairs	why it was needed here
514addf408	.github/actions/setup-mysql/action.yml	libtinfo5 6.3-2ubuntu0.1 → 0.2	the pinned .deb now 404s — Ubuntu dropped the superseded build from the pool, so Unit Test (mysql57) and (evalengine_mysql57) died at Setup MySQL
531f93e259	.github/workflows/vtop_example.yml	drop -retry=1	upstream removed the -retry flag in vitessio#19182; fork merge 234e15a386 resurrected the argument, so VTop Example aborted after 11 s with flag provided but not defined: -retry. Removing it restores byte-identical parity with upstream v22
712f7055bb	tools/get_previous_release.sh	match release-NN.0-github	the regex release-[0-9]*.0$ misses -github suffixes, so the upgrade/downgrade jobs resolved "previous release" to release-23.0 instead of release-21.0 and failed on the v22→v23 schema_change_signal→schema-change-signal rename
43df8a8dfa	.github/workflows/vtop_example.yml	runner selection	see sizing below
22362a104f, 550af21363	.github/workflows/vtop_example.yml	failure-only capacity dump	guardrail for the runner knob; 11 lines, if: failure()

VTop Example had never passed in this repository — it is failure on every non-cancelled run on release-22.0, release-23.0 and backport-18369-v22. It now passes in 8m43s.

VTop runner sizing — measured, and deliberately not overclaimed

runs-on is now:

runs-on: ${{ github.repository == 'vitessio/vitess' && 'oracle-vm-8cpu-32gb-x86-64' || vars.VTOP_RUNNER || 'ubuntu-latest-xl' }}

All three branches were verified by executing the expression in CI: upstream still resolves to oracle-vm-8cpu-32gb-x86-64; with a repository variable set the override wins; with it unset the fallback is ubuntu-latest-xl. Upstream behaviour is unchanged.

What the workload actually needs, sampled every 10 s across a full green run:

quantity	measured
peak scheduled CPU requests	4130 m
peak scheduled memory requests	9890 Mi
peak pod count	32
default ubuntu-24.04 allocatable	4000 m / 15.6 GiB

The default runner misses by 130 millicores (3.3 %). That is the entire failure: the kind node reached 3930m (98 %) allocated and two vtbackup-init pods could not be placed — 0/1 nodes are available: 1 Insufficient cpu. Memory was never a constraint (55 %).

So, stated precisely:

• actual workload requirement ≈ 4.2 CPU — any runner with ~5–6 CPU would schedule it;
• 16 CPU is not intrinsically required. ubuntu-latest-xl is simply the smallest label that both schedules in this organisation and has enough CPU.

Eight runner labels were tested. Only ubuntu-24.04/ubuntu-latest (4 CPU) and the -xl tier (16 CPU / 64 GB, both ubuntu-latest-xl and ubuntu-22.04-xl) ever schedule here; oracle-vm-8cpu-32gb-x86-64, gh-hosted-runners-16cores-1-24.04, ubuntu-latest-{4,8}-cores, ubuntu-latest-8-core, ubuntu-latest-l and ubuntu-latest-m stayed queued indefinitely and were cancelled. There is no intermediate tier to pick — the jump is 4 → 16.

Cost. Per PR, VTop went from 4 CPU × ~34 min = ~138 CPU-min and a guaranteed red result, to 16 CPU × ~11 min = ~176 CPU-min for a green one: ≈1.28× the CPU-minutes for a signal that previously did not exist. If a ~6–8 CPU label is ever exposed to this org, set the repository/organisation variable VTOP_RUNNER to it and the cost drops with no source change. That hook is the intended cost-control mechanism.

Extraction. All 3 CI files are repo-wide fixes that every PR in this fork benefits from. They should be lifted into a single follow-up CI PR against release-22.0-github after this merges; #241 can then drop them. They are retained here only because without them this PR has no trustworthy CI signal — not because they belong to the backport.

Relationship to #238

#238 is the same fix targeting release-21.0-github. waitlist.go and go/list/list.go are byte-identical between the two branches — the fixed waitlist.go blob is 6850a54632 on both — so the defect-relevant production code is the validated v21 artifact unchanged. pool.go is not byte-identical between v21 and v22 (see Port scope above); the hunk this PR applies to it is textually the same, but it lands on a different surrounding file. #238 could not be retargeted: its branch is rooted in v21, so repointing the base at release-22.0-github renders as 222 commits / 740 files / +20,234 −7,607 instead of the 6-file smartconnpool change here.

Incident context

Production evidence from incident 5229 is on #238. The confidence statement is unchanged:

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.

Upstream

vitessio#20308, backported upstream as vitessio#20354 (v23) and vitessio#20355 (v24).

[Copilot]

Copilot review overview

Review tier: Balanced
Findings: 2 · 1

新建议题s introduced by this change (3)

Severity	Finding
	go/​pools/​smartconnpool/​expired_waiter_test.go — This test can pass without exercising the eviction/removal race at all. The pool starts with two…
	go/​pools/​smartconnpool/​post_deadline_handoff_test.go — This polling loop has no timeout, so a regression that prevents the live waiter from enqueueing…
	go/​list/​list.go — The documentation implies this method can safely race with another goroutine, but e.list and the…

What changed in this PR

Prevents smartconnpool from assigning returned connections to expired waiters while preserving routing, metrics, and connection lifecycle behavior.

Changes:

• Evicts expired waiters safely during connection selection.
• Maps eviction to the existing timeout behavior.
• Adds extensive regression and invariant tests.

File	Description
go/​pools/​smartconnpool/​waitlist.go	Evicts expired waiters and excludes them from starvation counts.
go/​pools/​smartconnpool/​pool.go	Handles nil eviction results as timeouts.
go/​list/​list.go	Adds conditional O(1) removal.
go/​pools/​smartconnpool/​post_deadline_handoff_test.go	Tests deadline and interior-waiter handoffs.
go/​pools/​smartconnpool/​expired_waiter_test.go	Tests eviction behavior and concurrency.
go/​pools/​smartconnpool/​expired_waiter_invariants_test.go	Verifies lifecycle, errors, and ordering invariants.

Suppressed comments (1)

go/pools/smartconnpool/post_deadline_handoff_test.go:203

• This second enqueue wait is also unbounded; if the expired waiter exits early or never reaches the list, the test hangs forever. Make this wait bounded so the failure is reported deterministically.

	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.

[fasterrt]

Confirmed, and the defect was worse than described. Fixed in b4577f2.

Two independent reasons the test could pass vacuously:

1. Free slots. As you say, Capacity: 2 was never occupied, so a serialized run satisfied every Get immediately and the closing waiting() == 0 assertion succeeded with zero waiters ever enqueued.
2. The contexts could not reach the eviction path. The expiring group used context.WithTimeout. A real context closes Done on expiry, which wakes its own waiter, and that waiter then removes itself in waitForConn. The pool never evicts it. The eviction path was therefore only reachable in the scheduling gap between deadline expiry and wakeup. Only a context that becomes expired without firing Done (the existing lateExpiryCtx helper) leaves the element for tryReturnConnSlow to evict.

The test now does all four things you asked for:

• occupies both slots before launching workers, so every worker is forced onto the waitlist;
• synchronizes with require.Eventually(waiting() == workers) before proceeding, which is deterministic because nothing can leave the list until the gate closes;
• overlaps returns with cancellation by closing cancelGate and putting the held connections back together;
• asserts the eviction path actually ran, via evictedServed == 0 and liveServed == workers/3.

Measured against the unfixed base 3a64a919f9:

	old test	new test
vs unfixed pool	PASS 5/5	FAIL, 67 of 67 expired waiters served

So the old test could not have caught the regression it was named for. Also verified -race 5x and -cpu=1 3x (your serialized scenario) on the fixed code.

[fasterrt]

Agreed, fixed in b4577f2. Both loops now use require.Eventually with a 30s bound and the original 200µs tick.

Worth noting the failure mode you avoided: the package-level -timeout would eventually fire, but it kills the whole package with a goroutine dump rather than failing this test at a named assertion, so the real regression would be buried.

Your line list was exactly right: 186 and 201 were unbounded, and line 95 is not affected because it already carries a require.True(t, time.Now().Before(deadline)) guard inside the loop. Left that one alone. This also brings the file in line with expired_waiter_test.go and the CLAUDE.md testify convention.

[fasterrt]

Correct, the wording was misleading. Fixed in b4577f2 (comment only).

RemoveIfPresent calls the same unexported remove, which mutates e.list, e.next and e.prev as ordinary fields. It gives no synchronization; the old phrasing "may race with another goroutine removing e" invited exactly the lock-free misuse you describe. The only thing it actually buys a caller is skipping a membership scan.

新建 wording states that it saves the caller a membership scan and nothing more, that it performs no synchronization of its own, that callers must serialize every mutation externally as waitlist does with its mutex, and that under that lock the return value is what tells two contenders which of them owns e.

Verified comment-only: every changed line in list.go is a // line and no //go: directive is touched, so codegen is unaffected.

[fasterrt]

CI triage — all 5 failures are pre-existing / environmental, none caused by this PR

Totals: 100 pass / 5 fail / 2 skipping. Every job that compiles and runs this code passes, including Unit Test (Race).

Job	Verdict	Evidence
Unit Test (mysql80) / (mysql84) / (evalengine_mysql84) / Unit Test (Race) / Static Code Checks Etc / Code Freeze	✅ pass	These build and exercise go/pools/smartconnpool + go/list, including under -race.
Unit Test (mysql57), Unit Test (evalengine_mysql57)	pre-existing, environmental	Fail at step 10 Setup MySQL; steps 11–14 (Get dependencies, make tools, Run test) are skipped — no Go code is ever fetched or built, so a Go source change cannot influence the result. Every recent run of unit_test_mysql57.yml in this repo fails across unrelated branches; last success was 2025-09. MySQL 5.7 is EOL and its apt repos are retired.
VTop Example	pre-existing	Fails on every recent run of vtop_example.yml repo-wide, including release-22.0, release-23.0, and backport-18369-v22 — the branch lineage that produced this PR's base commit 3a64a919f9. No successful run in the visible history.
Run Upgrade Downgrade Test - Query Serving (Queries) and (Queries - 2)	pre-existing, version-skew	See below.

Upgrade/Downgrade: v22↔v23 CLI flag-rename skew

Both shards fail at step 20 Run query serving tests (vtgate=N, vttablet=N-1, vtctld=N-1) with:

Error: unknown flag: --schema-change-signal
Error: unknown flag: --mysql-auth-server-static-file
Error: unknown flag: --mysql-server-query-timeout

These flags were renamed from snake_case to kebab-case between v22 and v23:

• v22 go/vt/vtgate/vtgate.go:196 — fs.BoolVar(&enableSchemaChangeSignal, "schema_change_signal", ...)
• v23 go/vt/vtgate/vtgate.go:197 — utils.SetFlagBoolVar(fs, &enableSchemaChangeSignal, "schema-change-signal", ...)

The v23 test tree compensates via vtutils.GetFlagVariantForTestsByVersion(...); the v22 tree has no such shim. This is a harness/flag-naming incompatibility in go/test/endtoend/vtgate/queries/union, entirely orthogonal to connection pooling.

Two independent facts rule this PR out as the cause:

1. Step 18 Run query serving tests (vtgate=N-1, vttablet=N, vtctld=N) — the step where vttablet is built from this commit — PASSES. The failing step 20 runs vttablet/vtctld built from release-23.0, which does not contain this patch (and already contains the upstream smartconnpool: don't hand returned connections to expired waiters vitessio/vitess#20308 fix).
2. This PR changes no CLI surface. The diff is 6 files confined to go/list/list.go and go/pools/smartconnpool/{pool.go,waitlist.go} plus three test files; it adds zero flag registrations (grep -c '^\+.*\(flag\.\|Flags()\)' over the full diff = 0).

The same job also fails on the base-branch lineage (backport-18369-v22, 2026-06-04), there at step 18 — i.e. the shard is unstable in both directions independently of this change.

[fasterrt]

VTop Example — infra-limited, not caused by this PR

This is the only remaining red check (103 pass / 1 fail). It is not related to the smartconnpool backport.

Root cause: runner capacity.

# .github/workflows/vtop_example.yml:17
runs-on: ${{ github.repository == 'vitessio/vitess' && 'oracle-vm-8cpu-32gb-x86-64' || 'ubuntu-24.04' }}

The 8-CPU/32 GB machine is granted only when github.repository == 'vitessio/vitess'. In this fork the expression falls back to a standard ubuntu-24.04 hosted runner (confirmed from the job API: labels=["ubuntu-24.04"], group="GitHub 操作").

The example runs a single-node kind cluster (kind create cluster, no config), which then has to host the full 8-tablet resharding topology. It can't: both *-vtbackup-init pods stay Pending, so no seed backup is created, so the new-shard vttablets never leave 2/3 Running, and the run times out.

Evidence it is environmental, not code:

Check	Result
Upstream release-22.0 runs of this workflow	8/8 success, 10-14 min
This fork	timeout at 32-36 min (~3x slower => smaller machine)
test/vtop_example.sh vs upstream release-22.0	byte-identical
examples/operator/ vs upstream	byte-identical
Whole workflow delta vs upstream	quote style + one setup-go cache: line

This PR is excluded as a cause: a pod stuck in Pending has never started a container, so it never executed a Vitess binary — the pool change cannot influence it.

Re-running does not help; it failed identically on a second attempt (36m 14s). It is a deterministic capacity shortfall.

_{Note: this job had 0 successes in its last 100 runs. It was hard-blocked much earlier by an invalid -retry=1 flag (removed in 531f93e), which had masked this failure mode.}

[fasterrt]

Scope & cost hardening pass — evidence

Every non-core change on this branch was re-tested for necessity, not retained because CI was once red.

change	necessity evidence	needed for #241 CI today?	belongs in #241 conceptually?
libtinfo5 0.1→0.2	libtinfo5_6.3-2ubuntu0.1_amd64.deb returns HTTP 404 and is absent from the pool index; …0.2… returns 200. Block is inside the mysql-5.7 branch only, so it affects exactly the two mysql57 jobs.	YES	NO
drop -retry=1	grep -c retry test.go = 0 at the v22 base — the flag does not exist. Job aborted at 11 s. Removal restores byte-identical parity with upstream v22.	YES	NO
get_previous_release.sh	Executed both regexes against the real github.base_ref: release-22.0-github → old release-23.0 (wrong, causes the v22↔v23 schema_change_signal rename failure) vs new release-21.0. For release-22.0 and main old and new are identical, so the change is a no-op outside -github refs.	YES	NO
VTop runner	Peak scheduled CPU 4130 m vs ubuntu-24.04 allocatable 4000 m. Cross-checked by the failing run (3930 m = 98 % allocated, 2 pods Insufficient cpu).	YES	NO
failure diagnostics	NOT required for green CI. Retained as the guardrail for the VTOP_RUNNER knob and minimized 28 → 11 lines in 550af21363; a broken sub-block (kubectl get pods -o name emits no namespace → a resource cannot be retrieved by name across all namespaces) was deleted.	no	no

Runner sizing is deliberately not overclaimed. Requirement ≈ 4.2 CPU; ubuntu-latest-xl (16 CPU) is merely the smallest label that schedules in this org. Eight labels were tested — only the 4-CPU and 16-CPU tiers exist here (oracle-vm-8cpu-32gb-x86-64, gh-hosted-runners-16cores-1-24.04, ubuntu-latest-{4,8}-cores, -8-core, -l, -m all stayed queued and were cancelled). Set VTOP_RUNNER to any ~6–8 CPU label if one is ever exposed — no source change needed. All three branches of the runs-on expression were executed in CI: upstream → oracle-vm-8cpu-32gb-x86-64, override → wins over fallback, unset → ubuntu-latest-xl.

Unit Test (mysql57) is flaky, not broken by this PR. With byte-identical Go code (git diff 43df8a8dfa 550af21363 touches only vtop_example.yml): pass → fail (~15 tests in vplayer_flaky_test.go) → fail (a different single test, TestVStreamsMetrics) → pass. Different failure sets across attempts ⇒ nondeterministic. mysql80, mysql84 and Race were green throughout. This job had been dead at Setup MySQL for months, so the libtinfo5 repair revived it and exposed long-hidden MySQL 5.7 flakiness — it did not create it.

Core untouched: git diff c60ea43e05 550af21363 -- go/ is empty.

关注-up: the 3 CI files are repo-wide repairs and should be lifted into one CI PR against release-22.0-github after this merges; #241 can then drop them. They are retained here only so this PR has a trustworthy signal.

[fasterrt]

Copilot review findings addressed — b4577f263a

All 3 findings (+1 suppressed duplicate) were valid and are fixed. No production logic changed: pool.go and waitlist.go are byte-identical to the reviewed head 550af21363, and list.go is comment-only (every changed line is a // line, no //go: directives).

Medium — expired_waiter_test.go: test could pass vacuously

Valid, and the defect had a second cause the review did not name:

1. Capacity: 2 was never occupied, so a lightly loaded run satisfied every Get immediately, enqueued no waiters, and the closing waiting() == 0 assertion passed vacuously.
2. The expiring group used context.WithTimeout. A real context closes Done on expiry, waking its own waiter, which then removes itself in waitForConn. The pool never evicts it. The eviction path under test was therefore only reachable in the scheduling gap between deadline expiry and wakeup.

Now: both slots are held until every worker is parked; require.Eventually(waiting() == workers) gates the race deterministically; the expiring group uses lateExpiryCtx (expires without firing Done) so the pool must evict it; self-removal and eviction are deliberately overlapped by closing the cancel gate and returning the held connections together; and it asserts evictedServed == 0 and liveServed == workers/3.

Regression-detection proof against the unfixed base 3a64a919f9:

	vs unfixed pool
old test	PASS 5/5 — caught nothing
new test	FAIL — 67 of 67 expired waiters served

Also verified on the fixed code: -race 5x, and -cpu=1 3x, which is the serialized scenario raised in the review.

Medium — post_deadline_handoff_test.go: unbounded polling loops

Both loops (186, and 201 from the suppressed comment) now use require.Eventually with a 30s bound and the original 200µs tick. The package-level -timeout would have fired eventually, but it kills the whole package with a goroutine dump rather than failing at a named assertion, so the real regression would be buried. Line 95 was correctly not flagged: it already carries a require.True(t, time.Now().Before(deadline)) guard inside the loop.

Low — list.go: RemoveIfPresent doc implied race safety

Correct. It calls the same unexported remove, which mutates e.list, e.next and e.prev as ordinary fields. The doc now states that it only saves the caller a membership scan, that it performs no synchronisation of its own, that callers must serialise every mutation externally as waitlist does with its mutex, and that under that lock the return value is what tells two contenders which of them owns e.

CI

104 pass / 0 fail / 2 skipping, MERGEABLE/CLEAN.

One transient Unit Test (mysql84) failure was investigated rather than waved away. go/vt/wrangler hit its 10-minute package timeout in TestTableMigrateJournalExists (9m57s); go/pools/smartconnpool passed in 25.6s and go/list passed in that same job. This commit cannot affect that package: smartconnpool production code is byte-identical, list.go is comment-only, and Go does not compile one package's _test.go files into another package's test binary. The re-run on the identical SHA passed.