Feature/mcp port validation#453
Conversation
- Add PortUtilityService with IsTcpPortListeningAsync, CanBindTcpPortAsync, and GetFreeTcpPortAsync methods (both Dashboard and Lite) - Validate port availability in Settings before saving when MCP is enabled and port changed - Check port availability at startup before starting MCP server; log error and skip if occupied - Feature parity: both Dashboard and Lite have identical validation behavior Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Show error for invalid MCP port values in both Dashboard and Lite Settings - Enforce minimum port 1024 to avoid well-known privileged ports (0-1023) - Use IPEndPoint.MaxPort instead of magic number 65535 - Error message explains privileged port reservation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add 'Auto' button next to MCP Port in Settings (both Dashboard and Lite) - Button calls GetFreeTcpPortAsync to find an available port on localhost - All IsTcpPortListeningAsync calls now explicitly pass IPAddress.Loopback to match MCP server binding Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…-memory objects in MainWindow_Closing(), detection of MCP Server status at Settings_Click() for context sensitivity around MCP status in the Settings window along with a call to RestartMcpServerIfNeeded() after the user makes changes to MCP-related settings. Made similar changes to the Lite app.
erikdarlingdata
left a comment
There was a problem hiding this comment.
Code Review — 3 bugs, 3 minor items
Nice feature — port validation, the Auto button, and the auto-restart on settings change are all welcome improvements. A few issues to address before merging.
Bug 1: Port conflict check skipped when enabling MCP with unchanged port
Files:
Dashboard/SettingsWindow.xaml.cs— insideOkButton_Click, the new port validation blockLite/Windows/SettingsWindow.xaml.cs— insideSaveMcpSettings()
Problem: Both editions only run the port-in-use check when the port value changes:
Dashboard:
if (prefs.McpEnabled && mcpPort != prefs.McpPort)Lite:
if (newEnabled && newPort != oldPort)If MCP was previously disabled and the user enables it without changing the default port (5151), newPort == oldPort evaluates to true so the entire port conflict check is skipped. If another process is already listening on 5151, the user gets no warning — MCP silently fails to start at runtime. The startup check logs an error, but there's no user-visible feedback.
Fix: Run the port conflict check whenever MCP will be enabled, not only when the port value changes. For example:
Dashboard — change the condition to:
if (prefs.McpEnabled)Lite — change the condition to:
if (newEnabled)The IsTcpPortListeningAsync call is cheap (reads the OS TCP listener table), so running it on every save when MCP is enabled has no noticeable cost.
Note on the range check: The newPort < 1024 || newPort > IPEndPoint.MaxPort range validation inside this block should also run whenever MCP is enabled, not just when the port changes. Move it outside the port-change condition or merge it with the broader if (newEnabled) check.
Bug 2: Dashboard return on MCP validation aborts ALL settings save
File: Dashboard/SettingsWindow.xaml.cs — OkButton_Click, around the new MCP validation block
Problem: The two new return; statements (one for port-in-use, one for invalid range) exit OkButton_Click entirely. This skips _preferencesService.SavePreferences(prefs), DialogResult = true, and Close(). If a user changes their SMTP server, alert thresholds, AND enters a bad MCP port, all of their other changes are lost.
The existing alert threshold validation (lines ~615-621) follows a different pattern: it collects errors into validationErrors, shows a warning, and continues saving the rest. MCP validation should follow the same pattern.
Fix: Instead of return, add the MCP error to validationErrors (or a separate list) and skip only prefs.McpPort = mcpPort. Let the rest of the method continue so SavePreferences, DialogResult = true, and Close() still execute. Example:
prefs.McpEnabled = McpEnabledCheckBox.IsChecked == true;
if (int.TryParse(McpPortTextBox.Text, out int mcpPort) && mcpPort >= 1024 && mcpPort <= IPEndPoint.MaxPort)
{
if (prefs.McpEnabled)
{
bool inUse = Task.Run(() => PortUtilityService.IsTcpPortListeningAsync(mcpPort, IPAddress.Loopback)).GetAwaiter().GetResult();
if (inUse)
{
validationErrors.Add($"Port {mcpPort} is already in use — MCP port was not changed.");
}
else
{
prefs.McpPort = mcpPort;
}
}
else
{
prefs.McpPort = mcpPort;
}
}
else if (prefs.McpEnabled)
{
validationErrors.Add($"MCP port must be between 1024 and {IPEndPoint.MaxPort}.");
}This keeps the save flowing for all other settings.
Bug 3: Lite shows "Settings saved." when MCP validation fails
File: Lite/Windows/SettingsWindow.xaml.cs — SaveButton_Click and SaveMcpSettings()
Problem: SaveMcpSettings() returns false both when validation fails (port in use, invalid range) AND when nothing changed. The caller doesn't distinguish these cases — it always proceeds to _saved = true and shows MessageBox.Show("Settings saved.", ...). The user sees a success message even though MCP settings were rejected.
Fix: Either:
- Return a tri-state or separate bool to distinguish "failed" from "unchanged"
- Show a combined message like:
"Settings saved. MCP port was not changed: {reason}" - Or have
SaveMcpSettingsshow its own error and return asavedbool that the caller checks before displaying the generic success message
Minor 1: CanBindTcpPortAsync is unused
Files: Dashboard/Services/PortUtilityService.cs, Lite/Services/PortUtilityService.cs
CanBindTcpPortAsync is defined in both files but never called anywhere. Consider removing it to avoid dead code. If it's intended for future use, that's fine, but it can always be added back when needed.
Minor 2: Dashboard closing handler duplicates StopMcpServerAsync
File: Dashboard/MainWindow.xaml.cs — MainWindow_Closing
The closing handler now has the same cancel → stop → null → dispose sequence as the new StopMcpServerAsync() method. Consider replacing the inline code with await StopMcpServerAsync() for consistency and to avoid maintaining two copies.
Minor 3: Shutdown timeout inconsistency
Dashboard StopMcpServerAsync uses a 5-second timeout; Lite uses 3 seconds. Not a functional issue, but worth aligning for consistency.
Note: Merge conflict with PR #444
Both this PR and #444 modify the same sections of Dashboard/SettingsWindow.xaml.cs and Lite/Windows/SettingsWindow.xaml.cs. Whichever merges second will need conflict resolution.
What does this PR do?
Added validation for the MCP port settings in the Settings window of both Full Dashboard and Lite Dashboard such that the user-defined port must be between 1024 and 65536, and added details to the documentation stating same.
MCP Server Port Validation
Adds TCP port conflict detection and range validation for the MCP server in both Dashboard and Lite editions.
Problem
Previously, the MCP server port setting had no validation beyond a basic numeric check. Users could enter invalid port numbers (e.g., 0), privileged ports (1–1023), or ports already in use by another process — all without any feedback. At startup, the MCP server would silently fail if the port was occupied.
Changes
New:
PortUtilityService(Dashboard + Lite)IsTcpPortListeningAsync— checks if a TCP port is already in use (non-invasive, reads OS connection table)CanBindTcpPortAsync— attempts to bind aTcpListenerto verify availabilityGetFreeTcpPortAsync— asks the OS to allocate an available port (via the new "Auto" button in the Settings Window)Settings Window validation (Dashboard + Lite)
Startup port check (Dashboard + Lite)
Documentation
Files changed (7)
Dashboard/Services/PortUtilityService.csLite/Services/PortUtilityService.csDashboard/SettingsWindow.xaml.csDashboard/MainWindow.xaml.csLite/Windows/SettingsWindow.xaml.csLite/MainWindow.xaml.csREADME.mdWhich component(s) does this affect?
How was this tested?
dotnet build PerformanceMonitor.sln— 0 errorsclaude mcp add --transport http --scope user sql-monitor http://localhost:5151/using various port numbers, including auto-assigned port numbers, in both Dashboard and Lite.Checklist
dotnet build -c Debug)