fix: Make third-party migration writes an all-or-nothing batch - #1701
Conversation
A mid-batch File.Replace failure previously left projects half-migrated with no rollback. Prepare all temp sidecars first, keep .bak files until the full commit succeeds, and restore already committed targets on failure. Co-authored-by: Cursor <cursoragent@cursor.com>
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMigration file updates now use atomic batch writes with temporary files, backups, rollback, and cleanup. Synchronous and asynchronous services delegate to the batch writer, with tests covering success, failures, rollback, sidecar cleanup, and large batches. ChangesBatch migration writes
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MigrationFileService
participant ThirdPartyToolMigrationFileWriter
participant FileSystem
MigrationFileService->>ThirdPartyToolMigrationFileWriter: Submit migration changes
ThirdPartyToolMigrationFileWriter->>FileSystem: Write temporary sidecars
ThirdPartyToolMigrationFileWriter->>FileSystem: Commit moves or replacements
FileSystem-->>ThirdPartyToolMigrationFileWriter: Return commit result
ThirdPartyToolMigrationFileWriter->>FileSystem: Restore backups or delete new targets on failure
ThirdPartyToolMigrationFileWriter-->>MigrationFileService: Complete batch or propagate exception
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationFileWriter.cs`:
- Around line 90-95: Update PrepareAll and PrepareAllAsync to register each
generated temp sidecar path in preparedWrites before invoking Prepare or
WriteAllText. Ensure cleanup can reach the path when writing fails, while
preserving the existing PreparedWrite flow for successful writes.
🪄 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: 8ddb61f3-c3c3-4625-888d-5d46ed50334e
⛔ Files ignored due to path filters (1)
Assets/Tests/Editor/ThirdPartyToolMigrationFileWriterTests.cs.metais excluded by none and included by none
📒 Files selected for processing (3)
Assets/Tests/Editor/ThirdPartyToolMigrationFileWriterTests.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationFileService.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationFileWriter.cs
WriteAllText can leave a partial .tmp if it fails mid-write. Registering the sidecar path first lets prepare-failure cleanup still delete it. Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
WritewithWriteBatch/WriteBatchAsyncso migration apply is one prepare → commit transaction.baksidecars until the full commit succeeds; on mid-batch failure, restore already committed targets and leave no half-migrated projectUser Impact
Applying a V2→V3 third-party tool migration no longer leaves the project half-rewritten when a file write fails mid-batch; already written files are restored from backups.
Test plan
uloop compile— 0 errors / 0 warningsuloop run-tests --filter-type regex --filter-value ThirdPartyToolMigrationFileWriterTests— 5/5 passeduloop execute-dynamic-code+ reflection: inject commit failure with a directory target →IOException, file1 restored to original, file3 untouched, sidecarCount=0Live verification log