Skip to content

Show when the MCP server fails to start - #598

Merged
erikdarlingdata merged 4 commits into
devfrom
fix/mcp-bind-failure
Sep 28, 2026
Merged

erikdarlingdata merged 4 commits into
devfrom
fix/mcp-bind-failure

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 28, 2026 •

Copy link
Copy Markdown
Owner

What does this PR do?

The MCP status item in the Help menu said "MCP Server: Running (port N)" as soon as the app asked the server to start. It did not wait to see if the server came up. When the port was already in use, the start failed, but the menu still said Running. The error went only to the debug output.

Now the item says "MCP Server: Starting (port N)" first. When the start is done, it changes to "MCP Server: Running (port N)" or to "MCP Server: Failed (port N is in use)". A start that fails for another reason shows the port and the first line of the error's message, for example "MCP Server: Failed (port N: Unable to start.)". A message longer than 100 characters is cut.

McpHostService:

  • It starts Kestrel with StartAsync and then waits with WaitForShutdownAsync. Before, one RunAsync call did both, so a start failure never got back to the app.
  • A new internal Started task gives the result: null when the server is listening, or a short reason when the start failed. If the app closes before the start is done, the task is cancelled.
  • To find a port that is in use, it walks the whole exception chain. It looks for Kestrel's AddressInUseException and for a SocketException with AddressAlreadyInUse.

MainWindow waits for Started, then sets the menu text on the UI thread. If the window is closed by then, it does nothing. A small internal function makes the menu text, so tests can check it. A menu header reads _ as an access key marker, so that function doubles each _ in a failure reason to show it as written.

Which component(s) does this affect?

  • Desktop App (PlanViewer.App)
  • Core Library (PlanViewer.Core)
  • CLI Tool (PlanViewer.Cli)
  • SSMS Extension (PlanViewer.Ssms)
  • Tests
  • Documentation

How was this tested?

  • McpHostServiceTests:
    • A test holds a loopback port open with a TcpListener, then starts the server on that port. Started gives "port N is in use".
    • A test starts the server on a free port. Started gives null.
    • The existing tests still pass.
    • Two tests check the text for another failure: the port and the first line of the message, and a long message cut to 100 characters.
  • McpStatusHeaderTests: the menu text for Starting, Running and Failed, and a _ in a failure reason.
  • Full suite on Windows: 1,046 tests, 1,044 passed, 0 failed, 2 skipped. The Release build has 0 warnings.

Checklist

  • I have read the contributing guide
  • My code builds with zero warnings (Release build, --no-incremental)
  • All tests pass (dotnet test)
  • I have not introduced any hardcoded credentials or server names

🤖 Generated with Claude Code

https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza

StartMcpServer fired McpHostService.StartAsync and set the menu to
"Running" in the same breath, before Kestrel had bound anything. A
failure to bind -- most often another process already on the
configured port -- only reached Debug.WriteLine, so the menu kept
saying Running while the server was actually down.

- McpHostService.ExecuteAsync splits the old combined RunAsync into
  StartAsync followed by WaitForShutdownAsync, so a bind failure is
  observable on its own. A new Started task resolves to null on
  success, a short reason on failure (naming the port when it is
  taken), or cancelled if the host stopped before either happened.
- DescribeStartFailure walks the exception chain for Kestrel's
  AddressInUseException or a bare SocketException(AddressAlreadyInUse),
  and falls back to the exception's own message for anything else.
- MainWindow sets "MCP Server: Starting (port N)" immediately, then
  resolves it to Running or Failed once Started completes, posted to
  the UI thread. A closed window is left alone. BuildMcpStatusHeader
  holds the three header strings so they can be tested without a real
  port or window, the same way DecideClose already covers CloseAction.
- McpHostServiceTests: an occupied port resolves Started to its
  reason; a free port resolves it to null.
- McpStatusHeaderTests: pins the three header strings directly.
- Updated the now-stale comment on RunningServer.WaitUntilListeningAsync,
  which used to say a start failure reached only the debugger.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G625JBNh45iTR1hpT4CxNR
