[claude] Warn when a sync will leave a client's commits still differing

Directory.Packages.props

@@ -18,6 +18,7 @@
     <PackageVersion Include="System.Linq.Async" Version="7.0.1" />
     <PackageVersion Include="BenchmarkDotNet" Version="0.16.0-preview.1" />
     <PackageVersion Include="FluentAssertions" Version="8.9.0" />
+    <PackageVersion Include="Microsoft.Extensions.Diagnostics.Testing" Version="10.11.0" />
     <PackageVersion Include="GitHubActionsTestLogger" Version="3.0.5" />
     <PackageVersion Include="JetBrains.Profiler.SelfApi" Version="2.5.18" />
     <PackageVersion Include="System.Text.Json" Version="10.0.7" />

src/SIL.Harmony.Core/QueryHelpers.cs

@@ -72,14 +72,14 @@ public static async IAsyncEnumerable<TCommit> GetCommitsMissingFromRemote<TCommi
         }
     }
 
-    private enum SyncPlan
+    internal enum SyncPlan
     {
         SendNothing,
         SendAll,
         SendAfterRemoteTimestamp
     }
 
-    private static SyncPlan PlanFor(ClientState local, ClientState? remote)
+    internal static SyncPlan PlanFor(ClientState local, ClientState? remote)
     {
         //the remote has never seen this client, so push everything
         if (remote is null)

src/SIL.Harmony.Core/SyncState.cs

@@ -52,6 +52,16 @@ public record ClientState(Guid ClientId, long MaxTimestamp, int CommitCount, ulo
     //0 means this came from a ClientHeads-only SyncState (a peer on an older version);
     //states we build always have at least one commit
     public bool OnlyHasTimestamp => CommitCount == 0;
+
+    //count and XOR hash add up over disjoint commit sets, so the state of a union is the sum of the parts
+    internal ClientState Plus(ClientState? other) => other is null ? this : this with
+    {
+        MaxTimestamp = Math.Max(MaxTimestamp, other.MaxTimestamp),
+        CommitCount = CommitCount + other.CommitCount,
+        Hash = Hash ^ other.Hash
+    };
+
+    internal bool HasSameCommits(ClientState other) => CommitCount == other.CommitCount && Hash == other.Hash;
 }
 
 public class ClientStateBuilder

src/SIL.Harmony.Tests/SIL.Harmony.Tests.csproj

@@ -13,6 +13,7 @@
         <PackageReference Include="FluentAssertions" />
         <PackageReference Include="GitHubActionsTestLogger" />
         <PackageReference Include="JetBrains.Profiler.SelfApi" />
+        <PackageReference Include="Microsoft.Extensions.Diagnostics.Testing" />
         <PackageReference Include="Verify.DiffPlex" />
         <PackageReference Include="Verify.EntityFramework" />
         <PackageReference Include="Verify.XunitV3" />

src/SIL.Harmony.Tests/Syncable/SyncRecoveryTests.cs

@@ -1,5 +1,9 @@
 using System.Runtime.CompilerServices;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Logging.Testing;
 using SIL.Harmony.Changes;
+using SIL.Harmony.Sample;
 using SIL.Harmony.Sample.Changes;
 
 namespace SIL.Harmony.Tests.Syncable;
@@ -221,6 +225,46 @@ public async Task Sync_ReportsBothErrors_WhenBothDirectionsFail()
             .Which.InnerExceptions.Should().HaveCount(2).And.AllBeOfType<NotSupportedException>();
     }
 
