fix: shared Roslyn worker ReadLine no longer blocks Editor shutdown

[hatayama]

Summary

• Domain reload / Editor quit can interrupt a stuck shared Roslyn worker read without waiting up to 30s on the sync lock
• Dynamic-code compile conversations stay single-flight (write→read), while timeout paths kill the worker and observe abandoned reads

User Impact

• Before: A blocked worker ReadLine held the session lock, so shutdown (reload/quit) could stall the Editor for the full response timeout
• After: Shutdown kills the worker under a short state lock and does not wait on the compile gate; in-flight readers fail fast when pipes close

Lock boundary (before / after)

BEFORE
┌─────────────────────────────────────────────────────────┐
│ ExecuteLocked(_syncRoot)                                │
│   write request                                         │
│   ReadLine()  ← blocks up to 30s WHILE HOLDING LOCK     │
│   read diagnostics                                      │
└─────────────────────────────────────────────────────────┘
ShutdownProcessLocked ALSO requires _syncRoot
→ shutdown waits behind the stuck ReadLine

AFTER
┌─ SemaphoreSlim compile gate (async, serializes conversations)
│    short _syncRoot: write + take StreamReader
│    await ReadLineAsync  ← OUTSIDE _syncRoot
│    short _syncRoot only if kill/retry needed
└─────────────────────────────────────────────────────────
Shutdown: lock(_syncRoot) only → kill/close pipes
          does NOT WaitAsync on the compile gate
→ stuck read unblocks via pipe close; abandoned Task observed

Changes

• Extract Unity-free SharedRoslynCompilerWorkerLineReader (timeout/cancel + observe abandoned ReadLine)
• Extract SharedRoslynCompilerWorkerSessionCoordination (async compile gate + short state lock + shutdown bypass)
• TryCompile → TryCompileAsync; host kills worker on empty/invalid/timeout responses
• Pure unit tests for line reader + “shutdown while compile gate held”
• Wire tests into EditMode / compile-check CI workflows

Out of scope (umbrella note)

• SharedRoslynCompilerWorkerAssemblyBuilder still uses sync WaitForExit for worker DLL builds; call sites can run on the Unity main thread during compile/prewarm

Verification

• dotnet test tests/SharedRoslynCompilerWorker.UnitTests/... (8 passed)
• Unity Bee compiled ExecuteDynamicCode.Editor with no CS errors for these files
• Local uloop compile hung after domain reload in this environment (IPC warmup hit disposed DynamicCodeExecutionScheduler); CI compile check is the authoritative follow-up

Test plan

• CI: SharedRoslynCompilerWorker unit tests green
• CI / local: EditMode SharedRoslynCompilerWorkerHostTests still pass
• Manual: trigger domain reload during a slow dynamic-code compile; Editor should not stick on worker ReadLine

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: e41df71d-7a0a-4d03-87c5-61a71fe90e47

📥 Commits

Reviewing files that changed from the base of the PR and between 10d0884 and 7f05782.

📒 Files selected for processing (1)

• Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerHost.cs

🚧 Files skipped from review as they are similar to previous changes (1)

• Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerHost.cs

---

📝 Walkthrough

Walkthrough

The shared Roslyn compiler worker now uses asynchronous compilation and protocol reading, separate state locking and compile serialization, coordinated shutdown handling, expanded unit tests, and CI execution of the shared worker test project.

Changes

Async Roslyn worker

