-
Notifications
You must be signed in to change notification settings - Fork 0
Fix debouncing race condition in RefreshDebounce #14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix debouncing race condition in RefreshDebounce #14
Conversation
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Summary
This PR addresses a critical race condition in the RefreshDebounce
class within the Unity MCP Bridge's script management system. The RefreshDebounce
class is responsible for coalescing multiple asset refresh requests within a specified time window to prevent excessive Unity asset database operations that can impact editor performance.
The original implementation had a fundamental flaw in its debouncing logic where rapid successive requests could bypass the intended debounce window. This happened because the timing check (now - _last) < window
could be evaluated before the window had fully elapsed, particularly in multi-threaded scenarios or when multiple script changes occurred in quick succession.
The new implementation completely restructures the debouncing mechanism to use a continuous "ticking" approach. Instead of immediately scheduling work when the window appears to have elapsed, it now:
- Records all incoming requests with proper thread synchronization using locks and atomic operations
- Tracks the timestamp of the most recent request (
_lastRequest
) - Uses a recursive
EditorApplication.delayCall
mechanism that continuously checks if the full debounce window has elapsed from the last request - Only executes the accumulated refresh work once the window has completely passed
This change is particularly important in the Unity MCP context because the system handles rapid script modifications and asset imports that need to be batched efficiently. The ManageScript.cs
file appears to be a core component of the MCP bridge that manages Unity script assets, and proper debouncing ensures that compilation and asset database operations don't overwhelm the editor during active development sessions.
Important Files Changed
Filename | Score | Overview |
---|---|---|
UnityMcpBridge/Editor/Tools/ManageScript.cs | 5/5 | Fixed race condition in RefreshDebounce class by implementing proper thread-safe debouncing with continuous ticking mechanism |
Confidence score: 5/5
- This PR is safe to merge with minimal risk as it addresses a clear race condition with a well-structured solution
- Score reflects the focused nature of the change, proper thread safety implementation, and clear improvement to existing flawed logic
- No files require special attention as the change is isolated to a single method with clear threading and timing improvements
Sequence Diagram
sequenceDiagram
participant User
participant ManageScript
participant RefreshDebounce
participant EditorApplication
participant AssetDatabase
participant CompilationPipeline
User->>ManageScript: "HandleCommand (create/update/edit script)"
ManageScript->>ManageScript: "Write script to disk"
ManageScript->>ManageScriptRefreshHelpers: "ScheduleScriptRefresh(relativePath)"
ManageScriptRefreshHelpers->>RefreshDebounce: "Schedule(relPath, 200ms window)"
RefreshDebounce->>RefreshDebounce: "Set _pending = 1, add path to _paths"
RefreshDebounce->>RefreshDebounce: "Update _lastRequest timestamp"
alt First schedule call
RefreshDebounce->>RefreshDebounce: "Set _scheduled = true"
RefreshDebounce->>EditorApplication: "delayCall += Tick(window)"
else Subsequent calls during window
RefreshDebounce->>RefreshDebounce: "Update _lastRequest only"
end
ManageScript-->>User: "Return success response"
loop Until debounce window elapsed
EditorApplication->>RefreshDebounce: "Execute Tick(window)"
RefreshDebounce->>RefreshDebounce: "Check if (UtcNow - _lastRequest) >= window"
alt Window not elapsed
RefreshDebounce->>EditorApplication: "delayCall += Tick(window) again"
end
end
EditorApplication->>RefreshDebounce: "Execute Tick(window) - window elapsed"
RefreshDebounce->>RefreshDebounce: "Set _scheduled = false"
RefreshDebounce->>RefreshDebounce: "Exchange _pending to 0"
RefreshDebounce->>RefreshDebounce: "Get all paths from _paths, clear set"
loop For each path
RefreshDebounce->>AssetDatabase: "ImportAsset(path, ForceUpdate)"
end
RefreshDebounce->>CompilationPipeline: "RequestScriptCompilation()"
1 file reviewed, no comments
Summary
Testing
pytest -q
https://chatgpt.com/codex/tasks/task_e_68a1f53683d483279a293aeade114dbf