[MCP] Initialize metadata providers in stdio mode - #3784
[MCP] Initialize metadata providers in stdio mode#3784Christos Despotakis (despotak) wants to merge 4 commits into
Conversation
dab start --mcp-stdio returns from Program.StartEngine before host.Run(), so
Startup.Configure never executes -- and with it PerformOnConfigChangeAsync, the
only caller of IMetadataProviderFactory.InitializeAsync(). Entity names reach the
tool registry from config, but no entity ever receives a database object, so every
MCP tool call fails with:
Database object for entity '<name>' has not been inferred.
The identical configuration serves the same entity correctly over REST, because
the web path does call host.Run().
This is a side effect of Azure#3676 (Avoid starting web host in MCP stdio mode). That
change was correct in itself -- stdio mode should not bind an HTTP port -- but
PerformOnConfigChangeAsync did more than serve HTTP, and nothing took over its
metadata-initialization duty on the stdio path.
RunMcpStdioHost now initializes the metadata providers itself, before registering
tools. The existing assertions that StartAsync and StopAsync are never called
still hold, so Azure#3676 is preserved; the unit test gains a stub factory and an
assertion that InitializeAsync is invoked exactly once.
Verified against SQL Server: describe_entities and read_records both succeed on a
one-entity and a twenty-eight-entity configuration, and REST is unchanged.
Fixes Azure#3783
Co-authored-by: Νύξ (Nyx) 🌑 <noreply@anthropic.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
Restores schema inference for MCP stdio mode by explicitly initializing the metadata providers on the stdio startup path (which bypasses ASP.NET Core Startup.Configure). This addresses the 2.1.x regression where tools were registered from config but all tool calls failed because database objects were never inferred.
Changes:
- Initialize
IMetadataProviderFactoryinsideMcpStdioHelper.RunMcpStdioHostbefore registering MCP tools. - Extend the existing unit test to assert
IMetadataProviderFactory.InitializeAsync()is invoked exactly once in stdio mode.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Service/Utilities/McpStdioHelper.cs | Initializes metadata providers during stdio startup so entities have inferred database objects before tool calls. |
| src/Service.Tests/UnitTests/McpStdioHelperTests.cs | Adds a stub IMetadataProviderFactory and asserts InitializeAsync() is called once without starting the web host. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Core.Services.MetadataProviders.IMetadataProviderFactory metadataProviderFactory = | ||
| host.Services.GetRequiredService<Core.Services.MetadataProviders.IMetadataProviderFactory>(); |
There was a problem hiding this comment.
nit- just import using Azure.DataApiBuilder.Core.Services.MetadataProviders instead of calling the same multiple times.
There was a problem hiding this comment.
Done in 6b2fea1 — and I applied it to the rest of the method too: importing Azure.DataApiBuilder.Mcp.Core and .Mcp.Model removes seven more qualifications (McpToolRegistry ×3, IMcpTool ×2, IMcpStdioServer ×2). Those sit on lines this PR did not introduce, so happy to drop that hunk if you would rather the diff stayed strictly on the lines it added.
| // "Database object for entity '<name>' has not been inferred." | ||
| Core.Services.MetadataProviders.IMetadataProviderFactory metadataProviderFactory = | ||
| host.Services.GetRequiredService<Core.Services.MetadataProviders.IMetadataProviderFactory>(); | ||
| metadataProviderFactory.InitializeAsync().GetAwaiter().GetResult(); |
There was a problem hiding this comment.
since this inside a try/finally that only disposes the host, so any initialization exception propagates raw out of RunMcpStdioHost (which otherwise returns bool). Consider wrapping it so a metadata-inference failure produces a clear, logged error over the stdio channel rather than an unhandled exception
There was a problem hiding this comment.
Addressed in 6b2fea1. The catch went on the existing outer try rather than around the initialization alone, taking "any initialization exception" literally — and that turned out to matter: GetRequiredService<IMetadataProviderFactory>() activates MetadataProviderFactory, whose constructor calls ConfigureMetadataProviders() → RuntimeConfigProvider.GetConfig(), so a missing or unparseable config file throws one line above a narrower guard.
It reports on stderr rather than through ILogger, because stdio clears every logging provider and the remaining McpLoggerProvider stays disabled until the client sends logging/setLevel — which cannot happen before the JSON-RPC loop runs. Full reasoning and the before/after measurement are in the comment on the PR.
Souvik Ghosh (souvikghosh04)
moved this from In Progress
to Review In Progress
in Data API builder
There was a problem hiding this comment.
🟢 Approval recommended
The change is small, directly addresses the reported regression in stdio mode, and is covered by a focused unit test that preserves the existing “does not start web host” guarantees.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
RunMcpStdioHost had a try/finally that only disposed the host, so any exception
propagated out of a method whose contract is a bool. The catch goes on the
existing outer try rather than around the metadata initialization alone, because
the resolution one line above it is where the commonest failures land:
GetRequiredService<IMetadataProviderFactory>() activates MetadataProviderFactory,
whose constructor calls ConfigureMetadataProviders() -> RuntimeConfigProvider
.GetConfig(), which throws "Runtime config isn't setup." for a missing or
unparseable config file. Guarding the initialization alone would have left that
untouched.
This follows Startup.PerformOnConfigChangeAsync: report, return false. Program
.Main already maps false to ExitCode -1, the stdio analogue of that path's
hostLifetime.StopApplication(), so no caller changes. The catch spans the stdio
loop as well as startup, which is why the message says "run the MCP stdio host"
rather than naming a phase. OperationCanceledException is filtered out so a
normal shutdown is not relabelled as a failure; it continues to reach
Program.StartEngine's dedicated handler unchanged.
The report goes to stderr rather than through ILogger because no logger can
reach anyone at that point: stdio clears every provider and leaves
McpLoggerProvider, whose McpLogger stays disabled until the client sends
logging/setLevel, which cannot happen before the JSON-RPC loop runs. Writing a
notifications/message frame by hand would precede the initialize response the
server contracts to send first.
stderr itself may be suppressed. --mcp-stdio defaults to LogLevel.None, at which
Program points both console streams at TextWriter.Null for "ZERO output", which
is why the existing "Unable to launch the runtime" message is never seen in that
mode either. This reports anyway, on the view that a refusal to run is not log
output and an exit code alone is not diagnosable; it writes to the standard error
stream directly rather than installing a replacement writer that would outlive
the call. stdout is untouched and stays reserved for JSON-RPC.
Measured against the built engine with a missing config file, default log level:
before: exit 255, stdout 0 bytes, stderr 0 bytes
after : exit 255, stdout 0 bytes, stderr 2635 bytes
Imports Azure.DataApiBuilder.Core.Services.MetadataProviders so the factory type
is not fully qualified twice, and extends the same treatment to the rest of the
method: importing Azure.DataApiBuilder.Mcp.Core and .Mcp.Model removes seven
further qualifications of McpToolRegistry, IMcpTool and IMcpStdioServer. Those
seven sit on lines this PR did not introduce and can be dropped if the reviewer
would rather the diff stayed on the lines it added.
Both new tests were verified to fail when only the catch is reverted.
Co-Authored-By: Νύξ (Nyx, AI) 🌑 <nyx@despotak.is>
|
Both addressed in the follow-up commit. Nit: done — Error handling: the It follows On the channel: the report goes to stderr, because the logging pipeline cannot carry it this Worth flagging: today this failure is silent, not just raw. Tests: two new, beside the existing one, both proven red by reverting only the Two things I left out to keep the diff to what you asked for, happy to add either: folding the stderr |
Souvik Ghosh (souvikghosh04)
assigned
Souvik Ghosh (souvikghosh04) and
aaronburtle and unassigned
aaronburtle and
Souvik Ghosh (souvikghosh04)
|
/azp run |
|
Azure Pipelines: Successfully started running 6 pipeline(s). |
Why make this change?
Closes #3783.
Summary of the linked issue: in every
2.1.xbuild,dab start --mcp-stdioregisters entities but never infers their database objects, so every MCP tool call fails withDatabase object for entity '<name>' has not been inferred.while the identical config serves the same entity correctly over REST.2.0.12is unaffected.--mcp-stdioRelated: #3676, #3675 (the change this regressed from), and #3430 — see the note at the bottom.
What is this change?
Schema inference is reachable only through the ASP.NET Core startup path, which stdio mode skips:
Program.cs—StartEnginereturns before the host is started:McpStdioHelper.RunMcpStdioHost— initialises the tool registry and nothing else, soStartup.Configurenever runs.Startup.cs:822—Configureis the only place that callsPerformOnConfigChangeAsync(app)(line 866).Startup.cs:1431—PerformOnConfigChangeAsyncis the only caller ofIMetadataProviderFactory.InitializeAsync().So entity names reach the tool registry from config, while
IMetadataProviderFactoryis never initialised and no entity receives a database object. That is exactly the observed split:tools/listsucceeds, every tool call fails.This is a side effect of #3676 "Avoid starting web host in MCP stdio mode". That change was right — stdio mode should not bind an HTTP port — but
PerformOnConfigChangeAsyncdid more than serve HTTP, and nothing took over its metadata-initialisation duty on the stdio path.The fix is 10 lines:
RunMcpStdioHostresolvesIMetadataProviderFactoryfrom DI and initialises it before registering tools.Program.csis untouched, and #3676's behaviour is preserved — the existing assertions thatStartAsync/StopAsyncare never called still pass.How was this tested?
RunMcpStdioHost_DoesNotStartWebHostgains a stubIMetadataProviderFactoryand an assertion thatInitializeAsyncis called exactly once. Its original assertions are unchanged and still pass, so this cannot silently re-introduce the web host.Also verified by hand against SQL Server, on
mainbuilt from source with the pinned SDK (10.0.302), before and after the patch:describe_entities, 1-entity confighas not been inferredsuccess(1 entity)describe_entities, 28-entity confighas not been inferredsuccess(28 entities)read_recordson a tablesuccess, rows returnedGET /api/<entity>HTTP 200HTTP 200(unchanged)dotnet format --verify-no-changeson both touched files exits 0.Sample Request(s)
Driving the stdio server by hand, so no MCP client is involved:
Before:
{"toolName":"describe_entities","status":"error", "error":{"type":"DataApiBuilderError", "message":"Database object for entity 'MyTable' has not been inferred."}}After:
{"entities":[{"name":"MyTable","description":"","fields":[],"permissions":["READ"]}], "count":1,"status":"success"}One note for reviewers
#3430 reports that stdio blocks the
initializeresponse until introspection finishes (~17 s against a remote instance with 53 entities). Restoring inference on this path necessarily brings that latency back — it was only absent because inference was not happening at all. If you would prefer inference to run asynchronously afterinitializereturns, that is a larger change and I am happy to rework this accordingly; this PR deliberately restores correctness first.Investigated and written with Claude Code (Νύξ) 🌑 — the version bracket, the REST/stdio control and the root-cause trace were worked out together. Reviewed and submitted by me.