fix: Interrupted PlayMode tests no longer leave domain reload disabled#1254
Merged
Conversation
Save the original Enter Play Mode settings in a delete-on-restore UserSettings marker before disabling domain reload. This prevents abandoned PlayMode test runs from leaving project settings dirty and keeps nested scopes active until the final scope exits.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds crash-safe recovery for DomainReloadDisableScope by persisting original Enter Play Mode settings to an atomic JSON marker, restoring them on next load if a run was abandoned, making the scope reference-counted so only the last Dispose restores settings, integrating pre-run recovery into the PlayMode executor, and adding tests and friend assembly access. ChangesCrash-safe Domain Reload Scope Recovery
Sequence Diagram(s)sequenceDiagram
participant Editor
participant DomainReloadDisableScopeRecovery
participant FileSystem
participant EditorSettings
Editor->>DomainReloadDisableScopeRecovery: InitializeOnLoadMethod()
DomainReloadDisableScopeRecovery->>FileSystem: Delete temp file if present
DomainReloadDisableScopeRecovery->>FileSystem: Check for marker file
FileSystem-->>DomainReloadDisableScopeRecovery: marker JSON payload
DomainReloadDisableScopeRecovery->>DomainReloadDisableScopeRecovery: Read and validate payload bits
DomainReloadDisableScopeRecovery->>EditorSettings: Restore originalOptionsEnabled & originalOptions
DomainReloadDisableScopeRecovery->>FileSystem: Delete marker and temp artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/src/Editor/FirstPartyTools/Common/EditorUtility/DomainReloadDisableScope.cs`:
- Around line 16-22: The code only runs
DomainReloadDisableScopeRecovery.RestoreIfPending() and SaveCurrentSettings()
when _activeScopeCount == 0, which fails to recover after an abandoned scope
within the same Editor session; add stale-run detection and reset logic so a new
top-level scope always triggers recovery. Implement a lightweight run/session
token (e.g., a static GUID or timestamp) stored alongside _activeScopeCount,
compare the stored token to a current RunId at scope creation in the
DomainReloadDisableScope constructor/enter path, and if it differs reset
_activeScopeCount to 0 and call
DomainReloadDisableScopeRecovery.RestoreIfPending() and
DomainReloadDisableScopeRecovery.SaveCurrentSettings() before incrementing
_activeScopeCount; apply the same detection to the analogous block around lines
45-47; keep ResetActiveScopeCountForTests() behavior consistent by updating the
token there for tests.
In
`@Packages/src/Editor/FirstPartyTools/Common/EditorUtility/DomainReloadDisableScopeRecovery.cs`:
- Around line 57-60: ReadMarkerData()/RestoreIfPending() currently casts
DomainReloadDisableScopeRecoveryData.originalOptions into EnterPlayModeOptions
without validation; add a bitmask validation step that masks the recovered int
against the whitelist of valid flags (EnterPlayModeOptions.DisableDomainReload |
EnterPlayModeOptions.DisableSceneReload |
EnterPlayModeOptions.DisableSceneBackupUnlessDirty) and reject or clamp any bits
outside that mask (e.g., treat invalid mask as EnterPlayModeOptions.None or
ignore the marker) before assigning to EditorSettings.enterPlayModeOptions in
RestoreIfPending()/right after ReadMarkerData() so corrupted marker files cannot
set unsupported flags.
🪄 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: 342b7cb7-909f-422b-91f2-6313da2e3123
⛔ Files ignored due to path filters (3)
Assets/Tests/Editor/DomainReloadDisableScopeTests.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Common/EditorUtility/AssemblyInfo.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Common/EditorUtility/DomainReloadDisableScopeRecovery.cs.metais excluded by none and included by none
📒 Files selected for processing (4)
Assets/Tests/Editor/DomainReloadDisableScopeTests.csPackages/src/Editor/FirstPartyTools/Common/EditorUtility/AssemblyInfo.csPackages/src/Editor/FirstPartyTools/Common/EditorUtility/DomainReloadDisableScope.csPackages/src/Editor/FirstPartyTools/Common/EditorUtility/DomainReloadDisableScopeRecovery.cs
There was a problem hiding this comment.
3 issues found across 7 files
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="Packages/src/Editor/FirstPartyTools/Common/EditorUtility/DomainReloadDisableScopeRecovery.cs">
<violation number="1" location="Packages/src/Editor/FirstPartyTools/Common/EditorUtility/DomainReloadDisableScopeRecovery.cs:122">
P2: File deletion in recovery cleanup is not best-effort; unhandled IO/access errors can break restore flow during editor load or test scope setup.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| { | ||
| if (File.Exists(DomainReloadDisableScopeRecoveryConstants.MarkerFilePath)) | ||
| { | ||
| File.Delete(DomainReloadDisableScopeRecoveryConstants.MarkerFilePath); |
There was a problem hiding this comment.
P2: File deletion in recovery cleanup is not best-effort; unhandled IO/access errors can break restore flow during editor load or test scope setup.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/src/Editor/FirstPartyTools/Common/EditorUtility/DomainReloadDisableScopeRecovery.cs, line 122:
<comment>File deletion in recovery cleanup is not best-effort; unhandled IO/access errors can break restore flow during editor load or test scope setup.</comment>
<file context>
@@ -0,0 +1,156 @@
+ {
+ if (File.Exists(DomainReloadDisableScopeRecoveryConstants.MarkerFilePath))
+ {
+ File.Delete(DomainReloadDisableScopeRecoveryConstants.MarkerFilePath);
+ }
+ }
</file context>
Owner
Author
There was a problem hiding this comment.
Thanks for the note. I intentionally left this unchanged in 3a616a6.
Deletion failures should remain fail-fast here: swallowing IO/access failures would hide a stale recovery marker and make later setting restoration harder to reason about. This also matches the repository policy to avoid catch blocks unless there is a clear recovery path.
There was a problem hiding this comment.
Thanks for the feedback! I already have a similar learning that covers this, so I'm keeping the existing one.
Owner
Author
There was a problem hiding this comment.
Follow-up addressed in 6a58b17.
I kept deletion failures fail-fast rather than best-effort, but I did remove the delete-before-move window from the marker save path. SaveCurrentSettings() now fails before writing a temp marker when an unrecovered marker already exists, so saving no longer deletes the existing recovery marker before moving the new one into place.
The focused EditMode test now covers that an existing marker is preserved and no temp marker is left behind.
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
Reset stale scope state when a new PlayMode test run starts so pending recovery markers are consumed in the same Editor session. Validate recovered Enter Play Mode option bits before applying marker data, and align EditorUtility internal visibility with sibling assemblies.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Assets/Tests/Editor/DomainReloadDisableScopeTests.cs (1)
169-183:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
RestoreFileleaks files a test created when none existed atSetUp.
RestoreFilereturns early whenfileExistedisfalse, so it never deletes a file the test wrote.RestoreIfPending_RejectsUnsupportedEnterPlayModeOptionBitswrites the marker at Line 153 andRestoreIfPending()throws before deleting it, so on a clean checkout (no pre-existing marker) the marker survivesTearDown. That stale marker can then be consumed by a later test or by the next Editor startup, breaking isolation.🧹 Proposed fix: delete the file when it did not exist originally
private static void RestoreFile(string filePath, bool fileExisted, string fileContent) { if (!fileExisted) { + if (File.Exists(filePath)) + { + File.Delete(filePath); + } return; }🤖 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 `@Assets/Tests/Editor/DomainReloadDisableScopeTests.cs` around lines 169 - 183, RestoreFile currently returns immediately when fileExisted is false, which leaves test-created files on disk; update RestoreFile (used by RestoreIfPending and tests like RestoreIfPending_RejectsUnsupportedEnterPlayModeOptionBits) so that when fileExisted is false it deletes the file if it exists (e.g., File.Exists(filePath) -> File.Delete(filePath)) rather than returning early, and retain the existing logic to recreate directories/write content when fileExisted is true.
🤖 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.
Outside diff comments:
In `@Assets/Tests/Editor/DomainReloadDisableScopeTests.cs`:
- Around line 169-183: RestoreFile currently returns immediately when
fileExisted is false, which leaves test-created files on disk; update
RestoreFile (used by RestoreIfPending and tests like
RestoreIfPending_RejectsUnsupportedEnterPlayModeOptionBits) so that when
fileExisted is false it deletes the file if it exists (e.g.,
File.Exists(filePath) -> File.Delete(filePath)) rather than returning early, and
retain the existing logic to recreate directories/write content when fileExisted
is true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c11be77c-95e6-4b04-8423-c5ce939a7a80
📒 Files selected for processing (5)
Assets/Tests/Editor/DomainReloadDisableScopeTests.csPackages/src/Editor/FirstPartyTools/Common/EditorUtility/AssemblyInfo.csPackages/src/Editor/FirstPartyTools/Common/EditorUtility/DomainReloadDisableScope.csPackages/src/Editor/FirstPartyTools/Common/EditorUtility/DomainReloadDisableScopeRecovery.csPackages/src/Editor/FirstPartyTools/RunTests/TestFramework/PlayModeTestExecuter.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Packages/src/Editor/FirstPartyTools/Common/EditorUtility/DomainReloadDisableScopeRecovery.cs
- Packages/src/Editor/FirstPartyTools/Common/EditorUtility/DomainReloadDisableScope.cs
There was a problem hiding this comment.
1 issue found across 5 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="Packages/src/Editor/FirstPartyTools/Common/EditorUtility/DomainReloadDisableScopeRecovery.cs">
<violation number="1" location="Packages/src/Editor/FirstPartyTools/Common/EditorUtility/DomainReloadDisableScopeRecovery.cs:122">
P2: File deletion in recovery cleanup is not best-effort; unhandled IO/access errors can break restore flow during editor load or test scope setup.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Remove corrupted recovery markers before failing fast so invalid marker data cannot trap the Editor in the same recovery exception on every load. Keep the existing validation strict and cover marker cleanup in the focused EditMode test.
Fail before writing a temp marker when an existing recovery marker is still present, so marker saves cannot create a window where no recovery marker exists. Cover the precondition with focused EditMode coverage.
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
User Impact
Changes
Verification
cli/dist/darwin-arm64/uloop compile --project-path "$(git rev-parse --show-toplevel)"cli/dist/darwin-arm64/uloop run-tests --test-mode EditMode --filter-type regex --filter-value "DomainReloadDisableScope" --project-path "$(git rev-parse --show-toplevel)"codex-review v3-beta --parallel-tests "cli/dist/darwin-arm64/uloop run-tests --test-mode EditMode --filter-type regex --filter-value DomainReloadDisableScope --project-path <PROJECT_ROOT>"Closes #1243