Server: add robust shutdown on stdio detach (signal handlers, stdin/p… #91
Conversation
…arent monitoring, forced exit)\n\nTests: move telemetry tests to tests/ and convert to pytest asserts\n\nNotes: Avoids zombie processes when client disconnects; keeps tests centralized.
|
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. WalkthroughThis PR removes deprecated telemetry test infrastructure and cleans up test project assets. The old server/test_telemetry.py is replaced with a new tests/test_telemetry_server.py using pytest conventions and monkeypatching for test isolation. Multiple Unity test scripts and metadata files are deleted from the test project. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Areas for attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (13)
💤 Files with no reviewable changes (13)
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Implements robust shutdown mechanisms to prevent zombie processes when clients disconnect via stdio, and refactors telemetry tests to use pytest conventions.
Key changes:
- Added signal handlers (
SIGTERM,SIGINT,SIGBREAK) andSIGPIPEignoring for cross-platform shutdown support - Implemented background thread monitoring stdin EOF and parent process lifecycle
- Added
_shutdown_flagthreading event for coordinated shutdown across multiple paths - Introduced
_force_exit()withos._exit(0)fallback to terminate lingering background threads - Migrated telemetry tests from
Server/test_telemetry.pytotests/test_telemetry_server.pywith pytest-style assertions
Issues found:
- Race condition: multiple shutdown paths can start duplicate
_force_exit()timers - Inconsistent handling:
BrokenPipeErrorcalls_force_exit()immediately while other paths use timers
Confidence Score: 3/5
- This PR solves the zombie process problem but introduces race conditions in shutdown logic that should be fixed
- The shutdown implementation achieves its goal of preventing zombie processes, but has logical flaws: (1) BrokenPipeError path calls _force_exit() immediately instead of using a timer, creating inconsistent behavior, and (2) multiple shutdown paths can spawn duplicate timers leading to race conditions. The test refactoring is clean and safe. These are functional bugs that won't cause crashes but could lead to non-deterministic shutdown behavior.
- Pay close attention to
Server/server.pyshutdown paths (lines 239-242 and 270-273)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| Server/server.py | 3/5 | Adds robust shutdown mechanisms with signal handlers, stdin monitoring, and forced exit timers. Contains race conditions in shutdown paths and inconsistent BrokenPipeError handling. |
| Server/test_telemetry.py | 5/5 | File deleted - telemetry tests moved to centralized tests/ directory |
| tests/test_telemetry_server.py | 5/5 | Clean refactor to pytest-style assertions, properly uses monkeypatch fixture, and maintains test coverage |
Sequence Diagram
sequenceDiagram
participant Client
participant Main
participant Monitor
participant SignalHandler
participant ForceExit
Note over Main: Server starts
Main->>SignalHandler: Register SIGTERM/SIGINT/SIGBREAK
Main->>Monitor: Start daemon thread
Main->>Main: mcp.run(transport='stdio')
alt Client Disconnects (stdin EOF)
Client->>Monitor: stdin closes
Monitor->>Monitor: Detect stdin.closed or fd<0
Monitor->>Monitor: Set _shutdown_flag
Monitor->>ForceExit: Timer(0.5s, _force_exit)
ForceExit-->>Main: sys.exit(0) or os._exit(0)
end
alt Parent Process Dies
Client->>Monitor: Parent PID invalid
Monitor->>Monitor: os.kill(parent_pid, 0) fails
Monitor->>Monitor: Set _shutdown_flag
Monitor->>ForceExit: Timer(0.5s, _force_exit)
ForceExit-->>Main: sys.exit(0) or os._exit(0)
end
alt Signal Received
Client->>SignalHandler: Send SIGTERM/SIGINT
SignalHandler->>SignalHandler: Set _shutdown_flag
SignalHandler->>ForceExit: Timer(1.0s, _force_exit)
ForceExit-->>Main: sys.exit(0) or os._exit(0)
end
alt BrokenPipeError
Main->>Main: Catch BrokenPipeError
Main->>Main: Set _shutdown_flag
Main->>ForceExit: Immediate _force_exit()
ForceExit-->>Main: sys.exit(0) or os._exit(0)
end
Note over Main: finally block
Main->>Main: Set _shutdown_flag
Main->>ForceExit: Timer(0.5s, _force_exit)
ForceExit-->>Main: sys.exit(0) or os._exit(0)
3 files reviewed, 2 comments
Server/server.py
Outdated
| except BrokenPipeError: | ||
| logger.info("Broken pipe; shutting down") | ||
| _shutdown_flag.set() | ||
| _force_exit() |
There was a problem hiding this comment.
logic: calling _force_exit() immediately on BrokenPipeError bypasses the timer coordination used in other paths, creating inconsistent shutdown behavior
| except BrokenPipeError: | |
| logger.info("Broken pipe; shutting down") | |
| _shutdown_flag.set() | |
| _force_exit() | |
| except BrokenPipeError: | |
| logger.info("Broken pipe; shutting down") | |
| _shutdown_flag.set() |
Prompt To Fix With AI
This is a comment left during a code review.
Path: Server/server.py
Line: 270:273
Comment:
**logic:** calling `_force_exit()` immediately on `BrokenPipeError` bypasses the timer coordination used in other paths, creating inconsistent shutdown behavior
```suggestion
except BrokenPipeError:
logger.info("Broken pipe; shutting down")
_shutdown_flag.set()
```
How can I resolve this? If you propose a fix, please make it concise.
Server/server.py
Outdated
| if not _shutdown_flag.is_set(): | ||
| logger.info("Client disconnected (stdin or parent), initiating shutdown...") | ||
| _shutdown_flag.set() | ||
| threading.Timer(0.5, _force_exit).start() |
There was a problem hiding this comment.
logic: check if not _shutdown_flag.is_set() before starting timer to prevent duplicate shutdown timers when multiple paths trigger simultaneously
Prompt To Fix With AI
This is a comment left during a code review.
Path: Server/server.py
Line: 239:242
Comment:
**logic:** check `if not _shutdown_flag.is_set()` before starting timer to prevent duplicate shutdown timers when multiple paths trigger simultaneously
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Server/server.py(3 hunks)Server/test_telemetry.py(0 hunks)tests/test_telemetry_server.py(1 hunks)
💤 Files with no reviewable changes (1)
- Server/test_telemetry.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T05:19:59.247Z
Learnt from: dsarno
Repo: dsarno/unity-mcp PR: 85
File: tests/test_telemetry_queue_worker.py:33-37
Timestamp: 2025-10-21T05:19:59.247Z
Learning: In the unity-mcp repository, pyproject.toml is located in MCPForUnity/UnityMcpServer~/src/, not at the repository root. When testing code that imports telemetry.py, the working directory should be changed to SRC (MCPForUnity/UnityMcpServer~/src/) so that telemetry.py's get_package_version() can find pyproject.toml via relative path.
Applied to files:
tests/test_telemetry_server.py
🪛 Ruff (0.14.2)
Server/server.py
203-203: Unused function argument: frame
(ARG001)
235-237: try-except-pass detected, consider logging the exception
(S110)
235-235: Do not catch blind exception: Exception
(BLE001)
243-245: try-except-pass detected, consider logging the exception
(S110)
243-243: Do not catch blind exception: Exception
(BLE001)
257-259: try-except-pass detected, consider logging the exception
(S110)
257-257: Do not catch blind exception: Exception
(BLE001)
| def test_telemetry_disabled(monkeypatch): | ||
| # Disable telemetry via env and reload module | ||
| monkeypatch.setenv("DISABLE_TELEMETRY", "true") | ||
| import telemetry | ||
|
|
||
| importlib.reload(telemetry) | ||
|
|
||
| from telemetry import is_telemetry_enabled, record_telemetry, RecordType | ||
|
|
||
| assert is_telemetry_enabled() is False | ||
|
|
||
| # Should not raise even when disabled (no-op) | ||
| record_telemetry(RecordType.USAGE, {"test": "should_be_ignored"}) | ||
|
|
There was a problem hiding this comment.
Reload telemetry after clearing the disable flag.
This test reloads telemetry while DISABLE_TELEMETRY=true, but never reloads it once the env var is cleared, so later tests keep running against a cached “disabled” module. Restore the env and reload so subsequent tests see the normal enabled state.
def test_telemetry_disabled(monkeypatch):
# Disable telemetry via env and reload module
monkeypatch.setenv("DISABLE_TELEMETRY", "true")
import telemetry
importlib.reload(telemetry)
from telemetry import is_telemetry_enabled, record_telemetry, RecordType
assert is_telemetry_enabled() is False
# Should not raise even when disabled (no-op)
record_telemetry(RecordType.USAGE, {"test": "should_be_ignored"})
+
+ # Restore module state for subsequent tests
+ monkeypatch.delenv("DISABLE_TELEMETRY", raising=False)
+ importlib.reload(telemetry)🤖 Prompt for AI Agents
In tests/test_telemetry_server.py around lines 37 to 50, the test sets
DISABLE_TELEMETRY=true and reloads the telemetry module but never restores the
environment or reloads telemetry afterward, leaving subsequent tests with the
module stuck in the disabled state; after calling record_telemetry, delete or
reset the DISABLE_TELEMETRY env var (e.g.,
monkeypatch.delenv("DISABLE_TELEMETRY") or
monkeypatch.setenv("DISABLE_TELEMETRY", "false")) and then
importlib.reload(telemetry) so the telemetry module is reloaded with the normal
enabled state for later tests.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
TestProjects/UnityMCPTests/Assets/Scripts/AudioPlayer.cs (1)
47-51: Consider simplifying the public API.The
PlayAudioClip()method is a thin wrapper that just delegates toPlayAudio()without adding functionality. Consider makingPlayAudio()public directly instead, or add distinct behavior to justify the wrapper.Apply this diff if you want to simplify:
- private void PlayAudio() + public void PlayAudio() { if (audioSource != null && audioSource.clip != null) { // Stop any currently playing audio and play the clip audioSource.Stop(); audioSource.Play(); Debug.Log("Playing audio: " + audioSource.clip.name); } else { Debug.LogWarning("AudioSource or AudioClip is missing on " + gameObject.name); } } - - // Public method to play audio (can be called from other scripts or UI) - public void PlayAudioClip() - { - PlayAudio(); - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (24)
Server/uv.lockis excluded by!**/*.lockTestProjects/UnityMCPTests/Assets/Materials/DebugTest.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/GlowingMetallicBlue.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/GreenMaterial.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/JsonTestMaterial.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/RedMaterial.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/SE_Fire.m4ais excluded by!**/*.m4aTestProjects/UnityMCPTests/Assets/Materials/TestMaterial7.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/TestMaterial8.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/water.mp3is excluded by!**/*.mp3TestProjects/UnityMCPTests/Assets/Temp/JsonTest.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Temp/LiveTests/BadJson_TEST01.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Temp/LiveTests/BadJson_addbc94a.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Temp/LiveTests/CleanMat_addbc94a.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Temp/LiveTests/FreshMat_addbc94a.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Temp/LiveTests/LiveMat_API.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Temp/LiveTests/Mat_TEST01.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Temp/LiveTests/Mat_addbc94a.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Temp/LiveTests/SmokeMat.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Temp/LiveTests/Test5BaseMap_addbc94a.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Temp/LiveTests/Test5Final_addbc94a.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Temp/LiveTests/Test5Fresh2_addbc94a.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Temp/LiveTests/Test5Fresh_addbc94a.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Test.matis excluded by!**/*.mat
📒 Files selected for processing (30)
Material Verification Plan.md(1 hunks)TestProjects/UnityMCPTests/Assets/Editor/GenTempTex.cs.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Editor/GenTempTex.cs/GenTempTex.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Editor/GenTempTex.cs/GenTempTex.cs.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Materials/DebugTest.mat.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Materials/GlowingMetallicBlue.mat.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Materials/GreenMaterial.mat.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Materials/JsonTestMaterial.mat.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Materials/RedMaterial.mat.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Materials/SE_Fire.m4a.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Materials/TestMaterial7.mat.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Materials/TestMaterial8.mat.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Materials/water.mp3.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Scripts/AudioPlayer.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Scripts/AudioPlayer.cs.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Temp/JsonTest.mat.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Temp/LiveTests/BadJson_TEST01.mat.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Temp/LiveTests/BadJson_addbc94a.mat.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Temp/LiveTests/CleanMat_addbc94a.mat.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Temp/LiveTests/FreshMat_addbc94a.mat.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Temp/LiveTests/LiveMat_API.mat.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Temp/LiveTests/Mat_TEST01.mat.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Temp/LiveTests/Mat_addbc94a.mat.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Temp/LiveTests/SmokeMat.mat.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Temp/LiveTests/TempBaseTex.asset.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Temp/LiveTests/Test5BaseMap_addbc94a.mat.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Temp/LiveTests/Test5Final_addbc94a.mat.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Temp/LiveTests/Test5Fresh2_addbc94a.mat.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Temp/LiveTests/Test5Fresh_addbc94a.mat.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Test.mat.meta(1 hunks)
✅ Files skipped from review due to trivial changes (24)
- TestProjects/UnityMCPTests/Assets/Temp/LiveTests/CleanMat_addbc94a.mat.meta
- TestProjects/UnityMCPTests/Assets/Test.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/DebugTest.mat.meta
- TestProjects/UnityMCPTests/Assets/Scripts/AudioPlayer.cs.meta
- TestProjects/UnityMCPTests/Assets/Materials/GreenMaterial.mat.meta
- TestProjects/UnityMCPTests/Assets/Temp/LiveTests/BadJson_TEST01.mat.meta
- TestProjects/UnityMCPTests/Assets/Temp/LiveTests/LiveMat_API.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/TestMaterial7.mat.meta
- TestProjects/UnityMCPTests/Assets/Temp/JsonTest.mat.meta
- TestProjects/UnityMCPTests/Assets/Temp/LiveTests/BadJson_addbc94a.mat.meta
- TestProjects/UnityMCPTests/Assets/Temp/LiveTests/TempBaseTex.asset.meta
- TestProjects/UnityMCPTests/Assets/Temp/LiveTests/SmokeMat.mat.meta
- TestProjects/UnityMCPTests/Assets/Editor/GenTempTex.cs.meta
- TestProjects/UnityMCPTests/Assets/Temp/LiveTests/FreshMat_addbc94a.mat.meta
- TestProjects/UnityMCPTests/Assets/Editor/GenTempTex.cs/GenTempTex.cs.meta
- TestProjects/UnityMCPTests/Assets/Temp/LiveTests/Mat_TEST01.mat.meta
- TestProjects/UnityMCPTests/Assets/Temp/LiveTests/Mat_addbc94a.mat.meta
- TestProjects/UnityMCPTests/Assets/Temp/LiveTests/Test5BaseMap_addbc94a.mat.meta
- TestProjects/UnityMCPTests/Assets/Temp/LiveTests/Test5Fresh_addbc94a.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/JsonTestMaterial.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/TestMaterial8.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/SE_Fire.m4a.meta
- TestProjects/UnityMCPTests/Assets/Materials/water.mp3.meta
- TestProjects/UnityMCPTests/Assets/Temp/LiveTests/Test5Final_addbc94a.mat.meta
🧰 Additional context used
🪛 LanguageTool
Material Verification Plan.md
[style] ~4-~4: To elevate your writing, try using a synonym here.
Context: ...efinements to make it deterministic and hard to regress. ### Suggested test additio...
(HARD_TO)
🔇 Additional comments (7)
TestProjects/UnityMCPTests/Assets/Temp/LiveTests/Test5Fresh2_addbc94a.mat.meta (1)
1-8: The original review comment is based on incorrect assumptions and should be disregarded.The analysis revealed that:
Temp/ files ARE intentionally tracked: The repository's .gitignore explicitly does NOT exclude the
/Temp/directory. The fileTest5Fresh2_addbc94a.mat.metaand 14+ related test asset files are deliberately committed to version control, as confirmed bygit ls-files.PR scope alignment IS correct: The repository's test infrastructure deliberately stores test fixture materials in
Assets/Temp/LiveTests/, which supports the telemetry and material testing referenced in the PR. The searchable codebase confirms telemetry IS part of the PR scope (telemetry.py, telemetry_decorator.py, extensive test coverage).GUID concerns were unverified: The claim about GUID mismatches between summaries was not substantiated by actual code analysis and appears to be a spurious observation.
Standard guidance doesn't apply here: While standard Unity projects exclude
/Temp/, this repository has intentionally configured git to track these test assets. The recommendation to add them to.gitignoreis inappropriate for this project's testing strategy.Likely an incorrect or invalid review comment.
TestProjects/UnityMCPTests/Assets/Materials/RedMaterial.mat.meta (1)
1-8: Standard Unity metadata - no concerns.This is auto-generated Unity asset metadata with standard import settings. No review needed.
TestProjects/UnityMCPTests/Assets/Materials/GlowingMetallicBlue.mat.meta (1)
1-8: Standard Unity metadata - no concerns.Auto-generated Unity asset metadata with standard import settings. No review needed.
TestProjects/UnityMCPTests/Assets/Scripts/AudioPlayer.cs (3)
8-21: Good defensive initialization.The Awake method correctly handles component retrieval fallback and disables
playOnAwakefor manual control. The defensive null checks are appropriate.
23-30: Input handling is appropriate for test infrastructure.The use of Unity's legacy Input system is acceptable for test code and is properly documented in the comment.
32-45: Audio playback logic is correct.The method properly validates the AudioSource and clip before playback, stops any currently playing audio, and provides clear logging for debugging.
Material Verification Plan.md (1)
1-24: Comprehensive testing plan with practical recommendations.The document provides thorough coverage of material property verification scenarios, including edge cases (wrong types, keyword dependencies, persistence, atomicity) and practical implementation guidance. The recommendations for deterministic visual testing and structured error codes are particularly valuable.
The static analysis style suggestion for "hard to" on line 4 can be safely ignored—the phrase is clear and appropriate in this context.
Also applies to: 45-92
Material Verification Plan.md
Outdated
| ```csharp | ||
| public static Color ComputeAverage(RenderTexture rt) | ||
| { | ||
| var prev = RenderTexture.active; | ||
| RenderTexture.active = rt; | ||
| var tex = new Texture2D(rt.width, rt.height, TextureFormat.RGBA32, false, true); | ||
| tex.ReadPixels(new Rect(0, 0, rt.width, rt.height), 0, 0); | ||
| tex.Apply(); | ||
| RenderTexture.active = prev; | ||
|
|
||
| var pixels = tex.GetPixels(); | ||
| float r = 0f, g = 0f, b = 0f, a = 0f; | ||
| for (int i = 0; i < pixels.Length; i++) | ||
| { | ||
| r += pixels[i].r; g += pixels[i].g; b += pixels[i].b; a += pixels[i].a; | ||
| } | ||
| float inv = 1f / pixels.Length; | ||
| return new Color(r * inv, g * inv, b * inv, a * inv); | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Memory leak in example code: Texture2D is never destroyed.
The ComputeAverage helper creates a Texture2D (line 30) but never destroys it, which will cause a memory leak. Since this is example code that will be referenced by developers, it should demonstrate proper resource cleanup.
Apply this diff to fix the memory leak:
public static Color ComputeAverage(RenderTexture rt)
{
var prev = RenderTexture.active;
RenderTexture.active = rt;
var tex = new Texture2D(rt.width, rt.height, TextureFormat.RGBA32, false, true);
tex.ReadPixels(new Rect(0, 0, rt.width, rt.height), 0, 0);
tex.Apply();
RenderTexture.active = prev;
var pixels = tex.GetPixels();
float r = 0f, g = 0f, b = 0f, a = 0f;
for (int i = 0; i < pixels.Length; i++)
{
r += pixels[i].r; g += pixels[i].g; b += pixels[i].b; a += pixels[i].a;
}
float inv = 1f / pixels.Length;
+ Object.DestroyImmediate(tex);
return new Color(r * inv, g * inv, b * inv, a * inv);
}📝 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.
| ```csharp | |
| public static Color ComputeAverage(RenderTexture rt) | |
| { | |
| var prev = RenderTexture.active; | |
| RenderTexture.active = rt; | |
| var tex = new Texture2D(rt.width, rt.height, TextureFormat.RGBA32, false, true); | |
| tex.ReadPixels(new Rect(0, 0, rt.width, rt.height), 0, 0); | |
| tex.Apply(); | |
| RenderTexture.active = prev; | |
| var pixels = tex.GetPixels(); | |
| float r = 0f, g = 0f, b = 0f, a = 0f; | |
| for (int i = 0; i < pixels.Length; i++) | |
| { | |
| r += pixels[i].r; g += pixels[i].g; b += pixels[i].b; a += pixels[i].a; | |
| } | |
| float inv = 1f / pixels.Length; | |
| return new Color(r * inv, g * inv, b * inv, a * inv); | |
| } | |
| ``` | |
| public static Color ComputeAverage(RenderTexture rt) | |
| { | |
| var prev = RenderTexture.active; | |
| RenderTexture.active = rt; | |
| var tex = new Texture2D(rt.width, rt.height, TextureFormat.RGBA32, false, true); | |
| tex.ReadPixels(new Rect(0, 0, rt.width, rt.height), 0, 0); | |
| tex.Apply(); | |
| RenderTexture.active = prev; | |
| var pixels = tex.GetPixels(); | |
| float r = 0f, g = 0f, b = 0f, a = 0f; | |
| for (int i = 0; i < pixels.Length; i++) | |
| { | |
| r += pixels[i].r; g += pixels[i].g; b += pixels[i].b; a += pixels[i].a; | |
| } | |
| float inv = 1f / pixels.Length; | |
| Object.DestroyImmediate(tex); | |
| return new Color(r * inv, g * inv, b * inv, a * inv); | |
| } |
🤖 Prompt for AI Agents
Material Verification Plan.md around lines 25 to 44: the example ComputeAverage
creates a Texture2D and never releases it, causing a memory leak; after
tex.Apply() (and after you finish using tex.GetPixels()) explicitly destroy the
texture (e.g., call UnityEngine.Object.Destroy(tex) or DestroyImmediate(tex) as
appropriate for editor vs runtime) and ensure RenderTexture.active is restored
in a finally block so cleanup always runs.
| static GenTempTex() | ||
| { | ||
| try | ||
| { | ||
| var folder = "Assets/Temp/LiveTests"; | ||
| if (!AssetDatabase.IsValidFolder("Assets/Temp")) AssetDatabase.CreateFolder("Assets", "Temp"); | ||
| if (!AssetDatabase.IsValidFolder(folder)) AssetDatabase.CreateFolder("Assets/Temp", "LiveTests"); | ||
| CreateSolidTextureAsset($"{folder}/TempBaseTex.asset", Color.white); | ||
| } | ||
| catch {} |
There was a problem hiding this comment.
Don't swallow initialization failures.
An empty catch {} inside this [InitializeOnLoad] static constructor makes any initialization failure permanent and silent. Static constructors run only once per domain reload, so if asset creation fails (bad path, I/O, etc.) the class stays partially initialized with no diagnostics, and dependent editor tooling breaks unpredictably. At minimum, log and rethrow (or otherwise surface) the exception so the domain fails fast instead of hiding the fault.(learn.microsoft.com)
- catch {}
+ catch (Exception ex)
+ {
+ Debug.LogError($"GenTempTex failed to prepare temp assets: {ex}");
+ throw;
+ }🤖 Prompt for AI Agents
In TestProjects/UnityMCPTests/Assets/Editor/GenTempTex.cs/GenTempTex.cs around
lines 7 to 16, the static constructor swallows all exceptions with an empty
catch block which hides initialization failures; change the catch to surface the
error by logging the exception (e.g., Debug.LogException or
EditorUtility.DisplayDialog/Debug.LogError with the exception message) and
rethrowing it (throw;) so the domain reload fails fast and diagnostics are
visible; ensure System.Exception is caught, UnityEditor methods used are
available in this editor-only file, and avoid leaving an empty catch.
…arent monitoring, forced exit)\n\nTests: move telemetry tests to tests/ and convert to pytest asserts\n\nNotes: Avoids zombie processes when client disconnects; keeps tests centralized.
Summary by CodeRabbit
Tests
Chores