fix: Keep execute-dynamic-code waiting through self-induced main-thread stalls - #1940
Merged
hatayama merged 3 commits intoJul 22, 2026
Conversation
…alls Unity's heartbeat reports mainThreadStallSeconds as time since the last update tick, which climbs identically whether the editor is frozen or a command is legitimately blocking the main thread by design. A ~150-line execute-dynamic-code snippet triggered a false EditorUnresponsiveError: CLI-side VibeLogs showed the CLI giving up at ~300s while Unity's own logs showed the snippet actually finishing seconds later. execute-dynamic-code now gets a stall exemption that only applies while the reported stall could not predate this request's own accept (within a margin); a stall that already exceeds elapsed-since-accept still fails as a genuine freeze, so other commands and pre-existing freezes keep the original diagnosis unchanged.
execute_dynamic_code_start had no matching completion entry, so a client-side stall diagnosis could not be correlated against whether Unity had actually finished the snippet by the time the CLI gave up on it. This gap made a recent false-stall investigation harder than it needed to be.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change adds correlated completion logging for dynamic code execution and introduces Unity IPC handling for self-induced main-thread stalls. Project-runner retry clients enable this tolerance only for the execute-dynamic-code command. ChangesExecution observability and stall tolerance
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ProjectRunner
participant UnityIPCClient
participant Unity
ProjectRunner->>UnityIPCClient: Create retry client for execute-dynamic-code
UnityIPCClient->>Unity: Send RPC request
Unity-->>UnityIPCClient: Return accepted response
UnityIPCClient->>UnityIPCClient: Capture acceptedAt and evaluate heartbeat stall
UnityIPCClient-->>ProjectRunner: Continue waiting or return EditorUnresponsiveError
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/common/unityipc/client.go (1)
354-378: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport the classified stall state, not merely the enabled option.
A pre-existing stall with tolerance enabled emits “busy executing this command” before returning
EditorUnresponsiveError. ComputeisSelfInducedStallonce and pass that result toreportMainThreadStall, so genuine freezes retain the existing “stuck” message.Proposed fix
+ selfInducedStall := client.isSelfInducedStall(stallSeconds, acceptedAt) if stallSeconds >= mainThreadStallProgressThresholdSeconds { - client.reportMainThreadStall(stallSeconds, progress) + client.reportMainThreadStall(stallSeconds, selfInducedStall, progress) } - if stallSeconds >= client.getMainThreadStallLimit().Seconds() && !client.isSelfInducedStall(stallSeconds, acceptedAt) { + if stallSeconds >= client.getMainThreadStallLimit().Seconds() && !selfInducedStall { return &EditorUnresponsiveError{StallSeconds: stallSeconds} }🤖 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 `@cli/common/unityipc/client.go` around lines 354 - 378, Compute isSelfInducedStall once in the surrounding stall-handling flow, before reporting or returning the error. Pass that classified result to reportMainThreadStall instead of having the method inspect the selfInducedStallTolerant option, and update reportMainThreadStall to select the “busy executing” message only for self-induced stalls while retaining the existing “stuck” message for genuine freezes.
🤖 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/ExecuteDynamicCodeUseCase.cs`:
- Around line 70-71: Update ExecuteDynamicCodeUseCase so cancellation during
warmup, execution, or retry also emits exactly one completion event. Centralize
the LogExecutionComplete call or add an explicit cancelled outcome in the outer
cancellation handler, ensuring the existing finalResult path does not produce
duplicate logs.
---
Outside diff comments:
In `@cli/common/unityipc/client.go`:
- Around line 354-378: Compute isSelfInducedStall once in the surrounding
stall-handling flow, before reporting or returning the error. Pass that
classified result to reportMainThreadStall instead of having the method inspect
the selfInducedStallTolerant option, and update reportMainThreadStall to select
the “busy executing” message only for self-induced stalls while retaining the
existing “stuck” message for genuine freezes.
🪄 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: ccdc4ff9-af00-4495-9a9f-81c597b09606
📒 Files selected for processing (9)
Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/ExecuteDynamicCodeUseCase.cscli/common/unityipc/client.gocli/common/unityipc/client_heartbeat_test.gocli/common/unityipc/client_options.gocli/dispatcher/shared-inputs-stamp.jsoncli/project-runner/internal/projectrunner/connection_retry.gocli/project-runner/internal/projectrunner/connection_retry_flow.gocli/project-runner/internal/projectrunner/connection_retry_flow_test.gocli/project-runner/shared-inputs-stamp.json
Classify the stall as self-induced-or-not once and pass that result to reportMainThreadStall, instead of having it just check the tolerant option: a stall predating this request's accept is a genuine freeze even on a tolerant client, and the progress message must keep saying "stuck" for it instead of wrongly implying the command caused it. Also cover the cancellation path that throws before a final ExecutionResult exists (e.g. during foreground warmup), which the completion log added in the previous commit could not reach, while guarding against double-logging when a later await on the finalResult path throws after completion was already logged.
Owner
Author
|
Fixed both findings in c146b4b: (1) reportMainThreadStall now takes the classified self-induced-or-not result instead of re-deriving it from the option, so a stall predating accept keeps the "stuck" wording even on a tolerant client; (2) added a completion log for the cancellation path that throws before a final ExecutionResult exists, guarded by a completionLogged flag so the finalResult path's later awaits throwing OperationCanceledException don't double-log. |
hatayama
merged commit Jul 22, 2026
ac70353
into
feature/round8-pause-point-improvements
2 checks passed
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
execute-dynamic-codeno longer fails with a false "editor unresponsive" error while a long-running snippet is still legitimately executing.User Impact
execute-dynamic-codesnippet (observed with a ~150-line scene-construction script, ~6 minutes) could make the CLI reportunity editor main thread has been unresponsiveand suggest restarting the editor, even though Unity was alive and finished the snippet normally seconds later. The false report came from a heuristic that cannot tell "this command is busy running" apart from "the editor is frozen," because both look identical on the main-thread stall counter.execute-dynamic-codekeeps waiting through a stall that started no earlier than the request's own acceptance (i.e. it's this command keeping the main thread busy). A stall that already predates the request — meaning the editor was frozen before this command ever started — still fails with the same diagnosis as before, so a genuine freeze is still caught.Changes
cli/common/unityipc: addedWithSelfInducedMainThreadStallTolerance, which exempts a heartbeat stall fromEditorUnresponsiveErroronly whilestallSecondscannot exceed the time elapsed since this request's own accept (plus a margin for heartbeat/clock slack). The progress message while exempted is reworded to describe busyness instead of implying a freeze.cli/project-runner: applies the new option only for theexecute-dynamic-codecommand; every other command's stall handling is unchanged.Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode: added a completion VibeLog entry (execute_dynamic_code_complete) alongside the existing start entry, so a future stall report can be correlated against whether Unity actually finished — the gap that made this investigation harder than necessary.Verification
execute_dynamic_code_start, while the CLI's heartbeat-driven stall heuristic (cli/common/unityipc/client.go) gave up unconditionally at the fixed 300s limit.TestSendKeepsWaitingThroughSelfInducedMainThreadStall) reproducing the false failure against the pre-fix code, confirmed it failed withEditorUnresponsiveError, then implemented the fix to make it pass. Also addedTestSendFailsWhenMainThreadStallPredatesAcceptDespiteToleranceto confirm the exemption does not mask a stall that predates the request's accept.sh scripts/check-go-cli.sh: format/vet/lint 0 issues, all Go packages pass (including the new tests).go test -run TestProductionGoFilesStayFocused ./...(release-automation): pass.sh scripts/stamp-release-inputs.sh: re-run and committed, sincecli/commonchanged.dist/darwin-arm64/uloop compile: 0 errors / 0 warnings after the C# completion-log addition.