Repository navigation
Conversation
Move worker compiler invocation, response-file construction, references, quoting, cleanup, and .NET process policy out of the lifecycle host. Keep the mutable test override in the host through a narrow seam adapter.
📝 WalkthroughWalkthroughWorker assembly build logic (response-file writing, subprocess launch/timeout handling, reference-set construction, environment configuration, and quoting helpers) is extracted from ChangesWorker assembly build extraction
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Host as SharedRoslynCompilerWorkerHost
participant Builder as SharedRoslynCompilerWorkerAssemblyBuilder
participant Process as dotnet process
Host->>Builder: CompileWorkerAssembly(...)
Builder->>Builder: WriteWorkerCompilerResponseFile(...)
Builder->>Process: Start dotnet with response file
Builder->>Process: ConfigureWorkerDotnetRuntimeEnvironment(startInfo)
Process-->>Builder: stdout/stderr, exit code (or timeout kill)
Builder-->>Host: WorkerAssemblyBuildResult
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
You’re at about 92% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerAssemblyBuilder.cs (1)
105-124: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffOptional: bound the post-kill stream drain.
On the timeout path you
Kill()thenWaitForExit(500), but the subsequentTask.WaitAll(stdoutTask, stderrTask)(Line 113) has no timeout. If the redirected pipes stay open (e.g., kill didn't fully tear down thedotnet execprocess within 500ms), this blocks the compilation thread indefinitely while holdingSharedCompilerWorkerLock. Consider a boundedTask.WaitAll(new[]{stdoutTask, stderrTask}, someTimeout)so the timeout failure always returns.Behavior is carried over from the host, so this is not a regression—just a resilience hardening opportunity while the logic is being relocated.
🤖 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/DynamicCompilation/SharedRoslynCompilerWorkerAssemblyBuilder.cs` around lines 105 - 124, The timeout handling in SharedRoslynCompilerWorkerAssemblyBuilder still blocks indefinitely on Task.WaitAll(stdoutTask, stderrTask) after Kill() and WaitForExit(500), which can hold SharedCompilerWorkerLock forever if the pipes never close. Update the timeout path in the worker process wait logic to use a bounded wait for the stream-drain tasks, and if that wait expires, return the existing worker_compiler_timeout StartFailure instead of waiting unboundedly. Keep the fix localized around the process.WaitForExit timeout branch and the stdoutTask/stderrTask handling.
🤖 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/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerAssemblyBuilder.cs`:
- Around line 105-124: The timeout handling in
SharedRoslynCompilerWorkerAssemblyBuilder still blocks indefinitely on
Task.WaitAll(stdoutTask, stderrTask) after Kill() and WaitForExit(500), which
can hold SharedCompilerWorkerLock forever if the pipes never close. Update the
timeout path in the worker process wait logic to use a bounded wait for the
stream-drain tasks, and if that wait expires, return the existing
worker_compiler_timeout StartFailure instead of waiting unboundedly. Keep the
fix localized around the process.WaitForExit timeout branch and the
stdoutTask/stderrTask handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 777d8811-b210-495f-93be-94d7cae5d96a
⛔ Files ignored due to path filters (1)
Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerAssemblyBuilder.cs.metais excluded by none and included by none
📒 Files selected for processing (3)
Assets/Tests/Editor/DynamicCodeToolTests/SharedRoslynCompilerWorkerHostTests.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerAssemblyBuilder.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerHost.cs
1 participant
Summary
User Impact
Changes
SharedRoslynCompilerWorkerAssemblyBuilderto own compiler invocation, reference selection, response-file construction, quoting, build timeout/kill behavior, compiler output parsing, and stale assembly deletion..NETinvocation environment policy with the builder; both worker startup and compiler startup still setDOTNET_MULTILEVEL_LOOKUP=0.s_compileWorkerAssemblyForTestsand its swap API in the host as mutable lifecycle test state.CompileWorkerAssemblyinto a host seam adapter and the builder implementation: test overrides are still evaluated first, while production delegates with the same four arguments.WorkerPaths, temporary-directory mutation, retry/lifecycle state, protocol handling, and shutdown in the host.SharedRoslynCompilerWorkerHostfrom 865 to 677 lines.Verification
dist/darwin-arm64/uloop compile --project-path "$(git rev-parse --show-toplevel)"(0 errors, 0 warnings)SharedRoslynCompilerWorkerHostTests(10 passed)ExternalCompilerPathResolverTests(15 passed, including worker-build fallback through the host seam)StaticFacadeStateGuardTests(20 passed)