fix(core-internal): cache AJV validators for schemas without $id - #2699
fix(core-internal): cache AJV validators for schemas without $id#2699kyletser wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: dcbbab9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
There was a problem hiding this comment.
Pull request overview
Fixes repeated AJV compilation for schemas without usable $id by caching validators by schema serialization.
Changes:
- Adds compiled-validator caching.
- Adds regression tests for cache behavior.
- Adds a patch changeset.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Summary |
|---|---|
packages/core-internal/test/validators/ajvProviderCache.test.ts |
Adds caching and validation regression tests. |
packages/core-internal/src/validators/ajvProvider.ts |
Implements schema-source caching. Critical: compile a snapshot to prevent cache poisoning when schemas mutate in place, and add coverage for that case. |
.changeset/fix-ajv-validator-cache.md |
Documents the patch release. |
Suppressed comments (2)
packages/core-internal/src/validators/ajvProvider.ts:166
JSON.stringifycan throw for programmatically supplied non-serializable schema objects (for example, cyclic schemas or BigInt values). This makesgetValidator()fail before AJV sees the schema, whereas the previous implementation delegated compilation to the configured engine. Treat serialization as a best-effort cache key and fall back toengine.compile(schema)without caching when it fails.
const key = JSON.stringify(schema);
packages/core-internal/src/validators/ajvProvider.ts:166
- JSON Schema object member order is not significant, but
JSON.stringifypreserves insertion order. Consequently, the same schema with reordered keywords orpropertiesgets a separate AJV compilation; repeatedtools/listresponses that vary key order can still retain duplicate validators. Use a canonical serialization that recursively sorts object keys if this cache is intended to deduplicate structural equality.
const key = JSON.stringify(schema);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (cached !== undefined) { | ||
| return cached; | ||
| } | ||
| const compiled = engine.compile(schema); |
There was a problem hiding this comment.
Thanks — fixed. _compiledValidator now compiles a snapshot derived from the cache key (engine.compile(JSON.parse(key))) instead of the caller's live object, so AJV's identity-based compilation cache cannot be poisoned by in-place mutation. Also addressed the two suppressed notes: JSON.stringify is wrapped and falls back to an uncached engine.compile() for non-serializable schemas, and the cache key is now canonical (recursively sorted object keys) so structurally identical schemas with different key order share one compilation. Added 3 regression tests: in-place mutation re-compiles under a fresh key, key-order variation hits the same cache entry, and cyclic schemas compile without caching.
AjvJsonSchemaValidator recompiled schemas without an $id on every getValidator() call, and AJV retained each compiled validator forever. Long-running clients that periodically refresh their tool catalogue (e.g. via Client.listTools()) therefore grew the heap without bound until the process aborted (modelcontextprotocol#2605). Identical schemas are now cached by their JSON serialization, so each distinct schema compiles at most once. Schemas with an $id keep using the existing $id-based lookup.
fb9ba17 to
dcbbab9
Compare
|
Great fix — reviewed in depth alongside #2626 since both address #2605. The core approach here is the strongest of the two: canonical content keys ( Two things worth considering before merge:
Neither blocks the idea of the fix; 1 is the one I'd insist on before this ships. |
Fixes #2605.
What
AjvJsonSchemaValidator.getValidator()recompiled any schema without a usable$idon every call (engine.compile(schema)), and the AJV engine retains every compiled validator forever. Long-running clients that periodically refresh their tool catalogue — e.g. aClientcallinglistTools()repeatedly — therefore grew the heap without bound until the process aborted.Root cause
getValidator()only deduplicates through AJV's$id-keyed cache (engine.getSchema($id)); the no-$idbranch always calledengine.compile(). The docstring already states "The validator is compiled once and can be reused multiple times", but caching only happened on the$idpath.Fix
Cache compiled validators for schemas without
$idby the schema's JSON serialization, so each distinct schema compiles at most once per provider instance:packages/core-internal/src/validators/ajvProvider.ts: new_compiledBySourcemap +_compiledValidator()helper; the$idpath is unchanged.$id→ unchanged behavior (AJVgetSchemalookup, compile on miss).Changeset
Added
.changeset/fix-ajv-validator-cache.md(@modelcontextprotocol/core-internalpatch).Verification
pnpm typecheck:all— all packages passpnpm build:all— passespnpm --filter @modelcontextprotocol/core-internal test— 1450/1452 passtest/validators/ajvProviderCache.test.ts(5 cases): identical schema compiles once; structurally identical schemas hit the cache; distinct schemas compile independently;$idpath unchanged; cached validator still validates correctly.eslinton the touched files — cleanPre-existing failures, unrelated to this change
schemaTwinConformance.test.tsreports 2 failures: the vendored2026-07-28twin undercorpus/schema-twins/is 184,637 bytes whilemanifest.jsonpins 180,695 ("twin provenance integrity"). This file is untouched by this PR and reproduces on a cleanmaincheckout; it needs apnpm fetch:schema-twinsrefresh by a maintainer.lint:alldocs-snippets sync check (pnpm sync:snippets) is out of date onmainand unrelated to this PR; CI does not runlint:all.How to review
packages/core-internal/src/validators/ajvProvider.ts—_compiledValidator()+_compiledBySource(the only runtime change, ~20 lines).packages/core-internal/test/validators/ajvProviderCache.test.ts— regression coverage using a fake engine that countscompile()calls.getValidator(sameSchemaNoId)twice callscompile()twice; with this change, once.