Session pool follow-ups: minPoolSize: 0, pending waiters on close(), and a failed idle-session close
还没有人认领这个 Issue。
评估
- 难度
- 4/5
- 预计耗时
- 3-5 天
- 新手友好度
- 48/100
- Issue 类型
- 缺陷
- 描述清晰度
- 基本清楚
- 活跃度
- 冷清
- 技术栈
- nodejs, typescript
调研方向
从 BaseSessionPool.ts 中 minPoolSize 处理逻辑所在的第 129 行和第 373 行附近、close() 所在的第 505 行附近,以及 cleanupIdleSessions 所在的第 391 行附近开始。检查现有的 acquire 和 cleanup 流程,然后针对大小为零的 pool、close() 期间排队的 waiter,以及关闭 idle sessions 失败的情况添加有针对性的覆盖。完成的标准是每种行为都有明确的结果,并且没有 waiter 或失败的关闭操作被静默放弃。
由索引模型根据 Issue 内容生成。
描述
Summary
Three non-blocking observations about BaseSessionPool that came out of the reviews of #19 and #20. None of them is a crash or a leak on the normal path, so I did not fold them into those PRs; @CritasWang suggested collecting them in a single tracking issue.
Line references are against develop at a8ca4d2.
1. minPoolSize: 0 is silently coerced to 1
BaseSessionPool.ts:129 (and the cleanup floor at :373) read the setting as:
const minSize = this.config.minPoolSize || 1;
0 || 1 is 1, so a caller who explicitly asks for minPoolSize: 0 gets one eagerly created session at init(), and cleanupIdleSessions then keeps one session alive forever rather than letting the pool drain to empty. In most pool implementations 0 means "create nothing up front, and allow the pool to go back to empty when idle", which is a reasonable ask for short-lived or cost-sensitive processes.
If 0 is intended to be legal, ?? instead of || would express it. If it is not intended to be legal, rejecting it in config validation would be clearer than silently changing it.
2. close() clears the wait queue without settling the waiters
close() at :505 ends with this.waitQueue.clear() (:525). The queued entries are the waiter callbacks themselves, so clearing the queue drops them without ever resolving or rejecting their promises.
A caller that is inside await pool.getSession() when another part of the application closes the pool therefore does not fail fast. It stays pending until the acquire timeout fires (waitTimeout, default 60000ms at :290) and then rejects with Timeout waiting for available session (:320) — which points at pool exhaustion rather than at the real cause. Rejecting each queued waiter during close() with a "pool is closed" error would make both the latency and the message correct.
3. A failed session.close() during idle cleanup orphans the connection
cleanupIdleSessions (:391 onward) deliberately removes a session from pool and idleSessions before awaiting close(), which is correct — it prevents a concurrent getSession() from handing out a session that is about to be destroyed. But if close() rejects, the error is logged (:409) and nothing else happens: the session is already out of every pool structure, so its underlying connection is no longer referenced, never retried, and never counted anywhere.
That is not a leak in the common case — the RPC usually fails because the connection is already gone — but a transport-level failure would leave a real socket open with no owner. Keeping the session in a small "failed to close" set for a later retry, or at least counting it in a metric, would make the failure visible.
Not included
The getSession() create-branch race that also came up in the #19 review is already fixed on develop (:266-276 now locates the session by identity instead of shift()ing the front of the idle queue), so there is nothing left to track there.
I am happy to send a PR for any of these — please let me know which are worth changing and whether you would prefer them separately.
- 主要语言
- TypeScript
- 星标
- 3
- 派生
- 2
- 平均合并
- 7 分钟
- 30 天内合并 PR
- 2
贡献指南
从这里开始
- 先读完整个 Issue,再读项目的贡献指南。
- 在 Issue 下留言说明你要接手 —— 这能避免两个人做同样的事。
- Fork 仓库,在一个分支上完成修改。
- 提交 Pull Request,并在描述里引用这个 Issue 编号。
相似的 Issue
-
难度 2/5 1-3 小时 新手友好度 75/100
safetrustcr/dApp-SafeTrust#426 ·
-
area:workflow bug ready-for-agent
难度 2/5 1-3 小时 新手友好度 75/100
fil-donadoni/tolaria#4409 ·
-
难度 2/5 1-3 小时 新手友好度 70/100
Fission-AI/OpenSpec#1960 ·
-
Add dependabot 未关闭
难度 2/5 1-3 小时 新手友好度 70/100
-
难度 2/5 1-3 小时 新手友好度 75/100
corsairdev/corsair#1764 ·