fix(pool): prevent num_idle underflow that wedges the maintenance task#5
Merged
Merged
Conversation
Cherry-pick of transact-rs#4289 onto the madsim v0.8.2 branch. PoolInner::release() published a returned connection (push to the idle queue + release its semaphore permit, which wakes waiters) before incrementing num_idle. A concurrent pop_idle could pop that connection and decrement num_idle while it was still 0, wrapping the usize to usize::MAX. The maintenance task's `for _ in 0..num_idle()` sweep then spins ~forever inside a single non-yielding poll, pegging a worker; on a single-connection pool one of its add_permits handoffs strands the permit on a waiter parked in the spinning worker's LIFO slot, permanently wedging the pool (RisingWave meta silent hang, risingwavelabs/risingwave#24991; upstream report transact-rs#3645). Three-layer fix, each independently sufficient: 1. release(): increment num_idle before publishing the connection, so the counter provably never goes below 0 (subs <= pops <= pushes <= adds) and never exceeds max_connections. 2. pop_idle(): saturating decrement, so even a future ordering bug can only over-count, never wrap. 3. maintenance sweep: cap iterations at max_connections. Includes a 12-thread stress test over the real try_acquire/release paths (fails in <1s with observed=usize::MAX before the fix). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cherry-pick of transact-rs#4289 (unmerged upstream) onto
madsim-v0.8.2, fixing thenum_idleunderflow that RisingWave root-caused as the source of its CI silent meta hang (risingwavelabs/risingwave#24991, full RCA: risingwavelabs/risingwave#24991 (comment)). Same bug reported upstream as transact-rs#3645 ("100% CPU inspawn_maintenance_tasks").The bug
PoolInner::release()published a returned connection before updating the idle counter:idle_conns.push(conn)guard.release_permit()— this wakes a queued waiter, which can immediately pop the connection on another corenum_idle.fetch_add(1)A
pop_idleracing between 2 and 3 runsnum_idle.fetch_sub(1)while the counter is still 0 → wraps tousize::MAX. The counter self-heals microseconds later, but the maintenance task's sweep uses it as a loop bound:The sweep then spins inside a single non-yielding poll (~forever;
try_acquireconsumes no coop budget so tokio cannot preempt it), pegging a worker. On a single-connection pool (RisingWave's SQLite meta store) one of the spin'sadd_permitshandoffs assigns the only permit to a queued waiter whose wakeup lands in the spinning worker's LIFO slot — unstealable on tokio 1.49 (tokio-rs/tokio#4941) — permanently wedging the pool.Observed in production captures: thread dumps catch the reaper mid-
try_acquireminutes into total quiescence; an isolation repro observes theusize::MAXread within 11 ms of hammering.The fix (three layers, each independently sufficient)
release(): incrementnum_idlebefore publishing the connection. Invariant: every successful pop is preceded by its push, which is preceded by its increment, sosubs ≤ pops ≤ pushes ≤ addsat all times ⇒ the counter can provably never go below 0 (and never exceedsmax_connections). The transient over-count (incremented, not yet pushed) is harmless: a racingpop_idlejust finds an empty queue and returns the permit.pop_idle(): saturating decrement — a future ordering regression can then only over-count, never wrap.max_connections, so even a corrupt counter cannot spin the non-yielding loop unboundedly.Test
Port of the upstream stress test to the v0.8.2
AnyAPI: 12 OS threads × 200k realtry_acquire/releasecycles on amax_connections = 3pool, samplingnum_idle()after every op and asserting the high-water mark never exceedsmax_connections(the bound is exact: legitimate transient over-count is still ≤ total connections, so the assert cannot flake).fetch_sub): fails in ~0.2 s withsaw 18446744073709551615— the exact production signature.sqlx-coresuite green (cargo test -p sqlx-core --features any,_rt-tokio, 30 passed).Deviations from the upstream test:
idle_timeout(None)/max_lifetime(None)/min_connections(0)so pool creation spawns no maintenance task and the test needs no async runtime; cfg-gated on a runtime feature because v0.8.2'sAsyncSemaphore::newpanics without one.RisingWave will bump its pinned rev to pick this up.