create project snapshot from crdt project when missing#1753
Conversation
…roject to restart the sync from scratch, rather than trying to resume where we left off
…apshot, create one from what's currently in the database, this means even a partial import will resume more or less where it left off.
📝 WalkthroughWalkthroughThe changes refactor project setup and snapshot handling logic in the synchronization process. Methods are reorganized for clarity, with new helpers for CRDT project opening and snapshot checks. The code now leverages injected services, reduces parameter passing, and introduces snapshot availability checks to improve robustness during sync operations. Changes
Assessment against linked issues
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/FwHeadless/Services/SyncHostedService.cs (1)
196-210: Snapshot write happens without cancellation / atomicity
SetupCrdtProjectwrites a new snapshot when missing, but:
- No
CancellationTokenis passed down – an aborted host shutdown will leave a partial file.SaveProjectSnapshotusesFile.Create, meaning readers can open a half-written file.Use a temp-file +
File.Replace(atomic move) and plumb the existingstoppingTokenthrough to allow graceful cancellation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/FwHeadless/Services/SyncHostedService.cs(4 hunks)backend/FwLite/FwLiteProjectSync/CrdtFwdataProjectSyncService.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Build API / publish-api
- GitHub Check: Build FwHeadless / publish-fw-headless
- GitHub Check: Analyze (csharp)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build FW Lite and run tests
- GitHub Check: frontend
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
hahn-kev
changed the title
| { | ||
| return new CrdtProject("crdt", crdtFile); | ||
| logger.LogWarning("No snapshot available for project {ProjectId}, creating a new one, maybe the last sync didn't finish?", projectId); | ||
| await syncService.SaveProjectSnapshot(fwDataProject, api); |
There was a problem hiding this comment.
I think we should throw (or something) if projectLookupService.IsCrdtProject(projectId).
I don't know how that would happen, but it wouldn't be good 🤷.
Would it maybe make sense to query and assert that sort of stuff all at once higher up? Something like IsInValidSyncableState()
|
per a discussion in a team meeting, this will be changed to just make the import resumable, rather than messing around with snapshots. |
|
replaced by #1760 |
2 participants
closes #1735
Changes FW Headless so that if the previous sync failed to create a snapshot (OOM, restart, or bug) then we will create a snapshot based on what's in the CRDT db. Then when the sync executes it will be able to resume where it left off, instead of trying to import again.