+    private static (SyncService, FakeLogCollector) SyncServiceWithFakeLogging()
+    {
+        var services = new ServiceCollection().AddCrdtDataSample(":memory:").AddFakeLogging().BuildServiceProvider();
+        return (services.GetRequiredService<SyncService>(), services.GetFakeLogCollector());
+    }
+
+    [Fact]
+    public async Task Sync_WarnsThatCommitsWillStillDiffer_UntilTheWithheldCreateArrives()
+    {
+        var (syncService, logs) = SyncServiceWithFakeLogging();
+        await using var local = await new DataModelSyncBackend().CreateAsync();
+        await using var remote = await new DataModelSyncBackend().CreateAsync();
+        await SetUpEditOnWithheldCreate(behind: remote, ahead: local);
+        var sync = () => syncService.SyncWith(local.Syncable, remote.Syncable);
+
+        await sync.Should().ThrowExactlyAsync<NotSupportedException>();
+        logs.GetSnapshot().Should().ContainSingle(r => r.Level == LogLevel.Warning)
+            .Which.Message.Should().StartWith("Commits from client");
+        logs.Clear();
+
+        await sync.Should().NotThrowAsync();
+        logs.GetSnapshot().Should().NotContain(r => r.Level >= LogLevel.Warning);
+    }
+
+    [Fact]
+    public async Task Sync_DoesNotWarn_WhenEachSideIsJustBehindOnADifferentClient()
+    {
+        var (syncService, logs) = SyncServiceWithFakeLogging();
+        await using var local = await new DataModelSyncBackend().CreateAsync();
+        await using var remote = await new DataModelSyncBackend().CreateAsync();
+        Guid localAhead = Guid.NewGuid(), remoteAhead = Guid.NewGuid();
+        Commit[] shared = [SetWordCommit("a1", localAhead, Date), SetWordCommit("b1", remoteAhead, Date)];
+        await local.Syncable.AddRangeFromSync([.. shared, SetWordCommit("a2", localAhead, Date.AddDays(1))]);
+        await remote.Syncable.AddRangeFromSync([.. shared, SetWordCommit("b2", remoteAhead, Date.AddDays(1))]);
+
+        await syncService.SyncWith(local.Syncable, remote.Syncable);
+
+        logs.GetSnapshot().Should().NotContain(r => r.Level >= LogLevel.Warning);
+    }
+
     [Fact(Skip = "Known sync blocker: each side withholds a create the other needs, so both directions throw on every sync. Revisit when we know more about missing commits.")]
     public async Task Sync_RecoversWithinTwoSyncs_WhenBothWithholdCreatesForOtherClientsEdits()
     {

src/SIL.Harmony/SyncService.cs

@@ -1,8 +1,10 @@
 using System.Runtime.ExceptionServices;
 using System.Text.Json;
+using Microsoft.Extensions.Logging;
+using static SIL.Harmony.Core.QueryHelpers;
 namespace SIL.Harmony;
 
-internal class SyncService(JsonSerializerOptions serializerOptions)
+internal class SyncService(JsonSerializerOptions serializerOptions, ILogger<SyncService> logger)
 {
     public async Task<SyncResults> SyncWithResourceUpload<TMetadata>(DataModel localModel,
         ISyncable remoteModel,
@@ -22,7 +24,8 @@ public async Task<SyncResults> SyncWith(ISyncable localModel, ISyncable remoteMo
         var localSyncState = await localModel.GetSyncState();
         var (missingFromLocal, remoteSyncState) = await remoteModel.GetChanges(localSyncState);
         //todo abort if local and remote heads are the same
-        var (missingFromRemote, _) = await localModel.GetChanges(remoteSyncState);
+        var (missingFromRemote, currentLocalSyncState) = await localModel.GetChanges(remoteSyncState);
+        await WarnIfCommitsWillStillDiffer(currentLocalSyncState, remoteSyncState, missingFromLocal, missingFromRemote);
         if (localModel is DataModel && remoteModel is DataModel)
         {
             //cloning just to simulate the objects going over the wire
@@ -82,6 +85,32 @@ public async Task SyncMany(ISyncable localModel, ISyncable[] remotes)
         }
     }
 
+    //a clean sync sends each side only commits it lacks, leaving local + pulled == remote + pushed; a mismatch means a side withheld commits.
+    //a side that sends all of a client's commits resends ones the other already has, so the sums don't apply and that client is skipped
+    private async Task WarnIfCommitsWillStillDiffer(SyncState local, SyncState remote, Commit[] pulled, Commit[] pushed)
+    {
+        if (!logger.IsEnabled(LogLevel.Warning)) return;
+        SyncState? pulledState = null, pushedState = null;
+        foreach (var localClient in local.ClientStates)
+        {
+            var remoteClient = remote.GetClientState(localClient.ClientId);
+            if (remoteClient is null || remoteClient.OnlyHasTimestamp || localClient == remoteClient) continue;
+            if (PlanFor(localClient, remoteClient) == SyncPlan.SendAll || PlanFor(remoteClient, localClient) == SyncPlan.SendAll) continue;
+            pulledState ??= await StateOf(pulled);
+            pushedState ??= await StateOf(pushed);
+            var localAfter = localClient.Plus(pulledState.GetClientState(localClient.ClientId));
+            var remoteAfter = remoteClient.Plus(pushedState.GetClientState(localClient.ClientId));
+            if (!localAfter.HasSameCommits(remoteAfter))
+                logger.LogWarning("Commits from client {ClientId} will still differ after this sync: local has {LocalCount} up to {LocalHead}, remote has {RemoteCount} up to {RemoteHead}",
+                    localClient.ClientId, localClient.CommitCount, Head(localClient), remoteClient.CommitCount, Head(remoteClient));
+        }
+    }
+
+    private static DateTimeOffset Head(ClientState state) => DateTimeOffset.FromUnixTimeMilliseconds(state.MaxTimestamp);
+
+    private static async Task<SyncState> StateOf(Commit[] commits) =>
+        new(await BuildSyncState(commits.Select(c => new SimpleCommit(c.ClientId, c.Id, c.HybridDateTime.DateTime)).ToAsyncEnumerable()));
+
     private static void ThrowIfAnyErrors(IEnumerable<Exception?> errors)
     {
         Exception[] thrown = [.. errors.OfType<Exception>()];