Only regenerate snapshot if Send/Receive works#2069
Conversation
github-actions
bot
added
💻 FW Lite
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR refactors dependency injection to use interface abstractions for four services (ISyncJobStatusService, ISendReceiveService, IProjectLookupService, IProjectMetadataService), introduces a new ProjectSnapshotService for snapshot management, and enhances the sync workflow to detect and handle Send/Receive rollbacks by blocking projects from future synchronization. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 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 |
C# Unit Tests160 tests 160 ✅ 18s ⏱️ Results for commit b04fc67. ♻️ This comment has been updated with latest results. |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/FwHeadless/Services/SyncHostedService.cs(1 hunks)backend/FwLite/FwLiteProjectSync.Tests/CrdtRepairTests.cs(2 hunks)backend/FwLite/FwLiteProjectSync.Tests/Sena3SyncTests.cs(1 hunks)backend/FwLite/FwLiteProjectSync/CrdtFwdataProjectSyncService.cs(0 hunks)
💤 Files with no reviewable changes (1)
- backend/FwLite/FwLiteProjectSync/CrdtFwdataProjectSyncService.cs
🧰 Additional context used
🧬 Code graph analysis (2)
backend/FwHeadless/Services/SyncHostedService.cs (1)
backend/LexCore/Entities/Project.cs (1)
Project(9-75)
backend/FwLite/FwLiteProjectSync.Tests/CrdtRepairTests.cs (1)
backend/FwLite/FwLiteShared/Sync/SyncService.cs (1)
SyncService(23-240)
🪛 GitHub Actions: Develop FwHeadless CI/CD
backend/FwHeadless/Services/SyncHostedService.cs
[error] 223-223: CS1002: ; expected. Command: dotnet build backend/FwHeadless/FwHeadless.csproj
🪛 GitHub Check: Build FwHeadless / publish-fw-headless
backend/FwHeadless/Services/SyncHostedService.cs
[failure] 223-223:
} expected
[failure] 223-223:
; expected
[failure] 223-223:
} expected
[failure] 223-223:
; expected
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build API / publish-api
- GitHub Check: Build FW Lite and run tests
- GitHub Check: frontend
- GitHub Check: frontend-component-unit-tests
- GitHub Check: Analyze (csharp)
🔇 Additional comments (5)
backend/FwLite/FwLiteProjectSync.Tests/Sena3SyncTests.cs (1)
228-228: Test adapted to new explicit snapshot regeneration pattern.The explicit call to
RegenerateProjectSnapshotafterSyncis the expected pattern following the removal of automatic snapshot regeneration from the sync service.backend/FwLite/FwLiteProjectSync.Tests/CrdtRepairTests.cs (2)
93-93: Test adapted to new explicit snapshot regeneration pattern.The explicit call to
RegenerateProjectSnapshotafterSyncis the expected pattern following the removal of automatic snapshot regeneration from the sync service.
160-160: Test adapted to new explicit snapshot regeneration pattern.The explicit call to
RegenerateProjectSnapshotafterSyncis the expected pattern following the removal of automatic snapshot regeneration from the sync service.backend/FwHeadless/Services/SyncHostedService.cs (2)
210-224: Conditional snapshot regeneration correctly implements PR objective.The logic correctly ensures snapshots are only regenerated when:
- CRDT changes exist (
result.CrdtChanges > 0), AND- Send/Receive either succeeded or wasn't needed (no early return at line 203)
If Send/Receive fails after CRDT sync (line 203), the early return prevents snapshot regeneration, which is correct since the changes might have been rolled back.
226-226: Verify dual Harmony sync pattern is intentional.The code now performs Harmony sync twice:
- Conditionally at line 176 (if
HasSyncedSuccessfully)- Unconditionally at line 226 (after snapshot regeneration)
This appears intentional to ensure Harmony is synced both before and after the CRDT/FW sync operations, but you may want to verify this pattern is necessary and doesn't cause redundant work.
myieye
force-pushed
the
2052-only-regenerate-snapshot-if-send-receive-works
branch
2 times, most recently
from
3e9c79d to
d14de81
Compare
myieye
force-pushed
the
2052-only-regenerate-snapshot-if-send-receive-works
branch
from
3e4c29b to
3007145
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@backend/FwHeadless/Services/ProjectMetadataService.cs`:
- Around line 10-22: BlockFromSyncAsync currently sets
metadata.SyncBlocked.Reason to reason ?? "Manual block" but logs the original
nullable reason, which can print "null"; fix by computing a single
resolvedReason (e.g., var resolvedReason = reason ?? "Manual block") and use
that resolvedReason both when assigning metadata.SyncBlocked.Reason inside the
_store.UpdateAsync lambda and when calling logger.LogWarning so the log shows
the actual stored reason; reference BlockFromSyncAsync, _store.UpdateAsync,
metadata.SyncBlocked, SyncBlockedInfo, and logger.LogWarning.
In `@backend/FwLite/FwLiteProjectSync/CrdtFwdataProjectSyncService.cs`:
- Around line 57-88: The callers of Sync()/Import() must regenerate the project
snapshot after a successful non-dry-run operation; update the call sites in
Program.cs and Utils.cs to, after the awaited Sync()/Import() returns
successfully and dryRun is false, call and await
projectSnapshotService.RegenerateProjectSnapshot(crdtApi, fwdataApi.Project,
keepBackup: false) (following the pattern in SyncHostedService.cs). Ensure you
pass the same crdtApi and fwdataApi.Project objects used for the sync/import,
await the regeneration, and only skip this step when dryRun is true.
In `@backend/FwLite/FwLiteProjectSync/Program.cs`:
- Around line 68-75: The code handles Import vs Sync but never regenerates the
ProjectSnapshot afterwards; update the flow so that once syncService.Import or
syncService.Sync completes successfully you regenerate and persist the snapshot
from the CRDT (not from FwData). After the existing call to
logger.LogInformation, call the ProjectSnapshotService method that creates/saves
a snapshot from the CRDT (use ProjectSnapshotService with the crdtApi/its
Project or CRDT root) so the new snapshot replaces the old one and future syncs
use the CRDT-derived snapshot.
In `@backend/FwLite/FwLiteProjectSync/ProjectSnapshotService.cs`:
- Around line 12-18: GetProjectSnapshot currently deserializes with
crdtConfig.Value.JsonSerializerOptions which filters out [MiniLcmInternal]
fields and causes data loss; change GetProjectSnapshot to deserialize using the
same serializer options used in SaveProjectSnapshot (the options that preserve
[MiniLcmInternal] fields) so round-trip preserves Order/MaybeId/etc. Locate
GetProjectSnapshot and replace the JsonSerializer.DeserializeAsync call to pass
the matching preserved-options instance (the same variable or constant used by
SaveProjectSnapshot) and ensure SnapshotPath and file handling remain unchanged.
In `@backend/FwLite/LcmDebugger/Utils.cs`:
- Around line 77-88: SyncFwHeadlessProject currently chooses Import vs Sync
(CrdtFwdataProjectSyncService.Import / Sync) based on
snapshotService.GetProjectSnapshot but never updates the stored snapshot after a
successful non-dry-run run; update the code so that when dryRun is false and the
Import/Sync returns success you regenerate/persist the new snapshot via the
ProjectSnapshotService (e.g., call the service's method that creates/updates the
project snapshot for fwDataMiniLcmApi.Project — use the appropriate
ProjectSnapshotService method such as
UpdateProjectSnapshot/CreateProjectSnapshot/SaveProjectSnapshot) so subsequent
runs see the fresh snapshot.
🧹 Nitpick comments (4)
backend/Testing/FwHeadless/ProjectMetadataServiceTests.cs (1)
14-17: Consider disposing the LoggerFactory.The
LoggerFactorycreated in the constructor implementsIDisposablebut is never disposed. While this is unlikely to cause issues in test scenarios, it's good practice to dispose it.♻️ Proposed fix
-public class ProjectMetadataServiceTests : IAsyncLifetime +public class ProjectMetadataServiceTests : IAsyncLifetime, IDisposable { - private readonly ILogger<ProjectMetadataService> _logger; + private readonly LoggerFactory _loggerFactory; + private readonly ILogger<ProjectMetadataService> _logger; private FwHeadlessConfig _config = null!; private string _tempDir = null!; public ProjectMetadataServiceTests() { - _logger = new LoggerFactory().CreateLogger<ProjectMetadataService>(); + _loggerFactory = new LoggerFactory(); + _logger = _loggerFactory.CreateLogger<ProjectMetadataService>(); } + + public void Dispose() + { + _loggerFactory.Dispose(); + }backend/FwLite/FwLiteProjectSync/Program.cs (1)
20-37: Placeholder commands - consider removing or documenting intent.The
before-syncandafter-synccommands only print the arguments and don't perform any actual operations. If these are intentional hooks for future use, consider adding a comment explaining their purpose. Otherwise, consider removing them to avoid confusion.backend/FwLite/FwLiteProjectSync.Tests/ProjectSnapshotServiceTests.cs (1)
134-151: TODO tracking: CRDT-only enforcement.
Please track/resolve this TODO before release so the invariant doesn’t get lost.If you want, I can draft a small follow-up task or propose an API change to enforce this invariant.
backend/FwLite/FwLiteProjectSync/ProjectSnapshotService.cs (1)
36-54: Consider atomic snapshot writes to avoid partial files.
Directly writing to the target path risks truncated or partially written snapshots if the process crashes mid‑write; that can later break sync flows. Writing to a temp file and replacing reduces this risk.✅ Safer write pattern
internal static async Task SaveProjectSnapshot(FwDataProject project, ProjectSnapshot projectSnapshot, bool keepBackup = false) { var snapshotPath = SnapshotPath(project); @@ - await using var file = File.Create(snapshotPath); - //not using our serialization options because we don't want to exclude MiniLcmInternal - await JsonSerializer.SerializeAsync(file, projectSnapshot); + var tempPath = Path.Combine(Path.GetDirectoryName(snapshotPath)!, Path.GetRandomFileName()); + await using (var file = File.Create(tempPath)) + { + //not using our serialization options because we don't want to exclude MiniLcmInternal + await JsonSerializer.SerializeAsync(file, projectSnapshot); + } + File.Move(tempPath, snapshotPath, overwrite: true); }
backend/FwLite/FwLiteProjectSync/CrdtFwdataProjectSyncService.cs
Outdated
Show resolved
Hide resolved
|
Some files (e.g. MediaFileService.cs) have been converted from LF line endings to CRLF line endings in this PR, resulting in a diff that incorrectly shows every line changed (until you click on the gear icon in GitHub and choose "Hide whitespace". Is that the result of editor misconfiguration, or did an AI touch that file and cause the CRLF to get added? |
myieye
force-pushed
the
2052-only-regenerate-snapshot-if-send-receive-works
branch
from
6b07241 to
b04fc67
Compare
myieye
deleted the
2052-only-regenerate-snapshot-if-send-receive-works
branch
2 participants
Resolves #2052
This PR pulls snapshot regeneration/updating out of the Sync method, so that it is only done after a successful Send/Receive.
Additionally, all the other snapshot code was pulled out (except for a basic verification that Sync vs Import are being called correctly), which meant a large refactor, because snapshots are now managed externally to the Sync method: they need to be manually fetched, explicitly passed and manually regenerated. This makes things more explicit, definitely a bit more verbose, but, in my opinion easier to follow and there are now certainly enough tests in place to ensure that our sync code does this correctly.