Fix PlayMode tests stalling when unfocused (python refresh utility), improve domain reload recovery and refresh tool #554
Conversation
- TestRunnerService.RunFinished: Always clean up job state even when _runCompletionSource is null (happens after PlayMode domain reload) - TestJobManager: Detect and clear stale jobs (5+ min without updates) on startup to recover from stuck state after domain reload - refresh_unity.py: Add "could not connect" to retryable errors when wait_for_ready=True, so connection failures during domain reload trigger waiting instead of immediate failure Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Reviewer's GuideAdjusts Unity test runner lifecycle handling so test jobs are correctly finalized after domain reloads and stale jobs are auto-recovered on startup, and makes the refresh_unity tool treat connection failures during domain reload as retryable when waiting for Unity to become ready. Sequence diagram for stale test job recovery on startupsequenceDiagram
participant UnityEditorStartup
participant TestJobManager
participant TestRunStatus
UnityEditorStartup->>TestJobManager: TryRestoreFromSessionState()
TestJobManager->>TestRunStatus: Query isRunning state
alt TestRunStatus not running but _currentJobId present
TestJobManager->>TestJobManager: Lookup currentJob in Jobs
alt currentJob.Status == Running and stale (>5 minutes)
TestJobManager->>TestJobManager: Mark job Failed
TestJobManager->>TestJobManager: Set job.Error = "Job orphaned after domain reload"
TestJobManager->>TestJobManager: job.FinishedUnixMs = now
TestJobManager->>TestJobManager: _currentJobId = null
else not stale or not running
TestJobManager->>TestJobManager: Leave job state unchanged
end
else no current job or TestRunStatus still running
TestJobManager->>TestJobManager: No cleanup needed
end
Class diagram for updated test job lifecycle managementclassDiagram
class TestRunnerService {
- object _runCompletionSource
- IList leafResults
+ void RunStarted(ITestAdaptor testsToRun)
+ void RunFinished(ITestResultAdaptor result)
}
class TestJobManager {
<<static>>
- string _currentJobId
- Dictionary~string, TestJob~ Jobs
+ void OnRunFinished()
+ void FinalizeCurrentJobFromRunFinished(TestRunResult payload)
+ void TryRestoreFromSessionState()
}
class TestRunStatus {
<<static>>
+ void MarkFinished()
+ bool IsRunning()
}
class TestJob {
+ string Id
+ TestJobStatus Status
+ string Error
+ long LastUpdateUnixMs
+ long FinishedUnixMs
}
class TestRunResult {
+ static TestRunResult Create(ITestResultAdaptor result, IList leafResults)
}
class TestJobStatus {
<<enumeration>>
Pending
Running
Succeeded
Failed
}
TestRunnerService --> TestRunResult : creates
TestRunnerService --> TestRunStatus : updates
TestRunnerService --> TestJobManager : notifies
TestJobManager --> TestJob : manages
TestJobManager --> TestJobStatus : uses
TestJob --> TestJobStatus : has
TestJobManager --> TestRunResult : consumes
TestRunStatus --> TestJobManager : state queried during restore
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds domain-reload resilience in the editor by clearing stale/orphaned test jobs and making RunFinished perform payload creation/cleanup even without a completion source; server-side introduces cross-platform OS focus nudging to unstick stalled test runs and broadens retry logic for Unity connection errors. Changes
Sequence Diagram(s)sequenceDiagram
participant Poller as RunTests Poller
participant DB as Test Job API/State
participant Unity as Unity Editor
participant OS as Host OS (focus control)
Poller->>DB: fetch test job status
alt job is Running but stalled & editor not focused
Poller->>Poller: should_nudge(status, editor_is_focused, last_update)
Poller->>OS: nudge_unity_focus()
OS->>Unity: bring Unity frontmost
Unity-->>Poller: may progress job (update)
OS->>OS: restore previous focus
Poller->>DB: re-fetch job status
else job progressing or focused
Poller->>DB: wait / continue polling
end
Poller->>DB: return/finalize job status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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. 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}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The 5-minute stale job cutoff in
TestJobManager.TryRestoreFromSessionStateis currently a magic number; consider extracting it to a named constant or config so its intent and tunability are clearer. - In the stale job cleanup path, you directly mutate
currentJob's status and timestamps; if other code relies on a centralized update/notification method for job state changes, consider routing this through that helper to keep behavior consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The 5-minute stale job cutoff in `TestJobManager.TryRestoreFromSessionState` is currently a magic number; consider extracting it to a named constant or config so its intent and tunability are clearer.
- In the stale job cleanup path, you directly mutate `currentJob`'s status and timestamps; if other code relies on a centralized update/notification method for job state changes, consider routing this through that helper to keep behavior consistent.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Services/TestJobManager.cs:190` </location>
<code_context>
+ if (currentJob.Status == TestJobStatus.Running)
+ {
+ long now = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds();
+ long staleCutoffMs = 5 * 60 * 1000; // 5 minutes
+ if (now - currentJob.LastUpdateUnixMs > staleCutoffMs)
+ {
</code_context>
<issue_to_address>
**suggestion:** Extract the stale job cutoff into a named constant or configuration to avoid hard-coded timing logic.
Using the inline `5 * 60 * 1000` magic number makes this timeout harder to manage and align with other components. Consider defining a static readonly constant (e.g., `DefaultStaleJobTimeoutMs`) or reading it from config so the stale job policy is centralized and easier to adjust.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (currentJob.Status == TestJobStatus.Running) | ||
| { | ||
| long now = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); | ||
| long staleCutoffMs = 5 * 60 * 1000; // 5 minutes |
There was a problem hiding this comment.
suggestion: Extract the stale job cutoff into a named constant or configuration to avoid hard-coded timing logic.
Using the inline 5 * 60 * 1000 magic number makes this timeout harder to manage and align with other components. Consider defining a static readonly constant (e.g., DefaultStaleJobTimeoutMs) or reading it from config so the stale job policy is centralized and easier to adjust.
When Unity is unfocused, macOS App Nap (and similar OS features) can throttle the process, causing PlayMode tests to stall even with Unity No Throttling mode enabled. Changes: - Add ApplyNoThrottlingPreemptive() to TestRunnerNoThrottle for early throttle prevention before PlayMode Execute() - Add focus_nudge.py utility that temporarily focuses Unity and returns focus to the original app (supports macOS, Windows, Linux) - Integrate focus nudge into get_test_job polling - when tests appear stalled (unfocused + no progress for 10s), automatically nudge Unity Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
dsarno
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Server/src/utils/focus_nudge.py`:
- Around line 128-146: The PowerShell script currently interpolates window_title
directly into the script string (see variable window_title and the script block
assigned to script), which can break on special characters or allow injection;
change the script to accept the title as a parameter (e.g., start the script
with "param($title)" and use $title in place of the interpolated
"{window_title}"), and invoke PowerShell with the title passed via -ArgumentList
(or the subprocess equivalent) instead of embedding it; update the call site
that uses _get_frontmost_app_windows() or the literal "Unity" to pass the title
as the argument.
🧹 Nitpick comments (1)
Server/src/utils/focus_nudge.py (1)
315-317: Optional: Redundant import.The
timemodule is already imported at module level (line 16), so the local import at line 316 is unnecessary.Suggested fix
if current_time_ms is None: - import time current_time_ms = int(time.time() * 1000)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
MCPForUnity/Editor/Services/TestRunnerNoThrottle.csMCPForUnity/Editor/Services/TestRunnerService.csServer/src/services/tools/run_tests.pyServer/src/utils/focus_nudge.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Services/TestRunnerNoThrottle.csMCPForUnity/Editor/Services/TestRunnerService.cs
🧬 Code graph analysis (1)
Server/src/services/tools/run_tests.py (1)
Server/src/utils/focus_nudge.py (2)
nudge_unity_focus(227-280)should_nudge(283-320)
🪛 Ruff (0.14.11)
Server/src/utils/focus_nudge.py
43-46: Starting a process with a partial executable path
(S607)
53-53: Do not catch blind exception: Exception
(BLE001)
61-61: subprocess call: check for execution of untrusted input
(S603)
62-62: Starting a process with a partial executable path
(S607)
67-67: Consider moving this statement to an else block
(TRY300)
68-68: Do not catch blind exception: Exception
(BLE001)
93-93: subprocess call: check for execution of untrusted input
(S603)
94-94: Starting a process with a partial executable path
(S607)
101-101: Do not catch blind exception: Exception
(BLE001)
147-147: subprocess call: check for execution of untrusted input
(S603)
148-148: Starting a process with a partial executable path
(S607)
153-153: Consider moving this statement to an else block
(TRY300)
154-154: Do not catch blind exception: Exception
(BLE001)
163-163: Starting a process with a partial executable path
(S607)
170-170: Do not catch blind exception: Exception
(BLE001)
181-181: Starting a process with a partial executable path
(S607)
191-191: subprocess call: check for execution of untrusted input
(S603)
192-192: Starting a process with a partial executable path
(S607)
197-197: Consider moving this statement to an else block
(TRY300)
198-198: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (9)
MCPForUnity/Editor/Services/TestRunnerNoThrottle.cs (1)
76-85: LGTM!The
ApplyNoThrottlingPreemptive()method cleanly addresses the PlayMode transition issue by ensuring no-throttling is applied beforeExecute()is called. The implementation correctly marks the test run as active first (for domain reload recovery), then applies the settings. The documentation clearly explains the purpose.MCPForUnity/Editor/Services/TestRunnerService.cs (2)
113-120: LGTM!Good defensive approach for PlayMode tests. The preemptive no-throttling application ensures Unity isn't throttled during the multi-frame Play mode transition, which can otherwise prevent
RunStartedfrom ever being called when Unity is backgrounded.
194-213: LGTM - Robust domain reload handling.The restructured
RunFinishedcorrectly ensures:
- Payload is always created regardless of completion source state
- All cleanup methods execute unconditionally (safe for manual test runs too)
- Result delivery only attempts if there's an awaiting caller
This properly handles the domain reload scenario where
_runCompletionSourceis lost butTestJobManagerstate persists viaSessionState.Server/src/services/tools/run_tests.py (2)
5-6: LGTM!Clean imports for the new focus nudging functionality. The module-level logger follows the standard pattern.
Also applies to: 19-21
219-235: LGTM - Focus nudge integration.The focus nudge logic is well-integrated:
- Correctly extracts
editor_is_focusedfrom progress with a safe default ofTrue(no nudge if unknown)- Uses the
should_nudgehelper to encapsulate stall detection logic- 10-second stall threshold is reasonable
- Logging provides visibility into nudge events
The nudge is appropriately placed before the sleep, giving Unity time to process after being focused.
Server/src/utils/focus_nudge.py (4)
1-36: LGTM - Platform availability checks.Good defensive setup with platform-specific tool detection. The availability check ensures graceful degradation when required tools aren't present.
227-280: LGTM - Well-designed nudge function.Good implementation with:
- Rate limiting using monotonic time (correct for elapsed time measurement)
- Early exits for unavailable platforms and already-focused Unity
- Graceful handling of failures at each step
- Focus restoration to original app
283-320: LGTM - Clean decision logic.The
should_nudgefunction encapsulates the stall detection logic well:
- Only nudges running jobs
- Respects Unity's self-reported focus state
- Handles the "no updates yet" edge case appropriately
- Configurable stall threshold
39-71: Regarding static analysis hints - acceptable patterns.The Ruff warnings for this file are largely acceptable given the context:
- S607 (partial executable path):
osascript,powershell, andxdotoolare standard system utilities expected to be on PATH- BLE001 (blind exception): Catching broad exceptions is intentional here for graceful degradation - the utility should never crash the caller
- TRY300 (return in try): Minor style preference, no functional impact
These patterns are appropriate for a best-effort utility that must handle diverse system configurations gracefully.
Also applies to: 159-200
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| else: | ||
| # Try to find window by title | ||
| script = f''' | ||
| Add-Type @" | ||
| using System; | ||
| using System.Runtime.InteropServices; | ||
| public class Win32 {{ | ||
| [DllImport("user32.dll")] | ||
| public static extern bool SetForegroundWindow(IntPtr hWnd); | ||
| [DllImport("user32.dll")] | ||
| public static extern bool ShowWindow(IntPtr hWnd, int nCmdShow); | ||
| }} | ||
| "@ | ||
| $proc = Get-Process | Where-Object {{$_.MainWindowTitle -eq "{window_title}"}} | Select-Object -First 1 | ||
| if ($proc) {{ | ||
| [Win32]::ShowWindow($proc.MainWindowHandle, 9) | ||
| [Win32]::SetForegroundWindow($proc.MainWindowHandle) | ||
| }} | ||
| ''' |
There was a problem hiding this comment.
Minor: Unsanitized string interpolation in PowerShell script.
The window_title parameter is interpolated directly into the PowerShell script at line 141. While the current usage only passes titles retrieved from _get_frontmost_app_windows() or the literal "Unity", this pattern is fragile. If a window title contains special characters (e.g., quotes, backticks, $), it could break the script or potentially allow command injection.
Consider escaping the window title or using a safer parameter passing mechanism:
Suggested fix using escaped string
else:
# Try to find window by title
+ # Escape special PowerShell characters in window title
+ escaped_title = window_title.replace('"', '`"').replace('$', '`$')
script = f'''
Add-Type @"
using System;
using System.Runtime.InteropServices;
public class Win32 {{
[DllImport("user32.dll")]
public static extern bool SetForegroundWindow(IntPtr hWnd);
[DllImport("user32.dll")]
public static extern bool ShowWindow(IntPtr hWnd, int nCmdShow);
}}
"@
-$proc = Get-Process | Where-Object {{$_.MainWindowTitle -eq "{window_title}"}} | Select-Object -First 1
+$proc = Get-Process | Where-Object {{$_.MainWindowTitle -eq "{escaped_title}"}} | Select-Object -First 1
if ($proc) {{
[Win32]::ShowWindow($proc.MainWindowHandle, 9)
[Win32]::SetForegroundWindow($proc.MainWindowHandle)
}}
'''📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| else: | |
| # Try to find window by title | |
| script = f''' | |
| Add-Type @" | |
| using System; | |
| using System.Runtime.InteropServices; | |
| public class Win32 {{ | |
| [DllImport("user32.dll")] | |
| public static extern bool SetForegroundWindow(IntPtr hWnd); | |
| [DllImport("user32.dll")] | |
| public static extern bool ShowWindow(IntPtr hWnd, int nCmdShow); | |
| }} | |
| "@ | |
| $proc = Get-Process | Where-Object {{$_.MainWindowTitle -eq "{window_title}"}} | Select-Object -First 1 | |
| if ($proc) {{ | |
| [Win32]::ShowWindow($proc.MainWindowHandle, 9) | |
| [Win32]::SetForegroundWindow($proc.MainWindowHandle) | |
| }} | |
| ''' | |
| else: | |
| # Try to find window by title | |
| # Escape special PowerShell characters in window title | |
| escaped_title = window_title.replace('"', '`"').replace('$', '`$') | |
| script = f''' | |
| Add-Type @" | |
| using System; | |
| using System.Runtime.InteropServices; | |
| public class Win32 {{ | |
| [DllImport("user32.dll")] | |
| public static extern bool SetForegroundWindow(IntPtr hWnd); | |
| [DllImport("user32.dll")] | |
| public static extern bool ShowWindow(IntPtr hWnd, int nCmdShow); | |
| }} | |
| "@ | |
| $proc = Get-Process | Where-Object {{$_.MainWindowTitle -eq "{escaped_title}"}} | Select-Object -First 1 | |
| if ($proc) {{ | |
| [Win32]::ShowWindow($proc.MainWindowHandle, 9) | |
| [Win32]::SetForegroundWindow($proc.MainWindowHandle) | |
| }} | |
| ''' |
🤖 Prompt for AI Agents
In `@Server/src/utils/focus_nudge.py` around lines 128 - 146, The PowerShell
script currently interpolates window_title directly into the script string (see
variable window_title and the script block assigned to script), which can break
on special characters or allow injection; change the script to accept the title
as a parameter (e.g., start the script with "param($title)" and use $title in
place of the interpolated "{window_title}"), and invoke PowerShell with the
title passed via -ArgumentList (or the subprocess equivalent) instead of
embedding it; update the call site that uses _get_frontmost_app_windows() or the
literal "Unity" to pass the title as the argument.
- Remove redundant time import (already imported at module level) - Escape window titles in PowerShell script to prevent injection - Remove unused Callable import Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Improve logging in focus_nudge.py: rate limit skip and focus return at INFO level - Improve logging in run_tests.py: show nudge completion status - Fix path resolution in test_logging_stdout.py and test_transport_framing.py - Add PlayMode tests to UnityMCPTests project for testing PlayMode runner Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When running PlayMode tests with Unity in the background, the focus nudge feature may trigger OS permission prompts (especially on macOS for accessibility/automation). Document this expected behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 participant
Summary
refresh_unityfailing during domain reload instead of waitingChanges
RunFinishedTest plan
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.