Hi Xuan, Thanks for going through all seven against develop so quickly, and for the concrete direction on the redirect design.
On 1-6, your suggested fixes match what I have in the regression branches: 1 is a named waiter guarded by a settled flag with the timeout removing that exact reference; 2 removes the specific pooledSession from idle instead of shift(); 3 is the one-line inUse = true in the reuse branch; 4 subtracts the already-queued removals from the guard; 5 splices out of pool/idle before awaiting close(); 6 mirrors close()'s teardown (removeAllListeners + destroy) in the catch. I'll open the two PRs (pool lifecycle 1-5, connection leak 6) referencing this thread, each with the tests that fail on develop and pass with the fix. On 7, I agree option (a) is the cleaner contract - a redirect-owned session's lifecycle is tied to its endpoint, and mixing it into the general pool is what creates the double-ownership. I'll follow the shape you laid out: getSessionForEndpoint stops pushing into pool/idleSessions, release of a redirect-owned session routes back to endPointToSession (marked idle there) instead of the general idle queue, and I'll track redirect-owned sessions in a separate set so close()/cleanup still reach them. I'll send that as a follow-up PR once 1-6 land, to keep the diff focused. On the mcp-server line - you're right that it doesn't belong here; it was only meant as context that the same pooled-return pattern appears elsewhere. It's a separate, tree-mode-only issue (a pooled session returned with close() instead of put_back()), and I'll raise it on its own rather than mixing it into this thread. Thanks again for the review - I'll get the PRs up. Best, Zihan Dai On Wed, Jul 22, 2026 04:51 PM, 王旋 <[email protected]> wrote: > Hi Zihan, > Thank you for this thorough audit — these are genuinely subtle concurrency > bugs, and I appreciate that you've reproduced each one with tests and > prepared fixes. I've gone through all seven findings against the current > develop code, and they all check out. Some inline notes: > > *Finding 1 (waiter never removed on timeout):* Confirmed. The > waitQueue.toArray() returns wrapper closures, but the timeout handler > searches for the raw resolve function — so indexOf(resolve) is always -1, > the wrapper stays in the queue forever, and a later releaseSession feeds > the session to a dead waiter. The fix should either store a reference to > the wrapper or use a different bookkeeping scheme for timeout cleanup. > > *Finding 2 (blind shift() after await createSession):* Confirmed. > createSession() pushes to the *back* of idleSessions, but after the await, > getSession does this.idleSessions.shift() from the *front*. A session > released during the await lands at the front and gets evicted. The fix > should find and remove the specific newly-created pooledSession rather than > blindly shifting the front. > > *Finding 3 (idle-reuse branch missing inUse = true):* Confirmed. The fast > path at line 235-245 of BaseSessionPool.getSession() never sets > pooledSession.inUse > = true. This means TableSessionPool.syncDatabaseContextToPool — which > filters on !ps.inUse — can issue USE <db> on a session that is currently > serving another caller's query. The fix is a one-liner: add > pooledSession.inUse > = true after the isOpen check in the idle-reuse branch. > > *Finding 4 (minPoolSize guard evaluated once pre-cleanup):* Confirmed. > The this.pool.length > > minSize check in the for-loop is evaluated against the pre-cleanup size. > If the pool has 10 sessions, minSize is 5, and 6 are idle-timed-out, all 6 > pass the guard (10 > 5) and get removed, leaving the pool at 4. The fix > should re-evaluate the pool size after each removal, or pre-calculate how > many to keep. > > *Finding 5 (close before idle-queue removal):* Confirmed. > cleanupIdleSessions awaits ps.session.close() before removing from > idleSessions. Between the close completing and the remove, a concurrent > getSession() can shift the closed session from the idle queue. The fix > should remove from the idle queue *first*, then close. > > *Finding 6 (Connection.open socket leak):* Confirmed. The catch block in > Connection.open() rethrows without destroying the TCP connection or > removing listeners. Each failed connect attempt leaks one socket. The fix > should add this.connection.destroy() + removeAllListeners() in the catch > block. > > *Finding 7 (redirect sessions shared between endpoint cache and general > pool):* Confirmed. getSessionForEndpoint pushes into this.pool (line 217), > and releaseSession pushes into this.idleSessions (line 326/330), so a > session created for a specific redirect endpoint can later be handed out by > a plain getSession() — while simultaneously being used by a redirect-path > caller via endPointToSession. Two different callers could drive the same > Thrift socket concurrently. > > I think *option (a) — keep endpoint-owned sessions out of the general pool > entirely* — is the cleaner direction. A redirect-owned session has a > different lifecycle contract: it's tied to a specific endpoint, and its > reuse should be scoped to future writes targeting that same device/table. > Mixing it into the general pool creates the double-ownership problem you > identified. Concretely, I'd suggest: > > - getSessionForEndpoint should *not* push into this.pool or > this.idleSessions > - Release of a redirect-owned session should route back to the > endPointToSession map (mark it idle there), not to the general idle > queue > - Maybe even track redirect-owned sessions in a separate set so close() > / > cleanup still reaches them > > Regarding the fix plan: the two-PR split (pool lifecycle 1-5 + connection > leak 6) makes sense. Holding 7 for a follow-up after we settle on the > design is fine. Once you send the PRs I'll review them against the code. > > (By the way, the mention of iotdb-mcp-server seems unrelated to the above — > maybe intended for a different thread?) > > Thanks again for the careful audit! > > Best, > Xuan Wang > > Zh D <[email protected]> 于2026年7月22日周三 14:19写道： > > > Hi all, > > > > While working with the nodejs client, I audited the session pool and > > connection lifecycle in iotdb-client-nodejs and found seven related > > issues. Before sending patches I'd like to share the findings and the > > fix plan here, and get input on one design question. > > > > Findings (all reproduced with unit tests against develop): > > > > Acquisition path (BaseSessionPool.getSession): > > > > 1. A timed-out waiter is never removed from the wait queue: the timeout > > handler searches the queue for the resolve function, but the queue > > stores a wrapper closure, so the lookup never matches. A later > > releaseSession then hands the session to the dead waiter, the session > > is marked in-use forever, and the pool starves. > > > > 2. The create-new-session branch removes the wrong idle entry under > > interleaving: it does a blind shift() from the front after an await, > > while createSession pushes the new session to the back. A session > > released into the idle queue during the await gets evicted instead, > > and the new session ends up tracked as both idle and active. > > > > 3. The idle-reuse branch never sets inUse = true (the new-session and > > waiter branches both do), so syncDatabaseContextToPool, which filters > > on !inUse, can run "USE <db>" on a session that is mid-request for > > another caller. > > > > Cleanup path (cleanupIdleSessions): > > > > 4. The minPoolSize guard is evaluated against the pre-cleanup pool size, > > so one cleanup round can retire every idle session and shrink the > > pool below minPoolSize (down to zero). > > > > 5. Sessions are closed before being removed from the idle queue, and > > isOpen() stays true until the async close completes, so a concurrent > > getSession() can be handed a session that is being destroyed. > > > > Connection: > > > > 6. Connection.open() leaks the established socket and its listeners when > > openSession/requestStatementId fails after the TCP connect succeeded; > > through the pool, each failed connect attempt leaks one socket. > > > > Design question (redirect sessions): > > > > 7. With enableRedirection, a session created for a redirect endpoint is > > tracked in endPointToSession AND ends up in the general idle queue > > after release, so a plain getSession() and a redirect insert can > > drive the same Thrift socket concurrently. Two possible directions: > > (a) keep endpoint-owned sessions out of the general pool entirely, or > > (b) route their release back to the endpoint cache instead of the > > idle queue. Which of the two matches the intended redirect-reuse > > design? > > > > Fix plan: I have fixes for 1-6 ready with regression tests (each test > > fails on current develop and passes with the fix; the full unit suite is > > green). I would propose landing them as two focused PRs, one for the > > pool lifecycle (1-5, they share a test harness) and one for the > > connection open leak (6), and holding 7 for whichever direction the > > maintainers prefer. The same pool-misuse pattern also affects > > iotdb-mcp-server in tree mode (sessions are returned with close() > > instead of put_back(), which exhausts the pool after max_pool_size > > queries); I can follow up there once the approach here is agreed. > > > > Feedback welcome - happy to adjust the scope or split differently. > > > > Best, > > Zihan Dai > > >
