Drop a stale active-instance pin instead of failing forever - #1266
Drop a stale active-instance pin instead of failing forever#1266lgarczyn wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthrough
ChangesStale pin cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to The change adds bounded recovery for stale editor pins and prevents the current request from silently switching projects, but cleanup may leave an obsolete routing target or clear a newer pin when requests overlap in the same session. This is a bounded merge-readiness risk that should have explicit owner follow-up. Sequence Diagram(s)sequenceDiagram
participant Request
participant UnityInstanceMiddleware
participant PluginHub
participant SessionState
Request->>UnityInstanceMiddleware: inject Unity instance
UnityInstanceMiddleware->>PluginHub: discover registered instances
PluginHub-->>UnityInstanceMiddleware: return registry
UnityInstanceMiddleware->>PluginHub: poll during reconnect window
UnityInstanceMiddleware->>SessionState: clear stale active-instance state
UnityInstanceMiddleware-->>Request: raise ValueError for absent pin
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the problem and intended behavior, but most required template information is missing. The change list, change type, compatibility details, testing status, documentation status, related issues, and additional notes are not completed. Resolution Complete the required template sections. Select the applicable change types, list the implementation and test changes, provide Unity and package-source details or mark them not applicable, record executed tests, document documentation impact, add related issue references if applicable, and include relevant additional notes.
✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Server/src/transport/unity_instance_middleware.py (1)
215-243: 🚀 Performance & Scalability | 🔵 TrivialStale-pin detection adds a
PluginHub.get_sessions()round-trip to every HTTP request with a pinned instance.The logic is correct — empty registry preserves the pin, a registered pin is kept, and a stale pin is dropped and cleared. However,
_drop_stale_pincalls_discover_instances(which callsPluginHub.get_sessions) on every request that has a non-nullactive_instance, even when the pin is valid. This adds a network round-trip to the hot path alongside the existingPluginHub._resolve_session_idcall at line 403.Consider caching the discovery result for a short TTL (similar to the
_tool_visibility_refresh_interval_secondspattern already used in this class) or reusing the session data fetched here to avoid the redundant_resolve_session_idcall when the pin is confirmed valid.
[medium_effort_and_high_reward]🤖 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/src/transport/unity_instance_middleware.py` around lines 215 - 243, The _drop_stale_pin path performs an extra PluginHub.get_sessions round-trip on every request with an active pin. Add short-TTL caching for _discover_instances using the class’s existing _tool_visibility_refresh_interval_seconds pattern, or reuse the fetched session data through the subsequent request flow to avoid redundant discovery/resolution when the pin is valid; preserve empty-registry and stale-pin clearing behavior.
🤖 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.
Nitpick comments:
In `@Server/src/transport/unity_instance_middleware.py`:
- Around line 215-243: The _drop_stale_pin path performs an extra
PluginHub.get_sessions round-trip on every request with an active pin. Add
short-TTL caching for _discover_instances using the class’s existing
_tool_visibility_refresh_interval_seconds pattern, or reuse the fetched session
data through the subsequent request flow to avoid redundant discovery/resolution
when the pin is valid; preserve empty-registry and stale-pin clearing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5adc114b-949f-4895-b72a-f095dd01dd9d
📒 Files selected for processing (2)
Server/src/transport/unity_instance_middleware.pyServer/tests/test_stale_instance_pin.py
…ailing forever (resolved conflict with CoplayDev#1194 launch dir)
…n with explicit routing (keep HEAD stale-pin + middleware fixes) - Server/src/transport/unity_instance_middleware.py: keep HEAD's _file_uri_to_path + _strip_assets (HEAD) + pr/981's _get_http_request_for_binding; keep both imports (parse_qs, unquote, urlparse); resolve conflict in _inject_unity_instance to keep both _drop_stale_pin (HEAD, CoplayDev#1266) and allow_autoselect (pr/981) — now checks stale pin then autoselect with allow_autoselect flag - Server/src/transport/legacy/unity_connection.py: keep HEAD's CoplayDev#1023 available_ids error (more recent than pr/981's suggestions dict) - New file Server/src/services/registry/unity_targeting.py kept from pr/981 - Other Server files auto-merged (resource_registry, tool_registry, plugin_hub, etc.)
|
Hi there, I think this might need to be rebased as well first. Thanks for the PR and let me know if you can resolve these comments! Two changes are needed, and website/docs/architecture/instance-routing.md:181-183 specifies the shape: "Clearing the pin automatically is not the fix. Clearing it and then selecting the one remaining Editor is #1023's exact harm arriving through a different door. The acceptable shape is to clear only after the reconnect wait has expired for that specific hash, and never to retarget silently." |
Review feedback on CoplayDev#1266. instance-routing.md is explicit that clearing a pin and then selecting the one remaining Editor is CoplayDev#1023's harm through another door, and that the acceptable shape is to clear only after the reconnect wait has expired for that hash, never retargeting silently. _drop_stale_pin ran upstream of the poll in _resolve_session_id, so a routine domain reload read as a departure. It now gives the pinned hash the same UNITY_MCP_SESSION_RESOLVE_MAX_WAIT_S window before deciding. Returning None also let a call pinned to project A fall through to _maybe_autoselect_instance, land in project B, report success and re-pin B. A departure now raises and names the instance that went away. The autoselect tests stub transport.plugin_hub, so their stub gains the wait helper the middleware now reads.
a8cd30a to
0dc7d20
Compare
All done |
A pin outlives the editor it names. While it stayed pinned it also suppressed auto-select. Every later call then failed with no_unity_session. Neither waiting nor relaunching the editor recovered. The editor re-registers under its own name, not the name the pin holds. The pin is now dropped once another instance is registered. An empty registry means a domain reload is in flight, so the pin is kept.
Review feedback on CoplayDev#1266. instance-routing.md is explicit that clearing a pin and then selecting the one remaining Editor is CoplayDev#1023's harm through another door, and that the acceptable shape is to clear only after the reconnect wait has expired for that hash, never retargeting silently. _drop_stale_pin ran upstream of the poll in _resolve_session_id, so a routine domain reload read as a departure. It now gives the pinned hash the same UNITY_MCP_SESSION_RESOLVE_MAX_WAIT_S window before deciding. Returning None also let a call pinned to project A fall through to _maybe_autoselect_instance, land in project B, report success and re-pin B. A departure now raises and names the instance that went away. The autoselect tests stub transport.plugin_hub, so their stub gains the wait helper the middleware now reads.
0dc7d20 to
cdbcb21
Compare
2 participants
A pin outlives the editor it names.
While it stayed pinned it also suppressed auto-select. Every later call then failed with no_unity_session. Neither waiting nor relaunching the editor recovered. The editor re-registers under its own name, not the name the pin holds. The pin is now dropped once another instance is registered. An empty registry means a domain reload is in flight, so the pin is kept.
Description
Type of Change
Changes Made
Compatibility / Package Source
#beta,#main, tag, branch, orfile:):Packages/packages-lock.json(if using a Git package URL):Testing/Screenshots/Recordings
cd Server && uv run pytest tests/ -v)Documentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)Related Issues
Additional Notes
Summary by CodeRabbit