Skip to content

fix: Surface startup recovery failures when no server is restored#1553

Merged
hatayama merged 4 commits into
v3-betafrom
codex/c4-7-recovery-unification
Jul 6, 2026
Merged

fix: Surface startup recovery failures when no server is restored#1553
hatayama merged 4 commits into
v3-betafrom
codex/c4-7-recovery-unification

Conversation

@hatayama

@hatayama hatayama commented Jul 6, 2026

Copy link
Copy Markdown
Owner

Summary

  • Startup recovery now reports a failure when recovery finishes without a running server.
  • Clean editor startup still starts the server automatically, matching the existing reload recovery behavior.

User Impact

  • Before this change, startup protection could suppress startup recovery with no current server and leave the failure silent.
  • After this change, the same missing-server condition is surfaced as a startup recovery failure instead of looking successful.

Changes

  • Route editor-startup restoration through the same session recovery decision service used after domain reload.
  • Keep the controller-owned background-process guard before the shared recovery decision.
  • Add pure C# decision-tree pins for session recovery and update the startup protection gap expectation.
  • Emit a normal-build UnityEngine.Debug.LogError when startup restore faults, matching the tracked recovery path.
  • Consolidate the recovery coordinator test fake and fold duplicate success/failure-message scenarios into the existing service tests.
  • Leave recovery task arbitration, in-flight recovery scheduling, duplicate startup-protection clearing, and the SettingsWindow RecoveryTask exception-handling gap for separate follow-up PRs.

Verification

  • dist/darwin-arm64/uloop compile --project-path "$(git rev-parse --show-toplevel)" -> 0 errors / 0 warnings
  • dist/darwin-arm64/uloop run-tests --project-path "$(git rev-parse --show-toplevel)" --test-mode EditMode --filter-type regex --filter-value "(DomainReloadRecoveryUseCaseTests|UnityCliLoopServerControllerRecoveryTests|UnityCliLoopServerControllerStartupLockTests|UnityCliLoopServerStartupProtectionTests|UnityCliLoopBridgeServerShutdownTests|JsonRpcHeartbeatTests|UnityCliLoopServerLifecycleRegistryServiceTests|ServerLifecycleContractTests|UnityCliLoopFirstPartyServerLifecycleBindingTests|JsonRpcProcessorCliVersionGateTests|OnionAssemblyDependencyTests)" -> 145/145 passed

hatayama added 3 commits July 6, 2026 20:52
Capture the current SessionRecoveryService decision tree and the startup-protection gap before unifying startup restoration with reload recovery.
Delegate editor-startup restoration to the same recovery decision service used after domain reload while keeping the controller-owned background-process guard.
Update the startup recovery pin to match the shared session recovery validation now used by the controller.
@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This PR introduces a SessionRecoveryService dependency into UnityCliLoopServerControllerService, replacing inline recovery decision logic in RestoreServerStateIfNeeded with a delegated async call that throws InvalidOperationException on invalid results. Composition root and test wiring are updated accordingly, alongside new and expanded recovery-related tests.

Changes

Session recovery delegation

Layer / File(s) Summary
Controller field, constructor, and recovery call
Packages/src/Editor/Infrastructure/Server/UnityCliLoopServerController.cs
Adds a SessionRecoveryService field and constructor parameter with null validation; RestoreServerStateIfNeeded now delegates to RestoreServerStateIfNeededAsync and throws InvalidOperationException on an invalid ValidationResult, replacing prior _sessionFlagsRepository-driven branching.
Composition root and test helper wiring
Packages/src/Editor/CompositionRoot/UnityCliLoopApplicationRegistration.cs, Assets/Tests/Editor/UnityCliLoopServerStartupProtectionTests.cs, Assets/Tests/Editor/UnityCliLoopServerControllerStartupLockTests.cs
Passes sessionRecoveryService into UnityCliLoopServerControllerService construction in production registration and test helpers; adds an optional startupProtectionService parameter to one test helper.
Recovery behavior tests
Assets/Tests/Editor/DomainReloadRecoveryUseCaseTests.cs, Assets/Tests/Editor/UnityCliLoopServerControllerStartupLockTests.cs
Adds tests covering AfterCompile consumption, manual stop precedence, missing/persisted server state, and startup-protection failure scenarios, with new in-memory test infrastructure (NoOpDomainReloadDetectionService, InMemorySessionFlagsRepository, PureRecoveryCoordinator).

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant UnityCliLoopServerControllerService
  participant SessionRecoveryService

  Caller->>UnityCliLoopServerControllerService: RestoreServerStateIfNeeded()
  UnityCliLoopServerControllerService->>SessionRecoveryService: RestoreServerStateIfNeededAsync()
  SessionRecoveryService-->>UnityCliLoopServerControllerService: ValidationResult
  alt result invalid
    UnityCliLoopServerControllerService-->>Caller: throw InvalidOperationException(ErrorMessage)
  else result valid
    UnityCliLoopServerControllerService-->>Caller: recovery complete
  end
Loading

Possibly related PRs

  • hatayama/unity-cli-loop#1166: Refactors SessionRecoveryService.RestoreServerStateIfNeededAsync to use session-scoped state, the same logic delegated to and tested in this PR.
  • hatayama/unity-cli-loop#1182: Modifies recovery-after-reload control flow and manual-stop handling closely aligned with the tests expanded here.
  • hatayama/unity-cli-loop#1534: Refactors SessionRecoveryService.RestoreServerStateIfNeededAsync to use ISessionFlagsRepository, directly related to the controller/test wiring changed here.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title clearly summarizes the main change: surfacing startup recovery failures when no server is restored.
Description check ✅ Passed The description is directly related to the changeset and accurately describes the startup recovery behavior and test updates.
✨ 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/c4-7-recovery-unification

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 5 files

Re-trigger cubic

Add a Unity console error for startup restore failures and consolidate recovery test fakes so the new pins stay focused on one behavior each.
@hatayama
hatayama merged commit d5e5d55 into v3-beta Jul 6, 2026
10 checks passed
@hatayama
hatayama deleted the codex/c4-7-recovery-unification branch July 6, 2026 12:16
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