fix: Compile commands no longer hang across Unity reloads#1240
Merged
hatayama merged 8 commits intoMay 30, 2026
Conversation
Persist pending compile request IDs in SessionState so reload recovery can write an indeterminate result file when the live compile callback disappears before persistence. Bound accepted compile RPC response waits so the CLI falls back to result-file polling instead of waiting on a stuck connection, and regenerate the native CLI binaries.
Expire pending compile SessionState after the CLI wait window and clear it when caller cancellation happens before Domain Reload starts, so later reload recovery cannot create stale compile results.
Use real elapsed time for post-reload compile recovery and keep accepted compile wait requests running after CLI response timeouts so Unity can persist the result file.
Keep the existing accepted-RPC final response budget for long compile requests while still allowing accepted timeout errors to fall back to result-file polling. Remove the compile-only response timeout path so large clean imports and forced recompiles do not lose their result before Unity finishes.
Register pending compile recovery from the existing first-party startup path and extend the pending compile lifetime so active long-running accepted compiles do not expire before the CLI wait budget ends.
Match the accepted-request disconnect policy to the camelCase parameter contract used by tool deserialization so explicit no-reload compile requests still cancel when the client disconnects.
Contributor
📝 Walkthrough(Note: hidden review stack artifact above) WalkthroughPersist pending compile-request metadata across domain reload, recover indeterminate compile results after reload when needed, and make early-response client-disconnect cancellation selective (compile requests that wait for domain reload stay alive). Added safe result-publishing, compile-use-case integration, recovery startup retry, CLI wait handling, and tests. ChangesCompile Domain Reload Recovery
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 4
🤖 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/Domain/UnityCliLoopEditorSessionStateService.cs`:
- Around line 186-190: The code in UnityCliLoopEditorSessionStateService
currently uses Convert.ToInt64 on the value returned by
_sessionStatePort.GetPendingCompileExpiresAtUtcTicks which will throw on
malformed data; change this to use long.TryParse to parse expiresAtUtcTicksText
into expiresAtUtcTicks (defaulting to 0 or treating it as missing/expired when
TryParse fails) and ensure UnityCliLoopPendingCompileRequest.Create is called
with the safe parsed value (or skip/clear the pending request) so startup
recovery treats invalid persisted ticks as expired instead of throwing.
In
`@Packages/src/Editor/FirstPartyTools/Compile/CompileResultPersistenceService.cs`:
- Around line 60-62: The current implementation in
CompileResultPersistenceService serializes the response and writes directly to
the polled path (CreateResultFilePath), which can be observed mid-write;
instead, write the JSON to a temp file in the same directory (e.g., append a
unique suffix or .tmp with requestId), fully flush and close the temp file to
ensure data is on disk, then atomically replace/rename the temp file into the
final path using an atomic operation (File.Replace or File.Move with overwrite)
so the reader never sees a partially-written JSON; update the code around
JsonConvert.SerializeObject/WriteAllText and CreateResultFilePath to implement
this pattern.
- Around line 73-80: The CreateResultFilePath method silently normalizes request
IDs by using Path.GetFileName and only using Debug.Assert; instead validate at
runtime and reject any requestId containing path separators to avoid collisions
(e.g., "foo/bar" becoming "bar"). In CreateResultFilePath, check whether
Path.GetFileName(requestId) equals requestId and if not throw an appropriate
runtime exception (ArgumentException or similar) with a clear message; ensure
callers like CompileTool.ToRequest (which forwards parameters.RequestId) will
receive the runtime failure rather than allowing silent normalization.
In `@Packages/src/Editor/Infrastructure/Api/JsonRpcProcessor.cs`:
- Around line 229-246: The helper CompileRequestWaitsForDomainReload currently
returns true for absent/invalid params causing requests to stay alive; change
its default behavior to return false when paramsToken is not a JObject, when
WaitForDomainReloadParamName is missing, or when the token is non-boolean so
that ShouldCancelAcceptedRequestOnClientDisconnect will cancel on client
disconnect by default; update the boolean branch to return the actual token
value (true only when the token is a boolean true) and return false in all other
early-exit cases (refer to CompileRequestWaitsForDomainReload and
WaitForDomainReloadParamName, and the caller
ShouldCancelAcceptedRequestOnClientDisconnect).
🪄 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: 78e50347-0865-4bd0-be30-18f8a8e18232
⛔ Files ignored due to path filters (7)
Assets/Tests/Editor/PendingCompileResultRecoveryServiceTests.cs.metais excluded by none and included by nonePackages/src/Cli~/dist/darwin-amd64/uloopis excluded by!**/dist/**and included by nonePackages/src/Cli~/dist/darwin-arm64/uloopis excluded by!**/dist/**and included by nonePackages/src/Cli~/dist/windows-amd64/uloop.exeis excluded by!**/dist/**,!**/*.exeand included by nonePackages/src/Editor/FirstPartyTools/Compile/CompileDomainReloadRecoveryStartup.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Compile/PendingCompileResultRecoveryService.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Compile/UnityCLILoop.FirstPartyTools.Compile.Editor.asmdefis excluded by none and included by none
📒 Files selected for processing (17)
Assets/Tests/Editor/JsonRpcProcessorCliVersionGateTests.csAssets/Tests/Editor/PendingCompileResultRecoveryServiceTests.csAssets/Tests/Editor/UnityCliLoopEditorSessionStateRepositoryTests.csAssets/Tests/Editor/UnityCliLoopEditorSessionStateTestFactory.csPackages/src/Cli~/internal/cli/compile_wait.goPackages/src/Cli~/internal/cli/compile_wait_test.goPackages/src/Editor/Domain/UnityCliLoopEditorSessionStateService.csPackages/src/Editor/FirstPartyTools/Compile/CompileDomainReloadRecoveryStartup.csPackages/src/Editor/FirstPartyTools/Compile/CompileResultPersistenceService.csPackages/src/Editor/FirstPartyTools/Compile/CompileTool.csPackages/src/Editor/FirstPartyTools/Compile/CompileUseCase.csPackages/src/Editor/FirstPartyTools/Compile/PendingCompileResultRecoveryService.csPackages/src/Editor/FirstPartyTools/FirstPartyToolsEditorStartup.csPackages/src/Editor/Infrastructure/Api/JsonRpcProcessor.csPackages/src/Editor/Infrastructure/Settings/UnityCliLoopEditorSessionStateRepository.csPackages/src/Editor/Infrastructure/UnityCliLoopBridgeServer.csPackages/src/Editor/ToolContracts/UnityCliLoopConstants.cs
There was a problem hiding this comment.
2 issues found
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Persist compile results through a temp-file publish path and clear stale pending compile recovery when SessionState data is malformed or execution stops before reload. Also keep recovered indeterminate counts unknown and codify the compile reload-wait default.
There was a problem hiding this comment.
2 issues found across 9 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Centralize the compile request ID character contract so wait setup and result persistence cannot drift apart. Clarify that pending recovery cleanup depends on result persistence completion, not compile execution completion.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Packages/src/Editor/FirstPartyTools/Compile/CompileRequestIdRules.cs (1)
8-24: 💤 Low valueConsider adding Debug.Assert for the null precondition.
The current caller guards against null/empty before invoking
IsSafe, but adding the assert documents the contract explicitly and catches misuse from future callers.🛡️ Proposed fix to add precondition assert
internal static bool IsSafe(string requestId) { + Debug.Assert(requestId != null, "requestId must not be null"); + foreach (char character in requestId)Based on learnings: "use Debug.Assert to document and enforce programmer preconditions (fail-fast during development). Treat null/invalid inputs caused by incorrect usage as programmer errors."
🤖 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/Compile/CompileRequestIdRules.cs` around lines 8 - 24, Add a Debug.Assert at the start of CompileRequestIdRules.IsSafe to document and enforce the non-null/non-empty precondition: assert that requestId is not null (and optionally not empty) so future incorrect callers fail fast in debug builds; keep the existing loop logic and return behavior unchanged and only add the assertion statement referencing the IsSafe parameter.
🤖 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.
Nitpick comments:
In `@Packages/src/Editor/FirstPartyTools/Compile/CompileRequestIdRules.cs`:
- Around line 8-24: Add a Debug.Assert at the start of
CompileRequestIdRules.IsSafe to document and enforce the non-null/non-empty
precondition: assert that requestId is not null (and optionally not empty) so
future incorrect callers fail fast in debug builds; keep the existing loop logic
and return behavior unchanged and only add the assertion statement referencing
the IsSafe parameter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eee256b1-c69b-4b78-862a-9596e725c0c1
⛔ Files ignored due to path filters (1)
Packages/src/Editor/FirstPartyTools/Compile/CompileRequestIdRules.cs.metais excluded by none and included by none
📒 Files selected for processing (5)
Assets/Tests/Editor/CompileResultPersistenceServiceTests.csAssets/Tests/Editor/PendingCompileResultRecoveryServiceTests.csPackages/src/Editor/FirstPartyTools/Compile/CompileRequestIdRules.csPackages/src/Editor/FirstPartyTools/Compile/CompileResultPersistenceService.csPackages/src/Editor/FirstPartyTools/Compile/CompileUseCase.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- Packages/src/Editor/FirstPartyTools/Compile/CompileResultPersistenceService.cs
- Assets/Tests/Editor/CompileResultPersistenceServiceTests.cs
- Assets/Tests/Editor/PendingCompileResultRecoveryServiceTests.cs
Merged
This was referenced Jul 6, 2026
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
User Impact
Changes
Verification