fix: prevent execute-dynamic-code execution slot leaks after Undo/reload#1761
Conversation
CommandRunner marked itself running before Undo APIs that can throw during startup/reload, so EndExecution never cleared the flag and later requests kept returning the shared "already in progress" error. Clear the slot even when undo collapse throws, and harden the scheduler so yield-setup failures or dispose throws cannot skip semaphore Release. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCommandRunner now delegates execution and undo lifecycle management to dedicated components. DynamicCodeExecutionScheduler centralizes slot cleanup and semaphore release, including exception paths. New hooks and unit tests cover undo failures, concurrent execution, setup failures, and disposal failures. ChangesExecution Safety
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant DynamicCodeExecutionScheduler
participant ExecutionSemaphore
participant DeferredResources
Caller->>DynamicCodeExecutionScheduler: TryRunIfIdleAsync
DynamicCodeExecutionScheduler->>ExecutionSemaphore: Acquire slot
DynamicCodeExecutionScheduler->>DynamicCodeExecutionScheduler: Setup execution state
DynamicCodeExecutionScheduler->>DeferredResources: Dispose resources during cleanup
DeferredResources-->>DynamicCodeExecutionScheduler: Complete or throw
DynamicCodeExecutionScheduler->>ExecutionSemaphore: Release slot
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In
`@Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/DynamicCodeExecutionScheduler.cs`:
- Around line 143-146: Update the setup-failure catch in the dynamic code
execution scheduler to call the existing ReleaseExecutionSlot helper instead of
directly invoking _executionSemaphore.Release(). Preserve rethrowing the
original exception so failed setup both releases the slot and triggers any
deferred resource disposal and shutdown completion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c61adc54-6f75-49ae-92ea-cf9f4be66abc
⛔ Files ignored due to path filters (3)
Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunnerExecutionSlot.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunnerUndoHooks.cs.metais excluded by none and included by nonetests/DynamicCodeExecutionScheduler.UnitTests/DynamicCodeExecutionScheduler.UnitTests.csprojis excluded by none and included by none
📒 Files selected for processing (7)
Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunner.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunnerExecutionSlot.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/CommandRunnerUndoHooks.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/DynamicCodeExecutionScheduler.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/DynamicCodeExecutionSchedulerHooks.cstests/DynamicCodeExecutionScheduler.UnitTests/CommandRunnerExecutionSlotTests.cstests/DynamicCodeExecutionScheduler.UnitTests/DynamicCodeExecutionSchedulerTests.cs
| catch | ||
| { | ||
| _executionSemaphore.Release(); | ||
| throw; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Run deferred disposal before releasing a failed setup slot.
If Dispose() races after Wait(0), it returns because the semaphore is held; ThrowIfDisposed() then reaches this catch, which releases the semaphore but never calls DisposeResourcesIfRequested(). Resources and shutdown completion remain stranded.
Reuse ReleaseExecutionSlot here so setup failures also complete deferred cleanup.
Proposed fix
catch
{
- _executionSemaphore.Release();
+ ReleaseExecutionSlot(true, executionCancellationTokenSource);
throw;
}📝 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.
| catch | |
| { | |
| _executionSemaphore.Release(); | |
| throw; | |
| catch | |
| { | |
| ReleaseExecutionSlot(true, executionCancellationTokenSource); | |
| throw; |
🤖 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
`@Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/DynamicCodeExecutionScheduler.cs`
around lines 143 - 146, Update the setup-failure catch in the dynamic code
execution scheduler to call the existing ReleaseExecutionSlot helper instead of
directly invoking _executionSemaphore.Release(). Preserve rethrowing the
original exception so failed setup both releases the slot and triggers any
deferred resource disposal and shutdown completion.
Workspace rules forbid unapproved reflection; prove yield-path release via a follow-up foreground enter hook, and keep dispose-throw coverage on call counts plus the post-dispose ObjectDisposedException path. Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
Refs: Obsidian follow-up フォローアップ / まとめブランチ6 PR 6-1 (ODE execution slot stick)
CommandRunnerset_isRunningbeforeUndo.GetCurrentGroup/SetCurrentGroupName. Undo can throwUnityExceptionright after domain reload/startup; the exception escaped outsidetry/finally, soEndExecutionnever cleared the flag. Later requests hit the sameERROR_MESSAGE_EXECUTION_IN_PROGRESSstring used for scheduler busy (vibe looked like a stuckSemaphoreSlim, but the Roslyn worker was idle).EndExecutionnow clears_isRunning/ disposes the CTS infinallyeven ifUndo.CollapseUndoOperationsthrows.Wait(0)release the semaphore; resource dispose throws no longer skipRelease.Evidence / notes
execute_code_exceptionrecordsexception_type=UnityExceptionwith 0–3ms; message text is not stored in vibe.ConfigureAwait(false)can resume off-main-thread before Undo. Follow-up candidate: keep the Undo / invoke path on the main thread.dynamic_executor_created ×2(production factory never returns null).Known limitations / follow-ups (not in this PR)
ReleaseExecutionSlot: ifClearExecutionState/ CTSDisposethrew,DisposeResourcesIfRequestedwould be skipped (neither throws today).Test plan
dotnet test tests/DynamicCodeExecutionScheduler.UnitTests(scheduler +CommandRunnerExecutionSlot; no private-field reflection)uloop compile(0 errors / 0 warnings)uloop execute-dynamic-codemust not stick on "already in progress" until Unity quit