Fix: we should always regenerate snapshot

Author: myieye

backend/FwHeadless/Services/SyncHostedService.cs

@@ -239,22 +239,26 @@ public async Task<SyncJobResult> ExecuteSync(CancellationToken stoppingToken, bo
             }
         }
 
-        //We defer regenerating the snapshot until we know the changes applied to fwdata were successfully pushed.
-        //Otherwise, the changes might have been rolled back. In that case, the next sync could overwrite
-        //CRDT data with old/rolled back FW data.
-        if (result.CrdtChanges > 0)
-        {
-            logger.LogInformation("Regenerating project snapshot to include {CrdtChangeCount} crdt changes", result.CrdtChanges);
-            //note we are using the crdt API, this avoids issues where some data isn't synced yet
-            //later when we add the ability to sync that data we need the snapshot to reflect the synced state, not what was in the FW project
-            //related to https://github.com/sillsdev/languageforge-lexbox/issues/1912
-            await projectSnapshotService.RegenerateProjectSnapshot(miniLcmApi, fwdataApi.Project, keepBackup: false);
-        }
-        else
-        {
-            logger.LogInformation("Skipping regenerating project snapshot, because there were no crdt changes");
-        }
-
+        /*
+        Notes:
+        1) We defer regenerating the snapshot until we know the changes applied to fwdata were successfully pushed.
+        Otherwise, the changes might have been rolled back. In that case, the next sync could overwrite CRDT data with old/rolled back FW data.
+        2) We are intentionaly using the crdt API. This avoids issues when new data/fields don't yet get synced.
+        When we start syncing that data/those fields we need the snapshot to reflect the CRDT state, rather than the FW project,
+        otherwise existing FW data will never be synced to CRDT. Related to https://github.com/sillsdev/languageforge-lexbox/issues/1912
+        3) We should always regenerate the snapshot even if no changes were detected.
+        If the same change was made in both fwdata and crdt, no changes would be detected,
+        but we still need that change to get into the snapshot
+        */
+        logger.LogInformation("Regenerating project snapshot");
+        await projectSnapshotService.RegenerateProjectSnapshot(miniLcmApi, fwdataApi.Project, keepBackup: false);
+
+        /*
+        Push new changes to Harmony (changes that came from FW)
+        Important: we only sync the Harmony project AFTER regenerating the snapshot.
+        Otherwise, the sync could pull changes into the snapshot that were not respected during the sync.
+        Could presumably be skipped if 0 CrdtChanges, but it's cheap.
+        */
         await crdtSyncService.SyncHarmonyProject();
 
         activity?.SetStatus(ActivityStatusCode.Ok, "Sync finished");

backend/Testing/FwHeadless/Services/SyncWorkerTests.cs

@@ -51,7 +51,7 @@ public async Task ExecuteSync_CrdtChangesNoFwChanges_RegeneratesSnapshotWithoutP
     }
 
     [Fact]
-    public async Task ExecuteSync_NoCrdtChanges_DoesNotRegenerateSnapshot()
+    public async Task ExecuteSync_NoChanges_RegeneratesSnapshotAnyway()
     {
         using var h = new SyncWorkerTestHarness();
         var syncResult = new SyncResult(CrdtChanges: 0, FwdataChanges: 0);
@@ -67,6 +67,7 @@ public async Task ExecuteSync_NoCrdtChanges_DoesNotRegenerateSnapshot()
             MediaSyncCrdt,
             GetSnapshot,
             Sync,
+            RegenerateSnapshot,
             HarmonySync);
     }
 
@@ -238,6 +239,7 @@ public async Task ExecuteSync_FwDataFileMissing_ClonesProject()
             MediaSyncCrdt,
             GetSnapshot,
             Sync,
+            RegenerateSnapshot,
             HarmonySync);
     }