Skip to content

Commit 844f4e0

Browse files
shebsonclaude
shebson
and
claude
committed
fix(mcp): dispose previous in-flight runtime and treat ttl=0 as unbounded
Two narrow fixes to the bundled MCP runtime cache: 1. When a sessionId moves to a different runtimeKey (workspace, fingerprint, or scope changed) while the previous key still has an in-flight create promise, wait for that promise to settle, drop the orphan entry written by its .then, and dispose the runtime. Previously the createInFlight tracker was deleted but the pending promise still resolved and stored the runtime under previousRuntimeKey with no attached sessions, leaking an MCP child until a later sweep (or indefinitely if no sweep timer). 2. Treat sessionIdleTtlMs=0 as unbounded when merging shared-runtime TTLs. The sweep already skips runtimes with ttl<=0; the TTL-merge function was comparing as plain numbers, so an attaching session with a finite TTL would override a prior session that had disabled eviction. Now 0 wins over any nonzero TTL and can't be displaced. Adds two regression tests, both verified to fail without the matching fix. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
1 parent ad89d0b commit 844f4e0

2 files changed

Lines changed: 115 additions & 3 deletions

File tree

‎src/agents/agent-bundle-mcp-runtime.test.ts‎

Lines changed: 97 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1647,7 +1647,7 @@ process.on("SIGINT", shutdown);`,
16471647
},
16481648
};
16491649
};
1650-
const manager = __testing.createSessionMcpRuntimeManager({
1650+
const manager = testing.createSessionMcpRuntimeManager({
16511651
createRuntime,
16521652
now: () => now,
16531653
enableIdleSweepTimer: false,
@@ -1902,6 +1902,102 @@ process.on("SIGINT", shutdown);`,
19021902
// Both sessions are detached when the shared runtime is evicted.
19031903
expect(manager.listSessionIds()).toEqual([]);
19041904
});
1905+
1906+
it("preserves a disabled TTL when another session attaches with a finite TTL", async () => {
1907+
let now = 1_000;
1908+
const disposed: string[] = [];
1909+
const createRuntime: RuntimeFactory = (params) => {
1910+
let lastUsedAt = now;
1911+
return {
1912+
...makeRuntime([{ toolName: "bundle_probe", description: "Bundle MCP probe" }]),
1913+
sessionId: params.sessionId,
1914+
sessionKey: params.sessionKey,
1915+
workspaceDir: params.workspaceDir,
1916+
configFingerprint: params.configFingerprint ?? "fingerprint",
1917+
get lastUsedAt() {
1918+
return lastUsedAt;
1919+
},
1920+
markUsed: () => {
1921+
lastUsedAt = now;
1922+
},
1923+
dispose: async () => {
1924+
disposed.push(params.sessionId);
1925+
},
1926+
};
1927+
};
1928+
const manager = testing.createSessionMcpRuntimeManager({
1929+
createRuntime,
1930+
now: () => now,
1931+
enableIdleSweepTimer: false,
1932+
});
1933+
1934+
// First attacher disables idle eviction (TTL=0).
1935+
await manager.getOrCreate({
1936+
sessionId: "session-zero-ttl",
1937+
workspaceDir: "/workspace",
1938+
cfg: { mcp: { servers: {}, runtimeScope: "shared", sessionIdleTtlMs: 0 } },
1939+
});
1940+
// Second attacher has a finite TTL. The shared runtime must keep TTL=0
1941+
// (unbounded) so the disabling session's expectation is honored.
1942+
await manager.getOrCreate({
1943+
sessionId: "session-finite-ttl",
1944+
workspaceDir: "/workspace",
1945+
cfg: { mcp: { servers: {}, runtimeScope: "shared", sessionIdleTtlMs: 60 } },
1946+
});
1947+
1948+
now += 60_000_000;
1949+
await expect(manager.sweepIdleRuntimes()).resolves.toBe(0);
1950+
expect(disposed).toEqual([]);
1951+
});
1952+
1953+
it("disposes a previous in-flight runtime when a session moves to a new key", async () => {
1954+
const now = 1_000;
1955+
const created: string[] = [];
1956+
const disposed: string[] = [];
1957+
const createRuntime: RuntimeFactory = (params) => {
1958+
created.push(params.sessionId);
1959+
return {
1960+
...makeRuntime([{ toolName: "bundle_probe", description: "Bundle MCP probe" }]),
1961+
sessionId: params.sessionId,
1962+
sessionKey: params.sessionKey,
1963+
workspaceDir: params.workspaceDir,
1964+
configFingerprint: params.configFingerprint ?? "fingerprint",
1965+
dispose: async () => {
1966+
disposed.push(params.sessionId);
1967+
},
1968+
};
1969+
};
1970+
const manager = testing.createSessionMcpRuntimeManager({
1971+
createRuntime,
1972+
now: () => now,
1973+
enableIdleSweepTimer: false,
1974+
});
1975+
1976+
// Concurrent calls for the same sessionId across different scopes. The
1977+
// session-scope call sets an in-flight runtime under sessionId="A"; the
1978+
// shared-scope call moves the session to a new runtimeKey before that
1979+
// in-flight has landed. The previous in-flight runtime must be awaited
1980+
// and disposed, not orphaned in runtimesByKey.
1981+
const sessionScopeCall = manager.getOrCreate({
1982+
sessionId: "A",
1983+
workspaceDir: "/workspace",
1984+
cfg: { mcp: { servers: {} } },
1985+
});
1986+
const sharedScopeCall = manager.getOrCreate({
1987+
sessionId: "A",
1988+
workspaceDir: "/workspace",
1989+
cfg: { mcp: { servers: {}, runtimeScope: "shared" } },
1990+
});
1991+
1992+
await Promise.all([sessionScopeCall, sharedScopeCall]);
1993+
1994+
// Two runtimes were created (one per scope). The session-scoped one,
1995+
// identified by sessionId="A", was awaited and disposed by the shared call.
1996+
expect(created).toHaveLength(2);
1997+
expect(disposed).toEqual(["A"]);
1998+
// Only the shared runtime remains attached to the session.
1999+
expect(manager.listSessionIds()).toEqual(["A"]);
2000+
});
19052001
});
19062002

