fix: Unity busy detection no longer relies on obsolete lock files#1144
Merged
Conversation
The CLI now uses server-state.json and readiness probes as the readiness contract, so the legacy Temp lock hints no longer need producers, cleanup paths, or startup lifecycle policy. - Keep uloop fix focused on stale server readiness state files. - Remove domain reload, compile, and server startup lock file handling from the Editor side. - Refresh generated skill copies and native CLI binaries after the contract change.
Contributor
📝 WalkthroughWalkthroughThis PR removes lock-file-based server recovery mechanisms across the codebase and simplifies server initialization by eliminating the ChangesLock-File Removal and Startup Flow Simplification
🎯 4 (Complex) | ⏱️ ~60 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 |
Keep the pull request focused on source files and Claude skill copies after the generated .agents snapshots were requested to stay out of the branch diff.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Packages/src/Cli~/internal/cli/fix_test.go (1)
35-68: ⚡ Quick winAdd coverage for
serverStateBackupSuffixcleanup path.
cleanupServerStateFilesalso removes the backup state artifact, but this test only asserts three files. Please seed/assert the backup file too so the cleanup contract is fully covered.Suggested test update
func TestCleanupStaleRecoveryStateRemovesServerStateFiles(t *testing.T) { @@ if err := os.WriteFile(statePath+serverStateInProgressTempSuffix, []byte(`{"phase":"recovering"}`), 0o644); err != nil { t.Fatalf("failed to seed in-progress temp state file: %v", err) } + if err := os.WriteFile(statePath+serverStateBackupSuffix, []byte(`{"phase":"backup"}`), 0o644); err != nil { + t.Fatalf("failed to seed backup state file: %v", err) + } @@ - if cleaned != 3 { + if cleaned != 4 { t.Fatalf("cleaned count mismatch: %d", cleaned) } @@ if _, err := os.Stat(statePath + serverStateInProgressTempSuffix); err == nil { t.Fatal("in-progress temporary server state file was not removed") } + if _, err := os.Stat(statePath + serverStateBackupSuffix); err == nil { + t.Fatal("backup server state file was not removed") + } }🤖 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/Cli`~/internal/cli/fix_test.go around lines 35 - 68, The test TestCleanupStaleRecoveryStateRemovesServerStateFiles seeds three server state files but omits the backup file that cleanupServerStateFiles should also remove; update the test to create a backup file at statePath+serverStateBackupSuffix before calling cleanupStaleRecoveryState, assert the returned cleaned count increments to 4 (or adjust expectation accordingly), and add an os.Stat check to ensure the backup file was removed after cleanup; reference TestCleanupStaleRecoveryStateRemovesServerStateFiles, cleanupStaleRecoveryState, cleanupServerStateFiles, and serverStateBackupSuffix to locate where to add the seed and assertion.
🤖 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/docs/ARCHITECTURE_Unity_ja.md`:
- Line 149: 図で契約を ExecuteAsync(CancellationToken):
Task~ServerInitializationResult~ に変更したので、本文（3.6節、該当箇所は約354行付近）に残っている
"request/result" の記述を同一契約に合わせて書き換えてください。具体的には、ExecuteAsync が CancellationToken
を受け取り ServerInitializationResult を返す旨を明記し、旧来の request/result
フローやそれに依存する用語・例を削除または置換して、ServerInitializationResult
の意味（初期化成功/失敗やエラー情報等）と呼び出し側の期待動作を簡潔に記述してください。
In `@Packages/docs/ARCHITECTURE_Unity.md`:
- Line 260: Update the prose that describes the initialization lifecycle so it
matches the new no-request contract: replace any wording that says
UnityCliLoopServerInitializationUseCase orchestrates request/result values with
wording that ExecuteAsync(CancellationToken): Task<ServerInitializationResult>
performs initialization without client request/response semantics. Locate
references to UnityCliLoopServerInitializationUseCase and the lifecycle prose
around the later section (around the current Line ~465) and rephrase to state
that the use case returns a ServerInitializationResult directly from
ExecuteAsync and does not depend on incoming requests or request/result
orchestration.
---
Nitpick comments:
In `@Packages/src/Cli`~/internal/cli/fix_test.go:
- Around line 35-68: The test
TestCleanupStaleRecoveryStateRemovesServerStateFiles seeds three server state
files but omits the backup file that cleanupServerStateFiles should also remove;
update the test to create a backup file at statePath+serverStateBackupSuffix
before calling cleanupStaleRecoveryState, assert the returned cleaned count
increments to 4 (or adjust expectation accordingly), and add an os.Stat check to
ensure the backup file was removed after cleanup; reference
TestCleanupStaleRecoveryStateRemovesServerStateFiles, cleanupStaleRecoveryState,
cleanupServerStateFiles, and serverStateBackupSuffix to locate where to add the
seed and assertion.
🪄 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: d216f06b-fc13-4166-96c0-ff61c132c6f1
⛔ Files ignored due to path filters (5)
Assets/Tests/Editor/ServerStartingLockServiceTests.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/Infrastructure/Server/ServerStartingLockService.cs.metais excluded by none and included by none
📒 Files selected for processing (50)
.agents/skills/uloop-clear-console/SKILL.md.agents/skills/uloop-compile/SKILL.md.agents/skills/uloop-find-game-objects/SKILL.md.agents/skills/uloop-get-hierarchy/SKILL.md.agents/skills/uloop-get-logs/SKILL.md.agents/skills/uloop-record-input/SKILL.md.agents/skills/uloop-replay-input/SKILL.md.agents/skills/uloop-run-tests/SKILL.md.agents/skills/uloop-screenshot/SKILL.md.agents/skills/uloop-simulate-keyboard/SKILL.md.agents/skills/uloop-simulate-mouse-input/SKILL.md.agents/skills/uloop-simulate-mouse-ui/SKILL.md.claude/skills/uloop-clear-console/SKILL.md.claude/skills/uloop-compile/SKILL.md.claude/skills/uloop-find-game-objects/SKILL.md.claude/skills/uloop-get-hierarchy/SKILL.md.claude/skills/uloop-get-logs/SKILL.md.claude/skills/uloop-record-input/SKILL.md.claude/skills/uloop-replay-input/SKILL.md.claude/skills/uloop-run-tests/SKILL.md.claude/skills/uloop-screenshot/SKILL.md.claude/skills/uloop-simulate-keyboard/SKILL.md.claude/skills/uloop-simulate-mouse-input/SKILL.md.claude/skills/uloop-simulate-mouse-ui/SKILL.mdAssets/Tests/Editor/CompilationLockFileServiceTests.csAssets/Tests/Editor/DomainReloadDetectionServiceTests.csAssets/Tests/Editor/DomainReloadRecoveryUseCaseTests.csAssets/Tests/Editor/ServerLifecycleContractTests.csAssets/Tests/Editor/ServerStartingLockServiceTests.csAssets/Tests/Editor/UnityCliLoopEditorSettingsRecoveryTests.csAssets/Tests/Editor/UnityCliLoopServerControllerStartupLockTests.csAssets/Tests/Editor/UnityCliLoopServerStartupProtectionTests.csPackages/docs/ARCHITECTURE_Unity.mdPackages/docs/ARCHITECTURE_Unity_ja.mdPackages/src/Cli~/internal/cli/fix.goPackages/src/Cli~/internal/cli/fix_test.goPackages/src/Editor/Application/CompilationLockService.csPackages/src/Editor/Application/SessionRecoveryService.csPackages/src/Editor/Application/UnityCliLoopServerApplicationService.csPackages/src/Editor/Application/UnityCliLoopServerStartupService.csPackages/src/Editor/Application/UseCases/UnityCliLoopServerInitializationUseCase.csPackages/src/Editor/Domain/DomainReloadDetectionService.csPackages/src/Editor/Domain/ServerLifecycleContract.csPackages/src/Editor/FirstPartyTools/Compile/CompileResponse.csPackages/src/Editor/FirstPartyTools/Compile/Skill/SKILL.mdPackages/src/Editor/Infrastructure/Server/CompilationLockFileService.csPackages/src/Editor/Infrastructure/Server/DomainReloadDetectionFileService.csPackages/src/Editor/Infrastructure/Server/ServerStartingLockService.csPackages/src/Editor/Infrastructure/Server/UnityCliLoopServerController.csPackages/src/Editor/Infrastructure/UnityCliLoopBridgeServer.cs
💤 Files with no reviewable changes (7)
- Packages/src/Editor/Domain/ServerLifecycleContract.cs
- Assets/Tests/Editor/CompilationLockFileServiceTests.cs
- Packages/src/Editor/Domain/DomainReloadDetectionService.cs
- Assets/Tests/Editor/ServerLifecycleContractTests.cs
- Assets/Tests/Editor/ServerStartingLockServiceTests.cs
- Packages/src/Editor/Application/SessionRecoveryService.cs
- Packages/src/Editor/Infrastructure/Server/ServerStartingLockService.cs
Keep the pull request focused on the readiness contract change by removing generated .claude skill snapshots from the branch diff.
Drop the unused Unity architecture markdown and its Unity meta file so the pull request no longer carries changes to a document that should not be maintained. Remove stale code-comment references that would otherwise point to the deleted file.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 24 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".claude/skills/uloop-compile/SKILL.md">
<violation number="1">
P2: The documented compile flag was flipped to `--wait-for-domain-reload`, but the CLI/docs still use `--no-wait-for-domain-reload`, so this skill now teaches an option name that may not work.</violation>
<violation number="2">
P1: Troubleshooting was changed back to lock-file cleanup, but `uloop fix` now cleans server readiness state files; this guidance will send users to the wrong root cause.</violation>
</file>
<file name=".agents/skills/uloop-compile/SKILL.md">
<violation number="1">
P2: This troubleshooting instruction is outdated: `uloop fix` now cleans stale recovery/readiness state files (`server-state.json*`), not lock files.</violation>
<violation number="2">
P2: The documented `--wait-for-domain-reload` flag does not match the CLI’s actual option (`--no-wait-for-domain-reload`), so users can be given an unsupported command.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Delete the remaining Japanese Unity architecture document and its Unity meta file so the PR no longer retains the removed architecture docs in any language.
hatayama
changed the title
Merged
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
uloop fixnow cleans the stale readiness state files that actually affect busy detection.User Impact
uloop fixtargets the current readiness state source instead of obsolete lock files.Changes
domainreload.lock,compiling.lock, andserverstarting.lockproducers, cleanup paths, tests, and startup policy plumbing.ARCHITECTURE_Unity.md,ARCHITECTURE_Unity_ja.md, and stale references to those docs.Verification
scripts/check-go-cli.shPackages/src/Cli~/dist/darwin-arm64/uloop compile --project-path /Users/a12115/ghq/hatayama/unity-cli-loopPackages/src/Cli~/dist/darwin-arm64/uloop run-tests --project-path /Users/a12115/ghq/hatayama/unity-cli-loop --test-mode EditMode --filter-type regex --filter-value ".*(CompilationLockFileServiceTests|DomainReloadDetectionServiceTests|DomainReloadRecoveryUseCaseTests|UnityCliLoopServerControllerRecoveryTests|UnityCliLoopServerStartupProtectionTests|ServerLifecycleContractTests|UnityCliLoopEditorSettingsRecoveryTests).*"codex-review v3-beta