From 2d546efca01dcb08acde59cb88532a4bebd93ccb Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Fri, 22 May 2026 00:47:40 +0800 Subject: [PATCH] =?UTF-8?q?refactor(core):=20F2=20PR=20A=20W11=20=E2=80=94?= =?UTF-8?q?=20extract=20attachPooledSession=20+=20rollbackReservationOnSpa?= =?UTF-8?q?wnFailure?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit W11 (filed as F2 follow-up from #4336 review): two private helpers on `McpTransportPool` to eliminate inline duplication in `acquire()`: - `attachPooledSession(entry, id, serverName, cfg, sessionId, toolReg, promptReg)`: builds `SessionMcpView` + `entry.attach` with the standard pool release callback. Used by both the fast-path attach (existing entry) and the post-spawn attach (after `await inFlight`). NOT used by `createUnpooledConnection` — its release callback runs `entry.forceShutdown('manual')` + `indexDetach` directly (no pool refcount accounting since unpooled entries are per-session). - `rollbackReservationOnSpawnFailure(reservationResult, serverName)`: R24 T17 contract — only release the budget slot if THIS acquire actually reserved a new slot (`'reserved'`); `'already_held'` skips because the sibling owns it. Used by both the unpooled catch and the pooled spawn-in-flight catch. Race-window invariants (W10 / W77 / W90 / W111 / W125 / R24 T17) stay at the call sites because they describe the SURROUNDING ordering, not the helpers themselves. Helpers are documented to defer those decisions back to callers. Behavior unchanged. Filed bucket: F2 perf cleanup PR A (R9 done / W11 this commit / W12 + R10 to follow). Test sweep: 28/28 mcp-transport-pool.test.ts pass; typecheck clean. --- packages/core/src/tools/mcp-transport-pool.ts | 153 ++++++++++++------ 1 file changed, 108 insertions(+), 45 deletions(-) diff --git a/packages/core/src/tools/mcp-transport-pool.ts b/packages/core/src/tools/mcp-transport-pool.ts index a4cdb4f6b9..e1d671cdd7 100644 --- a/packages/core/src/tools/mcp-transport-pool.ts +++ b/packages/core/src/tools/mcp-transport-pool.ts @@ -237,13 +237,6 @@ export class McpTransportPool { // session caller. Also handles the leftover stale entry case // (any pre-existing zombie not yet evicted by `onClosed`). if (existing && !existing.isTerminated()) { - const view = new SessionMcpView( - sessionToolRegistry, - sessionPromptRegistry, - sessionId, - serverName, - cfg, - ); // F2 (#4175 commit 6 review fix — wenshao W10): index update // happens AFTER `attach` succeeds. Pre-fix the order was // reversed; an `attach` rejection (e.g., entry transitioned @@ -251,11 +244,20 @@ export class McpTransportPool { // `attach` call) left a stale `sessionToEntries[sessionId]` // mapping with no matching `entry.refs.has(sessionId)` — // `releaseSession` would later iterate the stale id and call - // `entry.detach` on a non-attached session. + // `entry.detach` on a non-attached session. W11/PR A: + // `attachPooledSession` is the shared view+attach helper; + // call-site ordering (indexAttach AFTER attach, terminal-state + // self-heal in catch) stays here, not in the helper. try { - const conn = existing.attach(sessionId, view, { - release: () => this.release(id, sessionId), - }); + const conn = this.attachPooledSession( + existing, + id, + serverName, + cfg, + sessionId, + sessionToolRegistry, + sessionPromptRegistry, + ); this.indexAttach(sessionId, id); return conn; } catch (err) { @@ -355,16 +357,11 @@ export class McpTransportPool { sessionPromptRegistry, ); } catch (err) { - // R24 T17: only release if THIS acquire actually reserved a - // new slot. `'already_held'` means the sibling holds it; not - // ours to release. - if ( - this.opts.budget !== undefined && - reservationResult === 'reserved' && - !this.hasNameSibling(serverName) - ) { - this.opts.budget.release(serverName); - } + // R24 T17 (codified by W11/PR A as a shared helper): only + // release if THIS acquire actually reserved a new slot. + // `'already_held'` means the sibling holds it; not ours to + // release. + this.rollbackReservationOnSpawnFailure(reservationResult, serverName); throw err; } } @@ -397,20 +394,14 @@ export class McpTransportPool { // spawn failure (V21-4) so a transient connect failure // doesn't leak the slot until daemon restart. // - // R24 T17: only release if THIS acquire actually reserved a - // new slot (`reservationResult === 'reserved'`). When - // `tryReserve` returned `'already_held'`, a same-name - // sibling held the slot; this acquire reserved nothing, so - // a release here would phantom-decrement the budget counter - // if the sibling were concurrently evicted between - // `tryReserve` and this catch. - if ( - this.opts.budget !== undefined && - reservationResult === 'reserved' && - !this.hasNameSibling(serverName) - ) { - this.opts.budget.release(serverName); - } + // R24 T17 contract (codified as `rollbackReservationOnSpawnFailure` + // helper in W11/PR A): only release if THIS acquire actually + // reserved a new slot (`reservationResult === 'reserved'`). + // `'already_held'` means a sibling holds the slot — phantom- + // releasing here would decrement the counter if the sibling + // were concurrently evicted between `tryReserve` and this + // catch. + this.rollbackReservationOnSpawnFailure(reservationResult, serverName); throw err; }); this.spawnInFlight.set(id, inFlight); @@ -455,17 +446,16 @@ export class McpTransportPool { ); } - const view = new SessionMcpView( - sessionToolRegistry, - sessionPromptRegistry, - sessionId, - serverName, - cfg, - ); try { - const conn = entry.attach(sessionId, view, { - release: () => this.release(id, sessionId), - }); + const conn = this.attachPooledSession( + entry, + id, + serverName, + cfg, + sessionId, + sessionToolRegistry, + sessionPromptRegistry, + ); // F2 (#4175 commit 6 review fix — qwen-latest W111): re-index // AFTER attach succeeds. Pre-fix the early `indexAttach` at the // top of this branch was enough on the unpooled path (W77) @@ -831,6 +821,79 @@ export class McpTransportPool { // ---------- internals ---------- + /** + * F2 (#4175 commit 6 review fix — wenshao W11 / PR A): shared + * view+attach helper for the two POOLED `acquire()` branches (the + * fast-path for an existing entry, and the post-spawn attach after + * `await inFlight`). Pre-fix both branches inlined the same 3-step + * pattern (build view → entry.attach → return) with identical + * release-callback wiring; PR A's stated cleanup goal is to dedupe + * without losing the per-call-site race-window invariant comments + * that explain WHY each branch's surrounding ordering is what it is. + * + * NOT used by `createUnpooledConnection` — the unpooled release + * callback runs `entry.forceShutdown('manual')` directly (no pool + * refcount accounting since unpooled entries are per-session) and + * also calls `indexDetach` from the release callback itself. + * + * Caller is responsible for: + * - Terminal-state pre-check (`!entry.isTerminated()`) + race- + * window self-heal (`evictEntry` on the W125 catch path). + * - Reverse-index ordering (early `indexAttach` BEFORE await on + * the post-spawn branch per W90; AFTER attach on the fast-path + * per W10; W111 re-indexAttach AFTER attach on post-spawn). + * The race-window comments live at the call sites because they + * describe the surrounding ordering, not the attach itself. + */ + private attachPooledSession( + entry: PoolEntry, + id: ConnectionId, + serverName: string, + cfg: MCPServerConfig, + sessionId: string, + sessionToolRegistry: ToolRegistry, + sessionPromptRegistry: PromptRegistry, + ): PooledConnection { + const view = new SessionMcpView( + sessionToolRegistry, + sessionPromptRegistry, + sessionId, + serverName, + cfg, + ); + return entry.attach(sessionId, view, { + release: () => this.release(id, sessionId), + }); + } + + /** + * F2 (#4175 commit 6 review fix — wenshao W11 / PR A; codifies the + * R24 T17 contract): roll back THIS acquire's slot reservation on + * spawn failure. Used by both the unpooled-spawn catch and the + * pooled-spawn-in-flight catch — both decisions are identical: + * - `'reserved'` → THIS acquire newly held the slot; release + * if no sibling holds it + * - `'already_held'` → sibling holds it; never release here (the + * sibling's own onClosed / evictEntry will + * handle it). Pre-R24 the bare + * `!hasNameSibling()` check would phantom- + * release a slot this acquire never reserved + * when the sibling was concurrently evicted. + * - `undefined` → no budget configured; nothing to do. + */ + private rollbackReservationOnSpawnFailure( + reservationResult: 'reserved' | 'already_held' | undefined, + serverName: string, + ): void { + if ( + this.opts.budget !== undefined && + reservationResult === 'reserved' && + !this.hasNameSibling(serverName) + ) { + this.opts.budget.release(serverName); + } + } + /** * F2 (#4175 commit 6 review fix — wenshao R22 W125-followup A+B): * Single source of truth for evicting a pooled entry from