fix(mcp): make initiliasation faster#59232
Conversation
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const _t0 = performance.now() | ||
| const _lap = (label: string): void => { | ||
| const elapsed = performance.now() - _t0 | ||
| console.log(`[init-profile] ${label.padEnd(40)} +${(elapsed).toFixed(0)}ms`) | ||
| } |
There was a problem hiding this comment.
_lap and the 8+ console.log('[init-profile]...') calls inside initInner are unconditional, so every request initialisation emits a burst of structured profiling lines to stdout. At any meaningful request volume this creates significant log noise and makes it harder to signal real errors. The PR description says these were measured in "local testing", so this looks like instrumentation that was left in by accident. Either gate it behind an env-var/debug flag or remove it before merging.
Prompt To Fix With AI
This is a comment left during a code review.
Path: services/mcp/src/hono/mcp-server.ts
Line: 378-382
Comment:
**Debug profiling logs shipped to production**
`_lap` and the 8+ `console.log('[init-profile]...')` calls inside `initInner` are unconditional, so every request initialisation emits a burst of structured profiling lines to stdout. At any meaningful request volume this creates significant log noise and makes it harder to signal real errors. The PR description says these were measured in "local testing", so this looks like instrumentation that was left in by accident. Either gate it behind an env-var/debug flag or remove it before merging.
How can I resolve this? If you propose a fix, please make it concise.| expect(getGroupTypes).toHaveBeenCalledTimes(2) | ||
| }) | ||
|
|
||
| it('should return undefined (not stale data) when fetch succeeds then later fails', async () => { |
There was a problem hiding this comment.
toEqual(mockGroupTypes) — the actual behaviour being verified is that stale cached data is served when a re-fetch fails, not undefined. The misleading name will confuse anyone reading the test output or the test suite in the future.
| it('should return undefined (not stale data) when fetch succeeds then later fails', async () => { | |
| it('should return stale cached data (not undefined) when fetch succeeds then later fails', async () => { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: services/mcp/tests/unit/StateManager.test.ts
Line: 482
Comment:
The test title says "return undefined" but the assertion is `toEqual(mockGroupTypes)` — the actual behaviour being verified is that stale cached data is served when a re-fetch fails, not `undefined`. The misleading name will confuse anyone reading the test output or the test suite in the future.
```suggestion
it('should return stale cached data (not undefined) when fetch succeeds then later fails', async () => {
```
How can I resolve this? If you propose a fix, please make it concise.![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| if (this._warmup?.resourceEntries.length) { | ||
| const { registerResourceEntries } = await import('@/resources') | ||
| registerResourceEntries(this.server, this._warmup.resourceEntries) | ||
| } else { | ||
| await registerResources(this.server, context) | ||
| } |
There was a problem hiding this comment.
Logic error with empty array check causes fallback even when warmup succeeded.
If warmup succeeds but finds zero valid resources (empty array), the condition this._warmup?.resourceEntries.length evaluates to 0 (falsy), causing the code to fall back to registerResources() and re-fetch everything. This defeats the warmup optimization when there are no resources.
Fix: Check for warmup data existence instead:
if (this._warmup) {
const { registerResourceEntries } = await import('@/resources')
registerResourceEntries(this.server, this._warmup.resourceEntries)
} else {
await registerResources(this.server, context)
}| if (this._warmup?.resourceEntries.length) { | |
| const { registerResourceEntries } = await import('@/resources') | |
| registerResourceEntries(this.server, this._warmup.resourceEntries) | |
| } else { | |
| await registerResources(this.server, context) | |
| } | |
| if (this._warmup) { | |
| const { registerResourceEntries } = await import('@/resources') | |
| registerResourceEntries(this.server, this._warmup.resourceEntries) | |
| } else { | |
| await registerResources(this.server, context) | |
| } | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
| try { | ||
| const { fetchContextMillResources, filterValidEntries, loadManifestFromArchive, clearResourceCache } = | ||
| await import('@/resources/internals') | ||
| const archive = await fetchContextMillResources() |
There was a problem hiding this comment.
Environment variable POSTHOG_MCP_LOCAL_SKILLS_URL is ignored during warmup.
The warmup calls fetchContextMillResources() without parameters, always fetching from GitHub. However, the fallback path in registerContextMillResources respects the local URL override. This causes inconsistent behavior in development/testing - warmup uses GitHub data while per-request fallback uses local data.
Fix: Pass environment variable through the warmup chain. The handler needs access to the environment or the local URL needs to be passed from startup.
| const archive = await fetchContextMillResources() | |
| const archive = await fetchContextMillResources(process.env.POSTHOG_MCP_LOCAL_SKILLS_URL) |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce per-request MCP initialization cost by frontloading work (tool construction and context-mill resource parsing) into a warmup step, while also improving caching behavior for some API-derived state.
Changes:
- Introduces a
ToolCatalogthat pre-builds tool bases/definitions during warmup and filters tools per-request without re-initializing everything. - Adds resource “internals” helpers and preloads/parses context-mill resources at server startup, then reuses pre-parsed resource entries during request init.
- Adjusts
StateManagercached-fetch behavior to support “negative caching” (avoid repeated retries on failures) and adds tests around this.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| services/mcp/tests/unit/tool-catalog.test.ts | Adds unit coverage for ToolCatalog warmup and filtering behavior. |
| services/mcp/tests/unit/StateManager.test.ts | Adds tests for stale/negative caching behavior via getOrFetchGroupTypes. |
| services/mcp/tests/integration/manifest.test.ts | Refactors integration test to use new resource internals helpers. |
| services/mcp/src/resources/ui-apps.ts | Avoids repeated warning spam when MCP_APPS_BASE_URL is missing. |
| services/mcp/src/resources/internals.ts | New module that fetches/caches/unzips resources and loads/filters manifest entries. |
| services/mcp/src/resources/index.ts | Refactors resource registration to use internals helpers and shared entry-registration helper. |
| services/mcp/src/lib/StateManager.ts | Changes stale-cache decision logic and adds negative-caching timestamp on fetch failure. |
| services/mcp/src/hono/tool-catalog.ts | New warmup-able catalog that pre-builds tools and filters them quickly per request. |
| services/mcp/src/hono/streamable-handler.ts | Adds handler warmup (tools + resources) and passes warmup data into HonoMcpServer. |
| services/mcp/src/hono/request-utils.ts | Adds unconditional console error logging in handleCatchError. |
| services/mcp/src/hono/mcp-server.ts | Uses warmup-provided tool catalog/resources and adds init profiling logs + tool resolution refactor. |
| services/mcp/src/hono/index.ts | Calls app warmup before serving requests. |
| services/mcp/src/hono/app.ts | Exposes a warmup() function from createApp(). |
Comments suppressed due to low confidence (1)
services/mcp/src/hono/request-utils.ts:93
handleCatchErrornow logs the rawerrorto stderr for every caught exception. This can be noisy (especially for expected/auth-related failures) and may inadvertently include sensitive details in logs. Prefer relying on the existingcaptureException(or log only a sanitized message / behind a debug flag).
export function handleCatchError(error: unknown, props: RequestProperties): Response {
console.error('[handleCatchError]', error)
const authResponse = mapErrorToAuthResponse(error)
if (authResponse) {
return authResponse
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const _lap = (label: string): void => { | ||
| const elapsed = performance.now() - _t0 | ||
| console.log(`[init-profile] ${label.padEnd(40)} +${(elapsed).toFixed(0)}ms`) |
| try { | ||
| const { fetchContextMillResources, filterValidEntries, loadManifestFromArchive, clearResourceCache } = | ||
| await import('@/resources/internals') | ||
| const archive = await fetchContextMillResources() |
| import { | ||
| type ToolDefinition, | ||
| type ToolFilterOptions, | ||
| getToolDefinition, |
| it('should return undefined (not stale data) when fetch succeeds then later fails', async () => { | ||
| const mockGroupTypes = [{ group_type: 'company', group_type_index: 0 }] | ||
| const getGroupTypes = vi.fn() | ||
| .mockResolvedValueOnce(mockGroupTypes) | ||
| .mockRejectedValueOnce(new Error('API error')) | ||
| const mockApi = stateManager as any |
| // Register prompts and resources — use pre-parsed entries when available | ||
| await registerPrompts(this.server) | ||
| if (this._warmup?.resourceEntries.length) { | ||
| const { registerResourceEntries } = await import('@/resources') | ||
| registerResourceEntries(this.server, this._warmup.resourceEntries) | ||
| } else { |
skoob13
left a comment
skoob13
left a comment
There was a problem hiding this comment.
Looks good, stamping. From a high overview, I still couldn't understand where the bottleneck is, if it's not context-mill or feature flags. I think those two were the biggest latency issues.
| // Register prompts and resources — use pre-parsed entries when available | ||
| await registerPrompts(this.server) | ||
| if (this._warmup?.resourceEntries.length) { | ||
| const { registerResourceEntries } = await import('@/resources') | ||
| registerResourceEntries(this.server, this._warmup.resourceEntries) | ||
| } else { | ||
| await registerResources(this.server, context) | ||
| } | ||
| await registerUiAppResources(this.server, context) | ||
| _lap('registerPrompts + registerResources') |
There was a problem hiding this comment.
Shouldn't it happen in parallel?
| flagPromise, | ||
| toolFlagsPromise, | ||
| singleExecPromise, |
There was a problem hiding this comment.
I'm sure this is the biggest latency chunk. I have some work on that but decided to drop it back then because people yolo use properties on flags. Perhaps not a big deal if we only support a subset of props for local evaluation.
We’re doing a lot of work on every request, lets try frontload as much work as possible so we can make initialisation faster.
This brings init time down from ~400ms to ~15ms in local testing, and reduced how much memory is used initialising.