Fix and enhance: Handle WebSocket connection errors on Windows , enhance parameter validation to accept JSON string inputs for complex types (Vector2/3/4, Color, Quaternion) across all manage_* tools. #637
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideAdds Windows-specific handling for benign WebSocket accept errors and broadens all manage_* MCP tools to accept JSON string inputs for complex parameters (vectors, colors, component lists, properties, and textures), backed by normalization helpers and new async-aware tests. Sequence diagram for Windows WebSocket accept error handlingsequenceDiagram
actor UnityEditor
participant OS as WindowsSocket
participant Asyncio as asyncioEventLoop
participant PluginHub as PluginHub
participant Logger as Logger
UnityEditor->>OS: Open WebSocket connection
OS-->>PluginHub: New connection pending
PluginHub->>Asyncio: await server.accept
UnityEditor-->>OS: Disconnect during handshake
OS-->>Asyncio: OSError WinError64
Asyncio-->>PluginHub: Raise OSError
PluginHub->>PluginHub: try accept_connection
PluginHub-->>Logger: log_debug("WinError 64 during accept")
PluginHub-->>Asyncio: return gracefully
rect rgb(200,200,200)
UnityEditor->>OS: Other connection failure
OS-->>Asyncio: Other OSError
Asyncio-->>PluginHub: Raise OSError
PluginHub->>PluginHub: catch OSError
PluginHub-->>Logger: log_warning("WebSocket accept error")
PluginHub-->>Asyncio: return gracefully
end
Asyncio->>Logger: emit internal error logs
Logger->>Logger: asyncio_logger.setLevel(WARNING) on win32
Logger-->>Asyncio: Suppress noisy ERROR logs
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
whatevertogo
changed the title
Contributor
📝 WalkthroughWalkthroughThe PR extends multiple server-side tool functions to accept JSON string representations of complex parameters (dicts, lists) alongside their native types, enabling flexible input handling. Additionally, it configures asyncio event loop handling for pytest and adds Windows-specific asyncio error suppression in the main module. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The broad
except Exceptionblocks inon_connectwill also catchasyncio.CancelledErrorand similar control-flow exceptions; consider explicitly re-raisingCancelledError(and possiblyKeyboardInterrupt) to avoid masking task cancellation semantics. - When logging connection failures (both during
acceptand sending the welcome message), consider including contextual details such as the remote address (getattr(websocket, 'client', None)or similar) andexc_info=Trueto make debugging sporadic connection issues easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The broad `except Exception` blocks in `on_connect` will also catch `asyncio.CancelledError` and similar control-flow exceptions; consider explicitly re-raising `CancelledError` (and possibly `KeyboardInterrupt`) to avoid masking task cancellation semantics.
- When logging connection failures (both during `accept` and sending the welcome message), consider including contextual details such as the remote address (`getattr(websocket, 'client', None)` or similar) and `exc_info=True` to make debugging sporadic connection issues easier.
## Individual Comments
### Comment 1
<location> `Server/src/transport/plugin_hub.py:94-103` </location>
<code_context>
+ # Log other socket errors at warning level
+ logger.warning("WebSocket accept failed: %s", exc)
+ return
+ except Exception as exc:
+ logger.error("Unexpected error in WebSocket on_connect: %s", exc)
+ return
+
+ # Send welcome message only if accept succeeded
msg = WelcomeMessage(
serverTimeout=self.SERVER_TIMEOUT,
keepAliveInterval=self.KEEP_ALIVE_INTERVAL,
)
- await websocket.send_json(msg.model_dump())
+ try:
+ await websocket.send_json(msg.model_dump())
+ except Exception as exc:
+ logger.warning("Failed to send welcome message: %s", exc)
async def on_receive(self, websocket: WebSocket, data: Any) -> None:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider handling asyncio.CancelledError separately instead of catching it in the broad Exception block.
Since `asyncio.CancelledError` is a subclass of `Exception`, these broad `except Exception` blocks will also catch task cancellations from `websocket.accept()` and `websocket.send_json(...)`, turning them into logged errors instead of propagating the cancellation. That can interfere with cooperative cancellation and pollute logs. Consider adding a dedicated `except asyncio.CancelledError: raise` before the generic handler to maintain normal cancellation behavior.
Suggested implementation:
```python
# WinError 64: "The specified network name is no longer available"
# This occurs when clients disconnect before the WebSocket handshake completes.
if hasattr(exc, 'winerror') and exc.winerror == 64:
logger.debug(
"WebSocket accept failed: client disconnected early (WinError 64)"
)
return
# Log other socket errors at warning level
logger.warning("WebSocket accept failed: %s", exc)
return
except asyncio.CancelledError:
# Preserve cooperative cancellation semantics
raise
except Exception as exc:
logger.error("Unexpected error in WebSocket on_connect: %s", exc)
return
# Send welcome message only if accept succeeded
msg = WelcomeMessage(
serverTimeout=self.SERVER_TIMEOUT,
keepAliveInterval=self.KEEP_ALIVE_INTERVAL,
)
try:
await websocket.send_json(msg.model_dump())
except asyncio.CancelledError:
# Let cancellations propagate instead of turning them into warnings
raise
except Exception as exc:
logger.warning("Failed to send welcome message: %s", exc)
```
1. Add `import asyncio` to the imports near the top of `Server/src/transport/plugin_hub.py` if it is not already present.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Server/src/transport/plugin_hub.py
Outdated
Comment on lines
94
to
103
| except Exception as exc: | ||
| logger.error("Unexpected error in WebSocket on_connect: %s", exc) | ||
| return | ||
|
|
||
| # Send welcome message only if accept succeeded | ||
| msg = WelcomeMessage( | ||
| serverTimeout=self.SERVER_TIMEOUT, | ||
| keepAliveInterval=self.KEEP_ALIVE_INTERVAL, | ||
| ) | ||
| await websocket.send_json(msg.model_dump()) | ||
| try: |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Consider handling asyncio.CancelledError separately instead of catching it in the broad Exception block.
Since asyncio.CancelledError is a subclass of Exception, these broad except Exception blocks will also catch task cancellations from websocket.accept() and websocket.send_json(...), turning them into logged errors instead of propagating the cancellation. That can interfere with cooperative cancellation and pollute logs. Consider adding a dedicated except asyncio.CancelledError: raise before the generic handler to maintain normal cancellation behavior.
Suggested implementation:
# WinError 64: "The specified network name is no longer available"
# This occurs when clients disconnect before the WebSocket handshake completes.
if hasattr(exc, 'winerror') and exc.winerror == 64:
logger.debug(
"WebSocket accept failed: client disconnected early (WinError 64)"
)
return
# Log other socket errors at warning level
logger.warning("WebSocket accept failed: %s", exc)
return
except asyncio.CancelledError:
# Preserve cooperative cancellation semantics
raise
except Exception as exc:
logger.error("Unexpected error in WebSocket on_connect: %s", exc)
return
# Send welcome message only if accept succeeded
msg = WelcomeMessage(
serverTimeout=self.SERVER_TIMEOUT,
keepAliveInterval=self.KEEP_ALIVE_INTERVAL,
)
try:
await websocket.send_json(msg.model_dump())
except asyncio.CancelledError:
# Let cancellations propagate instead of turning them into warnings
raise
except Exception as exc:
logger.warning("Failed to send welcome message: %s", exc)- Add
import asyncioto the imports near the top ofServer/src/transport/plugin_hub.pyif it is not already present.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds robust error handling for WebSocket connection failures in the PluginHub, specifically addressing WinError 64 that occurs on Windows when Unity clients disconnect during the WebSocket handshake. The changes prevent uncaught task exceptions and noisy error logs during normal Unity editor operations like domain reloads.
Changes:
- Added comprehensive exception handling in
on_connectmethod to gracefully handle OSErrors and unexpected exceptions - Added error handling for welcome message transmission failures
- Configured pytest to automatically detect and run async tests with
asyncio_mode = auto
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Server/src/transport/plugin_hub.py | Added try-except blocks to handle OSError (including Windows WinError 64), other exceptions during WebSocket accept, and welcome message send failures with appropriate logging levels |
| Server/tests/pytest.ini | Added asyncio_mode = auto configuration to enable automatic async test detection for pytest-asyncio |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Server/src/transport/plugin_hub.py
Outdated
Comment on lines
79
to
106
| async def on_connect(self, websocket: WebSocket) -> None: | ||
| await websocket.accept() | ||
| try: | ||
| await websocket.accept() | ||
| except OSError as exc: | ||
| # Handle Windows-specific socket errors during connection accept. | ||
| # WinError 64: "The specified network name is no longer available" | ||
| # This occurs when clients disconnect before the WebSocket handshake completes. | ||
| if hasattr(exc, 'winerror') and exc.winerror == 64: | ||
| logger.debug( | ||
| "WebSocket accept failed: client disconnected early (WinError 64)" | ||
| ) | ||
| return | ||
| # Log other socket errors at warning level | ||
| logger.warning("WebSocket accept failed: %s", exc) | ||
| return | ||
| except Exception as exc: | ||
| logger.error("Unexpected error in WebSocket on_connect: %s", exc) | ||
| return | ||
|
|
||
| # Send welcome message only if accept succeeded | ||
| msg = WelcomeMessage( | ||
| serverTimeout=self.SERVER_TIMEOUT, | ||
| keepAliveInterval=self.KEEP_ALIVE_INTERVAL, | ||
| ) | ||
| await websocket.send_json(msg.model_dump()) | ||
| try: | ||
| await websocket.send_json(msg.model_dump()) | ||
| except Exception as exc: | ||
| logger.warning("Failed to send welcome message: %s", exc) |
There was a problem hiding this comment.
The new error handling for WebSocket connection errors lacks test coverage. Consider adding tests that verify:
- WinError 64 is logged at DEBUG level and handled gracefully
- Other OSErrors are logged at WARNING level
- Unexpected exceptions are logged at ERROR level
- The welcome message failure is logged appropriately
This is especially important since the codebase has comprehensive test coverage for other async functionality as evidenced by the many integration tests.
whatevertogo
force-pushed
the
fix/asyncio-socket-accept-error
branch
5 times, most recently
from
5d66e37 to
9a24c04
Compare
**Windows Socket Accept Error Handling:** - Add event loop exception handler for Windows-specific socket errors - Catch OSError [WinError 64] "ERROR_NETNAME_DELETED" gracefully - Log as WARNING instead of ERROR, allowing server to continue - Only active on Windows (sys.platform == 'win32') - Fixes asyncio IocpProactor.accept() errors when clients disconnect during accept **Parameter Validation Improvements:** - Accept JSON string parameters for complex types in manage_ tools - Support Vector2, Vector3, Vector4, Color, and Quaternion as JSON strings - Improves usability when tools are called from command-line or HTTP APIs - Updated manage_asset, manage_components, manage_gameobject, manage_material, manage_prefabs, and manage_texture tools **Testing:** - Added asyncio_mode = auto to pytest.ini for async test support Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
whatevertogo
force-pushed
the
fix/asyncio-socket-accept-error
branch
from
9a24c04 to
6df7cdf
Compare
whatevertogo
changed the title
whatevertogo
changed the title
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
_normalize_component_listhelper is now duplicated betweenmanage_gameobject.pyandmanage_prefabs.py; consider centralizing this (and the vector normalization) in a shared utility module to avoid drift and keep behavior consistent. - Setting the global
asynciologger level to WARNING on Windows will also suppress unrelated asyncio ERRORs; it may be safer to use a more targeted exception handler or filter that only demotes the specific WinError 64 accept failures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_normalize_component_list` helper is now duplicated between `manage_gameobject.py` and `manage_prefabs.py`; consider centralizing this (and the vector normalization) in a shared utility module to avoid drift and keep behavior consistent.
- Setting the global `asyncio` logger level to WARNING on Windows will also suppress unrelated asyncio ERRORs; it may be safer to use a more targeted exception handler or filter that only demotes the specific WinError 64 accept failures.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+15
to
+52
| def _normalize_vector(value: Any, param_name: str = "vector") -> tuple[list[float] | None, str | None]: | ||
| """ | ||
| Robustly normalize a vector parameter to [x, y, z] format. | ||
| Handles: list, tuple, JSON string. | ||
| Returns (parsed_vector, error_message). If error_message is set, parsed_vector is None. | ||
| """ | ||
| if value is None: | ||
| return None, None | ||
|
|
||
| # If already a list/tuple with 3 elements, convert to floats | ||
| if isinstance(value, (list, tuple)) and len(value) == 3: | ||
| try: | ||
| vec = [float(value[0]), float(value[1]), float(value[2])] | ||
| if all(math.isfinite(n) for n in vec): | ||
| return vec, None | ||
| return None, f"{param_name} values must be finite numbers, got {value}" | ||
| except (ValueError, TypeError): | ||
| return None, f"{param_name} values must be numbers, got {value}" | ||
|
|
||
| # Try parsing as JSON string | ||
| if isinstance(value, str): | ||
| # Check for obviously invalid values | ||
| if value in ("[object Object]", "undefined", "null", ""): | ||
| return None, f"{param_name} received invalid value: '{value}'. Expected [x, y, z] array (list or JSON string)" | ||
|
|
||
| parsed = parse_json_payload(value) | ||
| if isinstance(parsed, list) and len(parsed) == 3: | ||
| try: | ||
| vec = [float(parsed[0]), float(parsed[1]), float(parsed[2])] | ||
| if all(math.isfinite(n) for n in vec): | ||
| return vec, None | ||
| return None, f"{param_name} values must be finite numbers, got {parsed}" | ||
| except (ValueError, TypeError): | ||
| return None, f"{param_name} values must be numbers, got {parsed}" | ||
|
|
||
| return None, f"{param_name} must be a [x, y, z] array (list or JSON string), got: {value}" | ||
|
|
||
| return None, f"{param_name} must be a list or JSON string, got {type(value).__name__}" |
There was a problem hiding this comment.
The _normalize_vector function is duplicated between manage_prefabs.py and manage_gameobject.py. However, the implementation in manage_gameobject.py includes additional legacy support for comma-separated strings (lines 51-63 in manage_gameobject.py) which this version lacks. This inconsistency means the two tools will handle the same type of input differently. Consider either extracting this function to a shared utility module (like services/tools/utils.py) or ensuring both implementations support the same input formats for consistency across tools.
Comment on lines
+55
to
+82
| def _normalize_component_list(value: Any, param_name: str = "component_list") -> tuple[list[str] | None, str | None]: | ||
| """ | ||
| Robustly normalize a component list parameter. | ||
| Handles: list, tuple, JSON string. | ||
| Returns (parsed_list, error_message). If error_message is set, parsed_list is None. | ||
| """ | ||
| if value is None: | ||
| return None, None | ||
|
|
||
| # Already a list/tuple - validate it's a list of strings | ||
| if isinstance(value, (list, tuple)): | ||
| if all(isinstance(item, str) for item in value): | ||
| return list(value), None | ||
| return None, f"{param_name} must be a list of strings, got mixed types in {value}" | ||
|
|
||
| # Try parsing as JSON string | ||
| if isinstance(value, str): | ||
| # Check for obviously invalid values | ||
| if value in ("[object Object]", "undefined", "null", ""): | ||
| return None, f"{param_name} received invalid value: '{value}'. Expected a list of component type names (list or JSON string)" | ||
|
|
||
| parsed = parse_json_payload(value) | ||
| if isinstance(parsed, list) and all(isinstance(item, str) for item in parsed): | ||
| return parsed, None | ||
|
|
||
| return None, f"{param_name} must be a list of strings (component names), got string that parsed to {type(parsed).__name__ if not isinstance(parsed, list) else 'list with non-string items'}" | ||
|
|
||
| return None, f"{param_name} must be a list or JSON string, got {type(value).__name__}" |
There was a problem hiding this comment.
The _normalize_component_list function is duplicated between manage_prefabs.py and manage_gameobject.py with identical implementations (lines 97-124 in manage_gameobject.py). This code duplication violates the DRY principle and increases maintenance burden. Consider extracting this function to a shared utility module like services/tools/utils.py (similar to how normalize_properties is already centralized) so there's a single source of truth for this normalization logic.
Comment on lines
+116
to
+121
| # Suppress asyncio ERROR logs for Windows socket accept errors (WinError 64) | ||
| # This occurs when clients disconnect during accept, which is normal during reconnects | ||
| if sys.platform == 'win32': | ||
| asyncio_logger = logging.getLogger("asyncio") | ||
| asyncio_logger.setLevel(logging.WARNING) | ||
| logger.info("Suppressed asyncio ERROR logs for Windows socket accept errors") |
There was a problem hiding this comment.
The PR description mentions adding error handling in plugin_hub.py for OSError during WebSocket accept, but those changes are not present in this diff. Instead, the approach taken is to suppress asyncio ERROR logs globally for Windows. While this suppresses the noise, it also suppresses ALL asyncio ERROR logs on Windows, not just the WinError 64 from WebSocket accept. A more targeted approach would be to add try-except handling in the on_connect method of plugin_hub.py (lines 79-85) to catch and log OSError at an appropriate level, rather than suppressing all asyncio errors globally.
Comment on lines
+16
to
+20
| """ | ||
| Robustly normalize a vector parameter to [x, y, z] format. | ||
| Handles: list, tuple, JSON string. | ||
| Returns (parsed_vector, error_message). If error_message is set, parsed_vector is None. | ||
| """ |
There was a problem hiding this comment.
The docstring states this function "Handles: list, tuple, JSON string" but the implementation in manage_gameobject.py (lines 16-67) also handles comma-separated strings as a legacy feature. Since this version doesn't include that legacy support, the docstring is accurate. However, this creates an inconsistency in user experience where the same vector parameter behaves differently across tools. Consider documenting this limitation or adding the same legacy support for consistency.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Description
Description
Fix unhandled OSError exceptions during WebSocket accept when clients disconnect before the handshake completes and enhance parameter validation to accept JSON string inputs for complex types (Vector2/3/4, Color, Quaternion) across all manage_* tools.
Type of Change
Root Cause
Issue 1: Windows WebSocket Accept Errors
When Unity editor disconnects during WebSocket handshake (common during domain reloads), the asyncio IOCP proactor on Windows raises OSError [WinError 64] "The specified network name is no longer available". This error was not caught, leading to:
Issue 2: Parameter Type Limitations
The MCP tools previously only accepted native Python dict/list objects as parameters, which caused issues when:
Solution
Solution 1: Graceful WebSocket Error Handling
Solution 2: JSON String Parameter Support
dict[T]todict[T] | str_normalize_vector)_normalize_component_list)Changes Made
WebSocket Error Handling
Server/src/main.py: Add Windows-specific asyncio logger configuration to suppress socket accept error logsParameter Validation Improvements
Server/src/services/tools/manage_asset.py: properties parameter accepts JSON stringServer/src/services/tools/manage_components.py: properties parameter accepts JSON stringServer/src/services/tools/manage_gameobject.py:_normalize_vector()and_normalize_component_list()helper functionsServer/src/services/tools/manage_material.py:Server/src/services/tools/manage_prefabs.py:_normalize_vector()and_normalize_component_list()helper functionsServer/src/services/tools/manage_texture.py:Testing Infrastructure
Server/tests/pytest.ini: Addasyncio_mode = autoconfiguration for async test supportTesting
Automated Testing
Manual MCP Tool Testing
Manual Testing
Examples
Before (Required Native Objects)
After (Also Accepts JSON Strings)
Related Issues
Breaking Changes
None - All changes are backward compatible. Existing code using native dict/list objects continues to work without modification.
Summary by Sourcery
Handle WebSocket connection failures more gracefully, configure pytest for asyncio-aware testing, and accept JSON/string payloads in addition to native dict/list inputs for all manage_* tools.
Summary by CodeRabbit
Bug Fixes
Enhancements
Tests
Summary by Sourcery
Handle Windows WebSocket connection accept errors more gracefully and extend manage_* MCP tools to accept JSON string inputs for complex parameters while keeping existing behavior intact.
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.