Skip to content

Fix duplicate HTTP transport start on active Funplay server#7

Closed
JOY wants to merge 1 commit into
FunplayAI:mainfrom
JOY:fix/http-transport-existing-server-attach
Closed

Fix duplicate HTTP transport start on active Funplay server#7
JOY wants to merge 1 commit into
FunplayAI:mainfrom
JOY:fix/http-transport-existing-server-attach

Conversation

@JOY
Copy link
Copy Markdown

@JOY JOY commented May 22, 2026

Summary

  • Detect an existing Funplay MCP HTTP server on the configured port with a short JSON-RPC initialize probe after HttpListener reports address-in-use.
  • Reuse only a server whose serverInfo.name matches the current Unity project, so unrelated listeners still fail normally.
  • Track whether a transport owns the listener so stopping an attached duplicate transport does not close the real listener.
  • Pass the expected Funplay server name from MCPServerService into the transport, avoiding Unity API calls from background transport code.
  • Add an Editor test covering duplicate same-project transport startup and attached-stop behavior.

Root cause

MCPServerService.StartAsync is idempotent for one service instance, but HttpMCPTransport.StartAsync still treated any address-in-use result as an external conflict. When Unity ended up with an already-running Funplay listener in the same project/process and another start path created a fresh transport, the second start retried for 10 seconds and then logged a fatal startup failure even though the configured endpoint was already usable.

Verification

  • git diff --check: passed.
  • Direct transport harness: passed. Output: PASS: duplicate start attached in 58 ms and attached stop left listener alive.
  • Live Unity verification in D:\Projects\Second-Spawn\Unity on Unity 6.5 beta 6000.5.0b8 with package-cache test patch:
    • Restarted Funplay service by reflection: started=True isRunning=True port=8766.
    • External JSON-RPC initialize probe after restart: HTTP 200, serverInfo.name = Funplay MCP Server - Second Spawn.
    • Created a duplicate HttpMCPTransport inside Unity on port 8766: duplicateStarted=True; elapsedMs=114; port=8766.
    • External JSON-RPC initialize probe after stopping the duplicate transport: HTTP 200, confirming attached Stop() did not close the owning service.
  • Unity console after compile: no Funplay compile/start errors; one unrelated CoplayDev WebSocket warning remained.
  • Unity EditMode test runner was attempted with Unity 6000.5.0b8 and Unity 6000.4.6f1 in disposable projects, but both local batch runs failed before importing packages with Unity Package Manager: The "path" argument must be of type string. Received undefined.

Merge status

Keeping this PR draft until a maintainer or CI can run the formal Unity EditMode test assembly, but the live Second Spawn integration repro now passes.

@JOY JOY marked this pull request as draft May 22, 2026 00:36
@JOY JOY force-pushed the fix/http-transport-existing-server-attach branch from b49dca0 to 08cbdc4 Compare May 22, 2026 00:39
@JOY JOY force-pushed the fix/http-transport-existing-server-attach branch from 08cbdc4 to 814ff47 Compare May 22, 2026 00:50
@Winlifes
Copy link
Copy Markdown
Member

Thanks for the follow-up and the Windows/Unity 6 verification. The direction is useful: detecting an already-running Funplay endpoint and making attached Stop() non-destructive is a good test case to keep.

I do not want to merge this draft as-is yet, mainly because the attach criteria and lifecycle semantics need to be stricter:

  1. "Same project" currently means serverInfo.name == Funplay MCP Server - Application.productName. That can collide across different Unity projects with the same product name. We need a stable project identity in the probe, for example project path or a project-path hash exposed by initialize, before an attached transport can safely treat the listener as ours.

  2. When a fresh MCPServerService attaches to an existing listener, that service reports running but does not own the listener and will not handle requests. The real requests still go to the old listener/handler. That may be okay for a narrow duplicate-transport harness, but it can make the server window/state misleading around domain reload or stale service recovery. We need an explicit design for attached state: what the UI shows, what Stop() means, and whether the service should reconcile with the owning server instead of simply reporting fully running.

  3. Probe timeout/cancellation should not be treated the same as caller cancellation. A slow or unrelated listener on the port should fail the attach probe and continue the normal retry/failure path, not make StartAsync look externally cancelled because HttpClient timed out.

The test you added is valuable, but before merging I would like it to cover these cases too:

  • same Funplay server name but different project identity must not attach;
  • unrelated or unresponsive listener should not attach and should not convert timeout into caller cancellation;
  • attached Stop() must leave the owner alive, while owner Stop() still closes the listener;
  • service/window state should remain truthful when a transport is attached instead of owning the listener.

So I would keep this PR draft for now. We can either iterate here, or I can absorb the idea into a follow-up implementation that adds project identity to initialize first and then uses that for a safer attach path.

@JOY
Copy link
Copy Markdown
Author

JOY commented May 22, 2026

Closing this draft as superseded by upstream v0.3.7. The release implements the safer project-identity attach path requested in review and passed the live Second Spawn verification on my side: initialize returns version 0.3.7 with projectIdentity, tools/list responds, request_recompile reports no compilation errors, and the listener remains healthy after a forced compile check. Thanks for the quick follow-up fix.

@JOY JOY closed this May 22, 2026
@Winlifes
Copy link
Copy Markdown
Member

Thanks for verifying v0.3.7 on your side and for closing the draft. Your repro and PR were very helpful for shaping the safer project-identity attach path. Appreciate the quick follow-up testing.

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.

2 participants