diff --git a/Directory.Packages.props b/Directory.Packages.props index 804c881..1ae9d4f 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -18,6 +18,7 @@ + diff --git a/src/SIL.Harmony.Core/QueryHelpers.cs b/src/SIL.Harmony.Core/QueryHelpers.cs index d4294c4..75be189 100644 --- a/src/SIL.Harmony.Core/QueryHelpers.cs +++ b/src/SIL.Harmony.Core/QueryHelpers.cs @@ -72,14 +72,14 @@ public static async IAsyncEnumerable GetCommitsMissingFromRemote 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 diff --git a/src/SIL.Harmony.Tests/SIL.Harmony.Tests.csproj b/src/SIL.Harmony.Tests/SIL.Harmony.Tests.csproj index 5aa5871..d68a0f7 100644 --- a/src/SIL.Harmony.Tests/SIL.Harmony.Tests.csproj +++ b/src/SIL.Harmony.Tests/SIL.Harmony.Tests.csproj @@ -13,6 +13,7 @@ + diff --git a/src/SIL.Harmony.Tests/Syncable/SyncRecoveryTests.cs b/src/SIL.Harmony.Tests/Syncable/SyncRecoveryTests.cs index 3896e6f..b1699f7 100644 --- a/src/SIL.Harmony.Tests/Syncable/SyncRecoveryTests.cs +++ b/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(); } + private static (SyncService, FakeLogCollector) SyncServiceWithFakeLogging() + { + var services = new ServiceCollection().AddCrdtDataSample(":memory:").AddFakeLogging().BuildServiceProvider(); + return (services.GetRequiredService(), 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(); + 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() { diff --git a/src/SIL.Harmony/SyncService.cs b/src/SIL.Harmony/SyncService.cs index ad2d377..c087fa9 100644 --- a/src/SIL.Harmony/SyncService.cs +++ b/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 logger) { public async Task SyncWithResourceUpload(DataModel localModel, ISyncable remoteModel, @@ -22,7 +24,8 @@ public async Task 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 StateOf(Commit[] commits) => + new(await BuildSyncState(commits.Select(c => new SimpleCommit(c.ClientId, c.Id, c.HybridDateTime.DateTime)).ToAsyncEnumerable())); + private static void ThrowIfAnyErrors(IEnumerable errors) { Exception[] thrown = [.. errors.OfType()];