Repository navigation
fix: Server recovery and the setup wizard no longer wait for the Unity window to be focused - #3171
Merged
Merged
Conversation
…tead of delayCall Some Editor sessions stop flushing EditorApplication.delayCall while EditorApplication.update keeps running, so the settings file recovery queued at Editor startup never ran there. The main-thread dispatcher drains its queue on both update and tick, so the recovery now rides on it. ToolContracts now exposes its internals to Infrastructure, Presentation, and the Compile tool, so the remaining delayCall sites in those assemblies can hand their work to the same dispatcher.
…f delayCall The startup recovery and the recovery after an unexpected server loop exit were queued on EditorApplication.delayCall, which some Editor sessions stop flushing while update keeps running. There the server would not come back on its own. The dispatcher drains on update and tick and signals a tick when work is enqueued, so the manual SignalTick after the old delayCall registration is no longer needed. The startup scheduler parameter is renamed to describe what it does now, and a test counter that counted readiness retry waits, not delayCall registrations, gets a name that says so.
…yCall When the compile watchdog faults, its continuation runs on the thread pool and queued the abort on EditorApplication.delayCall. In an Editor session that stops flushing delayCall, the abort never ran, so a compile request that only the watchdog could end might stay unanswered. The dispatcher drains on update and tick, so the abort now reaches the main thread there too.
The version check that decides whether to show the setup wizard queued the window on EditorApplication.delayCall. In an Editor session that does not flush delayCall, the wizard never appeared, even when the CLI needed an update or the installed skills were outdated. The check already resumes on Unity's synchronization context after each await, so it is on the main thread and can show the window itself. New tests pin that a package change with an outdated CLI or outdated skills shows the window synchronously, and the source pin now checks that the flow registers nothing on delayCall instead of requiring it.
… instead of delayCall Toggling a tool in the settings window deferred the skill file sync to EditorApplication.delayCall to keep the UI responsive. In an Editor session that does not flush delayCall, the skill files never followed the toggle. The dispatcher drains on the next update or tick, so the work is still deferred but now runs.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughEditor startup and recovery callbacks now use the main-thread dispatcher instead of ChangesEditor callback dispatch
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue remains in the changed callback paths; the PR is mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The inspected safeguards and permissions remain in place, and no introduced security issue was established. The remaining uncertainty concerns pending recovery work during shutdown and behavior in an actual background Editor session. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
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
EditorApplication.delayCall. In the Editors measured for this change,delayCalldid not run at all while the Editor window was unfocused, so that work waited until someone focused Unity.updateandtick.updatekeeps running while the Editor is unfocused, and the dispatcher also signals a tick when work is queued.Closes #3168
User Impact
Before: with Unity in the background, which is the normal case when an agent drives it, the following steps waited for the Unity window to be focused:
This was observed directly only for the setup wizard, whose pending show ran right after the first focus (see the measurements below). For the other steps it follows from the same mechanism: they could not be triggered in a real Editor here.
After: they run on the next
updateortick, andupdatekeeps running while unfocused.The startup server recovery and the skill file sync after a tool toggle also used
delayCall, but mattered less in practice:They are moved too, so no code in the Editor package waits on
delayCall, which has also been reported to stop after the cold-start compiler-errors dialog.The six places
UnityCliLoopServerControllerService.InitializeForEditorStartup)InitializeOnLoad)delayCall +=MainThreadSwitcher.AddContinuationOnServerLoopUnexpectedlyExited)delayCall +=plus a manualSignalTick()AddContinuation(it signals a tick itself)UnityCliLoopEditorSettingsRecoveryScheduler)InitializeOnLoad)delayCall +=AddContinuationSetupWizardStartupFlow.EvaluateVersionChange)updatehandler)delayCall +=CompileLifecycleRecoveryCoordinator)delayCall +=AddContinuationUnityCliLoopSettingsWindow.HandleToolToggled)delayCall +=AddContinuationupdateandtick. That is fine: the dispatcher's queue keeps what was added beforeInitialize(), and the firstupdateortickafter it drains the queue.awaitbefore the show resumes on Unity's synchronization context (none usesConfigureAwait(false)), and its only caller is a self-removingupdatehandler, so the show already runs on the main thread.delayCallonly added a hop.UloopPausePointRawCaptureLifecyclein the Runtime assembly keeps itsdelayCall. The Runtime assembly cannot use the Editor dispatcher. That code also relies ondelayCallrunning after the rest of the tick: a Step can unpause and re-pause for a moment, and the check reads the pause state after it settles. Moving it toupdatechanges that timing in a way EditMode tests cannot tell apart, so it gets a separate issue.Measurements in a real Editor
The probe registers three callbacks, and each writes a timestamp file:
delayCallupdatehandlerMainThreadSwitcher.SwitchToMainThreadfrom a thread-pool task)Times are UTC on 2026-10-06. Platform: macOS, Unity 2022.3.62f3.
Before the fix
The Editor from the investigation had not been focused since it was launched about 10.5 hours earlier. No code was changed for this run.
updateand dispatcher files appeared within about 1 s.delayCallfile was still missing.delayCallat launch, and that show had never run.uloop statusreportedReady.Focus ends the stall
delayCallfile appeared. That was 152 ms after the Editor gained focus for the first time since launch: the tool's debug log has no focus event between the launch and 01:15:33.965. The focus did not come from the test.updateand dispatcher files appeared within about 1 s, and nodelayCallfile appeared within 12 s.After the fix
updateand dispatcher files appeared within about 1 s, and thedelayCallfile was missing after 12 s. The changed places no longer depend ondelayCall.uloop launch -r(cold start, no compiler-errors dialog): Ready in 14 s, anduloop statusreportedReady. The debug log shows no focus event after the restart, so the launch did not focus the window.updateand dispatcher files appeared within about 1 s, and nodelayCallfile appeared within 15 s. The stall reproduces in a new process, without the compiler-errors dialog.Open question, not resolved here
The issue asks what stops
delayCallbesides the cold-start compiler-errors dialog. In both processes measured here, the condition was that the Editor window did not have focus:delayCallwaited while unfocused and flushed right after focus.Changes
ToolContractsnow exposes its internals to Infrastructure, Presentation, and the Compile tool, becauseMainThreadSwitcher.AddContinuationis internal. No asmdef reference changed.scheduleDelayCallis renamed toscheduleOnEditorTick.delayCallas the mechanism now name the dispatcher. Comments that explain whydelayCallis avoided are kept.UnityCliLoopEditorSettingsRecoverySchedulerTests: the recovery is queued on the dispatcher, not run inline, and runs once when the queue drains.delayCall, instead of requiring it to.delayCallCountcounted readiness retry waits, notdelayCallregistrations, and is renamedreadinessRetryWaitCount.SignalTicklines by line number. Changing it here would make this fix cut a dispatcher release.Verification
Unity EditMode (local Editor, filtered to the changed classes)
UnityCliLoopEditorSettingsRecoverySchedulerTests: Red first (Expected: 1 / But was: 0after draining the queue), then 1/1.UnityCliLoopServerControllerRecoveryTests: 23/23.CompileLifecycleRecoveryCoordinatorWatchdogTests: 5/5.SetupWizardStartupFlowVersionChangeTests|OnionAssemblyDependencyTests: Red first with exactly 3 failures (the two new tests and the source pin) and 95 passing, including the existing "without showing" tests. Then 98/98.delayCallback in both the settings recovery and the wizard fails exactly 4 of 99 tests (the scheduler test, the two new wizard tests, and the source pin). Reverted.uloop compile: 0 errors.Static checks (local)
uloop compile-check: 0 errors. Its 7 warnings are in unchanged test fixtures.EvaluateVersionChange, is at 13./.claude/, and this checkout is under one, so it ran on a copy ofPackages/src. It reports 1009 findings at threshold 5 there, so the run was not empty.CI (head 08f255c)