Fix script edit retry duplication during reload#792
Merged
dsarno merged 1 commit intoCoplayDev:betafrom Feb 20, 2026
Merged
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideDisables automatic retry-on-reload for non-idempotent script mutation tool calls to Unity, ensuring script edits, creations, and deletions are sent exactly once while preserving retries for read-only operations. Sequence diagram for script mutation calls without retry on reloadsequenceDiagram
actor Developer
participant ScriptTool as ScriptMutationTool
participant Sender as send_with_unity_instance
participant Unity as UnityEditor
Developer->>ScriptTool: Request script change
ScriptTool->>Sender: manage_script(params, retry_on_reload=False)
Sender->>Unity: manage_script(params)
Unity-->>Sender: Response(success or failure)
Sender-->>ScriptTool: Single response
ScriptTool-->>Developer: Report result
rect rgb(235,235,235)
Unity->>Unity: Domain reload may occur
Note over Unity: No retry of mutation
end
Sequence diagram for read-only script operations with retry on reloadsequenceDiagram
actor Developer
participant ReadTool as ReadOnlyScriptTool
participant Sender as send_with_unity_instance
participant Unity as UnityEditor
Developer->>ReadTool: Request script read/validate/get_sha
ReadTool->>Sender: manage_script(params, retry_on_reload=True)
loop Until success or max retries
Sender->>Unity: manage_script(params)
Unity-->>Sender: Response or domain reload
alt Domain reload detected
Sender->>Sender: Wait for Unity to come back
else Successful response
Sender->>Sender: Break loop
end
end
Sender-->>ReadTool: Final response
ReadTool-->>Developer: Return data/result
Flow diagram for retry_on_reload selection in manage_scriptflowchart TD
A[manage_script called] --> B{action value}
B -->|read| C[Call send_with_unity_instance
with retry_on_reload True]
B -->|not read: create, delete, apply_edits, etc.| D[Call send_with_unity_instance
with retry_on_reload False]
C --> E[Read-only operation retries
across Unity domain reload]
D --> F[Mutation operation sent once
no retry on reload]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughAdds explicit retry_on_reload=False for write/manage_script Unity transport calls and replaces threading/time.sleep with asyncio-based background tasks; read operations keep retry_on_reload=True. This prevents automatic retries of non-idempotent edits during Unity domain reloads. Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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 left some high level feedback:
- Consider centralizing the
retry_on_reloaddecision (e.g., inmanage_scriptor a small helper) so that all script-mutating calls share a single source of truth instead of repeatingretry_on_reload=Falseacross multiple call sites. - It may be safer to make
retry_on_reloaddefault toFalseat thesend_with_unity_instance/manage_scriptboundary and explicitly opt in for read-only actions, to reduce the risk of future mutating actions accidentally inheriting retry-on-reload behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider centralizing the `retry_on_reload` decision (e.g., in `manage_script` or a small helper) so that all script-mutating calls share a single source of truth instead of repeating `retry_on_reload=False` across multiple call sites.
- It may be safer to make `retry_on_reload` default to `False` at the `send_with_unity_instance`/`manage_script` boundary and explicitly opt in for read-only actions, to reduce the risk of future mutating actions accidentally inheriting retry-on-reload behavior.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Server/src/services/tools/manage_script.py (1)
356-371:⚠️ Potential issue | 🟡 Minor
_flip_asyncis anasync defused as athreading.Threadtarget — the coroutine is never executed.
threading.Thread(target=_flip_async, daemon=True).start()calls_flip_async()in the new thread, which only creates a coroutine object; Python doesn't execute it, it just creates a coroutine object. Neithertime.sleep(0.1)nor theawait async_send_command_with_retry(...)ever runs, soforce_sentinel_reloadis silently a no-op.The fix is to run the coroutine in the existing event loop from the new thread using
asyncio.run_coroutine_threadsafe:🛠️ Suggested fix
-threading.Thread(target=_flip_async, daemon=True).start() +import asyncio +loop = asyncio.get_event_loop() +asyncio.run_coroutine_threadsafe(_flip_async(), loop)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/services/tools/manage_script.py` around lines 356 - 371, The _flip_async coroutine is never executed because threading.Thread(target=_flip_async, ...) only creates the coroutine object; replace this pattern by scheduling the coroutine on the running event loop with asyncio.run_coroutine_threadsafe. Specifically, make _flip_async use await asyncio.sleep(0.1) instead of time.sleep, keep the try/except around the await of transport.legacy.unity_connection.async_send_command_with_retry(...), and start it from the thread by calling asyncio.run_coroutine_threadsafe(_flip_async(), loop) where loop is the application event loop (capture/passed from the main thread) instead of threading.Thread(...).start(), so the coroutine actually runs and the async_send_command_with_retry executes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Server/src/services/tools/manage_script.py`:
- Around line 356-371: The _flip_async coroutine is never executed because
threading.Thread(target=_flip_async, ...) only creates the coroutine object;
replace this pattern by scheduling the coroutine on the running event loop with
asyncio.run_coroutine_threadsafe. Specifically, make _flip_async use await
asyncio.sleep(0.1) instead of time.sleep, keep the try/except around the await
of transport.legacy.unity_connection.async_send_command_with_retry(...), and
start it from the thread by calling
asyncio.run_coroutine_threadsafe(_flip_async(), loop) where loop is the
application event loop (capture/passed from the main thread) instead of
threading.Thread(...).start(), so the coroutine actually runs and the
async_send_command_with_retry executes.
dsarno
force-pushed
the
fix/script-apply-edits-retry-duplication
branch
from
af31636 to
cfbdd3a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Server/src/services/tools/manage_script.py (1)
339-342:import osis redundant; move standard-library imports to module level.
osis already imported at the module top (line 2); the re-import at line 342 is dead code.asyncio,json, andglobare standard-library modules and should also be hoisted to module-level imports rather than buried inside a conditionaltryblock.♻️ Proposed refactor
At the top of the file, add the missing module-level imports:
import base64 import os +import asyncio +import glob +import json from typing import Annotated, Any, LiteralThen remove the inline imports:
try: - import asyncio - import json - import glob - import os - def _latest_status() -> dict | None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/services/tools/manage_script.py` around lines 339 - 342, Remove the inline standard-library imports in manage_script.py and hoist them to the module level: add "import asyncio", "import json", and "import glob" to the top-of-file imports (where "os" is already imported) and delete the inline block that currently does "import asyncio, json, glob, os"; also remove the redundant re-import of "os" so only the top-level "os" import remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Server/src/services/tools/manage_script.py`:
- Line 370: The call to asyncio.create_task(_flip_async()) can be
garbage-collected because no strong reference is kept; retain a strong reference
to the Task so it isn't dropped (e.g., assign the returned Task to an attribute
or append it to a persistent list/set), and optionally attach a done callback to
log exceptions; update the code that creates the task to store the Task
(reference the asyncio.create_task call and the _flip_async coroutine) and
ensure cleanup after completion.
---
Nitpick comments:
In `@Server/src/services/tools/manage_script.py`:
- Around line 339-342: Remove the inline standard-library imports in
manage_script.py and hoist them to the module level: add "import asyncio",
"import json", and "import glob" to the top-of-file imports (where "os" is
already imported) and delete the inline block that currently does "import
asyncio, json, glob, os"; also remove the redundant re-import of "os" so only
the top-level "os" import remains.
When Unity enters domain reload after a script edit, the retry loop in send_command_with_retry would re-send the identical edit command up to 40 times, duplicating insert_method/anchor_insert edits. Pass retry_on_reload=False on all script-mutating send calls since the edit lands on disk before the reload triggers. Also fix _flip_async coroutine in apply_text_edits that was passed as a threading.Thread target — the coroutine was never awaited. Replace with asyncio.create_task so the sentinel reload flip actually executes. Fixes CoplayDev#790 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dsarno
force-pushed
the
fix/script-apply-edits-retry-duplication
branch
from
cfbdd3a to
dd6de1d
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Server/src/services/tools/manage_script.py (1)
342-346: Move lazy stdlib imports to the module level;import osat line 345 is redundant.
asyncio,json, andglobare imported inside thetry/except Exception: passblock (lines 342–346). Because the handler silently swallows all exceptions, any (unlikely) import failure is invisible. Additionally,osat line 345 is already imported at line 2.Moving these to the top of the file is the idiomatic pattern and would also allow
_background_tasksto carry an accurate type annotation.♻️ Proposed fix
Add at the top of the file (alongside the existing imports):
import base64 +import asyncio +import glob +import json import os from typing import Annotated, Any, LiteralThen remove the now-redundant in-function imports:
if resp.get("success") and (options or {}).get("force_sentinel_reload"): # Optional: flip sentinel via menu if explicitly requested try: - import asyncio - import json - import glob - import os def _latest_status() -> dict | None:And tighten the type annotation on the module-level set:
-_background_tasks: set = set() +_background_tasks: set[asyncio.Task[None]] = set()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/services/tools/manage_script.py` around lines 342 - 346, Move the lazy imports asyncio, json, and glob out of the try/except block and place them at module top alongside existing imports, remove the redundant in-function import of os, and remove the broad try/except that hides import errors; then update the module-level _background_tasks declaration to a precise type (e.g., Set[asyncio.Task] or set[asyncio.Task]) so its annotation is accurate. Ensure you reference the same symbol names (_background_tasks) and update any local code that assumed those imports remained local.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Server/src/services/tools/manage_script.py`:
- Around line 342-346: Move the lazy imports asyncio, json, and glob out of the
try/except block and place them at module top alongside existing imports, remove
the redundant in-function import of os, and remove the broad try/except that
hides import errors; then update the module-level _background_tasks declaration
to a precise type (e.g., Set[asyncio.Task] or set[asyncio.Task]) so its
annotation is accurate. Ensure you reference the same symbol names
(_background_tasks) and update any local code that assumed those imports
remained local.
2 tasks
msanatan
pushed a commit
to msanatan/unity-mcp
that referenced
this pull request
Feb 25, 2026
…oplayDev#792) When Unity enters domain reload after a script edit, the retry loop in send_command_with_retry would re-send the identical edit command up to 40 times, duplicating insert_method/anchor_insert edits. Pass retry_on_reload=False on all script-mutating send calls since the edit lands on disk before the reload triggers. Also fix _flip_async coroutine in apply_text_edits that was passed as a threading.Thread target — the coroutine was never awaited. Replace with asyncio.create_task so the sentinel reload flip actually executes. Fixes CoplayDev#790 Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
msanatan
pushed a commit
to msanatan/unity-mcp
that referenced
this pull request
Feb 25, 2026
… detection (CoplayDev#798) Fixes CoplayDev#795 — the HTTP/WebSocket transport path (PluginHub) now respects retry_on_reload=False and performs pre-send liveness checks, matching the stdio path behavior from PR CoplayDev#792.
msanatan
pushed a commit
to msanatan/unity-mcp
that referenced
this pull request
Feb 25, 2026
…oplayDev#792) When Unity enters domain reload after a script edit, the retry loop in send_command_with_retry would re-send the identical edit command up to 40 times, duplicating insert_method/anchor_insert edits. Pass retry_on_reload=False on all script-mutating send calls since the edit lands on disk before the reload triggers. Also fix _flip_async coroutine in apply_text_edits that was passed as a threading.Thread target — the coroutine was never awaited. Replace with asyncio.create_task so the sentinel reload flip actually executes. Fixes CoplayDev#790 Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
msanatan
pushed a commit
to msanatan/unity-mcp
that referenced
this pull request
Feb 25, 2026
… detection (CoplayDev#798) Fixes CoplayDev#795 — the HTTP/WebSocket transport path (PluginHub) now respects retry_on_reload=False and performs pre-send liveness checks, matching the stdio path behavior from PR CoplayDev#792.
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
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.
Summary
script_apply_edits,apply_text_edits,create_script,delete_script) were retrying non-idempotent commands up to 40 times when Unity entered domain reload, causing duplicated methods/editsretry_on_reload=Falseto all script-mutatingsend_with_unity_instance()calls — the edit lands on disk before the reload triggers, so retrying is unnecessary and harmfulvalidate_script,get_sha,read) are unchanged and still retry as expectedTest plan
create_scriptagainst live Unity — no duplicationscript_apply_editswithinsert_method— method inserted exactly onceapply_text_edits— edit applied exactly oncedelete_script— clean deletionFixes #790
🤖 Generated with Claude Code
Summary by Sourcery
Prevent non-idempotent Unity script mutation operations from being retried across domain reloads to avoid duplicate edits and creations.
Bug Fixes:
Summary by CodeRabbit