fix: cleanup possibel race conditions in mcpproxy

[Cali0707]

This resolves some possible race conditions in the mcpproxy server/server manager

Summary by CodeRabbit

• Improvements
  • Enhanced server startup error reporting and handling.
  • Startup process now waits for all servers to be ready before continuing.
  • Improved error messages during server initialization failures.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The changes modify startup behavior and error propagation in the MCP proxy server management. The server.go adds error tracking on startup failures and updates WaitReady to return errors instead of nil. The server_manager.go changes Start from non-blocking to blocking behavior, removing async readiness signaling in favor of sequential server readiness validation.

Changes

Cohort / File(s)	Summary
Server startup error tracking pkg/mcpproxy/server.go	Added startErr to capture startup failures; URL construction updated to use full listener address; WaitReady now returns captured startup error instead of nil.
Server manager startup flow pkg/mcpproxy/server_manager.go	Changed Start from non-blocking to blocking; removed async readiness signaling; added sequential server readiness validation with early cancellation and error return on first failure.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Changes concentrated in two closely-related files affecting critical startup paths
• Logic involves error tracking propagation and synchronization flow modifications
• Review requires understanding interaction between server and server manager startup sequences
• Focus areas: error handling paths, state transitions during startup, and readiness signaling removal/replacement

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title mentions fixing race conditions in mcpproxy, which aligns with the PR objectives and the actual changes involving startup error tracking and synchronization in server initialization.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

pkg/mcpproxy/server_manager.go (1)

109-115: Consider using gctx instead of ctx for WaitReady.

The servers run with gctx (errgroup context), but WaitReady uses ctx. If a server fails in Run after closing its ready channel (e.g., Serve fails immediately), gctx is cancelled but ctx is not. The remaining WaitReady calls would still succeed with nil, and Start would return nil despite a failed server.

Since close(s.ready) happens very early in Run (before Serve), this window is small but exists. Using gctx would provide tighter error propagation:

 	// Wait for all servers to be ready before returning
 	for name, srv := range m.servers {
-		if err := srv.WaitReady(ctx); err != nil {
+		if err := srv.WaitReady(gctx); err != nil {
 			cancel() // Cancel all servers if one fails to become ready
 			return fmt.Errorf("server %s failed to become ready: %w", name, err)
 		}
 	}

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between de2055d and 718e5df.

📒 Files selected for processing (2)

• pkg/mcpproxy/server.go (3 hunks)
• pkg/mcpproxy/server_manager.go (2 hunks)

🔇 Additional comments (4)

pkg/mcpproxy/server.go (3)

36-37: Good addition for startup error tracking.

The startErr field paired with ready channel provides proper synchronization—the write to startErr happens-before close(ready), and WaitReady reads it only after the channel closes, establishing a valid happens-before relationship.

---

181-189: Correct error signaling on startup failure.

The flow is correct: on listen failure, startErr is set before close(ready), ensuring callers of WaitReady see the error. The success path at line 189 and failure path at line 182 are mutually exclusive, avoiding double-close.

---

260-266: LGTM!

Returning s.startErr after the ready channel closes correctly propagates startup failures to callers.

pkg/mcpproxy/server_manager.go (1)

22-23: Doc comment accurately reflects the new blocking behavior.

The updated comment clearly states the contract: Start blocks until all servers are ready or an error occurs.

[nader-ziada]

/lgtm