Layer / File(s)	Summary
Session coordination and lifecycle Packages/src/.../SharedRoslynCompilerWorkerSessionCoordination.cs, Packages/src/.../SharedRoslynCompilerWorkerSession.cs	Compilation conversations are serialized with an async gate, while process and directory state uses short state-locked operations; shutdown bypasses the compile gate.
Asynchronous worker protocol Packages/src/.../SharedRoslynCompilerWorkerLineReader.cs, Packages/src/.../SharedRoslynCompilerWorkerProtocol.cs	Protocol and diagnostic reads use timeout- and cancellation-aware asynchronous line-reading helpers.
Async host compilation flow Packages/src/.../RoslynCompilerBackend.cs, Packages/src/.../SharedRoslynCompilerWorkerHost.cs	Compilation, worker invocation, response handling, cancellation shutdown, and protocol failure handling are converted to asynchronous control flow.
Concurrency validation and CI coverage tests/SharedRoslynCompilerWorker.UnitTests/SharedRoslynCompilerWorkerLineReaderTests.cs, .github/workflows/*	Tests cover line reading, cancellation, shutdown coordination, and single-flight compilation; both workflows run the shared worker tests.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant RoslynCompilerBackend
  participant SharedRoslynCompilerWorkerHost
  participant SharedRoslynCompilerWorkerSessionCoordination
  participant SharedRoslynCompilerWorkerLineReader

  RoslynCompilerBackend->>SharedRoslynCompilerWorkerHost: TryCompileAsync
  SharedRoslynCompilerWorkerHost->>SharedRoslynCompilerWorkerSessionCoordination: Serialize compilation
  SharedRoslynCompilerWorkerHost->>SharedRoslynCompilerWorkerLineReader: Read protocol response asynchronously
  SharedRoslynCompilerWorkerLineReader-->>SharedRoslynCompilerWorkerHost: Diagnostics and compiler messages
  SharedRoslynCompilerWorkerHost-->>RoslynCompilerBackend: CompilerMessage[]

Loading

Possibly related PRs

• hatayama/unity-cli-loop#901: Refactors the same shared Roslyn worker host and protocol components.
• hatayama/unity-cli-loop#1629: Introduces related shared Roslyn worker lifecycle ownership used by this async coordination work.
• hatayama/unity-cli-loop#1630: Modifies the same session process lifecycle and restart path.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 19.44% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: preventing shared Roslyn worker ReadLine from blocking Editor shutdown.
Description check	✅ Passed	The description is detailed and directly matches the changeset, covering the async worker, shutdown behavior, tests, and CI updates.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/roslyn-worker-readline-hardening

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (3)

Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerHost.cs (1)

344-395: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider extracting the repeated force-shutdown call.

ServiceValue.ExecuteWithStateLock(ServiceValue.ShutdownProcessLocked) appears in several failure paths (empty header, malformed header, missing end marker, retry loop, cancellation). A small private helper (e.g., ForceShutdownWorker()) would centralize the intent and reduce duplication.

🤖 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/SharedRoslynCompilerWorkerHost.cs`
around lines 344 - 395, Extract the repeated
ServiceValue.ExecuteWithStateLock(ServiceValue.ShutdownProcessLocked) operation
into a private helper such as ForceShutdownWorker(). Replace the direct calls in
ReadWorkerResponseAsync and the other retry, cancellation, and failure paths
with this helper, preserving the existing shutdown behavior.

tests/SharedRoslynCompilerWorker.UnitTests/SharedRoslynCompilerWorkerLineReaderTests.cs (2)

89-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider adding a timeout/cancellation test for ReadDiagnosticLinesAsync.

Only the happy path (end marker arrives) is tested. The upstream contract returns null when ReadLineAsync times out mid-aggregation, and throws OperationCanceledException on cancellation — neither path is covered.

🤖 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
`@tests/SharedRoslynCompilerWorker.UnitTests/SharedRoslynCompilerWorkerLineReaderTests.cs`
around lines 89 - 102, Add tests for
SharedRoslynCompilerWorkerLineReader.ReadDiagnosticLinesAsync covering both
failure paths: verify it returns null when ReadLineAsync times out during
diagnostic aggregation, and verify it propagates OperationCanceledException when
cancellation is requested. Keep the existing end-marker test unchanged and use
short, deterministic timeout/cancellation setup.

---

52-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider making this test async Task and awaiting Assert.ThrowsAsync.

The test is void and Assert.ThrowsAsync is not awaited. It works here because the pre-canceled token causes ThrowIfCancellationRequested to throw synchronously, but it's inconsistent with the other async tests and fragile if the upstream contract changes to an async path.

♻️ Proposed refactor

-        public void ReadLineAsync_WhenCanceled_ShouldThrowOperationCanceledException()
+        public async Task ReadLineAsync_WhenCanceled_ShouldThrowOperationCanceledException()
         {
             // Verifies cooperative cancel surfaces as OperationCanceledException before ReadLine starts.
             using CancellationTokenSource cancellationTokenSource = new();
             cancellationTokenSource.Cancel();
             using StringReader reader = new(string.Empty);
 
-            Assert.ThrowsAsync<OperationCanceledException>(
+            await Assert.ThrowsAsync<OperationCanceledException>(
                 async () => await SharedRoslynCompilerWorkerLineReader.ReadLineAsync(
                     reader,
                     cancellationTokenSource.Token,
                     timeoutMilliseconds: 1000));
         }

🤖 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
`@tests/SharedRoslynCompilerWorker.UnitTests/SharedRoslynCompilerWorkerLineReaderTests.cs`
around lines 52 - 65, Update
ReadLineAsync_WhenCanceled_ShouldThrowOperationCanceledException to return Task
and await Assert.ThrowsAsync, preserving the pre-canceled token setup and
existing assertion behavior.

🤖 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/SharedRoslynCompilerWorkerHost.cs`:
- Around line 344-395: Extract the repeated
ServiceValue.ExecuteWithStateLock(ServiceValue.ShutdownProcessLocked) operation
into a private helper such as ForceShutdownWorker(). Replace the direct calls in
ReadWorkerResponseAsync and the other retry, cancellation, and failure paths
with this helper, preserving the existing shutdown behavior.

In
`@tests/SharedRoslynCompilerWorker.UnitTests/SharedRoslynCompilerWorkerLineReaderTests.cs`:
- Around line 89-102: Add tests for
SharedRoslynCompilerWorkerLineReader.ReadDiagnosticLinesAsync covering both
failure paths: verify it returns null when ReadLineAsync times out during
diagnostic aggregation, and verify it propagates OperationCanceledException when
cancellation is requested. Keep the existing end-marker test unchanged and use
short, deterministic timeout/cancellation setup.
- Around line 52-65: Update
ReadLineAsync_WhenCanceled_ShouldThrowOperationCanceledException to return Task
and await Assert.ThrowsAsync, preserving the pre-canceled token setup and
existing assertion behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 234d03b9-b2a2-4939-a167-13eaa263ac86

📥 Commits

Reviewing files that changed from the base of the PR and between 26d48e9 and 10d0884.

⛔ Files ignored due to path filters (3)

• Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerLineReader.cs.meta is excluded by none and included by none
• Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerSessionCoordination.cs.meta is excluded by none and included by none
• tests/SharedRoslynCompilerWorker.UnitTests/SharedRoslynCompilerWorker.UnitTests.csproj is excluded by none and included by none

📒 Files selected for processing (9)

• .github/workflows/unity-compile-check-and-test-runner.yml
• .github/workflows/unity-editmode-tests.yml
• Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/RoslynCompilerBackend.cs
• Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerHost.cs
• Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerLineReader.cs
• Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerProtocol.cs
• Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerSession.cs
• Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerSessionCoordination.cs
• tests/SharedRoslynCompilerWorker.UnitTests/SharedRoslynCompilerWorkerLineReaderTests.cs

[hatayama]

Review (Fable) — conditional LGTM, 1 required fix

Full 1044-line diff reviewed against the approved Option A conditions, plus local verification (dotnet test tests/SharedRoslynCompilerWorker.UnitTests → 8/8 green) and an investigation of the local uloop compile hang report.

Design conditions — all satisfied

• Async compile gate (SemaphoreSlim(1,1), no GetResult) serializes worker conversations ✅
• RunShutdownWithoutCompileGate lets shutdown kill/close pipes without waiting on the gate ✅
• Monitor.IsEntered assert migrated to AssertStateLockHeld ✅
• Timeout → kill + abandoned-ReadLine fault observe ✅
• Pure unit tests cover shutdown-while-gate-held and post-shutdown compile ✅
• ObjectDisposedException from a shutdown-disposed reader is caught as retryable ✅

Required fix: compile request send lost its retry protection

Previously SendCompileRequest lived inside the try in InvokeWorkerOnce, so a broken stdin pipe (IOException from StandardInput.WriteLine/Flush when the worker died after the liveness check) became CreateRetryableWorkerCommunicationFailure. The new code moves the send into the prepare lambda before the try. An IOException there now escapes the retry loop AND the one-shot fallback, and RoslynCompilerBackend.CompileAsync has no catch — the raw exception surfaces to the tool. This is a regression from the old behavior.

Preferred shape: keep the prepare lock to liveness check + reader capture only, and perform the send inside the try via ExecuteWithStateLock(SendCompileRequestLocked) (this also restores the old incrementBuildCount/markBuildStarted ordering). Alternatively, wrap the prepare block with the same IOException/ObjectDisposedException → retryable conversion.

Umbrella notes to record (no code change required)

1. Post-shutdown worker resurrection: with the gate/shutdown split, a shutdown that interleaves with an in-flight compile can be followed by a retry that restarts the worker process. The worker self-exits on stdin EOF, so the orphan is self-limiting — record it; a shutdown flag making retries non-retryable is a possible future hardening.
2. In-flight request across server reset fails with ObjectDisposedException: see below.

Local uloop compile hang report — investigated, not a blocker

Reproduction attempt on this checkout (PR branch): uloop compile succeeded (0/0), no hang; the CLI printed warning: post-compile warmup skipped: ... server is not reachable ... i/o timeout and exited normally — hitting the post-reload server restart window is handled as a skip, not a hang. The only error left in the Unity log is a warmup execute-dynamic-code request that grabbed the runtime facade before ResetServerScopedServices and executed after it, hitting the disposed DynamicCodeExecutionScheduler → fail-fast JSON-RPC error, not a hang. _runtimeFacade is nulled immediately inside the reset lock, so new requests always get a fresh facade; the race window is pre-existing and was not widened by #1746 or this PR. If your environment saw the CLI process itself never return, please share that CLI output/process stack — that would change the assessment.

Will LGTM once the required fix lands and the umbrella notes are recorded.

[hatayama]

Addressed the required fix in 7f05782:

• Prepare lock now only checks liveness and takes the StreamReader
• SendCompileRequestLocked runs inside the try via ExecuteWithStateLock, so stdin IOException / ObjectDisposedException map back to CreateRetryableWorkerCommunicationFailure (retry / one-shot fallback)

Umbrella notes updated in /tmp/claude-shared/group3-umbrella-notes.md:

1. shutdown → retry may briefly resurrect a worker (stdin EOF self-exit, no lasting leak)
2. reset-crossing in-flight execute-dynamic-code ODE race (fail-fast; consider retryable "server restarting" later)

Also re-ran uloop compile here → 0 errors / 0 warnings (agrees with your hang investigation). Ready for LGTM.

[hatayama]

LGTM. Verified commit 7f05782: the compile request send is back inside the retryable try (ExecuteWithStateLock(() => SendCompileRequestLocked(...))), the prepare lock now only covers the liveness check and reader capture, and the incrementBuildCount/markBuildStarted ordering matches the pre-refactor behavior. Both umbrella notes (post-shutdown worker resurrection via retry; reset-crossing ObjectDisposedException race) are recorded. Re-ran dotnet test tests/SharedRoslynCompilerWorker.UnitTests at 7f05782 → 8/8 green. All approved design conditions for Option A are satisfied — good to merge into the umbrella branch after CI is green.