diff --git a/src/DodoSSH.Client.Sync/VaultItemRepository.cs b/src/DodoSSH.Client.Sync/VaultItemRepository.cs index ee808da..e64cc42 100644 --- a/src/DodoSSH.Client.Sync/VaultItemRepository.cs +++ b/src/DodoSSH.Client.Sync/VaultItemRepository.cs @@ -210,8 +210,14 @@ internal sealed class VaultItemRepository( /// Deletes an item. /// /// + /// /// Queued as a tombstone, never a local removal. An offline client that simply forgot the row would /// be unable to tell the server anything, and the item would come back on the next pull. + /// + /// + /// Unless the server has never heard of the item, which is the one case where a tombstone is not only + /// unnecessary but wrong — see . + /// /// internal async Task DeleteAsync(Guid vaultId, Guid entityId, CancellationToken cancellationToken) { @@ -219,6 +225,12 @@ internal sealed class VaultItemRepository( .FindAsync(vaultId, kind.EntityType, entityId, cancellationToken) .ConfigureAwait(false); + if (pending is not null && NeverReachedTheServer(pending)) + { + await outbox.CompleteAsync(pending.Sequence, cancellationToken).ConfigureAwait(false); + return; + } + var expectedVersion = pending is not null ? pending.ExpectedVersion : await MirrorVersionAsync(vaultId, entityId, cancellationToken).ConfigureAwait(false); @@ -239,6 +251,32 @@ internal sealed class VaultItemRepository( cancellationToken).ConfigureAwait(false); } + /// + /// Whether a queued change describes an item the server cannot be holding. + /// + /// + /// + /// A null ExpectedVersion means the row is a create — including a create that has since been + /// edited, because coalescing keeps the original expected version. So there is no server row and no + /// mirror row, and dropping the queued change makes the item genuinely gone. Queueing a tombstone + /// instead asks the server to delete something it has never seen, which it answers Invalid; the + /// change is parked, and the user is left with a rejected item they already deleted and a pending count + /// that never reaches zero. Add a host on a laptop with no network, change your mind, and that is the + /// state — it applies to all four item types. + /// + /// + /// The attempt count is what makes this safe rather than merely convenient. Nothing sent cannot have + /// landed. A parked row cannot have landed either — parking is what the pusher does when the server has + /// refused, so the refusal is the evidence. What is left is a create that went out and whose answer was + /// never seen: in flight, or failed in a way that might yet have been applied. That one still gets a + /// tombstone, because the server may be holding the item and a local drop would strand it there for + /// ever. A refused tombstone is recoverable; an orphan on the server is not. + /// + /// + private static bool NeverReachedTheServer(PendingOperation pending) => + pending is { Operation: SyncOperation.Upsert, ExpectedVersion: null } + && (pending.Attempts == 0 || pending.IsParked); + private static void Validate(TSecret secret) { if (!secret.TryValidate(out var error)) diff --git a/tests/DodoSSH.Client.Sync.Tests/UnpushedDeleteTests.cs b/tests/DodoSSH.Client.Sync.Tests/UnpushedDeleteTests.cs new file mode 100644 index 0000000..6c06e7b --- /dev/null +++ b/tests/DodoSSH.Client.Sync.Tests/UnpushedDeleteTests.cs @@ -0,0 +1,201 @@ +using DodoSSH.Contracts; +using static DodoSSH.Client.Sync.Tests.SyncHarness; + +namespace DodoSSH.Client.Sync.Tests; + +/// +/// Deleting something the server has never heard of. +/// +/// +/// +/// Every delete used to queue a tombstone, which is right for an item the server holds and wrong for one it +/// does not: it answers Invalid, the change is parked, and the user is left looking at a rejected +/// change for an item they already deleted and a pending count that never reaches zero. Adding a host on a +/// laptop with no network and changing your mind is enough to produce it. +/// +/// +/// The interesting half of these tests is the other direction — that the shortcut does not swallow a real +/// delete. A repository that quietly dropped tombstones would pass a test suite written only around the bug +/// and would lose data on every machine but the one that pressed the button. +/// +/// +public sealed class UnpushedDeleteTests +{ + private static CancellationToken Token => TestContext.Current.CancellationToken; + + [Fact] + public async Task CreatingAndDeletingBeforeAnySync_QueuesNothingAndPushesNothing() + { + using var harness = await CreateAsync(); + var laptop = harness.First; + + var entityId = await laptop.CreateAsync(Host("scratch")); + await laptop.DeleteAsync(entityId); + + (await laptop.Outbox.ListAllAsync(VaultId, Token)).ShouldBeEmpty(); + (await laptop.ListAsync()).Items.ShouldBeEmpty(); + + var report = await laptop.SyncAsync(); + + report.Parked.ShouldBe(0, "there is nothing for the server to refuse"); + report.Pushed.ShouldBe(0); + harness.Server.RowCount.ShouldBe(0); + } + + [Fact] + public async Task CreatingEditingAndDeletingBeforeAnySync_AlsoQueuesNothing() + { + // Coalescing keeps the original expected version, so a create that has since been edited is still a + // create. Reading the operation alone would see an Upsert with an ancestor and take it for an edit. + using var harness = await CreateAsync(); + var laptop = harness.First; + + var entityId = await laptop.CreateAsync(Host("scratch")); + await laptop.UpdateAsync(entityId, Host("scratch", port: 2222)); + await laptop.DeleteAsync(entityId); + + (await laptop.Outbox.ListAllAsync(VaultId, Token)).ShouldBeEmpty(); + } + + [Fact] + public async Task ACreateTheServerRefused_StopsBeingParkedWhenItIsDeleted() + { + // Parking is the pusher's record that the server said no, so a parked create is one the server + // provably does not hold. Before this, deleting it replaced a parked create with a tombstone that + // would be parked in its turn — the item could not be got rid of at all. + using var harness = await CreateAsync(); + var laptop = harness.First; + + harness.Server.DenyWrites = true; + + var entityId = await laptop.CreateAsync(Host("scratch")); + var refused = await laptop.SyncAsync(); + + refused.Parked.ShouldBe(1); + harness.Server.RowCount.ShouldBe(0); + + harness.Server.DenyWrites = false; + await laptop.DeleteAsync(entityId); + + (await laptop.Outbox.ListAllAsync(VaultId, Token)).ShouldBeEmpty(); + + var after = await laptop.SyncAsync(); + after.Parked.ShouldBe(0); + } + + [Fact] + public async Task DeletingAnItemTheServerHolds_StillQueuesATombstone() + { + // The guard on the whole change. A shortcut that fired here would delete the item on this machine + // and nowhere else, and the next pull would bring it back. + using var harness = await CreateAsync(); + var laptop = harness.First; + + var entityId = await laptop.CreateAsync(Host("prod-db")); + await laptop.SyncAsync(); + + harness.Server.RowCount.ShouldBe(1); + + await laptop.DeleteAsync(entityId); + + var queued = (await laptop.Outbox.ListAllAsync(VaultId, Token)).ShouldHaveSingleItem(); + queued.Operation.ShouldBe(SyncOperation.Delete); + + await laptop.SyncAsync(); + + harness.Server.RowCount.ShouldBe(0); + (await laptop.ListAsync()).Items.ShouldBeEmpty(); + } + + /// + /// The case the obvious test misses, and the one that would lose data. After a sync there is no queued + /// row at all, so deleting a synced item never reaches the shortcut and proves nothing about it; the way + /// to hold an unpushed Upsert over an item the server holds is to edit it offline. Dropping that + /// row would delete the item here, leave it on the server, and bring it back on the next pull. Found by + /// mutation — removing the expected-version guard left every other test in this file passing. + /// + [Fact] + public async Task EditingASyncedItemOfflineAndThenDeletingIt_StillQueuesATombstone() + { + using var harness = await CreateAsync(); + var laptop = harness.First; + + var entityId = await laptop.CreateAsync(Host("prod-db")); + await laptop.SyncAsync(); + + // An unpushed edit: a queued Upsert whose expected version says the server already has this. + await laptop.UpdateAsync(entityId, Host("prod-db", port: 2222)); + + var edit = (await laptop.Outbox.ListAllAsync(VaultId, Token)).ShouldHaveSingleItem(); + edit.Operation.ShouldBe(SyncOperation.Upsert); + edit.ExpectedVersion.ShouldNotBeNull(); + + await laptop.DeleteAsync(entityId); + + var tombstone = (await laptop.Outbox.ListAllAsync(VaultId, Token)).ShouldHaveSingleItem(); + tombstone.Operation.ShouldBe(SyncOperation.Delete); + + await laptop.SyncAsync(); + + harness.Server.RowCount.ShouldBe(0, "the deletion has to reach the server, not just this machine"); + (await laptop.ListAsync()).Items.ShouldBeEmpty(); + } + + [Fact] + public async Task ACreateThatWentOutAndWasNeverAnswered_StillQueuesATombstone() + { + // The ambiguous case, and the reason the attempt count is part of the test rather than just the + // expected version. A create that was dispatched may have been applied by a server whose answer never + // arrived. Dropping it locally would strand a row nobody can see and nobody can delete; a tombstone + // that turns out to be unnecessary is refused once and can be dealt with. + using var harness = await CreateAsync(); + var laptop = harness.First; + + var entityId = await laptop.CreateAsync(Host("in-flight")); + + var queued = (await laptop.Outbox.ListAllAsync(VaultId, Token)).ShouldHaveSingleItem(); + + // Sent, with the answer lost. Marked directly because the engine does this immediately before the + // call it may not survive, and reproducing a dropped response through the fake server would test the + // fake rather than the repository. + await laptop.Outbox.MarkDispatchedAsync(queued.Sequence, Token); + + await laptop.DeleteAsync(entityId); + + var tombstone = (await laptop.Outbox.ListAllAsync(VaultId, Token)).ShouldHaveSingleItem(); + tombstone.Operation.ShouldBe(SyncOperation.Delete); + } + + /// + /// The path most likely to meet this in practice, which is why it gets its own test rather than trusting + /// the generic repository: trust is pinned by connecting and withdrawn from the host editor, so trusting a + /// rebuilt server and then withdrawing it before the next sync is a minute's work rather than a contrived + /// sequence. It is the same code as the host case and would be caught by the same mutation — what this + /// pins is that the generic repository really is what all four types go through. + /// + [Fact] + public async Task TheSameHoldsForAPinTrustedAndForgottenOffline() + { + using var harness = await CreateAsync(); + var laptop = harness.First; + + var entityId = await laptop.CreateKnownHostAsync(KnownHost()); + await laptop.DeleteKnownHostAsync(entityId); + + (await laptop.Outbox.ListAllAsync(VaultId, Token)).ShouldBeEmpty(); + (await laptop.ListKnownHostsAsync()).Items.ShouldBeEmpty(); + } + + [Fact] + public async Task TheSameHoldsForAKeyAddedAndRemovedOffline() + { + using var harness = await CreateAsync(); + var laptop = harness.First; + + var entityId = await laptop.CreateKeyAsync(Key("scratch")); + await laptop.DeleteKeyAsync(entityId); + + (await laptop.Outbox.ListAllAsync(VaultId, Token)).ShouldBeEmpty(); + (await laptop.ListKeysAsync()).Items.ShouldBeEmpty(); + } +}