Fix local HTTP server lifecycle and TCP connect observation #107
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe PR introduces a distinction between user-selected and Unity-managed local HTTP servers in the MCP editor services. It adds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Greptile SummaryThis PR improves local HTTP server lifecycle management and fixes unobserved task exceptions. Key changes include moving handshake persistence to after Key Improvements:
Issue Found:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Unity as Unity Editor
participant ServerMgmt as ServerManagementService
participant Terminal
participant HTTPServer as HTTP Server Process
participant Shutdown as McpEditorShutdownCleanup
participant TCP as TCP Connection Check
Note over User,HTTPServer: Start Local HTTP Server
User->>Unity: Click "Start Server"
Unity->>ServerMgmt: StartLocalHttpServer()
ServerMgmt->>ServerMgmt: ClearLocalServerPidTracking()
ServerMgmt->>ServerMgmt: Generate instanceToken + pidFilePath
ServerMgmt->>Terminal: Process.Start(launchCommand)
Terminal->>HTTPServer: Launch server process
ServerMgmt->>ServerMgmt: StoreLocalHttpServerHandshake(pidFilePath, token)
Note right of ServerMgmt: Handshake persisted AFTER<br/>successful Process.Start
Note over User,TCP: Wait for Server Ready
Unity->>TCP: WaitForHttpServerAcceptingConnectionsAsync()
loop Poll until timeout
TCP->>HTTPServer: TcpClient.ConnectAsync()
alt Connection completes first
TCP->>TCP: await connectTask
TCP-->>Unity: true (if connected)
else Timeout completes first
TCP->>TCP: Attach continuation to observe exceptions
Note right of TCP: connectTask.ContinueWith(t => _ = t.Exception)
end
end
Note over User,HTTPServer: Shutdown Scenarios
alt User switches transport OR quits Unity
Unity->>Shutdown: OnEditorQuitting()
alt HTTP Local selected
Shutdown->>ServerMgmt: StopLocalHttpServer()
ServerMgmt->>ServerMgmt: Read pidFilePath from handshake
ServerMgmt->>ServerMgmt: Validate PID + token
ServerMgmt->>HTTPServer: TerminateProcess(pid)
else HTTP Local NOT selected
Shutdown->>ServerMgmt: StopManagedLocalHttpServer()
ServerMgmt->>ServerMgmt: TryGetPortFromPidFilePath()
ServerMgmt->>ServerMgmt: StopLocalHttpServerInternal(portOverride, allowNonLocalUrl=true)
Note right of ServerMgmt: Deterministic stop even when<br/>transport selection changed
ServerMgmt->>HTTPServer: TerminateProcess(pid)
end
end
Note over ServerMgmt,HTTPServer: Unix Process Validation Improvement
ServerMgmt->>ServerMgmt: TryGetUnixProcessArgs(pid)
ServerMgmt->>ServerMgmt: ExecPath.TryRun("ps", args)
alt ps command fails AND stdout empty
ServerMgmt-->>ServerMgmt: return false (reject)
Note right of ServerMgmt: Prevents treating error<br/>output as valid args
else ps succeeds OR stdout non-empty
ServerMgmt-->>ServerMgmt: return args (accept)
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (1)
-
MCPForUnity/Editor/Services/ServerManagementService.cs, line 668-674 (link)logic: missing
return falseafter warning - when pidfile PID is Unity Editor, should return immediately instead of falling through to port-based heuristics
5 files reviewed, 1 comment
Motivation
WaitForHttpServerAcceptingConnectionsAsynccould leaveConnectAsynctasks unobserved causing later task exceptions/noise.Process.Startfailed.psfailed, weakening PID verification.Description
connectTaskwhen it finishes and attach a continuation to consume exceptions when it does not complete (WaitForHttpServerAcceptingConnectionsAsyncchange).StoreLocalHttpServerHandshaketo afterProcess.Start.StopManagedLocalHttpServer()and aTryGetPortFromPidFilePathhelper, and extendStopLocalHttpServerInternalwithportOverride/allowNonLocalUrlto support deterministic managed stops even when transport selection changed.ExecPath.TryRunsuccess (or non-empty stdout) before usingpsoutput, and update shutdown cleanup to attempt stopping Unity-managed servers regardless of current transport selection.Testing
Codex Task
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.