fix(mcp-client): bind strict sync to activation
The supervisor selected strict startup registration with a shared isFirstSync flag. Because the MCP SDK may deliver tools/list_changed before connect() resolves, that notification could enter the sync queue first, consume the strict option inside its contained handler, and leave the actual activation sync non-fatal. Pass startup intent explicitly to connectGeneration(). Only the plugin activation attempt receives the failOnStartupError registration policy; notification-driven syncs and later reconnect generations always use contained runtime semantics. Queue arrival order can no longer redefine startup behavior. A regression test injects list_changed from inside connect(), keeps a foreign namespace squatter in place, and proves activation still rejects after the notification's contained sync. Focused package coverage remains 100%, and the bilingual reconnect note records the ownership rule.
This commit is contained in:
@@ -133,7 +133,6 @@ export function startConnection(ctx: Context, config: Config, policy: ResolvedRe
|
||||
const startupOpts: ToolBridgeOptions = config.failOnStartupError
|
||||
? { ...opts, registrationFailure: 'throw' }
|
||||
: opts
|
||||
let isFirstSync = true
|
||||
|
||||
let disposed = false
|
||||
/** Current generation: the connecting or connected client; undefined during backoff waits and after final failure. */
|
||||
@@ -160,9 +159,7 @@ export function startConnection(ctx: Context, config: Config, policy: ResolvedRe
|
||||
* generation and leak another).
|
||||
*/
|
||||
let syncChain: Promise<void> = Promise.resolve()
|
||||
function enqueueSync(generation: Client): Promise<void> {
|
||||
const syncOpts = isFirstSync ? startupOpts : opts
|
||||
isFirstSync = false
|
||||
function enqueueSync(generation: Client, syncOpts: ToolBridgeOptions = opts): Promise<void> {
|
||||
const run = syncChain.then(async () => {
|
||||
if (!isCurrent(generation)) return
|
||||
disposers = await syncTools(generation, ctx, syncOpts, disposers)
|
||||
@@ -219,7 +216,7 @@ export function startConnection(ctx: Context, config: Config, policy: ResolvedRe
|
||||
ctx.logger.warn(`${label}: connection lost; reconnecting in ${delayMs}ms (attempt ${failedAttempts}/${policy.maxAttempts})`)
|
||||
reconnectTimer = setTimeout(() => {
|
||||
reconnectTimer = undefined
|
||||
settling = connectGeneration()
|
||||
settling = connectGeneration(false)
|
||||
}, delayMs)
|
||||
// An armed reconnect timer must never hold the process open on its own.
|
||||
reconnectTimer.unref()
|
||||
@@ -228,10 +225,14 @@ export function startConnection(ctx: Context, config: Config, policy: ResolvedRe
|
||||
/**
|
||||
* One connection attempt: fresh transport + client (the MCP SDK binds a
|
||||
* Protocol to one transport for life), connect, then queue the initial tool
|
||||
* sync. Every failure funnels through {@link generationDown}; success arms
|
||||
* the onclose-driven disconnect path. Never rejects.
|
||||
* sync. The startup flag belongs to the attempt rather than the shared sync
|
||||
* queue, so an early notification cannot consume strict startup semantics.
|
||||
* Every failure funnels through {@link generationDown}; success arms the
|
||||
* onclose-driven disconnect path. Never rejects.
|
||||
*
|
||||
* @param startup - Whether this is the plugin's activation attempt.
|
||||
*/
|
||||
async function connectGeneration(): Promise<void> {
|
||||
async function connectGeneration(startup: boolean): Promise<void> {
|
||||
const generation = new Client(
|
||||
{ name: 'dsh-mcp-client', version: '0.0.1' },
|
||||
{ capabilities: {} },
|
||||
@@ -272,7 +273,7 @@ export function startConnection(ctx: Context, config: Config, policy: ResolvedRe
|
||||
generationDown(generation)
|
||||
return
|
||||
}
|
||||
await enqueueSync(generation)
|
||||
await enqueueSync(generation, startup ? startupOpts : opts)
|
||||
} catch (error) {
|
||||
if (firstAttemptError === undefined) firstAttemptError = error
|
||||
// Disposal clears current ownership before it closes the generation, so
|
||||
@@ -302,7 +303,7 @@ export function startConnection(ctx: Context, config: Config, policy: ResolvedRe
|
||||
}
|
||||
|
||||
/** The in-flight (or last settled) connection attempt; dispose awaits it for quiescence. */
|
||||
let settling = connectGeneration()
|
||||
let settling = connectGeneration(true)
|
||||
|
||||
// The ready promise settles when the first attempt finishes (regardless of
|
||||
// success). If the first attempt fails and reconnect is enabled, the
|
||||
|
||||
@@ -288,6 +288,32 @@ describe('apply (plugin lifecycle)', () => {
|
||||
expect(mockClose).toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('preserves strict startup registration when list_changed arrives before connect resolves', async () => {
|
||||
ctx.tools.register({
|
||||
name: 'mcp__srv__remote',
|
||||
description: 'Foreign squatter',
|
||||
parameters: { type: 'object' },
|
||||
output: {
|
||||
schema: { type: 'string' },
|
||||
render: (_args, value) => [{ type: 'text', text: value as string }],
|
||||
},
|
||||
execute: async () => 'foreign',
|
||||
})
|
||||
mockConnect.mockImplementation(async () => {
|
||||
const handler = mockSetNotificationHandler.mock.calls[0]![1] as () => Promise<void>
|
||||
await handler()
|
||||
})
|
||||
|
||||
await expect(apply(ctx, {
|
||||
...stdioConfig,
|
||||
failOnStartupError: true,
|
||||
})).rejects.toThrow('initial connection or tool synchronization failed')
|
||||
|
||||
expect(mockListTools).toHaveBeenCalledTimes(2)
|
||||
expect(ctx.tools.get('mcp__srv__remote')?.description).toBe('Foreign squatter')
|
||||
await ctx.fiber.dispose()
|
||||
})
|
||||
|
||||
it('re-syncs tools on ToolListChanged notification', async () => {
|
||||
await apply(ctx, stdioConfig)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user