fix: Unity launch waits reliably during slow startup and restart

[hatayama]

Summary

• uloop launch now gives slow Unity startup, imports, and domain reloads a longer readiness window before failing.
• Restart and quit flows now wait for the previous Unity process to exit before reporting success or launching a replacement Editor.

User Impact

• Slow project startup no longer looks like a generic unreachable-package failure after the shorter shared readiness timeout.
• uloop launch -r and uloop launch --quit are less likely to race Windows file locks from a still-exiting Unity process.
• Timeout errors now distinguish slow startup, exited Editors, and process-exit waits so agents and users get clearer next steps.

Changes

• Added launch-specific readiness timeout handling and structured startup timeout output.
• Kept CLI/package protocol mismatch errors on the existing update-required path.
• Added bounded process-exit polling for restart and quit paths with structured retryable timeout errors.
• Covered the new launch readiness and process-exit cases with focused Go tests.

Verification

• go test ./internal/cli -run 'TestRunLaunch|TestWaitForLaunchReadiness|TestWaitForUnityProcessExit|TestWaitForUnityStartupMarker|TestWaitForToolReadiness|TestToolReadinessDoneError|TestClassify(LaunchStartupTimeout|LaunchProcessExitTimeout|UnityServerNotResponding|CliUpdateRequired)|TestExecuteDynamicCodeReadinessProbeParamsUseForegroundWarmup'
• scripts/check-go-cli.sh
• /mnt/c/Users/booql/oss/unity-cli-loop/cli/dist/windows-amd64/uloop.exe --project-path 'C:\Users\booql\oss\cli-loop-tetris' launch -r
• codex-review v3-beta

[coderabbitai]

📝 Walkthrough

Walkthrough

Introduces two typed timeout errors (launchStartupTimeoutError, launchProcessExitTimeoutError) with cliError builders and classification in classifyError. Refactors runLaunch to use phase-specific wait helpers (waitForUnityProcessExit, waitForUnityStartupMarkerOrTimeout, waitForLaunchReadiness). Adds cli_update_required fast-exit to waitForToolReadiness. Adds comprehensive tests for all new behaviors.

Changes

Unity Launch Timeout Errors and Readiness Refactor

Layer / File(s)	Summary
New timeout error types and CLI error builders cli/internal/cli/launch_startup_timeout_error.go, cli/internal/cli/launch_process_exit_timeout_error.go	Defines launchStartupTimeoutError and launchProcessExitTimeoutError structs with Error()/Unwrap() methods and their unityStartupTimeoutCLIError/unityProcessExitTimeoutCLIError builder functions producing retryable cliErrors with pid/timeoutSeconds details.
Error code constants and classifyError dispatch cli/internal/cli/error_envelope.go, cli/internal/cli/error_envelope_test.go	Adds UNITY_STARTUP_TIMEOUT and UNITY_PROCESS_EXIT_TIMEOUT error code constants and extends classifyError to detect both timeout types. Tests verify error codes, messages, retryability flags, and detail values.
Tool readiness: timeout delegation and cli_update_required fast-exit cli/internal/cli/tool_readiness.go, cli/internal/cli/tool_readiness_test.go	Refactors waitForToolReadiness to delegate to waitForToolReadinessWithTimeout, adds isReadinessCLIUpdateRequiredError to inspect unityipc.RPCError payloads, and short-circuits the poll loop on cli_update_required. Tests verify timeout deadline and immediate error surfacing.
runLaunch synchronization: phase-specific wait helpers cli/internal/cli/launch.go	Expands launch constants, introduces waitForUnityProcessExit poller and waitForUnityStartupMarkerOrTimeout, and updates runLaunch to use waitForLaunchReadiness for existing/fresh-start paths and waitForUnityProcessExitForLaunch in the restart path after killing the old PID.
Launch and readiness test coverage cli/internal/cli/launch_test.go	Updates existing tests to match new wait function signatures. Adds tests for waitForLaunchReadiness timeout/wrapping/preservation, TestRunLaunchQuitWaitsForKilledUnityProcess, TestRunLaunchRestartReportsProcessExitWaitFailure, startup-marker wait tests, and TestWaitForUnityProcessExitBoundsProcessScan.

Sequence Diagram(s)

