fix(mcp-client): distinguish failure from loss

The reconnect supervisor used connection lost for every transition into backoff, including an initial startup attempt that never established a connection and later retry attempts that also failed. That wording implied a previously healthy generation and obscured whether any tools had ever been registered.

Capture whether the generation had reached the established state before scheduling recovery. Established disconnects retain connection lost/reconnecting; startup and retry failures now report connection failed/retrying. The reconnect-disabled diagnostic uses the same distinction while preserving its concrete manual-recovery guidance.

Unit assertions cover established loss, initial failure, retry failure, and both reconnect-disabled branches. Focused package coverage remains 100%, and the bilingual Agent Note records the observable state vocabulary.
This commit is contained in:
Tianyi Cui
2026-08-11 00:05:06 +08:00
parent 0e01036a2a
commit e2556c51bf
5 changed files with 18 additions and 12 deletions

View File

@@ -2,5 +2,5 @@
# side as of the last confirmed-consistent state. Both languages carry equal authority;
# after editing either side, bring the other along and re-record with:
# pnpm run verify-translation-pairing --write .agents/notes/implemented/feature/2026-08-06-mcp-client-auto-reconnect.md
2026-08-06-mcp-client-auto-reconnect.md: 75e17da716f817306bb30322678d26b0e18a5aff
2026-08-06-mcp-client-auto-reconnect.zh.md: 0ba06d17dca895be8449eb771d34dc096307d176
2026-08-06-mcp-client-auto-reconnect.md: 99a8aec1abe3713822f8f17c17d8efaca5d61a4d
2026-08-06-mcp-client-auto-reconnect.zh.md: 8d4dc935e6edee9a05f556774e743e48234ca709

View File

@@ -20,7 +20,7 @@ The [MCP client](2026-07-07-mcp-client-plugin.md) connected once at plugin load.
**Config and resolution.** Both transports accept `reconnect { enabled, initialDelayMs, maxDelayMs, maxAttempts }` with schemastery defaults (on, 500ms, 30s, 10). `resolveReconnectPolicy()` is the explicit resolve step: it re-judges every bound and cross-field constraint because programmatic construction may bypass Schemastery, and misconfiguration fails the plugin instance at load.
**Observable states.** Reconnecting logs at warn with attempt count and delay, recovery at info, final failure and disabled-loss at error. During an outage the last good generation stays registered and calls against it fail — deterministic public names mean a recovered unchanged tool list reproduces identical definitions, keeping the model-visible schema prefix stable instead of flapping. With `reconnect.enabled: false` a lost connection keeps the v1 manual-recovery behavior.
**Observable states.** An initial or retry-attempt failure says `connection failed`; an established generation ending says `connection lost`. Retrying logs at warn with attempt count and delay, recovery at info, final failure and disabled recovery at error. During an outage the last good generation stays registered and calls against it fail — deterministic public names mean a recovered unchanged tool list reproduces identical definitions, keeping the model-visible schema prefix stable instead of flapping. With `reconnect.enabled: false` a lost connection keeps the v1 manual-recovery behavior.
**Disposal.** Dispose flips the fence, cancels any pending timer, closes the current client, then awaits the in-flight attempt and the sync queue before unregistering — quiescence, not just a request to stop. The reconnect timer is unref'd so a waiting backoff never holds a finishing process open.
@@ -38,7 +38,7 @@ The [MCP client](2026-07-07-mcp-client-plugin.md) connected once at plugin load.
## Testing
Unit (`tests/reconnect.spec.ts`, mocked SDK): recovery swaps generations without duplication or leaks and serves post-recovery calls, strict startup registration survives a pre-connect `list_changed` notification, failed initialization waits for the old generation's close signal and fails closed when that signal never arrives, disposal waits for the same signal with a bounded incomplete-shutdown path, the failure cap unregisters tools and stops, dispose cancels a pending backoff and quiesces an in-flight sync, a close after dispose schedules nothing, disabled mode keeps the v1 behavior, the stability window resets the budget while a crash loop exhausts it, double failure signals schedule one retry, stale generations and handlers are inert, and `resolveReconnectPolicy` rejects each invalid bound. E2E (`tests/mcp-client.e2e.ts`, keyless): the fixture server gained a `crash` tool that replies then exits; real-process tests prove a stdio crash recovers end to end and that unloading the plugin mid-outage stops reconnection promptly. Snapshot: deliberately none, per the original note's rationale — reconnection adds no new presentation shape, and a snapshot composition spawning a crashing server would make replays timing-dependent.
Unit (`tests/reconnect.spec.ts`, mocked SDK): recovery swaps generations without duplication or leaks and serves post-recovery calls, diagnostics distinguish initial or retry failure from established connection loss, strict startup registration survives a pre-connect `list_changed` notification, failed initialization waits for the old generation's close signal and fails closed when that signal never arrives, disposal waits for the same signal with a bounded incomplete-shutdown path, the failure cap unregisters tools and stops, dispose cancels a pending backoff and quiesces an in-flight sync, a close after dispose schedules nothing, disabled mode keeps the v1 behavior, the stability window resets the budget while a crash loop exhausts it, double failure signals schedule one retry, stale generations and handlers are inert, and `resolveReconnectPolicy` rejects each invalid bound. E2E (`tests/mcp-client.e2e.ts`, keyless): the fixture server gained a `crash` tool that replies then exits; real-process tests prove a stdio crash recovers end to end and that unloading the plugin mid-outage stops reconnection promptly. Snapshot: deliberately none, per the original note's rationale — reconnection adds no new presentation shape, and a snapshot composition spawning a crashing server would make replays timing-dependent.
## Consequences

