fix: make stdio bridge command timeout configurable (default 5m) - #1320
Merged
Conversation
Long-running tool calls (asset imports, test runs, batched edits) were cut off ~30-90s into execution, so the task could never finish. On the stdio transport this was governed by hardcoded values on both hops: - Unity side: StdioBridgeHost.FrameIOTimeoutMs (30s const) capped every command's execution and frame I/O; on timeout the client reconnected and re-sent, which force-closed the prior client and made the bridge restart on a new port (the repeated "StdioBridgeHost started on port 6400/6402" churn). - Server side: ServerConfig.connection_timeout (30s socket recv) and command_total_timeout (90s cross-retry ceiling) cut the command off first. Unlike the WebSocket transport (WebSocketTransportClient reads a per-call timeout off the wire), the stdio bridge had no way to raise these. Make all three configurable with a 5-minute default: - FrameIOTimeoutMs: 30s -> 300s, env UNITY_MCP_STDIO_COMMAND_TIMEOUT_MS. ReceiveTimeout now scales with it (max(60s, timeout)). - connection_timeout: 30s -> 300s, env UNITY_MCP_CONNECTION_TIMEOUT. - command_total_timeout: 90s -> 600s, env UNITY_MCP_COMMAND_TOTAL_TIMEOUT. Invalid/non-positive env values fall back to the default so a bad override can't disable the timeout. Updates the config characterization test to the new defaults.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change makes Unity stdio bridge and server timeouts configurable through environment variables. It adds validated fallback values, derives related bridge thresholds from the configured timeout, and expands timeout configuration tests. ChangesTimeout configuration
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Server/tests/test_core_infrastructure_characterization.py (1)
664-665: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the new environment paths.
The updated test covers only fallback defaults. Add cases for valid overrides and invalid, zero, negative, and non-finite values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Server/tests/test_core_infrastructure_characterization.py` around lines 664 - 665, Extend the configuration tests around the connection_timeout and command_total_timeout assertions to cover environment-variable overrides, including valid values and invalid, zero, negative, and non-finite inputs. Verify valid overrides are applied and each disallowed value preserves the fallback defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Server/src/core/config.py`:
- Around line 16-20: Update _env_float to require math.isfinite(value) in
addition to the existing positive-value check before returning an environment
override. Non-finite inputs such as inf and overflowed values must use the
existing fallback path for MCP timeout settings.
In `@Server/tests/test_core_infrastructure_characterization.py`:
- Around line 664-665: Update the default-value test around ServerConfig() to
remove both environment variables controlling connection_timeout and
command_total_timeout with monkeypatch.delenv(..., raising=False) before
constructing the configuration, then retain the existing default assertions.
---
Nitpick comments:
In `@Server/tests/test_core_infrastructure_characterization.py`:
- Around line 664-665: Extend the configuration tests around the
connection_timeout and command_total_timeout assertions to cover
environment-variable overrides, including valid values and invalid, zero,
negative, and non-finite inputs. Verify valid overrides are applied and each
disallowed value preserves the fallback defaults.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 926d20a0-607f-42a9-b299-9bbb114bb9ad
📒 Files selected for processing (3)
MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.csServer/src/core/config.pyServer/tests/test_core_infrastructure_characterization.py
Address CodeRabbit review on #1320: - _env_float now requires math.isfinite(value): "inf"/"Infinity"/"1e309"/ "nan" are positive-or-parseable but would produce unusable socket/timeout behaviour, so they now fall back to the default like other invalid input. - test_config_default_values clears the two env vars first so ambient env can't mask the defaults. - Added coverage for valid overrides and for invalid/zero/negative/ non-finite values falling back to defaults.
Author
|
Thanks @coderabbitai — addressed in 7997788:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Problem
Long-running tool calls on the stdio transport (asset imports,
run_tests, batchedmanage_gameobject/execute_code, etc.) are cut off ~30–90s into execution and can never finish. In the Unity console the bridge visibly restarts on alternating ports:Root cause
On the stdio path the timeout is hardcoded on both hops, with no way to raise it (unlike the WebSocket transport, where
WebSocketTransportClientreads a per-calltimeoutoff the wire atWebSocketTransportClient.cs:605):StdioBridgeHost.FrameIOTimeoutMs = 30000(const)StdioBridgeHostclient.ReceiveTimeout = 600002×)StdioBridgeHoststaleThresholdMsServerConfig.connection_timeout = 30.0ServerConfig.command_total_timeout = 90.0When the 30s/90s cap fires, the Python client closes the socket and re-sends the command on a fresh connection; the new connection makes Unity force-close the prior client and re-listen — the restart/port-churn seen above.
Fix
Make all three configurable, defaulting to 5 minutes (from the historical 30s), with env-var overrides. Invalid or non-positive values fall back to the default so a bad override can't disable the timeout.
C# —
StdioBridgeHost.csFrameIOTimeoutMs:30000→ default300000, overrideUNITY_MCP_STDIO_COMMAND_TIMEOUT_MS. Changed fromconstto astatic readonlyresolved once at load.client.ReceiveTimeoutnowMath.Max(60000, FrameIOTimeoutMs)so it never fires before the command timeout.staleThresholdMskept at2× FrameIOTimeoutMs(was aconst, now a local since the operand is no longer compile-time constant).Python —
core/config.pyconnection_timeout:30.0→ default300.0, overrideUNITY_MCP_CONNECTION_TIMEOUT.command_total_timeout:90.0→ default600.0(kept aboveconnection_timeout), overrideUNITY_MCP_COMMAND_TOTAL_TIMEOUT._env_floathelper that ignores invalid/non-positive values.Tests
test_core_infrastructure_characterization.pyto assert the new defaults (connection_timeout == 300.0,command_total_timeout == 600.0).Notes / testing
config.pyverified in isolation (defaults300.0/600.0; a valid override applies; a bad value falls back). The repo's pytest collection currently fails on an unrelated missingtomliimport incore/telemetry.py, so the full suite wasn't run in this environment.UNITY_MCP_*env-var convention (UNITY_MCP_TIMEOUT,UNITY_MCP_DISABLE_TELEMETRY,UNITY_MCP_ALLOW_BATCH, …).Summary by CodeRabbit
New Features
Tests