19072003
describe("disposeSession timeout", () => {

‎src/agents/agent-bundle-mcp-runtime.ts‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -938,9 +938,14 @@ function createSessionMcpRuntimeManager(
938938

939939
const bumpIdleTtlForRuntimeKey = (runtimeKey: string, idleTtlMs: number) => {
940940
// Take the max across attached sessions so one strict-TTL session can't
941-
// evict a runtime another session expected to keep alive.
941+
// evict a runtime another session expected to keep alive. The sweep treats
942+
// ttl <= 0 as "never evict", so 0 is unbounded: it wins over any nonzero
943+
// TTL and can't be overridden by a later attaching session.
942944
const previous = idleTtlMsByRuntimeKey.get(runtimeKey);
943-
if (previous === undefined || idleTtlMs > previous) {
945+
if (previous === 0) {
946+
return;
947+
}
948+
if (idleTtlMs === 0 || previous === undefined || idleTtlMs > previous) {
944949
idleTtlMsByRuntimeKey.set(runtimeKey, idleTtlMs);
945950
}
946951
};
@@ -1055,6 +1060,7 @@ function createSessionMcpRuntimeManager(
10551060
if (previousRuntimeKey && previousRuntimeKey !== desiredRuntimeKey) {
10561061
detachSessionFromRuntimeKey(params.sessionId, previousRuntimeKey);
10571062
if ((sessionIdsByRuntimeKey.get(previousRuntimeKey)?.size ?? 0) === 0) {
1063+
const stalePending = createInFlight.get(previousRuntimeKey);
10581064
const stale = runtimesByKey.get(previousRuntimeKey);
10591065
createInFlight.delete(previousRuntimeKey);
10601066
runtimesByKey.delete(previousRuntimeKey);
@@ -1063,6 +1069,16 @@ function createSessionMcpRuntimeManager(
10631069
if (stale) {
10641070
await stale.dispose();
10651071
}
1072+
if (stalePending) {
1073+
// The pending promise's .then writes the runtime into runtimesByKey
1074+
// under previousRuntimeKey. Wait for it to settle, drop the orphan
1075+
// entry, and dispose the runtime so we don't leak an MCP child.
1076+
const pendingRuntime = await stalePending.promise.catch(() => undefined);
1077+
runtimesByKey.delete(previousRuntimeKey);
1078+
if (pendingRuntime) {
1079+
await pendingRuntime.dispose();
1080+
}
1081+
}
10661082
}
10671083
}
10681084

0 commit comments

Comments
 (0)