Skip to content

fix: Shared compiler restarts no longer leak process handles - #1630

Merged
hatayama merged 1 commit into
v3-betafrom
fix/hatayama/dispose-stale-shared-roslyn-worker-process
Jul 8, 2026
Merged

fix: Shared compiler restarts no longer leak process handles#1630
hatayama merged 1 commit into
v3-betafrom
fix/hatayama/dispose-stale-shared-roslyn-worker-process

Conversation

@hatayama

@hatayama hatayama commented Jul 8, 2026

Copy link
Copy Markdown
Owner

Summary

  • Shared compiler worker restarts now release the previous process handle before starting a replacement.
  • A failed replacement start no longer leaves the stale process object cached.

User Impact

  • Previously, an idle shared compiler worker that exited before the next compile could leave its operating-system process handle undisposed when the worker restarted.
  • Repeated recovery now releases each stale handle before acquiring the next worker process.

Changes

  • Dispose the cached worker process and clear session ownership before invoking the replacement process starter.
  • Add an injectable process starter that follows the session's existing test-seam pattern.
  • Add a process-free regression test that observes the previous Process object's synchronous Disposed event without waits or background work.
  • Keep the shared compiler wire format and protocol version unchanged.

Verification

  • Confirmed the regression test fails before the ownership fix with Expected: True / But was: False for the disposal observation.
  • dist/darwin-arm64/uloop compile (0 errors, 0 warnings)
  • SharedRoslynCompilerWorkerHostTests.StartProcessLocked_WhenReplacingCachedProcess_ShouldDisposePreviousHandle (1 passed)
  • SharedRoslynCompilerWorkerHostTests and StaticFacadeStateGuardTests (36 passed)
  • git diff --check

Review in cubic

Release the previously cached Process before starting its replacement so an
idle worker exit cannot leak an operating-system handle. Add an injectable
process starter and a process-free regression test that observes disposal
without introducing Unity Test Runner blocking or background work.
@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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: 06068ad0-8a32-41cd-9884-375c75ab927c

📥 Commits

Reviewing files that changed from the base of the PR and between 764021c and a688326.

📒 Files selected for processing (2)
  • Assets/Tests/Editor/DynamicCodeToolTests/SharedRoslynCompilerWorkerHostTests.cs
  • Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerSession.cs

📝 Walkthrough

Walkthrough

SharedRoslynCompilerWorkerSession now starts processes via an injectable delegate and disposes any previously cached process handle before replacing it. A test-only hook swaps the delegate for testing, and a new test verifies the previous process is disposed when replaced.

Changes

Stale process disposal

Layer / File(s) Summary
Injectable process starter and disposal logic
Packages/src/Editor/.../SharedRoslynCompilerWorkerSession.cs
Adds _startProcess delegate field defaulting to ProcessStartHelper.TryStart, updates StartProcessLocked to dispose the previously stored process before starting a new one via the delegate, and adds SwapProcessStarterForTests to swap the delegate for testing.
Test for previous process disposal
Assets/Tests/Editor/DynamicCodeToolTests/SharedRoslynCompilerWorkerHostTests.cs
Adds a test that configures the test starter to return a cached process then null, asserting the first start succeeds, the second fails, and the previous process is disposed.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: disposing leaked process handles during shared compiler restarts.
Description check ✅ Passed The description is directly related to the changeset and accurately explains the bug fix, test, and verification steps.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 fix/hatayama/dispose-stale-shared-roslyn-worker-process

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

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

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

You’re at about 94% 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.

Re-trigger cubic

@hatayama
hatayama merged commit 8f11be5 into v3-beta Jul 8, 2026
10 checks passed
@hatayama
hatayama deleted the fix/hatayama/dispose-stale-shared-roslyn-worker-process branch July 8, 2026 18:08
@github-actions github-actions Bot mentioned this pull request Jul 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant