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 1 commit into
Conversation
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.
📝 WalkthroughWalkthrough
ChangesStale pin cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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." |
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