sequenceDiagram
  participant runLaunch
  participant waitForUnityProcessExitForLaunch
  participant waitForUnityStartupMarkerForLaunch
  participant waitForLaunchReadiness
  participant waitForToolReadinessWithTimeout
  participant classifyError

  rect rgba(255, 200, 100, 0.5)
    Note over runLaunch: Restart path
    runLaunch->>waitForUnityProcessExitForLaunch: poll until PID disappears
    waitForUnityProcessExitForLaunch-->>runLaunch: ok or launchProcessExitTimeoutError
    runLaunch->>classifyError: on timeout error → errorCodeUnityProcessExitTimeout
  end

  rect rgba(100, 200, 255, 0.5)
    Note over runLaunch: Fresh start / existing process
    runLaunch->>waitForUnityStartupMarkerForLaunch: poll for lockfile
    runLaunch->>waitForLaunchReadiness: wait with launch timeout
    waitForLaunchReadiness->>waitForToolReadinessWithTimeout: delegate with timeout
    waitForToolReadinessWithTimeout-->>waitForLaunchReadiness: ok, cli_update_required (fast-exit), or timeout
    waitForLaunchReadiness-->>runLaunch: ok or launchStartupTimeoutError
    runLaunch->>classifyError: on timeout error → errorCodeUnityStartupTimeout
  end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

• hatayama/unity-cli-loop#1027: Modifies Unity launch readiness/wait synchronization and lock-based startup detection, directly overlapping with this PR's startup-marker and process-exit wait refactors in launch.go.
• hatayama/unity-cli-loop#1321: Both PRs modify tool_readiness.go's waitForToolReadiness polling logic and readiness-condition handling.
• hatayama/unity-cli-loop#1296: Changes the Unity-side server controller readiness timing, directly affecting the same startup/readiness probe path that this PR classifies with new timeout error types.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 13.51% 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 and concisely summarizes the main changes: improving reliability of Unity launch operations during slow startup and restart scenarios with proper timeout handling.
Description check	✅ Passed	The description comprehensively covers the changeset, explaining user impact, implementation details, and verification steps for the launch reliability improvements.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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 codex/fix-launch-restart-wait

---

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 and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

cli/internal/cli/launch_test.go (1)

428-430: ⚡ Quick win

Assert kill invocation in quit/restart exit-wait tests.

Line 428 and Line 478 stub killUnityProcessForLaunch but never assert it was called. Add killedPid checks so these tests fail if kill is accidentally skipped.

Suggested patch

 func TestRunLaunchQuitWaitsForKilledUnityProcess(t *testing.T) {
@@
-	waitedPid := 0
+	killedPid := 0
+	waitedPid := 0
@@
 	killUnityProcessForLaunch = func(pid int) error {
+		killedPid = pid
 		return nil
 	}
@@
 	if code != 0 {
 		t.Fatalf("exit code mismatch: %d stderr=%s", code, stderr.String())
 	}
+	if killedPid != 333 {
+		t.Fatalf("quit killed pid mismatch: %d", killedPid)
+	}
 	if waitedPid != 333 {
 		t.Fatalf("quit waited pid mismatch: %d", waitedPid)
 	}
@@
 func TestRunLaunchRestartReportsProcessExitWaitFailure(t *testing.T) {
@@
 	resolverCalled := false
+	killedPid := 0
@@
 	killUnityProcessForLaunch = func(pid int) error {
+		killedPid = pid
 		return nil
 	}
@@
 	if resolverCalled {
 		t.Fatal("restart should not launch a new Unity process before the old one exits")
 	}
+	if killedPid != 444 {
+		t.Fatalf("restart killed pid mismatch: %d", killedPid)
+	}
 	if !strings.Contains(stderr.String(), "still exiting") {
 		t.Fatalf("stderr should include wait failure: %s", stderr.String())
 	}
 }

Also applies to: 478-480

🤖 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/internal/cli/launch_test.go` around lines 428 - 430, The tests at
cli/internal/cli/launch_test.go lines 428-430 and lines 478-480 stub the
killUnityProcessForLaunch function but do not verify it was actually called. At
each location, add a killedPid variable to capture the pid argument when the
stub is invoked, then add assertions after the test execution to verify that
killedPid contains the expected process ID, ensuring the test fails if the kill
invocation is accidentally skipped.

🤖 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 `@cli/internal/cli/launch_test.go`:
- Around line 428-430: The tests at cli/internal/cli/launch_test.go lines
428-430 and lines 478-480 stub the killUnityProcessForLaunch function but do not
verify it was actually called. At each location, add a killedPid variable to
capture the pid argument when the stub is invoked, then add assertions after the
test execution to verify that killedPid contains the expected process ID,
ensuring the test fails if the kill invocation is accidentally skipped.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0d956882-1d15-4e55-b450-0213f82c6c40

📥 Commits

Reviewing files that changed from the base of the PR and between 1ff3b6b and 955be9f.

📒 Files selected for processing (8)

• cli/internal/cli/error_envelope.go
• cli/internal/cli/error_envelope_test.go
• cli/internal/cli/launch.go
• cli/internal/cli/launch_process_exit_timeout_error.go
• cli/internal/cli/launch_startup_timeout_error.go
• cli/internal/cli/launch_test.go
• cli/internal/cli/tool_readiness.go
• cli/internal/cli/tool_readiness_test.go

[cubic-dev-ai]

No issues found across 8 files

_{Re-trigger cubic}