@claude

claude Bot commented Sep 28, 2026

Copy link
Copy Markdown

Reviewed the diff. I didn't run the build or tests. It looks correct and I have no blocking findings.

  • Started task: every path settles it. Exceptions before or during StartAsync go through TrySetResult(DescribeStartFailure). Cancellation goes through the finally TrySetCanceled, and RunContinuationsAsynchronously avoids inline continuations. MainWindow handles the cancelled case.
  • Untrusted input, security, conventions: no SQL is generated, the MCP loopback and Host checks are unchanged, and there are no new NoWarn, version or Web csproj implications.
  • Tests: they cover the occupied-port and free-port cases and the header text.

Two minor, non-blocking notes:

  1. The Failed header for non-port errors shows the raw ex.Message and no port, and that message can be long or multi-line. Consider truncating it or prefixing the port.
  2. After a failed start, _app is never disposed. This was already the case, and Dispose on the service may not cover it. It is harmless for a one-shot startup failure.

A start failure that is not a taken port showed the exception's raw
message, which can be long or run to several lines. The menu item now
shows "port N: " and the message's first line, cut to 100 characters.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza
Comment on lines +118 to +122
/// <summary>
/// The success half of the same contract: Started resolves to null, not just "eventually
/// stops throwing". <see cref="AClientOnThisMachineCanListAndCallTools"/> already covers the
/// server actually working once up; this one is only about the signal that it got there.
/// </summary>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This "success half" summary is stacked directly on top of the next doc comment, so it's attached to AnotherFailureShowsThePortAndTheFirstLineOfItsMessage and reads as a second <summary> on it. It was meant for AFreePortResolvesStartedToNull further down. Move it there and delete it here.

@claude

claude Bot commented Sep 28, 2026

Copy link
Copy Markdown

Logic looks right: Started is set on success or failure, the finally only cancels if neither happened, and the caller handles cancellation and shutdown. Only one nit, inline: a misplaced doc comment in the tests. Another thing to check: DescribeStartFailure puts the raw exception message in the menu header, which is fine for a local single-user app but is unsanitized text. No version, NoWarn, or T-SQL concerns.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza
@claude

claude Bot commented Sep 28, 2026

Copy link
Copy Markdown

Reviewed the diff. I found nothing that needs changing.

  • The Started TCS settles on every path. It gets null on success, a reason on failure, or is cancelled by the finally block. TrySetResult runs before TrySetCanceled, so a real outcome is never overwritten.
  • MainWindow handles the cancelled case and checks IsShuttingDown before touching the menu.
  • DescribeStartFailure walks the whole exception chain and truncates the message. No untrusted plan data reaches it.
  • The tests cover the occupied-port, free-port, truncation and header-wording cases.
  • No version, NoWarn, Web.csproj link or T-SQL changes are involved.

I only read the diff. I did not build or run the tests. The occupied-port test binds IPAddress.Loopback and assumes Kestrel's bind to the same address fails there on every platform. That could differ on macOS or Windows, so watch CI on those.

A menu header reads "_" as an access key marker, so a reason taken from
an exception message now has its underscores doubled.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza
@claude

claude Bot commented Sep 28, 2026

Copy link
Copy Markdown

Reviewed the diff and found nothing blocking. Started is settled exactly once. The cancelled-during-start path is handled by the finally TrySetCanceled, and ReportMcpStartResultAsync swallows that cancellation. InternalsVisibleTo already covers the new internal members for the tests. I didn't build or run the tests.

Optional nit: BuildMcpStatusHeader(Failed, port) with a null reason renders Failed (). Falling back to port {port} would be safer if a caller ever omits the reason.

@erikdarlingdata
erikdarlingdata merged commit f4fcd0b into dev Sep 28, 2026
4 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/mcp-bind-failure branch September 28, 2026 20:59
@erikdarlingdata erikdarlingdata mentioned this pull request Sep 29, 2026
2 of 8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant