Uh oh!
There was an error while loading. Please reload this page.
Fix session pool lifecycle bugs in getSession/releaseSession/cleanupIdleSessions - #19
Conversation
…dleSessions Several related lifecycle bugs in BaseSessionPool where session bookkeeping (pool / idleSessions / activeSessions / waitQueue) was updated inconsistently across await points: - A timed-out getSession() waiter was never removed from the wait queue: the timeout matched indexOf(resolve), but the queue holds a wrapper closure, so the match always failed. releaseSession() later shifted that dead waiter, marked the session active, and resolved an already-rejected promise, leaking the session and eventually starving the pool. - The create-new-session branch reclaimed the freshly created session with a blind idleSessions.shift() (front), but createSession() pushes to the back; under interleaving (a session released into idle during the await) it evicted a different session and left the new one tracked as both idle and active. - The idle-reuse branch added the session to activeSessions but never set inUse=true (the new-session and waiter branches both do), so syncDatabaseContextToPool treated an actively-in-use session as idle and could issue USE on it concurrently with the caller's in-flight request. - cleanupIdleSessions() checked pool.length > minSize against the constant pre-cleanup size, so it could queue every idle session and shrink the pool below minPoolSize. - cleanupIdleSessions() awaited session.close() before removing the session from pool/idle; since isOpen() stays true until the close RPC resolves, a concurrent getSession() could hand out a session that was being destroyed. Fix each (named/settled-guarded waiter with correct removal + release loop; targeted idle removal; inUse on reuse; projected-size cleanup guard; splice-before-close) and add deterministic regression tests. Signed-off-by: Zihan Dai <99155080+PDGGK@users.noreply.github.com>
CritasWang
left a comment
There was a problem hiding this comment.
Did a deep pass over this against develop (21b9432), including async-interleaving analysis of the waiter/settled machinery under "timeout fires first", "release fires first", and both racing. All five bugs reproduce on the base branch as described, and each fix checks out:
- Dead-waiter leak — confirmed
indexOf(resolve)is always-1since the queue stores the wrapper closure. The namedwaiter+settledflag handles all three orderings correctly, including the edge where a timer callback is already in the ready queue whenclearTimeoutruns. - Blind
shift()in the create branch — confirmed wrong-eviction under interleaving; targeted removal is correct. - Missing
inUse = trueon idle reuse — verified the real-world impact:TableSessionPool.syncDatabaseContextToPoolfilters on!inUseand would fire a concurrentUSEon an in-flight session. - minPoolSize guard — projected-size guard is correct; the loop no longer queues every idle session.
- Close-before-remove TOCTOU — the splices run synchronously before the first
awaitin the map callback, so the window is closed.
The new test file is well done: gate-based deterministic interleaving, no real timers/servers, each case fails on base and passes here.
A few non-blocking observations in the inline comments (a pre-existing create-branch race worth a follow-up issue, draining waiters on close(), and the close-failure orphan trade-off). One more pre-existing nit not worth an inline thread: minPoolSize: 0 is coerced to 1 by an || 1 default elsewhere, so a true zero-min pool isn't currently expressible (the tests already work around this).
Overall this looks mergeable to me.
| // Remove *this* session from idle. createSession() pushed it to the | ||
| // back of idleSessions; a blind shift() removes the front, which under | ||
| // concurrent interleaving (a session released into idle while we were | ||
| // awaiting createSession) would evict a different session and leave | ||
| // this one double-tracked as both idle and active. | ||
| const idleIndex = this.idleSessions.toArray().indexOf(pooledSession); | ||
| if (idleIndex > -1) { | ||
| this.idleSessions.remove(idleIndex, 1); | ||
| } |
There was a problem hiding this comment.
The targeted removal is correct and fixes the wrong-eviction bug. While verifying it I noticed a related pre-existing race that this PR neither introduces nor fixes (out of scope, flagging for a follow-up issue):
createSession() pushes the new session into idleSessionsbefore it resolves, so between that push and this removal there is a microtask window in which a concurrent getSession() can grab the same session via the idle-reuse branch — two callers would then share one connection. A cleaner long-term shape might be for the create branch to claim the new session directly without routing it through idleSessions at all. Happy to open a separate issue for it so this PR stays focused.
| let settled = false; | ||
| // The queue stores this exact wrapper. On timeout we must remove *this* | ||
| // reference (not `resolve`, which is never in the queue), and the | ||
| // `settled` guard makes timeout and fulfillment mutually exclusive so a | ||
| // session is never handed to a waiter whose promise already rejected. | ||
| const waiter = (session: Session): boolean => { | ||
| if (settled) { | ||
| return false; | ||
| } | ||
| settled = true; | ||
| clearTimeout(timeoutId); | ||
| const duration = Date.now() - startTime; | ||
| logger.debug(`[PERF] getSession (waited): ${duration}ms`); | ||
| resolve(session); | ||
| return true; | ||
| }; |
There was a problem hiding this comment.
The settled guard on both sides (waiter and timeout callback) is the right call — clearTimeout can't cancel a timer whose callback is already in the ready queue, so guarding only one side would leave a small double-settle window. Nice.
One adjacent gap (pre-existing, not introduced here): close() doesn't reject pending waiters, so a caller parked in this queue when the pool shuts down only fails after the full waitTimeout. Since this PR already gives waiters a settled flag, draining the queue in close() (settle + reject each pending waiter) would be a natural small follow-up.
| sessionsToRemove.map(async (ps) => { | ||
| // Remove from pool + idle BEFORE closing. close() awaits the | ||
| // closeSession RPC and isOpen() stays true until it resolves, so a | ||
| // concurrent getSession() could otherwise shift() this session and | ||
| // hand out a connection that is about to be destroyed. | ||
| const poolIndex = this.pool.indexOf(ps); | ||
| if (poolIndex > -1) { | ||
| this.pool.splice(poolIndex, 1); | ||
| } | ||
| const idleIndex = this.idleSessions.toArray().indexOf(ps); | ||
| if (idleIndex > -1) { | ||
| this.idleSessions.remove(idleIndex, 1); | ||
| } | ||
| try { | ||
| await ps.session.close(); |
There was a problem hiding this comment.
Splice-before-close correctly closes the TOCTOU window. One side effect worth being aware of (acceptable trade-off, no change requested): if close() throws, the session has already been removed from pool/idleSessions, so it ends up untracked and possibly not fully closed. That's the standard connection-pool trade-off — better an orphaned close-failed session than handing out a dying one — but a comment noting it, or a best-effort session.close() retry in the catch, wouldn't hurt.
PDGGK
commented
Jul 23, 2026
Thanks for the deep pass @CritasWang — I appreciate you reproducing each case against
Let me know if you'd prefer one tracking issue for these or separate ones. |
CritasWang
commented
Jul 24, 2026
+1 |
Uh oh!
There was an error while loading. Please reload this page.
This is the pool-lifecycle PR (findings 1-5) from the dev@ discussion "[DISCUSS] Hardening the iotdb-client-nodejs session pool / connection lifecycle", where the findings and this two-PR split were reviewed.
Problems
Five related lifecycle bugs in
BaseSessionPoolwhere session bookkeeping (pool/idleSessions/activeSessions/waitQueue) is updated inconsistently acrossawaitpoints and on the timeout/cleanup paths.1. Timed-out waiter leaks a session → pool starvation. The wait-timeout handler removed the stale waiter with
waitQueue.indexOf(resolve), but the queue stores a wrapper closure, notresolve, soindexOfis always-1and the timed-out waiter is never removed. A laterreleaseSession()then shifts that dead waiter, marks the session active, and resolves an already-rejected promise — leaking the session and eventually starving the pool.2. Create-branch evicts the wrong idle session under interleaving. After
await createSession()(which pushes the new session to the back of idle), the branch did a blindidleSessions.shift()(front); under interleaving (a session released into idle during the await) it evicted a different session and left the new one tracked as both idle and active.3. Idle-reuse branch never marks the session in use. The idle-reuse path adds the session to
activeSessionsbut omitsinUse = true(the new-session and waiter branches both set it), sosyncDatabaseContextToPool(which filters!inUse) treats an actively-in-use session as idle and can runUSE <db>on it concurrently with the caller's in-flight request.4.
cleanupIdleSessions()can shrink the pool belowminPoolSize. The candidate guard checkspool.length > minSizeagainst the constant pre-cleanup size, so it can queue every idle session and collapse the pool to 0.5.
cleanupIdleSessions()can hand out a session being closed. It awaitedsession.close()before removing the session from pool/idle; sinceisOpen()stays true until the close resolves, a concurrentgetSession()could shift and hand out a session that is being destroyed.Fixes
waiterreturning boolean, guarded by asettledflag; the timeout removes that exactwaiter;releaseSession()loops over waiters skipping already-settled ones and falls back to idle.indexOf+remove) notshift().inUse = true.pool.length - sessionsToRemove.length > minSize).close().Tests
tests/unit/BaseSessionPoolLifecycle.test.ts(new) reproduces all five with a fake-session subclass; each fails on currentdevelopand passes with this change. Fulltests/unitgreen (159 tests).