View File

@@ -20,7 +20,7 @@ Status: implemented
**配置与解析。** 两种传输均接受 `reconnect { enabled, initialDelayMs, maxDelayMs, maxAttempts }` 配置Schemastery 默认值为启用、500ms、30s、10`resolveReconnectPolicy()` 是显式的解析步骤:它重新校验每个边界值和跨字段约束,因为程序化构造可能绕过 Schemastery配置错误在加载时即令插件实例失败。
**可观测状态。** 重连中以 warn 级别记录尝试次数和延迟,恢复以 info 级别记录,最终失败和禁用状态下的断连以 error 级别记录。故障期间,上一个正常代保持注册,对其工具的调用返回失败——确定性公开名称意味着恢复后未变化的工具列表会复现相同的定义,保持模型可见 schema 前缀稳定而非反复抖动。设置 `reconnect.enabled: false` 后,断连保持 v1 的手动恢复行为。
**可观测状态。** 初始尝试或重试尝试失败时记录 `connection failed`,已建立的代结束时记录 `connection lost`;重试的 warn 日志包含尝试次数和延迟,恢复以 info 级别记录,最终失败和禁用重连时的断连以 error 级别记录。故障期间,上一个正常代保持注册,对其工具的调用返回失败——确定性公开名称意味着恢复后未变化的工具列表会复现相同的定义,保持模型可见 schema 前缀稳定而非反复抖动。设置 `reconnect.enabled: false` 后,断连保持 v1 的手动恢复行为。
**资源释放。** dispose 翻转栅栏、取消待执行的定时器、关闭当前 client然后等待正在进行的尝试和同步队列完成后再注销工具——完全停稳而非仅发出停止请求。重连定时器使用 unref因此等待中的退避不会阻止进程正常退出。
@@ -38,7 +38,7 @@ Status: implemented
## 测试
单元测试(`tests/reconnect.spec.ts`mock SDK恢复在不产生重复或泄漏的前提下切换代并服务恢复后的调用、严格启动注册在连接前收到 `list_changed` 通知后仍然生效、初始化失败会等待旧代的关闭信号若该信号始终未到则停止重连、dispose 同样等待同一关闭信号并在有界等待到期时报告关停未完成、失败上限注销工具并停止、dispose 取消待执行的退避并使进行中的同步完全停稳、dispose 后的关闭不调度任何操作、禁用模式保持 v1 行为、稳定窗口重置预算而崩溃循环耗尽预算、双重失败信号仅调度一次重试、过时的代和处理器为惰性、`resolveReconnectPolicy` 拒绝每个无效边界值。E2E`tests/mcp-client.e2e.ts`无需密钥fixture 服务器新增了一个 `crash` 工具(先回复再退出);真实进程测试证明 stdio 崩溃端到端恢复,以及在故障期间卸载插件能立即停止重连。快照:刻意不做,原因与原 Agent Note 相同——重连不引入新的展示形态,而在快照组合中 spawn 崩溃服务器会使回放依赖时序。
单元测试(`tests/reconnect.spec.ts`mock SDK恢复在不产生重复或泄漏的前提下切换代并服务恢复后的调用、诊断区分初始或重试尝试失败与已建立连接丢失、严格启动注册在连接前收到 `list_changed` 通知后仍然生效、初始化失败会等待旧代的关闭信号若该信号始终未到则停止重连、dispose 同样等待同一关闭信号并在有界等待到期时报告关停未完成、失败上限注销工具并停止、dispose 取消待执行的退避并使进行中的同步完全停稳、dispose 后的关闭不调度任何操作、禁用模式保持 v1 行为、稳定窗口重置预算而崩溃循环耗尽预算、双重失败信号仅调度一次重试、过时的代和处理器为惰性、`resolveReconnectPolicy` 拒绝每个无效边界值。E2E`tests/mcp-client.e2e.ts`无需密钥fixture 服务器新增了一个 `crash` 工具(先回复再退出);真实进程测试证明 stdio 崩溃端到端恢复,以及在故障期间卸载插件能立即停止重连。快照:刻意不做,原因与原 Agent Note 相同——重连不引入新的展示形态,而在快照组合中 spawn 崩溃服务器会使回放依赖时序。
## 后果

View File

@@ -190,11 +190,12 @@ export function startConnection(ctx: Context, config: Config, policy: ResolvedRe
}
function scheduleReconnect(): void {
const lostEstablishedConnection = connectedAt !== undefined
if (!policy.enabled) {
const detail = connectedAt !== undefined
? 'registered tools will fail until an HMR reload or Host restart'
: 'no tools were registered; reload the plugin or restart the Host to connect'
ctx.logger.error(`${label}: connection lost and reconnect is disabled — ${detail}`)
const message = lostEstablishedConnection
? 'connection lost and reconnect is disabled — registered tools will fail until an HMR reload or Host restart'
: 'connection failed and reconnect is disabled — no tools were registered; reload the plugin or restart the Host to connect'
ctx.logger.error(`${label}: ${message}`)
return
}
// A connection that stayed up past the stability window (= maxDelayMs, the
@@ -213,7 +214,8 @@ export function startConnection(ctx: Context, config: Config, policy: ResolvedRe
return
}
const delayMs = Math.min(policy.maxDelayMs, policy.initialDelayMs * 2 ** (failedAttempts - 1))
ctx.logger.warn(`${label}: connection lost; reconnecting in ${delayMs}ms (attempt ${failedAttempts}/${policy.maxAttempts})`)
const action = lostEstablishedConnection ? 'connection lost; reconnecting' : 'connection failed; retrying'
ctx.logger.warn(`${label}: ${action} in ${delayMs}ms (attempt ${failedAttempts}/${policy.maxAttempts})`)
reconnectTimer = setTimeout(() => {
reconnectTimer = undefined
settling = connectGeneration(false)

View File

@@ -191,6 +191,7 @@ describe('reconnect supervisor', () => {
// Initial connect + exactly maxAttempts reconnect attempts.
expect(mockConnect).toHaveBeenCalledTimes(3)
expect(warns.some(line => line.includes('connection attempt failed: Error: server gone'))).toBe(true)
expect(warns.some(line => line.includes('connection failed; retrying in 4ms (attempt 2/2)'))).toBe(true)
await sleep(30)
expect(mockConnect).toHaveBeenCalledTimes(3)
})
@@ -222,6 +223,7 @@ describe('reconnect supervisor', () => {
})
it('does not start a replacement until a failed generation reports that it closed', async () => {
const { warns } = captureLogs(ctx)
mockConnect.mockRejectedValueOnce(new Error('initialize failed'))
// Model the SDK's fire-and-forget close after initialize fails: the
// harness's second close call returns, but the child has not exited yet.
@@ -235,6 +237,7 @@ describe('reconnect supervisor', () => {
instances[0]!.onclose?.()
await applying
await vi.waitFor(() => { expect(instances).toHaveLength(2) })
expect(warns.some(line => line.includes('connection failed; retrying in 2ms (attempt 1/2)'))).toBe(true)
})
it('stops reconnecting when a failed generation never reports that it closed', async () => {
@@ -329,7 +332,7 @@ describe('reconnect supervisor', () => {
expect(mockConnect).toHaveBeenCalledTimes(1)
// Pre-reconnect contract: the generation stays registered until disposal.
expect(ctx.tools.get('mcp__srv__remote')).toBeDefined()
expect(errors.some(line => line.includes('reconnect is disabled'))).toBe(true)
expect(errors.some(line => line.includes('connection lost and reconnect is disabled'))).toBe(true)
})
it('reconnect disabled after a failed initial connect reports no registered tools', async () => {
const { errors } = captureLogs(ctx)
@@ -337,6 +340,7 @@ describe('reconnect supervisor', () => {
await apply(ctx, stdioConfig({ enabled: false }))
await sleep(30)
expect(ctx.tools.get('mcp__srv__remote')).toBeUndefined()
expect(errors.some(line => line.includes('connection failed and reconnect is disabled'))).toBe(true)
expect(errors.some(line => line.includes('no tools were registered'))).toBe(true)
})