From 8202015e0acee1eb772c868b4b9d80d934c43ed0 Mon Sep 17 00:00:00 2001 From: Effie-2756 Date: Sat, 17 Oct 2026 14:09:42 -0400 Subject: [PATCH 1/6] fix(mcpl): the framework's closes for good go through the registry A reconnect: true server that the framework closes for good while it's down stayed registered. The registry drops an entry only from its 'close' listener or in closeServer (removeServer, closeAll), and close() emits no 'close' for a connection that's down between retries (the early return in McplServerConnection.close()). The framework closed servers for good with connection.close() at three sites: - connectMcplServerInternal's awareness-barrier catch, reached by a runtime connectMcplServer or restartMcplServer whose Discord awareness accounting fails. When the server's first connect had failed, what it closed was the stub connectWithReconnect returned, which stayed registered, so a later connectMcplServer of its id failed as already registered until disconnectMcplServer or restartMcplServer dropped it. Greptile's review of #451 found this path. - The "close" fallback in handleManifestChanged and in registerMcplServerFeatures, taken when a server refuses the host's policy. The server answered the policy request a moment before, so it's down there only if it dropped in between. Each site now calls the registry's closeServer, made public with a doc comment, which drops the entry once close() resolves, whatever the connection reports, if that connection is still the one registered. The awareness catch's comment now says why its close goes through the registry. Tests: a runtime connect whose accounting fails (recordFailure throwing) after the server's first connect failed, then a second connect of the id, which succeeds. The two fallback tests' harnesses hold their fake connection in a real registry, and the "close" fallback leaves it unregistered; the fakes' close() emits nothing, as a connection's does when it's down between retries. --- changelog.d/mcpl-close-down-server.fixed.md | 11 ++++ src/framework.ts | 11 +++- src/mcpl/server-registry.ts | 11 +++- test/mcpl-awareness-barrier.test.ts | 61 +++++++++++++++++++++ test/mcpl-config-diagnostics.test.ts | 13 ++++- test/mcpl-manifest-changed.test.ts | 14 +++-- 6 files changed, 111 insertions(+), 10 deletions(-) create mode 100644 changelog.d/mcpl-close-down-server.fixed.md diff --git a/changelog.d/mcpl-close-down-server.fixed.md b/changelog.d/mcpl-close-down-server.fixed.md new file mode 100644 index 000000000..7f58a950f --- /dev/null +++ b/changelog.d/mcpl-close-down-server.fixed.md @@ -0,0 +1,11 @@ +- A `reconnect: true` MCPL server that the framework closes for good now + leaves the registry even when it's down at the time, so a later connect of + its id isn't refused as already registered. The framework closes a server + for good when a runtime `connectMcplServer` or `restartMcplServer` fails + its Discord awareness accounting, and when the server refuses the host's + policy with the `"close"` fallback. The registry dropped a closed server + only when its connection reported the close, and a connection that's down + between retries reports none. So the stub a failed first connect leaves + stayed registered after such a close, and a later `connectMcplServer` of + its id failed as already registered until `disconnectMcplServer` or + `restartMcplServer` dropped it. diff --git a/src/framework.ts b/src/framework.ts index 3455acb3b..968b4b44b 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -17627,13 +17627,18 @@ export class AgentFramework { // the runtime connect or restart that asked for it. (A startup connect // defers awareness and returns before this gate; a startup that fails // ends in create()'s release, which closes its servers.) + // That close goes through the registry, which drops the entry itself + // rather than waiting for a 'close' event: close() emits none for a + // connection that's down between retries, such as a stub waiting for + // its next retry, and a stub left registered would refuse a later + // connect of its id as already registered. // Ordering matters: awareness accounting settles BEFORE the §5.3 // policy round-trip below, so an accounting failure aborts with the // grant still empty and every buffered event still behind the gate — // a failed connect never reports a released data plane. (The reverse // order let a push arrive during the policy await and get flushed by // teardown.) - await connection.close().catch(() => {}); + await this.mcplServerRegistry.closeServer(connection.id, connection).catch(() => {}); throw error; } @@ -17797,7 +17802,7 @@ export class AgentFramework { ...AgentFramework.policyRefusalFields(receipt), }); if (fallback === 'close') { - await connection.close(); + await this.mcplServerRegistry!.closeServer(connection.id, connection); return; } connection.establishGrant(CapabilityGrant.empty()); @@ -17915,7 +17920,7 @@ export class AgentFramework { ...AgentFramework.policyRefusalFields(receipt), }); if (fallback === 'close') { - await connection.close(); + await this.mcplServerRegistry!.closeServer(connection.id, connection); return; } // mcp-only (§3.2): grant stays empty, MCPL privileged surface stays diff --git a/src/mcpl/server-registry.ts b/src/mcpl/server-registry.ts index 7aed41e47..f4dd246f7 100644 --- a/src/mcpl/server-registry.ts +++ b/src/mcpl/server-registry.ts @@ -148,7 +148,16 @@ export class McplServerRegistry { })), timeoutMs); } - private closeServer(id: string, connection: McplServerConnection): Promise { + /** + * Close a server for good and drop its entry, if this connection is still + * the one registered under `id`. The entry goes when close() resolves, + * whatever the connection reports: close() emits no 'close' for a + * connection that's down between retries, such as a reconnect stub waiting + * for its next retry, so the 'close' listener alone would leave it + * registered. A close that fails keeps the entry and rejects; a close + * already under way is shared. + */ + closeServer(id: string, connection: McplServerConnection): Promise { const existing = this.closing.get(connection); if (existing) return existing; const attempt = Promise.resolve().then(() => connection.close()).then(() => { diff --git a/test/mcpl-awareness-barrier.test.ts b/test/mcpl-awareness-barrier.test.ts index 6530bee6a..0b0f3823c 100644 --- a/test/mcpl-awareness-barrier.test.ts +++ b/test/mcpl-awareness-barrier.test.ts @@ -700,6 +700,67 @@ for (const writeMethod of ['recordSuccess', 'recordFailure'] as const) test( }, ); +test('a runtime connect whose awareness accounting fails frees the id of the reconnect stub it closed', async () => { + mkdirSync(TEST_ROOT, { recursive: true }); + const dir = mkdtempSync(join(TEST_ROOT, 'awareness-runtime-stub-')); + const statusPath = join(dir, 'status.jsonl'); + const { outboxPath, config } = frameworkConfig(dir, { + STATUS_PATH: statusPath, + LEDGER_PATH: join(dir, 'awareness.json'), + GENERATION_PATH: join(dir, 'generation'), + FAIL_INITIAL_GENERATION: '1', + }, 1_000, { + reconnect: true, + // A stub waiting a minute for its retry is down between retries when the + // accounting failure closes it: the close that emits no 'close'. + reconnectIntervalMs: 60_000, + reconnectMaxIntervalMs: 60_000, + }); + const discord = config.mcplServers[0]!; + // Another server brings MCPL up at create(), so the runtime connect below + // is the one that accounts for the pending work. Brought up lazily by that + // connect, MCPL would account for it in its own startup barrier first. + const heartbeat: McplServerConfig = { + id: 'heartbeat', + command: process.execPath, + args: [FIXTURE], + requestTimeoutMs: 1_000, + env: { SERVER_ID: 'heartbeat' }, + enabledFeatureSets: ['chat'], + }; + const prototype = DiscordAwarenessOutbox.prototype as any; + const original = prototype.recordFailure; + let framework: AgentFramework | undefined; + + try { + framework = await AgentFramework.create({ ...config, mcplServers: [heartbeat] }); + preparePending(outboxPath); + prototype.recordFailure = () => { throw new Error('injected recordFailure write failure'); }; + try { + await assert.rejects( + framework.connectMcplServer(discord), + /awareness accounting failed.*injected recordFailure write failure/i, + ); + } finally { + prototype.recordFailure = original; + } + assert.equal( + records(statusPath).filter((record) => record.event === 'initial-connect-failure').length, + 1, + 'the first connect failed, so the registry held a reconnect stub', + ); + + // Connecting the id again reaches the server: a stub left registered + // would fail this connect as already registered. + await framework.connectMcplServer(discord); + assert.equal(framework.listMcplServers().find((server) => server.id === 'discord')?.connected, true); + } finally { + prototype.recordFailure = original; + await framework?.stop(); + rmSync(dir, { recursive: true, force: true }); + } +}); + test('runtime reconciliation failure recycles the connection and keeps fresh data gated', async () => { mkdirSync(TEST_ROOT, { recursive: true }); const dir = mkdtempSync(join(TEST_ROOT, 'awareness-runtime-accounting-')); diff --git a/test/mcpl-config-diagnostics.test.ts b/test/mcpl-config-diagnostics.test.ts index a49292a87..a820d86dc 100644 --- a/test/mcpl-config-diagnostics.test.ts +++ b/test/mcpl-config-diagnostics.test.ts @@ -18,6 +18,7 @@ import { join } from 'node:path'; import { AgentFramework } from '../src/index.js'; import type { EventResponse, Module, ModuleContext, ToolDefinition, ToolResult, TraceEvent } from '../src/index.js'; import { FeatureSetManager } from '../src/mcpl/feature-set-manager.js'; +import { McplServerRegistry } from '../src/mcpl/server-registry.js'; import { CapabilityGrant, computeGrant, expandAdvertisementShorthand } from '../src/mcpl/capability-grant.js'; import { checkToolPattern } from '../src/mcpl/tool-pattern-check.js'; import type { McplCapabilities, McplServerConfig } from '../src/mcpl/types.js'; @@ -333,7 +334,7 @@ test('FeatureSetManager: each §6.4 disablement is traced beside its console lin assert.equal(quiet.lines.length, 3, 'the console line stays'); }); -test('registerMcplServerFeatures: a refused initial policy is traced, and fallback close closes', async () => { +test('registerMcplServerFeatures: a refused initial policy is traced, and fallback close closes the server and unregisters it', async () => { for (const fallback of ['close', 'mcp-only'] as const) { const fw = Object.create(AgentFramework.prototype) as any; fw.traceListeners = []; @@ -343,6 +344,8 @@ test('registerMcplServerFeatures: a refused initial policy is traced, and fallba fw.onTrace((e: Traced) => traces.push(e)); let closed = 0; let established = 0; + // Its close() emits no 'close', as a server's doesn't once it's down + // between retries, so only a close through the registry unregisters it. const connection = { id: 'srv', capabilities: { version: '0.5', pushEvents: true } as unknown as McplCapabilities, @@ -351,6 +354,9 @@ test('registerMcplServerFeatures: a refused initial policy is traced, and fallba sendFeatureSetsUpdateRequest: async () => ({ accepted: false, fallback, reason: 'policy too narrow' }), close: async () => { closed++; }, }; + const registry = new McplServerRegistry(); + (registry as any).servers.set('srv', connection); + fw.mcplServerRegistry = registry; const capture = captureConsoleError(); try { await fw.registerMcplServerFeatures({ id: 'srv', command: 'unused' } as McplServerConfig, connection); @@ -358,6 +364,7 @@ test('registerMcplServerFeatures: a refused initial policy is traced, and fallba capture.stop(); } assert.equal(closed, fallback === 'close' ? 1 : 0, fallback); + assert.equal(registry.getServer('srv'), fallback === 'close' ? null : connection, `${fallback}: registered afterwards`); assert.equal(established, 0, 'a refusal never establishes the grant'); assert.deepEqual( traces.filter((t) => t.type === 'mcpl:policy-refused').map(withoutTimestamp), @@ -414,7 +421,9 @@ function manifestHarness(receipt: Record) { const config: McplServerConfig = { id: 'srv', command: 'unused', enabledFeatureSets: ['chat', 'bare'] }; fw.mcplServerConfigs = new Map([[config.id, config]]); const connection = new ManifestFake(receipt); - fw.mcplServerRegistry = { getAllServers: () => [connection], getServer: (id: string) => (id === 'srv' ? connection : null) }; + const registry = new McplServerRegistry(); + (registry as any).servers.set('srv', connection); + fw.mcplServerRegistry = registry; // Post-§5.3 state: initial policy done, full grant active. const grant = computeGrant(connection.capabilities!, config, { mcpToolsAdvertised: false }); const quiet = captureConsoleError(); diff --git a/test/mcpl-manifest-changed.test.ts b/test/mcpl-manifest-changed.test.ts index 82e12aec9..eeffc5383 100644 --- a/test/mcpl-manifest-changed.test.ts +++ b/test/mcpl-manifest-changed.test.ts @@ -16,6 +16,7 @@ import { EventEmitter } from 'node:events'; import { AgentFramework } from '../src/framework.js'; import { FeatureSetManager } from '../src/mcpl/feature-set-manager.js'; +import { McplServerRegistry } from '../src/mcpl/server-registry.js'; import { CapabilityGrant, computeGrant, expandAdvertisementShorthand } from '../src/mcpl/capability-grant.js'; import type { McplCapabilities, McplServerConfig } from '../src/mcpl/types.js'; @@ -87,7 +88,9 @@ function makeHarness() { const connection = new ManifestFake(); connection.capabilities = expandAdvertisementShorthand(INITIAL); - fw.mcplServerRegistry = { getAllServers: () => [connection], getServer: (id: string) => (id === 'srv' ? connection : null) }; + const registry = new McplServerRegistry(); + (registry as any).servers.set('srv', connection); + fw.mcplServerRegistry = registry; // Model post-§5.3 state: initial policy done, full grant active. const grant = computeGrant(connection.capabilities, config, { mcpToolsAdvertised: false }); @@ -95,7 +98,7 @@ function makeHarness() { connection.establishGrant(grant); connection.events = []; // discard setup noise - return { fw, connection, traces }; + return { fw, connection, traces, registry }; } const settle = () => new Promise((r) => setImmediate(r)); @@ -176,14 +179,17 @@ test('refusal with fallback mcp-only: empty grant, empty-derived feature state', assert.equal(connection.closed, false); }); -test('refusal with fallback close: the transport is closed', async () => { - const { fw, connection } = makeHarness(); +test('refusal with fallback close: the transport is closed and the server unregistered', async () => { + const { fw, connection, registry } = makeHarness(); connection.answerPolicy = 'refuse-close'; connection.nextManifest = () => INITIAL; await fw.handleManifestChanged(connection, { revision: 'sha256:r2', domains: ['capabilities'] }); assert.equal(connection.closed, true, 'fallback close must be honored (§6.7)'); + // The fake's close() emits no 'close', as a server's doesn't once it's down + // between retries, so only a close through the registry unregisters it. + assert.equal(registry.getServer('srv'), null); }); test('failed fetch: previous manifest and grant stand untouched', async () => { From afa80b53a3f56b3b2094041aa9059b9b84397f66 Mon Sep 17 00:00:00 2001 From: Effie-2756 Date: Sat, 17 Oct 2026 14:09:42 -0400 Subject: [PATCH 2/6] fix(mcpl): a runtime connect whose awareness accounting fails fails its gate connectMcplServerInternal installs the data-plane gate before it awaits awareness accounting, which pauses every registered server's data plane. When accounting failed, the catch closed the new connection and rethrew, and never failed or completed that gate. Every other runtime gate calls failMcplDataPlaneGate when its accounting fails: quiesce resume(), explicit syncDiscordAwarenessMarkers(), tools-list reconciliation and reconnect reconciliation. So after a runtime connectMcplServer or restartMcplServer whose accounting failed, every other server's pushes, incoming messages and inference requests were held until some other gate completed. A probe without this commit's change ran a server, then a runtime connect whose accounting failed: the failed connect's gate stayed current, and the running server's data plane stayed paused, still 3 s later. The catch now calls failMcplDataPlaneGate after the close for good, as those paths do. While serving, that recycles the running servers: each with reconnect: true comes back, and its data flows once its reconnect's accounting succeeds, while a server without it stays closed, as on those paths. While stopping, it releases the gate. The close comes first, so the connection being refused is out of the registry and isn't recycled, unless its close fails, which keeps its entry for a later close to retry. Tests: the runtime-connect fixture moves into a helper that stops the framework if anything after create() fails. A new test runs a reconnecting server, fails a runtime connect's accounting, and checks that the gate failed for the running server (its "accounting unhealthy" mcpl:server-error trace) and that the recycled server's next startup push is answered. --- changelog.d/mcpl-close-down-server.fixed.md | 10 +++ src/framework.ts | 9 +++ test/mcpl-awareness-barrier.test.ts | 83 +++++++++++++++++---- 3 files changed, 88 insertions(+), 14 deletions(-) diff --git a/changelog.d/mcpl-close-down-server.fixed.md b/changelog.d/mcpl-close-down-server.fixed.md index 7f58a950f..32bb0cd8c 100644 --- a/changelog.d/mcpl-close-down-server.fixed.md +++ b/changelog.d/mcpl-close-down-server.fixed.md @@ -9,3 +9,13 @@ stayed registered after such a close, and a later `connectMcplServer` of its id failed as already registered until `disconnectMcplServer` or `restartMcplServer` dropped it. +- A runtime `connectMcplServer` or `restartMcplServer` whose Discord + awareness accounting fails no longer leaves every other MCPL server's data + held. The connect paused their data planes behind a gate it never failed + or completed, so their pushes, incoming messages and inference requests + waited until some other gate completed. It now fails that gate, as a + failed reconnect, tools-list reconciliation, + `syncDiscordAwarenessMarkers()` or `resume()` already does: each running + server is recycled, and its data flows once its reconnect's accounting + succeeds. A server without `reconnect: true` is closed for good there, as + on those paths. While the host is stopping, the gate is released instead. diff --git a/src/framework.ts b/src/framework.ts index 968b4b44b..079e2f7b0 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -17632,6 +17632,14 @@ export class AgentFramework { // connection that's down between retries, such as a stub waiting for // its next retry, and a stub left registered would refuse a later // connect of its id as already registered. + // Then fail the gate, as every other runtime gate does when its + // accounting fails. Installing it paused every registered server's + // data plane, and left standing it would hold their data until some + // other gate completed. While serving, failMcplDataPlaneGate recycles + // those servers, so a server without `reconnect: true` stays closed; + // while stopping, it releases the gate. It runs after the close, which + // has taken this connection out of the registry unless the close + // failed. // Ordering matters: awareness accounting settles BEFORE the §5.3 // policy round-trip below, so an accounting failure aborts with the // grant still empty and every buffered event still behind the gate — @@ -17639,6 +17647,7 @@ export class AgentFramework { // order let a push arrive during the policy await and get flushed by // teardown.) await this.mcplServerRegistry.closeServer(connection.id, connection).catch(() => {}); + await this.failMcplDataPlaneGate(awarenessBarrier, `connect of MCPL server "${config.id}"`, error); throw error; } diff --git a/test/mcpl-awareness-barrier.test.ts b/test/mcpl-awareness-barrier.test.ts index 0b0f3823c..e2d819a23 100644 --- a/test/mcpl-awareness-barrier.test.ts +++ b/test/mcpl-awareness-barrier.test.ts @@ -700,10 +700,18 @@ for (const writeMethod of ['recordSuccess', 'recordFailure'] as const) test( }, ); -test('a runtime connect whose awareness accounting fails frees the id of the reconnect stub it closed', async () => { - mkdirSync(TEST_ROOT, { recursive: true }); - const dir = mkdtempSync(join(TEST_ROOT, 'awareness-runtime-stub-')); +/** + * A framework serving `heartbeat`, then a runtime connect of `discord` whose + * first connect fails, so the registry holds a reconnect stub, and whose + * awareness accounting then fails (recordFailure throws). Serving another + * server brings MCPL up at create(), so the runtime connect is the one that + * accounts for the pending work: brought up lazily by that connect, MCPL + * would account for it in its own startup barrier first. A failure after + * create() stops the framework, so no child outlives the test. + */ +async function failRuntimeConnectWhileServing(dir: string, heartbeatOverrides: Partial = {}) { const statusPath = join(dir, 'status.jsonl'); + const heartbeatStatusPath = join(dir, 'heartbeat-status.jsonl'); const { outboxPath, config } = frameworkConfig(dir, { STATUS_PATH: statusPath, LEDGER_PATH: join(dir, 'awareness.json'), @@ -717,24 +725,26 @@ test('a runtime connect whose awareness accounting fails frees the id of the rec reconnectMaxIntervalMs: 60_000, }); const discord = config.mcplServers[0]!; - // Another server brings MCPL up at create(), so the runtime connect below - // is the one that accounts for the pending work. Brought up lazily by that - // connect, MCPL would account for it in its own startup barrier first. const heartbeat: McplServerConfig = { id: 'heartbeat', command: process.execPath, args: [FIXTURE], requestTimeoutMs: 1_000, - env: { SERVER_ID: 'heartbeat' }, + env: { + SERVER_ID: 'heartbeat', + STATUS_PATH: heartbeatStatusPath, + GENERATION_PATH: join(dir, 'heartbeat-generation'), + }, enabledFeatureSets: ['chat'], + ...heartbeatOverrides, }; - const prototype = DiscordAwarenessOutbox.prototype as any; - const original = prototype.recordFailure; - let framework: AgentFramework | undefined; - + const framework = await AgentFramework.create({ ...config, mcplServers: [heartbeat] }); try { - framework = await AgentFramework.create({ ...config, mcplServers: [heartbeat] }); + const traces: Array<{ type: string; serverId?: string; error?: string }> = []; + framework.onTrace((event) => traces.push(event as never)); preparePending(outboxPath); + const prototype = DiscordAwarenessOutbox.prototype as any; + const original = prototype.recordFailure; prototype.recordFailure = () => { throw new Error('injected recordFailure write failure'); }; try { await assert.rejects( @@ -749,13 +759,58 @@ test('a runtime connect whose awareness accounting fails frees the id of the rec 1, 'the first connect failed, so the registry held a reconnect stub', ); + return { framework, discord, heartbeatStatusPath, traces }; + } catch (error) { + await framework.stop(); + throw error; + } +} + +test('a runtime connect whose awareness accounting fails frees the id of the reconnect stub it closed', async () => { + mkdirSync(TEST_ROOT, { recursive: true }); + const dir = mkdtempSync(join(TEST_ROOT, 'awareness-runtime-stub-')); + let framework: AgentFramework | undefined; + + try { + const failed = await failRuntimeConnectWhileServing(dir); + framework = failed.framework; // Connecting the id again reaches the server: a stub left registered // would fail this connect as already registered. - await framework.connectMcplServer(discord); + await framework.connectMcplServer(failed.discord); assert.equal(framework.listMcplServers().find((server) => server.id === 'discord')?.connected, true); } finally { - prototype.recordFailure = original; + await framework?.stop(); + rmSync(dir, { recursive: true, force: true }); + } +}); + +test('a runtime connect whose awareness accounting fails fails its gate, so a running server is recycled rather than held', async () => { + mkdirSync(TEST_ROOT, { recursive: true }); + const dir = mkdtempSync(join(TEST_ROOT, 'awareness-runtime-gate-')); + let framework: AgentFramework | undefined; + + try { + const failed = await failRuntimeConnectWhileServing(dir, { + reconnect: true, + reconnectIntervalMs: 20, + reconnectMaxIntervalMs: 20, + }); + framework = failed.framework; + assert.ok( + failed.traces.some((trace) => trace.type === 'mcpl:server-error' + && trace.serverId === 'heartbeat' + && /accounting unhealthy/.test(trace.error ?? '')), + 'the gate failed for the running server, as a failed gate on the other runtime paths does', + ); + + // Recycled, the server reconnects, and its data flows once its + // reconnect's accounting succeeds: the new process's startup push is + // answered. + await waitFor('the recycled server\'s startup push answered', () => + records(failed.heartbeatStatusPath).some((record) => + record.generation === 2 && record.event === 'push-response'), 10_000); + } finally { await framework?.stop(); rmSync(dir, { recursive: true, force: true }); } From c87c643637f0d5b53a25aa1f58ed261467102208 Mon Sep 17 00:00:00 2001 From: Effie-2756 Date: Sat, 17 Oct 2026 16:23:50 -0400 Subject: [PATCH 3/6] fix(mcpl): a runtime connect settles its gate on every exit; tests for the order, the identity guard and the closed server Answers the internal review of the first two commits. A runtime connect whose policy registration throws after its awareness accounting succeeded left the new connection live and registered, and the gate it installed standing over every other server's data planes, since only the success path completed the gate. That catch now closes the new connection for good through the registry, as the accounting catch does, and completes the gate rather than failing it: accounting succeeded, so nothing calls for a recycle. Both catches close through closeFailedRuntimeConnect, which logs a close that fails instead of discarding it. Such a close keeps the entry, so a later disconnect, restart or shutdown can close it again. close()'s comment now says only closeAll bounds its caller's wait for an in-flight close. Tests: - A runtime connect whose policy registration throws (initializeServer made to throw for it) while another server runs: the connect rejects, the gate is completed, the running server stays connected with its data plane ready and no gate fails, and the id connects again. - The runtime-connect helper checks that the refused connection gets no "accounting unhealthy" trace, which it would if the gate failed before the close took it out of the registry. - The stub test's running server has no reconnect: true; it checks that the server ends neither connected nor retrying. - A registry test: closeServer leaves an entry that a newer connection holds under the same id, as after a restart. The changelog fragment covers the policy registration exit and a failed close. --- changelog.d/mcpl-close-down-server.fixed.md | 42 ++++---- src/framework.ts | 37 ++++++- src/mcpl/server-connection.ts | 3 +- test/mcpl-awareness-barrier.test.ts | 103 +++++++++++++++++--- test/shutdown-errors.test.ts | 14 +++ 5 files changed, 164 insertions(+), 35 deletions(-) diff --git a/changelog.d/mcpl-close-down-server.fixed.md b/changelog.d/mcpl-close-down-server.fixed.md index 32bb0cd8c..a3cc33faf 100644 --- a/changelog.d/mcpl-close-down-server.fixed.md +++ b/changelog.d/mcpl-close-down-server.fixed.md @@ -2,20 +2,28 @@ leaves the registry even when it's down at the time, so a later connect of its id isn't refused as already registered. The framework closes a server for good when a runtime `connectMcplServer` or `restartMcplServer` fails - its Discord awareness accounting, and when the server refuses the host's - policy with the `"close"` fallback. The registry dropped a closed server - only when its connection reported the close, and a connection that's down - between retries reports none. So the stub a failed first connect leaves - stayed registered after such a close, and a later `connectMcplServer` of - its id failed as already registered until `disconnectMcplServer` or - `restartMcplServer` dropped it. -- A runtime `connectMcplServer` or `restartMcplServer` whose Discord - awareness accounting fails no longer leaves every other MCPL server's data - held. The connect paused their data planes behind a gate it never failed - or completed, so their pushes, incoming messages and inference requests - waited until some other gate completed. It now fails that gate, as a - failed reconnect, tools-list reconciliation, - `syncDiscordAwarenessMarkers()` or `resume()` already does: each running - server is recycled, and its data flows once its reconnect's accounting - succeeds. A server without `reconnect: true` is closed for good there, as - on those paths. While the host is stopping, the gate is released instead. + its Discord awareness accounting or its policy registration, and when the + server refuses the host's policy with the `"close"` fallback. The registry + dropped a closed server only when its connection reported the close, and a + connection that's down between retries reports none. So the stub a failed + first connect leaves stayed registered after such a close, and a later + `connectMcplServer` of its id failed as already registered until + `disconnectMcplServer` or `restartMcplServer` dropped it. A close that + fails still keeps the server registered, so a later disconnect, restart or + shutdown can close it again, and the failed connect now logs it. +- A runtime `connectMcplServer` or `restartMcplServer` that fails after + pausing the other MCPL servers' data planes no longer leaves their data + held. The connect paused them behind a gate that it settled only when it + succeeded, so their pushes, incoming messages and inference requests + waited until some other gate completed. + - When its Discord awareness accounting fails, it now fails that gate, as + a failed reconnect, tools-list reconciliation, + `syncDiscordAwarenessMarkers()` or `resume()` already does: each running + server is recycled, and its data flows once its reconnect's accounting + succeeds. A server without `reconnect: true` is closed for good there, + as on those paths. While the host is stopping, the gate is released + instead. + - When its policy registration throws after accounting succeeded, it now + closes the new server for good, where it stayed connected and half + registered, and completes the gate, so the running servers' data flows + on with no recycle. diff --git a/src/framework.ts b/src/framework.ts index 079e2f7b0..ee72b5be6 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -17535,6 +17535,25 @@ export class AgentFramework { })); } + /** + * Close a runtime connect's new connection for good, through the registry, + * after a step of that connect failed. A close that fails keeps the entry, + * so a later disconnect, restart or shutdown can close it again, and is + * logged here rather than replacing the failure the connect reports. + */ + private async closeFailedRuntimeConnect( + connection: McplServerConnection, + failedStep: string, + ): Promise { + await this.mcplServerRegistry!.closeServer(connection.id, connection).catch((closeError) => { + console.error( + `[mcpl] could not close MCPL server "${connection.id}" after its ${failedStep} failed; ` + + 'it stays registered until a disconnect, restart or shutdown closes it:', + closeError instanceof Error ? closeError.message : closeError, + ); + }); + } + /** * Connect a single MCPL server: register routing entries, open the * connection, wire events, and initialize feature sets / scopes / @@ -17639,14 +17658,14 @@ export class AgentFramework { // those servers, so a server without `reconnect: true` stays closed; // while stopping, it releases the gate. It runs after the close, which // has taken this connection out of the registry unless the close - // failed. + // failed, which closeFailedRuntimeConnect logs. // Ordering matters: awareness accounting settles BEFORE the §5.3 // policy round-trip below, so an accounting failure aborts with the // grant still empty and every buffered event still behind the gate — // a failed connect never reports a released data plane. (The reverse // order let a push arrive during the policy await and get flushed by // teardown.) - await this.mcplServerRegistry.closeServer(connection.id, connection).catch(() => {}); + await this.closeFailedRuntimeConnect(connection, 'awareness accounting'); await this.failMcplDataPlaneGate(awarenessBarrier, `connect of MCPL server "${config.id}"`, error); throw error; } @@ -17655,7 +17674,19 @@ export class AgentFramework { // completes: the grant is established (or knowingly left empty) before // ANY buffered traffic (control or data) flows. complete() then // ready()s both planes through the admission gate. - await this.registerMcplServerFeatures(config, connection); + try { + await this.registerMcplServerFeatures(config, connection); + } catch (error) { + // A policy registration that throws would leave the new connection + // live and half registered, and the gate standing over every other + // server's data. So close the connection for good through the + // registry, as the catch above does, then complete the gate rather + // than fail it: accounting succeeded, so nothing calls for a recycle. + // Every exit of a runtime connect settles the gate it installed. + await this.closeFailedRuntimeConnect(connection, 'policy registration'); + this.completeMcplDataPlaneGate(awarenessBarrier); + throw error; + } this.completeMcplDataPlaneGate(awarenessBarrier); this.emitTrace({ type: 'module:added', moduleName: `mcpl:${config.id}` }); diff --git a/src/mcpl/server-connection.ts b/src/mcpl/server-connection.ts index 16bcbbeb4..505bc2c1b 100644 --- a/src/mcpl/server-connection.ts +++ b/src/mcpl/server-connection.ts @@ -955,7 +955,8 @@ export class McplServerConnection extends EventEmitter { const attempt = Promise.resolve().then(async () => { // An already opened handshake owns a transport too. It hands that // transport back without admission once reconnect has been disabled. - // The registry bounds this wait; retry reuses this real close attempt. + // Only the registry's closeAll bounds its caller's wait for this; retry + // reuses this real close attempt. if (this.reconnectPromise) await this.reconnectPromise; if (this.transport) await this.transport.close(); this.closeIncomplete = false; diff --git a/test/mcpl-awareness-barrier.test.ts b/test/mcpl-awareness-barrier.test.ts index e2d819a23..31443394b 100644 --- a/test/mcpl-awareness-barrier.test.ts +++ b/test/mcpl-awareness-barrier.test.ts @@ -14,6 +14,7 @@ import { EventEmitter } from 'node:events'; import { AgentFramework } from '../src/framework.js'; import { McplServerConnection } from '../src/mcpl/server-connection.js'; import { CapabilityGrant } from '../src/mcpl/capability-grant.js'; +import { FeatureSetManager } from '../src/mcpl/feature-set-manager.js'; import type { McplServerConfig } from '../src/mcpl/types.js'; import { DiscordAwarenessOutbox } from '../src/recovery/discord-awareness-outbox.js'; import { MockMembrane } from './helpers/mock-membrane.js'; @@ -700,6 +701,23 @@ for (const writeMethod of ['recordSuccess', 'recordFailure'] as const) test( }, ); +/** A second fixture server, `heartbeat`, for a framework to serve from create(). */ +function heartbeatConfig(dir: string, overrides: Partial = {}): McplServerConfig { + return { + id: 'heartbeat', + command: process.execPath, + args: [FIXTURE], + requestTimeoutMs: 1_000, + env: { + SERVER_ID: 'heartbeat', + STATUS_PATH: join(dir, 'heartbeat-status.jsonl'), + GENERATION_PATH: join(dir, 'heartbeat-generation'), + }, + enabledFeatureSets: ['chat'], + ...overrides, + }; +} + /** * A framework serving `heartbeat`, then a runtime connect of `discord` whose * first connect fails, so the registry holds a reconnect stub, and whose @@ -725,19 +743,7 @@ async function failRuntimeConnectWhileServing(dir: string, heartbeatOverrides: P reconnectMaxIntervalMs: 60_000, }); const discord = config.mcplServers[0]!; - const heartbeat: McplServerConfig = { - id: 'heartbeat', - command: process.execPath, - args: [FIXTURE], - requestTimeoutMs: 1_000, - env: { - SERVER_ID: 'heartbeat', - STATUS_PATH: heartbeatStatusPath, - GENERATION_PATH: join(dir, 'heartbeat-generation'), - }, - enabledFeatureSets: ['chat'], - ...heartbeatOverrides, - }; + const heartbeat = heartbeatConfig(dir, heartbeatOverrides); const framework = await AgentFramework.create({ ...config, mcplServers: [heartbeat] }); try { const traces: Array<{ type: string; serverId?: string; error?: string }> = []; @@ -759,6 +765,16 @@ async function failRuntimeConnectWhileServing(dir: string, heartbeatOverrides: P 1, 'the first connect failed, so the registry held a reconnect stub', ); + // The catch closes the refused connection before it fails the gate, so + // the gate's failure finds it gone from the registry and doesn't trace or + // recycle it. + assert.equal( + traces.some((trace) => trace.type === 'mcpl:server-error' + && trace.serverId === discord.id + && /accounting unhealthy/.test(trace.error ?? '')), + false, + 'the refused connection was out of the registry when the gate failed', + ); return { framework, discord, heartbeatStatusPath, traces }; } catch (error) { await framework.stop(); @@ -766,7 +782,7 @@ async function failRuntimeConnectWhileServing(dir: string, heartbeatOverrides: P } } -test('a runtime connect whose awareness accounting fails frees the id of the reconnect stub it closed', async () => { +test('a runtime connect whose awareness accounting fails frees the id of the reconnect stub it closed, and closes a running server without reconnect: true for good', async () => { mkdirSync(TEST_ROOT, { recursive: true }); const dir = mkdtempSync(join(TEST_ROOT, 'awareness-runtime-stub-')); let framework: AgentFramework | undefined; @@ -775,6 +791,13 @@ test('a runtime connect whose awareness accounting fails frees the id of the rec const failed = await failRuntimeConnectWhileServing(dir); framework = failed.framework; + // The running server has no reconnect: true, so the failed gate's + // recycle closed it for good: neither connected and held, nor retrying. + await waitFor('the running server closed for good', () => { + const heartbeat = failed.framework.listMcplServers().find((server) => server.id === 'heartbeat'); + return heartbeat !== undefined && !heartbeat.connected && !heartbeat.retrying; + }, 10_000); + // Connecting the id again reaches the server: a stub left registered // would fail this connect as already registered. await framework.connectMcplServer(failed.discord); @@ -816,6 +839,58 @@ test('a runtime connect whose awareness accounting fails fails its gate, so a ru } }); +test('a runtime connect whose policy registration throws closes its connection for good and completes its gate, so a running server keeps its data', async () => { + mkdirSync(TEST_ROOT, { recursive: true }); + const dir = mkdtempSync(join(TEST_ROOT, 'awareness-runtime-policy-')); + const { config } = frameworkConfig(dir, { + STATUS_PATH: join(dir, 'status.jsonl'), + LEDGER_PATH: join(dir, 'awareness.json'), + GENERATION_PATH: join(dir, 'generation'), + }); + const discord = config.mcplServers[0]!; + let framework: AgentFramework | undefined; + + try { + framework = await AgentFramework.create({ ...config, mcplServers: [heartbeatConfig(dir)] }); + const traces: Array<{ type: string; serverId?: string; error?: string }> = []; + framework.onTrace((event) => traces.push(event as never)); + // Accounting succeeds; the §5.3 registration that follows it throws. + const prototype = FeatureSetManager.prototype as any; + const original = prototype.initializeServer; + prototype.initializeServer = function (this: unknown, serverId: string, ...rest: unknown[]) { + if (serverId === discord.id) throw new Error('injected policy registration failure'); + return original.call(this, serverId, ...rest); + }; + try { + await assert.rejects(framework.connectMcplServer(discord), /injected policy registration failure/); + } finally { + prototype.initializeServer = original; + } + + const internals = framework as unknown as { + discordAwarenessBarrier: unknown; + mcplServerRegistry: { getServer(id: string): unknown }; + }; + assert.equal(internals.discordAwarenessBarrier, null, 'the failed connect completed the gate it installed'); + const running = internals.mcplServerRegistry.getServer('heartbeat') as { isConnected: boolean; dataPlaneReady: boolean }; + assert.equal(running.isConnected, true, 'accounting succeeded, so the running server was not recycled'); + assert.equal(running.dataPlaneReady, true, 'the running server\'s data flows'); + assert.equal( + traces.some((trace) => trace.type === 'mcpl:server-error' && /accounting unhealthy/.test(trace.error ?? '')), + false, + 'no gate failed', + ); + assert.equal(internals.mcplServerRegistry.getServer(discord.id), null, 'the half-registered connection was closed for good'); + + // Connecting the id again reaches the server. + await framework.connectMcplServer(discord); + assert.equal(framework.listMcplServers().find((server) => server.id === discord.id)?.connected, true); + } finally { + await framework?.stop(); + rmSync(dir, { recursive: true, force: true }); + } +}); + test('runtime reconciliation failure recycles the connection and keeps fresh data gated', async () => { mkdirSync(TEST_ROOT, { recursive: true }); const dir = mkdtempSync(join(TEST_ROOT, 'awareness-runtime-accounting-')); diff --git a/test/shutdown-errors.test.ts b/test/shutdown-errors.test.ts index bfbc2433b..7fe241dcc 100644 --- a/test/shutdown-errors.test.ts +++ b/test/shutdown-errors.test.ts @@ -464,6 +464,20 @@ test('MCPL registry retains failed and pending servers and bounds concurrent att assert.equal(registry.getAllServers().length, 0); }); +test('MCPL registry closeServer leaves an entry that a newer connection holds under the same id', async () => { + const registry = new McplServerRegistry(); + const servers = (registry as unknown as { servers: Map }).servers; + let replacedCloses = 0; + const replaced = { close: async () => { replacedCloses++; } } as unknown as McplServerConnection; + const current = { close: async () => { throw new Error('the newer connection was closed'); } } as unknown as McplServerConnection; + // As when a restart has replaced the connection that a policy fallback or + // a failed connect is closing for good. + servers.set('restarted', current); + await registry.closeServer('restarted', replaced); + assert.equal(replacedCloses, 1, 'the connection asked about is closed'); + assert.equal(registry.getServer('restarted'), current, 'the newer connection keeps its entry'); +}); + test('public MCPL recovery serializes overlapping reconnects and close waits for late cleanup', async t => { t.mock.timers.enable({ apis: ['setTimeout'] }); From 767982287dbd372de8b51fe9e0b9d86f6c1d27c3 Mon Sep 17 00:00:00 2001 From: Effie-2756 Date: Sat, 17 Oct 2026 16:50:54 -0400 Subject: [PATCH 4/6] fix(mcpl): the policy exit's gate leaves the connection it closes paused; test a close that fails Answers the internal review of the previous commit. When a runtime connect's policy registration throws and the close that follows fails, the connection stays registered, as closeServer keeps it. Completing the gate then ready()d it along with every other connection. ready() doesn't check whether a connection is closed, so what it held that needs no grant would have reached listeners. completeMcplDataPlaneGate now takes a connection to leave paused, and the policy exit passes the one it is closing. Tests: the policy-registration test becomes a helper that runs twice. With a close that rejects once, the connect still rejects with the registration's error, the connection stays registered and paused, the gate is completed, and the failed close is logged. --- src/framework.ts | 7 ++- test/mcpl-awareness-barrier.test.ts | 88 +++++++++++++++++++++++------ 2 files changed, 78 insertions(+), 17 deletions(-) diff --git a/src/framework.ts b/src/framework.ts index ee72b5be6..f121824d7 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -17474,10 +17474,13 @@ export class AgentFramework { private completeMcplDataPlaneGate( barrier: DiscordAwarenessBarrier, + /** A connection to leave paused: one its connect is closing for good. */ + except?: McplServerConnection, ): boolean { if (this.discordAwarenessBarrier !== barrier) return false; this.discordAwarenessBarrier = null; for (const connection of this.mcplServerRegistry?.getAllServers() ?? []) { + if (connection === except) continue; // ready() can synchronously flush a nested list-change notification that // installs a newer global generation. Never let this older completion // release any remaining server behind that newer gate. (The quiesce @@ -17682,9 +17685,11 @@ export class AgentFramework { // server's data. So close the connection for good through the // registry, as the catch above does, then complete the gate rather // than fail it: accounting succeeded, so nothing calls for a recycle. + // The completion leaves this connection paused: a close that failed + // keeps it registered, and no grant was established for it. // Every exit of a runtime connect settles the gate it installed. await this.closeFailedRuntimeConnect(connection, 'policy registration'); - this.completeMcplDataPlaneGate(awarenessBarrier); + this.completeMcplDataPlaneGate(awarenessBarrier, connection); throw error; } this.completeMcplDataPlaneGate(awarenessBarrier); diff --git a/test/mcpl-awareness-barrier.test.ts b/test/mcpl-awareness-barrier.test.ts index 31443394b..bbb428028 100644 --- a/test/mcpl-awareness-barrier.test.ts +++ b/test/mcpl-awareness-barrier.test.ts @@ -839,34 +839,48 @@ test('a runtime connect whose awareness accounting fails fails its gate, so a ru } }); -test('a runtime connect whose policy registration throws closes its connection for good and completes its gate, so a running server keeps its data', async () => { - mkdirSync(TEST_ROOT, { recursive: true }); - const dir = mkdtempSync(join(TEST_ROOT, 'awareness-runtime-policy-')); +/** + * A framework serving `heartbeat`, then a runtime connect of `discord` whose + * awareness accounting succeeds and whose §5.3 policy registration throws + * (initializeServer made to throw for it). With `closeFails`, the close that + * follows rejects once, as a transport close that fails would. Returns the + * framework with both patches undone; a failure before then stops it. + */ +async function failPolicyRegistrationWhileServing(dir: string, closeFails: boolean) { const { config } = frameworkConfig(dir, { STATUS_PATH: join(dir, 'status.jsonl'), LEDGER_PATH: join(dir, 'awareness.json'), GENERATION_PATH: join(dir, 'generation'), }); const discord = config.mcplServers[0]!; - let framework: AgentFramework | undefined; - + const framework = await AgentFramework.create({ ...config, mcplServers: [heartbeatConfig(dir)] }); try { - framework = await AgentFramework.create({ ...config, mcplServers: [heartbeatConfig(dir)] }); const traces: Array<{ type: string; serverId?: string; error?: string }> = []; framework.onTrace((event) => traces.push(event as never)); - // Accounting succeeds; the §5.3 registration that follows it throws. - const prototype = FeatureSetManager.prototype as any; - const original = prototype.initializeServer; - prototype.initializeServer = function (this: unknown, serverId: string, ...rest: unknown[]) { + const features = FeatureSetManager.prototype as any; + const initializeServer = features.initializeServer; + features.initializeServer = function (this: unknown, serverId: string, ...rest: unknown[]) { if (serverId === discord.id) throw new Error('injected policy registration failure'); - return original.call(this, serverId, ...rest); + return initializeServer.call(this, serverId, ...rest); + }; + const connections = McplServerConnection.prototype as any; + const close = connections.close; + let closeFailed = false; + connections.close = function (this: McplServerConnection) { + if (closeFails && this.id === discord.id && !closeFailed) { + closeFailed = true; + return Promise.reject(new Error('injected close failure')); + } + return close.call(this); }; try { + // The connect reports its own failure, not the close's. await assert.rejects(framework.connectMcplServer(discord), /injected policy registration failure/); } finally { - prototype.initializeServer = original; + features.initializeServer = initializeServer; + connections.close = close; } - + assert.equal(closeFailed, closeFails, 'the close failed only when asked to'); const internals = framework as unknown as { discordAwarenessBarrier: unknown; mcplServerRegistry: { getServer(id: string): unknown }; @@ -880,11 +894,53 @@ test('a runtime connect whose policy registration throws closes its connection f false, 'no gate failed', ); - assert.equal(internals.mcplServerRegistry.getServer(discord.id), null, 'the half-registered connection was closed for good'); + return { framework, discord, registry: internals.mcplServerRegistry }; + } catch (error) { + await framework.stop(); + throw error; + } +} + +test('a runtime connect whose policy registration throws closes its connection for good and completes its gate, so a running server keeps its data', async () => { + mkdirSync(TEST_ROOT, { recursive: true }); + const dir = mkdtempSync(join(TEST_ROOT, 'awareness-runtime-policy-')); + let framework: AgentFramework | undefined; + + try { + const failed = await failPolicyRegistrationWhileServing(dir, false); + framework = failed.framework; + assert.equal(failed.registry.getServer(failed.discord.id), null, 'the half-registered connection was closed for good'); // Connecting the id again reaches the server. - await framework.connectMcplServer(discord); - assert.equal(framework.listMcplServers().find((server) => server.id === discord.id)?.connected, true); + await framework.connectMcplServer(failed.discord); + assert.equal(framework.listMcplServers().find((server) => server.id === failed.discord.id)?.connected, true); + } finally { + await framework?.stop(); + rmSync(dir, { recursive: true, force: true }); + } +}); + +test('a runtime connect whose policy registration throws and whose close then fails keeps that connection registered and paused, and logs the close', async (t) => { + mkdirSync(TEST_ROOT, { recursive: true }); + const dir = mkdtempSync(join(TEST_ROOT, 'awareness-runtime-policy-close-')); + const logged: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { logged.push(args.map(String).join(' ')); }); + let framework: AgentFramework | undefined; + + try { + const failed = await failPolicyRegistrationWhileServing(dir, true); + framework = failed.framework; + // A close that fails keeps the entry, so a later disconnect, restart or + // shutdown can close it again; the gate's completion leaves it paused, + // since no grant was established for it. + const refused = failed.registry.getServer(failed.discord.id) as { dataPlaneReady: boolean } | null; + assert.ok(refused, 'the connection whose close failed is still registered'); + assert.equal(refused.dataPlaneReady, false, 'the gate\'s completion left the refused connection paused'); + assert.ok( + logged.some((line) => line.includes(`could not close MCPL server "${failed.discord.id}" after its policy registration failed`) + && line.includes('injected close failure')), + 'the failed close was logged', + ); } finally { await framework?.stop(); rmSync(dir, { recursive: true, force: true }); From 8628705b8012bbdc3e39e9e3f8285221535383ef Mon Sep 17 00:00:00 2001 From: Effie-2756 Date: Sat, 17 Oct 2026 17:02:02 -0400 Subject: [PATCH 5/6] fix(mcpl): a connection closed for good readies nothing, in place of the completion's skip Answers the review of the previous commit. The previous commit left the connection a failed connect closes paused at that connect's own completion, but a later gate's completion, or a release of control traffic, still readied it: ready() and readyControlPlane() didn't check whether the connection was closed. The same held for any connection a failed close keeps registered, on main too. ready() and readyControlPlane() now return at once when the connection is closed for good: closed, with reconnect off. close() turns reconnect off and marks the connection closed before its transport close can fail, and a drop with reconnect off leaves it the same way. A connection between retries keeps reconnect on, so it still readies after its reconnect. completeMcplDataPlaneGate's skip goes. While the host stops, a connection closed for good, such as one closeAll has begun closing, no longer passes what it held to the stopping refusal, whose answers can't be sent on a closed connection anyway. What every other connection held reaches it as before. Tests: the failed-close run now fails the transport close, after close() has marked the connection, and checks that a later syncDiscordAwarenessMarkers() leaves it paused. A stop-path test holds a control event and a data event on two connections, closes one, and checks that releasing and then failing the gate while the host stops lets out only the other one's. --- changelog.d/mcpl-close-down-server.fixed.md | 7 +++ src/framework.ts | 10 ++-- src/mcpl/server-connection.ts | 16 +++++- test/mcpl-awareness-barrier.test.ts | 61 ++++++++++++++++++--- 4 files changed, 78 insertions(+), 16 deletions(-) diff --git a/changelog.d/mcpl-close-down-server.fixed.md b/changelog.d/mcpl-close-down-server.fixed.md index a3cc33faf..83091c4c9 100644 --- a/changelog.d/mcpl-close-down-server.fixed.md +++ b/changelog.d/mcpl-close-down-server.fixed.md @@ -27,3 +27,10 @@ closes the new server for good, where it stayed connected and half registered, and completes the gate, so the running servers' data flows on with no recycle. +- `McplServerConnection.ready()` and `readyControlPlane()` now do nothing once + the connection is closed for good: closed, with reconnect off, as after + `close()` or a drop with reconnect off. What it held stays held, where any + gate's completion let it out with no grant, as for a connection whose close + failed and so stayed registered. While the host stops, a connection being + closed no longer passes what it held to the stopping refusal; its server + learns of the close from the close itself. diff --git a/src/framework.ts b/src/framework.ts index f121824d7..6147b25f0 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -17474,13 +17474,10 @@ export class AgentFramework { private completeMcplDataPlaneGate( barrier: DiscordAwarenessBarrier, - /** A connection to leave paused: one its connect is closing for good. */ - except?: McplServerConnection, ): boolean { if (this.discordAwarenessBarrier !== barrier) return false; this.discordAwarenessBarrier = null; for (const connection of this.mcplServerRegistry?.getAllServers() ?? []) { - if (connection === except) continue; // ready() can synchronously flush a nested list-change notification that // installs a newer global generation. Never let this older completion // release any remaining server behind that newer gate. (The quiesce @@ -17685,11 +17682,12 @@ export class AgentFramework { // server's data. So close the connection for good through the // registry, as the catch above does, then complete the gate rather // than fail it: accounting succeeded, so nothing calls for a recycle. - // The completion leaves this connection paused: a close that failed - // keeps it registered, and no grant was established for it. + // A close that fails keeps the connection registered, but a connection + // closed for good readies nothing, so this completion, and any later + // one, leaves it paused. // Every exit of a runtime connect settles the gate it installed. await this.closeFailedRuntimeConnect(connection, 'policy registration'); - this.completeMcplDataPlaneGate(awarenessBarrier, connection); + this.completeMcplDataPlaneGate(awarenessBarrier); throw error; } this.completeMcplDataPlaneGate(awarenessBarrier); diff --git a/src/mcpl/server-connection.ts b/src/mcpl/server-connection.ts index 505bc2c1b..49ca6f3cd 100644 --- a/src/mcpl/server-connection.ts +++ b/src/mcpl/server-connection.ts @@ -315,6 +315,13 @@ export class McplServerConnection extends EventEmitter { return this.reconnectEnabled; } + /** Closed with no reconnect to come: after close(), whether or not its + * transport close succeeded, or a drop with reconnect off. A connection + * between retries keeps reconnectEnabled, so it isn't closed for good. */ + private get closedForGood(): boolean { + return this.closed && !this.reconnectEnabled; + } + /** * Private constructor. Use `McplServerConnection.connect()` instead. */ @@ -342,8 +349,13 @@ export class McplServerConnection extends EventEmitter { * Mark the connection as ready — flushes any events that arrived between * construction (when setupMessageRouting starts emitting) and now (when * the caller has attached listeners via wireMcplEvents). + * + * Does nothing once the connection is closed for good: its policy was + * reset when it closed, so what it held would flow with no grant, and its + * server learns of the close from the close itself. */ ready(): void { + if (this.closedForGood) return; this.controlPlaneReady = true; this.dataPlaneReady = true; const pending = this.bufferedEvents; @@ -375,9 +387,11 @@ export class McplServerConnection extends EventEmitter { * Release only server-establishment and lifecycle/control traffic. Responses * to host requests are never event-buffered, so tools/list and tools/call * responses remain live as well. Inference-bearing requests stay queued in - * their original relative order until {@link ready} is called. + * their original relative order until {@link ready} is called. Like + * {@link ready}, it does nothing once the connection is closed for good. */ readyControlPlane(): void { + if (this.closedForGood) return; this.controlPlaneReady = true; const pending = this.bufferedEvents; this.bufferedEvents = []; diff --git a/test/mcpl-awareness-barrier.test.ts b/test/mcpl-awareness-barrier.test.ts index bbb428028..146f1846a 100644 --- a/test/mcpl-awareness-barrier.test.ts +++ b/test/mcpl-awareness-barrier.test.ts @@ -843,8 +843,9 @@ test('a runtime connect whose awareness accounting fails fails its gate, so a ru * A framework serving `heartbeat`, then a runtime connect of `discord` whose * awareness accounting succeeds and whose §5.3 policy registration throws * (initializeServer made to throw for it). With `closeFails`, the close that - * follows rejects once, as a transport close that fails would. Returns the - * framework with both patches undone; a failure before then stops it. + * follows fails once in its transport close, after close() has marked the + * connection closed for good, as a real one fails. Returns the framework with + * every patch undone; a failure before then stops it. */ async function failPolicyRegistrationWhileServing(dir: string, closeFails: boolean) { const { config } = frameworkConfig(dir, { @@ -866,10 +867,15 @@ async function failPolicyRegistrationWhileServing(dir: string, closeFails: boole const connections = McplServerConnection.prototype as any; const close = connections.close; let closeFailed = false; - connections.close = function (this: McplServerConnection) { - if (closeFails && this.id === discord.id && !closeFailed) { + connections.close = function (this: any) { + if (closeFails && this.id === discord.id && !closeFailed && this.transport) { closeFailed = true; - return Promise.reject(new Error('injected close failure')); + const transport = this.transport; + const transportClose = transport.close; + transport.close = () => { + transport.close = transportClose; + return Promise.reject(new Error('injected close failure')); + }; } return close.call(this); }; @@ -920,7 +926,7 @@ test('a runtime connect whose policy registration throws closes its connection f } }); -test('a runtime connect whose policy registration throws and whose close then fails keeps that connection registered and paused, and logs the close', async (t) => { +test('a runtime connect whose policy registration throws and whose close then fails keeps that connection registered and paused through later gates, and logs the close', async (t) => { mkdirSync(TEST_ROOT, { recursive: true }); const dir = mkdtempSync(join(TEST_ROOT, 'awareness-runtime-policy-close-')); const logged: string[] = []; @@ -931,11 +937,14 @@ test('a runtime connect whose policy registration throws and whose close then fa const failed = await failPolicyRegistrationWhileServing(dir, true); framework = failed.framework; // A close that fails keeps the entry, so a later disconnect, restart or - // shutdown can close it again; the gate's completion leaves it paused, - // since no grant was established for it. - const refused = failed.registry.getServer(failed.discord.id) as { dataPlaneReady: boolean } | null; + // shutdown can close it again. Closed for good, it readies nothing: not + // at this connect's completion, and not at a later gate's. + const refused = failed.registry.getServer(failed.discord.id) as { dataPlaneReady: boolean; willReconnect: boolean } | null; assert.ok(refused, 'the connection whose close failed is still registered'); + assert.equal(refused.willReconnect, false, 'its close had disarmed it before the transport close failed'); assert.equal(refused.dataPlaneReady, false, 'the gate\'s completion left the refused connection paused'); + await failed.framework.syncDiscordAwarenessMarkers(); + assert.equal(refused.dataPlaneReady, false, 'a later gate\'s completion leaves it paused too'); assert.ok( logged.some((line) => line.includes(`could not close MCPL server "${failed.discord.id}" after its policy registration failed`) && line.includes('injected close failure')), @@ -1064,6 +1073,40 @@ test('a control flush that installs a newer generation cannot be released by the assert.equal(readyCalls, 1); }); +test('while the host stops, a failed gate lets out what a running connection held, and nothing of one closed for good', async () => { + const framework = Object.create(AgentFramework.prototype) as any; + framework.discordAwarenessBarrier = null; + framework.discordAwarenessBarrierGeneration = 0; + framework.beginDiscordAwarenessBarrier = () => ({ requiresBarrier: true, promise: new Promise(() => {}) }); + framework.queue = { isClosed: true }; // stop() has closed the queue + framework.emitTrace = () => {}; + const running = Reflect.construct(McplServerConnection, ['running', null, null]) as McplServerConnection; + const closing = Reflect.construct(McplServerConnection, ['closing', null, null]) as McplServerConnection; + framework.mcplServerRegistry = { getAllServers: () => [running, closing] }; + const delivered: string[] = []; + for (const connection of [running, closing]) { + connection.on('stderr', () => delivered.push(`${connection.id} control`)); + connection.on('held-data', () => delivered.push(`${connection.id} data`)); + } + + // Each connection holds a control event and a data event behind the gate. + const barrier = framework.installMcplDataPlaneGate(); + for (const connection of [running, closing]) { + connection.emit('stderr', 'a line'); + connection.emit('held-data', {}); + } + assert.deepEqual(delivered, []); + + // closeAll has begun closing one of them: close() closes it for good. + await closing.close(); + framework.releaseMcplDataPlaneGate(barrier); + assert.deepEqual(delivered, ['running control'], 'only the running connection lets out its control traffic'); + await framework.failMcplDataPlaneGate(barrier, 'test sync', new Error('accounting failed')); + assert.equal(framework.discordAwarenessBarrier, null, 'the stopping host released the failed gate'); + assert.deepEqual(delivered, ['running control', 'running data'], + 'the running connection\'s held data reaches its listeners, and the closed one lets out nothing'); +}); + test('overlapping reconnect/list-change generations release only the newest barrier', async () => { const reconnectDrain = deferred(); const listChangeDrain = deferred(); From 5822b438a19457e5d19d57172cc89ad5aaf0b409 Mon Sep 17 00:00:00 2001 From: Effie-2756 Date: Sat, 17 Oct 2026 17:24:38 -0400 Subject: [PATCH 6/6] fix(mcpl): say what a connection closed for good would let out; test a close still under way Answers the internal review of the previous commit. ready()'s comment gave the reason for doing nothing once a connection is closed for good as its server learning of the close from the close itself, which a close that fails doesn't give. The reason that holds is that nothing it held could be answered: a closed connection sends no response, and its reset policy refuses what needs a grant. Readying it would only pass what needs no grant to listeners of a server that won't be served again. The comment now says so, and that a connection between retries still readies, so a reconnect stub's connect-failed reaches its listeners. failMcplDataPlaneGate's comment no longer says everything the pause held back reaches the stopping refusal. The changelog fragment states the stopping case for every connection closed for good, not only one being closed, and no longer says the running servers' data flows once a gate completes, which a quiesced host doesn't do. Tests: the stop-path test starts closing a connection without waiting for the close, so the gate is released and then failed while that close is still under way, the state a connection closeAll has begun closing is in. --- changelog.d/mcpl-close-down-server.fixed.md | 22 ++++++++++----------- src/framework.ts | 3 ++- src/mcpl/server-connection.ts | 10 +++++++--- test/mcpl-awareness-barrier.test.ts | 9 ++++++--- 4 files changed, 26 insertions(+), 18 deletions(-) diff --git a/changelog.d/mcpl-close-down-server.fixed.md b/changelog.d/mcpl-close-down-server.fixed.md index 83091c4c9..635a3d0b4 100644 --- a/changelog.d/mcpl-close-down-server.fixed.md +++ b/changelog.d/mcpl-close-down-server.fixed.md @@ -19,18 +19,18 @@ - When its Discord awareness accounting fails, it now fails that gate, as a failed reconnect, tools-list reconciliation, `syncDiscordAwarenessMarkers()` or `resume()` already does: each running - server is recycled, and its data flows once its reconnect's accounting - succeeds. A server without `reconnect: true` is closed for good there, - as on those paths. While the host is stopping, the gate is released - instead. + server is recycled, and its reconnect's own gate holds its data until + that reconnect's accounting succeeds. A server without `reconnect: true` + is closed for good there, as on those paths. While the host is stopping, + the gate is released instead. - When its policy registration throws after accounting succeeded, it now closes the new server for good, where it stayed connected and half - registered, and completes the gate, so the running servers' data flows - on with no recycle. + registered, and completes the gate, with no recycle. - `McplServerConnection.ready()` and `readyControlPlane()` now do nothing once the connection is closed for good: closed, with reconnect off, as after - `close()` or a drop with reconnect off. What it held stays held, where any - gate's completion let it out with no grant, as for a connection whose close - failed and so stayed registered. While the host stops, a connection being - closed no longer passes what it held to the stopping refusal; its server - learns of the close from the close itself. + `close()` or a drop with reconnect off. What it held stays held. Any gate's + completion used to pass the part of it that needs no grant to listeners, + as for a connection whose close failed and so stayed registered. While the + host stops, a connection closed for good, such as one being closed, no + longer passes what it held to the stopping refusal, whose answers can't be + sent on a closed connection. diff --git a/src/framework.ts b/src/framework.ts index 6147b25f0..0f4e79003 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -17520,7 +17520,8 @@ export class AgentFramework { // would start reconnects only for stop() to close them again. Release the // barrier instead, as a completed one is released: nothing waits on a // rejected barrier, and unless the host is quiesced, what the pause held - // back reaches the listeners' stopping refusal. + // back reaches the listeners' stopping refusal, except what a connection + // closed for good held, which stays held. if (this.queue?.isClosed) { this.completeMcplDataPlaneGate(barrier); return; diff --git a/src/mcpl/server-connection.ts b/src/mcpl/server-connection.ts index 49ca6f3cd..bb105f53b 100644 --- a/src/mcpl/server-connection.ts +++ b/src/mcpl/server-connection.ts @@ -350,9 +350,13 @@ export class McplServerConnection extends EventEmitter { * construction (when setupMessageRouting starts emitting) and now (when * the caller has attached listeners via wireMcplEvents). * - * Does nothing once the connection is closed for good: its policy was - * reset when it closed, so what it held would flow with no grant, and its - * server learns of the close from the close itself. + * Does nothing once the connection is closed for good. Its policy was reset + * when it closed, so what it held that needs a grant would be refused, and a + * closed connection sends no response, so nothing it held could be answered. + * Only what needs no grant would pass, to listeners of a server that won't + * be served again. A connection between retries isn't closed for good: what + * it held, such as a reconnect stub's connect-failed, still reaches its + * listeners. */ ready(): void { if (this.closedForGood) return; diff --git a/test/mcpl-awareness-barrier.test.ts b/test/mcpl-awareness-barrier.test.ts index 146f1846a..6277c0797 100644 --- a/test/mcpl-awareness-barrier.test.ts +++ b/test/mcpl-awareness-barrier.test.ts @@ -1097,14 +1097,17 @@ test('while the host stops, a failed gate lets out what a running connection hel } assert.deepEqual(delivered, []); - // closeAll has begun closing one of them: close() closes it for good. - await closing.close(); + // closeAll has begun closing one of them, and that close is still under + // way while the gate is released and then failed: close() closes the + // connection for good as it starts. + const closed = closing.close(); framework.releaseMcplDataPlaneGate(barrier); assert.deepEqual(delivered, ['running control'], 'only the running connection lets out its control traffic'); await framework.failMcplDataPlaneGate(barrier, 'test sync', new Error('accounting failed')); assert.equal(framework.discordAwarenessBarrier, null, 'the stopping host released the failed gate'); assert.deepEqual(delivered, ['running control', 'running data'], - 'the running connection\'s held data reaches its listeners, and the closed one lets out nothing'); + 'the running connection\'s held data reaches its listeners, and the closing one lets out nothing'); + await closed; }); test('overlapping reconnect/list-change generations release only the newest barrier', async () => {