fix(controller): resolve MCP global middleware lazily on first request#460
fix(controller): resolve MCP global middleware lazily on first request#460elrrrrrrr wants to merge 2 commits into
Conversation
MCPControllerRegister.register() runs during the tegg load-unit init (postCreate), and eagerly called getGlobalMiddleware() which looks up the configured middleware names in app.middlewares. But app.middlewares is only populated later by loadMiddleware, so registering a controller whose config.mcp.middleware references any middleware throws 'TypeError: Middleware xxx not found' at boot. Defer the resolution: build the global middleware chain lazily on the first request (composeGlobalMiddleware wraps the base middleware), make getGlobalMiddleware idempotent, and drop the eager call in register(). The installed middleware is identical at request time; it just no longer requires app.middlewares to be ready during registration. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 47 minutes and 56 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughMCP controller registration now defers global middleware resolution until the first request. The same lazy wrapper is applied to stateless stream, stateful stream, SSE, and MCP POST paths, and eager global-middleware initialization is removed. ChangesLazy MCP middleware composition
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors MCPControllerRegister to lazily resolve and compose global middlewares on the first request rather than at controller registration time, preventing errors when middlewares are not yet fully loaded. Feedback on the changes suggests caching the composed middleware within composeGlobalMiddleware to avoid redundant allocations and improve performance on every request.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| composeGlobalMiddleware(mw: compose.Middleware<EggContext>): compose.Middleware<EggContext> { | ||
| const self = this; | ||
| return async (ctx, next) => { | ||
| self.getGlobalMiddleware(); | ||
| const composed = self.globalMiddlewares ? compose([ mw, self.globalMiddlewares ]) : mw; | ||
| return composed(ctx, next); | ||
| }; | ||
| } |
There was a problem hiding this comment.
Calling compose([ mw, self.globalMiddlewares ]) on every request allocates a new composed middleware function and array on every invocation, which is inefficient. Since self.globalMiddlewares is resolved once and mw is constant for each endpoint, we can cache the composed middleware locally inside composeGlobalMiddleware to avoid redundant allocations.
composeGlobalMiddleware(mw: compose.Middleware<EggContext>): compose.Middleware<EggContext> {
const self = this;
let composedCache: compose.Middleware<EggContext> | undefined;
return async (ctx, next) => {
self.getGlobalMiddleware();
if (!composedCache) {
composedCache = self.globalMiddlewares ? compose([ mw, self.globalMiddlewares ]) : mw;
}
return composedCache(ctx, next);
};
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
827-837: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the composed pipeline after lazy resolution.
getGlobalMiddleware()is guarded, but Line 834 still rebuilds the composed middleware function on every request. Cache it in the closure so each registered MCP route composes once afterapp.middlewaresis ready.♻️ Proposed fix
composeGlobalMiddleware(mw: compose.Middleware<EggContext>): compose.Middleware<EggContext> { const self = this; + let composed: compose.Middleware<EggContext> | undefined; return async (ctx, next) => { - self.getGlobalMiddleware(); - const composed = self.globalMiddlewares ? compose([ mw, self.globalMiddlewares ]) : mw; + if (!composed) { + self.getGlobalMiddleware(); + composed = self.globalMiddlewares ? compose([ mw, self.globalMiddlewares ]) : mw; + } return composed(ctx, next); }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/controller/lib/impl/mcp/MCPControllerRegister.ts` around lines 827 - 837, The composeGlobalMiddleware wrapper in MCPControllerRegister still rebuilds the composed middleware on every request even after getGlobalMiddleware() has resolved app.middlewares; cache the composed result in the returned closure so it is created once and reused. Update composeGlobalMiddleware to keep a per-route cached composed middleware (initialized after the first lazy resolution) and only fall back to mw until the global stack is available, ensuring each MCP route composes once after app.middlewares is ready.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@plugin/controller/lib/impl/mcp/MCPControllerRegister.ts`:
- Around line 827-837: The composeGlobalMiddleware wrapper in
MCPControllerRegister still rebuilds the composed middleware on every request
even after getGlobalMiddleware() has resolved app.middlewares; cache the
composed result in the returned closure so it is created once and reused. Update
composeGlobalMiddleware to keep a per-route cached composed middleware
(initialized after the first lazy resolution) and only fall back to mw until the
global stack is available, ensuring each MCP route composes once after
app.middlewares is ready.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16940fb5-9fd8-4ba0-aa7b-1a28ac44fb4b
📒 Files selected for processing (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts
Unit tests for the deferred resolution: - composeGlobalMiddleware does not read app.middlewares at registration time (no throw, globalMiddlewares stays unbuilt) — the robustness the fix adds. - the configured middleware is resolved and run on the first request, once app.middlewares has been populated by loadMiddleware. - getGlobalMiddleware is idempotent (builds once). - a genuinely unknown middleware name still throws at resolution time, so the deferral does not hide real misconfiguration. - koa-compose onion ordering is preserved for multiple middlewares. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
elrrrrrrr
force-pushed
the
fix/mcp-global-middleware-lazy
branch
from
5c3740b to
49bc715
Compare
|
Wrong repo — tegg was merged into the eggjs/egg monorepo for egg4. Re-submitted as eggjs/egg#5994 (against the |
1 participant
Problem
MCPControllerRegister.register()runs during the tegg load-unit init (postCreate) and eagerly calledgetGlobalMiddleware(), which resolves the names inconfig.mcp.middlewareagainstapp.middlewares. Butapp.middlewaresis only populated later byloadMiddleware, so booting an app whoseconfig.mcp.middlewarereferences any middleware throws at startup:Surfaced under egg4 when a controller app configures
mcp.middleware(e.g. a tracing middleware from another plugin).Fix
Defer global-middleware resolution to the first request, when
app.middlewaresis ready:composeGlobalMiddleware(mw)wraps the base middleware and builds/composes the global chain on first invocation.getGlobalMiddleware()is now idempotent (builds once).register()is removed.Middleware applied at request time is unchanged; it just no longer requires
app.middlewaresto be populated during controller registration.🤖 Generated with Claude Code
Summary by CodeRabbit