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 >
