diff --git a/src/SIL.Harmony.Tests/DataModelSimpleChanges.WriteMultipleCommits.verified.txt b/src/SIL.Harmony.Tests/DataModelSimpleChanges.WriteMultipleCommits.verified.txt index a378761..bd705c5 100644 --- a/src/SIL.Harmony.Tests/DataModelSimpleChanges.WriteMultipleCommits.verified.txt +++ b/src/SIL.Harmony.Tests/DataModelSimpleChanges.WriteMultipleCommits.verified.txt @@ -19,6 +19,7 @@ ], Hash: Hash_1, ParentHash: Hash_Empty, + IsSnapshotCheckpoint: true, ChangeEntities: [ { $type: ChangeEntity, @@ -68,6 +69,7 @@ ], Hash: Hash_2, ParentHash: Hash_1, + IsSnapshotCheckpoint: true, ChangeEntities: [ { $type: ChangeEntity, diff --git a/src/SIL.Harmony.Tests/DataModelSimpleChanges.Writing2ChangesAtOnceWithMergedHistory.verified.txt b/src/SIL.Harmony.Tests/DataModelSimpleChanges.Writing2ChangesAtOnceWithMergedHistory.verified.txt index acd4e0b..12748f3 100644 --- a/src/SIL.Harmony.Tests/DataModelSimpleChanges.Writing2ChangesAtOnceWithMergedHistory.verified.txt +++ b/src/SIL.Harmony.Tests/DataModelSimpleChanges.Writing2ChangesAtOnceWithMergedHistory.verified.txt @@ -19,6 +19,7 @@ ], Hash: Hash_1, ParentHash: Hash_Empty, + IsSnapshotCheckpoint: true, ChangeEntities: [ { $type: ChangeEntity, @@ -69,6 +70,7 @@ ], Hash: Hash_2, ParentHash: Hash_1, + IsSnapshotCheckpoint: false, ChangeEntities: [ { $type: ChangeEntity, @@ -102,6 +104,7 @@ $type: Commit, Hash: Hash_3, ParentHash: Hash_2, + IsSnapshotCheckpoint: false, ChangeEntities: [ { $type: ChangeEntity, @@ -152,6 +155,7 @@ ], Hash: Hash_4, ParentHash: Hash_3, + IsSnapshotCheckpoint: true, ChangeEntities: [ { $type: ChangeEntity, diff --git a/src/SIL.Harmony.Tests/DataModelSimpleChanges.WritingA2ndChangeDoesNotEffectTheFirstSnapshot.verified.txt b/src/SIL.Harmony.Tests/DataModelSimpleChanges.WritingA2ndChangeDoesNotEffectTheFirstSnapshot.verified.txt index 1611fcb..de7b619 100644 --- a/src/SIL.Harmony.Tests/DataModelSimpleChanges.WritingA2ndChangeDoesNotEffectTheFirstSnapshot.verified.txt +++ b/src/SIL.Harmony.Tests/DataModelSimpleChanges.WritingA2ndChangeDoesNotEffectTheFirstSnapshot.verified.txt @@ -19,6 +19,7 @@ ], Hash: Hash_1, ParentHash: Hash_Empty, + IsSnapshotCheckpoint: true, ChangeEntities: [ { $type: ChangeEntity, @@ -68,6 +69,7 @@ ], Hash: Hash_2, ParentHash: Hash_1, + IsSnapshotCheckpoint: true, ChangeEntities: [ { $type: ChangeEntity, diff --git a/src/SIL.Harmony.Tests/DataModelSimpleChanges.WritingAChangeMakesASnapshot.verified.txt b/src/SIL.Harmony.Tests/DataModelSimpleChanges.WritingAChangeMakesASnapshot.verified.txt index 5db6e99..cef91dd 100644 --- a/src/SIL.Harmony.Tests/DataModelSimpleChanges.WritingAChangeMakesASnapshot.verified.txt +++ b/src/SIL.Harmony.Tests/DataModelSimpleChanges.WritingAChangeMakesASnapshot.verified.txt @@ -19,6 +19,7 @@ ], Hash: Hash_1, ParentHash: Hash_Empty, + IsSnapshotCheckpoint: true, ChangeEntities: [ { $type: ChangeEntity, diff --git a/src/SIL.Harmony.Tests/DataModelSimpleChanges.WritingACommitWithMultipleChangesWorks.verified.txt b/src/SIL.Harmony.Tests/DataModelSimpleChanges.WritingACommitWithMultipleChangesWorks.verified.txt index d69bdc5..2540901 100644 --- a/src/SIL.Harmony.Tests/DataModelSimpleChanges.WritingACommitWithMultipleChangesWorks.verified.txt +++ b/src/SIL.Harmony.Tests/DataModelSimpleChanges.WritingACommitWithMultipleChangesWorks.verified.txt @@ -33,6 +33,7 @@ ], Hash: Hash_1, ParentHash: Hash_Empty, + IsSnapshotCheckpoint: true, ChangeEntities: [ { $type: ChangeEntity, diff --git a/src/SIL.Harmony.Tests/DataModelTestBase.cs b/src/SIL.Harmony.Tests/DataModelTestBase.cs index 8dcf44f..4b217dc 100644 --- a/src/SIL.Harmony.Tests/DataModelTestBase.cs +++ b/src/SIL.Harmony.Tests/DataModelTestBase.cs @@ -56,11 +56,11 @@ public DataModelTestBase ForkDatabase(bool alwaysValidate = true) if (DbContext.Database.GetDbConnection() is not SqliteConnection existingConnection) throw new InvalidOperationException("Database is not SQLite"); existingConnection.BackupDatabase(connection); var newTestBase = new DataModelTestBase(connection, alwaysValidate, performanceTest: _performanceTest); - newTestBase.SetCurrentDate(currentDate.DateTime); + newTestBase.SetCurrentDate(currentDate); return newTestBase; } - public void SetCurrentDate(DateTime dateTime) + public void SetCurrentDate(DateTimeOffset dateTime) { currentDate = dateTime; } diff --git a/src/SIL.Harmony.Tests/DbContextTests.VerifyModel.verified.txt b/src/SIL.Harmony.Tests/DbContextTests.VerifyModel.verified.txt index 3a780f3..9fbe310 100644 --- a/src/SIL.Harmony.Tests/DbContextTests.VerifyModel.verified.txt +++ b/src/SIL.Harmony.Tests/DbContextTests.VerifyModel.verified.txt @@ -4,6 +4,7 @@ Id (Guid) Required PK AfterSave:Throw ValueGenerated.OnAdd ClientId (Guid) Required Hash (string) Required + IsSnapshotCheckpoint (bool) Required Metadata (CommitMetadata) Required Annotations: Relational:ColumnType: jsonb diff --git a/src/SIL.Harmony.Tests/DbContextTests.cs b/src/SIL.Harmony.Tests/DbContextTests.cs index 8733439..cc3df5b 100644 --- a/src/SIL.Harmony.Tests/DbContextTests.cs +++ b/src/SIL.Harmony.Tests/DbContextTests.cs @@ -53,6 +53,7 @@ await DbContext.Set().ToLinqToDBTable().AsValueInsertable() .Value(c => c.Metadata, new CommitMetadata()) .Value(c => c.Hash, "") .Value(c => c.ParentHash, "") + .Value(c => c.IsSnapshotCheckpoint, false) .InsertAsync(TestContext.Current.CancellationToken); var actualCommit = await DbContext.Commits.SingleOrDefaultAsyncEF(c => c.Id == commitId, TestContext.Current.CancellationToken); actualCommit!.HybridDateTime.DateTime.Should().Be(expectedDateTime, "EF"); diff --git a/src/SIL.Harmony.Tests/LateCommitTests.cs b/src/SIL.Harmony.Tests/LateCommitTests.cs new file mode 100644 index 0000000..95bad98 --- /dev/null +++ b/src/SIL.Harmony.Tests/LateCommitTests.cs @@ -0,0 +1,92 @@ +using SIL.Harmony.Sample.Changes; +using SIL.Harmony.Sample.Models; + +namespace SIL.Harmony.Tests; + +/// +/// A commit dated before commits a replica already holds makes it roll its snapshots back and replay history. +/// These tests sync edits to the replica in one batch, which prunes some of their snapshots, and then land a late commit in the gap. +/// +public class LateCommitTests : IAsyncLifetime +{ + private readonly DataModelTestBase _author = new(); + private readonly DataModelTestBase _replica = new(); + private readonly DataModelTestBase _offlineClient = new(); + + public async ValueTask InitializeAsync() + { + await _author.InitializeAsync(); + await _replica.InitializeAsync(); + await _offlineClient.InitializeAsync(); + } + + public async ValueTask DisposeAsync() + { + await _author.DisposeAsync(); + await _replica.DisposeAsync(); + await _offlineClient.DisposeAsync(); + } + + // which of the edits' snapshots the batch prunes depends on their position in it, so land the late commit after each edit + [Theory] + [InlineData(0)] + [InlineData(1)] + public async Task EditsSyncedInOneBatchSurviveALateCommitBetweenThem(int lateCommitAfterEdit) + { + var wordId = Guid.NewGuid(); + var antonymId = Guid.NewGuid(); + await _author.WriteNextChange([_author.SetWord(wordId, "word"), _author.SetWord(antonymId, "antonym")]); + Commit[] edits = + [ + await _author.WriteNextChange(new SetWordNoteChange(wordId, "a note")), + await _author.WriteNextChange(new SetAntonymReferenceChange(wordId, antonymId)), + await _author.WriteNextChange(new SetWordTextChange(wordId, "renamed word")), + ]; + await _replica.DataModel.SyncWith(_author.DataModel); + + // another client wrote an unrelated word while offline, dated between two of the edits + await _offlineClient.WriteChangeAfter(edits[lateCommitAfterEdit], _offlineClient.SetWord(Guid.NewGuid(), "written offline")); + await _replica.DataModel.SyncWith(_offlineClient.DataModel); + + var replicaWord = await _replica.DataModel.GetLatest(wordId); + var authorWord = await _author.DataModel.GetLatest(wordId); + replicaWord.Should().BeEquivalentTo(authorWord); + } + + // whether the replay keeps the definition's cascade-delete snapshot depends on the edit's position in the batch, + // which the size of the backlog before the delete shifts by one + [Theory] + [InlineData(1)] + [InlineData(2)] + public async Task CascadeDeleteSurvivesALateCommitAfterIt(int backlogSize) + { + var wordId = Guid.NewGuid(); + var definitionId = Guid.NewGuid(); + await _author.WriteNextChange(_author.SetWord(wordId, "word")); + await _author.WriteNextChange(_author.NewDefinition(wordId, "a definition", "noun", definitionId: definitionId)); + await _replica.DataModel.SyncWith(_author.DataModel); + await _offlineClient.DataModel.SyncWith(_author.DataModel); + + // deleting the word deletes its definition too, but no commit records that: it only exists as a snapshot + var delete = await _author.WriteNextChange(_author.DeleteWord(wordId)); + await _replica.DataModel.SyncWith(_author.DataModel); + + // meanwhile the offline client wrote some words before the delete happened... + var backlog = delete; + for (var i = 0; i < backlogSize; i++) + { + backlog = await _offlineClient.WriteChangeBefore(backlog, _offlineClient.SetWord(Guid.NewGuid(), $"offline word {i}")); + } + // ...and, never having received the delete, edits the definition after it + _offlineClient.SetCurrentDate(delete.DateTime); + var edit = await _offlineClient.WriteNextChange(new SetDefinitionPartOfSpeechChange(definitionId, "verb")); + await _replica.DataModel.SyncWith(_offlineClient.DataModel); + (await _replica.DataModel.GetLatest(definitionId))!.DeletedAt.Should().NotBeNull(); + + // the author writes another word, dated between the delete and the edit it doesn't know about yet + await _author.WriteChangeBefore(edit, _author.SetWord(Guid.NewGuid(), "another word")); + await _replica.DataModel.SyncWith(_author.DataModel); + + (await _replica.DataModel.GetLatest(definitionId))!.DeletedAt.Should().NotBeNull(); + } +} diff --git a/src/SIL.Harmony.Tests/RepositoryTests.cs b/src/SIL.Harmony.Tests/RepositoryTests.cs index 3ad69d3..c6334da 100644 --- a/src/SIL.Harmony.Tests/RepositoryTests.cs +++ b/src/SIL.Harmony.Tests/RepositoryTests.cs @@ -245,16 +245,37 @@ await _repository.AddSnapshots([ } [Fact] - public async Task DeleteStaleSnapshots_WithNoSnapshots_DoesNothing() + public async Task DeleteSnapshotsAfter_WithNoSnapshots_DoesNothing() { - //the empty-repository branch: nothing to delete, must not throw - await _repository.DeleteStaleSnapshots(Commit(Guid.NewGuid(), Time(1, 0))); + await _repository.DeleteSnapshotsAfter(Commit(Guid.NewGuid(), Time(1, 0))); _crdtDbContext.Snapshots.Should().BeEmpty(); } [Fact] - public async Task DeleteStaleSnapshots_KeepsSnapshotsOlderThanTheCommit() + public async Task DeleteSnapshotsAfter_Null_DeletesEverySnapshot() + { + await _repository.AddSnapshots([ + Snapshot(Guid.NewGuid(), Guid.NewGuid(), Time(1, 0)), + Snapshot(Guid.NewGuid(), Guid.NewGuid(), Time(2, 0)), + ]); + + await _repository.DeleteSnapshotsAfter(null); + + _crdtDbContext.Snapshots.Should().BeEmpty(); + } + + [Fact] + public async Task HasSnapshotsAfter_ComparesTheFullCommitOrder() + { + await _repository.AddSnapshots([Snapshot(Guid.NewGuid(), Guid.NewGuid(), Time(1, 1))]); + + (await _repository.HasSnapshotsAfter(Commit(Guid.NewGuid(), Time(1, 0)))).Should().BeTrue(); + (await _repository.HasSnapshotsAfter(Commit(Guid.NewGuid(), Time(1, 2)))).Should().BeFalse(); + } + + [Fact] + public async Task DeleteSnapshotsAfter_KeepsSnapshotsOlderThanTheCommit() { await _repository.AddSnapshots([ Snapshot(Guid.NewGuid(), Guid.NewGuid(), Time(1, 0)), @@ -262,39 +283,39 @@ await _repository.AddSnapshots([ ]); //the new commit is newer than every existing snapshot, so none are stale - await _repository.DeleteStaleSnapshots(Commit(Guid.NewGuid(), Time(3, 0))); + await _repository.DeleteSnapshotsAfter(Commit(Guid.NewGuid(), Time(3, 0))); _crdtDbContext.Snapshots.Should().HaveCount(2); } [Fact] - public async Task DeleteStaleSnapshots_DeletesSnapshotsAfterCommitByTime() + public async Task DeleteSnapshotsAfter_DeletesSnapshotsAfterCommitByTime() { await _repository.AddSnapshots([ Snapshot(Guid.NewGuid(), Guid.NewGuid(), Time(1, 0)), Snapshot(Guid.NewGuid(), Guid.NewGuid(), Time(3, 0)), ]); - await _repository.DeleteStaleSnapshots(Commit(Guid.NewGuid(), Time(2, 0))); + await _repository.DeleteSnapshotsAfter(Commit(Guid.NewGuid(), Time(2, 0))); _crdtDbContext.Snapshots.Include(s => s.Commit).Should().ContainSingle() .Which.Commit.HybridDateTime.DateTime.Hour.Should().Be(1); } [Fact] - public async Task DeleteStaleSnapshots_DeletesSnapshotsAfterCommitByCount() + public async Task DeleteSnapshotsAfter_DeletesSnapshotsAfterCommitByCount() { await _repository.AddSnapshots([ Snapshot(Guid.NewGuid(), Guid.NewGuid(), Time(1, 0)), Snapshot(Guid.NewGuid(), Guid.NewGuid(), Time(1, 2)), ]); - await _repository.DeleteStaleSnapshots(Commit(Guid.NewGuid(), Time(1, 1))); + await _repository.DeleteSnapshotsAfter(Commit(Guid.NewGuid(), Time(1, 1))); _crdtDbContext.Snapshots.Include(s => s.Commit).Should().ContainSingle() .Which.Commit.HybridDateTime.Counter.Should().Be(0); } [Fact] - public async Task DeleteStaleSnapshots_DeletesSnapshotsAfterCommitByCommitId() + public async Task DeleteSnapshotsAfter_DeletesSnapshotsAfterCommitByCommitId() { var time = Time(1, 1); var entityId = Guid.NewGuid(); @@ -303,7 +324,7 @@ await _repository.AddSnapshots([ Snapshot(entityId, ids[0], time), Snapshot(entityId, ids[2], time), ]); - await _repository.DeleteStaleSnapshots(Commit(ids[1], time)); + await _repository.DeleteSnapshotsAfter(Commit(ids[1], time)); _crdtDbContext.Snapshots.Should().ContainSingle() .Which.CommitId.Should().Be(ids[0]); diff --git a/src/SIL.Harmony.Tests/SnapshotCheckpointTests.cs b/src/SIL.Harmony.Tests/SnapshotCheckpointTests.cs new file mode 100644 index 0000000..e49687c --- /dev/null +++ b/src/SIL.Harmony.Tests/SnapshotCheckpointTests.cs @@ -0,0 +1,67 @@ +using Microsoft.EntityFrameworkCore; + +namespace SIL.Harmony.Tests; + +public class SnapshotCheckpointTests : DataModelTestBase +{ + private async Task CheckpointIds() + { + return await DbContext.Commits.AsNoTracking() + .Where(c => c.IsSnapshotCheckpoint) + .Select(c => c.Id) + .ToArrayAsync(TestContext.Current.CancellationToken); + } + + [Fact] + public async Task EveryLocallyAuthoredCommitIsACheckpoint() + { + var entityId = Guid.NewGuid(); + var commit1 = await WriteNextChange(SetWord(entityId, "first")); + var commit2 = await WriteNextChange(SetWord(entityId, "second")); + + (await CheckpointIds()).Should().BeEquivalentTo([commit1.Id, commit2.Id]); + } + + [Fact] + public async Task ASyncedBatchOnlyMakesItsLastCommitACheckpoint() + { + var entityId = Guid.NewGuid(); + var commits = new[] + { + await WriteNextChange(SetWord(entityId, "first"), add: false), + await WriteNextChange(SetWord(entityId, "second"), add: false), + await WriteNextChange(SetWord(entityId, "third"), add: false), + }; + + await AddCommitsViaSync(commits); + + (await CheckpointIds()).Should().BeEquivalentTo([commits[2].Id]); + } + + [Fact] + public async Task ALateCommitClearsTheCheckpointsItReplays() + { + var entityId = Guid.NewGuid(); + var commit1 = await WriteNextChange(SetWord(entityId, "first")); + var commit2 = await WriteNextChange(SetWord(entityId, "second")); + var commit3 = await WriteNextChange(SetWord(entityId, "third")); + + var lateCommit = await WriteChangeBefore(commit2, SetWord(Guid.NewGuid(), "late")); + + //the replay resumed from commit1 and ran through commit3, so only its last commit is a checkpoint again + (await CheckpointIds()).Should().BeEquivalentTo([commit1.Id, commit3.Id]); + lateCommit.IsSnapshotCheckpoint.Should().BeFalse(); + } + + [Fact] + public async Task RegeneratingSnapshotsLeavesOnlyTheLastCommitAsACheckpoint() + { + var entityId = Guid.NewGuid(); + await WriteNextChange(SetWord(entityId, "first")); + var lastCommit = await WriteNextChange(SetWord(entityId, "second")); + + await DataModel.RegenerateSnapshots(); + + (await CheckpointIds()).Should().BeEquivalentTo([lastCommit.Id]); + } +} diff --git a/src/SIL.Harmony/Commit.cs b/src/SIL.Harmony/Commit.cs index 53eebbc..4db2946 100644 --- a/src/SIL.Harmony/Commit.cs +++ b/src/SIL.Harmony/Commit.cs @@ -40,4 +40,13 @@ internal Commit() : this(Guid.NewGuid()) [JsonIgnore] public string ParentHash { get; private set; } + + /// + /// Snapshots are complete as of this commit: every entity's newest snapshot at or before it is that entity's state after it, + /// except for changes this client could not apply (see ), which only folds in. + /// A commit that arrives out of order rolls snapshots back to the newest checkpoint before it and replays from there. + /// Local bookkeeping, never synced. + /// + [JsonIgnore] + public bool IsSnapshotCheckpoint { get; internal set; } } diff --git a/src/SIL.Harmony/DataModel.cs b/src/SIL.Harmony/DataModel.cs index 01e5219..beaa39b 100644 --- a/src/SIL.Harmony/DataModel.cs +++ b/src/SIL.Harmony/DataModel.cs @@ -191,7 +191,14 @@ private async Task UpdateSnapshots(CrdtRepository repo, SortedSet commit { if (commitsToApply.Count == 0) return; var oldestAddedCommit = commitsToApply.First(); - await repo.DeleteStaleSnapshots(oldestAddedCommit); + if (await repo.HasSnapshotsAfter(oldestAddedCommit)) + { + // rolling back to the new commit itself is not enough: an entity's newest surviving snapshot may predate edits whose snapshots were pruned + var checkpoint = await repo.FindSnapshotCheckpointBefore(oldestAddedCommit); + await repo.DeleteSnapshotsAfter(checkpoint); + commitsToApply = (await repo.GetCommitsAfter(checkpoint)).ToSortedSet(); + } + await repo.SetSnapshotCheckpoint(commitsToApply); Dictionary snapshotLookup = []; if (commitsToApply.Count > 10) { @@ -234,12 +241,15 @@ private async Task ValidateCommits(CrdtRepository repo) public async Task RegenerateSnapshots() { await using var repo = await _crdtRepositoryFactory.CreateRepository(); - await repo.DeleteSnapshotsAndProjectedTables(); + using var locked = await repo.Lock(); repo.ClearChangeTracker(); + await using var transaction = await repo.BeginTransactionAsync(); + await repo.DeleteSnapshotsAndProjectedTables(); var allCommits = await repo.CurrentCommits() .Include(c => c.ChangeEntities) .ToSortedSetAsync(); await UpdateSnapshots(repo, allCommits); + await transaction.CommitAsync(); } public async Task GetLatestSnapshotByObjectId(Guid entityId) diff --git a/src/SIL.Harmony/Db/CrdtRepository.cs b/src/SIL.Harmony/Db/CrdtRepository.cs index d7a8a10..feef923 100644 --- a/src/SIL.Harmony/Db/CrdtRepository.cs +++ b/src/SIL.Harmony/Db/CrdtRepository.cs @@ -127,15 +127,38 @@ public async Task HasCommit(Guid commitId) return (oldestChange, newCommits); } - public async Task DeleteStaleSnapshots(Commit oldestChange) - { - //use the oldest commit added to clear any snapshots that are based on a now incomplete history - //this is a performance optimization to avoid deleting snapshots where there are none to delete - var mostRecentCommit = await Snapshots.MaxAsync(s => (DateTimeOffset?)s.Commit.HybridDateTime.DateTime); - if (mostRecentCommit < oldestChange.HybridDateTime.DateTime) return; - await Snapshots - .WhereAfter(oldestChange) - .ExecuteDeleteAsync(); + public async Task HasSnapshotsAfter(Commit commit) + { + return await Snapshots.WhereAfter(commit).AnyAsync(); + } + + /// null deletes every snapshot + public async Task DeleteSnapshotsAfter(Commit? commit) + { + var snapshots = commit is null ? Snapshots : Snapshots.WhereAfter(commit); + await snapshots.ExecuteDeleteAsync(); + } + + public async Task FindSnapshotCheckpointBefore(Commit commit) + { + return await Commits + .Where(c => c.IsSnapshotCheckpoint) + .WhereBefore(commit) + .DefaultOrderDescending() + .FirstOrDefaultAsync(); + } + + /// + /// the last replayed commit becomes the checkpoint; the batch may have pruned snapshots inside it, so the others stop being one + /// + public async Task SetSnapshotCheckpoint(SortedSet replayedCommits) + { + foreach (var commit in replayedCommits) + { + commit.IsSnapshotCheckpoint = false; + } + replayedCommits.Max!.IsSnapshotCheckpoint = true; + await _dbContext.SaveChangesAsync(); } public async Task DeleteSnapshotsAndProjectedTables()