diff --git a/.changeset/fix-ajv-validator-cache.md b/.changeset/fix-ajv-validator-cache.md new file mode 100644 index 0000000000..89f8df3255 --- /dev/null +++ b/.changeset/fix-ajv-validator-cache.md @@ -0,0 +1,5 @@ +--- +'@modelcontextprotocol/core-internal': patch +--- + +Fix a memory leak in the validator providers: schemas without an `$id` were recompiled (or re-instantiated) on every `getValidator()` call and retained forever, so long-running clients that periodically refresh their tool catalogue (e.g. via `Client.listTools()`) grew the heap without bound. Identical schemas are now cached by their canonical serialization (key order independent), each distinct schema compiles at most once, compiled snapshots defeat Ajv's identity-based cache on in-place mutation, and the cache is FIFO-bounded so schemas that genuinely come and go cannot accumulate forever. Applies to both `AjvJsonSchemaValidator` and `CfWorkerJsonSchemaValidator` (the latter keys by schema + draft). diff --git a/packages/core-internal/src/validators/ajvProvider.ts b/packages/core-internal/src/validators/ajvProvider.ts index e33adb741f..8845be372d 100644 --- a/packages/core-internal/src/validators/ajvProvider.ts +++ b/packages/core-internal/src/validators/ajvProvider.ts @@ -8,6 +8,7 @@ import { Ajv2020 } from 'ajv/dist/2020.js'; import _addFormats from 'ajv-formats'; import { declaredDialect } from './dialects'; +import { canonicalJson, createBoundedCache, VALIDATOR_CACHE_LIMIT } from './schemaCache'; import type { JsonSchemaType, JsonSchemaValidator, jsonSchemaValidator, JsonSchemaValidatorResult } from './types'; /** Structural subset of the AJV interface used by {@link AjvJsonSchemaValidator}. */ @@ -81,6 +82,15 @@ export class AjvJsonSchemaValidator implements jsonSchemaValidator { private _ajvDraft7: AjvLike | undefined; /** Lazy 2019-09 engine, built on the first 2019-09-declared schema. */ private _ajv2019: AjvLike | undefined; + /** + * Compiled validators for schemas without a usable `$id`, keyed by the + * schema's canonical serialization and bounded to {@link VALIDATOR_CACHE_LIMIT} + * distinct schemas. AJV only deduplicates compilations by `$id`; without + * this cache every `getValidator` call recompiles the same schema and the + * engine retains each compiled validator forever, so long-running clients + * that refresh their tool catalogue grow the heap without bound (#2605). + */ + private readonly _compiledBySource = createBoundedCache(VALIDATOR_CACHE_LIMIT); /** True iff the constructor received a caller-supplied engine; the `$schema` dispatch is skipped. */ private readonly _userAjv: boolean; @@ -133,7 +143,7 @@ export class AjvJsonSchemaValidator implements jsonSchemaValidator { const ajvValidator = '$id' in schema && typeof schema.$id === 'string' ? (engine.getSchema(schema.$id) ?? engine.compile(schema)) - : engine.compile(schema); + : this._compiledValidator(engine, schema); return (input: unknown): JsonSchemaValidatorResult => { const valid = ajvValidator(input); @@ -151,6 +161,30 @@ export class AjvJsonSchemaValidator implements jsonSchemaValidator { }; }; } + + /** Compile a schema, reusing the previous compilation for identical schemas. */ + private _compiledValidator(engine: AjvLike, schema: JsonSchemaType): AjvValidateFunction { + const key = canonicalJson(schema); + if (key === undefined) { + // Non-serializable schema (e.g. cyclic): skip the content cache and + // let the engine compile it as before. + return engine.compile(schema); + } + + const cached = this._compiledBySource.get(key); + if (cached !== undefined) { + return cached; + } + + // Compile a snapshot derived from the key, never the caller's object. + // AJV caches compiled schemas by object identity, so if a caller + // mutates its schema in place, compiling the live object could return + // a stale validator that we would then store under the fresh key + // (#2605). + const compiled = engine.compile(JSON.parse(key) as JsonSchemaType); + this._compiledBySource.set(key, compiled); + return compiled; + } } /** diff --git a/packages/core-internal/src/validators/cfWorkerProvider.ts b/packages/core-internal/src/validators/cfWorkerProvider.ts index fe876bf9b6..1c83673468 100644 --- a/packages/core-internal/src/validators/cfWorkerProvider.ts +++ b/packages/core-internal/src/validators/cfWorkerProvider.ts @@ -11,6 +11,7 @@ import { Validator } from '@cfworker/json-schema'; import { declaredDialect } from './dialects'; +import { canonicalJson, createBoundedCache, VALIDATOR_CACHE_LIMIT } from './schemaCache'; import type { JsonSchemaType, JsonSchemaValidator, jsonSchemaValidator, JsonSchemaValidatorResult } from './types'; /** @@ -52,6 +53,14 @@ export class CfWorkerJsonSchemaValidator implements jsonSchemaValidator { private readonly shortcircuit: boolean; /** Caller-supplied draft; when set, the `$schema` check is skipped (caller owns dialect). */ private readonly draft?: CfWorkerSchemaDraft; + /** + * Instantiated validators by canonical schema + draft, bounded to + * {@link VALIDATOR_CACHE_LIMIT} distinct entries. `@cfworker/json-schema` + * compiles a fresh `Validator` per construction, so without caching + * repeated `getValidator` calls with identical schemas recompile each + * time and retain every instance (#2605). + */ + private readonly _validators = createBoundedCache(VALIDATOR_CACHE_LIMIT); /** * Create a validator @@ -87,8 +96,22 @@ export class CfWorkerJsonSchemaValidator implements jsonSchemaValidator { */ getValidator(schema: JsonSchemaType): JsonSchemaValidator { const draft = this.draft ?? this._draftFor(schema); - // Cast to the cfworker Schema type - our JsonSchemaType is structurally compatible - const validator = new Validator(schema as ConstructorParameters[0], draft, this.shortcircuit); + const key = canonicalJson(schema); + let validator: Validator; + if (key === undefined) { + // Non-serializable schema (e.g. cyclic): skip the cache entirely. + validator = new Validator(schema as ConstructorParameters[0], draft, this.shortcircuit); + } else { + const cacheKey = `${draft}:${key}`; + const cached = this._validators.get(cacheKey); + if (cached === undefined) { + // Cast to the cfworker Schema type - our JsonSchemaType is structurally compatible + validator = new Validator(schema as ConstructorParameters[0], draft, this.shortcircuit); + this._validators.set(cacheKey, validator); + } else { + validator = cached; + } + } return (input: unknown): JsonSchemaValidatorResult => { const result = validator.validate(input); diff --git a/packages/core-internal/src/validators/schemaCache.ts b/packages/core-internal/src/validators/schemaCache.ts new file mode 100644 index 0000000000..62f69d5529 --- /dev/null +++ b/packages/core-internal/src/validators/schemaCache.ts @@ -0,0 +1,68 @@ +/** + * Shared bounded-cache helpers for validator providers. + * + * Both `AjvJsonSchemaValidator` and `CfWorkerJsonSchemaValidator` compile + * (or instantiate) a validator per distinct schema. Without caching, repeated + * calls with identical schemas recompile every time; without a bound, a + * caller whose schemas genuinely evolve keeps every distinct schema ever + * seen. See #2605. + */ + +/** Number of distinct schemas a provider keeps compiled before evicting the oldest. */ +export const VALIDATOR_CACHE_LIMIT = 1000; + +/** Recursively sort object keys so structurally equal JSON has one representation. */ +export function sortJsonKeys(value: unknown): unknown { + if (Array.isArray(value)) { + return value.map(item => sortJsonKeys(item)); + } + if (value !== null && typeof value === 'object') { + return Object.fromEntries( + Object.keys(value as Record) + .toSorted() + .map(key => [key, sortJsonKeys((value as Record)[key])]) + ); + } + return value; +} + +/** + * Canonical cache key for a schema: JSON-serialized with object keys + * recursively sorted, so structurally identical schemas (regardless of key + * order or object identity) share one entry. Returns `undefined` for schemas + * that cannot be serialized (e.g. cyclic objects). + */ +export function canonicalJson(value: unknown): string | undefined { + try { + return JSON.stringify(sortJsonKeys(value)); + } catch { + return undefined; + } +} + +/** + * FIFO-bounded string-keyed cache. Evicts the oldest entry once `limit` is + * exceeded, so the cache cannot grow without bound. FIFO (rather than LRU) is + * deliberate: schema catalogs are refreshed wholesale, so recency is not a + * reliable signal of reuse. + */ +export function createBoundedCache(limit: number): { + get(key: string): T | undefined; + set(key: string, value: T): void; +} { + const entries = new Map(); + return { + get(key: string): T | undefined { + return entries.get(key); + }, + set(key: string, value: T): void { + entries.set(key, value); + if (entries.size > limit) { + const oldest = entries.keys().next(); + if (!oldest.done) { + entries.delete(oldest.value); + } + } + } + }; +} diff --git a/packages/core-internal/test/validators/ajvProviderCache.test.ts b/packages/core-internal/test/validators/ajvProviderCache.test.ts new file mode 100644 index 0000000000..958a4b5d19 --- /dev/null +++ b/packages/core-internal/test/validators/ajvProviderCache.test.ts @@ -0,0 +1,128 @@ +import { describe, expect, it } from 'vitest'; + +import { AjvJsonSchemaValidator } from '../../src/validators/ajvProvider'; + +/** + * Regression tests for #2605: `getValidator()` must not recompile a schema + * that has no `$id` on every call. Repeated `Client.listTools()` refreshes + * call `getValidator` with structurally identical schemas; without caching, + * each call compiles a fresh validator that the AJV engine retains forever, + * so the heap grows without bound in long-running clients. + */ + +function makeFakeEngine() { + let compiles = 0; + let getSchemaCalls = 0; + const engine = { + compile: () => { + compiles += 1; + return Object.assign(() => true, { errors: undefined }); + }, + getSchema: () => { + getSchemaCalls += 1; + // eslint-disable-next-line unicorn/no-useless-undefined -- AjvLike.getSchema must return undefined + return undefined; + }, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + errorsText: (_errors?: any) => '' + }; + return { engine, compileCount: () => compiles, getSchemaCount: () => getSchemaCalls }; +} + +describe('AjvJsonSchemaValidator validator caching', () => { + it('compiles a schema without $id only once across repeated getValidator calls', () => { + const { engine, compileCount } = makeFakeEngine(); + const provider = new AjvJsonSchemaValidator(engine); + const schema = { type: 'object', properties: { a: { type: 'string' } } }; + + provider.getValidator(schema); + provider.getValidator(schema); + + expect(compileCount()).toBe(1); + }); + + it('hits the cache for structurally identical schemas with different object identity', () => { + const { engine, compileCount } = makeFakeEngine(); + const provider = new AjvJsonSchemaValidator(engine); + + provider.getValidator({ type: 'object', properties: { a: { type: 'string' } } }); + provider.getValidator({ type: 'object', properties: { a: { type: 'string' } } }); + + expect(compileCount()).toBe(1); + }); + + it('compiles distinct schemas independently', () => { + const { engine, compileCount } = makeFakeEngine(); + const provider = new AjvJsonSchemaValidator(engine); + + provider.getValidator({ type: 'string' }); + provider.getValidator({ type: 'number' }); + provider.getValidator({ type: 'object' }); + + expect(compileCount()).toBe(3); + }); + + it('shares one compilation for schemas that differ only in key order', () => { + const { engine, compileCount } = makeFakeEngine(); + const provider = new AjvJsonSchemaValidator(engine); + + provider.getValidator({ type: 'object', properties: { a: { type: 'string' } } }); + provider.getValidator({ properties: { a: { type: 'string' } }, type: 'object' }); + + expect(compileCount()).toBe(1); + }); + + it('is not poisoned when a caller mutates its schema object in place', () => { + const { engine, compileCount } = makeFakeEngine(); + const provider = new AjvJsonSchemaValidator(engine); + + const schema: Record = { + type: 'object', + properties: { a: { type: 'string' } } + }; + provider.getValidator(schema); + expect(compileCount()).toBe(1); + + // Mutate the same object in place — a fresh content key must trigger a + // fresh compilation instead of reusing the stale validator for the + // original content. + schema.properties = { b: { type: 'number' } }; + provider.getValidator(schema); + + expect(compileCount()).toBe(2); + }); + + it('falls back to compiling without caching when a schema is not serializable', () => { + const { engine, compileCount } = makeFakeEngine(); + const provider = new AjvJsonSchemaValidator(engine); + + const cyclic: Record = { type: 'object' }; + cyclic.self = cyclic; + + expect(() => provider.getValidator(cyclic)).not.toThrow(); + expect(compileCount()).toBe(1); + }); + + it('keeps using the $id-based lookup for schemas with an $id', () => { + const { engine, compileCount, getSchemaCount } = makeFakeEngine(); + const provider = new AjvJsonSchemaValidator(engine); + const schema = { $id: 'https://example.com/schema', type: 'string' }; + + provider.getValidator(schema); + provider.getValidator(schema); + + expect(getSchemaCount()).toBe(2); + // getSchema always misses in the fake engine, so each call compiles once — + // the $id path is unchanged by this fix. + expect(compileCount()).toBe(2); + }); + + it('returns a working validator after caching', () => { + const provider = new AjvJsonSchemaValidator(); + const schema = { type: 'object', properties: { a: { type: 'string' } } }; + + const validate = provider.getValidator<{ a?: string }>(schema); + expect(validate({ a: 'x' }).valid).toBe(true); + expect(validate({ a: 1 }).valid).toBe(false); + }); +}); diff --git a/packages/core-internal/test/validators/schemaCache.test.ts b/packages/core-internal/test/validators/schemaCache.test.ts new file mode 100644 index 0000000000..ab5a0eeab8 --- /dev/null +++ b/packages/core-internal/test/validators/schemaCache.test.ts @@ -0,0 +1,73 @@ +import { describe, expect, it } from 'vitest'; + +import { canonicalJson, createBoundedCache } from '../../src/validators/schemaCache'; + +describe('createBoundedCache', () => { + it('returns undefined for missing keys', () => { + const cache = createBoundedCache(10); + expect(cache.get('nope')).toBeUndefined(); + }); + + it('stores and returns values', () => { + const cache = createBoundedCache(10); + cache.set('a', '1'); + expect(cache.get('a')).toBe('1'); + }); + + it('evicts the oldest entry beyond the limit (FIFO)', () => { + const cache = createBoundedCache(3); + cache.set('a', '1'); + cache.set('b', '2'); + cache.set('c', '3'); + cache.set('d', '4'); // evicts 'a' + + expect(cache.get('a')).toBeUndefined(); + expect(cache.get('b')).toBe('2'); + expect(cache.get('c')).toBe('3'); + expect(cache.get('d')).toBe('4'); + }); + + it('keeps only the most recent entries after repeated overflow', () => { + const cache = createBoundedCache(2); + for (let i = 0; i < 5; i++) { + cache.set(`k${i}`, String(i)); + } + expect(cache.get('k0')).toBeUndefined(); + expect(cache.get('k3')).toBe('3'); + expect(cache.get('k4')).toBe('4'); + }); + + it('overwriting an existing key does not count as a new entry', () => { + const cache = createBoundedCache(2); + cache.set('a', '1'); + cache.set('b', '2'); + cache.set('a', '1-updated'); // update, size still 2 + cache.set('c', '3'); // evicts 'a' (oldest insertion) + + expect(cache.get('a')).toBeUndefined(); + expect(cache.get('b')).toBe('2'); + expect(cache.get('c')).toBe('3'); + }); +}); + +describe('canonicalJson', () => { + it('serializes with recursively sorted keys', () => { + expect(canonicalJson({ b: 1, a: { d: 4, c: 3 } })).toBe('{"a":{"c":3,"d":4},"b":1}'); + }); + + it('produces the same key for structurally identical schemas regardless of key order', () => { + const first = canonicalJson({ type: 'object', properties: { a: { type: 'string' } } }); + const second = canonicalJson({ properties: { a: { type: 'string' } }, type: 'object' }); + expect(first).toBe(second); + }); + + it('returns undefined for cyclic objects', () => { + const cyclic: Record = { type: 'object' }; + cyclic.self = cyclic; + expect(canonicalJson(cyclic)).toBeUndefined(); + }); + + it('handles arrays', () => { + expect(canonicalJson({ a: [1, { b: 2, a: 1 }] })).toBe('{"a":[1,{"a":1,"b":2}]}'); + }); +});